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

Match webhook client auth with ports consistently #82252

Merged
merged 3 commits into from Sep 3, 2019

Conversation

@liggitt
Copy link
Member

commented Sep 3, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:

  • Adds an integration test ensuring client auth works properly for URL-configured admission webhooks, with and without aggregator routing enabled
  • Adds unit tests exercising matching with and without ports
  • Plumbs configured service ports to client auth matching so service- and url-configured webhooks work consistently
  • Defaults port-less URL webhooks to :443 (default https port) for purposes of matching client auth
  • When searching for credentials for service or urls with port 443, fall back to searching port-less stanzas

Which issue(s) this PR fixes:
Fixes #71087
Fixes #54709
Docs in https://github.com/kubernetes/website/pull/16204/files#diff-7d8ea01378bc95ad63ddb1317a8e7940

Does this PR introduce a user-facing change?:

Webhook client credentials configured with `--admission-control-config-file` must include non-default ports in the configured hostnames. For example, a webhook configured to speak to port 8443 on service `mysvc` in namespace `myns` would specify client credentials in a stanza with `name: mysvc.myns.svc:8443`. See https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#authenticate-apiservers for more details.

/sig api-machinery
/cc @sttts @jpbetz

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

/priority important-soon
/area admission-control

@liggitt liggitt added this to Bugs/Cleanup/Tests in Admission Webhooks Sep 3, 2019
@liggitt liggitt force-pushed the liggitt:webhook-client-auth-test branch from e686d24 to c9124ee Sep 3, 2019
@sttts

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Sgtm.

Leaving this open for some more eyes.

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

/cc @deads2k

@k8s-ci-robot k8s-ci-robot requested a review from deads2k Sep 3, 2019
@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

/milestone v1.16

to fix resolution to be consistent for webhook GA

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Sep 3, 2019
@deads2k

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Spoke via slack. I'd prefer to fix the bug about sending the wrong credentials through. If you have a non-443 port, you should be required to specify the port.

@liggitt liggitt force-pushed the liggitt:webhook-client-auth-test branch from c9124ee to e734c70 Sep 3, 2019
@liggitt liggitt changed the title Match webhook client auth with and without ports Match webhook client auth with ports consistently Sep 3, 2019
@deads2k

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

/lgtm

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit b47349a into kubernetes:master Sep 3, 2019
24 checks passed
24 checks passed
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@liggitt liggitt moved this from Bugs/Cleanup/Tests to Complete in Admission Webhooks Sep 3, 2019
@liggitt liggitt deleted the liggitt:webhook-client-auth-test branch Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.