Merge pull request #221 from karampok/OCPBUGS-42604

Throw better error when prefix for same localPref is added twice

Author: fedepaol

internal/controller/api_to_config.go

@@ -514,20 +514,36 @@ func localPrefPrefixesToMap(withLocalPref []v1beta1.LocalPrefPrefixes) (localPre
 		localPrefForPrefixV6: map[string]uint32{},
 	}
 
+	seen := make(map[uint32]bool)
 	for _, pfxs := range withLocalPref {
+		if _, exists := seen[pfxs.LocalPref]; exists {
+			// error when input is withLocalPref = []v1beta1.LocalPrefPrefixes{
+			//	                      Prefixes:  []string{"192.0.2.0/24"},LocalPref: 100}, []string{"100.0.0.0/24"},LocalPref: 100}}
+			// so we enforce       withLocalPref = []v1beta1.LocalPrefPrefixes{
+			//	                      Prefixes:  []string{"192.0.2.0/24, "100.0.0.0/24"},LocalPref: 100}}
+			return localPrefPrefixes{}, fmt.Errorf("multiple entries with the same localPref: %d", pfxs.LocalPref)
+		}
+		seen[pfxs.LocalPref] = true
+
 		for _, p := range pfxs.Prefixes {
 			family := ipfamily.ForCIDRString(p)
 			lpMap := res.localPrefForPrefixV4
 			if family == ipfamily.IPv6 {
 				lpMap = res.localPrefForPrefixV6
 			}
 
-			_, ok := lpMap[p]
-			if ok {
-				return localPrefPrefixes{}, fmt.Errorf("multiple local prefs specified for prefix %s", p)
+			v, ok := lpMap[p]
+			if !ok {
+				lpMap[p] = pfxs.LocalPref
+				continue
 			}
 
-			lpMap[p] = pfxs.LocalPref
+			if v != pfxs.LocalPref {
+				return localPrefPrefixes{}, fmt.Errorf("multiple local prefs (%d,%d) specified for prefix %s", v, pfxs.LocalPref, p)
+			}
+			if v == pfxs.LocalPref {
+				return localPrefPrefixes{}, fmt.Errorf("prefix %s with local prefs %d defined twice", p, v)
+			}
 		}
 	}
 

internal/controller/api_to_config_test.go

@@ -843,6 +843,88 @@ func TestConversion(t *testing.T) {
 			expected: nil,
 			err:      fmt.Errorf("multiple local prefs specified for prefix %s", "192.0.4.0/24"),
 		},
+		{
+			name: "One neighbor, trying to set samelocalPrefs for different prefix entries",
+			fromK8s: []v1beta1.FRRConfiguration{
+				{
+					Spec: v1beta1.FRRConfigurationSpec{
+						BGP: v1beta1.BGPConfig{
+							Routers: []v1beta1.Router{
+								{
+									ASN: 65040,
+									ID:  "192.0.2.20",
+									Neighbors: []v1beta1.Neighbor{
+										{
+											ASN:     65041,
+											Address: "192.0.2.21",
+											ToAdvertise: v1beta1.Advertise{
+												Allowed: v1beta1.AllowedOutPrefixes{
+													Prefixes: []string{"192.0.2.0/24", "10.0.0.0/24"},
+													Mode:     v1beta1.AllowRestricted,
+												},
+												PrefixesWithLocalPref: []v1beta1.LocalPrefPrefixes{
+													{
+														Prefixes:  []string{"10.0.0.0/24"},
+														LocalPref: 100,
+													},
+													{
+														Prefixes:  []string{"192.0.2.0/24"},
+														LocalPref: 100,
+													},
+												},
+											},
+										},
+									},
+									Prefixes: []string{"192.0.2.0/24", "10.0.0.0/24"},
+								},
+							},
+						},
+					},
+				},
+			},
+			secrets:  map[string]v1.Secret{},
+			expected: nil,
+			err:      fmt.Errorf("a not nil error"),
+		},
+		{
+			name: "One neighbor, trying to set samelocalPrefs for a prefix twice",
+			fromK8s: []v1beta1.FRRConfiguration{
+				{
+					Spec: v1beta1.FRRConfigurationSpec{
+						BGP: v1beta1.BGPConfig{
+							Routers: []v1beta1.Router{
+								{
+									ASN: 65040,
+									ID:  "192.0.2.20",
+									Neighbors: []v1beta1.Neighbor{
+										{
+											ASN:     65041,
+											Address: "192.0.2.21",
+											ToAdvertise: v1beta1.Advertise{
+												Allowed: v1beta1.AllowedOutPrefixes{
+													Prefixes: []string{"192.0.2.0/24", "192.0.2.0/24"},
+													Mode:     v1beta1.AllowRestricted,
+												},
+												PrefixesWithLocalPref: []v1beta1.LocalPrefPrefixes{
+													{
+														Prefixes:  []string{"192.0.2.0/24", "192.0.2.0/24"},
+														LocalPref: 100,
+													},
+												},
+											},
+										},
+									},
+									Prefixes: []string{"192.0.2.0/24"},
+								},
+							},
+						},
+					},
+				},
+			},
+			secrets:  map[string]v1.Secret{},
+			expected: nil,
+			err:      fmt.Errorf("a not nil error"),
+		},
 		{
 			name: "Neighbor with ToReceiveAll",
 			fromK8s: []v1beta1.FRRConfiguration{