Custom ServiceInstance tags (#89)

* Add custom service tags to service instance

Co-authored-by: Johannes Dillmann <[email protected]>

* ensure that combination of instance tags is unique

Co-authored-by: Pavel Busko <[email protected]>

* revert readme TOC

Co-authored-by: Johannes Dillmann <[email protected]>

* perform ServiceInstance tags merge in service binding controller

Co-authored-by: Pavel Busko <[email protected]>

* updated CRD manifest

Co-authored-by: Pavel Busko <[email protected]>

Co-authored-by: Johannes Dillmann <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>

README.md

@@ -216,6 +216,7 @@ This feature is still under development, review, and testing.
 | externalName       | `string` | The name for the service instance in SAP BTP, defaults to the instance `metadata.name` if not specified. |
 | parameters       | `[]object` | Some services support the provisioning of additional configuration parameters during the instance creation.<br/>For the list of supported parameters, check the documentation of the particular service offering. |
 | parametersFrom | `[]object` | List of sources to populate parameters. |
+| customTags | `[]string` | List of custom tags describing the ServiceInstance, will be copied to `ServiceBinding` secret in the key called `tags`. |
 | userInfo | `object` | Contains information about the user that last modified this service instance. | 
 
 #### Status
@@ -225,7 +226,7 @@ This feature is still under development, review, and testing.
 | operationURL | `string` | The URL of the current operation performed on the service instance.  |
 | operationType   |  `string`| The type of the current operation. Possible values are CREATE, UPDATE, or DELETE. |
 | conditions       |  `[]condition`   | An array of conditions describing the status of the service instance.<br/>The possible condition types are:<br>- `Ready`: set to `true`  if the instance is ready and usable<br/>- `Failed`: set to `true` when an operation on the service instance fails.<br/> In the case of failure, the details about the error are available in the condition message.<br>- `Succeeded`: set to `true` when an operation on the service instance succeeded. In case of `false` operation considered as in progress unless `Failed` condition exists.
-| tags       |  `[]string`   | Tags describing the ServiceInstance will be copied to `ServiceBinding` secret in key called `tags`.
+| tags       |  `[]string`   | Tags describing the ServiceInstance as provided in service catalog, will be copied to `ServiceBinding` secret in the key called `tags`.
 
 
 

api/v1alpha1/serviceinstance_types.go

@@ -63,6 +63,10 @@ type ServiceInstanceSpec struct {
 	// +optional
 	ParametersFrom []ParametersFromSource `json:"parametersFrom,omitempty"`
 
+	// List of custom tags describing the ServiceInstance, will be copied to `ServiceBinding` secret in the key called `tags`.
+	// +optional
+	CustomTags []string `json:"customTags,omitempty"`
+
 	// UserInfo contains information about the user that last modified this
 	// instance. This field is set by the API server and not settable by the
 	// end-user. User-provided values for this field are not saved.
@@ -79,7 +83,7 @@ type ServiceInstanceStatus struct {
 	// +optional
 	InstanceID string `json:"instanceID,omitempty"`
 
-	// Tags describing the ServiceInstance will be copied to `ServiceBinding` secret in key called `tags`.
+	// Tags describing the ServiceInstance as provided in service catalog, will be copied to `ServiceBinding` secret in the key called `tags`.
 	Tags []string `json:"tags,omitempty"`
 
 	// URL of ongoing operation for the service instance

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

@@ -56,6 +56,11 @@ spec:
           spec:
             description: ServiceInstanceSpec defines the desired state of ServiceInstance
             properties:
+              customTags:
+                description: List of custom tags describing the ServiceInstance, will be copied to `ServiceBinding` secret in the key called `tags`.
+                items:
+                  type: string
+                type: array
               externalName:
                 description: The name of the instance in Service Manager
                 type: string
@@ -185,7 +190,7 @@ spec:
                 description: Indicates whether instance is ready for usage
                 type: string
               tags:
-                description: Tags describing the ServiceInstance will be copied to `ServiceBinding` secret in key called `tags`.
+                description: Tags describing the ServiceInstance as provided in service catalog, will be copied to `ServiceBinding` secret in the key called `tags`.
                 items:
                   type: string
                 type: array

controllers/controller_util.go

@@ -53,3 +53,13 @@ func serialize(value interface{}) ([]byte, error) {
 	}
 	return data, nil
 }
+
+func contains(slice []string, i string) bool {
+	for _, s := range slice {
+		if s == i {
+			return true
+		}
+	}
+
+	return false
+}

controllers/servicebinding_controller.go

@@ -617,12 +617,24 @@ func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding
 	credentialsMap["instance_guid"] = []byte(instance.Status.InstanceID)
 	credentialsMap["plan"] = []byte(instance.Spec.ServicePlanName)
 	credentialsMap["label"] = []byte(instance.Spec.ServiceOfferingName)
-	if len(instance.Status.Tags) > 0 {
-		tagsBytes, err := json.Marshal(instance.Status.Tags)
+	if len(instance.Status.Tags) > 0 || len(instance.Spec.CustomTags) > 0 {
+		tagsBytes, err := json.Marshal(mergeInstanceTags(instance.Status.Tags, instance.Spec.CustomTags))
 		if err != nil {
 			return err
 		}
 		credentialsMap["tags"] = tagsBytes
 	}
 	return nil
 }
+
+func mergeInstanceTags(offeringTags, customTags []string) []string {
+	var tags []string
+
+	for _, tag := range append(offeringTags, customTags...) {
+		if !contains(tags, tag) {
+			tags = append(tags, tag)
+		}
+	}
+
+	return tags
+}

controllers/servicebinding_controller_test.go

@@ -18,7 +18,6 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -151,6 +150,7 @@ var _ = Describe("ServiceBinding controller", func() {
 				ExternalName:        externalName,
 				ServicePlanName:     "a-plan-name",
 				ServiceOfferingName: "an-offering-name",
+				CustomTags:          []string{"custom-tag"},
 			},
 		}
 		Expect(k8sClient.Create(ctx, instance)).Should(Succeed())
@@ -204,7 +204,7 @@ var _ = Describe("ServiceBinding controller", func() {
 			}, timeout, interval).Should(BeTrue())
 			if len(secretName) > 0 {
 				Eventually(func() bool {
-					err := k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &v1.Secret{})
+					err := k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &corev1.Secret{})
 					return apierrors.IsNotFound(err)
 				}, timeout, interval).Should(BeTrue())
 			}
@@ -246,25 +246,25 @@ var _ = Describe("ServiceBinding controller", func() {
 			When("referenced service instance exist in another namespace", func() {
 				var otherNamespace = "other-" + bindingTestNamespace
 				BeforeEach(func() {
-					nsSpec := &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: otherNamespace}}
+					nsSpec := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: otherNamespace}}
 					err := k8sClient.Create(context.Background(), nsSpec)
 					Expect(err).ToNot(HaveOccurred())
 				})
 				It("should fail", func() {
 					createBindingWithBlockedError(context.Background(), bindingName, otherNamespace, instanceName, "",
-						fmt.Sprintf("couldn't find the service instance"))
+						"couldn't find the service instance")
 				})
 			})
 
 			When("secret name is already taken", func() {
 				ctx := context.Background()
-				var secret *v1.Secret
+				var secret *corev1.Secret
 				JustBeforeEach(func() {
-					secret = &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "mysecret", Namespace: bindingTestNamespace}}
+					secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "mysecret", Namespace: bindingTestNamespace}}
 					Expect(k8sClient.Create(ctx, secret)).Should(Succeed())
 					By("Verify secret created")
 					Eventually(func() bool {
-						err := k8sClient.Get(ctx, types.NamespacedName{Name: "mysecret", Namespace: bindingTestNamespace}, &v1.Secret{})
+						err := k8sClient.Get(ctx, types.NamespacedName{Name: "mysecret", Namespace: bindingTestNamespace}, &corev1.Secret{})
 						return err == nil
 					}, timeout, interval).Should(BeTrue())
 				})
@@ -306,10 +306,10 @@ var _ = Describe("ServiceBinding controller", func() {
 		Context("Valid parameters", func() {
 			Context("Sync", func() {
 
-				validateInstanceInfo := func(bindingSecret *v1.Secret) {
+				validateInstanceInfo := func(bindingSecret *corev1.Secret) {
 					validateSecretData(bindingSecret, "plan", `a-plan-name`)
 					validateSecretData(bindingSecret, "label", `an-offering-name`)
-					validateSecretData(bindingSecret, "tags", "[\"test\"]")
+					validateSecretData(bindingSecret, "tags", "[\"test\",\"custom-tag\"]")
 					validateSecretData(bindingSecret, "instance_name", instanceName)
 					Expect(bindingSecret.Data).To(HaveKey("instance_guid"))
 				}
@@ -526,7 +526,7 @@ var _ = Describe("ServiceBinding controller", func() {
 					Expect(k8sClient.Create(context.Background(), binding)).Should(Succeed())
 
 					Eventually(func() bool {
-						secret := &v1.Secret{}
+						secret := &corev1.Secret{}
 						err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "my-special-secret", Namespace: bindingTestNamespace}, secret)
 						return err == nil
 					}, timeout, interval).Should(BeTrue())
@@ -646,7 +646,7 @@ var _ = Describe("ServiceBinding controller", func() {
 			Eventually(func() bool {
 				err := k8sClient.Get(context.Background(), types.NamespacedName{Name: bindingName, Namespace: bindingTestNamespace}, binding)
 				if apierrors.IsNotFound(err) {
-					err := k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &v1.Secret{})
+					err := k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &corev1.Secret{})
 					return apierrors.IsNotFound(err)
 				}
 				return false
@@ -677,7 +677,7 @@ var _ = Describe("ServiceBinding controller", func() {
 			Expect(createdBinding.Status.Conditions[2].Status).To(Equal(metav1.ConditionTrue))
 			Expect(createdBinding.Status.Conditions[2].Message).To(ContainSubstring(errorMessage))
 
-			err = k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &v1.Secret{})
+			err = k8sClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: bindingTestNamespace}, &corev1.Secret{})
 			Expect(err).ToNot(HaveOccurred())
 		}
 
@@ -859,14 +859,14 @@ var _ = Describe("ServiceBinding controller", func() {
 	})
 })
 
-func validateSecretData(secret *v1.Secret, expectedKey string, expectedValue string) {
+func validateSecretData(secret *corev1.Secret, expectedKey string, expectedValue string) {
 	Expect(secret.Data).ToNot(BeNil())
 	Expect(secret.Data).To(HaveKey(expectedKey))
 	Expect(string(secret.Data[expectedKey])).To(Equal(expectedValue))
 }
 
-func getSecret(ctx context.Context, name, namespace string, failOnError bool) *v1.Secret {
-	secret := &v1.Secret{}
+func getSecret(ctx context.Context, name, namespace string, failOnError bool) *corev1.Secret {
+	secret := &corev1.Secret{}
 	err := k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, secret)
 	if failOnError {
 		Expect(err).ToNot(HaveOccurred())

controllers/serviceinstance_controller.go

@@ -211,6 +211,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient
 				serviceInstance.Status.Tags = tags
 			}
 		}
+
 		log.Info("Provision request is in progress")
 		serviceInstance.Status.OperationURL = provision.Location
 		serviceInstance.Status.OperationType = smTypes.CREATE
@@ -223,6 +224,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient
 	}
 	log.Info("Instance provisioned successfully")
 	serviceInstance.Status.InstanceID = provision.InstanceID
+
 	if len(provision.Tags) > 0 {
 		tags, err := getTags(provision.Tags)
 		if err != nil {
@@ -231,6 +233,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient
 			serviceInstance.Status.Tags = tags
 		}
 	}
+
 	serviceInstance.Status.Ready = metav1.ConditionTrue
 	setSuccessConditions(smTypes.CREATE, serviceInstance)
 	return ctrl.Result{}, r.updateStatus(ctx, serviceInstance)
@@ -363,6 +366,7 @@ func (r *ServiceInstanceReconciler) resyncInstanceStatus(smClient sm.Client, k8s
 	if len(tags) > 0 {
 		k8sInstance.Status.Tags = tags
 	}
+
 	switch smInstance.LastOperation.State {
 	case smTypes.PENDING:
 		fallthrough

controllers/serviceinstance_controller_test.go

@@ -16,7 +16,6 @@ import (
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -86,7 +85,7 @@ var _ = Describe("ServiceInstance controller", func() {
 	deleteInstance := func(ctx context.Context, instanceToDelete *v1alpha1.ServiceInstance, wait bool) {
 		err := k8sClient.Get(ctx, types.NamespacedName{Name: instanceToDelete.Name, Namespace: instanceToDelete.Namespace}, &v1alpha1.ServiceInstance{})
 		if err != nil {
-			Expect(errors.IsNotFound(err)).To(Equal(true))
+			Expect(apierrors.IsNotFound(err)).To(Equal(true))
 			return
 		}
 
@@ -96,7 +95,7 @@ var _ = Describe("ServiceInstance controller", func() {
 			Eventually(func() bool {
 				a := &v1alpha1.ServiceInstance{}
 				err := k8sClient.Get(ctx, types.NamespacedName{Name: instanceToDelete.Name, Namespace: instanceToDelete.Namespace}, a)
-				return errors.IsNotFound(err)
+				return apierrors.IsNotFound(err)
 			}, timeout, interval).Should(BeTrue())
 		}
 	}
@@ -346,7 +345,7 @@ var _ = Describe("ServiceInstance controller", func() {
 					//validate deletion
 					Eventually(func() bool {
 						err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance)
-						return errors.IsNotFound(err)
+						return apierrors.IsNotFound(err)
 					}, timeout, interval).Should(BeTrue())
 				})
 			})
@@ -467,7 +466,7 @@ var _ = Describe("ServiceInstance controller", func() {
 							//validate deletion
 							Eventually(func() bool {
 								err := k8sClient.Get(ctx, defaultLookupKey, updatedInstance)
-								return errors.IsNotFound(err)
+								return apierrors.IsNotFound(err)
 							}, timeout, interval).Should(BeTrue())
 						})
 					})
@@ -649,7 +648,7 @@ var _ = Describe("ServiceInstance controller", func() {
 					deleteInstance(ctx, serviceInstance, false)
 					Eventually(func() bool {
 						err := k8sClient.Get(ctx, defaultLookupKey, serviceInstance)
-						if errors.IsNotFound(err) {
+						if apierrors.IsNotFound(err) {
 							return false
 						}
 						return isFailed(serviceInstance)