eBay / go-ovn

A Go library for OVN Northbound/Southbound DB access using native OVSDB protocol
Apache License 2.0
108 stars 59 forks source link

In pgUpdateImp method, check for port slice length instead of checkin… #151

Closed Pardhakeswar closed 3 years ago

Pardhakeswar commented 3 years ago

…g whether ports slice is nil

If we check for whether port is nil, in this case, there can be possibility of port not being nil and the port slice len is 0 (if port is created using make([]string, 0, 0)). Then, we are observing below error

Signed-off-by: Pardhakeswar Pacha ppacha@nvidia.com

Pardhakeswar commented 3 years ago

@hzhou8 @girishmg @noah8713 PTAL

girishmg commented 3 years ago

@Pardhakeswar

NIT: your commit subject is too big.. can you just say.. check for port slice length in pgUpdateImp

girishmg commented 3 years ago

@noah8713 can you please merge this?

hzhou8 commented 3 years ago

Thanks @Pardhakeswar and @girishmg . The change is small and obvious, so I just merged.

trozet commented 2 years ago

@girishmg @hzhou8 not sure if you guys care anymore, but I think this commit was wrong. This makes it so that if a user intentionally passes an empty array to update the ports from [a,b,c] to [] it will not work if external_ids is nil:

I0307 15:14:04.208929       1 policy.go:804] Processing NetworkPolicy e2e-network-policy-8729/deny-client-a-via-except-cidr-egress-rule to have 0 local pods...
I0307 15:14:04.211656       1 policy.go:813] Setting NetworkPolicy e2e-network-policy-8729/deny-client-a-via-except-cidr-egress-rule to have 0 local pods...
E0307 15:14:04.212115       1 policy.go:901] Failed to set ports in PortGroup for network policy e2e-network-policy-8729/deny-client-a-via-except-cidr-egress-rule: no changes requested
I0307 15:14:04.212138       1 policy.go:904] Done setting NetworkPolicy e2e-network-policy-8729/deny-client-a-via-except-cidr-egress-rule local pods

The reason @Pardhakeswar hit the error in his description is because NewOVSSet had a bug that we fixed in our downstream: https://github.com/openshift/ovn-kubernetes/commit/35677418d2bbfddb6229e1d776bba2064dde646b#diff-88e093886eb91e9ca5f9234d74a5f756c0251d685c141c902a7833d95bec5345R27

// NewOvsSet creates a new OVSDB style set from a Go slice
func NewOvsSet(goSlice interface{}) (*OvsSet, error) {
    v := reflect.ValueOf(goSlice)
    if v.Kind() != reflect.Slice {
        return nil, errors.New("OvsSet supports only Go Slice types")
    }

-   var ovsSet []interface{}
+   ovsSet := make([]interface{}, 0, v.Len())
    for i := 0; i < v.Len(); i++ {
        ovsSet = append(ovsSet, v.Index(i).Interface())
    }