Update http.Transport if it already exists in ExecProvider #66395

awly · 2018-07-19T19:00:15Z

What this PR does / why we need it:
This unbreaks ExecPlugin. Without the change, we hit this error
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/transport/transport.go#L32

Release note:

Fix kubelet startup failure when using ExecPlugin in kubeconfig

awly · 2018-07-19T19:00:40Z

/assign @mikedanese
/cc @liggitt

liggitt · 2018-07-19T19:20:49Z

/hold
this reverts the fix in #63492 and removes the ability to tear down kubelet connections on heartbeat failure. it's fine if we want to compose these two things differently, but we cannot reintroduce the kubelet TCP heartbeat hang issue.

awly · 2018-07-19T21:51:36Z

@liggitt good point, not sure how i forgot about that :|

Changed to handle existing Transport in ExecProvider and use stdlib GetCertificate version.
WDYT?

liggitt · 2018-07-20T16:30:13Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

-			cert, err := getCert()
-			if err != nil {
-				return nil, err
+	if t, ok := c.Transport.(*http.Transport); ok && t != nil {


what does this do when run with --v=6? might need a func TransportFor(transport http.RoundTripper) (*http.Transport, error) helper in pkg/util/net to unwrap nested debugging transports (see DialerFor as an example)

Just tested, --v=6 works fine.
But I agree with your point, someone can introduce a wrapper eventually, breaking this.
Added a TransportFor helper.

liggitt · 2018-07-20T16:30:57Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+		}
+		getCert := t.TLSClientConfig.GetClientCertificate
+		t.TLSClientConfig.GetClientCertificate = func(hi *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+			// If previous GetCert is present and returns a valid non-nil


trying to think through the implications if you use client cert rotation and an exec plugin... the client cert rotation code would always win. does that make sense?

where are we checking if the returned cert is valid?

where are we checking if the returned cert is valid?

refreshCredsLocked does validation. It fetches the actual certificate, validates and caches it. GetClientCertificate returns the cached copy.
I added cert expiry validation too in this PR, just in case.

trying to think through the implications if you use client cert rotation and an exec plugin... the client cert rotation code would always win. does that make sense?

Is cert rotation and exec plugin a valid configuration? Does it mean "fetch initial cert via plugin but then rotate manually"?
Our use case relies on exec plugin to bootstrap a fresh cert whenever needed and cert rotation in kubelet is disabled.

refreshCredsLocked does validation.

I was referring to what the previous GetCert was returning ("If previous GetCert is present and returns a valid non-nil..."), not what the exec plugin returned

Is cert rotation and exec plugin a valid configuration?

I don't know, but this is allowing it :)

I was referring to what the previous GetCert was returning ("If previous GetCert is present and returns a valid non-nil..."), not what the exec plugin returned

Ah, gotcha. I don't think it's this plugin's job to check that. If something returns a certificate here, it better be valid. Otherwise it's a bug.

I'd rather leave it as is, WDYT?

For the sanity of the next person who tries to debug this, I think it would be better to return an error if GetClientCertificate is not nil. That would effectively disallow client cert rotation to be used with the exec plugin, right? I'd be happy to avoid this complexity.

I think it would be better to return an error if GetClientCertificate is not nil. That would effectively disallow client cert rotation to be used with the exec plugin, right? I'd be happy to avoid this complexity.

that seems like a reasonable starting point. mixing the two gets weird.

SGTM, made it an error in both cases

liggitt · 2018-07-20T16:31:26Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+			if getCert != nil {
+				cert, err := getCert(hi)
+				if err != nil {
+					return nil, err


would we want to check the exec plugin in an error case? (I know this is pre-existing, just trying to understand the implications and whether they are different if we're dealing with an existing transport vs a GetCert func)

Makes sense to me, added.
It's an unlikely configuration, but doesn't hurt.

awly · 2018-07-20T20:57:39Z

@liggitt thanks for the comments, PTAL

awly · 2018-07-20T20:58:23Z

FYI, I'd like to cherry-pick this into 1.11.2.

mikedanese

The rest client keeps on giving. What if we added a WrapDialer field to the restclient.Config and used that to add the connection tracker? Would that be simpler?

mikedanese · 2018-07-24T13:53:29Z

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

+
+// TransportFor returns the root *http.Transport behind all wrapper
+// RoundTrippers. Wrappers must implement RoundTripperWrapper.
+func TransportFor(transport http.RoundTripper) (*http.Transport, error) {
 	if transport == nil {


when does this happen?

This shifts the nil check from every caller to here. transport.Config.Transport can be nil with valid config, afaik (just not in kubelet)

mikedanese · 2018-07-24T14:05:04Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+		}
+		getCert := t.TLSClientConfig.GetClientCertificate
+		t.TLSClientConfig.GetClientCertificate = func(hi *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+			// If previous GetCert is present and returns a valid non-nil


For the sanity of the next person who tries to debug this, I think it would be better to return an error if GetClientCertificate is not nil. That would effectively disallow client cert rotation to be used with the exec plugin, right? I'd be happy to avoid this complexity.

mikedanese · 2018-07-24T14:08:41Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+		if certBlock == nil {
+			return errors.New("can't parse client certificate returned by exec plugin as PEM block")
+		}
+		x509Cert, err := x509.ParseCertificate(certBlock.Bytes)


Use the cert on line 402 so you don't have to parse the pem twice.

You mean manually parse ClientKeyData and build tls.Certificate via a literal?
It feels like more work and easy to miss some validations that tls.X509KeyPair does

liggitt · 2018-07-24T14:47:03Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+		if err != nil {
+			return fmt.Errorf("failed parsing x509 client certificate returned by exec plugin: %v", err)
+		}
+		if time.Now().After(x509Cert.NotAfter) {


are we already checking time client-side elsewhere? this makes us vulnerable to client clock skew

I don't know if we're checking it elsewhere. It should be kube-apiserver's responsibility to verify cert validity on incoming requests. Could you clarify how this makes us vulnerable?

awly · 2018-07-25T23:13:31Z

@liggitt @mikedanese PTAL. Turns out we can just set restclient.Config.Dialer instead of Transport. This fixes the issue in my test cluster.

mikedanese · 2018-07-25T23:19:46Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

-		}
-		return a.cert()
+	if c.TLS.GetCert != nil {
+		return errors.New("can't add TLC certificate callback: transport.Config.TLS.GetCert already set")


s/TLC/TLS/?

Instead of Transport. This fixes ExecPlugin, which fails if restclient.Config.Transport is set.

mikedanese · 2018-07-25T23:24:40Z

I'm reasonably happy with this.

/lgtm

awly · 2018-07-25T23:25:17Z

Moved cert expiry check out to #66632

mikedanese · 2018-07-26T13:15:07Z

/retest

mikedanese · 2018-07-26T15:39:38Z

/retest

liggitt · 2018-07-26T16:18:30Z

/lgtm
/hold cancel

k8s-ci-robot · 2018-07-26T16:18:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, liggitt, mikedanese

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:

~~pkg/kubelet/certificate/OWNERS~~ [liggitt,mikedanese]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]

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 · 2018-07-26T16:26:30Z

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

k8s-github-robot · 2018-07-26T17:47:05Z

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

…pstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #66395: Set connrotation dialer via restclient.Config.Dialer Cherry pick of #66395 on release-1.11. #66395: Set connrotation dialer via restclient.Config.Dialer

marpaia · 2018-07-31T22:12:04Z

/sig cli

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. labels Jul 19, 2018

k8s-ci-robot requested review from liggitt and vishh July 19, 2018 19:00

k8s-ci-robot assigned mikedanese Jul 19, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2018

awly force-pushed the fix-kubelet-exec-plugin-startup branch from 5262e8e to 4471282 Compare July 19, 2018 21:50

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2018

awly changed the title ~~Only call kubeletcertificate.UpdateConfig if cert rotation is enabled~~ Update http.Transport if it already exists in ExecProvider Jul 19, 2018

liggitt reviewed Jul 20, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 20, 2018

mikedanese reviewed Jul 24, 2018

View reviewed changes

liggitt reviewed Jul 24, 2018

View reviewed changes

awly force-pushed the fix-kubelet-exec-plugin-startup branch from 97ab95f to 527b23b Compare July 25, 2018 23:11

mikedanese reviewed Jul 25, 2018

View reviewed changes

awly force-pushed the fix-kubelet-exec-plugin-startup branch 2 times, most recently from e619672 to 7293fab Compare July 25, 2018 23:23

Set connrotation dialer via restclient.Config.Dialer

3357b5e

Instead of Transport. This fixes ExecPlugin, which fails if restclient.Config.Transport is set.

awly force-pushed the fix-kubelet-exec-plugin-startup branch from 7293fab to 3357b5e Compare July 25, 2018 23:24

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2018

k8s-ci-robot assigned liggitt Jul 26, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2018

k8s-github-robot merged commit cef2d32 into kubernetes:master Jul 26, 2018

awly mentioned this pull request Jul 26, 2018

Automated cherry pick of #66395: Set connrotation dialer via restclient.Config.Dialer #66683

Merged

awly deleted the fix-kubelet-exec-plugin-startup branch July 26, 2018 17:51

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jul 31, 2018

Update http.Transport if it already exists in ExecProvider #66395

Update http.Transport if it already exists in ExecProvider #66395

Conversation

awly commented Jul 19, 2018

awly commented Jul 19, 2018

liggitt commented Jul 19, 2018 • edited

awly commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jul 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Jul 20, 2018

awly commented Jul 20, 2018

mikedanese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Jul 25, 2018

mikedanese Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Jul 25, 2018

awly commented Jul 25, 2018

mikedanese commented Jul 26, 2018

mikedanese commented Jul 26, 2018

liggitt commented Jul 26, 2018

k8s-ci-robot commented Jul 26, 2018

k8s-github-robot commented Jul 26, 2018

k8s-github-robot commented Jul 26, 2018

marpaia commented Jul 31, 2018

liggitt commented Jul 19, 2018 •

edited

liggitt Jul 20, 2018 •

edited

mikedanese Jul 25, 2018 •

edited