diff --git a/pkg/router/template/plugin.go b/pkg/router/template/plugin.go index 756de92ec7f4..c0aa4c9d2fcc 100644 --- a/pkg/router/template/plugin.go +++ b/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 } diff --git a/pkg/router/template/plugin_test.go b/pkg/router/template/plugin_test.go index 34999854c701..289ae8a10df3 100644 --- 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 index 95fda5771148..bcb8c329cd0a 100644 --- a/pkg/router/template/template_helper.go +++ b/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 } diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go index 9d0931676930..b041adb925fc 100644 --- a/pkg/router/template/template_helper_test.go +++ b/pkg/router/template/template_helper_test.go @@ -1,6 +1,7 @@ package templaterouter import ( + "reflect" "regexp" "testing" ) @@ -297,3 +298,168 @@ func TestMatchPattern(t *testing.T) { } } } + +func TestEndpointsForAlias(t *testing.T) { + testEndpoints := []struct { + name string + preferPort string + eps []Endpoint + expected []Endpoint + }{ + { + name: "Test no preference with a single endpoint", + preferPort: "", + eps: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + }, + }, + expected: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + NoHealthCheck: true, + }, + }, + }, + { + name: "Test no preference with multiple", + preferPort: "", + eps: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + }, + { + ID: "ep1", + Port: "443", + PortName: "https", + }, + { + ID: "ep2", + Port: "80", + PortName: "http", + }, + { + ID: "ep3", + Port: "80", + PortName: "http", + }, + }, + expected: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + }, + { + ID: "ep1", + Port: "443", + PortName: "https", + }, + { + ID: "ep2", + Port: "80", + PortName: "http", + }, + { + ID: "ep3", + Port: "80", + PortName: "http", + }, + }, + }, + { + name: "Test a preference resulting in multiple", + preferPort: "http", + eps: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + }, + { + ID: "ep1", + Port: "443", + PortName: "https", + }, + { + ID: "ep2", + Port: "80", + PortName: "http", + }, + { + ID: "ep3", + Port: "80", + PortName: "http", + }, + }, + expected: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + }, + { + ID: "ep2", + Port: "80", + PortName: "http", + }, + { + ID: "ep3", + Port: "80", + PortName: "http", + }, + }, + }, + { + name: "Test a preference resulting in one", + preferPort: "https", + eps: []Endpoint{ + { + ID: "ep1", + Port: "80", + PortName: "http", + }, + { + ID: "ep1", + Port: "443", + PortName: "https", + }, + { + ID: "ep2", + Port: "80", + PortName: "http", + }, + { + ID: "ep3", + Port: "80", + PortName: "http", + }, + }, + expected: []Endpoint{ + { + ID: "ep1", + Port: "443", + PortName: "https", + NoHealthCheck: true, + }, + }, + }, + } + + for _, te := range testEndpoints { + eps := endpointsForAlias( + ServiceAliasConfig{PreferPort: te.preferPort}, + ServiceUnit{EndpointTable: te.eps}, + ) + + if !reflect.DeepEqual(te.expected, eps) { + t.Errorf("%s: expected result: %#v, got: %#v", te.name, te.expected, eps) + } + } +}