Skip to content

Commit eb9738e

Browse files
committed
Address recent comments
1 parent afb98b2 commit eb9738e

File tree

8 files changed

+91
-114
lines changed

8 files changed

+91
-114
lines changed

internal/mode/static/state/dataplane/configuration.go

+4-6
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,7 @@ func generateCertBundleID(configMap types.NamespacedName) CertBundleID {
607607

608608
// buildTelemetry generates the Otel configuration.
609609
func buildTelemetry(g *graph.Graph) Telemetry {
610-
if g.NginxProxy == nil ||
610+
if g.NginxProxy == nil || !g.NginxProxy.Valid ||
611611
g.NginxProxy.Source.Spec.Telemetry == nil ||
612612
g.NginxProxy.Source.Spec.Telemetry.Exporter == nil {
613613
return Telemetry{}
@@ -647,11 +647,9 @@ func buildTelemetry(g *graph.Graph) Telemetry {
647647
}
648648
}
649649

650-
if len(ratioMap) > 0 {
651-
tel.Ratios = make([]Ratio, 0, len(ratioMap))
652-
for name, ratio := range ratioMap {
653-
tel.Ratios = append(tel.Ratios, Ratio{Name: name, Value: ratio})
654-
}
650+
tel.Ratios = make([]Ratio, 0, len(ratioMap))
651+
for name, ratio := range ratioMap {
652+
tel.Ratios = append(tel.Ratios, Ratio{Name: name, Value: ratio})
655653
}
656654

657655
return tel

internal/mode/static/state/dataplane/configuration_test.go

+20
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,7 @@ func TestBuildConfiguration(t *testing.T) {
634634
DisableHTTP2: true,
635635
},
636636
},
637+
Valid: true,
637638
}
638639

639640
tests := []struct {
@@ -2068,6 +2069,7 @@ func TestBuildConfiguration(t *testing.T) {
20682069
BatchSize: 512,
20692070
BatchCount: 4,
20702071
ServiceName: "ngf:ns:gw:my-svc",
2072+
Ratios: []Ratio{},
20712073
},
20722074
},
20732075
msg: "NginxProxy with tracing config and http2 disabled",
@@ -2985,6 +2987,7 @@ func TestBuildTelemetry(t *testing.T) {
29852987
},
29862988
},
29872989
},
2990+
Valid: true,
29882991
}
29892992

29902993
createTelemetry := func() Telemetry {
@@ -2994,6 +2997,7 @@ func TestBuildTelemetry(t *testing.T) {
29942997
Interval: "5s",
29952998
BatchSize: 512,
29962999
BatchCount: 4,
3000+
Ratios: []Ratio{},
29973001
}
29983002
}
29993003

@@ -3015,6 +3019,22 @@ func TestBuildTelemetry(t *testing.T) {
30153019
expTelemetry: Telemetry{},
30163020
msg: "No telemetry configured",
30173021
},
3022+
{
3023+
g: &graph.Graph{
3024+
NginxProxy: &graph.NginxProxy{
3025+
Source: &ngfAPI.NginxProxy{
3026+
Spec: ngfAPI.NginxProxySpec{
3027+
Telemetry: &ngfAPI.Telemetry{
3028+
Exporter: &ngfAPI.TelemetryExporter{},
3029+
},
3030+
},
3031+
},
3032+
Valid: false,
3033+
},
3034+
},
3035+
expTelemetry: Telemetry{},
3036+
msg: "Invalid NginxProxy configured",
3037+
},
30183038
{
30193039
g: &graph.Graph{
30203040
Gateway: &graph.Gateway{

internal/mode/static/state/graph/gatewayclass.go

+3-9
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass"
1414
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
1515
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
16-
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
1716
)
1817

1918
// GatewayClass represents the GatewayClass resource.
@@ -66,13 +65,12 @@ func buildGatewayClass(
6665
gc *v1.GatewayClass,
6766
npCfg *NginxProxy,
6867
crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata,
69-
validator validation.GenericValidator,
7068
) *GatewayClass {
7169
if gc == nil {
7270
return nil
7371
}
7472

75-
conds, valid := validateGatewayClass(gc, npCfg, crdVersions, validator)
73+
conds, valid := validateGatewayClass(gc, npCfg, crdVersions)
7674

7775
return &GatewayClass{
7876
Source: gc,
@@ -85,7 +83,6 @@ func validateGatewayClass(
8583
gc *v1.GatewayClass,
8684
npCfg *NginxProxy,
8785
crdVersions map[types.NamespacedName]*metav1.PartialObjectMetadata,
88-
validator validation.GenericValidator,
8986
) ([]conditions.Condition, bool) {
9087
var conds []conditions.Condition
9188

@@ -97,11 +94,8 @@ func validateGatewayClass(
9794
} else if npCfg == nil {
9895
err = field.NotFound(path.Child("name"), gc.Spec.ParametersRef.Name)
9996
conds = append(conds, staticConds.NewGatewayClassRefNotFound())
100-
} else {
101-
nginxProxyErrs := validateNginxProxy(validator, npCfg)
102-
if len(nginxProxyErrs) > 0 {
103-
err = errors.New(nginxProxyErrs.ToAggregate().Error())
104-
}
97+
} else if !npCfg.Valid {
98+
err = errors.New(npCfg.ErrMsgs.ToAggregate().Error())
10599
}
106100

107101
if err != nil {

internal/mode/static/state/graph/gatewayclass_test.go

+14-36
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
package graph
22

33
import (
4-
"errors"
54
"testing"
65

76
. "github.com/onsi/gomega"
87
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
98
"k8s.io/apimachinery/pkg/types"
9+
"k8s.io/apimachinery/pkg/util/validation/field"
1010
"sigs.k8s.io/controller-runtime/pkg/client"
1111
v1 "sigs.k8s.io/gateway-api/apis/v1"
1212

@@ -16,8 +16,6 @@ import (
1616
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
1717
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
1818
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
19-
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
20-
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes"
2119
)
2220

2321
func TestProcessGatewayClasses(t *testing.T) {
@@ -166,24 +164,7 @@ func TestBuildGatewayClass(t *testing.T) {
166164
},
167165
}
168166

169-
createValidNPValidator := func() *validationfakes.FakeGenericValidator {
170-
v := &validationfakes.FakeGenericValidator{}
171-
v.ValidateServiceNameReturns(nil)
172-
v.ValidateEndpointReturns(nil)
173-
174-
return v
175-
}
176-
177-
createInvalidNPValidator := func() *validationfakes.FakeGenericValidator {
178-
v := &validationfakes.FakeGenericValidator{}
179-
v.ValidateServiceNameReturns(errors.New("error"))
180-
v.ValidateEndpointReturns(errors.New("error"))
181-
182-
return v
183-
}
184-
185167
tests := []struct {
186-
validator validation.GenericValidator
187168
gc *v1.GatewayClass
188169
np *NginxProxy
189170
crdMetadata map[types.NamespacedName]*metav1.PartialObjectMetadata
@@ -220,7 +201,6 @@ func TestBuildGatewayClass(t *testing.T) {
220201
},
221202
Valid: true,
222203
},
223-
validator: createValidNPValidator(),
224204
expected: &GatewayClass{
225205
Source: gcWithParams,
226206
Valid: true,
@@ -267,22 +247,20 @@ func TestBuildGatewayClass(t *testing.T) {
267247
{
268248
gc: gcWithParams,
269249
np: &NginxProxy{
270-
Source: &ngfAPI.NginxProxy{
271-
TypeMeta: metav1.TypeMeta{
272-
Kind: kinds.NginxProxy,
273-
},
274-
Spec: ngfAPI.NginxProxySpec{
275-
Telemetry: &ngfAPI.Telemetry{
276-
ServiceName: helpers.GetPointer("my-svc"),
277-
Exporter: &ngfAPI.TelemetryExporter{
278-
Endpoint: "my-endpoint",
279-
},
280-
},
281-
},
250+
Valid: false,
251+
ErrMsgs: field.ErrorList{
252+
field.Invalid(
253+
field.NewPath("spec", "telemetry", "serviceName"),
254+
"my-svc",
255+
"error",
256+
),
257+
field.Invalid(
258+
field.NewPath("spec", "telemetry", "exporter", "endpoint"),
259+
"my-endpoint",
260+
"error",
261+
),
282262
},
283-
Valid: true,
284263
},
285-
validator: createInvalidNPValidator(),
286264
expected: &GatewayClass{
287265
Source: gcWithParams,
288266
Valid: true,
@@ -312,7 +290,7 @@ func TestBuildGatewayClass(t *testing.T) {
312290
t.Run(test.name, func(t *testing.T) {
313291
g := NewWithT(t)
314292

315-
result := buildGatewayClass(test.gc, test.np, test.crdMetadata, test.validator)
293+
result := buildGatewayClass(test.gc, test.np, test.crdMetadata)
316294
g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty())
317295
if test.np != nil {
318296
g.Expect(test.np.Valid).ToNot(Equal(test.expNPInvalid))

internal/mode/static/state/graph/graph.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,8 @@ func BuildGraph(
185185
return &Graph{}
186186
}
187187

188-
npCfg := getNginxProxy(state.NginxProxies, processedGwClasses.Winner)
189-
gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata, validators.GenericValidator)
188+
npCfg := buildNginxProxy(state.NginxProxies, processedGwClasses.Winner, validators.GenericValidator)
189+
gc := buildGatewayClass(processedGwClasses.Winner, npCfg, state.CRDMetadata)
190190
if gc != nil && npCfg != nil && npCfg.Source != nil {
191191
spec := npCfg.Source.Spec
192192
globalSettings = &policies.GlobalPolicySettings{

internal/mode/static/state/graph/nginxproxy.go

+12-10
Original file line numberDiff line numberDiff line change
@@ -14,21 +14,27 @@ import (
1414
type NginxProxy struct {
1515
// Source is the source resource.
1616
Source *ngfAPI.NginxProxy
17+
// ErrMsgs contains the validation errors if they exist, to be included in the GatewayClass condition.
18+
ErrMsgs field.ErrorList
1719
// Valid shows whether the NginxProxy is valid.
1820
Valid bool
1921
}
2022

21-
// getNginxProxy returns the NginxProxy associated with the GatewayClass (if it exists).
22-
func getNginxProxy(
23+
// buildNginxProxy validates and returns the NginxProxy associated with the GatewayClass (if it exists).
24+
func buildNginxProxy(
2325
nps map[types.NamespacedName]*ngfAPI.NginxProxy,
2426
gc *v1.GatewayClass,
27+
validator validation.GenericValidator,
2528
) *NginxProxy {
2629
if gcReferencesAnyNginxProxy(gc) {
2730
npCfg := nps[types.NamespacedName{Name: gc.Spec.ParametersRef.Name}]
2831
if npCfg != nil {
32+
errs := validateNginxProxy(validator, npCfg)
33+
2934
return &NginxProxy{
30-
Source: npCfg,
31-
Valid: true,
35+
Source: npCfg,
36+
Valid: len(errs) == 0,
37+
ErrMsgs: errs,
3238
}
3339
}
3440
}
@@ -54,12 +60,12 @@ func gcReferencesAnyNginxProxy(gc *v1.GatewayClass) bool {
5460
// validateNginxProxy performs re-validation on string values in the case of CRD validation failure.
5561
func validateNginxProxy(
5662
validator validation.GenericValidator,
57-
npCfg *NginxProxy,
63+
npCfg *ngfAPI.NginxProxy,
5864
) field.ErrorList {
5965
var allErrs field.ErrorList
6066
spec := field.NewPath("spec")
6167

62-
telemetry := npCfg.Source.Spec.Telemetry
68+
telemetry := npCfg.Spec.Telemetry
6369
if telemetry != nil {
6470
telPath := spec.Child("telemetry")
6571
if telemetry.ServiceName != nil {
@@ -102,9 +108,5 @@ func validateNginxProxy(
102108
}
103109
}
104110

105-
if len(allErrs) > 0 {
106-
npCfg.Valid = false
107-
}
108-
109111
return allErrs
110112
}

0 commit comments

Comments
 (0)