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

…rectly #124 external_ids should be ignored while checking for duplicates in ACLAdd 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]>
eBay · Jan 13, 2021 · 0e9a960 · 0e9a960
1 parent 746e766
commit 0e9a960
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 10 deletions.
diff --git a/acl.go b/acl.go
@@ -144,14 +144,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:
@@ -162,6 +154,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 {

diff --git a/acl_test.go b/acl_test.go
@@ -141,7 +141,34 @@ 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")
+
+	// 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)
 	}
@@ -156,7 +183,39 @@ 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)
+	}
+	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")
+
+
+	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1002, 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]", "add second acl")
+
+	cmd, err = ovndbapi.ACLDel(LSW, "from-lport", MATCH_SECOND, 1001, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -171,7 +230,7 @@ func TestACLs(t *testing.T) {
 	}
 	assert.Equal(t, true, len(acls) == 2, "test[%s]", "acl remove")
 
-	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "a"})
+	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH, 1001, map[string]string{})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -187,7 +246,14 @@ func TestACLs(t *testing.T) {
 
 	assert.Equal(t, true, len(acls) == 1, "test[%s]", "acl remove")
 
+	//The following delete should fail because external ids are provided, but they don't exist in any ACL
 	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "b"})
+	if err == nil {
+		t.Fatal(err)
+	}
+
+	//The following delete should succeed because the provided external_ids provided are a subset of thoe in an existing ACL
+	cmd, err = ovndbapi.ACLDel(LSW, "to-lport", MATCH_SECOND, 1001, map[string]string{"A": "a"})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -202,6 +268,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)