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/kustomization.yaml b/config/crd/kustomization.yaml index 0128a189cb..b067d64141 100644 --- a/config/crd/kustomization.yaml +++ b/config/crd/kustomization.yaml @@ -5,3 +5,5 @@ resources: - bases/gateway.nginx.org_nginxgateways.yaml - 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 4f367638b5..0131c0a534 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 f4916da3ff..4073472171 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 1e87d74771..73cb27daf3 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -1292,3 +1292,636 @@ spec: storage: true subresources: status: {} +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.16.5 + name: snippetsfilters.gateway.nginx.org +spec: + group: gateway.nginx.org + names: + categories: + - nginx-gateway-fabric + kind: SnippetsFilter + listKind: SnippetsFilterList + plural: snippetsfilters + shortNames: + - snippetsfilter + singular: snippetsfilter + scope: Namespaced + versions: + - additionalPrinterColumns: + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + name: v1alpha1 + schema: + openAPIV3Schema: + description: |- + SnippetsFilter is a filter that allows inserting NGINX configuration into the + generated NGINX config for HTTPRoute and GRPCRoute resources. + 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 SnippetsFilter. + properties: + snippets: + description: |- + Snippets is a list of NGINX configuration snippets. + There can only be one snippet per context. + Allowed contexts: main, http, http.server, http.server.location. + items: + description: Snippet represents an NGINX configuration snippet. + properties: + context: + description: Context is the NGINX context to insert the snippet + into. + enum: + - main + - http + - http.server + - http.server.location + type: string + value: + description: Value is the NGINX configuration snippet. + minLength: 1 + type: string + required: + - context + - value + type: object + maxItems: 4 + minItems: 1 + type: array + x-kubernetes-validations: + - message: Only one snippet allowed per context + rule: self.all(s1, self.exists_one(s2, s1.context == s2.context)) + required: + - snippets + type: object + status: + description: Status defines the state of the SnippetsFilter. + properties: + controllers: + description: |- + Controllers is a list of Gateway API controllers that processed the SnippetsFilter + and the status of the SnippetsFilter with respect to each controller. + items: + properties: + conditions: + description: Conditions describe the status of the SnippetsFilter. + 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: + - controllerName + type: object + maxItems: 16 + type: array + type: object + required: + - spec + type: object + served: true + 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 88aae1eedd..5eab1d6403 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 c2bcbafe09..8f8e1cc57a 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 be62207472..d22fff1298 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 f31b0da07b..fed7a22d11 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 25b6210ed1..772304fb1f 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 8231de661e..fc422eab2e 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 4a68115cce..c609ea4810 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 e6fd79ce24..02d310f524 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/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 index e49ed1dffd..95e8a34e6d 100644 --- a/examples/upstream-settings-policy/upstream-settings-policy.yaml +++ b/examples/upstream-settings-policy/upstream-settings-policy.yaml @@ -7,7 +7,7 @@ spec: targetRefs: - group: core kind: Service - name: service + name: coffee keepAlive: connections: 32 requests: 1001 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 4c80fc4260..80738018d2 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -52,6 +52,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" @@ -311,6 +312,10 @@ func createPolicyManager( GVK: mustExtractGVK(&ngfAPI.ObservabilityPolicy{}), Validator: observability.NewValidator(validator), }, + { + GVK: mustExtractGVK(&ngfAPI.UpstreamSettingsPolicy{}), + Validator: upstreamsettings.NewValidator(validator), + }, } return policies.NewManager(mustExtractGVK, cfgs...) @@ -492,6 +497,12 @@ func registerControllers( controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), }, }, + { + objectType: &ngfAPI.UpstreamSettingsPolicy{}, + options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + }, + }, } if cfg.ExperimentalFeatures { @@ -728,6 +739,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/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/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/upstreams_template.go b/internal/mode/static/nginx/config/upstreams_template.go index 47fac2dd00..88a588d136 100644 --- a/internal/mode/static/nginx/config/upstreams_template.go +++ b/internal/mode/static/nginx/config/upstreams_template.go @@ -14,7 +14,7 @@ upstream {{ $u.Name }} { {{ if $u.ZoneSize -}} zone {{ $u.Name }} {{ $u.ZoneSize }}; {{- end }} - {{ range $server := $u.Servers }} + {{ range $server := $u.Servers -}} server {{ $server.Address }}; {{- end }} {{ if $u.KeepAlive.Connections -}} 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 d1ec0d3c2c..1f51688506 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -213,13 +213,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), } } @@ -371,17 +369,21 @@ var _ = Describe("ChangeProcessor", func() { gatewayAPICRD, gatewayAPICRDUpdated *metav1.PartialObjectMetadata routeKey1, routeKey2 graph.RouteKey trKey1, trKey2 graph.L4RouteKey + refSvc, refTLSSvc types.NamespacedName ) BeforeAll(func() { gcUpdated = gc.DeepCopy() gcUpdated.Generation++ + refSvc = types.NamespacedName{Namespace: "service-ns", Name: "service"} + refTLSSvc = types.NamespacedName{Namespace: "tls-service-ns", Name: "tls-service"} + crossNsBackendRef := 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), }, }, @@ -398,7 +400,7 @@ var _ = Describe("ChangeProcessor", func() { routeKey2 = graph.CreateRouteKey(hr2) - tlsBackendRef := createTLSBackendRef("tls-service", "tls-service-ns") + tlsBackendRef := createTLSBackendRef(refTLSSvc.Name, refTLSSvc.Namespace) tr1 = createTLSRoute("tr-1", "gateway-1", "foo.tls.com", tlsBackendRef) @@ -562,7 +564,7 @@ var _ = Describe("ChangeProcessor", func() { { BackendRefs: []graph.BackendRef{ { - SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + SvcNsName: refSvc, Weight: 1, }, }, @@ -642,7 +644,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, }, }, @@ -670,7 +672,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, }, }, @@ -736,15 +738,9 @@ var _ = Describe("ChangeProcessor", func() { L4Routes: map[graph.L4RouteKey]*graph.L4Route{trKey1: expRouteTR1}, Routes: map[graph.RouteKey]*graph.L7Route{routeKey1: expRouteHR1}, ReferencedSecrets: map[types.NamespacedName]*graph.Secret{}, - ReferencedServices: map[types.NamespacedName]struct{}{ - { - Namespace: "service-ns", - Name: "service", - }: {}, - { - Namespace: "tls-service-ns", - Name: "tls-service", - }: {}, + ReferencedServices: map[types.NamespacedName]*graph.ReferencedService{ + refSvc: {}, + refTLSSvc: {}, }, } }) @@ -953,7 +949,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{ @@ -1996,11 +1992,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() { @@ -2011,7 +2009,27 @@ var _ = Describe("ChangeProcessor", func() { Expect(newGraph.NGFPolicies).To(BeEmpty()) gw = createGateway("gw", createHTTPListener()) - route = createRoute("hr-1", "gw", "foo.example.com", v1.HTTPBackendRef{}) + route = createRoute( + "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{ @@ -2072,6 +2090,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", + }, + } }) /* @@ -2084,6 +2131,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)) @@ -2104,12 +2152,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)) @@ -2117,12 +2172,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 974efbb142..8075fae374 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -111,7 +111,7 @@ type Upstream struct { ErrorMsg string // Endpoints are the endpoints of the Upstream. Endpoints []resolver.Endpoint - // Policies contains the list of policies that are applied to this Upstream. + // Policies holds all the valid policies that apply to the Upstream. Policies []policies.Policy } 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..9ce3e7eb23 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,42 @@ 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")} + policy.Ancestors = append(policy.Ancestors, ancestor) + return + } + + 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 +190,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 +207,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 +219,10 @@ func processPolicies( } else { targetedRoutes[client.ObjectKeyFromObject(route.Source)] = route } + case serviceGroupKind: + if _, exists := services[refNsName]; !exists { + continue + } default: continue } @@ -203,22 +239,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 +258,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 +403,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..2c11ac2229 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,83 @@ func TestAttachPolicyToGateway(t *testing.T) { } } +func TestAttachPolicyToService(t *testing.T) { + t.Parallel() + + gwNsname := types.NamespacedName{Namespace: testNs, Name: "gateway"} + + 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: "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 +622,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 +630,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 +640,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 +661,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 +721,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 +862,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 +1011,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 +1177,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 +1316,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/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_test.go b/internal/mode/static/telemetry/collector_test.go index 59bee05aae..3128531ebb 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): {}, @@ -583,7 +583,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{