From 43c9e7bed414376f3e0039a768d807b4eb8ac059 Mon Sep 17 00:00:00 2001 From: Marcin Szwed Date: Thu, 24 Oct 2024 19:31:31 +0200 Subject: [PATCH 1/3] Handle 410 HTTP status code --- internal/broker/client.go | 18 ++++++++++++++-- internal/servicebindingcleanup/service.go | 25 ++++++++++++++--------- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/internal/broker/client.go b/internal/broker/client.go index ae00176bc..c80d749cc 100644 --- a/internal/broker/client.go +++ b/internal/broker/client.go @@ -29,6 +29,21 @@ const ( unbindTmpl = "%s%s/%s/service_bindings/%s" ) +type UnexpectedStatusCodeError struct { + ExpectedStatusCode, UnexpectedStatusCode int +} + +func NewUnexpectedStatusCodeError(expectedStatusCode, unexpectedStatusCode int) UnexpectedStatusCodeError { + return UnexpectedStatusCodeError{ + ExpectedStatusCode: expectedStatusCode, + UnexpectedStatusCode: unexpectedStatusCode, + } +} + +func (e UnexpectedStatusCodeError) Error() string { + return fmt.Sprintf("unexpected status code: want %d, got: %d", e.ExpectedStatusCode, e.UnexpectedStatusCode) +} + type ( contextDTO struct { GlobalAccountID string `json:"globalaccount_id"` @@ -317,8 +332,7 @@ func (c *Client) executeRequest(method, url string, expectedStatus int, requestB defer c.warnOnError(resp.Body.Close) if resp.StatusCode != expectedStatus { - return fmt.Errorf("got unexpected status code while calling Kyma Environment Broker: want: %d, got: %d", - expectedStatus, resp.StatusCode) + return NewUnexpectedStatusCodeError(expectedStatus, resp.StatusCode) } err = json.NewDecoder(resp.Body).Decode(responseBody) diff --git a/internal/servicebindingcleanup/service.go b/internal/servicebindingcleanup/service.go index aa54c8e54..1b63fa9cf 100644 --- a/internal/servicebindingcleanup/service.go +++ b/internal/servicebindingcleanup/service.go @@ -5,9 +5,11 @@ import ( "errors" "fmt" "log/slog" + "net/http" "time" "github.com/kyma-project/kyma-environment-broker/internal" + "github.com/kyma-project/kyma-environment-broker/internal/broker" "github.com/kyma-project/kyma-environment-broker/internal/storage" ) @@ -39,17 +41,20 @@ func (s *Service) PerformCleanup() error { slog.Info(fmt.Sprintf("Expired Service Bindings: %d", len(bindings))) if s.dryRun { return nil - } else { - slog.Info("Requesting Service Bindings removal...") - for _, binding := range bindings { - err := s.brokerClient.Unbind(binding) - if err != nil { - if errors.Is(err, context.DeadlineExceeded) { - continue - } - slog.Error(fmt.Sprintf("while sending unbind request for service binding ID %q: %s", binding.ID, err)) - break + } + slog.Info("Requesting Service Bindings removal...") + for _, binding := range bindings { + if err := s.brokerClient.Unbind(binding); err != nil { + var unexpectedStatusCodeErr broker.UnexpectedStatusCodeError + if errors.Is(err, context.DeadlineExceeded) { + continue + } + if errors.As(err, &unexpectedStatusCodeErr) && unexpectedStatusCodeErr.UnexpectedStatusCode == http.StatusGone { + slog.Info(fmt.Sprintf("instance with ID: %q does not exist for service binding with ID %q", binding.InstanceID, binding.ID)) + continue } + slog.Error(fmt.Sprintf("while sending unbind request for service binding ID %q: %s", binding.ID, err)) + break } } return nil From 5244b11e31b26c7acc76ae4a82123f5c2e6f03d7 Mon Sep 17 00:00:00 2001 From: Marcin Szwed Date: Thu, 24 Oct 2024 19:31:42 +0200 Subject: [PATCH 2/3] Add unit tests --- .../servicebindingcleanup/service_test.go | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/internal/servicebindingcleanup/service_test.go b/internal/servicebindingcleanup/service_test.go index 098fcf5a8..3190470b2 100644 --- a/internal/servicebindingcleanup/service_test.go +++ b/internal/servicebindingcleanup/service_test.go @@ -123,6 +123,83 @@ func TestServiceBindingCleanupJob(t *testing.T) { require.NoError(t, bindingsStorage.Delete(expectedBinding.InstanceID, expectedBinding.ID)) handler.setHandlerFunc(handler.deleteServiceBindingFromStorage) }) + + t.Run("should only unbind expired service binding", func(t *testing.T) { + // given + activeBinding := internal.Binding{ + ID: "active-binding-id", + InstanceID: "instance-id", + ExpiresAt: time.Now().Add(time.Hour), + } + expiredBinding := internal.Binding{ + ID: "expired-binding-id", + InstanceID: "instance-id", + ExpiresAt: time.Now().Add(-time.Hour), + } + require.NoError(t, bindingsStorage.Insert(&activeBinding)) + require.NoError(t, bindingsStorage.Insert(&expiredBinding)) + + svc := servicebindingcleanup.NewService(false, brokerClient, bindingsStorage) + + // when + err := svc.PerformCleanup() + require.NoError(t, err) + + // then + actualBinding, err := bindingsStorage.Get(activeBinding.InstanceID, activeBinding.ID) + require.NoError(t, err) + assert.Equal(t, activeBinding.ID, actualBinding.ID) + + _, err = bindingsStorage.Get(expiredBinding.InstanceID, expiredBinding.ID) + assert.True(t, dberr.IsNotFound(err)) + }) + + t.Run("should continue cleanup when unbind endpoint returns 410 status code", func(t *testing.T) { + // given + binding := internal.Binding{ + ID: "binding-id-1", + InstanceID: "instance-id-1", + ExpiresAt: time.Now().Add(-time.Hour), + } + require.NoError(t, bindingsStorage.Insert(&binding)) + + svc := servicebindingcleanup.NewService(false, brokerClient, bindingsStorage) + + // when + handler.setHandlerFunc(func(w http.ResponseWriter, r *http.Request) { + bindingID := r.PathValue("binding_id") + instanceID := r.PathValue("instance_id") + if len(bindingID) == 0 || len(instanceID) == 0 { + w.WriteHeader(http.StatusBadRequest) + return + } + _, err := bindingsStorage.Get(instanceID, bindingID) + if err != nil { + w.WriteHeader(http.StatusNotFound) + return + } + if err = bindingsStorage.Delete(instanceID, bindingID); err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusGone) + encoder := json.NewEncoder(w) + if err := encoder.Encode(apiresponses.EmptyResponse{}); err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + }) + + err := svc.PerformCleanup() + require.NoError(t, err) + + // then + _, err = bindingsStorage.Get(binding.InstanceID, binding.ID) + assert.True(t, dberr.IsNotFound(err)) + + // cleanup + handler.setHandlerFunc(handler.deleteServiceBindingFromStorage) + }) } type server struct { From 9e87a469876c7c8f54c81bf4f42660f58296867d Mon Sep 17 00:00:00 2001 From: Marcin Szwed Date: Thu, 24 Oct 2024 19:44:08 +0200 Subject: [PATCH 3/3] Improve query to db --- internal/storage/driver/postsql/binding.go | 9 +++------ internal/storage/postsql/read.go | 3 +-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/storage/driver/postsql/binding.go b/internal/storage/driver/postsql/binding.go index 45a31570a..d47233966 100644 --- a/internal/storage/driver/postsql/binding.go +++ b/internal/storage/driver/postsql/binding.go @@ -151,11 +151,8 @@ func (s *Binding) toBinding(dto dbmodel.BindingDTO) (internal.Binding, error) { func (s *Binding) toExpiredBinding(dto dbmodel.BindingDTO) (internal.Binding, error) { return internal.Binding{ - ID: dto.ID, - InstanceID: dto.InstanceID, - CreatedAt: dto.CreatedAt, - ExpirationSeconds: dto.ExpirationSeconds, - CreatedBy: dto.CreatedBy, - ExpiresAt: dto.ExpiresAt, + ID: dto.ID, + InstanceID: dto.InstanceID, + ExpiresAt: dto.ExpiresAt, }, nil } diff --git a/internal/storage/postsql/read.go b/internal/storage/postsql/read.go index 86971b735..e2eb2e4db 100644 --- a/internal/storage/postsql/read.go +++ b/internal/storage/postsql/read.go @@ -59,10 +59,9 @@ func (r readSession) ListExpiredBindings() ([]dbmodel.BindingDTO, error) { currentTime := time.Now().UTC() var bindings []dbmodel.BindingDTO _, err := r.session. - Select("*"). + Select("id", "instance_id", "expires_at"). From(BindingsTableName). Where(dbr.Lte("expires_at", currentTime)). - OrderBy("created_at"). Load(&bindings) if err != nil {