From 4e3dfad1ac64aaa3788bdce840103f723ed92cee Mon Sep 17 00:00:00 2001 From: I501080 Date: Mon, 18 Dec 2023 17:41:47 +0200 Subject: [PATCH] review --- api/{util.go => utils/utils.go} | 19 +++++++------- api/v1/serviceinstance_types_test.go | 16 ++++++------ api/v1/serviceinstance_validating_webhook.go | 6 +++-- api/v1alpha1/serviceinstance_types_test.go | 24 ------------------ controllers/base_controller.go | 8 +++--- controllers/serviceinstance_controller.go | 25 +++++++------------ .../serviceinstance_controller_test.go | 16 ++++++------ 7 files changed, 45 insertions(+), 69 deletions(-) rename api/{util.go => utils/utils.go} (68%) diff --git a/api/util.go b/api/utils/utils.go similarity index 68% rename from api/util.go rename to api/utils/utils.go index e6c50ece..3a0e6e8d 100644 --- a/api/util.go +++ b/api/utils/utils.go @@ -1,24 +1,25 @@ -package api +package utils import ( "fmt" + "github.com/SAP/sap-btp-service-operator/api" "github.com/go-logr/logr" "time" ) -func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource SAPBTPResource) error { +func ValidateNonTransientTimestampAnnotation(log logr.Logger, resource api.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 fmt.Errorf("annotation %s cannot be a future timestamp", api.IgnoreNonTransientErrorTimestampAnnotation) } return nil } -func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource SAPBTPResource, timeout time.Duration) bool { +func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource api.SAPBTPResource, timeout time.Duration) bool { sinceAnnotation, exist, _ := GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log, resource) if !exist { @@ -33,15 +34,15 @@ func IsIgnoreNonTransientAnnotationExistAndValid(log logr.Logger, resource SAPBT } -func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource SAPBTPResource) (time.Duration, bool, error) { +func GetTimeSinceIgnoreNonTransientAnnotationTimestamp(log logr.Logger, resource api.SAPBTPResource) (time.Duration, bool, error) { annotation := resource.GetAnnotations() if annotation != nil { - if _, ok := annotation[IgnoreNonTransientErrorAnnotation]; ok { + if _, ok := annotation[api.IgnoreNonTransientErrorAnnotation]; ok { log.Info("ignoreNonTransientErrorAnnotation annotation exist- checking timeout") - annotationTime, err := time.Parse(time.RFC3339, annotation[IgnoreNonTransientErrorTimestampAnnotation]) + annotationTime, err := time.Parse(time.RFC3339, annotation[api.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) + 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 } diff --git a/api/v1/serviceinstance_types_test.go b/api/v1/serviceinstance_types_test.go index f3fc600e..06cf350b 100644 --- a/api/v1/serviceinstance_types_test.go +++ b/api/v1/serviceinstance_types_test.go @@ -1,13 +1,15 @@ package v1 import ( + "time" + "github.com/SAP/sap-btp-service-operator/api" + "github.com/SAP/sap-btp-service-operator/api/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "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() { @@ -138,7 +140,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: "true", } instance.SetAnnotations(annotation) - err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) + err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("is not a valid timestamp")) }) @@ -149,7 +151,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Add(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) - err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) + err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("cannot be a future timestamp")) }) @@ -160,7 +162,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeTrue()) }) It("validate annotation exist and valid", func() { @@ -170,7 +172,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - err := api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) + err := utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, instance) Expect(err).NotTo(HaveOccurred()) }) It("validate timeout for Ignore Non Transient Error Annotation", func() { @@ -180,7 +182,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Truncate(48 * time.Hour).Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + exist := utils.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) Expect(exist).To(BeFalse()) }) It("validate annotation not exist", func() { @@ -189,7 +191,7 @@ var _ = Describe("Service Instance Type Test", func() { api.IgnoreNonTransientErrorTimestampAnnotation: time.Now().Format(time.RFC3339), } instance.SetAnnotations(annotation) - exist := api.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, instance, time.Hour) + exist := utils.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 5a824fa9..6294ebc3 100644 --- a/api/v1/serviceinstance_validating_webhook.go +++ b/api/v1/serviceinstance_validating_webhook.go @@ -20,6 +20,8 @@ import ( "fmt" "strings" + "github.com/SAP/sap-btp-service-operator/api/utils" + "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -43,7 +45,7 @@ var _ webhook.Validator = &ServiceInstance{} var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err error) { - return nil, api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) + return nil, utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) } func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) { @@ -53,7 +55,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, api.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) + return nil, utils.ValidateNonTransientTimestampAnnotation(serviceinstancelog, si) } func (si *ServiceInstance) ValidateDelete() (warnings admission.Warnings, err error) { diff --git a/api/v1alpha1/serviceinstance_types_test.go b/api/v1alpha1/serviceinstance_types_test.go index 917bfed6..536fa1d6 100644 --- a/api/v1alpha1/serviceinstance_types_test.go +++ b/api/v1alpha1/serviceinstance_types_test.go @@ -7,13 +7,10 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "time" ) var _ = Describe("Service Instance Type Test", func() { var instance *ServiceInstance - var serviceinstancelog = logf.Log.WithName("serviceinstance-resource") BeforeEach(func() { instance = getInstance() @@ -133,25 +130,4 @@ var _ = Describe("Service Instance Type Test", func() { instance.SetAnnotations(annotation) Expect(instance.GetAnnotations()).To(Equal(annotation)) }) - - It("validate timestamp annotation- not date", func() { - - annotation := map[string]string{ - api.IgnoreNonTransientErrorAnnotation: "true", - api.IgnoreNonTransientErrorTimestampAnnotation: "true", - } - instance.SetAnnotations(annotation) - exist := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) - Expect(exist).To(BeFalse()) - }) - 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 := instance.IsIgnoreNonTransientAnnotationExistAndValid(serviceinstancelog, time.Hour) - Expect(exist).To(BeTrue()) - }) }) diff --git a/controllers/base_controller.go b/controllers/base_controller.go index 1c1cf180..bad76e45 100644 --- a/controllers/base_controller.go +++ b/controllers/base_controller.go @@ -6,6 +6,8 @@ import ( "fmt" "net/http" + "github.com/SAP/sap-btp-service-operator/api/utils" + "github.com/SAP/sap-btp-service-operator/api" "github.com/SAP/sap-btp-service-operator/client/sm" smClientTypes "github.com/SAP/sap-btp-service-operator/client/sm/types" @@ -320,7 +322,7 @@ func isMarkedForDeletion(object metav1.ObjectMeta) bool { return !object.DeletionTimestamp.IsZero() } -func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log logr.Logger, resource api.SAPBTPResource) bool { +func (r *BaseReconciler) isTransientError(smError *sm.ServiceManagerError, log logr.Logger) bool { statusCode := smError.GetStatusCode() log.Info(fmt.Sprintf("SM returned error with status code %d", statusCode)) return isTransientStatusCode(statusCode) || isConcurrentOperationError(smError) @@ -348,8 +350,8 @@ 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 r.isTransientError(smError, log, resource) || - api.IsIgnoreNonTransientAnnotationExistAndValid(log, resource, r.Config.IgnoreNonTransientTimeout) { + if r.isTransientError(smError, log) || + utils.IsIgnoreNonTransientAnnotationExistAndValid(log, resource, r.Config.IgnoreNonTransientTimeout) { return r.markAsTransientError(ctx, operationType, smError.Error(), resource) } diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index 6cb26b69..3d8a292c 100644 --- a/controllers/serviceinstance_controller.go +++ b/controllers/serviceinstance_controller.go @@ -22,10 +22,8 @@ import ( "encoding/hex" "encoding/json" "fmt" + "github.com/SAP/sap-btp-service-operator/api/utils" "net/http" - "time" - - "github.com/go-logr/logr" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -81,21 +79,18 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if meta.IsStatusConditionPresentAndEqual(serviceInstance.GetConditions(), api.ConditionSucceeded, metav1.ConditionFalse) && - api.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { + utils.IsIgnoreNonTransientAnnotationExistAndValid(log, serviceInstance, r.Config.IgnoreNonTransientTimeout) { operation := smClientTypes.CREATE if len(serviceInstance.Status.InstanceID) > 0 { operation = smClientTypes.UPDATE } setInProgressConditions(ctx, operation, "", serviceInstance) - conditions := serviceInstance.GetConditions() - if len(conditions) > 0 { - meta.RemoveStatusCondition(&conditions, api.ConditionFailed) + if err := r.Status().Update(ctx, serviceInstance); err != nil { + return ctrl.Result{}, err } - serviceInstance.SetConditions(conditions) - r.Status().Update(ctx, serviceInstance) } - if isFinalState(ctx, serviceInstance, r.Config.IgnoreNonTransientTimeout) { + if isFinalState(ctx, serviceInstance) { if len(serviceInstance.Status.HashedSpec) == 0 { updateHashedSpecValue(serviceInstance) err := r.Client.Status().Update(ctx, serviceInstance) @@ -151,7 +146,7 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ } // Update - if updateRequired(serviceInstance, log, r.Config.IgnoreNonTransientTimeout) { + if updateRequired(serviceInstance) { if res, err := r.updateInstance(ctx, smClient, serviceInstance); err != nil { log.Info("got error while trying to update instance") return ctrl.Result{}, err @@ -534,7 +529,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte if smError, ok := err.(*sm.ServiceManagerError); ok { log.Info(fmt.Sprintf("SM returned error with status code %d", smError.StatusCode)) - isTransient = r.isTransientError(smError, log, object) + isTransient = r.isTransientError(smError, log) errMsg = smError.Error() if smError.StatusCode == http.StatusTooManyRequests { @@ -553,7 +548,7 @@ func (r *ServiceInstanceReconciler) handleInstanceSharingError(ctx context.Conte return ctrl.Result{Requeue: isTransient}, r.updateStatus(ctx, object) } -func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance, nonTransientTimeout time.Duration) bool { +func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstance) bool { log := GetLogger(ctx) if isMarkedForDeletion(serviceInstance.ObjectMeta) { log.Info("instance is not in final state, it is marked for deletion") @@ -586,8 +581,7 @@ func isFinalState(ctx context.Context, serviceInstance *servicesv1.ServiceInstan return true } -// TODO unit test -func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger, nonTransientTimeout time.Duration) bool { +func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool { //update is not supported for failed instances (this can occur when instance creation was asynchronously) if serviceInstance.Status.Ready != metav1.ConditionTrue { return false @@ -601,7 +595,6 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance, log logr.Logger return getSpecHash(serviceInstance) != serviceInstance.Status.HashedSpec } -// TODO unit test func sharingUpdateRequired(serviceInstance *servicesv1.ServiceInstance) bool { //relevant only for non-shared instances - sharing instance is possible only for usable instances if serviceInstance.Status.Ready != metav1.ConditionTrue { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 7c35b169..d4cbc943 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -348,7 +348,7 @@ var _ = Describe("ServiceInstance controller", func() { When("polling ends with success", func() { BeforeEach(func() { - fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{Labels: map[string][]string{"subaccount_id": []string{fakeSubaccountID}}}, nil) + fakeClient.GetInstanceByIDReturns(&smclientTypes.ServiceInstance{Labels: map[string][]string{"subaccount_id": {fakeSubaccountID}}}, nil) }) It("should update in progress condition and provision the instance successfully", func() { serviceInstance = createInstance(ctx, instanceSpec, false) @@ -1118,7 +1118,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) When("Succeeded is false", func() { @@ -1144,7 +1144,7 @@ var _ = Describe("ServiceInstance controller", func() { }, } instance.SetGeneration(1) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) }) @@ -1168,7 +1168,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1194,7 +1194,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1220,7 +1220,7 @@ var _ = Describe("ServiceInstance controller", func() { ExternalName: "name", }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1252,7 +1252,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeFalse()) + Expect(isFinalState(ctx, instance)).To(BeFalse()) }) }) @@ -1284,7 +1284,7 @@ var _ = Describe("ServiceInstance controller", func() { Shared: pointer.Bool(true), }} instance.SetGeneration(2) - Expect(isFinalState(ctx, instance, ignoreNonTransientTimeout)).To(BeTrue()) + Expect(isFinalState(ctx, instance)).To(BeTrue()) }) }) })