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

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

Merged
merged 2 commits into from Jan 26, 2018

Conversation

yguo0905
Copy link
Contributor

@yguo0905 yguo0905 commented Jan 23, 2018

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

@k8s-ci-robot 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 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 k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2018
@caesarxuchao
Copy link
Member

lgtm.

@lavalamp could you also have a look?

@yguo0905
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@lavalamp lavalamp removed their assignment Jan 23, 2018
@timothysc timothysc removed their request for review January 23, 2018 22:25
@caesarxuchao
Copy link
Member

/lgtm

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

/approve

@liggitt
Copy link
Member

liggitt commented Jan 25, 2018

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

@k8s-ci-robot 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
@liggitt
Copy link
Member

liggitt commented Jan 25, 2018

/hold cancel
I thought this was updating the authentication webhook

@k8s-ci-robot 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
type AuthenticationInfoResolver interface {
ClientConfigFor(server string) (*rest.Config, error)
ClientConfigFor(server string, directRouting bool) (*rest.Config, error)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

if err != nil {
return nil, err
}
if proxyTransport != nil && proxyTransport.Dial != nil {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@liggitt
Copy link
Member

liggitt commented Jan 25, 2018

/hold
on interface change

@k8s-ci-robot 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 k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2018
@yguo0905
Copy link
Contributor Author

/test pull-kubernetes-unit

@yguo0905
Copy link
Contributor Author

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

// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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" {
Copy link
Member

Choose a reason for hiding this comment

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

preserve this in ClientConfigForFunc:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@liggitt
Copy link
Member

liggitt commented Jan 26, 2018

one comment, lgtm otherwise

@liggitt
Copy link
Member

liggitt commented Jan 26, 2018

/lgtm
/hold cancel

@k8s-ci-robot 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-ci-robot
Copy link
Contributor

[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:

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

@yguo0905
Copy link
Contributor Author

/retest

@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

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 ac495f1 into kubernetes:master Jan 26, 2018
@yguo0905 yguo0905 deleted the webhooks branch January 27, 2018 00:47
k8s-github-robot pushed a commit that referenced this pull request Jan 27, 2018
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants