From 47f48f45769e749178e613f7943598002f5bd7fc Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 4 Nov 2024 00:34:16 +0100 Subject: [PATCH 01/42] feat: Introduce Dedicated Istio Gateway Secret --- cmd/main.go | 38 +++++- config/manager/kustomization.yaml | 4 +- config/watcher/gateway.yaml | 2 +- internal/pkg/flags/flags.go | 4 - internal/pkg/flags/flags_test.go | 5 - pkg/watcher/ca_certificate_cache.go | 33 ----- pkg/watcher/certificate_manager.go | 44 +++---- pkg/watcher/skr_webhook_manifest_manager.go | 74 +++++++---- pkg/zerodw/gateway.go | 117 ++++++++++++++++++ pkg/zerodw/secret.go | 63 ++++++++++ .../controller/withwatcher/suite_test.go | 2 - 11 files changed, 284 insertions(+), 102 deletions(-) delete mode 100644 pkg/watcher/ca_certificate_cache.go create mode 100644 pkg/zerodw/gateway.go create mode 100644 pkg/zerodw/secret.go diff --git a/cmd/main.go b/cmd/main.go index 274f158f92..1fe50d858e 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -21,6 +21,11 @@ import ( "errors" "flag" "fmt" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/watch" "net/http" "net/http/pprof" "os" @@ -36,6 +41,7 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" machineryruntime "k8s.io/apimachinery/pkg/runtime" machineryutilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgok8s "k8s.io/client-go/kubernetes" k8sclientscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" @@ -196,6 +202,10 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma addHealthChecks(mgr, setupLog) + kcpClientset := clientgok8s.NewForConfigOrDie(config) + gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) + go watchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) @@ -203,6 +213,32 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma setupLog.Error(err, "problem running manager") os.Exit(runtimeProblemExitCode) } + +} + +func watchChangesOnRootCertificate(clientset *clientgok8s.Clientset, gatewaySecretHandler *zerodw.GatewaySecretHandler, + setupLog logr.Logger) { + secretWatch, err := clientset.CoreV1().Secrets("istio-system").Watch(context.Background(), v1.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(v1.ObjectNameField, "klm-watcher").String(), + }) + if err != nil { + setupLog.Error(err, "unable to start watching root certificate") + return + } + + for e := range secretWatch.ResultChan() { + item := e.Object.(*corev1.Secret) + + switch e.Type { + case watch.Modified: + fallthrough + case watch.Added: + err := gatewaySecretHandler.ManageGatewaySecret(item) + if err != nil { + setupLog.Error(err, "unable to manage istio gateway secret") + } + } + } } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { @@ -321,7 +357,6 @@ func setupKymaReconciler(mgr ctrl.Manager, func createSkrWebhookManager(mgr ctrl.Manager, skrContextFactory remote.SkrContextProvider, flagVar *flags.FlagVar, ) (*watcher.SKRWebhookManifestManager, error) { - caCertificateCache := watcher.NewCACertificateCache(flagVar.CaCertCacheTTL) config := watcher.SkrWebhookManagerConfig{ SKRWatcherPath: flagVar.WatcherResourcesPath, SkrWatcherImage: flagVar.GetWatcherImage(), @@ -351,7 +386,6 @@ func createSkrWebhookManager(mgr ctrl.Manager, skrContextFactory remote.SkrConte return watcher.NewSKRWebhookManifestManager( mgr.GetClient(), skrContextFactory, - caCertificateCache, config, certConfig, resolvedKcpAddr) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 00ae86ff75..a33bdd2380 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -10,5 +10,5 @@ generatorOptions: images: - name: controller - newName: europe-docker.pkg.dev/kyma-project/prod/lifecycle-manager - newTag: latest + newName: k3d-myregistry.localhost:5000/lifecycle-manager + newTag: "20241104001047" diff --git a/config/watcher/gateway.yaml b/config/watcher/gateway.yaml index 3e32c6f810..030ab60701 100644 --- a/config/watcher/gateway.yaml +++ b/config/watcher/gateway.yaml @@ -20,5 +20,5 @@ spec: number: 443 protocol: HTTPS tls: - credentialName: klm-watcher + credentialName: istio-gateway-secret mode: MUTUAL diff --git a/internal/pkg/flags/flags.go b/internal/pkg/flags/flags.go index 80036cde39..9896d38c04 100644 --- a/internal/pkg/flags/flags.go +++ b/internal/pkg/flags/flags.go @@ -43,7 +43,6 @@ const ( DefaultIstioGatewayNamespace = "kcp-system" DefaultIstioNamespace = "istio-system" DefaultCaCertName = "klm-watcher-serving" - DefaultCaCertCacheTTL time.Duration = 1 * time.Hour DefaultSelfSignedCertDuration time.Duration = 90 * 24 * time.Hour DefaultSelfSignedCertRenewBefore time.Duration = 60 * 24 * time.Hour DefaultSelfSignedCertificateRenewBuffer = 24 * time.Hour @@ -203,8 +202,6 @@ func DefineFlagVar() *FlagVar { "Name of the namespace for syncing remote Kyma and module catalog") flag.StringVar(&flagVar.CaCertName, "ca-cert-name", DefaultCaCertName, "Name of the CA Certificate in Istio Namespace which is used to sign SKR Certificates") - flag.DurationVar(&flagVar.CaCertCacheTTL, "ca-cert-cache-ttl", DefaultCaCertCacheTTL, - "The ttl for the CA Certificate Cache") flag.DurationVar(&flagVar.SelfSignedCertDuration, "self-signed-cert-duration", DefaultSelfSignedCertDuration, "The lifetime duration of self-signed certificate, minimum accepted duration is 1 hour.") flag.DurationVar(&flagVar.SelfSignedCertRenewBefore, "self-signed-cert-renew-before", @@ -287,7 +284,6 @@ type FlagVar struct { SkipPurgingFor string RemoteSyncNamespace string CaCertName string - CaCertCacheTTL time.Duration IsKymaManaged bool SelfSignedCertDuration time.Duration SelfSignedCertRenewBefore time.Duration diff --git a/internal/pkg/flags/flags_test.go b/internal/pkg/flags/flags_test.go index 9b097c3d39..fbe6aba7c4 100644 --- a/internal/pkg/flags/flags_test.go +++ b/internal/pkg/flags/flags_test.go @@ -175,11 +175,6 @@ func Test_ConstantFlags(t *testing.T) { constValue: DefaultCaCertName, expectedValue: "klm-watcher-serving", }, - { - constName: "DefaultCaCertCacheTTL", - constValue: DefaultCaCertCacheTTL.String(), - expectedValue: (1 * time.Hour).String(), - }, { constName: "DefaultSelfSignedCertDuration", constValue: DefaultSelfSignedCertDuration.String(), diff --git a/pkg/watcher/ca_certificate_cache.go b/pkg/watcher/ca_certificate_cache.go deleted file mode 100644 index e902fb769d..0000000000 --- a/pkg/watcher/ca_certificate_cache.go +++ /dev/null @@ -1,33 +0,0 @@ -package watcher - -import ( - "time" - - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - "github.com/jellydator/ttlcache/v3" -) - -type CACertificateCache struct { - TTL time.Duration - *ttlcache.Cache[string, certmanagerv1.Certificate] -} - -func NewCACertificateCache(ttl time.Duration) *CACertificateCache { - cache := ttlcache.New[string, certmanagerv1.Certificate]() - go cache.Start() - return &CACertificateCache{Cache: cache, TTL: ttl} -} - -func (c *CACertificateCache) GetCACertStatusFromCache(caCertName string) certmanagerv1.CertificateStatus { - value := c.Cache.Get(caCertName) - if value != nil { - cert := value.Value() - return cert.Status - } - - return certmanagerv1.CertificateStatus{} -} - -func (c *CACertificateCache) SetCACertToCache(cert certmanagerv1.Certificate) { - c.Cache.Set(cert.Name, cert, c.TTL) -} diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index 9982e03fbb..e137cd6bc2 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -43,7 +44,6 @@ type SubjectAltName struct { type CertificateManager struct { kcpClient client.Client - caCertCache *CACertificateCache certificateName string secretName string labelSet k8slabels.Set @@ -76,14 +76,12 @@ type CertificateSecret struct { // NewCertificateManager returns a new CertificateManager, which can be used for creating a cert-manager Certificates. func NewCertificateManager(kcpClient client.Client, kymaName string, config CertificateConfig, - caCertCache *CACertificateCache, ) *CertificateManager { return &CertificateManager{ kcpClient: kcpClient, certificateName: ResolveTLSCertName(kymaName), secretName: ResolveTLSCertName(kymaName), config: config, - caCertCache: caCertCache, labelSet: k8slabels.Set{ shared.PurposeLabel: shared.CertManager, shared.ManagedBy: shared.OperatorName, @@ -279,35 +277,21 @@ func (e *CertificateNotReadyError) Error() string { return "Certificate-Secret does not exist" } -func (c *CertificateManager) GetCACertificateStatus(ctx context.Context) (certmanagerv1.CertificateStatus, error) { - cachedCertStatus := c.caCertCache.GetCACertStatusFromCache(c.config.CACertificateName) - - if cachedCertStatus.RenewalTime == nil || certificateRenewalTimePassed(cachedCertStatus) { - caCert := certmanagerv1.Certificate{} - if err := c.kcpClient.Get(ctx, - client.ObjectKey{Namespace: c.config.IstioNamespace, Name: c.config.CACertificateName}, - &caCert); err != nil { - return certmanagerv1.CertificateStatus{}, fmt.Errorf("failed to get CA certificate %w", err) - } - c.caCertCache.SetCACertToCache(caCert) - return caCert.Status, nil - } - - return cachedCertStatus, nil -} - func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kymaObjKey client.ObjectKey) error { - caCertificateStatus, err := c.GetCACertificateStatus(ctx) + gatewaySecret, err := getCertificateSecret(ctx, c.kcpClient, client.ObjectKey{ + Namespace: c.config.IstioNamespace, + Name: zerodw.GatewaySecretName, + }) if err != nil { - return fmt.Errorf("error while fetching CA Certificate: %w", err) + return err } - certSecret, err := c.getCertificateSecret(ctx) + watcherCert, err := c.getCertificateSecret(ctx) if err != nil { return fmt.Errorf("error while fetching certificate: %w", err) } - if certSecret != nil && (certSecret.CreationTimestamp.Before(caCertificateStatus.NotBefore)) { + if watcherCert != nil && isGatewaySecretNewerThanWatcherCert(gatewaySecret, watcherCert) { logf.FromContext(ctx).V(log.DebugLevel).Info("CA Certificate was rotated, removing certificate", "kyma", kymaObjKey) if err = c.removeSecret(ctx); err != nil { @@ -318,6 +302,14 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym return nil } -func certificateRenewalTimePassed(certStatus certmanagerv1.CertificateStatus) bool { - return certStatus.RenewalTime.Before(&(apimetav1.Time{Time: time.Now()})) +func isGatewaySecretNewerThanWatcherCert(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { + if gwSecretLastModifiedAtValue, ok := gatewaySecret.Annotations[zerodw.LastModifiedAtAnnotation]; ok { + if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { + if watcherSecret.CreationTimestamp.Time.After(gwSecretLastModifiedAt) { + return false + } + } + } + + return true } diff --git a/pkg/watcher/skr_webhook_manifest_manager.go b/pkg/watcher/skr_webhook_manifest_manager.go index 06d313a2fc..73d9470ac3 100644 --- a/pkg/watcher/skr_webhook_manifest_manager.go +++ b/pkg/watcher/skr_webhook_manifest_manager.go @@ -21,17 +21,17 @@ import ( "github.com/kyma-project/lifecycle-manager/internal/util/collections" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/util" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" ) type SKRWebhookManifestManager struct { - kcpClient client.Client - skrContextFactory remote.SkrContextProvider - config SkrWebhookManagerConfig - kcpAddr string - baseResources []*unstructured.Unstructured - caCertificateCache *CACertificateCache - WatcherMetrics *metrics.WatcherMetrics - certificateConfig CertificateConfig + kcpClient client.Client + skrContextFactory remote.SkrContextProvider + config SkrWebhookManagerConfig + kcpAddr string + baseResources []*unstructured.Unstructured + WatcherMetrics *metrics.WatcherMetrics + certificateConfig CertificateConfig } type SkrWebhookManagerConfig struct { @@ -49,7 +49,6 @@ const rawManifestFilePathTpl = "%s/resources.yaml" func NewSKRWebhookManifestManager( kcpClient client.Client, skrContextFactory remote.SkrContextProvider, - caCertificateCache *CACertificateCache, managerConfig SkrWebhookManagerConfig, certificateConfig CertificateConfig, resolvedKcpAddr string, @@ -68,14 +67,13 @@ func NewSKRWebhookManifestManager( } return &SKRWebhookManifestManager{ - kcpClient: kcpClient, - skrContextFactory: skrContextFactory, - config: managerConfig, - certificateConfig: certificateConfig, - kcpAddr: resolvedKcpAddr, - baseResources: baseResources, - caCertificateCache: caCertificateCache, - WatcherMetrics: metrics.NewWatcherMetrics(), + kcpClient: kcpClient, + skrContextFactory: skrContextFactory, + config: managerConfig, + certificateConfig: certificateConfig, + kcpAddr: resolvedKcpAddr, + baseResources: baseResources, + WatcherMetrics: metrics.NewWatcherMetrics(), }, nil } @@ -87,9 +85,17 @@ func (m *SKRWebhookManifestManager) Install(ctx context.Context, kyma *v1beta2.K return fmt.Errorf("failed to get skrContext: %w", err) } + _, err = getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ + Namespace: m.certificateConfig.IstioNamespace, + Name: zerodw.GatewaySecretName, + }) + if err != nil { + return err + } + // Create CertificateCR which will be used for mTLS connection from SKR to KCP certificateMgr := NewCertificateManager(m.kcpClient, kyma.Name, - m.certificateConfig, m.caCertificateCache) + m.certificateConfig) certificate, err := certificateMgr.CreateSelfSignedCert(ctx, kyma) if err != nil { @@ -145,7 +151,7 @@ func (m *SKRWebhookManifestManager) Remove(ctx context.Context, kyma *v1beta2.Ky return fmt.Errorf("failed to get skrContext: %w", err) } certificate := NewCertificateManager(m.kcpClient, kyma.Name, - m.certificateConfig, m.caCertificateCache) + m.certificateConfig) if err = certificate.Remove(ctx); err != nil { return err } @@ -217,17 +223,20 @@ func (m *SKRWebhookManifestManager) getRawManifestClientObjects(cfg *unstructure func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.Context, kymaObjKey client.ObjectKey, remoteNs string, ) (*unstructuredResourcesConfig, error) { - tlsSecret := &apicorev1.Secret{} - certObjKey := client.ObjectKey{ + tlsSecret, err := getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ Namespace: m.certificateConfig.IstioNamespace, Name: ResolveTLSCertName(kymaObjKey.Name), + }) + if err != nil { + return nil, err } - if err := m.kcpClient.Get(ctx, certObjKey, tlsSecret); err != nil { - if util.IsNotFound(err) { - return nil, &CertificateNotReadyError{} - } - return nil, fmt.Errorf("error fetching TLS secret: %w", err) + gatewaySecret, err := getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ + Namespace: m.certificateConfig.IstioNamespace, + Name: zerodw.GatewaySecretName, + }) + if err != nil { + return nil, err } return &unstructuredResourcesConfig{ @@ -237,13 +246,24 @@ func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.C cpuResLimit: m.config.SkrWebhookCPULimits, memResLimit: m.config.SkrWebhookMemoryLimits, skrWatcherImage: m.config.SkrWatcherImage, - caCert: tlsSecret.Data[caCertKey], + caCert: gatewaySecret.Data[caCertKey], tlsCert: tlsSecret.Data[tlsCertKey], tlsKey: tlsSecret.Data[tlsPrivateKeyKey], remoteNs: remoteNs, }, nil } +func getCertificateSecret(ctx context.Context, clnt client.Client, objKey client.ObjectKey) (*apicorev1.Secret, error) { + certificateSecret := &apicorev1.Secret{} + if err := clnt.Get(ctx, objKey, certificateSecret); err != nil { + if util.IsNotFound(err) { + return nil, &CertificateNotReadyError{} + } + return nil, fmt.Errorf("error fetching TLS secret: %w", err) + } + return certificateSecret, nil +} + func (m *SKRWebhookManifestManager) getBaseClientObjects() []client.Object { if len(m.baseResources) == 0 { return nil diff --git a/pkg/zerodw/gateway.go b/pkg/zerodw/gateway.go new file mode 100644 index 0000000000..0079d95696 --- /dev/null +++ b/pkg/zerodw/gateway.go @@ -0,0 +1,117 @@ +package zerodw + +import ( + "context" + "fmt" + "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" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" +) + +const ( + GatewaySecretName = "istio-gateway-secret" + kcpRootSecretName = "klm-watcher" + kcpCACertName = "klm-watcher-serving" + istioNamespace = flags.DefaultIstioNamespace +) + +type GatewaySecretHandler struct { + *secretManager +} + +func NewGatewaySecretHandler(kcpClient client.Client, log logr.Logger) *GatewaySecretHandler { + return &GatewaySecretHandler{ + secretManager: &secretManager{ + kcpClient: kcpClient, + log: log, + }, + } +} + +func (gsh *GatewaySecretHandler) ManageGatewaySecret(rootSecret *apicorev1.Secret) error { + gwSecret, err := gsh.findGatewaySecret() + + if isNotFound(err) { + return gsh.handleNonExisting(rootSecret) + } + if err != nil { + return err + } + + return gsh.handleExisting(rootSecret, gwSecret) +} + +func (gsh *GatewaySecretHandler) handleNonExisting(rootSecret *apicorev1.Secret) error { + gwSecret := gsh.newGatewaySecret(rootSecret) + err := gsh.create(context.Background(), gwSecret) + if err == nil { + gsh.log.Info("created the gateway secret", "reason", "gateway secret does not exist") + } + return err +} + +func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret) error { + caCert := certmanagerv1.Certificate{} + if err := gsh.kcpClient.Get(context.Background(), + client.ObjectKey{Namespace: istioNamespace, Name: kcpCACertName}, + &caCert); err != nil { + return fmt.Errorf("failed to get CA certificate: %w", err) + } + + if gwSecretLastModifiedAtValue, ok := gwSecret.Annotations[LastModifiedAtAnnotation]; ok { + if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { + if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { + return nil + } + } + } + + gwSecret.Data["tls.crt"] = rootSecret.Data["tls.crt"] + gwSecret.Data["tls.key"] = rootSecret.Data["tls.key"] + gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] + err := gsh.update(context.Background(), gwSecret) + if err == nil { + gsh.log.Info("updated the gateway secret", "reason", "root ca is more recent than the gateway secret") + } + + return nil +} + +func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) { + return gsh.findSecret(context.Background(), client.ObjectKey{ + Name: GatewaySecretName, + Namespace: istioNamespace, + }) +} + +func (gsh *GatewaySecretHandler) findKcpRootSecret() (*apicorev1.Secret, error) { + return gsh.findSecret(context.Background(), client.ObjectKey{ + Name: kcpRootSecretName, + Namespace: istioNamespace, + }) +} + +func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { + gwSecret := &apicorev1.Secret{ + TypeMeta: apimetav1.TypeMeta{ + Kind: "Secret", + APIVersion: apicorev1.SchemeGroupVersion.String(), + }, + ObjectMeta: apimetav1.ObjectMeta{ + Name: GatewaySecretName, + Namespace: istioNamespace, + }, + Data: map[string][]byte{ + "tls.crt": rootSecret.Data["tls.crt"], + "tls.key": rootSecret.Data["tls.key"], + "ca.crt": rootSecret.Data["ca.crt"], + }, + } + return gwSecret +} diff --git a/pkg/zerodw/secret.go b/pkg/zerodw/secret.go new file mode 100644 index 0000000000..e4560801a0 --- /dev/null +++ b/pkg/zerodw/secret.go @@ -0,0 +1,63 @@ +package zerodw + +import ( + "context" + "fmt" + "time" + + "github.com/go-logr/logr" + apicorev1 "k8s.io/api/core/v1" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + LastModifiedAtAnnotation = "lastModifiedAt" +) + +type secretManager struct { + log logr.Logger + kcpClient client.Client +} + +func (sm *secretManager) findSecret(ctx context.Context, objKey client.ObjectKey) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + + err := sm.kcpClient.Get(ctx, objKey, secret) + if err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", objKey.Name, err) + } + + return secret, nil +} + +// isNotFound returns true if the error is a NotFound error. +func isNotFound(err error) bool { + if err == nil { + return false + } + return client.IgnoreNotFound(err) == nil +} + +func (sm *secretManager) create(ctx context.Context, secret *apicorev1.Secret) error { + sm.updateLastModifiedAt(secret) + if err := sm.kcpClient.Create(ctx, secret); err != nil { + return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) + } + return nil +} + +func (sm *secretManager) update(ctx context.Context, secret *apicorev1.Secret) error { + sm.updateLastModifiedAt(secret) + if err := sm.kcpClient.Update(ctx, secret); err != nil { + return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) + } + return nil +} + +func (sm *secretManager) updateLastModifiedAt(secret *apicorev1.Secret) { + if secret.Annotations == nil { + secret.Annotations = make(map[string]string) + } + secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) +} diff --git a/tests/integration/controller/withwatcher/suite_test.go b/tests/integration/controller/withwatcher/suite_test.go index d235d3b1a4..6af8bb5cef 100644 --- a/tests/integration/controller/withwatcher/suite_test.go +++ b/tests/integration/controller/withwatcher/suite_test.go @@ -195,7 +195,6 @@ var _ = BeforeSuite(func() { LocalGatewayPortOverwrite: "", } - caCertCache := watcher.NewCACertificateCache(5 * time.Minute) resolvedKcpAddr, err := gatewayConfig.ResolveKcpAddr(mgr) testEventRec := event.NewRecorderWrapper(mgr.GetEventRecorderFor(shared.OperatorName)) testSkrContextFactory = testskrcontext.NewDualClusterFactory(kcpClient.Scheme(), testEventRec) @@ -203,7 +202,6 @@ var _ = BeforeSuite(func() { skrWebhookChartManager, err := watcher.NewSKRWebhookManifestManager( kcpClient, testSkrContextFactory, - caCertCache, skrChartCfg, certificateConfig, resolvedKcpAddr) Expect(err).ToNot(HaveOccurred()) From a8217b5ad91eed38b20562a1b2691cbc974fb89e Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 4 Nov 2024 01:30:40 +0100 Subject: [PATCH 02/42] refactor: Lint --- cmd/main.go | 31 +++++++++++-------- pkg/watcher/certificate_manager.go | 2 +- pkg/zerodw/gateway.go | 9 +----- tests/integration/watcher/certificate_test.go | 2 +- 4 files changed, 21 insertions(+), 23 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 1fe50d858e..b0d7982ea4 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -21,11 +21,6 @@ import ( "errors" "flag" "fmt" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" - corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/watch" "net/http" "net/http/pprof" "os" @@ -38,10 +33,14 @@ import ( "go.uber.org/zap/zapcore" "golang.org/x/time/rate" istioclientapiv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1" + apicorev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" machineryruntime "k8s.io/apimachinery/pkg/runtime" machineryutilruntime "k8s.io/apimachinery/pkg/util/runtime" - clientgok8s "k8s.io/client-go/kubernetes" + "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/kubernetes" k8sclientscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" @@ -71,6 +70,7 @@ import ( "github.com/kyma-project/lifecycle-manager/pkg/matcher" "github.com/kyma-project/lifecycle-manager/pkg/queue" "github.com/kyma-project/lifecycle-manager/pkg/watcher" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" _ "k8s.io/client-go/plugin/pkg/client/auth" _ "ocm.software/ocm/api/ocm" @@ -202,7 +202,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma addHealthChecks(mgr, setupLog) - kcpClientset := clientgok8s.NewForConfigOrDie(config) + kcpClientset := kubernetes.NewForConfigOrDie(config) gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) go watchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) @@ -213,13 +213,13 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma setupLog.Error(err, "problem running manager") os.Exit(runtimeProblemExitCode) } - } -func watchChangesOnRootCertificate(clientset *clientgok8s.Clientset, gatewaySecretHandler *zerodw.GatewaySecretHandler, - setupLog logr.Logger) { - secretWatch, err := clientset.CoreV1().Secrets("istio-system").Watch(context.Background(), v1.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(v1.ObjectNameField, "klm-watcher").String(), +func watchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *zerodw.GatewaySecretHandler, + setupLog logr.Logger, +) { + secretWatch, err := clientset.CoreV1().Secrets("istio-system").Watch(context.Background(), apimetav1.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, zerodw.KCPRootSecretName).String(), }) if err != nil { setupLog.Error(err, "unable to start watching root certificate") @@ -227,7 +227,10 @@ func watchChangesOnRootCertificate(clientset *clientgok8s.Clientset, gatewaySecr } for e := range secretWatch.ResultChan() { - item := e.Object.(*corev1.Secret) + item, ok := e.Object.(*apicorev1.Secret) + if !ok { + setupLog.Info("unable to convert object to secret", "object", e.Object) + } switch e.Type { case watch.Modified: @@ -237,6 +240,8 @@ func watchChangesOnRootCertificate(clientset *clientgok8s.Clientset, gatewaySecr if err != nil { setupLog.Error(err, "unable to manage istio gateway secret") } + default: + setupLog.Info("unhandled event type", "event", e.Type) } } } diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index e137cd6bc2..814e9be0a5 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -19,6 +18,7 @@ import ( "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/util" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" ) const ( diff --git a/pkg/zerodw/gateway.go b/pkg/zerodw/gateway.go index 0079d95696..ac770020eb 100644 --- a/pkg/zerodw/gateway.go +++ b/pkg/zerodw/gateway.go @@ -16,7 +16,7 @@ import ( const ( GatewaySecretName = "istio-gateway-secret" - kcpRootSecretName = "klm-watcher" + KCPRootSecretName = "klm-watcher" kcpCACertName = "klm-watcher-serving" istioNamespace = flags.DefaultIstioNamespace ) @@ -90,13 +90,6 @@ func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) }) } -func (gsh *GatewaySecretHandler) findKcpRootSecret() (*apicorev1.Secret, error) { - return gsh.findSecret(context.Background(), client.ObjectKey{ - Name: kcpRootSecretName, - Namespace: istioNamespace, - }) -} - func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { gwSecret := &apicorev1.Secret{ TypeMeta: apimetav1.TypeMeta{ diff --git a/tests/integration/watcher/certificate_test.go b/tests/integration/watcher/certificate_test.go index 20fcc169df..5bf1ca83f8 100644 --- a/tests/integration/watcher/certificate_test.go +++ b/tests/integration/watcher/certificate_test.go @@ -84,7 +84,7 @@ var _ = Describe("Create Watcher Certificates", Ordered, func() { RenewBefore: 5 * time.Minute, } cert := watcher.NewCertificateManager(controlPlaneClient, - test.kyma.Name, config, watcher.NewCACertificateCache(1*time.Minute)) + test.kyma.Name, config) _, err := cert.CreateSelfSignedCert(ctx, test.kyma) if test.wantCreateErr { From c76cffac42286b6adb7130ec002a472a649aef31 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 4 Nov 2024 01:33:33 +0100 Subject: [PATCH 03/42] refactor: Lint --- cmd/main.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index b0d7982ea4..7d7a1de699 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -226,22 +226,28 @@ func watchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre return } - for e := range secretWatch.ResultChan() { - item, ok := e.Object.(*apicorev1.Secret) + for caughtEvent := range secretWatch.ResultChan() { + item, ok := caughtEvent.Object.(*apicorev1.Secret) if !ok { - setupLog.Info("unable to convert object to secret", "object", e.Object) + setupLog.Info("unable to convert object to secret", "object", caughtEvent.Object) } - switch e.Type { - case watch.Modified: - fallthrough + switch caughtEvent.Type { case watch.Added: + fallthrough + case watch.Modified: err := gatewaySecretHandler.ManageGatewaySecret(item) if err != nil { setupLog.Error(err, "unable to manage istio gateway secret") } + case watch.Deleted: + fallthrough + case watch.Error: + fallthrough + case watch.Bookmark: + fallthrough default: - setupLog.Info("unhandled event type", "event", e.Type) + setupLog.Info("ignored event type", "event", caughtEvent.Type) } } } From d4935ad882b6770c09a371c683adbb1e8a055ff0 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 4 Nov 2024 08:05:07 +0100 Subject: [PATCH 04/42] test: Simulate Gateway Secret in Watcher Suite --- cmd/main.go | 43 +-------------- pkg/zerodw/gateway.go | 42 +++++++++++++- .../controller/withwatcher/controller_test.go | 6 +- .../withwatcher/watcher_certificate_test.go | 6 +- .../watcher_controller_helper_test.go | 55 ++++++++----------- 5 files changed, 72 insertions(+), 80 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 7d7a1de699..552dd68482 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -33,13 +33,9 @@ import ( "go.uber.org/zap/zapcore" "golang.org/x/time/rate" istioclientapiv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1" - apicorev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" machineryruntime "k8s.io/apimachinery/pkg/runtime" machineryutilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" k8sclientscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/util/workqueue" @@ -204,7 +200,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma kcpClientset := kubernetes.NewForConfigOrDie(config) gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) - go watchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) @@ -215,43 +211,6 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma } } -func watchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *zerodw.GatewaySecretHandler, - setupLog logr.Logger, -) { - secretWatch, err := clientset.CoreV1().Secrets("istio-system").Watch(context.Background(), apimetav1.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, zerodw.KCPRootSecretName).String(), - }) - if err != nil { - setupLog.Error(err, "unable to start watching root certificate") - return - } - - for caughtEvent := range secretWatch.ResultChan() { - item, ok := caughtEvent.Object.(*apicorev1.Secret) - if !ok { - setupLog.Info("unable to convert object to secret", "object", caughtEvent.Object) - } - - switch caughtEvent.Type { - case watch.Added: - fallthrough - case watch.Modified: - err := gatewaySecretHandler.ManageGatewaySecret(item) - if err != nil { - setupLog.Error(err, "unable to manage istio gateway secret") - } - case watch.Deleted: - fallthrough - case watch.Error: - fallthrough - case watch.Bookmark: - fallthrough - default: - setupLog.Info("ignored event type", "event", caughtEvent.Type) - } - } -} - func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/pkg/zerodw/gateway.go b/pkg/zerodw/gateway.go index ac770020eb..0334c78cae 100644 --- a/pkg/zerodw/gateway.go +++ b/pkg/zerodw/gateway.go @@ -9,6 +9,9 @@ import ( "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" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" @@ -16,7 +19,7 @@ import ( const ( GatewaySecretName = "istio-gateway-secret" - KCPRootSecretName = "klm-watcher" + kcpRootSecretName = "klm-watcher" kcpCACertName = "klm-watcher-serving" istioNamespace = flags.DefaultIstioNamespace ) @@ -108,3 +111,40 @@ func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) } return gwSecret } + +func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, + setupLog logr.Logger, +) { + secretWatch, err := clientset.CoreV1().Secrets("istio-system").Watch(context.Background(), apimetav1.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), + }) + if err != nil { + setupLog.Error(err, "unable to start watching root certificate") + return + } + + for event := range secretWatch.ResultChan() { + item, ok := event.Object.(*apicorev1.Secret) + if !ok { + setupLog.Info("unable to convert object to secret", "object", event.Object) + } + + switch event.Type { + case watch.Added: + fallthrough + case watch.Modified: + err := gatewaySecretHandler.ManageGatewaySecret(item) + if err != nil { + setupLog.Error(err, "unable to manage istio gateway secret") + } + case watch.Deleted: + fallthrough + case watch.Error: + fallthrough + case watch.Bookmark: + fallthrough + default: + setupLog.Info("ignored event type", "event", event.Type) + } + } +} diff --git a/tests/integration/controller/withwatcher/controller_test.go b/tests/integration/controller/withwatcher/controller_test.go index 3b6c6b0a04..72590e6a03 100644 --- a/tests/integration/controller/withwatcher/controller_test.go +++ b/tests/integration/controller/withwatcher/controller_test.go @@ -44,9 +44,9 @@ var _ = Describe("Kyma with multiple module CRs in remote sync mode", Ordered, f watcherCrForKyma := createWatcherCR("skr-webhook-manager", true) issuer := NewTestIssuer(istioSystemNs) kymaObjKey := client.ObjectKeyFromObject(kyma) - tlsSecret := createTLSSecret(kymaObjKey) - caCertificate := createCaCertificate() - registerDefaultLifecycleForKymaWithWatcher(kyma, watcherCrForKyma, tlsSecret, issuer, caCertificate) + tlsSecret := createWatcherSecret(kymaObjKey) + gatewaySecret := createGatewaySecret() + registerDefaultLifecycleForKymaWithWatcher(kyma, watcherCrForKyma, tlsSecret, issuer, gatewaySecret) var skrClient client.Client var err error BeforeAll(func() { diff --git a/tests/integration/controller/withwatcher/watcher_certificate_test.go b/tests/integration/controller/withwatcher/watcher_certificate_test.go index b7a5b2c1e5..611263c5e9 100644 --- a/tests/integration/controller/withwatcher/watcher_certificate_test.go +++ b/tests/integration/controller/withwatcher/watcher_certificate_test.go @@ -28,11 +28,11 @@ var _ = Describe("Watcher Certificate Configuration in remote sync mode", Ordere watcherCrForKyma := createWatcherCR("skr-webhook-manager", true) issuer := NewTestIssuer(istioSystemNs) kymaObjKey := client.ObjectKeyFromObject(kyma) - tlsSecret := createTLSSecret(kymaObjKey) + tlsSecret := createWatcherSecret(kymaObjKey) skrTLSSecretObjKey := client.ObjectKey{Name: watcher.SkrTLSName, Namespace: flags.DefaultRemoteSyncNamespace} - caCertificate := createCaCertificate() + gatewaySecret := createGatewaySecret() - registerDefaultLifecycleForKymaWithWatcher(kyma, watcherCrForKyma, tlsSecret, issuer, caCertificate) + registerDefaultLifecycleForKymaWithWatcher(kyma, watcherCrForKyma, tlsSecret, issuer, gatewaySecret) var skrClient client.Client var err error BeforeAll(func() { diff --git a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go index ed39a9cd75..1f00d10622 100644 --- a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go +++ b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go @@ -3,8 +3,10 @@ package withwatcher_test import ( "context" "errors" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" "io" "os" + "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" istioapiv1beta1 "istio.io/api/networking/v1beta1" @@ -44,7 +46,7 @@ var ( ) func registerDefaultLifecycleForKymaWithWatcher(kyma *v1beta2.Kyma, watcher *v1beta2.Watcher, - tlsSecret *apicorev1.Secret, issuer *certmanagerv1.Issuer, caCert *certmanagerv1.Certificate, + tlsSecret *apicorev1.Secret, issuer *certmanagerv1.Issuer, gatewaySecret *apicorev1.Secret, ) { BeforeAll(func() { By("Creating watcher CR") @@ -56,7 +58,7 @@ func registerDefaultLifecycleForKymaWithWatcher(kyma *v1beta2.Kyma, watcher *v1b By("Creating Cert-Manager Issuer") Expect(kcpClient.Create(ctx, issuer)).To(Succeed()) By("Creating CA Certificate") - Expect(kcpClient.Create(ctx, caCert)).To(Succeed()) + Expect(kcpClient.Create(ctx, gatewaySecret)).To(Succeed()) }) AfterAll(func() { @@ -70,7 +72,7 @@ func registerDefaultLifecycleForKymaWithWatcher(kyma *v1beta2.Kyma, watcher *v1b By("Deleting Cert-Manager Issuer") Expect(kcpClient.Delete(ctx, issuer)).To(Succeed()) By("Deleting CA Certificate") - Expect(kcpClient.Delete(ctx, caCert)).To(Succeed()) + Expect(kcpClient.Delete(ctx, gatewaySecret)).To(Succeed()) }) BeforeEach(func() { @@ -126,33 +128,6 @@ func isEven(idx int) bool { return idx%2 == 0 } -func createCaCertificate() *certmanagerv1.Certificate { - return &certmanagerv1.Certificate{ - TypeMeta: apimetav1.TypeMeta{ - Kind: certmanagerv1.CertificateKind, - APIVersion: certmanagerv1.SchemeGroupVersion.String(), - }, - ObjectMeta: apimetav1.ObjectMeta{ - Name: "klm-watcher-serving", - Namespace: istioSystemNs, - }, - Spec: certmanagerv1.CertificateSpec{ - DNSNames: []string{"listener.kyma.cloud.sap"}, - IsCA: true, - CommonName: "klm-watcher-selfsigned-ca", - SecretName: "klm-watcher-root-secret", - SecretTemplate: &certmanagerv1.CertificateSecretTemplate{ - Labels: map[string]string{ - shared.ManagedBy: shared.OperatorName, - }, - }, - PrivateKey: &certmanagerv1.CertificatePrivateKey{ - Algorithm: "RSA", - }, - }, - } -} - func createWatcherCR(managerInstanceName string, statusOnly bool) *v1beta2.Watcher { field := v1beta2.SpecField if statusOnly { @@ -192,7 +167,7 @@ func createWatcherCR(managerInstanceName string, statusOnly bool) *v1beta2.Watch } } -func createTLSSecret(kymaObjKey client.ObjectKey) *apicorev1.Secret { +func createWatcherSecret(kymaObjKey client.ObjectKey) *apicorev1.Secret { return &apicorev1.Secret{ ObjectMeta: apimetav1.ObjectMeta{ Name: watcher.ResolveTLSCertName(kymaObjKey.Name), @@ -210,6 +185,24 @@ func createTLSSecret(kymaObjKey client.ObjectKey) *apicorev1.Secret { } } +func createGatewaySecret() *apicorev1.Secret { + return &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Name: zerodw.GatewaySecretName, + Namespace: istioSystemNs, + Annotations: map[string]string{ + zerodw.LastModifiedAtAnnotation: apimetav1.Now().Add(-1 * time.Hour).Format(time.RFC3339), + }, + }, + Data: map[string][]byte{ + "ca.crt": []byte("jelly"), + "tls.crt": []byte("jellyfish"), + "tls.key": []byte("jellyfishes"), + }, + Type: apicorev1.SecretTypeOpaque, + } +} + func getWatcher(name string) (*v1beta2.Watcher, error) { watcherCR := &v1beta2.Watcher{} err := kcpClient.Get(ctx, From b71c97536d1ffbe56520693447d5b54887ed890e Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 4 Nov 2024 09:06:03 +0100 Subject: [PATCH 05/42] refactor: gofumpt and gci --- .../controller/withwatcher/watcher_controller_helper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go index 1f00d10622..7d1947171d 100644 --- a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go +++ b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go @@ -3,7 +3,6 @@ package withwatcher_test import ( "context" "errors" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" "io" "os" "time" @@ -21,6 +20,7 @@ import ( "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/istio" "github.com/kyma-project/lifecycle-manager/pkg/watcher" + "github.com/kyma-project/lifecycle-manager/pkg/zerodw" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" From 897dd0f871be06cadba432dd11f2dbcc2d41ad8b Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 4 Nov 2024 10:27:39 +0100 Subject: [PATCH 06/42] fix: ca rotation e2e test --- pkg/testutils/watcher.go | 28 +++++++++++++++++++++-- tests/e2e/ca_certificate_rotation_test.go | 17 ++++++++------ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/pkg/testutils/watcher.go b/pkg/testutils/watcher.go index 0e1a5a8af4..e47adc2b59 100644 --- a/pkg/testutils/watcher.go +++ b/pkg/testutils/watcher.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" apicorev1 "k8s.io/api/core/v1" @@ -14,8 +15,9 @@ import ( ) var ( - errOldCreationTime = errors.New("certificate has an old creation timestamp") - errNotSyncedSecret = errors.New("secret is not synced to skr cluster") + errOldCreationTime = errors.New("certificate has an old creation timestamp") + errNotSyncedSecret = errors.New("secret is not synced to skr cluster") + errTlsSecretNotRotated = errors.New("tls secret did not rotated") ) func CertificateSecretExists(ctx context.Context, secretName types.NamespacedName, k8sClient client.Client) error { @@ -43,6 +45,18 @@ func CertificateSecretIsCreatedAfter(ctx context.Context, return nil } +func TlsSecretRotated(ctx context.Context, oldValue time.Time, + namespacedSecretName types.NamespacedName, kcpClient client.Client) error { + secret, err := GetTlsSecret(ctx, namespacedSecretName, kcpClient) + if err != nil { + return fmt.Errorf("failed to fetch tls secret: %w", err) + } + if secret.CreationTimestamp.Time == oldValue { + return errTlsSecretNotRotated + } + return nil +} + func CertificateSecretIsSyncedToSkrCluster(ctx context.Context, kcpSecretName types.NamespacedName, kcpClient client.Client, skrSecretName types.NamespacedName, skrClient client.Client, @@ -104,3 +118,13 @@ func GetCACertificate(ctx context.Context, namespacedCertName types.NamespacedNa return caCert, nil } + +func GetTlsSecret(ctx context.Context, namespacedSecretName types.NamespacedName, k8sClient client.Client, +) (*apicorev1.Secret, error) { + tlsSecret := &apicorev1.Secret{} + if err := k8sClient.Get(ctx, namespacedSecretName, tlsSecret); err != nil { + return nil, fmt.Errorf("failed to get secret %w", err) + } + + return tlsSecret, nil +} diff --git a/tests/e2e/ca_certificate_rotation_test.go b/tests/e2e/ca_certificate_rotation_test.go index 006f0889fe..1802bbeb72 100644 --- a/tests/e2e/ca_certificate_rotation_test.go +++ b/tests/e2e/ca_certificate_rotation_test.go @@ -4,7 +4,6 @@ import ( "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "github.com/kyma-project/lifecycle-manager/api/v1beta2" @@ -34,17 +33,21 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { Namespace: RemoteNamespace, } It("Then KCP TLS Certificate is removed", func() { - timeNow := &apimetav1.Time{Time: time.Now()} - expectedLogMessage := "CA Certificate was rotated, removing certificate" + var err error + namespacedSecretName := types.NamespacedName{ + Name: watcher.ResolveTLSCertName(kyma.Name), + Namespace: "istio-system", + } + tlsSecret, err := GetTlsSecret(ctx, namespacedSecretName, kcpClient) + Expect(err).NotTo(HaveOccurred()) + // The timeout used is 4 minutes bec the certificate gets rotated every 1 minute - Eventually(CheckPodLogs, 4*time.Minute). + Eventually(TlsSecretRotated, 4*time.Minute). WithContext(ctx). - WithArguments(ControlPlaneNamespace, KLMPodPrefix, KLMPodContainer, expectedLogMessage, kcpRESTConfig, - kcpClient, timeNow). + WithArguments(tlsSecret.CreationTimestamp.Time, namespacedSecretName, kcpClient). Should(Succeed()) By("And new TLS Certificate is created") - var err error namespacedCertName := types.NamespacedName{ Name: caCertName, Namespace: "istio-system", From 5cb068d82d7f9410d8db27ada7dd59b8bd57cc56 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 6 Nov 2024 09:25:09 +0100 Subject: [PATCH 07/42] test: ca rotation e2e fix --- cmd/main.go | 8 ++++---- pkg/testutils/watcher.go | 3 ++- pkg/watcher/certificate_manager.go | 20 +++++--------------- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 552dd68482..e64bc563db 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -198,10 +198,6 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma addHealthChecks(mgr, setupLog) - kcpClientset := kubernetes.NewForConfigOrDie(config) - gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) - go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) - go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) @@ -209,6 +205,10 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma setupLog.Error(err, "problem running manager") os.Exit(runtimeProblemExitCode) } + + kcpClientset := kubernetes.NewForConfigOrDie(config) + gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) + go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { diff --git a/pkg/testutils/watcher.go b/pkg/testutils/watcher.go index e47adc2b59..b8192212f4 100644 --- a/pkg/testutils/watcher.go +++ b/pkg/testutils/watcher.go @@ -46,7 +46,8 @@ func CertificateSecretIsCreatedAfter(ctx context.Context, } func TlsSecretRotated(ctx context.Context, oldValue time.Time, - namespacedSecretName types.NamespacedName, kcpClient client.Client) error { + namespacedSecretName types.NamespacedName, kcpClient client.Client, +) error { secret, err := GetTlsSecret(ctx, namespacedSecretName, kcpClient) if err != nil { return fmt.Errorf("failed to fetch tls secret: %w", err) diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index 814e9be0a5..6a6f043a95 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -259,18 +259,6 @@ func (c *CertificateManager) getIssuer(ctx context.Context) (*certmanagerv1.Issu return &issuerList.Items[0], nil } -func (c *CertificateManager) getCertificateSecret(ctx context.Context) (*apicorev1.Secret, error) { - secret := &apicorev1.Secret{} - err := c.kcpClient.Get(ctx, client.ObjectKey{Name: c.secretName, Namespace: c.config.IstioNamespace}, - secret) - if err != nil { - return nil, fmt.Errorf("failed to get secret for certificate %s-%s: %w", c.secretName, c.config.IstioNamespace, - err) - } - - return secret, nil -} - type CertificateNotReadyError struct{} func (e *CertificateNotReadyError) Error() string { @@ -285,10 +273,12 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym if err != nil { return err } - - watcherCert, err := c.getCertificateSecret(ctx) + watcherCert, err := getCertificateSecret(ctx, c.kcpClient, client.ObjectKey{ + Namespace: c.config.IstioNamespace, + Name: c.secretName, + }) if err != nil { - return fmt.Errorf("error while fetching certificate: %w", err) + return err } if watcherCert != nil && isGatewaySecretNewerThanWatcherCert(gatewaySecret, watcherCert) { From d48fd6f68b538b551dd1f032dfba343077c68c7c Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 7 Nov 2024 15:41:32 +0100 Subject: [PATCH 08/42] test: ca rotation e2e fix --- cmd/main.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index ce9ad3e6d4..6ff044686c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -203,14 +203,14 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) + kcpClientset := kubernetes.NewForConfigOrDie(config) + gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) + go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") os.Exit(runtimeProblemExitCode) } - - kcpClientset := kubernetes.NewForConfigOrDie(config) - gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) - go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { From 6e35338b130076f36e2900e73c3aa3645fe8fe81 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 7 Nov 2024 19:36:56 +0100 Subject: [PATCH 09/42] fix: give controller-manager permission to update istio-gateway-secret --- cmd/main.go | 2 +- .../watcher_certmanager_role.yaml | 1 + pkg/testutils/watcher.go | 10 +++++----- pkg/zerodw/gateway.go | 16 +++------------- pkg/zerodw/secret.go | 2 -- tests/e2e/ca_certificate_rotation_test.go | 4 ++-- 6 files changed, 12 insertions(+), 23 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 6ff044686c..61fbc382f5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -204,7 +204,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) kcpClientset := kubernetes.NewForConfigOrDie(config) - gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient, setupLog) + gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient) go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { diff --git a/config/rbac/namespace_bindings/watcher_certmanager_role.yaml b/config/rbac/namespace_bindings/watcher_certmanager_role.yaml index a183b1bfd9..e6bc636d36 100644 --- a/config/rbac/namespace_bindings/watcher_certmanager_role.yaml +++ b/config/rbac/namespace_bindings/watcher_certmanager_role.yaml @@ -15,6 +15,7 @@ rules: - watch - create - delete + - update - apiGroups: - cert-manager.io resources: diff --git a/pkg/testutils/watcher.go b/pkg/testutils/watcher.go index b8192212f4..2d7b4da834 100644 --- a/pkg/testutils/watcher.go +++ b/pkg/testutils/watcher.go @@ -17,7 +17,7 @@ import ( var ( errOldCreationTime = errors.New("certificate has an old creation timestamp") errNotSyncedSecret = errors.New("secret is not synced to skr cluster") - errTlsSecretNotRotated = errors.New("tls secret did not rotated") + errTLSSecretNotRotated = errors.New("tls secret did not rotated") ) func CertificateSecretExists(ctx context.Context, secretName types.NamespacedName, k8sClient client.Client) error { @@ -45,15 +45,15 @@ func CertificateSecretIsCreatedAfter(ctx context.Context, return nil } -func TlsSecretRotated(ctx context.Context, oldValue time.Time, +func TLSSecretRotated(ctx context.Context, oldValue time.Time, namespacedSecretName types.NamespacedName, kcpClient client.Client, ) error { - secret, err := GetTlsSecret(ctx, namespacedSecretName, kcpClient) + secret, err := GetTLSSecret(ctx, namespacedSecretName, kcpClient) if err != nil { return fmt.Errorf("failed to fetch tls secret: %w", err) } if secret.CreationTimestamp.Time == oldValue { - return errTlsSecretNotRotated + return errTLSSecretNotRotated } return nil } @@ -120,7 +120,7 @@ func GetCACertificate(ctx context.Context, namespacedCertName types.NamespacedNa return caCert, nil } -func GetTlsSecret(ctx context.Context, namespacedSecretName types.NamespacedName, k8sClient client.Client, +func GetTLSSecret(ctx context.Context, namespacedSecretName types.NamespacedName, k8sClient client.Client, ) (*apicorev1.Secret, error) { tlsSecret := &apicorev1.Secret{} if err := k8sClient.Get(ctx, namespacedSecretName, tlsSecret); err != nil { diff --git a/pkg/zerodw/gateway.go b/pkg/zerodw/gateway.go index 0334c78cae..ff6612250b 100644 --- a/pkg/zerodw/gateway.go +++ b/pkg/zerodw/gateway.go @@ -28,11 +28,10 @@ type GatewaySecretHandler struct { *secretManager } -func NewGatewaySecretHandler(kcpClient client.Client, log logr.Logger) *GatewaySecretHandler { +func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { return &GatewaySecretHandler{ secretManager: &secretManager{ kcpClient: kcpClient, - log: log, }, } } @@ -52,11 +51,7 @@ func (gsh *GatewaySecretHandler) ManageGatewaySecret(rootSecret *apicorev1.Secre func (gsh *GatewaySecretHandler) handleNonExisting(rootSecret *apicorev1.Secret) error { gwSecret := gsh.newGatewaySecret(rootSecret) - err := gsh.create(context.Background(), gwSecret) - if err == nil { - gsh.log.Info("created the gateway secret", "reason", "gateway secret does not exist") - } - return err + return gsh.create(context.Background(), gwSecret) } func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret) error { @@ -78,12 +73,7 @@ func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gw gwSecret.Data["tls.crt"] = rootSecret.Data["tls.crt"] gwSecret.Data["tls.key"] = rootSecret.Data["tls.key"] gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] - err := gsh.update(context.Background(), gwSecret) - if err == nil { - gsh.log.Info("updated the gateway secret", "reason", "root ca is more recent than the gateway secret") - } - - return nil + return gsh.update(context.Background(), gwSecret) } func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) { diff --git a/pkg/zerodw/secret.go b/pkg/zerodw/secret.go index e4560801a0..2d7d6f2b2a 100644 --- a/pkg/zerodw/secret.go +++ b/pkg/zerodw/secret.go @@ -5,7 +5,6 @@ import ( "fmt" "time" - "github.com/go-logr/logr" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -16,7 +15,6 @@ const ( ) type secretManager struct { - log logr.Logger kcpClient client.Client } diff --git a/tests/e2e/ca_certificate_rotation_test.go b/tests/e2e/ca_certificate_rotation_test.go index 1802bbeb72..15b76d170f 100644 --- a/tests/e2e/ca_certificate_rotation_test.go +++ b/tests/e2e/ca_certificate_rotation_test.go @@ -38,11 +38,11 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { Name: watcher.ResolveTLSCertName(kyma.Name), Namespace: "istio-system", } - tlsSecret, err := GetTlsSecret(ctx, namespacedSecretName, kcpClient) + tlsSecret, err := GetTLSSecret(ctx, namespacedSecretName, kcpClient) Expect(err).NotTo(HaveOccurred()) // The timeout used is 4 minutes bec the certificate gets rotated every 1 minute - Eventually(TlsSecretRotated, 4*time.Minute). + Eventually(TLSSecretRotated, 4*time.Minute). WithContext(ctx). WithArguments(tlsSecret.CreationTimestamp.Time, namespacedSecretName, kcpClient). Should(Succeed()) From aeacc42855c6ed03ae34fe4c25fb4efb26f08661 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 7 Nov 2024 20:52:52 +0100 Subject: [PATCH 10/42] fix: give controller-manager permission to update istio-gateway-secret --- cmd/main.go | 12 +++++++++--- pkg/zerodw/gateway.go | 19 ++++++++----------- tests/e2e/rbac_privileges_test.go | 2 +- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 61fbc382f5..87d51baa35 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -38,6 +38,7 @@ 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" @@ -203,9 +204,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) - kcpClientset := kubernetes.NewForConfigOrDie(config) - gatewaySecretHandler := zerodw.NewGatewaySecretHandler(kcpClient) - go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + setupIstioGatewaySecretRotation(config, kcpClient, setupLog) if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") @@ -213,6 +212,13 @@ 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 := zerodw.NewGatewaySecretHandler(kcpClient) + + go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) +} + func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/pkg/zerodw/gateway.go b/pkg/zerodw/gateway.go index ff6612250b..4efc98154f 100644 --- a/pkg/zerodw/gateway.go +++ b/pkg/zerodw/gateway.go @@ -103,29 +103,26 @@ func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) } func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, - setupLog logr.Logger, + log logr.Logger, ) { - secretWatch, err := clientset.CoreV1().Secrets("istio-system").Watch(context.Background(), apimetav1.ListOptions{ + secretWatch, err := clientset.CoreV1().Secrets(istioNamespace).Watch(context.Background(), apimetav1.ListOptions{ FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), }) if err != nil { - setupLog.Error(err, "unable to start watching root certificate") - return + log.Error(err, "unable to start watching root certificate") + panic(err) } for event := range secretWatch.ResultChan() { - item, ok := event.Object.(*apicorev1.Secret) - if !ok { - setupLog.Info("unable to convert object to secret", "object", event.Object) - } + rootCASecret, _ := event.Object.(*apicorev1.Secret) switch event.Type { case watch.Added: fallthrough case watch.Modified: - err := gatewaySecretHandler.ManageGatewaySecret(item) + err := gatewaySecretHandler.ManageGatewaySecret(rootCASecret) if err != nil { - setupLog.Error(err, "unable to manage istio gateway secret") + log.Error(err, "unable to manage istio gateway secret") } case watch.Deleted: fallthrough @@ -134,7 +131,7 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre case watch.Bookmark: fallthrough default: - setupLog.Info("ignored event type", "event", event.Type) + continue } } } diff --git a/tests/e2e/rbac_privileges_test.go b/tests/e2e/rbac_privileges_test.go index 1bc3fe8ede..2184178ad1 100644 --- a/tests/e2e/rbac_privileges_test.go +++ b/tests/e2e/rbac_privileges_test.go @@ -178,7 +178,7 @@ var _ = Describe("RBAC Privileges", func() { { APIGroups: []string{""}, Resources: []string{"secrets"}, - Verbs: []string{"list", "watch", "create", "delete"}, + Verbs: []string{"list", "watch", "create", "delete", "update"}, }, { APIGroups: []string{"cert-manager.io"}, From 758c4d321faafebb1a0eeb81646ff4a6bf068a29 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 7 Nov 2024 22:03:54 +0100 Subject: [PATCH 11/42] fix: KLM image name in kustomization --- config/manager/kustomization.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index a33bdd2380..00ae86ff75 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -10,5 +10,5 @@ generatorOptions: images: - name: controller - newName: k3d-myregistry.localhost:5000/lifecycle-manager - newTag: "20241104001047" + newName: europe-docker.pkg.dev/kyma-project/prod/lifecycle-manager + newTag: latest From ed7ea6355ffe1d4067b42ece462b055376ec5933 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 11 Nov 2024 14:11:06 +0100 Subject: [PATCH 12/42] refactor: Rename zerodw package --- cmd/main.go | 6 +++--- pkg/{zerodw/gateway.go => gatewaysecret/handler.go} | 2 +- pkg/{zerodw => gatewaysecret}/secret.go | 2 +- pkg/watcher/certificate_manager.go | 6 +++--- pkg/watcher/skr_webhook_manifest_manager.go | 6 +++--- .../withwatcher/watcher_controller_helper_test.go | 6 +++--- 6 files changed, 14 insertions(+), 14 deletions(-) rename pkg/{zerodw/gateway.go => gatewaysecret/handler.go} (99%) rename pkg/{zerodw => gatewaysecret}/secret.go (98%) diff --git a/cmd/main.go b/cmd/main.go index 87d51baa35..145b6df203 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -63,11 +63,11 @@ 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" "github.com/kyma-project/lifecycle-manager/pkg/watcher" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" _ "k8s.io/client-go/plugin/pkg/client/auth" _ "ocm.software/ocm/api/ocm" @@ -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 := zerodw.NewGatewaySecretHandler(kcpClient) + gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient) - go zerodw.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + go gatewaysecret.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { diff --git a/pkg/zerodw/gateway.go b/pkg/gatewaysecret/handler.go similarity index 99% rename from pkg/zerodw/gateway.go rename to pkg/gatewaysecret/handler.go index 4efc98154f..922ec8de22 100644 --- a/pkg/zerodw/gateway.go +++ b/pkg/gatewaysecret/handler.go @@ -1,4 +1,4 @@ -package zerodw +package gatewaysecret import ( "context" diff --git a/pkg/zerodw/secret.go b/pkg/gatewaysecret/secret.go similarity index 98% rename from pkg/zerodw/secret.go rename to pkg/gatewaysecret/secret.go index 2d7d6f2b2a..6caabe2661 100644 --- a/pkg/zerodw/secret.go +++ b/pkg/gatewaysecret/secret.go @@ -1,4 +1,4 @@ -package zerodw +package gatewaysecret import ( "context" diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index 6a6f043a95..b347584b40 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -16,9 +16,9 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/util" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" ) const ( @@ -268,7 +268,7 @@ func (e *CertificateNotReadyError) Error() string { func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kymaObjKey client.ObjectKey) error { gatewaySecret, err := getCertificateSecret(ctx, c.kcpClient, client.ObjectKey{ Namespace: c.config.IstioNamespace, - Name: zerodw.GatewaySecretName, + Name: gatewaysecret.GatewaySecretName, }) if err != nil { return err @@ -293,7 +293,7 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym } func isGatewaySecretNewerThanWatcherCert(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { - if gwSecretLastModifiedAtValue, ok := gatewaySecret.Annotations[zerodw.LastModifiedAtAnnotation]; ok { + if gwSecretLastModifiedAtValue, ok := gatewaySecret.Annotations[gatewaysecret.LastModifiedAtAnnotation]; ok { if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { if watcherSecret.CreationTimestamp.Time.After(gwSecretLastModifiedAt) { return false diff --git a/pkg/watcher/skr_webhook_manifest_manager.go b/pkg/watcher/skr_webhook_manifest_manager.go index 73d9470ac3..1f41a6459a 100644 --- a/pkg/watcher/skr_webhook_manifest_manager.go +++ b/pkg/watcher/skr_webhook_manifest_manager.go @@ -19,9 +19,9 @@ import ( "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/remote" "github.com/kyma-project/lifecycle-manager/internal/util/collections" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/util" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" ) type SKRWebhookManifestManager struct { @@ -87,7 +87,7 @@ func (m *SKRWebhookManifestManager) Install(ctx context.Context, kyma *v1beta2.K _, err = getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ Namespace: m.certificateConfig.IstioNamespace, - Name: zerodw.GatewaySecretName, + Name: gatewaysecret.GatewaySecretName, }) if err != nil { return err @@ -233,7 +233,7 @@ func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.C gatewaySecret, err := getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ Namespace: m.certificateConfig.IstioNamespace, - Name: zerodw.GatewaySecretName, + Name: gatewaysecret.GatewaySecretName, }) if err != nil { return nil, err diff --git a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go index 7d1947171d..624bf3d079 100644 --- a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go +++ b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go @@ -19,8 +19,8 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/istio" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "github.com/kyma-project/lifecycle-manager/pkg/watcher" - "github.com/kyma-project/lifecycle-manager/pkg/zerodw" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -188,10 +188,10 @@ func createWatcherSecret(kymaObjKey client.ObjectKey) *apicorev1.Secret { func createGatewaySecret() *apicorev1.Secret { return &apicorev1.Secret{ ObjectMeta: apimetav1.ObjectMeta{ - Name: zerodw.GatewaySecretName, + Name: gatewaysecret.GatewaySecretName, Namespace: istioSystemNs, Annotations: map[string]string{ - zerodw.LastModifiedAtAnnotation: apimetav1.Now().Add(-1 * time.Hour).Format(time.RFC3339), + gatewaysecret.LastModifiedAtAnnotation: apimetav1.Now().Add(-1 * time.Hour).Format(time.RFC3339), }, }, Data: map[string][]byte{ From 7016aaccd58528904b2d7d86e20b9b4fbc2d4370 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 14 Nov 2024 03:20:26 +0100 Subject: [PATCH 13/42] refactor: PR Review --- config/watcher/gateway.yaml | 2 +- pkg/gatewaysecret/handler.go | 91 ++++++++++++----- pkg/gatewaysecret/secret.go | 61 ----------- pkg/watcher/certificate_manager.go | 12 +-- pkg/watcher/certificate_manager_test.go | 107 ++++++++++++++++++++ pkg/watcher/skr_webhook_manifest_manager.go | 5 +- tests/e2e/ca_certificate_rotation_test.go | 6 +- 7 files changed, 187 insertions(+), 97 deletions(-) delete mode 100644 pkg/gatewaysecret/secret.go create mode 100644 pkg/watcher/certificate_manager_test.go diff --git a/config/watcher/gateway.yaml b/config/watcher/gateway.yaml index 030ab60701..5eb8034307 100644 --- a/config/watcher/gateway.yaml +++ b/config/watcher/gateway.yaml @@ -20,5 +20,5 @@ spec: number: 443 protocol: HTTPS tls: - credentialName: istio-gateway-secret + credentialName: klm-istio-gateway mode: MUTUAL diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 922ec8de22..afb2e45fd6 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -2,9 +2,12 @@ package gatewaysecret import ( "context" + "errors" "fmt" "time" + "github.com/kyma-project/lifecycle-manager/pkg/util" + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" apicorev1 "k8s.io/api/core/v1" @@ -13,33 +16,32 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" ) const ( - GatewaySecretName = "istio-gateway-secret" - kcpRootSecretName = "klm-watcher" - kcpCACertName = "klm-watcher-serving" - istioNamespace = flags.DefaultIstioNamespace + LastModifiedAtAnnotation = "lastModifiedAt" + GatewaySecretName = "klm-istio-gateway" //nolint:gosec // GatewaySecretName is not a credential + kcpRootSecretName = "klm-watcher" + kcpCACertName = "klm-watcher-serving" + istioNamespace = "istio-system" ) +var errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") + type GatewaySecretHandler struct { - *secretManager + kcpClient client.Client } func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { return &GatewaySecretHandler{ - secretManager: &secretManager{ - kcpClient: kcpClient, - }, + kcpClient: kcpClient, } } func (gsh *GatewaySecretHandler) ManageGatewaySecret(rootSecret *apicorev1.Secret) error { gwSecret, err := gsh.findGatewaySecret() - if isNotFound(err) { + if util.IsNotFound(err) { return gsh.handleNonExisting(rootSecret) } if err != nil { @@ -62,11 +64,9 @@ func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gw return fmt.Errorf("failed to get CA certificate: %w", err) } - if gwSecretLastModifiedAtValue, ok := gwSecret.Annotations[LastModifiedAtAnnotation]; ok { - if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { - if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { - return nil - } + if gwSecretLastModifiedAt, err := GetLastModifiedAt(gwSecret); err == nil { + if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { + return nil } } @@ -76,13 +76,6 @@ func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gw return gsh.update(context.Background(), gwSecret) } -func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) { - return gsh.findSecret(context.Background(), client.ObjectKey{ - Name: GatewaySecretName, - Namespace: istioNamespace, - }) -} - func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { gwSecret := &apicorev1.Secret{ TypeMeta: apimetav1.TypeMeta{ @@ -102,6 +95,56 @@ func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) return gwSecret } +func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) { + return gsh.findSecret(context.Background(), client.ObjectKey{ + Name: GatewaySecretName, + Namespace: istioNamespace, + }) +} + +func (gsh *GatewaySecretHandler) findSecret(ctx context.Context, objKey client.ObjectKey) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + + err := gsh.kcpClient.Get(ctx, objKey, secret) + if err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", objKey.Name, err) + } + + return secret, nil +} + +func (gsh *GatewaySecretHandler) create(ctx context.Context, secret *apicorev1.Secret) error { + gsh.updateLastModifiedAt(secret) + if err := gsh.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 { + return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) + } + return nil +} + +func (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) { + if secret.Annotations == nil { + secret.Annotations = make(map[string]string) + } + secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) +} + +func GetLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { + if gwSecretLastModifiedAtValue, ok := secret.Annotations[LastModifiedAtAnnotation]; ok { + if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { + return gwSecretLastModifiedAt, nil + } + } + return time.Time{}, errCouldNotGetLastModifiedAt +} + func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, log logr.Logger, ) { @@ -125,6 +168,8 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre log.Error(err, "unable to manage istio gateway secret") } case watch.Deleted: + // ignored because it is an invalid state and cert manager should not delete the root secret + // even if it is deleted, the certificate manager will recreate it, and trigger the watch event fallthrough case watch.Error: fallthrough diff --git a/pkg/gatewaysecret/secret.go b/pkg/gatewaysecret/secret.go deleted file mode 100644 index 6caabe2661..0000000000 --- a/pkg/gatewaysecret/secret.go +++ /dev/null @@ -1,61 +0,0 @@ -package gatewaysecret - -import ( - "context" - "fmt" - "time" - - apicorev1 "k8s.io/api/core/v1" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -const ( - LastModifiedAtAnnotation = "lastModifiedAt" -) - -type secretManager struct { - kcpClient client.Client -} - -func (sm *secretManager) findSecret(ctx context.Context, objKey client.ObjectKey) (*apicorev1.Secret, error) { - secret := &apicorev1.Secret{} - - err := sm.kcpClient.Get(ctx, objKey, secret) - if err != nil { - return nil, fmt.Errorf("failed to get secret %s: %w", objKey.Name, err) - } - - return secret, nil -} - -// isNotFound returns true if the error is a NotFound error. -func isNotFound(err error) bool { - if err == nil { - return false - } - return client.IgnoreNotFound(err) == nil -} - -func (sm *secretManager) create(ctx context.Context, secret *apicorev1.Secret) error { - sm.updateLastModifiedAt(secret) - if err := sm.kcpClient.Create(ctx, secret); err != nil { - return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) - } - return nil -} - -func (sm *secretManager) update(ctx context.Context, secret *apicorev1.Secret) error { - sm.updateLastModifiedAt(secret) - if err := sm.kcpClient.Update(ctx, secret); err != nil { - return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) - } - return nil -} - -func (sm *secretManager) updateLastModifiedAt(secret *apicorev1.Secret) { - if secret.Annotations == nil { - secret.Annotations = make(map[string]string) - } - secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) -} diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index b347584b40..0b4e3c9cf5 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -281,7 +281,7 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym return err } - if watcherCert != nil && isGatewaySecretNewerThanWatcherCert(gatewaySecret, watcherCert) { + if watcherCert != nil && IsGatewaySecretNewerThanWatcherCert(gatewaySecret, watcherCert) { logf.FromContext(ctx).V(log.DebugLevel).Info("CA Certificate was rotated, removing certificate", "kyma", kymaObjKey) if err = c.removeSecret(ctx); err != nil { @@ -292,12 +292,10 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym return nil } -func isGatewaySecretNewerThanWatcherCert(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { - if gwSecretLastModifiedAtValue, ok := gatewaySecret.Annotations[gatewaysecret.LastModifiedAtAnnotation]; ok { - if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { - if watcherSecret.CreationTimestamp.Time.After(gwSecretLastModifiedAt) { - return false - } +func IsGatewaySecretNewerThanWatcherCert(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { + if gwSecretLastModifiedAt, err := gatewaysecret.GetLastModifiedAt(gatewaySecret); err == nil { + if watcherSecret.CreationTimestamp.Time.After(gwSecretLastModifiedAt) { + return false } } diff --git a/pkg/watcher/certificate_manager_test.go b/pkg/watcher/certificate_manager_test.go new file mode 100644 index 0000000000..50bfdf1d6c --- /dev/null +++ b/pkg/watcher/certificate_manager_test.go @@ -0,0 +1,107 @@ +package watcher_test + +import ( + "github.com/kyma-project/lifecycle-manager/pkg/watcher" + apicorev1 "k8s.io/api/core/v1" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "testing" + "time" +) + +func TestIsGatewaySecretNewerThanWatcherCert(t *testing.T) { + type args struct { + gatewaySecret *apicorev1.Secret + watcherSecret *apicorev1.Secret + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "Gateway secret is not newer than watcher cert", + args: args{ + gatewaySecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00Z", + }, + }, + }, + watcherSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + CreationTimestamp: apimetav1.Time{ + Time: time.Date(2024, 11, 0o1, 0, 0, 5, 0, time.UTC), + }, + }, + }, + }, + want: false, + }, + { + name: "Gateway secret is newer than watcher cert", + args: args{ + gatewaySecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:05Z", + }, + }, + }, + watcherSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + CreationTimestamp: apimetav1.Time{ + Time: time.Date(2024, 11, 0o1, 0, 0, 0, 0, time.UTC), + }, + }, + }, + }, + want: true, + }, + { + name: "Invalid lastModifiedAt annotation in gateway secret", + args: args{ + gatewaySecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + watcherSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + CreationTimestamp: apimetav1.Time{ + Time: time.Date(2024, 11, 0o1, 0, 0, 0, 0, time.UTC), + }, + }, + }, + }, + want: true, + }, + { + name: "Missing lastModifiedAt annotation in gateway secret", + args: args{ + gatewaySecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "invalid", + }, + }, + }, + watcherSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + CreationTimestamp: apimetav1.Time{ + Time: time.Date(2024, 11, 0o1, 0, 0, 0, 0, time.UTC), + }, + }, + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := watcher.IsGatewaySecretNewerThanWatcherCert(tt.args.gatewaySecret, tt.args.watcherSecret); got != tt.want { + t.Errorf("IsGatewaySecretNewerThanWatcherCert() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/watcher/skr_webhook_manifest_manager.go b/pkg/watcher/skr_webhook_manifest_manager.go index 1f41a6459a..4d18ffe4db 100644 --- a/pkg/watcher/skr_webhook_manifest_manager.go +++ b/pkg/watcher/skr_webhook_manifest_manager.go @@ -85,11 +85,10 @@ func (m *SKRWebhookManifestManager) Install(ctx context.Context, kyma *v1beta2.K return fmt.Errorf("failed to get skrContext: %w", err) } - _, err = getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ + if _, err = getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ Namespace: m.certificateConfig.IstioNamespace, Name: gatewaysecret.GatewaySecretName, - }) - if err != nil { + }); err != nil { return err } diff --git a/tests/e2e/ca_certificate_rotation_test.go b/tests/e2e/ca_certificate_rotation_test.go index 15b76d170f..e6b6f6c0d4 100644 --- a/tests/e2e/ca_certificate_rotation_test.go +++ b/tests/e2e/ca_certificate_rotation_test.go @@ -15,6 +15,8 @@ import ( . "github.com/kyma-project/lifecycle-manager/pkg/testutils" ) +const istioNamespace = "istio-system" + var _ = Describe("CA Certificate Rotation", Ordered, func() { kyma := NewKymaWithSyncLabel("kyma-sample", ControlPlaneNamespace, v1beta2.DefaultChannel) InitEmptyKymaBeforeAll(kyma) @@ -36,7 +38,7 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { var err error namespacedSecretName := types.NamespacedName{ Name: watcher.ResolveTLSCertName(kyma.Name), - Namespace: "istio-system", + Namespace: istioNamespace, } tlsSecret, err := GetTLSSecret(ctx, namespacedSecretName, kcpClient) Expect(err).NotTo(HaveOccurred()) @@ -50,7 +52,7 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { By("And new TLS Certificate is created") namespacedCertName := types.NamespacedName{ Name: caCertName, - Namespace: "istio-system", + Namespace: istioNamespace, } caCertificate, err = GetCACertificate(ctx, namespacedCertName, kcpClient) Expect(err).NotTo(HaveOccurred()) From b63ffe40a199ba341e710f03100d0eaf2244addc Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 14 Nov 2024 03:28:36 +0100 Subject: [PATCH 14/42] refactor: PR Review --- pkg/gatewaysecret/handler.go | 4 ++-- pkg/watcher/certificate_manager_test.go | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index afb2e45fd6..d942adb07d 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -6,8 +6,6 @@ import ( "fmt" "time" - "github.com/kyma-project/lifecycle-manager/pkg/util" - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" apicorev1 "k8s.io/api/core/v1" @@ -16,6 +14,8 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kyma-project/lifecycle-manager/pkg/util" ) const ( diff --git a/pkg/watcher/certificate_manager_test.go b/pkg/watcher/certificate_manager_test.go index 50bfdf1d6c..8ab7b86050 100644 --- a/pkg/watcher/certificate_manager_test.go +++ b/pkg/watcher/certificate_manager_test.go @@ -1,11 +1,12 @@ package watcher_test import ( + "testing" + "time" + "github.com/kyma-project/lifecycle-manager/pkg/watcher" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "testing" - "time" ) func TestIsGatewaySecretNewerThanWatcherCert(t *testing.T) { From b467d13baca756b520a7e84bb48b27491f22e438 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 14 Nov 2024 15:14:36 +0100 Subject: [PATCH 15/42] refactor: gci --- pkg/watcher/certificate_manager_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/watcher/certificate_manager_test.go b/pkg/watcher/certificate_manager_test.go index 8ab7b86050..8258faff27 100644 --- a/pkg/watcher/certificate_manager_test.go +++ b/pkg/watcher/certificate_manager_test.go @@ -4,9 +4,10 @@ import ( "testing" "time" - "github.com/kyma-project/lifecycle-manager/pkg/watcher" apicorev1 "k8s.io/api/core/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kyma-project/lifecycle-manager/pkg/watcher" ) func TestIsGatewaySecretNewerThanWatcherCert(t *testing.T) { From 333fd098b9eb2ebbd163600b6918da8857fc6809 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 14 Nov 2024 16:45:51 +0100 Subject: [PATCH 16/42] refactor: hide gateway secret name --- pkg/gatewaysecret/handler.go | 31 +++++++-------- pkg/watcher/certificate_manager.go | 22 +++++++---- pkg/watcher/skr_webhook_manifest_manager.go | 38 +++++++------------ .../watcher_controller_helper_test.go | 2 +- 4 files changed, 42 insertions(+), 51 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index d942adb07d..fb4b605fee 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -20,7 +20,7 @@ import ( const ( LastModifiedAtAnnotation = "lastModifiedAt" - GatewaySecretName = "klm-istio-gateway" //nolint:gosec // GatewaySecretName is not a credential + gatewaySecretName = "klm-istio-gateway" //nolint:gosec // gatewaySecretName is not a credential kcpRootSecretName = "klm-watcher" kcpCACertName = "klm-watcher-serving" istioNamespace = "istio-system" @@ -83,7 +83,7 @@ func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) APIVersion: apicorev1.SchemeGroupVersion.String(), }, ObjectMeta: apimetav1.ObjectMeta{ - Name: GatewaySecretName, + Name: gatewaySecretName, Namespace: istioNamespace, }, Data: map[string][]byte{ @@ -96,21 +96,7 @@ func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) } func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) { - return gsh.findSecret(context.Background(), client.ObjectKey{ - Name: GatewaySecretName, - Namespace: istioNamespace, - }) -} - -func (gsh *GatewaySecretHandler) findSecret(ctx context.Context, objKey client.ObjectKey) (*apicorev1.Secret, error) { - secret := &apicorev1.Secret{} - - err := gsh.kcpClient.Get(ctx, objKey, secret) - if err != nil { - return nil, fmt.Errorf("failed to get secret %s: %w", objKey.Name, err) - } - - return secret, nil + return GetGatewaySecret(context.Background(), gsh.kcpClient) } func (gsh *GatewaySecretHandler) create(ctx context.Context, secret *apicorev1.Secret) error { @@ -136,6 +122,17 @@ func (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) } +func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + if err := clnt.Get(ctx, client.ObjectKey{ + Name: gatewaySecretName, + Namespace: istioNamespace, + }, secret); err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) + } + return secret, nil +} + func GetLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { if gwSecretLastModifiedAtValue, ok := secret.Annotations[LastModifiedAtAnnotation]; ok { if gwSecretLastModifiedAt, err := time.Parse(time.RFC3339, gwSecretLastModifiedAtValue); err == nil { diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index 0b4e3c9cf5..3f60fb8159 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -259,6 +259,18 @@ func (c *CertificateManager) getIssuer(ctx context.Context) (*certmanagerv1.Issu return &issuerList.Items[0], nil } +func (c *CertificateManager) getCertificateSecret(ctx context.Context) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + err := c.kcpClient.Get(ctx, client.ObjectKey{Name: c.secretName, Namespace: c.config.IstioNamespace}, + secret) + if err != nil { + return nil, fmt.Errorf("failed to get secret for certificate %s-%s: %w", c.secretName, c.config.IstioNamespace, + err) + } + + return secret, nil +} + type CertificateNotReadyError struct{} func (e *CertificateNotReadyError) Error() string { @@ -266,17 +278,11 @@ func (e *CertificateNotReadyError) Error() string { } func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kymaObjKey client.ObjectKey) error { - gatewaySecret, err := getCertificateSecret(ctx, c.kcpClient, client.ObjectKey{ - Namespace: c.config.IstioNamespace, - Name: gatewaysecret.GatewaySecretName, - }) + gatewaySecret, err := gatewaysecret.GetGatewaySecret(ctx, c.kcpClient) if err != nil { return err } - watcherCert, err := getCertificateSecret(ctx, c.kcpClient, client.ObjectKey{ - Namespace: c.config.IstioNamespace, - Name: c.secretName, - }) + watcherCert, err := c.getCertificateSecret(ctx) if err != nil { return err } diff --git a/pkg/watcher/skr_webhook_manifest_manager.go b/pkg/watcher/skr_webhook_manifest_manager.go index 4d18ffe4db..e5ec48aa51 100644 --- a/pkg/watcher/skr_webhook_manifest_manager.go +++ b/pkg/watcher/skr_webhook_manifest_manager.go @@ -4,12 +4,12 @@ import ( "context" "errors" "fmt" + apicorev1 "k8s.io/api/core/v1" "os" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" - apicorev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -85,10 +85,7 @@ func (m *SKRWebhookManifestManager) Install(ctx context.Context, kyma *v1beta2.K return fmt.Errorf("failed to get skrContext: %w", err) } - if _, err = getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ - Namespace: m.certificateConfig.IstioNamespace, - Name: gatewaysecret.GatewaySecretName, - }); err != nil { + if _, err = gatewaysecret.GetGatewaySecret(ctx, m.kcpClient); err != nil { return err } @@ -222,20 +219,22 @@ func (m *SKRWebhookManifestManager) getRawManifestClientObjects(cfg *unstructure func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.Context, kymaObjKey client.ObjectKey, remoteNs string, ) (*unstructuredResourcesConfig, error) { - tlsSecret, err := getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ + tlsSecret := &apicorev1.Secret{} + certObjKey := client.ObjectKey{ Namespace: m.certificateConfig.IstioNamespace, Name: ResolveTLSCertName(kymaObjKey.Name), - }) - if err != nil { - return nil, err } - gatewaySecret, err := getCertificateSecret(ctx, m.kcpClient, client.ObjectKey{ - Namespace: m.certificateConfig.IstioNamespace, - Name: gatewaysecret.GatewaySecretName, - }) + if err := m.kcpClient.Get(ctx, certObjKey, tlsSecret); err != nil { + if util.IsNotFound(err) { + return nil, &CertificateNotReadyError{} + } + return nil, fmt.Errorf("error fetching TLS secret: %w", err) + } + + gatewaySecret, err := gatewaysecret.GetGatewaySecret(ctx, m.kcpClient) if err != nil { - return nil, err + return nil, fmt.Errorf("error fetching gateway secret: %w", err) } return &unstructuredResourcesConfig{ @@ -252,17 +251,6 @@ func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.C }, nil } -func getCertificateSecret(ctx context.Context, clnt client.Client, objKey client.ObjectKey) (*apicorev1.Secret, error) { - certificateSecret := &apicorev1.Secret{} - if err := clnt.Get(ctx, objKey, certificateSecret); err != nil { - if util.IsNotFound(err) { - return nil, &CertificateNotReadyError{} - } - return nil, fmt.Errorf("error fetching TLS secret: %w", err) - } - return certificateSecret, nil -} - func (m *SKRWebhookManifestManager) getBaseClientObjects() []client.Object { if len(m.baseResources) == 0 { return nil diff --git a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go index 624bf3d079..92b9d53555 100644 --- a/tests/integration/controller/withwatcher/watcher_controller_helper_test.go +++ b/tests/integration/controller/withwatcher/watcher_controller_helper_test.go @@ -188,7 +188,7 @@ func createWatcherSecret(kymaObjKey client.ObjectKey) *apicorev1.Secret { func createGatewaySecret() *apicorev1.Secret { return &apicorev1.Secret{ ObjectMeta: apimetav1.ObjectMeta{ - Name: gatewaysecret.GatewaySecretName, + Name: "klm-istio-gateway", Namespace: istioSystemNs, Annotations: map[string]string{ gatewaysecret.LastModifiedAtAnnotation: apimetav1.Now().Add(-1 * time.Hour).Format(time.RFC3339), From f7fc41c7f7cdc0750dce43b6928205a91cb7159b Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 15 Nov 2024 14:43:57 +0100 Subject: [PATCH 17/42] test: add e2e test for istio gateway secret rotation --- .../deploy-lifecycle-manager-e2e/action.yml | 3 +- .../test-e2e-with-modulereleasemeta.yml | 1 + .github/workflows/test-e2e.yml | 1 + cmd/main.go | 8 +-- internal/pkg/flags/flags.go | 12 ++-- internal/pkg/flags/flags_test.go | 8 +-- pkg/gatewaysecret/handler.go | 9 +-- pkg/testutils/watcher.go | 60 +++++++++++++++++-- pkg/watcher/certificate_manager.go | 2 +- tests/e2e/Makefile | 3 + tests/e2e/ca_certificate_rotation_test.go | 4 +- .../e2e/istio_gateway_secret_rotation_test.go | 55 +++++++++++++++++ tests/e2e/suite_test.go | 1 + 13 files changed, 138 insertions(+), 29 deletions(-) create mode 100644 tests/e2e/istio_gateway_secret_rotation_test.go diff --git a/.github/actions/deploy-lifecycle-manager-e2e/action.yml b/.github/actions/deploy-lifecycle-manager-e2e/action.yml index 6d3debb57b..c5a5535f50 100644 --- a/.github/actions/deploy-lifecycle-manager-e2e/action.yml +++ b/.github/actions/deploy-lifecycle-manager-e2e/action.yml @@ -81,7 +81,8 @@ runs: kustomize edit add patch --path requeue-interval-patch.yaml --kind Deployment popd - name: Patch CA certificate renewBefore - if: ${{matrix.e2e-test == 'ca-certificate-rotation'}} + if: ${{matrix.e2e-test == 'ca-certificate-rotation' || + matrix.e2e-test == 'istio-gateway-secret-rotation'}} working-directory: lifecycle-manager shell: bash run: | diff --git a/.github/workflows/test-e2e-with-modulereleasemeta.yml b/.github/workflows/test-e2e-with-modulereleasemeta.yml index 96c1ce7055..3d3bdf7fbf 100644 --- a/.github/workflows/test-e2e-with-modulereleasemeta.yml +++ b/.github/workflows/test-e2e-with-modulereleasemeta.yml @@ -49,6 +49,7 @@ jobs: - unmanage-module - skip-manifest-reconciliation - ca-certificate-rotation + - istio-gateway-secret-rotation - self-signed-certificate-rotation - mandatory-module - mandatory-module-metrics diff --git a/.github/workflows/test-e2e.yml b/.github/workflows/test-e2e.yml index d1d9fe0d29..6d11ae881a 100644 --- a/.github/workflows/test-e2e.yml +++ b/.github/workflows/test-e2e.yml @@ -50,6 +50,7 @@ jobs: - module-install-by-version - skip-manifest-reconciliation - ca-certificate-rotation + - istio-gateway-secret-rotation - self-signed-certificate-rotation - mandatory-module - mandatory-module-metrics diff --git a/cmd/main.go b/cmd/main.go index f17bba73e4..d333e24bdf 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -115,7 +115,7 @@ func main() { } cacheOptions := internal.GetCacheOptions(flagVar.IsKymaManaged, flagVar.IstioNamespace, - flagVar.IstioGatewayNamespace, flagVar.RemoteSyncNamespace) + flagVar.RootCASecretNamespace, flagVar.RemoteSyncNamespace) setupManager(flagVar, cacheOptions, scheme, setupLog) } @@ -348,8 +348,8 @@ func createSkrWebhookManager(mgr ctrl.Manager, skrContextFactory remote.SkrConte KeySize: flagVar.SelfSignedCertKeySize, } gatewayConfig := watcher.GatewayConfig{ - IstioGatewayName: flagVar.IstioGatewayName, - IstioGatewayNamespace: flagVar.IstioGatewayNamespace, + IstioGatewayName: flagVar.RootCASecretName, + IstioGatewayNamespace: flagVar.RootCASecretNamespace, LocalGatewayPortOverwrite: flagVar.ListenerPortOverwrite, } @@ -433,7 +433,7 @@ func setupKcpWatcherReconciler(mgr ctrl.Manager, options ctrlruntime.Options, ev Error: flags.DefaultKymaRequeueErrInterval, Warning: flags.DefaultKymaRequeueWarningInterval, }, - IstioGatewayNamespace: flagVar.IstioGatewayNamespace, + IstioGatewayNamespace: flagVar.RootCASecretNamespace, }).SetupWithManager(mgr, options); err != nil { setupLog.Error(err, "unable to create watcher controller") os.Exit(bootstrapFailedExitCode) diff --git a/internal/pkg/flags/flags.go b/internal/pkg/flags/flags.go index 9896d38c04..e1c0527351 100644 --- a/internal/pkg/flags/flags.go +++ b/internal/pkg/flags/flags.go @@ -39,8 +39,8 @@ const ( DefaultMaxConcurrentWatcherReconciles = 1 DefaultMaxConcurrentMandatoryModuleReconciles = 1 DefaultMaxConcurrentMandatoryModuleDeletionReconciles = 1 - DefaultIstioGatewayName = "klm-watcher" - DefaultIstioGatewayNamespace = "kcp-system" + DefaultRootCASecretName = "klm-watcher" + DefaultRootCASecretNamespace = "istio-system" DefaultIstioNamespace = "istio-system" DefaultCaCertName = "klm-watcher-serving" DefaultSelfSignedCertDuration time.Duration = 90 * 24 * time.Hour @@ -164,9 +164,9 @@ func DefineFlagVar() *FlagVar { "comma-separated list, for example \"--additional-dns-names=localhost,127.0.0.1,host.k3d.internal\".") flag.StringVar(&flagVar.IstioNamespace, "istio-namespace", DefaultIstioNamespace, "Cluster Resource Namespace of Istio") - flag.StringVar(&flagVar.IstioGatewayName, "istio-gateway-name", DefaultIstioGatewayName, + flag.StringVar(&flagVar.RootCASecretName, "root-ca-secret-name", DefaultRootCASecretName, "Cluster Resource Name of Istio Gateway") - flag.StringVar(&flagVar.IstioGatewayNamespace, "istio-gateway-namespace", DefaultIstioGatewayNamespace, + flag.StringVar(&flagVar.RootCASecretNamespace, "root-ca-secret-namespace", DefaultRootCASecretNamespace, "Cluster Resource Namespace of Istio Gateway") flag.StringVar(&flagVar.ListenerPortOverwrite, "listener-port-overwrite", "", "Port that is mapped to HTTP port of the local k3d cluster using --port 9443:443@loadbalancer when "+ @@ -265,8 +265,8 @@ type FlagVar struct { ClientQPS float64 ClientBurst int IstioNamespace string - IstioGatewayName string - IstioGatewayNamespace string + RootCASecretName string + RootCASecretNamespace string AdditionalDNSNames string // ListenerPortOverwrite is used to enable the user to overwrite the port // used to expose the KCP cluster for the watcher. By default, it will be diff --git a/internal/pkg/flags/flags_test.go b/internal/pkg/flags/flags_test.go index fbe6aba7c4..1f3eccbf6c 100644 --- a/internal/pkg/flags/flags_test.go +++ b/internal/pkg/flags/flags_test.go @@ -156,13 +156,13 @@ func Test_ConstantFlags(t *testing.T) { expectedValue: "kyma-system", }, { - constName: "DefaultIstioGatewayName", - constValue: DefaultIstioGatewayName, + constName: "DefaultRootCASecretName", + constValue: DefaultRootCASecretName, expectedValue: "klm-watcher", }, { - constName: "DefaultIstioGatewayNamespace", - constValue: DefaultIstioGatewayNamespace, + constName: "DefaultRootCASecretNamespace", + constValue: DefaultRootCASecretNamespace, expectedValue: "kcp-system", }, { diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index fb4b605fee..d11b1843b4 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -21,7 +22,6 @@ import ( const ( LastModifiedAtAnnotation = "lastModifiedAt" gatewaySecretName = "klm-istio-gateway" //nolint:gosec // gatewaySecretName is not a credential - kcpRootSecretName = "klm-watcher" kcpCACertName = "klm-watcher-serving" istioNamespace = "istio-system" ) @@ -64,7 +64,7 @@ func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gw return fmt.Errorf("failed to get CA certificate: %w", err) } - if gwSecretLastModifiedAt, err := GetLastModifiedAt(gwSecret); err == nil { + if gwSecretLastModifiedAt, err := GetValidLastModifiedAt(gwSecret); err == nil { if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { return nil } @@ -120,6 +120,7 @@ func (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) secret.Annotations = make(map[string]string) } secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) + fmt.Printf("Updated lastModifiedAt annotation to %s\n", secret.Annotations[LastModifiedAtAnnotation]) } func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { @@ -133,7 +134,7 @@ func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secre return secret, nil } -func GetLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { +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 { return gwSecretLastModifiedAt, nil @@ -146,7 +147,7 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre log logr.Logger, ) { secretWatch, err := clientset.CoreV1().Secrets(istioNamespace).Watch(context.Background(), apimetav1.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), + FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, flags.DefaultRootCASecretName).String(), }) if err != nil { log.Error(err, "unable to start watching root certificate") diff --git a/pkg/testutils/watcher.go b/pkg/testutils/watcher.go index 2d7b4da834..f1ff716327 100644 --- a/pkg/testutils/watcher.go +++ b/pkg/testutils/watcher.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -15,9 +16,10 @@ import ( ) var ( - errOldCreationTime = errors.New("certificate has an old creation timestamp") - errNotSyncedSecret = errors.New("secret is not synced to skr cluster") - errTLSSecretNotRotated = errors.New("tls secret did not rotated") + errOldCreationTime = errors.New("certificate has an old creation timestamp") + errNotSyncedSecret = errors.New("secrets are not synced") + errTLSSecretNotRotated = errors.New("tls secret did not rotated") + errCreationTimeNotUpdated = errors.New("gateway secret has an old creation timestamp") ) func CertificateSecretExists(ctx context.Context, secretName types.NamespacedName, k8sClient client.Client) error { @@ -72,12 +74,41 @@ func CertificateSecretIsSyncedToSkrCluster(ctx context.Context, return fmt.Errorf("failed to fetch kcp certificate secret %w", err) } - for k, d := range kcpCertificateSecret.Data { - if !bytes.Equal(d, skrCertificateSecret.Data[k]) { + err = verifySecretsHaveSameData(kcpCertificateSecret, skrCertificateSecret) + if err != nil { + return err + } + + return nil +} + +func IstioGatewaySecretIsSyncedToRootCA(ctx context.Context, + rootCASecretName types.NamespacedName, kcpClient client.Client, +) error { + rootCASecret, err := fetchCertificateSecret(ctx, rootCASecretName, kcpClient) + if err != nil { + return fmt.Errorf("failed to fetch root CA secret: %w", err) + } + + gatewaySecret, err := gatewaysecret.GetGatewaySecret(ctx, kcpClient) + if err != nil { + return fmt.Errorf("failed to fetch istio gateway secret: %w", err) + } + + err = verifySecretsHaveSameData(rootCASecret, gatewaySecret) + if err != nil { + return err + } + + return nil +} + +func verifySecretsHaveSameData(secretA *apicorev1.Secret, secretB *apicorev1.Secret) error { + for k, d := range secretA.Data { + if !bytes.Equal(d, secretB.Data[k]) { return errNotSyncedSecret } } - return nil } @@ -129,3 +160,20 @@ func GetTLSSecret(ctx context.Context, namespacedSecretName types.NamespacedName return tlsSecret, nil } + +func GatewaySecretCreationTimeIsUpdated(ctx context.Context, oldTime time.Time, kcpClient client.Client) error { + gwSecret, err := gatewaysecret.GetGatewaySecret(ctx, kcpClient) + if err != nil { + return fmt.Errorf("failed to get gateway secret %w", err) + } + + currentTime, err := gatewaysecret.GetValidLastModifiedAt(gwSecret) + if err != nil { + return fmt.Errorf("failed to get last modified time %w", err) + } + + if currentTime != oldTime { + return nil + } + return errCreationTimeNotUpdated +} diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index 3f60fb8159..a570d25f21 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -299,7 +299,7 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym } func IsGatewaySecretNewerThanWatcherCert(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { - if gwSecretLastModifiedAt, err := gatewaysecret.GetLastModifiedAt(gatewaySecret); err == nil { + if gwSecretLastModifiedAt, err := gatewaysecret.GetValidLastModifiedAt(gatewaySecret); err == nil { if watcherSecret.CreationTimestamp.Time.After(gwSecretLastModifiedAt) { return false } diff --git a/tests/e2e/Makefile b/tests/e2e/Makefile index 171a33836a..f1afeff6c1 100644 --- a/tests/e2e/Makefile +++ b/tests/e2e/Makefile @@ -149,6 +149,9 @@ module-install-by-version: ca-certificate-rotation: go test -timeout 20m -ginkgo.v -ginkgo.focus "CA Certificate Rotation" +istio-gateway-secret-rotation: + go test -timeout 20m -ginkgo.v -ginkgo.focus "Istio Gateway Secret Rotation" + self-signed-certificate-rotation: go test -timeout 20m -ginkgo.v -ginkgo.focus "Self Signed Certificate Rotation" diff --git a/tests/e2e/ca_certificate_rotation_test.go b/tests/e2e/ca_certificate_rotation_test.go index e6b6f6c0d4..b89db5eda3 100644 --- a/tests/e2e/ca_certificate_rotation_test.go +++ b/tests/e2e/ca_certificate_rotation_test.go @@ -15,8 +15,6 @@ import ( . "github.com/kyma-project/lifecycle-manager/pkg/testutils" ) -const istioNamespace = "istio-system" - var _ = Describe("CA Certificate Rotation", Ordered, func() { kyma := NewKymaWithSyncLabel("kyma-sample", ControlPlaneNamespace, v1beta2.DefaultChannel) InitEmptyKymaBeforeAll(kyma) @@ -28,7 +26,7 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { Context("Given KCP Cluster and rotated CA certificate", func() { kcpSecretName := types.NamespacedName{ Name: kyma.Name + "-webhook-tls", - Namespace: "istio-system", + Namespace: istioNamespace, } skrSecretName := types.NamespacedName{ Name: watcher.SkrTLSName, diff --git a/tests/e2e/istio_gateway_secret_rotation_test.go b/tests/e2e/istio_gateway_secret_rotation_test.go new file mode 100644 index 0000000000..9c25c7ca3a --- /dev/null +++ b/tests/e2e/istio_gateway_secret_rotation_test.go @@ -0,0 +1,55 @@ +package e2e_test + +import ( + "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" + "time" + + "k8s.io/apimachinery/pkg/types" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + . "github.com/kyma-project/lifecycle-manager/pkg/testutils" +) + +var _ = Describe("Istio Gateway Secret Rotation", Ordered, func() { + kyma := NewKymaWithSyncLabel("kyma-sample", ControlPlaneNamespace, v1beta2.DefaultChannel) + InitEmptyKymaBeforeAll(kyma) + CleanupKymaAfterAll(kyma) + + Context("Given KCP Cluster, rotated CA certificate, and Istio Gateway Secret", func() { + It("Then Istio Gateway Secret is a copy of CA Certificate", func() { + namespacedRootCASecretName := types.NamespacedName{ + Name: flags.DefaultRootCASecretName, + Namespace: istioNamespace, + } + + // The timeout used is 4 minutes bec the certificate gets rotated every 1 minute + Eventually(IstioGatewaySecretIsSyncedToRootCA, 4*time.Minute). + WithContext(ctx). + WithArguments(namespacedRootCASecretName, kcpClient). + Should(Succeed()) + + By("And LastModifiedAt timestamp is valid") + gwSecret, err := gatewaysecret.GetGatewaySecret(ctx, kcpClient) + Expect(err).NotTo(HaveOccurred()) + lastModifiedAtTime, err := gatewaysecret.GetValidLastModifiedAt(gwSecret) + Expect(err).To(Succeed()) + + By("And LastModifiedAt timestamp is updated") + Eventually(GatewaySecretCreationTimeIsUpdated, 4*time.Minute). + WithContext(ctx). + WithArguments(lastModifiedAtTime, kcpClient). + Should(Succeed()) + + By("And new Istio Gateway Secret is also a copy of CA Certificate") + Eventually(IstioGatewaySecretIsSyncedToRootCA, 4*time.Minute). + WithContext(ctx). + WithArguments(namespacedRootCASecretName, kcpClient). + Should(Succeed()) + }) + }) +}) diff --git a/tests/e2e/suite_test.go b/tests/e2e/suite_test.go index 5fc03e01e9..dd2aa7ce88 100644 --- a/tests/e2e/suite_test.go +++ b/tests/e2e/suite_test.go @@ -32,6 +32,7 @@ import ( const ( kcpConfigEnvVar, skrConfigEnvVar = "KCP_KUBECONFIG", "SKR_KUBECONFIG" clientQPS, clientBurst = 1000, 2000 + istioNamespace = "istio-system" ) var errEmptyEnvVar = errors.New("environment variable is empty") From ce69e8ce0148e4b8e320100299d85de7fb5d5cf4 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 15 Nov 2024 14:49:02 +0100 Subject: [PATCH 18/42] refactor: gci and gofumpt --- pkg/gatewaysecret/handler.go | 2 +- pkg/testutils/watcher.go | 3 ++- pkg/watcher/skr_webhook_manifest_manager.go | 2 +- tests/e2e/istio_gateway_secret_rotation_test.go | 7 +++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index d11b1843b4..3c41ede979 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -16,6 +15,7 @@ import ( "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "github.com/kyma-project/lifecycle-manager/pkg/util" ) diff --git a/pkg/testutils/watcher.go b/pkg/testutils/watcher.go index f1ff716327..13f8067307 100644 --- a/pkg/testutils/watcher.go +++ b/pkg/testutils/watcher.go @@ -5,7 +5,6 @@ import ( "context" "errors" "fmt" - "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -13,6 +12,8 @@ import ( apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" ) var ( diff --git a/pkg/watcher/skr_webhook_manifest_manager.go b/pkg/watcher/skr_webhook_manifest_manager.go index e5ec48aa51..867b2b9293 100644 --- a/pkg/watcher/skr_webhook_manifest_manager.go +++ b/pkg/watcher/skr_webhook_manifest_manager.go @@ -4,12 +4,12 @@ import ( "context" "errors" "fmt" - apicorev1 "k8s.io/api/core/v1" "os" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/go-logr/logr" + apicorev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" diff --git a/tests/e2e/istio_gateway_secret_rotation_test.go b/tests/e2e/istio_gateway_secret_rotation_test.go index 9c25c7ca3a..d176e0f283 100644 --- a/tests/e2e/istio_gateway_secret_rotation_test.go +++ b/tests/e2e/istio_gateway_secret_rotation_test.go @@ -1,18 +1,17 @@ package e2e_test import ( - "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" - "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" "time" "k8s.io/apimachinery/pkg/types" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" + . "github.com/kyma-project/lifecycle-manager/pkg/testutils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - - . "github.com/kyma-project/lifecycle-manager/pkg/testutils" ) var _ = Describe("Istio Gateway Secret Rotation", Ordered, func() { From 5e71352a6c954accf767947291f498917d352376 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 15 Nov 2024 14:55:04 +0100 Subject: [PATCH 19/42] fix: flags_test.go --- internal/pkg/flags/flags_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/flags/flags_test.go b/internal/pkg/flags/flags_test.go index 1f3eccbf6c..949156cb15 100644 --- a/internal/pkg/flags/flags_test.go +++ b/internal/pkg/flags/flags_test.go @@ -163,7 +163,7 @@ func Test_ConstantFlags(t *testing.T) { { constName: "DefaultRootCASecretNamespace", constValue: DefaultRootCASecretNamespace, - expectedValue: "kcp-system", + expectedValue: "istio-system", }, { constName: "DefaultIstioNamespace", From 11779fed5184bd207213604736993728f18b623f Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 15 Nov 2024 15:01:53 +0100 Subject: [PATCH 20/42] chore: remove debug print statement --- pkg/gatewaysecret/handler.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 3c41ede979..133e93bbef 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -120,7 +120,6 @@ func (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) secret.Annotations = make(map[string]string) } secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) - fmt.Printf("Updated lastModifiedAt annotation to %s\n", secret.Annotations[LastModifiedAtAnnotation]) } func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { From 34363c6a33e4883bfcaf07c4a020efa26d87e6f5 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 15 Nov 2024 15:16:22 +0100 Subject: [PATCH 21/42] fix: remove background context --- pkg/gatewaysecret/handler.go | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 133e93bbef..553cefbdb6 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -38,27 +38,27 @@ func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { } } -func (gsh *GatewaySecretHandler) ManageGatewaySecret(rootSecret *apicorev1.Secret) error { - gwSecret, err := gsh.findGatewaySecret() +func (gsh *GatewaySecretHandler) ManageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { + gwSecret, err := gsh.findGatewaySecret(ctx) if util.IsNotFound(err) { - return gsh.handleNonExisting(rootSecret) + return gsh.handleNonExisting(ctx, rootSecret) } if err != nil { return err } - return gsh.handleExisting(rootSecret, gwSecret) + return gsh.handleExisting(ctx, rootSecret, gwSecret) } -func (gsh *GatewaySecretHandler) handleNonExisting(rootSecret *apicorev1.Secret) error { +func (gsh *GatewaySecretHandler) handleNonExisting(ctx context.Context, rootSecret *apicorev1.Secret) error { gwSecret := gsh.newGatewaySecret(rootSecret) - return gsh.create(context.Background(), gwSecret) + return gsh.create(ctx, gwSecret) } -func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret) error { +func (gsh *GatewaySecretHandler) handleExisting(ctx context.Context, rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret) error { caCert := certmanagerv1.Certificate{} - if err := gsh.kcpClient.Get(context.Background(), + if err := gsh.kcpClient.Get(ctx, client.ObjectKey{Namespace: istioNamespace, Name: kcpCACertName}, &caCert); err != nil { return fmt.Errorf("failed to get CA certificate: %w", err) @@ -73,7 +73,7 @@ func (gsh *GatewaySecretHandler) handleExisting(rootSecret *apicorev1.Secret, gw gwSecret.Data["tls.crt"] = rootSecret.Data["tls.crt"] gwSecret.Data["tls.key"] = rootSecret.Data["tls.key"] gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] - return gsh.update(context.Background(), gwSecret) + return gsh.update(ctx, gwSecret) } func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { @@ -95,8 +95,8 @@ func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) return gwSecret } -func (gsh *GatewaySecretHandler) findGatewaySecret() (*apicorev1.Secret, error) { - return GetGatewaySecret(context.Background(), gsh.kcpClient) +func (gsh *GatewaySecretHandler) findGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { + return GetGatewaySecret(ctx, gsh.kcpClient) } func (gsh *GatewaySecretHandler) create(ctx context.Context, secret *apicorev1.Secret) error { @@ -145,7 +145,10 @@ func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, log logr.Logger, ) { - secretWatch, err := clientset.CoreV1().Secrets(istioNamespace).Watch(context.Background(), apimetav1.ListOptions{ + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + secretWatch, err := clientset.CoreV1().Secrets(istioNamespace).Watch(ctx, apimetav1.ListOptions{ FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, flags.DefaultRootCASecretName).String(), }) if err != nil { @@ -160,7 +163,7 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre case watch.Added: fallthrough case watch.Modified: - err := gatewaySecretHandler.ManageGatewaySecret(rootCASecret) + err := gatewaySecretHandler.ManageGatewaySecret(ctx, rootCASecret) if err != nil { log.Error(err, "unable to manage istio gateway secret") } From 55fe04babd218dd2b23c2b04f12085c3f6d30ac3 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 18 Nov 2024 15:10:39 +0100 Subject: [PATCH 22/42] fix: test logics --- pkg/testutils/watcher.go | 2 +- pkg/watcher/certificate_manager_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/testutils/watcher.go b/pkg/testutils/watcher.go index 13f8067307..bb4ed55126 100644 --- a/pkg/testutils/watcher.go +++ b/pkg/testutils/watcher.go @@ -173,7 +173,7 @@ func GatewaySecretCreationTimeIsUpdated(ctx context.Context, oldTime time.Time, return fmt.Errorf("failed to get last modified time %w", err) } - if currentTime != oldTime { + if currentTime.After(oldTime) { return nil } return errCreationTimeNotUpdated diff --git a/pkg/watcher/certificate_manager_test.go b/pkg/watcher/certificate_manager_test.go index 8258faff27..0437866b5b 100644 --- a/pkg/watcher/certificate_manager_test.go +++ b/pkg/watcher/certificate_manager_test.go @@ -33,7 +33,7 @@ func TestIsGatewaySecretNewerThanWatcherCert(t *testing.T) { watcherSecret: &apicorev1.Secret{ ObjectMeta: apimetav1.ObjectMeta{ CreationTimestamp: apimetav1.Time{ - Time: time.Date(2024, 11, 0o1, 0, 0, 5, 0, time.UTC), + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), }, }, }, From 45691cbd3c1247abfdb4c04d8882cb5269d0c088 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 18 Nov 2024 15:22:11 +0100 Subject: [PATCH 23/42] refactor: go routine style in main --- cmd/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index d333e24bdf..acd4d312b3 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -204,7 +204,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog) go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog) - setupIstioGatewaySecretRotation(config, kcpClient, setupLog) + go setupIstioGatewaySecretRotation(config, kcpClient, setupLog) if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") @@ -216,7 +216,7 @@ func setupIstioGatewaySecretRotation(config *rest.Config, kcpClient *remote.Conf kcpClientset := kubernetes.NewForConfigOrDie(config) gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient) - go gatewaysecret.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + gatewaysecret.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { From b260cbf0d63431db40a0611c0f1a3a70a7d6e415 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 18 Nov 2024 18:16:52 +0100 Subject: [PATCH 24/42] fix: KLM health check --- cmd/main.go | 8 ++++---- internal/pkg/flags/flags.go | 12 ++++++------ internal/pkg/flags/flags_test.go | 8 ++++---- pkg/gatewaysecret/handler.go | 4 ++-- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index acd4d312b3..1cb8d9a891 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -115,7 +115,7 @@ func main() { } cacheOptions := internal.GetCacheOptions(flagVar.IsKymaManaged, flagVar.IstioNamespace, - flagVar.RootCASecretNamespace, flagVar.RemoteSyncNamespace) + flagVar.IstioGatewayNamespace, flagVar.RemoteSyncNamespace) setupManager(flagVar, cacheOptions, scheme, setupLog) } @@ -348,8 +348,8 @@ func createSkrWebhookManager(mgr ctrl.Manager, skrContextFactory remote.SkrConte KeySize: flagVar.SelfSignedCertKeySize, } gatewayConfig := watcher.GatewayConfig{ - IstioGatewayName: flagVar.RootCASecretName, - IstioGatewayNamespace: flagVar.RootCASecretNamespace, + IstioGatewayName: flagVar.IstioGatewayName, + IstioGatewayNamespace: flagVar.IstioGatewayNamespace, LocalGatewayPortOverwrite: flagVar.ListenerPortOverwrite, } @@ -433,7 +433,7 @@ func setupKcpWatcherReconciler(mgr ctrl.Manager, options ctrlruntime.Options, ev Error: flags.DefaultKymaRequeueErrInterval, Warning: flags.DefaultKymaRequeueWarningInterval, }, - IstioGatewayNamespace: flagVar.RootCASecretNamespace, + IstioGatewayNamespace: flagVar.IstioGatewayNamespace, }).SetupWithManager(mgr, options); err != nil { setupLog.Error(err, "unable to create watcher controller") os.Exit(bootstrapFailedExitCode) diff --git a/internal/pkg/flags/flags.go b/internal/pkg/flags/flags.go index e1c0527351..9896d38c04 100644 --- a/internal/pkg/flags/flags.go +++ b/internal/pkg/flags/flags.go @@ -39,8 +39,8 @@ const ( DefaultMaxConcurrentWatcherReconciles = 1 DefaultMaxConcurrentMandatoryModuleReconciles = 1 DefaultMaxConcurrentMandatoryModuleDeletionReconciles = 1 - DefaultRootCASecretName = "klm-watcher" - DefaultRootCASecretNamespace = "istio-system" + DefaultIstioGatewayName = "klm-watcher" + DefaultIstioGatewayNamespace = "kcp-system" DefaultIstioNamespace = "istio-system" DefaultCaCertName = "klm-watcher-serving" DefaultSelfSignedCertDuration time.Duration = 90 * 24 * time.Hour @@ -164,9 +164,9 @@ func DefineFlagVar() *FlagVar { "comma-separated list, for example \"--additional-dns-names=localhost,127.0.0.1,host.k3d.internal\".") flag.StringVar(&flagVar.IstioNamespace, "istio-namespace", DefaultIstioNamespace, "Cluster Resource Namespace of Istio") - flag.StringVar(&flagVar.RootCASecretName, "root-ca-secret-name", DefaultRootCASecretName, + flag.StringVar(&flagVar.IstioGatewayName, "istio-gateway-name", DefaultIstioGatewayName, "Cluster Resource Name of Istio Gateway") - flag.StringVar(&flagVar.RootCASecretNamespace, "root-ca-secret-namespace", DefaultRootCASecretNamespace, + flag.StringVar(&flagVar.IstioGatewayNamespace, "istio-gateway-namespace", DefaultIstioGatewayNamespace, "Cluster Resource Namespace of Istio Gateway") flag.StringVar(&flagVar.ListenerPortOverwrite, "listener-port-overwrite", "", "Port that is mapped to HTTP port of the local k3d cluster using --port 9443:443@loadbalancer when "+ @@ -265,8 +265,8 @@ type FlagVar struct { ClientQPS float64 ClientBurst int IstioNamespace string - RootCASecretName string - RootCASecretNamespace string + IstioGatewayName string + IstioGatewayNamespace string AdditionalDNSNames string // ListenerPortOverwrite is used to enable the user to overwrite the port // used to expose the KCP cluster for the watcher. By default, it will be diff --git a/internal/pkg/flags/flags_test.go b/internal/pkg/flags/flags_test.go index 949156cb15..5f20777c4d 100644 --- a/internal/pkg/flags/flags_test.go +++ b/internal/pkg/flags/flags_test.go @@ -156,13 +156,13 @@ func Test_ConstantFlags(t *testing.T) { expectedValue: "kyma-system", }, { - constName: "DefaultRootCASecretName", - constValue: DefaultRootCASecretName, + constName: "DefaultIstioGatewayName", + constValue: DefaultIstioGatewayName, expectedValue: "klm-watcher", }, { - constName: "DefaultRootCASecretNamespace", - constValue: DefaultRootCASecretNamespace, + constName: "DefaultIstioGatewayNamespace", + constValue: DefaultIstioGatewayNamespace, expectedValue: "istio-system", }, { diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 553cefbdb6..7943446b70 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -15,13 +15,13 @@ import ( "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "github.com/kyma-project/lifecycle-manager/pkg/util" ) const ( LastModifiedAtAnnotation = "lastModifiedAt" gatewaySecretName = "klm-istio-gateway" //nolint:gosec // gatewaySecretName is not a credential + kcpRootSecretName = "klm-watcher" kcpCACertName = "klm-watcher-serving" istioNamespace = "istio-system" ) @@ -149,7 +149,7 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre defer cancel() secretWatch, err := clientset.CoreV1().Secrets(istioNamespace).Watch(ctx, apimetav1.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, flags.DefaultRootCASecretName).String(), + FieldSelector: fields.OneTermEqualSelector(apimetav1.ObjectNameField, kcpRootSecretName).String(), }) if err != nil { log.Error(err, "unable to start watching root certificate") From 7f1110c538d76a239a8dd65c8ce0d66fd2f434f7 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 18 Nov 2024 18:22:42 +0100 Subject: [PATCH 25/42] fix: Istion Rotation E2E Check --- config/manager/kustomization.yaml | 4 ++-- internal/pkg/flags/flags_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 00ae86ff75..99c05dca78 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -10,5 +10,5 @@ generatorOptions: images: - name: controller - newName: europe-docker.pkg.dev/kyma-project/prod/lifecycle-manager - newTag: latest + newName: k3d-kcp-registry.localhost:5000/lifecycle-manager + newTag: "20241114183639" diff --git a/internal/pkg/flags/flags_test.go b/internal/pkg/flags/flags_test.go index 5f20777c4d..fbe6aba7c4 100644 --- a/internal/pkg/flags/flags_test.go +++ b/internal/pkg/flags/flags_test.go @@ -163,7 +163,7 @@ func Test_ConstantFlags(t *testing.T) { { constName: "DefaultIstioGatewayNamespace", constValue: DefaultIstioGatewayNamespace, - expectedValue: "istio-system", + expectedValue: "kcp-system", }, { constName: "DefaultIstioNamespace", From 04531587d511d7660b5b31724ea01ae5f38dce16 Mon Sep 17 00:00:00 2001 From: Raj Date: Tue, 19 Nov 2024 13:06:29 +0100 Subject: [PATCH 26/42] fix: Istion Rotation E2E Check --- tests/e2e/istio_gateway_secret_rotation_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/e2e/istio_gateway_secret_rotation_test.go b/tests/e2e/istio_gateway_secret_rotation_test.go index d176e0f283..677fcf6c4d 100644 --- a/tests/e2e/istio_gateway_secret_rotation_test.go +++ b/tests/e2e/istio_gateway_secret_rotation_test.go @@ -6,7 +6,6 @@ import ( "k8s.io/apimachinery/pkg/types" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" . "github.com/kyma-project/lifecycle-manager/pkg/testutils" @@ -22,7 +21,7 @@ var _ = Describe("Istio Gateway Secret Rotation", Ordered, func() { Context("Given KCP Cluster, rotated CA certificate, and Istio Gateway Secret", func() { It("Then Istio Gateway Secret is a copy of CA Certificate", func() { namespacedRootCASecretName := types.NamespacedName{ - Name: flags.DefaultRootCASecretName, + Name: "klm-watcher", Namespace: istioNamespace, } From 645dbdb7cd7753d99af9f25808ee1cc90fff618b Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 20 Nov 2024 16:54:36 +0100 Subject: [PATCH 27/42] test: Add Unit Tests for handler.go --- cmd/main.go | 4 +- pkg/gatewaysecret/handler.go | 120 +++----- pkg/gatewaysecret/handler_test.go | 351 ++++++++++++++++++++++++ pkg/gatewaysecret/secretmanager.go | 94 +++++++ pkg/gatewaysecret/secretmanager_test.go | 30 ++ pkg/watcher/certificate_manager.go | 6 +- pkg/watcher/certificate_manager_test.go | 4 +- 7 files changed, 522 insertions(+), 87 deletions(-) create mode 100644 pkg/gatewaysecret/handler_test.go create mode 100644 pkg/gatewaysecret/secretmanager.go create mode 100644 pkg/gatewaysecret/secretmanager_test.go diff --git a/cmd/main.go b/cmd/main.go index 1cb8d9a891..9220339c15 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(gatewaysecret.NewSecretManagerImpl(kcpClient)) - gatewaysecret.WatchChangesOnRootCertificate(kcpClientset, gatewaySecretHandler, setupLog) + gatewaysecret.StartRootCertificateWatch(kcpClientset, gatewaySecretHandler, setupLog) } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 7943446b70..32df3bd38f 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -3,7 +3,6 @@ package gatewaysecret import ( "context" "errors" - "fmt" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -13,7 +12,6 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" - "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kyma-project/lifecycle-manager/pkg/util" ) @@ -29,17 +27,17 @@ const ( var errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") type GatewaySecretHandler struct { - kcpClient client.Client + SecretManager } -func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { +func NewGatewaySecretHandler(secretManager SecretManager) *GatewaySecretHandler { return &GatewaySecretHandler{ - kcpClient: kcpClient, + secretManager, } } func (gsh *GatewaySecretHandler) ManageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { - gwSecret, err := gsh.findGatewaySecret(ctx) + gwSecret, err := gsh.FindGatewaySecret(ctx) if util.IsNotFound(err) { return gsh.handleNonExisting(ctx, rootSecret) @@ -52,85 +50,45 @@ func (gsh *GatewaySecretHandler) ManageGatewaySecret(ctx context.Context, rootSe } func (gsh *GatewaySecretHandler) handleNonExisting(ctx context.Context, rootSecret *apicorev1.Secret) error { - gwSecret := gsh.newGatewaySecret(rootSecret) - return gsh.create(ctx, gwSecret) + gwSecret := NewGatewaySecret(rootSecret) + return gsh.Create(ctx, gwSecret) } -func (gsh *GatewaySecretHandler) handleExisting(ctx context.Context, rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret) error { - caCert := certmanagerv1.Certificate{} - if err := gsh.kcpClient.Get(ctx, - client.ObjectKey{Namespace: istioNamespace, Name: kcpCACertName}, - &caCert); err != nil { - return fmt.Errorf("failed to get CA certificate: %w", err) +func (gsh *GatewaySecretHandler) handleExisting(ctx context.Context, + rootSecret *apicorev1.Secret, gwSecret *apicorev1.Secret, +) error { + caCert, err := gsh.GetRootCACertificate(ctx) + if err != nil { + return err } - if gwSecretLastModifiedAt, err := GetValidLastModifiedAt(gwSecret); err == nil { - if caCert.Status.NotBefore != nil && gwSecretLastModifiedAt.After(caCert.Status.NotBefore.Time) { - return nil - } + if copied := CopyRootSecretDataIntoGatewaySecret(gwSecret, caCert, rootSecret); !copied { + return nil } - gwSecret.Data["tls.crt"] = rootSecret.Data["tls.crt"] - gwSecret.Data["tls.key"] = rootSecret.Data["tls.key"] - gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] - return gsh.update(ctx, gwSecret) + return gsh.Update(ctx, gwSecret) } -func (gsh *GatewaySecretHandler) newGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { - gwSecret := &apicorev1.Secret{ - TypeMeta: apimetav1.TypeMeta{ - Kind: "Secret", - APIVersion: apicorev1.SchemeGroupVersion.String(), - }, - ObjectMeta: apimetav1.ObjectMeta{ - Name: gatewaySecretName, - Namespace: istioNamespace, - }, - Data: map[string][]byte{ - "tls.crt": rootSecret.Data["tls.crt"], - "tls.key": rootSecret.Data["tls.key"], - "ca.crt": rootSecret.Data["ca.crt"], - }, +func CopyRootSecretDataIntoGatewaySecret(gwSecret *apicorev1.Secret, caCert certmanagerv1.Certificate, + rootSecret *apicorev1.Secret, +) bool { + if !GatewaySecretRequiresUpdate(gwSecret, caCert) { + return false } - return gwSecret -} - -func (gsh *GatewaySecretHandler) findGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { - return GetGatewaySecret(ctx, gsh.kcpClient) -} -func (gsh *GatewaySecretHandler) create(ctx context.Context, secret *apicorev1.Secret) error { - gsh.updateLastModifiedAt(secret) - if err := gsh.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 { - return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) - } - return nil -} - -func (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) { - if secret.Annotations == nil { - secret.Annotations = make(map[string]string) - } - secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) + gwSecret.Data["tls.crt"] = rootSecret.Data["tls.crt"] + gwSecret.Data["tls.key"] = rootSecret.Data["tls.key"] + gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] + return true } -func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { - secret := &apicorev1.Secret{} - if err := clnt.Get(ctx, client.ObjectKey{ - Name: gatewaySecretName, - Namespace: istioNamespace, - }, secret); err != nil { - return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) +func GatewaySecretRequiresUpdate(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 secret, nil + return true } func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { @@ -142,7 +100,7 @@ func GetValidLastModifiedAt(secret *apicorev1.Secret) (time.Time, error) { return time.Time{}, errCouldNotGetLastModifiedAt } -func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, +func StartRootCertificateWatch(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, log logr.Logger, ) { ctx, cancel := context.WithCancel(context.TODO()) @@ -156,13 +114,17 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre panic(err) } - for event := range secretWatch.ResultChan() { + WatchEvents(ctx, secretWatch.ResultChan(), gatewaySecretHandler, log) +} + +func WatchEvents(ctx context.Context, watchEvents <-chan watch.Event, + gatewaySecretHandler *GatewaySecretHandler, log logr.Logger, +) { + for event := range watchEvents { rootCASecret, _ := event.Object.(*apicorev1.Secret) switch event.Type { - case watch.Added: - fallthrough - case watch.Modified: + case watch.Added, watch.Modified: err := gatewaySecretHandler.ManageGatewaySecret(ctx, rootCASecret) if err != nil { log.Error(err, "unable to manage istio gateway secret") @@ -171,9 +133,7 @@ func WatchChangesOnRootCertificate(clientset *kubernetes.Clientset, gatewaySecre // ignored because it is an invalid state and cert manager should not delete the root secret // even if it is deleted, the certificate manager will recreate it, and trigger the watch event fallthrough - case watch.Error: - fallthrough - case watch.Bookmark: + case watch.Error, watch.Bookmark: fallthrough default: continue diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go new file mode 100644 index 0000000000..8af96414dd --- /dev/null +++ b/pkg/gatewaysecret/handler_test.go @@ -0,0 +1,351 @@ +package gatewaysecret_test + +import ( + "context" + "reflect" + "sync" + "testing" + "time" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + apicorev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/watch" + + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" +) + +const ( + oldTLSCertValue = "old-value1" + oldTLSKeyValue = "old-value2" + oldCACertValue = "old-value3" + + newTLSCertValue = "value1" + newTLSKeyValue = "value2" + newCACertValue = "value3" +) + +func TestGetValidLastModifiedAt(t *testing.T) { + tests := []struct { + name string + secret *apicorev1.Secret + want time.Time + wantErr bool + }{ + { + name: "valid lastModifiedAt annotation", + secret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00Z", + }, + }, + }, + want: time.Date(2024, 11, 1, 0, 0, 0, 0, time.UTC), + wantErr: false, + }, + { + name: "missing lastModifiedAt annotation", + secret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + want: time.Time{}, + wantErr: true, + }, + { + name: "invalid lastModifiedAt annotation key", + secret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "LastModifiedAt": "2024-11-01T00:00:00Z", + }, + }, + }, + want: time.Time{}, + wantErr: true, + }, + { + name: "invalid lastModifiedAt annotation time format", + secret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00", + }, + }, + }, + want: time.Time{}, + wantErr: true, + }, + } + for _, testcase := range tests { + t.Run(testcase.name, func(t *testing.T) { + got, err := gatewaysecret.GetValidLastModifiedAt(testcase.secret) + if (err != nil) != testcase.wantErr { + t.Errorf("GetValidLastModifiedAt() error = %v, wantErr %v", err, testcase.wantErr) + return + } + if !reflect.DeepEqual(got, testcase.want) { + t.Errorf("GetValidLastModifiedAt() got = %v, want %v", got, testcase.want) + } + }) + } +} + +func TestGatewaySecretRequiresUpdate(t *testing.T) { + type args struct { + gwSecret *apicorev1.Secret + caCert certmanagerv1.Certificate + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "gateway secret is newer than CA certificate", + args: args{ + gwSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:10Z", + }, + }, + }, + caCert: certmanagerv1.Certificate{ + Status: certmanagerv1.CertificateStatus{ + NotBefore: &apimetav1.Time{ + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), + }, + }, + }, + }, + want: false, + }, + { + name: "gateway secret is older than CA certificate", + args: args{ + gwSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00Z", + }, + }, + }, + caCert: certmanagerv1.Certificate{ + Status: certmanagerv1.CertificateStatus{ + NotBefore: &apimetav1.Time{ + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), + }, + }, + }, + }, + want: true, + }, + { + name: "gateway secret is newer but has parsing error for lastModifiedAt", + args: args{ + gwSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00", + }, + }, + }, + caCert: certmanagerv1.Certificate{ + Status: certmanagerv1.CertificateStatus{ + NotBefore: &apimetav1.Time{ + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), + }, + }, + }, + }, + want: true, + }, + } + for _, testcase := range tests { + t.Run(testcase.name, func(t *testing.T) { + if got := gatewaysecret.GatewaySecretRequiresUpdate( + testcase.args.gwSecret, testcase.args.caCert); got != testcase.want { + t.Errorf("GatewaySecretRequiresUpdate() = %v, want %v", got, testcase.want) + } + }) + } +} + +func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { + t.Parallel() + + // Current gateway secret + gwSecret := &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00Z", + }, + }, + Data: map[string][]byte{ + "tls.crt": []byte(oldTLSCertValue), + "tls.key": []byte(oldTLSKeyValue), + "ca.crt": []byte(oldCACertValue), + }, + } + + // Newer than gateway secret + caCert := certmanagerv1.Certificate{ + Status: certmanagerv1.CertificateStatus{ + NotBefore: &apimetav1.Time{ + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), + }, + }, + } + rootSecret := &apicorev1.Secret{ + Data: map[string][]byte{ + "tls.crt": []byte(newTLSCertValue), + "tls.key": []byte(newTLSKeyValue), + "ca.crt": []byte(newCACertValue), + }, + } + + gatewaysecret.CopyRootSecretDataIntoGatewaySecret(gwSecret, caCert, rootSecret) + + require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) + require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) + require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) +} + +type MockSecretManager struct { + findGatewaySecretFunc func(ctx context.Context) (*apicorev1.Secret, error) + getRootCACertificateFunc func(ctx context.Context) (certmanagerv1.Certificate, error) + createFunc func(ctx context.Context, secret *apicorev1.Secret) error + updateFunc func(ctx context.Context, secret *apicorev1.Secret) error +} + +func (m MockSecretManager) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { + return m.findGatewaySecretFunc(ctx) +} + +func (m MockSecretManager) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { + return m.getRootCACertificateFunc(ctx) +} + +func (m MockSecretManager) Create(ctx context.Context, secret *apicorev1.Secret) error { + return m.createFunc(ctx, secret) +} + +func (m MockSecretManager) Update(ctx context.Context, secret *apicorev1.Secret) error { + return m.updateFunc(ctx, secret) +} + +func TestWatchEventsNewGatewaySecret(t *testing.T) { + t.Parallel() + + findGatewaySecretFunc := func(ctx context.Context) (*apicorev1.Secret, error) { + return nil, &meta.NoResourceMatchError{} + } + createFunc := func(ctx context.Context, gwSecret *apicorev1.Secret) error { + require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) + require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) + require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) + + return nil + } + mockManager := MockSecretManager{findGatewaySecretFunc: findGatewaySecretFunc, createFunc: createFunc} + handler := gatewaysecret.GatewaySecretHandler{SecretManager: mockManager} + + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + var waitGroup sync.WaitGroup + waitGroup.Add(1) + + events := make(chan watch.Event, 1) + go func() { + defer waitGroup.Done() + gatewaysecret.WatchEvents(ctx, events, &handler, logr.Logger{}) + }() + + events <- watch.Event{ + Type: watch.Added, + Object: &apicorev1.Secret{ + Data: map[string][]byte{ + "tls.crt": []byte(newTLSCertValue), + "tls.key": []byte(newTLSKeyValue), + "ca.crt": []byte(newCACertValue), + }, + }, + } + close(events) + + waitGroup.Wait() +} + +func TestWatchEventsExistingGatewaySecret(t *testing.T) { + t.Parallel() + + findGatewaySecretFunc := func(ctx context.Context) (*apicorev1.Secret, error) { + return &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + "lastModifiedAt": "2024-11-01T00:00:00Z", + }, + }, + Data: map[string][]byte{ + "tls.crt": []byte(oldTLSCertValue), + "tls.key": []byte(oldTLSKeyValue), + "ca.crt": []byte(oldCACertValue), + }, + }, nil + } + getRootCACertificateFunc := func(ctx context.Context) (certmanagerv1.Certificate, error) { + return certmanagerv1.Certificate{ + Status: certmanagerv1.CertificateStatus{ + NotBefore: &apimetav1.Time{ + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), + }, + }, + }, nil + } + updateFunc := func(ctx context.Context, gwSecret *apicorev1.Secret) error { + require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) + require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) + require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) + + return nil + } + + mockManager := MockSecretManager{ + findGatewaySecretFunc: findGatewaySecretFunc, + getRootCACertificateFunc: getRootCACertificateFunc, updateFunc: updateFunc, + } + handler := gatewaysecret.GatewaySecretHandler{SecretManager: mockManager} + + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + var waitGroupg sync.WaitGroup + waitGroupg.Add(1) + + events := make(chan watch.Event, 1) + go func() { + defer waitGroupg.Done() + gatewaysecret.WatchEvents(ctx, events, &handler, logr.Logger{}) + }() + + events <- watch.Event{ + Type: watch.Added, + Object: &apicorev1.Secret{ + Data: map[string][]byte{ + "tls.crt": []byte(newTLSCertValue), + "tls.key": []byte(newTLSKeyValue), + "ca.crt": []byte(newCACertValue), + }, + }, + } + close(events) + + waitGroupg.Wait() +} diff --git a/pkg/gatewaysecret/secretmanager.go b/pkg/gatewaysecret/secretmanager.go new file mode 100644 index 0000000000..680d0faf53 --- /dev/null +++ b/pkg/gatewaysecret/secretmanager.go @@ -0,0 +1,94 @@ +package gatewaysecret + +import ( + "context" + "fmt" + "time" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + apicorev1 "k8s.io/api/core/v1" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type SecretManager interface { + FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) + GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) + Create(ctx context.Context, secret *apicorev1.Secret) error + Update(ctx context.Context, secret *apicorev1.Secret) error +} + +type SecretManagerImpl struct { + kcpClient client.Client +} + +func NewSecretManagerImpl(kcpClient client.Client) *SecretManagerImpl { + return &SecretManagerImpl{kcpClient: kcpClient} +} + +func (sm *SecretManagerImpl) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { + return GetGatewaySecret(ctx, sm.kcpClient) +} + +func (sm *SecretManagerImpl) Create(ctx context.Context, secret *apicorev1.Secret) error { + sm.updateLastModifiedAt(secret) + if err := sm.kcpClient.Create(ctx, secret); err != nil { + return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) + } + return nil +} + +func (sm *SecretManagerImpl) Update(ctx context.Context, secret *apicorev1.Secret) error { + sm.updateLastModifiedAt(secret) + if err := sm.kcpClient.Update(ctx, secret); err != nil { + return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) + } + return nil +} + +func (sm *SecretManagerImpl) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { + caCert := certmanagerv1.Certificate{} + if err := sm.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 (sm *SecretManagerImpl) updateLastModifiedAt(secret *apicorev1.Secret) { + if secret.Annotations == nil { + secret.Annotations = make(map[string]string) + } + secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) +} + +func NewGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { + gwSecret := &apicorev1.Secret{ + TypeMeta: apimetav1.TypeMeta{ + Kind: "Secret", + APIVersion: apicorev1.SchemeGroupVersion.String(), + }, + ObjectMeta: apimetav1.ObjectMeta{ + Name: gatewaySecretName, + Namespace: istioNamespace, + }, + Data: map[string][]byte{ + "tls.crt": rootSecret.Data["tls.crt"], + "tls.key": rootSecret.Data["tls.key"], + "ca.crt": rootSecret.Data["ca.crt"], + }, + } + return gwSecret +} + +func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + if err := clnt.Get(ctx, client.ObjectKey{ + Name: gatewaySecretName, + Namespace: istioNamespace, + }, secret); err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) + } + return secret, nil +} diff --git a/pkg/gatewaysecret/secretmanager_test.go b/pkg/gatewaysecret/secretmanager_test.go new file mode 100644 index 0000000000..5a8e3e5109 --- /dev/null +++ b/pkg/gatewaysecret/secretmanager_test.go @@ -0,0 +1,30 @@ +package gatewaysecret_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + apicorev1 "k8s.io/api/core/v1" + + "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" +) + +func TestNewGatewaySecret(t *testing.T) { + t.Parallel() + + rootSecret := &apicorev1.Secret{ + Data: map[string][]byte{ + "tls.crt": []byte(newTLSCertValue), + "tls.key": []byte(newTLSKeyValue), + "ca.crt": []byte(newCACertValue), + }, + } + gwSecret := gatewaysecret.NewGatewaySecret(rootSecret) + + require.True(t, gwSecret.Name == "klm-istio-gateway") + require.True(t, gwSecret.Namespace == "istio-system") + + require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) + require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) + require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) +} diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index a570d25f21..04967a55a6 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -282,12 +282,12 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym if err != nil { return err } - watcherCert, err := c.getCertificateSecret(ctx) + watcherSecret, err := c.getCertificateSecret(ctx) if err != nil { return err } - if watcherCert != nil && IsGatewaySecretNewerThanWatcherCert(gatewaySecret, watcherCert) { + if watcherSecret != nil && SecretRequiresRotation(gatewaySecret, watcherSecret) { logf.FromContext(ctx).V(log.DebugLevel).Info("CA Certificate was rotated, removing certificate", "kyma", kymaObjKey) if err = c.removeSecret(ctx); err != nil { @@ -298,7 +298,7 @@ func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kym return nil } -func IsGatewaySecretNewerThanWatcherCert(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { +func SecretRequiresRotation(gatewaySecret *apicorev1.Secret, watcherSecret *apicorev1.Secret) bool { if gwSecretLastModifiedAt, err := gatewaysecret.GetValidLastModifiedAt(gatewaySecret); err == nil { if watcherSecret.CreationTimestamp.Time.After(gwSecretLastModifiedAt) { return false diff --git a/pkg/watcher/certificate_manager_test.go b/pkg/watcher/certificate_manager_test.go index 0437866b5b..8a9b7b5fe2 100644 --- a/pkg/watcher/certificate_manager_test.go +++ b/pkg/watcher/certificate_manager_test.go @@ -101,8 +101,8 @@ func TestIsGatewaySecretNewerThanWatcherCert(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := watcher.IsGatewaySecretNewerThanWatcherCert(tt.args.gatewaySecret, tt.args.watcherSecret); got != tt.want { - t.Errorf("IsGatewaySecretNewerThanWatcherCert() = %v, want %v", got, tt.want) + if got := watcher.SecretRequiresRotation(tt.args.gatewaySecret, tt.args.watcherSecret); got != tt.want { + t.Errorf("SecretRequiresRotation() = %v, want %v", got, tt.want) } }) } From 25fb3a254ee77b8af87ceec006a6fc94c8e48543 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 20 Nov 2024 16:57:46 +0100 Subject: [PATCH 28/42] fix: Event name in unit test --- pkg/gatewaysecret/handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 8af96414dd..0644ec5246 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -336,7 +336,7 @@ func TestWatchEventsExistingGatewaySecret(t *testing.T) { }() events <- watch.Event{ - Type: watch.Added, + Type: watch.Modified, Object: &apicorev1.Secret{ Data: map[string][]byte{ "tls.crt": []byte(newTLSCertValue), From 2fa53c85dc1d8e6a5d8b24d7b4750faefe008910 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 20 Nov 2024 17:03:57 +0100 Subject: [PATCH 29/42] refactor: require.Equal parameter order --- pkg/gatewaysecret/handler_test.go | 18 +++++++++--------- pkg/gatewaysecret/secretmanager_test.go | 6 +++--- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 0644ec5246..81c701b982 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -212,9 +212,9 @@ func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { gatewaysecret.CopyRootSecretDataIntoGatewaySecret(gwSecret, caCert, rootSecret) - require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) - require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) - require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) + require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) + require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) + require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) } type MockSecretManager struct { @@ -247,9 +247,9 @@ func TestWatchEventsNewGatewaySecret(t *testing.T) { return nil, &meta.NoResourceMatchError{} } createFunc := func(ctx context.Context, gwSecret *apicorev1.Secret) error { - require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) - require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) - require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) + require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) + require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) + require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) return nil } @@ -310,9 +310,9 @@ func TestWatchEventsExistingGatewaySecret(t *testing.T) { }, nil } updateFunc := func(ctx context.Context, gwSecret *apicorev1.Secret) error { - require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) - require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) - require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) + require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) + require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) + require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) return nil } diff --git a/pkg/gatewaysecret/secretmanager_test.go b/pkg/gatewaysecret/secretmanager_test.go index 5a8e3e5109..54dffd15ff 100644 --- a/pkg/gatewaysecret/secretmanager_test.go +++ b/pkg/gatewaysecret/secretmanager_test.go @@ -24,7 +24,7 @@ func TestNewGatewaySecret(t *testing.T) { require.True(t, gwSecret.Name == "klm-istio-gateway") require.True(t, gwSecret.Namespace == "istio-system") - require.Equal(t, string(gwSecret.Data["tls.crt"]), newTLSCertValue) - require.Equal(t, string(gwSecret.Data["tls.key"]), newTLSKeyValue) - require.Equal(t, string(gwSecret.Data["ca.crt"]), newCACertValue) + require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) + require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) + require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) } From 9f6646d292e9d222ca9b6367d019b08732f6476f Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 20 Nov 2024 17:06:41 +0100 Subject: [PATCH 30/42] refactor: shift namespace const to testutils package --- pkg/testutils/namespace.go | 1 + tests/e2e/ca_certificate_rotation_test.go | 6 +++--- tests/e2e/istio_gateway_secret_rotation_test.go | 2 +- tests/e2e/suite_test.go | 1 - 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/testutils/namespace.go b/pkg/testutils/namespace.go index a06968a487..8ee4e68ca6 100644 --- a/pkg/testutils/namespace.go +++ b/pkg/testutils/namespace.go @@ -14,6 +14,7 @@ import ( const ( RemoteNamespace = shared.DefaultRemoteNamespace ControlPlaneNamespace = "kcp-system" + IstioNamespace = "istio-system" ) func CreateNamespace(ctx context.Context, clnt client.Client, name string) error { diff --git a/tests/e2e/ca_certificate_rotation_test.go b/tests/e2e/ca_certificate_rotation_test.go index b89db5eda3..a7b88df960 100644 --- a/tests/e2e/ca_certificate_rotation_test.go +++ b/tests/e2e/ca_certificate_rotation_test.go @@ -26,7 +26,7 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { Context("Given KCP Cluster and rotated CA certificate", func() { kcpSecretName := types.NamespacedName{ Name: kyma.Name + "-webhook-tls", - Namespace: istioNamespace, + Namespace: IstioNamespace, } skrSecretName := types.NamespacedName{ Name: watcher.SkrTLSName, @@ -36,7 +36,7 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { var err error namespacedSecretName := types.NamespacedName{ Name: watcher.ResolveTLSCertName(kyma.Name), - Namespace: istioNamespace, + Namespace: IstioNamespace, } tlsSecret, err := GetTLSSecret(ctx, namespacedSecretName, kcpClient) Expect(err).NotTo(HaveOccurred()) @@ -50,7 +50,7 @@ var _ = Describe("CA Certificate Rotation", Ordered, func() { By("And new TLS Certificate is created") namespacedCertName := types.NamespacedName{ Name: caCertName, - Namespace: istioNamespace, + Namespace: IstioNamespace, } caCertificate, err = GetCACertificate(ctx, namespacedCertName, kcpClient) Expect(err).NotTo(HaveOccurred()) diff --git a/tests/e2e/istio_gateway_secret_rotation_test.go b/tests/e2e/istio_gateway_secret_rotation_test.go index 677fcf6c4d..31811142a9 100644 --- a/tests/e2e/istio_gateway_secret_rotation_test.go +++ b/tests/e2e/istio_gateway_secret_rotation_test.go @@ -22,7 +22,7 @@ var _ = Describe("Istio Gateway Secret Rotation", Ordered, func() { It("Then Istio Gateway Secret is a copy of CA Certificate", func() { namespacedRootCASecretName := types.NamespacedName{ Name: "klm-watcher", - Namespace: istioNamespace, + Namespace: IstioNamespace, } // The timeout used is 4 minutes bec the certificate gets rotated every 1 minute diff --git a/tests/e2e/suite_test.go b/tests/e2e/suite_test.go index dd2aa7ce88..5fc03e01e9 100644 --- a/tests/e2e/suite_test.go +++ b/tests/e2e/suite_test.go @@ -32,7 +32,6 @@ import ( const ( kcpConfigEnvVar, skrConfigEnvVar = "KCP_KUBECONFIG", "SKR_KUBECONFIG" clientQPS, clientBurst = 1000, 2000 - istioNamespace = "istio-system" ) var errEmptyEnvVar = errors.New("environment variable is empty") From 414f3680f755fafc416577d711cdb91bdeb9e8cf Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 20 Nov 2024 17:15:59 +0100 Subject: [PATCH 31/42] refactor: using require.Equal --- pkg/gatewaysecret/secretmanager_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/gatewaysecret/secretmanager_test.go b/pkg/gatewaysecret/secretmanager_test.go index 54dffd15ff..5b9be4b9bb 100644 --- a/pkg/gatewaysecret/secretmanager_test.go +++ b/pkg/gatewaysecret/secretmanager_test.go @@ -21,8 +21,8 @@ func TestNewGatewaySecret(t *testing.T) { } gwSecret := gatewaysecret.NewGatewaySecret(rootSecret) - require.True(t, gwSecret.Name == "klm-istio-gateway") - require.True(t, gwSecret.Namespace == "istio-system") + require.Equal(t, "klm-istio-gateway", gwSecret.Name) + require.Equal(t, "istio-system", gwSecret.Namespace) require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) From 841b97d39cf60ff4420d09b15572582f5ec64f93 Mon Sep 17 00:00:00 2001 From: Raj Date: Thu, 21 Nov 2024 13:33:14 +0100 Subject: [PATCH 32/42] refactor: PR comments --- pkg/gatewaysecret/handler.go | 14 ++------- pkg/gatewaysecret/handler_test.go | 35 ++++++++++++--------- pkg/watcher/certificate_manager.go | 8 ++--- pkg/watcher/skr_webhook_manifest_manager.go | 18 +++++------ 4 files changed, 33 insertions(+), 42 deletions(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 32df3bd38f..5ec92dc889 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -61,25 +61,17 @@ func (gsh *GatewaySecretHandler) handleExisting(ctx context.Context, if err != nil { return err } - - if copied := CopyRootSecretDataIntoGatewaySecret(gwSecret, caCert, rootSecret); !copied { + if !GatewaySecretRequiresUpdate(gwSecret, caCert) { return nil } - + CopyRootSecretDataIntoGatewaySecret(gwSecret, rootSecret) return gsh.Update(ctx, gwSecret) } -func CopyRootSecretDataIntoGatewaySecret(gwSecret *apicorev1.Secret, caCert certmanagerv1.Certificate, - rootSecret *apicorev1.Secret, -) bool { - if !GatewaySecretRequiresUpdate(gwSecret, caCert) { - return false - } - +func CopyRootSecretDataIntoGatewaySecret(gwSecret *apicorev1.Secret, rootSecret *apicorev1.Secret) { gwSecret.Data["tls.crt"] = rootSecret.Data["tls.crt"] gwSecret.Data["tls.key"] = rootSecret.Data["tls.key"] gwSecret.Data["ca.crt"] = rootSecret.Data["ca.crt"] - return true } func GatewaySecretRequiresUpdate(gwSecret *apicorev1.Secret, caCert certmanagerv1.Certificate) bool { diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 81c701b982..25cf3f5b9c 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -147,7 +147,7 @@ func TestGatewaySecretRequiresUpdate(t *testing.T) { want: true, }, { - name: "gateway secret is newer but has parsing error for lastModifiedAt", + name: "gateway secret has parsing error for lastModifiedAt", args: args{ gwSecret: &apicorev1.Secret{ ObjectMeta: apimetav1.ObjectMeta{ @@ -166,6 +166,24 @@ func TestGatewaySecretRequiresUpdate(t *testing.T) { }, want: true, }, + { + name: "gateway secret is missing lastModifiedAt", + args: args{ + gwSecret: &apicorev1.Secret{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + caCert: certmanagerv1.Certificate{ + Status: certmanagerv1.CertificateStatus{ + NotBefore: &apimetav1.Time{ + Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), + }, + }, + }, + }, + want: true, + }, } for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { @@ -182,11 +200,6 @@ func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { // Current gateway secret gwSecret := &apicorev1.Secret{ - ObjectMeta: apimetav1.ObjectMeta{ - Annotations: map[string]string{ - "lastModifiedAt": "2024-11-01T00:00:00Z", - }, - }, Data: map[string][]byte{ "tls.crt": []byte(oldTLSCertValue), "tls.key": []byte(oldTLSKeyValue), @@ -194,14 +207,6 @@ func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { }, } - // Newer than gateway secret - caCert := certmanagerv1.Certificate{ - Status: certmanagerv1.CertificateStatus{ - NotBefore: &apimetav1.Time{ - Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), - }, - }, - } rootSecret := &apicorev1.Secret{ Data: map[string][]byte{ "tls.crt": []byte(newTLSCertValue), @@ -210,7 +215,7 @@ func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { }, } - gatewaysecret.CopyRootSecretDataIntoGatewaySecret(gwSecret, caCert, rootSecret) + gatewaysecret.CopyRootSecretDataIntoGatewaySecret(gwSecret, rootSecret) require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) diff --git a/pkg/watcher/certificate_manager.go b/pkg/watcher/certificate_manager.go index 04967a55a6..86dc47dd16 100644 --- a/pkg/watcher/certificate_manager.go +++ b/pkg/watcher/certificate_manager.go @@ -277,11 +277,9 @@ func (e *CertificateNotReadyError) Error() string { return "Certificate-Secret does not exist" } -func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, kymaObjKey client.ObjectKey) error { - gatewaySecret, err := gatewaysecret.GetGatewaySecret(ctx, c.kcpClient) - if err != nil { - return err - } +func (c *CertificateManager) RemoveSecretAfterCARotated(ctx context.Context, gatewaySecret *apicorev1.Secret, + kymaObjKey client.ObjectKey, +) error { watcherSecret, err := c.getCertificateSecret(ctx) if err != nil { return err diff --git a/pkg/watcher/skr_webhook_manifest_manager.go b/pkg/watcher/skr_webhook_manifest_manager.go index 867b2b9293..ca11fe388f 100644 --- a/pkg/watcher/skr_webhook_manifest_manager.go +++ b/pkg/watcher/skr_webhook_manifest_manager.go @@ -85,7 +85,8 @@ func (m *SKRWebhookManifestManager) Install(ctx context.Context, kyma *v1beta2.K return fmt.Errorf("failed to get skrContext: %w", err) } - if _, err = gatewaysecret.GetGatewaySecret(ctx, m.kcpClient); err != nil { + var gatewaySecret *apicorev1.Secret + if gatewaySecret, err = gatewaysecret.GetGatewaySecret(ctx, m.kcpClient); err != nil { return err } @@ -100,14 +101,14 @@ func (m *SKRWebhookManifestManager) Install(ctx context.Context, kyma *v1beta2.K m.updateCertNotRenewMetrics(certificate, kyma) - if err := certificateMgr.RemoveSecretAfterCARotated(ctx, kymaObjKey); err != nil { + if err := certificateMgr.RemoveSecretAfterCARotated(ctx, gatewaySecret, kymaObjKey); err != nil { return fmt.Errorf("error verify CA cert rotation: %w", err) } logger.V(log.DebugLevel).Info("Successfully created Certificate", "kyma", kymaObjKey) resources, err := m.getSKRClientObjectsForInstall( - ctx, kymaObjKey, m.config.RemoteSyncNamespace, logger) + ctx, kymaObjKey, m.config.RemoteSyncNamespace, gatewaySecret, logger) if err != nil { return err } @@ -173,10 +174,10 @@ func (m *SKRWebhookManifestManager) Remove(ctx context.Context, kyma *v1beta2.Ky } func (m *SKRWebhookManifestManager) getSKRClientObjectsForInstall(ctx context.Context, - kymaObjKey client.ObjectKey, remoteNs string, logger logr.Logger, + kymaObjKey client.ObjectKey, remoteNs string, gatewaySecret *apicorev1.Secret, logger logr.Logger, ) ([]client.Object, error) { var skrClientObjects []client.Object - resourcesConfig, err := m.getUnstructuredResourcesConfig(ctx, kymaObjKey, remoteNs) + resourcesConfig, err := m.getUnstructuredResourcesConfig(ctx, kymaObjKey, remoteNs, gatewaySecret) if err != nil { return nil, err } @@ -217,7 +218,7 @@ func (m *SKRWebhookManifestManager) getRawManifestClientObjects(cfg *unstructure } func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.Context, - kymaObjKey client.ObjectKey, remoteNs string, + kymaObjKey client.ObjectKey, remoteNs string, gatewaySecret *apicorev1.Secret, ) (*unstructuredResourcesConfig, error) { tlsSecret := &apicorev1.Secret{} certObjKey := client.ObjectKey{ @@ -232,11 +233,6 @@ func (m *SKRWebhookManifestManager) getUnstructuredResourcesConfig(ctx context.C return nil, fmt.Errorf("error fetching TLS secret: %w", err) } - gatewaySecret, err := gatewaysecret.GetGatewaySecret(ctx, m.kcpClient) - if err != nil { - return nil, fmt.Errorf("error fetching gateway secret: %w", err) - } - return &unstructuredResourcesConfig{ contractVersion: version, kcpAddress: m.kcpAddr, From b03cd8fe7309b2b8483f56e733fc633d8d2c2c76 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 25 Nov 2024 09:51:13 +0100 Subject: [PATCH 33/42] refactor: handler unit test, remove secret manager interface --- cmd/main.go | 2 +- pkg/gatewaysecret/handler.go | 81 +++++++++++++-- pkg/gatewaysecret/handler_test.go | 128 ++++++------------------ pkg/gatewaysecret/secretmanager.go | 94 ----------------- pkg/gatewaysecret/secretmanager_test.go | 30 ------ 5 files changed, 106 insertions(+), 229 deletions(-) delete mode 100644 pkg/gatewaysecret/secretmanager.go delete mode 100644 pkg/gatewaysecret/secretmanager_test.go diff --git a/cmd/main.go b/cmd/main.go index 9220339c15..cdba1839ce 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -214,7 +214,7 @@ 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(gatewaysecret.NewSecretManagerImpl(kcpClient)) + gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient) gatewaysecret.StartRootCertificateWatch(kcpClientset, gatewaySecretHandler, setupLog) } diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 5ec92dc889..b4e1887670 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -3,6 +3,8 @@ package gatewaysecret import ( "context" "errors" + "fmt" + "sigs.k8s.io/controller-runtime/pkg/client" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -27,12 +29,12 @@ const ( var errCouldNotGetLastModifiedAt = errors.New("getting lastModifiedAt time failed") type GatewaySecretHandler struct { - SecretManager + kcpClient client.Client } -func NewGatewaySecretHandler(secretManager SecretManager) *GatewaySecretHandler { +func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { return &GatewaySecretHandler{ - secretManager, + kcpClient: kcpClient, } } @@ -92,6 +94,73 @@ 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 (gsh *GatewaySecretHandler) Create(ctx context.Context, secret *apicorev1.Secret) error { + gsh.updateLastModifiedAt(secret) + if err := gsh.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 { + return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) + } + return nil +} + +func (gsh *GatewaySecretHandler) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { + caCert := certmanagerv1.Certificate{} + if err := gsh.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 (gsh *GatewaySecretHandler) updateLastModifiedAt(secret *apicorev1.Secret) { + if secret.Annotations == nil { + secret.Annotations = make(map[string]string) + } + secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) +} + +func NewGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { + gwSecret := &apicorev1.Secret{ + TypeMeta: apimetav1.TypeMeta{ + Kind: "Secret", + APIVersion: apicorev1.SchemeGroupVersion.String(), + }, + ObjectMeta: apimetav1.ObjectMeta{ + Name: gatewaySecretName, + Namespace: istioNamespace, + }, + Data: map[string][]byte{ + "tls.crt": rootSecret.Data["tls.crt"], + "tls.key": rootSecret.Data["tls.key"], + "ca.crt": rootSecret.Data["ca.crt"], + }, + } + return gwSecret +} + +func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { + secret := &apicorev1.Secret{} + if err := clnt.Get(ctx, client.ObjectKey{ + Name: gatewaySecretName, + Namespace: istioNamespace, + }, secret); err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) + } + return secret, nil +} + func StartRootCertificateWatch(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, log logr.Logger, ) { @@ -106,18 +175,18 @@ func StartRootCertificateWatch(clientset *kubernetes.Clientset, gatewaySecretHan panic(err) } - WatchEvents(ctx, secretWatch.ResultChan(), gatewaySecretHandler, log) + WatchEvents(ctx, secretWatch.ResultChan(), gatewaySecretHandler.ManageGatewaySecret, log) } func WatchEvents(ctx context.Context, watchEvents <-chan watch.Event, - gatewaySecretHandler *GatewaySecretHandler, log logr.Logger, + manageGatewaySecretFunc func(context.Context, *apicorev1.Secret) error, log logr.Logger, ) { for event := range watchEvents { rootCASecret, _ := event.Object.(*apicorev1.Secret) switch event.Type { case watch.Added, watch.Modified: - err := gatewaySecretHandler.ManageGatewaySecret(ctx, rootCASecret) + err := manageGatewaySecretFunc(ctx, rootCASecret) if err != nil { log.Error(err, "unable to manage istio gateway secret") } diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 25cf3f5b9c..e5ef9edb91 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -11,7 +11,6 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/require" apicorev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/meta" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/watch" @@ -28,6 +27,26 @@ const ( newCACertValue = "value3" ) +func TestNewGatewaySecret(t *testing.T) { + t.Parallel() + + rootSecret := &apicorev1.Secret{ + Data: map[string][]byte{ + "tls.crt": []byte(newTLSCertValue), + "tls.key": []byte(newTLSKeyValue), + "ca.crt": []byte(newCACertValue), + }, + } + gwSecret := gatewaysecret.NewGatewaySecret(rootSecret) + + require.Equal(t, "klm-istio-gateway", gwSecret.Name) + require.Equal(t, "istio-system", gwSecret.Namespace) + + require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) + require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) + require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) +} + func TestGetValidLastModifiedAt(t *testing.T) { tests := []struct { name string @@ -198,7 +217,6 @@ func TestGatewaySecretRequiresUpdate(t *testing.T) { func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { t.Parallel() - // Current gateway secret gwSecret := &apicorev1.Secret{ Data: map[string][]byte{ "tls.crt": []byte(oldTLSCertValue), @@ -222,55 +240,25 @@ func TestCopyRootSecretDataIntoGatewaySecret(t *testing.T) { require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) } -type MockSecretManager struct { - findGatewaySecretFunc func(ctx context.Context) (*apicorev1.Secret, error) - getRootCACertificateFunc func(ctx context.Context) (certmanagerv1.Certificate, error) - createFunc func(ctx context.Context, secret *apicorev1.Secret) error - updateFunc func(ctx context.Context, secret *apicorev1.Secret) error -} - -func (m MockSecretManager) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { - return m.findGatewaySecretFunc(ctx) -} - -func (m MockSecretManager) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { - return m.getRootCACertificateFunc(ctx) -} - -func (m MockSecretManager) Create(ctx context.Context, secret *apicorev1.Secret) error { - return m.createFunc(ctx, secret) -} - -func (m MockSecretManager) Update(ctx context.Context, secret *apicorev1.Secret) error { - return m.updateFunc(ctx, secret) -} - -func TestWatchEventsNewGatewaySecret(t *testing.T) { +func TestWatchEvents(t *testing.T) { t.Parallel() - findGatewaySecretFunc := func(ctx context.Context) (*apicorev1.Secret, error) { - return nil, &meta.NoResourceMatchError{} - } - createFunc := func(ctx context.Context, gwSecret *apicorev1.Secret) error { - require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) - require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) - require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) - - return nil - } - mockManager := MockSecretManager{findGatewaySecretFunc: findGatewaySecretFunc, createFunc: createFunc} - handler := gatewaysecret.GatewaySecretHandler{SecretManager: mockManager} - ctx, cancel := context.WithCancel(context.TODO()) defer cancel() var waitGroup sync.WaitGroup waitGroup.Add(1) + calledTimes := 0 + mockManageSecretFunc := func(_ context.Context, _ *apicorev1.Secret) error { + calledTimes += 1 + return nil + } + events := make(chan watch.Event, 1) go func() { defer waitGroup.Done() - gatewaysecret.WatchEvents(ctx, events, &handler, logr.Logger{}) + gatewaysecret.WatchEvents(ctx, events, mockManageSecretFunc, logr.Logger{}) }() events <- watch.Event{ @@ -283,63 +271,6 @@ func TestWatchEventsNewGatewaySecret(t *testing.T) { }, }, } - close(events) - - waitGroup.Wait() -} - -func TestWatchEventsExistingGatewaySecret(t *testing.T) { - t.Parallel() - - findGatewaySecretFunc := func(ctx context.Context) (*apicorev1.Secret, error) { - return &apicorev1.Secret{ - ObjectMeta: apimetav1.ObjectMeta{ - Annotations: map[string]string{ - "lastModifiedAt": "2024-11-01T00:00:00Z", - }, - }, - Data: map[string][]byte{ - "tls.crt": []byte(oldTLSCertValue), - "tls.key": []byte(oldTLSKeyValue), - "ca.crt": []byte(oldCACertValue), - }, - }, nil - } - getRootCACertificateFunc := func(ctx context.Context) (certmanagerv1.Certificate, error) { - return certmanagerv1.Certificate{ - Status: certmanagerv1.CertificateStatus{ - NotBefore: &apimetav1.Time{ - Time: time.Date(2024, 11, 1, 0, 0, 5, 0, time.UTC), - }, - }, - }, nil - } - updateFunc := func(ctx context.Context, gwSecret *apicorev1.Secret) error { - require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) - require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) - require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) - - return nil - } - - mockManager := MockSecretManager{ - findGatewaySecretFunc: findGatewaySecretFunc, - getRootCACertificateFunc: getRootCACertificateFunc, updateFunc: updateFunc, - } - handler := gatewaysecret.GatewaySecretHandler{SecretManager: mockManager} - - ctx, cancel := context.WithCancel(context.TODO()) - defer cancel() - - var waitGroupg sync.WaitGroup - waitGroupg.Add(1) - - events := make(chan watch.Event, 1) - go func() { - defer waitGroupg.Done() - gatewaysecret.WatchEvents(ctx, events, &handler, logr.Logger{}) - }() - events <- watch.Event{ Type: watch.Modified, Object: &apicorev1.Secret{ @@ -351,6 +282,7 @@ func TestWatchEventsExistingGatewaySecret(t *testing.T) { }, } close(events) + waitGroup.Wait() - waitGroupg.Wait() + require.Equal(t, 2, calledTimes) } diff --git a/pkg/gatewaysecret/secretmanager.go b/pkg/gatewaysecret/secretmanager.go deleted file mode 100644 index 680d0faf53..0000000000 --- a/pkg/gatewaysecret/secretmanager.go +++ /dev/null @@ -1,94 +0,0 @@ -package gatewaysecret - -import ( - "context" - "fmt" - "time" - - certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - apicorev1 "k8s.io/api/core/v1" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type SecretManager interface { - FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) - GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) - Create(ctx context.Context, secret *apicorev1.Secret) error - Update(ctx context.Context, secret *apicorev1.Secret) error -} - -type SecretManagerImpl struct { - kcpClient client.Client -} - -func NewSecretManagerImpl(kcpClient client.Client) *SecretManagerImpl { - return &SecretManagerImpl{kcpClient: kcpClient} -} - -func (sm *SecretManagerImpl) FindGatewaySecret(ctx context.Context) (*apicorev1.Secret, error) { - return GetGatewaySecret(ctx, sm.kcpClient) -} - -func (sm *SecretManagerImpl) Create(ctx context.Context, secret *apicorev1.Secret) error { - sm.updateLastModifiedAt(secret) - if err := sm.kcpClient.Create(ctx, secret); err != nil { - return fmt.Errorf("failed to create secret %s: %w", secret.Name, err) - } - return nil -} - -func (sm *SecretManagerImpl) Update(ctx context.Context, secret *apicorev1.Secret) error { - sm.updateLastModifiedAt(secret) - if err := sm.kcpClient.Update(ctx, secret); err != nil { - return fmt.Errorf("failed to update secret %s: %w", secret.Name, err) - } - return nil -} - -func (sm *SecretManagerImpl) GetRootCACertificate(ctx context.Context) (certmanagerv1.Certificate, error) { - caCert := certmanagerv1.Certificate{} - if err := sm.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 (sm *SecretManagerImpl) updateLastModifiedAt(secret *apicorev1.Secret) { - if secret.Annotations == nil { - secret.Annotations = make(map[string]string) - } - secret.Annotations[LastModifiedAtAnnotation] = apimetav1.Now().Format(time.RFC3339) -} - -func NewGatewaySecret(rootSecret *apicorev1.Secret) *apicorev1.Secret { - gwSecret := &apicorev1.Secret{ - TypeMeta: apimetav1.TypeMeta{ - Kind: "Secret", - APIVersion: apicorev1.SchemeGroupVersion.String(), - }, - ObjectMeta: apimetav1.ObjectMeta{ - Name: gatewaySecretName, - Namespace: istioNamespace, - }, - Data: map[string][]byte{ - "tls.crt": rootSecret.Data["tls.crt"], - "tls.key": rootSecret.Data["tls.key"], - "ca.crt": rootSecret.Data["ca.crt"], - }, - } - return gwSecret -} - -func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secret, error) { - secret := &apicorev1.Secret{} - if err := clnt.Get(ctx, client.ObjectKey{ - Name: gatewaySecretName, - Namespace: istioNamespace, - }, secret); err != nil { - return nil, fmt.Errorf("failed to get secret %s: %w", gatewaySecretName, err) - } - return secret, nil -} diff --git a/pkg/gatewaysecret/secretmanager_test.go b/pkg/gatewaysecret/secretmanager_test.go deleted file mode 100644 index 5b9be4b9bb..0000000000 --- a/pkg/gatewaysecret/secretmanager_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package gatewaysecret_test - -import ( - "testing" - - "github.com/stretchr/testify/require" - apicorev1 "k8s.io/api/core/v1" - - "github.com/kyma-project/lifecycle-manager/pkg/gatewaysecret" -) - -func TestNewGatewaySecret(t *testing.T) { - t.Parallel() - - rootSecret := &apicorev1.Secret{ - Data: map[string][]byte{ - "tls.crt": []byte(newTLSCertValue), - "tls.key": []byte(newTLSKeyValue), - "ca.crt": []byte(newCACertValue), - }, - } - gwSecret := gatewaysecret.NewGatewaySecret(rootSecret) - - require.Equal(t, "klm-istio-gateway", gwSecret.Name) - require.Equal(t, "istio-system", gwSecret.Namespace) - - require.Equal(t, newTLSCertValue, string(gwSecret.Data["tls.crt"])) - require.Equal(t, newTLSKeyValue, string(gwSecret.Data["tls.key"])) - require.Equal(t, newCACertValue, string(gwSecret.Data["ca.crt"])) -} From 4f899dc90f97d34ff9f11bf6b5aecf053adcea5a Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 25 Nov 2024 10:11:43 +0100 Subject: [PATCH 34/42] refactor: lint gci and gofumpt --- pkg/gatewaysecret/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index b4e1887670..4dbc562816 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "sigs.k8s.io/controller-runtime/pkg/client" "time" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -14,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kyma-project/lifecycle-manager/pkg/util" ) From 8370566d85db186c0ab14938f30c9cf474021a21 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 25 Nov 2024 16:03:11 +0100 Subject: [PATCH 35/42] refactor: unparam lint error --- pkg/gatewaysecret/handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index e5ef9edb91..ee6a759227 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -252,7 +252,7 @@ func TestWatchEvents(t *testing.T) { calledTimes := 0 mockManageSecretFunc := func(_ context.Context, _ *apicorev1.Secret) error { calledTimes += 1 - return nil + return nil //nolint:unparam } events := make(chan watch.Event, 1) From 6eccbd832667e4d2058f70563bf38fdc2a581a3b Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 25 Nov 2024 16:05:26 +0100 Subject: [PATCH 36/42] refactor: unparam lint error --- pkg/gatewaysecret/handler_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index ee6a759227..554949eb0d 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -250,9 +250,10 @@ func TestWatchEvents(t *testing.T) { waitGroup.Add(1) calledTimes := 0 + //nolint:unparam The function has to have this signature to be used in WatchEvents mockManageSecretFunc := func(_ context.Context, _ *apicorev1.Secret) error { calledTimes += 1 - return nil //nolint:unparam + return nil } events := make(chan watch.Event, 1) From 8490271f6de9325175d7853708e3c1096b9ec0e1 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 25 Nov 2024 16:08:18 +0100 Subject: [PATCH 37/42] refactor: unparam lint error --- pkg/gatewaysecret/handler_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/gatewaysecret/handler_test.go b/pkg/gatewaysecret/handler_test.go index 554949eb0d..661ea3950b 100644 --- a/pkg/gatewaysecret/handler_test.go +++ b/pkg/gatewaysecret/handler_test.go @@ -250,8 +250,7 @@ func TestWatchEvents(t *testing.T) { waitGroup.Add(1) calledTimes := 0 - //nolint:unparam The function has to have this signature to be used in WatchEvents - mockManageSecretFunc := func(_ context.Context, _ *apicorev1.Secret) error { + mockManageSecretFunc := func(_ context.Context, _ *apicorev1.Secret) error { //nolint:unparam // The function has to have this signature to be used in WatchEvents calledTimes += 1 return nil } From ca5e190e1ffc9551f3743d16ac7709ffecdd023f Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 27 Nov 2024 08:49:09 +0100 Subject: [PATCH 38/42] chore: add pkg/gatewaysecret unittest coverage --- unit-test-coverage.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 7141c47673..4429378502 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -17,3 +17,4 @@ packages: internal/remote: 13.2 internal/util/collections: 86 pkg/templatelookup: 77.1 + pkg/gatewaysecret: 80 From 203d529a7e05ec3b06b3b8209548a8230feffe8f Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 27 Nov 2024 08:56:19 +0100 Subject: [PATCH 39/42] chore: add pkg/gatewaysecret unittest 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 4429378502..1e9cb284d5 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: 80 + pkg/gatewaysecret: 27.7 From f098fc647929d8ec9c61e59af58e62fbad2d37a2 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 27 Nov 2024 09:00:17 +0100 Subject: [PATCH 40/42] refactor: make ManageGatewaySecret private --- cmd/main.go | 2 +- pkg/gatewaysecret/handler.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index cdba1839ce..b2e0dfd446 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -216,7 +216,7 @@ func setupIstioGatewaySecretRotation(config *rest.Config, kcpClient *remote.Conf kcpClientset := kubernetes.NewForConfigOrDie(config) gatewaySecretHandler := gatewaysecret.NewGatewaySecretHandler(kcpClient) - gatewaysecret.StartRootCertificateWatch(kcpClientset, gatewaySecretHandler, setupLog) + gatewaySecretHandler.StartRootCertificateWatch(kcpClientset, setupLog) } func addHealthChecks(mgr manager.Manager, setupLog logr.Logger) { diff --git a/pkg/gatewaysecret/handler.go b/pkg/gatewaysecret/handler.go index 4dbc562816..2eac5c02aa 100644 --- a/pkg/gatewaysecret/handler.go +++ b/pkg/gatewaysecret/handler.go @@ -38,7 +38,7 @@ func NewGatewaySecretHandler(kcpClient client.Client) *GatewaySecretHandler { } } -func (gsh *GatewaySecretHandler) ManageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { +func (gsh *GatewaySecretHandler) manageGatewaySecret(ctx context.Context, rootSecret *apicorev1.Secret) error { gwSecret, err := gsh.FindGatewaySecret(ctx) if util.IsNotFound(err) { @@ -161,7 +161,7 @@ func GetGatewaySecret(ctx context.Context, clnt client.Client) (*apicorev1.Secre return secret, nil } -func StartRootCertificateWatch(clientset *kubernetes.Clientset, gatewaySecretHandler *GatewaySecretHandler, +func (gsh *GatewaySecretHandler) StartRootCertificateWatch(clientset *kubernetes.Clientset, log logr.Logger, ) { ctx, cancel := context.WithCancel(context.TODO()) @@ -175,7 +175,7 @@ func StartRootCertificateWatch(clientset *kubernetes.Clientset, gatewaySecretHan panic(err) } - WatchEvents(ctx, secretWatch.ResultChan(), gatewaySecretHandler.ManageGatewaySecret, log) + WatchEvents(ctx, secretWatch.ResultChan(), gsh.manageGatewaySecret, log) } func WatchEvents(ctx context.Context, watchEvents <-chan watch.Event, From 2207900c1af40ad4a13f8082bdd13b0cbb755427 Mon Sep 17 00:00:00 2001 From: Raj Date: Wed, 27 Nov 2024 09:32:08 +0100 Subject: [PATCH 41/42] refactor: remove local testing artefacts --- config/manager/kustomization.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 99c05dca78..00ae86ff75 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -10,5 +10,5 @@ generatorOptions: images: - name: controller - newName: k3d-kcp-registry.localhost:5000/lifecycle-manager - newTag: "20241114183639" + newName: europe-docker.pkg.dev/kyma-project/prod/lifecycle-manager + newTag: latest From e6459ac649841a3e848f537402654f613756ed85 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 29 Nov 2024 12:07:20 +0100 Subject: [PATCH 42/42] 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) } }) }