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 - set peer urls for default etcd instance #64988

Merged
merged 1 commit into from Jun 12, 2018

Conversation

@detiber
Copy link
Member

detiber commented Jun 11, 2018

What this PR does / why we need it:

Override the default peer URLs for the default etcd instance. Previously we left the defaults, which meant the peer URL was unsecured previously.

Release note:

kubeadm - Ensure the peer port is secured by explicitly setting the peer URLs for the default etcd instance.
kubeadm - Ensure that the etcd certificates are generated using a proper CN
kubeadm - Update generated etcd peer certificate to include localhost addresses for the default configuration.
kubeadm - Increase the manifest update timeout to make upgrades a bit more reliable.
@timothysc
Copy link
Member

timothysc left a comment

/approve
/hold

"name": cfg.GetNodeName(),
"listen-client-urls": "https://127.0.0.1:2379",
"advertise-client-urls": "https://127.0.0.1:2379",
"listen-peer-urls": "https://127.0.0.1:2380",

This comment has been minimized.

@timothysc

timothysc Jun 11, 2018

Member

I'd be pretty surprised that this would cause an issue.

@luxas

luxas approved these changes Jun 11, 2018

Copy link
Member

luxas left a comment

/lgtm
/hold cancel

@timothysc I was a bit surprised too to see this in the logs earlier today:

2018-06-11 18:58:38.994099 W | embed: The scheme of peer url http://localhost:2380 is HTTP while peer key/cert files are presented. Ignored peer key/cert files.
2018-06-11 18:58:38.994104 W | embed: The scheme of peer url http://localhost:2380 is HTTP while client cert auth (--peer-client-cert-auth) is enabled. Ignored client cert auth for this url.

But after chatting on Slack, this is what we came up with that fixes this specific issue. IMO this is an etcd issue, to not be able to autodetect certs and adjust the scheme to use accordingly but that's an other discussion.

@jberkus

This comment has been minimized.

Copy link

jberkus commented Jun 11, 2018

Does this change need docs?

@luxas

This comment has been minimized.

Copy link
Member

luxas commented Jun 11, 2018

@jberkus no, I don't think so. It's a pure bugfix. If anything (we're gonna investigate), we might wanna backport it to v1.10.x if the issue exists there as well. etcd really should detect the default scheme automatically :/

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 12, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

kubeadm - local etcd configuration bugfixes
- Set peer urls for default etcd instance to avoid leaving peer port unsecured
- Update generated etcd peer certificate SANs to include localhost
- Use a proper CN for etcd server and peer certificates
- Increase the manifest update timeout

@detiber detiber force-pushed the detiber:setPeerURLs branch from 4aa02eb to 793a51c Jun 12, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed lgtm size/S labels Jun 12, 2018

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented Jun 12, 2018

@luxas @timothysc Updated to fix the test issues, use a proper CN for the etcd certificates, ensuring the CN is included in the SANs for etcd certificates, and increasing the manifest deployment timeout.

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented Jun 12, 2018

@luxas backporting to v1.10 will not be entirely straightforward.

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented Jun 12, 2018

/test pull-kubernetes-e2e-gce

@detiber

This comment has been minimized.

Copy link
Member Author

detiber commented Jun 12, 2018

/test pull-kubernetes-e2e-gce

@timothysc
Copy link
Member

timothysc left a comment

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, luxas, 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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 12, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@detiber @luxas @timothysc

Pull Request Labels
  • sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help
@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 12, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@@ -42,7 +42,7 @@ import (
)

const (
upgradeManifestTimeout = 1 * time.Minute
upgradeManifestTimeout = 5 * time.Minute

This comment has been minimized.

@neolit123

neolit123 Jun 12, 2018

Member

i think it's best that we notify the user about the upgrade timeout in apply - in the lines of:
this operation will timeout in X minutes
or someone might decide to ctrl+c earlier thinking this has hanged.

This comment has been minimized.

@detiber

detiber Jun 12, 2018

Author Member

I created kubernetes/kubeadm#914 to track this.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 12, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 7f00fe4 into kubernetes:master Jun 12, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation detiber authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Jun 13, 2018

Kubernetes Submit Queue
Merge pull request #65029 from detiber/timeoutIncrease
Automatic merge from submit-queue.

kubeadm - increase upgrade manifest timeout

**What this PR does / why we need it**:

Backports the manifest upgrade timeout from #64988 to v1.10

**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#850

**Release note**:
```release-note
kubeadm - Increase the manifest upgrade timeout to make upgrades more reliable.
```
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.