Adjust HTTP codes for bindings PUT (#1345)

* Adjust HTTP codes for bindings PUT * Check expired * Test * Test * Test * Test * Return error * Test * Change message * Error handling * Not refned logic * First test version * Add test * Cleanup * Remove envtest * Clenup * Add more tests * Fix typ * Add comment * Change suite config * Linter
kyma-project · Oct 21, 2024 · 92d959c · 92d959c
1 parent f6d7724
commit 92d959c
Show file tree

Hide file tree

Showing 7 changed files with 231 additions and 18 deletions.
diff --git a/cmd/broker/binding_test.go b/cmd/broker/binding_test.go
@@ -64,4 +64,55 @@ func TestBinding(t *testing.T) {
 		assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
 	})
 
+	t.Run("should return 200 when creating a second binding with the same id and params as an existing one", func(t *testing.T) {
+		bid = uuid.New().String()
+		resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
+			`{
+                "service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
+                "plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
+				"parameters": {
+					"expiration_seconds": 600
+				}
+               }`)
+		resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
+			`{
+                "service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
+                "plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
+				"parameters": {
+					"expiration_seconds": 600
+				}
+               }`)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+		// "expiration_seconds": 600 is a default value from the config in tests
+		resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
+			`{
+                "service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
+                "plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15"
+               }`)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("should return 409 when creating a second binding with the same id as an existing one but different params", func(t *testing.T) {
+		bid = uuid.New().String()
+		resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
+			`{
+                "service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
+                "plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
+				"parameters": {
+					"expiration_seconds": 900
+				}
+               }`)
+		resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
+			`{
+                "service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
+                "plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
+				"parameters": {
+					"expiration_seconds": 930
+				}
+               }`)
+		assert.Equal(t, http.StatusConflict, resp.StatusCode)
+
+	})
+
 }
diff --git a/cmd/broker/suite_test.go b/cmd/broker/suite_test.go
@@ -928,10 +928,10 @@ func fixConfig() *Config {
 			Binding: broker.BindingConfig{
 				Enabled:              true,
 				BindablePlans:        []string{"aws", "azure"},
-				ExpirationSeconds:    900,
-				MaxExpirationSeconds: 1000,
+				ExpirationSeconds:    600,
+				MaxExpirationSeconds: 7200,
 				MinExpirationSeconds: 600,
-				MaxBindingsCount:     2,
+				MaxBindingsCount:     10,
 			},
 			AllowUpdateExpiredInstanceWithContext: true,
 			KimConfig: broker.KimConfig{

diff --git a/internal/broker/bind_create.go b/internal/broker/bind_create.go
@@ -118,6 +118,43 @@ func (b *BindEndpoint) Bind(ctx context.Context, instanceID, bindingID string, d
 		}
 	}
 
+	expirationSeconds := b.config.ExpirationSeconds
+	if parameters.ExpirationSeconds != 0 {
+		if parameters.ExpirationSeconds > b.config.MaxExpirationSeconds {
+			message := fmt.Sprintf("expiration_seconds cannot be greater than %d", b.config.MaxExpirationSeconds)
+			return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
+		}
+		if parameters.ExpirationSeconds < b.config.MinExpirationSeconds {
+			message := fmt.Sprintf("expiration_seconds cannot be less than %d", b.config.MinExpirationSeconds)
+			return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
+		}
+		expirationSeconds = parameters.ExpirationSeconds
+	}
+
+	bindingFromDB, err := b.bindingsStorage.Get(instanceID, bindingID)
+	if err != nil && !dberr.IsNotFound(err) {
+		message := fmt.Sprintf("failed to get Kyma binding from storage: %s", err)
+		return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusInternalServerError, message)
+	}
+	if bindingFromDB != nil {
+		if bindingFromDB.ExpirationSeconds != int64(expirationSeconds) {
+			message := fmt.Sprintf("binding already exists but with different parameters")
+			return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusConflict, message)
+		}
+		if bindingFromDB.ExpiresAt.After(time.Now()) {
+			return domain.Binding{
+				IsAsync:       false,
+				AlreadyExists: true,
+				Credentials: Credentials{
+					Kubeconfig: bindingFromDB.Kubeconfig,
+				},
+				Metadata: domain.BindingMetadata{
+					ExpiresAt: bindingFromDB.ExpiresAt.Format(expiresAtLayout),
+				},
+			}, nil
+		}
+	}
+
 	bindingList, err := b.bindingsStorage.ListByInstanceID(instanceID)
 	if err != nil {
 		message := fmt.Sprintf("failed to list Kyma bindings: %s", err)
@@ -138,19 +175,6 @@ func (b *BindEndpoint) Bind(ctx context.Context, instanceID, bindingID string, d
 		}
 	}
 
-	expirationSeconds := b.config.ExpirationSeconds
-	if parameters.ExpirationSeconds != 0 {
-		if parameters.ExpirationSeconds > b.config.MaxExpirationSeconds {
-			message := fmt.Sprintf("expiration_seconds cannot be greater than %d", b.config.MaxExpirationSeconds)
-			return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
-		}
-		if parameters.ExpirationSeconds < b.config.MinExpirationSeconds {
-			message := fmt.Sprintf("expiration_seconds cannot be less than %d", b.config.MinExpirationSeconds)
-			return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
-		}
-		expirationSeconds = parameters.ExpirationSeconds
-	}
-
 	var kubeconfig string
 	var expiresAt time.Time
 	binding := &internal.Binding{
@@ -174,7 +198,11 @@ func (b *BindEndpoint) Bind(ctx context.Context, instanceID, bindingID string, d
 	binding.Kubeconfig = kubeconfig
 
 	err = b.bindingsStorage.Insert(binding)
-	if err != nil {
+	switch {
+	case dberr.IsAlreadyExists(err):
+		message := fmt.Sprintf("failed to insert Kyma binding into storage: %s", err)
+		return domain.Binding{}, apiresponses.NewFailureResponse(fmt.Errorf(message), http.StatusBadRequest, message)
+	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)
 

diff --git a/internal/broker/bind_create_test.go b/internal/broker/bind_create_test.go
@@ -1,8 +1,15 @@
 package broker
 
 import (
+	"context"
+	"encoding/json"
 	"testing"
 
+	"github.com/google/uuid"
+	"github.com/kyma-project/kyma-environment-broker/internal/fixture"
+	"github.com/kyma-project/kyma-environment-broker/internal/storage"
+	"github.com/pivotal-cf/brokerapi/v8/domain"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -58,3 +65,117 @@ func TestCreatedBy(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateSecondBindingWithTheSameIdButDifferentParams(t *testing.T) {
+	// given
+	instanceID := uuid.New().String()
+	bindingID := uuid.New().String()
+	instance := fixture.FixInstance(instanceID)
+	bindingCfg := &BindingConfig{
+		Enabled: true,
+		BindablePlans: EnablePlans{
+			instance.ServicePlanName,
+		},
+		ExpirationSeconds:    600,
+		MaxExpirationSeconds: 7200,
+		MinExpirationSeconds: 600,
+		MaxBindingsCount:     10,
+	}
+	binding := fixture.FixBindingWithInstanceID(bindingID, instanceID)
+	st := storage.NewMemoryStorage()
+	err := st.Instances().Insert(instance)
+	assert.NoError(t, err)
+	err = st.Bindings().Insert(&binding)
+	assert.NoError(t, err)
+
+	svc := NewBind(*bindingCfg, st.Instances(), st.Bindings(), logrus.New(), nil, nil)
+	params := BindingParams{
+		ExpirationSeconds: 601,
+	}
+	rawParams, err := json.Marshal(params)
+	assert.NoError(t, err)
+	details := domain.BindDetails{
+		RawParameters: rawParams,
+	}
+
+	// when
+	_, err = svc.Bind(context.Background(), instanceID, bindingID, details, false)
+
+	// then
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "binding already exists but with different parameters")
+}
+
+func TestCreateSecondBindingWithTheSameIdAndParams(t *testing.T) {
+	// given
+	const expiresAtLayout = "2006-01-02T15:04:05.0Z"
+	instanceID := uuid.New().String()
+	bindingID := uuid.New().String()
+	instance := fixture.FixInstance(instanceID)
+	bindingCfg := &BindingConfig{
+		Enabled: true,
+		BindablePlans: EnablePlans{
+			instance.ServicePlanName,
+		},
+		ExpirationSeconds:    600,
+		MaxExpirationSeconds: 7200,
+		MinExpirationSeconds: 600,
+		MaxBindingsCount:     10,
+	}
+	binding := fixture.FixBindingWithInstanceID(bindingID, instanceID)
+	st := storage.NewMemoryStorage()
+	err := st.Instances().Insert(instance)
+	assert.NoError(t, err)
+	err = st.Bindings().Insert(&binding)
+	assert.NoError(t, err)
+
+	svc := NewBind(*bindingCfg, st.Instances(), st.Bindings(), logrus.New(), nil, nil)
+	params := BindingParams{
+		ExpirationSeconds: 600,
+	}
+	rawParams, err := json.Marshal(params)
+	assert.NoError(t, err)
+	details := domain.BindDetails{
+		RawParameters: rawParams,
+	}
+
+	// when
+	resp, err := svc.Bind(context.Background(), instanceID, bindingID, details, false)
+
+	// then
+	assert.NoError(t, err)
+	assert.Equal(t, binding.ExpiresAt.Format(expiresAtLayout), resp.Metadata.ExpiresAt)
+}
+
+func TestCreateSecondBindingWithTheSameIdAndParamsNotExplicitlyDefined(t *testing.T) {
+	// given
+	const expiresAtLayout = "2006-01-02T15:04:05.0Z"
+	instanceID := uuid.New().String()
+	bindingID := uuid.New().String()
+	instance := fixture.FixInstance(instanceID)
+	bindingCfg := &BindingConfig{
+		Enabled: true,
+		BindablePlans: EnablePlans{
+			instance.ServicePlanName,
+		},
+		ExpirationSeconds:    600,
+		MaxExpirationSeconds: 7200,
+		MinExpirationSeconds: 600,
+		MaxBindingsCount:     10,
+	}
+	binding := fixture.FixBindingWithInstanceID(bindingID, instanceID)
+	st := storage.NewMemoryStorage()
+	err := st.Instances().Insert(instance)
+	assert.NoError(t, err)
+	err = st.Bindings().Insert(&binding)
+	assert.NoError(t, err)
+
+	svc := NewBind(*bindingCfg, st.Instances(), st.Bindings(), logrus.New(), nil, nil)
+
+	// when
+	resp, err := svc.Bind(context.Background(), instanceID, bindingID, domain.BindDetails{}, false)
+
+	// then
+	assert.NoError(t, err)
+	assert.Equal(t, binding.ExpiresAt.Format(expiresAtLayout), resp.Metadata.ExpiresAt)
+}
diff --git a/internal/storage/dberr/errors.go b/internal/storage/dberr/errors.go
@@ -57,6 +57,13 @@ func AlreadyExists(format string, a ...interface{}) Error {
 	return errorf(CodeAlreadyExists, format, a...)
 }
 
+func IsAlreadyExists(err error) bool {
+	nf, ok := err.(interface {
+		Code() int
+	})
+	return ok && nf.Code() == CodeAlreadyExists
+}
+
 func Conflict(format string, a ...interface{}) Error {
 	return errorf(CodeConflict, format, a...)
 }

diff --git a/internal/storage/dberr/errors_test.go b/internal/storage/dberr/errors_test.go
@@ -64,15 +64,18 @@ func TestAppError(t *testing.T) {
 		internalErr := Internal("Some Internal apperror, %s", "Some pkg err")
 		notFoundErr := NotFound("Some NotFound apperror, %s", "Some pkg err")
 		conflict := Conflict("some conflict %s", "error")
+		alreadyExistsErr := AlreadyExists("Some AlreadyExists apperror, %s", "Some pkg err")
 
 		//when
 		checkOne := IsNotFound(internalErr)
 		checkTwo := IsNotFound(notFoundErr)
 		checkConflict := IsConflict(conflict)
+		checkAlreadyExists := IsAlreadyExists(alreadyExistsErr)
 
 		//then
 		assert.False(t, checkOne)
 		assert.True(t, checkTwo)
 		assert.True(t, checkConflict)
+		assert.True(t, checkAlreadyExists)
 	})
 }
diff --git a/internal/storage/driver/postsql/binding.go b/internal/storage/driver/postsql/binding.go
@@ -50,7 +50,10 @@ func (s *Binding) Insert(binding *internal.Binding) error {
 	sess := s.NewWriteSession()
 	err = sess.InsertBinding(dto)
 
-	if err != nil {
+	switch {
+	case dberr.IsAlreadyExists(err):
+		return dberr.AlreadyExists("while saving binding with ID %s: %v", binding.ID, err)
+	case err != nil:
 		return fmt.Errorf("while saving binding with ID %s: %w", binding.ID, err)
 	}