Skip to content

Commit

Permalink
Update validation struct name and add condition
Browse files Browse the repository at this point in the history
  • Loading branch information
sjberman committed May 23, 2024
1 parent fd592da commit 2443919
Show file tree
Hide file tree
Showing 13 changed files with 90 additions and 61 deletions.
2 changes: 1 addition & 1 deletion internal/mode/static/policies/clientsettings/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions internal/mode/static/policies/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -75,15 +75,15 @@ 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]
if !ok {
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.
Expand Down
4 changes: 2 additions & 2 deletions internal/mode/static/policies/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -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 },
Expand Down
14 changes: 11 additions & 3 deletions internal/mode/static/policies/observability/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
67 changes: 39 additions & 28 deletions internal/mode/static/policies/observability/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,91 +52,102 @@ 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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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",
policy: createModifiedPolicy(func(p *ngfAPI.ObservabilityPolicy) *ngfAPI.ObservabilityPolicy {
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,
},
}

Expand All @@ -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())
Expand Down
12 changes: 6 additions & 6 deletions internal/mode/static/policies/policiesfakes/fake_validator.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions internal/mode/static/policies/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 10 additions & 2 deletions internal/mode/static/state/conditions/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
}
}
7 changes: 4 additions & 3 deletions internal/mode/static/state/graph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
}
Expand Down Expand Up @@ -231,7 +232,7 @@ func BuildGraph(
validators.PolicyValidator,
processedGws,
routes,
policySettings,
policyValidationCtx,
)

g := &Graph{
Expand Down
4 changes: 2 additions & 2 deletions internal/mode/static/state/graph/policies.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -202,7 +202,7 @@ func processPolicies(
continue
}

conds := validator.Validate(policy, policySettings)
conds := validator.Validate(policy, policyValidationCtx)

processedPolicies[key] = &Policy{
Source: policy,
Expand Down
2 changes: 1 addition & 1 deletion internal/mode/static/state/graph/policies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")}
Expand Down
Loading

0 comments on commit 2443919

Please sign in to comment.