From 92d959c506e7647d531b474defc5bf0257a5996f Mon Sep 17 00:00:00 2001 From: Marek Michali <56163696+MarekMichali@users.noreply.github.com> Date: Mon, 21 Oct 2024 11:40:15 +0200 Subject: [PATCH] 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 --- cmd/broker/binding_test.go | 51 +++++++++ cmd/broker/suite_test.go | 6 +- internal/broker/bind_create.go | 56 +++++++--- internal/broker/bind_create_test.go | 121 +++++++++++++++++++++ internal/storage/dberr/errors.go | 7 ++ internal/storage/dberr/errors_test.go | 3 + internal/storage/driver/postsql/binding.go | 5 +- 7 files changed, 231 insertions(+), 18 deletions(-) diff --git a/cmd/broker/binding_test.go b/cmd/broker/binding_test.go index 9b818816e8..affffb3ef5 100644 --- 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 index 0835942a99..b2a8529a63 100644 --- 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 index 85a6719091..6ab7779faf 100644 --- 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 index c7eb004e1c..1cdfcfb7bd 100644 --- 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 index 15b9ac57bc..bd25f263c0 100644 --- 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 index 8d121fe6b2..cecc500a0f 100644 --- 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 index 982885039d..f69dff73ba 100644 --- 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) }