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 http/1.1 for apiserver->webhook clients #82090

Merged
merged 3 commits into from Aug 30, 2019

Conversation

@liggitt
Copy link
Member

commented Aug 28, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:

  • Allows specifying desired NextProtos in client-go transport config
  • Honor client requests to only support http/1.1 when building transports
  • Preserves the default idle connection timeout from the default transport
  • Use http/1.1-only for apiserver -> webhook clients to work around http/2 single-connection balancing issues
  • Adds an integration test verifying multiple connections are opened to handle concurrent admission webhook requests, and that idle connections are reused

Which issue(s) this PR fixes:
Fixes #75791

Does this PR introduce a user-facing change?:

The apiserver now uses http/1.1 to communicate with admission webhooks, opening multiple connections to satisfy concurrent requests, and allowing spreading requests across multiple backing pods.

/sig api-machinery
/priority important-soon
/milestone v1.16
/area admission-control

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Just a request for some better doc from me. This is extremely esoteric.

@liggitt liggitt force-pushed the liggitt:webhook-http2 branch from 44dc73b to a26bffd Aug 28, 2019
@liggitt liggitt force-pushed the liggitt:webhook-http2 branch from a26bffd to 0554d51 Aug 28, 2019
@liggitt

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

docs updated, rebased on master

@lavalamp

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

/hold

I actually am not convinced that this solves the problem. I would like to see a test reproducing the problem in a very close analogy to the way it actually occurs. HTTP1/2 stuff is complex enough that I don't trust analytic approaches.

I don't object to the client library changes, but they should be separately unit tested.

@lavalamp

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

correction: this is not about the thing I thought it was about. I still object, but on radically different grounds. chatting directly with Jordan about it.

@lavalamp

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

/hold cancel

Jordan convinced me this is urgent for making webhooks usable at high scale, and there aren't short-term alternatives.

I don't think turning off http2 is the right long-term solution, but I won't block this over that.

@liggitt liggitt force-pushed the liggitt:webhook-http2 branch from 0554d51 to ddc6978 Aug 28, 2019
@liggitt

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

/retest

1 similar comment
@liggitt

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/retest

@liggitt liggitt moved this from Triage to Required for GA, in progress in Admission Webhooks Aug 29, 2019
@liggitt

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/retest

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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

@k8s-ci-robot k8s-ci-robot merged commit f442b6e into kubernetes:master Aug 30, 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 Required for GA, in progress to Complete in Admission Webhooks Aug 30, 2019
@liggitt liggitt deleted the liggitt:webhook-http2 branch Aug 30, 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.