Repository: metallb/metallb
Pull request #1323 (2022-04-18 12:47:45)

Add unit tests conversion webhooks

liornoy: lgtm, only one small not critical comment I have is that maybe you could extract the 
definitions of the objects outside the test functions, and have something like:
convertBGPPeer := v1beta1BGPPeer
expectedBGPPeer := v1beta2BGPPeer
And this way you wouldn't need to have to repeat defining those objects twice in the
ConvertTo and ConverFrom functions.
Same for the addresspool.
fedepaol: LGTM

Patch:
diff --git a/api/v1alpha1/addresspool_conversion.go b/api/v1alpha1/addresspool_conversion.go
index 962c1450..db69d6f1 100644
--- a/api/v1alpha1/addresspool_conversion.go
+++ b/api/v1alpha1/addresspool_conversion.go
@@ -4,6 +4,7 @@ package v1alpha1
 
 import (
 	"go.universe.tf/metallb/api/v1beta1"
+	"go.universe.tf/metallb/internal/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 )
 
@@ -13,11 +14,17 @@ func (src *AddressPool) ConvertTo(dstRaw conversion.Hub) error {
 	dst.Spec.Protocol = src.Spec.Protocol
 	dst.Spec.Addresses = make([]string, len(src.Spec.Addresses))
 	copy(dst.Spec.Addresses, src.Spec.Addresses)
-	*dst.Spec.AutoAssign = *src.Spec.AutoAssign
-	dst.Spec.BGPAdvertisements = make([]v1beta1.LegacyBgpAdvertisement, 0)
+	if src.Spec.AutoAssign != nil {
+		dst.Spec.AutoAssign = pointer.BoolPtr(*src.Spec.AutoAssign)
+	}
+	dst.Spec.BGPAdvertisements = make([]v1beta1.LegacyBgpAdvertisement, len(src.Spec.BGPAdvertisements))
 	for i, adv := range src.Spec.BGPAdvertisements {
-		*dst.Spec.BGPAdvertisements[i].AggregationLength = *adv.AggregationLength
-		*dst.Spec.BGPAdvertisements[i].AggregationLengthV6 = *adv.AggregationLengthV6
+		if adv.AggregationLength != nil {
+			dst.Spec.BGPAdvertisements[i].AggregationLength = pointer.Int32Ptr(*adv.AggregationLength)
+		}
+		if adv.AggregationLengthV6 != nil {
+			dst.Spec.BGPAdvertisements[i].AggregationLengthV6 = pointer.Int32Ptr(*adv.AggregationLengthV6)
+		}
 		dst.Spec.BGPAdvertisements[i].LocalPref = adv.LocalPref
 		dst.Spec.BGPAdvertisements[i].Communities = make([]string, len(adv.Communities))
 		copy(dst.Spec.BGPAdvertisements[i].Communities, adv.Communities)
@@ -34,11 +41,17 @@ func (dst *AddressPool) ConvertFrom(srcRaw conversion.Hub) error {
 	dst.Spec.Protocol = src.Spec.Protocol
 	dst.Spec.Addresses = make([]string, len(src.Spec.Addresses))
 	copy(dst.Spec.Addresses, src.Spec.Addresses)
-	*dst.Spec.AutoAssign = *src.Spec.AutoAssign
-	dst.Spec.BGPAdvertisements = make([]BgpAdvertisement, 0)
+	if src.Spec.AutoAssign != nil {
+		dst.Spec.AutoAssign = pointer.BoolPtr(*src.Spec.AutoAssign)
+	}
+	dst.Spec.BGPAdvertisements = make([]BgpAdvertisement, len(src.Spec.BGPAdvertisements))
 	for i, adv := range src.Spec.BGPAdvertisements {
-		*dst.Spec.BGPAdvertisements[i].AggregationLength = *adv.AggregationLength
-		*dst.Spec.BGPAdvertisements[i].AggregationLengthV6 = *adv.AggregationLengthV6
+		if adv.AggregationLength != nil {
+			dst.Spec.BGPAdvertisements[i].AggregationLength = pointer.Int32Ptr(*adv.AggregationLength)
+		}
+		if adv.AggregationLengthV6 != nil {
+			dst.Spec.BGPAdvertisements[i].AggregationLengthV6 = pointer.Int32Ptr(*adv.AggregationLengthV6)
+		}
 		dst.Spec.BGPAdvertisements[i].LocalPref = adv.LocalPref
 		dst.Spec.BGPAdvertisements[i].Communities = make([]string, len(adv.Communities))
 		copy(dst.Spec.BGPAdvertisements[i].Communities, adv.Communities)
diff --git a/api/v1beta1/bgppeer_conversion.go b/api/v1beta1/bgppeer_conversion.go
index 2fa74572..da0dbd5e 100644
--- a/api/v1beta1/bgppeer_conversion.go
+++ b/api/v1beta1/bgppeer_conversion.go
@@ -93,7 +93,8 @@ func labelsToLegacySelector(selectors []metav1.LabelSelector) ([]NodeSelector, e
 	res := []NodeSelector{}
 	for _, sel := range selectors {
 		toAdd := NodeSelector{
-			MatchLabels: make(map[string]string),
+			MatchLabels:      make(map[string]string),
+			MatchExpressions: make([]MatchExpression, 0),
 		}
 		for k, v := range sel.MatchLabels {
 			toAdd.MatchLabels[k] = v
@@ -105,6 +106,7 @@ func labelsToLegacySelector(selectors []metav1.LabelSelector) ([]NodeSelector, e
 				Values:   make([]string, len(e.Values)),
 			}
 			copy(m.Values, e.Values)
+			toAdd.MatchExpressions = append(toAdd.MatchExpressions, m)
 		}
 		res = append(res, toAdd)
 	}


Test patch:
diff --git a/api/v1alpha1/addresspool_conversion_test.go b/api/v1alpha1/addresspool_conversion_test.go
new file mode 100644
index 00000000..58068097
--- /dev/null
+++ b/api/v1alpha1/addresspool_conversion_test.go
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package v1alpha1
+
+import (
+	"reflect"
+	"testing"
+
+	"go.universe.tf/metallb/api/v1beta1"
+	"go.universe.tf/metallb/internal/pointer"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const (
+	MetalLBTestNameSpace = "metallb-test-namespace"
+)
+
+func TestValidateAddressPoolConvertTo(t *testing.T) {
+	var err error
+	var resAddressPool v1beta1.AddressPool
+
+	convertAddressPool := AddressPool{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-addresspool",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: AddressPoolSpec{
+			Addresses: []string{
+				"1.1.1.1-1.1.1.100",
+			},
+
+			Protocol:   "bgp",
+			AutoAssign: pointer.BoolPtr(false),
+			BGPAdvertisements: []BgpAdvertisement{
+				{
+					AggregationLength:   pointer.Int32Ptr(32),
+					AggregationLengthV6: pointer.Int32Ptr(128),
+					LocalPref:           uint32(100),
+					Communities:         []string{"1234"},
+				},
+			},
+		},
+	}
+
+	expectedAddressPool := v1beta1.AddressPool{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-addresspool",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: v1beta1.AddressPoolSpec{
+			Addresses: []string{
+				"1.1.1.1-1.1.1.100",
+			},
+
+			Protocol:   "bgp",
+			AutoAssign: pointer.BoolPtr(false),
+			BGPAdvertisements: []v1beta1.LegacyBgpAdvertisement{
+				{
+					AggregationLength:   pointer.Int32Ptr(32),
+					AggregationLengthV6: pointer.Int32Ptr(128),
+					LocalPref:           uint32(100),
+					Communities:         []string{"1234"},
+				},
+			},
+		},
+	}
+
+	err = convertAddressPool.ConvertTo(&resAddressPool)
+	if err != nil {
+		t.Fatalf("failed converting AddressPool to v1beta1 version: %s", err)
+	}
+
+	if !reflect.DeepEqual(resAddressPool, expectedAddressPool) {
+		t.Fatalf("expected AddressPool different than converted: %s", err)
+	}
+}
+
+func TestValidateAddressPoolConvertFrom(t *testing.T) {
+	var err error
+	var resAddressPool AddressPool
+
+	convertAddressPool := v1beta1.AddressPool{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-addresspool",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: v1beta1.AddressPoolSpec{
+			Addresses: []string{
+				"1.1.1.1-1.1.1.100",
+			},
+
+			Protocol:   "bgp",
+			AutoAssign: pointer.BoolPtr(false),
+			BGPAdvertisements: []v1beta1.LegacyBgpAdvertisement{
+				{
+					AggregationLength:   pointer.Int32Ptr(32),
+					AggregationLengthV6: pointer.Int32Ptr(128),
+					LocalPref:           uint32(100),
+					Communities:         []string{"1234"},
+				},
+			},
+		},
+	}
+
+	expectedAddressPool := AddressPool{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-addresspool",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: AddressPoolSpec{
+			Addresses: []string{
+				"1.1.1.1-1.1.1.100",
+			},
+
+			Protocol:   "bgp",
+			AutoAssign: pointer.BoolPtr(false),
+			BGPAdvertisements: []BgpAdvertisement{
+				{
+					AggregationLength:   pointer.Int32Ptr(32),
+					AggregationLengthV6: pointer.Int32Ptr(128),
+					LocalPref:           uint32(100),
+					Communities:         []string{"1234"},
+				},
+			},
+		},
+	}
+
+	err = resAddressPool.ConvertFrom(&convertAddressPool)
+	if err != nil {
+		t.Fatalf("failed converting v1beta1 AddressPool: %s", err)
+	}
+
+	if !reflect.DeepEqual(resAddressPool, expectedAddressPool) {
+		t.Fatalf("expected AddressPool different than converted: %s", err)
+	}
+}
diff --git a/api/v1beta1/bgppeer_conversion_test.go b/api/v1beta1/bgppeer_conversion_test.go
new file mode 100644
index 00000000..c2055e17
--- /dev/null
+++ b/api/v1beta1/bgppeer_conversion_test.go
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package v1beta1
+
+import (
+	"reflect"
+	"testing"
+	"time"
+
+	"go.universe.tf/metallb/api/v1beta2"
+	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestValidateBGPPeerConvertTo(t *testing.T) {
+	var err error
+	var resBGPPeer v1beta2.BGPPeer
+
+	convertBGPPeer := BGPPeer{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "peer1",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: BGPPeerSpec{
+			MyASN:         42,
+			ASN:           142,
+			Address:       "1.2.3.4",
+			Port:          1179,
+			HoldTime:      v1.Duration{Duration: 180 * time.Second},
+			RouterID:      "10.20.30.40",
+			SrcAddress:    "10.20.30.40",
+			EBGPMultiHop:  true,
+			BFDProfile:    "default",
+			KeepaliveTime: v1.Duration{Duration: time.Second},
+			Password:      "nopass",
+			NodeSelectors: []NodeSelector{
+				{
+					MatchLabels: map[string]string{
+						"foo": "bar",
+					},
+					MatchExpressions: []MatchExpression{
+						{
+							Operator: "In",
+							Values:   []string{"quux"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	expectedBGPPeer := v1beta2.BGPPeer{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "peer1",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: v1beta2.BGPPeerSpec{
+			MyASN:          42,
+			ASN:            142,
+			Address:        "1.2.3.4",
+			Port:           1179,
+			HoldTime:       v1.Duration{Duration: 180 * time.Second},
+			RouterID:       "10.20.30.40",
+			SrcAddress:     "10.20.30.40",
+			EBGPMultiHop:   true,
+			Password:       "nopass",
+			PasswordSecret: corev1.SecretReference{},
+			BFDProfile:     "default",
+			KeepaliveTime:  v1.Duration{Duration: time.Second},
+			NodeSelectors: []v1.LabelSelector{
+				{
+					MatchLabels: map[string]string{
+						"foo": "bar",
+					},
+					MatchExpressions: []v1.LabelSelectorRequirement{
+						{
+							Operator: "In",
+							Values:   []string{"quux"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	err = convertBGPPeer.ConvertTo(&resBGPPeer)
+	if err != nil {
+		t.Fatalf("failed converting BGPPeer to v1beta2 version: %s", err)
+	}
+
+	if !reflect.DeepEqual(resBGPPeer, expectedBGPPeer) {
+		t.Fatalf("expected BGPPeer different than converted: %s", err)
+	}
+}
+
+func TestValidateBGPPeerConvertFrom(t *testing.T) {
+	var err error
+	var resBGPPeer BGPPeer
+
+	convertBGPPeer := v1beta2.BGPPeer{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "peer1",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: v1beta2.BGPPeerSpec{
+			MyASN:        42,
+			ASN:          142,
+			Address:      "1.2.3.4",
+			Port:         1179,
+			HoldTime:     v1.Duration{Duration: 180 * time.Second},
+			RouterID:     "10.20.30.40",
+			SrcAddress:   "10.20.30.40",
+			EBGPMultiHop: true,
+			Password:     "nopass",
+			PasswordSecret: corev1.SecretReference{Name: "nosecret",
+				Namespace: "metallb-system"},
+			BFDProfile:    "default",
+			KeepaliveTime: v1.Duration{Duration: time.Second},
+			NodeSelectors: []v1.LabelSelector{
+				{
+					MatchLabels: map[string]string{
+						"foo": "bar",
+					},
+					MatchExpressions: []v1.LabelSelectorRequirement{
+						{
+							Operator: "In",
+							Values:   []string{"quux"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	expectedBGPPeer := BGPPeer{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "peer1",
+			Namespace: MetalLBTestNameSpace,
+		},
+		Spec: BGPPeerSpec{
+			MyASN:         42,
+			ASN:           142,
+			Address:       "1.2.3.4",
+			Port:          1179,
+			HoldTime:      v1.Duration{Duration: 180 * time.Second},
+			RouterID:      "10.20.30.40",
+			SrcAddress:    "10.20.30.40",
+			EBGPMultiHop:  true,
+			BFDProfile:    "default",
+			KeepaliveTime: v1.Duration{Duration: time.Second},
+			Password:      "nopass",
+			NodeSelectors: []NodeSelector{
+				{
+					MatchLabels: map[string]string{
+						"foo": "bar",
+					},
+					MatchExpressions: []MatchExpression{
+						{
+							Operator: "In",
+							Values:   []string{"quux"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	err = resBGPPeer.ConvertFrom(&convertBGPPeer)
+	if err != nil {
+		t.Fatalf("failed converting v1beta2 BGPPeer: %s", err)
+	}
+
+	if !reflect.DeepEqual(resBGPPeer, expectedBGPPeer) {
+		t.Fatalf("expected BGPPeer different than converted: %s", err)
+	}
+}