s3.bucketpolicy: Ignore string order #653
Conversation
When checking if two policies are identical, we must ignore string order. Idea for the fix from the ECR policy check. Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
| want: want{ | ||
| local: `{"Statement":[{"Action":"s3:GetObject","Effect":"Allow","Principal":"*","Resource":"arn:aws:s3:::test.s3.crossplane.com/*"},{"Action":"s3:ListBucket","Effect":"Allow","Principal":"*","Resource":"arn:aws:s3:::test.s3.crossplane.com"}],"Version":"2012-10-17"}`, | ||
| remote: `{"Version":"2012-10-17","Statement":[{"Action":"s3:ListBucket","Effect":"Allow","Principal":"*","Resource":"arn:aws:s3:::test.s3.crossplane.com"},{"Action":"s3:GetObject","Effect":"Allow","Principal":"*","Resource":"arn:aws:s3:::test.s3.crossplane.com/*"}]}`, | ||
| uptodate: false, |
There was a problem hiding this comment.
| uptodate: false, | |
| uptodate: true, |
Don't you want these two statements to be considered equal?
There was a problem hiding this comment.
Good catch, I did intended to write that when I wrote the test, as the name suggests 👍
I think there are four different things that can happen, in theory:
- Indentation change (seen in real life, at least the AWS IAM Policy console)
- Map keys change order (seen in real life w/ bucketpolicy code). Top level map keys, like
Version/StatementandAction/Principal/EffectwithinStatements. - The order of the array items I think we don't need to fix (i.e. Action: [] and Statement: []) - but I'm not sure. If I understand correctly, it does not matter when evaluating the policy, so in theory aws could re-order them. On the other hand, from my testing, they are preserved and I think it would be confusing for the user (and terraform) if AWS would re-shuffle them.
- Single item as array vs string. For example,
Action: ["s3:ListBucket"]vsAction: "s3:ListBucket".
This change handles the two first, which are the ones I have observed so far. I'm not sure if we want to proactively handle case 3/4. What do you think?
There was a problem hiding this comment.
@muvaf should we start with the known, simple case, indentation change and map key order (i.e. keep the code as it is but fix the testcases)?
There was a problem hiding this comment.
#701 also has a similar discussion related to up to date check. Is there anything we can do to converge on one place to handle this tricky stuff for everyone and handle it well there?
There was a problem hiding this comment.
I think that is a good idea, I've written some notes in #704. Closing this issue to follow up there.
When checking if two policies are identical, we must ignore string
order. Idea for the fix from the ECR policy check.
Do we want to move this method to a shared library to avoid duplicating it for all controllers with resource based policies?
Description of your changes
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested