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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/printers: Support base64 decode in kubectl get go-template #60755

Merged
merged 1 commit into from Mar 22, 2018

Conversation

@glb
Copy link
Contributor

glb commented Mar 5, 2018

What this PR does / why we need it:

Adds a base64decode function to templates in kubectl so that it's possible to extract secret data in plaintext instead of base64 without requiring a separate executable to do the decode.

Sample usage:

kubectl get secret SECRET -o go-template='{{ .data.KEY | base64decode }}'

Which issue(s) this PR fixes:
Fixes #45293.

Special notes for your reviewer:

Release note:

You can now use the `base64decode` function in kubectl go templates to decode base64-encoded data, for example `kubectl get secret SECRET -o go-template='{{ .data.KEY | base64decode }}'`.
@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 5, 2018

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli label Mar 5, 2018

@jdumars

This comment has been minimized.

Copy link
Member

jdumars commented Mar 5, 2018

/ok-to-test

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 5, 2018

There seems to have been a segmentation fault during the tests:

W0305 23:18:56.315] build/gen-swagger-docs.sh: line 29:     9 Aborted                 (core dumped) ./gradle-2.5/bin/gradle gendocs --info

Not sure about the cause, and there doesn't seem to be an open issue tracking this. Maybe a retest will not encounter the problem?

/retest

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 6, 2018

What's the protocol for reporting the seg fault?

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 6, 2018

@ericchiang @derekwaynecarr do you know when you might be able to get a chance to look at this PR (a tiny change with a fair amount of test boilerplate and then a test)? Is there someone else I should ask to take a look? I've tried to get feedback on whether this is a reasonable solution through questions on the issue but there's been nothing for the past week.

func base64decode(v string) string {
data, err := base64.StdEncoding.DecodeString(v)
if err != nil {
return err.Error()

This comment has been minimized.

@liggitt

liggitt Mar 6, 2018

Member

this isn't good… it means the output from an error is indistinguishable from success

This comment has been minimized.

@liggitt

liggitt Mar 6, 2018

Member

Make this a two value return:

Each function must have either a single return value, or two return values of which the second has type error. In that case, if the second (error) return value evaluates to non-nil during execution, execution terminates and Execute returns that error.

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

Thanks! Will do.

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

I also figured out how to trigger the error path in the test using an intentionally-broken Marshaler.

Funcs(template.FuncMap{"exists": exists}).
Funcs(template.FuncMap{
"exists": exists,
"b64dec": base64decode,

This comment has been minimized.

@liggitt

liggitt Mar 6, 2018

Member

Is b64dec a name we're happy with long term? I have a slight preference for being explicit, e.g. base64decode

This comment has been minimized.

@liggitt

liggitt Mar 6, 2018

Member

Also, the fact that that's the name you picked for the actual function (which I hadn't noticed) makes me smile

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

:) Thanks @liggitt ! I picked b64dec for compatibility with sprig on the off chance that folks might want to expand the set of functions using that library.

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

(and I got used to sprig from using Helm, so thought it would be good to be consistent.)

If you'd prefer base64decode I'm not stuck on b64dec, just wanted to explain where the name came from.

This comment has been minimized.

@ericchiang

ericchiang Mar 7, 2018

Member

I'd prefer base64decode too.

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

OK. Sorry to be slightly stuck on this, but would it be a reasonable compromise to support both base64decode and b64dec to reduce frustration for folks who are also writing helm templates? I say this because I got here by writing a helm template that needed to print out some initial values for secret data for the user doing the install, and I'm sure that me three weeks from now will trip over the different name.

Feel free to say no; I realize that having multiple names for the same function is potentially confusing in a different direction.

@@ -112,3 +116,11 @@ func (p *TemplatePrinter) safeExecute(w io.Writer, obj interface{}) error {
}
return retErr
}

func base64decode(v string) string {
data, err := base64.StdEncoding.DecodeString(v)

This comment has been minimized.

@liggitt

liggitt Mar 6, 2018

Member

Does go json encoding use StdEncoding for []byte serialization?

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor
} else {
err = p.PrintObj(test.obj, buffer)
}
if err != nil {

This comment has been minimized.

@ericchiang

ericchiang Mar 7, 2018

Member

Because of

err = p.PrintObj(test.obj, buffer)

err can never be nil

Also please avoid else if. For example

if err != nil && err.Error() != test.expectErr {
    t.Errorf("[%s]expect:\n %v\n but got:\n %v\n", name, test.expectErr, err)
    continue
}
if test.expectErr != "" {
    t.Errorf("[%s]expect:\n error %v\n but got:\n no error\n", name, test.expectErr)
}

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

Thanks @ericchiang! I'm not sure I follow the first comment -- p.PrintObj(test.obj, buffer) will return nil if there is no error, so err could be nil, no?

I'll try to apply the second comment tomorrow; I am having some difficulty unwinding the logic I've got into the structure you requested without breaking the tests.

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

Just had to walk away for a couple of minutes, tests are passing now with simplified structure. Thanks for the comment, this looks a lot clearer now!

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 7, 2018

pull-kubernetes-kubemark-e2e-gce appears to be broken at the moment: #60870

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 7, 2018

From the comments it looks like the issue might be fixed, trying the test again.
/test pull-kubernetes-kubemark-e2e-gce

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 7, 2018

@liggitt @ericchiang I think I've addressed all of the comments. Let me know what you want to do on the function name, I'd prefer to be consistent with what users will see in Helm but will change if if you want. If everything is OK I can rebase and squash the commits.

@ericchiang
Copy link
Member

ericchiang left a comment

Thanks for the test flow changes. It's a lot easier to read.

Couple final nits. other than that lgtm

We're in freeze right now, so we're going to wait until master opens up for 1.11 in couple weeks or so.

"test error path for base64 decoding": {
template: "{{ .Data.username | b64dec }}",
obj: &badlyMarshaledSecret{},
expectErr: "error executing template \"{{ .Data.username | b64dec }}\": template: output:1:20: executing \"output\" at <b64dec>: error calling b64dec: illegal base64 data at input byte 0",

This comment has been minimized.

@ericchiang

ericchiang Mar 7, 2018

Member

This relies on explicit error strings from several packages in the Go std library, which may change depending on the Go version used to run the test. This needs to only rely on error strings we control (e.g. check for a substring in the error), or only test for the presence of an error.

This comment has been minimized.

@glb

glb Mar 7, 2018

Author Contributor

OK. I'll see what I can come up with. Thanks!

Funcs(template.FuncMap{"exists": exists}).
Funcs(template.FuncMap{
"exists": exists,
"b64dec": base64decode,

This comment has been minimized.

@ericchiang

ericchiang Mar 7, 2018

Member

I'd prefer base64decode too.

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 7, 2018

Thanks @ericchiang ! Sorry, I didn't realize about the freeze, I'm sure you are all extremely busy with actual stuff more people want. Once I hear back on my final (I promise) question about the function name and fix the error test case, should I leave the commit stream until later or squash it? Then once it's done, will everything happen automagically once master opens up again or should I set a reminder to come back and ping you?

Thanks again!

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 7, 2018

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 11, 2018

/test pull-kubernetes-e2e-gce

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Mar 13, 2018

Please squash your commits. Other than that lgtm!

pkg/printers: Support base64 decode in kubectl go-template
Adds a `base64decode` function to templates in `kubectl` so that
it's possible to extract secret data in plaintext instead of
base64 without requiring a separate executable.

Sample usage:

```sh
kubectl get secret SECRET -o go-template='{{ .data.KEY | base64decode }}'
```

@glb glb force-pushed the glb:support-b64dec-in-templates branch from ad5d812 to f8c56ba Mar 13, 2018

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 14, 2018

@ericchiang all squashed, commit & PR text updated to reflect base64decode instead of the original b64dec, tests passed. Good to go?

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Mar 20, 2018

/lgtm

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Mar 20, 2018

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 20, 2018

/assign @smarterclayton

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Mar 20, 2018

/approve

although we should write up what our API compatibility story and put a comment in the code around what it entails (does this need to be longer term than our standard kubectl guarantees).

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, glb, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@glb

This comment has been minimized.

Copy link
Contributor Author

glb commented Mar 20, 2018

@smarterclayton sorry, is this something you'd like me to add to this PR? I'm not sure what to add here.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 22, 2018

Automatic merge from submit-queue (batch tested with PRs 61354, 61366, 61386, 61394, 60755). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 1365ce3 into kubernetes:master Mar 22, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation glb authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@glb glb deleted the glb:support-b64dec-in-templates branch Mar 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.