From 9149f71629746386857a4d5dc791ebd49a7c5ff5 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Tue, 29 Apr 2025 11:08:37 -0600 Subject: [PATCH 1/3] CP/DP Split: Support configuring NodePorts Problem: Now that the control plane provisions the NGINX Service, users can't set specific NodePorts values. Solution: Allow users to specify NodePorts in the helm chart (globally) and in the NginxProxy resource. --- apis/v1alpha2/nginxproxy_types.go | 22 ++ apis/v1alpha2/zz_generated.deepcopy.go | 20 ++ charts/nginx-gateway-fabric/README.md | 15 +- .../templates/_helpers.tpl | 13 ++ .../templates/nginxproxy.yaml | 4 +- .../nginx-gateway-fabric/values.schema.json | 61 ++++- charts/nginx-gateway-fabric/values.yaml | 38 ++- .../bases/gateway.nginx.org_nginxproxies.yaml | 30 +++ deploy/crds.yaml | 30 +++ internal/mode/static/provisioner/handler.go | 57 ++++- .../mode/static/provisioner/handler_test.go | 33 ++- internal/mode/static/provisioner/objects.go | 9 + .../mode/static/provisioner/objects_test.go | 21 +- .../mode/static/provisioner/provisioner.go | 15 +- internal/mode/static/provisioner/store.go | 74 +++++- .../mode/static/provisioner/store_test.go | 218 ++++++++++++++++++ 16 files changed, 611 insertions(+), 49 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index 7c716824fa..13013d81cf 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -533,6 +533,13 @@ type ServiceSpec struct { // // +optional LoadBalancerSourceRanges []string `json:"loadBalancerSourceRanges,omitempty"` + + // NodePorts are the list of NodePorts to expose on the NGINX data plane service. + // Each NodePort MUST map to a Gateway listener port, otherwise it will be ignored. + // The default NodePort range enforced by Kubernetes is 30000-32767. + // + // +optional + NodePorts []NodePort `json:"nodePorts,omitempty"` } // ServiceType describes ingress method for the Service. @@ -569,3 +576,18 @@ const ( // (dropping the traffic if there are no local endpoints). ExternalTrafficPolicyLocal ExternalTrafficPolicy = ExternalTrafficPolicy(corev1.ServiceExternalTrafficPolicyLocal) ) + +// NodePort creates a port on each node on which the NGINX data plane service is exposed. The NodePort MUST +// map to a Gateway listener port, otherwise it will be ignored. If not specified, Kubernetes allocates a NodePort +// automatically if required. The default NodePort range enforced by Kubernetes is 30000-32767. +type NodePort struct { + // Port is the NodePort to expose. + // kubebuilder:validation:Minimum=1 + // kubebuilder:validation:Maximum=65535 + Port int32 `json:"port"` + + // ListenerPort is the Gateway listener port that this NodePort maps to. + // kubebuilder:validation:Minimum=1 + // kubebuilder:validation:Maximum=65535 + ListenerPort int32 `json:"listenerPort"` +} diff --git a/apis/v1alpha2/zz_generated.deepcopy.go b/apis/v1alpha2/zz_generated.deepcopy.go index 60bf2cd9cd..bd6d81bca2 100644 --- a/apis/v1alpha2/zz_generated.deepcopy.go +++ b/apis/v1alpha2/zz_generated.deepcopy.go @@ -328,6 +328,21 @@ func (in *NginxProxySpec) DeepCopy() *NginxProxySpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *NodePort) DeepCopyInto(out *NodePort) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodePort. +func (in *NodePort) DeepCopy() *NodePort { + if in == nil { + return nil + } + out := new(NodePort) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ObservabilityPolicy) DeepCopyInto(out *ObservabilityPolicy) { *out = *in @@ -547,6 +562,11 @@ func (in *ServiceSpec) DeepCopyInto(out *ServiceSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.NodePorts != nil { + in, out := &in.NodePorts, &out.NodePorts + *out = make([]NodePort, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceSpec. diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index f2141de4d2..27637e1220 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -258,19 +258,24 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `certGenerator.overwrite` | Overwrite existing TLS Secrets on startup. | bool | `false` | | `certGenerator.serverTLSSecretName` | The name of the Secret containing TLS CA, certificate, and key for the NGINX Gateway Fabric control plane to securely communicate with the NGINX Agent. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `"server-tls"` | | `clusterDomain` | The DNS cluster domain of your Kubernetes cluster. | string | `"cluster.local"` | -| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","plus":false,"pod":{},"replicas":1,"service":{"externalTrafficPolicy":"Local","type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | -| `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. | object | `{}` | -| `nginx.container` | The container configuration for the NGINX container. | object | `{}` | +| `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","plus":false,"pod":{},"replicas":1,"service":{"annotations":{},"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | +| `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways. | object | `{}` | +| `nginx.container` | The container configuration for the NGINX container. This is applied globally to all Gateways. | object | `{}` | | `nginx.debug` | Enable debugging for NGINX. Uses the nginx-debug binary. The NGINX error log level should be set to debug in the NginxProxy resource. | bool | `false` | | `nginx.image.repository` | The NGINX image to use. | string | `"ghcr.io/nginx/nginx-gateway-fabric/nginx"` | | `nginx.imagePullSecret` | The name of the secret containing docker registry credentials. Secret must exist in the same namespace as the helm release. The control plane will copy this secret into any namespace where NGINX is deployed. | string | `""` | | `nginx.imagePullSecrets` | A list of secret names containing docker registry credentials. Secrets must exist in the same namespace as the helm release. The control plane will copy these secrets into any namespace where NGINX is deployed. | list | `[]` | | `nginx.kind` | The kind of NGINX deployment. | string | `"deployment"` | | `nginx.plus` | Is NGINX Plus image being used. | bool | `false` | -| `nginx.pod` | The pod configuration for the NGINX data plane pod. | object | `{}` | +| `nginx.pod` | The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways. | object | `{}` | | `nginx.replicas` | The number of replicas of the NGINX Deployment. | int | `1` | -| `nginx.service` | The service configuration for the NGINX data plane. | object | `{"externalTrafficPolicy":"Local","type":"LoadBalancer"}` | +| `nginx.service` | The service configuration for the NGINX data plane. This is applied globally to all Gateways. | object | `{"annotations":{},"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"}` | +| `nginx.service.annotations` | The annotations of the NGINX data plane service. | object | `{}` | | `nginx.service.externalTrafficPolicy` | The externalTrafficPolicy of the service. The value Local preserves the client source IP. | string | `"Local"` | +| `nginx.service.loadBalancerClass` | LoadBalancerClass is the class of the load balancer implementation this Service belongs to. Requires nginx.service.type set to LoadBalancer. | string | `""` | +| `nginx.service.loadBalancerIP` | The static IP address for the load balancer. Requires nginx.service.type set to LoadBalancer. | string | `""` | +| `nginx.service.loadBalancerSourceRanges` | The IP ranges (CIDR) that are allowed to access the load balancer. Requires nginx.service.type set to LoadBalancer. | list | `[]` | +| `nginx.service.nodePorts` | A list of NodePorts to expose on the NGINX data plane service. Each NodePort MUST map to a Gateway listener port, otherwise it will be ignored. The default NodePort range enforced by Kubernetes is 30000-32767. | list | `[]` | | `nginx.service.type` | The type of service to create for the NGINX data plane. | string | `"LoadBalancer"` | | `nginx.usage.caSecretName` | The name of the Secret containing the NGINX Instance Manager CA certificate. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` | | `nginx.usage.clientSSLSecretName` | The name of the Secret containing the client certificate and key for authenticating with NGINX Instance Manager. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `""` | diff --git a/charts/nginx-gateway-fabric/templates/_helpers.tpl b/charts/nginx-gateway-fabric/templates/_helpers.tpl index 2a137d5fbd..01155eb707 100644 --- a/charts/nginx-gateway-fabric/templates/_helpers.tpl +++ b/charts/nginx-gateway-fabric/templates/_helpers.tpl @@ -91,3 +91,16 @@ Expand leader election lock name. {{- printf "%s-%s" (include "nginx-gateway.fullname" .) "leader-election" -}} {{- end -}} {{- end -}} + +{{/* +Filters out empty fields from a struct. +*/}} +{{- define "filterEmptyFields" -}} +{{- $result := dict }} +{{- range $key, $value := . }} + {{- if and (not (empty $value)) (not (and (kindIs "slice" $value) (eq (len $value) 0))) }} + {{- $result = merge $result (dict $key $value) }} + {{- end }} +{{- end }} +{{- $result | toYaml }} +{{- end }} diff --git a/charts/nginx-gateway-fabric/templates/nginxproxy.yaml b/charts/nginx-gateway-fabric/templates/nginxproxy.yaml index f77630fe95..56e4de6943 100644 --- a/charts/nginx-gateway-fabric/templates/nginxproxy.yaml +++ b/charts/nginx-gateway-fabric/templates/nginxproxy.yaml @@ -29,5 +29,7 @@ spec: {{- end }} {{- if .Values.nginx.service }} service: - {{- toYaml .Values.nginx.service | nindent 6 }} + {{- with .Values.nginx.service }} + {{- include "filterEmptyFields" . | nindent 6 }} + {{- end }} {{- end }} diff --git a/charts/nginx-gateway-fabric/values.schema.json b/charts/nginx-gateway-fabric/values.schema.json index ae9d507656..5b3f83c814 100644 --- a/charts/nginx-gateway-fabric/values.schema.json +++ b/charts/nginx-gateway-fabric/values.schema.json @@ -53,7 +53,7 @@ "description": "The nginx section contains the configuration for all NGINX data plane deployments\ninstalled by the NGINX Gateway Fabric control plane.", "properties": { "config": { - "description": "The configuration for the data plane that is contained in the NginxProxy resource.", + "description": "The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways.", "properties": { "disableHTTP2": { "description": "DisableHTTP2 defines if http2 should be disabled for all servers.", @@ -266,7 +266,7 @@ "type": "object" }, "container": { - "description": "The container configuration for the NGINX container.", + "description": "The container configuration for the NGINX container. This is applied globally to all Gateways.", "required": [], "title": "container", "type": "object" @@ -341,7 +341,7 @@ "type": "boolean" }, "pod": { - "description": "The pod configuration for the NGINX data plane pod.", + "description": "The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways.", "required": [], "title": "pod", "type": "object" @@ -354,8 +354,14 @@ "type": "integer" }, "service": { - "description": "The service configuration for the NGINX data plane.", + "description": "The service configuration for the NGINX data plane. This is applied globally to all Gateways.", "properties": { + "annotations": { + "description": "The annotations of the NGINX data plane service.", + "required": [], + "title": "annotations", + "type": "object" + }, "externalTrafficPolicy": { "default": "Local", "description": "The externalTrafficPolicy of the service. The value Local preserves the client source IP.", @@ -366,6 +372,53 @@ "required": [], "title": "externalTrafficPolicy" }, + "loadBalancerClass": { + "default": "", + "description": "LoadBalancerClass is the class of the load balancer implementation this Service belongs to.\nRequires nginx.service.type set to LoadBalancer.", + "required": [], + "title": "loadBalancerClass", + "type": "string" + }, + "loadBalancerIP": { + "default": "", + "description": "The static IP address for the load balancer. Requires nginx.service.type set to LoadBalancer.", + "required": [], + "title": "loadBalancerIP", + "type": "string" + }, + "loadBalancerSourceRanges": { + "description": "The IP ranges (CIDR) that are allowed to access the load balancer. Requires nginx.service.type set to LoadBalancer.", + "items": { + "required": [] + }, + "required": [], + "title": "loadBalancerSourceRanges", + "type": "array" + }, + "nodePorts": { + "description": "A list of NodePorts to expose on the NGINX data plane service. Each NodePort MUST map to a Gateway listener port,\notherwise it will be ignored. The default NodePort range enforced by Kubernetes is 30000-32767.", + "items": { + "properties": { + "listenerPort": { + "maximum": 65535, + "minimum": 1, + "required": [], + "type": "integer" + }, + "port": { + "maximum": 65535, + "minimum": 1, + "required": [], + "type": "integer" + } + }, + "required": [], + "type": "object" + }, + "required": [], + "title": "nodePorts", + "type": "array" + }, "type": { "default": "LoadBalancer", "description": "The type of service to create for the NGINX data plane.", diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index c41ea7bb9c..fad7f6372b 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -367,10 +367,10 @@ nginx: # value: # type: string # @schema - # -- The configuration for the data plane that is contained in the NginxProxy resource. + # -- The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways. config: {} - # -- The pod configuration for the NGINX data plane pod. + # -- The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways. pod: {} # -- The termination grace period of the NGINX data plane pod. # terminationGracePeriodSeconds: 30 @@ -391,7 +391,7 @@ nginx: # nginx.container.extraVolumeMounts mount additional volumes to the container. # extraVolumes: [] - # -- The container configuration for the NGINX container. + # -- The container configuration for the NGINX container. This is applied globally to all Gateways. container: {} # -- The resource requirements of the NGINX container. # resources: {} @@ -402,7 +402,7 @@ nginx: # -- extraVolumeMounts are the additional volume mounts for the NGINX container. # extraVolumeMounts: [] - # -- The service configuration for the NGINX data plane. + # -- The service configuration for the NGINX data plane. This is applied globally to all Gateways. service: # @schema # enum: @@ -422,17 +422,39 @@ nginx: externalTrafficPolicy: Local # -- The annotations of the NGINX data plane service. - # annotations: {} + annotations: {} # -- The static IP address for the load balancer. Requires nginx.service.type set to LoadBalancer. - # loadBalancerIP: "" + loadBalancerIP: "" # -- LoadBalancerClass is the class of the load balancer implementation this Service belongs to. # Requires nginx.service.type set to LoadBalancer. - # loadBalancerClass: "" + loadBalancerClass: "" # -- The IP ranges (CIDR) that are allowed to access the load balancer. Requires nginx.service.type set to LoadBalancer. - # loadBalancerSourceRanges: [] + loadBalancerSourceRanges: [] + + # @schema + # type: array + # items: + # type: object + # properties: + # port: + # type: integer + # required: true + # minimum: 1 + # maximum: 65535 + # listenerPort: + # type: integer + # required: true + # minimum: 1 + # maximum: 65535 + # @schema + # -- A list of NodePorts to expose on the NGINX data plane service. Each NodePort MUST map to a Gateway listener port, + # otherwise it will be ignored. The default NodePort range enforced by Kubernetes is 30000-32767. + nodePorts: [] + # - port: 30025 + # listenerPort: 80 # -- Enable debugging for NGINX. Uses the nginx-debug binary. The NGINX error log level should be set to debug in the NginxProxy resource. debug: false diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml index 0e28520896..a74ea35b32 100644 --- a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml +++ b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml @@ -3498,6 +3498,36 @@ spec: items: type: string type: array + nodePorts: + description: |- + NodePorts are the list of NodePorts to expose on the NGINX data plane service. + Each NodePort MUST map to a Gateway listener port, otherwise it will be ignored. + The default NodePort range enforced by Kubernetes is 30000-32767. + items: + description: |- + NodePort creates a port on each node on which the NGINX data plane service is exposed. The NodePort MUST + map to a Gateway listener port, otherwise it will be ignored. If not specified, Kubernetes allocates a NodePort + automatically if required. The default NodePort range enforced by Kubernetes is 30000-32767. + properties: + listenerPort: + description: |- + ListenerPort is the Gateway listener port that this NodePort maps to. + kubebuilder:validation:Minimum=1 + kubebuilder:validation:Maximum=65535 + format: int32 + type: integer + port: + description: |- + Port is the NodePort to expose. + kubebuilder:validation:Minimum=1 + kubebuilder:validation:Maximum=65535 + format: int32 + type: integer + required: + - listenerPort + - port + type: object + type: array type: default: LoadBalancer description: ServiceType describes ingress method for the diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 56cd27eacc..abb653ccc4 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -4083,6 +4083,36 @@ spec: items: type: string type: array + nodePorts: + description: |- + NodePorts are the list of NodePorts to expose on the NGINX data plane service. + Each NodePort MUST map to a Gateway listener port, otherwise it will be ignored. + The default NodePort range enforced by Kubernetes is 30000-32767. + items: + description: |- + NodePort creates a port on each node on which the NGINX data plane service is exposed. The NodePort MUST + map to a Gateway listener port, otherwise it will be ignored. If not specified, Kubernetes allocates a NodePort + automatically if required. The default NodePort range enforced by Kubernetes is 30000-32767. + properties: + listenerPort: + description: |- + ListenerPort is the Gateway listener port that this NodePort maps to. + kubebuilder:validation:Minimum=1 + kubebuilder:validation:Maximum=65535 + format: int32 + type: integer + port: + description: |- + Port is the NodePort to expose. + kubebuilder:validation:Minimum=1 + kubebuilder:validation:Maximum=65535 + format: int32 + type: integer + required: + - listenerPort + - port + type: object + type: array type: default: LoadBalancer description: ServiceType describes ingress method for the diff --git a/internal/mode/static/provisioner/handler.go b/internal/mode/static/provisioner/handler.go index ef6ba76b82..ee7813fb96 100644 --- a/internal/mode/static/provisioner/handler.go +++ b/internal/mode/static/provisioner/handler.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "reflect" "strings" "github.com/go-logr/logr" @@ -66,7 +67,7 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, gatewayName := objLabels.Get(controller.GatewayLabel) gatewayNSName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: gatewayName} - if err := h.updateOrDeleteResources(ctx, obj, gatewayNSName); err != nil { + if err := h.updateOrDeleteResources(ctx, logger, obj, gatewayNSName); err != nil { logger.Error(err, "error handling resource update") } } @@ -76,7 +77,7 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, gatewayName := objLabels.Get(controller.GatewayLabel) gatewayNSName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: gatewayName} - if err := h.updateOrDeleteResources(ctx, obj, gatewayNSName); err != nil { + if err := h.updateOrDeleteResources(ctx, logger, obj, gatewayNSName); err != nil { logger.Error(err, "error handling resource update") } @@ -93,12 +94,12 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, gatewayName := objLabels.Get(controller.GatewayLabel) gatewayNSName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: gatewayName} - if err := h.updateOrDeleteResources(ctx, obj, gatewayNSName); err != nil { + if err := h.updateOrDeleteResources(ctx, logger, obj, gatewayNSName); err != nil { logger.Error(err, "error handling resource update") } } else if h.provisioner.isUserSecret(obj.GetName()) { - if err := h.provisionResourcesForAllGateways(ctx); err != nil { - logger.Error(err, "error updating resources") + if err := h.provisionResourceForAllGateways(ctx, logger, obj); err != nil { + logger.Error(err, "error updating resource") } } default: @@ -144,6 +145,7 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, // - are updated to the proper state in case a user makes a change directly to the resource. func (h *eventHandler) updateOrDeleteResources( ctx context.Context, + logger logr.Logger, obj client.Object, gatewayNSName types.NamespacedName, ) error { @@ -160,26 +162,55 @@ func (h *eventHandler) updateOrDeleteResources( return nil } + if h.store.getResourceVersionForObject(gatewayNSName, obj) == obj.GetResourceVersion() { + return nil + } + h.store.registerResourceInGatewayConfig(gatewayNSName, obj) - if err := h.provisionResources(ctx, gatewayNSName); err != nil { + if err := h.provisionResource(ctx, logger, gatewayNSName, obj); err != nil { return fmt.Errorf("error updating nginx resource: %w", err) } return nil } -func (h *eventHandler) provisionResources( +func (h *eventHandler) provisionResource( ctx context.Context, + logger logr.Logger, gatewayNSName types.NamespacedName, + obj client.Object, ) error { resources := h.store.getNginxResourcesForGateway(gatewayNSName) if resources != nil && resources.Gateway != nil { resourceName := controller.CreateNginxResourceName(gatewayNSName.Name, h.gcName) + + objects, err := h.provisioner.buildNginxResourceObjects( + resourceName, + resources.Gateway.Source, + resources.Gateway.EffectiveNginxProxy, + ) + if err != nil { + logger.Error(err, "error building some nginx resources") + } + + // only provision the object that was updated + var objectToProvision client.Object + for _, object := range objects { + if strings.HasSuffix(object.GetName(), obj.GetName()) && reflect.TypeOf(object) == reflect.TypeOf(obj) { + objectToProvision = object + break + } + } + + if objectToProvision == nil { + return nil + } + if err := h.provisioner.provisionNginx( ctx, resourceName, resources.Gateway.Source, - resources.Gateway.EffectiveNginxProxy, + []client.Object{objectToProvision}, ); err != nil { return fmt.Errorf("error updating nginx resource: %w", err) } @@ -204,13 +235,17 @@ func (h *eventHandler) reprovisionResources(ctx context.Context, event *events.D return nil } -// provisionResourcesForAllGateways is called when a resource is updated that needs to be applied +// provisionResourceForAllGateways is called when a resource is updated that needs to be applied // to all Gateway deployments. For example, NGINX Plus secrets. -func (h *eventHandler) provisionResourcesForAllGateways(ctx context.Context) error { +func (h *eventHandler) provisionResourceForAllGateways( + ctx context.Context, + logger logr.Logger, + obj client.Object, +) error { var allErrs []error gateways := h.store.getGateways() for gateway := range gateways { - if err := h.provisionResources(ctx, gateway); err != nil { + if err := h.provisionResource(ctx, logger, gateway, obj); err != nil { allErrs = append(allErrs, err) } } diff --git a/internal/mode/static/provisioner/handler_test.go b/internal/mode/static/provisioner/handler_test.go index 9fd0dcc8d1..1f44a27de0 100644 --- a/internal/mode/static/provisioner/handler_test.go +++ b/internal/mode/static/provisioner/handler_test.go @@ -48,25 +48,28 @@ func TestHandleEventBatch_Upsert(t *testing.T) { deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ - Name: "gw-nginx", - Namespace: "default", - Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "gw"}, + Name: "gw-nginx", + Namespace: "default", + ResourceVersion: "1", + Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "gw"}, }, } service := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: "gw-nginx", - Namespace: "default", - Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "test-gateway"}, + Name: "gw-nginx", + Namespace: "default", + ResourceVersion: "1", + Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "gw"}, }, } jwtSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "gw-nginx-" + jwtTestSecretName, - Namespace: "default", - Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "test-gateway"}, + Name: "gw-nginx-" + jwtTestSecretName, + Namespace: "default", + ResourceVersion: "1", + Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "gw"}, }, Data: map[string][]byte{ "data": []byte("oldData"), @@ -86,8 +89,10 @@ func TestHandleEventBatch_Upsert(t *testing.T) { dockerSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "gw-nginx-" + dockerTestSecretName, - Namespace: "default", + Name: "gw-nginx-" + dockerTestSecretName, + Namespace: "default", + ResourceVersion: "1", + Labels: map[string]string{"app": "nginx", controller.GatewayLabel: "gw"}, }, Data: map[string][]byte{ "data": []byte("oldDockerData"), @@ -130,6 +135,7 @@ func TestHandleEventBatch_Upsert(t *testing.T) { handler.HandleEventBatch(ctx, logger, batch) g.Expect(provisioner.cfg.StatusQueue.Dequeue(ctx)).ToNot(BeNil()) + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(service), &corev1.Service{})).To(Succeed()) // Test handling provisioned Secret upsertEvent = &events.UpsertEvent{Resource: jwtSecret} @@ -155,6 +161,10 @@ func TestHandleEventBatch_Upsert(t *testing.T) { g.Expect(secret.Data["data"]).To(Equal([]byte("newData"))) // Test handling user Docker Secret + upsertEvent = &events.UpsertEvent{Resource: dockerSecret} + batch = events.EventBatch{upsertEvent} + handler.HandleEventBatch(ctx, logger, batch) + g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(dockerSecret), secret)).To(Succeed()) g.Expect(secret.Data).To(HaveKey("data")) g.Expect(secret.Data["data"]).To(Equal([]byte("oldDockerData"))) @@ -181,6 +191,7 @@ func TestHandleEventBatch_Upsert(t *testing.T) { // do the same thing but when provisioner is not leader. // non-leader should not delete resources, but instead track them + deployment.ResourceVersion = "" g.Expect(fakeClient.Create(ctx, deployment)).To(Succeed()) provisioner.leader = false diff --git a/internal/mode/static/provisioner/objects.go b/internal/mode/static/provisioner/objects.go index 93ee801fef..57d6933b2c 100644 --- a/internal/mode/static/provisioner/objects.go +++ b/internal/mode/static/provisioner/objects.go @@ -438,6 +438,15 @@ func buildNginxService( Port: port, TargetPort: intstr.FromInt32(port), } + + if serviceType != corev1.ServiceTypeClusterIP { + for _, nodePort := range serviceCfg.NodePorts { + if nodePort.ListenerPort == port { + servicePort.NodePort = nodePort.Port + } + } + } + servicePorts = append(servicePorts, servicePort) } diff --git a/internal/mode/static/provisioner/objects_test.go b/internal/mode/static/provisioner/objects_test.go index aa3a8dd747..9225e8a0ce 100644 --- a/internal/mode/static/provisioner/objects_test.go +++ b/internal/mode/static/provisioner/objects_test.go @@ -92,7 +92,25 @@ func TestBuildNginxResourceObjects(t *testing.T) { } resourceName := "gw-nginx" - objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, &graph.EffectiveNginxProxy{}) + objects, err := provisioner.buildNginxResourceObjects( + resourceName, + gateway, + &graph.EffectiveNginxProxy{ + Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{ + Service: &ngfAPIv1alpha2.ServiceSpec{ + NodePorts: []ngfAPIv1alpha2.NodePort{ + { + Port: 30000, + ListenerPort: 80, + }, + { // ignored + Port: 31000, + ListenerPort: 789, + }, + }, + }, + }, + }) g.Expect(err).ToNot(HaveOccurred()) g.Expect(objects).To(HaveLen(6)) @@ -150,6 +168,7 @@ func TestBuildNginxResourceObjects(t *testing.T) { Port: 80, Name: "port-80", TargetPort: intstr.FromInt(80), + NodePort: 30000, }, { Port: 8888, diff --git a/internal/mode/static/provisioner/provisioner.go b/internal/mode/static/provisioner/provisioner.go index 3151d56c4f..42bd761ba2 100644 --- a/internal/mode/static/provisioner/provisioner.go +++ b/internal/mode/static/provisioner/provisioner.go @@ -185,17 +185,12 @@ func (p *NginxProvisioner) provisionNginx( ctx context.Context, resourceName string, gateway *gatewayv1.Gateway, - nProxyCfg *graph.EffectiveNginxProxy, + objects []client.Object, ) error { if !p.isLeader() { return nil } - objects, err := p.buildNginxResourceObjects(resourceName, gateway, nProxyCfg) - if err != nil { - p.cfg.Logger.Error(err, "error provisioning some nginx resources") - } - p.cfg.Logger.Info( "Creating/Updating nginx resources", "namespace", gateway.GetNamespace(), @@ -261,6 +256,7 @@ func (p *NginxProvisioner) provisionNginx( "namespace", gateway.GetNamespace(), "name", resourceName, ) + p.store.registerResourceInGatewayConfig(client.ObjectKeyFromObject(gateway), obj) } // if agent configmap was updated, then we'll need to restart the deployment @@ -430,7 +426,12 @@ func (p *NginxProvisioner) RegisterGateway( } if gateway.Valid { - if err := p.provisionNginx(ctx, resourceName, gateway.Source, gateway.EffectiveNginxProxy); err != nil { + objects, err := p.buildNginxResourceObjects(resourceName, gateway.Source, gateway.EffectiveNginxProxy) + if err != nil { + p.cfg.Logger.Error(err, "error building some nginx resources") + } + + if err := p.provisionNginx(ctx, resourceName, gateway.Source, objects); err != nil { return fmt.Errorf("error provisioning nginx resources: %w", err) } } else { diff --git a/internal/mode/static/provisioner/store.go b/internal/mode/static/provisioner/store.go index 0af617852c..a487d29fb9 100644 --- a/internal/mode/static/provisioner/store.go +++ b/internal/mode/static/provisioner/store.go @@ -107,7 +107,7 @@ func (s *store) getGateways() map[types.NamespacedName]*gatewayv1.Gateway { // If the object being updated is the Gateway, check if anything that we care about changed. This ensures that // we don't attempt to update nginx resources when the main event handler triggers this call with an unrelated event // (like a Route update) that shouldn't result in nginx resource changes. -func (s *store) registerResourceInGatewayConfig(gatewayNSName types.NamespacedName, object interface{}) bool { +func (s *store) registerResourceInGatewayConfig(gatewayNSName types.NamespacedName, object any) bool { s.lock.Lock() defer s.lock.Unlock() @@ -348,3 +348,75 @@ func secretResourceMatches(resources *NginxResources, nsName types.NamespacedNam func resourceMatches(objMeta metav1.ObjectMeta, nsName types.NamespacedName) bool { return objMeta.GetName() == nsName.Name && objMeta.GetNamespace() == nsName.Namespace } + +func (s *store) getResourceVersionForObject(gatewayNSName types.NamespacedName, object client.Object) string { + s.lock.RLock() + defer s.lock.RUnlock() + + resources, exists := s.nginxResources[gatewayNSName] + if !exists { + return "" + } + + switch obj := object.(type) { + case *appsv1.Deployment: + if resources.Deployment.GetName() == obj.GetName() { + return resources.Deployment.GetResourceVersion() + } + case *corev1.Service: + if resources.Service.GetName() == obj.GetName() { + return resources.Service.GetResourceVersion() + } + case *corev1.ServiceAccount: + if resources.ServiceAccount.GetName() == obj.GetName() { + return resources.ServiceAccount.GetResourceVersion() + } + case *rbacv1.Role: + if resources.Role.GetName() == obj.GetName() { + return resources.Role.GetResourceVersion() + } + case *rbacv1.RoleBinding: + if resources.RoleBinding.GetName() == obj.GetName() { + return resources.RoleBinding.GetResourceVersion() + } + case *corev1.ConfigMap: + return getResourceVersionForConfigMap(resources, obj) + case *corev1.Secret: + return getResourceVersionForSecret(resources, obj) + } + + return "" +} + +func getResourceVersionForConfigMap(resources *NginxResources, configmap *corev1.ConfigMap) string { + if resources.BootstrapConfigMap.GetName() == configmap.GetName() { + return resources.BootstrapConfigMap.GetResourceVersion() + } + if resources.AgentConfigMap.GetName() == configmap.GetName() { + return resources.AgentConfigMap.GetResourceVersion() + } + + return "" +} + +func getResourceVersionForSecret(resources *NginxResources, secret *corev1.Secret) string { + if resources.AgentTLSSecret.GetName() == secret.GetName() { + return resources.AgentTLSSecret.GetResourceVersion() + } + for _, dockerSecret := range resources.DockerSecrets { + if dockerSecret.GetName() == secret.GetName() { + return dockerSecret.GetResourceVersion() + } + } + if resources.PlusJWTSecret.GetName() == secret.GetName() { + return resources.PlusJWTSecret.GetResourceVersion() + } + if resources.PlusClientSSLSecret.GetName() == secret.GetName() { + return resources.PlusClientSSLSecret.GetResourceVersion() + } + if resources.PlusCASecret.GetName() == secret.GetName() { + return resources.PlusCASecret.GetResourceVersion() + } + + return "" +} diff --git a/internal/mode/static/provisioner/store_test.go b/internal/mode/static/provisioner/store_test.go index 59e7da1207..bc52728631 100644 --- a/internal/mode/static/provisioner/store_test.go +++ b/internal/mode/static/provisioner/store_test.go @@ -618,3 +618,221 @@ func TestGatewayExistsForResource(t *testing.T) { }) } } + +func TestGetResourceVersionForObject(t *testing.T) { + t.Parallel() + + store := newStore(nil, "", "", "", "") + nsName := types.NamespacedName{Name: "test-gateway", Namespace: "default"} + store.nginxResources[nsName] = &NginxResources{ + Deployment: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "default", + ResourceVersion: "1", + }, + Service: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + ResourceVersion: "2", + }, + ServiceAccount: metav1.ObjectMeta{ + Name: "test-serviceaccount", + Namespace: "default", + ResourceVersion: "3", + }, + Role: metav1.ObjectMeta{ + Name: "test-role", + Namespace: "default", + ResourceVersion: "4", + }, + RoleBinding: metav1.ObjectMeta{ + Name: "test-rolebinding", + Namespace: "default", + ResourceVersion: "5", + }, + BootstrapConfigMap: metav1.ObjectMeta{ + Name: "test-bootstrap-configmap", + Namespace: "default", + ResourceVersion: "6", + }, + AgentConfigMap: metav1.ObjectMeta{ + Name: "test-agent-configmap", + Namespace: "default", + ResourceVersion: "7", + }, + AgentTLSSecret: metav1.ObjectMeta{ + Name: "test-agent-tls-secret", + Namespace: "default", + ResourceVersion: "8", + }, + PlusJWTSecret: metav1.ObjectMeta{ + Name: "test-jwt-secret", + Namespace: "default", + ResourceVersion: "9", + }, + PlusCASecret: metav1.ObjectMeta{ + Name: "test-ca-secret", + Namespace: "default", + ResourceVersion: "10", + }, + PlusClientSSLSecret: metav1.ObjectMeta{ + Name: "test-client-ssl-secret", + Namespace: "default", + ResourceVersion: "11", + }, + DockerSecrets: []metav1.ObjectMeta{ + { + Name: "test-docker-secret", + Namespace: "default", + ResourceVersion: "12", + }, + }, + } + + tests := []struct { + name string + object client.Object + expectedResult string + }{ + { + name: "Deployment resource version", + object: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "default", + }, + }, + expectedResult: "1", + }, + { + name: "Service resource version", + object: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + }, + }, + expectedResult: "2", + }, + { + name: "ServiceAccount resource version", + object: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-serviceaccount", + Namespace: "default", + }, + }, + expectedResult: "3", + }, + { + name: "Role resource version", + object: &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-role", + Namespace: "default", + }, + }, + expectedResult: "4", + }, + { + name: "RoleBinding resource version", + object: &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rolebinding", + Namespace: "default", + }, + }, + expectedResult: "5", + }, + { + name: "Bootstrap ConfigMap resource version", + object: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bootstrap-configmap", + Namespace: "default", + }, + }, + expectedResult: "6", + }, + { + name: "Agent ConfigMap resource version", + object: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-agent-configmap", + Namespace: "default", + }, + }, + expectedResult: "7", + }, + { + name: "Agent TLS Secret resource version", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-agent-tls-secret", + Namespace: "default", + }, + }, + expectedResult: "8", + }, + { + name: "JWT Secret resource version", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-jwt-secret", + Namespace: "default", + }, + }, + expectedResult: "9", + }, + { + name: "CA Secret resource version", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ca-secret", + Namespace: "default", + }, + }, + expectedResult: "10", + }, + { + name: "Client SSL Secret resource version", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-client-ssl-secret", + Namespace: "default", + }, + }, + expectedResult: "11", + }, + { + name: "Docker Secret resource version", + object: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-docker-secret", + Namespace: "default", + }, + }, + expectedResult: "12", + }, + { + name: "Non-existent resource", + object: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-existent-service", + Namespace: "default", + }, + }, + expectedResult: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + result := store.getResourceVersionForObject(nsName, test.object) + g.Expect(result).To(Equal(test.expectedResult)) + }) + } +} From 7ee9489dca0bd4cabe4b889e965baf5a51a3fddb Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Wed, 30 Apr 2025 12:22:55 -0600 Subject: [PATCH 2/3] Add retry for failing check --- internal/mode/static/handler.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 3114bea746..6372b55a68 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -12,6 +12,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -472,8 +473,20 @@ func getGatewayAddresses( if svc == nil { svcName := controller.CreateNginxResourceName(gateway.Source.GetName(), gatewayClassName) key := types.NamespacedName{Name: svcName, Namespace: gateway.Source.GetNamespace()} - if err := k8sClient.Get(ctx, key, &gwSvc); err != nil { - return nil, fmt.Errorf("error finding Service for Gateway: %w", err) + + if err := wait.PollUntilContextCancel( + ctx, + 500*time.Millisecond, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + if err := k8sClient.Get(ctx, key, &gwSvc); err != nil { + return false, nil //nolint:nilerr // need to retry without returning error + } + + return true, nil + }, + ); err != nil { + return nil, fmt.Errorf("error finding Service %s for Gateway: %w", svcName, err) } } else { gwSvc = *svc From f62ed374c7fbdf2d5c8d424257f449bd38098295 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Wed, 30 Apr 2025 13:01:36 -0600 Subject: [PATCH 3/3] Fix failing handler test --- charts/nginx-gateway-fabric/README.md | 8 ++--- .../nginx-gateway-fabric/values.schema.json | 8 ++--- charts/nginx-gateway-fabric/values.yaml | 12 ++++--- internal/mode/static/handler.go | 5 ++- internal/mode/static/handler_test.go | 32 +++++++++++++++---- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/charts/nginx-gateway-fabric/README.md b/charts/nginx-gateway-fabric/README.md index 27637e1220..8ed532ea95 100644 --- a/charts/nginx-gateway-fabric/README.md +++ b/charts/nginx-gateway-fabric/README.md @@ -259,17 +259,17 @@ The following table lists the configurable parameters of the NGINX Gateway Fabri | `certGenerator.serverTLSSecretName` | The name of the Secret containing TLS CA, certificate, and key for the NGINX Gateway Fabric control plane to securely communicate with the NGINX Agent. Must exist in the same namespace that the NGINX Gateway Fabric control plane is running in (default namespace: nginx-gateway). | string | `"server-tls"` | | `clusterDomain` | The DNS cluster domain of your Kubernetes cluster. | string | `"cluster.local"` | | `nginx` | The nginx section contains the configuration for all NGINX data plane deployments installed by the NGINX Gateway Fabric control plane. | object | `{"config":{},"container":{},"debug":false,"image":{"pullPolicy":"Always","repository":"ghcr.io/nginx/nginx-gateway-fabric/nginx","tag":"edge"},"imagePullSecret":"","imagePullSecrets":[],"kind":"deployment","plus":false,"pod":{},"replicas":1,"service":{"annotations":{},"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"},"usage":{"caSecretName":"","clientSSLSecretName":"","endpoint":"","resolver":"","secretName":"nplus-license","skipVerify":false}}` | -| `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways. | object | `{}` | -| `nginx.container` | The container configuration for the NGINX container. This is applied globally to all Gateways. | object | `{}` | +| `nginx.config` | The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | +| `nginx.container` | The container configuration for the NGINX container. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.debug` | Enable debugging for NGINX. Uses the nginx-debug binary. The NGINX error log level should be set to debug in the NginxProxy resource. | bool | `false` | | `nginx.image.repository` | The NGINX image to use. | string | `"ghcr.io/nginx/nginx-gateway-fabric/nginx"` | | `nginx.imagePullSecret` | The name of the secret containing docker registry credentials. Secret must exist in the same namespace as the helm release. The control plane will copy this secret into any namespace where NGINX is deployed. | string | `""` | | `nginx.imagePullSecrets` | A list of secret names containing docker registry credentials. Secrets must exist in the same namespace as the helm release. The control plane will copy these secrets into any namespace where NGINX is deployed. | list | `[]` | | `nginx.kind` | The kind of NGINX deployment. | string | `"deployment"` | | `nginx.plus` | Is NGINX Plus image being used. | bool | `false` | -| `nginx.pod` | The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways. | object | `{}` | +| `nginx.pod` | The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{}` | | `nginx.replicas` | The number of replicas of the NGINX Deployment. | int | `1` | -| `nginx.service` | The service configuration for the NGINX data plane. This is applied globally to all Gateways. | object | `{"annotations":{},"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"}` | +| `nginx.service` | The service configuration for the NGINX data plane. This is applied globally to all Gateways managed by this instance of NGINX Gateway Fabric. | object | `{"annotations":{},"externalTrafficPolicy":"Local","loadBalancerClass":"","loadBalancerIP":"","loadBalancerSourceRanges":[],"nodePorts":[],"type":"LoadBalancer"}` | | `nginx.service.annotations` | The annotations of the NGINX data plane service. | object | `{}` | | `nginx.service.externalTrafficPolicy` | The externalTrafficPolicy of the service. The value Local preserves the client source IP. | string | `"Local"` | | `nginx.service.loadBalancerClass` | LoadBalancerClass is the class of the load balancer implementation this Service belongs to. Requires nginx.service.type set to LoadBalancer. | string | `""` | diff --git a/charts/nginx-gateway-fabric/values.schema.json b/charts/nginx-gateway-fabric/values.schema.json index 5b3f83c814..806512fce8 100644 --- a/charts/nginx-gateway-fabric/values.schema.json +++ b/charts/nginx-gateway-fabric/values.schema.json @@ -53,7 +53,7 @@ "description": "The nginx section contains the configuration for all NGINX data plane deployments\ninstalled by the NGINX Gateway Fabric control plane.", "properties": { "config": { - "description": "The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways.", + "description": "The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways\nmanaged by this instance of NGINX Gateway Fabric.", "properties": { "disableHTTP2": { "description": "DisableHTTP2 defines if http2 should be disabled for all servers.", @@ -266,7 +266,7 @@ "type": "object" }, "container": { - "description": "The container configuration for the NGINX container. This is applied globally to all Gateways.", + "description": "The container configuration for the NGINX container. This is applied globally to all Gateways managed by this\ninstance of NGINX Gateway Fabric.", "required": [], "title": "container", "type": "object" @@ -341,7 +341,7 @@ "type": "boolean" }, "pod": { - "description": "The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways.", + "description": "The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways managed by this\ninstance of NGINX Gateway Fabric.", "required": [], "title": "pod", "type": "object" @@ -354,7 +354,7 @@ "type": "integer" }, "service": { - "description": "The service configuration for the NGINX data plane. This is applied globally to all Gateways.", + "description": "The service configuration for the NGINX data plane. This is applied globally to all Gateways managed by this\ninstance of NGINX Gateway Fabric.", "properties": { "annotations": { "description": "The annotations of the NGINX data plane service.", diff --git a/charts/nginx-gateway-fabric/values.yaml b/charts/nginx-gateway-fabric/values.yaml index fad7f6372b..077faeff80 100644 --- a/charts/nginx-gateway-fabric/values.yaml +++ b/charts/nginx-gateway-fabric/values.yaml @@ -367,10 +367,12 @@ nginx: # value: # type: string # @schema - # -- The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways. + # -- The configuration for the data plane that is contained in the NginxProxy resource. This is applied globally to all Gateways + # managed by this instance of NGINX Gateway Fabric. config: {} - # -- The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways. + # -- The pod configuration for the NGINX data plane pod. This is applied globally to all Gateways managed by this + # instance of NGINX Gateway Fabric. pod: {} # -- The termination grace period of the NGINX data plane pod. # terminationGracePeriodSeconds: 30 @@ -391,7 +393,8 @@ nginx: # nginx.container.extraVolumeMounts mount additional volumes to the container. # extraVolumes: [] - # -- The container configuration for the NGINX container. This is applied globally to all Gateways. + # -- The container configuration for the NGINX container. This is applied globally to all Gateways managed by this + # instance of NGINX Gateway Fabric. container: {} # -- The resource requirements of the NGINX container. # resources: {} @@ -402,7 +405,8 @@ nginx: # -- extraVolumeMounts are the additional volume mounts for the NGINX container. # extraVolumeMounts: [] - # -- The service configuration for the NGINX data plane. This is applied globally to all Gateways. + # -- The service configuration for the NGINX data plane. This is applied globally to all Gateways managed by this + # instance of NGINX Gateway Fabric. service: # @schema # enum: diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 6372b55a68..2e2fe6e9c2 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -474,8 +474,11 @@ func getGatewayAddresses( svcName := controller.CreateNginxResourceName(gateway.Source.GetName(), gatewayClassName) key := types.NamespacedName{Name: svcName, Namespace: gateway.Source.GetNamespace()} + pollCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if err := wait.PollUntilContextCancel( - ctx, + pollCtx, 500*time.Millisecond, true, /* poll immediately */ func(ctx context.Context) (bool, error) { diff --git a/internal/mode/static/handler_test.go b/internal/mode/static/handler_test.go index df5ee9d70a..59ed8d8d2a 100644 --- a/internal/mode/static/handler_test.go +++ b/internal/mode/static/handler_test.go @@ -3,6 +3,7 @@ package static import ( "context" "errors" + "time" "github.com/go-logr/logr" pb "github.com/nginx/agent/v3/api/grpc/mpi/v1" @@ -87,8 +88,13 @@ var _ = Describe("eventHandler", func() { baseGraph = &graph.Graph{ Gateways: map[types.NamespacedName]*graph.Gateway{ {Namespace: "test", Name: "gateway"}: { - Valid: true, - Source: &gatewayv1.Gateway{}, + Valid: true, + Source: &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gateway", + Namespace: "test", + }, + }, DeploymentName: types.NamespacedName{ Namespace: "test", Name: controller.CreateNginxResourceName("gateway", "nginx"), @@ -107,9 +113,19 @@ var _ = Describe("eventHandler", func() { fakeStatusUpdater = &statusfakes.FakeGroupUpdater{} fakeEventRecorder = record.NewFakeRecorder(1) zapLogLevelSetter = newZapLogLevelSetter(zap.NewAtomicLevel()) - fakeK8sClient = fake.NewFakeClient() queue = status.NewQueue() + gatewaySvc := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway-nginx", + }, + Spec: v1.ServiceSpec{ + ClusterIP: "1.2.3.4", + }, + } + fakeK8sClient = fake.NewFakeClient(gatewaySvc) + handler = newEventHandlerImpl(eventHandlerConfig{ ctx: ctx, k8sClient: fakeK8sClient, @@ -520,16 +536,20 @@ var _ = Describe("getGatewayAddresses", func() { It("gets gateway addresses from a Service", func() { fakeClient := fake.NewFakeClient() - // no Service exists yet, should get error and Pod Address + // no Service exists yet, should get error and no Address gateway := &graph.Gateway{ Source: &gatewayv1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Name: "gateway", - Namespace: "test-ns", + Namespace: "test", }, }, } - addrs, err := getGatewayAddresses(context.Background(), fakeClient, nil, gateway, "nginx") + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + addrs, err := getGatewayAddresses(ctx, fakeClient, nil, gateway, "nginx") Expect(err).To(HaveOccurred()) Expect(addrs).To(BeNil())