Write tests to include testing determinancy of various slice orders; ensure...

Write tests to include testing determinancy of various slice orders; ensure that container order is deterministic

retrieval/discovery/kubernetes/discovery.go

@@ -904,6 +904,12 @@ func (a ByContainerPort) Len() int           { return len(a) }
 func (a ByContainerPort) Less(i, j int) bool { return a[i].ContainerPort < a[j].ContainerPort }
 func (a ByContainerPort) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
 
+type ByContainerName []Container
+
+func (a ByContainerName) Len() int           { return len(a) }
+func (a ByContainerName) Less(i, j int) bool { return a[i].Name < a[j].Name }
+func (a ByContainerName) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
+
 func updatePodTargets(pod *Pod, allContainers bool) []model.LabelSet {
 	var targets []model.LabelSet = make([]model.LabelSet, 0, len(pod.PodSpec.Containers))
 	if pod.PodStatus.PodIP == "" {
@@ -923,6 +929,8 @@ func updatePodTargets(pod *Pod, allContainers bool) []model.LabelSet {
 		}
 	}
 
+	sort.Sort(ByContainerName(pod.PodSpec.Containers))
+
 	for _, container := range pod.PodSpec.Containers {
 		// Collect a list of TCP ports
 		// Sort by port number, ascending
@@ -959,7 +967,7 @@ func updatePodTargets(pod *Pod, allContainers bool) []model.LabelSet {
 			portLabel.WriteString("=")
 			portLabel.WriteString(strconv.FormatInt(int64(port.ContainerPort), 10))
 			portLabel.WriteString(",")
-			t[model.LabelName(podContainerPortMapPrefix + port.Name)] = model.LabelValue(strconv.FormatInt(int64(port.ContainerPort), 10))
+			t[model.LabelName(podContainerPortMapPrefix+port.Name)] = model.LabelValue(strconv.FormatInt(int64(port.ContainerPort), 10))
 		}
 
 		t[model.LabelName(podContainerPortListLabel)] = model.LabelValue(portLabel.String())

retrieval/discovery/kubernetes/discovery_test.go

@@ -15,6 +15,7 @@ package kubernetes
 
 import (
 	"flag"
+	"math/rand"
 	"os"
 	"testing"
 
@@ -27,72 +28,82 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
-var containerA = Container{
-	Name: "a",
-	Ports: []ContainerPort{
-		ContainerPort{
-			Name:          "http",
-			ContainerPort: 80,
-			Protocol:      "TCP",
-		},
+var portsA = []ContainerPort{
+	ContainerPort{
+		Name:          "http",
+		ContainerPort: 80,
+		Protocol:      "TCP",
 	},
 }
 
-var containerB = Container{
-	Name: "b",
-	Ports: []ContainerPort{
-		ContainerPort{
-			Name:          "https",
-			ContainerPort: 443,
-			Protocol:      "TCP",
-		},
+var portsB = []ContainerPort{
+	ContainerPort{
+		Name:          "https",
+		ContainerPort: 443,
+		Protocol:      "TCP",
 	},
 }
 
-var containerNoTcp = Container{
-	Name: "no-tcp",
-	Ports: []ContainerPort{
-		ContainerPort{
-			Name:          "dns",
-			ContainerPort: 53,
-			Protocol:      "UDP",
-		},
+var portsNoTcp = []ContainerPort{
+	ContainerPort{
+		Name:          "dns",
+		ContainerPort: 53,
+		Protocol:      "UDP",
 	},
 }
 
-var containerMultiA = Container{
-	Name: "a",
-	Ports: []ContainerPort{
-		ContainerPort{
-			Name:          "http",
-			ContainerPort: 80,
-			Protocol:      "TCP",
-		},
-		ContainerPort{
-			Name:          "ssh",
-			ContainerPort: 22,
-			Protocol:      "TCP",
-		},
+var portsMultiA = []ContainerPort{
+	ContainerPort{
+		Name:          "http",
+		ContainerPort: 80,
+		Protocol:      "TCP",
+	},
+	ContainerPort{
+		Name:          "ssh",
+		ContainerPort: 22,
+		Protocol:      "TCP",
 	},
 }
 
-var containerMultiB = Container{
-	Name: "b",
-	Ports: []ContainerPort{
-		ContainerPort{
-			Name:          "http",
-			ContainerPort: 80,
-			Protocol:      "TCP",
-		},
-		ContainerPort{
-			Name:          "https",
-			ContainerPort: 443,
-			Protocol:      "TCP",
-		},
+var portsMultiB = []ContainerPort{
+	ContainerPort{
+		Name:          "http",
+		ContainerPort: 80,
+		Protocol:      "TCP",
 	},
+	ContainerPort{
+		Name:          "https",
+		ContainerPort: 443,
+		Protocol:      "TCP",
+	},
+}
+
+func container(name string, ports []ContainerPort) Container {
+	p := make([]ContainerPort, len(ports))
+	copy(p, ports)
+
+	// Shuffle order of ports to ensure code enforces determinism
+	for i := range p {
+		j := rand.Intn(i + 1)
+		p[i], p[j] = p[j], p[i]
+	}
+
+	return Container{
+		Name:  name,
+		Ports: p,
+	}
 }
 
 func pod(name string, containers []Container) *Pod {
+	c := make([]Container, len(containers))
+	copy(c, containers)
+
+	// Shuffle order of containers to ensure code enforces determinism
+	for i := range c {
+		j := rand.Intn(i + 1)
+		c[i], c[j] = c[j], c[i]
+	}
+
 	return &Pod{
 		ObjectMeta: ObjectMeta{
 			Name: name,
@@ -108,7 +119,7 @@ func pod(name string, containers []Container) *Pod {
 			},
 		},
 		PodSpec: PodSpec{
-			Containers: containers,
+			Containers: c,
 		},
 	}
 }
@@ -116,72 +127,81 @@ func pod(name string, containers []Container) *Pod {
 func TestUpdatePodTargets(t *testing.T) {
 	var result []model.LabelSet
 
-	// Return no targets for a pod that isn't "Running"
-	result = updatePodTargets(&Pod{PodStatus: PodStatus{PodIP: "1.1.1.1"}}, true)
-	if len(result) > 0 {
-		t.Fatalf("expected 0 targets, received %d", len(result))
-	}
-
-	// Return no targets for a pod with no IP
-	result = updatePodTargets(&Pod{PodStatus: PodStatus{Phase: "Running"}}, true)
-	if len(result) > 0 {
-		t.Fatalf("expected 0 targets, received %d", len(result))
-	}
-
-	// A pod with no containers (?!) should not produce any targets
-	result = updatePodTargets(pod("empty", []Container{}), true)
-	if len(result) > 0 {
-		t.Fatalf("expected 0 targets, received %d", len(result))
-	}
-
-	// A pod with all valid containers should return one target per container with allContainers=true
-	result = updatePodTargets(pod("easy", []Container{containerA, containerB}), true)
-	if len(result) != 2 {
-		t.Fatalf("expected 2 targets, received %d", len(result))
-	}
-	if result[0][podReadyLabel] != "true" {
-		t.Fatalf("expected result[0] podReadyLabel 'true', received '%s'", result[0][podReadyLabel])
-	}
-	if _, ok := result[0][podContainerPortMapPrefix + "http"]; !ok {
-	  t.Fatalf("expected result[0][podContainerPortMapPrefix + 'http'] to be '80', but was missing")
-	}
-	if result[0][podContainerPortMapPrefix + "http"] != "80" {
-	  t.Fatalf("expected result[0][podContainerPortMapPrefix + 'http'] to be '80', but was %s", result[0][podContainerPortMapPrefix + "http"])
-	}
-	if _, ok := result[1][podContainerPortMapPrefix + "https"]; !ok {
-	  t.Fatalf("expected result[1][podContainerPortMapPrefix + 'https'] to be '443', but was missing")
-	}
-	if result[1][podContainerPortMapPrefix + "https"] != "443" {
-	  t.Fatalf("expected result[1][podContainerPortMapPrefix + 'https'] to be '443', but was %s", result[1][podContainerPortMapPrefix + "https"])
-	}
-
-	// A pod with all valid containers should return one target with allContainers=false
-	result = updatePodTargets(pod("easy", []Container{containerA, containerB}), false)
-	if len(result) != 1 {
-		t.Fatalf("expected 1 targets, received %d", len(result))
-	}
-
-	// A pod with some non-targetable containers should return one target per targetable container with allContainers=true
-	result = updatePodTargets(pod("mixed", []Container{containerA, containerNoTcp, containerB}), true)
-	if len(result) != 2 {
-		t.Fatalf("expected 2 targets, received %d", len(result))
-	}
-
-	// A pod with a container with multiple ports should return the numerically smallest port
-	result = updatePodTargets(pod("hard", []Container{containerMultiA, containerMultiB}), true)
-	if len(result) != 2 {
-		t.Fatalf("expected 2 targets, received %d", len(result))
-	}
-	if result[0][model.AddressLabel] != "1.1.1.1:22" {
-		t.Fatalf("expected result[0] address to be 1.1.1.1:22, received %s", result[0][model.AddressLabel])
-	}
-	if result[0][podContainerPortListLabel] != ",ssh=22,http=80," {
-		t.Fatalf("expected result[0] podContainerPortListLabel to be ',ssh=22,http=80,', received '%s'", result[0][podContainerPortListLabel])
-	}
-	if result[1][model.AddressLabel] != "1.1.1.1:80" {
-		t.Fatalf("expected result[1] address to be 1.1.1.1:80, received %s", result[1][model.AddressLabel])
-	}
-	if result[1][podContainerPortListLabel] != ",http=80,https=443," {
-		t.Fatalf("expected result[1] podContainerPortListLabel to be ',http=80,https=443,', received '%s'", result[1][podContainerPortListLabel])
+	// Multiple iterations help ensure that we'll see different permutations via the various randomizations that occur
+	for i := 0; i < 100; i++ {
+		// Return no targets for a pod that isn't "Running"
+		result = updatePodTargets(&Pod{PodStatus: PodStatus{PodIP: "1.1.1.1"}}, true)
+		if len(result) > 0 {
+			t.Fatalf("expected 0 targets, received %d", len(result))
+		}
+
+		// Return no targets for a pod with no IP
+		result = updatePodTargets(&Pod{PodStatus: PodStatus{Phase: "Running"}}, true)
+		if len(result) > 0 {
+			t.Fatalf("expected 0 targets, received %d", len(result))
+		}
+
+		// A pod with no containers (?!) should not produce any targets
+		result = updatePodTargets(pod("empty", []Container{}), true)
+		if len(result) > 0 {
+			t.Fatalf("expected 0 targets, received %d", len(result))
+		}
+
+		// A pod with all valid containers should return one target per container with allContainers=true
+		result = updatePodTargets(pod("easy", []Container{container("a", portsA), container("b", portsB)}), true)
+		if len(result) != 2 {
+			t.Fatalf("expected 2 targets, received %d", len(result))
+		}
+		if result[0][podReadyLabel] != "true" {
+			t.Fatalf("expected result[0] podReadyLabel 'true', received '%s'", result[0][podReadyLabel])
+		}
+		if _, ok := result[0][podContainerPortMapPrefix+"http"]; !ok {
+			t.Fatalf("expected result[0][podContainerPortMapPrefix + 'http'] to be '80', but was missing")
+		}
+		if result[0][podContainerPortMapPrefix+"http"] != "80" {
+			t.Fatalf("expected result[0][podContainerPortMapPrefix + 'http'] to be '80', but was %s", result[0][podContainerPortMapPrefix+"http"])
+		}
+		if _, ok := result[1][podContainerPortMapPrefix+"https"]; !ok {
+			t.Fatalf("expected result[1][podContainerPortMapPrefix + 'https'] to be '443', but was missing")
+		}
+		if result[1][podContainerPortMapPrefix+"https"] != "443" {
+			t.Fatalf("expected result[1][podContainerPortMapPrefix + 'https'] to be '443', but was %s", result[1][podContainerPortMapPrefix+"https"])
+		}
+
+		// A pod with all valid containers should return one target with allContainers=false, and it should be the alphabetically first container
+		result = updatePodTargets(pod("easy", []Container{container("a", portsA), container("b", portsB)}), false)
+		if len(result) != 1 {
+			t.Fatalf("expected 1 targets, received %d", len(result))
+		}
+		if _, ok := result[0][podContainerNameLabel]; !ok {
+			t.Fatalf("expected result[0][podContainerNameLabel] to be 'a', but was missing")
+		}
+		if result[0][podContainerNameLabel] != "a" {
+			t.Fatalf("expected result[0][podContainerNameLabel] to be 'a', but was '%s'", result[0][podContainerNameLabel])
+		}
+
+		// A pod with some non-targetable containers should return one target per targetable container with allContainers=true
+		result = updatePodTargets(pod("mixed", []Container{container("a", portsA), container("no-tcp", portsNoTcp), container("b", portsB)}), true)
+		if len(result) != 2 {
+			t.Fatalf("expected 2 targets, received %d", len(result))
+		}
+
+		// A pod with a container with multiple ports should return the numerically smallest port
+		result = updatePodTargets(pod("hard", []Container{container("multiA", portsMultiA), container("multiB", portsMultiB)}), true)
+		if len(result) != 2 {
+			t.Fatalf("expected 2 targets, received %d", len(result))
+		}
+		if result[0][model.AddressLabel] != "1.1.1.1:22" {
+			t.Fatalf("expected result[0] address to be 1.1.1.1:22, received %s", result[0][model.AddressLabel])
+		}
+		if result[0][podContainerPortListLabel] != ",ssh=22,http=80," {
+			t.Fatalf("expected result[0] podContainerPortListLabel to be ',ssh=22,http=80,', received '%s'", result[0][podContainerPortListLabel])
+		}
+		if result[1][model.AddressLabel] != "1.1.1.1:80" {
+			t.Fatalf("expected result[1] address to be 1.1.1.1:80, received %s", result[1][model.AddressLabel])
+		}
+		if result[1][podContainerPortListLabel] != ",http=80,https=443," {
+			t.Fatalf("expected result[1] podContainerPortListLabel to be ',http=80,https=443,', received '%s'", result[1][podContainerPortListLabel])
+		}
 	}
 }