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

Enable create cr ctl command to fetch certificate #3044

Merged
merged 18 commits into from Jul 10, 2020

Conversation

hzhou97
Copy link
Contributor

@hzhou97 hzhou97 commented Jun 29, 2020

What this PR does / why we need it:
This PR enables the user to specify --fetch-certificate and --output-cert-file flags to wait for the CertificateRequest that was created to be ready and store the certificate in a file.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

kubectl cert-manager: Added flags to wait for the CertificateRequest to be ready and store the certificate in a file.

/kind feature
/area ctl

@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jun 29, 2020
@jetstack-bot jetstack-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. area/testing Issues relating to testing and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 29, 2020
@meyskens meyskens added the area/ctl Indicates a PR or issue relates to the cert-manager-ctl CLI component label Jun 30, 2020
cmd/ctl/pkg/util/util.go Outdated Show resolved Hide resolved
cmd/ctl/pkg/util/util.go Outdated Show resolved Hide resolved
cmd/ctl/pkg/util/util.go Outdated Show resolved Hide resolved
@hzhou97 hzhou97 requested a review from meyskens June 30, 2020 15:13
@hzhou97 hzhou97 force-pushed the create_cr_fetch branch 2 times, most recently from 5e331fe to 05ec751 Compare June 30, 2020 15:28
if o.CertFileName != "" {
actualCertFileName = o.CertFileName
}
err = util.FetchCertificateFromCR(o.CMClient, req.Name, req.Namespace, actualCertFileName, o.IOStreams)
Copy link
Member

Choose a reason for hiding this comment

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

Once you in-line the code from PollUntilCRIsReadyOrTimeOut, I think you will be able to more easily access cr without having to call Get again (as we will have fetched it in that earlier 'wait' code).

We also shouldn't need to check CertificateRequestHasCondition either, as that will have already been verified above.

Once you remove these changes, the FetchCertificateFromCR function only consists of:

	// Store certificate to file
	err = ioutil.WriteFile(certFileName, req.Status.Certificate, 0600)
	if err != nil {
		return fmt.Errorf("error when writing certificate to file: %w", err)
	}

	fmt.Fprintf(ioStreams.Out, "Certificate has been stored in file %s\n", certFileName)

which I think is short and concise enough to be in-lined here, which should a) make this a bit simpler to read (less jumping between files 🎉) and b) mean we don't need to export new functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote this function with the separate fetch command in mind, thinking I could reuse this function.
I do agree that what you propose is cleaner if we are only using it in this context, but I'm not sure if we want to duplicate the writing to file logic in two places.

test/integration/ctl/ctl_create_cr_test.go Outdated Show resolved Hide resolved
test/integration/ctl/ctl_create_cr_test.go Outdated Show resolved Hide resolved
// Try to set Ready Condition if needed, otherwise the test just times out
if test.fetchCert {
go setCRReadyCondition(t, cmCl, test.inputArgs[0], test.inputNamespace)
}
// Create CR
err = opts.Run(test.inputArgs)
if err != nil {
// TODO: Maybe it is desirable to make the test more fine grained, i.e. specify which error is expected,
// to know where exactly things should fail and then check the correctness of the parts that shouldn't have failed
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't added here but.. yes, definitely 😄 especially as this output is user-facing in their CLI, so it's good to have the actual text encoded here so we know when we change it (either by accident or intentionally). It also helps us avoid doing something like attempting to print a structure or a pointer or something to the CLI, which is really confusing for people 😅

test/integration/ctl/ctl_create_cr_test.go Outdated Show resolved Hide resolved
test/integration/ctl/ctl_create_cr_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@james-w james-w left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I think it looks good, I just have some fairly minor suggestions.

Comment on lines -124 to -125
if o.KeyFilename != "" && (len(o.KeyFilename) < 4 || o.KeyFilename[len(o.KeyFilename)-4:] != ".key") {
return errors.New("file to store private key must end in '.key'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we no longer want this restriction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't, the team mentioned people might prefer other file extensions, so it would be easier to just not check at all.

cmd/ctl/pkg/util/util.go Outdated Show resolved Hide resolved
cmd/ctl/pkg/util/util.go Outdated Show resolved Hide resolved
@munnerz
Copy link
Member

munnerz commented Jul 10, 2020

Approval = this should exist in our codebase, which it should, so adding an approved label and we can get lgtm on there soon 😄

/approve

@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzhou97, munnerz

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

@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 10, 2020
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
…atus.certificate

Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2020
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
@hzhou97 hzhou97 requested a review from james-w July 10, 2020 14:51
@james-w
Copy link
Contributor

james-w commented Jul 10, 2020

/lgtm

Thanks!

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2020
@hzhou97
Copy link
Contributor Author

hzhou97 commented Jul 10, 2020

/kind feature

@jetstack-bot jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 10, 2020
@jetstack-bot jetstack-bot merged commit d007fe7 into cert-manager:master Jul 10, 2020
@jetstack-bot jetstack-bot added this to the v0.16 milestone Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ctl Indicates a PR or issue relates to the cert-manager-ctl CLI component area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants