Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix status reporting for invalid HTTPRoutes #582

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Fix status reporting for invalid HTTPRoutes #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf47234
Fix status reporting for invalid HTTPRoutes
pleshakov Apr 18, 2023
a718459
Merge branch 'main' into fix/invalid-route-conditions
pleshakov Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions internal/state/graph/backend_refs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,9 +311,11 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) {

sectionNameRefs := []ParentRef{
{
Idx: 0,
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"},
Attached: true,
Idx: 0,
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"},
Attachment: &ParentRefAttachmentStatus{
Attached: true,
},
},
}

Expand Down
16 changes: 10 additions & 6 deletions internal/state/graph/graph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,11 @@ func TestBuildGraph(t *testing.T) {
Source: hr1,
ParentRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw1),
Attached: true,
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw1),
Attachment: &ParentRefAttachmentStatus{
Attached: true,
},
},
},
Rules: []Rule{createValidRuleWithBackendGroup(hr1Group)},
Expand All @@ -193,9 +195,11 @@ func TestBuildGraph(t *testing.T) {
Source: hr3,
ParentRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw1),
Attached: true,
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw1),
Attachment: &ParentRefAttachmentStatus{
Attached: true,
},
},
},
Rules: []Rule{createValidRuleWithBackendGroup(hr3Group)},
Expand Down
29 changes: 20 additions & 9 deletions internal/state/graph/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,20 @@ type Rule struct {

// ParentRef describes a reference to a parent in an HTTPRoute.
type ParentRef struct {
// FailedAttachmentCondition describes the failure of the attachment when Attached is false.
FailedAttachmentCondition conditions.Condition
// Attachment is the attachment status of the ParentRef. It could be nil. In that case, NGK didn't attempt to
// attach because of problems with the Route.
Attachment *ParentRefAttachmentStatus
// Gateway is the NamespacedName of the referenced Gateway
Gateway types.NamespacedName
// Idx is the index of the corresponding ParentReference in the HTTPRoute.
Idx int
}

// ParentRefAttachmentStatus describes the attachment status of a ParentRef.
type ParentRefAttachmentStatus struct {
// FailedCondition is the condition that describes why the ParentRef is not attached to the Gateway. It is set
// when Attached is false.
FailedCondition conditions.Condition
// Attached indicates if the ParentRef is attached to the Gateway.
Attached bool
}
Expand Down Expand Up @@ -280,7 +288,10 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
}

for i := 0; i < len(r.ParentRefs); i++ {
attachment := &ParentRefAttachmentStatus{}
ref := &r.ParentRefs[i]
ref.Attachment = attachment

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

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

if routeRef.SectionName == nil || *routeRef.SectionName == "" {
valErr := field.Required(path.Child("sectionName"), "cannot be empty")
ref.FailedAttachmentCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
continue
}

if routeRef.Port != nil {
valErr := field.Forbidden(path.Child("port"), "cannot be set")
ref.FailedAttachmentCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
continue
}

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

if !referencesWinningGw {
ref.FailedAttachmentCondition = conditions.NewTODO("Gateway is ignored")
attachment.FailedCondition = conditions.NewTODO("Gateway is ignored")
continue
}

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

if !l.Valid {
ref.FailedAttachmentCondition = conditions.NewRouteInvalidListener()
attachment.FailedCondition = conditions.NewRouteInvalidListener()
continue
}

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

if len(accepted) == 0 {
ref.FailedAttachmentCondition = conditions.NewRouteNoMatchingListenerHostname()
attachment.FailedCondition = conditions.NewRouteNoMatchingListenerHostname()
continue
}

ref.Attached = true
attachment.Attached = true

for _, h := range accepted {
l.AcceptedHostnames[h] = struct{}{}
Expand Down
115 changes: 71 additions & 44 deletions internal/state/graph/httproute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,12 @@ func TestBindRouteToListeners(t *testing.T) {
}
notValidRoute := &Route{
Valid: false,
ParentRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
},
},
}

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

tests := []struct {
route *Route
gateway *Gateway
expectedRouteUnattachedSectionNameRefs map[string]conditions.Condition
expectedGatewayListeners map[string]*Listener
name string
expectedSectionNameRefs []ParentRef
route *Route
gateway *Gateway
expectedGatewayListeners map[string]*Listener
name string
expectedSectionNameRefs []ParentRef
}{
{
route: createNormalRoute(),
Expand All @@ -790,9 +795,11 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: true,
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: true,
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -817,12 +824,14 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: false,
FailedAttachmentCondition: conditions.NewRouteUnsupportedValue(
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
),
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewRouteUnsupportedValue(
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -840,12 +849,14 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: false,
FailedAttachmentCondition: conditions.NewRouteUnsupportedValue(
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
),
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewRouteUnsupportedValue(
`spec.parentRefs[0].sectionName: Required value: cannot be empty`,
),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -863,12 +874,14 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: false,
FailedAttachmentCondition: conditions.NewRouteUnsupportedValue(
`spec.parentRefs[0].port: Forbidden: cannot be set`,
),
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewRouteUnsupportedValue(
`spec.parentRefs[0].port: Forbidden: cannot be set`,
),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -886,10 +899,12 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: false,
FailedAttachmentCondition: conditions.NewTODO("listener is not found"),
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewTODO("listener is not found"),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -907,10 +922,12 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: false,
FailedAttachmentCondition: conditions.NewRouteInvalidListener(),
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewRouteInvalidListener(),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -928,10 +945,12 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attached: false,
FailedAttachmentCondition: conditions.NewRouteNoMatchingListenerHostname(),
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewRouteNoMatchingListenerHostname(),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -949,10 +968,12 @@ func TestBindRouteToListeners(t *testing.T) {
},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: ignoredGwNsName,
Attached: false,
FailedAttachmentCondition: conditions.NewTODO("Gateway is ignored"),
Idx: 0,
Gateway: ignoredGwNsName,
Attachment: &ParentRefAttachmentStatus{
Attached: false,
FailedCondition: conditions.NewTODO("Gateway is ignored"),
},
},
},
expectedGatewayListeners: map[string]*Listener{
Expand All @@ -968,7 +989,13 @@ func TestBindRouteToListeners(t *testing.T) {
"listener-80-1": createListener(),
},
},
expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{},
expectedSectionNameRefs: []ParentRef{
{
Idx: 0,
Gateway: client.ObjectKeyFromObject(gw),
Attachment: nil,
},
},
expectedGatewayListeners: map[string]*Listener{
"listener-80-1": createListener(),
},
Expand Down
4 changes: 2 additions & 2 deletions internal/state/statuses.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func buildStatuses(graph *graph.Graph) Statuses {

for _, ref := range r.ParentRefs {
failedAttachmentCondCount := 0
if !ref.Attached {
if ref.Attachment != nil && !ref.Attachment.Attached {
failedAttachmentCondCount = 1
}
allConds := make([]conditions.Condition, 0, len(r.Conditions)+len(defaultConds)+failedAttachmentCondCount)
Expand All @@ -143,7 +143,7 @@ func buildStatuses(graph *graph.Graph) Statuses {
allConds = append(allConds, defaultConds...)
allConds = append(allConds, r.Conditions...)
if failedAttachmentCondCount == 1 {
allConds = append(allConds, ref.FailedAttachmentCondition)
allConds = append(allConds, ref.Attachment.FailedCondition)
}

routeRef := r.Source.Spec.ParentRefs[ref.Idx]
Expand Down
Loading