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.
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
Added AWS Credentials Support for Scanning Private Registry (ECR) #1103
base: main
Are you sure you want to change the base?
Added AWS Credentials Support for Scanning Private Registry (ECR) #1103
Changes from 5 commits
b875dc8
1411d12
d361d7f
23bce93
58fb2c0
bc24ddd
10768d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should be restored as it was , we only change versions when we release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed back to normal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ecrTokenRefreshTTL
has to be shorter then 12h? as Trivy might want to authenticate with an expired credentialThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe a retry logic should be placed for
GetAuthorizationToken
in case tokens are expired unless we force refresh before 12hThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Trivy will never use an expired token as in the original part I pushed, the Token creation was part of "each" individual request Trivy did per repository. The only reason we have this timer now in place is to reduce the number of requests (Throttling) against the ECR-API.
So, if you want to detect an expired token and you want to make it part of a retry to refresh the token now you need to get the first trivy job triggered which tries to get the image and will run into the error.
In this case, a Token Refresh needs to be triggered. But this is a completely new scenario, which needs to be plugged into the trivy process in combination with the PlugIn routines.
Also, a drawback and a negative aspect are here is, that each request needs to be sent towards API to check if still valid or not. But this will be contradicting the Throttling which I wanted to solve.
As of now, I see no point in that we need that. The Throttling does its job, and it is up to the implementer to configure the TTLs on their own. In my view, this is the best approach.