-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(eks-v2-alpha): ensure kubectl provider access entry is depended upon by downstream resources #36734
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
Conversation
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 review is outdated)
|
|
||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
8554e9e to
109675f
Compare
109675f to
a912f56
Compare
|
Snapshots successfully deployed locally. |
kumvprat
left a comment
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.
Couple of high level comments :
- Can we look into why the linter says snapshot updates are required, even though the snapshots have literally changed ? (add the exemption requested label with some clarification around why we are not changing the integ tests itself but only the snapshots)
- Can we look into security guardian failures ? It looks like errors like these are the reason :
Check was not compliant as property [Condition] is missing. Value traversed to [Path=/Resources/AdminRole38563C57/Properties/AssumeRolePolicyDocument/Statement/0[L:8,C:12] Value={"Action":"sts:AssumeRole","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::123456789012:root"}}].If this indeed an exception we can suppress this rule for eks v2 tests and if not we need to investigate as to the reason behind security guardian alerts.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Agreed, can we have a comment explaining why we are witnessing snapshot changes even though we didn't change any integration tests ? (a one-liner like : changed/added a critical dependency in core eks setup which leads to all integration test use case snapshot changes)
Can you expand on this a bit ? Looks like the errors below are allowing
|
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status✅ The pull request has been merged at 19947cb This pull request spent 5 hours 13 minutes 18 seconds in the queue, including 31 minutes 30 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Note: Copied from #34898 with updated snapshots, credit to @msessa.
Closes #34897
Reason for this change
The
AccessEntryfor kubectl provider should be included as a dependency of the kubectl ready barrier.Description of changes
Add the kubectl
AccessEntryto the explicit dependencies for the ready barrier resourceDescription of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license