From 648aafb73299f0ed57fe83762049f35cf4a9e3ca Mon Sep 17 00:00:00 2001 From: Pavel Busko <17778211+pbusko@users.noreply.github.com> Date: Wed, 22 Sep 2021 18:33:54 +0200 Subject: [PATCH] Custom ServiceInstance tags (#89) * Add custom service tags to service instance Co-authored-by: Johannes Dillmann * ensure that combination of instance tags is unique Co-authored-by: Pavel Busko * revert readme TOC Co-authored-by: Johannes Dillmann * perform ServiceInstance tags merge in service binding controller Co-authored-by: Pavel Busko * updated CRD manifest Co-authored-by: Pavel Busko Co-authored-by: Johannes Dillmann Co-authored-by: Ralf Pannemans --- README.md | 3 +- api/v1alpha1/serviceinstance_types.go | 6 +++- api/v1alpha1/zz_generated.deepcopy.go | 5 ++++ ...rvices.cloud.sap.com_serviceinstances.yaml | 7 ++++- controllers/controller_util.go | 10 +++++++ controllers/servicebinding_controller.go | 16 ++++++++-- controllers/servicebinding_controller_test.go | 30 +++++++++---------- controllers/serviceinstance_controller.go | 4 +++ .../serviceinstance_controller_test.go | 11 ++++--- 9 files changed, 66 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 29883c40..9f3dfcf2 100644 --- a/README.md +++ b/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.
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.
The possible condition types are:
- `Ready`: set to `true` if the instance is ready and usable
- `Failed`: set to `true` when an operation on the service instance fails.
In the case of failure, the details about the error are available in the condition message.
- `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`. diff --git a/api/v1alpha1/serviceinstance_types.go b/api/v1alpha1/serviceinstance_types.go index 8ee3e466..4d6446f6 100644 --- a/api/v1alpha1/serviceinstance_types.go +++ b/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 diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index ab0da81f..9dd32cef 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -254,6 +254,11 @@ func (in *ServiceInstanceSpec) DeepCopyInto(out *ServiceInstanceSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.CustomTags != nil { + in, out := &in.CustomTags, &out.CustomTags + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.UserInfo != nil { in, out := &in.UserInfo, &out.UserInfo *out = new(v1.UserInfo) diff --git a/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml b/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml index 66feec3c..09fb3679 100644 --- a/config/crd/bases/services.cloud.sap.com_serviceinstances.yaml +++ b/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 diff --git a/controllers/controller_util.go b/controllers/controller_util.go index b68e59a2..56502612 100644 --- a/controllers/controller_util.go +++ b/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 +} diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go index d86cf30f..c696771f 100644 --- a/controllers/servicebinding_controller.go +++ b/controllers/servicebinding_controller.go @@ -617,8 +617,8 @@ 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 } @@ -626,3 +626,15 @@ func (r *ServiceBindingReconciler) addInstanceInfo(ctx context.Context, binding } 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 +} diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 2571c874..f8a222d7 100644 --- a/controllers/servicebinding_controller_test.go +++ b/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()) diff --git a/controllers/serviceinstance_controller.go b/controllers/serviceinstance_controller.go index eb3328b6..aa3e0e18 100644 --- a/controllers/serviceinstance_controller.go +++ b/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 diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 474ef8c7..5762d46e 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/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)