Skip to content

Commit

Permalink
Merge pull request #1670 from npinaeva/ocpbugs-12729
Browse files Browse the repository at this point in the history
Bug OCPBUGS-12792: [release-4.10] Delete equivalent ACLs when searching by predicate.
  • Loading branch information
openshift-merge-robot committed Jun 1, 2023
2 parents 7215581 + ad26eee commit 90754d1
Show file tree
Hide file tree
Showing 3 changed files with 219 additions and 6 deletions.
33 changes: 29 additions & 4 deletions go-controller/pkg/libovsdbops/acl.go
Expand Up @@ -10,6 +10,8 @@ import (

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"

"k8s.io/klog/v2"
)

// getACLName returns the ACL name if it has one otherwise returns
Expand Down Expand Up @@ -69,14 +71,17 @@ func findACL(nbClient libovsdbclient.Client, acl *nbdb.ACL) error {
return fmt.Errorf("can't find ACL by equivalence %+v: %v", *acl, err)
}

if len(acls) > 1 {
return fmt.Errorf("unexpectedly found multiple equivalent ACLs: %+v", acls)
}

if len(acls) == 0 {
return libovsdbclient.ErrNotFound
}

if len(acls) > 1 {
err = deleteDuplicateACLs(nbClient, acls)
if err != nil {
return fmt.Errorf("unexpectedly found multiple equivalent ACLs, failed to delete: %w", err)
}
}

acl.UUID = acls[0].UUID
return nil
}
Expand Down Expand Up @@ -249,6 +254,11 @@ func DeleteACLs(nbClient libovsdbclient.Client, acls []nbdb.ACL) error {
opModels = append(opModels, OperationModel{
Model: &acl,
ModelPredicate: func(item *nbdb.ACL) bool { return IsEquivalentACL(item, &acl) },
// don't error on equivalent acls
// DeleteACLs will always be a noop, because deleted ACLs should be de-referenced in another transaction,
// and then they are already garbage-collected, or deleting ACL without dereferencing will return error.
// DeleteACLs has only effect for unit tests.
BulkOp: true,
})
}

Expand All @@ -270,3 +280,18 @@ func UpdateACLLogging(nbClient libovsdbclient.Client, acl *nbdb.ACL) error {
_, err = TransactAndCheck(nbClient, ops)
return err
}

func deleteDuplicateACLs(nbClient libovsdbclient.Client, duplicateACLs []nbdb.ACL) error {
if len(duplicateACLs) > 1 {
klog.Warningf("Found multiple acls equivalent to %+v, cleanup", duplicateACLs[0])
err := DeleteACLsFromAllPortGroups(nbClient, duplicateACLs[1:]...)
if err != nil {
return fmt.Errorf("failed to delete duplicate acls: delete from port groups failed: %w", err)
}
err = RemoveACLsFromAllSwitches(nbClient, duplicateACLs[1:])
if err != nil {
return fmt.Errorf("failed to delete duplicate acls: delete from switches failed: %w", err)
}
}
return nil
}
159 changes: 159 additions & 0 deletions go-controller/pkg/libovsdbops/acl_test.go
@@ -0,0 +1,159 @@
package libovsdbops

import (
"fmt"
libovsdbclient "github.com/ovn-org/libovsdb/client"
"sync/atomic"
"testing"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
)

func buildNamedUUID() string {
return fmt.Sprintf("%c%010d", namedUUIDPrefix, atomic.AddUint32(&namedUUIDCounter, 1))
}

func TestCleanupEquivalentACLs(t *testing.T) {
aclName := "acl1"
anotherACLName := "acl2"
aclSev := nbdb.ACLSeverityInfo
aclMeter := types.OvnACLLoggingMeter
initialACL := &nbdb.ACL{
UUID: buildNamedUUID(),
Action: nbdb.ACLActionAllow,
Direction: nbdb.ACLDirectionToLport,
ExternalIDs: map[string]string{"key": "value"},
Log: true,
Match: "match",
Meter: &aclMeter,
Name: &aclName,
Options: nil,
Priority: 1,
Severity: &aclSev,
}
sameNameExitIDsACL := &nbdb.ACL{
UUID: buildNamedUUID(),
Action: nbdb.ACLActionAllow,
Direction: nbdb.ACLDirectionToLport,
ExternalIDs: map[string]string{"key": "value"},
Log: true,
Match: "match1",
Meter: &aclMeter,
Name: &aclName,
Options: map[string]string{"key": "value"},
Priority: 2,
Severity: &aclSev,
}
same4FieldACL := &nbdb.ACL{
UUID: buildNamedUUID(),
Action: nbdb.ACLActionAllow,
Direction: nbdb.ACLDirectionToLport,
ExternalIDs: map[string]string{"key1": "value"},
Log: true,
Match: "match",
Meter: &aclMeter,
Name: &anotherACLName,
Options: nil,
Priority: 1,
Severity: &aclSev,
}

pg := &nbdb.PortGroup{
Name: "testPG",
ACLs: []string{initialACL.UUID, sameNameExitIDsACL.UUID, same4FieldACL.UUID},
}

initialDB := []libovsdbtest.TestData{
initialACL,
sameNameExitIDsACL,
same4FieldACL,
pg,
}

// searchACL is initialACL without UUID, because we need to use predicate search
searchACL := nbdb.ACL{
Action: nbdb.ACLActionAllow,
Direction: nbdb.ACLDirectionToLport,
ExternalIDs: map[string]string{"key": "value"},
Log: true,
Match: "match",
Meter: &aclMeter,
Name: &aclName,
Options: nil,
Priority: 1,
Severity: &aclSev,
}

tests := []struct {
desc string
f func(nbClient libovsdbclient.Client) error
noChanges bool
}{
{
desc: "CreateOrUpdateACLsOps",
f: func(nbClient libovsdbclient.Client) error {
// reset UUID that may be set by the previous tests
searchACL.UUID = ""
_, err := CreateOrUpdateACLsOps(nbClient, nil, &searchACL)
return err
},
},
{
desc: "DeleteACLs",
f: func(nbClient libovsdbclient.Client) error {
// reset UUID that may be set by the previous tests
searchACL.UUID = ""
err := DeleteACLs(nbClient, []nbdb.ACL{searchACL})
return err
},
noChanges: true,
},
{
desc: "UpdateACLsLoggingOps",
f: func(nbClient libovsdbclient.Client) error {
// reset UUID that may be set by the previous tests
searchACL.UUID = ""
_, err := UpdateACLsLoggingOps(nbClient, nil, &searchACL)
return err
},
},
}
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
nbClient, cleanup, err := libovsdbtest.NewNBTestHarness(libovsdbtest.TestSetup{
NBData: initialDB,
}, nil)
if err != nil {
t.Fatalf("test: \"%s\" failed to set up test harness: %v", tt.desc, err)
}
t.Cleanup(cleanup.Cleanup)

pg, err := GetPortGroup(nbClient, pg)
if err != nil {
t.Fatalf("test: \"%s\": failed to get port group", tt.desc)
}
if len(pg.ACLs) != 3 {
t.Fatalf("test: \"%s\" setup failed: expected port group to have 3 ACLs, got %+v", tt.desc, pg.ACLs)
}

err = tt.f(nbClient)
if err != nil {
t.Fatalf("test: \"%s\" returned error: %v", tt.desc, err)
}

pg, err = GetPortGroup(nbClient, pg)
if err != nil {
t.Fatalf("test: \"%s\": failed to get port group", tt.desc)
}
expectedACLNum := 1
if tt.noChanges {
expectedACLNum = 3
}
if len(pg.ACLs) != expectedACLNum {
t.Fatalf("test: \"%s\": expected port group to have %d ACL left, got %+v", tt.desc, expectedACLNum, pg.ACLs)
}
})
}
}
33 changes: 31 additions & 2 deletions go-controller/pkg/libovsdbops/portgroup.go
Expand Up @@ -5,9 +5,8 @@ import (
libovsdbclient "github.com/ovn-org/libovsdb/client"
"github.com/ovn-org/libovsdb/model"
libovsdb "github.com/ovn-org/libovsdb/ovsdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"

"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
)

// findPortGroup looks up the port group in the cache and sets the UUID
Expand Down Expand Up @@ -313,6 +312,36 @@ func DeleteACLsFromPortGroupOps(nbClient libovsdbclient.Client, ops []libovsdb.O
return ops, nil
}

func DeleteACLsFromAllPortGroups(nbClient libovsdbclient.Client, acls ...nbdb.ACL) error {
if len(acls) == 0 {
return nil
}

pg := nbdb.PortGroup{
ACLs: make([]string, 0, len(acls)),
}
for _, acl := range acls {
pg.ACLs = append(pg.ACLs, acl.UUID)
}

opModel := OperationModel{
Model: &pg,
ModelPredicate: func(group *nbdb.PortGroup) bool { return true },
OnModelMutations: []interface{}{&pg.ACLs},
ErrNotFound: false,
BulkOp: true,
}

m := NewModelClient(nbClient)
ops, err := m.DeleteOps(nil, opModel)
if err != nil {
return err
}

_, err = TransactAndCheck(nbClient, ops)
return err
}

func deletePortGroupOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation, name string) ([]libovsdb.Operation, error) {
if ops == nil {
ops = []libovsdb.Operation{}
Expand Down

0 comments on commit 90754d1

Please sign in to comment.