adding private registry auth as task resource #1427

adnxn · 2018-07-02T17:06:10Z

Summary

CI tests passing here: https://github.com/adnxn/amazon-ecs-agent/commits/pra-dev

Implementation details

E2E Manual Testing

execution role does not have appropriate permissions → task is stopped, task reason error msg is set
asm resource does not exist → task is stopped, task reason error msg is set
asm resource has malformed key names → task is stopped, task reason error msg is set
asm resource has empty values for username/password → task is stopped, task reason error msg is set
the auth data is incorrect, wrong username, password → agent will attempt to start container, but auth will fail, task will be stopped
ensure the cache is working. a task with multiple containers using the same asm resource to authenticate will only make a single service call and cache the auth data

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

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

This change adds a terminalReason message to the task struct. The intended use is for the field in the task to be set when the agent explicitly transitions a task to stopped

WIP COMMENT: 1. pra resource still needs to implement the marshalling logic + add some concept of durable/not durable resource type for resources that have to be recreated on agent restart. 2. the asm package, needs some refactoring. needs to add some kind of create client factor (for testing) since we're building regional clients for these calls 3. the STSC "reason" field changes have to be consumed by this feature since we're using that to communicate async pra failiures

Added a new factory interface in asm package, which creates ASM clients based on the region in ASMAuthConfig. Wired this into the ResourceFields struct in the taskresource package as well.

Move ContainerStatus and TaskStatus into container/status and task/status respectively. This is required for TaskStatus to be used by the TaskResource struct, to avoid circular dependecies.

Implemented the Initialize() method for ASM Auth resource, which helps with retrieving registry creds on agent restart. Also, added a factory for creating new ASM SDK clients.

This commit add tests for ASMAuthStatus and also engine test for failiure cases in private registry over ASM code path. This also adds the agent capability for private-registry-authentication.secretsmanager Also includes lint edits.

This commit branches the docker clients auth data handling logic, ecr and asm auth are handled seperatly. Also includes changes to propagate resource creation failiure reason up to task terminal reason field.

This commit adds a code to check the asm resource cache before making a call to asm. Also adds changes to populate STSC reason field when a stopped containers force a task stop.

richardpen · 2018-07-02T22:11:27Z

agent/api/task/task.go

+	resourceFields *taskresource.ResourceFields) {
+	asmAuthResource := asmauth.NewASMAuthResource(task.Arn, task.getAllASMAuthDataRequirements(),
+		task.ExecutionCredentialsID, credentialsManager, resourceFields.ASMClientCreator)
+	task.AddResource(asmauth.ResourceName, asmAuthResource)


TestMarshalUnmarshalJSON of asmauth doesn't test the marshal/unmarshal from state file, as resources are stored as an interface in the task, you will need to make sure correct struct was used during unmarshal, like this and this.

TestMarshalUnmarshalJSON of asmauth doesn't test the marshal/unmarshal from state file

okay that makes sense. adding test.

richardpen · 2018-07-02T22:46:53Z

agent/api/container/container.go

+
+// ShouldPullWithASMAuth returns true if this container needs to retrieve
+// private registry authentication data from ASM
+func (c *Container) ShouldPullWithASMAuth() bool {


Is there a unit test for this?

richardpen · 2018-07-02T22:48:13Z

agent/api/task/task.go

@@ -202,6 +212,11 @@ func (task *Task) PostUnmarshalTask(cfg *config.Config,
 			return apierrors.NewResourceInitError(task.Arn, err)
 		}
 	}
+
+	if task.requiresASMDockerAuthData() {


Is this covered by a unit test?

added TestPostUnmarshalTaskASMDockerAuth in task_test.go

richardpen · 2018-07-02T22:49:19Z

agent/api/task/task.go

@@ -1214,6 +1263,50 @@ func (task *Task) AddResource(resourceType string, resource taskresource.TaskRes
 	task.ResourcesMapUnsafe[resourceType] = append(task.ResourcesMapUnsafe[resourceType], resource)
 }

+// SetTerminalReason sets the terminalReason string and this can only be set
+// once per the task's lifecycle. This field does not accept updates.
+func (task *Task) SetTerminalReason(reason string) {


This needs a unit test.

richardpen · 2018-07-02T22:52:33Z

agent/api/task/task.go

+}
+
+// PopulateASMAuthData sets docker auth credentials for a container
+func (task *Task) PopulateASMAuthData(container *apicontainer.Container) error {


Is this covered by an existing test?

it's captured by image pull tests, but added unit tests specifically for this.

richardpen · 2018-07-02T22:59:16Z

agent/asm/asm.go

+
+// GetDockerAuthFromASM makes the api call to the AWS Secrets Manager service to
+// retrieve the docker auth data
+func GetDockerAuthFromASM(secretID string, client secretsmanageriface.SecretsManagerAPI) (docker.AuthConfiguration, error) {


This needs a unit test for both happy path and failure case.

added asm_test.go, somehow was missed during merge 🤔

richardpen · 2018-07-03T20:46:37Z

agent/taskresource/asmauth/asmauthstatus_test.go

+}
+
+func TestMarshalASMAuthStatus(t *testing.T) {
+	status := ASMAuthStatusNone


Can you also test "ASMAuthStatusCreated"? And this can be combined together with below using table test.

richardpen · 2018-07-03T20:48:17Z

agent/taskresource/asmauth/asmauthstatus_test.go

+	SomeStatus ASMAuthStatus `json:"status"`
+}
+
+func TestUnmarshalASMAuthStatus(t *testing.T) {


Can these tests combined together with table tests?

richardpen · 2018-07-03T20:52:52Z

agent/taskresource/asmauth/asmauth.go

+	})
+}
+
+func (auth *ASMAuthResource) UnmarshalJSON(b []byte) error {


nit: comments

richardpen · 2018-07-03T20:53:08Z

agent/taskresource/asmauth/asmauth.go

+	ExecutionCredentialsID string                      `json:"executionCredentialsID"`
+}
+
+func (auth *ASMAuthResource) MarshalJSON() ([]byte, error) {


Lint: please add comments.

adnxn · 2018-07-06T02:16:01Z

@richardpen added tests you called out, this was very helpful thanks 😁

This commit adds unit tests to code paths that are exercised using the private registry auth over asm feature.

richardpen

Code mostly LGTM, did you have the end to end test pass?

richardpen · 2018-07-06T17:13:56Z

agent/dockerclient/dockerapi/docker_client.go

-		return authConfig, CannotPullECRContainerError{err}
+
+	switch authData.Type {
+	case "ecr":


You can use the constant defined above, same below.

making those constants exportable then.

richardpen · 2018-07-06T17:19:27Z

agent/engine/docker_task_engine.go

+			seelog.Errorf("Task engine [%s]: unable to acquire Docker registry credentials for container [%s]",
+				task.Arn, container.Name)
+			return dockerapi.DockerContainerMetadata{
+				Error: dockerapi.CannotPullECRContainerError{


Using CannotPullECRContainerError seems confusion here, as this is not necessary pulling from ECR, can you create a new error type?

yea you're right, i'm going to add some distinct error types here.

added a CannotPullContainerAuthError type

petderek · 2018-07-06T17:09:36Z

agent/api/container/container.go

+	c.lock.RLock()
+	defer c.lock.RUnlock()
+
+	return c.RegistryAuthentication != nil &&


I understand this, but would prefer it to be expressed more explicitly.

would prefer it to be expressed more explicitly.

how do you mean exactly? i dont follow.

Try to make it so that it isn't a multi-line boolean expression

petderek · 2018-07-06T17:15:29Z

agent/asm/asm.go

+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package asm


what does asm mean

aws secrets manager https://aws.amazon.com/secrets-manager/

petderek · 2018-07-06T17:15:54Z

agent/asm/asm.go

+	Password *string
+}
+
+// GetDockerAuthFromASM makes the api call to the AWS Secrets Manager service to


oh i get it now. but add a package doc note maybe?

petderek · 2018-07-06T17:21:04Z

agent/asm/asm.go

+	err := json.Unmarshal([]byte(secretValue), &authDataValue)
+	if err != nil {
+		// could  not unmarshal, incorrect secret value schema
+		return docker.AuthConfiguration{}, errors.Wrapf(err,


is it always safe to print this err? Does the json unmarshal error sometimes return parts of the original string? I'd lean toward not printing it.

I'd lean toward not printing it.

why exactly? this is an error that would be passed back up to CP as reason for why the the auth data wasn't acquired.

It might print some of the secret text, which we don't want.

It might print some of the secret text, which we don't want.

hm i can't seem to find anything that confirms that errors will not contain partials. probably best to leave it out then?

petderek · 2018-07-06T17:23:55Z

agent/asm/asm_test.go

+			ShouldError: false,
+		},
+		{
+			Name: "MissingUsername",


is username always required for all repos?

if it needs authentication, then it needs username and password.

petderek · 2018-07-06T17:25:07Z

agent/asm/asm_test.go

+			ShouldError: true,
+		},
+		{
+			Name: "MalformedJson",


what about empty json like {} or [] ? Or is that covered by the other empty cases?

should be captured under "...malformed empty field", but added an explicit case w/ {}

petderek · 2018-07-06T17:28:07Z

agent/taskresource/asmauth/asmauth_test.go

@@ -0,0 +1,143 @@
+// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


//+build unit

Did you miss this, I didn't see this was added?

yea missed it, added now.

petderek · 2018-07-06T17:35:39Z

agent/taskresource/asmauth/asmauth_test.go

+	}
+	gomock.InOrder(
+		credentialsManager.EXPECT().GetTaskCredentials(executionCredentialsID).Return(creds, true),
+		asmClientCreator.EXPECT().NewASMClient(region, iamRoleCreds).Return(mockASMClient),


I don't have a solution for you here, but we need to be careful with how many layers we go especially with new code. Remember:

Every time a mock returns a mock a fairy dies *

yea ended up adding this to be able to test more complete workflows like post unmarshall or the image pull workflows.

petderek · 2018-07-06T17:48:33Z

agent/taskresource/asmauth/asmauth_test.go

+	asmResOut := &ASMAuthResource{}
+	err = json.Unmarshal(bytes, asmResOut)
+	require.NoError(t, err)
+	assert.Equal(t, asmResIn.taskARN, asmResOut.taskARN)


assert does deep equal so assert.Equal(t,asmResIn,asmResOut) should work for you (I think?)

eh why not check field by field?

Its fine as is -- its just more concise the other way.

/// this... assert.Equal(t, expectedStruct, actualStruct) /// instead of ... assert.Equal(t, expectedStruct.field1, actualStruct.field1) assert.Equal(t, expectedStruct.field2, actualStruct.field2) assert.Equal(t, expectedStruct.field3, actualStruct.field3) // ...

petderek · 2018-07-06T17:49:34Z

agent/taskresource/asmauth/asmauth_test.go

+	assert.Equal(t, asmResIn.requiredASMResources[0], asmResOut.requiredASMResources[0])
+}
+
+func TestInitialize(t *testing.T) {


Do we test what happens when we initialize twice? Might be more appropriate for integ test

IE: what happens when we initialize, agent gets shut down, and then agent starts back up?

what happens when we initialize, agent gets shut down, and then agent starts back up

so initialize is part of resource provisioning, and if the agent is restarted the resources will be reinitialized as part of the task engine's synchronizeState() workflow.

sharanyad

questions and nits :)

sharanyad · 2018-07-06T00:02:20Z

agent/api/task/task.go

+	asmAuthResource := asmauth.NewASMAuthResource(task.Arn, task.getAllASMAuthDataRequirements(),
+		task.ExecutionCredentialsID, credentialsManager, resourceFields.ASMClientCreator)
+	task.AddResource(asmauth.ResourceName, asmAuthResource)
+	for _, container := range task.Containers {


Is it necessary to add asm resource dependency for even containers that don't need to be pulled with ASM auth?

it's scoped as a task resource and gated behind if task.requiresASMDockerAuthData() ...

Agree this is a task resource, but other containers(that don't need asm auth) need not wait for this resource's steady state to transition to PULLED. Isn't it?

oh i see what you mean, this bit should be gated behind container.ShouldPullWithASMAuth() { ...

good catch, fixed.

sharanyad · 2018-07-06T00:20:37Z

agent/app/agent_unix.go

-		IOUtil:  ioutilwrapper.NewIOUtil(),
+		ResourceFieldsCommon: &taskresource.ResourceFieldsCommon{
+			IOUtil:             ioutilwrapper.NewIOUtil(),
+			ASMClientCreator:   factory.NewClientCreator(),


Why is ASMClientCreator inside ResourceFieldsCommon? Do other resources use this?

mainly to be able to test the codepaths that need these clients. it's the regionalised asm clients requirement thats driving this, the resource will carry the creator with it, but the creation of the clients is still gated behind checks that ensure the task actually needs asm clients.

ResourceFields seemed oddly OS specific when I created this. ResourceFieldsCommon here is referring to common fields across OS's not to any "common" resources. Factories, managers etc shouldn't be OS specific imo and in the long term, there shouldn't be a need for the ResourceFieldsCommon struct. Everything should just be in one struct. But, this is a workaround for now.

sharanyad · 2018-07-06T00:27:58Z

agent/asm/genereate_mocks.go

@@ -0,0 +1,16 @@
+// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


nit: generate_mocks

yikes, fixed.

sharanyad · 2018-07-06T00:42:03Z

agent/asm/asm.go

+// GetDockerAuthFromASM makes the api call to the AWS Secrets Manager service to
+// retrieve the docker auth data
+func GetDockerAuthFromASM(secretID string, client secretsmanageriface.SecretsManagerAPI) (docker.AuthConfiguration, error) {
+	in := &secretsmanager.GetSecretValueInput{


Are timeouts handled within the API call?

yea the sdk does

sharanyad · 2018-07-06T16:41:33Z

agent/taskresource/asmauth/asmauth.go

+	executionCredentials, ok := auth.credentialsManager.GetTaskCredentials(auth.GetExecutionCredentialsID())
+	if !ok {
+		// No need to log here. managedTask.applyResourceState already does that
+		return errors.Errorf("asm resource: unable find execution role credentials")


nits: This should be errors.New and 'unable to find'.

adnxn · 2018-07-06T21:00:46Z

Code mostly LGTM, did you have the end to end test pass?

@richardpen: yea end to end tests are passing

aaithal · 2018-07-09T16:17:42Z

agent/taskresource/asmauth/asmauthstatus.go

+
+// MarshalJSON overrides the logic for JSON-encoding the ResourceStatus type
+func (as *ASMAuthStatus) MarshalJSON() ([]byte, error) {
+	if as == nil {


in taskresource/asmauth, this is treated as an error. Here, it's not. Any reason for the differing behavior here?

ah that's a miss, will also treat as error.

aaithal

Code lgtm. Won't give it a shitpit as i wrote some of this code and it'd be incredibly narcissistic of me 😄 to do so!

adnxn · 2018-07-09T16:48:09Z

Code lgtm. Won't give it a shitpit as i wrote some of this code and it'd be incredibly narcissistic of me 😄 to do so!

lol fairrrrr enough

Previously we were gating creation of factories and managers required by task resources behind the task cpu/memory limits flag. Now we gate the usage behind feature specific flags that require these dependencies.

adnxn and others added 8 commits June 7, 2018 09:12

taskresource: add terminalReason msg string

719a1a6

This change adds a terminalReason message to the task struct. The intended use is for the field in the task to be set when the agent explicitly transitions a task to stopped

Merge remote-tracking branch 'origin/dev' into pra-resource-wip-mirror

23d4590

asm: add client creator interface

82a6f1d

Added a new factory interface in asm package, which creates ASM clients based on the region in ASMAuthConfig. Wired this into the ResourceFields struct in the taskresource package as well.

container/task status: refactor into sub-packages

e797b90

Move ContainerStatus and TaskStatus into container/status and task/status respectively. This is required for TaskStatus to be used by the TaskResource struct, to avoid circular dependecies.

asmauth resource: recovery on agent restart

79cf221

Implemented the Initialize() method for ASM Auth resource, which helps with retrieving registry creds on agent restart. Also, added a factory for creating new ASM SDK clients.

agent: added asm auth tests

c5514eb

This commit add tests for ASMAuthStatus and also engine test for failiure cases in private registry over ASM code path. This also adds the agent capability for private-registry-authentication.secretsmanager Also includes lint edits.

asmauth resource: propagate resource terminal reason to task

ebe4eda

This commit branches the docker clients auth data handling logic, ecr and asm auth are handled seperatly. Also includes changes to propagate resource creation failiure reason up to task terminal reason field.

This was referenced Jul 2, 2018

[WIP] Private registry auth resource #1420

Closed

taskresource: add terminalReason msg string #1407

Closed

adnxn requested review from yhlee-aws, richardpen, a team and aaithal July 2, 2018 17:09

asmauth resource: check cache before service call

f601106

This commit adds a code to check the asm resource cache before making a call to asm. Also adds changes to populate STSC reason field when a stopped containers force a task stop.

adnxn requested a review from petderek July 3, 2018 18:20

richardpen reviewed Jul 3, 2018

View reviewed changes

adnxn force-pushed the pra-dev branch from b01561b to db1e8da Compare July 6, 2018 02:22

asmauth resource: added unit tests

3894385

This commit adds unit tests to code paths that are exercised using the private registry auth over asm feature.

adnxn force-pushed the pra-dev branch from db1e8da to 3894385 Compare July 6, 2018 02:27

richardpen reviewed Jul 6, 2018

View reviewed changes

petderek reviewed Jul 6, 2018

View reviewed changes

sharanyad reviewed Jul 6, 2018

View reviewed changes

adnxn force-pushed the pra-dev branch from 6c34c28 to fea1467 Compare July 8, 2018 18:17

aaithal reviewed Jul 9, 2018

View reviewed changes

adnxn force-pushed the pra-dev branch from fea1467 to 09a4184 Compare July 9, 2018 17:25

petderek approved these changes Jul 9, 2018

View reviewed changes

asmauth resource: added distinct auth error type

b31733a

adnxn force-pushed the pra-dev branch from 09a4184 to b31733a Compare July 9, 2018 18:13

richardpen approved these changes Jul 9, 2018

View reviewed changes

app: init resourceFields by default

4388e84

Previously we were gating creation of factories and managers required by task resources behind the task cpu/memory limits flag. Now we gate the usage behind feature specific flags that require these dependencies.

richardpen approved these changes Jul 10, 2018

View reviewed changes

adnxn merged commit 4388e84 into aws:dev Jul 10, 2018

adnxn added a commit that referenced this pull request Jul 10, 2018

Merge pull request #1427 from adnxn/pra-dev

e182979

adnxn deleted the pra-dev branch October 19, 2018 17:36

		@@ -0,0 +1,143 @@
		// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -0,0 +1,16 @@
		// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.

adding private registry auth as task resource #1427

adding private registry auth as task resource #1427

Conversation

adnxn commented Jul 2, 2018 • edited Loading

Summary

Implementation details

E2E Manual Testing

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

adnxn Jul 6, 2018 • 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

adnxn commented Jul 6, 2018

richardpen 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

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

adnxn Jul 9, 2018 • 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

sharanyad 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

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

adnxn commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal left a comment • edited Loading

Choose a reason for hiding this comment

adnxn commented Jul 2, 2018 •

edited

Loading

adnxn Jul 6, 2018 •

edited

Loading

adnxn Jul 9, 2018 •

edited

Loading

aaithal left a comment •

edited

Loading