Skip to content

Commit

Permalink
Don't delete equivalent ACLs by predicate, since it will fail if
Browse files Browse the repository at this point in the history 
equivalent ACL is not deleted from port group/switch.
e.g. on netpol sync, users (somehow) ended up with equivalent
default deny ACLs with different names, and extra ACL wasn't deleted.

Remove DeleteACLs and DeleteACLsOps functions, use
DeleteACLsFromPortGroups and RemoveACLsFromLogicalSwitchesWithPredicate
instead.
Hack unit tests, since we don't explicitly delete ACLs anymore, they
will be garbage collected by the ovsdb, but not by our unit test db.
Therefore, add dereferenced ACLs to the expected db state, remove that
when test server learns to garbage collect acls.

Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>
(cherry picked from commit d4604ec)
(cherry picked from commit e074706)

Conflits:
	go-controller/pkg/ovn/egressfirewal_test.go: Updated to expect not garbage-collected ACLs
  • Loading branch information
@npinaeva
npinaeva committed Dec 13, 2022
1 parent 54d844f commit eca2110
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 80 deletions.
49 changes: 0 additions & 49 deletions go-controller/pkg/libovsdbops/acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package libovsdbops

import (
"context"
"errors"
"fmt"
"reflect"

Expand Down Expand Up @@ -169,51 +168,3 @@ func UpdateACLsDirection(nbClient libovsdbclient.Client, acls ...*nbdb.ACL) erro
_, err := modelClient.CreateOrUpdate(opModels...)
return err
}

// DeleteACLsOps deletes the provided ACLs and returns the corresponding ops
// portGroupNames and switchPred reminds to delete ACL references for port groups or switches
// in case port group or switch is not completely deleted
func DeleteACLsOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation,
portGroupNames []string, switchPred switchPredicate, acls ...*nbdb.ACL) ([]libovsdb.Operation, error) {
var err error
for _, portGroupName := range portGroupNames {
ops, err = DeleteACLsFromPortGroupOps(nbClient, ops, portGroupName, acls...)
if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {
return ops, fmt.Errorf("deleting ACLs from port group %s failed: %v", portGroupName, err)
}
}
if switchPred != nil {
ops, err = RemoveACLsFromLogicalSwitchesWithPredicateOps(nbClient, ops, switchPred, acls...)
if err != nil && !errors.Is(err, libovsdbclient.ErrNotFound) {
return ops, fmt.Errorf("deleting ACLs from switch with predicate failed: %v", err)
}
}
opModels := make([]operationModel, 0, len(acls))
for i := range acls {
// can't use i in the predicate, for loop replaces it in-memory
acl := acls[i]
opModel := operationModel{
Model: acl,
ModelPredicate: func(item *nbdb.ACL) bool { return isEquivalentACL(item, acl) },
ErrNotFound: false,
BulkOp: true,
}
opModels = append(opModels, opModel)
}

modelClient := newModelClient(nbClient)
return modelClient.DeleteOps(ops, opModels...)
}

// DeleteACLs deletes the provided ACLs
// portGroupNames and switchPred reminds to delete ACL references for port groups or switches
// in case port group or switch is not completely deleted
func DeleteACLs(nbClient libovsdbclient.Client, portGroupNames []string, switchPred switchPredicate, acls ...*nbdb.ACL) error {
ops, err := DeleteACLsOps(nbClient, nil, portGroupNames, switchPred, acls...)
if err != nil {
return err
}

_, err = TransactAndCheck(nbClient, ops)
return err
}
13 changes: 13 additions & 0 deletions go-controller/pkg/libovsdbops/portgroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,19 @@ func DeleteACLsFromPortGroupOps(nbClient libovsdbclient.Client, ops []libovsdb.O
return m.DeleteOps(ops, opModel)
}

func DeleteACLsFromPortGroups(nbClient libovsdbclient.Client, names []string, acls ...*nbdb.ACL) error {
var err error
var ops []libovsdb.Operation
for _, pgName := range names {
ops, err = DeleteACLsFromPortGroupOps(nbClient, ops, pgName, acls...)
if err != nil {
return err
}
}
_, err = TransactAndCheck(nbClient, ops)
return err
}

// DeletePortGroupsOps deletes the provided port groups and returns the
// corresponding ops
func DeletePortGroupsOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation, names ...string) ([]libovsdb.Operation, error) {
Expand Down
2 changes: 1 addition & 1 deletion go-controller/pkg/ovn/egressfirewall.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ func (oc *Controller) deleteEgressFirewallRules(externalID string) error {
// delete egress firewall acls off any logical switch which has it,
// then manually remove the egressFirewall ACLs instead of relying on ovsdb garbage collection to do so
pSwitch := func(item *nbdb.LogicalSwitch) bool { return true }
err = libovsdbops.DeleteACLs(oc.nbClient, nil, pSwitch, egressFirewallACLs...)
err = libovsdbops.RemoveACLsFromLogicalSwitchesWithPredicate(oc.nbClient, pSwitch, egressFirewallACLs...)
if err != nil {
return err
}
Expand Down
74 changes: 72 additions & 2 deletions go-controller/pkg/ovn/egressfirewall_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,13 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
// Direction of both ACLs will be converted to
keepACL.Direction = t.DirectionToLPort

// purgeACL ACL will be deleted when test server starts deleting dereferenced ACLs
// for now we need to update its fields, since it is present in the db
purgeACL.Direction = t.DirectionToLPort

expectedDatabaseState := []libovsdb.TestData{
otherACL,
purgeACL,
keepACL,
finalNodeSwitch,
finalJoinSwitch,
Expand Down Expand Up @@ -574,6 +579,8 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
nodeSwitch1.ACLs = []string{}
nodeSwitch2.ACLs = []string{}
expectedDatabaseState = []libovsdb.TestData{
// this ACL will be deleted when test server starts deleting dereferenced ACLs
ipv4ACL,
nodeSwitch1,
nodeSwitch2,
clusterRouter,
Expand Down Expand Up @@ -684,8 +691,25 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
_, err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall1.Namespace).Update(context.TODO(), egressFirewall1, metav1.UpdateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred())

ipv4ACL.Action = nbdb.ACLActionDrop
// egress firewall is updated by deleting and creating from scratch.
// since old acl won't be garbage-collected by the test server, it will stay,
// but won't be referenced from the switch
ipv4ACLStale := libovsdbops.BuildACL(
"",
t.DirectionToLPort,
t.EgressFirewallStartPriority,
"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14",
nbdb.ACLActionAllow,
"",
"",
false,
map[string]string{"egressFirewall": "namespace1"},
nil,
)
ipv4ACLStale.UUID = "ipv4ACLStale-UUID"

ipv4ACL.Action = nbdb.ACLActionDrop
expectedDatabaseState = append(expectedDatabaseState, ipv4ACLStale)
gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))

return nil
Expand Down Expand Up @@ -818,6 +842,8 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
nodeSwitch1.ACLs = []string{}
nodeSwitch2.ACLs = []string{}
expectedDatabaseState = []libovsdb.TestData{
// this ACL will be deleted when test server starts deleting dereferenced ACLs
ipv4ACL,
nodeSwitch1,
nodeSwitch2,
clusterRouter,
Expand Down Expand Up @@ -956,7 +982,27 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode",
gomega.Eventually(func() *retryObjEntry {
return fakeOVN.controller.retryEgressFirewalls.getObjRetryEntry(key)
}).Should(gomega.BeNil())

// egress firewall is updated by deleting and creating from scratch.
// since old acl won't be garbage-collected by the test server, it will stay,
// but won't be referenced from the switch
ipv4ACLStale := libovsdbops.BuildACL(
"",
t.DirectionToLPort,
t.EgressFirewallStartPriority,
"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14",
nbdb.ACLActionAllow,
"",
"",
false,
map[string]string{"egressFirewall": "namespace1"},
nil,
)
ipv4ACLStale.UUID = "ipv4ACLStale-UUID"

ipv4ACL.Action = nbdb.ACLActionDrop
expectedDatabaseState = append(expectedDatabaseState, ipv4ACLStale)

gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
return nil
}
Expand Down Expand Up @@ -1102,8 +1148,13 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
// Direction of both ACLs will be converted to
keepACL.Direction = t.DirectionToLPort

// purgeACL ACL will be deleted when test server starts deleting dereferenced ACLs
// for now we need to update its fields, since it is present in the db
purgeACL.Direction = t.DirectionToLPort

expectedDatabaseState := []libovsdb.TestData{
otherACL,
purgeACL,
keepACL,
finalNodeSwitch,
finalJoinSwitch,
Expand Down Expand Up @@ -1465,7 +1516,9 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
gomega.Expect(err).NotTo(gomega.HaveOccurred())

// join switch should return to orignal state, egfw was deleted
gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(fakeOVN.dbSetup.NBData))
gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(append(fakeOVN.dbSetup.NBData,
// this ACL will be deleted when test server starts deleting dereferenced ACLs
ipv4ACL)))

return nil
}
Expand Down Expand Up @@ -1556,8 +1609,25 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode",
gomega.Expect(err).NotTo(gomega.HaveOccurred())
_, err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall1.Namespace).Update(context.TODO(), egressFirewall1, metav1.UpdateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
// egress firewall is updated by deleting and creating from scratch
// since old acl won't be garbage-collected by the test server, it will stay,
// but won't be referenced from the switch
ipv4ACLStale := libovsdbops.BuildACL(
"",
t.DirectionToLPort,
t.EgressFirewallStartPriority,
"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \""+t.JoinSwitchToGWRouterPrefix+t.OVNClusterRouter+"\"",
nbdb.ACLActionAllow,
"",
"",
false,
map[string]string{"egressFirewall": "namespace1"},
nil,
)
ipv4ACLStale.UUID = "ipv4ACLStale-UUID"

ipv4ACL.Action = nbdb.ACLActionDrop
expectedDatabaseState = append(expectedDatabaseState, ipv4ACLStale)

gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))

Expand Down
17 changes: 3 additions & 14 deletions go-controller/pkg/ovn/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ func (oc *Controller) updateStaleDefaultDenyACLNames(npType knet.PolicyType, gre
// this should never be the case but delete everything except 1st ACL
ingressPGName := defaultDenyPortGroupName(namespace, ingressDefaultDenySuffix)
egressPGName := defaultDenyPortGroupName(namespace, egressDefaultDenySuffix)
err := libovsdbops.DeleteACLs(oc.nbClient, []string{ingressPGName, egressPGName}, nil, aclList[1:]...)
err := libovsdbops.DeleteACLsFromPortGroups(oc.nbClient, []string{ingressPGName, egressPGName}, aclList[1:]...)
if err != nil {
return err
}
Expand Down Expand Up @@ -401,7 +401,7 @@ func (oc *Controller) syncNetworkPolicies(networkPolicies []interface{}) error {
pgName = strings.TrimPrefix(gressACL.Match, "outport == @")
}
pgName = strings.TrimSuffix(pgName, " && "+staleArpAllowPolicyMatch)
ops, err = libovsdbops.DeleteACLsOps(oc.nbClient, ops, []string{pgName}, nil, gressACL)
ops, err = libovsdbops.DeleteACLsFromPortGroupOps(oc.nbClient, ops, pgName, gressACL)
if err != nil {
return fmt.Errorf("failed getting delete acl ops: %v", err)
}
Expand Down Expand Up @@ -576,25 +576,14 @@ func (oc *Controller) createDefaultDenyPGAndACLs(namespace, policy string, aclLo
// deleteDefaultDenyPGAndACLs deletes the default port groups and acls for a namespace
// must be called with defaultDenyPortGroups lock
func (oc *Controller) deleteDefaultDenyPGAndACLs(namespace, policy string) error {
var aclsToBeDeleted []*nbdb.ACL

ingressPGName := defaultDenyPortGroupName(namespace, ingressDefaultDenySuffix)
ingressDenyACL, ingressAllowACL := buildDenyACLs(namespace, ingressPGName, nil, lportIngress)
aclsToBeDeleted = append(aclsToBeDeleted, ingressDenyACL, ingressAllowACL)
egressPGName := defaultDenyPortGroupName(namespace, egressDefaultDenySuffix)
egressDenyACL, egressAllowACL := buildDenyACLs(namespace, egressPGName, nil, lportEgressAfterLB)
aclsToBeDeleted = append(aclsToBeDeleted, egressDenyACL, egressAllowACL)

ops, err := libovsdbops.DeletePortGroupsOps(oc.nbClient, nil, ingressPGName, egressPGName)
if err != nil {
return err
}
// Manually remove the default ACLs instead of relying on ovsdb garbage collection to do so
// don't delete ACL references because port group is completely deleted in the same tnx
ops, err = libovsdbops.DeleteACLsOps(oc.nbClient, ops, nil, nil, aclsToBeDeleted...)
if err != nil {
return err
}
// No need to delete ACLs, since they will be garbage collected with deleted port groups
_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
if err != nil {
return fmt.Errorf("failed to transact deleteDefaultDenyPGAndACLs: %v", err)
Expand Down

0 comments on commit eca2110

Please sign in to comment.