kubeadm certs check-expiration support json/yaml output #123372

carlory · 2024-02-19T08:49:22Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

text print will not be changed.

CERTIFICATE                EXPIRES                  RESIDUAL TIME   CERTIFICATE AUTHORITY   EXTERNALLY MANAGED
admin.conf                 Feb 05, 2025 08:38 UTC   351d            ca                      no
apiserver                  Feb 05, 2025 08:38 UTC   351d            ca                      no
apiserver-etcd-client      Feb 05, 2025 08:38 UTC   351d            etcd-ca                 no
apiserver-kubelet-client   Feb 05, 2025 08:38 UTC   351d            ca                      no
controller-manager.conf    Feb 05, 2025 08:38 UTC   351d            ca                      no
etcd-healthcheck-client    Feb 05, 2025 08:38 UTC   351d            etcd-ca                 no
etcd-peer                  Feb 05, 2025 08:38 UTC   351d            etcd-ca                 no
etcd-server                Feb 05, 2025 08:38 UTC   351d            etcd-ca                 no
front-proxy-client         Feb 05, 2025 08:38 UTC   351d            front-proxy-ca          no
scheduler.conf             Feb 05, 2025 08:38 UTC   351d            ca                      no
super-admin.conf           Feb 05, 2025 08:38 UTC   351d            ca                      no

CERTIFICATE AUTHORITY   EXPIRES                  RESIDUAL TIME   EXTERNALLY MANAGED
ca                      Feb 03, 2034 08:38 UTC   9y              no
etcd-ca                 Feb 03, 2034 08:38 UTC   9y              no
front-proxy-ca          Feb 03, 2034 08:38 UTC   9y              no

json print

{
    "kind": "CertificateExpirationInfo",
    "apiVersion": "output.kubeadm.k8s.io/v1alpha3",
    "certificates": [
        {
            "name": "admin.conf",
            "expirationDate": "2025-02-05T08:38:15Z",
            "residualTime": 30410401634330825,
            "externallyManaged": false,
            "caName": "ca",
            "missing": false
        },
      ...
    ],
    "certificateAuthorities": [
        {
            "name": "ca",
            "expirationDate": "2034-02-03T08:38:14Z",
            "residualTime": 314234400633287229,
            "externallyManaged": false,
            "missing": false
        },
    ...
    ]
}

yaml print

apiVersion: output.kubeadm.k8s.io/v1alpha3
certificateAuthorities:
- expirationDate: 2025-02-05T08:38:15Z
  externallyManaged: false
  missing: false
  name: ca
  residualTime: 314234210341126462
...
certificates:
- caName: ca
  expirationDate: 2034-02-03T08:38:14Z
  externallyManaged: false
  missing: false
  name: admin.conf
  residualTime: 30410211342350352
...
kind: CertificateExpirationInfo

Which issue(s) this PR fixes:

Part of kubernetes/kubeadm#494

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: add support for machine readable output with "-o yaml" and "-o json" to the command "kubeadm certs check-expiration". This change is added in a new API "kind": "CertificateExpirationInfo",  "apiVersion": "output.kubeadm.k8s.io/v1alpha3". The existing non structured formatting is preserved. The output API version v1alpha2 is now deprecated and will be removed in a future release. Please migrate to using v1alpha3.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/2504

k8s-ci-robot · 2024-02-19T08:49:25Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2024-02-19T08:49:31Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cmd/kubeadm/app/apis/output/v1alpha2/types.go

neolit123

our "unwritten policy" is to make API changes in a new version.
so this has to go in v1alpha3 - a new API.

i will have a more detailed review pass once you complete that.

cmd/kubeadm/app/apis/output/v1alpha2/types.go

neolit123 · 2024-02-19T09:40:43Z

i think the important part is to not break the CLI output (stdout) as some users were already parsing it . :/
it has to continue to exist exactly the way it is.

but for the new structured output we can make some new decisions for example a boolean option can be marshaled to true/false instead of "yes"/"no" strings.

neolit123 · 2024-02-19T13:52:18Z

/release-note-edit

kubeadm: add support for machine readable output with "-o yaml" and "-o json" to the command "kubeadm certs check-expiration". This change is added in a new API "kind": "ExpirationInfo",  "apiVersion": "output.kubeadm.k8s.io/v1alpha3". The existing non structured formatting is preserved. The output API version v1alpha2 is now deprecated and will be removed in a future release. Please migrate to using v1alpha3.

neolit123 · 2024-02-19T13:54:00Z

The output API version v1alpha2 is now deprecated and will be removed in a future release.

@carlory please add a `DEPRECATED: this API will be removed in a future release. Please use v1alpha3." text above this line:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/output/v1alpha2/doc.go#L26

cmd/kubeadm/app/apis/output/types.go

cmd/kubeadm/app/apis/output/v1alpha3/doc.go

cmd/kubeadm/app/apis/output/types.go

cmd/kubeadm/app/apis/output/v1alpha3/types.go

cmd/kubeadm/app/cmd/certs.go

neolit123 · 2024-02-19T14:23:24Z

cmd/kubeadm/app/cmd/certs.go

-			}
-			w := tabwriter.NewWriter(out, 10, 4, 3, ' ', 0)
-			fmt.Fprintln(w, "CERTIFICATE\tEXPIRES\tRESIDUAL TIME\tCERTIFICATE AUTHORITY\tEXTERNALLY MANAGED")
+			info := &outputapiv1alpha3.ExpirationInfo{}


can we move the whole code in a new helper function and add a unit test for it?
no need to be very verbose in the unit test, just test a basic JSON output scenario with some certs and CAs, maybe some missing certs too.

@neolit123 added a unit test for this command. please see https://github.com/kubernetes/kubernetes/compare/0b3e5411c83eef6f658d037eb4c5e0a3847df0fc..6ec538fc25c17a8c72855c8117865be9f1fb3524

neolit123 · 2024-02-19T14:26:52Z

/uncc @chendave
/cc @pacoxu

the API change already looks good. added some general rename/organization suggestions.

cmd/kubeadm/app/cmd/certs_test.go

neolit123 · 2024-02-21T11:46:18Z

cmd/kubeadm/app/cmd/certs.go

-	if cfgPath == "" {
-		client, err := kubeconfigutil.ClientSetFromFile(kubeconfigPath)
+	if cfgPath == "" && client != nil {
+		internalcfg, err := configutil.FetchInitConfigurationFromCluster(client, printer, logPrefix, false, false)


@neolit123 if we give the printer to FetchInitConfigurationFromCluster, -o json will not print the message then.

I split the previous commit into two commits.

the first commit fixes the JSON output (-o json will not print the message)
the second commit adds some tests.

this seems OK, but can we also fix upgrade plan -o json?

carlory · 2024-02-21T12:18:22Z

This PR doesn't fix upgrade plan -o json issue. some messages are always printed. i.e [upgrade] Running cluster health checks

I can fix it in another PR after this PR is merged. @neolit123

neolit123 · 2024-02-21T12:39:42Z

This PR doesn't fix upgrade plan -o json issue. some messages are always printed. i.e [upgrade] Running cluster health checks

I can fix it in another PR after this PR is merged. @neolit123

thanks, ping me on that pr.
@pacoxu looks like we don't have users of the upgrade plan -o json, given we got no report about it.

cmd/kubeadm/app/cmd/certs_test.go

cmd/kubeadm/app/cmd/certs.go

cmd/kubeadm/app/cmd/certs_test.go

… server

neolit123

tested locally and it looks good.
added 2 minor comments / 1 question.

cmd/kubeadm/app/apis/output/types.go

k8s-ci-robot · 2024-02-22T03:19:49Z

@carlory: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-linter-hints	`0c4df64`	link	false	`/test pull-kubernetes-linter-hints`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

carlory · 2024-02-22T03:20:53Z

unrelated failures
/test pull-kubernetes-unit

pacoxu · 2024-02-22T03:41:51Z

/lgtm

This PR doesn't fix upgrade plan -o json issue. some messages are always printed. i.e [upgrade] Running cluster health checks

I can fix it in another PR after this PR is merged. @neolit123

Thanks.

k8s-ci-robot · 2024-02-22T03:41:57Z

LGTM label has been added.

Git tree hash: f68b696b1ffe0957766b792e1f474727d523fa7e

neolit123

thanks @carlory
please ping me on the fix PR for "upgrade plan" when you send it.

/hold cancel
/lgtm

neolit123 · 2024-02-22T09:04:42Z

cmd/kubeadm/app/cmd/certs.go:352:19: Error return value of `printer.Println` is not checked (errcheck)
			printer.Println() // add empty line to separate the FetchInitConfigurationFromCluster output from the command output
			               ^
cmd/kubeadm/app/cmd/certs.go:363:17: Error return value of `printer.Printf` is not checked (errcheck)
		printer.Printf("[%s] Error reading configuration from the Cluster. Falling back to default configuration\n\n", logPrefix)
		              ^

these can be fixed with _ = printer.Println()..

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 19, 2024

k8s-ci-robot requested review from neolit123 and SataQiu February 19, 2024 08:49

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 19, 2024

carlory force-pushed the fix-kubeadm-494 branch from 4b7b6c5 to 2e93d33 Compare February 19, 2024 09:17

carlory commented Feb 19, 2024

View reviewed changes

cmd/kubeadm/app/apis/output/v1alpha2/types.go Outdated Show resolved Hide resolved

neolit123 reviewed Feb 19, 2024

View reviewed changes

cmd/kubeadm/app/apis/output/v1alpha2/types.go Outdated Show resolved Hide resolved

pacoxu mentioned this pull request Feb 19, 2024

Machine readable output from kubeadm commands kubernetes/kubeadm#494

Closed

5 tasks

carlory force-pushed the fix-kubeadm-494 branch 2 times, most recently from 842e3d6 to 1e9f80c Compare February 19, 2024 10:45

carlory marked this pull request as ready for review February 19, 2024 10:45

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2024

k8s-ci-robot requested a review from chendave February 19, 2024 10:46

neolit123 reviewed Feb 19, 2024

View reviewed changes

k8s-ci-robot removed the request for review from chendave February 19, 2024 14:26

carlory force-pushed the fix-kubeadm-494 branch from be020f4 to 829cae6 Compare February 21, 2024 10:22

neolit123 reviewed Feb 21, 2024

View reviewed changes

cmd/kubeadm/app/cmd/certs_test.go Outdated Show resolved Hide resolved

carlory force-pushed the fix-kubeadm-494 branch from 829cae6 to 57c27fe Compare February 21, 2024 11:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2024

k8s-ci-robot requested a review from pacoxu February 21, 2024 11:00

carlory force-pushed the fix-kubeadm-494 branch 3 times, most recently from d68b0b5 to afa9ab4 Compare February 21, 2024 11:15

neolit123 reviewed Feb 21, 2024

View reviewed changes

carlory force-pushed the fix-kubeadm-494 branch from afa9ab4 to 4d49edf Compare February 21, 2024 12:12

neolit123 reviewed Feb 21, 2024

View reviewed changes

cmd/kubeadm/app/cmd/certs_test.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/certs.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/cmd/certs_test.go Outdated Show resolved Hide resolved

add new test cases to test json output when fetch kubeadm config from…

ecaf269

… server

carlory force-pushed the fix-kubeadm-494 branch from 4d49edf to ecaf269 Compare February 21, 2024 13:26

neolit123 reviewed Feb 21, 2024

View reviewed changes

cmd/kubeadm/app/apis/output/types.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/apis/output/types.go Outdated Show resolved Hide resolved

fix api

0c4df64

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2024

neolit123 reviewed Feb 22, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2024

k8s-ci-robot assigned neolit123 Feb 22, 2024

k8s-ci-robot merged commit ec58e1f into kubernetes:master Feb 22, 2024
13 of 14 checks passed

k8s-ci-robot added this to the v1.30 milestone Feb 22, 2024

carlory mentioned this pull request Apr 18, 2024

Update kubeadm reference for v1.30 kubernetes/website#45911

Merged

Payback159 mentioned this pull request May 6, 2024

Optimize logic of k8s-certs-renew.sh script kubernetes-sigs/kubespray#11164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm certs check-expiration support json/yaml output #123372

kubeadm certs check-expiration support json/yaml output #123372

carlory commented Feb 19, 2024 •

edited

Loading

k8s-ci-robot commented Feb 19, 2024

k8s-ci-robot commented Feb 19, 2024

neolit123 left a comment

neolit123 commented Feb 19, 2024 •

edited

Loading

neolit123 commented Feb 19, 2024

neolit123 commented Feb 19, 2024

neolit123 Feb 19, 2024

carlory Feb 20, 2024

neolit123 commented Feb 19, 2024 •

edited

Loading

neolit123 Feb 21, 2024

carlory commented Feb 21, 2024

neolit123 commented Feb 21, 2024

neolit123 left a comment

k8s-ci-robot commented Feb 22, 2024 •

edited

Loading

carlory commented Feb 22, 2024

pacoxu commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

neolit123 left a comment

neolit123 commented Feb 22, 2024

kubeadm certs check-expiration support json/yaml output #123372

kubeadm certs check-expiration support json/yaml output #123372

Conversation

carlory commented Feb 19, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 19, 2024

k8s-ci-robot commented Feb 19, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Feb 19, 2024 • edited Loading

neolit123 commented Feb 19, 2024

neolit123 commented Feb 19, 2024

neolit123 Feb 19, 2024

Choose a reason for hiding this comment

carlory Feb 20, 2024

Choose a reason for hiding this comment

neolit123 commented Feb 19, 2024 • edited Loading

neolit123 Feb 21, 2024

Choose a reason for hiding this comment

carlory commented Feb 21, 2024

neolit123 commented Feb 21, 2024

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 22, 2024 • edited Loading

carlory commented Feb 22, 2024

pacoxu commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Feb 22, 2024

carlory commented Feb 19, 2024 •

edited

Loading

neolit123 commented Feb 19, 2024 •

edited

Loading

neolit123 commented Feb 19, 2024 •

edited

Loading

k8s-ci-robot commented Feb 22, 2024 •

edited

Loading