From fad3e88269a3854c3283e6ef6330a759e52fd6cc Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Thu, 2 Feb 2017 18:31:07 -0800 Subject: [PATCH 1/2] Rename ECS Service node ids to be cluster;serviceName This is important for two reasons: * It prevents nasty false-equality bugs when two different services from different ECS clusters are present in the same report * It allows us to retrieve the cluster and service name - all the info we need to look up the service - using only the node ID. This matters, for example, when trying to handle a control request. --- probe/awsecs/reporter.go | 4 ++-- probe/awsecs/reporter_test.go | 4 ++-- render/detailed/parents.go | 2 +- render/detailed/summary.go | 2 +- report/id.go | 25 +++++++++++++++++++------ 5 files changed, 25 insertions(+), 12 deletions(-) diff --git a/probe/awsecs/reporter.go b/probe/awsecs/reporter.go index 95754d4a25..23cbc4e113 100644 --- a/probe/awsecs/reporter.go +++ b/probe/awsecs/reporter.go @@ -124,7 +124,7 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { // Create all the services first for serviceName, service := range ecsInfo.Services { - serviceID := report.MakeECSServiceNodeID(serviceName) + serviceID := report.MakeECSServiceNodeID(cluster, serviceName) rpt.ECSService = rpt.ECSService.AddNode(report.MakeNodeWith(serviceID, map[string]string{ Cluster: cluster, ServiceDesiredCount: fmt.Sprintf("%d", service.DesiredCount), @@ -153,7 +153,7 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { parentsSets := report.MakeSets() parentsSets = parentsSets.Add(report.ECSTask, report.MakeStringSet(taskID)) if serviceName, ok := ecsInfo.TaskServiceMap[taskArn]; ok { - serviceID := report.MakeECSServiceNodeID(serviceName) + serviceID := report.MakeECSServiceNodeID(cluster, serviceName) parentsSets = parentsSets.Add(report.ECSService, report.MakeStringSet(serviceID)) } for _, containerID := range info.ContainerIDs { diff --git a/probe/awsecs/reporter_test.go b/probe/awsecs/reporter_test.go index 117b5364d0..6f00f0c6ef 100644 --- a/probe/awsecs/reporter_test.go +++ b/probe/awsecs/reporter_test.go @@ -148,7 +148,7 @@ func TestTagReport(t *testing.T) { } // Check service node is present and contains expected values - service, ok := rpt.ECSService.Nodes[report.MakeECSServiceNodeID(testServiceName)] + service, ok := rpt.ECSService.Nodes[report.MakeECSServiceNodeID(testCluster, testServiceName)] if !ok { t.Fatalf("Result report did not contain service %v: %v", testServiceName, rpt.ECSService.Nodes) } @@ -175,7 +175,7 @@ func TestTagReport(t *testing.T) { } containerParentsExpected := map[string]string{ report.ECSTask: report.MakeECSTaskNodeID(testTaskARN), - report.ECSService: report.MakeECSServiceNodeID(testServiceName), + report.ECSService: report.MakeECSServiceNodeID(testCluster, testServiceName), } for key, expectedValue := range containerParentsExpected { values, ok := container.Parents.Lookup(key) diff --git a/render/detailed/parents.go b/render/detailed/parents.go index 910f1f49ab..e909d0e010 100644 --- a/render/detailed/parents.go +++ b/render/detailed/parents.go @@ -105,7 +105,7 @@ func ecsTaskParent(n report.Node) Parent { } func ecsServiceParent(n report.Node) Parent { - name, _ := report.ParseECSServiceNodeID(n.ID) + _, name, _ := report.ParseECSServiceNodeID(n.ID) return Parent{ ID: n.ID, Label: name, diff --git a/render/detailed/summary.go b/render/detailed/summary.go index e4ad49e242..b289bba532 100644 --- a/render/detailed/summary.go +++ b/render/detailed/summary.go @@ -249,7 +249,7 @@ func ecsTaskNodeSummary(base NodeSummary, n report.Node) (NodeSummary, bool) { } func ecsServiceNodeSummary(base NodeSummary, n report.Node) (NodeSummary, bool) { - base.Label, _ = report.ParseECSServiceNodeID(n.ID) + _, base.Label, _ = report.ParseECSServiceNodeID(n.ID) base.Stack = true return base, true } diff --git a/report/id.go b/report/id.go index 66a5b3418c..f243dafe82 100644 --- a/report/id.go +++ b/report/id.go @@ -77,6 +77,11 @@ func MakeProcessNodeID(hostID, pid string) string { return hostID + ScopeDelim + pid } +// MakeECSServiceNodeID produces an ECS Service node ID from its composite parts. +func MakeECSServiceNodeID(cluster, serviceName string) string { + return cluster + ScopeDelim + serviceName +} + var ( // MakeHostNodeID produces a host node ID from its composite parts. MakeHostNodeID = makeSingleComponentID("host") @@ -125,12 +130,6 @@ var ( // ParseECSTaskNodeID parses a replica set node ID ParseECSTaskNodeID = parseSingleComponentID("ecs_task") - - // MakeECSServiceNodeID produces a replica set node ID from its composite parts. - MakeECSServiceNodeID = makeSingleComponentID("ecs_service") - - // ParseECSServiceNodeID parses a replica set node ID - ParseECSServiceNodeID = parseSingleComponentID("ecs_service") ) // makeSingleComponentID makes a single-component node id encoder @@ -204,6 +203,20 @@ func ParseAddressNodeID(addressNodeID string) (hostID, address string, ok bool) return fields[0], fields[1], true } +// ParseECSServiceNodeID produces the cluster, service name from an ECS Service node ID +func ParseECSServiceNodeID(ecsServiceNodeID string) (cluster, serviceName string, ok bool) { + fields := strings.SplitN(ecsServiceNodeID, ScopeDelim, 2) + if len(fields) != 2 { + return "", "", false + } + // In previous versions, ECS Service node IDs were of form serviceName + "". + // For backwards compatibility, we should still return a sensical serviceName for these cases. + if fields[1] == "" { + return "unknown", fields[0], true + } + return fields[0], fields[1], true +} + // ExtractHostID extracts the host id from Node func ExtractHostID(m Node) string { hostNodeID, _ := m.Latest.Lookup(HostNodeID) From 1dcfe58b3de44b3bf7b36ea49f0799dc76e11987 Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Fri, 3 Feb 2017 13:55:11 -0800 Subject: [PATCH 2/2] Add test to check ECS Service node id parsing is back-compatible --- report/id_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/report/id_test.go b/report/id_test.go index 7854ae273a..f6306e5c12 100644 --- a/report/id_test.go +++ b/report/id_test.go @@ -66,3 +66,15 @@ func TestEndpointNodeID(t *testing.T) { } } } + +func TestECSServiceNodeIDCompat(t *testing.T) { + testID := "my-service;" + testName := "my-service" + _, name, ok := report.ParseECSServiceNodeID(testID) + if !ok { + t.Errorf("Failed to parse backwards-compatible id %q", testID) + } + if name != testName { + t.Errorf("Backwards-compatible id %q parsed name to %q, expected %q", testID, name, testName) + } +}