Skip to content

fix(cli): add --service-publishing-strategy flag to hcp create cluster agent#8985

Open
ketanvyas21 wants to merge 1 commit into
openshift:mainfrom
ketanvyas21:fix/agent-loadbalancer-publishing-strategy
Open

fix(cli): add --service-publishing-strategy flag to hcp create cluster agent#8985
ketanvyas21 wants to merge 1 commit into
openshift:mainfrom
ketanvyas21:fix/agent-loadbalancer-publishing-strategy

Conversation

@ketanvyas21

@ketanvyas21 ketanvyas21 commented Jul 10, 2026

Copy link
Copy Markdown

Summary

  • Adds a --service-publishing-strategy flag to hcp create cluster agent supporting NodePort (default, backward-compatible) and LoadBalancer options
  • When LoadBalancer is selected, APIServer uses LoadBalancer and OAuthServer/Konnectivity/Ignition use Route, matching the documented preferred method for bare metal
  • Adds validation: rejects unsupported strategies, rejects --api-server-address with LoadBalancer strategy, auto-detects address only for NodePort

Fixes OCPBUGS-97942

Test plan

  • New TestRawCreateOptions_Validate with 4 cases covering validation logic
  • New TestCreateCluster case for LoadBalancer strategy rendering with fixture
  • Existing NodePort test continues to pass unchanged
  • make lint-fix passes with 0 issues
  • CI e2e tests

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Added support for choosing how cluster services are published: NodePort or LoadBalancer.
    • Added the --service-publishing-strategy command-line option.
    • LoadBalancer publishing now supports optional external DNS configuration.
    • API server address configuration is validated according to the selected publishing strategy.
  • Bug Fixes

    • Improved conditional handling of API server address discovery and configuration.

…r agent

The hcp create cluster agent command unconditionally configured all
services with NodePort publishing, despite LoadBalancer being the
documented preferred method for bare metal environments. Users wanting
LoadBalancer had to render YAML and manually edit the manifest.

Add a --service-publishing-strategy flag (NodePort|LoadBalancer) that
defaults to NodePort for backward compatibility. When LoadBalancer is
selected, APIServer uses LoadBalancer and other services use Route,
matching the pattern used by other platforms.

Signed-off-by: Ketan Vyas <kvyas@redhat.com>
Assisted-by: Claude Opus 4.6 (via Cursor)
Co-authored-by: Cursor <cursoragent@cursor.com>
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci

openshift-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ketanvyas21
Once this PR has been reviewed and has the lgtm label, please assign csrwng for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 376cf77d-fc5d-4cb2-a227-16581a2067aa

📥 Commits

Reviewing files that changed from the base of the PR and between e708f6f and ae0ed9c.

⛔ Files ignored due to path filters (1)
  • cmd/cluster/agent/testdata/zz_fixture_TestCreateCluster_When_service_publishing_strategy_is_LoadBalancer__it_should_render_LoadBalancer_services.yaml is excluded by !**/testdata/**
📒 Files selected for processing (2)
  • cmd/cluster/agent/create.go
  • cmd/cluster/agent/create_test.go

📝 Walkthrough

Walkthrough

Cluster creation now supports NodePort and LoadBalancer service publishing strategies. NodePort remains the default and may use an API server address, while LoadBalancer rejects that option. Completion carries the external DNS domain into internal options, and platform-specific configuration selects the corresponding service mapping. CLI flags and tests cover validation, strategy selection, and LoadBalancer manifest generation.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant RawCreateOptions
  participant ValidatedCreateOptions
  participant CreateOptions
  participant ClusterSpec
  CLI->>RawCreateOptions: Set service publishing strategy
  RawCreateOptions->>ValidatedCreateOptions: Validate strategy and API server address
  ValidatedCreateOptions->>CreateOptions: Complete with external DNS domain
  CreateOptions->>ClusterSpec: Apply NodePort or LoadBalancer service mapping
Loading
🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding the --service-publishing-strategy flag to hcp create cluster agent.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The added subtest names are static literals; no generated IDs, timestamps, IPs, or other run-to-run values appear in test titles.
Test Structure And Quality ✅ Passed The added tests are table-driven unit tests with isolated subtests, temp dirs, explicit failure messages, and no cluster waits or unmanaged resources.
Topology-Aware Scheduling Compatibility ✅ Passed Only service-publishing flags and HostedCluster service mappings changed; no pod affinity, nodeSelector, replica, toleration, or PDB logic was added.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the changed tests are plain unit tests, and the fixture only contains cluster-internal CIDRs.
No-Weak-Crypto ✅ Passed Touched files add service-publishing validation/rendering only; crypto keyword scan in both files found no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or secret comparisons.
Container-Privileges ✅ Passed The PR only changes CLI/service-publishing logic and tests; no privileged, hostNetwork, hostPID/IPC, SYS_ADMIN, or allowPrivilegeEscalation settings were introduced.
No-Sensitive-Data-In-Logs ✅ Passed No new logging of secrets/PII/internal hosts was added; the patch only adds validation/errors and static flag help text, with one existing generic error log unchanged.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added area/cli Indicates the PR includes changes for CLI and removed do-not-merge/needs-area labels Jul 10, 2026
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.07692% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.68%. Comparing base (e708f6f) to head (ae0ed9c).

Files with missing lines Patch % Lines
cmd/cluster/agent/create.go 73.07% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8985   +/-   ##
=======================================
  Coverage   43.67%   43.68%           
=======================================
  Files         771      771           
  Lines       95840    95858   +18     
=======================================
+ Hits        41862    41876   +14     
- Misses      51067    51071    +4     
  Partials     2911     2911           
Files with missing lines Coverage Δ
cmd/cluster/agent/create.go 61.53% <73.07%> (+2.95%) ⬆️
Flag Coverage Δ
cmd-support 37.25% <73.07%> (+0.02%) ⬆️
cpo-hostedcontrolplane 45.91% <ø> (ø)
cpo-other 45.11% <ø> (ø)
hypershift-operator 53.91% <ø> (ø)
other 32.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-ci

openshift-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

@ketanvyas21: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli Indicates the PR includes changes for CLI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant