Neighbor merge: clean the default values

Since we don't want the end result of merging to depend on the order,
and we declare two neighbors compatible if:

- they have the same values
- some of the values are not set Or they are set to the default value of
  the field

Here we clean after ensuring the compatibility, to make sure the result
of the merging is stable.

Signed-off-by: Federico Paolinelli <[email protected]>

internal/controller/merge.go

@@ -10,6 +10,7 @@ import (
 )
 
 const (
+	defaultBGPPort       = 179
 	defaultHoldTime      = 180
 	defaultKeepaliveTime = 60
 	defaultConnectTime   = 60
@@ -70,6 +71,7 @@ func mergeNeighbors(curr, toMerge []*frr.NeighborConfig) ([]*frr.NeighborConfig,
 
 		curr.Incoming = mergeAllowedIn(curr.Incoming, n.Incoming)
 
+		cleanNeighborDefaults(curr)
 		mergedNeighbors[n.Addr] = curr
 	}
 
@@ -156,6 +158,23 @@ func mergeAllowedIn(r, toMerge frr.AllowedIn) frr.AllowedIn {
 	return res
 }
 
+// cleanNeighborDefaults unset any field whose value that is equal to the default
+// value for that field. This ensures consistency across conversions.
+func cleanNeighborDefaults(neigh *frr.NeighborConfig) {
+	if neigh.Port != nil && *neigh.Port == defaultBGPPort {
+		neigh.Port = nil
+	}
+	if neigh.HoldTime != nil && *neigh.HoldTime == defaultHoldTime {
+		neigh.HoldTime = nil
+	}
+	if neigh.KeepaliveTime != nil && *neigh.KeepaliveTime == defaultKeepaliveTime {
+		neigh.KeepaliveTime = nil
+	}
+	if neigh.ConnectTime != nil && *neigh.ConnectTime == defaultConnectTime {
+		neigh.ConnectTime = nil
+	}
+}
+
 func mergeIncomingFilters(curr, toMerge []frr.IncomingFilter) []frr.IncomingFilter {
 	all := curr
 	all = append(all, toMerge...)
@@ -207,7 +226,7 @@ func neighborsAreCompatible(n1, n2 *frr.NeighborConfig) error {
 		return fmt.Errorf("multiple asns specified for %s", neighborKey)
 	}
 
-	if !ptrsEqual(n1.Port, n2.Port, 179) {
+	if !ptrsEqual(n1.Port, n2.Port, defaultBGPPort) {
 		return fmt.Errorf("multiple ports specified for %s", neighborKey)
 	}
 

internal/controller/merge_test.go

@@ -1331,6 +1331,46 @@ func TestMergeNeighbors(t *testing.T) {
 			},
 			err: nil,
 		},
+		{
+			// This test is similar to the previous, to ensure conversion stability
+			name: "HoldTime / KeepAlive time, one set to the default, the other set to nil",
+			curr: []*frr.NeighborConfig{
+				{
+					IPFamily:      ipfamily.IPv4,
+					Name:          "[email protected]",
+					ASN:           65040,
+					Addr:          "192.0.1.20",
+					HoldTime:      ptr.To(uint64(180)),
+					KeepaliveTime: ptr.To(uint64(60)),
+					ConnectTime:   ptr.To(uint64(60)),
+				},
+			},
+			toMerge: []*frr.NeighborConfig{
+				{
+					IPFamily: ipfamily.IPv4,
+					Name:     "[email protected]",
+					ASN:      65040,
+					Addr:     "192.0.1.20",
+				},
+			},
+			expected: []*frr.NeighborConfig{
+				{
+					IPFamily: ipfamily.IPv4,
+					Name:     "[email protected]",
+					ASN:      65040,
+					Addr:     "192.0.1.20",
+					Outgoing: frr.AllowedOut{
+						PrefixesV4: []frr.OutgoingFilter{},
+						PrefixesV6: []frr.OutgoingFilter{},
+					},
+					Incoming: frr.AllowedIn{
+						PrefixesV4: []frr.IncomingFilter{},
+						PrefixesV6: []frr.IncomingFilter{},
+					},
+				},
+			},
+			err: nil,
+		},
 	}
 
 	for _, test := range tests {