Allow specifying baseArn to prepend to role names. #95

danp60 · 2020-11-21T02:47:22Z

Description of changes:
This PR introduces the ability to specify a baseArn to prepend to role names when we detect that the arn passed to the eks.amazonaws.com/role-arn annotation is not fully qualified.

Our use case is that we have different clusters that run in different AWS accounts. This will allow us to use the same manifest to deploy to these different clusters and allow each pod to assume the correct IAM role local to their account.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

If the role-arn specified in the eks.amazonaws.com/role-arn is not fully qualified, prepend it with a default base Arn.

danp60 · 2020-11-30T21:01:55Z

Any feedback or comments @aws/eks-contributors?

danp60 · 2020-12-05T00:05:30Z

@jqmichael @josselin-c @micahhausler Any chance someone could take a look at this?

danopia · 2020-12-08T23:58:21Z

I'd love to see this merged; it's silly to require the role annotation to differ between AWS accounts. Splitting accounts by environment is pretty common practice, as is one cluster per account.

Fixes #56

danp60 · 2022-04-30T00:14:01Z

@nckturner @wongma7 does this look like a worthwhile contribution to you? Please let me know and I will cleanup this PR

jyotimahapatra · 2022-07-01T19:58:20Z

main.go

@@ -60,6 +60,7 @@ func main() {
 	// annotation/volume configurations
 	annotationPrefix := flag.String("annotation-prefix", "eks.amazonaws.com", "The Service Account annotation to look for")
 	audience := flag.String("token-audience", "sts.amazonaws.com", "The default audience for tokens. Can be overridden by annotation")
+	baseArn := flag.String("base-arn", "", "The base arn to use if a non fully qualified role is detected")


I have few thoughts on this:
1- This mechanism allows usage only when the user has access to change the parameters. Non cluster admins would not be able to use this functionality. wdyt about making this extensible by adding an new annotations on service accounts?
2- rollout and rollback(if needed) of this might become tricky due to using the same rolearn annotation. wdyt about using entirely different parameters for annotation like eks.amazonaws.com/role and eks.amazonaws.com/role-prefix that clarifies the intent of the fields explicitly
3. Can we add a feature flag for this functionality (set to alpha first and disabled by default) so that only interested users can flip this on.

The prefix is always going to be specific to the cluster. It will be arn:aws:iam::, followed by the ID of the account the cluster is in, followed by :role/, followed by the name of the cluster, followed by some fixed separator (I use _).

So one possibility would be to make this not configurable, simply prepending unqualified values with the prefix described above.

Thanks for the comments. I did not forget about this PR, and I'll try and make some time to work on it soon.

1- This mechanism allows usage only when the user has access to change the parameters. Non cluster admins would not be able to use this functionality. wdyt about making this extensible by adding an new annotations on service accounts?

One of the main purposes of this feature is to hide the details of the environment (ie the AWS account ID) from the end user. If the end user had to add an annotation with the base ARN that included the account ID, then they could just write the fully qualified ARN and not use this feature at all, so I'm not sure if it would be that useful.

2- rollout and rollback(if needed) of this might become tricky due to using the same rolearn annotation. wdyt about using entirely different parameters for annotation like eks.amazonaws.com/role and eks.amazonaws.com/role-prefix that clarifies the intent of the fields explicitly

I can do this. If an end user sets both, the most reasonable (and backwards compatible) behavior to me seems to be to take the original annotation eks.amazonaws.com/role verbatim and not make any changes.

Can we add a feature flag for this functionality (set to alpha first and disabled by default) so that only interested users can flip this on.

Do you think this is required if we do 2. and make the new annotation only a fallback if the original one doesn't exist?

The prefix is always going to be specific to the cluster. It will be arn:aws:iam::, followed by the ID of the account the cluster is in, followed by :role/, followed by the name of the cluster, followed by some fixed separator (I use _).

So one possibility would be to make this not configurable, simply prepending unqualified values with the prefix described above.
This seems reasonable to me too. I can add a flag for --aws-default-account-id similar to how we have flags for other AWS environment like region.

WDYT?

seh · 2022-09-02T23:52:10Z

Is anyone still working on this patch?

chlunde · 2022-09-03T20:55:01Z

@jyotimahapatra would this approach help anyone running on EKS? I would expect some template like {{clusterAccount}} might be supported in EKS, but configuring a CLI option would only work for self-hosted installations

jyotimahapatra · 2022-09-05T16:40:46Z

would this approach help anyone running on EKS? I would expect some template like {{clusterAccount}} might be supported in EKS, but configuring a CLI option would only work for self-hosted installations

Correct. This is right now relevant for self hosted installations.

johngmyers · 2022-09-06T16:53:57Z

How this would work on EKS is a problem for EKS. It is not particularly relevant for the amazon-eks-pod-identity-webhook project.

jyotimahapatra · 2022-09-06T18:35:46Z

How this would work on EKS is a problem for EKS. It is not particularly relevant for the amazon-eks-pod-identity-webhook project.

Correct. Once this is implemented, EKS can make it work by default.

seh · 2022-09-13T00:16:31Z

README.md

@@ -133,6 +133,7 @@ Usage of amazon-eks-pod-identity-webhook:
      --alsologtostderr                  log to standard error as well as files
      --annotation-prefix string         The Service Account annotation to look for (default "eks.amazonaws.com")
      --aws-default-region string        If set, AWS_DEFAULT_REGION and AWS_REGION will be set to this value in mutated containers
+      --base-arn string                  The base arn to use if a non fully qualified role is detected


Suggested change

--base-arn string The base arn to use if a non fully qualified role is detected

--role-base-arn string The base ARN to use as a prefix for completing role ARNs that are not fully qualified

seh · 2022-09-13T00:17:26Z

main.go

@@ -60,6 +60,7 @@ func main() {
 	// annotation/volume configurations
 	annotationPrefix := flag.String("annotation-prefix", "eks.amazonaws.com", "The Service Account annotation to look for")
 	audience := flag.String("token-audience", "sts.amazonaws.com", "The default audience for tokens. Can be overridden by annotation")
+	baseArn := flag.String("base-arn", "", "The base arn to use if a non fully qualified role is detected")


Suggested change

baseArn := flag.String("base-arn", "", "The base arn to use if a non fully qualified role is detected")

roleBaseARN := flag.String("role-base-arn", "", "The base ARN to use as a prefix for completing role ARNs that are not fully qualified")

seh · 2022-09-13T00:17:44Z

main.go

@@ -112,6 +113,7 @@ func main() {
 		handler.WithServiceAccountCache(saCache),
 		handler.WithRegion(*region),
 		handler.WithRegionalSTS(*regionalSTS),
+		handler.WithBaseArn(*baseArn),


Suggested change

handler.WithBaseArn(*baseArn),

handler.WithRoleBaseARN(*roleBaseARN),

seh · 2022-09-13T00:18:46Z

pkg/handler/handler.go

+// WithBaseArn sets the baseArn to use if we detect role name is not fully qualified
+func WithBaseArn(baseArn string) ModifierOpt {


Suggested change

// WithBaseArn sets the baseArn to use if we detect role name is not fully qualified

func WithBaseArn(baseArn string) ModifierOpt {

// WithRoleBaseARN sets the base ARN to use as a prefix for completing role ARNs that are not fully qualified

func WithRoleBaseARN(baseARN string) ModifierOpt {

seh · 2022-09-13T00:19:05Z

pkg/handler/handler.go

@@ -102,6 +104,11 @@ func WithRegionalSTS(enabled bool) ModifierOpt {
 	return func(m *Modifier) { m.RegionalSTSEndpoint = enabled }
 }

+// WithBaseArn sets the baseArn to use if we detect role name is not fully qualified
+func WithBaseArn(baseArn string) ModifierOpt {
+	return func(m *Modifier) { m.BaseArn = baseArn }


Suggested change

return func(m *Modifier) { m.BaseArn = baseArn }

return func(m *Modifier) { m.RoleBaseARN = baseARN }

seh · 2022-09-13T00:20:54Z

pkg/handler/handler.go

 	var changed bool
 	var initContainers = []corev1.Container{}
 	for i := range pod.Spec.InitContainers {
 		container := pod.Spec.InitContainers[i]
-		changed = m.addEnvToContainer(&container, tokenFilePath, roleName, updateSettings)
+		changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)


Suggested change

changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)

changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleARN, updateSettings)

seh · 2022-09-13T00:21:08Z

pkg/handler/handler.go

 		initContainers = append(initContainers, container)
 	}
 	var containers = []corev1.Container{}
 	for i := range pod.Spec.Containers {
 		container := pod.Spec.Containers[i]
-		changed = m.addEnvToContainer(&container, tokenFilePath, roleName, updateSettings)
+		changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)


Suggested change

changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)

changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleARN, updateSettings)

seh · 2022-09-13T00:21:36Z

pkg/handler/handler_pod_test.go

@@ -48,6 +48,7 @@ var (
 	handlerExpirationAnnotation = "testing.eks.amazonaws.com/handler/expiration"
 	handlerRegionAnnotation     = "testing.eks.amazonaws.com/handler/region"
 	handlerSTSAnnotation        = "testing.eks.amazonaws.com/handler/injectSTS"
+	handlerBaseArnAnnotation    = "testing.eks.amazonaws.com/handler/baseArn"


Suggested change

handlerBaseArnAnnotation = "testing.eks.amazonaws.com/handler/baseArn"

handlerBaseARNAnnotation = "testing.eks.amazonaws.com/handler/baseARN"

seh · 2022-09-13T00:22:03Z

pkg/handler/handler_pod_test.go

+	if baseArnAnnotation, ok := pod.Annotations[handlerBaseArnAnnotation]; ok {
+		modifiers = append(modifiers, WithBaseArn(baseArnAnnotation))


Suggested change

if baseArnAnnotation, ok := pod.Annotations[handlerBaseArnAnnotation]; ok {

modifiers = append(modifiers, WithBaseArn(baseArnAnnotation))

if baseARNAnnotation, ok := pod.Annotations[handlerBaseARNAnnotation]; ok {

modifiers = append(modifiers, WithBaseARN(baseARNAnnotation))

seh · 2022-09-13T00:22:32Z

pkg/handler/testdata/rawPodNoBaseArn.pod.yaml

+metadata:
+  name: balajilovesoreos
+  annotations:
+    testing.eks.amazonaws.com/handler/baseArn: 'arn:aws:iam::111122223333:role/'


Suggested change

testing.eks.amazonaws.com/handler/baseArn: 'arn:aws:iam::111122223333:role/'

testing.eks.amazonaws.com/handler/baseARN: 'arn:aws:iam::111122223333:role/'

Allow specifying baseArn to prefix to role names when necessary

3cfd6a6

If the role-arn specified in the eks.amazonaws.com/role-arn is not fully qualified, prepend it with a default base Arn.

danp60 requested a review from a team as a code owner November 21, 2020 02:47

jyotimahapatra reviewed Jul 1, 2022

View reviewed changes

seh reviewed Sep 13, 2022

View reviewed changes

johngmyers mentioned this pull request Mar 20, 2024

[EKS]: IAM Roles for Service Accounts enhancements for usage across multiple clusters aws/containers-roadmap#1408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying baseArn to prepend to role names. #95

Allow specifying baseArn to prepend to role names. #95

danp60 commented Nov 21, 2020

danp60 commented Nov 30, 2020

danp60 commented Dec 5, 2020 •

edited

Loading

danopia commented Dec 8, 2020

danp60 commented Apr 30, 2022

jyotimahapatra Jul 1, 2022

johngmyers Jul 7, 2022

danp60 Jul 29, 2022

danp60 Jul 29, 2022

seh commented Sep 2, 2022

chlunde commented Sep 3, 2022

jyotimahapatra commented Sep 5, 2022

johngmyers commented Sep 6, 2022

jyotimahapatra commented Sep 6, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

seh Sep 13, 2022

	--base-arn string The base arn to use if a non fully qualified role is detected
	--role-base-arn string The base ARN to use as a prefix for completing role ARNs that are not fully qualified

	baseArn := flag.String("base-arn", "", "The base arn to use if a non fully qualified role is detected")
	roleBaseARN := flag.String("role-base-arn", "", "The base ARN to use as a prefix for completing role ARNs that are not fully qualified")

	handler.WithBaseArn(*baseArn),
	handler.WithRoleBaseARN(*roleBaseARN),

		// WithBaseArn sets the baseArn to use if we detect role name is not fully qualified
		func WithBaseArn(baseArn string) ModifierOpt {

	return func(m *Modifier) { m.BaseArn = baseArn }
	return func(m *Modifier) { m.RoleBaseARN = baseARN }

	changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleArn, updateSettings)
	changed = m.addEnvToContainer(&container, tokenFilePath, fullRoleARN, updateSettings)

	handlerBaseArnAnnotation = "testing.eks.amazonaws.com/handler/baseArn"
	handlerBaseARNAnnotation = "testing.eks.amazonaws.com/handler/baseARN"

		if baseArnAnnotation, ok := pod.Annotations[handlerBaseArnAnnotation]; ok {
		modifiers = append(modifiers, WithBaseArn(baseArnAnnotation))

	testing.eks.amazonaws.com/handler/baseArn: 'arn:aws:iam::111122223333:role/'
	testing.eks.amazonaws.com/handler/baseARN: 'arn:aws:iam::111122223333:role/'

Allow specifying baseArn to prepend to role names. #95

Are you sure you want to change the base?

Allow specifying baseArn to prepend to role names. #95

Conversation

danp60 commented Nov 21, 2020

danp60 commented Nov 30, 2020

danp60 commented Dec 5, 2020 • edited Loading

danopia commented Dec 8, 2020

danp60 commented Apr 30, 2022

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

seh commented Sep 2, 2022

chlunde commented Sep 3, 2022

jyotimahapatra commented Sep 5, 2022

johngmyers commented Sep 6, 2022

jyotimahapatra commented Sep 6, 2022

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

danp60 commented Dec 5, 2020 •

edited

Loading