From 1267e388b7802d7bfcf32ae37fda1ab32fbe18c9 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 27 Jan 2023 15:43:09 -0600 Subject: [PATCH 1/8] Transform RouteCondition into general Condition This will allow use the type for Gateway and other resources, not just HTTPRoute --- internal/state/change_processor_test.go | 8 +- internal/state/conditions/conditions.go | 86 +++++++++++++++++++ .../{httproute_test.go => conditions_test.go} | 6 +- internal/state/conditions/httproute.go | 86 ------------------- internal/state/configuration_test.go | 4 +- internal/state/graph.go | 10 +-- internal/state/graph_test.go | 24 +++--- internal/state/statuses.go | 12 +-- internal/state/statuses_test.go | 14 +-- internal/status/conditions.go | 28 ++++++ internal/status/conditions_test.go | 57 ++++++++++++ internal/status/httproute.go | 24 +----- internal/status/httproute_test.go | 57 ++---------- internal/status/updater_test.go | 2 +- 14 files changed, 218 insertions(+), 200 deletions(-) create mode 100644 internal/state/conditions/conditions.go rename internal/state/conditions/{httproute_test.go => conditions_test.go} (87%) delete mode 100644 internal/state/conditions/httproute.go create mode 100644 internal/status/conditions.go create mode 100644 internal/status/conditions_test.go diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index d4910098af..8052448f77 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -303,13 +303,13 @@ var _ = Describe("ChangeProcessor", func() { "listener-80-1": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, "listener-443-1": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, }, @@ -960,13 +960,13 @@ var _ = Describe("ChangeProcessor", func() { "listener-80-1": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("Gateway is ignored"), + conditions.NewTODO("Gateway is ignored"), ), }, "listener-443-1": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("Gateway is ignored"), + conditions.NewTODO("Gateway is ignored"), ), }, }, diff --git a/internal/state/conditions/conditions.go b/internal/state/conditions/conditions.go new file mode 100644 index 0000000000..8a4c5c4943 --- /dev/null +++ b/internal/state/conditions/conditions.go @@ -0,0 +1,86 @@ +package conditions + +import ( + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1beta1" +) + +// RouteReasonInvalidListener is used with the "Accepted" condition when the route references an invalid listener. +const RouteReasonInvalidListener v1beta1.RouteConditionReason = "InvalidListener" + +// Condition defines a condition to be reported in the status of resources. +type Condition struct { + Type string + Status metav1.ConditionStatus + Reason string + Message string +} + +// DeduplicateConditions removes duplicate conditions based on the condition type. +// The last condition wins. +func DeduplicateConditions(conds []Condition) []Condition { + uniqueConds := make(map[string]Condition) + + for _, cond := range conds { + uniqueConds[cond.Type] = cond + } + + result := make([]Condition, 0, len(uniqueConds)) + + for _, cond := range uniqueConds { + result = append(result, cond) + } + + return result +} + +// NewDefaultRouteConditions returns the default conditions that must be present in the status of an HTTPRoute. +func NewDefaultRouteConditions() []Condition { + return []Condition{ + NewRouteAccepted(), + } +} + +// NewRouteNoMatchingListenerHostname returns a Condition that indicates that the hostname of the listener +// does not match the hostnames of the HTTPRoute. +func NewRouteNoMatchingListenerHostname() Condition { + return Condition{ + Type: string(v1beta1.RouteConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.RouteReasonNoMatchingListenerHostname), + Message: "Listener hostname does not match the HTTPRoute hostnames", + } +} + +// NewRouteAccepted returns a Condition that indicates that the HTTPRoute is accepted. +func NewRouteAccepted() Condition { + return Condition{ + Type: string(v1beta1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.RouteReasonAccepted), + Message: "The route is accepted", + } +} + +// NewTODO returns a Condition that can be used as a placeholder for a condition that is not yet implemented. +func NewTODO(msg string) Condition { + return Condition{ + Type: "TODO", + Status: metav1.ConditionTrue, + Reason: "TODO", + Message: fmt.Sprintf("The condition for this has not been implemented yet: %s", msg), + } +} + +// NewRouteInvalidListener returns a Condition that indicates that the HTTPRoute is not accepted because of an +// invalid listener. +func NewRouteInvalidListener() Condition { + return Condition{ + Type: string(v1beta1.RouteConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(RouteReasonInvalidListener), + Message: "The listener is invalid for this parent ref", + } +} diff --git a/internal/state/conditions/httproute_test.go b/internal/state/conditions/conditions_test.go similarity index 87% rename from internal/state/conditions/httproute_test.go rename to internal/state/conditions/conditions_test.go index d514ae7c07..7d3b6aefc0 100644 --- a/internal/state/conditions/httproute_test.go +++ b/internal/state/conditions/conditions_test.go @@ -10,7 +10,7 @@ import ( func TestDeduplicateDeduplicateRouteConditions(t *testing.T) { g := NewGomegaWithT(t) - conds := []RouteCondition{ + conds := []Condition{ { Type: "Type1", Status: metav1.ConditionTrue, @@ -33,7 +33,7 @@ func TestDeduplicateDeduplicateRouteConditions(t *testing.T) { }, } - expected := []RouteCondition{ + expected := []Condition{ { Type: "Type1", Status: metav1.ConditionFalse, @@ -48,6 +48,6 @@ func TestDeduplicateDeduplicateRouteConditions(t *testing.T) { }, } - result := DeduplicateRouteConditions(conds) + result := DeduplicateConditions(conds) g.Expect(result).Should(ConsistOf(expected)) } diff --git a/internal/state/conditions/httproute.go b/internal/state/conditions/httproute.go deleted file mode 100644 index d63464a253..0000000000 --- a/internal/state/conditions/httproute.go +++ /dev/null @@ -1,86 +0,0 @@ -package conditions - -import ( - "fmt" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/gateway-api/apis/v1beta1" -) - -// RouteReasonInvalidListener is used with the "Accepted" condition when the route references an invalid listener. -const RouteReasonInvalidListener v1beta1.RouteConditionReason = "InvalidListener" - -// RouteCondition defines a condition to be reported in the status of an HTTPRoute. -type RouteCondition struct { - Type v1beta1.RouteConditionType - Status metav1.ConditionStatus - Reason v1beta1.RouteConditionReason - Message string -} - -// DeduplicateRouteConditions removes duplicate conditions based on the condition type. -// The last condition wins. -func DeduplicateRouteConditions(conds []RouteCondition) []RouteCondition { - uniqueConds := make(map[v1beta1.RouteConditionType]RouteCondition) - - for _, cond := range conds { - uniqueConds[cond.Type] = cond - } - - result := make([]RouteCondition, 0, len(uniqueConds)) - - for _, cond := range uniqueConds { - result = append(result, cond) - } - - return result -} - -// NewDefaultRouteConditions returns the default conditions that must be present in the status of an HTTPRoute. -func NewDefaultRouteConditions() []RouteCondition { - return []RouteCondition{ - NewRouteAccepted(), - } -} - -// NewRouteNoMatchingListenerHostname returns a RouteCondition that indicates that the hostname of the listener -// does not match the hostnames of the HTTPRoute. -func NewRouteNoMatchingListenerHostname() RouteCondition { - return RouteCondition{ - Type: v1beta1.RouteConditionAccepted, - Status: metav1.ConditionFalse, - Reason: v1beta1.RouteReasonNoMatchingListenerHostname, - Message: "Listener hostname does not match the HTTPRoute hostnames", - } -} - -// NewRouteAccepted returns a RouteCondition that indicates that the HTTPRoute is accepted. -func NewRouteAccepted() RouteCondition { - return RouteCondition{ - Type: v1beta1.RouteConditionAccepted, - Status: metav1.ConditionTrue, - Reason: v1beta1.RouteReasonAccepted, - Message: "The route is accepted", - } -} - -// NewRouteTODO returns a RouteCondition that can be used as a placeholder for a condition that is not yet implemented. -func NewRouteTODO(msg string) RouteCondition { - return RouteCondition{ - Type: "TODO", - Status: metav1.ConditionTrue, - Reason: "TODO", - Message: fmt.Sprintf("The condition for this has not been implemented yet: %s", msg), - } -} - -// NewRouteInvalidListener returns a RouteCondition that indicates that the HTTPRoute is not accepted because of an -// invalid listener. -func NewRouteInvalidListener() RouteCondition { - return RouteCondition{ - Type: v1beta1.RouteConditionAccepted, - Status: metav1.ConditionFalse, - Reason: RouteReasonInvalidListener, - Message: "The listener is invalid for this parent ref", - } -} diff --git a/internal/state/configuration_test.go b/internal/state/configuration_test.go index 949e78b115..b5ad796374 100644 --- a/internal/state/configuration_test.go +++ b/internal/state/configuration_test.go @@ -101,7 +101,7 @@ func TestBuildConfiguration(t *testing.T) { createInternalRoute := func(source *v1beta1.HTTPRoute, validSectionName string, groups ...BackendGroup) *route { r := &route{ Source: source, - InvalidSectionNameRefs: make(map[string]conditions.RouteCondition), + InvalidSectionNameRefs: make(map[string]conditions.Condition), ValidSectionNameRefs: map[string]struct{}{validSectionName: {}}, BackendGroups: groups, } @@ -180,7 +180,7 @@ func TestBuildConfiguration(t *testing.T) { routeHR5 := &route{ Source: hr5, - InvalidSectionNameRefs: make(map[string]conditions.RouteCondition), + InvalidSectionNameRefs: make(map[string]conditions.Condition), ValidSectionNameRefs: map[string]struct{}{"listener-80-1": {}}, BackendGroups: []BackendGroup{hr5BackendGroup}, } diff --git a/internal/state/graph.go b/internal/state/graph.go index 1e9587dc23..f29cfbd3ed 100644 --- a/internal/state/graph.go +++ b/internal/state/graph.go @@ -31,8 +31,8 @@ type route struct { // the Gateway resource has a corresponding valid listener. ValidSectionNameRefs map[string]struct{} // InvalidSectionNameRefs includes the sectionNames from the parentRefs of the HTTPRoute that are invalid. - // The RouteCondition describes why the sectionName is invalid. - InvalidSectionNameRefs map[string]conditions.RouteCondition + // The Condition describes why the sectionName is invalid. + InvalidSectionNameRefs map[string]conditions.Condition // BackendGroups includes the backend groups of the HTTPRoute. // There's one BackendGroup per rule in the HTTPRoute. // The BackendGroups are stored in order of the rules. @@ -192,7 +192,7 @@ func bindHTTPRouteToListeners( r = &route{ Source: ghr, ValidSectionNameRefs: make(map[string]struct{}), - InvalidSectionNameRefs: make(map[string]conditions.RouteCondition), + InvalidSectionNameRefs: make(map[string]conditions.Condition), } // FIXME (pleshakov) Handle the case when parent refs are duplicated @@ -238,7 +238,7 @@ func bindHTTPRouteToListeners( 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 - r.InvalidSectionNameRefs[name] = conditions.NewRouteTODO("listener is not found") + r.InvalidSectionNameRefs[name] = conditions.NewTODO("listener is not found") continue } @@ -269,7 +269,7 @@ func bindHTTPRouteToListeners( if _, exist := ignoredGws[key]; exist { // FIXME(pleshakov): Add a proper condition. // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 - r.InvalidSectionNameRefs[name] = conditions.NewRouteTODO("Gateway is ignored") + r.InvalidSectionNameRefs[name] = conditions.NewTODO("Gateway is ignored") processed = true continue diff --git a/internal/state/graph_test.go b/internal/state/graph_test.go index 7d776511f5..ae9c81a324 100644 --- a/internal/state/graph_test.go +++ b/internal/state/graph_test.go @@ -231,7 +231,7 @@ func TestBuildGraph(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, + InvalidSectionNameRefs: map[string]conditions.Condition{}, BackendGroups: []BackendGroup{hr1Group}, } @@ -240,7 +240,7 @@ func TestBuildGraph(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-443-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, + InvalidSectionNameRefs: map[string]conditions.Condition{}, BackendGroups: []BackendGroup{hr3Group}, } @@ -879,8 +879,8 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrNonExistingSectionName, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{ - "listener-80-2": conditions.NewRouteTODO("listener is not found"), + InvalidSectionNameRefs: map[string]conditions.Condition{ + "listener-80-2": conditions.NewTODO("listener is not found"), }, }, expectedListeners: map[string]*listener{ @@ -915,7 +915,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, + InvalidSectionNameRefs: map[string]conditions.Condition{}, }, expectedListeners: map[string]*listener{ "listener-80-1": createModifiedListener(func(l *listener) { @@ -925,7 +925,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, + InvalidSectionNameRefs: map[string]conditions.Condition{}, }, } l.AcceptedHostnames = map[string]struct{}{ @@ -948,7 +948,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, + InvalidSectionNameRefs: map[string]conditions.Condition{}, }, expectedListeners: map[string]*listener{ "listener-80-1": createModifiedListener(func(l *listener) { @@ -958,7 +958,7 @@ func TestBindRouteToListeners(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{}, + InvalidSectionNameRefs: map[string]conditions.Condition{}, }, } l.AcceptedHostnames = map[string]struct{}{ @@ -979,7 +979,7 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrBar, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + InvalidSectionNameRefs: map[string]conditions.Condition{ "listener-80-1": conditions.NewRouteNoMatchingListenerHostname(), }, }, @@ -1001,8 +1001,8 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrIgnoredGateway, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{ - "listener-80-1": conditions.NewRouteTODO("Gateway is ignored"), + InvalidSectionNameRefs: map[string]conditions.Condition{ + "listener-80-1": conditions.NewTODO("Gateway is ignored"), }, }, expectedListeners: map[string]*listener{ @@ -1033,7 +1033,7 @@ func TestBindRouteToListeners(t *testing.T) { expectedRoute: &route{ Source: hrFoo, ValidSectionNameRefs: map[string]struct{}{}, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + InvalidSectionNameRefs: map[string]conditions.Condition{ "listener-80-1": conditions.NewRouteInvalidListener(), }, }, diff --git a/internal/state/statuses.go b/internal/state/statuses.go index a277c85fab..f6cdf6d608 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -61,7 +61,7 @@ type HTTPRouteStatus struct { // ParentStatus holds status-related information related to how the HTTPRoute binds to a specific parentRef. type ParentStatus struct { // Conditions is the list of conditions that are relevant to the parentRef. - Conditions []conditions.RouteCondition + Conditions []conditions.Condition } // GatewayClassStatus holds status-related infortmation about the GatewayClass resource. @@ -117,7 +117,7 @@ func buildStatuses(graph *graph) Statuses { for ref := range r.ValidSectionNameRefs { parentStatuses[ref] = ParentStatus{ - Conditions: conditions.DeduplicateRouteConditions( + Conditions: conditions.DeduplicateConditions( buildBaseRouteConditions(gcValidAndExist), ), } @@ -125,12 +125,12 @@ func buildStatuses(graph *graph) Statuses { for ref, cond := range r.InvalidSectionNameRefs { baseConds := buildBaseRouteConditions(gcValidAndExist) - conds := make([]conditions.RouteCondition, 0, len(baseConds)+1) + conds := make([]conditions.Condition, 0, len(baseConds)+1) conds = append(conds, baseConds...) conds = append(conds, cond) parentStatuses[ref] = ParentStatus{ - Conditions: conditions.DeduplicateRouteConditions(conds), + Conditions: conditions.DeduplicateConditions(conds), } } @@ -143,7 +143,7 @@ func buildStatuses(graph *graph) Statuses { return statuses } -func buildBaseRouteConditions(gcValidAndExist bool) []conditions.RouteCondition { +func buildBaseRouteConditions(gcValidAndExist bool) []conditions.Condition { conds := conditions.NewDefaultRouteConditions() // FIXME(pleshakov): Figure out appropriate conditions for the cases when: @@ -151,7 +151,7 @@ func buildBaseRouteConditions(gcValidAndExist bool) []conditions.RouteCondition // (2) GatewayClass does not exist. // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/307 if !gcValidAndExist { - conds = append(conds, conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist")) + conds = append(conds, conditions.NewTODO("GatewayClass is invalid or doesn't exist")) } return conds diff --git a/internal/state/statuses_test.go b/internal/state/statuses_test.go index ecf001f765..52f6502793 100644 --- a/internal/state/statuses_test.go +++ b/internal/state/statuses_test.go @@ -13,7 +13,7 @@ import ( ) func TestBuildStatuses(t *testing.T) { - invalidCondition := conditions.RouteCondition{ + invalidCondition := conditions.Condition{ Type: "Test", Status: metav1.ConditionTrue, } @@ -37,7 +37,7 @@ func TestBuildStatuses(t *testing.T) { ValidSectionNameRefs: map[string]struct{}{ "listener-80-1": {}, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + InvalidSectionNameRefs: map[string]conditions.Condition{ "listener-80-2": invalidCondition, }, }, @@ -50,7 +50,7 @@ func TestBuildStatuses(t *testing.T) { Generation: 4, }, }, - InvalidSectionNameRefs: map[string]conditions.RouteCondition{ + InvalidSectionNameRefs: map[string]conditions.Condition{ "listener-80-2": invalidCondition, "listener-80-1": invalidCondition, }, @@ -166,13 +166,13 @@ func TestBuildStatuses(t *testing.T) { "listener-80-1": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, "listener-80-2": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), invalidCondition, ), }, @@ -226,13 +226,13 @@ func TestBuildStatuses(t *testing.T) { "listener-80-1": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, "listener-80-2": { Conditions: append( conditions.NewDefaultRouteConditions(), - conditions.NewRouteTODO("GatewayClass is invalid or doesn't exist"), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), invalidCondition, ), }, diff --git a/internal/status/conditions.go b/internal/status/conditions.go new file mode 100644 index 0000000000..c9ce4c21b1 --- /dev/null +++ b/internal/status/conditions.go @@ -0,0 +1,28 @@ +package status + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" +) + +func convertConditions( + conds []conditions.Condition, + observedGeneration int64, + transitionTime metav1.Time, +) []metav1.Condition { + apiConds := make([]metav1.Condition, len(conds)) + + for i := range conds { + apiConds[i] = metav1.Condition{ + Type: conds[i].Type, + Status: conds[i].Status, + ObservedGeneration: observedGeneration, + LastTransitionTime: transitionTime, + Reason: conds[i].Reason, + Message: conds[i].Message, + } + } + + return apiConds +} diff --git a/internal/status/conditions_test.go b/internal/status/conditions_test.go new file mode 100644 index 0000000000..4e575483e2 --- /dev/null +++ b/internal/status/conditions_test.go @@ -0,0 +1,57 @@ +package status + +import ( + "testing" + "time" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" +) + +func TestConvertRouteConditions(t *testing.T) { + g := NewGomegaWithT(t) + + routeConds := []conditions.Condition{ + { + Type: "Test", + Status: metav1.ConditionTrue, + Reason: "reason1", + Message: "message1", + }, + { + Type: "Test", + Status: metav1.ConditionFalse, + Reason: "reason2", + Message: "message2", + }, + } + + var generation int64 = 1 + transitionTime := metav1.NewTime(time.Now()) + + expected := []metav1.Condition{ + { + Type: "Test", + Status: metav1.ConditionTrue, + ObservedGeneration: generation, + LastTransitionTime: transitionTime, + Reason: "reason1", + Message: "message1", + }, + { + Type: "Test", + Status: metav1.ConditionFalse, + ObservedGeneration: generation, + LastTransitionTime: transitionTime, + Reason: "reason2", + Message: "message2", + }, + } + + result := convertConditions(routeConds, generation, transitionTime) + + g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) +} diff --git a/internal/status/httproute.go b/internal/status/httproute.go index f2c928f090..d6a8369302 100644 --- a/internal/status/httproute.go +++ b/internal/status/httproute.go @@ -8,7 +8,6 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) // prepareHTTPRouteStatus prepares the status for an HTTPRoute resource. @@ -42,7 +41,7 @@ func prepareHTTPRouteStatus( SectionName: (*v1beta1.SectionName)(§ionName), }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), - Conditions: convertRouteConditions(ps.Conditions, status.ObservedGeneration, transitionTime), + Conditions: convertConditions(ps.Conditions, status.ObservedGeneration, transitionTime), } parents = append(parents, p) } @@ -53,24 +52,3 @@ func prepareHTTPRouteStatus( }, } } - -func convertRouteConditions( - routeConds []conditions.RouteCondition, - observedGeneration int64, - transitionTime metav1.Time, -) []metav1.Condition { - conds := make([]metav1.Condition, len(routeConds)) - - for i := range routeConds { - conds[i] = metav1.Condition{ - Type: string(routeConds[i].Type), - Status: routeConds[i].Status, - ObservedGeneration: observedGeneration, - LastTransitionTime: transitionTime, - Reason: string(routeConds[i].Reason), - Message: routeConds[i].Message, - } - } - - return conds -} diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go index 41850864c1..19dcdc2049 100644 --- a/internal/status/httproute_test.go +++ b/internal/status/httproute_test.go @@ -17,12 +17,12 @@ import ( func TestPrepareHTTPRouteStatus(t *testing.T) { g := NewGomegaWithT(t) - acceptedTrue := conditions.RouteCondition{ - Type: v1beta1.RouteConditionAccepted, + acceptedTrue := conditions.Condition{ + Type: string(v1beta1.RouteConditionAccepted), Status: metav1.ConditionTrue, } - acceptedFalse := conditions.RouteCondition{ - Type: v1beta1.RouteConditionAccepted, + acceptedFalse := conditions.Condition{ + Type: string(v1beta1.RouteConditionAccepted), Status: metav1.ConditionFalse, } @@ -30,10 +30,10 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { ObservedGeneration: 1, ParentStatuses: map[string]state.ParentStatus{ "accepted": { - Conditions: []conditions.RouteCondition{acceptedTrue}, + Conditions: []conditions.Condition{acceptedTrue}, }, "not-accepted": { - Conditions: []conditions.RouteCondition{acceptedFalse}, + Conditions: []conditions.Condition{acceptedFalse}, }, }, } @@ -84,48 +84,3 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, transitionTime) g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } - -func TestConvertRouteConditions(t *testing.T) { - g := NewGomegaWithT(t) - - routeConds := []conditions.RouteCondition{ - { - Type: "Test", - Status: metav1.ConditionTrue, - Reason: "reason1", - Message: "message1", - }, - { - Type: "Test", - Status: metav1.ConditionFalse, - Reason: "reason2", - Message: "message2", - }, - } - - var generation int64 = 1 - transitionTime := metav1.NewTime(time.Now()) - - expected := []metav1.Condition{ - { - Type: "Test", - Status: metav1.ConditionTrue, - ObservedGeneration: generation, - LastTransitionTime: transitionTime, - Reason: "reason1", - Message: "message1", - }, - { - Type: "Test", - Status: metav1.ConditionFalse, - ObservedGeneration: generation, - LastTransitionTime: transitionTime, - Reason: "reason2", - Message: "message2", - }, - } - - result := convertRouteConditions(routeConds, generation, transitionTime) - - g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) -} diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 7d8c811600..19e3ae4e75 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -97,7 +97,7 @@ var _ = Describe("Updater", func() { ObservedGeneration: 5, ParentStatuses: map[string]state.ParentStatus{ "http": { - Conditions: []conditions.RouteCondition{ + Conditions: []conditions.Condition{ { Type: "Test", Status: metav1.ConditionTrue, From 17fce8819868c87245135fdc02aa5e46d2c3c5d7 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 27 Jan 2023 16:39:13 -0600 Subject: [PATCH 2/8] Implement NKG-specific field validation for Gateway resource Fixes https://github.com/nginxinc/nginx-kubernetes-gateway/issues/406 --- internal/state/change_processor_test.go | 52 +++-- internal/state/conditions/conditions.go | 140 +++++++++++- internal/state/conditions/conditions_test.go | 42 ++-- internal/state/graph_test.go | 177 +++++++++++++-- internal/state/listener.go | 224 ++++++++++++++----- internal/state/listener_test.go | 211 +++++++++++------ internal/state/statuses.go | 21 +- internal/state/statuses_test.go | 48 ++-- internal/status/conditions_test.go | 43 ++-- internal/status/gateway.go | 26 +-- internal/status/gateway_test.go | 50 +---- internal/status/httproute_test.go | 44 +--- internal/status/updater_test.go | 35 +-- 13 files changed, 744 insertions(+), 369 deletions(-) diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index 8052448f77..6dff58a11b 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -286,12 +286,18 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: false, AttachedRoutes: 1, + Conditions: append( + conditions.NewDefaultListenerConditions(), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), + ), }, "listener-443-1": { - Valid: false, AttachedRoutes: 1, + Conditions: append( + conditions.NewDefaultListenerConditions(), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), + ), }, }, }, @@ -391,12 +397,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -500,12 +506,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -610,12 +616,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1Updated.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -719,12 +725,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1Updated.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -827,12 +833,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1Updated.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -928,12 +934,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1Updated.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -1049,12 +1055,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw2.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -1113,12 +1119,12 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw2.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 0, + Conditions: conditions.NewDefaultListenerConditions(), }, "listener-443-1": { - Valid: true, AttachedRoutes: 0, + Conditions: conditions.NewDefaultListenerConditions(), }, }, }, @@ -1146,12 +1152,18 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw2.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - Valid: false, AttachedRoutes: 0, + Conditions: append( + conditions.NewDefaultListenerConditions(), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), + ), }, "listener-443-1": { - Valid: false, AttachedRoutes: 0, + Conditions: append( + conditions.NewDefaultListenerConditions(), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), + ), }, }, }, diff --git a/internal/state/conditions/conditions.go b/internal/state/conditions/conditions.go index 8a4c5c4943..205e061fcd 100644 --- a/internal/state/conditions/conditions.go +++ b/internal/state/conditions/conditions.go @@ -7,8 +7,13 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" ) -// RouteReasonInvalidListener is used with the "Accepted" condition when the route references an invalid listener. -const RouteReasonInvalidListener v1beta1.RouteConditionReason = "InvalidListener" +const ( + // RouteReasonInvalidListener is used with the "Accepted" condition when the route references an invalid listener. + RouteReasonInvalidListener v1beta1.RouteConditionReason = "InvalidListener" + // ListenerReasonUnsupportedValue is used with the "Accepted" condition when a value of a field in a Listener + // is invalid or not supported. + ListenerReasonUnsupportedValue v1beta1.ListenerConditionReason = "UnsupportedValue" +) // Condition defines a condition to be reported in the status of resources. type Condition struct { @@ -19,18 +24,32 @@ type Condition struct { } // DeduplicateConditions removes duplicate conditions based on the condition type. -// The last condition wins. +// The last condition wins. The order of conditions is preserved. func DeduplicateConditions(conds []Condition) []Condition { - uniqueConds := make(map[string]Condition) + type elem struct { + cond Condition + reverseIdx int + } + + uniqueElems := make(map[string]elem) - for _, cond := range conds { - uniqueConds[cond.Type] = cond + idx := 0 + for i := len(conds) - 1; i >= 0; i-- { + if _, exist := uniqueElems[conds[i].Type]; exist { + continue + } + + uniqueElems[conds[i].Type] = elem{ + cond: conds[i], + reverseIdx: idx, + } + idx++ } - result := make([]Condition, 0, len(uniqueConds)) + result := make([]Condition, len(uniqueElems)) - for _, cond := range uniqueConds { - result = append(result, cond) + for _, el := range uniqueElems { + result[len(result)-el.reverseIdx-1] = el.cond } return result @@ -81,6 +100,107 @@ func NewRouteInvalidListener() Condition { Type: string(v1beta1.RouteConditionAccepted), Status: metav1.ConditionFalse, Reason: string(RouteReasonInvalidListener), - Message: "The listener is invalid for this parent ref", + Message: "Listener is invalid for this parent ref", + } +} + +// NewListenerPortUnavailable returns a Condition that indicates a port is unavailable in a Listener. +func NewListenerPortUnavailable(msg string) Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonPortUnavailable), + Message: msg, + } +} + +// NewDefaultListenerConditions returns the default Conditions that must be present in the status of a Listener. +func NewDefaultListenerConditions() []Condition { + return []Condition{ + { + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.ListenerReasonAccepted), + Message: "Listener is accepted", + }, + { + Type: string(v1beta1.ListenerReasonResolvedRefs), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.ListenerReasonResolvedRefs), + Message: "All references are resolved", + }, + { + Type: string(v1beta1.ListenerConditionConflicted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonNoConflicts), + Message: "No conflicts", + }, + } +} + +// NewListenerUnsupportedValue returns a Condition that indicates that a field of a Listener has an unsupported value. +// Unsupported means that the value is not supported by the implementation or invalid. +func NewListenerUnsupportedValue(msg string) Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(ListenerReasonUnsupportedValue), + Message: msg, + } +} + +// NewListenerInvalidCertificateRef returns Conditions that indicate that a CertificateRef of a Listener is invalid. +func NewListenerInvalidCertificateRef(msg string) []Condition { + return []Condition{ + { + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonInvalidCertificateRef), + Message: msg, + }, + { + Type: string(v1beta1.ListenerReasonResolvedRefs), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonInvalidCertificateRef), + Message: msg, + }, + } +} + +// NewListenerConflictedHostname returns Conditions that indicate that a hostname of a Listener is conflicted. +func NewListenerConflictedHostname(msg string) []Condition { + return []Condition{ + { + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonHostnameConflict), + Message: msg, + }, + { + Type: string(v1beta1.ListenerConditionConflicted), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.ListenerReasonHostnameConflict), + Message: msg, + }, + } +} + +// NewListenerUnsupportedAddress returns a Condition that indicates that the address of a Listener is unsupported. +func NewListenerUnsupportedAddress(msg string) Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonUnsupportedAddress), + Message: msg, + } +} + +// NewListenerUnsupportedProtocol returns a Condition that indicates that the protocol of a Listener is unsupported. +func NewListenerUnsupportedProtocol(msg string) Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonUnsupportedProtocol), + Message: msg, } } diff --git a/internal/state/conditions/conditions_test.go b/internal/state/conditions/conditions_test.go index 7d3b6aefc0..06bb47e782 100644 --- a/internal/state/conditions/conditions_test.go +++ b/internal/state/conditions/conditions_test.go @@ -12,42 +12,50 @@ func TestDeduplicateDeduplicateRouteConditions(t *testing.T) { conds := []Condition{ { - Type: "Type1", - Status: metav1.ConditionTrue, + Type: "Type1", + Status: metav1.ConditionTrue, + Message: "0", }, { - Type: "Type1", - Status: metav1.ConditionFalse, + Type: "Type1", + Status: metav1.ConditionFalse, + Message: "1", }, { - Type: "Type2", - Status: metav1.ConditionFalse, + Type: "Type2", + Status: metav1.ConditionFalse, + Message: "2", }, { - Type: "Type2", - Status: metav1.ConditionTrue, + Type: "Type2", + Status: metav1.ConditionTrue, + Message: "3", }, { - Type: "Type3", - Status: metav1.ConditionTrue, + Type: "Type3", + Status: metav1.ConditionTrue, + Message: "4", }, } expected := []Condition{ { - Type: "Type1", - Status: metav1.ConditionFalse, + Type: "Type1", + Status: metav1.ConditionFalse, + Message: "1", }, { - Type: "Type2", - Status: metav1.ConditionTrue, + Type: "Type2", + Status: metav1.ConditionTrue, + Message: "3", }, { - Type: "Type3", - Status: metav1.ConditionTrue, + Type: "Type3", + Status: metav1.ConditionTrue, + Message: "4", }, } result := DeduplicateConditions(conds) - g.Expect(result).Should(ConsistOf(expected)) + g.Expect(result).Should(Equal(expected)) } diff --git a/internal/state/graph_test.go b/internal/state/graph_test.go index ae9c81a324..8103cdfaf7 100644 --- a/internal/state/graph_test.go +++ b/internal/state/graph_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -451,6 +452,17 @@ func TestBuildListeners(t *testing.T) { Port: 80, Protocol: v1beta1.HTTPProtocolType, } + listener805 := v1beta1.Listener{ + Name: "listener-80-5", + Port: 81, // invalid port + Protocol: v1beta1.HTTPProtocolType, + } + listener806 := v1beta1.Listener{ + Name: "listener-80-6", + Hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("$example.com")), // invalid hostname + Port: 80, + Protocol: v1beta1.HTTPProtocolType, + } gatewayTLSConfig := &v1beta1.GatewayTLSConfig{ Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), @@ -497,9 +509,9 @@ func TestBuildListeners(t *testing.T) { } listener4434 := v1beta1.Listener{ Name: "listener-443-4", - Hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), + Hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("$example.com")), // invalid hostname Port: 443, - TLS: nil, // invalid https listener; missing tls config + TLS: gatewayTLSConfig, Protocol: v1beta1.HTTPSProtocolType, } listener4435 := v1beta1.Listener{ @@ -509,10 +521,28 @@ func TestBuildListeners(t *testing.T) { TLS: tlsConfigInvalidSecret, // invalid https listener; secret does not exist Protocol: v1beta1.HTTPSProtocolType, } + listener4436 := v1beta1.Listener{ + Name: "listener-443-6", + Hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), + Port: 444, // invalid port + TLS: gatewayTLSConfig, + Protocol: v1beta1.HTTPSProtocolType, + } + + const ( + invalidHostnameMsg = "Invalid hostname: a lowercase RFC 1123 subdomain " + + "must consist of lower case alphanumeric characters, '-' or '.', and must start and end " + + "with an alphanumeric character (e.g. 'example.com', regex used for validation is " + + `'[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')` + + conflictedHostnamesMsg = `Multiple listeners for the same port use the same hostname "foo.example.com"; ` + + "ensure only one listener uses that hostname" + ) + tests := []struct { gateway *v1beta1.Gateway expected map[string]*listener - msg string + name string }{ { gateway: &v1beta1.Gateway{ @@ -535,7 +565,7 @@ func TestBuildListeners(t *testing.T) { AcceptedHostnames: map[string]struct{}{}, }, }, - msg: "valid http listener", + name: "valid http listener", }, { gateway: &v1beta1.Gateway{ @@ -558,7 +588,7 @@ func TestBuildListeners(t *testing.T) { SecretPath: secretPath, }, }, - msg: "valid https listener", + name: "valid https listener", }, { gateway: &v1beta1.Gateway{ @@ -578,9 +608,13 @@ func TestBuildListeners(t *testing.T) { Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, + Conditions: []conditions.Condition{ + conditions.NewListenerUnsupportedProtocol(`Protocol "TCP" is not supported, use "HTTP" ` + + `or "HTTPS"`), + }, }, }, - msg: "invalid listener protocol", + name: "invalid listener protocol", }, { gateway: &v1beta1.Gateway{ @@ -590,19 +624,82 @@ func TestBuildListeners(t *testing.T) { Spec: v1beta1.GatewaySpec{ GatewayClassName: gcName, Listeners: []v1beta1.Listener{ + listener805, + }, + }, + }, + expected: map[string]*listener{ + "listener-80-5": { + Source: listener805, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + Conditions: []conditions.Condition{ + conditions.NewListenerPortUnavailable("Port 81 is not supported for HTTP, use 80"), + }, + }, + }, + name: "invalid http listener", + }, + { + gateway: &v1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + }, + Spec: v1beta1.GatewaySpec{ + GatewayClassName: gcName, + Listeners: []v1beta1.Listener{ + listener4436, + }, + }, + }, + expected: map[string]*listener{ + "listener-443-6": { + Source: listener4436, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + Conditions: []conditions.Condition{ + conditions.NewListenerPortUnavailable("Port 444 is not supported for HTTPS, use 443"), + }, + }, + }, + name: "invalid https listener", + }, + { + gateway: &v1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + }, + Spec: v1beta1.GatewaySpec{ + GatewayClassName: gcName, + Listeners: []v1beta1.Listener{ + listener806, listener4434, }, }, }, expected: map[string]*listener{ + "listener-80-6": { + Source: listener806, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + Conditions: []conditions.Condition{ + conditions.NewListenerUnsupportedValue(invalidHostnameMsg), + }, + }, "listener-443-4": { Source: listener4434, Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, + Conditions: []conditions.Condition{ + conditions.NewListenerUnsupportedValue(invalidHostnameMsg), + }, }, }, - msg: "invalid https listener (tls config missing)", + name: "invalid hostnames", }, { gateway: &v1beta1.Gateway{ @@ -622,9 +719,11 @@ func TestBuildListeners(t *testing.T) { Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, + Conditions: conditions.NewListenerInvalidCertificateRef("Failed to get the certificate " + + "test/does-not-exist: secret test/does-not-exist does not exist"), }, }, - msg: "invalid https listener (secret does not exist)", + name: "invalid https listener (secret does not exist)", }, { gateway: &v1beta1.Gateway{ @@ -667,7 +766,7 @@ func TestBuildListeners(t *testing.T) { SecretPath: secretPath, }, }, - msg: "multiple valid http/https listeners", + name: "multiple valid http/https listeners", }, { gateway: &v1beta1.Gateway{ @@ -688,34 +787,75 @@ func TestBuildListeners(t *testing.T) { Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, + Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg), }, "listener-80-4": { Source: listener804, Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, + Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg), }, "listener-443-1": { Source: listener4431, Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, - SecretPath: secretPath, + Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg), }, "listener-443-3": { Source: listener4433, Valid: false, Routes: map[types.NamespacedName]*route{}, AcceptedHostnames: map[string]struct{}{}, - SecretPath: secretPath, + Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg), + }, + }, + name: "collisions", + }, + { + gateway: &v1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + }, + Spec: v1beta1.GatewaySpec{ + GatewayClassName: gcName, + Listeners: []v1beta1.Listener{ + listener801, + listener4431, + }, + Addresses: []v1beta1.GatewayAddress{ + {}, + }, + }, + }, + expected: map[string]*listener{ + "listener-80-1": { + Source: listener801, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + Conditions: []conditions.Condition{ + conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"), + }, + }, + "listener-443-1": { + Source: listener4431, + Valid: false, + Routes: map[types.NamespacedName]*route{}, + AcceptedHostnames: map[string]struct{}{}, + SecretPath: "", + Conditions: []conditions.Condition{ + conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"), + }, }, }, - msg: "collisions", + name: "gateway addresses are not supported", }, { gateway: nil, expected: map[string]*listener{}, - msg: "no gateway", + name: "no gateway", }, { gateway: &v1beta1.Gateway{ @@ -730,7 +870,7 @@ func TestBuildListeners(t *testing.T) { }, }, expected: map[string]*listener{}, - msg: "wrong gatewayclass", + name: "wrong gatewayclass", }, } @@ -741,11 +881,12 @@ func TestBuildListeners(t *testing.T) { secretMemoryMgr := NewSecretDiskMemoryManager(secretsDirectory, secretStore) for _, test := range tests { - result := buildListeners(test.gateway, gcName, secretMemoryMgr) + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) - if diff := cmp.Diff(test.expected, result); diff != "" { - t.Errorf("buildListeners() %q mismatch (-want +got):\n%s", test.msg, diff) - } + result := buildListeners(test.gateway, gcName, secretMemoryMgr) + g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) + }) } } diff --git a/internal/state/listener.go b/internal/state/listener.go index ca2c906934..a83beffdfd 100644 --- a/internal/state/listener.go +++ b/internal/state/listener.go @@ -1,8 +1,14 @@ package state import ( + "fmt" + "strings" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) // listener represents a listener of the Gateway resource. @@ -17,7 +23,10 @@ type listener struct { AcceptedHostnames map[string]struct{} // SecretPath is the path to the secret on disk. SecretPath string + // Conditions holds the conditions of the listener. + Conditions []conditions.Condition // Valid shows whether the listener is valid. + // A listener is considered valid if NKG can generate valid NGINX configuration for it. Valid bool } @@ -47,7 +56,7 @@ func newListenerConfiguratorFactory( ) *listenerConfiguratorFactory { return &listenerConfiguratorFactory{ https: newHTTPSListenerConfigurator(gw, secretMemoryMgr), - http: newHTTPListenerConfigurator(), + http: newHTTPListenerConfigurator(gw), } } @@ -69,61 +78,107 @@ func newHTTPSListenerConfigurator( } func (c *httpsListenerConfigurator) configure(gl v1beta1.Listener) *listener { - var path string - var err error + validate := func(gl v1beta1.Listener) []conditions.Condition { + return validateHTTPSListener(gl, c.gateway.Namespace) + } - valid := validateHTTPSListener(gl, c.gateway.Namespace) + l := configureListenerAndUseHostname(gl, c.gateway, c.usedHostnames, validate) - if valid { - nsname := types.NamespacedName{ - Namespace: c.gateway.Namespace, - Name: string(gl.TLS.CertificateRefs[0].Name), - } + if !l.Valid { + return l + } - path, err = c.secretMemoryMgr.Request(nsname) - if err != nil { - valid = false - } + nsname := types.NamespacedName{ + Namespace: c.gateway.Namespace, + Name: string(gl.TLS.CertificateRefs[0].Name), } - h := getHostname(gl.Hostname) + path, err := c.secretMemoryMgr.Request(nsname) + if err != nil { + l.Valid = false - if holder, exist := c.usedHostnames[h]; exist { - valid = false - holder.Valid = false // all listeners for the same hostname become conflicted - } + msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err) + l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...) - l := &listener{ - Source: gl, - Valid: valid, - SecretPath: path, - Routes: make(map[types.NamespacedName]*route), - AcceptedHostnames: make(map[string]struct{}), + return l } - c.usedHostnames[h] = l + l.SecretPath = path return l } type httpListenerConfigurator struct { usedHostnames map[string]*listener + gateway *v1beta1.Gateway } -func newHTTPListenerConfigurator() *httpListenerConfigurator { +func newHTTPListenerConfigurator(gw *v1beta1.Gateway) *httpListenerConfigurator { return &httpListenerConfigurator{ usedHostnames: make(map[string]*listener), + gateway: gw, } } func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { - valid := validateHTTPListener(gl) + return configureListenerAndUseHostname(gl, c.gateway, c.usedHostnames, validateHTTPListener) +} - h := getHostname(gl.Hostname) +type invalidProtocolListenerConfigurator struct{} - if holder, exist := c.usedHostnames[h]; exist { +func newInvalidProtocolListenerConfigurator() *invalidProtocolListenerConfigurator { + return &invalidProtocolListenerConfigurator{} +} + +func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *listener { + msg := fmt.Sprintf("Protocol %q is not supported, use %q or %q", + gl.Protocol, v1beta1.HTTPProtocolType, v1beta1.HTTPSProtocolType) + + return &listener{ + Source: gl, + Valid: false, + Routes: make(map[types.NamespacedName]*route), + AcceptedHostnames: make(map[string]struct{}), + Conditions: []conditions.Condition{ + conditions.NewListenerUnsupportedProtocol(msg), + }, + } +} + +func configureListenerAndUseHostname( + gl v1beta1.Listener, + gw *v1beta1.Gateway, + usedHostnames map[string]*listener, + validate func(listener v1beta1.Listener) []conditions.Condition, +) *listener { + conds := validate(gl) + + if len(gw.Spec.Addresses) > 0 { + conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported")) + } + + valid := len(conds) == 0 + + // we validate hostnames separately from validate() because we don't want to check for conflicts for + // invalid hostnames + validHostname, hostnameCond := validateListenerHostname(gl.Hostname) + if validHostname { + h := getHostname(gl.Hostname) + + if holder, exist := usedHostnames[h]; exist { + valid = false + holder.Valid = false // all listeners for the same hostname become conflicted + holder.SecretPath = "" // ensure secret path is unset for invalid listeners + + format := "Multiple listeners for the same port use the same hostname %q; " + + "ensure only one listener uses that hostname" + conflictedConds := conditions.NewListenerConflictedHostname(fmt.Sprintf(format, h)) + holder.Conditions = append(holder.Conditions, conflictedConds...) + conds = append(conds, conflictedConds...) + } + } else { valid = false - holder.Valid = false // all listeners for the same hostname become conflicted + conds = append(conds, hostnameCond) } l := &listener{ @@ -131,52 +186,105 @@ func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { Valid: valid, Routes: make(map[types.NamespacedName]*route), AcceptedHostnames: make(map[string]struct{}), + Conditions: conds, } - c.usedHostnames[h] = l + if !validHostname { + return l + } + + h := getHostname(gl.Hostname) + usedHostnames[h] = l return l } -type invalidProtocolListenerConfigurator struct{} +func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { + var conds []conditions.Condition -func newInvalidProtocolListenerConfigurator() *invalidProtocolListenerConfigurator { - return &invalidProtocolListenerConfigurator{} + if listener.Port != 80 { + msg := fmt.Sprintf("Port %d is not supported for HTTP, use 80", listener.Port) + conds = append(conds, conditions.NewListenerPortUnavailable(msg)) + } + + // The imported Webhook validation ensures the tls field is not set for an HTTP listener. + // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + + return conds } -func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *listener { - return &listener{ - Source: gl, - Valid: false, - Routes: make(map[types.NamespacedName]*route), - AcceptedHostnames: make(map[string]struct{}), +func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditions.Condition { + var conds []conditions.Condition + + if listener.Port != 443 { + msg := fmt.Sprintf("Port %d is not supported for HTTPS, use 443", listener.Port) + conds = append(conds, conditions.NewListenerPortUnavailable(msg)) } -} -func validateHTTPListener(listener v1beta1.Listener) bool { - // FIXME(pleshakov): For now we require that all HTTP listeners bind to port 80 - return listener.Port == 80 -} + // The imported Webhook validation ensures the tls field is not nil for an HTTPS listener. + // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. -func validateHTTPSListener(listener v1beta1.Listener, gwNsname string) bool { - // FIXME(kate-osborn): - // 1. For now we require that all HTTPS listeners bind to port 443 - // 2. Only TLSModeTerminate is supported. - if listener.Port != 443 || listener.TLS == nil || *listener.TLS.Mode != v1beta1.TLSModeTerminate || - len(listener.TLS.CertificateRefs) == 0 { - return false + if *listener.TLS.Mode != v1beta1.TLSModeTerminate { + msg := fmt.Sprintf("tls.mode %q is not supported, use %q", *listener.TLS.Mode, v1beta1.TLSModeTerminate) + conds = append(conds, conditions.NewListenerUnsupportedValue(msg)) } + if len(listener.TLS.Options) > 0 { + conds = append(conds, conditions.NewListenerUnsupportedValue("tls.options are not supported")) + } + + // The imported Webhook validation ensures len(listener.TLS.Certificates) is not 0. + // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + certRef := listener.TLS.CertificateRefs[0] - // certRef Kind has default of "Secret" so it's safe to directly access the Kind here - if *certRef.Kind != "Secret" { - return false + + if certRef.Kind != nil && *certRef.Kind != "Secret" { + msg := fmt.Sprintf("Kind must be Secret, got %q", *certRef.Kind) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...) + } + + // for Kind Secret, certRef.Group must be nil or empty + if certRef.Group != nil && *certRef.Group != "" { + msg := fmt.Sprintf("Group must be empty, got %q", *certRef.Group) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...) } // secret must be in the same namespace as the gateway - if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsname { - return false + if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName { + const msg = "Referenced Secret must belong to the same namespace as the Gateway" + conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...) + + } + + if len(listener.TLS.CertificateRefs) > 1 { + msg := fmt.Sprintf("Only 1 certificateRef is supported, got %d", len(listener.TLS.CertificateRefs)) + conds = append(conds, conditions.NewListenerUnsupportedValue(msg)) + } + + return conds +} + +func validateListenerHostname(host *v1beta1.Hostname) (valid bool, cond conditions.Condition) { + if host == nil { + return true, conditions.Condition{} + } + + h := string(*host) + + if h == "" { + return true, conditions.Condition{} + } + + // FIXME(pleshakov): For now, we don't support wildcard hostnames + if strings.HasPrefix(h, "*") { + return false, conditions.NewListenerUnsupportedValue("Wildcard hostnames are not supported") + } + + msgs := validation.IsDNS1123Subdomain(h) + if len(msgs) > 0 { + combined := strings.Join(msgs, ",") + return false, conditions.NewListenerUnsupportedValue(fmt.Sprintf("Invalid hostname: %s", combined)) } - return true + return true, conditions.Condition{} } diff --git a/internal/state/listener_test.go b/internal/state/listener_test.go index 9f06fe6c61..6ad3b4ceaa 100644 --- a/internal/state/listener_test.go +++ b/internal/state/listener_test.go @@ -3,64 +3,70 @@ package state import ( "testing" + . "github.com/onsi/gomega" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) func TestValidateHTTPListener(t *testing.T) { tests := []struct { l v1beta1.Listener - msg string - expected bool + name string + expected []conditions.Condition }{ { l: v1beta1.Listener{ - Port: 80, - Protocol: v1beta1.HTTPProtocolType, + Port: 80, }, - expected: true, - msg: "valid", + expected: nil, + name: "valid", }, { l: v1beta1.Listener{ - Port: 81, - Protocol: v1beta1.HTTPProtocolType, + Port: 81, }, - expected: false, - msg: "invalid port", + expected: []conditions.Condition{ + conditions.NewListenerPortUnavailable("Port 81 is not supported for HTTP, use 80"), + }, + name: "invalid port", }, } for _, test := range tests { - result := validateHTTPListener(test.l) - if result != test.expected { - t.Errorf( - "validateListener() returned %v but expected %v for the case of %q", - result, - test.expected, - test.msg, - ) - } + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + result := validateHTTPListener(test.l) + g.Expect(result).To(Equal(test.expected)) + }) } } func TestValidateHTTPSListener(t *testing.T) { gwNs := "gateway-ns" - validSecretRef := &v1beta1.SecretObjectReference{ + validSecretRef := v1beta1.SecretObjectReference{ Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), Name: "secret", Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer(gwNs)), } - invalidSecretRefType := &v1beta1.SecretObjectReference{ + invalidSecretRefGroup := v1beta1.SecretObjectReference{ + Group: (*v1beta1.Group)(helpers.GetStringPointer("some-group")), + Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), + Name: "secret", + Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer(gwNs)), + } + + invalidSecretRefKind := v1beta1.SecretObjectReference{ Kind: (*v1beta1.Kind)(helpers.GetStringPointer("ConfigMap")), Name: "secret", Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer(gwNs)), } - invalidSecretRefTNamespace := &v1beta1.SecretObjectReference{ + invalidSecretRefTNamespace := v1beta1.SecretObjectReference{ Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), Name: "secret", Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("diff-ns")), @@ -68,99 +74,170 @@ func TestValidateHTTPSListener(t *testing.T) { tests := []struct { l v1beta1.Listener - msg string - expected bool + name string + expected []conditions.Condition }{ { l: v1beta1.Listener{ - Port: 443, - Protocol: v1beta1.HTTPSProtocolType, + Port: 443, TLS: &v1beta1.GatewayTLSConfig{ Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{*validSecretRef}, + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, }, }, - expected: true, - msg: "valid", + expected: nil, + name: "valid", }, { l: v1beta1.Listener{ - Port: 80, - Protocol: v1beta1.HTTPSProtocolType, + Port: 80, TLS: &v1beta1.GatewayTLSConfig{ Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{*validSecretRef}, + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, }, }, - expected: false, - msg: "invalid port", + expected: []conditions.Condition{ + conditions.NewListenerPortUnavailable("Port 80 is not supported for HTTPS, use 443"), + }, + name: "invalid port", }, { l: v1beta1.Listener{ - Port: 443, - Protocol: v1beta1.HTTPSProtocolType, + Port: 443, TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, + Options: map[v1beta1.AnnotationKey]v1beta1.AnnotationValue{"key": "val"}, }, }, - expected: false, - msg: "invalid - no cert ref", + expected: []conditions.Condition{ + conditions.NewListenerUnsupportedValue("tls.options are not supported"), + }, + name: "invalid options", }, { l: v1beta1.Listener{ - Port: 443, - Protocol: v1beta1.HTTPSProtocolType, + Port: 443, TLS: &v1beta1.GatewayTLSConfig{ Mode: helpers.GetTLSModePointer(v1beta1.TLSModePassthrough), - CertificateRefs: []v1beta1.SecretObjectReference{*validSecretRef}, + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, + }, + }, + expected: []conditions.Condition{ + conditions.NewListenerUnsupportedValue(`tls.mode "Passthrough" is not supported, use "Terminate"`), + }, + name: "invalid tls mode", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefGroup}, }, }, - expected: false, - msg: "invalid tls mode", + expected: conditions.NewListenerInvalidCertificateRef(`Group must be empty, got "some-group"`), + name: "invalid cert ref group", }, { l: v1beta1.Listener{ - Port: 443, - Protocol: v1beta1.HTTPSProtocolType, + Port: 443, TLS: &v1beta1.GatewayTLSConfig{ Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{*invalidSecretRefType}, + CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefKind}, }, }, - expected: false, - msg: "invalid cert ref kind", + expected: conditions.NewListenerInvalidCertificateRef(`Kind must be Secret, got "ConfigMap"`), + name: "invalid cert ref kind", }, { l: v1beta1.Listener{ - Port: 443, - Protocol: v1beta1.HTTPSProtocolType, + Port: 443, TLS: &v1beta1.GatewayTLSConfig{ Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{*invalidSecretRefTNamespace}, + CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefTNamespace}, }, }, - expected: false, - msg: "invalid cert ref namespace", + expected: conditions.NewListenerInvalidCertificateRef("Referenced Secret must belong to the same " + + "namespace as the Gateway"), + name: "invalid cert ref namespace", }, { l: v1beta1.Listener{ - Port: 443, - Protocol: v1beta1.HTTPSProtocolType, + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef, validSecretRef}, + }, + }, + expected: []conditions.Condition{ + conditions.NewListenerUnsupportedValue("Only 1 certificateRef is supported, got 2"), }, - expected: false, - msg: "invalid - no tls config", + name: "too many cert refs", }, } for _, test := range tests { - result := validateHTTPSListener(test.l, gwNs) - if result != test.expected { - t.Errorf( - "validateHTTPSListener() returned %v but expected %v for the case of %q", - result, - test.expected, - test.msg, - ) - } + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + result := validateHTTPSListener(test.l, gwNs) + g.Expect(result).To(Equal(test.expected)) + }) + } +} + +func TestValidateListenerHostname(t *testing.T) { + tests := []struct { + hostname *v1beta1.Hostname + expectedCondition conditions.Condition + name string + expectedValid bool + }{ + { + hostname: nil, + expectedValid: true, + expectedCondition: conditions.Condition{}, + name: "nil hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("")), + expectedValid: true, + expectedCondition: conditions.Condition{}, + name: "empty hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), + expectedValid: true, + expectedCondition: conditions.Condition{}, + name: "valid hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")), + expectedValid: false, + expectedCondition: conditions.NewListenerUnsupportedValue("Wildcard hostnames are not supported"), + name: "wildcard hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")), + expectedValid: false, + expectedCondition: conditions.NewListenerUnsupportedValue( + "Invalid hostname: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric " + + "characters, '-' or '.', and must start and end with an alphanumeric character " + + "(e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?" + + `(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`), + name: "invalid hostname", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + valid, cond := validateListenerHostname(test.hostname) + + g.Expect(valid).To(Equal(test.expectedValid)) + g.Expect(cond).To(Equal(test.expectedCondition)) + }) } } diff --git a/internal/state/statuses.go b/internal/state/statuses.go index f6cdf6d608..71e9549d53 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -41,8 +41,8 @@ type IgnoredGatewayStatus struct { // ListenerStatus holds the status-related information about a listener in the Gateway resource. type ListenerStatus struct { - // Valid shows if the listener is valid. - Valid bool + // Conditions is the list of conditions for this listener. + Conditions []conditions.Condition // AttachedRoutes is the number of routes attached to the listener. AttachedRoutes int32 } @@ -94,10 +94,25 @@ func buildStatuses(graph *graph) Statuses { if graph.Gateway != nil { listenerStatuses := make(map[string]ListenerStatus) + defaultConds := conditions.NewDefaultListenerConditions() + for name, l := range graph.Gateway.Listeners { + conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)+1) // 1 is for missing GC + + conds = append(conds, defaultConds...) + conds = append(conds, l.Conditions...) + + if !gcValidAndExist { + // FIXME(pleshakov): Figure out appropriate conditions for the cases when: + // (1) GatewayClass is invalid. + // (2) GatewayClass does not exist. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/307 + conds = append(conds, conditions.NewTODO("GatewayClass is invalid or doesn't exist")) + } + listenerStatuses[name] = ListenerStatus{ - Valid: l.Valid && gcValidAndExist, AttachedRoutes: int32(len(l.Routes)), + Conditions: conditions.DeduplicateConditions(conds), } } diff --git a/internal/state/statuses_test.go b/internal/state/statuses_test.go index 52f6502793..0a32beff61 100644 --- a/internal/state/statuses_test.go +++ b/internal/state/statuses_test.go @@ -1,14 +1,14 @@ package state import ( - "sort" "testing" - "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1beta1" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) @@ -76,7 +76,7 @@ func TestBuildStatuses(t *testing.T) { tests := []struct { graph *graph expected Statuses - msg string + name string }{ { graph: &graph{ @@ -104,8 +104,8 @@ func TestBuildStatuses(t *testing.T) { NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, ListenerStatuses: map[string]ListenerStatus{ "listener-80-1": { - Valid: true, AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, }, ObservedGeneration: 2, @@ -130,7 +130,7 @@ func TestBuildStatuses(t *testing.T) { }, }, }, - msg: "normal case", + name: "normal case", }, { graph: &graph{ @@ -150,8 +150,11 @@ func TestBuildStatuses(t *testing.T) { NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, ListenerStatuses: map[string]ListenerStatus{ "listener-80-1": { - Valid: false, AttachedRoutes: 1, + Conditions: append( + conditions.NewDefaultListenerConditions(), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), + ), }, }, ObservedGeneration: 2, @@ -180,7 +183,7 @@ func TestBuildStatuses(t *testing.T) { }, }, }, - msg: "gatewayclass doesn't exist", + name: "gatewayclass doesn't exist", }, { graph: &graph{ @@ -210,8 +213,11 @@ func TestBuildStatuses(t *testing.T) { NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, ListenerStatuses: map[string]ListenerStatus{ "listener-80-1": { - Valid: false, AttachedRoutes: 1, + Conditions: append( + conditions.NewDefaultListenerConditions(), + conditions.NewTODO("GatewayClass is invalid or doesn't exist"), + ), }, }, ObservedGeneration: 2, @@ -240,7 +246,7 @@ func TestBuildStatuses(t *testing.T) { }, }, }, - msg: "gatewayclass is not valid", + name: "gatewayclass is not valid", }, { graph: &graph{ @@ -281,28 +287,16 @@ func TestBuildStatuses(t *testing.T) { }, }, }, - msg: "gateway and ignored gateways don't exist", + name: "gateway and ignored gateways don't exist", }, } - sortConditions := func(statuses Statuses) { - for _, rs := range statuses.HTTPRouteStatuses { - for _, ps := range rs.ParentStatuses { - sort.Slice(ps.Conditions, func(i, j int) bool { - return ps.Conditions[i].Type < ps.Conditions[j].Type - }) - } - } - } - for _, test := range tests { - result := buildStatuses(test.graph) - - sortConditions(result) - sortConditions(test.expected) + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) - if diff := cmp.Diff(test.expected, result); diff != "" { - t.Errorf("buildStatuses() '%v' mismatch (-want +got):\n%s", test.msg, diff) - } + result := buildStatuses(test.graph) + g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) + }) } } diff --git a/internal/status/conditions_test.go b/internal/status/conditions_test.go index 4e575483e2..c38216b33c 100644 --- a/internal/status/conditions_test.go +++ b/internal/status/conditions_test.go @@ -11,47 +11,52 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) -func TestConvertRouteConditions(t *testing.T) { - g := NewGomegaWithT(t) - - routeConds := []conditions.Condition{ +func CreateTestConditions() []conditions.Condition { + return []conditions.Condition{ { Type: "Test", Status: metav1.ConditionTrue, - Reason: "reason1", - Message: "message1", + Reason: "TestReason1", + Message: "Test message1", }, { Type: "Test", Status: metav1.ConditionFalse, - Reason: "reason2", - Message: "message2", + Reason: "TestReason2", + Message: "Test message2", }, } +} - var generation int64 = 1 - transitionTime := metav1.NewTime(time.Now()) - - expected := []metav1.Condition{ +func CreateExpectedAPIConditions(observedGeneration int64, transitionTime metav1.Time) []metav1.Condition { + return []metav1.Condition{ { Type: "Test", Status: metav1.ConditionTrue, - ObservedGeneration: generation, + ObservedGeneration: observedGeneration, LastTransitionTime: transitionTime, - Reason: "reason1", - Message: "message1", + Reason: "TestReason1", + Message: "Test message1", }, { Type: "Test", Status: metav1.ConditionFalse, - ObservedGeneration: generation, + ObservedGeneration: observedGeneration, LastTransitionTime: transitionTime, - Reason: "reason2", - Message: "message2", + Reason: "TestReason2", + Message: "Test message2", }, } +} + +func TestConvertRouteConditions(t *testing.T) { + g := NewGomegaWithT(t) + + var generation int64 = 1 + transitionTime := metav1.NewTime(time.Now()) - result := convertConditions(routeConds, generation, transitionTime) + expected := CreateExpectedAPIConditions(generation, transitionTime) + result := convertConditions(CreateTestConditions(), generation, transitionTime) g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } diff --git a/internal/status/gateway.go b/internal/status/gateway.go index 2b289569b8..06e12e268d 100644 --- a/internal/status/gateway.go +++ b/internal/status/gateway.go @@ -36,28 +36,6 @@ func prepareGatewayStatus(gatewayStatus state.GatewayStatus, transitionTime meta for _, name := range names { s := gatewayStatus.ListenerStatuses[name] - var ( - status metav1.ConditionStatus - reason v1beta1.ListenerConditionReason - ) - - if s.Valid { - status = metav1.ConditionTrue - reason = v1beta1.ListenerReasonReady - } else { - status = metav1.ConditionFalse - reason = v1beta1.ListenerReasonInvalid - } - - cond := metav1.Condition{ - Type: string(v1beta1.ListenerConditionReady), - Status: status, - ObservedGeneration: gatewayStatus.ObservedGeneration, - LastTransitionTime: transitionTime, - Reason: string(reason), - Message: "", // FIXME(pleshakov) Come up with a good message - } - listenerStatuses = append(listenerStatuses, v1beta1.ListenerStatus{ Name: v1beta1.SectionName(name), SupportedKinds: []v1beta1.RouteGroupKind{ @@ -66,7 +44,7 @@ func prepareGatewayStatus(gatewayStatus state.GatewayStatus, transitionTime meta }, }, AttachedRoutes: s.AttachedRoutes, - Conditions: []metav1.Condition{cond}, + Conditions: convertConditions(s.Conditions, gatewayStatus.ObservedGeneration, transitionTime), }) } @@ -78,6 +56,8 @@ func prepareGatewayStatus(gatewayStatus state.GatewayStatus, transitionTime meta // prepareIgnoredGatewayStatus prepares the status for an ignored Gateway resource. // TODO: is it reasonable to not set the listener statuses? +// FIXME(pleshakov): Simplify the code, so that we don't need a separate prepareIgnoredGatewayStatus +// and state.IgnoredGatewayStatus. func prepareIgnoredGatewayStatus(status state.IgnoredGatewayStatus, transitionTime metav1.Time) v1beta1.GatewayStatus { return v1beta1.GatewayStatus{ Conditions: []metav1.Condition{ diff --git a/internal/status/gateway_test.go b/internal/status/gateway_test.go index aabccebf76..79303c89ff 100644 --- a/internal/status/gateway_test.go +++ b/internal/status/gateway_test.go @@ -5,22 +5,20 @@ import ( "time" "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/gateway-api/apis/v1beta1" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" ) func TestPrepareGatewayStatus(t *testing.T) { status := state.GatewayStatus{ ListenerStatuses: state.ListenerStatuses{ - "valid-listener": { - Valid: true, - AttachedRoutes: 2, - }, - "invalid-listener": { - Valid: false, - AttachedRoutes: 1, + "listener": { + AttachedRoutes: 3, + Conditions: CreateTestConditions(), }, }, ObservedGeneration: 1, @@ -31,48 +29,22 @@ func TestPrepareGatewayStatus(t *testing.T) { expected := v1beta1.GatewayStatus{ Listeners: []v1beta1.ListenerStatus{ { - Name: "invalid-listener", + Name: "listener", SupportedKinds: []v1beta1.RouteGroupKind{ { Kind: "HTTPRoute", }, }, - AttachedRoutes: 1, - Conditions: []metav1.Condition{ - { - Type: string(v1beta1.ListenerConditionReady), - Status: metav1.ConditionFalse, - ObservedGeneration: 1, - LastTransitionTime: transitionTime, - Reason: string(v1beta1.ListenerReasonInvalid), - }, - }, - }, - { - Name: "valid-listener", - SupportedKinds: []v1beta1.RouteGroupKind{ - { - Kind: "HTTPRoute", - }, - }, - AttachedRoutes: 2, - Conditions: []metav1.Condition{ - { - Type: string(v1beta1.ListenerConditionReady), - Status: metav1.ConditionTrue, - ObservedGeneration: 1, - LastTransitionTime: transitionTime, - Reason: string(v1beta1.ListenerReasonReady), - }, - }, + AttachedRoutes: 3, + Conditions: CreateExpectedAPIConditions(1, transitionTime), }, }, } + g := NewGomegaWithT(t) + result := prepareGatewayStatus(status, transitionTime) - if diff := cmp.Diff(expected, result); diff != "" { - t.Errorf("prepareGatewayStatus() mismatch (-want +got):\n%s", diff) - } + g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } func TestPrepareIgnoredGatewayStatus(t *testing.T) { diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go index 19dcdc2049..1bbbcac9f8 100644 --- a/internal/status/httproute_test.go +++ b/internal/status/httproute_test.go @@ -11,29 +11,16 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) func TestPrepareHTTPRouteStatus(t *testing.T) { g := NewGomegaWithT(t) - acceptedTrue := conditions.Condition{ - Type: string(v1beta1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - } - acceptedFalse := conditions.Condition{ - Type: string(v1beta1.RouteConditionAccepted), - Status: metav1.ConditionFalse, - } - status := state.HTTPRouteStatus{ ObservedGeneration: 1, ParentStatuses: map[string]state.ParentStatus{ - "accepted": { - Conditions: []conditions.Condition{acceptedTrue}, - }, - "not-accepted": { - Conditions: []conditions.Condition{acceptedFalse}, + "parent": { + Conditions: CreateTestConditions(), }, }, } @@ -50,33 +37,10 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { ParentRef: v1beta1.ParentReference{ Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), Name: "gateway", - SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("accepted")), + SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("parent")), }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), - Conditions: []metav1.Condition{ - { - Type: string(v1beta1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - ObservedGeneration: 1, - LastTransitionTime: transitionTime, - }, - }, - }, - { - ParentRef: v1beta1.ParentReference{ - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), - Name: "gateway", - SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("not-accepted")), - }, - ControllerName: v1beta1.GatewayController(gatewayCtlrName), - Conditions: []metav1.Condition{ - { - Type: string(v1beta1.RouteConditionAccepted), - Status: metav1.ConditionFalse, - ObservedGeneration: 1, - LastTransitionTime: transitionTime, - }, - }, + Conditions: CreateExpectedAPIConditions(1, transitionTime), }, }, }, diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 19e3ae4e75..94ab50bc47 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -17,7 +17,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status" "github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes" ) @@ -81,8 +80,8 @@ var _ = Describe("Updater", func() { NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, ListenerStatuses: map[string]state.ListenerStatus{ "http": { - Valid: valid, AttachedRoutes: 1, + Conditions: status.CreateTestConditions(), }, }, ObservedGeneration: generation, @@ -97,12 +96,7 @@ var _ = Describe("Updater", func() { ObservedGeneration: 5, ParentStatuses: map[string]state.ParentStatus{ "http": { - Conditions: []conditions.Condition{ - { - Type: "Test", - Status: metav1.ConditionTrue, - }, - }, + Conditions: status.CreateTestConditions(), }, }, }, @@ -139,7 +133,7 @@ var _ = Describe("Updater", func() { } } - createExpectedGw = func(status metav1.ConditionStatus, generation int64, reason string) *v1beta1.Gateway { + createExpectedGw = func(generation int64) *v1beta1.Gateway { return &v1beta1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -159,15 +153,7 @@ var _ = Describe("Updater", func() { }, }, AttachedRoutes: 1, - Conditions: []metav1.Condition{ - { - Type: string(v1beta1.ListenerConditionReady), - Status: status, - ObservedGeneration: generation, - LastTransitionTime: fakeClockTime, - Reason: reason, - }, - }, + Conditions: status.CreateExpectedAPIConditions(generation, fakeClockTime), }, }, }, @@ -219,14 +205,7 @@ var _ = Describe("Updater", func() { Name: "gateway", SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("http")), }, - Conditions: []metav1.Condition{ - { - Type: "Test", - Status: metav1.ConditionTrue, - ObservedGeneration: 5, - LastTransitionTime: fakeClockTime, - }, - }, + Conditions: status.CreateExpectedAPIConditions(5, fakeClockTime), }, }, }, @@ -307,7 +286,7 @@ var _ = Describe("Updater", func() { It("should have the updated status of Gateway in the API server", func() { latestGw := &v1beta1.Gateway{} - expectedGw := createExpectedGw(metav1.ConditionTrue, 1, string(v1beta1.ListenerReasonReady)) + expectedGw := createExpectedGw(1) err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) Expect(err).Should(Not(HaveOccurred())) @@ -371,7 +350,7 @@ var _ = Describe("Updater", func() { It("should have the updated status of Gateway in the API server", func() { latestGw := &v1beta1.Gateway{} - expectedGw := createExpectedGw(metav1.ConditionFalse, 2, string(v1beta1.ListenerReasonInvalid)) + expectedGw := createExpectedGw(2) err := client.Get( context.Background(), From 136ad2e98ce0ddf066db41c0db5850aaf57165cc Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 7 Feb 2023 17:10:29 -0600 Subject: [PATCH 3/8] Refactor configurators --- internal/state/listener.go | 162 ++++++++++++++------------------ internal/state/listener_test.go | 55 +++++------ 2 files changed, 94 insertions(+), 123 deletions(-) diff --git a/internal/state/listener.go b/internal/state/listener.go index a83beffdfd..ce50e1ce49 100644 --- a/internal/state/listener.go +++ b/internal/state/listener.go @@ -1,6 +1,7 @@ package state import ( + "errors" "fmt" "strings" @@ -35,7 +36,7 @@ type listenerConfigurator interface { } type listenerConfiguratorFactory struct { - https *httpsListenerConfigurator + https *httpListenerConfigurator http *httpListenerConfigurator } @@ -60,68 +61,99 @@ func newListenerConfiguratorFactory( } } -type httpsListenerConfigurator struct { +type httpListenerConfigurator struct { gateway *v1beta1.Gateway secretMemoryMgr SecretDiskMemoryManager usedHostnames map[string]*listener + validate func(gl v1beta1.Listener) []conditions.Condition +} + +func newHTTPListenerConfigurator(gw *v1beta1.Gateway) *httpListenerConfigurator { + return &httpListenerConfigurator{ + usedHostnames: make(map[string]*listener), + gateway: gw, + validate: validateHTTPListener, + } } func newHTTPSListenerConfigurator( gateway *v1beta1.Gateway, secretMemoryMgr SecretDiskMemoryManager, -) *httpsListenerConfigurator { - return &httpsListenerConfigurator{ +) *httpListenerConfigurator { + return &httpListenerConfigurator{ gateway: gateway, secretMemoryMgr: secretMemoryMgr, usedHostnames: make(map[string]*listener), + validate: func(gl v1beta1.Listener) []conditions.Condition { + return validateHTTPSListener(gl, gateway.Namespace) + }, } } -func (c *httpsListenerConfigurator) configure(gl v1beta1.Listener) *listener { - validate := func(gl v1beta1.Listener) []conditions.Condition { - return validateHTTPSListener(gl, c.gateway.Namespace) +func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { + conds := c.validate(gl) + + if len(c.gateway.Spec.Addresses) > 0 { + conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported")) } - l := configureListenerAndUseHostname(gl, c.gateway, c.usedHostnames, validate) + validHostnameErr := validateListenerHostname(gl.Hostname) + if validHostnameErr != nil { + msg := fmt.Sprintf("Invalid hostname: %v", validHostnameErr) + conds = append(conds, conditions.NewListenerUnsupportedValue(msg)) + } - if !l.Valid { - return l + l := &listener{ + Source: gl, + Valid: len(conds) == 0, + Routes: make(map[types.NamespacedName]*route), + AcceptedHostnames: make(map[string]struct{}), + Conditions: conds, } - nsname := types.NamespacedName{ - Namespace: c.gateway.Namespace, - Name: string(gl.TLS.CertificateRefs[0].Name), + // this ensures that we don't check for hostname collisions for invalid hostnames + if validHostnameErr != nil { + return l } - path, err := c.secretMemoryMgr.Request(nsname) - if err != nil { + h := getHostname(gl.Hostname) + if holder, exist := c.usedHostnames[h]; exist { l.Valid = false - msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err) - l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...) + holder.Valid = false // all listeners for the same hostname become conflicted + holder.SecretPath = "" // ensure secret path is unset for invalid listeners - return l + format := "Multiple listeners for the same port use the same hostname %q; " + + "ensure only one listener uses that hostname" + conflictedConds := conditions.NewListenerConflictedHostname(fmt.Sprintf(format, h)) + + holder.Conditions = append(holder.Conditions, conflictedConds...) + l.Conditions = append(l.Conditions, conflictedConds...) + } else { + c.usedHostnames[h] = l } - l.SecretPath = path + if !l.Valid { + return l + } - return l -} + if gl.Protocol == v1beta1.HTTPSProtocolType { + nsname := types.NamespacedName{ + Namespace: c.gateway.Namespace, + Name: string(gl.TLS.CertificateRefs[0].Name), + } -type httpListenerConfigurator struct { - usedHostnames map[string]*listener - gateway *v1beta1.Gateway -} + var err error -func newHTTPListenerConfigurator(gw *v1beta1.Gateway) *httpListenerConfigurator { - return &httpListenerConfigurator{ - usedHostnames: make(map[string]*listener), - gateway: gw, + l.SecretPath, err = c.secretMemoryMgr.Request(nsname) + if err != nil { + msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err) + l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...) + l.Valid = false + } } -} -func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { - return configureListenerAndUseHostname(gl, c.gateway, c.usedHostnames, validateHTTPListener) + return l } type invalidProtocolListenerConfigurator struct{} @@ -145,60 +177,6 @@ func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *li } } -func configureListenerAndUseHostname( - gl v1beta1.Listener, - gw *v1beta1.Gateway, - usedHostnames map[string]*listener, - validate func(listener v1beta1.Listener) []conditions.Condition, -) *listener { - conds := validate(gl) - - if len(gw.Spec.Addresses) > 0 { - conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported")) - } - - valid := len(conds) == 0 - - // we validate hostnames separately from validate() because we don't want to check for conflicts for - // invalid hostnames - validHostname, hostnameCond := validateListenerHostname(gl.Hostname) - if validHostname { - h := getHostname(gl.Hostname) - - if holder, exist := usedHostnames[h]; exist { - valid = false - holder.Valid = false // all listeners for the same hostname become conflicted - holder.SecretPath = "" // ensure secret path is unset for invalid listeners - - format := "Multiple listeners for the same port use the same hostname %q; " + - "ensure only one listener uses that hostname" - conflictedConds := conditions.NewListenerConflictedHostname(fmt.Sprintf(format, h)) - holder.Conditions = append(holder.Conditions, conflictedConds...) - conds = append(conds, conflictedConds...) - } - } else { - valid = false - conds = append(conds, hostnameCond) - } - - l := &listener{ - Source: gl, - Valid: valid, - Routes: make(map[types.NamespacedName]*route), - AcceptedHostnames: make(map[string]struct{}), - Conditions: conds, - } - - if !validHostname { - return l - } - - h := getHostname(gl.Hostname) - usedHostnames[h] = l - - return l -} - func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { var conds []conditions.Condition @@ -264,27 +242,27 @@ func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditi return conds } -func validateListenerHostname(host *v1beta1.Hostname) (valid bool, cond conditions.Condition) { +func validateListenerHostname(host *v1beta1.Hostname) error { if host == nil { - return true, conditions.Condition{} + return nil } h := string(*host) if h == "" { - return true, conditions.Condition{} + return nil } // FIXME(pleshakov): For now, we don't support wildcard hostnames if strings.HasPrefix(h, "*") { - return false, conditions.NewListenerUnsupportedValue("Wildcard hostnames are not supported") + return fmt.Errorf("wildcard hostnames are not supported") } msgs := validation.IsDNS1123Subdomain(h) if len(msgs) > 0 { combined := strings.Join(msgs, ",") - return false, conditions.NewListenerUnsupportedValue(fmt.Sprintf("Invalid hostname: %s", combined)) + return errors.New(combined) } - return true, conditions.Condition{} + return nil } diff --git a/internal/state/listener_test.go b/internal/state/listener_test.go index 6ad3b4ceaa..3eb58f4ddf 100644 --- a/internal/state/listener_test.go +++ b/internal/state/listener_test.go @@ -189,44 +189,34 @@ func TestValidateHTTPSListener(t *testing.T) { func TestValidateListenerHostname(t *testing.T) { tests := []struct { - hostname *v1beta1.Hostname - expectedCondition conditions.Condition - name string - expectedValid bool + hostname *v1beta1.Hostname + name string + expectErr bool }{ { - hostname: nil, - expectedValid: true, - expectedCondition: conditions.Condition{}, - name: "nil hostname", + hostname: nil, + expectErr: false, + name: "nil hostname", }, { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("")), - expectedValid: true, - expectedCondition: conditions.Condition{}, - name: "empty hostname", + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("")), + expectErr: false, + name: "empty hostname", }, { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), - expectedValid: true, - expectedCondition: conditions.Condition{}, - name: "valid hostname", + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), + expectErr: false, + name: "valid hostname", }, { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")), - expectedValid: false, - expectedCondition: conditions.NewListenerUnsupportedValue("Wildcard hostnames are not supported"), - name: "wildcard hostname", + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")), + expectErr: true, + name: "wildcard hostname", }, { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")), - expectedValid: false, - expectedCondition: conditions.NewListenerUnsupportedValue( - "Invalid hostname: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric " + - "characters, '-' or '.', and must start and end with an alphanumeric character " + - "(e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?" + - `(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`), - name: "invalid hostname", + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")), + expectErr: true, + name: "invalid hostname", }, } @@ -234,10 +224,13 @@ func TestValidateListenerHostname(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - valid, cond := validateListenerHostname(test.hostname) + err := validateListenerHostname(test.hostname) - g.Expect(valid).To(Equal(test.expectedValid)) - g.Expect(cond).To(Equal(test.expectedCondition)) + if test.expectErr { + g.Expect(err).ToNot(BeNil()) + } else { + g.Expect(err).To(BeNil()) + } }) } } From 6f056971925f0641466cdd2bec2199d45bc4e011 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 7 Feb 2023 17:48:48 -0600 Subject: [PATCH 4/8] Add comments --- internal/state/statuses.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/state/statuses.go b/internal/state/statuses.go index 71e9549d53..3cbd48fb9c 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -99,6 +99,8 @@ func buildStatuses(graph *graph) Statuses { for name, l := range graph.Gateway.Listeners { conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)+1) // 1 is for missing GC + // We add default conds first, so that any additional conditions will override them, which is + // ensured by DeduplicateConditions. conds = append(conds, defaultConds...) conds = append(conds, l.Conditions...) @@ -140,6 +142,8 @@ func buildStatuses(graph *graph) Statuses { for ref, cond := range r.InvalidSectionNameRefs { baseConds := buildBaseRouteConditions(gcValidAndExist) + // We add baseConds first, so that any additional conditions will override them, which is + // ensured by DeduplicateConditions. conds := make([]conditions.Condition, 0, len(baseConds)+1) conds = append(conds, baseConds...) conds = append(conds, cond) From ef98d5810d61f0652c03b6799e8d289846628a40 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 7 Feb 2023 18:04:09 -0600 Subject: [PATCH 5/8] Simply test name --- internal/state/conditions/conditions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/state/conditions/conditions_test.go b/internal/state/conditions/conditions_test.go index 06bb47e782..b0510dc667 100644 --- a/internal/state/conditions/conditions_test.go +++ b/internal/state/conditions/conditions_test.go @@ -7,7 +7,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestDeduplicateDeduplicateRouteConditions(t *testing.T) { +func TestDeduplicateConditions(t *testing.T) { g := NewGomegaWithT(t) conds := []Condition{ From d00a1f0332efabeec45fde40f681d5b51ce13549 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 7 Feb 2023 18:41:06 -0600 Subject: [PATCH 6/8] Refactor configure --- internal/state/listener.go | 68 +++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/internal/state/listener.go b/internal/state/listener.go index ce50e1ce49..987123bfe6 100644 --- a/internal/state/listener.go +++ b/internal/state/listener.go @@ -90,10 +90,14 @@ func newHTTPSListenerConfigurator( } } -func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { - conds := c.validate(gl) +func buildListener( + gl v1beta1.Listener, + gw *v1beta1.Gateway, + validate func(gl v1beta1.Listener) []conditions.Condition, +) (l *listener, validHostname bool) { + conds := validate(gl) - if len(c.gateway.Spec.Addresses) > 0 { + if len(gw.Spec.Addresses) > 0 { conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported")) } @@ -103,20 +107,18 @@ func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { conds = append(conds, conditions.NewListenerUnsupportedValue(msg)) } - l := &listener{ + return &listener{ Source: gl, Valid: len(conds) == 0, Routes: make(map[types.NamespacedName]*route), AcceptedHostnames: make(map[string]struct{}), Conditions: conds, - } + }, validHostnameErr == nil +} - // this ensures that we don't check for hostname collisions for invalid hostnames - if validHostnameErr != nil { - return l - } +func (c *httpListenerConfigurator) ensureUniqueHostnamesAmongListeners(l *listener) { + h := getHostname(l.Source.Hostname) - h := getHostname(gl.Hostname) if holder, exist := c.usedHostnames[h]; exist { l.Valid = false @@ -129,28 +131,42 @@ func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { holder.Conditions = append(holder.Conditions, conflictedConds...) l.Conditions = append(l.Conditions, conflictedConds...) - } else { - c.usedHostnames[h] = l + + return } + c.usedHostnames[h] = l +} + +func (c *httpListenerConfigurator) loadSecretIntoListener(l *listener) { if !l.Valid { - return l + return + } + + nsname := types.NamespacedName{ + Namespace: c.gateway.Namespace, + Name: string(l.Source.TLS.CertificateRefs[0].Name), + } + + var err error + + l.SecretPath, err = c.secretMemoryMgr.Request(nsname) + if err != nil { + msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err) + l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...) + l.Valid = false + } +} + +func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { + l, validHostname := buildListener(gl, c.gateway, c.validate) + + if validHostname { + c.ensureUniqueHostnamesAmongListeners(l) } if gl.Protocol == v1beta1.HTTPSProtocolType { - nsname := types.NamespacedName{ - Namespace: c.gateway.Namespace, - Name: string(gl.TLS.CertificateRefs[0].Name), - } - - var err error - - l.SecretPath, err = c.secretMemoryMgr.Request(nsname) - if err != nil { - msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err) - l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...) - l.Valid = false - } + c.loadSecretIntoListener(l) } return l From 9dc2eaf95cb72db9881da75cb20a7ca9fa2f16f4 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 8 Feb 2023 12:39:32 -0600 Subject: [PATCH 7/8] Introduce a variable --- internal/state/listener.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/state/listener.go b/internal/state/listener.go index 987123bfe6..9a9b9e4b2f 100644 --- a/internal/state/listener.go +++ b/internal/state/listener.go @@ -250,8 +250,8 @@ func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditi } - if len(listener.TLS.CertificateRefs) > 1 { - msg := fmt.Sprintf("Only 1 certificateRef is supported, got %d", len(listener.TLS.CertificateRefs)) + if l := len(listener.TLS.CertificateRefs); l > 1 { + msg := fmt.Sprintf("Only 1 certificateRef is supported, got %d", l) conds = append(conds, conditions.NewListenerUnsupportedValue(msg)) } From 932b4bb9674d32de6edd732382e4fffb7c7c7ace Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 8 Feb 2023 12:49:10 -0600 Subject: [PATCH 8/8] Refactor buildListener --- internal/state/listener.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/internal/state/listener.go b/internal/state/listener.go index 9a9b9e4b2f..d38d0f485c 100644 --- a/internal/state/listener.go +++ b/internal/state/listener.go @@ -90,12 +90,12 @@ func newHTTPSListenerConfigurator( } } -func buildListener( +func validateListener( gl v1beta1.Listener, gw *v1beta1.Gateway, validate func(gl v1beta1.Listener) []conditions.Condition, -) (l *listener, validHostname bool) { - conds := validate(gl) +) (conds []conditions.Condition, validHostname bool) { + conds = validate(gl) if len(gw.Spec.Addresses) > 0 { conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported")) @@ -107,13 +107,7 @@ func buildListener( conds = append(conds, conditions.NewListenerUnsupportedValue(msg)) } - return &listener{ - Source: gl, - Valid: len(conds) == 0, - Routes: make(map[types.NamespacedName]*route), - AcceptedHostnames: make(map[string]struct{}), - Conditions: conds, - }, validHostnameErr == nil + return conds, validHostnameErr == nil } func (c *httpListenerConfigurator) ensureUniqueHostnamesAmongListeners(l *listener) { @@ -159,7 +153,15 @@ func (c *httpListenerConfigurator) loadSecretIntoListener(l *listener) { } func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener { - l, validHostname := buildListener(gl, c.gateway, c.validate) + conds, validHostname := validateListener(gl, c.gateway, c.validate) + + l := &listener{ + Source: gl, + Valid: len(conds) == 0, + Routes: make(map[types.NamespacedName]*route), + AcceptedHostnames: make(map[string]struct{}), + Conditions: conds, + } if validHostname { c.ensureUniqueHostnamesAmongListeners(l)