Skip to content

Commit

Permalink
Fix issue (eBay#124): acl add/delete code isn't handling external_ids…
Browse files Browse the repository at this point in the history 
… correctly

eBay#124

external_ids should be ignored while checking for duplicates in ACLAdd and
matching in ACLDel

The parameters that define a unique ACL for a given entity (logical switch or
port group) are direction, match, and priority. The code currently also
attempts to also use external_ids, but it should not.

Signed-off-by: Andre Fredette <[email protected]>
  • Loading branch information
@anfredette
anfredette committed Jan 13, 2021
1 parent 746e766 commit 3b324f0
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 23 deletions.
44 changes: 25 additions & 19 deletions acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package goovn

import (
"github.com/ebay/libovsdb"
"reflect"
)

// ACL ovnnb item
Expand Down Expand Up @@ -78,8 +79,14 @@ func (odbi *ovndb) getACLUUIDByRow(lsw, table string, row OVNRow) (string, error
goto unmatched
}
case "external_ids":
if value != nil && !odbi.oMapContians(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap, value.(*libovsdb.OvsMap).GoMap) {
goto unmatched
if value == nil {
if len(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap) != 0 {
goto unmatched
}
} else {
if !reflect.DeepEqual(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap, value.(*libovsdb.OvsMap).GoMap) {
goto unmatched
}
}
}
}
Expand Down Expand Up @@ -119,8 +126,14 @@ func (odbi *ovndb) getACLUUIDByRow(lsw, table string, row OVNRow) (string, error
goto out
}
case "external_ids":
if value != nil && !odbi.oMapContians(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap, value.(*libovsdb.OvsMap).GoMap) {
goto out
if value == nil {
if len(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap) != 0 {
goto out
}
} else {
if !reflect.DeepEqual(cacheACL.Fields["external_ids"].(libovsdb.OvsMap).GoMap, value.(*libovsdb.OvsMap).GoMap) {
goto out
}
}
}
}
Expand All @@ -144,14 +157,6 @@ func (odbi *ovndb) aclAddImp(lsw, direct, match, action string, priority int, ex
row["match"] = match
row["priority"] = priority

if external_ids != nil {
oMap, err := libovsdb.NewOvsMap(external_ids)
if err != nil {
return nil, err
}
row["external_ids"] = oMap
}

_, err = odbi.getACLUUIDByRow(lsw, TableACL, row)
switch err {
case ErrorNotFound:
Expand All @@ -162,6 +167,14 @@ func (odbi *ovndb) aclAddImp(lsw, direct, match, action string, priority int, ex
return nil, err
}

if external_ids != nil {
oMap, err := libovsdb.NewOvsMap(external_ids)
if err != nil {
return nil, err
}
row["external_ids"] = oMap
}

row["action"] = action
row["log"] = logflag
if logflag {
Expand Down Expand Up @@ -220,13 +233,6 @@ func (odbi *ovndb) aclDelImp(lsw, direct, match string, priority int, external_i
row["priority"] = priority
}

if external_ids != nil {
oMap, err := libovsdb.NewOvsMap(external_ids)
if err != nil {
return nil, err
}
row["external_ids"] = oMap
}

aclUUID, err := odbi.getACLUUIDByRow(lsw, TableACL, row)
if err != nil {
Expand Down
77 changes: 73 additions & 4 deletions acl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ func TestACLs(t *testing.T) {
cmd, err = ovndbapi.ACLAdd(LSW, "to-lport", MATCH, "drop", 1001, nil, true, "", "")
// ACLAdd must return error
assert.Equal(t, true, nil != err, "test[%s]", "add same acl twice, should only one added.")
if cmd != nil {
t.Fatal(err)
}
// cmd is nil, so this is noop
err = ovndbapi.Execute(cmd)
assert.Equal(t, true, nil == err, "test[%s]", "add same acl twice, should only one added.")
Expand All @@ -141,7 +144,35 @@ func TestACLs(t *testing.T) {
}
assert.Equal(t, true, len(acls) == 2, "test[%s]", "add second acl")

// The following should fail because it is considered a duplicate of an existing ACL
cmd, err = ovndbapi.ACLAdd(LSW, "to-lport", MATCH_SECOND, "drop", 1001, map[string]string{"A": "b", "B": "b"}, false, "", "")
if err == nil {
t.Fatal(err)
}
if cmd != nil {
t.Fatal(err)
}
// cmd is nil, so this is noop
err = ovndbapi.Execute(cmd)
if err != nil {
t.Fatal(err)
}

acls, err = ovndbapi.ACLList(LSW)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, true, len(acls) == 2, "test[%s]", "add second acl")

//***************** Add more tests here
// The following should fail because it is considered a duplicate of an existing ACL
cmd, err = ovndbapi.ACLAdd(LSW, "to-lport", MATCH_SECOND, "drop", 1001, nil, false, "", "")
if err == nil {
t.Fatal(err)
}

// Different priority is a different ACL, so this should succeed
cmd, err = ovndbapi.ACLAdd(LSW, "to-lport", MATCH_SECOND, "drop", 1002, map[string]string{"A": "a", "B": "b"}, false, "", "")
if err != nil {
t.Fatal(err)
}
Expand All @@ -156,7 +187,8 @@ func TestACLs(t *testing.T) {
}
assert.Equal(t, true, len(acls) == 3, "test[%s]", "add second acl")

cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH, 1001, map[string]string{})
// Different direction is a different ACL, so this should succeed
cmd, err = ovndbapi.ACLAdd(LSW, "from-lport", MATCH_SECOND, "drop", 1001, map[string]string{"A": "a", "B": "b"}, false, "", "")
if err != nil {
t.Fatal(err)
}
Expand All @@ -169,9 +201,10 @@ func TestACLs(t *testing.T) {
if err != nil {
t.Fatal(err)
}
assert.Equal(t, true, len(acls) == 2, "test[%s]", "acl remove")
assert.Equal(t, true, len(acls) == 4, "test[%s]", "add second acl")

cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "a"})

cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1002, nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -184,10 +217,40 @@ func TestACLs(t *testing.T) {
if err != nil {
t.Fatal(err)
}
assert.Equal(t, true, len(acls) == 3, "test[%s]", "add second acl")

cmd, err = ovndbapi.ACLDel(LSW, "from-lport", MATCH_SECOND, 1001, nil)
if err != nil {
t.Fatal(err)
}
err = ovndbapi.Execute(cmd)
if err != nil {
t.Fatal(err)
}

acls, err = ovndbapi.ACLList(LSW)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, true, len(acls) == 2, "test[%s]", "acl remove")

cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH, 1001, map[string]string{})
if err != nil {
t.Fatal(err)
}
err = ovndbapi.Execute(cmd)
if err != nil {
t.Fatal(err)
}

acls, err = ovndbapi.ACLList(LSW)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, true, len(acls) == 1, "test[%s]", "acl remove")

cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "b"})
// The following ACLDel should succeed. The external_ids are different, but they are ignored during the delete.
cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "a"})
if err != nil {
t.Fatal(err)
}
Expand All @@ -202,6 +265,12 @@ func TestACLs(t *testing.T) {
}
assert.Equal(t, true, len(acls) == 0, "test[%s]", "acl remove")

// The following ACLDel should fail because all the ACLs have been deleted.
cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "b"})
if err == nil {
t.Fatal(err)
}

cmd, err = ovndbapi.LSPDel(LSP)
if err != nil {
t.Fatal(err)
Expand Down

0 comments on commit 3b324f0

Please sign in to comment.