Revert "Made the router skip health checks when there is one endpoint"

This reverts commit 4833eb5. This logic is not correct. If a router is backed by two services, it will misbehave.
openshift · Oct 27, 2017 · e1704f5 · e1704f5
1 parent d3924ff
commit e1704f5
Show file tree

Hide file tree

Showing 4 changed files with 188 additions and 158 deletions.
diff --git a/pkg/router/template/plugin.go b/pkg/router/template/plugin.go
@@ -265,10 +265,9 @@ 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
+	// TODO: review me for sanity
 	for _, s := range subsets {
 		for _, p := range s.Ports {
 			if excludeUDP && p.Protocol == kapi.ProtocolUDP {
@@ -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
 }
diff --git a/pkg/router/template/plugin_test.go b/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{

diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go
@@ -157,16 +157,22 @@ func processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action
 }
 
 func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
-	if len(alias.PreferPort) == 0 {
-		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
 }