From 1469afea839c753571c362be3810f7b081012a82 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 4 Dec 2023 11:17:59 +0200 Subject: [PATCH 01/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/common.go | 19 ++++---- api/v1/servicebinding_types.go | 8 ++++ api/v1/serviceinstance_types.go | 7 +++ controllers/base_controller.go | 53 +++++++++++++++++++---- controllers/serviceinstance_controller.go | 2 +- 5 files changed, 72 insertions(+), 17 deletions(-) diff --git a/api/common.go b/api/common.go index f81a1638..10f0eabe 100644 --- a/api/common.go +++ b/api/common.go @@ -11,14 +11,15 @@ import ( type ControllerName string const ( - ServiceInstanceController ControllerName = "ServiceInstance" - ServiceBindingController ControllerName = "ServiceBinding" - FinalizerName string = "services.cloud.sap.com/sap-btp-finalizer" - StaleBindingIDLabel string = "services.cloud.sap.com/stale" - StaleBindingRotationOfLabel string = "services.cloud.sap.com/rotationOf" - ForceRotateAnnotation string = "services.cloud.sap.com/forceRotate" - PreventDeletion string = "services.cloud.sap.com/preventDeletion" - UseInstanceMetadataNameInSecret string = "services.cloud.sap.com/useInstanceMetadataName" + ServiceInstanceController ControllerName = "ServiceInstance" + ServiceBindingController ControllerName = "ServiceBinding" + FinalizerName string = "services.cloud.sap.com/sap-btp-finalizer" + StaleBindingIDLabel string = "services.cloud.sap.com/stale" + StaleBindingRotationOfLabel string = "services.cloud.sap.com/rotationOf" + ForceRotateAnnotation string = "services.cloud.sap.com/forceRotate" + PreventDeletion string = "services.cloud.sap.com/preventDeletion" + UseInstanceMetadataNameInSecret string = "services.cloud.sap.com/useInstanceMetadataName" + IgnoreNonTransientErrorAnnotation string = "services.cloud.sap.com/ignoreNonTransientError" ) type HTTPStatusCodeError struct { @@ -79,4 +80,6 @@ type SAPBTPResource interface { DeepClone() SAPBTPResource SetReady(metav1.ConditionStatus) GetReady() metav1.ConditionStatus + GetAnnotations() map[string]string + SetAnnotations(map[string]string) } diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index 86c3e638..22466678 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -188,6 +188,14 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) { sb.Status.Ready = ready } +func (sb *ServiceBinding) GetAnnotations() map[string]string { + return sb.Annotations +} + +func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) { + sb.Annotations = annotations +} + // +kubebuilder:object:root=true // ServiceBindingList contains a list of ServiceBinding diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index 02f2eb38..f56b6e4a 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -185,6 +185,13 @@ func (si *ServiceInstance) GetReady() metav1.ConditionStatus { func (si *ServiceInstance) SetReady(ready metav1.ConditionStatus) { si.Status.Ready = ready } +func (si *ServiceInstance) GetAnnotations() map[string]string { + return si.Annotations +} + +func (si *ServiceInstance) SetAnnotations(annotations map[string]string) { + si.Annotations = annotations +} // +kubebuilder:object:root=true diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 5944dd80..1e7341a1 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -3,8 +3,6 @@ package controllers import ( "context" "fmt" - "net/http" - "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -17,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + "net/http" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -135,7 +134,15 @@ func (r *BaseReconciler) removeFinalizer(ctx context.Context, object api.SAPBTPR func (r *BaseReconciler) updateStatus(ctx context.Context, object api.SAPBTPResource) error { log := GetLogger(ctx) log.Info(fmt.Sprintf("updating %s status", object.GetObjectKind().GroupVersionKind().Kind)) - return r.Client.Status().Update(ctx, object) + if err := r.Client.Status().Update(ctx, object); err != nil { + return err + } + succeededCondition := meta.FindStatusCondition(object.GetConditions(), api.ConditionSucceeded) + if succeededCondition != nil && + succeededCondition.Reason != InProgress && succeededCondition.Reason != CreateInProgress && succeededCondition.Reason != UpdateInProgress { + return r.removeIgnoreNonTransientErrorAnnotation(ctx, object) + } + return nil } func (r *BaseReconciler) init(ctx context.Context, obj api.SAPBTPResource) error { @@ -244,7 +251,6 @@ func setSuccessConditions(operationType smClientTypes.OperationCategory, object object.SetConditions(conditions) object.SetReady(metav1.ConditionTrue) - } func setCredRotationInProgressConditions(reason, message string, object api.SAPBTPResource) { @@ -314,10 +320,24 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func isTransientError(smError *sm.ServiceManagerError, log logr.Logger) bool { +func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { statusCode := smError.GetStatusCode() log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) - return isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) + isTransient := isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) + annotations := resource.GetAnnotations() + if !isTransient && statusCode == http.StatusBadRequest && annotations != nil { + if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + log.Info("ignoreNonTransientErrorAnnotation checking timeout") + timeoutReached := true + if !timeoutReached { + log.Info("ignoreNonTransientErrorAnnotation consider error to be transient") + return true + } + log.Info("ignoreNonTransientErrorAnnotation timeout reached") + return false + } + } + return isTransient } func isConcurrentOperationError(smError *sm.ServiceManagerError) bool { @@ -341,8 +361,8 @@ func (r *BaseReconciler) handleError(ctx context.Context, operationType smClient log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) } - - if isTransient := isTransientError(smError, log); isTransient { + isTransient := isTransientError(smError, log, resource) + if isTransient { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } return r.markAsNonTransientError(ctx, operationType, smError.Error(), resource) @@ -359,12 +379,29 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT if err != nil { return ctrl.Result{}, err } + err = r.removeIgnoreNonTransientErrorAnnotation(ctx, object) + if err != nil { + return ctrl.Result{}, err + } if operationType == smClientTypes.DELETE { return ctrl.Result{}, fmt.Errorf(errMsg) } return ctrl.Result{}, nil } +func (r *BaseReconciler) removeIgnoreNonTransientErrorAnnotation(ctx context.Context, object api.SAPBTPResource) error { + log := GetLogger(ctx) + annotations := object.GetAnnotations() + if annotations != nil { + if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + delete(annotations, api.IgnoreNonTransientErrorAnnotation) + log.Info("deleting ignoreNonTransientErrorAnnotation") + return r.Client.Update(ctx, object) + } + } + return nil +} + func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) setInProgressConditions(ctx, operationType, errMsg, object) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 41936e76..ee8fd11d 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -504,7 +504,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error with status code %d", smError.StatusCode)) - isTransient = isTransientError(smError, log) + isTransient = isTransientError(smError, log, object) errMsg = smError.Error() if smError.StatusCode == http.StatusTooManyRequests { From d1e9565d622000678c9f6b7b11cea3b0c3ff2761 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 4 Dec 2023 12:03:38 +0200 Subject: [PATCH 02/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 1e7341a1..1ce847d4 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -325,7 +325,7 @@ func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) isTransient := isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) annotations := resource.GetAnnotations() - if !isTransient && statusCode == http.StatusBadRequest && annotations != nil { + if !isTransient && statusCode != http.StatusInternalServerError && annotations != nil { if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation checking timeout") timeoutReached := true @@ -379,10 +379,6 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT if err != nil { return ctrl.Result{}, err } - err = r.removeIgnoreNonTransientErrorAnnotation(ctx, object) - if err != nil { - return ctrl.Result{}, err - } if operationType == smClientTypes.DELETE { return ctrl.Result{}, fmt.Errorf(errMsg) } From b5b904d8812af1d1d24b4d979a47681701b90521 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 4 Dec 2023 12:06:12 +0200 Subject: [PATCH 03/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 1ce847d4..7d35be22 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -361,8 +361,7 @@ func (r *BaseReconciler) handleError(ctx context.Context, operationType smClient log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) } - isTransient := isTransientError(smError, log, resource) - if isTransient { + if isTransient := isTransientError(smError, log, resource); isTransient { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } return r.markAsNonTransientError(ctx, operationType, smError.Error(), resource) From 4baf2a29000775daf5ac7a98e2e102af21b4f4ac Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 4 Dec 2023 12:34:13 +0200 Subject: [PATCH 04/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 7d35be22..96cb13d8 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -3,6 +3,8 @@ package controllers import ( "context" "fmt" + "net/http" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -15,7 +17,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" - "net/http" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" From dd373f0ee621c9832d5effe9a966661a783c55ac Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 4 Dec 2023 13:53:19 +0200 Subject: [PATCH 05/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/servicebinding_types_test.go | 9 +++++++++ api/v1/serviceinstance_types_test.go | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/api/v1/servicebinding_types_test.go b/api/v1/servicebinding_types_test.go index ed4d9cb2..0f354a93 100644 --- a/api/v1/servicebinding_types_test.go +++ b/api/v1/servicebinding_types_test.go @@ -97,4 +97,13 @@ var _ = Describe("Service Binding Type Test", func() { binding.SetStatus(status) Expect(binding.GetStatus()).To(Equal(status)) }) + + It("should update annotation", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + } + binding.SetAnnotations(annotation) + Expect(binding.GetAnnotations()).To(Equal(annotation)) + }) + }) diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 5f1494cb..14d845f1 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -121,4 +121,12 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetStatus(status) Expect(instance.GetStatus()).To(Equal(status)) }) + + It("should update annotation", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + } + instance.SetAnnotations(annotation) + Expect(instance.GetAnnotations()).To(Equal(annotation)) + }) }) From 1519b547b4b9d3a5ee5f18c0cbab466700e35caa Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 4 Dec 2023 15:37:29 +0200 Subject: [PATCH 06/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 12 ++----- controllers/servicebinding_controller.go | 4 ++- controllers/serviceinstance_controller.go | 3 +- internal/config/config.go | 40 ++++++++++++----------- 4 files changed, 28 insertions(+), 31 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 96cb13d8..7c639e3c 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -135,15 +135,7 @@ func (r *BaseReconciler) removeFinalizer(ctx context.Context, object api.SAPBTPR func (r *BaseReconciler) updateStatus(ctx context.Context, object api.SAPBTPResource) error { log := GetLogger(ctx) log.Info(fmt.Sprintf("updating %s status", object.GetObjectKind().GroupVersionKind().Kind)) - if err := r.Client.Status().Update(ctx, object); err != nil { - return err - } - succeededCondition := meta.FindStatusCondition(object.GetConditions(), api.ConditionSucceeded) - if succeededCondition != nil && - succeededCondition.Reason != InProgress && succeededCondition.Reason != CreateInProgress && succeededCondition.Reason != UpdateInProgress { - return r.removeIgnoreNonTransientErrorAnnotation(ctx, object) - } - return nil + return r.Client.Status().Update(ctx, object) } func (r *BaseReconciler) init(ctx context.Context, obj api.SAPBTPResource) error { @@ -326,7 +318,7 @@ func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) isTransient := isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) annotations := resource.GetAnnotations() - if !isTransient && statusCode != http.StatusInternalServerError && annotations != nil { + if !isTransient && annotations != nil { if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation checking timeout") timeoutReached := true diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index 76e583bb..ca2fbf23 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -138,6 +138,8 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque if isBindingReady { log.Info("Binding in final state") + r.removeIgnoreNonTransientErrorAnnotation(ctx, serviceInstance) + return r.maintain(ctx, serviceBinding) } @@ -472,7 +474,7 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *servic return ctrl.Result{}, r.updateStatus(ctx, binding) } - return ctrl.Result{}, nil + return ctrl.Result{}, r.removeIgnoreNonTransientErrorAnnotation(ctx, binding) } func (r *ServiceBindingReconciler) getServiceInstanceForBinding(ctx context.Context, binding *servicesv1.ServiceBinding) (*servicesv1.ServiceInstance, error) { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index ee8fd11d..b9301652 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -82,7 +82,8 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ updateHashedSpecValue(serviceInstance) return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance) } - return ctrl.Result{}, nil + + return ctrl.Result{}, r.removeIgnoreNonTransientErrorAnnotation(ctx, serviceInstance) } if isMarkedForDeletion(serviceInstance.ObjectMeta) { diff --git a/internal/config/config.go b/internal/config/config.go index f162bc8c..3fb3dfc8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -13,30 +13,32 @@ var ( ) type Config struct { - SyncPeriod time.Duration `envconfig:"sync_period"` - PollInterval time.Duration `envconfig:"poll_interval"` - LongPollInterval time.Duration `envconfig:"long_poll_interval"` - ManagementNamespace string `envconfig:"management_namespace"` - ReleaseNamespace string `envconfig:"release_namespace"` - AllowClusterAccess bool `envconfig:"allow_cluster_access"` - AllowedNamespaces []string `envconfig:"allowed_namespaces"` - EnableNamespaceSecrets bool `envconfig:"enable_namespace_secrets"` - ClusterID string `envconfig:"cluster_id"` - RetryBaseDelay time.Duration `envconfig:"retry_base_delay"` - RetryMaxDelay time.Duration `envconfig:"retry_max_delay"` + SyncPeriod time.Duration `envconfig:"sync_period"` + PollInterval time.Duration `envconfig:"poll_interval"` + LongPollInterval time.Duration `envconfig:"long_poll_interval"` + ManagementNamespace string `envconfig:"management_namespace"` + ReleaseNamespace string `envconfig:"release_namespace"` + AllowClusterAccess bool `envconfig:"allow_cluster_access"` + AllowedNamespaces []string `envconfig:"allowed_namespaces"` + EnableNamespaceSecrets bool `envconfig:"enable_namespace_secrets"` + ClusterID string `envconfig:"cluster_id"` + RetryBaseDelay time.Duration `envconfig:"retry_base_delay"` + RetryMaxDelay time.Duration `envconfig:"retry_max_delay"` + IgnoreNonTransientTimeout time.Duration `envconfig:"ignore_non_transient_timeout"` } func Get() Config { loadOnce.Do(func() { config = Config{ // default values - SyncPeriod: 60 * time.Second, - PollInterval: 10 * time.Second, - LongPollInterval: 5 * time.Minute, - EnableNamespaceSecrets: true, - AllowedNamespaces: []string{}, - AllowClusterAccess: true, - RetryBaseDelay: 10 * time.Second, - RetryMaxDelay: time.Hour, + SyncPeriod: 60 * time.Second, + PollInterval: 10 * time.Second, + LongPollInterval: 5 * time.Minute, + EnableNamespaceSecrets: true, + AllowedNamespaces: []string{}, + AllowClusterAccess: true, + RetryBaseDelay: 10 * time.Second, + RetryMaxDelay: time.Hour, + IgnoreNonTransientTimeout: 2 * time.Hour, } envconfig.MustProcess("", &config) }) From e882c72c339ede6e477cde9e64b5b18cd26a8432 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 5 Dec 2023 14:25:02 +0200 Subject: [PATCH 07/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 7c639e3c..28da36da 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "time" "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" @@ -316,21 +317,22 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { statusCode := smError.GetStatusCode() log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) - isTransient := isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) + if isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) { + return true + } annotations := resource.GetAnnotations() - if !isTransient && annotations != nil { + if annotations != nil { if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation checking timeout") - timeoutReached := true - if !timeoutReached { - log.Info("ignoreNonTransientErrorAnnotation consider error to be transient") - return true + if time.Since(resource.GetCreationTimestamp().Time) > config.Get().IgnoreNonTransientTimeout { + log.Info("ignoreNonTransientErrorAnnotation timeout reached") + return false } - log.Info("ignoreNonTransientErrorAnnotation timeout reached") - return false + log.Info("ignoreNonTransientErrorAnnotation consider error to be transient") + return true } } - return isTransient + return false } func isConcurrentOperationError(smError *sm.ServiceManagerError) bool { @@ -384,6 +386,7 @@ func (r *BaseReconciler) removeIgnoreNonTransientErrorAnnotation(ctx context.Con if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { delete(annotations, api.IgnoreNonTransientErrorAnnotation) log.Info("deleting ignoreNonTransientErrorAnnotation") + object.SetAnnotations(annotations) return r.Client.Update(ctx, object) } } From a4714f8e5669c3bbd25c8bb2e6e856d88d27f501 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 5 Dec 2023 14:45:28 +0200 Subject: [PATCH 08/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 6 +++--- controllers/servicebinding_controller.go | 4 ++-- controllers/serviceinstance_controller.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 28da36da..d20db55d 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -379,12 +379,12 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT return ctrl.Result{}, nil } -func (r *BaseReconciler) removeIgnoreNonTransientErrorAnnotation(ctx context.Context, object api.SAPBTPResource) error { +func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTPResource, key string) error { log := GetLogger(ctx) annotations := object.GetAnnotations() if annotations != nil { - if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - delete(annotations, api.IgnoreNonTransientErrorAnnotation) + if _, ok := annotations[key]; ok { + delete(annotations, key) log.Info("deleting ignoreNonTransientErrorAnnotation") object.SetAnnotations(annotations) return r.Client.Update(ctx, object) diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index ca2fbf23..fbc7bbe8 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -138,7 +138,7 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque if isBindingReady { log.Info("Binding in final state") - r.removeIgnoreNonTransientErrorAnnotation(ctx, serviceInstance) + r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation) return r.maintain(ctx, serviceBinding) } @@ -474,7 +474,7 @@ func (r *ServiceBindingReconciler) maintain(ctx context.Context, binding *servic return ctrl.Result{}, r.updateStatus(ctx, binding) } - return ctrl.Result{}, r.removeIgnoreNonTransientErrorAnnotation(ctx, binding) + return ctrl.Result{}, nil } func (r *ServiceBindingReconciler) getServiceInstanceForBinding(ctx context.Context, binding *servicesv1.ServiceBinding) (*servicesv1.ServiceInstance, error) { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index b9301652..22d6c41b 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -83,7 +83,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance) } - return ctrl.Result{}, r.removeIgnoreNonTransientErrorAnnotation(ctx, serviceInstance) + return ctrl.Result{}, r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation) } if isMarkedForDeletion(serviceInstance.ObjectMeta) { From 8069aa17b2c3bf3329a67b4dca92189a3a4195bd Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 5 Dec 2023 15:09:55 +0200 Subject: [PATCH 09/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/common.go | 1 + api/v1/servicebinding_types.go | 4 ++++ api/v1/serviceinstance_types.go | 4 ++++ controllers/base_controller.go | 2 +- controllers/servicebinding_controller.go | 2 -- 5 files changed, 10 insertions(+), 3 deletions(-) diff --git a/api/common.go b/api/common.go index 10f0eabe..199dd425 100644 --- a/api/common.go +++ b/api/common.go @@ -82,4 +82,5 @@ type SAPBTPResource interface { GetReady() metav1.ConditionStatus GetAnnotations() map[string]string SetAnnotations(map[string]string) + SupportIgnoreNonTransientErrorAnnotation() bool } diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index 22466678..99144c46 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -196,6 +196,10 @@ func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) { sb.Annotations = annotations } +func (si *ServiceBinding) SupportIgnoreNonTransientErrorAnnotation() bool { + return false +} + // +kubebuilder:object:root=true // ServiceBindingList contains a list of ServiceBinding diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index f56b6e4a..90361389 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -193,6 +193,10 @@ func (si *ServiceInstance) SetAnnotations(annotations map[string]string) { si.Annotations = annotations } +func (si *ServiceInstance) SupportIgnoreNonTransientErrorAnnotation() bool { + return true +} + // +kubebuilder:object:root=true // ServiceInstanceList contains a list of ServiceInstance diff --git a/controllers/base_controller.go b/controllers/base_controller.go index d20db55d..f9eb7c23 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -321,7 +321,7 @@ func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource return true } annotations := resource.GetAnnotations() - if annotations != nil { + if annotations != nil && resource.SupportIgnoreNonTransientErrorAnnotation() { if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation checking timeout") if time.Since(resource.GetCreationTimestamp().Time) > config.Get().IgnoreNonTransientTimeout { diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index fbc7bbe8..76e583bb 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -138,8 +138,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque if isBindingReady { log.Info("Binding in final state") - r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation) - return r.maintain(ctx, serviceBinding) } From 5ac9c7b447dd00762fd0f943fa21b8c7adaf841a Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 5 Dec 2023 15:11:55 +0200 Subject: [PATCH 10/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index f9eb7c23..5e6512ad 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -379,6 +379,17 @@ func (r *BaseReconciler) markAsNonTransientError(ctx context.Context, operationT return ctrl.Result{}, nil } +func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { + log := GetLogger(ctx) + setInProgressConditions(ctx, 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{}, fmt.Errorf(errMsg) +} + func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTPResource, key string) error { log := GetLogger(ctx) annotations := object.GetAnnotations() @@ -393,17 +404,6 @@ func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTP return nil } -func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType smClientTypes.OperationCategory, errMsg string, object api.SAPBTPResource) (ctrl.Result, error) { - log := GetLogger(ctx) - setInProgressConditions(ctx, 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{}, fmt.Errorf(errMsg) -} - func isInProgress(object api.SAPBTPResource) bool { conditions := object.GetConditions() return meta.IsStatusConditionPresentAndEqual(conditions, api.ConditionSucceeded, metav1.ConditionFalse) && From 7c9ae23006d272da6c7ec2431b0dcd9b5c5cda4d Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Wed, 6 Dec 2023 09:28:33 +0200 Subject: [PATCH 11/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/servicebinding_types.go | 2 +- api/v1alpha1/servicebinding_types.go | 4 ++++ api/v1alpha1/serviceinstance_types.go | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index 99144c46..aae3054b 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -196,7 +196,7 @@ func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) { sb.Annotations = annotations } -func (si *ServiceBinding) SupportIgnoreNonTransientErrorAnnotation() bool { +func (sb *ServiceBinding) SupportIgnoreNonTransientErrorAnnotation() bool { return false } diff --git a/api/v1alpha1/servicebinding_types.go b/api/v1alpha1/servicebinding_types.go index 131e95f6..7a04b722 100644 --- a/api/v1alpha1/servicebinding_types.go +++ b/api/v1alpha1/servicebinding_types.go @@ -180,6 +180,10 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) { sb.Status.Ready = ready } +func (sb *ServiceBinding) SupportIgnoreNonTransientErrorAnnotation() bool { + return false +} + // +kubebuilder:object:root=true // ServiceBindingList contains a list of ServiceBinding diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index 04879bb2..a683cde7 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/api/v1alpha1/serviceinstance_types.go @@ -170,6 +170,10 @@ func (in *ServiceInstance) SetReady(ready metav1.ConditionStatus) { in.Status.Ready = ready } +func (in *ServiceInstance) SupportIgnoreNonTransientErrorAnnotation() bool { + return true +} + // +kubebuilder:object:root=true // ServiceInstanceList contains a list of ServiceInstance From 672a596fcbb38cae4a9355539f9116f55cff5575 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Wed, 6 Dec 2023 09:35:37 +0200 Subject: [PATCH 12/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- 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 5e6512ad..a1738513 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -395,8 +395,8 @@ func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTP annotations := object.GetAnnotations() if annotations != nil { if _, ok := annotations[key]; ok { + log.Info("deleting annotation with key %s", key) delete(annotations, key) - log.Info("deleting ignoreNonTransientErrorAnnotation") object.SetAnnotations(annotations) return r.Client.Update(ctx, object) } From c17ca72d290f210e0e0c31434f7857fbaee75d0d Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Wed, 6 Dec 2023 16:08:52 +0200 Subject: [PATCH 13/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/base_controller.go | 15 ++++---- controllers/serviceinstance_controller.go | 2 +- .../serviceinstance_controller_test.go | 35 ++++++++++++++++++- controllers/suite_test.go | 10 +++--- 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index a1738513..05a27ac3 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -314,7 +314,7 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { +func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { statusCode := smError.GetStatusCode() log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) if isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) { @@ -323,12 +323,13 @@ func isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource annotations := resource.GetAnnotations() if annotations != nil && resource.SupportIgnoreNonTransientErrorAnnotation() { if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - log.Info("ignoreNonTransientErrorAnnotation checking timeout") - if time.Since(resource.GetCreationTimestamp().Time) > config.Get().IgnoreNonTransientTimeout { - log.Info("ignoreNonTransientErrorAnnotation timeout reached") + log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") + sinceCreate := time.Since(resource.GetCreationTimestamp().Time) + if sinceCreate > r.Config.IgnoreNonTransientTimeout { + log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceCreate, r.Config.IgnoreNonTransientTimeout)) return false } - log.Info("ignoreNonTransientErrorAnnotation consider error to be transient") + log.Info("timeout didn't reached- consider error to be transient") return true } } @@ -356,7 +357,7 @@ func (r *BaseReconciler) handleError(ctx context.Context, operationType smClient log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) } - if isTransient := isTransientError(smError, log, resource); isTransient { + if isTransient := r.isTransientError(smError, log, resource); isTransient { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } return r.markAsNonTransientError(ctx, operationType, smError.Error(), resource) @@ -395,7 +396,7 @@ func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTP annotations := object.GetAnnotations() if annotations != nil { if _, ok := annotations[key]; ok { - log.Info("deleting annotation with key %s", key) + log.Info(fmt.Sprintf("deleting annotation with key %s", key)) delete(annotations, key) object.SetAnnotations(annotations) return r.Client.Update(ctx, object) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 22d6c41b..e837738b 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -505,7 +505,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error with status code %d", smError.StatusCode)) - isTransient = isTransientError(smError, log, object) + isTransient = r.isTransientError(smError, log, object) errMsg = smError.Error() if smError.StatusCode == http.StatusTooManyRequests { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 4a11bcb2..282f2d25 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -101,6 +101,28 @@ var _ = Describe("ServiceInstance controller", func() { return instance } + createInstanceWithAnnotation := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, annotation map[string]string, waitForReady bool) *v1.ServiceInstance { + instance := &v1.ServiceInstance{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "services.cloud.sap.com/v1", + Kind: "ServiceInstance", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fakeInstanceName, + Namespace: testNamespace, + Annotations: annotation, + }, + Spec: instanceSpec, + } + Expect(k8sClient.Create(ctx, instance)).Should(Succeed()) + + if !waitForReady { + return instance + } + waitForResourceToBeReady(ctx, instance) + return instance + } + 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 { @@ -120,7 +142,7 @@ var _ = Describe("ServiceInstance controller", func() { }, timeout, interval).Should(BeTrue()) } } - + ignoreNonTransientErrorAnnotation := map[string]string{api.IgnoreNonTransientErrorAnnotation: "true"} BeforeEach(func() { ctx = context.Background() log := ctrl.Log.WithName("instanceTest") @@ -238,6 +260,12 @@ var _ = Describe("ServiceInstance controller", func() { It("should have failure condition", func() { serviceInstance = createInstance(ctx, instanceSpec, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) + Expect(fakeClient.ProvisionCallCount()).To(Equal(1)) + }) + It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { + serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") + Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) }) }) @@ -282,6 +310,11 @@ var _ = Describe("ServiceInstance controller", func() { serviceInstance = createInstance(ctx, instanceSpec, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) }) + It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { + serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") + Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) + }) }) }) }) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 8ba241cf..a5044395 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -63,10 +63,11 @@ import ( // http://onsi.github.io/ginkgo/ to learn more about Ginkgo. const ( - timeout = time.Second * 20 - interval = time.Millisecond * 50 - syncPeriod = time.Millisecond * 250 - pollInterval = time.Millisecond * 250 + timeout = time.Second * 20 + interval = time.Millisecond * 50 + syncPeriod = time.Millisecond * 250 + pollInterval = time.Millisecond * 250 + ignoreNonTransientTimeout = time.Second * 1 fakeBindingID = "fake-binding-id" bindingTestNamespace = "test-namespace" @@ -128,6 +129,7 @@ var _ = BeforeSuite(func(done Done) { testConfig := config.Get() testConfig.SyncPeriod = syncPeriod testConfig.PollInterval = pollInterval + testConfig.IgnoreNonTransientTimeout = ignoreNonTransientTimeout By("registering webhooks") k8sManager.GetWebhookServer().Register("/mutate-services-cloud-sap-com-v1-serviceinstance", &webhook.Admission{Handler: &webhooks.ServiceInstanceDefaulter{Decoder: admission.NewDecoder(k8sManager.GetScheme())}}) From 3dba10fe1238da0db77b5feab9b43261dcf66943 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Wed, 6 Dec 2023 16:50:29 +0200 Subject: [PATCH 14/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- .../serviceinstance_controller_test.go | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 282f2d25..705fa5e2 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -247,7 +247,7 @@ var _ = Describe("ServiceInstance controller", func() { }) When("provision request to SM fails", func() { - Context("with 400 status", func() { + Context("first with 400 status then success", func() { errMessage := "failed to provision instance" BeforeEach(func() { fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ @@ -260,15 +260,35 @@ var _ = Describe("ServiceInstance controller", func() { It("should have failure condition", func() { serviceInstance = createInstance(ctx, instanceSpec, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) - Expect(fakeClient.ProvisionCallCount()).To(Equal(1)) }) It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") - Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) + key := getResourceNamespacedName(serviceInstance) + err := k8sClient.Get(ctx, key, serviceInstance) + Expect(err).NotTo(HaveOccurred()) + _, ok := serviceInstance.Annotations[api.IgnoreNonTransientErrorAnnotation] + Expect(ok).To(BeFalse()) + }) + }) + Context("with 400 status", func() { + errMessage := "failed to provision instance" + BeforeEach(func() { + fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ + StatusCode: http.StatusBadRequest, + Description: errMessage, + }) + }) + It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { + serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) + key := getResourceNamespacedName(serviceInstance) + err := k8sClient.Get(ctx, key, serviceInstance) + Expect(err).NotTo(HaveOccurred()) + _, ok := serviceInstance.Annotations[api.IgnoreNonTransientErrorAnnotation] + Expect(ok).To(BeFalse()) }) }) - Context("with 429 status eventually succeeds", func() { BeforeEach(func() { errMessage := "failed to provision instance" From 0843bff258fe8d43e2b6761113bdadfcc11c0c3f Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Thu, 7 Dec 2023 11:01:56 +0200 Subject: [PATCH 15/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/serviceinstance_controller_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 705fa5e2..e0db9c4f 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -23,6 +23,7 @@ import ( "net/http" ctrl "sigs.k8s.io/controller-runtime" "strings" + "time" ) // +kubebuilder:docs-gen:collapse=Imports @@ -287,6 +288,8 @@ var _ = Describe("ServiceInstance controller", func() { Expect(err).NotTo(HaveOccurred()) _, ok := serviceInstance.Annotations[api.IgnoreNonTransientErrorAnnotation] Expect(ok).To(BeFalse()) + sinceCreate := time.Since(serviceInstance.GetCreationTimestamp().Time) + Expect(sinceCreate > ignoreNonTransientTimeout) }) }) Context("with 429 status eventually succeeds", func() { From 752ae8fc3dc238ca4f1831130cdebe8c0b9c6f5f Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Thu, 7 Dec 2023 16:13:18 +0200 Subject: [PATCH 16/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/serviceinstance_controller_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index e0db9c4f..f9c75e8b 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -574,6 +574,22 @@ var _ = Describe("ServiceInstance controller", func() { waitForResourceToBeReady(ctx, serviceInstance) }) }) + Context("spec is changed,ignoreNonTransientErrorAnnotation and sm returns 502 and broker returns non transient error", func() { + errMessage := "broker too many requests" + BeforeEach(func() { + fakeClient.UpdateInstanceReturns(nil, "", getNonTransientBrokerError(errMessage)) + }) + It("recognize the error as transient and eventually succeed", func() { + newExternalName := "my-new-external-name" + uuid.New().String() + serviceInstance.Spec.ExternalName = newExternalName + serviceInstance.Annotations = ignoreNonTransientErrorAnnotation + updateInstance(ctx, serviceInstance) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateInProgress, "") + fakeClient.UpdateInstanceReturns(nil, "", nil) + updateInstance(ctx, serviceInstance) + waitForResourceToBeReady(ctx, serviceInstance) + }) + }) Context("Async", func() { When("spec is changed", func() { From 635aa5f2068877c9fc91b2cbc90dc5bd226f8070 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 11 Dec 2023 13:25:04 +0200 Subject: [PATCH 17/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/common.go | 23 ++++++---- api/v1/servicebinding_types.go | 4 +- api/v1/serviceinstance_types.go | 46 +++++++++++++++++-- api/v1/serviceinstance_types_test.go | 22 +++++++++ api/v1/serviceinstance_validating_webhook.go | 4 +- .../serviceinstance_mutating_webhook.go | 11 +++++ controllers/base_controller.go | 33 +++++-------- controllers/serviceinstance_controller.go | 29 +++++++++--- .../serviceinstance_controller_test.go | 37 +++++++-------- controllers/suite_test.go | 20 ++++++++ 10 files changed, 164 insertions(+), 65 deletions(-) diff --git a/api/common.go b/api/common.go index 199dd425..9cfee480 100644 --- a/api/common.go +++ b/api/common.go @@ -2,6 +2,8 @@ package api import ( "fmt" + "github.com/go-logr/logr" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -11,15 +13,16 @@ import ( type ControllerName string const ( - ServiceInstanceController ControllerName = "ServiceInstance" - ServiceBindingController ControllerName = "ServiceBinding" - FinalizerName string = "services.cloud.sap.com/sap-btp-finalizer" - StaleBindingIDLabel string = "services.cloud.sap.com/stale" - StaleBindingRotationOfLabel string = "services.cloud.sap.com/rotationOf" - ForceRotateAnnotation string = "services.cloud.sap.com/forceRotate" - PreventDeletion string = "services.cloud.sap.com/preventDeletion" - UseInstanceMetadataNameInSecret string = "services.cloud.sap.com/useInstanceMetadataName" - IgnoreNonTransientErrorAnnotation string = "services.cloud.sap.com/ignoreNonTransientError" + ServiceInstanceController ControllerName = "ServiceInstance" + ServiceBindingController ControllerName = "ServiceBinding" + FinalizerName string = "services.cloud.sap.com/sap-btp-finalizer" + StaleBindingIDLabel string = "services.cloud.sap.com/stale" + StaleBindingRotationOfLabel string = "services.cloud.sap.com/rotationOf" + ForceRotateAnnotation string = "services.cloud.sap.com/forceRotate" + PreventDeletion string = "services.cloud.sap.com/preventDeletion" + UseInstanceMetadataNameInSecret string = "services.cloud.sap.com/useInstanceMetadataName" + IgnoreNonTransientErrorAnnotation string = "services.cloud.sap.com/ignoreNonTransientError" + IgnoreNonTransientErrorTimestampAnnotation string = "services.cloud.sap.com/ignoreNonTransientTimestampError" ) type HTTPStatusCodeError struct { @@ -82,5 +85,5 @@ type SAPBTPResource interface { GetReady() metav1.ConditionStatus GetAnnotations() map[string]string SetAnnotations(map[string]string) - SupportIgnoreNonTransientErrorAnnotation() bool + IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool } diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index aae3054b..c02aca3d 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -19,9 +19,11 @@ package v1 import ( "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" + "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -196,7 +198,7 @@ func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) { sb.Annotations = annotations } -func (sb *ServiceBinding) SupportIgnoreNonTransientErrorAnnotation() bool { +func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { return false } diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index 90361389..48c89ff2 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -17,11 +17,14 @@ limitations under the License. package v1 import ( + "fmt" "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" + "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -193,10 +196,6 @@ func (si *ServiceInstance) SetAnnotations(annotations map[string]string) { si.Annotations = annotations } -func (si *ServiceInstance) SupportIgnoreNonTransientErrorAnnotation() bool { - return true -} - // +kubebuilder:object:root=true // ServiceInstanceList contains a list of ServiceInstance @@ -215,3 +214,42 @@ func (si *ServiceInstance) Hub() {} func (si *ServiceInstance) ShouldBeShared() bool { return si.Spec.Shared != nil && *si.Spec.Shared } + +func (si *ServiceInstance) ValidateNonTransientTimestampAnnotation(log logr.Logger) error { + if si.Annotations != nil { + if _, ok := si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; ok { + log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") + annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) + if err != nil { + return fmt.Errorf("annotation %s is not a valid timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) + } + sinceAnnotation := time.Since(annotationTime) + if sinceAnnotation < 0 { + return fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) + } + } + } + + return nil +} + +func (si *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { + if si.Annotations != nil { + if _, ok := si.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") + annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) + if err != nil { + log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) + return false + } + sinceAnnotation := time.Since(annotationTime) + if sinceAnnotation > timeout { + log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) + return false + } + log.Info("timeout didn't reached- consider error to be transient") + return true + } + } + return false +} diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 14d845f1..4e8b5b4a 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -7,6 +7,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "time" ) var _ = Describe("Service Instance Type Test", func() { @@ -129,4 +130,25 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetAnnotations(annotation) Expect(instance.GetAnnotations()).To(Equal(annotation)) }) + + It("validate timestamp annotation- not date", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: "true", + } + instance.SetAnnotations(annotation) + err := instance.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("is not a valid timestamp")) + }) + It("validate timestamp annotation- future date", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + err := instance.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot be a future timestamp")) + }) }) diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index d55d74be..8147ddbd 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -43,7 +43,7 @@ var _ webhook.Validator = &ServiceInstance{} var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) { - return nil, nil + return nil, si.ValidateNonTransientTimestampAnnotation(serviceinstancelog) } func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { @@ -53,7 +53,7 @@ func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admissio if oldInstance.Spec.BTPAccessCredentialsSecret != si.Spec.BTPAccessCredentialsSecret { return nil, fmt.Errorf("changing the btpAccessCredentialsSecret for an existing instance is not allowed") } - return nil, nil + return nil, si.ValidateNonTransientTimestampAnnotation(serviceinstancelog) } func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) { diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index 9efc1a30..bb962ab6 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -3,8 +3,10 @@ package webhooks import ( "context" "encoding/json" + "github.com/SAP/sap-btp-service-operator/api" "net/http" "reflect" + "time" v1admission "k8s.io/api/admission/v1" v1 "k8s.io/api/authentication/v1" @@ -55,6 +57,15 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ Groups: req.UserInfo.Groups, Extra: req.UserInfo.Extra, } + + if instance.Annotations != nil { + if _, ok := instance.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + if _, exist := instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; !exist { + instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation] = time.Now().Format(time.RFC3339) + } + } + } + if req.Operation == v1admission.Create || req.Operation == v1admission.Delete { instance.Spec.UserInfo = userInfo } else if req.Operation == v1admission.Update { diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 05a27ac3..89f7c9a4 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -3,9 +3,6 @@ package controllers import ( "context" "fmt" - "net/http" - "time" - "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -18,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + "net/http" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -320,20 +318,7 @@ func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log l if isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) { return true } - annotations := resource.GetAnnotations() - if annotations != nil && resource.SupportIgnoreNonTransientErrorAnnotation() { - if _, ok := annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - sinceCreate := time.Since(resource.GetCreationTimestamp().Time) - if sinceCreate > r.Config.IgnoreNonTransientTimeout { - log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceCreate, r.Config.IgnoreNonTransientTimeout)) - return false - } - log.Info("timeout didn't reached- consider error to be transient") - return true - } - } - return false + return resource.IsIgnoreNonTransientAnnotationExistAndValid(log, r.Config.IgnoreNonTransientTimeout) } func isConcurrentOperationError(smError *sm.ServiceManagerError) bool { @@ -391,13 +376,19 @@ func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType return ctrl.Result{}, fmt.Errorf(errMsg) } -func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTPResource, key string) error { +func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTPResource, keys ...string) error { log := GetLogger(ctx) annotations := object.GetAnnotations() + shouldUpdate := false if annotations != nil { - if _, ok := annotations[key]; ok { - log.Info(fmt.Sprintf("deleting annotation with key %s", key)) - delete(annotations, key) + for _, key := range keys { + if _, ok := annotations[key]; ok { + log.Info(fmt.Sprintf("deleting annotation with key %s", key)) + delete(annotations, key) + shouldUpdate = true + } + } + if shouldUpdate { object.SetAnnotations(annotations) return r.Client.Update(ctx, object) } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index e837738b..5591a996 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -22,7 +22,9 @@ import ( "encoding/hex" "encoding/json" "fmt" + "github.com/go-logr/logr" "net/http" + "time" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -77,13 +79,13 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } - if isFinalState(ctx, serviceInstance) { + if isFinalState(ctx, serviceInstance, r.Config.IgnoreNonTransientTimeout) { if len(serviceInstance.Status.HashedSpec) == 0 { updateHashedSpecValue(serviceInstance) return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance) } - return ctrl.Result{}, r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation) + return ctrl.Result{}, r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) } if isMarkedForDeletion(serviceInstance.ObjectMeta) { @@ -130,7 +132,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } // Update - if updateRequired(serviceInstance) { + if updateRequired(serviceInstance, log, r.Config.IgnoreNonTransientTimeout) { if res, err := r.updateInstance(ctx, smClient, serviceInstance); err != nil { log.Info("got error while trying to update instance") return ctrl.Result{}, err @@ -524,7 +526,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte return ctrl.Result{Requeue: isTransient}, r.updateStatus(ctx, object) } -func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) bool { +func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance, nonTransientTimeout time.Duration) bool { log := GetLogger(ctx) if isMarkedForDeletion(serviceInstance.ObjectMeta) { log.Info("instance is not in final state, it is marked for deletion") @@ -544,7 +546,10 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan log.Info("instance is not in final state, sync operation is in progress") return false } - + if isInRetry(serviceInstance, log, nonTransientTimeout) { + log.Info("instance is not in final state, IgnoreNonTransientErrorAnnotation exist") + return false + } if sharingUpdateRequired(serviceInstance) { log.Info("instance is not in final state, need to sync sharing status") if len(serviceInstance.Status.HashedSpec) == 0 { @@ -557,8 +562,16 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan return true } +func isInRetry(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { + conditions := serviceInstance.GetConditions() + if meta.IsStatusConditionPresentAndEqual(conditions, api.ConditionSucceeded, metav1.ConditionFalse) { + return serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, nonTransientTimeout) + } + return false +} + // TODO unit test -func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { +func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { //update is not supported for failed instances (this can occur when instance creation was asynchronously) if serviceInstance.Status.Ready != metav1.ConditionTrue { return false @@ -568,7 +581,9 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { if cond != nil && cond.Reason == UpdateInProgress { //in case of transient error occurred return true } - + if serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, nonTransientTimeout) { + return true + } return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec } diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index f9c75e8b..de5d88d1 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -255,7 +255,7 @@ var _ = Describe("ServiceInstance controller", func() { StatusCode: http.StatusBadRequest, Description: errMessage, }) - fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + fakeClient.ProvisionReturnsOnCall(2, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) It("should have failure condition", func() { @@ -265,11 +265,7 @@ var _ = Describe("ServiceInstance controller", func() { It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") - key := getResourceNamespacedName(serviceInstance) - err := k8sClient.Get(ctx, key, serviceInstance) - Expect(err).NotTo(HaveOccurred()) - _, ok := serviceInstance.Annotations[api.IgnoreNonTransientErrorAnnotation] - Expect(ok).To(BeFalse()) + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) }) }) Context("with 400 status", func() { @@ -283,11 +279,7 @@ var _ = Describe("ServiceInstance controller", func() { It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) - key := getResourceNamespacedName(serviceInstance) - err := k8sClient.Get(ctx, key, serviceInstance) - Expect(err).NotTo(HaveOccurred()) - _, ok := serviceInstance.Annotations[api.IgnoreNonTransientErrorAnnotation] - Expect(ok).To(BeFalse()) + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) sinceCreate := time.Since(serviceInstance.GetCreationTimestamp().Time) Expect(sinceCreate > ignoreNonTransientTimeout) }) @@ -326,7 +318,7 @@ var _ = Describe("ServiceInstance controller", func() { errMessage := "failed to provision instance" BeforeEach(func() { fakeClient.ProvisionReturns(nil, getNonTransientBrokerError(errMessage)) - fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + fakeClient.ProvisionReturnsOnCall(2, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) It("should have failure condition - non transient error", func() { @@ -336,6 +328,7 @@ var _ = Describe("ServiceInstance controller", func() { It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) }) }) @@ -582,12 +575,16 @@ var _ = Describe("ServiceInstance controller", func() { It("recognize the error as transient and eventually succeed", func() { newExternalName := "my-new-external-name" + uuid.New().String() serviceInstance.Spec.ExternalName = newExternalName + updateInstance(ctx, serviceInstance) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateFailed, "") serviceInstance.Annotations = ignoreNonTransientErrorAnnotation updateInstance(ctx, serviceInstance) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateInProgress, "") fakeClient.UpdateInstanceReturns(nil, "", nil) - updateInstance(ctx, serviceInstance) + waitForResourceToBeReady(ctx, serviceInstance) + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) + }) }) @@ -1120,7 +1117,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) }) When("Succeeded is false", func() { @@ -1146,7 +1143,7 @@ var _ = Describe("ServiceInstance controller", func() { }, } instance.SetGeneration(1) - Expect(isFinalState(ctx, instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) }) }) }) @@ -1170,7 +1167,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) }) }) @@ -1196,7 +1193,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) }) }) @@ -1222,7 +1219,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) }) }) @@ -1254,7 +1251,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance)).To(BeFalse()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) }) }) @@ -1286,7 +1283,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance)).To(BeTrue()) + Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeTrue()) }) }) }) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index a5044395..29e48924 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -259,6 +259,26 @@ func waitForResourceCondition(ctx context.Context, resource api.SAPBTPResource, resource), ) } +func waitForResourceAnnotationRemove(ctx context.Context, resource api.SAPBTPResource, annotationsKey ...string) { + key := getResourceNamespacedName(resource) + Eventually(func() bool { + if err := k8sClient.Get(ctx, key, resource); err != nil { + return false + } + for _, annotationKey := range annotationsKey { + _, ok := resource.GetAnnotations()[annotationKey] + if ok { + return false + } + } + return true + }, timeout*2, interval).Should(BeTrue(), + eventuallyMsgForResource( + fmt.Sprintf("annotation %s was not removed", annotationsKey), + key, + resource), + ) +} func getResourceNamespacedName(resource client.Object) types.NamespacedName { return types.NamespacedName{Namespace: resource.GetNamespace(), Name: resource.GetName()} From 2753508d1f732d46c06e3c70eab2c058567daa86 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 11 Dec 2023 13:34:52 +0200 Subject: [PATCH 18/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- ...serviceinstance_validating_webhook_test.go | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/api/v1/serviceinstance_validating_webhook_test.go b/api/v1/serviceinstance_validating_webhook_test.go index 3323830d..f2270031 100644 --- a/api/v1/serviceinstance_validating_webhook_test.go +++ b/api/v1/serviceinstance_validating_webhook_test.go @@ -4,6 +4,7 @@ import ( "github.com/SAP/sap-btp-service-operator/api" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "time" ) var _ = Describe("Service Instance Webhook Test", func() { @@ -24,6 +25,25 @@ var _ = Describe("Service Instance Webhook Test", func() { Expect(err.Error()).To(ContainSubstring("changing the btpAccessCredentialsSecret for an existing instance is not allowed")) }) }) + When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with not a date", func() { + It("should not return error from webhook", func() { + instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: "true", + } + _, err := instance.ValidateUpdate(instance) + Expect(err).To(HaveOccurred()) + + }) + }) + When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with future date", func() { + It("should not return error from webhook", func() { + instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), + } + _, err := instance.ValidateUpdate(instance) + Expect(err).To(HaveOccurred()) + }) + }) }) Context("Validate Delete", func() { @@ -53,5 +73,26 @@ var _ = Describe("Service Instance Webhook Test", func() { Expect(err).ToNot(HaveOccurred()) }) }) + Context("Validate Create", func() { + When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with not a date", func() { + It("should not return error from webhook", func() { + instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: "true", + } + _, err := instance.ValidateCreate() + Expect(err).To(HaveOccurred()) + + }) + }) + When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with future date", func() { + It("should not return error from webhook", func() { + instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), + } + _, err := instance.ValidateCreate() + Expect(err).To(HaveOccurred()) + }) + }) + }) }) }) From 2a17b4aa9e4af8275b229e7c6680e245a3c9a42f Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 11 Dec 2023 13:57:16 +0200 Subject: [PATCH 19/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1alpha1/servicebinding_types.go | 4 +++- api/v1alpha1/serviceinstance_types.go | 24 ++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/servicebinding_types.go b/api/v1alpha1/servicebinding_types.go index 7a04b722..57131011 100644 --- a/api/v1alpha1/servicebinding_types.go +++ b/api/v1alpha1/servicebinding_types.go @@ -19,9 +19,11 @@ package v1alpha1 import ( "github.com/SAP/sap-btp-service-operator/api" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" + "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -180,7 +182,7 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) { sb.Status.Ready = ready } -func (sb *ServiceBinding) SupportIgnoreNonTransientErrorAnnotation() bool { +func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { return false } diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index a683cde7..775e6ea4 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/api/v1alpha1/serviceinstance_types.go @@ -17,11 +17,14 @@ limitations under the License. package v1alpha1 import ( + "fmt" "github.com/SAP/sap-btp-service-operator/api" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" + "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -170,8 +173,25 @@ func (in *ServiceInstance) SetReady(ready metav1.ConditionStatus) { in.Status.Ready = ready } -func (in *ServiceInstance) SupportIgnoreNonTransientErrorAnnotation() bool { - return true +func (si *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { + if si.Annotations != nil { + if _, ok := si.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") + annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) + if err != nil { + log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) + return false + } + sinceAnnotation := time.Since(annotationTime) + if sinceAnnotation > timeout { + log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) + return false + } + log.Info("timeout didn't reached- consider error to be transient") + return true + } + } + return false } // +kubebuilder:object:root=true From 6adea665dfb4dfa59e76295bc04e48ad023ab032 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 11 Dec 2023 14:24:43 +0200 Subject: [PATCH 20/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/common.go | 3 ++- api/v1/servicebinding_types.go | 5 +++-- api/v1/serviceinstance_types.go | 3 ++- api/v1/webhooks/serviceinstance_mutating_webhook.go | 3 ++- api/v1alpha1/servicebinding_types.go | 5 +++-- api/v1alpha1/serviceinstance_types.go | 11 ++++++----- controllers/base_controller.go | 3 ++- controllers/serviceinstance_controller.go | 3 ++- 8 files changed, 22 insertions(+), 14 deletions(-) diff --git a/api/common.go b/api/common.go index 9cfee480..dbcd10c3 100644 --- a/api/common.go +++ b/api/common.go @@ -2,9 +2,10 @@ package api import ( "fmt" - "github.com/go-logr/logr" "time" + "github.com/go-logr/logr" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index c02aca3d..50825676 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -17,13 +17,14 @@ limitations under the License. package v1 import ( + "time" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -198,7 +199,7 @@ func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) { sb.Annotations = annotations } -func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { +func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(_ logr.Logger, _ time.Duration) bool { return false } diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index 48c89ff2..5a246d69 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -18,13 +18,14 @@ package v1 import ( "fmt" + "time" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index bb962ab6..fbf9e886 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -3,11 +3,12 @@ package webhooks import ( "context" "encoding/json" - "github.com/SAP/sap-btp-service-operator/api" "net/http" "reflect" "time" + "github.com/SAP/sap-btp-service-operator/api" + v1admission "k8s.io/api/admission/v1" v1 "k8s.io/api/authentication/v1" diff --git a/api/v1alpha1/servicebinding_types.go b/api/v1alpha1/servicebinding_types.go index 57131011..8fc3b104 100644 --- a/api/v1alpha1/servicebinding_types.go +++ b/api/v1alpha1/servicebinding_types.go @@ -17,13 +17,14 @@ limitations under the License. package v1alpha1 import ( + "time" + "github.com/SAP/sap-btp-service-operator/api" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -182,7 +183,7 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) { sb.Status.Ready = ready } -func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { +func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(_ logr.Logger, _ time.Duration) bool { return false } diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index 775e6ea4..df973d2b 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/api/v1alpha1/serviceinstance_types.go @@ -18,13 +18,14 @@ package v1alpha1 import ( "fmt" + "time" + "github.com/SAP/sap-btp-service-operator/api" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "time" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! @@ -173,11 +174,11 @@ func (in *ServiceInstance) SetReady(ready metav1.ConditionStatus) { in.Status.Ready = ready } -func (si *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { - if si.Annotations != nil { - if _, ok := si.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { +func (in *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { + if in.Annotations != nil { + if _, ok := in.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) + annotationTime, err := time.Parse(time.RFC3339, in.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) if err != nil { log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) return false diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 89f7c9a4..42d71f38 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -3,6 +3,8 @@ package controllers import ( "context" "fmt" + "net/http" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -15,7 +17,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" - "net/http" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 5591a996..ba065f53 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -22,10 +22,11 @@ import ( "encoding/hex" "encoding/json" "fmt" - "github.com/go-logr/logr" "net/http" "time" + "github.com/go-logr/logr" + "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/controller" From cb317065475754417648d17a1ee4202e059ccdde Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 11 Dec 2023 15:16:17 +0200 Subject: [PATCH 21/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/serviceinstance_types.go | 47 ++++++++++--------- api/v1/serviceinstance_types_test.go | 2 + ...serviceinstance_validating_webhook_test.go | 4 ++ 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index 5a246d69..ba7a5d7a 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -217,40 +217,43 @@ func (si *ServiceInstance) ShouldBeShared() bool { } func (si *ServiceInstance) ValidateNonTransientTimestampAnnotation(log logr.Logger) error { - if si.Annotations != nil { - if _, ok := si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; ok { - log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) - if err != nil { - return fmt.Errorf("annotation %s is not a valid timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) - } - sinceAnnotation := time.Since(annotationTime) - if sinceAnnotation < 0 { - return fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) - } - } - } + sinceAnnotation, exist, err := si.GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log) + if err != nil { + return err + } + if exist && sinceAnnotation < 0 { + return fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) + } return nil } func (si *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { + + sinceAnnotation, exist, _ := si.GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log) + if !exist { + return false + } + if sinceAnnotation > timeout { + log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) + return false + } + log.Info("timeout didn't reached- consider error to be transient") + return true + +} + +func (si *ServiceInstance) GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger) (time.Duration, bool, error) { if si.Annotations != nil { if _, ok := si.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) if err != nil { log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) - return false - } - sinceAnnotation := time.Since(annotationTime) - if sinceAnnotation > timeout { - log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) - return false + return time.Since(time.Now()), false, fmt.Errorf("annotation %s is not a valid timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) } - log.Info("timeout didn't reached- consider error to be transient") - return true + return time.Since(annotationTime), true, nil } } - return false + return time.Since(time.Now()), false, nil } diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 4e8b5b4a..ab4b1f04 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -134,6 +134,7 @@ var _ = Describe("Service Instance Type Test", func() { It("validate timestamp annotation- not date", func() { annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: "true", } instance.SetAnnotations(annotation) @@ -144,6 +145,7 @@ var _ = Describe("Service Instance Type Test", func() { It("validate timestamp annotation- future date", func() { annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) diff --git a/api/v1/serviceinstance_validating_webhook_test.go b/api/v1/serviceinstance_validating_webhook_test.go index f2270031..cd1a5fba 100644 --- a/api/v1/serviceinstance_validating_webhook_test.go +++ b/api/v1/serviceinstance_validating_webhook_test.go @@ -28,6 +28,7 @@ var _ = Describe("Service Instance Webhook Test", func() { When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with not a date", func() { It("should not return error from webhook", func() { instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: "true", } _, err := instance.ValidateUpdate(instance) @@ -38,6 +39,7 @@ var _ = Describe("Service Instance Webhook Test", func() { When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with future date", func() { It("should not return error from webhook", func() { instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), } _, err := instance.ValidateUpdate(instance) @@ -77,6 +79,7 @@ var _ = Describe("Service Instance Webhook Test", func() { When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with not a date", func() { It("should not return error from webhook", func() { instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: "true", } _, err := instance.ValidateCreate() @@ -87,6 +90,7 @@ var _ = Describe("Service Instance Webhook Test", func() { When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with future date", func() { It("should not return error from webhook", func() { instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), } _, err := instance.ValidateCreate() From e5d86016aa961f2d6c98f119a303174c65aac8df Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 11 Dec 2023 15:43:00 +0200 Subject: [PATCH 22/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/serviceinstance_types_test.go | 10 +++++++ api/v1alpha1/serviceinstance_types_test.go | 33 ++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index ab4b1f04..79935d74 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -153,4 +153,14 @@ var _ = Describe("Service Instance Type Test", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("cannot be a future timestamp")) }) + It("validate annotation exist and valid", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + Expect(exist).To(BeTrue()) + }) }) diff --git a/api/v1alpha1/serviceinstance_types_test.go b/api/v1alpha1/serviceinstance_types_test.go index 41ac8e59..917bfed6 100644 --- a/api/v1alpha1/serviceinstance_types_test.go +++ b/api/v1alpha1/serviceinstance_types_test.go @@ -7,10 +7,14 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "time" ) var _ = Describe("Service Instance Type Test", func() { var instance *ServiceInstance + var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") + BeforeEach(func() { instance = getInstance() conditions := instance.GetConditions() @@ -121,4 +125,33 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetStatus(status) Expect(instance.GetStatus()).To(Equal(status)) }) + + It("should update annotation", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + } + instance.SetAnnotations(annotation) + Expect(instance.GetAnnotations()).To(Equal(annotation)) + }) + + It("validate timestamp annotation- not date", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: "true", + } + instance.SetAnnotations(annotation) + exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + Expect(exist).To(BeFalse()) + }) + It("validate annotation exist and valid", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + Expect(exist).To(BeTrue()) + }) }) From 140e4a6b0d189ff9204c9b1c46e43eb69d4ac763 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Wed, 13 Dec 2023 11:21:55 +0200 Subject: [PATCH 23/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/servicebinding_types_test.go | 10 ++++++++++ api/v1/serviceinstance_types_test.go | 29 ++++++++++++++++++++++++++++ internal/config/config.go | 2 +- 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/api/v1/servicebinding_types_test.go b/api/v1/servicebinding_types_test.go index 0f354a93..328d6424 100644 --- a/api/v1/servicebinding_types_test.go +++ b/api/v1/servicebinding_types_test.go @@ -7,6 +7,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "time" ) var _ = Describe("Service Binding Type Test", func() { @@ -105,5 +106,14 @@ var _ = Describe("Service Binding Type Test", func() { binding.SetAnnotations(annotation) Expect(binding.GetAnnotations()).To(Equal(annotation)) }) + It("validate IsIgnoreNonTransientAnnotationExistAndValid return false", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), + } + binding.SetAnnotations(annotation) + exist := binding.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + Expect(exist).To(BeFalse()) + }) }) diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 79935d74..b62154ec 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -163,4 +163,33 @@ var _ = Describe("Service Instance Type Test", func() { exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) Expect(exist).To(BeTrue()) }) + It("validate annotation exist and valid", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + err := instance.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + Expect(err).NotTo(HaveOccurred()) + }) + It("validate timeout for Ignore Non Transient Error Annotation", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + Expect(exist).To(BeFalse()) + }) + It("validate annotation not exist", func() { + + annotation := map[string]string{ + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + Expect(exist).To(BeFalse()) + }) }) diff --git a/internal/config/config.go b/internal/config/config.go index 3fb3dfc8..ca2f1ee7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,7 +24,7 @@ type Config struct { ClusterID string `envconfig:"cluster_id"` RetryBaseDelay time.Duration `envconfig:"retry_base_delay"` RetryMaxDelay time.Duration `envconfig:"retry_max_delay"` - IgnoreNonTransientTimeout time.Duration `envconfig:"ignore_non_transient_timeout"` + IgnoreNonTransientTimeout time.Duration } func Get() Config { From 4ab647e1e9227498792ec6cfca0e0c602d58c4d5 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:39:40 +0200 Subject: [PATCH 24/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/serviceinstance_types.go | 4 ++-- controllers/serviceinstance_controller.go | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index ba7a5d7a..fda8530a 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -235,10 +235,10 @@ func (si *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr. return false } if sinceAnnotation > timeout { - log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) + log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) return false } - log.Info("timeout didn't reached- consider error to be transient") + log.Info(fmt.Sprintf("timeout didn't reached- consider error to be transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) return true } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index ba065f53..881c502a 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -83,7 +83,10 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ if isFinalState(ctx, serviceInstance, r.Config.IgnoreNonTransientTimeout) { if len(serviceInstance.Status.HashedSpec) == 0 { updateHashedSpecValue(serviceInstance) - return ctrl.Result{}, r.Client.Status().Update(ctx, serviceInstance) + err := r.Client.Status().Update(ctx, serviceInstance) + if err != nil { + return ctrl.Result{}, err + } } return ctrl.Result{}, r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) From c7b6f66edf68674d17cf2c3a4262da8a2fc3f1a3 Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 18 Dec 2023 15:20:14 +0200 Subject: [PATCH 25/39] review --- controllers/base_controller.go | 4 +++- controllers/serviceinstance_controller.go | 24 +++++++++++++------ .../serviceinstance_controller_test.go | 7 +++--- controllers/suite_test.go | 2 +- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index eed15035..34339bcc 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "errors" "fmt" "net/http" @@ -344,7 +345,8 @@ func isTransientStatusCode(StatusCode int) bool { func (r *BaseReconciler) handleError(ctx context.Context, operationType smClientTypes.OperationCategory, err error, resource api.SAPBTPResource) (ctrl.Result, error) { log := GetLogger(ctx) - smError, ok := err.(*sm.ServiceManagerError) + var smError *sm.ServiceManagerError + ok := errors.As(err, &smError) if !ok { log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 4b551484..fcd3bf28 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -80,6 +80,21 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } + if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && + serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, r.Config.IgnoreNonTransientTimeout) { + operation := smClientTypes.CREATE + if len(serviceInstance.Status.InstanceID) > 0 { + operation = smClientTypes.UPDATE + } + setInProgressConditions(ctx, operation, "", serviceInstance) + conditions := serviceInstance.GetConditions() + if len(conditions) > 0 { + meta.RemoveStatusCondition(&conditions, api.ConditionFailed) + } + serviceInstance.SetConditions(conditions) + r.Status().Update(ctx, serviceInstance) + } + if isFinalState(ctx, serviceInstance, r.Config.IgnoreNonTransientTimeout) { if len(serviceInstance.Status.HashedSpec) == 0 { updateHashedSpecValue(serviceInstance) @@ -558,10 +573,7 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan log.Info("instance is not in final state, sync operation is in progress") return false } - if isInRetry(serviceInstance, log, nonTransientTimeout) { - log.Info("instance is not in final state, IgnoreNonTransientErrorAnnotation exist") - return false - } + if sharingUpdateRequired(serviceInstance) { log.Info("instance is not in final state, need to sync sharing status") if len(serviceInstance.Status.HashedSpec) == 0 { @@ -593,9 +605,7 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger if cond != nil && cond.Reason == UpdateInProgress { //in case of transient error occurred return true } - if serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, nonTransientTimeout) { - return true - } + return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec } diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index de5d88d1..7c35b169 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -251,18 +251,18 @@ var _ = Describe("ServiceInstance controller", func() { Context("first with 400 status then success", func() { errMessage := "failed to provision instance" BeforeEach(func() { - fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ + fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, Description: errMessage, }) - fakeClient.ProvisionReturnsOnCall(2, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) It("should have failure condition", func() { serviceInstance = createInstance(ctx, instanceSpec, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) }) - It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { + It("ignoreNonTransientErrorAnnotation should remove the annotation once succeeded", func() { serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) @@ -280,6 +280,7 @@ var _ = Describe("ServiceInstance controller", func() { serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) + waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) sinceCreate := time.Since(serviceInstance.GetCreationTimestamp().Time) Expect(sinceCreate > ignoreNonTransientTimeout) }) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 29e48924..3cfdddcc 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -67,7 +67,7 @@ const ( interval = time.Millisecond * 50 syncPeriod = time.Millisecond * 250 pollInterval = time.Millisecond * 250 - ignoreNonTransientTimeout = time.Second * 1 + ignoreNonTransientTimeout = time.Second * 10 fakeBindingID = "fake-binding-id" bindingTestNamespace = "test-namespace" From e8c737ba9805dd9cec07a047fd59de76d52a7318 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Mon, 18 Dec 2023 16:08:09 +0200 Subject: [PATCH 26/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/common.go | 5 -- api/util.go | 50 +++++++++++++++++++ api/v1/servicebinding_types.go | 7 --- api/v1/servicebinding_types_test.go | 11 ---- api/v1/serviceinstance_types.go | 46 ----------------- api/v1/serviceinstance_types_test.go | 12 ++--- api/v1/serviceinstance_validating_webhook.go | 4 +- .../serviceinstance_mutating_webhook.go | 20 ++++---- api/v1alpha1/servicebinding_types.go | 7 --- api/v1alpha1/serviceinstance_types.go | 25 ---------- .../samples/services_v1_serviceinstance.yaml | 8 +-- controllers/base_controller.go | 9 ++-- controllers/serviceinstance_controller.go | 10 +--- 13 files changed, 79 insertions(+), 135 deletions(-) create mode 100644 api/util.go diff --git a/api/common.go b/api/common.go index dbcd10c3..0280175a 100644 --- a/api/common.go +++ b/api/common.go @@ -2,10 +2,6 @@ package api import ( "fmt" - "time" - - "github.com/go-logr/logr" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -86,5 +82,4 @@ type SAPBTPResource interface { GetReady() metav1.ConditionStatus GetAnnotations() map[string]string SetAnnotations(map[string]string) - IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool } diff --git a/api/util.go b/api/util.go new file mode 100644 index 00000000..e6c50ece --- /dev/null +++ b/api/util.go @@ -0,0 +1,50 @@ +package api + +import ( + "fmt" + "github.com/go-logr/logr" + "time" +) + +func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource SAPBTPResource) error { + + sinceAnnotation, exist, err := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) + if err != nil { + return err + } + if exist && sinceAnnotation < 0 { + return fmt.Errorf("annotation %s cannot be a future timestamp", IgnoreNonTransientErrorTimestampAnnotation) + } + return nil +} + +func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource SAPBTPResource, timeout time.Duration) bool { + + sinceAnnotation, exist, _ := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) + if !exist { + return false + } + if sinceAnnotation > timeout { + log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) + return false + } + log.Info(fmt.Sprintf("timeout didn't reached- consider error to be transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) + return true + +} + +func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource SAPBTPResource) (time.Duration, bool, error) { + annotation := resource.GetAnnotations() + if annotation != nil { + if _, ok := annotation[IgnoreNonTransientErrorAnnotation]; ok { + log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") + annotationTime, err := time.Parse(time.RFC3339, annotation[IgnoreNonTransientErrorTimestampAnnotation]) + if err != nil { + log.Error(err, fmt.Sprintf("failed to parse %s", IgnoreNonTransientErrorTimestampAnnotation)) + return time.Since(time.Now()), false, fmt.Errorf("annotation %s is not a valid timestamp", IgnoreNonTransientErrorTimestampAnnotation) + } + return time.Since(annotationTime), true, nil + } + } + return time.Since(time.Now()), false, nil +} diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index 50825676..22466678 100644 --- a/api/v1/servicebinding_types.go +++ b/api/v1/servicebinding_types.go @@ -17,11 +17,8 @@ limitations under the License. package v1 import ( - "time" - "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" - "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -199,10 +196,6 @@ func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) { sb.Annotations = annotations } -func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(_ logr.Logger, _ time.Duration) bool { - return false -} - // +kubebuilder:object:root=true // ServiceBindingList contains a list of ServiceBinding diff --git a/api/v1/servicebinding_types_test.go b/api/v1/servicebinding_types_test.go index 328d6424..14ce9c5a 100644 --- a/api/v1/servicebinding_types_test.go +++ b/api/v1/servicebinding_types_test.go @@ -7,7 +7,6 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "time" ) var _ = Describe("Service Binding Type Test", func() { @@ -106,14 +105,4 @@ var _ = Describe("Service Binding Type Test", func() { binding.SetAnnotations(annotation) Expect(binding.GetAnnotations()).To(Equal(annotation)) }) - It("validate IsIgnoreNonTransientAnnotationExistAndValid return false", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), - } - binding.SetAnnotations(annotation) - exist := binding.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) - Expect(exist).To(BeFalse()) - }) }) diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index fda8530a..f56b6e4a 100644 --- a/api/v1/serviceinstance_types.go +++ b/api/v1/serviceinstance_types.go @@ -17,12 +17,8 @@ limitations under the License. package v1 import ( - "fmt" - "time" - "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm/types" - "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -215,45 +211,3 @@ func (si *ServiceInstance) Hub() {} func (si *ServiceInstance) ShouldBeShared() bool { return si.Spec.Shared != nil && *si.Spec.Shared } - -func (si *ServiceInstance) ValidateNonTransientTimestampAnnotation(log logr.Logger) error { - - sinceAnnotation, exist, err := si.GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log) - if err != nil { - return err - } - if exist && sinceAnnotation < 0 { - return fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) - } - return nil -} - -func (si *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { - - sinceAnnotation, exist, _ := si.GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log) - if !exist { - return false - } - if sinceAnnotation > timeout { - log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) - return false - } - log.Info(fmt.Sprintf("timeout didn't reached- consider error to be transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) - return true - -} - -func (si *ServiceInstance) GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger) (time.Duration, bool, error) { - if si.Annotations != nil { - if _, ok := si.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) - if err != nil { - log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) - return time.Since(time.Now()), false, fmt.Errorf("annotation %s is not a valid timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) - } - return time.Since(annotationTime), true, nil - } - } - return time.Since(time.Now()), false, nil -} diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index b62154ec..f3fc600e 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -138,7 +138,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: "true", } instance.SetAnnotations(annotation) - err := instance.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("is not a valid timestamp")) }) @@ -149,7 +149,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) - err := instance.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("cannot be a future timestamp")) }) @@ -160,7 +160,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeTrue()) }) It("validate annotation exist and valid", func() { @@ -170,7 +170,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - err := instance.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).NotTo(HaveOccurred()) }) It("validate timeout for Ignore Non Transient Error Annotation", func() { @@ -180,7 +180,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeFalse()) }) It("validate annotation not exist", func() { @@ -189,7 +189,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) + exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeFalse()) }) }) diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index 8147ddbd..5a824fa9 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -43,7 +43,7 @@ var _ webhook.Validator = &ServiceInstance{} var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) { - return nil, si.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + return nil, api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) } func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { @@ -53,7 +53,7 @@ func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admissio if oldInstance.Spec.BTPAccessCredentialsSecret != si.Spec.BTPAccessCredentialsSecret { return nil, fmt.Errorf("changing the btpAccessCredentialsSecret for an existing instance is not allowed") } - return nil, si.ValidateNonTransientTimestampAnnotation(serviceinstancelog) + return nil, api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) } func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) { diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index fbf9e886..da64aaf5 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -43,7 +43,7 @@ func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Reque if err != nil { return admission.Errored(http.StatusInternalServerError, err) } - + s.setIgnoreNonTransientErrorTimestampAnnotation(instance) marshaledInstance, err := json.Marshal(instance) if err != nil { return admission.Errored(http.StatusInternalServerError, err) @@ -59,14 +59,6 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ Extra: req.UserInfo.Extra, } - if instance.Annotations != nil { - if _, ok := instance.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - if _, exist := instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; !exist { - instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation] = time.Now().Format(time.RFC3339) - } - } - } - if req.Operation == v1admission.Create || req.Operation == v1admission.Delete { instance.Spec.UserInfo = userInfo } else if req.Operation == v1admission.Update { @@ -81,3 +73,13 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ } return nil } + +func (s *ServiceInstanceDefaulter) setIgnoreNonTransientErrorTimestampAnnotation(instance *servicesv1.ServiceInstance) { + if instance.Annotations != nil { + if _, ok := instance.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + if _, exist := instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; !exist { + instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation] = time.Now().Format(time.RFC3339) + } + } + } +} diff --git a/api/v1alpha1/servicebinding_types.go b/api/v1alpha1/servicebinding_types.go index 8fc3b104..131e95f6 100644 --- a/api/v1alpha1/servicebinding_types.go +++ b/api/v1alpha1/servicebinding_types.go @@ -17,11 +17,8 @@ limitations under the License. package v1alpha1 import ( - "time" - "github.com/SAP/sap-btp-service-operator/api" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" - "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -183,10 +180,6 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) { sb.Status.Ready = ready } -func (sb *ServiceBinding) IsIgnoreNonTransientAnnotationExistAndValid(_ logr.Logger, _ time.Duration) bool { - return false -} - // +kubebuilder:object:root=true // ServiceBindingList contains a list of ServiceBinding diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index df973d2b..04879bb2 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/api/v1alpha1/serviceinstance_types.go @@ -17,12 +17,8 @@ limitations under the License. package v1alpha1 import ( - "fmt" - "time" - "github.com/SAP/sap-btp-service-operator/api" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" - "github.com/go-logr/logr" v1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -174,27 +170,6 @@ func (in *ServiceInstance) SetReady(ready metav1.ConditionStatus) { in.Status.Ready = ready } -func (in *ServiceInstance) IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, timeout time.Duration) bool { - if in.Annotations != nil { - if _, ok := in.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, in.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) - if err != nil { - log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) - return false - } - sinceAnnotation := time.Since(annotationTime) - if sinceAnnotation > timeout { - log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. sinceCreate %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) - return false - } - log.Info("timeout didn't reached- consider error to be transient") - return true - } - } - return false -} - // +kubebuilder:object:root=true // ServiceInstanceList contains a list of ServiceInstance diff --git a/config/samples/services_v1_serviceinstance.yaml b/config/samples/services_v1_serviceinstance.yaml index bd19f598..55043953 100644 --- a/config/samples/services_v1_serviceinstance.yaml +++ b/config/samples/services_v1_serviceinstance.yaml @@ -1,7 +1,9 @@ apiVersion: services.cloud.sap.com/v1 kind: ServiceInstance metadata: - name: sample-instance-1 + name: sample-instance-5 + annotations: + services.cloud.sap.com/ignoreNonTransientError: "true" spec: - serviceOfferingName: service-manager - servicePlanName: subaccount-audit + serviceOfferingName: led-zeppelin + servicePlanName: x-small diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 34339bcc..1c1cf180 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -323,10 +323,7 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { statusCode := smError.GetStatusCode() log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) - if isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) { - return true - } - return resource.IsIgnoreNonTransientAnnotationExistAndValid(log, r.Config.IgnoreNonTransientTimeout) + return isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) } func isConcurrentOperationError(smError *sm.ServiceManagerError) bool { @@ -351,9 +348,11 @@ func (r *BaseReconciler) handleError(ctx context.Context, operationType smClient log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) } - if isTransient := r.isTransientError(smError, log, resource); isTransient { + if r.isTransientError(smError, log, resource) || + api.IsIgnoreNonTransientAnnotationExistAndValid(log, resource, r.Config.IgnoreNonTransientTimeout) { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } + return r.markAsNonTransientError(ctx, operationType, smError.Error(), resource) } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index fcd3bf28..6cb26b69 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 } if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && - serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, r.Config.IgnoreNonTransientTimeout) { + api.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { operation := smClientTypes.CREATE if len(serviceInstance.Status.InstanceID) > 0 { operation = smClientTypes.UPDATE @@ -586,14 +586,6 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan return true } -func isInRetry(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { - conditions := serviceInstance.GetConditions() - if meta.IsStatusConditionPresentAndEqual(conditions, api.ConditionSucceeded, metav1.ConditionFalse) { - return serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, nonTransientTimeout) - } - return false -} - // TODO unit test func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { //update is not supported for failed instances (this can occur when instance creation was asynchronously) From 4e3dfad1ac64aaa3788bdce840103f723ed92cee Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 18 Dec 2023 17:41:47 +0200 Subject: [PATCH 27/39] review --- api/{util.go => utils/utils.go} | 19 +++++++------- api/v1/serviceinstance_types_test.go | 16 ++++++------ api/v1/serviceinstance_validating_webhook.go | 6 +++-- api/v1alpha1/serviceinstance_types_test.go | 24 ------------------ controllers/base_controller.go | 8 +++--- controllers/serviceinstance_controller.go | 25 +++++++------------ .../serviceinstance_controller_test.go | 16 ++++++------ 7 files changed, 45 insertions(+), 69 deletions(-) rename api/{util.go => utils/utils.go} (68%) diff --git a/api/util.go b/api/utils/utils.go similarity index 68% rename from api/util.go rename to api/utils/utils.go index e6c50ece..3a0e6e8d 100644 --- a/api/util.go +++ b/api/utils/utils.go @@ -1,24 +1,25 @@ -package api +package utils import ( "fmt" + "github.com/SAP/sap-btp-service-operator/api" "github.com/go-logr/logr" "time" ) -func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource SAPBTPResource) error { +func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource api.SAPBTPResource) error { sinceAnnotation, exist, err := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) if err != nil { return err } if exist && sinceAnnotation < 0 { - return fmt.Errorf("annotation %s cannot be a future timestamp", IgnoreNonTransientErrorTimestampAnnotation) + return fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) } return nil } -func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource SAPBTPResource, timeout time.Duration) bool { +func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource api.SAPBTPResource, timeout time.Duration) bool { sinceAnnotation, exist, _ := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) if !exist { @@ -33,15 +34,15 @@ func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource SAPBT } -func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource SAPBTPResource) (time.Duration, bool, error) { +func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource api.SAPBTPResource) (time.Duration, bool, error) { annotation := resource.GetAnnotations() if annotation != nil { - if _, ok := annotation[IgnoreNonTransientErrorAnnotation]; ok { + if _, ok := annotation[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, annotation[IgnoreNonTransientErrorTimestampAnnotation]) + annotationTime, err := time.Parse(time.RFC3339, annotation[api.IgnoreNonTransientErrorTimestampAnnotation]) if err != nil { - log.Error(err, fmt.Sprintf("failed to parse %s", IgnoreNonTransientErrorTimestampAnnotation)) - return time.Since(time.Now()), false, fmt.Errorf("annotation %s is not a valid timestamp", IgnoreNonTransientErrorTimestampAnnotation) + log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) + return time.Since(time.Now()), false, fmt.Errorf("annotation %s is not a valid timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) } return time.Since(annotationTime), true, nil } diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index f3fc600e..06cf350b 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -1,13 +1,15 @@ package v1 import ( + "time" + "github.com/SAP/sap-btp-service-operator/api" + "github.com/SAP/sap-btp-service-operator/api/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "time" ) var _ = Describe("Service Instance Type Test", func() { @@ -138,7 +140,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: "true", } instance.SetAnnotations(annotation) - err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) + err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("is not a valid timestamp")) }) @@ -149,7 +151,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) - err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) + err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("cannot be a future timestamp")) }) @@ -160,7 +162,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeTrue()) }) It("validate annotation exist and valid", func() { @@ -170,7 +172,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) + err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).NotTo(HaveOccurred()) }) It("validate timeout for Ignore Non Transient Error Annotation", func() { @@ -180,7 +182,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeFalse()) }) It("validate annotation not exist", func() { @@ -189,7 +191,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeFalse()) }) }) diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index 5a824fa9..6294ebc3 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -20,6 +20,8 @@ import ( "fmt" "strings" + "github.com/SAP/sap-btp-service-operator/api/utils" + "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -43,7 +45,7 @@ var _ webhook.Validator = &ServiceInstance{} var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) { - return nil, api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) + return nil, utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) } func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { @@ -53,7 +55,7 @@ func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admissio if oldInstance.Spec.BTPAccessCredentialsSecret != si.Spec.BTPAccessCredentialsSecret { return nil, fmt.Errorf("changing the btpAccessCredentialsSecret for an existing instance is not allowed") } - return nil, api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) + return nil, utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) } func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) { diff --git a/api/v1alpha1/serviceinstance_types_test.go b/api/v1alpha1/serviceinstance_types_test.go index 917bfed6..536fa1d6 100644 --- a/api/v1alpha1/serviceinstance_types_test.go +++ b/api/v1alpha1/serviceinstance_types_test.go @@ -7,13 +7,10 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "time" ) var _ = Describe("Service Instance Type Test", func() { var instance *ServiceInstance - var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") BeforeEach(func() { instance = getInstance() @@ -133,25 +130,4 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetAnnotations(annotation) Expect(instance.GetAnnotations()).To(Equal(annotation)) }) - - It("validate timestamp annotation- not date", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: "true", - } - instance.SetAnnotations(annotation) - exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) - Expect(exist).To(BeFalse()) - }) - It("validate annotation exist and valid", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), - } - instance.SetAnnotations(annotation) - exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) - Expect(exist).To(BeTrue()) - }) }) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 1c1cf180..bad76e45 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -6,6 +6,8 @@ import ( "fmt" "net/http" + "github.com/SAP/sap-btp-service-operator/api/utils" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -320,7 +322,7 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { +func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log logr.Logger) bool { statusCode := smError.GetStatusCode() log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) return isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) @@ -348,8 +350,8 @@ func (r *BaseReconciler) handleError(ctx context.Context, operationType smClient log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) } - if r.isTransientError(smError, log, resource) || - api.IsIgnoreNonTransientAnnotationExistAndValid(log, resource, r.Config.IgnoreNonTransientTimeout) { + if r.isTransientError(smError, log) || + utils.IsIgnoreNonTransientAnnotationExistAndValid(log, resource, r.Config.IgnoreNonTransientTimeout) { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 6cb26b69..3d8a292c 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -22,10 +22,8 @@ import ( "encoding/hex" "encoding/json" "fmt" + "github.com/SAP/sap-btp-service-operator/api/utils" "net/http" - "time" - - "github.com/go-logr/logr" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -81,21 +79,18 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && - api.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { + utils.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { operation := smClientTypes.CREATE if len(serviceInstance.Status.InstanceID) > 0 { operation = smClientTypes.UPDATE } setInProgressConditions(ctx, operation, "", serviceInstance) - conditions := serviceInstance.GetConditions() - if len(conditions) > 0 { - meta.RemoveStatusCondition(&conditions, api.ConditionFailed) + if err := r.Status().Update(ctx, serviceInstance); err != nil { + return ctrl.Result{}, err } - serviceInstance.SetConditions(conditions) - r.Status().Update(ctx, serviceInstance) } - if isFinalState(ctx, serviceInstance, r.Config.IgnoreNonTransientTimeout) { + if isFinalState(ctx, serviceInstance) { if len(serviceInstance.Status.HashedSpec) == 0 { updateHashedSpecValue(serviceInstance) err := r.Client.Status().Update(ctx, serviceInstance) @@ -151,7 +146,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } // Update - if updateRequired(serviceInstance, log, r.Config.IgnoreNonTransientTimeout) { + if updateRequired(serviceInstance) { if res, err := r.updateInstance(ctx, smClient, serviceInstance); err != nil { log.Info("got error while trying to update instance") return ctrl.Result{}, err @@ -534,7 +529,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error with status code %d", smError.StatusCode)) - isTransient = r.isTransientError(smError, log, object) + isTransient = r.isTransientError(smError, log) errMsg = smError.Error() if smError.StatusCode == http.StatusTooManyRequests { @@ -553,7 +548,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte return ctrl.Result{Requeue: isTransient}, r.updateStatus(ctx, object) } -func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance, nonTransientTimeout time.Duration) bool { +func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) bool { log := GetLogger(ctx) if isMarkedForDeletion(serviceInstance.ObjectMeta) { log.Info("instance is not in final state, it is marked for deletion") @@ -586,8 +581,7 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan return true } -// TODO unit test -func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { +func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { //update is not supported for failed instances (this can occur when instance creation was asynchronously) if serviceInstance.Status.Ready != metav1.ConditionTrue { return false @@ -601,7 +595,6 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec } -// TODO unit test func sharingUpdateRequired(serviceInstance *servicesv1.ServiceInstance) bool { //relevant only for non-shared instances - sharing instance is possible only for usable instances if serviceInstance.Status.Ready != metav1.ConditionTrue { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 7c35b169..d4cbc943 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -348,7 +348,7 @@ var _ = Describe("ServiceInstance controller", func() { When("polling ends with success", func() { BeforeEach(func() { - fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{Labels: map[string][]string{"subaccount_id": []string{fakeSubaccountID}}}, nil) + fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{Labels: map[string][]string{"subaccount_id": {fakeSubaccountID}}}, nil) }) It("should update in progress condition and provision the instance successfully", func() { serviceInstance = createInstance(ctx, instanceSpec, false) @@ -1118,7 +1118,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) When("Succeeded is false", func() { @@ -1144,7 +1144,7 @@ var _ = Describe("ServiceInstance controller", func() { }, } instance.SetGeneration(1) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) }) @@ -1168,7 +1168,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1194,7 +1194,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1220,7 +1220,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1252,7 +1252,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1284,7 +1284,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeTrue()) + Expect(isFinalState(ctx, instance)).To(BeTrue()) }) }) }) From bd3eb96b31ece68880b743a8af18e958969e08d6 Mon Sep 17 00:00:00 2001 From: I501080 Date: Tue, 19 Dec 2023 08:45:28 +0200 Subject: [PATCH 28/39] go import --- api/common.go | 1 + api/utils/utils.go | 3 ++- controllers/serviceinstance_controller.go | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/common.go b/api/common.go index 0280175a..325e8491 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" diff --git a/api/utils/utils.go b/api/utils/utils.go index 3a0e6e8d..7e9265c0 100644 --- a/api/utils/utils.go +++ b/api/utils/utils.go @@ -2,9 +2,10 @@ package utils import ( "fmt" + "time" + "github.com/SAP/sap-btp-service-operator/api" "github.com/go-logr/logr" - "time" ) func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource api.SAPBTPResource) error { diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 3d8a292c..66ae2d50 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -22,9 +22,10 @@ import ( "encoding/hex" "encoding/json" "fmt" - "github.com/SAP/sap-btp-service-operator/api/utils" "net/http" + "github.com/SAP/sap-btp-service-operator/api/utils" + "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/controller" From 6708dc9d4746d700011599fd242c9926ea29c9f8 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 10:13:18 +0200 Subject: [PATCH 29/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/servicebinding_validating_webhook.go | 15 +++++++++++++++ .../servicebinding_validating_webhook_test.go | 17 +++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/api/v1/servicebinding_validating_webhook.go b/api/v1/servicebinding_validating_webhook.go index 04df9b93..af2e69c6 100644 --- a/api/v1/servicebinding_validating_webhook.go +++ b/api/v1/servicebinding_validating_webhook.go @@ -49,6 +49,9 @@ var _ webhook.Validator = &ServiceBinding{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (sb *ServiceBinding) ValidateCreate() (admission.Warnings, error) { servicebindinglog.Info("validate create", "name", sb.Name) + if err := sb.validateAnnotations(); err != nil { + return nil, err + } if sb.Spec.CredRotationPolicy != nil { if err := sb.validateCredRotatingConfig(); err != nil { return nil, err @@ -60,6 +63,9 @@ func (sb *ServiceBinding) ValidateCreate() (admission.Warnings, error) { // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type func (sb *ServiceBinding) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { servicebindinglog.Info("validate update", "name", sb.Name) + if err := sb.validateAnnotations(); err != nil { + return nil, err + } if sb.Spec.CredRotationPolicy != nil { if err := sb.validateCredRotatingConfig(); err != nil { return nil, err @@ -124,3 +130,12 @@ func (sb *ServiceBinding) validateCredRotatingConfig() error { return nil } + +func (sb *ServiceBinding) validateAnnotations() error { + if sb.Annotations != nil { + if _, ok := sb.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { + return fmt.Errorf("annotation %s is not suppoted in service binding", api.IgnoreNonTransientErrorAnnotation) + } + } + return nil +} diff --git a/api/v1/servicebinding_validating_webhook_test.go b/api/v1/servicebinding_validating_webhook_test.go index b9ae8aab..adb3d3ca 100644 --- a/api/v1/servicebinding_validating_webhook_test.go +++ b/api/v1/servicebinding_validating_webhook_test.go @@ -19,6 +19,13 @@ var _ = Describe("Service Binding Webhook Test", func() { _, err := binding.ValidateCreate() Expect(err).ToNot(HaveOccurred()) }) + It("should failed", func() { + binding.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "false", + } + _, err := binding.ValidateCreate() + Expect(err).To(HaveOccurred()) + }) }) Context("Validate update of spec before binding is created (failure recovery)", func() { @@ -109,6 +116,16 @@ var _ = Describe("Service Binding Webhook Test", func() { Expect(err).ToNot(HaveOccurred()) }) }) + When("Annotation changed", func() { + It("should fail", func() { + newBinding.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "false", + } + _, err := newBinding.ValidateUpdate(binding) + Expect(err).To(HaveOccurred()) + }) + }) + }) Context("Validate update of spec after binding is created", func() { From 2c813f00242b97cef2650c69e45bca51f259bdba Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 10:26:24 +0200 Subject: [PATCH 30/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/v1/serviceinstance_validating_webhook_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/v1/serviceinstance_validating_webhook_test.go b/api/v1/serviceinstance_validating_webhook_test.go index cd1a5fba..6e020f93 100644 --- a/api/v1/serviceinstance_validating_webhook_test.go +++ b/api/v1/serviceinstance_validating_webhook_test.go @@ -26,7 +26,7 @@ var _ = Describe("Service Instance Webhook Test", func() { }) }) When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with not a date", func() { - It("should not return error from webhook", func() { + It("should return error from webhook", func() { instance.Annotations = map[string]string{ api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: "true", @@ -37,7 +37,7 @@ var _ = Describe("Service Instance Webhook Test", func() { }) }) When("service instance IgnoreNonTransientErrorTimestampAnnotation annotation is not set with future date", func() { - It("should not return error from webhook", func() { + It("should return error from webhook", func() { instance.Annotations = map[string]string{ api.IgnoreNonTransientErrorAnnotation: "true", api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), From 6f3fc965825520ca427f11c15ec5908ab1906556 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 11:22:17 +0200 Subject: [PATCH 31/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/serviceinstance_controller.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 66ae2d50..1bf0b980 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -81,11 +81,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && utils.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { - operation := smClientTypes.CREATE - if len(serviceInstance.Status.InstanceID) > 0 { - operation = smClientTypes.UPDATE - } - setInProgressConditions(ctx, operation, "", serviceInstance) + changeLastConditionToInProgress(serviceInstance) if err := r.Status().Update(ctx, serviceInstance); err != nil { return ctrl.Result{}, err } @@ -724,3 +720,18 @@ func getErrorMsgFromLastOperation(status *smClientTypes.Operation) string { } return errMsg } + +func changeLastConditionToInProgress(serviceInstance *servicesv1.ServiceInstance) { + conditions := serviceInstance.GetConditions() + lastOpCondition := meta.FindStatusCondition(conditions, api.ConditionSucceeded) + operation := smClientTypes.CREATE + if len(serviceInstance.Status.InstanceID) > 0 { + operation = smClientTypes.UPDATE + } + lastOpCondition.Reason = getConditionReason(operation, smClientTypes.INPROGRESS) + + if len(conditions) > 0 { + meta.RemoveStatusCondition(&conditions, api.ConditionFailed) + } + meta.SetStatusCondition(&conditions, *lastOpCondition) +} From 2a907810aebf408148a54db1449cb81d30e9332d Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 12:00:12 +0200 Subject: [PATCH 32/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/serviceinstance_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 1bf0b980..47e229f2 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -734,4 +734,5 @@ func changeLastConditionToInProgress(serviceInstance *servicesv1.ServiceInstance meta.RemoveStatusCondition(&conditions, api.ConditionFailed) } meta.SetStatusCondition(&conditions, *lastOpCondition) + serviceInstance.SetConditions(conditions) } From d54080b707841e30c908dbe1074d62a12bcb4646 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 12:20:11 +0200 Subject: [PATCH 33/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- config/samples/services_v1_serviceinstance.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/samples/services_v1_serviceinstance.yaml b/config/samples/services_v1_serviceinstance.yaml index 55043953..867fec80 100644 --- a/config/samples/services_v1_serviceinstance.yaml +++ b/config/samples/services_v1_serviceinstance.yaml @@ -1,9 +1,9 @@ apiVersion: services.cloud.sap.com/v1 kind: ServiceInstance metadata: - name: sample-instance-5 + name: sample-instance-1 annotations: services.cloud.sap.com/ignoreNonTransientError: "true" spec: - serviceOfferingName: led-zeppelin - servicePlanName: x-small + serviceOfferingName: service-manager + servicePlanName: subaccount-audit From 25555452d9da4fbb221455d8a0ae6d285502e1ea Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 13:35:44 +0200 Subject: [PATCH 34/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/common.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/common.go b/api/common.go index 325e8491..76256401 100644 --- a/api/common.go +++ b/api/common.go @@ -35,8 +35,8 @@ type HTTPStatusCodeError struct { } func (e HTTPStatusCodeError) Error() string { - errorMessage := "" - description := "" + errorMessage := "" + description := "" if e.ErrorMessage != nil { errorMessage = *e.ErrorMessage @@ -44,7 +44,7 @@ func (e HTTPStatusCodeError) Error() string { if e.Description != nil { description = *e.Description } - return fmt.Sprintf("Status: %v; ErrorMessage: %v; Description: %v; ResponseError: %v", e.StatusCode, errorMessage, description, e.ResponseError) + return fmt.Sprintf("BrokerError:%s, Status: %d, Description: %s", errorMessage, e.StatusCode, description) } const ( From 911ac917d091cddc62a6a0a35d046b08edf21737 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Tue, 19 Dec 2023 14:57:23 +0200 Subject: [PATCH 35/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- api/utils/utils.go | 7 +++++-- api/v1/servicebinding_validating_webhook.go | 3 ++- api/v1/servicebinding_validating_webhook_test.go | 5 +++++ api/v1/serviceinstance_types_test.go | 3 ++- api/v1/serviceinstance_validating_webhook_test.go | 5 +++++ 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/api/utils/utils.go b/api/utils/utils.go index 7e9265c0..b67ff1e8 100644 --- a/api/utils/utils.go +++ b/api/utils/utils.go @@ -8,6 +8,9 @@ import ( "github.com/go-logr/logr" ) +var AnnotationInFutureError = "Annotation %s cannot be a date in the future." +var AnnotationNotValidTimestampError = "Annotation %s is not a valid timestamp" + func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource api.SAPBTPResource) error { sinceAnnotation, exist, err := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) @@ -15,7 +18,7 @@ func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource api.SAPBT return err } if exist && sinceAnnotation < 0 { - return fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) + return fmt.Errorf(AnnotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation) } return nil } @@ -43,7 +46,7 @@ func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource annotationTime, err := time.Parse(time.RFC3339, annotation[api.IgnoreNonTransientErrorTimestampAnnotation]) if err != nil { log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) - return time.Since(time.Now()), false, fmt.Errorf("annotation %s is not a valid timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) + return time.Since(time.Now()), false, fmt.Errorf(AnnotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation) } return time.Since(annotationTime), true, nil } diff --git a/api/v1/servicebinding_validating_webhook.go b/api/v1/servicebinding_validating_webhook.go index af2e69c6..5c488652 100644 --- a/api/v1/servicebinding_validating_webhook.go +++ b/api/v1/servicebinding_validating_webhook.go @@ -32,6 +32,7 @@ import ( // log is for logging in this package. var servicebindinglog = logf.Log.WithName("servicebinding-resource") +var AnnotationNotSupportedError = "The specified annotation '%s' is not supported within the service binding object." func (sb *ServiceBinding) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). @@ -134,7 +135,7 @@ func (sb *ServiceBinding) validateCredRotatingConfig() error { func (sb *ServiceBinding) validateAnnotations() error { if sb.Annotations != nil { if _, ok := sb.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - return fmt.Errorf("annotation %s is not suppoted in service binding", api.IgnoreNonTransientErrorAnnotation) + return fmt.Errorf(AnnotationNotSupportedError, api.IgnoreNonTransientErrorAnnotation) } } return nil diff --git a/api/v1/servicebinding_validating_webhook_test.go b/api/v1/servicebinding_validating_webhook_test.go index adb3d3ca..f2f631fb 100644 --- a/api/v1/servicebinding_validating_webhook_test.go +++ b/api/v1/servicebinding_validating_webhook_test.go @@ -1,6 +1,7 @@ package v1 import ( + "fmt" "github.com/SAP/sap-btp-service-operator/api" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -25,6 +26,8 @@ var _ = Describe("Service Binding Webhook Test", func() { } _, err := binding.ValidateCreate() Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(AnnotationNotSupportedError, api.IgnoreNonTransientErrorAnnotation))) + }) }) @@ -123,6 +126,8 @@ var _ = Describe("Service Binding Webhook Test", func() { } _, err := newBinding.ValidateUpdate(binding) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(AnnotationNotSupportedError, api.IgnoreNonTransientErrorAnnotation))) + }) }) diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 06cf350b..75f2e2e2 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -1,6 +1,7 @@ package v1 import ( + "fmt" "time" "github.com/SAP/sap-btp-service-operator/api" @@ -153,7 +154,7 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetAnnotations(annotation) err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("cannot be a future timestamp")) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(utils.AnnotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation))) }) It("validate annotation exist and valid", func() { diff --git a/api/v1/serviceinstance_validating_webhook_test.go b/api/v1/serviceinstance_validating_webhook_test.go index 6e020f93..ceb25177 100644 --- a/api/v1/serviceinstance_validating_webhook_test.go +++ b/api/v1/serviceinstance_validating_webhook_test.go @@ -1,7 +1,9 @@ package v1 import ( + "fmt" "github.com/SAP/sap-btp-service-operator/api" + "github.com/SAP/sap-btp-service-operator/api/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "time" @@ -84,6 +86,7 @@ var _ = Describe("Service Instance Webhook Test", func() { } _, err := instance.ValidateCreate() Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(utils.AnnotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation))) }) }) @@ -95,6 +98,8 @@ var _ = Describe("Service Instance Webhook Test", func() { } _, err := instance.ValidateCreate() Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(utils.AnnotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation))) + }) }) }) From ca9e9f98b2aff0f382da0c261621830f077d4a6d Mon Sep 17 00:00:00 2001 From: Evyatar Yaffe Date: Tue, 19 Dec 2023 17:18:51 +0200 Subject: [PATCH 36/39] review --- api/common.go | 2 +- api/utils/utils.go | 55 ----- api/v1/serviceinstance_types_test.go | 66 ------ api/v1/serviceinstance_validating_webhook.go | 29 ++- ...serviceinstance_validating_webhook_test.go | 68 +++++- .../serviceinstance_mutating_webhook.go | 31 ++- .../samples/services_v1_serviceinstance.yaml | 2 - controllers/base_controller.go | 8 +- controllers/controller_util.go | 24 ++ controllers/controller_util_test.go | 41 +++- controllers/serviceinstance_controller.go | 10 +- .../serviceinstance_controller_test.go | 215 +++++++++--------- 12 files changed, 278 insertions(+), 273 deletions(-) delete mode 100644 api/utils/utils.go diff --git a/api/common.go b/api/common.go index 76256401..d2e2b02e 100644 --- a/api/common.go +++ b/api/common.go @@ -20,7 +20,7 @@ const ( PreventDeletion string = "services.cloud.sap.com/preventDeletion" UseInstanceMetadataNameInSecret string = "services.cloud.sap.com/useInstanceMetadataName" IgnoreNonTransientErrorAnnotation string = "services.cloud.sap.com/ignoreNonTransientError" - IgnoreNonTransientErrorTimestampAnnotation string = "services.cloud.sap.com/ignoreNonTransientTimestampError" + IgnoreNonTransientErrorTimestampAnnotation string = "services.cloud.sap.com/ignoreNonTransientErrorTimestamp" ) type HTTPStatusCodeError struct { diff --git a/api/utils/utils.go b/api/utils/utils.go deleted file mode 100644 index b67ff1e8..00000000 --- a/api/utils/utils.go +++ /dev/null @@ -1,55 +0,0 @@ -package utils - -import ( - "fmt" - "time" - - "github.com/SAP/sap-btp-service-operator/api" - "github.com/go-logr/logr" -) - -var AnnotationInFutureError = "Annotation %s cannot be a date in the future." -var AnnotationNotValidTimestampError = "Annotation %s is not a valid timestamp" - -func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource api.SAPBTPResource) error { - - sinceAnnotation, exist, err := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) - if err != nil { - return err - } - if exist && sinceAnnotation < 0 { - return fmt.Errorf(AnnotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation) - } - return nil -} - -func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource api.SAPBTPResource, timeout time.Duration) bool { - - sinceAnnotation, exist, _ := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) - if !exist { - return false - } - if sinceAnnotation > timeout { - log.Info(fmt.Sprintf("timeout reached- consider error to be non transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) - return false - } - log.Info(fmt.Sprintf("timeout didn't reached- consider error to be transient. since annotation timestamp %s, IgnoreNonTransientTimeout %s", sinceAnnotation, timeout)) - return true - -} - -func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource api.SAPBTPResource) (time.Duration, bool, error) { - annotation := resource.GetAnnotations() - if annotation != nil { - if _, ok := annotation[api.IgnoreNonTransientErrorAnnotation]; ok { - log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, annotation[api.IgnoreNonTransientErrorTimestampAnnotation]) - if err != nil { - log.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) - return time.Since(time.Now()), false, fmt.Errorf(AnnotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation) - } - return time.Since(annotationTime), true, nil - } - } - return time.Since(time.Now()), false, nil -} diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index 75f2e2e2..14d845f1 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -1,11 +1,7 @@ package v1 import ( - "fmt" - "time" - "github.com/SAP/sap-btp-service-operator/api" - "github.com/SAP/sap-btp-service-operator/api/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/meta" @@ -133,66 +129,4 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetAnnotations(annotation) Expect(instance.GetAnnotations()).To(Equal(annotation)) }) - - It("validate timestamp annotation- not date", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: "true", - } - instance.SetAnnotations(annotation) - err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("is not a valid timestamp")) - }) - It("validate timestamp annotation- future date", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), - } - instance.SetAnnotations(annotation) - err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(utils.AnnotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation))) - }) - It("validate annotation exist and valid", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), - } - instance.SetAnnotations(annotation) - exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) - Expect(exist).To(BeTrue()) - }) - It("validate annotation exist and valid", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), - } - instance.SetAnnotations(annotation) - err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) - Expect(err).NotTo(HaveOccurred()) - }) - It("validate timeout for Ignore Non Transient Error Annotation", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), - } - instance.SetAnnotations(annotation) - exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) - Expect(exist).To(BeFalse()) - }) - It("validate annotation not exist", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), - } - instance.SetAnnotations(annotation) - exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) - Expect(exist).To(BeFalse()) - }) }) diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index 6294ebc3..3d18905e 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -19,8 +19,7 @@ package v1 import ( "fmt" "strings" - - "github.com/SAP/sap-btp-service-operator/api/utils" + "time" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -31,6 +30,11 @@ import ( "github.com/SAP/sap-btp-service-operator/api" ) +const ( + annotationInFutureError = "Annotation %s cannot be a date in the future." + annotationNotValidTimestampError = "Annotation %s is not a valid timestamp" +) + func (si *ServiceInstance) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). For(si). @@ -45,7 +49,7 @@ var _ webhook.Validator = &ServiceInstance{} var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) { - return nil, utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) + return nil, si.validateNonTransientTimestampAnnotation() } func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { @@ -55,7 +59,7 @@ func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admissio if oldInstance.Spec.BTPAccessCredentialsSecret != si.Spec.BTPAccessCredentialsSecret { return nil, fmt.Errorf("changing the btpAccessCredentialsSecret for an existing instance is not allowed") } - return nil, utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) + return nil, si.validateNonTransientTimestampAnnotation() } func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) { @@ -68,3 +72,20 @@ func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err er } return nil, nil } + +func (si *ServiceInstance) validateNonTransientTimestampAnnotation() error { + if len(si.Annotations) > 0 && len(si.Annotations[api.IgnoreNonTransientErrorAnnotation]) > 0 { + serviceinstancelog.Info(fmt.Sprintf("%s annotation exist, validating %s annotation", api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation)) + annotationTime, err := time.Parse(time.RFC3339, si.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) + if err != nil { + serviceinstancelog.Error(err, fmt.Sprintf("failed to parse %s", api.IgnoreNonTransientErrorTimestampAnnotation)) + return fmt.Errorf(annotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation) + } + + if time.Since(annotationTime) < 0 { + return fmt.Errorf(annotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation) + } + } + + return nil +} diff --git a/api/v1/serviceinstance_validating_webhook_test.go b/api/v1/serviceinstance_validating_webhook_test.go index ceb25177..514a1e42 100644 --- a/api/v1/serviceinstance_validating_webhook_test.go +++ b/api/v1/serviceinstance_validating_webhook_test.go @@ -3,7 +3,6 @@ package v1 import ( "fmt" "github.com/SAP/sap-btp-service-operator/api" - "github.com/SAP/sap-btp-service-operator/api/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "time" @@ -86,7 +85,7 @@ var _ = Describe("Service Instance Webhook Test", func() { } _, err := instance.ValidateCreate() Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(utils.AnnotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation))) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(annotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation))) }) }) @@ -98,10 +97,73 @@ var _ = Describe("Service Instance Webhook Test", func() { } _, err := instance.ValidateCreate() Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(utils.AnnotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation))) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(annotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation))) }) }) }) }) + + Context("validateNonTransientTimestampAnnotation", func() { + It("fails when not a valid date", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: "true", + } + instance.SetAnnotations(annotation) + err := instance.validateNonTransientTimestampAnnotation() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("is not a valid timestamp")) + }) + + It("fails when a future date", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + err := instance.validateNonTransientTimestampAnnotation() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(annotationInFutureError, api.IgnoreNonTransientErrorTimestampAnnotation))) + }) + + It("succeeds when timestamp is valid and in the past", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + err := instance.validateNonTransientTimestampAnnotation() + Expect(err).NotTo(HaveOccurred()) + }) + // + //It("validate annotation exist and valid", func() { + // annotation := map[string]string{ + // api.IgnoreNonTransientErrorAnnotation: "true", + // api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + // } + // instance.SetAnnotations(annotation) + // exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + // Expect(exist).To(BeTrue()) + //}) + // + //It("validate timeout for Ignore Non Transient Error Annotation", func() { + // annotation := map[string]string{ + // api.IgnoreNonTransientErrorAnnotation: "true", + // api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), + // } + // instance.SetAnnotations(annotation) + // exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + // Expect(exist).To(BeFalse()) + //}) + // + //It("validate annotation not exist", func() { + // annotation := map[string]string{ + // api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + // } + // instance.SetAnnotations(annotation) + // exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + // Expect(exist).To(BeFalse()) + //}) + }) }) diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index da64aaf5..8ea43fd9 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -3,6 +3,7 @@ package webhooks import ( "context" "encoding/json" + "fmt" "net/http" "reflect" "time" @@ -28,26 +29,32 @@ type ServiceInstanceDefaulter struct { func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Request) admission.Response { instancelog.Info("Defaulter webhook for serviceinstance") instance := &servicesv1.ServiceInstance{} - err := s.Decoder.Decode(req, instance) - if err != nil { + if err := s.Decoder.Decode(req, instance); err != nil { return admission.Errored(http.StatusBadRequest, err) } - // mutate the fields if len(instance.Spec.ExternalName) == 0 { - instancelog.Info("externalName not provided, defaulting to k8s name", "name", instance.Name) + instancelog.Info(fmt.Sprintf("externalName not provided, defaulting to k8s name: %s", instance.Name)) instance.Spec.ExternalName = instance.Name } - err = s.setServiceInstanceUserInfo(req, instance) - if err != nil { + if err := s.setServiceInstanceUserInfo(req, instance); err != nil { return admission.Errored(http.StatusInternalServerError, err) } - s.setIgnoreNonTransientErrorTimestampAnnotation(instance) + + if len(instance.Annotations) > 0 && len(instance.Annotations[api.IgnoreNonTransientErrorAnnotation]) > 0 { + if _, exist := instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; !exist { + annotationValue := time.Now().Format(time.RFC3339) + instancelog.Info(fmt.Sprintf("%s annotation exists, adding %s annotation with value %s", api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation, annotationValue)) + instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation] = annotationValue + } + } + marshaledInstance, err := json.Marshal(instance) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } + return admission.PatchResponseFromRaw(req.Object.Raw, marshaledInstance) } @@ -73,13 +80,3 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ } return nil } - -func (s *ServiceInstanceDefaulter) setIgnoreNonTransientErrorTimestampAnnotation(instance *servicesv1.ServiceInstance) { - if instance.Annotations != nil { - if _, ok := instance.Annotations[api.IgnoreNonTransientErrorAnnotation]; ok { - if _, exist := instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation]; !exist { - instance.Annotations[api.IgnoreNonTransientErrorTimestampAnnotation] = time.Now().Format(time.RFC3339) - } - } - } -} diff --git a/config/samples/services_v1_serviceinstance.yaml b/config/samples/services_v1_serviceinstance.yaml index 867fec80..bd19f598 100644 --- a/config/samples/services_v1_serviceinstance.yaml +++ b/config/samples/services_v1_serviceinstance.yaml @@ -2,8 +2,6 @@ apiVersion: services.cloud.sap.com/v1 kind: ServiceInstance metadata: name: sample-instance-1 - annotations: - services.cloud.sap.com/ignoreNonTransientError: "true" spec: serviceOfferingName: service-manager servicePlanName: subaccount-audit diff --git a/controllers/base_controller.go b/controllers/base_controller.go index bad76e45..7deced23 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -4,10 +4,6 @@ import ( "context" "errors" "fmt" - "net/http" - - "github.com/SAP/sap-btp-service-operator/api/utils" - "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -20,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + "net/http" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -350,8 +347,7 @@ func (r *BaseReconciler) handleError(ctx context.Context, operationType smClient log.Info("unable to cast error to SM error, will be treated as non transient") return r.markAsNonTransientError(ctx, operationType, err.Error(), resource) } - if r.isTransientError(smError, log) || - utils.IsIgnoreNonTransientAnnotationExistAndValid(log, resource, r.Config.IgnoreNonTransientTimeout) { + if r.isTransientError(smError, log) || shouldIgnoreNonTransient(log, resource, r.Config.IgnoreNonTransientTimeout) { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } diff --git a/controllers/controller_util.go b/controllers/controller_util.go index ebdd9b00..d0927776 100644 --- a/controllers/controller_util.go +++ b/controllers/controller_util.go @@ -3,7 +3,11 @@ package controllers import ( "context" "encoding/json" + "fmt" + "github.com/SAP/sap-btp-service-operator/api" + "github.com/go-logr/logr" "strings" + "time" "k8s.io/apimachinery/pkg/util/rand" @@ -24,6 +28,26 @@ const ( UNKNOWN format = "unknown" ) +func shouldIgnoreNonTransient(log logr.Logger, resource api.SAPBTPResource, timeout time.Duration) bool { + annotations := resource.GetAnnotations() + if len(annotations) == 0 || len(annotations[api.IgnoreNonTransientErrorAnnotation]) == 0 { + return false + } + + // we ignore the error + // for service instances, the value is validated in webhook + // for service bindings, the annotation is not allowed + annotationTime, _ := time.Parse(time.RFC3339, annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) + + if sinceAnnotation := time.Since(annotationTime); sinceAnnotation > timeout { + log.Info(fmt.Sprintf("timeout of %s reached - error is considered to be non transient. time passed since annotation timestamp %s", timeout, sinceAnnotation)) + return false + } else { + log.Info(fmt.Sprintf("timeout of %s was not reached - error is considered to be transient. ime passed since annotation timestamp %s", timeout, sinceAnnotation)) + return true + } +} + func normalizeCredentials(credentialsJSON json.RawMessage) (map[string][]byte, []SecretMetadataProperty, error) { var credentialsMap map[string]interface{} err := json.Unmarshal(credentialsJSON, &credentialsMap) diff --git a/controllers/controller_util_test.go b/controllers/controller_util_test.go index 790186a5..5e94c7f1 100644 --- a/controllers/controller_util_test.go +++ b/controllers/controller_util_test.go @@ -2,12 +2,15 @@ package controllers import ( "encoding/json" - + "github.com/SAP/sap-btp-service-operator/api" + v1 "github.com/SAP/sap-btp-service-operator/api/v1" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "time" ) -var _ = Describe("Controller Util", func() { +var _ = FDescribe("Controller Util", func() { Context("normalize credentials", func() { var credentialsJSON json.RawMessage @@ -40,4 +43,38 @@ var _ = Describe("Controller Util", func() { }) }) + + Context("shouldIgnoreNonTransient", func() { + var ( + instance *v1.ServiceInstance + logger = logf.Log.WithName("test-logger") + ) + + BeforeEach(func() { + instance = &v1.ServiceInstance{} + }) + + It("should return false if no ignore annotation", func() { + instance.SetAnnotations(nil) + Expect(shouldIgnoreNonTransient(logger, instance, time.Hour)).To(BeFalse()) + }) + + It("should return false if time exceeded", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + Expect(shouldIgnoreNonTransient(logger, instance, time.Hour)).To(BeFalse()) + }) + + It("should return true if time not exceeded", func() { + annotation := map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), + } + instance.SetAnnotations(annotation) + Expect(shouldIgnoreNonTransient(logger, instance, time.Hour)).To(BeTrue()) + }) + }) }) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 47e229f2..b50a17be 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -24,8 +24,6 @@ import ( "fmt" "net/http" - "github.com/SAP/sap-btp-service-operator/api/utils" - "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -79,9 +77,9 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } - if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && - utils.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { - changeLastConditionToInProgress(serviceInstance) + if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionFailed, metav1.ConditionTrue) && + shouldIgnoreNonTransient(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { + markNonTransientStatusAsTransient(serviceInstance) if err := r.Status().Update(ctx, serviceInstance); err != nil { return ctrl.Result{}, err } @@ -721,7 +719,7 @@ func getErrorMsgFromLastOperation(status *smClientTypes.Operation) string { return errMsg } -func changeLastConditionToInProgress(serviceInstance *servicesv1.ServiceInstance) { +func markNonTransientStatusAsTransient(serviceInstance *servicesv1.ServiceInstance) { conditions := serviceInstance.GetConditions() lastOpCondition := meta.FindStatusCondition(conditions, api.ConditionSucceeded) operation := smClientTypes.CREATE diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index d4cbc943..d38b8762 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -81,28 +81,7 @@ var _ = Describe("ServiceInstance controller", func() { }, } - createInstance := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, waitForReady bool) *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()) - - if !waitForReady { - return instance - } - waitForResourceToBeReady(ctx, instance) - return instance - } - - createInstanceWithAnnotation := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, annotation map[string]string, waitForReady bool) *v1.ServiceInstance { + createInstance := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, annotations map[string]string, waitForReady bool) *v1.ServiceInstance { instance := &v1.ServiceInstance{ TypeMeta: metav1.TypeMeta{ APIVersion: "services.cloud.sap.com/v1", @@ -111,7 +90,7 @@ var _ = Describe("ServiceInstance controller", func() { ObjectMeta: metav1.ObjectMeta{ Name: fakeInstanceName, Namespace: testNamespace, - Annotations: annotation, + Annotations: annotations, }, Spec: instanceSpec, } @@ -143,7 +122,7 @@ var _ = Describe("ServiceInstance controller", func() { }, timeout, interval).Should(BeTrue()) } } - ignoreNonTransientErrorAnnotation := map[string]string{api.IgnoreNonTransientErrorAnnotation: "true"} + BeforeEach(func() { ctx = context.Background() log := ctrl.Log.WithName("instanceTest") @@ -222,7 +201,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("provisioning should fail", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForInstanceConditionAndMessage(ctx, defaultLookupKey, api.ConditionSucceeded, "provided plan id does not match") }) }) @@ -232,7 +211,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("Sync", func() { When("provision request to SM succeeds", func() { It("should provision instance of the provided offering and plan name successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) Expect(serviceInstance.Status.SubaccountID).To(Equal(fakeSubaccountID)) Expect(serviceInstance.Spec.ExternalName).To(Equal(fakeInstanceExternalName)) @@ -248,8 +227,9 @@ var _ = Describe("ServiceInstance controller", func() { }) When("provision request to SM fails", func() { - Context("first with 400 status then success", func() { - errMessage := "failed to provision instance" + errMessage := "failed to provision instance" + + Context("provision fails once and then succeeds", func() { BeforeEach(func() { fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, @@ -259,35 +239,43 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should have failure condition", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) }) - It("ignoreNonTransientErrorAnnotation should remove the annotation once succeeded", func() { - serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) - waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") - waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) - }) }) - Context("with 400 status", func() { - errMessage := "failed to provision instance" - BeforeEach(func() { - fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ - StatusCode: http.StatusBadRequest, - Description: errMessage, + + Context("ignoreNonTransientErrorAnnotation exists", func() { + When("provision fails once and then succeeds", func() { + It("should remove the annotation", func() { + fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ + StatusCode: http.StatusBadRequest, + Description: errMessage, + }) + fakeClient.ProvisionReturnsOnCall(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + serviceInstance = createInstance(ctx, instanceSpec, map[string]string{api.IgnoreNonTransientErrorAnnotation: "true"}, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) }) }) - It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { - serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) - waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) - waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) - waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) - sinceCreate := time.Since(serviceInstance.GetCreationTimestamp().Time) - Expect(sinceCreate > ignoreNonTransientTimeout) + + When("provision fails until timeout", func() { + It("should have failure conditions and remove the annotation", func() { + fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ + StatusCode: http.StatusBadRequest, + Description: errMessage, + }) + serviceInstance = createInstance(ctx, instanceSpec, map[string]string{api.IgnoreNonTransientErrorAnnotation: "true"}, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) + waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) + sinceCreate := time.Since(serviceInstance.GetCreationTimestamp().Time) + Expect(sinceCreate > ignoreNonTransientTimeout) + }) }) }) + Context("with 429 status eventually succeeds", func() { BeforeEach(func() { - errMessage := "failed to provision instance" fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ StatusCode: http.StatusTooManyRequests, Description: errMessage, @@ -296,47 +284,49 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should retry until success", func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") }) }) Context("with sm status code 502 and broker status code 429", func() { - errMessage := "broker too many requests" + tooManyRequestsError := "broker too many requests" BeforeEach(func() { - fakeClient.ProvisionReturns(nil, getTransientBrokerError(errMessage)) + fakeClient.ProvisionReturns(nil, getTransientBrokerError(tooManyRequestsError)) }) It("should be transient error and eventually succeed", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) - waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, CreateInProgress, errMessage) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, CreateInProgress, tooManyRequestsError) fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) waitForResourceToBeReady(ctx, serviceInstance) }) }) Context("with sm status code 502 and broker status code 400", func() { - errMessage := "failed to provision instance" BeforeEach(func() { fakeClient.ProvisionReturns(nil, getNonTransientBrokerError(errMessage)) fakeClient.ProvisionReturnsOnCall(2, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) }) It("should have failure condition - non transient error", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, errMessage) }) - It("ignoreNonTransientErrorAnnotation should have failure condition after timeout", func() { - serviceInstance = createInstanceWithAnnotation(ctx, instanceSpec, ignoreNonTransientErrorAnnotation, false) - waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") - waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) - Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) + + When("ignoreNonTransientErrorAnnotation exists", func() { + It("should have failure conditions and remove the annotation after timeout", func() { + serviceInstance = createInstance(ctx, instanceSpec, map[string]string{api.IgnoreNonTransientErrorAnnotation: "true"}, false) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionTrue, Created, "") + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) + Expect(fakeClient.ProvisionCallCount()).To(BeNumerically(">", 1)) + }) }) }) }) }) - Context("async", func() { + Context("Async", func() { BeforeEach(func() { fakeClient.ProvisionReturns(&sm.ProvisionResponse{InstanceID: fakeInstanceID, Location: "/v1/service_instances/fakeid/operations/1234"}, nil) fakeClient.StatusReturns(&smclientTypes.Operation{ @@ -351,7 +341,7 @@ var _ = Describe("ServiceInstance controller", func() { fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{Labels: map[string][]string{"subaccount_id": {fakeSubaccountID}}}, nil) }) It("should update in progress condition and provision the instance successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) fakeClient.StatusReturns(&smclientTypes.Operation{ ID: "1234", Type: smClientTypes.CREATE, @@ -364,7 +354,7 @@ var _ = Describe("ServiceInstance controller", func() { When("polling ends with failure", func() { It("should update to failure condition with the broker err description", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) fakeClient.StatusReturns(&smclientTypes.Operation{ ID: "1234", Type: smClientTypes.CREATE, @@ -377,7 +367,7 @@ var _ = Describe("ServiceInstance controller", func() { When("updating during create", func() { It("should update the instance after created successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, CreateInProgress, "") newName := "new-name" + uuid.New().String() @@ -405,7 +395,7 @@ var _ = Describe("ServiceInstance controller", func() { When("deleting while create is in progress", func() { It("should be deleted successfully", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) By("waiting for instance to be CreateInProgress") waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, CreateInProgress, "") @@ -437,7 +427,7 @@ var _ = Describe("ServiceInstance controller", func() { ServicePlanName: "a-plan-name", ServiceOfferingName: "an-offering-name", } - serviceInstance = createInstance(ctx, withoutExternal, true) + serviceInstance = createInstance(ctx, withoutExternal, nil, true) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) Expect(serviceInstance.Spec.ExternalName).To(Equal(fakeInstanceName)) Expect(serviceInstance.Name).To(Equal(fakeInstanceName)) @@ -447,7 +437,7 @@ var _ = Describe("ServiceInstance controller", func() { Describe("Update", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) Expect(serviceInstance.Spec.ExternalName).To(Equal(fakeInstanceExternalName)) }) @@ -552,40 +542,43 @@ var _ = Describe("ServiceInstance controller", func() { }) }) - Context("spec is changed, sm returns 502 and broker returns 429", func() { - errMessage := "broker too many requests" - BeforeEach(func() { - fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError(errMessage)) - }) + Context("spec is changed and sm returns 502", func() { + When("the error is transient", func() { + errMessage := "broker too many requests" + BeforeEach(func() { + fakeClient.UpdateInstanceReturns(nil, "", getTransientBrokerError(errMessage)) + }) - It("recognize the error as transient and eventually succeed", func() { - newExternalName := "my-new-external-name" + uuid.New().String() - serviceInstance.Spec.ExternalName = newExternalName - updateInstance(ctx, serviceInstance) - waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateInProgress, "") - fakeClient.UpdateInstanceReturns(nil, "", nil) - updateInstance(ctx, serviceInstance) - waitForResourceToBeReady(ctx, serviceInstance) - }) - }) - Context("spec is changed,ignoreNonTransientErrorAnnotation and sm returns 502 and broker returns non transient error", func() { - errMessage := "broker too many requests" - BeforeEach(func() { - fakeClient.UpdateInstanceReturns(nil, "", getNonTransientBrokerError(errMessage)) + It("recognize the error as transient and eventually succeed", func() { + newExternalName := "my-new-external-name" + uuid.New().String() + serviceInstance.Spec.ExternalName = newExternalName + updateInstance(ctx, serviceInstance) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateInProgress, "") + fakeClient.UpdateInstanceReturns(nil, "", nil) + updateInstance(ctx, serviceInstance) + waitForResourceToBeReady(ctx, serviceInstance) + }) }) - It("recognize the error as transient and eventually succeed", func() { - newExternalName := "my-new-external-name" + uuid.New().String() - serviceInstance.Spec.ExternalName = newExternalName - updateInstance(ctx, serviceInstance) - waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateFailed, "") - serviceInstance.Annotations = ignoreNonTransientErrorAnnotation - updateInstance(ctx, serviceInstance) - waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateInProgress, "") - fakeClient.UpdateInstanceReturns(nil, "", nil) - waitForResourceToBeReady(ctx, serviceInstance) - waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) + When("the error is non transient but ignoreNonTransientErrorAnnotation exists", func() { + errMessage := "broker update error" + BeforeEach(func() { + fakeClient.UpdateInstanceReturns(nil, "", getNonTransientBrokerError(errMessage)) + }) + It("recognizes the error as transient and eventually succeed", func() { + newExternalName := "my-new-external-name" + uuid.New().String() + serviceInstance.Spec.ExternalName = newExternalName + updateInstance(ctx, serviceInstance) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateFailed, "") + serviceInstance.Annotations = map[string]string{api.IgnoreNonTransientErrorAnnotation: "true"} + updateInstance(ctx, serviceInstance) + waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, UpdateInProgress, "") + fakeClient.UpdateInstanceReturns(nil, "", nil) + + waitForResourceToBeReady(ctx, serviceInstance) + waitForResourceAnnotationRemove(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) + }) }) }) @@ -639,7 +632,7 @@ var _ = Describe("ServiceInstance controller", func() { When("subaccount id changed", func() { It("should fail", func() { deleteInstance(ctx, serviceInstance, true) - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) serviceInstance.Spec.BTPAccessCredentialsSecret = "12345" err := k8sClient.Update(ctx, serviceInstance) Expect(err).To(HaveOccurred()) @@ -650,7 +643,7 @@ var _ = Describe("ServiceInstance controller", func() { Describe("Delete", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) fakeClient.DeprovisionReturns("", nil) }) AfterEach(func() { @@ -799,7 +792,7 @@ var _ = Describe("ServiceInstance controller", func() { Describe("full reconcile", func() { When("instance hashedSpec is not initialized", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) }) It("should not send update request and update the hashed spec", func() { hashed := serviceInstance.Status.HashedSpec @@ -830,7 +823,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should call correctly to SM and recover the instance", func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) smCallArgs := fakeClient.ListInstancesArgsForCall(0) @@ -853,7 +846,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should recover the existing instance and poll until instance is ready", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) key := getResourceNamespacedName(serviceInstance) Eventually(func() bool { _ = k8sClient.Get(ctx, key, serviceInstance) @@ -874,7 +867,7 @@ var _ = Describe("ServiceInstance controller", func() { }) It("should recover the existing instance and update condition failure", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionSucceeded, metav1.ConditionFalse, CreateFailed, "") Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) @@ -892,7 +885,7 @@ var _ = Describe("ServiceInstance controller", func() { recoveredInstance.Ready = true }) It("should recover the instance with status Ready=true", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForResourceToBeReady(ctx, serviceInstance) Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) @@ -903,7 +896,7 @@ var _ = Describe("ServiceInstance controller", func() { recoveredInstance.Ready = false }) It("should recover the instance with status Ready=false", func() { - serviceInstance = createInstance(ctx, instanceSpec, false) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, "") Expect(fakeClient.ProvisionCallCount()).To(Equal(0)) Expect(serviceInstance.Status.InstanceID).To(Equal(fakeInstanceID)) @@ -920,14 +913,14 @@ var _ = Describe("ServiceInstance controller", func() { When("creating instance with shared=true", func() { It("should succeed to provision and sharing the instance", func() { fakeClient.ShareInstanceReturns(nil) - serviceInstance = createInstance(ctx, sharedInstanceSpec, true) + serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, true) waitForInstanceToBeShared(ctx, serviceInstance) }) }) Context("sharing an existing instance", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) }) When("updating existing instance to shared", func() { @@ -968,7 +961,7 @@ var _ = Describe("ServiceInstance controller", func() { StatusCode: http.StatusBadRequest, Description: "errMessage", }) - serviceInstance = createInstance(ctx, sharedInstanceSpec, false) + serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, false) waitForResourceCondition(ctx, serviceInstance, api.ConditionFailed, metav1.ConditionTrue, CreateFailed, "") Expect(fakeClient.ShareInstanceCallCount()).To(BeZero()) }) @@ -976,7 +969,7 @@ var _ = Describe("ServiceInstance controller", func() { When("instance is valid and share failed", func() { BeforeEach(func() { - serviceInstance = createInstance(ctx, instanceSpec, true) + serviceInstance = createInstance(ctx, instanceSpec, nil, true) }) When("shared failed with rate limit error", func() { @@ -1038,7 +1031,7 @@ var _ = Describe("ServiceInstance controller", func() { Context("un-sharing an existing shared instance", func() { BeforeEach(func() { fakeClient.ShareInstanceReturns(nil) - serviceInstance = createInstance(ctx, sharedInstanceSpec, true) + serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, true) waitForInstanceToBeShared(ctx, serviceInstance) }) @@ -1077,7 +1070,7 @@ var _ = Describe("ServiceInstance controller", func() { When("instance is valid and un-share failed", func() { BeforeEach(func() { fakeClient.ShareInstanceReturns(nil) - serviceInstance = createInstance(ctx, sharedInstanceSpec, true) + serviceInstance = createInstance(ctx, sharedInstanceSpec, nil, true) waitForInstanceToBeShared(ctx, serviceInstance) fakeClient.UnShareInstanceReturns(&sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, From 997f0fccd63a5a0f5400eefcfac3a7ee4f626932 Mon Sep 17 00:00:00 2001 From: Evyatar Yaffe Date: Tue, 19 Dec 2023 18:18:46 +0200 Subject: [PATCH 37/39] remove test focus --- controllers/controller_util_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/controller_util_test.go b/controllers/controller_util_test.go index 5e94c7f1..a78018a6 100644 --- a/controllers/controller_util_test.go +++ b/controllers/controller_util_test.go @@ -10,7 +10,7 @@ import ( "time" ) -var _ = FDescribe("Controller Util", func() { +var _ = Describe("Controller Util", func() { Context("normalize credentials", func() { var credentialsJSON json.RawMessage From 9576626973b5b4c8bd714a3b699d5a0702c3e110 Mon Sep 17 00:00:00 2001 From: Naama Solomon <52195888+I065450@users.noreply.github.com> Date: Wed, 20 Dec 2023 09:52:54 +0200 Subject: [PATCH 38/39] [SAPBTPCFS-7876] Optimize handling of non-transient errors --- controllers/controller_util.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/controllers/controller_util.go b/controllers/controller_util.go index d0927776..98adf6f5 100644 --- a/controllers/controller_util.go +++ b/controllers/controller_util.go @@ -38,14 +38,13 @@ func shouldIgnoreNonTransient(log logr.Logger, resource api.SAPBTPResource, time // for service instances, the value is validated in webhook // for service bindings, the annotation is not allowed annotationTime, _ := time.Parse(time.RFC3339, annotations[api.IgnoreNonTransientErrorTimestampAnnotation]) - - if sinceAnnotation := time.Since(annotationTime); sinceAnnotation > timeout { + sinceAnnotation := time.Since(annotationTime) + if sinceAnnotation > timeout { log.Info(fmt.Sprintf("timeout of %s reached - error is considered to be non transient. time passed since annotation timestamp %s", timeout, sinceAnnotation)) return false - } else { - log.Info(fmt.Sprintf("timeout of %s was not reached - error is considered to be transient. ime passed since annotation timestamp %s", timeout, sinceAnnotation)) - return true } + log.Info(fmt.Sprintf("timeout of %s was not reached - error is considered to be transient. ime passed since annotation timestamp %s", timeout, sinceAnnotation)) + return true } func normalizeCredentials(credentialsJSON json.RawMessage) (map[string][]byte, []SecretMetadataProperty, error) { From 9e7269da20e441aecf8808f7262fa12c092b5ec1 Mon Sep 17 00:00:00 2001 From: I501080 Date: Wed, 20 Dec 2023 10:04:16 +0200 Subject: [PATCH 39/39] go import --- controllers/base_controller.go | 3 ++- controllers/controller_util.go | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 7deced23..81170c1f 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "net/http" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -16,7 +18,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" - "net/http" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" diff --git a/controllers/controller_util.go b/controllers/controller_util.go index 98adf6f5..84f599a6 100644 --- a/controllers/controller_util.go +++ b/controllers/controller_util.go @@ -4,11 +4,12 @@ import ( "context" "encoding/json" "fmt" - "github.com/SAP/sap-btp-service-operator/api" - "github.com/go-logr/logr" "strings" "time" + "github.com/SAP/sap-btp-service-operator/api" + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/util/rand" v1 "k8s.io/api/authentication/v1"