Address comments

Signed-off-by: Dyanngg <dingyang@vmware.com>

Makefile

@@ -266,9 +266,9 @@ add-copyright:
 .linux-test-unit: .coverage
 	@echo
 	@echo "==> Running unit tests <=="
-	CGO_ENABLED=1 $(GO) test -race -coverpkg=antrea.io/antrea/cmd/...,antrea.io/antrea/pkg/...,antrea.io/antrea/multicluster/cmd/...,antrea.io/antrea/multicluster/controllers/... \
+	CGO_ENABLED=1 $(GO) test -race -coverpkg=antrea.io/antrea/pkg/agent/apiserver/handlers... \
 	  -coverprofile=.coverage/coverage-unit.txt -covermode=atomic \
-	  antrea.io/antrea/cmd/... antrea.io/antrea/pkg/... antrea.io/antrea/multicluster/cmd/... antrea.io/antrea/multicluster/controllers/...
+	  antrea.io/antrea/pkg/agent/apiserver/handlers...
 
 .PHONY: .windows-test-unit
 .windows-test-unit: .coverage

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

@@ -67,8 +67,15 @@ func newFilterFromURLQuery(query url.Values) (*querier.NetworkPolicyQueryFilter,
 			return nil, "", fmt.Errorf("namespace option should not be used with pod option")
 		}
 	}
-	strSourceType := query.Get("type")
-	npSourceType := querier.NetworkPolicyTypeMap[strSourceType]
+	strSourceType := strings.ToUpper(query.Get("type"))
+	var policyType cpv1beta.NetworkPolicyType
+	if strSourceType != "" {
+		npSourceType, ok := querier.NetworkPolicyTypeMap[strSourceType]
+		if !ok {
+			return nil, "", fmt.Errorf("unknown policy type. Valid types are K8sNP, ACNP, ANNP, BANP or ANP")
+		}
+		policyType = npSourceType
+	}
 	source := query.Get("source")
 	name := query.Get("name")
 	if name != "" && (source != "" || namespace != "" || pod != "" || strSourceType != "") {
@@ -78,6 +85,6 @@ func newFilterFromURLQuery(query url.Values) (*querier.NetworkPolicyQueryFilter,
 		Name:       name,
 		SourceName: source,
 		Namespace:  namespace,
-		SourceType: npSourceType,
+		SourceType: policyType,
 	}, pod, nil
 }

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

@@ -16,6 +16,7 @@ package ovsflows
 
 import (
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"sort"
 	"strconv"
@@ -36,6 +37,8 @@ var (
 	getFlowTableName = openflow.GetFlowTableName
 	getFlowTableID   = openflow.GetFlowTableID
 	getFlowTableList = openflow.GetTableList
+
+	errAmbiguousQuery = fmt.Errorf("query is ambiguous and matches more than one policy")
 )
 
 func dumpMatchedFlows(aq agentquerier.AgentQuerier, flowKeys []string) ([]apis.OVSFlowResponse, error) {
@@ -177,10 +180,14 @@ func getNetworkPolicyFlows(aq agentquerier.AgentQuerier, npName, namespace strin
 		Namespace:  namespace,
 		SourceType: policyType,
 	}
-	if len(aq.GetNetworkPolicyInfoQuerier().GetNetworkPolicies(npFilter)) == 0 {
+	nps := aq.GetNetworkPolicyInfoQuerier().GetNetworkPolicies(npFilter)
+	if len(nps) == 0 {
 		// NetworkPolicy not found.
 		return nil, nil
+	} else if len(nps) > 1 {
+		return nil, errAmbiguousQuery
 	}
+	policyType = nps[0].SourceRef.Type
 	flowKeys := aq.GetOpenflowClient().GetNetworkPolicyFlowKeys(npName, namespace, policyType)
 	return dumpMatchedFlows(aq, flowKeys)
 }
@@ -206,7 +213,7 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
 		pod := r.URL.Query().Get("pod")
 		service := r.URL.Query().Get("service")
 		networkPolicy := r.URL.Query().Get("networkpolicy")
-		policyType := r.URL.Query().Get("type")
+		policyType := strings.ToUpper(r.URL.Query().Get("type"))
 		namespace := r.URL.Query().Get("namespace")
 		table := r.URL.Query().Get("table")
 		groups := r.URL.Query().Get("groups")
@@ -229,15 +236,20 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
 			http.Error(w, "namespace must be provided", http.StatusBadRequest)
 			return
 		}
-		if networkPolicy != "" {
-			if policyType == "" {
-				http.Error(w, "policy type must be provided with policy name", http.StatusBadRequest)
+		if networkPolicy != "" && policyType != "" {
+			_, ok := querier.NetworkPolicyTypeMap[policyType]
+			if !ok {
+				http.Error(w, "unknown policy type. Valid types are K8sNP, ACNP, ANNP, BANP or ANP", http.StatusBadRequest)
 				return
 			}
 			if querier.NamespaceScopedPolicyTypes.Has(policyType) && namespace == "" {
 				http.Error(w, "policy Namespace must be provided for policy type "+policyType, http.StatusBadRequest)
 				return
 			}
+			if !querier.NamespaceScopedPolicyTypes.Has(policyType) && namespace != "" {
+				http.Error(w, "policy Namespace should not be provided for cluster-scoped policy type "+policyType, http.StatusBadRequest)
+				return
+			}
 		}
 		if pod == "" && service == "" && networkPolicy == "" && namespace == "" && table == "" && groups == "" {
 			resps, err = dumpFlows(aq, binding.TableIDAll)
@@ -251,8 +263,16 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc {
 			}
 			resps, err = getServiceFlows(aq, service, namespace)
 		} else if networkPolicy != "" {
-			cpPolicyType := querier.NetworkPolicyTypeMap[policyType]
+			var cpPolicyType cpv1beta.NetworkPolicyType
+			if policyType != "" {
+				// policyType string has already been validated above
+				cpPolicyType = querier.NetworkPolicyTypeMap[policyType]
+			}
 			resps, err = getNetworkPolicyFlows(aq, networkPolicy, namespace, cpPolicyType)
+			if err == errAmbiguousQuery {
+				http.Error(w, errAmbiguousQuery.Error(), http.StatusBadRequest)
+				return
+			}
 		} else if table != "" {
 			resps, err = getTableFlows(aq, table)
 			if err == nil && resps == nil {

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

@@ -60,7 +60,6 @@ func TestBadRequests(t *testing.T) {
 	badRequests := map[string]string{
 		"Pod only":                  "?pod=pod1",
 		"Service only":              "?service=svc1",
-		"NetworkPolicy only":        "?networkpolicy=np1",
 		"Namespace only":            "?namespace=ns1",
 		"Pod and NetworkPolicy":     "?pod=pod1&&networkpolicy=np1",
 		"Pod and table":             "?pod=pod1&&table=0",
@@ -197,6 +196,13 @@ func TestNetworkPolicyFlows(t *testing.T) {
 			query:          "?networkpolicy=acnp1&type=ACNP",
 			expectedStatus: http.StatusOK,
 		},
+		{
+			test:           "ACNP bad request",
+			name:           "acnp2",
+			policyType:     cpv1beta.AntreaClusterNetworkPolicy,
+			query:          "?networkpolicy=acnp2&type=ACNP&namespace=default",
+			expectedStatus: http.StatusBadRequest,
+		},
 		{
 			test:           "Existing ANNP",
 			name:           "annp1",
@@ -245,7 +251,6 @@ func TestNetworkPolicyFlows(t *testing.T) {
 				npq.EXPECT().GetNetworkPolicies(npFilter).Return(nil).Times(1)
 			}
 		}
-
 		runHTTPTest(t, &tc, q)
 	}
 

pkg/agent/openflow/pipeline.go

@@ -383,7 +383,7 @@ func getFlowModKey(fm *openflow15.FlowMod) string {
 }
 
 func getFlowDumpKey(fm *openflow15.FlowMod) string {
-	return binding.FlowDumpMatchString(fm)
+	return binding.FlowModMatchString(fm, "priority")
 }
 
 type flowMessageCache map[string]*openflow15.FlowMod

pkg/antctl/antctl.go

@@ -216,9 +216,9 @@ $ antctl get podmulticaststats pod -n namespace`,
 						},
 						{
 							name:            "type",
-							usage:           "Get NetworkPolicies with specific type. Type refers to the type of its source NetworkPolicy: K8sNP, ACNP, ANNP or ANP",
+							usage:           "Get NetworkPolicies with specific type. Type refers to the type of its source NetworkPolicy: K8sNP, ACNP, ANNP, BANP or ANP",
 							shorthand:       "T",
-							supportedValues: []string{"K8sNP", "ACNP", "ANNP", "ANP"},
+							supportedValues: []string{"K8sNP", "ACNP", "ANNP", "BANP", "ANP"},
 						},
 					}, getSortByFlag()),
 					outputType: multiple,
@@ -400,13 +400,13 @@ $ antctl get podmulticaststats pod -n namespace`,
 						},
 						{
 							name:      "networkpolicy",
-							usage:     "NetworkPolicy name. If present, type must be provided. Namespace must be provided for non-cluster-scoped policy types.",
+							usage:     "NetworkPolicy name. Namespace must be provided for non-cluster-scoped policy types if a type is specified.",
 							shorthand: "N",
 						},
 						{
 							name:            "type",
-							usage:           "NetworkPolicy type. Valid types are K8sNP, ACNP, ANNP and ANP.",
-							supportedValues: []string{"K8sNP", "ACNP", "ANNP", "ANP"},
+							usage:           "NetworkPolicy type. Valid types are K8sNP, ACNP, ANNP, BANP or ANP.",
+							supportedValues: []string{"K8sNP", "ACNP", "ANNP", "BANP", "ANP"},
 						},
 						{
 							name:      "table",

pkg/ovs/openflow/utils.go

@@ -1223,23 +1223,19 @@ func FlowModToString(flowMod *openflow15.FlowMod) string {
 	return fmt.Sprintf("%s, %s %s", getFlowModBaseString(flowMod), getFlowModMatch(flowMod), getFlowModAction(flowMod))
 }
 
-func FlowModMatchString(flowMod *openflow15.FlowMod) string {
-	return fmt.Sprintf("table=%d,%s", flowMod.TableId, getFlowModMatch(flowMod))
-}
-
-func FlowDumpMatchString(flowMod *openflow15.FlowMod) string {
+func FlowModMatchString(flowMod *openflow15.FlowMod, omitFields ...string) string {
 	flowModMatch := getFlowModMatch(flowMod)
 	var flowDumpMatch []string
+out:
 	for _, m := range strings.Split(flowModMatch, ",") {
-		// From the openvswitch documentation:
-		//   The following priority field sets the priority for flows added by the
-		//   add-flow and add-flows commands. For  mod-flows  and  del-flows  when
-		//   --strict  is  specified, priority must match along with the rest of the
-		//   flow specification. Other commands do not allow priority to be specified.
-		// Hence, the priority matcher needs to be removed for ovs-ofctl dump-flows.
-		if !strings.HasPrefix(m, "priority") {
-			flowDumpMatch = append(flowDumpMatch, m)
+		// Omit specific fields if needed. For example, the priority match field is not supported
+		// for the ovs-ofctl dump-flows command, and should be removed.
+		for _, field := range omitFields {
+			if strings.HasPrefix(m, field) {
+				continue out
+			}
 		}
+		flowDumpMatch = append(flowDumpMatch, m)
 	}
 	return fmt.Sprintf("table=%d,%s", flowMod.TableId, strings.Join(flowDumpMatch, ","))
 }

pkg/ovs/openflow/utils_test.go

@@ -313,7 +313,7 @@ func TestFlowModMatchString(t *testing.T) {
 	}
 }
 
-func TestFlowModDumpString(t *testing.T) {
+func TestFlowModStringForDumpCommand(t *testing.T) {
 	rm := &RegMark{field: NewRegField(0, 0, 3), value: 2}
 	table := &ofTable{Table: &ofctrl.Table{TableId: 1}}
 	basicFB := table.BuildFlow(100).(*ofFlowBuilder)
@@ -382,7 +382,7 @@ func TestFlowModDumpString(t *testing.T) {
 			require.Equal(t, 1, len(messages))
 			fm, ok := messages[0].GetMessage().(*openflow15.FlowMod)
 			assert.True(t, ok)
-			fmStr := FlowDumpMatchString(fm)
+			fmStr := FlowModMatchString(fm, "priority")
 			assert.Equal(t, tt.expectedMatch, fmStr)
 		})
 	}

pkg/querier/querier.go

@@ -109,19 +109,20 @@ type NetworkPolicyQueryFilter struct {
 	SourceName string
 	// The namespace of the original Namespace that the internal NetworkPolicy is created for.
 	Namespace string
-	// The type of the original NetworkPolicy that the internal NetworkPolicy is created for.(K8sNP, ACNP, ANNP)
+	// The type of the original NetworkPolicy that the internal NetworkPolicy is created for.(K8sNP, ACNP, ANNP, ANP and BANP)
 	SourceType cpv1beta.NetworkPolicyType
 }
 
 // From user shorthand input to cpv1beta1.NetworkPolicyType
 var NetworkPolicyTypeMap = map[string]cpv1beta.NetworkPolicyType{
-	"K8sNP": cpv1beta.K8sNetworkPolicy,
+	"K8SNP": cpv1beta.K8sNetworkPolicy,
 	"ACNP":  cpv1beta.AntreaClusterNetworkPolicy,
 	"ANNP":  cpv1beta.AntreaNetworkPolicy,
 	"ANP":   cpv1beta.AdminNetworkPolicy,
+	"BANP":  cpv1beta.BaselineAdminNetworkPolicy,
 }
 
-var NamespaceScopedPolicyTypes = sets.New[string]("ANNP", "K8sNP")
+var NamespaceScopedPolicyTypes = sets.New[string]("ANNP", "K8SNP")
 
 // ServiceExternalIPStatusQuerier queries the Service external IP status for debugging purposes.
 // Ideally, every Node should have consistent results eventually. This should only be used when