Skip to content

Commit

Permalink
review
Browse files Browse the repository at this point in the history
  • Loading branch information
@kerenlahav
kerenlahav committed Dec 18, 2023
1 parent e8c737b commit 4e3dfad
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 69 deletions.
19 changes: 10 additions & 9 deletions api/util.go → api/utils/utils.go
View file
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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
}
Expand Down
16 changes: 9 additions & 7 deletions api/v1/serviceinstance_types_test.go
View file
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down Expand Up @@ -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"))
})
Expand All @@ -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"))
})
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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())
})
})
6 changes: 4 additions & 2 deletions api/v1/serviceinstance_validating_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
24 changes: 0 additions & 24 deletions api/v1alpha1/serviceinstance_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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())
})
})
8 changes: 5 additions & 3 deletions controllers/base_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

Expand Down
25 changes: 9 additions & 16 deletions controllers/serviceinstance_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
16 changes: 8 additions & 8 deletions controllers/serviceinstance_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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() {
Expand All @@ -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())
})
})
})
Expand All @@ -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())
})
})

Expand All @@ -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())
})
})

Expand All @@ -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())
})
})

Expand Down Expand Up @@ -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())
})
})

Expand Down Expand Up @@ -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())
})
})
})
Expand Down

0 comments on commit 4e3dfad

Please sign in to comment.