diff --git a/api/common.go b/api/common.go index f81a1638..d2e2b02e 100644 --- a/api/common.go +++ b/api/common.go @@ -11,14 +11,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" + 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/ignoreNonTransientErrorTimestamp" ) type HTTPStatusCodeError struct { @@ -33,8 +35,8 @@ type HTTPStatusCodeError struct { } func (e HTTPStatusCodeError) Error() string { - errorMessage := "" - description := "" + errorMessage := "" + description := "" if e.ErrorMessage != nil { errorMessage = *e.ErrorMessage @@ -42,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 ( @@ -79,4 +81,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/servicebinding_types_test.go b/api/v1/servicebinding_types_test.go index ed4d9cb2..14ce9c5a 100644 --- a/api/v1/servicebinding_types_test.go +++ b/api/v1/servicebinding_types_test.go @@ -97,4 +97,12 @@ 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/servicebinding_validating_webhook.go b/api/v1/servicebinding_validating_webhook.go index 04df9b93..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). @@ -49,6 +50,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 +64,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 +131,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(AnnotationNotSupportedError, api.IgnoreNonTransientErrorAnnotation) + } + } + return nil +} diff --git a/api/v1/servicebinding_validating_webhook_test.go b/api/v1/servicebinding_validating_webhook_test.go index b9ae8aab..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" @@ -19,6 +20,15 @@ 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()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(AnnotationNotSupportedError, api.IgnoreNonTransientErrorAnnotation))) + + }) }) Context("Validate update of spec before binding is created (failure recovery)", func() { @@ -109,6 +119,18 @@ 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()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(AnnotationNotSupportedError, api.IgnoreNonTransientErrorAnnotation))) + + }) + }) + }) Context("Validate update of spec after binding is created", func() { 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/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)) + }) }) diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index d55d74be..3d18905e 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -19,6 +19,7 @@ package v1 import ( "fmt" "strings" + "time" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -29,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). @@ -43,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, nil + return nil, si.validateNonTransientTimestampAnnotation() } func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { @@ -53,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, nil + return nil, si.validateNonTransientTimestampAnnotation() } func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) { @@ -66,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 3323830d..514a1e42 100644 --- a/api/v1/serviceinstance_validating_webhook_test.go +++ b/api/v1/serviceinstance_validating_webhook_test.go @@ -1,9 +1,11 @@ package v1 import ( + "fmt" "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 +26,27 @@ 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 return error from webhook", func() { + instance.Annotations = map[string]string{ + api.IgnoreNonTransientErrorAnnotation: "true", + 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 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) + Expect(err).To(HaveOccurred()) + }) + }) }) Context("Validate Delete", func() { @@ -53,5 +76,94 @@ 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.IgnoreNonTransientErrorAnnotation: "true", + api.IgnoreNonTransientErrorTimestampAnnotation: "true", + } + _, err := instance.ValidateCreate() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(annotationNotValidTimestampError, api.IgnoreNonTransientErrorTimestampAnnotation))) + + }) + }) + 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() + Expect(err).To(HaveOccurred()) + 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 9efc1a30..8ea43fd9 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -3,8 +3,12 @@ package webhooks import ( "context" "encoding/json" + "fmt" "net/http" "reflect" + "time" + + "github.com/SAP/sap-btp-service-operator/api" v1admission "k8s.io/api/admission/v1" v1 "k8s.io/api/authentication/v1" @@ -25,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) } + 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) } @@ -55,6 +65,7 @@ func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Requ Groups: req.UserInfo.Groups, Extra: req.UserInfo.Extra, } + if req.Operation == v1admission.Create || req.Operation == v1admission.Delete { instance.Spec.UserInfo = userInfo } else if req.Operation == v1admission.Update { diff --git a/api/v1alpha1/serviceinstance_types_test.go b/api/v1alpha1/serviceinstance_types_test.go index 41ac8e59..536fa1d6 100644 --- a/api/v1alpha1/serviceinstance_types_test.go +++ b/api/v1alpha1/serviceinstance_types_test.go @@ -11,6 +11,7 @@ import ( var _ = Describe("Service Instance Type Test", func() { var instance *ServiceInstance + BeforeEach(func() { instance = getInstance() conditions := instance.GetConditions() @@ -121,4 +122,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)) + }) }) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 9ee69908..81170c1f 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -2,6 +2,7 @@ package controllers import ( "context" + "errors" "fmt" "net/http" @@ -250,7 +251,6 @@ func setSuccessConditions(operationType smClientTypes.OperationCategory, object object.SetConditions(conditions) object.SetReady(metav1.ConditionTrue) - } func setCredRotationInProgressConditions(reason, message string, object api.SAPBTPResource) { @@ -320,7 +320,7 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func isTransientError(smError *sm.ServiceManagerError, log logr.Logger) 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) @@ -342,15 +342,16 @@ 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) } - - if isTransient := isTransientError(smError, log); isTransient { + if r.isTransientError(smError, log) || shouldIgnoreNonTransient(log, resource, r.Config.IgnoreNonTransientTimeout) { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } + return r.markAsNonTransientError(ctx, operationType, smError.Error(), resource) } @@ -382,6 +383,26 @@ func (r *BaseReconciler) markAsTransientError(ctx context.Context, operationType return ctrl.Result{}, fmt.Errorf(errMsg) } +func (r *BaseReconciler) removeAnnotation(ctx context.Context, object api.SAPBTPResource, keys ...string) error { + log := GetLogger(ctx) + annotations := object.GetAnnotations() + shouldUpdate := false + if annotations != nil { + 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) + } + } + return nil +} + func isInProgress(object api.SAPBTPResource) bool { conditions := object.GetConditions() return meta.IsStatusConditionPresentAndEqual(conditions, api.ConditionSucceeded, metav1.ConditionFalse) && diff --git a/controllers/controller_util.go b/controllers/controller_util.go index ebdd9b00..84f599a6 100644 --- a/controllers/controller_util.go +++ b/controllers/controller_util.go @@ -3,7 +3,12 @@ package controllers import ( "context" "encoding/json" + "fmt" "strings" + "time" + + "github.com/SAP/sap-btp-service-operator/api" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/util/rand" @@ -24,6 +29,25 @@ 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]) + 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 + } + 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..a78018a6 100644 --- a/controllers/controller_util_test.go +++ b/controllers/controller_util_test.go @@ -2,9 +2,12 @@ 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() { @@ -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 fe27f3e4..b50a17be 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -77,12 +77,24 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } + 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 + } + } + if isFinalState(ctx, serviceInstance) { 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{}, nil + + return ctrl.Result{}, r.removeAnnotation(ctx, serviceInstance, api.IgnoreNonTransientErrorAnnotation, api.IgnoreNonTransientErrorTimestampAnnotation) } if isMarkedForDeletion(serviceInstance.ObjectMeta) { @@ -512,7 +524,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 = r.isTransientError(smError, log) errMsg = smError.Error() if smError.StatusCode == http.StatusTooManyRequests { @@ -564,7 +576,6 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan return true } -// TODO unit test 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 { @@ -579,7 +590,6 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { 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 { @@ -708,3 +718,19 @@ func getErrorMsgFromLastOperation(status *smClientTypes.Operation) string { } return errMsg } + +func markNonTransientStatusAsTransient(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) + serviceInstance.SetConditions(conditions) +} diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 4a11bcb2..d38b8762 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 @@ -80,15 +81,16 @@ var _ = Describe("ServiceInstance controller", func() { }, } - createInstance := func(ctx context.Context, instanceSpec v1.ServiceInstanceSpec, 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", Kind: "ServiceInstance", }, ObjectMeta: metav1.ObjectMeta{ - Name: fakeInstanceName, - Namespace: testNamespace, + Name: fakeInstanceName, + Namespace: testNamespace, + Annotations: annotations, }, Spec: instanceSpec, } @@ -199,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") }) }) @@ -209,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)) @@ -225,10 +227,11 @@ var _ = Describe("ServiceInstance controller", func() { }) When("provision request to SM fails", func() { - Context("with 400 status", func() { - errMessage := "failed to provision instance" + errMessage := "failed to provision instance" + + Context("provision fails once and then succeeds", func() { BeforeEach(func() { - fakeClient.ProvisionReturns(nil, &sm.ServiceManagerError{ + fakeClient.ProvisionReturnsOnCall(0, nil, &sm.ServiceManagerError{ StatusCode: http.StatusBadRequest, Description: errMessage, }) @@ -236,14 +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) }) }) + 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) + }) + }) + + 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, @@ -252,41 +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(1, &sm.ProvisionResponse{InstanceID: fakeInstanceID}, nil) + 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) }) + + 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{ @@ -298,10 +338,10 @@ 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) + serviceInstance = createInstance(ctx, instanceSpec, nil, false) fakeClient.StatusReturns(&smclientTypes.Operation{ ID: "1234", Type: smClientTypes.CREATE, @@ -314,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, @@ -327,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() @@ -355,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, "") @@ -387,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)) @@ -397,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)) }) @@ -502,20 +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) + }) }) - 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) + 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) + + }) }) }) @@ -569,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()) @@ -580,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() { @@ -729,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 @@ -760,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) @@ -783,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) @@ -804,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)) @@ -822,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)) @@ -833,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)) @@ -850,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() { @@ -898,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()) }) @@ -906,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() { @@ -968,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) }) @@ -1007,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, diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 8ba241cf..3cfdddcc 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 * 10 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())}}) @@ -257,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()} diff --git a/internal/config/config.go b/internal/config/config.go index f162bc8c..ca2f1ee7 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 } 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) })