From b4fc25f37d6d861050b97f076555401e24d995d4 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 11:52:03 +0300 Subject: [PATCH 01/49] recognize better transient error --- client/sm/client.go | 6 ++++-- controllers/base_controller.go | 20 ++++++++++++++++++-- go.mod | 1 + go.sum | 4 ++++ sapbtp-operator-charts/values.yaml | 6 ++++++ 5 files changed, 33 insertions(+), 4 deletions(-) diff --git a/client/sm/client.go b/client/sm/client.go index 6b888c1e..08d5c81d 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -21,6 +21,7 @@ import ( "context" "encoding/json" "fmt" + osbc "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "io" "net/http" "reflect" @@ -67,8 +68,9 @@ type Client interface { Call(method string, smpath string, body io.Reader, q *Parameters) (*http.Response, error) } type ServiceManagerError struct { - Message string - StatusCode int + Message string + StatusCode int + BrokerError *osbc.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { diff --git a/controllers/base_controller.go b/controllers/base_controller.go index b8741bb0..e226221b 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -306,12 +306,28 @@ func isTransientError(ctx context.Context, err error) bool { log := GetLogger(ctx) if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) - return smError.StatusCode == http.StatusTooManyRequests || smError.StatusCode == http.StatusServiceUnavailable || - smError.StatusCode == http.StatusGatewayTimeout || smError.StatusCode == http.StatusNotFound || smError.StatusCode == http.StatusBadGateway + if isBrokerStatusCodeExist(smError) { + log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) + } + if isTransientStatusCode(smError.StatusCode) { + return true + } + if smError.StatusCode == http.StatusBadGateway { + return isBrokerStatusCodeExist(smError) && isTransientStatusCode(smError.BrokerError.StatusCode) + } } return false } +func isTransientStatusCode(StatusCode int) bool { + return StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable || + StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound +} + +func isBrokerStatusCodeExist(smError *sm.ServiceManagerError) bool { + return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0 +} + func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, nonTransientErr error, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) setFailureConditions(operationType, nonTransientErr.Error(), object) diff --git a/go.mod b/go.mod index 527ace73..140dd1f7 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/go-logr/logr v1.2.4 github.com/google/uuid v1.3.0 github.com/kelseyhightower/envconfig v1.4.0 + github.com/kubernetes-sigs/go-open-service-broker-client v0.0.0-20200527163240-4406bd2cb6b8 github.com/onsi/ginkgo v1.16.5 github.com/onsi/gomega v1.27.7 golang.org/x/oauth2 v0.5.0 diff --git a/go.sum b/go.sum index 74895516..37e7956f 100644 --- a/go.sum +++ b/go.sum @@ -24,6 +24,7 @@ github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMo github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ= github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY= github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw= +github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.2.4 h1:g01GSCwiDw2xSZfjJ2/T9M+S6pFdcNtFYsp+Y43HYDQ= github.com/go-logr/logr v1.2.4/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= @@ -92,6 +93,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/kubernetes-sigs/go-open-service-broker-client v0.0.0-20200527163240-4406bd2cb6b8 h1:37JsHMAiuoyIFPyYUJ6jOTXznsmcDZec6tkPod+Tqto= +github.com/kubernetes-sigs/go-open-service-broker-client v0.0.0-20200527163240-4406bd2cb6b8/go.mod h1:5VFrdwwxqkzCF3pL7MY5Om1btQ6UsxO87DyjZFO0s5M= github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0= github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zkfA9PSy5pEvNWRP0ET0TIVo= @@ -289,6 +292,7 @@ k8s.io/client-go v0.27.3 h1:7dnEGHZEJld3lYwxvLl7WoehK6lAq7GvgjxpA3nv1E8= k8s.io/client-go v0.27.3/go.mod h1:2MBEKuTo6V1lbKy3z1euEGnhPfGZLKTS9tiJ2xodM48= k8s.io/component-base v0.27.2 h1:neju+7s/r5O4x4/txeUONNTS9r1HsPbyoPBAtHsDCpo= k8s.io/component-base v0.27.2/go.mod h1:5UPk7EjfgrfgRIuDBFtsEFAe4DAvP3U+M8RTzoSJkpo= +k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw= k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f h1:2kWPakN3i/k81b0gvD5C5FJ2kxm1WrQFanWchyKuqGg= diff --git a/sapbtp-operator-charts/values.yaml b/sapbtp-operator-charts/values.yaml index 15f3065b..6ff28a31 100644 --- a/sapbtp-operator-charts/values.yaml +++ b/sapbtp-operator-charts/values.yaml @@ -93,3 +93,9 @@ externalImages: repository: bitnami/kubectl sha: "" tag: latest +externalImages: + kubectl: + image: + repository: bitnami/kubectl + sha: "" + tag: latest From 928937dc21eaea3ec328a89f31a53cf3dc3e53b1 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 12:07:28 +0300 Subject: [PATCH 02/49] recognize better transient error --- sapbtp-operator-charts/values.yaml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/sapbtp-operator-charts/values.yaml b/sapbtp-operator-charts/values.yaml index 6ff28a31..15f3065b 100644 --- a/sapbtp-operator-charts/values.yaml +++ b/sapbtp-operator-charts/values.yaml @@ -93,9 +93,3 @@ externalImages: repository: bitnami/kubectl sha: "" tag: latest -externalImages: - kubectl: - image: - repository: bitnami/kubectl - sha: "" - tag: latest From c9c1c42e75dc05f843bf9945ed188bc3318eb9c6 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 13:38:00 +0300 Subject: [PATCH 03/49] recognize better transient error --- api/v1alpha1/zz_generated.deepcopy.go | 2 +- client/sm/client.go | 5 +++-- controllers/base_controller.go | 6 ++---- controllers/serviceinstance_controller_test.go | 5 +++-- controllers/suite_test.go | 3 ++- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 86815d6b..1411b6b7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package v1alpha1 import ( - "k8s.io/api/authentication/v1" + v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) diff --git a/client/sm/client.go b/client/sm/client.go index 08d5c81d..4a4699d7 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -21,7 +21,6 @@ import ( "context" "encoding/json" "fmt" - osbc "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "io" "net/http" "reflect" @@ -29,6 +28,8 @@ import ( "strconv" "strings" + v2 "github.com/kubernetes-sigs/go-open-service-broker-client/v2" + "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/SAP/sap-btp-service-operator/internal/auth" "github.com/SAP/sap-btp-service-operator/internal/httputil" @@ -70,7 +71,7 @@ type Client interface { type ServiceManagerError struct { Message string StatusCode int - BrokerError *osbc.HTTPStatusCodeError `json:"broker_error,omitempty"` + BrokerError *v2.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { diff --git a/controllers/base_controller.go b/controllers/base_controller.go index e226221b..4247dead 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -137,10 +137,8 @@ func (r *BaseReconciler) updateStatus(ctx context.Context, object api.SAPBTPReso func (r *BaseReconciler) init(ctx context.Context, obj api.SAPBTPResource) error { obj.SetReady(metav1.ConditionFalse) setInProgressConditions(smClientTypes.CREATE, "Pending", obj) - if err := r.updateStatus(ctx, obj); err != nil { - return err - } - return nil + err := r.updateStatus(ctx, obj) + return err } func getConditionReason(opType smClientTypes.OperationCategory, state smClientTypes.OperationState) string { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 43ecb0e6..164f2847 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -2,11 +2,12 @@ package controllers import ( "context" + "net/http" + "strings" + "github.com/SAP/sap-btp-service-operator/api" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/utils/pointer" - "net/http" - "strings" "fmt" diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 5b674872..e838e0dc 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -21,10 +21,11 @@ import ( "crypto/tls" "net" "path/filepath" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "testing" "time" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/api/v1/webhooks" "github.com/SAP/sap-btp-service-operator/client/sm" From a858fbfaf69409051528fab665ccefc1871320e1 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 13:39:01 +0300 Subject: [PATCH 04/49] recognize better transient error --- controllers/base_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 4247dead..f6a55f4c 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -137,8 +137,8 @@ func (r *BaseReconciler) updateStatus(ctx context.Context, object api.SAPBTPReso func (r *BaseReconciler) init(ctx context.Context, obj api.SAPBTPResource) error { obj.SetReady(metav1.ConditionFalse) setInProgressConditions(smClientTypes.CREATE, "Pending", obj) - err := r.updateStatus(ctx, obj) - return err + return r.updateStatus(ctx, obj) + } func getConditionReason(opType smClientTypes.OperationCategory, state smClientTypes.OperationState) string { From 1af0fdf31a2c6f93220598fec63fefeac7c3d00d Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 13:39:07 +0300 Subject: [PATCH 05/49] recognize better transient error --- controllers/base_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index f6a55f4c..0cb89b8d 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -138,7 +138,6 @@ func (r *BaseReconciler) init(ctx context.Context, obj api.SAPBTPResource) error obj.SetReady(metav1.ConditionFalse) setInProgressConditions(smClientTypes.CREATE, "Pending", obj) return r.updateStatus(ctx, obj) - } func getConditionReason(opType smClientTypes.OperationCategory, state smClientTypes.OperationState) string { From cfcc2ec2333a6cd2b0a4716e74a6cdafa6b24543 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:43:21 +0300 Subject: [PATCH 06/49] tests --- .../serviceinstance_controller_test.go | 60 ++++++++++++++++--- 1 file changed, 51 insertions(+), 9 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 164f2847..09f95f5e 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -2,15 +2,15 @@ package controllers import ( "context" + "fmt" "net/http" "strings" "github.com/SAP/sap-btp-service-operator/api" + v2 "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/utils/pointer" - "fmt" - v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" "github.com/SAP/sap-btp-service-operator/client/sm/smfakes" @@ -30,12 +30,11 @@ import ( // +kubebuilder:docs-gen:collapse=Imports const ( - fakeInstanceID = "ic-fake-instance-id" - fakeInstanceExternalName = "ic-test-instance-external-name" - fakeInstanceExternalNameNonShared = "ic-test-instance-external-name-non-shared" - testNamespace = "ic-test-namespace" - fakeOfferingName = "offering-a" - fakePlanName = "plan-a" + fakeInstanceID = "ic-fake-instance-id" + fakeInstanceExternalName = "ic-test-instance-external-name" + testNamespace = "ic-test-namespace" + fakeOfferingName = "offering-a" + fakePlanName = "plan-a" ) var _ = Describe("ServiceInstance controller", func() { @@ -275,7 +274,6 @@ var _ = Describe("ServiceInstance controller", func() { Message: errMessage, }) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) - }) It("should have failure condition", func() { @@ -310,6 +308,25 @@ var _ = Describe("ServiceInstance controller", func() { Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) }) }) + + Context("with 502 and broker 429", func() { + JustBeforeEach(func() { + fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ + StatusCode: http.StatusBadGateway, + Message: "errMessage", + BrokerError: &v2.HTTPStatusCodeError{ + StatusCode: http.StatusTooManyRequests, + }, + }) + fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + }) + + It("should be transient error and eventually succeed", func() { + serviceInstance = createInstance(ctx, instanceSpec, true) + Expect(len(serviceInstance.Status.Conditions)).To(Equal(2)) + Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) + }) + }) }) }) @@ -534,6 +551,31 @@ var _ = Describe("ServiceInstance controller", func() { }) }) }) + Context("spec is changed, sm returns 502 and broker returns 429", func() { + JustBeforeEach(func() { + fakeClient.ProvisionReturnsOnCall(0, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + fakeClient.UpdateInstanceReturns(nil, "", &sm.ServiceManagerError{ + StatusCode: http.StatusBadGateway, + Message: "errMessage", + BrokerError: &v2.HTTPStatusCodeError{ + StatusCode: http.StatusTooManyRequests, + }, + }) + fakeClient.UpdateInstanceReturns(nil, "", nil) + }) + + It("recognize the error as transient and eventually succeed", func() { + serviceInstance = createInstance(ctx, instanceSpec, true) + Expect(len(serviceInstance.Status.Conditions)).To(Equal(2)) + Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) + newSpec := updateSpec() + serviceInstance.Spec = newSpec + serviceInstance = updateInstance(ctx, serviceInstance) + Expect(serviceInstance.Spec.ExternalName).To(Equal(newSpec.ExternalName)) + Expect(serviceInstance.Status.Conditions[0].Reason).To(Equal(Updated)) + Expect(serviceInstance.Spec.UserInfo).NotTo(BeNil()) + }) + }) Context("Async", func() { When("spec is changed", func() { From ea4c1b72506bf8c5242d5c14a2a4bf5aee6557a3 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:48:23 +0300 Subject: [PATCH 07/49] tests --- controllers/base_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 0cb89b8d..d302f5f9 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -305,13 +305,13 @@ func isTransientError(ctx context.Context, err error) bool { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) if isBrokerStatusCodeExist(smError) { log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) + if smError.StatusCode == http.StatusBadGateway && isTransientStatusCode(smError.BrokerError.StatusCode) { + return true + } } if isTransientStatusCode(smError.StatusCode) { return true } - if smError.StatusCode == http.StatusBadGateway { - return isBrokerStatusCodeExist(smError) && isTransientStatusCode(smError.BrokerError.StatusCode) - } } return false } From 48ba5a00e7758c6a5d6fa618a5b3de430f19571b Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:48:57 +0300 Subject: [PATCH 08/49] tests --- api/v1alpha1/zz_generated.deepcopy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 1411b6b7..86815d6b 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ limitations under the License. package v1alpha1 import ( - v1 "k8s.io/api/authentication/v1" + "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) From adae8e9d9481d373f59ca0b5c31643cb83198a93 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:51:31 +0300 Subject: [PATCH 09/49] tests --- client/sm/client.go | 5 ++--- controllers/serviceinstance_controller_test.go | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/client/sm/client.go b/client/sm/client.go index 4a4699d7..08d5c81d 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -21,6 +21,7 @@ import ( "context" "encoding/json" "fmt" + osbc "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "io" "net/http" "reflect" @@ -28,8 +29,6 @@ import ( "strconv" "strings" - v2 "github.com/kubernetes-sigs/go-open-service-broker-client/v2" - "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/SAP/sap-btp-service-operator/internal/auth" "github.com/SAP/sap-btp-service-operator/internal/httputil" @@ -71,7 +70,7 @@ type Client interface { type ServiceManagerError struct { Message string StatusCode int - BrokerError *v2.HTTPStatusCodeError `json:"broker_error,omitempty"` + BrokerError *osbc.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 09f95f5e..2bbb9761 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -7,7 +7,7 @@ import ( "strings" "github.com/SAP/sap-btp-service-operator/api" - v2 "github.com/kubernetes-sigs/go-open-service-broker-client/v2" + osbc "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/utils/pointer" @@ -314,7 +314,7 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, Message: "errMessage", - BrokerError: &v2.HTTPStatusCodeError{ + BrokerError: &osbc.HTTPStatusCodeError{ StatusCode: http.StatusTooManyRequests, }, }) @@ -557,7 +557,7 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.UpdateInstanceReturns(nil, "", &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, Message: "errMessage", - BrokerError: &v2.HTTPStatusCodeError{ + BrokerError: &osbc.HTTPStatusCodeError{ StatusCode: http.StatusTooManyRequests, }, }) From 702076a194a7a24a4b64416a483b0d7380c91613 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:52:25 +0300 Subject: [PATCH 10/49] tests --- controllers/base_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index d302f5f9..5ebfbcbb 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -303,7 +303,7 @@ func isTransientError(ctx context.Context, err error) bool { log := GetLogger(ctx) if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) - if isBrokerStatusCodeExist(smError) { + if isBrokerErrorExist(smError) { log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) if smError.StatusCode == http.StatusBadGateway && isTransientStatusCode(smError.BrokerError.StatusCode) { return true @@ -321,7 +321,7 @@ func isTransientStatusCode(StatusCode int) bool { StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound } -func isBrokerStatusCodeExist(smError *sm.ServiceManagerError) bool { +func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0 } From 943c067af872071d536008d6e98543e743b16934 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:54:34 +0300 Subject: [PATCH 11/49] tests --- controllers/serviceinstance_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 2bbb9761..f61eb8c7 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -309,7 +309,7 @@ var _ = Describe("ServiceInstance controller", func() { }) }) - Context("with 502 and broker 429", func() { + Context("with sm status code 502 and broker status code 429", func() { JustBeforeEach(func() { fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, From 9ecc34304ced8faec32488bc9b0b28de58f284e5 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 15:56:01 +0300 Subject: [PATCH 12/49] tests --- .../serviceinstance_controller_test.go | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index f61eb8c7..3f5bcaa9 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -311,13 +311,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("with sm status code 502 and broker status code 429", func() { JustBeforeEach(func() { - fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ - StatusCode: http.StatusBadGateway, - Message: "errMessage", - BrokerError: &osbc.HTTPStatusCodeError{ - StatusCode: http.StatusTooManyRequests, - }, - }) + fakeClient.ProvisionReturns(nil, getTransientBrokerError()) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) @@ -554,13 +548,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("spec is changed, sm returns 502 and broker returns 429", func() { JustBeforeEach(func() { fakeClient.ProvisionReturnsOnCall(0, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) - fakeClient.UpdateInstanceReturns(nil, "", &sm.ServiceManagerError{ - StatusCode: http.StatusBadGateway, - Message: "errMessage", - BrokerError: &osbc.HTTPStatusCodeError{ - StatusCode: http.StatusTooManyRequests, - }, - }) + fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError()) fakeClient.UpdateInstanceReturns(nil, "", nil) }) @@ -1139,6 +1127,16 @@ var _ = Describe("ServiceInstance controller", func() { }) }) +func getTransientBrokerError() error { + return &sm.ServiceManagerError{ + StatusCode: http.StatusBadGateway, + Message: "errMessage", + BrokerError: &osbc.HTTPStatusCodeError{ + StatusCode: http.StatusTooManyRequests, + }, + } +} + func instanceSharingReturnSuccess() { fakeClient.ShareInstanceReturns(nil) } From c284392ff9714e7869323bef42d1c6f2a917cc4d Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 16:38:16 +0300 Subject: [PATCH 13/49] tests --- .../serviceinstance_controller_test.go | 47 ++++++++++++++----- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 3f5bcaa9..8eafe895 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -266,7 +266,7 @@ var _ = Describe("ServiceInstance controller", func() { When("provision request to SM fails", func() { var errMessage string - Context("with 400 status", func() { + FContext("with 400 status", func() { JustBeforeEach(func() { errMessage = "failed to provision instance" fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ @@ -278,17 +278,7 @@ var _ = Describe("ServiceInstance controller", func() { It("should have failure condition", func() { serviceInstance = createInstance(ctx, instanceSpec, false) - Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - if err != nil { - return false - } - if len(serviceInstance.Status.Conditions) != 3 { - return false - } - cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) - return cond != nil && cond.Status == metav1.ConditionFalse && strings.Contains(cond.Message, errMessage) - }, timeout, interval).Should(BeTrue()) + expectForInstanceFailure(ctx, defaultLookupKey, serviceInstance, errMessage) }) }) @@ -321,6 +311,25 @@ var _ = Describe("ServiceInstance controller", func() { Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) }) }) + + Context("with sm status code 502 and broker status code 400", func() { + JustBeforeEach(func() { + errMessage = "failed to provision instance" + fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ + StatusCode: http.StatusBadRequest, + Message: errMessage, + BrokerError: &osbc.HTTPStatusCodeError{ + StatusCode: 400, + }, + }) + fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + }) + + It("should have failure condition - non transient error", func() { + serviceInstance = createInstance(ctx, instanceSpec, false) + expectForInstanceFailure(ctx, defaultLookupKey, serviceInstance, errMessage) + }) + }) }) }) @@ -1137,6 +1146,20 @@ func getTransientBrokerError() error { } } +func expectForInstanceFailure(ctx context.Context, defaultLookupKey types.NamespacedName, serviceInstance *v1.ServiceInstance, errMessage string) { + Eventually(func() bool { + err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance) + if err != nil { + return false + } + if len(serviceInstance.Status.Conditions) != 3 { + return false + } + cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) + return cond != nil && cond.Status == metav1.ConditionFalse && strings.Contains(cond.Message, errMessage) + }, timeout, interval).Should(BeTrue()) +} + func instanceSharingReturnSuccess() { fakeClient.ShareInstanceReturns(nil) } From 4b1bf1f7e945ce3d69f1c93aa48d39b1f58d4d87 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 16:38:52 +0300 Subject: [PATCH 14/49] tests --- controllers/serviceinstance_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 8eafe895..d01e074e 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -266,7 +266,7 @@ var _ = Describe("ServiceInstance controller", func() { When("provision request to SM fails", func() { var errMessage string - FContext("with 400 status", func() { + Context("with 400 status", func() { JustBeforeEach(func() { errMessage = "failed to provision instance" fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ From f809d610203f093af764662139fd16fc10611d97 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 16:41:34 +0300 Subject: [PATCH 15/49] tests --- controllers/serviceinstance_controller_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index d01e074e..6a448e25 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -315,13 +315,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("with sm status code 502 and broker status code 400", func() { JustBeforeEach(func() { errMessage = "failed to provision instance" - fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Message: errMessage, - BrokerError: &osbc.HTTPStatusCodeError{ - StatusCode: 400, - }, - }) + fakeClient.ProvisionReturns(nil, getNonTransientBrokerError(errMessage)) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) @@ -1136,6 +1130,15 @@ var _ = Describe("ServiceInstance controller", func() { }) }) +func getNonTransientBrokerError(errMessage string) error { + return &sm.ServiceManagerError{ + StatusCode: http.StatusBadRequest, + Message: errMessage, + BrokerError: &osbc.HTTPStatusCodeError{ + StatusCode: 400, + }} +} + func getTransientBrokerError() error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, From 2eeab58c6d56c800995c23d6e1ec000fe88683ae Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 16:42:14 +0300 Subject: [PATCH 16/49] tests --- controllers/serviceinstance_controller_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 6a448e25..509e42b3 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -278,7 +278,7 @@ var _ = Describe("ServiceInstance controller", func() { It("should have failure condition", func() { serviceInstance = createInstance(ctx, instanceSpec, false) - expectForInstanceFailure(ctx, defaultLookupKey, serviceInstance, errMessage) + expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errMessage) }) }) @@ -321,7 +321,7 @@ var _ = Describe("ServiceInstance controller", func() { It("should have failure condition - non transient error", func() { serviceInstance = createInstance(ctx, instanceSpec, false) - expectForInstanceFailure(ctx, defaultLookupKey, serviceInstance, errMessage) + expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errMessage) }) }) }) @@ -1149,7 +1149,7 @@ func getTransientBrokerError() error { } } -func expectForInstanceFailure(ctx context.Context, defaultLookupKey types.NamespacedName, serviceInstance *v1.ServiceInstance, errMessage string) { +func expectForInstanceCreationFailure(ctx context.Context, defaultLookupKey types.NamespacedName, serviceInstance *v1.ServiceInstance, errMessage string) { Eventually(func() bool { err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance) if err != nil { From 07423f9b09156ba0283bb7cf056a217f5d691784 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 16:50:00 +0300 Subject: [PATCH 17/49] tests --- client/sm/client.go | 4 ++-- controllers/serviceinstance_controller_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/client/sm/client.go b/client/sm/client.go index 08d5c81d..d609ffeb 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -21,7 +21,6 @@ import ( "context" "encoding/json" "fmt" - osbc "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "io" "net/http" "reflect" @@ -32,6 +31,7 @@ import ( "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/SAP/sap-btp-service-operator/internal/auth" "github.com/SAP/sap-btp-service-operator/internal/httputil" + v2 "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "golang.org/x/oauth2" "golang.org/x/oauth2/clientcredentials" ) @@ -70,7 +70,7 @@ type Client interface { type ServiceManagerError struct { Message string StatusCode int - BrokerError *osbc.HTTPStatusCodeError `json:"broker_error,omitempty"` + BrokerError *v2.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 509e42b3..5f3488a7 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -7,7 +7,7 @@ import ( "strings" "github.com/SAP/sap-btp-service-operator/api" - osbc "github.com/kubernetes-sigs/go-open-service-broker-client/v2" + "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/utils/pointer" @@ -1134,7 +1134,7 @@ func getNonTransientBrokerError(errMessage string) error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, Message: errMessage, - BrokerError: &osbc.HTTPStatusCodeError{ + BrokerError: &v2.HTTPStatusCodeError{ StatusCode: 400, }} } @@ -1143,7 +1143,7 @@ func getTransientBrokerError() error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, Message: "errMessage", - BrokerError: &osbc.HTTPStatusCodeError{ + BrokerError: &v2.HTTPStatusCodeError{ StatusCode: http.StatusTooManyRequests, }, } From f15314cc784ccf99970958002f2a0c0113727800 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 12 Jul 2023 16:58:45 +0300 Subject: [PATCH 18/49] tests --- controllers/servicebinding_controller_test.go | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index aea216f2..0dd29f59 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -538,6 +538,31 @@ var _ = Describe("ServiceBinding controller", func() { }) }) + When("SM returned error 502 and broker returned 429", func() { + BeforeEach(func() { + errorMessage = "too many requests" + fakeClient.BindReturnsOnCall(0, nil, "", getTransientBrokerError()) + fakeClient.BindReturnsOnCall(1, &smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) + }) + + It("should eventually succeed", func() { + b, err := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", true) + Expect(err).ToNot(HaveOccurred()) + Expect(isReady(b)).To(BeTrue()) + }) + }) + + When("SM returned 502 and broker returned 400", func() { + BeforeEach(func() { + errorMessage = "very bad request" + fakeClient.BindReturnsOnCall(0, nil, "", getNonTransientBrokerError(errorMessage)) + }) + + It("should fail", func() { + createBindingWithError(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage) + }) + }) + }) When("SM returned invalid credentials json", func() { From f69dca83a566f64282527d9dac43f2d49b13b50a Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 16 Jul 2023 10:43:01 +0300 Subject: [PATCH 19/49] fix --- controllers/servicebinding_controller_test.go | 4 ++-- controllers/serviceinstance_controller_test.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 0dd29f59..fb736a67 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -545,7 +545,7 @@ var _ = Describe("ServiceBinding controller", func() { fakeClient.BindReturnsOnCall(1, &smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) }) - It("should eventually succeed", func() { + It("should detect the error as transient and eventually succeed", func() { b, err := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", true) Expect(err).ToNot(HaveOccurred()) Expect(isReady(b)).To(BeTrue()) @@ -558,7 +558,7 @@ var _ = Describe("ServiceBinding controller", func() { fakeClient.BindReturnsOnCall(0, nil, "", getNonTransientBrokerError(errorMessage)) }) - It("should fail", func() { + It("should detect the error as non-transient and fail", func() { createBindingWithError(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage) }) }) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 5f3488a7..a61b0e07 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -1151,8 +1151,7 @@ func getTransientBrokerError() error { func expectForInstanceCreationFailure(ctx context.Context, defaultLookupKey types.NamespacedName, serviceInstance *v1.ServiceInstance, errMessage string) { Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - if err != nil { + if err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance); err != nil { return false } if len(serviceInstance.Status.Conditions) != 3 { From 9a93f49ddfce88351a78e2d1a87fda2a578a8152 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 17 Jul 2023 11:55:58 +0300 Subject: [PATCH 20/49] fix TESTS --- controllers/serviceinstance_controller_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index a61b0e07..db9f20f0 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -140,7 +140,6 @@ var _ = Describe("ServiceInstance controller", func() { isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { conditionReason := instance.Status.Conditions[0].Reason return strings.Contains(conditionReason, Updated) || strings.Contains(conditionReason, UpdateInProgress) || strings.Contains(conditionReason, UpdateFailed) - } _ = k8sClient.Update(ctx, serviceInstance) @@ -534,7 +533,7 @@ var _ = Describe("ServiceInstance controller", func() { }) }) - Context("When update call to SM fails", func() { + FContext("When update call to SM fails", func() { Context("Sync", func() { When("spec is changed", func() { BeforeEach(func() { @@ -548,17 +547,14 @@ var _ = Describe("ServiceInstance controller", func() { }) }) }) + Context("spec is changed, sm returns 502 and broker returns 429", func() { JustBeforeEach(func() { - fakeClient.ProvisionReturnsOnCall(0, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError()) fakeClient.UpdateInstanceReturns(nil, "", nil) }) It("recognize the error as transient and eventually succeed", func() { - serviceInstance = createInstance(ctx, instanceSpec, true) - Expect(len(serviceInstance.Status.Conditions)).To(Equal(2)) - Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) newSpec := updateSpec() serviceInstance.Spec = newSpec serviceInstance = updateInstance(ctx, serviceInstance) From 2ef13a35b293a97e73206451805d03e5c490e9ad Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 17 Jul 2023 12:06:21 +0300 Subject: [PATCH 21/49] fix TESTS --- controllers/serviceinstance_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index db9f20f0..2a734792 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -533,7 +533,7 @@ var _ = Describe("ServiceInstance controller", func() { }) }) - FContext("When update call to SM fails", func() { + Context("When update call to SM fails", func() { Context("Sync", func() { When("spec is changed", func() { BeforeEach(func() { From e0a34d8c6e38a4be1fa7dc68fc2cc445184e3f5b Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Tue, 18 Jul 2023 15:43:17 +0300 Subject: [PATCH 22/49] fix --- controllers/base_controller.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 5ebfbcbb..550192c4 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -303,24 +303,29 @@ func isTransientError(ctx context.Context, err error) bool { log := GetLogger(ctx) if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) - if isBrokerErrorExist(smError) { - log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) - if smError.StatusCode == http.StatusBadGateway && isTransientStatusCode(smError.BrokerError.StatusCode) { + if isSMTransientStatusCode(smError.StatusCode) { + return true + } + if smError.StatusCode == http.StatusBadGateway { + if !isBrokerErrorExist(smError) { return true } - } - if isTransientStatusCode(smError.StatusCode) { - return true + log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) + return isBrokerTransientStatusCode(smError.BrokerError.StatusCode) } } return false } -func isTransientStatusCode(StatusCode int) bool { +func isSMTransientStatusCode(StatusCode int) bool { return StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable || StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound } +func isBrokerTransientStatusCode(StatusCode int) bool { + return isSMTransientStatusCode(StatusCode) || StatusCode == http.StatusBadGateway +} + func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0 } From 25eb4533bb718c1bf7053b374ca1dea06bbf19f1 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Wed, 19 Jul 2023 10:03:29 +0300 Subject: [PATCH 23/49] fix --- controllers/serviceinstance_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 2a734792..8b80f4d8 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -560,7 +560,6 @@ var _ = Describe("ServiceInstance controller", func() { serviceInstance = updateInstance(ctx, serviceInstance) Expect(serviceInstance.Spec.ExternalName).To(Equal(newSpec.ExternalName)) Expect(serviceInstance.Status.Conditions[0].Reason).To(Equal(Updated)) - Expect(serviceInstance.Spec.UserInfo).NotTo(BeNil()) }) }) From 22982f51ed535d89a8bfea6a7dc799ca4952fa95 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Thu, 20 Jul 2023 11:12:08 +0300 Subject: [PATCH 24/49] fix Keren review --- controllers/base_controller.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 550192c4..c8f139f1 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -303,16 +303,13 @@ func isTransientError(ctx context.Context, err error) bool { log := GetLogger(ctx) if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) - if isSMTransientStatusCode(smError.StatusCode) { - return true - } - if smError.StatusCode == http.StatusBadGateway { - if !isBrokerErrorExist(smError) { - return true - } + if isBrokerErrorExist(smError) { log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) - return isBrokerTransientStatusCode(smError.BrokerError.StatusCode) + return isSMTransientStatusCode(smError.StatusCode) || isBrokerTransientStatusCode(smError.BrokerError.StatusCode) } + /* In case broker status does not exist we want to classify smError status code + as transient even if it's 502 */ + return isBrokerTransientStatusCode(smError.StatusCode) } return false } From 6c5a2b2ca3791acdaebe2e43d4579c0abcf544cc Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 09:50:27 +0300 Subject: [PATCH 25/49] fix Keren review --- api/common.go | 14 ++++++++++++++ client/sm/client.go | 4 ++-- controllers/serviceinstance_controller_test.go | 5 ++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/api/common.go b/api/common.go index 79bb2e9a..d3f87cfa 100644 --- a/api/common.go +++ b/api/common.go @@ -17,6 +17,20 @@ const ( ForceRotateAnnotation string = "services.cloud.sap.com/forceRotate" ) +type HTTPStatusCodeError struct { + // StatusCode is the HTTP status code returned by the broker. + StatusCode int + // ErrorMessage is a machine-readable error string that may be returned by + // the broker. + ErrorMessage *string + // Description is a human-readable description of the error that may be + // returned by the broker. + Description *string + // ResponseError is set to the error that occurred when unmarshalling a + // response body from the broker. + ResponseError error +} + const ( // ConditionSucceeded represents whether the last operation CREATE/UPDATE/DELETE was successful. ConditionSucceeded = "Succeeded" diff --git a/client/sm/client.go b/client/sm/client.go index d609ffeb..35414a30 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -21,6 +21,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/SAP/sap-btp-service-operator/api" "io" "net/http" "reflect" @@ -31,7 +32,6 @@ import ( "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/SAP/sap-btp-service-operator/internal/auth" "github.com/SAP/sap-btp-service-operator/internal/httputil" - v2 "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "golang.org/x/oauth2" "golang.org/x/oauth2/clientcredentials" ) @@ -70,7 +70,7 @@ type Client interface { type ServiceManagerError struct { Message string StatusCode int - BrokerError *v2.HTTPStatusCodeError `json:"broker_error,omitempty"` + BrokerError *api.HTTPStatusCodeError `json:"broker_error,omitempty"` } func (e *ServiceManagerError) Error() string { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 8b80f4d8..8f8ddfbc 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/SAP/sap-btp-service-operator/api" - "github.com/kubernetes-sigs/go-open-service-broker-client/v2" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/utils/pointer" @@ -1129,7 +1128,7 @@ func getNonTransientBrokerError(errMessage string) error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, Message: errMessage, - BrokerError: &v2.HTTPStatusCodeError{ + BrokerError: &api.HTTPStatusCodeError{ StatusCode: 400, }} } @@ -1138,7 +1137,7 @@ func getTransientBrokerError() error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, Message: "errMessage", - BrokerError: &v2.HTTPStatusCodeError{ + BrokerError: &api.HTTPStatusCodeError{ StatusCode: http.StatusTooManyRequests, }, } From a65d818f861a916f51123ca95b48544debab27d4 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 09:51:15 +0300 Subject: [PATCH 26/49] fix Keren review --- api/common.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/api/common.go b/api/common.go index d3f87cfa..1efa0ed3 100644 --- a/api/common.go +++ b/api/common.go @@ -20,14 +20,11 @@ const ( type HTTPStatusCodeError struct { // StatusCode is the HTTP status code returned by the broker. StatusCode int - // ErrorMessage is a machine-readable error string that may be returned by - // the broker. + // ErrorMessage is a machine-readable error string that may be returned by the broker. ErrorMessage *string - // Description is a human-readable description of the error that may be - // returned by the broker. + // Description is a human-readable description of the error that may be returned by the broker. Description *string - // ResponseError is set to the error that occurred when unmarshalling a - // response body from the broker. + // ResponseError is set to the error that occurred when unmarshalling a response body from the broker. ResponseError error } From 6e0a76c72712114a50e3e39489b6e82feb7174f8 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 09:58:46 +0300 Subject: [PATCH 27/49] fix Keren review --- client/sm/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/sm/client.go b/client/sm/client.go index 35414a30..9cc2c7a4 100644 --- a/client/sm/client.go +++ b/client/sm/client.go @@ -21,7 +21,6 @@ import ( "context" "encoding/json" "fmt" - "github.com/SAP/sap-btp-service-operator/api" "io" "net/http" "reflect" @@ -29,6 +28,7 @@ import ( "strconv" "strings" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/SAP/sap-btp-service-operator/internal/auth" "github.com/SAP/sap-btp-service-operator/internal/httputil" From 360a0095aa72156c00f1e795d4556073e27122e1 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 09:59:32 +0300 Subject: [PATCH 28/49] fix Keren review --- controllers/serviceinstance_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 8f8ddfbc..1ab978f3 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -6,10 +6,10 @@ import ( "net/http" "strings" - "github.com/SAP/sap-btp-service-operator/api" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/utils/pointer" + "github.com/SAP/sap-btp-service-operator/api" v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" "github.com/SAP/sap-btp-service-operator/client/sm/smfakes" From d396d4cb6715deaad23f626d7ce3b880aa120870 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 11:20:32 +0300 Subject: [PATCH 29/49] fix Keren review --- controllers/base_controller.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index c8f139f1..3f58f5ca 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -305,22 +305,16 @@ func isTransientError(ctx context.Context, err error) bool { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) if isBrokerErrorExist(smError) { log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) - return isSMTransientStatusCode(smError.StatusCode) || isBrokerTransientStatusCode(smError.BrokerError.StatusCode) + return isTransientStatusCode(smError.BrokerError.StatusCode) } - /* In case broker status does not exist we want to classify smError status code - as transient even if it's 502 */ - return isBrokerTransientStatusCode(smError.StatusCode) + return isTransientStatusCode(smError.StatusCode) } return false } -func isSMTransientStatusCode(StatusCode int) bool { +func isTransientStatusCode(StatusCode int) bool { return StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable || - StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound -} - -func isBrokerTransientStatusCode(StatusCode int) bool { - return isSMTransientStatusCode(StatusCode) || StatusCode == http.StatusBadGateway + StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound || StatusCode == http.StatusBadGateway } func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { From 193fbc91c889edbf86d4bc3f18b4fe8cee533cc6 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 11:20:51 +0300 Subject: [PATCH 30/49] fix Keren review --- controllers/servicebinding_controller_test.go | 50 ++++++++++++++- .../serviceinstance_controller_test.go | 63 ++++++++++++++++++- 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index fb736a67..14191ec0 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -81,6 +81,51 @@ var _ = Describe("ServiceBinding controller", func() { return createdBinding, nil } + createBindingExpectErrorAndEventuallySucceed := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, errMessage string) (*v1.ServiceBinding, error) { + binding := newBindingObject(name, namespace) + binding.Spec.ServiceInstanceName = instanceName + binding.Spec.ExternalName = externalName + binding.Spec.Parameters = &runtime.RawExtension{ + Raw: []byte(`{"key": "value"}`), + } + binding.Spec.ParametersFrom = []v1.ParametersFromSource{ + { + SecretKeyRef: &v1.SecretKeyReference{ + Name: "param-secret", + Key: "secret-parameter", + }, + }, + } + + if err := k8sClient.Create(ctx, binding); err != nil { + return nil, err + } + + bindingLookupKey := types.NamespacedName{Name: name, Namespace: namespace} + createdBinding = &v1.ServiceBinding{} + Eventually(func() bool { + err := k8sClient.Get(ctx, bindingLookupKey, createdBinding) + if err != nil { + return false + } + + return len(createdBinding.Status.Conditions) > 0 && strings.EqualFold(createdBinding.Status.Conditions[0].Message, errMessage) + + }, timeout, interval).Should(BeTrue()) + + Eventually(func() bool { + err := k8sClient.Get(ctx, bindingLookupKey, createdBinding) + if err != nil { + return false + } + + return isReady(createdBinding) || isFailed(createdBinding) + + }, timeout, interval).Should(BeTrue()) + + return createdBinding, nil + } + createBindingWithoutAssertions := func(ctx context.Context, name string, namespace string, instanceName string, externalName string) (*v1.ServiceBinding, error) { return createBindingWithoutAssertionsAndWait(ctx, name, namespace, instanceName, externalName, true) } @@ -542,11 +587,12 @@ var _ = Describe("ServiceBinding controller", func() { BeforeEach(func() { errorMessage = "too many requests" fakeClient.BindReturnsOnCall(0, nil, "", getTransientBrokerError()) - fakeClient.BindReturnsOnCall(1, &smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) + fakeClient.BindReturnsOnCall(1, nil, "", getTransientBrokerError()) + fakeClient.BindReturnsOnCall(2, &smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) }) It("should detect the error as transient and eventually succeed", func() { - b, err := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", true) + b, err := createBindingExpectErrorAndEventuallySucceed(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", "errMessage") Expect(err).ToNot(HaveOccurred()) Expect(isReady(b)).To(BeTrue()) }) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 1ab978f3..949cd3b1 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -117,6 +117,41 @@ var _ = Describe("ServiceInstance controller", func() { return createdInstance } + createInstanceWithErrorAndEvetuallySucceed := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, errMessage string) *v1.ServiceInstance { + instance := &v1.ServiceInstance{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "services.cloud.sap.com/v1", + Kind: "ServiceInstance", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fakeInstanceName, + Namespace: testNamespace, + }, + Spec: instanceSpec, + } + Expect(k8sClient.Create(ctx, instance)).Should(Succeed()) + + createdInstance := &v1.ServiceInstance{} + + Eventually(func() bool { + err := k8sClient.Get(ctx, defaultLookupKey, createdInstance) + if err != nil { + return false + } + return len(createdInstance.Status.Conditions) > 0 && strings.EqualFold(createdInstance.Status.Conditions[0].Message, errMessage) + }, timeout, interval).Should(BeTrue()) + + Eventually(func() bool { + err := k8sClient.Get(ctx, defaultLookupKey, createdInstance) + if err != nil { + return false + } + return isReady(createdInstance) + }, timeout, interval).Should(BeTrue()) + + return createdInstance + } + deleteInstance := func(ctx context.Context, instanceToDelete *v1.ServiceInstance, wait bool) { err := k8sClient.Get(ctx, types.NamespacedName{Name: instanceToDelete.Name, Namespace: instanceToDelete.Namespace}, &v1.ServiceInstance{}) if err != nil { @@ -155,6 +190,26 @@ var _ = Describe("ServiceInstance controller", func() { return updatedInstance } + //updateInstanceWaitForErrorAndEvetuallySucceed := func(ctx context.Context, serviceInstance *v1.ServiceInstance, errMessage string) *v1.ServiceInstance { + // isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { + // conditionReason := instance.Status.Conditions[0].Reason + // return strings.Contains(conditionReason, Updated) || strings.Contains(conditionReason, UpdateInProgress) || strings.Contains(conditionReason, UpdateFailed) + // } + // + // _ = k8sClient.Update(ctx, serviceInstance) + // updatedInstance := &v1.ServiceInstance{} + // + // Eventually(func() bool { + // err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance) + // if err != nil { + // return false + // } + // return len(updatedInstance.Status.Conditions) > 0 && isConditionRefersUpdateOp(updatedInstance) + // }, timeout, interval).Should(BeTrue()) + // + // return updatedInstance + //} + BeforeEach(func() { ctx = context.Background() fakeInstanceName = "ic-test-" + uuid.New().String() @@ -299,12 +354,14 @@ var _ = Describe("ServiceInstance controller", func() { Context("with sm status code 502 and broker status code 429", func() { JustBeforeEach(func() { - fakeClient.ProvisionReturns(nil, getTransientBrokerError()) - fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + for i := 0; i < 2; i++ { + fakeClient.ProvisionReturnsOnCall(i, nil, getTransientBrokerError()) + } + fakeClient.ProvisionReturnsOnCall(3, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) It("should be transient error and eventually succeed", func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstanceWithErrorAndEvetuallySucceed(ctx, instanceSpec, "errMessage") Expect(len(serviceInstance.Status.Conditions)).To(Equal(2)) Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) }) From 45a2d96b67d4912fc4e8b74f64402bd4eebaa24d Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 11:22:53 +0300 Subject: [PATCH 31/49] fix Keren review --- controllers/servicebinding_controller_test.go | 4 ++-- controllers/serviceinstance_controller_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 14191ec0..a4247cbb 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -81,7 +81,7 @@ var _ = Describe("ServiceBinding controller", func() { return createdBinding, nil } - createBindingExpectErrorAndEventuallySucceed := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, errMessage string) (*v1.ServiceBinding, error) { + createBindingWithErrorAndEventuallySucceed := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, errMessage string) (*v1.ServiceBinding, error) { binding := newBindingObject(name, namespace) binding.Spec.ServiceInstanceName = instanceName binding.Spec.ExternalName = externalName @@ -592,7 +592,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should detect the error as transient and eventually succeed", func() { - b, err := createBindingExpectErrorAndEventuallySucceed(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", "errMessage") + b, err := createBindingWithErrorAndEventuallySucceed(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", "errMessage") Expect(err).ToNot(HaveOccurred()) Expect(isReady(b)).To(BeTrue()) }) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 949cd3b1..16b5221f 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -117,7 +117,7 @@ var _ = Describe("ServiceInstance controller", func() { return createdInstance } - createInstanceWithErrorAndEvetuallySucceed := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, errMessage string) *v1.ServiceInstance { + createInstanceWithErrorAndEventuallySucceed := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, errMessage string) *v1.ServiceInstance { instance := &v1.ServiceInstance{ TypeMeta: metav1.TypeMeta{ APIVersion: "services.cloud.sap.com/v1", @@ -361,7 +361,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should be transient error and eventually succeed", func() { - serviceInstance = createInstanceWithErrorAndEvetuallySucceed(ctx, instanceSpec, "errMessage") + serviceInstance = createInstanceWithErrorAndEventuallySucceed(ctx, instanceSpec, "errMessage") Expect(len(serviceInstance.Status.Conditions)).To(Equal(2)) Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) }) From 16659700ca6a459fe8d19e7edf8f3471c9c61d71 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 11:29:01 +0300 Subject: [PATCH 32/49] fix Keren review --- controllers/servicebinding_controller_test.go | 48 ++++++++----------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index a4247cbb..bfe7b6e7 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -44,20 +44,7 @@ var _ = Describe("ServiceBinding controller", func() { var guid string createBindingWithoutAssertionsAndWait := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, wait bool) (*v1.ServiceBinding, error) { - binding := newBindingObject(name, namespace) - binding.Spec.ServiceInstanceName = instanceName - binding.Spec.ExternalName = externalName - binding.Spec.Parameters = &runtime.RawExtension{ - Raw: []byte(`{"key": "value"}`), - } - binding.Spec.ParametersFrom = []v1.ParametersFromSource{ - { - SecretKeyRef: &v1.SecretKeyReference{ - Name: "param-secret", - Key: "secret-parameter", - }, - }, - } + binding := generateBasicBindingTemplate(name, namespace, instanceName, externalName) if err := k8sClient.Create(ctx, binding); err != nil { return nil, err @@ -82,20 +69,7 @@ var _ = Describe("ServiceBinding controller", func() { } createBindingWithErrorAndEventuallySucceed := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, errMessage string) (*v1.ServiceBinding, error) { - binding := newBindingObject(name, namespace) - binding.Spec.ServiceInstanceName = instanceName - binding.Spec.ExternalName = externalName - binding.Spec.Parameters = &runtime.RawExtension{ - Raw: []byte(`{"key": "value"}`), - } - binding.Spec.ParametersFrom = []v1.ParametersFromSource{ - { - SecretKeyRef: &v1.SecretKeyReference{ - Name: "param-secret", - Key: "secret-parameter", - }, - }, - } + binding := generateBasicBindingTemplate(name, namespace, instanceName, externalName) if err := k8sClient.Create(ctx, binding); err != nil { return nil, err @@ -1238,6 +1212,24 @@ var _ = Describe("ServiceBinding controller", func() { }) }) +func generateBasicBindingTemplate(name, namespace, instanceName, externalName string) *v1.ServiceBinding { + binding := newBindingObject(name, namespace) + binding.Spec.ServiceInstanceName = instanceName + binding.Spec.ExternalName = externalName + binding.Spec.Parameters = &runtime.RawExtension{ + Raw: []byte(`{"key": "value"}`), + } + binding.Spec.ParametersFrom = []v1.ParametersFromSource{ + { + SecretKeyRef: &v1.SecretKeyReference{ + Name: "param-secret", + Key: "secret-parameter", + }, + }, + } + return binding +} + func validateSecretData(secret *corev1.Secret, expectedKey string, expectedValue string) { Expect(secret.Data).ToNot(BeNil()) Expect(secret.Data).To(HaveKey(expectedKey)) From e20735017a6d190dee65abb0834acd28dc366673 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 11:33:00 +0300 Subject: [PATCH 33/49] fix Keren review --- .../serviceinstance_controller_test.go | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 16b5221f..fab80946 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -190,26 +190,6 @@ var _ = Describe("ServiceInstance controller", func() { return updatedInstance } - //updateInstanceWaitForErrorAndEvetuallySucceed := func(ctx context.Context, serviceInstance *v1.ServiceInstance, errMessage string) *v1.ServiceInstance { - // isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { - // conditionReason := instance.Status.Conditions[0].Reason - // return strings.Contains(conditionReason, Updated) || strings.Contains(conditionReason, UpdateInProgress) || strings.Contains(conditionReason, UpdateFailed) - // } - // - // _ = k8sClient.Update(ctx, serviceInstance) - // updatedInstance := &v1.ServiceInstance{} - // - // Eventually(func() bool { - // err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance) - // if err != nil { - // return false - // } - // return len(updatedInstance.Status.Conditions) > 0 && isConditionRefersUpdateOp(updatedInstance) - // }, timeout, interval).Should(BeTrue()) - // - // return updatedInstance - //} - BeforeEach(func() { ctx = context.Background() fakeInstanceName = "ic-test-" + uuid.New().String() From b983bf367a42faa5ca85efe18c90cbfef055fac3 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 13:58:43 +0300 Subject: [PATCH 34/49] fix Keren review --- api/common.go | 14 +++ controllers/base_controller.go | 10 +- controllers/servicebinding_controller_test.go | 53 +++------- .../serviceinstance_controller_test.go | 100 +++++++++--------- 4 files changed, 88 insertions(+), 89 deletions(-) diff --git a/api/common.go b/api/common.go index 1efa0ed3..8f40645a 100644 --- a/api/common.go +++ b/api/common.go @@ -1,6 +1,7 @@ package api import ( + "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -28,6 +29,19 @@ type HTTPStatusCodeError struct { ResponseError error } +func (e HTTPStatusCodeError) Error() string { + errorMessage := "" + description := "" + + if e.ErrorMessage != nil { + errorMessage = *e.ErrorMessage + } + if e.Description != nil { + description = *e.Description + } + return fmt.Sprintf("Status: %v; ErrorMessage: %v; Description: %v; ResponseError: %v", e.StatusCode, errorMessage, description, e.ResponseError) +} + const ( // ConditionSucceeded represents whether the last operation CREATE/UPDATE/DELETE was successful. ConditionSucceeded = "Succeeded" diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 3f58f5ca..7e1c248c 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -324,6 +324,11 @@ func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, nonTransientErr error, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) setFailureConditions(operationType, nonTransientErr.Error(), object) + if smError, ok := nonTransientErr.(*sm.ServiceManagerError); ok { + if isBrokerErrorExist(smError) { + setFailureConditions(operationType, smError.BrokerError.Error(), object) + } + } if operationType != smClientTypes.DELETE { log.Info(fmt.Sprintf("operation %s of %s encountered a non transient error %s, giving up operation :(", operationType, object.GetControllerName(), nonTransientErr.Error())) } @@ -340,8 +345,11 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, transientErr error, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) + setInProgressConditions(operationType, transientErr.Error(), object) if smError, ok := transientErr.(*sm.ServiceManagerError); ok && smError.StatusCode != http.StatusTooManyRequests { - setInProgressConditions(operationType, transientErr.Error(), object) + if isBrokerErrorExist(smError) { + setInProgressConditions(operationType, smError.BrokerError.Error(), object) + } log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), transientErr.Error())) if err := r.updateStatus(ctx, object); err != nil { return ctrl.Result{}, err diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index bfe7b6e7..a02cf53d 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -68,38 +68,6 @@ var _ = Describe("ServiceBinding controller", func() { return createdBinding, nil } - createBindingWithErrorAndEventuallySucceed := func(ctx context.Context, name string, namespace string, instanceName string, externalName string, errMessage string) (*v1.ServiceBinding, error) { - binding := generateBasicBindingTemplate(name, namespace, instanceName, externalName) - - if err := k8sClient.Create(ctx, binding); err != nil { - return nil, err - } - - bindingLookupKey := types.NamespacedName{Name: name, Namespace: namespace} - createdBinding = &v1.ServiceBinding{} - Eventually(func() bool { - err := k8sClient.Get(ctx, bindingLookupKey, createdBinding) - if err != nil { - return false - } - - return len(createdBinding.Status.Conditions) > 0 && strings.EqualFold(createdBinding.Status.Conditions[0].Message, errMessage) - - }, timeout, interval).Should(BeTrue()) - - Eventually(func() bool { - err := k8sClient.Get(ctx, bindingLookupKey, createdBinding) - if err != nil { - return false - } - - return isReady(createdBinding) || isFailed(createdBinding) - - }, timeout, interval).Should(BeTrue()) - - return createdBinding, nil - } - createBindingWithoutAssertions := func(ctx context.Context, name string, namespace string, instanceName string, externalName string) (*v1.ServiceBinding, error) { return createBindingWithoutAssertionsAndWait(ctx, name, namespace, instanceName, externalName, true) } @@ -559,16 +527,23 @@ var _ = Describe("ServiceBinding controller", func() { When("SM returned error 502 and broker returned 429", func() { BeforeEach(func() { - errorMessage = "too many requests" - fakeClient.BindReturnsOnCall(0, nil, "", getTransientBrokerError()) - fakeClient.BindReturnsOnCall(1, nil, "", getTransientBrokerError()) - fakeClient.BindReturnsOnCall(2, &smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) + errorMessage = "too many requests from broker" + fakeClient.BindReturns(nil, "", getTransientBrokerError(errorMessage)) }) It("should detect the error as transient and eventually succeed", func() { - b, err := createBindingWithErrorAndEventuallySucceed(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", "errMessage") - Expect(err).ToNot(HaveOccurred()) - Expect(isReady(b)).To(BeTrue()) + createdBinding, _ := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", false) + cond := meta.FindStatusCondition(createdBinding.GetConditions(), api.ConditionSucceeded) + Expect(cond.Message).To(ContainSubstring(errorMessage)) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) + Eventually(func() bool { + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace}, createdBinding) + if err != nil { + return false + } + return isReady(createdBinding) + }, timeout, interval).Should(BeTrue()) }) }) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index fab80946..ced538fd 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -117,41 +117,6 @@ var _ = Describe("ServiceInstance controller", func() { return createdInstance } - createInstanceWithErrorAndEventuallySucceed := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, errMessage string) *v1.ServiceInstance { - instance := &v1.ServiceInstance{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "services.cloud.sap.com/v1", - Kind: "ServiceInstance", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: fakeInstanceName, - Namespace: testNamespace, - }, - Spec: instanceSpec, - } - Expect(k8sClient.Create(ctx, instance)).Should(Succeed()) - - createdInstance := &v1.ServiceInstance{} - - Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, createdInstance) - if err != nil { - return false - } - return len(createdInstance.Status.Conditions) > 0 && strings.EqualFold(createdInstance.Status.Conditions[0].Message, errMessage) - }, timeout, interval).Should(BeTrue()) - - Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, createdInstance) - if err != nil { - return false - } - return isReady(createdInstance) - }, timeout, interval).Should(BeTrue()) - - return createdInstance - } - deleteInstance := func(ctx context.Context, instanceToDelete *v1.ServiceInstance, wait bool) { err := k8sClient.Get(ctx, types.NamespacedName{Name: instanceToDelete.Name, Namespace: instanceToDelete.Namespace}, &v1.ServiceInstance{}) if err != nil { @@ -170,6 +135,34 @@ var _ = Describe("ServiceInstance controller", func() { } } + updateInstanceWithErrorAndEventuallySucceed := func(ctx context.Context, serviceInstance *v1.ServiceInstance, errMessage string) *v1.ServiceInstance { + isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { + conditionReason := instance.Status.Conditions[0].Reason + return strings.Contains(conditionReason, Updated) || strings.Contains(conditionReason, UpdateInProgress) || strings.Contains(conditionReason, UpdateFailed) + } + + _ = k8sClient.Update(ctx, serviceInstance) + updatedInstance := &v1.ServiceInstance{} + + Eventually(func() bool { + err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance) + if err != nil { + return false + } + return len(updatedInstance.Status.Conditions) > 0 && strings.ContainsAny(updatedInstance.Status.Conditions[0].Message, errMessage) + }, timeout, interval).Should(BeTrue()) + + Eventually(func() bool { + err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance) + if err != nil { + return false + } + return len(updatedInstance.Status.Conditions) > 0 && isConditionRefersUpdateOp(updatedInstance) + }, timeout, interval).Should(BeTrue()) + + return updatedInstance + } + updateInstance := func(ctx context.Context, serviceInstance *v1.ServiceInstance) *v1.ServiceInstance { isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { conditionReason := instance.Status.Conditions[0].Reason @@ -333,17 +326,23 @@ var _ = Describe("ServiceInstance controller", func() { }) Context("with sm status code 502 and broker status code 429", func() { + errMessage := "broker too many requests" JustBeforeEach(func() { - for i := 0; i < 2; i++ { - fakeClient.ProvisionReturnsOnCall(i, nil, getTransientBrokerError()) - } - fakeClient.ProvisionReturnsOnCall(3, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + fakeClient.ProvisionReturns(nil, getTransientBrokerError(errMessage)) }) It("should be transient error and eventually succeed", func() { - serviceInstance = createInstanceWithErrorAndEventuallySucceed(ctx, instanceSpec, "errMessage") - Expect(len(serviceInstance.Status.Conditions)).To(Equal(2)) - Expect(meta.IsStatusConditionPresentAndEqual(serviceInstance.Status.Conditions, api.ConditionSucceeded, metav1.ConditionTrue)).To(Equal(true)) + serviceInstance = createInstance(ctx, instanceSpec, false) + Eventually(func() bool { + _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) + cond := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded) + return strings.Contains(cond.Message, errMessage) && cond.Status == metav1.ConditionFalse + }, timeout, interval).Should(BeTrue()) + fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + Eventually(func() bool { + _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) + return isReady(serviceInstance) + }, timeout, interval).Should(BeTrue()) }) }) @@ -585,15 +584,16 @@ var _ = Describe("ServiceInstance controller", func() { }) Context("spec is changed, sm returns 502 and broker returns 429", func() { + errMessage := "broker too many requests" JustBeforeEach(func() { - fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError()) + fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError(errMessage)) fakeClient.UpdateInstanceReturns(nil, "", nil) }) It("recognize the error as transient and eventually succeed", func() { newSpec := updateSpec() serviceInstance.Spec = newSpec - serviceInstance = updateInstance(ctx, serviceInstance) + serviceInstance = updateInstanceWithErrorAndEventuallySucceed(ctx, serviceInstance, errMessage) Expect(serviceInstance.Spec.ExternalName).To(Equal(newSpec.ExternalName)) Expect(serviceInstance.Status.Conditions[0].Reason).To(Equal(Updated)) }) @@ -1164,18 +1164,20 @@ var _ = Describe("ServiceInstance controller", func() { func getNonTransientBrokerError(errMessage string) error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, - Message: errMessage, + Message: "smErrMessage", BrokerError: &api.HTTPStatusCodeError{ - StatusCode: 400, + StatusCode: 400, + ErrorMessage: &errMessage, }} } -func getTransientBrokerError() error { +func getTransientBrokerError(errorMessage string) error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadGateway, - Message: "errMessage", + Message: "smErrMessage", BrokerError: &api.HTTPStatusCodeError{ - StatusCode: http.StatusTooManyRequests, + StatusCode: http.StatusTooManyRequests, + ErrorMessage: &errorMessage, }, } } From f150f3899a2630c3636abcca049aa82195e4b478 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 15:52:38 +0300 Subject: [PATCH 35/49] fix Keren review --- controllers/servicebinding_controller_test.go | 20 ++++--- controllers/serviceinstance_controller.go | 5 ++ .../serviceinstance_controller_test.go | 58 ++++++------------- 3 files changed, 35 insertions(+), 48 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index a02cf53d..3163fddf 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -533,17 +533,13 @@ var _ = Describe("ServiceBinding controller", func() { It("should detect the error as transient and eventually succeed", func() { createdBinding, _ := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", false) + cond := meta.FindStatusCondition(createdBinding.GetConditions(), api.ConditionSucceeded) Expect(cond.Message).To(ContainSubstring(errorMessage)) Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) - Eventually(func() bool { - err := k8sClient.Get(context.Background(), types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace}, createdBinding) - if err != nil { - return false - } - return isReady(createdBinding) - }, timeout, interval).Should(BeTrue()) + validateBindingIsReady(createdBinding, bindingName) }) }) @@ -1187,6 +1183,16 @@ var _ = Describe("ServiceBinding controller", func() { }) }) +func validateBindingIsReady(createdBinding *v1.ServiceBinding, bindingName string) { + Eventually(func() bool { + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace}, createdBinding) + if err != nil { + return false + } + return isReady(createdBinding) + }, timeout, interval).Should(BeTrue()) +} + func generateBasicBindingTemplate(name, namespace, instanceName, externalName string) *v1.ServiceBinding { binding := newBindingObject(name, namespace) binding.Spec.ServiceInstanceName = instanceName diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 45797c3d..5031d41d 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -554,6 +554,11 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { return false } + cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) + if cond != nil && cond.Reason == UpdateInProgress { + return true + } + if getSpecHash(serviceInstance) == serviceInstance.Status.HashedSpec { return false } diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index ced538fd..35b4b0c8 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -135,34 +135,6 @@ var _ = Describe("ServiceInstance controller", func() { } } - updateInstanceWithErrorAndEventuallySucceed := func(ctx context.Context, serviceInstance *v1.ServiceInstance, errMessage string) *v1.ServiceInstance { - isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { - conditionReason := instance.Status.Conditions[0].Reason - return strings.Contains(conditionReason, Updated) || strings.Contains(conditionReason, UpdateInProgress) || strings.Contains(conditionReason, UpdateFailed) - } - - _ = k8sClient.Update(ctx, serviceInstance) - updatedInstance := &v1.ServiceInstance{} - - Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance) - if err != nil { - return false - } - return len(updatedInstance.Status.Conditions) > 0 && strings.ContainsAny(updatedInstance.Status.Conditions[0].Message, errMessage) - }, timeout, interval).Should(BeTrue()) - - Eventually(func() bool { - err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance) - if err != nil { - return false - } - return len(updatedInstance.Status.Conditions) > 0 && isConditionRefersUpdateOp(updatedInstance) - }, timeout, interval).Should(BeTrue()) - - return updatedInstance - } - updateInstance := func(ctx context.Context, serviceInstance *v1.ServiceInstance) *v1.ServiceInstance { isConditionRefersUpdateOp := func(instance *v1.ServiceInstance) bool { conditionReason := instance.Status.Conditions[0].Reason @@ -291,10 +263,9 @@ var _ = Describe("ServiceInstance controller", func() { }) When("provision request to SM fails", func() { - var errMessage string Context("with 400 status", func() { + errMessage := "failed to provision instance" JustBeforeEach(func() { - errMessage = "failed to provision instance" fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, Message: errMessage, @@ -310,7 +281,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("with 429 status eventually succeeds", func() { JustBeforeEach(func() { - errMessage = "failed to provision instance" + errMessage := "failed to provision instance" fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ StatusCode: http.StatusTooManyRequests, Message: errMessage, @@ -326,9 +297,9 @@ var _ = Describe("ServiceInstance controller", func() { }) Context("with sm status code 502 and broker status code 429", func() { - errMessage := "broker too many requests" + errorMessage := "broker too many requests" JustBeforeEach(func() { - fakeClient.ProvisionReturns(nil, getTransientBrokerError(errMessage)) + fakeClient.ProvisionReturns(nil, getTransientBrokerError(errorMessage)) }) It("should be transient error and eventually succeed", func() { @@ -336,8 +307,9 @@ var _ = Describe("ServiceInstance controller", func() { Eventually(func() bool { _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) cond := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded) - return strings.Contains(cond.Message, errMessage) && cond.Status == metav1.ConditionFalse + return cond != nil && strings.Contains(cond.Message, errorMessage) && cond.Status == metav1.ConditionFalse }, timeout, interval).Should(BeTrue()) + fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) Eventually(func() bool { _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) @@ -347,8 +319,8 @@ var _ = Describe("ServiceInstance controller", func() { }) Context("with sm status code 502 and broker status code 400", func() { + errMessage := "failed to provision instance" JustBeforeEach(func() { - errMessage = "failed to provision instance" fakeClient.ProvisionReturns(nil, getNonTransientBrokerError(errMessage)) fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) @@ -587,15 +559,19 @@ var _ = Describe("ServiceInstance controller", func() { errMessage := "broker too many requests" JustBeforeEach(func() { fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError(errMessage)) - fakeClient.UpdateInstanceReturns(nil, "", nil) }) It("recognize the error as transient and eventually succeed", func() { - newSpec := updateSpec() - serviceInstance.Spec = newSpec - serviceInstance = updateInstanceWithErrorAndEventuallySucceed(ctx, serviceInstance, errMessage) - Expect(serviceInstance.Spec.ExternalName).To(Equal(newSpec.ExternalName)) - Expect(serviceInstance.Status.Conditions[0].Reason).To(Equal(Updated)) + serviceInstance.Spec = updateSpec() + updatedInstance := updateInstance(ctx, serviceInstance) + Expect(updatedInstance.Status.Conditions[0].Reason).To(Equal(UpdateInProgress)) + Expect(updatedInstance.Status.Conditions[0].Message).To(ContainSubstring(errMessage)) + fakeClient.UpdateInstanceReturns(nil, "", nil) + updatedInstance = updateInstance(ctx, serviceInstance) + Eventually(func() bool { + _ = k8sClient.Get(ctx, defaultLookupKey, updatedInstance) + return isReady(updatedInstance) + }, timeout, interval).Should(BeTrue()) }) }) From 7f105714e42136e43135b76cb33be526540fac50 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 15:55:59 +0300 Subject: [PATCH 36/49] fix Keren review --- controllers/base_controller.go | 14 +++++++++----- controllers/serviceinstance_controller.go | 6 +++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 7e1c248c..2ab1de89 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -345,16 +345,20 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, transientErr error, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) - setInProgressConditions(operationType, transientErr.Error(), object) + errMsg := transientErr.Error() if smError, ok := transientErr.(*sm.ServiceManagerError); ok && smError.StatusCode != http.StatusTooManyRequests { if isBrokerErrorExist(smError) { setInProgressConditions(operationType, smError.BrokerError.Error(), object) + errMsg = smError.BrokerError.Error() } - log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), transientErr.Error())) - if err := r.updateStatus(ctx, object); err != nil { - return ctrl.Result{}, err - } + } else { + setInProgressConditions(operationType, transientErr.Error(), object) } + log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), errMsg)) + if err := r.updateStatus(ctx, object); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, transientErr } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 5031d41d..b220af4d 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -518,9 +518,9 @@ func (r *ServiceInstanceReconciler) HandleInstanceSharingError(ctx context.Conte reason = InProgress } else if reason == ShareFailed && (smError.StatusCode == http.StatusBadRequest || smError.StatusCode == http.StatusInternalServerError) { - // non-transient error may occur only when sharing - // SM return 400 when plan is not sharable - // SM returns 500 when TOGGLES_ENABLE_INSTANCE_SHARE_FROM_OPERATOR feature toggle is off + /* non-transient error may occur only when sharing + SM return 400 when plan is not sharable + SM returns 500 when TOGGLES_ENABLE_INSTANCE_SHARE_FROM_OPERATOR feature toggle is off */ setSharedCondition(object, status, ShareNotSupported, err.Error()) return r.updateStatus(ctx, object) } From 48e9c05f0e2af6b634e470b1b10f05c8ba4acf44 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 16:01:50 +0300 Subject: [PATCH 37/49] fix Keren review --- api/common.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/common.go b/api/common.go index 8f40645a..ef48488e 100644 --- a/api/common.go +++ b/api/common.go @@ -2,6 +2,7 @@ package api import ( "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" From 49ee9b40e0346c7bfcb9346f5bc8c93d5006168a Mon Sep 17 00:00:00 2001 From: I501080 Date: Sun, 23 Jul 2023 16:20:58 +0300 Subject: [PATCH 38/49] fix isTransient --- controllers/base_controller.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 2ab1de89..f77aace4 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -301,15 +301,17 @@ func isDelete(object metav1.ObjectMeta) bool { func isTransientError(ctx context.Context, err error) bool { log := GetLogger(ctx) - if smError, ok := err.(*sm.ServiceManagerError); ok { - log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) - if isBrokerErrorExist(smError) { - log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) - return isTransientStatusCode(smError.BrokerError.StatusCode) - } - return isTransientStatusCode(smError.StatusCode) + smError, ok := err.(*sm.ServiceManagerError) + if !ok { + return false + } + + statusCode := smError.StatusCode + if isBrokerErrorExist(smError) { + log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) + statusCode = smError.BrokerError.StatusCode } - return false + return isTransientStatusCode(statusCode) } func isTransientStatusCode(StatusCode int) bool { From 6ac0055bc7db6a2b4f1796d6e3da7cf597e38e45 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 17:13:35 +0300 Subject: [PATCH 39/49] fix Keren review --- controllers/base_controller.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index f77aace4..e43d0a2c 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -310,6 +310,8 @@ func isTransientError(ctx context.Context, err error) bool { if isBrokerErrorExist(smError) { log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) statusCode = smError.BrokerError.StatusCode + } else { + log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) } return isTransientStatusCode(statusCode) } From 29f52fe426048f8b401f4ab3a54a07f975c9e99a Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 17:17:46 +0300 Subject: [PATCH 40/49] fix Keren review --- controllers/base_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index e43d0a2c..a92e0c06 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -352,8 +352,8 @@ func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType errMsg := transientErr.Error() if smError, ok := transientErr.(*sm.ServiceManagerError); ok && smError.StatusCode != http.StatusTooManyRequests { if isBrokerErrorExist(smError) { - setInProgressConditions(operationType, smError.BrokerError.Error(), object) errMsg = smError.BrokerError.Error() + setInProgressConditions(operationType, errMsg, object) } } else { setInProgressConditions(operationType, transientErr.Error(), object) From 5affe17fef4b90d1f10f6f53343ceafa705d995f Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Sun, 23 Jul 2023 17:22:33 +0300 Subject: [PATCH 41/49] clean code --- controllers/servicebinding_controller_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 3163fddf..77115236 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -532,13 +532,15 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should detect the error as transient and eventually succeed", func() { - createdBinding, _ := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", false) + createdBinding, _ := createBindingWithoutAssertionsAndWait(context.Background(), + bindingName, bindingTestNamespace, instanceName, "binding-external-name", false) cond := meta.FindStatusCondition(createdBinding.GetConditions(), api.ConditionSucceeded) Expect(cond.Message).To(ContainSubstring(errorMessage)) Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) + fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, + Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) validateBindingIsReady(createdBinding, bindingName) }) }) From 2e4a5e53dda8ab5cce26e9d2a0a5449780242e4f Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 24 Jul 2023 08:16:44 +0300 Subject: [PATCH 42/49] clean code --- controllers/servicebinding_controller_test.go | 12 ++++--- .../serviceinstance_controller_test.go | 32 +++++++------------ 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 77115236..57460a7a 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -534,10 +534,7 @@ var _ = Describe("ServiceBinding controller", func() { It("should detect the error as transient and eventually succeed", func() { createdBinding, _ := createBindingWithoutAssertionsAndWait(context.Background(), bindingName, bindingTestNamespace, instanceName, "binding-external-name", false) - - cond := meta.FindStatusCondition(createdBinding.GetConditions(), api.ConditionSucceeded) - Expect(cond.Message).To(ContainSubstring(errorMessage)) - Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + expectBindingToBeInFailedStateWithMsg(createdBinding, errorMessage) fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage("{\"secret_key\": \"secret_value\"}")}, "", nil) @@ -1185,6 +1182,13 @@ var _ = Describe("ServiceBinding controller", func() { }) }) +func expectBindingToBeInFailedStateWithMsg(binding *v1.ServiceBinding, message string) { + cond := meta.FindStatusCondition(binding.GetConditions(), api.ConditionSucceeded) + Expect(cond).To(Not(BeNil())) + Expect(cond.Message).To(ContainSubstring(message)) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) +} + func validateBindingIsReady(createdBinding *v1.ServiceBinding, bindingName string) { Eventually(func() bool { err := k8sClient.Get(context.Background(), types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace}, createdBinding) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 35b4b0c8..e4210d39 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -311,10 +311,7 @@ var _ = Describe("ServiceInstance controller", func() { }, timeout, interval).Should(BeTrue()) fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - return isReady(serviceInstance) - }, timeout, interval).Should(BeTrue()) + waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey) }) }) @@ -351,10 +348,7 @@ var _ = Describe("ServiceInstance controller", func() { Type: smClientTypes.CREATE, State: smClientTypes.SUCCEEDED, }, nil) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - return isReady(serviceInstance) - }, timeout, interval).Should(BeTrue()) + waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey) }) }) @@ -485,10 +479,7 @@ var _ = Describe("ServiceInstance controller", func() { Type: smClientTypes.UPDATE, State: smClientTypes.SUCCEEDED, }, nil) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, updatedInstance) - return isReady(serviceInstance) - }, timeout, interval).Should(BeTrue()) + waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey) Expect(updatedInstance.Spec.ExternalName).To(Equal(newSpec.ExternalName)) }) @@ -568,10 +559,7 @@ var _ = Describe("ServiceInstance controller", func() { Expect(updatedInstance.Status.Conditions[0].Message).To(ContainSubstring(errMessage)) fakeClient.UpdateInstanceReturns(nil, "", nil) updatedInstance = updateInstance(ctx, serviceInstance) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, updatedInstance) - return isReady(updatedInstance) - }, timeout, interval).Should(BeTrue()) + waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey) }) }) @@ -596,10 +584,7 @@ var _ = Describe("ServiceInstance controller", func() { Type: smClientTypes.UPDATE, State: smClientTypes.FAILED, }, nil) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, updatedInstance) - return isReady(serviceInstance) - }, timeout, interval).Should(BeTrue()) + waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey) }) }) @@ -1137,6 +1122,13 @@ var _ = Describe("ServiceInstance controller", func() { }) }) +func waitForInstanceToBeReady(instance *v1.ServiceInstance, ctx context.Context, key types.NamespacedName) { + Eventually(func() bool { + _ = k8sClient.Get(ctx, key, instance) + return isReady(instance) + }, timeout, interval).Should(BeTrue()) +} + func getNonTransientBrokerError(errMessage string) error { return &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, From 936180ea2d643b4ae883c5db62dbe833d4b4c940 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 24 Jul 2023 08:21:14 +0300 Subject: [PATCH 43/49] clean code --- controllers/serviceinstance_controller_test.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index e4210d39..67561dae 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -302,14 +302,9 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.ProvisionReturns(nil, getTransientBrokerError(errorMessage)) }) - It("should be transient error and eventually succeed", func() { + FIt("should be transient error and eventually succeed", func() { serviceInstance = createInstance(ctx, instanceSpec, false) - Eventually(func() bool { - _ = k8sClient.Get(ctx, defaultLookupKey, serviceInstance) - cond := meta.FindStatusCondition(serviceInstance.GetConditions(), api.ConditionSucceeded) - return cond != nil && strings.Contains(cond.Message, errorMessage) && cond.Status == metav1.ConditionFalse - }, timeout, interval).Should(BeTrue()) - + expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errorMessage) fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) waitForInstanceToBeReady(serviceInstance, ctx, defaultLookupKey) }) @@ -1155,9 +1150,6 @@ func expectForInstanceCreationFailure(ctx context.Context, defaultLookupKey type if err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance); err != nil { return false } - if len(serviceInstance.Status.Conditions) != 3 { - return false - } cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded) return cond != nil && cond.Status == metav1.ConditionFalse && strings.Contains(cond.Message, errMessage) }, timeout, interval).Should(BeTrue()) From cbded30f85fd806c136447571b7bfe98d110bb48 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 24 Jul 2023 08:23:38 +0300 Subject: [PATCH 44/49] go mod vendor && tidy --- go.mod | 1 - go.sum | 4 ---- 2 files changed, 5 deletions(-) diff --git a/go.mod b/go.mod index 140dd1f7..527ace73 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,6 @@ require ( github.com/go-logr/logr v1.2.4 github.com/google/uuid v1.3.0 github.com/kelseyhightower/envconfig v1.4.0 - github.com/kubernetes-sigs/go-open-service-broker-client v0.0.0-20200527163240-4406bd2cb6b8 github.com/onsi/ginkgo v1.16.5 github.com/onsi/gomega v1.27.7 golang.org/x/oauth2 v0.5.0 diff --git a/go.sum b/go.sum index 37e7956f..74895516 100644 --- a/go.sum +++ b/go.sum @@ -24,7 +24,6 @@ github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMo github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ= github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY= github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw= -github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.2.4 h1:g01GSCwiDw2xSZfjJ2/T9M+S6pFdcNtFYsp+Y43HYDQ= github.com/go-logr/logr v1.2.4/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= @@ -93,8 +92,6 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/kubernetes-sigs/go-open-service-broker-client v0.0.0-20200527163240-4406bd2cb6b8 h1:37JsHMAiuoyIFPyYUJ6jOTXznsmcDZec6tkPod+Tqto= -github.com/kubernetes-sigs/go-open-service-broker-client v0.0.0-20200527163240-4406bd2cb6b8/go.mod h1:5VFrdwwxqkzCF3pL7MY5Om1btQ6UsxO87DyjZFO0s5M= github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0= github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zkfA9PSy5pEvNWRP0ET0TIVo= @@ -292,7 +289,6 @@ k8s.io/client-go v0.27.3 h1:7dnEGHZEJld3lYwxvLl7WoehK6lAq7GvgjxpA3nv1E8= k8s.io/client-go v0.27.3/go.mod h1:2MBEKuTo6V1lbKy3z1euEGnhPfGZLKTS9tiJ2xodM48= k8s.io/component-base v0.27.2 h1:neju+7s/r5O4x4/txeUONNTS9r1HsPbyoPBAtHsDCpo= k8s.io/component-base v0.27.2/go.mod h1:5UPk7EjfgrfgRIuDBFtsEFAe4DAvP3U+M8RTzoSJkpo= -k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw= k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f h1:2kWPakN3i/k81b0gvD5C5FJ2kxm1WrQFanWchyKuqGg= From 1e86338542df982fb3417017a01848a23490d808 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 24 Jul 2023 09:13:37 +0300 Subject: [PATCH 45/49] go mod vendor && tidy --- controllers/serviceinstance_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 67561dae..229b26c3 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -302,7 +302,7 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.ProvisionReturns(nil, getTransientBrokerError(errorMessage)) }) - FIt("should be transient error and eventually succeed", func() { + It("should be transient error and eventually succeed", func() { serviceInstance = createInstance(ctx, instanceSpec, false) expectForInstanceCreationFailure(ctx, defaultLookupKey, serviceInstance, errorMessage) fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) From e39c93325fde4dfa00248ecd7ed1c6cd4f49ab62 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 24 Jul 2023 09:24:13 +0300 Subject: [PATCH 46/49] go mod vendor && tidy --- controllers/serviceinstance_controller_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 229b26c3..9c4dffe2 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -474,7 +474,10 @@ var _ = Describe("ServiceInstance controller", func() { Type: smClientTypes.UPDATE, State: smClientTypes.SUCCEEDED, }, nil) - waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey) + Eventually(func() bool { + _ = k8sClient.Get(ctx, defaultLookupKey, updatedInstance) + return isReady(serviceInstance) + }, timeout, interval).Should(BeTrue()) Expect(updatedInstance.Spec.ExternalName).To(Equal(newSpec.ExternalName)) }) @@ -579,7 +582,10 @@ var _ = Describe("ServiceInstance controller", func() { Type: smClientTypes.UPDATE, State: smClientTypes.FAILED, }, nil) - waitForInstanceToBeReady(updatedInstance, ctx, defaultLookupKey) + Eventually(func() bool { + _ = k8sClient.Get(ctx, defaultLookupKey, updatedInstance) + return isReady(serviceInstance) + }, timeout, interval).Should(BeTrue()) }) }) From 1534dea9c5f69cb6e1eeba98f129ca2afc945add Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 24 Jul 2023 11:40:13 +0300 Subject: [PATCH 47/49] cosmetics --- controllers/base_controller.go | 35 ++++++++--------------- controllers/servicebinding_controller.go | 24 ++++++++-------- controllers/serviceinstance_controller.go | 24 +++++++++------- 3 files changed, 37 insertions(+), 46 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index a92e0c06..5ee4e5dd 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -299,21 +299,23 @@ func isDelete(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func isTransientError(ctx context.Context, err error) bool { +func isTransientError(ctx context.Context, err error) (bool, string) { log := GetLogger(ctx) smError, ok := err.(*sm.ServiceManagerError) if !ok { - return false + return false, err.Error() } statusCode := smError.StatusCode + errMsg := smError.Error() if isBrokerErrorExist(smError) { log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode)) statusCode = smError.BrokerError.StatusCode + errMsg = smError.BrokerError.Error() } else { log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode)) } - return isTransientStatusCode(statusCode) + return isTransientStatusCode(statusCode), errMsg } func isTransientStatusCode(StatusCode int) bool { @@ -325,16 +327,11 @@ func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0 } -func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, nonTransientErr error, object api.SAPBTPResource) (ctrl.Result, error) { +func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) - setFailureConditions(operationType, nonTransientErr.Error(), object) - if smError, ok := nonTransientErr.(*sm.ServiceManagerError); ok { - if isBrokerErrorExist(smError) { - setFailureConditions(operationType, smError.BrokerError.Error(), object) - } - } + setFailureConditions(operationType, errMsg, object) if operationType != smClientTypes.DELETE { - log.Info(fmt.Sprintf("operation %s of %s encountered a non transient error %s, giving up operation :(", operationType, object.GetControllerName(), nonTransientErr.Error())) + log.Info(fmt.Sprintf("operation %s of %s encountered a non transient error %s, giving up operation :(", operationType, object.GetControllerName(), errMsg)) } object.SetObservedGeneration(object.GetGeneration()) err := r.updateStatus(ctx, object) @@ -342,28 +339,20 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT return ctrl.Result{}, err } if operationType == smClientTypes.DELETE { - return ctrl.Result{}, nonTransientErr + return ctrl.Result{}, fmt.Errorf(errMsg) } return ctrl.Result{}, nil } -func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, transientErr error, object api.SAPBTPResource) (ctrl.Result, error) { +func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) - errMsg := transientErr.Error() - if smError, ok := transientErr.(*sm.ServiceManagerError); ok && smError.StatusCode != http.StatusTooManyRequests { - if isBrokerErrorExist(smError) { - errMsg = smError.BrokerError.Error() - setInProgressConditions(operationType, errMsg, object) - } - } else { - setInProgressConditions(operationType, transientErr.Error(), object) - } + setInProgressConditions(operationType, errMsg, object) log.Info(fmt.Sprintf("operation %s of %s encountered a transient error %s, retrying operation :)", operationType, object.GetControllerName(), errMsg)) if err := r.updateStatus(ctx, object); err != nil { return ctrl.Result{}, err } - return ctrl.Result{}, transientErr + return ctrl.Result{}, fmt.Errorf(errMsg) } func isInProgress(object api.SAPBTPResource) bool { diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 3d40126d..b787986f 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -85,7 +85,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque smClient, err := r.getSMClient(ctx, serviceBinding) if err != nil { - return r.markAsTransientError(ctx, Unknown, err, serviceBinding) + return r.markAsTransientError(ctx, Unknown, err.Error(), serviceBinding) } if len(serviceBinding.Status.OperationURL) > 0 { @@ -173,7 +173,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque binding, err := r.getBindingForRecovery(ctx, smClient, serviceBinding) if err != nil { log.Error(err, "failed to check binding recovery") - return r.markAsTransientError(ctx, smClientTypes.CREATE, err, serviceBinding) + return r.markAsTransientError(ctx, smClientTypes.CREATE, err.Error(), serviceBinding) } if binding != nil { // Recovery - restore binding from SM @@ -209,7 +209,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s _, bindingParameters, err := buildParameters(r.Client, serviceBinding.Namespace, serviceBinding.Spec.ParametersFrom, serviceBinding.Spec.Parameters) if err != nil { log.Error(err, "failed to parse smBinding parameters") - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err, serviceBinding) + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err.Error(), serviceBinding) } smBinding, operationURL, bindErr := smClient.Bind(&smClientTypes.ServiceBinding{ @@ -225,16 +225,17 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s if bindErr != nil { log.Error(err, "failed to create service binding", "serviceInstanceID", serviceInstance.Status.InstanceID) - if isTransientError(ctx, bindErr) { - return r.markAsTransientError(ctx, smClientTypes.CREATE, bindErr, serviceBinding) + if isTransient, errMsg := isTransientError(ctx, bindErr); isTransient { + return r.markAsTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) + } else { + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) } - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, bindErr, serviceBinding) } if operationURL != "" { var bindingID string if bindingID = sm.ExtractBindingID(operationURL); len(bindingID) == 0 { - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, fmt.Errorf("failed to extract smBinding ID from operation URL %s", operationURL), serviceBinding) + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, fmt.Sprintf("failed to extract smBinding ID from operation URL %s", operationURL), serviceBinding) } serviceBinding.Status.BindingID = bindingID @@ -295,7 +296,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, smClient sm.Clien operationURL, unbindErr := smClient.Unbind(serviceBinding.Status.BindingID, nil, buildUserInfo(ctx, serviceBinding.Spec.UserInfo)) if unbindErr != nil { // delete will proceed anyway - return r.markAsNonTransientError(ctx, smClientTypes.DELETE, unbindErr, serviceBinding) + return r.markAsNonTransientError(ctx, smClientTypes.DELETE, unbindErr.Error(), serviceBinding) } if operationURL != "" { @@ -336,8 +337,7 @@ func (r *ServiceBindingReconciler) poll(ctx context.Context, smClient sm.Client, } if status == nil { - err := fmt.Errorf("failed to get last operation status of %s", serviceBinding.Name) - return r.markAsTransientError(ctx, serviceBinding.Status.OperationType, err, serviceBinding) + return r.markAsTransientError(ctx, serviceBinding.Status.OperationType, fmt.Sprintf("failed to get last operation status of %s", serviceBinding.Name), serviceBinding) } switch status.State { case smClientTypes.INPROGRESS: @@ -689,9 +689,9 @@ func (r *ServiceBindingReconciler) handleSecretError(ctx context.Context, op smC log := GetLogger(ctx) log.Error(err, fmt.Sprintf("failed to store secret %s for binding %s", binding.Spec.SecretName, binding.Name)) if apierrors.ReasonForError(err) == metav1.StatusReasonUnknown { - return r.markAsNonTransientError(ctx, op, err, binding) + return r.markAsNonTransientError(ctx, op, err.Error(), binding) } - return r.markAsTransientError(ctx, op, err, binding) + return r.markAsTransientError(ctx, op, err.Error(), binding) } func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding *servicesv1.ServiceBinding, credentialsMap map[string][]byte) ([]SecretMetadataProperty, error) { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index b220af4d..c6719bdc 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -81,7 +81,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ smClient, err := r.getSMClient(ctx, serviceInstance) if err != nil { - return r.markAsTransientError(ctx, Unknown, err, serviceInstance) + return r.markAsTransientError(ctx, Unknown, err.Error(), serviceInstance) } if len(serviceInstance.Status.OperationURL) > 0 { @@ -112,7 +112,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ instance, err := r.getInstanceForRecovery(ctx, smClient, serviceInstance) if err != nil { log.Error(err, "failed to check instance recovery") - return r.markAsTransientError(ctx, Unknown, err, serviceInstance) + return r.markAsTransientError(ctx, Unknown, err.Error(), serviceInstance) } if instance != nil { log.Info(fmt.Sprintf("found existing instance in SM with id %s, updating status", instance.ID)) @@ -257,7 +257,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient if err != nil { // if parameters are invalid there is nothing we can do, the user should fix it according to the error message in the condition log.Error(err, "failed to parse instance parameters") - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err, serviceInstance) + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, err.Error(), serviceInstance) } provision, provisionErr := smClient.Provision(&smClientTypes.ServiceInstance{ @@ -274,10 +274,11 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient if provisionErr != nil { log.Error(provisionErr, "failed to create service instance", "serviceOfferingName", serviceInstance.Spec.ServiceOfferingName, "servicePlanName", serviceInstance.Spec.ServicePlanName) - if isTransientError(ctx, provisionErr) { - return r.markAsTransientError(ctx, smClientTypes.CREATE, provisionErr, serviceInstance) + if isTransient, errMsg := isTransientError(ctx, provisionErr); isTransient { + return r.markAsTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) + } else { + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) } - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, provisionErr, serviceInstance) } if provision.Location != "" { @@ -329,7 +330,7 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient _, instanceParameters, err := buildParameters(r.Client, serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters) if err != nil { log.Error(err, "failed to parse instance parameters") - return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, fmt.Errorf("failed to parse parameters: %v", err.Error()), serviceInstance) + return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, fmt.Sprintf("failed to parse parameters: %v", err.Error()), serviceInstance) } _, operationURL, err := smClient.UpdateInstance(serviceInstance.Status.InstanceID, &smClientTypes.ServiceInstance{ @@ -340,10 +341,11 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient if err != nil { log.Error(err, fmt.Sprintf("failed to update service instance with ID %s", serviceInstance.Status.InstanceID)) - if isTransientError(ctx, err) { - return r.markAsTransientError(ctx, smClientTypes.UPDATE, err, serviceInstance) + if isTransient, errMsg := isTransientError(ctx, err); isTransient { + return r.markAsTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) + } else { + return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) } - return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, err, serviceInstance) } if operationURL != "" { @@ -387,7 +389,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient operationURL, deprovisionErr := smClient.Deprovision(serviceInstance.Status.InstanceID, nil, buildUserInfo(ctx, serviceInstance.Spec.UserInfo)) if deprovisionErr != nil { // delete will proceed anyway - return r.markAsNonTransientError(ctx, smClientTypes.DELETE, deprovisionErr, serviceInstance) + return r.markAsNonTransientError(ctx, smClientTypes.DELETE, deprovisionErr.Error(), serviceInstance) } if operationURL != "" { From a5ff4a2b5d7642145ac24bf22d1d05df34406211 Mon Sep 17 00:00:00 2001 From: Tal Shor Date: Mon, 24 Jul 2023 12:41:51 +0300 Subject: [PATCH 48/49] fix --- controllers/servicebinding_controller.go | 9 ++++++--- controllers/serviceinstance_controller.go | 21 ++++++++++++++------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index b787986f..b9726904 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -203,6 +203,10 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance, serviceBinding *servicesv1.ServiceBinding) (ctrl.Result, error) { + var ( + isTransient bool + errMsg string + ) log := GetLogger(ctx) log.Info("Creating smBinding in SM") serviceBinding.Status.InstanceID = serviceInstance.Status.InstanceID @@ -225,11 +229,10 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s if bindErr != nil { log.Error(err, "failed to create service binding", "serviceInstanceID", serviceInstance.Status.InstanceID) - if isTransient, errMsg := isTransientError(ctx, bindErr); isTransient { + if isTransient, errMsg = isTransientError(ctx, bindErr); isTransient { return r.markAsTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) - } else { - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) } + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) } if operationURL != "" { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index c6719bdc..f9c0f233 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -250,6 +250,10 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, smClient sm.Client } func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) { + var ( + isTransient bool + errMsg string + ) log := GetLogger(ctx) log.Info("Creating instance in SM") updateHashedSpecValue(serviceInstance) @@ -274,11 +278,11 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient if provisionErr != nil { log.Error(provisionErr, "failed to create service instance", "serviceOfferingName", serviceInstance.Spec.ServiceOfferingName, "servicePlanName", serviceInstance.Spec.ServicePlanName) - if isTransient, errMsg := isTransientError(ctx, provisionErr); isTransient { + if isTransient, errMsg = isTransientError(ctx, provisionErr); isTransient { return r.markAsTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) - } else { - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) } + return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) + } if provision.Location != "" { @@ -321,7 +325,11 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient } func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) { - var err error + var ( + isTransient bool + errMsg string + err error + ) log := GetLogger(ctx) log.Info(fmt.Sprintf("updating instance %s in SM", serviceInstance.Status.InstanceID)) @@ -341,11 +349,10 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient if err != nil { log.Error(err, fmt.Sprintf("failed to update service instance with ID %s", serviceInstance.Status.InstanceID)) - if isTransient, errMsg := isTransientError(ctx, err); isTransient { + if isTransient, errMsg = isTransientError(ctx, err); isTransient { return r.markAsTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) - } else { - return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) } + return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) } if operationURL != "" { From 7457b3a2eb93b8de10855f62cfe468820ab053ea Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 24 Jul 2023 13:32:50 +0300 Subject: [PATCH 49/49] cosmetics --- controllers/base_controller.go | 12 ++++++++++++ controllers/servicebinding_controller.go | 9 +-------- controllers/serviceinstance_controller.go | 20 ++------------------ 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 5ee4e5dd..2fd3b46f 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -327,6 +327,18 @@ func isBrokerErrorExist(smError *sm.ServiceManagerError) bool { return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0 } +func (r *BaseReconciler) handleError(ctx context.Context, operationType smClientTypes.OperationCategory, err error, resource api.SAPBTPResource) (ctrl.Result, error) { + var ( + isTransient bool + errMsg string + ) + + if isTransient, errMsg = isTransientError(ctx, err); isTransient { + return r.markAsTransientError(ctx, operationType, errMsg, resource) + } + return r.markAsNonTransientError(ctx, operationType, errMsg, resource) +} + func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) setFailureConditions(operationType, errMsg, object) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index b9726904..479896cb 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -203,10 +203,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque } func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance, serviceBinding *servicesv1.ServiceBinding) (ctrl.Result, error) { - var ( - isTransient bool - errMsg string - ) log := GetLogger(ctx) log.Info("Creating smBinding in SM") serviceBinding.Status.InstanceID = serviceInstance.Status.InstanceID @@ -229,10 +225,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s if bindErr != nil { log.Error(err, "failed to create service binding", "serviceInstanceID", serviceInstance.Status.InstanceID) - if isTransient, errMsg = isTransientError(ctx, bindErr); isTransient { - return r.markAsTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) - } - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceBinding) + return r.handleError(ctx, smClientTypes.CREATE, bindErr, serviceBinding) } if operationURL != "" { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index f9c0f233..e8d5f629 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -250,10 +250,6 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, smClient sm.Client } func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) { - var ( - isTransient bool - errMsg string - ) log := GetLogger(ctx) log.Info("Creating instance in SM") updateHashedSpecValue(serviceInstance) @@ -278,11 +274,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient if provisionErr != nil { log.Error(provisionErr, "failed to create service instance", "serviceOfferingName", serviceInstance.Spec.ServiceOfferingName, "servicePlanName", serviceInstance.Spec.ServicePlanName) - if isTransient, errMsg = isTransientError(ctx, provisionErr); isTransient { - return r.markAsTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) - } - return r.markAsNonTransientError(ctx, smClientTypes.CREATE, errMsg, serviceInstance) - + return r.handleError(ctx, smClientTypes.CREATE, provisionErr, serviceInstance) } if provision.Location != "" { @@ -325,11 +317,6 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient } func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1.ServiceInstance) (ctrl.Result, error) { - var ( - isTransient bool - errMsg string - err error - ) log := GetLogger(ctx) log.Info(fmt.Sprintf("updating instance %s in SM", serviceInstance.Status.InstanceID)) @@ -349,10 +336,7 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient if err != nil { log.Error(err, fmt.Sprintf("failed to update service instance with ID %s", serviceInstance.Status.InstanceID)) - if isTransient, errMsg = isTransientError(ctx, err); isTransient { - return r.markAsTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) - } - return r.markAsNonTransientError(ctx, smClientTypes.UPDATE, errMsg, serviceInstance) + return r.handleError(ctx, smClientTypes.UPDATE, err, serviceInstance) } if operationURL != "" {