BUG FIX - delete rotated bindings when root binding is deleted (#313)

api/v1/servicebinding_validating_webhook.go

@@ -68,22 +68,35 @@ func (sb *ServiceBinding) ValidateUpdate(old runtime.Object) (admission.Warnings
 		}
 	}
 
-	specChanged := sb.specChanged(old)
+	oldBinding := old.(*ServiceBinding)
 	isStale := false
-	if sb.Labels != nil {
-		if _, ok := sb.Labels[api.StaleBindingIDLabel]; ok {
+	if oldBinding.Labels != nil {
+		if _, ok := oldBinding.Labels[api.StaleBindingIDLabel]; ok {
+			if sb.Spec.CredRotationPolicy.Enabled {
+				return nil, fmt.Errorf("enabling cred rotation for rotated binding is not allowed")
+			}
+			if !sb.validateRotationLabels(oldBinding) {
+				return nil, fmt.Errorf("modifying rotation labels is not allowed")
+			}
 			isStale = true
 		}
 	}
 
+	specChanged := sb.specChanged(oldBinding)
 	if specChanged && (sb.Status.BindingID != "" || isStale) {
 		return nil, fmt.Errorf("updating service bindings is not supported")
 	}
 	return nil, nil
 }
 
-func (sb *ServiceBinding) specChanged(old runtime.Object) bool {
-	oldBinding := old.(*ServiceBinding)
+func (sb *ServiceBinding) validateRotationLabels(old *ServiceBinding) bool {
+	if sb.Labels[api.StaleBindingIDLabel] != old.Labels[api.StaleBindingIDLabel] {
+		return false
+	}
+	return sb.Labels[api.StaleBindingRotationOfLabel] == old.Labels[api.StaleBindingRotationOfLabel]
+}
+
+func (sb *ServiceBinding) specChanged(oldBinding *ServiceBinding) bool {
 	oldSpec := oldBinding.Spec.DeepCopy()
 	newSpec := sb.Spec.DeepCopy()
 

api/v1/servicebinding_validating_webhook_test.go

@@ -93,6 +93,7 @@ var _ = Describe("Service Binding Webhook Test", func() {
 				})
 
 				It("should fail on update with stale label", func() {
+					binding.Labels = map[string]string{api.StaleBindingIDLabel: "true"}
 					newBinding.Spec.ParametersFrom[0].SecretKeyRef.Name = "newName"
 					newBinding.Labels = map[string]string{api.StaleBindingIDLabel: "true"}
 					_, err := newBinding.ValidateUpdate(binding)

api/v1/webhooks/servicebinding_mutating_webhook.go

@@ -5,8 +5,6 @@ import (
 	"encoding/json"
 	"net/http"
 
-	"github.com/SAP/sap-btp-service-operator/api"
-
 	servicesv1 "github.com/SAP/sap-btp-service-operator/api/v1"
 	v1admission "k8s.io/api/admission/v1"
 	v1 "k8s.io/api/authentication/v1"
@@ -40,21 +38,13 @@ func (s *ServiceBindingDefaulter) Handle(_ context.Context, req admission.Reques
 		binding.Spec.SecretName = binding.Name
 	}
 
-	if binding.Labels != nil {
-		if _, ok := binding.Labels[api.StaleBindingIDLabel]; ok {
-			if binding.Spec.CredRotationPolicy != nil {
-				binding.Spec.CredRotationPolicy.Enabled = false
-			}
-		}
-	}
-
 	if binding.Spec.CredRotationPolicy != nil {
 		if len(binding.Spec.CredRotationPolicy.RotationFrequency) == 0 {
-			binding.Spec.CredRotationPolicy.RotationFrequency = "0ns"
+			binding.Spec.CredRotationPolicy.RotationFrequency = "72h"
 		}
 
 		if len(binding.Spec.CredRotationPolicy.RotatedBindingTTL) == 0 {
-			binding.Spec.CredRotationPolicy.RotatedBindingTTL = "0ns"
+			binding.Spec.CredRotationPolicy.RotatedBindingTTL = "48h"
 		}
 	}
 

client/sm/client.go

@@ -69,8 +69,9 @@ type Client interface {
 }
 
 type ServiceManagerError struct {
-	Description string
-	StatusCode  int
+	ErrorType   string                   `json:"error,omitempty"`
+	Description string                   `json:"description,omitempty"`
+	StatusCode  int                      `json:"-"`
 	BrokerError *api.HTTPStatusCodeError `json:"broker_error,omitempty"`
 }
 
@@ -240,8 +241,7 @@ func (client *serviceManagerClient) RenameBinding(id, newName, newK8SName string
 		"labels": []*types.LabelChange{
 			{
 				Key:       k8sNameLabel,
-				Operation: types.AddLabelValuesOperation,
-				Values:    []string{newK8SName},
+				Operation: types.RemoveLabelOperation,
 			},
 		},
 	}
@@ -251,6 +251,22 @@ func (client *serviceManagerClient) RenameBinding(id, newName, newK8SName string
 	if err != nil {
 		return nil, err
 	}
+
+	// due to sm issue (no "replace" label and remove+add not supported in the same request)
+	// adding the new _k8sname value in another request
+	addLabelRequest := map[string]interface{}{
+		"labels": []*types.LabelChange{
+			{
+				Key:       k8sNameLabel,
+				Operation: types.AddLabelOperation,
+				Values:    []string{newK8SName},
+			},
+		},
+	}
+	_, err = client.update(addLabelRequest, types.ServiceBindingsURL, id, nil, "", &result)
+	if err != nil {
+		return nil, err
+	}
 	return result, err
 }
 
@@ -535,8 +551,7 @@ func handleResponseError(response *http.Response) error {
 	}
 
 	smError := &ServiceManagerError{
-		StatusCode:  response.StatusCode,
-		Description: "",
+		StatusCode: response.StatusCode,
 	}
 	_ = json.Unmarshal(body, &smError)
 

client/sm/types/types.go

@@ -1,6 +1,7 @@
 package types
 
-const AddLabelValuesOperation = "add_values"
+const AddLabelOperation = "add"
+const RemoveLabelOperation = "remove"
 
 type Labels map[string][]string
 

controllers/base_controller.go

@@ -2,10 +2,8 @@ package controllers
 
 import (
 	"context"
-	"net/http"
-	"strings"
-
 	"fmt"
+	"net/http"
 
 	"github.com/SAP/sap-btp-service-operator/api"
 	"github.com/SAP/sap-btp-service-operator/client/sm"
@@ -307,30 +305,27 @@ func isTransientError(ctx context.Context, err error) (bool, string) {
 		return false, err.Error()
 	}
 
+	log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode))
 	statusCode := smError.StatusCode
 	errMsg := smError.Error()
-	if isBrokerErrorExist(smError) {
+	if smError.BrokerError != nil {
 		log.Info(fmt.Sprintf("Broker returned error status code %d", smError.BrokerError.StatusCode))
 		statusCode = smError.BrokerError.StatusCode
 		errMsg = smError.BrokerError.Error()
-	} else {
-		log.Info(fmt.Sprintf("SM returned error status code %d", smError.StatusCode))
 	}
-	return isTransientStatusCode(statusCode, errMsg), errMsg
+
+	return isConcurrentOperationError(smError) || isTransientStatusCode(statusCode), errMsg
 }
 
-func isTransientStatusCode(StatusCode int, description string) bool {
-	if StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable ||
-		StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound || StatusCode == http.StatusBadGateway {
-		return true
-	}
-	/* In case of 422 we only want to identify the error as transient if it is a concurrent operation error,
-	   it may be also un-processable entity which is non-transient */
-	return StatusCode == http.StatusUnprocessableEntity && strings.Contains(description, "Another concurrent operation in progress for this resource")
+func isConcurrentOperationError(smError *sm.ServiceManagerError) bool {
+	// service manager returns 422 for resources that have another operation in progress
+	// in this case 422 status code is transient
+	return smError.StatusCode == http.StatusUnprocessableEntity && smError.ErrorType == "ConcurrentOperationInProgress"
 }
 
-func isBrokerErrorExist(smError *sm.ServiceManagerError) bool {
-	return smError.BrokerError != nil && smError.BrokerError.StatusCode != 0
+func isTransientStatusCode(StatusCode int) bool {
+	return StatusCode == http.StatusTooManyRequests || StatusCode == http.StatusServiceUnavailable ||
+		StatusCode == http.StatusGatewayTimeout || StatusCode == http.StatusNotFound || StatusCode == http.StatusBadGateway
 }
 
 func (r *BaseReconciler) handleError(ctx context.Context, operationType smClientTypes.OperationCategory, err error, resource api.SAPBTPResource) (ctrl.Result, error) {

controllers/servicebinding_controller.go

@@ -159,8 +159,11 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return ctrl.Result{Requeue: true, RequeueAfter: r.Config.PollInterval}, nil
 	}
 
-	if !bindingAlreadyOwnedByInstance(serviceInstance, serviceBinding) {
-		return ctrl.Result{}, r.SetOwner(ctx, serviceInstance, serviceBinding)
+	//set owner instance only for original bindings (not rotated)
+	if serviceBinding.Labels == nil || len(serviceBinding.Labels[api.StaleBindingIDLabel]) == 0 {
+		if !bindingAlreadyOwnedByInstance(serviceInstance, serviceBinding) {
+			return ctrl.Result{}, r.SetOwner(ctx, serviceInstance, serviceBinding)
+		}
 	}
 
 	if serviceBinding.Status.BindingID == "" {
@@ -756,7 +759,7 @@ func (r *ServiceBindingReconciler) singleKeyMap(credentialsMap map[string][]byte
 }
 
 func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, smClient sm.Client, binding *servicesv1.ServiceBinding) error {
-	oldSuffix := "-" + RandStringRunes(6)
+	suffix := "-" + RandStringRunes(6)
 	log := GetLogger(ctx)
 	if binding.Annotations != nil {
 		if _, ok := binding.Annotations[api.ForceRotateAnnotation]; ok {
@@ -769,9 +772,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, smClie
 		}
 	}
 
-	conditions := binding.GetConditions()
-	credInProgressCondition := meta.FindStatusCondition(conditions, api.ConditionCredRotationInProgress)
-
+	credInProgressCondition := meta.FindStatusCondition(binding.GetConditions(), api.ConditionCredRotationInProgress)
 	if credInProgressCondition.Reason == CredRotating {
 		if len(binding.Status.BindingID) > 0 && binding.Status.Ready == metav1.ConditionTrue {
 			log.Info("Credentials rotation - finished successfully")
@@ -800,7 +801,7 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, smClie
 	if len(bindings.Items) == 0 {
 		// rename current binding
 		log.Info("Credentials rotation - renaming binding to old in SM", "current", binding.Spec.ExternalName)
-		if _, errRenaming := smClient.RenameBinding(binding.Status.BindingID, binding.Spec.ExternalName+oldSuffix, binding.Name+oldSuffix); errRenaming != nil {
+		if _, errRenaming := smClient.RenameBinding(binding.Status.BindingID, binding.Spec.ExternalName+suffix, binding.Name+suffix); errRenaming != nil {
 			log.Error(errRenaming, "Credentials rotation - failed renaming binding to old in SM", "binding", binding.Spec.ExternalName)
 			setCredRotationInProgressConditions(CredPreparing, errRenaming.Error(), binding)
 			if errStatus := r.updateStatus(ctx, binding); errStatus != nil {
@@ -809,8 +810,8 @@ func (r *ServiceBindingReconciler) rotateCredentials(ctx context.Context, smClie
 			return errRenaming
 		}
 
-		log.Info("Credentials rotation - backing up old binding in K8S", "name", binding.Name+oldSuffix)
-		if err := r.createOldBinding(ctx, oldSuffix, binding); err != nil {
+		log.Info("Credentials rotation - backing up old binding in K8S", "name", binding.Name+suffix)
+		if err := r.createOldBinding(ctx, suffix, binding); err != nil {
 			log.Error(err, "Credentials rotation - failed to back up old binding in K8S")
 
 			setCredRotationInProgressConditions(CredPreparing, err.Error(), binding)
@@ -862,6 +863,10 @@ func (r *ServiceBindingReconciler) initCredRotationIfRequired(binding *servicesv
 
 func (r *ServiceBindingReconciler) createOldBinding(ctx context.Context, suffix string, binding *servicesv1.ServiceBinding) error {
 	oldBinding := newBindingObject(binding.Name+suffix, binding.Namespace)
+	err := controllerutil.SetControllerReference(binding, oldBinding, r.Scheme)
+	if err != nil {
+		return err
+	}
 	oldBinding.Labels = map[string]string{
 		api.StaleBindingIDLabel:         binding.Status.BindingID,
 		api.StaleBindingRotationOfLabel: binding.Name,

controllers/servicebinding_controller_test.go

@@ -1107,7 +1107,9 @@ var _ = Describe("ServiceBinding controller", func() {
 					api.StaleBindingRotationOfLabel: createdBinding.Name,
 				}
 				staleBinding.Spec.CredRotationPolicy = &v1.CredentialsRotationPolicy{
-					Enabled: false,
+					Enabled:           false,
+					RotatedBindingTTL: "0ns",
+					RotationFrequency: "0ns",
 				}
 				Expect(k8sClient.Create(ctx, staleBinding)).To(Succeed())
 				Eventually(func() bool {
@@ -1143,7 +1145,9 @@ var _ = Describe("ServiceBinding controller", func() {
 					api.StaleBindingRotationOfLabel: failedBinding.Name,
 				}
 				staleBinding.Spec.CredRotationPolicy = &v1.CredentialsRotationPolicy{
-					Enabled: false,
+					Enabled:           false,
+					RotatedBindingTTL: "0ns",
+					RotationFrequency: "0ns",
 				}
 				Expect(k8sClient.Create(ctx, staleBinding)).To(Succeed())
 
@@ -1170,7 +1174,9 @@ var _ = Describe("ServiceBinding controller", func() {
 					api.StaleBindingIDLabel: createdBinding.Status.BindingID,
 				}
 				staleBinding.Spec.CredRotationPolicy = &v1.CredentialsRotationPolicy{
-					Enabled: false,
+					Enabled:           false,
+					RotatedBindingTTL: "0ns",
+					RotationFrequency: "0ns",
 				}
 				Expect(k8sClient.Create(ctx, staleBinding)).To(Succeed())
 				Eventually(func() bool {

controllers/serviceinstance_controller.go

@@ -548,7 +548,7 @@ func updateRequired(serviceInstance *servicesv1.ServiceInstance) bool {
 	}
 
 	cond := meta.FindStatusCondition(serviceInstance.Status.Conditions, api.ConditionSucceeded)
-	if cond != nil && cond.Reason == UpdateInProgress {
+	if cond != nil && cond.Reason == UpdateInProgress { //in case of transient error occurred
 		return true
 	}