Address comments

Signed-off-by: Dyanngg <[email protected]>

docs/antctl.md

@@ -350,7 +350,7 @@ in the specified OVS flow tables, or all or the specified OVS groups.
 antctl get ovsflows
 antctl get ovsflows -p POD -n NAMESPACE
 antctl get ovsflows -S SERVICE -n NAMESPACE
-antctl get ovsflows -N NETWORKPOLICY -n NAMESPACE
+antctl get ovsflows -N NETWORKPOLICY -n NAMESPACE --type NETWORKPOLICY_TYPE
 antctl get ovsflows -T TABLE_A,TABLE_B
 antctl get ovsflows -T TABLE_A,TABLE_B_NUM
 antctl get ovsflows -G all
@@ -381,13 +381,22 @@ kube-system kube-dns 160ea6d7-0234-5d1d-8ea0-b703d0aa3b46 1
 # Dump OVS flows of NetworkPolicy "kube-dns"
 $ antctl get of -N kube-dns -n kube-system
 FLOW
-table=90, n_packets=0, n_bytes=0, priority=190,conj_id=1,ip actions=resubmit(,105)
-table=90, n_packets=0, n_bytes=0, priority=200,ip actions=conjunction(1,1/3)
-table=90, n_packets=0, n_bytes=0, priority=200,ip,reg1=0x5 actions=conjunction(2,2/3),conjunction(1,2/3)
-table=90, n_packets=0, n_bytes=0, priority=200,udp,tp_dst=53 actions=conjunction(1,3/3)
-table=90, n_packets=0, n_bytes=0, priority=200,tcp,tp_dst=53 actions=conjunction(1,3/3)
-table=90, n_packets=0, n_bytes=0, priority=200,tcp,tp_dst=9153 actions=conjunction(1,3/3)
-table=100, n_packets=0, n_bytes=0, priority=200,ip,reg1=0x5 actions=drop
+table=IngressRule, n_packets=0, n_bytes=0, priority=190,conj_id=1,ip actions=set_field:0x1->reg5,ct(commit,table=IngressMetric,zone=65520,exec(set_field:0x1/0xffffffff->ct_label))
+table=IngressRule, n_packets=0, n_bytes=0, priority=200,ip actions=conjunction(1,1/3)
+table=IngressRule, n_packets=0, n_bytes=0, priority=200,ip,reg1=0x5 actions=conjunction(2,2/3),conjunction(1,2/3)
+table=IngressRule, n_packets=0, n_bytes=0, priority=200,udp,tp_dst=53 actions=conjunction(1,3/3)
+table=IngressRule, n_packets=0, n_bytes=0, priority=200,tcp,tp_dst=53 actions=conjunction(1,3/3)
+table=IngressRule, n_packets=0, n_bytes=0, priority=200,tcp,tp_dst=9153 actions=conjunction(1,3/3)
+table=IngressDefaultRule, n_packets=0, n_bytes=0, priority=200,ip,reg1=0x5 actions=drop
+
+# Dump OVS flows of AntreaNetworkPolicy "test-annp"
+$ antctl get ovsflows -N test-annp -n default --type ANNP
+FLOW
+table=AntreaPolicyIngressRule, n_packets=0, n_bytes=0, priority=14900,conj_id=6 actions=set_field:0x6->reg3,set_field:0x400/0x400->reg0,goto_table:IngressMetric
+table=AntreaPolicyIngressRule, n_packets=0, n_bytes=0, priority=14900,ip,nw_src=10.20.1.8 actions=conjunction(6,1/3)
+table=AntreaPolicyIngressRule, n_packets=0, n_bytes=0, priority=14900,ip,nw_src=10.20.2.8 actions=conjunction(6,1/3)
+table=AntreaPolicyIngressRule, n_packets=0, n_bytes=0, priority=14900,reg1=0x3 actions=conjunction(6,2/3)
+table=AntreaPolicyIngressRule, n_packets=0, n_bytes=0, priority=14900,tcp,tp_dst=443 actions=conjunction(6,3/3)
 ```
 
 ### OVS packet tracing

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
 	}
+	namespace, policyType = nps[0].SourceRef.Namespace, 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

@@ -53,14 +53,14 @@ type testCase struct {
 	policyType     cpv1beta.NetworkPolicyType
 	query          string
 	expectedStatus int
+	expectedErr    error
 	resps          []apis.OVSFlowResponse
 }
 
 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",
@@ -172,14 +172,35 @@ func TestServiceFlows(t *testing.T) {
 
 func TestNetworkPolicyFlows(t *testing.T) {
 	ctrl := gomock.NewController(t)
-	testNetworkPolicy := &cpv1beta.NetworkPolicy{}
+	testNetworkPolicy := &cpv1beta.NetworkPolicy{
+		SourceRef: &cpv1beta.NetworkPolicyReference{
+			Type:      cpv1beta.K8sNetworkPolicy,
+			Namespace: "default",
+		},
+	}
+	testANNP := &cpv1beta.NetworkPolicy{
+		SourceRef: &cpv1beta.NetworkPolicyReference{
+			Type:      cpv1beta.AntreaNetworkPolicy,
+			Namespace: "default",
+		},
+	}
+	testACNP := &cpv1beta.NetworkPolicy{
+		SourceRef: &cpv1beta.NetworkPolicyReference{
+			Type: cpv1beta.AntreaClusterNetworkPolicy,
+		},
+	}
+	testANP := &cpv1beta.NetworkPolicy{
+		SourceRef: &cpv1beta.NetworkPolicyReference{
+			Type: cpv1beta.AdminNetworkPolicy,
+		},
+	}
 	testcases := []testCase{
 		{
 			test:           "Existing NetworkPolicy",
 			name:           "np1",
-			namespace:      "ns1",
+			namespace:      "default",
 			policyType:     cpv1beta.K8sNetworkPolicy,
-			query:          "?networkpolicy=np1&namespace=ns1&type=K8sNP",
+			query:          "?networkpolicy=np1&namespace=default&type=K8sNP",
 			expectedStatus: http.StatusOK,
 		},
 		{
@@ -190,13 +211,28 @@ func TestNetworkPolicyFlows(t *testing.T) {
 			query:          "?networkpolicy=np2&namespace=ns2&type=K8sNP",
 			expectedStatus: http.StatusNotFound,
 		},
+		{
+			test:           "Ambiguous query",
+			name:           "np3",
+			namespace:      "ns3",
+			query:          "?networkpolicy=np3&namespace=ns3",
+			expectedStatus: http.StatusBadRequest,
+			expectedErr:    errAmbiguousQuery,
+		},
 		{
 			test:           "Existing ACNP",
 			name:           "acnp1",
 			policyType:     cpv1beta.AntreaClusterNetworkPolicy,
 			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",
@@ -229,23 +265,33 @@ func TestNetworkPolicyFlows(t *testing.T) {
 			Namespace:  tc.namespace,
 			SourceType: tc.policyType,
 		}
-		if tc.expectedStatus != http.StatusBadRequest {
+		if tc.expectedStatus != http.StatusBadRequest || tc.expectedErr == errAmbiguousQuery {
 			q.EXPECT().GetNetworkPolicyInfoQuerier().Return(npq).Times(1)
 			if tc.expectedStatus == http.StatusOK {
 				ofc := oftest.NewMockClient(ctrl)
 				ovsctl := ovsctltest.NewMockOVSCtlClient(ctrl)
-				npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testNetworkPolicy}).Times(1)
+				switch tc.policyType {
+				case cpv1beta.K8sNetworkPolicy:
+					npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testNetworkPolicy}).Times(1)
+				case cpv1beta.AntreaClusterNetworkPolicy:
+					npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testACNP}).Times(1)
+				case cpv1beta.AntreaNetworkPolicy:
+					npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testANNP}).Times(1)
+				case cpv1beta.AdminNetworkPolicy:
+					npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testANP}).Times(1)
+				}
 				ofc.EXPECT().GetNetworkPolicyFlowKeys(tc.name, tc.namespace, tc.policyType).Return(testFlowKeys).Times(1)
 				q.EXPECT().GetOpenflowClient().Return(ofc).Times(1)
 				q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(len(testFlowKeys))
 				for i := range testFlowKeys {
 					ovsctl.EXPECT().DumpMatchedFlow(testFlowKeys[i]).Return(testDumpFlows[i], nil).Times(1)
 				}
-			} else {
+			} else if tc.expectedStatus == http.StatusNotFound {
 				npq.EXPECT().GetNetworkPolicies(npFilter).Return(nil).Times(1)
+			} else {
+				npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testNetworkPolicy, *testNetworkPolicy}).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