Fix ECR Repo policy eternally updating #701
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves a couple of issues around detecting changes on the ECR
RepositoryPolicy type:
1. Converts `awsAccountId` principals that were not specified as an
ARN into the ARN format, as AWS always returns in this format from
the API.
2. Makes sure to sort string slices within the policy document during
comparison, to avoid issues where AWS returns values in a different
order.
Fixes crossplane-contrib#700
Signed-off-by: Ben Agricola <bagricola@squiz.co.uk>
benagricola
commented
Jun 7, 2021
muvaf
reviewed
Jun 7, 2021
Previous implementation would panic if the user (or AWS for that matter) inputted a non-string into an ECR RepositoryPolicy. Rather than panic, we should simply return false (i.e. no change to sort order). This may mean that slices containing unknown types are not sorted correctly, but that is better than panicing. This also adds a single test that confirms the behaviour - numbers are not sorted while the strings are, and no panic is observed. Signed-off-by: Ben Agricola <bagricola@squiz.co.uk>
|
@muvaf latest commit conditionally sorts or returns false based on whether the interfaces can be converted to string or not. I've only implemented string as I believe it is the only valid format, but again - this is an assumption made based on the doco, not knowledge 🙂 |
chlunde
reviewed
Jun 8, 2021
Instead of detecting the lack of an ARN prefix, we now detect AWS account ID's by trying to decode a 64 bit integer from the input value. By doing this, we avoid accidentally formatting values that do not only contain an account ID but do not match the ARN prefix format we were positively checking for previously. Signed-off-by: Ben Agricola <bagricola@squiz.co.uk>
2 tasks
|
@muvaf I think this is good to go - I've left all the changes in, as I think @chlunde planned on using this as a base for merging the policy code from ECR and S3 (and maybe others?). Any chance we can merge this before #704 is implemented as it should at least fix the underlying issue for ECR repos 😃 |
tektondeploy
pushed a commit
to gtn3010/provider-aws
that referenced
this pull request
Mar 12, 2024
…-package Rename family parent package from provider-aws-config to provider-family-aws
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
Resolves a couple of issues around detecting changes on the ECR
RepositoryPolicytype:awsAccountIdprincipals that were not specified as anARN into the ARN format, as AWS always returns in this format from
the API.
comparison, to avoid issues where AWS returns values in a different
order.
Fixes #700
Signed-off-by: Ben Agricola bagricola@squiz.co.uk
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Additional test added to compare policies with defined principals as a string slice, that tests both of these cases.
That test fails without the fixes applied.