Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce a new CARM config map with support for teamIDs and service level isolation #155

Merged
merged 3 commits into from
Aug 12, 2024
Merged

Introduce a new CARM config map with support for teamIDs and service level isolation #155

merged 3 commits into from
Aug 12, 2024

Conversation

TiberiuGC
Copy link
Member

@TiberiuGC TiberiuGC commented Aug 2, 2024 •
edited
Loading

Issue #, if available: aws-controllers-k8s/community#2031

Description of changes:

data:
  team-a: "arn:aws:iam::111111111111:role/team-a-global-role"
  s3.team-a: "arn:aws:iam::111111111111:role/team-a-s3-role"
  dynamodb.team-a: "arn:aws:iam::111111111111:role/team-a-dynamodb-role"

OR

data:
  111111111111: arn:aws:iam::111111111111:role/global-role
  s3.111111111111: arn:aws:iam::111111111111:role/s3-role
  dynamodb.111111111111: arn:aws:iam::111111111111:role/dynamodb-role

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ACK Prow
Copy link

ack-prow bot commented Aug 2, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ack-prow ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2024
@ack-prow ack-prow bot requested review from a-hilaly and jlbutler August 2, 2024 11:39
@TiberiuGC TiberiuGC marked this pull request as ready for review August 6, 2024 04:04
@ack-prow ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2024
a-hilaly
a-hilaly reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tibi! I left a few thoughts in line

pkg/featuregate/features.go Outdated
Comment on lines 19 to 22
const (
// FeatureCARMv2 is the name of the CARMv2 feature.
FeatureCARMv2 = "CARMv2"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open question, is there a better name for this feature? to me this sounds like something a bit wide than what CARM does today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's common knowledge that naming stuff is the hardest part of programming 😆 , I don't have any better suggestions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha, oh yeah this one is tricky let's leave it until the end :)

pkg/runtime/cache/cache.go Outdated
Comment on lines 47 to 51
// The prefix for owner account ID in the v2 CARM map.
OwnerAccountIDPrefix = "owner-account-id."

// The prefix for owner team ID in the v2 CARM map.
TeamIDPrefix = "team-id."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: are we assuming that users will populate the configmap with something like team-id.abc and owner-account-id.012345678912 ? trying to understand the liaison with aws-controllers-k8s/community#2031

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, and in addition, with something like s3.team-id.abc or s3.owner-account-id.0123456789012. I've left an example in the PR description.

I'm happy to reconsider UX, but I thought it more or less aligns with the original ticket.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the format should exactly be s3.abc or s3.012345678912 - adding team-id or owner-account-id makes things a little unusual. However mind proposing this new format in the Github issue? maybe i'm wrong and ACK users would like to have it this way :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like your approach better, as long as the teamIDs and accountIDs are easily distinguishable, and they most likely are, the labels are redundant. I left these labels intentionally as they were part of the original PR and thought they were discussed about after the original proposal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

59c445a

apis/core/v1alpha1/annotations.go
// to succeed, the AWS IAM Role that the ACK service controller runs as needs
// to have the ability to call the AWS STS::AssumeRole API call and assume an
// IAM Role in the target AWS Account.
AnnotationTeamID = AnnotationPrefix + "team-id"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts: the more I think about this, the more I think maybe this should just be a role-arn .. looks like every team-id is just a RoleARN at the end of the day?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that, it's just a roleARN per annotated namespace, right? The whole teamID and ownerAccountID notations are just syntactic sugar, but I assume it makes it easier for the user when reasoning who should have which permissions? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I assume it makes it easier for the user when reasoning who should have which permissions? 🤔

You convinced me with this one.. it makes sense to reason that a team have a role...

a-hilaly
a-hilaly reviewed Aug 8, 2024
View reviewed changes
pkg/featuregate/features.go Outdated
@@ -16,11 +16,16 @@
// optionally overridden.
package featuregate

const (
// FeatureCARMv2 is the name of the CARMv2 feature.
FeatureCARMv2 = "CARMv2"
Copy link
Member

@a-hilaly a-hilaly Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
FeatureCARMv2 = "CARMv2"
CARMv2 = "CARMv2"

I'm thinking about reducing the verbosity when reusing this variable in r.cfg.FeatureGates.IsEnabled

r.cfg.FeatureGates.IsEnabled(featuregate.CARMv2)

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

88a1c3f

a-hilaly
a-hilaly approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super neat, thanks a lot @TiberiuGC 🙇
/lgtm

@ack-prow ack-prow bot added lgtm Indicates that a PR is ready to be merged. approved labels Aug 12, 2024
@a-hilaly
Copy link
Member

/lgtm cancel

@ack-prow ack-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2024
@ack-prow ACK Prow
Copy link

ack-prow bot commented Aug 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TiberiuGC
Once this PR has been reviewed and has the lgtm label, please ask for approval from a-hilaly by writing /assign @a-hilaly in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot removed the approved label Aug 12, 2024
@a-hilaly a-hilaly added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Aug 12, 2024
@a-hilaly a-hilaly merged commit a1359c9 into aws-controllers-k8s:main Aug 12, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-hilaly a-hilaly a-hilaly approved these changes

@jlbutler jlbutler Awaiting requested review from jlbutler

Assignees

@a-hilaly a-hilaly

Labels
tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TiberiuGC @a-hilaly @zicongmei