[SAPBTPCFS-7876] Optimize handling of non-transient errors (#394)

SAP · Feb 6, 2024 · d3d7887 · d3d7887
1 parent d938af4
commit d3d7887
Show file tree

Hide file tree

Showing 20 changed files with 98 additions and 373 deletions.
diff --git a/Makefile b/Makefile
@@ -108,3 +108,9 @@ lint-deps:
 
 helm-charts:
 	kustomize build config/default > ./sapbtp-operator-charts/templates/crd.yml
+
+precommit: goimports lint ## Run this before commiting
+
+goimports:
+	go install golang.org/x/tools/cmd/goimports@latest
+	goimports -l -w .
diff --git a/api/common/common.go b/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 {

diff --git a/api/v1/servicebinding_types_test.go b/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",
+			"key": "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
@@ -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
-}
diff --git a/api/v1/servicebinding_validating_webhook_test.go b/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() {

diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go
@@ -124,7 +124,7 @@ var _ = Describe("Service Instance Type Test", func() {
 
 	It("should update annotation", func() {
 		annotation := map[string]string{
-			common.IgnoreNonTransientErrorAnnotation: "true",
+			"key": "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
@@ -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
-}
diff --git a/api/v1/serviceinstance_validating_webhook_test.go b/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())
-		//})
 	})
 })
diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go
@@ -6,9 +6,6 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
-	"time"
-
-	"github.com/SAP/sap-btp-service-operator/api/common"
 
 	v1admission "k8s.io/api/admission/v1"
 	v1 "k8s.io/api/authentication/v1"
@@ -42,14 +39,6 @@ func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Reque
 		return admission.Errored(http.StatusInternalServerError, err)
 	}
 
-	if len(instance.Annotations) > 0 && len(instance.Annotations[common.IgnoreNonTransientErrorAnnotation]) > 0 {
-		if _, exist := instance.Annotations[common.IgnoreNonTransientErrorTimestampAnnotation]; !exist {
-			annotationValue := time.Now().Format(time.RFC3339)
-			instancelog.Info(fmt.Sprintf("%s annotation exists, adding %s annotation with value %s", common.IgnoreNonTransientErrorAnnotation, common.IgnoreNonTransientErrorTimestampAnnotation, annotationValue))
-			instance.Annotations[common.IgnoreNonTransientErrorTimestampAnnotation] = annotationValue
-		}
-	}
-
 	marshaledInstance, err := json.Marshal(instance)
 	if err != nil {
 		return admission.Errored(http.StatusInternalServerError, err)

diff --git a/api/v1alpha1/serviceinstance_types_test.go b/api/v1alpha1/serviceinstance_types_test.go
@@ -125,7 +125,7 @@ var _ = Describe("Service Instance Type Test", func() {
 
 	It("should update annotation", func() {
 		annotation := map[string]string{
-			common.IgnoreNonTransientErrorAnnotation: "true",
+			"key": "true",
 		}
 		instance.SetAnnotations(annotation)
 		Expect(instance.GetAnnotations()).To(Equal(annotation))

diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go
@@ -243,7 +243,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s
 
 	if bindErr != nil {
 		log.Error(err, "failed to create service binding", "serviceInstanceID", serviceInstance.Status.InstanceID)
-		return utils.HandleError(ctx, r.Client, smClientTypes.CREATE, bindErr, serviceBinding, false)
+		return utils.HandleError(ctx, r.Client, smClientTypes.CREATE, bindErr, serviceBinding, true)
 	}
 
 	if operationURL != "" {