Plumbing the proxy dialer to the webhook admission plugin; Fix the tls config #50476

caesarxuchao · 2017-08-10T22:08:00Z

Fixing Plumb the proxy dialer to the webhook admission plugin #49987. Plumb the Dial function to the transport.Config
Fixing Webhook admission plugin should set ServerName in its tls config #52366. Let the webhook admission plugin sets the TLSConfg.ServerName.

I tested it in my gke setup. I don't have time to implement an e2e test before 1.8 release. I think it's ok to add the test later, because i) the change only affects the alpha webhook admission feature, and ii) the webhook feature is unusable without the fix. That said, it's up to my reviewer to decide.

Filed #52368 for the missing e2e test.

( The second commit is #52372, which is just a cleanup of client configuration in e2e tests. It removed a function that marshalled the client config to json and then unmarshalled it. It is a prerequisite of this PR, because this PR added the Dial function to the config which is not json marshallable.)

Fixed the webhook admission plugin so that it works even if the apiserver and the nodes are in two networks (e.g., in GKE).
Fixed the webhook admission plugin so that webhook author could use the DNS name of the service as the CommonName when generating the server cert for the webhook.

Action required:
Anyone who generated server cert for admission webhooks need to regenerate the cert. Previously, when generating server cert for the admission webhook, the CN value doesn't matter. Now you must set it to the DNS name of the webhook service, i.e., `<service.Name>.<service.Namespace>.svc`.

caesarxuchao · 2017-08-10T22:09:38Z

Still a WIP, because I don't know what's the proper way to test it.
@mandarjog when you said you could help testing it, you meant you could test it after the code is rolled out in gke, right?

caesarxuchao · 2017-08-10T22:09:45Z

/release-note-none
/sig api-machinery

cheftako · 2017-08-16T22:19:43Z

This seems somewhat GKE specific. Can we add apimachinery e2e test for at least one cloud provider which uses the dialer (GKE) and one which does not (GCE)?

caesarxuchao · 2017-09-12T04:51:48Z

The fix is not working yet.

$ kubectl get endpoints
webhook      10.32.0.5:8000     15m

API server log:

I0912 04:42:13.119439       5 ssh.go:400] [3971e1b2076ea996: 10.32.0.5:8000] Dialing...
W0912 04:42:13.119461       5 ssh.go:424] SSH tunnel not found for address "10.32.0.5", picking random node
I0912 04:42:13.121601       5 ssh.go:402] [3971e1b2076ea996: 10.32.0.5:8000] Dialed in 2.163495ms.
W0912 04:42:13.122779       5 admission.go:181] Failed calling webhook pod-image.k8s.io: failed calling admission webhook "pod-image.k8s.io": Post http://10.32.0.5:8000/: malformed HTTP response "\x15\x03\x01\x00\x02\x02\x16"
E0912 04:42:13.122808       5 admission.go:182] failed calling admission webhook "pod-image.k8s.io": Post http://10.32.0.5:8000/: malformed HTTP response "\x15\x03\x01\x00\x02\x02\x16"

http: TLS handshake error from 10.128.0.3:55796: tls: oversized record received with length 21536

caesarxuchao · 2017-09-12T05:28:01Z

Reproduced in the unit test.

caesarxuchao · 2017-09-12T22:11:36Z

Got it working in my local gke setup. I'll try to get together an gke e2e test, but no promise at the moment.

k8s-ci-robot · 2017-09-12T22:29:41Z

@caesarxuchao: Your pull request title starts with "[WIP]", so the do-not-merge/work-in-progress label will be added.

This label will ensure that your pull request will not be merged. Remove the prefix from your pull request title to trigger the removal of the label and allow for your pull request to be merged.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cheftako

Need to look at the old certs customers may have had.

cheftako · 2017-09-12T23:50:28Z

plugin/pkg/admission/webhook/admission.go

-			CAData:   h.ClientConfig.CABundle,
-			CertData: a.clientCert,
-			KeyData:  a.clientKey,
+			ServerName: h.ClientConfig.Service.Name + "." + h.ClientConfig.Service.Namespace + "." + "svc",


Is this backward compatible for old clients?

Not a big deal but do we really need to do "." + "svc" vs ".svc"

For 1, It's not backward compatible. Added "Action Required" in the release note.

Will fix 2.

lavalamp · 2017-09-13T21:47:50Z

cmd/kube-apiserver/app/server.go

@@ -510,6 +510,7 @@ func BuildAdmissionPluginInitializer(s *options.ServerRunOptions, client interna
 	}

 	pluginInitializer = pluginInitializer.SetServiceResolver(serviceResolver)
+	pluginInitializer = pluginInitializer.SetProxyTransport(proxyTransport)


I was kinda hoping to combine the service resolver with this proxy transport. But maybe it's not important. hm.

Do you want them to be combined because both of them are needed iff an admission plugin wants to contact a service provided by the user? I can put them into a single struct if you think it's necessary.

lavalamp · 2017-09-14T17:42:51Z

/approve

k8s-github-robot · 2017-09-14T17:43:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 49987

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]
~~pkg/kubeapiserver/OWNERS~~ [lavalamp]
~~plugin/pkg/admission/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao,lavalamp]
~~test/OWNERS~~ [lavalamp]

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

set the ServerName in the config for webhook admission plugin.

fix a fuzzer;

caesarxuchao · 2017-09-14T22:37:37Z

Rebased and squashed. Adding back the label.

caesarxuchao · 2017-09-14T23:48:28Z

/retest

caesarxuchao · 2017-09-15T00:54:24Z

/retest

caesarxuchao · 2017-09-15T02:47:00Z

/retest

caesarxuchao · 2017-09-15T03:59:40Z

/retest

k8s-ci-robot · 2017-09-15T05:47:47Z

@caesarxuchao: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`5ac3afe`	link	`/test pull-kubernetes-bazel`
pull-kubernetes-e2e-gce-bazel	`856a1db`	link	`/test pull-kubernetes-e2e-gce-bazel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-09-15T08:07:59Z

Automatic merge from submit-queue (batch tested with PRs 51824, 50476, 52451, 52009, 52237)

prabushyam · 2017-09-22T23:11:30Z

@caesarxuchao: Could we cherry pick this into 1.7 ?

kubernetes/kubernetes#50476

caesarxuchao assigned cheftako Aug 10, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Aug 10, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed release-note-label-needed labels Aug 10, 2017

caesarxuchao force-pushed the plumb-proxy branch from 4c7cd44 to 5ac3afe Compare August 10, 2017 23:19

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2017

caesarxuchao mentioned this pull request Aug 23, 2017

Plumb the proxy dialer to the webhook admission plugin #49987

Closed

caesarxuchao force-pushed the plumb-proxy branch from 5ac3afe to 6c8b934 Compare September 11, 2017 17:47

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2017

caesarxuchao force-pushed the plumb-proxy branch from 6c8b934 to e8aa5c1 Compare September 12, 2017 22:17

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2017

caesarxuchao force-pushed the plumb-proxy branch from e8aa5c1 to 8b8917d Compare September 12, 2017 22:31

caesarxuchao changed the title ~~[WIP] Plumbing the proxy dialer to the webhook admission plugin~~ Plumbing the proxy dialer to the webhook admission plugin Sep 12, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2017

caesarxuchao mentioned this pull request Sep 12, 2017

Adding e2e test for the admission webhook #52368

Closed

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 Sep 12, 2017

cheftako reviewed Sep 12, 2017

View reviewed changes

caesarxuchao force-pushed the plumb-proxy branch from 575372d to be33e24 Compare September 13, 2017 00:12

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2017

lavalamp reviewed Sep 13, 2017

View reviewed changes

caesarxuchao mentioned this pull request Sep 14, 2017

Remove the conversion of client config #52372

Merged

lavalamp added this to the v1.8 milestone Sep 14, 2017

lavalamp added the status/approved-for-milestone label Sep 14, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2017

Chao Xu added 2 commits September 14, 2017 15:35

plumb the proxyTransport to the webhook admission plugin;

186a068

set the ServerName in the config for webhook admission plugin.

fix the webhook unit test; the server cert needs to have a valid CN;

856a1db

fix a fuzzer;

caesarxuchao force-pushed the plumb-proxy branch from fd4d25b to 856a1db Compare September 14, 2017 22:36

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 14, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2017

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2017

k8s-github-robot merged commit 7181dd4 into kubernetes:master Sep 15, 2017

mbohlool mentioned this pull request Sep 22, 2017

API Machinery release notes kubernetes/enhancements#446

Merged

caesarxuchao changed the title ~~Plumbing the proxy dialer to the webhook admission plugin~~ Plumbing the proxy dialer to the webhook admission plugin; Fix the tls config Sep 26, 2017

caesarxuchao mentioned this pull request Sep 26, 2017

Has anyone tried this admission controller with success? caesarxuchao/example-webhook-admission-controller#2

Open

caesarxuchao pushed a commit to caesarxuchao/example-webhook-admission-controller that referenced this pull request Sep 27, 2017

Use the service DNS name as CN in the server CSR, to cope with

268dd93

kubernetes/kubernetes#50476

mandarjog mentioned this pull request Nov 20, 2017

add validator server for mixer istio/istio#1728

Merged

tamalsaha mentioned this pull request Jan 1, 2018

Document how to use kube-dns to connect api server to guard server kubeguard/guard#47

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plumbing the proxy dialer to the webhook admission plugin; Fix the tls config #50476

Plumbing the proxy dialer to the webhook admission plugin; Fix the tls config #50476

caesarxuchao commented Aug 10, 2017 •

edited

Loading

caesarxuchao commented Aug 10, 2017

caesarxuchao commented Aug 10, 2017

cheftako commented Aug 16, 2017 •

edited

Loading

caesarxuchao commented Sep 12, 2017

caesarxuchao commented Sep 12, 2017

caesarxuchao commented Sep 12, 2017

k8s-ci-robot commented Sep 12, 2017

cheftako left a comment

cheftako Sep 12, 2017

caesarxuchao Sep 12, 2017

lavalamp Sep 13, 2017

caesarxuchao Sep 13, 2017

lavalamp commented Sep 14, 2017

k8s-github-robot commented Sep 14, 2017

caesarxuchao commented Sep 14, 2017

caesarxuchao commented Sep 14, 2017

caesarxuchao commented Sep 15, 2017

caesarxuchao commented Sep 15, 2017

caesarxuchao commented Sep 15, 2017

k8s-ci-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

prabushyam commented Sep 22, 2017

Plumbing the proxy dialer to the webhook admission plugin; Fix the tls config #50476

Plumbing the proxy dialer to the webhook admission plugin; Fix the tls config #50476

Conversation

caesarxuchao commented Aug 10, 2017 • edited Loading

caesarxuchao commented Aug 10, 2017

caesarxuchao commented Aug 10, 2017

cheftako commented Aug 16, 2017 • edited Loading

caesarxuchao commented Sep 12, 2017

caesarxuchao commented Sep 12, 2017

caesarxuchao commented Sep 12, 2017

k8s-ci-robot commented Sep 12, 2017

cheftako left a comment

Choose a reason for hiding this comment

cheftako Sep 12, 2017

Choose a reason for hiding this comment

caesarxuchao Sep 12, 2017

Choose a reason for hiding this comment

lavalamp Sep 13, 2017

Choose a reason for hiding this comment

caesarxuchao Sep 13, 2017

Choose a reason for hiding this comment

lavalamp commented Sep 14, 2017

k8s-github-robot commented Sep 14, 2017

caesarxuchao commented Sep 14, 2017

caesarxuchao commented Sep 14, 2017

caesarxuchao commented Sep 15, 2017

caesarxuchao commented Sep 15, 2017

caesarxuchao commented Sep 15, 2017

k8s-ci-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

prabushyam commented Sep 22, 2017

caesarxuchao commented Aug 10, 2017 •

edited

Loading

cheftako commented Aug 16, 2017 •

edited

Loading