diff --git a/config/crd/bases/skupper_router_access_crd.yaml b/config/crd/bases/skupper_router_access_crd.yaml index 63e435b05..2966e4454 100644 --- a/config/crd/bases/skupper_router_access_crd.yaml +++ b/config/crd/bases/skupper_router_access_crd.yaml @@ -54,10 +54,16 @@ spec: Available access types and the default selection is configured on the Skupper controller for Kubernetes. - The options available by default are: - - `local`: No external ingress. Implies a Kubernetes Service with type CluterIP. + Additional options when enabled on the Skupper controller include: + - `local`: No external ingress. Implies a Kubernetes Service with type ClusterIP. - `route`: Exposed via an OpenShift Route. - `loadbalancer`: Exposed via a Kubernetes Service with type LoadBalancer. + - `ingress`: Exposed via a Kubernetes Ingress (no nginx-specific annotations). + - `ingress-nginx`: Exposed via Ingress with the NGINX Ingress Controller annotations. + + To select which IngressClass handles Skupper-managed Ingress resources, set + controller flag/env `SKUPPER_INGRESS_CLASS_NAME` or per-site + `spec.settings.ingressClassName` on this resource. type: string tlsCredentials: description: |- @@ -88,8 +94,12 @@ spec: Advanced. A map containing additional settings. Each map entry has a string name and a string value. - **Note:** In general, we recommend not changing `settings` - from their default values. + For access types `ingress` and `ingress-nginx`, the key + `ingressClassName` sets `spec.ingressClassName` on the managed + Ingress (overrides the controller-wide default). + + **Note:** In general, we recommend not changing `settings` + from their default values except when required. type: object additionalProperties: type: string diff --git a/config/crd/bases/skupper_site_crd.yaml b/config/crd/bases/skupper_site_crd.yaml index a48cd71f2..b0c2d4968 100644 --- a/config/crd/bases/skupper_site_crd.yaml +++ b/config/crd/bases/skupper_site_crd.yaml @@ -41,6 +41,7 @@ spec: - `default`: Use the default link access for the current platform. For OpenShift, the default is `route`. For other Kubernetes flavors, the default is `loadbalancer`. - `route`: Use an OpenShift route. - `loadbalancer`: Use a Kubernetes load balancer. + - `ingress` / `ingress-nginx`: When enabled on the controller, use a Kubernetes Ingress. Use `ingress` for generic controllers; `ingress-nginx` adds NGINX-specific TLS annotations. Set `ingressClassName` via RouterAccess `spec.settings` or controller `SKUPPER_INGRESS_CLASS_NAME`. type: string defaultIssuer: description: |- @@ -89,6 +90,7 @@ spec: - `disable-anti-affinity`: Set to "true" in order to prevent skupper from specifying router pod affinity. - `size`: The desired site sizing profile to use for constraining pod resources. Corresponds to a ConfigMap with matching `skupper.io/site-sizing` label. - `tls-prior-valid-revisions`: Set the number of revisions to TLS Secrets backing Site Link connections that are permissible to hold open to preserve established service connections. An unsigned integer defaults to 1. Set to 0 to immediately disrupt connections secured with old TLS configurations. + - `ingressClassName`: When using `ingress` or `ingress-nginx` link access, sets the Kubernetes IngressClass for Skupper-managed Ingress resources (also copied to RouterAccess). Overrides controller `SKUPPER_INGRESS_CLASS_NAME`. Use empty string to clear a previously set class. type: object additionalProperties: type: string diff --git a/internal/kube/securedaccess/access.go b/internal/kube/securedaccess/access.go index 422156f7d..96c31e91f 100644 --- a/internal/kube/securedaccess/access.go +++ b/internal/kube/securedaccess/access.go @@ -71,7 +71,9 @@ func NewSecuredAccessManager(clients internalclient.Clients, certMgr certificate } else if accessType == ACCESS_TYPE_LOADBALANCER { mgr.enabledAccessTypes[accessType] = newLoadbalancerAccess(mgr) } else if accessType == ACCESS_TYPE_INGRESS_NGINX { - mgr.enabledAccessTypes[accessType] = newIngressAccess(mgr, true, config.IngressDomain) + mgr.enabledAccessTypes[accessType] = newIngressAccess(mgr, true, config.IngressDomain, config.IngressClassName) + } else if accessType == ACCESS_TYPE_INGRESS { + mgr.enabledAccessTypes[accessType] = newIngressAccess(mgr, false, config.IngressDomain, config.IngressClassName) } else if accessType == ACCESS_TYPE_CONTOUR_HTTP_PROXY { mgr.enabledAccessTypes[accessType] = newContourHttpProxyAccess(mgr, config.HttpProxyDomain) } else if accessType == ACCESS_TYPE_GATEWAY { @@ -460,16 +462,20 @@ func (m *SecuredAccessManager) CheckTlsRoute(key string, o *unstructured.Unstruc return m.reconcile(sa) } +func isIngressBackedAccessType(accessType string) bool { + return accessType == ACCESS_TYPE_INGRESS_NGINX || accessType == ACCESS_TYPE_INGRESS +} + func (m *SecuredAccessManager) CheckIngress(key string, ingress *networkingv1.Ingress) error { sa, ok := m.definitions[key] if ingress == nil { delete(m.ingresses, key) - if !ok || m.actualAccessType(sa) != ACCESS_TYPE_INGRESS_NGINX { + if !ok || !isIngressBackedAccessType(m.actualAccessType(sa)) { return nil } } else { m.ingresses[key] = ingress - if !ok || m.actualAccessType(sa) != ACCESS_TYPE_INGRESS_NGINX { + if !ok || !isIngressBackedAccessType(m.actualAccessType(sa)) { if !canDelete(&ingress.ObjectMeta) { return nil } diff --git a/internal/kube/securedaccess/config.go b/internal/kube/securedaccess/config.go index 35e2018a4..ab9f3dfba 100644 --- a/internal/kube/securedaccess/config.go +++ b/internal/kube/securedaccess/config.go @@ -13,6 +13,8 @@ const ACCESS_TYPE_LOADBALANCER = "loadbalancer" const ACCESS_TYPE_ROUTE = "route" const ACCESS_TYPE_NODEPORT = "nodeport" const ACCESS_TYPE_INGRESS_NGINX = "ingress-nginx" +const ACCESS_TYPE_INGRESS = "ingress" +const SettingIngressClassName = "ingressClassName" const ACCESS_TYPE_CONTOUR_HTTP_PROXY = "contour-http-proxy" const ACCESS_TYPE_GATEWAY = "gateway" const ACCESS_TYPE_LOCAL = "local" @@ -22,6 +24,7 @@ type Config struct { DefaultAccessType string ClusterHost string IngressDomain string + IngressClassName string HttpProxyDomain string GatewayPort int GatewayClass string @@ -73,7 +76,8 @@ func BoundConfig(flags *flag.FlagSet) (*Config, error) { iflag.MultiStringVar(flags, &c.EnabledAccessTypes, "enabled-access-types", "SKUPPER_ENABLED_ACCESS_TYPES", defaultEnabledAccessTypes(), "The access types which should be enabled for sites to choose from.") iflag.StringVar(flags, &c.DefaultAccessType, "default-access-type", "SKUPPER_DEFAULT_ACCESS_TYPE", "", "The default access type.") iflag.StringVar(flags, &c.ClusterHost, "cluster-host", "SKUPPER_CLUSTER_HOST", "", "The hostname or IP address through which the cluster can be reached. Required for configuring nodeport as an access type.") - iflag.StringVar(flags, &c.IngressDomain, "ingress-domain", "SKUPPER_INGRESS_DOMAIN", "", "The domain to use in constructing the fully qualified hostname for Ingress resources, through which the ingress controller can be reached. Only used when selecting ingress-nginx as an access type.") + iflag.StringVar(flags, &c.IngressDomain, "ingress-domain", "SKUPPER_INGRESS_DOMAIN", "", "The domain to use in constructing the fully qualified hostname for Ingress resources, through which the ingress controller can be reached. Used for ingress and ingress-nginx access types.") + iflag.StringVar(flags, &c.IngressClassName, "ingress-class-name", "SKUPPER_INGRESS_CLASS_NAME", "", "Optional ingressClassName for Skupper-managed Ingress resources. Per-resource override: RouterAccess spec.settings."+SettingIngressClassName+". For ingress-nginx, defaults to \"nginx\" if unset.") iflag.StringVar(flags, &c.HttpProxyDomain, "http-proxy-domain", "SKUPPER_HTTP_PROXY_DOMAIN", "", "The domain to use in constructing the fully qualified hostname for contour HttpProxy resources, through which the contour controller can be reached. Only used when selecting contour-http-proxy as an access type.") iflag.StringVar(flags, &c.GatewayDomain, "gateway-domain", "SKUPPER_GATEWAY_DOMAIN", "", "The domain to use in constructing the fully qualified hostname for TLSRoutes resources. Only used when selecting gateway as an access type.") iflag.StringVar(flags, &c.GatewayClass, "gateway-class", "SKUPPER_GATEWAY_CLASS", "", "The class of Gateway to use. This is required to enable gateway as an access type.") diff --git a/internal/kube/securedaccess/config_test.go b/internal/kube/securedaccess/config_test.go index 4b7f33797..cfbccc878 100644 --- a/internal/kube/securedaccess/config_test.go +++ b/internal/kube/securedaccess/config_test.go @@ -33,6 +33,7 @@ func Test_BoundConfig(t *testing.T) { "SKUPPER_CLUSTER_HOST": "mycluster.org", "SKUPPER_ENABLED_ACCESS_TYPES": "nodeport,ingress-nginx", "SKUPPER_INGRESS_DOMAIN": "gateway.ingress.com", + "SKUPPER_INGRESS_CLASS_NAME": "public-ingress", "SKUPPER_HTTP_PROXY_DOMAIN": "gateway.contour.com", }, expectedValue: &Config{ @@ -43,6 +44,7 @@ func Test_BoundConfig(t *testing.T) { DefaultAccessType: "nodeport", ClusterHost: "mycluster.org", IngressDomain: "gateway.ingress.com", + IngressClassName: "public-ingress", HttpProxyDomain: "gateway.contour.com", GatewayPort: 8443, }, @@ -54,6 +56,7 @@ func Test_BoundConfig(t *testing.T) { "--default-access-type=ingress-nginx", "--cluster-host=foo.bar.com", "--ingress-domain=baz.com", + "--ingress-class-name=my-class", "--http-proxy-domain=bif.baf.bof.com", }, expectedValue: &Config{ @@ -64,6 +67,7 @@ func Test_BoundConfig(t *testing.T) { DefaultAccessType: "ingress-nginx", ClusterHost: "foo.bar.com", IngressDomain: "baz.com", + IngressClassName: "my-class", HttpProxyDomain: "bif.baf.bof.com", GatewayPort: 8443, }, diff --git a/internal/kube/securedaccess/ingress.go b/internal/kube/securedaccess/ingress.go index d7b360389..dfee6aa6d 100644 --- a/internal/kube/securedaccess/ingress.go +++ b/internal/kube/securedaccess/ingress.go @@ -15,26 +15,46 @@ import ( ) type IngressAccessType struct { - manager *SecuredAccessManager - nginx bool // if true add nginx class and nginx specific annotations - domain string - logger *slog.Logger + manager *SecuredAccessManager + nginx bool // if true add nginx-specific annotations and default class "nginx" + domain string + defaultIngressClass string // from SKUPPER_INGRESS_CLASS_NAME / --ingress-class-name + logger *slog.Logger } -func newIngressAccess(manager *SecuredAccessManager, nginx bool, domain string) AccessType { +func newIngressAccess(manager *SecuredAccessManager, nginx bool, domain string, defaultIngressClass string) AccessType { return &IngressAccessType{ - manager: manager, - nginx: nginx, - domain: domain, - logger: slog.New(slog.Default().Handler()).With(slog.String("component", "kube.securedaccess.ingress")), + manager: manager, + nginx: nginx, + domain: domain, + defaultIngressClass: strings.TrimSpace(defaultIngressClass), + logger: slog.New(slog.Default().Handler()).With(slog.String("component", "kube.securedaccess.ingress")), } } +// ingressClassNameForDesiredIngress returns the ingressClassName to set on the Ingress. +// spec.settings[ingressClassName] > operator default > (nginx only) "nginx". +func ingressClassNameForDesiredIngress(nginx bool, defaultClass string, settings map[string]string) string { + if settings != nil { + if v := strings.TrimSpace(settings[SettingIngressClassName]); v != "" { + return v + } + } + if v := strings.TrimSpace(defaultClass); v != "" { + return v + } + if nginx { + return "nginx" + } + return "" +} + func (o *IngressAccessType) RealiseAndResolve(access *skupperv2alpha1.SecuredAccess, svc *corev1.Service) ([]skupperv2alpha1.Endpoint, error) { desired := toIngress(qualify(access.Namespace, o.domain), access) - if o.nginx { - className := "nginx" + if className := ingressClassNameForDesiredIngress(o.nginx, o.defaultIngressClass, access.Spec.Settings); className != "" { desired.Spec.IngressClassName = &className + } + if o.nginx { addNginxIngressAnnotations(desired.ObjectMeta.Annotations) } ingress, qualified, err := o.ensureIngress(access.Namespace, desired) diff --git a/internal/kube/securedaccess/ingress_test.go b/internal/kube/securedaccess/ingress_test.go new file mode 100644 index 000000000..7d099542a --- /dev/null +++ b/internal/kube/securedaccess/ingress_test.go @@ -0,0 +1,63 @@ +package securedaccess + +import "testing" + +func TestIngressClassNameForDesiredIngress(t *testing.T) { + tests := []struct { + name string + nginx bool + defaultClass string + settings map[string]string + want string + }{ + { + name: "ingress-nginx implicit nginx", + nginx: true, + want: "nginx", + }, + { + name: "ingress-nginx global default", + nginx: true, + defaultClass: "openshift-public", + want: "openshift-public", + }, + { + name: "settings override global", + nginx: true, + defaultClass: "nginx", + settings: map[string]string{SettingIngressClassName: "public-ic"}, + want: "public-ic", + }, + { + name: "plain ingress no default", + nginx: false, + want: "", + }, + { + name: "plain ingress with global", + nginx: false, + defaultClass: "openshift-public", + want: "openshift-public", + }, + { + name: "plain ingress settings only", + nginx: false, + settings: map[string]string{SettingIngressClassName: "custom"}, + want: "custom", + }, + { + name: "trim spaces", + nginx: false, + settings: map[string]string{SettingIngressClassName: " x "}, + want: "x", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ingressClassNameForDesiredIngress(tt.nginx, tt.defaultClass, tt.settings) + if got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) + } +} diff --git a/internal/kube/site/site.go b/internal/kube/site/site.go index a5a431fbf..31c809ce2 100644 --- a/internal/kube/site/site.go +++ b/internal/kube/site/site.go @@ -13,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -277,6 +278,47 @@ func (s *Site) groups() []string { } } +// routerAccessSettingsMerge copies existing RouterAccess settings, then applies +// site.spec.settings.ingressClassName when that key is present (non-empty sets, +// empty string clears). Other RouterAccess-only settings are preserved. +func routerAccessSettingsMerge(site *skupperv2alpha1.Site, current *skupperv2alpha1.RouterAccess) map[string]string { + const ingressClassKey = "ingressClassName" + out := map[string]string{} + if current != nil && current.Spec.Settings != nil { + for k, v := range current.Spec.Settings { + out[k] = v + } + } + if site.Spec.Settings != nil { + if v, ok := site.Spec.Settings[ingressClassKey]; ok { + v = strings.TrimSpace(v) + if v != "" { + out[ingressClassKey] = v + } else { + delete(out, ingressClassKey) + } + } + } + if len(out) == 0 { + return nil + } + return out +} + +// routerAccessSpecEqual compares RouterAccess specs treating nil and empty Settings as equal. +// Kubernetes/client round-trips often produce map[string]string{} where we use nil, which would +// otherwise cause endless RouterAccess updates. +func routerAccessSpecEqual(a, b skupperv2alpha1.RouterAccessSpec) bool { + aCopy, bCopy := a, b + if len(aCopy.Settings) == 0 { + aCopy.Settings = nil + } + if len(bCopy.Settings) == 0 { + bCopy.Settings = nil + } + return reflect.DeepEqual(aCopy, bCopy) +} + func (s *Site) checkDefaultRouterAccess(ctxt context.Context, site *skupperv2alpha1.Site) error { if site.Spec.LinkAccess == "" || site.Spec.LinkAccess == "none" { return nil @@ -286,6 +328,7 @@ func (s *Site) checkDefaultRouterAccess(ctxt context.Context, site *skupperv2alp if site.Spec.LinkAccess == "default" { accessType = "" } + current, ok := s.linkAccess[name] desired := &skupperv2alpha1.RouterAccess{ TypeMeta: metav1.TypeMeta{ APIVersion: "skupper.io/v2alpha1", @@ -313,11 +356,11 @@ func (s *Site) checkDefaultRouterAccess(ctxt context.Context, site *skupperv2alp Port: 45671, }, }, + Settings: routerAccessSettingsMerge(site, current), }, } - current, ok := s.linkAccess[name] if ok { - if reflect.DeepEqual(current.Spec, desired.Spec) { + if routerAccessSpecEqual(current.Spec, desired.Spec) { return nil } current.Spec = desired.Spec @@ -1170,6 +1213,7 @@ func (s *Site) NetworkStatusUpdated(network []skupperv2alpha1.SiteRecord) error if s.site == nil || reflect.DeepEqual(s.site.Status.Network, network) { return nil } + prev := s.site.DeepCopy() s.site.Status.Network = network s.site.Status.SitesInNetwork = len(network) updated, err := s.UpdateSiteStatus(s.site) @@ -1177,6 +1221,29 @@ func (s *Site) NetworkStatusUpdated(network []skupperv2alpha1.SiteRecord) error return err } s.site = updated + // UpdateStatus on fake clients (and some responses) can drop unrelated conditions. + if meta.FindStatusCondition(s.site.Status.Conditions, skupperv2alpha1.CONDITION_TYPE_RUNNING) == nil || + (len(s.site.Status.Network) == 0 && len(network) > 0) { + for _, typ := range []string{ + skupperv2alpha1.CONDITION_TYPE_CONFIGURED, + skupperv2alpha1.CONDITION_TYPE_RUNNING, + skupperv2alpha1.CONDITION_TYPE_RESOLVED, + } { + if meta.FindStatusCondition(s.site.Status.Conditions, typ) == nil { + if old := meta.FindStatusCondition(prev.Status.Conditions, typ); old != nil { + meta.SetStatusCondition(&s.site.Status.Conditions, *old) + } + } + } + s.site.Status.Network = network + s.site.Status.SitesInNetwork = len(network) + s.site.RefreshAggregatedStatus() + updated, err = s.UpdateSiteStatus(s.site) + if err != nil { + return err + } + s.site = updated + } // find the site record for this site, then process the link records it contains linkRecords := internalnetwork.GetLinkRecordsForSite(s.site.GetSiteId(), network) diff --git a/pkg/apis/skupper/v2alpha1/types.go b/pkg/apis/skupper/v2alpha1/types.go index 50db39420..bf6a48280 100644 --- a/pkg/apis/skupper/v2alpha1/types.go +++ b/pkg/apis/skupper/v2alpha1/types.go @@ -252,6 +252,11 @@ func (s *Site) SetRunning(state ConditionState) bool { return false } +// RefreshAggregatedStatus updates StatusType, Message, and Ready from current conditions. +func (s *Site) RefreshAggregatedStatus() bool { + return s.Status.setReady(s.requiredConditions(), s.ObjectMeta.Generation) +} + func (s *Site) resolutionRequired() bool { return s.Spec.LinkAccess != "" && s.Spec.LinkAccess != "none" }