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: Migrate cert and PKI helpers to ClusterConfiguration #67542

Closed
wants to merge 2 commits into from

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Aug 17, 2018

What this PR does / why we need it:

This PR is the second in a series, targeting the replacement of InitConfiguration with ClusterConfiguration, when the former is not needed. Please, review only the last commit.

This PR migrates cert and PKI helper APIs to use ClusterConfiguration (and away from InitConfiguration).

The certificate and PKI related helper APIs used InitConfiguration. With the introduction of ClusterConfiguration it became more viable to use that instead (in most cases in combination with NodeRegistrationOptions).

This change will allow us to avoid the use of InitConfiguration outside of kubeadm init

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
refs kubernetes/kubeadm#963

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/kind enhancement
/assign @luxas
/assign @timothysc
/assign @fabriziopandini
/cc @stealthybox

Depends on:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 17, 2018
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 17, 2018
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 17, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2018
@fabriziopandini
Copy link
Member

/hold
@rosti please wait until the config file reworking is completed in order to avoid unnecessary reworks. E.g we are going to move advertise address out of ClusterConfiguration -> this might require to revert some changes in this pr

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2018
Replace the unnecessary use of InitConfiguration in images.go with
ClusterConfiguration. This changes the interfaces of the following functions:

- GetKubeControlPlaneImage
- GetEtcdImage
- GetAllImages

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2018
The certificate and PKI related helper APIs used InitConfiguration. With the
introduction of ClusterConfiguration it became more viable to use that instead
(in most cases in combination with NodeRegistrationOptions).

This change will allow us to avoid the use of InitConfiguration outside of
kubeadm init.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@rosti rosti force-pushed the kubeadm_clusterconfig_certs branch from 9a9eecc to 8ebca43 Compare August 24, 2018 11:55
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 24, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rosti
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: fabriziopandini

If they are not already assigned, you can assign the PR to them by writing /assign @fabriziopandini in a comment when ready.

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

@rosti
Copy link
Contributor Author

rosti commented Aug 24, 2018

@fabriziopandini I can see your point here and I agree with the hold.
I did the PR thinking, that even if API server advertise address moves, it will likely be a move somewhere inside ClusterConfiguration as opposed to external. Thus, changing the interface to a combo of ClusterConfiguration and NodeRegistrationOptions looked safe to me.

@timothysc
Copy link
Member

We are going to need to lock down the config for 1.12 and punt the rest of the shuffling to try and make a complete push to beta in 1.13.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2018
@k8s-ci-robot
Copy link
Contributor

@rosti: PR needs rebase.

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.

@neolit123
Copy link
Member

do we still need this for 1.13?
i haven't checked.

@rosti
Copy link
Contributor Author

rosti commented Oct 31, 2018

Not in this form and certainly this should not have a high priority. I'll close this for now.

/close

@k8s-ci-robot
Copy link
Contributor

@rosti: Closing this PR.

In response to this:

Not in this form and certainly this should not have a high priority. I'll close this for now.

/close

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.

@rosti rosti deleted the kubeadm_clusterconfig_certs branch November 22, 2018 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. 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

6 participants