don't mutate original master->kubelet TLS config #33141

liggitt · 2016-09-20T23:54:51Z

Resolves x509 verification issue with masters dialing nodes when started with --kubelet-certificate-authority

This change is

marun · 2016-09-21T00:09:48Z

Consider adding a test to prevent this regression from recurring.

liggitt · 2016-09-21T04:25:50Z

Updated to copy the struct field-by-field (which makes me cringe, but is what golang also ended up doing in net/http/cloneTLSClientConfig when copying tls.Config structs for client use). In go1.8, we can use tls.Config#Clone instead. Added a reflective test to catch drift on the field-by-field copy until then.

Added dial tests to make sure insecure, secured, and pinned name dials all work as expected, and don't mutate the transport's tls.Config

smarterclayton

One minor comment

smarterclayton · 2016-09-21T04:54:42Z

pkg/util/net/http.go

@@ -108,6 +108,33 @@ func Dialer(transport http.RoundTripper) (DialFunc, error) {
 	}
 }

+// CloneTLSConfig returns a tls.Config with all exported fields except SessionTicketsDisabled and SessionTicketKey copied.
+// This makes it safe to call CloneTLSConfig on a config in active use by a server.


Todo about replacing with Clone() in Go 1.8

liggitt · 2016-09-21T15:19:40Z

comment addressed

smarterclayton · 2016-09-21T16:15:05Z

LGTM

pwittrock · 2016-09-21T17:41:49Z

@k8s-bot gce e2e test this

pwittrock · 2016-09-21T17:42:04Z

@k8s-bot kubemark e2e test this

liggitt · 2016-09-21T18:34:42Z

#31570

@k8s-bot gce e2e test this

pwittrock · 2016-09-21T19:51:42Z

@k8s-bot gce e2e test this

liggitt · 2016-09-21T19:56:26Z

unrelated flake (passed in the cherry pick PR run, and isn't making a TLS Upgrade request, which is the only thing this PR would affect):

STEP: validating guestbook app
Sep 21 12:02:25.051: INFO: Waiting for all frontend pods to be Running.
Sep 21 12:02:55.053: INFO: Waiting for frontend to serve content.
Sep 21 12:02:55.089: INFO: Trying to add a new entry to the guestbook.
Sep 21 12:02:55.109: INFO: Verifying that added entry can be retrieved.
Sep 21 12:02:55.119: INFO: Failed to get response from guestbook. err: <nil>, response: {"data": ""}
Sep 21 12:03:00.138: INFO: Failed to get response from guestbook. err: <nil>, response: {"data": ""}
Sep 21 12:03:05.149: INFO: Failed to get response from guestbook. err: <nil>, response: {"data": ""}
Sep 21 12:03:10.154: INFO: Failed to get response from guestbook. err: an error on the server ("Error: 'EOF'\nTrying to reach: 'http://10.180.4.95:80/guestbook.php?cmd=get&key=messages&value='") has prevented the request from succeeding (get services frontend), response: 
Sep 21 12:03:15.169: INFO: Failed to get response from guestbook. err: <nil>, response: {"data": ""}
Sep 21 12:03:20.174: INFO: Failed to get response from guestbook. err: an error on the server ("Error: 'EOF'\nTrying to reach: 'http://10.180.4.95:80/guestbook.php?cmd=get&key=messages&value='") has prevented the request from succeeding (get services frontend), response: 
Sep 21 12:03:25.199: INFO: Failed to get response from guestbook. err: <nil>, response: {"data": ""}

from the master log, showing the DialURL function wasn't in the error path:

I0921 19:03:10.164479       5 handlers.go:163] GET /api/v1/namespaces/e2e-tests-kubectl-9jl4i/services/frontend/proxy/guestbook.php?cmd=get&key=messages&value=: (1.699288ms) 503
goroutine 238596 [running]:
k8s.io/kubernetes/pkg/httplog.(*respLogger).recordStatus(0xc8211988c0, 0x1f7)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/httplog/log.go:219 +0xa3
k8s.io/kubernetes/pkg/httplog.(*respLogger).WriteHeader(0xc8211988c0, 0x1f7)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/httplog/log.go:198 +0x2b
k8s.io/kubernetes/pkg/apiserver/metrics.(*responseWriterDelegator).WriteHeader(0xc824fc1bc0, 0x1f7)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/metrics/metrics.go:117 +0x4c
net/http/httputil.(*ReverseProxy).ServeHTTP(0xc82465b180, 0x7ff9845c8ef8, 0xc821481c48, 0xc82353d960)
    /usr/local/go/src/net/http/httputil/reverseproxy.go:233 +0xcd1
k8s.io/kubernetes/pkg/registry/generic/rest.(*UpgradeAwareProxyHandler).ServeHTTP(0xc8269fe870, 0x7ff9845c8ef8, 0xc821481c48, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/registry/generic/rest/proxy.go:125 +0x8d9
k8s.io/kubernetes/pkg/apiserver.ConnectResource.func1(0xc824fc1ad0, 0xc823186d20)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/resthandler.go:214 +0xc1b
k8s.io/kubernetes/pkg/apiserver/metrics.InstrumentRouteFunc.func1(0xc824fc1ad0, 0xc823186d20)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/metrics/metrics.go:101 +0x25f
k8s.io/kubernetes/vendor/github.com/emicklei/go-restful.(*Container).dispatch(0xc8206a0cf0, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/github.com/emicklei/go-restful/container.go:272 +0xf30
k8s.io/kubernetes/vendor/github.com/emicklei/go-restful.(*Container).(k8s.io/kubernetes/vendor/github.com/emicklei/go-restful.dispatch)-fm(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/github.com/emicklei/go-restful/container.go:120 +0x3e
net/http.HandlerFunc.ServeHTTP(0xc8203f5df0, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
net/http.(*ServeMux).ServeHTTP(0xc8201ae4e0, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1910 +0x17d
k8s.io/kubernetes/pkg/apiserver.WithAuthorizationCheck.func1(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/handlers.go:477 +0x36c
net/http.HandlerFunc.ServeHTTP(0xc8206a8680, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
k8s.io/kubernetes/pkg/apiserver.WithImpersonation.func1(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/handler_impersonation.go:44 +0x2b4
net/http.HandlerFunc.ServeHTTP(0xc8206a86c0, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
k8s.io/kubernetes/pkg/auth/handlers.NewRequestAuthenticator.func1(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/auth/handlers/handlers.go:66 +0x2c9
net/http.HandlerFunc.ServeHTTP(0xc820695130, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
k8s.io/kubernetes/pkg/api.NewRequestContextFilter.func1(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/api/requestcontext.go:101 +0x157
net/http.HandlerFunc.ServeHTTP(0xc8206aab40, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
k8s.io/kubernetes/pkg/api.NewRequestContextFilter.func1(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/api/requestcontext.go:101 +0x157
net/http.HandlerFunc.ServeHTTP(0xc8206aab60, 0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
k8s.io/kubernetes/pkg/apiserver.RecoverPanics.func1(0x7ff9845c8e10, 0xc8211988c0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/handlers.go:162 +0x1f5
net/http.HandlerFunc.ServeHTTP(0xc821944780, 0x7ff9845c8d10, 0xc82402c5b0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
k8s.io/kubernetes/pkg/apiserver.(*timeoutHandler).ServeHTTP(0xc8219447a0, 0x7ff9845c8d10, 0xc82402c5b0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/handlers.go:188 +0xb4
k8s.io/kubernetes/pkg/apiserver.MaxInFlightLimit.func1(0x7ff9845c8d10, 0xc82402c5b0, 0xc82353d7a0)
    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/apiserver/handlers.go:113 +0x86
net/http.HandlerFunc.ServeHTTP(0xc821911110, 0x7ff9845c8d10, 0xc82402c5b0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:1618 +0x3a
net/http.serverHandler.ServeHTTP(0xc8218f6900, 0x7ff9845c8d10, 0xc82402c5b0, 0xc82353d7a0)
    /usr/local/go/src/net/http/server.go:2081 +0x19e
net/http.(*conn).serve(0xc829b45180)
    /usr/local/go/src/net/http/server.go:1472 +0xf2e
created by net/http.(*Server).Serve
    /usr/local/go/src/net/http/server.go:2137 +0x44e

logging error output: "Error: 'EOF'\nTrying to reach: 'http://10.180.4.95:80/guestbook.php?cmd=get&key=messages&value='"

pwittrock · 2016-09-21T20:07:49Z

What is the cherrypick PR?

liggitt · 2016-09-21T20:08:07Z

What is the cherrypick PR?

#33187

…1-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #33141 Cherry pick of #33141 on release-1.4. ```release-note Resolves x509 verification issue with masters dialing nodes when started with --kubelet-certificate-authority ```

k8s-ci-robot · 2016-09-21T22:39:39Z

Jenkins GCE e2e failed for commit f3c8004.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

liggitt · 2016-09-22T00:40:56Z

#33175 in https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/33141/kubernetes-pull-build-test-e2e-gce/59982/

@k8s-bot gce e2e test this

k8s-github-robot · 2016-09-22T01:43:33Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-09-22T02:22:46Z

Automatic merge from submit-queue

k8s-cherrypick-bot · 2016-09-22T02:23:29Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…k-of-#33141-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#33141 Cherry pick of kubernetes#33141 on release-1.4. ```release-note Resolves x509 verification issue with masters dialing nodes when started with --kubelet-certificate-authority ```

googlebot added the cla: yes label Sep 20, 2016

k8s-github-robot assigned smarterclayton Sep 21, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Sep 21, 2016

liggitt changed the title ~~don't mutate original TLS config~~ don't mutate original master->kubelet TLS config Sep 21, 2016

liggitt added the cherrypick-candidate label Sep 21, 2016

liggitt added this to the v1.4 milestone Sep 21, 2016

liggitt mentioned this pull request Sep 21, 2016

"oc exec" is wrongly caching pod-to-node name/certificate/? mappings openshift/origin#11025

Closed

liggitt force-pushed the tls-config-mutation branch from e6e247f to dfff9f4 Compare September 21, 2016 04:00

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 21, 2016

liggitt force-pushed the tls-config-mutation branch from dfff9f4 to 9e604ce Compare September 21, 2016 04:12

liggitt added 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. and removed release-note-label-needed labels Sep 21, 2016

liggitt mentioned this pull request Sep 21, 2016

Dialing different nodes breaks TLS-verified master<->kubelet connections #33140

Closed

liggitt force-pushed the tls-config-mutation branch from 9e604ce to 6c49a16 Compare September 21, 2016 04:54

smarterclayton reviewed Sep 21, 2016

View reviewed changes

don't mutate original client TLS config

f3c8004

liggitt force-pushed the tls-config-mutation branch from 6c49a16 to f3c8004 Compare September 21, 2016 04:57

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2016

liggitt mentioned this pull request Sep 21, 2016

Automated cherry pick of #33141 #33187

Merged

pwittrock added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 21, 2016

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 21, 2016

k8s-github-robot merged commit fee69ee into kubernetes:master Sep 22, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 22, 2016

liggitt deleted the tls-config-mutation branch September 22, 2016 03:52

enj mentioned this pull request Jan 5, 2017

go vet check failed openshift/origin#12393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't mutate original master->kubelet TLS config #33141

don't mutate original master->kubelet TLS config #33141

liggitt commented Sep 20, 2016 •

edited

Loading

marun commented Sep 21, 2016

liggitt commented Sep 21, 2016 •

edited

Loading

smarterclayton left a comment

smarterclayton Sep 21, 2016

liggitt Sep 21, 2016

liggitt commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

pwittrock commented Sep 21, 2016

pwittrock commented Sep 21, 2016

liggitt commented Sep 21, 2016

pwittrock commented Sep 21, 2016

liggitt commented Sep 21, 2016 •

edited

Loading

pwittrock commented Sep 21, 2016

liggitt commented Sep 21, 2016 •

edited

Loading

k8s-ci-robot commented Sep 21, 2016

liggitt commented Sep 22, 2016 •

edited

Loading

k8s-github-robot commented Sep 22, 2016

k8s-github-robot commented Sep 22, 2016

k8s-cherrypick-bot commented Sep 22, 2016

don't mutate original master->kubelet TLS config #33141

don't mutate original master->kubelet TLS config #33141

Conversation

liggitt commented Sep 20, 2016 • edited Loading

marun commented Sep 21, 2016

liggitt commented Sep 21, 2016 • edited Loading

smarterclayton left a comment

Choose a reason for hiding this comment

smarterclayton Sep 21, 2016

Choose a reason for hiding this comment

liggitt Sep 21, 2016

Choose a reason for hiding this comment

liggitt commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

pwittrock commented Sep 21, 2016

pwittrock commented Sep 21, 2016

liggitt commented Sep 21, 2016

pwittrock commented Sep 21, 2016

liggitt commented Sep 21, 2016 • edited Loading

pwittrock commented Sep 21, 2016

liggitt commented Sep 21, 2016 • edited Loading

k8s-ci-robot commented Sep 21, 2016

liggitt commented Sep 22, 2016 • edited Loading

k8s-github-robot commented Sep 22, 2016

k8s-github-robot commented Sep 22, 2016

k8s-cherrypick-bot commented Sep 22, 2016

liggitt commented Sep 20, 2016 •

edited

Loading

liggitt commented Sep 21, 2016 •

edited

Loading

liggitt commented Sep 21, 2016 •

edited

Loading

liggitt commented Sep 21, 2016 •

edited

Loading

liggitt commented Sep 22, 2016 •

edited

Loading