Repository: metallb/metallb
Pull request #2714 (2025-03-10 11:22:20)

Handle IP allocation when only priority is defined
When both namespace and service selectors are absent in ServiceAllocation spec of IPAddressPool, we now default to selecting everything using `labels.Everything()`.

This resolves https://github.com/metallb/metallb/issues/2656

**Is this a BUG FIX or a FEATURE ?**:

> Uncomment only one, leave it on its own line:
>
/kind bug
> /kind cleanup
> /kind feature
> /kind design
> /kind flake
> /kind failing
> /kind documentation
> /kind regression

**What this PR does / why we need it**:
Handle IP allocation when only priority is defined ServiceAllocation spec of IPAddressPool

**Special notes for your reviewer**:
This is as per the discussion in https://github.com/metallb/metallb/issues/2656

```release-note
Bug Fix : When both namespace and service selectors are absent in ServiceAllocation spec of IPAddressPool, we now default to selecting everything using `labels.Everything()` for ServiceSelectors.
```


ashikjm: @oribon Please review. TIA.
ashikjm: @oribon Thanks a lot for the guidance.

Patch:
diff --git a/internal/config/config.go b/internal/config/config.go
index c6d28be1..17f782bf 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -565,6 +565,12 @@ func addressPoolServiceAllocationsFromCR(p metallbv1beta1.IPAddressPool, namespa
 		}
 		poolNamespaces.Insert(poolNs)
 	}
+	serviceAllocations := &ServiceAllocation{Priority: p.Spec.AllocateTo.Priority, Namespaces: poolNamespaces}
+	if len(poolNamespaces) == 0 && len(p.Spec.AllocateTo.NamespaceSelectors) == 0 && len(p.Spec.AllocateTo.ServiceSelectors) == 0 {
+		// If no specific namespaces or service selectors are set, match everything
+		serviceAllocations.ServiceSelectors = []labels.Selector{labels.Everything()}
+		return serviceAllocations, nil
+	}
 	err := validateLabelSelectorDuplicate(p.Spec.AllocateTo.NamespaceSelectors, "namespaceSelectors")
 	if err != nil {
 		return nil, err
@@ -573,7 +579,6 @@ func addressPoolServiceAllocationsFromCR(p metallbv1beta1.IPAddressPool, namespa
 	if err != nil {
 		return nil, err
 	}
-	serviceAllocations := &ServiceAllocation{Priority: p.Spec.AllocateTo.Priority, Namespaces: poolNamespaces}
 	for i := range p.Spec.AllocateTo.NamespaceSelectors {
 		l, err := metav1.LabelSelectorAsSelector(&p.Spec.AllocateTo.NamespaceSelectors[i])
 		if err != nil {
diff --git a/internal/k8s/controllers/layer2_status_controller.go b/internal/k8s/controllers/layer2_status_controller.go
index 1ef426f2..1a53b7d3 100644
--- a/internal/k8s/controllers/layer2_status_controller.go
+++ b/internal/k8s/controllers/layer2_status_controller.go
@@ -14,7 +14,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	utilErrors "k8s.io/apimachinery/pkg/util/errors"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -81,17 +81,31 @@ func (r *Layer2StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	var errs []error
 	if len(ipAdvS) == 0 {
 		for key, item := range serviceL2statuses.Items {
-			if item.Status.Node != r.NodeName {
+			if item.Labels[LabelAnnounceNode] != r.NodeName {
 				continue
 			}
 			if err := r.Client.Delete(ctx, &serviceL2statuses.Items[key]); err != nil && !errors.IsNotFound(err) {
 				errs = append(errs, err)
 			}
 		}
-		if len(errs) > 0 {
-			return ctrl.Result{}, utilErrors.NewAggregate(errs)
+		return ctrl.Result{}, utilerrors.NewAggregate(errs)
+	}
+
+	deleteRedundantErrs := []error{}
+	if len(serviceL2statuses.Items) > 1 {
+		// We shouldn't get here, just in case the controller created redundant resources
+		for i := range serviceL2statuses.Items[1:] {
+			if serviceL2statuses.Items[i+1].Labels[LabelAnnounceNode] != r.NodeName {
+				continue
+			}
+			if err := r.Client.Delete(ctx, &serviceL2statuses.Items[i+1]); err != nil && !errors.IsNotFound(err) {
+				deleteRedundantErrs = append(deleteRedundantErrs, err)
+			}
 		}
-		return ctrl.Result{}, nil
+	}
+
+	if len(deleteRedundantErrs) > 0 {
+		return ctrl.Result{}, utilerrors.NewAggregate(deleteRedundantErrs)
 	}
 
 	// creating a brand new cr
@@ -131,8 +145,10 @@ func (r *Layer2StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	}
 	if result == controllerutil.OperationResultCreated {
 		// According to https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil#CreateOrPatch
-		// If the object is created for the first time, we have to requeue it to ensure that the status is updated.
-		return ctrl.Result{Requeue: true}, nil
+		// If the object is created, we have to patch it again to ensure the status is created.
+		// This will happen when we reconcile the creation event.
+		level.Debug(r.Logger).Log("controller", "Layer2StatusReconciler", "created state", dumpResource(state))
+		return ctrl.Result{}, nil
 	}
 
 	level.Debug(r.Logger).Log("controller", "Layer2StatusReconciler", "updated state", dumpResource(state))
diff --git a/troubleshooting/collect.sh b/troubleshooting/collect.sh
index 7c494e57..ce05ae40 100755
--- a/troubleshooting/collect.sh
+++ b/troubleshooting/collect.sh
@@ -9,7 +9,8 @@ METALLB_NS="${METALLB_NS:-metallb-system}"
 OUTPUT="./report"
 
 function get_metallb_crs() {
-    declare -a METALLB_CRDS=("bgppeers" "bfdprofiles" "bgpAdvertisements" "ipaddresspools" "l2advertisements" "communities")
+    declare -a METALLB_CRDS=("bgppeers" "bfdprofiles" "bgpAdvertisements" "ipaddresspools" 
+    "l2advertisements" "communities" "servicel2statuses" "servicebgpstatuses" "frrconfigurations" "frrnodestates")
     mkdir -p ${OUTPUT}/crds/
 
     for CRD in "${METALLB_CRDS[@]}"; do


Test patch:
diff --git a/e2etest/l2tests/assignment.go b/e2etest/l2tests/assignment.go
index f63d02a1..1cc97c3d 100644
--- a/e2etest/l2tests/assignment.go
+++ b/e2etest/l2tests/assignment.go
@@ -486,6 +486,52 @@ var _ = ginkgo.Describe("IP Assignment", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		ginkgo.It("with only priority", func() {
+			namespacePoolWithLowerPriority := metallbv1beta1.IPAddressPool{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-priority-pool-1"},
+				Spec: metallbv1beta1.IPAddressPoolSpec{
+					Addresses: []string{
+						"192.168.5.0/32",
+					},
+					AllocateTo: &metallbv1beta1.ServiceAllocation{Priority: 20},
+				},
+			}
+			namespacePoolWithHigherPriority := metallbv1beta1.IPAddressPool{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-priority-pool-2"},
+				Spec: metallbv1beta1.IPAddressPoolSpec{
+					Addresses: []string{
+						"192.168.10.0/32",
+					},
+					AllocateTo: &metallbv1beta1.ServiceAllocation{Priority: 10},
+				},
+			}
+			resources := config.Resources{
+				Pools: []metallbv1beta1.IPAddressPool{namespacePoolWithLowerPriority, namespacePoolWithHigherPriority},
+			}
+
+			err := ConfigUpdater.Update(resources)
+			Expect(err).NotTo(HaveOccurred())
+
+			svc1, _ := service.CreateWithBackend(cs, testNamespace, "svc-test-priority-pool-1")
+			svc2, _ := service.CreateWithBackend(cs, testNamespace, "svc-test-priority-pool-2")
+			defer func() {
+				service.Delete(cs, svc1)
+				service.Delete(cs, svc2)
+			}()
+
+			// The createWithBackend method always wait for service to acquire an ingress IP, so
+			// just validate service ingress ip address are assigned from appropriate ip
+			// address pool.
+			ginkgo.By("validate LoadBalancer IP is allocated from 1st higher priority address pool")
+			err = config.ValidateIPInRange([]metallbv1beta1.IPAddressPool{namespacePoolWithHigherPriority}, jigservice.GetIngressPoint(
+				&svc1.Status.LoadBalancer.Ingress[0]))
+			Expect(err).NotTo(HaveOccurred())
+			ginkgo.By("validate LoadBalancer IP is allocated from 2nd higher priority address pool")
+			err = config.ValidateIPInRange([]metallbv1beta1.IPAddressPool{namespacePoolWithLowerPriority}, jigservice.GetIngressPoint(
+				&svc2.Status.LoadBalancer.Ingress[0]))
+			Expect(err).NotTo(HaveOccurred())
+		})
+
 		ginkgo.It("with namespace and namespace labels", func() {
 			namespacePoolWithLowerPriority := metallbv1beta1.IPAddressPool{
 				ObjectMeta: metav1.ObjectMeta{Name: "test-ns-label-pool-1"},
diff --git a/e2etest/pkg/status/l2status.go b/e2etest/pkg/status/l2status.go
index 81ca9ce5..7a666d37 100644
--- a/e2etest/pkg/status/l2status.go
+++ b/e2etest/pkg/status/l2status.go
@@ -34,5 +34,11 @@ func L2ForService(cs client.Client, svc *v1.Service) (*v1beta1.ServiceL2Status,
 	if len(statuses) > 1 {
 		return nil, fmt.Errorf("got more than 1 serviceL2Status object: %d", len(statuses))
 	}
-	return &(statuses[0]), nil
+
+	s := statusList.Items[0]
+	if s.Status.Node == "" {
+		return nil, fmt.Errorf(".Status.Node not populated for service %s/%s, resource: %v", svc.Name, svc.Namespace, s)
+	}
+
+	return &s, nil
 }
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 39866140..b8cbca99 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -635,6 +635,44 @@ func TestParse(t *testing.T) {
 			},
 		},
 
+		{
+			desc: "ip address pool with only priority",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "pool1",
+						},
+						Spec: v1beta1.IPAddressPoolSpec{
+							Addresses: []string{
+								"10.20.0.0/16",
+								"10.50.0.0/24",
+							},
+							AvoidBuggyIPs: true,
+							AutoAssign:    ptr.To(false),
+							AllocateTo:    &v1beta1.ServiceAllocation{Priority: 1},
+						},
+					},
+				},
+			},
+			want: &Config{
+				Pools: &Pools{
+					ByName: map[string]*Pool{
+						"pool1": {
+							Name:               "pool1",
+							CIDR:               []*net.IPNet{ipnet("10.20.0.0/16"), ipnet("10.50.0.0/24")},
+							AvoidBuggyIPs:      true,
+							AutoAssign:         false,
+							ServiceAllocations: &ServiceAllocation{Priority: 1, ServiceSelectors: []labels.Selector{labels.Everything()}},
+						},
+					},
+					ByServiceSelector: []string{"pool1"},
+				},
+				BFDProfiles: map[string]*BFDProfile{},
+				Peers:       map[string]*Peer{},
+			},
+		},
+
 		{
 			desc: "peer-only",
 			crs: ClusterResources{