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.

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

bundle: Replace HasPrefix in erasePolicy #3863

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

edpaget
Copy link
Contributor

@edpaget edpaget commented Oct 5, 2021

Using HasPrefix to compare a root path to the path of policy can result
in unexpected behaviour when different bundles have similarly named
roots.

This replaces the HasPrefix comparison with a comparison that treats the
paths as directories and checks if the policy path is the same path as
the root or a subdirectory of the root path.

Signed-off-by: Edward Paget edward.paget@chime.com

For a full explanation of this fix, I ran into this problem with our OPA deployment where I have bundles that are related to specific events. I have two events which I'll call event_one and another called event_one_for_the_books. They have policies packaged under com/example/event_one and com/example/event_one_for_the_books. I found that -- depending on the order the bundles were initialized on my OPA node -- policies under the root com/example/event_one_for_the_books were not present because they were being deleted when the event_one bundle was initialized.

I think there's a good case to be made for my events to not be named such that the name of one is a substring of the other, but policies being overwritten was still surprising.

Also since bundle roots and package names seem to be generally treated as paths, it made sense to me that the comparison should look for equal paths or subdirectories instead of a string prefix.

@edpaget edpaget force-pushed the ep-fix-has-prefix branch from 467ed03 to f10c74e Compare October 5, 2021 22:39
@srenatus srenatus changed the title bundle: Repalce HasPrefix in erasePolicy bundle: Replace HasPrefix in erasePolicy Oct 6, 2021
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! This is a good bug to fix indeed.

One question re: re-using existing code inline 👇 🙃

bundle/store.go Outdated Show resolved Hide resolved
@edpaget edpaget force-pushed the ep-fix-has-prefix branch from 25628ad to c8f16f2 Compare October 6, 2021 16:19
srenatus
srenatus previously approved these changes Oct 6, 2021
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks! Would you mind squashing the commits so we can get this merged?

Using HasPrefix to compare a root path to the path of policy can result
in unexpected behaviour when different bundles have similarly named
roots.

This replaces the HasPrefix comparison with a comparison that treats the
paths as directories and checks if the policy path is the same path as
the root or a subdirectory of the root path.

Signed-off-by: Edward Paget <edward.paget@chime.com>

Fixed changes requested by @srenatus

Replace custom function with existing one.

Signed-off-by: Edward Paget <edward.paget@chime.com>
@edpaget
Copy link
Contributor Author

edpaget commented Oct 6, 2021

All done thanks for the quick responses!

@srenatus srenatus merged commit 02a3bf0 into open-policy-agent:main Oct 6, 2021
edpaget added a commit to 1debit/opa that referenced this pull request Oct 7, 2021
Using HasPrefix to compare a root path to the path of policy can result
in unexpected behaviour when different bundles have similarly named
roots.

This replaces the HasPrefix comparison with a comparison that treats the
paths as directories and checks if the policy path is the same path as
the root or a subdirectory of the root path, using an existing helper.

Signed-off-by: Edward Paget <edward.paget@chime.com>
dolevf pushed a commit to dolevf/opa that referenced this pull request Nov 4, 2021
Using HasPrefix to compare a root path to the path of policy can result
in unexpected behaviour when different bundles have similarly named
roots.

This replaces the HasPrefix comparison with a comparison that treats the
paths as directories and checks if the policy path is the same path as
the root or a subdirectory of the root path, using an existing helper.

Signed-off-by: Edward Paget <edward.paget@chime.com>
Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants