From 24439196621fe334cfa7f0b0f65207085afc84bd Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Thu, 23 May 2024 09:51:00 -0600 Subject: [PATCH] Update validation struct name and add condition --- .../policies/clientsettings/validator.go | 2 +- internal/mode/static/policies/manager.go | 6 +- internal/mode/static/policies/manager_test.go | 4 +- .../policies/observability/validator.go | 14 +++- .../policies/observability/validator_test.go | 67 +++++++++++-------- .../policies/policiesfakes/fake_validator.go | 12 ++-- internal/mode/static/policies/policy.go | 7 +- .../static/state/conditions/conditions.go | 12 +++- internal/mode/static/state/graph/graph.go | 7 +- internal/mode/static/state/graph/policies.go | 4 +- .../mode/static/state/graph/policies_test.go | 2 +- .../validationfakes/fake_policy_validator.go | 12 ++-- .../mode/static/state/validation/validator.go | 2 +- 13 files changed, 90 insertions(+), 61 deletions(-) diff --git a/internal/mode/static/policies/clientsettings/validator.go b/internal/mode/static/policies/clientsettings/validator.go index edcf7cd63b..ef5da00194 100644 --- a/internal/mode/static/policies/clientsettings/validator.go +++ b/internal/mode/static/policies/clientsettings/validator.go @@ -28,7 +28,7 @@ func NewValidator(genericValidator validation.GenericValidator) *Validator { } // Validate validates the spec of a ClientSettingsPolicy. -func (v *Validator) Validate(policy policies.Policy, _ *policies.GlobalPolicySettings) []conditions.Condition { +func (v *Validator) Validate(policy policies.Policy, _ *policies.ValidationContext) []conditions.Condition { csp := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](policy) if err := validateTargetRef(csp.Spec.TargetRef); err != nil { diff --git a/internal/mode/static/policies/manager.go b/internal/mode/static/policies/manager.go index 61513edc0c..cf34abcbf1 100644 --- a/internal/mode/static/policies/manager.go +++ b/internal/mode/static/policies/manager.go @@ -20,7 +20,7 @@ type GenerateFunc func(policy Policy, globalSettings *ngfAPI.NginxProxy) []byte //counterfeiter:generate . Validator type Validator interface { // Validate validates an NGF Policy. - Validate(policy Policy, globalSettings *GlobalPolicySettings) []conditions.Condition + Validate(policy Policy, policyValidationCtx *ValidationContext) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b Policy) bool } @@ -75,7 +75,7 @@ func (m *Manager) Generate(policy Policy, globalSettings *ngfAPI.NginxProxy) []b } // Validate validates the policy. -func (m *Manager) Validate(policy Policy, globalSettings *GlobalPolicySettings) []conditions.Condition { +func (m *Manager) Validate(policy Policy, policyValidationCtx *ValidationContext) []conditions.Condition { gvk := m.mustExtractGVK(policy) validator, ok := m.validators[gvk] @@ -83,7 +83,7 @@ func (m *Manager) Validate(policy Policy, globalSettings *GlobalPolicySettings) panic(fmt.Sprintf("no validator registered for policy %T", policy)) } - return validator.Validate(policy, globalSettings) + return validator.Validate(policy, policyValidationCtx) } // 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 a3d9579052..3b75720a48 100644 --- a/internal/mode/static/policies/manager_test.go +++ b/internal/mode/static/policies/manager_test.go @@ -43,7 +43,7 @@ var _ = Describe("Policy Manager", func() { mustExtractGVK, policies.ManagerConfig{ Validator: &policiesfakes.FakeValidator{ - ValidateStub: func(_ policies.Policy, _ *policies.GlobalPolicySettings) []conditions.Condition { + ValidateStub: func(_ policies.Policy, _ *policies.ValidationContext) []conditions.Condition { return []conditions.Condition{staticConds.NewPolicyInvalid("apple error")} }, ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return true }, @@ -55,7 +55,7 @@ var _ = Describe("Policy Manager", func() { }, policies.ManagerConfig{ Validator: &policiesfakes.FakeValidator{ - ValidateStub: func(_ policies.Policy, _ *policies.GlobalPolicySettings) []conditions.Condition { + ValidateStub: func(_ policies.Policy, _ *policies.ValidationContext) []conditions.Condition { return []conditions.Condition{staticConds.NewPolicyInvalid("orange error")} }, ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return false }, diff --git a/internal/mode/static/policies/observability/validator.go b/internal/mode/static/policies/observability/validator.go index 5e64a650cd..134d6d394d 100644 --- a/internal/mode/static/policies/observability/validator.go +++ b/internal/mode/static/policies/observability/validator.go @@ -30,15 +30,23 @@ func NewValidator(genericValidator validation.GenericValidator) *Validator { // Validate validates the spec of an ObservabilityPolicy. func (v *Validator) Validate( policy policies.Policy, - globalSettings *policies.GlobalPolicySettings, + policyValidationCtx *policies.ValidationContext, ) []conditions.Condition { obs, ok := policy.(*ngfAPI.ObservabilityPolicy) if !ok { panic(fmt.Sprintf("expected ObservabilityPolicy, got: %T", policy)) } - if globalSettings == nil || !globalSettings.NginxProxyValid { - return []conditions.Condition{staticConds.NewPolicyNotAcceptedNginxProxyNotSet()} + if policyValidationCtx == nil || !policyValidationCtx.NginxProxyValid { + return []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageNginxProxyInvalid), + } + } + + if !policyValidationCtx.TelemetryEnabled { + return []conditions.Condition{ + staticConds.NewPolicyNotAcceptedNginxProxyNotSet(staticConds.PolicyMessageTelemetryNotEnabled), + } } if err := validateTargetRefs(obs.Spec.TargetRefs); err != nil { diff --git a/internal/mode/static/policies/observability/validator_test.go b/internal/mode/static/policies/observability/validator_test.go index b9a96b26d2..7410a806ae 100644 --- a/internal/mode/static/policies/observability/validator_test.go +++ b/internal/mode/static/policies/observability/validator_test.go @@ -52,22 +52,33 @@ func createModifiedPolicy(mod policyModFunc) *ngfAPI.ObservabilityPolicy { } func TestValidator_Validate(t *testing.T) { + validCtx := &policies.ValidationContext{ + NginxProxyValid: true, + TelemetryEnabled: true, + } + tests := []struct { - name string - policy *ngfAPI.ObservabilityPolicy - globalSettings *policies.GlobalPolicySettings - expErrSubstrings []string + name string + policy *ngfAPI.ObservabilityPolicy + policyValidationCtx *policies.ValidationContext + expErrSubstrings []string }{ { - name: "global settings are nil", + name: "validation context is nil", policy: createValidPolicy(), expErrSubstrings: []string{"NginxProxy configuration is either invalid or not attached"}, }, { - name: "global settings are invalid", - policy: createValidPolicy(), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: false}, - expErrSubstrings: []string{"NginxProxy configuration is either invalid or not attached"}, + name: "validation context is invalid", + policy: createValidPolicy(), + policyValidationCtx: &policies.ValidationContext{NginxProxyValid: false}, + expErrSubstrings: []string{"NginxProxy configuration is either invalid or not attached"}, + }, + { + name: "telemetry is not enabled", + policy: createValidPolicy(), + policyValidationCtx: &policies.ValidationContext{NginxProxyValid: true, TelemetryEnabled: false}, + expErrSubstrings: []string{"Telemetry is not enabled"}, }, { name: "invalid target ref; unsupported group", @@ -75,8 +86,8 @@ func TestValidator_Validate(t *testing.T) { p.Spec.TargetRefs[0].Group = "Unsupported" return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.targetRefs.group"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.targetRefs.group"}, }, { name: "invalid target ref; unsupported kind", @@ -84,8 +95,8 @@ func TestValidator_Validate(t *testing.T) { p.Spec.TargetRefs[0].Kind = "Unsupported" return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.targetRefs.kind"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.targetRefs.kind"}, }, { name: "invalid strategy", @@ -93,8 +104,8 @@ func TestValidator_Validate(t *testing.T) { p.Spec.Tracing.Strategy = "invalid" return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.tracing.strategy"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.tracing.strategy"}, }, { name: "invalid context", @@ -102,8 +113,8 @@ func TestValidator_Validate(t *testing.T) { p.Spec.Tracing.Context = helpers.GetPointer[ngfAPI.TraceContext]("invalid") return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.tracing.context"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.tracing.context"}, }, { name: "invalid span name", @@ -111,8 +122,8 @@ func TestValidator_Validate(t *testing.T) { p.Spec.Tracing.SpanName = helpers.GetPointer("invalid$$$") return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.tracing.spanName"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.tracing.spanName"}, }, { name: "invalid span attribute key", @@ -120,8 +131,8 @@ func TestValidator_Validate(t *testing.T) { p.Spec.Tracing.SpanAttributes[0].Key = "invalid$$$" return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.tracing.spanAttributes.key"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.tracing.spanAttributes.key"}, }, { name: "invalid span attribute value", @@ -129,14 +140,14 @@ func TestValidator_Validate(t *testing.T) { p.Spec.Tracing.SpanAttributes[0].Value = "invalid$$$" return p }), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: []string{"spec.tracing.spanAttributes.value"}, + policyValidationCtx: validCtx, + expErrSubstrings: []string{"spec.tracing.spanAttributes.value"}, }, { - name: "valid", - policy: createValidPolicy(), - globalSettings: &policies.GlobalPolicySettings{NginxProxyValid: true}, - expErrSubstrings: nil, + name: "valid", + policy: createValidPolicy(), + policyValidationCtx: validCtx, + expErrSubstrings: nil, }, } @@ -146,7 +157,7 @@ func TestValidator_Validate(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - conds := v.Validate(test.policy, test.globalSettings) + conds := v.Validate(test.policy, test.policyValidationCtx) if len(test.expErrSubstrings) == 0 { g.Expect(conds).To(BeEmpty()) diff --git a/internal/mode/static/policies/policiesfakes/fake_validator.go b/internal/mode/static/policies/policiesfakes/fake_validator.go index 9a4211795c..d222f24d7a 100644 --- a/internal/mode/static/policies/policiesfakes/fake_validator.go +++ b/internal/mode/static/policies/policiesfakes/fake_validator.go @@ -21,11 +21,11 @@ type FakeValidator struct { conflictsReturnsOnCall map[int]struct { result1 bool } - ValidateStub func(policies.Policy, *policies.GlobalPolicySettings) []conditions.Condition + ValidateStub func(policies.Policy, *policies.ValidationContext) []conditions.Condition validateMutex sync.RWMutex validateArgsForCall []struct { arg1 policies.Policy - arg2 *policies.GlobalPolicySettings + arg2 *policies.ValidationContext } validateReturns struct { result1 []conditions.Condition @@ -99,12 +99,12 @@ func (fake *FakeValidator) ConflictsReturnsOnCall(i int, result1 bool) { }{result1} } -func (fake *FakeValidator) Validate(arg1 policies.Policy, arg2 *policies.GlobalPolicySettings) []conditions.Condition { +func (fake *FakeValidator) Validate(arg1 policies.Policy, arg2 *policies.ValidationContext) []conditions.Condition { fake.validateMutex.Lock() ret, specificReturn := fake.validateReturnsOnCall[len(fake.validateArgsForCall)] fake.validateArgsForCall = append(fake.validateArgsForCall, struct { arg1 policies.Policy - arg2 *policies.GlobalPolicySettings + arg2 *policies.ValidationContext }{arg1, arg2}) stub := fake.ValidateStub fakeReturns := fake.validateReturns @@ -125,13 +125,13 @@ func (fake *FakeValidator) ValidateCallCount() int { return len(fake.validateArgsForCall) } -func (fake *FakeValidator) ValidateCalls(stub func(policies.Policy, *policies.GlobalPolicySettings) []conditions.Condition) { +func (fake *FakeValidator) ValidateCalls(stub func(policies.Policy, *policies.ValidationContext) []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = stub } -func (fake *FakeValidator) ValidateArgsForCall(i int) (policies.Policy, *policies.GlobalPolicySettings) { +func (fake *FakeValidator) ValidateArgsForCall(i int) (policies.Policy, *policies.ValidationContext) { fake.validateMutex.RLock() defer fake.validateMutex.RUnlock() argsForCall := fake.validateArgsForCall[i] diff --git a/internal/mode/static/policies/policy.go b/internal/mode/static/policies/policy.go index 340e28eea5..686f00a7e2 100644 --- a/internal/mode/static/policies/policy.go +++ b/internal/mode/static/policies/policy.go @@ -26,10 +26,11 @@ type ConfigGenerator interface { Generate(policy Policy, globalSettings *ngfAPI.NginxProxy) []byte } -// GlobalPolicySettings are settings from the current state of the graph that may be +// ValidationContext contains context from the current state of the graph that may be // needed for policy validation if certain policies rely on those global settings. -type GlobalPolicySettings struct { - NginxProxyValid bool +type ValidationContext struct { + NginxProxyValid bool + TelemetryEnabled bool } // We generate a mock of ObjectKind so that we can create fake policies and set their GVKs. diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index a3e58b540b..a0adcbbc70 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -77,6 +77,14 @@ const ( // 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 NgixnProxy resource" ) // NewTODO returns a Condition that can be used as a placeholder for a condition that is not yet implemented. @@ -646,11 +654,11 @@ func NewPolicyTargetNotFound(msg string) conditions.Condition { // 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() conditions.Condition { +func NewPolicyNotAcceptedNginxProxyNotSet(msg string) conditions.Condition { return conditions.Condition{ Type: string(v1alpha2.PolicyConditionAccepted), Status: metav1.ConditionFalse, Reason: string(PolicyReasonNginxProxyConfigNotSet), - Message: "The NginxProxy configuration is either invalid or not attached to the GatewayClass", + Message: msg, } } diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index 78f8d7cf3e..d61f6cee1b 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -175,7 +175,7 @@ func BuildGraph( validators validation.Validators, protectedPorts ProtectedPorts, ) *Graph { - policySettings := &policies.GlobalPolicySettings{} + policyValidationCtx := &policies.ValidationContext{} processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { @@ -188,7 +188,8 @@ func BuildGraph( if gc != nil && npCfg != nil { for _, cond := range gc.Conditions { if cond.Type == string(conditions.GatewayClassResolvedRefs) && cond.Status == metav1.ConditionTrue { - policySettings.NginxProxyValid = true + policyValidationCtx.NginxProxyValid = true + policyValidationCtx.TelemetryEnabled = npCfg.Spec.Telemetry != nil && npCfg.Spec.Telemetry.Exporter != nil break } } @@ -231,7 +232,7 @@ func BuildGraph( validators.PolicyValidator, processedGws, routes, - policySettings, + policyValidationCtx, ) g := &Graph{ diff --git a/internal/mode/static/state/graph/policies.go b/internal/mode/static/state/graph/policies.go index 0043713084..61bae16444 100644 --- a/internal/mode/static/state/graph/policies.go +++ b/internal/mode/static/state/graph/policies.go @@ -161,7 +161,7 @@ func processPolicies( validator validation.PolicyValidator, gateways processedGateways, routes map[RouteKey]*L7Route, - policySettings *policies.GlobalPolicySettings, + policyValidationCtx *policies.ValidationContext, ) map[PolicyKey]*Policy { if len(policies) == 0 || gateways.Winner == nil { return nil @@ -202,7 +202,7 @@ func processPolicies( continue } - conds := validator.Validate(policy, policySettings) + conds := validator.Validate(policy, policyValidationCtx) processedPolicies[key] = &Policy{ Source: policy, diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index d8ca2fc507..38afeceaf7 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/internal/mode/static/state/graph/policies_test.go @@ -622,7 +622,7 @@ func TestProcessPolicies(t *testing.T) { validator: &policiesfakes.FakeValidator{ ValidateStub: func( policy policies.Policy, - _ *policies.GlobalPolicySettings, + _ *policies.ValidationContext, ) []conditions.Condition { if policy.GetName() == "pol1" { return []conditions.Condition{staticConds.NewPolicyInvalid("invalid error")} 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 f391cf9bf7..7496c52e0c 100644 --- a/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go +++ b/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go @@ -22,11 +22,11 @@ type FakePolicyValidator struct { conflictsReturnsOnCall map[int]struct { result1 bool } - ValidateStub func(policies.Policy, *policies.GlobalPolicySettings) []conditions.Condition + ValidateStub func(policies.Policy, *policies.ValidationContext) []conditions.Condition validateMutex sync.RWMutex validateArgsForCall []struct { arg1 policies.Policy - arg2 *policies.GlobalPolicySettings + arg2 *policies.ValidationContext } validateReturns struct { result1 []conditions.Condition @@ -100,12 +100,12 @@ func (fake *FakePolicyValidator) ConflictsReturnsOnCall(i int, result1 bool) { }{result1} } -func (fake *FakePolicyValidator) Validate(arg1 policies.Policy, arg2 *policies.GlobalPolicySettings) []conditions.Condition { +func (fake *FakePolicyValidator) Validate(arg1 policies.Policy, arg2 *policies.ValidationContext) []conditions.Condition { fake.validateMutex.Lock() ret, specificReturn := fake.validateReturnsOnCall[len(fake.validateArgsForCall)] fake.validateArgsForCall = append(fake.validateArgsForCall, struct { arg1 policies.Policy - arg2 *policies.GlobalPolicySettings + arg2 *policies.ValidationContext }{arg1, arg2}) stub := fake.ValidateStub fakeReturns := fake.validateReturns @@ -126,13 +126,13 @@ func (fake *FakePolicyValidator) ValidateCallCount() int { return len(fake.validateArgsForCall) } -func (fake *FakePolicyValidator) ValidateCalls(stub func(policies.Policy, *policies.GlobalPolicySettings) []conditions.Condition) { +func (fake *FakePolicyValidator) ValidateCalls(stub func(policies.Policy, *policies.ValidationContext) []conditions.Condition) { fake.validateMutex.Lock() defer fake.validateMutex.Unlock() fake.ValidateStub = stub } -func (fake *FakePolicyValidator) ValidateArgsForCall(i int) (policies.Policy, *policies.GlobalPolicySettings) { +func (fake *FakePolicyValidator) ValidateArgsForCall(i int) (policies.Policy, *policies.ValidationContext) { fake.validateMutex.RLock() defer fake.validateMutex.RUnlock() argsForCall := fake.validateArgsForCall[i] diff --git a/internal/mode/static/state/validation/validator.go b/internal/mode/static/state/validation/validator.go index 747e4e9624..25f8d2b797 100644 --- a/internal/mode/static/state/validation/validator.go +++ b/internal/mode/static/state/validation/validator.go @@ -54,7 +54,7 @@ type GenericValidator interface { //counterfeiter:generate . PolicyValidator type PolicyValidator interface { // Validate validates an NGF Policy. - Validate(policy policies.Policy, globalSettings *policies.GlobalPolicySettings) []conditions.Condition + Validate(policy policies.Policy, policyValidationCtx *policies.ValidationContext) []conditions.Condition // Conflicts returns true if the two Policies conflict. Conflicts(a, b policies.Policy) bool }