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

api/common/common.go

@@ -11,16 +11,14 @@ 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"
-	IgnoreNonTransientErrorTimestampAnnotation string         = "services.cloud.sap.com/ignoreNonTransientErrorTimestamp"
+	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"
 )
 
 type HTTPStatusCodeError struct {
@@ -83,4 +81,5 @@ type SAPBTPResource interface {
 	GetReady() metav1.ConditionStatus
 	GetAnnotations() map[string]string
 	SetAnnotations(map[string]string)
+	SetFirstErrorTimestamp(*metav1.Time)
 }

api/v1/servicebinding_types.go

@@ -195,6 +195,8 @@ func (sb *ServiceBinding) GetAnnotations() map[string]string {
 func (sb *ServiceBinding) SetAnnotations(annotations map[string]string) {
 	sb.Annotations = annotations
 }
+func (sb *ServiceBinding) SetFirstErrorTimestamp(_ *metav1.Time) {
+}
 
 // +kubebuilder:object:root=true
 

api/v1/servicebinding_types_test.go

@@ -100,7 +100,7 @@ var _ = Describe("Service Binding Type Test", func() {
 
 	It("should update annotation", func() {
 		annotation := map[string]string{
-			common.IgnoreNonTransientErrorAnnotation: "true",
+			"annotation": "true",
 		}
 		binding.SetAnnotations(annotation)
 		Expect(binding.GetAnnotations()).To(Equal(annotation))

api/v1/servicebinding_validating_webhook.go

@@ -50,9 +50,6 @@ 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
@@ -64,9 +61,6 @@ 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
@@ -131,12 +125,3 @@ func (sb *ServiceBinding) validateCredRotatingConfig() error {
 
 	return nil
 }
-
-func (sb *ServiceBinding) validateAnnotations() error {
-	if sb.Annotations != nil {
-		if _, ok := sb.Annotations[common.IgnoreNonTransientErrorAnnotation]; ok {
-			return fmt.Errorf(AnnotationNotSupportedError, common.IgnoreNonTransientErrorAnnotation)
-		}
-	}
-	return nil
-}

api/v1/servicebinding_validating_webhook_test.go

@@ -1,7 +1,6 @@
 package v1
 
 import (
-	"fmt"
 	"github.com/SAP/sap-btp-service-operator/api/common"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -20,15 +19,6 @@ var _ = Describe("Service Binding Webhook Test", func() {
 				_, err := binding.ValidateCreate()
 				Expect(err).ToNot(HaveOccurred())
 			})
-			It("should failed", func() {
-				binding.Annotations = map[string]string{
-					common.IgnoreNonTransientErrorAnnotation: "false",
-				}
-				_, err := binding.ValidateCreate()
-				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(AnnotationNotSupportedError, common.IgnoreNonTransientErrorAnnotation)))
-
-			})
 		})
 
 		Context("Validate update of spec before binding is created (failure recovery)", func() {
@@ -119,18 +109,6 @@ var _ = Describe("Service Binding Webhook Test", func() {
 					Expect(err).ToNot(HaveOccurred())
 				})
 			})
-			When("Annotation changed", func() {
-				It("should fail", func() {
-					newBinding.Annotations = map[string]string{
-						common.IgnoreNonTransientErrorAnnotation: "false",
-					}
-					_, err := newBinding.ValidateUpdate(binding)
-					Expect(err).To(HaveOccurred())
-					Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(AnnotationNotSupportedError, common.IgnoreNonTransientErrorAnnotation)))
-
-				})
-			})
-
 		})
 
 		Context("Validate update of spec after binding is created", func() {

api/v1/serviceinstance_types.go

@@ -119,6 +119,10 @@ type ServiceInstanceStatus struct {
 
 	// The subaccount id of the service instance
 	SubaccountID string `json:"subaccountID,omitempty"`
+
+	// The first error timestamp - used to stop retrying after a while
+	// +optional
+	FirstErrorTimestamp *metav1.Time `json:"firstErrorTimestamp,omitempty"`
 }
 
 // +kubebuilder:object:root=true
@@ -130,6 +134,7 @@ type ServiceInstanceStatus struct {
 // +kubebuilder:printcolumn:JSONPath=".spec.dataCenter",name="dataCenter",type=string
 // +kubebuilder:printcolumn:JSONPath=".status.conditions[0].reason",name="Status",type=string
 // +kubebuilder:printcolumn:JSONPath=".status.ready",name="Ready",type=string
+// +kubebuilder:printcolumn:JSONPath=".status.firstErrorTimestamp",name="FirstErrorTimestamp",type=date
 // +kubebuilder:printcolumn:JSONPath=".metadata.creationTimestamp",name="Age",type=date
 // +kubebuilder:printcolumn:JSONPath=".status.instanceID",name="ID",type=string,priority=1
 // +kubebuilder:printcolumn:JSONPath=".status.conditions[0].message",name="Message",type=string,priority=1
@@ -192,6 +197,9 @@ func (si *ServiceInstance) GetAnnotations() map[string]string {
 func (si *ServiceInstance) SetAnnotations(annotations map[string]string) {
 	si.Annotations = annotations
 }
+func (si *ServiceInstance) SetFirstErrorTimestamp(time *metav1.Time) {
+	si.Status.FirstErrorTimestamp = time
+}
 
 // +kubebuilder:object:root=true
 

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() {
@@ -17,6 +18,7 @@ var _ = Describe("Service Instance Type Test", func() {
 		lastOpCondition := metav1.Condition{Type: common.ConditionSucceeded, Status: metav1.ConditionTrue, Reason: "reason", Message: "message"}
 		meta.SetStatusCondition(&conditions, lastOpCondition)
 		instance.SetConditions(conditions)
+		instance.SetFirstErrorTimestamp(&metav1.Time{Time: time.Now()})
 	})
 
 	It("should clone correctly", func() {
@@ -42,6 +44,7 @@ var _ = Describe("Service Instance Type Test", func() {
 		clonedStatus := &ServiceInstanceStatus{}
 		instance.Status.DeepCopyInto(clonedStatus)
 		Expect(&instance.Status).To(Equal(clonedStatus))
+		Expect(instance.Status.FirstErrorTimestamp).To(Equal(clonedStatus.FirstErrorTimestamp))
 
 		clonedSpec := &ServiceInstanceSpec{}
 		instance.Spec.DeepCopyInto(clonedSpec)
@@ -124,9 +127,20 @@ var _ = Describe("Service Instance Type Test", func() {
 
 	It("should update annotation", func() {
 		annotation := map[string]string{
-			common.IgnoreNonTransientErrorAnnotation: "true",
+			"annotation": "true",
 		}
 		instance.SetAnnotations(annotation)
 		Expect(instance.GetAnnotations()).To(Equal(annotation))
 	})
+
+	It("should set first error timestamp", func() {
+		now := time.Now()
+		instance.SetFirstErrorTimestamp(&metav1.Time{Time: now})
+		Expect(instance.Status.FirstErrorTimestamp.Time).To(Equal(now))
+	})
+
+	It("should set first error timestamp", func() {
+		instance.SetFirstErrorTimestamp(nil)
+		Expect(instance.Status.FirstErrorTimestamp).To(BeNil())
+	})
 })

api/v1/serviceinstance_validating_webhook.go

@@ -19,7 +19,6 @@ package v1
 import (
 	"fmt"
 	"strings"
-	"time"
 
 	"github.com/SAP/sap-btp-service-operator/api/common"
 
@@ -30,11 +29,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
-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).
@@ -49,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()
+	return nil, nil
 }
 
 func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) {
@@ -59,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()
+	return nil, nil
 }
 
 func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) {
@@ -72,20 +66,3 @@ 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[common.IgnoreNonTransientErrorAnnotation]) > 0 {
-		serviceinstancelog.Info(fmt.Sprintf("%s annotation exist, validating %s annotation", common.IgnoreNonTransientErrorAnnotation, common.IgnoreNonTransientErrorTimestampAnnotation))
-		annotationTime, err := time.Parse(time.RFC3339, si.Annotations[common.IgnoreNonTransientErrorTimestampAnnotation])
-		if err != nil {
-			serviceinstancelog.Error(err, fmt.Sprintf("failed to parse %s", common.IgnoreNonTransientErrorTimestampAnnotation))
-			return fmt.Errorf(annotationNotValidTimestampError, common.IgnoreNonTransientErrorTimestampAnnotation)
-		}
-
-		if time.Since(annotationTime) < 0 {
-			return fmt.Errorf(annotationInFutureError, common.IgnoreNonTransientErrorTimestampAnnotation)
-		}
-	}
-
-	return nil
-}

api/v1/serviceinstance_validating_webhook_test.go

@@ -1,11 +1,9 @@
 package v1
 
 import (
-	"fmt"
 	"github.com/SAP/sap-btp-service-operator/api/common"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	"time"
 )
 
 var _ = Describe("Service Instance Webhook Test", func() {
@@ -26,27 +24,6 @@ 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{
-					common.IgnoreNonTransientErrorAnnotation:          "true",
-					common.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{
-					common.IgnoreNonTransientErrorAnnotation:          "true",
-					common.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339),
-				}
-				_, err := instance.ValidateUpdate(instance)
-				Expect(err).To(HaveOccurred())
-			})
-		})
 	})
 
 	Context("Validate Delete", func() {
@@ -76,94 +53,5 @@ 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{
-						common.IgnoreNonTransientErrorAnnotation:          "true",
-						common.IgnoreNonTransientErrorTimestampAnnotation: "true",
-					}
-					_, err := instance.ValidateCreate()
-					Expect(err).To(HaveOccurred())
-					Expect(err.Error()).To(ContainSubstring(fmt.Sprintf(annotationNotValidTimestampError, common.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{
-						common.IgnoreNonTransientErrorAnnotation:          "true",
-						common.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, common.IgnoreNonTransientErrorTimestampAnnotation)))
-
-				})
-			})
-		})
-	})
-
-	Context("validateNonTransientTimestampAnnotation", func() {
-		It("fails when not a valid date", func() {
-			annotation := map[string]string{
-				common.IgnoreNonTransientErrorAnnotation:          "true",
-				common.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{
-				common.IgnoreNonTransientErrorAnnotation:          "true",
-				common.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, common.IgnoreNonTransientErrorTimestampAnnotation)))
-		})
-
-		It("succeeds when timestamp is valid and in the past", func() {
-			annotation := map[string]string{
-				common.IgnoreNonTransientErrorAnnotation:          "true",
-				common.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())
-		//})
 	})
 })