handlers: add eni info to instrospection endpoint #1271

adnxn · 2018-03-01T01:05:03Z

Summary

adding eni info to instrospection endpoint, addresses #1247

in awsvpc mode:

$ curl localhost:51678/v1/tasks | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   433  100   433    0     0   270k      0 --:--:-- --:--:-- --:--:--  422k
  {
      "Arn": "arn:aws:ecs:us-west-2:410584942969:task/c1bc8e40-35d9-4e1e-ba36-3e366fd8c97c",
      "DesiredStatus": "RUNNING",
      "KnownStatus": "RUNNING",
      "Family": "sleep300",
      "Version": "6",
      "Containers": [
        {
          "DockerId": "fc43f49749cfa09fba02ccf7cdf88c453604b05686b07b278de849466c32ec1f",
          "DockerName": "ecs-sleep300-6-sleep-aef3d2fbb494a5e08201",
          "Name": "sleep",
          "Ports": [],
          "Networks": [
            {
              "NetworkMode": "awsvpc",
              "IPv4Addresses": [
                "172.31.23.89"
              ]
            }
          ]
        }
      ]
    }

in bridge mode:

  {
      "Arn": "arn:aws:ecs:us-west-2:410584942969:task/5a54d2c3-9a13-42a0-82aa-4ffbe0570de1",
      "DesiredStatus": "RUNNING",
      "KnownStatus": "RUNNING",
      "Family": "sleep300",
      "Version": "8",
      "Containers": [
        {
          "DockerId": "58a5d93e855286c6d5557b2acc2aec9ef932e7da7c7004c73cfedb1f24bd8be2",
          "DockerName": "ecs-sleep300-8-sleep-beabc8d5aaf593ff0c00",
          "Name": "sleep",
          "Ports": [
            {
              "ContainerPort": 80,
              "Protocol": "tcp",
              "HostPort": 80
            },
            {
              "ContainerPort": 88,
              "Protocol": "tcp",
              "HostPort": 32769
            },
            {
              "ContainerPort": 90,
              "Protocol": "tcp",
              "HostPort": 32768
            }
          ],
          "Networks": null
        }
      ]
    }

in host mode:

  {
  "Tasks": [
    {
      "Arn": "arn:aws:ecs:us-west-2:410584942969:task/93a01f80-25c2-4517-9e5f-c302a8228623",
      "DesiredStatus": "RUNNING",
      "KnownStatus": "RUNNING",
      "Family": "sleep300",
      "Version": "7",
      "Containers": [
        {
          "DockerId": "cf8d7dc7532e0fc0b84605af13ecd6fb6822a38e50e1cb1fe774782d88a963b2",
          "DockerName": "ecs-sleep300-7-sleep-ceeaf4c78ea281ac3000",
          "Name": "sleep",
          "Ports": [
            {
              "ContainerPort": 80,
              "Protocol": "tcp",
              "HostPort": 80
            },
            {
              "ContainerPort": 90,
              "Protocol": "udp",
              "HostPort": 90
            }
          ],
          "Networks": null
        }
      ]
    }
  ]
}

Implementation details

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.

aaithal · 2018-03-01T16:06:19Z

agent/handlers/types/v1/response.go

@@ -28,6 +28,7 @@ type TaskResponse struct {
 	Family        string
 	Version       string
 	Containers    []ContainerResponse
+	Networks      []NetworkResponse


Similar to

amazon-ecs-agent/agent/handlers/types/v2/response.go

Line 60 in cc9fcff

Networks []containermetadata.Network `json:",omitempty"`

, this should be part of the ContainerResponse struct.

aaithal · 2018-03-01T20:11:41Z

agent/handlers/types/v1/response.go

+}
+
+// NetworkResponse is the schema for the network response JSON object
+type NetworkResponse struct {


You should just reuse this

amazon-ecs-agent/agent/containermetadata/types.go

Line 86 in cc9fcff

type Network struct {

. I don't think you need to create a new struct

aaithal · 2018-03-01T20:12:36Z

agent/handlers/v1_handlers.go

+	resp.DockerId = container.DockerID
+	resp.DockerName = container.DockerName
+
+	if eni != nil {


we should also populate resp.Networks when eni == nil , with whatever value we can get from container's metadata

richardpen · 2018-03-01T21:53:59Z

agent/handlers/v1_handlers.go

 	containers := []v1.ContainerResponse{}
 	for containerName, container := range containerMap {
 		if container.Container.IsInternal() {
 			continue
 		}
-		containers = append(containers, v1.ContainerResponse{DockerId: container.DockerID, DockerName: container.DockerName, Name: containerName})
+		containerResponse := newContainerResponse(containerName, container, task.GetTaskENI())
+		containers = append(containers, containerResponse)


nit: would it be more readable to use containerResponses instead of containers?

i decided against changing this because containerResponse and containerResponses are very similar

richardpen · 2018-03-01T22:41:51Z

agent/handlers/v1_handlers.go

+		DockerName: dockerContainer.DockerName,
+	}
+
+	for _, binding := range container.Ports {


Can you add a getter in the container struct to return the Ports and use the getter instead?

I'll make the change. Just wondering why we should use a getter if Ports is an exported field. I think we make it an exported field so we can serialize it.

yes, some other fields like KnowStatus is exported for the same purpose.

aaithal · 2018-03-01T23:50:38Z

agent/handlers/v1_handlers.go

+	if eni != nil {
+		resp.Networks = []containermetadata.Network{
+			{
+				NetworkMode:   "awsvpc",


minor, const this. there must be a const somewhere with that string.

aaithal · 2018-03-01T23:52:08Z

agent/api/container.go

+	c.lock.RLock()
+	defer c.lock.RUnlock()
+
+	return c.Ports


Can you rename Ports to PortsUnsafe?

richardpen · 2018-03-02T18:39:41Z

agent/api/container.go

@@ -493,6 +493,22 @@ func (c *Container) GetLabels() map[string]string {
 	return c.labels
 }

+// SetKnownPortBindings gets the ports for a container
+func (c *Container) SetKnownPortBindings(ports []PortBinding) {
+	c.lock.RLock()


This should be a write lock.

oops, good catch.

richardpen · 2018-03-02T18:40:12Z

agent/api/container.go

@@ -493,6 +493,22 @@ func (c *Container) GetLabels() map[string]string {
 	return c.labels
 }

+// SetKnownPortBindings gets the ports for a container


minor s/get/set/

richardpen · 2018-03-02T18:44:00Z

agent/handlers/v1_handlers.go

+
+		if eni == nil {
+			port.HostPort = binding.HostPort
+		} else {


If the container does use ENI, the binding should be empty, you should read the port information from container.Port. So I think you can do this:

bindings := container.GetKnownPortBindings() if len(bindings) == 0{ // Put the port information from task definition }else{ // put the information from bindings }

richardpen · 2018-03-02T19:54:39Z

agent/handlers/v1_handlers.go

+
+	// if KnownPortBindings list is empty, then we use the port mapping
+	// information that was passed down from ACS.
+	if len(bindings) == 0 {


Can you update the test to cover this?

aaithal · 2018-03-02T16:56:09Z

agent/handlers/types/v1/response.go

@@ -40,4 +45,6 @@ type ContainerResponse struct {
 	DockerId   string
 	DockerName string
 	Name       string
+	Ports      []v2.PortResponse


i think this needs an omitempty tag

ya you're right. added.

adnxn requested review from aaithal and richardpen March 1, 2018 01:05

aaithal requested changes Mar 1, 2018

View reviewed changes

adnxn added this to the 1.17.2 milestone Mar 1, 2018

adnxn force-pushed the eni-metadata branch from 72d6a78 to 1715fef Compare March 1, 2018 19:51

aaithal reviewed Mar 1, 2018

View reviewed changes

richardpen reviewed Mar 1, 2018

View reviewed changes

adnxn force-pushed the eni-metadata branch from 1715fef to 5b48302 Compare March 1, 2018 22:22

adnxn changed the title ~~[wip] handlers: add eni info to instrospection endpoint~~ handlers: add eni info to instrospection endpoint Mar 1, 2018

adnxn force-pushed the eni-metadata branch 2 times, most recently from e777310 to 3b39656 Compare March 1, 2018 22:33

richardpen reviewed Mar 1, 2018

View reviewed changes

handlers: add eni info to introspection endpoint

a490ba1

adnxn force-pushed the eni-metadata branch from 3b39656 to a490ba1 Compare March 1, 2018 23:09

aaithal reviewed Mar 1, 2018

View reviewed changes

richardpen approved these changes Mar 2, 2018

View reviewed changes

adnxn force-pushed the eni-metadata branch from 048eb8c to 0d58ae1 Compare March 2, 2018 18:33

api: add set/get for KnownPortBindings field

d9d127f

adnxn force-pushed the eni-metadata branch from 0d58ae1 to d9d127f Compare March 2, 2018 18:35

richardpen reviewed Mar 2, 2018

View reviewed changes

aaithal reviewed Mar 2, 2018

View reviewed changes

adnxn force-pushed the eni-metadata branch 2 times, most recently from a02f0bf to 374b08b Compare March 2, 2018 20:15

handler: use container.Ports when KnownPortsBindings empty

64a0760

adnxn force-pushed the eni-metadata branch from 374b08b to 64a0760 Compare March 2, 2018 21:12

richardpen approved these changes Mar 2, 2018

View reviewed changes

aaithal approved these changes Mar 2, 2018

View reviewed changes

adnxn merged commit 64a0760 into aws:dev Mar 2, 2018

adnxn added a commit that referenced this pull request Mar 2, 2018

Merge pull request #1271 from adnxn/eni-metadata

0fb8ab5

adnxn deleted the eni-metadata branch October 19, 2018 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handlers: add eni info to instrospection endpoint #1271

handlers: add eni info to instrospection endpoint #1271

adnxn commented Mar 1, 2018 •

edited

Loading

aaithal Mar 1, 2018

aaithal Mar 1, 2018

aaithal Mar 1, 2018

richardpen Mar 1, 2018

adnxn Mar 1, 2018

richardpen Mar 1, 2018

adnxn Mar 1, 2018

richardpen Mar 1, 2018

aaithal Mar 1, 2018

aaithal Mar 1, 2018

richardpen Mar 2, 2018

adnxn Mar 2, 2018

richardpen Mar 2, 2018

richardpen Mar 2, 2018

richardpen Mar 2, 2018

aaithal Mar 2, 2018

adnxn Mar 2, 2018

handlers: add eni info to instrospection endpoint #1271

handlers: add eni info to instrospection endpoint #1271

Conversation

adnxn commented Mar 1, 2018 • 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

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 Mar 1, 2018 •

edited

Loading