diff --git a/api/common.go b/api/common.go index dbcd10c3..0280175a 100644 --- a/api/common.go +++ b/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 } diff --git a/api/util.go b/api/util.go new file mode 100644 index 00000000..e6c50ece --- /dev/null +++ b/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 +} diff --git a/api/v1/servicebinding_types.go b/api/v1/servicebinding_types.go index 50825676..22466678 100644 --- a/api/v1/servicebinding_types.go +++ b/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 diff --git a/api/v1/servicebinding_types_test.go b/api/v1/servicebinding_types_test.go index 328d6424..14ce9c5a 100644 --- a/api/v1/servicebinding_types_test.go +++ b/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()) - }) }) diff --git a/api/v1/serviceinstance_types.go b/api/v1/serviceinstance_types.go index fda8530a..f56b6e4a 100644 --- a/api/v1/serviceinstance_types.go +++ b/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 -} diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index b62154ec..f3fc600e 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/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()) }) }) diff --git a/api/v1/serviceinstance_validating_webhook.go b/api/v1/serviceinstance_validating_webhook.go index 8147ddbd..5a824fa9 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/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) { diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index fbf9e886..da64aaf5 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/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) + } + } + } +} diff --git a/api/v1alpha1/servicebinding_types.go b/api/v1alpha1/servicebinding_types.go index 8fc3b104..131e95f6 100644 --- a/api/v1alpha1/servicebinding_types.go +++ b/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 diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index df973d2b..04879bb2 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/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 diff --git a/config/samples/services_v1_serviceinstance.yaml b/config/samples/services_v1_serviceinstance.yaml index bd19f598..55043953 100644 --- a/config/samples/services_v1_serviceinstance.yaml +++ b/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 diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 34339bcc..1c1cf180 100644 --- a/controllers/base_controller.go +++ b/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) } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index fcd3bf28..6cb26b69 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -81,7 +81,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && - serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, r.Config.IgnoreNonTransientTimeout) { + api.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { operation := smClientTypes.CREATE if len(serviceInstance.Status.InstanceID) > 0 { operation = smClientTypes.UPDATE @@ -586,14 +586,6 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan return true } -func isInRetry(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { - conditions := serviceInstance.GetConditions() - if meta.IsStatusConditionPresentAndEqual(conditions, api.ConditionSucceeded, metav1.ConditionFalse) { - return serviceInstance.IsIgnoreNonTransientAnnotationExistAndValid(log, nonTransientTimeout) - } - return false -} - // TODO unit test func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { //update is not supported for failed instances (this can occur when instance creation was asynchronously)