From b6fe7bda537cd44e9b03c15fd8da597ff0e6e134 Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 26 Sep 2024 16:00:11 +0100 Subject: [PATCH 01/16] refactor: tls policy status to state of the world tasks Signed-off-by: KevFan --- controllers/state_of_the_world.go | 34 +- controllers/tls_workflow.go | 8 +- .../tlspolicy_certmanager_certificates.go | 8 +- controllers/tlspolicy_controller.go | 68 +--- controllers/tlspolicy_links.go | 86 ++++ controllers/tlspolicy_mappers.go | 59 --- controllers/tlspolicy_mappers_test.go | 223 ----------- controllers/tlspolicy_status.go | 162 -------- controllers/tlspolicy_status_test.go | 367 ------------------ controllers/tlspolicy_tasks.go | 318 +++++++++++++++ controllers/tlspolicy_tasks_test.go | 35 ++ go.sum | 6 - .../tlspolicy/tlspolicy_controller_test.go | 93 ++--- tests/commons.go | 6 - 14 files changed, 525 insertions(+), 948 deletions(-) create mode 100644 controllers/tlspolicy_links.go delete mode 100644 controllers/tlspolicy_mappers.go delete mode 100644 controllers/tlspolicy_mappers_test.go delete mode 100644 controllers/tlspolicy_status.go delete mode 100644 controllers/tlspolicy_status_test.go create mode 100644 controllers/tlspolicy_tasks.go create mode 100644 controllers/tlspolicy_tasks_test.go diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 6ec8f6671..8e9a8d5f6 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -2,6 +2,7 @@ package controllers import ( "fmt" + "reflect" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" egv1alpha1 "github.com/envoyproxy/gateway/api/v1alpha1" @@ -19,6 +20,7 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/utils/env" ctrlruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/event" ctrlruntimepredicate "sigs.k8s.io/controller-runtime/pkg/predicate" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -203,24 +205,42 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D controller.WithRunnable("certificate watcher", controller.Watch( &certmanagerv1.Certificate{}, CertManagerCertificatesResource, - metav1.NamespaceAll, - )), + metav1.NamespaceAll), + ), controller.WithRunnable("issuers watcher", controller.Watch( &certmanagerv1.Issuer{}, CertManagerIssuersResource, metav1.NamespaceAll, - )), + controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.Issuer]{ + UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.Issuer]) bool { + oldStatus := e.ObjectOld.GetStatus() + newStatus := e.ObjectOld.GetStatus() + return !reflect.DeepEqual(oldStatus, newStatus) + }, + })), + ), controller.WithRunnable("clusterissuers watcher", controller.Watch( - &certmanagerv1.Certificate{}, + &certmanagerv1.ClusterIssuer{}, CertMangerClusterIssuersResource, metav1.NamespaceAll, - )), + controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.ClusterIssuer]{ + UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.ClusterIssuer]) bool { + oldStatus := e.ObjectOld.GetStatus() + newStatus := e.ObjectOld.GetStatus() + return !reflect.DeepEqual(oldStatus, newStatus) + }, + })), + ), controller.WithObjectKinds( CertManagerCertificateKind, CertManagerIssuerKind, CertManagerClusterIssuerKind, ), - // TODO: add object links + controller.WithObjectLinks( + LinkGatewayToCertificateFunc, + LinkGatewayToIssuerFunc, + LinkGatewayToClusterIssuerFunc, + ), ) // TODO: add tls policy specific tasks to workflow } @@ -249,7 +269,7 @@ func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, NewAuthorinoReconciler(client).Subscription().Reconcile, NewLimitadorReconciler(client).Subscription().Reconcile, NewDNSWorkflow().Run, - NewTLSWorkflow().Run, + NewTLSWorkflow(client).Run, NewAuthWorkflow().Run, NewRateLimitWorkflow().Run, }, diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 40c1f4f68..c8efd7627 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -2,8 +2,12 @@ package controllers import ( "github.com/kuadrant/policy-machinery/controller" + "k8s.io/client-go/dynamic" ) -func NewTLSWorkflow() *controller.Workflow { - return &controller.Workflow{} +func NewTLSWorkflow(client *dynamic.DynamicClient) *controller.Workflow { + return &controller.Workflow{ + Precondition: NewValidateTLSPolicyTask().Reconcile, + Postcondition: NewTLSPolicyStatusTask(client).Reconcile, + } } diff --git a/controllers/tlspolicy_certmanager_certificates.go b/controllers/tlspolicy_certmanager_certificates.go index c9c65aa52..4a98e6656 100644 --- a/controllers/tlspolicy_certmanager_certificates.go +++ b/controllers/tlspolicy_certmanager_certificates.go @@ -33,7 +33,7 @@ func (r *TLSPolicyReconciler) reconcileCertificates(ctx context.Context, tlsPoli // 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 := r.expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) + 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) } @@ -102,7 +102,7 @@ func (r *TLSPolicyReconciler) deleteUnexpectedCertificates(ctx context.Context, return nil } -func (r *TLSPolicyReconciler) expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) []*certmanv1.Certificate { +func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) []*certmanv1.Certificate { log := crlog.FromContext(ctx) tlsHosts := make(map[corev1.ObjectReference][]string) @@ -130,12 +130,12 @@ func (r *TLSPolicyReconciler) expectedCertificatesForGateway(ctx context.Context certs := make([]*certmanv1.Certificate, 0, len(tlsHosts)) for secretRef, hosts := range tlsHosts { - certs = append(certs, r.buildCertManagerCertificate(gateway, tlsPolicy, secretRef, hosts)) + certs = append(certs, buildCertManagerCertificate(gateway, tlsPolicy, secretRef, hosts)) } return certs } -func (r *TLSPolicyReconciler) buildCertManagerCertificate(gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy, secretRef corev1.ObjectReference, hosts []string) *certmanv1.Certificate { +func buildCertManagerCertificate(gateway *gatewayapiv1.Gateway, tlsPolicy *v1alpha1.TLSPolicy, secretRef corev1.ObjectReference, hosts []string) *certmanv1.Certificate { tlsCertLabels := commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), tlsPolicy) crt := &certmanv1.Certificate{ diff --git a/controllers/tlspolicy_controller.go b/controllers/tlspolicy_controller.go index 90ba0bc7d..723d0e263 100644 --- a/controllers/tlspolicy_controller.go +++ b/controllers/tlspolicy_controller.go @@ -19,43 +19,25 @@ package controllers import ( "context" "fmt" - "reflect" - "github.com/cert-manager/cert-manager/pkg/apis/certmanager" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" crlog "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/reconcile" 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/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/mappers" "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" ) const TLSPolicyFinalizer = "kuadrant.io/tls-policy" -var ( - CertManagerCertificatesResource = certmanagerv1.SchemeGroupVersion.WithResource("certificates") - CertManagerIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("issuers") - CertMangerClusterIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("clusterissuers") - - CertManagerCertificateKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.CertificateKind} - CertManagerIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.IssuerKind} - CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} -) - // TLSPolicyReconciler reconciles a TLSPolicy object type TLSPolicyReconciler struct { *reconcilers.BaseReconciler @@ -99,7 +81,7 @@ func (r *TLSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if delResErr == nil { delResErr = err } - return r.reconcileStatus(ctx, tlsPolicy, targetReferenceObject, kuadrant.NewErrTargetNotFound(tlsPolicy.Kind(), tlsPolicy.GetTargetRef(), delResErr)) + return ctrl.Result{}, delResErr } return ctrl.Result{}, err } @@ -125,25 +107,9 @@ func (r *TLSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } } - specErr := r.reconcileResources(ctx, tlsPolicy, targetReferenceObject) - statusResult, statusErr := r.reconcileStatus(ctx, tlsPolicy, targetReferenceObject, specErr) - - if specErr != nil { - return ctrl.Result{}, specErr - } - - if statusErr != nil { - return ctrl.Result{}, statusErr - } - - if statusResult.Requeue { - log.V(1).Info("Reconciling status not finished. Requeing.") - return statusResult, nil - } - - return statusResult, statusErr + return ctrl.Result{}, specErr } func (r *TLSPolicyReconciler) reconcileResources(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) error { @@ -223,39 +189,9 @@ func (r *TLSPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { mappers.WithClient(mgr.GetClient()), ) - issuerStatusChangedPredicate := predicate.Funcs{ - UpdateFunc: func(ev event.UpdateEvent) bool { - oldPolicy, ok := ev.ObjectOld.(certmanagerv1.GenericIssuer) - if !ok { - return false - } - newPolicy, ok := ev.ObjectNew.(certmanagerv1.GenericIssuer) - if !ok { - return false - } - oldStatus := oldPolicy.GetStatus() - newStatus := newPolicy.GetStatus() - return !reflect.DeepEqual(oldStatus, newStatus) - }, - } - return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.TLSPolicy{}). Owns(&certmanagerv1.Certificate{}). Watches(&gatewayapiv1.Gateway{}, handler.EnqueueRequestsFromMapFunc(gatewayEventMapper.Map)). - Watches( - &certmanagerv1.Issuer{}, - handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, object client.Object) []reconcile.Request { - return mapIssuerToPolicy(ctx, mgr.GetClient(), r.Logger(), object) - }), - builder.WithPredicates(issuerStatusChangedPredicate), - ). - Watches( - &certmanagerv1.ClusterIssuer{}, - handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, object client.Object) []reconcile.Request { - return mapClusterIssuerToPolicy(ctx, mgr.GetClient(), r.Logger(), object) - }), - builder.WithPredicates(issuerStatusChangedPredicate), - ). Complete(r) } diff --git a/controllers/tlspolicy_links.go b/controllers/tlspolicy_links.go new file mode 100644 index 000000000..2f8c8624b --- /dev/null +++ b/controllers/tlspolicy_links.go @@ -0,0 +1,86 @@ +package controllers + +import ( + 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" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + + return machinery.LinkFunc{ + From: machinery.GatewayGroupKind, + To: CertManagerCertificateKind, + Func: func(child machinery.Object) []machinery.Object { + o := child.(*controller.RuntimeObject) + cert := o.Object.(*certmanagerv1.Certificate) + + gateway, ok := lo.Find(gateways, func(item *gwapiv1.Gateway) bool { + for _, l := range item.Spec.Listeners { + if l.TLS != nil && l.TLS.CertificateRefs != nil { + for _, certRef := range l.TLS.CertificateRefs { + certRefNS := "" + if certRef.Namespace == nil { + certRefNS = item.GetNamespace() + } else { + certRefNS = string(*certRef.Namespace) + } + if certRefNS == cert.GetNamespace() && string(certRef.Name) == cert.GetName() { + return true + } + } + } + } + + return false + }) + + if ok { + return []machinery.Object{&machinery.Gateway{Gateway: gateway}} + } + + return nil + }, + } +} + +func LinkGatewayToIssuerFunc(objs controller.Store) machinery.LinkFunc { + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + + return machinery.LinkFunc{ + From: machinery.GatewayGroupKind, + To: CertManagerIssuerKind, + Func: func(child machinery.Object) []machinery.Object { + o := child.(*controller.RuntimeObject) + issuer := o.Object.(*certmanagerv1.Issuer) + + // TODO: Refine + gateway, ok := lo.Find(gateways, func(item *gwapiv1.Gateway) bool { + return item.GetNamespace() == issuer.GetNamespace() + }) + + if ok { + return []machinery.Object{&machinery.Gateway{Gateway: gateway}} + } + + return nil + }, + } +} + +func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[machinery.Object]) + + return machinery.LinkFunc{ + From: machinery.GatewayGroupKind, + To: CertManagerClusterIssuerKind, + Func: func(child machinery.Object) []machinery.Object { + o := child.(*controller.RuntimeObject) + _ = o.Object.(*certmanagerv1.ClusterIssuer) + return gateways + }, + } +} diff --git a/controllers/tlspolicy_mappers.go b/controllers/tlspolicy_mappers.go deleted file mode 100644 index f4be97032..000000000 --- a/controllers/tlspolicy_mappers.go +++ /dev/null @@ -1,59 +0,0 @@ -package controllers - -import ( - "context" - "fmt" - - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - "github.com/go-logr/logr" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" -) - -func mapIssuerToPolicy(ctx context.Context, k8sClient client.Client, logger logr.Logger, object client.Object) []reconcile.Request { - _, ok := object.(*certmanagerv1.Issuer) - if !ok { - logger.V(1).Info("cannot map Issuer related event to tls policy", "error", fmt.Sprintf("%T is not a *certmanagerv1.Issuer", object)) - return nil - } - - policies := &v1alpha1.TLSPolicyList{} - if err := k8sClient.List(ctx, policies, client.InNamespace(object.GetNamespace())); err != nil { - logger.V(1).Error(err, "cannot list policies", "namespace", object.GetNamespace()) - return nil - } - - return policiesToRequests(logger, policies, object, certmanagerv1.IssuerKind) -} - -func mapClusterIssuerToPolicy(ctx context.Context, k8sClient client.Client, logger logr.Logger, object client.Object) []reconcile.Request { - _, ok := object.(*certmanagerv1.ClusterIssuer) - if !ok { - logger.V(1).Info("cannot map ClusterIssuer related event to tls policy", "error", fmt.Sprintf("%T is not a *certmanagerv1.ClusterIssuer", object)) - return nil - } - - policies := &v1alpha1.TLSPolicyList{} - if err := k8sClient.List(ctx, policies); err != nil { - logger.V(1).Error(err, "cannot list policies for all namespaces") - return nil - } - - return policiesToRequests(logger, policies, object, certmanagerv1.ClusterIssuerKind) -} - -func policiesToRequests(logger logr.Logger, policies *v1alpha1.TLSPolicyList, object client.Object, issuerKind string) []reconcile.Request { - filteredPolicies := utils.Filter(policies.Items, func(policy v1alpha1.TLSPolicy) bool { - return policy.Spec.IssuerRef.Name == object.GetName() && - policy.Spec.IssuerRef.Kind == issuerKind && - policy.Spec.IssuerRef.Group == certmanagerv1.SchemeGroupVersion.Group - }) - - return utils.Map(filteredPolicies, func(p v1alpha1.TLSPolicy) reconcile.Request { - logger.V(1).Info("tls policy possibly affected by related event", "eventKind", issuerKind, "policyName", p.Name, "policyNamespace", p.Namespace) - return reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&p)} - }) -} diff --git a/controllers/tlspolicy_mappers_test.go b/controllers/tlspolicy_mappers_test.go deleted file mode 100644 index 2ba29b5cd..000000000 --- a/controllers/tlspolicy_mappers_test.go +++ /dev/null @@ -1,223 +0,0 @@ -//go:build unit - -package controllers - -import ( - "context" - "errors" - "reflect" - "testing" - - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - v1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" - "github.com/go-logr/logr" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" -) - -func Test_mapClusterIssuerToPolicy(t *testing.T) { - clusterIssuer := &certmanagerv1.ClusterIssuer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-issuer", - }, - } - - s := runtime.NewScheme() - sb := runtime.NewSchemeBuilder(certmanagerv1.AddToScheme, v1alpha1.AddToScheme) - if err := sb.AddToScheme(s); err != nil { - t.Fatal(err) - } - - type args struct { - k8sClient client.Client - object client.Object - } - tests := []struct { - name string - args args - want []reconcile.Request - }{ - { - name: "not a cluster issuer", - args: args{ - object: &certmanagerv1.Certificate{}, - }, - want: nil, - }, - { - name: "list error", - args: args{ - k8sClient: fake.NewClientBuilder().WithScheme(s).WithInterceptorFuncs(interceptor.Funcs{ - List: func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { - return errors.New("list error") - }, - }).Build(), - object: clusterIssuer, - }, - want: nil, - }, - { - name: "map cluster issuer to matching policies", - args: args{ - k8sClient: fake.NewClientBuilder().WithScheme(s).WithObjects(clusterIssuer).WithLists(testInitTLSPolicies(clusterIssuer.Name, certmanagerv1.ClusterIssuerKind)).Build(), - object: clusterIssuer, - }, - want: []reconcile.Request{ - { - NamespacedName: types.NamespacedName{Name: "p1", Namespace: "n1"}, - }, - { - NamespacedName: types.NamespacedName{Name: "p4", Namespace: "n2"}, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := mapClusterIssuerToPolicy(context.Background(), tt.args.k8sClient, logr.Logger{}, tt.args.object); !reflect.DeepEqual(got, tt.want) { - t.Errorf("mapClusterIssuerToPolicy() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_mapIssuerToPolicy(t *testing.T) { - issuer := &certmanagerv1.Issuer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-issuer", - Namespace: "n1", - }, - } - - s := runtime.NewScheme() - sb := runtime.NewSchemeBuilder(certmanagerv1.AddToScheme, v1alpha1.AddToScheme) - if err := sb.AddToScheme(s); err != nil { - t.Fatal(err) - } - - type args struct { - ctx context.Context - k8sClient client.Client - logger logr.Logger - object client.Object - } - tests := []struct { - name string - args args - want []reconcile.Request - }{ - { - name: "not an issuer", - args: args{ - object: &certmanagerv1.Certificate{}, - }, - want: nil, - }, - { - name: "list error", - args: args{ - k8sClient: fake.NewClientBuilder().WithScheme(s).WithInterceptorFuncs(interceptor.Funcs{ - List: func(ctx context.Context, client client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { - return errors.New("list error") - }, - }).Build(), - object: issuer, - }, - want: nil, - }, - { - name: "map issuer to matching policies", - args: args{ - k8sClient: fake.NewClientBuilder().WithScheme(s).WithObjects(issuer).WithLists(testInitTLSPolicies(issuer.Name, certmanagerv1.IssuerKind)).Build(), - object: issuer, - }, - want: []reconcile.Request{ - { - NamespacedName: types.NamespacedName{Name: "p1", Namespace: "n1"}, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := mapIssuerToPolicy(tt.args.ctx, tt.args.k8sClient, tt.args.logger, tt.args.object); !reflect.DeepEqual(got, tt.want) { - t.Errorf("mapIssuerToPolicy() = %v, want %v", got, tt.want) - } - }) - } -} - -func testInitTLSPolicies(issuerName, issuerKind string) *v1alpha1.TLSPolicyList { - return &v1alpha1.TLSPolicyList{ - Items: []v1alpha1.TLSPolicy{ - - { - ObjectMeta: metav1.ObjectMeta{ - Name: "p1", - Namespace: "n1", - }, - Spec: v1alpha1.TLSPolicySpec{ - CertificateSpec: v1alpha1.CertificateSpec{ - IssuerRef: v1.ObjectReference{ - Name: issuerName, - Group: certmanagerv1.SchemeGroupVersion.Group, - Kind: issuerKind, - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "p2", - Namespace: "n1", - }, - Spec: v1alpha1.TLSPolicySpec{ - CertificateSpec: v1alpha1.CertificateSpec{ - IssuerRef: v1.ObjectReference{ - Name: issuerName, - Group: "unknown.example.com", - Kind: issuerKind, - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "p3", - Namespace: "n1", - }, - Spec: v1alpha1.TLSPolicySpec{ - CertificateSpec: v1alpha1.CertificateSpec{ - IssuerRef: v1.ObjectReference{ - Name: issuerName, - Group: certmanagerv1.SchemeGroupVersion.Group, - Kind: "Unknown", - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "p4", - Namespace: "n2", - }, - Spec: v1alpha1.TLSPolicySpec{ - CertificateSpec: v1alpha1.CertificateSpec{ - IssuerRef: v1.ObjectReference{ - Name: issuerName, - Group: certmanagerv1.SchemeGroupVersion.Group, - Kind: issuerKind, - }, - }, - }, - }, - }, - } -} diff --git a/controllers/tlspolicy_status.go b/controllers/tlspolicy_status.go deleted file mode 100644 index b185b5944..000000000 --- a/controllers/tlspolicy_status.go +++ /dev/null @@ -1,162 +0,0 @@ -/* -Copyright 2024. - -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" - "errors" - "fmt" - "slices" - - certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - "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" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" - "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" - "github.com/kuadrant/kuadrant-operator/pkg/library/utils" -) - -func (r *TLSPolicyReconciler) reconcileStatus(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object, specErr error) (ctrl.Result, error) { - newStatus := r.calculateStatus(ctx, tlsPolicy, targetNetworkObject, specErr) - - equalStatus := equality.Semantic.DeepEqual(newStatus, tlsPolicy.Status) - if equalStatus && tlsPolicy.Generation == tlsPolicy.Status.ObservedGeneration { - return reconcile.Result{}, nil - } - - newStatus.ObservedGeneration = tlsPolicy.Generation - - tlsPolicy.Status = *newStatus - updateErr := r.Client().Status().Update(ctx, tlsPolicy) - if updateErr != nil { - // Ignore conflicts, resource might just be outdated. - if apierrors.IsConflict(updateErr) { - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, updateErr - } - - if kuadrant.IsTargetNotFound(specErr) { - return ctrl.Result{Requeue: true}, nil - } - - return ctrl.Result{}, nil -} - -func (r *TLSPolicyReconciler) calculateStatus(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object, specErr error) *v1alpha1.TLSPolicyStatus { - newStatus := &v1alpha1.TLSPolicyStatus{ - // Copy initial conditions. Otherwise, status will always be updated - Conditions: slices.Clone(tlsPolicy.Status.Conditions), - ObservedGeneration: tlsPolicy.Status.ObservedGeneration, - } - - acceptedCond := kuadrant.AcceptedCondition(tlsPolicy, specErr) - meta.SetStatusCondition(&newStatus.Conditions, *acceptedCond) - - // 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)) - return newStatus - } - - enforcedCond := r.enforcedCondition(ctx, tlsPolicy, targetNetworkObject) - meta.SetStatusCondition(&newStatus.Conditions, *enforcedCond) - - return newStatus -} - -func (r *TLSPolicyReconciler) enforcedCondition(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) *metav1.Condition { - if err := r.isIssuerReady(ctx, tlsPolicy); err != nil { - return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) - } - - if err := r.isCertificatesReady(ctx, tlsPolicy, targetNetworkObject); err != nil { - return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) - } - - return kuadrant.EnforcedCondition(tlsPolicy, nil, true) -} - -func (r *TLSPolicyReconciler) isIssuerReady(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy) error { - var conditions []certmanv1.IssuerCondition - - switch tlsPolicy.Spec.IssuerRef.Kind { - case "", certmanv1.IssuerKind: - issuer := &certmanv1.Issuer{} - if err := r.Client().Get(ctx, client.ObjectKey{Name: tlsPolicy.Spec.IssuerRef.Name, Namespace: tlsPolicy.Namespace}, issuer); err != nil { - return err - } - conditions = issuer.Status.Conditions - case certmanv1.ClusterIssuerKind: - issuer := &certmanv1.ClusterIssuer{} - if err := r.Client().Get(ctx, client.ObjectKey{Name: tlsPolicy.Spec.IssuerRef.Name}, issuer); err != nil { - return err - } - conditions = issuer.Status.Conditions - default: - return fmt.Errorf(`invalid value %q for issuerRef.kind. Must be empty, %q or %q`, tlsPolicy.Spec.IssuerRef.Kind, certmanv1.IssuerKind, certmanv1.ClusterIssuerKind) - } - - transformedCond := utils.Map(conditions, func(c certmanv1.IssuerCondition) metav1.Condition { - return metav1.Condition{Reason: c.Reason, Status: metav1.ConditionStatus(c.Status), Type: string(c.Type), Message: c.Message} - }) - - if !meta.IsStatusConditionTrue(transformedCond, string(certmanv1.IssuerConditionReady)) { - return errors.New("issuer not ready") - } - - return nil -} - -func (r *TLSPolicyReconciler) isCertificatesReady(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) error { - gwDiffObj, err := reconcilers.ComputeGatewayDiffs(ctx, r.Client(), tlsPolicy, targetNetworkObject) - if err != nil { - return err - } - - if len(gwDiffObj.GatewaysWithValidPolicyRef) == 0 { - return errors.New("no valid gateways found") - } - - for _, gw := range gwDiffObj.GatewaysWithValidPolicyRef { - expectedCertificates := r.expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) - - for _, cert := range expectedCertificates { - c := &certmanv1.Certificate{} - if err := r.Client().Get(ctx, client.ObjectKeyFromObject(cert), c); err != nil { - return err - } - conditions := utils.Map(c.Status.Conditions, func(c certmanv1.CertificateCondition) metav1.Condition { - return metav1.Condition{Reason: c.Reason, Status: metav1.ConditionStatus(c.Status), Type: string(c.Type), Message: c.Message} - }) - - if !meta.IsStatusConditionTrue(conditions, string(certmanv1.CertificateConditionReady)) { - return fmt.Errorf("certificate %s not ready", cert.Name) - } - } - } - - return nil -} diff --git a/controllers/tlspolicy_status_test.go b/controllers/tlspolicy_status_test.go deleted file mode 100644 index 3e11ea699..000000000 --- a/controllers/tlspolicy_status_test.go +++ /dev/null @@ -1,367 +0,0 @@ -//go:build unit - -package controllers - -import ( - "context" - "encoding/json" - "fmt" - "reflect" - "testing" - - certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - certmanmetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - - "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" - "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" - "github.com/kuadrant/kuadrant-operator/pkg/log" -) - -func TestTLSPolicyReconciler_enforcedCondition(t *testing.T) { - const ( - ns = "default" - tlsPolicyName = "kuadrant-tls-policy" - issuerName = "kuadrant-issuer" - certificateName = "kuadrant-certifcate" - gwName = "kuadrant-gateway" - ) - - scheme := runtime.NewScheme() - sb := runtime.NewSchemeBuilder(certmanv1.AddToScheme, gatewayapiv1.AddToScheme) - if err := sb.AddToScheme(scheme); err != nil { - t.Fatal(err) - } - - policyFactory := func(mutateFn ...func(policy *v1alpha1.TLSPolicy)) *v1alpha1.TLSPolicy { - p := &v1alpha1.TLSPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: ns, - Name: tlsPolicyName, - }, - TypeMeta: metav1.TypeMeta{ - Kind: "TLSPolicy", - }, - Spec: v1alpha1.TLSPolicySpec{ - CertificateSpec: v1alpha1.CertificateSpec{ - IssuerRef: certmanmetav1.ObjectReference{ - Name: issuerName, - }, - }, - TargetRef: gatewayapiv1alpha2.LocalPolicyTargetReference{ - Name: gwName, - }, - }, - } - for _, mutate := range mutateFn { - mutate(p) - } - - return p - } - - withClusterIssuerMutater := func(p *v1alpha1.TLSPolicy) { - p.Spec.CertificateSpec.IssuerRef.Kind = certmanv1.ClusterIssuerKind - } - - issuerFactory := func(mutateFn ...func(issuer *certmanv1.Issuer)) *certmanv1.Issuer { - issuer := &certmanv1.Issuer{ - ObjectMeta: metav1.ObjectMeta{Name: issuerName, Namespace: ns}, - Status: certmanv1.IssuerStatus{ - Conditions: []certmanv1.IssuerCondition{ - { - Type: certmanv1.IssuerConditionReady, - Status: certmanmetav1.ConditionTrue, - }, - }, - }, - } - - for _, mutate := range mutateFn { - mutate(issuer) - } - - return issuer - } - - issuerNotReadyMutater := func(issuer *certmanv1.Issuer) { - issuer.Status = certmanv1.IssuerStatus{ - Conditions: []certmanv1.IssuerCondition{ - { - Type: certmanv1.IssuerConditionReady, - Status: certmanmetav1.ConditionFalse, - }, - }, - } - } - - certificateFactory := func(mutateFn ...func(certificate *certmanv1.Certificate)) *certmanv1.Certificate { - c := &certmanv1.Certificate{ - ObjectMeta: metav1.ObjectMeta{Name: certificateName, Namespace: ns}, - Status: certmanv1.CertificateStatus{ - Conditions: []certmanv1.CertificateCondition{ - { - Type: certmanv1.CertificateConditionReady, - Status: certmanmetav1.ConditionTrue, - }, - }, - }, - } - - for _, mutate := range mutateFn { - mutate(c) - } - - return c - } - - certificateNotReadyMutater := func(certificate *certmanv1.Certificate) { - certificate.Status = certmanv1.CertificateStatus{ - Conditions: []certmanv1.CertificateCondition{ - { - Type: certmanv1.CertificateConditionReady, - Status: certmanmetav1.ConditionFalse, - }, - }, - } - } - - refs, err := json.Marshal([]client.ObjectKey{{Name: tlsPolicyName, Namespace: ns}}) - if err != nil { - t.Fatal(err) - } - - gwFactory := func() *gatewayapiv1.Gateway { - return &gatewayapiv1.Gateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: gwName, - Namespace: ns, - Annotations: map[string]string{ - v1alpha1.TLSPolicyBackReferenceAnnotationName: string(refs), - }, - }, - Spec: gatewayapiv1.GatewaySpec{ - Listeners: []gatewayapiv1.Listener{ - { - Name: "http", - Hostname: ptr.To[gatewayapiv1.Hostname]("localhost"), - TLS: &gatewayapiv1.GatewayTLSConfig{ - CertificateRefs: []gatewayapiv1.SecretObjectReference{{ - Group: ptr.To[gatewayapiv1.Group]("core"), - Kind: ptr.To[gatewayapiv1.Kind]("Secret"), - Name: certificateName, - Namespace: ptr.To[gatewayapiv1.Namespace]("default"), - }}, - Mode: ptr.To(gatewayapiv1.TLSModeTerminate), - }, - }, - }, - }, - } - } - - type fields struct { - BaseReconciler *reconcilers.BaseReconciler - } - type args struct { - tlsPolicy *v1alpha1.TLSPolicy - targetNetworkObject client.Object - } - tests := []struct { - name string - fields fields - args args - want *metav1.Condition - }{ - { - name: "unable to get issuer", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: fmt.Sprintf("TLSPolicy has encountered some issues: issuers.cert-manager.io \"%s\" not found", issuerName), - }, - }, - { - name: "unable to get cluster issuer", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(withClusterIssuerMutater), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: fmt.Sprintf("TLSPolicy has encountered some issues: clusterissuers.cert-manager.io \"%s\" not found", issuerName), - }, - }, - { - name: "issuer not ready", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder(). - WithObjects(issuerFactory(issuerNotReadyMutater)). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: "TLSPolicy has encountered some issues: issuer not ready", - }, - }, - { - name: "issuer has no ready condition", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder(). - WithObjects( - issuerFactory(func(issuer *certmanv1.Issuer) { - issuer.Status.Conditions = []certmanv1.IssuerCondition{} - })). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: "TLSPolicy has encountered some issues: issuer not ready", - }, - }, - { - name: "no valid gateways found", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithObjects(issuerFactory()). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - targetNetworkObject: gwFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: "TLSPolicy has encountered some issues: no valid gateways found", - }, - }, - { - name: "unable to get certificate", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithObjects(issuerFactory(), gwFactory()). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - targetNetworkObject: gwFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: fmt.Sprintf("TLSPolicy has encountered some issues: certificates.cert-manager.io \"%s\" not found", certificateName), - }, - }, - { - name: "certificate is not ready", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithObjects(issuerFactory(), gwFactory(), certificateFactory(certificateNotReadyMutater)). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - targetNetworkObject: gwFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: fmt.Sprintf("TLSPolicy has encountered some issues: certificate %s not ready", certificateName), - }, - }, - { - name: "certificate has no ready condition", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithObjects( - issuerFactory(), gwFactory(), certificateFactory(func(certificate *certmanv1.Certificate) { - certificate.Status.Conditions = []certmanv1.CertificateCondition{} - })). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - targetNetworkObject: gwFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionFalse, - Reason: string(kuadrant.PolicyReasonUnknown), - Message: fmt.Sprintf("TLSPolicy has encountered some issues: certificate %s not ready", certificateName), - }, - }, - { - name: "is enforced", - fields: fields{ - BaseReconciler: reconcilers.NewBaseReconciler( - fake.NewClientBuilder().WithObjects(issuerFactory(), gwFactory(), certificateFactory()). - WithScheme(scheme).Build(), nil, nil, log.NewLogger(), nil, - ), - }, - args: args{ - tlsPolicy: policyFactory(), - targetNetworkObject: gwFactory(), - }, - want: &metav1.Condition{ - Type: string(kuadrant.PolicyConditionEnforced), - Status: metav1.ConditionTrue, - Reason: string(kuadrant.PolicyConditionEnforced), - Message: "TLSPolicy has been successfully enforced", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := &TLSPolicyReconciler{ - BaseReconciler: tt.fields.BaseReconciler, - } - if got := r.enforcedCondition(context.Background(), tt.args.tlsPolicy, tt.args.targetNetworkObject); !reflect.DeepEqual(got, tt.want) { - t.Errorf("enforcedCondition() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go new file mode 100644 index 000000000..0bb589c58 --- /dev/null +++ b/controllers/tlspolicy_tasks.go @@ -0,0 +1,318 @@ +package controllers + +import ( + "context" + "errors" + "fmt" + "slices" + "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/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/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/dynamic" + "k8s.io/utils/ptr" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" +) + +var ( + CertManagerCertificatesResource = certmanagerv1.SchemeGroupVersion.WithResource("certificates") + CertManagerIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("issuers") + CertMangerClusterIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("clusterissuers") + + CertManagerCertificateKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.CertificateKind} + CertManagerIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.IssuerKind} + CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} +) + +type ValidateTLSPolicyTask struct{} + +func NewValidateTLSPolicyTask() *ValidateTLSPolicyTask { + return &ValidateTLSPolicyTask{} +} + +func (t *ValidateTLSPolicyTask) Subscription() *controller.Subscription { + return &controller.Subscription{ + Events: []controller.ResourceEventMatcher{ + {Kind: &machinery.GatewayGroupKind}, + {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.CreateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.UpdateEvent)}, + {Kind: &CertManagerCertificateKind}, + {Kind: &CertManagerIssuerKind}, + {Kind: &CertManagerClusterIssuerKind}, + }, + ReconcileFunc: t.Reconcile, + } +} + +func (t *ValidateTLSPolicyTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("ValidateTLSPolicyTask").WithName("Reconcile") + + // 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 + }) + + // Get all gateways + gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { + gw, ok := item.(*machinery.Gateway) + return gw, ok + }) + + for _, policy := range policies { + if policy.DeletionTimestamp != nil { + logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) + continue + } + + // TODO: This should be only one target ref for now, but what should happen if multiple target refs is supported in the future? + targetRefs := policy.GetTargetRefs() + for _, targetRef := range targetRefs { + // Find gateway defined by target ref + _, ok := lo.Find(gws, func(item *machinery.Gateway) bool { + if item.GetName() == targetRef.GetName() && item.GetNamespace() == targetRef.GetNamespace() { + return true + } + return false + }) + + // Can't find gateway target ref + if !ok { + logger.Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) + s.Store(TLSPolicyValidKey(policy.GetUID()), false) + continue + } + + logger.Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) + s.Store(TLSPolicyValidKey(policy.GetUID()), true) + } + } + + return nil +} + +type TLSPolicyStatusTask struct { + Client *dynamic.DynamicClient +} + +func NewTLSPolicyStatusTask(client *dynamic.DynamicClient) *TLSPolicyStatusTask { + return &TLSPolicyStatusTask{Client: client} +} + +func (t *TLSPolicyStatusTask) Subscription() *controller.Subscription { + return &controller.Subscription{ + Events: []controller.ResourceEventMatcher{ + {Kind: &machinery.GatewayGroupKind}, + {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.CreateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.UpdateEvent)}, + {Kind: &CertManagerCertificateKind}, + {Kind: &CertManagerIssuerKind}, + {Kind: &CertManagerClusterIssuerKind}, + }, + ReconcileFunc: t.Reconcile, + } +} + +func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusTask").WithName("Reconcile") + + policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { + p, ok := item.(*kuadrantv1alpha1.TLSPolicy) + return p, ok + }) + + for _, policy := range policies { + if policy.DeletionTimestamp != nil { + logger.Info("tls policy is marked for deletion", "name", policy.Name, "namespace", policy.Namespace) + continue + } + + newStatus := &kuadrantv1alpha1.TLSPolicyStatus{ + // Copy initial conditions. Otherwise, status will always be updated + Conditions: slices.Clone(policy.Status.Conditions), + ObservedGeneration: policy.Status.ObservedGeneration, + } + + var err error + isValid, ok := s.Load(TLSPolicyValidKey(policy.GetUID())) + // Should not happen unless this was triggered by an event where the ValidateTLSPolicyTask.Reconcile function was not called + if !ok { + err = fmt.Errorf("unable to find %s key in sync map", policy.GetUID()) + logger.Error(err, "unexpected error") + continue + } + // Target Ref not found + if !isValid.(bool) { + err = kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName())) + } + + meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(policy, 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) + meta.SetStatusCondition(&newStatus.Conditions, *enforcedCond) + } + + // Nothing to do + equalStatus := equality.Semantic.DeepEqual(newStatus, policy.Status) + if equalStatus && policy.Generation == policy.Status.ObservedGeneration { + logger.Info("policy status unchanged, skipping update") + continue + } + newStatus.ObservedGeneration = policy.Generation + policy.Status = *newStatus + + resource := t.Client.Resource(kuadrantv1alpha1.TLSPoliciesResource).Namespace(policy.GetNamespace()) + un, err := controller.Destruct(policy) + if err != nil { + logger.Error(err, "unable to destruct policy") + continue + } + + _, err = resource.UpdateStatus(ctx, un, metav1.UpdateOptions{}) + if err != nil { + logger.Error(err, "unable to update status for TLSPolicy", "uid", policy.GetUID()) + } + } + + return nil +} + +func (t *TLSPolicyStatusTask) 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) + } + + if err := t.isCertificatesReady(ctx, tlsPolicy, topology); err != nil { + return kuadrant.EnforcedCondition(tlsPolicy, kuadrant.NewErrUnknown(tlsPolicy.Kind(), err), false) + } + + return kuadrant.EnforcedCondition(tlsPolicy, nil, true) +} + +func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { + logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusTask").WithName("isIssuerReady") + + // Get all gateways + gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { + gw, ok := item.(*machinery.Gateway) + return gw, ok + }) + + // Find gateway defined by target ref + gw, _ := lo.Find(gws, func(item *machinery.Gateway) bool { + if item.GetName() == string(tlsPolicy.GetTargetRef().Name) && item.GetNamespace() == tlsPolicy.GetNamespace() { + return true + } + return false + }) + + var conditions []certmanagerv1.IssuerCondition + + switch tlsPolicy.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 + }) + if !ok { + err := errors.New("unable to find issuer for TLSPolicy") + logger.Error(err, "unable to find issuer for TLSPolicy") + return err + } + + issuer := obj.(*controller.RuntimeObject).Object.(*certmanagerv1.Issuer) + + conditions = issuer.Status.Conditions + case certmanagerv1.ClusterIssuerKind: + // TODO: Why cant use gateway children + obj, ok := lo.Find(topology.Objects().Items(), func(o machinery.Object) bool { + return o.GroupVersionKind().GroupKind() == CertManagerClusterIssuerKind && o.GetName() == tlsPolicy.Spec.IssuerRef.Name + }) + if !ok { + err := errors.New("unable to find cluster issuer for TLSPolicy") + logger.Error(err, "unable to find cluster issuer for TLSPolicy") + return err + } + + 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) + } + + transformedCond := utils.Map(conditions, func(c certmanagerv1.IssuerCondition) metav1.Condition { + return metav1.Condition{Reason: c.Reason, Status: metav1.ConditionStatus(c.Status), Type: string(c.Type), Message: c.Message} + }) + + if !meta.IsStatusConditionTrue(transformedCond, string(certmanagerv1.IssuerConditionReady)) { + return errors.New("issuer not ready") + } + + return nil +} + +func (t *TLSPolicyStatusTask) isCertificatesReady(ctx context.Context, p machinery.Policy, topology *machinery.Topology) error { + tlsPolicy, ok := p.(*kuadrantv1alpha1.TLSPolicy) + if !ok { + return errors.New("invalid policy") + } + + // Get all gateways that contains this policy + gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { + gw, ok := item.(*machinery.Gateway) + + return gw, ok && lo.Contains(gw.Policies(), p) + }) + + if len(gws) == 0 { + return errors.New("no valid gateways found") + } + + for _, gw := range gws { + expectedCertificates := expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) + + for _, cert := range expectedCertificates { + // TODO: Why cant use gateway + obj, ok := lo.Find(topology.Objects().Children(gw), func(o machinery.Object) bool { + return o.GroupVersionKind().GroupKind() == CertManagerCertificateKind && o.GetNamespace() == cert.GetNamespace() && o.GetName() == cert.GetName() + }) + + if !ok { + return errors.New("certificate not found") + } + + c := obj.(*controller.RuntimeObject).Object.(*certmanagerv1.Certificate) + + conditions := utils.Map(c.Status.Conditions, func(c certmanagerv1.CertificateCondition) metav1.Condition { + return metav1.Condition{Reason: c.Reason, Status: metav1.ConditionStatus(c.Status), Type: string(c.Type), Message: c.Message} + }) + + if !meta.IsStatusConditionTrue(conditions, string(certmanagerv1.CertificateConditionReady)) { + return fmt.Errorf("certificate %s not ready", cert.Name) + } + } + } + + return nil +} + +func TLSPolicyValidKey(uid types.UID) string { + return fmt.Sprintf("TLSPolicyValid:%s", uid) +} diff --git a/controllers/tlspolicy_tasks_test.go b/controllers/tlspolicy_tasks_test.go new file mode 100644 index 000000000..58a0da459 --- /dev/null +++ b/controllers/tlspolicy_tasks_test.go @@ -0,0 +1,35 @@ +//go:build unit + +package controllers + +import ( + "testing" + + "k8s.io/apimachinery/pkg/types" +) + +func TestTLSPolicyValidKey(t *testing.T) { + type args struct { + uid types.UID + } + tests := []struct { + name string + args args + want string + }{ + { + name: "test uid is appended", + args: args{ + types.UID("unqiueid"), + }, + want: "TLSPolicyValid:unqiueid", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := TLSPolicyValidKey(tt.args.uid); got != tt.want { + t.Errorf("TLSPolicyValidKey() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/go.sum b/go.sum index b217cff0b..0d2923869 100644 --- a/go.sum +++ b/go.sum @@ -258,16 +258,10 @@ github.com/kuadrant/authorino v0.18.0 h1:9NjOly+Esy63JTE+CA2Aoapq5t1nJPi3lR50B5j github.com/kuadrant/authorino v0.18.0/go.mod h1:70VTo3BuAgHFqWhQHEgjJwlDOKNcsmA0A3WebRQeGUA= github.com/kuadrant/authorino-operator v0.11.1 h1:jndTZhiHMU+2Dk0NU+KP2+MUSfvclrn+YtTCQDJj+1s= github.com/kuadrant/authorino-operator v0.11.1/go.mod h1:TeFFdX477vUTMushCojaHpvwPLga4DpErGI2oQbqFIs= -github.com/kuadrant/dns-operator v0.0.0-20240926100317-2e2497411ab3 h1:r5Ed62AetTJhbJGEinM/G7ugdxV6Kp/kcVIpxOVxduM= -github.com/kuadrant/dns-operator v0.0.0-20240926100317-2e2497411ab3/go.mod h1:IHAt2o/VH1c0GIZTprggUDZuxoH0I304R9DUErBNIhk= github.com/kuadrant/dns-operator v0.0.0-20241002074817-d0cab9eecbdb h1:8cb/OsWDOrtjyFBobTzy2NGMQdVxGl4xAaQryFQYdQQ= github.com/kuadrant/dns-operator v0.0.0-20241002074817-d0cab9eecbdb/go.mod h1:IHAt2o/VH1c0GIZTprggUDZuxoH0I304R9DUErBNIhk= -github.com/kuadrant/dns-operator v0.6.0 h1:PH/cbK8Oz6EXcKO9kQH7Ovt8EskoAr70s58BHNppWpY= -github.com/kuadrant/dns-operator v0.6.0/go.mod h1:v6I6UWXmyVx3u5fatdw3PoS5owNvxkTMUE7gTC/Tfd0= github.com/kuadrant/limitador-operator v0.9.0 h1:hTQ6CFPayf/sL7cIzwWjCoU8uTn6fzWdsJgKbDlnFts= github.com/kuadrant/limitador-operator v0.9.0/go.mod h1:DQOlg9qFOcnWPrwO529JRCMLLOEXJQxkmOes952S/Hw= -github.com/kuadrant/policy-machinery v0.2.0 h1:6kACb+bdEwHXz2tvTs6dlLgvxFgFrowvGTZKMI9p0Qo= -github.com/kuadrant/policy-machinery v0.2.0/go.mod h1:ZV4xS0CCxPgu/Xg6gz+YUaS9zqEXKOiAj33bZ67B6Lo= github.com/kuadrant/policy-machinery v0.5.0 h1:hTllNYswhEOFrS/uj8kY4a4wq2W1xL2hagHeftn9TTY= github.com/kuadrant/policy-machinery v0.5.0/go.mod h1:ZV4xS0CCxPgu/Xg6gz+YUaS9zqEXKOiAj33bZ67B6Lo= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= diff --git a/tests/common/tlspolicy/tlspolicy_controller_test.go b/tests/common/tlspolicy/tlspolicy_controller_test.go index 90036e0c7..6eac28912 100644 --- a/tests/common/tlspolicy/tlspolicy_controller_test.go +++ b/tests/common/tlspolicy/tlspolicy_controller_test.go @@ -26,6 +26,10 @@ import ( ) var _ = Describe("TLSPolicy controller", func() { + const ( + testTimeOut = SpecTimeout(1 * time.Minute) + afterEachTimeOut = NodeTimeout(2 * time.Minute) + ) var gatewayClass *gatewayapiv1.GatewayClass var testNamespace string @@ -33,10 +37,8 @@ var _ = Describe("TLSPolicy controller", func() { var issuerRef *certmanmetav1.ObjectReference var gateway *gatewayapiv1.Gateway var tlsPolicy *v1alpha1.TLSPolicy - var ctx context.Context - BeforeEach(func() { - ctx = context.Background() + BeforeEach(func(ctx SpecContext) { testNamespace = tests.CreateNamespace(ctx, testClient()) gatewayClass = tests.BuildGatewayClass("gwc-"+testNamespace, "default", "kuadrant.io/bar") @@ -46,7 +48,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, issuer)).To(BeNil()) }) - AfterEach(func() { + AfterEach(func(ctx SpecContext) { if gateway != nil { err := k8sClient.Delete(ctx, gateway) Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) @@ -64,17 +66,17 @@ var _ = Describe("TLSPolicy controller", func() { Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) } tests.DeleteNamespace(ctx, testClient(), testNamespace) - }) + }, afterEachTimeOut) Context("invalid target", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { tlsPolicy = v1alpha1.NewTLSPolicy("test-tls-policy", testNamespace). WithTargetGateway("test-gateway"). WithIssuerRef(*issuerRef) Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should have accepted condition with status false and correct reason", func() { + It("should have accepted condition with status false and correct reason", func(ctx SpecContext) { Eventually(func(g Gomega) { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy) g.Expect(err).NotTo(HaveOccurred()) @@ -92,9 +94,9 @@ var _ = Describe("TLSPolicy controller", func() { })), ) }, tests.TimeoutMedium, time.Second).Should(Succeed()) - }) + }, testTimeOut) - It("should have accepted condition with status true", func() { + It("should have accepted condition with status true", func(ctx SpecContext) { Eventually(func(g Gomega) { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy) g.Expect(err).NotTo(HaveOccurred()) @@ -139,12 +141,12 @@ var _ = Describe("TLSPolicy controller", func() { })), ) }, tests.TimeoutMedium, time.Second).Should(Succeed()) - }) + }, testTimeOut) }) Context("valid target, issuer and policy", func() { - BeforeEach(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()) @@ -154,7 +156,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should have accepted condition with status true", func() { + It("should have accepted condition with status true", func(ctx SpecContext) { Eventually(func(g Gomega) { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy) g.Expect(err).NotTo(HaveOccurred()) @@ -175,9 +177,9 @@ var _ = Describe("TLSPolicy controller", func() { })), ) }, tests.TimeoutMedium, time.Second).Should(Succeed()) - }) + }, testTimeOut) - It("should set gateway back reference and policy affected status", func() { + 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) @@ -190,14 +192,14 @@ var _ = Describe("TLSPolicy controller", func() { 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() { var clusterIssuer *certmanv1.ClusterIssuer var clusterIssuerRef *certmanmetav1.ObjectReference - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { clusterIssuer, clusterIssuerRef = tests.BuildSelfSignedClusterIssuer("testclusterissuer", testNamespace) Expect(k8sClient.Create(ctx, clusterIssuer)).To(BeNil()) gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). @@ -209,12 +211,12 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - AfterEach(func() { + AfterEach(func(ctx SpecContext) { err := k8sClient.Delete(ctx, clusterIssuer) Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) - }) + }, afterEachTimeOut) - It("should have accepted and enforced condition with status true", func() { + It("should have accepted and enforced condition with status true", func(ctx SpecContext) { Eventually(func(g Gomega) { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy) g.Expect(err).NotTo(HaveOccurred()) @@ -235,11 +237,11 @@ var _ = Describe("TLSPolicy controller", func() { })), ) }, tests.TimeoutMedium, time.Second).Should(Succeed()) - }) + }, testTimeOut) }) Context("with http listener", func() { - BeforeEach(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()) @@ -249,16 +251,16 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should not create any certificates when TLS is not present", func() { + It("should not create any certificates when TLS is not present", func(ctx SpecContext) { Consistently(func() []certmanv1.Certificate { certList := &certmanv1.CertificateList{} err := k8sClient.List(ctx, certList, &client.ListOptions{Namespace: testNamespace}) Expect(err).ToNot(HaveOccurred()) return certList.Items }, time.Second*10, time.Second).Should(BeEmpty()) - }) + }, testTimeOut) - It("should create certificate when TLS is present", func() { + It("should create certificate when TLS is present", func(ctx SpecContext) { certNS := gatewayapiv1.Namespace(testNamespace) patch := client.MergeFrom(gateway.DeepCopy()) gateway.Spec.Listeners[0].Protocol = gatewayapiv1.HTTPSProtocolType @@ -283,12 +285,12 @@ var _ = Describe("TLSPolicy controller", func() { HaveField("Name", "test-tls-secret"), )) }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) - }) + }, testTimeOut) }) Context("with https listener", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). WithHTTPSListener("test.example.com", "test-tls-secret").Gateway Expect(k8sClient.Create(ctx, gateway)).To(BeNil()) @@ -298,7 +300,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should create tls certificate", func() { + It("should create tls certificate", func(ctx SpecContext) { Eventually(func(g Gomega, ctx context.Context) { certList := &certmanv1.CertificateList{} err := k8sClient.List(ctx, certList, &client.ListOptions{Namespace: testNamespace}) @@ -309,12 +311,11 @@ var _ = Describe("TLSPolicy controller", func() { HaveField("Name", "test-tls-secret"), )) }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) - }) + }, testTimeOut) }) Context("with multiple https listener and some shared secrets", func() { - - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). WithHTTPSListener("test1.example.com", "test-tls-secret"). WithHTTPSListener("test2.example.com", "test-tls-secret"). @@ -326,7 +327,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should create tls certificates", func() { + It("should create tls certificates", func(ctx SpecContext) { Eventually(func(g Gomega, ctx context.Context) { certList := &certmanv1.CertificateList{} err := k8sClient.List(ctx, certList, &client.ListOptions{Namespace: testNamespace}) @@ -349,11 +350,11 @@ 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()) - }) + }, testTimeOut) }) Context("with multiple https listener", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). WithHTTPSListener("test1.example.com", "test1-tls-secret"). WithHTTPSListener("test2.example.com", "test2-tls-secret"). @@ -365,7 +366,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should create tls certificates", func() { + It("should create tls certificates", func(ctx SpecContext) { Eventually(func(g Gomega, ctx context.Context) { certList := &certmanv1.CertificateList{} err := k8sClient.List(ctx, certList, &client.ListOptions{Namespace: testNamespace}) @@ -394,9 +395,9 @@ var _ = Describe("TLSPolicy controller", func() { 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()) - }) + }, testTimeOut) - It("should delete tls certificate when listener is removed", func() { + It("should delete tls certificate when listener is removed", func(ctx SpecContext) { //confirm all expected certificates are present Eventually(func() error { certificateList := &certmanv1.CertificateList{} @@ -423,9 +424,9 @@ var _ = Describe("TLSPolicy controller", func() { } return nil }, tests.TimeoutMedium, time.Second).Should(BeNil()) - }) + }, testTimeOut) - It("should delete all tls certificates when tls policy is removed even if gateway is already removed", func() { + 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 Eventually(func() error { certificateList := &certmanv1.CertificateList{} @@ -451,11 +452,11 @@ var _ = Describe("TLSPolicy controller", func() { } return nil }, time.Second*60, time.Second).Should(BeNil()) - }) + }, testTimeOut) }) Context("with https listener and multiple issuer configurations", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace). WithHTTPSListener("test.example.com", "test-tls-secret").Gateway Expect(k8sClient.Create(ctx, gateway)).To(BeNil()) @@ -481,7 +482,7 @@ var _ = Describe("TLSPolicy controller", func() { Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil()) }) - It("should create tls certificate", func() { + It("should create tls certificate", func(ctx SpecContext) { Eventually(func(g Gomega, ctx context.Context) { certList := &certmanv1.CertificateList{} err := k8sClient.List(ctx, certList, &client.ListOptions{Namespace: testNamespace}) @@ -511,12 +512,12 @@ var _ = Describe("TLSPolicy controller", func() { )) Expect(cert1.Spec.RevisionHistoryLimit).To(PointTo(Equal(int32(1)))) }, tests.TimeoutMedium, time.Second, ctx).Should(Succeed()) - }) + }, testTimeOut) }) Context("cel validation", func() { - It("should error targeting invalid group", func() { + It("should error targeting invalid group", func(ctx SpecContext) { p := v1alpha1.NewTLSPolicy("test-tls-policy", testNamespace). WithTargetGateway("gateway") p.Spec.TargetRef.Group = "not-gateway.networking.k8s.io" @@ -524,9 +525,9 @@ var _ = Describe("TLSPolicy controller", func() { err := k8sClient.Create(ctx, p) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'")) - }) + }, testTimeOut) - It("should error targeting invalid kind", func() { + It("should error targeting invalid kind", func(ctx SpecContext) { p := v1alpha1.NewTLSPolicy("test-tls-policy", testNamespace). WithTargetGateway("gateway") p.Spec.TargetRef.Kind = "TCPRoute" @@ -534,6 +535,6 @@ var _ = Describe("TLSPolicy controller", func() { err := k8sClient.Create(ctx, p) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Invalid targetRef.kind. The only supported values are 'Gateway'")) - }) + }, testTimeOut) }) }) diff --git a/tests/commons.go b/tests/commons.go index d60ca4247..a23e760d8 100644 --- a/tests/commons.go +++ b/tests/commons.go @@ -106,12 +106,6 @@ func DeleteNamespace(ctx context.Context, cl client.Client, namespace string) { }).WithContext(ctx).Should(Succeed()) } -func DeleteNamespaceCallback(ctx context.Context, cl client.Client, namespace string) func() { - return func() { - DeleteNamespace(ctx, cl, namespace) - } -} - func CreateNamespace(ctx context.Context, cl client.Client) string { nsObject := &corev1.Namespace{ TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"}, From 0fe2d41001a5d5c053d443dc244149f77176cf6b Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 30 Sep 2024 18:06:46 +0100 Subject: [PATCH 02/16] refactor: improve linking functions Signed-off-by: KevFan --- controllers/tlspolicy_links.go | 65 +++++++++++++++++++++++++++------- controllers/tlspolicy_tasks.go | 12 +++---- 2 files changed, 59 insertions(+), 18 deletions(-) diff --git a/controllers/tlspolicy_links.go b/controllers/tlspolicy_links.go index 2f8c8624b..08389be07 100644 --- a/controllers/tlspolicy_links.go +++ b/controllers/tlspolicy_links.go @@ -6,6 +6,8 @@ import ( "github.com/kuadrant/policy-machinery/machinery" "github.com/samber/lo" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" ) func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { @@ -18,13 +20,13 @@ func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { o := child.(*controller.RuntimeObject) cert := o.Object.(*certmanagerv1.Certificate) - gateway, ok := lo.Find(gateways, func(item *gwapiv1.Gateway) bool { - for _, l := range item.Spec.Listeners { + gateway, ok := lo.Find(gateways, func(g *gwapiv1.Gateway) bool { + for _, l := range g.Spec.Listeners { if l.TLS != nil && l.TLS.CertificateRefs != nil { for _, certRef := range l.TLS.CertificateRefs { certRefNS := "" if certRef.Namespace == nil { - certRefNS = item.GetNamespace() + certRefNS = g.GetNamespace() } else { certRefNS = string(*certRef.Namespace) } @@ -49,6 +51,7 @@ func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { func LinkGatewayToIssuerFunc(objs controller.Store) machinery.LinkFunc { gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + tlsPolicies := lo.Map(objs.FilterByGroupKind(kuadrantv1alpha1.TLSPolicyKind), controller.ObjectAs[*kuadrantv1alpha1.TLSPolicy]) return machinery.LinkFunc{ From: machinery.GatewayGroupKind, @@ -57,30 +60,68 @@ func LinkGatewayToIssuerFunc(objs controller.Store) machinery.LinkFunc { o := child.(*controller.RuntimeObject) issuer := o.Object.(*certmanagerv1.Issuer) - // TODO: Refine - gateway, ok := lo.Find(gateways, func(item *gwapiv1.Gateway) bool { - return item.GetNamespace() == issuer.GetNamespace() + // Policies linked to Issuer + // Issuer must be in the same namespace as the policy + linkedPolicies := lo.Filter(tlsPolicies, func(p *kuadrantv1alpha1.TLSPolicy, index int) bool { + return p.Spec.IssuerRef.Name == issuer.GetName() && p.GetNamespace() == issuer.GetNamespace() && p.Spec.IssuerRef.Kind == certmanagerv1.IssuerKind }) - if ok { - return []machinery.Object{&machinery.Gateway{Gateway: gateway}} + if len(linkedPolicies) == 0 { + return nil } - 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} + }) }, } } func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { - gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[machinery.Object]) + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + tlsPolicies := lo.Map(objs.FilterByGroupKind(kuadrantv1alpha1.TLSPolicyKind), controller.ObjectAs[*kuadrantv1alpha1.TLSPolicy]) return machinery.LinkFunc{ From: machinery.GatewayGroupKind, To: CertManagerClusterIssuerKind, Func: func(child machinery.Object) []machinery.Object { o := child.(*controller.RuntimeObject) - _ = o.Object.(*certmanagerv1.ClusterIssuer) - return gateways + clusterIssuer := o.Object.(*certmanagerv1.ClusterIssuer) + + // Policies linked to ClusterIssuer + linkedPolicies := lo.Filter(tlsPolicies, func(p *kuadrantv1alpha1.TLSPolicy, index int) bool { + return p.Spec.IssuerRef.Name == clusterIssuer.GetName() && p.Spec.IssuerRef.Kind == certmanagerv1.ClusterIssuerKind + }) + + 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} + }) }, } } diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go index 0bb589c58..8419e5f92 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_tasks.go @@ -233,7 +233,7 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad }) if !ok { err := errors.New("unable to find issuer for TLSPolicy") - logger.Error(err, "unable to find issuer for TLSPolicy") + logger.Error(err, "error finding object in topology") return err } @@ -241,13 +241,13 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad conditions = issuer.Status.Conditions case certmanagerv1.ClusterIssuerKind: - // TODO: Why cant use gateway children - obj, ok := lo.Find(topology.Objects().Items(), func(o machinery.Object) bool { + 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 }) if !ok { err := errors.New("unable to find cluster issuer for TLSPolicy") - logger.Error(err, "unable to find cluster issuer for TLSPolicy") + logger.Error(err, "error finding object in topology") return err } @@ -289,8 +289,8 @@ func (t *TLSPolicyStatusTask) isCertificatesReady(ctx context.Context, p machine expectedCertificates := expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) for _, cert := range expectedCertificates { - // TODO: Why cant use gateway - obj, ok := lo.Find(topology.Objects().Children(gw), func(o machinery.Object) bool { + 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 fdf9bd47ca0cc47ec797d265f5014759358c2c06 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 1 Oct 2024 13:05:47 +0100 Subject: [PATCH 03/16] refactor: events from certificate owned by TLSPolicies only Signed-off-by: KevFan --- controllers/state_of_the_world.go | 120 ++++++++++++++++++++---------- controllers/tlspolicy_tasks.go | 6 +- 2 files changed, 84 insertions(+), 42 deletions(-) diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 8e9a8d5f6..27801e889 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -20,6 +20,7 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/utils/env" ctrlruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" ctrlruntimepredicate "sigs.k8s.io/controller-runtime/pkg/predicate" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -201,47 +202,7 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D if err != nil || !isCertManagerInstalled { logger.Info("cert manager is not installed, skipping related watches and reconcilers", "err", err) } else { - controllerOpts = append(controllerOpts, - controller.WithRunnable("certificate watcher", controller.Watch( - &certmanagerv1.Certificate{}, - CertManagerCertificatesResource, - metav1.NamespaceAll), - ), - controller.WithRunnable("issuers watcher", controller.Watch( - &certmanagerv1.Issuer{}, - CertManagerIssuersResource, - metav1.NamespaceAll, - controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.Issuer]{ - UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.Issuer]) bool { - oldStatus := e.ObjectOld.GetStatus() - newStatus := e.ObjectOld.GetStatus() - return !reflect.DeepEqual(oldStatus, newStatus) - }, - })), - ), - controller.WithRunnable("clusterissuers watcher", controller.Watch( - &certmanagerv1.ClusterIssuer{}, - CertMangerClusterIssuersResource, - metav1.NamespaceAll, - controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.ClusterIssuer]{ - UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.ClusterIssuer]) bool { - oldStatus := e.ObjectOld.GetStatus() - newStatus := e.ObjectOld.GetStatus() - return !reflect.DeepEqual(oldStatus, newStatus) - }, - })), - ), - controller.WithObjectKinds( - CertManagerCertificateKind, - CertManagerIssuerKind, - CertManagerClusterIssuerKind, - ), - controller.WithObjectLinks( - LinkGatewayToCertificateFunc, - LinkGatewayToIssuerFunc, - LinkGatewayToClusterIssuerFunc, - ), - ) + controllerOpts = append(controllerOpts, certManagerControllerOpts()...) // TODO: add tls policy specific tasks to workflow } @@ -262,6 +223,68 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D return controller.NewController(controllerOpts...) } +func certManagerControllerOpts() []controller.ControllerOption { + isCertificateOwnedByTLSPolicy := func(c *certmanagerv1.Certificate) bool { + return isObjectOwnedByKind(c, kuadrantv1alpha1.TLSPolicyGroupKind.Group, kuadrantv1alpha1.TLSPolicyGroupKind.Kind) + } + + return []controller.ControllerOption{ + controller.WithRunnable("certificate watcher", controller.Watch( + &certmanagerv1.Certificate{}, + CertManagerCertificatesResource, + metav1.NamespaceAll, + controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.Certificate]{ + CreateFunc: func(e event.TypedCreateEvent[*certmanagerv1.Certificate]) bool { + return isCertificateOwnedByTLSPolicy(e.Object) + }, + UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.Certificate]) bool { + return isCertificateOwnedByTLSPolicy(e.ObjectNew) + }, + DeleteFunc: func(e event.TypedDeleteEvent[*certmanagerv1.Certificate]) bool { + return isCertificateOwnedByTLSPolicy(e.Object) + }, + GenericFunc: func(e event.TypedGenericEvent[*certmanagerv1.Certificate]) bool { + return isCertificateOwnedByTLSPolicy(e.Object) + }, + })), + ), + controller.WithRunnable("issuers watcher", controller.Watch( + &certmanagerv1.Issuer{}, + CertManagerIssuersResource, + metav1.NamespaceAll, + controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.Issuer]{ + UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.Issuer]) bool { + oldStatus := e.ObjectOld.GetStatus() + newStatus := e.ObjectOld.GetStatus() + return !reflect.DeepEqual(oldStatus, newStatus) + }, + })), + ), + controller.WithRunnable("clusterissuers watcher", controller.Watch( + &certmanagerv1.ClusterIssuer{}, + CertMangerClusterIssuersResource, + metav1.NamespaceAll, + controller.WithPredicates(ctrlruntimepredicate.TypedFuncs[*certmanagerv1.ClusterIssuer]{ + UpdateFunc: func(e event.TypedUpdateEvent[*certmanagerv1.ClusterIssuer]) bool { + oldStatus := e.ObjectOld.GetStatus() + newStatus := e.ObjectOld.GetStatus() + return !reflect.DeepEqual(oldStatus, newStatus) + }, + })), + ), + controller.WithObjectKinds( + CertManagerCertificateKind, + CertManagerIssuerKind, + CertManagerClusterIssuerKind, + ), + controller.WithObjectLinks( + LinkGatewayToCertificateFunc, + LinkGatewayToIssuerFunc, + LinkGatewayToClusterIssuerFunc, + ), + } +} + func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled bool) controller.ReconcileFunc { mainWorkflow := &controller.Workflow{ Precondition: initWorkflow(client).Run, @@ -339,3 +362,18 @@ func GetOldestKuadrant(kuadrants []*kuadrantv1beta1.Kuadrant) (*kuadrantv1beta1. } return oldest, nil } + +func isObjectOwnedByKind(o client.Object, ownerGroup, ownerKind string) bool { + for _, o := range o.GetOwnerReferences() { + oGV, err := schema.ParseGroupVersion(o.APIVersion) + if err != nil { + return false + } + + if oGV.Group == ownerGroup && o.Kind == ownerKind { + return true + } + } + + return false +} \ No newline at end of file diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go index 8419e5f92..d9f7fb050 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_tasks.go @@ -216,13 +216,17 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad }) // Find gateway defined by target ref - gw, _ := lo.Find(gws, func(item *machinery.Gateway) bool { + gw, ok := lo.Find(gws, func(item *machinery.Gateway) bool { if item.GetName() == string(tlsPolicy.GetTargetRef().Name) && item.GetNamespace() == tlsPolicy.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) + } + var conditions []certmanagerv1.IssuerCondition switch tlsPolicy.Spec.IssuerRef.Kind { From 270c7c534a851802e2e86fa607ab22b0769d8714 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 1 Oct 2024 16:10:05 +0100 Subject: [PATCH 04/16] test: re-add unit tests for enforced condition Signed-off-by: KevFan --- controllers/tlspolicy_tasks.go | 7 +- controllers/tlspolicy_tasks_test.go | 460 ++++++++++++++++++++++++++++ 2 files changed, 463 insertions(+), 4 deletions(-) diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go index d9f7fb050..e0f26d16f 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_tasks.go @@ -236,7 +236,7 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad return o.GroupVersionKind().GroupKind() == CertManagerIssuerKind && o.GetNamespace() == tlsPolicy.GetNamespace() && o.GetName() == tlsPolicy.Spec.IssuerRef.Name }) if !ok { - err := errors.New("unable to find issuer for TLSPolicy") + err := fmt.Errorf("%s \"%s\" not found", tlsPolicy.Spec.IssuerRef.Kind, tlsPolicy.Spec.IssuerRef.Name) logger.Error(err, "error finding object in topology") return err } @@ -250,7 +250,7 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad return o.GroupVersionKind().GroupKind() == CertManagerClusterIssuerKind && o.GetName() == tlsPolicy.Spec.IssuerRef.Name }) if !ok { - err := errors.New("unable to find cluster issuer for TLSPolicy") + err := fmt.Errorf("%s \"%s\" not found", tlsPolicy.Spec.IssuerRef.Kind, tlsPolicy.Spec.IssuerRef.Name) logger.Error(err, "error finding object in topology") return err } @@ -266,7 +266,7 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad }) if !meta.IsStatusConditionTrue(transformedCond, string(certmanagerv1.IssuerConditionReady)) { - return errors.New("issuer not ready") + return fmt.Errorf("%s not ready", tlsPolicy.Spec.IssuerRef.Kind) } return nil @@ -281,7 +281,6 @@ func (t *TLSPolicyStatusTask) isCertificatesReady(ctx context.Context, p machine // Get all gateways that contains this policy gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { gw, ok := item.(*machinery.Gateway) - return gw, ok && lo.Contains(gw.Policies(), p) }) diff --git a/controllers/tlspolicy_tasks_test.go b/controllers/tlspolicy_tasks_test.go index 58a0da459..40527f25c 100644 --- a/controllers/tlspolicy_tasks_test.go +++ b/controllers/tlspolicy_tasks_test.go @@ -3,9 +3,24 @@ package controllers import ( + "context" + "fmt" + "reflect" "testing" + certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + certmanmetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + "github.com/kuadrant/policy-machinery/controller" + "github.com/kuadrant/policy-machinery/machinery" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/utils/ptr" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) func TestTLSPolicyValidKey(t *testing.T) { @@ -33,3 +48,448 @@ func TestTLSPolicyValidKey(t *testing.T) { }) } } + +func TestTLSPolicyStatusTask_enforcedCondition(t *testing.T) { + const ( + ns = "default" + tlsPolicyName = "kuadrant-tls-policy" + issuerName = "kuadrant-issuer" + certificateName = "kuadrant-certifcate" + gwName = "kuadrant-gateway" + ) + + policyFactory := func(mutateFn ...func(policy *kuadrantv1alpha1.TLSPolicy)) *kuadrantv1alpha1.TLSPolicy { + p := &kuadrantv1alpha1.TLSPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: tlsPolicyName, + UID: types.UID(rand.String(9)), + }, + TypeMeta: metav1.TypeMeta{ + Kind: "TLSPolicy", + APIVersion: kuadrantv1alpha1.GroupVersion.String(), + }, + Spec: kuadrantv1alpha1.TLSPolicySpec{ + CertificateSpec: kuadrantv1alpha1.CertificateSpec{ + IssuerRef: certmanmetav1.ObjectReference{ + Name: issuerName, + Kind: certmanv1.IssuerKind, + }, + }, + TargetRef: gatewayapiv1alpha2.LocalPolicyTargetReference{ + Name: gwName, + Kind: "Gateway", + Group: gatewayapiv1alpha2.GroupName, + }, + }, + } + for _, mutate := range mutateFn { + mutate(p) + } + + return p + } + + withClusterIssuerMutater := func(p *kuadrantv1alpha1.TLSPolicy) { + p.Spec.CertificateSpec.IssuerRef.Kind = certmanv1.ClusterIssuerKind + } + + issuerFactory := func(mutateFn ...func(issuer *certmanv1.Issuer)) *certmanv1.Issuer { + issuer := &certmanv1.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: issuerName, + Namespace: ns, + UID: types.UID(rand.String(9)), + }, + TypeMeta: metav1.TypeMeta{ + Kind: certmanv1.IssuerKind, + APIVersion: certmanv1.SchemeGroupVersion.String(), + }, + Status: certmanv1.IssuerStatus{ + Conditions: []certmanv1.IssuerCondition{ + { + Type: certmanv1.IssuerConditionReady, + Status: certmanmetav1.ConditionTrue, + }, + }, + }, + } + + for _, mutate := range mutateFn { + mutate(issuer) + } + + return issuer + } + + issuerNotReadyMutater := func(issuer *certmanv1.Issuer) { + issuer.Status = certmanv1.IssuerStatus{ + Conditions: []certmanv1.IssuerCondition{ + { + Type: certmanv1.IssuerConditionReady, + Status: certmanmetav1.ConditionFalse, + }, + }, + } + } + + clusterIssuerFactory := func(mutateFn ...func(issuer *certmanv1.ClusterIssuer)) *certmanv1.ClusterIssuer { + issuer := &certmanv1.ClusterIssuer{ + ObjectMeta: metav1.ObjectMeta{Name: issuerName, Namespace: ns}, + TypeMeta: metav1.TypeMeta{ + Kind: certmanv1.ClusterIssuerKind, + APIVersion: certmanv1.SchemeGroupVersion.String(), + }, + Status: certmanv1.IssuerStatus{ + Conditions: []certmanv1.IssuerCondition{ + { + Type: certmanv1.IssuerConditionReady, + Status: certmanmetav1.ConditionTrue, + }, + }, + }, + } + + for _, mutate := range mutateFn { + mutate(issuer) + } + + return issuer + } + + clusterIssuerNotReadyMutater := func(issuer *certmanv1.ClusterIssuer) { + issuer.Status = certmanv1.IssuerStatus{ + Conditions: []certmanv1.IssuerCondition{ + { + Type: certmanv1.IssuerConditionReady, + Status: certmanmetav1.ConditionFalse, + }, + }, + } + } + + certificateFactory := func(mutateFn ...func(certificate *certmanv1.Certificate)) *certmanv1.Certificate { + c := &certmanv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: certificateName, + Namespace: ns, + UID: types.UID(rand.String(9)), + }, + TypeMeta: metav1.TypeMeta{ + Kind: certmanv1.CertificateKind, + APIVersion: certmanv1.SchemeGroupVersion.String(), + }, + Status: certmanv1.CertificateStatus{ + Conditions: []certmanv1.CertificateCondition{ + { + Type: certmanv1.CertificateConditionReady, + Status: certmanmetav1.ConditionTrue, + }, + }, + }, + } + + for _, mutate := range mutateFn { + mutate(c) + } + + return c + } + + certificateNotReadyMutater := func(certificate *certmanv1.Certificate) { + certificate.Status = certmanv1.CertificateStatus{ + Conditions: []certmanv1.CertificateCondition{ + { + Type: certmanv1.CertificateConditionReady, + Status: certmanmetav1.ConditionFalse, + }, + }, + } + } + + gwFactory := func() *gatewayapiv1.Gateway { + return &gatewayapiv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: gwName, + Namespace: ns, + UID: types.UID(rand.String(9)), + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Gateway", + APIVersion: gatewayapiv1.GroupVersion.String(), + }, + Spec: gatewayapiv1.GatewaySpec{ + Listeners: []gatewayapiv1.Listener{ + { + Name: "http", + Hostname: ptr.To[gatewayapiv1.Hostname]("localhost"), + TLS: &gatewayapiv1.GatewayTLSConfig{ + CertificateRefs: []gatewayapiv1.SecretObjectReference{{ + Group: ptr.To[gatewayapiv1.Group]("core"), + Kind: ptr.To[gatewayapiv1.Kind]("Secret"), + Name: certificateName, + Namespace: ptr.To[gatewayapiv1.Namespace]("default"), + }}, + Mode: ptr.To(gatewayapiv1.TLSModeTerminate), + }, + }, + }, + }, + } + } + + topologyOpts := func(policy *kuadrantv1alpha1.TLSPolicy, additionalOps ...machinery.GatewayAPITopologyOptionsFunc) []machinery.GatewayAPITopologyOptionsFunc { + store := make(controller.Store) + gw := gwFactory() + store[string(gw.UID)] = gw + store[string(policy.UID)] = policy + + opts := []machinery.GatewayAPITopologyOptionsFunc{ + machinery.WithGateways(gw), + machinery.WithGatewayAPITopologyPolicies(policy), + machinery.WithGatewayAPITopologyLinks( + LinkGatewayToCertificateFunc(store), + LinkGatewayToIssuerFunc(store), + LinkGatewayToClusterIssuerFunc(store), + ), + } + opts = append(opts, additionalOps...) + + return opts + } + + type args struct { + tlsPolicy *kuadrantv1alpha1.TLSPolicy + topology func(*kuadrantv1alpha1.TLSPolicy) *machinery.Topology + } + tests := []struct { + name string + args args + want *metav1.Condition + }{ + { + name: "unable to get issuer", + args: args{ + tlsPolicy: policyFactory(), + topology: func(p *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + topology, _ := machinery.NewGatewayAPITopology( + topologyOpts(p)..., + ) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: fmt.Sprintf("TLSPolicy has encountered some issues: Issuer \"%s\" not found", issuerName), + }, + }, + { + name: "unable to get cluster issuer", + args: args{ + tlsPolicy: policyFactory(withClusterIssuerMutater), + topology: func(p *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + topology, _ := machinery.NewGatewayAPITopology( + topologyOpts(p)..., + ) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: fmt.Sprintf("TLSPolicy has encountered some issues: ClusterIssuer \"%s\" not found", issuerName), + }, + }, + { + name: "issuer not ready", + args: args{ + tlsPolicy: policyFactory(), + topology: func(p *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(p, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory(issuerNotReadyMutater)}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: "TLSPolicy has encountered some issues: Issuer not ready", + }, + }, + { + name: "issuer has no ready condition", + args: args{ + tlsPolicy: policyFactory(), + topology: func(p *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(p, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory(func(issuer *certmanv1.Issuer) { + issuer.Status.Conditions = []certmanv1.IssuerCondition{} + })}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: "TLSPolicy has encountered some issues: Issuer not ready", + }, + }, + { + name: "cluster issuer not ready", + args: args{ + tlsPolicy: policyFactory(withClusterIssuerMutater), + topology: func(p *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(p, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: clusterIssuerFactory(clusterIssuerNotReadyMutater)}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: "TLSPolicy has encountered some issues: ClusterIssuer not ready", + }, + }, + { + name: "cluster issuer has no ready condition", + args: args{ + tlsPolicy: policyFactory(withClusterIssuerMutater), + topology: func(p *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(p, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: clusterIssuerFactory(func(issuer *certmanv1.ClusterIssuer) { + issuer.Status.Conditions = []certmanv1.IssuerCondition{} + })}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: "TLSPolicy has encountered some issues: ClusterIssuer not ready", + }, + }, + { + name: "no valid gateways found", + args: args{ + tlsPolicy: policyFactory(), + topology: func(_ *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(policyFactory(), machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory()}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: "TLSPolicy has encountered some issues: no valid gateways found", + }, + }, + { + name: "unable to get certificate", + args: args{ + tlsPolicy: policyFactory(), + topology: func(policy *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(policy, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory()}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: "TLSPolicy has encountered some issues: certificate not found", + }, + }, + { + name: "certificate is not ready", + args: args{ + tlsPolicy: policyFactory(), + topology: func(policy *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(policy, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory()}, + &controller.RuntimeObject{Object: certificateFactory(certificateNotReadyMutater)}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: fmt.Sprintf("TLSPolicy has encountered some issues: certificate %s not ready", certificateName), + }, + }, + { + name: "certificate has no ready condition", + args: args{ + tlsPolicy: policyFactory(), + topology: func(policy *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(policy, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory()}, + &controller.RuntimeObject{Object: certificateFactory(func(certificate *certmanv1.Certificate) { + certificate.Status.Conditions = []certmanv1.CertificateCondition{} + })}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionFalse, + Reason: string(kuadrant.PolicyReasonUnknown), + Message: fmt.Sprintf("TLSPolicy has encountered some issues: certificate %s not ready", certificateName), + }, + }, + { + name: "is enforced", + args: args{ + tlsPolicy: policyFactory(), + topology: func(policy *kuadrantv1alpha1.TLSPolicy) *machinery.Topology { + opts := topologyOpts(policy, machinery.WithGatewayAPITopologyObjects( + &controller.RuntimeObject{Object: issuerFactory()}, + &controller.RuntimeObject{Object: certificateFactory()}, + )) + topology, _ := machinery.NewGatewayAPITopology(opts...) + return topology + }, + }, + want: &metav1.Condition{ + Type: string(kuadrant.PolicyConditionEnforced), + Status: metav1.ConditionTrue, + Reason: string(kuadrant.PolicyConditionEnforced), + Message: "TLSPolicy has been successfully enforced", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t1 *testing.T) { + t := &TLSPolicyStatusTask{} + 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 33a80fc432a6f1436b528d0df11fa72eb9dd5356 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 2 Oct 2024 11:46:21 +0100 Subject: [PATCH 05/16] feat: status for cert manager not installed in tls policies Signed-off-by: KevFan --- controllers/state_of_the_world.go | 9 +-- controllers/tlspolicy_tasks.go | 68 +++++++++++++++---- controllers/tlspolicy_tasks_test.go | 4 +- .../apimachinery_status_conditions.go | 7 +- pkg/library/kuadrant/errors.go | 20 ++++++ 5 files changed, 84 insertions(+), 24 deletions(-) diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 27801e889..d64e79809 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -15,6 +15,7 @@ import ( istioclientnetworkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" istioclientgosecurityv1beta1 "istio.io/client-go/pkg/apis/security/v1beta1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -203,7 +204,6 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D logger.Info("cert manager is not installed, skipping related watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, certManagerControllerOpts()...) - // TODO: add tls policy specific tasks to workflow } isConsolePluginInstalled, err := openshift.IsConsolePluginInstalled(manager.GetRESTMapper()) @@ -287,7 +287,7 @@ func certManagerControllerOpts() []controller.ControllerOption { func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled bool) controller.ReconcileFunc { mainWorkflow := &controller.Workflow{ - Precondition: initWorkflow(client).Run, + Precondition: initWorkflow(client, manager.GetRESTMapper()).Run, Tasks: []controller.ReconcileFunc{ NewAuthorinoReconciler(client).Subscription().Reconcile, NewLimitadorReconciler(client).Subscription().Reconcile, @@ -308,11 +308,12 @@ func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, return mainWorkflow.Run } -func initWorkflow(client *dynamic.DynamicClient) *controller.Workflow { +func initWorkflow(client *dynamic.DynamicClient, restMapper meta.RESTMapper) *controller.Workflow { return &controller.Workflow{ Precondition: NewEventLogger().Log, Tasks: []controller.ReconcileFunc{ NewTopologyReconciler(client, operatorNamespace).Reconcile, + NewIsCertManagerInstalledTask(restMapper).Reconcile, }, } } @@ -376,4 +377,4 @@ func isObjectOwnedByKind(o client.Object, ownerGroup, ownerKind string) bool { } return false -} \ No newline at end of file +} diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go index e0f26d16f..b53191c43 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_tasks.go @@ -23,6 +23,7 @@ import ( gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) @@ -37,6 +38,13 @@ var ( CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} ) +func NewTLSPolicyWorkflow(client *dynamic.DynamicClient) *controller.Workflow { + return &controller.Workflow{ + Precondition: NewValidateTLSPolicyTask().Reconcile, + Postcondition: NewTLSPolicyStatusTask(client).Reconcile, + } +} + type ValidateTLSPolicyTask struct{} func NewValidateTLSPolicyTask() *ValidateTLSPolicyTask { @@ -72,12 +80,25 @@ func (t *ValidateTLSPolicyTask) Reconcile(ctx context.Context, _ []controller.Re return gw, ok }) + isCertManagerInstalled := false + installed, ok := s.Load(IsCertManagerInstalledKey) + if ok { + isCertManagerInstalled = installed.(bool) + } else { + logger.V(1).Error(errors.New("isCertManagerInstalled was not found in sync map, defaulting to false"), "sync map error") + } + for _, policy := range policies { if policy.DeletionTimestamp != nil { logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) continue } + if !isCertManagerInstalled { + s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrDependencyNotInstalled("Cert Manager")) + continue + } + // TODO: This should be only one target ref for now, but what should happen if multiple target refs is supported in the future? targetRefs := policy.GetTargetRefs() for _, targetRef := range targetRefs { @@ -92,12 +113,12 @@ func (t *ValidateTLSPolicyTask) Reconcile(ctx context.Context, _ []controller.Re // Can't find gateway target ref if !ok { logger.Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) - s.Store(TLSPolicyValidKey(policy.GetUID()), false) + s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName()))) continue } logger.Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) - s.Store(TLSPolicyValidKey(policy.GetUID()), true) + s.Store(TLSPolicyAcceptedKey(policy.GetUID()), nil) } } @@ -136,7 +157,7 @@ func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.Reso for _, policy := range policies { if policy.DeletionTimestamp != nil { - logger.Info("tls policy is marked for deletion", "name", policy.Name, "namespace", policy.Namespace) + logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) continue } @@ -147,18 +168,10 @@ func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.Reso } var err error - isValid, ok := s.Load(TLSPolicyValidKey(policy.GetUID())) - // Should not happen unless this was triggered by an event where the ValidateTLSPolicyTask.Reconcile function was not called - if !ok { - err = fmt.Errorf("unable to find %s key in sync map", policy.GetUID()) - logger.Error(err, "unexpected error") - continue - } - // Target Ref not found - if !isValid.(bool) { - err = kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName())) + accepted, ok := s.Load(TLSPolicyAcceptedKey(policy.GetUID())) + if ok && accepted != nil { + err = accepted.(error) } - meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(policy, err)) // Do not set enforced condition if Accepted condition is false @@ -316,6 +329,31 @@ func (t *TLSPolicyStatusTask) isCertificatesReady(ctx context.Context, p machine return nil } -func TLSPolicyValidKey(uid types.UID) string { +const IsCertManagerInstalledKey = "IsCertManagerInstalled" + +func NewIsCertManagerInstalledTask(restMapper meta.RESTMapper) IsCertManagerInstalledTask { + return IsCertManagerInstalledTask{ + restMapper: restMapper, + } +} + +type IsCertManagerInstalledTask struct { + restMapper meta.RESTMapper +} + +func (t IsCertManagerInstalledTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("IsCertManagerInstalledTask").WithName("Reconcile") + isCertManagerInstalled, err := kuadrantgatewayapi.IsCertManagerInstalled(t.restMapper, logger) + + if err != nil { + logger.Error(err, "error checking IsCertManagerInstalled") + } + + s.Store(IsCertManagerInstalledKey, isCertManagerInstalled) + + return nil +} + +func TLSPolicyAcceptedKey(uid types.UID) string { return fmt.Sprintf("TLSPolicyValid:%s", uid) } diff --git a/controllers/tlspolicy_tasks_test.go b/controllers/tlspolicy_tasks_test.go index 40527f25c..0724a2d34 100644 --- a/controllers/tlspolicy_tasks_test.go +++ b/controllers/tlspolicy_tasks_test.go @@ -23,7 +23,7 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) -func TestTLSPolicyValidKey(t *testing.T) { +func TestTLSPolicyAcceptedKey(t *testing.T) { type args struct { uid types.UID } @@ -42,7 +42,7 @@ func TestTLSPolicyValidKey(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := TLSPolicyValidKey(tt.args.uid); got != tt.want { + if got := TLSPolicyAcceptedKey(tt.args.uid); got != tt.want { t.Errorf("TLSPolicyValidKey() = %v, want %v", got, tt.want) } }) diff --git a/pkg/library/kuadrant/apimachinery_status_conditions.go b/pkg/library/kuadrant/apimachinery_status_conditions.go index eb4a42da4..6c98f4bf8 100644 --- a/pkg/library/kuadrant/apimachinery_status_conditions.go +++ b/pkg/library/kuadrant/apimachinery_status_conditions.go @@ -17,9 +17,10 @@ import ( const ( PolicyConditionEnforced gatewayapiv1alpha2.PolicyConditionType = "Enforced" - PolicyReasonEnforced gatewayapiv1alpha2.PolicyConditionReason = "Enforced" - PolicyReasonOverridden gatewayapiv1alpha2.PolicyConditionReason = "Overridden" - PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown" + PolicyReasonEnforced gatewayapiv1alpha2.PolicyConditionReason = "Enforced" + PolicyReasonOverridden gatewayapiv1alpha2.PolicyConditionReason = "Overridden" + PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown" + PolicyReasonMissingDependency gatewayapiv1alpha2.PolicyConditionReason = "MissingDependency" ) func NewAffectedPolicyMap() *AffectedPolicyMap { diff --git a/pkg/library/kuadrant/errors.go b/pkg/library/kuadrant/errors.go index e3602f8a8..da996b634 100644 --- a/pkg/library/kuadrant/errors.go +++ b/pkg/library/kuadrant/errors.go @@ -144,3 +144,23 @@ func reasonForError(err error) gatewayapiv1alpha2.PolicyConditionReason { } return "" } + +func NewErrDependencyNotInstalled(dependencyName string) ErrDependencyNotInstalled { + return ErrDependencyNotInstalled{ + dependencyName: dependencyName, + } +} + +var _ PolicyError = ErrDependencyNotInstalled{} + +type ErrDependencyNotInstalled struct { + dependencyName string +} + +func (e ErrDependencyNotInstalled) Error() string { + return fmt.Sprintf("%s is not installed, please restart pod once dependency has been installed", e.dependencyName) +} + +func (e ErrDependencyNotInstalled) Reason() gatewayapiv1alpha2.PolicyConditionReason { + return PolicyReasonMissingDependency +} From e6a1e4f8c111d13335cad63cb0d02f96fc644713 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 2 Oct 2024 14:21:49 +0100 Subject: [PATCH 06/16] fixup: use GroupKind for isObjectOwnedBy function Signed-off-by: KevFan --- controllers/state_of_the_world.go | 6 +++--- controllers/tlspolicy_tasks.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index d64e79809..b733f79dc 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -225,7 +225,7 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D func certManagerControllerOpts() []controller.ControllerOption { isCertificateOwnedByTLSPolicy := func(c *certmanagerv1.Certificate) bool { - return isObjectOwnedByKind(c, kuadrantv1alpha1.TLSPolicyGroupKind.Group, kuadrantv1alpha1.TLSPolicyGroupKind.Kind) + return isObjectOwnedByGroupKind(c, kuadrantv1alpha1.TLSPolicyGroupKind) } return []controller.ControllerOption{ @@ -364,14 +364,14 @@ func GetOldestKuadrant(kuadrants []*kuadrantv1beta1.Kuadrant) (*kuadrantv1beta1. return oldest, nil } -func isObjectOwnedByKind(o client.Object, ownerGroup, ownerKind string) bool { +func isObjectOwnedByGroupKind(o client.Object, groupKind schema.GroupKind) bool { for _, o := range o.GetOwnerReferences() { oGV, err := schema.ParseGroupVersion(o.APIVersion) if err != nil { return false } - if oGV.Group == ownerGroup && o.Kind == ownerKind { + if oGV.Group == groupKind.Group && o.Kind == groupKind.Kind { return true } } diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go index b53191c43..6f2238496 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_tasks.go @@ -341,7 +341,7 @@ type IsCertManagerInstalledTask struct { restMapper meta.RESTMapper } -func (t IsCertManagerInstalledTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { +func (t IsCertManagerInstalledTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, _ *machinery.Topology, _ error, s *sync.Map) error { logger := controller.LoggerFromContext(ctx).WithName("IsCertManagerInstalledTask").WithName("Reconcile") isCertManagerInstalled, err := kuadrantgatewayapi.IsCertManagerInstalled(t.restMapper, logger) From a6649f2fa62028228856bc148417667ade7bc460 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 2 Oct 2024 14:46:54 +0100 Subject: [PATCH 07/16] fixup: logs Signed-off-by: KevFan --- controllers/tlspolicy_tasks.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_tasks.go index 6f2238496..b18bcb8a5 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_tasks.go @@ -112,12 +112,12 @@ func (t *ValidateTLSPolicyTask) Reconcile(ctx context.Context, _ []controller.Re // Can't find gateway target ref if !ok { - logger.Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) + logger.V(1).Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName()))) continue } - logger.Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) + logger.V(1).Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) s.Store(TLSPolicyAcceptedKey(policy.GetUID()), nil) } } @@ -185,7 +185,7 @@ func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.Reso // Nothing to do equalStatus := equality.Semantic.DeepEqual(newStatus, policy.Status) if equalStatus && policy.Generation == policy.Status.ObservedGeneration { - logger.Info("policy status unchanged, skipping update") + logger.V(1).Info("policy status unchanged, skipping update") continue } newStatus.ObservedGeneration = policy.Generation @@ -200,7 +200,7 @@ func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.Reso _, err = resource.UpdateStatus(ctx, un, metav1.UpdateOptions{}) if err != nil { - logger.Error(err, "unable to update status for TLSPolicy", "uid", policy.GetUID()) + logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetUID(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) } } From bd3b3c11dc0f019a4049d17458c8c2b6fa418659 Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 3 Oct 2024 16:42:12 +0100 Subject: [PATCH 08/16] refactor: align with some common conventions Signed-off-by: KevFan --- controllers/cert_manager_installed.go | 37 +++++ controllers/state_of_the_world.go | 2 +- controllers/tls_workflow.go | 138 +++++++++++++++- controllers/tlspolicies_validator.go | 96 ++++++++++++ controllers/tlspolicy_links.go | 127 --------------- ...y_tasks.go => tlspolicy_status_updater.go} | 148 ++---------------- ...st.go => tlspolicy_status_updater_test.go} | 2 +- 7 files changed, 282 insertions(+), 268 deletions(-) create mode 100644 controllers/cert_manager_installed.go create mode 100644 controllers/tlspolicies_validator.go delete mode 100644 controllers/tlspolicy_links.go rename controllers/{tlspolicy_tasks.go => tlspolicy_status_updater.go} (57%) rename controllers/{tlspolicy_tasks_test.go => tlspolicy_status_updater_test.go} (99%) diff --git a/controllers/cert_manager_installed.go b/controllers/cert_manager_installed.go new file mode 100644 index 000000000..87f030b57 --- /dev/null +++ b/controllers/cert_manager_installed.go @@ -0,0 +1,37 @@ +package controllers + +import ( + "context" + "sync" + + "github.com/kuadrant/policy-machinery/controller" + "github.com/kuadrant/policy-machinery/machinery" + "k8s.io/apimachinery/pkg/api/meta" + + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" +) + +const IsCertManagerInstalledKey = "IsCertManagerInstalled" + +func NewIsCertManagerInstalledReconciler(restMapper meta.RESTMapper) IsCertManagerInstalledReconciler { + return IsCertManagerInstalledReconciler{ + restMapper: restMapper, + } +} + +type IsCertManagerInstalledReconciler struct { + restMapper meta.RESTMapper +} + +func (t IsCertManagerInstalledReconciler) Check(ctx context.Context, _ []controller.ResourceEvent, _ *machinery.Topology, _ error, s *sync.Map) error { + logger := controller.LoggerFromContext(ctx).WithName("IsCertManagerInstalledReconciler").WithName("Reconcile") + isCertManagerInstalled, err := kuadrantgatewayapi.IsCertManagerInstalled(t.restMapper, logger) + + if err != nil { + logger.Error(err, "error checking IsCertManagerInstalled") + } + + s.Store(IsCertManagerInstalledKey, isCertManagerInstalled) + + return nil +} diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index b733f79dc..16992c2f2 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -313,7 +313,7 @@ func initWorkflow(client *dynamic.DynamicClient, restMapper meta.RESTMapper) *co Precondition: NewEventLogger().Log, Tasks: []controller.ReconcileFunc{ NewTopologyReconciler(client, operatorNamespace).Reconcile, - NewIsCertManagerInstalledTask(restMapper).Reconcile, + NewIsCertManagerInstalledReconciler(restMapper).Check, }, } } diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index c8efd7627..47c46b2c0 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -1,13 +1,147 @@ package controllers import ( + "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/runtime/schema" "k8s.io/client-go/dynamic" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" +) + +var ( + CertManagerCertificatesResource = certmanagerv1.SchemeGroupVersion.WithResource("certificates") + CertManagerIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("issuers") + CertMangerClusterIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("clusterissuers") + + CertManagerCertificateKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.CertificateKind} + CertManagerIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.IssuerKind} + CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} ) func NewTLSWorkflow(client *dynamic.DynamicClient) *controller.Workflow { return &controller.Workflow{ - Precondition: NewValidateTLSPolicyTask().Reconcile, - Postcondition: NewTLSPolicyStatusTask(client).Reconcile, + Precondition: NewValidateTLSPoliciesValidatorReconciler().Validate, + Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).UpdateStatus, + } +} + +func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + + return machinery.LinkFunc{ + From: machinery.GatewayGroupKind, + To: CertManagerCertificateKind, + Func: func(child machinery.Object) []machinery.Object { + o := child.(*controller.RuntimeObject) + cert := o.Object.(*certmanagerv1.Certificate) + + gateway, ok := lo.Find(gateways, func(g *gwapiv1.Gateway) bool { + for _, l := range g.Spec.Listeners { + if l.TLS != nil && l.TLS.CertificateRefs != nil { + for _, certRef := range l.TLS.CertificateRefs { + certRefNS := "" + if certRef.Namespace == nil { + certRefNS = g.GetNamespace() + } else { + certRefNS = string(*certRef.Namespace) + } + if certRefNS == cert.GetNamespace() && string(certRef.Name) == cert.GetName() { + return true + } + } + } + } + + return false + }) + + if ok { + return []machinery.Object{&machinery.Gateway{Gateway: gateway}} + } + + return nil + }, + } +} + +func LinkGatewayToIssuerFunc(objs controller.Store) machinery.LinkFunc { + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + tlsPolicies := lo.Map(objs.FilterByGroupKind(kuadrantv1alpha1.TLSPolicyGroupKind), controller.ObjectAs[*kuadrantv1alpha1.TLSPolicy]) + + return machinery.LinkFunc{ + From: machinery.GatewayGroupKind, + To: CertManagerIssuerKind, + Func: func(child machinery.Object) []machinery.Object { + o := child.(*controller.RuntimeObject) + issuer := o.Object.(*certmanagerv1.Issuer) + + // Policies linked to Issuer + // Issuer must be in the same namespace as the policy + linkedPolicies := lo.Filter(tlsPolicies, func(p *kuadrantv1alpha1.TLSPolicy, index int) bool { + 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} + }) + }, + } +} + +func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { + gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) + tlsPolicies := lo.Map(objs.FilterByGroupKind(kuadrantv1alpha1.TLSPolicyGroupKind), controller.ObjectAs[*kuadrantv1alpha1.TLSPolicy]) + + return machinery.LinkFunc{ + From: machinery.GatewayGroupKind, + To: CertManagerClusterIssuerKind, + Func: func(child machinery.Object) []machinery.Object { + o := child.(*controller.RuntimeObject) + clusterIssuer := o.Object.(*certmanagerv1.ClusterIssuer) + + // Policies linked to ClusterIssuer + linkedPolicies := lo.Filter(tlsPolicies, func(p *kuadrantv1alpha1.TLSPolicy, index int) bool { + return p.Spec.IssuerRef.Name == clusterIssuer.GetName() && p.Spec.IssuerRef.Kind == certmanagerv1.ClusterIssuerKind + }) + + 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} + }) + }, } } diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go new file mode 100644 index 000000000..f085cd7e5 --- /dev/null +++ b/controllers/tlspolicies_validator.go @@ -0,0 +1,96 @@ +package controllers + +import ( + "context" + "errors" + "sync" + + "github.com/kuadrant/policy-machinery/controller" + "github.com/kuadrant/policy-machinery/machinery" + "github.com/samber/lo" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/utils/ptr" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" +) + +func NewValidateTLSPoliciesValidatorReconciler() *ValidateTLSPoliciesValidatorReconciler { + return &ValidateTLSPoliciesValidatorReconciler{} +} + +type ValidateTLSPoliciesValidatorReconciler struct{} + +func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subscription { + return &controller.Subscription{ + Events: []controller.ResourceEventMatcher{ + {Kind: &machinery.GatewayGroupKind}, + {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.CreateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.UpdateEvent)}, + {Kind: &CertManagerCertificateKind}, + {Kind: &CertManagerIssuerKind}, + {Kind: &CertManagerClusterIssuerKind}, + }, + ReconcileFunc: t.Validate, + } +} + +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") + + // 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 + }) + + // Get all gateways + gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { + gw, ok := item.(*machinery.Gateway) + return gw, ok + }) + + isCertManagerInstalled := false + installed, ok := s.Load(IsCertManagerInstalledKey) + if ok { + isCertManagerInstalled = installed.(bool) + } else { + logger.V(1).Error(errors.New("isCertManagerInstalled was not found in sync map, defaulting to false"), "sync map error") + } + + for _, policy := range policies { + if policy.DeletionTimestamp != nil { + logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) + continue + } + + if !isCertManagerInstalled { + s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrDependencyNotInstalled("Cert Manager")) + continue + } + + // TODO: This should be only one target ref for now, but what should happen if multiple target refs is supported in the future? + targetRefs := policy.GetTargetRefs() + for _, targetRef := range targetRefs { + // Find gateway defined by target ref + _, ok := lo.Find(gws, func(item *machinery.Gateway) bool { + if item.GetName() == targetRef.GetName() && item.GetNamespace() == targetRef.GetNamespace() { + return true + } + return false + }) + + // Can't find gateway target ref + if !ok { + logger.V(1).Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) + s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName()))) + continue + } + + logger.V(1).Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) + s.Store(TLSPolicyAcceptedKey(policy.GetUID()), nil) + } + } + + return nil +} diff --git a/controllers/tlspolicy_links.go b/controllers/tlspolicy_links.go deleted file mode 100644 index 08389be07..000000000 --- a/controllers/tlspolicy_links.go +++ /dev/null @@ -1,127 +0,0 @@ -package controllers - -import ( - 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" - gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" - - kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" -) - -func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { - gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) - - return machinery.LinkFunc{ - From: machinery.GatewayGroupKind, - To: CertManagerCertificateKind, - Func: func(child machinery.Object) []machinery.Object { - o := child.(*controller.RuntimeObject) - cert := o.Object.(*certmanagerv1.Certificate) - - gateway, ok := lo.Find(gateways, func(g *gwapiv1.Gateway) bool { - for _, l := range g.Spec.Listeners { - if l.TLS != nil && l.TLS.CertificateRefs != nil { - for _, certRef := range l.TLS.CertificateRefs { - certRefNS := "" - if certRef.Namespace == nil { - certRefNS = g.GetNamespace() - } else { - certRefNS = string(*certRef.Namespace) - } - if certRefNS == cert.GetNamespace() && string(certRef.Name) == cert.GetName() { - return true - } - } - } - } - - return false - }) - - if ok { - return []machinery.Object{&machinery.Gateway{Gateway: gateway}} - } - - return nil - }, - } -} - -func LinkGatewayToIssuerFunc(objs controller.Store) machinery.LinkFunc { - gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) - tlsPolicies := lo.Map(objs.FilterByGroupKind(kuadrantv1alpha1.TLSPolicyKind), controller.ObjectAs[*kuadrantv1alpha1.TLSPolicy]) - - return machinery.LinkFunc{ - From: machinery.GatewayGroupKind, - To: CertManagerIssuerKind, - Func: func(child machinery.Object) []machinery.Object { - o := child.(*controller.RuntimeObject) - issuer := o.Object.(*certmanagerv1.Issuer) - - // Policies linked to Issuer - // Issuer must be in the same namespace as the policy - linkedPolicies := lo.Filter(tlsPolicies, func(p *kuadrantv1alpha1.TLSPolicy, index int) bool { - 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} - }) - }, - } -} - -func LinkGatewayToClusterIssuerFunc(objs controller.Store) machinery.LinkFunc { - gateways := lo.Map(objs.FilterByGroupKind(machinery.GatewayGroupKind), controller.ObjectAs[*gwapiv1.Gateway]) - tlsPolicies := lo.Map(objs.FilterByGroupKind(kuadrantv1alpha1.TLSPolicyKind), controller.ObjectAs[*kuadrantv1alpha1.TLSPolicy]) - - return machinery.LinkFunc{ - From: machinery.GatewayGroupKind, - To: CertManagerClusterIssuerKind, - Func: func(child machinery.Object) []machinery.Object { - o := child.(*controller.RuntimeObject) - clusterIssuer := o.Object.(*certmanagerv1.ClusterIssuer) - - // Policies linked to ClusterIssuer - linkedPolicies := lo.Filter(tlsPolicies, func(p *kuadrantv1alpha1.TLSPolicy, index int) bool { - return p.Spec.IssuerRef.Name == clusterIssuer.GetName() && p.Spec.IssuerRef.Kind == certmanagerv1.ClusterIssuerKind - }) - - 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} - }) - }, - } -} diff --git a/controllers/tlspolicy_tasks.go b/controllers/tlspolicy_status_updater.go similarity index 57% rename from controllers/tlspolicy_tasks.go rename to controllers/tlspolicy_status_updater.go index b18bcb8a5..4bb0e510e 100644 --- a/controllers/tlspolicy_tasks.go +++ b/controllers/tlspolicy_status_updater.go @@ -7,133 +7,32 @@ import ( "slices" "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/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/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "k8s.io/utils/ptr" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) -var ( - CertManagerCertificatesResource = certmanagerv1.SchemeGroupVersion.WithResource("certificates") - CertManagerIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("issuers") - CertMangerClusterIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("clusterissuers") - - CertManagerCertificateKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.CertificateKind} - CertManagerIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.IssuerKind} - CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} -) - -func NewTLSPolicyWorkflow(client *dynamic.DynamicClient) *controller.Workflow { - return &controller.Workflow{ - Precondition: NewValidateTLSPolicyTask().Reconcile, - Postcondition: NewTLSPolicyStatusTask(client).Reconcile, - } -} - -type ValidateTLSPolicyTask struct{} - -func NewValidateTLSPolicyTask() *ValidateTLSPolicyTask { - return &ValidateTLSPolicyTask{} -} - -func (t *ValidateTLSPolicyTask) Subscription() *controller.Subscription { - return &controller.Subscription{ - Events: []controller.ResourceEventMatcher{ - {Kind: &machinery.GatewayGroupKind}, - {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.CreateEvent)}, - {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.UpdateEvent)}, - {Kind: &CertManagerCertificateKind}, - {Kind: &CertManagerIssuerKind}, - {Kind: &CertManagerClusterIssuerKind}, - }, - ReconcileFunc: t.Reconcile, - } -} - -func (t *ValidateTLSPolicyTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("ValidateTLSPolicyTask").WithName("Reconcile") - - // 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 - }) - - // Get all gateways - gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { - gw, ok := item.(*machinery.Gateway) - return gw, ok - }) - - isCertManagerInstalled := false - installed, ok := s.Load(IsCertManagerInstalledKey) - if ok { - isCertManagerInstalled = installed.(bool) - } else { - logger.V(1).Error(errors.New("isCertManagerInstalled was not found in sync map, defaulting to false"), "sync map error") - } - - for _, policy := range policies { - if policy.DeletionTimestamp != nil { - logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) - continue - } - - if !isCertManagerInstalled { - s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrDependencyNotInstalled("Cert Manager")) - continue - } - - // TODO: This should be only one target ref for now, but what should happen if multiple target refs is supported in the future? - targetRefs := policy.GetTargetRefs() - for _, targetRef := range targetRefs { - // Find gateway defined by target ref - _, ok := lo.Find(gws, func(item *machinery.Gateway) bool { - if item.GetName() == targetRef.GetName() && item.GetNamespace() == targetRef.GetNamespace() { - return true - } - return false - }) - - // Can't find gateway target ref - if !ok { - logger.V(1).Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) - s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName()))) - continue - } - - logger.V(1).Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) - s.Store(TLSPolicyAcceptedKey(policy.GetUID()), nil) - } - } - - return nil -} - -type TLSPolicyStatusTask struct { +type TLSPolicyStatusUpdaterReconciler struct { Client *dynamic.DynamicClient } -func NewTLSPolicyStatusTask(client *dynamic.DynamicClient) *TLSPolicyStatusTask { - return &TLSPolicyStatusTask{Client: client} +func NewTLSPolicyStatusUpdaterReconciler(client *dynamic.DynamicClient) *TLSPolicyStatusUpdaterReconciler { + return &TLSPolicyStatusUpdaterReconciler{Client: client} } -func (t *TLSPolicyStatusTask) Subscription() *controller.Subscription { +func (t *TLSPolicyStatusUpdaterReconciler) Subscription() *controller.Subscription { return &controller.Subscription{ Events: []controller.ResourceEventMatcher{ {Kind: &machinery.GatewayGroupKind}, @@ -143,12 +42,12 @@ func (t *TLSPolicyStatusTask) Subscription() *controller.Subscription { {Kind: &CertManagerIssuerKind}, {Kind: &CertManagerClusterIssuerKind}, }, - ReconcileFunc: t.Reconcile, + ReconcileFunc: t.UpdateStatus, } } -func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusTask").WithName("Reconcile") +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") policies := lo.FilterMap(topology.Policies().Items(), func(item machinery.Policy, index int) (*kuadrantv1alpha1.TLSPolicy, bool) { p, ok := item.(*kuadrantv1alpha1.TLSPolicy) @@ -207,7 +106,7 @@ func (t *TLSPolicyStatusTask) Reconcile(ctx context.Context, _ []controller.Reso return nil } -func (t *TLSPolicyStatusTask) enforcedCondition(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) *metav1.Condition { +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) } @@ -219,8 +118,8 @@ func (t *TLSPolicyStatusTask) enforcedCondition(ctx context.Context, tlsPolicy * return kuadrant.EnforcedCondition(tlsPolicy, nil, true) } -func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { - logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusTask").WithName("isIssuerReady") +func (t *TLSPolicyStatusUpdaterReconciler) isIssuerReady(ctx context.Context, tlsPolicy *kuadrantv1alpha1.TLSPolicy, topology *machinery.Topology) error { + logger := controller.LoggerFromContext(ctx).WithName("TLSPolicyStatusUpdaterReconciler").WithName("isIssuerReady") // Get all gateways gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { @@ -285,7 +184,7 @@ func (t *TLSPolicyStatusTask) isIssuerReady(ctx context.Context, tlsPolicy *kuad return nil } -func (t *TLSPolicyStatusTask) isCertificatesReady(ctx context.Context, 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") @@ -329,31 +228,6 @@ func (t *TLSPolicyStatusTask) isCertificatesReady(ctx context.Context, p machine return nil } -const IsCertManagerInstalledKey = "IsCertManagerInstalled" - -func NewIsCertManagerInstalledTask(restMapper meta.RESTMapper) IsCertManagerInstalledTask { - return IsCertManagerInstalledTask{ - restMapper: restMapper, - } -} - -type IsCertManagerInstalledTask struct { - restMapper meta.RESTMapper -} - -func (t IsCertManagerInstalledTask) Reconcile(ctx context.Context, _ []controller.ResourceEvent, _ *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("IsCertManagerInstalledTask").WithName("Reconcile") - isCertManagerInstalled, err := kuadrantgatewayapi.IsCertManagerInstalled(t.restMapper, logger) - - if err != nil { - logger.Error(err, "error checking IsCertManagerInstalled") - } - - s.Store(IsCertManagerInstalledKey, isCertManagerInstalled) - - return nil -} - func TLSPolicyAcceptedKey(uid types.UID) string { return fmt.Sprintf("TLSPolicyValid:%s", uid) } diff --git a/controllers/tlspolicy_tasks_test.go b/controllers/tlspolicy_status_updater_test.go similarity index 99% rename from controllers/tlspolicy_tasks_test.go rename to controllers/tlspolicy_status_updater_test.go index 0724a2d34..ea6d90a76 100644 --- a/controllers/tlspolicy_tasks_test.go +++ b/controllers/tlspolicy_status_updater_test.go @@ -486,7 +486,7 @@ func TestTLSPolicyStatusTask_enforcedCondition(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t1 *testing.T) { - t := &TLSPolicyStatusTask{} + t := &TLSPolicyStatusUpdaterReconciler{} 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 fddf4eebef7a25301d7998dde5a9a10fe223ff6f Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 4 Oct 2024 17:16:28 +0100 Subject: [PATCH 09/16] fixup: map[string]error for TLSPolicy validation & refine target not found logic Signed-off-by: KevFan --- controllers/tls_workflow.go | 4 ++ controllers/tlspolicies_validator.go | 48 +++++++------------- controllers/tlspolicy_status_updater.go | 23 +++++----- controllers/tlspolicy_status_updater_test.go | 26 ----------- 4 files changed, 32 insertions(+), 69 deletions(-) diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 47c46b2c0..961ce19cd 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -13,6 +13,10 @@ import ( kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" ) +const ( + TLSPolicyAcceptedKey = "TLSPolicyValid" +) + var ( CertManagerCertificatesResource = certmanagerv1.SchemeGroupVersion.WithResource("certificates") CertManagerIssuersResource = certmanagerv1.SchemeGroupVersion.WithResource("issuers") diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index f085cd7e5..334368b0c 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -25,8 +25,8 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subs return &controller.Subscription{ Events: []controller.ResourceEventMatcher{ {Kind: &machinery.GatewayGroupKind}, - {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.CreateEvent)}, - {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.UpdateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.CreateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.UpdateEvent)}, {Kind: &CertManagerCertificateKind}, {Kind: &CertManagerIssuerKind}, {Kind: &CertManagerClusterIssuerKind}, @@ -44,11 +44,7 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ return p, ok }) - // Get all gateways - gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { - gw, ok := item.(*machinery.Gateway) - return gw, ok - }) + isPolicyValidErrorMap := make(map[string]error, len(policies)) isCertManagerInstalled := false installed, ok := s.Load(IsCertManagerInstalledKey) @@ -58,39 +54,29 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ logger.V(1).Error(errors.New("isCertManagerInstalled was not found in sync map, defaulting to false"), "sync map error") } - for _, policy := range policies { - if policy.DeletionTimestamp != nil { - logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) + for _, p := range policies { + if p.DeletionTimestamp != nil { + logger.V(1).Info("tls policy is marked for deletion, skipping", "name", p.Name, "namespace", p.Namespace) continue } if !isCertManagerInstalled { - s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrDependencyNotInstalled("Cert Manager")) + isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrDependencyNotInstalled("Cert Manager") continue } - // TODO: This should be only one target ref for now, but what should happen if multiple target refs is supported in the future? - targetRefs := policy.GetTargetRefs() - for _, targetRef := range targetRefs { - // Find gateway defined by target ref - _, ok := lo.Find(gws, func(item *machinery.Gateway) bool { - if item.GetName() == targetRef.GetName() && item.GetNamespace() == targetRef.GetNamespace() { - return true - } - return false - }) - - // Can't find gateway target ref - if !ok { - logger.V(1).Info("tls policy cannot find target ref", "name", policy.Name, "namespace", policy.Namespace) - s.Store(TLSPolicyAcceptedKey(policy.GetUID()), kuadrant.NewErrTargetNotFound(policy.Kind(), policy.GetTargetRef(), apierrors.NewNotFound(kuadrantv1alpha1.TLSPoliciesResource.GroupResource(), policy.GetName()))) - continue - } - - logger.V(1).Info("tls policy found target ref", "name", policy.Name, "namespace", policy.Namespace) - s.Store(TLSPolicyAcceptedKey(policy.GetUID()), nil) + // 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())) + continue } + + isPolicyValidErrorMap[p.GetLocator()] = nil } + s.Store(TLSPolicyAcceptedKey, isPolicyValidErrorMap) + return nil } diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index 4bb0e510e..e13a0a370 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/types" "k8s.io/client-go/dynamic" "k8s.io/utils/ptr" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -36,8 +35,8 @@ func (t *TLSPolicyStatusUpdaterReconciler) Subscription() *controller.Subscripti return &controller.Subscription{ Events: []controller.ResourceEventMatcher{ {Kind: &machinery.GatewayGroupKind}, - {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.CreateEvent)}, - {Kind: &kuadrantv1alpha1.TLSPolicyKind, EventType: ptr.To(controller.UpdateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.CreateEvent)}, + {Kind: &kuadrantv1alpha1.TLSPolicyGroupKind, EventType: ptr.To(controller.UpdateEvent)}, {Kind: &CertManagerCertificateKind}, {Kind: &CertManagerIssuerKind}, {Kind: &CertManagerClusterIssuerKind}, @@ -54,6 +53,14 @@ 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.Name, "namespace", policy.Namespace) @@ -66,11 +73,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ ObservedGeneration: policy.Status.ObservedGeneration, } - var err error - accepted, ok := s.Load(TLSPolicyAcceptedKey(policy.GetUID())) - if ok && accepted != nil { - err = accepted.(error) - } + err := isPolicyValidErrorMap[policy.GetLocator()] meta.SetStatusCondition(&newStatus.Conditions, *kuadrant.AcceptedCondition(policy, err)) // Do not set enforced condition if Accepted condition is false @@ -227,7 +230,3 @@ func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(ctx context.Conte return nil } - -func TLSPolicyAcceptedKey(uid types.UID) string { - return fmt.Sprintf("TLSPolicyValid:%s", uid) -} diff --git a/controllers/tlspolicy_status_updater_test.go b/controllers/tlspolicy_status_updater_test.go index ea6d90a76..0c4675d5e 100644 --- a/controllers/tlspolicy_status_updater_test.go +++ b/controllers/tlspolicy_status_updater_test.go @@ -23,32 +23,6 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) -func TestTLSPolicyAcceptedKey(t *testing.T) { - type args struct { - uid types.UID - } - tests := []struct { - name string - args args - want string - }{ - { - name: "test uid is appended", - args: args{ - types.UID("unqiueid"), - }, - want: "TLSPolicyValid:unqiueid", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := TLSPolicyAcceptedKey(tt.args.uid); got != tt.want { - t.Errorf("TLSPolicyValidKey() = %v, want %v", got, tt.want) - } - }) - } -} - func TestTLSPolicyStatusTask_enforcedCondition(t *testing.T) { const ( ns = "default" From 36081c46f89c76ab9c1ec32fdb621a4cc9e4132d Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 7 Oct 2024 09:22:44 +0100 Subject: [PATCH 10/16] refactor: link certs to listeners instead Signed-off-by: KevFan --- controllers/state_of_the_world.go | 2 +- controllers/tls_workflow.go | 37 +++++++++++-------- .../tlspolicy_certmanager_certificates.go | 25 +++++++++++++ controllers/tlspolicy_status_updater.go | 28 +++++++++----- controllers/tlspolicy_status_updater_test.go | 3 +- 5 files changed, 67 insertions(+), 28 deletions(-) diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 16992c2f2..f34db7d4d 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -278,7 +278,7 @@ func certManagerControllerOpts() []controller.ControllerOption { CertManagerClusterIssuerKind, ), controller.WithObjectLinks( - LinkGatewayToCertificateFunc, + LinkListenerToCertificateFunc, LinkGatewayToIssuerFunc, LinkGatewayToClusterIssuerFunc, ), diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index 961ce19cd..fe51ff90f 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -34,29 +34,34 @@ func NewTLSWorkflow(client *dynamic.DynamicClient) *controller.Workflow { } } -func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { +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 { + return &machinery.Gateway{Gateway: g} + }), machinery.ListenersFromGatewayFunc) return machinery.LinkFunc{ - From: machinery.GatewayGroupKind, + From: machinery.ListenerGroupKind, To: CertManagerCertificateKind, Func: func(child machinery.Object) []machinery.Object { o := child.(*controller.RuntimeObject) cert := o.Object.(*certmanagerv1.Certificate) - gateway, ok := lo.Find(gateways, func(g *gwapiv1.Gateway) bool { - for _, l := range g.Spec.Listeners { - if l.TLS != nil && l.TLS.CertificateRefs != nil { - for _, certRef := range l.TLS.CertificateRefs { - certRefNS := "" - if certRef.Namespace == nil { - certRefNS = g.GetNamespace() - } else { - certRefNS = string(*certRef.Namespace) - } - if certRefNS == cert.GetNamespace() && string(certRef.Name) == cert.GetName() { - return true - } + if len(listeners) == 0 { + return nil + } + + listener, ok := lo.Find(listeners, func(l *machinery.Listener) bool { + if l.TLS != nil && l.TLS.CertificateRefs != nil { + for _, certRef := range l.TLS.CertificateRefs { + certRefNS := "" + if certRef.Namespace == nil { + certRefNS = l.GetNamespace() + } else { + certRefNS = string(*certRef.Namespace) + } + if certRefNS == cert.GetNamespace() && string(certRef.Name) == cert.GetName() { + return true } } } @@ -65,7 +70,7 @@ func LinkGatewayToCertificateFunc(objs controller.Store) machinery.LinkFunc { }) if ok { - return []machinery.Object{&machinery.Gateway{Gateway: gateway}} + return []machinery.Object{listener} } return nil diff --git a/controllers/tlspolicy_certmanager_certificates.go b/controllers/tlspolicy_certmanager_certificates.go index 4a98e6656..2e6dd3fc1 100644 --- a/controllers/tlspolicy_certmanager_certificates.go +++ b/controllers/tlspolicy_certmanager_certificates.go @@ -7,6 +7,7 @@ import ( "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" @@ -135,6 +136,30 @@ func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.G return certs } +func expectedCertificatesForListener(l *machinery.Listener, tlsPolicy *v1alpha1.TLSPolicy) []*certmanv1.Certificate { + tlsHosts := make(map[corev1.ObjectReference][]string) + + 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], string(*l.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) diff --git a/controllers/tlspolicy_status_updater.go b/controllers/tlspolicy_status_updater.go index e13a0a370..cb90c36ce 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" @@ -114,7 +115,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) } @@ -187,27 +188,34 @@ 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") } - // Get all gateways that contains this policy - gws := lo.FilterMap(topology.Targetables().Items(), func(item machinery.Targetable, index int) (*machinery.Gateway, bool) { - gw, ok := item.(*machinery.Gateway) - return gw, ok && lo.Contains(gw.Policies(), p) + // 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) }) - if len(gws) == 0 { + if len(listeners) == 0 { return errors.New("no valid gateways found") } - for _, gw := range gws { - expectedCertificates := expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) + for i, l := range listeners { + // Not valid - so no need to check if cert is ready since it 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() }) diff --git a/controllers/tlspolicy_status_updater_test.go b/controllers/tlspolicy_status_updater_test.go index 0c4675d5e..f43199e21 100644 --- a/controllers/tlspolicy_status_updater_test.go +++ b/controllers/tlspolicy_status_updater_test.go @@ -221,8 +221,9 @@ func TestTLSPolicyStatusTask_enforcedCondition(t *testing.T) { opts := []machinery.GatewayAPITopologyOptionsFunc{ machinery.WithGateways(gw), machinery.WithGatewayAPITopologyPolicies(policy), + machinery.ExpandGatewayListeners(), machinery.WithGatewayAPITopologyLinks( - LinkGatewayToCertificateFunc(store), + LinkListenerToCertificateFunc(store), LinkGatewayToIssuerFunc(store), LinkGatewayToClusterIssuerFunc(store), ), From 1880122b927014e66594a58ebf9d7a65c7dfc0b6 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 7 Oct 2024 11:53:41 +0100 Subject: [PATCH 11/16] fixup: subscribe to events for tls status Signed-off-by: KevFan --- controllers/tls_workflow.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index fe51ff90f..fd0827672 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -29,8 +29,8 @@ var ( func NewTLSWorkflow(client *dynamic.DynamicClient) *controller.Workflow { return &controller.Workflow{ - Precondition: NewValidateTLSPoliciesValidatorReconciler().Validate, - Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).UpdateStatus, + Precondition: NewValidateTLSPoliciesValidatorReconciler().Subscription().Reconcile, + Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).Subscription().Reconcile, } } From fef1ae4dc14dbcc6dbc6ccbcd88272ff0aee647c Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 7 Oct 2024 13:05:12 +0100 Subject: [PATCH 12/16] refactor: check for isCertManagerInstalled only from boot Signed-off-by: KevFan --- controllers/cert_manager_installed.go | 37 --------------------------- controllers/state_of_the_world.go | 16 +++++------- controllers/tls_workflow.go | 4 +-- controllers/tlspolicies_validator.go | 21 ++++++--------- 4 files changed, 17 insertions(+), 61 deletions(-) delete mode 100644 controllers/cert_manager_installed.go diff --git a/controllers/cert_manager_installed.go b/controllers/cert_manager_installed.go deleted file mode 100644 index 87f030b57..000000000 --- a/controllers/cert_manager_installed.go +++ /dev/null @@ -1,37 +0,0 @@ -package controllers - -import ( - "context" - "sync" - - "github.com/kuadrant/policy-machinery/controller" - "github.com/kuadrant/policy-machinery/machinery" - "k8s.io/apimachinery/pkg/api/meta" - - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" -) - -const IsCertManagerInstalledKey = "IsCertManagerInstalled" - -func NewIsCertManagerInstalledReconciler(restMapper meta.RESTMapper) IsCertManagerInstalledReconciler { - return IsCertManagerInstalledReconciler{ - restMapper: restMapper, - } -} - -type IsCertManagerInstalledReconciler struct { - restMapper meta.RESTMapper -} - -func (t IsCertManagerInstalledReconciler) Check(ctx context.Context, _ []controller.ResourceEvent, _ *machinery.Topology, _ error, s *sync.Map) error { - logger := controller.LoggerFromContext(ctx).WithName("IsCertManagerInstalledReconciler").WithName("Reconcile") - isCertManagerInstalled, err := kuadrantgatewayapi.IsCertManagerInstalled(t.restMapper, logger) - - if err != nil { - logger.Error(err, "error checking IsCertManagerInstalled") - } - - s.Store(IsCertManagerInstalledKey, isCertManagerInstalled) - - return nil -} diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index f34db7d4d..652247a77 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -15,7 +15,6 @@ import ( istioclientnetworkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" istioclientgosecurityv1beta1 "istio.io/client-go/pkg/apis/security/v1beta1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -116,8 +115,8 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D ), } - ok, err := kuadrantgatewayapi.IsGatewayAPIInstalled(manager.GetRESTMapper()) - if err != nil || !ok { + isGatewayAPIInstalled, err := kuadrantgatewayapi.IsGatewayAPIInstalled(manager.GetRESTMapper()) + if err != nil || !isGatewayAPIInstalled { logger.Info("gateway api is not installed, skipping watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, @@ -218,7 +217,7 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D ) } - controllerOpts = append(controllerOpts, controller.WithReconcile(buildReconciler(manager, client, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled))) + controllerOpts = append(controllerOpts, controller.WithReconcile(buildReconciler(manager, client, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled, isCertManagerInstalled))) return controller.NewController(controllerOpts...) } @@ -285,14 +284,14 @@ func certManagerControllerOpts() []controller.ControllerOption { } } -func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled bool) controller.ReconcileFunc { +func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled, isCertManagerInstalled bool) controller.ReconcileFunc { mainWorkflow := &controller.Workflow{ - Precondition: initWorkflow(client, manager.GetRESTMapper()).Run, + Precondition: initWorkflow(client).Run, Tasks: []controller.ReconcileFunc{ NewAuthorinoReconciler(client).Subscription().Reconcile, NewLimitadorReconciler(client).Subscription().Reconcile, NewDNSWorkflow().Run, - NewTLSWorkflow(client).Run, + NewTLSWorkflow(client, isCertManagerInstalled).Run, NewAuthWorkflow().Run, NewRateLimitWorkflow().Run, }, @@ -308,12 +307,11 @@ func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, return mainWorkflow.Run } -func initWorkflow(client *dynamic.DynamicClient, restMapper meta.RESTMapper) *controller.Workflow { +func initWorkflow(client *dynamic.DynamicClient) *controller.Workflow { return &controller.Workflow{ Precondition: NewEventLogger().Log, Tasks: []controller.ReconcileFunc{ NewTopologyReconciler(client, operatorNamespace).Reconcile, - NewIsCertManagerInstalledReconciler(restMapper).Check, }, } } diff --git a/controllers/tls_workflow.go b/controllers/tls_workflow.go index fd0827672..627fa1896 100644 --- a/controllers/tls_workflow.go +++ b/controllers/tls_workflow.go @@ -27,9 +27,9 @@ var ( CertManagerClusterIssuerKind = schema.GroupKind{Group: certmanager.GroupName, Kind: certmanagerv1.ClusterIssuerKind} ) -func NewTLSWorkflow(client *dynamic.DynamicClient) *controller.Workflow { +func NewTLSWorkflow(client *dynamic.DynamicClient, isCertManagerInstalled bool) *controller.Workflow { return &controller.Workflow{ - Precondition: NewValidateTLSPoliciesValidatorReconciler().Subscription().Reconcile, + Precondition: NewValidateTLSPoliciesValidatorReconciler(isCertManagerInstalled).Subscription().Reconcile, Postcondition: NewTLSPolicyStatusUpdaterReconciler(client).Subscription().Reconcile, } } diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index 334368b0c..368395caf 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "errors" "sync" "github.com/kuadrant/policy-machinery/controller" @@ -15,11 +14,15 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" ) -func NewValidateTLSPoliciesValidatorReconciler() *ValidateTLSPoliciesValidatorReconciler { - return &ValidateTLSPoliciesValidatorReconciler{} +func NewValidateTLSPoliciesValidatorReconciler(isCertManagerInstalled bool) *ValidateTLSPoliciesValidatorReconciler { + return &ValidateTLSPoliciesValidatorReconciler{ + isCertManagerInstalled: isCertManagerInstalled, + } } -type ValidateTLSPoliciesValidatorReconciler struct{} +type ValidateTLSPoliciesValidatorReconciler struct { + isCertManagerInstalled bool +} func (t *ValidateTLSPoliciesValidatorReconciler) Subscription() *controller.Subscription { return &controller.Subscription{ @@ -46,21 +49,13 @@ func (t *ValidateTLSPoliciesValidatorReconciler) Validate(ctx context.Context, _ isPolicyValidErrorMap := make(map[string]error, len(policies)) - isCertManagerInstalled := false - installed, ok := s.Load(IsCertManagerInstalledKey) - if ok { - isCertManagerInstalled = installed.(bool) - } else { - logger.V(1).Error(errors.New("isCertManagerInstalled was not found in sync map, defaulting to false"), "sync map error") - } - for _, p := range policies { if p.DeletionTimestamp != nil { logger.V(1).Info("tls policy is marked for deletion, skipping", "name", p.Name, "namespace", p.Namespace) continue } - if !isCertManagerInstalled { + if !t.isCertManagerInstalled { isPolicyValidErrorMap[p.GetLocator()] = kuadrant.NewErrDependencyNotInstalled("Cert Manager") continue } From e974c4b4af600acf67559ca8cc57e5547e5c6f22 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 7 Oct 2024 13:46:30 +0100 Subject: [PATCH 13/16] refactor: struct to store state of dependencies in initial boot Signed-off-by: KevFan --- controllers/state_of_the_world.go | 41 +++++++++++++++++++------------ controllers/test_common.go | 2 +- main.go | 2 +- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 652247a77..17699142d 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -42,9 +42,17 @@ var ( operatorNamespace = env.GetString("OPERATOR_NAMESPACE", "kuadrant-system") ) +type PolicyMachineryController struct { + isGatewayAPIInstalled bool + isEnvoyGatewayInstalled bool + isIstioInstalled bool + isCertManagerInstalled bool + isConsolePluginInstalled bool +} + //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gatewayclasses,verbs=list;watch -func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.DynamicClient, logger logr.Logger) *controller.Controller { +func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, client *dynamic.DynamicClient, logger logr.Logger) *controller.Controller { controllerOpts := []controller.ControllerOption{ controller.ManagedBy(manager), controller.WithLogger(logger), @@ -115,8 +123,9 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D ), } - isGatewayAPIInstalled, err := kuadrantgatewayapi.IsGatewayAPIInstalled(manager.GetRESTMapper()) - if err != nil || !isGatewayAPIInstalled { + var err error + c.isGatewayAPIInstalled, err = kuadrantgatewayapi.IsGatewayAPIInstalled(manager.GetRESTMapper()) + if err != nil || !c.isGatewayAPIInstalled { logger.Info("gateway api is not installed, skipping watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, @@ -138,8 +147,8 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D ) } - isEnvoyGatewayInstalled, err := envoygateway.IsEnvoyGatewayInstalled(manager.GetRESTMapper()) - if err != nil || !isEnvoyGatewayInstalled { + c.isEnvoyGatewayInstalled, err = envoygateway.IsEnvoyGatewayInstalled(manager.GetRESTMapper()) + if err != nil || !c.isEnvoyGatewayInstalled { logger.Info("envoygateway is not installed, skipping related watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, @@ -168,8 +177,8 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D // TODO: add specific tasks to workflow } - isIstioInstalled, err := istio.IsIstioInstalled(manager.GetRESTMapper()) - if err != nil || !isIstioInstalled { + c.isIstioInstalled, err = istio.IsIstioInstalled(manager.GetRESTMapper()) + if err != nil || !c.isIstioInstalled { logger.Info("istio is not installed, skipping related watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, @@ -198,15 +207,15 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D // TODO: add istio specific tasks to workflow } - isCertManagerInstalled, err := kuadrantgatewayapi.IsCertManagerInstalled(manager.GetRESTMapper(), logger) - if err != nil || !isCertManagerInstalled { + c.isCertManagerInstalled, err = kuadrantgatewayapi.IsCertManagerInstalled(manager.GetRESTMapper(), logger) + if err != nil || !c.isCertManagerInstalled { logger.Info("cert manager is not installed, skipping related watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, certManagerControllerOpts()...) } - isConsolePluginInstalled, err := openshift.IsConsolePluginInstalled(manager.GetRESTMapper()) - if err != nil || !isConsolePluginInstalled { + c.isConsolePluginInstalled, err = openshift.IsConsolePluginInstalled(manager.GetRESTMapper()) + if err != nil || !c.isConsolePluginInstalled { logger.Info("console plugin is not installed, skipping related watches and reconcilers", "err", err) } else { controllerOpts = append(controllerOpts, @@ -217,7 +226,7 @@ func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.D ) } - controllerOpts = append(controllerOpts, controller.WithReconcile(buildReconciler(manager, client, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled, isCertManagerInstalled))) + controllerOpts = append(controllerOpts, controller.WithReconcile(c.buildReconciler(manager, client))) return controller.NewController(controllerOpts...) } @@ -284,21 +293,21 @@ func certManagerControllerOpts() []controller.ControllerOption { } } -func buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient, isIstioInstalled, isEnvoyGatewayInstalled, isConsolePluginInstalled, isCertManagerInstalled bool) controller.ReconcileFunc { +func (c *PolicyMachineryController) buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient) controller.ReconcileFunc { mainWorkflow := &controller.Workflow{ Precondition: initWorkflow(client).Run, Tasks: []controller.ReconcileFunc{ NewAuthorinoReconciler(client).Subscription().Reconcile, NewLimitadorReconciler(client).Subscription().Reconcile, NewDNSWorkflow().Run, - NewTLSWorkflow(client, isCertManagerInstalled).Run, + NewTLSWorkflow(client, c.isCertManagerInstalled).Run, NewAuthWorkflow().Run, NewRateLimitWorkflow().Run, }, - Postcondition: finalStepsWorkflow(client, isIstioInstalled, isEnvoyGatewayInstalled).Run, + Postcondition: finalStepsWorkflow(client, c.isIstioInstalled, c.isGatewayAPIInstalled).Run, } - if isConsolePluginInstalled { + if c.isConsolePluginInstalled { mainWorkflow.Tasks = append(mainWorkflow.Tasks, NewConsolePluginReconciler(manager, operatorNamespace).Subscription().Reconcile, ) diff --git a/controllers/test_common.go b/controllers/test_common.go index 43d355ab9..cb1357fd9 100644 --- a/controllers/test_common.go +++ b/controllers/test_common.go @@ -262,7 +262,7 @@ func SetupKuadrantOperatorForTest(s *runtime.Scheme, cfg *rest.Config) { dClient, err := dynamic.NewForConfig(mgr.GetConfig()) Expect(err).NotTo(HaveOccurred()) - stateOfTheWorld := NewPolicyMachineryController(mgr, dClient, log.Log) + stateOfTheWorld := (&PolicyMachineryController{}).Controller(mgr, dClient, log.Log) go func() { defer GinkgoRecover() diff --git a/main.go b/main.go index e9aa559e4..f9e193b60 100644 --- a/main.go +++ b/main.go @@ -371,7 +371,7 @@ func main() { os.Exit(1) } - stateOfTheWorld := controllers.NewPolicyMachineryController(mgr, client, log.Log) + stateOfTheWorld := (&controllers.PolicyMachineryController{}).Controller(mgr, client, log.Log) if err = stateOfTheWorld.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "unable to start stateOfTheWorld controller") os.Exit(1) From c83ca3dc1db57c2ca36d663373dcff01e6903910 Mon Sep 17 00:00:00 2001 From: KevFan Date: Tue, 8 Oct 2024 14:21:31 +0100 Subject: [PATCH 14/16] refactor: bootOptionsBuilder Signed-off-by: KevFan --- controllers/state_of_the_world.go | 168 ++++++++++++++++++++---------- controllers/test_common.go | 2 +- main.go | 2 +- 3 files changed, 116 insertions(+), 56 deletions(-) diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go index 17699142d..3a6a31d7d 100644 --- a/controllers/state_of_the_world.go +++ b/controllers/state_of_the_world.go @@ -42,17 +42,10 @@ var ( operatorNamespace = env.GetString("OPERATOR_NAMESPACE", "kuadrant-system") ) -type PolicyMachineryController struct { - isGatewayAPIInstalled bool - isEnvoyGatewayInstalled bool - isIstioInstalled bool - isCertManagerInstalled bool - isConsolePluginInstalled bool -} - //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gatewayclasses,verbs=list;watch -func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, client *dynamic.DynamicClient, logger logr.Logger) *controller.Controller { +func NewPolicyMachineryController(manager ctrlruntime.Manager, client *dynamic.DynamicClient, logger logr.Logger) *controller.Controller { + // Base options controllerOpts := []controller.ControllerOption{ controller.ManagedBy(manager), controller.WithLogger(logger), @@ -94,6 +87,7 @@ func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, clie controller.WithPredicates(&ctrlruntimepredicate.TypedGenerationChangedPredicate[*corev1.ConfigMap]{}), controller.FilterResourcesByLabel[*corev1.ConfigMap](fmt.Sprintf("%s=true", kuadrant.TopologyLabel)), )), + // TODO: Move as boot options for Limitador and Authorino as there can be a possibility that the operators are not installed controller.WithRunnable("limitador watcher", controller.Watch( &limitadorv1alpha1.Limitador{}, kuadrantv1beta1.LimitadorsResource, @@ -123,12 +117,56 @@ func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, clie ), } + // Boot options and reconciler based on detected dependencies + bootOptions := NewBootOptionsBuilder(manager, client, logger) + controllerOpts = append(controllerOpts, bootOptions.getOptions()...) + controllerOpts = append(controllerOpts, controller.WithReconcile(bootOptions.Reconciler())) + + return controller.NewController(controllerOpts...) +} + +// NewBootOptionsBuilder is used to return a list of controller.ControllerOption and a controller.ReconcileFunc that depend +// on if external dependent CRDs are installed at boot time +func NewBootOptionsBuilder(manager ctrlruntime.Manager, client *dynamic.DynamicClient, logger logr.Logger) *BootOptionsBuilder { + return &BootOptionsBuilder{ + manager: manager, + client: client, + logger: logger, + } +} + +type BootOptionsBuilder struct { + logger logr.Logger + manager ctrlruntime.Manager + client *dynamic.DynamicClient + + // Internal configurations + isGatewayAPIInstalled bool + isEnvoyGatewayInstalled bool + isIstioInstalled bool + isCertManagerInstalled bool + isConsolePluginInstalled bool +} + +func (b *BootOptionsBuilder) getOptions() []controller.ControllerOption { + var opts []controller.ControllerOption + opts = append(opts, b.getGatewayAPIOptions()...) + opts = append(opts, b.getIstioOptions()...) + opts = append(opts, b.getEnvoyGatewayOptions()...) + opts = append(opts, b.getCertManagerOptions()...) + opts = append(opts, b.getConsolePluginOptions()...) + + return opts +} + +func (b *BootOptionsBuilder) getGatewayAPIOptions() []controller.ControllerOption { + var opts []controller.ControllerOption var err error - c.isGatewayAPIInstalled, err = kuadrantgatewayapi.IsGatewayAPIInstalled(manager.GetRESTMapper()) - if err != nil || !c.isGatewayAPIInstalled { - logger.Info("gateway api is not installed, skipping watches and reconcilers", "err", err) + b.isGatewayAPIInstalled, err = kuadrantgatewayapi.IsGatewayAPIInstalled(b.manager.GetRESTMapper()) + if err != nil || !b.isGatewayAPIInstalled { + b.logger.Info("gateway api is not installed, skipping watches and reconcilers", "err", err) } else { - controllerOpts = append(controllerOpts, + opts = append(opts, controller.WithRunnable("gatewayclass watcher", controller.Watch( &gwapiv1.GatewayClass{}, controller.GatewayClassesResource, @@ -147,11 +185,17 @@ func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, clie ) } - c.isEnvoyGatewayInstalled, err = envoygateway.IsEnvoyGatewayInstalled(manager.GetRESTMapper()) - if err != nil || !c.isEnvoyGatewayInstalled { - logger.Info("envoygateway is not installed, skipping related watches and reconcilers", "err", err) + return opts +} + +func (b *BootOptionsBuilder) getEnvoyGatewayOptions() []controller.ControllerOption { + var opts []controller.ControllerOption + var err error + b.isEnvoyGatewayInstalled, err = envoygateway.IsEnvoyGatewayInstalled(b.manager.GetRESTMapper()) + if err != nil || !b.isEnvoyGatewayInstalled { + b.logger.Info("envoygateway is not installed, skipping related watches and reconcilers", "err", err) } else { - controllerOpts = append(controllerOpts, + opts = append(opts, controller.WithRunnable("envoypatchpolicy watcher", controller.Watch( &egv1alpha1.EnvoyPatchPolicy{}, envoygateway.EnvoyPatchPoliciesResource, @@ -177,11 +221,17 @@ func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, clie // TODO: add specific tasks to workflow } - c.isIstioInstalled, err = istio.IsIstioInstalled(manager.GetRESTMapper()) - if err != nil || !c.isIstioInstalled { - logger.Info("istio is not installed, skipping related watches and reconcilers", "err", err) + return opts +} + +func (b *BootOptionsBuilder) getIstioOptions() []controller.ControllerOption { + var opts []controller.ControllerOption + var err error + b.isIstioInstalled, err = istio.IsIstioInstalled(b.manager.GetRESTMapper()) + if err != nil || !b.isIstioInstalled { + b.logger.Info("istio is not installed, skipping related watches and reconcilers", "err", err) } else { - controllerOpts = append(controllerOpts, + opts = append(opts, controller.WithRunnable("envoyfilter watcher", controller.Watch( &istioclientnetworkingv1alpha3.EnvoyFilter{}, istio.EnvoyFiltersResource, @@ -207,18 +257,30 @@ func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, clie // TODO: add istio specific tasks to workflow } - c.isCertManagerInstalled, err = kuadrantgatewayapi.IsCertManagerInstalled(manager.GetRESTMapper(), logger) - if err != nil || !c.isCertManagerInstalled { - logger.Info("cert manager is not installed, skipping related watches and reconcilers", "err", err) + return opts +} + +func (b *BootOptionsBuilder) getCertManagerOptions() []controller.ControllerOption { + var opts []controller.ControllerOption + var err error + b.isCertManagerInstalled, err = kuadrantgatewayapi.IsCertManagerInstalled(b.manager.GetRESTMapper(), b.logger) + if err != nil || !b.isCertManagerInstalled { + b.logger.Info("cert manager is not installed, skipping related watches and reconcilers", "err", err) } else { - controllerOpts = append(controllerOpts, certManagerControllerOpts()...) + opts = append(opts, certManagerControllerOpts()...) } - c.isConsolePluginInstalled, err = openshift.IsConsolePluginInstalled(manager.GetRESTMapper()) - if err != nil || !c.isConsolePluginInstalled { - logger.Info("console plugin is not installed, skipping related watches and reconcilers", "err", err) + return opts +} + +func (b *BootOptionsBuilder) getConsolePluginOptions() []controller.ControllerOption { + var opts []controller.ControllerOption + var err error + b.isConsolePluginInstalled, err = openshift.IsConsolePluginInstalled(b.manager.GetRESTMapper()) + if err != nil || !b.isConsolePluginInstalled { + b.logger.Info("console plugin is not installed, skipping related watches and reconcilers", "err", err) } else { - controllerOpts = append(controllerOpts, + opts = append(opts, controller.WithRunnable("consoleplugin watcher", controller.Watch( &consolev1.ConsolePlugin{}, openshift.ConsolePluginsResource, metav1.NamespaceAll, controller.FilterResourcesByLabel[*consolev1.ConsolePlugin](fmt.Sprintf("%s=%s", consoleplugin.AppLabelKey, consoleplugin.AppLabelValue)))), @@ -226,9 +288,30 @@ func (c *PolicyMachineryController) Controller(manager ctrlruntime.Manager, clie ) } - controllerOpts = append(controllerOpts, controller.WithReconcile(c.buildReconciler(manager, client))) + return opts +} - return controller.NewController(controllerOpts...) +func (b *BootOptionsBuilder) Reconciler() controller.ReconcileFunc { + mainWorkflow := &controller.Workflow{ + Precondition: initWorkflow(b.client).Run, + Tasks: []controller.ReconcileFunc{ + NewAuthorinoReconciler(b.client).Subscription().Reconcile, + NewLimitadorReconciler(b.client).Subscription().Reconcile, + NewDNSWorkflow().Run, + NewTLSWorkflow(b.client, b.isCertManagerInstalled).Run, + NewAuthWorkflow().Run, + NewRateLimitWorkflow().Run, + }, + Postcondition: finalStepsWorkflow(b.client, b.isIstioInstalled, b.isGatewayAPIInstalled).Run, + } + + if b.isConsolePluginInstalled { + mainWorkflow.Tasks = append(mainWorkflow.Tasks, + NewConsolePluginReconciler(b.manager, operatorNamespace).Subscription().Reconcile, + ) + } + + return mainWorkflow.Run } func certManagerControllerOpts() []controller.ControllerOption { @@ -293,29 +376,6 @@ func certManagerControllerOpts() []controller.ControllerOption { } } -func (c *PolicyMachineryController) buildReconciler(manager ctrlruntime.Manager, client *dynamic.DynamicClient) controller.ReconcileFunc { - mainWorkflow := &controller.Workflow{ - Precondition: initWorkflow(client).Run, - Tasks: []controller.ReconcileFunc{ - NewAuthorinoReconciler(client).Subscription().Reconcile, - NewLimitadorReconciler(client).Subscription().Reconcile, - NewDNSWorkflow().Run, - NewTLSWorkflow(client, c.isCertManagerInstalled).Run, - NewAuthWorkflow().Run, - NewRateLimitWorkflow().Run, - }, - Postcondition: finalStepsWorkflow(client, c.isIstioInstalled, c.isGatewayAPIInstalled).Run, - } - - if c.isConsolePluginInstalled { - mainWorkflow.Tasks = append(mainWorkflow.Tasks, - NewConsolePluginReconciler(manager, operatorNamespace).Subscription().Reconcile, - ) - } - - return mainWorkflow.Run -} - func initWorkflow(client *dynamic.DynamicClient) *controller.Workflow { return &controller.Workflow{ Precondition: NewEventLogger().Log, diff --git a/controllers/test_common.go b/controllers/test_common.go index cb1357fd9..43d355ab9 100644 --- a/controllers/test_common.go +++ b/controllers/test_common.go @@ -262,7 +262,7 @@ func SetupKuadrantOperatorForTest(s *runtime.Scheme, cfg *rest.Config) { dClient, err := dynamic.NewForConfig(mgr.GetConfig()) Expect(err).NotTo(HaveOccurred()) - stateOfTheWorld := (&PolicyMachineryController{}).Controller(mgr, dClient, log.Log) + stateOfTheWorld := NewPolicyMachineryController(mgr, dClient, log.Log) go func() { defer GinkgoRecover() diff --git a/main.go b/main.go index f9e193b60..e9aa559e4 100644 --- a/main.go +++ b/main.go @@ -371,7 +371,7 @@ func main() { os.Exit(1) } - stateOfTheWorld := (&controllers.PolicyMachineryController{}).Controller(mgr, client, log.Log) + stateOfTheWorld := controllers.NewPolicyMachineryController(mgr, client, log.Log) if err = stateOfTheWorld.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "unable to start stateOfTheWorld controller") os.Exit(1) From 1093827f50caddb0dadb3fae4a4bd04b0adc5fb5 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 9 Oct 2024 12:11:44 +0100 Subject: [PATCH 15/16] fixup: address comments Signed-off-by: KevFan --- controllers/tlspolicy_certmanager_certificates.go | 6 +++++- controllers/tlspolicy_status_updater.go | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/controllers/tlspolicy_certmanager_certificates.go b/controllers/tlspolicy_certmanager_certificates.go index 2e6dd3fc1..f242a7cea 100644 --- a/controllers/tlspolicy_certmanager_certificates.go +++ b/controllers/tlspolicy_certmanager_certificates.go @@ -150,7 +150,11 @@ func expectedCertificatesForListener(l *machinery.Listener, tlsPolicy *v1alpha1. } // Gateway API hostname explicitly disallows IP addresses, so this // should be OK. - tlsHosts[secretRef] = append(tlsHosts[secretRef], string(*l.Hostname)) + hostname := "*" + if l.Hostname != nil { + hostname = string(*l.Hostname) + } + 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 cb90c36ce..98c1c6d1d 100644 --- a/controllers/tlspolicy_status_updater.go +++ b/controllers/tlspolicy_status_updater.go @@ -64,7 +64,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) UpdateStatus(ctx context.Context, _ [ for _, policy := range policies { if policy.DeletionTimestamp != nil { - logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.Name, "namespace", policy.Namespace) + logger.V(1).Info("tls policy is marked for deletion, skipping", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) continue } @@ -103,7 +103,7 @@ 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.GetUID(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) + logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", policy.GetUID()) } } @@ -206,7 +206,7 @@ func (t *TLSPolicyStatusUpdaterReconciler) isCertificatesReady(p machinery.Polic } for i, l := range listeners { - // Not valid - so no need to check if cert is ready since it should not be one created + // 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 From 1a391b87495e20323571252d836add562a04dc91 Mon Sep 17 00:00:00 2001 From: KevFan Date: Wed, 9 Oct 2024 13:41:43 +0100 Subject: [PATCH 16/16] fixup: move getting hostname to outside loop Signed-off-by: KevFan --- controllers/tlspolicy_certmanager_certificates.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/controllers/tlspolicy_certmanager_certificates.go b/controllers/tlspolicy_certmanager_certificates.go index f242a7cea..fcce87f80 100644 --- a/controllers/tlspolicy_certmanager_certificates.go +++ b/controllers/tlspolicy_certmanager_certificates.go @@ -139,6 +139,11 @@ func expectedCertificatesForGateway(ctx context.Context, gateway *gatewayapiv1.G 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), @@ -150,10 +155,6 @@ func expectedCertificatesForListener(l *machinery.Listener, tlsPolicy *v1alpha1. } // Gateway API hostname explicitly disallows IP addresses, so this // should be OK. - hostname := "*" - if l.Hostname != nil { - hostname = string(*l.Hostname) - } tlsHosts[secretRef] = append(tlsHosts[secretRef], hostname) }