From 6cb9d26280d863660e4b200f29dcc4cfb336158d Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 9 Oct 2024 18:42:18 +0100 Subject: [PATCH 01/13] refactor: effective tls policies reconciler Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 289 ++++++++++++++++++ controllers/state_of_the_world.go | 2 +- controllers/test_common.go | 14 - controllers/tls_workflow.go | 41 ++- controllers/tlspolicies_validator.go | 37 ++- controllers/tlspolicy_certmanager.go | 20 -- .../tlspolicy_certmanager_certificates.go | 233 -------------- controllers/tlspolicy_controller.go | 197 ------------ controllers/tlspolicy_status_updater.go | 10 +- main.go | 15 - .../tlspolicy/tlspolicy_controller_test.go | 16 - 11 files changed, 360 insertions(+), 514 deletions(-) create mode 100644 controllers/effective_tls_policies_reconciler.go delete mode 100644 controllers/tlspolicy_certmanager_certificates.go delete mode 100644 controllers/tlspolicy_controller.go diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go new file mode 100644 index 000000000..e85cf00e2 --- /dev/null +++ b/controllers/effective_tls_policies_reconciler.go @@ -0,0 +1,289 @@ +package controllers + +import ( + "context" + "reflect" + "sync" + + certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/kuadrant/policy-machinery/controller" + "github.com/kuadrant/policy-machinery/machinery" + "github.com/samber/lo" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/client-go/dynamic" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + crlog "sigs.k8s.io/controller-runtime/pkg/log" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" +) + +type EffectiveTLSPoliciesReconciler struct { + client *dynamic.DynamicClient + scheme *runtime.Scheme +} + +func NewEffectiveTLSPoliciesReconciler(client *dynamic.DynamicClient, scheme *runtime.Scheme) *EffectiveTLSPoliciesReconciler { + return &EffectiveTLSPoliciesReconciler{client: client, scheme: scheme} +} + +func (t *EffectiveTLSPoliciesReconciler) Subscription() *controller.Subscription { + return &controller.Subscription{ + Events: []controller.ResourceEventMatcher{ + {Kind: &machinery.GatewayGroupKind}, + {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind}, + {Kind: &CertManagerCertificateKind}, + }, + ReconcileFunc: t.Reconcile, + } +} + +//+kubebuilder:rbac:groups=kuadrant.io,resources=tlspolicies,verbs=get;list;watch;update;patch;delete +//+kubebuilder:rbac:groups=kuadrant.io,resources=tlspolicies/status,verbs=get;update;patch +//+kubebuilder:rbac:groups=kuadrant.io,resources=tlspolicies/finalizers,verbs=update +//+kubebuilder:rbac:groups="cert-manager.io",resources=issuers,verbs=get;list;watch; +//+kubebuilder:rbac:groups="cert-manager.io",resources=clusterissuers,verbs=get;list;watch; +//+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch +//+kubebuilder:rbac:groups="cert-manager.io",resources=certificates,verbs=get;list;watch;create;update;patch;delete + +func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("Reconcile") + + // Get all TLS Policies + policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { + _, ok := item.(*kuadrantv1alpha1.TLSPolicy) + return ok + }) + + for _, p := range policies { + policy := p.(*kuadrantv1alpha1.TLSPolicy) + + // Get all listeners where the gateway contains this policy + // TODO: Update when targeting by section name is allowed, the listener will contain the policy rather than the gateway + 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) + }) + + // Policy is deleted + if policy.DeletionTimestamp != nil { + logger.V(1).Info("policy is marked for deletion, nothing to do", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + continue + } + + // Policy is not valid + isValid, _ := IsPolicyValid(ctx, s, policy) + if !isValid { + logger.V(1).Info("deleting certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + if err := t.deleteCertificatesForPolicy(ctx, topology, listeners); err != nil { + logger.Error(err, "unable to delete certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + } + continue + } + + // Policy is valid + // Get all certs in topology + certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { + r, ok := item.(*controller.RuntimeObject) + if !ok { + return nil, false + } + c, ok := r.Object.(*certmanv1.Certificate) + if !ok { + return nil, false + } + + // Only want certs owned by TLSPolicies + if isObjectOwnedByGroupKind(c, kuadrantv1alpha1.TLSPolicyGroupKind) { + return c, true + } + + return nil, false + }) + + var expectedCerts []*certmanv1.Certificate + + for _, l := range listeners { + // Need to use Gateway as listener hosts can be merged into a singular cert if using the same cert reference + expectedCertificates := expectedCertificatesForGateway(ctx, l.Gateway.Gateway, policy) + + for _, cert := range expectedCertificates { + resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) + + // Check is cert already in topology + 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() + }) + + // Create + if !ok { + expectedCerts = append(expectedCerts, cert) + if err := controllerutil.SetControllerReference(policy, cert, t.scheme); err != nil { + logger.Error(err, "failed to set owner reference on certificate", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + continue + } + + un, err := controller.Destruct(cert) + if err != nil { + logger.Error(err, "unable to destruct cert") + continue + } + _, err = resource.Create(ctx, un, metav1.CreateOptions{}) + if err != nil { + logger.Error(err, "unable to create certificate", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + } + + continue + } + + // Update + tCert := obj.(*controller.RuntimeObject).Object.(*certmanv1.Certificate) + expectedCerts = append(expectedCerts, tCert) + if reflect.DeepEqual(tCert.Spec, cert.Spec) { + logger.V(1).Info("skipping update, cert specs are the same, nothing to do") + continue + } + + tCert.Spec = cert.Spec + un, err := controller.Destruct(tCert) + if err != nil { + logger.Error(err, "unable to destruct cert") + continue + } + _, err = resource.Update(ctx, un, metav1.UpdateOptions{}) + if err != nil { + logger.Error(err, "unable to update certificate", "policy", policy.Name) + } + } + } + + // Clean up orphaned certs + orphanedCerts, _ := lo.Difference(certs, expectedCerts) + for _, orphanedCert := range orphanedCerts { + resource := t.client.Resource(CertManagerCertificatesResource).Namespace(orphanedCert.GetNamespace()) + if err := resource.Delete(ctx, orphanedCert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "unable to delete orphaned certificate", "policy", policy.Name) + continue + } + } + } + + return nil +} + +func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context.Context, topology *machinery.Topology, listeners []*machinery.Listener) error { + logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("deleteCertificatesForPolicy") + + for _, l := range listeners { + // Get children of listeners + objs := topology.Objects().Children(l) + + certs := lo.FilterMap(objs, func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { + c, ok := item.(*controller.RuntimeObject) + if !ok { + return nil, false + } + ce, ok := c.Object.(*certmanv1.Certificate) + return ce, ok + }) + + for _, cert := range certs { + resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) + + if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil { + logger.Error(err, "delete certificate", "name", cert.Name) + return err + } + } + } + + return nil +} + +func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.Gateway, tlsPolicy *kuadrantv1alpha1.TLSPolicy) []*certmanv1.Certificate { + log := crlog.FromContext(ctx) + + tlsHosts := make(map[corev1.ObjectReference][]string) + for i, l := range gateway.Spec.Listeners { + err := validateGatewayListenerBlock(field.NewPath("spec", "listeners").Index(i), l, gateway).ToAggregate() + if err != nil { + log.Info("Skipped a listener block: " + err.Error()) + continue + } + + 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 = gateway.GetNamespace() + } + // Gateway API hostname explicitly disallows IP addresses, so this + // should be OK. + 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)) + for secretRef, hosts := range tlsHosts { + certs = append(certs, buildCertManagerCertificate(tlsPolicy, secretRef, hosts)) + } + return certs +} + +func buildCertManagerCertificate(tlsPolicy *kuadrantv1alpha1.TLSPolicy, secretRef corev1.ObjectReference, hosts []string) *certmanv1.Certificate { + crt := &certmanv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretRef.Name, + Namespace: secretRef.Namespace, + }, + TypeMeta: metav1.TypeMeta{ + Kind: certmanv1.CertificateKind, + APIVersion: certmanv1.SchemeGroupVersion.String(), + }, + Spec: certmanv1.CertificateSpec{ + DNSNames: hosts, + SecretName: secretRef.Name, + IssuerRef: tlsPolicy.Spec.IssuerRef, + Usages: certmanv1.DefaultKeyUsages(), + }, + } + translatePolicy(crt, tlsPolicy.Spec) + return crt +} diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 945216ca9..c18d66fa0 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -301,7 +301,7 @@ func (b *BootOptionsBuilder) Reconciler() controller.ReconcileFunc { NewAuthorinoReconciler(b.client).Subscription().Reconcile, NewLimitadorReconciler(b.client).Subscription().Reconcile, NewDNSWorkflow().Run, - NewTLSWorkflow(b.client, b.isCertManagerInstalled).Run, + NewTLSWorkflow(b.client, b.manager.GetScheme(), b.isCertManagerInstalled).Run, NewAuthWorkflow().Run, NewRateLimitWorkflow().Run, }, diff --git a/controllers/test_common.go b/controllers/test_common.go index 43d355ab9..232d19943 100644 --- a/controllers/test_common.go +++ b/controllers/test_common.go @@ -100,20 +100,6 @@ func SetupKuadrantOperatorForTest(s *runtime.Scheme, cfg *rest.Config) { Expect(err).NotTo(HaveOccurred()) - tlsPolicyBaseReconciler := reconcilers.NewBaseReconciler( - mgr.GetClient(), mgr.GetScheme(), mgr.GetAPIReader(), - log.Log.WithName("tlspolicy"), - mgr.GetEventRecorderFor("TLSPolicy"), - ) - - err = (&TLSPolicyReconciler{ - BaseReconciler: tlsPolicyBaseReconciler, - TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, - RestMapper: mgr.GetRESTMapper(), - }).SetupWithManager(mgr) - - Expect(err).NotTo(HaveOccurred()) - dnsPolicyBaseReconciler := reconcilers.NewBaseReconciler( mgr.GetClient(), mgr.GetScheme(), mgr.GetAPIReader(), log.Log.WithName("dnspolicy"), diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 627fa1896..3baaeab63 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -1,14 +1,20 @@ package controllers import ( + "context" + "sync" + "github.com/cert-manager/cert-manager/pkg/apis/certmanager" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/kuadrant/policy-machinery/controller" "github.com/kuadrant/policy-machinery/machinery" "github.com/samber/lo" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" ) @@ -27,13 +33,18 @@ var ( CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} ) -func NewTLSWorkflow(client *dynamic.DynamicClient, isCertManagerInstalled bool) *controller.Workflow { +func NewTLSWorkflow(client *dynamic.DynamicClient, scheme *runtime.Scheme, isCertManagerInstalled bool) *controller.Workflow { return &controller.Workflow{ - Precondition: NewValidateTLSPoliciesValidatorReconciler(isCertManagerInstalled).Subscription().Reconcile, + Precondition: NewValidateTLSPoliciesValidatorReconciler(isCertManagerInstalled).Subscription().Reconcile, + Tasks: []controller.ReconcileFunc{ + NewEffectiveTLSPoliciesReconciler(client, scheme).Subscription().Reconcile, + }, Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).Subscription().Reconcile, } } +// Linking functions + func LinkListenerToCertificateFunc(objs controller.Store) machinery.LinkFunc { gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) listeners := lo.FlatMap(lo.Map(gateways, func(g *gwapiv1.Gateway, _ int) *machinery.Gateway { @@ -51,7 +62,7 @@ func LinkListenerToCertificateFunc(objs controller.Store) machinery.LinkFunc { return nil } - listener, ok := lo.Find(listeners, func(l *machinery.Listener) bool { + linkedListeners := lo.Filter(listeners, func(l *machinery.Listener, index int) bool { if l.TLS != nil && l.TLS.CertificateRefs != nil { for _, certRef := range l.TLS.CertificateRefs { certRefNS := "" @@ -69,11 +80,9 @@ func LinkListenerToCertificateFunc(objs controller.Store) machinery.LinkFunc { return false }) - if ok { - return []machinery.Object{listener} - } - - return nil + return lo.Map(linkedListeners, func(l *machinery.Listener, index int) machinery.Object { + return l + }) }, } } @@ -154,3 +163,19 @@ func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { }, } } + +// Common functions used across multiple reconcilers + +func IsPolicyValid(ctx context.Context, s *sync.Map, policy *kuadrantv1alpha1.TLSPolicy) (bool, error) { + logger := controller.LoggerFromContext(ctx).WithName("IsPolicyValid") + + store, ok := s.Load(TLSPolicyAcceptedKey) + if !ok { + logger.V(1).Info("TLSPolicyAcceptedKey not found, policies will be checked for validity by current status") + return meta.IsStatusConditionTrue(policy.Status.Conditions, string(gatewayapiv1alpha2.PolicyReasonAccepted)), nil + } + + isPolicyValidErrorMap := store.(map[string]error) + + return isPolicyValidErrorMap[policy.GetLocator()] == nil, isPolicyValidErrorMap[policy.GetLocator()] +} diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index 368395caf..db5857dc8 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -2,8 +2,10 @@ package controllers import ( "context" + "fmt" "sync" + certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/kuadrant/policy-machinery/controller" "github.com/kuadrant/policy-machinery/machinery" "github.com/samber/lo" @@ -30,7 +32,6 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subs {Kind: &machinery.GatewayGroupKind}, {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.CreateEvent)}, {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.UpdateEvent)}, - {Kind: &CertManagerCertificateKind}, {Kind: &CertManagerIssuerKind}, {Kind: &CertManagerClusterIssuerKind}, }, @@ -68,6 +69,40 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ continue } + // Validate IssuerRef is correct + if !lo.Contains([]string{"", certmanv1.IssuerKind, certmanv1.ClusterIssuerKind}, p.Spec.IssuerRef.Kind) { + isPolicyValidErrorMap[p.GetLocator()] = fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, p.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind) + continue + } + + // Validate Issuer is present on cluster through the topology + _, ok := lo.Find(topology.Objects().Items(), func(item machinery.Object) bool { + runtimeObj, ok := item.(*controller.RuntimeObject) + if !ok { + return false + } + + issuer, ok := runtimeObj.Object.(certmanv1.GenericIssuer) + if !ok { + return false + } + + match := issuer.GetName() == p.Spec.IssuerRef.Name + if lo.Contains([]string{"", certmanv1.IssuerKind}, p.Spec.IssuerRef.Kind) { + match = match && issuer.GetNamespace() == p.GetNamespace() && + issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.IssuerKind + } else { + match = match && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.ClusterIssuerKind + } + + return match + }) + + if !ok { + isPolicyValidErrorMap[p.GetLocator()] = fmt.Errorf("unable to find issuer") + continue + } + isPolicyValidErrorMap[p.GetLocator()] = nil } diff --git a/controllers/tlspolicy_certmanager.go b/controllers/tlspolicy_certmanager.go index c9cf53ded..7cdee7de0 100644 --- a/controllers/tlspolicy_certmanager.go +++ b/controllers/tlspolicy_certmanager.go @@ -1,14 +1,10 @@ package controllers import ( - "context" - "fmt" - certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/kuadrant/kuadrant-operator/api/v1alpha1" @@ -114,19 +110,3 @@ func translatePolicy(crt *certmanv1.Certificate, tlsPolicy v1alpha1.TLSPolicySpe } } } - -// validateIssuer validates that the issuer specified exists -func validateIssuer(ctx context.Context, k8sClient client.Client, policy *v1alpha1.TLSPolicy) error { - var issuer client.Object - issuerNamespace := "" - switch policy.Spec.IssuerRef.Kind { - case "", certmanv1.IssuerKind: - issuer = &certmanv1.Issuer{} - issuerNamespace = policy.Namespace - case certmanv1.ClusterIssuerKind: - issuer = &certmanv1.ClusterIssuer{} - default: - return fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, policy.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind) - } - return k8sClient.Get(ctx, client.ObjectKey{Name: policy.Spec.IssuerRef.Name, Namespace: issuerNamespace}, issuer) -} diff --git a/controllers/tlspolicy_certmanager_certificates.go b/controllers/tlspolicy_certmanager_certificates.go deleted file mode 100644 index fcce87f80..000000000 --- a/controllers/tlspolicy_certmanager_certificates.go +++ /dev/null @@ -1,233 +0,0 @@ -package controllers - -import ( - "context" - "fmt" - "reflect" - "slices" - - certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - "github.com/kuadrant/policy-machinery/machinery" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/controller-runtime/pkg/client" - crlog "sigs.k8s.io/controller-runtime/pkg/log" - gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - reconcilerutils "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" -) - -func (r *TLSPolicyReconciler) reconcileCertificates(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, gwDiffObj *reconcilerutils.GatewayDiffs) error { - log := crlog.FromContext(ctx) - - log.V(3).Info("reconciling certificates") - for _, gw := range gwDiffObj.GatewaysWithInvalidPolicyRef { - log.V(1).Info("reconcileCertificates: gateway with invalid policy ref", "key", gw.Key()) - if err := r.deleteGatewayCertificates(ctx, gw.Gateway, tlsPolicy); err != nil { - return fmt.Errorf("error deleting certificates for gw %v: %w", gw.Gateway.Name, err) - } - } - - // Reconcile Certificates for each gateway directly referred by the policy (existing and new) - for _, gw := range append(gwDiffObj.GatewaysWithValidPolicyRef, gwDiffObj.GatewaysMissingPolicyRef...) { - log.V(1).Info("reconcileCertificates: gateway with valid or missing policy ref", "key", gw.Key()) - expectedCertificates := expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) - if err := r.createOrUpdateGatewayCertificates(ctx, tlsPolicy, expectedCertificates); err != nil { - return fmt.Errorf("error creating and updating expected certificates for gateway %v: %w", gw.Gateway.Name, err) - } - if err := r.deleteUnexpectedCertificates(ctx, expectedCertificates, gw.Gateway, tlsPolicy); err != nil { - return fmt.Errorf("error removing unexpected certificate for gateway %v: %w", gw.Gateway.Name, err) - } - } - return nil -} - -func (r *TLSPolicyReconciler) createOrUpdateGatewayCertificates(ctx context.Context, tlspolicy *v1alpha1.TLSPolicy, expectedCertificates []*certmanv1.Certificate) error { - //create or update all expected Certificates - for idx := range expectedCertificates { - cert := expectedCertificates[idx] - if err := r.SetOwnerReference(tlspolicy, cert); err != nil { - return err - } - - if err := r.ReconcileResource(ctx, &certmanv1.Certificate{}, cert, certificateBasicMutator); err != nil { - return err - } - } - return nil -} - -func (r *TLSPolicyReconciler) deleteGatewayCertificates(ctx context.Context, gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error { - return r.deleteCertificatesWithLabels(ctx, commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), tlsPolicy), tlsPolicy.Namespace) -} - -func (r *TLSPolicyReconciler) deleteCertificates(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy) error { - return r.deleteCertificatesWithLabels(ctx, policyTLSCertificateLabels(tlsPolicy), tlsPolicy.Namespace) -} - -func (r *TLSPolicyReconciler) deleteCertificatesWithLabels(ctx context.Context, lbls map[string]string, namespace string) error { - listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(lbls), Namespace: namespace} - certList := &certmanv1.CertificateList{} - if err := r.Client().List(ctx, certList, listOptions); err != nil { - return err - } - - for i := range certList.Items { - if err := r.Client().Delete(ctx, &certList.Items[i]); err != nil { - return err - } - } - return nil -} - -func (r *TLSPolicyReconciler) deleteUnexpectedCertificates(ctx context.Context, expectedCertificates []*certmanv1.Certificate, gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error { - // remove any certificates for this gateway and TLSPolicy that are no longer expected - existingCertificates := &certmanv1.CertificateList{} - dnsLabels := commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), tlsPolicy) - listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(dnsLabels)} - if err := r.Client().List(ctx, existingCertificates, listOptions); client.IgnoreNotFound(err) != nil { - return err - } - for i, p := range existingCertificates.Items { - if !slices.ContainsFunc(expectedCertificates, func(expectedCertificate *certmanv1.Certificate) bool { - return expectedCertificate.Name == p.Name && expectedCertificate.Namespace == p.Namespace - }) { - if err := r.Client().Delete(ctx, &existingCertificates.Items[i]); err != nil { - return err - } - } - } - return nil -} - -func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) []*certmanv1.Certificate { - log := crlog.FromContext(ctx) - - tlsHosts := make(map[corev1.ObjectReference][]string) - for i, l := range gateway.Spec.Listeners { - err := validateGatewayListenerBlock(field.NewPath("spec", "listeners").Index(i), l, gateway).ToAggregate() - if err != nil { - log.Info("Skipped a listener block: " + err.Error()) - continue - } - - 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 = gateway.GetNamespace() - } - // Gateway API hostname explicitly disallows IP addresses, so this - // should be OK. - tlsHosts[secretRef] = append(tlsHosts[secretRef], string(*l.Hostname)) - } - } - - certs := make([]*certmanv1.Certificate, 0, len(tlsHosts)) - for secretRef, hosts := range tlsHosts { - certs = append(certs, buildCertManagerCertificate(gateway, tlsPolicy, secretRef, hosts)) - } - return certs -} - -func expectedCertificatesForListener(l *machinery.Listener, tlsPolicy *v1alpha1.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)) - for secretRef, hosts := range tlsHosts { - certs = append(certs, buildCertManagerCertificate(l.Gateway.Gateway, tlsPolicy, secretRef, hosts)) - } - return certs -} - -func buildCertManagerCertificate(gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy, secretRef corev1.ObjectReference, hosts []string) *certmanv1.Certificate { - tlsCertLabels := commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), tlsPolicy) - - crt := &certmanv1.Certificate{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretRef.Name, - Namespace: secretRef.Namespace, - Labels: tlsCertLabels, - }, - Spec: certmanv1.CertificateSpec{ - DNSNames: hosts, - SecretName: secretRef.Name, - SecretTemplate: &certmanv1.CertificateSecretTemplate{ - Labels: tlsCertLabels, - }, - IssuerRef: tlsPolicy.Spec.IssuerRef, - Usages: certmanv1.DefaultKeyUsages(), - }, - } - translatePolicy(crt, tlsPolicy.Spec) - return crt -} - -func commonTLSCertificateLabels(gwKey client.ObjectKey, p *v1alpha1.TLSPolicy) map[string]string { - common := map[string]string{} - for k, v := range policyTLSCertificateLabels(p) { - common[k] = v - } - for k, v := range gatewayTLSCertificateLabels(gwKey) { - common[k] = v - } - return common -} - -func policyTLSCertificateLabels(p *v1alpha1.TLSPolicy) map[string]string { - return map[string]string{ - p.DirectReferenceAnnotationName(): p.Name, - fmt.Sprintf("%s-namespace", p.DirectReferenceAnnotationName()): p.Namespace, - } -} - -func gatewayTLSCertificateLabels(gwKey client.ObjectKey) map[string]string { - return map[string]string{ - "gateway-namespace": gwKey.Namespace, - "gateway": gwKey.Name, - } -} - -func certificateBasicMutator(existingObj, desiredObj client.Object) (bool, error) { - existing, ok := existingObj.(*certmanv1.Certificate) - if !ok { - return false, fmt.Errorf("%T is not an *certmanv1.Certificate", existingObj) - } - desired, ok := desiredObj.(*certmanv1.Certificate) - if !ok { - return false, fmt.Errorf("%T is not an *certmanv1.Certificate", desiredObj) - } - - if reflect.DeepEqual(existing.Spec, desired.Spec) { - return false, nil - } - - existing.Spec = desired.Spec - - return true, nil -} diff --git a/controllers/tlspolicy_controller.go b/controllers/tlspolicy_controller.go deleted file mode 100644 index 723d0e263..000000000 --- a/controllers/tlspolicy_controller.go +++ /dev/null @@ -1,197 +0,0 @@ -/* -Copyright 2023. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controllers - -import ( - "context" - "fmt" - - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/handler" - crlog "sigs.k8s.io/controller-runtime/pkg/log" - gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" - "github.com/kuadrant/kuadrant-operator/pkg/library/mappers" - "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" -) - -const TLSPolicyFinalizer = "kuadrant.io/tls-policy" - -// TLSPolicyReconciler reconciles a TLSPolicy object -type TLSPolicyReconciler struct { - *reconcilers.BaseReconciler - TargetRefReconciler reconcilers.TargetRefReconciler - RestMapper meta.RESTMapper -} - -//+kubebuilder:rbac:groups=kuadrant.io,resources=tlspolicies,verbs=get;list;watch;update;patch;delete -//+kubebuilder:rbac:groups=kuadrant.io,resources=tlspolicies/status,verbs=get;update;patch -//+kubebuilder:rbac:groups=kuadrant.io,resources=tlspolicies/finalizers,verbs=update -//+kubebuilder:rbac:groups="cert-manager.io",resources=issuers,verbs=get;list;watch; -//+kubebuilder:rbac:groups="cert-manager.io",resources=clusterissuers,verbs=get;list;watch; -//+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch -//+kubebuilder:rbac:groups="cert-manager.io",resources=certificates,verbs=get;list;watch;create;update;patch;delete - -func (r *TLSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := r.Logger().WithValues("TLSPolicy", req.NamespacedName) - log.Info("Reconciling TLSPolicy") - ctx = crlog.IntoContext(ctx, log) - - previous := &v1alpha1.TLSPolicy{} - if err := r.Client().Get(ctx, req.NamespacedName, previous); err != nil { - if err := client.IgnoreNotFound(err); err == nil { - return ctrl.Result{}, nil - } - return ctrl.Result{}, err - } - - tlsPolicy := previous.DeepCopy() - log.V(3).Info("TLSPolicyReconciler Reconcile", "tlsPolicy", tlsPolicy, "tlsPolicy.Spec", tlsPolicy.Spec) - - markedForDeletion := tlsPolicy.GetDeletionTimestamp() != nil - - targetReferenceObject, err := reconcilers.FetchTargetRefObject(ctx, r.Client(), tlsPolicy.GetTargetRef(), tlsPolicy.Namespace, tlsPolicy.TargetProgrammedGatewaysOnly()) - log.V(3).Info("TLSPolicyReconciler targetReferenceObject", "targetReferenceObject", targetReferenceObject) - if err != nil { - if !markedForDeletion { - if apierrors.IsNotFound(err) { - log.V(3).Info("Network object not found. Cleaning up") - delResErr := r.deleteResources(ctx, tlsPolicy, nil) - if delResErr == nil { - delResErr = err - } - return ctrl.Result{}, delResErr - } - return ctrl.Result{}, err - } - targetReferenceObject = nil // we need the object set to nil when there's an error, otherwise deleting the resources (when marked for deletion) will panic - } - - if markedForDeletion { - log.V(3).Info("cleaning up tls policy") - if controllerutil.ContainsFinalizer(tlsPolicy, TLSPolicyFinalizer) { - if err := r.deleteResources(ctx, tlsPolicy, targetReferenceObject); err != nil { - return ctrl.Result{}, err - } - if err := r.RemoveFinalizer(ctx, tlsPolicy, TLSPolicyFinalizer); err != nil { - return ctrl.Result{}, err - } - } - return ctrl.Result{}, nil - } - - // add finalizer to the tlsPolicy - if !controllerutil.ContainsFinalizer(tlsPolicy, TLSPolicyFinalizer) { - if err := r.AddFinalizer(ctx, tlsPolicy, TLSPolicyFinalizer); client.IgnoreNotFound(err) != nil { - return ctrl.Result{}, err - } - } - specErr := r.reconcileResources(ctx, tlsPolicy, targetReferenceObject) - - return ctrl.Result{}, specErr -} - -func (r *TLSPolicyReconciler) reconcileResources(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) error { - err := validateIssuer(ctx, r.Client(), tlsPolicy) - if err != nil { - return err - } - - // reconcile based on gateway diffs - gatewayDiffObj, err := reconcilers.ComputeGatewayDiffs(ctx, r.Client(), tlsPolicy, targetNetworkObject) - if err != nil { - return err - } - - if err = r.reconcileCertificates(ctx, tlsPolicy, gatewayDiffObj); err != nil { - return fmt.Errorf("reconcile Certificates error %w", err) - } - - // set direct back ref - i.e. claim the target network object as taken asap - if err = r.TargetRefReconciler.ReconcileTargetBackReference(ctx, tlsPolicy, targetNetworkObject, tlsPolicy.DirectReferenceAnnotationName()); err != nil { - return fmt.Errorf("reconcile TargetBackReference error %w", err) - } - - // set annotation of policies affecting the gateway - if err = r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, tlsPolicy, gatewayDiffObj); err != nil { - return fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err) - } - - return nil -} - -func (r *TLSPolicyReconciler) deleteResources(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) error { - // delete based on gateway diffs - gatewayDiffObj, err := reconcilers.ComputeGatewayDiffs(ctx, r.Client(), tlsPolicy, targetNetworkObject) - if err != nil { - return err - } - - if err := r.deleteCertificates(ctx, tlsPolicy); err != nil { - return err - } - - // remove direct back ref - if targetNetworkObject != nil { - if err := r.TargetRefReconciler.DeleteTargetBackReference(ctx, targetNetworkObject, tlsPolicy.DirectReferenceAnnotationName()); err != nil { - return err - } - } - - // update annotation of policies affecting the gateway - return r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, tlsPolicy, gatewayDiffObj) -} - -// SetupWithManager sets up the controller with the Manager. -func (r *TLSPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { - ok, err := kuadrantgatewayapi.IsGatewayAPIInstalled(mgr.GetRESTMapper()) - if err != nil { - return err - } - if !ok { - r.Logger().Info("TLSPolicy controller disabled. GatewayAPI was not found") - return nil - } - - ok, err = kuadrantgatewayapi.IsCertManagerInstalled(mgr.GetRESTMapper(), r.Logger()) - if err != nil { - return err - } - if !ok { - r.Logger().Info("TLSPolicy controller disabled. CertManager was not found") - return nil - } - - gatewayEventMapper := mappers.NewGatewayEventMapper( - v1alpha1.NewTLSPolicyType(), - mappers.WithLogger(r.Logger().WithName("gateway.mapper")), - mappers.WithClient(mgr.GetClient()), - ) - - return ctrl.NewControllerManagedBy(mgr). - For(&v1alpha1.TLSPolicy{}). - Owns(&certmanagerv1.Certificate{}). - Watches(&gatewayapiv1.Gateway{}, handler.EnqueueRequestsFromMapFunc(gatewayEventMapper.Map)). - Complete(r) -} diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 98c1c6d1d..ed21ae5f0 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -54,14 +54,6 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ return p, ok }) - store, ok := s.Load(TLSPolicyAcceptedKey) - if !ok { - logger.Error(errors.New("TLSPolicyAcceptedKey not found, skipping update of tls policy statuses"), "sync map error") - return nil - } - - isPolicyValidErrorMap := store.(map[string]error) - for _, policy := range policies { if policy.DeletionTimestamp != nil { logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) @@ -74,7 +66,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ ObservedGeneration: policy.Status.ObservedGeneration, } - err := isPolicyValidErrorMap[policy.GetLocator()] + _, err := IsPolicyValid(ctx, s, policy) meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(policy, err)) // Do not set enforced condition if Accepted condition is false diff --git a/main.go b/main.go index e9aa559e4..40c604343 100644 --- a/main.go +++ b/main.go @@ -216,21 +216,6 @@ func main() { os.Exit(1) } - tlsPolicyBaseReconciler := reconcilers.NewBaseReconciler( - mgr.GetClient(), mgr.GetScheme(), mgr.GetAPIReader(), - log.Log.WithName("tlspolicy"), - mgr.GetEventRecorderFor("TLSPolicy"), - ) - - if err = (&controllers.TLSPolicyReconciler{ - BaseReconciler: tlsPolicyBaseReconciler, - TargetRefReconciler: reconcilers.TargetRefReconciler{Client: mgr.GetClient()}, - RestMapper: mgr.GetRESTMapper(), - }).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "TLSPolicy") - os.Exit(1) - } - limitadorClusterEnvoyFilterBaseReconciler := reconcilers.NewBaseReconciler( mgr.GetClient(), mgr.GetScheme(), mgr.GetAPIReader(), log.Log.WithName("ratelimitpolicy").WithName("envoyfilter"), diff --git a/tests/common/tlspolicy/tlspolicy_controller_test.go b/tests/common/tlspolicy/tlspolicy_controller_test.go index 6eac28912..9223892c3 100644 --- a/tests/common/tlspolicy/tlspolicy_controller_test.go +++ b/tests/common/tlspolicy/tlspolicy_controller_test.go @@ -4,7 +4,6 @@ package tlspolicy import ( "context" - "encoding/json" "fmt" "time" @@ -178,21 +177,6 @@ var _ = Describe("TLSPolicy controller", func() { ) }, tests.TimeoutMedium, time.Second).Should(Succeed()) }, testTimeOut) - - It("should set gateway back reference and policy affected status", func(ctx SpecContext) { - policyBackRefValue := testNamespace + "/" + tlsPolicy.Name - refs, _ := json.Marshal([]client.ObjectKey{{Name: tlsPolicy.Name, Namespace: testNamespace}}) - policiesBackRefValue := string(refs) - - Eventually(func(g Gomega) { - gw := &gatewayapiv1.Gateway{} - err := k8sClient.Get(ctx, client.ObjectKey{Name: gateway.Name, Namespace: testNamespace}, gw) - //Check annotations - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(gw.Annotations).To(HaveKeyWithValue(v1alpha1.TLSPolicyDirectReferenceAnnotationName, policyBackRefValue)) - g.Expect(gw.Annotations).To(HaveKeyWithValue(v1alpha1.TLSPolicyBackReferenceAnnotationName, policiesBackRefValue)) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) - }, testTimeOut) }) Context("valid target, clusterissuer and policy", func() { From 332f7f80977f1646c5d0862a3ef5c58602f200c9 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 11 Oct 2024 11:08:47 +0100 Subject: [PATCH 02/13] refactor: use gateways again for now to check for expected certs Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 36 ++++--------------- controllers/tlspolicy_status_updater.go | 29 +++++++-------- 2 files changed, 19 insertions(+), 46 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index e85cf00e2..bc4e446c5 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -216,6 +216,11 @@ 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), @@ -227,37 +232,8 @@ 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], 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() + tlsHosts[secretRef] = append(tlsHosts[secretRef], hostname) } - // 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 ed21ae5f0..bd245aa2e 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -14,7 +14,6 @@ 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" @@ -107,7 +106,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) enforcedCondition(ctx context.Context return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) } - if err := t.isCertificatesReady(tlsPolicy, topology); err != nil { + if err := t.isCertificatesReady(ctx, tlsPolicy, topology); err != nil { return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) } @@ -180,7 +179,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl return nil } -func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { +func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(ctx context.Context, p machinery.Policy, topology *machinery.Topology) error { tlsPolicy, ok := p.(*kuadrantv1alpha1.TLSPolicy) if !ok { return errors.New("invalid policy") @@ -188,26 +187,24 @@ func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Polic // 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 - 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) + 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) }) - if len(listeners) == 0 { + if len(gateways) == 0 { return errors.New("no valid gateways found") } - 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) + // 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 _, cert := range expectedCertificates { - objs := topology.Objects().Children(l) + objs := topology.Objects().Children(gw) obj, ok := lo.Find(objs, func(o machinery.Object) bool { return o.GroupVersionKind().GroupKind() == CertManagerCertificateKind && o.GetNamespace() == cert.GetNamespace() && o.GetName() == cert.GetName() }) From 19a55d0b42698a30ac72bb02d30c01671c5853be Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 11 Oct 2024 15:18:09 +0100 Subject: [PATCH 03/13] refactor: tests for new invalid conditions for tls policy Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 2 +- controllers/tls_workflow.go | 2 +- controllers/tlspolicies_validator.go | 13 +++-- controllers/tlspolicy_status_updater.go | 2 +- .../tlspolicy/tlspolicy_controller_test.go | 58 +++++++++++++++++++ 5 files changed, 70 insertions(+), 7 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index bc4e446c5..8adfde8d0 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -76,7 +76,7 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont } // Policy is not valid - isValid, _ := IsPolicyValid(ctx, s, policy) + isValid, _ := IsTLSPolicyValid(ctx, s, policy) if !isValid { logger.V(1).Info("deleting certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) if err := t.deleteCertificatesForPolicy(ctx, topology, listeners); err != nil { diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 3baaeab63..2b327af54 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -166,7 +166,7 @@ func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { // Common functions used across multiple reconcilers -func IsPolicyValid(ctx context.Context, s *sync.Map, policy *kuadrantv1alpha1.TLSPolicy) (bool, error) { +func IsTLSPolicyValid(ctx context.Context, s *sync.Map, policy *kuadrantv1alpha1.TLSPolicy) (bool, error) { logger := controller.LoggerFromContext(ctx).WithName("IsPolicyValid") store, ok := s.Load(TLSPolicyAcceptedKey) diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index db5857dc8..3904f57b9 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "errors" "fmt" "sync" @@ -62,16 +63,20 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ } // TODO: What should happen if multiple target refs is supported in the future in terms of reporting in log and policy status? - // Policies are already linked to their targets, if is target ref length and length of targetables by this policy is the same + // Policies are already linked to their targets. If the target ref length and length of targetables by this policy is not the same, + // then the policy could not find the target if len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) { logger.V(1).Info("tls policy cannot find target ref", "name", p.Name, "namespace", p.Namespace) - isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrTargetNotFound(p.Kind(), p.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), p.GetName())) + isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrTargetNotFound(p.Kind(), p.GetTargetRef(), + apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), p.GetName())) continue } // Validate IssuerRef is correct if !lo.Contains([]string{"", certmanv1.IssuerKind, certmanv1.ClusterIssuerKind}, p.Spec.IssuerRef.Kind) { - isPolicyValidErrorMap[p.GetLocator()] = fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, p.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind) + isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrInvalid(p.Kind(), + fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, + p.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind)) continue } @@ -99,7 +104,7 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ }) if !ok { - isPolicyValidErrorMap[p.GetLocator()] = fmt.Errorf("unable to find issuer") + isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrInvalid(p.Kind(), errors.New("unable to find issuer")) continue } diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index bd245aa2e..378eb7a79 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -65,7 +65,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ ObservedGeneration: policy.Status.ObservedGeneration, } - _, err := IsPolicyValid(ctx, s, policy) + _, err := IsTLSPolicyValid(ctx, s, policy) meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(policy, err)) // Do not set enforced condition if Accepted condition is false diff --git a/tests/common/tlspolicy/tlspolicy_controller_test.go b/tests/common/tlspolicy/tlspolicy_controller_test.go index 9223892c3..92fe6cbbd 100644 --- a/tests/common/tlspolicy/tlspolicy_controller_test.go +++ b/tests/common/tlspolicy/tlspolicy_controller_test.go @@ -144,6 +144,64 @@ var _ = Describe("TLSPolicy controller", func() { }) + Context("valid target, invalid issuer", func() { + BeforeEach(func(ctx SpecContext) { + gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). + WithHTTPListener("test-listener", "test.example.com").Gateway + Expect(k8sClient.Create(ctx, gateway)).To(BeNil()) + }) + + It("invalid kind - should have accepted condition with status false and correct reason", func(ctx SpecContext) { + tlsPolicy = v1alpha1.NewTLSPolicy("test-tls-policy", testNamespace). + WithTargetGateway(gateway.Name). + WithIssuerRef(certmanmetav1.ObjectReference{Kind: "NotIssuer"}) + Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) + + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(tlsPolicy.Status.Conditions).To( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal(string(gatewayapiv1alpha2.PolicyReasonInvalid)), + "Message": Equal("TLSPolicy target is invalid: invalid value \"NotIssuer\" for issuerRef.kind. Must be empty, \"Issuer\" or \"ClusterIssuer\""), + })), + ) + g.Expect(tlsPolicy.Status.Conditions).ToNot( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(kuadrant.PolicyConditionEnforced)), + })), + ) + }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, testTimeOut) + + It("unable to find issuer - should have accepted condition with status false and correct reason", func(ctx SpecContext) { + tlsPolicy = v1alpha1.NewTLSPolicy("test-tls-policy", testNamespace). + WithTargetGateway(gateway.Name). + WithIssuerRef(certmanmetav1.ObjectReference{Name: "DoesNotExist"}) + Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) + + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(tlsPolicy.Status.Conditions).To( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal(string(gatewayapiv1alpha2.PolicyReasonInvalid)), + "Message": Equal("TLSPolicy target is invalid: unable to find issuer"), + })), + ) + g.Expect(tlsPolicy.Status.Conditions).ToNot( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(kuadrant.PolicyConditionEnforced)), + })), + ) + }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, testTimeOut) + }) + Context("valid target, issuer and policy", func() { BeforeEach(func(ctx SpecContext) { gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). From 8f5fc773fad06647537877b921b4f0743374c327 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 11 Oct 2024 16:15:12 +0100 Subject: [PATCH 04/13] refactor: validation methods Signed-off-by: KevFan --- controllers/tlspolicies_validator.go | 99 +++++++++++++++++----------- 1 file changed, 61 insertions(+), 38 deletions(-) diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index 3904f57b9..6da50c026 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -41,9 +41,8 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subs } func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("ValidateTLSPolicyTask").WithName("Reconcile") + logger := controller.LoggerFromContext(ctx).WithName("ValidateTLSPoliciesValidatorReconciler").WithName("Validate") - // Get all TLS Policies policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { p, ok := item.(*kuadrantv1alpha1.TLSPolicy) return p, ok @@ -62,49 +61,21 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ continue } - // TODO: What should happen if multiple target refs is supported in the future in terms of reporting in log and policy status? - // Policies are already linked to their targets. If the target ref length and length of targetables by this policy is not the same, - // then the policy could not find the target - if len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) { - logger.V(1).Info("tls policy cannot find target ref", "name", p.Name, "namespace", p.Namespace) - isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrTargetNotFound(p.Kind(), p.GetTargetRef(), - apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), p.GetName())) + // Validate target ref + if err := t.isTargetRefsFound(topology, p); err != nil { + isPolicyValidErrorMap[p.GetLocator()] = err continue } - // Validate IssuerRef is correct - if !lo.Contains([]string{"", certmanv1.IssuerKind, certmanv1.ClusterIssuerKind}, p.Spec.IssuerRef.Kind) { - isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrInvalid(p.Kind(), - fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, - p.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind)) + // Validate IssuerRef kind is correct + if err := t.isValidIssuerKind(p); err != nil { + isPolicyValidErrorMap[p.GetLocator()] = err continue } // Validate Issuer is present on cluster through the topology - _, ok := lo.Find(topology.Objects().Items(), func(item machinery.Object) bool { - runtimeObj, ok := item.(*controller.RuntimeObject) - if !ok { - return false - } - - issuer, ok := runtimeObj.Object.(certmanv1.GenericIssuer) - if !ok { - return false - } - - match := issuer.GetName() == p.Spec.IssuerRef.Name - if lo.Contains([]string{"", certmanv1.IssuerKind}, p.Spec.IssuerRef.Kind) { - match = match && issuer.GetNamespace() == p.GetNamespace() && - issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.IssuerKind - } else { - match = match && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.ClusterIssuerKind - } - - return match - }) - - if !ok { - isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrInvalid(p.Kind(), errors.New("unable to find issuer")) + if err := t.isIssuerFound(topology, p); err != nil { + isPolicyValidErrorMap[p.GetLocator()] = err continue } @@ -115,3 +86,55 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ return nil } + +// isTargetRefsFound Policies are already linked to their targets. If the target ref length and length of targetables by this policy is not the same, +// then the policy could not find the target +// TODO: What should happen if multiple target refs is supported in the future in terms of reporting in log and policy status? +func (t *ValidateTLSPoliciesValidatorReconciler) isTargetRefsFound(topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { + if len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) { + return kuadrant.NewErrTargetNotFound(p.Kind(), p.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), p.GetName())) + } + + return nil +} + +// isValidIssuerKind Validates that the Issuer Ref kind is either empty, Issuer or ClusterIssuer +func (t *ValidateTLSPoliciesValidatorReconciler) isValidIssuerKind(p *kuadrantv1alpha1.TLSPolicy) error { + if !lo.Contains([]string{"", certmanv1.IssuerKind, certmanv1.ClusterIssuerKind}, p.Spec.IssuerRef.Kind) { + return kuadrant.NewErrInvalid(p.Kind(), fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, + p.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind)) + } + + return nil +} + +// isIssuerFound Validates that the Issuer specified can be found in the topology +func (t *ValidateTLSPoliciesValidatorReconciler) isIssuerFound(topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { + _, ok := lo.Find(topology.Objects().Items(), func(item machinery.Object) bool { + runtimeObj, ok := item.(*controller.RuntimeObject) + if !ok { + return false + } + + issuer, ok := runtimeObj.Object.(certmanv1.GenericIssuer) + if !ok { + return false + } + + match := issuer.GetName() == p.Spec.IssuerRef.Name + if lo.Contains([]string{"", certmanv1.IssuerKind}, p.Spec.IssuerRef.Kind) { + match = match && issuer.GetNamespace() == p.GetNamespace() && + issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.IssuerKind + } else { + match = match && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.ClusterIssuerKind + } + + return match + }) + + if !ok { + return kuadrant.NewErrInvalid(p.Kind(), errors.New("unable to find issuer")) + } + + return nil +} From 8f8c0d182288f37d2c4a11998a54787b09440315 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 15 Oct 2024 14:27:08 +0100 Subject: [PATCH 05/13] fixup: deletes certs owned by policy when invalid instead Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index 8adfde8d0..f6e92c34d 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -20,6 +20,7 @@ import ( gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) type EffectiveTLSPoliciesReconciler struct { @@ -79,7 +80,7 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont isValid, _ := IsTLSPolicyValid(ctx, s, policy) if !isValid { logger.V(1).Info("deleting certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) - if err := t.deleteCertificatesForPolicy(ctx, topology, listeners); err != nil { + if err := t.deleteCertificatesForPolicy(ctx, topology, policy); err != nil { logger.Error(err, "unable to delete certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) } continue @@ -176,29 +177,29 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont return nil } -func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context.Context, topology *machinery.Topology, listeners []*machinery.Listener) error { +func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context.Context, topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("deleteCertificatesForPolicy") - for _, l := range listeners { - // Get children of listeners - objs := topology.Objects().Children(l) + certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { + r, ok := item.(*controller.RuntimeObject) + if !ok { + return nil, false + } + c, ok := r.Object.(*certmanv1.Certificate) + if !ok { + return nil, false + } - certs := lo.FilterMap(objs, func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { - c, ok := item.(*controller.RuntimeObject) - if !ok { - return nil, false - } - ce, ok := c.Object.(*certmanv1.Certificate) - return ce, ok - }) + // Only want certs owned by this policy + return c, utils.IsOwnedBy(c, p) + }) - for _, cert := range certs { - resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) + for _, cert := range certs { + resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) - if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil { - logger.Error(err, "delete certificate", "name", cert.Name) - return err - } + if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil { + logger.Error(err, "delete certificate", "name", cert.Name) + return err } } From 530a34d176f818c0cddf08c25067b1b5473435db Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 15 Oct 2024 14:33:49 +0100 Subject: [PATCH 06/13] fixup: delete orphaned certs from change in target ref -> invalid target ref Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index f6e92c34d..b210f5fed 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -60,6 +60,27 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont return ok }) + // Get all certs in topology for comparison with expected certs to determine orphaned certs later + certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { + r, ok := item.(*controller.RuntimeObject) + if !ok { + return nil, false + } + c, ok := r.Object.(*certmanv1.Certificate) + if !ok { + return nil, false + } + + // Only want certs owned by TLSPolicies + if isObjectOwnedByGroupKind(c, kuadrantv1alpha1.TLSPolicyGroupKind) { + return c, true + } + + return nil, false + }) + + var expectedCerts []*certmanv1.Certificate + for _, p := range policies { policy := p.(*kuadrantv1alpha1.TLSPolicy) @@ -87,27 +108,6 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont } // Policy is valid - // Get all certs in topology - certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { - r, ok := item.(*controller.RuntimeObject) - if !ok { - return nil, false - } - c, ok := r.Object.(*certmanv1.Certificate) - if !ok { - return nil, false - } - - // Only want certs owned by TLSPolicies - if isObjectOwnedByGroupKind(c, kuadrantv1alpha1.TLSPolicyGroupKind) { - return c, true - } - - return nil, false - }) - - var expectedCerts []*certmanv1.Certificate - for _, l := range listeners { // Need to use Gateway as listener hosts can be merged into a singular cert if using the same cert reference expectedCertificates := expectedCertificatesForGateway(ctx, l.Gateway.Gateway, policy) @@ -162,15 +162,15 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont } } } + } - // Clean up orphaned certs - orphanedCerts, _ := lo.Difference(certs, expectedCerts) - for _, orphanedCert := range orphanedCerts { - resource := t.client.Resource(CertManagerCertificatesResource).Namespace(orphanedCert.GetNamespace()) - if err := resource.Delete(ctx, orphanedCert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { - logger.Error(err, "unable to delete orphaned certificate", "policy", policy.Name) - continue - } + // Clean up orphaned certs + orphanedCerts, _ := lo.Difference(certs, expectedCerts) + for _, orphanedCert := range orphanedCerts { + resource := t.client.Resource(CertManagerCertificatesResource).Namespace(orphanedCert.GetNamespace()) + if err := resource.Delete(ctx, orphanedCert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + logger.Error(err, "unable to delete orphaned certificate", "name", orphanedCert.GetName(), "namespace", orphanedCert.GetNamespace(), "uid", orphanedCert.GetUID()) + continue } } From c0760785e4649b7c90ea9730856f8ad56f027227 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 15 Oct 2024 16:40:34 +0100 Subject: [PATCH 07/13] Revert "refactor: use gateways again for now to check for expected certs" This reverts commit 552d9f348f219dc77fe467c83bb4f73c2eb04f40. Signed-off-by: KevFan --- .../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() }) From bc3f71b461d693ac391aac7844819221fa65d07e Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 15 Oct 2024 16:57:51 +0100 Subject: [PATCH 08/13] fixup: log messages Signed-off-by: KevFan --- controllers/effective_tls_policies_reconciler.go | 3 --- controllers/tlspolicy_status_updater.go | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index 948283885..7098a739a 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -178,8 +178,6 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont } func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context.Context, topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { - logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("deleteCertificatesForPolicy") - certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { r, ok := item.(*controller.RuntimeObject) if !ok { @@ -198,7 +196,6 @@ func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil { - logger.Error(err, "delete certificate", "name", cert.Name) return err } } diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 45c8017a9..35c45df09 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -47,7 +47,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) Subscription() *controller.Subscripti } func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdaterReconciler").WithName("Reconcile") + logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdaterReconciler").WithName("UpdateStatus") policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { p, ok := item.(*kuadrantv1alpha1.TLSPolicy) From 59ab5ed655a9bba45548c934dca5d53e9da76544 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 15 Oct 2024 17:10:40 +0100 Subject: [PATCH 09/13] fixup: conventions naming and move functions Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 108 ++++++++++++++++- ...effective_tls_policies_reconciler_test.go} | 0 controllers/tls_workflow.go | 4 +- controllers/tlspolicies_validator.go | 26 ++-- controllers/tlspolicy_certmanager.go | 112 ------------------ controllers/tlspolicy_status_updater.go | 20 ++-- controllers/tlspolicy_status_updater_test.go | 2 +- 7 files changed, 132 insertions(+), 140 deletions(-) rename controllers/{tlspolicy_certmanager_test.go => effective_tls_policies_reconciler_test.go} (100%) delete mode 100644 controllers/tlspolicy_certmanager.go diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index 7098a739a..e6cf2601f 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -208,6 +208,11 @@ func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.G tlsHosts := make(map[corev1.ObjectReference][]string) for i, l := range gateway.Spec.Listeners { + hostname := "*" + if l.Hostname != nil { + hostname = string(*l.Hostname) + } + err := validateGatewayListenerBlock(field.NewPath("spec", "listeners").Index(i), l, gateway).ToAggregate() if err != nil { log.Info("Skipped a listener block: " + err.Error()) @@ -225,7 +230,7 @@ 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], string(*l.Hostname)) + tlsHosts[secretRef] = append(tlsHosts[secretRef], hostname) } } @@ -285,3 +290,104 @@ func buildCertManagerCertificate(tlsPolicy *kuadrantv1alpha1.TLSPolicy, secretRe translatePolicy(crt, tlsPolicy.Spec) return crt } + +// https://cert-manager.io/docs/usage/gateway/#supported-annotations +// Helper functions largely based on cert manager https://github.com/cert-manager/cert-manager/blob/master/pkg/controller/certificate-shim/sync.go + +func validateGatewayListenerBlock(path *field.Path, l gatewayapiv1.Listener, ingLike metav1.Object) field.ErrorList { + var errs field.ErrorList + + if l.Hostname == nil || *l.Hostname == "" { + errs = append(errs, field.Required(path.Child("hostname"), "the hostname cannot be empty")) + } + + if l.TLS == nil { + errs = append(errs, field.Required(path.Child("tls"), "the TLS block cannot be empty")) + return errs + } + + if len(l.TLS.CertificateRefs) == 0 { + errs = append(errs, field.Required(path.Child("tls").Child("certificateRef"), + "listener has no certificateRefs")) + } else { + // check that each CertificateRef is valid + for i, secretRef := range l.TLS.CertificateRefs { + if *secretRef.Group != "core" && *secretRef.Group != "" { + errs = append(errs, field.NotSupported(path.Child("tls").Child("certificateRef").Index(i).Child("group"), + *secretRef.Group, []string{"core", ""})) + } + + if *secretRef.Kind != "Secret" && *secretRef.Kind != "" { + errs = append(errs, field.NotSupported(path.Child("tls").Child("certificateRef").Index(i).Child("kind"), + *secretRef.Kind, []string{"Secret", ""})) + } + + if secretRef.Namespace != nil && string(*secretRef.Namespace) != ingLike.GetNamespace() { + errs = append(errs, field.Invalid(path.Child("tls").Child("certificateRef").Index(i).Child("namespace"), + *secretRef.Namespace, "cross-namespace secret references are not allowed in listeners")) + } + } + } + + if l.TLS.Mode == nil { + errs = append(errs, field.Required(path.Child("tls").Child("mode"), + "the mode field is required")) + } else { + if *l.TLS.Mode != gatewayapiv1.TLSModeTerminate { + errs = append(errs, field.NotSupported(path.Child("tls").Child("mode"), + *l.TLS.Mode, []string{string(gatewayapiv1.TLSModeTerminate)})) + } + } + + return errs +} + +// translatePolicy updates the Certificate spec using the TLSPolicy spec +// converted from https://github.com/cert-manager/cert-manager/blob/master/pkg/controller/certificate-shim/helper.go#L63 +func translatePolicy(crt *certmanv1.Certificate, tlsPolicy kuadrantv1alpha1.TLSPolicySpec) { + if tlsPolicy.CommonName != "" { + crt.Spec.CommonName = tlsPolicy.CommonName + } + + if tlsPolicy.Duration != nil { + crt.Spec.Duration = tlsPolicy.Duration + } + + if tlsPolicy.RenewBefore != nil { + crt.Spec.RenewBefore = tlsPolicy.RenewBefore + } + + if tlsPolicy.RenewBefore != nil { + crt.Spec.RenewBefore = tlsPolicy.RenewBefore + } + + if tlsPolicy.Usages != nil { + crt.Spec.Usages = tlsPolicy.Usages + } + + if tlsPolicy.RevisionHistoryLimit != nil { + crt.Spec.RevisionHistoryLimit = tlsPolicy.RevisionHistoryLimit + } + + if tlsPolicy.PrivateKey != nil { + if crt.Spec.PrivateKey == nil { + crt.Spec.PrivateKey = &certmanv1.CertificatePrivateKey{} + } + + if tlsPolicy.PrivateKey.Algorithm != "" { + crt.Spec.PrivateKey.Algorithm = tlsPolicy.PrivateKey.Algorithm + } + + if tlsPolicy.PrivateKey.Encoding != "" { + crt.Spec.PrivateKey.Encoding = tlsPolicy.PrivateKey.Encoding + } + + if tlsPolicy.PrivateKey.Size != 0 { + crt.Spec.PrivateKey.Size = tlsPolicy.PrivateKey.Size + } + + if tlsPolicy.PrivateKey.RotationPolicy != "" { + crt.Spec.PrivateKey.RotationPolicy = tlsPolicy.PrivateKey.RotationPolicy + } + } +} diff --git a/controllers/tlspolicy_certmanager_test.go b/controllers/effective_tls_policies_reconciler_test.go similarity index 100% rename from controllers/tlspolicy_certmanager_test.go rename to controllers/effective_tls_policies_reconciler_test.go diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 2b327af54..5e47ec1c9 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -35,11 +35,11 @@ var ( func NewTLSWorkflow(client *dynamic.DynamicClient, scheme *runtime.Scheme, isCertManagerInstalled bool) *controller.Workflow { return &controller.Workflow{ - Precondition: NewValidateTLSPoliciesValidatorReconciler(isCertManagerInstalled).Subscription().Reconcile, + Precondition: NewTLSPoliciesValidator(isCertManagerInstalled).Subscription().Reconcile, Tasks: []controller.ReconcileFunc{ NewEffectiveTLSPoliciesReconciler(client, scheme).Subscription().Reconcile, }, - Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).Subscription().Reconcile, + Postcondition: NewTLSPolicyStatusUpdater(client).Subscription().Reconcile, } } diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index 6da50c026..ff84c82d7 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -17,17 +17,17 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) -func NewValidateTLSPoliciesValidatorReconciler(isCertManagerInstalled bool) *ValidateTLSPoliciesValidatorReconciler { - return &ValidateTLSPoliciesValidatorReconciler{ +func NewTLSPoliciesValidator(isCertManagerInstalled bool) *TLSPoliciesValidator { + return &TLSPoliciesValidator{ isCertManagerInstalled: isCertManagerInstalled, } } -type ValidateTLSPoliciesValidatorReconciler struct { +type TLSPoliciesValidator struct { isCertManagerInstalled bool } -func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subscription { +func (t *TLSPoliciesValidator) Subscription() *controller.Subscription { return &controller.Subscription{ Events: []controller.ResourceEventMatcher{ {Kind: &machinery.GatewayGroupKind}, @@ -40,8 +40,8 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subs } } -func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("ValidateTLSPoliciesValidatorReconciler").WithName("Validate") +func (t *TLSPoliciesValidator) Validate(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("TLSPoliciesValidator").WithName("Validate") policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { p, ok := item.(*kuadrantv1alpha1.TLSPolicy) @@ -90,7 +90,7 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ // isTargetRefsFound Policies are already linked to their targets. If the target ref length and length of targetables by this policy is not the same, // then the policy could not find the target // TODO: What should happen if multiple target refs is supported in the future in terms of reporting in log and policy status? -func (t *ValidateTLSPoliciesValidatorReconciler) isTargetRefsFound(topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { +func (t *TLSPoliciesValidator) isTargetRefsFound(topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { if len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) { return kuadrant.NewErrTargetNotFound(p.Kind(), p.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), p.GetName())) } @@ -99,7 +99,7 @@ func (t *ValidateTLSPoliciesValidatorReconciler) isTargetRefsFound(topology *mac } // isValidIssuerKind Validates that the Issuer Ref kind is either empty, Issuer or ClusterIssuer -func (t *ValidateTLSPoliciesValidatorReconciler) isValidIssuerKind(p *kuadrantv1alpha1.TLSPolicy) error { +func (t *TLSPoliciesValidator) isValidIssuerKind(p *kuadrantv1alpha1.TLSPolicy) error { if !lo.Contains([]string{"", certmanv1.IssuerKind, certmanv1.ClusterIssuerKind}, p.Spec.IssuerRef.Kind) { return kuadrant.NewErrInvalid(p.Kind(), fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, p.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind)) @@ -109,7 +109,7 @@ func (t *ValidateTLSPoliciesValidatorReconciler) isValidIssuerKind(p *kuadrantv1 } // isIssuerFound Validates that the Issuer specified can be found in the topology -func (t *ValidateTLSPoliciesValidatorReconciler) isIssuerFound(topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { +func (t *TLSPoliciesValidator) isIssuerFound(topology *machinery.Topology, p *kuadrantv1alpha1.TLSPolicy) error { _, ok := lo.Find(topology.Objects().Items(), func(item machinery.Object) bool { runtimeObj, ok := item.(*controller.RuntimeObject) if !ok { @@ -121,15 +121,13 @@ func (t *ValidateTLSPoliciesValidatorReconciler) isIssuerFound(topology *machine return false } - match := issuer.GetName() == p.Spec.IssuerRef.Name + nameMatch := issuer.GetName() == p.Spec.IssuerRef.Name if lo.Contains([]string{"", certmanv1.IssuerKind}, p.Spec.IssuerRef.Kind) { - match = match && issuer.GetNamespace() == p.GetNamespace() && + return nameMatch && issuer.GetNamespace() == p.GetNamespace() && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.IssuerKind - } else { - match = match && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.ClusterIssuerKind } - return match + return nameMatch && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.ClusterIssuerKind }) if !ok { diff --git a/controllers/tlspolicy_certmanager.go b/controllers/tlspolicy_certmanager.go deleted file mode 100644 index 7cdee7de0..000000000 --- a/controllers/tlspolicy_certmanager.go +++ /dev/null @@ -1,112 +0,0 @@ -package controllers - -import ( - certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/validation/field" - gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" -) - -// https://cert-manager.io/docs/usage/gateway/#supported-annotations -// Helper functions largely based on cert manager https://github.com/cert-manager/cert-manager/blob/master/pkg/controller/certificate-shim/sync.go - -func validateGatewayListenerBlock(path *field.Path, l gatewayapiv1.Listener, ingLike metav1.Object) field.ErrorList { - var errs field.ErrorList - - if l.Hostname == nil || *l.Hostname == "" { - errs = append(errs, field.Required(path.Child("hostname"), "the hostname cannot be empty")) - } - - if l.TLS == nil { - errs = append(errs, field.Required(path.Child("tls"), "the TLS block cannot be empty")) - return errs - } - - if len(l.TLS.CertificateRefs) == 0 { - errs = append(errs, field.Required(path.Child("tls").Child("certificateRef"), - "listener has no certificateRefs")) - } else { - // check that each CertificateRef is valid - for i, secretRef := range l.TLS.CertificateRefs { - if *secretRef.Group != "core" && *secretRef.Group != "" { - errs = append(errs, field.NotSupported(path.Child("tls").Child("certificateRef").Index(i).Child("group"), - *secretRef.Group, []string{"core", ""})) - } - - if *secretRef.Kind != "Secret" && *secretRef.Kind != "" { - errs = append(errs, field.NotSupported(path.Child("tls").Child("certificateRef").Index(i).Child("kind"), - *secretRef.Kind, []string{"Secret", ""})) - } - - if secretRef.Namespace != nil && string(*secretRef.Namespace) != ingLike.GetNamespace() { - errs = append(errs, field.Invalid(path.Child("tls").Child("certificateRef").Index(i).Child("namespace"), - *secretRef.Namespace, "cross-namespace secret references are not allowed in listeners")) - } - } - } - - if l.TLS.Mode == nil { - errs = append(errs, field.Required(path.Child("tls").Child("mode"), - "the mode field is required")) - } else { - if *l.TLS.Mode != gatewayapiv1.TLSModeTerminate { - errs = append(errs, field.NotSupported(path.Child("tls").Child("mode"), - *l.TLS.Mode, []string{string(gatewayapiv1.TLSModeTerminate)})) - } - } - - return errs -} - -// translatePolicy updates the Certificate spec using the TLSPolicy spec -// converted from https://github.com/cert-manager/cert-manager/blob/master/pkg/controller/certificate-shim/helper.go#L63 -func translatePolicy(crt *certmanv1.Certificate, tlsPolicy v1alpha1.TLSPolicySpec) { - if tlsPolicy.CommonName != "" { - crt.Spec.CommonName = tlsPolicy.CommonName - } - - if tlsPolicy.Duration != nil { - crt.Spec.Duration = tlsPolicy.Duration - } - - if tlsPolicy.RenewBefore != nil { - crt.Spec.RenewBefore = tlsPolicy.RenewBefore - } - - if tlsPolicy.RenewBefore != nil { - crt.Spec.RenewBefore = tlsPolicy.RenewBefore - } - - if tlsPolicy.Usages != nil { - crt.Spec.Usages = tlsPolicy.Usages - } - - if tlsPolicy.RevisionHistoryLimit != nil { - crt.Spec.RevisionHistoryLimit = tlsPolicy.RevisionHistoryLimit - } - - if tlsPolicy.PrivateKey != nil { - if crt.Spec.PrivateKey == nil { - crt.Spec.PrivateKey = &certmanv1.CertificatePrivateKey{} - } - - if tlsPolicy.PrivateKey.Algorithm != "" { - crt.Spec.PrivateKey.Algorithm = tlsPolicy.PrivateKey.Algorithm - } - - if tlsPolicy.PrivateKey.Encoding != "" { - crt.Spec.PrivateKey.Encoding = tlsPolicy.PrivateKey.Encoding - } - - if tlsPolicy.PrivateKey.Size != 0 { - crt.Spec.PrivateKey.Size = tlsPolicy.PrivateKey.Size - } - - if tlsPolicy.PrivateKey.RotationPolicy != "" { - crt.Spec.PrivateKey.RotationPolicy = tlsPolicy.PrivateKey.RotationPolicy - } - } -} diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 35c45df09..82f7fe15e 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -24,15 +24,15 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) -type TLSPolicyStatusUpdaterReconciler struct { +type TLSPolicyStatusUpdater struct { Client *dynamic.DynamicClient } -func NewTLSPolicyStatusUpdaterReconciler(client *dynamic.DynamicClient) *TLSPolicyStatusUpdaterReconciler { - return &TLSPolicyStatusUpdaterReconciler{Client: client} +func NewTLSPolicyStatusUpdater(client *dynamic.DynamicClient) *TLSPolicyStatusUpdater { + return &TLSPolicyStatusUpdater{Client: client} } -func (t *TLSPolicyStatusUpdaterReconciler) Subscription() *controller.Subscription { +func (t *TLSPolicyStatusUpdater) Subscription() *controller.Subscription { return &controller.Subscription{ Events: []controller.ResourceEventMatcher{ {Kind: &machinery.GatewayGroupKind}, @@ -46,8 +46,8 @@ func (t *TLSPolicyStatusUpdaterReconciler) Subscription() *controller.Subscripti } } -func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdaterReconciler").WithName("UpdateStatus") +func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdater").WithName("UpdateStatus") policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { p, ok := item.(*kuadrantv1alpha1.TLSPolicy) @@ -102,7 +102,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ return nil } -func (t *TLSPolicyStatusUpdaterReconciler) enforcedCondition(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) *metav1.Condition { +func (t *TLSPolicyStatusUpdater) enforcedCondition(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) *metav1.Condition { if err := t.isIssuerReady(ctx, tlsPolicy, topology); err != nil { return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) } @@ -114,8 +114,8 @@ func (t *TLSPolicyStatusUpdaterReconciler) enforcedCondition(ctx context.Context return kuadrant.EnforcedCondition(tlsPolicy, nil, true) } -func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { - logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdaterReconciler").WithName("isIssuerReady") +func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { + logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdater").WithName("isIssuerReady") // Get all gateways gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { @@ -180,7 +180,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl return nil } -func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { +func (t *TLSPolicyStatusUpdater) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { tlsPolicy, ok := p.(*kuadrantv1alpha1.TLSPolicy) if !ok { return errors.New("invalid policy") diff --git a/controllers/tlspolicy_status_updater_test.go b/controllers/tlspolicy_status_updater_test.go index f43199e21..d482d82d4 100644 --- a/controllers/tlspolicy_status_updater_test.go +++ b/controllers/tlspolicy_status_updater_test.go @@ -461,7 +461,7 @@ func TestTLSPolicyStatusTask_enforcedCondition(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t1 *testing.T) { - t := &TLSPolicyStatusUpdaterReconciler{} + t := TLSPolicyStatusUpdater{} if got := t.enforcedCondition(context.Background(), tt.args.tlsPolicy, tt.args.topology(tt.args.tlsPolicy)); !reflect.DeepEqual(got, tt.want) { t1.Errorf("enforcedCondition() = %v, want %v", got, tt.want) } From b75f287dc75418d5811a10580f4bf3770ce6c1bd Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 16 Oct 2024 09:38:58 +0100 Subject: [PATCH 10/13] refactor: only reconcile tls policies that were affected by events Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 11 +- controllers/tls_workflow.go | 138 +++++++++++++----- controllers/tlspolicies_validator.go | 10 +- controllers/tlspolicy_status_updater.go | 68 +++++---- 4 files changed, 144 insertions(+), 83 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index e6cf2601f..9089e0ab6 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -51,14 +51,11 @@ func (t *EffectiveTLSPoliciesReconciler) Subscription() *controller.Subscription //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch //+kubebuilder:rbac:groups="cert-manager.io",resources=certificates,verbs=get;list;watch;create;update;patch;delete -func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, events []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("Reconcile") - // Get all TLS Policies - policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { - _, ok := item.(*kuadrantv1alpha1.TLSPolicy) - return ok - }) + // Get affected TLS Policies + policies := GetTLSPoliciesByEvents(topology, events) // Get all certs in topology for comparison with expected certs to determine orphaned certs later certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { @@ -135,7 +132,7 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []cont continue } _, err = resource.Create(ctx, un, metav1.CreateOptions{}) - if err != nil { + if err != nil && !apierrors.IsAlreadyExists(err) { logger.Error(err, "unable to create certificate", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) } diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 5e47ec1c9..46c01c6bf 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -17,6 +17,7 @@ import ( gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) const ( @@ -104,24 +105,7 @@ func LinkGatewayToIssuerFunc(objs controller.Store) machinery.LinkFunc { return p.Spec.IssuerRef.Name == issuer.GetName() && p.GetNamespace() == issuer.GetNamespace() && p.Spec.IssuerRef.Kind == certmanagerv1.IssuerKind }) - if len(linkedPolicies) == 0 { - return nil - } - - // Can infer linked gateways through the policy - linkedGateways := lo.Filter(gateways, func(g *gwapiv1.Gateway, index int) bool { - for _, l := range linkedPolicies { - if string(l.Spec.TargetRef.Name) == g.GetName() && g.GetNamespace() == l.GetNamespace() { - return true - } - } - - return false - }) - - return lo.Map(linkedGateways, func(item *gwapiv1.Gateway, index int) machinery.Object { - return &machinery.Gateway{Gateway: item} - }) + return findLinkedGatewaysForIssuer(linkedPolicies, gateways) }, } } @@ -142,26 +126,30 @@ func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { return p.Spec.IssuerRef.Name == clusterIssuer.GetName() && p.Spec.IssuerRef.Kind == certmanagerv1.ClusterIssuerKind }) - if len(linkedPolicies) == 0 { - return nil - } + return findLinkedGatewaysForIssuer(linkedPolicies, gateways) + }, + } +} - // Can infer linked gateways through the policy - linkedGateways := lo.Filter(gateways, func(g *gwapiv1.Gateway, index int) bool { - for _, l := range linkedPolicies { - if string(l.Spec.TargetRef.Name) == g.GetName() && g.GetNamespace() == l.GetNamespace() { - return true - } - } +func findLinkedGatewaysForIssuer(linkedPolicies []*kuadrantv1alpha1.TLSPolicy, gateways []*gwapiv1.Gateway) []machinery.Object { + if len(linkedPolicies) == 0 { + return nil + } - return false - }) + // Can infer linked gateways through the policy + linkedGateways := lo.Filter(gateways, func(g *gwapiv1.Gateway, index int) bool { + for _, l := range linkedPolicies { + if string(l.Spec.TargetRef.Name) == g.GetName() && g.GetNamespace() == l.GetNamespace() { + return true + } + } - return lo.Map(linkedGateways, func(item *gwapiv1.Gateway, index int) machinery.Object { - return &machinery.Gateway{Gateway: item} - }) - }, - } + return false + }) + + return lo.Map(linkedGateways, func(item *gwapiv1.Gateway, index int) machinery.Object { + return &machinery.Gateway{Gateway: item} + }) } // Common functions used across multiple reconcilers @@ -179,3 +167,83 @@ func IsTLSPolicyValid(ctx context.Context, s *sync.Map, policy *kuadrantv1alpha1 return isPolicyValidErrorMap[policy.GetLocator()] == nil, isPolicyValidErrorMap[policy.GetLocator()] } + +func GetTLSPoliciesByEvents(topology *machinery.Topology, events []controller.ResourceEvent) []machinery.Policy { + policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { + _, ok := item.(*kuadrantv1alpha1.TLSPolicy) + return ok + }) + + var affectedPolicies []machinery.Policy + for _, event := range events { + if event.Kind == machinery.GatewayGroupKind { + ob := event.NewObject + if ob == nil { + ob = event.OldObject + } + + g := machinery.Gateway{Gateway: ob.(*gwapiv1.Gateway)} + + affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { + for _, tg := range item.GetTargetRefs() { + if g.GetLocator() == tg.GetLocator() { + return true + } + } + return false + })...) + } + + if event.Kind == kuadrantv1alpha1.TLSPolicyGroupKind { + ob := event.NewObject + if ob == nil { + ob = event.OldObject + } + + affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { + return item.GetName() == ob.GetName() && item.GetNamespace() == ob.GetNamespace() + })...) + } + + if event.Kind == CertManagerCertificateKind { + ob := event.NewObject + if ob == nil { + ob = event.OldObject + } + + affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { + p := item.(*kuadrantv1alpha1.TLSPolicy) + return utils.IsOwnedBy(ob, p) + })...) + } + + if event.Kind == CertManagerIssuerKind { + ob := event.NewObject + if ob == nil { + ob = event.OldObject + } + + affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { + p := item.(*kuadrantv1alpha1.TLSPolicy) + + return ob.GetName() == p.Spec.IssuerRef.Name && lo.Contains([]string{"", certmanagerv1.IssuerKind}, p.Spec.IssuerRef.Kind) && + item.GetNamespace() == ob.GetNamespace() + })...) + } + + if event.Kind == CertManagerClusterIssuerKind { + ob := event.NewObject + if ob == nil { + ob = event.OldObject + } + + affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { + p := item.(*kuadrantv1alpha1.TLSPolicy) + return ob.GetName() == p.Spec.IssuerRef.Name && p.Spec.IssuerRef.Kind == certmanagerv1.ClusterIssuerKind + })...) + } + } + + // Return only unique policies as there can be duplicates from multiple events + return lo.Uniq(affectedPolicies) +} diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index ff84c82d7..a510ec603 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -40,17 +40,15 @@ func (t *TLSPoliciesValidator) Subscription() *controller.Subscription { } } -func (t *TLSPoliciesValidator) Validate(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t *TLSPoliciesValidator) Validate(ctx context.Context, events []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("TLSPoliciesValidator").WithName("Validate") - policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { - p, ok := item.(*kuadrantv1alpha1.TLSPolicy) - return p, ok - }) + policies := GetTLSPoliciesByEvents(topology, events) isPolicyValidErrorMap := make(map[string]error, len(policies)) - for _, p := range policies { + for _, policy := range policies { + p := policy.(*kuadrantv1alpha1.TLSPolicy) if p.DeletionTimestamp != nil { logger.V(1).Info("tls policy is marked for deletion, skipping", "name", p.Name, "namespace", p.Namespace) continue diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 82f7fe15e..d5b3077cf 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -46,45 +46,43 @@ func (t *TLSPolicyStatusUpdater) Subscription() *controller.Subscription { } } -func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, events []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdater").WithName("UpdateStatus") - policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { - p, ok := item.(*kuadrantv1alpha1.TLSPolicy) - return p, ok - }) + policies := GetTLSPoliciesByEvents(topology, events) for _, policy := range policies { - if policy.DeletionTimestamp != nil { - logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) + p := policy.(*kuadrantv1alpha1.TLSPolicy) + if p.DeletionTimestamp != nil { + logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", p.GetUID()) continue } newStatus := &kuadrantv1alpha1.TLSPolicyStatus{ // Copy initial conditions. Otherwise, status will always be updated - Conditions: slices.Clone(policy.Status.Conditions), - ObservedGeneration: policy.Status.ObservedGeneration, + Conditions: slices.Clone(p.Status.Conditions), + ObservedGeneration: p.Status.ObservedGeneration, } - _, err := IsTLSPolicyValid(ctx, s, policy) - meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(policy, err)) + _, err := IsTLSPolicyValid(ctx, s, p) + meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(p, err)) // Do not set enforced condition if Accepted condition is false if meta.IsStatusConditionFalse(newStatus.Conditions, string(gatewayapiv1alpha2.PolicyReasonAccepted)) { meta.RemoveStatusCondition(&newStatus.Conditions, string(kuadrant.PolicyConditionEnforced)) } else { - enforcedCond := t.enforcedCondition(ctx, policy, topology) + enforcedCond := t.enforcedCondition(ctx, p, topology) meta.SetStatusCondition(&newStatus.Conditions, *enforcedCond) } // Nothing to do - equalStatus := equality.Semantic.DeepEqual(newStatus, policy.Status) - if equalStatus && policy.Generation == policy.Status.ObservedGeneration { + equalStatus := equality.Semantic.DeepEqual(newStatus, p.Status) + if equalStatus && p.Generation == p.Status.ObservedGeneration { logger.V(1).Info("policy status unchanged, skipping update") continue } - newStatus.ObservedGeneration = policy.Generation - policy.Status = *newStatus + newStatus.ObservedGeneration = p.Generation + p.Status = *newStatus resource := t.Client.Resource(kuadrantv1alpha1.TLSPoliciesResource).Namespace(policy.GetNamespace()) un, err := controller.Destruct(policy) @@ -95,26 +93,26 @@ func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, _ []controlle _, err = resource.UpdateStatus(ctx, un, metav1.UpdateOptions{}) if err != nil { - logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) + logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", p.GetUID()) } } return nil } -func (t *TLSPolicyStatusUpdater) enforcedCondition(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) *metav1.Condition { - if err := t.isIssuerReady(ctx, tlsPolicy, topology); err != nil { - return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) +func (t *TLSPolicyStatusUpdater) enforcedCondition(ctx context.Context, policy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) *metav1.Condition { + if err := t.isIssuerReady(ctx, policy, topology); err != nil { + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), err), false) } - if err := t.isCertificatesReady(tlsPolicy, topology); err != nil { - return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) + if err := t.isCertificatesReady(policy, topology); err != nil { + return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policy.Kind(), err), false) } - return kuadrant.EnforcedCondition(tlsPolicy, nil, true) + return kuadrant.EnforcedCondition(policy, nil, true) } -func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { +func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, policy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdater").WithName("isIssuerReady") // Get all gateways @@ -125,26 +123,26 @@ func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, tlsPolicy *k // Find gateway defined by target ref gw, ok := lo.Find(gws, func(item *machinery.Gateway) bool { - if item.GetName() == string(tlsPolicy.GetTargetRef().Name) && item.GetNamespace() == tlsPolicy.GetNamespace() { + if item.GetName() == string(policy.GetTargetRef().Name) && item.GetNamespace() == policy.GetNamespace() { return true } return false }) if !ok { - return fmt.Errorf("unable to find target ref %s for policy %s in ns %s in topology", tlsPolicy.GetTargetRef(), tlsPolicy.Name, tlsPolicy.Namespace) + return fmt.Errorf("unable to find target ref %s for policy %s in ns %s in topology", policy.GetTargetRef(), policy.Name, policy.Namespace) } var conditions []certmanagerv1.IssuerCondition - switch tlsPolicy.Spec.IssuerRef.Kind { + switch policy.Spec.IssuerRef.Kind { case "", certmanagerv1.IssuerKind: objs := topology.Objects().Children(gw) obj, ok := lo.Find(objs, func(o machinery.Object) bool { - return o.GroupVersionKind().GroupKind() == CertManagerIssuerKind && o.GetNamespace() == tlsPolicy.GetNamespace() && o.GetName() == tlsPolicy.Spec.IssuerRef.Name + return o.GroupVersionKind().GroupKind() == CertManagerIssuerKind && o.GetNamespace() == policy.GetNamespace() && o.GetName() == policy.Spec.IssuerRef.Name }) if !ok { - err := fmt.Errorf("%s \"%s\" not found", tlsPolicy.Spec.IssuerRef.Kind, tlsPolicy.Spec.IssuerRef.Name) + err := fmt.Errorf("%s \"%s\" not found", policy.Spec.IssuerRef.Kind, policy.Spec.IssuerRef.Name) logger.Error(err, "error finding object in topology") return err } @@ -155,10 +153,10 @@ func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, tlsPolicy *k case certmanagerv1.ClusterIssuerKind: objs := topology.Objects().Children(gw) obj, ok := lo.Find(objs, func(o machinery.Object) bool { - return o.GroupVersionKind().GroupKind() == CertManagerClusterIssuerKind && o.GetName() == tlsPolicy.Spec.IssuerRef.Name + return o.GroupVersionKind().GroupKind() == CertManagerClusterIssuerKind && o.GetName() == policy.Spec.IssuerRef.Name }) if !ok { - err := fmt.Errorf("%s \"%s\" not found", tlsPolicy.Spec.IssuerRef.Kind, tlsPolicy.Spec.IssuerRef.Name) + err := fmt.Errorf("%s \"%s\" not found", policy.Spec.IssuerRef.Kind, policy.Spec.IssuerRef.Name) logger.Error(err, "error finding object in topology") return err } @@ -166,7 +164,7 @@ func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, tlsPolicy *k issuer := obj.(*controller.RuntimeObject).Object.(*certmanagerv1.ClusterIssuer) conditions = issuer.Status.Conditions default: - return fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, tlsPolicy.Spec.IssuerRef.Kind, certmanagerv1.IssuerKind, certmanagerv1.ClusterIssuerKind) + return fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, policy.Spec.IssuerRef.Kind, certmanagerv1.IssuerKind, certmanagerv1.ClusterIssuerKind) } transformedCond := utils.Map(conditions, func(c certmanagerv1.IssuerCondition) metav1.Condition { @@ -174,14 +172,14 @@ func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, tlsPolicy *k }) if !meta.IsStatusConditionTrue(transformedCond, string(certmanagerv1.IssuerConditionReady)) { - return fmt.Errorf("%s not ready", tlsPolicy.Spec.IssuerRef.Kind) + return fmt.Errorf("%s not ready", policy.Spec.IssuerRef.Kind) } return nil } func (t *TLSPolicyStatusUpdater) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { - tlsPolicy, ok := p.(*kuadrantv1alpha1.TLSPolicy) + policy, ok := p.(*kuadrantv1alpha1.TLSPolicy) if !ok { return errors.New("invalid policy") } @@ -204,7 +202,7 @@ func (t *TLSPolicyStatusUpdater) isCertificatesReady(p machinery.Policy, topolog continue } - expectedCertificates := expectedCertificatesForListener(l, tlsPolicy) + expectedCertificates := expectedCertificatesForListener(l, policy) for _, cert := range expectedCertificates { objs := topology.Objects().Children(l) From bd7fc23b117afa1557f44a2bcfa250f948648b39 Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 17 Oct 2024 20:53:25 +0100 Subject: [PATCH 11/13] refactor: integration tests for orphaned tests & address some comments Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 8 +- controllers/tls_workflow.go | 4 +- controllers/tlspolicy_status_updater.go | 9 +- .../tlspolicy/tlspolicy_controller_test.go | 138 +++++++++++++++--- 4 files changed, 131 insertions(+), 28 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index 9089e0ab6..f737e57f5 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -13,6 +13,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/dynamic" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -162,7 +163,10 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, events [ } // Clean up orphaned certs - orphanedCerts, _ := lo.Difference(certs, expectedCerts) + uniqueExpectedCerts := lo.UniqBy(expectedCerts, func(item *certmanv1.Certificate) types.UID { + return item.GetUID() + }) + orphanedCerts, _ := lo.Difference(certs, uniqueExpectedCerts) for _, orphanedCert := range orphanedCerts { resource := t.client.Resource(CertManagerCertificatesResource).Namespace(orphanedCert.GetNamespace()) if err := resource.Delete(ctx, orphanedCert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { @@ -192,7 +196,7 @@ func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context for _, cert := range certs { resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) - if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil { + if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { return err } } diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 46c01c6bf..20df50c01 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -245,5 +245,7 @@ func GetTLSPoliciesByEvents(topology *machinery.Topology, events []controller.Re } // Return only unique policies as there can be duplicates from multiple events - return lo.Uniq(affectedPolicies) + return lo.UniqBy(affectedPolicies, func(item machinery.Policy) string { + return item.GetLocator() + }) } diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index d5b3077cf..6982183f2 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -12,6 +12,7 @@ import ( "github.com/kuadrant/policy-machinery/machinery" "github.com/samber/lo" "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" @@ -92,7 +93,7 @@ func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, events []cont } _, err = resource.UpdateStatus(ctx, un, metav1.UpdateOptions{}) - if err != nil { + if err != nil && !apierrors.IsConflict(err) { logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", p.GetUID()) } } @@ -142,7 +143,11 @@ func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, policy *kuad return o.GroupVersionKind().GroupKind() == CertManagerIssuerKind && o.GetNamespace() == policy.GetNamespace() && o.GetName() == policy.Spec.IssuerRef.Name }) if !ok { - err := fmt.Errorf("%s \"%s\" not found", policy.Spec.IssuerRef.Kind, policy.Spec.IssuerRef.Name) + issuerRef := policy.Spec.IssuerRef.Kind + if issuerRef == "" { + issuerRef = certmanagerv1.IssuerKind + } + err := fmt.Errorf("%s \"%s\" not found", issuerRef, policy.Spec.IssuerRef.Name) logger.Error(err, "error finding object in topology") return err } diff --git a/tests/common/tlspolicy/tlspolicy_controller_test.go b/tests/common/tlspolicy/tlspolicy_controller_test.go index 92fe6cbbd..b199e7485 100644 --- a/tests/common/tlspolicy/tlspolicy_controller_test.go +++ b/tests/common/tlspolicy/tlspolicy_controller_test.go @@ -92,7 +92,7 @@ var _ = Describe("TLSPolicy controller", func() { "Type": Equal(string(kuadrant.PolicyConditionEnforced)), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) It("should have accepted condition with status true", func(ctx SpecContext) { @@ -112,7 +112,7 @@ var _ = Describe("TLSPolicy controller", func() { "Type": Equal(string(kuadrant.PolicyConditionEnforced)), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) By("creating a valid Gateway") gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). @@ -139,7 +139,7 @@ var _ = Describe("TLSPolicy controller", func() { "Message": Equal("TLSPolicy has been successfully enforced"), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) }) @@ -173,7 +173,7 @@ var _ = Describe("TLSPolicy controller", func() { "Type": Equal(string(kuadrant.PolicyConditionEnforced)), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) It("unable to find issuer - should have accepted condition with status false and correct reason", func(ctx SpecContext) { @@ -198,7 +198,7 @@ var _ = Describe("TLSPolicy controller", func() { "Type": Equal(string(kuadrant.PolicyConditionEnforced)), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) }) @@ -233,7 +233,7 @@ var _ = Describe("TLSPolicy controller", func() { "Message": Equal("TLSPolicy has been successfully enforced"), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) }) @@ -278,7 +278,7 @@ var _ = Describe("TLSPolicy controller", func() { "Message": Equal("TLSPolicy has been successfully enforced"), })), ) - }, tests.TimeoutMedium, time.Second).Should(Succeed()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) }) @@ -326,7 +326,7 @@ var _ = Describe("TLSPolicy controller", func() { ContainElements( HaveField("Name", "test-tls-secret"), )) - }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) + }, tests.TimeoutLong, time.Second, ctx).Should(Succeed()) }, testTimeOut) }) @@ -352,7 +352,7 @@ var _ = Describe("TLSPolicy controller", func() { ContainElements( HaveField("Name", "test-tls-secret"), )) - }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) + }, tests.TimeoutLong, time.Second, ctx).Should(Succeed()) }, testTimeOut) }) @@ -391,7 +391,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(cert1.Spec.DNSNames).To(ConsistOf("test1.example.com", "test2.example.com")) Expect(cert2.Spec.DNSNames).To(ConsistOf("test3.example.com")) - }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) + }, tests.TimeoutLong, time.Second, ctx).Should(Succeed()) }, testTimeOut) }) @@ -436,11 +436,38 @@ var _ = Describe("TLSPolicy controller", func() { Expect(cert1.Spec.DNSNames).To(ConsistOf("test1.example.com")) Expect(cert2.Spec.DNSNames).To(ConsistOf("test2.example.com")) Expect(cert3.Spec.DNSNames).To(ConsistOf("test3.example.com")) - }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) + }, tests.TimeoutLong, time.Second, ctx).Should(Succeed()) + }, testTimeOut) + + It("should delete all tls certificates when policy is deleted", func(ctx SpecContext) { + // confirm all expected certificates are present + Eventually(func() error { + certificateList := &certmanv1.CertificateList{} + Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) + if len(certificateList.Items) != 3 { + return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items)) + } + return nil + }, time.Second*60, time.Second).Should(BeNil()) + + // delete the tls policy + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, tlsPolicy))).ToNot(HaveOccurred()) + + // confirm all certificates have been deleted + Eventually(func() error { + certificateList := &certmanv1.CertificateList{} + if err := k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace}); err != nil { + return err + } + if len(certificateList.Items) != 0 { + return fmt.Errorf("expected 0 certificates, found: %v", len(certificateList.Items)) + } + return nil + }, tests.TimeoutLong, time.Second).Should(BeNil()) }, testTimeOut) It("should delete tls certificate when listener is removed", func(ctx SpecContext) { - //confirm all expected certificates are present + // confirm all expected certificates are present Eventually(func() error { certificateList := &certmanv1.CertificateList{} Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) @@ -450,12 +477,12 @@ var _ = Describe("TLSPolicy controller", func() { return nil }, time.Second*60, time.Second).Should(BeNil()) - //remove a listener + // remove a listener patch := client.MergeFrom(gateway.DeepCopy()) gateway.Spec.Listeners = gateway.Spec.Listeners[1:] Expect(k8sClient.Patch(ctx, gateway, patch)).To(BeNil()) - //confirm a certificate has been deleted + // confirm a certificate has been deleted Eventually(func() error { certificateList := &certmanv1.CertificateList{} if err := k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace}); err != nil { @@ -465,11 +492,11 @@ var _ = Describe("TLSPolicy controller", func() { return fmt.Errorf("expected 2 certificates, found: %v", len(certificateList.Items)) } return nil - }, tests.TimeoutMedium, time.Second).Should(BeNil()) + }, tests.TimeoutLong, time.Second).Should(BeNil()) }, testTimeOut) - It("should delete all tls certificates when tls policy is removed even if gateway is already removed", func(ctx SpecContext) { - //confirm all expected certificates are present + It("should delete all tls certificates when gateway is deleted", func(ctx SpecContext) { + // confirm all expected certificates are present Eventually(func() error { certificateList := &certmanv1.CertificateList{} Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) @@ -477,15 +504,12 @@ var _ = Describe("TLSPolicy controller", func() { return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items)) } return nil - }, time.Second*10, time.Second).Should(BeNil()) + }, time.Second*60, time.Second).Should(BeNil()) // delete the gateway Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, gateway))).ToNot(HaveOccurred()) - //delete the tls policy - Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, tlsPolicy))).ToNot(HaveOccurred()) - - //confirm all certificates have been deleted + // confirm all certificates have been deleted Eventually(func() error { certificateList := &certmanv1.CertificateList{} Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) @@ -493,7 +517,75 @@ var _ = Describe("TLSPolicy controller", func() { return fmt.Errorf("expected 0 certificates, found: %v", len(certificateList.Items)) } return nil + }, tests.TimeoutLong, time.Second).Should(BeNil()) + }, testTimeOut) + + It("Should delete orphaned tls certificates when changing to valid target ref", func(ctx SpecContext) { + // confirm all expected certificates are present + Eventually(func() error { + certificateList := &certmanv1.CertificateList{} + Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) + if len(certificateList.Items) != 3 { + return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items)) + } + return nil }, time.Second*60, time.Second).Should(BeNil()) + + // new gateway with one listener + gateway2 := tests.NewGatewayBuilder("test-gateway-2", gatewayClass.Name, testNamespace). + WithHTTPSListener("gateway2.example.com", "gateway2-tls-secret").Gateway + Expect(k8sClient.Create(ctx, gateway2)).To(BeNil()) + + // update tls policy target ref to new gateway + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy)).To(Succeed()) + tlsPolicy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gateway2.Name) + g.Expect(k8sClient.Update(ctx, tlsPolicy)).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + // confirm orphaned certs are deleted + Eventually(func() error { + certificateList := &certmanv1.CertificateList{} + Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) + if len(certificateList.Items) != 1 { + return fmt.Errorf("expected 1 certificates, found: %v", len(certificateList.Items)) + } + + if certificateList.Items[0].Name != "gateway2-tls-secret" { + return fmt.Errorf("expected certificate to be 'gateway2-tls-secret', found: %s", certificateList.Items[0].Name) + + } + return nil + }, tests.TimeoutLong, time.Second).Should(BeNil()) + }, testTimeOut) + + It("Should delete orphaned tls certificates when changing to invalid target ref", func(ctx SpecContext) { + // confirm all expected certificates are present + Eventually(func() error { + certificateList := &certmanv1.CertificateList{} + Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) + if len(certificateList.Items) != 3 { + return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items)) + } + return nil + }, time.Second*60, time.Second).Should(BeNil()) + + // update tlspolicy target ref to invalid reference + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy)).To(Succeed()) + tlsPolicy.Spec.TargetRef.Name = "does-not-exist" + g.Expect(k8sClient.Update(ctx, tlsPolicy)).To(Succeed()) + }).WithContext(ctx).Should(Succeed()) + + // confirm orphaned certs are deleted + Eventually(func() error { + certificateList := &certmanv1.CertificateList{} + Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil()) + if len(certificateList.Items) != 0 { + return fmt.Errorf("expected 0 certificates, found: %v", len(certificateList.Items)) + } + return nil + }, tests.TimeoutLong, time.Second).Should(BeNil()) }, testTimeOut) }) @@ -553,7 +645,7 @@ var _ = Describe("TLSPolicy controller", func() { certmanv1.UsageCertSign, )) Expect(cert1.Spec.RevisionHistoryLimit).To(PointTo(Equal(int32(1)))) - }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) + }, tests.TimeoutLong, time.Second, ctx).Should(Succeed()) }, testTimeOut) }) From 6c7b249157e5f092cf3f5da911327664e2a5dc89 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 18 Oct 2024 15:34:51 +0100 Subject: [PATCH 12/13] refactor: loop over listeners instead for effective policies Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/controllers/effective_tls_policies_reconciler.go b/controllers/effective_tls_policies_reconciler.go index f737e57f5..1e38e9ae7 100644 --- a/controllers/effective_tls_policies_reconciler.go +++ b/controllers/effective_tls_policies_reconciler.go @@ -52,11 +52,13 @@ func (t *EffectiveTLSPoliciesReconciler) Subscription() *controller.Subscription //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch //+kubebuilder:rbac:groups="cert-manager.io",resources=certificates,verbs=get;list;watch;create;update;patch;delete -func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, events []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("EffectiveTLSPoliciesReconciler").WithName("Reconcile") - // Get affected TLS Policies - policies := GetTLSPoliciesByEvents(topology, events) + listeners := topology.Targetables().Items(func(object machinery.Object) bool { + _, ok := object.(*machinery.Listener) + return ok + }) // Get all certs in topology for comparison with expected certs to determine orphaned certs later certs := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, index int) (*certmanv1.Certificate, bool) { @@ -79,34 +81,34 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, events [ var expectedCerts []*certmanv1.Certificate - for _, p := range policies { - policy := p.(*kuadrantv1alpha1.TLSPolicy) - - // Get all listeners where the gateway contains this policy - // TODO: Update when targeting by section name is allowed, the listener will contain the policy rather than the gateway - 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) - }) + for _, listener := range listeners { + l := listener.(*machinery.Listener) - // Policy is deleted - if policy.DeletionTimestamp != nil { - logger.V(1).Info("policy is marked for deletion, nothing to do", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) - continue + policies := l.Policies() + if len(policies) == 0 { + policies = l.Gateway.Policies() } - // Policy is not valid - isValid, _ := IsTLSPolicyValid(ctx, s, policy) - if !isValid { - logger.V(1).Info("deleting certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) - if err := t.deleteCertificatesForPolicy(ctx, topology, policy); err != nil { - logger.Error(err, "unable to delete certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + for _, p := range policies { + policy := p.(*kuadrantv1alpha1.TLSPolicy) + + // Policy is deleted + if policy.DeletionTimestamp != nil { + logger.V(1).Info("policy is marked for deletion, nothing to do", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + continue + } + + // Policy is not valid + isValid, _ := IsTLSPolicyValid(ctx, s, policy) + if !isValid { + logger.V(1).Info("deleting certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + if err := t.deleteCertificatesForPolicy(ctx, topology, policy); err != nil { + logger.Error(err, "unable to delete certs for invalid policy", "name", policy.Name, "namespace", policy.Namespace, "uid", policy.GetUID()) + } + continue } - continue - } - // Policy is valid - for _, l := range listeners { + // Policy is valid // Need to use Gateway as listener hosts can be merged into a singular cert if using the same cert reference expectedCertificates := expectedCertificatesForGateway(ctx, l.Gateway.Gateway, policy) From d8d388d09b5b2a9dfb2a6bfe4c975e0890953257 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 18 Oct 2024 16:00:14 +0100 Subject: [PATCH 13/13] revert: filtering policies by events received Signed-off-by: KevFan --- controllers/tls_workflow.go | 83 ------------------------- controllers/tlspolicies_validator.go | 7 ++- controllers/tlspolicy_status_updater.go | 7 ++- 3 files changed, 10 insertions(+), 87 deletions(-) diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 20df50c01..c1114f639 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -17,7 +17,6 @@ import ( gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) const ( @@ -167,85 +166,3 @@ func IsTLSPolicyValid(ctx context.Context, s *sync.Map, policy *kuadrantv1alpha1 return isPolicyValidErrorMap[policy.GetLocator()] == nil, isPolicyValidErrorMap[policy.GetLocator()] } - -func GetTLSPoliciesByEvents(topology *machinery.Topology, events []controller.ResourceEvent) []machinery.Policy { - policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { - _, ok := item.(*kuadrantv1alpha1.TLSPolicy) - return ok - }) - - var affectedPolicies []machinery.Policy - for _, event := range events { - if event.Kind == machinery.GatewayGroupKind { - ob := event.NewObject - if ob == nil { - ob = event.OldObject - } - - g := machinery.Gateway{Gateway: ob.(*gwapiv1.Gateway)} - - affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { - for _, tg := range item.GetTargetRefs() { - if g.GetLocator() == tg.GetLocator() { - return true - } - } - return false - })...) - } - - if event.Kind == kuadrantv1alpha1.TLSPolicyGroupKind { - ob := event.NewObject - if ob == nil { - ob = event.OldObject - } - - affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { - return item.GetName() == ob.GetName() && item.GetNamespace() == ob.GetNamespace() - })...) - } - - if event.Kind == CertManagerCertificateKind { - ob := event.NewObject - if ob == nil { - ob = event.OldObject - } - - affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { - p := item.(*kuadrantv1alpha1.TLSPolicy) - return utils.IsOwnedBy(ob, p) - })...) - } - - if event.Kind == CertManagerIssuerKind { - ob := event.NewObject - if ob == nil { - ob = event.OldObject - } - - affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { - p := item.(*kuadrantv1alpha1.TLSPolicy) - - return ob.GetName() == p.Spec.IssuerRef.Name && lo.Contains([]string{"", certmanagerv1.IssuerKind}, p.Spec.IssuerRef.Kind) && - item.GetNamespace() == ob.GetNamespace() - })...) - } - - if event.Kind == CertManagerClusterIssuerKind { - ob := event.NewObject - if ob == nil { - ob = event.OldObject - } - - affectedPolicies = append(affectedPolicies, lo.Filter(policies, func(item machinery.Policy, index int) bool { - p := item.(*kuadrantv1alpha1.TLSPolicy) - return ob.GetName() == p.Spec.IssuerRef.Name && p.Spec.IssuerRef.Kind == certmanagerv1.ClusterIssuerKind - })...) - } - } - - // Return only unique policies as there can be duplicates from multiple events - return lo.UniqBy(affectedPolicies, func(item machinery.Policy) string { - return item.GetLocator() - }) -} diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index a510ec603..419b1e61a 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -40,10 +40,13 @@ func (t *TLSPoliciesValidator) Subscription() *controller.Subscription { } } -func (t *TLSPoliciesValidator) Validate(ctx context.Context, events []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t *TLSPoliciesValidator) Validate(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("TLSPoliciesValidator").WithName("Validate") - policies := GetTLSPoliciesByEvents(topology, events) + policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { + _, ok := item.(*kuadrantv1alpha1.TLSPolicy) + return ok + }) isPolicyValidErrorMap := make(map[string]error, len(policies)) diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 6982183f2..c71e35dda 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -47,10 +47,13 @@ func (t *TLSPolicyStatusUpdater) Subscription() *controller.Subscription { } } -func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, events []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdater").WithName("UpdateStatus") - policies := GetTLSPoliciesByEvents(topology, events) + policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { + _, ok := item.(*kuadrantv1alpha1.TLSPolicy) + return ok + }) for _, policy := range policies { p := policy.(*kuadrantv1alpha1.TLSPolicy)