Skip to content

Commit

Permalink
ECS Service Discovery: Fixup implicit network mode
Browse files Browse the repository at this point in the history
I originally submitted #330
with the goal of correcting the service discovery behavior of a task when its
definition did not specify a network mode.

Very graciously, @javabrett submitted a fix for this via
#335. Upon testing this out
today, I found out we're not _quite_ there.

Crucially, when network mode is undefined in a task definition, it will parse as
`nil`, not the empty string. Correcting this in our unit tests reveals the
behavior isn't working as desired, since the `getPrivateIp` function isn't
properly handling the `nil` case. I believe this addresses the issue.
  • Loading branch information
Will Myers committed Feb 28, 2022
1 parent c2219fa commit c70d7bb
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 10 deletions.
5 changes: 3 additions & 2 deletions internal/ecsservicediscovery/decoratedtask.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,13 @@ func addExporterLabels(labels map[string]string, labelKey string, labelValue *st
// Get the private ip of the decorated task.
// Return "" when fail to get the private ip
func (t *DecoratedTask) getPrivateIp() string {
if t.TaskDefinition.NetworkMode == nil || *t.TaskDefinition.NetworkMode == ecs.NetworkModeNone {
networkMode := aws.StringValue(t.TaskDefinition.NetworkMode)
if networkMode == ecs.NetworkModeNone {
return ""
}

// AWSVPC: Get Private IP from tasks->attachments (ElasticNetworkInterface -> privateIPv4Address)
if *t.TaskDefinition.NetworkMode == ecs.NetworkModeAwsvpc {
if networkMode == ecs.NetworkModeAwsvpc {
for _, v := range t.Task.Attachments {
if aws.StringValue(v.Type) == "ElasticNetworkInterface" {
for _, d := range v.Details {
Expand Down
17 changes: 9 additions & 8 deletions internal/ecsservicediscovery/decoratedtask_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ func Test_ExportDockerLabelBasedTarget_Fargate_AWSVPC(t *testing.T) {
assert.Equal(t, "/metrics", target.Labels["__metrics_path__"])
assert.Equal(t, "/metrics", target.Labels["ECS_PROMETHEUS_METRICS_PATH"])


}

func Test_ExportTaskDefBasedTarget_Fargate_AWSVPC(t *testing.T) {
Expand Down Expand Up @@ -307,7 +306,7 @@ func Test_ExportMixedSDTarget_Fargate_AWSVPC(t *testing.T) {
assert.True(t, ok, "Missing target: 10.0.0.129:9406/metrics")
}

func buildWorkloadEC2BridgeDynamicPort(dockerLabel bool, taskDef bool, serviceName string, networkMode string) *DecoratedTask {
func buildWorkloadEC2BridgeDynamicPort(dockerLabel bool, taskDef bool, serviceName string, networkMode *string) *DecoratedTask {
taskContainersArn := "arn:aws:ecs:us-east-2:211220956907:container/3b288961-eb2c-4de5-a4c5-682c0a7cc625"
var taskContainersDynamicHostPort int64 = 32774
var taskContainersMappedHostPort int64 = 9494
Expand Down Expand Up @@ -367,7 +366,7 @@ func buildWorkloadEC2BridgeDynamicPort(dockerLabel bool, taskDef bool, serviceNa
},
},
TaskDefinition: &ecs.TaskDefinition{
NetworkMode: &networkMode,
NetworkMode: networkMode,
TaskDefinitionArn: &taskDefinitionArn,
Revision: &taskRevision,
ContainerDefinitions: []*ecs.ContainerDefinition{
Expand Down Expand Up @@ -403,18 +402,20 @@ func buildWorkloadEC2BridgeDynamicPort(dockerLabel bool, taskDef bool, serviceNa
}

func Test_ExportMixedSDTarget_EC2_Bridge_DynamicPort(t *testing.T) {
testExportMixedSDTarget_EC2_Bridge_DynamicPort(t, ecs.NetworkModeBridge, 2)
networkMode := ecs.NetworkModeBridge
testExportMixedSDTarget_EC2_Bridge_DynamicPort(t, &networkMode, 2)
}

func Test_ExportMixedSDTarget_EC2_Bridge_DynamicPort_With_Implicit_NetworkMode(t *testing.T) {
testExportMixedSDTarget_EC2_Bridge_DynamicPort(t, "", 2)
testExportMixedSDTarget_EC2_Bridge_DynamicPort(t, nil, 2)
}

func Test_ExportMixedSDTarget_EC2_Bridge_DynamicPort_With_NetworkModeNone(t *testing.T) {
testExportMixedSDTarget_EC2_Bridge_DynamicPort(t, ecs.NetworkModeNone, 0)
networkMode := ecs.NetworkModeNone
testExportMixedSDTarget_EC2_Bridge_DynamicPort(t, &networkMode, 0)
}

func testExportMixedSDTarget_EC2_Bridge_DynamicPort(t *testing.T, networkMode string, expectedTargets int) {
func testExportMixedSDTarget_EC2_Bridge_DynamicPort(t *testing.T, networkMode *string, expectedTargets int) {
fullTask := buildWorkloadEC2BridgeDynamicPort(true, true, "", networkMode)
if expectedTargets == 0 {
assert.Equal(t, "", fullTask.getPrivateIp())
Expand Down Expand Up @@ -492,7 +493,7 @@ func Test_ExportContainerNameSDTarget_EC2_Bridge_DynamicPort_With_NetworkModeNon
}

func testExportContainerNameSDTarget_EC2_Bridge_DynamicPort(t *testing.T, networkMode string, expectedTargets int) {
fullTask := buildWorkloadEC2BridgeDynamicPort(false, true, "", networkMode)
fullTask := buildWorkloadEC2BridgeDynamicPort(false, true, "", &networkMode)
log.Print(fullTask)
if expectedTargets == 0 {
assert.Equal(t, "", fullTask.getPrivateIp())
Expand Down

0 comments on commit c70d7bb

Please sign in to comment.