Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: template issues #160

Merged
merged 1 commit into from
May 21, 2021
Merged

fix: template issues #160

merged 1 commit into from
May 21, 2021

Conversation

moolen
Copy link
Member

@moolen moolen commented May 20, 2021

This fixes #124 and #125.

What was the issue?
It did not work as documented.

  1. Secret.Metadata and Secret.Data was not updated after the external-secret has been changed: CreateOrUpdate does a .Get() before .Update() and modifies the object at the pointer.
  2. External Secrets templates use []byte instead of string.聽#125 Template.Data was basically unusable due to having a Data: []byte. users would have to base64 encode the template string 馃槹. This is now a simple string like it should have been from the get-go.
  3. External Secrets w/templated keys are overwritten by remote keys of the same name聽#124 Data from the secret provider could not be overridden with a template. Now the template takes precedence.

Example:

apiVersion: external-secrets.io/v1alpha1
kind: ExternalSecret
metadata:
  name: example
spec:
  ...
  target:
    name: secret-to-be-created
    template:
      data:
        firstname: "yadayada {{ .firstname | toString }}!"
  data:
  - secretKey: firstname
    remoteRef:
      key: json-string
      property: mykey

Given a json-string={"mykey": "foobar"} we get a Secret with:

data:
  firstname: "yadayada foobar!"

When we update the ExternalSecret's metadata or template then the secret will change aswell.

I will add proper template-tests later (i did the tests manually for now), i feel like we should refactor the controller_tests like the aws/secrets-manager.

@@ -59,10 +60,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
var externalSecret esv1alpha1.ExternalSecret

err := r.Get(ctx, req.NamespacedName, &externalSecret)
if err != nil {
if apierrors.IsNotFound(err) {
syncCallsTotal.With(syncCallsMetricLabels).Inc()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a user deletes the secret it should not be counted as a syncCallsError

@Sadzeih
Copy link

Sadzeih commented May 20, 2021

After reading the code, I believe this might fix an issue when only specifying a template type, but not providing data which crashes the external secrets controller.

@moolen
Copy link
Member Author

moolen commented May 20, 2021

right! That's something i didn't discover, thanks for pointing that out!
(+1 on my todo list for adding proper tests for the template feature 馃槃 )

@moolen moolen linked an issue May 21, 2021 that may be closed by this pull request
@knelasevero
Copy link
Member

Good catch! What @Sadzeih reported also happened with someone else that pinged me on slack. Was going to open a issue for it 馃槄

@knelasevero
Copy link
Member

/approve

@moolen
Copy link
Member Author

moolen commented May 21, 2021

/merge

@paul-the-alien paul-the-alien bot merged commit 62b3f12 into main May 21, 2021
@paul-the-alien paul-the-alien bot deleted the fix/tpl-api branch May 21, 2021 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants