From aba1a78d3893992ebf37e51b13e9d9b225cab701 Mon Sep 17 00:00:00 2001 From: Kevin Fan Date: Mon, 21 Oct 2024 16:00:18 +0100 Subject: [PATCH] refactor: effective tls policies reconciler (#927) * refactor: effective tls policies reconciler Signed-off-by: KevFan * refactor: use gateways again for now to check for expected certs Signed-off-by: KevFan * refactor: tests for new invalid conditions for tls policy Signed-off-by: KevFan * refactor: validation methods Signed-off-by: KevFan * fixup: deletes certs owned by policy when invalid instead Signed-off-by: KevFan * fixup: delete orphaned certs from change in target ref -> invalid target ref Signed-off-by: KevFan * Revert "refactor: use gateways again for now to check for expected certs" This reverts commit 552d9f348f219dc77fe467c83bb4f73c2eb04f40. Signed-off-by: KevFan * fixup: log messages Signed-off-by: KevFan * fixup: conventions naming and move functions Signed-off-by: KevFan * refactor: only reconcile tls policies that were affected by events Signed-off-by: KevFan * refactor: integration tests for orphaned tests & address some comments Signed-off-by: KevFan * refactor: loop over listeners instead for effective policies Signed-off-by: KevFan * revert: filtering policies by events received Signed-off-by: KevFan * fixup: address comments Signed-off-by: KevFan * fixup: filter for tls policies when looping over policies Signed-off-by: KevFan --------- Signed-off-by: KevFan --- .../effective_tls_policies_reconciler.go | 393 ++++++++++++++++++ ...effective_tls_policies_reconciler_test.go} | 0 controllers/state_of_the_world.go | 2 +- controllers/test_common.go | 14 - controllers/tls_workflow.go | 96 +++-- controllers/tlspolicies_validator.go | 96 ++++- controllers/tlspolicy_certmanager.go | 132 ------ .../tlspolicy_certmanager_certificates.go | 233 ----------- controllers/tlspolicy_controller.go | 197 --------- controllers/tlspolicy_status_updater.go | 98 +++-- controllers/tlspolicy_status_updater_test.go | 2 +- main.go | 15 - .../tlspolicy/tlspolicy_controller_test.go | 208 +++++++-- 13 files changed, 747 insertions(+), 739 deletions(-) create mode 100644 controllers/effective_tls_policies_reconciler.go rename controllers/{tlspolicy_certmanager_test.go => effective_tls_policies_reconciler_test.go} (100%) delete mode 100644 controllers/tlspolicy_certmanager.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..1cf26ca4a --- /dev/null +++ b/controllers/effective_tls_policies_reconciler.go @@ -0,0 +1,393 @@ +package controllers + +import ( + "context" + "errors" + "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/types" + "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") + + 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 + // Only certs owned by TLSPolicies should be in the topology - no need to check again + 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) + + return c, ok + }) + + var expectedCerts []*certmanv1.Certificate + filterForTLSPolicies := func(p machinery.Policy, _ int) bool { + _, ok := p.(*kuadrantv1alpha1.TLSPolicy) + return ok + } + + for _, listener := range listeners { + l := listener.(*machinery.Listener) + + policies := lo.Filter(l.Policies(), filterForTLSPolicies) + + if len(policies) == 0 { + policies = lo.Filter(l.Gateway.Policies(), filterForTLSPolicies) + } + + 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.deleteCertificatesForTargetable(ctx, topology, l); 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 + // 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 && !apierrors.IsAlreadyExists(err) { + 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 + 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) { + logger.Error(err, "unable to delete orphaned certificate", "name", orphanedCert.GetName(), "namespace", orphanedCert.GetNamespace(), "uid", orphanedCert.GetUID()) + continue + } + } + + return nil +} + +func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForTargetable(ctx context.Context, topology *machinery.Topology, target machinery.Targetable) error { + children := topology.Objects().Children(target) + + certs := lo.FilterMap(children, 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) + + return c, ok + }) + + var deletionErr error + for _, cert := range certs { + resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace()) + + if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + deletionErr = errors.Join(err) + } + } + + return deletionErr +} + +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 { + 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()) + 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], 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 +} + +// 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/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..c1114f639 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, - Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).Subscription().Reconcile, + Precondition: NewTLSPoliciesValidator(isCertManagerInstalled).Subscription().Reconcile, + Tasks: []controller.ReconcileFunc{ + NewEffectiveTLSPoliciesReconciler(client, scheme).Subscription().Reconcile, + }, + Postcondition: NewTLSPolicyStatusUpdater(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 + }) }, } } @@ -95,24 +104,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) }, } } @@ -133,24 +125,44 @@ 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) + }, + } +} + +func findLinkedGatewaysForIssuer(linkedPolicies []*kuadrantv1alpha1.TLSPolicy, gateways []*gwapiv1.Gateway) []machinery.Object { + 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 } + } - // 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 false - }) + return lo.Map(linkedGateways, func(item *gwapiv1.Gateway, index int) machinery.Object { + return &machinery.Gateway{Gateway: item} + }) +} - return lo.Map(linkedGateways, func(item *gwapiv1.Gateway, index int) machinery.Object { - return &machinery.Gateway{Gateway: item} - }) - }, +// Common functions used across multiple reconcilers + +func IsTLSPolicyValid(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..419b1e61a 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -2,8 +2,11 @@ package controllers import ( "context" + "errors" + "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" @@ -14,23 +17,22 @@ 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}, {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.CreateEvent)}, {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.UpdateEvent)}, - {Kind: &CertManagerCertificateKind}, {Kind: &CertManagerIssuerKind}, {Kind: &CertManagerClusterIssuerKind}, }, @@ -38,18 +40,18 @@ 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") +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") - // 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 + 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)) - 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 @@ -60,11 +62,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 is target ref length and length of targetables by this policy is the same - 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 kind is correct + if err := t.isValidIssuerKind(p); err != nil { + isPolicyValidErrorMap[p.GetLocator()] = err + continue + } + + // Validate Issuer is present on cluster through the topology + if err := t.isIssuerFound(topology, p); err != nil { + isPolicyValidErrorMap[p.GetLocator()] = err continue } @@ -75,3 +87,53 @@ 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 *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())) + } + + return nil +} + +// isValidIssuerKind Validates that the Issuer Ref kind is either empty, Issuer or ClusterIssuer +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)) + } + + return nil +} + +// isIssuerFound Validates that the Issuer specified can be found in the topology +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 { + return false + } + + issuer, ok := runtimeObj.Object.(certmanv1.GenericIssuer) + if !ok { + return false + } + + nameMatch := issuer.GetName() == p.Spec.IssuerRef.Name + if lo.Contains([]string{"", certmanv1.IssuerKind}, p.Spec.IssuerRef.Kind) { + return nameMatch && issuer.GetNamespace() == p.GetNamespace() && + issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.IssuerKind + } + + return nameMatch && issuer.GetObjectKind().GroupVersionKind().Kind == certmanv1.ClusterIssuerKind + }) + + if !ok { + return kuadrant.NewErrInvalid(p.Kind(), errors.New("unable to find issuer")) + } + + return nil +} diff --git a/controllers/tlspolicy_certmanager.go b/controllers/tlspolicy_certmanager.go deleted file mode 100644 index c9cf53ded..000000000 --- a/controllers/tlspolicy_certmanager.go +++ /dev/null @@ -1,132 +0,0 @@ -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" -) - -// 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 - } - } -} - -// 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..c71e35dda 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" @@ -24,15 +25,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,53 +47,46 @@ 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") +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) - return p, ok + policies := lo.Filter(topology.Policies().Items(), func(item machinery.Policy, index int) bool { + _, ok := item.(*kuadrantv1alpha1.TLSPolicy) + return 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()) + 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 := isPolicyValidErrorMap[policy.GetLocator()] - 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) @@ -102,28 +96,28 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ } _, 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()) + if err != nil && !apierrors.IsConflict(err) { + logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", p.GetUID()) } } return nil } -func (t *TLSPolicyStatusUpdaterReconciler) 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 *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, policy *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) { @@ -133,26 +127,30 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl // 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) + 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 } @@ -163,10 +161,10 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl 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 } @@ -174,7 +172,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl 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 { @@ -182,14 +180,14 @@ func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tl }) 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 *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { - tlsPolicy, ok := p.(*kuadrantv1alpha1.TLSPolicy) +func (t *TLSPolicyStatusUpdater) isCertificatesReady(p machinery.Policy, topology *machinery.Topology) error { + policy, ok := p.(*kuadrantv1alpha1.TLSPolicy) if !ok { return errors.New("invalid policy") } @@ -212,7 +210,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Polic continue } - expectedCertificates := expectedCertificatesForListener(l, tlsPolicy) + expectedCertificates := expectedCertificatesForListener(l, policy) for _, cert := range expectedCertificates { objs := topology.Objects().Children(l) 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) } 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..b199e7485 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" @@ -93,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) { @@ -113,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). @@ -140,11 +139,69 @@ 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) }) + 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.TimeoutLong, 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.TimeoutLong, time.Second).Should(Succeed()) + }, testTimeOut) + }) + Context("valid target, issuer and policy", func() { BeforeEach(func(ctx SpecContext) { gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). @@ -176,22 +233,7 @@ var _ = Describe("TLSPolicy controller", func() { "Message": Equal("TLSPolicy has been successfully enforced"), })), ) - }, 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()) + }, tests.TimeoutLong, time.Second).Should(Succeed()) }, testTimeOut) }) @@ -236,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) }) @@ -284,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) }) @@ -310,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) }) @@ -349,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) }) @@ -394,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()) @@ -408,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 { @@ -423,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()) @@ -435,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()) @@ -451,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) }) @@ -511,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) })