secret-generator controller: avoid reporting 'Secret not found' error…

… in reconcile (#1312)
opendatahub-io · Oct 24, 2024 · bde4b4e · bde4b4e
1 parent 3427b72
commit bde4b4e
Show file tree

Hide file tree

Showing 2 changed files with 109 additions and 13 deletions.
diff --git a/controllers/secretgenerator/secretgenerator_controller.go b/controllers/secretgenerator/secretgenerator_controller.go
@@ -107,12 +107,17 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.
 	foundSecret := &corev1.Secret{}
 	err := r.Client.Get(ctx, request.NamespacedName, foundSecret)
 	if err != nil {
-		if k8serr.IsNotFound(err) {
-			// If Secret is deleted, delete OAuthClient if exists
-			err = r.deleteOAuthClient(ctx, request.Name)
+		if !k8serr.IsNotFound(err) {
+			return ctrl.Result{}, err
 		}
 
-		return ctrl.Result{}, err
+		// If Secret is deleted, delete OAuthClient if exists
+		err = r.deleteOAuthClient(ctx, request.NamespacedName)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+
+		return ctrl.Result{}, nil
 	}
 
 	// Generate the secret if it does not previously exist
@@ -252,17 +257,16 @@ func (r *SecretGeneratorReconciler) createOAuthClient(ctx context.Context, name
 	return err
 }
 
-func (r *SecretGeneratorReconciler) deleteOAuthClient(ctx context.Context, secretName string) error {
-	oauthClient := &oauthv1.OAuthClient{}
-
-	err := r.Client.Get(ctx, client.ObjectKey{
-		Name: secretName,
-	}, oauthClient)
-	if err != nil {
-		return client.IgnoreNotFound(err)
+func (r *SecretGeneratorReconciler) deleteOAuthClient(ctx context.Context, secretNamespacedName types.NamespacedName) error {
+	oauthClient := &oauthv1.OAuthClient{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretNamespacedName.Name,
+			Namespace: secretNamespacedName.Namespace,
+		},
 	}
 
-	if err = r.Client.Delete(ctx, oauthClient); err != nil {
+	err := r.Client.Delete(ctx, oauthClient)
+	if err != nil && !k8serr.IsNotFound(err) {
 		return fmt.Errorf("error deleting OAuthClient %s: %w", oauthClient.Name, err)
 	}
 

diff --git a/controllers/secretgenerator/secretgenerator_controller_test.go b/controllers/secretgenerator/secretgenerator_controller_test.go
@@ -5,8 +5,10 @@ import (
 	"testing"
 
 	"github.com/onsi/gomega/gstruct"
+	oauthv1 "github.com/openshift/api/oauth/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	k8serr "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -26,6 +28,7 @@ func newFakeClient(objs ...client.Object) client.Client {
 	scheme := runtime.NewScheme()
 	utilruntime.Must(corev1.AddToScheme(scheme))
 	utilruntime.Must(appsv1.AddToScheme(scheme))
+	utilruntime.Must(oauthv1.AddToScheme(scheme))
 
 	return fake.NewClientBuilder().
 		WithScheme(scheme).
@@ -152,3 +155,92 @@ func TestExistingSecret(t *testing.T) {
 	g.Expect(generatedSecret.Labels).To(BeEmpty())
 	g.Expect(generatedSecret.StringData).To(BeEmpty())
 }
+
+func TestSecretNotFound(t *testing.T) {
+	g := NewWithT(t)
+	ctx := context.Background()
+
+	secretName := "fooo"
+	secretNs := "foooNs"
+
+	cli := newFakeClient()
+
+	r := secretgenerator.SecretGeneratorReconciler{
+		Client: cli,
+	}
+
+	_, err := r.Reconcile(ctx, reconcile.Request{
+		NamespacedName: types.NamespacedName{
+			Name:      secretName,
+			Namespace: secretNs,
+		},
+	})
+	// secret not found, reconcile should end without error
+	g.Expect(err).ShouldNot(HaveOccurred())
+}
+
+func TestDeleteOAuthClientIfSecretNotFound(t *testing.T) {
+	g := NewWithT(t)
+	ctx := context.Background()
+
+	secretName := "fooo"
+	secretNs := "foooNs"
+
+	// secret expected to be deleted
+	existingSecret := corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretName,
+			Namespace: secretNs,
+			Labels: map[string]string{
+				"fooo": "bar",
+			},
+			Annotations: map[string]string{
+				annotations.SecretNameAnnotation: "bar",
+				annotations.SecretTypeAnnotation: "random",
+			},
+		},
+	}
+
+	// future left-over oauth client to be cleaned up during reconcile
+	existingOauthClient := oauthv1.OAuthClient{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "OAuthClient",
+			APIVersion: "oauth.openshift.io/v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secretName,
+			Namespace: secretNs,
+		},
+		Secret:       secretName,
+		RedirectURIs: []string{"https://foo.bar"},
+		GrantMethod:  oauthv1.GrantHandlerAuto,
+	}
+
+	cli := newFakeClient(&existingSecret, &existingOauthClient)
+
+	r := secretgenerator.SecretGeneratorReconciler{
+		Client: cli,
+	}
+
+	// delete secret
+	err := cli.Delete(ctx, &existingSecret)
+	g.Expect(err).ShouldNot(HaveOccurred())
+
+	// ensure the secret is deleted
+	err = cli.Get(ctx, client.ObjectKeyFromObject(&existingSecret), &existingSecret)
+	g.Expect(err).To(MatchError(k8serr.IsNotFound, "NotFound"))
+
+	_, err = r.Reconcile(ctx, reconcile.Request{
+		NamespacedName: types.NamespacedName{
+			Name:      secretName,
+			Namespace: secretNs,
+		},
+	})
+	// secret not found, reconcile should clean-up left-over oauth client and end without error
+	g.Expect(err).ShouldNot(HaveOccurred())
+
+	// ensure the leftover OauthClient was deleted during reconcile
+	foundOauthClient := oauthv1.OAuthClient{}
+	err = cli.Get(ctx, client.ObjectKeyFromObject(&existingOauthClient), &foundOauthClient)
+	g.Expect(err).To(MatchError(k8serr.IsNotFound, "NotFound"))
+}