[SAPBTPCFS-7876] Optimize handling of non-transient errors

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,16 +35,16 @@ type HTTPStatusCodeError struct {
 }
 
 func (e HTTPStatusCodeError) Error() string {
-	errorMessage := "<nil>"
-	description := "<nil>"
+	errorMessage := ""
+	description := ""
 
 	if e.ErrorMessage != nil {
 		errorMessage = *e.ErrorMessage
 	}
 	if e.Description != nil {
 		description = *e.Description
 	}
-	return fmt.Sprintf("Status: %v; ErrorMessage: %v; Description: %v; ResponseError: %v", e.StatusCode, errorMessage, description, e.ResponseError)
+	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)
 }

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

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))
+	})
 })

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
+}

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() {

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
 

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))
+	})
 })

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
+}

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())
+		//})
 	})
 })