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.

Sign up for GitHub

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

Jump to bottom

Renew certificates as part of upgrade rather than recreating them #68749

Merged
merged 1 commit into from
Sep 18, 2018
Merged

Renew certificates as part of upgrade rather than recreating them #68749

merged 1 commit into from
Sep 18, 2018

Conversation

liztio
Copy link
Contributor

@liztio liztio commented Sep 17, 2018

What this PR does / why we need it:

Right now the static pod upgrade process is broken. It incorrectly attempts to create new certificates rather than just renewing the existing certificates.

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

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 17, 2018
@liztio
Copy link
Contributor Author

liztio commented Sep 17, 2018

/cc @detiber can you see if this fixes your issue?

@liztio liztio mentioned this pull request Sep 17, 2018
Closed
@liztio
Copy link
Contributor Author

liztio commented Sep 17, 2018

Supersedes #68485

@liztio
Copy link
Contributor Author

liztio commented Sep 17, 2018

/test pull-kubernetes-integration

@neolit123
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 17, 2018
@timothysc timothysc added this to the v1.12 milestone Sep 17, 2018
@timothysc timothysc added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 17, 2018
timothysc
timothysc approved these changes Sep 17, 2018
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/approve

/assign @detiber

@detiber given that you filed the original issue could you please verify this works for you.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2018
fabriziopandini
fabriziopandini approved these changes Sep 18, 2018
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Thanks @liztio for this fix!
/approve

tested with success upgrade from v1.11.3 to v1.12.0-beta.2; etcd certs get renewed (apiserver-etcd-client, healthcheck-client, peer, server)!

@timothysc
Copy link
Member

timothysc commented Sep 18, 2018 •
edited
Loading

@kubernetes/sig-release-members @tpepper

We'd ideally like to get this in before RC.

@detiber
Copy link
Member

detiber commented Sep 18, 2018

Actively testing this now.

@detiber
Copy link
Member

detiber commented Sep 18, 2018

Verified this resolves the issue I was hitting testing HA stacked control plane upgrades.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, fabriziopandini, liztio, timothysc

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

@tpepper
Copy link
Member

tpepper commented Sep 18, 2018

Should be in RC given tide merge trajectory and RC timing later today, will watch that it's merged by then.

@timothysc
Copy link
Member

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot merged commit e7eb269 into kubernetes:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini approved these changes

@timothysc timothysc timothysc approved these changes

@kad kad Awaiting requested review from kad

Assignees

@detiber detiber

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

kubeadm upgrades from v1.11 to v1.12 broken on etcd cert generation
7 participants
@liztio @neolit123 @timothysc @detiber @k8s-ci-robot @tpepper @fabriziopandini