OCPBUGS-86661: Konnectivity retry proxy connection on timeout - #8579
Open
YamunadeviShanmugam wants to merge 1 commit into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
📝 WalkthroughWalkthroughThe SOCKS5 proxy now uses a guarded Konnectivity bootstrap and serving loop. It retries transient bootstrap and serving failures with exponential backoff and performs graceful shutdown on context cancellation. Startup now reports non-transient errors and exits accordingly. Konnectivity containers now expose TCP startup probes using configured or default serving ports. The OAuth API server now declares the Konnectivity agent dependency. Tests cover these behaviors. Sequence Diagram(s)sequenceDiagram
participant Main
participant CoreGuard
participant Konnectivity
participant SOCKS5
Main->>CoreGuard: start with signal context
CoreGuard->>Konnectivity: bootstrap and validate TCP reachability
Konnectivity-->>CoreGuard: return proxy dialer
CoreGuard->>SOCKS5: serve with dialer
Main->>CoreGuard: cancel on signal
CoreGuard->>SOCKS5: close listener
SOCKS5-->>Main: return graceful shutdown
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/control-plane-operator
YamunadeviShanmugam
force-pushed
the
fix-add-startup-probe-konnectivity-sock5-sidecar
branch
from
May 22, 2026 17:40
e81da62 to
c1f6355
Compare
Contributor
Author
|
/test all |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8579 +/- ##
==========================================
+ Coverage 44.96% 45.20% +0.24%
==========================================
Files 778 779 +1
Lines 97452 97627 +175
==========================================
+ Hits 43820 44137 +317
+ Misses 50607 50461 -146
- Partials 3025 3029 +4
... and 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
konnectivity-socks5-proxy/guard_test.go (2)
28-58: ⚡ Quick winConsider refactoring to a table-driven test.
The test cases all follow the same structure (create error, call
isTransientKonnectivityError, assert result). Table-driven tests improve maintainability and make it easier to add new cases. As per coding guidelines, prefer table-driven tests where possible.♻️ Proposed table-driven refactor
func TestIsTransientKonnectivityError(t *testing.T) { - t.Run("When error is nil, it should not be transient", func(t *testing.T) { - g := NewGomegaWithT(t) - g.Expect(isTransientKonnectivityError(nil)).To(BeFalse()) - }) - - t.Run("When error is context deadline exceeded, it should be transient", func(t *testing.T) { - g := NewGomegaWithT(t) - g.Expect(isTransientKonnectivityError(context.DeadlineExceeded)).To(BeTrue()) - }) - - t.Run("When error is connection refused, it should be transient", func(t *testing.T) { - g := NewGomegaWithT(t) - g.Expect(isTransientKonnectivityError(fmt.Errorf("dial tcp: connection refused"))).To(BeTrue()) - }) - - t.Run("When error is syscall ECONNREFUSED, it should be transient", func(t *testing.T) { - g := NewGomegaWithT(t) - g.Expect(isTransientKonnectivityError(syscall.ECONNREFUSED)).To(BeTrue()) - }) - - t.Run("When error is validation failure, it should not be transient", func(t *testing.T) { - g := NewGomegaWithT(t) - g.Expect(isTransientKonnectivityError(errors.New("failed validation: KonnectivityHost is required"))).To(BeFalse()) - }) - - t.Run("When error is file not found, it should not be transient", func(t *testing.T) { - g := NewGomegaWithT(t) - g.Expect(isTransientKonnectivityError(os.ErrNotExist)).To(BeFalse()) - }) + testCases := []struct { + name string + err error + isTransient bool + }{ + { + name: "When error is nil, it should not be transient", + err: nil, + isTransient: false, + }, + { + name: "When error is context deadline exceeded, it should be transient", + err: context.DeadlineExceeded, + isTransient: true, + }, + { + name: "When error is connection refused, it should be transient", + err: fmt.Errorf("dial tcp: connection refused"), + isTransient: true, + }, + { + name: "When error is syscall ECONNREFUSED, it should be transient", + err: syscall.ECONNREFUSED, + isTransient: true, + }, + { + name: "When error is validation failure, it should not be transient", + err: errors.New("failed validation: KonnectivityHost is required"), + isTransient: false, + }, + { + name: "When error is file not found, it should not be transient", + err: os.ErrNotExist, + isTransient: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewGomegaWithT(t) + if tc.isTransient { + g.Expect(isTransientKonnectivityError(tc.err)).To(BeTrue()) + } else { + g.Expect(isTransientKonnectivityError(tc.err)).To(BeFalse()) + } + }) + } }🤖 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 `@konnectivity-socks5-proxy/guard_test.go` around lines 28 - 58, Refactor TestIsTransientKonnectivityError into a table-driven test: create a slice of cases with fields name, err and want (bool) containing the current scenarios (nil, context.DeadlineExceeded, fmt.Errorf("dial tcp: connection refused"), syscall.ECONNREFUSED, errors.New("failed validation..."), os.ErrNotExist), then loop over cases calling t.Run(case.name, func(t *testing.T){ g := NewGomegaWithT(t); g.Expect(isTransientKonnectivityError(case.err)).To(Equal(case.want)) }); keep the test function name TestIsTransientKonnectivityError and reuse isTransientKonnectivityError and NewGomegaWithT to preserve existing assertions.
95-103: ⚡ Quick winAvoid mutating package-level state in tests.
Modifying the package-level
dialKonnectivityServervariable violates the guideline to avoid global state in tests. While the defer restores the original value, this pattern is unsafe for parallel test execution and can cause race conditions. As per coding guidelines, tests should run with parallel execution enabled.Consider refactoring the code under test to accept the dialer as a parameter (dependency injection) rather than relying on a package-level variable. This would make tests safer, support parallel execution, and improve testability.
Example approach:
// In guard.go, make dialKonnectivityServer a parameter or field func retryDialKonnectivityServer(ctx context.Context, log logr.Logger, host string, port uint32, dialFunc func(string, uint32) error) error { // ... use dialFunc instead of global dialKonnectivityServer } // In test func TestDialKonnectivityServerRetries(t *testing.T) { attempts := 0 mockDial := func(host string, port uint32) error { attempts++ if attempts < 2 { return fmt.Errorf("dial tcp: connection refused") } return dialKonnectivityServerTCP(host, port) } err := retryDialKonnectivityServer(ctx, log, "127.0.0.1", port, mockDial) // ... assertions }As per coding guidelines, avoid global state in tests and run tests with parallel execution.
🤖 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 `@konnectivity-socks5-proxy/guard_test.go` around lines 95 - 103, The test mutates the package-level dialKonnectivityServer which is unsafe for parallel tests; refactor retryDialKonnectivityServer (in guard.go) to accept a dial function parameter (e.g., dialFunc func(string,uint32) error or as a field on a struct) and update the test to call retryDialKonnectivityServer with a local mockDial closure that increments attempts and returns the simulated error on the first call and delegates to the real dial (e.g., dialKonnectivityServerTCP) on success; remove any package-level reassignment in tests and use dependency injection to supply the mock instead.support/controlplane-component/konnectivity-container.go (1)
184-198: ⚡ Quick winRefactor to eliminate duplication and add defensive default case.
The HTTPS and Socks5 branches contain identical logic, violating DRY. Additionally, the switch lacks a default case—if
opts.Modeis neither HTTPS nor Socks5,servingPortremains zero, creating an invalid probe configuration.♻️ Proposed refactoring
- var servingPort int32 - switch opts.Mode { - case HTTPS: - if opts.HTTPSOptions.ServingPort != 0 { - servingPort = int32(opts.HTTPSOptions.ServingPort) - } else { - servingPort = 8090 - } - case Socks5: - if opts.Socks5Options.ServingPort != 0 { - servingPort = int32(opts.Socks5Options.ServingPort) - } else { - servingPort = 8090 - } - } + servingPort := int32(8090) + switch opts.Mode { + case HTTPS: + if opts.HTTPSOptions.ServingPort != 0 { + servingPort = int32(opts.HTTPSOptions.ServingPort) + } + case Socks5: + if opts.Socks5Options.ServingPort != 0 { + servingPort = int32(opts.Socks5Options.ServingPort) + } + default: + // Should never occur given validation at line 81-84, but defensive. + panic(fmt.Sprintf("unexpected konnectivity proxy mode: %s", opts.Mode)) + }As per coding guidelines, follow the 100-go-mistakes rules: prefer eliminating code duplication and using defensive programming patterns.
🤖 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 `@support/controlplane-component/konnectivity-container.go` around lines 184 - 198, The servingPort selection duplicates identical logic for HTTPS and Socks5 and lacks a default branch, so update the code that sets servingPort (variable servingPort using opts.Mode, HTTPS, Socks5, opts.HTTPSOptions.ServingPort, opts.Socks5Options.ServingPort) to remove duplication by first selecting the candidate port (e.g., from opts.HTTPSOptions.ServingPort or opts.Socks5Options.ServingPort based on opts.Mode) and then applying a single fallback to 8090 if the candidate is zero; also add a defensive default for the switch (or handle unknown modes) to ensure servingPort is always set to a valid default.
🤖 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 `@konnectivity-socks5-proxy/guard.go`:
- Around line 59-60: The retry backoff currently uses time.Sleep(delay()) which
ignores ctx cancellation; update both retry sites in guard.go (where
time.Sleep(delay()) is called) to use a cancellable wait: replace
time.Sleep(delay()) with a select that waits on time.After(delay()) and
ctx.Done(), returning or breaking when ctx is cancelled. Ensure the surrounding
function (the loop that calls delay()) respects ctx cancellation (e.g., check
ctx.Err() or return on ctx.Done()) so retries exit immediately when the provided
ctx is cancelled.
- Around line 30-35: coreGuardBackoff currently has Steps==0 so DelayFunc
returns a constant delay and exponential growth never happens; update
coreGuardBackoff to set a positive Steps (e.g., 5 or 10) so Factor and Cap take
effect, and replace every time.Sleep(delay()) call (where delay is produced by
coreGuardBackoff.DelayFunc or similar) with a ctx-aware wait: compute delay :=
delay() then use select { case <-ctx.Done(): return ctx.Err() (or break out
appropriately); case <-time.After(delay): /* continue retry */ } so retries exit
promptly on cancellation; reference coreGuardBackoff, DelayFunc and the sites
using time.Sleep(delay()) when making these edits.
In `@konnectivity-socks5-proxy/main.go`:
- Around line 55-71: signals.SetupSignalHandler() cancels ctx but the current
runWithCoreGuard call blocks on server.ListenAndServe and never observes
ctx.Done(), so SIGTERM doesn't stop the proxy and context cancellation becomes a
fatal error; change the socks5 server start to create a net.Listener (net.Listen
with fmt.Sprintf(":%d", servingPort)), run server.Serve(listener) in a
goroutine, and in the main goroutine select on ctx.Done() vs serve result: on
ctx.Done() call listener.Close() and wait for Serve to return and then return
nil (treat context cancellation as clean shutdown), and if Serve returns a
non-context error propagate it as before; update the block that constructs
socks5.Config and calls server.ListenAndServe to use this listener/Serve + ctx
cancellation handling so signals.SetupSignalHandler(), runWithCoreGuard,
server.Serve, and listener.Close() coordinate graceful shutdown.
---
Nitpick comments:
In `@konnectivity-socks5-proxy/guard_test.go`:
- Around line 28-58: Refactor TestIsTransientKonnectivityError into a
table-driven test: create a slice of cases with fields name, err and want (bool)
containing the current scenarios (nil, context.DeadlineExceeded,
fmt.Errorf("dial tcp: connection refused"), syscall.ECONNREFUSED,
errors.New("failed validation..."), os.ErrNotExist), then loop over cases
calling t.Run(case.name, func(t *testing.T){ g := NewGomegaWithT(t);
g.Expect(isTransientKonnectivityError(case.err)).To(Equal(case.want)) }); keep
the test function name TestIsTransientKonnectivityError and reuse
isTransientKonnectivityError and NewGomegaWithT to preserve existing assertions.
- Around line 95-103: The test mutates the package-level dialKonnectivityServer
which is unsafe for parallel tests; refactor retryDialKonnectivityServer (in
guard.go) to accept a dial function parameter (e.g., dialFunc
func(string,uint32) error or as a field on a struct) and update the test to call
retryDialKonnectivityServer with a local mockDial closure that increments
attempts and returns the simulated error on the first call and delegates to the
real dial (e.g., dialKonnectivityServerTCP) on success; remove any package-level
reassignment in tests and use dependency injection to supply the mock instead.
In `@support/controlplane-component/konnectivity-container.go`:
- Around line 184-198: The servingPort selection duplicates identical logic for
HTTPS and Socks5 and lacks a default branch, so update the code that sets
servingPort (variable servingPort using opts.Mode, HTTPS, Socks5,
opts.HTTPSOptions.ServingPort, opts.Socks5Options.ServingPort) to remove
duplication by first selecting the candidate port (e.g., from
opts.HTTPSOptions.ServingPort or opts.Socks5Options.ServingPort based on
opts.Mode) and then applying a single fallback to 8090 if the candidate is zero;
also add a defensive default for the switch (or handle unknown modes) to ensure
servingPort is always set to a valid default.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 56b1509a-1ce4-4bfd-9308-e453f94028b4
⛔ Files ignored due to path filters (40)
control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/AROSwift/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/GCP/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/AROSwift/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/GCP/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/AROSwift/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/GCP/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/IBMCloud/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/AROSwift/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/GCP/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/AROSwift/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/GCP/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/IBMCloud/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component.gokonnectivity-socks5-proxy/guard.gokonnectivity-socks5-proxy/guard_test.gokonnectivity-socks5-proxy/main.gosupport/controlplane-component/konnectivity-container.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
konnectivity-socks5-proxy/guard_test.go (1)
79-110: 🏗️ Heavy liftTest the production retry path instead of a local clone.
retryDialKonnectivityServerreimplements its own loop, so this test can stay green whilebootstrapKonnectivityorrunWithCoreGuardregress. Prefer injecting the dial/bootstrap dependency into the real code path and asserting retries there.As per coding guidelines, "Always include unit tests when creating new functions or modifying existing ones" and "Unit test any code changes and additions."
Also applies to: 113-131
🤖 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 `@konnectivity-socks5-proxy/guard_test.go` around lines 79 - 110, The test currently calls retryDialKonnectivityServer directly and stubs dialKonnectivityServer, which doesn't exercise the real production path; refactor the code to inject the dialing/bootstrap dependency (e.g., accept a DialFunc or a KonnectivityBootstrap interface) into bootstrapKonnectivity/runWithCoreGuard and update the test to call the real entrypoint (bootstrapKonnectivity or runWithCoreGuard) with a test dial implementation that fails once then succeeds, assert the entrypoint eventually succeeds and that the injected dial was invoked multiple times; keep references to dialKonnectivityServer, retryDialKonnectivityServer, bootstrapKonnectivity, and runWithCoreGuard to locate and change signatures, and ensure tests restore any global state and use context timeouts as before.
🤖 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 `@konnectivity-socks5-proxy/guard.go`:
- Around line 36-37: Thread a context through bootstrapKonnectivity →
tryBootstrapKonnectivity → dialKonnectivityServerTCP so the TCP probe is
cancellable: add a ctx parameter to bootstrapKonnectivity and propagate it into
tryBootstrapKonnectivity and dialKonnectivityServerTCP, then replace the
net.DialTimeout(...) call inside dialKonnectivityServerTCP with
(&net.Dialer{Timeout: konnectivityDialTimeout}).DialContext(ctx, network, addr)
so the in-progress probe is aborted when ctx is cancelled; update all callers to
pass the ctx accordingly.
---
Nitpick comments:
In `@konnectivity-socks5-proxy/guard_test.go`:
- Around line 79-110: The test currently calls retryDialKonnectivityServer
directly and stubs dialKonnectivityServer, which doesn't exercise the real
production path; refactor the code to inject the dialing/bootstrap dependency
(e.g., accept a DialFunc or a KonnectivityBootstrap interface) into
bootstrapKonnectivity/runWithCoreGuard and update the test to call the real
entrypoint (bootstrapKonnectivity or runWithCoreGuard) with a test dial
implementation that fails once then succeeds, assert the entrypoint eventually
succeeds and that the injected dial was invoked multiple times; keep references
to dialKonnectivityServer, retryDialKonnectivityServer, bootstrapKonnectivity,
and runWithCoreGuard to locate and change signatures, and ensure tests restore
any global state and use context timeouts as before.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a56c9242-6df5-4a69-85b4-450d2a131e1a
⛔ Files ignored due to path filters (40)
control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/AROSwift/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/GCP/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/AROSwift/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/GCP/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/AROSwift/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/GCP/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/IBMCloud/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/AROSwift/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/GCP/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/AROSwift/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/GCP/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/IBMCloud/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component.gokonnectivity-socks5-proxy/guard.gokonnectivity-socks5-proxy/guard_test.gokonnectivity-socks5-proxy/main.gosupport/controlplane-component/konnectivity-container.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
konnectivity-socks5-proxy/main.go (1)
94-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat bootstrap-time context cancellation as a clean exit.
If
runWithCoreGuardreturnscontext.Canceledwhile still bootstrapping, Lines 94-96 still print an error andos.Exit(1). That makes an intentional SIGTERM during startup look like a crash.Suggested change
import ( + "context" + "errors" "fmt" "net" "os" @@ }) if err != nil { + if errors.Is(err, context.Canceled) { + return + } fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } }🤖 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 `@konnectivity-socks5-proxy/main.go` around lines 94 - 96, When handling the error returned from runWithCoreGuard, treat bootstrap-time cancellation as a clean exit by checking if errors.Is(err, context.Canceled) (or err == context.Canceled) and, if so, do not print an error and exit 0; otherwise keep the existing fmt.Fprintf(os.Stderr, ...) and os.Exit(1). Update the error handling around runWithCoreGuard to perform this conditional check (and add an import for the errors package if needed) so a SIGTERM during startup is treated as a normal shutdown.
🧹 Nitpick comments (1)
konnectivity-socks5-proxy/main_test.go (1)
100-134: ⚡ Quick winExercise the production shutdown path instead of a copied closure.
This subtest reimplements the listener/
Serve/ctx.Done()flow locally, so it can stay green even ifNewStartCommanddrifts. Extract that block into a small package-private helper and call the helper from bothmain.goand this test.As per coding guidelines, "Unit test any code changes and additions".
🤖 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 `@konnectivity-socks5-proxy/main_test.go` around lines 100 - 134, The test duplicates the listener/Serve/ctx.Done() shutdown logic (serveFunc) instead of exercising the real production path; refactor that logic into a package-private helper (e.g., create a function serveWithGracefulShutdown(ctx context.Context, servingPort uint32, server *socks5.Server) error) and replace the test-local closure and the implementation in main.go to call this helper; update NewStartCommand (or the function in main.go that currently inlines the logic) to call serveWithGracefulShutdown so the test can simply invoke the real code path and verify graceful shutdown, and adjust tests to pass a server instance and listener port to the new helper.
🤖 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 `@konnectivity-socks5-proxy/guard_test.go`:
- Around line 213-219: The test currently dials a hardcoded port 54321 which can
be occupied on CI; instead create a short-lived listener on "127.0.0.1:0" to get
an ephemeral port, read its actual port from listener.Addr(), close the listener
to free the port, then call dialKonnectivityServerTCP(ctx, "127.0.0.1", port)
and assert error with isTransientKonnectivityError; update the anonymous t.Run
block to use this ephemeral-port pattern and keep the same assertions using
dialKonnectivityServerTCP and isTransientKonnectivityError.
---
Duplicate comments:
In `@konnectivity-socks5-proxy/main.go`:
- Around line 94-96: When handling the error returned from runWithCoreGuard,
treat bootstrap-time cancellation as a clean exit by checking if errors.Is(err,
context.Canceled) (or err == context.Canceled) and, if so, do not print an error
and exit 0; otherwise keep the existing fmt.Fprintf(os.Stderr, ...) and
os.Exit(1). Update the error handling around runWithCoreGuard to perform this
conditional check (and add an import for the errors package if needed) so a
SIGTERM during startup is treated as a normal shutdown.
---
Nitpick comments:
In `@konnectivity-socks5-proxy/main_test.go`:
- Around line 100-134: The test duplicates the listener/Serve/ctx.Done()
shutdown logic (serveFunc) instead of exercising the real production path;
refactor that logic into a package-private helper (e.g., create a function
serveWithGracefulShutdown(ctx context.Context, servingPort uint32, server
*socks5.Server) error) and replace the test-local closure and the implementation
in main.go to call this helper; update NewStartCommand (or the function in
main.go that currently inlines the logic) to call serveWithGracefulShutdown so
the test can simply invoke the real code path and verify graceful shutdown, and
adjust tests to pass a server instance and listener port to the new helper.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 099bd414-6684-4f69-8d67-a453ed5f21b3
📒 Files selected for processing (5)
konnectivity-socks5-proxy/guard.gokonnectivity-socks5-proxy/guard_test.gokonnectivity-socks5-proxy/main.gokonnectivity-socks5-proxy/main_test.gosupport/controlplane-component/konnectivity-container_test.go
Contributor
Author
|
/test all |
Contributor
Author
|
/pipeline required |
Contributor
|
Scheduling tests matching the |
Contributor
Author
|
/test e2e-v2-gke |
Contributor
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aws
e2e-aks
|
Contributor
Author
|
/test e2e-aws |
Contributor
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
Contributor
|
/area dependencies |
Contributor
|
@jparrill: The label(s) 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 kubernetes-sigs/prow repository. |
jparrill
reviewed
May 26, 2026
jparrill
left a comment
jparrill
left a comment
Contributor
There was a problem hiding this comment.
Dropped some comments. Thanks
Apart from those comments inline, you will need to:
- Create a bug in OCPBUGS project
/retitle OCPBUGS-XXXXX: Konnectivity retry proxy connection on timeout/jira refresh
enxebre
reviewed
May 26, 2026
Contributor
Author
|
/pipeline required |
Contributor
|
Scheduling tests matching the |
Member
|
@YamunadeviShanmugam thanks for your PR! Overall it looks good, but it is hard to get an overview. Could you rebase your branch instead of adding a merge commit on top and squash your 2 commits with the same message? |
YamunadeviShanmugam
force-pushed
the
fix-add-startup-probe-konnectivity-sock5-sidecar
branch
from
July 27, 2026 11:59
69a1e36 to
56176f1
Compare
openshift-ci
Bot
added
the
needs-rebase
YamunadeviShanmugam
force-pushed
the
fix-add-startup-probe-konnectivity-sock5-sidecar
branch
from
July 27, 2026 12:02
56176f1 to
1f9d879
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: YamunadeviShanmugam 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 |
YamunadeviShanmugam
force-pushed
the
fix-add-startup-probe-konnectivity-sock5-sidecar
branch
3 times, most recently
from
July 28, 2026 04:47
2aac4ad to
2f9d51e
Compare
Contributor
Author
|
@clebs : Squashed all commits to single commit, thanks for looking into this! |
Member
|
Is this comment from @enxebre resolved? https://github.com/openshift/hypershift/pull/8579/changes#r3504381520 |
Contributor
Author
Yes @clebs |
Member
|
/lgtm |
Contributor
|
Scheduling tests matching the |
Contributor
Author
|
/test e2e-azure-v2-self-managed e2e-aws e2e-v2-aws |
openshift-ci
Bot
added
needs-rebase
The proxy was exiting hard on startup timeouts.
Contributor
Author
Member
|
/lgtm |
Contributor
|
Scheduling tests matching the |
Contributor
Author
|
/test e2e-kubevirt-aws-ovn-reduced |
Member
|
@CodeRabbit resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
support/controlplane-component/konnectivity-container.go (2)
237-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the startup-probe timing constants.
These literals define the startup policy. Give them named constants near
DefaultServingPortand reuse them in tests. This keeps the retry budget explicit.As per coding guidelines: “Avoid magic numbers — use named constants.”
Suggested refactor
const ( DefaultServingPort int32 = 8090 + startupProbeInitialDelaySeconds int32 = 5 + startupProbePeriodSeconds int32 = 2 + startupProbeFailureThreshold int32 = 30 + startupProbeTimeoutSeconds int32 = 1 ) ... - InitialDelaySeconds: 5, - PeriodSeconds: 2, - FailureThreshold: 30, - TimeoutSeconds: 1, + InitialDelaySeconds: startupProbeInitialDelaySeconds, + PeriodSeconds: startupProbePeriodSeconds, + FailureThreshold: startupProbeFailureThreshold, + TimeoutSeconds: startupProbeTimeoutSeconds,🤖 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 `@support/controlplane-component/konnectivity-container.go` around lines 237 - 240, The startup probe timing values should not remain as magic numbers. Define named constants for the initial delay, period, failure threshold, and timeout near DefaultServingPort, then use those constants in the startup probe configuration and corresponding tests.Source: Coding guidelines
203-213: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the production
Dualport contract in the test.The production
Dualcaller sets HTTPS to8092and leaves Socks5 at its8090default. The startup probes match these listeners, so no port conflict occurs. Assert these command arguments and probe ports in theDualtest.🤖 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 `@support/controlplane-component/konnectivity-container.go` around lines 203 - 213, Update the Dual-mode test to configure HTTPS on port 8092 while leaving Socks5 at its default 8090, then assert the generated command arguments and startup probe ports match those listeners. Use the existing Dual test setup and its command/probe assertions without changing production behavior.
🤖 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 `@support/controlplane-component/konnectivity-container.go`:
- Around line 237-239: Update the startup probe configuration around
InitialDelaySeconds, PeriodSeconds, and FailureThreshold so the total startup
allowance, including container startup, is no more than 60 seconds. Remove the
initial delay or reduce the failure budget while preserving the existing probe
behavior.
- Around line 206-211: Update the ServingPort handling in the HTTPS and Socks5
branches to validate nonzero values are within 1–65535 before converting them to
int32 or using them for command and startup-probe configuration. Reject invalid
values, while preserving 0 as the default sentinel and keeping valid ports
unchanged.
---
Nitpick comments:
In `@support/controlplane-component/konnectivity-container.go`:
- Around line 237-240: The startup probe timing values should not remain as
magic numbers. Define named constants for the initial delay, period, failure
threshold, and timeout near DefaultServingPort, then use those constants in the
startup probe configuration and corresponding tests.
- Around line 203-213: Update the Dual-mode test to configure HTTPS on port 8092
while leaving Socks5 at its default 8090, then assert the generated command
arguments and startup probe ports match those listeners. Use the existing Dual
test setup and its command/probe assertions without changing production
behavior.
🪄 Autofix
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d136419a-baf2-4a02-a8a8-6c5e1b71e48b
⛔ Files ignored due to path filters (48)
control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/AROSwift/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/GCP/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/catalog-operator/zz_fixture_TestControlPlaneComponents_catalog_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-network-operator/zz_fixture_TestControlPlaneComponents_cluster_network_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/AROSwift/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/GCP/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/AROSwift/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/GCP/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/IBMCloud/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/ModernTLS/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/oauth-openshift/zz_fixture_TestControlPlaneComponents_oauth_openshift_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/AROSwift/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/GCP/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/olm-operator/zz_fixture_TestControlPlaneComponents_olm_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/ModernTLS/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-apiserver/zz_fixture_TestControlPlaneComponents_openshift_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/GCP/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/ModernTLS/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/openshift-oauth-apiserver/zz_fixture_TestControlPlaneComponents_openshift_oauth_apiserver_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/AROSwift/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/GCP/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/IBMCloud/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/ModernTLS/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/packageserver/zz_fixture_TestControlPlaneComponents_packageserver_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (10)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/packageserver/component_test.gokonnectivity-socks5-proxy/guard.gokonnectivity-socks5-proxy/guard_test.gokonnectivity-socks5-proxy/main.gokonnectivity-socks5-proxy/main_test.gosupport/controlplane-component/konnectivity-container.gosupport/controlplane-component/konnectivity-container_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- konnectivity-socks5-proxy/main.go
- control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/component.go
- konnectivity-socks5-proxy/guard.go
- konnectivity-socks5-proxy/main_test.go
- support/controlplane-component/konnectivity-container_test.go
- konnectivity-socks5-proxy/guard_test.go
Comment on lines
+206
to
+211
| if opts.HTTPSOptions.ServingPort != 0 { | ||
| servingPort = int32(opts.HTTPSOptions.ServingPort) | ||
| } | ||
| case Socks5: | ||
| if opts.Socks5Options.ServingPort != 0 { | ||
| servingPort = int32(opts.Socks5Options.ServingPort) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 '\bServingPort\b' --glob '*.go'Repository: openshift/hypershift
Length of output: 158
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- tracked candidate files ---'
git ls-files 'support/controlplane-component/konnectivity-container.go' 'support/controlplane-component/*' | head -50
printf '%s\n' '--- file status ---'
if [ -f support/controlplane-component/konnectivity-container.go ]; then
wc -l support/controlplane-component/konnectivity-container.go
sed -n '1,280p' support/controlplane-component/konnectivity-container.go
else
printf '%s\n' 'candidate file not found at the supplied path'
fi
printf '%s\n' '--- ServingPort references ---'
rg -n -C 4 'ServingPort|HTTPSOptions|Socks5Options' support/controlplane-component --glob '*.go' || trueRepository: openshift/hypershift
Length of output: 40176
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- local contract and tests ---'
sed -n '1,240p' support/controlplane-component/README.md
sed -n '1,130p' support/controlplane-component/konnectivity-container_test.go
printf '%s\n' '--- option construction and port consumers ---'
rg -n -C 4 'HTTPSOptions\s*:|Socks5Options\s*:|ServingPort\s*:|serving-port|DefaultServingPort' --glob '*.go' --glob '*.yaml' --glob '*.yml' . || true
printf '%s\n' '--- Kubernetes and operator port validation sources ---'
rg -n -C 4 'type TCPSocketAction|FromInt32|servingPort|ServingPort|serving-port' --glob '*.go' --glob 'go.mod' --glob 'go.sum' vendor 2>/dev/null || true
rg -n -C 4 'konnectivity-(https|socks5)-proxy|serving-port' --glob '*.go' . || trueRepository: openshift/hypershift
Length of output: 50378
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- proxy command implementations ---'
sed -n '1,180p' konnectivity-https-proxy/cmd.go
sed -n '1,160p' konnectivity-socks5-proxy/main.go
printf '%s\n' '--- exact Kubernetes dependency versions ---'
rg -n 'k8s.io/(api|apimachinery)' go.mod go.sum | head -20
printf '%s\n' '--- relevant API validation implementations if vendored ---'
find . -path '*/k8s.io/api*' -o -path '*/k8s.io/apimachinery*' | head -30
rg -n -C 5 'TCPSocketAction|Validate.*Port|port.*65535|MaxPort' --glob '*.go' . | head -160 || true
printf '%s\n' '--- deterministic uint32-to-int32 cases ---'
python3 - <<'PY'
values = [0, 8090, 65535, 2_147_483_647, 2_147_483_648, 3_000_000_000, 4_294_967_295]
for value in values:
narrowed = value & 0xffffffff
if narrowed >= 2**31:
narrowed -= 2**32
print(f"{value:>12} -> {narrowed:>12}")
PYRepository: openshift/hypershift
Length of output: 13346
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- all ServingPort assignments and call sites ---'
rg -n -C 2 'ServingPort\s*:|ServingPort\b' --glob '*.go' . | sed -n '1,240p'
printf '%s\n' '--- vendored Kubernetes probe and port validation ---'
rg -n -C 6 'type TCPSocketAction|TCPSocketAction|validate.*Port|Validate.*Port|MaxPort|65535' vendor/k8s.io/api vendor/k8s.io/apimachinery 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- SOCKS5 serving path ---'
rg -n -C 5 'serveWithGracefulShutdown|Listen|servingPort' konnectivity-socks5-proxy --glob '*.go' | sed -n '1,220p'
printf '%s\n' '--- deterministic conversion and valid-port boundary ---'
python3 - <<'PY'
values = [0, 8090, 65535, 65536, 2_147_483_647, 2_147_483_648, 3_000_000_000, 4_294_967_295]
for value in values:
narrowed = value if value < 2**31 else value - 2**32
print(f"uint32 {value:>12} -> int32 {narrowed:>12}; valid TCP port: {1 <= value <= 65535}")
PYRepository: openshift/hypershift
Length of output: 43496
Validate ServingPort before narrowing it to int32.
ServingPort must be in the range 1–65535. Reject nonzero values outside this range before building the command and startup probe. Values above the signed 32-bit range become negative or unrelated probe ports, while --serving-port keeps the original value. Keep 0 as the default sentinel.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 206-206: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(opts.HTTPSOptions.ServingPort)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 210-210: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(opts.Socks5Options.ServingPort)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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 `@support/controlplane-component/konnectivity-container.go` around lines 206 -
211, Update the ServingPort handling in the HTTPS and Socks5 branches to
validate nonzero values are within 1–65535 before converting them to int32 or
using them for command and startup-probe configuration. Reject invalid values,
while preserving 0 as the default sentinel and keeping valid ports unchanged.
Sources: Coding guidelines, Path instructions, Linters/SAST tools
Comment on lines
+237
to
+239
| InitialDelaySeconds: 5, | ||
| PeriodSeconds: 2, | ||
| FailureThreshold: 30, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="support/controlplane-component/konnectivity-container.go"
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" || true
printf '%s\n' '--- relevant source ---'
sed -n '1,280p' "$file"
printf '%s\n' '--- related port/probe references ---'
rg -n -C 3 'InitialDelaySeconds|PeriodSeconds|FailureThreshold|StartupProbe|servingPort|ServingPort|SOCKS5|Dual|dual|konnectivity' support/controlplane-component --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*_test.go' || true
printf '%s\n' '--- lifecycle-component documentation ---'
sed -n '1,240p' support/controlplane-component/README.mdRepository: openshift/hypershift
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="support/controlplane-component/konnectivity-container.go"
printf '%s\n' '--- changed lines ---'
git diff --unified=20 -- "$file" support/controlplane-component/konnectivity-container_test.go
printf '%s\n' '--- complete focused tests ---'
sed -n '1,210p' support/controlplane-component/konnectivity-container_test.go
sed -n '310,365p' support/controlplane-component/konnectivity-container_test.go
printf '%s\n' '--- all initialization-window references ---'
rg -n -i -C 3 '60 seconds|60-second|startup window|initializ|startup probe|failure threshold|initial delay|period seconds' . \
--glob '!vendor/**' --glob '!third_party/**' --glob '!**/go.sum' | head -n 300
printf '%s\n' '--- konnectivity proxy probe and port contracts ---'
rg -n -i -C 4 'konnectivity-(https|socks5)-proxy|serving-port|DefaultServingPort|8090|8092|startupProbe|startup-probe' . \
--glob '!vendor/**' --glob '!third_party/**' --glob '!**/go.sum' | head -n 500
printf '%s\n' '--- relevant git metadata ---'
git diff --stat -- "$file" support/controlplane-component/konnectivity-container_test.go
git status --shortRepository: openshift/hypershift
Length of output: 50378
🌐 Web query:
Kubernetes startup probe initialDelaySeconds periodSeconds failureThreshold total startup time formula
💡 Result:
The maximum time a container is allowed to complete its startup before it is considered failed by a Kubernetes startup probe is calculated using the following formula [1][2][3]: Total Startup Time = failureThreshold * periodSeconds [1][4][2] In this formula [2]: - failureThreshold: The number of consecutive failed attempts the kubelet will tolerate before restarting the container [5]. - periodSeconds: The time interval (in seconds) between each probe attempt [5]. If a startup probe is configured, it disables liveness and readiness probes until it succeeds [2][3]. Once the startup probe succeeds, it does not run again for the life of that container, and the liveness and readiness probes take over [1][4][3]. If the startup probe fails to succeed within this calculated window, the kubelet kills the container, and the pod's restartPolicy determines the next action [1][6]. While startup probes also support initialDelaySeconds, it is generally recommended to rely primarily on failureThreshold and periodSeconds to define the startup window [7]. If initialDelaySeconds is used, it adds an additional static wait time before the first probe attempt occurs, which can lead to unnecessarily long startup times [5][7].
Citations:
- 1: https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/
- 2: https://kubernetes.recipes/recipes/configuration/kubernetes-startup-probe-slow-containers/
- 3: https://kubernetes.recipes/recipes/configuration/kubernetes-container-probes-startup/
- 4: https://jorijn.com/en/knowledge-base/kubernetes/troubleshooting/configure-kubernetes-health-probes/
- 5: https://kubernetes.io/docs/concepts/workloads/pods/probes/
- 6: https://github.com/kubernetes/enhancements/blob/c1cec820b3b3d0fa18dede73107a2cbb43e27e33/keps/sig-node/950-liveness-probe-holdoff/README.md
- 7: https://stackoverflow.com/questions/65113084/k8s-pod-with-startupprobe-and-initialdelayseconds-specified-waits-too-long-to-be
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("support/controlplane-component/konnectivity-container.go").read_text()
fields = {
"initial_delay_seconds": r"InitialDelaySeconds:\s*(\d+)",
"period_seconds": r"PeriodSeconds:\s*(\d+)",
"failure_threshold": r"FailureThreshold:\s*(\d+)",
"timeout_seconds": r"TimeoutSeconds:\s*(\d+)",
}
values = {name: int(re.search(pattern, source).group(1)) for name, pattern in fields.items()}
first_probe = values["initial_delay_seconds"]
last_probe = first_probe + (values["failure_threshold"] - 1) * values["period_seconds"]
failure_budget = values["failure_threshold"] * values["period_seconds"]
configured_upper_bound = values["initial_delay_seconds"] + failure_budget
print(values)
print(f"first probe: {first_probe}s after container start")
print(f"{values['failure_threshold']}th probe start: {last_probe}s after container start")
print(f"failure budget after initial delay: {failure_budget}s")
print(f"configured upper-bound calculation including initial delay: {configured_upper_bound}s")
for token in ("60 seconds", "60-second", "startup window", "initialization window"):
print(f"{token!r} references outside ignored vendor paths:",
bool(re.search(re.escape(token), source, re.IGNORECASE)))
PY
printf '%s\n' '--- focused option call sites and probe-related comments ---'
rg -n -i -C 3 'KonnectivityContainerOptions|HTTPSOptions|Socks5Options|startup probe|startupProbe|60.?second|initializ' \
--glob '*.go' --glob '*.yaml' --glob '*.yml' \
--glob '!api/vendor/**' --glob '!vendor/**' \
control-plane-operator support konnectivity-https-proxy konnectivity-socks5-proxy | head -n 500Repository: openshift/hypershift
Length of output: 50376
Align startup probe timing with the 60-second startup budget.
FailureThreshold: 30 × PeriodSeconds: 2 provides a 60-second failure budget after InitialDelaySeconds: 5. From container start, the nominal upper bound is 65 seconds. Remove the delay or reduce the failure budget if the 60-second limit includes container startup.
🤖 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 `@support/controlplane-component/konnectivity-container.go` around lines 237 -
239, Update the startup probe configuration around InitialDelaySeconds,
PeriodSeconds, and FailureThreshold so the total startup allowance, including
container startup, is no more than 60 seconds. Remove the initial delay or
reduce the failure budget while preserving the existing probe behavior.
Contributor
|
@YamunadeviShanmugam: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the intermittent E2E test failures where the konnectivity-proxy-socks5 sidecar container crashes and restarts during cluster bootstrap. Fix includes retry loop, native Kubernetes startup probes, and explicit component dependency ordering.
Proxy Internal Retry Logic
Native Startup Probes
konnectivity-container.go: Added a native TCP startup probe to all Konnectivity sidecar containers. This gives the sidecars a safe 60-second window to initialize during cluster bootstrap before any readiness or liveness checks trigger.
Component Dependency Ordering
Deployment graph is updated with dependency check. The openshift-oauth-apiserver pods will now explicitly wait for the konnectivity-agent to be completely Available and RolloutComplete before they begin scheduling.
Test Fixtures are updated accordingly
Summary by CodeRabbit
New Features
Bug Fixes
Tests