ROSAENG-60642: feat(install): add --operator-pprof-addr flag to enable pprof on the HyperShift Operator - #8853
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@Ajpantuso: This pull request references ROSAENG-60642 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 story 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. |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThe install command adds Sequence Diagram(s)sequenceDiagram
participant Installer
participant Deployment
participant Operator
participant Manager
Installer->>Deployment: Pass validated pprof address
Deployment->>Operator: Set --pprof-addr and expose pprof port
Operator->>Manager: Set PprofBindAddress
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
area/cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/install/install.go`:
- Around line 195-208: The validateOperatorPprofAddr() check currently only
verifies format and port range, so it still allows ports already reserved by the
operator. Update this validation to reject the reserved listener ports used by
metrics and the manager/webhook server (including the values accepted by
net.SplitHostPort in this path), and make sure the error returned from
validateOperatorPprofAddr() clearly reports the port is unavailable. Add
regression cases to the existing validation table covering the reserved-port
inputs and a valid non-reserved port.
In `@hypershift-operator/main.go`:
- Line 203: The new --pprof-addr flag is only validated on the installer path,
so the run path can still accept malformed or conflicting bind addresses and
fail later. Add the same upfront validation before calling run() in the main
command flow, using the existing pprof address handling around opts.PprofAddr
and the run() entrypoint so both paths reject bad values consistently.
🪄 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: a8405f28-d4c1-432d-987d-184300c152b1
📒 Files selected for processing (5)
cmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gohypershift-operator/main.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8853 +/- ##
==========================================
+ Coverage 44.96% 44.98% +0.01%
==========================================
Files 778 778
Lines 97452 97496 +44
==========================================
+ Hits 43820 43856 +36
- Misses 50607 50613 +6
- Partials 3025 3027 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
95ce94f to
415d103
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/install/assets/hypershift_operator.go`:
- Around line 775-777: The pprof flag assembly currently allows an address that
conflicts with existing operator listeners, so add a reserved-port validation
for o.PprofAddr that rejects values colliding with metrics/listener ports before
appending "--pprof-addr" in the installer flow. Mirror the same check in the
safety-net parser path that handles pprof address parsing, and make sure any
address-parse errors are returned or handled instead of being ignored.
🪄 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: 2f30fe6d-f76a-4e90-ace6-81d693ba0a73
📒 Files selected for processing (5)
cmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gohypershift-operator/main.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/install/install_test.go
- hypershift-operator/main.go
- cmd/install/assets/hypershift_operator_test.go
| if o.PprofAddr != "" { | ||
| args = append(args, "--pprof-addr="+o.PprofAddr) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject pprof ports that collide with existing operator listeners.
--pprof-addr=:9000 can currently be emitted even though metrics already bind :9000, which would make the operator fail to start. Add this reserved-port check to installer validation and mirror it in this safety-net parser; also don’t discard the parse errors.
Suggested hardening
- if o.PprofAddr != "" {
+ if _, ok := o.pprofContainerPort(); ok {
args = append(args, "--pprof-addr="+o.PprofAddr)
}- _, portStr, _ := net.SplitHostPort(o.PprofAddr)
- port, _ := strconv.Atoi(portStr)
- if port < 1 || port > 65535 {
+ _, portStr, err := net.SplitHostPort(o.PprofAddr)
+ if err != nil {
+ return corev1.ContainerPort{}, false
+ }
+ port, err := strconv.Atoi(portStr)
+ if err != nil || port < 1 || port > 65535 || port == 9000 {
return corev1.ContainerPort{}, false
}As per coding guidelines, **/*.go: “Always check errors — don’t ignore them.” As per path instructions, Go security requires “Never ignore error returns.”
Also applies to: 941-945
🤖 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/install/assets/hypershift_operator.go` around lines 775 - 777, The pprof
flag assembly currently allows an address that conflicts with existing operator
listeners, so add a reserved-port validation for o.PprofAddr that rejects values
colliding with metrics/listener ports before appending "--pprof-addr" in the
installer flow. Mirror the same check in the safety-net parser path that handles
pprof address parsing, and make sure any address-parse errors are returned or
handled instead of being ignored.
Sources: Coding guidelines, Path instructions
415d103 to
ccbf829
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
needs-rebase
ccbf829 to
84185e0
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required |
84185e0 to
ebd4865
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ajpantuso 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/install/assets/hypershift_operator.go (1)
976-977: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse named constants for the port bounds.
Replace
1and65535with named constants so the validation rules are self-documenting and consistent across the installer and safety-net parser. As per coding guidelines, Go code should avoid magic numbers.🤖 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/install/assets/hypershift_operator.go` around lines 976 - 977, Update the port validation in the strconv.Atoi parsing flow to replace the literal bounds 1 and 65535 with named constants. Define or reuse shared constants representing the minimum and maximum valid port values so the installer and safety-net parser use the same validation rules.Source: Coding guidelines
🤖 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/install/assets/hypershift_operator.go`:
- Around line 965-985: Update the pprof address validation used by buildArgs and
HyperShiftOperatorDeployment.pprofContainerPort to reject port 9000, matching
the fixed metrics listener configured by --metrics-addr=:9000. Preserve existing
empty, parse-error, and out-of-range rejection behavior while ensuring the
helper also returns false for the reserved metrics port.
---
Nitpick comments:
In `@cmd/install/assets/hypershift_operator.go`:
- Around line 976-977: Update the port validation in the strconv.Atoi parsing
flow to replace the literal bounds 1 and 65535 with named constants. Define or
reuse shared constants representing the minimum and maximum valid port values so
the installer and safety-net parser use the same validation rules.
🪄 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: a1b12ea6-45bb-457d-9a9f-f5a860b5b879
📒 Files selected for processing (5)
cmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gohypershift-operator/main.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/install/assets/hypershift_operator_test.go
- hypershift-operator/main.go
- cmd/install/install_test.go
- cmd/install/install.go
| // pprofContainerPort parses o.PprofAddr and returns the corresponding | ||
| // ContainerPort when the address is non-empty and valid. The installer | ||
| // validates the address before Build is called, so errors here are a safety net. | ||
| func (o HyperShiftOperatorDeployment) pprofContainerPort() (corev1.ContainerPort, bool) { | ||
| if o.PprofAddr == "" { | ||
| return corev1.ContainerPort{}, false | ||
| } | ||
| _, portStr, err := net.SplitHostPort(o.PprofAddr) | ||
| if err != nil { | ||
| return corev1.ContainerPort{}, false | ||
| } | ||
| port, err := strconv.Atoi(portStr) | ||
| if err != nil || port < 1 || port > 65535 { | ||
| return corev1.ContainerPort{}, false | ||
| } | ||
| return corev1.ContainerPort{ | ||
| Name: "pprof", | ||
| ContainerPort: int32(port), | ||
| Protocol: corev1.ProtocolTCP, | ||
| }, true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Still reject pprof addresses that collide with the metrics listener.
buildArgs() always passes --metrics-addr=:9000 at Line [794], but this helper accepts :9000; the operator can then fail when both listeners bind the same port. Reject the reserved metrics port in both installer validation and this safety-net helper. The prior parse-error concern is fixed, but this collision remains unresolved.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 981-981: 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(port)
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 `@cmd/install/assets/hypershift_operator.go` around lines 965 - 985, Update the
pprof address validation used by buildArgs and
HyperShiftOperatorDeployment.pprofContainerPort to reject port 9000, matching
the fixed metrics listener configured by --metrics-addr=:9000. Preserve existing
empty, parse-error, and out-of-range rejection behavior while ensuring the
helper also returns false for the reserved metrics port.
|
/lgtm |
|
✅ Action performedReviews resumed. |
|
Scheduling tests matching the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/install/assets/hypershift_operator.go`:
- Around line 729-733: Update the shared pprof port validation used by
pprofContainerPort() to reject port 9000, matching the installer validator and
the fixed --metrics-addr=:9000 configuration in buildArgs(). Ensure invalid
:9000 input does not add a container port or propagate pprof arguments, while
preserving existing behavior for valid pprof ports.
🪄 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: 22eb46d1-f0e6-4f0e-b1ca-e69933b69bac
📒 Files selected for processing (5)
cmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gohypershift-operator/main.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hypershift-operator/main.go
- cmd/install/install_test.go
- cmd/install/install.go
- cmd/install/assets/hypershift_operator_test.go
| if port, ok := o.pprofContainerPort(); ok { | ||
| deployment.Spec.Template.Spec.Containers[0].Ports = append( | ||
| deployment.Spec.Template.Spec.Containers[0].Ports, port, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Reject pprof port 9000 in the shared validation helper.
buildArgs() always passes --metrics-addr=:9000 at Line [794], but pprofContainerPort() accepts :9000; both deployment generation and argument propagation then configure conflicting listeners, causing operator startup failure when upstream validation is bypassed. Reject the reserved metrics port here and keep the installer validator aligned.
Suggested fix
- if err != nil || port < 1 || port > 65535 {
+ if err != nil || port < 1 || port > 65535 || port == 9000 {Also applies to: 806-808, 965-986
🤖 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/install/assets/hypershift_operator.go` around lines 729 - 733, Update the
shared pprof port validation used by pprofContainerPort() to reject port 9000,
matching the installer validator and the fixed --metrics-addr=:9000
configuration in buildArgs(). Ensure invalid :9000 input does not add a
container port or propagate pprof arguments, while preserving existing behavior
for valid pprof ports.
|
/retest-required |
|
All 4 background agents have now completed. Their findings are fully consistent with the report I already delivered above. The e2e-aks agent confirmed the AKS failures were caused by management cluster resource exhaustion (pods stuck |
|
/retest-required |
1 similar comment
|
/retest-required |
…HyperShift Operator Add an installer option to conditionally configure the Go pprof profiling server on the HyperShift Operator. When set, the installer passes --pprof-addr to the operator binary and adds the corresponding container port to the Deployment. The pprof endpoint is not exposed via a Service. The installer validates the address format and port range before generating manifests so that invalid values fail early rather than after deployment. Ref: ROSAENG-60642 Signed-off-by: Andrew Pantuso <apantuso@redhat.com>
ebd4865 to
14339aa
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/install/assets/hypershift_operator.go (1)
729-733: 🩺 Stability & Availability | 🟠 MajorReject pprof addresses that reuse operator listener ports.
buildArgs()always passes--metrics-addr=:9000, butpprofContainerPort()accepts:9000.Build()then adds approfport and passes a second listener on port 9000. The operator can fail withaddress already in use.Reject port
9000in the shared helper. Add a regression test for:9000that expects no pprof argument and no pprof container port. Use named constants for the metrics port and valid port bounds. Also verify whether the existingmanagerport9443is bound by the webhook server. If it is, reject that port when the listener is enabled.#!/bin/bash set -euo pipefail rg -n -C 6 \ 'PprofBindAddress|WebhookServer|HealthProbeBindAddress|metrics-addr|health-probe|9443|9000' \ --glob '*.go' \ --glob '*.yaml' \ --glob '*.yml' \ .As per coding guidelines, use named constants instead of magic numbers and run
make lint-fixafter the Go change.Also applies to: 806-808, 965-986
🤖 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/install/assets/hypershift_operator.go` around lines 729 - 733, The shared pprof address validation used by pprofContainerPort() must reject the metrics port 9000, and reject 9443 when the webhook listener binds it. Define named constants for the metrics port and valid port bounds, update validation accordingly, and add regression coverage ensuring :9000 produces neither a pprof argument nor container port; verify the webhook binding before applying the 9443 restriction, then run make lint-fix.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@cmd/install/assets/hypershift_operator.go`:
- Around line 729-733: The shared pprof address validation used by
pprofContainerPort() must reject the metrics port 9000, and reject 9443 when the
webhook listener binds it. Define named constants for the metrics port and valid
port bounds, update validation accordingly, and add regression coverage ensuring
:9000 produces neither a pprof argument nor container port; verify the webhook
binding before applying the 9443 restriction, then run make lint-fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 51c80537-e445-40d2-bc49-f72f14273bc1
📒 Files selected for processing (4)
cmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/install/assets/hypershift_operator_test.go
- cmd/install/install_test.go
- cmd/install/install.go
|
/lgtm |
|
Scheduling tests matching the |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
|
PR needs rebase. 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. |
|
@Ajpantuso: The following tests 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. |
3 participants
What this PR does / why we need it:
Add an installer option to conditionally enable the Go pprof profiling server on the HyperShift Operator. The HyperShift Operator uses controller-runtime, which has built-in pprof support via `manager.Options.PprofBindAddress`, but this field was never set. There was no way to enable pprof without modifying code and redeploying.
When `--operator-pprof-addr` is provided (e.g. `:6060`), the installer passes `--pprof-addr` to the operator binary and adds the corresponding container port to the Deployment. The pprof endpoint is intentionally not exposed via a Service.
Which issue(s) this PR fixes:
Fixes ROSAENG-60642
Special notes for your reviewer:
The installer validates the address format and port range (1–65535) before generating manifests so that invalid values fail early rather than after deployment.
Checklist:
Summary by CodeRabbit
host:portaddress.1–65535), and reserved ports (9000,9443).