Skip to content

Commit

Permalink
Bug fix - secret template cannot be parsed error (#424)
Browse files Browse the repository at this point in the history 
* Bug fix - secret template cannot be parsed error

* .

* fix tests

* fix tests

---------

Co-authored-by: Naama Solomon <[email protected]>
  • Loading branch information
@kerenlahav @I065450
kerenlahav and I065450 authored Apr 30, 2024
1 parent 47d252d commit 9e1c86e
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 100 deletions.
30 changes: 0 additions & 30 deletions api/v1/servicebinding_validating_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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).
Expand All @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
62 changes: 8 additions & 54 deletions api/v1/servicebinding_validating_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(`
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down
54 changes: 38 additions & 16 deletions controllers/servicebinding_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,17 @@ 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 {
waitForResourceCondition(ctx, binding, common.ConditionSucceeded, metav1.ConditionFalse, common.CreateInProgress, failureMessage)
}
}

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 {
Expand Down Expand Up @@ -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", "")
})
})

Expand Down Expand Up @@ -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, "")
})
})

Expand Down Expand Up @@ -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, "")
})
})

Expand Down Expand Up @@ -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, "")
})
})

Expand Down Expand Up @@ -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, "")
})
})

Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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))
})
})
})

Expand Down

0 comments on commit 9e1c86e

Please sign in to comment.