From 0cb1dba812b53b064c6b93796d17d01b78af94d6 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Tue, 22 Oct 2024 08:07:24 +0200 Subject: [PATCH] fix: refactor the secretg generator controlle4 The commit is meant to: - make the code easier to understand, reducing complexity caused by nested if/else and error conditions (align happy path to the left) - remove shadowed error vars to avoid reporting misleading errors - add some basic unit test for the reconcile loop --- .../secretgenerator_controller.go | 115 +++++++------ .../secretgenerator_controller_test.go | 153 ++++++++++++++++++ 2 files changed, 216 insertions(+), 52 deletions(-) create mode 100644 controllers/secretgenerator/secretgenerator_controller_test.go diff --git a/controllers/secretgenerator/secretgenerator_controller.go b/controllers/secretgenerator/secretgenerator_controller.go index 37519d734ea..931e45d42f3 100644 --- a/controllers/secretgenerator/secretgenerator_controller.go +++ b/controllers/secretgenerator/secretgenerator_controller.go @@ -104,7 +104,6 @@ func (r *SecretGeneratorReconciler) SetupWithManager(ctx context.Context, mgr ct // based on the specified type and complexity. This will avoid possible race // conditions when a deployment mounts the secret before it is reconciled. func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - log := logf.FromContext(ctx).WithName("SecretGenerator") foundSecret := &corev1.Secret{} err := r.Client.Get(ctx, request.NamespacedName, foundSecret) if err != nil { @@ -116,70 +115,82 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl. return ctrl.Result{}, err } - owner := []metav1.OwnerReference{ - *metav1.NewControllerRef(foundSecret, foundSecret.GroupVersionKind()), - } // Generate the secret if it does not previously exist generatedSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: foundSecret.Name + "-generated", - Namespace: foundSecret.Namespace, - Labels: foundSecret.Labels, - OwnerReferences: owner, + Name: foundSecret.Name + "-generated", + Namespace: foundSecret.Namespace, }, } - generatedSecretKey := types.NamespacedName{ - Name: generatedSecret.Name, Namespace: generatedSecret.Namespace, + err = r.Client.Get(ctx, client.ObjectKeyFromObject(generatedSecret), generatedSecret) + if err == nil || !k8serr.IsNotFound(err) { + return ctrl.Result{}, err } - err = r.Client.Get(ctx, generatedSecretKey, generatedSecret) + + err = r.generateSecret(ctx, foundSecret, generatedSecret) if err != nil { - if k8serr.IsNotFound(err) { - // Generate secret random value - log.Info("Generating a random value for a secret in a namespace", - "secret", generatedSecret.Name, "namespace", generatedSecret.Namespace) + return ctrl.Result{}, err + } - secret, err := NewSecretFrom(foundSecret.GetAnnotations()) - if err != nil { - log.Error(err, "error creating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace) - return ctrl.Result{}, err - } + // Don't requeue if secret is created successfully + return ctrl.Result{}, nil +} - generatedSecret.StringData = map[string]string{ - secret.Name: secret.Value, - } +func (r *SecretGeneratorReconciler) generateSecret(ctx context.Context, foundSecret *corev1.Secret, generatedSecret *corev1.Secret) error { + log := logf.FromContext(ctx).WithName("SecretGenerator") - err = r.Client.Create(ctx, generatedSecret) - if err != nil { - return ctrl.Result{}, err - } - log.Info("Done generating secret in namespace", - "secret", generatedSecret.Name, "namespace", generatedSecret.Namespace) - // check if annotation oauth-client-route exists - if secret.OAuthClientRoute != "" { - // Get OauthClient Route - oauthClientRoute, err := r.getRoute(ctx, secret.OAuthClientRoute, request.Namespace) - if err != nil { - log.Error(err, "Unable to retrieve route from OAuthClient", "route-name", secret.OAuthClientRoute) - return ctrl.Result{}, err - } - // Generate OAuthClient for the generated secret - log.Info("Generating an OAuthClient CR for route", "route-name", oauthClientRoute.Name) - err = r.createOAuthClient(ctx, foundSecret.Name, secret.Value, oauthClientRoute.Spec.Host) - if err != nil { - log.Error(err, "error creating oauth client resource. Recreate the Secret", "secret-name", - foundSecret.Name) - - return ctrl.Result{}, err - } - } - } else { - return ctrl.Result{}, err - } + // Generate secret random value + log.Info("Generating a random value for a secret in a namespace", + "secret", generatedSecret.Name, "namespace", generatedSecret.Namespace) + + generatedSecret.Labels = foundSecret.Labels + + generatedSecret.OwnerReferences = []metav1.OwnerReference{ + *metav1.NewControllerRef(foundSecret, foundSecret.GroupVersionKind()), } - // Don't requeue if secret is created successfully - return ctrl.Result{}, err + secret, err := NewSecretFrom(foundSecret.GetAnnotations()) + if err != nil { + log.Error(err, "error creating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace) + return err + } + + generatedSecret.StringData = map[string]string{ + secret.Name: secret.Value, + } + + err = r.Client.Create(ctx, generatedSecret) + if err != nil { + return err + } + + log.Info("Done generating secret in namespace", + "secret", generatedSecret.Name, "namespace", generatedSecret.Namespace) + + // check if annotation oauth-client-route exists + if secret.OAuthClientRoute == "" { + return nil + } + + // Get OauthClient Route + oauthClientRoute, err := r.getRoute(ctx, secret.OAuthClientRoute, foundSecret.Namespace) + if err != nil { + log.Error(err, "Unable to retrieve route from OAuthClient", "route-name", secret.OAuthClientRoute) + return err + } + + // Generate OAuthClient for the generated secret + log.Info("Generating an OAuthClient CR for route", "route-name", oauthClientRoute.Name) + err = r.createOAuthClient(ctx, foundSecret.Name, secret.Value, oauthClientRoute.Spec.Host) + if err != nil { + log.Error(err, "error creating oauth client resource. Recreate the Secret", "secret-name", + foundSecret.Name) + + return err + } + + return nil } // getRoute returns an OpenShift route object. It waits until the .spec.host value exists to avoid possible race conditions, fails otherwise. diff --git a/controllers/secretgenerator/secretgenerator_controller_test.go b/controllers/secretgenerator/secretgenerator_controller_test.go new file mode 100644 index 00000000000..a0e4c30c2d4 --- /dev/null +++ b/controllers/secretgenerator/secretgenerator_controller_test.go @@ -0,0 +1,153 @@ +package secretgenerator_test + +import ( + "context" + "github.com/onsi/gomega/gstruct" + "github.com/opendatahub-io/opendatahub-operator/v2/controllers/secretgenerator" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "testing" + + . "github.com/onsi/gomega" +) + +func newFakeClient(objs ...client.Object) (client.Client, error) { + scheme := runtime.NewScheme() + utilruntime.Must(corev1.AddToScheme(scheme)) + utilruntime.Must(appsv1.AddToScheme(scheme)) + + fakeMapper := meta.NewDefaultRESTMapper(scheme.PreferredVersionAllGroups()) + for gvk := range scheme.AllKnownTypes() { + fakeMapper.Add(gvk, meta.RESTScopeNamespace) + } + + return fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(fakeMapper). + WithObjects(objs...). + Build(), + nil +} + +func TestGenerateSecret(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + secretName := "foo" + secretNs := "ns" + + // secret expected to be found + existingSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: secretNs, + Labels: map[string]string{ + "foo": "bar", + }, + Annotations: map[string]string{ + annotations.SecretNameAnnotation: "bar", + annotations.SecretTypeAnnotation: "random", + }, + }, + } + + // secret to be generated + generatedSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName + "-generated", + Namespace: secretNs, + }, + } + + cli, err := newFakeClient(&existingSecret) + g.Expect(err).ShouldNot(HaveOccurred()) + + r := secretgenerator.SecretGeneratorReconciler{ + Client: cli, + } + + _, err = r.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: existingSecret.Name, + Namespace: existingSecret.Namespace, + }, + }) + + g.Expect(err).ShouldNot(HaveOccurred()) + + err = cli.Get(ctx, client.ObjectKeyFromObject(&generatedSecret), &generatedSecret) + g.Expect(err).ShouldNot(HaveOccurred()) + + g.Expect(generatedSecret.StringData).To( + HaveKey(existingSecret.Annotations[annotations.SecretNameAnnotation])) + g.Expect(generatedSecret.Labels).To( + gstruct.MatchAllKeys(gstruct.Keys{ + "foo": Equal("bar"), + }), + ) +} + +func TestExistingSecret(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + secretName := "foo" + secretNs := "ns" + + // secret expected to be found + existingSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: secretNs, + Labels: map[string]string{ + "foo": "bar", + }, + Annotations: map[string]string{ + annotations.SecretNameAnnotation: "bar", + annotations.SecretTypeAnnotation: "random", + }, + }, + } + + // secret to be generated + generatedSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName + "-generated", + Namespace: secretNs, + }, + } + + cli, err := newFakeClient(&existingSecret, &generatedSecret) + g.Expect(err).ShouldNot(HaveOccurred()) + + r := secretgenerator.SecretGeneratorReconciler{ + Client: cli, + } + + _, err = r.Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: existingSecret.Name, + Namespace: existingSecret.Namespace, + }, + }) + + g.Expect(err).ShouldNot(HaveOccurred()) + + err = cli.Get(ctx, client.ObjectKeyFromObject(&generatedSecret), &generatedSecret) + g.Expect(err).ShouldNot(HaveOccurred()) + + // assuming an existing secret is left untouched + g.Expect(generatedSecret.Labels).To(BeEmpty()) + g.Expect(generatedSecret.StringData).To(BeEmpty()) +}