Skip to content

Commit 1bfa2e9

Browse files
tatarutytataruty
authored
Warnings for unsupported Gateway and Routes rules fields (#4036)
Proposed changes Problem: As an NGF user, I want clear feedback when I've specified a field that is unsupported by NGF, So that I understand why certain features aren't working. Routes should still be considered valid and configured, so we don't break existing users Solution: For all fields in various resources (Gateway, HTTPRoute, GRPCRoute, etc) that NGF does not yet support, add validation and error conditions if they are specified Routes should still be considered valid and configured, so we don't break existing users
1 parent a4ff583 commit 1bfa2e9

File tree

8 files changed

+697
-4
lines changed

8 files changed

+697
-4
lines changed

‎internal/controller/state/conditions/conditions.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ const (
3838
// Route rules has a backendRef with an unsupported value.
3939
RouteReasonBackendRefUnsupportedValue v1.RouteConditionReason = "UnsupportedValue"
4040

41+
// RouteReasonUnsupportedField is used with the "Accepted" condition when a Route contains fields that are
42+
// not yet supported.
43+
RouteReasonUnsupportedField v1.RouteConditionReason = "UnsupportedField"
44+
4145
// RouteReasonInvalidGateway is used with the "Accepted" (false) condition when the Gateway the Route
4246
// references is invalid.
4347
RouteReasonInvalidGateway v1.RouteConditionReason = "InvalidGateway"
@@ -62,6 +66,10 @@ const (
6266
// invalid. Used with ResolvedRefs (false).
6367
RouteReasonInvalidFilter v1.RouteConditionReason = "InvalidFilter"
6468

69+
// GatewayReasonUnsupportedField is used with the "Accepted" condition when a Gateway contains fields
70+
// that are not yet supported.
71+
GatewayReasonUnsupportedField v1.GatewayConditionReason = "UnsupportedField"
72+
6573
// GatewayReasonUnsupportedValue is used with GatewayConditionAccepted (false) when a value of a field in a Gateway
6674
// is invalid or not supported.
6775
GatewayReasonUnsupportedValue v1.GatewayConditionReason = "UnsupportedValue"
@@ -354,6 +362,17 @@ func NewRouteUnsupportedValue(msg string) Condition {
354362
}
355363
}
356364

365+
// NewRouteAcceptedUnsupportedField returns a Condition that indicates that the Route is accepted but
366+
// includes an unsupported field.
367+
func NewRouteAcceptedUnsupportedField(msg string) Condition {
368+
return Condition{
369+
Type: string(v1.RouteConditionAccepted),
370+
Status: metav1.ConditionTrue,
371+
Reason: string(RouteReasonUnsupportedField),
372+
Message: fmt.Sprintf("The following unsupported parameters were ignored: %s", msg),
373+
}
374+
}
375+
357376
// NewRoutePartiallyInvalid returns a Condition that indicates that the Route contains a combination
358377
// of both valid and invalid rules.
359378
//
@@ -945,6 +964,17 @@ func NewGatewayInvalidParameters(msg string) Condition {
945964
}
946965
}
947966

967+
// NewGatewayAcceptedUnsupportedField returns a Condition that indicates the Gateway is accepted but
968+
// contains a field that is not supported.
969+
func NewGatewayAcceptedUnsupportedField(msg string) Condition {
970+
return Condition{
971+
Type: string(v1.GatewayConditionAccepted),
972+
Status: metav1.ConditionTrue,
973+
Reason: string(GatewayReasonUnsupportedField),
974+
Message: fmt.Sprintf("Gateway accepted but the following unsupported parameters were ignored: %s", msg),
975+
}
976+
}
977+
948978
// NewPolicyAccepted returns a Condition that indicates that the Policy is accepted.
949979
func NewPolicyAccepted() Condition {
950980
return Condition{

‎internal/controller/state/graph/gateway.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,9 @@ func validateGateway(gw *v1.Gateway, gc *GatewayClass, npCfg *NginxProxy) ([]con
193193
// invalidate the entire Gateway.
194194
valid := len(conds) == 0
195195

196+
// Validate unsupported fields - these are warnings, don't affect validity
197+
conds = append(conds, validateUnsupportedGatewayFields(gw)...)
198+
196199
if gw.Spec.Infrastructure != nil && gw.Spec.Infrastructure.ParametersRef != nil {
197200
paramConds := validateGatewayParametersRef(npCfg, *gw.Spec.Infrastructure.ParametersRef)
198201
conds = append(conds, paramConds...)
@@ -267,3 +270,17 @@ func (g *Gateway) collectSnippetsFiltersFromRoute(
267270
}
268271
}
269272
}
273+
274+
func validateUnsupportedGatewayFields(gw *v1.Gateway) []conditions.Condition {
275+
var conds []conditions.Condition
276+
277+
if gw.Spec.AllowedListeners != nil {
278+
conds = append(conds, conditions.NewGatewayAcceptedUnsupportedField("AllowedListeners"))
279+
}
280+
281+
if gw.Spec.BackendTLS != nil {
282+
conds = append(conds, conditions.NewGatewayAcceptedUnsupportedField("BackendTLS"))
283+
}
284+
285+
return conds
286+
}

‎internal/controller/state/graph/gateway_test.go‎

Lines changed: 157 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -286,10 +286,12 @@ func TestBuildGateway(t *testing.T) {
286286
)
287287

288288
type gatewayCfg struct {
289-
name string
290-
ref *v1.LocalParametersReference
291-
listeners []v1.Listener
292-
addresses []v1.GatewaySpecAddress
289+
ref *v1.LocalParametersReference
290+
allowedListeners *v1.AllowedListeners
291+
backendTLS *v1.GatewayBackendTLS
292+
name string
293+
listeners []v1.Listener
294+
addresses []v1.GatewaySpecAddress
293295
}
294296

295297
var lastCreatedGateway *v1.Gateway
@@ -304,6 +306,8 @@ func TestBuildGateway(t *testing.T) {
304306
GatewayClassName: gcName,
305307
Listeners: cfg.listeners,
306308
Addresses: cfg.addresses,
309+
AllowedListeners: cfg.allowedListeners,
310+
BackendTLS: cfg.backendTLS,
307311
},
308312
}
309313

@@ -1513,6 +1517,103 @@ func TestBuildGateway(t *testing.T) {
15131517
},
15141518
},
15151519
},
1520+
{
1521+
name: "One unsupported field + supported fields (valid)",
1522+
gateway: createGateway(gatewayCfg{
1523+
name: "gateway-valid-np",
1524+
listeners: []v1.Listener{foo80Listener1},
1525+
ref: validGwNpRef,
1526+
allowedListeners: &v1.AllowedListeners{},
1527+
}),
1528+
gatewayClass: validGCWithNp,
1529+
expected: map[types.NamespacedName]*Gateway{
1530+
{Namespace: validGwNp.Namespace, Name: "gateway-valid-np"}: {
1531+
Source: getLastCreatedGateway(),
1532+
Listeners: []*Listener{
1533+
{
1534+
Name: "foo-80-1",
1535+
GatewayName: client.ObjectKeyFromObject(getLastCreatedGateway()),
1536+
Source: foo80Listener1,
1537+
Valid: true,
1538+
Attachable: true,
1539+
Routes: map[RouteKey]*L7Route{},
1540+
L4Routes: map[L4RouteKey]*L4Route{},
1541+
SupportedKinds: supportedKindsForListeners,
1542+
},
1543+
},
1544+
DeploymentName: types.NamespacedName{
1545+
Namespace: "test",
1546+
Name: controller.CreateNginxResourceName("gateway-valid-np", gcName),
1547+
},
1548+
Valid: true,
1549+
NginxProxy: &NginxProxy{
1550+
Source: validGwNp,
1551+
Valid: true,
1552+
},
1553+
EffectiveNginxProxy: &EffectiveNginxProxy{
1554+
Logging: &ngfAPIv1alpha2.NginxLogging{
1555+
ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelError),
1556+
},
1557+
IPFamily: helpers.GetPointer(ngfAPIv1alpha2.Dual),
1558+
Metrics: &ngfAPIv1alpha2.Metrics{
1559+
Disable: helpers.GetPointer(false),
1560+
Port: helpers.GetPointer(int32(90)),
1561+
},
1562+
},
1563+
Conditions: []conditions.Condition{
1564+
conditions.NewGatewayAcceptedUnsupportedField("AllowedListeners"),
1565+
conditions.NewGatewayResolvedRefs(),
1566+
},
1567+
},
1568+
},
1569+
},
1570+
{
1571+
name: "One unsupported field + NewGatewayRefInvalid (invalid)",
1572+
gateway: createGateway(gatewayCfg{
1573+
name: "gateway-valid-np",
1574+
listeners: []v1.Listener{foo80Listener1},
1575+
ref: &v1.LocalParametersReference{
1576+
Kind: "wrong-kind", // Invalid reference
1577+
Name: "invalid-ref",
1578+
},
1579+
backendTLS: &v1.GatewayBackendTLS{},
1580+
}),
1581+
gatewayClass: validGCWithNp,
1582+
expected: map[types.NamespacedName]*Gateway{
1583+
{Namespace: validGwNp.Namespace, Name: "gateway-valid-np"}: {
1584+
Source: getLastCreatedGateway(),
1585+
Listeners: []*Listener{
1586+
{
1587+
Name: "foo-80-1",
1588+
GatewayName: client.ObjectKeyFromObject(getLastCreatedGateway()),
1589+
Source: foo80Listener1,
1590+
Valid: true,
1591+
Attachable: true,
1592+
Routes: map[RouteKey]*L7Route{},
1593+
L4Routes: map[L4RouteKey]*L4Route{},
1594+
SupportedKinds: supportedKindsForListeners,
1595+
},
1596+
},
1597+
DeploymentName: types.NamespacedName{
1598+
Namespace: "test",
1599+
Name: controller.CreateNginxResourceName("gateway-valid-np", gcName),
1600+
},
1601+
Valid: true,
1602+
EffectiveNginxProxy: &EffectiveNginxProxy{
1603+
IPFamily: helpers.GetPointer(ngfAPIv1alpha2.Dual),
1604+
},
1605+
Conditions: []conditions.Condition{
1606+
conditions.NewGatewayAcceptedUnsupportedField("BackendTLS"),
1607+
conditions.NewGatewayRefInvalid(
1608+
"spec.infrastructure.parametersRef.kind: Unsupported value: \"wrong-kind\": supported values: \"NginxProxy\"",
1609+
),
1610+
conditions.NewGatewayInvalidParameters(
1611+
"spec.infrastructure.parametersRef.kind: Unsupported value: \"wrong-kind\": supported values: \"NginxProxy\"",
1612+
),
1613+
},
1614+
},
1615+
},
1616+
},
15161617
}
15171618

15181619
secretResolver := newSecretResolver(
@@ -1821,3 +1922,55 @@ func TestGetReferencedSnippetsFilters(t *testing.T) {
18211922
emptyFilterResult := gw.GetReferencedSnippetsFilters(routes, map[types.NamespacedName]*SnippetsFilter{})
18221923
g.Expect(emptyFilterResult).To(BeEmpty())
18231924
}
1925+
1926+
func TestValidateUnsupportedGatewayFields(t *testing.T) {
1927+
t.Parallel()
1928+
1929+
tests := []struct {
1930+
name string
1931+
gateway *v1.Gateway
1932+
expectedConds []conditions.Condition
1933+
}{
1934+
{
1935+
name: "No unsupported fields",
1936+
gateway: &v1.Gateway{
1937+
Spec: v1.GatewaySpec{},
1938+
},
1939+
expectedConds: nil,
1940+
},
1941+
{
1942+
name: "One unsupported field: AllowedListeners",
1943+
gateway: &v1.Gateway{
1944+
Spec: v1.GatewaySpec{
1945+
AllowedListeners: &v1.AllowedListeners{},
1946+
},
1947+
},
1948+
expectedConds: []conditions.Condition{
1949+
conditions.NewGatewayAcceptedUnsupportedField("AllowedListeners"),
1950+
},
1951+
},
1952+
{
1953+
name: "Multiple unsupported fields: AllowedListeners and BackendTLS",
1954+
gateway: &v1.Gateway{
1955+
Spec: v1.GatewaySpec{
1956+
AllowedListeners: &v1.AllowedListeners{},
1957+
BackendTLS: &v1.GatewayBackendTLS{},
1958+
},
1959+
},
1960+
expectedConds: []conditions.Condition{
1961+
conditions.NewGatewayAcceptedUnsupportedField("AllowedListeners"),
1962+
conditions.NewGatewayAcceptedUnsupportedField("BackendTLS"),
1963+
},
1964+
},
1965+
}
1966+
1967+
for _, test := range tests {
1968+
t.Run(test.name, func(t *testing.T) {
1969+
t.Parallel()
1970+
g := NewWithT(t)
1971+
1972+
conds := validateUnsupportedGatewayFields(test.gateway)
1973+
g.Expect(conds).To(Equal(test.expectedConds))
1974+
})
1975+
}
1976+
}

‎internal/controller/state/graph/grpcroute.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,11 @@ func processGRPCRouteRule(
165165

166166
validMatches := true
167167

168+
unsupportedFieldsErrors := checkForUnsupportedGRPCFields(specRule, rulePath)
169+
if len(unsupportedFieldsErrors) > 0 {
170+
errors.warn = append(errors.warn, unsupportedFieldsErrors...)
171+
}
172+
168173
for j, match := range specRule.Matches {
169174
matchPath := rulePath.Child("matches").Index(j)
170175

@@ -260,6 +265,11 @@ func processGRPCRouteRules(
260265
conds = make([]conditions.Condition, 0, 2)
261266
valid = true
262267

268+
// add warning condition for unsupported fields if any
269+
if len(allRulesErrors.warn) > 0 {
270+
conds = append(conds, conditions.NewRouteAcceptedUnsupportedField(allRulesErrors.warn.ToAggregate().Error()))
271+
}
272+
263273
if len(allRulesErrors.invalid) > 0 {
264274
msg := allRulesErrors.invalid.ToAggregate().Error()
265275

@@ -444,3 +454,26 @@ func validateGRPCHeaderMatch(
444454

445455
return allErrs
446456
}
457+
458+
func checkForUnsupportedGRPCFields(rule v1.GRPCRouteRule, rulePath *field.Path) field.ErrorList {
459+
var ruleErrors field.ErrorList
460+
461+
if rule.Name != nil {
462+
ruleErrors = append(ruleErrors, field.Forbidden(
463+
rulePath.Child("name"),
464+
"Name",
465+
))
466+
}
467+
if rule.SessionPersistence != nil {
468+
ruleErrors = append(ruleErrors, field.Forbidden(
469+
rulePath.Child("sessionPersistence"),
470+
"SessionPersistence",
471+
))
472+
}
473+
474+
if len(ruleErrors) == 0 {
475+
return nil
476+
}
477+
478+
return ruleErrors
479+
}

0 commit comments

Comments
 (0)