Skip to content

Commit

Permalink
don't create acl if no namespace address sets are selected.
Browse files Browse the repository at this point in the history 
Previously "ip4.dst == {}" match was created, and ovn-controller
throws an error on such acl

Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>
  • Loading branch information
@npinaeva
npinaeva committed Mar 16, 2023
1 parent 7f4b409 commit 54b67a3
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 32 deletions.
27 changes: 27 additions & 0 deletions go-controller/pkg/libovsdbops/acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,33 @@ func FindACLsWithPredicate(nbClient libovsdbclient.Client, p aclPredicate) ([]*n
return acls, err
}

func FindACLs(nbClient libovsdbclient.Client, acls []*nbdb.ACL) ([]*nbdb.ACL, error) {
opModels := make([]operationModel, 0, len(acls))
foundACLs := make([]*nbdb.ACL, 0, len(acls))
for i := range acls {
// can't use i in the predicate, for loop replaces it in-memory
acl := acls[i]
found := []*nbdb.ACL{}
opModel := operationModel{
Model: acl,
ModelPredicate: func(item *nbdb.ACL) bool { return isEquivalentACL(item, acl) },
ExistingResult: &found,
ErrNotFound: false,
BulkOp: false,
DoAfter: func() {
if len(found) > 0 {
foundACLs = append(foundACLs, found[0])
}
},
}
opModels = append(opModels, opModel)
}

modelClient := newModelClient(nbClient)
err := modelClient.Lookup(opModels...)
return foundACLs, err
}

// BuildACL builds an ACL with empty optional properties unset
func BuildACL(name string, direction nbdb.ACLDirection, priority int, match string, action nbdb.ACLAction, meter string,
severity nbdb.ACLSeverity, log bool, externalIds map[string]string, options map[string]string) *nbdb.ACL {
Expand Down
26 changes: 18 additions & 8 deletions go-controller/pkg/ovn/gress_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ func (gp *gressPolicy) addIPBlock(ipblockJSON *knet.IPBlock) {
gp.ipBlock = append(gp.ipBlock, ipblockJSON)
}

// getL3MatchFromAddressSet may return empty string, which means that there are no address sets selected for giver
// gressPolicy at the time, and acl should not be created.
func (gp *gressPolicy) getL3MatchFromAddressSet() string {
v4AddressSets := syncMapToSortedList(gp.peerV4AddressSets)
v6AddressSets := syncMapToSortedList(gp.peerV6AddressSets)
Expand All @@ -141,7 +143,7 @@ func (gp *gressPolicy) getL3MatchFromAddressSet() string {
// At this point there will be address sets in one or both of them.
// Contents in both address sets mean dual stack, else one will be empty because we will only populate
// entries for enabled stacks
if config.IPv4Mode {
if config.IPv4Mode && len(v4AddressSets) > 0 {
v4AddressSetStr := strings.Join(v4AddressSets, ", ")
v4Match = fmt.Sprintf("%s.%s == {%s}", "ip4", direction, v4AddressSetStr)
if gp.policyType == knet.PolicyTypeIngress {
Expand All @@ -150,7 +152,7 @@ func (gp *gressPolicy) getL3MatchFromAddressSet() string {
}
match = v4Match
}
if config.IPv6Mode {
if config.IPv6Mode && len(v6AddressSets) > 0 {
v6AddressSetStr := strings.Join(v6AddressSets, ", ")
v6Match = fmt.Sprintf("%s.%s == {%s}", "ip6", direction, v6AddressSetStr)
if gp.policyType == knet.PolicyTypeIngress {
Expand All @@ -159,7 +161,8 @@ func (gp *gressPolicy) getL3MatchFromAddressSet() string {
}
match = v6Match
}
if config.IPv4Mode && config.IPv6Mode {
// if at least one match is empty in dualstack mode, the non-empty one will be assigned to match
if config.IPv4Mode && config.IPv6Mode && v4Match != "" && v6Match != "" {
match = fmt.Sprintf("(%s || %s)", v4Match, v6Match)
}
return match
Expand Down Expand Up @@ -251,7 +254,8 @@ func (gp *gressPolicy) isEmpty() bool {
// by the parent NetworkPolicy)
// buildLocalPodACLs is safe for concurrent use, since it only uses gressPolicy fields that don't change
// since creation, or are safe for concurrent use like peerVXAddressSets
func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLoggingLevels) []*nbdb.ACL {
func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLoggingLevels) (createdACLs []*nbdb.ACL,
skippedACLs []*nbdb.ACL) {
var lportMatch, match, l3Match string
if gp.policyType == knet.PolicyTypeIngress {
lportMatch = fmt.Sprintf("outport == @%s", portGroupName)
Expand All @@ -272,14 +276,13 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLo
l4Matches = append(l4Matches, l4Match)
}
}
acls := []*nbdb.ACL{}
for _, l4Match := range l4Matches {
if len(gp.ipBlock) > 0 {
// Add ACL allow rule for IPBlock CIDR
cidrMatches := gp.getMatchFromIPBlock(lportMatch, l4Match)
for i, cidrMatch := range cidrMatches {
acl := gp.buildACLAllow(cidrMatch, l4Match, i+1, aclLogging)
acls = append(acls, acl)
createdACLs = append(createdACLs, acl)
}
}
// if there are pod/namespace selector, then allow packets from/to that address_set or
Expand All @@ -297,10 +300,17 @@ func (gp *gressPolicy) buildLocalPodACLs(portGroupName string, aclLogging *ACLLo
match = fmt.Sprintf("%s && %s && %s", l3Match, l4Match, lportMatch)
}
acl := gp.buildACLAllow(match, l4Match, 0, aclLogging)
acls = append(acls, acl)
if l3Match == "" {
// if l3Match is empty, then no address sets are selected for a given gressPolicy.
// fortunately l3 match is not a part of externalIDs, that means that we can find
// the deleted acl without knowing the previous match.
skippedACLs = append(skippedACLs, acl)
} else {
createdACLs = append(createdACLs, acl)
}
}
}
return acls
return
}

func getGressPolicyACLName(ns, policyName string, idx int) string {
Expand Down
17 changes: 14 additions & 3 deletions go-controller/pkg/ovn/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1327,11 +1327,11 @@ func (oc *DefaultNetworkController) addNetworkPolicy(policy *knet.NetworkPolicy)
func (oc *DefaultNetworkController) buildNetworkPolicyACLs(np *networkPolicy, aclLogging *ACLLoggingLevels) []*nbdb.ACL {
acls := []*nbdb.ACL{}
for _, gp := range np.ingressPolicies {
acl := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acl, _ := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acls = append(acls, acl...)
}
for _, gp := range np.egressPolicies {
acl := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acl, _ := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acls = append(acls, acl...)
}

Expand Down Expand Up @@ -1525,7 +1525,7 @@ func (oc *DefaultNetworkController) peerNamespaceUpdate(np *networkPolicy, gp *g
return nil
}
// buildLocalPodACLs is safe for concurrent use, see function comment for details
acls := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
acls, deletedACLs := gp.buildLocalPodACLs(np.portGroupName, aclLogging)
ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, acls...)
if err != nil {
return err
Expand All @@ -1534,6 +1534,17 @@ func (oc *DefaultNetworkController) peerNamespaceUpdate(np *networkPolicy, gp *g
if err != nil {
return err
}
if len(deletedACLs) > 0 {
deletedACLsWithUUID, err := libovsdbops.FindACLs(oc.nbClient, deletedACLs)
if err != nil {
return fmt.Errorf("failed to find deleted acls: %w", err)
}

ops, err = libovsdbops.DeleteACLsFromPortGroupOps(oc.nbClient, ops, np.portGroupName, deletedACLsWithUUID...)
if err != nil {
return err
}
}
_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
return err
}
Expand Down
44 changes: 23 additions & 21 deletions go-controller/pkg/ovn/policy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,22 +220,18 @@ func getGressACLs(i int, namespace, policyName string, peerNamespaces []string,
hashedASName, _ := getNsAddrSetHashNames(nsName)
hashedASNames = append(hashedASNames, hashedASName)
}
hasNsSelector := false
ipBlock := ""
for _, peer := range peers {
if peer.PodSelector != nil && len(peerNamespaces) == 0 {
peerIndex := getPodSelectorAddrSetDbIDs(getPodSelectorKey(peer.PodSelector, peer.NamespaceSelector, namespace), DefaultNetworkControllerName)
asv4, _ := addressset.GetHashNamesForAS(peerIndex)
hashedASNames = append(hashedASNames, asv4)
}
if peer.NamespaceSelector != nil {
hasNsSelector = true
}
if peer.IPBlock != nil {
ipBlock = peer.IPBlock.CIDR
}
}
if len(hashedASNames) > 0 || hasNsSelector {
if len(hashedASNames) > 0 {
gressAsMatch := asMatch(hashedASNames)
match := fmt.Sprintf("ip4.%s == {%s}", ipDir, gressAsMatch)
if policyType == knet.PolicyTypeIngress {
Expand Down Expand Up @@ -1592,8 +1588,12 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
Delete(context.TODO(), namespace2.Name, *metav1.NewDeleteOptions(0))
gomega.Expect(err).NotTo(gomega.HaveOccurred())
expectedData = []libovsdb.TestData{}
gressPolicyExpectedData = getPolicyData(networkPolicy, []string{nPodTest.portUUID}, []string{},
nil, "", false)
// acls will be deleted, since no namespaces are selected at this point.
// since test server doesn't garbage-collect de-referenced acls, they will stay in the db
// gressPolicyExpectedData[2] is the policy port group
gressPolicyExpectedData[2].(*nbdb.PortGroup).ACLs = nil
//gressPolicyExpectedData = getPolicyData(networkPolicy, []string{nPodTest.portUUID}, []string{},
// nil, "", false)
expectedData = append(expectedData, gressPolicyExpectedData...)
expectedData = append(expectedData, defaultDenyExpectedData...)
expectedData = append(expectedData, getExpectedDataPodsAndSwitches([]testPod{nPodTest}, []string{nodeName})...)
Expand Down Expand Up @@ -1632,8 +1632,10 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Operations", func() {
gomega.Expect(err).NotTo(gomega.HaveOccurred())

expectedData = []libovsdb.TestData{}
gressPolicyExpectedData = getPolicyData(networkPolicy, nil, []string{},
nil, "", false)
// acls will be deleted, since no namespaces are selected at this point.
// since test server doesn't garbage-collect de-referenced acls, they will stay in the db
// gressPolicyExpectedData[2] is the policy port group
gressPolicyExpectedData[2].(*nbdb.PortGroup).ACLs = nil
expectedData = append(expectedData, gressPolicyExpectedData...)
expectedData = append(expectedData, defaultDenyExpectedData...)
expectedData = append(expectedData, initialDB.NBData...)
Expand Down Expand Up @@ -2211,20 +2213,20 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(one.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected := buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one}, &defaultAclLogging)
actual := gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ := gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.addNamespaceAddressSet(two.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one, two}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// address sets should be alphabetized
gomega.Expect(gp.addNamespaceAddressSet(three.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one, two, three}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// re-adding an existing set is a no-op
Expand All @@ -2233,14 +2235,14 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(four.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, one, two, three, four}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// now delete a set
gomega.Expect(gp.delNamespaceAddressSet(one.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, three, four}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// deleting again is a no-op
Expand All @@ -2250,13 +2252,13 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(five.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, three, four, five}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(three.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, four, five}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// deleting again is no-op
Expand All @@ -2265,31 +2267,31 @@ var _ = ginkgo.Describe("OVN NetworkPolicy Low-Level Operations", func() {
gomega.Expect(gp.addNamespaceAddressSet(six.GetObjectID(libovsdbops.ObjectNameKey), asFactory)).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, two, four, five, six}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(two.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, four, five, six}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(five.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, four, six}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(six.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs, four}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

gomega.Expect(gp.delNamespaceAddressSet(four.GetObjectID(libovsdbops.ObjectNameKey))).To(gomega.BeTrue())
expected = buildExpectedIngressPeerNSv4ACL(gp, pgName, []*libovsdbops.DbObjectIDs{
asIDs}, &defaultAclLogging)
actual = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
actual, _ = gp.buildLocalPodACLs(pgName, &defaultAclLogging)
gomega.Expect(actual).To(libovsdb.ConsistOfIgnoringUUIDs(expected))

// deleting again is no-op
Expand Down

0 comments on commit 54b67a3

Please sign in to comment.