Add skipServices to support NodeLocal DNSCache with AntreaProxy #2882

luolanzone · 2021-10-11T10:34:49Z

Add a skipServices in antrea-agent.conf so AntreaProxy can be configured to skip proxying kube-dns service which allow user to use NodeLocal DNSCache

Resolves #2137

Signed-off-by: Lan Luo [email protected]

luolanzone · 2021-10-11T10:38:02Z

@antoninbas @tnqn I didn't find an efficient way to check the cache hit inside a DNS pod, I used a prometheus locally to check the metric coredns_cache_hits_total from NodeLocal DNSCache, it looks fine from the metrics, Could you help to take a look? thanks.

codecov-commenter · 2021-10-11T10:40:02Z

Codecov Report

Merging #2882 (aeb8266) into main (95e836d) will decrease coverage by 20.23%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##             main    #2882       +/-   ##
===========================================
- Coverage   61.18%   40.94%   -20.24%     
===========================================
  Files         284      157      -127     
  Lines       23494    19586     -3908     
===========================================
- Hits        14374     8020     -6354     
- Misses       7565    10803     +3238     
+ Partials     1555      763      -792

Flag	Coverage Δ
kind-e2e-tests	`?`
unit-tests	`40.94% <50.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/proxy/proxier.go	`41.81% <0.00%> (-19.01%)`	⬇️
pkg/agent/proxy/service.go	`100.00% <100.00%> (ø)`
pkg/ovs/openflow/default.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/util/runtime/runtime.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/openflow/logs.go	`9.52% <0.00%> (-90.48%)`	⬇️
pkg/apis/controlplane/register.go	`0.00% <0.00%> (-90.00%)`	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	`0.00% <0.00%> (-83.88%)`	⬇️
pkg/agent/agent_linux.go	`0.00% <0.00%> (-80.00%)`	⬇️
pkg/agent/client.go	`0.00% <0.00%> (-77.42%)`	⬇️
... and 232 more

antoninbas · 2021-10-11T19:02:41Z

build/yamls/base/conf/antrea-agent.conf

@@ -190,3 +190,6 @@ antreaProxy:
  # (e.g. 1.2.3.0/24, 1.2.3.4/32). An empty string slice is meant to select all host IPv4/IPv6 addresses.
  # Note that the option is only valid when proxyAll is true.
  #nodePortAddresses: []
+  # A string array of values which specifies the service list should skip proxying. Values can be a valid ClusterIP (e.g. 10.11.1.2)


line is a bit long

antoninbas · 2021-10-11T19:04:49Z

cmd/antrea-agent/agent.go

+	if o.config.AntreaProxy.SkipServices != nil {
+		util.SkipServices = o.config.AntreaProxy.SkipServices
+	}


Setting a global variable in a separate package like this is not good practice IMO. Please add a method on the proxier to configure this, or pass the list during proxier initialization.

done via pass the list through proxier initialization

antoninbas · 2021-10-11T19:06:26Z

third_party/proxy/util/utils.go

@@ -77,6 +81,21 @@ func ShouldSkipService(service *v1.Service) bool {
 		klog.V(3).Infof("Skipping service %s in namespace %s due to Type=ExternalName", service.Name, service.Namespace)
 		return true
 	}
+
+	if matchSkipServices(service.Namespace+"/"+service.Name) || matchSkipServices(service.Spec.ClusterIP) {
+		klog.Infof("Skipping service %s in namespace %s due to it matches SkipService List ", service.Name, service.Namespace)


use structured logging

klog.InfoS("Skipping Service because it matches skipServices list ", "service", klog.KObj(svc)

antoninbas · 2021-10-12T23:15:12Z

build/yamls/antrea.yml

@@ -3953,6 +3953,9 @@ data:
      # (e.g. 1.2.3.0/24, 1.2.3.4/32). An empty string slice is meant to select all host IPv4/IPv6 addresses.
      # Note that the option is only valid when proxyAll is true.
      #nodePortAddresses: []
+      # A string array of values which specifies the service list should skip proxying. Values can be a valid


An array of string values to specify a list of Services which should be ignored by AntreaProxy (traffic to these Services will not be load-balanced).

antoninbas · 2021-10-12T23:16:20Z

cmd/antrea-agent/agent.go

@@ -203,14 +203,15 @@ func run(o *Options) error {
 		v4Enabled := config.IsIPv4Enabled(nodeConfig, networkConfig.TrafficEncapMode)
 		v6Enabled := config.IsIPv6Enabled(nodeConfig, networkConfig.TrafficEncapMode)
 		proxyAll := o.config.AntreaProxy.ProxyAll
+		skipServices := o.config.AntreaProxy.SkipServices


should there be a check in options.go with a warning if AntreaProxy is disabled by len(skipServices) > 0?

antoninbas · 2021-10-12T23:24:47Z

pkg/agent/proxy/proxier_test.go

+		makeTestService(svc1PortName.Namespace, svc1PortName.Name, func(svc *corev1.Service) {
+			svc.Spec.ClusterIP = "10.96.10.12"
+			svc.Spec.Ports = []corev1.ServicePort{{
+				Name:     svc1PortName.Port,
+				Port:     int32(svc1Port),
+				Protocol: corev1.ProtocolTCP,
+			}}
+		}),
+		makeTestService(svc2PortName.Namespace, svc2PortName.Name, func(svc *corev1.Service) {
+			svc.Spec.ClusterIP = "192.168.1.2"
+			svc.Spec.Ports = []corev1.ServicePort{{
+				Name:     svc2PortName.Port,
+				Port:     int32(svc2Port),
+				Protocol: corev1.ProtocolTCP,
+			}}
+		}),


maybe this should be a separate test case because with the current code, it's not clear what all what the test is actually verifying. I assume that the point of the test is that even though we add these Services, because they are skipped, we don't expect any new call to InstallServiceFlows?

But these Services don't seem to have any Endpoints, and IIRC we skip such Services completely in installServices. So not sure what we are testing here. At the very least there should be an explanatory comment.

yes, it expects with no new call to InstallServiceFlows, let me check if a separate case works or a comment.

done, added a new test case TestClusterSkipServices

antoninbas · 2021-10-13T19:59:28Z

cmd/antrea-agent/options.go

@@ -232,6 +232,10 @@ func (o *Options) setDefaults() {
 }

 func (o *Options) validateAntreaProxyConfig() error {
+	if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) && len(o.config.AntreaProxy.SkipServices) > 0 {
+		klog.Warningf("skipServices will be ignored due to AntreaProxy is disabled")


Unfortunately (?) there is no klog.WarningS and the recommendation is to use klog.InfoS (kubernetes/klog#184)

there is klog.Warning, but I'd like to update it as klog.Warningf("skipServices %v will be ignored due to AntreaProxy is disabled",o.config.AntreaProxy.SkipServices)

@luolanzone my point was that we are transitioning to structured logging everywhere, so we should not use klog.Warning / klog.Warningf at all. We should use klog.InfoS instead.

ah, Ok, I am not aware of this kind of transition, I have changed it to use klog.InfoS now.

antoninbas · 2021-10-13T19:59:59Z

pkg/agent/proxy/proxier_test.go

-		}),
-	)
+
+	fullServices := append(extraSvcs, makeTestService(svcPortName.Namespace, svcPortName.Name, func(svc *corev1.Service) {


s/fullServices/allServices

pkg/agent/proxy/proxier_test.go

antoninbas

LGTM, except for the use of klog.Warning and one comment improvement

antoninbas · 2021-10-14T20:27:54Z

third_party/proxy/service.go

-
-	recorder record.EventRecorder
+	recorder                record.EventRecorder
+	// skipServices indicates the service list which should skip proxying


Suggested change

// skipServices indicates the service list which should skip proxying

// skipServices indicates the service list for which we should skip proxying

tnqn

I think this title doesn't reflect what the patch does properly, which will confuse code reader. Instead of supporting NodeLocal DNSCache with AntreaProxy, it just provides a way that can support NodeLocal DNSCache and user still needs to configure Antrea to achieve it.

tnqn · 2021-10-18T14:46:04Z

cmd/antrea-agent/options.go

@@ -232,6 +232,10 @@ func (o *Options) setDefaults() {
 }

 func (o *Options) validateAntreaProxyConfig() error {
+	if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) && len(o.config.AntreaProxy.SkipServices) > 0 {
+		klog.InfoS("skipServices will be ignored due to AntreaProxy is disabled", "skipServices", o.config.AntreaProxy.SkipServices)


Is "due to AntreaProxy is disabled" correct grammatically? I think it should be "due to AntreaProxy being disabled" or "when AntreaProxy is disabled" @antoninbas

yes you're correct. Either solution works, or even "skipServices will be ignored because AntreaProxy is disabled"

done, change it to skipServices will be ignored because AntreaProxy is disabled and updated PR's summary and description

tnqn · 2021-10-18T14:57:53Z

third_party/proxy/util/utils.go

+		return false
+	}
+	if matchSkipServices(service.Namespace+"/"+service.Name, skipServices) || matchSkipServices(service.Spec.ClusterIP, skipServices) {
+		klog.InfoS("Skipping service because it matches skipServices list ", "service", klog.KObj(service))


Suggested change

klog.InfoS("Skipping service because it matches skipServices list ", "service", klog.KObj(service))

klog.InfoS("Skipping service because it matches skipServices list", "service", klog.KObj(service))

tnqn · 2021-10-18T15:01:48Z

third_party/proxy/service.go

+	recorder                record.EventRecorder
+	// skipServices indicates the service list for which we should skip proxying
+	// it will be initialized from antrea-agent.conf
+	skipServices []string


It should be worthy to use a set to keep the services which provides O(1) time for query, otherwise having N skipServices means the code needs to compare 2*N times for each service.

I didn't point this out, because my assumption was that the list of services (provided in the antrea-agent configuration) would always be small (< 10 or so), in which case a slice works fine and may even be faster than a map / set. However, if we think someone may want to use this with more services, then we should definitely use a set. That's the least risky option since we don't have too much control over how people may use this option :)

tnqn

One nit-picking comment. And you should update the commit message as well to reflect the real change

tnqn · 2021-10-19T04:35:14Z

third_party/proxy/service.go

 	return &ServiceChangeTracker{
 		items:                   make(map[types.NamespacedName]*serviceChange),
 		makeServiceInfo:         makeServiceInfo,
 		recorder:                recorder,
 		ipFamily:                ipFamily,
 		processServiceMapChange: processServiceMapChange,
+		skipServices:            skipSvcSet,


Suggested change

skipServices: skipSvcSet,

skipServices: sets.NewString(skipServices...),

done and updated the commit msg.

Add a skipServices in antrea-agent.conf so AntreaProxy can be configured to skip proxying kube-dns service which allow user to use NodeLocal DNSCache Resolves antrea-io#2137 Signed-off-by: Lan Luo <[email protected]>

tnqn

LGTM

tnqn · 2021-10-19T08:52:31Z

/test-all

luolanzone requested review from antoninbas and tnqn October 11, 2021 10:34

luolanzone force-pushed the local-dns-cache branch from b8e2be4 to 1f9b648 Compare October 11, 2021 10:40

antoninbas reviewed Oct 11, 2021

View reviewed changes

luolanzone force-pushed the local-dns-cache branch from 1f9b648 to c1eaea1 Compare October 12, 2021 02:32

antoninbas reviewed Oct 12, 2021

View reviewed changes

luolanzone force-pushed the local-dns-cache branch 2 times, most recently from 144aa05 to 9263f5a Compare October 13, 2021 04:01

antoninbas reviewed Oct 13, 2021

View reviewed changes

luolanzone force-pushed the local-dns-cache branch from 9263f5a to 3f92aee Compare October 14, 2021 02:27

antoninbas reviewed Oct 14, 2021

View reviewed changes

luolanzone force-pushed the local-dns-cache branch from 3f92aee to fee8df3 Compare October 15, 2021 13:38

tnqn reviewed Oct 18, 2021

View reviewed changes

antoninbas added this to the Antrea v1.4 release milestone Oct 18, 2021

luolanzone changed the title ~~Support NodeLocal DNSCache with AntreaProxy~~ Add skipServices to support NodeLocal DNSCache with AntreaProxy Oct 19, 2021

luolanzone force-pushed the local-dns-cache branch 2 times, most recently from 4cae243 to ff855d3 Compare October 19, 2021 02:44

tnqn reviewed Oct 19, 2021

View reviewed changes

luolanzone force-pushed the local-dns-cache branch from ff855d3 to 0960c0f Compare October 19, 2021 06:26

Add skipServices to support NodeLocal DNSCache with AntreaProxy

aeb8266

Add a skipServices in antrea-agent.conf so AntreaProxy can be configured to skip proxying kube-dns service which allow user to use NodeLocal DNSCache Resolves antrea-io#2137 Signed-off-by: Lan Luo <[email protected]>

luolanzone force-pushed the local-dns-cache branch from 0960c0f to aeb8266 Compare October 19, 2021 06:28

tnqn approved these changes Oct 19, 2021

View reviewed changes

tnqn merged commit 9e7f20b into antrea-io:main Oct 19, 2021

	// skipServices indicates the service list which should skip proxying
	// skipServices indicates the service list for which we should skip proxying

	klog.InfoS("Skipping service because it matches skipServices list ", "service", klog.KObj(service))
	klog.InfoS("Skipping service because it matches skipServices list", "service", klog.KObj(service))

	skipServices: skipSvcSet,
	skipServices: sets.NewString(skipServices...),

Add skipServices to support NodeLocal DNSCache with AntreaProxy #2882

Add skipServices to support NodeLocal DNSCache with AntreaProxy #2882

Conversation

luolanzone commented Oct 11, 2021 • edited Loading

luolanzone commented Oct 11, 2021

codecov-commenter commented Oct 11, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Oct 19, 2021

luolanzone commented Oct 11, 2021 •

edited

Loading

codecov-commenter commented Oct 11, 2021 •

edited

Loading

luolanzone Oct 19, 2021 •

edited

Loading

luolanzone Oct 19, 2021 •

edited

Loading