Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding private registry auth as task resource #1427

Merged
merged 12 commits into from
Jul 10, 2018
4 changes: 3 additions & 1 deletion agent/Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 7 additions & 6 deletions agent/acs/handler/payload_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/api"
apieni "github.com/aws/amazon-ecs-agent/agent/api/eni"
apitask "github.com/aws/amazon-ecs-agent/agent/api/task"
apitaskstatus "github.com/aws/amazon-ecs-agent/agent/api/task/status"
"github.com/aws/amazon-ecs-agent/agent/credentials"
"github.com/aws/amazon-ecs-agent/agent/engine"
"github.com/aws/amazon-ecs-agent/agent/eventhandler"
Expand Down Expand Up @@ -308,16 +309,16 @@ func (payloadHandler *payloadRequestHandler) ackCredentials(messageID *string, c

// skipAddTaskComparatorFunc defines the function pointer that accepts task status
// and returns the boolean comparison result
type skipAddTaskComparatorFunc func(apitask.TaskStatus) bool
type skipAddTaskComparatorFunc func(apitaskstatus.TaskStatus) bool

// isTaskStatusStopped returns true if the task status == STOPPTED
func isTaskStatusStopped(status apitask.TaskStatus) bool {
return status == apitask.TaskStopped
func isTaskStatusStopped(status apitaskstatus.TaskStatus) bool {
return status == apitaskstatus.TaskStopped
}

// isTaskStatusNotStopped returns true if the task status != STOPPTED
func isTaskStatusNotStopped(status apitask.TaskStatus) bool {
return status != apitask.TaskStopped
func isTaskStatusNotStopped(status apitaskstatus.TaskStatus) bool {
return status != apitaskstatus.TaskStopped
}

// handleUnrecognizedTask handles unrecognized tasks by sending 'stopped' with
Expand All @@ -334,7 +335,7 @@ func (payloadHandler *payloadRequestHandler) handleUnrecognizedTask(task *ecsacs
// Only need to stop the task; it brings down the containers too.
taskEvent := api.TaskStateChange{
TaskARN: *task.Arn,
Status: apitask.TaskStopped,
Status: apitaskstatus.TaskStopped,
Reason: UnrecognizedTaskError{err}.Error(),
}

Expand Down
5 changes: 3 additions & 2 deletions agent/acs/handler/payload_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/api"
"github.com/aws/amazon-ecs-agent/agent/api/mocks"
apitask "github.com/aws/amazon-ecs-agent/agent/api/task"
apitaskstatus "github.com/aws/amazon-ecs-agent/agent/api/task/status"
"github.com/aws/amazon-ecs-agent/agent/credentials"
"github.com/aws/amazon-ecs-agent/agent/engine/mocks"
"github.com/aws/amazon-ecs-agent/agent/eventhandler"
Expand Down Expand Up @@ -324,11 +325,11 @@ func TestAddPayloadTaskAddsNonStoppedTasksAfterStoppedTasks(t *testing.T) {
// Verify if stopped task is added before running task
firstTaskAdded := tasksAddedToEngine[0]
assert.Equal(t, firstTaskAdded.Arn, stoppedTaskArn)
assert.Equal(t, firstTaskAdded.GetDesiredStatus(), apitask.TaskStopped)
assert.Equal(t, firstTaskAdded.GetDesiredStatus(), apitaskstatus.TaskStopped)

secondTaskAdded := tasksAddedToEngine[1]
assert.Equal(t, secondTaskAdded.Arn, runningTaskArn)
assert.Equal(t, secondTaskAdded.GetDesiredStatus(), apitask.TaskRunning)
assert.Equal(t, secondTaskAdded.GetDesiredStatus(), apitaskstatus.TaskRunning)
}

// TestPayloadBufferHandler tests if the async payloadBufferHandler routine
Expand Down
85 changes: 56 additions & 29 deletions agent/api/container/container.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
// Copyright 2014-2018 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
Expand All @@ -19,17 +19,20 @@ import (
"sync"
"time"

apicontainerstatus "github.com/aws/amazon-ecs-agent/agent/api/container/status"
apierrors "github.com/aws/amazon-ecs-agent/agent/api/errors"
"github.com/aws/amazon-ecs-agent/agent/credentials"
"github.com/aws/amazon-ecs-agent/agent/taskresource"
"github.com/aws/aws-sdk-go/aws"

docker "github.com/fsouza/go-dockerclient"
)

const (
// defaultContainerSteadyStateStatus defines the container status at
// which the container is assumed to be in steady state. It is set
// to 'ContainerRunning' unless overridden
defaultContainerSteadyStateStatus = ContainerRunning
defaultContainerSteadyStateStatus = apicontainerstatus.ContainerRunning

// awslogsAuthExecutionRole is the string value passed in the task payload
// that specifies that the log driver should be authenticated using the
Expand All @@ -38,6 +41,12 @@ const (

// DockerHealthCheckType is the type of container health check provided by docker
DockerHealthCheckType = "docker"

// authTypeECR is to use image pull auth over ECR
authTypeECR = "ecr"

// authTypeASM is to use image pull auth over AWS Secrets Manager
authTypeASM = "asm"
)

// DockerConfig represents additional metadata about a container to run. It's
Expand All @@ -55,7 +64,7 @@ type DockerConfig struct {
// HealthStatus contains the health check result returned by docker
type HealthStatus struct {
// Status is the container health status
Status ContainerHealthStatus `json:"status,omitempty"`
Status apicontainerstatus.ContainerHealthStatus `json:"status,omitempty"`
// Since is the timestamp when container health status changed
Since *time.Time `json:"statusSince,omitempty"`
// ExitCode is the exitcode of health check if failed
Expand Down Expand Up @@ -120,15 +129,15 @@ type Container struct {
// TODO DesiredStatusUnsafe should probably be private with appropriately written
// setter/getter. When this is done, we need to ensure that the UnmarshalJSON
// is handled properly so that the state storage continues to work.
DesiredStatusUnsafe ContainerStatus `json:"desiredStatus"`
DesiredStatusUnsafe apicontainerstatus.ContainerStatus `json:"desiredStatus"`

// KnownStatusUnsafe represents the state where the container is.
// NOTE: Do not access `KnownStatusUnsafe` directly. Instead, use `GetKnownStatus`
// and `SetKnownStatus`.
// TODO KnownStatusUnsafe should probably be private with appropriately written
// setter/getter. When this is done, we need to ensure that the UnmarshalJSON
// is handled properly so that the state storage continues to work.
KnownStatusUnsafe ContainerStatus `json:"KnownStatus"`
KnownStatusUnsafe apicontainerstatus.ContainerStatus `json:"KnownStatus"`

// TransitionDependenciesMap is a map of the dependent container status to other
// dependencies that must be satisfied in order for this container to transition.
Expand All @@ -154,7 +163,7 @@ type Container struct {
// Create, Start, or Stop) but we don't yet know that the application was successful.
// No need to save it in the state file, as agent will synchronize the container status
// on restart and for some operation eg: pull, it has to be recalled again.
AppliedStatus ContainerStatus `json:"-"`
AppliedStatus apicontainerstatus.ContainerStatus `json:"-"`
// ApplyingError is an error that occurred trying to transition the container
// to its desired state. It is propagated to the backend in the form
// 'Name: ErrorString' as the 'reason' field.
Expand All @@ -165,7 +174,7 @@ type Container struct {
// TODO SentStatusUnsafe should probably be private with appropriately written
// setter/getter. When this is done, we need to ensure that the UnmarshalJSON is
// handled properly so that the state storage continues to work.
SentStatusUnsafe ContainerStatus `json:"SentStatus"`
SentStatusUnsafe apicontainerstatus.ContainerStatus `json:"SentStatus"`

// MetadataFileUpdated is set to true when we have completed updating the
// metadata file
Expand All @@ -186,7 +195,7 @@ type Container struct {
// it's not only supposed to be set when the container is being created, it's
// exposed outside of the package so that it's marshalled/unmarshalled in the
// the JSON body while saving the state
SteadyStateStatusUnsafe *ContainerStatus `json:"SteadyStateStatus,omitempty"`
SteadyStateStatusUnsafe *apicontainerstatus.ContainerStatus `json:"SteadyStateStatus,omitempty"`

createdAt time.Time
startedAt time.Time
Expand Down Expand Up @@ -231,7 +240,7 @@ func (dc *DockerContainer) String() string {
// NewContainerWithSteadyState creates a new Container object with the specified
// steady state. Containers that need the non default steady state set will
// use this method instead of setting it directly
func NewContainerWithSteadyState(steadyState ContainerStatus) *Container {
func NewContainerWithSteadyState(steadyState apicontainerstatus.ContainerStatus) *Container {
steadyStateStatus := steadyState
return &Container{
SteadyStateStatusUnsafe: &steadyStateStatus,
Expand All @@ -249,7 +258,7 @@ func (c *Container) DesiredTerminal() bool {
}

// GetKnownStatus returns the known status of the container
func (c *Container) GetKnownStatus() ContainerStatus {
func (c *Container) GetKnownStatus() apicontainerstatus.ContainerStatus {
c.lock.RLock()
defer c.lock.RUnlock()

Expand All @@ -258,7 +267,7 @@ func (c *Container) GetKnownStatus() ContainerStatus {

// SetKnownStatus sets the known status of the container and update the container
// applied status
func (c *Container) SetKnownStatus(status ContainerStatus) {
func (c *Container) SetKnownStatus(status apicontainerstatus.ContainerStatus) {
c.lock.Lock()
defer c.lock.Unlock()

Expand All @@ -267,31 +276,31 @@ func (c *Container) SetKnownStatus(status ContainerStatus) {
}

// GetDesiredStatus gets the desired status of the container
func (c *Container) GetDesiredStatus() ContainerStatus {
func (c *Container) GetDesiredStatus() apicontainerstatus.ContainerStatus {
c.lock.RLock()
defer c.lock.RUnlock()

return c.DesiredStatusUnsafe
}

// SetDesiredStatus sets the desired status of the container
func (c *Container) SetDesiredStatus(status ContainerStatus) {
func (c *Container) SetDesiredStatus(status apicontainerstatus.ContainerStatus) {
c.lock.Lock()
defer c.lock.Unlock()

c.DesiredStatusUnsafe = status
}

// GetSentStatus safely returns the SentStatusUnsafe of the container
func (c *Container) GetSentStatus() ContainerStatus {
func (c *Container) GetSentStatus() apicontainerstatus.ContainerStatus {
c.lock.RLock()
defer c.lock.RUnlock()

return c.SentStatusUnsafe
}

// SetSentStatus safely sets the SentStatusUnsafe of the container
func (c *Container) SetSentStatus(status ContainerStatus) {
func (c *Container) SetSentStatus(status apicontainerstatus.ContainerStatus) {
c.lock.Lock()
defer c.lock.Unlock()

Expand Down Expand Up @@ -328,7 +337,7 @@ func (c *Container) ShouldPullWithExecutionRole() bool {
defer c.lock.RUnlock()

return c.RegistryAuthentication != nil &&
c.RegistryAuthentication.Type == "ecr" &&
c.RegistryAuthentication.Type == authTypeECR &&
c.RegistryAuthentication.ECRAuthData != nil &&
c.RegistryAuthentication.ECRAuthData.UseExecutionRole
}
Expand All @@ -350,7 +359,7 @@ func (c *Container) String() string {
// 'pause' container can reach its teady state once networking resources
// have been provisioned for it, which is done in the `ContainerResourcesProvisioned`
// state
func (c *Container) GetSteadyStateStatus() ContainerStatus {
func (c *Container) GetSteadyStateStatus() apicontainerstatus.ContainerStatus {
if c.SteadyStateStatusUnsafe == nil {
return defaultContainerSteadyStateStatus
}
Expand Down Expand Up @@ -381,9 +390,9 @@ func (c *Container) IsKnownSteadyState() bool {
// c. if the steady state of the container is defined as `ContainerCreated`,
// the progression is:
// Container: None -> Pulled -> Created* -> Stopped -> Zombie
func (c *Container) GetNextKnownStateProgression() ContainerStatus {
func (c *Container) GetNextKnownStateProgression() apicontainerstatus.ContainerStatus {
if c.IsKnownSteadyState() {
return ContainerStopped
return apicontainerstatus.ContainerStopped
}

return c.GetKnownStatus() + 1
Expand Down Expand Up @@ -546,7 +555,7 @@ func (c *Container) SetHealthStatus(health HealthStatus) {
c.Health.Output = health.Output

// Set the health exit code if the health check failed
if c.Health.Status == ContainerUnhealthy {
if c.Health.Status == apicontainerstatus.ContainerUnhealthy {
c.Health.ExitCode = health.ExitCode
}
}
Expand All @@ -569,8 +578,8 @@ func (c *Container) GetHealthStatus() HealthStatus {
// BuildContainerDependency adds a new dependency container and satisfied status
// to the dependent container
func (c *Container) BuildContainerDependency(contName string,
satisfiedStatus ContainerStatus,
dependentStatus ContainerStatus) {
satisfiedStatus apicontainerstatus.ContainerStatus,
dependentStatus apicontainerstatus.ContainerStatus) {
contDep := ContainerDependency{
ContainerName: contName,
SatisfiedStatus: satisfiedStatus,
Expand All @@ -590,7 +599,7 @@ func (c *Container) BuildContainerDependency(contName string,
// CREATED status, then RequiredStatus=VolumeCreated and dependentStatus=ContainerPulled
func (c *Container) BuildResourceDependency(resourceName string,
requiredStatus taskresource.ResourceStatus,
dependentStatus ContainerStatus) {
dependentStatus apicontainerstatus.ContainerStatus) {
resourceDep := ResourceDependency{
Name: resourceName,
RequiredStatus: requiredStatus,
Expand All @@ -604,24 +613,24 @@ func (c *Container) BuildResourceDependency(resourceName string,
}

// updateAppliedStatusUnsafe updates the container transitioning status
func (c *Container) updateAppliedStatusUnsafe(knownStatus ContainerStatus) {
if c.AppliedStatus == ContainerStatusNone {
func (c *Container) updateAppliedStatusUnsafe(knownStatus apicontainerstatus.ContainerStatus) {
if c.AppliedStatus == apicontainerstatus.ContainerStatusNone {
return
}

// Check if the container transition has already finished
if c.AppliedStatus <= knownStatus {
c.AppliedStatus = ContainerStatusNone
c.AppliedStatus = apicontainerstatus.ContainerStatusNone
}
}

// SetAppliedStatus sets the applied status of container and returns whether
// the container is already in a transition
func (c *Container) SetAppliedStatus(status ContainerStatus) bool {
func (c *Container) SetAppliedStatus(status apicontainerstatus.ContainerStatus) bool {
c.lock.Lock()
defer c.lock.Unlock()

if c.AppliedStatus != ContainerStatusNone {
if c.AppliedStatus != apicontainerstatus.ContainerStatusNone {
// return false to indicate the set operation failed
return false
}
Expand All @@ -631,9 +640,27 @@ func (c *Container) SetAppliedStatus(status ContainerStatus) bool {
}

// GetAppliedStatus returns the transitioning status of container
func (c *Container) GetAppliedStatus() ContainerStatus {
func (c *Container) GetAppliedStatus() apicontainerstatus.ContainerStatus {
c.lock.RLock()
defer c.lock.RUnlock()

return c.AppliedStatus
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a unit test for this?

c.lock.RLock()
defer c.lock.RUnlock()

return c.RegistryAuthentication != nil &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer it to be expressed more explicitly.

how do you mean exactly? i dont follow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

c.RegistryAuthentication.Type == authTypeASM &&
c.RegistryAuthentication.ASMAuthData != nil
}

// SetASMDockerAuthConfig add the docker auth config data to the
// RegistryAuthentication struct held by the container, this is then passed down
// to the docker client to pull the image
func (c *Container) SetASMDockerAuthConfig(dac docker.AuthConfiguration) {
c.RegistryAuthentication.ASMAuthData.SetDockerAuthConfig(dac)
}
Loading