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

fix: Validate scope chain in the AddUpdatePolicy RPC #1602

Closed
wants to merge 2 commits into from
Closed

fix: Validate scope chain in the AddUpdatePolicy RPC #1602

wants to merge 2 commits into from

Conversation

oguzhand95
Copy link
Member

Fixes #1591

@oguzhand95 oguzhand95 added the kind/bug Something isn't working label May 30, 2023
@oguzhand95 oguzhand95 self-assigned this May 30, 2023
@codecov
Copy link

codecov bot commented May 30, 2023 •
edited

Codecov Report

Merging #1602 (0093562) into main (7220e09) will increase coverage by 0.12%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1602      +/-   ##
==========================================
+ Coverage   53.34%   53.46%   +0.12%     
==========================================
  Files         130      130              
  Lines       14848    14866      +18     
==========================================
+ Hits         7920     7948      +28     
+ Misses       6233     6222      -11     
- Partials      695      696       +1
Impacted Files Coverage Δ
internal/storage/db/internal/db.go 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

@oguzhand95 oguzhand95 marked this pull request as ready for review May 30, 2023 14:29
charithe
charithe reviewed May 30, 2023
View reviewed changes
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

Worth adding a test or two to make sure this works as expected.

internal/storage/db/internal/db.go

var brokenChainPolicies []string
for _, p := range policies {
if has, ok := hasDescendants[p.ID]; ok && has {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if all the descendants are in the list of policies sent in the request? For example, if I have the policies p1/a, p1/a.b and p1/a.b.c and send them all in the request with disabled: true, then that request is valid because the whole chain is getting disabled. It shouldn't return an error.

Do we handle this case in the DisablePolicy RPC as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we don't handle this for both of them.

@oguzhand95
fix: Validate scope chain in the AddUpdatePolicy RPC
4176c80 
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
@oguzhand95
Add test case
0093562 
Signed-off-by: Oğuzhan Durgun <oguzhandurgun95@gmail.com>
@oguzhand95 oguzhand95 closed this Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charithe charithe charithe left review comments

Assignees

@oguzhand95 oguzhand95

Labels
kind/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate scope chain in the AddUpdatePolicy RPC
2 participants
@oguzhand95 @charithe