Repository: metallb/metallb (go)

Issue:
Not announce Service if Endpoint is not available, but service marked to publishNotReadyAddresses 
### MetalLB Version

0.14.3

### Deployment method

Charts

### Main CNI

cilium

### Kubernetes Version

1.28.5

### Cluster Distribution

cluster-api

### Describe the bug

l2 don't announce IPs for Services with publishNotReadyAddresses:true, if pod isn't running fully.

With 0.13.12, it did work and with 0.14.3, it don't work.

### To Reproduce

1. Create a Service with publishNotReadyAddresses: true
2. Start Pod which is not ready until IP is announced and used.


### Expected Behavior

IP get's announced.

### Additional Context

```
{"caller":"layer2_controller.go:82","event":"shouldannounce","ips":["ip"],"level":"debug","message":"failed no active endpoints","pool":"default","protocol":"l2","service":"namespace/name","ts":"2024-03-26T10:51:13Z"}
```

### I've read and agree with the following

- [X] I've checked all open and closed issues and my request is not there.
- [X] I've checked all open and closed pull requests and my request is not there.

### I've read and agree with the following

- [X] I've checked all open and closed issues and my issue is not there.
- [X] This bug is reproducible when deploying MetalLB from the main branch
- [X] I have read the [troubleshooting guide](https://metallb.universe.tf/troubleshooting/) and I am still not able to make it work
- [X] I checked the logs and MetalLB is not discarding the configuration as not valid
- [X] I enabled the debug logs, collected the information required from the cluster using the [collect script](https://raw.githubusercontent.com/metallb/metallb/main/troubleshooting/collect.sh) and will attach them to the issue
- [X] I will provide the definition of my service and the related endpoint slices and attach them to this issue

Pull request description:
Fix the handling of readyness conditions of endpointslices
Fixes #2336

<!-- Thanks for sending a pull request!
1. If this is your first time, please read the [contributing guide](https://metallb.universe.tf/community/#contributing)
2. For non-trivial pull requests, please [file an
   issue](https://github.com/metallb/metallb/issues/new) first, and get
   agreement that the change is a good idea, and a general guideline
   for how it should be implemented, before sending code. Large PRs
   that weren't first discussed and agreed upon in an issue won't be
   accepted.
3. If the PR fixes a particular bug, please include the words "Fixed
   #<issue number>" in the PR text, so that the bug auto-closes when
   the PR is merged.
-->

**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**:

**Special notes for your reviewer**:

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. Follow the instructions for writing a release note from k8s: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If no release note is required, just write "NONE".
-->

```release-note
Fix the handling of readyness conditions of endpointslices: instead of looking at the serving condition first, we look at the ready condition. This allows metallb to respect the publishNotReadyAddresses field of the services.
```


Patch:
diff --git a/internal/k8s/epslices/endpoint_slices.go b/internal/k8s/epslices/endpoint_slices.go
index e4d0c0b6..8dfa8cec 100644
--- a/internal/k8s/epslices/endpoint_slices.go
+++ b/internal/k8s/epslices/endpoint_slices.go
@@ -11,16 +11,15 @@ import (
 
 const SlicesServiceIndexName = "ServiceName"
 
-// IsConditionServing tells if the conditions represent a serving state, deferring
-// to ready state if serving == nil.
-func IsConditionServing(conditions discovery.EndpointConditions) bool {
-	if conditions.Serving == nil {
-		if conditions.Ready == nil {
-			return true
-		}
-		return *conditions.Ready
+// EndpointCanServe tells if the conditions represent a ready state or serving state is ready.
+func EndpointCanServe(conditions discovery.EndpointConditions) bool {
+	if conditions.Ready == nil || *conditions.Ready {
+		return true
 	}
-	return *conditions.Serving
+	if conditions.Serving != nil && *conditions.Serving {
+		return true
+	}
+	return false
 }
 
 func ServiceKeyForSlice(endpointSlice *discovery.EndpointSlice) (types.NamespacedName, error) {
diff --git a/speaker/bgp_controller.go b/speaker/bgp_controller.go
index 5351cd36..244b9ee9 100644
--- a/speaker/bgp_controller.go
+++ b/speaker/bgp_controller.go
@@ -125,13 +125,13 @@ func hasHealthyEndpoint(eps []discovery.EndpointSlice, filterNode func(*string)
 				continue
 			}
 			for _, addr := range ep.Addresses {
-				if _, ok := ready[addr]; !ok && epslices.IsConditionServing(ep.Conditions) {
+				if _, ok := ready[addr]; !ok && epslices.EndpointCanServe(ep.Conditions) {
 					// Only set true if nothing else has expressed an
 					// opinion. This means that false will take precedence
 					// if there's any unready ports for a given endpoint.
 					ready[addr] = true
 				}
-				if !epslices.IsConditionServing(ep.Conditions) {
+				if !epslices.EndpointCanServe(ep.Conditions) {
 					ready[addr] = false
 				}
 			}
diff --git a/speaker/layer2_controller.go b/speaker/layer2_controller.go
index 175ad808..77055934 100644
--- a/speaker/layer2_controller.go
+++ b/speaker/layer2_controller.go
@@ -54,7 +54,7 @@ func usableNodes(eps []discovery.EndpointSlice, speakers map[string]bool) []stri
 	usable := map[string]bool{}
 	for _, slice := range eps {
 		for _, ep := range slice.Endpoints {
-			if !epslices.IsConditionServing(ep.Conditions) {
+			if !epslices.EndpointCanServe(ep.Conditions) {
 				continue
 			}
 			if ep.NodeName == nil {
@@ -204,7 +204,7 @@ func nodesWithActiveSpeakers(speakers map[string]bool) []string {
 func activeEndpointExists(eps []discovery.EndpointSlice) bool {
 	for _, slice := range eps {
 		for _, ep := range slice.Endpoints {
-			if !epslices.IsConditionServing(ep.Conditions) {
+			if !epslices.EndpointCanServe(ep.Conditions) {
 				continue
 			}
 			return true


Test patch:
diff --git a/internal/k8s/epslices/endpoint_slices_test.go b/internal/k8s/epslices/endpoint_slices_test.go
new file mode 100644
index 00000000..4266ec0b
--- /dev/null
+++ b/internal/k8s/epslices/endpoint_slices_test.go
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package epslices
+
+import (
+	"testing"
+
+	v1 "k8s.io/api/discovery/v1"
+	"k8s.io/utils/ptr"
+)
+
+func TestIsConditionReadyOrServing(t *testing.T) {
+	tests := []struct {
+		name       string
+		conditions v1.EndpointConditions
+		want       bool
+	}{
+		{
+			name: "Is Ready",
+			conditions: v1.EndpointConditions{
+				Ready: ptr.To(true),
+			},
+			want: true,
+		},
+		{
+			name: "Is Serving",
+			conditions: v1.EndpointConditions{
+				Serving: ptr.To(true),
+			},
+			want: true,
+		},
+		{
+			name: "Is Ready but not serving",
+			conditions: v1.EndpointConditions{
+				Ready:   ptr.To(true),
+				Serving: ptr.To(false),
+			},
+			want: true,
+		},
+		{
+			name:       "Ready and Serving not set",
+			conditions: v1.EndpointConditions{},
+			want:       true,
+		},
+		{
+			name: "Ready not set and not serving",
+			conditions: v1.EndpointConditions{
+				Serving: ptr.To(false),
+			},
+			want: true,
+		},
+		{
+			name: "Not Ready",
+			conditions: v1.EndpointConditions{
+				Ready: ptr.To(false),
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := EndpointCanServe(tt.conditions); got != tt.want {
+				t.Errorf("EndpointCanServe() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/speaker/bgp_controller_test.go b/speaker/bgp_controller_test.go
index 725b385a..e11f6e53 100644
--- a/speaker/bgp_controller_test.go
+++ b/speaker/bgp_controller_test.go
@@ -617,6 +617,41 @@ func TestBGPSpeakerEPSlices(t *testing.T) {
 			},
 		},
 
+		{
+			desc:     "Endpoint list contains ready but not serving endpoints",
+			balancer: "test1",
+			svc: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Type:                  "LoadBalancer",
+					ExternalTrafficPolicy: "Cluster",
+				},
+				Status: statusAssigned("10.20.30.1"),
+			},
+			eps: []discovery.EndpointSlice{
+				{
+					Endpoints: []discovery.Endpoint{
+						{
+							Addresses: []string{
+								"2.3.4.5",
+							},
+							NodeName: ptr.To("iris"),
+							Conditions: discovery.EndpointConditions{
+								Ready:   ptr.To(true),
+								Serving: ptr.To(false),
+							},
+						},
+					},
+				},
+			},
+			wantAds: map[string][]*bgp.Advertisement{
+				"1.2.3.4:0": {
+					{
+						Prefix: ipnet("10.20.30.1/32"),
+					},
+				},
+			},
+		},
+
 		{
 			desc: "Multiple advertisement config",
 			config: &config.Config{
diff --git a/speaker/layer2_controller_test.go b/speaker/layer2_controller_test.go
index ad640ce6..572fc878 100644
--- a/speaker/layer2_controller_test.go
+++ b/speaker/layer2_controller_test.go
@@ -222,6 +222,37 @@ func TestUsableNodesEPSlices(t *testing.T) {
 			usableSpeakers:  map[string]bool{"iris1": true, "iris2": true},
 			cExpectedResult: []string{"iris1", "iris2"},
 		},
+		{
+			desc: "Two endpoints, different hosts, ready but not serving",
+			eps: []discovery.EndpointSlice{
+				{
+					Endpoints: []discovery.Endpoint{
+						{
+							Addresses: []string{
+								"2.3.4.5",
+							},
+							NodeName: ptr.To("iris1"),
+							Conditions: discovery.EndpointConditions{
+								Ready:   ptr.To(true),
+								Serving: ptr.To(false),
+							},
+						},
+						{
+							Addresses: []string{
+								"2.3.4.15",
+							},
+							NodeName: ptr.To("iris2"),
+							Conditions: discovery.EndpointConditions{
+								Ready:   ptr.To(true),
+								Serving: ptr.To(false),
+							},
+						},
+					},
+				},
+			},
+			usableSpeakers:  map[string]bool{"iris1": true, "iris2": true},
+			cExpectedResult: []string{"iris1", "iris2"},
+		},
 	}
 
 	for _, test := range tests {