diff --git a/cmd/broker/bind_create_test.go b/cmd/broker/bind_create_test.go index 3f5a4562ac..9f088afeca 100644 --- a/cmd/broker/bind_create_test.go +++ b/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 := "john.smith@email.com" @@ -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) diff --git a/cmd/broker/main.go b/cmd/broker/main.go index 8000075979..75eb47dc45 100644 --- a/cmd/broker/main.go +++ b/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), } diff --git a/internal/broker/bind_delete.go b/internal/broker/bind_delete.go index 2f477667d2..734344baaa 100644 --- a/internal/broker/bind_delete.go +++ b/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 } diff --git a/internal/broker/bindings/bindings_manager.go b/internal/broker/bindings/bindings_manager.go index dc36064954..52819e36d0 100644 --- a/internal/broker/bindings/bindings_manager.go +++ b/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) +}