Add antctl get fqdncache #6868

Dhruv-J · 2024-12-16T21:37:28Z

This PR adds functionality for a new antctl command: antctl get fqdncache This command fetches the DNS mapping entries for FQDN policies by reading the cache for DNS entries and outputting FQDN name, associated IP, and expiration time. It can also be used with a --domain (-d) flag that can be specified to filter the result for only a certain FQDN name.

Dyanngg

Please remove the debug logs before pusing a PR, or mark this WIP/draft for now. Also, your change seems to break a ton of AgentQuerier interface implementations if you check the lint failures

This PR adds functionality for a new antctl command: antctl get fqdncache This command fetches the DNS mapping entries for FQDN policies by reading the cache for DNS entries and outputting FQDN name, associated IP, and expiration time. It can also be used with a --domain (-d) flag that can be specified to filter the result for only a certain FQDN name. Signed-off-by: Dhruv-J <[email protected]>

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J · 2025-01-06T21:33:54Z

/test-all

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J · 2025-01-07T06:25:12Z

/test-conformance
/test-networkpolicy
/test-e2e

Dyanngg · 2025-01-07T17:34:48Z

pkg/antctl/transform/fqdncache/response_test.go

+	"time"
+
+	"github.com/stretchr/testify/require"
+	"gotest.tools/assert"


use github.com/stretchr/testify/assert to avoid extra dependency added in go.mod

Dyanngg · 2025-01-07T17:35:09Z

hack/update-codegen.sh

@@ -34,6 +34,7 @@ IMAGE_NAME="antrea/codegen:kubernetes-1.31.1-build.1"
 # to the "safe" list in the Git config when starting the container (required
 # because of user mismatch).
 if git_status=$(git status --porcelain --untracked=no 2>/dev/null) && [[ -n "${git_status}" ]]; then
+  echo ${git_status}


unrelated change

Dyanngg · 2025-01-07T17:35:17Z

multicluster/hack/update-codegen.sh

@@ -34,6 +34,7 @@ IMAGE_NAME="antrea/codegen:kubernetes-1.31.1-build.1"
 # to the "safe" list in the Git config when starting the container (required
 # because of user mismatch).
 if git_status=$(git status --porcelain --untracked=no 2>/dev/null) && [[ -n "${git_status}" ]]; then
+  echo ${git_status}


Dyanngg · 2025-01-07T17:36:06Z

pkg/agent/apis/types.go

+	return false
+}
+
+// maybe some helper funcs needed here to help parse through ^^^^


Can you explain what this means

was a dev note, removing now, as functionality has been added

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

Dyanngg · 2025-01-07T17:49:37Z

pkg/agent/controller/networkpolicy/networkpolicy_controller_test.go

+	for _, dnsCacheEntryExp := range expectedEntryList {
+		found := false
+		for j, dnsCacheEntryRet := range returnedList {
+			if !found && dnsCacheEntryExp.FqdnName == dnsCacheEntryRet.FqdnName {


Did you try if assert.ElementsMatch works out of the box for comparing those lists?

elton-furtado · 2025-01-07T20:00:41Z

pkg/agent/apis/types.go

+	}
+}
+
+func (r FqdnCacheResponse) SortRows() bool {


why is the default option for sorting false?

the rows of the table won't need to be sorted, as there's no filter or flag for it, so the default is false

I don't remember how this function is used exactly, but it would be nice if the output rows were filtered based on the FQDN.

elton-furtado · 2025-01-07T20:13:15Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			assert.Equal(t, tt.expectedStatus, recorder.Code)
+			if tt.expectedStatus == http.StatusOK {
+				var received []map[string]interface{}
+				err = json.Unmarshal(recorder.Body.Bytes(), &received)


any tests to add to check for marshal and unmarshal operation correctly?

triggering this error manually would be difficult, as the json transformation is not something the test is directly in control of, it is also not tested on other PRs which add antctl functionality

elton-furtado · 2025-01-07T20:14:10Z

pkg/antctl/transform/fqdncache/response_test.go

+		t.Run(tt.name, func(t *testing.T) {
+			reqByte, _ := json.Marshal(tt.fqdnList)
+			reqReader := bytes.NewReader(reqByte)
+			result, err := Transform(reqReader, false, tt.opts)


any test to add specific transform function?

the transform function is tested as part of response.go, if it fails, then the test will fail as well

elton-furtado · 2025-01-07T20:14:39Z

pkg/agent/apis/types.go

+	expirationTime time.Time
+}
+
+func (r FqdnCacheResponse) GetTableHeader() []string {


can you check if we can add specific test cases to cover this in code cov

it seems to be left uncovered conventionally, as it is a simple method that primarily relies on the Transform() function to execute correctly, I don't think unit tests should be added for these funcs

elton-furtado · 2025-01-07T20:15:35Z

there's also kind e2e test noEncap mode also failing

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J · 2025-01-08T18:42:30Z

/test-conformance
/test-networkpolicy
/test-e2e

antoninbas · 2025-01-09T21:03:03Z

pkg/agent/apis/types.go

@@ -72,6 +74,28 @@ func (r AntreaAgentInfoResponse) SortRows() bool {
 	return true
 }

+type FqdnCacheResponse struct {


should be FQDNCacheResponse per our coding conventions
you can refer to Quan's document: https://github.com/tnqn/code-review-comments

antoninbas · 2025-01-09T21:06:19Z

pkg/agent/apis/types.go

+	}
+}
+
+func (r FqdnCacheResponse) SortRows() bool {


I don't remember how this function is used exactly, but it would be nice if the output rows were filtered based on the FQDN.

antoninbas · 2025-01-09T21:09:48Z

pkg/agent/apiserver/handlers/fqdncache/handler.go

+		if dnsEntryCache == nil {
+			return
+		}


note that your GetFqdnCache function will never return nil as far as I can tell. It's fine to keep the code as is, but maybe add a comment to this effect.

I think that was for the older function, when it would return nil instead of empty, but that's fixed now, so I can remove this bit of code.

antoninbas · 2025-01-09T21:16:11Z

pkg/agent/controller/networkpolicy/networkpolicy_controller_test.go

+		"*.example.com": {
+			responseIPs: map[string]ipWithExpiration{
+				"maps.example.com": {
+					ip:             net.ParseIP("10.0.0.1"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+				"mail.example.com": {
+					ip:             net.ParseIP("10.0.0.2"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+				"photos.example.com": {
+					ip:             net.ParseIP("10.0.0.3"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+			},
+		},
+		"antrea.io": {
+			responseIPs: map[string]ipWithExpiration{
+				"antrea.io": {
+					ip:             net.ParseIP("10.0.0.4"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+			},
+		},


the test input data doesn't match what we actually store in the cache
There is never a wildcard domain ("*.example.com") as the key in the dnsEntryCache map
For the nested map (map[string]ipWithExpiration) the key is actually the string representation of the IP (for historical reasons). See:

antrea/pkg/agent/controller/networkpolicy/fqdn.go

Lines 79 to 82 in 357d38f

// Key for responseIPs is the string representation of the IP.

// It helps to quickly identify IP address updates when a

// new DNS response is received.

responseIPs map[string]ipWithExpiration

So it's actually like this:

"antrea.io": { responseIPs: map[string]ipWithExpiration{ ""10.0.0.4"": { ip: net.ParseIP("10.0.0.4"), expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC), }, }, },

antoninbas · 2025-01-09T21:18:10Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			recorder := httptest.NewRecorder()
+			handler.ServeHTTP(recorder, req)
+			assert.Equal(t, tt.expectedStatus, recorder.Code)
+			if tt.expectedStatus == http.StatusOK {


I would just remove the if here, as the status code is always 200

Note that this unit test actually validates very little (which is expected), so in my opinion we didn't really need multiple test cases, just the second one ("FQDN cache exists - multiple addresses multiple domains") would have been fine as a basic (not table-driven) test.

antoninbas · 2025-01-09T21:20:55Z

pkg/antctl/transform/fqdncache/response.go

+	if err != nil {
+		return nil, err
+	}
+	domain, exists := opts["domain"]


I am not sure why you chose this approach, but I think we usually do server-side filtering. Did you follow a specific example that prompted you to do client-side filtering?
The advantage of server-side filtering is that the dns cache can be huge, so being able to filter on a specific domain name in the Agent directly would be more efficient.
BTW, I think we should support a wildcard domain in the query, as that would be useful when debugging a policy rule which uses a wildcard in the FQDN. cc @Dyanngg

Signed-off-by: Dhruv-J <[email protected]>

Signed-off-by: Dhruv Jain <[email protected]>

Signed-off-by: Dhruv-J <[email protected]>

Dyanngg · 2025-01-28T18:04:40Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

+	cacheEntryList := []types.DnsCacheEntry{}
+	var pattern *regexp.Regexp
+	var err error
+	if fqdnFilter != (querier.FQDNCacheFilter{}) {


This is quite unconventional imo, and I doubt it works the way you think it would. In newFilterFromURLQuery, it would return a new FQDNCacheFilter with "DomainName" set to "" if no filter is provided in the url, thus not matching the else branch below. What can be done is making the fqdnFilter a pointer, and check if is nil to see if there are any filtering requirements at all

Dyanngg · 2025-01-28T18:05:24Z

pkg/querier/querier.go

+// FQDNCacheFilter is used to filter the result while retrieving FQDN cache
+type FQDNCacheFilter struct {
+	// The Name or wildcard matching expression of the domain that is being filtered
+	DomainName string


nit: I'll just call it Domain since it can actually be a regex

Dyanngg · 2025-01-28T18:06:55Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

+	}
+	for fqdn, dnsMeta := range c.fqdnController.dnsEntryCache {
+		for _, ipWithExpiration := range dnsMeta.responseIPs {
+			if fqdnFilter == (querier.FQDNCacheFilter{}) || pattern.MatchString(fqdn) {


fqdnFilter == nil will also be a much more appropriate check here to avoid constructing an empty FQDNCacheFilter for each dns data in cache.

antoninbas

There is a key issue with the PR right now. You have several types that you use for the same purpose but they are not used correctly IMO.

FQDNCacheResponse in pkg/agent/apis
DnsCacheEntry in pkg/agent/types
Response in pkg/antctl/transform/fqdncache, which wraps a *DnsCacheEntry.

FQDNCacheResponse is actually not really used at all. It is definitely not used on the server side, as you are marshaling a []DnsCacheEntry. It does not have any unexported

DnsCacheEntry (and everything in pkg/agent/types) is not really meant to be used in antctl - you will notice that antctl didn't import this package prior to your change. There is also nothing with json tags in pkg/agent/types. This is what the pkg/agent/apis is meant to be for.

The data you marshal on the server side and unmarshal on the client side (antctl) should have the same type to ensure correctness.

There are 2 possible approaches:

Fix FQDNCacheResponse (export fields, add json tags, maybe replace the net.IP with a string). In the handler, convert each DnsCacheEntry to a FQDNCacheResponse manually. You can refer the bgp* agent handlers for examples. json tags should be removed from DnsCacheEntry, as this type is not meant to be serialized.
Unify FQDNCacheResponse and DnsCacheEntry as a single type (there again, I would use a string instead of a net.IP for the IP address field), and use it everywhere. We have one handler like this: pkg/agent/apiserver/handlers/serviceexternalip/handler.go, with the ServiceExternalIPInfo struct.

In both cases I think you can / should remove pkg/antctl/transform/fqdncache/response.go.

I don't have a strong preference between the 2 approaches. Typically I'd go with the first one and keep 2 types. The conversion logic is a good place to convert the net.IP field in DnsCacheEntry to a string field in FQDNCacheResponse.

antoninbas · 2025-01-30T02:07:17Z

pkg/agent/querier/querier.go

@@ -47,6 +48,7 @@ type AgentQuerier interface {
 	GetMemberlistCluster() memberlist.Interface
 	GetNodeLister() corelisters.NodeLister
 	GetBGPPolicyInfoQuerier() querier.AgentBGPPolicyInfoQuerier
+	GetFqdnCache(querier.FQDNCacheFilter) []types.DnsCacheEntry


This seems wrong: there is no need to add this function to the interface because you can already call GetNetworkPolicyInfoQuerier().GetFqdnCache(filter), which is what we do for other handlers.

antoninbas · 2025-01-30T02:07:50Z

pkg/agent/apiserver/handlers/fqdncache/handler.go

+func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		fqdnFilter := newFilterFromURLQuery(r.URL.Query())
+		dnsEntryCache := aq.GetFqdnCache(fqdnFilter)


should be GetFQDNCache per the style guide. Please update all other occurrences accordingly.

Signed-off-by: Dhruv-J <[email protected]>

Signed-off-by: Dhruv Jain <[email protected]>

Signed-off-by: Dhruv-J <[email protected]>

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas · 2025-02-07T19:50:57Z

pkg/agent/apis/types.go

+	FqdnName       string    `json:"fqdnName,omitempty"`
+	IpAddress      string    `json:"ipAddress,omitempty"`


Suggested change

FqdnName string `json:"fqdnName,omitempty"`

IpAddress string `json:"ipAddress,omitempty"`

FQDNName string `json:"fqdnName,omitempty"`

IPAddress string `json:"ipAddress,omitempty"`

Please review other occurrences as well

antoninbas · 2025-02-07T19:58:40Z

pkg/agent/apiserver/handlers/fqdncache/handler.go

+	return func(w http.ResponseWriter, r *http.Request) {
+		fqdnFilter := newFilterFromURLQuery(r.URL.Query())
+		dnsEntryCache := npq.GetFQDNCache(fqdnFilter)
+		resp := []agentapi.FQDNCacheResponse{}


Suggested change

resp := []agentapi.FQDNCacheResponse{}

resp := make([]agentapi.FQDNCacheResponse, 0, len(dnsEntryCache))

to pre-allocate the memory

antoninbas · 2025-02-07T20:05:07Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+		{
+			name:           "FQDN cache exists - multiple addresses multiple domains",
+			expectedStatus: http.StatusOK,
+			expectedResponse: []types.DnsCacheEntry{


Rename this field to filteredCacheEntries and make expectedResponse a field of type []FQDNCacheResponse instead. It will feel like you are duplicating data, but really it will lead to a cleaner test.

antoninbas · 2025-02-07T20:07:58Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

+		if err != nil {
+			// this pattern will match no strings if there is an error with the regex formatting or usage with the user specified --domain flag
+			pattern = regexp.MustCompile(`a\A`)
+		}


we should handle such errors better, i.e., by returning a BadRequest HTTP code if the domain arg is not valid.

so maybe the regex conversion should happen in the handler and FQDNCacheResponse should include a regex instead of a string? cc @Dyanngg

what about a warning displayed to the user but still an empty output?

if it's an invalid pattern, it should be an error. A warning in the Antrea Agent logs is much harder to find. We should not treat 1) an unsupported pattern, and 2) a valid pattern that doesn't match anything, in the same way IMO.

oops I thought you were done addressing comments, but looks like it is not the case
I will review again later

antoninbas · 2025-02-07T20:08:31Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

+	} else {
+		// this pattern will match all strings if the filter is unset
+		pattern = regexp.MustCompile(`.*`)
+	}


not needed, you should just skip regex evaluation in this case

Signed-off-by: Dhruv-J <[email protected]>

antoninbas

some more comments

antoninbas · 2025-02-12T01:21:49Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

-					FqdnName:       "example.com",
-					IpAddress:      net.ParseIP("10.0.0.1"),
+					FQDNName:       "example.com",
+					IPAddress:      net.ParseIP("10.0.0.1"),
 					ExpirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),


Since the same time is used everywhere, I suggest you declare one variable of type time.Time at the beginning of the test function, which you can then use everywhere. Actually, we usually do something like this:

expirationTime := time.Now().Add(1*time.Hour)

then use ExpirationTime: expirationTime everywhere

antoninbas · 2025-02-12T01:22:11Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			expectedResponse: []apis.FQDNCacheResponse{
+				{
+					FQDNName:       "example.com",
+					IPAddress:      net.ParseIP("10.0.0.1").String(),


Suggested change

IPAddress: net.ParseIP("10.0.0.1").String(),

IPAddress: "10.0.0.1",

same in other places below

antoninbas · 2025-02-12T01:23:45Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			handler.ServeHTTP(recorder, req)
+			assert.Equal(t, tt.expectedStatus, recorder.Code)
+			var receivedResponse []map[string]interface{}
+			err = json.Unmarshal(recorder.Body.Bytes(), &receivedResponse)


unmarshal the response as a []apis.FQDNCacheResponse, not as a []map[string]interface{}

and then just use assert.Equal(t, tt.expectedResponse, receivedResponse)

antoninbas · 2025-02-12T01:24:26Z

pkg/agent/controller/networkpolicy/networkpolicy_controller_test.go

+			responseIPs: map[string]ipWithExpiration{
+				"10.0.0.1": {
+					ip:             net.ParseIP("10.0.0.1"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),


same comment as for the handler unit test

antoninbas · 2025-02-12T01:24:54Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

+		if err != nil {
+			// this pattern will match no strings if there is an error with the regex formatting or usage with the user specified --domain flag
+			pattern = regexp.MustCompile(`a\A`)
+		}


oops I thought you were done addressing comments, but looks like it is not the case
I will review again later

Signed-off-by: Dhruv-J <[email protected]>

Dyanngg reviewed Dec 16, 2024

View reviewed changes

Dhruv-J marked this pull request as draft December 19, 2024 19:43

Dhruv-J force-pushed the antctl-get-fqdncache branch 3 times, most recently from ae05e95 to 6a561ee Compare December 30, 2024 10:22

Dhruv-J force-pushed the antctl-get-fqdncache branch 2 times, most recently from 0f68993 to 17487a4 Compare January 2, 2025 20:50

Dhruv-J added 3 commits January 2, 2025 13:34

linter fix

a0e5592

Signed-off-by: Dhruv-J <[email protected]>

linter fixes additional

e3ba43e

Signed-off-by: Dhruv-J <[email protected]>

removed last debugs, ready for review

5a81c61

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J marked this pull request as ready for review January 6, 2025 21:32

Dhruv-J added 3 commits January 6, 2025 15:44

codecov update and unit test for GetFqdnCache()

e77eead

Signed-off-by: Dhruv-J <[email protected]>

unit test check

2ed9291

Signed-off-by: Dhruv-J <[email protected]>

test checking logic updated

fcab9bd

Signed-off-by: Dhruv-J <[email protected]>

Dyanngg reviewed Jan 7, 2025

View reviewed changes

elton-furtado reviewed Jan 7, 2025

View reviewed changes

addressed feedback

796eec1

Signed-off-by: Dhruv-J <[email protected]>

antoninbas reviewed Jan 9, 2025

View reviewed changes

Dhruv-J and others added 5 commits January 12, 2025 16:52

adding server side handling for flags progress

4021601

Signed-off-by: Dhruv-J <[email protected]>

codegen changes for updated GetFqdnCache()

bb124b8

Signed-off-by: Dhruv Jain <[email protected]>

fixed unit tests, ready to add wildcard domain flag

0e6dbaf

Signed-off-by: Dhruv-J <[email protected]>

updated with feedback and everything working

cdeca7e

Signed-off-by: Dhruv-J <[email protected]>

small update adding usage instructions for command

3165341

Signed-off-by: Dhruv-J <[email protected]>

Dyanngg reviewed Jan 28, 2025

View reviewed changes

antoninbas reviewed Jan 30, 2025

View reviewed changes

Dhruv-J and others added 4 commits February 6, 2025 12:11

updated name, commit before generating code

d0ef27c

Signed-off-by: Dhruv-J <[email protected]>

updated generated code

90454ac

Signed-off-by: Dhruv Jain <[email protected]>

updated feedback - final | pre gen code fix

85ca02e

Signed-off-by: Dhruv-J <[email protected]>

codegen

11df03b

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas reviewed Feb 7, 2025

View reviewed changes

Dhruv-J added 2 commits February 10, 2025 11:00

name change updates

e3d2e41

Signed-off-by: Dhruv-J <[email protected]>

feedback addressed

6b4339f

Signed-off-by: Dhruv-J <[email protected]>

antoninbas reviewed Feb 12, 2025

View reviewed changes

updated error handling for query

9bacd69

Signed-off-by: Dhruv-J <[email protected]>

	// Key for responseIPs is the string representation of the IP.
	// It helps to quickly identify IP address updates when a
	// new DNS response is received.
	responseIPs map[string]ipWithExpiration

		FqdnName string `json:"fqdnName,omitempty"`
		IpAddress string `json:"ipAddress,omitempty"`

	resp := []agentapi.FQDNCacheResponse{}
	resp := make([]agentapi.FQDNCacheResponse, 0, len(dnsEntryCache))

	IPAddress: net.ParseIP("10.0.0.1").String(),
	IPAddress: "10.0.0.1",

Add antctl get fqdncache #6868

Are you sure you want to change the base?

Add antctl get fqdncache #6868

Conversation

Dhruv-J commented Dec 16, 2024

Dyanngg left a comment • edited Loading

Choose a reason for hiding this comment

Dhruv-J commented Jan 6, 2025

Dhruv-J commented Jan 7, 2025

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

elton-furtado commented Jan 7, 2025

Dhruv-J commented Jan 8, 2025

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

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

Dyanngg left a comment •

edited

Loading