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

Configure the kubelet to use HTTPS #6243

Merged
merged 3 commits into from
Apr 1, 2015
Merged

Configure the kubelet to use HTTPS #6243

merged 3 commits into from
Apr 1, 2015

Conversation

roberthbailey
Copy link
Contributor

As of now, the master connects to the kubelet without verifying the certificate presented by the kubelet.

This is step 1 from #3168.

@roberthbailey
Copy link
Contributor Author

/cc @erictune @liggitt

roberthbailey
roberthbailey reviewed Mar 31, 2015
View reviewed changes
pkg/apiserver/validator_test.go
t.Errorf("unexpected error: %v", err)
}
components := util.StringSet{}
for _, s := range status {
if s.Err != "nil" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, this test was passing before when it should have been failing because we weren't checking for error in the sub-components. The test would pass after 30 seconds (hitting the url fetch timeout) because it had a result for each component -- but the result was a failure.

Copy link
Member

Choose a reason for hiding this comment

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

good catch

@roberthbailey roberthbailey force-pushed the kubelet-ssl branch 2 times, most recently from 7c7a011 to 89cd1ba Compare March 31, 2015 23:23
erictune
erictune reviewed Apr 1, 2015
View reviewed changes
cmd/kubelet/app/server.go
tlsOptions := &kubelet.TLSOptions{
Config: &tls.Config{
// Change default from SSLv3 to TLSv1.0 (because of POODLE vulnerability).
MinVersion: tls.VersionTLS10,
Copy link
Member

Choose a reason for hiding this comment

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

FYI @liggitt @hmrm you were both interested in this in #2799

Copy link
Member

Choose a reason for hiding this comment

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

That'll just request, but not require (or validate) client certs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I didn't want to also require the master to send client certs as part of the same change. This could probably be left out since we aren't doing anything with certs yet, but I don't think it hurts to leave it in.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, just noting it

@erictune
Copy link
Member

erictune commented Apr 1, 2015

IIUC, this PR is additionally changing the master to use TLS when validating the scheduler and the controller-manager? Do mention that in the commit description somewhere.

@roberthbailey
Copy link
Contributor Author

@erictune The second commit's description is "Configure the master to connect to the kubelet using HTTPS.". If you approve this change, I plan to squash the commits into one and that will certainly be part of the commit log.

I had to change the master to use TLS in a couple of places when contacting the Kubelet. The more difficult one was in the /validate endpoint on the master, which previously was using a raw http.Client to connect to each listed endpoint to fetch it's status. It now uses TLS when connecting to Kubelets and a regular http client when connecting to other URLs.

@erictune
Copy link
Member

erictune commented Apr 1, 2015

Okay, was confused, your explanation helped. Thanks.

@erictune
Copy link
Member

erictune commented Apr 1, 2015

Code LGTM.
Thinking about how this works with upgrades and for other cloud providers.

@erictune
Copy link
Member

erictune commented Apr 1, 2015

Wondering whether ContainerVM users may be depending on ability to connect to Kubelet over HTTP. Suggest flag to disable it this case.

@roberthbailey
Copy link
Contributor Author

It should be the same for all cloud providers: kubelet uses TLS. As for upgrades, we will need to think about how we manage the certs (provided by startup scripts or generated on demand).

@erictune
Copy link
Member

erictune commented Apr 1, 2015

If someone upgrades their kubelets from pre-this-PR to post-this-PR, then they just lose the ability to /validate the kubelets, which presumably they aren't using after the cluster is setup?
Same thing if the master is upgraded first. That seems okay.

@roberthbailey
Copy link
Contributor Author

Wondering whether ContainerVM users may be depending on ability to connect to Kubelet over HTTP. Suggest flag to disable it this case.

Until we add client cert checking, you can still connect over HTTPS if you do curl --insecure or configure your client to ignore the self signed cert. Or you can plumb in a valid cert that your client trusts.

@erictune
Copy link
Member

erictune commented Apr 1, 2015

LGTM

erictune added a commit that referenced this pull request Apr 1, 2015
@erictune
Merge pull request #6243 from roberthbailey/kubelet-ssl
92b6f49 
Configure the kubelet to use HTTPS
@erictune erictune merged commit 92b6f49 into kubernetes:master Apr 1, 2015
@hmrm
Copy link
Contributor

hmrm commented Apr 1, 2015

@jseaidou @gkeramidas FYI

@zmerlynn
Copy link
Member

zmerlynn commented Apr 1, 2015

I believe this PR broke Monitoring verify monitoring pods and all cluster nodes are available on influxdb using heapster.. It was the only PR that landed in the build, and now the test fails with:

INFO: failed to query list of pods from influxdb. Query: "select distinct(pod_id) from /cpu.*/", Err: Server returned (500): {
  "kind": "Status",
  "creationTimestamp": null,
  "apiVersion": "v1beta1",
  "status": "Failure",
  "message": "no endpoints available for \"monitoring-influxdb\"",
  "code": 500
}

@roberthbailey
Copy link
Contributor Author

I just chatted with @vishh and it definitely broke heapster's ability to collect stats from the kubelet. Rolling it back....

This was referenced Apr 1, 2015
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erictune erictune

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@roberthbailey @erictune @hmrm @zmerlynn @liggitt @googlebot