Fix issue (eBay#124): acl add/delete code isn't handling external_ids…

… correctly See eBay#124 for details Signed-off-by: Andre Fredette <[email protected]>
anfredette · Jan 7, 2021 · 30caecc · 30caecc
1 parent 746e766
commit 30caecc
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 6 deletions.
diff --git a/acl.go b/acl.go
@@ -18,6 +18,7 @@ package goovn
 
 import (
 	"github.com/ebay/libovsdb"
+	"reflect"
 )
 
 // ACL ovnnb item
@@ -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
+											}
 										}
 									}
 								}
@@ -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
+									}
 								}
 							}
 						}
@@ -150,6 +163,9 @@ func (odbi *ovndb) aclAddImp(lsw, direct, match, action string, priority int, ex
 			return nil, err
 		}
 		row["external_ids"] = oMap
+	} else {
+		// Add nil so we only match existing ACLs without external_ids in getACLUUIDByRow
+		row["external_ids"] = nil
 	}
 
 	_, err = odbi.getACLUUIDByRow(lsw, TableACL, row)
@@ -162,6 +178,11 @@ func (odbi *ovndb) aclAddImp(lsw, direct, match, action string, priority int, ex
 		return nil, err
 	}
 
+	// Remove row["external_ids"] = nil because it causes an error during execution
+	if external_ids == nil {
+		delete(row, "external_ids")
+	}
+
 	row["action"] = action
 	row["log"] = logflag
 	if logflag {
@@ -226,13 +247,21 @@ func (odbi *ovndb) aclDelImp(lsw, direct, match string, priority int, external_i
 			return nil, err
 		}
 		row["external_ids"] = oMap
+	} else {
+		// Add nil so we only match existing ACLs without external_ids in getACLUUIDByRow
+		row["external_ids"] = nil
 	}
 
 	aclUUID, err := odbi.getACLUUIDByRow(lsw, TableACL, row)
 	if err != nil {
 		return nil, err
 	}
 
+	// Remove row["external_ids"] = nil because it causes an error during execution
+	if external_ids == nil {
+		delete(row, "external_ids")
+	}
+
 	uuidcondition := libovsdb.NewCondition("_uuid", "==", stringToGoUUID(aclUUID))
 	wherecondition = append(wherecondition, uuidcondition)
 	deleteOp := libovsdb.Operation{

diff --git a/acl_test.go b/acl_test.go
@@ -154,7 +154,44 @@ func TestACLs(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	assert.Equal(t, true, len(acls) == 3, "test[%s]", "add second acl")
+	assert.Equal(t, true, len(acls) == 3, "test[%s]", "add second acl with different external ids")
+
+	cmd, err = ovndbapi.ACLAdd(LSW, "to-lport", MATCH_SECOND, "drop", 1001, nil, false, "", "")
+	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) == 4, "test[%s]", "add second acl with nil external ids")
+
+	cmd, err = ovndbapi.ACLDel(LSW, "to-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) == 3, "test[%s]", "acl remove")
+
+	// Try deleting the ACL without external ids again.  This should fail
+	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, nil)
+	if err == nil {
+		t.Fatal(err)
+	}
 
 	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH, 1001, map[string]string{})
 	if err != nil {
@@ -171,7 +208,14 @@ func TestACLs(t *testing.T) {
 	}
 	assert.Equal(t, true, len(acls) == 2, "test[%s]", "acl remove")
 
+	// The following delete should fail because the external ID's are only a subset of an existing ACL
 	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "a"})
+	if err == nil {
+		t.Fatal(err)
+	}
+
+	// Delete ACL w/external_ids provided in a different order
+	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"B": "b", "A": "a"})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -187,7 +231,7 @@ func TestACLs(t *testing.T) {
 
 	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"})
+	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "b", "B": "b"})
 	if err != nil {
 		t.Fatal(err)
 	}