Skip to content

Commit fc720bd

Browse files
miledxzmiledxz
committed
Fix status reporting for invalid HTTPRoutes (nginx#582)
918d650 introduced a bug which made NKG fail to report status conditions for an invalid HTTPRoute. It would try to report an empty condition in the status and fail because of the status CRD validation (example error): {"level":"error","ts":"2023-04-18T15:37:49Z","logger":"statusUpdater","msg":"Failed to update status","namespace":"default","name":"coffee","kind":"HTTPRoute","error":"HTTPRoute.gateway.networking.k8s.io \"coffee\" is invalid: [status.parents[0].conditions[1].reason: Invalid value: \"\": status.parents[0].conditions[1].reason in body should be at least 1 chars long, status.parents[0].conditions[1].status: Unsupported value: \"\": supported values: \"True\", \"False\", \"Unknown\", status.parents[0].conditions[1].type: Invalid value: \"\": status.parents[0].conditions[1].type in body should match '^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$']","stacktrace":"github.com/nginxinc/nginx-kubernetes-gateway/internal/status.(*updaterImpl).update\n\t/home/runner/work/nginx-kubernetes-gateway/nginx-kubernetes-gateway/internal/status/updater.go:168\ngithub.com/nginxinc/nginx-kubernetes-gateway/internal/status.(*updaterImpl).Update\n\t/home/runner/work/nginx-kubernetes-gateway/nginx-kubernetes-gateway/internal/status/updater.go:128\ngithub.com/nginxinc/nginx-kubernetes-gateway/internal/events.(*EventHandlerImpl).HandleEventBatch\n\t/home/runner/work/nginx-kubernetes-gateway/nginx-kubernetes-gateway/internal/events/handler.go:90\ngithub.com/nginxinc/nginx-kubernetes-gateway/internal/events.(*EventLoop).Start.func1.1\n\t/home/runner/work/nginx-kubernetes-gateway/nginx-kubernetes-gateway/internal/events/loop.go:67"} This commit fixes that bug.
1 parent 32b13fc commit fc720bd

File tree

6 files changed

+167
-76
lines changed

6 files changed

+167
-76
lines changed

‎internal/state/graph/backend_refs_test.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,9 +311,11 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) {
311311

312312
sectionNameRefs := []ParentRef{
313313
{
314-
Idx: 0,
315-
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"},
316-
Attached: true,
314+
Idx: 0,
315+
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"},
316+
Attachment: &ParentRefAttachmentStatus{
317+
Attached: true,
318+
},
317319
},
318320
}
319321

‎internal/state/graph/graph_test.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -180,9 +180,11 @@ func TestBuildGraph(t *testing.T) {
180180
Source: hr1,
181181
ParentRefs: []ParentRef{
182182
{
183-
Idx: 0,
184-
Gateway: client.ObjectKeyFromObject(gw1),
185-
Attached: true,
183+
Idx: 0,
184+
Gateway: client.ObjectKeyFromObject(gw1),
185+
Attachment: &ParentRefAttachmentStatus{
186+
Attached: true,
187+
},
186188
},
187189
},
188190
Rules: []Rule{createValidRuleWithBackendGroup(hr1Group)},
@@ -193,9 +195,11 @@ func TestBuildGraph(t *testing.T) {
193195
Source: hr3,
194196
ParentRefs: []ParentRef{
195197
{
196-
Idx: 0,
197-
Gateway: client.ObjectKeyFromObject(gw1),
198-
Attached: true,
198+
Idx: 0,
199+
Gateway: client.ObjectKeyFromObject(gw1),
200+
Attachment: &ParentRefAttachmentStatus{
201+
Attached: true,
202+
},
199203
},
200204
},
201205
Rules: []Rule{createValidRuleWithBackendGroup(hr3Group)},

‎internal/state/graph/httproute.go

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,20 @@ type Rule struct {
2727

2828
// ParentRef describes a reference to a parent in an HTTPRoute.
2929
type ParentRef struct {
30-
// FailedAttachmentCondition describes the failure of the attachment when Attached is false.
31-
FailedAttachmentCondition conditions.Condition
30+
// Attachment is the attachment status of the ParentRef. It could be nil. In that case, NGK didn't attempt to
31+
// attach because of problems with the Route.
32+
Attachment *ParentRefAttachmentStatus
3233
// Gateway is the NamespacedName of the referenced Gateway
3334
Gateway types.NamespacedName
3435
// Idx is the index of the corresponding ParentReference in the HTTPRoute.
3536
Idx int
37+
}
38+
39+
// ParentRefAttachmentStatus describes the attachment status of a ParentRef.
40+
type ParentRefAttachmentStatus struct {
41+
// FailedCondition is the condition that describes why the ParentRef is not attached to the Gateway. It is set
42+
// when Attached is false.
43+
FailedCondition conditions.Condition
3644
// Attached indicates if the ParentRef is attached to the Gateway.
3745
Attached bool
3846
}
@@ -280,7 +288,10 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
280288
}
281289

282290
for i := 0; i < len(r.ParentRefs); i++ {
291+
attachment := &ParentRefAttachmentStatus{}
283292
ref := &r.ParentRefs[i]
293+
ref.Attachment = attachment
294+
284295
routeRef := r.Source.Spec.ParentRefs[ref.Idx]
285296

286297
path := field.NewPath("spec").Child("parentRefs").Index(ref.Idx)
@@ -289,13 +300,13 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
289300

290301
if routeRef.SectionName == nil || *routeRef.SectionName == "" {
291302
valErr := field.Required(path.Child("sectionName"), "cannot be empty")
292-
ref.FailedAttachmentCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
303+
attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
293304
continue
294305
}
295306

296307
if routeRef.Port != nil {
297308
valErr := field.Forbidden(path.Child("port"), "cannot be set")
298-
ref.FailedAttachmentCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
309+
attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
299310
continue
300311
}
301312

@@ -304,7 +315,7 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
304315
referencesWinningGw := ref.Gateway.Namespace == gw.Source.Namespace && ref.Gateway.Name == gw.Source.Name
305316

306317
if !referencesWinningGw {
307-
ref.FailedAttachmentCondition = conditions.NewTODO("Gateway is ignored")
318+
attachment.FailedCondition = conditions.NewTODO("Gateway is ignored")
308319
continue
309320
}
310321

@@ -327,23 +338,23 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
327338
if !exists {
328339
// FIXME(pleshakov): Add a proper condition once it is available in the Gateway API.
329340
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306
330-
ref.FailedAttachmentCondition = conditions.NewTODO("listener is not found")
341+
attachment.FailedCondition = conditions.NewTODO("listener is not found")
331342
continue
332343
}
333344

334345
if !l.Valid {
335-
ref.FailedAttachmentCondition = conditions.NewRouteInvalidListener()
346+
attachment.FailedCondition = conditions.NewRouteInvalidListener()
336347
continue
337348
}
338349

339350
accepted := findAcceptedHostnames(l.Source.Hostname, r.Source.Spec.Hostnames)
340351

341352
if len(accepted) == 0 {
342-
ref.FailedAttachmentCondition = conditions.NewRouteNoMatchingListenerHostname()
353+
attachment.FailedCondition = conditions.NewRouteNoMatchingListenerHostname()
343354
continue
344355
}
345356

346-
ref.Attached = true
357+
attachment.Attached = true
347358

348359
for _, h := range accepted {
349360
l.AcceptedHostnames[h] = struct{}{}

‎internal/state/graph/httproute_test.go

Lines changed: 71 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -763,6 +763,12 @@ func TestBindRouteToListeners(t *testing.T) {
763763
}
764764
notValidRoute := &Route{
765765
Valid: false,
766+
ParentRefs: []ParentRef{
767+
{
768+
Idx: 0,
769+
Gateway: client.ObjectKeyFromObject(gw),
770+
},
771+
},
766772
}
767773

768774
notValidListener := createModifiedListener(func(l *Listener) {
@@ -773,12 +779,11 @@ func TestBindRouteToListeners(t *testing.T) {
773779
})
774780

775781
tests := []struct {
776-
route *Route
777-
gateway *Gateway
778-
expectedRouteUnattachedSectionNameRefs map[string]conditions.Condition
779-
expectedGatewayListeners map[string]*Listener
780-
name string
781-
expectedSectionNameRefs []ParentRef
782+
route *Route
783+
gateway *Gateway
784+
expectedGatewayListeners map[string]*Listener
785+
name string
786+
expectedSectionNameRefs []ParentRef
782787
}{
783788
{
784789
route: createNormalRoute(),
@@ -790,9 +795,11 @@ func TestBindRouteToListeners(t *testing.T) {
790795
},
791796
expectedSectionNameRefs: []ParentRef{
792797
{
793-
Idx: 0,
794-
Gateway: client.ObjectKeyFromObject(gw),
795-
Attached: true,
798+
Idx: 0,
799+
Gateway: client.ObjectKeyFromObject(gw),
800+
Attachment: &ParentRefAttachmentStatus{
801+
Attached: true,
802+
},
796803
},
797804
},
798805
expectedGatewayListeners: map[string]*Listener{
@@ -817,12 +824,14 @@ func TestBindRouteToListeners(t *testing.T) {
817824
},
818825
expectedSectionNameRefs: []ParentRef{
819826
{
820-
Idx: 0,
821-
Gateway: client.ObjectKeyFromObject(gw),
822-
Attached: false,
823-
FailedAttachmentCondition: conditions.NewRouteUnsupportedValue(
824-
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
825-
),
827+
Idx: 0,
828+
Gateway: client.ObjectKeyFromObject(gw),
829+
Attachment: &ParentRefAttachmentStatus{
830+
Attached: false,
831+
FailedCondition: conditions.NewRouteUnsupportedValue(
832+
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
833+
),
834+
},
826835
},
827836
},
828837
expectedGatewayListeners: map[string]*Listener{
@@ -840,12 +849,14 @@ func TestBindRouteToListeners(t *testing.T) {
840849
},
841850
expectedSectionNameRefs: []ParentRef{
842851
{
843-
Idx: 0,
844-
Gateway: client.ObjectKeyFromObject(gw),
845-
Attached: false,
846-
FailedAttachmentCondition: conditions.NewRouteUnsupportedValue(
847-
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
848-
),
852+
Idx: 0,
853+
Gateway: client.ObjectKeyFromObject(gw),
854+
Attachment: &ParentRefAttachmentStatus{
855+
Attached: false,
856+
FailedCondition: conditions.NewRouteUnsupportedValue(
857+
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
858+
),
859+
},
849860
},
850861
},
851862
expectedGatewayListeners: map[string]*Listener{
@@ -863,12 +874,14 @@ func TestBindRouteToListeners(t *testing.T) {
863874
},
864875
expectedSectionNameRefs: []ParentRef{
865876
{
866-
Idx: 0,
867-
Gateway: client.ObjectKeyFromObject(gw),
868-
Attached: false,
869-
FailedAttachmentCondition: conditions.NewRouteUnsupportedValue(
870-
`spec.parentRefs[0].port: Forbidden: cannot be set`,
871-
),
877+
Idx: 0,
878+
Gateway: client.ObjectKeyFromObject(gw),
879+
Attachment: &ParentRefAttachmentStatus{
880+
Attached: false,
881+
FailedCondition: conditions.NewRouteUnsupportedValue(
882+
`spec.parentRefs[0].port: Forbidden: cannot be set`,
883+
),
884+
},
872885
},
873886
},
874887
expectedGatewayListeners: map[string]*Listener{
@@ -886,10 +899,12 @@ func TestBindRouteToListeners(t *testing.T) {
886899
},
887900
expectedSectionNameRefs: []ParentRef{
888901
{
889-
Idx: 0,
890-
Gateway: client.ObjectKeyFromObject(gw),
891-
Attached: false,
892-
FailedAttachmentCondition: conditions.NewTODO("listener is not found"),
902+
Idx: 0,
903+
Gateway: client.ObjectKeyFromObject(gw),
904+
Attachment: &ParentRefAttachmentStatus{
905+
Attached: false,
906+
FailedCondition: conditions.NewTODO("listener is not found"),
907+
},
893908
},
894909
},
895910
expectedGatewayListeners: map[string]*Listener{
@@ -907,10 +922,12 @@ func TestBindRouteToListeners(t *testing.T) {
907922
},
908923
expectedSectionNameRefs: []ParentRef{
909924
{
910-
Idx: 0,
911-
Gateway: client.ObjectKeyFromObject(gw),
912-
Attached: false,
913-
FailedAttachmentCondition: conditions.NewRouteInvalidListener(),
925+
Idx: 0,
926+
Gateway: client.ObjectKeyFromObject(gw),
927+
Attachment: &ParentRefAttachmentStatus{
928+
Attached: false,
929+
FailedCondition: conditions.NewRouteInvalidListener(),
930+
},
914931
},
915932
},
916933
expectedGatewayListeners: map[string]*Listener{
@@ -928,10 +945,12 @@ func TestBindRouteToListeners(t *testing.T) {
928945
},
929946
expectedSectionNameRefs: []ParentRef{
930947
{
931-
Idx: 0,
932-
Gateway: client.ObjectKeyFromObject(gw),
933-
Attached: false,
934-
FailedAttachmentCondition: conditions.NewRouteNoMatchingListenerHostname(),
948+
Idx: 0,
949+
Gateway: client.ObjectKeyFromObject(gw),
950+
Attachment: &ParentRefAttachmentStatus{
951+
Attached: false,
952+
FailedCondition: conditions.NewRouteNoMatchingListenerHostname(),
953+
},
935954
},
936955
},
937956
expectedGatewayListeners: map[string]*Listener{
@@ -949,10 +968,12 @@ func TestBindRouteToListeners(t *testing.T) {
949968
},
950969
expectedSectionNameRefs: []ParentRef{
951970
{
952-
Idx: 0,
953-
Gateway: ignoredGwNsName,
954-
Attached: false,
955-
FailedAttachmentCondition: conditions.NewTODO("Gateway is ignored"),
971+
Idx: 0,
972+
Gateway: ignoredGwNsName,
973+
Attachment: &ParentRefAttachmentStatus{
974+
Attached: false,
975+
FailedCondition: conditions.NewTODO("Gateway is ignored"),
976+
},
956977
},
957978
},
958979
expectedGatewayListeners: map[string]*Listener{
@@ -968,7 +989,13 @@ func TestBindRouteToListeners(t *testing.T) {
968989
"listener-80-1": createListener(),
969990
},
970991
},
971-
expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{},
992+
expectedSectionNameRefs: []ParentRef{
993+
{
994+
Idx: 0,
995+
Gateway: client.ObjectKeyFromObject(gw),
996+
Attachment: nil,
997+
},
998+
},
972999
expectedGatewayListeners: map[string]*Listener{
9731000
"listener-80-1": createListener(),
9741001
},

‎internal/state/statuses.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func buildStatuses(graph *graph.Graph) Statuses {
133133

134134
for _, ref := range r.ParentRefs {
135135
failedAttachmentCondCount := 0
136-
if !ref.Attached {
136+
if ref.Attachment != nil && !ref.Attachment.Attached {
137137
failedAttachmentCondCount = 1
138138
}
139139
allConds := make([]conditions.Condition, 0, len(r.Conditions)+len(defaultConds)+failedAttachmentCondCount)
@@ -143,7 +143,7 @@ func buildStatuses(graph *graph.Graph) Statuses {
143143
allConds = append(allConds, defaultConds...)
144144
allConds = append(allConds, r.Conditions...)
145145
if failedAttachmentCondCount == 1 {
146-
allConds = append(allConds, ref.FailedAttachmentCondition)
146+
allConds = append(allConds, ref.Attachment.FailedCondition)
147147
}
148148

149149
routeRef := r.Source.Spec.ParentRefs[ref.Idx]

0 commit comments

Comments
 (0)