Migrate eks discovery to aws sdk v2 #50603

creack · 2024-12-28T14:56:44Z

Removes all usage of sdk v1 for EKS.

rosstimothy · 2025-01-06T20:14:21Z

lib/kube/proxy/cluster_details.go

+// ClientGetter is an interface for getting an EKS client and an STS client.
+type ClientGetter interface {


Is this intended to only ever be for retrieving AWS clients? If so should this be renamed to reflect that.

I vote for the same

rosstimothy · 2025-01-06T20:23:30Z

lib/kube/proxy/cluster_details.go

+			// TODO(@GavinFrazar): Re-enable this when session cache v2 gets merged (#50561).
+			// awsconfig.WithoutSessionCache(),


#50561 was merged earlier today

might be worth mentioning #50809 as well, though I'm not sure it applies here - you can just call awsconfig.GetConfig, which is not cached, instead of using awsconfig.Cache.

tigrato · 2025-01-08T17:10:12Z

lib/integrations/awsoidc/eks_enroll_clusters.go

 	for k, v := range cfg.eksCluster.Tags {
-		eksTags[k] = aws.String(v)
+		eksTags[k] = v
 	}


can you use maps.Clone/Copy now?

tigrato · 2025-01-08T17:11:08Z

lib/kube/proxy/cluster_details.go

+// ClientGetter is an interface for getting an EKS client and an STS client.
+type ClientGetter interface {


I vote for the same

tigrato · 2025-01-08T17:13:15Z

lib/kube/proxy/cluster_details.go

+type EKSClient interface {
+	eks.DescribeClusterAPIClient
+}
+
+// STSClient is the subset of the STS Client interface we use.
+type STSClient interface {
+	stscreds.AssumeRoleAPIClient
+}


If our goal is to expose on those two interfaces, why not referring them directly?
This is mostly internal packages and I don't see them being expanded in the future

it is easier to have them split as one is implemented by sts.Client the other by eks.Client, used in different places.

tigrato · 2025-01-08T17:16:24Z

lib/kube/proxy/cluster_details.go

+		opts := []awsconfig.OptionsFn{
+			awsconfig.WithAmbientCredentials(),
+			// TODO(@GavinFrazar): Re-enable this when session cache v2 gets merged (#50561).
+			// awsconfig.WithoutSessionCache(),


we need this merged otherwise we will introduce the same bug #50074 again

it's not going through cache in this case, since the ClientGetter is calling the uncached awsconfig.GetConfig free function:

func (*awsClientsGetter) getAWSConfig(ctx context.Context, region string, opts ...awsconfig.OptionsFn) (aws.Config, error) { cfg, err := awsconfig.GetConfig(ctx, region, opts...) return cfg, trace.Wrap(err) }

I think the expiry time returned from GenAWSEKSToken should be taken from the underlying aws.Credentials expiry instead of the 14 minute const, but just skipping the cache works too

tigrato · 2025-01-08T17:22:15Z

lib/kube/proxy/server.go

@@ -74,6 +78,7 @@ type TLSServerConfig struct {
 	OnReconcile func(types.KubeClusters)
 	// CloudClients is a set of cloud clients that Teleport supports.
 	CloudClients cloud.Clients
+	AWSClients   *awsClientsGetter


if this is a private structure w/out any way of building it, does it make sense for the field to public?

tigrato · 2025-01-08T17:25:53Z

lib/kube/utils/eks_token_signed.go

 // GenAWSEKSToken creates an AWS token to access EKS clusters.
 // Logic from https://github.com/aws/aws-cli/blob/6c0d168f0b44136fc6175c57c090d4b115437ad1/awscli/customizations/eks/get_token.py#L211-L229
-func GenAWSEKSToken(stsClient stsiface.STSAPI, clusterID string, clock clockwork.Clock) (string, time.Time, error) {
+func GenAWSEKSToken(ctx context.Context, stsClient STSPresignClient, clusterID string, clock clockwork.Clock) (string, time.Time, error) {


now that this package was converted to sdkv2, we can make unify

teleport/lib/integrations/awsoidc/eks_enroll_clusters.go

Lines 341 to 372 in d37da51

func presignCallerIdentityURL(ctx context.Context, stsClient *sts.Client, clusterName string) (string, error) {

presignClient := sts.NewPresignClient(stsClient)

// This function adds required headers for accessing an EKS cluster to the presigned URL.

// Header "x-k8s-aws-id" specifies EKS cluster name and header "X-Amz-Expires" is just required for compatibility reasons.

addEKSHeaders := func(ctx context.Context, in middleware.BuildInput, next middleware.BuildHandler) (

out middleware.BuildOutput, metadata middleware.Metadata, err error,

) {

req, ok := in.Request.(*smithyhttp.Request)

if !ok {

return out, metadata, fmt.Errorf("unknown transport type %T", req)

}

req.Header.Add(awsHeaderClusterName, clusterName)

// 60 is put for compatibility reasons, in reality it is ignored and real expiration time is 15 minutes.

req.Header.Add(awsHeaderExpires, "60")

return next.HandleBuild(ctx, in)

}

presigned, err := presignClient.PresignGetCallerIdentity(ctx, &sts.GetCallerIdentityInput{}, func(options *sts.PresignOptions) {

options.ClientOptions = append(options.ClientOptions,

sts.WithAPIOptions(func(stack *middleware.Stack) error {

return stack.Build.Add(middleware.BuildMiddlewareFunc("AddEKSHeaders", addEKSHeaders), 0)

}))

})

if err != nil {

return "", trace.Wrap(err, "failed to presign caller identity")

}

return presigned.URL, nil

}

to use this function

They are essentially doing the same thing

maybe better in a separate PR as the integration one requires a full blown sts Client, which would be difficult to test. Added a TODO.

tigrato · 2025-01-08T17:32:37Z

lib/srv/discovery/discovery.go

@@ -115,6 +117,13 @@ type gcpInstaller interface {
 type Config struct {
 	// CloudClients is an interface for retrieving cloud clients.
 	CloudClients cloud.Clients
+
+	// FetchersClients gets the AWS clients for the given region for the fetchers.
+	FetchersClients fetchers.ClientGetter


IF we reserve FetchersClients for AWS, it should be more clear about it.
We should probably also split the cloud clients as well

This is the idea indeed, EC2 and SSM have been split out from cloud clients already, this extracts out completely EKS and partially STS. I'll rename to be more explicit about AWS.

GavinFrazar · 2025-01-08T23:22:05Z

lib/srv/discovery/discovery.go

+	c *Config
+}
+
+func (f *fetchersClientsGetter) GetAWSEKSClient(ctx context.Context, region string, opts ...awsconfig.OptionsFn) (fetchers.EKSClient, error) {


I recommend just making these into simple functions that take an aws.Config and return the relevant interface, and using the awsconfig.Provider along side them instead.

Then in tests you just set Config.AWSConfigProvider to &mocks.AWSConfigProvider{} to mock out the config loading

It's a little more tedious/ceremony because you end up doing this:

awsCfg, err := foo.getAWSConfig(...options...) if err != nil { ... } barClt := foo.GetBarClient(awsCfg)

But the benefit is that you always test the options passed to getAWSConfig, and each client can be mocked separately as a trivial function rather than trying to fake config loading in each mock client provider.

I nitpick on this because the sdk v1 cloud clients code was structured like you've done so here and I've found that tests end up faking more than they should, e.g not checking the credentials source at all.

I followed #48950 model 😅 we probably should align everything. Should it be done upfront here though, or in a separate PR?

creack changed the title ~~Migrate eks discovery to aws sdk v2~~ [wip] Migrate eks discovery to aws sdk v2 Dec 28, 2024

creack added 2 commits December 28, 2024 10:15

migrate lib/srv/discovery/fetchers/aws-sync eks to aws sdk v2

e0aac84

migrate lib/srv/discovery/fetchers eks to aws sdk v2

8c5273e

creack force-pushed the creack/eks-sdk-v2 branch from e270e36 to 6432b9a Compare December 29, 2024 18:05

migrate lib/srv/discovery eks to aws sdk v2

03fd997

creack force-pushed the creack/eks-sdk-v2 branch from 6432b9a to 03fd997 Compare December 29, 2024 19:41

migrate lib/kube/proxy eks to aws sdk v2

3f265de

creack force-pushed the creack/eks-sdk-v2 branch from e662c31 to 3f265de Compare December 30, 2024 19:01

Remove all references to EKS sdk v1.

8de9fdc

creack force-pushed the creack/eks-sdk-v2 branch from 2c56eb2 to 8de9fdc Compare December 30, 2024 21:55

creack added 6 commits January 4, 2025 13:11

Add missing aws clients in kube proxy

29e95d9

dont apply aws opts to sts client

e92f579

fix GenAWSEKSToken

11dfeb0

shot in the dark, attempt to fix db tests

1c51640

shot in the dark, attempt to fix db tests - 2

6852c7f

Cleanup

d968a0e

creack force-pushed the creack/eks-sdk-v2 branch from 7174944 to d968a0e Compare January 5, 2025 18:35

creack marked this pull request as ready for review January 5, 2025 18:41

Merge branch 'master' into creack/eks-sdk-v2

73ad618

github-actions bot added database-access Database access related issues and PRs discovery kubernetes-access labels Jan 5, 2025

github-actions bot requested a review from greedy52 January 5, 2025 18:41

github-actions bot added the size/md label Jan 5, 2025

github-actions bot requested a review from tigrato January 5, 2025 18:41

creack requested review from rosstimothy and GavinFrazar January 5, 2025 18:42

creack added the no-changelog Indicates that a PR does not require a changelog entry label Jan 5, 2025

creack changed the title ~~[wip] Migrate eks discovery to aws sdk v2~~ Migrate eks discovery to aws sdk v2 Jan 5, 2025

rosstimothy reviewed Jan 6, 2025

View reviewed changes

creack added 3 commits January 8, 2025 08:36

Merge branch 'master' into creack/eks-sdk-v2

a6d0e70

merge conflict

c5e4205

Merge branch 'master' into creack/eks-sdk-v2

81e53a8

tigrato reviewed Jan 8, 2025

View reviewed changes

creack added 3 commits January 8, 2025 15:19

Merge branch 'master' into creack/eks-sdk-v2-post

aef6d28

fix tests

5541091

Add missing call to withstsclientprovider

10b5182

GavinFrazar reviewed Jan 8, 2025

View reviewed changes

creack added 2 commits January 8, 2025 17:53

cleanup / address PR comments

70eddcc

Leverage awsconfig.Provider

bcca00e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate eks discovery to aws sdk v2 #50603

Migrate eks discovery to aws sdk v2 #50603

creack commented Dec 28, 2024 •

edited

Loading

rosstimothy Jan 6, 2025

tigrato Jan 8, 2025

rosstimothy Jan 6, 2025

GavinFrazar Jan 7, 2025 •

edited

Loading

tigrato Jan 8, 2025

tigrato Jan 8, 2025

tigrato Jan 8, 2025

creack Jan 8, 2025

tigrato Jan 8, 2025

GavinFrazar Jan 8, 2025 •

edited

Loading

tigrato Jan 8, 2025

tigrato Jan 8, 2025

tigrato Jan 8, 2025

creack Jan 8, 2025 •

edited

Loading

tigrato Jan 8, 2025

creack Jan 8, 2025

GavinFrazar Jan 8, 2025

creack Jan 8, 2025

		// ClientGetter is an interface for getting an EKS client and an STS client.
		type ClientGetter interface {

		// TODO(@GavinFrazar): Re-enable this when session cache v2 gets merged (#50561).
		// awsconfig.WithoutSessionCache(),

	func presignCallerIdentityURL(ctx context.Context, stsClient *sts.Client, clusterName string) (string, error) {
	presignClient := sts.NewPresignClient(stsClient)

	// This function adds required headers for accessing an EKS cluster to the presigned URL.
	// Header "x-k8s-aws-id" specifies EKS cluster name and header "X-Amz-Expires" is just required for compatibility reasons.
	addEKSHeaders := func(ctx context.Context, in middleware.BuildInput, next middleware.BuildHandler) (
	out middleware.BuildOutput, metadata middleware.Metadata, err error,
	) {
	req, ok := in.Request.(*smithyhttp.Request)
	if !ok {
	return out, metadata, fmt.Errorf("unknown transport type %T", req)
	}

	req.Header.Add(awsHeaderClusterName, clusterName)
	// 60 is put for compatibility reasons, in reality it is ignored and real expiration time is 15 minutes.
	req.Header.Add(awsHeaderExpires, "60")

	return next.HandleBuild(ctx, in)
	}

	presigned, err := presignClient.PresignGetCallerIdentity(ctx, &sts.GetCallerIdentityInput{}, func(options *sts.PresignOptions) {
	options.ClientOptions = append(options.ClientOptions,
	sts.WithAPIOptions(func(stack *middleware.Stack) error {
	return stack.Build.Add(middleware.BuildMiddlewareFunc("AddEKSHeaders", addEKSHeaders), 0)
	}))
	})
	if err != nil {
	return "", trace.Wrap(err, "failed to presign caller identity")
	}

	return presigned.URL, nil
	}

Migrate eks discovery to aws sdk v2 #50603

Are you sure you want to change the base?

Migrate eks discovery to aws sdk v2 #50603

Conversation

creack commented Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinFrazar Jan 7, 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

GavinFrazar Jan 8, 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

creack Jan 8, 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

creack commented Dec 28, 2024 •

edited

Loading

GavinFrazar Jan 7, 2025 •

edited

Loading

GavinFrazar Jan 8, 2025 •

edited

Loading

creack Jan 8, 2025 •

edited

Loading