diff --git a/go-controller/pkg/libovsdb/ops/db_object_types.go b/go-controller/pkg/libovsdb/ops/db_object_types.go index 45c2777637..87102451c7 100644 --- a/go-controller/pkg/libovsdb/ops/db_object_types.go +++ b/go-controller/pkg/libovsdb/ops/db_object_types.go @@ -242,9 +242,10 @@ var ACLNetworkPolicyPortIndex = newObjectIDsType(acl, NetworkPolicyPortIndexOwne // ingress/egress + NetworkPolicy[In/E]gressRule idx - defines given gressPolicy. // ACLs are created for gp.portPolicies which are grouped by protocol: // - for empty policy (no selectors and no ip blocks) - empty ACL (see allIPsMatch) +// with idx=emptyIdx (-1) // OR -// - all selector-based peers ACL -// - for every IPBlock +1 ACL +// - all selector-based peers ACL with idx=emptyIdx (-1) +// - all ipBlocks combined into a single ACL with idx=ipBlockCombinedIdx (-2) // Therefore unique id for a given gressPolicy is protocol name + IPBlock idx // (protocol will be "None" if no port policy is defined, and empty policy and all // selector-based peers ACLs will have idx=-1) diff --git a/go-controller/pkg/ovn/gress_policy.go b/go-controller/pkg/ovn/gress_policy.go index ad20fadfb3..b1f844123c 100644 --- a/go-controller/pkg/ovn/gress_policy.go +++ b/go-controller/pkg/ovn/gress_policy.go @@ -22,6 +22,11 @@ import ( const ( // emptyIdx is used to create ACL for gressPolicy that doesn't have ipBlocks emptyIdx = -1 + // ipBlockCombinedIdx is used when creating an ACL for a gressPolicy + // that contains ipBlocks. Previously, one ACL was created per ipBlock. + // This is changed to create a single combined ACL for all ipBlocks, + // and this special index value identifies those new ACLs. + ipBlockCombinedIdx = -2 ) type gressPolicy struct { @@ -167,14 +172,14 @@ func (gp *gressPolicy) allIPsMatch() string { } } -func (gp *gressPolicy) getMatchFromIPBlock(lportMatch, l4Match string) []string { +func (gp *gressPolicy) getMatchFromIPBlock(lportMatch, l4Match string) string { var direction string if gp.policyType == knet.PolicyTypeIngress { direction = "src" } else { direction = "dst" } - var matchStrings []string + var ipBlockMatches []string var matchStr, ipVersion string for _, ipBlock := range gp.ipBlocks { if utilnet.IsIPv6CIDRString(ipBlock.CIDR) { @@ -185,17 +190,22 @@ func (gp *gressPolicy) getMatchFromIPBlock(lportMatch, l4Match string) []string if len(ipBlock.Except) == 0 { matchStr = fmt.Sprintf("%s.%s == %s", ipVersion, direction, ipBlock.CIDR) } else { - matchStr = fmt.Sprintf("%s.%s == %s && %s.%s != {%s}", ipVersion, direction, ipBlock.CIDR, + matchStr = fmt.Sprintf("(%s.%s == %s && %s.%s != {%s})", ipVersion, direction, ipBlock.CIDR, ipVersion, direction, strings.Join(ipBlock.Except, ", ")) } - if l4Match == libovsdbutil.UnspecifiedL4Match { - matchStr = fmt.Sprintf("%s && %s", matchStr, lportMatch) - } else { - matchStr = fmt.Sprintf("%s && %s && %s", matchStr, l4Match, lportMatch) - } - matchStrings = append(matchStrings, matchStr) + ipBlockMatches = append(ipBlockMatches, matchStr) } - return matchStrings + var l3Match string + if len(ipBlockMatches) == 1 { + l3Match = ipBlockMatches[0] + } else { + l3Match = fmt.Sprintf("(%s)", strings.Join(ipBlockMatches, " || ")) + } + + if l4Match == libovsdbutil.UnspecifiedL4Match { + return fmt.Sprintf("%s && %s", l3Match, lportMatch) + } + return fmt.Sprintf("%s && %s && %s", l3Match, l4Match, lportMatch) } // addNamespaceAddressSet adds a namespace address set to the gress policy. @@ -285,13 +295,11 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *libov for protocol, l4Match := range libovsdbutil.GetL4MatchesFromNetworkPolicyPorts(gp.portPolicies) { if len(gp.ipBlocks) > 0 { // Add ACL allow rule for IPBlock CIDR - ipBlockMatches := gp.getMatchFromIPBlock(lportMatch, l4Match) - for ipBlockIdx, ipBlockMatch := range ipBlockMatches { - aclIDs := gp.getNetpolACLDbIDs(ipBlockIdx, protocol) - acl := libovsdbutil.BuildACLWithDefaultTier(aclIDs, types.DefaultAllowPriority, ipBlockMatch, action, - aclLogging, gp.aclPipeline) - createdACLs = append(createdACLs, acl) - } + ipBlockMatch := gp.getMatchFromIPBlock(lportMatch, l4Match) + aclIDs := gp.getNetpolACLDbIDs(ipBlockCombinedIdx, protocol) + acl := libovsdbutil.BuildACLWithDefaultTier(aclIDs, types.DefaultAllowPriority, ipBlockMatch, action, + aclLogging, gp.aclPipeline) + createdACLs = append(createdACLs, acl) } // if there are pod/namespace selector, then allow packets from/to that address_set or // if the NetworkPolicyPeer is empty, then allow from all sources or to all destinations. @@ -334,10 +342,10 @@ func (gp *gressPolicy) getNetpolACLDbIDs(ipBlockIdx int, protocol string) *libov // gress rule index libovsdbops.GressIdxKey: strconv.Itoa(gp.idx), // acls are created for every gp.portPolicies which are grouped by protocol: - // - for empty policy (no selectors and no ip blocks) - empty ACL + // - for empty policy (no selectors and no ip blocks) - empty ACL with idx=emptyIdx (-1) // OR - // - all selector-based peers ACL - // - for every IPBlock +1 ACL + // - all selector-based peers ACL with idx=emptyIdx (-1) + // - all ipBlocks combined into a single ACL with idx=ipBlockCombinedIdx (-2) // Therefore unique id for a given gressPolicy is protocol name + IPBlock idx // (protocol will be "None" if no port policy is defined, and empty policy and all // selector-based peers ACLs will have idx=-1) diff --git a/go-controller/pkg/ovn/gress_policy_test.go b/go-controller/pkg/ovn/gress_policy_test.go index 14b2a65a7c..f45be5385a 100644 --- a/go-controller/pkg/ovn/gress_policy_test.go +++ b/go-controller/pkg/ovn/gress_policy_test.go @@ -16,7 +16,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { ipBlocks []*knet.IPBlock lportMatch string l4Match string - expected []string + expected string }{ { desc: "IPv4 only no except", @@ -27,7 +27,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip4.src == 0.0.0.0/0 && input && fake"}, + expected: "ip4.src == 0.0.0.0/0 && input && fake", }, { desc: "multiple IPv4 only no except", @@ -41,8 +41,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip4.src == 0.0.0.0/0 && input && fake", - "ip4.src == 10.1.0.0/16 && input && fake"}, + expected: "(ip4.src == 0.0.0.0/0 || ip4.src == 10.1.0.0/16) && input && fake", }, { desc: "IPv6 only no except", @@ -53,7 +52,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip6.src == fd00:10:244:3::49/32 && input && fake"}, + expected: "ip6.src == fd00:10:244:3::49/32 && input && fake", }, { desc: "mixed IPv4 and IPv6 no except", @@ -67,8 +66,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip6.src == ::/0 && input && fake", - "ip4.src == 0.0.0.0/0 && input && fake"}, + expected: "(ip6.src == ::/0 || ip4.src == 0.0.0.0/0) && input && fake", }, { desc: "IPv4 only with except", @@ -80,7 +78,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16} && input && fake"}, + expected: "(ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16}) && input && fake", }, { desc: "multiple IPv4 with except", @@ -95,8 +93,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16} && input && fake", - "ip4.src == 10.1.0.0/16 && input && fake"}, + expected: "((ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16}) || ip4.src == 10.1.0.0/16) && input && fake", }, { desc: "IPv4 with IPv4 except", @@ -108,7 +105,7 @@ func TestGetMatchFromIPBlock(t *testing.T) { }, lportMatch: "fake", l4Match: "input", - expected: []string{"ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16} && input && fake"}, + expected: "(ip4.src == 0.0.0.0/0 && ip4.src != {10.1.0.0/16}) && input && fake", }, } diff --git a/go-controller/pkg/ovn/policy_test.go b/go-controller/pkg/ovn/policy_test.go index c02af4575f..af7923a5cf 100644 --- a/go-controller/pkg/ovn/policy_test.go +++ b/go-controller/pkg/ovn/policy_test.go @@ -6,6 +6,7 @@ import ( "net" "runtime" "sort" + "strings" "time" "github.com/onsi/ginkgo/v2" @@ -291,10 +292,24 @@ func getGressACLs(gressIdx int, peers []knet.NetworkPolicyPeer, policyType knet. acl.UUID = dbIDs.String() + "-UUID" acls = append(acls, acl) } - for i, ipBlock := range ipBlocks { - match := fmt.Sprintf("ip4.%s == %s && %s == @%s", ipDir, ipBlock, portDir, pgName) + if len(ipBlocks) > 0 { + var ipBlockMatches []string + for _, ipBlock := range ipBlocks { + ipVersion := "ip4" + if utilnet.IsIPv6CIDRString(ipBlock) { + ipVersion = "ip6" + } + ipBlockMatches = append(ipBlockMatches, fmt.Sprintf("%s.%s == %s", ipVersion, ipDir, ipBlock)) + } + var match string + if len(ipBlockMatches) == 1 { + match = ipBlockMatches[0] + } else { + match = fmt.Sprintf("(%s)", strings.Join(ipBlockMatches, " || ")) + } + match = fmt.Sprintf("%s && %s == @%s", match, portDir, pgName) action := allowAction(params.statelessNetPol) - dbIDs := gp.getNetpolACLDbIDs(i, libovsdbutil.UnspecifiedL4Protocol) + dbIDs := gp.getNetpolACLDbIDs(ipBlockCombinedIdx, libovsdbutil.UnspecifiedL4Protocol) acl := libovsdbops.BuildACL( libovsdbutil.GetACLName(dbIDs), direction, @@ -361,6 +376,17 @@ func getPolicyData(params *netpolDataParams) []libovsdbtest.TestData { acls = append(acls, getGressACLs(i, egress.To, knet.PolicyTypeEgress, params)...) } + pg := getPolicyPortGroup(params, acls) + + data := []libovsdbtest.TestData{} + for _, acl := range acls { + data = append(data, acl) + } + data = append(data, pg) + return data +} + +func getPolicyPortGroup(params *netpolDataParams, acls []*nbdb.ACL) *nbdb.PortGroup { lsps := []*nbdb.LogicalSwitchPort{} for _, uuid := range params.localPortUUIDs { lsps = append(lsps, &nbdb.LogicalSwitchPort{UUID: uuid}) @@ -375,12 +401,7 @@ func getPolicyData(params *netpolDataParams) []libovsdbtest.TestData { ) pg.UUID = pg.Name + "-UUID" - data := []libovsdbtest.TestData{} - for _, acl := range acls { - data = append(data, acl) - } - data = append(data, pg) - return data + return pg } func newNetpolDataParams(networkPolicy *knet.NetworkPolicy) *netpolDataParams { @@ -956,6 +977,149 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() { } gomega.Expect(app.Run([]string{app.Name})).To(gomega.Succeed()) }) + + ginkgo.It("reconciles existing networkPolicies with has legacy ipBlock ACLs", func() { + app.Action = func(*cli.Context) error { + namespace1 := *newNamespace(namespaceName1) + namespace1AddressSetv4, _ := buildNamespaceAddressSets(namespace1.Name, nil) + peer := knet.NetworkPolicyPeer{ + IPBlock: &knet.IPBlock{ + CIDR: "1.1.1.1", + }, + } + // equivalent rules in one peer + networkPolicy1 := newNetworkPolicy(netPolicyName1, namespace1.Name, metav1.LabelSelector{}, + []knet.NetworkPolicyIngressRule{{ + From: []knet.NetworkPolicyPeer{peer, peer}, + }}, nil) + // equivalent rules in different peers + networkPolicy2 := newNetworkPolicy(netPolicyName2, namespace1.Name, metav1.LabelSelector{}, + []knet.NetworkPolicyIngressRule{ + { + From: []knet.NetworkPolicyPeer{peer}, + }, + { + From: []knet.NetworkPolicyPeer{peer}, + }, + }, nil) + initialData := initialDB.NBData + initialData = append(initialData, namespace1AddressSetv4) + defaultDenyExpectedData := getDefaultDenyDataMultiplePolicies([]*knet.NetworkPolicy{networkPolicy1, networkPolicy2}) + initialData = append(initialData, defaultDenyExpectedData...) + + // NetworkPolicy 1 contains a single gress policy that previously + // created one legacy ACL per ipBlock. Simulate two legacy ACLs + // corresponding to ipBlock indexes 0 and 1 of the gress policy. + // ACL1 => libovsdbops.GressIdxKey: 0, libovsdbops.IpBlockIndexKey: 0 + // ACL2 => libovsdbops.GressIdxKey: 0, libovsdbops.IpBlockIndexKey: 1 + netInfo := &util.DefaultNetInfo{} + fakeController := getFakeBaseController(netInfo) + controllerName := getNetworkControllerName(netInfo.GetNetworkName()) + pgName1 := fakeController.getNetworkPolicyPGName(namespace1.Name, networkPolicy1.Name) + gp1 := gressPolicy{ + policyNamespace: networkPolicy1.Namespace, + policyName: networkPolicy1.Name, + policyType: knet.PolicyTypeIngress, + idx: 0, + controllerName: controllerName, + } + var legacyACLPolicy1 []*nbdb.ACL + for idx := 0; idx < 2; idx++ { + legacyACLIDs := gp1.getNetpolACLDbIDs(idx, libovsdbutil.UnspecifiedL4Protocol) + legacyACL := libovsdbops.BuildACL( + libovsdbutil.GetACLName(legacyACLIDs), + nbdb.ACLDirectionToLport, + types.DefaultAllowPriority, + fmt.Sprintf("ip4.src == 1.1.1.1 && outport == @%s", pgName1), + nbdb.ACLActionAllowRelated, + types.OvnACLLoggingMeter, + "", + false, + legacyACLIDs.GetExternalIDs(), + nil, + types.DefaultACLTier, + ) + legacyACL.UUID = legacyACLIDs.String() + "-UUID" + initialData = append(initialData, legacyACL) + legacyACLPolicy1 = append(legacyACLPolicy1, legacyACL) + } + pgNetworkPolicy1 := getPolicyPortGroup(newNetpolDataParams(networkPolicy1), legacyACLPolicy1) + initialData = append(initialData, pgNetworkPolicy1) + + // NetworkPolicy 2 contains two gress policies, each with one legacy + // ACL per ipBlock. Simulate two legacy ACL corresponding to gress + // policy indexes 0 and 1, respectively. + // ACL1 => libovsdbops.GressIdxKey: 0, libovsdbops.IpBlockIndexKey: 0 + // ACL2 => libovsdbops.GressIdxKey: 1, libovsdbops.IpBlockIndexKey: 0 + pgName2 := fakeController.getNetworkPolicyPGName(namespace1.Name, networkPolicy2.Name) + firstgp2 := gressPolicy{ + policyNamespace: networkPolicy2.Namespace, + policyName: networkPolicy2.Name, + policyType: knet.PolicyTypeIngress, + idx: 0, + controllerName: controllerName, + } + secondgp2 := gressPolicy{ + policyNamespace: networkPolicy2.Namespace, + policyName: networkPolicy2.Name, + policyType: knet.PolicyTypeIngress, + idx: 1, + controllerName: controllerName, + } + legacyACLID := firstgp2.getNetpolACLDbIDs(0, libovsdbutil.UnspecifiedL4Protocol) + legacyACL := libovsdbops.BuildACL( + libovsdbutil.GetACLName(legacyACLID), + nbdb.ACLDirectionToLport, + types.DefaultAllowPriority, + fmt.Sprintf("ip4.src == 1.1.1.1 && outport == @%s", pgName2), + nbdb.ACLActionAllowRelated, + types.OvnACLLoggingMeter, + "", + false, + legacyACLID.GetExternalIDs(), + nil, + types.DefaultACLTier, + ) + legacyACL.UUID = legacyACLID.String() + "-UUID" + initialData = append(initialData, legacyACL) + + legacyACLID2 := secondgp2.getNetpolACLDbIDs(0, libovsdbutil.UnspecifiedL4Protocol) + legacyACL2 := libovsdbops.BuildACL( + libovsdbutil.GetACLName(legacyACLID2), + nbdb.ACLDirectionToLport, + types.DefaultAllowPriority, + fmt.Sprintf("ip4.src == 1.1.1.1 && outport == @%s", pgName2), + nbdb.ACLActionAllowRelated, + types.OvnACLLoggingMeter, + "", + false, + legacyACLID2.GetExternalIDs(), + nil, + types.DefaultACLTier, + ) + legacyACL2.UUID = legacyACLID2.String() + "-UUID" + initialData = append(initialData, legacyACL2) + pgNetworkPolicy2 := getPolicyPortGroup(newNetpolDataParams(networkPolicy2), []*nbdb.ACL{legacyACL, legacyACL2}) + initialData = append(initialData, pgNetworkPolicy2) + + startOvn(libovsdbtest.TestSetup{NBData: initialData}, []corev1.Namespace{namespace1}, + []knet.NetworkPolicy{*networkPolicy1, *networkPolicy2}, + nil, nil) + + // check the initial data is updated and all legacy ACLs should be cleaned up + gressPolicy1ExpectedData := getPolicyData(newNetpolDataParams(networkPolicy1)) + gressPolicy2ExpectedData := getPolicyData(newNetpolDataParams(networkPolicy2)) + finalData := initialDB.NBData + finalData = append(finalData, namespace1AddressSetv4) + finalData = append(finalData, gressPolicy1ExpectedData...) + finalData = append(finalData, gressPolicy2ExpectedData...) + finalData = append(finalData, defaultDenyExpectedData...) + gomega.Eventually(fakeOvn.nbClient).Should(libovsdbtest.HaveData(finalData)) + + return nil + } + gomega.Expect(app.Run([]string{app.Name})).To(gomega.Succeed()) + }) }) ginkgo.Context("during execution", func() {