Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Authorize requests #301

Merged
merged 37 commits into from
Jul 19, 2024
Merged

Authorize requests #301

merged 37 commits into from
Jul 19, 2024

Conversation

christoph-hamm
Copy link
Contributor

@christoph-hamm christoph-hamm commented Jun 21, 2024 •
edited by inf17101
Loading

Issues: #22

Definition of Done

The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.

inf17101
inf17101 reviewed Jul 3, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am almost done with the review. I will review the last four files tomorrow. Here is the first part of the comments.

agent/src/control_interface/authorizer.rs Outdated Show resolved Hide resolved
agent/src/control_interface/authorizer.rs Outdated Show resolved Hide resolved
server/src/ankaios_server.rs Show resolved Hide resolved
grpc/proto/grpc_api.proto Outdated Show resolved Hide resolved
common/src/objects/control_interface_access.rs Show resolved Hide resolved
api/proto/ank_base.proto Outdated Show resolved Hide resolved
api/proto/ank_base.proto Outdated Show resolved Hide resolved
common/src/objects/workload_states_map.rs Outdated Show resolved Hide resolved
common/src/objects/workload_states_map.rs Outdated Show resolved Hide resolved
agent/src/control_interface/pipes_channel_context_info.rs Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor

inf17101 commented Jul 3, 2024

@christoph-hamm: When specifying an allowRule but no denyRule the yaml parser forces to define an empty "denyRule" in the config. I would have expect that I can leave away the denyRule and internally it is treated as empty.

inf17101
inf17101 reviewed Jul 4, 2024
View reviewed changes
agent/src/control_interface/authorizer.rs Outdated Show resolved Hide resolved
agent/src/control_interface/authorizer.rs Outdated Show resolved Hide resolved
agent/src/control_interface/authorizer.rs Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor

inf17101 commented Jul 4, 2024 •
edited
Loading

@krucod3 , @christoph-hamm: I think we need a user documentation for this, right?

@inf17101
Copy link
Contributor

inf17101 commented Jul 4, 2024 •
edited
Loading

@krucod3, @christoph-hamm: I have updated the control interface examples and pushed. The only restriction I had is that I get the whole CompleteState for now when requesting the workload states because the filtering is not ready yet, but I know that @krucod3 is working already on that. So, I need to test the examples again if the filtering is in. So please let me know.

inf17101 and others added 4 commits July 4, 2024 15:51
@inf17101
Copy link
Contributor

inf17101 commented Jul 9, 2024

@christoph-hamm: When specifying an allowRule but no denyRule the yaml parser forces to define an empty "denyRule" in the config. I would have expect that I can leave away the denyRule and internally it is treated as empty.

I have changed this already by adding serde attribute setting on top of the vectors.

@christoph-hamm christoph-hamm mentioned this pull request Jul 10, 2024
Closed
@inf17101 inf17101 mentioned this pull request Jul 12, 2024
Closed
christoph-hamm
christoph-hamm commented Jul 15, 2024
View reviewed changes
Copy link
Contributor Author

@christoph-hamm christoph-hamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inf17101 , I have reviewed your changes on the examples.

examples/config/startupState.yaml Outdated Show resolved Hide resolved
inf17101
inf17101 approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@krucod3 krucod3 merged commit f09cb68 into main Jul 19, 2024
10 checks passed
@krucod3 krucod3 deleted the 22_authorize_requests branch July 19, 2024 07:41
@krucod3 krucod3 added enhancement New feature or request. Issue will appear in the change log "Features" breaking-change Issue will appear in the change log "Breaking Changes" labels Jul 31, 2024
@inf17101 inf17101 mentioned this pull request Aug 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krucod3 krucod3 krucod3 left review comments

@inf17101 inf17101 inf17101 approved these changes

Assignees
No one assigned
Labels
breaking-change Issue will appear in the change log "Breaking Changes" enhancement New feature or request. Issue will appear in the change log "Features"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@christoph-hamm @inf17101 @krucod3