Migrate ECS client to aws-sdk-go-v2 #4447

tinnywang · 2024-12-05T07:52:02Z

Summary

Migrate ECS client to aws-sdk-go-v2.

#4406 added our bespoke codegen'd aws-sdk-go-v2 ECS client. This PR integrates the v2 ECS client with Agent and removes all uses of our v1 ECS client.

Implementation details

In go.mod for the agent and ecs-agent modules,

add a dependency ongithub.com/aws/aws-sdk-go-v2/service/ecs
add a replace directive so that we use our bespoke ECS package instead of the official SDK package

replace github.com/aws/aws-sdk-go-v2/service/ecs => ../aws-sdk-go-v2/service/ecs

Update ECS client construction per the docs.

Replace all uses of our v1 ECS client with the v2 ECS client.

Package aws/aws-sdk-go/service/ecs is now split between aws/aws-sdk-go-v2/service/ecs and aws/aws-sdk-go-v2/service/ecs/types.
Many pointer types are now non-pointers (e.g. ContainerStateChange.NetworkBindings changed from []*NetworkBinding to []NetworkBinding). See the docs for more info.
Many int64 types are now int32 (e.g. NetworkBinding.ContainerPort and NetworkBinding.HostPort changed from *int64 to *int32).
Many string constants now have their own enum types aliased to string (e.g. the AgentUpdateStatus* constants changed from string to type AgentUpdateStatus.
ErrCode*Exception string constants have been replaced with Exception types (e.g. ErrCodeAccessDeniedException ("AccessDeniedException") now corresponds to the AccessDeniedException type).

Update error handling to handle SDKv1 and SDKv2 errors. v1 error handling can be removed after all clients have been migrated to aws-sdk-go-v2. See the docs for more info on error handling in v2.

aws.Error is analogous to smithy.APIError.
Service-specific errors that were identifiable via string error codes now have modeled error types (compare this v1 example with this v2 example).
aws.RequestFailure, which includes the request ID, is analogous to http.ResponseError (see the docs for more info).

Rewrite the oneDayRetrier to be compatible with aws-sdk-go-v2 per the docs.

Testing

GitHub checks pass.

New tests cover the changes: no

Description for the changelog

Migrate ECS client toaws-sdk-go-v2.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

singholt · 2025-01-27T23:52:25Z

ecs-agent/utils/utils.go

@@ -64,12 +53,20 @@ func Uint16SliceToStringSlice(slice []uint16) []string {
 	return stringSlice
 }

-// Int64PtrToIntPtr converts a *int64 to *int.
-func Int64PtrToIntPtr(int64ptr *int64) *int {
+// Int32PtrToIntPtr converts a *int64 to *int.


// Int32PtrToIntPtr converts a *int32 to *int.

singholt · 2025-01-27T23:55:21Z

ecs-agent/utils/utils_test.go

@@ -83,36 +83,28 @@ func TestUint16SliceToStringSlice(t *testing.T) {
 			stringSlice := Uint16SliceToStringSlice(tc.param)
 			assert.Equal(t, tc.expected, len(stringSlice), tc.name)

-			stringPtrSlice := Uint16SliceToStringPtrSlice(tc.param)


Can we remove this unit test entirely, since the function Uint16SliceToStringPtrSlice is removed? And add a new unit test for the new Int64PtrToInt32Ptr method?

sure, will add it

singholt · 2025-01-28T00:18:07Z

ecs-agent/api/errors/errors.go

 	"fmt"
 	"strings"

+	"github.com/aws/aws-sdk-go-v2/service/ecs/types"
 	"github.com/aws/aws-sdk-go/aws/awserr"


we need to move to using this awserr stuff from aws-sdk-v2 too?

yes, v1 error handling will be removed after all clients have been migrated to aws-sdk-go-v2.

I see, thanks. Can we add a comment to both the modified methods below about this? It isnt immediately clear why we do error handling both v1 and v2 way right now.

sure, will add

ecs-agent/api/errors/errors.go

danehlim · 2025-01-31T20:59:13Z

ecs-agent/api/ecs/client/retry_handler.go

 	return client
 }

 // oneDayRetrier is a retrier for the AWS SDK that retries up to one day.
 // Each retry will have an exponential backoff from 30ms to 5 minutes. Once the
 // backoff has reached 5 minutes, it will not increase further.
-// Conforms to the request.Retryer interface https://github.com/aws/aws-sdk-go/blob/v1.0.0/aws/request/retryer.go#L13
+// Confirms to the aws.Retryer interface https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer


nit: I think "Conforms" should be the correct word choice here

danehlim · 2025-01-31T21:34:25Z

ecs-agent/netlib/platform/common_linux.go

 	}

 	return &tasknetworkconfig.TaskNetworkConfig{
 		NetworkNamespaces: netNSs,
-		NetworkMode:       mode,
+		NetworkMode:       types.NetworkMode(mode),


Isn't mode already type types.NetworkMode here? Type conversion seems unnecessary here, let's remove it.

danehlim · 2025-02-03T18:18:32Z

agent/api/statechange.go

@@ -481,7 +481,7 @@ func (AttachmentStateChange) GetEventType() statechange.EventType {
 	return statechange.AttachmentEvent
 }

-func buildManagedAgentStateChangePayload(change ManagedAgentStateChange) *ecsmodel.ManagedAgentStateChange {
+func buildManagedAgentStateChangePayload(change ManagedAgentStateChange) *types.ManagedAgentStateChange {


Is there any specific reason why we don't just return type types.ManagedAgentStateChange here? Returning pointer type *types.ManagedAgentStateChange here seems unnecessary to me (i.e., we just end up dereferencing on L427 anyway), but I may be missing something. It seems like in most other places in this pull request we replace *ecsmodel.<INSERT TYPE HERE> with types.<INSERT TYPE HERE> as opposed to *types.<INSERT TYPE HERE>.

danehlim · 2025-02-03T18:19:29Z

agent/api/statechange.go

 		ContainerName:    aws.String(change.Container.Name),
 		Status:           aws.String(change.Status.String()),
 		Reason:           aws.String(change.Reason),
 	}
 }

-func buildContainerStateChangePayload(change ContainerStateChange) (*ecsmodel.ContainerStateChange, error) {
+func buildContainerStateChangePayload(change ContainerStateChange) (*types.ContainerStateChange, error) {


Same comment as this previous comment applies here too.

danehlim · 2025-02-03T18:49:33Z

agent/utils/utils_test.go

+			res: 400,
+		},
+		{
+			name: "TestGetRequestFailureStatusCodeSuccess SDKv2",


Test case names on L152 and L165 should be swapped (or alternatively, L154-L162 and L166-L167 should be swapped).

singholt · 2025-01-28T00:38:04Z

agent/handlers/v2/response.go

+	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/aws/smithy-go"
+	"github.com/cihub/seelog"
+	"github.com/pkg/errors"


this import block should go below with other external imports. import order -

standard imports, 2. ecs agent imports, 3. external imports

singholt · 2025-02-03T18:50:16Z

ecs-agent/tmds/handlers/taskprotection/v1/handlers/handlers.go

+
+		logger.Error(fmt.Sprintf(
+			"got a %T exception that does not implement ResponseError. This should not happen, return statusCode 500 for whatever errorCode. Original err: %v.",
+			err, err))


logging the err twice in the same log statement doesn't help much. I would suggest removing the first one and just logging the error under "Original err:"

logger.Error(fmt.Sprintf("Got an exception that does not implement ResponseError. This should not happen, return statusCode 500 for whatever errorCode. Original err: %v.",err))

also, can you update all logger.Error statements in this file to start with a capitalized letter?

singholt · 2025-02-03T18:50:38Z

ecs-agent/tmds/handlers/taskprotection/v1/handlers/handlers.go

+	awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http"
 	"github.com/aws/aws-sdk-go-v2/service/ecs"
 	ecstypes "github.com/aws/aws-sdk-go-v2/service/ecs/types"


these sdk imports should go along with the other external import block below.

applicable to multiple files throughout this PR, please take a look

singholt · 2025-02-03T19:03:50Z

ecs-agent/tmds/handlers/taskprotection/v1/handlers/handlers.go

}
+

please add a comment here that this is the aws-sdk-go-v2 error handling and the above error handling (L407-422) will be removed once we complete our migration.

singholt · 2025-02-03T19:06:32Z

ecs-agent/tmds/handlers/taskprotection/v1/handlers/handlers.go

@@ -59,6 +61,10 @@ type TaskProtectionRequest struct {
 	ExpiresInMinutes  *int64
 }

+type CanceledError interface {


please add a comment explaining what this is useful for, and a link to https://pkg.go.dev/github.com/aws/smithy-go#CanceledError.CanceledError

tinnywang added the bot/test label Dec 5, 2024

amazon-ecs-bot removed the bot/test label Dec 5, 2024

tinnywang added the bot/test label Dec 5, 2024

amazon-ecs-bot removed the bot/test label Dec 5, 2024

tinnywang force-pushed the aws-sdk-go-v2/ecs branch 2 times, most recently from 6b18859 to a1a9480 Compare December 6, 2024 08:34

tinnywang added the bot/test label Dec 6, 2024

amazon-ecs-bot removed the bot/test label Dec 6, 2024

tinnywang marked this pull request as ready for review December 6, 2024 18:06

tinnywang requested a review from a team as a code owner December 6, 2024 18:06

tinnywang added the bot/test label Dec 10, 2024

amazon-ecs-bot removed the bot/test label Dec 10, 2024

tinnywang force-pushed the aws-sdk-go-v2/ecs branch 2 times, most recently from 1c2d4b8 to 7d2a689 Compare December 10, 2024 11:19

tinnywang added the bot/test label Dec 10, 2024

amazon-ecs-bot removed the bot/test label Dec 10, 2024

Yiyuanzzz previously approved these changes Dec 13, 2024

View reviewed changes

tinnywang added 13 commits December 18, 2024 19:38

migrate ecs-agent/api/ecs

d763eee

migrate ecs-agent/utils

d7f7a27

migrate ecs-agent/tmds/handlers/taskprotection/v1

e64f752

migrate agent module

99a016e

migrate ECS client credential provider

98226e0

ecs-agent/api/errors package handles SDK v1 and v2 errors

bdce259

regenerate code

ac956e9

handle SDK v2 errors in task protection handlers

c3cc11b

fix tests

bea4e89

migrate remaining packages

5124bd9

remove SDK v1 ECS client

b7ab4bb

cleanup

76f9073

update more error handling

50f1c9a

fix windows test

c5bf057

tinnywang force-pushed the aws-sdk-go-v2/ecs branch from 7d2a689 to 0d3c3a7 Compare December 18, 2024 19:38

tinnywang added the bot/test label Dec 18, 2024

amazon-ecs-bot removed the bot/test label Dec 18, 2024

Yiyuanzzz added the bot/test label Jan 21, 2025

amazon-ecs-bot removed the bot/test label Jan 21, 2025

Yiyuanzzz dismissed their stale review via 47db0c7 January 23, 2025 19:38

Yiyuanzzz added the bot/test label Jan 23, 2025

amazon-ecs-bot removed the bot/test label Jan 23, 2025

Yiyuanzzz force-pushed the aws-sdk-go-v2/ecs branch 2 times, most recently from 84b24b0 to 874dc29 Compare January 23, 2025 22:29

fix after rebase

447b0eb

Yiyuanzzz force-pushed the aws-sdk-go-v2/ecs branch from 874dc29 to 447b0eb Compare January 23, 2025 22:38

Merge branch 'dev' into aws-sdk-go-v2/ecs

443e1d5

Yiyuanzzz added the bot/test label Jan 23, 2025

amazon-ecs-bot removed the bot/test label Jan 23, 2025

singholt reviewed Jan 27, 2025

View reviewed changes

singholt reviewed Jan 28, 2025

View reviewed changes

Yiyuanzzz added 2 commits January 29, 2025 12:11

Merge branch 'dev' into aws-sdk-go-v2/ecs

029b479

Merge branch 'dev' into aws-sdk-go-v2/ecs

6b5700a

Yiyuanzzz added the bot/test label Jan 31, 2025

amazon-ecs-bot removed the bot/test label Jan 31, 2025

danehlim reviewed Jan 31, 2025

View reviewed changes

danehlim reviewed Feb 3, 2025

View reviewed changes

singholt reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate ECS client to aws-sdk-go-v2 #4447

Migrate ECS client to aws-sdk-go-v2 #4447

tinnywang commented Dec 5, 2024 •

edited

Loading

singholt Jan 27, 2025

singholt Jan 27, 2025

Yiyuanzzz Jan 28, 2025

singholt Jan 28, 2025

Yiyuanzzz Jan 28, 2025

singholt Jan 28, 2025 •

edited

Loading

Yiyuanzzz Jan 28, 2025

danehlim Jan 31, 2025

danehlim Jan 31, 2025

danehlim Feb 3, 2025 •

edited

Loading

danehlim Feb 3, 2025

danehlim Feb 3, 2025

singholt Jan 28, 2025

singholt Feb 3, 2025

singholt Feb 3, 2025

singholt Feb 3, 2025

singholt Feb 3, 2025

singholt Feb 3, 2025

singholt Feb 3, 2025

Migrate ECS client to aws-sdk-go-v2 #4447

Are you sure you want to change the base?

Migrate ECS client to aws-sdk-go-v2 #4447

Conversation

tinnywang commented Dec 5, 2024 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

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

singholt Jan 28, 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

danehlim Feb 3, 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

tinnywang commented Dec 5, 2024 •

edited

Loading

singholt Jan 28, 2025 •

edited

Loading

danehlim Feb 3, 2025 •

edited

Loading