Cleanup in case of unsuccesfull token linking (#164)

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>

Co-authored-by: Lukas Krejci <lkrejci@redhat.com>

controllers/spiaccesstokenbinding_controller.go

@@ -273,6 +273,7 @@ func (r *SPIAccessTokenBindingReconciler) linkToken(ctx context.Context, sp serv
 		return nil, fmt.Errorf("failed to lookup the token in the service provider: %w", err)
 	}
 
+	newTokenCreated := false
 	if token == nil {
 		log.FromContext(ctx).Info("creating a new token because none found for binding")
 
@@ -298,11 +299,20 @@ func (r *SPIAccessTokenBindingReconciler) linkToken(ctx context.Context, sp serv
 			r.updateBindingStatusError(ctx, binding, api.SPIAccessTokenBindingErrorReasonLinkedToken, err)
 			return nil, fmt.Errorf("failed to create the token: %w", err)
 		}
+		newTokenCreated = true
 	}
 
 	// we need to have this label so that updates to the linked SPIAccessToken are reflected here, too... We're setting
 	// up the watch to use the label to limit the scope...
 	if err := r.persistWithMatchingLabels(ctx, binding, token); err != nil {
+		// linking newly created token failed, lets cleanup it
+		if newTokenCreated {
+			log.FromContext(ctx).Error(err, "linking of the created token failed, cleaning up token.", "namespace", token.GetNamespace(), "token", token.GetName())
+			err := r.Client.Delete(ctx, token)
+			if err != nil {
+				log.FromContext(ctx).Error(err, "failed to delete token after the an unsuccessful linking attempt", "namespace", token.GetNamespace(), "token", token.GetName())
+			}
+		}
 		return nil, err
 	}
 

go.mod

@@ -24,6 +24,7 @@ require (
 	golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
 	k8s.io/api v0.24.2
+	k8s.io/apiextensions-apiserver v0.24.0
 	k8s.io/apimachinery v0.24.2
 	k8s.io/client-go v0.24.2
 	k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
@@ -204,7 +205,6 @@ require (
 	gopkg.in/square/go-jose.v2 v2.6.0 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
-	k8s.io/apiextensions-apiserver v0.24.0 // indirect
 	k8s.io/component-base v0.24.0 // indirect
 	k8s.io/klog/v2 v2.60.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20220328201542-3ee0da9b0b42 // indirect

integration_tests/spiaccessbindingcontroller_test.go

@@ -21,6 +21,8 @@ import (
 	"strings"
 	"time"
 
+	apiexv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+
 	"github.com/redhat-appstudio/service-provider-integration-operator/pkg/serviceprovider"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -641,4 +643,78 @@ var _ = Describe("Status updates", func() {
 			})
 		})
 	})
+
+	When("linking fails", func() {
+		// This simulates a situation where the CRDs and the code is out-of-sync and any updates to the binding status
+		// fail.
+		origCRD := &apiexv1.CustomResourceDefinition{}
+		testBinding := &api.SPIAccessTokenBinding{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "link-failing-",
+				Namespace:    "default",
+			},
+			Spec: api.SPIAccessTokenBindingSpec{
+				RepoUrl: "test-provider://acme/acme",
+				Secret: api.SecretSpec{
+					Type: corev1.SecretTypeBasicAuth,
+				},
+			},
+		}
+
+		BeforeEach(func() {
+			// we need to modify the cluster somehow so that updating the object status fails
+			// we will add a required property to the status schema so that the status update fails
+			Eventually(func(g Gomega) {
+				g.Expect(ITest.Client.Get(ITest.Context, client.ObjectKey{Name: "spiaccesstokenbindings.appstudio.redhat.com"}, origCRD)).To(Succeed())
+				updatedCRD := origCRD.DeepCopy()
+				status := updatedCRD.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["status"]
+				status.Properties["__test"] = apiexv1.JSONSchemaProps{
+					Type: "string",
+				}
+				requiredStatusProps := status.Required
+				requiredStatusProps = append(requiredStatusProps, "__test")
+				status.Required = requiredStatusProps
+				updatedCRD.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["status"] = status
+
+				g.Expect(ITest.Client.Update(ITest.Context, updatedCRD)).To(Succeed())
+			}).Should(Succeed())
+
+			ITest.TestServiceProvider.LookupTokenImpl = nil
+			Expect(ITest.Client.Create(ITest.Context, testBinding)).Should(Succeed())
+		})
+
+		AfterEach(func() {
+			// restore the CRD into its original state
+			Eventually(func(g Gomega) {
+				currentCRD := &apiexv1.CustomResourceDefinition{}
+				g.Expect(ITest.Client.Get(ITest.Context, client.ObjectKeyFromObject(origCRD), currentCRD)).To(Succeed())
+				currentCRD.Spec = origCRD.Spec
+				g.Expect(ITest.Client.Update(ITest.Context, currentCRD)).To(Succeed())
+			}).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				currentBinding := &api.SPIAccessTokenBinding{}
+				g.Expect(ITest.Client.Get(ITest.Context, client.ObjectKeyFromObject(testBinding), currentBinding)).To(Succeed())
+				g.Expect(ITest.Client.Delete(ITest.Context, currentBinding)).To(Succeed())
+			}).Should(Succeed())
+		})
+
+		It("should not create a token", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(ITest.Client.Get(ITest.Context, client.ObjectKeyFromObject(binding), binding)).To(Succeed())
+				g.Expect(binding.Status.LinkedAccessTokenName).To(Equal(token.Name))
+			}).Should(Succeed())
+
+			Consistently(func(g Gomega) {
+				tokens := &api.SPIAccessTokenList{}
+				g.Expect(ITest.Client.List(ITest.Context, tokens)).Should(Succeed())
+				// there should only be 1 token (the one created in the outer level). The change to the CRD makes every
+				// attempt to create a new token and link it fail and clean up the freshly created token.
+				// Because of the errors, we clean up but are left in a perpetual cycle of trying to create the linked
+				// token and failing to link it and thus the new tokens are continuously appearing and disappearing.
+				// Let's just check here that their number is not growing too much too quickly by this crude measure.
+				g.Expect(len(tokens.Items)).To(BeNumerically("<", 5))
+			}, "10s").Should(Succeed())
+		})
+	})
 })

integration_tests/suite_test.go

@@ -22,6 +22,8 @@ import (
 	"testing"
 	"time"
 
+	apiexv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+
 	"github.com/hashicorp/vault/vault"
 
 	config2 "github.com/onsi/ginkgo/config"
@@ -94,6 +96,7 @@ var _ = BeforeSuite(func() {
 	testEnv := &envtest.Environment{
 		CRDDirectoryPaths:     []string{filepath.Join("..", "config", "crd", "bases")},
 		ErrorIfCRDPathMissing: true,
+		//UseExistingCluster: pointer.BoolPtr(true),
 	}
 	ITest.TestEnvironment = testEnv
 
@@ -104,7 +107,7 @@ var _ = BeforeSuite(func() {
 	noPrivsUser, err := testEnv.AddUser(envtest.User{
 		Name:   "test-user",
 		Groups: []string{},
-	}, nil)
+	}, cfg)
 	Expect(err).NotTo(HaveOccurred())
 
 	scheme := runtime.NewScheme()
@@ -124,6 +127,9 @@ var _ = BeforeSuite(func() {
 	err = rbac.AddToScheme(scheme)
 	Expect(err).NotTo(HaveOccurred())
 
+	err = apiexv1.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+
 	//+kubebuilder:scaffold:scheme
 
 	cl, err := client.New(cfg, client.Options{Scheme: scheme})