Corrected Binding Insert Order (#1366)

* Corrected Binding Insert Order

* Linter

* Review Remarks - Removed Unnecessary Configs

* Test Correction

* Import Issues Fix

* Linter

* Update with Created Kubeconfig

* Corrected Memory Bindings Repository to Reflect Real Usage

* Setting ExpiresAt

* Linter

* Merge Correction

* Review Remarks - #1366 (comment)

* Linter

* Update internal/storage/driver/memory/binding.go

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

* Update internal/storage/driver/memory/binding.go

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

---------

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

internal/broker/bind_create.go

@@ -178,7 +178,6 @@ func (b *BindEndpoint) Bind(ctx context.Context, instanceID, bindingID string, d
 	}
 
 	var kubeconfig string
-	var expiresAt time.Time
 	binding := &internal.Binding{
 		ID:         bindingID,
 		InstanceID: instanceID,
@@ -187,17 +186,9 @@ func (b *BindEndpoint) Bind(ctx context.Context, instanceID, bindingID string, d
 		UpdatedAt: time.Now(),
 
 		ExpirationSeconds: int64(expirationSeconds),
+		ExpiresAt:         time.Now().Add(time.Duration(expirationSeconds) * time.Second),
 		CreatedBy:         bindingContext.CreatedBy(),
 	}
-	// get kubeconfig for the instance
-	kubeconfig, expiresAt, err = b.serviceAccountBindingManager.Create(ctx, instance, bindingID, expirationSeconds)
-	if err != nil {
-		message := fmt.Sprintf("failed to create a Kyma binding using service account's kubeconfig: %s", err)
-		return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
-	}
-
-	binding.ExpiresAt = expiresAt
-	binding.Kubeconfig = kubeconfig
 
 	err = b.bindingsStorage.Insert(binding)
 	switch {
@@ -207,7 +198,23 @@ func (b *BindEndpoint) Bind(ctx context.Context, instanceID, bindingID string, d
 	case err != nil:
 		message := fmt.Sprintf("failed to insert Kyma binding into storage: %s", err)
 		return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusInternalServerError, message)
+	}
+
+	// create kubeconfig for the instance
+	var expiresAt time.Time
+	kubeconfig, expiresAt, err = b.serviceAccountBindingManager.Create(ctx, instance, bindingID, expirationSeconds)
+	if err != nil {
+		message := fmt.Sprintf("failed to create a Kyma binding using service account's kubeconfig: %s", err)
+		return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
+	}
 
+	binding.ExpiresAt = expiresAt
+	binding.Kubeconfig = kubeconfig
+
+	err = b.bindingsStorage.Update(binding)
+	if err != nil {
+		message := fmt.Sprintf("failed to update Kyma binding in storage: %s", err)
+		return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusInternalServerError, message)
 	}
 
 	return domain.Binding{

internal/broker/bind_create_test.go

@@ -3,16 +3,120 @@ package broker
 import (
 	"context"
 	"encoding/json"
+	"fmt"
+	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"k8s.io/client-go/kubernetes"
+
+	"code.cloudfoundry.org/lager"
 	"github.com/kyma-project/kyma-environment-broker/internal/fixture"
 	"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/sirupsen/logrus"
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type Kubeconfig struct {
+	Users []User `yaml:"users"`
+}
+
+type User struct {
+	Name string `yaml:"name"`
+	User struct {
+		Token string `yaml:"token"`
+	} `yaml:"user"`
+}
+
+const (
+	instanceID1      = "1"
+	instanceID2      = "2"
+	instanceID3      = "max-bindings"
+	maxBindingsCount = 10
 )
 
+var httpServer *httptest.Server
+
+type provider struct {
+}
+
+func (p *provider) K8sClientSetForRuntimeID(runtimeID string) (kubernetes.Interface, error) {
+	return nil, fmt.Errorf("error")
+}
+
+func (p *provider) KubeconfigForRuntimeID(runtimeID string) ([]byte, error) {
+	return []byte{}, nil
+}
+
+func TestCreateBindingEndpoint(t *testing.T) {
+	t.Log("test create binding endpoint")
+
+	// Given
+	//// logger
+	logs := logrus.New()
+	logs.SetLevel(logrus.DebugLevel)
+	logs.SetFormatter(&logrus.JSONFormatter{
+		TimestampFormat: time.RFC3339Nano,
+	})
+
+	brokerLogger := lager.NewLogger("test")
+	brokerLogger.RegisterSink(lager.NewWriterSink(logs.Writer(), lager.DEBUG))
+
+	//// schema
+
+	//// database
+	db := storage.NewMemoryStorage()
+
+	err := db.Instances().Insert(fixture.FixInstance(instanceID1))
+	require.NoError(t, err)
+
+	err = db.Instances().Insert(fixture.FixInstance(instanceID2))
+	require.NoError(t, err)
+
+	err = db.Instances().Insert(fixture.FixInstance(instanceID3))
+	require.NoError(t, err)
+
+	//// binding configuration
+	bindingCfg := &BindingConfig{
+		Enabled: true,
+		BindablePlans: EnablePlans{
+			fixture.PlanName,
+		},
+		MaxBindingsCount: maxBindingsCount,
+	}
+
+	//// api handler
+	bindEndpoint := NewBind(*bindingCfg, db, logs, &provider{}, &provider{})
+
+	// test relies on checking if got nil on kubeconfig provider but the instance got inserted either way
+	t.Run("should INSERT binding despite error on k8s api call", func(t *testing.T) {
+		// given
+		_, err := db.Bindings().Get(instanceID1, "binding-id")
+		require.Error(t, err)
+		require.True(t, dberr.IsNotFound(err))
+
+		// when
+		_, err = bindEndpoint.Bind(context.Background(), instanceID1, "binding-id", domain.BindDetails{
+			ServiceID: "123",
+			PlanID:    fixture.PlanId,
+		}, false)
+
+		require.Error(t, err)
+
+		binding, err := db.Bindings().Get(instanceID1, "binding-id")
+		require.NoError(t, err)
+		require.Equal(t, instanceID1, binding.InstanceID)
+		require.Equal(t, "binding-id", binding.ID)
+
+		require.NotNil(t, binding.ExpiresAt)
+		require.Empty(t, binding.Kubeconfig)
+	})
+}
+
 func TestCreatedBy(t *testing.T) {
 	emptyStr := ""
 	email := "[email protected]"

internal/storage/driver/memory/binding.go

@@ -32,8 +32,20 @@ func (s *Binding) Insert(binding *internal.Binding) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	if _, found := s.data[binding.ID]; found {
-		return dberr.AlreadyExists("binding with id %s already exist", binding.ID)
+	if foundBinding, found := s.data[binding.ID]; found && binding.InstanceID == foundBinding.InstanceID {
+		return dberr.AlreadyExists("binding with id %s already exists", binding.ID)
+	}
+	s.data[binding.ID] = *binding
+
+	return nil
+}
+
+func (s *Binding) Update(binding *internal.Binding) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	if foundBinding, found := s.data[binding.ID]; !(found && binding.InstanceID == foundBinding.InstanceID) {
+		return dberr.AlreadyExists("binding with id %s does not exist", binding.ID)
 	}
 	s.data[binding.ID] = *binding
 

internal/storage/driver/postsql/binding.go

@@ -60,6 +60,22 @@ func (s *Binding) Insert(binding *internal.Binding) error {
 	return nil
 }
 
+func (s *Binding) Update(binding *internal.Binding) error {
+	dto, err := s.toBindingDTO(binding)
+	if err != nil {
+		return err
+	}
+
+	sess := s.NewWriteSession()
+	err = sess.UpdateBinding(dto)
+
+	if err != nil {
+		return fmt.Errorf("while updating binding with ID %s: %w", binding.ID, err)
+	}
+
+	return nil
+}
+
 func (s *Binding) Delete(instanceID, bindingID string) error {
 	sess := s.NewWriteSession()
 	return sess.DeleteBinding(instanceID, bindingID)

internal/storage/ext.go

@@ -130,6 +130,7 @@ type SubaccountStates interface {
 
 type Bindings interface {
 	Insert(binding *internal.Binding) error
+	Update(binding *internal.Binding) error
 	Get(instanceID string, bindingID string) (*internal.Binding, error)
 	ListByInstanceID(instanceID string) ([]internal.Binding, error)
 	Delete(instanceID, bindingID string) error

internal/storage/postsql/factory.go

@@ -83,6 +83,7 @@ type WriteSession interface {
 	DeleteOperationByID(operationID string) dberr.Error
 	InsertInstanceArchived(instance dbmodel.InstanceArchivedDTO) dberr.Error
 	InsertBinding(binding dbmodel.BindingDTO) dberr.Error
+	UpdateBinding(binding dbmodel.BindingDTO) dberr.Error
 	DeleteBinding(instanceID, bindingID string) dberr.Error
 }
 

internal/storage/postsql/write.go

@@ -56,6 +56,21 @@ func (ws writeSession) InsertBinding(binding dbmodel.BindingDTO) dberr.Error {
 	return nil
 }
 
+func (ws writeSession) UpdateBinding(binding dbmodel.BindingDTO) dberr.Error {
+	_, err := ws.update(BindingsTableName).
+		Set("kubeconfig", binding.Kubeconfig).
+		Set("expires_at", binding.ExpiresAt).
+		Where(dbr.Eq("id", binding.ID)).
+		Where(dbr.Eq("instance_id", binding.InstanceID)).
+		Exec()
+
+	if err != nil {
+		return dberr.Internal("Failed to update record to Binding table: %s", err)
+	}
+
+	return nil
+}
+
 func (ws writeSession) InsertInstanceArchived(instance dbmodel.InstanceArchivedDTO) dberr.Error {
 	_, err := ws.insertInto(InstancesArchivedTableName).
 		Pair("instance_id", instance.InstanceID).