-
Notifications
You must be signed in to change notification settings - Fork 152
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
Split CARMv2 functionality into Team Level Role and Service Level Role #158
Conversation
Skipping CI for Draft Pull Request. |
/test all |
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 is great work! Thanks Tibi!
// The user is specifying a namespace that is annotated with an owner account ID. | ||
// Requeue if the corresponding roleARN is not available in the Accounts (CARMv1) configmap. | ||
roleARN, err = r.getRoleARN(acctID) |
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.
Can we add a comment stating that we're looking i the classic CARM?
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.
The comment is stating we're looking in Accounts
configmap. Which is the default one. I refrained from using CARMv1 and v2 terminology as with this new approach.
/retest |
2 similar comments
/retest |
/retest |
This looks stable to me. Any other updates you want to make to the code/PR description? cc @TiberiuGC |
I could rename the flags as per your suggestion, just to be more descriptive?
|
Done c2213da |
/test all |
1 similar comment
/test all |
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.
Great stuff, thanks a lot @TiberiuGC !
/lgtm
/retest |
c2213da
to
3174b22
Compare
/test all |
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, TiberiuGC The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available:
Description of changes:
This PR aims to resolve a concern where a user migrating from CARMv1 to v2 (i.e. to teamIDs and service level isolation support) might end up with their resources re-created into incorrect accounts just by enabling the feature flag, due to lack of v2 configuration.
The PR splits CARMv2 feature into 2 different features, each behind its own feature flag:
TeamLevelCARM
, the mappings are being stored in a new configmapack-role-team-map
ServiceLevelCARM
, the mappings can be stored in both the existing configmapack-role-account-map
and the new configmapack-role-team-map
When both feature flags are ENABLED, the configmap setup may look like below (this is currently all squeezed into the CARMv2 map i.e.
ack-carm-map
):ack-role-team-map
👇ack-role-account-map
👇When both feature flags are DISABLED, or neither teamID annotation or service level roles are setup, runtime continues to use the existing CARMv1 setup:
ack-role-account-map
👇By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.