Repository: nginx/nginx-gateway-fabric

Issue:
Endpoints are not updated correctly by NGF for NGINX Plus
**Describe the bug**
When using NGINX Plus with NGINX Gateway Fabric, the upstreams are applied correctly when HTTPRoute is applied via the API, but when the deployment is scaled, upstreams are removed incorrectly. 

**To Reproduce**
Steps to reproduce the behavior:

1. Deploy NGINX Gateway Fabric with NGINX Plus. 
2.  Port-Forward the traffic for NGF using:

```
kubectl port-forward -n nginx-gateway nginx-gateway-nginx-gateway-fabric-67fd757b54-tlvvc 8765:8765 &
```

3. Apply this [example](https://github.com/nginxinc/nginx-gateway-fabric/tree/main/examples/cafe-example) with updating `cafe.yaml` to 
```
apiVersion: apps/v1
kind: Deployment
metadata:
  name: coffee
spec:
  replicas: 1
  selector:
    matchLabels:
      app: coffee
  template:
    metadata:
      labels:
        app: coffee
    spec:
      containers:
      - name: coffee
        image: nginxdemos/nginx-hello:plain-text
        ports:
        - containerPort: 8080
---
apiVersion: v1
kind: Service
metadata:
  name: coffee
spec:
  ports:
  - port: 80
    targetPort: 8080
    protocol: TCP
    name: http
  selector:
    app: coffee
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: tea
spec:
  replicas: 1
  selector:
    matchLabels:
      app: tea
  template:
    metadata:
      labels:
        app: tea
    spec:
      containers:
      - name: tea
        image: nginxdemos/nginx-hello:plain-text
        ports:
        - containerPort: 8080
        readinessProbe:
          httpGet:
            path: /
            port: 1234
---
apiVersion: v1
kind: Service
metadata:
  name: tea
spec:
  ports:
  - port: 80
    targetPort: 8080
    protocol: TCP
    name: http
  selector:
    app: tea
``` 

4. Check pods and tea pod will not be ready due to the readiness probe at port 1234

```
kubectl get pods

coffee-56b44d4c55-j4wpq   1/1     Running   0          130m
tea-7f9b79bc55-lbz95      0/1     Running   0          130m
```

Check the NGINX Plus [Dashboard](http://localhost:8765/dashboard.html#upstreams) at the forwarded port.  You should see upstreams for both `tea` and `coffee`.

![Screenshot 2024-06-04 at 3 57 14 PM](https://github.com/nginxinc/nginx-gateway-fabric/assets/146118978/d5ee0017-8aa6-43af-b6a9-2696b34bc186)

5. Now, scale the tea deployment

```
kubectl scale deploy tea --replicas=2
```

Check the dashboard again, you will see only upstream for `coffee` and not `tea`.

![Screenshot 2024-06-04 at 4 00 33 PM](https://github.com/nginxinc/nginx-gateway-fabric/assets/146118978/78b609e3-5cf3-4a3e-af92-35b2696e34bd)

When sending a curl request to `tea`, it return 502 which is expected behavior
```
curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/tea
```
```
http://cafe.example.com:$GW_PORT/tea
Handling connection for 8080
<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx/1.25.5</center>
</body>
</html>
```

Check nginx logs 
```
kubectl logs -n nginx-gateway nginx-gateway-nginx-gateway-fabric-67fd757b54-tlvvc -c nginx
```

You should see error about no upstream available
```
2024/06/04 22:03:08 [info] 184#184: *1777 client 127.0.0.1 closed keepalive connection
2024/06/04 22:03:49 [error] 179#179: *1782 no live upstreams while connecting to upstream, client: 127.0.0.1, server: cafe.example.com, request: "GET /tea HTTP/1.1", upstream: "http://default_tea_80/tea", host: "cafe.example.com:8080"
127.0.0.1 - - [04/Jun/2024:22:03:49 +0000] "GET /tea HTTP/1.1" 502 157 "-" "curl/8.4.0"
2024/06/04 22:03:50 [info] 179#179: *1782 client 127.0.0.1 closed keepalive connection
```

5. 
**Expected behavior**
The expected behavior is that upstreams are added correctly when HTTPRoute is added from API as well as when it is scaled. The curl request to tea should return bad gateway, but no errors should be reported in nginx error logs.

**Your environment**
* Version of the NGINX Gateway Fabric - release version 1.2 
* Version of Kubernetes
```
Client Version: version.Info{Major:"1", Minor:"26+", GitVersion:"v1.26.13-dispatcher", GitCommit:"eb237aa977a4a0cf4fcec65fc730b4d96af37ccf", GitTreeState:"clean", BuildDate:"2024-02-08T00:19:39Z", GoVersion:"go1.20.13", Compiler:"gc", Platform:"darwin/arm64"}
Kustomize Version: v4.5.7
Server Version: version.Info{Major:"1", Minor:"30", GitVersion:"v1.30.0", GitCommit:"7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a", GitTreeState:"clean", BuildDate:"2024-05-13T22:02:25Z", GoVersion:"go1.22.2", Compiler:"gc", Platform:"linux/arm64"}
```

* Kubernetes platform - Kind
* Details on how you expose the NGINX Gateway Fabric Pod - Added in reproducing steps 
* Logs of NGINX container: `kubectl -n nginx-gateway logs -l app=nginx-gateway -c nginx`

```
2024/06/04 22:03:08 [info] 184#184: *1777 client 127.0.0.1 closed keepalive connection
2024/06/04 22:03:49 [error] 179#179: *1782 no live upstreams while connecting to upstream, client: 127.0.0.1, server: cafe.example.com, request: "GET /tea HTTP/1.1", upstream: "http://default_tea_80/tea", host: "cafe.example.com:8080"
127.0.0.1 - - [04/Jun/2024:22:03:49 +0000] "GET /tea HTTP/1.1" 502 157 "-" "curl/8.4.0"
2024/06/04 22:03:50 [info] 179#179: *1782 client 127.0.0.1 closed keepalive connection
```

* NGINX Configuration: `kubectl -n nginx-gateway exec <gateway-pod> -c nginx -- nginx [-T`]([ngf.txt](https://github.com/user-attachments/files/15570939/ngf.txt))


**Additional context**
Add any other context about the problem here. Any log files you want to share.


### Acceptance
- The Graceful Recovery NFR test is updated to remove the filter for the error above.

Discussion:
@bjee19: Does this only occur when the readiness probe is specified on the tea deployment? @salonichf5: > Does this only occur when the readiness probe is specified on the tea deployment?

Yes, i think so. Could you reverify the bug?@bjee19: Discussed offline with @salonichf5, concluded that this bug has slightly changed. There is no longer a discrepancy in the upstreams from when they are added via API or scaling. They both do not show up in the NGINX Plus dashboard.

However, the upstreams are not correctly pointing to a 503 status code and that is what should be focused on. Hopefully by throwing a 503 status code, the nginx error will also be resolved.

Solution patch:
diff --git a/internal/controller/nginx/agent/agent.go b/internal/controller/nginx/agent/agent.go
index ea0dba3a..7aba3a81 100644
--- a/internal/controller/nginx/agent/agent.go
+++ b/internal/controller/nginx/agent/agent.go
@@ -15,6 +15,7 @@ import (
 
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/agent/broadcast"
 	agentgrpc "github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/agent/grpc"
+	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/types"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/dataplane"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/resolver"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/status"
@@ -179,6 +180,16 @@ func buildStreamUpstreamServers(upstream dataplane.Upstream) *pb.UpdateStreamSer
 }
 
 func buildUpstreamServers(upstream dataplane.Upstream) []*structpb.Struct {
+	if len(upstream.Endpoints) == 0 {
+		return []*structpb.Struct{
+			{
+				Fields: map[string]*structpb.Value{
+					"server": structpb.NewStringValue(types.Nginx503Server),
+				},
+			},
+		}
+	}
+
 	servers := make([]*structpb.Struct, 0, len(upstream.Endpoints))
 
 	for _, endpoint := range upstream.Endpoints {
diff --git a/internal/controller/nginx/config/upstreams.go b/internal/controller/nginx/config/upstreams.go
index ead41886..c87eb0a8 100644
--- a/internal/controller/nginx/config/upstreams.go
+++ b/internal/controller/nginx/config/upstreams.go
@@ -7,6 +7,7 @@ import (
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/http"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/policies/upstreamsettings"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/stream"
+	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/types"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/dataplane"
 	"github.com/nginx/nginx-gateway-fabric/internal/framework/helpers"
 )
@@ -17,8 +18,6 @@ var (
 )
 
 const (
-	// nginx503Server is used as a backend for services that cannot be resolved (have no IP address).
-	nginx503Server = "unix:/var/run/nginx/nginx-503-server.sock"
 	// nginx500Server is used as a server for the invalid backend ref upstream.
 	nginx500Server = "unix:/var/run/nginx/nginx-500-server.sock"
 	// invalidBackendRef is used as an upstream name for invalid backend references.
@@ -159,7 +158,7 @@ func (g GeneratorImpl) createUpstream(
 			StateFile: stateFile,
 			Servers: []http.UpstreamServer{
 				{
-					Address: nginx503Server,
+					Address: types.Nginx503Server,
 				},
 			},
 		}
diff --git a/internal/controller/nginx/types/types.go b/internal/controller/nginx/types/types.go
new file mode 100644
index 00000000..7d018dce
--- /dev/null
+++ b/internal/controller/nginx/types/types.go
@@ -0,0 +1,6 @@
+package types
+
+const (
+	// Nginx503Server is used as a backend for services that cannot be resolved (have no IP address).
+	Nginx503Server = "unix:/var/run/nginx/nginx-503-server.sock"
+)



Test patch:
diff --git a/internal/controller/nginx/agent/agent_test.go b/internal/controller/nginx/agent/agent_test.go
index 5bc6bb77..73031d05 100644
--- a/internal/controller/nginx/agent/agent_test.go
+++ b/internal/controller/nginx/agent/agent_test.go
@@ -12,6 +12,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/agent/broadcast/broadcastfakes"
+	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/types"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/dataplane"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/resolver"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/status"
@@ -179,6 +180,10 @@ func TestUpdateUpstreamServers(t *testing.T) {
 								},
 							},
 						},
+						{
+							Name:      "empty-upstream",
+							Endpoints: []resolver.Endpoint{},
+						},
 					},
 					StreamUpstreams: []dataplane.Upstream{
 						{
@@ -212,6 +217,20 @@ func TestUpdateUpstreamServers(t *testing.T) {
 							},
 						},
 					},
+					{
+						Action: &pb.NGINXPlusAction_UpdateHttpUpstreamServers{
+							UpdateHttpUpstreamServers: &pb.UpdateHTTPUpstreamServers{
+								HttpUpstreamName: "empty-upstream",
+								Servers: []*structpb.Struct{
+									{
+										Fields: map[string]*structpb.Value{
+											"server": structpb.NewStringValue(types.Nginx503Server),
+										},
+									},
+								},
+							},
+						},
+					},
 					{
 						Action: &pb.NGINXPlusAction_UpdateStreamServers{
 							UpdateStreamServers: &pb.UpdateStreamServers{
@@ -234,13 +253,14 @@ func TestUpdateUpstreamServers(t *testing.T) {
 				g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(0))
 			} else if test.buildUpstreams {
 				g.Expect(deployment.GetNGINXPlusActions()).To(Equal(expActions))
-				g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(2))
+				g.Expect(fakeBroadcaster.SendCallCount()).To(Equal(3))
 			}
 
 			if test.expErr {
 				expErr := errors.Join(
 					fmt.Errorf("couldn't update upstream via the API: %w", testErr),
 					fmt.Errorf("couldn't update upstream via the API: %w", testErr),
+					fmt.Errorf("couldn't update upstream via the API: %w", testErr),
 				)
 
 				g.Expect(deployment.GetLatestUpstreamError()).To(Equal(expErr))
diff --git a/internal/controller/nginx/config/upstreams_test.go b/internal/controller/nginx/config/upstreams_test.go
index 5b522f6f..dbd00aad 100644
--- a/internal/controller/nginx/config/upstreams_test.go
+++ b/internal/controller/nginx/config/upstreams_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/policies"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/policies/upstreamsettings"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/stream"
+	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/types"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/dataplane"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/state/resolver"
 	"github.com/nginx/nginx-gateway-fabric/internal/framework/helpers"
@@ -216,7 +217,7 @@ func TestCreateUpstreams(t *testing.T) {
 			ZoneSize: ossZoneSize,
 			Servers: []http.UpstreamServer{
 				{
-					Address: nginx503Server,
+					Address: types.Nginx503Server,
 				},
 			},
 		},
@@ -277,7 +278,7 @@ func TestCreateUpstream(t *testing.T) {
 				ZoneSize: ossZoneSize,
 				Servers: []http.UpstreamServer{
 					{
-						Address: nginx503Server,
+						Address: types.Nginx503Server,
 					},
 				},
 			},
@@ -293,7 +294,7 @@ func TestCreateUpstream(t *testing.T) {
 				ZoneSize: ossZoneSize,
 				Servers: []http.UpstreamServer{
 					{
-						Address: nginx503Server,
+						Address: types.Nginx503Server,
 					},
 				},
 			},
@@ -581,7 +582,7 @@ func TestCreateUpstreamPlus(t *testing.T) {
 				StateFile: stateDir + "/no-endpoints.conf",
 				Servers: []http.UpstreamServer{
 					{
-						Address: nginx503Server,
+						Address: types.Nginx503Server,
 					},
 				},
 			},
diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go
index c2987610..ef5c4f49 100644
--- a/tests/suite/graceful_recovery_test.go
+++ b/tests/suite/graceful_recovery_test.go
@@ -585,9 +585,6 @@ func getUnexpectedNginxErrorLogs(nginxPodName, namespace string) string {
 		"connect() failed (111: Connection refused)",
 		"could not be resolved (host not found) during usage report",
 		"server returned 429",
-		// FIXME(salonichf5) remove this error message check
-		// when https://github.com/nginx/nginx-gateway-fabric/issues/2090 is completed.
-		"no live upstreams while connecting to upstream",
 	}
 
 	unexpectedErrors := ""