From 0d747a1989415efaf4102fd0d7c6bada030cc616 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 22 May 2024 17:41:54 -0400 Subject: [PATCH] fix cluster-local routes being stalled when external-domain-tls is enabled --- pkg/apis/serving/v1/route_lifecycle.go | 5 ++ pkg/reconciler/route/route.go | 28 +++++++---- pkg/reconciler/route/table_test.go | 65 ++++++++++++++++++++++++++ pkg/testing/v1/route.go | 8 ++++ 4 files changed, 97 insertions(+), 9 deletions(-) diff --git a/pkg/apis/serving/v1/route_lifecycle.go b/pkg/apis/serving/v1/route_lifecycle.go index 35492cd27f98..d371b80f3a05 100644 --- a/pkg/apis/serving/v1/route_lifecycle.go +++ b/pkg/apis/serving/v1/route_lifecycle.go @@ -202,6 +202,11 @@ const ( // RouteConditionCertificateProvisioned condition when it is set to True // because external-domain-tls was not enabled. ExternalDomainTLSNotEnabledMessage = "external-domain-tls is not enabled" + + // TLSNotEnabledForClusterLocalMessage is the message which is set on the + // RouteConditionCertificateProvisioned condition when it is set to True + // because the domain is cluster-local. + TLSNotEnabledForClusterLocalMessage = "TLS is not enabled for cluster-local" ) // MarkTLSNotEnabled sets RouteConditionCertificateProvisioned to true when diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go index ccff359b4ac2..fd391bb8155c 100644 --- a/pkg/reconciler/route/route.go +++ b/pkg/reconciler/route/route.go @@ -143,7 +143,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil return err } - tls, acmeChallenges, err := c.externalDomainTLS(ctx, r.Status.URL.Host, r, traffic) + tls, acmeChallenges, desiredCerts, err := c.externalDomainTLS(ctx, r.Status.URL.Host, r, traffic) if err != nil { return err } @@ -154,6 +154,10 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil return err } tls = append(tls, internalTLS...) + } else if externalDomainTLSEnabled(ctx, r) && len(desiredCerts) == 0 { + // If external TLS is enabled but we have no desired certs then the route + // must have only cluster local hosts + r.Status.MarkTLSNotEnabled(v1.TLSNotEnabledForClusterLocalMessage) } // Reconcile ingress and its children resources. @@ -195,18 +199,24 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil return nil } -func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.Route, traffic *traffic.Config) ([]netv1alpha1.IngressTLS, []netv1alpha1.HTTP01Challenge, error) { +func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.Route, traffic *traffic.Config) ( + []netv1alpha1.IngressTLS, + []netv1alpha1.HTTP01Challenge, + []*netv1alpha1.Certificate, + error, +) { + var desiredCerts []*netv1alpha1.Certificate logger := logging.FromContext(ctx) tls := []netv1alpha1.IngressTLS{} if !externalDomainTLSEnabled(ctx, r) { r.Status.MarkTLSNotEnabled(v1.ExternalDomainTLSNotEnabledMessage) - return tls, nil, nil + return tls, nil, desiredCerts, nil } domainToTagMap, err := domains.GetAllDomainsAndTags(ctx, r, getTrafficNames(traffic.Targets), traffic.Visibility) if err != nil { - return nil, nil, err + return nil, nil, desiredCerts, err } for domain := range domainToTagMap { @@ -223,7 +233,7 @@ func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.R allWildcardCerts, err := c.certificateLister.Certificates(r.Namespace).List(labelSelector) if err != nil { - return nil, nil, err + return nil, nil, desiredCerts, err } domainConfig := config.FromContext(ctx).Domain @@ -231,7 +241,7 @@ func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.R domain := domainConfig.LookupDomainForLabels(rLabels) acmeChallenges := []netv1alpha1.HTTP01Challenge{} - desiredCerts := resources.MakeCertificates(r, domainToTagMap, certClass(ctx, r), domain) + desiredCerts = resources.MakeCertificates(r, domainToTagMap, certClass(ctx, r), domain) for _, desiredCert := range desiredCerts { dnsNames := sets.New(desiredCert.Spec.DNSNames...) // Look for a matching wildcard cert before provisioning a new one. This saves the @@ -247,7 +257,7 @@ func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.R } else { r.Status.MarkCertificateProvisionFailed(desiredCert) } - return nil, nil, err + return nil, nil, desiredCerts, err } dnsNames = sets.New(cert.Spec.DNSNames...) } @@ -306,12 +316,12 @@ func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.R orphanCerts, err := c.getOrphanRouteCerts(r, domainToTagMap, netcfg.CertificateExternalDomain) if err != nil { - return nil, nil, err + return nil, nil, desiredCerts, err } c.deleteOrphanedCerts(ctx, orphanCerts) - return tls, acmeChallenges, nil + return tls, acmeChallenges, desiredCerts, nil } func (c *Reconciler) clusterLocalDomainTLS(ctx context.Context, r *v1.Route, tc *traffic.Config) ([]netv1alpha1.IngressTLS, error) { diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go index 9482b375d166..71e7a10255d5 100644 --- a/pkg/reconciler/route/table_test.go +++ b/pkg/reconciler/route/table_test.go @@ -75,6 +75,7 @@ const ( rolloutDurationKey key = iota externalSchemeKey enableExternalDomainTLSKey + domainConfigKey ) // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. @@ -3276,6 +3277,7 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { WithRouteUID("65-23"), WithRouteGeneration(1), WithRouteObservedGeneration, MarkTrafficAssigned, MarkIngressNotConfigured, + WithRouteConditionsTLSNotEnabledForClusterLocalMessage, WithLocalDomain, WithAddress, WithInitRouteConditions, WithRouteLabel(map[string]string{netapi.VisibilityLabelKey: serving.VisibilityClusterLocal}), WithStatusTraffic( @@ -3286,6 +3288,66 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { })), }}, Key: "default/becomes-local", + }, { + Name: "local domain route should mark certificate provisioned TLS disabled", + Key: "default/local-domain", + Ctx: context.WithValue(context.Background(), domainConfigKey, &config.Domain{ + Domains: map[string]config.DomainConfig{ + "svc.cluster.local": {}, + }, + }), + Objects: []runtime.Object{ + Route("default", "local-domain", WithConfigTarget("config"), WithRouteGeneration(1), + WithRouteObservedGeneration, + WithRouteUID("65-23")), + cfg("default", "config", + WithConfigGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001")), + rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001")), + simpleIngress( + Route("default", "local-domain", WithConfigTarget("config"), WithRouteUID("65-23")), + &traffic.Config{ + Targets: map[string]traffic.RevisionTargets{ + traffic.DefaultTarget: {{ + TrafficTarget: v1.TrafficTarget{ + ConfigurationName: "config", + LatestRevision: ptr.Bool(true), + RevisionName: "config-00001", + Percent: ptr.Int64(100), + }, + }}, + }, + }, + withReadyIngress, + // simpleIngress and the test use different 'configs' (limit of reading config from the context) + // so we need to delete the external visible host rules + func(i *netv1alpha1.Ingress) { + localRules := i.Spec.Rules[:0] + for _, rule := range i.Spec.Rules { + if rule.Visibility == netv1alpha1.IngressVisibilityClusterLocal { + localRules = append(localRules, rule) + } + } + i.Spec.Rules = localRules + }, + ), + simpleK8sService(Route("default", "local-domain", WithConfigTarget("config"), + WithRouteUID("65-23"))), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: Route("default", "local-domain", WithConfigTarget("config"), + WithRouteUID("65-23"), + WithRouteGeneration(1), WithRouteObservedGeneration, + MarkTrafficAssigned, + MarkIngressReady, + WithRouteConditionsTLSNotEnabledForClusterLocalMessage, + WithLocalDomain, WithAddress, WithInitRouteConditions, + WithStatusTraffic( + v1.TrafficTarget{ + RevisionName: "config-00001", + Percent: ptr.Int64(100), + LatestRevision: ptr.Bool(true), + })), + }}, }} for i, row := range table { @@ -3323,6 +3385,9 @@ func NewTestReconciler(ctx context.Context, listers *Listers, cmw configmap.Watc if v := ctx.Value(externalSchemeKey); v != nil { cfg.Network.DefaultExternalScheme = v.(string) } + if v := ctx.Value(domainConfigKey); v != nil { + cfg.Domain = v.(*config.Domain) + } return routereconciler.NewReconciler(ctx, logging.FromContext(ctx), diff --git a/pkg/testing/v1/route.go b/pkg/testing/v1/route.go index f1d26d55b561..935cdb7f5100 100644 --- a/pkg/testing/v1/route.go +++ b/pkg/testing/v1/route.go @@ -180,6 +180,14 @@ func WithRouteConditionsExternalDomainTLSDisabled(rt *v1.Route) { rt.Status.MarkTLSNotEnabled(v1.ExternalDomainTLSNotEnabledMessage) } +// WithRouteConditionsTLSNotEnabledForClusterLocalMessage calls +// MarkTLSNotEnabled with TLSNotEnabledForClusterLocalMessage after initialized +// the Service's conditions. +func WithRouteConditionsTLSNotEnabledForClusterLocalMessage(rt *v1.Route) { + rt.Status.InitializeConditions() + rt.Status.MarkTLSNotEnabled(v1.TLSNotEnabledForClusterLocalMessage) +} + // WithRouteConditionsHTTPDowngrade calls MarkHTTPDowngrade after initialized the Service's conditions. func WithRouteConditionsHTTPDowngrade(rt *v1.Route) { rt.Status.InitializeConditions()