From 84ad54f0e550bb059b3e48f85858b219eb51ae9d Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 6 Apr 2022 10:59:05 -0400 Subject: [PATCH] Don't increment "no local endpoints" metric when there are no remote endpoints A service having no _local_ endpoints when it does have remote endpoints is different from a service having no endpoints at all. --- pkg/proxy/iptables/proxier.go | 19 +++++++------------ pkg/proxy/iptables/proxier_test.go | 8 ++++++++ pkg/proxy/ipvs/proxier.go | 6 +++--- pkg/proxy/ipvs/proxier_test.go | 20 ++++++++++++++------ 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 5f8094ef605..00f14f50b32 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1354,24 +1354,19 @@ func (proxier *Proxier) syncProxyRules() { if len(localEndpoints) != 0 { // Write rules jumping from localPolicyChain to localEndpointChains proxier.writeServiceToEndpointRules(svcNameString, svcInfo, localPolicyChain, localEndpoints, args) - } else { + } else if hasEndpoints { if svcInfo.InternalPolicyLocal() && utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { serviceNoLocalEndpointsTotalInternal++ } if svcInfo.ExternalPolicyLocal() { serviceNoLocalEndpointsTotalExternal++ } - if hasEndpoints { - // Blackhole all traffic since there are no local endpoints - args = append(args[:0], - "-A", string(localPolicyChain), - "-m", "comment", "--comment", - fmt.Sprintf(`"%s has no local endpoints"`, svcNameString), - "-j", - string(KubeMarkDropChain), - ) - proxier.natRules.Write(args) - } + // Blackhole all traffic since there are no local endpoints + proxier.natRules.Write( + "-A", string(localPolicyChain), + "-m", "comment", "--comment", + fmt.Sprintf(`"%s has no local endpoints"`, svcNameString), + "-j", string(KubeMarkDropChain)) } } } diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index fc2feadd874..997d5ba42e7 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -7726,6 +7726,14 @@ func TestNoEndpointsMetric(t *testing.T) { expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 1, expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 1, }, + { + name: "both policies are set and there are no endpoints at all", + internalTrafficPolicy: &internalTrafficPolicyLocal, + externalTrafficPolicy: externalTrafficPolicyLocal, + endpoints: []endpoint{}, + expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 0, + expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 0, + }, } for _, tc := range testCases { diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 6dcfc2533cf..566a60bbc62 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1990,7 +1990,7 @@ func (proxier *Proxier) syncEndpoint(svcPortName proxy.ServicePortName, onlyNode if !ok { klog.InfoS("Unable to filter endpoints due to missing service info", "servicePortName", svcPortName) } else { - clusterEndpoints, localEndpoints, _, _ := proxy.CategorizeEndpoints(endpoints, svcInfo, proxier.nodeLabels) + clusterEndpoints, localEndpoints, _, hasAnyEndpoints := proxy.CategorizeEndpoints(endpoints, svcInfo, proxier.nodeLabels) if onlyNodeLocalEndpoints { if len(localEndpoints) > 0 { endpoints = localEndpoints @@ -2001,11 +2001,11 @@ func (proxier *Proxier) syncEndpoint(svcPortName proxy.ServicePortName, onlyNode // will have the POD address and will be discarded. endpoints = clusterEndpoints - if svcInfo.InternalPolicyLocal() && utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { + if hasAnyEndpoints && svcInfo.InternalPolicyLocal() && utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { proxier.serviceNoLocalEndpointsInternal.Insert(svcPortName.NamespacedName.String()) } - if svcInfo.ExternalPolicyLocal() { + if hasAnyEndpoints && svcInfo.ExternalPolicyLocal() { proxier.serviceNoLocalEndpointsExternal.Insert(svcPortName.NamespacedName.String()) } } diff --git a/pkg/proxy/ipvs/proxier_test.go b/pkg/proxy/ipvs/proxier_test.go index 2f6cbb2f05d..52ac3c670bb 100644 --- a/pkg/proxy/ipvs/proxier_test.go +++ b/pkg/proxy/ipvs/proxier_test.go @@ -5821,7 +5821,7 @@ func TestNoEndpointsMetric(t *testing.T) { expectedSyncProxyRulesNoLocalEndpointsTotalExternal int }{ { - name: "internalTrafficPolicy is set and there is non-zero local endpoints", + name: "internalTrafficPolicy is set and there are local endpoints", internalTrafficPolicy: &internalTrafficPolicyLocal, endpoints: []endpoint{ {"10.0.1.1", testHostname}, @@ -5830,7 +5830,7 @@ func TestNoEndpointsMetric(t *testing.T) { }, }, { - name: "externalTrafficPolicy is set and there is non-zero local endpoints", + name: "externalTrafficPolicy is set and there are local endpoints", externalTrafficPolicy: externalTrafficPolicyLocal, endpoints: []endpoint{ {"10.0.1.1", testHostname}, @@ -5839,7 +5839,7 @@ func TestNoEndpointsMetric(t *testing.T) { }, }, { - name: "both policies are set and there is non-zero local endpoints", + name: "both policies are set and there are local endpoints", internalTrafficPolicy: &internalTrafficPolicyLocal, externalTrafficPolicy: externalTrafficPolicyLocal, endpoints: []endpoint{ @@ -5849,7 +5849,7 @@ func TestNoEndpointsMetric(t *testing.T) { }, }, { - name: "internalTrafficPolicy is set and there is zero local endpoint", + name: "internalTrafficPolicy is set and there are no local endpoints", internalTrafficPolicy: &internalTrafficPolicyLocal, endpoints: []endpoint{ {"10.0.1.1", "host0"}, @@ -5859,7 +5859,7 @@ func TestNoEndpointsMetric(t *testing.T) { expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 1, }, { - name: "externalTrafficPolicy is set and there is zero local endpoint", + name: "externalTrafficPolicy is set and there are no local endpoints", externalTrafficPolicy: externalTrafficPolicyLocal, endpoints: []endpoint{ {"10.0.1.1", "host0"}, @@ -5869,7 +5869,7 @@ func TestNoEndpointsMetric(t *testing.T) { expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 1, }, { - name: "Both policies are set and there is zero local endpoint", + name: "Both policies are set and there are no local endpoints", internalTrafficPolicy: &internalTrafficPolicyLocal, externalTrafficPolicy: externalTrafficPolicyLocal, endpoints: []endpoint{ @@ -5880,6 +5880,14 @@ func TestNoEndpointsMetric(t *testing.T) { expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 1, expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 1, }, + { + name: "Both policies are set and there are no endpoints at all", + internalTrafficPolicy: &internalTrafficPolicyLocal, + externalTrafficPolicy: externalTrafficPolicyLocal, + endpoints: []endpoint{}, + expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 0, + expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 0, + }, } for _, tc := range testCases { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServiceInternalTrafficPolicy, true)()