From 34207d943ab5aa7d44dc9144a5b171d5ca3df03d Mon Sep 17 00:00:00 2001 From: Dyanngg Date: Tue, 13 Aug 2024 11:54:21 -0700 Subject: [PATCH] Address more comments Signed-off-by: Dyanngg --- docs/antctl.md | 2 +- .../apiserver/handlers/ovsflows/handler.go | 3 +- .../handlers/ovsflows/handler_test.go | 270 ++++++++++-------- pkg/ovs/openflow/utils.go | 14 +- pkg/ovs/openflow/utils_test.go | 6 +- pkg/querier/querier.go | 9 + 6 files changed, 168 insertions(+), 136 deletions(-) diff --git a/docs/antctl.md b/docs/antctl.md index 41992f287d9..17ebcc53aa5 100644 --- a/docs/antctl.md +++ b/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 --type NETWORKPOLICY_TYPE +antctl get ovsflows [-n NAMESPACE] -N NETWORKPOLICY --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 diff --git a/pkg/agent/apiserver/handlers/ovsflows/handler.go b/pkg/agent/apiserver/handlers/ovsflows/handler.go index db640b6c72b..12a9067043f 100644 --- a/pkg/agent/apiserver/handlers/ovsflows/handler.go +++ b/pkg/agent/apiserver/handlers/ovsflows/handler.go @@ -239,7 +239,8 @@ func HandleFunc(aq agentquerier.AgentQuerier) http.HandlerFunc { 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) + errorMsg := fmt.Sprintf("unknown policy type. Valid types are %v", querier.GetNetworkPolicyTypeShorthands()) + http.Error(w, errorMsg, http.StatusBadRequest) return } if querier.NamespaceScopedPolicyTypes.Has(policyType) && namespace == "" { diff --git a/pkg/agent/apiserver/handlers/ovsflows/handler_test.go b/pkg/agent/apiserver/handlers/ovsflows/handler_test.go index eb88a30487b..aae82231f43 100644 --- a/pkg/agent/apiserver/handlers/ovsflows/handler_test.go +++ b/pkg/agent/apiserver/handlers/ovsflows/handler_test.go @@ -44,16 +44,28 @@ var ( testDumpGroups = []string{"group1", "group2"} testResponses = []apis.OVSFlowResponse{{Flow: "flow1"}, {Flow: "flow2"}} testGroupResponses = []apis.OVSFlowResponse{{Flow: "group1"}, {Flow: "group2"}} + + testNetworkPolicy = &cpv1beta.NetworkPolicy{ + SourceRef: &cpv1beta.NetworkPolicyReference{ + Type: cpv1beta.K8sNetworkPolicy, + Namespace: "default", + }, + } + testANNP = &cpv1beta.NetworkPolicy{ + SourceRef: &cpv1beta.NetworkPolicyReference{ + Type: cpv1beta.AntreaNetworkPolicy, + Namespace: "default", + }, + } ) type testCase struct { - test string + testName string name string namespace string policyType cpv1beta.NetworkPolicyType query string expectedStatus int - expectedErr error resps []apis.OVSFlowResponse } @@ -89,14 +101,14 @@ func TestPodFlows(t *testing.T) { testInterface := &interfacestore.InterfaceConfig{InterfaceName: "interface0"} testcases := []testCase{ { - test: "Existing Pod", + testName: "Existing Pod", name: "pod1", namespace: "ns1", query: "?pod=pod1&&namespace=ns1", expectedStatus: http.StatusOK, }, { - test: "Non-existing Pod", + testName: "Non-existing Pod", name: "pod2", namespace: "ns2", query: "?pod=pod2&&namespace=ns2", @@ -131,7 +143,7 @@ func TestServiceFlows(t *testing.T) { ctrl := gomock.NewController(t) testcases := []testCase{ { - test: "Existing Service", + testName: "Existing Service", name: "svc1", namespace: "ns1", query: "?service=svc1&&namespace=ns1", @@ -139,7 +151,7 @@ func TestServiceFlows(t *testing.T) { resps: append(testResponses, testGroupResponses...), }, { - test: "Non-existing Service", + testName: "Non-existing Service", name: "svc2", namespace: "ns2", query: "?service=svc2&&namespace=ns2", @@ -170,33 +182,10 @@ func TestServiceFlows(t *testing.T) { } } -func TestNetworkPolicyFlows(t *testing.T) { - ctrl := gomock.NewController(t) - 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, - }, - } +func TestNetworkPolicyFlowsSuccess(t *testing.T) { testcases := []testCase{ { - test: "Existing NetworkPolicy", + testName: "Existing NetworkPolicy", name: "np1", namespace: "default", policyType: cpv1beta.K8sNetworkPolicy, @@ -204,37 +193,14 @@ func TestNetworkPolicyFlows(t *testing.T) { expectedStatus: http.StatusOK, }, { - test: "Non-existing NetworkPolicy", - name: "np2", - namespace: "ns2", - policyType: cpv1beta.K8sNetworkPolicy, - 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", + testName: "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", + testName: "Existing ANNP", name: "annp1", namespace: "default", policyType: cpv1beta.AntreaNetworkPolicy, @@ -242,59 +208,112 @@ func TestNetworkPolicyFlows(t *testing.T) { expectedStatus: http.StatusOK, }, { - test: "ANNP bad request", + testName: "Existing ANP", + name: "anp1", + policyType: cpv1beta.AdminNetworkPolicy, + query: "?networkpolicy=anp1&type=ANP", + expectedStatus: http.StatusOK, + }, + } + for _, tc := range testcases { + t.Run(tc.testName, func(t *testing.T) { + ctrl := gomock.NewController(t) + npq := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl) + q := aqtest.NewMockAgentQuerier(ctrl) + ofc := oftest.NewMockClient(ctrl) + ovsctl := ovsctltest.NewMockOVSCtlClient(ctrl) + npFilter := &querier.NetworkPolicyQueryFilter{ + SourceName: tc.name, + Namespace: tc.namespace, + SourceType: tc.policyType, + } + q.EXPECT().GetNetworkPolicyInfoQuerier().Return(npq).Times(1) + npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{ + { + SourceRef: &cpv1beta.NetworkPolicyReference{ + Type: tc.policyType, + Namespace: tc.namespace, + Name: tc.name, + }, + }, + }).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) + } + runHTTPTest(t, &tc, q) + }) + } +} + +func TestNetworkPolicyFlowsBadRequest(t *testing.T) { + testcases := []testCase{ + { + testName: "ACNP bad request", + name: "acnp2", + policyType: cpv1beta.AntreaClusterNetworkPolicy, + query: "?networkpolicy=acnp2&type=ACNP&namespace=default", + expectedStatus: http.StatusBadRequest, + }, + { + testName: "ANNP bad request", name: "annp2", policyType: cpv1beta.AntreaNetworkPolicy, query: "?networkpolicy=annp2&type=ANNP", expectedStatus: http.StatusBadRequest, }, { - test: "Existing ANP", - name: "anp1", - policyType: cpv1beta.AdminNetworkPolicy, - query: "?networkpolicy=anp1&type=ANP", - expectedStatus: http.StatusOK, + testName: "Ambiguous query", + name: "np-annp-same-name", + namespace: "ns1", + query: "?networkpolicy=np-annp-same-name&namespace=ns1", + expectedStatus: http.StatusBadRequest, }, } - for i := range testcases { - tc := testcases[i] - npq := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl) + for _, tc := range testcases { + t.Run(tc.testName, func(t *testing.T) { + ctrl := gomock.NewController(t) + q := aqtest.NewMockAgentQuerier(ctrl) + if tc.name == "np-annp-same-name" { + // Simulates an ambiguous query where more than one matching NP is returned + npq := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl) + q.EXPECT().GetNetworkPolicyInfoQuerier().Return(npq).Times(1) + npFilter := &querier.NetworkPolicyQueryFilter{ + SourceName: tc.name, + Namespace: tc.namespace, + SourceType: tc.policyType, + } + npq.EXPECT().GetNetworkPolicies(npFilter).Return([]cpv1beta.NetworkPolicy{*testNetworkPolicy, *testANNP}).Times(1) + } + runHTTPTest(t, &tc, q) + }) + } +} + +func TestNetworkPolicyFlowsPolicyNotFound(t *testing.T) { + tc := &testCase{ + testName: "Non-existing NetworkPolicy", + name: "np1", + namespace: "ns1", + policyType: cpv1beta.K8sNetworkPolicy, + query: "?networkpolicy=np1&namespace=ns1&type=K8sNP", + expectedStatus: http.StatusNotFound, + } + t.Run(tc.testName, func(t *testing.T) { + ctrl := gomock.NewController(t) q := aqtest.NewMockAgentQuerier(ctrl) + npq := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl) + q.EXPECT().GetNetworkPolicyInfoQuerier().Return(npq).Times(1) npFilter := &querier.NetworkPolicyQueryFilter{ SourceName: tc.name, Namespace: tc.namespace, SourceType: tc.policyType, } - 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) - 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 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) - } - + npq.EXPECT().GetNetworkPolicies(npFilter).Return(nil).Times(1) + runHTTPTest(t, tc, q) + }) } func TestTableFlows(t *testing.T) { @@ -303,33 +322,33 @@ func TestTableFlows(t *testing.T) { getFlowTableID = mockGetFlowTableID testcases := []testCase{ { - test: "Table 80", + testName: "Table 80", query: "?table=80", expectedStatus: http.StatusOK, }, { - test: "Table IngressRule", + testName: "Table IngressRule", query: "?table=IngressRule", expectedStatus: http.StatusOK, }, } - for i := range testcases { - tc := testcases[i] - ovsctl := ovsctltest.NewMockOVSCtlClient(ctrl) - q := aqtest.NewMockAgentQuerier(ctrl) - q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(1) - ovsctl.EXPECT().DumpTableFlows(gomock.Any()).Return(testDumpFlows, nil).Times(1) + for _, tc := range testcases { + t.Run(tc.testName, func(t *testing.T) { + ovsctl := ovsctltest.NewMockOVSCtlClient(ctrl) + q := aqtest.NewMockAgentQuerier(ctrl) + q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(1) + ovsctl.EXPECT().DumpTableFlows(gomock.Any()).Return(testDumpFlows, nil).Times(1) - runHTTPTest(t, &tc, q) + runHTTPTest(t, &tc, q) + }) } - } func TestTableNamesOnly(t *testing.T) { ctrl := gomock.NewController(t) getFlowTableList = mockGetTableList tc := testCase{ - test: "Get table names only", + testName: "Get table names only", query: "?table-names-only", expectedStatus: http.StatusOK, resps: []apis.OVSFlowResponse{{Flow: "table0"}, {Flow: "table1"}}, @@ -368,7 +387,7 @@ func TestGroups(t *testing.T) { }{ { testCase: testCase{ - test: "All groups", + testName: "All groups", query: "?groups=all", expectedStatus: http.StatusOK, resps: testGroupResponses, @@ -377,7 +396,7 @@ func TestGroups(t *testing.T) { }, { testCase: testCase{ - test: "Group 1234", + testName: "Group 1234", query: "?groups=1234", expectedStatus: http.StatusOK, resps: []apis.OVSFlowResponse{{Flow: "group1234"}}, @@ -387,7 +406,7 @@ func TestGroups(t *testing.T) { }, { testCase: testCase{ - test: "Non-existing group 1234", + testName: "Non-existing group 1234", query: "?groups=1234", expectedStatus: http.StatusOK, resps: []apis.OVSFlowResponse{}, @@ -397,7 +416,7 @@ func TestGroups(t *testing.T) { }, { testCase: testCase{ - test: "Group 10, 100, and 1000", + testName: "Group 10, 100, and 1000", query: "?groups=10,100,1000", expectedStatus: http.StatusOK, resps: []apis.OVSFlowResponse{{Flow: "group10"}, {Flow: "group1000"}}, @@ -407,21 +426,22 @@ func TestGroups(t *testing.T) { }, } for _, tc := range testcases { - ovsctl := ovsctltest.NewMockOVSCtlClient(ctrl) - q := aqtest.NewMockAgentQuerier(ctrl) - if tc.groupIDs == nil { - // Get all. - q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(1) - ovsctl.EXPECT().DumpGroups().Return(tc.dumpedGroups, nil).Times(1) - } else { - // Get all. - q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(len(tc.groupIDs)) - for i, id := range tc.groupIDs { - ovsctl.EXPECT().DumpGroup(id).Return(tc.dumpedGroups[i], nil).Times(1) + t.Run(tc.testName, func(t *testing.T) { + ovsctl := ovsctltest.NewMockOVSCtlClient(ctrl) + q := aqtest.NewMockAgentQuerier(ctrl) + if tc.groupIDs == nil { + // Get all. + q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(1) + ovsctl.EXPECT().DumpGroups().Return(tc.dumpedGroups, nil).Times(1) + } else { + // Get all. + q.EXPECT().GetOVSCtlClient().Return(ovsctl).Times(len(tc.groupIDs)) + for i, id := range tc.groupIDs { + ovsctl.EXPECT().DumpGroup(id).Return(tc.dumpedGroups[i], nil).Times(1) + } } - } - - runHTTPTest(t, &tc.testCase, q) + runHTTPTest(t, &tc.testCase, q) + }) } } @@ -432,7 +452,7 @@ func runHTTPTest(t *testing.T, tc *testCase, aq agentquerier.AgentQuerier) { recorder := httptest.NewRecorder() handler.ServeHTTP(recorder, req) - assert.Equal(t, tc.expectedStatus, recorder.Code, tc.test) + assert.Equal(t, tc.expectedStatus, recorder.Code, tc.testName) if tc.expectedStatus == http.StatusOK { var received []apis.OVSFlowResponse diff --git a/pkg/ovs/openflow/utils.go b/pkg/ovs/openflow/utils.go index ded90121e26..d3af3a56877 100644 --- a/pkg/ovs/openflow/utils.go +++ b/pkg/ovs/openflow/utils.go @@ -17,6 +17,7 @@ package openflow import ( "fmt" "net" + "slices" "strings" "k8s.io/klog/v2" @@ -1225,17 +1226,18 @@ func FlowModToString(flowMod *openflow15.FlowMod) string { func FlowModMatchString(flowMod *openflow15.FlowMod, omitFields ...string) string { flowModMatch := getFlowModMatch(flowMod) + if len(omitFields) == 0 { + return fmt.Sprintf("table=%d,%s", flowMod.TableId, flowModMatch) + } var flowDumpMatch []string -out: for _, m := range strings.Split(flowModMatch, ",") { // 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 - } + if !slices.ContainsFunc(omitFields, func(field string) bool { + return strings.HasPrefix(m, field) + }) { + flowDumpMatch = append(flowDumpMatch, m) } - 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 3bbaaaabe9e..e0d52e13610 100644 --- a/pkg/ovs/openflow/utils_test.go +++ b/pkg/ovs/openflow/utils_test.go @@ -143,7 +143,7 @@ func TestFlowModToString(t *testing.T) { newFb := *(basicFB.CopyToBuilder(basicFB.FlowPriority(), false).(*ofFlowBuilder)) f := tt.flowFunc(&newFb) messages, err := f.GetBundleMessages(AddMessage) - assert.NoError(t, err) + require.NoError(t, err) require.Equal(t, 1, len(messages)) fm, ok := messages[0].GetMessage().(*openflow15.FlowMod) assert.True(t, ok) @@ -303,7 +303,7 @@ func TestFlowModMatchString(t *testing.T) { newFb := *(basicFB.CopyToBuilder(basicFB.FlowPriority(), false).(*ofFlowBuilder)) f := tt.flowFunc(&newFb) messages, err := f.GetBundleMessages(AddMessage) - assert.NoError(t, err) + require.NoError(t, err) require.Equal(t, 1, len(messages)) fm, ok := messages[0].GetMessage().(*openflow15.FlowMod) assert.True(t, ok) @@ -378,7 +378,7 @@ func TestFlowModStringForDumpCommand(t *testing.T) { newFb := *(basicFB.CopyToBuilder(basicFB.FlowPriority(), false).(*ofFlowBuilder)) f := tt.flowFunc(&newFb) messages, err := f.GetBundleMessages(AddMessage) - assert.NoError(t, err) + require.NoError(t, err) require.Equal(t, 1, len(messages)) fm, ok := messages[0].GetMessage().(*openflow15.FlowMod) assert.True(t, ok) diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go index a60f5e0fd9e..1994dce0064 100644 --- a/pkg/querier/querier.go +++ b/pkg/querier/querier.go @@ -122,6 +122,15 @@ var NetworkPolicyTypeMap = map[string]cpv1beta.NetworkPolicyType{ "BANP": cpv1beta.BaselineAdminNetworkPolicy, } +func GetNetworkPolicyTypeShorthands() []string { + validTypes, i := make([]string, len(NetworkPolicyTypeMap)), 0 + for k := range NetworkPolicyTypeMap { + validTypes[i] = k + i++ + } + return validTypes +} + var NamespaceScopedPolicyTypes = sets.New[string]("ANNP", "K8SNP") // ServiceExternalIPStatusQuerier queries the Service external IP status for debugging purposes.