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

v3 metadata networks response for non-awsvpc tasks #1833

Merged
merged 2 commits into from
Feb 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions agent/api/container/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,12 @@ type Container struct {
// VolumesUnsafe is an array of volume mounts in the container.
VolumesUnsafe []types.MountPoint `json:"-"`

// NetworkModeUnsafe is the network mode in which the container is started
NetworkModeUnsafe string `json:"-"`

// NetworksUnsafe denotes the Docker Network Settings in the container.
NetworkSettingsUnsafe *types.NetworkSettings `json:"-"`

// SteadyStateStatusUnsafe specifies the steady state status for the container
// If uninitialized, it's assumed to be set to 'ContainerRunning'. Even though
// it's not only supposed to be set when the container is being created, it's
Expand Down Expand Up @@ -593,6 +599,38 @@ func (c *Container) GetVolumes() []types.MountPoint {
return c.VolumesUnsafe
}

// SetNetworkSettings sets the networks field in a container
func (c *Container) SetNetworkSettings(networks *types.NetworkSettings) {
c.lock.Lock()
defer c.lock.Unlock()

c.NetworkSettingsUnsafe = networks
}

// GetNetworkSettings returns the networks field in a container
func (c *Container) GetNetworkSettings() *types.NetworkSettings {
c.lock.RLock()
defer c.lock.RUnlock()

return c.NetworkSettingsUnsafe
}

// SetNetworkMode sets the network mode of the container
func (c *Container) SetNetworkMode(networkMode string) {
c.lock.Lock()
defer c.lock.Unlock()

c.NetworkModeUnsafe = networkMode
}

// GetNetworkMode returns the network mode of the container
func (c *Container) GetNetworkMode() string {
c.lock.RLock()
defer c.lock.RUnlock()

return c.NetworkModeUnsafe
}

// HealthStatusShouldBeReported returns true if the health check is defined in
// the task definition
func (c *Container) HealthStatusShouldBeReported() bool {
Expand Down
9 changes: 9 additions & 0 deletions agent/dockerclient/dockerapi/docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,15 @@ func MetadataFromContainer(dockerContainer *types.ContainerJSON) DockerContainer
StartedAt: startedTime,
FinishedAt: finishedTime,
}

if dockerContainer.NetworkSettings != nil {
metadata.NetworkSettings = dockerContainer.NetworkSettings
}

if dockerContainer.HostConfig != nil {
metadata.NetworkMode = string(dockerContainer.HostConfig.NetworkMode)
}

if dockerContainer.Config != nil {
metadata.Labels = dockerContainer.Config.Labels
}
Expand Down
9 changes: 9 additions & 0 deletions agent/dockerclient/dockerapi/docker_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1425,6 +1425,9 @@ func TestMetadataFromContainer(t *testing.T) {
NetworkSettingsBase: types.NetworkSettingsBase{
Ports: ports,
},
DefaultNetworkSettings: types.DefaultNetworkSettings{
IPAddress: "17.0.0.3",
},
},
ContainerJSONBase: &types.ContainerJSONBase{
ID: "1234",
Expand All @@ -1434,6 +1437,9 @@ func TestMetadataFromContainer(t *testing.T) {
StartedAt: started,
FinishedAt: finished,
},
HostConfig: &dockercontainer.HostConfig{
NetworkMode: dockercontainer.NetworkMode("bridge"),
},
},
Config: &dockercontainer.Config{
Labels: labels,
Expand All @@ -1446,6 +1452,9 @@ func TestMetadataFromContainer(t *testing.T) {
assert.Equal(t, volumes, metadata.Volumes)
assert.Equal(t, labels, metadata.Labels)
assert.Len(t, metadata.PortBindings, 1)
assert.Equal(t, "bridge", metadata.NetworkMode)
assert.NotNil(t, metadata.NetworkSettings)
assert.Equal(t, "17.0.0.3", metadata.NetworkSettings.IPAddress)

// Need to convert both strings to same format to be able to compare. Parse and Format are not inverses.
createdTimeSDK, _ := time.Parse(time.RFC3339, dockerContainer.Created)
Expand Down
4 changes: 4 additions & 0 deletions agent/dockerclient/dockerapi/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ type DockerContainerMetadata struct {
FinishedAt time.Time
// Health contains the result of a container health check
Health apicontainer.HealthStatus
// NetworkMode denotes the network mode in which the container is started
NetworkMode string
// NetworksUnsafe denotes the Docker Network Settings in the container
NetworkSettings *types.NetworkSettings
}

// ListContainersResponse encapsulates the response from the docker client for the
Expand Down
2 changes: 2 additions & 0 deletions agent/engine/docker_task_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,8 @@ func updateContainerMetadata(metadata *dockerapi.DockerContainerMetadata, contai
if container.HealthStatusShouldBeReported() {
container.SetHealthStatus(metadata.Health)
}
container.SetNetworkMode(metadata.NetworkMode)
container.SetNetworkSettings(metadata.NetworkSettings)
}

// synchronizeContainerStatus checks and updates the container status with docker
Expand Down
152 changes: 152 additions & 0 deletions agent/handlers/task_server_setup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ const (
associationName = "dev1"
associationEncoding = "base64"
associationValue = "val"
bridgeMode = "bridge"
bridgeIPAddr = "17.0.0.3"
)

var (
Expand Down Expand Up @@ -192,10 +194,99 @@ var (
Associations: []string{associationName},
}
expectedAssociationResponse = associationValue

bridgeTask = &apitask.Task{
Arn: taskARN,
Associations: []apitask.Association{association},
Family: family,
Version: version,
DesiredStatusUnsafe: apitaskstatus.TaskRunning,
KnownStatusUnsafe: apitaskstatus.TaskRunning,
CPU: cpu,
Memory: memory,
PullStartedAtUnsafe: now,
PullStoppedAtUnsafe: now,
ExecutionStoppedAtUnsafe: now,
}
container1 = &apicontainer.Container{
Name: containerName,
Image: imageName,
ImageID: imageID,
DesiredStatusUnsafe: apicontainerstatus.ContainerRunning,
KnownStatusUnsafe: apicontainerstatus.ContainerRunning,
CPU: cpu,
Memory: memory,
Type: apicontainer.ContainerNormal,
Ports: []apicontainer.PortBinding{
{
ContainerPort: containerPort,
Protocol: apicontainer.TransportProtocolTCP,
},
},
NetworkModeUnsafe: bridgeMode,
Copy link
Member

Choose a reason for hiding this comment

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

nit: did you run go fmt on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did and doesn't report of anything different

NetworkSettingsUnsafe: &types.NetworkSettings{
DefaultNetworkSettings: types.DefaultNetworkSettings{
IPAddress: bridgeIPAddr,
},
},
}
bridgeContainer = &apicontainer.DockerContainer{
DockerID: containerID,
DockerName: containerName,
Container: container1,
}
containerNameToBridgeContainer = map[string]*apicontainer.DockerContainer{
taskARN: bridgeContainer,
}
expectedBridgeContainerResponse = v2.ContainerResponse{
ID: containerID,
Name: containerName,
DockerName: containerName,
Image: imageName,
ImageID: imageID,
DesiredStatus: statusRunning,
KnownStatus: statusRunning,
Limits: v2.LimitsResponse{
CPU: aws.Float64(cpu),
Memory: aws.Int64(memory),
},
Type: containerType,
Labels: labels,
Ports: []v1.PortResponse{
{
ContainerPort: containerPort,
Protocol: containerPortProtocol,
},
},
Networks: []containermetadata.Network{
{
NetworkMode: bridgeMode,
IPv4Addresses: []string{bridgeIPAddr},
},
},
}
expectedBridgeTaskResponse = v2.TaskResponse{
Cluster: clusterName,
TaskARN: taskARN,
Family: family,
Revision: version,
DesiredStatus: statusRunning,
KnownStatus: statusRunning,
Containers: []v2.ContainerResponse{expectedBridgeContainerResponse},
Limits: &v2.LimitsResponse{
CPU: aws.Float64(cpu),
Memory: aws.Int64(memory),
},
PullStartedAt: aws.Time(now.UTC()),
PullStoppedAt: aws.Time(now.UTC()),
ExecutionStoppedAt: aws.Time(now.UTC()),
AvailabilityZone: availabilityzone,
}
)

func init() {
container.SetLabels(labels)
container1.SetLabels(labels)
}

// TestInvalidPath tests if HTTP status code 404 is returned when invalid path is queried.
Expand Down Expand Up @@ -664,6 +755,7 @@ func TestV3TaskMetadata(t *testing.T) {
state.EXPECT().TaskARNByV3EndpointID(v3EndpointID).Return(taskARN, true),
state.EXPECT().TaskByArn(taskARN).Return(task, true),
state.EXPECT().ContainerMapByArn(taskARN).Return(containerNameToDockerContainer, true),
state.EXPECT().TaskByArn(taskARN).Return(task, true),
)
server := taskServerSetup(credentials.NewManager(), auditLog, state, ecsClient, clusterName, statsEngine,
config.DefaultTaskMetadataSteadyStateRate, config.DefaultTaskMetadataBurstRate, availabilityzone, containerInstanceArn)
Expand All @@ -679,6 +771,65 @@ func TestV3TaskMetadata(t *testing.T) {
assert.Equal(t, expectedTaskResponse, taskResponse)
}

func TestV3BridgeTaskMetadata(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

state := mock_dockerstate.NewMockTaskEngineState(ctrl)
auditLog := mock_audit.NewMockAuditLogger(ctrl)
statsEngine := mock_stats.NewMockEngine(ctrl)
ecsClient := mock_api.NewMockECSClient(ctrl)

gomock.InOrder(
state.EXPECT().TaskARNByV3EndpointID(v3EndpointID).Return(taskARN, true),
state.EXPECT().TaskByArn(taskARN).Return(bridgeTask, true),
state.EXPECT().ContainerMapByArn(taskARN).Return(containerNameToBridgeContainer, true),
state.EXPECT().TaskByArn(taskARN).Return(bridgeTask, true),
state.EXPECT().ContainerByID(containerID).Return(bridgeContainer, true),
)
server := taskServerSetup(credentials.NewManager(), auditLog, state, ecsClient, clusterName, statsEngine,
config.DefaultTaskMetadataSteadyStateRate, config.DefaultTaskMetadataBurstRate, availabilityzone, containerInstanceArn)
recorder := httptest.NewRecorder()
req, _ := http.NewRequest("GET", v3BasePath+v3EndpointID+"/task", nil)
server.Handler.ServeHTTP(recorder, req)
res, err := ioutil.ReadAll(recorder.Body)
assert.NoError(t, err)
assert.Equal(t, http.StatusOK, recorder.Code)
var taskResponse v2.TaskResponse
err = json.Unmarshal(res, &taskResponse)
assert.NoError(t, err)
assert.Equal(t, expectedBridgeTaskResponse, taskResponse)
}

func TestV3BridgeContainerMetadata(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

state := mock_dockerstate.NewMockTaskEngineState(ctrl)
auditLog := mock_audit.NewMockAuditLogger(ctrl)
statsEngine := mock_stats.NewMockEngine(ctrl)
ecsClient := mock_api.NewMockECSClient(ctrl)

gomock.InOrder(
state.EXPECT().DockerIDByV3EndpointID(v3EndpointID).Return(containerID, true),
state.EXPECT().ContainerByID(containerID).Return(bridgeContainer, true),
state.EXPECT().TaskByID(containerID).Return(bridgeTask, true),
state.EXPECT().ContainerByID(containerID).Return(bridgeContainer, true),
)
server := taskServerSetup(credentials.NewManager(), auditLog, state, ecsClient, clusterName, statsEngine,
config.DefaultTaskMetadataSteadyStateRate, config.DefaultTaskMetadataBurstRate, "", containerInstanceArn)
recorder := httptest.NewRecorder()
req, _ := http.NewRequest("GET", v3BasePath+v3EndpointID, nil)
server.Handler.ServeHTTP(recorder, req)
res, err := ioutil.ReadAll(recorder.Body)
assert.NoError(t, err)
assert.Equal(t, http.StatusOK, recorder.Code)
var containerResponse v2.ContainerResponse
err = json.Unmarshal(res, &containerResponse)
assert.NoError(t, err)
assert.Equal(t, expectedBridgeContainerResponse, containerResponse)
}

// Test API calls for propagating Tags to Task Metadata
func TestV3TaskMetadataWithTags(t *testing.T) {
ctrl := gomock.NewController(t)
Expand Down Expand Up @@ -734,6 +885,7 @@ func TestV3TaskMetadataWithTags(t *testing.T) {
Value: &taskTag2Val,
},
}, nil),
state.EXPECT().TaskByArn(taskARN).Return(task, true),
)
server := taskServerSetup(credentials.NewManager(), auditLog, state, ecsClient, clusterName, statsEngine,
config.DefaultTaskMetadataSteadyStateRate, config.DefaultTaskMetadataBurstRate, availabilityzone, containerInstanceArn)
Expand Down
64 changes: 61 additions & 3 deletions agent/handlers/v3/container_metadata_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ import (
"fmt"
"net/http"

"github.com/aws/amazon-ecs-agent/agent/containermetadata"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
"github.com/aws/amazon-ecs-agent/agent/handlers/utils"
"github.com/aws/amazon-ecs-agent/agent/handlers/v2"
"github.com/cihub/seelog"
"github.com/pkg/errors"
)

// ContainerMetadataPath specifies the relative URI path for serving container metadata.
Expand All @@ -37,10 +39,66 @@ func ContainerMetadataHandler(state dockerstate.TaskEngineState) func(http.Respo
utils.WriteJSONToResponse(w, http.StatusBadRequest, responseJSON, utils.RequestTypeContainerMetadata)
return
}

containerResponse, err := GetContainerResponse(containerID, state)
if err != nil {
errResponseJSON, _ := json.Marshal(err.Error())
utils.WriteJSONToResponse(w, http.StatusBadRequest, errResponseJSON, utils.RequestTypeContainerMetadata)
return
}
seelog.Infof("V3 container metadata handler: writing response for container '%s'", containerID)

// v3 handler shares the same container metadata response format with v2 handler.
v2.WriteContainerMetadataResponse(w, containerID, state)
responseJSON, _ := json.Marshal(containerResponse)
utils.WriteJSONToResponse(w, http.StatusOK, responseJSON, utils.RequestTypeContainerMetadata)
}
}

// GetContainerResponse gets container response for v3 metadata
func GetContainerResponse(containerID string, state dockerstate.TaskEngineState) (*v2.ContainerResponse, error) {
containerResponse, err := v2.NewContainerResponse(containerID, state)
Copy link
Contributor

Choose a reason for hiding this comment

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

for my understanding -

why aren't we keeping the logic to build up the responses in response.go where the rest of the structure is constructed? it's not obvious to me why we are appending this metadata a layer above.

same question for the task response structure.

Copy link
Contributor Author

@sharanyad sharanyad Feb 7, 2019

Choose a reason for hiding this comment

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

the network information is populated in v2's response.go only if the task has an ENI, since it should work only for awsvpc tasks. So I added logic here to add network response for non awsvpc tasks. let me know if that makes sense

if err != nil {
return nil, errors.Errorf("Unable to generate metadata for container '%s'", containerID)
}
// fill in network details if not set
if containerResponse.Networks == nil {
if containerResponse.Networks, err = GetContainerNetworkMetadata(containerID, state); err != nil {
return nil, err
}
}
return containerResponse, nil
}

// GetContainerNetworkMetadata returns the network metadata for the container
func GetContainerNetworkMetadata(containerID string, state dockerstate.TaskEngineState) ([]containermetadata.Network, error) {
dockerContainer, ok := state.ContainerByID(containerID)
if !ok {
return nil, errors.Errorf("Unable to find container '%s'", containerID)
}
// the logic here has been reused from
// https://github.com/aws/amazon-ecs-agent/blob/0c8913ba33965cf6ffdd6253fad422458d9346bd/agent/containermetadata/parse_metadata.go#L123
settings := dockerContainer.Container.GetNetworkSettings()
if settings == nil {
return nil, errors.Errorf("Unable to generate network response for container '%s'", containerID)
}
// This metadata is the information provided in older versions of the API
// We get the NetworkMode (Network interface name) from the HostConfig because this
// this is the network with which the container is created
ipv4AddressFromSettings := settings.IPAddress
networkModeFromHostConfig := dockerContainer.Container.GetNetworkMode()

// Extensive Network information is not available for Docker API versions 1.17-1.20
// Instead we only get the details of the first network
networks := make([]containermetadata.Network, 0)
if len(settings.Networks) > 0 {
for modeFromSettings, containerNetwork := range settings.Networks {
networkMode := modeFromSettings
ipv4Addresses := []string{containerNetwork.IPAddress}
network := containermetadata.Network{NetworkMode: networkMode, IPv4Addresses: ipv4Addresses}
networks = append(networks, network)
}
} else {
ipv4Addresses := []string{ipv4AddressFromSettings}
network := containermetadata.Network{NetworkMode: networkModeFromHostConfig, IPv4Addresses: ipv4Addresses}
networks = append(networks, network)
}
return networks, nil
}
Loading