Use http/2 for localhost webhook #122558

linxiulei · 2024-01-02T16:37:25Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Previously we changed to http1 because of #82090. However, the fixed issue shouldn't affect cases where the webhook services are hosted in the same host but the fix did affect performance with using http1.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Changed the API server so that for admission webhooks that have a URL matching the hostname `localhost`, or a loopback IP address, the connection supports HTTP/2 where it can be negotiated.

linxiulei · 2024-01-02T16:37:54Z

/assign @liggitt
/cc @aojea

liggitt · 2024-01-02T16:41:37Z

staging/src/k8s.io/apiserver/pkg/util/webhook/client.go

@@ -142,7 +143,9 @@ func (cm *ClientManager) HookClient(cc ClientConfig) (*rest.RESTClient, error) {
 		// Use http/1.1 instead of http/2.
 		// This is a workaround for http/2-enabled clients not load-balancing concurrent requests to multiple backends.
 		// See https://issue.k8s.io/75791 for details.
-		cfg.NextProtos = []string{"http/1.1"}
+		if isLocalHost(cc.URL) {


Both service and URL-based versions go through this code path. We also are already parsing the URL below.

This would be clearer as a forceHTTP1 := true which we only set to false after parsing u, err := url.Parse(cc.URL) below and checking the hostname there for localhost / loopback

Done, thanks!

hmm, @aojea you suggested what I initially did but @liggitt suggested otherwise. I have no strong opinion on either.

I think @aojea was suggesting moving the NextProtos assignment down below to where we had already parsed the URL (do it unconditionally in the service branch, and conditionally in the URL branch)

I don't feel this is simpler since we still need to change it in two places.

I may be missing something, why do we have such large diff? do we need to breaking it down in func (cm *ClientManager) hookClientConfig(cc ClientConfig) (*rest.Config, error) {

I may be missing something, why do we have such large diff?

it was due to bringing several dependencies. I removed them since your suggestion doesn't need them.

do we need to breaking it down in func (cm *ClientManager) hookClientConfig(cc ClientConfig) (*rest.Config, error) {

yes, this refactoring would facilitate writing the unit test linxiulei@612c36d#diff-0afdfbb98099d9c00bfb801579318a05e3f226eb384d50561cef6886e7631046R1

staging/src/k8s.io/apiserver/pkg/util/webhook/client.go

liggitt · 2024-01-02T16:42:08Z

this definitely needs a test and a release note

linxiulei · 2024-01-02T23:00:19Z

One option for testing is to add the http1 behavior test as an e2e test in test/e2e/apimachinery/webhook.go and modify the existing integration test to validate the http2 behavior.

I took a hacky approach to mock the dns for resolving example.com. Will clean up by splitting them into smaller commits if needed.

test/integration/apiserver/admissionwebhook/load_balance_test.go

aojea · 2024-01-03T00:04:47Z

staging/src/k8s.io/apiserver/pkg/util/webhook/client.go

+		if forceHTTP1 {
+			cfg.NextProtos = []string{"http/1.1"}
+		}


oh, I see, for me is easier to read the code top down, and not have to remember that something is going to mutate it later ... it is also less fragile

I don't have strong preference on either way. But changed as you suggested.

test/integration/apiserver/admissionwebhook/load_balance_test.go

test/integration/apiserver/admissionwebhook/admission_test.go

aojea · 2024-01-03T14:22:03Z

some comment lefts but overall LGTM, I think we can assume http2 is prefered for localhost webhooks is relatively safe , as Services will not have a ClusterIP that has a localhost address, and webhooks that are listening on localhost are not supposed to require loadbalancing.

staging/src/k8s.io/apiserver/pkg/util/webhook/client_test.go

Signed-off-by: Eric Lin <[email protected]>

aojea · 2024-01-04T12:25:15Z

/lgtm
/approve

Thanks

k8s-ci-robot · 2024-01-04T12:25:23Z

LGTM label has been added.

Git tree hash: 5f986a1a4a11a8620b08a2fa8f4f3e7f632c401d

liggitt · 2024-01-04T15:19:15Z

test/integration/apiserver/admissionwebhook/load_balance_test.go

@@ -58,154 +65,191 @@ func TestWebhookLoadBalance(t *testing.T) {
 		t.Fatalf("Failed to build cert with error: %+v", err)


if I recall correctly, we had issues with this test flaking when it was first introduced. Did you run it a bunch of times in a row to check that this reworked version is stable?

You mean tls.X509KeyPair() here or the whole test, which I'd guess the connection count is not always predictable.

Anyway, I ran more than a dozen times in a row and no failures.

sorry, the whole test, just had to pick a random line to comment on

FTR, @aojea suggested stress following https://gist.github.com/liggitt/6a3a2217fa5f846b52519acfc0ffece0#running-integration-tests-to-reproduce-flakes

stress ./admissionwebhook.test -test.run TestWebhookLoadBalance 5s: 0 runs so far, 0 failures 10s: 0 runs so far, 0 failures 15s: 0 runs so far, 0 failures 20s: 0 runs so far, 0 failures 25s: 0 runs so far, 0 failures 30s: 0 runs so far, 0 failures 35s: 3 runs so far, 0 failures 40s: 11 runs so far, 0 failures 45s: 16 runs so far, 0 failures 50s: 16 runs so far, 0 failures 55s: 16 runs so far, 0 failures 1m0s: 16 runs so far, 0 failures 1m5s: 20 runs so far, 0 failures 1m10s: 24 runs so far, 0 failures 1m15s: 31 runs so far, 0 failures 1m20s: 32 runs so far, 0 failures 1m25s: 32 runs so far, 0 failures 1m30s: 32 runs so far, 0 failures 1m35s: 36 runs so far, 0 failures 1m40s: 41 runs so far, 0 failures 1m45s: 46 runs so far, 0 failures 1m50s: 48 runs so far, 0 failures 1m55s: 48 runs so far, 0 failures 2m0s: 48 runs so far, 0 failures 2m5s: 48 runs so far, 0 failures 2m10s: 48 runs so far, 0 failures 2m15s: 50 runs so far, 0 failures 2m20s: 53 runs so far, 0 failures 2m25s: 57 runs so far, 0 failures 2m30s: 60 runs so far, 0 failures 2m35s: 63 runs so far, 0 failures 2m40s: 64 runs so far, 0 failures 2m45s: 64 runs so far, 0 failures 2m50s: 67 runs so far, 0 failures 2m55s: 71 runs so far, 0 failures 3m0s: 75 runs so far, 0 failures 3m5s: 78 runs so far, 0 failures 3m10s: 80 runs so far, 0 failures 3m15s: 80 runs so far, 0 failures 3m20s: 82 runs so far, 0 failures 3m25s: 84 runs so far, 0 failures 3m30s: 89 runs so far, 0 failures 3m35s: 92 runs so far, 0 failures 3m40s: 95 runs so far, 0 failures 3m45s: 96 runs so far, 0 failures 3m50s: 98 runs so far, 0 failures 3m55s: 99 runs so far, 0 failures 4m0s: 104 runs so far, 0 failures 4m5s: 108 runs so far, 0 failures 4m10s: 110 runs so far, 0 failures 4m15s: 112 runs so far, 0 failures

liggitt · 2024-01-04T15:20:50Z

change lgtm, one question about making sure the updated integration test is non-flaky

liggitt · 2024-01-04T16:42:26Z

/approve

k8s-ci-robot · 2024-01-04T16:43:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, liggitt, linxiulei

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~test/integration/apiserver/OWNERS~~ [aojea,liggitt]

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

sftim · 2024-01-09T17:27:50Z

Changelog suggestion

-The apiserver uses http/2 to communicate with admission webhooks specified with an `URL` of localhost or loopback IP addresses.
+Changed the API server so that for admission webhooks that have a URL matching the hostname
+`localhost`, or a loopback IP address, the connection supports HTTP/2 where it can be negotiated.

I presume that other webhooks, eg authz webhooks, use the legacy behavior. Have I got that right?

linxiulei · 2024-01-12T17:34:28Z

Changelog changed as suggested.

I presume that other webhooks, eg authz webhooks, use the legacy behavior. Have I got that right?

I don't think this PR touched clients for other webhooks (i.e. only for clients of validating and mutating webhooks). But others might be more knowledgable than me.

k8s-ci-robot assigned liggitt Jan 2, 2024

k8s-ci-robot requested review from aojea, hzxuzhonghu and smarterclayton January 2, 2024 16:37

liggitt reviewed Jan 2, 2024

View reviewed changes

staging/src/k8s.io/apiserver/pkg/util/webhook/client.go Outdated Show resolved Hide resolved

linxiulei force-pushed the webhook-http2 branch from 6a282a0 to ad492e1 Compare January 2, 2024 17:44

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 2, 2024

linxiulei force-pushed the webhook-http2 branch from ad492e1 to 08ec05b Compare January 2, 2024 18:29

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 2, 2024

linxiulei force-pushed the webhook-http2 branch from 08ec05b to 9e6ec01 Compare January 2, 2024 18: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 Jan 2, 2024

linxiulei force-pushed the webhook-http2 branch from 9e6ec01 to 768a3f3 Compare January 2, 2024 20:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 2, 2024

aojea reviewed Jan 2, 2024

View reviewed changes

test/integration/apiserver/admissionwebhook/load_balance_test.go Outdated Show resolved Hide resolved

aojea reviewed Jan 3, 2024

View reviewed changes

linxiulei force-pushed the webhook-http2 branch from 6b39041 to 08a7b2b Compare January 3, 2024 01:17

aojea reviewed Jan 3, 2024

View reviewed changes

test/integration/apiserver/admissionwebhook/load_balance_test.go Outdated Show resolved Hide resolved

linxiulei force-pushed the webhook-http2 branch from 08a7b2b to 612c36d Compare January 3, 2024 13:50

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

linxiulei force-pushed the webhook-http2 branch from 612c36d to f798519 Compare January 3, 2024 13:59

aojea reviewed Jan 3, 2024

View reviewed changes

test/integration/apiserver/admissionwebhook/admission_test.go Outdated Show resolved Hide resolved

linxiulei force-pushed the webhook-http2 branch from f798519 to 4bcca1a Compare January 3, 2024 15:42

aojea reviewed Jan 3, 2024

View reviewed changes

staging/src/k8s.io/apiserver/pkg/util/webhook/client_test.go Outdated Show resolved Hide resolved

Use http/2 for localhost webhook

246e69f

Signed-off-by: Eric Lin <[email protected]>

linxiulei force-pushed the webhook-http2 branch from 4bcca1a to 246e69f Compare January 4, 2024 09:11

k8s-ci-robot assigned aojea Jan 4, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2024

liggitt reviewed Jan 4, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 4, 2024

k8s-ci-robot merged commit 9bd0ef5 into kubernetes:master Jan 4, 2024
15 checks passed

k8s-ci-robot added this to the v1.30 milestone Jan 4, 2024

kyrtapz mentioned this pull request May 23, 2024

OCPBUGS-34359: Ensure that the node-identity webhook address contains colons for IPv6 openshift/cluster-network-operator#2382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use http/2 for localhost webhook #122558

Use http/2 for localhost webhook #122558

linxiulei commented Jan 2, 2024 •

edited

Loading

linxiulei commented Jan 2, 2024

liggitt Jan 2, 2024

linxiulei Jan 2, 2024

linxiulei Jan 2, 2024

liggitt Jan 2, 2024

linxiulei Jan 2, 2024

aojea Jan 3, 2024

linxiulei Jan 3, 2024

liggitt commented Jan 2, 2024

linxiulei commented Jan 2, 2024

aojea Jan 3, 2024

linxiulei Jan 3, 2024

aojea commented Jan 3, 2024

aojea commented Jan 4, 2024

k8s-ci-robot commented Jan 4, 2024

liggitt Jan 4, 2024

linxiulei Jan 4, 2024 •

edited

Loading

liggitt Jan 4, 2024

linxiulei Jan 5, 2024

liggitt commented Jan 4, 2024

liggitt commented Jan 4, 2024

k8s-ci-robot commented Jan 4, 2024

sftim commented Jan 9, 2024 •

edited

Loading

linxiulei commented Jan 12, 2024

		@@ -58,154 +65,191 @@ func TestWebhookLoadBalance(t *testing.T) {
		t.Fatalf("Failed to build cert with error: %+v", err)

Use http/2 for localhost webhook #122558

Use http/2 for localhost webhook #122558

Conversation

linxiulei commented Jan 2, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

linxiulei commented Jan 2, 2024

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 Jan 2, 2024

linxiulei commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jan 3, 2024

aojea commented Jan 4, 2024

k8s-ci-robot commented Jan 4, 2024

Choose a reason for hiding this comment

linxiulei Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 4, 2024

liggitt commented Jan 4, 2024

k8s-ci-robot commented Jan 4, 2024

sftim commented Jan 9, 2024 • edited Loading

linxiulei commented Jan 12, 2024

linxiulei commented Jan 2, 2024 •

edited

Loading

linxiulei Jan 4, 2024 •

edited

Loading

sftim commented Jan 9, 2024 •

edited

Loading