From 5f9da63044e050b38854f531f1df1cb99e417d8d Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 13 Apr 2023 10:18:05 -0400 Subject: [PATCH 1/3] Refactor listener validation --- internal/state/graph/gateway.go | 401 ++++++++++++++------------- internal/state/graph/gateway_test.go | 56 ++-- 2 files changed, 228 insertions(+), 229 deletions(-) diff --git a/internal/state/graph/gateway.go b/internal/state/graph/gateway.go index b8ff28a83c..ea7ebdcb92 100644 --- a/internal/state/graph/gateway.go +++ b/internal/state/graph/gateway.go @@ -134,23 +134,18 @@ func buildListeners( return listeners } -type listenerConfigurator interface { - configure(listener v1beta1.Listener) *Listener -} - type listenerConfiguratorFactory struct { - https *httpListenerConfigurator - http *httpListenerConfigurator + http, https, unsupportedProtocol *listenerConfigurator } -func (f *listenerConfiguratorFactory) getConfiguratorForListener(l v1beta1.Listener) listenerConfigurator { +func (f *listenerConfiguratorFactory) getConfiguratorForListener(l v1beta1.Listener) *listenerConfigurator { switch l.Protocol { case v1beta1.HTTPProtocolType: return f.http case v1beta1.HTTPSProtocolType: return f.https default: - return newInvalidProtocolListenerConfigurator() + return f.unsupportedProtocol } } @@ -159,253 +154,267 @@ func newListenerConfiguratorFactory( secretMemoryMgr secrets.SecretDiskMemoryManager, ) *listenerConfiguratorFactory { return &listenerConfiguratorFactory{ - https: newHTTPSListenerConfigurator(gw, secretMemoryMgr), - http: newHTTPListenerConfigurator(gw), + unsupportedProtocol: &listenerConfigurator{ + validators: []listenerValidator{ + func(listener v1beta1.Listener) []conditions.Condition { + valErr := field.NotSupported( + field.NewPath("protocol"), + listener.Protocol, + []string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)}, + ) + return []conditions.Condition{conditions.NewListenerUnsupportedProtocol(valErr.Error())} + }, + }, + }, + http: &listenerConfigurator{ + validators: []listenerValidator{ + validateListenerHostname, + createAddressesValidator(gw), + validateHTTPListener, + }, + conflictResolvers: []listenerConflictResolver{ + createHostnameConflictResolver(), + }, + }, + https: &listenerConfigurator{ + validators: []listenerValidator{ + validateListenerHostname, + createAddressesValidator(gw), + createHTTPSListenerValidator(gw.Namespace), + }, + conflictResolvers: []listenerConflictResolver{ + createHostnameConflictResolver(), + }, + externalReferenceResolvers: []listenerExternalReferenceResolver{ + createExternalReferencesForTLSSecretsResolver(gw.Namespace, secretMemoryMgr), + }, + }, } } -type httpListenerConfigurator struct { - gateway *v1beta1.Gateway - secretMemoryMgr secrets.SecretDiskMemoryManager - usedHostnames map[string]*Listener - validate func(gl v1beta1.Listener) []conditions.Condition +// listenerValidator validates a listener. If the listener is invalid, the validator will return appropriate conditions. +type listenerValidator func(v1beta1.Listener) []conditions.Condition + +// listenerConflictResolver resolves conflicts between listeners. In case of a conflict, the resolver will make +// the conflicting listeners invalid - i.e. it will modify the passed listener and the previously processed conflicting +// listener. It will also add appropriate conditions to the listeners. +type listenerConflictResolver func(listener *Listener) + +// listenerExternalReferenceResolver resolves external references for a listener. If the reference is not resolvable, +// the resolver will make the listener invalid and add appropriate conditions. +type listenerExternalReferenceResolver func(listener *Listener) + +// listenerConfigurator is responsible for configuring a listener. +// validators, conflictResolvers, externalReferenceResolvers generate conditions for invalid fields of the listener. +// Because the Gateway status includes a status field for each listener, the messages in those conditions +// don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include +// a path starting from the field of a listener (e.g. "hostname", "tls.options"). +type listenerConfigurator struct { + // validators must not depend on the order of execution. + validators []listenerValidator + + // conflictResolvers can depend on validators - they will only be executed if all validators pass. + conflictResolvers []listenerConflictResolver + // externalReferenceResolvers can depend on validators - they will only be executed if all validators pass. + externalReferenceResolvers []listenerExternalReferenceResolver } -func newHTTPListenerConfigurator(gw *v1beta1.Gateway) *httpListenerConfigurator { - return &httpListenerConfigurator{ - usedHostnames: make(map[string]*Listener), - gateway: gw, - validate: validateHTTPListener, - } -} +func (c *listenerConfigurator) configure(listener v1beta1.Listener) *Listener { + var conds []conditions.Condition -func newHTTPSListenerConfigurator( - gateway *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, -) *httpListenerConfigurator { - return &httpListenerConfigurator{ - gateway: gateway, - secretMemoryMgr: secretMemoryMgr, - usedHostnames: make(map[string]*Listener), - validate: func(gl v1beta1.Listener) []conditions.Condition { - return validateHTTPSListener(gl, gateway.Namespace) - }, + // validators might return different conditions, so we run them all. + for _, validator := range c.validators { + conds = append(conds, validator(listener)...) } -} -func validateListener( - gl v1beta1.Listener, - gw *v1beta1.Gateway, - validate func(gl v1beta1.Listener) []conditions.Condition, -) (conds []conditions.Condition, validHostname bool) { - conds = validate(gl) - - if len(gw.Spec.Addresses) > 0 { - path := field.NewPath("spec", "addresses") - valErr := field.Forbidden(path, "addresses are not supported") - conds = append(conds, conditions.NewListenerUnsupportedAddress(valErr.Error())) + if len(conds) > 0 { + return &Listener{ + Source: listener, + Conditions: conds, + Valid: false, + } } - validHostnameErr := validateListenerHostname(gl.Hostname) - if validHostnameErr != nil { - path := field.NewPath("hostname") - valErr := field.Invalid(path, gl.Hostname, validHostnameErr.Error()) - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + l := &Listener{ + Source: listener, + Routes: make(map[types.NamespacedName]*Route), + AcceptedHostnames: make(map[string]struct{}), + Valid: true, } - return conds, validHostnameErr == nil -} - -func (c *httpListenerConfigurator) ensureUniqueHostnamesAmongListeners(l *Listener) { - h := getHostname(l.Source.Hostname) - - if holder, exist := c.usedHostnames[h]; exist { - l.Valid = false - - holder.Valid = false // all listeners for the same hostname become conflicted - holder.SecretPath = "" // ensure secret path is unset for invalid listeners + // resolvers might add different conditions to the listener, so we run them all. - 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...) + for _, resolver := range c.conflictResolvers { + resolver(l) + } - return + for _, resolver := range c.externalReferenceResolvers { + resolver(l) } - c.usedHostnames[h] = l + return l } -func (c *httpListenerConfigurator) loadSecretIntoListener(l *Listener) { - if !l.Valid { - return +func validateListenerHostname(listener v1beta1.Listener) []conditions.Condition { + if listener.Hostname == nil { + return nil } - nsname := types.NamespacedName{ - Namespace: c.gateway.Namespace, - Name: string(l.Source.TLS.CertificateRefs[0].Name), - } + h := string(*listener.Hostname) - var err error + if h == "" { + return nil + } - l.SecretPath, err = c.secretMemoryMgr.Request(nsname) + err := validateHostname(h) if err != nil { - path := field.NewPath("tls", "certificateRefs").Index(0) - // field.NotFound could be better, but it doesn't allow us to set the error message. - valErr := field.Invalid(path, nsname, err.Error()) - - l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - l.Valid = false + path := field.NewPath("hostname") + valErr := field.Invalid(path, listener.Hostname, err.Error()) + return []conditions.Condition{conditions.NewListenerUnsupportedValue(valErr.Error())} } + return nil } -func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *Listener { - // The functions called by configure() generate conditions for invalid fields of the listener. - // Because the Gateway status includes a status field for each listener, the messages in those conditions - // don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include - // a path starting from the field of a listener (e.g. "hostname", "tls.options"). - - 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, +func createAddressesValidator(gw *v1beta1.Gateway) listenerValidator { + return func(listener v1beta1.Listener) []conditions.Condition { + if len(gw.Spec.Addresses) > 0 { + path := field.NewPath("spec", "addresses") + valErr := field.Forbidden(path, "addresses are not supported") + return []conditions.Condition{conditions.NewListenerUnsupportedAddress(valErr.Error())} + } + return nil } +} - if validHostname { - c.ensureUniqueHostnamesAmongListeners(l) +func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { + if listener.Port != 80 { + path := field.NewPath("port") + valErr := field.NotSupported(path, listener.Port, []string{"80"}) + return []conditions.Condition{conditions.NewListenerPortUnavailable(valErr.Error())} } - if gl.Protocol == v1beta1.HTTPSProtocolType { - c.loadSecretIntoListener(l) + if listener.TLS != nil { + panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) } - return l + return nil } -type invalidProtocolListenerConfigurator struct{} +func createHTTPSListenerValidator(gwNsName string) listenerValidator { + return func(listener v1beta1.Listener) []conditions.Condition { + var conds []conditions.Condition -func newInvalidProtocolListenerConfigurator() *invalidProtocolListenerConfigurator { - return &invalidProtocolListenerConfigurator{} -} + if listener.Port != 443 { + path := field.NewPath("port") + valErr := field.NotSupported(path, listener.Port, []string{"443"}) + conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) + } -func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *Listener { - valErr := field.NotSupported( - field.NewPath("protocol"), - gl.Protocol, - []string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)}, - ) + if listener.TLS == nil { + panicForBrokenWebhookAssumption(fmt.Errorf("tls is nil for HTTPS listener %q", listener.Name)) + } - return &Listener{ - Source: gl, - Valid: false, - Routes: make(map[types.NamespacedName]*Route), - AcceptedHostnames: make(map[string]struct{}), - Conditions: []conditions.Condition{ - conditions.NewListenerUnsupportedProtocol(valErr.Error()), - }, - } -} + tlsPath := field.NewPath("tls") -func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { - var conds []conditions.Condition + if *listener.TLS.Mode != v1beta1.TLSModeTerminate { + valErr := field.NotSupported( + tlsPath.Child("mode"), + *listener.TLS.Mode, + []string{string(v1beta1.TLSModeTerminate)}, + ) + conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + } - if listener.Port != 80 { - path := field.NewPath("port") - valErr := field.NotSupported(path, listener.Port, []string{"80"}) - conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) - } + if len(listener.TLS.Options) > 0 { + path := tlsPath.Child("options") + valErr := field.Forbidden(path, "options are not supported") + conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + } - if listener.TLS != nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) - } + if len(listener.TLS.CertificateRefs) == 0 { + panicForBrokenWebhookAssumption(fmt.Errorf("zero certificateRefs for HTTPS listener %q", listener.Name)) + } - return conds -} + certRef := listener.TLS.CertificateRefs[0] -func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditions.Condition { - var conds []conditions.Condition + certRefPath := tlsPath.Child("certificateRefs").Index(0) - if listener.Port != 443 { - path := field.NewPath("port") - valErr := field.NotSupported(path, listener.Port, []string{"443"}) - conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) - } + if certRef.Kind != nil && *certRef.Kind != "Secret" { + path := certRefPath.Child("kind") + valErr := field.NotSupported(path, *certRef.Kind, []string{"Secret"}) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + } - if listener.TLS == nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is nil for HTTPS listener %q", listener.Name)) - } + // for Kind Secret, certRef.Group must be nil or empty + if certRef.Group != nil && *certRef.Group != "" { + path := certRefPath.Child("group") + valErr := field.NotSupported(path, *certRef.Group, []string{""}) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + } - tlsPath := field.NewPath("tls") + // secret must be in the same namespace as the gateway + if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName { + const detail = "Referenced Secret must belong to the same namespace as the Gateway" + path := certRefPath.Child("namespace") + valErr := field.Invalid(path, certRef.Namespace, detail) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + } - if *listener.TLS.Mode != v1beta1.TLSModeTerminate { - valErr := field.NotSupported( - tlsPath.Child("mode"), - *listener.TLS.Mode, - []string{string(v1beta1.TLSModeTerminate)}, - ) - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) - } + if l := len(listener.TLS.CertificateRefs); l > 1 { + path := tlsPath.Child("certificateRefs") + valErr := field.TooMany(path, l, 1) + conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + } - if len(listener.TLS.Options) > 0 { - path := tlsPath.Child("options") - valErr := field.Forbidden(path, "options are not supported") - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + return conds } +} - if len(listener.TLS.CertificateRefs) == 0 { - panicForBrokenWebhookAssumption(fmt.Errorf("zero certificateRefs for HTTPS listener %q", listener.Name)) - } +func createHostnameConflictResolver() listenerConflictResolver { + usedHostnames := make(map[string]*Listener) - certRef := listener.TLS.CertificateRefs[0] + return func(l *Listener) { + h := getHostname(l.Source.Hostname) - certRefPath := tlsPath.Child("certificateRefs").Index(0) + if holder, exist := usedHostnames[h]; exist { + l.Valid = false - if certRef.Kind != nil && *certRef.Kind != "Secret" { - path := certRefPath.Child("kind") - valErr := field.NotSupported(path, *certRef.Kind, []string{"Secret"}) - conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - } + holder.Valid = false // all listeners for the same hostname become conflicted - // for Kind Secret, certRef.Group must be nil or empty - if certRef.Group != nil && *certRef.Group != "" { - path := certRefPath.Child("group") - valErr := field.NotSupported(path, *certRef.Group, []string{""}) - conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - } + 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)) - // secret must be in the same namespace as the gateway - if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName { - const detail = "Referenced Secret must belong to the same namespace as the Gateway" - path := certRefPath.Child("namespace") - valErr := field.Invalid(path, certRef.Namespace, detail) - conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - } + holder.Conditions = append(holder.Conditions, conflictedConds...) + l.Conditions = append(l.Conditions, conflictedConds...) - if l := len(listener.TLS.CertificateRefs); l > 1 { - path := tlsPath.Child("certificateRefs") - valErr := field.TooMany(path, l, 1) - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) - } + return + } - return conds + usedHostnames[h] = l + } } -func validateListenerHostname(host *v1beta1.Hostname) error { - if host == nil { - return nil - } +func createExternalReferencesForTLSSecretsResolver( + gwNs string, + secretMemoryMgr secrets.SecretDiskMemoryManager, +) listenerExternalReferenceResolver { + return func(l *Listener) { + nsname := types.NamespacedName{ + Namespace: gwNs, + Name: string(l.Source.TLS.CertificateRefs[0].Name), + } - h := string(*host) + var err error - if h == "" { - return nil - } + l.SecretPath, err = secretMemoryMgr.Request(nsname) + if err != nil { + path := field.NewPath("tls", "certificateRefs").Index(0) + // field.NotFound could be better, but it doesn't allow us to set the error message. + valErr := field.Invalid(path, nsname, err.Error()) - return validateHostname(h) + l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + l.Valid = false + } + } } diff --git a/internal/state/graph/gateway_test.go b/internal/state/graph/gateway_test.go index e824c5a18f..b276f4c660 100644 --- a/internal/state/graph/gateway_test.go +++ b/internal/state/graph/gateway_test.go @@ -320,10 +320,8 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-80-2": { - Source: listener802, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, + Source: listener802, + Valid: false, Conditions: []conditions.Condition{ conditions.NewListenerUnsupportedProtocol( `protocol: Unsupported value: "TCP": supported values: "HTTP", "HTTPS"`, @@ -340,10 +338,8 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-80-5": { - Source: listener805, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, + Source: listener805, + Valid: false, Conditions: []conditions.Condition{ conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`), }, @@ -358,10 +354,8 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-443-6": { - Source: listener4436, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, + Source: listener4436, + Valid: false, Conditions: []conditions.Condition{ conditions.NewListenerPortUnavailable(`port: Unsupported value: 444: supported values: "443"`), }, @@ -376,19 +370,15 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-80-6": { - Source: listener806, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, + Source: listener806, + Valid: false, Conditions: []conditions.Condition{ conditions.NewListenerUnsupportedValue(invalidHostnameMsg), }, }, "listener-443-4": { - Source: listener4434, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, + Source: listener4434, + Valid: false, Conditions: []conditions.Condition{ conditions.NewListenerUnsupportedValue(invalidHostnameMsg), }, @@ -479,6 +469,7 @@ func TestBuildGateway(t *testing.T) { Routes: map[types.NamespacedName]*Route{}, AcceptedHostnames: map[string]struct{}{}, Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg), + SecretPath: "/etc/nginx/secrets/test_secret", }, "listener-443-3": { Source: listener4433, @@ -486,6 +477,7 @@ func TestBuildGateway(t *testing.T) { Routes: map[types.NamespacedName]*Route{}, AcceptedHostnames: map[string]struct{}{}, Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg), + SecretPath: "/etc/nginx/secrets/test_secret", }, }, }, @@ -502,10 +494,8 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-80-1": { - Source: listener801, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, + Source: listener801, + Valid: false, Conditions: []conditions.Condition{ conditions.NewListenerUnsupportedAddress( "spec.addresses: Forbidden: addresses are not supported", @@ -513,11 +503,9 @@ func TestBuildGateway(t *testing.T) { }, }, "listener-443-1": { - Source: listener4431, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, - AcceptedHostnames: map[string]struct{}{}, - SecretPath: "", + Source: listener4431, + Valid: false, + SecretPath: "", Conditions: []conditions.Condition{ conditions.NewListenerUnsupportedAddress( "spec.addresses: Forbidden: addresses are not supported", @@ -731,7 +719,9 @@ func TestValidateHTTPSListener(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - result := validateHTTPSListener(test.l, gwNs) + v := createHTTPSListenerValidator(gwNs) + + result := v(test.l) g.Expect(result).To(Equal(test.expected)) }) } @@ -774,12 +764,12 @@ func TestValidateListenerHostname(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - err := validateListenerHostname(test.hostname) + conds := validateListenerHostname(v1beta1.Listener{Hostname: test.hostname}) if test.expectErr { - g.Expect(err).ToNot(BeNil()) + g.Expect(conds).ToNot(BeEmpty()) } else { - g.Expect(err).To(BeNil()) + g.Expect(conds).To(BeEmpty()) } }) } From d08424454c09388cae5218af7b730f9fa3171327 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 13 Apr 2023 13:26:34 -0400 Subject: [PATCH 2/3] Report proper conditions when GatewayClass is invalid or doesn't exist If GatewayClass is invalid or doesn't exist: - For Gateway Listeners: NKG will make every listener invalid and report Accepted condition with status False and reason NoValidGatewayClass in every listener status. - For HTTPRoutes: An HTTPRoute will not be able to attach to any listener, because they will be invalid. This is already handled: NKG will report Accepted condition with status False and reason InvalidListener. Fixes https://github.com/nginxinc/nginx-kubernetes-gateway/issues/307 --- internal/state/change_processor_test.go | 54 +++--- internal/state/conditions/conditions.go | 66 +++++-- internal/state/graph/gateway.go | 24 ++- internal/state/graph/gateway_test.go | 74 +++++-- internal/state/graph/graph.go | 2 +- internal/state/statuses.go | 39 +--- internal/state/statuses_test.go | 245 +++++------------------- 7 files changed, 218 insertions(+), 286 deletions(-) diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index 1788be7ae0..f4e2bf0d76 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -316,18 +316,20 @@ var _ = Describe("ChangeProcessor", func() { ObservedGeneration: gw1.Generation, ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { - AttachedRoutes: 1, - Conditions: append( - conditions.NewDefaultListenerConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), + AttachedRoutes: 0, + Conditions: []conditions.Condition{ + conditions.NewListenerResolvedRefs(), + conditions.NewListenerNoConflicts(), + conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist"), + }, }, "listener-443-1": { - AttachedRoutes: 1, - Conditions: append( - conditions.NewDefaultListenerConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), + AttachedRoutes: 0, + Conditions: []conditions.Condition{ + conditions.NewListenerResolvedRefs(), + conditions.NewListenerNoConflicts(), + conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist"), + }, }, }, }, @@ -339,18 +341,16 @@ var _ = Describe("ChangeProcessor", func() { { GatewayNsName: client.ObjectKeyFromObject(gw1), SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), + Conditions: []conditions.Condition{ + conditions.NewRouteInvalidListener(), + }, }, { GatewayNsName: client.ObjectKeyFromObject(gw1), SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), + Conditions: []conditions.Condition{ + conditions.NewRouteInvalidListener(), + }, }, }, }, @@ -1219,17 +1219,19 @@ var _ = Describe("ChangeProcessor", func() { ListenerStatuses: map[string]state.ListenerStatus{ "listener-80-1": { AttachedRoutes: 0, - Conditions: append( - conditions.NewDefaultListenerConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), + Conditions: []conditions.Condition{ + conditions.NewListenerResolvedRefs(), + conditions.NewListenerNoConflicts(), + conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist"), + }, }, "listener-443-1": { AttachedRoutes: 0, - Conditions: append( - conditions.NewDefaultListenerConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), + Conditions: []conditions.Condition{ + conditions.NewListenerResolvedRefs(), + conditions.NewListenerNoConflicts(), + conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist"), + }, }, }, }, diff --git a/internal/state/conditions/conditions.go b/internal/state/conditions/conditions.go index e70ca2d250..e686386490 100644 --- a/internal/state/conditions/conditions.go +++ b/internal/state/conditions/conditions.go @@ -15,6 +15,10 @@ const ( // is invalid or not supported. ListenerReasonUnsupportedValue v1beta1.ListenerConditionReason = "UnsupportedValue" + // ListenerReasonNoValidGatewayClass is used with the "Accepted" condition when there is no valid GatewayClass + // in the cluster. + ListenerReasonNoValidGatewayClass v1beta1.ListenerConditionReason = "NoValidGatewayClass" + // RouteReasonBackendRefUnsupportedValue is used with the "ResolvedRefs" condition when one of the // Route rules has a backendRef with an unsupported value. RouteReasonBackendRefUnsupportedValue = "UnsupportedValue" @@ -129,27 +133,42 @@ func NewListenerPortUnavailable(msg string) Condition { } } +// NewListenerAccepted returns a Condition that indicates that the Listener is accepted. +func NewListenerAccepted() Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.ListenerReasonAccepted), + Message: "Listener is accepted", + } +} + +// NewListenerResolvedRefs returns a Condition that indicates that all references in a Listener are resolved. +func NewListenerResolvedRefs() Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: string(v1beta1.ListenerReasonResolvedRefs), + Message: "All references are resolved", + } +} + +// NewListenerNoConflicts returns a Condition that indicates that there are no conflicts in a Listener. +func NewListenerNoConflicts() Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionConflicted), + Status: metav1.ConditionFalse, + Reason: string(v1beta1.ListenerReasonNoConflicts), + Message: "No conflicts", + } +} + // 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", - }, + NewListenerAccepted(), + NewListenerResolvedRefs(), + NewListenerNoConflicts(), } } @@ -220,6 +239,17 @@ func NewListenerUnsupportedProtocol(msg string) Condition { } } +// NewListenerNoValidGatewayClass returns a Condition that indicates that the Listener is not accepted because +// there is no valid GatewayClass. +func NewListenerNoValidGatewayClass(msg string) Condition { + return Condition{ + Type: string(v1beta1.ListenerConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(ListenerReasonNoValidGatewayClass), + Message: msg, + } +} + // NewRouteBackendRefInvalidKind returns a Condition that indicates that the Route has a backendRef with an // invalid kind. func NewRouteBackendRefInvalidKind(msg string) Condition { diff --git a/internal/state/graph/gateway.go b/internal/state/graph/gateway.go index ea7ebdcb92..6ca5d5c16b 100644 --- a/internal/state/graph/gateway.go +++ b/internal/state/graph/gateway.go @@ -107,24 +107,25 @@ func processGateways( } } -func buildGateway(gw *v1beta1.Gateway, secretMemoryMgr secrets.SecretDiskMemoryManager) *Gateway { +func buildGateway(gw *v1beta1.Gateway, secretMemoryMgr secrets.SecretDiskMemoryManager, gc *GatewayClass) *Gateway { if gw == nil { return nil } return &Gateway{ Source: gw, - Listeners: buildListeners(gw, secretMemoryMgr), + Listeners: buildListeners(gw, secretMemoryMgr, gc), } } func buildListeners( gw *v1beta1.Gateway, secretMemoryMgr secrets.SecretDiskMemoryManager, + gc *GatewayClass, ) map[string]*Listener { listeners := make(map[string]*Listener) - listenerFactory := newListenerConfiguratorFactory(gw, secretMemoryMgr) + listenerFactory := newListenerConfiguratorFactory(gw, secretMemoryMgr, gc) for _, gl := range gw.Spec.Listeners { configurator := listenerFactory.getConfiguratorForListener(gl) @@ -152,6 +153,7 @@ func (f *listenerConfiguratorFactory) getConfiguratorForListener(l v1beta1.Liste func newListenerConfiguratorFactory( gw *v1beta1.Gateway, secretMemoryMgr secrets.SecretDiskMemoryManager, + gc *GatewayClass, ) *listenerConfiguratorFactory { return &listenerConfiguratorFactory{ unsupportedProtocol: &listenerConfigurator{ @@ -170,6 +172,7 @@ func newListenerConfiguratorFactory( validators: []listenerValidator{ validateListenerHostname, createAddressesValidator(gw), + createNoValidGatewayClassValidator(gc), validateHTTPListener, }, conflictResolvers: []listenerConflictResolver{ @@ -180,6 +183,7 @@ func newListenerConfiguratorFactory( validators: []listenerValidator{ validateListenerHostname, createAddressesValidator(gw), + createNoValidGatewayClassValidator(gc), createHTTPSListenerValidator(gw.Namespace), }, conflictResolvers: []listenerConflictResolver{ @@ -286,6 +290,20 @@ func createAddressesValidator(gw *v1beta1.Gateway) listenerValidator { } } +func createNoValidGatewayClassValidator(gc *GatewayClass) listenerValidator { + return func(listener v1beta1.Listener) []conditions.Condition { + if gc == nil { + return []conditions.Condition{conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist")} + } + + if !gc.Valid { + return []conditions.Condition{conditions.NewListenerNoValidGatewayClass("GatewayClass is invalid")} + } + + return nil + } +} + func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { if listener.Port != 80 { path := field.NewPath("port") diff --git a/internal/state/graph/gateway_test.go b/internal/state/graph/gateway_test.go index b276f4c660..a5f18a27c9 100644 --- a/internal/state/graph/gateway_test.go +++ b/internal/state/graph/gateway_test.go @@ -278,13 +278,22 @@ func TestBuildGateway(t *testing.T) { return lastCreatedGateway } + validGC := &GatewayClass{ + Valid: true, + } + invalidGC := &GatewayClass{ + Valid: false, + } + tests := []struct { - gateway *v1beta1.Gateway - expected *Gateway - name string + gateway *v1beta1.Gateway + gatewayClass *GatewayClass + expected *Gateway + name string }{ { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener801}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener801}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -299,7 +308,8 @@ func TestBuildGateway(t *testing.T) { name: "valid http listener", }, { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener4431}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener4431}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -315,7 +325,8 @@ func TestBuildGateway(t *testing.T) { name: "valid https listener", }, { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener802}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener802}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -333,7 +344,8 @@ func TestBuildGateway(t *testing.T) { name: "invalid listener protocol", }, { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener805}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener805}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -349,7 +361,8 @@ func TestBuildGateway(t *testing.T) { name: "invalid http listener", }, { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener4436}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener4436}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -365,7 +378,8 @@ func TestBuildGateway(t *testing.T) { name: "invalid https listener", }, { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener806, listener4434}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener806, listener4434}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -388,7 +402,8 @@ func TestBuildGateway(t *testing.T) { name: "invalid hostnames", }, { - gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener4435}}), + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener4435}}), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -409,6 +424,7 @@ func TestBuildGateway(t *testing.T) { gateway: createGateway( gatewayCfg{listeners: []v1beta1.Listener{listener801, listener803, listener4431, listener4432}}, ), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -446,6 +462,7 @@ func TestBuildGateway(t *testing.T) { gateway: createGateway( gatewayCfg{listeners: []v1beta1.Listener{listener801, listener804, listener4431, listener4433}}, ), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -490,6 +507,7 @@ func TestBuildGateway(t *testing.T) { {}, }, }), + gatewayClass: validGC, expected: &Gateway{ Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ @@ -521,6 +539,40 @@ func TestBuildGateway(t *testing.T) { expected: nil, name: "nil gateway", }, + { + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener801}}), + gatewayClass: invalidGC, + expected: &Gateway{ + Source: getLastCreatedGetaway(), + Listeners: map[string]*Listener{ + "listener-80-1": { + Source: listener801, + Valid: false, + Conditions: []conditions.Condition{ + conditions.NewListenerNoValidGatewayClass("GatewayClass is invalid"), + }, + }, + }, + }, + name: "invalid gatewayclass", + }, + { + gateway: createGateway(gatewayCfg{listeners: []v1beta1.Listener{listener801}}), + gatewayClass: nil, + expected: &Gateway{ + Source: getLastCreatedGetaway(), + Listeners: map[string]*Listener{ + "listener-80-1": { + Source: listener801, + Valid: false, + Conditions: []conditions.Condition{ + conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist"), + }, + }, + }, + }, + name: "nil gatewayclass", + }, } secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} @@ -534,7 +586,7 @@ func TestBuildGateway(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - result := buildGateway(test.gateway, secretMemoryMgr) + result := buildGateway(test.gateway, secretMemoryMgr, test.gatewayClass) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) }) } diff --git a/internal/state/graph/graph.go b/internal/state/graph/graph.go index 4098401f88..300916b32c 100644 --- a/internal/state/graph/graph.go +++ b/internal/state/graph/graph.go @@ -49,7 +49,7 @@ func BuildGraph( processedGws := processGateways(state.Gateways, gcName) - gw := buildGateway(processedGws.Winner, secretMemoryMgr) + gw := buildGateway(processedGws.Winner, secretMemoryMgr, gc) routes := buildRoutesForGateways(validators.HTTPFieldsValidator, state.HTTPRoutes, processedGws.GetAllNsNames()) bindRoutesToListeners(routes, gw) diff --git a/internal/state/statuses.go b/internal/state/statuses.go index a4898c833d..10a75757a7 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -96,34 +96,19 @@ func buildStatuses(graph *graph.Graph) Statuses { } } - gcValidAndExist := graph.GatewayClass != nil && graph.GatewayClass.Valid - if graph.Gateway != nil { listenerStatuses := make(map[string]ListenerStatus) defaultConds := conditions.NewDefaultListenerConditions() for name, l := range graph.Gateway.Listeners { - missingGCCondCount := 0 - if !gcValidAndExist { - missingGCCondCount = 1 - } - - conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)+missingGCCondCount) + conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)) // 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...) - if missingGCCondCount == 1 { - // 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{ AttachedRoutes: int32(len(l.Routes)), Conditions: conditions.DeduplicateConditions(conds), @@ -144,18 +129,18 @@ func buildStatuses(graph *graph.Graph) Statuses { for nsname, r := range graph.Routes { parentStatuses := make([]ParentStatus, 0, len(r.ParentRefs)) - baseConds := buildBaseRouteConditions(gcValidAndExist) + defaultConds := conditions.NewDefaultRouteConditions() for _, ref := range r.ParentRefs { failedAttachmentCondCount := 0 if !ref.Attached { failedAttachmentCondCount = 1 } - allConds := make([]conditions.Condition, 0, len(r.Conditions)+len(baseConds)+failedAttachmentCondCount) + allConds := make([]conditions.Condition, 0, len(r.Conditions)+len(defaultConds)+failedAttachmentCondCount) - // We add baseConds first, so that any additional conditions will override them, which is + // We add defaultConds first, so that any additional conditions will override them, which is // ensured by DeduplicateConditions. - allConds = append(allConds, baseConds...) + allConds = append(allConds, defaultConds...) allConds = append(allConds, r.Conditions...) if failedAttachmentCondCount == 1 { allConds = append(allConds, ref.FailedAttachmentCondition) @@ -178,17 +163,3 @@ func buildStatuses(graph *graph.Graph) Statuses { return statuses } - -func buildBaseRouteConditions(gcValidAndExist bool) []conditions.Condition { - conds := conditions.NewDefaultRouteConditions() - - // 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 - if !gcValidAndExist { - 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 299a1d9125..5111a62dae 100644 --- a/internal/state/statuses_test.go +++ b/internal/state/statuses_test.go @@ -20,15 +20,6 @@ func TestBuildStatuses(t *testing.T) { Status: metav1.ConditionTrue, } - listeners := map[string]*graph.Listener{ - "listener-80-1": { - Valid: true, - Routes: map[types.NamespacedName]*graph.Route{ - {Namespace: "test", Name: "hr-1"}: {}, - }, - }, - } - gw := &v1beta1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -80,204 +71,72 @@ func TestBuildStatuses(t *testing.T) { }, } - tests := []struct { - graph *graph.Graph - expected Statuses - name string - }{ - { - graph: &graph.Graph{ - GatewayClass: &graph.GatewayClass{ - Source: &v1beta1.GatewayClass{ - ObjectMeta: metav1.ObjectMeta{Generation: 1}, - }, - Valid: true, - }, - Gateway: &graph.Gateway{ - Source: gw, - Listeners: listeners, - }, - IgnoredGateways: map[types.NamespacedName]*v1beta1.Gateway{ - {Namespace: "test", Name: "ignored-gateway"}: ignoredGw, - }, - Routes: routes, + graph := &graph.Graph{ + GatewayClass: &graph.GatewayClass{ + Source: &v1beta1.GatewayClass{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, }, - expected: Statuses{ - GatewayClassStatus: &GatewayClassStatus{ - ObservedGeneration: 1, - Conditions: conditions.NewDefaultGatewayClassConditions(), - }, - GatewayStatus: &GatewayStatus{ - NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, - ListenerStatuses: map[string]ListenerStatus{ - "listener-80-1": { - AttachedRoutes: 1, - Conditions: conditions.NewDefaultListenerConditions(), - }, - }, - ObservedGeneration: 2, - }, - IgnoredGatewayStatuses: map[types.NamespacedName]IgnoredGatewayStatus{ - {Namespace: "test", Name: "ignored-gateway"}: {ObservedGeneration: 1}, - }, - HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ - {Namespace: "test", Name: "hr-1"}: { - ObservedGeneration: 3, - ParentStatuses: []ParentStatus{ - { - GatewayNsName: client.ObjectKeyFromObject(gw), - SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), - Conditions: conditions.NewDefaultRouteConditions(), - }, - { - GatewayNsName: client.ObjectKeyFromObject(gw), - SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - invalidCondition, - ), - }, - }, - }, - }, - }, - name: "normal case", + Valid: true, }, - { - graph: &graph.Graph{ - GatewayClass: nil, - Gateway: &graph.Gateway{ - Source: gw, - Listeners: listeners, - }, - IgnoredGateways: map[types.NamespacedName]*v1beta1.Gateway{ - {Namespace: "test", Name: "ignored-gateway"}: ignoredGw, - }, - Routes: routes, - }, - expected: Statuses{ - GatewayClassStatus: nil, - GatewayStatus: &GatewayStatus{ - NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, - ListenerStatuses: map[string]ListenerStatus{ - "listener-80-1": { - AttachedRoutes: 1, - Conditions: append( - conditions.NewDefaultListenerConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), - }, - }, - ObservedGeneration: 2, - }, - IgnoredGatewayStatuses: map[types.NamespacedName]IgnoredGatewayStatus{ - {Namespace: "test", Name: "ignored-gateway"}: {ObservedGeneration: 1}, - }, - HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ - {Namespace: "test", Name: "hr-1"}: { - ObservedGeneration: 3, - ParentStatuses: []ParentStatus{ - { - GatewayNsName: client.ObjectKeyFromObject(gw), - SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), - }, - { - GatewayNsName: client.ObjectKeyFromObject(gw), - SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - invalidCondition, - ), - }, - }, + Gateway: &graph.Gateway{ + Source: gw, + Listeners: map[string]*graph.Listener{ + "listener-80-1": { + Valid: true, + Routes: map[types.NamespacedName]*graph.Route{ + {Namespace: "test", Name: "hr-1"}: {}, }, }, }, - name: "gatewayclass doesn't exist", }, - { - graph: &graph.Graph{ - GatewayClass: &graph.GatewayClass{ - Source: &v1beta1.GatewayClass{ - ObjectMeta: metav1.ObjectMeta{Generation: 1}, - }, - Valid: false, - Conditions: []conditions.Condition{ - conditions.NewGatewayClassInvalidParameters("error"), - }, - }, - Gateway: &graph.Gateway{ - Source: gw, - Listeners: listeners, - }, - IgnoredGateways: map[types.NamespacedName]*v1beta1.Gateway{ - {Namespace: "test", Name: "ignored-gateway"}: ignoredGw, + IgnoredGateways: map[types.NamespacedName]*v1beta1.Gateway{ + client.ObjectKeyFromObject(ignoredGw): ignoredGw, + }, + Routes: routes, + } + + expected := Statuses{ + GatewayClassStatus: &GatewayClassStatus{ + ObservedGeneration: 1, + Conditions: conditions.NewDefaultGatewayClassConditions(), + }, + GatewayStatus: &GatewayStatus{ + NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + ListenerStatuses: map[string]ListenerStatus{ + "listener-80-1": { + AttachedRoutes: 1, + Conditions: conditions.NewDefaultListenerConditions(), }, - Routes: routes, }, - expected: Statuses{ - GatewayClassStatus: &GatewayClassStatus{ - ObservedGeneration: 1, - Conditions: []conditions.Condition{ - conditions.NewGatewayClassInvalidParameters("error"), - }, - }, - GatewayStatus: &GatewayStatus{ - NsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, - ListenerStatuses: map[string]ListenerStatus{ - "listener-80-1": { - AttachedRoutes: 1, - Conditions: append( - conditions.NewDefaultListenerConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), - }, + ObservedGeneration: 2, + }, + IgnoredGatewayStatuses: map[types.NamespacedName]IgnoredGatewayStatus{ + {Namespace: "test", Name: "ignored-gateway"}: {ObservedGeneration: 1}, + }, + HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ + {Namespace: "test", Name: "hr-1"}: { + ObservedGeneration: 3, + ParentStatuses: []ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - ObservedGeneration: 2, - }, - IgnoredGatewayStatuses: map[types.NamespacedName]IgnoredGatewayStatus{ - {Namespace: "test", Name: "ignored-gateway"}: {ObservedGeneration: 1}, - }, - HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ - {Namespace: "test", Name: "hr-1"}: { - ObservedGeneration: 3, - ParentStatuses: []ParentStatus{ - { - GatewayNsName: client.ObjectKeyFromObject(gw), - SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - ), - }, - { - GatewayNsName: client.ObjectKeyFromObject(gw), - SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), - Conditions: append( - conditions.NewDefaultRouteConditions(), - conditions.NewTODO("GatewayClass is invalid or doesn't exist"), - invalidCondition, - ), - }, - }, + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), + Conditions: append( + conditions.NewDefaultRouteConditions(), + invalidCondition, + ), }, }, }, - name: "gatewayclass is not valid", }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) + g := NewGomegaWithT(t) - result := buildStatuses(test.graph) - g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) - }) - } + result := buildStatuses(graph) + g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } From 061920cd2e6f8fd4d69d4d9cbd148206002711c5 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Mon, 17 Apr 2023 15:44:59 -0400 Subject: [PATCH 3/3] Introduce gateway_listener.go --- internal/state/graph/gateway.go | 342 ----------------- internal/state/graph/gateway_listener.go | 351 ++++++++++++++++++ internal/state/graph/gateway_listener_test.go | 246 ++++++++++++ internal/state/graph/gateway_test.go | 235 ------------ 4 files changed, 597 insertions(+), 577 deletions(-) create mode 100644 internal/state/graph/gateway_listener.go create mode 100644 internal/state/graph/gateway_listener_test.go diff --git a/internal/state/graph/gateway.go b/internal/state/graph/gateway.go index 6ca5d5c16b..90a47fb3e7 100644 --- a/internal/state/graph/gateway.go +++ b/internal/state/graph/gateway.go @@ -1,16 +1,13 @@ package graph import ( - "fmt" "sort" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1beta1" nkgsort "github.com/nginxinc/nginx-kubernetes-gateway/internal/sort" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" ) @@ -22,26 +19,6 @@ type Gateway struct { Listeners map[string]*Listener } -// Listener represents a Listener of the Gateway resource. -// FIXME(pleshakov) For now, we only support HTTP and HTTPS listeners. -type Listener struct { - // Source holds the source of the Listener from the Gateway resource. - Source v1beta1.Listener - // Routes holds the routes attached to the Listener. - // Only valid routes are attached. - Routes map[types.NamespacedName]*Route - // AcceptedHostnames is an intersection between the hostnames supported by the Listener and the hostnames - // from the attached routes. - 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 -} - // processedGateways holds the resources that belong to NKG. type processedGateways struct { Winner *v1beta1.Gateway @@ -117,322 +94,3 @@ func buildGateway(gw *v1beta1.Gateway, secretMemoryMgr secrets.SecretDiskMemoryM Listeners: buildListeners(gw, secretMemoryMgr, gc), } } - -func buildListeners( - gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, - gc *GatewayClass, -) map[string]*Listener { - listeners := make(map[string]*Listener) - - listenerFactory := newListenerConfiguratorFactory(gw, secretMemoryMgr, gc) - - for _, gl := range gw.Spec.Listeners { - configurator := listenerFactory.getConfiguratorForListener(gl) - listeners[string(gl.Name)] = configurator.configure(gl) - } - - return listeners -} - -type listenerConfiguratorFactory struct { - http, https, unsupportedProtocol *listenerConfigurator -} - -func (f *listenerConfiguratorFactory) getConfiguratorForListener(l v1beta1.Listener) *listenerConfigurator { - switch l.Protocol { - case v1beta1.HTTPProtocolType: - return f.http - case v1beta1.HTTPSProtocolType: - return f.https - default: - return f.unsupportedProtocol - } -} - -func newListenerConfiguratorFactory( - gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, - gc *GatewayClass, -) *listenerConfiguratorFactory { - return &listenerConfiguratorFactory{ - unsupportedProtocol: &listenerConfigurator{ - validators: []listenerValidator{ - func(listener v1beta1.Listener) []conditions.Condition { - valErr := field.NotSupported( - field.NewPath("protocol"), - listener.Protocol, - []string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)}, - ) - return []conditions.Condition{conditions.NewListenerUnsupportedProtocol(valErr.Error())} - }, - }, - }, - http: &listenerConfigurator{ - validators: []listenerValidator{ - validateListenerHostname, - createAddressesValidator(gw), - createNoValidGatewayClassValidator(gc), - validateHTTPListener, - }, - conflictResolvers: []listenerConflictResolver{ - createHostnameConflictResolver(), - }, - }, - https: &listenerConfigurator{ - validators: []listenerValidator{ - validateListenerHostname, - createAddressesValidator(gw), - createNoValidGatewayClassValidator(gc), - createHTTPSListenerValidator(gw.Namespace), - }, - conflictResolvers: []listenerConflictResolver{ - createHostnameConflictResolver(), - }, - externalReferenceResolvers: []listenerExternalReferenceResolver{ - createExternalReferencesForTLSSecretsResolver(gw.Namespace, secretMemoryMgr), - }, - }, - } -} - -// listenerValidator validates a listener. If the listener is invalid, the validator will return appropriate conditions. -type listenerValidator func(v1beta1.Listener) []conditions.Condition - -// listenerConflictResolver resolves conflicts between listeners. In case of a conflict, the resolver will make -// the conflicting listeners invalid - i.e. it will modify the passed listener and the previously processed conflicting -// listener. It will also add appropriate conditions to the listeners. -type listenerConflictResolver func(listener *Listener) - -// listenerExternalReferenceResolver resolves external references for a listener. If the reference is not resolvable, -// the resolver will make the listener invalid and add appropriate conditions. -type listenerExternalReferenceResolver func(listener *Listener) - -// listenerConfigurator is responsible for configuring a listener. -// validators, conflictResolvers, externalReferenceResolvers generate conditions for invalid fields of the listener. -// Because the Gateway status includes a status field for each listener, the messages in those conditions -// don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include -// a path starting from the field of a listener (e.g. "hostname", "tls.options"). -type listenerConfigurator struct { - // validators must not depend on the order of execution. - validators []listenerValidator - - // conflictResolvers can depend on validators - they will only be executed if all validators pass. - conflictResolvers []listenerConflictResolver - // externalReferenceResolvers can depend on validators - they will only be executed if all validators pass. - externalReferenceResolvers []listenerExternalReferenceResolver -} - -func (c *listenerConfigurator) configure(listener v1beta1.Listener) *Listener { - var conds []conditions.Condition - - // validators might return different conditions, so we run them all. - for _, validator := range c.validators { - conds = append(conds, validator(listener)...) - } - - if len(conds) > 0 { - return &Listener{ - Source: listener, - Conditions: conds, - Valid: false, - } - } - - l := &Listener{ - Source: listener, - Routes: make(map[types.NamespacedName]*Route), - AcceptedHostnames: make(map[string]struct{}), - Valid: true, - } - - // resolvers might add different conditions to the listener, so we run them all. - - for _, resolver := range c.conflictResolvers { - resolver(l) - } - - for _, resolver := range c.externalReferenceResolvers { - resolver(l) - } - - return l -} - -func validateListenerHostname(listener v1beta1.Listener) []conditions.Condition { - if listener.Hostname == nil { - return nil - } - - h := string(*listener.Hostname) - - if h == "" { - return nil - } - - err := validateHostname(h) - if err != nil { - path := field.NewPath("hostname") - valErr := field.Invalid(path, listener.Hostname, err.Error()) - return []conditions.Condition{conditions.NewListenerUnsupportedValue(valErr.Error())} - } - return nil -} - -func createAddressesValidator(gw *v1beta1.Gateway) listenerValidator { - return func(listener v1beta1.Listener) []conditions.Condition { - if len(gw.Spec.Addresses) > 0 { - path := field.NewPath("spec", "addresses") - valErr := field.Forbidden(path, "addresses are not supported") - return []conditions.Condition{conditions.NewListenerUnsupportedAddress(valErr.Error())} - } - return nil - } -} - -func createNoValidGatewayClassValidator(gc *GatewayClass) listenerValidator { - return func(listener v1beta1.Listener) []conditions.Condition { - if gc == nil { - return []conditions.Condition{conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist")} - } - - if !gc.Valid { - return []conditions.Condition{conditions.NewListenerNoValidGatewayClass("GatewayClass is invalid")} - } - - return nil - } -} - -func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { - if listener.Port != 80 { - path := field.NewPath("port") - valErr := field.NotSupported(path, listener.Port, []string{"80"}) - return []conditions.Condition{conditions.NewListenerPortUnavailable(valErr.Error())} - } - - if listener.TLS != nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) - } - - return nil -} - -func createHTTPSListenerValidator(gwNsName string) listenerValidator { - return func(listener v1beta1.Listener) []conditions.Condition { - var conds []conditions.Condition - - if listener.Port != 443 { - path := field.NewPath("port") - valErr := field.NotSupported(path, listener.Port, []string{"443"}) - conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) - } - - if listener.TLS == nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is nil for HTTPS listener %q", listener.Name)) - } - - tlsPath := field.NewPath("tls") - - if *listener.TLS.Mode != v1beta1.TLSModeTerminate { - valErr := field.NotSupported( - tlsPath.Child("mode"), - *listener.TLS.Mode, - []string{string(v1beta1.TLSModeTerminate)}, - ) - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) - } - - if len(listener.TLS.Options) > 0 { - path := tlsPath.Child("options") - valErr := field.Forbidden(path, "options are not supported") - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) - } - - if len(listener.TLS.CertificateRefs) == 0 { - panicForBrokenWebhookAssumption(fmt.Errorf("zero certificateRefs for HTTPS listener %q", listener.Name)) - } - - certRef := listener.TLS.CertificateRefs[0] - - certRefPath := tlsPath.Child("certificateRefs").Index(0) - - if certRef.Kind != nil && *certRef.Kind != "Secret" { - path := certRefPath.Child("kind") - valErr := field.NotSupported(path, *certRef.Kind, []string{"Secret"}) - conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - } - - // for Kind Secret, certRef.Group must be nil or empty - if certRef.Group != nil && *certRef.Group != "" { - path := certRefPath.Child("group") - valErr := field.NotSupported(path, *certRef.Group, []string{""}) - conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - } - - // secret must be in the same namespace as the gateway - if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName { - const detail = "Referenced Secret must belong to the same namespace as the Gateway" - path := certRefPath.Child("namespace") - valErr := field.Invalid(path, certRef.Namespace, detail) - conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - } - - if l := len(listener.TLS.CertificateRefs); l > 1 { - path := tlsPath.Child("certificateRefs") - valErr := field.TooMany(path, l, 1) - conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) - } - - return conds - } -} - -func createHostnameConflictResolver() listenerConflictResolver { - usedHostnames := make(map[string]*Listener) - - return func(l *Listener) { - h := getHostname(l.Source.Hostname) - - if holder, exist := usedHostnames[h]; exist { - l.Valid = false - - holder.Valid = false // all listeners for the same hostname become conflicted - - 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...) - - return - } - - usedHostnames[h] = l - } -} - -func createExternalReferencesForTLSSecretsResolver( - gwNs string, - secretMemoryMgr secrets.SecretDiskMemoryManager, -) listenerExternalReferenceResolver { - return func(l *Listener) { - nsname := types.NamespacedName{ - Namespace: gwNs, - Name: string(l.Source.TLS.CertificateRefs[0].Name), - } - - var err error - - l.SecretPath, err = secretMemoryMgr.Request(nsname) - if err != nil { - path := field.NewPath("tls", "certificateRefs").Index(0) - // field.NotFound could be better, but it doesn't allow us to set the error message. - valErr := field.Invalid(path, nsname, err.Error()) - - l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) - l.Valid = false - } - } -} diff --git a/internal/state/graph/gateway_listener.go b/internal/state/graph/gateway_listener.go new file mode 100644 index 0000000000..f55da62768 --- /dev/null +++ b/internal/state/graph/gateway_listener.go @@ -0,0 +1,351 @@ +package graph + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets" +) + +// Listener represents a Listener of the Gateway resource. +// FIXME(pleshakov) For now, we only support HTTP and HTTPS listeners. +type Listener struct { + // Source holds the source of the Listener from the Gateway resource. + Source v1beta1.Listener + // Routes holds the routes attached to the Listener. + // Only valid routes are attached. + Routes map[types.NamespacedName]*Route + // AcceptedHostnames is an intersection between the hostnames supported by the Listener and the hostnames + // from the attached routes. + 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 +} + +func buildListeners( + gw *v1beta1.Gateway, + secretMemoryMgr secrets.SecretDiskMemoryManager, + gc *GatewayClass, +) map[string]*Listener { + listeners := make(map[string]*Listener) + + listenerFactory := newListenerConfiguratorFactory(gw, secretMemoryMgr, gc) + + for _, gl := range gw.Spec.Listeners { + configurator := listenerFactory.getConfiguratorForListener(gl) + listeners[string(gl.Name)] = configurator.configure(gl) + } + + return listeners +} + +type listenerConfiguratorFactory struct { + http, https, unsupportedProtocol *listenerConfigurator +} + +func (f *listenerConfiguratorFactory) getConfiguratorForListener(l v1beta1.Listener) *listenerConfigurator { + switch l.Protocol { + case v1beta1.HTTPProtocolType: + return f.http + case v1beta1.HTTPSProtocolType: + return f.https + default: + return f.unsupportedProtocol + } +} + +func newListenerConfiguratorFactory( + gw *v1beta1.Gateway, + secretMemoryMgr secrets.SecretDiskMemoryManager, + gc *GatewayClass, +) *listenerConfiguratorFactory { + return &listenerConfiguratorFactory{ + unsupportedProtocol: &listenerConfigurator{ + validators: []listenerValidator{ + func(listener v1beta1.Listener) []conditions.Condition { + valErr := field.NotSupported( + field.NewPath("protocol"), + listener.Protocol, + []string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)}, + ) + return []conditions.Condition{conditions.NewListenerUnsupportedProtocol(valErr.Error())} + }, + }, + }, + http: &listenerConfigurator{ + validators: []listenerValidator{ + validateListenerHostname, + createAddressesValidator(gw), + createNoValidGatewayClassValidator(gc), + validateHTTPListener, + }, + conflictResolvers: []listenerConflictResolver{ + createHostnameConflictResolver(), + }, + }, + https: &listenerConfigurator{ + validators: []listenerValidator{ + validateListenerHostname, + createAddressesValidator(gw), + createNoValidGatewayClassValidator(gc), + createHTTPSListenerValidator(gw.Namespace), + }, + conflictResolvers: []listenerConflictResolver{ + createHostnameConflictResolver(), + }, + externalReferenceResolvers: []listenerExternalReferenceResolver{ + createExternalReferencesForTLSSecretsResolver(gw.Namespace, secretMemoryMgr), + }, + }, + } +} + +// listenerValidator validates a listener. If the listener is invalid, the validator will return appropriate conditions. +type listenerValidator func(v1beta1.Listener) []conditions.Condition + +// listenerConflictResolver resolves conflicts between listeners. In case of a conflict, the resolver will make +// the conflicting listeners invalid - i.e. it will modify the passed listener and the previously processed conflicting +// listener. It will also add appropriate conditions to the listeners. +type listenerConflictResolver func(listener *Listener) + +// listenerExternalReferenceResolver resolves external references for a listener. If the reference is not resolvable, +// the resolver will make the listener invalid and add appropriate conditions. +type listenerExternalReferenceResolver func(listener *Listener) + +// listenerConfigurator is responsible for configuring a listener. +// validators, conflictResolvers, externalReferenceResolvers generate conditions for invalid fields of the listener. +// Because the Gateway status includes a status field for each listener, the messages in those conditions +// don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include +// a path starting from the field of a listener (e.g. "hostname", "tls.options"). +type listenerConfigurator struct { + // validators must not depend on the order of execution. + validators []listenerValidator + + // conflictResolvers can depend on validators - they will only be executed if all validators pass. + conflictResolvers []listenerConflictResolver + // externalReferenceResolvers can depend on validators - they will only be executed if all validators pass. + externalReferenceResolvers []listenerExternalReferenceResolver +} + +func (c *listenerConfigurator) configure(listener v1beta1.Listener) *Listener { + var conds []conditions.Condition + + // validators might return different conditions, so we run them all. + for _, validator := range c.validators { + conds = append(conds, validator(listener)...) + } + + if len(conds) > 0 { + return &Listener{ + Source: listener, + Conditions: conds, + Valid: false, + } + } + + l := &Listener{ + Source: listener, + Routes: make(map[types.NamespacedName]*Route), + AcceptedHostnames: make(map[string]struct{}), + Valid: true, + } + + // resolvers might add different conditions to the listener, so we run them all. + + for _, resolver := range c.conflictResolvers { + resolver(l) + } + + for _, resolver := range c.externalReferenceResolvers { + resolver(l) + } + + return l +} + +func validateListenerHostname(listener v1beta1.Listener) []conditions.Condition { + if listener.Hostname == nil { + return nil + } + + h := string(*listener.Hostname) + + if h == "" { + return nil + } + + err := validateHostname(h) + if err != nil { + path := field.NewPath("hostname") + valErr := field.Invalid(path, listener.Hostname, err.Error()) + return []conditions.Condition{conditions.NewListenerUnsupportedValue(valErr.Error())} + } + return nil +} + +func createAddressesValidator(gw *v1beta1.Gateway) listenerValidator { + return func(listener v1beta1.Listener) []conditions.Condition { + if len(gw.Spec.Addresses) > 0 { + path := field.NewPath("spec", "addresses") + valErr := field.Forbidden(path, "addresses are not supported") + return []conditions.Condition{conditions.NewListenerUnsupportedAddress(valErr.Error())} + } + return nil + } +} + +func createNoValidGatewayClassValidator(gc *GatewayClass) listenerValidator { + return func(listener v1beta1.Listener) []conditions.Condition { + if gc == nil { + return []conditions.Condition{conditions.NewListenerNoValidGatewayClass("GatewayClass doesn't exist")} + } + + if !gc.Valid { + return []conditions.Condition{conditions.NewListenerNoValidGatewayClass("GatewayClass is invalid")} + } + + return nil + } +} + +func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { + if listener.Port != 80 { + path := field.NewPath("port") + valErr := field.NotSupported(path, listener.Port, []string{"80"}) + return []conditions.Condition{conditions.NewListenerPortUnavailable(valErr.Error())} + } + + if listener.TLS != nil { + panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) + } + + return nil +} + +func createHTTPSListenerValidator(gwNsName string) listenerValidator { + return func(listener v1beta1.Listener) []conditions.Condition { + var conds []conditions.Condition + + if listener.Port != 443 { + path := field.NewPath("port") + valErr := field.NotSupported(path, listener.Port, []string{"443"}) + conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) + } + + if listener.TLS == nil { + panicForBrokenWebhookAssumption(fmt.Errorf("tls is nil for HTTPS listener %q", listener.Name)) + } + + tlsPath := field.NewPath("tls") + + if *listener.TLS.Mode != v1beta1.TLSModeTerminate { + valErr := field.NotSupported( + tlsPath.Child("mode"), + *listener.TLS.Mode, + []string{string(v1beta1.TLSModeTerminate)}, + ) + conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + } + + if len(listener.TLS.Options) > 0 { + path := tlsPath.Child("options") + valErr := field.Forbidden(path, "options are not supported") + conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + } + + if len(listener.TLS.CertificateRefs) == 0 { + panicForBrokenWebhookAssumption(fmt.Errorf("zero certificateRefs for HTTPS listener %q", listener.Name)) + } + + certRef := listener.TLS.CertificateRefs[0] + + certRefPath := tlsPath.Child("certificateRefs").Index(0) + + if certRef.Kind != nil && *certRef.Kind != "Secret" { + path := certRefPath.Child("kind") + valErr := field.NotSupported(path, *certRef.Kind, []string{"Secret"}) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + } + + // for Kind Secret, certRef.Group must be nil or empty + if certRef.Group != nil && *certRef.Group != "" { + path := certRefPath.Child("group") + valErr := field.NotSupported(path, *certRef.Group, []string{""}) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + } + + // secret must be in the same namespace as the gateway + if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName { + const detail = "Referenced Secret must belong to the same namespace as the Gateway" + path := certRefPath.Child("namespace") + valErr := field.Invalid(path, certRef.Namespace, detail) + conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + } + + if l := len(listener.TLS.CertificateRefs); l > 1 { + path := tlsPath.Child("certificateRefs") + valErr := field.TooMany(path, l, 1) + conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) + } + + return conds + } +} + +func createHostnameConflictResolver() listenerConflictResolver { + usedHostnames := make(map[string]*Listener) + + return func(l *Listener) { + h := getHostname(l.Source.Hostname) + + if holder, exist := usedHostnames[h]; exist { + l.Valid = false + + holder.Valid = false // all listeners for the same hostname become conflicted + + 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...) + + return + } + + usedHostnames[h] = l + } +} + +func createExternalReferencesForTLSSecretsResolver( + gwNs string, + secretMemoryMgr secrets.SecretDiskMemoryManager, +) listenerExternalReferenceResolver { + return func(l *Listener) { + nsname := types.NamespacedName{ + Namespace: gwNs, + Name: string(l.Source.TLS.CertificateRefs[0].Name), + } + + var err error + + l.SecretPath, err = secretMemoryMgr.Request(nsname) + if err != nil { + path := field.NewPath("tls", "certificateRefs").Index(0) + // field.NotFound could be better, but it doesn't allow us to set the error message. + valErr := field.Invalid(path, nsname, err.Error()) + + l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...) + l.Valid = false + } + } +} diff --git a/internal/state/graph/gateway_listener_test.go b/internal/state/graph/gateway_listener_test.go new file mode 100644 index 0000000000..553bf5769a --- /dev/null +++ b/internal/state/graph/gateway_listener_test.go @@ -0,0 +1,246 @@ +package graph + +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 + name string + expected []conditions.Condition + }{ + { + l: v1beta1.Listener{ + Port: 80, + }, + expected: nil, + name: "valid", + }, + { + l: v1beta1.Listener{ + Port: 81, + }, + expected: []conditions.Condition{ + conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`), + }, + name: "invalid port", + }, + } + + for _, test := range tests { + 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{ + Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), + Name: "secret", + Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer(gwNs)), + } + + 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{ + Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), + Name: "secret", + Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("diff-ns")), + } + + tests := []struct { + l v1beta1.Listener + name string + expected []conditions.Condition + }{ + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, + }, + }, + expected: nil, + name: "valid", + }, + { + l: v1beta1.Listener{ + Port: 80, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, + }, + }, + expected: []conditions.Condition{ + conditions.NewListenerPortUnavailable(`port: Unsupported value: 80: supported values: "443"`), + }, + name: "invalid port", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, + Options: map[v1beta1.AnnotationKey]v1beta1.AnnotationValue{"key": "val"}, + }, + }, + expected: []conditions.Condition{ + conditions.NewListenerUnsupportedValue("tls.options: Forbidden: options are not supported"), + }, + name: "invalid options", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModePassthrough), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, + }, + }, + expected: []conditions.Condition{ + conditions.NewListenerUnsupportedValue( + `tls.mode: Unsupported value: "Passthrough": supported values: "Terminate"`, + ), + }, + name: "invalid tls mode", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefGroup}, + }, + }, + expected: conditions.NewListenerInvalidCertificateRef( + `tls.certificateRefs[0].group: Unsupported value: "some-group": supported values: ""`, + ), + name: "invalid cert ref group", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefKind}, + }, + }, + expected: conditions.NewListenerInvalidCertificateRef( + `tls.certificateRefs[0].kind: Unsupported value: "ConfigMap": supported values: "Secret"`, + ), + name: "invalid cert ref kind", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefTNamespace}, + }, + }, + expected: conditions.NewListenerInvalidCertificateRef( + `tls.certificateRefs[0].namespace: Invalid value: "diff-ns": Referenced Secret must belong to ` + + `the same namespace as the Gateway`, + ), + name: "invalid cert ref namespace", + }, + { + l: v1beta1.Listener{ + Port: 443, + TLS: &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef, validSecretRef}, + }, + }, + expected: []conditions.Condition{ + conditions.NewListenerUnsupportedValue("tls.certificateRefs: Too many: 2: must have at most 1 items"), + }, + name: "too many cert refs", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + v := createHTTPSListenerValidator(gwNs) + + result := v(test.l) + g.Expect(result).To(Equal(test.expected)) + }) + } +} + +func TestValidateListenerHostname(t *testing.T) { + tests := []struct { + hostname *v1beta1.Hostname + name string + expectErr bool + }{ + { + hostname: nil, + expectErr: false, + name: "nil hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("")), + expectErr: false, + name: "empty hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), + expectErr: false, + name: "valid hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")), + expectErr: true, + name: "wildcard hostname", + }, + { + hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")), + expectErr: true, + name: "invalid hostname", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + conds := validateListenerHostname(v1beta1.Listener{Hostname: test.hostname}) + + if test.expectErr { + g.Expect(conds).ToNot(BeEmpty()) + } else { + g.Expect(conds).To(BeEmpty()) + } + }) + } +} diff --git a/internal/state/graph/gateway_test.go b/internal/state/graph/gateway_test.go index a5f18a27c9..75f9232d03 100644 --- a/internal/state/graph/gateway_test.go +++ b/internal/state/graph/gateway_test.go @@ -591,238 +591,3 @@ func TestBuildGateway(t *testing.T) { }) } } - -func TestValidateHTTPListener(t *testing.T) { - tests := []struct { - l v1beta1.Listener - name string - expected []conditions.Condition - }{ - { - l: v1beta1.Listener{ - Port: 80, - }, - expected: nil, - name: "valid", - }, - { - l: v1beta1.Listener{ - Port: 81, - }, - expected: []conditions.Condition{ - conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`), - }, - name: "invalid port", - }, - } - - for _, test := range tests { - 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{ - Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), - Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer(gwNs)), - } - - 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{ - Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), - Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("diff-ns")), - } - - tests := []struct { - l v1beta1.Listener - name string - expected []conditions.Condition - }{ - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, - }, - }, - expected: nil, - name: "valid", - }, - { - l: v1beta1.Listener{ - Port: 80, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, - }, - }, - expected: []conditions.Condition{ - conditions.NewListenerPortUnavailable(`port: Unsupported value: 80: supported values: "443"`), - }, - name: "invalid port", - }, - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, - Options: map[v1beta1.AnnotationKey]v1beta1.AnnotationValue{"key": "val"}, - }, - }, - expected: []conditions.Condition{ - conditions.NewListenerUnsupportedValue("tls.options: Forbidden: options are not supported"), - }, - name: "invalid options", - }, - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModePassthrough), - CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef}, - }, - }, - expected: []conditions.Condition{ - conditions.NewListenerUnsupportedValue( - `tls.mode: Unsupported value: "Passthrough": supported values: "Terminate"`, - ), - }, - name: "invalid tls mode", - }, - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefGroup}, - }, - }, - expected: conditions.NewListenerInvalidCertificateRef( - `tls.certificateRefs[0].group: Unsupported value: "some-group": supported values: ""`, - ), - name: "invalid cert ref group", - }, - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefKind}, - }, - }, - expected: conditions.NewListenerInvalidCertificateRef( - `tls.certificateRefs[0].kind: Unsupported value: "ConfigMap": supported values: "Secret"`, - ), - name: "invalid cert ref kind", - }, - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefTNamespace}, - }, - }, - expected: conditions.NewListenerInvalidCertificateRef( - `tls.certificateRefs[0].namespace: Invalid value: "diff-ns": Referenced Secret must belong to ` + - `the same namespace as the Gateway`, - ), - name: "invalid cert ref namespace", - }, - { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef, validSecretRef}, - }, - }, - expected: []conditions.Condition{ - conditions.NewListenerUnsupportedValue("tls.certificateRefs: Too many: 2: must have at most 1 items"), - }, - name: "too many cert refs", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) - - v := createHTTPSListenerValidator(gwNs) - - result := v(test.l) - g.Expect(result).To(Equal(test.expected)) - }) - } -} - -func TestValidateListenerHostname(t *testing.T) { - tests := []struct { - hostname *v1beta1.Hostname - name string - expectErr bool - }{ - { - hostname: nil, - expectErr: false, - name: "nil hostname", - }, - { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("")), - expectErr: false, - name: "empty hostname", - }, - { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")), - expectErr: false, - name: "valid hostname", - }, - { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")), - expectErr: true, - name: "wildcard hostname", - }, - { - hostname: (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")), - expectErr: true, - name: "invalid hostname", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) - - conds := validateListenerHostname(v1beta1.Listener{Hostname: test.hostname}) - - if test.expectErr { - g.Expect(conds).ToNot(BeEmpty()) - } else { - g.Expect(conds).To(BeEmpty()) - } - }) - } -}