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 b8ff28a83c..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 @@ -107,305 +84,13 @@ 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), - } -} - -func buildListeners( - gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, -) map[string]*Listener { - listeners := make(map[string]*Listener) - - listenerFactory := newListenerConfiguratorFactory(gw, secretMemoryMgr) - - for _, gl := range gw.Spec.Listeners { - configurator := listenerFactory.getConfiguratorForListener(gl) - listeners[string(gl.Name)] = configurator.configure(gl) - } - - return listeners -} - -type listenerConfigurator interface { - configure(listener v1beta1.Listener) *Listener -} - -type listenerConfiguratorFactory struct { - https *httpListenerConfigurator - http *httpListenerConfigurator -} - -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() - } -} - -func newListenerConfiguratorFactory( - gw *v1beta1.Gateway, - secretMemoryMgr secrets.SecretDiskMemoryManager, -) *listenerConfiguratorFactory { - return &listenerConfiguratorFactory{ - https: newHTTPSListenerConfigurator(gw, secretMemoryMgr), - http: newHTTPListenerConfigurator(gw), - } -} - -type httpListenerConfigurator struct { - gateway *v1beta1.Gateway - secretMemoryMgr secrets.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 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) - }, - } -} - -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())) - } - - 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())) - } - - 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 - - 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 - } - - c.usedHostnames[h] = l -} - -func (c *httpListenerConfigurator) loadSecretIntoListener(l *Listener) { - if !l.Valid { - 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 { - 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 - } -} - -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, - } - - if validHostname { - c.ensureUniqueHostnamesAmongListeners(l) - } - - if gl.Protocol == v1beta1.HTTPSProtocolType { - c.loadSecretIntoListener(l) - } - - return l -} - -type invalidProtocolListenerConfigurator struct{} - -func newInvalidProtocolListenerConfigurator() *invalidProtocolListenerConfigurator { - return &invalidProtocolListenerConfigurator{} -} - -func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *Listener { - valErr := field.NotSupported( - field.NewPath("protocol"), - gl.Protocol, - []string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)}, - ) - - return &Listener{ - Source: gl, - Valid: false, - Routes: make(map[types.NamespacedName]*Route), - AcceptedHostnames: make(map[string]struct{}), - Conditions: []conditions.Condition{ - conditions.NewListenerUnsupportedProtocol(valErr.Error()), - }, - } -} - -func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { - var conds []conditions.Condition - - if listener.Port != 80 { - path := field.NewPath("port") - valErr := field.NotSupported(path, listener.Port, []string{"80"}) - conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) - } - - if listener.TLS != nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) - } - - return conds -} - -func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []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)) + Listeners: buildListeners(gw, secretMemoryMgr, gc), } - - 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 validateListenerHostname(host *v1beta1.Hostname) error { - if host == nil { - return nil - } - - h := string(*host) - - if h == "" { - return nil - } - - return validateHostname(h) } 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 e824c5a18f..75f9232d03 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,15 +325,14 @@ 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{ "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"`, @@ -335,15 +344,14 @@ 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{ "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"`), }, @@ -353,15 +361,14 @@ 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{ "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"`), }, @@ -371,24 +378,21 @@ 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{ "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), }, @@ -398,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{ @@ -419,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{ @@ -456,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{ @@ -479,6 +486,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 +494,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", }, }, }, @@ -498,14 +507,13 @@ func TestBuildGateway(t *testing.T) { {}, }, }), + gatewayClass: validGC, expected: &Gateway{ 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 +521,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", @@ -533,254 +539,55 @@ func TestBuildGateway(t *testing.T) { expected: nil, name: "nil gateway", }, - } - - secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} - secretMemoryMgr.RequestCalls(func(nsname types.NamespacedName) (string, error) { - if (nsname == types.NamespacedName{Namespace: "test", Name: "secret"}) { - return secretPath, nil - } - return "", errors.New("secret not found") - }) - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) - result := buildGateway(test.gateway, secretMemoryMgr) - g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) - }) - } -} - -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}, + 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"), + }, + }, }, }, - 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", + name: "invalid gatewayclass", }, { - l: v1beta1.Listener{ - Port: 443, - TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), - CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef, validSecretRef}, + 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"), + }, + }, }, }, - expected: []conditions.Condition{ - conditions.NewListenerUnsupportedValue("tls.certificateRefs: Too many: 2: must have at most 1 items"), - }, - name: "too many cert refs", + name: "nil gatewayclass", }, } - for _, test := range tests { - 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 - 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", - }, - } + secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} + secretMemoryMgr.RequestCalls(func(nsname types.NamespacedName) (string, error) { + if (nsname == types.NamespacedName{Namespace: "test", Name: "secret"}) { + return secretPath, nil + } + return "", errors.New("secret not found") + }) for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - - err := validateListenerHostname(test.hostname) - - if test.expectErr { - g.Expect(err).ToNot(BeNil()) - } else { - g.Expect(err).To(BeNil()) - } + 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()) }