nebius/SWE-rebench-V2-PRs huggingface dataset:
Repository: metallb/metallb
Pull request #1166 (2022-01-13 14:25:24)

Controller: enqueue the failed configmap till services update
When edit configmap for existing service, the config was rejected and
not re-enqueued for reprocess when the service update takes place,
causing the service to stay in pending state till it get deleted
by k8s.

Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>


fedepaol: LGTM, waiting for CI

Patch:
diff --git a/controller/main.go b/controller/main.go
index 3153b4e9..409a8446 100644
--- a/controller/main.go
+++ b/controller/main.go
@@ -110,7 +110,7 @@ func (c *controller) SetConfig(l log.Logger, cfg *config.Config) k8s.SyncState {
 
 	if err := c.ips.SetPools(cfg.Pools); err != nil {
 		level.Error(l).Log("op", "setConfig", "error", err, "msg", "applying new configuration failed")
-		return k8s.SyncStateErrorNoRetry
+		return k8s.SyncStateError
 	}
 	c.config = cfg
 	return k8s.SyncStateReprocessAll


Test patch:
diff --git a/controller/controller_test.go b/controller/controller_test.go
index 6a358e5f..17abb163 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -708,7 +708,7 @@ func TestControllerConfig(t *testing.T) {
 	}
 
 	// Now that an IP is allocated, removing the IP pool is not allowed.
-	if c.SetConfig(l, &config.Config{}) != k8s.SyncStateErrorNoRetry {
+	if c.SetConfig(l, &config.Config{}) != k8s.SyncStateError {
 		t.Fatalf("SetConfig that deletes allocated IPs was accepted")
 	}
 
@@ -874,7 +874,7 @@ func TestControllerDualStackConfig(t *testing.T) {
 	}
 
 	// Now that an IP is allocated, removing the IP pool is not allowed.
-	if c.SetConfig(l, &config.Config{}) != k8s.SyncStateErrorNoRetry {
+	if c.SetConfig(l, &config.Config{}) != k8s.SyncStateError {
 		t.Fatalf("SetConfig that deletes allocated IPs was accepted")
 	}
Repository: metallb/metallb
Pull request #1187 (2022-01-24 10:33:20)

AddressPool validation: Add BGPAdvertisement validation to avoid duplication
When configuring an addressPool with two same BGPAdvertisements, an error should be produced.

fedepaol: @sabinaaledort can you fix the commit messages?
The subject is truncated and part of it is moved to the body. Also, some of the commits are without the body.
fedepaol: LGTM, thanks!

Patch:
diff --git a/internal/config/config.go b/internal/config/config.go
index 31991fdf..5f72f762 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -506,8 +506,18 @@ func parseBGPAdvertisements(ads []bgpAdvertisement, cidrsPerAddresses map[string
 		}, nil
 	}
 
+	err := validateDuplicateAdvertisements(ads)
+	if err != nil {
+		return nil, err
+	}
+
 	var ret []*BGPAdvertisement
 	for _, rawAd := range ads {
+		err := validateDuplicateCommunities(rawAd.Communities)
+		if err != nil {
+			return nil, err
+		}
+
 		ad := &BGPAdvertisement{
 			AggregationLength:   32,
 			AggregationLengthV6: 128,
@@ -579,7 +589,7 @@ func ParseCommunity(c string) (uint32, error) {
 	}
 	b, err := strconv.ParseUint(fs[1], 10, 16)
 	if err != nil {
-		return 0, fmt.Errorf("invalid second section of community %q: %s", fs[0], err)
+		return 0, fmt.Errorf("invalid second section of community %q: %s", fs[1], err)
 	}
 
 	return (uint32(a) << 16) + uint32(b), nil
@@ -667,3 +677,25 @@ func bfdIntFromConfig(value *uint32, min, max uint32) (*uint32, error) {
 	}
 	return value, nil
 }
+
+func validateDuplicateAdvertisements(ads []bgpAdvertisement) error {
+	for i := 0; i < len(ads); i++ {
+		for j := i + 1; j < len(ads); j++ {
+			if reflect.DeepEqual(ads[i], ads[j]) {
+				return fmt.Errorf("duplicate definition of bgpadvertisements. advertisement %d and %d are equal", i+1, j+1)
+			}
+		}
+	}
+	return nil
+}
+
+func validateDuplicateCommunities(communities []string) error {
+	for i := 0; i < len(communities); i++ {
+		for j := i + 1; j < len(communities); j++ {
+			if communities[i] == communities[j] {
+				return fmt.Errorf("duplicate definition of community %q", communities[i])
+			}
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 4ed9c736..137fd0dd 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -859,6 +859,33 @@ bfd-profiles:
 bfd-profiles:
 - name: default
   receive-interval: 90000
+`,
+		},
+		{
+			desc: "Duplicate bgp advertisment definition",
+			raw: `
+address-pools:
+- name: pool1
+  protocol: bgp
+  addresses:
+  - 10.20.0.0/16
+  bgp-advertisements:
+  - aggregation-length: 26
+  - aggregation-length: 26
+`,
+		},
+		{
+			desc: "Duplicate communities definition",
+			raw: `
+bgp-communities:
+  bar: 64512:1234
+address-pools:
+- name: pool1
+  protocol: bgp
+  addresses:
+  - 10.20.0.0/16
+  bgp-advertisements:
+  - communities: ["bar", "bar"]
 `,
 		},
 	}
Repository: metallb/metallb
Pull request #1190 (2022-01-24 16:25:23)

FRR mode doesn't support duplicate peers in the same VRF
Add check to fail a config with duplicate BGPPeer for FRR mode

Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>



fedepaol: couple of minor comments, looks good, thanks!
fedepaol: LGTM

Patch:
diff --git a/internal/config/validate.go b/internal/config/validate.go
index 255e02b0..dca8f07b 100644
--- a/internal/config/validate.go
+++ b/internal/config/validate.go
@@ -56,16 +56,22 @@ func DontValidate(c *configFile) error {
 // DiscardNativeOnly returns an error if the current configFile contains
 // any options that are available only in the native implementation.
 func DiscardNativeOnly(c *configFile) error {
-	if len(c.Peers) > 0 {
+	if len(c.Peers) > 1 {
+		peerAddr := make(map[string]bool)
 		myAsn := c.Peers[0].MyASN
 		routerID := c.Peers[0].RouterID
-		for _, p := range c.Peers {
+		peerAddr[c.Peers[0].Addr] = true
+		for _, p := range c.Peers[1:] {
 			if p.RouterID != routerID {
 				return fmt.Errorf("peer %s has RouterID different from %s, in FRR mode all RouterID must be equal", p.RouterID, c.Peers[0].RouterID)
 			}
 			if p.MyASN != myAsn {
 				return fmt.Errorf("peer %s has myAsn different from %s, in FRR mode all myAsn must be equal", p.Addr, c.Peers[0].Addr)
 			}
+			if _, ok := peerAddr[p.Addr]; ok {
+				return fmt.Errorf("peer %s already exists, FRR mode doesn't support duplicate BGPPeers", p.Addr)
+			}
+			peerAddr[p.Addr] = true
 		}
 	}
 	return nil


Test patch:
diff --git a/internal/config/validate_test.go b/internal/config/validate_test.go
index a86c7d8c..26ea0f19 100644
--- a/internal/config/validate_test.go
+++ b/internal/config/validate_test.go
@@ -215,6 +215,20 @@ func TestValidateFRR(t *testing.T) {
 			},
 			mustFail: true,
 		},
+		{
+			desc: "duplicate BGPPeer address",
+			config: &configFile{
+				Peers: []peer{
+					{
+						Addr: "1.2.3.4",
+					},
+					{
+						Addr: "1.2.3.4",
+					},
+				},
+			},
+			mustFail: true,
+		},
 	}
 
 	for _, test := range tests {