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

kubeadm: apply deterministic order to certificate phases #78556

Merged
merged 1 commit into from Jun 14, 2019

Conversation

@neolit123
Copy link
Member

commented May 31, 2019

What this PR does / why we need it:
The existing logic already creates a proper "tree"
where a CA is always generated before the certs that are signed
by this CA, however the tree is not deterministic, because it uses a map.

Always use the default list of certs when generating the
"kubeadm init phase certs" phases. Add a unit test that
makes sure that CA always precede signed certs in the default
lists.

This solves the problem where the help screen for "kubeadm
init" cert sub-phases can have a random order.

Which issue(s) this PR fixes:

xref: #78544
xref: #70131 (comment)

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: introduce deterministic ordering for the certificates generation in the phase command "kubeadm init phase certs" .

@kubernetes/sig-cluster-lifecycle-pr-reviews
/assign @fabriziopandini
cc @dims @praseodym
/kind bug
/priority important-longterm
/milestone v1.16

kubeadm: apply deterministic order on certificate phases
The existing logic already creates a proper "tree"
where a CA is always generated before the certs that are signed
by this CA, however the tree is not deterministic.

Always use the default list of certs when generating the
"kubeadm init phase certs" phases. Add a unit test that
makes sure that CA always precede signed certs in the default
lists.

This solves the problem where the help screen for "kubeadm
init" cert sub-phases can have a random order.

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone May 31, 2019

@neolit123 neolit123 changed the title kubeadm: apply deterministic order on certificate phases kubeadm: apply deterministic order for certificate phases May 31, 2019

@k8s-ci-robot k8s-ci-robot requested review from dixudx and timothysc May 31, 2019

@neolit123 neolit123 changed the title kubeadm: apply deterministic order for certificate phases kubeadm: apply deterministic order to certificate phases May 31, 2019

@dims

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Excellent @neolit123

/lgtm
/hold

(please feel free to remove hold when appropriate)

var phase workflow.Phase
if cert.CAName == "" {
phase = newCertSubPhase(cert, runCAPhase(cert))
lastCACert = cert

This comment has been minimized.

Copy link
@chenzhiwei

chenzhiwei May 31, 2019

Contributor

is there a case that multiple certs have a blank CAName?

This comment has been minimized.

Copy link
@neolit123

neolit123 May 31, 2019

Author Member

not currently - we don't generate a CA that does not sign any certs.
but even if we have consecutive CAs they should be appended just fine.

@rosti

rosti approved these changes May 31, 2019

Copy link
Member

left a comment

This actually simplifies the code. Thanks @neolit123 !
/lgtm

@praseodym

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Looks good!
/approve
(can I do this?)

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, praseodym

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

@dims

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit fadb63c into kubernetes:master Jun 14, 2019

21 checks passed

cla/linuxfoundation neolit123 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
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.