From 6fad00544607fe5f0b2e6653da7f9113da63219a Mon Sep 17 00:00:00 2001 From: Kate Osborn <50597707+kate-osborn@users.noreply.github.com> Date: Fri, 20 Dec 2024 16:05:28 -0700 Subject: [PATCH] Add UpstreamSetttingsPolicy (#2941) Problem: As a user, I want to be able to configure the upstream settings for a Service referenced by a HTTP or GRPCRoute. Solution: Add UpstreamSettingsPolicy CRD. This is a direct policy that can be attached to one or more Services. The Service must be referenced by an HTTP or GRPCRoute that is owned by the "winning" NGF Gateway. Co-authored-by: bjee19 <139261241+bjee19@users.noreply.github.com> Co-authored-by: salonichf5 <146118978+salonichf5@users.noreply.github.com> --- apis/v1alpha1/policy_methods.go | 12 + apis/v1alpha1/register.go | 2 + apis/v1alpha1/upstreamsettingspolicy_types.go | 97 ++++ apis/v1alpha1/zz_generated.deepcopy.go | 124 ++++ .../templates/clusterrole.yaml | 2 + ...ay.nginx.org_upstreamsettingspolicies.yaml | 444 +++++++++++++++ config/crd/kustomization.yaml | 1 + deploy/aws-nlb/deploy.yaml | 2 + deploy/azure/deploy.yaml | 2 + deploy/crds.yaml | 444 +++++++++++++++ deploy/default/deploy.yaml | 2 + deploy/experimental-nginx-plus/deploy.yaml | 2 + deploy/experimental/deploy.yaml | 2 + deploy/nginx-plus/deploy.yaml | 2 + deploy/nodeport/deploy.yaml | 2 + deploy/openshift/deploy.yaml | 2 + .../snippets-filters-nginx-plus/deploy.yaml | 2 + deploy/snippets-filters/deploy.yaml | 2 + docs/proposals/upstream-settings.md | 2 +- examples/upstream-settings-policy/README.md | 4 + .../upstream-settings-policy/cafe-routes.yaml | 37 ++ examples/upstream-settings-policy/cafe.yaml | 65 +++ .../upstream-settings-policy/gateway.yaml | 11 + .../upstream-settings-policy.yaml | 15 + internal/framework/kinds/kinds.go | 12 +- internal/mode/static/manager.go | 12 + internal/mode/static/manager_test.go | 5 + .../mode/static/nginx/config/generator.go | 17 +- .../mode/static/nginx/config/http/config.go | 13 +- .../policies/clientsettings/validator.go | 4 +- .../policies/observability/validator.go | 4 +- .../static/nginx/config/policies/policy.go | 9 +- .../policies/upstreamsettings/processor.go | 67 +++ .../upstreamsettings/processor_test.go | 334 +++++++++++ .../policies/upstreamsettings/validator.go | 125 +++++ .../upstreamsettings/validator_test.go | 266 +++++++++ internal/mode/static/nginx/config/servers.go | 215 +++---- .../mode/static/nginx/config/servers_test.go | 431 +++++++++----- .../mode/static/nginx/config/upstreams.go | 54 +- .../static/nginx/config/upstreams_template.go | 32 ++ .../static/nginx/config/upstreams_test.go | 479 +++++++++++++++- .../mode/static/state/change_processor.go | 5 + .../static/state/change_processor_test.go | 127 +++-- .../static/state/dataplane/configuration.go | 16 +- .../state/dataplane/configuration_test.go | 63 ++- internal/mode/static/state/dataplane/types.go | 2 + internal/mode/static/state/graph/graph.go | 22 +- .../mode/static/state/graph/graph_test.go | 46 +- internal/mode/static/state/graph/policies.go | 102 +++- .../mode/static/state/graph/policies_test.go | 392 ++++++++++--- .../static/state/graph/policy_ancestor.go | 34 ++ .../state/graph/policy_ancestor_test.go | 110 ++++ internal/mode/static/state/graph/service.go | 52 +- .../mode/static/state/graph/service_test.go | 143 +++-- internal/mode/static/telemetry/collector.go | 4 + .../mode/static/telemetry/collector_test.go | 31 +- internal/mode/static/telemetry/data.avdl | 3 + internal/mode/static/telemetry/data_test.go | 3 + .../ngfresourcecounts_attributes_generated.go | 1 + site/content/overview/product-telemetry.md | 2 +- site/content/reference/api.md | 280 +++++++++- tests/framework/crossplane.go | 64 ++- tests/suite/client_settings_test.go | 16 +- .../upstream-settings-policy/cafe.yaml | 66 +++ .../upstream-settings-policy/gateway.yaml | 11 + .../grpc-backend.yaml | 39 ++ .../invalid-svc-usps.yaml | 15 + .../invalid-target-usps.yaml | 81 +++ .../upstream-settings-policy/routes.yaml | 54 ++ .../valid-merge-usps.yaml | 60 ++ .../upstream-settings-policy/valid-usps.yaml | 33 ++ tests/suite/snippets_filter_test.go | 8 +- tests/suite/telemetry_test.go | 1 + tests/suite/upstream_settings_test.go | 528 ++++++++++++++++++ 74 files changed, 5219 insertions(+), 554 deletions(-) create mode 100644 apis/v1alpha1/upstreamsettingspolicy_types.go create mode 100644 config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml create mode 100644 examples/upstream-settings-policy/README.md create mode 100644 examples/upstream-settings-policy/cafe-routes.yaml create mode 100644 examples/upstream-settings-policy/cafe.yaml create mode 100644 examples/upstream-settings-policy/gateway.yaml create mode 100644 examples/upstream-settings-policy/upstream-settings-policy.yaml create mode 100644 internal/mode/static/nginx/config/policies/upstreamsettings/processor.go create mode 100644 internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go create mode 100644 internal/mode/static/nginx/config/policies/upstreamsettings/validator.go create mode 100644 internal/mode/static/nginx/config/policies/upstreamsettings/validator_test.go create mode 100644 tests/suite/manifests/upstream-settings-policy/cafe.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/gateway.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/routes.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/valid-usps.yaml create mode 100644 tests/suite/upstream_settings_test.go diff --git a/apis/v1alpha1/policy_methods.go b/apis/v1alpha1/policy_methods.go index 97e7074a62..ad399c0897 100644 --- a/apis/v1alpha1/policy_methods.go +++ b/apis/v1alpha1/policy_methods.go @@ -31,3 +31,15 @@ func (p *ObservabilityPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { func (p *ObservabilityPolicy) SetPolicyStatus(status v1alpha2.PolicyStatus) { p.Status = status } + +func (p *UpstreamSettingsPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { + return p.Spec.TargetRefs +} + +func (p *UpstreamSettingsPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { + return p.Status +} + +func (p *UpstreamSettingsPolicy) SetPolicyStatus(status v1alpha2.PolicyStatus) { + p.Status = status +} diff --git a/apis/v1alpha1/register.go b/apis/v1alpha1/register.go index f9970f4b4c..0d18c29eaa 100644 --- a/apis/v1alpha1/register.go +++ b/apis/v1alpha1/register.go @@ -42,6 +42,8 @@ func addKnownTypes(scheme *runtime.Scheme) error { &ClientSettingsPolicyList{}, &SnippetsFilter{}, &SnippetsFilterList{}, + &UpstreamSettingsPolicy{}, + &UpstreamSettingsPolicyList{}, ) // AddToGroupVersion allows the serialization of client types like ListOptions. metav1.AddToGroupVersion(scheme, SchemeGroupVersion) diff --git a/apis/v1alpha1/upstreamsettingspolicy_types.go b/apis/v1alpha1/upstreamsettingspolicy_types.go new file mode 100644 index 0000000000..f3276d0f69 --- /dev/null +++ b/apis/v1alpha1/upstreamsettingspolicy_types.go @@ -0,0 +1,97 @@ +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +// +genclient +// +kubebuilder:object:root=true +// +kubebuilder:storageversion +// +kubebuilder:subresource:status +// +kubebuilder:resource:categories=nginx-gateway-fabric,scope=Namespaced,shortName=uspolicy +// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` +// +kubebuilder:metadata:labels="gateway.networking.k8s.io/policy=direct" + +// UpstreamSettingsPolicy is a Direct Attached Policy. It provides a way to configure the behavior of +// the connection between NGINX and the upstream applications. +type UpstreamSettingsPolicy struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + // Spec defines the desired state of the UpstreamSettingsPolicy. + Spec UpstreamSettingsPolicySpec `json:"spec"` + + // Status defines the state of the UpstreamSettingsPolicy. + Status gatewayv1alpha2.PolicyStatus `json:"status,omitempty"` +} + +// +kubebuilder:object:root=true + +// UpstreamSettingsPolicyList contains a list of UpstreamSettingsPolicies. +type UpstreamSettingsPolicyList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []UpstreamSettingsPolicy `json:"items"` +} + +// UpstreamSettingsPolicySpec defines the desired state of the UpstreamSettingsPolicy. +type UpstreamSettingsPolicySpec struct { + // ZoneSize is the size of the shared memory zone used by the upstream. This memory zone is used to share + // the upstream configuration between nginx worker processes. The more servers that an upstream has, + // the larger memory zone is required. + // Default: OSS: 512k, Plus: 1m. + // Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#zone + // + // +optional + ZoneSize *Size `json:"zoneSize,omitempty"` + + // KeepAlive defines the keep-alive settings. + // + // +optional + KeepAlive *UpstreamKeepAlive `json:"keepAlive,omitempty"` + + // TargetRefs identifies API object(s) to apply the policy to. + // Objects must be in the same namespace as the policy. + // Support: Service + // + // +kubebuilder:validation:MinItems=1 + // +kubebuilder:validation:MaxItems=16 + // +kubebuilder:validation:XValidation:message="TargetRefs Kind must be: Service",rule="self.all(t, t.kind=='Service')" + // +kubebuilder:validation:XValidation:message="TargetRefs Group must be core",rule="self.exists(t, t.group=='') || self.exists(t, t.group=='core')" + //nolint:lll + TargetRefs []gatewayv1alpha2.LocalPolicyTargetReference `json:"targetRefs"` +} + +// UpstreamKeepAlive defines the keep-alive settings for upstreams. +type UpstreamKeepAlive struct { + // Connections sets the maximum number of idle keep-alive connections to upstream servers that are preserved + // in the cache of each nginx worker process. When this number is exceeded, the least recently used + // connections are closed. + // Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive + // + // +optional + // +kubebuilder:validation:Minimum=1 + Connections *int32 `json:"connections,omitempty"` + + // Requests sets the maximum number of requests that can be served through one keep-alive connection. + // After the maximum number of requests are made, the connection is closed. + // Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_requests + // + // +optional + // +kubebuilder:validation:Minimum=0 + Requests *int32 `json:"requests,omitempty"` + + // Time defines the maximum time during which requests can be processed through one keep-alive connection. + // After this time is reached, the connection is closed following the subsequent request processing. + // Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_time + // + // +optional + Time *Duration `json:"time,omitempty"` + + // Timeout defines the keep-alive timeout for upstreams. + // Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_timeout + // + // +optional + Timeout *Duration `json:"timeout,omitempty"` +} diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index aa249ed430..9624b658aa 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -785,3 +785,127 @@ func (in *Tracing) DeepCopy() *Tracing { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *UpstreamKeepAlive) DeepCopyInto(out *UpstreamKeepAlive) { + *out = *in + if in.Connections != nil { + in, out := &in.Connections, &out.Connections + *out = new(int32) + **out = **in + } + if in.Requests != nil { + in, out := &in.Requests, &out.Requests + *out = new(int32) + **out = **in + } + if in.Time != nil { + in, out := &in.Time, &out.Time + *out = new(Duration) + **out = **in + } + if in.Timeout != nil { + in, out := &in.Timeout, &out.Timeout + *out = new(Duration) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpstreamKeepAlive. +func (in *UpstreamKeepAlive) DeepCopy() *UpstreamKeepAlive { + if in == nil { + return nil + } + out := new(UpstreamKeepAlive) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *UpstreamSettingsPolicy) DeepCopyInto(out *UpstreamSettingsPolicy) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + in.Spec.DeepCopyInto(&out.Spec) + in.Status.DeepCopyInto(&out.Status) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpstreamSettingsPolicy. +func (in *UpstreamSettingsPolicy) DeepCopy() *UpstreamSettingsPolicy { + if in == nil { + return nil + } + out := new(UpstreamSettingsPolicy) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *UpstreamSettingsPolicy) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *UpstreamSettingsPolicyList) DeepCopyInto(out *UpstreamSettingsPolicyList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]UpstreamSettingsPolicy, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpstreamSettingsPolicyList. +func (in *UpstreamSettingsPolicyList) DeepCopy() *UpstreamSettingsPolicyList { + if in == nil { + return nil + } + out := new(UpstreamSettingsPolicyList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *UpstreamSettingsPolicyList) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *UpstreamSettingsPolicySpec) DeepCopyInto(out *UpstreamSettingsPolicySpec) { + *out = *in + if in.ZoneSize != nil { + in, out := &in.ZoneSize, &out.ZoneSize + *out = new(Size) + **out = **in + } + if in.KeepAlive != nil { + in, out := &in.KeepAlive, &out.KeepAlive + *out = new(UpstreamKeepAlive) + (*in).DeepCopyInto(*out) + } + if in.TargetRefs != nil { + in, out := &in.TargetRefs, &out.TargetRefs + *out = make([]v1alpha2.LocalPolicyTargetReference, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpstreamSettingsPolicySpec. +func (in *UpstreamSettingsPolicySpec) DeepCopy() *UpstreamSettingsPolicySpec { + if in == nil { + return nil + } + out := new(UpstreamSettingsPolicySpec) + in.DeepCopyInto(out) + return out +} diff --git a/charts/nginx-gateway-fabric/templates/clusterrole.yaml b/charts/nginx-gateway-fabric/templates/clusterrole.yaml index cbb163ae1d..9ee1be4254 100644 --- a/charts/nginx-gateway-fabric/templates/clusterrole.yaml +++ b/charts/nginx-gateway-fabric/templates/clusterrole.yaml @@ -104,6 +104,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies {{- if .Values.nginxGateway.snippetsFilters.enable }} - snippetsfilters {{- end }} @@ -116,6 +117,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status {{- if .Values.nginxGateway.snippetsFilters.enable }} - snippetsfilters/status {{- end }} diff --git a/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml new file mode 100644 index 0000000000..992af79932 --- /dev/null +++ b/config/crd/bases/gateway.nginx.org_upstreamsettingspolicies.yaml @@ -0,0 +1,444 @@ +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.16.5 + labels: + gateway.networking.k8s.io/policy: direct + name: upstreamsettingspolicies.gateway.nginx.org +spec: + group: gateway.nginx.org + names: + categories: + - nginx-gateway-fabric + kind: UpstreamSettingsPolicy + listKind: UpstreamSettingsPolicyList + plural: upstreamsettingspolicies + shortNames: + - uspolicy + singular: upstreamsettingspolicy + scope: Namespaced + versions: + - additionalPrinterColumns: + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 + schema: + openAPIV3Schema: + description: |- + UpstreamSettingsPolicy is a Direct Attached Policy. It provides a way to configure the behavior of + the connection between NGINX and the upstream applications. + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + spec: + description: Spec defines the desired state of the UpstreamSettingsPolicy. + properties: + keepAlive: + description: KeepAlive defines the keep-alive settings. + properties: + connections: + description: |- + Connections sets the maximum number of idle keep-alive connections to upstream servers that are preserved + in the cache of each nginx worker process. When this number is exceeded, the least recently used + connections are closed. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive + format: int32 + minimum: 1 + type: integer + requests: + description: |- + Requests sets the maximum number of requests that can be served through one keep-alive connection. + After the maximum number of requests are made, the connection is closed. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_requests + format: int32 + minimum: 0 + type: integer + time: + description: |- + Time defines the maximum time during which requests can be processed through one keep-alive connection. + After this time is reached, the connection is closed following the subsequent request processing. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_time + pattern: ^[0-9]{1,4}(ms|s|m|h)?$ + type: string + timeout: + description: |- + Timeout defines the keep-alive timeout for upstreams. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_timeout + pattern: ^[0-9]{1,4}(ms|s|m|h)?$ + type: string + type: object + targetRefs: + description: |- + TargetRefs identifies API object(s) to apply the policy to. + Objects must be in the same namespace as the policy. + Support: Service + items: + description: |- + LocalPolicyTargetReference identifies an API object to apply a direct or + inherited policy to. This should be used as part of Policy resources + that can target Gateway API resources. For more information on how this + policy attachment model works, and a sample Policy resource, refer to + the policy attachment documentation for Gateway API. + properties: + group: + description: Group is the group of the target resource. + maxLength: 253 + pattern: ^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + kind: + description: Kind is kind of the target resource. + maxLength: 63 + minLength: 1 + pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$ + type: string + name: + description: Name is the name of the target resource. + maxLength: 253 + minLength: 1 + type: string + required: + - group + - kind + - name + type: object + maxItems: 16 + minItems: 1 + type: array + x-kubernetes-validations: + - message: 'TargetRefs Kind must be: Service' + rule: self.all(t, t.kind=='Service') + - message: TargetRefs Group must be core + rule: self.exists(t, t.group=='') || self.exists(t, t.group=='core') + zoneSize: + description: |- + ZoneSize is the size of the shared memory zone used by the upstream. This memory zone is used to share + the upstream configuration between nginx worker processes. The more servers that an upstream has, + the larger memory zone is required. + Default: OSS: 512k, Plus: 1m. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#zone + pattern: ^\d{1,4}(k|m|g)?$ + type: string + required: + - targetRefs + type: object + status: + description: Status defines the state of the UpstreamSettingsPolicy. + properties: + ancestors: + description: |- + Ancestors is a list of ancestor resources (usually Gateways) that are + associated with the policy, and the status of the policy with respect to + each ancestor. When this policy attaches to a parent, the controller that + manages the parent and the ancestors MUST add an entry to this list when + the controller first sees the policy and SHOULD update the entry as + appropriate when the relevant ancestor is modified. + + Note that choosing the relevant ancestor is left to the Policy designers; + an important part of Policy design is designing the right object level at + which to namespace this status. + + Note also that implementations MUST ONLY populate ancestor status for + the Ancestor resources they are responsible for. Implementations MUST + use the ControllerName field to uniquely identify the entries in this list + that they are responsible for. + + Note that to achieve this, the list of PolicyAncestorStatus structs + MUST be treated as a map with a composite key, made up of the AncestorRef + and ControllerName fields combined. + + A maximum of 16 ancestors will be represented in this list. An empty list + means the Policy is not relevant for any ancestors. + + If this slice is full, implementations MUST NOT add further entries. + Instead they MUST consider the policy unimplementable and signal that + on any related resources such as the ancestor that would be referenced + here. For example, if this list was full on BackendTLSPolicy, no + additional Gateways would be able to reference the Service targeted by + the BackendTLSPolicy. + items: + description: |- + PolicyAncestorStatus describes the status of a route with respect to an + associated Ancestor. + + Ancestors refer to objects that are either the Target of a policy or above it + in terms of object hierarchy. For example, if a policy targets a Service, the + Policy's Ancestors are, in order, the Service, the HTTPRoute, the Gateway, and + the GatewayClass. Almost always, in this hierarchy, the Gateway will be the most + useful object to place Policy status on, so we recommend that implementations + SHOULD use Gateway as the PolicyAncestorStatus object unless the designers + have a _very_ good reason otherwise. + + In the context of policy attachment, the Ancestor is used to distinguish which + resource results in a distinct application of this policy. For example, if a policy + targets a Service, it may have a distinct result per attached Gateway. + + Policies targeting the same resource may have different effects depending on the + ancestors of those resources. For example, different Gateways targeting the same + Service may have different capabilities, especially if they have different underlying + implementations. + + For example, in BackendTLSPolicy, the Policy attaches to a Service that is + used as a backend in a HTTPRoute that is itself attached to a Gateway. + In this case, the relevant object for status is the Gateway, and that is the + ancestor object referred to in this status. + + Note that a parent is also an ancestor, so for objects where the parent is the + relevant object for status, this struct SHOULD still be used. + + This struct is intended to be used in a slice that's effectively a map, + with a composite key made up of the AncestorRef and the ControllerName. + properties: + ancestorRef: + description: |- + AncestorRef corresponds with a ParentRef in the spec that this + PolicyAncestorStatus struct describes the status of. + properties: + group: + default: gateway.networking.k8s.io + description: |- + Group is the group of the referent. + When unspecified, "gateway.networking.k8s.io" is inferred. + To set the core API group (such as for a "Service" kind referent), + Group must be explicitly set to "" (empty string). + + Support: Core + maxLength: 253 + pattern: ^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + kind: + default: Gateway + description: |- + Kind is kind of the referent. + + There are two kinds of parent resources with "Core" support: + + * Gateway (Gateway conformance profile) + * Service (Mesh conformance profile, ClusterIP Services only) + + Support for other resources is Implementation-Specific. + maxLength: 63 + minLength: 1 + pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$ + type: string + name: + description: |- + Name is the name of the referent. + + Support: Core + maxLength: 253 + minLength: 1 + type: string + namespace: + description: |- + Namespace is the namespace of the referent. When unspecified, this refers + to the local namespace of the Route. + + Note that there are specific rules for ParentRefs which cross namespace + boundaries. Cross-namespace references are only valid if they are explicitly + allowed by something in the namespace they are referring to. For example: + Gateway has the AllowedRoutes field, and ReferenceGrant provides a + generic way to enable any other kind of cross-namespace reference. + + + ParentRefs from a Route to a Service in the same namespace are "producer" + routes, which apply default routing rules to inbound connections from + any namespace to the Service. + + ParentRefs from a Route to a Service in a different namespace are + "consumer" routes, and these routing rules are only applied to outbound + connections originating from the same namespace as the Route, for which + the intended destination of the connections are a Service targeted as a + ParentRef of the Route. + + + Support: Core + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string + port: + description: |- + Port is the network port this Route targets. It can be interpreted + differently based on the type of parent resource. + + When the parent resource is a Gateway, this targets all listeners + listening on the specified port that also support this kind of Route(and + select this Route). It's not recommended to set `Port` unless the + networking behaviors specified in a Route must apply to a specific port + as opposed to a listener(s) whose port(s) may be changed. When both Port + and SectionName are specified, the name and port of the selected listener + must match both specified values. + + + When the parent resource is a Service, this targets a specific port in the + Service spec. When both Port (experimental) and SectionName are specified, + the name and port of the selected port must match both specified values. + + + Implementations MAY choose to support other parent resources. + Implementations supporting other types of parent resources MUST clearly + document how/if Port is interpreted. + + For the purpose of status, an attachment is considered successful as + long as the parent resource accepts it partially. For example, Gateway + listeners can restrict which Routes can attach to them by Route kind, + namespace, or hostname. If 1 of 2 Gateway listeners accept attachment + from the referencing Route, the Route MUST be considered successfully + attached. If no Gateway listeners accept attachment from this Route, + the Route MUST be considered detached from the Gateway. + + Support: Extended + format: int32 + maximum: 65535 + minimum: 1 + type: integer + sectionName: + description: |- + SectionName is the name of a section within the target resource. In the + following resources, SectionName is interpreted as the following: + + * Gateway: Listener name. When both Port (experimental) and SectionName + are specified, the name and port of the selected listener must match + both specified values. + * Service: Port name. When both Port (experimental) and SectionName + are specified, the name and port of the selected listener must match + both specified values. + + Implementations MAY choose to support attaching Routes to other resources. + If that is the case, they MUST clearly document how SectionName is + interpreted. + + When unspecified (empty string), this will reference the entire resource. + For the purpose of status, an attachment is considered successful if at + least one section in the parent resource accepts it. For example, Gateway + listeners can restrict which Routes can attach to them by Route kind, + namespace, or hostname. If 1 of 2 Gateway listeners accept attachment from + the referencing Route, the Route MUST be considered successfully + attached. If no Gateway listeners accept attachment from this Route, the + Route MUST be considered detached from the Gateway. + + Support: Core + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + required: + - name + type: object + conditions: + description: Conditions describes the status of the Policy with + respect to the given Ancestor. + items: + description: Condition contains details for one aspect of + the current state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, + Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + maxItems: 8 + minItems: 1 + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map + controllerName: + description: |- + ControllerName is a domain/path string that indicates the name of the + controller that wrote this status. This corresponds with the + controllerName field on GatewayClass. + + Example: "example.net/gateway-controller". + + The format of this field is DOMAIN "/" PATH, where DOMAIN and PATH are + valid Kubernetes names + (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names). + + Controllers MUST populate this field when writing status. Controllers should ensure that + entries to status populated with their ControllerName are cleaned up when they are no + longer necessary. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$ + type: string + required: + - ancestorRef + - controllerName + type: object + maxItems: 16 + type: array + required: + - ancestors + type: object + required: + - spec + type: object + served: true + storage: true + subresources: + status: {} diff --git a/config/crd/kustomization.yaml b/config/crd/kustomization.yaml index 0258b9baaa..b067d64141 100644 --- a/config/crd/kustomization.yaml +++ b/config/crd/kustomization.yaml @@ -6,3 +6,4 @@ resources: - bases/gateway.nginx.org_nginxproxies.yaml - bases/gateway.nginx.org_observabilitypolicies.yaml - bases/gateway.nginx.org_snippetsfilters.yaml + - bases/gateway.nginx.org_upstreamsettingspolicies.yaml diff --git a/deploy/aws-nlb/deploy.yaml b/deploy/aws-nlb/deploy.yaml index 6e4ff4865f..018cf36107 100644 --- a/deploy/aws-nlb/deploy.yaml +++ b/deploy/aws-nlb/deploy.yaml @@ -98,6 +98,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -107,6 +108,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/azure/deploy.yaml b/deploy/azure/deploy.yaml index 06ba7a02ae..a8dc451b7e 100644 --- a/deploy/azure/deploy.yaml +++ b/deploy/azure/deploy.yaml @@ -98,6 +98,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -107,6 +108,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 83fe910a61..5a9d708c60 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -1482,3 +1482,447 @@ spec: storage: true subresources: status: {} +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.16.5 + labels: + gateway.networking.k8s.io/policy: direct + name: upstreamsettingspolicies.gateway.nginx.org +spec: + group: gateway.nginx.org + names: + categories: + - nginx-gateway-fabric + kind: UpstreamSettingsPolicy + listKind: UpstreamSettingsPolicyList + plural: upstreamsettingspolicies + shortNames: + - uspolicy + singular: upstreamsettingspolicy + scope: Namespaced + versions: + - additionalPrinterColumns: + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 + schema: + openAPIV3Schema: + description: |- + UpstreamSettingsPolicy is a Direct Attached Policy. It provides a way to configure the behavior of + the connection between NGINX and the upstream applications. + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + spec: + description: Spec defines the desired state of the UpstreamSettingsPolicy. + properties: + keepAlive: + description: KeepAlive defines the keep-alive settings. + properties: + connections: + description: |- + Connections sets the maximum number of idle keep-alive connections to upstream servers that are preserved + in the cache of each nginx worker process. When this number is exceeded, the least recently used + connections are closed. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive + format: int32 + minimum: 1 + type: integer + requests: + description: |- + Requests sets the maximum number of requests that can be served through one keep-alive connection. + After the maximum number of requests are made, the connection is closed. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_requests + format: int32 + minimum: 0 + type: integer + time: + description: |- + Time defines the maximum time during which requests can be processed through one keep-alive connection. + After this time is reached, the connection is closed following the subsequent request processing. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_time + pattern: ^[0-9]{1,4}(ms|s|m|h)?$ + type: string + timeout: + description: |- + Timeout defines the keep-alive timeout for upstreams. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_timeout + pattern: ^[0-9]{1,4}(ms|s|m|h)?$ + type: string + type: object + targetRefs: + description: |- + TargetRefs identifies API object(s) to apply the policy to. + Objects must be in the same namespace as the policy. + Support: Service + items: + description: |- + LocalPolicyTargetReference identifies an API object to apply a direct or + inherited policy to. This should be used as part of Policy resources + that can target Gateway API resources. For more information on how this + policy attachment model works, and a sample Policy resource, refer to + the policy attachment documentation for Gateway API. + properties: + group: + description: Group is the group of the target resource. + maxLength: 253 + pattern: ^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + kind: + description: Kind is kind of the target resource. + maxLength: 63 + minLength: 1 + pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$ + type: string + name: + description: Name is the name of the target resource. + maxLength: 253 + minLength: 1 + type: string + required: + - group + - kind + - name + type: object + maxItems: 16 + minItems: 1 + type: array + x-kubernetes-validations: + - message: 'TargetRefs Kind must be: Service' + rule: self.all(t, t.kind=='Service') + - message: TargetRefs Group must be core + rule: self.exists(t, t.group=='') || self.exists(t, t.group=='core') + zoneSize: + description: |- + ZoneSize is the size of the shared memory zone used by the upstream. This memory zone is used to share + the upstream configuration between nginx worker processes. The more servers that an upstream has, + the larger memory zone is required. + Default: OSS: 512k, Plus: 1m. + Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#zone + pattern: ^\d{1,4}(k|m|g)?$ + type: string + required: + - targetRefs + type: object + status: + description: Status defines the state of the UpstreamSettingsPolicy. + properties: + ancestors: + description: |- + Ancestors is a list of ancestor resources (usually Gateways) that are + associated with the policy, and the status of the policy with respect to + each ancestor. When this policy attaches to a parent, the controller that + manages the parent and the ancestors MUST add an entry to this list when + the controller first sees the policy and SHOULD update the entry as + appropriate when the relevant ancestor is modified. + + Note that choosing the relevant ancestor is left to the Policy designers; + an important part of Policy design is designing the right object level at + which to namespace this status. + + Note also that implementations MUST ONLY populate ancestor status for + the Ancestor resources they are responsible for. Implementations MUST + use the ControllerName field to uniquely identify the entries in this list + that they are responsible for. + + Note that to achieve this, the list of PolicyAncestorStatus structs + MUST be treated as a map with a composite key, made up of the AncestorRef + and ControllerName fields combined. + + A maximum of 16 ancestors will be represented in this list. An empty list + means the Policy is not relevant for any ancestors. + + If this slice is full, implementations MUST NOT add further entries. + Instead they MUST consider the policy unimplementable and signal that + on any related resources such as the ancestor that would be referenced + here. For example, if this list was full on BackendTLSPolicy, no + additional Gateways would be able to reference the Service targeted by + the BackendTLSPolicy. + items: + description: |- + PolicyAncestorStatus describes the status of a route with respect to an + associated Ancestor. + + Ancestors refer to objects that are either the Target of a policy or above it + in terms of object hierarchy. For example, if a policy targets a Service, the + Policy's Ancestors are, in order, the Service, the HTTPRoute, the Gateway, and + the GatewayClass. Almost always, in this hierarchy, the Gateway will be the most + useful object to place Policy status on, so we recommend that implementations + SHOULD use Gateway as the PolicyAncestorStatus object unless the designers + have a _very_ good reason otherwise. + + In the context of policy attachment, the Ancestor is used to distinguish which + resource results in a distinct application of this policy. For example, if a policy + targets a Service, it may have a distinct result per attached Gateway. + + Policies targeting the same resource may have different effects depending on the + ancestors of those resources. For example, different Gateways targeting the same + Service may have different capabilities, especially if they have different underlying + implementations. + + For example, in BackendTLSPolicy, the Policy attaches to a Service that is + used as a backend in a HTTPRoute that is itself attached to a Gateway. + In this case, the relevant object for status is the Gateway, and that is the + ancestor object referred to in this status. + + Note that a parent is also an ancestor, so for objects where the parent is the + relevant object for status, this struct SHOULD still be used. + + This struct is intended to be used in a slice that's effectively a map, + with a composite key made up of the AncestorRef and the ControllerName. + properties: + ancestorRef: + description: |- + AncestorRef corresponds with a ParentRef in the spec that this + PolicyAncestorStatus struct describes the status of. + properties: + group: + default: gateway.networking.k8s.io + description: |- + Group is the group of the referent. + When unspecified, "gateway.networking.k8s.io" is inferred. + To set the core API group (such as for a "Service" kind referent), + Group must be explicitly set to "" (empty string). + + Support: Core + maxLength: 253 + pattern: ^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + kind: + default: Gateway + description: |- + Kind is kind of the referent. + + There are two kinds of parent resources with "Core" support: + + * Gateway (Gateway conformance profile) + * Service (Mesh conformance profile, ClusterIP Services only) + + Support for other resources is Implementation-Specific. + maxLength: 63 + minLength: 1 + pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$ + type: string + name: + description: |- + Name is the name of the referent. + + Support: Core + maxLength: 253 + minLength: 1 + type: string + namespace: + description: |- + Namespace is the namespace of the referent. When unspecified, this refers + to the local namespace of the Route. + + Note that there are specific rules for ParentRefs which cross namespace + boundaries. Cross-namespace references are only valid if they are explicitly + allowed by something in the namespace they are referring to. For example: + Gateway has the AllowedRoutes field, and ReferenceGrant provides a + generic way to enable any other kind of cross-namespace reference. + + + ParentRefs from a Route to a Service in the same namespace are "producer" + routes, which apply default routing rules to inbound connections from + any namespace to the Service. + + ParentRefs from a Route to a Service in a different namespace are + "consumer" routes, and these routing rules are only applied to outbound + connections originating from the same namespace as the Route, for which + the intended destination of the connections are a Service targeted as a + ParentRef of the Route. + + + Support: Core + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string + port: + description: |- + Port is the network port this Route targets. It can be interpreted + differently based on the type of parent resource. + + When the parent resource is a Gateway, this targets all listeners + listening on the specified port that also support this kind of Route(and + select this Route). It's not recommended to set `Port` unless the + networking behaviors specified in a Route must apply to a specific port + as opposed to a listener(s) whose port(s) may be changed. When both Port + and SectionName are specified, the name and port of the selected listener + must match both specified values. + + + When the parent resource is a Service, this targets a specific port in the + Service spec. When both Port (experimental) and SectionName are specified, + the name and port of the selected port must match both specified values. + + + Implementations MAY choose to support other parent resources. + Implementations supporting other types of parent resources MUST clearly + document how/if Port is interpreted. + + For the purpose of status, an attachment is considered successful as + long as the parent resource accepts it partially. For example, Gateway + listeners can restrict which Routes can attach to them by Route kind, + namespace, or hostname. If 1 of 2 Gateway listeners accept attachment + from the referencing Route, the Route MUST be considered successfully + attached. If no Gateway listeners accept attachment from this Route, + the Route MUST be considered detached from the Gateway. + + Support: Extended + format: int32 + maximum: 65535 + minimum: 1 + type: integer + sectionName: + description: |- + SectionName is the name of a section within the target resource. In the + following resources, SectionName is interpreted as the following: + + * Gateway: Listener name. When both Port (experimental) and SectionName + are specified, the name and port of the selected listener must match + both specified values. + * Service: Port name. When both Port (experimental) and SectionName + are specified, the name and port of the selected listener must match + both specified values. + + Implementations MAY choose to support attaching Routes to other resources. + If that is the case, they MUST clearly document how SectionName is + interpreted. + + When unspecified (empty string), this will reference the entire resource. + For the purpose of status, an attachment is considered successful if at + least one section in the parent resource accepts it. For example, Gateway + listeners can restrict which Routes can attach to them by Route kind, + namespace, or hostname. If 1 of 2 Gateway listeners accept attachment from + the referencing Route, the Route MUST be considered successfully + attached. If no Gateway listeners accept attachment from this Route, the + Route MUST be considered detached from the Gateway. + + Support: Core + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + required: + - name + type: object + conditions: + description: Conditions describes the status of the Policy with + respect to the given Ancestor. + items: + description: Condition contains details for one aspect of + the current state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, + Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + maxItems: 8 + minItems: 1 + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map + controllerName: + description: |- + ControllerName is a domain/path string that indicates the name of the + controller that wrote this status. This corresponds with the + controllerName field on GatewayClass. + + Example: "example.net/gateway-controller". + + The format of this field is DOMAIN "/" PATH, where DOMAIN and PATH are + valid Kubernetes names + (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names). + + Controllers MUST populate this field when writing status. Controllers should ensure that + entries to status populated with their ControllerName are cleaned up when they are no + longer necessary. + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$ + type: string + required: + - ancestorRef + - controllerName + type: object + maxItems: 16 + type: array + required: + - ancestors + type: object + required: + - spec + type: object + served: true + storage: true + subresources: + status: {} diff --git a/deploy/default/deploy.yaml b/deploy/default/deploy.yaml index 7771ea7a3f..69853e77d3 100644 --- a/deploy/default/deploy.yaml +++ b/deploy/default/deploy.yaml @@ -98,6 +98,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -107,6 +108,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/experimental-nginx-plus/deploy.yaml b/deploy/experimental-nginx-plus/deploy.yaml index 6e37bdbba6..9f30307439 100644 --- a/deploy/experimental-nginx-plus/deploy.yaml +++ b/deploy/experimental-nginx-plus/deploy.yaml @@ -111,6 +111,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -120,6 +121,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/experimental/deploy.yaml b/deploy/experimental/deploy.yaml index e6dd9e8f24..f3114d3b55 100644 --- a/deploy/experimental/deploy.yaml +++ b/deploy/experimental/deploy.yaml @@ -103,6 +103,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -112,6 +113,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/nginx-plus/deploy.yaml b/deploy/nginx-plus/deploy.yaml index 65d5e68bee..4561a6edb9 100644 --- a/deploy/nginx-plus/deploy.yaml +++ b/deploy/nginx-plus/deploy.yaml @@ -106,6 +106,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -115,6 +116,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/nodeport/deploy.yaml b/deploy/nodeport/deploy.yaml index 36d5ff08ec..19bae58059 100644 --- a/deploy/nodeport/deploy.yaml +++ b/deploy/nodeport/deploy.yaml @@ -98,6 +98,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -107,6 +108,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/openshift/deploy.yaml b/deploy/openshift/deploy.yaml index d66e286be5..8dd460e35a 100644 --- a/deploy/openshift/deploy.yaml +++ b/deploy/openshift/deploy.yaml @@ -98,6 +98,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies verbs: - list - watch @@ -107,6 +108,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status verbs: - update - apiGroups: diff --git a/deploy/snippets-filters-nginx-plus/deploy.yaml b/deploy/snippets-filters-nginx-plus/deploy.yaml index efc9f843ad..176cf08c0a 100644 --- a/deploy/snippets-filters-nginx-plus/deploy.yaml +++ b/deploy/snippets-filters-nginx-plus/deploy.yaml @@ -106,6 +106,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies - snippetsfilters verbs: - list @@ -116,6 +117,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status - snippetsfilters/status verbs: - update diff --git a/deploy/snippets-filters/deploy.yaml b/deploy/snippets-filters/deploy.yaml index 743b8f7fdf..6c177233bd 100644 --- a/deploy/snippets-filters/deploy.yaml +++ b/deploy/snippets-filters/deploy.yaml @@ -98,6 +98,7 @@ rules: - nginxproxies - clientsettingspolicies - observabilitypolicies + - upstreamsettingspolicies - snippetsfilters verbs: - list @@ -108,6 +109,7 @@ rules: - nginxgateways/status - clientsettingspolicies/status - observabilitypolicies/status + - upstreamsettingspolicies/status - snippetsfilters/status verbs: - update diff --git a/docs/proposals/upstream-settings.md b/docs/proposals/upstream-settings.md index a56b3346f3..7928220c5b 100644 --- a/docs/proposals/upstream-settings.md +++ b/docs/proposals/upstream-settings.md @@ -89,7 +89,7 @@ type UpstreamKeepAlive struct { // Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive // // +optional - Connections *int32 `json"connections,omitempty"` + Connections *int32 `json:"connections,omitempty"` // Requests sets the maximum number of requests that can be served through one keep-alive connection. // After the maximum number of requests are made, the connection is closed. diff --git a/examples/upstream-settings-policy/README.md b/examples/upstream-settings-policy/README.md new file mode 100644 index 0000000000..a6ad17a087 --- /dev/null +++ b/examples/upstream-settings-policy/README.md @@ -0,0 +1,4 @@ +# UpstreamSettingsPolicy + +This directory contains example YAMLs for testing the UpstreamSettingsPolicy CRD. Eventually, this will be converted +into a how-to-guide. diff --git a/examples/upstream-settings-policy/cafe-routes.yaml b/examples/upstream-settings-policy/cafe-routes.yaml new file mode 100644 index 0000000000..67927335cb --- /dev/null +++ b/examples/upstream-settings-policy/cafe-routes.yaml @@ -0,0 +1,37 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: coffee +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: PathPrefix + value: /coffee + backendRefs: + - name: coffee + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: tea +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: Exact + value: /tea + backendRefs: + - name: tea + port: 80 diff --git a/examples/upstream-settings-policy/cafe.yaml b/examples/upstream-settings-policy/cafe.yaml new file mode 100644 index 0000000000..2d03ae59ff --- /dev/null +++ b/examples/upstream-settings-policy/cafe.yaml @@ -0,0 +1,65 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: coffee +spec: + replicas: 1 + selector: + matchLabels: + app: coffee + template: + metadata: + labels: + app: coffee + spec: + containers: + - name: coffee + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: coffee +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: coffee +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tea +spec: + replicas: 1 + selector: + matchLabels: + app: tea + template: + metadata: + labels: + app: tea + spec: + containers: + - name: tea + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: tea +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: tea diff --git a/examples/upstream-settings-policy/gateway.yaml b/examples/upstream-settings-policy/gateway.yaml new file mode 100644 index 0000000000..e6507f613b --- /dev/null +++ b/examples/upstream-settings-policy/gateway.yaml @@ -0,0 +1,11 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway +spec: + gatewayClassName: nginx + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: "*.example.com" diff --git a/examples/upstream-settings-policy/upstream-settings-policy.yaml b/examples/upstream-settings-policy/upstream-settings-policy.yaml new file mode 100644 index 0000000000..95e8a34e6d --- /dev/null +++ b/examples/upstream-settings-policy/upstream-settings-policy.yaml @@ -0,0 +1,15 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: upstream-settings-policy +spec: + zoneSize: 512k + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + connections: 32 + requests: 1001 + time: 300s + timeout: 60s diff --git a/internal/framework/kinds/kinds.go b/internal/framework/kinds/kinds.go index 7700ad39ce..baeda6f9ee 100644 --- a/internal/framework/kinds/kinds.go +++ b/internal/framework/kinds/kinds.go @@ -11,9 +11,9 @@ import ( // Gateway API Kinds. const ( - // Gateway is the Gateway Kind. + // Gateway is the Gateway kind. Gateway = "Gateway" - // GatewayClass is the GatewayClass Kind. + // GatewayClass is the GatewayClass kind. GatewayClass = "GatewayClass" // HTTPRoute is the HTTPRoute kind. HTTPRoute = "HTTPRoute" @@ -23,6 +23,12 @@ const ( TLSRoute = "TLSRoute" ) +// Core API Kinds. +const ( + // Service is the Service kind. + Service = "Service" +) + // NGINX Gateway Fabric kinds. const ( // ClientSettingsPolicy is the ClientSettingsPolicy kind. @@ -33,6 +39,8 @@ const ( NginxProxy = "NginxProxy" // SnippetsFilter is the SnippetsFilter kind. SnippetsFilter = "SnippetsFilter" + // UpstreamSettingsPolicy is the UpstreamSettingsPolicy kind. + UpstreamSettingsPolicy = "UpstreamSettingsPolicy" ) // MustExtractGVK is a function that extracts the GroupVersionKind (GVK) of a client.object. diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index bc94e61346..afb0560179 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -53,6 +53,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" ngxvalidation "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" ngxruntime "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime" @@ -320,6 +321,10 @@ func createPolicyManager( GVK: mustExtractGVK(&ngfAPI.ObservabilityPolicy{}), Validator: observability.NewValidator(validator), }, + { + GVK: mustExtractGVK(&ngfAPI.UpstreamSettingsPolicy{}), + Validator: upstreamsettings.NewValidator(validator), + }, } return policies.NewManager(mustExtractGVK, cfgs...) @@ -501,6 +506,12 @@ func registerControllers( controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), }, }, + { + objectType: &ngfAPI.UpstreamSettingsPolicy{}, + options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + }, + }, } if cfg.ExperimentalFeatures { @@ -737,6 +748,7 @@ func prepareFirstEventBatchPreparerArgs(cfg config.Config) ([]client.Object, []c &gatewayv1.GRPCRouteList{}, &ngfAPI.ClientSettingsPolicyList{}, &ngfAPI.ObservabilityPolicyList{}, + &ngfAPI.UpstreamSettingsPolicyList{}, partialObjectMetadataList, } diff --git a/internal/mode/static/manager_test.go b/internal/mode/static/manager_test.go index 5a61a75854..0f2d802d32 100644 --- a/internal/mode/static/manager_test.go +++ b/internal/mode/static/manager_test.go @@ -67,6 +67,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { partialObjectMetadataList, &ngfAPI.ClientSettingsPolicyList{}, &ngfAPI.ObservabilityPolicyList{}, + &ngfAPI.UpstreamSettingsPolicyList{}, }, }, { @@ -96,6 +97,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { partialObjectMetadataList, &ngfAPI.ClientSettingsPolicyList{}, &ngfAPI.ObservabilityPolicyList{}, + &ngfAPI.UpstreamSettingsPolicyList{}, }, }, { @@ -128,6 +130,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &gatewayv1.GRPCRouteList{}, &ngfAPI.ClientSettingsPolicyList{}, &ngfAPI.ObservabilityPolicyList{}, + &ngfAPI.UpstreamSettingsPolicyList{}, }, }, { @@ -158,6 +161,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &ngfAPI.ClientSettingsPolicyList{}, &ngfAPI.ObservabilityPolicyList{}, &ngfAPI.SnippetsFilterList{}, + &ngfAPI.UpstreamSettingsPolicyList{}, }, }, { @@ -191,6 +195,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &ngfAPI.ClientSettingsPolicyList{}, &ngfAPI.ObservabilityPolicyList{}, &ngfAPI.SnippetsFilterList{}, + &ngfAPI.UpstreamSettingsPolicyList{}, }, }, } diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 714ade9dd3..a408cffd6a 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -8,9 +8,11 @@ import ( "github.com/go-logr/logr" ngfConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) @@ -152,7 +154,10 @@ func (g GeneratorImpl) executeConfigTemplates( ) []file.File { fileBytes := make(map[string][]byte) - for _, execute := range g.getExecuteFuncs(generator) { + httpUpstreams := g.createUpstreams(conf.Upstreams, upstreamsettings.NewProcessor()) + keepAliveCheck := newKeepAliveChecker(httpUpstreams) + + for _, execute := range g.getExecuteFuncs(generator, httpUpstreams, keepAliveCheck) { results := execute(conf) for _, res := range results { fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) @@ -177,12 +182,16 @@ func (g GeneratorImpl) executeConfigTemplates( return files } -func (g GeneratorImpl) getExecuteFuncs(generator policies.Generator) []executeFunc { +func (g GeneratorImpl) getExecuteFuncs( + generator policies.Generator, + upstreams []http.Upstream, + keepAliveCheck keepAliveChecker, +) []executeFunc { return []executeFunc{ executeMainConfig, executeBaseHTTPConfig, - g.newExecuteServersFunc(generator), - g.executeUpstreams, + g.newExecuteServersFunc(generator, keepAliveCheck), + newExecuteUpstreamsFunc(upstreams), executeSplitClients, executeMaps, executeTelemetry, diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 6d063dc8a7..6a8e77ff4d 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -1,6 +1,8 @@ package http -import "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" +import ( + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/shared" +) const ( InternalRoutePathPrefix = "/_ngf-internal" @@ -85,9 +87,18 @@ type Upstream struct { Name string ZoneSize string // format: 512k, 1m StateFile string + KeepAlive UpstreamKeepAlive Servers []UpstreamServer } +// UpstreamKeepAlive holds the keepalive configuration for an HTTP upstream. +type UpstreamKeepAlive struct { + Time string + Timeout string + Connections int32 + Requests int32 +} + // UpstreamServer holds all configuration for an HTTP upstream server. type UpstreamServer struct { Address string diff --git a/internal/mode/static/nginx/config/policies/clientsettings/validator.go b/internal/mode/static/nginx/config/policies/clientsettings/validator.go index 60ce55c7a9..79a390ce9b 100644 --- a/internal/mode/static/nginx/config/policies/clientsettings/validator.go +++ b/internal/mode/static/nginx/config/policies/clientsettings/validator.go @@ -30,7 +30,9 @@ func (v *Validator) Validate(policy policies.Policy, _ *policies.GlobalSettings) targetRefPath := field.NewPath("spec").Child("targetRef") supportedKinds := []gatewayv1.Kind{kinds.Gateway, kinds.HTTPRoute, kinds.GRPCRoute} - if err := policies.ValidateTargetRef(csp.Spec.TargetRef, targetRefPath, supportedKinds); err != nil { + supportedGroups := []gatewayv1.Group{gatewayv1.GroupName} + + if err := policies.ValidateTargetRef(csp.Spec.TargetRef, targetRefPath, supportedGroups, supportedKinds); err != nil { return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} } diff --git a/internal/mode/static/nginx/config/policies/observability/validator.go b/internal/mode/static/nginx/config/policies/observability/validator.go index 3fa7ea2289..b93902d958 100644 --- a/internal/mode/static/nginx/config/policies/observability/validator.go +++ b/internal/mode/static/nginx/config/policies/observability/validator.go @@ -45,8 +45,10 @@ func (v *Validator) Validate( targetRefPath := field.NewPath("spec").Child("targetRefs") supportedKinds := []gatewayv1.Kind{kinds.HTTPRoute, kinds.GRPCRoute} + supportedGroups := []gatewayv1.Group{gatewayv1.GroupName} + for _, ref := range obs.Spec.TargetRefs { - if err := policies.ValidateTargetRef(ref, targetRefPath, supportedKinds); err != nil { + if err := policies.ValidateTargetRef(ref, targetRefPath, supportedGroups, supportedKinds); err != nil { return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} } } diff --git a/internal/mode/static/nginx/config/policies/policy.go b/internal/mode/static/nginx/config/policies/policy.go index c26f3bec7b..b818f5a216 100644 --- a/internal/mode/static/nginx/config/policies/policy.go +++ b/internal/mode/static/nginx/config/policies/policy.go @@ -24,9 +24,9 @@ type Policy interface { // GlobalSettings contains global settings from the current state of the graph that may be // needed for policy validation or generation if certain policies rely on those global settings. type GlobalSettings struct { - // NginxProxyValid is whether or not the NginxProxy resource is valid. + // NginxProxyValid is whether the NginxProxy resource is valid. NginxProxyValid bool - // TelemetryEnabled is whether or not telemetry is enabled in the NginxProxy resource. + // TelemetryEnabled is whether telemetry is enabled in the NginxProxy resource. TelemetryEnabled bool } @@ -34,15 +34,16 @@ type GlobalSettings struct { func ValidateTargetRef( ref v1alpha2.LocalPolicyTargetReference, basePath *field.Path, + groups []gatewayv1.Group, supportedKinds []gatewayv1.Kind, ) error { - if ref.Group != gatewayv1.GroupName { + if !slices.Contains(groups, ref.Group) { path := basePath.Child("group") return field.NotSupported( path, ref.Group, - []string{gatewayv1.GroupName}, + groups, ) } diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go new file mode 100644 index 0000000000..5df29eed64 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor.go @@ -0,0 +1,67 @@ +package upstreamsettings + +import ( + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" +) + +// Processor processes UpstreamSettingsPolicies. +type Processor struct{} + +// UpstreamSettings contains settings from UpstreamSettingsPolicy. +type UpstreamSettings struct { + // ZoneSize is the zone size setting. + ZoneSize string + // KeepAlive contains the keepalive settings. + KeepAlive http.UpstreamKeepAlive +} + +// NewProcessor returns a new Processor. +func NewProcessor() Processor { + return Processor{} +} + +// Process processes policies into an UpstreamSettings object. The policies are already validated and are guaranteed +// to not contain overlapping settings. This method merges all fields in the policies into a single UpstreamSettings +// object. +func (g Processor) Process(pols []policies.Policy) UpstreamSettings { + return processPolicies(pols) +} + +func processPolicies(pols []policies.Policy) UpstreamSettings { + upstreamSettings := UpstreamSettings{} + + for _, pol := range pols { + usp, ok := pol.(*ngfAPI.UpstreamSettingsPolicy) + if !ok { + continue + } + + // we can assume that there will be no instance of two or more policies setting the same + // field for the same service + if usp.Spec.ZoneSize != nil { + upstreamSettings.ZoneSize = string(*usp.Spec.ZoneSize) + } + + if usp.Spec.KeepAlive != nil { + if usp.Spec.KeepAlive.Connections != nil { + upstreamSettings.KeepAlive.Connections = *usp.Spec.KeepAlive.Connections + } + + if usp.Spec.KeepAlive.Requests != nil { + upstreamSettings.KeepAlive.Requests = *usp.Spec.KeepAlive.Requests + } + + if usp.Spec.KeepAlive.Time != nil { + upstreamSettings.KeepAlive.Time = string(*usp.Spec.KeepAlive.Time) + } + + if usp.Spec.KeepAlive.Timeout != nil { + upstreamSettings.KeepAlive.Timeout = string(*usp.Spec.KeepAlive.Timeout) + } + } + } + + return upstreamSettings +} diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go new file mode 100644 index 0000000000..b7c785376f --- /dev/null +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/processor_test.go @@ -0,0 +1,334 @@ +package upstreamsettings + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" +) + +func TestProcess(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + expUpstreamSettings UpstreamSettings + policies []policies.Policy + }{ + { + name: "all fields populated", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + ZoneSize: "2m", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + { + name: "zone size set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + ZoneSize: "2m", + }, + }, + { + name: "keep alive connections set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + }), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, + }, + }, + { + name: "keep alive requests set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ + Requests: 1, + }, + }, + }, + { + name: "keep alive time set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + }), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ + Time: "5s", + }, + }, + }, + { + name: "keep alive timeout set", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + KeepAlive: http.UpstreamKeepAlive{ + Timeout: "10s", + }, + }, + }, + { + name: "no fields populated", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{}, + }, + }, + expUpstreamSettings: UpstreamSettings{}, + }, + { + name: "multiple UpstreamSettingsPolicies", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-zonesize", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-connections", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-requests", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-time", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-timeout", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + ZoneSize: "2m", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + { + name: "multiple UpstreamSettingsPolicies along with other policies", + policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-zonesize", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-connections", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-requests", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + &ngfAPI.ClientSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "client-settings-policy", + Namespace: "test", + }, + Spec: ngfAPI.ClientSettingsPolicySpec{ + Body: &ngfAPI.ClientBody{ + MaxSize: helpers.GetPointer[ngfAPI.Size]("1m"), + }, + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-time", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp-keepalive-timeout", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "observability-policy", + Namespace: "test", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Ratio: helpers.GetPointer(int32(1)), + }, + }, + }, + }, + expUpstreamSettings: UpstreamSettings{ + ZoneSize: "2m", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + processor := NewProcessor() + + g.Expect(processor.Process(test.policies)).To(Equal(test.expUpstreamSettings)) + }) + } +} diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/validator.go b/internal/mode/static/nginx/config/policies/upstreamsettings/validator.go new file mode 100644 index 0000000000..9fccebd166 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/validator.go @@ -0,0 +1,125 @@ +package upstreamsettings + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + 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/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" +) + +// Validator validates an UpstreamSettingsPolicy. +// Implements policies.Validator interface. +type Validator struct { + genericValidator validation.GenericValidator +} + +// NewValidator returns a new Validator. +func NewValidator(genericValidator validation.GenericValidator) Validator { + return Validator{genericValidator: genericValidator} +} + +// Validate validates the spec of an UpstreamsSettingsPolicy. +func (v Validator) Validate(policy policies.Policy, _ *policies.GlobalSettings) []conditions.Condition { + usp := helpers.MustCastObject[*ngfAPI.UpstreamSettingsPolicy](policy) + + targetRefsPath := field.NewPath("spec").Child("targetRefs") + supportedKinds := []gatewayv1.Kind{kinds.Service} + supportedGroups := []gatewayv1.Group{"", "core"} + + for i, ref := range usp.Spec.TargetRefs { + indexedPath := targetRefsPath.Index(i) + if err := policies.ValidateTargetRef(ref, indexedPath, supportedGroups, supportedKinds); err != nil { + return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} + } + } + + if err := v.validateSettings(usp.Spec); err != nil { + return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} + } + + return nil +} + +// Conflicts returns true if the two UpstreamsSettingsPolicies conflict. +func (v Validator) Conflicts(polA, polB policies.Policy) bool { + cspA := helpers.MustCastObject[*ngfAPI.UpstreamSettingsPolicy](polA) + cspB := helpers.MustCastObject[*ngfAPI.UpstreamSettingsPolicy](polB) + + return conflicts(cspA.Spec, cspB.Spec) +} + +func conflicts(a, b ngfAPI.UpstreamSettingsPolicySpec) bool { + if a.ZoneSize != nil && b.ZoneSize != nil { + return true + } + + if a.KeepAlive != nil && b.KeepAlive != nil { + if a.KeepAlive.Connections != nil && b.KeepAlive.Connections != nil { + return true + } + if a.KeepAlive.Requests != nil && b.KeepAlive.Requests != nil { + return true + } + + if a.KeepAlive.Time != nil && b.KeepAlive.Time != nil { + return true + } + + if a.KeepAlive.Timeout != nil && b.KeepAlive.Timeout != nil { + return true + } + } + + return false +} + +// validateSettings performs validation on fields in the spec that are vulnerable to code injection. +// For all other fields, we rely on the CRD validation. +func (v Validator) validateSettings(spec ngfAPI.UpstreamSettingsPolicySpec) error { + var allErrs field.ErrorList + fieldPath := field.NewPath("spec") + + if spec.ZoneSize != nil { + if err := v.genericValidator.ValidateNginxSize(string(*spec.ZoneSize)); err != nil { + path := fieldPath.Child("zoneSize") + allErrs = append(allErrs, field.Invalid(path, spec.ZoneSize, err.Error())) + } + } + + if spec.KeepAlive != nil { + allErrs = append(allErrs, v.validateUpstreamKeepAlive(*spec.KeepAlive, fieldPath.Child("keepAlive"))...) + } + + return allErrs.ToAggregate() +} + +func (v Validator) validateUpstreamKeepAlive( + keepAlive ngfAPI.UpstreamKeepAlive, + fieldPath *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + if keepAlive.Time != nil { + if err := v.genericValidator.ValidateNginxDuration(string(*keepAlive.Time)); err != nil { + path := fieldPath.Child("time") + + allErrs = append(allErrs, field.Invalid(path, *keepAlive.Time, err.Error())) + } + } + + if keepAlive.Timeout != nil { + if err := v.genericValidator.ValidateNginxDuration(string(*keepAlive.Timeout)); err != nil { + path := fieldPath.Child("timeout") + + allErrs = append(allErrs, field.Invalid(path, *keepAlive.Timeout, err.Error())) + } + } + + return allErrs +} diff --git a/internal/mode/static/nginx/config/policies/upstreamsettings/validator_test.go b/internal/mode/static/nginx/config/policies/upstreamsettings/validator_test.go new file mode 100644 index 0000000000..3e303a8229 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/upstreamsettings/validator_test.go @@ -0,0 +1,266 @@ +package upstreamsettings_test + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + 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/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation" + staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" +) + +type policyModFunc func(policy *ngfAPI.UpstreamSettingsPolicy) *ngfAPI.UpstreamSettingsPolicy + +func createValidPolicy() *ngfAPI.UpstreamSettingsPolicy { + return &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + TargetRefs: []v1alpha2.LocalPolicyTargetReference{ + { + Group: "core", + Kind: kinds.Service, + Name: "svc", + }, + }, + ZoneSize: helpers.GetPointer[ngfAPI.Size]("1k"), + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer[int32](900), + Time: helpers.GetPointer[ngfAPI.Duration]("50s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("30s"), + Connections: helpers.GetPointer[int32](100), + }, + }, + Status: v1alpha2.PolicyStatus{}, + } +} + +func createModifiedPolicy(mod policyModFunc) *ngfAPI.UpstreamSettingsPolicy { + return mod(createValidPolicy()) +} + +func TestValidator_Validate(t *testing.T) { + t.Parallel() + tests := []struct { + name string + policy *ngfAPI.UpstreamSettingsPolicy + expConditions []conditions.Condition + }{ + { + name: "invalid target ref; unsupported group", + policy: createModifiedPolicy(func(p *ngfAPI.UpstreamSettingsPolicy) *ngfAPI.UpstreamSettingsPolicy { + p.Spec.TargetRefs = append( + p.Spec.TargetRefs, + v1alpha2.LocalPolicyTargetReference{ + Group: "Unsupported", + Kind: kinds.Service, + Name: "svc", + }) + return p + }), + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.targetRefs[1].group: Unsupported value: \"Unsupported\": " + + "supported values: \"\", \"core\""), + }, + }, + { + name: "invalid target ref; unsupported kind", + policy: createModifiedPolicy(func(p *ngfAPI.UpstreamSettingsPolicy) *ngfAPI.UpstreamSettingsPolicy { + p.Spec.TargetRefs = append( + p.Spec.TargetRefs, + v1alpha2.LocalPolicyTargetReference{ + Group: "", + Kind: "Unsupported", + Name: "svc", + }) + return p + }), + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.targetRefs[1].kind: Unsupported value: \"Unsupported\": " + + "supported values: \"Service\""), + }, + }, + { + name: "invalid zone size", + policy: createModifiedPolicy(func(p *ngfAPI.UpstreamSettingsPolicy) *ngfAPI.UpstreamSettingsPolicy { + p.Spec.ZoneSize = helpers.GetPointer[ngfAPI.Size]("invalid") + return p + }), + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.zoneSize: Invalid value: \"invalid\": ^\\d{1,4}(k|m|g)?$ " + + "(e.g. '1024', or '8k', or '20m', or '1g', regex used for validation is 'must contain a number. " + + "May be followed by 'k', 'm', or 'g', otherwise bytes are assumed')"), + }, + }, + { + name: "invalid durations", + policy: createModifiedPolicy(func(p *ngfAPI.UpstreamSettingsPolicy) *ngfAPI.UpstreamSettingsPolicy { + p.Spec.KeepAlive.Time = helpers.GetPointer[ngfAPI.Duration]("invalid") + p.Spec.KeepAlive.Timeout = helpers.GetPointer[ngfAPI.Duration]("invalid") + return p + }), + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid( + "[spec.keepAlive.time: Invalid value: \"invalid\": ^[0-9]{1,4}(ms|s|m|h)? " + + "(e.g. '5ms', or '10s', or '500m', or '1000h', regex used for validation is " + + "'must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h''), " + + "spec.keepAlive.timeout: Invalid value: \"invalid\": ^[0-9]{1,4}(ms|s|m|h)? " + + "(e.g. '5ms', or '10s', or '500m', or '1000h', regex used for validation is " + + "'must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h'')]"), + }, + }, + { + name: "valid", + policy: createValidPolicy(), + expConditions: nil, + }, + } + + v := upstreamsettings.NewValidator(validation.GenericValidator{}) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + conds := v.Validate(test.policy, nil) + g.Expect(conds).To(Equal(test.expConditions)) + }) + } +} + +func TestValidator_ValidatePanics(t *testing.T) { + t.Parallel() + v := upstreamsettings.NewValidator(nil) + + validate := func() { + _ = v.Validate(&policiesfakes.FakePolicy{}, nil) + } + + g := NewWithT(t) + + g.Expect(validate).To(Panic()) +} + +func TestValidator_Conflicts(t *testing.T) { + t.Parallel() + tests := []struct { + polA *ngfAPI.UpstreamSettingsPolicy + polB *ngfAPI.UpstreamSettingsPolicy + name string + conflicts bool + }{ + { + name: "no conflicts", + polA: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("10m"), + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer[int32](900), + Time: helpers.GetPointer[ngfAPI.Duration]("50s"), + }, + }, + }, + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("30s"), + Connections: helpers.GetPointer[int32](50), + }, + }, + }, + conflicts: false, + }, + { + name: "zone max size conflicts", + polA: createValidPolicy(), + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("10m"), + }, + }, + conflicts: true, + }, + { + name: "keepalive requests conflicts", + polA: createValidPolicy(), + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Requests: helpers.GetPointer[int32](900), + }, + }, + }, + conflicts: true, + }, + { + name: "keepalive connections conflicts", + polA: createValidPolicy(), + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer[int32](900), + }, + }, + }, + conflicts: true, + }, + { + name: "keepalive time conflicts", + polA: createValidPolicy(), + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("50s"), + }, + }, + }, + conflicts: true, + }, + { + name: "keepalive timeout conflicts", + polA: createValidPolicy(), + polB: &ngfAPI.UpstreamSettingsPolicy{ + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: &ngfAPI.UpstreamKeepAlive{ + Timeout: helpers.GetPointer[ngfAPI.Duration]("30s"), + }, + }, + }, + conflicts: true, + }, + } + + v := upstreamsettings.NewValidator(nil) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(v.Conflicts(test.polA, test.polB)).To(Equal(test.conflicts)) + }) + } +} + +func TestValidator_ConflictsPanics(t *testing.T) { + t.Parallel() + v := upstreamsettings.NewValidator(nil) + + conflicts := func() { + _ = v.Conflicts(&policiesfakes.FakePolicy{}, &policiesfakes.FakePolicy{}) + } + + g := NewWithT(t) + + g.Expect(conflicts).To(Panic()) +} diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 33ea858f31..e7ccef05fc 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -23,82 +23,41 @@ const ( rootPath = "/" ) -// httpBaseHeaders contains the constant headers set in each HTTP server block. -var httpBaseHeaders = []http.Header{ - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, -} - -// grpcBaseHeaders contains the constant headers set in each gRPC server block. -var grpcBaseHeaders = []http.Header{ - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Authority", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, -} - -func (g GeneratorImpl) newExecuteServersFunc(generator policies.Generator) executeFunc { +var grpcAuthorityHeader = http.Header{ + Name: "Authority", + Value: "$gw_api_compliant_host", +} + +var httpConnectionHeader = http.Header{ + Name: "Connection", + Value: "$connection_upgrade", +} + +var unsetHTTPConnectionHeader = http.Header{ + Name: "Connection", + Value: "", +} + +var httpUpgradeHeader = http.Header{ + Name: "Upgrade", + Value: "$http_upgrade", +} + +func (g GeneratorImpl) newExecuteServersFunc( + generator policies.Generator, + keepAliveCheck keepAliveChecker, +) executeFunc { return func(configuration dataplane.Configuration) []executeResult { - return g.executeServers(configuration, generator) + return g.executeServers(configuration, generator, keepAliveCheck) } } -func (g GeneratorImpl) executeServers(conf dataplane.Configuration, generator policies.Generator) []executeResult { - servers, httpMatchPairs := createServers(conf, generator) +func (g GeneratorImpl) executeServers( + conf dataplane.Configuration, + generator policies.Generator, + keepAliveCheck keepAliveChecker, +) []executeResult { + servers, httpMatchPairs := createServers(conf, generator, keepAliveCheck) serverConfig := http.ServerConfig{ Servers: servers, @@ -145,7 +104,11 @@ func getIPFamily(baseHTTPConfig dataplane.BaseHTTPConfig) shared.IPFamily { return shared.IPFamily{IPv4: true, IPv6: true} } -func createServers(conf dataplane.Configuration, generator policies.Generator) ([]http.Server, httpMatchPairs) { +func createServers( + conf dataplane.Configuration, + generator policies.Generator, + keepAliveCheck keepAliveChecker, +) ([]http.Server, httpMatchPairs) { servers := make([]http.Server, 0, len(conf.HTTPServers)+len(conf.SSLServers)) finalMatchPairs := make(httpMatchPairs) sharedTLSPorts := make(map[int32]struct{}) @@ -156,7 +119,7 @@ func createServers(conf dataplane.Configuration, generator policies.Generator) ( for idx, s := range conf.HTTPServers { serverID := fmt.Sprintf("%d", idx) - httpServer, matchPairs := createServer(s, serverID, generator) + httpServer, matchPairs := createServer(s, serverID, generator, keepAliveCheck) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPairs) } @@ -164,7 +127,7 @@ func createServers(conf dataplane.Configuration, generator policies.Generator) ( for idx, s := range conf.SSLServers { serverID := fmt.Sprintf("SSL_%d", idx) - sslServer, matchPairs := createSSLServer(s, serverID, generator) + sslServer, matchPairs := createSSLServer(s, serverID, generator, keepAliveCheck) if _, portInUse := sharedTLSPorts[s.Port]; portInUse { sslServer.Listen = getSocketNameHTTPS(s.Port) sslServer.IsSocket = true @@ -180,6 +143,7 @@ func createSSLServer( virtualServer dataplane.VirtualServer, serverID string, generator policies.Generator, + keepAliveCheck keepAliveChecker, ) (http.Server, httpMatchPairs) { listen := fmt.Sprint(virtualServer.Port) if virtualServer.IsDefault { @@ -189,7 +153,7 @@ func createSSLServer( }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, keepAliveCheck) server := http.Server{ ServerName: virtualServer.Hostname, @@ -218,6 +182,7 @@ func createServer( virtualServer dataplane.VirtualServer, serverID string, generator policies.Generator, + keepAliveCheck keepAliveChecker, ) (http.Server, httpMatchPairs) { listen := fmt.Sprint(virtualServer.Port) @@ -228,7 +193,7 @@ func createServer( }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator, keepAliveCheck) server := http.Server{ ServerName: virtualServer.Hostname, @@ -264,6 +229,7 @@ func createLocations( server *dataplane.VirtualServer, serverID string, generator policies.Generator, + keepAliveCheck keepAliveChecker, ) ([]http.Location, httpMatchPairs, bool) { maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules) locs := make([]http.Location, 0, maxLocs) @@ -292,7 +258,15 @@ func createLocations( if !needsInternalLocations(rule) { for _, r := range rule.MatchRules { - extLocations = updateLocations(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) + extLocations = updateLocations( + r.Filters, + extLocations, + r, + server.Port, + rule.Path, + rule.GRPC, + keepAliveCheck, + ) } locs = append(locs, extLocations...) @@ -314,6 +288,7 @@ func createLocations( server.Port, rule.Path, rule.GRPC, + keepAliveCheck, ) internalLocations = append(internalLocations, intLocation) @@ -450,6 +425,7 @@ func updateLocation( listenerPort int32, path string, grpc bool, + keepAliveCheck keepAliveChecker, ) http.Location { if filters.InvalidFilter != nil { location.Return = &http.Return{Code: http.StatusInternalServerError} @@ -465,7 +441,16 @@ func updateLocation( } rewrites := createRewritesValForRewriteFilter(filters.RequestURLRewrite, path) - proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, grpc) + + extraHeaders := make([]http.Header, 0, 3) + if grpc { + extraHeaders = append(extraHeaders, grpcAuthorityHeader) + } else { + extraHeaders = append(extraHeaders, httpUpgradeHeader) + extraHeaders = append(extraHeaders, getConnectionHeader(keepAliveCheck, matchRule.BackendGroup.Backends)) + } + + proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, createBaseProxySetHeaders(extraHeaders...)) responseHeaders := generateResponseHeaders(&matchRule.Filters) if rewrites != nil { @@ -502,11 +487,12 @@ func updateLocations( listenerPort int32, path string, grpc bool, + keepAliveCheck keepAliveChecker, ) []http.Location { updatedLocations := make([]http.Location, len(buildLocations)) for i, loc := range buildLocations { - updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc) + updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc, keepAliveCheck) } return updatedLocations @@ -760,32 +746,26 @@ func createMatchLocation(path string, grpc bool) http.Location { return loc } -func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.Header { - var headers []http.Header - if !grpc { - headers = make([]http.Header, len(httpBaseHeaders)) - copy(headers, httpBaseHeaders) - } else { - headers = make([]http.Header, len(grpcBaseHeaders)) - copy(headers, grpcBaseHeaders) - } - +func generateProxySetHeaders( + filters *dataplane.HTTPFilters, + baseHeaders []http.Header, +) []http.Header { if filters != nil && filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil { - for i, header := range headers { + for i, header := range baseHeaders { if header.Name == "Host" { - headers[i].Value = *filters.RequestURLRewrite.Hostname + baseHeaders[i].Value = *filters.RequestURLRewrite.Hostname break } } } if filters == nil || filters.RequestHeaderModifiers == nil { - return headers + return baseHeaders } headerFilter := filters.RequestHeaderModifiers - headerLen := len(headerFilter.Add) + len(headerFilter.Set) + len(headerFilter.Remove) + len(headers) + headerLen := len(headerFilter.Add) + len(headerFilter.Set) + len(headerFilter.Remove) + len(baseHeaders) proxySetHeaders := make([]http.Header, 0, headerLen) if len(headerFilter.Add) > 0 { addHeaders := createHeadersWithVarName(headerFilter.Add) @@ -803,7 +783,7 @@ func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.H }) } - return append(proxySetHeaders, headers...) + return append(proxySetHeaders, baseHeaders...) } func generateResponseHeaders(filters *dataplane.HTTPFilters) http.ResponseHeaders { @@ -887,3 +867,48 @@ func getRewriteClientIPSettings(rewriteIPConfig dataplane.RewriteClientIPSetting ProxyProtocol: proxyProtocol, } } + +func createBaseProxySetHeaders(extraHeaders ...http.Header) []http.Header { + baseHeaders := []http.Header{ + { + Name: "Host", + Value: "$gw_api_compliant_host", + }, + { + Name: "X-Forwarded-For", + Value: "$proxy_add_x_forwarded_for", + }, + { + Name: "X-Real-IP", + Value: "$remote_addr", + }, + { + Name: "X-Forwarded-Proto", + Value: "$scheme", + }, + { + Name: "X-Forwarded-Host", + Value: "$host", + }, + { + Name: "X-Forwarded-Port", + Value: "$server_port", + }, + } + + baseHeaders = append(baseHeaders, extraHeaders...) + + return baseHeaders +} + +func getConnectionHeader(keepAliveCheck keepAliveChecker, backends []dataplane.Backend) http.Header { + for _, backend := range backends { + if keepAliveCheck(backend.UpstreamName) { + // if keep-alive settings are enabled on any upstream, the connection header value + // must be empty for the location + return unsetHTTPConnectionHeader + } + } + + return httpConnectionHeader +} diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 112991b521..24b40d0f60 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -17,6 +17,12 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) +var ( + httpBaseHeaders = createBaseProxySetHeaders(httpUpgradeHeader, httpConnectionHeader) + grpcBaseHeaders = createBaseProxySetHeaders(grpcAuthorityHeader) + alwaysFalseKeepAliveChecker = func(_ string) bool { return false } +) + func TestExecuteServers(t *testing.T) { t.Parallel() @@ -182,7 +188,7 @@ func TestExecuteServers(t *testing.T) { ) gen := GeneratorImpl{} - results := gen.executeServers(conf, fakeGenerator) + results := gen.executeServers(conf, fakeGenerator, alwaysFalseKeepAliveChecker) g.Expect(results).To(HaveLen(len(expectedResults))) for _, res := range results { @@ -321,7 +327,8 @@ func TestExecuteServers_IPFamily(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}) + results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) + g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -439,7 +446,7 @@ func TestExecuteServers_RewriteClientIP(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}) + results := gen.executeServers(test.config, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -481,7 +488,7 @@ func TestExecuteServers_Plus(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{plus: true} - results := gen.executeServers(config, &policiesfakes.FakeGenerator{}) + results := gen.executeServers(config, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) @@ -565,7 +572,7 @@ func TestExecuteForDefaultServers(t *testing.T) { g := NewWithT(t) gen := GeneratorImpl{} - serverResults := gen.executeServers(tc.conf, &policiesfakes.FakeGenerator{}) + serverResults := gen.executeServers(tc.conf, &policiesfakes.FakeGenerator{}, alwaysFalseKeepAliveChecker) g.Expect(serverResults).To(HaveLen(2)) serverConf := string(serverResults[0].data) httpMatchConf := string(serverResults[1].data) @@ -649,6 +656,18 @@ func TestCreateServers(t *testing.T) { }, } + keepAliveGroup := dataplane.BackendGroup{ + Source: hrNsName, + RuleIdx: 4, + Backends: []dataplane.Backend{ + { + UpstreamName: "test_keep_alive_80", + Valid: true, + Weight: 1, + }, + }, + } + filterGroup1 := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 3} filterGroup2 := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 4} @@ -969,6 +988,16 @@ func TestCreateServers(t *testing.T) { }, }, }, + { + Path: "/keep-alive-enabled", + PathType: dataplane.PathTypeExact, + MatchRules: []dataplane.MatchRule{ + { + Match: dataplane.Match{}, + BackendGroup: keepAliveGroup, + }, + }, + }, } conf := dataplane.Configuration{ @@ -1072,14 +1101,6 @@ func TestCreateServers(t *testing.T) { Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for", }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, { Name: "X-Real-IP", Value: "$remote_addr", @@ -1096,6 +1117,14 @@ func TestCreateServers(t *testing.T) { Name: "X-Forwarded-Port", Value: "$server_port", }, + { + Name: "Upgrade", + Value: "$http_upgrade", + }, + { + Name: "Connection", + Value: "$connection_upgrade", + }, } externalIncludes := []shared.Include{ @@ -1343,44 +1372,12 @@ func TestCreateServers(t *testing.T) { { Path: "/proxy-set-headers/", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: []http.Header{ + ProxySetHeaders: append([]http.Header{ { Name: "my-header", Value: "${my_header_header_var}some-value-123", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, - }, + }, httpBaseHeaders...), ResponseHeaders: http.ResponseHeaders{ Add: []http.Header{ { @@ -1397,44 +1394,12 @@ func TestCreateServers(t *testing.T) { { Path: "= /proxy-set-headers", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: []http.Header{ + ProxySetHeaders: append([]http.Header{ { Name: "my-header", Value: "${my_header_header_var}some-value-123", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, - }, + }, httpBaseHeaders...), ResponseHeaders: http.ResponseHeaders{ Add: []http.Header{ { @@ -1489,6 +1454,13 @@ func TestCreateServers(t *testing.T) { Type: http.InternalLocationType, Includes: internalIncludes, }, + { + Path: "= /keep-alive-enabled", + ProxyPass: "http://test_keep_alive_80$request_uri", + ProxySetHeaders: createBaseProxySetHeaders(httpUpgradeHeader, unsetHTTPConnectionHeader), + Type: http.ExternalLocationType, + Includes: externalIncludes, + }, } } @@ -1541,7 +1513,15 @@ func TestCreateServers(t *testing.T) { }, }) - result, httpMatchPair := createServers(conf, fakeGenerator) + keepAliveEnabledUpstream := http.Upstream{ + Name: "test_keep_alive_80", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, + } + keepAliveCheck := newKeepAliveChecker([]http.Upstream{keepAliveEnabledUpstream}) + + result, httpMatchPair := createServers(conf, fakeGenerator, keepAliveCheck) g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) @@ -1762,6 +1742,7 @@ func TestCreateServersConflicts(t *testing.T) { result, _ := createServers( dataplane.Configuration{HTTPServers: httpServers}, &policiesfakes.FakeGenerator{}, + alwaysFalseKeepAliveChecker, ) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) @@ -1912,7 +1893,7 @@ func TestCreateServers_Includes(t *testing.T) { conf := dataplane.Configuration{HTTPServers: httpServers, SSLServers: sslServers} - actualServers, matchPairs := createServers(conf, fakeGenerator) + actualServers, matchPairs := createServers(conf, fakeGenerator, alwaysFalseKeepAliveChecker) g.Expect(matchPairs).To(BeEmpty()) g.Expect(actualServers).To(HaveLen(len(expServers))) @@ -2073,7 +2054,7 @@ func TestCreateLocations_Includes(t *testing.T) { }, }) - locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator) + locations, matches, grpc := createLocations(&httpServer, "1", fakeGenerator, alwaysFalseKeepAliveChecker) g := NewWithT(t) g.Expect(grpc).To(BeFalse()) @@ -2256,10 +2237,15 @@ func TestCreateLocationsRootPath(t *testing.T) { t.Parallel() g := NewWithT(t) - locs, httpMatchPair, grpc := createLocations(&dataplane.VirtualServer{ - PathRules: test.pathRules, - Port: 80, - }, "1", &policiesfakes.FakeGenerator{}) + locs, httpMatchPair, grpc := createLocations( + &dataplane.VirtualServer{ + PathRules: test.pathRules, + Port: 80, + }, + "1", + &policiesfakes.FakeGenerator{}, + alwaysFalseKeepAliveChecker, + ) g.Expect(locs).To(Equal(test.expLocations)) g.Expect(httpMatchPair).To(BeEmpty()) g.Expect(grpc).To(Equal(test.grpc)) @@ -2897,7 +2883,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { filters *dataplane.HTTPFilters msg string expectedHeaders []http.Header - GRPC bool + baseHeaders []http.Header }{ { msg: "header filter", @@ -2918,7 +2904,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Remove: []string{"my-header"}, }, }, - expectedHeaders: []http.Header{ + expectedHeaders: append([]http.Header{ { Name: "Authorization", Value: "${authorization_header_var}my-auth", @@ -2931,39 +2917,8 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "my-header", Value: "", }, - { - Name: "Host", - Value: "$gw_api_compliant_host", - }, - { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", - }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, - { - Name: "X-Real-IP", - Value: "$remote_addr", - }, - { - Name: "X-Forwarded-Proto", - Value: "$scheme", - }, - { - Name: "X-Forwarded-Host", - Value: "$host", - }, - { - Name: "X-Forwarded-Port", - Value: "$server_port", - }, - }, + }, httpBaseHeaders...), + baseHeaders: httpBaseHeaders, }, { msg: "with url rewrite hostname", @@ -2993,14 +2948,6 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for", }, - { - Name: "Upgrade", - Value: "$http_upgrade", - }, - { - Name: "Connection", - Value: "$connection_upgrade", - }, { Name: "X-Real-IP", Value: "$remote_addr", @@ -3017,11 +2964,19 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "X-Forwarded-Port", Value: "$server_port", }, + { + Name: "Upgrade", + Value: "$http_upgrade", + }, + { + Name: "Connection", + Value: "$connection_upgrade", + }, }, + baseHeaders: createBaseProxySetHeaders(httpUpgradeHeader, httpConnectionHeader), }, { - msg: "header filter with gRPC", - GRPC: true, + msg: "header filter with gRPC", filters: &dataplane.HTTPFilters{ RequestHeaderModifiers: &dataplane.HTTPHeaderFilter{ Add: []dataplane.HTTPHeader{ @@ -3039,7 +2994,7 @@ func TestGenerateProxySetHeaders(t *testing.T) { Remove: []string{"my-header"}, }, }, - expectedHeaders: []http.Header{ + expectedHeaders: append([]http.Header{ { Name: "Authorization", Value: "${authorization_header_var}my-auth", @@ -3052,33 +3007,207 @@ func TestGenerateProxySetHeaders(t *testing.T) { Name: "my-header", Value: "", }, + }, grpcBaseHeaders...), + baseHeaders: grpcBaseHeaders, + }, + } + + for _, tc := range tests { + t.Run(tc.msg, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + headers := generateProxySetHeaders(tc.filters, tc.baseHeaders) + g.Expect(headers).To(Equal(tc.expectedHeaders)) + }) + } +} + +func TestCreateBaseProxySetHeaders(t *testing.T) { + t.Parallel() + + expBaseHeaders := []http.Header{ + { + Name: "Host", + Value: "$gw_api_compliant_host", + }, + { + Name: "X-Forwarded-For", + Value: "$proxy_add_x_forwarded_for", + }, + { + Name: "X-Real-IP", + Value: "$remote_addr", + }, + { + Name: "X-Forwarded-Proto", + Value: "$scheme", + }, + { + Name: "X-Forwarded-Host", + Value: "$host", + }, + { + Name: "X-Forwarded-Port", + Value: "$server_port", + }, + } + + tests := []struct { + msg string + additionalHeaders []http.Header + expBaseHeaders []http.Header + }{ + { + msg: "no additional headers", + additionalHeaders: []http.Header{}, + expBaseHeaders: expBaseHeaders, + }, + { + msg: "single additional headers", + additionalHeaders: []http.Header{ + grpcAuthorityHeader, + }, + expBaseHeaders: append(expBaseHeaders, grpcAuthorityHeader), + }, + { + msg: "multiple additional headers", + additionalHeaders: []http.Header{ + httpConnectionHeader, + httpUpgradeHeader, + }, + expBaseHeaders: append(expBaseHeaders, httpConnectionHeader, httpUpgradeHeader), + }, + { + msg: "unset connection header and upgrade header", + additionalHeaders: []http.Header{ + unsetHTTPConnectionHeader, + httpUpgradeHeader, + }, + expBaseHeaders: append(expBaseHeaders, unsetHTTPConnectionHeader, httpUpgradeHeader), + }, + } + + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + result := createBaseProxySetHeaders(test.additionalHeaders...) + g.Expect(result).To(Equal(test.expBaseHeaders)) + }) + } +} + +func TestGetConnectionHeader(t *testing.T) { + t.Parallel() + + tests := []struct { + msg string + upstreams []http.Upstream + expConnectionHeader http.Header + backends []dataplane.Backend + }{ + { + msg: "no upstreams with keepAlive enabled", + upstreams: []http.Upstream{ + { + Name: "upstream1", + }, { - Name: "Host", - Value: "$gw_api_compliant_host", + Name: "upstream2", }, { - Name: "X-Forwarded-For", - Value: "$proxy_add_x_forwarded_for", + Name: "upstream3", }, + }, + backends: []dataplane.Backend{ { - Name: "Authority", - Value: "$gw_api_compliant_host", + UpstreamName: "upstream1", }, { - Name: "X-Real-IP", - Value: "$remote_addr", + UpstreamName: "upstream2", }, { - Name: "X-Forwarded-Proto", - Value: "$scheme", + UpstreamName: "upstream3", }, + }, + expConnectionHeader: httpConnectionHeader, + }, + { + msg: "upstream with keepAlive enabled", + upstreams: []http.Upstream{ { - Name: "X-Forwarded-Host", - Value: "$host", + Name: "upstream", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, }, + }, + backends: []dataplane.Backend{ { - Name: "X-Forwarded-Port", - Value: "$server_port", + UpstreamName: "upstream", + }, + }, + expConnectionHeader: unsetHTTPConnectionHeader, + }, + { + msg: "multiple upstreams with keepAlive enabled", + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, + }, + { + Name: "upstream2", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 2, + Requests: 1, + }, + }, + { + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 3, + Time: "5s", + }, + }, + }, + backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + }, + { + UpstreamName: "upstream2", + }, + { + UpstreamName: "upstream3", + }, + }, + expConnectionHeader: unsetHTTPConnectionHeader, + }, + { + msg: "mix of upstreams with keepAlive enabled and disabled", + expConnectionHeader: unsetHTTPConnectionHeader, + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, + }, + { + Name: "upstream2", + }, + }, + backends: []dataplane.Backend{ + { + UpstreamName: "upstream1", + }, + { + UpstreamName: "upstream2", }, }, }, @@ -3089,8 +3218,10 @@ func TestGenerateProxySetHeaders(t *testing.T) { t.Parallel() g := NewWithT(t) - headers := generateProxySetHeaders(tc.filters, tc.GRPC) - g.Expect(headers).To(Equal(tc.expectedHeaders)) + keepAliveCheck := newKeepAliveChecker(tc.upstreams) + + connectionHeader := getConnectionHeader(keepAliveCheck, tc.backends) + g.Expect(connectionHeader).To(Equal(tc.expConnectionHeader)) }) } } diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 51af6f4f4b..bcee82c68f 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -6,11 +6,15 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/stream" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) -var upstreamsTemplate = gotemplate.Must(gotemplate.New("upstreams").Parse(upstreamsTemplateText)) +var ( + upstreamsTemplate = gotemplate.Must(gotemplate.New("upstreams").Parse(upstreamsTemplateText)) + streamUpstreamsTemplate = gotemplate.Must(gotemplate.New("streamUpstreams").Parse(streamUpstreamsTemplateText)) +) const ( // nginx503Server is used as a backend for services that cannot be resolved (have no IP address). @@ -31,9 +35,32 @@ const ( stateDir = "/var/lib/nginx/state" ) -func (g GeneratorImpl) executeUpstreams(conf dataplane.Configuration) []executeResult { - upstreams := g.createUpstreams(conf.Upstreams) +// keepAliveChecker takes an upstream name and returns if it has keep alive settings enabled. +type keepAliveChecker func(upstreamName string) bool + +func newKeepAliveChecker(upstreams []http.Upstream) keepAliveChecker { + upstreamMap := make(map[string]http.Upstream) + + for _, upstream := range upstreams { + upstreamMap[upstream.Name] = upstream + } + + return func(upstreamName string) bool { + if upstream, exists := upstreamMap[upstreamName]; exists { + return upstream.KeepAlive.Connections != 0 + } + + return false + } +} + +func newExecuteUpstreamsFunc(upstreams []http.Upstream) executeFunc { + return func(_ dataplane.Configuration) []executeResult { + return executeUpstreams(upstreams) + } +} +func executeUpstreams(upstreams []http.Upstream) []executeResult { result := executeResult{ dest: httpConfigFile, data: helpers.MustExecuteTemplate(upstreamsTemplate, upstreams), @@ -47,7 +74,7 @@ func (g GeneratorImpl) executeStreamUpstreams(conf dataplane.Configuration) []ex result := executeResult{ dest: streamConfigFile, - data: helpers.MustExecuteTemplate(upstreamsTemplate, upstreams), + data: helpers.MustExecuteTemplate(streamUpstreamsTemplate, upstreams), } return []executeResult{result} @@ -92,12 +119,15 @@ func (g GeneratorImpl) createStreamUpstream(up dataplane.Upstream) stream.Upstre } } -func (g GeneratorImpl) createUpstreams(upstreams []dataplane.Upstream) []http.Upstream { +func (g GeneratorImpl) createUpstreams( + upstreams []dataplane.Upstream, + processor upstreamsettings.Processor, +) []http.Upstream { // capacity is the number of upstreams + 1 for the invalid backend ref upstream ups := make([]http.Upstream, 0, len(upstreams)+1) for _, u := range upstreams { - ups = append(ups, g.createUpstream(u)) + ups = append(ups, g.createUpstream(u, processor)) } ups = append(ups, createInvalidBackendRefUpstream()) @@ -105,14 +135,23 @@ func (g GeneratorImpl) createUpstreams(upstreams []dataplane.Upstream) []http.Up return ups } -func (g GeneratorImpl) createUpstream(up dataplane.Upstream) http.Upstream { +func (g GeneratorImpl) createUpstream( + up dataplane.Upstream, + processor upstreamsettings.Processor, +) http.Upstream { var stateFile string + upstreamPolicySettings := processor.Process(up.Policies) + zoneSize := ossZoneSize if g.plus { zoneSize = plusZoneSize stateFile = fmt.Sprintf("%s/%s.conf", stateDir, up.Name) } + if upstreamPolicySettings.ZoneSize != "" { + zoneSize = upstreamPolicySettings.ZoneSize + } + if len(up.Endpoints) == 0 { return http.Upstream{ Name: up.Name, @@ -142,6 +181,7 @@ func (g GeneratorImpl) createUpstream(up dataplane.Upstream) http.Upstream { ZoneSize: zoneSize, StateFile: stateFile, Servers: upstreamServers, + KeepAlive: upstreamPolicySettings.KeepAlive, } } diff --git a/internal/mode/static/nginx/config/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index 40d5740ad0..e4bcaa69ad 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -5,6 +5,8 @@ package config // NGINX Plus needs 1m to support roughly the same amount of http servers (556 upstream servers). // For stream upstream servers, 512k will support 576 in OSS and 1m will support 991 in NGINX Plus // https://github.com/nginxinc/nginx-gateway-fabric/issues/483 +// +// if the keepalive directive is present, it is necessary to activate the load balancing method before the directive. const upstreamsTemplateText = ` {{ range $u := . }} upstream {{ $u.Name }} { @@ -20,6 +22,36 @@ upstream {{ $u.Name }} { server {{ $server.Address }}; {{- end }} {{- end }} + {{ if $u.KeepAlive.Connections -}} + keepalive {{ $u.KeepAlive.Connections }}; + {{- end }} + {{ if $u.KeepAlive.Requests -}} + keepalive_requests {{ $u.KeepAlive.Requests }}; + {{- end }} + {{ if $u.KeepAlive.Time -}} + keepalive_time {{ $u.KeepAlive.Time }}; + {{- end }} + {{ if $u.KeepAlive.Timeout -}} + keepalive_timeout {{ $u.KeepAlive.Timeout }}; + {{- end }} +} +{{ end -}} +` + +const streamUpstreamsTemplateText = ` +{{ range $u := . }} +upstream {{ $u.Name }} { + random two least_conn; + {{ if $u.ZoneSize -}} + zone {{ $u.Name }} {{ $u.ZoneSize }}; + {{- end }} + {{- if $u.StateFile }} + state {{ $u.StateFile }}; + {{- else }} + {{ range $server := $u.Servers }} + server {{ $server.Address }}; + {{- end }} + {{- end }} } {{ end -}} ` diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index f2e5b1071b..9d6cfdf208 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -4,8 +4,13 @@ import ( "testing" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/upstreamsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/stream" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" @@ -47,6 +52,32 @@ func TestExecuteUpstreams(t *testing.T) { }, }, }, + { + Name: "up5-usp", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.0", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, } expectedSubStrings := []string{ @@ -54,21 +85,32 @@ func TestExecuteUpstreams(t *testing.T) { "upstream up2", "upstream up3", "upstream up4-ipv6", + "upstream up5-usp", "upstream invalid-backend-ref", + "server 10.0.0.0:80;", "server 11.0.0.0:80;", "server [2001:db8::1]:80", + "server 12.0.0.0:80;", "server unix:/var/run/nginx/nginx-503-server.sock;", + + "keepalive 1;", + "keepalive_requests 1;", + "keepalive_time 5s;", + "keepalive_timeout 10s;", + "zone up5-usp 2m;", } - upstreamResults := gen.executeUpstreams(dataplane.Configuration{Upstreams: stateUpstreams}) + upstreams := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) + + upstreamResults := executeUpstreams(upstreams) g := NewWithT(t) g.Expect(upstreamResults).To(HaveLen(1)) - upstreams := string(upstreamResults[0].data) + nginxUpstreams := string(upstreamResults[0].data) g.Expect(upstreamResults[0].dest).To(Equal(httpConfigFile)) for _, expSubString := range expectedSubStrings { - g.Expect(upstreams).To(ContainSubstring(expSubString)) + g.Expect(nginxUpstreams).To(ContainSubstring(expSubString)) } } @@ -116,6 +158,32 @@ func TestCreateUpstreams(t *testing.T) { }, }, }, + { + Name: "up5-usp", + Endpoints: []resolver.Endpoint{ + { + Address: "12.0.0.0", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, } expUpstreams := []http.Upstream{ @@ -161,6 +229,21 @@ func TestCreateUpstreams(t *testing.T) { }, }, }, + { + Name: "up5-usp", + ZoneSize: "2m", + Servers: []http.UpstreamServer{ + { + Address: "12.0.0.0:80", + }, + }, + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, { Name: invalidBackendRef, Servers: []http.UpstreamServer{ @@ -172,7 +255,7 @@ func TestCreateUpstreams(t *testing.T) { } g := NewWithT(t) - result := gen.createUpstreams(stateUpstreams) + result := gen.createUpstreams(stateUpstreams, upstreamsettings.NewProcessor()) g.Expect(result).To(Equal(expUpstreams)) } @@ -181,8 +264,8 @@ func TestCreateUpstream(t *testing.T) { gen := GeneratorImpl{} tests := []struct { msg string - stateUpstream dataplane.Upstream expectedUpstream http.Upstream + stateUpstream dataplane.Upstream }{ { stateUpstream: dataplane.Upstream{ @@ -273,13 +356,183 @@ func TestCreateUpstream(t *testing.T) { }, msg: "endpoint ipv6", }, + { + stateUpstream: dataplane.Upstream{ + Name: "single upstreamSettingsPolicy", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "single upstreamSettingsPolicy", + ZoneSize: "2m", + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + msg: "single upstreamSettingsPolicy", + }, + { + stateUpstream: dataplane.Upstream{ + Name: "multiple upstreamSettingsPolicies", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("2m"), + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp2", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "multiple upstreamSettingsPolicies", + ZoneSize: "2m", + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + msg: "multiple upstreamSettingsPolicies", + }, + { + stateUpstream: dataplane.Upstream{ + Name: "empty upstreamSettingsPolicies", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "empty upstreamSettingsPolicies", + ZoneSize: ossZoneSize, + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + }, + msg: "empty upstreamSettingsPolicies", + }, + { + stateUpstream: dataplane.Upstream{ + Name: "upstreamSettingsPolicy with only keep alive settings", + Endpoints: []resolver.Endpoint{ + { + Address: "10.0.0.1", + Port: 80, + }, + }, + Policies: []policies.Policy{ + &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp1", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + KeepAlive: helpers.GetPointer(ngfAPI.UpstreamKeepAlive{ + Connections: helpers.GetPointer(int32(1)), + Requests: helpers.GetPointer(int32(1)), + Time: helpers.GetPointer[ngfAPI.Duration]("5s"), + Timeout: helpers.GetPointer[ngfAPI.Duration]("10s"), + }), + }, + }, + }, + }, + expectedUpstream: http.Upstream{ + Name: "upstreamSettingsPolicy with only keep alive settings", + ZoneSize: ossZoneSize, + Servers: []http.UpstreamServer{ + { + Address: "10.0.0.1:80", + }, + }, + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + msg: "upstreamSettingsPolicy with only keep alive settings", + }, } for _, test := range tests { t.Run(test.msg, func(t *testing.T) { t.Parallel() g := NewWithT(t) - result := gen.createUpstream(test.stateUpstream) + result := gen.createUpstream(test.stateUpstream, upstreamsettings.NewProcessor()) g.Expect(result).To(Equal(test.expectedUpstream)) }) } @@ -339,7 +592,7 @@ func TestCreateUpstreamPlus(t *testing.T) { t.Run(test.msg, func(t *testing.T) { t.Parallel() g := NewWithT(t) - result := gen.createUpstream(test.stateUpstream) + result := gen.createUpstream(test.stateUpstream, upstreamsettings.NewProcessor()) g.Expect(result).To(Equal(test.expectedUpstream)) }) } @@ -537,3 +790,215 @@ func TestCreateStreamUpstreamPlus(t *testing.T) { g := NewWithT(t) g.Expect(result).To(Equal(expectedUpstream)) } + +func TestKeepAliveChecker(t *testing.T) { + t.Parallel() + + tests := []struct { + msg string + upstreams []http.Upstream + expKeepAliveEnabled []bool + }{ + { + msg: "upstream with all keepAlive fields set", + upstreams: []http.Upstream{ + { + Name: "upAllKeepAliveFieldsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + true, + }, + }, + { + msg: "upstream with keepAlive connection field set", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveConnectionsSet", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + }, + }, + }, + expKeepAliveEnabled: []bool{ + true, + }, + }, + { + msg: "upstream with keepAlive requests field set", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveRequestsSet", + KeepAlive: http.UpstreamKeepAlive{ + Requests: 1, + }, + }, + }, + expKeepAliveEnabled: []bool{ + false, + }, + }, + { + msg: "upstream with keepAlive time field set", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveTimeSet", + KeepAlive: http.UpstreamKeepAlive{ + Time: "5s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + false, + }, + }, + { + msg: "upstream with keepAlive timeout field set", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveTimeoutSet", + KeepAlive: http.UpstreamKeepAlive{ + Timeout: "10s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + false, + }, + }, + { + msg: "upstream with no keepAlive fields set", + upstreams: []http.Upstream{ + { + Name: "upNoKeepAliveFieldsSet", + }, + }, + expKeepAliveEnabled: []bool{ + false, + }, + }, + { + msg: "upstream with keepAlive fields set to empty values", + upstreams: []http.Upstream{ + { + Name: "upKeepAliveFieldsEmpty", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 0, + Requests: 0, + Time: "", + Timeout: "", + }, + }, + }, + expKeepAliveEnabled: []bool{ + false, + }, + }, + { + msg: "multiple upstreams with keepAlive fields set", + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + { + Name: "upstream2", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + { + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + true, + true, + true, + }, + }, + { + msg: "mix of keepAlive enabled upstreams and disabled upstreams", + upstreams: []http.Upstream{ + { + Name: "upstream1", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + { + Name: "upstream2", + }, + { + Name: "upstream3", + KeepAlive: http.UpstreamKeepAlive{ + Connections: 1, + Requests: 1, + Time: "5s", + Timeout: "10s", + }, + }, + }, + expKeepAliveEnabled: []bool{ + true, + false, + true, + }, + }, + { + msg: "all upstreams without keepAlive fields set", + upstreams: []http.Upstream{ + { + Name: "upstream1", + }, + { + Name: "upstream2", + }, + { + Name: "upstream3", + }, + }, + expKeepAliveEnabled: []bool{ + false, + false, + false, + }, + }, + } + + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + keepAliveCheck := newKeepAliveChecker(test.upstreams) + + for index, upstream := range test.upstreams { + g.Expect(keepAliveCheck(upstream.Name)).To(Equal(test.expKeepAliveEnabled[index])) + } + }) + } +} diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index cb35491209..82edbcc7b1 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -216,6 +216,11 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { store: commonPolicyObjectStore, predicate: funcPredicate{stateChanged: isNGFPolicyRelevant}, }, + { + gvk: cfg.MustExtractGVK(&ngfAPI.UpstreamSettingsPolicy{}), + store: commonPolicyObjectStore, + predicate: funcPredicate{stateChanged: isNGFPolicyRelevant}, + }, { gvk: cfg.MustExtractGVK(&v1alpha2.TLSRoute{}), store: newObjectStoreMapAdapter(clusterStore.TLSRoutes), diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 334335c804..03d86a377b 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -265,13 +265,11 @@ func createHTTPBackendRef( } } -func createTLSBackendRef( - name v1.ObjectName, - namespace v1.Namespace, -) v1.BackendRef { +func createTLSBackendRef(name, namespace string) v1.BackendRef { kindSvc := v1.Kind("Service") + ns := v1.Namespace(namespace) return v1.BackendRef{ - BackendObjectReference: createBackendRefObj(&kindSvc, name, &namespace), + BackendObjectReference: createBackendRefObj(&kindSvc, v1.ObjectName(name), &ns), } } @@ -437,6 +435,7 @@ var _ = Describe("ChangeProcessor", func() { gatewayAPICRD, gatewayAPICRDUpdated *metav1.PartialObjectMetadata httpRouteKey1, httpRouteKey2, grpcRouteKey1, grpcRouteKey2 graph.RouteKey trKey1, trKey2 graph.L4RouteKey + refSvc, refGRPCSvc, refTLSSvc types.NamespacedName ) processAndValidateGraph := func(expGraph *graph.Graph) { @@ -450,12 +449,16 @@ var _ = Describe("ChangeProcessor", func() { gcUpdated = gc.DeepCopy() gcUpdated.Generation++ + refSvc = types.NamespacedName{Namespace: "service-ns", Name: "service"} + refGRPCSvc = types.NamespacedName{Namespace: "grpc-service-ns", Name: "grpc-service"} + refTLSSvc = types.NamespacedName{Namespace: "tls-service-ns", Name: "tls-service"} + crossNsHTTPBackendRef := v1.HTTPBackendRef{ BackendRef: v1.BackendRef{ BackendObjectReference: v1.BackendObjectReference{ Kind: helpers.GetPointer[v1.Kind]("Service"), - Name: "service", - Namespace: helpers.GetPointer[v1.Namespace]("service-ns"), + Name: v1.ObjectName(refSvc.Name), + Namespace: helpers.GetPointer(v1.Namespace(refSvc.Namespace)), Port: helpers.GetPointer[v1.PortNumber](80), }, }, @@ -465,8 +468,8 @@ var _ = Describe("ChangeProcessor", func() { BackendRef: v1.BackendRef{ BackendObjectReference: v1.BackendObjectReference{ Kind: helpers.GetPointer[v1.Kind]("Service"), - Name: "grpc-service", - Namespace: helpers.GetPointer[v1.Namespace]("grpc-service-ns"), + Name: v1.ObjectName(refGRPCSvc.Name), + Namespace: helpers.GetPointer(v1.Namespace(refGRPCSvc.Namespace)), Port: helpers.GetPointer[v1.PortNumber](80), }, }, @@ -486,7 +489,7 @@ var _ = Describe("ChangeProcessor", func() { gr2 = createGRPCRoute("gr-2", "gateway-2", "bar.example.com") grpcRouteKey2 = graph.CreateRouteKey(gr2) - tlsBackendRef := createTLSBackendRef("tls-service", "tls-service-ns") + tlsBackendRef := createTLSBackendRef(refTLSSvc.Name, refTLSSvc.Namespace) tr1 = createTLSRoute("tr-1", "gateway-1", "foo.tls.com", tlsBackendRef) trKey1 = graph.CreateRouteKeyL4(tr1) tr1Updated = tr1.DeepCopy() @@ -666,7 +669,7 @@ var _ = Describe("ChangeProcessor", func() { { BackendRefs: []graph.BackendRef{ { - SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + SvcNsName: refSvc, Weight: 1, }, }, @@ -761,7 +764,7 @@ var _ = Describe("ChangeProcessor", func() { { BackendRefs: []graph.BackendRef{ { - SvcNsName: types.NamespacedName{Namespace: "grpc-service-ns", Name: "grpc-service"}, + SvcNsName: refGRPCSvc, Weight: 1, }, }, @@ -841,7 +844,7 @@ var _ = Describe("ChangeProcessor", func() { Spec: graph.L4RouteSpec{ Hostnames: tr1.Spec.Hostnames, BackendRef: graph.BackendRef{ - SvcNsName: types.NamespacedName{Namespace: "tls-service-ns", Name: "tls-service"}, + SvcNsName: refTLSSvc, Valid: false, }, }, @@ -869,7 +872,7 @@ var _ = Describe("ChangeProcessor", func() { Spec: graph.L4RouteSpec{ Hostnames: tr2.Spec.Hostnames, BackendRef: graph.BackendRef{ - SvcNsName: types.NamespacedName{Namespace: "tls-service-ns", Name: "tls-service"}, + SvcNsName: refTLSSvc, Valid: false, }, }, @@ -935,19 +938,10 @@ var _ = Describe("ChangeProcessor", func() { L4Routes: map[graph.L4RouteKey]*graph.L4Route{trKey1: expRouteTR1}, Routes: map[graph.RouteKey]*graph.L7Route{httpRouteKey1: expRouteHR1, grpcRouteKey1: expRouteGR1}, ReferencedSecrets: map[types.NamespacedName]*graph.Secret{}, - ReferencedServices: map[types.NamespacedName]struct{}{ - { - Namespace: "service-ns", - Name: "service", - }: {}, - { - Namespace: "grpc-service-ns", - Name: "grpc-service", - }: {}, - { - Namespace: "tls-service-ns", - Name: "tls-service", - }: {}, + ReferencedServices: map[types.NamespacedName]*graph.ReferencedService{ + refSvc: {}, + refTLSSvc: {}, + refGRPCSvc: {}, }, } }) @@ -1181,7 +1175,7 @@ var _ = Describe("ChangeProcessor", func() { "Backend ref to Service grpc-service-ns/grpc-service not permitted by any ReferenceGrant", ), } - delete(expGraph.ReferencedServices, types.NamespacedName{Namespace: "grpc-service-ns", Name: "grpc-service"}) + delete(expGraph.ReferencedServices, refGRPCSvc) expRouteGR1.Spec.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} // no ref grant exists yet for tr1 @@ -1190,7 +1184,7 @@ var _ = Describe("ChangeProcessor", func() { "Backend ref to Service tls-service-ns/tls-service not permitted by any ReferenceGrant", ), } - delete(expGraph.ReferencedServices, types.NamespacedName{Namespace: "tls-service-ns", Name: "tls-service"}) + delete(expGraph.ReferencedServices, refTLSSvc) expRouteTR1.Spec.BackendRef.SvcNsName = types.NamespacedName{} expGraph.ReferencedSecrets[client.ObjectKeyFromObject(diffNsTLSSecret)] = &graph.Secret{ @@ -2350,11 +2344,13 @@ var _ = Describe("ChangeProcessor", func() { Describe("NGF Policy resource changes", Ordered, func() { var ( - gw *v1.Gateway - route *v1.HTTPRoute - csp, cspUpdated *ngfAPI.ClientSettingsPolicy - obs, obsUpdated *ngfAPI.ObservabilityPolicy - cspKey, obsKey graph.PolicyKey + gw *v1.Gateway + route *v1.HTTPRoute + svc *apiv1.Service + csp, cspUpdated *ngfAPI.ClientSettingsPolicy + obs, obsUpdated *ngfAPI.ObservabilityPolicy + usp, uspUpdated *ngfAPI.UpstreamSettingsPolicy + cspKey, obsKey, uspKey graph.PolicyKey ) BeforeAll(func() { @@ -2365,7 +2361,28 @@ var _ = Describe("ChangeProcessor", func() { Expect(newGraph.NGFPolicies).To(BeEmpty()) gw = createGateway("gw", createHTTPListener()) - route = createHTTPRoute("hr-1", "gw", "foo.example.com", v1.HTTPBackendRef{}) + route = createHTTPRoute( + "hr-1", + "gw", + "foo.example.com", + v1.HTTPBackendRef{ + BackendRef: v1.BackendRef{ + BackendObjectReference: v1.BackendObjectReference{ + Group: helpers.GetPointer[v1.Group](""), + Kind: helpers.GetPointer[v1.Kind](kinds.Service), + Name: "svc", + Port: helpers.GetPointer[v1.PortNumber](80), + }, + }, + }, + ) + + svc = &apiv1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc", + Namespace: "test", + }, + } csp = &ngfAPI.ClientSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -2426,6 +2443,35 @@ var _ = Describe("ChangeProcessor", func() { Version: "v1alpha1", }, } + + usp = &ngfAPI.UpstreamSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "usp", + Namespace: "test", + }, + Spec: ngfAPI.UpstreamSettingsPolicySpec{ + ZoneSize: helpers.GetPointer[ngfAPI.Size]("10m"), + TargetRefs: []v1alpha2.LocalPolicyTargetReference{ + { + Group: "core", + Kind: kinds.Service, + Name: "svc", + }, + }, + }, + } + + uspUpdated = usp.DeepCopy() + uspUpdated.Spec.ZoneSize = helpers.GetPointer[ngfAPI.Size]("20m") + + uspKey = graph.PolicyKey{ + NsName: types.NamespacedName{Name: "usp", Namespace: "test"}, + GVK: schema.GroupVersionKind{ + Group: ngfAPI.GroupName, + Kind: kinds.UpstreamSettingsPolicy, + Version: "v1alpha1", + }, + } }) /* @@ -2438,6 +2484,7 @@ var _ = Describe("ChangeProcessor", func() { It("reports no changes", func() { processor.CaptureUpsertChange(csp) processor.CaptureUpsertChange(obs) + processor.CaptureUpsertChange(usp) changed, _ := processor.Process() Expect(changed).To(Equal(state.NoChange)) @@ -2458,12 +2505,19 @@ var _ = Describe("ChangeProcessor", func() { Expect(changed).To(Equal(state.ClusterStateChange)) Expect(graph.NGFPolicies).To(HaveKey(obsKey)) Expect(graph.NGFPolicies[obsKey].Source).To(Equal(obs)) + + processor.CaptureUpsertChange(svc) + changed, graph = processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + Expect(graph.NGFPolicies).To(HaveKey(uspKey)) + Expect(graph.NGFPolicies[uspKey].Source).To(Equal(usp)) }) }) When("the policy is updated", func() { It("captures changes for a policy", func() { processor.CaptureUpsertChange(cspUpdated) processor.CaptureUpsertChange(obsUpdated) + processor.CaptureUpsertChange(uspUpdated) changed, graph := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) @@ -2471,12 +2525,15 @@ var _ = Describe("ChangeProcessor", func() { Expect(graph.NGFPolicies[cspKey].Source).To(Equal(cspUpdated)) Expect(graph.NGFPolicies).To(HaveKey(obsKey)) Expect(graph.NGFPolicies[obsKey].Source).To(Equal(obsUpdated)) + Expect(graph.NGFPolicies).To(HaveKey(uspKey)) + Expect(graph.NGFPolicies[uspKey].Source).To(Equal(uspUpdated)) }) }) When("the policy is deleted", func() { It("removes the policy from the graph", func() { processor.CaptureDeleteChange(&ngfAPI.ClientSettingsPolicy{}, client.ObjectKeyFromObject(csp)) processor.CaptureDeleteChange(&ngfAPI.ObservabilityPolicy{}, client.ObjectKeyFromObject(obs)) + processor.CaptureDeleteChange(&ngfAPI.UpstreamSettingsPolicy{}, client.ObjectKeyFromObject(usp)) changed, graph := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index f533b67a1b..cafcbc0db8 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -41,12 +41,19 @@ func BuildConfiguration( httpServers, sslServers := buildServers(g) backendGroups := buildBackendGroups(append(httpServers, sslServers...)) + upstreams := buildUpstreams( + ctx, + g.Gateway.Listeners, + serviceResolver, + g.ReferencedServices, + baseHTTPConfig.IPFamily, + ) config := Configuration{ HTTPServers: httpServers, SSLServers: sslServers, TLSPassthroughServers: buildPassthroughServers(g), - Upstreams: buildUpstreams(ctx, g.Gateway.Listeners, serviceResolver, baseHTTPConfig.IPFamily), + Upstreams: upstreams, StreamUpstreams: buildStreamUpstreams(ctx, g.Gateway.Listeners, serviceResolver, baseHTTPConfig.IPFamily), BackendGroups: backendGroups, SSLKeyPairs: buildSSLKeyPairs(g.ReferencedSecrets, g.Gateway.Listeners), @@ -602,6 +609,7 @@ func buildUpstreams( ctx context.Context, listeners []*graph.Listener, svcResolver resolver.ServiceResolver, + referencedServices map[types.NamespacedName]*graph.ReferencedService, ipFamily IPFamilyType, ) []Upstream { // There can be duplicate upstreams if multiple routes reference the same upstream. @@ -642,10 +650,16 @@ func buildUpstreams( errMsg = err.Error() } + var upstreamPolicies []policies.Policy + if graphSvc, exists := referencedServices[br.SvcNsName]; exists { + upstreamPolicies = buildPolicies(graphSvc.Policies) + } + uniqueUpstreams[upstreamName] = Upstream{ Name: upstreamName, Endpoints: eps, ErrorMsg: errMsg, + Policies: upstreamPolicies, } } } diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index 7bed89e5d2..037bcd7d9d 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -81,6 +81,7 @@ func getNormalGraph() *graph.Graph { Routes: map[graph.RouteKey]*graph.L7Route{}, ReferencedSecrets: map[types.NamespacedName]*graph.Secret{}, ReferencedCaCertConfigMaps: map[types.NamespacedName]*graph.CaCertConfigMap{}, + ReferencedServices: map[types.NamespacedName]*graph.ReferencedService{}, } } @@ -2804,6 +2805,13 @@ func TestBuildUpstreams(t *testing.T) { }, } + policyEndpoints := []resolver.Endpoint{ + { + Address: "16.0.0.0", + Port: 80, + }, + } + createBackendRefs := func(serviceNames ...string) []graph.BackendRef { var backends []graph.BackendRef for _, name := range serviceNames { @@ -2836,6 +2844,8 @@ func TestBuildUpstreams(t *testing.T) { invalidHRRefs := createBackendRefs("abc") + refsWithPolicies := createBackendRefs("policies") + routes := map[graph.RouteKey]*graph.L7Route{ {NamespacedName: types.NamespacedName{Name: "hr1", Namespace: "test"}}: { Valid: true, @@ -2893,6 +2903,15 @@ func TestBuildUpstreams(t *testing.T) { }, } + routesWithPolicies := map[graph.RouteKey]*graph.L7Route{ + {NamespacedName: types.NamespacedName{Name: "policies", Namespace: "test"}}: { + Valid: true, + Spec: graph.L7RouteSpec{ + Rules: refsToValidRules(refsWithPolicies), + }, + }, + } + listeners := []*graph.Listener{ { Name: "invalid-listener", @@ -2919,6 +2938,41 @@ func TestBuildUpstreams(t *testing.T) { Valid: true, Routes: routes3, }, + { + Name: "listener-5", + Valid: true, + Routes: routesWithPolicies, + }, + } + + validPolicy1 := &policiesfakes.FakePolicy{} + validPolicy2 := &policiesfakes.FakePolicy{} + invalidPolicy := &policiesfakes.FakePolicy{} + + referencedServices := map[types.NamespacedName]*graph.ReferencedService{ + {Name: "bar", Namespace: "test"}: {}, + {Name: "baz", Namespace: "test"}: {}, + {Name: "baz2", Namespace: "test"}: {}, + {Name: "foo", Namespace: "test"}: {}, + {Name: "empty-endpoints", Namespace: "test"}: {}, + {Name: "nil-endpoints", Namespace: "test"}: {}, + {Name: "ipv6-endpoints", Namespace: "test"}: {}, + {Name: "policies", Namespace: "test"}: { + Policies: []*graph.Policy{ + { + Valid: true, + Source: validPolicy1, + }, + { + Valid: false, + Source: invalidPolicy, + }, + { + Valid: true, + Source: validPolicy2, + }, + }, + }, } emptyEndpointsErrMsg := "empty endpoints error" @@ -2955,6 +3009,11 @@ func TestBuildUpstreams(t *testing.T) { Name: "test_ipv6-endpoints_80", Endpoints: ipv6Endpoints, }, + { + Name: "test_policies_80", + Endpoints: policyEndpoints, + Policies: []policies.Policy{validPolicy1, validPolicy2}, + }, } fakeResolver := &resolverfakes.FakeServiceResolver{} @@ -2981,6 +3040,8 @@ func TestBuildUpstreams(t *testing.T) { return abcEndpoints, nil case "ipv6-endpoints": return ipv6Endpoints, nil + case "policies": + return policyEndpoints, nil default: return nil, fmt.Errorf("unexpected service %s", svcNsName.Name) } @@ -2988,7 +3049,7 @@ func TestBuildUpstreams(t *testing.T) { g := NewWithT(t) - upstreams := buildUpstreams(context.TODO(), listeners, fakeResolver, Dual) + upstreams := buildUpstreams(context.TODO(), listeners, fakeResolver, referencedServices, Dual) g.Expect(upstreams).To(ConsistOf(expUpstreams)) } diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index f9ed79b762..128990617b 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -111,6 +111,8 @@ type Upstream struct { ErrorMsg string // Endpoints are the endpoints of the Upstream. Endpoints []resolver.Endpoint + // Policies holds all the valid policies that apply to the Upstream. + Policies []policies.Policy } // SSL is the SSL configuration for a server. diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index 73b2ecac68..0b56ec1018 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -66,9 +66,8 @@ type Graph struct { ReferencedSecrets map[types.NamespacedName]*Secret // ReferencedNamespaces includes Namespaces with labels that match the Gateway Listener's label selector. ReferencedNamespaces map[types.NamespacedName]*v1.Namespace - // ReferencedServices includes the NamespacedNames of all the Services that are referenced by at least one HTTPRoute. - // Storing the whole resource is not necessary, compared to the similar maps above. - ReferencedServices map[types.NamespacedName]struct{} + // ReferencedServices includes the NamespacedNames of all the Services that are referenced by at least one Route. + ReferencedServices map[types.NamespacedName]*ReferencedService // ReferencedCaCertConfigMaps includes ConfigMaps that have been referenced by any BackendTLSPolicies. ReferencedCaCertConfigMaps map[types.NamespacedName]*CaCertConfigMap // BackendTLSPolicies holds BackendTLSPolicy resources. @@ -155,8 +154,18 @@ func (g *Graph) IsNGFPolicyRelevant( } for _, ref := range policy.GetTargetRefs() { - if ref.Group == gatewayv1.GroupName && g.gatewayAPIResourceExist(ref, policy.GetNamespace()) { - return true + switch ref.Group { + case gatewayv1.GroupName: + if g.gatewayAPIResourceExist(ref, policy.GetNamespace()) { + return true + } + case "", "core": + if ref.Kind == kinds.Service { + svcNsName := types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} + if _, exists := g.ReferencedServices[svcNsName]; exists { + return true + } + } } } @@ -249,7 +258,7 @@ func BuildGraph( referencedNamespaces := buildReferencedNamespaces(state.Namespaces, gw) - referencedServices := buildReferencedServices(routes, l4routes) + referencedServices := buildReferencedServices(routes, l4routes, gw) // policies must be processed last because they rely on the state of the other resources in the graph processedPolicies := processPolicies( @@ -257,6 +266,7 @@ func BuildGraph( validators.PolicyValidator, processedGws, routes, + referencedServices, globalSettings, ) diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index 627e308bc0..f2b71d05f8 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -33,7 +33,6 @@ func TestBuildGraph(t *testing.T) { const ( gcName = "my-class" controllerName = "my.controller" - testNS = "test" ) protectedPorts := ProtectedPorts{ @@ -894,7 +893,7 @@ func TestBuildGraph(t *testing.T) { ReferencedNamespaces: map[types.NamespacedName]*v1.Namespace{ client.ObjectKeyFromObject(ns): ns, }, - ReferencedServices: map[types.NamespacedName]struct{}{ + ReferencedServices: map[types.NamespacedName]*ReferencedService{ client.ObjectKeyFromObject(svc): {}, client.ObjectKeyFromObject(svc1): {}, }, @@ -1157,7 +1156,7 @@ func TestIsReferenced(t *testing.T) { ReferencedNamespaces: map[types.NamespacedName]*v1.Namespace{ client.ObjectKeyFromObject(nsInGraph): nsInGraph, }, - ReferencedServices: map[types.NamespacedName]struct{}{ + ReferencedServices: map[types.NamespacedName]*ReferencedService{ client.ObjectKeyFromObject(serviceInGraph): {}, }, ReferencedCaCertConfigMaps: map[types.NamespacedName]*CaCertConfigMap{ @@ -1359,6 +1358,7 @@ func TestIsNGFPolicyRelevant(t *testing.T) { Source: &policiesfakes.FakePolicy{}, }, }, + ReferencedServices: nil, } } @@ -1469,6 +1469,46 @@ func TestIsNGFPolicyRelevant(t *testing.T) { nsname: types.NamespacedName{Namespace: "test", Name: "nil-gw-source"}, expRelevant: false, }, + { + name: "relevant; policy references a Service that is referenced by a route, group core is inferred", + graph: getModifiedGraph(func(g *Graph) *Graph { + g.ReferencedServices = map[types.NamespacedName]*ReferencedService{ + {Namespace: "test", Name: "ref-service"}: {}, + } + + return g + }), + policy: getPolicy(createTestRef(kinds.Service, "", "ref-service")), + nsname: types.NamespacedName{Namespace: "test", Name: "policy-for-svc"}, + expRelevant: true, + }, + { + name: "relevant; policy references a Service that is referenced by a route, group core is explicit", + graph: getModifiedGraph(func(g *Graph) *Graph { + g.ReferencedServices = map[types.NamespacedName]*ReferencedService{ + {Namespace: "test", Name: "ref-service"}: {}, + } + + return g + }), + policy: getPolicy(createTestRef(kinds.Service, "core", "ref-service")), + nsname: types.NamespacedName{Namespace: "test", Name: "policy-for-svc"}, + expRelevant: true, + }, + { + name: "irrelevant; policy references a Service that is not referenced by a route, group core is inferred", + graph: getGraph(), + policy: getPolicy(createTestRef(kinds.Service, "", "not-ref-service")), + nsname: types.NamespacedName{Namespace: "test", Name: "policy-for-not-ref-svc"}, + expRelevant: false, + }, + { + name: "irrelevant; policy references a Service that is not referenced by a route, group core is explicit", + graph: getGraph(), + policy: getPolicy(createTestRef(kinds.Service, "core", "not-ref-service")), + nsname: types.NamespacedName{Namespace: "test", Name: "policy-for-not-ref-svc"}, + expRelevant: false, + }, } for _, test := range tests { diff --git a/internal/mode/static/state/graph/policies.go b/internal/mode/static/state/graph/policies.go index a4b794c5e1..7634c2d099 100644 --- a/internal/mode/static/state/graph/policies.go +++ b/internal/mode/static/state/graph/policies.go @@ -21,7 +21,7 @@ import ( type Policy struct { // Source is the corresponding Policy resource. Source policies.Policy - // Ancestors is list of ancestor objects of the Policy. Used in status. + // Ancestors is a list of ancestor objects of the Policy. Used in status. Ancestors []PolicyAncestor // TargetRefs are the resources that the Policy targets. TargetRefs []PolicyTargetRef @@ -63,6 +63,7 @@ const ( gatewayGroupKind = v1.GroupName + "/" + kinds.Gateway hrGroupKind = v1.GroupName + "/" + kinds.HTTPRoute grpcGroupKind = v1.GroupName + "/" + kinds.GRPCRoute + serviceGroupKind = "core" + "/" + kinds.Service ) // attachPolicies attaches the graph's processed policies to the resources they target. It modifies the graph in place. @@ -83,11 +84,49 @@ func (g *Graph) attachPolicies(ctlrName string) { } attachPolicyToRoute(policy, route, ctlrName) + case kinds.Service: + svc, exists := g.ReferencedServices[ref.Nsname] + if !exists { + continue + } + + attachPolicyToService(policy, svc, g.Gateway, ctlrName) } } } } +func attachPolicyToService( + policy *Policy, + svc *ReferencedService, + gw *Gateway, + ctlrName string, +) { + if ngfPolicyAncestorsFull(policy, ctlrName) { + return + } + + ancestor := PolicyAncestor{ + Ancestor: createParentReference(v1.GroupName, kinds.Gateway, client.ObjectKeyFromObject(gw.Source)), + } + + if !gw.Valid { + ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("Parent Gateway is invalid")} + if ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) { + return + } + + policy.Ancestors = append(policy.Ancestors, ancestor) + return + } + + if !ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) { + policy.Ancestors = append(policy.Ancestors, ancestor) + } + + svc.Policies = append(svc.Policies, policy) +} + func attachPolicyToRoute(policy *Policy, route *L7Route, ctlrName string) { kind := v1.Kind(kinds.HTTPRoute) if route.RouteType == RouteTypeGRPC { @@ -158,6 +197,7 @@ func processPolicies( validator validation.PolicyValidator, gateways processedGateways, routes map[RouteKey]*L7Route, + services map[types.NamespacedName]*ReferencedService, globalSettings *policies.GlobalSettings, ) map[PolicyKey]*Policy { if len(pols) == 0 || gateways.Winner == nil { @@ -174,9 +214,8 @@ func processPolicies( for _, ref := range policy.GetTargetRefs() { refNsName := types.NamespacedName{Name: string(ref.Name), Namespace: policy.GetNamespace()} - refGroupKind := fmt.Sprintf("%s/%s", ref.Group, ref.Kind) - switch refGroupKind { + switch refGroupKind(ref.Group, ref.Kind) { case gatewayGroupKind: if !gatewayExists(refNsName, gateways.Winner, gateways.Ignored) { continue @@ -187,6 +226,10 @@ func processPolicies( } else { targetedRoutes[client.ObjectKeyFromObject(route.Source)] = route } + case serviceGroupKind: + if _, exists := services[refNsName]; !exists { + continue + } default: continue } @@ -203,22 +246,8 @@ func processPolicies( continue } - for _, targetedRoute := range targetedRoutes { - // We need to check if this route referenced in the policy has an overlapping - // hostname:port/path with any other route that isn't referenced by this policy. - // If so, deny the policy. - hostPortPaths := buildHostPortPaths(targetedRoute) - - for _, route := range routes { - if _, ok := targetedRoutes[client.ObjectKeyFromObject(route.Source)]; ok { - continue - } - - if cond := checkForRouteOverlap(route, hostPortPaths); cond != nil { - conds = append(conds, *cond) - } - } - } + overlapConds := checkTargetRoutesForOverlap(targetedRoutes, routes) + conds = append(conds, overlapConds...) conds = append(conds, validator.Validate(policy, globalSettings)...) @@ -236,6 +265,32 @@ func processPolicies( return processedPolicies } +func checkTargetRoutesForOverlap( + targetedRoutes map[types.NamespacedName]*L7Route, + graphRoutes map[RouteKey]*L7Route, +) []conditions.Condition { + var conds []conditions.Condition + + for _, targetedRoute := range targetedRoutes { + // We need to check if this route referenced in the policy has an overlapping + // hostname:port/path with any other route that isn't referenced by this policy. + // If so, deny the policy. + hostPortPaths := buildHostPortPaths(targetedRoute) + + for _, route := range graphRoutes { + if _, ok := targetedRoutes[client.ObjectKeyFromObject(route.Source)]; ok { + continue + } + + if cond := checkForRouteOverlap(route, hostPortPaths); cond != nil { + conds = append(conds, *cond) + } + } + } + + return conds +} + // checkForRouteOverlap checks if any route references the same hostname:port/path combination // as a route referenced in a policy. func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *conditions.Condition { @@ -355,3 +410,12 @@ func markConflictedPolicies(pols map[PolicyKey]*Policy, validator validation.Pol } } } + +// refGroupKind formats the group and kind as a string. +func refGroupKind(group v1.Group, kind v1.Kind) string { + if group == "" { + return fmt.Sprintf("core/%s", kind) + } + + return fmt.Sprintf("%s/%s", group, kind) +} diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index 340d3f10f0..6a840230fb 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/internal/mode/static/state/graph/policies_test.go @@ -15,7 +15,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" - policiesfakes "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -24,7 +24,9 @@ var testNs = "test" func TestAttachPolicies(t *testing.T) { t.Parallel() + policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "Policy"} + createPolicy := func(targetRefsNames []string, refKind v1.Kind) *Policy { targetRefs := make([]PolicyTargetRef, 0, len(targetRefsNames)) for _, name := range targetRefsNames { @@ -48,18 +50,6 @@ func TestAttachPolicies(t *testing.T) { } } - createGateway := func(name string) *Gateway { - return &Gateway{ - Source: &v1.Gateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: testNs, - }, - }, - Valid: true, - } - } - createRoutesForGraph := func(routes map[string]RouteType) map[RouteKey]*L7Route { routesMap := make(map[RouteKey]*L7Route, len(routes)) for routeName, routeType := range routes { @@ -84,94 +74,137 @@ func TestAttachPolicies(t *testing.T) { return routesMap } - expectNoPolicyAttachment := func(g *WithT, graph *Graph) { + expectNoGatewayPolicyAttachment := func(g *WithT, graph *Graph) { if graph.Gateway != nil { g.Expect(graph.Gateway.Policies).To(BeNil()) } + } + expectNoRoutePolicyAttachment := func(g *WithT, graph *Graph) { for _, r := range graph.Routes { g.Expect(r.Policies).To(BeNil()) } } - expectPolicyAttachment := func(g *WithT, graph *Graph) { + expectNoSvcPolicyAttachment := func(g *WithT, graph *Graph) { + for _, r := range graph.ReferencedServices { + g.Expect(r.Policies).To(BeNil()) + } + } + + expectGatewayPolicyAttachment := func(g *WithT, graph *Graph) { if graph.Gateway != nil { g.Expect(graph.Gateway.Policies).To(HaveLen(1)) } + } + expectRoutePolicyAttachment := func(g *WithT, graph *Graph) { for _, r := range graph.Routes { g.Expect(r.Policies).To(HaveLen(1)) } } - expectGatewayPolicyAttachment := func(g *WithT, graph *Graph) { - if graph.Gateway != nil { - g.Expect(graph.Gateway.Policies).To(HaveLen(1)) + expectSvcPolicyAttachment := func(g *WithT, graph *Graph) { + for _, r := range graph.ReferencedServices { + g.Expect(r.Policies).To(HaveLen(1)) } + } - for _, r := range graph.Routes { - g.Expect(r.Policies).To(BeNil()) + expectNoAttachmentList := []func(g *WithT, graph *Graph){ + expectNoGatewayPolicyAttachment, + expectNoSvcPolicyAttachment, + expectNoRoutePolicyAttachment, + } + + expectAllAttachmentList := []func(g *WithT, graph *Graph){ + expectGatewayPolicyAttachment, + expectSvcPolicyAttachment, + expectRoutePolicyAttachment, + } + + getPolicies := func() map[PolicyKey]*Policy { + return map[PolicyKey]*Policy{ + createTestPolicyKey(policyGVK, "gw-policy1"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway), + createTestPolicyKey(policyGVK, "route-policy1"): createPolicy( + []string{"hr1-route", "hr2-route"}, + kinds.HTTPRoute, + ), + createTestPolicyKey(policyGVK, "grpc-route-policy1"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute), + createTestPolicyKey(policyGVK, "svc-policy"): createPolicy([]string{"svc-1"}, kinds.Service), + } + } + + getRoutes := func() map[RouteKey]*L7Route { + return createRoutesForGraph( + map[string]RouteType{ + "hr1-route": RouteTypeHTTP, + "hr2-route": RouteTypeHTTP, + "grpc-route": RouteTypeGRPC, + }, + ) + } + + getGateway := func() *Gateway { + return &Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gateway", + Namespace: testNs, + }, + }, + Valid: true, + } + } + + getServices := func() map[types.NamespacedName]*ReferencedService { + return map[types.NamespacedName]*ReferencedService{ + {Namespace: testNs, Name: "svc-1"}: {}, } } tests := []struct { gateway *Gateway routes map[RouteKey]*L7Route + svcs map[types.NamespacedName]*ReferencedService ngfPolicies map[PolicyKey]*Policy - expect func(g *WithT, graph *Graph) name string + expects []func(g *WithT, graph *Graph) }{ { - name: "nil Gateway", - routes: createRoutesForGraph( - map[string]RouteType{ - "hr1-route": RouteTypeHTTP, - "hr2-route": RouteTypeHTTP, - "grpc-route": RouteTypeGRPC, - }, - ), - ngfPolicies: map[PolicyKey]*Policy{ - createTestPolicyKey(policyGVK, "gw-policy"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway), - createTestPolicyKey(policyGVK, "route-policy"): createPolicy( - []string{"hr1-route", "hr2-route"}, - kinds.HTTPRoute, - ), - createTestPolicyKey(policyGVK, "grpc-route-policy"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute), - }, - expect: expectNoPolicyAttachment, + name: "nil Gateway; no policies attach", + routes: getRoutes(), + ngfPolicies: getPolicies(), + expects: expectNoAttachmentList, }, { - name: "nil routes", - gateway: createGateway("gateway"), - ngfPolicies: map[PolicyKey]*Policy{ - createTestPolicyKey(policyGVK, "gw-policy1"): createPolicy([]string{"gateway", "gateway1"}, kinds.Gateway), - createTestPolicyKey(policyGVK, "route-policy1"): createPolicy( - []string{"hr1-route", "hr2-route"}, - kinds.HTTPRoute, - ), - createTestPolicyKey(policyGVK, "grpc-route-policy1"): createPolicy([]string{"grpc-route"}, kinds.GRPCRoute), + name: "nil Routes; gateway and service policies attach", + gateway: getGateway(), + svcs: getServices(), + ngfPolicies: getPolicies(), + expects: []func(g *WithT, graph *Graph){ + expectGatewayPolicyAttachment, + expectSvcPolicyAttachment, + expectNoRoutePolicyAttachment, }, - expect: expectGatewayPolicyAttachment, }, { - name: "normal", - routes: createRoutesForGraph( - map[string]RouteType{ - "hr-1": RouteTypeHTTP, - "hr-2": RouteTypeHTTP, - "grpc-1": RouteTypeGRPC, - }, - ), - ngfPolicies: map[PolicyKey]*Policy{ - createTestPolicyKey(policyGVK, "gw-policy2"): createPolicy([]string{"gateway2", "gateway3"}, kinds.Gateway), - createTestPolicyKey(policyGVK, "route-policy2"): createPolicy( - []string{"hr-1", "hr-2"}, - kinds.HTTPRoute, - ), - createTestPolicyKey(policyGVK, "grpc-route-policy2"): createPolicy([]string{"grpc-1"}, kinds.GRPCRoute), + name: "nil ReferencedServices; gateway and route policies attach", + routes: getRoutes(), + ngfPolicies: getPolicies(), + gateway: getGateway(), + expects: []func(g *WithT, graph *Graph){ + expectGatewayPolicyAttachment, + expectRoutePolicyAttachment, + expectNoSvcPolicyAttachment, }, - gateway: createGateway("gateway2"), - expect: expectPolicyAttachment, + }, + { + name: "all policies attach", + routes: getRoutes(), + svcs: getServices(), + ngfPolicies: getPolicies(), + gateway: getGateway(), + expects: expectAllAttachmentList, }, } @@ -181,13 +214,16 @@ func TestAttachPolicies(t *testing.T) { g := NewWithT(t) graph := &Graph{ - Gateway: test.gateway, - Routes: test.routes, - NGFPolicies: test.ngfPolicies, + Gateway: test.gateway, + Routes: test.routes, + ReferencedServices: test.svcs, + NGFPolicies: test.ngfPolicies, } graph.attachPolicies("nginx-gateway") - test.expect(g, graph) + for _, expect := range test.expects { + expect(g, graph) + } }) } } @@ -360,15 +396,6 @@ func TestAttachPolicyToGateway(t *testing.T) { } } - getGatewayParentRef := func(gwNsName types.NamespacedName) v1.ParentReference { - return v1.ParentReference{ - Group: helpers.GetPointer[v1.Group](v1.GroupName), - Kind: helpers.GetPointer[v1.Kind]("Gateway"), - Namespace: (*v1.Namespace)(&gwNsName.Namespace), - Name: v1.ObjectName(gwNsName.Name), - } - } - tests := []struct { policy *Policy gw *Gateway @@ -508,6 +535,125 @@ func TestAttachPolicyToGateway(t *testing.T) { } } +func TestAttachPolicyToService(t *testing.T) { + t.Parallel() + + gwNsname := types.NamespacedName{Namespace: testNs, Name: "gateway"} + gw2Nsname := types.NamespacedName{Namespace: testNs, Name: "gateway2"} + + getGateway := func(valid bool) *Gateway { + return &Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: gwNsname.Name, + Namespace: gwNsname.Namespace, + }, + }, + Valid: valid, + } + } + + tests := []struct { + policy *Policy + svc *ReferencedService + gw *Gateway + name string + expAncestors []PolicyAncestor + expAttached bool + }{ + { + name: "attachment", + policy: &Policy{Source: &policiesfakes.FakePolicy{}}, + svc: &ReferencedService{}, + gw: getGateway(true /*valid*/), + expAttached: true, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), + }, + }, + }, + { + name: "attachment; ancestor already exists so don't duplicate", + policy: &Policy{ + Source: &policiesfakes.FakePolicy{}, + Ancestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), + }, + }, + }, + svc: &ReferencedService{}, + gw: getGateway(true /*valid*/), + expAttached: true, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), // only one ancestor per Gateway + }, + }, + }, + { + name: "attachment; ancestor doesn't exists so add it", + policy: &Policy{ + Source: &policiesfakes.FakePolicy{}, + Ancestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gw2Nsname), + }, + }, + }, + svc: &ReferencedService{}, + gw: getGateway(true /*valid*/), + expAttached: true, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gw2Nsname), + }, + { + Ancestor: getGatewayParentRef(gwNsname), + }, + }, + }, + { + name: "no attachment; gateway is invalid", + policy: &Policy{Source: &policiesfakes.FakePolicy{}}, + svc: &ReferencedService{}, + gw: getGateway(false /*invalid*/), + expAttached: false, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gwNsname), + Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("Parent Gateway is invalid")}, + }, + }, + }, + { + name: "no attachment; max ancestor", + policy: &Policy{Source: createTestPolicyWithAncestors(16)}, + svc: &ReferencedService{}, + gw: getGateway(true /*valid*/), + expAttached: false, + expAncestors: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + attachPolicyToService(test.policy, test.svc, test.gw, "ctlr") + if test.expAttached { + g.Expect(test.svc.Policies).To(HaveLen(1)) + } else { + g.Expect(test.svc.Policies).To(BeEmpty()) + } + + g.Expect(test.policy.Ancestors).To(BeEquivalentTo(test.expAncestors)) + }) + } +} + func TestProcessPolicies(t *testing.T) { t.Parallel() policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "MyPolicy"} @@ -518,6 +664,7 @@ func TestProcessPolicies(t *testing.T) { grpcRef := createTestRef(kinds.GRPCRoute, v1.GroupName, "grpc") gatewayRef := createTestRef(kinds.Gateway, v1.GroupName, "gw") ignoredGatewayRef := createTestRef(kinds.Gateway, v1.GroupName, "ignored") + svcRef := createTestRef(kinds.Service, "core", "svc") // These refs reference objects that do not belong to NGF. // Policies that contain these refs should NOT be processed. @@ -525,6 +672,7 @@ func TestProcessPolicies(t *testing.T) { hrWrongGroup := createTestRef(kinds.HTTPRoute, "WrongGroup", "hr") gatewayWrongGroupRef := createTestRef(kinds.Gateway, "WrongGroup", "gw") nonNGFGatewayRef := createTestRef(kinds.Gateway, v1.GroupName, "not-ours") + svcDoesNotExistRef := createTestRef(kinds.Service, "core", "dne") pol1, pol1Key := createTestPolicyAndKey(policyGVK, "pol1", hrRef) pol2, pol2Key := createTestPolicyAndKey(policyGVK, "pol2", grpcRef) @@ -534,6 +682,8 @@ func TestProcessPolicies(t *testing.T) { pol6, pol6Key := createTestPolicyAndKey(policyGVK, "pol6", hrWrongGroup) pol7, pol7Key := createTestPolicyAndKey(policyGVK, "pol7", gatewayWrongGroupRef) pol8, pol8Key := createTestPolicyAndKey(policyGVK, "pol8", nonNGFGatewayRef) + pol9, pol9Key := createTestPolicyAndKey(policyGVK, "pol9", svcDoesNotExistRef) + pol10, pol10Key := createTestPolicyAndKey(policyGVK, "pol10", svcRef) pol1Conflict, pol1ConflictKey := createTestPolicyAndKey(policyGVK, "pol1-conflict", hrRef) @@ -553,14 +703,16 @@ func TestProcessPolicies(t *testing.T) { name: "mix of relevant and irrelevant policies", validator: allValidValidator, policies: map[PolicyKey]policies.Policy{ - pol1Key: pol1, - pol2Key: pol2, - pol3Key: pol3, - pol4Key: pol4, - pol5Key: pol5, - pol6Key: pol6, - pol7Key: pol7, - pol8Key: pol8, + pol1Key: pol1, + pol2Key: pol2, + pol3Key: pol3, + pol4Key: pol4, + pol5Key: pol5, + pol6Key: pol6, + pol7Key: pol7, + pol8Key: pol8, + pol9Key: pol9, + pol10Key: pol10, }, expProcessedPolicies: map[PolicyKey]*Policy{ pol1Key: { @@ -611,6 +763,18 @@ func TestProcessPolicies(t *testing.T) { Ancestors: []PolicyAncestor{}, Valid: true, }, + pol10Key: { + Source: pol10, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "svc"}, + Kind: kinds.Service, + Group: "core", + }, + }, + Ancestors: []PolicyAncestor{}, + Valid: true, + }, }, }, { @@ -740,12 +904,16 @@ func TestProcessPolicies(t *testing.T) { }, } + services := map[types.NamespacedName]*ReferencedService{ + {Namespace: testNs, Name: "svc"}: {}, + } + for _, test := range tests { t.Run(test.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) - processed := processPolicies(test.policies, test.validator, gateways, routes, nil) + processed := processPolicies(test.policies, test.validator, gateways, routes, services, nil) g.Expect(processed).To(BeEquivalentTo(test.expProcessedPolicies)) }) } @@ -885,7 +1053,7 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { t.Parallel() g := NewWithT(t) - processed := processPolicies(test.policies, test.validator, gateways, test.routes, nil) + processed := processPolicies(test.policies, test.validator, gateways, test.routes, nil, nil) g.Expect(processed).To(HaveLen(1)) for _, pol := range processed { @@ -1051,6 +1219,45 @@ func TestMarkConflictedPolicies(t *testing.T) { } } +func TestRefGroupKind(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + group v1.Group + kind v1.Kind + expString string + }{ + { + name: "explicit group core", + group: "core", + kind: kinds.Service, + expString: "core/Service", + }, + { + name: "implicit group core", + group: "", + kind: kinds.Service, + expString: "core/Service", + }, + { + name: "gateway group", + group: v1.GroupName, + kind: kinds.HTTPRoute, + expString: "gateway.networking.k8s.io/HTTPRoute", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(refGroupKind(test.group, test.kind)).To(Equal(test.expString)) + }) + } +} + func createTestPolicyWithAncestors(numAncestors int) policies.Policy { policy := &policiesfakes.FakePolicy{} @@ -1151,3 +1358,12 @@ func createTestRouteWithPaths(name string, paths ...string) *L7Route { return route } + +func getGatewayParentRef(gwNsName types.NamespacedName) v1.ParentReference { + return v1.ParentReference{ + Group: helpers.GetPointer[v1.Group](v1.GroupName), + Kind: helpers.GetPointer[v1.Kind]("Gateway"), + Namespace: (*v1.Namespace)(&gwNsName.Namespace), + Name: v1.ObjectName(gwNsName.Name), + } +} diff --git a/internal/mode/static/state/graph/policy_ancestor.go b/internal/mode/static/state/graph/policy_ancestor.go index 7120f94afa..b55990d6d3 100644 --- a/internal/mode/static/state/graph/policy_ancestor.go +++ b/internal/mode/static/state/graph/policy_ancestor.go @@ -4,6 +4,8 @@ import ( "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" ) const maxAncestors = 16 @@ -61,3 +63,35 @@ func createParentReference( Name: v1.ObjectName(nsname.Name), } } + +func ancestorsContainsAncestorRef(ancestors []PolicyAncestor, ref v1.ParentReference) bool { + for _, an := range ancestors { + if parentRefEqual(an.Ancestor, ref) { + return true + } + } + + return false +} + +func parentRefEqual(ref1, ref2 v1.ParentReference) bool { + if !helpers.EqualPointers(ref1.Kind, ref2.Kind) { + return false + } + + if !helpers.EqualPointers(ref1.Group, ref2.Group) { + return false + } + + if !helpers.EqualPointers(ref1.Namespace, ref2.Namespace) { + return false + } + + // we don't check the other fields in ParentRef because we don't set them + + if ref1.Name != ref2.Name { + return false + } + + return true +} diff --git a/internal/mode/static/state/graph/policy_ancestor_test.go b/internal/mode/static/state/graph/policy_ancestor_test.go index 794caf6c2f..0b34f8e1ef 100644 --- a/internal/mode/static/state/graph/policy_ancestor_test.go +++ b/internal/mode/static/state/graph/policy_ancestor_test.go @@ -4,10 +4,12 @@ import ( "testing" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ) func TestBackendTLSPolicyAncestorsFull(t *testing.T) { @@ -169,3 +171,111 @@ func TestNGFPolicyAncestorsFull(t *testing.T) { }) } } + +func TestAncestorContainsAncestorRef(t *testing.T) { + t.Parallel() + + gw1 := types.NamespacedName{Namespace: testNs, Name: "gw1"} + gw2 := types.NamespacedName{Namespace: testNs, Name: "gw2"} + route := types.NamespacedName{Namespace: testNs, Name: "route"} + newRoute := types.NamespacedName{Namespace: testNs, Name: "new-route"} + + ancestors := []PolicyAncestor{ + { + Ancestor: createParentReference(v1.GroupName, kinds.Gateway, gw1), + }, + { + Ancestor: createParentReference(v1.GroupName, kinds.Gateway, gw2), + }, + { + Ancestor: createParentReference(v1.GroupName, kinds.HTTPRoute, route), + }, + } + + tests := []struct { + ref v1.ParentReference + name string + contains bool + }{ + { + name: "contains Gateway ref", + ref: createParentReference(v1.GroupName, kinds.Gateway, gw1), + contains: true, + }, + { + name: "contains Route ref", + ref: createParentReference(v1.GroupName, kinds.HTTPRoute, route), + contains: true, + }, + { + name: "does not contain ref", + ref: createParentReference(v1.GroupName, kinds.HTTPRoute, newRoute), + contains: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(ancestorsContainsAncestorRef(ancestors, test.ref)).To(Equal(test.contains)) + }) + } +} + +func TestParentRefEqual(t *testing.T) { + t.Parallel() + ref1NsName := types.NamespacedName{Namespace: testNs, Name: "ref1"} + + ref1 := createParentReference(v1.GroupName, kinds.HTTPRoute, ref1NsName) + + tests := []struct { + ref v1.ParentReference + name string + equal bool + }{ + { + name: "kinds different", + ref: createParentReference(v1.GroupName, kinds.Gateway, ref1NsName), + equal: false, + }, + { + name: "groups different", + ref: createParentReference("diff-group", kinds.HTTPRoute, ref1NsName), + equal: false, + }, + { + name: "namespace different", + ref: createParentReference( + v1.GroupName, + kinds.HTTPRoute, + types.NamespacedName{Namespace: "diff-ns", Name: "ref1"}, + ), + equal: false, + }, + { + name: "name different", + ref: createParentReference( + v1.GroupName, + kinds.HTTPRoute, + types.NamespacedName{Namespace: testNs, Name: "diff-name"}, + ), + equal: false, + }, + { + name: "equal", + ref: createParentReference(v1.GroupName, kinds.HTTPRoute, ref1NsName), + equal: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + g.Expect(parentRefEqual(ref1, test.ref)).To(Equal(test.equal)) + }) + } +} diff --git a/internal/mode/static/state/graph/service.go b/internal/mode/static/state/graph/service.go index ad33579f1e..ad6fb817ef 100644 --- a/internal/mode/static/state/graph/service.go +++ b/internal/mode/static/state/graph/service.go @@ -2,17 +2,31 @@ package graph import ( "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" ) +// A ReferencedService represents a Kubernetes Service that is referenced by a Route and that belongs to the +// winning Gateway. It does not contain the v1.Service object, because Services are resolved when building +// the dataplane.Configuration. +type ReferencedService struct { + // Policies is a list of NGF Policies that target this Service. + Policies []*Policy +} + func buildReferencedServices( l7routes map[RouteKey]*L7Route, l4Routes map[L4RouteKey]*L4Route, -) map[types.NamespacedName]struct{} { - svcNames := make(map[types.NamespacedName]struct{}) + gw *Gateway, +) map[types.NamespacedName]*ReferencedService { + if gw == nil { + return nil + } - attached := func(parentRefs []ParentRef) bool { - for _, ref := range parentRefs { - if ref.Attachment.Attached { + referencedServices := make(map[types.NamespacedName]*ReferencedService) + + belongsToWinningGw := func(refs []ParentRef) bool { + for _, ref := range refs { + if ref.Gateway == client.ObjectKeyFromObject(gw.Source) { return true } } @@ -22,21 +36,24 @@ func buildReferencedServices( // Processes both valid and invalid BackendRefs as invalid ones still have referenced services // we may want to track. - - populateServiceNamesForL7Routes := func(routeRules []RouteRule) { + addServicesForL7Routes := func(routeRules []RouteRule) { for _, rule := range routeRules { for _, ref := range rule.BackendRefs { if ref.SvcNsName != (types.NamespacedName{}) { - svcNames[ref.SvcNsName] = struct{}{} + referencedServices[ref.SvcNsName] = &ReferencedService{ + Policies: nil, + } } } } } - populateServiceNamesForL4Routes := func(route *L4Route) { + addServicesForL4Routes := func(route *L4Route) { nsname := route.Spec.BackendRef.SvcNsName if nsname != (types.NamespacedName{}) { - svcNames[nsname] = struct{}{} + referencedServices[nsname] = &ReferencedService{ + Policies: nil, + } } } @@ -48,12 +65,11 @@ func buildReferencedServices( continue } - // If none of the ParentRefs are attached to the Gateway, we want to skip the route. - if !attached(route.ParentRefs) { + if !belongsToWinningGw(route.ParentRefs) { continue } - populateServiceNamesForL7Routes(route.Spec.Rules) + addServicesForL7Routes(route.Spec.Rules) } for _, route := range l4Routes { @@ -61,16 +77,16 @@ func buildReferencedServices( continue } - // If none of the ParentRefs are attached to the Gateway, we want to skip the route. - if !attached(route.ParentRefs) { + if !belongsToWinningGw(route.ParentRefs) { continue } - populateServiceNamesForL4Routes(route) + addServicesForL4Routes(route) } - if len(svcNames) == 0 { + if len(referencedServices) == 0 { return nil } - return svcNames + + return referencedServices } diff --git a/internal/mode/static/state/graph/service_test.go b/internal/mode/static/state/graph/service_test.go index 5c60831a31..0fa316e73f 100644 --- a/internal/mode/static/state/graph/service_test.go +++ b/internal/mode/static/state/graph/service_test.go @@ -4,18 +4,30 @@ import ( "testing" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + v1 "sigs.k8s.io/gateway-api/apis/v1" ) func TestBuildReferencedServices(t *testing.T) { t.Parallel() + + gwNsname := types.NamespacedName{Namespace: "test", Name: "gwNsname"} + gw := &Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: gwNsname.Namespace, + Name: gwNsname.Name, + }, + }, + } + ignoredGw := types.NamespacedName{Namespace: "test", Name: "ignoredGw"} + getNormalL7Route := func() *L7Route { return &L7Route{ ParentRefs: []ParentRef{ { - Attachment: &ParentRefAttachmentStatus{ - Attached: true, - }, + Gateway: gwNsname, }, }, Valid: true, @@ -48,9 +60,7 @@ func TestBuildReferencedServices(t *testing.T) { Valid: true, ParentRefs: []ParentRef{ { - Attachment: &ParentRefAttachmentStatus{ - Attached: true, - }, + Gateway: gwNsname, }, }, } @@ -117,111 +127,102 @@ func TestBuildReferencedServices(t *testing.T) { return route }) - unattachedRoute := getModifiedL7Route(func(route *L7Route) *L7Route { - route.ParentRefs[0].Attachment.Attached = false + validRouteNoServiceNsName := getModifiedL7Route(func(route *L7Route) *L7Route { + route.Spec.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} return route }) - unattachedL4Route := getModifiedL4Route(func(route *L4Route) *L4Route { - route.ParentRefs[0].Attachment.Attached = false + validL4RouteNoServiceNsName := getModifiedL4Route(func(route *L4Route) *L4Route { + route.Spec.BackendRef.SvcNsName = types.NamespacedName{} return route }) - attachedRouteWithManyParentRefs := getModifiedL7Route(func(route *L7Route) *L7Route { + normalL4RouteWinningAndIgnoredGws := getModifiedL4Route(func(route *L4Route) *L4Route { route.ParentRefs = []ParentRef{ { - Attachment: &ParentRefAttachmentStatus{ - Attached: false, - }, + Gateway: ignoredGw, }, { - Attachment: &ParentRefAttachmentStatus{ - Attached: false, - }, + Gateway: ignoredGw, }, { - Attachment: &ParentRefAttachmentStatus{ - Attached: true, - }, + Gateway: gwNsname, }, } - return route }) - attachedL4RoutesWithManyParentRefs := getModifiedL4Route(func(route *L4Route) *L4Route { + normalRouteWinningAndIgnoredGws := getModifiedL7Route(func(route *L7Route) *L7Route { route.ParentRefs = []ParentRef{ { - Attachment: &ParentRefAttachmentStatus{ - Attached: false, - }, + Gateway: ignoredGw, }, { - Attachment: &ParentRefAttachmentStatus{ - Attached: true, - }, + Gateway: gwNsname, }, { - Attachment: &ParentRefAttachmentStatus{ - Attached: false, - }, + Gateway: ignoredGw, }, } - return route }) - validRouteNoServiceNsName := getModifiedL7Route(func(route *L7Route) *L7Route { - route.Spec.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + normalL4RouteIgnoredGw := getModifiedL4Route(func(route *L4Route) *L4Route { + route.ParentRefs[0].Gateway = ignoredGw return route }) - validL4RouteNoServiceNsName := getModifiedL4Route(func(route *L4Route) *L4Route { - route.Spec.BackendRef.SvcNsName = types.NamespacedName{} + normalL7RouteIgnoredGw := getModifiedL7Route(func(route *L7Route) *L7Route { + route.ParentRefs[0].Gateway = ignoredGw return route }) tests := []struct { l7Routes map[RouteKey]*L7Route l4Routes map[L4RouteKey]*L4Route - exp map[types.NamespacedName]struct{} + exp map[types.NamespacedName]*ReferencedService + gw *Gateway name string }{ { name: "normal routes", + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "normal-route"}}: normalRoute, }, l4Routes: map[L4RouteKey]*L4Route{ {NamespacedName: types.NamespacedName{Name: "normal-l4-route"}}: normalL4Route, }, - exp: map[types.NamespacedName]struct{}{ + exp: map[types.NamespacedName]*ReferencedService{ {Namespace: "banana-ns", Name: "service"}: {}, {Namespace: "tlsroute-ns", Name: "service"}: {}, }, }, { name: "l7 route with two services in one Rule", // l4 routes don't support multiple services right now + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "two-svc-one-rule"}}: validRouteTwoServicesOneRule, }, - exp: map[types.NamespacedName]struct{}{ + exp: map[types.NamespacedName]*ReferencedService{ {Namespace: "service-ns", Name: "service"}: {}, {Namespace: "service-ns2", Name: "service2"}: {}, }, }, { name: "route with one service per rule", // l4 routes don't support multiple rules right now + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "one-svc-per-rule"}}: validRouteTwoServicesTwoRules, }, - exp: map[types.NamespacedName]struct{}{ + exp: map[types.NamespacedName]*ReferencedService{ {Namespace: "service-ns", Name: "service"}: {}, {Namespace: "service-ns2", Name: "service2"}: {}, }, }, { name: "multiple valid routes with same services", + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "one-svc-per-rule"}}: validRouteTwoServicesTwoRules, {NamespacedName: types.NamespacedName{Name: "two-svc-one-rule"}}: validRouteTwoServicesOneRule, @@ -231,15 +232,41 @@ func TestBuildReferencedServices(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "l4-route-2"}}: normalL4Route2, {NamespacedName: types.NamespacedName{Name: "l4-route-same-svc-as-l7-route"}}: normalL4RouteWithSameSvcAsL7Route, }, - exp: map[types.NamespacedName]struct{}{ + exp: map[types.NamespacedName]*ReferencedService{ {Namespace: "service-ns", Name: "service"}: {}, {Namespace: "service-ns2", Name: "service2"}: {}, {Namespace: "tlsroute-ns", Name: "service"}: {}, {Namespace: "tlsroute-ns", Name: "service2"}: {}, }, }, + { + name: "valid routes that do not belong to winning gateway", + gw: gw, + l7Routes: map[RouteKey]*L7Route{ + {NamespacedName: types.NamespacedName{Name: "belongs-to-ignored-gws"}}: normalL7RouteIgnoredGw, + }, + l4Routes: map[L4RouteKey]*L4Route{ + {NamespacedName: types.NamespacedName{Name: "belongs-to-ignored-gw"}}: normalL4RouteIgnoredGw, + }, + exp: nil, + }, + { + name: "valid routes that belong to both winning and ignored gateways", + gw: gw, + l7Routes: map[RouteKey]*L7Route{ + {NamespacedName: types.NamespacedName{Name: "belongs-to-ignored-gws"}}: normalRouteWinningAndIgnoredGws, + }, + l4Routes: map[L4RouteKey]*L4Route{ + {NamespacedName: types.NamespacedName{Name: "ignored-gw"}}: normalL4RouteWinningAndIgnoredGws, + }, + exp: map[types.NamespacedName]*ReferencedService{ + {Namespace: "banana-ns", Name: "service"}: {}, + {Namespace: "tlsroute-ns", Name: "service"}: {}, + }, + }, { name: "valid routes with different services", + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "one-svc-per-rule"}}: validRouteTwoServicesTwoRules, {NamespacedName: types.NamespacedName{Name: "normal-route"}}: normalRoute, @@ -247,7 +274,7 @@ func TestBuildReferencedServices(t *testing.T) { l4Routes: map[L4RouteKey]*L4Route{ {NamespacedName: types.NamespacedName{Name: "normal-l4-route"}}: normalL4Route, }, - exp: map[types.NamespacedName]struct{}{ + exp: map[types.NamespacedName]*ReferencedService{ {Namespace: "service-ns", Name: "service"}: {}, {Namespace: "service-ns2", Name: "service2"}: {}, {Namespace: "banana-ns", Name: "service"}: {}, @@ -256,6 +283,7 @@ func TestBuildReferencedServices(t *testing.T) { }, { name: "invalid routes", + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "invalid-route"}}: invalidRoute, }, @@ -264,18 +292,9 @@ func TestBuildReferencedServices(t *testing.T) { }, exp: nil, }, - { - name: "unattached route", - l7Routes: map[RouteKey]*L7Route{ - {NamespacedName: types.NamespacedName{Name: "unattached-route"}}: unattachedRoute, - }, - l4Routes: map[L4RouteKey]*L4Route{ - {NamespacedName: types.NamespacedName{Name: "unattached-l4-route"}}: unattachedL4Route, - }, - exp: nil, - }, { name: "combination of valid and invalid routes", + gw: gw, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "normal-route"}}: normalRoute, {NamespacedName: types.NamespacedName{Name: "invalid-route"}}: invalidRoute, @@ -284,26 +303,25 @@ func TestBuildReferencedServices(t *testing.T) { {NamespacedName: types.NamespacedName{Name: "invalid-l4-route"}}: invalidL4Route, {NamespacedName: types.NamespacedName{Name: "normal-l4-route"}}: normalL4Route, }, - exp: map[types.NamespacedName]struct{}{ + exp: map[types.NamespacedName]*ReferencedService{ {Namespace: "banana-ns", Name: "service"}: {}, {Namespace: "tlsroute-ns", Name: "service"}: {}, }, }, { - name: "route with many parentRefs and one is attached", + name: "valid route no service nsname", + gw: gw, l7Routes: map[RouteKey]*L7Route{ - {NamespacedName: types.NamespacedName{Name: "multiple-parent-ref-route"}}: attachedRouteWithManyParentRefs, + {NamespacedName: types.NamespacedName{Name: "no-service-nsname"}}: validRouteNoServiceNsName, }, l4Routes: map[L4RouteKey]*L4Route{ - {NamespacedName: types.NamespacedName{Name: "multiple-parent-ref-l4-route"}}: attachedL4RoutesWithManyParentRefs, - }, - exp: map[types.NamespacedName]struct{}{ - {Namespace: "banana-ns", Name: "service"}: {}, - {Namespace: "tlsroute-ns", Name: "service"}: {}, + {NamespacedName: types.NamespacedName{Name: "no-service-nsname-l4"}}: validL4RouteNoServiceNsName, }, + exp: nil, }, { - name: "valid route no service nsname", + name: "nil gateway", + gw: nil, l7Routes: map[RouteKey]*L7Route{ {NamespacedName: types.NamespacedName{Name: "no-service-nsname"}}: validRouteNoServiceNsName, }, @@ -318,7 +336,8 @@ func TestBuildReferencedServices(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) - g.Expect(buildReferencedServices(test.l7Routes, test.l4Routes)).To(Equal(test.exp)) + + g.Expect(buildReferencedServices(test.l7Routes, test.l4Routes, test.gw)).To(Equal(test.exp)) }) } } diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index cf58cc3d20..d809ab342c 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -97,6 +97,8 @@ type NGFResourceCounts struct { NginxProxyCount int64 // SnippetsFilterCount is the number of SnippetsFilters. SnippetsFilterCount int64 + // UpstreamSettingsPolicyCount is the number of UpstreamSettingsPolicies. + UpstreamSettingsPolicyCount int64 } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. @@ -239,6 +241,8 @@ func collectGraphResourceCount( } case kinds.ObservabilityPolicy: ngfResourceCounts.ObservabilityPolicyCount++ + case kinds.UpstreamSettingsPolicy: + ngfResourceCounts.UpstreamSettingsPolicyCount++ } } diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 59bee05aae..5a7b11b582 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -302,7 +302,7 @@ var _ = Describe("Collector", Ordered, func() { }, client.ObjectKeyFromObject(nilsecret): nil, }, - ReferencedServices: map[types.NamespacedName]struct{}{ + ReferencedServices: map[types.NamespacedName]*graph.ReferencedService{ client.ObjectKeyFromObject(svc1): {}, client.ObjectKeyFromObject(svc2): {}, client.ObjectKeyFromObject(nilsvc): {}, @@ -329,6 +329,10 @@ var _ = Describe("Collector", Ordered, func() { NsName: types.NamespacedName{Namespace: "test", Name: "ObservabilityPolicy-1"}, GVK: schema.GroupVersionKind{Kind: kinds.ObservabilityPolicy}, }: {}, + { + NsName: types.NamespacedName{Namespace: "test", Name: "UpstreamSettingsPolicy-1"}, + GVK: schema.GroupVersionKind{Kind: kinds.UpstreamSettingsPolicy}, + }: {}, }, NginxProxy: &graph.NginxProxy{}, SnippetsFilters: map[types.NamespacedName]*graph.SnippetsFilter{ @@ -412,6 +416,7 @@ var _ = Describe("Collector", Ordered, func() { ObservabilityPolicyCount: 1, NginxProxyCount: 1, SnippetsFilterCount: 3, + UpstreamSettingsPolicyCount: 1, } expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "kind" @@ -583,7 +588,7 @@ var _ = Describe("Collector", Ordered, func() { Source: secret, }, }, - ReferencedServices: map[types.NamespacedName]struct{}{ + ReferencedServices: map[types.NamespacedName]*graph.ReferencedService{ client.ObjectKeyFromObject(svc): {}, }, NGFPolicies: map[graph.PolicyKey]*graph.Policy{ @@ -603,6 +608,10 @@ var _ = Describe("Collector", Ordered, func() { NsName: types.NamespacedName{Namespace: "test", Name: "ObservabilityPolicy-1"}, GVK: schema.GroupVersionKind{Kind: kinds.ObservabilityPolicy}, }: {}, + { + NsName: types.NamespacedName{Namespace: "test", Name: "UpstreamSettingsPolicy-1"}, + GVK: schema.GroupVersionKind{Kind: kinds.UpstreamSettingsPolicy}, + }: {}, }, NginxProxy: &graph.NginxProxy{}, SnippetsFilters: map[types.NamespacedName]*graph.SnippetsFilter{ @@ -682,6 +691,7 @@ var _ = Describe("Collector", Ordered, func() { ObservabilityPolicyCount: 1, NginxProxyCount: 1, SnippetsFilterCount: 1, + UpstreamSettingsPolicyCount: 1, } data, err := dataCollector.Collect(ctx) @@ -693,22 +703,7 @@ var _ = Describe("Collector", Ordered, func() { It("ignores invalid and empty upstreams", func(ctx SpecContext) { fakeGraphGetter.GetLatestGraphReturns(&graph.Graph{}) fakeConfigurationGetter.GetLatestConfigurationReturns(invalidUpstreamsConfig) - expData.NGFResourceCounts = telemetry.NGFResourceCounts{ - GatewayCount: 0, - GatewayClassCount: 0, - HTTPRouteCount: 0, - TLSRouteCount: 0, - SecretCount: 0, - ServiceCount: 0, - EndpointCount: 0, - GRPCRouteCount: 0, - BackendTLSPolicyCount: 0, - GatewayAttachedClientSettingsPolicyCount: 0, - RouteAttachedClientSettingsPolicyCount: 0, - ObservabilityPolicyCount: 0, - NginxProxyCount: 0, - SnippetsFilterCount: 0, - } + expData.NGFResourceCounts = telemetry.NGFResourceCounts{} data, err := dataCollector.Collect(ctx) diff --git a/internal/mode/static/telemetry/data.avdl b/internal/mode/static/telemetry/data.avdl index 71515e0e53..6909878866 100644 --- a/internal/mode/static/telemetry/data.avdl +++ b/internal/mode/static/telemetry/data.avdl @@ -99,6 +99,9 @@ attached at the Gateway level. */ /** SnippetsFilterCount is the number of SnippetsFilters. */ long? SnippetsFilterCount = null; + /** UpstreamSettingsPolicyCount is the number of UpstreamSettingsPolicies. */ + long? UpstreamSettingsPolicyCount = null; + /** NGFReplicaCount is the number of replicas of the NGF Pod. */ long? NGFReplicaCount = null; diff --git a/internal/mode/static/telemetry/data_test.go b/internal/mode/static/telemetry/data_test.go index cb8f084b39..d0fe63820a 100644 --- a/internal/mode/static/telemetry/data_test.go +++ b/internal/mode/static/telemetry/data_test.go @@ -39,6 +39,7 @@ func TestDataAttributes(t *testing.T) { ObservabilityPolicyCount: 11, NginxProxyCount: 12, SnippetsFilterCount: 13, + UpstreamSettingsPolicyCount: 14, }, NGFReplicaCount: 3, SnippetsFiltersDirectives: []string{"main-three-count", "http-two-count", "server-one-count"}, @@ -77,6 +78,7 @@ func TestDataAttributes(t *testing.T) { attribute.Int64("ObservabilityPolicyCount", 11), attribute.Int64("NginxProxyCount", 12), attribute.Int64("SnippetsFilterCount", 13), + attribute.Int64("UpstreamSettingsPolicyCount", 14), attribute.Int64("NGFReplicaCount", 3), } @@ -119,6 +121,7 @@ func TestDataAttributesWithEmptyData(t *testing.T) { attribute.Int64("ObservabilityPolicyCount", 0), attribute.Int64("NginxProxyCount", 0), attribute.Int64("SnippetsFilterCount", 0), + attribute.Int64("UpstreamSettingsPolicyCount", 0), attribute.Int64("NGFReplicaCount", 0), } diff --git a/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go b/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go index 318cbd0b62..343c6fd9ed 100644 --- a/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go +++ b/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go @@ -26,6 +26,7 @@ func (d *NGFResourceCounts) Attributes() []attribute.KeyValue { attrs = append(attrs, attribute.Int64("ObservabilityPolicyCount", d.ObservabilityPolicyCount)) attrs = append(attrs, attribute.Int64("NginxProxyCount", d.NginxProxyCount)) attrs = append(attrs, attribute.Int64("SnippetsFilterCount", d.SnippetsFilterCount)) + attrs = append(attrs, attribute.Int64("UpstreamSettingsPolicyCount", d.UpstreamSettingsPolicyCount)) return attrs } diff --git a/site/content/overview/product-telemetry.md b/site/content/overview/product-telemetry.md index 237abb1303..701774547d 100644 --- a/site/content/overview/product-telemetry.md +++ b/site/content/overview/product-telemetry.md @@ -27,7 +27,7 @@ Telemetry data is collected once every 24 hours and sent to a service managed by - **Deployment Replica Count:** the count of NGINX Gateway Fabric Pods. - **Image Build Source:** whether the image was built by GitHub or locally (values are `gha`, `local`, or `unknown`). The source repository of the images is **not** collected. - **Deployment Flags:** a list of NGINX Gateway Fabric Deployment flags that are specified by a user. The actual values of non-boolean flags are **not** collected; we only record that they are either `true` or `false` for boolean flags and `default` or `user-defined` for the rest. -- **Count of Resources:** the total count of resources related to NGINX Gateway Fabric. This includes `GatewayClasses`, `Gateways`, `HTTPRoutes`,`GRPCRoutes`, `TLSRoutes`, `Secrets`, `Services`, `BackendTLSPolicies`, `ClientSettingsPolicies`, `NginxProxies`, `ObservabilityPolicies`, `SnippetsFilters`, and `Endpoints`. The data within these resources is **not** collected. +- **Count of Resources:** the total count of resources related to NGINX Gateway Fabric. This includes `GatewayClasses`, `Gateways`, `HTTPRoutes`,`GRPCRoutes`, `TLSRoutes`, `Secrets`, `Services`, `BackendTLSPolicies`, `ClientSettingsPolicies`, `NginxProxies`, `ObservabilityPolicies`, `UpstreamSettingsPolicies`, `SnippetsFilters`, and `Endpoints`. The data within these resources is **not** collected. - **SnippetsFilters Info**a list of directive-context strings from applied SnippetFilters and a total count per strings. The actual value of any NGINX directive is **not** collected. This data is used to identify the following information: diff --git a/site/content/reference/api.md b/site/content/reference/api.md index f74d7f984f..74ea08d3c8 100644 --- a/site/content/reference/api.md +++ b/site/content/reference/api.md @@ -27,6 +27,8 @@ Resource Types: ObservabilityPolicy
  • SnippetsFilter +
  • +UpstreamSettingsPolicy

    • ClientSettingsPolicy @@ -576,6 +578,131 @@ SnippetsFilterStatus +

    UpstreamSettingsPolicy + +

    +

    +

    UpstreamSettingsPolicy is a Direct Attached Policy. It provides a way to configure the behavior of +the connection between NGINX and the upstream applications.

    +

    + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    FieldDescription
    +apiVersion
    +string    		 + +gateway.nginx.org/v1alpha1 + +
    +kind
    +string +    		UpstreamSettingsPolicy
    +metadata
    + + +Kubernetes meta/v1.ObjectMeta + + +    		 +Refer to the Kubernetes API documentation for the fields of the +metadata field. +
    +spec
    + + +UpstreamSettingsPolicySpec + + +    		 +

    Spec defines the desired state of the UpstreamSettingsPolicy.

    +
    +
    + + + + + + + + + + + + + +
    +zoneSize
    + + +Size + + +    		 +(Optional) +

    ZoneSize is the size of the shared memory zone used by the upstream. This memory zone is used to share +the upstream configuration between nginx worker processes. The more servers that an upstream has, +the larger memory zone is required. +Default: OSS: 512k, Plus: 1m. +Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#zone

    +
    +keepAlive
    + + +UpstreamKeepAlive + + +    		 +(Optional) +

    KeepAlive defines the keep-alive settings.

    +
    +targetRefs
    + + +[]sigs.k8s.io/gateway-api/apis/v1alpha2.LocalPolicyTargetReference + + +    		 +

    TargetRefs identifies API object(s) to apply the policy to. +Objects must be in the same namespace as the policy. +Support: Service

    +
    +
    +status
    + + +sigs.k8s.io/gateway-api/apis/v1alpha2.PolicyStatus + + +    		 +

    Status defines the state of the UpstreamSettingsPolicy.

    +

    Address

    @@ -974,7 +1101,8 @@ longer necessary.

    ClientBody, ClientKeepAlive, ClientKeepAliveTimeout, -TelemetryExporter) +TelemetryExporter, +UpstreamKeepAlive)

    Duration is a string value representing a duration in time. @@ -1521,7 +1649,8 @@ IP address in the X-Forwarded-For HTTP header.

    (Appears on: -ClientBody) +ClientBody, +UpstreamSettingsPolicySpec)

    Size is a string value representing a size. Size can be specified in bytes, kilobytes (k), megabytes (m), @@ -2018,6 +2147,153 @@ Examples of invalid names: some-$value, quoted-“value”-name, unescap +

    UpstreamKeepAlive + +

    +

    +(Appears on: +UpstreamSettingsPolicySpec) +

    +

    +

    UpstreamKeepAlive defines the keep-alive settings for upstreams.

    +

    + + + + + + + + + + + + + + + + + + + + + + + + + +
    FieldDescription
    +connections
    + +int32 + +    		 +(Optional) +

    Connections sets the maximum number of idle keep-alive connections to upstream servers that are preserved +in the cache of each nginx worker process. When this number is exceeded, the least recently used +connections are closed. +Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive

    +
    +requests
    + +int32 + +    		 +(Optional) +

    Requests sets the maximum number of requests that can be served through one keep-alive connection. +After the maximum number of requests are made, the connection is closed. +Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_requests

    +
    +time
    + + +Duration + + +    		 +(Optional) +

    Time defines the maximum time during which requests can be processed through one keep-alive connection. +After this time is reached, the connection is closed following the subsequent request processing. +Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_time

    +
    +timeout
    + + +Duration + + +    		 +(Optional) +

    Timeout defines the keep-alive timeout for upstreams. +Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive_timeout

    +
    +

    UpstreamSettingsPolicySpec + +

    +

    +(Appears on: +UpstreamSettingsPolicy) +

    +

    +

    UpstreamSettingsPolicySpec defines the desired state of the UpstreamSettingsPolicy.

    +

    + + + + + + + + + + + + + + + + + + + + + +
    FieldDescription
    +zoneSize
    + + +Size + + +    		 +(Optional) +

    ZoneSize is the size of the shared memory zone used by the upstream. This memory zone is used to share +the upstream configuration between nginx worker processes. The more servers that an upstream has, +the larger memory zone is required. +Default: OSS: 512k, Plus: 1m. +Directive: https://nginx.org/en/docs/http/ngx_http_upstream_module.html#zone

    +
    +keepAlive
    + + +UpstreamKeepAlive + + +    		 +(Optional) +

    KeepAlive defines the keep-alive settings.

    +
    +targetRefs
    + + +[]sigs.k8s.io/gateway-api/apis/v1alpha2.LocalPolicyTargetReference + + +    		 +

    TargetRefs identifies API object(s) to apply the policy to. +Objects must be in the same namespace as the policy. +Support: Service

    +

    Generated with gen-crd-api-reference-docs diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index b6925dc8e1..81f47e3567 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -28,8 +28,10 @@ type ExpectedNginxField struct { File string // Location is the location name that the directive should exist in. Location string - // Servers are the server names that the directive should exist in. - Servers []string + // Server is the server name that the directive should exist in. + Server string + // Upstream is the upstream name that the directive should exist in. + Upstream string // ValueSubstringAllowed allows the expected value to be a substring of the real value. // This makes it easier for cases when real values are complex file names or contain things we // don't care about, and we just want to check if a substring exists. @@ -39,40 +41,66 @@ type ExpectedNginxField struct { // ValidateNginxFieldExists accepts the nginx config and the configuration for the expected field, // and returns whether or not that field exists where it should. func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) error { + b, err := json.Marshal(conf) + if err != nil { + return fmt.Errorf("error marshaling nginx config: %w", err) + } + for _, config := range conf.Config { if !strings.Contains(config.File, expFieldCfg.File) { continue } for _, directive := range config.Parsed { - if len(expFieldCfg.Servers) == 0 { + if expFieldCfg.Server == "" && expFieldCfg.Upstream == "" { if expFieldCfg.fieldFound(directive) { return nil } continue } - for _, serverName := range expFieldCfg.Servers { - if directive.Directive == "server" && getServerName(directive.Block) == serverName { - for _, serverDirective := range directive.Block { - if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { - return nil - } else if serverDirective.Directive == "location" && - fieldExistsInLocation(serverDirective, expFieldCfg) { - return nil - } - } - } + if expFieldCfg.Server != "" && fieldExistsInServer(expFieldCfg, *directive) { + return nil + } + + if expFieldCfg.Upstream != "" && fieldExistsInUpstream(expFieldCfg, *directive) { + return nil } } } - b, err := json.Marshal(conf) - if err != nil { - return fmt.Errorf("error marshaling nginx config: %w", err) + return fmt.Errorf("directive %s not found in: nginx config %s", expFieldCfg.Directive, string(b)) +} + +func fieldExistsInServer( + expFieldCfg ExpectedNginxField, + directive Directive, +) bool { + if directive.Directive == "server" && getServerName(directive.Block) == expFieldCfg.Server { + for _, serverDirective := range directive.Block { + if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { + return true + } else if serverDirective.Directive == "location" && + fieldExistsInLocation(serverDirective, expFieldCfg) { + return true + } + } } + return false +} - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, string(b)) +func fieldExistsInUpstream( + expFieldCfg ExpectedNginxField, + directive Directive, +) bool { + if directive.Directive == "upstream" && directive.Args[0] == expFieldCfg.Upstream { + for _, directive := range directive.Block { + if expFieldCfg.fieldFound(directive) { + return true + } + } + } + return false } func getServerName(serverBlock Directives) string { diff --git a/tests/suite/client_settings_test.go b/tests/suite/client_settings_test.go index b5e5a8ec48..f7ab05c88f 100644 --- a/tests/suite/client_settings_test.go +++ b/tests/suite/client_settings_test.go @@ -117,7 +117,13 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_gw-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"*.example.com", "cafe.example.com"}, + Server: "*.example.com", + }, + { + Directive: "include", + Value: fmt.Sprintf("%s_gw-csp.conf", filePrefix), + File: "http.conf", + Server: "cafe.example.com", }, { Directive: "client_max_body_size", @@ -150,7 +156,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_coffee-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", Location: "/coffee", }, { @@ -164,7 +170,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_tea-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", Location: "/tea", }, { @@ -178,7 +184,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_soda-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", Location: "/soda", }, { @@ -192,7 +198,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_grpc-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"*.example.com"}, + Server: "*.example.com", Location: "/helloworld.Greeter/SayHello", }, { diff --git a/tests/suite/manifests/upstream-settings-policy/cafe.yaml b/tests/suite/manifests/upstream-settings-policy/cafe.yaml new file mode 100644 index 0000000000..c0466158f8 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/cafe.yaml @@ -0,0 +1,66 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: coffee +spec: + replicas: 1 + selector: + matchLabels: + app: coffee + template: + metadata: + labels: + app: coffee + spec: + containers: + - name: coffee + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: coffee +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: coffee +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tea +spec: + replicas: 1 + selector: + matchLabels: + app: tea + template: + metadata: + labels: + app: tea + spec: + containers: + - name: tea + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: tea +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: tea +--- diff --git a/tests/suite/manifests/upstream-settings-policy/gateway.yaml b/tests/suite/manifests/upstream-settings-policy/gateway.yaml new file mode 100644 index 0000000000..e6507f613b --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/gateway.yaml @@ -0,0 +1,11 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway +spec: + gatewayClassName: nginx + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: "*.example.com" diff --git a/tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml b/tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml new file mode 100644 index 0000000000..3ae352f573 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml @@ -0,0 +1,39 @@ +apiVersion: v1 +kind: Service +metadata: + name: grpc-backend +spec: + selector: + app: grpc-backend + ports: + - protocol: TCP + port: 8080 + targetPort: 50051 +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: grpc-backend + labels: + app: grpc-backend +spec: + replicas: 1 + selector: + matchLabels: + app: grpc-backend + template: + metadata: + labels: + app: grpc-backend + spec: + containers: + - name: grpc-backend + image: ghcr.io/nginxinc/kic-test-grpc-server:0.2.3 + env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + resources: + requests: + cpu: 10m diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml new file mode 100644 index 0000000000..cc1ed6c55d --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml @@ -0,0 +1,15 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: usps-target-not-found +spec: + zoneSize: 512k + targetRefs: + - group: core + kind: Service + name: targeted-svc-dne + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml new file mode 100644 index 0000000000..8bbd25366e --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml @@ -0,0 +1,81 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway-not-valid +spec: + gatewayClassName: nginx + addresses: + - value: "10.0.0.1" + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: "soda.example.com" +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: soda +spec: + replicas: 1 + selector: + matchLabels: + app: soda + template: + metadata: + labels: + app: soda + spec: + containers: + - name: soda + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: soda +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: soda +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: soda +spec: + parentRefs: + - name: gateway-not-valid + sectionName: http + hostnames: + - "soda.example.com" + rules: + - matches: + - path: + type: Exact + value: /soda + backendRefs: + - name: soda + port: 80 +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: soda-svc-usp +spec: + zoneSize: 512k + targetRefs: + - group: core + kind: Service + name: soda + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s diff --git a/tests/suite/manifests/upstream-settings-policy/routes.yaml b/tests/suite/manifests/upstream-settings-policy/routes.yaml new file mode 100644 index 0000000000..f26e705a40 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/routes.yaml @@ -0,0 +1,54 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: coffee +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: PathPrefix + value: /coffee + backendRefs: + - name: coffee + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: tea +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: Exact + value: /tea + backendRefs: + - name: tea + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: GRPCRoute +metadata: + name: grpc-route +spec: + parentRefs: + - name: gateway + sectionName: http + rules: + - matches: + - method: + service: helloworld.Greeter + method: SayHello + backendRefs: + - name: grpc-backend + port: 8080 diff --git a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml new file mode 100644 index 0000000000..7c713b4752 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml @@ -0,0 +1,60 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: merge-usp-1 +spec: + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + time: 1m + timeout: 5h +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: merge-usp-2 +spec: + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + connections: 100 + requests: 55 +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: z-merge-usp-3 +spec: + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + connections: 11 + requests: 15 +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: z-usp +spec: + zoneSize: 64k + targetRefs: + - group: core + kind: Service + name: tea +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: a-usp-wins +spec: + zoneSize: 128k + targetRefs: + - group: core + kind: Service + name: tea diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml new file mode 100644 index 0000000000..cb4e4bf058 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml @@ -0,0 +1,33 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: multiple-http-svc-usp +spec: + targetRefs: + - group: core + kind: Service + name: coffee + - group: core + kind: Service + name: tea + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: grpc-svc-usp +spec: + targetRefs: + - group: core + kind: Service + name: grpc-backend + zoneSize: 64k + keepAlive: + connections: 100 + requests: 45 + time: 1m + timeout: 5h diff --git a/tests/suite/snippets_filter_test.go b/tests/suite/snippets_filter_test.go index 632a8199e0..e92e44d0ad 100644 --- a/tests/suite/snippets_filter_test.go +++ b/tests/suite/snippets_filter_test.go @@ -149,7 +149,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter { Directive: "include", Value: fmt.Sprintf("%s%s", httpServerContext, httpRouteSuffix), - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", File: "http.conf", }, { @@ -157,7 +157,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Value: fmt.Sprintf("%s%s", httpServerLocationContext, httpRouteSuffix), File: "http.conf", Location: "/coffee", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", }, { Directive: "keepalive_time", @@ -194,7 +194,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter { Directive: "include", Value: fmt.Sprintf("%s%s", httpServerContext, grpcRouteSuffix), - Servers: []string{"*.example.com"}, + Server: "*.example.com", File: "http.conf", }, { @@ -207,7 +207,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Value: fmt.Sprintf("%s%s", httpServerLocationContext, grpcRouteSuffix), File: "http.conf", Location: "/helloworld.Greeter/SayHello", - Servers: []string{"*.example.com"}, + Server: "*.example.com", }, }), ) diff --git a/tests/suite/telemetry_test.go b/tests/suite/telemetry_test.go index ac8f84267c..011b3d312e 100644 --- a/tests/suite/telemetry_test.go +++ b/tests/suite/telemetry_test.go @@ -88,6 +88,7 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( "ObservabilityPolicyCount: Int(0)", "NginxProxyCount: Int(0)", "SnippetsFilterCount: Int(0)", + "UpstreamSettingsPolicyCount: Int(0)", "NGFReplicaCount: Int(1)", }, ) diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go new file mode 100644 index 0000000000..fcc4e6ef42 --- /dev/null +++ b/tests/suite/upstream_settings_test.go @@ -0,0 +1,528 @@ +package main + +import ( + "context" + "errors" + "fmt" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + core "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/tests/framework" +) + +var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolicy"), func() { + var ( + files = []string{ + "upstream-settings-policy/cafe.yaml", + "upstream-settings-policy/gateway.yaml", + "upstream-settings-policy/grpc-backend.yaml", + "upstream-settings-policy/routes.yaml", + } + + namespace = "uspolicy" + gatewayName = "gateway" + ) + + zoneSize := "512k" + if *plusEnabled { + zoneSize = "1m" + } + + BeforeAll(func() { + ns := &core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + } + + Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + }) + + AfterAll(func() { + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) + }) + + When("UpstreamSettingsPolicies target distinct Services", func() { + usps := []string{ + "upstream-settings-policy/valid-usps.yaml", + } + + BeforeAll(func() { + Expect(resourceManager.ApplyFromFiles(usps, namespace)).To(Succeed()) + }) + + AfterAll(func() { + Expect(resourceManager.DeleteFromFiles(usps, namespace)).To(Succeed()) + }) + + Specify("they are accepted", func() { + usPolicies := []string{ + "multiple-http-svc-usp", + "grpc-svc-usp", + } + + for _, name := range usPolicies { + uspolicyNsName := types.NamespacedName{Name: name, Namespace: namespace} + + err := waitForUSPolicyStatus( + uspolicyNsName, + gatewayName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + ) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name)) + } + }) + + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoutes", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") + + Eventually( + func() error { + return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + Eventually( + func() error { + return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + }) + }) + + Context("nginx directives", func() { + var conf *framework.Payload + + BeforeAll(func() { + podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(podNames).To(HaveLen(1)) + + ngfPodName := podNames[0] + + conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + Expect(err).ToNot(HaveOccurred()) + }) + + DescribeTable("are set properly for", + func(expCfgs []framework.ExpectedNginxField) { + for _, expCfg := range expCfgs { + Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) + } + }, + Entry("HTTP upstreams", []framework.ExpectedNginxField{ + { + Directive: "upstream", + Value: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "upstream", + Value: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "zone", + Value: fmt.Sprintf("uspolicy_coffee_80 %s", zoneSize), + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "zone", + Value: fmt.Sprintf("uspolicy_tea_80 %s", zoneSize), + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "10", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "10", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "3", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "3", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "10s", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "10s", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "50s", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "50s", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + }), + Entry("GRPC upstreams", []framework.ExpectedNginxField{ + { + Directive: "upstream", + Value: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, + { + Directive: "zone", + Value: "uspolicy_grpc-backend_8080 64k", + Upstream: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "100", + Upstream: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "45", + Upstream: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "1m", + Upstream: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "5h", + Upstream: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, + }), + ) + }) + }) + + When("multiple UpstreamSettingsPolicies with overlapping settings target the same Service", func() { + usps := []string{ + "upstream-settings-policy/valid-merge-usps.yaml", + } + + BeforeAll(func() { + Expect(resourceManager.ApplyFromFiles(usps, namespace)).To(Succeed()) + }) + + AfterAll(func() { + Expect(resourceManager.DeleteFromFiles(usps, namespace)).To(Succeed()) + }) + + DescribeTable("upstreamSettingsPolicy status is set as expected", + func(name string, status metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason) { + uspolicyNsName := types.NamespacedName{Name: name, Namespace: namespace} + Expect(waitForUSPolicyStatus(uspolicyNsName, gatewayName, status, condReason)).To(Succeed()) + }, + Entry("uspolicy merge-usp-1", "merge-usp-1", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), + Entry("uspolicy merge-usp-2", "merge-usp-2", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), + Entry("uspolicy merge-usp-3", "z-merge-usp-3", metav1.ConditionFalse, v1alpha2.PolicyReasonConflicted), + Entry("uspolicy a-usp-wins", "a-usp-wins", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), + Entry("uspolicy z-usp", "z-usp", metav1.ConditionFalse, v1alpha2.PolicyReasonConflicted), + ) + + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoutes", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") + + Eventually( + func() error { + return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(1000 * time.Millisecond). + Should(Succeed()) + + Eventually( + func() error { + return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(1000 * time.Millisecond). + Should(Succeed()) + }) + }) + + Context("nginx directives", func() { + var conf *framework.Payload + + BeforeAll(func() { + podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(podNames).To(HaveLen(1)) + + ngfPodName := podNames[0] + + conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + Expect(err).ToNot(HaveOccurred()) + }) + + DescribeTable("are set properly for", + func(expCfgs []framework.ExpectedNginxField) { + for _, expCfg := range expCfgs { + Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) + } + }, + Entry("Coffee upstream", []framework.ExpectedNginxField{ + { + Directive: "upstream", + Value: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "zone", + Value: fmt.Sprintf("uspolicy_coffee_80 %s", zoneSize), + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "100", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "55", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "1m", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "5h", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + }), + Entry("Tea upstream", []framework.ExpectedNginxField{ + { + Directive: "zone", + Value: "uspolicy_tea_80 128k", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "upstream", + Value: "uspolicy_tea_80", + File: "http.conf", + }, + }), + ) + }) + }) + + When("UpstreamSettingsPolicy targets a Service that does not exist", func() { + Specify("upstreamSettingsPolicy sets no condition", func() { + files := []string{"upstream-settings-policy/invalid-svc-usps.yaml"} + + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + + uspolicyNsName := types.NamespacedName{Name: "usps-target-not-found", Namespace: namespace} + + Consistently( + func() bool { + return usPolicyHasNoAncestors(uspolicyNsName) + }).WithTimeout(timeoutConfig.GetTimeout). + WithPolling(500 * time.Millisecond). + Should(BeTrue()) + + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + }) + }) + + When("UpstreamSettingsPolicy targets a Service that is owned by an invalid Gateway", func() { + Specify("upstreamSettingsPolicy is not Accepted with the reason TargetNotFound", func() { + // delete existing gateway + gatewayFileName := "upstream-settings-policy/gateway.yaml" + Expect(resourceManager.DeleteFromFiles([]string{gatewayFileName}, namespace)).To(Succeed()) + + files := []string{"upstream-settings-policy/invalid-target-usps.yaml"} + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + + uspolicyNsName := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} + gatewayName = "gateway-not-valid" + Expect(waitForUSPolicyStatus( + uspolicyNsName, + gatewayName, + metav1.ConditionFalse, + v1alpha2.PolicyReasonTargetNotFound, + )).To(Succeed()) + + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + }) + }) +}) + +func usPolicyHasNoAncestors(usPolicyNsName types.NamespacedName) bool { + GinkgoWriter.Printf("Checking that UpstreamSettingsPolicy %q has no ancestors in status\n", usPolicyNsName) + + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout) + defer cancel() + + var usPolicy ngfAPI.UpstreamSettingsPolicy + if err := k8sClient.Get(ctx, usPolicyNsName, &usPolicy); err != nil { + GinkgoWriter.Printf("Failed to get UpstreamSettingsPolicy %q: %s", usPolicyNsName, err.Error()) + return false + } + + return len(usPolicy.Status.Ancestors) == 0 +} + +func waitForUSPolicyStatus( + usPolicyNsName types.NamespacedName, + gatewayName string, + condStatus metav1.ConditionStatus, + condReason v1alpha2.PolicyConditionReason, +) error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout*2) + defer cancel() + + GinkgoWriter.Printf( + "Waiting for UpstreamSettings Policy %q to have the condition %q/%q\n", + usPolicyNsName, + condStatus, + condReason, + ) + + return wait.PollUntilContextCancel( + ctx, + 2000*time.Millisecond, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + var usPolicy ngfAPI.UpstreamSettingsPolicy + var err error + + if err := k8sClient.Get(ctx, usPolicyNsName, &usPolicy); err != nil { + return false, err + } + + if len(usPolicy.Status.Ancestors) == 0 { + GinkgoWriter.Printf("UpstreamSettingsPolicy %q does not have an ancestor status yet\n", usPolicy) + + return false, nil + } + + if len(usPolicy.Status.Ancestors) != 1 { + return false, fmt.Errorf("policy has %d ancestors, expected 1", len(usPolicy.Status.Ancestors)) + } + + ancestors := usPolicy.Status.Ancestors + + for _, ancestor := range ancestors { + if err := ancestorMustEqualGatewayRef(ancestor, gatewayName, usPolicy.Namespace); err != nil { + return false, err + } + + err = ancestorStatusMustHaveAcceptedCondition(ancestor, condStatus, condReason) + } + return err == nil, err + }, + ) +} + +func ancestorMustEqualGatewayRef( + ancestor v1alpha2.PolicyAncestorStatus, + gatewayName string, + namespace string, +) error { + if ancestor.ControllerName != ngfControllerName { + return fmt.Errorf( + "expected ancestor controller name to be %s, got %s", + ngfControllerName, + ancestor.ControllerName, + ) + } + + if ancestor.AncestorRef.Namespace == nil { + return fmt.Errorf("expected ancestor namespace to be %s, got nil", namespace) + } + + if string(*ancestor.AncestorRef.Namespace) != namespace { + return fmt.Errorf( + "expected ancestor namespace to be %s, got %s", + namespace, + string(*ancestor.AncestorRef.Namespace), + ) + } + + ancestorRef := ancestor.AncestorRef + + if string(ancestorRef.Name) != gatewayName { + return fmt.Errorf("expected ancestorRef to have name %s, got %s", gatewayName, ancestorRef.Name) + } + + if ancestorRef.Kind == nil { + return errors.New("expected ancestorRef to have kind Gateway, got nil") + } + + if *ancestorRef.Kind != gatewayv1.Kind("Gateway") { + return fmt.Errorf( + "expected ancestorRef to have kind %s, got %s", + "Gateway", + string(*ancestorRef.Kind), + ) + } + + return nil +}