diff --git a/apis/v1alpha1/observabilitypolicy_types.go b/apis/v1alpha1/observabilitypolicy_types.go index 6902db834..22cb36688 100644 --- a/apis/v1alpha1/observabilitypolicy_types.go +++ b/apis/v1alpha1/observabilitypolicy_types.go @@ -42,11 +42,15 @@ type ObservabilityPolicySpec struct { // +optional Tracing *Tracing `json:"tracing,omitempty"` - // TargetRef identifies an API object to apply the policy to. - // Object must be in the same namespace as the policy. - // + // TargetRefs identifies the API object(s) to apply the policy to. + // Objects must be in the same namespace as the policy. // Support: HTTPRoute - TargetRef gatewayv1alpha2.LocalPolicyTargetReference `json:"targetRef"` + // + // +kubebuilder:validation:MaxItems=16 + // +kubebuilder:validation:XValidation:message="TargetRef Kind must be: HTTPRoute or GRPCRoute",rule="(self.exists(t, t.kind=='HTTPRoute') || self.exists(t, t.kind=='GRPCRoute'))" + // +kubebuilder:validation:XValidation:message="TargetRef Group must be gateway.networking.k8s.io.",rule="self.all(t, t.group=='gateway.networking.k8s.io')" + //nolint:lll + TargetRefs []gatewayv1alpha2.LocalPolicyTargetReference `json:"targetRefs"` } // Tracing allows for enabling and configuring OpenTelemetry tracing. @@ -60,6 +64,7 @@ type Tracing struct { // Ratio is the percentage of traffic that should be sampled. Integer from 0 to 100. // By default, 100% of http requests are traced. Not applicable for parent-based tracing. + // If ratio is set to 0, tracing is disabled. // // +optional // +kubebuilder:validation:Minimum=0 diff --git a/apis/v1alpha1/policy_methods.go b/apis/v1alpha1/policy_methods.go index e4bb43911..97e7074a6 100644 --- a/apis/v1alpha1/policy_methods.go +++ b/apis/v1alpha1/policy_methods.go @@ -8,8 +8,8 @@ import ( // Figure out a way to generate these methods for all our policies. // These methods implement the policies.Policy interface which extends client.Object to add the following methods. -func (p *ClientSettingsPolicy) GetTargetRef() v1alpha2.LocalPolicyTargetReference { - return p.Spec.TargetRef +func (p *ClientSettingsPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{p.Spec.TargetRef} } func (p *ClientSettingsPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { @@ -19,3 +19,15 @@ func (p *ClientSettingsPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { func (p *ClientSettingsPolicy) SetPolicyStatus(status v1alpha2.PolicyStatus) { p.Status = status } + +func (p *ObservabilityPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { + return p.Spec.TargetRefs +} + +func (p *ObservabilityPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { + return p.Status +} + +func (p *ObservabilityPolicy) SetPolicyStatus(status v1alpha2.PolicyStatus) { + p.Status = status +} diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 2b3946d72..a34114056 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -7,6 +7,7 @@ package v1alpha1 import ( "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/gateway-api/apis/v1alpha2" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. @@ -440,7 +441,11 @@ func (in *ObservabilityPolicySpec) DeepCopyInto(out *ObservabilityPolicySpec) { *out = new(Tracing) (*in).DeepCopyInto(*out) } - out.TargetRef = in.TargetRef + if in.TargetRefs != nil { + in, out := &in.TargetRefs, &out.TargetRefs + *out = make([]v1alpha2.LocalPolicyTargetReference, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObservabilityPolicySpec. diff --git a/charts/nginx-gateway-fabric/templates/rbac.yaml b/charts/nginx-gateway-fabric/templates/rbac.yaml index 823669534..330d89b0e 100644 --- a/charts/nginx-gateway-fabric/templates/rbac.yaml +++ b/charts/nginx-gateway-fabric/templates/rbac.yaml @@ -122,6 +122,7 @@ rules: resources: - nginxproxies - clientsettingspolicies + - observabilitypolicies verbs: - list - watch @@ -130,6 +131,7 @@ rules: resources: - nginxgateways/status - clientsettingspolicies/status + - observabilitypolicies/status verbs: - update {{- if .Values.nginxGateway.leaderElection.enable }} diff --git a/config/crd/bases/gateway.nginx.org_observabilitypolicies.yaml b/config/crd/bases/gateway.nginx.org_observabilitypolicies.yaml index 461637d60..cf83c4174 100644 --- a/config/crd/bases/gateway.nginx.org_observabilitypolicies.yaml +++ b/config/crd/bases/gateway.nginx.org_observabilitypolicies.yaml @@ -50,35 +50,47 @@ spec: spec: description: Spec defines the desired state of the ObservabilityPolicy. properties: - targetRef: + targetRefs: description: |- - TargetRef identifies an API object to apply the policy to. - Object must be in the same namespace as the policy. - - + TargetRefs identifies the API object(s) to apply the policy to. + Objects must be in the same namespace as the policy. Support: HTTPRoute - 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 + 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 + type: array + x-kubernetes-validations: + - message: 'TargetRef Kind must be: HTTPRoute or GRPCRoute' + rule: (self.exists(t, t.kind=='HTTPRoute') || self.exists(t, t.kind=='GRPCRoute')) + - message: TargetRef Group must be gateway.networking.k8s.io. + rule: self.all(t, t.group=='gateway.networking.k8s.io') tracing: description: Tracing allows for enabling and configuring tracing. properties: @@ -96,6 +108,7 @@ spec: description: |- Ratio is the percentage of traffic that should be sampled. Integer from 0 to 100. By default, 100% of http requests are traced. Not applicable for parent-based tracing. + If ratio is set to 0, tracing is disabled. format: int32 maximum: 100 minimum: 0 @@ -155,7 +168,7 @@ spec: - message: ratio can only be specified if strategy is of type ratio rule: '!(has(self.ratio) && self.strategy != ''ratio'')' required: - - targetRef + - targetRefs type: object status: description: Status defines the state of the ObservabilityPolicy. diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 82e793e4b..0da03a830 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -832,35 +832,47 @@ spec: spec: description: Spec defines the desired state of the ObservabilityPolicy. properties: - targetRef: + targetRefs: description: |- - TargetRef identifies an API object to apply the policy to. - Object must be in the same namespace as the policy. - - + TargetRefs identifies the API object(s) to apply the policy to. + Objects must be in the same namespace as the policy. Support: HTTPRoute - 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 + 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 + type: array + x-kubernetes-validations: + - message: 'TargetRef Kind must be: HTTPRoute or GRPCRoute' + rule: (self.exists(t, t.kind=='HTTPRoute') || self.exists(t, t.kind=='GRPCRoute')) + - message: TargetRef Group must be gateway.networking.k8s.io. + rule: self.all(t, t.group=='gateway.networking.k8s.io') tracing: description: Tracing allows for enabling and configuring tracing. properties: @@ -878,6 +890,7 @@ spec: description: |- Ratio is the percentage of traffic that should be sampled. Integer from 0 to 100. By default, 100% of http requests are traced. Not applicable for parent-based tracing. + If ratio is set to 0, tracing is disabled. format: int32 maximum: 100 minimum: 0 @@ -937,7 +950,7 @@ spec: - message: ratio can only be specified if strategy is of type ratio rule: '!(has(self.ratio) && self.strategy != ''ratio'')' required: - - targetRef + - targetRefs type: object status: description: Status defines the state of the ObservabilityPolicy. diff --git a/deploy/manifests/nginx-gateway-experimental.yaml b/deploy/manifests/nginx-gateway-experimental.yaml index b0e068ebb..ca2c559b3 100644 --- a/deploy/manifests/nginx-gateway-experimental.yaml +++ b/deploy/manifests/nginx-gateway-experimental.yaml @@ -104,6 +104,7 @@ rules: resources: - nginxproxies - clientsettingspolicies + - observabilitypolicies verbs: - list - watch @@ -112,6 +113,7 @@ rules: resources: - nginxgateways/status - clientsettingspolicies/status + - observabilitypolicies/status verbs: - update - apiGroups: diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 021b24088..2e67db186 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -101,6 +101,7 @@ rules: resources: - nginxproxies - clientsettingspolicies + - observabilitypolicies verbs: - list - watch @@ -109,6 +110,7 @@ rules: resources: - nginxgateways/status - clientsettingspolicies/status + - observabilitypolicies/status verbs: - update - apiGroups: diff --git a/deploy/manifests/nginx-plus-gateway-experimental.yaml b/deploy/manifests/nginx-plus-gateway-experimental.yaml index c5b61cdc6..f92a10699 100644 --- a/deploy/manifests/nginx-plus-gateway-experimental.yaml +++ b/deploy/manifests/nginx-plus-gateway-experimental.yaml @@ -110,6 +110,7 @@ rules: resources: - nginxproxies - clientsettingspolicies + - observabilitypolicies verbs: - list - watch @@ -118,6 +119,7 @@ rules: resources: - nginxgateways/status - clientsettingspolicies/status + - observabilitypolicies/status verbs: - update - apiGroups: diff --git a/deploy/manifests/nginx-plus-gateway.yaml b/deploy/manifests/nginx-plus-gateway.yaml index b2f0a1e3e..57fcbbe57 100644 --- a/deploy/manifests/nginx-plus-gateway.yaml +++ b/deploy/manifests/nginx-plus-gateway.yaml @@ -107,6 +107,7 @@ rules: resources: - nginxproxies - clientsettingspolicies + - observabilitypolicies verbs: - list - watch @@ -115,6 +116,7 @@ rules: resources: - nginxgateways/status - clientsettingspolicies/status + - observabilitypolicies/status verbs: - update - apiGroups: diff --git a/docs/proposals/observability.md b/docs/proposals/observability.md index fd4987787..67cee1b93 100644 --- a/docs/proposals/observability.md +++ b/docs/proposals/observability.md @@ -1,7 +1,7 @@ # Enhancement Proposal-1778: Observability Policy - Issue: https://github.com/nginxinc/nginx-gateway-fabric/issues/1778 -- Status: Implementable +- Status: Completed ## Summary @@ -65,10 +65,10 @@ type ObservabilityPolicy struct { } type ObservabilityPolicySpec struct { - // TargetRef identifies an API object to apply the policy to. - // Object must be in the same namespace as the policy. + // TargetRefs identifies API object(s) to apply the policy to. + // Objects must be in the same namespace as the policy. // Support: HTTPRoute - TargetRef gatewayv1alpha2.LocalPolicyTargetReference `json:"targetRef"` + TargetRefs []gatewayv1alpha2.LocalPolicyTargetReference `json:"targetRefs"` // Tracing allows for enabling and configuring tracing. // @@ -154,8 +154,8 @@ metadata: name: example-observability-policy namespace: default spec: - targetRef: - group: gateway.networking.k8s.io + targetRefs: + - group: gateway.networking.k8s.io kind: HTTPRoute name: example-route tracing: diff --git a/internal/framework/kinds/kinds.go b/internal/framework/kinds/kinds.go index e67376ea3..790ae4eab 100644 --- a/internal/framework/kinds/kinds.go +++ b/internal/framework/kinds/kinds.go @@ -25,6 +25,8 @@ const ( const ( // ClientSettingsPolicy is the ClientSettingsPolicy kind. ClientSettingsPolicy = "ClientSettingsPolicy" + // ObservabilityPolicy is the ObservabilityPolicy kind. + ObservabilityPolicy = "ObservabilityPolicy" // NginxProxy is the NginxProxy kind. NginxProxy = "NginxProxy" ) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index a11e67067..f31f5d383 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -53,6 +53,7 @@ import ( ngxruntime "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" @@ -292,6 +293,11 @@ func createPolicyManager( Validator: clientsettings.NewValidator(validator), Generator: clientsettings.Generate, }, + { + GVK: mustExtractGVK(&ngfAPI.ObservabilityPolicy{}), + Validator: observability.NewValidator(validator), + Generator: observability.Generate, + }, } return policies.NewManager(mustExtractGVK, cfgs...) @@ -461,6 +467,12 @@ func registerControllers( controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), }, }, + { + objectType: &ngfAPI.ObservabilityPolicy{}, + options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + }, + }, } if cfg.ExperimentalFeatures { @@ -642,6 +654,7 @@ func prepareFirstEventBatchPreparerArgs( &ngfAPI.NginxProxyList{}, &gatewayv1.GRPCRouteList{}, &ngfAPI.ClientSettingsPolicyList{}, + &ngfAPI.ObservabilityPolicyList{}, partialObjectMetadataList, } diff --git a/internal/mode/static/manager_test.go b/internal/mode/static/manager_test.go index 1d02cee61..73cfa7be2 100644 --- a/internal/mode/static/manager_test.go +++ b/internal/mode/static/manager_test.go @@ -57,6 +57,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &gatewayv1.GRPCRouteList{}, partialObjectMetadataList, &ngfAPI.ClientSettingsPolicyList{}, + &ngfAPI.ObservabilityPolicyList{}, }, }, { @@ -80,6 +81,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &gatewayv1.GRPCRouteList{}, partialObjectMetadataList, &ngfAPI.ClientSettingsPolicyList{}, + &ngfAPI.ObservabilityPolicyList{}, }, }, { @@ -105,6 +107,7 @@ func TestPrepareFirstEventBatchPreparerArgs(t *testing.T) { &gatewayv1alpha3.BackendTLSPolicyList{}, &gatewayv1.GRPCRouteList{}, &ngfAPI.ClientSettingsPolicyList{}, + &ngfAPI.ObservabilityPolicyList{}, }, experimentalEnabled: true, }, diff --git a/internal/mode/static/nginx/config/telemetry_template.go b/internal/mode/static/nginx/config/telemetry_template.go index 515207583..1157d97d2 100644 --- a/internal/mode/static/nginx/config/telemetry_template.go +++ b/internal/mode/static/nginx/config/telemetry_template.go @@ -16,7 +16,10 @@ otel_exporter { otel_service_name {{ .ServiceName }}; -{{- range $attr := .SpanAttributes }} -otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; +{{- range $ratio := .Ratios }} +split_clients $otel_trace_id {{ $ratio.Name }} { + {{ $ratio.Value }}% on; + * off; +} {{- end }} ` diff --git a/internal/mode/static/nginx/config/telemetry_test.go b/internal/mode/static/nginx/config/telemetry_test.go index 27226cfba..6d59c1aba 100644 --- a/internal/mode/static/nginx/config/telemetry_test.go +++ b/internal/mode/static/nginx/config/telemetry_test.go @@ -17,14 +17,14 @@ func TestExecuteTelemetry(t *testing.T) { Interval: "5s", BatchSize: 512, BatchCount: 4, - SpanAttributes: []dataplane.SpanAttribute{ + Ratios: []dataplane.Ratio{ { - Key: "key1", - Value: "val1", + Name: "ratio1", + Value: 10, }, { - Key: "key2", - Value: "val2", + Name: "ratio2", + Value: 20, }, }, }, @@ -37,7 +37,9 @@ func TestExecuteTelemetry(t *testing.T) { "interval 5s;": 1, "batch_size 512;": 1, "batch_count 4;": 1, - "otel_span_attr": 2, + "split_clients $otel_trace_id": 2, + "10% on": 1, + "20% on": 1, } for expSubStr, expCount := range expSubStrings { diff --git a/internal/mode/static/policies/clientsettings/generator.go b/internal/mode/static/policies/clientsettings/generator.go index e7f50ef8b..943c8e2c6 100644 --- a/internal/mode/static/policies/clientsettings/generator.go +++ b/internal/mode/static/policies/clientsettings/generator.go @@ -37,7 +37,7 @@ keepalive_timeout {{ .KeepAlive.Timeout.Server }}; ` // Generate generates configuration as []byte for a ClientSettingsPolicy. -func Generate(policy policies.Policy) []byte { +func Generate(policy policies.Policy, _ *policies.GlobalSettings) []byte { csp := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](policy) return helpers.MustExecuteTemplate(tmpl, csp.Spec) diff --git a/internal/mode/static/policies/clientsettings/generator_test.go b/internal/mode/static/policies/clientsettings/generator_test.go index a286342d5..19b3b1de3 100644 --- a/internal/mode/static/policies/clientsettings/generator_test.go +++ b/internal/mode/static/policies/clientsettings/generator_test.go @@ -153,7 +153,7 @@ func TestGenerate(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - cfgString := string(clientsettings.Generate(test.policy)) + cfgString := string(clientsettings.Generate(test.policy, nil)) for _, str := range test.expStrings { g.Expect(cfgString).To(ContainSubstring(str)) @@ -166,7 +166,7 @@ func TestGeneratePanics(t *testing.T) { g := NewWithT(t) generate := func() { - clientsettings.Generate(&policiesfakes.FakePolicy{}) + clientsettings.Generate(&policiesfakes.FakePolicy{}, nil) } g.Expect(generate).To(Panic()) diff --git a/internal/mode/static/policies/clientsettings/validator.go b/internal/mode/static/policies/clientsettings/validator.go index efe863043..4e5ea1007 100644 --- a/internal/mode/static/policies/clientsettings/validator.go +++ b/internal/mode/static/policies/clientsettings/validator.go @@ -1,16 +1,15 @@ package clientsettings import ( - "slices" - "k8s.io/apimachinery/pkg/util/validation/field" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" - "sigs.k8s.io/gateway-api/apis/v1alpha2" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/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/policies" + staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -26,14 +25,20 @@ func NewValidator(genericValidator validation.GenericValidator) *Validator { } // Validate validates the spec of a ClientSettingsPolicy. -func (v *Validator) Validate(policy policies.Policy) error { +func (v *Validator) Validate(policy policies.Policy, _ *policies.GlobalSettings) []conditions.Condition { csp := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](policy) - if err := validateTargetRef(csp.Spec.TargetRef); err != nil { - return err + 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 { + return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} } - return v.validateSettings(csp.Spec) + if err := v.validateSettings(csp.Spec); err != nil { + return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} + } + + return nil } // Conflicts returns true if the two ClientSettingsPolicies conflict. @@ -73,34 +78,6 @@ func conflicts(a, b ngfAPI.ClientSettingsPolicySpec) bool { return false } -func validateTargetRef(ref v1alpha2.LocalPolicyTargetReference) error { - basePath := field.NewPath("spec").Child("targetRef") - - if ref.Group != gatewayv1.GroupName { - path := basePath.Child("group") - - return field.NotSupported( - path, - ref.Group, - []string{gatewayv1.GroupName}, - ) - } - - supportedKinds := []gatewayv1.Kind{kinds.Gateway, kinds.HTTPRoute, kinds.GRPCRoute} - - if !slices.Contains(supportedKinds, ref.Kind) { - path := basePath.Child("kind") - - return field.NotSupported( - path, - ref.Kind, - supportedKinds, - ) - } - - return nil -} - // 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.ClientSettingsPolicySpec) error { diff --git a/internal/mode/static/policies/clientsettings/validator_test.go b/internal/mode/static/policies/clientsettings/validator_test.go index 04d83ae97..8b9a7fcdd 100644 --- a/internal/mode/static/policies/clientsettings/validator_test.go +++ b/internal/mode/static/policies/clientsettings/validator_test.go @@ -9,11 +9,13 @@ import ( "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/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" + staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) type policyModFunc func(policy *ngfAPI.ClientSettingsPolicy) *ngfAPI.ClientSettingsPolicy @@ -52,9 +54,9 @@ func createModifiedPolicy(mod policyModFunc) *ngfAPI.ClientSettingsPolicy { func TestValidator_Validate(t *testing.T) { tests := []struct { - name string - policy *ngfAPI.ClientSettingsPolicy - expErrSubstrings []string + name string + policy *ngfAPI.ClientSettingsPolicy + expConditions []conditions.Condition }{ { name: "invalid target ref; unsupported group", @@ -62,7 +64,10 @@ func TestValidator_Validate(t *testing.T) { p.Spec.TargetRef.Group = "Unsupported" return p }), - expErrSubstrings: []string{"spec.targetRef.group"}, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.targetRef.group: Unsupported value: \"Unsupported\": " + + "supported values: \"gateway.networking.k8s.io\""), + }, }, { name: "invalid target ref; unsupported kind", @@ -70,7 +75,10 @@ func TestValidator_Validate(t *testing.T) { p.Spec.TargetRef.Kind = "Unsupported" return p }), - expErrSubstrings: []string{"spec.targetRef.kind"}, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.targetRef.kind: Unsupported value: \"Unsupported\": " + + "supported values: \"Gateway\", \"HTTPRoute\", \"GRPCRoute\""), + }, }, { name: "invalid client max body size", @@ -78,7 +86,11 @@ func TestValidator_Validate(t *testing.T) { p.Spec.Body.MaxSize = helpers.GetPointer[ngfAPI.Size]("invalid") return p }), - expErrSubstrings: []string{"spec.body.maxSize"}, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.body.maxSize: 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", @@ -89,11 +101,14 @@ func TestValidator_Validate(t *testing.T) { p.Spec.KeepAlive.Timeout.Header = helpers.GetPointer[ngfAPI.Duration]("invalid") return p }), - expErrSubstrings: []string{ - "spec.body.timeout", - "spec.keepAlive.time", - "spec.keepAlive.timeout.server", - "spec.keepAlive.timeout.header", + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("[spec.body.timeout: Invalid value: \"invalid\": \\d{1,4}(ms|s)? " + + "(e.g. '5ms', or '10s', regex used for validation is 'must contain a number followed by 'ms' or 's''), " + + "spec.keepAlive.time: Invalid value: \"invalid\": \\d{1,4}(ms|s)? (e.g. '5ms', or '10s', regex used for " + + "validation is 'must contain a number followed by 'ms' or 's''), spec.keepAlive.timeout.server: Invalid value: " + + "\"invalid\": \\d{1,4}(ms|s)? (e.g. '5ms', or '10s', regex used for validation is 'must contain a number " + + "followed by 'ms' or 's''), spec.keepAlive.timeout.header: Invalid value: \"invalid\": \\d{1,4}(ms|s)? " + + "(e.g. '5ms', or '10s', regex used for validation is 'must contain a number followed by 'ms' or 's'')]"), }, }, { @@ -102,12 +117,15 @@ func TestValidator_Validate(t *testing.T) { p.Spec.KeepAlive.Timeout.Server = nil return p }), - expErrSubstrings: []string{"spec.keepAlive.timeout"}, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.keepAlive.timeout: Invalid value: \"null\": " + + "server timeout must be set if header timeout is set"), + }, }, { - name: "valid", - policy: createValidPolicy(), - expErrSubstrings: nil, + name: "valid", + policy: createValidPolicy(), + expConditions: nil, }, } @@ -117,17 +135,8 @@ func TestValidator_Validate(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - err := v.Validate(test.policy) - - if len(test.expErrSubstrings) == 0 { - g.Expect(err).ToNot(HaveOccurred()) - } else { - g.Expect(err).To(HaveOccurred()) - } - - for _, str := range test.expErrSubstrings { - g.Expect(err.Error()).To(ContainSubstring(str)) - } + conds := v.Validate(test.policy, nil) + g.Expect(conds).To(Equal(test.expConditions)) }) } } @@ -136,7 +145,7 @@ func TestValidator_ValidatePanics(t *testing.T) { v := clientsettings.NewValidator(nil) validate := func() { - _ = v.Validate(&policiesfakes.FakePolicy{}) + _ = v.Validate(&policiesfakes.FakePolicy{}, nil) } g := NewWithT(t) diff --git a/internal/mode/static/policies/manager.go b/internal/mode/static/policies/manager.go index f1f7a15fb..c7355720b 100644 --- a/internal/mode/static/policies/manager.go +++ b/internal/mode/static/policies/manager.go @@ -7,18 +7,19 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ) // GenerateFunc generates config as []byte for an NGF Policy. -type GenerateFunc func(policy Policy) []byte +type GenerateFunc func(policy Policy, globalSettings *GlobalSettings) []byte // Validator validates an NGF Policy. // //counterfeiter:generate . Validator type Validator interface { // Validate validates an NGF Policy. - Validate(policy Policy) error + Validate(policy Policy, globalSettings *GlobalSettings) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b Policy) bool } @@ -61,7 +62,7 @@ func NewManager( } // Generate generates config for the policy as a byte array. -func (m *Manager) Generate(policy Policy) []byte { +func (m *Manager) Generate(policy Policy, globalSettings *GlobalSettings) []byte { gvk := m.mustExtractGVK(policy) generate, ok := m.generators[gvk] @@ -69,11 +70,11 @@ func (m *Manager) Generate(policy Policy) []byte { panic(fmt.Sprintf("no generate function registered for policy %T", policy)) } - return generate(policy) + return generate(policy, globalSettings) } // Validate validates the policy. -func (m *Manager) Validate(policy Policy) error { +func (m *Manager) Validate(policy Policy, globalSettings *GlobalSettings) []conditions.Condition { gvk := m.mustExtractGVK(policy) validator, ok := m.validators[gvk] @@ -81,7 +82,7 @@ func (m *Manager) Validate(policy Policy) error { panic(fmt.Sprintf("no validator registered for policy %T", policy)) } - return validator.Validate(policy) + return validator.Validate(policy, globalSettings) } // Conflicts returns true if the policies conflict. diff --git a/internal/mode/static/policies/manager_test.go b/internal/mode/static/policies/manager_test.go index db6d81fda..5699ac048 100644 --- a/internal/mode/static/policies/manager_test.go +++ b/internal/mode/static/policies/manager_test.go @@ -1,15 +1,15 @@ package policies_test import ( - "errors" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" + staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) var _ = Describe("Policy Manager", func() { @@ -42,20 +42,24 @@ var _ = Describe("Policy Manager", func() { mustExtractGVK, policies.ManagerConfig{ Validator: &policiesfakes.FakeValidator{ - ValidateStub: func(_ policies.Policy) error { return errors.New("apple error") }, + ValidateStub: func(_ policies.Policy, _ *policies.GlobalSettings) []conditions.Condition { + return []conditions.Condition{staticConds.NewPolicyInvalid("apple error")} + }, ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return true }, }, - Generator: func(_ policies.Policy) []byte { + Generator: func(_ policies.Policy, _ *policies.GlobalSettings) []byte { return []byte("apple") }, GVK: appleGVK, }, policies.ManagerConfig{ Validator: &policiesfakes.FakeValidator{ - ValidateStub: func(_ policies.Policy) error { return errors.New("orange error") }, + ValidateStub: func(_ policies.Policy, _ *policies.GlobalSettings) []conditions.Condition { + return []conditions.Condition{staticConds.NewPolicyInvalid("orange error")} + }, ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return false }, }, - Generator: func(_ policies.Policy) []byte { + Generator: func(_ policies.Policy, _ *policies.GlobalSettings) []byte { return []byte("orange") }, GVK: orangeGVK, @@ -65,13 +69,13 @@ var _ = Describe("Policy Manager", func() { Context("Validation", func() { When("Policy is registered with manager", func() { It("Validates the policy", func() { - err := mgr.Validate(applePolicy) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(Equal("apple error")) + conds := mgr.Validate(applePolicy, nil) + Expect(conds).To(HaveLen(1)) + Expect(conds[0].Message).To(Equal("apple error")) - err = mgr.Validate(orangePolicy) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(Equal("orange error")) + conds = mgr.Validate(orangePolicy, nil) + Expect(conds).To(HaveLen(1)) + Expect(conds[0].Message).To(Equal("orange error")) }) It("Returns whether the policies conflict", func() { Expect(mgr.Conflicts(applePolicy, applePolicy)).To(BeTrue()) @@ -81,7 +85,7 @@ var _ = Describe("Policy Manager", func() { When("Policy is not registered with manager", func() { It("Panics on call to validate", func() { validate := func() { - _ = mgr.Validate(&policiesfakes.FakePolicy{}) + _ = mgr.Validate(&policiesfakes.FakePolicy{}, nil) } Expect(validate).To(Panic()) @@ -98,14 +102,14 @@ var _ = Describe("Policy Manager", func() { Context("Generation", func() { When("Policy is registered with manager", func() { It("Generates the configuration for the policy", func() { - Expect(mgr.Generate(applePolicy)).To(Equal([]byte("apple"))) - Expect(mgr.Generate(orangePolicy)).To(Equal([]byte("orange"))) + Expect(mgr.Generate(applePolicy, nil)).To(Equal([]byte("apple"))) + Expect(mgr.Generate(orangePolicy, nil)).To(Equal([]byte("orange"))) }) }) When("Policy is not registered with manager", func() { It("Panics on generate", func() { generate := func() { - _ = mgr.Generate(&policiesfakes.FakePolicy{}) + _ = mgr.Generate(&policiesfakes.FakePolicy{}, nil) } Expect(generate).To(Panic()) diff --git a/internal/mode/static/policies/observability/generator.go b/internal/mode/static/policies/observability/generator.go new file mode 100644 index 000000000..046881ca6 --- /dev/null +++ b/internal/mode/static/policies/observability/generator.go @@ -0,0 +1,73 @@ +package observability + +import ( + "fmt" + "text/template" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" +) + +var tmpl = template.Must(template.New("observability policy").Parse(observabilityTemplate)) + +const observabilityTemplate = ` +{{- if .Tracing }} +otel_trace {{ .Strategy }}; + {{- if .Tracing.Context }} +otel_trace_context {{ .Tracing.Context }}; + {{- end }} + {{- if .Tracing.SpanName }} +otel_span_name "{{ .Tracing.SpanName }}"; + {{- end }} + {{- range $attr := .Tracing.SpanAttributes }} +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; + {{- end }} + {{- range $attr := .GlobalSpanAttributes }} +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; + {{- end }} +{{- end }} +` + +// Generate generates configuration as []byte for an ObservabilityPolicy. +func Generate(policy policies.Policy, globalSettings *policies.GlobalSettings) []byte { + obs := helpers.MustCastObject[*ngfAPI.ObservabilityPolicy](policy) + + var strategy string + if obs.Spec.Tracing != nil { + switch obs.Spec.Tracing.Strategy { + case ngfAPI.TraceStrategyParent: + strategy = "$otel_parent_sampled" + case ngfAPI.TraceStrategyRatio: + strategy = "on" + if obs.Spec.Tracing.Ratio != nil { + if *obs.Spec.Tracing.Ratio > 0 { + strategy = CreateRatioVarName(obs) + } else { + strategy = "off" + } + } + default: + strategy = "off" + } + } + + var spanAttributes []ngfAPI.SpanAttribute + if globalSettings != nil { + spanAttributes = globalSettings.TracingSpanAttributes + } + + fields := map[string]interface{}{ + "Tracing": obs.Spec.Tracing, + "Strategy": strategy, + "GlobalSpanAttributes": spanAttributes, + } + + return helpers.MustExecuteTemplate(tmpl, fields) +} + +// CreateRatioVarName builds a variable name for an ObservabilityPolicy to be used with +// ratio-based trace sampling. +func CreateRatioVarName(policy *ngfAPI.ObservabilityPolicy) string { + return fmt.Sprintf("$otel_ratio_%d", *policy.Spec.Tracing.Ratio) +} diff --git a/internal/mode/static/policies/observability/generator_test.go b/internal/mode/static/policies/observability/generator_test.go new file mode 100644 index 000000000..0a5d33d2e --- /dev/null +++ b/internal/mode/static/policies/observability/generator_test.go @@ -0,0 +1,214 @@ +package observability_test + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" +) + +func TestGenerate(t *testing.T) { + ratio := helpers.GetPointer[int32](25) + zeroRatio := helpers.GetPointer[int32](0) + context := helpers.GetPointer[ngfAPI.TraceContext](ngfAPI.TraceContextExtract) + spanName := helpers.GetPointer("my-span") + + tests := []struct { + name string + policy policies.Policy + globalSettings *policies.GlobalSettings + expStrings []string + }{ + { + name: "strategy set to default ratio", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + }, + }, + }, + expStrings: []string{ + "otel_trace on;", + }, + }, + { + name: "strategy set to custom ratio", + policy: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "test-namespace", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Ratio: ratio, + }, + }, + }, + expStrings: []string{ + "otel_trace $otel_ratio_25;", + }, + }, + { + name: "strategy set to zero ratio", + policy: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "test-namespace", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Ratio: zeroRatio, + }, + }, + }, + expStrings: []string{ + "otel_trace off;", + }, + }, + { + name: "strategy set to parent", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyParent, + }, + }, + }, + expStrings: []string{ + "otel_trace $otel_parent_sampled;", + }, + }, + { + name: "context is set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Context: context, + }, + }, + }, + expStrings: []string{ + "otel_trace off;", + "otel_trace_context extract;", + }, + }, + { + name: "spanName is set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + SpanName: spanName, + }, + }, + }, + expStrings: []string{ + "otel_trace off;", + "otel_span_name \"my-span\";", + }, + }, + { + name: "span attributes set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "test-key", Value: "test-value"}, + }, + }, + }, + }, + expStrings: []string{ + "otel_trace off;", + "otel_span_attr \"test-key\" \"test-value\";", + }, + }, + { + name: "global span attributes set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{}, + }, + }, + globalSettings: &policies.GlobalSettings{ + TracingSpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "test-global-key", Value: "test-global-value"}, + }, + }, + expStrings: []string{ + "otel_trace off;", + "otel_span_attr \"test-global-key\" \"test-global-value\";", + }, + }, + { + name: "all fields populated", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Context: context, + SpanName: spanName, + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "test-key", Value: "test-value"}, + }, + }, + }, + }, + globalSettings: &policies.GlobalSettings{ + TracingSpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "test-global-key", Value: "test-global-value"}, + }, + }, + expStrings: []string{ + "otel_trace on;", + "otel_trace_context extract;", + "otel_span_name \"my-span\";", + "otel_span_attr \"test-key\" \"test-value\";", + "otel_span_attr \"test-global-key\" \"test-global-value\";", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + cfgString := string(observability.Generate(test.policy, test.globalSettings)) + + for _, str := range test.expStrings { + g.Expect(cfgString).To(ContainSubstring(str)) + } + }) + } +} + +func TestGeneratePanics(t *testing.T) { + g := NewWithT(t) + + generate := func() { + observability.Generate(&policiesfakes.FakePolicy{}, nil) + } + + g.Expect(generate).To(Panic()) +} + +func TestCreateRatioVarName(t *testing.T) { + pol := &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Ratio: helpers.GetPointer[int32](25), + }, + }, + } + + g := NewWithT(t) + g.Expect(observability.CreateRatioVarName(pol)).To(Equal("$otel_ratio_25")) +} diff --git a/internal/mode/static/policies/observability/validator.go b/internal/mode/static/policies/observability/validator.go new file mode 100644 index 000000000..78d4e5d9d --- /dev/null +++ b/internal/mode/static/policies/observability/validator.go @@ -0,0 +1,134 @@ +package observability + +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/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 ObservabilityPolicy. +// Implements policies.Validator interface. +type Validator struct { + genericValidator validation.GenericValidator +} + +// NewValidator returns a new instance of Validator. +func NewValidator(genericValidator validation.GenericValidator) *Validator { + return &Validator{genericValidator: genericValidator} +} + +// Validate validates the spec of an ObservabilityPolicy. +func (v *Validator) Validate( + policy policies.Policy, + globalSettings *policies.GlobalSettings, +) []conditions.Condition { + obs := helpers.MustCastObject[*ngfAPI.ObservabilityPolicy](policy) + + if globalSettings == nil || !globalSettings.NginxProxyValid { + return []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageNginxProxyInvalid), + } + } + + if !globalSettings.TelemetryEnabled { + return []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageTelemetryNotEnabled), + } + } + + targetRefPath := field.NewPath("spec").Child("targetRefs") + supportedKinds := []gatewayv1.Kind{kinds.HTTPRoute, kinds.GRPCRoute} + for _, ref := range obs.Spec.TargetRefs { + if err := policies.ValidateTargetRef(ref, targetRefPath, supportedKinds); err != nil { + return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} + } + } + + if err := v.validateSettings(obs.Spec); err != nil { + return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())} + } + + return nil +} + +// Conflicts returns true if the two ObservabilityPolicies conflict. +func (v *Validator) Conflicts(polA, polB policies.Policy) bool { + a := helpers.MustCastObject[*ngfAPI.ObservabilityPolicy](polA) + b := helpers.MustCastObject[*ngfAPI.ObservabilityPolicy](polB) + + return a.Spec.Tracing != nil && b.Spec.Tracing != nil +} + +func (v *Validator) validateSettings(spec ngfAPI.ObservabilityPolicySpec) error { + var allErrs field.ErrorList + fieldPath := field.NewPath("spec") + + if spec.Tracing != nil { + tracePath := fieldPath.Child("tracing") + + switch spec.Tracing.Strategy { + case ngfAPI.TraceStrategyRatio, ngfAPI.TraceStrategyParent: + default: + allErrs = append( + allErrs, + field.NotSupported( + tracePath.Child("strategy"), + spec.Tracing.Strategy, + []string{ + string(ngfAPI.TraceStrategyRatio), + string(ngfAPI.TraceStrategyParent), + }), + ) + } + + if spec.Tracing.Context != nil { + switch *spec.Tracing.Context { + case ngfAPI.TraceContextExtract, + ngfAPI.TraceContextInject, + ngfAPI.TraceContextPropagate, + ngfAPI.TraceContextIgnore: + default: + allErrs = append( + allErrs, + field.NotSupported( + tracePath.Child("context"), + spec.Tracing.Context, + []string{ + string(ngfAPI.TraceContextExtract), + string(ngfAPI.TraceContextInject), + string(ngfAPI.TraceContextPropagate), + string(ngfAPI.TraceContextIgnore), + }), + ) + } + } + + if spec.Tracing.SpanName != nil { + if err := v.genericValidator.ValidateEscapedStringNoVarExpansion(*spec.Tracing.SpanName); err != nil { + allErrs = append(allErrs, field.Invalid(tracePath.Child("spanName"), *spec.Tracing.SpanName, err.Error())) + } + } + + if spec.Tracing.SpanAttributes != nil { + spanAttrPath := tracePath.Child("spanAttributes") + for _, spanAttr := range spec.Tracing.SpanAttributes { + if err := v.genericValidator.ValidateEscapedStringNoVarExpansion(spanAttr.Key); err != nil { + allErrs = append(allErrs, field.Invalid(spanAttrPath.Child("key"), spanAttr.Key, err.Error())) + } + + if err := v.genericValidator.ValidateEscapedStringNoVarExpansion(spanAttr.Value); err != nil { + allErrs = append(allErrs, field.Invalid(spanAttrPath.Child("value"), spanAttr.Value, err.Error())) + } + } + } + } + + return allErrs.ToAggregate() +} diff --git a/internal/mode/static/policies/observability/validator_test.go b/internal/mode/static/policies/observability/validator_test.go new file mode 100644 index 000000000..1e726b9fd --- /dev/null +++ b/internal/mode/static/policies/observability/validator_test.go @@ -0,0 +1,264 @@ +package observability_test + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/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/validation" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" + staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" +) + +type policyModFunc func(policy *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy + +func createValidPolicy() *ngfAPI.ObservabilityPolicy { + return &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + TargetRefs: []v1alpha2.LocalPolicyTargetReference{ + { + Group: gatewayv1.GroupName, + Kind: kinds.HTTPRoute, + Name: "route", + }, + }, + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Context: helpers.GetPointer(ngfAPI.TraceContextExtract), + SpanName: helpers.GetPointer("spanName"), + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "key", Value: "value"}, + }, + }, + }, + Status: v1alpha2.PolicyStatus{}, + } +} + +func createModifiedPolicy(mod policyModFunc) *ngfAPI.ObservabilityPolicy { + return mod(createValidPolicy()) +} + +func TestValidator_Validate(t *testing.T) { + globalSettings := &policies.GlobalSettings{ + NginxProxyValid: true, + TelemetryEnabled: true, + } + + tests := []struct { + name string + policy *ngfAPI.ObservabilityPolicy + globalSettings *policies.GlobalSettings + expConditions []conditions.Condition + }{ + { + name: "validation context is nil", + policy: createValidPolicy(), + expConditions: []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageNginxProxyInvalid), + }, + }, + { + name: "validation context is invalid", + policy: createValidPolicy(), + globalSettings: &policies.GlobalSettings{NginxProxyValid: false}, + expConditions: []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageNginxProxyInvalid), + }, + }, + { + name: "telemetry is not enabled", + policy: createValidPolicy(), + globalSettings: &policies.GlobalSettings{NginxProxyValid: true, TelemetryEnabled: false}, + expConditions: []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageTelemetryNotEnabled), + }, + }, + { + name: "invalid target ref; unsupported group", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.TargetRefs[0].Group = "Unsupported" + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.targetRefs.group: Unsupported value: \"Unsupported\": " + + "supported values: \"gateway.networking.k8s.io\""), + }, + }, + { + name: "invalid target ref; unsupported kind", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.TargetRefs[0].Kind = "Unsupported" + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.targetRefs.kind: Unsupported value: \"Unsupported\": " + + "supported values: \"HTTPRoute\", \"GRPCRoute\""), + }, + }, + { + name: "invalid strategy", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.Tracing.Strategy = "invalid" + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.tracing.strategy: Unsupported value: \"invalid\": " + + "supported values: \"ratio\", \"parent\""), + }, + }, + { + name: "invalid context", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.Tracing.Context = helpers.GetPointer[ngfAPI.TraceContext]("invalid") + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.tracing.context: Unsupported value: \"invalid\": " + + "supported values: \"extract\", \"inject\", \"propagate\", \"ignore\""), + }, + }, + { + name: "invalid span name", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.Tracing.SpanName = helpers.GetPointer("invalid$$$") + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.tracing.spanName: Invalid value: \"invalid$$$\": " + + "a valid value must have all '\"' escaped and must not contain any '$' or end with an " + + "unescaped '\\' (regex used for validation is '([^\"$\\\\]|\\\\[^$])*')"), + }, + }, + { + name: "invalid span attribute key", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.Tracing.SpanAttributes[0].Key = "invalid$$$" + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.tracing.spanAttributes.key: Invalid value: \"invalid$$$\": " + + "a valid value must have all '\"' escaped and must not contain any '$' or end with an " + + "unescaped '\\' (regex used for validation is '([^\"$\\\\]|\\\\[^$])*')"), + }, + }, + { + name: "invalid span attribute value", + policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy { + p.Spec.Tracing.SpanAttributes[0].Value = "invalid$$$" + return p + }), + globalSettings: globalSettings, + expConditions: []conditions.Condition{ + staticConds.NewPolicyInvalid("spec.tracing.spanAttributes.value: Invalid value: \"invalid$$$\": " + + "a valid value must have all '\"' escaped and must not contain any '$' or end with an " + + "unescaped '\\' (regex used for validation is '([^\"$\\\\]|\\\\[^$])*')"), + }, + }, + { + name: "valid", + policy: createValidPolicy(), + globalSettings: globalSettings, + expConditions: nil, + }, + } + + v := observability.NewValidator(validation.GenericValidator{}) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + conds := v.Validate(test.policy, test.globalSettings) + g.Expect(conds).To(Equal(test.expConditions)) + }) + } +} + +func TestValidator_ValidatePanics(t *testing.T) { + v := observability.NewValidator(nil) + + validate := func() { + _ = v.Validate(&policiesfakes.FakePolicy{}, nil) + } + + g := NewWithT(t) + + g.Expect(validate).To(Panic()) +} + +func TestValidator_Conflicts(t *testing.T) { + tests := []struct { + polA *ngfAPI.ObservabilityPolicy + polB *ngfAPI.ObservabilityPolicy + name string + conflicts bool + }{ + { + name: "no conflicts", + polA: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{}, + }, + }, + polB: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{}, + }, + conflicts: false, + }, + { + name: "conflicts", + polA: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{}, + }, + }, + polB: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{}, + }, + }, + conflicts: true, + }, + } + + v := observability.NewValidator(nil) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + g.Expect(v.Conflicts(test.polA, test.polB)).To(Equal(test.conflicts)) + }) + } +} + +func TestValidator_ConflictsPanics(t *testing.T) { + v := observability.NewValidator(nil) + + conflicts := func() { + _ = v.Conflicts(&policiesfakes.FakePolicy{}, &policiesfakes.FakePolicy{}) + } + + g := NewWithT(t) + + g.Expect(conflicts).To(Panic()) +} diff --git a/internal/mode/static/policies/policiesfakes/fake_config_generator.go b/internal/mode/static/policies/policiesfakes/fake_config_generator.go index 92680ac25..17640f029 100644 --- a/internal/mode/static/policies/policiesfakes/fake_config_generator.go +++ b/internal/mode/static/policies/policiesfakes/fake_config_generator.go @@ -8,10 +8,11 @@ import ( ) type FakeConfigGenerator struct { - GenerateStub func(policies.Policy) []byte + GenerateStub func(policies.Policy, *policies.GlobalSettings) []byte generateMutex sync.RWMutex generateArgsForCall []struct { arg1 policies.Policy + arg2 *policies.GlobalSettings } generateReturns struct { result1 []byte @@ -23,18 +24,19 @@ type FakeConfigGenerator struct { invocationsMutex sync.RWMutex } -func (fake *FakeConfigGenerator) Generate(arg1 policies.Policy) []byte { +func (fake *FakeConfigGenerator) Generate(arg1 policies.Policy, arg2 *policies.GlobalSettings) []byte { fake.generateMutex.Lock() ret, specificReturn := fake.generateReturnsOnCall[len(fake.generateArgsForCall)] fake.generateArgsForCall = append(fake.generateArgsForCall, struct { arg1 policies.Policy - }{arg1}) + arg2 *policies.GlobalSettings + }{arg1, arg2}) stub := fake.GenerateStub fakeReturns := fake.generateReturns - fake.recordInvocation("Generate", []interface{}{arg1}) + fake.recordInvocation("Generate", []interface{}{arg1, arg2}) fake.generateMutex.Unlock() if stub != nil { - return stub(arg1) + return stub(arg1, arg2) } if specificReturn { return ret.result1 @@ -48,17 +50,17 @@ func (fake *FakeConfigGenerator) GenerateCallCount() int { return len(fake.generateArgsForCall) } -func (fake *FakeConfigGenerator) GenerateCalls(stub func(policies.Policy) []byte) { +func (fake *FakeConfigGenerator) GenerateCalls(stub func(policies.Policy, *policies.GlobalSettings) []byte) { fake.generateMutex.Lock() defer fake.generateMutex.Unlock() fake.GenerateStub = stub } -func (fake *FakeConfigGenerator) GenerateArgsForCall(i int) policies.Policy { +func (fake *FakeConfigGenerator) GenerateArgsForCall(i int) (policies.Policy, *policies.GlobalSettings) { fake.generateMutex.RLock() defer fake.generateMutex.RUnlock() argsForCall := fake.generateArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeConfigGenerator) GenerateReturns(result1 []byte) { diff --git a/internal/mode/static/policies/policiesfakes/fake_policy.go b/internal/mode/static/policies/policiesfakes/fake_policy.go index 649d78963..5fb03a937 100644 --- a/internal/mode/static/policies/policiesfakes/fake_policy.go +++ b/internal/mode/static/policies/policiesfakes/fake_policy.go @@ -183,15 +183,15 @@ type FakePolicy struct { getSelfLinkReturnsOnCall map[int]struct { result1 string } - GetTargetRefStub func() v1alpha2.LocalPolicyTargetReference - getTargetRefMutex sync.RWMutex - getTargetRefArgsForCall []struct { + GetTargetRefsStub func() []v1alpha2.LocalPolicyTargetReference + getTargetRefsMutex sync.RWMutex + getTargetRefsArgsForCall []struct { } - getTargetRefReturns struct { - result1 v1alpha2.LocalPolicyTargetReference + getTargetRefsReturns struct { + result1 []v1alpha2.LocalPolicyTargetReference } - getTargetRefReturnsOnCall map[int]struct { - result1 v1alpha2.LocalPolicyTargetReference + getTargetRefsReturnsOnCall map[int]struct { + result1 []v1alpha2.LocalPolicyTargetReference } GetUIDStub func() types.UID getUIDMutex sync.RWMutex @@ -1188,15 +1188,15 @@ func (fake *FakePolicy) GetSelfLinkReturnsOnCall(i int, result1 string) { }{result1} } -func (fake *FakePolicy) GetTargetRef() v1alpha2.LocalPolicyTargetReference { - fake.getTargetRefMutex.Lock() - ret, specificReturn := fake.getTargetRefReturnsOnCall[len(fake.getTargetRefArgsForCall)] - fake.getTargetRefArgsForCall = append(fake.getTargetRefArgsForCall, struct { +func (fake *FakePolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { + fake.getTargetRefsMutex.Lock() + ret, specificReturn := fake.getTargetRefsReturnsOnCall[len(fake.getTargetRefsArgsForCall)] + fake.getTargetRefsArgsForCall = append(fake.getTargetRefsArgsForCall, struct { }{}) - stub := fake.GetTargetRefStub - fakeReturns := fake.getTargetRefReturns - fake.recordInvocation("GetTargetRef", []interface{}{}) - fake.getTargetRefMutex.Unlock() + stub := fake.GetTargetRefsStub + fakeReturns := fake.getTargetRefsReturns + fake.recordInvocation("GetTargetRefs", []interface{}{}) + fake.getTargetRefsMutex.Unlock() if stub != nil { return stub() } @@ -1206,38 +1206,38 @@ func (fake *FakePolicy) GetTargetRef() v1alpha2.LocalPolicyTargetReference { return fakeReturns.result1 } -func (fake *FakePolicy) GetTargetRefCallCount() int { - fake.getTargetRefMutex.RLock() - defer fake.getTargetRefMutex.RUnlock() - return len(fake.getTargetRefArgsForCall) +func (fake *FakePolicy) GetTargetRefsCallCount() int { + fake.getTargetRefsMutex.RLock() + defer fake.getTargetRefsMutex.RUnlock() + return len(fake.getTargetRefsArgsForCall) } -func (fake *FakePolicy) GetTargetRefCalls(stub func() v1alpha2.LocalPolicyTargetReference) { - fake.getTargetRefMutex.Lock() - defer fake.getTargetRefMutex.Unlock() - fake.GetTargetRefStub = stub +func (fake *FakePolicy) GetTargetRefsCalls(stub func() []v1alpha2.LocalPolicyTargetReference) { + fake.getTargetRefsMutex.Lock() + defer fake.getTargetRefsMutex.Unlock() + fake.GetTargetRefsStub = stub } -func (fake *FakePolicy) GetTargetRefReturns(result1 v1alpha2.LocalPolicyTargetReference) { - fake.getTargetRefMutex.Lock() - defer fake.getTargetRefMutex.Unlock() - fake.GetTargetRefStub = nil - fake.getTargetRefReturns = struct { - result1 v1alpha2.LocalPolicyTargetReference +func (fake *FakePolicy) GetTargetRefsReturns(result1 []v1alpha2.LocalPolicyTargetReference) { + fake.getTargetRefsMutex.Lock() + defer fake.getTargetRefsMutex.Unlock() + fake.GetTargetRefsStub = nil + fake.getTargetRefsReturns = struct { + result1 []v1alpha2.LocalPolicyTargetReference }{result1} } -func (fake *FakePolicy) GetTargetRefReturnsOnCall(i int, result1 v1alpha2.LocalPolicyTargetReference) { - fake.getTargetRefMutex.Lock() - defer fake.getTargetRefMutex.Unlock() - fake.GetTargetRefStub = nil - if fake.getTargetRefReturnsOnCall == nil { - fake.getTargetRefReturnsOnCall = make(map[int]struct { - result1 v1alpha2.LocalPolicyTargetReference +func (fake *FakePolicy) GetTargetRefsReturnsOnCall(i int, result1 []v1alpha2.LocalPolicyTargetReference) { + fake.getTargetRefsMutex.Lock() + defer fake.getTargetRefsMutex.Unlock() + fake.GetTargetRefsStub = nil + if fake.getTargetRefsReturnsOnCall == nil { + fake.getTargetRefsReturnsOnCall = make(map[int]struct { + result1 []v1alpha2.LocalPolicyTargetReference }) } - fake.getTargetRefReturnsOnCall[i] = struct { - result1 v1alpha2.LocalPolicyTargetReference + fake.getTargetRefsReturnsOnCall[i] = struct { + result1 []v1alpha2.LocalPolicyTargetReference }{result1} } @@ -1858,8 +1858,8 @@ func (fake *FakePolicy) Invocations() map[string][][]interface{} { defer fake.getResourceVersionMutex.RUnlock() fake.getSelfLinkMutex.RLock() defer fake.getSelfLinkMutex.RUnlock() - fake.getTargetRefMutex.RLock() - defer fake.getTargetRefMutex.RUnlock() + fake.getTargetRefsMutex.RLock() + defer fake.getTargetRefsMutex.RUnlock() fake.getUIDMutex.RLock() defer fake.getUIDMutex.RUnlock() fake.setAnnotationsMutex.RLock() diff --git a/internal/mode/static/policies/policiesfakes/fake_validator.go b/internal/mode/static/policies/policiesfakes/fake_validator.go index 79a19826a..288ce3c57 100644 --- a/internal/mode/static/policies/policiesfakes/fake_validator.go +++ b/internal/mode/static/policies/policiesfakes/fake_validator.go @@ -4,6 +4,7 @@ package policiesfakes import ( "sync" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" ) @@ -20,16 +21,17 @@ type FakeValidator struct { conflictsReturnsOnCall map[int]struct { result1 bool } - ValidateStub func(policies.Policy) error + ValidateStub func(policies.Policy, *policies.GlobalSettings) []conditions.Condition validateMutex sync.RWMutex validateArgsForCall []struct { arg1 policies.Policy + arg2 *policies.GlobalSettings } validateReturns struct { - result1 error + result1 []conditions.Condition } validateReturnsOnCall map[int]struct { - result1 error + result1 []conditions.Condition } invocations map[string][][]interface{} invocationsMutex sync.RWMutex @@ -97,18 +99,19 @@ func (fake *FakeValidator) ConflictsReturnsOnCall(i int, result1 bool) { }{result1} } -func (fake *FakeValidator) Validate(arg1 policies.Policy) error { +func (fake *FakeValidator) Validate(arg1 policies.Policy, arg2 *policies.GlobalSettings) []conditions.Condition { fake.validateMutex.Lock() ret, specificReturn := fake.validateReturnsOnCall[len(fake.validateArgsForCall)] fake.validateArgsForCall = append(fake.validateArgsForCall, struct { arg1 policies.Policy - }{arg1}) + arg2 *policies.GlobalSettings + }{arg1, arg2}) stub := fake.ValidateStub fakeReturns := fake.validateReturns - fake.recordInvocation("Validate", []interface{}{arg1}) + fake.recordInvocation("Validate", []interface{}{arg1, arg2}) fake.validateMutex.Unlock() if stub != nil { - return stub(arg1) + return stub(arg1, arg2) } if specificReturn { return ret.result1 @@ -122,39 +125,39 @@ func (fake *FakeValidator) ValidateCallCount() int { return len(fake.validateArgsForCall) } -func (fake *FakeValidator) ValidateCalls(stub func(policies.Policy) error) { +func (fake *FakeValidator) ValidateCalls(stub func(policies.Policy, *policies.GlobalSettings) []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = stub } -func (fake *FakeValidator) ValidateArgsForCall(i int) policies.Policy { +func (fake *FakeValidator) ValidateArgsForCall(i int) (policies.Policy, *policies.GlobalSettings) { fake.validateMutex.RLock() defer fake.validateMutex.RUnlock() argsForCall := fake.validateArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeValidator) ValidateReturns(result1 error) { +func (fake *FakeValidator) ValidateReturns(result1 []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = nil fake.validateReturns = struct { - result1 error + result1 []conditions.Condition }{result1} } -func (fake *FakeValidator) ValidateReturnsOnCall(i int, result1 error) { +func (fake *FakeValidator) ValidateReturnsOnCall(i int, result1 []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = nil if fake.validateReturnsOnCall == nil { fake.validateReturnsOnCall = make(map[int]struct { - result1 error + result1 []conditions.Condition }) } fake.validateReturnsOnCall[i] = struct { - result1 error + result1 []conditions.Condition }{result1} } diff --git a/internal/mode/static/policies/policy.go b/internal/mode/static/policies/policy.go index 12135d8f8..7072fe8d2 100644 --- a/internal/mode/static/policies/policy.go +++ b/internal/mode/static/policies/policy.go @@ -1,8 +1,14 @@ package policies import ( + "slices" + + "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" ) //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate @@ -11,7 +17,7 @@ import ( // //counterfeiter:generate . Policy type Policy interface { - GetTargetRef() v1alpha2.LocalPolicyTargetReference + GetTargetRefs() []v1alpha2.LocalPolicyTargetReference GetPolicyStatus() v1alpha2.PolicyStatus SetPolicyStatus(status v1alpha2.PolicyStatus) client.Object @@ -21,7 +27,47 @@ type Policy interface { // //counterfeiter:generate . ConfigGenerator type ConfigGenerator interface { - Generate(policy Policy) []byte + Generate(policy Policy, globalSettings *GlobalSettings) []byte +} + +// 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 { + // TracingSpanAttributes contain the attributes specified in the NginxProxy resource. + TracingSpanAttributes []ngfAPI.SpanAttribute + // NginxProxyValid is whether or not the NginxProxy resource is valid. + NginxProxyValid bool + // TelemetryEnabled is whether or not telemetry is enabled in the NginxProxy resource. + TelemetryEnabled bool +} + +// ValidateTargetRef validates a policy's targetRef for the proper group and kind. +func ValidateTargetRef( + ref v1alpha2.LocalPolicyTargetReference, + basePath *field.Path, + supportedKinds []gatewayv1.Kind, +) error { + if ref.Group != gatewayv1.GroupName { + path := basePath.Child("group") + + return field.NotSupported( + path, + ref.Group, + []string{gatewayv1.GroupName}, + ) + } + + if !slices.Contains(supportedKinds, ref.Kind) { + path := basePath.Child("kind") + + return field.NotSupported( + path, + ref.Kind, + supportedKinds, + ) + } + + return nil } // We generate a mock of ObjectKind so that we can create fake policies and set their GVKs. diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index e8342aa6c..d3bb4c131 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -205,6 +205,11 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { store: commonPolicyObjectStore, predicate: funcPredicate{stateChanged: isNGFPolicyRelevant}, }, + { + gvk: cfg.MustExtractGVK(&ngfAPI.ObservabilityPolicy{}), + store: commonPolicyObjectStore, + predicate: funcPredicate{stateChanged: isNGFPolicyRelevant}, + }, }, ) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 55073b00f..53d1e7c04 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -1596,7 +1596,7 @@ var _ = Describe("ChangeProcessor", func() { changed, graph := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) - Expect(graph.NginxProxy).To(Equal(np)) + Expect(graph.NginxProxy.Source).To(Equal(np)) }) It("captures changes for an NginxProxy", func() { processor.CaptureUpsertChange(npUpdated) @@ -1604,7 +1604,7 @@ var _ = Describe("ChangeProcessor", func() { changed, graph := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) - Expect(graph.NginxProxy).To(Equal(npUpdated)) + Expect(graph.NginxProxy.Source).To(Equal(npUpdated)) }) It("handles deletes for an NginxProxy", func() { processor.CaptureDeleteChange(np, client.ObjectKeyFromObject(np)) @@ -1618,8 +1618,10 @@ var _ = Describe("ChangeProcessor", func() { Describe("NGF Policy resource changes", Ordered, func() { var ( gw *v1.Gateway + route *v1.HTTPRoute csp, cspUpdated *ngfAPI.ClientSettingsPolicy - cspKey graph.PolicyKey + obs, obsUpdated *ngfAPI.ObservabilityPolicy + cspKey, obsKey graph.PolicyKey ) BeforeAll(func() { @@ -1630,6 +1632,7 @@ var _ = Describe("ChangeProcessor", func() { Expect(newGraph.NGFPolicies).To(BeEmpty()) gw = createGateway("gw") + route = createRoute("hr-1", "gw", "foo.example.com", v1.HTTPBackendRef{}) csp = &ngfAPI.ClientSettingsPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -1659,6 +1662,37 @@ var _ = Describe("ChangeProcessor", func() { Version: "v1alpha1", }, } + + obs = &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obs", + Namespace: "test", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + TargetRefs: []v1alpha2.LocalPolicyTargetReference{ + { + Group: v1.GroupName, + Kind: kinds.HTTPRoute, + Name: "hr-1", + }, + }, + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + }, + }, + } + + obsUpdated = obs.DeepCopy() + obsUpdated.Spec.Tracing.Strategy = ngfAPI.TraceStrategyParent + + obsKey = graph.PolicyKey{ + NsName: types.NamespacedName{Name: "obs", Namespace: "test"}, + GVK: schema.GroupVersionKind{ + Group: ngfAPI.GroupName, + Kind: kinds.ObservabilityPolicy, + Version: "v1alpha1", + }, + } }) /* @@ -1670,6 +1704,7 @@ var _ = Describe("ChangeProcessor", func() { When("a policy is created that references a resource that is not in the last graph", func() { It("reports no changes", func() { processor.CaptureUpsertChange(csp) + processor.CaptureUpsertChange(obs) changed, _ := processor.Process() Expect(changed).To(Equal(state.NoChange)) @@ -1683,21 +1718,32 @@ var _ = Describe("ChangeProcessor", func() { Expect(changed).To(Equal(state.ClusterStateChange)) Expect(graph.NGFPolicies).To(HaveKey(cspKey)) Expect(graph.NGFPolicies[cspKey].Source).To(Equal(csp)) + Expect(graph.NGFPolicies).ToNot(HaveKey(obsKey)) + + processor.CaptureUpsertChange(route) + changed, graph = processor.Process() + Expect(changed).To(Equal(state.ClusterStateChange)) + Expect(graph.NGFPolicies).To(HaveKey(obsKey)) + Expect(graph.NGFPolicies[obsKey].Source).To(Equal(obs)) }) }) When("the policy is updated", func() { It("captures changes for a policy", func() { processor.CaptureUpsertChange(cspUpdated) + processor.CaptureUpsertChange(obsUpdated) changed, graph := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) Expect(graph.NGFPolicies).To(HaveKey(cspKey)) Expect(graph.NGFPolicies[cspKey].Source).To(Equal(cspUpdated)) + Expect(graph.NGFPolicies).To(HaveKey(obsKey)) + Expect(graph.NGFPolicies[obsKey].Source).To(Equal(obsUpdated)) }) }) 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)) changed, graph := processor.Process() Expect(changed).To(Equal(state.ClusterStateChange)) diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index aa7c73ea3..aa32a3076 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -73,6 +73,18 @@ const ( // GatewayClassReasonParamsRefNotFound is used with the "GatewayClassResolvedRefs" condition when the // parametersRef resource does not exist. GatewayClassReasonParamsRefNotFound v1.GatewayClassConditionReason = "ParametersRefNotFound" + + // PolicyReasonNginxProxyConfigNotSet is used with the "PolicyAccepted" condition when the + // NginxProxy resource is missing or invalid. + PolicyReasonNginxProxyConfigNotSet v1alpha2.PolicyConditionReason = "NginxProxyConfigNotSet" + + // PolicyMessageNginxProxyInvalid is a message used with the PolicyReasonNginxProxyConfigNotSet reason + // when the NginxProxy resource is either invalid or not attached. + PolicyMessageNginxProxyInvalid = "The NginxProxy configuration is either invalid or not attached to the GatewayClass" + + // PolicyMessageTelemetryNotEnabled is a message used with the PolicyReasonNginxProxyConfigNotSet reason + // when telemetry is not enabled in the NginxProxy resource. + PolicyMessageTelemetryNotEnabled = "Telemetry is not enabled in the NginxProxy resource" ) // NewTODO returns a Condition that can be used as a placeholder for a condition that is not yet implemented. @@ -639,3 +651,14 @@ func NewPolicyTargetNotFound(msg string) conditions.Condition { Message: msg, } } + +// NewPolicyNotAcceptedNginxProxyNotSet returns a Condition that indicates that the Policy is not accepted +// because it relies in the NginxProxy configuration which is missing or invalid. +func NewPolicyNotAcceptedNginxProxyNotSet(msg string) conditions.Condition { + return conditions.Condition{ + Type: string(v1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(PolicyReasonNginxProxyConfigNotSet), + Message: msg, + } +} diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index a8ed937ca..be2e39fd6 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -12,8 +12,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" ) @@ -40,7 +42,7 @@ func BuildConfiguration( } upstreams := buildUpstreams(ctx, g.Gateway.Listeners, resolver) - httpServers, sslServers := buildServers(g.Gateway, generator) + httpServers, sslServers := buildServers(g, generator) backendGroups := buildBackendGroups(append(httpServers, sslServers...)) keyPairs := buildSSLKeyPairs(g.ReferencedSecrets, g.Gateway.Listeners) certBundles := buildCertBundles(g.ReferencedCaCertConfigMaps, backendGroups) @@ -202,13 +204,13 @@ func convertBackendTLS(btp *graph.BackendTLSPolicy) *VerifyTLS { return verify } -func buildServers(gw *graph.Gateway, generator policies.ConfigGenerator) (http, ssl []VirtualServer) { +func buildServers(g *graph.Graph, generator policies.ConfigGenerator) (http, ssl []VirtualServer) { rulesForProtocol := map[v1.ProtocolType]portPathRules{ v1.HTTPProtocolType: make(portPathRules), v1.HTTPSProtocolType: make(portPathRules), } - for _, l := range gw.Listeners { + for _, l := range g.Gateway.Listeners { if l.Valid { rules := rulesForProtocol[l.Source.Protocol][l.Source.Port] if rules == nil { @@ -216,7 +218,7 @@ func buildServers(gw *graph.Gateway, generator policies.ConfigGenerator) (http, rulesForProtocol[l.Source.Protocol][l.Source.Port] = rules } - rules.upsertListener(l) + rules.upsertListener(l, g.GlobalSettings) } } @@ -225,7 +227,7 @@ func buildServers(gw *graph.Gateway, generator policies.ConfigGenerator) (http, httpServers, sslServers := httpRules.buildServers(), sslRules.buildServers() - additions := buildAdditions(gw.Policies, generator) + additions := buildAdditions(g.Gateway.Policies, g.GlobalSettings, generator) for i := range httpServers { httpServers[i].Additions = additions @@ -279,7 +281,7 @@ func newHostPathRules(generator policies.ConfigGenerator) *hostPathRules { } } -func (hpr *hostPathRules) upsertListener(l *graph.Listener) { +func (hpr *hostPathRules) upsertListener(l *graph.Listener, globalSettings *policies.GlobalSettings) { hpr.listenersExist = true hpr.port = int32(l.Source.Port) @@ -292,11 +294,15 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) { continue } - hpr.upsertRoute(r, l) + hpr.upsertRoute(r, l, globalSettings) } } -func (hpr *hostPathRules) upsertRoute(route *graph.L7Route, listener *graph.Listener) { +func (hpr *hostPathRules) upsertRoute( + route *graph.L7Route, + listener *graph.Listener, + globalSettings *policies.GlobalSettings, +) { var hostnames []string GRPC := route.RouteType == graph.RouteTypeGRPC @@ -343,7 +349,7 @@ func (hpr *hostPathRules) upsertRoute(route *graph.L7Route, listener *graph.List } } - additions := buildAdditions(route.Policies, hpr.generator) + additions := buildAdditions(route.Policies, globalSettings, hpr.generator) for _, h := range hostnames { for _, m := range rule.Matches { @@ -601,12 +607,14 @@ func generateCertBundleID(configMap types.NamespacedName) CertBundleID { // buildTelemetry generates the Otel configuration. func buildTelemetry(g *graph.Graph) Telemetry { - if g.NginxProxy == nil || g.NginxProxy.Spec.Telemetry == nil || g.NginxProxy.Spec.Telemetry.Exporter == nil { + if g.NginxProxy == nil || !g.NginxProxy.Valid || + g.NginxProxy.Source.Spec.Telemetry == nil || + g.NginxProxy.Source.Spec.Telemetry.Exporter == nil { return Telemetry{} } serviceName := fmt.Sprintf("ngf:%s:%s", g.Gateway.Source.Namespace, g.Gateway.Source.Name) - telemetry := g.NginxProxy.Spec.Telemetry + telemetry := g.NginxProxy.Source.Spec.Telemetry if telemetry.ServiceName != nil { serviceName = serviceName + ":" + *telemetry.ServiceName } @@ -616,17 +624,6 @@ func buildTelemetry(g *graph.Graph) Telemetry { ServiceName: serviceName, } - spanAttrs := make([]SpanAttribute, 0, len(g.NginxProxy.Spec.Telemetry.SpanAttributes)) - for _, spanAttr := range g.NginxProxy.Spec.Telemetry.SpanAttributes { - sa := SpanAttribute{ - Key: spanAttr.Key, - Value: spanAttr.Value, - } - spanAttrs = append(spanAttrs, sa) - } - - tel.SpanAttributes = spanAttrs - if telemetry.Exporter.BatchCount != nil { tel.BatchCount = *telemetry.Exporter.BatchCount } @@ -637,6 +634,24 @@ func buildTelemetry(g *graph.Graph) Telemetry { tel.Interval = string(*telemetry.Exporter.Interval) } + // FIXME(sberman): https://github.com/nginxinc/nginx-gateway-fabric/issues/2038 + // Find a generic way to include relevant policy info at the http context so we don't need policy-specific + // logic in this function + ratioMap := make(map[string]int32) + for _, pol := range g.NGFPolicies { + if obsPol, ok := pol.Source.(*ngfAPI.ObservabilityPolicy); ok { + if obsPol.Spec.Tracing != nil && obsPol.Spec.Tracing.Ratio != nil && *obsPol.Spec.Tracing.Ratio > 0 { + ratioName := observability.CreateRatioVarName(obsPol) + ratioMap[ratioName] = *obsPol.Spec.Tracing.Ratio + } + } + } + + tel.Ratios = make([]Ratio, 0, len(ratioMap)) + for name, ratio := range ratioMap { + tel.Ratios = append(tel.Ratios, Ratio{Name: name, Value: ratio}) + } + return tel } @@ -646,18 +661,22 @@ func buildBaseHTTPConfig(g *graph.Graph) BaseHTTPConfig { // HTTP2 should be enabled by default HTTP2: true, } - if g.NginxProxy == nil { + if g.NginxProxy == nil || !g.NginxProxy.Valid { return baseConfig } - if g.NginxProxy.Spec.DisableHTTP2 { + if g.NginxProxy.Source.Spec.DisableHTTP2 { baseConfig.HTTP2 = false } return baseConfig } -func buildAdditions(policies []*graph.Policy, generator policies.ConfigGenerator) []Addition { +func buildAdditions( + policies []*graph.Policy, + globalSettings *policies.GlobalSettings, + generator policies.ConfigGenerator, +) []Addition { if len(policies) == 0 { return nil } @@ -670,7 +689,7 @@ func buildAdditions(policies []*graph.Policy, generator policies.ConfigGenerator } additions = append(additions, Addition{ - Bytes: generator.Generate(policy.Source), + Bytes: generator.Generate(policy.Source, globalSettings), Identifier: fmt.Sprintf( "%s_%s_%s", policy.Source.GetObjectKind().GroupVersionKind().Kind, diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index 8ea630a20..e053a42d9 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "sort" "testing" . "github.com/onsi/gomega" @@ -619,19 +620,22 @@ func TestBuildConfiguration(t *testing.T) { }, } - nginxProxy := &ngfAPI.NginxProxy{ - Spec: ngfAPI.NginxProxySpec{ - Telemetry: &ngfAPI.Telemetry{ - Exporter: &ngfAPI.TelemetryExporter{ - Endpoint: "my-otel.svc:4563", - BatchSize: helpers.GetPointer(int32(512)), - BatchCount: helpers.GetPointer(int32(4)), - Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + nginxProxy := &graph.NginxProxy{ + Source: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-otel.svc:4563", + BatchSize: helpers.GetPointer(int32(512)), + BatchCount: helpers.GetPointer(int32(4)), + Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + }, + ServiceName: helpers.GetPointer("my-svc"), }, - ServiceName: helpers.GetPointer("my-svc"), + DisableHTTP2: true, }, - DisableHTTP2: true, }, + Valid: true, } tests := []struct { @@ -2061,16 +2065,68 @@ func TestBuildConfiguration(t *testing.T) { CertBundles: map[CertBundleID]CertBundle{}, BaseHTTPConfig: BaseHTTPConfig{HTTP2: false}, Telemetry: Telemetry{ - Endpoint: "my-otel.svc:4563", - Interval: "5s", - BatchSize: 512, - BatchCount: 4, - ServiceName: "ngf:ns:gw:my-svc", - SpanAttributes: []SpanAttribute{}, + Endpoint: "my-otel.svc:4563", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + ServiceName: "ngf:ns:gw:my-svc", + Ratios: []Ratio{}, }, }, msg: "NginxProxy with tracing config and http2 disabled", }, + { + graph: &graph.Graph{ + GatewayClass: &graph.GatewayClass{ + Source: &v1.GatewayClass{}, + Valid: true, + }, + Gateway: &graph.Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "ns", + }, + }, + Listeners: []*graph.Listener{ + { + Name: "listener-80-1", + Source: listener80, + Valid: true, + Routes: map[graph.RouteKey]*graph.L7Route{}, + }, + }, + }, + Routes: map[graph.RouteKey]*graph.L7Route{}, + NginxProxy: &graph.NginxProxy{ + Valid: false, + Source: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + DisableHTTP2: true, + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "some-endpoint", + }, + }, + }, + }, + }, + }, + expConf: Configuration{ + HTTPServers: []VirtualServer{ + { + IsDefault: true, + Port: 80, + }, + }, + SSLServers: []VirtualServer{}, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{}, + CertBundles: map[CertBundleID]CertBundle{}, + BaseHTTPConfig: BaseHTTPConfig{HTTP2: true}, + Telemetry: Telemetry{}, + }, + msg: "invalid NginxProxy", + }, { graph: &graph.Graph{ GatewayClass: &graph.GatewayClass{ @@ -2246,7 +2302,7 @@ func TestBuildConfiguration(t *testing.T) { g := NewWithT(t) fakeGenerator := &policiesfakes.FakeConfigGenerator{ - GenerateStub: func(p policies.Policy) []byte { + GenerateStub: func(p policies.Policy, _ *policies.GlobalSettings) []byte { switch kind := p.GetObjectKind().GroupVersionKind().Kind; kind { case "ApplePolicy": return []byte("apple") @@ -2967,32 +3023,39 @@ func TestConvertBackendTLS(t *testing.T) { } func TestBuildTelemetry(t *testing.T) { - telemetryConfigured := &ngfAPI.NginxProxy{ - Spec: ngfAPI.NginxProxySpec{ - Telemetry: &ngfAPI.Telemetry{ - Exporter: &ngfAPI.TelemetryExporter{ - Endpoint: "my-otel.svc:4563", - BatchSize: helpers.GetPointer(int32(512)), - BatchCount: helpers.GetPointer(int32(4)), - Interval: helpers.GetPointer(ngfAPI.Duration("5s")), - }, - ServiceName: helpers.GetPointer("my-svc"), - SpanAttributes: []ngfAPI.SpanAttribute{ - {Key: "key", Value: "value"}, + telemetryConfigured := &graph.NginxProxy{ + Source: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{ + Endpoint: "my-otel.svc:4563", + BatchSize: helpers.GetPointer(int32(512)), + BatchCount: helpers.GetPointer(int32(4)), + Interval: helpers.GetPointer(ngfAPI.Duration("5s")), + }, + ServiceName: helpers.GetPointer("my-svc"), + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "key", Value: "value"}, + }, }, }, }, + Valid: true, } - expTelemetryConfigured := Telemetry{ - Endpoint: "my-otel.svc:4563", - ServiceName: "ngf:ns:gw:my-svc", - Interval: "5s", - BatchSize: 512, - BatchCount: 4, - SpanAttributes: []SpanAttribute{ - {Key: "key", Value: "value"}, - }, + createTelemetry := func() Telemetry { + return Telemetry{ + Endpoint: "my-otel.svc:4563", + ServiceName: "ngf:ns:gw:my-svc", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + Ratios: []Ratio{}, + } + } + + createModifiedTelemetry := func(mod func(Telemetry) Telemetry) Telemetry { + return mod(createTelemetry()) } tests := []struct { @@ -3002,11 +3065,29 @@ func TestBuildTelemetry(t *testing.T) { }{ { g: &graph.Graph{ - NginxProxy: &ngfAPI.NginxProxy{}, + NginxProxy: &graph.NginxProxy{ + Source: &ngfAPI.NginxProxy{}, + }, }, expTelemetry: Telemetry{}, msg: "No telemetry configured", }, + { + g: &graph.Graph{ + NginxProxy: &graph.NginxProxy{ + Source: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + Exporter: &ngfAPI.TelemetryExporter{}, + }, + }, + }, + Valid: false, + }, + }, + expTelemetry: Telemetry{}, + msg: "Invalid NginxProxy configured", + }, { g: &graph.Graph{ Gateway: &graph.Gateway{ @@ -3019,15 +3100,154 @@ func TestBuildTelemetry(t *testing.T) { }, NginxProxy: telemetryConfigured, }, - expTelemetry: expTelemetryConfigured, + expTelemetry: createTelemetry(), msg: "Telemetry configured", }, + { + g: &graph.Graph{ + Gateway: &graph.Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "ns", + }, + }, + }, + NginxProxy: telemetryConfigured, + NGFPolicies: map[graph.PolicyKey]*graph.Policy{ + {NsName: types.NamespacedName{Name: "obsPolicy"}}: { + Source: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obsPolicy", + Namespace: "custom-ns", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Ratio: helpers.GetPointer[int32](25), + }, + }, + }, + }, + }, + }, + expTelemetry: createModifiedTelemetry(func(t Telemetry) Telemetry { + t.Ratios = []Ratio{ + {Name: "$otel_ratio_25", Value: 25}, + } + return t + }), + msg: "Telemetry configured with observability policy ratio", + }, + { + g: &graph.Graph{ + Gateway: &graph.Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "ns", + }, + }, + }, + NginxProxy: telemetryConfigured, + NGFPolicies: map[graph.PolicyKey]*graph.Policy{ + {NsName: types.NamespacedName{Name: "obsPolicy"}}: { + Source: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obsPolicy", + Namespace: "custom-ns", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Ratio: helpers.GetPointer[int32](25), + }, + }, + }, + }, + {NsName: types.NamespacedName{Name: "obsPolicy2"}}: { + Source: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obsPolicy2", + Namespace: "custom-ns", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Ratio: helpers.GetPointer[int32](50), + }, + }, + }, + }, + {NsName: types.NamespacedName{Name: "obsPolicy3"}}: { + Source: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obsPolicy3", + Namespace: "custom-ns", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Ratio: helpers.GetPointer[int32](25), + }, + }, + }, + }, + {NsName: types.NamespacedName{Name: "csPolicy"}}: { + Source: &ngfAPI.ClientSettingsPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csPolicy", + Namespace: "custom-ns", + }, + }, + }, + }, + }, + expTelemetry: createModifiedTelemetry(func(t Telemetry) Telemetry { + t.Ratios = []Ratio{ + {Name: "$otel_ratio_25", Value: 25}, + {Name: "$otel_ratio_50", Value: 50}, + } + return t + }), + msg: "Multiple policies exist; telemetry ratio is properly set", + }, + { + g: &graph.Graph{ + Gateway: &graph.Gateway{ + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "ns", + }, + }, + }, + NginxProxy: telemetryConfigured, + NGFPolicies: map[graph.PolicyKey]*graph.Policy{ + {NsName: types.NamespacedName{Name: "obsPolicy"}}: { + Source: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "obsPolicy", + Namespace: "custom-ns", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Ratio: helpers.GetPointer[int32](0), + }, + }, + }, + }, + }, + }, + expTelemetry: createTelemetry(), + msg: "Telemetry configured with zero observability policy ratio", + }, } for _, tc := range tests { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - g.Expect(buildTelemetry(tc.g)).To(Equal(tc.expTelemetry)) + tel := buildTelemetry(tc.g) + sort.Slice(tel.Ratios, func(i, j int) bool { + return tel.Ratios[i].Value < tel.Ratios[j].Value + }) + g.Expect(tel).To(Equal(tc.expTelemetry)) }) } } @@ -3109,12 +3329,12 @@ func TestBuildAdditions(t *testing.T) { g := NewWithT(t) generator := &policiesfakes.FakeConfigGenerator{ - GenerateStub: func(policy policies.Policy) []byte { + GenerateStub: func(policy policies.Policy, _ *policies.GlobalSettings) []byte { return []byte(policy.GetName()) }, } - additions := buildAdditions(test.policies, generator) + additions := buildAdditions(test.policies, nil, generator) g.Expect(additions).To(BeEquivalentTo(test.expAdditions)) }) } diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index 7e7feb8f5..77cd66c00 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -278,8 +278,8 @@ type Telemetry struct { ServiceName string // Interval specifies the export interval. Interval string - // SpanAttributes are custom key/value attributes that are added to each span. - SpanAttributes []SpanAttribute + // Ratios is a list of tracing sampling ratios. + Ratios []Ratio // BatchSize specifies the maximum number of spans to be sent in one batch per worker. BatchSize int32 // BatchCount specifies the number of pending batches per worker, spans exceeding the limit are dropped. @@ -299,3 +299,12 @@ type BaseHTTPConfig struct { // HTTP2 specifies whether http2 should be enabled for all servers. HTTP2 bool } + +// Ratio represents a tracing sampling ratio used in an nginx config with the otel_module. +type Ratio struct { + // Name is based on the associated ObservabilityPolicy's NamespacedName, + // and is used as the nginx variable name for this ratio. + Name string + // Value is the value of the ratio. + Value int32 +} diff --git a/internal/mode/static/state/graph/backend_tls_policy.go b/internal/mode/static/state/graph/backend_tls_policy.go index 1545abf68..5e2169cfb 100644 --- a/internal/mode/static/state/graph/backend_tls_policy.go +++ b/internal/mode/static/state/graph/backend_tls_policy.go @@ -74,7 +74,7 @@ func validateBackendTLSPolicy( ignored = false // FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987 - if ancestorsFull(backendTLSPolicy.Status.Ancestors, ctlrName) { + if backendTLSPolicyAncestorsFull(backendTLSPolicy.Status.Ancestors, ctlrName) { valid = false ignored = true } diff --git a/internal/mode/static/state/graph/gatewayclass.go b/internal/mode/static/state/graph/gatewayclass.go index d54e14e62..9fc1f6725 100644 --- a/internal/mode/static/state/graph/gatewayclass.go +++ b/internal/mode/static/state/graph/gatewayclass.go @@ -9,12 +9,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" v1 "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/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) // GatewayClass represents the GatewayClass resource. @@ -65,15 +63,14 @@ func processGatewayClasses( func buildGatewayClass( gc *v1.GatewayClass, - npCfg *ngfAPI.NginxProxy, + npCfg *NginxProxy, crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, - validator validation.GenericValidator, ) *GatewayClass { if gc == nil { return nil } - conds, valid := validateGatewayClass(gc, npCfg, crdVersions, validator) + conds, valid := validateGatewayClass(gc, npCfg, crdVersions) return &GatewayClass{ Source: gc, @@ -84,9 +81,8 @@ func buildGatewayClass( func validateGatewayClass( gc *v1.GatewayClass, - npCfg *ngfAPI.NginxProxy, + npCfg *NginxProxy, crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata, - validator validation.GenericValidator, ) ([]conditions.Condition, bool) { var conds []conditions.Condition @@ -98,11 +94,8 @@ func validateGatewayClass( } else if npCfg == nil { err = field.NotFound(path.Child("name"), gc.Spec.ParametersRef.Name) conds = append(conds, staticConds.NewGatewayClassRefNotFound()) - } else { - nginxProxyErrs := validateNginxProxy(validator, npCfg) - if len(nginxProxyErrs) > 0 { - err = errors.New(nginxProxyErrs.ToAggregate().Error()) - } + } else if !npCfg.Valid { + err = errors.New(npCfg.ErrMsgs.ToAggregate().Error()) } if err != nil { diff --git a/internal/mode/static/state/graph/gatewayclass_test.go b/internal/mode/static/state/graph/gatewayclass_test.go index 5ec28371a..1b10997e4 100644 --- a/internal/mode/static/state/graph/gatewayclass_test.go +++ b/internal/mode/static/state/graph/gatewayclass_test.go @@ -1,12 +1,12 @@ package graph import ( - "errors" "testing" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -16,8 +16,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) func TestProcessGatewayClasses(t *testing.T) { @@ -166,29 +164,13 @@ func TestBuildGatewayClass(t *testing.T) { }, } - createValidNPValidator := func() *validationfakes.FakeGenericValidator { - v := &validationfakes.FakeGenericValidator{} - v.ValidateServiceNameReturns(nil) - v.ValidateEndpointReturns(nil) - - return v - } - - createInvalidNPValidator := func() *validationfakes.FakeGenericValidator { - v := &validationfakes.FakeGenericValidator{} - v.ValidateServiceNameReturns(errors.New("error")) - v.ValidateEndpointReturns(errors.New("error")) - - return v - } - tests := []struct { - gc *v1.GatewayClass - np *ngfAPI.NginxProxy - validator validation.GenericValidator - crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata - expected *GatewayClass - name string + gc *v1.GatewayClass + np *NginxProxy + crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata + expected *GatewayClass + name string + expNPInvalid bool }{ { gc: validGC, @@ -206,17 +188,19 @@ func TestBuildGatewayClass(t *testing.T) { }, { gc: gcWithParams, - np: &ngfAPI.NginxProxy{ - TypeMeta: metav1.TypeMeta{ - Kind: kinds.NginxProxy, - }, - Spec: ngfAPI.NginxProxySpec{ - Telemetry: &ngfAPI.Telemetry{ - ServiceName: helpers.GetPointer("my-svc"), + np: &NginxProxy{ + Source: &ngfAPI.NginxProxy{ + TypeMeta: metav1.TypeMeta{ + Kind: kinds.NginxProxy, + }, + Spec: ngfAPI.NginxProxySpec{ + Telemetry: &ngfAPI.Telemetry{ + ServiceName: helpers.GetPointer("my-svc"), + }, }, }, + Valid: true, }, - validator: createValidNPValidator(), expected: &GatewayClass{ Source: gcWithParams, Valid: true, @@ -226,10 +210,13 @@ func TestBuildGatewayClass(t *testing.T) { }, { gc: gcWithInvalidKind, - np: &ngfAPI.NginxProxy{ - TypeMeta: metav1.TypeMeta{ - Kind: kinds.NginxProxy, + np: &NginxProxy{ + Source: &ngfAPI.NginxProxy{ + TypeMeta: metav1.TypeMeta{ + Kind: kinds.NginxProxy, + }, }, + Valid: true, }, expected: &GatewayClass{ Source: gcWithInvalidKind, @@ -254,24 +241,26 @@ func TestBuildGatewayClass(t *testing.T) { ), }, }, - name: "invalid gatewayclass with paramsRef resource that doesn't exist", + expNPInvalid: true, + name: "invalid gatewayclass with paramsRef resource that doesn't exist", }, { gc: gcWithParams, - np: &ngfAPI.NginxProxy{ - TypeMeta: metav1.TypeMeta{ - Kind: kinds.NginxProxy, - }, - Spec: ngfAPI.NginxProxySpec{ - Telemetry: &ngfAPI.Telemetry{ - ServiceName: helpers.GetPointer("my-svc"), - Exporter: &ngfAPI.TelemetryExporter{ - Endpoint: "my-endpoint", - }, - }, + np: &NginxProxy{ + Valid: false, + ErrMsgs: field.ErrorList{ + field.Invalid( + field.NewPath("spec", "telemetry", "serviceName"), + "my-svc", + "error", + ), + field.Invalid( + field.NewPath("spec", "telemetry", "exporter", "endpoint"), + "my-endpoint", + "error", + ), }, }, - validator: createInvalidNPValidator(), expected: &GatewayClass{ Source: gcWithParams, Valid: true, @@ -282,7 +271,8 @@ func TestBuildGatewayClass(t *testing.T) { ), }, }, - name: "invalid gatewayclass with invalid paramsRef resource", + expNPInvalid: true, + name: "invalid gatewayclass with invalid paramsRef resource", }, { gc: validGC, @@ -300,8 +290,11 @@ func TestBuildGatewayClass(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - result := buildGatewayClass(test.gc, test.np, test.crdMetadata, test.validator) + result := buildGatewayClass(test.gc, test.np, test.crdMetadata) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) + if test.np != nil { + g.Expect(test.np.Valid).ToNot(Equal(test.expNPInvalid)) + } }) } } diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index 610ea4dc4..050ff3409 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -67,9 +67,12 @@ type Graph struct { // BackendTLSPolicies holds BackendTLSPolicy resources. BackendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy // NginxProxy holds the NginxProxy config for the GatewayClass. - NginxProxy *ngfAPI.NginxProxy + NginxProxy *NginxProxy // NGFPolicies holds all NGF Policies. NGFPolicies map[PolicyKey]*Policy + // 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. + GlobalSettings *policies.GlobalSettings } // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. @@ -138,14 +141,13 @@ func (g *Graph) IsNGFPolicyRelevant( panic("policy cannot be nil") } - ref := policy.GetTargetRef() - - switch ref.Group { - case gatewayv1.GroupName: - return g.gatewayAPIResourceExist(ref, policy.GetNamespace()) - default: - return false + for _, ref := range policy.GetTargetRefs() { + if ref.Group == gatewayv1.GroupName && g.gatewayAPIResourceExist(ref, policy.GetNamespace()) { + return true + } } + + return false } func (g *Graph) gatewayAPIResourceExist(ref v1alpha2.LocalPolicyTargetReference, policyNs string) bool { @@ -175,14 +177,27 @@ func BuildGraph( validators validation.Validators, protectedPorts ProtectedPorts, ) *Graph { + var globalSettings *policies.GlobalSettings + processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { // configured GatewayClass does not reference this controller return &Graph{} } - npCfg := getNginxProxy(state.NginxProxies, processedGwClasses.Winner) - gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata, validators.GenericValidator) + npCfg := buildNginxProxy(state.NginxProxies, processedGwClasses.Winner, validators.GenericValidator) + gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata) + if gc != nil && npCfg != nil && npCfg.Source != nil { + spec := npCfg.Source.Spec + globalSettings = &policies.GlobalSettings{ + NginxProxyValid: npCfg.Valid, + TelemetryEnabled: spec.Telemetry != nil && spec.Telemetry.Exporter != nil, + } + + if spec.Telemetry != nil { + globalSettings.TracingSpanAttributes = spec.Telemetry.SpanAttributes + } + } secretResolver := newSecretResolver(state.Secrets) configMapResolver := newConfigMapResolver(state.ConfigMaps) @@ -215,7 +230,14 @@ func BuildGraph( referencedServices := buildReferencedServices(routes) - processedPolicies := processPolicies(state.NGFPolicies, validators.PolicyValidator, processedGws, routes) + // policies must be processed last because they rely on the state of the other resources in the graph + processedPolicies := processPolicies( + state.NGFPolicies, + validators.PolicyValidator, + processedGws, + routes, + globalSettings, + ) g := &Graph{ GatewayClass: gc, @@ -230,6 +252,7 @@ func BuildGraph( BackendTLSPolicies: processedBackendTLSPolicies, NginxProxy: npCfg, NGFPolicies: processedPolicies, + GlobalSettings: globalSettings, } g.attachPolicies(controllerName) diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index e8097270c..7c3bc3ea0 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -341,6 +341,9 @@ func TestBuildGraph(t *testing.T) { BatchCount: helpers.GetPointer(int32(4)), }, ServiceName: helpers.GetPointer("my-svc"), + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "key", Value: "value"}, + }, }, }, } @@ -364,18 +367,22 @@ func TestBuildGraph(t *testing.T) { } processedRoutePolicy := &Policy{ Source: hrPolicy, - Ancestor: &PolicyAncestor{ - Ancestor: gatewayv1.ParentReference{ - Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), - Kind: helpers.GetPointer[gatewayv1.Kind](kinds.HTTPRoute), - Namespace: (*gatewayv1.Namespace)(&testNs), - Name: "hr-1", + Ancestors: []PolicyAncestor{ + { + Ancestor: gatewayv1.ParentReference{ + Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.HTTPRoute), + Namespace: (*gatewayv1.Namespace)(&testNs), + Name: "hr-1", + }, }, }, - TargetRef: PolicyTargetRef{ - Kind: kinds.HTTPRoute, - Group: gatewayv1.GroupName, - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-1"}, + TargetRefs: []PolicyTargetRef{ + { + Kind: kinds.HTTPRoute, + Group: gatewayv1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-1"}, + }, }, Valid: true, } @@ -393,18 +400,22 @@ func TestBuildGraph(t *testing.T) { } processedGwPolicy := &Policy{ Source: gwPolicy, - Ancestor: &PolicyAncestor{ - Ancestor: gatewayv1.ParentReference{ - Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), - Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Gateway), - Namespace: (*gatewayv1.Namespace)(&testNs), - Name: "gateway-1", + Ancestors: []PolicyAncestor{ + { + Ancestor: gatewayv1.ParentReference{ + Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Gateway), + Namespace: (*gatewayv1.Namespace)(&testNs), + Name: "gateway-1", + }, }, }, - TargetRef: PolicyTargetRef{ - Kind: kinds.Gateway, - Group: gatewayv1.GroupName, - Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway-1"}, + TargetRefs: []PolicyTargetRef{ + { + Kind: kinds.Gateway, + Group: gatewayv1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway-1"}, + }, }, Valid: true, } @@ -587,11 +598,21 @@ func TestBuildGraph(t *testing.T) { BackendTLSPolicies: map[types.NamespacedName]*BackendTLSPolicy{ client.ObjectKeyFromObject(btp.Source): &btp, }, - NginxProxy: proxy, + NginxProxy: &NginxProxy{ + Source: proxy, + Valid: true, + }, NGFPolicies: map[PolicyKey]*Policy{ hrPolicyKey: processedRoutePolicy, gwPolicyKey: processedGwPolicy, }, + GlobalSettings: &policies.GlobalSettings{ + NginxProxyValid: true, + TelemetryEnabled: true, + TracingSpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "key", Value: "value"}, + }, + }, } } @@ -638,6 +659,8 @@ func TestBuildGraph(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) + fakePolicyValidator := &validationfakes.FakePolicyValidator{} + result := BuildGraph( test.store, controllerName, @@ -645,7 +668,7 @@ func TestBuildGraph(t *testing.T) { validation.Validators{ HTTPFieldsValidator: &validationfakes.FakeHTTPFieldsValidator{}, GenericValidator: &validationfakes.FakeGenericValidator{}, - PolicyValidator: &validationfakes.FakePolicyValidator{}, + PolicyValidator: fakePolicyValidator, }, protectedPorts, ) @@ -1001,8 +1024,8 @@ func TestIsNGFPolicyRelevant(t *testing.T) { GetNamespaceStub: func() string { return testNs }, - GetTargetRefStub: func() v1alpha2.LocalPolicyTargetReference { - return ref + GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{ref} }, } } diff --git a/internal/mode/static/state/graph/grpcroute_test.go b/internal/mode/static/state/graph/grpcroute_test.go index 0a732dedd..ed8e45585 100644 --- a/internal/mode/static/state/graph/grpcroute_test.go +++ b/internal/mode/static/state/graph/grpcroute_test.go @@ -127,9 +127,11 @@ func TestBuildGRPCRoutes(t *testing.T) { validator := &validationfakes.FakeHTTPFieldsValidator{} - npCfg := &ngfAPI.NginxProxy{ - Spec: ngfAPI.NginxProxySpec{ - DisableHTTP2: false, + npCfg := &NginxProxy{ + Source: &ngfAPI.NginxProxy{ + Spec: ngfAPI.NginxProxySpec{ + DisableHTTP2: false, + }, }, } diff --git a/internal/mode/static/state/graph/nginxproxy.go b/internal/mode/static/state/graph/nginxproxy.go index 7df28d261..29cebdbea 100644 --- a/internal/mode/static/state/graph/nginxproxy.go +++ b/internal/mode/static/state/graph/nginxproxy.go @@ -10,13 +10,33 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) -// getNginxProxy returns the NginxProxy associated with the GatewayClass (if it exists). -func getNginxProxy( +// NginxProxy represents the NginxProxy resource. +type NginxProxy struct { + // Source is the source resource. + Source *ngfAPI.NginxProxy + // ErrMsgs contains the validation errors if they exist, to be included in the GatewayClass condition. + ErrMsgs field.ErrorList + // Valid shows whether the NginxProxy is valid. + Valid bool +} + +// buildNginxProxy validates and returns the NginxProxy associated with the GatewayClass (if it exists). +func buildNginxProxy( nps map[types.NamespacedName]*ngfAPI.NginxProxy, gc *v1.GatewayClass, -) *ngfAPI.NginxProxy { + validator validation.GenericValidator, +) *NginxProxy { if gcReferencesAnyNginxProxy(gc) { - return nps[types.NamespacedName{Name: gc.Spec.ParametersRef.Name}] + npCfg := nps[types.NamespacedName{Name: gc.Spec.ParametersRef.Name}] + if npCfg != nil { + errs := validateNginxProxy(validator, npCfg) + + return &NginxProxy{ + Source: npCfg, + Valid: len(errs) == 0, + ErrMsgs: errs, + } + } } return nil diff --git a/internal/mode/static/state/graph/nginxproxy_test.go b/internal/mode/static/state/graph/nginxproxy_test.go index 6c0f3d52b..1aed0913f 100644 --- a/internal/mode/static/state/graph/nginxproxy_test.go +++ b/internal/mode/static/state/graph/nginxproxy_test.go @@ -19,7 +19,7 @@ func TestGetNginxProxy(t *testing.T) { tests := []struct { nps map[types.NamespacedName]*ngfAPI.NginxProxy gc *v1.GatewayClass - expNP *ngfAPI.NginxProxy + expNP *NginxProxy name string }{ { @@ -66,10 +66,13 @@ func TestGetNginxProxy(t *testing.T) { }, }, }, - expNP: &ngfAPI.NginxProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "np2", + expNP: &NginxProxy{ + Source: &ngfAPI.NginxProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "np2", + }, }, + Valid: true, }, name: "returns correct resource", }, @@ -79,7 +82,7 @@ func TestGetNginxProxy(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - g.Expect(getNginxProxy(test.nps, test.gc)).To(Equal(test.expNP)) + g.Expect(buildNginxProxy(test.nps, test.gc, &validationfakes.FakeGenericValidator{})).To(Equal(test.expNP)) }) } } diff --git a/internal/mode/static/state/graph/policies.go b/internal/mode/static/state/graph/policies.go index e8cbb50a5..bd4fa29a4 100644 --- a/internal/mode/static/state/graph/policies.go +++ b/internal/mode/static/state/graph/policies.go @@ -21,10 +21,10 @@ import ( type Policy struct { // Source is the corresponding Policy resource. Source policies.Policy - // Ancestor is the ancestor object of the Policy. Used in status. - Ancestor *PolicyAncestor - // TargetRef is the resource that the Policy targets. - TargetRef PolicyTargetRef + // Ancestors is list of ancestor objects of the Policy. Used in status. + Ancestors []PolicyAncestor + // TargetRefs are the resources that the Policy targets. + TargetRefs []PolicyTargetRef // Conditions holds the conditions for the Policy. // These conditions apply to the entire Policy. // The conditions in the Ancestor apply only to the Policy in regard to the Ancestor. @@ -72,18 +72,18 @@ func (g *Graph) attachPolicies(ctlrName string) { } for _, policy := range g.NGFPolicies { - ref := policy.TargetRef - - switch ref.Kind { - case kinds.Gateway: - attachPolicyToGateway(policy, g.Gateway, g.IgnoredGateways, ctlrName) - case kinds.HTTPRoute, kinds.GRPCRoute: - route, exists := g.Routes[routeKeyForKind(ref.Kind, ref.Nsname)] - if !exists { - continue - } + for _, ref := range policy.TargetRefs { + switch ref.Kind { + case kinds.Gateway: + attachPolicyToGateway(policy, ref, g.Gateway, g.IgnoredGateways, ctlrName) + case kinds.HTTPRoute, kinds.GRPCRoute: + route, exists := g.Routes[routeKeyForKind(ref.Kind, ref.Nsname)] + if !exists { + continue + } - attachPolicyToRoute(policy, route, ctlrName) + attachPolicyToRoute(policy, route, ctlrName) + } } } } @@ -96,63 +96,60 @@ func attachPolicyToRoute(policy *Policy, route *L7Route, ctlrName string) { routeNsName := types.NamespacedName{Namespace: route.Source.GetNamespace(), Name: route.Source.GetName()} - ancestor := &PolicyAncestor{ + ancestor := PolicyAncestor{ Ancestor: createParentReference(v1.GroupName, kind, routeNsName), } - curAncestorStatus := policy.Source.GetPolicyStatus().Ancestors - if ancestorsFull(curAncestorStatus, ctlrName) { + if ngfPolicyAncestorsFull(policy, ctlrName) { // FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987 return } - policy.Ancestor = ancestor - if !route.Valid || !route.Attachable || len(route.ParentRefs) == 0 { - policy.Ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")} - + ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")} + policy.Ancestors = append(policy.Ancestors, ancestor) return } + policy.Ancestors = append(policy.Ancestors, ancestor) route.Policies = append(route.Policies, policy) } func attachPolicyToGateway( policy *Policy, + ref PolicyTargetRef, gw *Gateway, ignoredGateways map[types.NamespacedName]*v1.Gateway, ctlrName string, ) { - ref := policy.TargetRef - _, ignored := ignoredGateways[ref.Nsname] if !ignored && ref.Nsname != client.ObjectKeyFromObject(gw.Source) { return } - ancestor := &PolicyAncestor{ + ancestor := PolicyAncestor{ Ancestor: createParentReference(v1.GroupName, kinds.Gateway, ref.Nsname), } - curAncestorStatus := policy.Source.GetPolicyStatus().Ancestors - if ancestorsFull(curAncestorStatus, ctlrName) { + if ngfPolicyAncestorsFull(policy, ctlrName) { // FIXME (kate-osborn): https://github.com/nginxinc/nginx-gateway-fabric/issues/1987 return } - policy.Ancestor = ancestor - if ignored { - policy.Ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is ignored")} + ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is ignored")} + policy.Ancestors = append(policy.Ancestors, ancestor) return } if !gw.Valid { - policy.Ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")} + ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")} + policy.Ancestors = append(policy.Ancestors, ancestor) return } + policy.Ancestors = append(policy.Ancestors, ancestor) gw.Policies = append(gw.Policies, policy) } @@ -161,6 +158,7 @@ func processPolicies( validator validation.PolicyValidator, gateways processedGateways, routes map[RouteKey]*L7Route, + globalSettings *policies.GlobalSettings, ) map[PolicyKey]*Policy { if len(policies) == 0 || gateways.Winner == nil { return nil @@ -169,39 +167,45 @@ func processPolicies( processedPolicies := make(map[PolicyKey]*Policy) for key, policy := range policies { - ref := policy.GetTargetRef() - refNsName := types.NamespacedName{Name: string(ref.Name), Namespace: policy.GetNamespace()} + targetRefs := make([]PolicyTargetRef, 0, len(policy.GetTargetRefs())) - refGroupKind := fmt.Sprintf("%s/%s", ref.Group, ref.Kind) + 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 { - case gatewayGroupKind: - if !gatewayExists(refNsName, gateways.Winner, gateways.Ignored) { - continue - } - case hrGroupKind, grpcGroupKind: - if _, exists := routes[routeKeyForKind(ref.Kind, refNsName)]; !exists { + switch refGroupKind { + case gatewayGroupKind: + if !gatewayExists(refNsName, gateways.Winner, gateways.Ignored) { + continue + } + case hrGroupKind, grpcGroupKind: + if _, exists := routes[routeKeyForKind(ref.Kind, refNsName)]; !exists { + continue + } + default: continue } - default: - continue - } - var conds []conditions.Condition + targetRefs = append(targetRefs, + PolicyTargetRef{ + Kind: ref.Kind, + Group: ref.Group, + Nsname: refNsName, + }) + } - if err := validator.Validate(policy); err != nil { - conds = append(conds, staticConds.NewPolicyInvalid(err.Error())) + if len(targetRefs) == 0 { + continue } + conds := validator.Validate(policy, globalSettings) + processedPolicies[key] = &Policy{ Source: policy, Valid: len(conds) == 0, Conditions: conds, - TargetRef: PolicyTargetRef{ - Kind: ref.Kind, - Group: ref.Group, - Nsname: refNsName, - }, + TargetRefs: targetRefs, + Ancestors: make([]PolicyAncestor, 0, len(targetRefs)), } } @@ -213,7 +217,7 @@ func processPolicies( // markConflictedPolicies marks policies that conflict with a policy of greater precedence as invalid. // Policies are sorted by timestamp and then alphabetically. func markConflictedPolicies(policies map[PolicyKey]*Policy, validator validation.PolicyValidator) { - // Policies can only conflict if they are the same policy type (gvk) and they target the same resource. + // Policies can only conflict if they are the same policy type (gvk) and they target the same resource(s). type key struct { policyGVK schema.GroupVersionKind PolicyTargetRef @@ -221,17 +225,19 @@ func markConflictedPolicies(policies map[PolicyKey]*Policy, validator validation possibles := make(map[key][]*Policy) - for pk, p := range policies { + for policyKey, policy := range policies { // If a policy is invalid, it cannot conflict with another policy. - if p.Valid { - ak := key{ - PolicyTargetRef: p.TargetRef, - policyGVK: pk.GVK, - } - if possibles[ak] == nil { - possibles[ak] = make([]*Policy, 0) + if policy.Valid { + for _, ref := range policy.TargetRefs { + ak := key{ + PolicyTargetRef: ref, + policyGVK: policyKey.GVK, + } + if possibles[ak] == nil { + possibles[ak] = make([]*Policy, 0) + } + possibles[ak] = append(possibles[ak], policy) } - possibles[ak] = append(possibles[ak], p) } } diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index a2ee60204..b41d66353 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/internal/mode/static/state/graph/policies_test.go @@ -1,7 +1,6 @@ package graph import ( - "errors" "slices" "testing" @@ -30,10 +29,17 @@ func TestAttachPolicies(t *testing.T) { gwPolicy := &Policy{ Valid: true, Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Kind: kinds.Gateway, - Group: v1.GroupName, - Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway"}, + TargetRefs: []PolicyTargetRef{ + { + Kind: kinds.Gateway, + Group: v1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway"}, + }, + { + Kind: kinds.Gateway, + Group: v1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "gateway2"}, // ignored + }, }, } @@ -41,10 +47,17 @@ func TestAttachPolicies(t *testing.T) { routePolicy := &Policy{ Valid: true, Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-route"}, + TargetRefs: []PolicyTargetRef{ + { + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-route"}, + }, + { + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr2-route"}, + }, }, } @@ -52,10 +65,12 @@ func TestAttachPolicies(t *testing.T) { grpcRoutePolicy := &Policy{ Valid: true, Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Kind: kinds.GRPCRoute, - Group: v1.GroupName, - Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc-route"}, + TargetRefs: []PolicyTargetRef{ + { + Kind: kinds.GRPCRoute, + Group: v1.GroupName, + Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc-route"}, + }, }, } @@ -101,6 +116,23 @@ func TestAttachPolicies(t *testing.T) { Valid: true, Attachable: true, }, + createRouteKey("hr2-route", RouteTypeHTTP): { + Source: &v1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hr2-route", + Namespace: testNs, + }, + }, + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: true, + Attachable: true, + }, createRouteKey("grpc-route", RouteTypeGRPC): { Source: &v1alpha2.GRPCRoute{ ObjectMeta: metav1.ObjectMeta{ @@ -243,18 +275,18 @@ func TestAttachPolicyToRoute(t *testing.T) { } tests := []struct { - route *L7Route - policy *Policy - expAncestor *PolicyAncestor - name string - expAttached bool + route *L7Route + policy *Policy + name string + expAncestors []PolicyAncestor + expAttached bool }{ { name: "policy attaches to http route", route: createHTTPRoute(true /*valid*/, true /*attachable*/, true /*parentRefs*/), policy: &Policy{Source: &policiesfakes.FakePolicy{}}, - expAncestor: &PolicyAncestor{ - Ancestor: createExpAncestor(kinds.HTTPRoute), + expAncestors: []PolicyAncestor{ + {Ancestor: createExpAncestor(kinds.HTTPRoute)}, }, expAttached: true, }, @@ -262,8 +294,23 @@ func TestAttachPolicyToRoute(t *testing.T) { name: "policy attaches to grpc route", route: createGRPCRoute(true /*valid*/, true /*attachable*/, true /*parentRefs*/), policy: &Policy{Source: &policiesfakes.FakePolicy{}}, - expAncestor: &PolicyAncestor{ - Ancestor: createExpAncestor(kinds.GRPCRoute), + expAncestors: []PolicyAncestor{ + {Ancestor: createExpAncestor(kinds.GRPCRoute)}, + }, + expAttached: true, + }, + { + name: "attachment with existing ancestor", + route: createHTTPRoute(true /*valid*/, true /*attachable*/, true /*parentRefs*/), + policy: &Policy{ + Source: &policiesfakes.FakePolicy{}, + Ancestors: []PolicyAncestor{ + {Ancestor: createExpAncestor(kinds.HTTPRoute)}, + }, + }, + expAncestors: []PolicyAncestor{ + {Ancestor: createExpAncestor(kinds.HTTPRoute)}, + {Ancestor: createExpAncestor(kinds.HTTPRoute)}, }, expAttached: true, }, @@ -271,9 +318,11 @@ func TestAttachPolicyToRoute(t *testing.T) { name: "no attachment; unattachable route", route: createHTTPRoute(true /*valid*/, false /*attachable*/, true /*parentRefs*/), policy: &Policy{Source: &policiesfakes.FakePolicy{}}, - expAncestor: &PolicyAncestor{ - Ancestor: createExpAncestor(kinds.HTTPRoute), - Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + expAncestors: []PolicyAncestor{ + { + Ancestor: createExpAncestor(kinds.HTTPRoute), + Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + }, }, expAttached: false, }, @@ -281,9 +330,11 @@ func TestAttachPolicyToRoute(t *testing.T) { name: "no attachment; missing parentRefs", route: createHTTPRoute(true /*valid*/, true /*attachable*/, false /*parentRefs*/), policy: &Policy{Source: &policiesfakes.FakePolicy{}}, - expAncestor: &PolicyAncestor{ - Ancestor: createExpAncestor(kinds.HTTPRoute), - Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + expAncestors: []PolicyAncestor{ + { + Ancestor: createExpAncestor(kinds.HTTPRoute), + Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + }, }, expAttached: false, }, @@ -291,18 +342,20 @@ func TestAttachPolicyToRoute(t *testing.T) { name: "no attachment; invalid route", route: createHTTPRoute(false /*valid*/, true /*attachable*/, true /*parentRefs*/), policy: &Policy{Source: &policiesfakes.FakePolicy{}}, - expAncestor: &PolicyAncestor{ - Ancestor: createExpAncestor(kinds.HTTPRoute), - Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + expAncestors: []PolicyAncestor{ + { + Ancestor: createExpAncestor(kinds.HTTPRoute), + Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + }, }, expAttached: false, }, { - name: "no attachment; max ancestors", - route: createHTTPRoute(true /*valid*/, true /*attachable*/, true /*parentRefs*/), - policy: &Policy{Source: createTestPolicyWithAncestors(16)}, - expAncestor: nil, - expAttached: false, + name: "no attachment; max ancestors", + route: createHTTPRoute(true /*valid*/, true /*attachable*/, true /*parentRefs*/), + policy: &Policy{Source: createTestPolicyWithAncestors(16)}, + expAncestors: nil, + expAttached: false, }, } @@ -318,7 +371,7 @@ func TestAttachPolicyToRoute(t *testing.T) { g.Expect(test.route.Policies).To(BeEmpty()) } - g.Expect(test.policy.Ancestor).To(BeEquivalentTo(test.expAncestor)) + g.Expect(test.policy.Ancestors).To(BeEquivalentTo(test.expAncestors)) }) } } @@ -350,24 +403,47 @@ func TestAttachPolicyToGateway(t *testing.T) { } tests := []struct { - policy *Policy - gw *Gateway - expAncestor *PolicyAncestor - name string - expAttached bool + policy *Policy + gw *Gateway + name string + expAncestors []PolicyAncestor + expAttached bool }{ { name: "attached", policy: &Policy{ Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Nsname: gatewayNsName, - Kind: "Gateway", + TargetRefs: []PolicyTargetRef{ + { + Nsname: gatewayNsName, + Kind: "Gateway", + }, }, }, gw: newGateway(true, gatewayNsName), - expAncestor: &PolicyAncestor{ - Ancestor: getGatewayParentRef(gatewayNsName), + expAncestors: []PolicyAncestor{ + {Ancestor: getGatewayParentRef(gatewayNsName)}, + }, + expAttached: true, + }, + { + name: "attached with existing ancestor", + policy: &Policy{ + Source: &policiesfakes.FakePolicy{}, + TargetRefs: []PolicyTargetRef{ + { + Nsname: gatewayNsName, + Kind: "Gateway", + }, + }, + Ancestors: []PolicyAncestor{ + {Ancestor: getGatewayParentRef(gatewayNsName)}, + }, + }, + gw: newGateway(true, gatewayNsName), + expAncestors: []PolicyAncestor{ + {Ancestor: getGatewayParentRef(gatewayNsName)}, + {Ancestor: getGatewayParentRef(gatewayNsName)}, }, expAttached: true, }, @@ -375,15 +451,19 @@ func TestAttachPolicyToGateway(t *testing.T) { name: "not attached; gateway ignored", policy: &Policy{ Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Nsname: ignoredGatewayNsName, - Kind: "Gateway", + TargetRefs: []PolicyTargetRef{ + { + Nsname: ignoredGatewayNsName, + Kind: "Gateway", + }, }, }, gw: newGateway(true, gatewayNsName), - expAncestor: &PolicyAncestor{ - Ancestor: getGatewayParentRef(ignoredGatewayNsName), - Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is ignored")}, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(ignoredGatewayNsName), + Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is ignored")}, + }, }, expAttached: false, }, @@ -391,15 +471,19 @@ func TestAttachPolicyToGateway(t *testing.T) { name: "not attached; invalid gateway", policy: &Policy{ Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Nsname: gatewayNsName, - Kind: "Gateway", + TargetRefs: []PolicyTargetRef{ + { + Nsname: gatewayNsName, + Kind: "Gateway", + }, }, }, gw: newGateway(false, gatewayNsName), - expAncestor: &PolicyAncestor{ - Ancestor: getGatewayParentRef(gatewayNsName), - Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gatewayNsName), + Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("TargetRef is invalid")}, + }, }, expAttached: false, }, @@ -407,27 +491,31 @@ func TestAttachPolicyToGateway(t *testing.T) { name: "not attached; non-NGF gateway", policy: &Policy{ Source: &policiesfakes.FakePolicy{}, - TargetRef: PolicyTargetRef{ - Nsname: gateway2NsName, - Kind: "Gateway", + TargetRefs: []PolicyTargetRef{ + { + Nsname: gateway2NsName, + Kind: "Gateway", + }, }, }, - gw: newGateway(true, gatewayNsName), - expAncestor: nil, - expAttached: false, + gw: newGateway(true, gatewayNsName), + expAncestors: nil, + expAttached: false, }, { name: "not attached; max ancestors", policy: &Policy{ Source: createTestPolicyWithAncestors(16), - TargetRef: PolicyTargetRef{ - Nsname: gatewayNsName, - Kind: "Gateway", + TargetRefs: []PolicyTargetRef{ + { + Nsname: gatewayNsName, + Kind: "Gateway", + }, }, }, - gw: newGateway(true, gatewayNsName), - expAncestor: nil, - expAttached: false, + gw: newGateway(true, gatewayNsName), + expAncestors: nil, + expAttached: false, }, } @@ -439,7 +527,7 @@ func TestAttachPolicyToGateway(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - attachPolicyToGateway(test.policy, test.gw, ignoredGateways, "nginx-gateway") + attachPolicyToGateway(test.policy, test.policy.TargetRefs[0], test.gw, ignoredGateways, "nginx-gateway") if test.expAttached { g.Expect(test.gw.Policies).To(HaveLen(1)) @@ -447,7 +535,7 @@ func TestAttachPolicyToGateway(t *testing.T) { g.Expect(test.gw.Policies).To(BeEmpty()) } - g.Expect(test.policy.Ancestor).To(BeEquivalentTo(test.expAncestor)) + g.Expect(test.policy.Ancestors).To(BeEquivalentTo(test.expAncestors)) }) } } @@ -508,48 +596,63 @@ func TestProcessPolicies(t *testing.T) { expProcessedPolicies: map[PolicyKey]*Policy{ pol1Key: { Source: pol1, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, }, - Valid: true, + Ancestors: []PolicyAncestor{}, + Valid: true, }, pol2Key: { Source: pol2, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc"}, - Kind: kinds.GRPCRoute, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc"}, + Kind: kinds.GRPCRoute, + Group: v1.GroupName, + }, }, - Valid: true, + Ancestors: []PolicyAncestor{}, + Valid: true, }, pol3Key: { Source: pol3, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "gw"}, - Kind: kinds.Gateway, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "gw"}, + Kind: kinds.Gateway, + Group: v1.GroupName, + }, }, - Valid: true, + Ancestors: []PolicyAncestor{}, + Valid: true, }, pol4Key: { Source: pol4, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "ignored"}, - Kind: kinds.Gateway, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "ignored"}, + Kind: kinds.Gateway, + Group: v1.GroupName, + }, }, - Valid: true, + Ancestors: []PolicyAncestor{}, + Valid: true, }, }, }, { name: "invalid and valid policies", validator: &policiesfakes.FakeValidator{ - ValidateStub: func(policy policies.Policy) error { + ValidateStub: func( + policy policies.Policy, + _ *policies.GlobalSettings, + ) []conditions.Condition { if policy.GetName() == "pol1" { - return errors.New("invalid error") + return []conditions.Condition{staticConds.NewPolicyInvalid("invalid error")} } return nil @@ -562,24 +665,30 @@ func TestProcessPolicies(t *testing.T) { expProcessedPolicies: map[PolicyKey]*Policy{ pol1Key: { Source: pol1, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, }, Conditions: []conditions.Condition{ staticConds.NewPolicyInvalid("invalid error"), }, - Valid: false, + Ancestors: []PolicyAncestor{}, + Valid: false, }, pol2Key: { Source: pol2, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc"}, - Kind: kinds.GRPCRoute, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "grpc"}, + Kind: kinds.GRPCRoute, + Group: v1.GroupName, + }, }, - Valid: true, + Ancestors: []PolicyAncestor{}, + Valid: true, }, }, }, @@ -597,24 +706,30 @@ func TestProcessPolicies(t *testing.T) { expProcessedPolicies: map[PolicyKey]*Policy{ pol1Key: { Source: pol1, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, }, - Valid: true, + Ancestors: []PolicyAncestor{}, + Valid: true, }, pol1ConflictKey: { Source: pol1Conflict, - TargetRef: PolicyTargetRef{ - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, }, Conditions: []conditions.Condition{ staticConds.NewPolicyConflicted("Conflicts with another MyPolicy"), }, - Valid: false, + Ancestors: []PolicyAncestor{}, + Valid: false, }, }, }, @@ -646,7 +761,7 @@ func TestProcessPolicies(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - processed := processPolicies(test.policies, test.validator, gateways, routes) + processed := processPolicies(test.policies, test.validator, gateways, routes, nil) g.Expect(processed).To(BeEquivalentTo(test.expProcessedPolicies)) }) } @@ -681,14 +796,14 @@ func TestMarkConflictedPolicies(t *testing.T) { name: "different policy types can not conflict", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "orange"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(appleGVK, "apple"): { - Source: createTestPolicy(appleGVK, hrRef, "apple"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(appleGVK, hrRef, "apple"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, }, fakeValidator: &policiesfakes.FakeValidator{}, @@ -698,14 +813,14 @@ func TestMarkConflictedPolicies(t *testing.T) { name: "policies of the same type but with different target refs can not conflict", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "orange1"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange1"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange1"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(orangeGVK, "orange2"): { - Source: createTestPolicy(orangeGVK, grpcRef, "orange2"), - TargetRef: grpcTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, grpcRef, "orange2"), + TargetRefs: []PolicyTargetRef{grpcTargetRef}, + Valid: true, }, }, fakeValidator: &policiesfakes.FakeValidator{}, @@ -715,14 +830,14 @@ func TestMarkConflictedPolicies(t *testing.T) { name: "invalid policies can not conflict", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "valid"): { - Source: createTestPolicy(orangeGVK, hrRef, "valid"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "valid"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(orangeGVK, "invalid"): { - Source: createTestPolicy(orangeGVK, hrRef, "invalid"), - TargetRef: hrTargetRef, - Valid: false, + Source: createTestPolicy(orangeGVK, hrRef, "invalid"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: false, }, }, fakeValidator: &policiesfakes.FakeValidator{}, @@ -733,29 +848,29 @@ func TestMarkConflictedPolicies(t *testing.T) { " condition is added", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "orange1"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange1"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange1"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(orangeGVK, "orange2"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange2"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange2"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(orangeGVK, "orange3-conflicts-with-1"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange3-conflicts-with-1"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange3-conflicts-with-1"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(orangeGVK, "orange4"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange4"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange4"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, createTestPolicyKey(orangeGVK, "orange5-conflicts-with-4"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange5-conflicts-with-4"), - TargetRef: hrTargetRef, - Valid: true, + Source: createTestPolicy(orangeGVK, hrRef, "orange5-conflicts-with-4"), + TargetRefs: []PolicyTargetRef{hrTargetRef}, + Valid: true, }, }, fakeValidator: &policiesfakes.FakeValidator{ @@ -841,8 +956,8 @@ func createTestPolicy( GetNamespaceStub: func() string { return testNs }, - GetTargetRefStub: func() v1alpha2.LocalPolicyTargetReference { - return ref + GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{ref} }, GetObjectKindStub: func() schema.ObjectKind { return &policiesfakes.FakeObjectKind{ diff --git a/internal/mode/static/state/graph/policy_ancestor.go b/internal/mode/static/state/graph/policy_ancestor.go index b37335413..7120f94af 100644 --- a/internal/mode/static/state/graph/policy_ancestor.go +++ b/internal/mode/static/state/graph/policy_ancestor.go @@ -8,7 +8,11 @@ import ( const maxAncestors = 16 -func ancestorsFull( +// backendTLSPolicyAncestorsFull returns whether or not an ancestor list is full. A list is not full when: +// - the number of current ancestors is less than the maximum allowed +// - an entry for an NGF managed resource already exists in the ancestor list. This means that we are overwriting +// that status entry with the current status entry, since there is only one ancestor (Gateway) for this policy. +func backendTLSPolicyAncestorsFull( ancestors []v1alpha2.PolicyAncestorStatus, ctlrName string, ) bool { @@ -25,6 +29,26 @@ func ancestorsFull( return true } +// ngfPolicyAncestorsFull returns whether or not an ancestor list is full. A list is full when +// the sum of the following is greater than or equal to the maximum allowed: +// - number of non-NGF managed ancestors +// - number of NGF managed ancestors already added to the updated list +// +// We aren't considering the number of NGF managed ancestors in the current list because the updated list +// is the new source of truth. +func ngfPolicyAncestorsFull(policy *Policy, ctlrName string) bool { + currAncestors := policy.Source.GetPolicyStatus().Ancestors + + var nonNGFControllerCount int + for _, ancestor := range currAncestors { + if ancestor.ControllerName != v1.GatewayController(ctlrName) { + nonNGFControllerCount++ + } + } + + return nonNGFControllerCount+len(policy.Ancestors) >= maxAncestors +} + func createParentReference( group v1.Group, kind v1.Kind, diff --git a/internal/mode/static/state/graph/policy_ancestor_test.go b/internal/mode/static/state/graph/policy_ancestor_test.go index 114ce80dd..7b90a3b3a 100644 --- a/internal/mode/static/state/graph/policy_ancestor_test.go +++ b/internal/mode/static/state/graph/policy_ancestor_test.go @@ -6,13 +6,15 @@ import ( . "github.com/onsi/gomega" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" ) -func TestAncestorsFull(t *testing.T) { +func TestBackendTLSPolicyAncestorsFull(t *testing.T) { createCurStatus := func(numAncestors int, ctlrName string) []v1alpha2.PolicyAncestorStatus { statuses := make([]v1alpha2.PolicyAncestorStatus, 0, numAncestors) - for i := 0; i < numAncestors; i++ { + for range numAncestors { statuses = append(statuses, v1alpha2.PolicyAncestorStatus{ ControllerName: v1.GatewayController(ctlrName), }) @@ -47,7 +49,118 @@ func TestAncestorsFull(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - full := ancestorsFull(test.curStatus, "nginx-gateway") + full := backendTLSPolicyAncestorsFull(test.curStatus, "nginx-gateway") + g.Expect(full).To(Equal(test.expFull)) + }) + } +} + +func TestNGFPolicyAncestorsFull(t *testing.T) { + type ancestorConfig struct { + numCurrNGFAncestors int + numCurrNonNGFAncestors int + numNewNGFAncestors int + } + + createPolicy := func(cfg ancestorConfig) *Policy { + currAncestors := make([]v1alpha2.PolicyAncestorStatus, 0, cfg.numCurrNGFAncestors+cfg.numCurrNonNGFAncestors) + ngfAncestors := make([]PolicyAncestor, 0, cfg.numNewNGFAncestors) + + for range cfg.numCurrNonNGFAncestors { + currAncestors = append(currAncestors, v1alpha2.PolicyAncestorStatus{ + ControllerName: "non-ngf", + }) + } + + for range cfg.numCurrNGFAncestors { + currAncestors = append(currAncestors, v1alpha2.PolicyAncestorStatus{ + ControllerName: "nginx-gateway", + }) + } + + for range cfg.numNewNGFAncestors { + ngfAncestors = append(ngfAncestors, PolicyAncestor{ + Ancestor: v1.ParentReference{}, + }) + } + + return &Policy{ + Source: &ngfAPI.ObservabilityPolicy{ + Status: v1alpha2.PolicyStatus{ + Ancestors: currAncestors, + }, + }, + Ancestors: ngfAncestors, + } + } + + tests := []struct { + name string + expFull bool + cfg ancestorConfig + }{ + { + name: "current policy not full, no new NGF ancestors have been built yet", + cfg: ancestorConfig{ + numCurrNGFAncestors: 3, + numCurrNonNGFAncestors: 12, + numNewNGFAncestors: 0, + }, + expFull: false, + }, + { + name: "current policy not full, and some new NGF ancestors have been built (not at max)", + cfg: ancestorConfig{ + numCurrNGFAncestors: 3, + numCurrNonNGFAncestors: 11, + numNewNGFAncestors: 2, + }, + expFull: false, + }, + { + name: "current policy not full, and some new NGF ancestors have been built (at max)", + cfg: ancestorConfig{ + numCurrNGFAncestors: 3, + numCurrNonNGFAncestors: 11, + numNewNGFAncestors: 5, + }, + expFull: true, + }, + { + name: "current policy is full of non-NGF ancestors", + cfg: ancestorConfig{ + numCurrNGFAncestors: 0, + numCurrNonNGFAncestors: 16, + numNewNGFAncestors: 0, + }, + expFull: true, + }, + { + name: "current policy is full of a mix of ancestors, but updated list is empty", + cfg: ancestorConfig{ + numCurrNGFAncestors: 3, + numCurrNonNGFAncestors: 13, + numNewNGFAncestors: 0, + }, + expFull: false, + }, + { + name: "current policy is full of NGF ancestors, but updated ancestors is less than that", + cfg: ancestorConfig{ + numCurrNGFAncestors: 16, + numCurrNonNGFAncestors: 0, + numNewNGFAncestors: 5, + }, + expFull: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + policy := createPolicy(test.cfg) + full := ngfPolicyAncestorsFull(policy, "nginx-gateway") g.Expect(full).To(Equal(test.expFull)) }) } diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index c68152fde..25001cc32 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -11,7 +11,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" v1 "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/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" @@ -139,7 +138,7 @@ func buildRoutesForGateways( httpRoutes map[types.NamespacedName]*v1.HTTPRoute, grpcRoutes map[types.NamespacedName]*v1.GRPCRoute, gatewayNsNames []types.NamespacedName, - npCfg *ngfAPI.NginxProxy, + npCfg *NginxProxy, ) map[RouteKey]*L7Route { if len(gatewayNsNames) == 0 { return nil @@ -166,11 +165,11 @@ func buildRoutesForGateways( return routes } -func isHTTP2Disabled(npCfg *ngfAPI.NginxProxy) bool { +func isHTTP2Disabled(npCfg *NginxProxy) bool { if npCfg == nil { return false } - return npCfg.Spec.DisableHTTP2 + return npCfg.Source.Spec.DisableHTTP2 } func buildSectionNameRefs( diff --git a/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go b/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go index 9d08181f7..1eaa2745e 100644 --- a/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go +++ b/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go @@ -4,6 +4,7 @@ package validationfakes import ( "sync" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -21,16 +22,17 @@ type FakePolicyValidator struct { conflictsReturnsOnCall map[int]struct { result1 bool } - ValidateStub func(policies.Policy) error + ValidateStub func(policies.Policy, *policies.GlobalSettings) []conditions.Condition validateMutex sync.RWMutex validateArgsForCall []struct { arg1 policies.Policy + arg2 *policies.GlobalSettings } validateReturns struct { - result1 error + result1 []conditions.Condition } validateReturnsOnCall map[int]struct { - result1 error + result1 []conditions.Condition } invocations map[string][][]interface{} invocationsMutex sync.RWMutex @@ -98,18 +100,19 @@ func (fake *FakePolicyValidator) ConflictsReturnsOnCall(i int, result1 bool) { }{result1} } -func (fake *FakePolicyValidator) Validate(arg1 policies.Policy) error { +func (fake *FakePolicyValidator) Validate(arg1 policies.Policy, arg2 *policies.GlobalSettings) []conditions.Condition { fake.validateMutex.Lock() ret, specificReturn := fake.validateReturnsOnCall[len(fake.validateArgsForCall)] fake.validateArgsForCall = append(fake.validateArgsForCall, struct { arg1 policies.Policy - }{arg1}) + arg2 *policies.GlobalSettings + }{arg1, arg2}) stub := fake.ValidateStub fakeReturns := fake.validateReturns - fake.recordInvocation("Validate", []interface{}{arg1}) + fake.recordInvocation("Validate", []interface{}{arg1, arg2}) fake.validateMutex.Unlock() if stub != nil { - return stub(arg1) + return stub(arg1, arg2) } if specificReturn { return ret.result1 @@ -123,39 +126,39 @@ func (fake *FakePolicyValidator) ValidateCallCount() int { return len(fake.validateArgsForCall) } -func (fake *FakePolicyValidator) ValidateCalls(stub func(policies.Policy) error) { +func (fake *FakePolicyValidator) ValidateCalls(stub func(policies.Policy, *policies.GlobalSettings) []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = stub } -func (fake *FakePolicyValidator) ValidateArgsForCall(i int) policies.Policy { +func (fake *FakePolicyValidator) ValidateArgsForCall(i int) (policies.Policy, *policies.GlobalSettings) { fake.validateMutex.RLock() defer fake.validateMutex.RUnlock() argsForCall := fake.validateArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakePolicyValidator) ValidateReturns(result1 error) { +func (fake *FakePolicyValidator) ValidateReturns(result1 []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = nil fake.validateReturns = struct { - result1 error + result1 []conditions.Condition }{result1} } -func (fake *FakePolicyValidator) ValidateReturnsOnCall(i int, result1 error) { +func (fake *FakePolicyValidator) ValidateReturnsOnCall(i int, result1 []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = nil if fake.validateReturnsOnCall == nil { fake.validateReturnsOnCall = make(map[int]struct { - result1 error + result1 []conditions.Condition }) } fake.validateReturnsOnCall[i] = struct { - result1 error + result1 []conditions.Condition }{result1} } diff --git a/internal/mode/static/state/validation/validator.go b/internal/mode/static/state/validation/validator.go index 15566901b..9e0108428 100644 --- a/internal/mode/static/state/validation/validator.go +++ b/internal/mode/static/state/validation/validator.go @@ -3,6 +3,7 @@ package validation //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate import ( + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" ) @@ -53,7 +54,7 @@ type GenericValidator interface { //counterfeiter:generate . PolicyValidator type PolicyValidator interface { // Validate validates an NGF Policy. - Validate(policy policies.Policy) error + Validate(policy policies.Policy, globalSettings *policies.GlobalSettings) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b policies.Policy) bool } diff --git a/internal/mode/static/status/prepare_requests.go b/internal/mode/static/status/prepare_requests.go index 8b9799437..b0b00c797 100644 --- a/internal/mode/static/status/prepare_requests.go +++ b/internal/mode/static/status/prepare_requests.go @@ -306,30 +306,31 @@ func PrepareNGFPolicyRequests( reqs := make([]frameworkStatus.UpdateRequest, 0, len(policies)) for key, pol := range policies { - ancestorStatuses := make([]v1alpha2.PolicyAncestorStatus, 0, 1) - ancestor := pol.Ancestor + ancestorStatuses := make([]v1alpha2.PolicyAncestorStatus, 0, len(pol.TargetRefs)) - if ancestor == nil { + if len(pol.Ancestors) == 0 { continue } - allConds := make([]conditions.Condition, 0, len(pol.Conditions)+len(ancestor.Conditions)+1) + for _, ancestor := range pol.Ancestors { + allConds := make([]conditions.Condition, 0, len(pol.Conditions)+len(ancestor.Conditions)+1) - // The order of conditions matters here. - // We add the default condition first, followed by the ancestor conditions, and finally the policy conditions. - // DeduplicateConditions will ensure the last condition wins. - allConds = append(allConds, staticConds.NewPolicyAccepted()) - allConds = append(allConds, ancestor.Conditions...) - allConds = append(allConds, pol.Conditions...) + // The order of conditions matters here. + // We add the default condition first, followed by the ancestor conditions, and finally the policy conditions. + // DeduplicateConditions will ensure the last condition wins. + allConds = append(allConds, staticConds.NewPolicyAccepted()) + allConds = append(allConds, ancestor.Conditions...) + allConds = append(allConds, pol.Conditions...) - conds := conditions.DeduplicateConditions(allConds) - apiConds := conditions.ConvertConditions(conds, pol.Source.GetGeneration(), transitionTime) + conds := conditions.DeduplicateConditions(allConds) + apiConds := conditions.ConvertConditions(conds, pol.Source.GetGeneration(), transitionTime) - ancestorStatuses = append(ancestorStatuses, v1alpha2.PolicyAncestorStatus{ - AncestorRef: ancestor.Ancestor, - ControllerName: v1alpha2.GatewayController(gatewayCtlrName), - Conditions: apiConds, - }) + ancestorStatuses = append(ancestorStatuses, v1alpha2.PolicyAncestorStatus{ + AncestorRef: ancestor.Ancestor, + ControllerName: v1alpha2.GatewayController(gatewayCtlrName), + Conditions: apiConds, + }) + } status := v1alpha2.PolicyStatus{Ancestors: ancestorStatuses} diff --git a/internal/mode/static/status/prepare_requests_test.go b/internal/mode/static/status/prepare_requests_test.go index 546b534e0..407aff11e 100644 --- a/internal/mode/static/status/prepare_requests_test.go +++ b/internal/mode/static/status/prepare_requests_test.go @@ -1392,7 +1392,7 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { transitionTime := helpers.PrepareTimeForFakeClient(metav1.Now()) type policyCfg struct { - Ancestor *graph.PolicyAncestor + Ancestors []graph.PolicyAncestor Name string Conditions []conditions.Condition } @@ -1409,7 +1409,7 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { }, }, Conditions: cfg.Conditions, - Ancestor: cfg.Ancestor, + Ancestors: cfg.Ancestors, } } @@ -1422,9 +1422,16 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { } validPolicyCfg := policyCfg{ Name: validPolicyKey.NsName.Name, - Ancestor: &graph.PolicyAncestor{ - Ancestor: v1.ParentReference{ - Name: "ancestor", + Ancestors: []graph.PolicyAncestor{ + { + Ancestor: v1.ParentReference{ + Name: "ancestor1", + }, + }, + { + Ancestor: v1.ParentReference{ + Name: "ancestor2", + }, }, }, } @@ -1436,9 +1443,16 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { invalidPolicyCfg := policyCfg{ Name: invalidPolicyKey.NsName.Name, Conditions: invalidConds, - Ancestor: &graph.PolicyAncestor{ - Ancestor: v1.ParentReference{ - Name: "ancestor", + Ancestors: []graph.PolicyAncestor{ + { + Ancestor: v1.ParentReference{ + Name: "ancestor1", + }, + }, + { + Ancestor: v1.ParentReference{ + Name: "ancestor2", + }, }, }, } @@ -1449,11 +1463,13 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { } targetRefNotFoundPolicyCfg := policyCfg{ Name: targetRefNotFoundPolicyKey.NsName.Name, - Ancestor: &graph.PolicyAncestor{ - Ancestor: v1.ParentReference{ - Name: "ancestor", + Ancestors: []graph.PolicyAncestor{ + { + Ancestor: v1.ParentReference{ + Name: "ancestor1", + }, + Conditions: targetRefNotFoundConds, }, - Conditions: targetRefNotFoundConds, }, } @@ -1464,11 +1480,13 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { multiInvalidCondsPolicyCfg := policyCfg{ Name: multiInvalidCondsPolicyKey.NsName.Name, Conditions: invalidConds, - Ancestor: &graph.PolicyAncestor{ - Ancestor: v1.ParentReference{ - Name: "ancestor", + Ancestors: []graph.PolicyAncestor{ + { + Ancestor: v1.ParentReference{ + Name: "ancestor1", + }, + Conditions: targetRefNotFoundConds, }, - Conditions: targetRefNotFoundConds, }, } @@ -1477,8 +1495,8 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { GVK: schema.GroupVersionKind{Group: ngfAPI.GroupName, Kind: kinds.ClientSettingsPolicy}, } nilAncestorPolicyCfg := policyCfg{ - Name: nilAncestorPolicyKey.NsName.Name, - Ancestor: nil, + Name: nilAncestorPolicyKey.NsName.Name, + Ancestors: nil, } tests := []struct { @@ -1502,7 +1520,23 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { Ancestors: []v1alpha2.PolicyAncestorStatus{ { AncestorRef: v1.ParentReference{ - Name: "ancestor", + Name: "ancestor1", + }, + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.PolicyReasonInvalid), + Message: "invalid", + }, + }, + }, + { + AncestorRef: v1.ParentReference{ + Name: "ancestor2", }, ControllerName: gatewayCtlrName, Conditions: []metav1.Condition{ @@ -1522,7 +1556,7 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { Ancestors: []v1alpha2.PolicyAncestorStatus{ { AncestorRef: v1.ParentReference{ - Name: "ancestor", + Name: "ancestor1", }, ControllerName: gatewayCtlrName, Conditions: []metav1.Condition{ @@ -1542,7 +1576,23 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { Ancestors: []v1alpha2.PolicyAncestorStatus{ { AncestorRef: v1.ParentReference{ - Name: "ancestor", + Name: "ancestor1", + }, + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 2, + LastTransitionTime: transitionTime, + Reason: string(v1alpha2.PolicyReasonAccepted), + Message: "Policy is accepted", + }, + }, + }, + { + AncestorRef: v1.ParentReference{ + Name: "ancestor2", }, ControllerName: gatewayCtlrName, Conditions: []metav1.Condition{ @@ -1570,7 +1620,7 @@ func TestBuildNGFPolicyStatuses(t *testing.T) { Ancestors: []v1alpha2.PolicyAncestorStatus{ { AncestorRef: v1.ParentReference{ - Name: "ancestor", + Name: "ancestor1", }, ControllerName: gatewayCtlrName, Conditions: []metav1.Condition{ diff --git a/tests/framework/collector.go b/tests/framework/collector.go new file mode 100644 index 000000000..7847f9ac0 --- /dev/null +++ b/tests/framework/collector.go @@ -0,0 +1,77 @@ +package framework + +import ( + "fmt" + "os/exec" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + CollectorNamespace = "collector" + collectorChartReleaseName = "otel-collector" + // FIXME(pleshakov): Find a automated way to keep the version updated here similar to dependabot. + // https://github.com/nginxinc/nginx-gateway-fabric/issues/1665 + collectorChartVersion = "0.73.1" +) + +// InstallCollector installs the otel-collector. +func InstallCollector() ([]byte, error) { + repoAddArgs := []string{ + "repo", + "add", + "open-telemetry", + "https://open-telemetry.github.io/opentelemetry-helm-charts", + } + + if output, err := exec.Command("helm", repoAddArgs...).CombinedOutput(); err != nil { + return output, err + } + + args := []string{ + "install", + collectorChartReleaseName, + "open-telemetry/opentelemetry-collector", + "--create-namespace", + "--namespace", CollectorNamespace, + "--version", collectorChartVersion, + "-f", "manifests/telemetry/collector-values.yaml", + "--wait", + } + + return exec.Command("helm", args...).CombinedOutput() +} + +// UninstallCollector uninstalls the otel-collector. +func UninstallCollector(resourceManager ResourceManager) ([]byte, error) { + args := []string{ + "uninstall", collectorChartReleaseName, + "--namespace", CollectorNamespace, + } + + output, err := exec.Command("helm", args...).CombinedOutput() + if err != nil { + return output, err + } + + return nil, resourceManager.DeleteNamespace(CollectorNamespace) +} + +// GetCollectorPodName returns the name of the collector Pod. +func GetCollectorPodName(resourceManager ResourceManager) (string, error) { + collectorPodNames, err := resourceManager.GetPodNames( + CollectorNamespace, + client.MatchingLabels{ + "app.kubernetes.io/name": "opentelemetry-collector", + }, + ) + if err != nil { + return "", err + } + + if len(collectorPodNames) != 1 { + return "", fmt.Errorf("expected 1 collector pod, got %d", len(collectorPodNames)) + } + + return collectorPodNames[0], nil +} diff --git a/tests/suite/client_settings_test.go b/tests/suite/client_settings_test.go index 2619fc2a8..26ef6fc4f 100644 --- a/tests/suite/client_settings_test.go +++ b/tests/suite/client_settings_test.go @@ -242,7 +242,6 @@ func waitForClientSettingsAncestorStatus( 500*time.Millisecond, true, /* poll immediately */ func(ctx context.Context) (bool, error) { - var pol ngfAPI.ClientSettingsPolicy if err := k8sClient.Get(ctx, policyNsname, &pol); err != nil { @@ -261,7 +260,7 @@ func waitForClientSettingsAncestorStatus( ancestor := pol.Status.Ancestors[0] - if err := ancestorMustEqualTargetRef(ancestor, pol.GetTargetRef(), policyNsname.Namespace); err != nil { + if err := ancestorMustEqualTargetRef(ancestor, pol.GetTargetRefs()[0], policyNsname.Namespace); err != nil { return false, err } diff --git a/tests/suite/dataplane_perf_test.go b/tests/suite/dataplane_perf_test.go index 3860133e7..a1c3a6fb4 100644 --- a/tests/suite/dataplane_perf_test.go +++ b/tests/suite/dataplane_perf_test.go @@ -23,11 +23,8 @@ var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"), "dp-perf/gateway.yaml", "dp-perf/cafe-routes.yaml", } - ns := &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dp-perf", - }, - } + + var ns core.Namespace var addr string targetURL := "http://cafe.example.com" @@ -56,7 +53,13 @@ var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"), } BeforeAll(func() { - Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + ns = core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp-perf", + }, + } + + Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index b4148039e..b9cfe5d13 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -36,11 +36,7 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov "graceful-recovery/cafe-routes.yaml", } - ns := &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "graceful-recovery", - }, - } + var ns core.Namespace teaURL := "https://cafe.example.com/tea" coffeeURL := "http://cafe.example.com/coffee" @@ -63,7 +59,13 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov }) BeforeEach(func() { - Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + ns = core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "graceful-recovery", + }, + } + + Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) @@ -82,13 +84,13 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov }) It("recovers when NGF container is restarted", func() { - runRecoveryTest(teaURL, coffeeURL, ngfPodName, ngfContainerName, files, ns) + runRecoveryTest(teaURL, coffeeURL, ngfPodName, ngfContainerName, files, &ns) }) It("recovers when nginx container is restarted", func() { // FIXME(bjee19) remove Skip() when https://github.com/nginxinc/nginx-gateway-fabric/issues/1108 is completed. Skip("Test currently fails due to this issue: https://github.com/nginxinc/nginx-gateway-fabric/issues/1108") - runRecoveryTest(teaURL, coffeeURL, ngfPodName, nginxContainerName, files, ns) + runRecoveryTest(teaURL, coffeeURL, ngfPodName, nginxContainerName, files, &ns) }) }) @@ -206,10 +208,10 @@ func checkForWorkingTraffic(teaURL, coffeeURL string) error { } func checkForFailingTraffic(teaURL, coffeeURL string) error { - if err := expectRequestToFail(teaURL, address, "URI: /tea"); err != nil { + if err := expectRequestToFail(teaURL, address); err != nil { return err } - if err := expectRequestToFail(coffeeURL, address, "URI: /coffee"); err != nil { + if err := expectRequestToFail(coffeeURL, address); err != nil { return err } return nil @@ -228,7 +230,7 @@ func expectRequestToSucceed(appURL, address string, responseBodyMessage string) return err } -func expectRequestToFail(appURL, address string, responseBodyMessage string) error { +func expectRequestToFail(appURL, address string) error { status, body, err := framework.Get(appURL, address, timeoutConfig.RequestTimeout) if status != 0 { return errors.New("expected http status to be 0") diff --git a/tests/suite/longevity_test.go b/tests/suite/longevity_test.go index 2ddf66c44..d42ebc8b2 100644 --- a/tests/suite/longevity_test.go +++ b/tests/suite/longevity_test.go @@ -31,16 +31,18 @@ var _ = Describe("Longevity", Label("longevity-setup", "longevity-teardown"), fu "longevity/prom.yaml", } - ns = &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "longevity", - }, - } + ns core.Namespace labelFilter = GinkgoLabelFilter() ) BeforeEach(func() { + ns = core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "longevity", + }, + } + if !strings.Contains(labelFilter, "longevity") { Skip("skipping longevity test unless 'longevity' label is explicitly defined when running") } @@ -51,7 +53,7 @@ var _ = Describe("Longevity", Label("longevity-setup", "longevity-teardown"), fu Skip("'longevity-setup' label not specified; skipping...") } - Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) Expect(resourceManager.ApplyFromFiles(promFile, ngfNamespace)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) diff --git a/tests/suite/manifests/hello-world/apps.yaml b/tests/suite/manifests/hello-world/apps.yaml new file mode 100644 index 000000000..9e0df181c --- /dev/null +++ b/tests/suite/manifests/hello-world/apps.yaml @@ -0,0 +1,98 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: hello +spec: + replicas: 1 + selector: + matchLabels: + app: hello + template: + metadata: + labels: + app: hello + spec: + containers: + - name: hello + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: hello +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: hello +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: world +spec: + replicas: 1 + selector: + matchLabels: + app: world + template: + metadata: + labels: + app: world + spec: + containers: + - name: world + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: world +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: world +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: hello-world +spec: + replicas: 1 + selector: + matchLabels: + app: hello-world + template: + metadata: + labels: + app: hello-world + spec: + containers: + - name: hello-world + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: hello-world +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: hello-world diff --git a/tests/suite/manifests/hello/gateway.yaml b/tests/suite/manifests/hello-world/gateway.yaml similarity index 85% rename from tests/suite/manifests/hello/gateway.yaml rename to tests/suite/manifests/hello-world/gateway.yaml index e6507f613..81234f950 100644 --- a/tests/suite/manifests/hello/gateway.yaml +++ b/tests/suite/manifests/hello-world/gateway.yaml @@ -8,4 +8,4 @@ spec: - name: http port: 80 protocol: HTTP - hostname: "*.example.com" + hostname: foo.example.com diff --git a/tests/suite/manifests/hello-world/routes.yaml b/tests/suite/manifests/hello-world/routes.yaml new file mode 100644 index 000000000..9b7b456e9 --- /dev/null +++ b/tests/suite/manifests/hello-world/routes.yaml @@ -0,0 +1,50 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: hello +spec: + parentRefs: + - name: gateway + sectionName: http + rules: + - matches: + - path: + type: Exact + value: /hello + backendRefs: + - name: hello + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: world +spec: + parentRefs: + - name: gateway + sectionName: http + rules: + - matches: + - path: + type: Exact + value: /world + backendRefs: + - name: world + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: hello-world +spec: + parentRefs: + - name: gateway + sectionName: http + rules: + - matches: + - path: + type: Exact + value: /helloworld + backendRefs: + - name: hello-world + port: 80 diff --git a/tests/suite/manifests/hello/hello.yaml b/tests/suite/manifests/hello/hello.yaml deleted file mode 100644 index 1d566b215..000000000 --- a/tests/suite/manifests/hello/hello.yaml +++ /dev/null @@ -1,32 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: hello -spec: - replicas: 1 - selector: - matchLabels: - app: hello - template: - metadata: - labels: - app: hello - spec: - containers: - - name: hello - image: nginxdemos/nginx-hello:plain-text - ports: - - containerPort: 8080 ---- -apiVersion: v1 -kind: Service -metadata: - name: hello -spec: - ports: - - port: 80 - targetPort: 8080 - protocol: TCP - name: http - selector: - app: hello diff --git a/tests/suite/manifests/hello/route.yaml b/tests/suite/manifests/hello/route.yaml deleted file mode 100644 index e70cce260..000000000 --- a/tests/suite/manifests/hello/route.yaml +++ /dev/null @@ -1,18 +0,0 @@ -apiVersion: gateway.networking.k8s.io/v1 -kind: HTTPRoute -metadata: - name: hello -spec: - parentRefs: - - name: gateway - sectionName: http - hostnames: - - "hello.example.com" - rules: - - matches: - - path: - type: Exact - value: /hello - backendRefs: - - name: hello - port: 80 diff --git a/tests/suite/manifests/tracing/nginxproxy.yaml b/tests/suite/manifests/tracing/nginxproxy.yaml new file mode 100644 index 000000000..ed1f62104 --- /dev/null +++ b/tests/suite/manifests/tracing/nginxproxy.yaml @@ -0,0 +1,12 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: NginxProxy +metadata: + name: nginx-proxy +spec: + telemetry: + exporter: + endpoint: otel-collector-opentelemetry-collector.collector.svc:4317 + serviceName: my-test-svc + spanAttributes: + - key: testkey1 + value: testval1 diff --git a/tests/suite/manifests/tracing/policy-multiple.yaml b/tests/suite/manifests/tracing/policy-multiple.yaml new file mode 100644 index 000000000..851f573fd --- /dev/null +++ b/tests/suite/manifests/tracing/policy-multiple.yaml @@ -0,0 +1,17 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: ObservabilityPolicy +metadata: + name: test-observability-policy +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: hello + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: world + tracing: + strategy: ratio + spanAttributes: + - key: testkey2 + value: testval2 diff --git a/tests/suite/manifests/tracing/policy-single.yaml b/tests/suite/manifests/tracing/policy-single.yaml new file mode 100644 index 000000000..6d8da1581 --- /dev/null +++ b/tests/suite/manifests/tracing/policy-single.yaml @@ -0,0 +1,14 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: ObservabilityPolicy +metadata: + name: test-observability-policy +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: hello + tracing: + strategy: ratio + spanAttributes: + - key: testkey2 + value: testval2 diff --git a/tests/suite/sample_test.go b/tests/suite/sample_test.go index 56d08713a..d573c5903 100644 --- a/tests/suite/sample_test.go +++ b/tests/suite/sample_test.go @@ -4,6 +4,8 @@ import ( "fmt" "net/http" "strconv" + "strings" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -16,18 +18,21 @@ import ( var _ = Describe("Basic test example", Label("functional"), func() { files := []string{ - "hello/hello.yaml", - "hello/gateway.yaml", - "hello/route.yaml", - } - ns := &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "hello", - }, + "hello-world/apps.yaml", + "hello-world/gateway.yaml", + "hello-world/routes.yaml", } + var ns core.Namespace + BeforeEach(func() { - Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + ns = core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "helloworld", + }, + } + + Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) }) @@ -38,13 +43,28 @@ var _ = Describe("Basic test example", Label("functional"), func() { }) It("sends traffic", func() { - url := "http://hello.example.com/hello" + url := "http://foo.example.com/hello" if portFwdPort != 0 { - url = fmt.Sprintf("http://hello.example.com:%s/hello", strconv.Itoa(portFwdPort)) + url = fmt.Sprintf("http://foo.example.com:%s/hello", strconv.Itoa(portFwdPort)) } - status, body, err := framework.Get(url, address, timeoutConfig.RequestTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(status).To(Equal(http.StatusOK)) - Expect(body).To(ContainSubstring("URI: /hello")) + + Eventually( + func() error { + status, body, err := framework.Get(url, address, timeoutConfig.RequestTimeout) + if err != nil { + return err + } + if status != http.StatusOK { + return fmt.Errorf("status not 200; got %d", status) + } + expBody := "URI: /hello" + if !strings.Contains(body, expBody) { + return fmt.Errorf("bad body: got %s; expected %s", body, expBody) + } + return nil + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) }) }) diff --git a/tests/suite/system_suite_test.go b/tests/suite/system_suite_test.go index e8a7ea987..3cb155a3b 100644 --- a/tests/suite/system_suite_test.go +++ b/tests/suite/system_suite_test.go @@ -80,6 +80,7 @@ var ( const ( releaseName = "ngf-test" ngfNamespace = "nginx-gateway" + gatewayClassName = "nginx" ngfHTTPForwardedPort = 10080 ngfHTTPSForwardedPort = 10443 ngfControllerName = "gateway.nginx.org/nginx-gateway-controller" diff --git a/tests/suite/telemetry_test.go b/tests/suite/telemetry_test.go index ec6cbf099..b1517fc9a 100644 --- a/tests/suite/telemetry_test.go +++ b/tests/suite/telemetry_test.go @@ -2,21 +2,13 @@ package suite import ( "fmt" - "os/exec" "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" core "k8s.io/api/core/v1" - crClient "sigs.k8s.io/controller-runtime/pkg/client" -) -const ( - collectorNamespace = "collector" - collectorChartReleaseName = "otel-collector" - // FIXME(pleshakov): Find a automated way to keep the version updated here similar to dependabot. - // https://github.com/nginxinc/nginx-gateway-fabric/issues/1665 - collectorChartVersion = "0.73.1" + "github.com/nginxinc/nginx-gateway-fabric/tests/framework" ) var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func() { @@ -24,7 +16,7 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( // Because NGF reports telemetry on start, we need to install the collector first. // Install collector - output, err := installCollector() + output, err := framework.InstallCollector() Expect(err).ToNot(HaveOccurred(), string(output)) // Install NGF @@ -37,22 +29,13 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( }) AfterEach(func() { - output, err := uninstallCollector() + output, err := framework.UninstallCollector(resourceManager) Expect(err).ToNot(HaveOccurred(), string(output)) }) It("sends telemetry", func() { - names, err := resourceManager.GetPodNames( - collectorNamespace, - crClient.MatchingLabels{ - "app.kubernetes.io/name": "opentelemetry-collector", - }, - ) - + name, err := framework.GetCollectorPodName(resourceManager) Expect(err).ToNot(HaveOccurred()) - Expect(names).To(HaveLen(1)) - - name := names[0] // We assert that all data points were sent // For some data points, as a sanity check, we assert on sent values. @@ -64,7 +47,7 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( Expect(err).ToNot(HaveOccurred()) matchFirstExpectedLine := func() bool { - logs, err := resourceManager.GetPodLogs(collectorNamespace, name, &core.PodLogOptions{}) + logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, name, &core.PodLogOptions{}) Expect(err).ToNot(HaveOccurred()) return strings.Contains(logs, "dataType: Str(ngf-product-telemetry)") } @@ -72,7 +55,7 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( // Wait until the collector has received the telemetry data Eventually(matchFirstExpectedLine, "30s", "5s").Should(BeTrue()) - logs, err := resourceManager.GetPodLogs(collectorNamespace, name, &core.PodLogOptions{}) + logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, name, &core.PodLogOptions{}) Expect(err).ToNot(HaveOccurred()) assertConsecutiveLinesInLogs( @@ -103,41 +86,6 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( }) }) -func installCollector() ([]byte, error) { - repoAddArgs := []string{ - "repo", - "add", - "open-telemetry", - "https://open-telemetry.github.io/opentelemetry-helm-charts", - } - - if output, err := exec.Command("helm", repoAddArgs...).CombinedOutput(); err != nil { - return output, err - } - - args := []string{ - "install", - collectorChartReleaseName, - "open-telemetry/opentelemetry-collector", - "--create-namespace", - "--namespace", collectorNamespace, - "--version", collectorChartVersion, - "-f", "manifests/telemetry/collector-values.yaml", - "--wait", - } - - return exec.Command("helm", args...).CombinedOutput() -} - -func uninstallCollector() ([]byte, error) { - args := []string{ - "uninstall", collectorChartReleaseName, - "--namespace", collectorNamespace, - } - - return exec.Command("helm", args...).CombinedOutput() -} - func assertConsecutiveLinesInLogs(logs string, expectedLines []string) { lines := strings.Split(logs, "\n") diff --git a/tests/suite/tracing_test.go b/tests/suite/tracing_test.go new file mode 100644 index 000000000..c7d525863 --- /dev/null +++ b/tests/suite/tracing_test.go @@ -0,0 +1,263 @@ +package suite + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + core "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + v1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" + "github.com/nginxinc/nginx-gateway-fabric/tests/framework" +) + +// This test can be flaky when waiting to see traces show up in the collector logs. +// Sometimes they get there right away, sometimes it takes 30 seconds. Retries were +// added to attempt to mitigate the issue, but it didn't fix it 100% +var _ = Describe("Tracing", FlakeAttempts(2), Label("functional"), func() { + var ( + files = []string{ + "hello-world/apps.yaml", + "hello-world/gateway.yaml", + "hello-world/routes.yaml", + } + nginxProxyFile = "tracing/nginxproxy.yaml" + policySingleFile = "tracing/policy-single.yaml" + policyMultipleFile = "tracing/policy-multiple.yaml" + + ns core.Namespace + + collectorPodName, helloURL, worldURL, helloworldURL string + ) + + BeforeEach(func() { + ns = core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "helloworld", + }, + } + + output, err := framework.InstallCollector() + Expect(err).ToNot(HaveOccurred(), string(output)) + + collectorPodName, err = framework.GetCollectorPodName(resourceManager) + Expect(err).ToNot(HaveOccurred()) + + Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) + + url := "http://foo.example.com" + helloURL = url + "/hello" + worldURL = url + "/world" + helloworldURL = url + "/helloworld" + if portFwdPort != 0 { + helloURL = fmt.Sprintf("%s:%d/hello", url, portFwdPort) + worldURL = fmt.Sprintf("%s:%d/world", url, portFwdPort) + helloworldURL = fmt.Sprintf("%s:%d/helloworld", url, portFwdPort) + } + }) + + AfterEach(func() { + output, err := framework.UninstallCollector(resourceManager) + Expect(err).ToNot(HaveOccurred(), string(output)) + + Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) + Expect(resourceManager.DeleteFromFiles( + []string{nginxProxyFile, policySingleFile, policyMultipleFile}, ns.Name)).To(Succeed()) + Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed()) + + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.CreateTimeout) + defer cancel() + + key := types.NamespacedName{Name: gatewayClassName} + var gwClass gatewayv1.GatewayClass + Expect(k8sClient.Get(ctx, key, &gwClass)).To(Succeed()) + + gwClass.Spec.ParametersRef = nil + + Expect(k8sClient.Update(ctx, &gwClass)).To(Succeed()) + }) + + updateGatewayClass := func() error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.CreateTimeout) + defer cancel() + + key := types.NamespacedName{Name: gatewayClassName} + var gwClass gatewayv1.GatewayClass + if err := k8sClient.Get(ctx, key, &gwClass); err != nil { + return err + } + + gwClass.Spec.ParametersRef = &gatewayv1.ParametersReference{ + Group: ngfAPI.GroupName, + Kind: gatewayv1.Kind("NginxProxy"), + Name: "nginx-proxy", + } + + return k8sClient.Update(ctx, &gwClass) + } + + sendRequests := func(url string, count int) { + for range count { + Eventually( + func() error { + status, _, err := framework.Get(url, address, timeoutConfig.RequestTimeout) + if err != nil { + return err + } + if status != http.StatusOK { + return fmt.Errorf("status not 200; got %d", status) + } + return nil + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + } + } + + // Send traffic and verify that traces exist for hello app. We send every time this is called because + // sometimes it takes awhile to see the traces show up. + findTraces := func() bool { + sendRequests(helloURL, 25) + sendRequests(worldURL, 25) + sendRequests(helloworldURL, 25) + + logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, collectorPodName, &core.PodLogOptions{}) + Expect(err).ToNot(HaveOccurred()) + return strings.Contains(logs, "service.name: Str(ngf:helloworld:gateway:my-test-svc)") + } + + checkStatusAndTraces := func() { + Eventually( + func() error { + return verifyGatewayClassResolvedRefs() + }). + WithTimeout(timeoutConfig.GetTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + Eventually( + func() error { + return verifyPolicyStatus() + }). + WithTimeout(timeoutConfig.GetTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + // wait for expected first line to show up + Eventually(findTraces, "1m", "5s").Should(BeTrue()) + } + + It("sends tracing spans for one policy attached to one route", func() { + sendRequests(helloURL, 5) + + // verify that no traces exist yet + logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, collectorPodName, &core.PodLogOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(logs).ToNot(ContainSubstring("service.name: Str(ngf:helloworld:gateway:my-test-svc)")) + + // install tracing configuration + traceFiles := []string{ + nginxProxyFile, + policySingleFile, + } + Expect(resourceManager.ApplyFromFiles(traceFiles, ns.Name)).To(Succeed()) + Expect(updateGatewayClass()).To(Succeed()) + + checkStatusAndTraces() + + logs, err = resourceManager.GetPodLogs(framework.CollectorNamespace, collectorPodName, &core.PodLogOptions{}) + Expect(err).ToNot(HaveOccurred()) + + Expect(logs).To(ContainSubstring("http.method: Str(GET)")) + Expect(logs).To(ContainSubstring("http.target: Str(/hello)")) + Expect(logs).To(ContainSubstring("testkey1: Str(testval1)")) + Expect(logs).To(ContainSubstring("testkey2: Str(testval2)")) + + // verify traces don't exist for other apps + Expect(logs).ToNot(ContainSubstring("http.target: Str(/world)")) + Expect(logs).ToNot(ContainSubstring("http.target: Str(/helloworld)")) + }) + + It("sends tracing spans for one policy attached to multiple routes", func() { + // install tracing configuration + traceFiles := []string{ + nginxProxyFile, + policyMultipleFile, + } + Expect(resourceManager.ApplyFromFiles(traceFiles, ns.Name)).To(Succeed()) + Expect(updateGatewayClass()).To(Succeed()) + + checkStatusAndTraces() + + logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, collectorPodName, &core.PodLogOptions{}) + Expect(err).ToNot(HaveOccurred()) + + Expect(logs).To(ContainSubstring("http.method: Str(GET)")) + Expect(logs).To(ContainSubstring("http.target: Str(/hello)")) + Expect(logs).To(ContainSubstring("http.target: Str(/world)")) + Expect(logs).To(ContainSubstring("testkey1: Str(testval1)")) + Expect(logs).To(ContainSubstring("testkey2: Str(testval2)")) + + // verify traces don't exist for helloworld apps + Expect(logs).ToNot(ContainSubstring("http.target: Str(/helloworld)")) + }) +}) + +func verifyGatewayClassResolvedRefs() error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) + defer cancel() + + var gc v1.GatewayClass + if err := k8sClient.Get(ctx, types.NamespacedName{Name: gatewayClassName}, &gc); err != nil { + return err + } + + for _, cond := range gc.Status.Conditions { + if cond.Type == string(conditions.GatewayClassResolvedRefs) && cond.Status == metav1.ConditionTrue { + return nil + } + } + + return errors.New("ResolvedRefs status not set to true on GatewayClass") +} + +func verifyPolicyStatus() error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) + defer cancel() + + var pol ngfAPI.ObservabilityPolicy + key := types.NamespacedName{Name: "test-observability-policy", Namespace: "helloworld"} + if err := k8sClient.Get(ctx, key, &pol); err != nil { + return err + } + + var count int + for _, ancestor := range pol.Status.Ancestors { + for _, cond := range ancestor.Conditions { + if cond.Type == string(gatewayv1alpha2.PolicyConditionAccepted) && cond.Status == metav1.ConditionTrue { + count++ + } + } + } + + if count != len(pol.Status.Ancestors) { + return fmt.Errorf("Policy not accepted; expected %d accepted conditions, got %d", len(pol.Status.Ancestors), count) + } + + return nil +} diff --git a/tests/suite/upgrade_test.go b/tests/suite/upgrade_test.go index 37226ac71..c16248719 100644 --- a/tests/suite/upgrade_test.go +++ b/tests/suite/upgrade_test.go @@ -35,11 +35,7 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() { "ngf-upgrade/cafe-routes.yaml", } - ns = &core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ngf-upgrade", - }, - } + ns core.Namespace valuesFile = "manifests/ngf-upgrade/values.yaml" resultsFile *os.File @@ -60,7 +56,13 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() { } setup(cfg, "--values", valuesFile) - Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + ns = core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ngf-upgrade", + }, + } + + Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed())