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.

Sign up for GitHub

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

Jump to bottom

replace commands/envcommand by DataSource in SecretGenerator #703

Merged
merged 2 commits into from
Jan 16, 2019
Merged

replace commands/envcommand by DataSource in SecretGenerator #703

merged 2 commits into from
Jan 16, 2019

Conversation

Liujingfang1
Copy link
Contributor

The change is a different approach for fixing #692. It changes SecretGenerator works the same way as ConfigMapGenerator, loading data from files or literals.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Liujingfang1

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 16, 2019
@Liujingfang1 Liujingfang1 mentioned this pull request Jan 16, 2019
Closed
@Liujingfang1 Liujingfang1 requested review from liggitt and removed request for justinsb and droot January 16, 2019 16:54
liggitt
liggitt reviewed Jan 16, 2019
View reviewed changes
Copy link
Contributor

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

a few questions

looks reasonable overall, will defer to someone more familiar with the configmap generator for specifics of the wiring

docs/kustomization.yaml Outdated
tls.crt: "cat secret/tls.cert"
tls.key: "cat secret/tls.key"
files:
- tls.crt=./catsecret/tls.cert
Copy link
Contributor

Choose a reason for hiding this comment

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

is the leading ./ required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not required.

docs/kustomization.yaml Outdated
# you can only specify one envCommand per secret.
envCommand: printf \"DB_USERNAME=admin\nDB_PASSWORD=somepw\"
env: ./env.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

same question, is leading ./ required? if not, consider dropping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required. Will drop it.

examples/combineConfigs.md Outdated
tls.key: "cat tls.key"
literals:
tls.crt=./tls.cert
tls.key=./tls.key
Copy link
Contributor

Choose a reason for hiding this comment

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

is this example correct? it looks like it would create secret keys with the contents ./tls.cert, etc, which probably isn't what someone would want

Copy link
Contributor Author

@Liujingfang1 Liujingfang1 Jan 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

My bad, will change those literals to better examples. Those should be files instead of literals.

pkg/types/kustomization.go Outdated
// TimeoutSeconds specifies the timeout for commands.
TimeoutSeconds *int64 `json:"timeoutSeconds,omitempty" yaml:"timeoutSeconds,omitempty"`
// DataSources for secret.
DataSources `json:",inline,omitempty" yaml:",inline,omitempty"`
}

// CommandSources contains some generic sources for secrets.
Copy link
Contributor

Choose a reason for hiding this comment

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

drop unused CommandSources?

@Liujingfang1 Liujingfang1 force-pushed the changeSecretG branch 2 times, most recently from f7d47de to 6ad62f1 Compare January 16, 2019 18:18
monopole
monopole reviewed Jan 16, 2019
View reviewed changes
k8sdeps/configmapandsecret/secretfactory.go Outdated
const (
defaultCommandTimeout = 5 * time.Second
)

// SecretFactory makes Secrets.
type SecretFactory struct {
fSys fs.FileSystem
Copy link
Contributor

Choose a reason for hiding this comment

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

fSys no longer needed.

It was there just to set a current working directory for the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. fSys is not needed. I removed it from both SecretGenerator and ConfigMapGenerator.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 16, 2019
@monopole
Copy link
Contributor

/lgtm

Grepped through the docs, seems like everything that matters is captured here.

Lets do this rather than #692 :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2019
@k8s-ci-robot k8s-ci-robot merged commit 4e2c4b9 into kubernetes-sigs:master Jan 16, 2019
@Liujingfang1 Liujingfang1 mentioned this pull request Jan 16, 2019
Closed
@sethpollack
Copy link
Contributor

Is there a recommended way to generate secrets now?

@Liujingfang1
Copy link
Contributor Author

@sethpollack The secret data can be put into files by custom commands. Then those files can be added to SecretGenerator.

secretGenerator:
- name: a-secret
  files:
    - file1
    - file2

@jcassee jcassee mentioned this pull request Jan 31, 2019
Merged
@Liujingfang1 Liujingfang1 deleted the changeSecretG branch April 4, 2019 20:45
@jonasrmichel jonasrmichel mentioned this pull request Apr 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monopole monopole monopole left review comments

@liggitt liggitt liggitt left review comments

Assignees

@monopole monopole

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Liujingfang1 @k8s-ci-robot @monopole @sethpollack @liggitt