diff --git a/Makefile b/Makefile index e455cad33dc..34553f3d284 100644 --- a/Makefile +++ b/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 diff --git a/pkg/agent/apiserver/handlers/networkpolicy/handler.go b/pkg/agent/apiserver/handlers/networkpolicy/handler.go index f5a275ce64f..e2334da968e 100644 --- a/pkg/agent/apiserver/handlers/networkpolicy/handler.go +++ b/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 } diff --git a/pkg/agent/apiserver/handlers/ovsflows/handler.go b/pkg/agent/apiserver/handlers/ovsflows/handler.go index 31c628b4d14..e01623faac8 100644 --- a/pkg/agent/apiserver/handlers/ovsflows/handler.go +++ b/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 { diff --git a/pkg/agent/apiserver/handlers/ovsflows/handler_test.go b/pkg/agent/apiserver/handlers/ovsflows/handler_test.go index f21506bb977..a6713be7d85 100644 --- a/pkg/agent/apiserver/handlers/ovsflows/handler_test.go +++ b/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) } diff --git a/pkg/agent/openflow/pipeline.go b/pkg/agent/openflow/pipeline.go index b663daae11e..c07d843dc6f 100644 --- a/pkg/agent/openflow/pipeline.go +++ b/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 diff --git a/pkg/antctl/antctl.go b/pkg/antctl/antctl.go index 2f9d26ae8d2..ddd91e6a6ab 100644 --- a/pkg/antctl/antctl.go +++ b/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", diff --git a/pkg/ovs/openflow/utils.go b/pkg/ovs/openflow/utils.go index e562f3b6db9..ded90121e26 100644 --- a/pkg/ovs/openflow/utils.go +++ b/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, ",")) } diff --git a/pkg/ovs/openflow/utils_test.go b/pkg/ovs/openflow/utils_test.go index 8fa309de637..3bbaaaabe9e 100644 --- a/pkg/ovs/openflow/utils_test.go +++ b/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) }) } diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go index b3f151cc022..a60f5e0fd9e 100644 --- a/pkg/querier/querier.go +++ b/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