Ctl commmand status certificate: Add events output #3102
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e95666a to
8e46021
Compare
wallrj
requested changes
Jul 17, 2020
There was a problem hiding this comment.
Hey @hzhou97
I compiled and tested ctl. Here's the output:
bazel-out/k8-fastbuild-ST-3bfd66f45e612c1a5c797474a25664e227d81bf914f3b08a40e00b2e2692afa4/bin/cmd/ctl/kubectl-cert_manager status certificate --namespace cert-manager-test selfsigned-cert
Name: selfsigned-cert
Namespace: cert-manager-test
Conditions:
Ready: True, Reason: Ready, Message: Certificate is up to date and has not expired
DNS Names:
- example.com
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal Issuing 2m26s cert-manager Issuing certificate as Secret does not exist
Normal Generated 2m26s cert-manager Stored new private key in temporary Secret resource "selfsigned-cert-kbhzm"
Normal Requested 2m26s cert-manager Created new CertificateRequest resource "selfsigned-cert-h94ds"
Normal Issuing 2m26s cert-manager The certificate has been successfully issued
Issuer:
Name: test-selfsigned
Kind:
Secret Name: selfsigned-cert-tls
Not Before: 2020-07-17T11:17:27+01:00
Not After: 2020-10-15T11:17:27+01:00
Renewal Time: 2020-09-15T11:17:27+01:00
No CertificateRequest found for this Certificate richard add_events_output ~ projects cert-manager cert-manager
Some problems I noticed:
- Missing newline at at the end of the output
- The Event reasons don't line up with the column heading
- Issuer > Kind: line has no value.
- There is a certificaterequest for this certificate, but I guess it's been filtered out by the
next revisioncheck (discussed in an earlier PR)....I think we should show all existing CRs for a cert but order them, most recent first, and somehow mark the ones which are in-progress vs CRs which are complete. - I think it'd be nice to show more information about the secret content...i.e. some summary information about the key and about the certificate that has actually been issued.
- I also think it'd be useful to show the time when the Certificate was created and when it was last updated.
- I think I'd like to see none-normal events associated with the Issuer, if the certificate / cr is in a failed state.
Just some suggestions, mostly for future PRs. But please fix the formatting of the events and the missing newline before merging this.
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
…f kubectl 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>
8e46021 to
38f50ca
Compare
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
38f50ca to
8525a26
Compare
wallrj
requested changes
Jul 22, 2020
| // This implementation is based on the one in the describe package, with a slight modification of having a baseLevel | ||
| // on top of which any other indentations are added. | ||
| // The purpose is be able to reuse functions in the describe package where the Level of the output is fixed, | ||
| // e.g. DescribeEvents() only prints out at Level 0. |
There was a problem hiding this comment.
Did you try nestedPrefixWriter ? It looks like it should allow you to write the events table, indented: https://github.com/kubernetes/kubernetes/blob/0b678bbb51a83e47df912f1205907418e354b281/staging/src/k8s.io/kubectl/pkg/describe/describe.go#L159
If not, then add a note to the top of this file explaining that parts of this code have been copied from a particular revision of Kubernetes and link to the file.
There was a problem hiding this comment.
Hmm. nestedPrefixWriter does not show up in the code I'm reading..
We still need the copied and slightly modified DescribeEvents because on this line they print the column headers by using \n and two spaces and not with Prefixing, that means that the column headers are slightly misformatted if we use the originial DescribeEvents to print Events for the CR.
I did remove the implementation of the PrefixWriter as it's not needed anymore.
Signed-off-by: Haoxiang Zhou <haoxiang.zhou@jetstack.io>
wallrj
approved these changes
Jul 23, 2020
There was a problem hiding this comment.
/lgtm
$ bazel-out/k8-fastbuild-ST-3bfd66f45e612c1a5c797474a25664e227d81bf914f3b08a40e00b2e2692afa4/bin/cmd/ctl/kubectl-cert_manager status certificate -n cert-manager-test selfsigned-cert
Name: selfsigned-cert
Namespace: cert-manager-test
Conditions:
Ready: True, Reason: Ready, Message: Certificate is up to date and has not expired
DNS Names:
- example.com
Events: <none>
Issuer:
Name: test-selfsigned
Kind: Issuer
Secret Name: selfsigned-cert-tls
Not Before: 2020-07-22T15:24:28+01:00
Not After: 2020-10-20T15:24:28+01:00
Renewal Time: 2020-09-20T15:24:28+01:00
No CertificateRequest found for this Certificate
$ bazel-out/k8-fastbuild-ST-3bfd66f45e612c1a5c797474a25664e227d81bf914f3b08a40e00b2e2692afa4/bin/cmd/ctl/kubectl-cert_manager renew -n cert-manager-test selfsigned-cert
Manually triggered issuance of Certificate cert-manager-test/selfsigned-cert
$ bazel-out/k8-fastbuild-ST-3bfd66f45e612c1a5c797474a25664e227d81bf914f3b08a40e00b2e2692afa4/bin/cmd/ctl/kubectl-cert_manager status certificate -n cert-manager-test selfsigned-cert
Name: selfsigned-cert
Namespace: cert-manager-test
Conditions:
Ready: True, Reason: Ready, Message: Certificate is up to date and has not expired
DNS Names:
- example.com
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal Issuing 3s (x2 over 21h) cert-manager The certificate has been successfully issued
Normal Reused 3s cert-manager Reusing private key stored in existing Secret resource "selfsigned-cert-tls"
Normal Requested 3s cert-manager Created new CertificateRequest resource "selfsigned-cert-z92xb"
Issuer:
Name: test-selfsigned
Kind: Issuer
Secret Name: selfsigned-cert-tls
Not Before: 2020-07-23T13:12:35+01:00
Not After: 2020-10-21T13:12:35+01:00
Renewal Time: 2020-09-21T13:12:35+01:00
No CertificateRequest found for this Certificate
|
Hmm why did it not find the CR? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzhou97, meyskens, wallrj 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR adds the Events of the Certificate and of the CertificateRequest to the output of the ctl command
status certificateWhich 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:
PrefixWriterinterface that is defined in thedescribepackage ofkubectlin order to reuse the existing functionDescribeEvents(). Not sure if there is a better way? An alternative I considered was to copyDescribeEvents()(along with a few private functions) and modify the copy.LICENSESandgo.sumcome from, but it was automatically generated when I ran./hack/update-deps.sh.DescribeEvents()is implemented, where they printEvents:\nType\tReasonand so on. It's not very big of a problem as the rest of the output is still correct. What do you think?Release note:
/area ctl
/kind feature