Merge pull request #17077 from knobunc/fix/health-supression

Automatic merge from submit-queue.

Fix the "supress health checks when only one backing service" logic

This reverts commit 4833eb5 and then fixes the implementation.

Previously we had attempted to suppress the health checks, but it was looking at the endpoints of the service, not the servers that back the route.  The problem is that if we just look at endpoints, then if a route has two backing services, each with one endpoint, we would incorrectly suppress the health checks.

Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)

Author: openshift-merge-robot

pkg/router/template/plugin.go

@@ -265,7 +265,6 @@ func createRouterEndpoints(endpoints *kapi.Endpoints, excludeUDP bool, lookupSvc
 		wasIdled = true
 	}
 
-	// Take a reasonable guess at the number of endpoints to avoid reallocating the out array when we append
 	out := make([]Endpoint, 0, len(endpoints.Subsets)*4)
 
 	// Now build the actual endpoints we pass to the template
@@ -307,11 +306,5 @@ func createRouterEndpoints(endpoints *kapi.Endpoints, excludeUDP bool, lookupSvc
 		}
 	}
 
-	// We want to disable endpoint checks if there is only one endpoint
-	// We skip the case where it is idled, since we set NoHealthCheck above
-	if wasIdled == false && len(out) == 1 {
-		out[0].NoHealthCheck = true
-	}
-
 	return out
 }

pkg/router/template/plugin_test.go

@@ -275,16 +275,15 @@ func TestHandleEndpoints(t *testing.T) {
 				Name: "foo/test", //service name from kapi.endpoints object
 				EndpointTable: []Endpoint{
 					{
-						ID:            "ept:test:1.1.1.1:345",
-						IP:            "1.1.1.1",
-						Port:          "345",
-						NoHealthCheck: true,
+						ID:   "ept:test:1.1.1.1:345",
+						IP:   "1.1.1.1",
+						Port: "345",
 					},
 				},
 			},
 		},
 		{
-			name:      "Endpoint mod (one ep, one address)",
+			name:      "Endpoint mod",
 			eventType: watch.Modified,
 			endpoints: &kapi.Endpoints{
 				ObjectMeta: metav1.ObjectMeta{
@@ -300,143 +299,9 @@ func TestHandleEndpoints(t *testing.T) {
 				Name: "foo/test",
 				EndpointTable: []Endpoint{
 					{
-						ID:            "pod:pod-1:test:2.2.2.2:8080",
-						IP:            "2.2.2.2",
-						Port:          "8080",
-						NoHealthCheck: true,
-					},
-				},
-			},
-		},
-		{
-			name:      "Endpoint mod (second ep, one address each)",
-			eventType: watch.Modified,
-			endpoints: &kapi.Endpoints{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "foo",
-					Name:      "test",
-				},
-				Subsets: []kapi.EndpointSubset{
-					{
-						Addresses: []kapi.EndpointAddress{{IP: "2.2.2.2", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-1"}}},
-						Ports:     []kapi.EndpointPort{{Port: 8080}},
-					},
-					{
-						Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-2"}}},
-						Ports:     []kapi.EndpointPort{{Port: 8081}},
-					},
-				},
-			},
-			expectedServiceUnit: &ServiceUnit{
-				Name: "foo/test",
-				EndpointTable: []Endpoint{
-					{
-						ID:            "pod:pod-1:test:2.2.2.2:8080",
-						IP:            "2.2.2.2",
-						Port:          "8080",
-						NoHealthCheck: false,
-					},
-					{
-						ID:            "pod:pod-2:test:3.3.3.3:8081",
-						IP:            "3.3.3.3",
-						Port:          "8081",
-						NoHealthCheck: false,
-					},
-				},
-			},
-		},
-		{
-			name:      "Endpoint mod (one ep, two addresses)",
-			eventType: watch.Modified,
-			endpoints: &kapi.Endpoints{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "foo",
-					Name:      "test",
-				},
-				Subsets: []kapi.EndpointSubset{
-					{
-						Addresses: []kapi.EndpointAddress{
-							{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-2"}},
-							{IP: "4.4.4.4", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-3"}},
-						},
-						Ports: []kapi.EndpointPort{{Port: 8080}},
-					},
-				},
-			},
-			expectedServiceUnit: &ServiceUnit{
-				Name: "foo/test",
-				EndpointTable: []Endpoint{
-					{
-						ID:            "pod:pod-2:test:3.3.3.3:8080",
-						IP:            "3.3.3.3",
-						Port:          "8080",
-						NoHealthCheck: false,
-					},
-					{
-						ID:            "pod:pod-3:test:4.4.4.4:8080",
-						IP:            "4.4.4.4",
-						Port:          "8080",
-						NoHealthCheck: false,
-					},
-				},
-			},
-		},
-		{
-			name:      "Endpoint mod (one ep, two ports)",
-			eventType: watch.Modified,
-			endpoints: &kapi.Endpoints{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "foo",
-					Name:      "test",
-				},
-				Subsets: []kapi.EndpointSubset{
-					{
-						Addresses: []kapi.EndpointAddress{
-							{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-2"}},
-						},
-						Ports: []kapi.EndpointPort{{Port: 8080}, {Port: 8081}},
-					},
-				},
-			},
-			expectedServiceUnit: &ServiceUnit{
-				Name: "foo/test",
-				EndpointTable: []Endpoint{
-					{
-						ID:            "pod:pod-2:test:3.3.3.3:8080",
-						IP:            "3.3.3.3",
-						Port:          "8080",
-						NoHealthCheck: false,
-					},
-					{
-						ID:            "pod:pod-2:test:3.3.3.3:8081",
-						IP:            "3.3.3.3",
-						Port:          "8081",
-						NoHealthCheck: false,
-					},
-				},
-			},
-		},
-		{
-			name:      "Endpoint mod (back to one ep)",
-			eventType: watch.Modified,
-			endpoints: &kapi.Endpoints{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "foo",
-					Name:      "test",
-				},
-				Subsets: []kapi.EndpointSubset{{
-					Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-1"}}},
-					Ports:     []kapi.EndpointPort{{Port: 8080}},
-				}},
-			},
-			expectedServiceUnit: &ServiceUnit{
-				Name: "foo/test",
-				EndpointTable: []Endpoint{
-					{
-						ID:            "pod:pod-1:test:3.3.3.3:8080",
-						IP:            "3.3.3.3",
-						Port:          "8080",
-						NoHealthCheck: true,
+						ID:   "pod:pod-1:test:2.2.2.2:8080",
+						IP:   "2.2.2.2",
+						Port: "8080",
 					},
 				},
 			},
@@ -450,8 +315,8 @@ func TestHandleEndpoints(t *testing.T) {
 					Name:      "test",
 				},
 				Subsets: []kapi.EndpointSubset{{
-					Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-1"}}},
-					Ports:     []kapi.EndpointPort{{Port: 8080}},
+					Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3"}},
+					Ports:     []kapi.EndpointPort{{Port: 0}},
 				}},
 			},
 			expectedServiceUnit: &ServiceUnit{

pkg/router/template/template_helper.go

@@ -157,16 +157,28 @@ func processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action
 }
 
 func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
-	if len(alias.PreferPort) == 0 {
+	if len(alias.PreferPort) == 0 && len(svc.EndpointTable) != 1 {
+		// We can skip the work.  They are selecting everything, and since they don't have one endpoint,
+		// we can't disable the health checks.  So we can just use their list.
 		return svc.EndpointTable
 	}
+
 	endpoints := make([]Endpoint, 0, len(svc.EndpointTable))
-	for i := range svc.EndpointTable {
-		endpoint := svc.EndpointTable[i]
-		if endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
+	for _, endpoint := range svc.EndpointTable {
+		// Filter the list:
+		// - If PreferPort length is 0, match everything
+		// - Otherwise, if the PortName or Port matches PreferPort, then we have a match
+		if len(alias.PreferPort) == 0 || endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
 			endpoints = append(endpoints, endpoint)
 		}
 	}
+
+	// We want to disable endpoint checks if there is only one endpoint since there's no point in
+	// testing and removing it from the set ahead of time since there are no other eps to replace it.
+	if len(endpoints) == 1 {
+		endpoints[0].NoHealthCheck = true
+	}
+
 	return endpoints
 }