Missing Bindings' Resources Removal (#1322)

* Missing Bindings' Resources Removal

* Linter

* Review Remarks - Extracted Name and Namespace Constants

* Review Remarks

* Update internal/broker/bind_delete.go

Co-authored-by: Jarosław Pieszka <[email protected]>

* Review Remarks

---------

Co-authored-by: Jarosław Pieszka <[email protected]>

cmd/broker/bind_create_test.go

@@ -15,7 +15,10 @@ import (
 
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/uuid"
+	brokerBindings "github.com/kyma-project/kyma-environment-broker/internal/broker/bindings"
 	"gopkg.in/yaml.v2"
+	rbacv1 "k8s.io/api/rbac/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 
 	"code.cloudfoundry.org/lager"
 	"github.com/gorilla/mux"
@@ -53,17 +56,16 @@ type User struct {
 	} `yaml:"user"`
 }
 
-const expirationSeconds = 10000
-const maxExpirationSeconds = 7200
-const minExpirationSeconds = 600
-const bindingsPath = "v2/service_instances/%s/service_bindings/%s"
-const deleteParams = "?accepts_incomplete=false&service_id=%s&plan_id=%s"
-const maxBindingsCount = 10
-
 const (
-	instanceID1 = "1"
-	instanceID2 = "2"
-	instanceID3 = "max-bindings"
+	instanceID1          = "1"
+	instanceID2          = "2"
+	instanceID3          = "max-bindings"
+	expirationSeconds    = 10000
+	maxExpirationSeconds = 7200
+	minExpirationSeconds = 600
+	bindingsPath         = "v2/service_instances/%s/service_bindings/%s"
+	deleteParams         = "?accepts_incomplete=false&service_id=%s&plan_id=%s"
+	maxBindingsCount     = 10
 )
 
 var httpServer *httptest.Server
@@ -192,7 +194,7 @@ func TestCreateBindingEndpoint(t *testing.T) {
 	//// api handler
 	bindEndpoint := broker.NewBind(*bindingCfg, db.Instances(), db.Bindings(), logs, skrK8sClientProvider, skrK8sClientProvider)
 	getBindingEndpoint := broker.NewGetBinding(logs, db.Bindings())
-	unbindEndpoint := broker.NewUnbind(logs, db.Bindings())
+	unbindEndpoint := broker.NewUnbind(logs, db.Bindings(), db.Instances(), brokerBindings.NewServiceAccountBindingsManager(skrK8sClientProvider, skrK8sClientProvider))
 	apiHandler := handlers.NewApiHandler(broker.KymaEnvironmentBroker{
 		ServicesEndpoint:             nil,
 		ProvisionEndpoint:            nil,
@@ -228,7 +230,7 @@ func TestCreateBindingEndpoint(t *testing.T) {
 
 		//// verify connectivity using kubeconfig from the generated binding
 		assertClusterAccess(t, "secret-to-check-first", binding)
-		assertRolesExistence(t, "kyma-binding-binding-id", binding)
+		assertRolesExistence(t, brokerBindings.BindingName("binding-id"), binding)
 	})
 
 	t.Run("should create a new service binding with custom token expiration time", func(t *testing.T) {
@@ -366,26 +368,39 @@ func TestCreateBindingEndpoint(t *testing.T) {
 		assert.Nil(t, createdBindingIDDB)
 	})
 
-	t.Run("should selectively delete created binding", func(t *testing.T) {
+	t.Run("should selectively delete created binding and its service account resources", func(t *testing.T) {
 		// given
 		instanceFirst := "1"
+
+		//// first instance first binding
 		createdBindingIDInstanceFirstFirst, createdBindingInstanceFirstFirst := createBindingForInstanceWithRandomBindingID(instanceFirst, httpServer, t)
 
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceFirstFirst, createdBindingIDInstanceFirstFirst, instanceFirst, httpServer, db)
 
+		assertResourcesExistence(t, clientFirst, createdBindingIDInstanceFirstFirst)
+
+		//// first instance second binding
 		createdBindingIDInstanceFirstSecond, createdBindingInstanceFirstSecond := createBindingForInstanceWithRandomBindingID(instanceFirst, httpServer, t)
 
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceFirstSecond, createdBindingIDInstanceFirstSecond, instanceFirst, httpServer, db)
 
+		assertResourcesExistence(t, clientFirst, createdBindingIDInstanceFirstSecond)
+
+		//// second instance first binding
 		instanceSecond := "2"
 		createdBindingIDInstanceSecondFirst, createdBindingInstanceSecondFirst := createBindingForInstanceWithRandomBindingID(instanceSecond, httpServer, t)
 
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceSecondFirst, createdBindingIDInstanceSecondFirst, instanceSecond, httpServer, db)
 
+		assertResourcesExistence(t, clientSecond, createdBindingIDInstanceSecondFirst)
+
+		//// second instance second binding
 		createdBindingIDInstanceSecondSecond, createdBindingInstanceSecondSecond := createBindingForInstanceWithRandomBindingID(instanceSecond, httpServer, t)
 
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceSecondSecond, createdBindingIDInstanceSecondSecond, instanceSecond, httpServer, db)
 
+		assertResourcesExistence(t, clientSecond, createdBindingIDInstanceSecondSecond)
+
 		// when
 		path := fmt.Sprintf(bindingsPath+deleteParams, instanceFirst, createdBindingIDInstanceFirstFirst, "123", fixture.PlanId)
 
@@ -395,12 +410,20 @@ func TestCreateBindingEndpoint(t *testing.T) {
 		// then
 		assert.Equal(t, http.StatusOK, response.StatusCode)
 
+		assertServiceAccountsNotExists(t, clientFirst, createdBindingIDInstanceFirstFirst)
+
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceFirstSecond, createdBindingIDInstanceFirstSecond, instanceFirst, httpServer, db)
 
+		assertResourcesExistence(t, clientFirst, createdBindingIDInstanceFirstSecond)
+
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceSecondFirst, createdBindingIDInstanceSecondFirst, instanceSecond, httpServer, db)
 
+		assertResourcesExistence(t, clientSecond, createdBindingIDInstanceSecondFirst)
+
 		assertExistsAndKubeconfigCreated(t, createdBindingInstanceSecondSecond, createdBindingIDInstanceSecondSecond, instanceSecond, httpServer, db)
 
+		assertResourcesExistence(t, clientSecond, createdBindingIDInstanceSecondSecond)
+
 		removedBinding, err := db.Bindings().Get(instanceFirst, createdBindingIDInstanceFirstFirst)
 		assert.Error(t, err)
 		assert.Nil(t, removedBinding)
@@ -426,6 +449,41 @@ func TestCreateBindingEndpoint(t *testing.T) {
 	})
 }
 
+func assertResourcesExistence(t *testing.T, k8sClient client.Client, bindingID string) {
+	name := brokerBindings.BindingName(bindingID)
+
+	serviceAccount := corev1.ServiceAccount{}
+	err := k8sClient.Get(context.Background(), client.ObjectKey{Name: name, Namespace: brokerBindings.BindingNamespace}, &serviceAccount)
+	assert.NoError(t, err)
+	assert.NotNil(t, serviceAccount)
+
+	clusterRole := rbacv1.ClusterRole{}
+	err = k8sClient.Get(context.Background(), client.ObjectKey{Name: name, Namespace: brokerBindings.BindingNamespace}, &clusterRole)
+	assert.NoError(t, err)
+	assert.NotNil(t, clusterRole)
+
+	clusterRoleBinding := rbacv1.ClusterRoleBinding{}
+	err = k8sClient.Get(context.Background(), client.ObjectKey{Name: name, Namespace: brokerBindings.BindingNamespace}, &clusterRoleBinding)
+	assert.NoError(t, err)
+	assert.NotNil(t, clusterRoleBinding)
+}
+
+func assertServiceAccountsNotExists(t *testing.T, k8sClient client.Client, bindingID string) {
+	name := brokerBindings.BindingName(bindingID)
+
+	serviceAccount := corev1.ServiceAccount{}
+	err := k8sClient.Get(context.Background(), client.ObjectKey{Name: name, Namespace: brokerBindings.BindingNamespace}, &serviceAccount)
+	assert.True(t, apierrors.IsNotFound(err))
+
+	clusterRole := rbacv1.ClusterRole{}
+	err = k8sClient.Get(context.Background(), client.ObjectKey{Name: name, Namespace: brokerBindings.BindingNamespace}, &clusterRole)
+	assert.True(t, apierrors.IsNotFound(err))
+
+	clusterRoleBinding := rbacv1.ClusterRoleBinding{}
+	err = k8sClient.Get(context.Background(), client.ObjectKey{Name: name, Namespace: brokerBindings.BindingNamespace}, &clusterRoleBinding)
+	assert.True(t, apierrors.IsNotFound(err))
+}
+
 func TestCreatedBy(t *testing.T) {
 	emptyStr := ""
 	email := "[email protected]"
@@ -481,8 +539,8 @@ func TestCreatedBy(t *testing.T) {
 
 func assertExistsAndKubeconfigCreated(t *testing.T, actual domain.Binding, bindingID, instanceID string, httpServer *httptest.Server, db storage.BrokerStorage) {
 	expected, err := db.Bindings().Get(instanceID, bindingID)
-	assert.NoError(t, err)
-	assert.Equal(t, actual.Credentials.(map[string]interface{})["kubeconfig"], expected.Kubeconfig)
+	require.NoError(t, err)
+	require.Equal(t, actual.Credentials.(map[string]interface{})["kubeconfig"], expected.Kubeconfig)
 }
 
 func assertClusterAccess(t *testing.T, controlSecretName string, binding domain.Binding) {
@@ -505,7 +563,7 @@ func assertRolesExistence(t *testing.T, bindingID string, binding domain.Binding
 
 	newClient := kubeconfigClient(t, kubeconfig)
 
-	_, err := newClient.CoreV1().ServiceAccounts("kyma-system").Get(context.Background(), bindingID, v1.GetOptions{})
+	_, err := newClient.CoreV1().ServiceAccounts(brokerBindings.BindingNamespace).Get(context.Background(), bindingID, v1.GetOptions{})
 	assert.NoError(t, err)
 	_, err = newClient.RbacV1().ClusterRoles().Get(context.Background(), bindingID, v1.GetOptions{})
 	assert.NoError(t, err)
@@ -708,7 +766,7 @@ func createEnvTest(t *testing.T) (envtest.Environment, *rest.Config, client.Clie
 
 	err = skrClient.Create(context.Background(), &corev1.Namespace{
 		ObjectMeta: v1.ObjectMeta{
-			Name: "kyma-system",
+			Name: brokerBindings.BindingNamespace,
 		},
 	})
 	require.NoError(t, err)

cmd/broker/main.go

@@ -28,6 +28,7 @@ import (
 	"github.com/kyma-project/kyma-environment-broker/internal"
 	"github.com/kyma-project/kyma-environment-broker/internal/appinfo"
 	"github.com/kyma-project/kyma-environment-broker/internal/broker"
+	brokerBindings "github.com/kyma-project/kyma-environment-broker/internal/broker/bindings"
 	kebConfig "github.com/kyma-project/kyma-environment-broker/internal/config"
 	"github.com/kyma-project/kyma-environment-broker/internal/dashboard"
 	"github.com/kyma-project/kyma-environment-broker/internal/edp"
@@ -446,7 +447,7 @@ func createAPI(router *mux.Router, servicesConfig broker.ServicesConfig, planVal
 		GetInstanceEndpoint:          broker.NewGetInstance(cfg.Broker, db.Instances(), db.Operations(), kcBuilder, logs),
 		LastOperationEndpoint:        broker.NewLastOperation(db.Operations(), db.InstancesArchived(), logs),
 		BindEndpoint:                 broker.NewBind(cfg.Broker.Binding, db.Instances(), db.Bindings(), logs, clientProvider, kubeconfigProvider),
-		UnbindEndpoint:               broker.NewUnbind(logs, db.Bindings()),
+		UnbindEndpoint:               broker.NewUnbind(logs, db.Bindings(), db.Instances(), brokerBindings.NewServiceAccountBindingsManager(clientProvider, kubeconfigProvider)),
 		GetBindingEndpoint:           broker.NewGetBinding(logs, db.Bindings()),
 		LastBindingOperationEndpoint: broker.NewLastBindingOperation(logs),
 	}

internal/broker/bind_delete.go

@@ -2,19 +2,26 @@ package broker
 
 import (
 	"context"
+	"fmt"
+	"net/http"
 
+	broker "github.com/kyma-project/kyma-environment-broker/internal/broker/bindings"
 	"github.com/kyma-project/kyma-environment-broker/internal/storage"
+	"github.com/kyma-project/kyma-environment-broker/internal/storage/dberr"
 	"github.com/pivotal-cf/brokerapi/v8/domain"
+	"github.com/pivotal-cf/brokerapi/v8/domain/apiresponses"
 	"github.com/sirupsen/logrus"
 )
 
 type UnbindEndpoint struct {
-	log             logrus.FieldLogger
-	bindingsStorage storage.Bindings
+	log              logrus.FieldLogger
+	bindingsStorage  storage.Bindings
+	instancesStorage storage.Instances
+	bindingsManager  broker.BindingsManager
 }
 
-func NewUnbind(log logrus.FieldLogger, bindingsStorage storage.Bindings) *UnbindEndpoint {
-	return &UnbindEndpoint{log: log.WithField("service", "UnbindEndpoint"), bindingsStorage: bindingsStorage}
+func NewUnbind(log logrus.FieldLogger, bindingsStorage storage.Bindings, instancesStorage storage.Instances, bindingsManager broker.BindingsManager) *UnbindEndpoint {
+	return &UnbindEndpoint{log: log.WithField("service", "UnbindEndpoint"), bindingsStorage: bindingsStorage, instancesStorage: instancesStorage, bindingsManager: bindingsManager}
 }
 
 // Unbind deletes an existing service binding
@@ -25,10 +32,23 @@ func (b *UnbindEndpoint) Unbind(ctx context.Context, instanceID, bindingID strin
 	b.log.Infof("Unbind details: %+v", details)
 	b.log.Infof("Unbind asyncAllowed: %v", asyncAllowed)
 
-	err := b.bindingsStorage.Delete(instanceID, bindingID)
+	instance, err := b.instancesStorage.GetByID(instanceID)
+	switch {
+	case dberr.IsNotFound(err):
+		return domain.UnbindSpec{}, apiresponses.ErrInstanceDoesNotExist
+	case err != nil:
+		return domain.UnbindSpec{}, apiresponses.NewFailureResponse(fmt.Errorf("failed to get instance %s", instanceID), http.StatusInternalServerError, fmt.Sprintf("failed to get instance %s", instanceID))
+	}
+
+	err = b.bindingsManager.Delete(ctx, instance, bindingID)
+	if err != nil {
+		b.log.Errorf("Unbind error during removal of service account resources: %s", err)
+		return domain.UnbindSpec{}, err
+	}
 
+	err = b.bindingsStorage.Delete(instanceID, bindingID)
 	if err != nil {
-		b.log.Errorf("Unbind error: %s", err)
+		b.log.Errorf("Unbind error during removal of db entity: %v", err)
 		return domain.UnbindSpec{}, err
 	}
 

internal/broker/bindings/bindings_manager.go

@@ -17,11 +17,17 @@ import (
 	"k8s.io/client-go/kubernetes"
 )
 
+const (
+	BindingNameFormat = "kyma-binding-%s"
+	BindingNamespace  = "kyma-system"
+)
+
 type Credentials struct {
 }
 
 type BindingsManager interface {
 	Create(ctx context.Context, instance *internal.Instance, bindingID string, expirationSeconds int) (string, time.Time, error)
+	Delete(ctx context.Context, instance *internal.Instance, bindingID string) error
 }
 
 type ClientProvider interface {
@@ -51,13 +57,14 @@ func (c *ServiceAccountBindingsManager) Create(ctx context.Context, instance *in
 		return "", time.Time{}, fmt.Errorf("while creating a runtime client for binding creation: %v", err)
 	}
 
-	serviceBindingName := fmt.Sprintf("kyma-binding-%s", bindingID)
+	serviceBindingName := BindingName(bindingID)
+	fmt.Printf("Creating a service account binding for runtime %s with name %s", instance.RuntimeID, serviceBindingName)
 
-	_, err = clientset.CoreV1().ServiceAccounts("kyma-system").Create(ctx,
+	_, err = clientset.CoreV1().ServiceAccounts(BindingNamespace).Create(ctx,
 		&v1.ServiceAccount{
 			ObjectMeta: mv1.ObjectMeta{
 				Name:      serviceBindingName,
-				Namespace: "kyma-system",
+				Namespace: BindingNamespace,
 				Labels:    map[string]string{"app.kubernetes.io/managed-by": "kcp-kyma-environment-broker"},
 			},
 		}, mv1.CreateOptions{})
@@ -72,7 +79,7 @@ func (c *ServiceAccountBindingsManager) Create(ctx context.Context, instance *in
 			ObjectMeta: mv1.ObjectMeta{
 				Name:      serviceBindingName,
 				Labels:    map[string]string{"app.kubernetes.io/managed-by": "kcp-kyma-environment-broker"},
-				Namespace: "kyma-system",
+				Namespace: BindingNamespace,
 			},
 			Rules: []rbacv1.PolicyRule{
 				{
@@ -137,3 +144,40 @@ func (c *ServiceAccountBindingsManager) Create(ctx context.Context, instance *in
 
 	return string(kubeconfigContent), expiresAt, nil
 }
+
+func (c *ServiceAccountBindingsManager) Delete(ctx context.Context, instance *internal.Instance, bindingID string) error {
+	clientset, err := c.clientProvider.K8sClientSetForRuntimeID(instance.RuntimeID)
+
+	if err != nil {
+		return fmt.Errorf("while creating a runtime client for binding creation: %v", err)
+	}
+
+	serviceBindingName := BindingName(bindingID)
+
+	// remove a binding
+	err = clientset.RbacV1().ClusterRoleBindings().Delete(ctx, serviceBindingName, mv1.DeleteOptions{})
+
+	if err != nil && !apierrors.IsNotFound(err) {
+		return fmt.Errorf("while removing a cluster role binding: %v", err)
+	}
+
+	// remove a role
+	err = clientset.RbacV1().ClusterRoles().Delete(ctx, serviceBindingName, mv1.DeleteOptions{})
+
+	if err != nil && !apierrors.IsNotFound(err) {
+		return fmt.Errorf("while removing a cluster role: %v", err)
+	}
+
+	// remove an account
+	err = clientset.CoreV1().ServiceAccounts("kyma-system").Delete(ctx, serviceBindingName, mv1.DeleteOptions{})
+
+	if err != nil && !apierrors.IsNotFound(err) {
+		return fmt.Errorf("while creating a service account: %v", err)
+	}
+
+	return nil
+}
+
+func BindingName(bindingID string) string {
+	return fmt.Sprintf(BindingNameFormat, bindingID)
+}