Add fqdnCacheMinTTL configuration parameter #6808

hkiiita · 2024-11-13T17:29:50Z

This PR introduces a fqdnCacheMinTTL setting which would help address the problem of applications caching DNS response IPs indefinitely. Cluster administrators should be able to configure this value, ideally setting it to be equal to or greater than the maximum TTL value of the application's DNS cache.

This feature is a work towards resolving the issue of indefinite caching of DNS response IPs by certain applications.

Signed-off-by: Hemant <hkbiet@gmail.com>

pkg/config/agent/config.go

build/yamls/antrea.yml

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-11-14T18:45:21Z

cmd/antrea-agent/options.go

+	// If minTTL is greater than 1, it indicates that the value has been set externally by a cluster admin, and should be respected.
+	if o.config.MinTTL > 1 {


why 1 and not 0?

antoninbas · 2024-11-14T18:51:18Z

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

+# The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
+# The Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
+# value of the application's DNS cache.


I would remove "indefinitely". If the application does indeed cache the DNS entry forever, there is not much we can do.
We should also mention that this is for FQDN policy enforcement.
So maybe something like this:

The minTTL setting helps address the problem of applications caching DNS response IPs beyond the TTL value for the DNS record.
It is used to enforce FQDN policy rules, ensuring that resolved IPs are included in datapath rules for as long as the application is caching them.
This value should ideally be set to the maximum caching duration across all applications.

antoninbas · 2024-11-14T18:52:03Z

cmd/antrea-agent/options.go

@@ -90,6 +90,7 @@ type Options struct {
 	nplEndPort             int
 	dnsServerOverride      string
 	nodeType               config.NodeType
+	minTTL                 uint32


this field doesn't seem necessary?

antoninbas · 2024-11-14T18:53:37Z

pkg/agent/controller/networkpolicy/fqdn.go

+	getMaxTTL := func(ttl1, ttl2 uint32) uint32 {
+		if ttl1 > ttl2 {
+			return ttl1
+		} else {
+			return ttl2
+		}
+	}


there is a max built in in Golang: https://pkg.go.dev/builtin#max
It was introduced in Go 1.21

antoninbas · 2024-11-14T18:59:02Z

pkg/agent/controller/networkpolicy/fqdn.go

 	currentTime := f.clock.Now()
 	for _, ans := range msg.Answer {
 		switch r := ans.(type) {
 		case *dns.A:
 			if f.ipv4Enabled {
 				responseIPs[r.A.String()] = ipWithExpiration{
 					ip:             r.A,
-					expirationTime: currentTime.Add(time.Duration(r.Header().Ttl) * time.Second),
+					expirationTime: currentTime.Add(time.Duration(getMaxTTL(f.minTTL, r.Header().Ttl)) * time.Second),


I feel like technically, minTTL should only apply to DNS responses which were not initiated by Antrea, but intercepted by Antrea (responses to DNS queries generated by the application). However, this code applies to responses to DNS queries sent by Antrea (when an override DNS server is configured). Maybe we need to introduce a flag to distinguish between the 2 cases?

cc @tnqn @Dyanngg

We actually spent the length of last syncup meeting to discuss this, because I had the same suggestion but @tnqn’s opinion was that we don’t have to differentiate between the two cases. For security purpose we advise users to use FQDN rules only in allowlists for Antrea-native policies, and he thinks it’s okay that the clients’ TTL for a FQDN goes “out of sync” with the antrea agent since that’s not the gaurentee we want: we only want to enforce that client cannot access unintended addresses. So having an address for a domain which has longer TTL in antrea cache compared to the client is ok. I’ll let Quan chime in to see if I’m summarizing this correctly, but the end result of the discussion was we told Hemant to not worry about differentiating Antrea and client initiated dns queries

For security purpose we advise users to use FQDN rules only in allowlists for Antrea-native policies

Got it, makes sense to me

@hkiiita please ignore this comment

- Remove the redundant minTTL field from Options struct. - Use in built max function for comparison of max TTL values. - Improve descriptive comment about minTTL in config file. Signed-off-by: Hemant <hkbiet@gmail.com>

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-11-15T23:57:58Z

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

+# The minTTL setting helps address the problem of applications caching DNS response IPs beyond the TTL value for the DNS record.
+# It is used to enforce FQDN policy rules, ensuring that resolved IPs are included in datapath rules for as long as the application is caching them.
+# This value should ideally be set to the maximum caching duration across all applications.
+minTTL: {{ .Values.minTTL }}


the name is a bit generic, maybe we should go with fqdnCacheMinTTL?

antoninbas · 2024-11-16T00:00:45Z

cmd/antrea-agent/options.go

+	// Ensure that the minTTL is not negative.
+	o.config.MinTTL = max(o.config.MinTTL, 0)


it would be better to return an error if o.config.MinTTL < 0 IMO

while this code is in validateK8sNodeOptions, I am not sure this configuration parameter is specific to the "K8sNode" case. Maybe the check should be in the parent function? cc @tnqn

I think it can be used in ExternalNode too but guess no one validates it. I'm fine with keeping it in the parent function for now. We might create a validate function for networkpolicy related configurations later.

antoninbas · 2024-11-16T00:01:41Z

pkg/agent/controller/networkpolicy/fqdn.go

@@ -160,7 +161,7 @@ type fqdnController struct {
 	clock clock.Clock
 }

-func newFQDNController(client openflow.Client, allocator *idAllocator, dnsServerOverride string, dirtyRuleHandler func(string), v4Enabled, v6Enabled bool, gwPort uint32, clock clock.WithTicker) (*fqdnController, error) {
+func newFQDNController(client openflow.Client, allocator *idAllocator, dnsServerOverride string, dirtyRuleHandler func(string), v4Enabled, v6Enabled bool, gwPort uint32, clock clock.WithTicker, minTTL int) (*fqdnController, error) {


I wonder if minTTL should be uint32 here. This way we know it is a positive number. The conversion from int to uint32 can happen in cmd/antrea-agent.

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-11-18T19:04:29Z

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

 # It is used to enforce FQDN policy rules, ensuring that resolved IPs are included in datapath rules for as long as the application is caching them.
 # This value should ideally be set to the maximum caching duration across all applications.
-minTTL: {{ .Values.minTTL }}
+fqdnCacheMinTTL: {{ .Values.minTTL }}


Suggested change

fqdnCacheMinTTL: {{ .Values.minTTL }}

fqdnCacheMinTTL: {{ .Values.fqdnCacheMinTTL }}

This is why the manifests are not generated correctly (fqdnCacheMinTTL: instead of fqdnCacheMinTTL: 0)

Sorry , my bad. Will correct that.

antoninbas · 2024-11-18T19:05:08Z

cmd/antrea-agent/options.go

+	switch o.config.NodeType {
+	case config.ExternalNode.String():
 		o.nodeType = config.ExternalNode
 		return o.validateExternalNodeOptions()
-	} else if o.config.NodeType == config.K8sNode.String() {
+	case config.K8sNode.String():
 		o.nodeType = config.K8sNode
 		return o.validateK8sNodeOptions()
-	} else {
+	default:
 		return fmt.Errorf("unsupported nodeType %s", o.config.NodeType)


this is not a bad change, but I would avoid doing it in this PR as it is unrelated

antoninbas · 2024-11-18T19:06:25Z

cmd/antrea-agent/options.go

-	if o.config.NodeType == config.ExternalNode.String() {
+	// validate FqdnCacheMinTTL
+	if o.config.FqdnCacheMinTTL < 0 {
+		return fmt.Errorf("fqdnCacheMinTTL set to an invalid value, its must be a positive integer")


Suggested change

return fmt.Errorf("fqdnCacheMinTTL set to an invalid value, its must be a positive integer")

return fmt.Errorf("fqdnCacheMinTTL must be greater than or equal to 0")

antoninbas · 2024-11-18T19:07:13Z

pkg/config/agent/config.go

@@ -158,7 +158,7 @@ type AgentConfig struct {
 	// The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
 	// The Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
 	// value of the application's DNS cache.
-	MinTTL int `yaml:"minTTL,omitempty"`
+	FqdnCacheMinTTL int `yaml:"fqdnCacheMinTTL,omitempty"`


I think the field name here should be FQDNCacheMinTTL per our conventions

comment not addressed

Dyanngg

I think we should also add unit test cases to verify that with fqdnController with minTTL set will set correct dnsEntry expiration times in cache

… per review. Signed-off-by: Hemant <hkbiet@gmail.com>

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-11-21T19:27:24Z

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

@@ -306,6 +306,11 @@ kubeAPIServerOverride: {{ .Values.kubeAPIServerOverride | quote }}
 # 10.96.0.10:53, [fd00:10:96::a]:53).
 dnsServerOverride: {{ .Values.dnsServerOverride | quote }}

+# The fqdnCacheMinTTL setting helps address the problem of applications caching DNS response IPs beyond the TTL value for the DNS record.


s/The fqdnCacheMinTTL setting helps address/fqdnCacheMinTTL helps address

@hkiiita not addressed correctly, the current sentence is not grammatically correct

antoninbas · 2024-11-21T19:43:22Z

pkg/agent/controller/networkpolicy/fqdn_test.go

@@ -744,3 +747,140 @@ func TestOnDNSResponse(t *testing.T) {
 		})
 	}
 }
+func TestFQDNCacheMinTTL(t *testing.T) {


I feel like in theory we only need to test parseDNSResponse, because this is the only place where minTTL is actually used. We just need to make sure that minTTL can override the TTL included in the DNS response. cc @Dyanngg

However, I don't feel super strongly about it, so if others think it is better to test onDNSResponseMsg "end-to-end", it is fine by me.

I am awaiting a comment on this one to implement the nit change asked above in previous comment , moreover, i had also pushed the commit below refactor to test just the parseDNSResponse considering the feedback.

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-11-26T00:11:13Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			name:            "Response TTL less than FQDNCacheTTL",
+			expectedTTL:     currentTime.Add(10 * time.Second),
+			fqdnCacheMinTTL: 10,
+			dnsMsg: &dns.Msg{


There is a lot of repetition: we use the same DNS message every time as far as I can tell (with the only change being the ttl value).
One option is to use a closure such as this one:

getDNSMsg := func(ttl in) *dns.Msg { return &dns.Msg{ ... } }

pkg/agent/controller/networkpolicy/fqdn_test.go

antoninbas · 2024-11-26T00:12:46Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			fakeClock := newFakeClock(currentTime)
+			controller := gomock.NewController(t)
+			f, _ := newMockFQDNController(t, controller, nil, fakeClock, tc.fqdnCacheMinTTL)
+			require.Zero(t, fakeClock.TimersAdded())


is this useful in the context of this test? Please add a short comment if you think it is necessary.

antoninbas · 2024-11-26T00:12:58Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			f, _ := newMockFQDNController(t, controller, nil, fakeClock, tc.fqdnCacheMinTTL)
+			require.Zero(t, fakeClock.TimersAdded())
+			_, responseIPs, err := f.parseDNSResponse(tc.dnsMsg)
+			assert.NoError(t, err)


this one should be require.NoError

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas

~~just a couple of nits, otherwise LGTM~~

Edit: the e2e test is not correct, it doesn't actually set "minTTL"

please resolve conflicts as well

antoninbas · 2024-11-26T19:17:22Z

build/charts/antrea/values.yaml

@@ -186,6 +186,10 @@ kubeAPIServerOverride: ""
 # -- Address of DNS server, to override the kube-dns Service. It's used to
 # resolve hostnames in a FQDN policy.
 dnsServerOverride: ""
+# -- The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.


comment out of date

antoninbas · 2024-11-26T19:30:45Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+		ttl             uint32
+	}{
+		{
+			name:            "Response TTL less than FQDNCacheTTL",


rename all occurrences of FQDNCacheTTL to fqdnCacheMinTTL

antoninbas · 2024-11-26T19:32:47Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+	}{
+		{
+			name:            "Response TTL less than FQDNCacheTTL",
+			expectedTTL:     currentTime.Add(10 * time.Second),


to improve readability of the test case definitions, it would be nice if expectedTTL followed the same "format" as fqdnCacheMinTTL and ttl

I would suggest using a plain integer for expectedTTL (like for the other 2 fields), and making the necessary conversion in the test implementation (t.Run)

antoninbas · 2024-11-26T19:33:03Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+		name            string
+		expectedTTL     time.Time
+		fqdnCacheMinTTL uint32
+		ttl             uint32


nit: rename ttl to responseTTL

test/e2e/antreapolicy_test.go

- Added mocking of fqdnCacheMinTTl set and unset in the e2e test. - Refactored fqdn test by enclosing the DNS message construction in a closure and - setting the TTL value in DNS message in t.Run() - updated outdated comment in values.yaml for the fqdnCacheMinTTL. Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-11-27T18:26:09Z

build/yamls/antrea.yml

@@ -5383,8 +5388,8 @@ spec:
        # Using "kubectl logs/exec/attach/cp" doesn't have to specify "-c antrea-agent" when troubleshooting.
        kubectl.kubernetes.io/default-container: antrea-agent
        # Automatically restart Pods with a RollingUpdate if the ConfigMap changes
-        # See https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deployments
-        checksum/config: ee43fc8c8c5ac8097a757da0545e43b25f98d8f2d831842f7ee76e1ed7581267
+        # See https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deploymentstl


I don't think this is an intentional change?

not addressed

antoninbas · 2024-11-27T18:26:56Z

pkg/agent/controller/networkpolicy/fqdn_test.go

+			dnsMsg := getDNSMsg(tc.responseTTL)
+			_, responseIPs, err := f.parseDNSResponse(dnsMsg)
+			require.NoError(t, err)
+			expectedTTL := currentTime.Add(tc.expectedTTL * time.Second)


nit: s/expectedTTL/expectedExpirationTime for this variable name

antoninbas · 2024-11-27T18:28:53Z

test/e2e/antreapolicy_test.go

+	testWithFQDNCacheMinTTL(t, 0)
+	testWithFQDNCacheMinTTL(t, 10)


make them independent subtests with their own name. For example:

t.Run("FQDNCacheMinTTL unset", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 0) }) t.Run("FQDNCacheMinTTL set to 10s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 10) })

antoninbas · 2024-11-27T18:29:20Z

test/e2e/antreapolicy_test.go

+	if fqdnCacheMinTTL == 0 {
+		t.Logf("Running the test with FQDNCacheMinTTL unset")
+	} else {
+		t.Logf("Running the test with FQDNCacheMinTTL set to %d ", fqdnCacheMinTTL)
+	}


Instead of this, each case should be a subtest, with its own name (see comment above)
After making that change, you can remove these logs

antoninbas · 2024-11-27T18:30:59Z

test/e2e/antreapolicy_test.go

@@ -5342,6 +5359,7 @@ func TestFQDNCacheMinTTL(t *testing.T) {
 		}
 		return stdout, nil
 	}
+	setFQDNCacheMinTTLInAntrea(t, data, fqdnCacheMinTTL)


it's better if you only mutate the Antrea config once. So you need to find a way to "merge" this with setDNSServerAddressInAntrea

additionally, the config should be restored at the end of the test

antoninbas · 2024-11-27T18:31:40Z

test/e2e/antreapolicy_test.go

 	assert.EventuallyWithT(t, func(t *assert.CollectT) {
 		_, err := curlFQDN(fqdnIP)
 		assert.Error(t, err)
-	}, 10*time.Second, 1*time.Second)
+	}, 20*time.Second, 1*time.Second)


the assertion should be different based on whether fqdnCacheMinTTL is set or not? otherwise there is something wrong with the test.

antoninbas · 2024-11-27T18:31:48Z

test/e2e/antreapolicy_test.go

 	// The wait time here should be slightly longer than the reload value specified in the custom DNS configuration.
-	// TODO: This assertion currently verifies the issue described in https://github.com/antrea-io/antrea/issues/6229.
-	// It will need to be updated once minTTL support is implemented.
+	// TODO: This assertion verifies the fix to the issue described in https://github.com/antrea-io/antrea/issues/6229.


this is no longer a TODO

not addressed

tnqn · 2024-11-28T14:35:16Z

cmd/antrea-agent/options.go

@@ -155,6 +155,11 @@ func (o *Options) validate(args []string) error {
 		return fmt.Errorf("nodeType %s requires feature gate ExternalNode to be enabled", o.config.NodeType)
 	}

+	// validate FqdnCacheMinTTL


no need to have the comment as the code is obvious and the comment doesn't provide more information

tnqn · 2024-11-28T14:42:23Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

@@ -196,7 +196,7 @@ func NewNetworkPolicyController(antreaClientGetter client.AntreaClientProvider,
 	gwPort, tunPort uint32,
 	nodeConfig *config.NodeConfig,
 	podNetworkWait *utilwait.Group,
-	l7Reconciler *l7engine.Reconciler) (*Controller, error) {
+	l7Reconciler *l7engine.Reconciler, fqdnCacheMinTTL uint32) (*Controller, error) {


Use a separate line for the new argument like other args.

tnqn · 2024-11-28T14:43:10Z

pkg/config/agent/config.go

@@ -158,7 +158,7 @@ type AgentConfig struct {
 	// The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
 	// The Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
 	// value of the application's DNS cache.
-	MinTTL int `yaml:"minTTL,omitempty"`
+	FqdnCacheMinTTL int `yaml:"fqdnCacheMinTTL,omitempty"`


comment not addressed

tnqn · 2024-11-28T14:45:24Z

test/e2e/antreapolicy_test.go

+// applications might cache them, thereby preventing intermittent network connectivity issues to the FQDN concerned.
+// Actual test logic runs in testWithFQDNCacheMinTTL, which gets called by  TestFQDNCacheMinTTL with 2 fqdnCacheMinTTL values


These two lines are obviously longer than other lines.

And remove the extra space between "by TestFQDNCacheMinTTL"

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-12-02T21:28:37Z

build/yamls/antrea.yml

@@ -5383,8 +5388,8 @@ spec:
        # Using "kubectl logs/exec/attach/cp" doesn't have to specify "-c antrea-agent" when troubleshooting.
        kubectl.kubernetes.io/default-container: antrea-agent
        # Automatically restart Pods with a RollingUpdate if the ConfigMap changes
-        # See https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deployments
-        checksum/config: ee43fc8c8c5ac8097a757da0545e43b25f98d8f2d831842f7ee76e1ed7581267
+        # See https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deploymentstl


not addressed

antoninbas · 2024-12-02T21:28:57Z

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

+# fqdnCacheMinTTL helps address the issue of applications caching DNS response IPs beyond the TTL value for the DNS record.
+# It is used to enforce FQDN policy rules, ensuring that resolved IPs are included in datapath rules for as long as the application caches them.
+# Ideally, this value should be set to the maximum caching duration across all applications.
+fqdnCacheMinTTL: {{ .Values.fqdnCacheMinTTL }}
+


it would be good to wrap the lines here as well

antoninbas · 2024-12-02T21:29:02Z

build/charts/antrea/values.yaml

+# fqdnCacheMinTTL helps address the issue of applications caching DNS response IPs beyond the TTL value for the DNS record.
+# It is used to enforce FQDN policy rules, ensuring that resolved IPs are included in datapath rules for as long as the application caches them.
+# Ideally, this value should be set to the maximum caching duration across all applications.


antoninbas · 2024-12-02T21:30:28Z

test/e2e/antreapolicy_test.go

+	t.Run("FQDNCacheMinTTL-unset", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 0) })
+	t.Run("FQDNCacheMinTTL-set-to-10s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 10) })


Why are you adding dashes to the sub-test names?

Note that the go testing framework takes care of escaping whitespaces automatically.

@antoninbas Actually , i was getting the following error , hence i had done the said change .

fixtures.go:288: Creating 'testfqdncacheminttl-fqdncacheminttl_unset-c2njzgdw' K8s Namespace fixtures.go:355: Exporting test logs to '/tmp/antrea-test-2503355158/TestFQDNCacheMinTTL_FQDNCacheMinTTL_unset/afterSetupTest.Dec04-00-40-40' antreapolicy_test.go:5301: Error when setting up test: error when creating 'testfqdncacheminttl-fqdncacheminttl_unset-c2njzgdw' Namespace: Namespace "testfqdncacheminttl-fqdncacheminttl_unset-c2njzgdw" is invalid: metadata.name: Invalid value: "testfqdncacheminttl-fqdncacheminttl_unset-c2njzgdw": a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')

After you pointed this out, Initially i thought maybe i should look at the test framework code to check if we are doing this replacement of whitespace with underscores but i found its not being done there as the setupTest function gets the name from *testing.T type, hence its being modified by go.

resolved as per guidance in slack.

antoninbas · 2024-12-02T21:31:17Z

test/e2e/antreapolicy_test.go

 	// The wait time here should be slightly longer than the reload value specified in the custom DNS configuration.
-	// TODO: This assertion currently verifies the issue described in https://github.com/antrea-io/antrea/issues/6229.
-	// It will need to be updated once minTTL support is implemented.
+	// TODO: This assertion verifies the fix to the issue described in https://github.com/antrea-io/antrea/issues/6229.


not addressed

antoninbas · 2024-12-02T21:33:50Z

test/e2e/antreapolicy_test.go

@@ -5322,8 +5334,8 @@ func TestFQDNCacheMinTTL(t *testing.T) {
 	createCustomDNSPod(t, data, configMap.Name)

 	// set the custom DNS server IP address in Antrea ConfigMap.
-	setDNSServerAddressInAntrea(t, data, dnsServiceIP)
-	defer setDNSServerAddressInAntrea(t, data, "") //reset after the test.
+	setDNSServerAddressInAntrea(t, data, dnsServiceIP, fqdnCacheMinTTL)


rename setDNSServerAddressInAntrea to configureFQDNPolicyEnforcement, which is more generic and more suitable since you are not configuring 2 different things

antoninbas · 2024-12-02T21:34:51Z

test/e2e/antreapolicy_test.go

+		assert.EventuallyWithT(t, func(t *assert.CollectT) {
+			_, err := curlFQDN(fqdnIP)
+			assert.NoError(t, err)
+		}, time.Duration(fqdnCacheMinTTL)*time.Second, 1*time.Second)


it feels like this should be assert.Never. I don't understand how assert.Eventually is valid.

- Fixed final e2e assertion by using assert.Never - Fixed various formatting issues as pointe di review - Renamed the function which modifies DNS ConfigMap Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-12-05T18:57:20Z

test/e2e/antreapolicy_test.go

+		assert.Never(t, func() bool {
+			_, err := curlFQDN(fqdnIP)
+			return err != nil
+		}, time.Duration(fqdnCacheMinTTL)*time.Second, 1*time.Second)


It feels like the test is likely to be "flaky" in practice. The assertion will verify that the condition is not met for fqdnCacheMinTTL seconds. However, by the time we start running the assertion, Antrea may have cached the IP for a few seconds already? So assuming that fqdnCacheMinTTL is 10s (as is the case currently), could the assertion actually fail after 7 or 8s?

Unless I am missing something, we should probably have some logic like this:

// get timestamp before the Pod resolves the FQDN for the first time startCacheTime := time.Now() // this is the first time we make the Pod curl the FQDN assert.EventuallyWithT(t, func(t *assert.CollectT) { _, err := curlFQDN(testFQDN) assert.NoError(t, err) }, 2*time.Second, 100*time.Millisecond, "failed to curl test FQDN: ", testFQDN) // rest of the test code // when we get to the "Never" assertion, we need to deduct some time from fqdnCacheMinTTL // the extra 1s is for "safety" waitFor := time.Duration(fqdnCacheMinTTL)*time.Second - time.Since(startCacheTime) - 1*time.Second assert.Never(t, func() bool { _, err := curlFQDN(fqdnIP) return err != nil }, waitFor, 1*time.Second)

It may make sense to increase the minTTL value from 10s to 20s for the test IMO. I am not exactly sure how much time elapses between startCacheTime and this assertion being executed.

antoninbas · 2024-12-05T18:58:31Z

test/e2e/antreapolicy_test.go

@@ -5368,26 +5380,34 @@ func TestFQDNCacheMinTTL(t *testing.T) {
 	require.NoError(t, data.setPodAnnotation(data.testNamespace, "custom-dns-server", "test.antrea.io/random-value",


not introduced in this PR, but I just noticed the following assertion above:

assert.EventuallyWithT(t, func(t *assert.CollectT) { _, err := curlFQDN(testFQDN) assert.NoError(t, err) }, 2*time.Second, 1*time.Millisecond, "failed to curl test FQDN: ", testFQDN)

1ms is much too frequent to be executing the condition function. Please update to 100ms.

antoninbas · 2024-12-05T18:58:52Z

test/e2e/antreapolicy_test.go

@@ -5368,26 +5380,34 @@ func TestFQDNCacheMinTTL(t *testing.T) {
 	require.NoError(t, data.setPodAnnotation(data.testNamespace, "custom-dns-server", "test.antrea.io/random-value",
 		randSeq(8)), "failed to update custom DNS Pod annotation.")

-	// finally verify that Curling the previously cached IP fails after DNS update.
+	// finally verify that Curling the previously cached IP does not fail after DNS update.


Suggested change

// finally verify that Curling the previously cached IP does not fail after DNS update.

// finally verify that Curling the previously cached IP does not fail after DNS update, as long as fqdnCacheMinTTL is set.

- added a waitFor time equivalent to the difference of time elapsed since last DNS request. - updated manifests using `make manifests` for build fail. - updated helm chart docs. Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas · 2024-12-10T02:56:49Z

test/e2e/antreapolicy_test.go

+	// Calculate `waitFor` to determine the duration to wait for the 'Never' assertion.
+	// This accounts for the elapsed time since the initial DNS request was made from the Pod
+	// and the start of the FQDN cache's minimum TTL (fqdnCacheMinTTL). The duration is reduced
+	// by 1 second as a buffer acting as a safety margin.
+	safetyMargin := 1 * time.Second
+	waitFor := (time.Duration(fqdnCacheMinTTL)*time.Second - time.Since(startCacheTime)) - safetyMargin
+
+	if waitFor <= 0 {
+		t.Logf("Invalid waitFor computed: %v. Clamping to 1 second.", waitFor)
+		waitFor = 1 * time.Second
+	}


this should be moved to the else block of the code below. We only need that for the case where fqdnCacheMinTTL is greater than 0.
I would also remove the sanity checking code for waitFor and replace it with require.GreaterOrEqual(t, waitFor, 5*time.Second). I think the test is only meaningful if waitFor is large enough. If this requirement fails, it either means that there is an issue with the test or that fqdnCacheMinTTL should be set to a larger value.

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas

LGTM. Only a handful of comments to address.
@tnqn @Dyanngg do you want to take another look?

antoninbas · 2024-12-10T18:53:08Z

test/e2e/antreapolicy_test.go

 func TestFQDNCacheMinTTL(t *testing.T) {
+	t.Run("minTTLUnset", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 0) })
+	t.Run("minTTL10s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 10) })


Suggested change

t.Run("minTTL10s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 10) })

t.Run("minTTL20s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 20) })

In #6808 (comment), this is what I was actually recommending

antoninbas · 2024-12-10T18:54:02Z

test/e2e/antreapolicy_test.go

+		assert.EventuallyWithT(t, func(t *assert.CollectT) {
+			_, err := curlFQDN(fqdnIP)
+			assert.Error(t, err)
+		}, 20*time.Second, 1*time.Second)


you can keep the timeout as 10s here. I think there was some confusion with my request in #6808 (comment)

Yes, got it.

antoninbas · 2024-12-10T18:54:34Z

test/e2e/antreapolicy_test.go

+		waitFor := (time.Duration(fqdnCacheMinTTL)*time.Second - time.Since(startCacheTime)) - safetyMargin
+		require.GreaterOrEqual(t, waitFor, 5*time.Second)
+
+		// fqdnCacheMinTTL is set hence we expect no error at least till the period equivalent to fqdnCacheMinTTL's value.


Suggested change

// fqdnCacheMinTTL is set hence we expect no error at least till the period equivalent to fqdnCacheMinTTL's value.

// fqdnCacheMinTTL is set hence we expect no error at least until fqdnCacheMinTTL expires.

antoninbas · 2024-12-10T19:38:34Z

pkg/agent/controller/networkpolicy/fqdn_test.go

@@ -350,7 +353,7 @@ func TestDeleteFQDNRule(t *testing.T) {
 func TestLookupIPFallback(t *testing.T) {


@hkiiita this test is failing for your PR. The test itself can probably be improved, but right now it is failing because you are enabling IPv6 unconditionally in newMockFQDNController, and this test cannot handle IPv6.
My recommendation for this PR would be to revert the change in newMockFQDNController so that IPv6 stays disabled by default. Then in your new test function (TestParseDNSResponseOnFQDNCacheMinTTL), which requires IPv6, you manually enabled it like this:

f, _ := newMockFQDNController(t, controller, nil, fakeClock, tc.fqdnCacheMinTTL) f.ipv6Enabled = true // this test needs IPv6 enabled

I think this is good enough for now.

Yes, did the changes.

The integration test seems to be failing , am not sure but it seems to be failing at

gobgp_test.go:108: Getting peers of BGP server1 and verifying them while verifying peers for BGP Server 1 , i checked the test and though i cant find it but i am wondering if its related to above change in any way ?

You can ignore the integration test failure here. It is unrelated to your change. The test is known to be flaky and we already have an issue to track it: #6501

Dyanngg

LGTM overall once Antonin's comments are addressed

Dyanngg · 2024-12-10T21:34:13Z

cmd/antrea-agent/options.go

@@ -155,6 +155,10 @@ func (o *Options) validate(args []string) error {
 		return fmt.Errorf("nodeType %s requires feature gate ExternalNode to be enabled", o.config.NodeType)
 	}

+	if o.config.FQDNCacheMinTTL < 0 {


TODO: not necessarily need to be part of this PR, but I just want to note @salv-orlando's comment in the community meeting: we should consider setting a sensible max value for this field, so that Antrea will not just cache each FQDN ip forever

I was thinking on it (i am inexperienced in a real world scenario for this though) and thought that IPs mapped to FQDN's wont probably update/change at a rate that it might make us ending up with too many datapath rules for each IP ? and maybe can this be left to the cluster admin to determine and set it ? What if the admin wishes to place a large value , for his own use case, which surpasses the 'max' that we set ?

I don't know about a max value for FQDNCacheMinTTL, but we should have a upper bound on the size of the cache (which would also be a upper bound on the number of datapath rules). This is independent of this feature IMO, because I could also have a DNS server returning records with a very high TTL, and it seems that the issue would be the same.

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas

LGTM, thanks for all the work

antoninbas · 2024-12-11T21:19:00Z

/test-all

initial commit for implementing minTTL

346988f

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 13, 2024

View reviewed changes

pkg/config/agent/config.go Outdated Show resolved Hide resolved

build/yamls/antrea.yml Show resolved Hide resolved

auto generated antrea yml files

66e72f1

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 14, 2024

View reviewed changes

hkiiita added 3 commits November 15, 2024 22:08

Refactored minTTL confgiyration and use.

d92246f

- Remove the redundant minTTL field from Options struct. - Use in built max function for comparison of max TTL values. - Improve descriptive comment about minTTL in config file. Signed-off-by: Hemant <hkbiet@gmail.com>

Merge branch 'antrea-io:main' into fqdn-cache-minttl

2c58ad2

update final assertion and comment for the test.

e9e0286

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 16, 2024

View reviewed changes

move minTTL validation to parent function and rename to fqdnCacheMinTTL

dd43d6b

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 18, 2024

View reviewed changes

Dyanngg reviewed Nov 18, 2024

View reviewed changes

hkiiita added 2 commits November 21, 2024 23:05

add tests for FQDNCacheMinTTL functionality and made other changes as…

7de5ee7

… per review. Signed-off-by: Hemant <hkbiet@gmail.com>

re-grouped imports

fb28bba

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 21, 2024

View reviewed changes

refactor to test just the parseDNSResponse

6c6dcc8

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 26, 2024

View reviewed changes

update test to include IPv6 and doc strng change in config

b0548d5

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Nov 26, 2024

View reviewed changes

test/e2e/antreapolicy_test.go Show resolved Hide resolved

hkiiita marked this pull request as ready for review November 27, 2024 15:47

Merge branch 'main' into fqdn-cache-minttl

0d86321

antoninbas reviewed Nov 27, 2024

View reviewed changes

tnqn reviewed Nov 28, 2024

View reviewed changes

refactor e2e test into 2 sub-tests and other small changes

2b05275

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Dec 2, 2024

View reviewed changes

hkiiita added 2 commits December 5, 2024 22:01

Fix assertion and formatting issues

2231173

- Fixed final e2e assertion by using assert.Never - Fixed various formatting issues as pointe di review - Renamed the function which modifies DNS ConfigMap Signed-off-by: Hemant <hkbiet@gmail.com>

Merge branch 'main' into fqdn-cache-minttl

5b801c5

antoninbas reviewed Dec 5, 2024

View reviewed changes

Refactor e2e test to include a waitFor time

3287148

- added a waitFor time equivalent to the difference of time elapsed since last DNS request. - updated manifests using `make manifests` for build fail. - updated helm chart docs. Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Dec 10, 2024

View reviewed changes

moved waitFor statements to place where FQDNCacheMinTTL is set

b1cdb6f

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas reviewed Dec 10, 2024

View reviewed changes

Dyanngg reviewed Dec 10, 2024

View reviewed changes

Disable IPv6 in mock fqdnController as default and other nit changes

e80d7bd

Signed-off-by: Hemant <hkbiet@gmail.com>

antoninbas approved these changes Dec 11, 2024

View reviewed changes

antoninbas changed the title ~~Implementation for minTTL~~ Add fqdnCacheMinTTL configuration parameter Dec 11, 2024

antoninbas added the area/network-policy Issues or PRs related to network policies. label Dec 11, 2024

antoninbas merged commit e404ccc into antrea-io:main Dec 11, 2024
58 of 60 checks passed

antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Dec 11, 2024

		// If minTTL is greater than 1, it indicates that the value has been set externally by a cluster admin, and should be respected.
		if o.config.MinTTL > 1 {

		// Ensure that the minTTL is not negative.
		o.config.MinTTL = max(o.config.MinTTL, 0)

	fqdnCacheMinTTL: {{ .Values.minTTL }}
	fqdnCacheMinTTL: {{ .Values.fqdnCacheMinTTL }}

	return fmt.Errorf("fqdnCacheMinTTL set to an invalid value, its must be a positive integer")
	return fmt.Errorf("fqdnCacheMinTTL must be greater than or equal to 0")

		// applications might cache them, thereby preventing intermittent network connectivity issues to the FQDN concerned.
		// Actual test logic runs in testWithFQDNCacheMinTTL, which gets called by TestFQDNCacheMinTTL with 2 fqdnCacheMinTTL values

		t.Run("FQDNCacheMinTTL-unset", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 0) })
		t.Run("FQDNCacheMinTTL-set-to-10s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 10) })

		@@ -5368,26 +5380,34 @@ func TestFQDNCacheMinTTL(t *testing.T) {
		require.NoError(t, data.setPodAnnotation(data.testNamespace, "custom-dns-server", "test.antrea.io/random-value",

	// finally verify that Curling the previously cached IP does not fail after DNS update.
	// finally verify that Curling the previously cached IP does not fail after DNS update, as long as fqdnCacheMinTTL is set.

	t.Run("minTTL10s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 10) })
	t.Run("minTTL20s", func(t *testing.T) { testWithFQDNCacheMinTTL(t, 20) })

	// fqdnCacheMinTTL is set hence we expect no error at least till the period equivalent to fqdnCacheMinTTL's value.
	// fqdnCacheMinTTL is set hence we expect no error at least until fqdnCacheMinTTL expires.

		@@ -350,7 +353,7 @@ func TestDeleteFQDNRule(t *testing.T) {
		func TestLookupIPFallback(t *testing.T) {

Add fqdnCacheMinTTL configuration parameter #6808

Add fqdnCacheMinTTL configuration parameter #6808

Conversation

hkiiita commented Nov 13, 2024 • edited by antoninbas 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

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

Dyanngg left a 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 • 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

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

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

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

Dyanngg left a 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

antoninbas commented Dec 11, 2024

hkiiita commented Nov 13, 2024 •

edited by antoninbas

Loading

antoninbas left a comment •

edited

Loading