Use SSH tunnel for webhook communication iff the webhook is deployed as a service #58644

yguo0905 · 2018-01-23T00:34:07Z

What this PR does / why we need it:

We are getting the following error when the apiserver connects the webhook on localhost (configured via URL). We should only use the SSL tunnel for the connections to nodes when the webhooks are running as services.

I0119 17:41:18.678436       1 ssh.go:400] [4cdf44753cc3705d: localhost:10258] Dialing...
W0119 17:41:18.678483       1 ssh.go:424] SSH tunnel not found for address "localhost", picking random node
I0119 17:41:18.679810       1 ssh.go:402] [4cdf44753cc3705d: localhost:10258] Dialed in 1.398691ms.
W0119 17:41:18.679928       1 admission.go:256] Failed calling webhook, failing closed xxx: failed calling admission webhook "xxx": Post xxx: ssh: rejected: connect failed (Connection refused)
I0119 17:41:18.680346       1 wrap.go:42] POST /api/v1/namespaces/kube-system/pods: (5.725588ms) 500

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 # #58779

Special notes for your reviewer:

Release note:

kube-apiserver is changed to use SSH tunnels for webhook iff the webhook is not directly routable from apiserver's network environment.

/assign @lavalamp @caesarxuchao @cheftako

caesarxuchao · 2018-01-23T01:52:42Z

lgtm.

@lavalamp could you also have a look?

yguo0905 · 2018-01-23T19:02:56Z

/test pull-kubernetes-e2e-kops-aws

lavalamp · 2018-01-23T22:12:49Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/config/authentication.go

@@ -31,17 +31,18 @@ import (
 // rest.Config generated by the resolver.
 type AuthenticationInfoResolverWrapper func(AuthenticationInfoResolver) AuthenticationInfoResolver

-// AuthenticationInfoResolver builds rest.Config base on the server name.
+// AuthenticationInfoResolver builds rest.Config base on the server name and
+// the isService flag indicating whether the webhook is deployed as a service.


The thing we want this name to highlight is whether the server is routable directly from apiserver's network environment. isService isn't the right name. Maybe indirectRouting? Note that the ssh tunnels are going away in less than a year, so please leave a TODO to remove this when they go away.

Done. Changed the variable name to directRouting and added TODO.

…as a service

caesarxuchao · 2018-01-24T22:43:04Z

/lgtm

lavalamp · 2018-01-25T00:11:12Z

/approve

liggitt · 2018-01-25T01:00:01Z

/hold
@kubernetes/sig-auth-pr-reviews

liggitt · 2018-01-25T01:01:12Z

/hold cancel
I thought this was updating the authentication webhook

liggitt · 2018-01-25T01:05:11Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/config/authentication.go

 type AuthenticationInfoResolver interface {
-	ClientConfigFor(server string) (*rest.Config, error)
+	ClientConfigFor(server string, directRouting bool) (*rest.Config, error)


More than just the kube apiserver will use this. We shouldn't change the interface just to accommodate kube apiserver

I think the new flag might be useful in other cases. Or do you mean the client of the AuthenticationInfoResolver should never care where/how the webhooks are deployed?

Open to other suggestions. Given that we do have the ssh tunnels right now this doesn't seem completely wrong to me.

since the API driving webhooks distinguishes between service name/namespace and url.URL, seems like it would be better to just make that the API here:

ClientConfig(server string) (*rest.Config, error) ClientConfigForService(serviceName, serviceNamespace string) (*rest.Config, error)

kube-apiserver's override would only inject the proxy dialer for ClientConfigForService

liggitt · 2018-01-25T01:06:33Z

cmd/kube-apiserver/app/server.go

 			if err != nil {
 				return nil, err
 			}
-			if proxyTransport != nil && proxyTransport.Dial != nil {


We should find a different way to signal this special handling for kube apiserver than polluting the interface for all consumers

An alternative is to check whether the server is a [host|ip]:port or not, but that seems a little bit hacky.

liggitt · 2018-01-25T01:24:28Z

/hold
on interface change

yguo0905 · 2018-01-26T07:01:31Z

/test pull-kubernetes-unit

yguo0905 · 2018-01-26T17:45:10Z

@liggitt and @lavalamp, please take a look at this proposed solution.

liggitt · 2018-01-26T17:57:43Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/config/authentication.go

+	// is used for the communication from master to nodes get removed.
+	//
+	// ClientConfigForService builds rest.Config based on the service name.
+	ClientConfigForService(service string) (*rest.Config, error)


should probably be serviceName, serviceNamespace string, and defaultAuthenticationInfoResolver#ClientConfigForService does the same serviceName.serviceNamespace.svc lookup.

liggitt · 2018-01-26T17:58:22Z

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/config/authentication.go

-	ClientConfigFor(server string) (*rest.Config, error)
+	// ClientConfigFor builds rest.Config based on the host.
+	ClientConfigFor(host string) (*rest.Config, error)
+	// TODO(yguo0905): Remove ClientConfigForService once the SSH tunnels that


I'd remove the TODO... the webhook registration API has URL and service reference... I'd anticipate both being represented here, even once the kube-apiserver tunneling dialer is not needed

liggitt · 2018-01-26T19:16:13Z

cmd/kube-apiserver/app/server.go

@@ -459,27 +459,30 @@ func BuildGenericConfig(s *options.ServerRunOptions, proxyTransport *http.Transp
 		genericConfig.DisabledPostStartHooks.Insert(rbacrest.PostStartHookName)
 	}

-	webhookAuthResolver := func(delegate webhookconfig.AuthenticationInfoResolver) webhookconfig.AuthenticationInfoResolver {
-		return webhookconfig.AuthenticationInfoResolverFunc(func(server string) (*rest.Config, error) {
-			if server == "kubernetes.default.svc" {


preserve this in ClientConfigForFunc:

ClientConfigForFunc: func(server string) (*rest.Config, error) { if server == "kubernetes.default.svc" { return genericConfig.LoopbackClientConfig, nil } return delegate.ClientConfigFor(server) }

I think the server will be [host|ip]:port in ClientConfigForFunc so this would never be true?

https://kubernetes.default.svc is a valid URL that would parse to that host. Just wanted consistency between the two

You're right. I thought the port is required in the URL but it's not.

liggitt · 2018-01-26T19:16:26Z

one comment, lgtm otherwise

liggitt · 2018-01-26T20:12:29Z

/lgtm
/hold cancel

k8s-ci-robot · 2018-01-26T20:12:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp, liggitt, yguo0905

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [caesarxuchao,lavalamp,liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp,liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

yguo0905 · 2018-01-26T22:15:16Z

/retest

k8s-github-robot · 2018-01-26T23:17:04Z

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

k8s-github-robot · 2018-01-26T23:58:27Z

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

…44-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #58644: Use SSH tunnel for webhook communication iff the webhook is deployed as service Cherry pick of #58644 on release-1.9. #58644: Use SSH tunnel for webhook communication iff the webhook

k8s-ci-robot assigned caesarxuchao and cheftako Jan 23, 2018

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 23, 2018

k8s-ci-robot assigned lavalamp Jan 23, 2018

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 23, 2018

k8s-ci-robot requested review from derekwaynecarr and timothysc January 23, 2018 00:34

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2018

lavalamp reviewed Jan 23, 2018

View reviewed changes

lavalamp removed their assignment Jan 23, 2018

timothysc removed their request for review January 23, 2018 22:25

Use SSH tunnel for webhook communication iff the webhook is deployed …

454276c

…as a service

yguo0905 force-pushed the webhooks branch from 139c0d8 to 454276c Compare January 23, 2018 22:32

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

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jan 25, 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 Jan 25, 2018

liggitt reviewed Jan 25, 2018

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 25, 2018

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2018

yguo0905 force-pushed the webhooks branch from e71f445 to 994ce08 Compare January 26, 2018 06:01

liggitt reviewed Jan 26, 2018

View reviewed changes

yguo0905 force-pushed the webhooks branch from 994ce08 to 3a8767c Compare January 26, 2018 19:08

liggitt reviewed Jan 26, 2018

View reviewed changes

Split ClientConfigFor()

05fbc22

yguo0905 force-pushed the webhooks branch from 3a8767c to 05fbc22 Compare January 26, 2018 19:51

k8s-ci-robot assigned liggitt Jan 26, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 26, 2018

k8s-github-robot merged commit ac495f1 into kubernetes:master Jan 26, 2018

yguo0905 deleted the webhooks branch January 27, 2018 00:47

yguo0905 mentioned this pull request Jan 27, 2018

Automated cherry pick of #58644: Use SSH tunnel for webhook communication iff the webhook is deployed as service #58897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SSH tunnel for webhook communication iff the webhook is deployed as a service #58644

Use SSH tunnel for webhook communication iff the webhook is deployed as a service #58644

yguo0905 commented Jan 23, 2018 •

edited

Loading

caesarxuchao commented Jan 23, 2018

yguo0905 commented Jan 23, 2018

lavalamp Jan 23, 2018

yguo0905 Jan 23, 2018

caesarxuchao commented Jan 24, 2018

lavalamp commented Jan 25, 2018

liggitt commented Jan 25, 2018

liggitt commented Jan 25, 2018

liggitt Jan 25, 2018

yguo0905 Jan 25, 2018

lavalamp Jan 25, 2018

liggitt Jan 25, 2018

liggitt Jan 25, 2018

yguo0905 Jan 25, 2018

liggitt commented Jan 25, 2018

yguo0905 commented Jan 26, 2018

yguo0905 commented Jan 26, 2018

liggitt Jan 26, 2018

liggitt Jan 26, 2018

liggitt Jan 26, 2018

yguo0905 Jan 26, 2018

liggitt Jan 26, 2018

yguo0905 Jan 26, 2018

liggitt commented Jan 26, 2018

liggitt commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

yguo0905 commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

Use SSH tunnel for webhook communication iff the webhook is deployed as a service #58644

Use SSH tunnel for webhook communication iff the webhook is deployed as a service #58644

Conversation

yguo0905 commented Jan 23, 2018 • edited Loading

caesarxuchao commented Jan 23, 2018

yguo0905 commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Jan 24, 2018

lavalamp commented Jan 25, 2018

liggitt commented Jan 25, 2018

liggitt commented Jan 25, 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

liggitt commented Jan 25, 2018

yguo0905 commented Jan 26, 2018

yguo0905 commented Jan 26, 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

liggitt commented Jan 26, 2018

liggitt commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

yguo0905 commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

k8s-github-robot commented Jan 26, 2018

yguo0905 commented Jan 23, 2018 •

edited

Loading