deps: upgrade aws-sdk-go from v1 to v2 #24720

mismithhisler · 2024-12-19T16:04:41Z

Description

Upgrades aws-sdk-go from v1 to v2 as the v1 library is in maintenance mode.

Closes GH #24680

Testing & Reproduction steps

Links

Contributor Checklist

Changelog Entry If this PR changes user-facing behavior, please generate and add a
changelog entry using the make cl command.
Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
ensure regressions will be caught.
Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
and job configuration, please update the Nomad website documentation to reflect this. Refer to
the website README for docs guidelines. Please also consider whether the
change requires notes within the upgrade guide.

Reviewer Checklist

Backport Labels Please add the correct backport labels as described by the internal
backporting document.
Commit Type Ensure the correct merge method is selected which should be "squash and merge"
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
within the public repository.

tgross · 2024-12-19T16:09:31Z

@mismithhisler can you check the impact of this change on binary size? In #24701 I noted that updating this library doesn't update the dependencies that have it as a transient dependency for v1, and so we'll be carrying around both versions.

mismithhisler · 2024-12-19T18:29:21Z

@mismithhisler can you check the impact of this change on binary size? In #24701 I noted that updating this library doesn't update the dependencies that have it as a transient dependency for v1, and so we'll be carrying around both versions.

@tgross I saw that and using go mod why it looks indirectly needed via the go-getter library.

It looks like it added 2.7MB to the binary size.

tgross · 2024-12-19T18:36:15Z

It looks like it added 2.7MB to the binary size.

Eh, ok. That stinks but not worth delaying the fix for. It'd be nice if we could nudge along the other HashiCorp libraries that are pulling this in.

tgross

LGTM!

tgross · 2024-12-19T19:18:02Z

client/fingerprint/env_aws.go

+	ctx, cancel := context.WithTimeout(context.TODO(), timeout)
+	defer cancel()
+
+	imdsClient, err := imdsClient(ctx, f.endpoint)


Where does f.endpoint actually get set if we're no longer configuring it in the constructor? It looks like there's possibly fallback behavior if it's empty (maybe the client library reads the env var if set?), but are we ever using the endpoint field if we're falling back to that?

I was actually meaning to leave a comment around this. I took it out here because from what I can tell, AWS makes no mention of AWS_ENV_URL, and I don't think you can specify custom IMDS endpoints. So that really just leaves overriding the endpoint for testing.

+1 on some comments here; took a moment of digging around to see what was happening, otherwise it looks a bit suspect on the surface.

tgross · 2024-12-19T19:19:04Z

e2e/remotetasks/remotetasks.go

@@ -4,14 +4,15 @@
 package remotetasks


Should we just remove these tests and the related ECS infra? Let's see what @schmichael thinks. (But probably in a separate PR anyways.)

+1 as the feature is deprecated unless we want to keep this in for future reference of potential.

Please remove. We removed RTD from the docs and archived the repo. Not a bad idea, but incomplete and lacking in demand.

jrasell

Thanks for doing this @mismithhisler! I've added a couple of inline comments/questions which would be good to resolve before merging but looks and works great.

I ran this locally on macOS and everything looked OK, with the correct message showing up in the logs:

client.fingerprint_mgr.env_aws: error querying AWS IDMS URL, skipping

I then ran this PR on an AWS cluster and the node attributes looked how I would expect:

$ nomad node status -verbose bd306835 |grep aws
kernel.version                           = 6.8.0-1009-aws
platform.aws.ami-id                      = ami-0474244c88b835731
platform.aws.instance-life-cycle         = on-demand
platform.aws.instance-type               = m5.xlarge
platform.aws.placement.availability-zone = eu-west-2a
unique.platform.aws.hostname             = ip-10-0-1-15.eu-west-2.compute.internal
unique.platform.aws.instance-id          = i-0765dd85a71bc86bb
unique.platform.aws.local-hostname       = ip-10-0-1-15.eu-west-2.compute.internal
unique.platform.aws.local-ipv4           = 10.0.1.15
unique.platform.aws.mac                  = 06:72:45:96:05:15
unique.platform.aws.public-ipv4          = xx.xxx.xxx.xxx

The output from the same Nomad client running the official v1.9.4 release:

$ nomad node status -verbose bd306835 |grep aws
kernel.version                           = 6.8.0-1009-aws
platform.aws.ami-id                      = ami-0474244c88b835731
platform.aws.instance-life-cycle         = on-demand
platform.aws.instance-type               = m5.xlarge
platform.aws.placement.availability-zone = eu-west-2a
unique.platform.aws.hostname             = ip-10-0-1-15.eu-west-2.compute.internal
unique.platform.aws.instance-id          = i-0765dd85a71bc86bb
unique.platform.aws.local-hostname       = ip-10-0-1-15.eu-west-2.compute.internal
unique.platform.aws.local-ipv4           = 10.0.1.15
unique.platform.aws.mac                  = 06:72:45:96:05:15
unique.platform.aws.public-ipv4          = xx.xxx.xxx.xxx

jrasell · 2024-12-20T06:46:53Z

e2e/remotetasks/remotetasks.go

@@ -4,14 +4,15 @@
 package remotetasks


+1 as the feature is deprecated unless we want to keep this in for future reference of potential.

jrasell · 2024-12-20T06:57:20Z

client/fingerprint/env_aws.go

+// See https://aws.github.io/aws-sdk-go-v2/docs/handling-errors for
+// recommended error handling with aws-sdk-go-v2.
+// See also: https://github.com/aws/aws-sdk-go-v2/issues/1306
+func (f *EnvAWSFingerprint) handleImdsError(err error, attr string) error {


Mind adding a small table driven unit test to cover this functionality?

jrasell · 2024-12-20T07:05:58Z

client/fingerprint/env_aws.go

+	ctx, cancel := context.WithTimeout(context.TODO(), timeout)
+	defer cancel()
+
+	imdsClient, err := imdsClient(ctx, f.endpoint)


+1 on some comments here; took a moment of digging around to see what was happening, otherwise it looks a bit suspect on the surface.

jrasell · 2024-12-20T07:11:15Z

client/fingerprint/env_aws.go

-				return nil
-			}
+		}
+		defer resp.Content.Close()


This defer is inside a loop, so won't get executed until the iteration finishes and we reach the function end; is this OK? If we wanted to change this, we could move the loop body into it's own function.

Great catch! I'll get this updated.

jrasell · 2024-12-20T07:12:42Z

client/fingerprint/env_aws.go

+		v := strings.TrimSpace(string(bytes))
+		if v == "" {
+			f.logger.Debug("read an empty value", "attribute", k)
+		}


Do we want a continue statement after the log line here which will match the existing behaviour and mean we do not have empty string attributes in node data? It would also be nice if we do need this continue to craft a test to catch this in the future, although I don't know if that is possible or feasible.

There is a test that mimics empty response data, so I've updated it to make sure the corresponding key is not in the node attributes. I didn't see IMDS respond with empty values, but instead it removes the endpoint available, resulting in a 404. It's probably good to handle this just in case though.

jrasell · 2024-12-20T07:16:23Z

client/fingerprint/env_aws.go

-			continue
-		} else if awsErr, ok := err.(awserr.RequestFailure); ok {
-			f.logger.Debug("could not read attribute value", "attribute", k, "error", awsErr)
+		resp, err := imdsClient.GetMetadata(ctx, &imds.GetMetadataInput{


Outside the scope for this PR but I wonder if there would be any benefit in the future from not passing individual keys to the metadata call, and rather get the whole response and process that. The benefit would be a single API call at the cost of more complex response processing.

This would be nice, but I'm not sure it's possible with the current IMDS. If you pass in an empty path, you just get a list of available paths, not all the instance metadata.

pkazmierczak · 2024-12-20T08:54:20Z

go.mod

@@ -175,6 +179,16 @@ require (
 	github.com/apparentlymart/go-textseg/v13 v13.0.0 // indirect
 	github.com/apparentlymart/go-textseg/v15 v15.0.0 // indirect
 	github.com/armon/go-radix v1.0.0 // indirect
+	github.com/aws/aws-sdk-go v1.55.5 // indirect


why do we still need this? Is it a go mod tidy snafu or something actually pulls it as a dependency?

This is go-discover et al.

Also go-getter, I opened an issue to see if we can get the AWS SDK updated in that repo. Would be nice to get this dependency removed.

deps: upgrade aws-sdk-go from v1 to v2

a1b3d73

add back endpoint for testing

97ae9db

vercel bot deployed to Preview – nomad-ui December 19, 2024 17:58 View deployment

mismithhisler marked this pull request as ready for review December 19, 2024 19:07

mismithhisler requested review from a team as code owners December 19, 2024 19:07

mismithhisler requested review from shoenig, tgross and jrasell December 19, 2024 19:07

tgross previously approved these changes Dec 19, 2024

View reviewed changes

close all io.ReadClosers

5a33062

mismithhisler dismissed tgross’s stale review via 5a33062 December 19, 2024 21:44

vercel bot deployed to Preview – nomad-ui December 19, 2024 21:45 View deployment

jrasell reviewed Dec 20, 2024

View reviewed changes

pkazmierczak reviewed Dec 20, 2024

View reviewed changes

refactor handling imds response

88a457c

vercel bot deployed to Preview – nomad-ui December 20, 2024 17:35 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: upgrade aws-sdk-go from v1 to v2 #24720

deps: upgrade aws-sdk-go from v1 to v2 #24720

mismithhisler commented Dec 19, 2024

tgross commented Dec 19, 2024

mismithhisler commented Dec 19, 2024 •

edited

Loading

tgross commented Dec 19, 2024

tgross left a comment

tgross Dec 19, 2024

mismithhisler Dec 19, 2024

jrasell Dec 20, 2024

tgross Dec 19, 2024

jrasell Dec 20, 2024

schmichael Dec 20, 2024

jrasell left a comment

jrasell Dec 20, 2024

jrasell Dec 20, 2024

jrasell Dec 20, 2024

jrasell Dec 20, 2024

mismithhisler Dec 20, 2024

jrasell Dec 20, 2024

mismithhisler Dec 20, 2024 •

edited

Loading

jrasell Dec 20, 2024

mismithhisler Dec 20, 2024

pkazmierczak Dec 20, 2024

jrasell Dec 20, 2024

mismithhisler Dec 20, 2024

deps: upgrade aws-sdk-go from v1 to v2 #24720

Are you sure you want to change the base?

deps: upgrade aws-sdk-go from v1 to v2 #24720

Conversation

mismithhisler commented Dec 19, 2024

Description

Testing & Reproduction steps

Links

Contributor Checklist

Reviewer Checklist

tgross commented Dec 19, 2024

mismithhisler commented Dec 19, 2024 • edited Loading

tgross commented Dec 19, 2024

tgross left a 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

jrasell left a 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

mismithhisler Dec 20, 2024 • 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

mismithhisler commented Dec 19, 2024 •

edited

Loading

mismithhisler Dec 20, 2024 •

edited

Loading