Repository: ovn-org/libovsdb (go)

Issue:
Unable to reset ACL.Severity to empty (nil) value
After updating ACL.Severity to non empty value it seems like there is no way back.

With libovsdb create ACL and port group
```
        egressFirewallACL := BuildACL(
		"acl",
		nbdb.ACLDirectionFromLport,
		1,
		"ip4.src == 1.2.3.4",
		nbdb.ACLActionDrop,
		"",
		"",
		false,
		nil,
		nil,
	)
	ops, err := CreateOrUpdateACLsOps(nbClient, nil, egressFirewallACL)
	if err != nil {
		fmt.Printf("unable to update ACL: %v", err)
		return
	}
	pg := &nbdb.PortGroup{Name: "pg",
		ACLs: []string{egressFirewallACL.UUID}}

	ops, err = CreateOrUpdatePortGroupsOps(nbClient, ops, pg)
	if err != nil {
		fmt.Printf("unable to create pg: %v", err)
		return
	}
	_, err := TransactAndCheck(nbClient, ops)
	if err != nil {
		fmt.Printf("transact err: %v", err)
	}
```
that will result in 
```
ovn-nbctl list acl acl
_uuid               : 41391edb-a290-406d-b329-3bc580325df0
action              : drop
direction           : from-lport
external_ids        : {}
label               : 0
log                 : false
match               : "ip4.src == 1.2.3.4"
meter               : []
name                : acl
options             : {}
priority            : 1
severity            : []
```
Then update Severity
```
         egressFirewallACL := BuildACL(
		"acl",
		nbdb.ACLDirectionFromLport,
		1,
		"ip4.src == 1.2.3.4",
		nbdb.ACLActionDrop,
		"",
		nbdb.ACLSeverityInfo,
		false,
		nil,
		nil,
	)
	ops, err := CreateOrUpdateACLsOps(nbClient, nil, egressFirewallACL)
	if err != nil {
		fmt.Printf("unable to update ACL: %v", err)
		return
	}
	res, err := TransactAndCheck(nbClient, ops)
	_, err := TransactAndCheck(nbClient, ops)
	if err != nil {
		fmt.Printf("transact err: %v", err)
	}
```
that will result in 
```
ovn-nbctl list acl acl
_uuid               : 41391edb-a290-406d-b329-3bc580325df0
action              : drop
direction           : from-lport
external_ids        : {}
label               : 0
log                 : false
match               : "ip4.src == 1.2.3.4"
meter               : []
name                : acl
options             : {}
priority            : 1
severity            : info
```
Trying to update severity back to empty value (`BuildACL` will set severity to nil when empty string is passed https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/libovsdbops/acl.go#L69-L71, and `UpdateACLsLoggingOps` will force severity to get updated to nil https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/libovsdbops/acl.go#L130):
```
         egressFirewallACL := BuildACL(
		"acl",
		nbdb.ACLDirectionFromLport,
		1,
		"ip4.src == 1.2.3.4",
		nbdb.ACLActionDrop,
		"",
		"",
		false,
		nil,
		nil,
	)
	ops, err := UpdateACLsLoggingOps(nbClient, nil, egressFirewallACL)
	if err != nil {
		fmt.Printf("unable to update ACL: %v", err)
		return
	}
	_, err := TransactAndCheck(nbClient, ops)
	if err != nil {
		fmt.Printf("transact err: %v", err)
	}
```
will result in 
```
unable to update ACL: unable to update model, err: table ACL, column severity: failed to generate ovs element. ovsset supports only go slice/string/numbers/uuid or pointers to those types
```

Pull request:
Fixes creating ovs sets for types with nil pointers
NewOvsSet tries to get the concrete value of a pointer and then check
its type. This wont work for nil pointers that are still holding valid
types themselves. If we get a nil pointer we can simply return an empty
set.

Fixes #310

Signed-off-by: Tim Rozet <trozet@redhat.com>

Patch:
diff --git a/ovsdb/set.go b/ovsdb/set.go
index 60628a2..d5fab1e 100644
--- a/ovsdb/set.go
+++ b/ovsdb/set.go
@@ -19,13 +19,18 @@ type OvsSet struct {
 
 // NewOvsSet creates a new OVSDB style set from a Go interface (object)
 func NewOvsSet(obj interface{}) (OvsSet, error) {
+	ovsSet := make([]interface{}, 0)
 	var v reflect.Value
 	if reflect.TypeOf(obj).Kind() == reflect.Ptr {
 		v = reflect.ValueOf(obj).Elem()
+		if v.Kind() == reflect.Invalid {
+			// must be a nil pointer, so just return an empty set
+			return OvsSet{ovsSet}, nil
+		}
 	} else {
 		v = reflect.ValueOf(obj)
 	}
-	ovsSet := make([]interface{}, 0)
+
 	switch v.Kind() {
 	case reflect.Slice, reflect.Array:
 		for i := 0; i < v.Len(); i++ {


Test patch:
diff --git a/client/api_test.go b/client/api_test.go
index c878191..f731f10 100644
--- a/client/api_test.go
+++ b/client/api_test.go
@@ -970,11 +970,17 @@ func TestAPIUpdate(t *testing.T) {
 	testObj := testLogicalSwitchPort{}
 	testRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse", "tag": testOvsSet(t, []int{6})})
 	tagRow := ovsdb.Row(map[string]interface{}{"tag": testOvsSet(t, []int{6})})
+	var nilInt *int
+	testNilRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse", "tag": testOvsSet(t, nilInt)})
+	typeRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse"})
+	fields := []interface{}{&testObj.Tag, &testObj.Type}
+
 	test := []struct {
 		name      string
 		condition func(API) ConditionalAPI
 		prepare   func(t *testLogicalSwitchPort)
 		result    []ovsdb.Operation
+		fields    []interface{}
 		err       bool
 	}{
 		{
@@ -998,6 +1004,49 @@ func TestAPIUpdate(t *testing.T) {
 			},
 			err: false,
 		},
+		{
+			name: "select by UUID change multiple field with nil pointer/empty set",
+			condition: func(a API) ConditionalAPI {
+				return a.Where(&testLogicalSwitch{
+					UUID: aUUID0,
+				})
+			},
+			prepare: func(t *testLogicalSwitchPort) {
+				t.Type = "somethingElse"
+				t.Tag = nilInt
+			},
+			fields: fields,
+			result: []ovsdb.Operation{
+				{
+					Op:    ovsdb.OperationUpdate,
+					Table: "Logical_Switch_Port",
+					Row:   testNilRow,
+					Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID0}}},
+				},
+			},
+			err: false,
+		},
+		{
+			name: "select by UUID with no fields does not change multiple field with nil pointer/empty set",
+			condition: func(a API) ConditionalAPI {
+				return a.Where(&testLogicalSwitch{
+					UUID: aUUID0,
+				})
+			},
+			prepare: func(t *testLogicalSwitchPort) {
+				t.Type = "somethingElse"
+				t.Tag = nilInt
+			},
+			result: []ovsdb.Operation{
+				{
+					Op:    ovsdb.OperationUpdate,
+					Table: "Logical_Switch_Port",
+					Row:   typeRow,
+					Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID0}}},
+				},
+			},
+			err: false,
+		},
 		{
 			name: "select by index change multiple field with no cache",
 			condition: func(a API) ConditionalAPI {
@@ -1220,7 +1269,7 @@ func TestAPIUpdate(t *testing.T) {
 			// clean test Object
 			testObj = testLogicalSwitchPort{}
 			tt.prepare(&testObj)
-			ops, err := cond.Update(&testObj)
+			ops, err := cond.Update(&testObj, tt.fields...)
 			if tt.err {
 				assert.NotNil(t, err)
 			} else {
diff --git a/ovsdb/encoding_test.go b/ovsdb/encoding_test.go
index f88153d..2ea6c60 100644
--- a/ovsdb/encoding_test.go
+++ b/ovsdb/encoding_test.go
@@ -59,6 +59,8 @@ func TestMap(t *testing.T) {
 }
 
 func TestSet(t *testing.T) {
+	var x *int
+	var y *string
 	tests := []struct {
 		name     string
 		input    interface{}
@@ -149,6 +151,16 @@ func TestSet(t *testing.T) {
 			[]UUID{validUUID0, validUUID1},
 			fmt.Sprintf(`["set",[["uuid","%v"],["uuid","%v"]]]`, validUUIDStr0, validUUIDStr1),
 		},
+		{
+			name:     "nil pointer of valid *int type",
+			input:    x,
+			expected: `["set",[]]`,
+		},
+		{
+			name:     "nil pointer of valid *string type",
+			input:    y,
+			expected: `["set",[]]`,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {