CNTRLPLANE-1551: feat(nodepool): add HAProxy image override via annotation #7187
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
area/documentation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdded a NodePool-scoped HAProxy image override constant and implemented precedence-based HAProxy image resolution (NodePool annotation → shared ingress override → release payload), updating function signatures, call sites, docs, and tests accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
hypershift-operator/controllers/nodepool/nodepool_controller_test.go (1)
1479-1606: Great coverage; add a case for shared-ingress default (no env var) and whitespace annotation
- Add a test where shared ingress is enabled but IMAGE_SHARED_INGRESS_HAPROXY is unset to assert the hardcoded default is used.
- Optional: once trim logic is added (see resolver comment), add a case where annotation is " " and confirm it falls back.
Apply this minimal diff to include the default shared-ingress case:
@@ -import ( +import ( "fmt" "regexp" "strings" "testing" @@ - fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake" + fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake" + "github.com/openshift/hypershift/support/images" @@ testCases := []struct { name string nodePoolAnnotations map[string]string useSharedIngress bool envVarImage string expectedImage string }{ @@ + { + name: "When shared ingress enabled and no env var set it should use hardcoded default", + useSharedIngress: true, + expectedImage: images.GetSharedIngressHAProxyImage(), + }, }docs/content/how-to/automated-machine-management/haproxy-image-override.md (2)
14-20: Document the shared‑ingress default explicitlyConsider adding a note that when shared ingress is enabled and IMAGE_SHARED_INGRESS_HAPROXY is unset, a built‑in default image is used.
51-57: Minor: use plural resource name in CLI examplesMost kubectl verbs prefer plural resources. Example:
kubectl annotate nodepools my-nodepool -n clusters ....hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
979-999: Edge case: missing haproxy component in release imageError message is clear. Consider wrapping with the current release version for faster triage, e.g., include
releaseImage.Version()in the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
api/hypershift/v1beta1/hostedcluster_types.go(1 hunks)docs/content/how-to/automated-machine-management/haproxy-image-override.md(1 hunks)hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go(0 hunks)hypershift-operator/controllers/nodepool/conditions.go(1 hunks)hypershift-operator/controllers/nodepool/nodepool_controller.go(4 hunks)hypershift-operator/controllers/nodepool/nodepool_controller_test.go(2 hunks)hypershift-operator/controllers/nodepool/secret_janitor.go(1 hunks)
💤 Files with no reviewable changes (1)
- hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
🔇 Additional comments (8)
hypershift-operator/controllers/nodepool/secret_janitor.go (1)
96-99: LGTM on updated call signaturePasses nodePool into generateHAProxyRawConfig; flow and errors preserved.
hypershift-operator/controllers/nodepool/nodepool_controller_test.go (1)
13-13: Import alias for haproxy constants is appropriateKeeps tests decoupled from component image name string.
api/hypershift/v1beta1/hostedcluster_types.go (1)
53-57: Annotation constant addition is clear and accurateName and docs match resolver precedence; scoped to NodePool usage.
hypershift-operator/controllers/nodepool/nodepool_controller.go (4)
16-20: New imports are appropriate for shared‑ingress image resolutionNo impact on existing behavior outside HAProxy image selection.
341-344: Call-site update to pass NodePool is correctConsistent with new generator signature; error handling unchanged.
418-421: Token path updated consistentlygenerateHAProxyRawConfig now receives nodePool; good.
1003-1017: Generator uses resolved image correctlyEncapsulates image choice; keeps haproxy generation unchanged.
hypershift-operator/controllers/nodepool/conditions.go (1)
328-331: Plumbing NodePool into HAProxy config generation verifiedAll 4 call sites (secret_janitor.go:96, conditions.go:328, nodepool_controller.go:341 and 418) correctly pass nodePool. Function signature and all invocations are consistent.
muraee
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@muraee: This pull request references CNTRLPLANE-1551 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 "4.21.0" version, but no target version was set. In 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. |
muraee
force-pushed
the
CNTRLPLANE-1551-haproxy-image-override
branch
from
96f2de9 to
900158f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/content/how-to/automated-machine-management/haproxy-image-override.md (3)
75-94: Consider adding verification for the token secret content.The verification steps are helpful. Consider adding a step to check the NodePool's token secret to verify the HAProxy image is in the ignition configuration before checking the worker node itself.
Example addition after line 79:
2. Check the token secret for the HAProxy image reference: ```bash kubectl get secret <nodepool-name>-token -n <controlplane-namespace> -o jsonpath='{.data.payload}' | base64 -d | grep haproxy--- `96-103`: **Remove extra blank line.** The important notes are comprehensive and accurate. However, there's an unnecessary extra blank line at line 103. Apply this diff: ```diff 5. **Multiple NodePools**: Each NodePool can have a different HAProxy image override - ## Troubleshooting
104-126: Consider adding example commands for troubleshooting.The troubleshooting guidance is helpful. Consider enhancing it with specific commands to inspect NodePool conditions or retrieve ignition configuration from the token secret.
Example addition after line 118:
```bash # Check NodePool status conditions kubectl get nodepool <nodepool-name> -n clusters -o jsonpath='{.status.conditions}' | jq # Inspect token secret ignition config kubectl get secret <nodepool-name>-token -n <controlplane-namespace> -o jsonpath='{.data.payload}' | base64 -d | jq '.storage.files[] | select(.path | contains("kube-apiserver-proxy"))'</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 96f2de914754055a90bbf5691b6825d345efd02f and 900158fa135552eebb1881a2a327820992f27065. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.go` is excluded by `!vendor/**`, `!**/vendor/**` </details> <details> <summary>📒 Files selected for processing (7)</summary> * `api/hypershift/v1beta1/hostedcluster_types.go` (1 hunks) * `docs/content/how-to/automated-machine-management/haproxy-image-override.md` (1 hunks) * `hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go` (0 hunks) * `hypershift-operator/controllers/nodepool/conditions.go` (1 hunks) * `hypershift-operator/controllers/nodepool/nodepool_controller.go` (4 hunks) * `hypershift-operator/controllers/nodepool/nodepool_controller_test.go` (2 hunks) * `hypershift-operator/controllers/nodepool/secret_janitor.go` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (4)</summary> * api/hypershift/v1beta1/hostedcluster_types.go * hypershift-operator/controllers/nodepool/secret_janitor.go * hypershift-operator/controllers/nodepool/conditions.go * hypershift-operator/controllers/nodepool/nodepool_controller.go </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>hypershift-operator/controllers/nodepool/nodepool_controller_test.go (2)</summary><blockquote> `13-13`: **LGTM: Import addition is appropriate.** The haproxy package import is correctly used in the test to reference `HAProxyRouterImageName`. --- `1479-1614`: **LGTM: Well-structured test with comprehensive coverage.** The test function is well-designed with table-driven tests covering the image resolution priority order. The setup properly mocks all dependencies including the client, release provider, and environment variables. The test cases validate annotation precedence, shared ingress fallback, and release payload fallback scenarios. </blockquote></details> <details> <summary>docs/content/how-to/automated-machine-management/haproxy-image-override.md (2)</summary><blockquote> `1-20`: **LGTM: Clear overview and correct priority order.** The documentation accurately describes the HAProxy image resolution priority, which matches the implementation described in the PR objectives. --- `21-68`: **LGTM: Examples are syntactically correct and practical.** The YAML configuration example and kubectl commands are accurate and provide clear guidance for users to apply and remove the annotation. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
muraee
force-pushed
the
CNTRLPLANE-1551-haproxy-image-override
branch
from
900158f to
b5c903d
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
This change enables per-NodePool customization of the HAProxy image used for worker node API server proxy through the annotation hypershift.openshift.io/haproxy-image. The implementation includes: - New annotation constant HAProxyImageAnnotation in API types - Image resolution function with priority order: 1. NodePool annotation (highest priority) 2. Environment variable when shared ingress enabled 3. Hardcoded default when shared ingress enabled 4. Release payload (default) - Updated all call sites to use the new resolution logic - Comprehensive unit tests validating all priority scenarios - User documentation with examples and troubleshooting The refactored resolveHAProxyImage() function is independently testable, avoiding complex HAProxy config generation setup. Signed-off-by: Mulham Raee <[email protected]> Commit-Message-Assisted-by: Claude (via Claude Code)
muraee
force-pushed
the
CNTRLPLANE-1551-haproxy-image-override
branch
from
b5c903d to
95392ef
Compare
|
@muraee: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Summary
This PR enables per-NodePool customization of the HAProxy image used for worker node API server proxy through the annotation
hypershift.openshift.io/haproxy-image.Problem
Currently, there is no way to override the HAProxy image on a per-NodePool basis. The image is determined globally by:
IMAGE_SHARED_INGRESS_HAPROXYwhen shared ingress is enabledThis limitation prevents users from testing custom HAProxy images or applying image overrides to specific NodePools without affecting the entire cluster.
Solution
Introduced a new NodePool annotation
hypershift.openshift.io/haproxy-imagethat allows users to specify a custom HAProxy image per NodePool. The image resolution follows this priority order:hypershift.openshift.io/haproxy-imageIMAGE_SHARED_INGRESS_HAPROXYwhen shared ingress enabledImplementation Details
API Changes
HAProxyImageAnnotationconstant toapi/hypershift/v1beta1/hostedcluster_types.goCore Logic
resolveHAProxyImage()function innodepool_controller.goto handle image resolution with proper prioritygenerateHAProxyRawConfig()to use the new resolution functionTesting
TestResolveHAProxyImagecovering all priority scenarios:Documentation
docs/content/how-to/automated-machine-management/haproxy-image-override.mdTesting Performed
✅ All unit tests pass
✅ Linting passes (
make lint-fix)✅ Verification passes (
make verify)✅ 6 test scenarios covering all image resolution priority cases
Related Issues
🤖 Generated with Claude Code