CNTRLPLANE-3510: Enable additional golangci-lint linters#8567
Open
bryan-cox wants to merge 7 commits into
Open
CNTRLPLANE-3510: Enable additional golangci-lint linters#8567bryan-cox wants to merge 7 commits into
bryan-cox wants to merge 7 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request explicitly references no jira issue. 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR standardizes error handling and context usage across the repository: it enables the errorlint and nilerr linters; replaces many non-wrapping fmt.Errorf uses with %w; converts direct type assertions/equality to errors.As / errors.Is where appropriate; makes HTTP requests, dialing, and some command executions context-aware (http.NewRequestWithContext, DialContext, exec.CommandContext); adds //nolint:nilerr annotations on intentional retry branches returning nil errors; and applies corresponding test updates. No public API signatures were changed. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
bryan-cox
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8567 +/- ##
==========================================
+ Coverage 40.34% 40.45% +0.11%
==========================================
Files 755 755
Lines 93167 93259 +92
==========================================
+ Hits 37587 37728 +141
+ Misses 52877 52831 -46
+ Partials 2703 2700 -3
... and 4 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: 2
🤖 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 `@cmd/nodepool/core/create.go`:
- Around line 198-201: The Get call that checks the HostedCluster payload
currently swallows all errors (client.Get(..., hc) then return nil), which hides
transient API/RBAC issues; update the error handling to only ignore NotFound by
using apierrors.IsNotFound(err) (from k8s.io/apimachinery/pkg/api/errors) — if
IsNotFound(err) return nil, otherwise return the error (or wrap and return it);
ensure you add the import for apierrors and adjust the branch after the
client.Get so logger.Info remains for NotFound but other errors are propagated
instead of being dropped.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 4469-4471: The code currently embeds the raw AWS error text into
the returned error (fmt.Errorf("%w: aws returned an error: %w", wrapped, err)),
which then flows into the ValidOIDCConfiguration status message; change this to
avoid surfacing provider-specific error text by returning a generic provider
error (e.g., fmt.Errorf("%w: aws returned an error", wrapped)) and separately
log the original err to the controller logger (or record it as an event) so the
detailed AWS error is available in logs but not in the status message; update
the site that constructs/returns wrapped (the place where variables wrapped and
err are used with fmt.Errorf) to remove embedding err and ensure you call the
controller logger (or event recorder) to log err.
🪄 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: 567af7d2-b72e-4f37-b4ed-c788a51be81d
📒 Files selected for processing (86)
.golangci.ymlcmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/infra/aws/route53.go`:
- Around line 189-191: The cleanup currently swallows every error from
LookupZone; change it to only ignore the specific "zone not found" case and
return any other error. Update the block around the LookupZone call (id, err :=
LookupZone(ctx, client, name, false)) to inspect err: if it matches the
sentinel/not-found condition returned by LookupZone (or use errors.Is/ a helper
like IsZoneNotFound(err) / AWS's NotFound error check), then continue silently,
otherwise return the error so real Route53/API failures are propagated.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Line 1435: The error return wraps the wrong variable because the outer err
(from platform.GetPlatform) is nil; update the return in the
PlatformCredentialsFound status update failure so only the statusErr is wrapped
with %w and the reconcile error is included with a non-wrapping verb (or capture
the reconcile error into a separate variable). Locate the code around
p.ReconcileCredentials(...) and the fmt.Errorf(...) return and change the
formatting to include the reconcile error without %w (e.g., %v) and use %w only
for statusErr so you don't accidentally wrap a nil outer err.
🪄 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: c16340aa-bb8a-4eb6-91c1-7304d463990f
📒 Files selected for processing (86)
.golangci.ymlcmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.go
✅ Files skipped from review due to trivial changes (16)
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.go
- etcd-recovery/etcdrecovery.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.go
- test/e2e/util/reqserving/vpa.go
- support/thirdparty/docker/pkg/archive/archive.go
- .golangci.yml
- test/e2e/util/reqserving/verifycp.go
- hypershift-operator/controllers/nodepool/conditions.go
- test/e2e/util/node.go
- hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.go
- test/e2e/util/reqserving/waitfor.go
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.go
- karpenter-operator/controllers/karpenter/machine_approver.go
- test/e2e/util/aws.go
- test/e2e/util/oauth.go
| }) | ||
| if statusErr := r.Client.Status().Update(ctx, hcluster); statusErr != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %s, failed to update status: %w", err, statusErr) | ||
| return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %w, failed to update status: %w", err, statusErr) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go"
# Print a window around the referenced line with line numbers
nl -ba "$FILE" | sed -n '1400,1480p'Repository: openshift/hypershift
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go"
# Show numbered lines around the reported line
cat -n "$FILE" | sed -n '1400,1480p'
echo "----"
# Find the exact error string occurrence(s)
rg -n "failed to reconcile platform credentials" "$FILE" || trueRepository: openshift/hypershift
Length of output: 4670
Fix error wrapping to avoid nil/stale err in PlatformCredentialsFound status update failure.
Line 1435 is outside the if err := p.ReconcileCredentials(...); ... shadowed scope, so %w wraps the outer err from platform.GetPlatform (nil when execution reaches this branch). Wrap only statusErr.
Suggested fix
- return ctrl.Result{}, fmt.Errorf("failed to reconcile platform credentials: %w, failed to update status: %w", err, statusErr)
+ return ctrl.Result{}, fmt.Errorf("failed to update PlatformCredentialsFound status: %w", statusErr)🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` at
line 1435, The error return wraps the wrong variable because the outer err (from
platform.GetPlatform) is nil; update the return in the PlatformCredentialsFound
status update failure so only the statusErr is wrapped with %w and the reconcile
error is included with a non-wrapping verb (or capture the reconcile error into
a separate variable). Locate the code around p.ReconcileCredentials(...) and the
fmt.Errorf(...) return and change the formatting to include the reconcile error
without %w (e.g., %v) and use %w only for statusErr so you don't accidentally
wrap a nil outer err.
Member
Author
There was a problem hiding this comment.
This isn't part of the changes in this PR. Scope creep.
Contributor
There was a problem hiding this comment.
@bryan-cox Understood—agreed, that’s outside the linter-focused scope of this PR. I’ll drop that suggestion here.
Would you like me to open a follow-up GitHub issue to track it? If yes, I’ll file one referencing this PR/comment with something like:
- Title: hostedcluster_controller: wrap only statusErr on PlatformCredentialsFound status update failure
- Summary: In hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (around Line 1435), the return path on status update failure wraps the wrong variable; should wrap only statusErr for accurate error propagation.
openshift-ci
Bot
added
the
area/platform/azure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
1004-1011:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose HTTP response body in KAS health check.
httpClient.Do(req)response is never closed. In a recurrent reconcile loop this can leak connections and eventually degrade controller reliability.Suggested fix
resp, err := httpClient.Do(req) if err != nil { return err } +defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("APIServer endpoint %s is not healthy", ingressPoint) }🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 1004 - 1011, The HTTP response from httpClient.Do(req) is not closed, which can leak connections; after the successful call to httpClient.Do(req) (the resp variable returned) add proper cleanup by ensuring the response body is consumed/ discarded as needed and closed (e.g., call io.Copy(io.Discard, resp.Body) then defer resp.Body.Close() immediately after checking err) before checking resp.StatusCode for ingressPoint health so connections are returned to the pool.kubernetes-default-proxy/kubernetes_default_proxy.go (1)
70-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the listener on ctx cancellation so
Accept()unblocks and shutdown completes.
net.ListenConfig.Listen(ctx, ...)only usesctxduring the listen setup; cancelingctxdoes not close the returnednet.Listener, so a blockedlistener.Accept()can remain stuck. Trigger shutdown by callinglistener.Close()whenctx.Done()fires, and whenAccept()returns an error, exit cleanly when the error is due to closure (e.g.,net.ErrClosed) / whenctx.Err()!=nil—don’t just log andcontinue.🤖 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 `@kubernetes-default-proxy/kubernetes_default_proxy.go` around lines 70 - 84, The listener created by (&net.ListenConfig{}).Listen(ctx, ...) must be closed when ctx is cancelled so Accept() unblocks: start a goroutine that waits for <-ctx.Done() and calls listener.Close(), and change the Accept() error handling in the accept loop (function/method containing listener, Accept, s.log and ctx) to stop continuing on errors caused by closure—use errors.Is(err, net.ErrClosed) or check ctx.Err()!=nil and then return (or return ctx.Err()) instead of logging and continue; keep logging for unexpected Accept errors only.
🤖 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 `@availability-prober/availability_prober.go`:
- Around line 120-127: The infinite retry loop around creating and sending the
request (the for ; ; time.Sleep(sleepTime) loop that calls
http.NewRequestWithContext(ctx, ...) and client.Do(req)) doesn't stop when ctx
is canceled; update the loop to observe ctx.Done() and exit cleanly: before
sleeping or retrying check if ctx.Err() != nil (or select on ctx.Done()) and
break/return when canceled, and also after a failed client.Do(req) check
ctx.Err() and stop rather than continuing; ensure this change touches the loop
that constructs req and calls client.Do so the goroutine can exit when the
provided context is canceled.
---
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 1004-1011: The HTTP response from httpClient.Do(req) is not
closed, which can leak connections; after the successful call to
httpClient.Do(req) (the resp variable returned) add proper cleanup by ensuring
the response body is consumed/ discarded as needed and closed (e.g., call
io.Copy(io.Discard, resp.Body) then defer resp.Body.Close() immediately after
checking err) before checking resp.StatusCode for ingressPoint health so
connections are returned to the pool.
In `@kubernetes-default-proxy/kubernetes_default_proxy.go`:
- Around line 70-84: The listener created by (&net.ListenConfig{}).Listen(ctx,
...) must be closed when ctx is cancelled so Accept() unblocks: start a
goroutine that waits for <-ctx.Done() and calls listener.Close(), and change the
Accept() error handling in the accept loop (function/method containing listener,
Accept, s.log and ctx) to stop continuing on errors caused by closure—use
errors.Is(err, net.ErrClosed) or check ctx.Err()!=nil and then return (or return
ctx.Err()) instead of logging and continue; keep logging for unexpected Accept
errors only.
🪄 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: 47b1656b-eed1-4f13-beb4-589d2a91e790
📒 Files selected for processing (98)
.golangci.ymlavailability-prober/availability_prober.gocmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/azure/rbac.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/endpoint-resolver/server_test.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-operator/metrics-proxy/proxy_test.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokubernetes-default-proxy/kubernetes_default_proxy.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosharedingress-config-generator/controller.gosharedingress-config-generator/controller_test.gosharedingress-config-generator/haproxy_client.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/dump/journals.gotest/e2e/util/external_oidc.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.gotest/integration/framework/hosted-cluster.go
✅ Files skipped from review due to trivial changes (14)
- support/validations/authentication.go
- support/util/util.go
- .golangci.yml
- control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
- hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go
- test/e2e/util/aws.go
- hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go
- test/e2e/util/reqserving/vpa.go
- etcd-backup/etcdbackup.go
- test/e2e/util/node.go
- karpenter-operator/controllers/karpenter/machine_approver.go
- test/e2e/util/reqserving/verifycp.go
- cmd/nodepool/core/create.go
- cmd/cluster/core/create.go
Comment on lines
120
to
127
| for ; ; time.Sleep(sleepTime) { | ||
| response, err := client.Get(target.String()) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) | ||
| if err != nil { | ||
| log.Error(err, "Failed to create request, retrying...") | ||
| continue | ||
| } | ||
| response, err := client.Do(req) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
Handle context cancellation to stop the probe loop.
Line 120 currently retries forever. Once ctx is canceled, Do(req) will keep failing and this loop never exits.
Suggested fix
for ; ; time.Sleep(sleepTime) {
+ select {
+ case <-ctx.Done():
+ log.Info("probe canceled, exiting", "reason", ctx.Err())
+ return
+ default:
+ }
req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil)
if err != nil {
log.Error(err, "Failed to create request, retrying...")
continue
}
response, err := client.Do(req)
if err != nil {
+ if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+ log.Info("probe canceled, exiting", "reason", err)
+ return
+ }
log.Error(err, "Request failed, retrying...")
continue
}As per coding guidelines "Do not leak goroutines — ensure they exit cleanly".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for ; ; time.Sleep(sleepTime) { | |
| response, err := client.Get(target.String()) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) | |
| if err != nil { | |
| log.Error(err, "Failed to create request, retrying...") | |
| continue | |
| } | |
| response, err := client.Do(req) | |
| if err != nil { | |
| for ; ; time.Sleep(sleepTime) { | |
| select { | |
| case <-ctx.Done(): | |
| log.Info("probe canceled, exiting", "reason", ctx.Err()) | |
| return | |
| default: | |
| } | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) | |
| if err != nil { | |
| log.Error(err, "Failed to create request, retrying...") | |
| continue | |
| } | |
| response, err := client.Do(req) | |
| if err != nil { | |
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| log.Info("probe canceled, exiting", "reason", err) | |
| return | |
| } | |
| log.Error(err, "Request failed, retrying...") | |
| continue | |
| } |
🤖 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 `@availability-prober/availability_prober.go` around lines 120 - 127, The
infinite retry loop around creating and sending the request (the for ; ;
time.Sleep(sleepTime) loop that calls http.NewRequestWithContext(ctx, ...) and
client.Do(req)) doesn't stop when ctx is canceled; update the loop to observe
ctx.Done() and exit cleanly: before sleeping or retrying check if ctx.Err() !=
nil (or select on ctx.Done()) and break/return when canceled, and also after a
failed client.Do(req) check ctx.Err() and stop rather than continuing; ensure
this change touches the loop that constructs req and calls client.Do so the
goroutine can exit when the provided context is canceled.
21fb28a to
3726f85
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
availability-prober/availability_prober.go (1)
120-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit context cancellation check to exit the loop.
The infinite retry loop now uses a context-aware HTTP request but never checks if the context is canceled. When
ctxis done,client.Do(req)will fail but the loop continues retrying forever, leaking the goroutine.🔄 Proposed fix
for ; ; time.Sleep(sleepTime) { + select { + case <-ctx.Done(): + log.Info("probe canceled, exiting", "reason", ctx.Err()) + return + default: + } req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) if err != nil { log.Error(err, "Failed to create request, retrying...") continue } response, err := client.Do(req) if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + log.Info("probe canceled, exiting", "reason", err) + return + } log.Error(err, "Request failed, retrying...") continue }As per coding guidelines "Do not leak goroutines — ensure they exit cleanly".
🤖 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 `@availability-prober/availability_prober.go` around lines 120 - 129, The retry loop in availability_prober.go (the for ; ; time.Sleep(sleepTime) loop) must explicitly check ctx cancellation so the goroutine can exit instead of retrying forever; update the loop in the probe function to select on ctx.Done() (or check ctx.Err()) before/inside each iteration and return/break when the context is canceled, and also after client.Do(req) errors verify ctx cancellation before continuing to retry so the function exits cleanly on cancellation.cmd/infra/aws/route53.go (1)
189-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify error type before suppressing; only ignore "zone not found".
Line 191 returns
nilfor everyLookupZonefailure. This silently skips real Route53 or API errors during cleanup. Only "zone not found" should be non-fatal.🔍 Proposed fix
id, err := LookupZone(ctx, client, name, false) if err != nil { - return nil //nolint:nilerr // zone not found during cleanup is non-fatal + if strings.Contains(err.Error(), "not found") { + return nil //nolint:nilerr // zone not found during cleanup is non-fatal + } + return fmt.Errorf("failed to lookup public hosted zone %s: %w", name, err) }As per coding guidelines "Always check errors — don't ignore them".
🤖 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 `@cmd/infra/aws/route53.go` around lines 189 - 191, The current cleanup ignores every LookupZone error by returning nil; change this to only suppress the specific "zone not found" case and return other errors. After calling LookupZone(ctx, client, name, false) check the error with the appropriate predicate (e.g., errors.Is(err, ErrZoneNotFound) or the SDK-specific sentinel / error code check used by LookupZone), only treat that case as non-fatal and continue, otherwise return the actual err instead of nil and remove the blanket //nolint:nilerr suppression; reference LookupZone and the surrounding cleanup logic to implement this conditional handling.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go (1)
894-894: 💤 Low valueIncorrect pattern: wrapping error in assertion failure message.
The second argument to
BeNil()should be a descriptive string, not a wrapped error. Iferris nil, the message is unused. Iferris not nil, you're creating a new error wrapping the original, which doesn't help test readability. Use a plain string or just pass the error directly.✨ Suggested fix
- g.Expect(err).To(BeNil(), fmt.Errorf("error setting custom kubeconfig status failed: %w", err)) + g.Expect(err).To(BeNil(), "error setting custom kubeconfig status failed")🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go` at line 894, The test currently wraps the returned error into fmt.Errorf and passes it as the message to g.Expect(err).To(BeNil(...)), which is incorrect; replace the wrapped-error message with a plain descriptive string or assert the error directly (e.g., use g.Expect(err).ToNot(HaveOccurred()) or g.Expect(err).To(BeNil(), "error setting custom kubeconfig status failed")) and remove the fmt.Errorf(...) wrapping around err so the assertion message is a simple string and the original err is used only as the value under test.
🤖 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 `@hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go`:
- Around line 105-108: The current r.Get(ctx,
client.ObjectKeyFromObject(telemeterClientSecret), telemeterClientSecret) call
treats all errors as a missing secret; change the logic in the Reconcile (or the
method containing this r.Get) to distinguish a NotFound error from other errors:
if apierrors.IsNotFound(err) then log the disabled-telemetry message and return
ctrl.Result{}, nil, but for any other err return ctrl.Result{}, err (or wrap and
return the error) so real failures are surfaced; update the branch that
references telemeterClientSecret and r.Get to perform this explicit error check.
---
Duplicate comments:
In `@availability-prober/availability_prober.go`:
- Around line 120-129: The retry loop in availability_prober.go (the for ; ;
time.Sleep(sleepTime) loop) must explicitly check ctx cancellation so the
goroutine can exit instead of retrying forever; update the loop in the probe
function to select on ctx.Done() (or check ctx.Err()) before/inside each
iteration and return/break when the context is canceled, and also after
client.Do(req) errors verify ctx cancellation before continuing to retry so the
function exits cleanly on cancellation.
In `@cmd/infra/aws/route53.go`:
- Around line 189-191: The current cleanup ignores every LookupZone error by
returning nil; change this to only suppress the specific "zone not found" case
and return other errors. After calling LookupZone(ctx, client, name, false)
check the error with the appropriate predicate (e.g., errors.Is(err,
ErrZoneNotFound) or the SDK-specific sentinel / error code check used by
LookupZone), only treat that case as non-fatal and continue, otherwise return
the actual err instead of nil and remove the blanket //nolint:nilerr
suppression; reference LookupZone and the surrounding cleanup logic to implement
this conditional handling.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go`:
- Line 894: The test currently wraps the returned error into fmt.Errorf and
passes it as the message to g.Expect(err).To(BeNil(...)), which is incorrect;
replace the wrapped-error message with a plain descriptive string or assert the
error directly (e.g., use g.Expect(err).ToNot(HaveOccurred()) or
g.Expect(err).To(BeNil(), "error setting custom kubeconfig status failed")) and
remove the fmt.Errorf(...) wrapping around err so the assertion message is a
simple string and the original err is used only as the value under test.
🪄 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: af9d2add-8cea-4043-8b6d-f6eecd27403c
📒 Files selected for processing (101)
.golangci.ymlavailability-prober/availability_prober.gocmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/azure/create_iam.gocmd/infra/azure/rbac.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/endpoint-resolver/server_test.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-operator/metrics-proxy/proxy_test.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokonnectivity-socks5-proxy/http_proxy.gokubernetes-default-proxy/kubernetes_default_proxy.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosharedingress-config-generator/controller.gosharedingress-config-generator/controller_test.gosharedingress-config-generator/haproxy_client.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/konnectivityproxy/proxy_dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/dump/journals.gotest/e2e/util/external_oidc.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.gotest/integration/framework/hosted-cluster.go
✅ Files skipped from review due to trivial changes (12)
- support/konnectivityproxy/proxy_dialer.go
- konnectivity-socks5-proxy/http_proxy.go
- control-plane-operator/hostedclusterconfigoperator/operator/config.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
- control-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.go
- control-plane-pki-operator/topology/detector.go
- test/e2e/util/reqserving/vpa.go
- test/e2e/util/node.go
- hypershift-operator/controllers/platform/aws/controller.go
- test/e2e/util/reqserving/verifycp.go
- test/e2e/util/reqserving/waitfor.go
- cmd/nodepool/core/create.go
4 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
1004-1012:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the HTTP response body in
healthCheckKASEndpoint
httpClient.Do(req)returns aresp.Bodythat must be closed; otherwise repeated health checks can leak resources.Suggested fix
resp, err := httpClient.Do(req) if err != nil { return err } +defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("APIServer endpoint %s is not healthy", ingressPoint) } return nil🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 1004 - 1012, The healthCheckKASEndpoint function currently returns after httpClient.Do(req) without closing resp.Body, leaking resources on repeated checks; after ensuring err is nil and resp is non-nil, immediately schedule resp.Body.Close() (e.g., defer resp.Body.Close()) so the body is always closed before returning, and keep the existing status check that returns an error when resp.StatusCode != http.StatusOK; reference resp from the httpClient.Do(req) call and the resp.Body to apply the fix.kubernetes-default-proxy/kubernetes_default_proxy.go (1)
70-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the listener on context cancellation to prevent blocked shutdown.
listener.Accept()can block indefinitely afterctxis canceled because the listener is never closed. That can stall graceful termination.Suggested fix
func (s *server) run(ctx context.Context) error { listener, err := (&net.ListenConfig{}).Listen(ctx, "tcp", s.listenAddr) if err != nil { return fmt.Errorf("failed to listen on tcp:%s: %w", s.listenAddr, err) } + defer listener.Close() + go func() { + <-ctx.Done() + _ = listener.Close() + }() s.log.Info("Starting to listen", "listen-address", s.listenAddr, "version", supportedversion.String()) for { if ctx.Err() != nil { return ctx.Err() } conn, err := listener.Accept() if err != nil { + if errors.Is(err, net.ErrClosed) && ctx.Err() != nil { + return ctx.Err() + } s.log.Error(err, "accepting connection failed") continue }🤖 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 `@kubernetes-default-proxy/kubernetes_default_proxy.go` around lines 70 - 85, The accept loop can block after ctx cancellation because the TCP listener is never closed; after creating listener (via net.ListenConfig{}.Listen using s.listenAddr) spawn a goroutine that waits for ctx.Done() and calls listener.Close(), and update the Accept error handling in the loop (e.g., treat net.ErrClosed as shutdown and return ctx.Err() or break) so the server stops promptly and no goroutine remains blocked; reference listener, net.ListenConfig{}.Listen, s.listenAddr, listener.Accept and ctx in your changes.
♻️ Duplicate comments (2)
cmd/infra/aws/route53.go (1)
189-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly ignore the explicit “zone not found” case in cleanup.
This currently suppresses all
LookupZoneerrors, which can hide real Route53/API failures.Suggested fix
id, err := LookupZone(ctx, client, name, false) if err != nil { - return nil //nolint:nilerr // zone not found during cleanup is non-fatal + if strings.Contains(err.Error(), "not found") { + return nil //nolint:nilerr // zone not found during cleanup is non-fatal + } + return fmt.Errorf("failed to lookup public hosted zone %s: %w", name, err) }As per coding guidelines "Always check errors — don't ignore them".
availability-prober/availability_prober.go (1)
121-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExit the probe loop when
ctxis canceled.This retry loop still continues indefinitely on context cancellation;
client.Do(req)can keep returning cancellation errors and the loop never returns.Suggested fix
for ; ; time.Sleep(sleepTime) { + select { + case <-ctx.Done(): + log.Info("probe canceled, exiting", "reason", ctx.Err()) + return + default: + } req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) if err != nil { log.Error(err, "Failed to create request, retrying...") continue } response, err := client.Do(req) if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + log.Info("probe canceled, exiting", "reason", err) + return + } log.Error(err, "Request failed, retrying...") continue }As per coding guidelines "Do not leak goroutines — ensure they exit cleanly".
🤖 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 `@availability-prober/availability_prober.go` around lines 121 - 129, The retry loop around http.NewRequestWithContext and client.Do must exit when the provided ctx is canceled; detect ctx.Done() (or check ctx.Err()) inside the loop and return instead of continuing retries so the goroutine doesn't leak. Update the loop that builds the request and calls client.Do(req) to break/return immediately when ctx is canceled (e.g., before retrying after an error from http.NewRequestWithContext or client.Do), ensuring the probe function exits cleanly on context cancellation.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.go`:
- Around line 151-152: Replace the unbounded context creation used for running
the test script with a timeout context to prevent hangs: when constructing ctx
for exec.CommandContext(ctx, "bash", "-c", testScript) use context.WithTimeout
(e.g., a short, test-appropriate duration) and ensure you call the returned
cancel function (defer cancel()) so the command is killed if it exceeds the
timeout; update the code around the ctx variable and exec.CommandContext
invocation accordingly.
In `@test/e2e/util/util.go`:
- Around line 445-451: WaitForGuestKubeconfigHostResolutionUpdate currently
indexes ipAddrs[0] after LookupIPAddr which can panic if ipAddrs is empty; add a
guard checking len(ipAddrs) > 0 (e.g., if len(ipAddrs) == 0 { t.Logf("no IP
addresses returned for host %s", host); return false, nil }) before accessing
ipAddrs[0] so the function handles empty DNS results safely and
returns/continues without panicking.
---
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 1004-1012: The healthCheckKASEndpoint function currently returns
after httpClient.Do(req) without closing resp.Body, leaking resources on
repeated checks; after ensuring err is nil and resp is non-nil, immediately
schedule resp.Body.Close() (e.g., defer resp.Body.Close()) so the body is always
closed before returning, and keep the existing status check that returns an
error when resp.StatusCode != http.StatusOK; reference resp from the
httpClient.Do(req) call and the resp.Body to apply the fix.
In `@kubernetes-default-proxy/kubernetes_default_proxy.go`:
- Around line 70-85: The accept loop can block after ctx cancellation because
the TCP listener is never closed; after creating listener (via
net.ListenConfig{}.Listen using s.listenAddr) spawn a goroutine that waits for
ctx.Done() and calls listener.Close(), and update the Accept error handling in
the loop (e.g., treat net.ErrClosed as shutdown and return ctx.Err() or break)
so the server stops promptly and no goroutine remains blocked; reference
listener, net.ListenConfig{}.Listen, s.listenAddr, listener.Accept and ctx in
your changes.
---
Duplicate comments:
In `@availability-prober/availability_prober.go`:
- Around line 121-129: The retry loop around http.NewRequestWithContext and
client.Do must exit when the provided ctx is canceled; detect ctx.Done() (or
check ctx.Err()) inside the loop and return instead of continuing retries so the
goroutine doesn't leak. Update the loop that builds the request and calls
client.Do(req) to break/return immediately when ctx is canceled (e.g., before
retrying after an error from http.NewRequestWithContext or client.Do), ensuring
the probe function exits cleanly on context cancellation.
🪄 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: adfa8bc8-944f-4afd-bc85-065370a008ea
📒 Files selected for processing (111)
.golangci.ymlavailability-prober/availability_prober.gocmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/cluster/powervs/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/azure/create_iam.gocmd/infra/azure/rbac.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/endpoint-resolver/server_test.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/reconcile.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-operator/metrics-proxy/proxy_test.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/cache.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokonnectivity-socks5-proxy/http_proxy.gokubernetes-default-proxy/kubernetes_default_proxy.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosharedingress-config-generator/controller.gosharedingress-config-generator/controller_test.gosharedingress-config-generator/haproxy_client.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/controlplane-component/token-minter-container.gosupport/gcpapi/gcs_client.gosupport/globalconfig/proxy.gosupport/konnectivityproxy/dialer.gosupport/konnectivityproxy/proxy_dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/dockerv1client/types.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gosync-fg-configmap/update.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/dump/journals.gotest/e2e/util/external_oidc.gotest/e2e/util/fixture.gotest/e2e/util/generate.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/sharedoidc.gotest/e2e/util/util.gotest/e2e/util/version.gotest/integration/framework/hosted-cluster.go
✅ Files skipped from review due to trivial changes (24)
- cmd/bastion/aws/create.go
- cmd/cluster/powervs/create.go
- support/controlplane-component/token-minter-container.go
- support/thirdparty/library-go/pkg/image/dockerv1client/types.go
- test/e2e/util/reqserving/vpa.go
- .golangci.yml
- ignition-server/controllers/cache.go
- test/e2e/util/generate.go
- karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go
- hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go
- support/util/util.go
- test/e2e/util/fixture.go
- support/globalconfig/proxy.go
- control-plane-pki-operator/topology/detector.go
- support/releaseinfo/releaseinfo.go
- cmd/nodepool/core/create.go
- test/e2e/util/node.go
- hypershift-operator/controllers/nodepool/conditions.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go
- test/e2e/util/aws.go
- konnectivity-socks5-proxy/http_proxy.go
- cmd/infra/aws/iam.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.go
Enables dupword, durationcheck, errorlint, fatcontext, nilerr, noctx, and usestdlibvars linters in golangci-lint configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
bryan-cox
changed the title
|
@bryan-cox: This pull request references CNTRLPLANE-3510 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests verifying wrapped error detection via errors.As for GCP isNotFound/isNotFoundError functions, semver validation in parseComponentVersionsLabel, URL port defaulting in joinDefaultPortIfMissing, error retryability classification in IsErrorRetryable, and nilerr fix error paths in CleanupPublicZone, nodepool create, and UWM telemetry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
@bryan-cox: 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. |
|
The background task completed but wasn't needed — the analysis is already complete above. The report has been fully generated with all the evidence gathered from the build log, JUnit XML, and PR diff analysis. |
4 tasks
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
2 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.
Summary
Enables five new golangci-lint linters and fixes all violations across the codebase:
errors.As/errors.Isinstead of type assertions/direct comparisons on errors, enabling proper wrapped-error detectionerr != nilthen silently returnnil, swallowing errors. Intentional cases (e.g., not-found during cleanup) are suppressed with//nolint:nilerrcommentshttp.NewRequestWithContextinstead ofhttp.NewRequest/http.Get, ensuring HTTP requests respect context cancellationhttp.StatusOK,http.MethodGet) instead of magic literalsAlso adds unit tests covering the behavioral changes introduced by the linter fixes.
Commits
.golangci.ymlconfigurationerrors.As/errors.Ismigration across ~30 files//nolint:nilerrfor legitimate error-swallowing patternshttp.NewRequestWithContextmigrationTest plan
make lintpasses with all new linters enabledmake testpasses (unit tests)errorlint(errors.Aswrapped error detection) andnoctxfixes🤖 Generated with Claude Code