From dc2e7754229d6e39e24585e33581a4b3808f62b3 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Thu, 12 Oct 2023 13:00:47 -0600 Subject: [PATCH 1/4] Set Service address in Gateway Status Problem: NGF would only set the Gateway Status to the IP address of the NGF Pod. However, a Service will generally be the entrypoint, so we need to set that address in the status for application developers. Solution: Using a CLI argument to identify the Service that is linked to this NGF Pod, we will get the Service and set the Addresses accordingly. LoadBalancer Service will set to the ingress IP and/or Hostname. If no Service exists or there is an error, then the Pod IP is used. We also shouldn't deploy a Service for conformance tests now, since they could be run in an environment where NodePort doesn't work (like kind). These tests will use the Pod IP. --- cmd/gateway/commands.go | 17 +- cmd/gateway/commands_test.go | 17 ++ conformance/Makefile | 2 - .../provisioner/static-deployment.yaml | 1 + deploy/helm-chart/templates/deployment.yaml | 1 + deploy/helm-chart/templates/rbac.yaml | 1 + deploy/manifests/nginx-gateway.yaml | 2 + docs/cli-help.md | 1 + docs/gateway-api-compatibility.md | 2 +- .../framework/controller/predicate/service.go | 46 +++++ .../controller/predicate/service_test.go | 182 ++++++++++++++++++ internal/framework/status/gateway.go | 109 ++++++++++- internal/framework/status/gateway_test.go | 92 ++++++++- internal/framework/status/statuses.go | 4 + .../status/statusfakes/fake_updater.go | 42 ++++ internal/framework/status/updater.go | 28 ++- internal/framework/status/updater_test.go | 94 ++++++--- internal/mode/provisioner/handler_test.go | 1 - internal/mode/static/build_statuses.go | 19 +- internal/mode/static/build_statuses_test.go | 36 +++- internal/mode/static/config/config.go | 14 +- internal/mode/static/handler.go | 64 ++++-- internal/mode/static/handler_test.go | 59 +++++- internal/mode/static/manager.go | 14 +- 24 files changed, 777 insertions(+), 71 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 20d6f639bd..e35efed9c0 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -69,6 +69,7 @@ func createStaticModeCommand() *cobra.Command { const ( gatewayFlag = "gateway" configFlag = "config" + serviceNameFlag = "service-name" updateGCStatusFlag = "update-gatewayclass-status" metricsDisableFlag = "metrics-disable" metricsSecureFlag = "metrics-secure-serving" @@ -86,6 +87,9 @@ func createStaticModeCommand() *cobra.Command { configName = stringValidatingValue{ validator: validateResourceName, } + serviceName = stringValidatingValue{ + validator: validateResourceName, + } disableMetrics bool metricsSecure bool metricsListenPort = intValidatingValue{ @@ -150,10 +154,14 @@ func createStaticModeCommand() *cobra.Command { Logger: logger, AtomicLevel: atom, GatewayClassName: gatewayClassName.value, - PodIP: podIP, Namespace: namespace, GatewayNsName: gwNsName, UpdateGatewayClassStatus: updateGCStatus, + GatewayPodConfig: config.GatewayPodConfig{ + PodIP: podIP, + ServiceName: serviceName.value, + Namespace: namespace, + }, HealthConfig: config.HealthConfig{ Enabled: !disableHealth, Port: healthListenPort.value, @@ -196,6 +204,13 @@ func createStaticModeCommand() *cobra.Command { ` Lives in the same Namespace as the controller.`, ) + cmd.Flags().Var( + &serviceName, + serviceNameFlag, + `The name of the Service that fronts this NGINX Gateway Fabric Pod.`+ + ` Lives in the same Namespace as the controller.`, + ) + cmd.Flags().BoolVar( &updateGCStatus, updateGCStatusFlag, diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 5f545520ce..f0ac2904dd 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -117,6 +117,7 @@ func TestStaticModeCmdFlagValidation(t *testing.T) { args: []string{ "--gateway=nginx-gateway/nginx", "--config=nginx-gateway-config", + "--service-name=nginx-gateway", "--update-gatewayclass-status=true", "--metrics-port=9114", "--metrics-disable", @@ -166,6 +167,22 @@ func TestStaticModeCmdFlagValidation(t *testing.T) { wantErr: true, expectedErrPrefix: `invalid argument "!@#$" for "-c, --config" flag: invalid format`, }, + { + name: "service-name is set to empty string", + args: []string{ + "--service-name=", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "" for "--service-name" flag: must be set`, + }, + { + name: "service-name is set to invalid string", + args: []string{ + "--service-name=!@#$", + }, + wantErr: true, + expectedErrPrefix: `invalid argument "!@#$" for "--service-name" flag: invalid format`, + }, { name: "update-gatewayclass-status is set to empty string", args: []string{ diff --git a/conformance/Makefile b/conformance/Makefile index 9894ee2dae..4cea342f5f 100644 --- a/conformance/Makefile +++ b/conformance/Makefile @@ -10,7 +10,6 @@ TAG = latest PREFIX = conformance-test-runner NGF_MANIFEST=../deploy/manifests/nginx-gateway.yaml CRDS=../deploy/manifests/crds/ -SERVICE_MANIFEST=../deploy/manifests/service/nodeport.yaml STATIC_MANIFEST=provisioner/static-deployment.yaml PROVISIONER_MANIFEST=provisioner/provisioner.yaml NGINX_IMAGE=$(shell yq '.spec.template.spec.containers[1].image as $$nginx_ver | $$nginx_ver' $(STATIC_MANIFEST)) @@ -52,7 +51,6 @@ prepare-ngf-dependencies: update-ngf-manifest ## Install NGF dependencies on con kubectl wait --for=condition=available --timeout=60s deployment gateway-api-admission-server -n gateway-system kubectl apply -f $(CRDS) kubectl apply -f $(NGF_MANIFEST) - kubectl apply -f $(SERVICE_MANIFEST) .PHONY: deploy-updated-provisioner deploy-updated-provisioner: ## Update provisioner manifest and deploy to the configured kind cluster diff --git a/conformance/provisioner/static-deployment.yaml b/conformance/provisioner/static-deployment.yaml index 0895770db8..3f5c78b241 100644 --- a/conformance/provisioner/static-deployment.yaml +++ b/conformance/provisioner/static-deployment.yaml @@ -27,6 +27,7 @@ spec: - --gateway-ctlr-name=gateway.nginx.org/nginx-gateway-controller - --gatewayclass=nginx - --config=nginx-gateway-config + - --service-name=nginx-gateway - --metrics-disable - --health-port=8081 - --leader-election-lock-name=nginx-gateway-leader-election diff --git a/deploy/helm-chart/templates/deployment.yaml b/deploy/helm-chart/templates/deployment.yaml index d1561cacfa..9e778d735f 100644 --- a/deploy/helm-chart/templates/deployment.yaml +++ b/deploy/helm-chart/templates/deployment.yaml @@ -30,6 +30,7 @@ spec: - --gateway-ctlr-name={{ .Values.nginxGateway.gatewayControllerName }} - --gatewayclass={{ .Values.nginxGateway.gatewayClassName }} - --config={{ include "nginx-gateway.config-name" . }} + - --service-name={{ include "nginx-gateway.fullname" . }} {{- if .Values.metrics.enable }} - --metrics-port={{ .Values.metrics.port }} {{- if .Values.metrics.secure }} diff --git a/deploy/helm-chart/templates/rbac.yaml b/deploy/helm-chart/templates/rbac.yaml index 405c34525e..d1b8dceded 100644 --- a/deploy/helm-chart/templates/rbac.yaml +++ b/deploy/helm-chart/templates/rbac.yaml @@ -21,6 +21,7 @@ rules: - namespaces - services - secrets + - nodes verbs: - list - watch diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 648785e01d..ab26cef1ba 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -32,6 +32,7 @@ rules: - namespaces - services - secrets + - nodes verbs: - list - watch @@ -139,6 +140,7 @@ spec: - --gateway-ctlr-name=gateway.nginx.org/nginx-gateway-controller - --gatewayclass=nginx - --config=nginx-gateway-config + - --service-name=nginx-gateway - --metrics-port=9113 - --health-port=8081 - --leader-election-lock-name=nginx-gateway-leader-election diff --git a/docs/cli-help.md b/docs/cli-help.md index 48869c44c0..deedb57ef8 100644 --- a/docs/cli-help.md +++ b/docs/cli-help.md @@ -20,6 +20,7 @@ Flags: | `gatewayclass` | `string` | The name of the GatewayClass resource. Every NGINX Gateway Fabric must have a unique corresponding GatewayClass resource. | | `gateway` | `string` | The namespaced name of the Gateway resource to use. Must be of the form: `NAMESPACE/NAME`. If not specified, the control plane will process all Gateways for the configured GatewayClass. However, among them, it will choose the oldest resource by creation timestamp. If the timestamps are equal, it will choose the resource that appears first in alphabetical order by {namespace}/{name}. | | `config` | `string` | The name of the NginxGateway resource to be used for this controller's dynamic configuration. Lives in the same Namespace as the controller. | +| `service-name` | `string` | The name of the Service that fronts this NGINX Gateway Fabric Pod. Lives in the same Namespace as the controller. | | `metrics-disable` | `bool` | Disable exposing metrics in the Prometheus format. (default false) | | `metrics-listen-port` | `int` | Sets the port where the Prometheus metrics are exposed. Format: `[1024 - 65535]` (default `9113`) | | `metrics-secure-serving` | `bool` | Configures if the metrics endpoint should be secured using https. Please note that this endpoint will be secured with a self-signed certificate. (default false) | diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md index 488ca4ed61..79d06b6ee8 100644 --- a/docs/gateway-api-compatibility.md +++ b/docs/gateway-api-compatibility.md @@ -91,7 +91,7 @@ Fields: - `allowedRoutes` - supported. - `addresses` - not supported. - `status` - - `addresses` - Pod IPAddress supported. + - `addresses` - supported. - `conditions` - supported (Condition/Status/Reason): - `Accepted/True/Accepted` - `Accepted/True/ListenersNotValid` diff --git a/internal/framework/controller/predicate/service.go b/internal/framework/controller/predicate/service.go index 9ac8a1b1e4..b746cd3959 100644 --- a/internal/framework/controller/predicate/service.go +++ b/internal/framework/controller/predicate/service.go @@ -63,3 +63,49 @@ func (ServicePortsChangedPredicate) Update(e event.UpdateEvent) bool { return len(newPortSet) > 0 } + +// GatewayServicePredicate implements predicate functions for this Pod's Service. +type GatewayServicePredicate struct { + predicate.Funcs +} + +// Update implements the default UpdateEvent filter for the Gateway Service. +func (gsp GatewayServicePredicate) Update(e event.UpdateEvent) bool { + if e.ObjectOld == nil { + return false + } + if e.ObjectNew == nil { + return false + } + + oldSvc, ok := e.ObjectOld.(*apiv1.Service) + if !ok { + return false + } + + newSvc, ok := e.ObjectNew.(*apiv1.Service) + if !ok { + return false + } + + if oldSvc.Spec.Type != newSvc.Spec.Type { + return true + } + + if newSvc.Spec.Type == apiv1.ServiceTypeLoadBalancer { + oldIngress := oldSvc.Status.LoadBalancer.Ingress + newIngress := newSvc.Status.LoadBalancer.Ingress + + if len(oldIngress) != len(newIngress) { + return true + } + + for i, ingress := range oldIngress { + if ingress.IP != newIngress[i].IP || ingress.Hostname != newIngress[i].Hostname { + return true + } + } + } + + return false +} diff --git a/internal/framework/controller/predicate/service_test.go b/internal/framework/controller/predicate/service_test.go index 85051c94f0..faa40e3ef3 100644 --- a/internal/framework/controller/predicate/service_test.go +++ b/internal/framework/controller/predicate/service_test.go @@ -246,3 +246,185 @@ func TestServicePortsChangedPredicate(t *testing.T) { g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue()) g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeTrue()) } + +func TestGatewayServicePredicate_Update(t *testing.T) { + testcases := []struct { + objectOld client.Object + objectNew client.Object + msg string + expUpdate bool + }{ + { + msg: "nil objectOld", + objectOld: nil, + objectNew: &v1.Service{}, + expUpdate: false, + }, + { + msg: "nil objectNew", + objectOld: &v1.Service{}, + objectNew: nil, + expUpdate: false, + }, + { + msg: "non-Service objectOld", + objectOld: &v1.Namespace{}, + objectNew: &v1.Service{}, + expUpdate: false, + }, + { + msg: "non-Service objectNew", + objectOld: &v1.Service{}, + objectNew: &v1.Namespace{}, + expUpdate: false, + }, + { + msg: "something irrelevant changed", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + ClusterIP: "1.2.3.4", + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + ClusterIP: "5.6.7.8", + }, + }, + expUpdate: false, + }, + { + msg: "type changed", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, + }, + expUpdate: true, + }, + { + msg: "ingress changed length", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "1.2.3.4", + }, + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "1.2.3.4", + }, + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + expUpdate: true, + }, + { + msg: "IP address changed", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "1.2.3.4", + }, + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + expUpdate: true, + }, + { + msg: "Hostname changed", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + Hostname: "one", + }, + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + Hostname: "two", + }, + }, + }, + }, + }, + expUpdate: true, + }, + } + + p := GatewayServicePredicate{} + + for _, tc := range testcases { + t.Run(tc.msg, func(t *testing.T) { + g := NewWithT(t) + update := p.Update(event.UpdateEvent{ + ObjectOld: tc.objectOld, + ObjectNew: tc.objectNew, + }) + + g.Expect(update).To(Equal(tc.expUpdate)) + }) + } +} + +func TestGatewayServicePredicate(t *testing.T) { + g := NewWithT(t) + + p := GatewayServicePredicate{} + + g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeTrue()) + g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue()) + g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeTrue()) +} diff --git a/internal/framework/status/gateway.go b/internal/framework/status/gateway.go index 366aaa920b..6f42772cdb 100644 --- a/internal/framework/status/gateway.go +++ b/internal/framework/status/gateway.go @@ -1,16 +1,115 @@ package status import ( + "context" + "fmt" "sort" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ) +// GetGatewayAddresses gets the addresses for the Gateway. +func GetGatewayAddresses( + ctx context.Context, + k8sClient client.Client, + svc *v1.Service, + podConfig config.GatewayPodConfig, +) ([]v1beta1.GatewayStatusAddress, error) { + podAddress := []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: podConfig.PodIP, + }, + } + + var gwSvc v1.Service + if svc == nil { + key := types.NamespacedName{Name: podConfig.ServiceName, Namespace: podConfig.Namespace} + if err := k8sClient.Get(ctx, key, &gwSvc); err != nil { + return podAddress, fmt.Errorf("error finding Service for Gateway: %w", err) + } + } else { + gwSvc = *svc + } + + var addresses, hostnames []string + switch gwSvc.Spec.Type { + case v1.ServiceTypeNodePort: + var err error + addresses, err = getNodeAddresses(ctx, k8sClient) + if err != nil { + return podAddress, fmt.Errorf("error getting Node addresses: %w", err) + } + case v1.ServiceTypeLoadBalancer: + for _, ingress := range gwSvc.Status.LoadBalancer.Ingress { + if ingress.IP != "" { + addresses = append(addresses, ingress.IP) + } else if ingress.Hostname != "" { + hostnames = append(hostnames, ingress.Hostname) + } + } + } + + gwAddresses := make([]v1beta1.GatewayStatusAddress, 0, len(addresses)+len(hostnames)) + for _, addr := range addresses { + statusAddr := v1beta1.GatewayStatusAddress{ + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: addr, + } + gwAddresses = append(gwAddresses, statusAddr) + } + + for _, hostname := range hostnames { + statusAddr := v1beta1.GatewayStatusAddress{ + Type: helpers.GetPointer(v1beta1.HostnameAddressType), + Value: hostname, + } + gwAddresses = append(gwAddresses, statusAddr) + } + + return gwAddresses, nil +} + +func getNodeAddresses( + ctx context.Context, + k8sClient client.Client, +) ([]string, error) { + var nodeList v1.NodeList + if err := k8sClient.List(ctx, &nodeList); err != nil { + return nil, err + } + + nodeAddresses := make([]string, 0, len(nodeList.Items)) + for _, node := range nodeList.Items { + var externalIP, internalIP string + for _, address := range node.Status.Addresses { + if address.Type == v1.NodeExternalIP { + externalIP = address.Address + } + if address.Type == v1.NodeInternalIP { + internalIP = address.Address + } + } + if externalIP != "" { + nodeAddresses = append(nodeAddresses, externalIP) + } else if internalIP != "" { + nodeAddresses = append(nodeAddresses, internalIP) + } + } + + return nodeAddresses, nil +} + // prepareGatewayStatus prepares the status for a Gateway resource. func prepareGatewayStatus( gatewayStatus GatewayStatus, - podIP string, transitionTime metav1.Time, ) v1beta1.GatewayStatus { listenerStatuses := make([]v1beta1.ListenerStatus, 0, len(gatewayStatus.ListenerStatuses)) @@ -34,15 +133,9 @@ func prepareGatewayStatus( }) } - ipAddrType := v1beta1.IPAddressType - gwPodIP := v1beta1.GatewayStatusAddress{ - Type: &ipAddrType, - Value: podIP, - } - return v1beta1.GatewayStatus{ Listeners: listenerStatuses, - Addresses: []v1beta1.GatewayStatusAddress{gwPodIP}, + Addresses: gatewayStatus.Addresses, Conditions: convertConditions(gatewayStatus.Conditions, gatewayStatus.ObservedGeneration, transitionTime), } } diff --git a/internal/framework/status/gateway_test.go b/internal/framework/status/gateway_test.go index b01832915d..4a631f546e 100644 --- a/internal/framework/status/gateway_test.go +++ b/internal/framework/status/gateway_test.go @@ -1,20 +1,105 @@ package status import ( + "context" "testing" "time" . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ) +func TestGetGatewayAddresses(t *testing.T) { + g := NewWithT(t) + + fakeClient := fake.NewFakeClient() + podConfig := config.GatewayPodConfig{ + PodIP: "1.2.3.4", + ServiceName: "my-service", + Namespace: "nginx-gateway", + } + + // no Service exists yet, should get error and Pod Address + addrs, err := GetGatewayAddresses(context.Background(), fakeClient, nil, podConfig) + g.Expect(err).To(HaveOccurred()) + g.Expect(addrs).To(HaveLen(1)) + g.Expect(addrs[0].Value).To(Equal("1.2.3.4")) + + // Create NodePort Service and Nodes + svc := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "nginx-gateway", + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, + } + node1 := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + }, + Status: v1.NodeStatus{ + Addresses: []v1.NodeAddress{ + { + Type: v1.NodeExternalIP, + Address: "172.0.0.1", + }, + }, + }, + } + + node2 := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node2", + }, + Status: v1.NodeStatus{ + Addresses: []v1.NodeAddress{ + { + Type: v1.NodeInternalIP, + Address: "10.10.10.10", + }, + }, + }, + } + + g.Expect(fakeClient.Create(context.Background(), &svc)).To(Succeed()) + g.Expect(fakeClient.Create(context.Background(), &node1)).To(Succeed()) + g.Expect(fakeClient.Create(context.Background(), &node2)).To(Succeed()) + + addrs, err = GetGatewayAddresses(context.Background(), fakeClient, nil, podConfig) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(addrs).To(HaveLen(2)) + g.Expect(addrs[0].Value).To(Equal("172.0.0.1")) + g.Expect(addrs[1].Value).To(Equal("10.10.10.10")) + + // Change to LoadBalancer Service + svc.Spec.Type = v1.ServiceTypeLoadBalancer + svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{ + { + IP: "34.35.36.37", + }, + { + Hostname: "myhost", + }, + } + + addrs, err = GetGatewayAddresses(context.Background(), fakeClient, &svc, podConfig) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(addrs).To(HaveLen(2)) + g.Expect(addrs[0].Value).To(Equal("34.35.36.37")) + g.Expect(addrs[1].Value).To(Equal("myhost")) +} + func TestPrepareGatewayStatus(t *testing.T) { - ipAddrType := v1beta1.IPAddressType podIP := v1beta1.GatewayStatusAddress{ - Type: &ipAddrType, + Type: helpers.GetPointer(v1beta1.IPAddressType), Value: "1.2.3.4", } status := GatewayStatus{ @@ -30,6 +115,7 @@ func TestPrepareGatewayStatus(t *testing.T) { }, }, }, + Addresses: []v1beta1.GatewayStatusAddress{podIP}, ObservedGeneration: 1, } @@ -54,6 +140,6 @@ func TestPrepareGatewayStatus(t *testing.T) { g := NewWithT(t) - result := prepareGatewayStatus(status, "1.2.3.4", transitionTime) + result := prepareGatewayStatus(status, transitionTime) g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } diff --git a/internal/framework/status/statuses.go b/internal/framework/status/statuses.go index 89284f1661..9af0108aa7 100644 --- a/internal/framework/status/statuses.go +++ b/internal/framework/status/statuses.go @@ -57,8 +57,12 @@ type GatewayStatus struct { ListenerStatuses ListenerStatuses // Conditions is the list of conditions for this Gateway. Conditions []conditions.Condition + // Addresses holds the list of GatewayStatusAddresses. + Addresses []v1beta1.GatewayStatusAddress // ObservedGeneration is the generation of the resource that was processed. ObservedGeneration int64 + // Ignored tells whether or not this Gateway is ignored. + Ignored bool } // ListenerStatus holds the status-related information about a listener in the Gateway resource. diff --git a/internal/framework/status/statusfakes/fake_updater.go b/internal/framework/status/statusfakes/fake_updater.go index 6fcc488c62..c826942085 100644 --- a/internal/framework/status/statusfakes/fake_updater.go +++ b/internal/framework/status/statusfakes/fake_updater.go @@ -6,6 +6,7 @@ import ( "sync" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" + v1 "k8s.io/api/core/v1" ) type FakeUpdater struct { @@ -24,6 +25,12 @@ type FakeUpdater struct { arg1 context.Context arg2 status.Status } + UpdateAddressesStub func(context.Context, *v1.Service) + updateAddressesMutex sync.RWMutex + updateAddressesArgsForCall []struct { + arg1 context.Context + arg2 *v1.Service + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -117,6 +124,39 @@ func (fake *FakeUpdater) UpdateArgsForCall(i int) (context.Context, status.Statu return argsForCall.arg1, argsForCall.arg2 } +func (fake *FakeUpdater) UpdateAddresses(arg1 context.Context, arg2 *v1.Service) { + fake.updateAddressesMutex.Lock() + fake.updateAddressesArgsForCall = append(fake.updateAddressesArgsForCall, struct { + arg1 context.Context + arg2 *v1.Service + }{arg1, arg2}) + stub := fake.UpdateAddressesStub + fake.recordInvocation("UpdateAddresses", []interface{}{arg1, arg2}) + fake.updateAddressesMutex.Unlock() + if stub != nil { + fake.UpdateAddressesStub(arg1, arg2) + } +} + +func (fake *FakeUpdater) UpdateAddressesCallCount() int { + fake.updateAddressesMutex.RLock() + defer fake.updateAddressesMutex.RUnlock() + return len(fake.updateAddressesArgsForCall) +} + +func (fake *FakeUpdater) UpdateAddressesCalls(stub func(context.Context, *v1.Service)) { + fake.updateAddressesMutex.Lock() + defer fake.updateAddressesMutex.Unlock() + fake.UpdateAddressesStub = stub +} + +func (fake *FakeUpdater) UpdateAddressesArgsForCall(i int) (context.Context, *v1.Service) { + fake.updateAddressesMutex.RLock() + defer fake.updateAddressesMutex.RUnlock() + argsForCall := fake.updateAddressesArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + func (fake *FakeUpdater) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() @@ -126,6 +166,8 @@ func (fake *FakeUpdater) Invocations() map[string][][]interface{} { defer fake.enableMutex.RUnlock() fake.updateMutex.RLock() defer fake.updateMutex.RUnlock() + fake.updateAddressesMutex.RLock() + defer fake.updateAddressesMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index d14bebfa47..fb0eadef82 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -8,6 +8,7 @@ import ( "time" "github.com/go-logr/logr" + v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" @@ -16,6 +17,7 @@ import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ) //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Updater @@ -27,6 +29,8 @@ import ( type Updater interface { // Update updates the statuses of the resources. Update(context.Context, Status) + // UpdateAddresses updates the Gateway Addresses when the Gateway Service changes. + UpdateAddresses(context.Context, *v1.Service) // Enable enables status updates. The updater will update the statuses in Kubernetes API to ensure they match the // statuses of the last Update invocation. Enable(ctx context.Context) @@ -40,14 +44,14 @@ type UpdaterConfig struct { Client client.Client // Clock is used as a source of time for the LastTransitionTime field in Conditions in resource statuses. Clock Clock + // GatewayPodConfig contains information about this Pod. + GatewayPodConfig config.GatewayPodConfig // Logger holds a logger to be used. Logger logr.Logger // GatewayCtlrName is the name of the Gateway controller. GatewayCtlrName string // GatewayClassName is the name of the GatewayClass resource. GatewayClassName string - // PodIP is the IP address of this Pod. - PodIP string // UpdateGatewayClassStatus enables updating the status of the GatewayClass resource. UpdateGatewayClassStatus bool // LeaderElectionEnabled indicates whether Leader Election is enabled. @@ -200,7 +204,7 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP } upd.writeStatuses(ctx, nsname, &v1beta1.Gateway{}, func(object client.Object) { gw := object.(*v1beta1.Gateway) - gw.Status = prepareGatewayStatus(gs, upd.cfg.PodIP, upd.cfg.Clock.Now()) + gw.Status = prepareGatewayStatus(gs, upd.cfg.Clock.Now()) }) } @@ -250,6 +254,24 @@ func (upd *UpdaterImpl) writeStatuses( } } +// UpdateAddresses is called when the Gateway Status needs its addresses updated. +func (upd *UpdaterImpl) UpdateAddresses(ctx context.Context, svc *v1.Service) { + addresses, err := GetGatewayAddresses(ctx, upd.cfg.Client, svc, upd.cfg.GatewayPodConfig) + if err != nil { + upd.cfg.Logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") + } + + for name, status := range upd.lastStatuses.gatewayAPI.GatewayStatuses { + if status.Ignored { + continue + } + status.Addresses = addresses + upd.lastStatuses.gatewayAPI.GatewayStatuses[name] = status + } + + upd.Update(ctx, upd.lastStatuses.gatewayAPI) +} + // NewRetryUpdateFunc returns a function which will be used in wait.ExponentialBackoffWithContext. // The function will attempt to Update a kubernetes resource and will be retried in // wait.ExponentialBackoffWithContext if an error occurs. Exported for testing purposes. diff --git a/internal/framework/status/updater_test.go b/internal/framework/status/updater_test.go index 30b8d4127d..49e2189984 100644 --- a/internal/framework/status/updater_test.go +++ b/internal/framework/status/updater_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -19,6 +20,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) @@ -73,9 +75,8 @@ var _ = Describe("Updater", func() { gw, ignoredGw *v1beta1.Gateway hr *v1beta1.HTTPRoute ng *ngfAPI.NginxGateway - ipAddrType = v1beta1.IPAddressType addr = v1beta1.GatewayStatusAddress{ - Type: &ipAddrType, + Type: helpers.GetPointer(v1beta1.IPAddressType), Value: "1.2.3.4", } @@ -97,11 +98,13 @@ var _ = Describe("Updater", func() { SupportedKinds: []v1beta1.RouteGroupKind{{Kind: "HTTPRoute"}}, }, }, + Addresses: []v1beta1.GatewayStatusAddress{addr}, ObservedGeneration: gens.gateways, }, {Namespace: "test", Name: "ignored-gateway"}: { Conditions: staticConds.NewGatewayConflict(), ObservedGeneration: 1, + Ignored: true, }, }, HTTPRouteStatuses: status.HTTPRouteStatuses{ @@ -199,7 +202,6 @@ var _ = Describe("Updater", func() { Message: staticConds.GatewayMessageGatewayConflict, }, }, - Addresses: []v1beta1.GatewayStatusAddress{addr}, }, } } @@ -251,12 +253,14 @@ var _ = Describe("Updater", func() { BeforeAll(func() { updater = status.NewUpdater(status.UpdaterConfig{ - GatewayCtlrName: gatewayCtrlName, - GatewayClassName: gcName, - Client: client, - Logger: zap.New(), - Clock: fakeClock, - PodIP: "1.2.3.4", + GatewayCtlrName: gatewayCtrlName, + GatewayClassName: gcName, + Client: client, + Logger: zap.New(), + Clock: fakeClock, + GatewayPodConfig: config.GatewayPodConfig{ + PodIP: "1.2.3.4", + }, UpdateGatewayClassStatus: true, }) @@ -398,6 +402,44 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedNG, latestNG)).To(BeEmpty()) }) + When("the Gateway Service is updated with a new address", func() { + svc := &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "5.6.7.8", + }, + }, + }, + }, + } + + AfterEach(func() { + // reset the IP for the remaining tests + svc.Status.LoadBalancer.Ingress[0].IP = "1.2.3.4" + updater.UpdateAddresses(context.Background(), svc) + }) + + It("should update the previous Gateway statuses with new address", func() { + latestGw := &v1beta1.Gateway{} + expectedGw := createExpectedGwWithGeneration(1) + expectedGw.Status.Addresses[0].Value = "5.6.7.8" + + updater.UpdateAddresses(context.Background(), svc) + + err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) + Expect(err).ToNot(HaveOccurred()) + + expectedGw.ResourceVersion = latestGw.ResourceVersion + + Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) + }) + }) + It("should not update Gateway API statuses with canceled context - function normally returns", func() { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -646,9 +688,9 @@ var _ = Describe("Updater", func() { ) Expect(err).ToNot(HaveOccurred()) - // Before this test there were 4 updates to the Gateway resource. - // So now the resource version should equal 24. - Expect(latestGw.ResourceVersion).To(Equal("24")) + // Before this test there were 6 updates to the Gateway resource. + // So now the resource version should equal 26. + Expect(latestGw.ResourceVersion).To(Equal("26")) }) }) }) @@ -661,12 +703,14 @@ var _ = Describe("Updater", func() { BeforeAll(func() { updater = status.NewUpdater(status.UpdaterConfig{ - GatewayCtlrName: gatewayCtrlName, - GatewayClassName: gcName, - Client: client, - Logger: zap.New(), - Clock: fakeClock, - PodIP: "1.2.3.4", + GatewayCtlrName: gatewayCtrlName, + GatewayClassName: gcName, + Client: client, + Logger: zap.New(), + Clock: fakeClock, + GatewayPodConfig: config.GatewayPodConfig{ + PodIP: "1.2.3.4", + }, UpdateGatewayClassStatus: false, }) @@ -710,12 +754,14 @@ var _ = Describe("Updater", func() { Describe("Edge cases", func() { It("panics on update if status type is unknown", func() { updater := status.NewUpdater(status.UpdaterConfig{ - GatewayCtlrName: gatewayCtrlName, - GatewayClassName: gcName, - Client: client, - Logger: zap.New(), - Clock: fakeClock, - PodIP: "1.2.3.4", + GatewayCtlrName: gatewayCtrlName, + GatewayClassName: gcName, + Client: client, + Logger: zap.New(), + Clock: fakeClock, + GatewayPodConfig: config.GatewayPodConfig{ + PodIP: "1.2.3.4", + }, UpdateGatewayClassStatus: true, }) diff --git a/internal/mode/provisioner/handler_test.go b/internal/mode/provisioner/handler_test.go index da61348413..da29a9c1a2 100644 --- a/internal/mode/provisioner/handler_test.go +++ b/internal/mode/provisioner/handler_test.go @@ -60,7 +60,6 @@ var _ = Describe("handler", func() { Logger: zap.New(), GatewayCtlrName: "test.example.com", GatewayClassName: gcName, - PodIP: "1.2.3.4", UpdateGatewayClassStatus: true, }) }) diff --git a/internal/mode/static/build_statuses.go b/internal/mode/static/build_statuses.go index 8a51dd4cbd..2a009f5580 100644 --- a/internal/mode/static/build_statuses.go +++ b/internal/mode/static/build_statuses.go @@ -16,14 +16,18 @@ type nginxReloadResult struct { } // buildGatewayAPIStatuses builds status.Statuses from a Graph. -func buildGatewayAPIStatuses(graph *graph.Graph, nginxReloadRes nginxReloadResult) status.GatewayAPIStatuses { +func buildGatewayAPIStatuses( + graph *graph.Graph, + gwAddresses []v1beta1.GatewayStatusAddress, + nginxReloadRes nginxReloadResult, +) status.GatewayAPIStatuses { statuses := status.GatewayAPIStatuses{ HTTPRouteStatuses: make(status.HTTPRouteStatuses), } statuses.GatewayClassStatuses = buildGatewayClassStatuses(graph.GatewayClass, graph.IgnoredGatewayClasses) - statuses.GatewayStatuses = buildGatewayStatuses(graph.Gateway, graph.IgnoredGateways, nginxReloadRes) + statuses.GatewayStatuses = buildGatewayStatuses(graph.Gateway, graph.IgnoredGateways, gwAddresses, nginxReloadRes) for nsname, r := range graph.Routes { parentStatuses := make([]status.ParentStatus, 0, len(r.ParentRefs)) @@ -105,25 +109,31 @@ func buildGatewayClassStatuses( func buildGatewayStatuses( gateway *graph.Gateway, ignoredGateways map[types.NamespacedName]*v1beta1.Gateway, + gwAddresses []v1beta1.GatewayStatusAddress, nginxReloadRes nginxReloadResult, ) status.GatewayStatuses { statuses := make(status.GatewayStatuses) if gateway != nil { - statuses[client.ObjectKeyFromObject(gateway.Source)] = buildGatewayStatus(gateway, nginxReloadRes) + statuses[client.ObjectKeyFromObject(gateway.Source)] = buildGatewayStatus(gateway, gwAddresses, nginxReloadRes) } for nsname, gw := range ignoredGateways { statuses[nsname] = status.GatewayStatus{ Conditions: staticConds.NewGatewayConflict(), ObservedGeneration: gw.Generation, + Ignored: true, } } return statuses } -func buildGatewayStatus(gateway *graph.Gateway, nginxReloadRes nginxReloadResult) status.GatewayStatus { +func buildGatewayStatus( + gateway *graph.Gateway, + gwAddresses []v1beta1.GatewayStatusAddress, + nginxReloadRes nginxReloadResult, +) status.GatewayStatus { if !gateway.Valid { return status.GatewayStatus{ Conditions: staticConds.DeduplicateConditions(gateway.Conditions), @@ -175,6 +185,7 @@ func buildGatewayStatus(gateway *graph.Gateway, nginxReloadRes nginxReloadResult return status.GatewayStatus{ Conditions: staticConds.DeduplicateConditions(gwConds), ListenerStatuses: listenerStatuses, + Addresses: gwAddresses, ObservedGeneration: gateway.Source.Generation, } } diff --git a/internal/mode/static/build_statuses_test.go b/internal/mode/static/build_statuses_test.go index 77d803840a..74010237be 100644 --- a/internal/mode/static/build_statuses_test.go +++ b/internal/mode/static/build_statuses_test.go @@ -36,6 +36,13 @@ var ( ) func TestBuildStatuses(t *testing.T) { + addr := []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: "1.2.3.4", + }, + } + invalidRouteCondition := conditions.Condition{ Type: "TestInvalidRoute", Status: metav1.ConditionTrue, @@ -151,11 +158,13 @@ func TestBuildStatuses(t *testing.T) { Conditions: staticConds.NewDefaultListenerConditions(), }, }, + Addresses: addr, ObservedGeneration: 2, }, {Namespace: "test", Name: "ignored-gateway"}: { Conditions: staticConds.NewGatewayConflict(), ObservedGeneration: 1, + Ignored: true, }, }, HTTPRouteStatuses: status.HTTPRouteStatuses{ @@ -196,11 +205,18 @@ func TestBuildStatuses(t *testing.T) { g := NewWithT(t) var nginxReloadRes nginxReloadResult - result := buildGatewayAPIStatuses(graph, nginxReloadRes) + result := buildGatewayAPIStatuses(graph, addr, nginxReloadRes) g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } func TestBuildStatusesNginxErr(t *testing.T) { + addr := []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: "1.2.3.4", + }, + } + routes := map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "hr-valid"}: { Valid: true, @@ -265,6 +281,7 @@ func TestBuildStatusesNginxErr(t *testing.T) { }, }, }, + Addresses: addr, ObservedGeneration: 2, }, }, @@ -288,7 +305,7 @@ func TestBuildStatusesNginxErr(t *testing.T) { g := NewWithT(t) nginxReloadRes := nginxReloadResult{error: errors.New("test error")} - result := buildGatewayAPIStatuses(graph, nginxReloadRes) + result := buildGatewayAPIStatuses(graph, addr, nginxReloadRes) g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } @@ -358,6 +375,13 @@ func TestBuildGatewayClassStatuses(t *testing.T) { } func TestBuildGatewayStatuses(t *testing.T) { + addr := []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: "1.2.3.4", + }, + } + tests := []struct { nginxReloadRes nginxReloadResult gateway *graph.Gateway @@ -387,10 +411,12 @@ func TestBuildGatewayStatuses(t *testing.T) { {Namespace: "test", Name: "ignored-1"}: { Conditions: staticConds.NewGatewayConflict(), ObservedGeneration: 1, + Ignored: true, }, {Namespace: "test", Name: "ignored-2"}: { Conditions: staticConds.NewGatewayConflict(), ObservedGeneration: 2, + Ignored: true, }, }, }, @@ -427,6 +453,7 @@ func TestBuildGatewayStatuses(t *testing.T) { Conditions: staticConds.NewDefaultListenerConditions(), }, }, + Addresses: addr, ObservedGeneration: 2, }, }, @@ -464,6 +491,7 @@ func TestBuildGatewayStatuses(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedValue("unsupported value"), }, }, + Addresses: addr, ObservedGeneration: 2, }, }, @@ -495,6 +523,7 @@ func TestBuildGatewayStatuses(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedValue("unsupported value"), }, }, + Addresses: addr, ObservedGeneration: 2, }, }, @@ -547,6 +576,7 @@ func TestBuildGatewayStatuses(t *testing.T) { }, }, }, + Addresses: addr, ObservedGeneration: 2, }, }, @@ -558,7 +588,7 @@ func TestBuildGatewayStatuses(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - result := buildGatewayStatuses(test.gateway, test.ignoredGateways, test.nginxReloadRes) + result := buildGatewayStatuses(test.gateway, test.ignoredGateways, addr, test.nginxReloadRes) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) }) } diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index 0a6910afc7..45134a33ad 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -12,6 +12,8 @@ type Config struct { // GatewayNsName is the namespaced name of a Gateway resource that the Gateway will use. // The Gateway will ignore all other Gateway resources. GatewayNsName *types.NamespacedName + // GatewayPodConfig contains information about this Pod. + GatewayPodConfig GatewayPodConfig // Logger is the Zap Logger used by all components. Logger logr.Logger // GatewayCtlrName is the name of this controller. @@ -20,8 +22,6 @@ type Config struct { ConfigName string // GatewayClassName is the name of the GatewayClass resource that the Gateway will use. GatewayClassName string - // PodIP is the IP address of this Pod. - PodIP string // Namespace is the Namespace of this Pod. Namespace string // LeaderElection contains the configuration for leader election. @@ -34,6 +34,16 @@ type Config struct { HealthConfig HealthConfig } +// GatewayPodConfig contains information about this Pod. +type GatewayPodConfig struct { + // PodIP is the IP address of this Pod. + PodIP string + // ServiceName is the name of the Service that fronts this Pod. + ServiceName string + // Namespace is the namespace of this Pod. + Namespace string +} + // MetricsConfig specifies the metrics config. type MetricsConfig struct { // Port is the port the metrics should be exposed on. diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 71f24672b4..9248dbe516 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -15,6 +15,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" + ngfConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime" @@ -30,6 +31,10 @@ type handlerMetricsCollector interface { // eventHandlerConfig holds configuration parameters for eventHandlerImpl. type eventHandlerConfig struct { + // k8sClient is a Kubernetes API client + k8sClient client.Client + // gatewayPodConfig contains information about this Pod. + gatewayPodConfig ngfConfig.GatewayPodConfig // processor is the state ChangeProcessor. processor state.ChangeProcessor // serviceResolver resolves Services to Endpoints. @@ -86,22 +91,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log }() for _, event := range batch { - switch e := event.(type) { - case *events.UpsertEvent: - if cfg, ok := e.Resource.(*ngfAPI.NginxGateway); ok { - h.updateControlPlaneAndSetStatus(ctx, logger, cfg) - } else { - h.cfg.processor.CaptureUpsertChange(e.Resource) - } - case *events.DeleteEvent: - if _, ok := e.Type.(*ngfAPI.NginxGateway); ok { - h.updateControlPlaneAndSetStatus(ctx, logger, nil) - } else { - h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) - } - default: - panic(fmt.Errorf("unknown event type %T", e)) - } + h.handleEvent(ctx, logger, event) } changed, graph := h.cfg.processor.Process() @@ -131,7 +121,47 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log } } - h.cfg.statusUpdater.Update(ctx, buildGatewayAPIStatuses(graph, nginxReloadRes)) + gwAddresses, err := status.GetGatewayAddresses(ctx, h.cfg.k8sClient, nil, h.cfg.gatewayPodConfig) + if err != nil { + logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") + } + + h.cfg.statusUpdater.Update(ctx, buildGatewayAPIStatuses(graph, gwAddresses, nginxReloadRes)) +} + +func (h *eventHandlerImpl) handleEvent(ctx context.Context, logger logr.Logger, event interface{}) { + switch e := event.(type) { + case *events.UpsertEvent: + switch obj := e.Resource.(type) { + case *ngfAPI.NginxGateway: + h.updateControlPlaneAndSetStatus(ctx, logger, obj) + case *apiv1.Service: + podConfig := h.cfg.gatewayPodConfig + if obj.Name == podConfig.ServiceName && obj.Namespace == podConfig.Namespace { + h.cfg.statusUpdater.UpdateAddresses(ctx, obj) + } else { + h.cfg.processor.CaptureUpsertChange(e.Resource) + } + default: + h.cfg.processor.CaptureUpsertChange(e.Resource) + } + case *events.DeleteEvent: + switch e.Type.(type) { + case *ngfAPI.NginxGateway: + h.updateControlPlaneAndSetStatus(ctx, logger, nil) + case *apiv1.Service: + podConfig := h.cfg.gatewayPodConfig + if e.NamespacedName.Name == podConfig.ServiceName && e.NamespacedName.Namespace == podConfig.Namespace { + h.cfg.statusUpdater.UpdateAddresses(ctx, nil) + } else { + h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) + } + default: + h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) + } + default: + panic(fmt.Errorf("unknown event type %T", e)) + } } func (h *eventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Configuration) error { diff --git a/internal/mode/static/handler_test.go b/internal/mode/static/handler_test.go index 94aad2fb40..6045876707 100644 --- a/internal/mode/static/handler_test.go +++ b/internal/mode/static/handler_test.go @@ -7,9 +7,11 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "go.uber.org/zap" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ctlrZap "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -19,6 +21,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/configfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" @@ -67,6 +70,7 @@ var _ = Describe("eventHandler", func() { fakeEventRecorder = record.NewFakeRecorder(1) handler = newEventHandlerImpl(eventHandlerConfig{ + k8sClient: fake.NewFakeClient(), processor: fakeProcessor, generator: fakeGenerator, logLevelSetter: newZapLogLevelSetter(zap.NewAtomicLevel()), @@ -76,7 +80,11 @@ var _ = Describe("eventHandler", func() { eventRecorder: fakeEventRecorder, healthChecker: &healthChecker{}, controlConfigNSName: types.NamespacedName{Namespace: namespace, Name: configName}, - metricsCollector: collectors.NewControllerNoopCollector(), + gatewayPodConfig: config.GatewayPodConfig{ + ServiceName: "nginx-gateway", + Namespace: "nginx-gateway", + }, + metricsCollector: collectors.NewControllerNoopCollector(), }) Expect(handler.cfg.healthChecker.ready).To(BeFalse()) }) @@ -221,6 +229,55 @@ var _ = Describe("eventHandler", func() { }) }) + When("receiving Service updates", func() { + It("should not call UpdateAddresses if the Service is not for the Gateway Pod", func() { + e := &events.UpsertEvent{Resource: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "not-nginx-gateway", + }, + }} + batch := []interface{}{e} + + handler.HandleEventBatch(context.Background(), ctlrZap.New(), batch) + + Expect(fakeStatusUpdater.UpdateAddressesCallCount()).To(BeZero()) + + de := &events.DeleteEvent{Type: &v1.Service{}} + batch = []interface{}{de} + + handler.HandleEventBatch(context.Background(), ctlrZap.New(), batch) + + Expect(fakeStatusUpdater.UpdateAddressesCallCount()).To(BeZero()) + }) + + It("should update the addresses when the Gateway Service is upserted", func() { + e := &events.UpsertEvent{Resource: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-gateway", + Namespace: "nginx-gateway", + }, + }} + batch := []interface{}{e} + + handler.HandleEventBatch(context.Background(), ctlrZap.New(), batch) + Expect(fakeStatusUpdater.UpdateAddressesCallCount()).ToNot(BeZero()) + }) + + It("should update the addresses when the Gateway Service is deleted", func() { + e := &events.DeleteEvent{ + Type: &v1.Service{}, + NamespacedName: types.NamespacedName{ + Name: "nginx-gateway", + Namespace: "nginx-gateway", + }, + } + batch := []interface{}{e} + + handler.HandleEventBatch(context.Background(), ctlrZap.New(), batch) + Expect(fakeStatusUpdater.UpdateAddressesCallCount()).ToNot(BeZero()) + }) + }) + It("should set the health checker status properly when there are changes", func() { e := &events.UpsertEvent{Resource: &v1beta1.HTTPRoute{}} batch := []interface{}{e} diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 90ec48275b..82d46f8539 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -157,14 +157,15 @@ func StartManager(cfg config.Config) error { GatewayCtlrName: cfg.GatewayCtlrName, GatewayClassName: cfg.GatewayClassName, Client: mgr.GetClient(), - PodIP: cfg.PodIP, Logger: cfg.Logger.WithName("statusUpdater"), Clock: status.NewRealClock(), UpdateGatewayClassStatus: cfg.UpdateGatewayClassStatus, LeaderElectionEnabled: cfg.LeaderElection.Enabled, + GatewayPodConfig: cfg.GatewayPodConfig, }) eventHandler := newEventHandlerImpl(eventHandlerConfig{ + k8sClient: mgr.GetClient(), processor: processor, serviceResolver: resolver.NewServiceResolverImpl(mgr.GetClient()), generator: ngxcfg.NewGeneratorImpl(), @@ -178,6 +179,7 @@ func StartManager(cfg config.Config) error { eventRecorder: recorder, healthChecker: hc, controlConfigNSName: controlConfigNSName, + gatewayPodConfig: cfg.GatewayPodConfig, metricsCollector: handlerCollector, }) @@ -268,6 +270,16 @@ func registerControllers( controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}), }, }, + { + objectType: &apiv1.Service{}, + options: func() []controller.Option { + svcNSName := types.NamespacedName{Namespace: cfg.Namespace, Name: cfg.GatewayPodConfig.ServiceName} + return []controller.Option{ + controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(svcNSName)), + controller.WithK8sPredicate(predicate.GatewayServicePredicate{}), + } + }(), + }, { objectType: &apiv1.Secret{}, }, From 73049cbbdf541e5aaa7554aab2a761d907694a68 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 16 Oct 2023 13:03:35 -0600 Subject: [PATCH 2/4] First round of reviews --- cmd/gateway/commands.go | 5 +- cmd/gateway/commands_test.go | 14 +-- .../provisioner/static-deployment.yaml | 2 +- deploy/helm-chart/templates/deployment.yaml | 2 +- deploy/manifests/nginx-gateway.yaml | 2 +- docs/cli-help.md | 2 +- docs/gateway-api-compatibility.md | 4 +- docs/installation.md | 5 + internal/framework/status/gateway.go | 100 ------------------ internal/framework/status/gateway_test.go | 86 --------------- .../status/statusfakes/fake_updater.go | 23 ++-- internal/framework/status/updater.go | 12 +-- internal/framework/status/updater_test.go | 31 +++--- internal/mode/static/config/config.go | 2 - internal/mode/static/handler.go | 78 +++++++++++++- internal/mode/static/handler_test.go | 48 +++++++++ internal/mode/static/manager.go | 6 +- 17 files changed, 174 insertions(+), 248 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index e35efed9c0..43c9974e1a 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -69,7 +69,7 @@ func createStaticModeCommand() *cobra.Command { const ( gatewayFlag = "gateway" configFlag = "config" - serviceNameFlag = "service-name" + serviceFlag = "service" updateGCStatusFlag = "update-gatewayclass-status" metricsDisableFlag = "metrics-disable" metricsSecureFlag = "metrics-secure-serving" @@ -154,7 +154,6 @@ func createStaticModeCommand() *cobra.Command { Logger: logger, AtomicLevel: atom, GatewayClassName: gatewayClassName.value, - Namespace: namespace, GatewayNsName: gwNsName, UpdateGatewayClassStatus: updateGCStatus, GatewayPodConfig: config.GatewayPodConfig{ @@ -206,7 +205,7 @@ func createStaticModeCommand() *cobra.Command { cmd.Flags().Var( &serviceName, - serviceNameFlag, + serviceFlag, `The name of the Service that fronts this NGINX Gateway Fabric Pod.`+ ` Lives in the same Namespace as the controller.`, ) diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index f0ac2904dd..1b8302def4 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -117,7 +117,7 @@ func TestStaticModeCmdFlagValidation(t *testing.T) { args: []string{ "--gateway=nginx-gateway/nginx", "--config=nginx-gateway-config", - "--service-name=nginx-gateway", + "--service=nginx-gateway", "--update-gatewayclass-status=true", "--metrics-port=9114", "--metrics-disable", @@ -168,20 +168,20 @@ func TestStaticModeCmdFlagValidation(t *testing.T) { expectedErrPrefix: `invalid argument "!@#$" for "-c, --config" flag: invalid format`, }, { - name: "service-name is set to empty string", + name: "service is set to empty string", args: []string{ - "--service-name=", + "--service=", }, wantErr: true, - expectedErrPrefix: `invalid argument "" for "--service-name" flag: must be set`, + expectedErrPrefix: `invalid argument "" for "--service" flag: must be set`, }, { - name: "service-name is set to invalid string", + name: "service is set to invalid string", args: []string{ - "--service-name=!@#$", + "--service=!@#$", }, wantErr: true, - expectedErrPrefix: `invalid argument "!@#$" for "--service-name" flag: invalid format`, + expectedErrPrefix: `invalid argument "!@#$" for "--service" flag: invalid format`, }, { name: "update-gatewayclass-status is set to empty string", diff --git a/conformance/provisioner/static-deployment.yaml b/conformance/provisioner/static-deployment.yaml index 3f5c78b241..8d2b4c68a6 100644 --- a/conformance/provisioner/static-deployment.yaml +++ b/conformance/provisioner/static-deployment.yaml @@ -27,7 +27,7 @@ spec: - --gateway-ctlr-name=gateway.nginx.org/nginx-gateway-controller - --gatewayclass=nginx - --config=nginx-gateway-config - - --service-name=nginx-gateway + - --service=nginx-gateway - --metrics-disable - --health-port=8081 - --leader-election-lock-name=nginx-gateway-leader-election diff --git a/deploy/helm-chart/templates/deployment.yaml b/deploy/helm-chart/templates/deployment.yaml index 9e778d735f..bc7195b478 100644 --- a/deploy/helm-chart/templates/deployment.yaml +++ b/deploy/helm-chart/templates/deployment.yaml @@ -30,7 +30,7 @@ spec: - --gateway-ctlr-name={{ .Values.nginxGateway.gatewayControllerName }} - --gatewayclass={{ .Values.nginxGateway.gatewayClassName }} - --config={{ include "nginx-gateway.config-name" . }} - - --service-name={{ include "nginx-gateway.fullname" . }} + - --service={{ include "nginx-gateway.fullname" . }} {{- if .Values.metrics.enable }} - --metrics-port={{ .Values.metrics.port }} {{- if .Values.metrics.secure }} diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index ab26cef1ba..37da000ef3 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -140,7 +140,7 @@ spec: - --gateway-ctlr-name=gateway.nginx.org/nginx-gateway-controller - --gatewayclass=nginx - --config=nginx-gateway-config - - --service-name=nginx-gateway + - --service=nginx-gateway - --metrics-port=9113 - --health-port=8081 - --leader-election-lock-name=nginx-gateway-leader-election diff --git a/docs/cli-help.md b/docs/cli-help.md index deedb57ef8..7c20ae892e 100644 --- a/docs/cli-help.md +++ b/docs/cli-help.md @@ -20,7 +20,7 @@ Flags: | `gatewayclass` | `string` | The name of the GatewayClass resource. Every NGINX Gateway Fabric must have a unique corresponding GatewayClass resource. | | `gateway` | `string` | The namespaced name of the Gateway resource to use. Must be of the form: `NAMESPACE/NAME`. If not specified, the control plane will process all Gateways for the configured GatewayClass. However, among them, it will choose the oldest resource by creation timestamp. If the timestamps are equal, it will choose the resource that appears first in alphabetical order by {namespace}/{name}. | | `config` | `string` | The name of the NginxGateway resource to be used for this controller's dynamic configuration. Lives in the same Namespace as the controller. | -| `service-name` | `string` | The name of the Service that fronts this NGINX Gateway Fabric Pod. Lives in the same Namespace as the controller. | +| `service` | `string` | The name of the Service that fronts this NGINX Gateway Fabric Pod. Lives in the same Namespace as the controller. | | `metrics-disable` | `bool` | Disable exposing metrics in the Prometheus format. (default false) | | `metrics-listen-port` | `int` | Sets the port where the Prometheus metrics are exposed. Format: `[1024 - 65535]` (default `9113`) | | `metrics-secure-serving` | `bool` | Configures if the metrics endpoint should be secured using https. Please note that this endpoint will be secured with a self-signed certificate. (default false) | diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md index 79d06b6ee8..f9984d7d87 100644 --- a/docs/gateway-api-compatibility.md +++ b/docs/gateway-api-compatibility.md @@ -68,7 +68,7 @@ Fields: > Support Levels: > > - Core: Supported. -> - Extended: Not supported. +> - Extended: Partially supported. > - Implementation-specific: Not supported. NGINX Gateway Fabric supports only a single Gateway resource. The Gateway resource must reference NGINX Gateway @@ -91,7 +91,7 @@ Fields: - `allowedRoutes` - supported. - `addresses` - not supported. - `status` - - `addresses` - supported. + - `addresses` - partially supported. LoadBalancer and Pod IP. - `conditions` - supported (Condition/Status/Reason): - `Accepted/True/Accepted` - `Accepted/True/ListenersNotValid` diff --git a/docs/installation.md b/docs/installation.md index 6a8d508fe0..c69e5604e6 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -64,6 +64,8 @@ page. ## Expose NGINX Gateway Fabric You can gain access to NGINX Gateway Fabric by creating a `NodePort` Service or a `LoadBalancer` Service. +This Service must live in the same Namespace as the controller. The name of this Service is provided in +the `--service` argument to the controller. > Important > @@ -72,6 +74,9 @@ You can gain access to NGINX Gateway Fabric by creating a `NodePort` Service or > configured for those ports. If you'd like to use different ports in your listeners, > update the manifests accordingly. +NGINX Gateway Fabric will use this Service to set the Addresses field in the Gateway Status resource. A LoadBalancer +Service sets the status field to the IP address and/or Hostname. If no Service exists, the Pod IP address is used. + ### Create a NodePort Service Create a Service with type `NodePort`: diff --git a/internal/framework/status/gateway.go b/internal/framework/status/gateway.go index 6f42772cdb..644be890e5 100644 --- a/internal/framework/status/gateway.go +++ b/internal/framework/status/gateway.go @@ -1,112 +1,12 @@ package status import ( - "context" - "fmt" "sort" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1beta1" - - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ) -// GetGatewayAddresses gets the addresses for the Gateway. -func GetGatewayAddresses( - ctx context.Context, - k8sClient client.Client, - svc *v1.Service, - podConfig config.GatewayPodConfig, -) ([]v1beta1.GatewayStatusAddress, error) { - podAddress := []v1beta1.GatewayStatusAddress{ - { - Type: helpers.GetPointer(v1beta1.IPAddressType), - Value: podConfig.PodIP, - }, - } - - var gwSvc v1.Service - if svc == nil { - key := types.NamespacedName{Name: podConfig.ServiceName, Namespace: podConfig.Namespace} - if err := k8sClient.Get(ctx, key, &gwSvc); err != nil { - return podAddress, fmt.Errorf("error finding Service for Gateway: %w", err) - } - } else { - gwSvc = *svc - } - - var addresses, hostnames []string - switch gwSvc.Spec.Type { - case v1.ServiceTypeNodePort: - var err error - addresses, err = getNodeAddresses(ctx, k8sClient) - if err != nil { - return podAddress, fmt.Errorf("error getting Node addresses: %w", err) - } - case v1.ServiceTypeLoadBalancer: - for _, ingress := range gwSvc.Status.LoadBalancer.Ingress { - if ingress.IP != "" { - addresses = append(addresses, ingress.IP) - } else if ingress.Hostname != "" { - hostnames = append(hostnames, ingress.Hostname) - } - } - } - - gwAddresses := make([]v1beta1.GatewayStatusAddress, 0, len(addresses)+len(hostnames)) - for _, addr := range addresses { - statusAddr := v1beta1.GatewayStatusAddress{ - Type: helpers.GetPointer(v1beta1.IPAddressType), - Value: addr, - } - gwAddresses = append(gwAddresses, statusAddr) - } - - for _, hostname := range hostnames { - statusAddr := v1beta1.GatewayStatusAddress{ - Type: helpers.GetPointer(v1beta1.HostnameAddressType), - Value: hostname, - } - gwAddresses = append(gwAddresses, statusAddr) - } - - return gwAddresses, nil -} - -func getNodeAddresses( - ctx context.Context, - k8sClient client.Client, -) ([]string, error) { - var nodeList v1.NodeList - if err := k8sClient.List(ctx, &nodeList); err != nil { - return nil, err - } - - nodeAddresses := make([]string, 0, len(nodeList.Items)) - for _, node := range nodeList.Items { - var externalIP, internalIP string - for _, address := range node.Status.Addresses { - if address.Type == v1.NodeExternalIP { - externalIP = address.Address - } - if address.Type == v1.NodeInternalIP { - internalIP = address.Address - } - } - if externalIP != "" { - nodeAddresses = append(nodeAddresses, externalIP) - } else if internalIP != "" { - nodeAddresses = append(nodeAddresses, internalIP) - } - } - - return nodeAddresses, nil -} - // prepareGatewayStatus prepares the status for a Gateway resource. func prepareGatewayStatus( gatewayStatus GatewayStatus, diff --git a/internal/framework/status/gateway_test.go b/internal/framework/status/gateway_test.go index 4a631f546e..7ff2d23d0f 100644 --- a/internal/framework/status/gateway_test.go +++ b/internal/framework/status/gateway_test.go @@ -1,102 +1,16 @@ package status import ( - "context" "testing" "time" . "github.com/onsi/gomega" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ) -func TestGetGatewayAddresses(t *testing.T) { - g := NewWithT(t) - - fakeClient := fake.NewFakeClient() - podConfig := config.GatewayPodConfig{ - PodIP: "1.2.3.4", - ServiceName: "my-service", - Namespace: "nginx-gateway", - } - - // no Service exists yet, should get error and Pod Address - addrs, err := GetGatewayAddresses(context.Background(), fakeClient, nil, podConfig) - g.Expect(err).To(HaveOccurred()) - g.Expect(addrs).To(HaveLen(1)) - g.Expect(addrs[0].Value).To(Equal("1.2.3.4")) - - // Create NodePort Service and Nodes - svc := v1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-service", - Namespace: "nginx-gateway", - }, - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeNodePort, - }, - } - node1 := v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node1", - }, - Status: v1.NodeStatus{ - Addresses: []v1.NodeAddress{ - { - Type: v1.NodeExternalIP, - Address: "172.0.0.1", - }, - }, - }, - } - - node2 := v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node2", - }, - Status: v1.NodeStatus{ - Addresses: []v1.NodeAddress{ - { - Type: v1.NodeInternalIP, - Address: "10.10.10.10", - }, - }, - }, - } - - g.Expect(fakeClient.Create(context.Background(), &svc)).To(Succeed()) - g.Expect(fakeClient.Create(context.Background(), &node1)).To(Succeed()) - g.Expect(fakeClient.Create(context.Background(), &node2)).To(Succeed()) - - addrs, err = GetGatewayAddresses(context.Background(), fakeClient, nil, podConfig) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(addrs).To(HaveLen(2)) - g.Expect(addrs[0].Value).To(Equal("172.0.0.1")) - g.Expect(addrs[1].Value).To(Equal("10.10.10.10")) - - // Change to LoadBalancer Service - svc.Spec.Type = v1.ServiceTypeLoadBalancer - svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{ - { - IP: "34.35.36.37", - }, - { - Hostname: "myhost", - }, - } - - addrs, err = GetGatewayAddresses(context.Background(), fakeClient, &svc, podConfig) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(addrs).To(HaveLen(2)) - g.Expect(addrs[0].Value).To(Equal("34.35.36.37")) - g.Expect(addrs[1].Value).To(Equal("myhost")) -} - func TestPrepareGatewayStatus(t *testing.T) { podIP := v1beta1.GatewayStatusAddress{ Type: helpers.GetPointer(v1beta1.IPAddressType), diff --git a/internal/framework/status/statusfakes/fake_updater.go b/internal/framework/status/statusfakes/fake_updater.go index c826942085..6ca5b60c32 100644 --- a/internal/framework/status/statusfakes/fake_updater.go +++ b/internal/framework/status/statusfakes/fake_updater.go @@ -6,7 +6,7 @@ import ( "sync" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" - v1 "k8s.io/api/core/v1" + "sigs.k8s.io/gateway-api/apis/v1beta1" ) type FakeUpdater struct { @@ -25,11 +25,11 @@ type FakeUpdater struct { arg1 context.Context arg2 status.Status } - UpdateAddressesStub func(context.Context, *v1.Service) + UpdateAddressesStub func(context.Context, []v1beta1.GatewayStatusAddress) updateAddressesMutex sync.RWMutex updateAddressesArgsForCall []struct { arg1 context.Context - arg2 *v1.Service + arg2 []v1beta1.GatewayStatusAddress } invocations map[string][][]interface{} invocationsMutex sync.RWMutex @@ -124,14 +124,19 @@ func (fake *FakeUpdater) UpdateArgsForCall(i int) (context.Context, status.Statu return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeUpdater) UpdateAddresses(arg1 context.Context, arg2 *v1.Service) { +func (fake *FakeUpdater) UpdateAddresses(arg1 context.Context, arg2 []v1beta1.GatewayStatusAddress) { + var arg2Copy []v1beta1.GatewayStatusAddress + if arg2 != nil { + arg2Copy = make([]v1beta1.GatewayStatusAddress, len(arg2)) + copy(arg2Copy, arg2) + } fake.updateAddressesMutex.Lock() fake.updateAddressesArgsForCall = append(fake.updateAddressesArgsForCall, struct { arg1 context.Context - arg2 *v1.Service - }{arg1, arg2}) + arg2 []v1beta1.GatewayStatusAddress + }{arg1, arg2Copy}) stub := fake.UpdateAddressesStub - fake.recordInvocation("UpdateAddresses", []interface{}{arg1, arg2}) + fake.recordInvocation("UpdateAddresses", []interface{}{arg1, arg2Copy}) fake.updateAddressesMutex.Unlock() if stub != nil { fake.UpdateAddressesStub(arg1, arg2) @@ -144,13 +149,13 @@ func (fake *FakeUpdater) UpdateAddressesCallCount() int { return len(fake.updateAddressesArgsForCall) } -func (fake *FakeUpdater) UpdateAddressesCalls(stub func(context.Context, *v1.Service)) { +func (fake *FakeUpdater) UpdateAddressesCalls(stub func(context.Context, []v1beta1.GatewayStatusAddress)) { fake.updateAddressesMutex.Lock() defer fake.updateAddressesMutex.Unlock() fake.UpdateAddressesStub = stub } -func (fake *FakeUpdater) UpdateAddressesArgsForCall(i int) (context.Context, *v1.Service) { +func (fake *FakeUpdater) UpdateAddressesArgsForCall(i int) (context.Context, []v1beta1.GatewayStatusAddress) { fake.updateAddressesMutex.RLock() defer fake.updateAddressesMutex.RUnlock() argsForCall := fake.updateAddressesArgsForCall[i] diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index fb0eadef82..ff8fbbaeb9 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -8,7 +8,6 @@ import ( "time" "github.com/go-logr/logr" - v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" @@ -30,7 +29,7 @@ type Updater interface { // Update updates the statuses of the resources. Update(context.Context, Status) // UpdateAddresses updates the Gateway Addresses when the Gateway Service changes. - UpdateAddresses(context.Context, *v1.Service) + UpdateAddresses(context.Context, []v1beta1.GatewayStatusAddress) // Enable enables status updates. The updater will update the statuses in Kubernetes API to ensure they match the // statuses of the last Update invocation. Enable(ctx context.Context) @@ -255,12 +254,8 @@ func (upd *UpdaterImpl) writeStatuses( } // UpdateAddresses is called when the Gateway Status needs its addresses updated. -func (upd *UpdaterImpl) UpdateAddresses(ctx context.Context, svc *v1.Service) { - addresses, err := GetGatewayAddresses(ctx, upd.cfg.Client, svc, upd.cfg.GatewayPodConfig) - if err != nil { - upd.cfg.Logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") - } - +func (upd *UpdaterImpl) UpdateAddresses(ctx context.Context, addresses []v1beta1.GatewayStatusAddress) { + upd.lock.Lock() for name, status := range upd.lastStatuses.gatewayAPI.GatewayStatuses { if status.Ignored { continue @@ -268,6 +263,7 @@ func (upd *UpdaterImpl) UpdateAddresses(ctx context.Context, svc *v1.Service) { status.Addresses = addresses upd.lastStatuses.gatewayAPI.GatewayStatuses[name] = status } + upd.lock.Unlock() upd.Update(ctx, upd.lastStatuses.gatewayAPI) } diff --git a/internal/framework/status/updater_test.go b/internal/framework/status/updater_test.go index 49e2189984..66512a9bdc 100644 --- a/internal/framework/status/updater_test.go +++ b/internal/framework/status/updater_test.go @@ -7,7 +7,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -403,25 +402,14 @@ var _ = Describe("Updater", func() { }) When("the Gateway Service is updated with a new address", func() { - svc := &v1.Service{ - Spec: v1.ServiceSpec{ - Type: v1.ServiceTypeLoadBalancer, - }, - Status: v1.ServiceStatus{ - LoadBalancer: v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{ - { - IP: "5.6.7.8", - }, - }, - }, - }, - } - AfterEach(func() { // reset the IP for the remaining tests - svc.Status.LoadBalancer.Ingress[0].IP = "1.2.3.4" - updater.UpdateAddresses(context.Background(), svc) + updater.UpdateAddresses(context.Background(), []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: "1.2.3.4", + }, + }) }) It("should update the previous Gateway statuses with new address", func() { @@ -429,7 +417,12 @@ var _ = Describe("Updater", func() { expectedGw := createExpectedGwWithGeneration(1) expectedGw.Status.Addresses[0].Value = "5.6.7.8" - updater.UpdateAddresses(context.Background(), svc) + updater.UpdateAddresses(context.Background(), []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: "5.6.7.8", + }, + }) err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) Expect(err).ToNot(HaveOccurred()) diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index 45134a33ad..5bfe6a6244 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -22,8 +22,6 @@ type Config struct { ConfigName string // GatewayClassName is the name of the GatewayClass resource that the Gateway will use. GatewayClassName string - // Namespace is the Namespace of this Pod. - Namespace string // LeaderElection contains the configuration for leader election. LeaderElection LeaderElection // UpdateGatewayClassStatus enables updating the status of the GatewayClass resource. diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 9248dbe516..903c100185 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -7,16 +7,20 @@ import ( "github.com/go-logr/logr" apiv1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1beta1" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" ngfConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config" + ngxConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state" @@ -40,7 +44,7 @@ type eventHandlerConfig struct { // serviceResolver resolves Services to Endpoints. serviceResolver resolver.ServiceResolver // generator is the nginx config generator. - generator config.Generator + generator ngxConfig.Generator // nginxFileMgr is the file Manager for nginx. nginxFileMgr file.Manager // nginxRuntimeMgr manages nginx runtime. @@ -121,7 +125,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log } } - gwAddresses, err := status.GetGatewayAddresses(ctx, h.cfg.k8sClient, nil, h.cfg.gatewayPodConfig) + gwAddresses, err := getGatewayAddresses(ctx, h.cfg.k8sClient, nil, h.cfg.gatewayPodConfig) if err != nil { logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") } @@ -138,7 +142,11 @@ func (h *eventHandlerImpl) handleEvent(ctx context.Context, logger logr.Logger, case *apiv1.Service: podConfig := h.cfg.gatewayPodConfig if obj.Name == podConfig.ServiceName && obj.Namespace == podConfig.Namespace { - h.cfg.statusUpdater.UpdateAddresses(ctx, obj) + gwAddresses, err := getGatewayAddresses(ctx, h.cfg.k8sClient, obj, h.cfg.gatewayPodConfig) + if err != nil { + logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") + } + h.cfg.statusUpdater.UpdateAddresses(ctx, gwAddresses) } else { h.cfg.processor.CaptureUpsertChange(e.Resource) } @@ -152,7 +160,11 @@ func (h *eventHandlerImpl) handleEvent(ctx context.Context, logger logr.Logger, case *apiv1.Service: podConfig := h.cfg.gatewayPodConfig if e.NamespacedName.Name == podConfig.ServiceName && e.NamespacedName.Namespace == podConfig.Namespace { - h.cfg.statusUpdater.UpdateAddresses(ctx, nil) + gwAddresses, err := getGatewayAddresses(ctx, h.cfg.k8sClient, nil, h.cfg.gatewayPodConfig) + if err != nil { + logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") + } + h.cfg.statusUpdater.UpdateAddresses(ctx, gwAddresses) } else { h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) } @@ -218,3 +230,59 @@ func (h *eventHandlerImpl) updateControlPlaneAndSetStatus( logger.Info("Reconfigured control plane.") } } + +// getGatewayAddresses gets the addresses for the Gateway. +func getGatewayAddresses( + ctx context.Context, + k8sClient client.Client, + svc *v1.Service, + podConfig config.GatewayPodConfig, +) ([]v1beta1.GatewayStatusAddress, error) { + podAddress := []v1beta1.GatewayStatusAddress{ + { + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: podConfig.PodIP, + }, + } + + var gwSvc v1.Service + if svc == nil { + key := types.NamespacedName{Name: podConfig.ServiceName, Namespace: podConfig.Namespace} + if err := k8sClient.Get(ctx, key, &gwSvc); err != nil { + return podAddress, fmt.Errorf("error finding Service for Gateway: %w", err) + } + } else { + gwSvc = *svc + } + + var addresses, hostnames []string + switch gwSvc.Spec.Type { + case v1.ServiceTypeLoadBalancer: + for _, ingress := range gwSvc.Status.LoadBalancer.Ingress { + if ingress.IP != "" { + addresses = append(addresses, ingress.IP) + } else if ingress.Hostname != "" { + hostnames = append(hostnames, ingress.Hostname) + } + } + } + + gwAddresses := make([]v1beta1.GatewayStatusAddress, 0, len(addresses)+len(hostnames)) + for _, addr := range addresses { + statusAddr := v1beta1.GatewayStatusAddress{ + Type: helpers.GetPointer(v1beta1.IPAddressType), + Value: addr, + } + gwAddresses = append(gwAddresses, statusAddr) + } + + for _, hostname := range hostnames { + statusAddr := v1beta1.GatewayStatusAddress{ + Type: helpers.GetPointer(v1beta1.HostnameAddressType), + Value: hostname, + } + gwAddresses = append(gwAddresses, statusAddr) + } + + return gwAddresses, nil +} diff --git a/internal/mode/static/handler_test.go b/internal/mode/static/handler_test.go index 6045876707..d5f3b5377c 100644 --- a/internal/mode/static/handler_test.go +++ b/internal/mode/static/handler_test.go @@ -336,3 +336,51 @@ var _ = Describe("eventHandler", func() { Expect(handle).Should(Panic()) }) }) + +var _ = Describe("getGatewayAddresses", func() { + It("gets gateway addresses from a Service", func() { + fakeClient := fake.NewFakeClient() + podConfig := config.GatewayPodConfig{ + PodIP: "1.2.3.4", + ServiceName: "my-service", + Namespace: "nginx-gateway", + } + + // no Service exists yet, should get error and Pod Address + addrs, err := getGatewayAddresses(context.Background(), fakeClient, nil, podConfig) + Expect(err).To(HaveOccurred()) + Expect(addrs).To(HaveLen(1)) + Expect(addrs[0].Value).To(Equal("1.2.3.4")) + + // Create LoadBalancer Service + svc := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-service", + Namespace: "nginx-gateway", + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "34.35.36.37", + }, + { + Hostname: "myhost", + }, + }, + }, + }, + } + + Expect(fakeClient.Create(context.Background(), &svc)).To(Succeed()) + + addrs, err = getGatewayAddresses(context.Background(), fakeClient, &svc, podConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(addrs).To(HaveLen(2)) + Expect(addrs[0].Value).To(Equal("34.35.36.37")) + Expect(addrs[1].Value).To(Equal("myhost")) + }) +}) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 82d46f8539..4c66da33fe 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -90,7 +90,7 @@ func StartManager(cfg config.Config) error { ctx := ctlr.SetupSignalHandler() controlConfigNSName := types.NamespacedName{ - Namespace: cfg.Namespace, + Namespace: cfg.GatewayPodConfig.Namespace, Name: cfg.ConfigName, } if err := registerControllers(ctx, cfg, mgr, recorder, logLevelSetter, eventCh, controlConfigNSName); err != nil { @@ -210,7 +210,7 @@ func StartManager(cfg config.Config) error { leaderElectorLogger.Info("Stopped leading") statusUpdater.Disable() }, - lockNs: cfg.Namespace, + lockNs: cfg.GatewayPodConfig.Namespace, lockName: cfg.LeaderElection.LockName, identity: cfg.LeaderElection.Identity, }) @@ -273,7 +273,7 @@ func registerControllers( { objectType: &apiv1.Service{}, options: func() []controller.Option { - svcNSName := types.NamespacedName{Namespace: cfg.Namespace, Name: cfg.GatewayPodConfig.ServiceName} + svcNSName := types.NamespacedName{Namespace: cfg.GatewayPodConfig.Namespace, Name: cfg.GatewayPodConfig.ServiceName} return []controller.Option{ controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(svcNSName)), controller.WithK8sPredicate(predicate.GatewayServicePredicate{}), From a9f61922b4cc16d6e910fded1b80b373f14f31cb Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Tue, 17 Oct 2023 13:59:03 -0600 Subject: [PATCH 3/4] Fix predicate and locking --- deploy/helm-chart/templates/rbac.yaml | 1 - deploy/manifests/nginx-gateway.yaml | 1 - .../framework/controller/predicate/service.go | 34 ++++++++++++ .../controller/predicate/service_test.go | 53 +++++++++++++++++-- internal/framework/status/updater.go | 5 +- internal/mode/static/handler.go | 6 +-- internal/mode/static/manager.go | 3 +- 7 files changed, 88 insertions(+), 15 deletions(-) diff --git a/deploy/helm-chart/templates/rbac.yaml b/deploy/helm-chart/templates/rbac.yaml index d1b8dceded..405c34525e 100644 --- a/deploy/helm-chart/templates/rbac.yaml +++ b/deploy/helm-chart/templates/rbac.yaml @@ -21,7 +21,6 @@ rules: - namespaces - services - secrets - - nodes verbs: - list - watch diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 37da000ef3..28c26b86b2 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -32,7 +32,6 @@ rules: - namespaces - services - secrets - - nodes verbs: - list - watch diff --git a/internal/framework/controller/predicate/service.go b/internal/framework/controller/predicate/service.go index b746cd3959..dcda296d46 100644 --- a/internal/framework/controller/predicate/service.go +++ b/internal/framework/controller/predicate/service.go @@ -2,7 +2,9 @@ package predicate import ( apiv1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -67,6 +69,16 @@ func (ServicePortsChangedPredicate) Update(e event.UpdateEvent) bool { // GatewayServicePredicate implements predicate functions for this Pod's Service. type GatewayServicePredicate struct { predicate.Funcs + NSName types.NamespacedName +} + +// Create implements the default CreateEvent filter for the Gateway Service. +func (gsp GatewayServicePredicate) Create(e event.CreateEvent) bool { + if e.Object == nil { + return false + } + + return client.ObjectKeyFromObject(e.Object) == gsp.NSName } // Update implements the default UpdateEvent filter for the Gateway Service. @@ -88,6 +100,10 @@ func (gsp GatewayServicePredicate) Update(e event.UpdateEvent) bool { return false } + if client.ObjectKeyFromObject(newSvc) != gsp.NSName { + return false + } + if oldSvc.Spec.Type != newSvc.Spec.Type { return true } @@ -109,3 +125,21 @@ func (gsp GatewayServicePredicate) Update(e event.UpdateEvent) bool { return false } + +// Delete implements the default DeleteEvent filter for the Gateway Service. +func (gsp GatewayServicePredicate) Delete(e event.DeleteEvent) bool { + if e.Object == nil { + return false + } + + return client.ObjectKeyFromObject(e.Object) == gsp.NSName +} + +// Generic implements the default GenericEvent filter for the Gateway Service. +func (gsp GatewayServicePredicate) Generic(e event.GenericEvent) bool { + if e.Object == nil { + return false + } + + return client.ObjectKeyFromObject(e.Object) == gsp.NSName +} diff --git a/internal/framework/controller/predicate/service_test.go b/internal/framework/controller/predicate/service_test.go index faa40e3ef3..d777760f5e 100644 --- a/internal/framework/controller/predicate/service_test.go +++ b/internal/framework/controller/predicate/service_test.go @@ -5,6 +5,8 @@ import ( . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -240,11 +242,21 @@ func TestServicePortsChangedPredicate_Update(t *testing.T) { func TestServicePortsChangedPredicate(t *testing.T) { g := NewWithT(t) - p := ServicePortsChangedPredicate{} + p := GatewayServicePredicate{NSName: types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx"}} - g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeTrue()) - g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue()) - g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeTrue()) + g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeFalse()) + g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeFalse()) + g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeFalse()) + + correctSvc := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "nginx", + }, + } + g.Expect(p.Delete(event.DeleteEvent{Object: correctSvc})).To(BeTrue()) + g.Expect(p.Create(event.CreateEvent{Object: correctSvc})).To(BeTrue()) + g.Expect(p.Generic(event.GenericEvent{Object: correctSvc})).To(BeTrue()) } func TestGatewayServicePredicate_Update(t *testing.T) { @@ -278,6 +290,17 @@ func TestGatewayServicePredicate_Update(t *testing.T) { objectNew: &v1.Namespace{}, expUpdate: false, }, + { + msg: "Service not watched", + objectOld: &v1.Service{}, + objectNew: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "not-watched", + }, + }, + expUpdate: false, + }, { msg: "something irrelevant changed", objectOld: &v1.Service{ @@ -286,6 +309,10 @@ func TestGatewayServicePredicate_Update(t *testing.T) { }, }, objectNew: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "nginx", + }, Spec: v1.ServiceSpec{ ClusterIP: "5.6.7.8", }, @@ -300,6 +327,10 @@ func TestGatewayServicePredicate_Update(t *testing.T) { }, }, objectNew: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "nginx", + }, Spec: v1.ServiceSpec{ Type: v1.ServiceTypeNodePort, }, @@ -323,6 +354,10 @@ func TestGatewayServicePredicate_Update(t *testing.T) { }, }, objectNew: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "nginx", + }, Spec: v1.ServiceSpec{ Type: v1.ServiceTypeNodePort, }, Status: v1.ServiceStatus{ @@ -357,6 +392,10 @@ func TestGatewayServicePredicate_Update(t *testing.T) { }, }, objectNew: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "nginx", + }, Spec: v1.ServiceSpec{ Type: v1.ServiceTypeNodePort, }, Status: v1.ServiceStatus{ @@ -388,6 +427,10 @@ func TestGatewayServicePredicate_Update(t *testing.T) { }, }, objectNew: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "nginx-gateway", + Name: "nginx", + }, Spec: v1.ServiceSpec{ Type: v1.ServiceTypeNodePort, }, Status: v1.ServiceStatus{ @@ -404,7 +447,7 @@ func TestGatewayServicePredicate_Update(t *testing.T) { }, } - p := GatewayServicePredicate{} + p := GatewayServicePredicate{NSName: types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx"}} for _, tc := range testcases { t.Run(tc.msg, func(t *testing.T) { diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index ff8fbbaeb9..6dd114b4ea 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -255,7 +255,9 @@ func (upd *UpdaterImpl) writeStatuses( // UpdateAddresses is called when the Gateway Status needs its addresses updated. func (upd *UpdaterImpl) UpdateAddresses(ctx context.Context, addresses []v1beta1.GatewayStatusAddress) { + defer upd.lock.Unlock() upd.lock.Lock() + for name, status := range upd.lastStatuses.gatewayAPI.GatewayStatuses { if status.Ignored { continue @@ -263,9 +265,8 @@ func (upd *UpdaterImpl) UpdateAddresses(ctx context.Context, addresses []v1beta1 status.Addresses = addresses upd.lastStatuses.gatewayAPI.GatewayStatuses[name] = status } - upd.lock.Unlock() - upd.Update(ctx, upd.lastStatuses.gatewayAPI) + upd.updateGatewayAPI(ctx, upd.lastStatuses.gatewayAPI) } // NewRetryUpdateFunc returns a function which will be used in wait.ExponentialBackoffWithContext. diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 903c100185..5ac9812525 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -147,9 +147,8 @@ func (h *eventHandlerImpl) handleEvent(ctx context.Context, logger logr.Logger, logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") } h.cfg.statusUpdater.UpdateAddresses(ctx, gwAddresses) - } else { - h.cfg.processor.CaptureUpsertChange(e.Resource) } + h.cfg.processor.CaptureUpsertChange(e.Resource) default: h.cfg.processor.CaptureUpsertChange(e.Resource) } @@ -165,9 +164,8 @@ func (h *eventHandlerImpl) handleEvent(ctx context.Context, logger logr.Logger, logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address") } h.cfg.statusUpdater.UpdateAddresses(ctx, gwAddresses) - } else { - h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) } + h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) default: h.cfg.processor.CaptureDeleteChange(e.Type, e.NamespacedName) } diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 4c66da33fe..7a04f33e8d 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -275,8 +275,7 @@ func registerControllers( options: func() []controller.Option { svcNSName := types.NamespacedName{Namespace: cfg.GatewayPodConfig.Namespace, Name: cfg.GatewayPodConfig.ServiceName} return []controller.Option{ - controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(svcNSName)), - controller.WithK8sPredicate(predicate.GatewayServicePredicate{}), + controller.WithK8sPredicate(predicate.GatewayServicePredicate{NSName: svcNSName}), } }(), }, From 3ebbdd559899b95f1cc760ed58bb36e2967a4162 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Tue, 17 Oct 2023 14:28:43 -0600 Subject: [PATCH 4/4] Remove unnecessary predicate funcs --- .../framework/controller/predicate/service.go | 27 ------------------- .../controller/predicate/service_test.go | 18 +++---------- 2 files changed, 4 insertions(+), 41 deletions(-) diff --git a/internal/framework/controller/predicate/service.go b/internal/framework/controller/predicate/service.go index dcda296d46..d1b361fb13 100644 --- a/internal/framework/controller/predicate/service.go +++ b/internal/framework/controller/predicate/service.go @@ -72,15 +72,6 @@ type GatewayServicePredicate struct { NSName types.NamespacedName } -// Create implements the default CreateEvent filter for the Gateway Service. -func (gsp GatewayServicePredicate) Create(e event.CreateEvent) bool { - if e.Object == nil { - return false - } - - return client.ObjectKeyFromObject(e.Object) == gsp.NSName -} - // Update implements the default UpdateEvent filter for the Gateway Service. func (gsp GatewayServicePredicate) Update(e event.UpdateEvent) bool { if e.ObjectOld == nil { @@ -125,21 +116,3 @@ func (gsp GatewayServicePredicate) Update(e event.UpdateEvent) bool { return false } - -// Delete implements the default DeleteEvent filter for the Gateway Service. -func (gsp GatewayServicePredicate) Delete(e event.DeleteEvent) bool { - if e.Object == nil { - return false - } - - return client.ObjectKeyFromObject(e.Object) == gsp.NSName -} - -// Generic implements the default GenericEvent filter for the Gateway Service. -func (gsp GatewayServicePredicate) Generic(e event.GenericEvent) bool { - if e.Object == nil { - return false - } - - return client.ObjectKeyFromObject(e.Object) == gsp.NSName -} diff --git a/internal/framework/controller/predicate/service_test.go b/internal/framework/controller/predicate/service_test.go index d777760f5e..90bda2fd2d 100644 --- a/internal/framework/controller/predicate/service_test.go +++ b/internal/framework/controller/predicate/service_test.go @@ -242,21 +242,11 @@ func TestServicePortsChangedPredicate_Update(t *testing.T) { func TestServicePortsChangedPredicate(t *testing.T) { g := NewWithT(t) - p := GatewayServicePredicate{NSName: types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx"}} - - g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeFalse()) - g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeFalse()) - g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeFalse()) + p := GatewayServicePredicate{} - correctSvc := &v1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "nginx-gateway", - Name: "nginx", - }, - } - g.Expect(p.Delete(event.DeleteEvent{Object: correctSvc})).To(BeTrue()) - g.Expect(p.Create(event.CreateEvent{Object: correctSvc})).To(BeTrue()) - g.Expect(p.Generic(event.GenericEvent{Object: correctSvc})).To(BeTrue()) + g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeTrue()) + g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue()) + g.Expect(p.Generic(event.GenericEvent{Object: &v1.Service{}})).To(BeTrue()) } func TestGatewayServicePredicate_Update(t *testing.T) {