OCPBUGS-100065: UPSTREAM: <carry>: kube-aggregator: fast http2 health checking for backend connections - #2732
Conversation
…ckend connections The aggregator proxies requests to aggregated apiservers over pooled http2 connections. When such a connection is silently broken - for instance when it was established while the pod network on a freshly rebooted control plane node was still converging - the default http2 health check parameters (ReadIdleTimeout=30s, PingTimeout=15s) keep the dead connection pinned for up to ~45 seconds while every request multiplexed onto it fails with 503 'error trying to reach service: http2: client connection lost'. Observed as 10-15s of oauth-api/openshift-api new-connection disruption during metal-ipi upgrade jobs, with a residual episode remaining even after the aggregated apiserver readyz reachability check was strengthened, because connections can break after readiness. Configure the aggregator's backend proxy transport with aggressive http2 connection health checking (ReadIdleTimeout=5s, PingTimeout=5s) so broken connections are detected and dropped within seconds. Aggregated apiservers are same-cluster backends with sub-second round trips, so a connection that cannot answer a ping for a few seconds is broken for practical purposes and re-dialing is cheap. The construction mirrors client-go transport.New (including the config wrappers, so the x509 metrics wrapper still applies) and falls back to transport.New on any unexpected configuration. Assisted-By: Claude Fable 5
openshift-ci-robot
added
backports/unvalidated-commits
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100065, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@mkowalski: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/payload-aggregate-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 10 #2730 |
WalkthroughThe aggregated API proxy now uses a specialized backend round-tripper with aggressive HTTP/2 health checks, fallback construction, and preserved transport wrappers. Tests cover TLS requests, non-TLS fallback, and wrapper application. ChangesAggregated API transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant updateAPIService
participant newAggregatedAPIBackendRoundTripper
participant http2.ConfigureTransports
participant transport.HTTPWrappersForConfig
updateAPIService->>newAggregatedAPIBackendRoundTripper: build aggregated API round-tripper
newAggregatedAPIBackendRoundTripper->>http2.ConfigureTransports: configure HTTP/2 transport
http2.ConfigureTransports-->>newAggregatedAPIBackendRoundTripper: return configured transport
newAggregatedAPIBackendRoundTripper->>transport.HTTPWrappersForConfig: apply transport wrappers
transport.HTTPWrappersForConfig-->>updateAPIService: return proxy round-tripper
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@mkowalski: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5f31a690-8b50-11f1-893d-e74f5bc1047d-0 |
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100065, which is invalid:
Comment DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go`:
- Around line 11-35: Update
TestNewAggregatedAPIBackendRoundTripperServesRequests to create the TLS server
with httptest.NewUnstartedServer, enable HTTP/2 via EnableHTTP2, and start it
before issuing the request. Preserve the existing handler and cleanup, then
assert the response’s ProtoMajor is 2 in addition to the current status check.
In `@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.go`:
- Around line 49-58: Update the transport construction in the patch backend
setup to preserve client-go defaults: use cfg.Proxy for Proxy, explicitly carry
cfg.DisableCompression, and apply utilnet.SetTransportDefaults to the
http.Transport before any custom DialContext override. Retain the existing TLS
settings and cfg.DialHolder behavior while allowing the standard CIDR-aware
proxy, dial, and keepalive defaults to be applied.
- Around line 60-67: Update the HTTP/2 setup around http2.ConfigureTransports to
bypass configuration and return transport.New(cfg) when cfg.TLS.NextProtos
excludes h2 or DISABLE_HTTP2 is set. Preserve the existing HTTP/2 timeout
configuration for transports where HTTP/2 is explicitly allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf71cec6-845d-47d9-9924-914773fe4549
📒 Files selected for processing (3)
staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.gostaging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.gostaging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go
| func TestNewAggregatedAPIBackendRoundTripperServesRequests(t *testing.T) { | ||
| server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| rt, err := newAggregatedAPIBackendRoundTripper(&transport.Config{ | ||
| TLS: transport.TLSConfig{Insecure: true}, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error building round tripper: %v", err) | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, server.URL, nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error building request: %v", err) | ||
| } | ||
| resp, err := rt.RoundTrip(req) | ||
| if err != nil { | ||
| t.Fatalf("unexpected round trip error: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusOK { | ||
| t.Errorf("expected 200, got %d", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "FILE:"
sed -n '1,120p' staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go
echo
echo "GOROOT:"
go env GOROOT || true
GOROOT="$(go env GOROOT 2>/dev/null || true)"
if [ -n "${GOROOT:-}" ] && [ -f "$GOROOT/src/net/http/httptest/server.go" ]; then
echo
echo "HTTTPTEST SOURCE:"
rg -n "EnableHTTP2|NewTLSServer|StartTLS|NextProtos|http2" "$GOROOT/src/net/http/httptest/server.go" "$GOROOT/src/net/http/httptest/httptest.go" || true
echo
sed -n '120,280p' "$GOROOT/src/net/http/httptest/server.go"
else
echo "std lib source not found"
fiRepository: openshift/kubernetes
Length of output: 8297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the specific test plus the stdlib docs/source for httptest TLS defaults.
sed -n '1,120p' staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go
echo
go doc net/http/httptest.NewTLSServer 2>/dev/null || true
echo
go doc net/http/httptest.Server 2>/dev/null | sed -n '1,120p' || trueRepository: openshift/kubernetes
Length of output: 3307
🌐 Web query:
Go httptest NewTLSServer EnableHTTP2 default HTTP/2 StartTLS docs
💡 Result:
In Go's net/http/httptest package, HTTP/2 is not enabled by default for servers [1]. To enable HTTP/2, you must use the Server.EnableHTTP2 field [2][3]. Because httptest.NewTLSServer automatically starts the server, you cannot use it to enable HTTP/2 [4]. Instead, you must use httptest.NewUnstartedServer to create the server, set EnableHTTP2 to true before starting the server, and then call StartTLS [2][3][4]. The following example demonstrates the correct implementation [2][3]: go ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "Hello, %s", r.Proto) })) ts.EnableHTTP2 = true ts.StartTLS() defer ts.Close() The EnableHTTP2 field must be set after calling NewUnstartedServer and before calling Start or StartTLS [5][2][4]. When EnableHTTP2 is true, the httptest server will configure its internal http.Transport to attempt HTTP/2 [4].
Citations:
- 1: net/http/httptest: NewTLSServer doesn't support HTTP/2 requests golang/go#34939
- 2: https://pkg.go.dev/net/http/httptest
- 3: https://pkg.go.dev/net/http/httptest@go1.26.5
- 4: https://github.com/golang/go/blob/master/src/net/http/httptest/server.go
- 5: https://go.dev/src/net/http/httptest/server.go
Make this TLS test run over HTTP/2. httptest.NewTLSServer starts with HTTP/1.1, so this still passes even if HTTP/2 negotiation is broken. Use httptest.NewUnstartedServer, set EnableHTTP2 = true, and assert resp.ProtoMajor == 2.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport_test.go`
around lines 11 - 35, Update
TestNewAggregatedAPIBackendRoundTripperServesRequests to create the TLS server
with httptest.NewUnstartedServer, enable HTTP/2 via EnableHTTP2, and start it
before issuing the request. Preserve the existing handler and cleanup, then
assert the response’s ProtoMajor is 2 in addition to the current status check.
Source: MCP tools
| t := &http.Transport{ | ||
| Proxy: http.ProxyFromEnvironment, | ||
| TLSClientConfig: tlsConfig, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| MaxIdleConnsPerHost: 25, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| } | ||
| if cfg.DialHolder != nil { | ||
| t.DialContext = cfg.DialHolder.Dial | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
sed -n '1,220p' staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.go
echo
echo "== transport defaults references =="
rg -n "SetTransportDefaults|SetOldTransportDefaults|ProxyFromEnvironment|DisableCompression|DialHolder|DialContext|TLSHandshakeTimeout|IdleConnTimeout" \
staging/src/k8s.io/client-go staging/src/k8s.io/apimachinery staging/src/k8s.io/kube-aggregator -g '!**/vendor/**'
echo
echo "== client-go transport cache =="
sed -n '1,240p' staging/src/k8s.io/client-go/transport/cache.goRepository: openshift/kubernetes
Length of output: 30926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.go')
print(p.exists(), p)
PYRepository: openshift/kubernetes
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.goRepository: openshift/kubernetes
Length of output: 3148
🌐 Web query:
Kubernetes kube-aggregator patch_backend_transport.go ProxyFromEnvironment DisableCompression DialHolder utilnet SetTransportDefaults
💡 Result:
The terms and code structures you identified are related to the implementation and management of HTTP transports within Kubernetes, particularly concerning how the kube-aggregator and API machinery handle proxies, connection settings, and resource leaks [1]. Key components and their roles include: 1. patch_backend_transport.go: This is often associated with custom modifications to HTTP transports, particularly for ensuring that proxy settings (ProxyFromEnvironment) and compression settings (DisableCompression) are correctly handled when Kubernetes components (like the kube-aggregator) act as proxies to backend services [2][3][4]. 2. ProxyFromEnvironment and DisableCompression: These are standard fields in the Go http.Transport struct. Kubernetes components use these to respect system proxy environment variables (e.g., HTTP_PROXY, HTTPS_PROXY) and to manage gzip compression behavior, often disabling it when the application requires raw stream access [2][3][5]. 3. DialHolder and SetTransportDefaults: These elements appear in the context of Kubernetes' internal library (k8s.io/apimachinery/pkg/util/net) [1]. SetTransportDefaults is a utility function used to apply standard Kubernetes configuration to a transport (such as TLS timeouts, connection limits, and HTTP/2 settings) [1]. A DialHolder is a structural pattern (sometimes involving a pointer to a dialer) used to manage and cache transports [1]. 4. Context: The reference to these components in Kubernetes often relates to efforts to resolve resource leaks, such as when transports were being created with unique DialHolder pointers on every synchronization loop, leading to an unbounded growth of the global transport cache [1]. Issues of this nature in the kube-aggregator (specifically in its proxy handler) have historically prompted updates to correctly integrate with the client-go TLS cache and ensure proper reuse of transport configurations [1].
Citations:
- 1: Global transport cache grows unbounded in kube-apiserver process with each sync of APIServiceRegistrationController. kubernetes/kubernetes#117250
- 2: https://git.stelm.me/astelm/minio/commit/2d60bf8c5098990644cafa21d6e91a8c118c3361.diff
- 3: http_proxy environment variables support for doh nextdns/nextdns#670
- 4: https://git.stelm.me/astelm/minio/commit/2a7b123895ccbb5ed36235d2f68c980a6e7e6bdc
- 5: https://coveralls.io/github/UiPath/uipathcli?branch=feature%2Fhttp-proxy-support
🌐 Web query:
site:github.com kubernetes kube-aggregator patch_backend_transport.go "newAggregatedAPIBackendRoundTripper" http2.ConfigureTransports
💡 Result:
There is no specific function named newAggregatedAPIBackendRoundTripper in the Kubernetes kube-aggregator codebase, nor a file named patch_backend_transport.go [1][2]. The kube-aggregator (which handles API service registration and proxying) constructs its transport mechanisms using standard patterns from client-go and the Go net/http ecosystem [1][2]. The primary mechanism for managing backend transport configurations in the kube-aggregator is via the BuildTransportConfig and transport.New functions found in the aggregator's handler_proxy.go [1]. These functions build and configure the http.RoundTripper used to proxy requests to extension API servers [1]. Regarding the HTTP/2 configuration mentioned, Go's x/net/http2 package historically provides http2.ConfigureTransports (and formerly http2.ConfigureTransport) to manually enable and configure HTTP/2 support on an existing http.Transport [3][4]. When an http.Transport is customized—such as by adding a custom Dialer or specific TLS configurations—the standard library does not always auto-enable HTTP/2, necessitating the use of http2.ConfigureTransports to ensure the transport is correctly initialized for HTTP/2 [5][6]. In recent versions of Go and Kubernetes, the way HTTP/2 is enabled on transports has evolved, leading to issues where improper configuration or wrapper changes could downgrade connections to HTTP/1.1 or cause compatibility problems [6]. Kube-aggregator relies on these underlying transport layers to communicate with custom API servers, and configurations related to TLS client certificates, dialers, and proxying are managed through the aggregator's proxyHandler [1][2][7].
Citations:
- 1: https://github.com/kubernetes/kube-aggregator/blob/master/pkg/apiserver/handler_proxy.go
- 2: https://github.com/kubernetes/kube-aggregator/blob/master/pkg/apiserver/apiserver.go
- 3: https://github.com/golang/net/blob/master/http2/transport.go
- 4: https://github.com/golang/go/blob/2403e594/src/net/http/http2.go
- 5: https://github.com/golang/net/blob/master/http2/transport_wrap.go
- 6: x/net/http2: ConfigureTransport no longer enables HTTP/2 under go1.27 (transport_wrap.go) golang/go#79778
- 7: https://github.com/kubernetes/kubernetes/blob/v1.21.0/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go
Preserve client-go transport defaults here. Hardcoding ProxyFromEnvironment ignores cfg.Proxy, leaving DisableCompression unset forces the default false, and skipping utilnet.SetTransportDefaults drops the CIDR-aware proxy handling plus the default dial/keepalive behavior that transport.New applies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.go`
around lines 49 - 58, Update the transport construction in the patch backend
setup to preserve client-go defaults: use cfg.Proxy for Proxy, explicitly carry
cfg.DisableCompression, and apply utilnet.SetTransportDefaults to the
http.Transport before any custom DialContext override. Retain the existing TLS
settings and cfg.DialHolder behavior while allowing the standard CIDR-aware
proxy, dial, and keepalive defaults to be applied.
Source: MCP tools
| t2, err := http2.ConfigureTransports(t) | ||
| if err != nil { | ||
| // should not happen; fall back to the default construction rather than failing the APIService | ||
| klog.Warningf("failed to configure http2 health checking for aggregated API backend transport, falling back to defaults: %v", err) | ||
| return transport.New(cfg) | ||
| } | ||
| t2.ReadIdleTimeout = aggregatedAPIBackendReadIdleTimeout | ||
| t2.PingTimeout = aggregatedAPIBackendPingTimeout |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the patched file and nearby transport setup.
sed -n '1,220p' staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.go
printf '\n--- search for DISABLE_HTTP2 / NextProtos / ConfigureTransports ---\n'
rg -n "DISABLE_HTTP2|ConfigureTransports|NextProtos|http2" staging/src/k8s.io/client-go staging/src/k8s.io/kube-aggregator staging/src/k8s.io/apimachinery -g '*.go'
printf '\n--- locate transport.New implementation ---\n'
rg -n "func New\\(" staging/src/k8s.io/client-go/transport -g '*.go'Repository: openshift/kubernetes
Length of output: 13952
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact transport code in this checkout.
fd 'transport.go' staging/src/k8s.io/client-go/transport staging/src/golang.org/x/net/http2 staging/src/golang.org/x/net -t f
rg -n "DISABLE_HTTP2|ConfigureTransports|NextProtos|TLSNextProto" staging/src/golang.org/x/net staging/src/k8s.io/client-go/transport -g '*.go'Repository: openshift/kubernetes
Length of output: 1416
🌐 Web query:
Go x/net/http2 ConfigureTransports respects TLS NextProtos http/1.1 DISABLE_HTTP2 client-go transport.New
💡 Result:
The behavior of x/net/http2.ConfigureTransports regarding TLS NextProtos and HTTP/2 negotiation depends on the Go version and implementation details [1][2]. 1. HTTP/2 Negotiation and NextProtos: When using x/net/http2.ConfigureTransports, the package ensures that "h2" is present in the TLSClientConfig.NextProtos slice of the provided *http.Transport [3]. Specifically, it appends "h2" to the list of protocols to facilitate ALPN negotiation during the TLS handshake [3]. If a server supports HTTP/2, this allows the client to negotiate the protocol successfully [2]. 2. Disabling HTTP/2: If you wish to disable HTTP/2, you can manipulate the TLSNextProto map on the *http.Transport [4][5]. As documented in the Go standard library, setting Transport.TLSNextProto to an empty map (or a map that does not contain "h2") is the standard way to disable HTTP/2 support on a transport, forcing it to fall back to HTTP/1.1 [4][5]. 3. Interaction with kubernetes/client-go: The kubernetes/client-go library uses a custom transport configuration via transport.New(cfg) [6][7]. The transport.Config struct provides a NextProtos field that allows users to explicitly specify supported protocols [8]. By setting NextProtos to ["http/1.1"], you can force the client-go transport to prefer or exclusively use HTTP/1.1, effectively disabling HTTP/2 negotiation for those connections [8]. 4. Important Version Note: Recent changes in how x/net/http2 integrates with net/http (particularly starting with Go 1.27) have altered the internal implementation [1]. Previously, ConfigureTransport performed multiple setup tasks, including ensuring a non-nil TLSClientConfig and registering the protocol [1]. In newer versions, behavior is delegated to internal wrappers that focus on protocol registration [9][1]. Users relying on custom configurations should ensure their environment correctly supports the expected protocol negotiation, especially when mixing custom TLS settings with HTTP/2 [1][2].
Citations:
- 1: x/net/http2: ConfigureTransport no longer enables HTTP/2 under go1.27 (transport_wrap.go) golang/go#79778
- 2: https://github.com/golang/net/blob/master/http2/transport_configure_test.go
- 3: https://github.com/golang/net/blob/master/http2/transport.go
- 4: net/http: HTTP version selection API golang/go#67814
- 5: https://github.com/golang/go/blob/adf9c27874d8b59b9632a23d9148db4faef36d98/src/net/http/transport.go
- 6: https://github.com/kubernetes/client-go/blob/v0.35.3/rest/transport.go
- 7: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/transport.go
- 8: https://github.com/kubernetes/client-go/blob/master/transport/config.go
- 9: https://github.com/golang/net/blob/master/http2/transport_wrap.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant client-go transport code and HTTP/2 gating helper.
sed -n '1,220p' staging/src/k8s.io/client-go/transport/transport.go
printf '\n--- util/net/http.go ---\n'
sed -n '120,230p' staging/src/k8s.io/apimachinery/pkg/util/net/http.go
printf '\n--- rest/transport.go (wrapper usage) ---\n'
sed -n '1,180p' staging/src/k8s.io/client-go/rest/transport.goRepository: openshift/kubernetes
Length of output: 15928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact helper used to gate HTTP/2 and the transport constructor.
sed -n '120,230p' staging/src/k8s.io/apimachinery/pkg/util/net/http.go
printf '\n---\n'
sed -n '1,140p' staging/src/k8s.io/client-go/transport/transport.goRepository: openshift/kubernetes
Length of output: 8151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how explicit NextProtos are treated in this codebase's tests and proxy helper.
sed -n '80,120p' staging/src/k8s.io/apimachinery/pkg/util/proxy/dial.go
printf '\n---\n'
sed -n '300,335p' staging/src/k8s.io/apimachinery/pkg/util/net/http_test.goRepository: openshift/kubernetes
Length of output: 2588
Respect explicit HTTP/1.1-only transport settings.
This path calls http2.ConfigureTransports directly, so it can re-enable HTTP/2 even when cfg.TLS.NextProtos excludes h2 or DISABLE_HTTP2 is set. Skip HTTP/2 setup in those cases and fall back to transport.New(cfg).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@staging/src/k8s.io/kube-aggregator/pkg/apiserver/patch_backend_transport.go`
around lines 60 - 67, Update the HTTP/2 setup around http2.ConfigureTransports
to bypass configuration and return transport.New(cfg) when cfg.TLS.NextProtos
excludes h2 or DISABLE_HTTP2 is set. Preserve the existing HTTP/2 timeout
configuration for transports where HTTP/2 is explicitly allowed.
Source: MCP tools
|
@mkowalski: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/payload-aggregate-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-upgrade-ovn-ipv6 10 #2730 |
|
Combined payload aggregate results for #2730 + #2732 (via test vehicle #2734; 10 runs, all rebooted 3/3 masters): oauth-api-new-connections totals: 0, 0, 0, 0, 0, 1, 1, 1, 1, 3 seconds (openshift-api similar: max 3s)
The residual reboot-correlated blips are now 1-2s (two runs, at +64s and +77-80s after a master reboot) — exactly the bound expected from the 5s/5s h2 dead-connection detection in this PR. The 12s-class episodes are gone: the readyz gate (#2730) prevents routing to unconverged nodes, and this PR bounds any post-readiness connection breakage. Both PRs validated individually and together; ready for review. Test vehicle #2734 will be closed. This comment was generated using AI. Please verify before acting on it. |
2 participants
Summary
Second half of the fix for OCPBUGS-100065 (companion to #2730).
ReadIdleTimeout=30s/PingTimeout=15sfromk8s.io/apimachinery/pkg/util/net) keep the dead connection pinned for up to ~45s while every request multiplexed onto it fails with503 error trying to reach service: http2: client connection lost.ReadIdleTimeout=5s/PingTimeout=5s, bounding the impact of any dead backend connection to ~10s of a single endpoint being slow-failed, after which the transport re-dials. Aggregated apiservers are same-cluster backends with sub-second RTTs, so these values are safe.client-go transport.New(TLS config, dial holder, wrappers — the x509 metrics wrapper still applies) and falls back totransport.Newon any unexpected configuration. Touches only the proxy path (handler_proxy.go); the availability controller keeps its own transport.Note for upstreaming: unlike #2730 (OpenShift-specific carry), this change is a candidate for upstream
kube-aggregator— either as configurable transport health-check parameters or better defaults for same-cluster backends. Will pursue after downstream validation.Test plan
gofmt,go vet,go build ./staging/src/k8s.io/kube-aggregator/...go test ./staging/src/k8s.io/kube-aggregator/pkg/apiserver/(new + existing tests pass)/payload-aggregatemetal-ipi upgrade jobs together with OCPBUGS-100065: UPSTREAM: <carry>: require all aggregated apiserver endpoints reachable in readyz check #2730: expect oauth-api-new-connections ≤ ~3s across all runs, no ≥10s episodesThis PR was generated using AI. Please verify before acting on it.
Summary by CodeRabbit
Improvements
Tests