Fix webhook API to also support URLs #54889

lavalamp · 2017-10-31T23:30:38Z

The dynamic admission webhook now supports a URL in addition to a service reference, to accommodate out-of-cluster webhooks.

lavalamp · 2017-10-31T23:33:39Z

Also @liggitt

deads2k · 2017-11-01T12:13:09Z

staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

+	// webhook, for example, a cluster identifier.
+	//
+	// Required.
+	URL string


I like this change.

Unfortunately I'm hearing from @bowei that there is no safe standard DNS form that we could parse? Bowei, please say I misunderstood :( :(

I'm unsure what is meant by that. I'd expect to fully specify the URL, including the scheme: scheme://host[:port][/path]. What is ambiguous about that?

Do we need to distinguish between a service exported by the cluster vs a server with a DNS name reachable on the network?

What Daniel is referring to is that we do not want to bake in the current DNS schema into the system in case the schema changes. This is a particularly subtle place to have to be maintained if any changes are done.

Any alternative is to have a different scheme for identifying cluster only services, something like: k8s-service://.

@liggitt, kube-apiserver must be able to tell whether the DNS name refers to something in the cluster or out of the cluster, because it can't assume the presence of DNS resolution for in-cluster things (or, currently, routes, though that will change in a few quarters).

I've pushed a commit that keeps both. I don't like it very much. Why not maintain the Service field, and make the external hostname field of that work correctly?

@lavalamp @deads2k

I'm in favor of only a service reference, and opposed to a URL. As we can see from this discussion, URLs obscure information that needs to be first-class. In particular, if something generates or munges the Service name, it needs to know how to populate this field. Everywhere else in the API we use object references or names.

So, option 3.

For self-hosted webhooks, I can live with service references, resolved as they are today.

For non-cluster-hosted webhooks, it doesn't make sense to require the indirection of using a service reference pointing to a service which itself points to an external hostname. That still requires the external hostname to be resolvable, and requires all consumers of admission webhook config (even ones in pods that could have used DNS for in- and out- of cluster resolution) to have custom dialers and read permission on the service API objects.

@erictune
Suggestion:
at most one of Service ServiceReference or URL string may be set.
if URL string is used, normal DNS resolution is used, then no magic happens, and <name>.<namespace>.svc. is not expected to work.
This lets the user chose which is the lesser evil: layering violation or non-portable configuration.

That seems reasonable to me (and it's not a layer violation to use DNS to resolve off-cluster webhook hostnames)

It seems like I never submitted my reply to eric's comment above, but Jordan and Eric's approach seems to be reasonable.

What I care most about is that we keep service/object references for self-hosted webhooks.

erictune · 2017-11-01T20:41:22Z

staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

+	// render your config non-portable; apiservers will route to the
+	// service correctly.
+	//
+	// If the scheme is present, it must be "https://".


Why isn't http:// permissible?

If it were, then the CABundle field would have to be optional, but it isn't.

I don't mind changing this--was just keeping the change limited.

deads2k · 2017-11-02T12:01:09Z

I think using name.namespace.svc as the trigger for your dialer treating it as an internal name is your best option. All in-cluster DNS will resolve that and having the kube-apiserver be consistent with in-cluster DNS seems like a very good idea.

You were concerned about .svc becoming a TLD, but even if that happens people still have a fairly simple way to specify that using the DNS standard trailing dot to indicate a complete name (external.svc. is fully qualified, name.namespace.svc is not fully qualified). In addition, that change would be consistent for things using in-cluster DNS and things not using it.

I would not use option 1 because of the extra DNS changes.

I'm not a fan of allowing multiple options in the API type. It seems like an unnecessary layer. Expecting the kube-apiserver to be able to use DNS names directly and resolve in-cluster names seems reasonable and doable.

deads2k · 2017-11-03T12:15:51Z

That seems reasonable to me (and it's not a layer violation to use DNS to resolve off-cluster webhook hostnames)

Not very pretty, but it at least makes it possible to use externally hosted hooks.

lavalamp · 2017-11-03T16:42:32Z

Thank you everyone.

That extension apiservers may not have permission to read arbitrary services seems like a good reason to include the URL. Or at least, I'm not in favor of forcing the system administrator to set up RBAC rules to make sure every apiserver can read every webhook's service. And I agree that relying on external DNS is not a layer violation.

So, I now believe that both are necessary, sadly.

bgrant0607 · 2017-11-03T17:08:23Z

I'm fine with keeping both.

lavalamp · 2017-11-04T00:13:08Z

To be my own devil's advocate, it's worth noting we'll already have to make sure extension apiservers can read all namespaces.

liggitt · 2017-11-04T00:16:16Z

To be my own devil's advocate, it's worth noting we'll already have to make sure extension apiservers can read all namespaces.

That was always the case in order to correctly allow/prevent creation of namespaced resources. I'd really avoid requiring visibility into unrelated namespace content like services.

thockin · 2017-11-04T03:23:27Z

On mobile, apologies for mistakes.

I am very not fond of ad hoc syntax definitions, and that's what using "foo.bar.svc" is. It is not a real host name, but it could become one. It is not an in-cluster FQDN. It is just a syntax that happens to lean a bit on DNS, as defined today.

DNS will change eventually (though it will be a long transition). This is one more place to adapt or else you carry your own syntax.

If you need a URL,why not k8s-svc://service.namespace ?

bowei · 2017-11-04T04:48:55Z

@thockin -- is there a place where we can make something like k8s-service:// (or kubernetes-services://) official? Seems the "proper" way to go...

lavalamp · 2017-11-04T17:28:55Z

Yeah, the custom scheme mechanism doesn't work because there's no programatic way for extension apiservers to turn that into a proper string they can DNS resolve. So both it is :(

bgrant0607 · 2017-11-09T04:37:56Z

@bowei A string with a custom format is not the right way to go. The syntax would be less discoverable, it would be different from all other references, defaulting from context would be harder, ...

deads2k · 2017-11-09T19:47:07Z

pkg/apis/admissionregistration/types.go

+	// webhook, for example, a cluster identifier.
+	//
+	// +optional
+	URL *string `json:"url,omitempty"`


internal types don't get tags

deads2k · 2017-11-09T19:49:38Z

pkg/apis/admissionregistration/validation/validation_test.go

+						ClientConfig: admissionregistration.AdmissionHookClientConfig{},
+					},
+				}),
+			expectedError: `exactly one of`,


get slightly more specific so you include the fields too.

I intended to copy/paste from the failing test to put in the thing to check, but then the test unexpectedly passed and I didn't...

I include each field in at least one of the test cases now, I don't like making change-detector tests so I don't think it needs to be universal.

deads2k · 2017-11-09T19:50:15Z

staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

+	// `path` is an optional URL path which will be sent in any request to
+	// this service.
+	// +optional
+	Path *string `json:"path,omitempty"`


missing proto tag

I thought the proto generator was supposed to do that, I guess make generated_files didn't actually run that (WHY?), which explains some other things.

deads2k · 2017-11-09T19:50:24Z

staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

+	// webhook, for example, a cluster identifier.
+	//
+	// +optional
+	URL *string `json:"url,omitempty"`


deads2k · 2017-11-09T19:53:01Z

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

 	}

 	// TODO: cache these instead of constructing one each time
-	restConfig, err := a.authInfoResolver.ClientConfigFor(serverName)
+	restConfig, err := a.authInfoResolver.ClientConfigFor(u.Host)


Doesn't host include port? I wonder what the clientConfigFor code does in that case.

I am not sure if I should split here or let authInfoResolver worry about it-- what if people run two different things on two ports on the same host? (parameter name is "server" which is pretty ambiguous as to whether it should include a port)

Previously we mandated 443, so at least this doesn't make it worse.

lavalamp · 2017-11-11T20:51:41Z

/retest pull-kubernetes-bazel-test

lavalamp · 2017-11-11T20:54:18Z

/retest

caesarxuchao

A nit. Otherwise lgtm.

caesarxuchao · 2017-11-11T22:21:27Z

pkg/apis/admissionregistration/types.go

+	//
+	// If the webhook is running within the cluster, then you should use `service`.
+	//
+	// If there is only


nit: premature new line

caesarxuchao · 2017-11-12T00:34:38Z

/lgtm

k8s-github-robot · 2017-11-12T00:35:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 492

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

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

~~api/OWNERS~~ [lavalamp]
~~docs/api-reference/OWNERS~~ [lavalamp]
~~pkg/apis/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [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

caesarxuchao · 2017-11-12T00:56:24Z

/retest

liggitt · 2017-11-12T00:54:20Z

staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

+	// webhook, for example, a cluster identifier.
+	//
+	// +optional
+	URL *string `json:"url,omitempty" protobuf:"bytes,3,opt,name=url"`


Why not require scheme? Making it optional means url.Parse can't be used on the value as specified, right? (I think Parse treats a schemeless host as a relative path)

https://play.golang.org/p/2lAiMHlmY7

validation does require the scheme, I forgot to fix the comment.

Looks like it allows "". Should just require https

liggitt · 2017-11-12T00:55:44Z

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

@@ -395,47 +400,71 @@ func toStatusErr(name string, result *metav1.Status) *apierrors.StatusError {

 func (a *GenericAdmissionWebhook) hookClient(h *v1alpha1.Webhook) (*rest.RESTClient, error) {
 	cacheKey, err := json.Marshal(h.ClientConfig)
-	if err != nil {


If an error occurs, we shouldn't use the cacheKey to look up a client

(Or store a client)

this was a rebase problem, oops.

liggitt · 2017-11-12T00:57:18Z

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

+		return nil, &ErrCallingWebhook{WebhookName: h.Name, Reason: ErrNeedServiceOrURL}
+	}
+
+	u, err := url.Parse(*h.ClientConfig.URL)


Does this do the right thing with schemeless urls?

liggitt · 2017-11-12T00:57:45Z

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

+	cfg := rest.CopyConfig(restConfig)
+	cfg.Host = u.Host
+	cfg.APIPath = u.Path
+	// TODO: test if this is needed: cfg.TLSClientConfig.ServerName = u.Host


Not needed if host and serverName are the same

liggitt · 2017-11-12T00:59:17Z

pkg/apis/admissionregistration/validation/validation.go

+			}
+			if len(u.Host) == 0 {
+				allErrors = append(allErrors, field.Required(fldPath.Child("url"), "host must be provided"+form))
+			}


Make sure there's no query or user info?

liggitt · 2017-11-12T01:05:42Z

/hold
Just a couple comments on the handling of an error building the cacheKey, and whether handling of a schemeless host is correct/necessary

lavalamp · 2017-11-12T01:18:15Z

chatted w/ @liggitt, going to fix comments in a followup. Don't want to block the merge train.

liggitt · 2017-11-12T01:18:36Z

Talked offline. Will take a follow up on the scheme and cacheKey issues

lavalamp · 2017-11-12T01:36:15Z

/retest

lavalamp · 2017-11-12T02:48:16Z

Followup is #55534

lavalamp · 2017-11-12T03:36:48Z

/retest

caesarxuchao · 2017-11-12T05:39:44Z

/retest

lavalamp · 2017-11-12T05:54:42Z

I know the e2e isn't due to this PR because #55534 passes...

k8s-github-robot · 2017-11-12T06:13:42Z

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

k8s-github-robot · 2017-11-12T07:01:39Z

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

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Tighten webhook client config validation ref kubernetes/enhancements#492 Fix up some nits left from #54889. ```release-note NONE ```

lavalamp assigned deads2k Oct 31, 2017

lavalamp force-pushed the wh-api branch from f110b46 to 73818b4 Compare October 31, 2017 23:38

deads2k reviewed Nov 1, 2017

View reviewed changes

erictune reviewed Nov 1, 2017

View reviewed changes

frankfarzan mentioned this pull request Nov 6, 2017

Adds support for in-server service name resolution. #54733

Closed

lavalamp force-pushed the wh-api branch from 8f3509e to 1be518b Compare November 9, 2017 19:40

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 Nov 9, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 9, 2017

lavalamp changed the title ~~RFC: Remove service reference and combine with URLPath~~ Fix webhook API to also support URLs Nov 9, 2017

lavalamp assigned caesarxuchao Nov 9, 2017

deads2k reviewed Nov 9, 2017

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 11, 2017

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

caesarxuchao reviewed Nov 11, 2017

View reviewed changes

Add URL beside service

a0cb2ce

lavalamp force-pushed the wh-api branch from 91fc8fa to 21ec2a5 Compare November 12, 2017 00:09

generated

fc0924c

lavalamp force-pushed the wh-api branch from 21ec2a5 to fc0924c Compare November 12, 2017 00:25

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

liggitt requested changes Nov 12, 2017

View reviewed changes

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

lavalamp removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2017

lavalamp mentioned this pull request Nov 12, 2017

Tighten webhook client config validation #55534

Merged

k8s-github-robot merged commit e938190 into kubernetes:master Nov 12, 2017

Fix webhook API to also support URLs #54889

Fix webhook API to also support URLs #54889

Conversation

lavalamp commented Oct 31, 2017 • edited

lavalamp commented Oct 31, 2017

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

lavalamp Nov 1, 2017 • edited

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

deads2k commented Nov 2, 2017

deads2k commented Nov 3, 2017

lavalamp commented Nov 3, 2017

bgrant0607 commented Nov 3, 2017

lavalamp commented Nov 4, 2017

liggitt commented Nov 4, 2017

thockin commented Nov 4, 2017

bowei commented Nov 4, 2017

lavalamp commented Nov 4, 2017

bgrant0607 commented Nov 9, 2017

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

lavalamp commented Nov 11, 2017

lavalamp commented Nov 11, 2017

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Nov 12, 2017

k8s-github-robot commented Nov 12, 2017

caesarxuchao commented Nov 12, 2017

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 commented Nov 12, 2017

lavalamp commented Nov 12, 2017

liggitt commented Nov 12, 2017

lavalamp commented Nov 12, 2017

lavalamp commented Nov 12, 2017

lavalamp commented Nov 12, 2017

caesarxuchao commented Nov 12, 2017

lavalamp commented Nov 12, 2017

k8s-github-robot commented Nov 12, 2017

k8s-github-robot commented Nov 12, 2017

lavalamp commented Oct 31, 2017 •

edited

lavalamp Nov 1, 2017 •

edited