✨ Add EKS Pod Identities to AWSManagedControlPlane #4906

stefanmcshane · 2024-04-03T22:32:58Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support using AWS EKS Pod Identity Associations for mapping pods to IAM roles without needing OIDC or IRSA

https://docs.aws.amazon.com/eks/latest/userguide/pod-identities.html

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

N/A

Special notes for your reviewer:

Docs available here, for a brief overview on setup

Checklist:

Release note:

Support mapping kubernetes service account to IAM role using EKS Pod Identity Associations

k8s-ci-robot · 2024-04-03T22:33:08Z

Hi @stefanmcshane. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

stefanmcshane · 2024-04-05T15:30:08Z

/retest

stefanmcshane · 2024-04-05T16:36:40Z

/retest-required

controlplane/eks/api/v1beta2/types.go

AndiDog · 2024-04-09T11:05:15Z

controlplane/eks/api/v1beta2/types.go

+	// ServiceAccountName is the name of the kubernetes Service Account within the namespace
+	// +kubebuilder:validation:Required
+	ServiceAccountName string `json:"serviceAccountName"`
+	// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.
+	// +kubebuilder:validation:Required
+	// +kubebuilder:default=default
+	ServiceAccountNamespace string `json:"serviceAccountNamespace"`
+	// RoleARN is the ARN of an IAM role which the Service Account can assume.
+	// +kubebuilder:validation:Required
+	RoleARN string `json:"serviceAccountRoleARN,omitempty"`


They're all called serviceAccountXXX which is an antipattern in Kubernetes manifests. I'd recommend moving these to .serviceAccount.{name,namespace,roleARN}.

The goal here was to be overtly explicit as I have found that our customers get confused on that sometimes. Nothing that docs cannot fix however

In thinking more about this, I remembered that the kubernetes Pod spec switched from serviceAccount to serviceAccountName a few years back.
I do err on the side of clarity for maintainability's sake when it comes to naming, but would definitely not go against whatever is considered standard. Do you have a reference for the naming convention to help me decide?

In thinking about this more, whilst the medium is a CRD, the keys are in reference to an AWS object. I would expect a PodIdentityAssociation.Name to refer to the name of an AWS object when using the AWS API

Pod.spec.serviceAccountName is a singular field that won't change. It doesn't allow referring to another namespace on purpose.

The 3 fields you're adding will always be filled and none can be optional or defaulted (even the namespace), so a common prefix is unusual for newly-designed CRDs. See for example Ingress.spec.defaultBackend.service.{name,port} or Gateway API's Gateway.[...].listeners[*].{name,protocol,port}. Core APIs such as Pod aren't great examples since they can't be evolved without lots of breaking changes, and also a pod spec mostly only refers to names and doesn't provide many comparable examples here.

Therefore, here's my suggestion (dedenting roleARN as compared to my initial comment, since it's not referring to a service account):

podIdentityAssociations: - serviceAccount: name: myserviceaccount namespace: default roleARN: arn:aws:iam::012345678901:role/capi-test-role

There is definitely a back and forth on the semantics on this. I personally dont see this as being a blocking change, and moreso a semantic preference.

At least the JSON field name and struct name should be aligned, since currently they're not lowercase-equal which could be confusing:

Suggested change

// ServiceAccountName is the name of the kubernetes Service Account within the namespace

// +kubebuilder:validation:Required

ServiceAccountName string `json:"serviceAccountName"`

// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.

// +kubebuilder:validation:Required

// +kubebuilder:default=default

ServiceAccountNamespace string `json:"serviceAccountNamespace"`

// RoleARN is the ARN of an IAM role which the Service Account can assume.

// +kubebuilder:validation:Required

RoleARN string `json:"serviceAccountRoleARN,omitempty"`

// ServiceAccountName is the name of the kubernetes Service Account within the namespace

// +kubebuilder:validation:Required

ServiceAccountName string `json:"serviceAccountName"`

// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.

// +kubebuilder:validation:Required

// +kubebuilder:default=default

ServiceAccountNamespace string `json:"serviceAccountNamespace"`

// RoleARN is the ARN of an IAM role which the Service Account can assume.

// +kubebuilder:validation:Required

ServiceAccountRoleARN string `json:"serviceAccountRoleARN,omitempty"`

Or the other way around (json:"roleARN") – doesn't matter.

Then the documented examples in docs/book/src/topics/eks/pod-identity-associations.md must be converted to the final naming choice.

In this case, the roleARN is an AWS resource, not a service account, so the naming shouldnt be consistent. The suggested shouldn't be committed imo

controlplane/eks/api/v1beta2/conditions_consts.go

docs/book/src/topics/eks/pod-identity-associations.md

pkg/eks/podidentities/plan.go

pkg/eks/podidentities/procedures.go

AndiDog · 2024-04-09T12:14:49Z

pkg/eks/podidentities/procedures.go

+	if output.Association == nil {
+		return ErrNilPodIdentityAssociation
+	}


Can this really happen? Or is there a specific reason why we want to check this field here, since we're not even reading it? I'd rather remove this.

If there is a possibility for it in their SDK, I personally wouldnt rule it out. I'd personally rather catch the possibility of this error loudly with an error returned, than letting it silently continue. Ill follow your lead and remove it as the arn is not read/used at present

pkg/eks/podidentities/types.go

stefanmcshane · 2024-04-09T20:03:18Z

Split out the AWS updates to reduce the size of this PR
#4916

AndiDog · 2024-04-22T08:15:22Z

controlplane/eks/api/v1beta2/types.go

+	// ServiceAccountName is the name of the kubernetes Service Account within the namespace
+	// +kubebuilder:validation:Required
+	ServiceAccountName string `json:"serviceAccountName"`
+	// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.
+	// +kubebuilder:validation:Required
+	// +kubebuilder:default=default
+	ServiceAccountNamespace string `json:"serviceAccountNamespace"`
+	// RoleARN is the ARN of an IAM role which the Service Account can assume.
+	// +kubebuilder:validation:Required
+	RoleARN string `json:"serviceAccountRoleARN,omitempty"`


Pod.spec.serviceAccountName is a singular field that won't change. It doesn't allow referring to another namespace on purpose.

The 3 fields you're adding will always be filled and none can be optional or defaulted (even the namespace), so a common prefix is unusual for newly-designed CRDs. See for example Ingress.spec.defaultBackend.service.{name,port} or Gateway API's Gateway.[...].listeners[*].{name,protocol,port}. Core APIs such as Pod aren't great examples since they can't be evolved without lots of breaking changes, and also a pod spec mostly only refers to names and doesn't provide many comparable examples here.

Therefore, here's my suggestion (dedenting roleARN as compared to my initial comment, since it's not referring to a service account):

podIdentityAssociations: - serviceAccount: name: myserviceaccount namespace: default roleARN: arn:aws:iam::012345678901:role/capi-test-role

pkg/eks/podidentities/plan.go

AndiDog · 2024-04-22T08:36:22Z

pkg/eks/podidentities/plan.go

@@ -0,0 +1,102 @@
+/*


We don't have a style or coding guide, I think. I did a regular code review and while reading, I found this to be a bit too complex for what it's doing, especially since one needs to read multiple functions. The source example pkg/eks/addons/plan.go is doing a bit more because of addon "states", and didn't have access to the nice slices module at the time. I don't think the code is very bad here. But maybe we can modernize rather than copy the existing idea as-is. It can be mostly a single function.

pkg/eks/podidentities/types.go

AndiDog · 2024-05-25T15:36:07Z

controlplane/eks/api/v1beta2/types.go

+	// ServiceAccountName is the name of the kubernetes Service Account within the namespace
+	// +kubebuilder:validation:Required
+	ServiceAccountName string `json:"serviceAccountName"`
+	// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.
+	// +kubebuilder:validation:Required
+	// +kubebuilder:default=default
+	ServiceAccountNamespace string `json:"serviceAccountNamespace"`
+	// RoleARN is the ARN of an IAM role which the Service Account can assume.
+	// +kubebuilder:validation:Required
+	RoleARN string `json:"serviceAccountRoleARN,omitempty"`


At least the JSON field name and struct name should be aligned, since currently they're not lowercase-equal which could be confusing:

Suggested change

// ServiceAccountName is the name of the kubernetes Service Account within the namespace

// +kubebuilder:validation:Required

ServiceAccountName string `json:"serviceAccountName"`

// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.

// +kubebuilder:validation:Required

// +kubebuilder:default=default

ServiceAccountNamespace string `json:"serviceAccountNamespace"`

// RoleARN is the ARN of an IAM role which the Service Account can assume.

// +kubebuilder:validation:Required

RoleARN string `json:"serviceAccountRoleARN,omitempty"`

// ServiceAccountName is the name of the kubernetes Service Account within the namespace

// +kubebuilder:validation:Required

ServiceAccountName string `json:"serviceAccountName"`

// ServiceAccountNamespace is the kubernetes namespace, which the kubernetes Service Account resides in. Defaults to "default" namespace.

// +kubebuilder:validation:Required

// +kubebuilder:default=default

ServiceAccountNamespace string `json:"serviceAccountNamespace"`

// RoleARN is the ARN of an IAM role which the Service Account can assume.

// +kubebuilder:validation:Required

ServiceAccountRoleARN string `json:"serviceAccountRoleARN,omitempty"`

Or the other way around (json:"roleARN") – doesn't matter.

Then the documented examples in docs/book/src/topics/eks/pod-identity-associations.md must be converted to the final naming choice.

AndiDog · 2024-05-25T15:44:57Z

pkg/eks/podidentities/types.go

+}
+
+// IsEqual determines if 2 EKSPodIdentityAssociation are equal.
+func (e *EKSPodIdentityAssociation) IsEqual(other *EKSPodIdentityAssociation) bool {


Is this function maybe unused? I checked that make test still works without it.

k8s-ci-robot · 2024-06-03T15:40:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. 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:

OWNERS

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

AndiDog

Thanks for the small last changes to finish this up.

/lgtm

k8s-ci-robot · 2024-06-05T21:11:22Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2024-07-30T01:06:47Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-10-23T17:04:38Z

@stefanmcshane: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-build-release-2-6	`1cfd3e7`	link	true	`/test pull-cluster-api-provider-aws-build-release-2-6`
pull-cluster-api-provider-aws-build-docker-release-2-6	`1cfd3e7`	link	true	`/test pull-cluster-api-provider-aws-build-docker-release-2-6`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

richardcase · 2024-10-24T06:06:16Z

/retest

AndiDog · 2024-11-18T13:05:00Z

PR needs a rebase due to merge conflicts, please

k8s-triage-robot · 2025-02-16T13:36:01Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

richardcase · 2025-02-17T14:40:34Z

/remove-lifecycle stale

Danil-Grigorev · 2025-04-22T19:33:24Z

Hey @stefanmcshane - pinging in case you want to follow up on this PR? If not, would you mind if I try to rebase it to get it merged?

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Apr 3, 2024

k8s-ci-robot requested review from AndiDog and cnmcavoy April 3, 2024 22:33

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 3, 2024

stefanmcshane changed the title ~~Add EKS Pod Identities to AWSManagedControlPlane~~ ✨ Add EKS Pod Identities to AWSManagedControlPlane Apr 3, 2024

Ankitasw added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 5, 2024

AndiDog reviewed Apr 9, 2024

View reviewed changes

stefanmcshane mentioned this pull request Apr 9, 2024

🌱 bump aws sdk version #4916

Merged

5 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2024

stefanmcshane force-pushed the main branch 2 times, most recently from 836fbba to 121d8b1 Compare April 19, 2024 15:08

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 19, 2024

Add eks pod identities

bae312d

stefanmcshane force-pushed the main branch from c68a8b7 to bae312d Compare April 19, 2024 16:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

AndiDog reviewed Apr 22, 2024

View reviewed changes

stefanmcshane added 3 commits May 1, 2024 17:56

semantic changes

57d769f

memory aliasing patch

8b271f6

memory aliasing patch

bec51fd

AndiDog reviewed May 25, 2024

View reviewed changes

Update types.go

82c3c48

update json tag

342ca84

AndiDog reviewed Jun 4, 2024

View reviewed changes

k8s-ci-robot assigned AndiDog Jun 4, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2024

update deps

1cfd3e7

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2024

richardcase mentioned this pull request Nov 4, 2024

WIP: feat: eks pod identity support for controllers #5205

Open

5 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 16, 2025

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add EKS Pod Identities to AWSManagedControlPlane #4906

✨ Add EKS Pod Identities to AWSManagedControlPlane #4906

stefanmcshane commented Apr 3, 2024 •

edited

Loading

k8s-ci-robot commented Apr 3, 2024

stefanmcshane commented Apr 5, 2024

stefanmcshane commented Apr 5, 2024

AndiDog Apr 9, 2024

stefanmcshane Apr 9, 2024

stefanmcshane Apr 9, 2024

stefanmcshane Apr 9, 2024

AndiDog Apr 22, 2024

stefanmcshane May 1, 2024

AndiDog May 25, 2024

stefanmcshane May 29, 2024

AndiDog Apr 9, 2024

stefanmcshane Apr 9, 2024

stefanmcshane commented Apr 9, 2024

AndiDog Apr 22, 2024

AndiDog Apr 22, 2024

AndiDog May 25, 2024

AndiDog May 25, 2024

k8s-ci-robot commented Jun 3, 2024

AndiDog left a comment

k8s-ci-robot commented Jun 5, 2024

k8s-ci-robot commented Jul 30, 2024

k8s-ci-robot commented Oct 23, 2024

richardcase commented Oct 24, 2024

AndiDog commented Nov 18, 2024

k8s-triage-robot commented Feb 16, 2025

richardcase commented Feb 17, 2025

Danil-Grigorev commented Apr 22, 2025

✨ Add EKS Pod Identities to AWSManagedControlPlane #4906

Are you sure you want to change the base?

✨ Add EKS Pod Identities to AWSManagedControlPlane #4906

Conversation

stefanmcshane commented Apr 3, 2024 • edited Loading

k8s-ci-robot commented Apr 3, 2024

stefanmcshane commented Apr 5, 2024

stefanmcshane commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanmcshane commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 3, 2024

AndiDog left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 5, 2024

k8s-ci-robot commented Jul 30, 2024

k8s-ci-robot commented Oct 23, 2024

richardcase commented Oct 24, 2024

AndiDog commented Nov 18, 2024

k8s-triage-robot commented Feb 16, 2025

richardcase commented Feb 17, 2025

Danil-Grigorev commented Apr 22, 2025

stefanmcshane commented Apr 3, 2024 •

edited

Loading