From 94e171b5b4624728d69c713a76876cc80b5f43db Mon Sep 17 00:00:00 2001 From: EC2 Default User Date: Tue, 5 Sep 2023 19:19:12 +0000 Subject: [PATCH 1/4] Fix: gMSA s3 test * Update NewS3Client * Updated mocks * Updated unit tests --- agent/s3/factory/factory.go | 14 ++++++++++---- agent/s3/factory/mocks/factory_mocks.go | 11 ++++++----- .../credentialspec/credentialspec_linux.go | 2 +- .../credentialspec/credentialspec_linux_test.go | 6 +++--- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/agent/s3/factory/factory.go b/agent/s3/factory/factory.go index b5111439514..716c61ee215 100644 --- a/agent/s3/factory/factory.go +++ b/agent/s3/factory/factory.go @@ -34,7 +34,7 @@ const ( type S3ClientCreator interface { NewS3ManagerClient(bucket, region string, creds credentials.IAMRoleCredentials) (s3client.S3ManagerClient, error) - NewS3Client(region string, creds credentials.IAMRoleCredentials) s3client.S3Client + NewS3Client(bucket, region string, creds credentials.IAMRoleCredentials) (s3client.S3Client, error) } // NewS3ClientCreator provide 2 implementations @@ -65,15 +65,21 @@ func (*s3ClientCreator) NewS3ManagerClient(bucket, region string, } // NewS3Client returns a new S3 client to support s3 operations which are not provided by s3manager. -func (*s3ClientCreator) NewS3Client(region string, - creds credentials.IAMRoleCredentials) s3client.S3Client { +func (*s3ClientCreator) NewS3Client(bucket, region string, + creds credentials.IAMRoleCredentials) (s3client.S3Client, error) { cfg := aws.NewConfig(). WithHTTPClient(httpclient.New(roundtripTimeout, false)). WithCredentials( awscreds.NewStaticCredentials(creds.AccessKeyID, creds.SecretAccessKey, creds.SessionToken)).WithRegion(region) sess := session.Must(session.NewSession(cfg)) - return s3.New(sess) + svc := s3.New(sess) + bucketRegion, err := getRegionFromBucket(svc, bucket) + if err != nil { + return nil, err + } + sessWithRegion := session.Must(session.NewSession(cfg.WithRegion(bucketRegion))) + return s3.New(sessWithRegion), nil } func getRegionFromBucket(svc *s3.S3, bucket string) (string, error) { input := &s3.GetBucketLocationInput{ diff --git a/agent/s3/factory/mocks/factory_mocks.go b/agent/s3/factory/mocks/factory_mocks.go index 0cd1e64aa35..29e7af44089 100644 --- a/agent/s3/factory/mocks/factory_mocks.go +++ b/agent/s3/factory/mocks/factory_mocks.go @@ -50,17 +50,18 @@ func (m *MockS3ClientCreator) EXPECT() *MockS3ClientCreatorMockRecorder { } // NewS3Client mocks base method. -func (m *MockS3ClientCreator) NewS3Client(arg0 string, arg1 credentials.IAMRoleCredentials) s3.S3Client { +func (m *MockS3ClientCreator) NewS3Client(arg0, arg1 string, arg2 credentials.IAMRoleCredentials) (s3.S3Client, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewS3Client", arg0, arg1) + ret := m.ctrl.Call(m, "NewS3Client", arg0, arg1, arg2) ret0, _ := ret[0].(s3.S3Client) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // NewS3Client indicates an expected call of NewS3Client. -func (mr *MockS3ClientCreatorMockRecorder) NewS3Client(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockS3ClientCreatorMockRecorder) NewS3Client(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3Client", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3Client), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3Client", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3Client), arg0, arg1, arg2) } // NewS3ManagerClient mocks base method. diff --git a/agent/taskresource/credentialspec/credentialspec_linux.go b/agent/taskresource/credentialspec/credentialspec_linux.go index fc293312a6c..c2ea4fdeb81 100644 --- a/agent/taskresource/credentialspec/credentialspec_linux.go +++ b/agent/taskresource/credentialspec/credentialspec_linux.go @@ -435,7 +435,7 @@ func (cs *CredentialSpecResource) handleS3CredentialspecFile(originalCredentialS return } - s3Client := cs.s3ClientCreator.NewS3Client(cs.region, iamCredentials) + s3Client, err := cs.s3ClientCreator.NewS3Client(bucket, cs.region, iamCredentials) credSpecJsonStringUnformatted, err := s3.GetObject(bucket, key, s3Client) diff --git a/agent/taskresource/credentialspec/credentialspec_linux_test.go b/agent/taskresource/credentialspec/credentialspec_linux_test.go index dbe11e5d2b2..a7931a182c6 100644 --- a/agent/taskresource/credentialspec/credentialspec_linux_test.go +++ b/agent/taskresource/credentialspec/credentialspec_linux_test.go @@ -374,7 +374,7 @@ func TestHandleS3CredentialSpecFileGetS3SecretValue(t *testing.T) { Body: io.NopCloser(strings.NewReader(testData)), } gomock.InOrder( - s3ClientCreator.EXPECT().NewS3Client(gomock.Any(), gomock.Any()).Return(mockS3Client), + s3ClientCreator.EXPECT().NewS3Client(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockS3Client, nil), mockS3Client.EXPECT().GetObject(gomock.Any()).Return(s3GetObjectResponse, nil).Times(1), ) @@ -439,7 +439,7 @@ func TestHandleS3DomainlessCredentialSpecFileGetS3SecretValue(t *testing.T) { Body: io.NopCloser(strings.NewReader(testData)), } gomock.InOrder( - s3ClientCreator.EXPECT().NewS3Client(gomock.Any(), gomock.Any()).Return(mockS3Client), + s3ClientCreator.EXPECT().NewS3Client(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockS3Client, nil), mockS3Client.EXPECT().GetObject(gomock.Any()).Return(s3GetObjectResponse, nil).Times(1), ) @@ -501,7 +501,7 @@ func TestHandleS3CredentialSpecFileGetS3SecretValueErr(t *testing.T) { }, apitaskstatus.TaskStatusNone, apitaskstatus.TaskRunning) gomock.InOrder( - s3ClientCreator.EXPECT().NewS3Client(gomock.Any(), gomock.Any()).Return(mockS3Client), + s3ClientCreator.EXPECT().NewS3Client(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockS3Client, nil), mockS3Client.EXPECT().GetObject(gomock.Any()).Return(nil, errors.New("test-error")).Times(1), ) From 3aaee10c4d117ca99e90a9c8f2a637893c6ddb29 Mon Sep 17 00:00:00 2001 From: EC2 Default User Date: Wed, 6 Sep 2023 04:01:51 +0000 Subject: [PATCH 2/4] Create mocks for modified S3ClientCreator interface --- agent/s3/factory/mocks/factory_mocks.go | 43 +++++++++++++------------ 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/agent/s3/factory/mocks/factory_mocks.go b/agent/s3/factory/mocks/factory_mocks.go index 29e7af44089..8e1a2223a0c 100644 --- a/agent/s3/factory/mocks/factory_mocks.go +++ b/agent/s3/factory/mocks/factory_mocks.go @@ -1,19 +1,22 @@ -// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// // Copyright 2015-2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// // +// // Licensed under the Apache License, Version 2.0 (the "License"). You may +// // not use this file except in compliance with the License. A copy of the +// // License is located at +// // +// // http://aws.amazon.com/apache2.0/ +// // +// // or in the "license" file accompanying this file. This file is distributed +// // on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// // express or implied. See the License for the specific language governing +// // permissions and limitations under the License. +// // +// // Source: dependencies.go in package cache // -// Licensed under the Apache License, Version 2.0 (the "License"). You may -// not use this file except in compliance with the License. A copy of the -// License is located at -// -// http://aws.amazon.com/apache2.0/ -// -// or in the "license" file accompanying this file. This file is distributed -// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either -// express or implied. See the License for the specific language governing -// permissions and limitations under the License. // // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/aws/amazon-ecs-agent/agent/s3/factory (interfaces: S3ClientCreator) +// Source: github.com/aws/amazon-ecs-agent/agent/s3/factory/factory.go // Package mock_factory is a generated GoMock package. package mock_factory @@ -50,31 +53,31 @@ func (m *MockS3ClientCreator) EXPECT() *MockS3ClientCreatorMockRecorder { } // NewS3Client mocks base method. -func (m *MockS3ClientCreator) NewS3Client(arg0, arg1 string, arg2 credentials.IAMRoleCredentials) (s3.S3Client, error) { +func (m *MockS3ClientCreator) NewS3Client(bucket, region string, creds credentials.IAMRoleCredentials) (s3.S3Client, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewS3Client", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "NewS3Client", bucket, region, creds) ret0, _ := ret[0].(s3.S3Client) ret1, _ := ret[1].(error) return ret0, ret1 } // NewS3Client indicates an expected call of NewS3Client. -func (mr *MockS3ClientCreatorMockRecorder) NewS3Client(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockS3ClientCreatorMockRecorder) NewS3Client(bucket, region, creds interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3Client", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3Client), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3Client", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3Client), bucket, region, creds) } // NewS3ManagerClient mocks base method. -func (m *MockS3ClientCreator) NewS3ManagerClient(arg0, arg1 string, arg2 credentials.IAMRoleCredentials) (s3.S3ManagerClient, error) { +func (m *MockS3ClientCreator) NewS3ManagerClient(bucket, region string, creds credentials.IAMRoleCredentials) (s3.S3ManagerClient, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewS3ManagerClient", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "NewS3ManagerClient", bucket, region, creds) ret0, _ := ret[0].(s3.S3ManagerClient) ret1, _ := ret[1].(error) return ret0, ret1 } // NewS3ManagerClient indicates an expected call of NewS3ManagerClient. -func (mr *MockS3ClientCreatorMockRecorder) NewS3ManagerClient(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockS3ClientCreatorMockRecorder) NewS3ManagerClient(bucket, region, creds interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3ManagerClient", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3ManagerClient), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3ManagerClient", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3ManagerClient), bucket, region, creds) } From 6830ce22f3fef6b97e2143e926f607a17e5c2d61 Mon Sep 17 00:00:00 2001 From: EC2 Default User Date: Thu, 7 Sep 2023 19:20:22 +0000 Subject: [PATCH 3/4] Modify mocks for S3ClientCreator interface --- agent/s3/factory/mocks/factory_mocks.go | 43 ++++++++++++------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/agent/s3/factory/mocks/factory_mocks.go b/agent/s3/factory/mocks/factory_mocks.go index 8e1a2223a0c..29e7af44089 100644 --- a/agent/s3/factory/mocks/factory_mocks.go +++ b/agent/s3/factory/mocks/factory_mocks.go @@ -1,22 +1,19 @@ -// // Copyright 2015-2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. -// // -// // Licensed under the Apache License, Version 2.0 (the "License"). You may -// // not use this file except in compliance with the License. A copy of the -// // License is located at -// // -// // http://aws.amazon.com/apache2.0/ -// // -// // or in the "license" file accompanying this file. This file is distributed -// // on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either -// // express or implied. See the License for the specific language governing -// // permissions and limitations under the License. -// // -// // Source: dependencies.go in package cache +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. // +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. // // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/aws/amazon-ecs-agent/agent/s3/factory/factory.go +// Source: github.com/aws/amazon-ecs-agent/agent/s3/factory (interfaces: S3ClientCreator) // Package mock_factory is a generated GoMock package. package mock_factory @@ -53,31 +50,31 @@ func (m *MockS3ClientCreator) EXPECT() *MockS3ClientCreatorMockRecorder { } // NewS3Client mocks base method. -func (m *MockS3ClientCreator) NewS3Client(bucket, region string, creds credentials.IAMRoleCredentials) (s3.S3Client, error) { +func (m *MockS3ClientCreator) NewS3Client(arg0, arg1 string, arg2 credentials.IAMRoleCredentials) (s3.S3Client, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewS3Client", bucket, region, creds) + ret := m.ctrl.Call(m, "NewS3Client", arg0, arg1, arg2) ret0, _ := ret[0].(s3.S3Client) ret1, _ := ret[1].(error) return ret0, ret1 } // NewS3Client indicates an expected call of NewS3Client. -func (mr *MockS3ClientCreatorMockRecorder) NewS3Client(bucket, region, creds interface{}) *gomock.Call { +func (mr *MockS3ClientCreatorMockRecorder) NewS3Client(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3Client", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3Client), bucket, region, creds) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3Client", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3Client), arg0, arg1, arg2) } // NewS3ManagerClient mocks base method. -func (m *MockS3ClientCreator) NewS3ManagerClient(bucket, region string, creds credentials.IAMRoleCredentials) (s3.S3ManagerClient, error) { +func (m *MockS3ClientCreator) NewS3ManagerClient(arg0, arg1 string, arg2 credentials.IAMRoleCredentials) (s3.S3ManagerClient, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "NewS3ManagerClient", bucket, region, creds) + ret := m.ctrl.Call(m, "NewS3ManagerClient", arg0, arg1, arg2) ret0, _ := ret[0].(s3.S3ManagerClient) ret1, _ := ret[1].(error) return ret0, ret1 } // NewS3ManagerClient indicates an expected call of NewS3ManagerClient. -func (mr *MockS3ClientCreatorMockRecorder) NewS3ManagerClient(bucket, region, creds interface{}) *gomock.Call { +func (mr *MockS3ClientCreatorMockRecorder) NewS3ManagerClient(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3ManagerClient", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3ManagerClient), bucket, region, creds) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewS3ManagerClient", reflect.TypeOf((*MockS3ClientCreator)(nil).NewS3ManagerClient), arg0, arg1, arg2) } From 6199d6c66c6d0cbaac453771a156f5e33ab4616d Mon Sep 17 00:00:00 2001 From: EC2 Default User Date: Thu, 21 Sep 2023 22:06:42 +0000 Subject: [PATCH 4/4] Add error handling on creating a new S3 client --- agent/taskresource/credentialspec/credentialspec_linux.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/agent/taskresource/credentialspec/credentialspec_linux.go b/agent/taskresource/credentialspec/credentialspec_linux.go index c2ea4fdeb81..f2c8a305391 100644 --- a/agent/taskresource/credentialspec/credentialspec_linux.go +++ b/agent/taskresource/credentialspec/credentialspec_linux.go @@ -436,6 +436,11 @@ func (cs *CredentialSpecResource) handleS3CredentialspecFile(originalCredentialS } s3Client, err := cs.s3ClientCreator.NewS3Client(bucket, cs.region, iamCredentials) + if err != nil { + cs.setTerminalReason(err.Error()) + errorEvents <- err + return + } credSpecJsonStringUnformatted, err := s3.GetObject(bucket, key, s3Client)