Add support for EKS Pod Identity #416

tongshen-stephanie · 2025-01-08T17:53:06Z

Issue #, if available: Closes #300

Description of changes:
Introduce an optional parameter usePodIdentity to support EKS Pod Identity

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

codecov-commenter · 2025-01-08T22:58:50Z

Codecov Report

Attention: Patch coverage is 68.42105% with 72 lines in your changes missing coverage. Please review.

Project coverage is 52.62%. Comparing base (be94dd0) to head (4dd8774).

Files with missing lines	Patch %	Lines
...ntial_provider/pod_identity_credential_provider.go	70.43%	28 Missing and 6 partials ⚠️
auth/auth.go	31.42%	23 Missing and 1 partial ⚠️
credential_provider/irsa_credential_provider.go	83.33%	10 Missing ⚠️
provider/parameter_store_provider.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #416      +/-   ##
==========================================
+ Coverage   49.67%   52.62%   +2.94%     
==========================================
  Files           9       11       +2     
  Lines         781      952     +171     
==========================================
+ Hits          388      501     +113     
- Misses        383      436      +53     
- Partials       10       15       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

README.md

auth/auth.go

simonmarty · 2025-01-15T22:45:58Z

auth/auth.go

+			return nil, err
+		}
+		credProvider := NewIRSACredentialProvider(p.stsClient, *roleArn, p.region)
+		config, err = credProvider.GetAWSConfig(fetcher)


config, err = credProvider.GetAWSConfig(fetcher) if err != nil { return nil, err }

is common to both blocks of the if statement, it can be extracted out.

made change in commit c3ca813

simonmarty · 2025-01-15T23:27:08Z

auth/auth.go

+	}
+}
+
+func (p *PodIdentityCredentialProvider) GetAWSConfig(fetcher *authTokenFetcher) (*aws.Config, error) {


Can we move the two credential providers to their own files in a subfolder?

Do you mean creating a separate folder/package similar to auth? I didn't do it because moving credential providers to a new folder would also require moving authTokenFetcher at the same time (otherwise it causes circular dependency since credential provider and auth try to import each other). auth is pretty much empty after moving out those 2 big pieces.

made the change in commit 703570b

simonmarty · 2025-01-16T18:35:54Z

auth/auth.go

+	return podIdentityAgentEndpointIPv4
+}()
+
+var podIdentityAgentEndpoint = defaultPodIdentityAgentEndpoint


Is defaultPodIdentityAgentEndpoint used anywhere else? If not you can remove the extra assignment.

defaultPodIdentityAgentEndpoint is used in tests. The endpoint is overridden in test, so assign it back to default state after test completion.

simonmarty · 2025-01-16T18:38:50Z

auth/auth.go

+			return nil, roleErr
+		}
+		credProvider := NewIRSACredentialProvider(p.stsClient, *roleArn, p.region)
+		config, err = credProvider.GetAWSConfig(fetcher)


I think you missed config, err = credProvider.GetAWSConfig(fetcher)

sorry for missing this. corrected in commit 703570b

simonmarty · 2025-01-16T18:41:27Z

auth/auth.go

+		config, err = credProvider.GetAWSConfig(fetcher)
+
+	} else {
+		roleArn, roleErr := p.getRoleARN()


This check should be the responsibility of the IRSA credential provider, let's move it to that provider's implementation in its constructor.

made the move in commit 703570b

simonmarty · 2025-01-21T18:22:00Z

credential_provider/credential_provider_test.go

@@ -0,0 +1,279 @@
+package credential_provider


Can we also split this test file into two test files for both implementations of the credential providers?

made the change in commit 539f715

simonmarty · 2025-01-21T18:23:55Z

server/server.go

+	}
+
+	if usePodIdentity {
+		klog.Infof("Using Pod Identity for authentication in namespace: %s, service account: %s", nameSpace, svcAcct)


These can be logged from here for better encapsulation.

made the change in commit 539f715

simonmarty · 2025-01-21T18:26:50Z

charts/secrets-store-csi-driver-provider-aws/templates/daemonset.yaml

@@ -53,6 +53,10 @@ spec:
          env:
            - name: AWS_USE_FIPS_ENDPOINT
              value: {{ .Values.useFipsEndpoint | quote }}
+            - name: POD_IP


Just for good measure, let's leave a comment above this explaining that this is needed for the pod identity agent to choose between IPV4/IPV6

added in commit 539f715

joebaro · 2025-01-22T23:24:41Z

examples/ExampleDeployment-IRSA.yaml

 spec:
  selector:
-    app: nginx
+    app: nginx-irsas


Looks like an extra trailing 's'.

fixed in commit 4d4df72. Thank you for catching!

joebaro · 2025-01-24T23:04:05Z

auth/auth.go

 func NewAuth(
 	ctx context.Context,
-	region, nameSpace, svcAcc string,
+	region, nameSpace, svcAcc, podName string,
+	usePodIdentity bool,
 	k8sClient k8sv1.CoreV1Interface,
 ) (auth *Auth, e error) {



We do not use sess nor stsClient when usePodIdentity is true. Can move the sts.new(sess) call up here and condition it all on !usePodIdentity (setting stsClient to nil otherwise)?

change made in commit 4d4df72

joebaro · 2025-01-25T00:56:15Z

README.md

@@ -8,14 +8,14 @@ AWS offers two services to manage secrets and parameters conveniently in your co
 ## Installation

 ### Requirements
-* Amazon Elastic Kubernetes Service (EKS) 1.17+ running an EC2 node group (Fargate node groups are not supported **[^1]**)
+* Amazon Elastic Kubernetes Service (EKS) 1.24+ running an EC2 node group (Fargate node groups are not supported **[^1]**)


I am guessing this is being driven by Pod Identity only being supported on 1.24. I know 1.17 is no longer supported but this does not match what is required by the helm chart and if we change that it could break users running ancient installations out there who need to reload this driver for some reason.

Instead, we could just leave this at 1.17 and mention somewhere below that using Pod Identity requires EKS 1.24.

change made in commit 4d4df72

joebaro · 2025-01-25T02:59:50Z

charts/secrets-store-csi-driver-provider-aws/templates/daemonset.yaml

+            - name: POD_IP  # This is needed to choose between IPv4 and IPv6 Pod Identity Agent endpoint
+              valueFrom:
+                fieldRef:
+                  fieldPath: status.podIP


This is all inside an {{- if .Values.useFipsEndpoint }} condition so when using the helm chart install option it will never be set unless the administrator turns on FIPS. We could move the "if FIPS" condition to be just around the FIP env variable, but it would be better to just not need to pass in the address in configuration and dynamically determine the address. See comments below.

removed setting POD_IP env variable in daemonset template and installers in commit 4d4df72

joebaro · 2025-01-27T18:00:50Z

credential_provider/pod_identity_credential_provider.go

+var defaultPodIdentityAgentEndpoint = func() string {
+	isIPv6, err := isIPv6()
+	if err != nil {
+		klog.Warningf("Error determining IP version: %v. Defaulting to IPv4 endpoint", err)
+		return podIdentityAgentEndpointIPv4
+	}
+
+	if isIPv6 {
+		klog.Infof("Using Pod Identity Agent IPv6 endpoint")
+		return podIdentityAgentEndpointIPv6
+	}
+	klog.Infof("Using Pod Identity Agent IPv4 endpoint")
+	return podIdentityAgentEndpointIPv4
+}()


Looking up the address type is cumbersome and even if we get it right, there are a lot of ways the network could get mis-configured. A more reliable approach is to just try both addresses in quick succession. Try IPV4 with a 100ms timeout, and if that fails log the error as a warning and then try IPV6 with the same timeout and only fail if both tries fail.

This adds a slight penalty for IPV6 only configurations so we would also want to give the option to turn off IPV4. To make this general we would probably let the user turn off either (maybe something like preferedAddressType = IPV4/IPV6).

introduce a new optional parameter preferredAddressType in commit 4d4df72.

Implemented behavior:

if preferredAddressType is set to ipv4, only try ipv4

if preferredAddressType is set to ipv6, only try ipv6

if preferredAddressType is not set or is set with value other than ipv4/ipv6, try ipv4 first with 100ms timeout and then try ipv6. if both tries fail, error out.

joebaro · 2025-01-27T20:48:13Z

credential_provider/pod_identity_credential_provider.go

+		},
+	}
+
+	// Use the K8s API to fetch the token from the OIDC provider.


In this case there likely will not be an OIDC provider.

updated in commit 4d4df72

… endpoint

joebaro · 2025-02-04T02:48:51Z

credential_provider/pod_identity_credential_provider.go

+		if preferredAddressType != "" {
+			klog.Warningf("Unknown preferred address type: %s, falling back to auto selection", preferredAddressType)
+		}


Directly failing on bad configuration is the best option. It makes problems easier to find and it is difficult to add in later.

Made change in commit c88e0f6

joebaro · 2025-02-04T18:12:21Z

credential_provider/pod_identity_credential_provider.go

+	switch p.preferredEndpoint {
+	case preferenceIPv4:
+		klog.Infof("Using preferred Pod Identity Agent IPv4 endpoint")
+		config, err := p.getAWSConfigFromPodIdentityAgent(token, podIdentityAgentEndpointIPv4)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get AWS config from pod identity agent IPv4 endpoint: %+v", err)
+		}
+		return config, nil
+	case preferenceIPv6:
+		klog.Infof("Using preferred Pod Identity Agent IPv6 endpoint")
+		config, err := p.getAWSConfigFromPodIdentityAgent(token, podIdentityAgentEndpointIPv6)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get AWS config from pod identity agent IPv6 endpoint: %+v", err)
+		}
+		return config, nil
+	default:
+		klog.Infof("Using auto Pod Identity Agent endpoint selection")
+		config, err := p.getAWSConfigFromPodIdentityAgent(token, podIdentityAgentEndpointIPv4)
+		if err != nil {
+			klog.Warningf("IPv4 endpoint attempt failed: %+v. Trying IPv6 endpoint", err)
+			config, err = p.getAWSConfigFromPodIdentityAgent(token, podIdentityAgentEndpointIPv6)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get AWS config from pod identity agent: %+v", err)
+			}
+		}
+		return config, nil
+	}


It is a little more natural to solve things in general by avoiding handling each case separately. For example:

if( p.preferredEndpoint == preferenceIPv4 || p.preferredEndpoint == preferenceAuto ) { config, err := p.getAWSConfigFromPodIdentityAgent(token, podIdentityAgentEndpointIPv4) if err != nil { klog.Warningf("IPv4 endpoint attempt failed: %+v.", err) } else { return config, nil; } } if( p.preferredEndpoint == preferenceIPv6 || p.preferredEndpoint == preferenceAuto ) { config, err := p.getAWSConfigFromPodIdentityAgent(token, podIdentityAgentEndpointIPv6) if err != nil { klog.Warningf("IPv6 endpoint attempt failed: %+v.", err) } } if err != nil { return nil, fmt.Errorf("failed to get AWS config from pod identity agent: %+v", err); } return config, nil;

made change in commit c88e0f6. This does look a lot cleaner. Thank you!

Add support for Pod Idenity

a8d669d

tongshen-stephanie requested a review from a team as a code owner January 8, 2025 17:53

tongshen-stephanie requested a review from simonmarty January 8, 2025 17:58

tongshen-stephanie changed the title ~~Add support for Pod Idenity~~ Add support for EKS Pod Identity Jan 8, 2025

Punctuation

a34275e

simonmarty requested changes Jan 8, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

auth/auth.go Outdated Show resolved Hide resolved

auth/auth.go Show resolved Hide resolved

simonmarty mentioned this pull request Jan 8, 2025

Add support for Pod Identity Association #376

Closed

Add support for IPv6, create CredentialProvider interface

01b160c

tongshen-stephanie requested review from joebaro and simonmarty January 13, 2025 22:45

Stephanie Shen and others added 3 commits January 14, 2025 21:48

Update ReadMe for using Pod Identity in cross-account use case

6a64907

Add space

902481f

Fix command

cc7b30f

simonmarty requested changes Jan 16, 2025

View reviewed changes

Extract common error checking block

c3ca813

tongshen-stephanie requested a review from simonmarty January 16, 2025 04:49

simonmarty requested changes Jan 16, 2025

View reviewed changes

Move credential provider to separate package

703570b

tongshen-stephanie requested a review from simonmarty January 21, 2025 05:37

simonmarty requested changes Jan 21, 2025

View reviewed changes

Split credential provider tests

539f715

tongshen-stephanie requested a review from simonmarty January 22, 2025 05:52

simonmarty approved these changes Jan 23, 2025

View reviewed changes

joebaro reviewed Jan 28, 2025

View reviewed changes

Add optional parameter preferredAddressType to configure Pod Identity…

4d4df72

… endpoint

tongshen-stephanie requested a review from joebaro January 29, 2025 07:24

Stephanie Shen added 2 commits January 31, 2025 12:40

Clarify preferredAddressType is EKS Pod Identity specific

02e6b54

Add support for cross-account parameter fetching

1359065

joebaro reviewed Feb 4, 2025

View reviewed changes

Stephanie Shen added 2 commits February 4, 2025 13:08

Switch to failing directly on bad preferredAddressType configuration

c88e0f6

Fix invalid preferred address type error message

4dd8774

joebaro approved these changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for EKS Pod Identity #416

Add support for EKS Pod Identity #416

tongshen-stephanie commented Jan 8, 2025 •

edited by simonmarty

Loading

codecov-commenter commented Jan 8, 2025 •

edited

Loading

simonmarty Jan 15, 2025

tongshen-stephanie Jan 16, 2025

simonmarty Jan 15, 2025

tongshen-stephanie Jan 16, 2025 •

edited

Loading

tongshen-stephanie Jan 21, 2025

simonmarty Jan 16, 2025

tongshen-stephanie Jan 21, 2025

simonmarty Jan 16, 2025

tongshen-stephanie Jan 21, 2025

simonmarty Jan 16, 2025

tongshen-stephanie Jan 21, 2025

simonmarty Jan 21, 2025

tongshen-stephanie Jan 22, 2025

simonmarty Jan 21, 2025

tongshen-stephanie Jan 22, 2025

simonmarty Jan 21, 2025

tongshen-stephanie Jan 22, 2025

joebaro Jan 22, 2025

tongshen-stephanie Jan 29, 2025

joebaro Jan 24, 2025

tongshen-stephanie Jan 29, 2025

joebaro Jan 25, 2025

tongshen-stephanie Jan 29, 2025

joebaro Jan 25, 2025

tongshen-stephanie Jan 29, 2025

joebaro Jan 27, 2025

tongshen-stephanie Jan 29, 2025

joebaro Jan 27, 2025

tongshen-stephanie Jan 29, 2025

joebaro Feb 4, 2025

tongshen-stephanie Feb 4, 2025

joebaro Feb 4, 2025

tongshen-stephanie Feb 4, 2025

Add support for EKS Pod Identity #416

Are you sure you want to change the base?

Add support for EKS Pod Identity #416

Conversation

tongshen-stephanie commented Jan 8, 2025 • edited by simonmarty Loading

codecov-commenter commented Jan 8, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tongshen-stephanie Jan 16, 2025 • edited Loading

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

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

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

tongshen-stephanie commented Jan 8, 2025 •

edited by simonmarty

Loading

codecov-commenter commented Jan 8, 2025 •

edited

Loading

tongshen-stephanie Jan 16, 2025 •

edited

Loading