From 9e1c86edb903b4f66cc7acfbe1500bfc9d827d6f Mon Sep 17 00:00:00 2001 From: kerenlahav <45451976+kerenlahav@users.noreply.github.com> Date: Tue, 30 Apr 2024 10:45:32 +0300 Subject: [PATCH] Bug fix - secret template cannot be parsed error (#424) * Bug fix - secret template cannot be parsed error * . * fix tests * fix tests --------- Co-authored-by: Naama Solomon <52195888+I065450@users.noreply.github.com> --- api/v1/servicebinding_validating_webhook.go | 30 --------- .../servicebinding_validating_webhook_test.go | 62 +++---------------- controllers/servicebinding_controller_test.go | 54 +++++++++++----- 3 files changed, 46 insertions(+), 100 deletions(-) diff --git a/api/v1/servicebinding_validating_webhook.go b/api/v1/servicebinding_validating_webhook.go index 8f68b1b1..439ae64c 100644 --- a/api/v1/servicebinding_validating_webhook.go +++ b/api/v1/servicebinding_validating_webhook.go @@ -21,11 +21,7 @@ import ( "reflect" "time" - "github.com/SAP/sap-btp-service-operator/api/common/utils" - "github.com/SAP/sap-btp-service-operator/api/common" - "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -35,7 +31,6 @@ import ( // log is for logging in this package. var servicebindinglog = logf.Log.WithName("servicebinding-resource") -var secretTemplateError = "spec.secretTemplate is invalid" func (sb *ServiceBinding) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). @@ -58,11 +53,6 @@ func (sb *ServiceBinding) ValidateCreate() (admission.Warnings, error) { return nil, err } } - if sb.Spec.SecretTemplate != "" { - if err := sb.validateSecretTemplate(); err != nil { - return nil, err - } - } return nil, nil } @@ -93,11 +83,6 @@ func (sb *ServiceBinding) ValidateUpdate(old runtime.Object) (admission.Warnings if specChanged && (sb.Status.BindingID != "" || isStale) { return nil, fmt.Errorf("updating service bindings is not supported") } - if sb.Spec.SecretTemplate != "" { - if err := sb.validateSecretTemplate(); err != nil { - return nil, err - } - } return nil, nil } @@ -143,18 +128,3 @@ func (sb *ServiceBinding) validateCredRotatingConfig() error { return nil } - -func (sb *ServiceBinding) validateSecretTemplate() error { - servicebindinglog.Info("validate specified secretTemplate") - x := make(map[string]interface{}) - y := make(map[string]string) - parameters := utils.GetSecretDataForTemplate(x, y) - - templateName := fmt.Sprintf("%s/%s", sb.Namespace, sb.Name) - _, err := utils.CreateSecretFromTemplate(templateName, sb.Spec.SecretTemplate, "missingkey=zero", parameters) - if err != nil { - servicebindinglog.Error(err, "failed to create secret from template") - return errors.Wrap(err, secretTemplateError) - } - return nil -} diff --git a/api/v1/servicebinding_validating_webhook_test.go b/api/v1/servicebinding_validating_webhook_test.go index 0dc34b93..0db74c5d 100644 --- a/api/v1/servicebinding_validating_webhook_test.go +++ b/api/v1/servicebinding_validating_webhook_test.go @@ -20,21 +20,6 @@ var _ = Describe("Service Binding Webhook Test", func() { _, err := binding.ValidateCreate() Expect(err).ToNot(HaveOccurred()) }) - It("should succeed if secretTemplate can be parsed", func() { - binding.Spec.SecretTemplate = dedent.Dedent( - `apiVersion: v1 -kind: Secret -metadata: - labels: - instance_plan: "a-new-plan-name" - annotations: - instance_name: "a-new-instance-name" -stringData: - newKey2: {{ .credentials.secret_key }}`) - _, err := binding.ValidateCreate() - - Expect(err).ToNot(HaveOccurred()) - }) It("should succeed if using allowed sprig function", func() { //write test for secretTemplateError binding.Spec.SecretTemplate = dedent.Dedent(` @@ -45,45 +30,6 @@ stringData: _, err := binding.ValidateCreate() Expect(err).ToNot(HaveOccurred()) }) - It("should fail if can't secretTemplate can be parsed", func() { - //write test for secretTemplateError - binding.Spec.SecretTemplate = "{{" - _, err := binding.ValidateCreate() - Expect(err).To(HaveOccurred()) - errMsg := err.Error() - Expect(errMsg).To(ContainSubstring("unclosed action")) - }) - It("should fail if can't secretTemplate have invalid function", func() { - //write test for secretTemplateError - binding.Spec.SecretTemplate = dedent.Dedent(` - apiVersion: v1 - kind: Secret - stringData: - secretKey: {{ .secretValue | env }}`) - _, err := binding.ValidateCreate() - Expect(err).To(HaveOccurred()) - errMsg := err.Error() - Expect(errMsg).To(ContainSubstring(" function \"env\" not defined")) - }) - It("should fail if template contains metadata.name", func() { - //write test for secretTemplateError - binding.Spec.SecretTemplate = dedent.Dedent( - `apiVersion: v1 -kind: Secret -metadata: - name: "a-new-secret" - labels: - instance_plan: "a-new-plan-name" - annotations: - instance_name: "a-new-instance-name" -stringData: - newKey2: {{ .credentials.secret_key }}`) - - _, err := binding.ValidateCreate() - Expect(err).To(HaveOccurred()) - errMsg := err.Error() - Expect(errMsg).To(ContainSubstring("the Secret template is invalid: Secret's metadata field")) - }) }) Context("Validate update of spec before binding is created (failure recovery)", func() { @@ -276,6 +222,14 @@ stringData: }) }) + + When("secretTemplate changed", func() { + It("should succeed", func() { + newBinding.Spec.SecretTemplate = "new-template" + _, err := newBinding.ValidateUpdate(binding) + Expect(err).ToNot(HaveOccurred()) + }) + }) }) When("Metadata changed", func() { diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index 5def7685..f01d6ddd 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -77,8 +77,8 @@ var _ = Describe("ServiceBinding controller", func() { return createBindingWithoutAssertionsAndWait(ctx, name, namespace, instanceName, instanceNamespace, externalName, secretTemplate, false) } - createBindingWithTransitError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string) { - binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, "") + createBindingWithTransitError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string, secretTemplate string) { + binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, secretTemplate) if err != nil { Expect(err.Error()).To(ContainSubstring(failureMessage)) } else { @@ -86,8 +86,8 @@ var _ = Describe("ServiceBinding controller", func() { } } - createBindingWithError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string) { - binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, "") + createBindingWithError := func(ctx context.Context, name, namespace, instanceName, externalName, failureMessage string, secretTemplate string) { + binding, err := createBindingWithoutAssertions(ctx, name, namespace, instanceName, "", externalName, secretTemplate) if err != nil { Expect(err.Error()).To(ContainSubstring(failureMessage)) } else { @@ -212,7 +212,7 @@ var _ = Describe("ServiceBinding controller", func() { When("service instance name is not provided", func() { It("should fail", func() { createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, "", "", - "spec.serviceInstanceName in body should be at least 1 chars long") + "spec.serviceInstanceName in body should be at least 1 chars long", "") }) }) @@ -396,8 +396,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should fail with the error returned from SM", func() { - createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", - errorMessage) + createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage, "") }) }) @@ -428,7 +427,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should fail", func() { - createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage) + createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage, "") }) }) @@ -466,7 +465,7 @@ var _ = Describe("ServiceBinding controller", func() { }) It("should detect the error as non-transient and fail", func() { - createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage) + createBindingWithTransitError(ctx, bindingName, bindingTestNamespace, instanceName, "binding-external-name", errorMessage, "") }) }) @@ -504,7 +503,7 @@ var _ = Describe("ServiceBinding controller", func() { State: smClientTypes.FAILED, Description: errorMessage, }, nil) - createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "existing-name", errorMessage) + createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "existing-name", errorMessage, "") }) }) @@ -651,8 +650,7 @@ stringData: kind: Secret metadata: name: my-secret-name`) - _, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate) - Expect(err.Error()).To(ContainSubstring("the Secret template is invalid: Secret's metadata field")) + createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "", "the Secret template is invalid: Secret's metadata field", secretTemplate) }) It("should fail to create the secret if wrong template key in the spec.secretTemplate is provided", func() { ctx := context.Background() @@ -678,10 +676,7 @@ stringData: secretTemplate := dedent.Dedent(` apiVersion: v1 kind: Pod`) - - _, err := createBindingWithoutAssertions(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("but needs to be of kind 'Secret'")) + createBindingWithError(ctx, bindingName, bindingTestNamespace, instanceName, "", "but needs to be of kind 'Secret'", secretTemplate) }) It("should succeed to create the secret- empty data", func() { ctx := context.Background() @@ -750,6 +745,33 @@ metadata: } validateSecretMetadata(bindingSecret, credentialProperties) }) + It("should succeed to create the secret, with depth key", func() { + + fakeClient.BindReturns(&smClientTypes.ServiceBinding{ID: fakeBindingID, Credentials: json.RawMessage(`{ "auth":{ "basic":{ "password":"secret_value","userName":"name"}},"url":"yourluckyday.com"}`)}, "", nil) + + ctx := context.Background() + secretTemplate := dedent.Dedent( + `apiVersion: v1 +kind: Secret +metadata: + labels: + instance_plan: {{ .instance.plan }} + annotations: + instance_name: {{ .instance.instance_name }} +stringData: + newKey: {{ .credentials.auth.basic.password }} + tags: {{ .instance.tags }}`) + + createdBinding, err := createBindingWithoutAssertionsAndWait(ctx, bindingName, bindingTestNamespace, instanceName, "", "", secretTemplate, true) + Expect(err).ToNot(HaveOccurred()) + Expect(isResourceReady(createdBinding)).To(BeTrue()) + By("Verify binding secret created") + bindingSecret := getSecret(ctx, createdBinding.Spec.SecretName, createdBinding.Namespace, true) + validateSecretData(bindingSecret, "newKey", "secret_value") + validateSecretData(bindingSecret, "tags", strings.Join(mergeInstanceTags(createdInstance.Status.Tags, createdInstance.Spec.CustomTags), ",")) + Expect(bindingSecret.Labels["instance_plan"]).To(Equal("a-plan-name")) + Expect(bindingSecret.Annotations["instance_name"]).To(Equal(instanceExternalName)) + }) }) })