Fix incomplete topology when destination port is not found

pkg/topology/builder.go

@@ -90,65 +90,65 @@ func (b *Builder) evaluateTrafficTarget(res *resources, topology *Topology, tt *
 
 	sources := b.buildTrafficTargetSources(res, topology, tt)
 
-	specs, err := b.buildTrafficTargetSpecs(res, tt)
-	if err != nil {
-		err = fmt.Errorf("unable to build spec: %v", err)
-		setTrafficTargetWithErr(topology, tt, sources, err)
-		b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", Key{tt.Name, tt.Namespace}, err)
-
-		return
-	}
-
 	for svcKey, pods := range res.PodsBySvcBySa[destSaKey] {
+		stt := &ServiceTrafficTarget{
+			Name:      tt.Name,
+			Namespace: tt.Namespace,
+			Service:   svcKey,
+			Sources:   sources,
+			Destination: ServiceTrafficTargetDestination{
+				ServiceAccount: tt.Destination.Name,
+				Namespace:      tt.Destination.Namespace,
+			},
+		}
+
 		svcTTKey := ServiceTrafficTargetKey{
 			Service:       svcKey,
 			TrafficTarget: Key{tt.Name, tt.Namespace},
 		}
+		topology.ServiceTrafficTargets[svcTTKey] = stt
 
 		svc, ok := topology.Services[svcKey]
 		if !ok {
-			err = fmt.Errorf("unable to find Service %q", svcKey)
-			setTrafficTargetWithErr(topology, tt, sources, err)
+			err := fmt.Errorf("unable to find Service %q", svcKey)
+			stt.AddError(err)
 			b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", Key{tt.Name, tt.Namespace}, err)
 
-			return
+			continue
 		}
 
-		var destPods []Key
+		// Build the TrafficTarget Specs.
+		specs, err := b.buildTrafficTargetSpecs(res, tt)
+		if err != nil {
+			err = fmt.Errorf("unable to build spec: %v", err)
+			stt.AddError(err)
+			b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", Key{tt.Name, tt.Namespace}, err)
+
+			continue
+		}
+
+		stt.Specs = specs
 
 		// Find out which are the destination pods.
 		for _, pod := range pods {
 			if pod.Status.PodIP == "" {
 				continue
 			}
 
-			destPods = append(destPods, getOrCreatePod(topology, pod))
+			stt.Destination.Pods = append(stt.Destination.Pods, getOrCreatePod(topology, pod))
 		}
 
 		// Find out which ports can be used on the destination service.
 		destPorts, err := b.getTrafficTargetDestinationPorts(svc, tt)
 		if err != nil {
 			err = fmt.Errorf("unable to find destination ports on Service %q: %w", svcKey, err)
-			setTrafficTargetWithErr(topology, tt, sources, err)
+			stt.AddError(err)
 			b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", Key{tt.Name, tt.Namespace}, err)
 
-			return
+			continue
 		}
 
-		// Create the ServiceTrafficTarget for the given service.
-		topology.ServiceTrafficTargets[svcTTKey] = &ServiceTrafficTarget{
-			Service:   svcKey,
-			Name:      tt.Name,
-			Namespace: tt.Namespace,
-			Sources:   sources,
-			Destination: ServiceTrafficTargetDestination{
-				ServiceAccount: tt.Destination.Name,
-				Namespace:      tt.Destination.Namespace,
-				Ports:          destPorts,
-				Pods:           destPods,
-			},
-			Specs: specs,
-		}
+		stt.Destination.Ports = destPorts
 
 		svc.TrafficTargets = append(svc.TrafficTargets, svcTTKey)
 
@@ -179,74 +179,61 @@ func addSourceAndDestinationToPods(topology *Topology, sources []ServiceTrafficT
 	}
 }
 
-func setTrafficTargetWithErr(topology *Topology, tt *access.TrafficTarget, sources []ServiceTrafficTargetSource, err error) {
-	stt := &ServiceTrafficTarget{
-		Name:      tt.Name,
-		Namespace: tt.Namespace,
-		Sources:   sources,
-		Destination: ServiceTrafficTargetDestination{
-			ServiceAccount: tt.Destination.Name,
-			Namespace:      tt.Destination.Namespace,
-		},
-	}
-	stt.AddError(err)
-
-	svcTTKey := ServiceTrafficTargetKey{TrafficTarget: Key{tt.Name, tt.Namespace}}
-	topology.ServiceTrafficTargets[svcTTKey] = stt
-}
-
 // evaluateTrafficSplit evaluates the given traffic-split. If the traffic-split targets a known Service, a new TrafficSplit
 // will be added to it. The TrafficSplit will be added only if all its backends expose the ports required by the Service.
 func (b *Builder) evaluateTrafficSplit(topology *Topology, trafficSplit *split.TrafficSplit) {
 	svcKey := Key{trafficSplit.Spec.Service, trafficSplit.Namespace}
+	ts := &TrafficSplit{
+		Name:      trafficSplit.Name,
+		Namespace: trafficSplit.Namespace,
+		Service:   svcKey,
+	}
+
 	tsKey := Key{trafficSplit.Name, trafficSplit.Namespace}
+	topology.TrafficSplits[tsKey] = ts
 
 	svc, ok := topology.Services[svcKey]
 	if !ok {
 		err := fmt.Errorf("unable to find root Service %q", svcKey)
-		setTrafficSplitWithErr(topology, trafficSplit, svcKey, err)
+		ts.AddError(err)
 		b.Logger.Errorf("Error building topology for TrafficSplit %q: %v", tsKey, err)
 
 		return
 	}
 
-	backends := make([]TrafficSplitBackend, len(trafficSplit.Spec.Backends))
-
-	for i, backend := range trafficSplit.Spec.Backends {
+	for _, backend := range trafficSplit.Spec.Backends {
 		backendSvcKey := Key{backend.Service, trafficSplit.Namespace}
 
 		backendSvc, ok := topology.Services[backendSvcKey]
 		if !ok {
 			err := fmt.Errorf("unable to find backend Service %q", backendSvcKey)
-			setTrafficSplitWithErr(topology, trafficSplit, svcKey, err)
+			ts.AddError(err)
 			b.Logger.Errorf("Error building topology for TrafficSplit %q: %v", tsKey, err)
 
-			return
+			continue
 		}
 
 		// As required by the SMI specification, backends must expose at least the same ports as the Service on
 		// which the TrafficSplit is.
-		b.validateServiceAndBackendPorts(svc.Ports, backendSvc.Ports, topology, trafficSplit, svcKey, tsKey, backendSvcKey)
+		if err := b.validateServiceAndBackendPorts(svc.Ports, backendSvc.Ports); err != nil {
+			ts.AddError(err)
+			b.Logger.Errorf("Error building topology for TrafficSplit %q: backend %q and service %q ports mismatch: %v", tsKey, backendSvcKey, svcKey, err)
+
+			continue
+		}
 
-		backends[i] = TrafficSplitBackend{
+		ts.Backends = append(ts.Backends, TrafficSplitBackend{
 			Weight:  backend.Weight,
 			Service: backendSvcKey,
-		}
+		})
 
 		backendSvc.BackendOf = append(backendSvc.BackendOf, tsKey)
 	}
 
-	topology.TrafficSplits[tsKey] = &TrafficSplit{
-		Name:      trafficSplit.Name,
-		Namespace: trafficSplit.Namespace,
-		Service:   svcKey,
-		Backends:  backends,
-	}
-
 	svc.TrafficSplits = append(svc.TrafficSplits, tsKey)
 }
 
-func (b *Builder) validateServiceAndBackendPorts(svcPorts []corev1.ServicePort, backendPorts []corev1.ServicePort, topology *Topology, trafficSplit *split.TrafficSplit, svcKey Key, tsKey Key, backendSvcKey Key) {
+func (b *Builder) validateServiceAndBackendPorts(svcPorts []corev1.ServicePort, backendPorts []corev1.ServicePort) error {
 	for _, svcPort := range svcPorts {
 		var portFound bool
 
@@ -258,25 +245,11 @@ func (b *Builder) validateServiceAndBackendPorts(svcPorts []corev1.ServicePort,
 		}
 
 		if !portFound {
-			err := fmt.Errorf("port %d must be exposed by Service %q in order to be used as a backend", svcPort.Port, backendSvcKey)
-			setTrafficSplitWithErr(topology, trafficSplit, svcKey, err)
-			b.Logger.Errorf("Error building topology for TrafficSplit %q: %v", tsKey, err)
-
-			return
+			return fmt.Errorf("port %d must be exposed", svcPort.Port)
 		}
 	}
-}
-
-func setTrafficSplitWithErr(topology *Topology, trafficSplit *split.TrafficSplit, svcKey Key, err error) {
-	ts := &TrafficSplit{
-		Name:      trafficSplit.Name,
-		Namespace: trafficSplit.Namespace,
-		Service:   svcKey,
-	}
-	ts.AddError(err)
 
-	tsKey := Key{trafficSplit.Name, trafficSplit.Namespace}
-	topology.TrafficSplits[tsKey] = ts
+	return nil
 }
 
 // populateTrafficSplitsAuthorizedIncomingTraffic computes the list of pods allowed to access a traffic-split. As

pkg/topology/builder_test.go

@@ -433,6 +433,44 @@ func TestTopologyBuilder_BuildWithTrafficTargetEmptyDestinationPort(t *testing.T
 	assertTopology(t, "testdata/topology-empty-destination-port.json", got)
 }
 
+func TestTopologyBuilder_BuildWithTrafficTargetAndMismatchServicePort(t *testing.T) {
+	annotations := map[string]string{}
+
+	selectorAppA := map[string]string{"app": "app-a"}
+	saA := createServiceAccount("my-ns", "service-account-a")
+	podA := createPod("my-ns", "app-a", saA, selectorAppA, "10.10.1.1")
+
+	saB := createServiceAccount("my-ns", "service-account-b")
+
+	selectorAppB1 := map[string]string{"app": "app-b1"}
+	svcB1Ports := []corev1.ServicePort{svcPort("port-8080", 8080, 8080)}
+	podB1 := createPod("my-ns", "app-b1", saB, selectorAppB1, "10.10.1.2")
+	svcB1 := createService("my-ns", "svc-b1", annotations, svcB1Ports, selectorAppB1, "10.10.1.16")
+	epB1 := createEndpoints(svcB1, []*corev1.Pod{podB1})
+
+	selectorAppB2 := map[string]string{"app": "app-b2"}
+	svcB2Ports := []corev1.ServicePort{svcPort("port-80", 80, 80)}
+	podB2 := createPod("my-ns", "app-b2", saB, selectorAppB2, "10.10.1.3")
+	svcB2 := createService("my-ns", "svc-b2", annotations, svcB2Ports, selectorAppB2, "10.10.1.17")
+	epB2 := createEndpoints(svcB2, []*corev1.Pod{podB2})
+
+	tt := createTrafficTarget("my-ns", "tt", saB, "80", []*corev1.ServiceAccount{saA}, nil, []string{})
+
+	k8sClient := fake.NewSimpleClientset(saA, podA, saB, podB1, svcB1, epB1, podB2, svcB2, epB2)
+	smiAccessClient := accessfake.NewSimpleClientset(tt)
+	smiSplitClient := splitfake.NewSimpleClientset()
+	smiSpecClient := specfake.NewSimpleClientset()
+
+	builder, err := createBuilder(k8sClient, smiAccessClient, smiSpecClient, smiSplitClient)
+	require.NoError(t, err)
+
+	ignoredResources := mk8s.NewIgnored()
+	got, err := builder.Build(ignoredResources)
+	require.NoError(t, err)
+
+	assertTopology(t, "testdata/topology-traffic-target-service-port-mismatch.json", got)
+}
+
 // TestTopologyBuilder_BuildTrafficTargetMultipleSourcesAndDestinations makes sure we can build a topology with
 // a TrafficTarget defined with multiple sources.
 func TestTopologyBuilder_BuildTrafficTargetMultipleSourcesAndDestinations(t *testing.T) {

pkg/topology/testdata/topology-traffic-target-service-port-mismatch.json

@@ -0,0 +1,130 @@
+{
+  "services": {
+    "svc-b1@my-ns": {
+      "name": "svc-b1",
+      "namespace": "my-ns",
+      "selector": {
+        "app": "app-b1"
+      },
+      "annotations": {},
+      "ports": [
+        {
+          "name": "port-8080",
+          "protocol": "TCP",
+          "port": 8080,
+          "targetPort": 8080
+        }
+      ],
+      "clusterIp": "10.10.1.16",
+      "pods": [
+        "app-b1@my-ns"
+      ]
+    },
+    "svc-b2@my-ns": {
+      "name": "svc-b2",
+      "namespace": "my-ns",
+      "selector": {
+        "app": "app-b2"
+      },
+      "annotations": {},
+      "ports": [
+        {
+          "name": "port-80",
+          "protocol": "TCP",
+          "port": 80,
+          "targetPort": 80
+        }
+      ],
+      "clusterIp": "10.10.1.17",
+      "pods": [
+        "app-b2@my-ns"
+      ],
+      "trafficTargets": [
+        "svc-b2@my-ns:tt@my-ns"
+      ]
+    }
+  },
+  "pods": {
+    "app-a@my-ns": {
+      "name": "app-a",
+      "namespace": "my-ns",
+      "serviceAccount": "service-account-a",
+      "ip": "10.10.1.1",
+      "sourceOf": [
+        "svc-b2@my-ns:tt@my-ns"
+      ]
+    },
+    "app-b1@my-ns": {
+      "name": "app-b1",
+      "namespace": "my-ns",
+      "serviceAccount": "service-account-b",
+      "ip": "10.10.1.2"
+    },
+    "app-b2@my-ns": {
+      "name": "app-b2",
+      "namespace": "my-ns",
+      "serviceAccount": "service-account-b",
+      "ip": "10.10.1.3",
+      "destinationOf": [
+        "svc-b2@my-ns:tt@my-ns"
+      ]
+    }
+  },
+  "serviceTrafficTargets": {
+    "svc-b1@my-ns:tt@my-ns": {
+      "service": "svc-b1@my-ns",
+      "name": "tt",
+      "namespace": "my-ns",
+      "sources": [
+        {
+          "serviceAccount": "service-account-a",
+          "namespace": "my-ns",
+          "pods": [
+            "app-a@my-ns"
+          ]
+        }
+      ],
+      "destination": {
+        "serviceAccount": "service-account-b",
+        "namespace": "my-ns",
+        "ports": [],
+        "pods": [
+          "app-b1@my-ns"
+        ]
+      },
+      "errors": [
+        "unable to find destination ports on Service \"svc-b1@my-ns\": destination port 80 of TrafficTarget \"tt@my-ns\" is not exposed by the service"
+      ]
+    },
+    "svc-b2@my-ns:tt@my-ns": {
+      "service": "svc-b2@my-ns",
+      "name": "tt",
+      "namespace": "my-ns",
+      "sources": [
+        {
+          "serviceAccount": "service-account-a",
+          "namespace": "my-ns",
+          "pods": [
+            "app-a@my-ns"
+          ]
+        }
+      ],
+      "destination": {
+        "serviceAccount": "service-account-b",
+        "namespace": "my-ns",
+        "ports": [
+          {
+            "name": "port-80",
+            "protocol": "TCP",
+            "port": 80,
+            "targetPort": 80
+          }
+        ],
+        "pods": [
+          "app-b2@my-ns"
+        ]
+      }
+    }
+  },
+  "trafficSplits": {}
+}