From a0db85a56fa127e00fca06ca79f3fff66d24f44a Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:11:12 -0500 Subject: [PATCH 1/7] Dedup secret handling --- internal/controller/client_controller.go | 87 +++------------------- internal/controller/exporter_controller.go | 87 +++------------------- internal/controller/secret_helpers.go | 61 +++++++++++++++ 3 files changed, 83 insertions(+), 152 deletions(-) create mode 100644 internal/controller/secret_helpers.go diff --git a/internal/controller/client_controller.go b/internal/controller/client_controller.go index 7b26cf30..47f987a7 100644 --- a/internal/controller/client_controller.go +++ b/internal/controller/client_controller.go @@ -21,7 +21,6 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -73,65 +72,24 @@ func (r *ClientReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } -func (r *ClientReconciler) clientSecretExists( - ctx context.Context, - client *jumpstarterdevv1alpha1.Client, -) (bool, error) { - logger := log.FromContext(ctx) - - if client.Status.Credential == nil { - return false, nil - } - // NOTE: this could deserve some level of optimization/caching in the future - secret := &corev1.Secret{} - err := r.Get(ctx, kclient.ObjectKey{ - Namespace: client.Namespace, - Name: client.Status.Credential.Name, - }, secret) - if err != nil { - return false, kclient.IgnoreNotFound(err) - } - - token, ok := secret.Data["token"] - if !ok || r.Signer.UnsafeValidate(string(token)) != nil { - logger.Info("reconcileStatusCredential: the client secret is invalid", "client", client.Name) - return false, r.Delete(ctx, secret) - } - - return true, nil -} - func (r *ClientReconciler) reconcileStatusCredential( ctx context.Context, client *jumpstarterdevv1alpha1.Client, ) error { - logger := log.FromContext(ctx) - - exists, err := r.clientSecretExists(ctx, client) + secret, err := ensureSecret(ctx, kclient.ObjectKey{ + Name: client.Name + "-client", + Namespace: client.Namespace, + }, r.Client, r.Signer, client.Username(r.Signer.Prefix())) if err != nil { - logger.Info("reconcileStatusCredential: the client secret's existence cannot be checked", "client", client.Name) - return err + return fmt.Errorf("reconcileStatusCredential: failed to prepare credential for client: %w", err) } - - if !exists { - if client.Status.Credential != nil { - // TODO: Send an alert notification to cluster - logger.Info("reconcileStatusCredential: the client secret has ceased to exist, will be recreated", "client", client.Name) - } else { - logger.Info("reconcileStatusCredential: creating credential for client") - } - secret, err := r.secretForClient(client) - if err != nil { - return fmt.Errorf("reconcileStatusCredential: failed to prepare credential for client: %w", err) - } - if err := r.Create(ctx, secret); err != nil { - return fmt.Errorf("reconcileStatusCredential: failed to create credential for client: %w", err) - } - client.Status.Credential = &corev1.LocalObjectReference{ - Name: secret.Name, - } + // enable garbage collection on the created resource + if err := controllerutil.SetControllerReference(client, secret, r.Scheme); err != nil { + return fmt.Errorf("reconcileStatusCredential: error setting owner reference: %w", err) + } + client.Status.Credential = &corev1.LocalObjectReference{ + Name: secret.Name, } - return nil } @@ -151,29 +109,6 @@ func (r *ClientReconciler) reconcileStatusEndpoint( return nil } -func (r *ClientReconciler) secretForClient(client *jumpstarterdevv1alpha1.Client) (*corev1.Secret, error) { - token, err := r.Signer.Token(client.Username(r.Signer.Prefix())) - if err != nil { - return nil, err - } - - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: client.Name + "-client", - Namespace: client.Namespace, - }, - Type: corev1.SecretTypeOpaque, - StringData: map[string]string{ - "token": token, - }, - } - // enable garbage collection on the created resource - if err := controllerutil.SetControllerReference(client, secret, r.Scheme); err != nil { - return nil, fmt.Errorf("secretForClient, error setting owner reference: %w", err) - } - return secret, nil -} - // SetupWithManager sets up the controller with the Manager. func (r *ClientReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/exporter_controller.go b/internal/controller/exporter_controller.go index d8d8a3a9..bed30a24 100644 --- a/internal/controller/exporter_controller.go +++ b/internal/controller/exporter_controller.go @@ -21,7 +21,6 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -85,64 +84,23 @@ func (r *ExporterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{}, nil } -func (r *ExporterReconciler) exporterSecretExists( - ctx context.Context, - exporter *jumpstarterdevv1alpha1.Exporter, -) (bool, error) { - logger := log.FromContext(ctx) - - if exporter.Status.Credential == nil { - return false, nil - } - // NOTE: this could deserve some level of optimization/caching in the future - secret := &corev1.Secret{} - err := r.Get(ctx, client.ObjectKey{ - Namespace: exporter.Namespace, - Name: exporter.Status.Credential.Name, - }, secret) - if err != nil { - return false, client.IgnoreNotFound(err) - } - - token, ok := secret.Data["token"] - - if !ok || r.Signer.UnsafeValidate(string(token)) != nil { - logger.Info("reconcileStatusCredential: the exporter secret is invalid", "exporter", exporter.Name) - return false, r.Delete(ctx, secret) - } - - return true, nil -} - func (r *ExporterReconciler) reconcileStatusCredential( ctx context.Context, exporter *jumpstarterdevv1alpha1.Exporter, ) error { - logger := log.FromContext(ctx) - - exists, err := r.exporterSecretExists(ctx, exporter) + secret, err := ensureSecret(ctx, client.ObjectKey{ + Name: exporter.Name + "-exporter", + Namespace: exporter.Namespace, + }, r.Client, r.Signer, exporter.Username(r.Signer.Prefix())) if err != nil { - logger.Info("reconcileStatusCredential: the exporter secret's existence cannot be checked", "exporter", exporter.Name) - return err + return fmt.Errorf("reconcileStatusCredential: failed to prepare credential for exporter: %w", err) } - - if !exists { - if exporter.Status.Credential != nil { - // TODO: Send an alert notification to cluster - logger.Info("reconcileStatusCredential: the exporter secret has ceased to exist, will be recreated", "exporter", exporter.Name) - } else { - logger.Info("reconcileStatusCredential: creating credential for exporter") - } - secret, err := r.secretForExporter(exporter) - if err != nil { - return fmt.Errorf("reconcileStatusCredential: failed to prepare credential for exporter: %w", err) - } - if err := r.Create(ctx, secret); err != nil { - return fmt.Errorf("reconcileStatusCredential: failed to create credential for exporter: %w", err) - } - exporter.Status.Credential = &corev1.LocalObjectReference{ - Name: secret.Name, - } + // enable garbage collection on the created resource + if err := controllerutil.SetControllerReference(exporter, secret, r.Scheme); err != nil { + return fmt.Errorf("reconcileStatusCredential: error setting owner reference: %w", err) + } + exporter.Status.Credential = &corev1.LocalObjectReference{ + Name: secret.Name, } return nil } @@ -191,29 +149,6 @@ func (r *ExporterReconciler) reconcileStatusEndpoint( return nil } -func (r *ExporterReconciler) secretForExporter(exporter *jumpstarterdevv1alpha1.Exporter) (*corev1.Secret, error) { - token, err := r.Signer.Token(exporter.Username(r.Signer.Prefix())) - if err != nil { - return nil, err - } - - secret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: exporter.Name + "-exporter", - Namespace: exporter.Namespace, - }, - Type: corev1.SecretTypeOpaque, - StringData: map[string]string{ - "token": token, - }, - } - // enable garbage collection on the created resource - if err := controllerutil.SetControllerReference(exporter, secret, r.Scheme); err != nil { - return nil, fmt.Errorf("secretForExporter, error setting owner reference: %w", err) - } - return secret, nil -} - // SetupWithManager sets up the controller with the Manager. func (r *ExporterReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/secret_helpers.go b/internal/controller/secret_helpers.go new file mode 100644 index 00000000..a6f3c0d6 --- /dev/null +++ b/internal/controller/secret_helpers.go @@ -0,0 +1,61 @@ +package controller + +import ( + "context" + + "github.com/jumpstarter-dev/jumpstarter-controller/internal/oidc" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const TokenKey string = "token" + +func ensureSecret( + ctx context.Context, + key client.ObjectKey, + kclient client.Client, + signer *oidc.Signer, + username string, +) (*corev1.Secret, error) { + var secret corev1.Secret + if err := kclient.Get(ctx, key, &secret); err != nil { + if !errors.IsNotFound(err) { + return nil, err + } + // Secret not present + token, err := signer.Token(username) + if err != nil { + return nil, err + } + secret = corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: key.Namespace, + Name: key.Name, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + TokenKey: []byte(token), + }, + } + return &secret, kclient.Create(ctx, &secret) + } else { + token, ok := secret.Data[TokenKey] + if !ok || signer.UnsafeValidate(string(token)) != nil { + // Secret present but invalid + original := client.MergeFrom(secret.DeepCopy()) + token, err := signer.Token(username) + if err != nil { + return nil, err + } + secret.Data = map[string][]byte{ + TokenKey: []byte(token), + } + return &secret, kclient.Patch(ctx, &secret, original) + } else { + // Secret present and valid + return &secret, nil + } + } +} From 2b853864406a47c317c91b49a68d8c6eb9049ef7 Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:19:08 -0500 Subject: [PATCH 2/7] Test recreating invalid secret --- internal/controller/client_controller_test.go | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/internal/controller/client_controller_test.go b/internal/controller/client_controller_test.go index 4eda0569..21aac28d 100644 --- a/internal/controller/client_controller_test.go +++ b/internal/controller/client_controller_test.go @@ -126,5 +126,44 @@ var _ = Describe("Identity Controller", func() { Name: resourceName + "-client", }, secret)).To(Succeed()) }) + + It("should reconcile an invalid token secret", func() { + By("recreating the secret") + signer, err := oidc.NewSignerFromSeed([]byte{}, "https://example.com", "dummy", "dummy:") + Expect(err).NotTo(HaveOccurred()) + + controllerReconciler := &ClientReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Signer: signer, + } + + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + // modify the secret to something invalid + secret := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Namespace: "default", + Name: resourceName + "-client", + }, secret)).To(Succeed()) + secret.Data[TokenKey] = []byte("invalid") + Expect(k8sClient.Update(ctx, secret)).To(Succeed()) + + _, err = controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + + By("verifying the secret was updated") + secret = &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Namespace: "default", + Name: resourceName + "-client", + }, secret)).To(Succeed()) + Expect(secret.Data[TokenKey]).NotTo(Equal([]byte("invalid"))) + }) }) }) From 4a7c81f1e5bece9bf49e4e2535a40f264f62817d Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:24:16 -0500 Subject: [PATCH 3/7] fixup! Dedup secret handling --- internal/controller/secret_helpers.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/controller/secret_helpers.go b/internal/controller/secret_helpers.go index a6f3c0d6..13860508 100644 --- a/internal/controller/secret_helpers.go +++ b/internal/controller/secret_helpers.go @@ -8,6 +8,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" ) const TokenKey string = "token" @@ -19,14 +20,18 @@ func ensureSecret( signer *oidc.Signer, username string, ) (*corev1.Secret, error) { + logger := log.FromContext(ctx) var secret corev1.Secret if err := kclient.Get(ctx, key, &secret); err != nil { if !errors.IsNotFound(err) { + logger.Error(err, "ensureSecret: failed to get secret") return nil, err } // Secret not present + logger.Info("ensureSecret: secret not present, creating") token, err := signer.Token(username) if err != nil { + logger.Info("ensureSecret: failed to sign token") return nil, err } secret = corev1.Secret{ @@ -39,20 +44,30 @@ func ensureSecret( TokenKey: []byte(token), }, } - return &secret, kclient.Create(ctx, &secret) + if err = kclient.Create(ctx, &secret); err != nil { + logger.Error(err, "ensureSecret: failed to create secret") + return nil, err + } + return &secret, nil } else { token, ok := secret.Data[TokenKey] if !ok || signer.UnsafeValidate(string(token)) != nil { // Secret present but invalid + logger.Info("ensureSecret: secret present but invalid, updating") original := client.MergeFrom(secret.DeepCopy()) token, err := signer.Token(username) if err != nil { + logger.Info("ensureSecret: failed to sign token") return nil, err } secret.Data = map[string][]byte{ TokenKey: []byte(token), } - return &secret, kclient.Patch(ctx, &secret, original) + if err = kclient.Patch(ctx, &secret, original); err != nil { + logger.Error(err, "ensureSecret: failed to update secret") + return nil, err + } + return &secret, nil } else { // Secret present and valid return &secret, nil From 601d207da629b2ba9b586a1b655df59d28371011 Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:29:57 -0500 Subject: [PATCH 4/7] Set logger.WithName --- internal/controller/secret_helpers.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/controller/secret_helpers.go b/internal/controller/secret_helpers.go index 13860508..e9480e98 100644 --- a/internal/controller/secret_helpers.go +++ b/internal/controller/secret_helpers.go @@ -20,18 +20,18 @@ func ensureSecret( signer *oidc.Signer, username string, ) (*corev1.Secret, error) { - logger := log.FromContext(ctx) + logger := log.FromContext(ctx).WithName("ensureSecret") var secret corev1.Secret if err := kclient.Get(ctx, key, &secret); err != nil { if !errors.IsNotFound(err) { - logger.Error(err, "ensureSecret: failed to get secret") + logger.Error(err, "failed to get secret") return nil, err } // Secret not present - logger.Info("ensureSecret: secret not present, creating") + logger.Info("secret not present, creating") token, err := signer.Token(username) if err != nil { - logger.Info("ensureSecret: failed to sign token") + logger.Info("failed to sign token") return nil, err } secret = corev1.Secret{ @@ -45,7 +45,7 @@ func ensureSecret( }, } if err = kclient.Create(ctx, &secret); err != nil { - logger.Error(err, "ensureSecret: failed to create secret") + logger.Error(err, "failed to create secret") return nil, err } return &secret, nil @@ -53,18 +53,18 @@ func ensureSecret( token, ok := secret.Data[TokenKey] if !ok || signer.UnsafeValidate(string(token)) != nil { // Secret present but invalid - logger.Info("ensureSecret: secret present but invalid, updating") + logger.Info("secret present but invalid, updating") original := client.MergeFrom(secret.DeepCopy()) token, err := signer.Token(username) if err != nil { - logger.Info("ensureSecret: failed to sign token") + logger.Info("failed to sign token") return nil, err } secret.Data = map[string][]byte{ TokenKey: []byte(token), } if err = kclient.Patch(ctx, &secret, original); err != nil { - logger.Error(err, "ensureSecret: failed to update secret") + logger.Error(err, "failed to update secret") return nil, err } return &secret, nil From 1c2da431848ed3f33ae8face404cab7ee4d3620d Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:31:22 -0500 Subject: [PATCH 5/7] fixup! Dedup secret handling --- internal/controller/secret_helpers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/secret_helpers.go b/internal/controller/secret_helpers.go index e9480e98..25050814 100644 --- a/internal/controller/secret_helpers.go +++ b/internal/controller/secret_helpers.go @@ -31,7 +31,7 @@ func ensureSecret( logger.Info("secret not present, creating") token, err := signer.Token(username) if err != nil { - logger.Info("failed to sign token") + logger.Error(err, "failed to sign token") return nil, err } secret = corev1.Secret{ @@ -57,7 +57,7 @@ func ensureSecret( original := client.MergeFrom(secret.DeepCopy()) token, err := signer.Token(username) if err != nil { - logger.Info("failed to sign token") + logger.Error(err, "failed to sign token") return nil, err } secret.Data = map[string][]byte{ From e0f8c73d3fcf728ef4037636b180472a91ea6333 Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:41:54 -0500 Subject: [PATCH 6/7] Create unversioned symlink for helper binaries --- Makefile | 22 ++++++++++++---------- hack/deploy_with_helm.sh | 4 ++-- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index cd353239..a4845f91 100644 --- a/Makefile +++ b/Makefile @@ -175,12 +175,12 @@ $(LOCALBIN): ## Tool Binaries KUBECTL ?= kubectl -KUSTOMIZE ?= $(LOCALBIN)/kustomize-$(KUSTOMIZE_VERSION) -CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen-$(CONTROLLER_TOOLS_VERSION) -ENVTEST ?= $(LOCALBIN)/setup-envtest-$(ENVTEST_VERSION) -GOLANGCI_LINT = $(LOCALBIN)/golangci-lint-$(GOLANGCI_LINT_VERSION) -KIND = $(LOCALBIN)/kind-$(KIND_VERSION) -GRPCURL = $(LOCALBIN)/grpcurl-$(GRPCURL_VERSION) +KUSTOMIZE ?= $(LOCALBIN)/kustomize +CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen +ENVTEST ?= $(LOCALBIN)/setup-envtest +GOLANGCI_LINT = $(LOCALBIN)/golangci-lint +KIND = $(LOCALBIN)/kind +GRPCURL = $(LOCALBIN)/grpcurl ## Tool Versions KUSTOMIZE_VERSION ?= v5.4.1 @@ -231,15 +231,17 @@ clean: $(KIND) # go-install-tool will 'go install' any package with custom target and name of binary, if it doesn't exist -# $1 - target path with name of binary (ideally with version) +# $1 - target path with name of binary # $2 - package url which can be installed # $3 - specific version of package define go-install-tool -@[ -f $(1) ] || { \ +@[ -f "$(1)-$(3)" ] || { \ set -e; \ package=$(2)@$(3) ;\ echo "Downloading $${package}" ;\ +rm -f $(1) || true ;\ GOBIN=$(LOCALBIN) go install $${package} ;\ -mv "$$(echo "$(1)" | sed "s/-$(3)$$//")" $(1) ;\ -} +mv $(1) $(1)-$(3) ;\ +} ;\ +ln -sf $(1)-$(3) $(1) endef diff --git a/hack/deploy_with_helm.sh b/hack/deploy_with_helm.sh index f8b0a821..db54bc67 100755 --- a/hack/deploy_with_helm.sh +++ b/hack/deploy_with_helm.sh @@ -2,8 +2,8 @@ set -eo pipefail SCRIPT_DIR="$(dirname "$(readlink -f "$0")")" -KIND=${KIND:-bin/kind-v0.23.0} -GRPCURL=${GRPCURL:-bin/grpcurl-v1.9.2} +KIND=${KIND:-bin/kind} +GRPCURL=${GRPCURL:-bin/grpcurl} IMG=${IMG:-quay.io/jumpstarter-dev/jumpstarter-controller:latest} INGRESS_ENABLED=${INGRESS_ENABLED:-false} From 1a20cd825a12e668733b7478f3aae593317bbec9 Mon Sep 17 00:00:00 2001 From: Nick Cao Date: Mon, 17 Feb 2025 15:53:11 -0500 Subject: [PATCH 7/7] Update kind to v0.27.0 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index a4845f91..c25ec3ad 100644 --- a/Makefile +++ b/Makefile @@ -187,7 +187,7 @@ KUSTOMIZE_VERSION ?= v5.4.1 CONTROLLER_TOOLS_VERSION ?= v0.16.3 ENVTEST_VERSION ?= release-0.18 GOLANGCI_LINT_VERSION ?= v1.61.0 -KIND_VERSION ?= v0.23.0 +KIND_VERSION ?= v0.27.0 GRPCURL_VERSION ?= v1.9.2 .PHONY: kustomize