Raise error when unknown key specified in template by shuheiktgw · Pull Request #3480 · external-secrets/external-secrets

shuheiktgw · 2024-05-12T01:51:22Z

Problem Statement

While working on the template, I noticed if I specify a non-existent key in the template, the reconciliation succeeds and inserts value <no value>. This behavior seems quite error-prone since users could accidentally insert base64 encoded <no value> to their secret value.

Related Issue

N/A

Proposed Changes

Use the missingkey=error option for the go template to raise the error.

Checklist

I have read the contribution guidelines
All commits are signed with git commit --signoff
My changes have reasonable test coverage
All tests pass with make test
I ensured my PR is ready for review with make reviewable

Signed-off-by: shuheiktgw <[email protected]>

gusfcarvalho · 2024-05-13T11:31:38Z

IIRC this was set on purpose - @moolen do you happen to know what was the reason we did that on purpose?

moolen · 2024-05-13T18:43:07Z

I don't know, it doesn't ring a bell. From the looks of it i think this is fine.

gusfcarvalho · 2024-05-13T21:12:34Z

In that case, I think we need to update documentation as well in https://external-secrets.io/latest/guides/templating/:

base64encode was renamed to b64enc.
base64decode was renamed to b64dec. Any errors that occur during decoding are silenced.
fromJSON was renamed to fromJson. Any errors that occur during unmarshalling are silenced.
toJSON was renamed to toJson. Any errors that occur during marshalling are silenced.

As these are no longer silenced if I understand correctly 😄

shuheiktgw · 2024-05-13T23:38:14Z

Thanks for the feedback! IIUC, the doc states errors returned from those three functions are ignored, I personally believe we don't have to change that part since we keep ignoring those errors even after the change, but WDYT? 🤔

gusfcarvalho · 2024-05-14T15:46:28Z

wouldn't a (fromJson .).key start failing now?

shuheiktgw · 2024-05-15T00:40:33Z

wouldn't a (fromJson .).key start failing now?

Yes, that is correct! At the same time, the current statement "fromJSON was renamed to fromJson. Any errors that occur during unmarshalling are silenced." also stays true even after this change. Why don't we simply update the doc and tell users that any missing keys raise an error?

gusfcarvalho · 2024-05-15T07:50:30Z

wouldn't a (fromJson .).key start failing now?

Yes, that is correct! At the same time, the current statement "fromJSON was renamed to fromJson. Any errors that occur during unmarshalling are silenced." also stays true even after this change. Why don't we simply update the doc and tell users that any missing keys raise an error?

works for me. :) sorry for nitting

…g keys Signed-off-by: shuheiktgw <[email protected]>

sonarqubecloud · 2024-05-16T23:17:01Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.5% Duplication on New Code

See analysis details on SonarCloud

shuheiktgw · 2024-05-16T23:18:26Z

@gusfcarvalho Thanks, updated the docs!

gusfcarvalho · 2024-05-17T12:57:09Z

I cannot be more grateful by having you as maintainer 😄

qlonik · 2024-06-06T02:19:26Z

Hey, just wanted to ask about this. With this change it is now not possible to use .missing_key | default "some-default" anymore, since it throws an error when accessing the missing key. Is there any way to make | default work?

Because of the changes in external-secrets/external-secrets#3480, external-secrets controller fails when template tries to use a key that does not exist (even though there is a `| default` filter in-use).

shuheiktgw · 2024-06-06T02:40:37Z

@qlonik Hmm, yes that is correct... This is more like a Go template level problem, so there must be some ways to workaround the problem so let me check 🙇

shuheiktgw · 2024-06-06T21:40:47Z

@qlonik I've figured out how we can set the default values without raising errors. If this is a common use case, it might be favorable to add syntax sugar for it...

{{ if index . "missing_key" }}{{ .missing_key }}{{ else }}some-default{{ end }}

…3480) * Raise error when unknown key specified in template Signed-off-by: shuheiktgw <[email protected]> * Update the template docs to clarify the new behavior with non-existing keys Signed-off-by: shuheiktgw <[email protected]> --------- Signed-off-by: shuheiktgw <[email protected]> Signed-off-by: Bude8 <[email protected]>

qlonik · 2024-06-21T03:14:26Z

Hey @shuheiktgw, sorry, I haven't responded sooner. I don't know if this is necessary - I only had a few spots where this was an issue, and I just inlined default values where it was necessary.

However, I also found dig function, which I think can be used here and also can be used in the pipe:

{{ dig "missing_key" "some-default" . }}
{{ . | dig "missing_key" "some-default" }}
{{ . | ... | dig "missing_key" "some-default" | ... }}

…3480) * Raise error when unknown key specified in template Signed-off-by: shuheiktgw <[email protected]> * Update the template docs to clarify the new behavior with non-existing keys Signed-off-by: shuheiktgw <[email protected]> --------- Signed-off-by: shuheiktgw <[email protected]> Signed-off-by: Bill Hamilton <[email protected]>

ghost · 2024-07-08T22:44:38Z

Hi.

I think this broke my push secret as prior to upgrading this was working for me. I create a secret from a k8s secret on top of data I set using the templating and I get the error of:

external-secrets {"level":"error","ts":1720450212.1143377,"msg":"Reconciler error","controller":"pushsecret","controllerGroup":"[external-secrets.io](http://external-secrets.io/)","controllerKind":"PushSecret","PushSecret":{"name":"rabbitmq","namespace":"rabbitmq"},"namespace":"rabbitmq","name":"rabbitmq","reconcileID":"<REDACTED>","error":"could not execute template: could not execute template: unable to execute template at key endpoint: unable to execute template at key endpoint: template: endpoint:1:3: executing \"endpoint\" at <.endpoint>: map has no entry for key \"endpoint\"","stacktrace":"[sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler](http://sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler)\n\t/home/runner/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:324\[nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem](http://nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem)\n\t/home/runner/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:261\[nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2](http://nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2)\n\t/home/runner/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:222"}

This is my spec

Spec:
  Data:
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:         endpoint
        Remote Key:       rabbitmq
      Secret Key:         host
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:         endpoint-scheme
        Remote Key:       rabbitmq
      Secret Key:         endpoint-scheme
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:         management-port
        Remote Key:       rabbitmq
      Secret Key:         management-port
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:         management-scheme
        Remote Key:       rabbitmq
      Secret Key:         management-scheme
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:         port
        Remote Key:       rabbitmq
      Secret Key:         port
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:         password
        Remote Key:       rabbitmq
      Secret Key:         password
    Conversion Strategy:  None
    Match:
      Remote Ref:
        Property:    username
        Remote Key:  rabbitmq
      Secret Key:    username
  Deletion Policy:   None
  Refresh Interval:  1h
  Secret Store Refs:
    Kind:  ClusterSecretStore
    Name:  cluster-secret-store
  Selector:
    Secret:
      Name:  rabbitmq-default-user
  Template:
    Data:
      Endpoint:             {{ .endpoint }}
      Endpoint - Scheme:    amqp
      Management - Port:    15672
      Management - Scheme:  http
      Password:             {{ .password }}
      Port:                 {{ .port }}
      Username:             {{ .username }}
    Engine Version:         v2
    Merge Policy:           Replace
  Update Policy:            Replace

The secret should be

{
  "endpoint": "rabbitmq.rabbitmq.svc",
  "endpoint-scheme": "amqp",
  "management-port": "15672",
  "management-scheme": "http",
  "port": "5672",
  "password": "<REDACTED>",
  "username": "<REDACTED>"
}

What's weird is if I set

Endpoint:             {{ .endpoint }}

to

Endpoint:             {{ .host }}

It now works.... ?

Raise error when unknown key specified in template

b9dc1fe

Signed-off-by: shuheiktgw <[email protected]>

shuheiktgw requested a review from a team as a code owner May 12, 2024 01:51

shuheiktgw requested a review from rogertuma May 12, 2024 01:51

Skarlso previously approved these changes May 16, 2024

View reviewed changes

Update the template docs to clarify the new behavior with non-existin…

27206c9

…g keys Signed-off-by: shuheiktgw <[email protected]>

shuheiktgw dismissed Skarlso’s stale review via 27206c9 May 16, 2024 23:15

gusfcarvalho approved these changes May 17, 2024

View reviewed changes

gusfcarvalho merged commit b156e23 into external-secrets:main May 17, 2024

shuheiktgw deleted the fail_for_unknown_key branch June 6, 2024 02:35

Skarlso mentioned this pull request Sep 23, 2024

Regression in v0.9.19 when using 'default' in secret templates #3843

Closed

Uh oh!

Comments

Conversation

shuheiktgw commented May 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem Statement

Related Issue

Proposed Changes

Checklist

Uh oh!

gusfcarvalho commented May 13, 2024

Uh oh!

moolen commented May 13, 2024

Uh oh!

gusfcarvalho commented May 13, 2024

Uh oh!

shuheiktgw commented May 13, 2024

Uh oh!

gusfcarvalho commented May 14, 2024

Uh oh!

shuheiktgw commented May 15, 2024

Uh oh!

gusfcarvalho commented May 15, 2024

Uh oh!

sonarqubecloud bot commented May 16, 2024

Quality Gate passed

Uh oh!

shuheiktgw commented May 16, 2024

Uh oh!

gusfcarvalho commented May 17, 2024

Uh oh!

qlonik commented Jun 6, 2024

Uh oh!

shuheiktgw commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shuheiktgw commented Jun 6, 2024

Uh oh!

qlonik commented Jun 21, 2024

Uh oh!

ghost commented Jul 8, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shuheiktgw commented May 12, 2024 •

edited

Loading

shuheiktgw commented Jun 6, 2024 •

edited

Loading

ghost commented Jul 8, 2024 •

edited by ghost

Loading