From 3335e734071894f70a9ab33c734799f56c049689 Mon Sep 17 00:00:00 2001 From: ceclinux Date: Thu, 22 Apr 2021 22:10:49 +0800 Subject: [PATCH] Fix NetworkPolicy Ingress rule stats miscalculation (#2078) Packets to the tunnel or gateway port will go directly to ConntrackCommitTable and bypass the IngressMetricsTable, which causing these packets uncounted when applying by certain Networkpolicy with ingress rules. Fixed by forwarding these packets to IngressMetricsTable. --- pkg/agent/openflow/pipeline.go | 19 ++++++++++--------- test/e2e/networkpolicy_test.go | 12 ++++++++++++ test/integration/agent/openflow_test.go | 6 +++--- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/pkg/agent/openflow/pipeline.go b/pkg/agent/openflow/pipeline.go index ec29eca1d5b..2ab968c46a4 100644 --- a/pkg/agent/openflow/pipeline.go +++ b/pkg/agent/openflow/pipeline.go @@ -2073,15 +2073,16 @@ func priorityIndexFunc(obj interface{}) ([]string, error) { func (c *client) generatePipeline() { bridge := c.bridge c.pipeline = map[binding.TableIDType]binding.Table{ - ClassifierTable: bridge.CreateTable(ClassifierTable, spoofGuardTable, binding.TableMissActionDrop), - arpResponderTable: bridge.CreateTable(arpResponderTable, binding.LastTableID, binding.TableMissActionDrop), - conntrackTable: bridge.CreateTable(conntrackTable, conntrackStateTable, binding.TableMissActionNone), - EgressRuleTable: bridge.CreateTable(EgressRuleTable, EgressDefaultTable, binding.TableMissActionNext), - EgressDefaultTable: bridge.CreateTable(EgressDefaultTable, EgressMetricTable, binding.TableMissActionNext), - EgressMetricTable: bridge.CreateTable(EgressMetricTable, l3ForwardingTable, binding.TableMissActionNext), - l3ForwardingTable: bridge.CreateTable(l3ForwardingTable, l2ForwardingCalcTable, binding.TableMissActionNext), - l3DecTTLTable: bridge.CreateTable(l3DecTTLTable, l2ForwardingCalcTable, binding.TableMissActionNext), - l2ForwardingCalcTable: bridge.CreateTable(l2ForwardingCalcTable, conntrackCommitTable, binding.TableMissActionNext), + ClassifierTable: bridge.CreateTable(ClassifierTable, spoofGuardTable, binding.TableMissActionDrop), + arpResponderTable: bridge.CreateTable(arpResponderTable, binding.LastTableID, binding.TableMissActionDrop), + conntrackTable: bridge.CreateTable(conntrackTable, conntrackStateTable, binding.TableMissActionNone), + EgressRuleTable: bridge.CreateTable(EgressRuleTable, EgressDefaultTable, binding.TableMissActionNext), + EgressDefaultTable: bridge.CreateTable(EgressDefaultTable, EgressMetricTable, binding.TableMissActionNext), + EgressMetricTable: bridge.CreateTable(EgressMetricTable, l3ForwardingTable, binding.TableMissActionNext), + l3ForwardingTable: bridge.CreateTable(l3ForwardingTable, l2ForwardingCalcTable, binding.TableMissActionNext), + l3DecTTLTable: bridge.CreateTable(l3DecTTLTable, l2ForwardingCalcTable, binding.TableMissActionNext), + // Packets from l2ForwardingCalcTable should be forwarded to IngressMetricTable by default to collect ingress stats. + l2ForwardingCalcTable: bridge.CreateTable(l2ForwardingCalcTable, IngressMetricTable, binding.TableMissActionNext), IngressRuleTable: bridge.CreateTable(IngressRuleTable, IngressDefaultTable, binding.TableMissActionNext), IngressDefaultTable: bridge.CreateTable(IngressDefaultTable, IngressMetricTable, binding.TableMissActionNext), IngressMetricTable: bridge.CreateTable(IngressMetricTable, conntrackCommitTable, binding.TableMissActionNext), diff --git a/test/e2e/networkpolicy_test.go b/test/e2e/networkpolicy_test.go index 6af86a68f77..eeb20ee7506 100644 --- a/test/e2e/networkpolicy_test.go +++ b/test/e2e/networkpolicy_test.go @@ -31,6 +31,7 @@ import ( "github.com/vmware-tanzu/antrea/pkg/agent/apiserver/handlers/agentinfo" "github.com/vmware-tanzu/antrea/pkg/apis/crd/v1beta1" + "github.com/vmware-tanzu/antrea/pkg/apis/stats/v1alpha1" ) func TestNetworkPolicyStats(t *testing.T) { @@ -142,12 +143,23 @@ func TestNetworkPolicyStats(t *testing.T) { } if err := wait.Poll(5*time.Second, defaultTimeout, func() (bool, error) { + var ingressStats *v1alpha1.NetworkPolicyStats for _, np := range []string{"test-networkpolicy-ingress", "test-networkpolicy-egress"} { stats, err := data.crdClient.StatsV1alpha1().NetworkPolicyStats(testNamespace).Get(context.TODO(), np, metav1.GetOptions{}) if err != nil { return false, err } t.Logf("Got NetworkPolicy stats: %v", stats) + if ingressStats != nil { + if stats.TrafficStats.Packets != ingressStats.TrafficStats.Packets { + return false, nil + } + if stats.TrafficStats.Bytes != ingressStats.TrafficStats.Bytes { + return false, nil + } + } else { + ingressStats = stats + } if stats.TrafficStats.Sessions != int64(totalSessions) { return false, nil } diff --git a/test/integration/agent/openflow_test.go b/test/integration/agent/openflow_test.go index f2c987e38db..b368494b3a9 100644 --- a/test/integration/agent/openflow_test.go +++ b/test/integration/agent/openflow_test.go @@ -1016,7 +1016,7 @@ func prepareGatewayFlows(gwIPs []net.IP, gwMAC net.HardwareAddr, vMAC net.Hardwa []*ofTestUtils.ExpectFlow{ { MatchStr: fmt.Sprintf("priority=200,dl_dst=%s", gwMAC.String()), - ActStr: fmt.Sprintf("load:0x%x->NXM_NX_REG1[],load:0x1->NXM_NX_REG0[16],goto_table:105", config1.HostGatewayOFPort), + ActStr: fmt.Sprintf("load:0x%x->NXM_NX_REG1[],load:0x1->NXM_NX_REG0[16],goto_table:101", config1.HostGatewayOFPort), }, }, }, @@ -1097,7 +1097,7 @@ func prepareTunnelFlows(tunnelPort uint32, vMAC net.HardwareAddr) []expectTableF []*ofTestUtils.ExpectFlow{ { MatchStr: fmt.Sprintf("priority=200,dl_dst=%s", vMAC.String()), - ActStr: fmt.Sprintf("load:0x%x->NXM_NX_REG1[],load:0x1->NXM_NX_REG0[16],goto_table:105", config1.DefaultTunOFPort), + ActStr: fmt.Sprintf("load:0x%x->NXM_NX_REG1[],load:0x1->NXM_NX_REG0[16],goto_table:101", config1.DefaultTunOFPort), }, }, }, @@ -1236,7 +1236,7 @@ func prepareDefaultFlows(config *testConfig) []expectTableFlows { }, { uint8(80), - []*ofTestUtils.ExpectFlow{{MatchStr: "priority=0", ActStr: "goto_table:105"}}, + []*ofTestUtils.ExpectFlow{{MatchStr: "priority=0", ActStr: "goto_table:101"}}, }, { uint8(90),