Fix FQDN policy support for IPv6 #3869

tnqn · 2022-06-08T07:59:01Z

IPv6 address must be wrapped with "[]" when used in network API.

This patch ensures that the auto-discovered and the user-provided DNS
servers use correct format. It also adds the missing configuration
"dnsServerOverride" to the configuration file.

Signed-off-by: Quan Tian [email protected]

Fixes #3873

tnqn · 2022-06-08T08:00:09Z

/test-ipv6-only-e2e
/test-ipv6-e2e
/test-all

tnqn · 2022-06-08T10:52:06Z

/test-ipv6-only-e2e
/test-ipv6-e2e
/test-all

tnqn · 2022-06-08T11:42:00Z

/test-ipv6-only-e2e
/test-ipv6-e2e
/test-e2e

jianjuns

In the commit description:

dns server -> DNS servers

jianjuns · 2022-06-08T16:34:03Z

build/charts/antrea/conf/antrea-agent.conf

@@ -207,6 +207,11 @@ nodePortLocal:
 # Defaults to "". It must be a host string, a host:port pair, or a URL to the base of the apiserver.
 kubeAPIServerOverride: {{ .Values.kubeAPIServerOverride | quote }}

+# Provide the address of DNS server, to override the kube-dns service. It's used to resolve hostname in FQDN policy.
+# Defaults to "". It must be a host string or a host:port pair of the dns server (e.g. 10.96.0.10, 10.96.0.10:53,


Dyanngg · 2022-06-08T17:26:16Z

test/e2e/antreapolicy_test.go

 	builder := &ClusterNetworkPolicySpecBuilder{}
 	builder = builder.SetName("test-acnp-fqdn-cluster-svc").
 		SetTier("application").
 		SetPriority(1.0)
 	for idx, service := range services {
-		builder.AddFQDNRule(svcDNSName(service), ProtocolTCP, nil, nil, nil, fmt.Sprintf("r%d", idx*2), []ACNPAppliedToSpec{{NSSelector: map[string]string{"ns": namespaces["y"]}, PodSelector: map[string]string{"pod": "b"}}}, crdv1alpha1.RuleActionReject)
-		builder.AddFQDNRule(svcDNSName(service), ProtocolTCP, nil, nil, nil, fmt.Sprintf("r%d", idx*2+1), []ACNPAppliedToSpec{{NSSelector: map[string]string{"ns": namespaces["z"]}, PodSelector: map[string]string{"pod": "c"}}}, crdv1alpha1.RuleActionDrop)
+		builder.AddFQDNRule(svcFQDN(service), ProtocolTCP, nil, nil, nil, fmt.Sprintf("r%d", idx*2), []ACNPAppliedToSpec{{NSSelector: map[string]string{"ns": namespaces["y"]}, PodSelector: map[string]string{"pod": "b"}}}, crdv1alpha1.RuleActionReject)


Based on your comment above, should we use svcDNSName here?

We expect the string set in the rule to be FQDN, not short names, right? otherwise it should be written with wildcard. The dns go library doesn't append the domains in the search list like what the system resolver does, using short name here would get no resolution result.

I see what you mean now. If that's the case why do we need the svcDNSName function then?

It's used in podToAddrTestStep as the server address. As the function comment explains, if we use FQDN as the server address, the client would first try FQDN.<namespace>.svc.cluster.local, then FQDN.svc.cluster.local, then FQDN.cluster.local, last FQDN, likely exceeding the timeout.

Got it. Thanks for the explanation. Maybe worth adding to the comment in the podToAddrTestStep below that each client Pod's /etc/resolve.conf will append svc.cluster.local as first dns resolution trial, just for laymans like me to understand this better when we look back at the code.

used another way to avoid unnecessary resolution attemps, and add comment to explain it, PTAL

codecov-commenter · 2022-06-08T17:30:27Z

Codecov Report

Merging #3869 (822caa0) into main (4e60600) will decrease coverage by 10.19%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #3869       +/-   ##
===========================================
- Coverage   64.42%   54.22%   -10.20%     
===========================================
  Files         291      407      +116     
  Lines       42745    58804    +16059     
===========================================
+ Hits        27539    31888     +4349     
- Misses      12990    24282    +11292     
- Partials     2216     2634      +418

Flag	Coverage Δ
integration-tests	`37.44% <ø> (?)`
kind-e2e-tests	`43.85% <9.09%> (-7.00%)`	⬇️
unit-tests	`44.77% <90.90%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/fqdn.go	`39.51% <100.00%> (-36.00%)`	⬇️
pkg/util/ip/ip.go	`78.86% <100.00%> (+0.10%)`	⬆️
...g/agent/apiserver/handlers/featuregates/handler.go	`9.09% <0.00%> (-72.73%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`40.00% <0.00%> (-60.00%)`	⬇️
pkg/controller/egress/store/egressgroup.go	`1.72% <0.00%> (-54.32%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`22.17% <0.00%> (-50.44%)`	⬇️
pkg/support/dump.go	`7.90% <0.00%> (-49.16%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-44.74%)`	⬇️
pkg/agent/types/networkpolicy.go	`37.83% <0.00%> (-43.25%)`	⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go	`49.36% <0.00%> (-43.04%)`	⬇️
... and 171 more

tnqn · 2022-06-09T12:03:40Z

/test-ipv6-only-e2e
/test-ipv6-e2e
/test-all

tnqn · 2022-06-09T15:13:12Z

/test-ipv6-only-e2e
/test-ipv6-e2e
/test-all

Dyanngg

LGTM

IPv6 address must be wrapped with "[]" when used in network API. This patch ensures that the auto-discovered and the user-provided DNS servers use correct format. It also adds the missing configuration "dnsServerOverride" to the configuration file. Signed-off-by: Quan Tian <[email protected]>

tnqn · 2022-06-10T12:58:58Z

/test-ipv6-only-e2e
/test-ipv6-e2e
/test-all

tnqn · 2022-06-13T07:31:35Z

/test-ipv6-only-e2e
/test-ipv6-e2e

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. area/network-policy Issues or PRs related to network policies. labels Jun 8, 2022

tnqn force-pushed the fix-fqdn-ipv6 branch 2 times, most recently from 329caa1 to 47207e5 Compare June 8, 2022 10:50

tnqn force-pushed the fix-fqdn-ipv6 branch from 47207e5 to de6aaf5 Compare June 8, 2022 11:41

tnqn mentioned this pull request Jun 8, 2022

ipv6 e2e tests keeps failing on TestAntreaPolicy #3842

Closed

tnqn requested review from GraysonWu, Dyanngg and jianjuns June 8, 2022 14:57

jianjuns reviewed Jun 8, 2022

View reviewed changes

tnqn force-pushed the fix-fqdn-ipv6 branch from de6aaf5 to ca34947 Compare June 8, 2022 16:48

jianjuns previously approved these changes Jun 8, 2022

View reviewed changes

Dyanngg reviewed Jun 8, 2022

View reviewed changes

tnqn mentioned this pull request Jun 9, 2022

Add trafficControlController to handle TrafficControl requests #3487

Merged

4 tasks

tnqn dismissed jianjuns’s stale review via ca1d8a5 June 9, 2022 15:12

tnqn force-pushed the fix-fqdn-ipv6 branch from ca34947 to ca1d8a5 Compare June 9, 2022 15:12

Dyanngg previously approved these changes Jun 9, 2022

View reviewed changes

tnqn dismissed Dyanngg’s stale review via 822caa0 June 10, 2022 12:57

tnqn force-pushed the fix-fqdn-ipv6 branch from ca1d8a5 to 822caa0 Compare June 10, 2022 12:57

jianjuns approved these changes Jun 10, 2022

View reviewed changes

tnqn merged commit 7817152 into antrea-io:main Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FQDN policy support for IPv6 #3869

Fix FQDN policy support for IPv6 #3869

tnqn commented Jun 8, 2022 •

edited

Loading

tnqn commented Jun 8, 2022

tnqn commented Jun 8, 2022

tnqn commented Jun 8, 2022

jianjuns left a comment

jianjuns Jun 8, 2022

tnqn Jun 8, 2022

Dyanngg Jun 8, 2022

tnqn Jun 9, 2022 •

edited

Loading

Dyanngg Jun 9, 2022

tnqn Jun 9, 2022

Dyanngg Jun 9, 2022

tnqn Jun 10, 2022

codecov-commenter commented Jun 8, 2022 •

edited

Loading

tnqn commented Jun 9, 2022

tnqn commented Jun 9, 2022

Dyanngg left a comment

tnqn commented Jun 10, 2022

tnqn commented Jun 13, 2022

Fix FQDN policy support for IPv6 #3869

Fix FQDN policy support for IPv6 #3869

Conversation

tnqn commented Jun 8, 2022 • edited Loading

tnqn commented Jun 8, 2022

tnqn commented Jun 8, 2022

tnqn commented Jun 8, 2022

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns Jun 8, 2022

Choose a reason for hiding this comment

tnqn Jun 8, 2022

Choose a reason for hiding this comment

Dyanngg Jun 8, 2022

Choose a reason for hiding this comment

tnqn Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Dyanngg Jun 9, 2022

Choose a reason for hiding this comment

tnqn Jun 9, 2022

Choose a reason for hiding this comment

Dyanngg Jun 9, 2022

Choose a reason for hiding this comment

tnqn Jun 10, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jun 8, 2022 • edited Loading

Codecov Report

tnqn commented Jun 9, 2022

tnqn commented Jun 9, 2022

Dyanngg left a comment

Choose a reason for hiding this comment

tnqn commented Jun 10, 2022

tnqn commented Jun 13, 2022

tnqn commented Jun 8, 2022 •

edited

Loading

tnqn Jun 9, 2022 •

edited

Loading

codecov-commenter commented Jun 8, 2022 •

edited

Loading