Skip to content

Commit c1552b6

Browse files
sjbermansjberman
committed
Product code review
1 parent 58ac9d2 commit c1552b6

31 files changed

+515
-383
lines changed

‎apis/v1alpha1/observabilitypolicy_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ type Tracing struct {
6464

6565
// Ratio is the percentage of traffic that should be sampled. Integer from 0 to 100.
6666
// By default, 100% of http requests are traced. Not applicable for parent-based tracing.
67+
// If ratio is set to 0, tracing is disabled.
6768
//
6869
// +optional
6970
// +kubebuilder:validation:Minimum=0

‎config/crd/bases/gateway.nginx.org_observabilitypolicies.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ spec:
108108
description: |-
109109
Ratio is the percentage of traffic that should be sampled. Integer from 0 to 100.
110110
By default, 100% of http requests are traced. Not applicable for parent-based tracing.
111+
If ratio is set to 0, tracing is disabled.
111112
format: int32
112113
maximum: 100
113114
minimum: 0

‎deploy/crds.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -890,6 +890,7 @@ spec:
890890
description: |-
891891
Ratio is the percentage of traffic that should be sampled. Integer from 0 to 100.
892892
By default, 100% of http requests are traced. Not applicable for parent-based tracing.
893+
If ratio is set to 0, tracing is disabled.
893894
format: int32
894895
maximum: 100
895896
minimum: 0

‎internal/framework/kinds/kinds.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ const (
2525
const (
2626
// ClientSettingsPolicy is the ClientSettingsPolicy kind.
2727
ClientSettingsPolicy = "ClientSettingsPolicy"
28+
// ObservabilityPolicy is the ObservabilityPolicy kind.
29+
ObservabilityPolicy = "ObservabilityPolicy"
2830
// NginxProxy is the NginxProxy kind.
2931
NginxProxy = "NginxProxy"
3032
)

‎internal/mode/static/policies/clientsettings/generator.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ keepalive_timeout {{ .KeepAlive.Timeout.Server }};
3737
`
3838

3939
// Generate generates configuration as []byte for a ClientSettingsPolicy.
40-
func Generate(policy policies.Policy, _ *ngfAPI.NginxProxy) []byte {
40+
func Generate(policy policies.Policy, _ *policies.GlobalPolicySettings) []byte {
4141
csp := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](policy)
4242

4343
return helpers.MustExecuteTemplate(tmpl, csp.Spec)

‎internal/mode/static/policies/clientsettings/validator.go

Lines changed: 4 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
package clientsettings
22

33
import (
4-
"slices"
5-
64
"k8s.io/apimachinery/pkg/util/validation/field"
75
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
8-
"sigs.k8s.io/gateway-api/apis/v1alpha2"
96

107
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
118
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
@@ -28,10 +25,12 @@ func NewValidator(genericValidator validation.GenericValidator) *Validator {
2825
}
2926

3027
// Validate validates the spec of a ClientSettingsPolicy.
31-
func (v *Validator) Validate(policy policies.Policy, _ *policies.ValidationContext) []conditions.Condition {
28+
func (v *Validator) Validate(policy policies.Policy, _ *policies.GlobalPolicySettings) []conditions.Condition {
3229
csp := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](policy)
3330

34-
if err := validateTargetRef(csp.Spec.TargetRef); err != nil {
31+
targetRefPath := field.NewPath("spec").Child("targetRef")
32+
supportedKinds := []gatewayv1.Kind{kinds.Gateway, kinds.HTTPRoute, kinds.GRPCRoute}
33+
if err := policies.ValidateTargetRef(csp.Spec.TargetRef, targetRefPath, supportedKinds); err != nil {
3534
return []conditions.Condition{staticConds.NewPolicyInvalid(err.Error())}
3635
}
3736

@@ -79,34 +78,6 @@ func conflicts(a, b ngfAPI.ClientSettingsPolicySpec) bool {
7978
return false
8079
}
8180

82-
func validateTargetRef(ref v1alpha2.LocalPolicyTargetReference) error {
83-
basePath := field.NewPath("spec").Child("targetRef")
84-
85-
if ref.Group != gatewayv1.GroupName {
86-
path := basePath.Child("group")
87-
88-
return field.NotSupported(
89-
path,
90-
ref.Group,
91-
[]string{gatewayv1.GroupName},
92-
)
93-
}
94-
95-
supportedKinds := []gatewayv1.Kind{kinds.Gateway, kinds.HTTPRoute, kinds.GRPCRoute}
96-
97-
if !slices.Contains(supportedKinds, ref.Kind) {
98-
path := basePath.Child("kind")
99-
100-
return field.NotSupported(
101-
path,
102-
ref.Kind,
103-
supportedKinds,
104-
)
105-
}
106-
107-
return nil
108-
}
109-
11081
// validateSettings performs validation on fields in the spec that are vulnerable to code injection.
11182
// For all other fields, we rely on the CRD validation.
11283
func (v *Validator) validateSettings(spec ngfAPI.ClientSettingsPolicySpec) error {

‎internal/mode/static/policies/clientsettings/validator_test.go

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package clientsettings_test
22

33
import (
4-
"fmt"
5-
"strings"
64
"testing"
75

86
. "github.com/onsi/gomega"
@@ -11,11 +9,13 @@ import (
119
"sigs.k8s.io/gateway-api/apis/v1alpha2"
1210

1311
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
12+
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
1413
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
1514
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
1615
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation"
1716
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings"
1817
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes"
18+
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
1919
)
2020

2121
type policyModFunc func(policy *ngfAPI.ClientSettingsPolicy) *ngfAPI.ClientSettingsPolicy
@@ -54,33 +54,43 @@ func createModifiedPolicy(mod policyModFunc) *ngfAPI.ClientSettingsPolicy {
5454

5555
func TestValidator_Validate(t *testing.T) {
5656
tests := []struct {
57-
name string
58-
policy *ngfAPI.ClientSettingsPolicy
59-
expErrSubstrings []string
57+
name string
58+
policy *ngfAPI.ClientSettingsPolicy
59+
expConditions []conditions.Condition
6060
}{
6161
{
6262
name: "invalid target ref; unsupported group",
6363
policy: createModifiedPolicy(func(p *ngfAPI.ClientSettingsPolicy) *ngfAPI.ClientSettingsPolicy {
6464
p.Spec.TargetRef.Group = "Unsupported"
6565
return p
6666
}),
67-
expErrSubstrings: []string{"spec.targetRef.group"},
67+
expConditions: []conditions.Condition{
68+
staticConds.NewPolicyInvalid("spec.targetRef.group: Unsupported value: \"Unsupported\": " +
69+
"supported values: \"gateway.networking.k8s.io\""),
70+
},
6871
},
6972
{
7073
name: "invalid target ref; unsupported kind",
7174
policy: createModifiedPolicy(func(p *ngfAPI.ClientSettingsPolicy) *ngfAPI.ClientSettingsPolicy {
7275
p.Spec.TargetRef.Kind = "Unsupported"
7376
return p
7477
}),
75-
expErrSubstrings: []string{"spec.targetRef.kind"},
78+
expConditions: []conditions.Condition{
79+
staticConds.NewPolicyInvalid("spec.targetRef.kind: Unsupported value: \"Unsupported\": " +
80+
"supported values: \"Gateway\", \"HTTPRoute\", \"GRPCRoute\""),
81+
},
7682
},
7783
{
7884
name: "invalid client max body size",
7985
policy: createModifiedPolicy(func(p *ngfAPI.ClientSettingsPolicy) *ngfAPI.ClientSettingsPolicy {
8086
p.Spec.Body.MaxSize = helpers.GetPointer[ngfAPI.Size]("invalid")
8187
return p
8288
}),
83-
expErrSubstrings: []string{"spec.body.maxSize"},
89+
expConditions: []conditions.Condition{
90+
staticConds.NewPolicyInvalid("spec.body.maxSize: Invalid value: \"invalid\": ^\\d{1,4}(k|m|g)?$ " +
91+
"(e.g. '1024', or '8k', or '20m', or '1g', regex used for validation is 'must contain a number. " +
92+
"May be followed by 'k', 'm', or 'g', otherwise bytes are assumed')"),
93+
},
8494
},
8595
{
8696
name: "invalid durations",
@@ -91,11 +101,14 @@ func TestValidator_Validate(t *testing.T) {
91101
p.Spec.KeepAlive.Timeout.Header = helpers.GetPointer[ngfAPI.Duration]("invalid")
92102
return p
93103
}),
94-
expErrSubstrings: []string{
95-
"spec.body.timeout",
96-
"spec.keepAlive.time",
97-
"spec.keepAlive.timeout.server",
98-
"spec.keepAlive.timeout.header",
104+
expConditions: []conditions.Condition{
105+
staticConds.NewPolicyInvalid("[spec.body.timeout: Invalid value: \"invalid\": \\d{1,4}(ms|s)? " +
106+
"(e.g. '5ms', or '10s', regex used for validation is 'must contain a number followed by 'ms' or 's''), " +
107+
"spec.keepAlive.time: Invalid value: \"invalid\": \\d{1,4}(ms|s)? (e.g. '5ms', or '10s', regex used for " +
108+
"validation is 'must contain a number followed by 'ms' or 's''), spec.keepAlive.timeout.server: Invalid value: " +
109+
"\"invalid\": \\d{1,4}(ms|s)? (e.g. '5ms', or '10s', regex used for validation is 'must contain a number " +
110+
"followed by 'ms' or 's''), spec.keepAlive.timeout.header: Invalid value: \"invalid\": \\d{1,4}(ms|s)? " +
111+
"(e.g. '5ms', or '10s', regex used for validation is 'must contain a number followed by 'ms' or 's'')]"),
99112
},
100113
},
101114
{
@@ -104,12 +117,15 @@ func TestValidator_Validate(t *testing.T) {
104117
p.Spec.KeepAlive.Timeout.Server = nil
105118
return p
106119
}),
107-
expErrSubstrings: []string{"spec.keepAlive.timeout"},
120+
expConditions: []conditions.Condition{
121+
staticConds.NewPolicyInvalid("spec.keepAlive.timeout: Invalid value: \"null\": " +
122+
"server timeout must be set if header timeout is set"),
123+
},
108124
},
109125
{
110-
name: "valid",
111-
policy: createValidPolicy(),
112-
expErrSubstrings: nil,
126+
name: "valid",
127+
policy: createValidPolicy(),
128+
expConditions: nil,
113129
},
114130
}
115131

@@ -120,23 +136,7 @@ func TestValidator_Validate(t *testing.T) {
120136
g := NewWithT(t)
121137

122138
conds := v.Validate(test.policy, nil)
123-
124-
if len(test.expErrSubstrings) == 0 {
125-
g.Expect(conds).To(BeEmpty())
126-
} else {
127-
g.Expect(conds).ToNot(BeEmpty())
128-
}
129-
130-
for _, str := range test.expErrSubstrings {
131-
var msg string
132-
for _, cond := range conds {
133-
if strings.Contains(cond.Message, str) {
134-
msg = cond.Message
135-
break
136-
}
137-
}
138-
g.Expect(msg).To(ContainSubstring(str), fmt.Sprintf("error not found in %v", conds))
139-
}
139+
g.Expect(conds).To(Equal(test.expConditions))
140140
})
141141
}
142142
}

‎internal/mode/static/policies/manager.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,19 @@ import (
77

88
"k8s.io/apimachinery/pkg/runtime/schema"
99

10-
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
1110
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
1211
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
1312
)
1413

1514
// GenerateFunc generates config as []byte for an NGF Policy.
16-
type GenerateFunc func(policy Policy, globalSettings *ngfAPI.NginxProxy) []byte
15+
type GenerateFunc func(policy Policy, globalSettings *GlobalPolicySettings) []byte
1716

1817
// Validator validates an NGF Policy.
1918
//
2019
//counterfeiter:generate . Validator
2120
type Validator interface {
2221
// Validate validates an NGF Policy.
23-
Validate(policy Policy, policyValidationCtx *ValidationContext) []conditions.Condition
22+
Validate(policy Policy, globalSettings *GlobalPolicySettings) []conditions.Condition
2423
// Conflicts returns true if the two Policies conflict.
2524
Conflicts(a, b Policy) bool
2625
}
@@ -63,7 +62,7 @@ func NewManager(
6362
}
6463

6564
// Generate generates config for the policy as a byte array.
66-
func (m *Manager) Generate(policy Policy, globalSettings *ngfAPI.NginxProxy) []byte {
65+
func (m *Manager) Generate(policy Policy, globalSettings *GlobalPolicySettings) []byte {
6766
gvk := m.mustExtractGVK(policy)
6867

6968
generate, ok := m.generators[gvk]
@@ -75,15 +74,15 @@ func (m *Manager) Generate(policy Policy, globalSettings *ngfAPI.NginxProxy) []b
7574
}
7675

7776
// Validate validates the policy.
78-
func (m *Manager) Validate(policy Policy, policyValidationCtx *ValidationContext) []conditions.Condition {
77+
func (m *Manager) Validate(policy Policy, globalSettings *GlobalPolicySettings) []conditions.Condition {
7978
gvk := m.mustExtractGVK(policy)
8079

8180
validator, ok := m.validators[gvk]
8281
if !ok {
8382
panic(fmt.Sprintf("no validator registered for policy %T", policy))
8483
}
8584

86-
return validator.Validate(policy, policyValidationCtx)
85+
return validator.Validate(policy, globalSettings)
8786
}
8887

8988
// Conflicts returns true if the policies conflict.

‎internal/mode/static/policies/manager_test.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"k8s.io/apimachinery/pkg/runtime/schema"
77
"sigs.k8s.io/controller-runtime/pkg/client"
88

9-
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
109
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
1110
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
1211
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes"
@@ -43,24 +42,24 @@ var _ = Describe("Policy Manager", func() {
4342
mustExtractGVK,
4443
policies.ManagerConfig{
4544
Validator: &policiesfakes.FakeValidator{
46-
ValidateStub: func(_ policies.Policy, _ *policies.ValidationContext) []conditions.Condition {
45+
ValidateStub: func(_ policies.Policy, _ *policies.GlobalPolicySettings) []conditions.Condition {
4746
return []conditions.Condition{staticConds.NewPolicyInvalid("apple error")}
4847
},
4948
ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return true },
5049
},
51-
Generator: func(_ policies.Policy, _ *ngfAPI.NginxProxy) []byte {
50+
Generator: func(_ policies.Policy, _ *policies.GlobalPolicySettings) []byte {
5251
return []byte("apple")
5352
},
5453
GVK: appleGVK,
5554
},
5655
policies.ManagerConfig{
5756
Validator: &policiesfakes.FakeValidator{
58-
ValidateStub: func(_ policies.Policy, _ *policies.ValidationContext) []conditions.Condition {
57+
ValidateStub: func(_ policies.Policy, _ *policies.GlobalPolicySettings) []conditions.Condition {
5958
return []conditions.Condition{staticConds.NewPolicyInvalid("orange error")}
6059
},
6160
ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return false },
6261
},
63-
Generator: func(_ policies.Policy, _ *ngfAPI.NginxProxy) []byte {
62+
Generator: func(_ policies.Policy, _ *policies.GlobalPolicySettings) []byte {
6463
return []byte("orange")
6564
},
6665
GVK: orangeGVK,

‎internal/mode/static/policies/observability/generator.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}";
3131
`
3232

3333
// Generate generates configuration as []byte for an ObservabilityPolicy.
34-
func Generate(policy policies.Policy, globalSettings *ngfAPI.NginxProxy) []byte {
34+
func Generate(policy policies.Policy, globalSettings *policies.GlobalPolicySettings) []byte {
3535
obs := helpers.MustCastObject[*ngfAPI.ObservabilityPolicy](policy)
3636

3737
var strategy string
@@ -54,8 +54,8 @@ func Generate(policy policies.Policy, globalSettings *ngfAPI.NginxProxy) []byte
5454
}
5555

5656
var spanAttributes []ngfAPI.SpanAttribute
57-
if globalSettings != nil && globalSettings.Spec.Telemetry != nil {
58-
spanAttributes = globalSettings.Spec.Telemetry.SpanAttributes
57+
if globalSettings != nil {
58+
spanAttributes = globalSettings.TracingSpanAttributes
5959
}
6060

6161
fields := map[string]interface{}{
@@ -73,5 +73,5 @@ func CreateRatioVarName(policy *ngfAPI.ObservabilityPolicy) string {
7373
namespace := strings.ReplaceAll(policy.GetNamespace(), "-", "_")
7474
name := strings.ReplaceAll(policy.GetName(), "-", "_")
7575

76-
return fmt.Sprintf("$ratio_%s_%s", namespace, name)
76+
return fmt.Sprintf("$ratio_ns_%s_name_%s", namespace, name)
7777
}

0 commit comments

Comments
 (0)