From e2593b365e23ee74160f10cb69ff8bf5de56bf8c Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 29 Nov 2024 12:07:20 +0100 Subject: [PATCH 01/12] fix(gatewaysecret/handler.go): Handle Existing Root Certificates --- cmd/main.go | 4 +- pkg/gatewaysecret/handler.go | 93 ++++++++++++++++++++----------- pkg/gatewaysecret/handler_test.go | 4 +- 3 files changed, 64 insertions(+), 37 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index b2e0dfd446..a1fd3062f5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -214,9 +214,9 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma func setupIstioGatewaySecretRotation(config *rest.Config, kcpClient *remote.ConfigAndClient, setupLog logr.Logger) { kcpClientset := kubernetes.NewForConfigOrDie(config) - gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient) + gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient, kcpClientset, setupLog) - gatewaySecretHandler.StartRootCertificateWatch(kcpClientset, setupLog) + gatewaySecretHandler.StartRootCertificateWatch() } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 2eac5c02aa..d23e6c231d 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -28,46 +28,52 @@ const ( var errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") -type GatewaySecretHandler struct { - kcpClient client.Client +type Handler struct { + kcpClient client.Client + kcpClientset *kubernetes.Clientset + log logr.Logger } -func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { - return &GatewaySecretHandler{ - kcpClient: kcpClient, +func NewGatewaySecretHandler(kcpClient client.Client, kcpClientset *kubernetes.Clientset, + log logr.Logger, +) *Handler { + return &Handler{ + kcpClient: kcpClient, + kcpClientset: kcpClientset, + log: log, } } -func (gsh *GatewaySecretHandler) manageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { - gwSecret, err := gsh.FindGatewaySecret(ctx) +func (h *Handler) manageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { + gwSecret, err := h.FindGatewaySecret(ctx) if util.IsNotFound(err) { - return gsh.handleNonExisting(ctx, rootSecret) + return h.handleNonExisting(ctx, rootSecret) } if err != nil { return err } - return gsh.handleExisting(ctx, rootSecret, gwSecret) + return h.handleExisting(ctx, rootSecret, gwSecret) } -func (gsh *GatewaySecretHandler) handleNonExisting(ctx context.Context, rootSecret *apicorev1.Secret) error { +func (h *Handler) handleNonExisting(ctx context.Context, rootSecret *apicorev1.Secret) error { gwSecret := NewGatewaySecret(rootSecret) - return gsh.Create(ctx, gwSecret) + return h.Create(ctx, gwSecret) } -func (gsh *GatewaySecretHandler) handleExisting(ctx context.Context, +func (h *Handler) handleExisting(ctx context.Context, rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret, ) error { - caCert, err := gsh.GetRootCACertificate(ctx) + caCert, err := h.GetRootCACertificate(ctx) if err != nil { return err } - if !GatewaySecretRequiresUpdate(gwSecret, caCert) { + if !RequiresUpdate(gwSecret, caCert) { return nil } CopyRootSecretDataIntoGatewaySecret(gwSecret, rootSecret) - return gsh.Update(ctx, gwSecret) + return h.Update(ctx, gwSecret) } func CopyRootSecretDataIntoGatewaySecret(gwSecret *apicorev1.Secret, rootSecret *apicorev1.Secret) { @@ -76,7 +82,7 @@ func CopyRootSecretDataIntoGatewaySecret(gwSecret *apicorev1.Secret, rootSecret gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] } -func GatewaySecretRequiresUpdate(gwSecret *apicorev1.Secret, caCert certmanagerv1.Certificate) bool { +func RequiresUpdate(gwSecret *apicorev1.Secret, caCert certmanagerv1.Certificate) bool { if gwSecretLastModifiedAt, err := GetValidLastModifiedAt(gwSecret); err == nil { if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { return false @@ -94,29 +100,29 @@ func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { return time.Time{}, errCouldNotGetLastModifiedAt } -func (gsh *GatewaySecretHandler) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { - return GetGatewaySecret(ctx, gsh.kcpClient) +func (h *Handler) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { + return GetGatewaySecret(ctx, h.kcpClient) } -func (gsh *GatewaySecretHandler) Create(ctx context.Context, secret *apicorev1.Secret) error { - gsh.updateLastModifiedAt(secret) - if err := gsh.kcpClient.Create(ctx, secret); err != nil { +func (h *Handler) Create(ctx context.Context, secret *apicorev1.Secret) error { + h.updateLastModifiedAt(secret) + if err := h.kcpClient.Create(ctx, secret); err != nil { return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) } return nil } -func (gsh *GatewaySecretHandler) Update(ctx context.Context, secret *apicorev1.Secret) error { - gsh.updateLastModifiedAt(secret) - if err := gsh.kcpClient.Update(ctx, secret); err != nil { +func (h *Handler) Update(ctx context.Context, secret *apicorev1.Secret) error { + h.updateLastModifiedAt(secret) + if err := h.kcpClient.Update(ctx, secret); err != nil { return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) } return nil } -func (gsh *GatewaySecretHandler) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { +func (h *Handler) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { caCert := certmanagerv1.Certificate{} - if err := gsh.kcpClient.Get(ctx, + if err := h.kcpClient.Get(ctx, client.ObjectKey{Namespace: istioNamespace, Name: kcpCACertName}, &caCert); err != nil { return certmanagerv1.Certificate{}, fmt.Errorf("failed to get CA certificate: %w", err) @@ -124,7 +130,7 @@ func (gsh *GatewaySecretHandler) GetRootCACertificate(ctx context.Context) (cert return caCert, nil } -func (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) { +func (h *Handler) updateLastModifiedAt(secret *apicorev1.Secret) { if secret.Annotations == nil { secret.Annotations = make(map[string]string) } @@ -161,21 +167,42 @@ func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secre return secret, nil } -func (gsh *GatewaySecretHandler) StartRootCertificateWatch(clientset *kubernetes.Clientset, - log logr.Logger, -) { +func (h *Handler) StartRootCertificateWatch() { ctx, cancel := context.WithCancel(context.TODO()) defer cancel() - secretWatch, err := clientset.CoreV1().Secrets(istioNamespace).Watch(ctx, apimetav1.ListOptions{ + h.handleAlreadyCreatedRootCertificate(ctx) + h.handleNewRootCertificates(ctx) +} + +func (h *Handler) handleAlreadyCreatedRootCertificate(ctx context.Context) { + rootCASecrets, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).List(ctx, apimetav1.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), + }) + if err != nil { + h.log.Error(err, "unable to list root certificate") + panic(err) + } + if len(rootCASecrets.Items) != 1 { + panic(fmt.Errorf("expected exactly one root CA secret, found %d", len(rootCASecrets.Items))) + } + rootCASecret := &rootCASecrets.Items[0] + err = h.manageGatewaySecret(ctx, rootCASecret) + if err != nil { + panic(err) + } +} + +func (h *Handler) handleNewRootCertificates(ctx context.Context) { + secretWatch, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Watch(ctx, apimetav1.ListOptions{ FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), }) if err != nil { - log.Error(err, "unable to start watching root certificate") + h.log.Error(err, "unable to start watching root certificate") panic(err) } - WatchEvents(ctx, secretWatch.ResultChan(), gsh.manageGatewaySecret, log) + WatchEvents(ctx, secretWatch.ResultChan(), h.manageGatewaySecret, h.log) } func WatchEvents(ctx context.Context, watchEvents <-chan watch.Event, diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 661ea3950b..34c2306a2f 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -206,9 +206,9 @@ func TestGatewaySecretRequiresUpdate(t *testing.T) { } for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { - if got := gatewaysecret.GatewaySecretRequiresUpdate( + if got := gatewaysecret.RequiresUpdate( testcase.args.gwSecret, testcase.args.caCert); got != testcase.want { - t.Errorf("GatewaySecretRequiresUpdate() = %v, want %v", got, testcase.want) + t.Errorf("RequiresUpdate() = %v, want %v", got, testcase.want) } }) } From 7410f28a0bd2b9b5b1937d114f3332113cf8e6d0 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 29 Nov 2024 12:20:04 +0100 Subject: [PATCH 02/12] chore: Update test coverage --- unit-test-coverage.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 1e9cb284d5..d42d42593a 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -17,4 +17,4 @@ packages: internal/remote: 13.2 internal/util/collections: 86 pkg/templatelookup: 77.1 - pkg/gatewaysecret: 27.7 + pkg/gatewaysecret: 23.4 From 12bcc106c477e2fe35e42e0c32415b934828337f Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 29 Nov 2024 12:42:56 +0100 Subject: [PATCH 03/12] refactor: Removing dynamic error for static error --- pkg/gatewaysecret/handler.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index d23e6c231d..d746895bb6 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -26,7 +26,10 @@ const ( istioNamespace = "istio-system" ) -var errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") +var ( + errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") + errExpectedOneRootCASecret = errors.New("expected exactly one root CA secret") +) type Handler struct { kcpClient client.Client @@ -184,7 +187,9 @@ func (h *Handler) handleAlreadyCreatedRootCertificate(ctx context.Context) { panic(err) } if len(rootCASecrets.Items) != 1 { - panic(fmt.Errorf("expected exactly one root CA secret, found %d", len(rootCASecrets.Items))) + h.log.Error(errExpectedOneRootCASecret, errExpectedOneRootCASecret.Error(), + "found", len(rootCASecrets.Items)) + panic(fmt.Errorf("%w: found %d", errExpectedOneRootCASecret, len(rootCASecrets.Items))) } rootCASecret := &rootCASecrets.Items[0] err = h.manageGatewaySecret(ctx, rootCASecret) From e7634673c6a7ded755143a38b6aa34370a355f14 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 29 Nov 2024 12:47:10 +0100 Subject: [PATCH 04/12] chore: Update Unit Test Coverage --- unit-test-coverage.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index d42d42593a..75d1d6a979 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -17,4 +17,4 @@ packages: internal/remote: 13.2 internal/util/collections: 86 pkg/templatelookup: 77.1 - pkg/gatewaysecret: 23.4 + pkg/gatewaysecret: 23.1 From 0171bb78adf78cbec2e8b772c650f59651612207 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 29 Nov 2024 12:52:41 +0100 Subject: [PATCH 05/12] fix: Goroutine Race Condition --- cmd/main.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index a1fd3062f5..6eaf10041b 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -38,7 +38,6 @@ import ( machineryutilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes" k8sclientscheme "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/rest" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -204,7 +203,8 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) - go setupIstioGatewaySecretRotation(config, kcpClient, setupLog) + go gatewaysecret.NewGatewaySecretHandler(kcpClient, kubernetes.NewForConfigOrDie(config), setupLog). + StartRootCertificateWatch() if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") @@ -212,13 +212,6 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma } } -func setupIstioGatewaySecretRotation(config *rest.Config, kcpClient *remote.ConfigAndClient, setupLog logr.Logger) { - kcpClientset := kubernetes.NewForConfigOrDie(config) - gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient, kcpClientset, setupLog) - - gatewaySecretHandler.StartRootCertificateWatch() -} - func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { From 0e99160ce775d257b6bf3c0c85d3384d849b61e2 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 2 Dec 2024 12:21:05 +0100 Subject: [PATCH 06/12] fix: Panic Handling --- cmd/main.go | 10 +- .../watcher_certmanager_role.yaml | 1 + pkg/gatewaysecret/certmanagerclient.go | 32 ++++++ pkg/gatewaysecret/handler.go | 99 ++++++++----------- pkg/gatewaysecret/handler_test.go | 2 +- 5 files changed, 82 insertions(+), 62 deletions(-) create mode 100644 pkg/gatewaysecret/certmanagerclient.go diff --git a/cmd/main.go b/cmd/main.go index 6eaf10041b..47d6a09715 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -21,6 +21,7 @@ import ( "errors" "flag" "fmt" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "net/http" "net/http/pprof" "os" @@ -36,7 +37,6 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" machineryruntime "k8s.io/apimachinery/pkg/runtime" machineryutilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/client-go/kubernetes" k8sclientscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" @@ -62,7 +62,6 @@ import ( "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/remote" - "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/matcher" "github.com/kyma-project/lifecycle-manager/pkg/queue" @@ -203,8 +202,11 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) - go gatewaysecret.NewGatewaySecretHandler(kcpClient, kubernetes.NewForConfigOrDie(config), setupLog). - StartRootCertificateWatch() + if err := gatewaysecret.NewGatewaySecretHandler(config, setupLog). + StartRootCertificateWatch(); err != nil { + setupLog.Error(err, "unable to start root certificate watch") + os.Exit(bootstrapFailedExitCode) + } if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") diff --git a/config/rbac/namespace_bindings/watcher_certmanager_role.yaml b/config/rbac/namespace_bindings/watcher_certmanager_role.yaml index e6bc636d36..05b512e6a5 100644 --- a/config/rbac/namespace_bindings/watcher_certmanager_role.yaml +++ b/config/rbac/namespace_bindings/watcher_certmanager_role.yaml @@ -13,6 +13,7 @@ rules: verbs: - list - watch + - get - create - delete - update diff --git a/pkg/gatewaysecret/certmanagerclient.go b/pkg/gatewaysecret/certmanagerclient.go new file mode 100644 index 0000000000..aef919ee48 --- /dev/null +++ b/pkg/gatewaysecret/certmanagerclient.go @@ -0,0 +1,32 @@ +package gatewaysecret + +import ( + "context" + "k8s.io/client-go/rest" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + certmanagerclientset "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned" + apicorev1 "k8s.io/api/core/v1" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +type CertManagerClient struct { + clientset certmanagerclientset.Clientset +} + +func NewCertManagerClient(config *rest.Config) *CertManagerClient { + return &CertManagerClient{clientset: *certmanagerclientset.NewForConfigOrDie(config)} +} + +func (h *CertManagerClient) GetRootCACertificate(ctx context.Context) (*certmanagerv1.Certificate, error) { + return h.clientset.CertmanagerV1().Certificates(istioNamespace).Get(ctx, kcpCACertName, apimetav1.GetOptions{}) +} + +func RequiresUpdate(gwSecret *apicorev1.Secret, caCert *certmanagerv1.Certificate) bool { + if gwSecretLastModifiedAt, err := GetValidLastModifiedAt(gwSecret); err == nil { + if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { + return false + } + } + return true +} diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index d746895bb6..59b18281b8 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -4,9 +4,9 @@ import ( "context" "errors" "fmt" + "k8s.io/client-go/rest" "time" - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,27 +28,26 @@ const ( var ( errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") - errExpectedOneRootCASecret = errors.New("expected exactly one root CA secret") ) type Handler struct { - kcpClient client.Client - kcpClientset *kubernetes.Clientset - log logr.Logger + certManagerClient *CertManagerClient + kcpClientset *kubernetes.Clientset + log logr.Logger } -func NewGatewaySecretHandler(kcpClient client.Client, kcpClientset *kubernetes.Clientset, +func NewGatewaySecretHandler(config *rest.Config, log logr.Logger, ) *Handler { return &Handler{ - kcpClient: kcpClient, - kcpClientset: kcpClientset, - log: log, + certManagerClient: NewCertManagerClient(config), + kcpClientset: kubernetes.NewForConfigOrDie(config), + log: log, } } func (h *Handler) manageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { - gwSecret, err := h.FindGatewaySecret(ctx) + gwSecret, err := h.findGatewaySecret(ctx) if util.IsNotFound(err) { return h.handleNonExisting(ctx, rootSecret) @@ -68,7 +67,7 @@ func (h *Handler) handleNonExisting(ctx context.Context, rootSecret *apicorev1.S func (h *Handler) handleExisting(ctx context.Context, rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret, ) error { - caCert, err := h.GetRootCACertificate(ctx) + caCert, err := h.certManagerClient.GetRootCACertificate(ctx) if err != nil { return err } @@ -85,15 +84,6 @@ func CopyRootSecretDataIntoGatewaySecret(gwSecret *apicorev1.Secret, rootSecret gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] } -func RequiresUpdate(gwSecret *apicorev1.Secret, caCert certmanagerv1.Certificate) bool { - if gwSecretLastModifiedAt, err := GetValidLastModifiedAt(gwSecret); err == nil { - if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { - return false - } - } - return true -} - func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { if gwSecretLastModifiedAtValue, ok := secret.Annotations[LastModifiedAtAnnotation]; ok { if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { @@ -103,13 +93,19 @@ func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { return time.Time{}, errCouldNotGetLastModifiedAt } -func (h *Handler) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { - return GetGatewaySecret(ctx, h.kcpClient) +func (h *Handler) findGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + var err error + if secret, err = h.kcpClientset.CoreV1().Secrets(istioNamespace).Get(ctx, gatewaySecretName, + apimetav1.GetOptions{}); err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) + } + return secret, nil } func (h *Handler) Create(ctx context.Context, secret *apicorev1.Secret) error { h.updateLastModifiedAt(secret) - if err := h.kcpClient.Create(ctx, secret); err != nil { + if _, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Create(ctx, secret, apimetav1.CreateOptions{}); err != nil { return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) } return nil @@ -117,22 +113,12 @@ func (h *Handler) Create(ctx context.Context, secret *apicorev1.Secret) error { func (h *Handler) Update(ctx context.Context, secret *apicorev1.Secret) error { h.updateLastModifiedAt(secret) - if err := h.kcpClient.Update(ctx, secret); err != nil { + if _, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Update(ctx, secret, apimetav1.UpdateOptions{}); err != nil { return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) } return nil } -func (h *Handler) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { - caCert := certmanagerv1.Certificate{} - if err := h.kcpClient.Get(ctx, - client.ObjectKey{Namespace: istioNamespace, Name: kcpCACertName}, - &caCert); err != nil { - return certmanagerv1.Certificate{}, fmt.Errorf("failed to get CA certificate: %w", err) - } - return caCert, nil -} - func (h *Handler) updateLastModifiedAt(secret *apicorev1.Secret) { if secret.Annotations == nil { secret.Annotations = make(map[string]string) @@ -170,44 +156,43 @@ func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secre return secret, nil } -func (h *Handler) StartRootCertificateWatch() { - ctx, cancel := context.WithCancel(context.TODO()) +func (h *Handler) StartRootCertificateWatch() error { + ctx, cancel := context.WithCancel(context.Background()) defer cancel() - h.handleAlreadyCreatedRootCertificate(ctx) - h.handleNewRootCertificates(ctx) -} - -func (h *Handler) handleAlreadyCreatedRootCertificate(ctx context.Context) { - rootCASecrets, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).List(ctx, apimetav1.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), - }) - if err != nil { - h.log.Error(err, "unable to list root certificate") - panic(err) + if err := h.handleAlreadyCreatedRootCertificate(ctx); err != nil { + return err } - if len(rootCASecrets.Items) != 1 { - h.log.Error(errExpectedOneRootCASecret, errExpectedOneRootCASecret.Error(), - "found", len(rootCASecrets.Items)) - panic(fmt.Errorf("%w: found %d", errExpectedOneRootCASecret, len(rootCASecrets.Items))) + if err := h.handleNewRootCertificates(ctx); err != nil { + return err } - rootCASecret := &rootCASecrets.Items[0] - err = h.manageGatewaySecret(ctx, rootCASecret) + return nil +} + +func (h *Handler) handleAlreadyCreatedRootCertificate(ctx context.Context) error { + rootCASecret, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Get(ctx, kcpRootSecretName, + apimetav1.GetOptions{}) if err != nil { - panic(err) + if util.IsNotFound(err) { + return nil + } + h.log.Error(err, "unable to get root certificate") + return fmt.Errorf("unable to get root certificate: %w", err) } + return h.manageGatewaySecret(ctx, rootCASecret) } -func (h *Handler) handleNewRootCertificates(ctx context.Context) { +func (h *Handler) handleNewRootCertificates(ctx context.Context) error { secretWatch, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Watch(ctx, apimetav1.ListOptions{ FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), }) if err != nil { h.log.Error(err, "unable to start watching root certificate") - panic(err) + return fmt.Errorf("unable to start watching root certificate: %w", err) } - WatchEvents(ctx, secretWatch.ResultChan(), h.manageGatewaySecret, h.log) + go WatchEvents(ctx, secretWatch.ResultChan(), h.manageGatewaySecret, h.log) + return nil } func WatchEvents(ctx context.Context, watchEvents <-chan watch.Event, diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 34c2306a2f..4ad40a5d24 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -207,7 +207,7 @@ func TestGatewaySecretRequiresUpdate(t *testing.T) { for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { if got := gatewaysecret.RequiresUpdate( - testcase.args.gwSecret, testcase.args.caCert); got != testcase.want { + testcase.args.gwSecret, &testcase.args.caCert); got != testcase.want { t.Errorf("RequiresUpdate() = %v, want %v", got, testcase.want) } }) From d79dda2a9c65b981ed5cf8effc62f5b5708f373e Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 2 Dec 2024 12:42:12 +0100 Subject: [PATCH 07/12] fix: Content Cancel --- pkg/gatewaysecret/handler.go | 13 ++++++++----- pkg/gatewaysecret/handler_test.go | 3 +-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 59b18281b8..f23f788fa7 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -158,12 +158,13 @@ func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secre func (h *Handler) StartRootCertificateWatch() error { ctx, cancel := context.WithCancel(context.Background()) - defer cancel() if err := h.handleAlreadyCreatedRootCertificate(ctx); err != nil { + cancel() return err } - if err := h.handleNewRootCertificates(ctx); err != nil { + if err := h.handleNewRootCertificates(ctx, cancel); err != nil { + cancel() return err } return nil @@ -182,7 +183,7 @@ func (h *Handler) handleAlreadyCreatedRootCertificate(ctx context.Context) error return h.manageGatewaySecret(ctx, rootCASecret) } -func (h *Handler) handleNewRootCertificates(ctx context.Context) error { +func (h *Handler) handleNewRootCertificates(ctx context.Context, cancel context.CancelFunc) error { secretWatch, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Watch(ctx, apimetav1.ListOptions{ FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), }) @@ -191,13 +192,15 @@ func (h *Handler) handleNewRootCertificates(ctx context.Context) error { return fmt.Errorf("unable to start watching root certificate: %w", err) } - go WatchEvents(ctx, secretWatch.ResultChan(), h.manageGatewaySecret, h.log) + go WatchEvents(ctx, cancel, secretWatch.ResultChan(), h.manageGatewaySecret, h.log) return nil } -func WatchEvents(ctx context.Context, watchEvents <-chan watch.Event, +func WatchEvents(ctx context.Context, cancel context.CancelFunc, watchEvents <-chan watch.Event, manageGatewaySecretFunc func(context.Context, *apicorev1.Secret) error, log logr.Logger, ) { + defer cancel() + for event := range watchEvents { rootCASecret, _ := event.Object.(*apicorev1.Secret) diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 4ad40a5d24..83d37f7f53 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -244,7 +244,6 @@ func TestWatchEvents(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.TODO()) - defer cancel() var waitGroup sync.WaitGroup waitGroup.Add(1) @@ -258,7 +257,7 @@ func TestWatchEvents(t *testing.T) { events := make(chan watch.Event, 1) go func() { defer waitGroup.Done() - gatewaysecret.WatchEvents(ctx, events, mockManageSecretFunc, logr.Logger{}) + gatewaysecret.WatchEvents(ctx, cancel, events, mockManageSecretFunc, logr.Logger{}) }() events <- watch.Event{ From 795fe8e267e0129729692dd0e52299b302c9e7f2 Mon Sep 17 00:00:00 2001 From: Xin Ruan Date: Mon, 2 Dec 2024 14:15:25 +0100 Subject: [PATCH 08/12] formatting --- cmd/main.go | 3 ++- pkg/gatewaysecret/certmanagerclient.go | 1 + pkg/gatewaysecret/handler.go | 7 +++---- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 47d6a09715..f762996437 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -21,13 +21,14 @@ import ( "errors" "flag" "fmt" - "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "net/http" "net/http/pprof" "os" "strings" "time" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-co-op/gocron" "github.com/go-logr/logr" diff --git a/pkg/gatewaysecret/certmanagerclient.go b/pkg/gatewaysecret/certmanagerclient.go index aef919ee48..4909f54a0b 100644 --- a/pkg/gatewaysecret/certmanagerclient.go +++ b/pkg/gatewaysecret/certmanagerclient.go @@ -2,6 +2,7 @@ package gatewaysecret import ( "context" + "k8s.io/client-go/rest" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index f23f788fa7..f782884fad 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -4,9 +4,10 @@ import ( "context" "errors" "fmt" - "k8s.io/client-go/rest" "time" + "k8s.io/client-go/rest" + "github.com/go-logr/logr" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,9 +27,7 @@ const ( istioNamespace = "istio-system" ) -var ( - errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") -) +var errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") type Handler struct { certManagerClient *CertManagerClient From a26ae04b155621edf37430034e6cb8d549bb564a Mon Sep 17 00:00:00 2001 From: Xin Ruan Date: Mon, 2 Dec 2024 14:56:46 +0100 Subject: [PATCH 09/12] fix lint --- .golangci.yaml | 2 ++ cmd/main.go | 18 +++++++++++------- pkg/gatewaysecret/certmanagerclient.go | 11 ++++++++--- pkg/gatewaysecret/handler.go | 16 ++++++++-------- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 6e3a796b0c..f3afd53d92 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -54,6 +54,8 @@ linters-settings: no-unaliased: true no-extra-aliases: true alias: + - pkg: github.com/cert-manager/cert-manager/pkg/client/clientset/versioned + alias: certmanagerclientset - pkg: github.com/kyma-project/template-operator/api/v1alpha1 alias: templatev1alpha1 - pkg: github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 diff --git a/cmd/main.go b/cmd/main.go index f762996437..e805077093 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -27,8 +27,6 @@ import ( "strings" "time" - "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-co-op/gocron" "github.com/go-logr/logr" @@ -39,6 +37,7 @@ import ( machineryruntime "k8s.io/apimachinery/pkg/runtime" machineryutilruntime "k8s.io/apimachinery/pkg/util/runtime" k8sclientscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -63,6 +62,7 @@ import ( "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/remote" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/matcher" "github.com/kyma-project/lifecycle-manager/pkg/queue" @@ -203,11 +203,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) - if err := gatewaysecret.NewGatewaySecretHandler(config, setupLog). - StartRootCertificateWatch(); err != nil { - setupLog.Error(err, "unable to start root certificate watch") - os.Exit(bootstrapFailedExitCode) - } + startCAWatch(config, setupLog) if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") @@ -215,6 +211,14 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma } } +func startCAWatch(config *rest.Config, setupLog logr.Logger) { + if err := gatewaysecret.NewGatewaySecretHandler(config, setupLog). + StartRootCertificateWatch(); err != nil { + setupLog.Error(err, "unable to start root certificate watch") + os.Exit(bootstrapFailedExitCode) + } +} + func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/pkg/gatewaysecret/certmanagerclient.go b/pkg/gatewaysecret/certmanagerclient.go index 4909f54a0b..6313066985 100644 --- a/pkg/gatewaysecret/certmanagerclient.go +++ b/pkg/gatewaysecret/certmanagerclient.go @@ -2,13 +2,13 @@ package gatewaysecret import ( "context" - - "k8s.io/client-go/rest" + "fmt" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" certmanagerclientset "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" ) type CertManagerClient struct { @@ -20,7 +20,12 @@ func NewCertManagerClient(config *rest.Config) *CertManagerClient { } func (h *CertManagerClient) GetRootCACertificate(ctx context.Context) (*certmanagerv1.Certificate, error) { - return h.clientset.CertmanagerV1().Certificates(istioNamespace).Get(ctx, kcpCACertName, apimetav1.GetOptions{}) + caCert, err := h.clientset.CertmanagerV1().Certificates(istioNamespace).Get(ctx, kcpCACertName, + apimetav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to get CA certificate: %w", err) + } + return caCert, nil } func RequiresUpdate(gwSecret *apicorev1.Secret, caCert *certmanagerv1.Certificate) bool { diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index f782884fad..b967c0ed54 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -6,14 +6,13 @@ import ( "fmt" "time" - "k8s.io/client-go/rest" - "github.com/go-logr/logr" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kyma-project/lifecycle-manager/pkg/util" @@ -93,10 +92,9 @@ func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { } func (h *Handler) findGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { - secret := &apicorev1.Secret{} - var err error - if secret, err = h.kcpClientset.CoreV1().Secrets(istioNamespace).Get(ctx, gatewaySecretName, - apimetav1.GetOptions{}); err != nil { + secret, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Get(ctx, gatewaySecretName, + apimetav1.GetOptions{}) + if err != nil { return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) } return secret, nil @@ -104,7 +102,8 @@ func (h *Handler) findGatewaySecret(ctx context.Context) (*apicorev1.Secret, err func (h *Handler) Create(ctx context.Context, secret *apicorev1.Secret) error { h.updateLastModifiedAt(secret) - if _, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Create(ctx, secret, apimetav1.CreateOptions{}); err != nil { + if _, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Create(ctx, secret, + apimetav1.CreateOptions{}); err != nil { return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) } return nil @@ -112,7 +111,8 @@ func (h *Handler) Create(ctx context.Context, secret *apicorev1.Secret) error { func (h *Handler) Update(ctx context.Context, secret *apicorev1.Secret) error { h.updateLastModifiedAt(secret) - if _, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Update(ctx, secret, apimetav1.UpdateOptions{}); err != nil { + if _, err := h.kcpClientset.CoreV1().Secrets(istioNamespace).Update(ctx, secret, + apimetav1.UpdateOptions{}); err != nil { return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) } return nil From 3909e33854f32e228e37246434dfc544e41b08fc Mon Sep 17 00:00:00 2001 From: Xin Ruan Date: Mon, 2 Dec 2024 15:18:35 +0100 Subject: [PATCH 10/12] fix permission --- tests/e2e/rbac_privileges_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/rbac_privileges_test.go b/tests/e2e/rbac_privileges_test.go index 2184178ad1..4931f7531e 100644 --- a/tests/e2e/rbac_privileges_test.go +++ b/tests/e2e/rbac_privileges_test.go @@ -178,12 +178,12 @@ var _ = Describe("RBAC Privileges", func() { { APIGroups: []string{""}, Resources: []string{"secrets"}, - Verbs: []string{"list", "watch", "create", "delete", "update"}, + Verbs: []string{"list", "watch", "create", "delete", "update", "get"}, }, { APIGroups: []string{"cert-manager.io"}, Resources: []string{"certificates"}, - Verbs: []string{"patch", "list", "watch", "get", "create", "delete"}, + Verbs: []string{"patch", "list", "watch", "get", "create", "delete", "get"}, }, { APIGroups: []string{"cert-manager.io"}, From a9b01185a2866802ae3a1fd747280ac1802d143a Mon Sep 17 00:00:00 2001 From: Xin Ruan Date: Mon, 2 Dec 2024 15:33:45 +0100 Subject: [PATCH 11/12] reset coverage --- unit-test-coverage.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 75d1d6a979..3e2973feb9 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -17,4 +17,4 @@ packages: internal/remote: 13.2 internal/util/collections: 86 pkg/templatelookup: 77.1 - pkg/gatewaysecret: 23.1 + pkg/gatewaysecret: 22.6 From 4ca37354d4d15e704a5bfd772f51c117556fe024 Mon Sep 17 00:00:00 2001 From: Xin Ruan Date: Mon, 2 Dec 2024 15:59:47 +0100 Subject: [PATCH 12/12] fix rbac --- tests/e2e/rbac_privileges_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/rbac_privileges_test.go b/tests/e2e/rbac_privileges_test.go index 4931f7531e..0eb7b51b31 100644 --- a/tests/e2e/rbac_privileges_test.go +++ b/tests/e2e/rbac_privileges_test.go @@ -178,12 +178,12 @@ var _ = Describe("RBAC Privileges", func() { { APIGroups: []string{""}, Resources: []string{"secrets"}, - Verbs: []string{"list", "watch", "create", "delete", "update", "get"}, + Verbs: []string{"list", "watch", "get", "create", "delete", "update"}, }, { APIGroups: []string{"cert-manager.io"}, Resources: []string{"certificates"}, - Verbs: []string{"patch", "list", "watch", "get", "create", "delete", "get"}, + Verbs: []string{"patch", "list", "watch", "get", "create", "delete"}, }, { APIGroups: []string{"cert-manager.io"},