Add tests and validate update

api/common.go

@@ -78,5 +78,4 @@ type SAPBTPResource interface {
 	DeepClone() SAPBTPResource
 	SetReady(metav1.ConditionStatus)
 	GetReady() metav1.ConditionStatus
-	GetSubaccountID() string
 }

api/v1/servicebinding_types.go

@@ -88,9 +88,6 @@ type ServiceBindingSpec struct {
 	// CredentialsRotationPolicy holds automatic credentials rotation configuration.
 	// +optional
 	CredRotationPolicy *CredentialsRotationPolicy `json:"credentialsRotationPolicy,omitempty"`
-
-	// The subaccount id of the service binding
-	SubaccountID string `json:"subaccountID,omitempty"`
 }
 
 // ServiceBindingStatus defines the observed state of ServiceBinding
@@ -188,10 +185,6 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) {
 	sb.Status.Ready = ready
 }
 
-func (sb *ServiceBinding) GetSubaccountID() string {
-	return sb.Spec.SubaccountID
-}
-
 // +kubebuilder:object:root=true
 
 // ServiceBindingList contains a list of ServiceBinding

api/v1/serviceinstance_types.go

@@ -116,6 +116,9 @@ type ServiceInstanceStatus struct {
 
 	// HashedSpec is the hashed spec without the shared property
 	HashedSpec string `json:"hashedSpec,omitempty"`
+
+	// The subaccount id of the service binding
+	SubaccountID string `json:"subaccountID,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1/serviceinstance_validating_webhook.go

@@ -43,6 +43,10 @@ func (si *ServiceInstance) ValidateCreate() (warnings admission.Warnings, err er
 }
 
 func (si *ServiceInstance) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) {
+	oldInstance := old.(*ServiceInstance)
+	if oldInstance.Spec.SubaccountID != si.Spec.SubaccountID {
+		return nil, fmt.Errorf("subaccountID spec field can not be changed")
+	}
 	return nil, nil
 }
 

api/v1/serviceinstance_validating_webhook_test.go

@@ -40,4 +40,17 @@ var _ = Describe("Service Instance Webhook Test", func() {
 			})
 		})
 	})
+
+	Context("Validate Update", func() {
+		When("service instance subaccountID changed", func() {
+			It("should return error from webhook", func() {
+				instance := getInstanceWithSubaccountID()
+				newInstance := getInstanceWithSubaccountID()
+				newInstance.Spec.SubaccountID = "12345"
+				_, err := newInstance.ValidateUpdate(instance)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("subaccountID spec field can not be changed"))
+			})
+		})
+	})
 })

api/v1/suite_test.go

@@ -88,3 +88,9 @@ func getInstance() *ServiceInstance {
 		Status: ServiceInstanceStatus{},
 	}
 }
+
+func getInstanceWithSubaccountID() *ServiceInstance {
+	si := getInstance()
+	si.Spec.SubaccountID = "testsubaccountid"
+	return si
+}

api/v1alpha1/servicebinding_types.go

@@ -84,9 +84,6 @@ type ServiceBindingSpec struct {
 	// CredentialsRotationPolicy holds automatic credentials rotation configuration.
 	// +optional
 	CredRotationPolicy *CredentialsRotationPolicy `json:"credentialsRotationPolicy,omitempty"`
-
-	// The subaccount id of the service binding
-	SubaccountID string `json:"subaccountID,omitempty"`
 }
 
 // ServiceBindingStatus defines the observed state of ServiceBinding
@@ -183,10 +180,6 @@ func (sb *ServiceBinding) SetReady(ready metav1.ConditionStatus) {
 	sb.Status.Ready = ready
 }
 
-func (sb *ServiceBinding) GetSubaccountID() string {
-	return sb.Spec.SubaccountID
-}
-
 // +kubebuilder:object:root=true
 
 // ServiceBindingList contains a list of ServiceBinding

api/v1alpha1/serviceinstance_types.go

@@ -78,9 +78,6 @@ type ServiceInstanceSpec struct {
 	// end-user. User-provided values for this field are not saved.
 	// +optional
 	UserInfo *v1.UserInfo `json:"userInfo,omitempty"`
-
-	// The subaccount id of the service instance
-	SubaccountID string `json:"subaccountID,omitempty"`
 }
 
 // ServiceInstanceStatus defines the observed state of ServiceInstance
@@ -173,10 +170,6 @@ func (in *ServiceInstance) SetReady(ready metav1.ConditionStatus) {
 	in.Status.Ready = ready
 }
 
-func (in *ServiceInstance) GetSubaccountID() string {
-	return in.Spec.SubaccountID
-}
-
 // +kubebuilder:object:root=true
 
 // ServiceInstanceList contains a list of ServiceInstance

config/crd/bases/services.cloud.sap.com_servicebindings.yaml

@@ -74,9 +74,6 @@ spec:
               externalName:
                 description: The name of the binding in Service Manager
                 type: string
-              subaccountID:
-                description: The subaccount id of the service binding
-                type: string
               parameters:
                 description: "Parameters for the binding. \n The Parameters field
                   is NOT secret or secured in any way and should NEVER be used to
@@ -333,9 +330,6 @@ spec:
               externalName:
                 description: The name of the binding in Service Manager
                 type: string
-              subaccountID:
-                description: The subaccount id of the service binding
-                type: string
               parameters:
                 description: "Parameters for the binding. \n The Parameters field
                   is NOT secret or secured in any way and should NEVER be used to

config/crd/bases/services.cloud.sap.com_serviceinstances.yaml

@@ -239,6 +239,9 @@ spec:
               hashedSpec:
                 description: HashedSpec is the hashed spec without the shared property
                 type: string
+              subaccountID:
+                description: SubaccountID is the service instance subaccount id
+                type: string
               instanceID:
                 description: The generated ID of the instance, will be automatically
                   filled once the instance is created

config/webhook/manifests.yaml

@@ -90,6 +90,7 @@ webhooks:
     - v1
     operations:
     - DELETE
+    - UPDATE
     resources:
     - serviceinstances
   sideEffects: None

controllers/base_controller.go

@@ -74,13 +74,13 @@ func GetLogger(ctx context.Context) logr.Logger {
 	return ctx.Value(LogKey{}).(logr.Logger)
 }
 
-func (r *BaseReconciler) getSMClient(ctx context.Context, object api.SAPBTPResource) (sm.Client, error) {
+func (r *BaseReconciler) getSMClient(ctx context.Context, object api.SAPBTPResource, subaccountId string) (sm.Client, error) {
 	if r.SMClient != nil {
 		return r.SMClient(), nil
 	}
 	log := GetLogger(ctx)
 
-	secret, err := r.SecretResolver.GetSecretForResource(ctx, object.GetNamespace(), secrets.SAPBTPOperatorSecretName, object.GetSubaccountID())
+	secret, err := r.SecretResolver.GetSecretForResource(ctx, object.GetNamespace(), secrets.SAPBTPOperatorSecretName, subaccountId)
 	if err != nil {
 		return nil, err
 	}
@@ -96,7 +96,7 @@ func (r *BaseReconciler) getSMClient(ctx context.Context, object api.SAPBTPResou
 	}
 
 	if len(cfg.ClientSecret) == 0 {
-		tls, err := r.SecretResolver.GetSecretForResource(ctx, object.GetNamespace(), secrets.SAPBTPOperatorTLSSecretName, object.GetSubaccountID())
+		tls, err := r.SecretResolver.GetSecretForResource(ctx, object.GetNamespace(), secrets.SAPBTPOperatorTLSSecretName, subaccountId)
 		if client.IgnoreNotFound(err) != nil {
 			return nil, err
 		}

controllers/servicebinding_controller.go

@@ -89,7 +89,25 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		}
 	}
 
-	smClient, err := r.getSMClient(ctx, serviceBinding)
+	log.Info("service instance name " + serviceBinding.Spec.ServiceInstanceName + " binding namespace " + serviceBinding.Namespace)
+	serviceInstance, err := r.getServiceInstanceForBinding(ctx, serviceBinding)
+	if err != nil || serviceNotUsable(serviceInstance) {
+		var instanceErr error
+		if err != nil {
+			instanceErr = fmt.Errorf("couldn't find the service instance '%s'. Error: %v", serviceBinding.Spec.ServiceInstanceName, err.Error())
+		} else {
+			instanceErr = fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName)
+		}
+
+		setBlockedCondition(instanceErr.Error(), serviceBinding)
+		if err := r.updateStatus(ctx, serviceBinding); err != nil {
+			return ctrl.Result{}, err
+		}
+
+		return ctrl.Result{}, instanceErr
+	}
+
+	smClient, err := r.getSMClient(ctx, serviceBinding, serviceInstance.Spec.SubaccountID)
 	if err != nil {
 		return r.markAsTransientError(ctx, Unknown, err.Error(), serviceBinding)
 	}
@@ -135,24 +153,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	log.Info(fmt.Sprintf("Current generation is %v and observed is %v", serviceBinding.Generation, serviceBinding.GetObservedGeneration()))
 	serviceBinding.SetObservedGeneration(serviceBinding.Generation)
 
-	log.Info("service instance name " + serviceBinding.Spec.ServiceInstanceName + " binding namespace " + serviceBinding.Namespace)
-	serviceInstance, err := r.getServiceInstanceForBinding(ctx, serviceBinding)
-	if err != nil || serviceNotUsable(serviceInstance) {
-		var instanceErr error
-		if err != nil {
-			instanceErr = fmt.Errorf("couldn't find the service instance '%s'. Error: %v", serviceBinding.Spec.ServiceInstanceName, err.Error())
-		} else {
-			instanceErr = fmt.Errorf("service instance '%s' is not usable", serviceBinding.Spec.ServiceInstanceName)
-		}
-
-		setBlockedCondition(instanceErr.Error(), serviceBinding)
-		if err := r.updateStatus(ctx, serviceBinding); err != nil {
-			return ctrl.Result{}, err
-		}
-
-		return ctrl.Result{}, instanceErr
-	}
-
 	if isInProgress(serviceInstance) {
 		log.Info(fmt.Sprintf("Service instance with k8s name %s is not ready for binding yet", serviceInstance.Name))
 

controllers/serviceinstance_controller.go

@@ -78,8 +78,9 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}
 
-	smClient, err := r.getSMClient(ctx, serviceInstance)
+	smClient, err := r.getSMClient(ctx, serviceInstance, serviceInstance.Spec.SubaccountID)
 	if err != nil {
+		fmt.Println(err)
 		return r.markAsTransientError(ctx, Unknown, err.Error(), serviceInstance)
 	}
 

controllers/serviceinstance_controller_test.go

@@ -561,6 +561,15 @@ var _ = Describe("ServiceInstance controller", func() {
 				})
 			})
 		})
+
+		Context("When subaccountID changes", func() {
+			It("should fail", func() {
+				serviceInstance.Spec.SubaccountID = "12345"
+				err := k8sClient.Update(ctx, serviceInstance)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("subaccountID spec field can not be changed"))
+			})
+		})
 	})
 
 	Describe("Delete", func() {

internal/secrets/resolver.go

@@ -33,9 +33,9 @@ func (sr *SecretResolver) GetSecretForResource(ctx context.Context, namespace, n
 	found := false
 
 	if subaccountID != "" {
-		secretName := fmt.Sprintf("%s-%s-%s", subaccountID, namespace, name)
-		sr.Log.Info(fmt.Sprintf("Searching for secret name %s", secretName))
-		secretForResource, err = sr.getSubaccountSecret(ctx, secretName)
+		secretName := fmt.Sprintf("%s-%s", subaccountID, name)
+		sr.Log.Info(fmt.Sprintf("Searching for secret name %s in management namespace", secretName))
+		secretForResource, err = sr.getSubaccountSecret(ctx, secretName, sr.ManagementNamespace)
 		if err != nil {
 			sr.Log.Error(err, "Could not fetch subaccount secret")
 			return nil, err
@@ -97,8 +97,8 @@ func (sr *SecretResolver) getClusterSecret(ctx context.Context, name string) (*v
 	return secret, err
 }
 
-func (sr *SecretResolver) getSubaccountSecret(ctx context.Context, secretName string) (*v1.Secret, error) {
+func (sr *SecretResolver) getSubaccountSecret(ctx context.Context, secretName, namespace string) (*v1.Secret, error) {
 	secret := &v1.Secret{}
-	err := sr.Client.Get(ctx, types.NamespacedName{Namespace: sr.ReleaseNamespace, Name: secretName}, secret)
+	err := sr.Client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: secretName}, secret)
 	return secret, err
 }

internal/secrets/resolver_test.go

@@ -188,7 +188,7 @@ var _ = Describe("Secrets Resolver", func() {
 	Context("Subaccount secret in management namespace", func() {
 		subaccountID := "12345"
 		BeforeEach(func() {
-			secret = createSecret(fmt.Sprintf("%s-%s", subaccountID, testNamespace), managementNamespace)
+			secret = createSecret(subaccountID, managementNamespace)
 		})
 
 		It("should resolve the secret", func() {

sapbtp-operator-charts/templates/crd.yml

@@ -73,9 +73,6 @@ spec:
               externalName:
                 description: The name of the binding in Service Manager
                 type: string
-              subaccountID:
-                description: The subaccount id of the service binding
-                type: string
               parameters:
                 description: "Parameters for the binding. \n The Parameters field
                   is NOT secret or secured in any way and should NEVER be used to
@@ -328,9 +325,6 @@ spec:
               externalName:
                 description: The name of the binding in Service Manager
                 type: string
-              subaccountID:
-                description: The subaccount id of the service binding
-                type: string
               parameters:
                 description: "Parameters for the binding. \n The Parameters field
                   is NOT secret or secured in any way and should NEVER be used to
@@ -780,6 +774,9 @@ spec:
               hashedSpec:
                 description: hashed spec
                 type: string
+              subaccountID:
+                description: SubaccountID is the service instance subaccount id
+                type: string
               tags:
                 description: Tags describing the ServiceInstance as provided in service
                   catalog, will be copied to `ServiceBinding` secret in the key called
@@ -1030,6 +1027,9 @@ spec:
               hashedSpec:
                 description: hashed spec
                 type: string
+              subaccountID:
+                description: SubaccountID is the service instance subaccount id
+                type: string
               tags:
                 description: Tags describing the ServiceInstance as provided in service
                   catalog, will be copied to `ServiceBinding` secret in the key called

sapbtp-operator-charts/templates/webhook.yml

@@ -121,6 +121,7 @@ webhooks:
           - v1
         operations:
           - DELETE
+          - UPDATE
         resources:
           - serviceinstances
     sideEffects: None