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

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
 }

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

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

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

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

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

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

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

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

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

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

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