From e95be4aa846f4fadec1ba6b6f0883b8bcdd07c2f Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 15 Oct 2024 16:40:34 +0100 Subject: [PATCH] Revert "refactor: use gateways again for now to check for expected certs" This reverts commit 552d9f348f219dc77fe467c83bb4f73c2eb04f40. --- .../effective_tls_policies_reconciler.go | 36 +++++++++++++++---- controllers/tlspolicy_status_updater.go | 29 ++++++++------- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index b210f5fed..948283885 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -217,11 +217,6 @@ func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.G continue } - hostname := "*" - if l.Hostname != nil { - hostname = string(*l.Hostname) - } - for _, certRef := range l.TLS.CertificateRefs { secretRef := corev1.ObjectReference{ Name: string(certRef.Name), @@ -233,8 +228,37 @@ func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.G } // Gateway API hostname explicitly disallows IP addresses, so this // should be OK. - tlsHosts[secretRef] = append(tlsHosts[secretRef], hostname) + tlsHosts[secretRef] = append(tlsHosts[secretRef], string(*l.Hostname)) + } + } + + certs := make([]*certmanv1.Certificate, 0, len(tlsHosts)) + for secretRef, hosts := range tlsHosts { + certs = append(certs, buildCertManagerCertificate(tlsPolicy, secretRef, hosts)) + } + return certs +} + +func expectedCertificatesForListener(l *machinery.Listener, tlsPolicy *kuadrantv1alpha1.TLSPolicy) []*certmanv1.Certificate { + tlsHosts := make(map[corev1.ObjectReference][]string) + + hostname := "*" + if l.Hostname != nil { + hostname = string(*l.Hostname) + } + + for _, certRef := range l.TLS.CertificateRefs { + secretRef := corev1.ObjectReference{ + Name: string(certRef.Name), + } + if certRef.Namespace != nil { + secretRef.Namespace = string(*certRef.Namespace) + } else { + secretRef.Namespace = l.GetNamespace() } + // Gateway API hostname explicitly disallows IP addresses, so this + // should be OK. + tlsHosts[secretRef] = append(tlsHosts[secretRef], hostname) } certs := make([]*certmanv1.Certificate, 0, len(tlsHosts)) diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 378eb7a79..45c8017a9 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/dynamic" "k8s.io/utils/ptr" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -106,7 +107,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) enforcedCondition(ctx context.Context return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) } - if err := t.isCertificatesReady(ctx, tlsPolicy, topology); err != nil { + if err := t.isCertificatesReady(tlsPolicy, topology); err != nil { return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) } @@ -179,7 +180,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl return nil } -func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(ctx context.Context, p machinery.Policy, topology *machinery.Topology) error { +func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { tlsPolicy, ok := p.(*kuadrantv1alpha1.TLSPolicy) if !ok { return errors.New("invalid policy") @@ -187,24 +188,26 @@ func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(ctx context.Conte // Get all listeners where the gateway contains this // TODO: Update when targeting by section name is allowed, the listener will contain the policy rather than the gateway - gateways := lo.FilterMap(topology.Targetables().Items(), func(t machinery.Targetable, index int) (*machinery.Gateway, bool) { - gw, ok := t.(*machinery.Gateway) - return gw, ok && lo.Contains(gw.Policies(), p) + listeners := lo.FilterMap(topology.Targetables().Items(), func(t machinery.Targetable, index int) (*machinery.Listener, bool) { + l, ok := t.(*machinery.Listener) + return l, ok && lo.Contains(l.Gateway.Policies(), p) }) - if len(gateways) == 0 { + if len(listeners) == 0 { return errors.New("no valid gateways found") } - // Use gateway instead of listener for calculating expected certs - // This is because listeners that reference the same cert secret but with different host names are merged to a - // singular Certificate resource containing the hostnames. However, this means for Gateways with multiple listeners - // the expected certificates will be checked multiple times - for _, gw := range gateways { - expectedCertificates := expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) + for i, l := range listeners { + // Not valid - so no need to check if cert is ready since there should not be one created + err := validateGatewayListenerBlock(field.NewPath("").Index(i), *l.Listener, l.Gateway).ToAggregate() + if err != nil { + continue + } + + expectedCertificates := expectedCertificatesForListener(l, tlsPolicy) for _, cert := range expectedCertificates { - objs := topology.Objects().Children(gw) + objs := topology.Objects().Children(l) obj, ok := lo.Find(objs, func(o machinery.Object) bool { return o.GroupVersionKind().GroupKind() == CertManagerCertificateKind && o.GetNamespace() == cert.GetNamespace() && o.GetName() == cert.GetName() })