ROSAENG-62084 | feat: Integrate Hyperfleet Platform API v2 with the ROSA CLI - #3448
ROSAENG-62084 | feat: Integrate Hyperfleet Platform API v2 with the ROSA CLI#3448gdbranco wants to merge 9 commits into
Conversation
…ntime Adds Platform API v2 (hyperfleet) client support as the foundation for routing HCP cluster and nodepool commands to the v2 SDK instead of OCM. - pkg/hyperfleet: new package with --hyperfleet-url flag wiring, AWS region extraction from URL (standard + GovCloud regex), WarnOnMismatch helper, and IAM role/instance-profile helpers - pkg/rosa: Runtime.WithHyperFleet() builds a hyperfleet Clientset via AWS SigV4 (awsconfig + STS GetCallerIdentity, no OCM login required); RuntimeWithHyperFleet() visitor wires it into DefaultRunner - cmd/rosa/main.go: registers --hyperfleet-url as a persistent root flag - test/reporter: shared FakeLogger test double (callback-based) replaces ad-hoc fakeReporter/mockReporter in pkg/output and cmd/verify/oc - vendor: adds rosa-hyperfleet-api/clientset, hypershift/api, and related transitive dependencies
…st/edit/delete cluster Routes rosa create/describe/list/edit/delete cluster through the Platform API (hyperfleet) when --hyperfleet-url is supplied, bypassing the OCM path entirely. Includes mappers from v1alpha1.Cluster to the existing rosa describe/list output shapes, expiration support in edit, and an e2e sanity test that exercises the full cluster lifecycle via CLI subprocesses. Each CLI call in the e2e test now explicitly supplies --hyperfleet-url so subprocesses enter hyperfleet mode without relying on env-var magic.
Wire --hyperfleet-url through rosa login and rosa whoami so that the Platform API URL is persisted to config on login and displayed in whoami output. Support hyperfleet-only login (no OCM token required) by short-circuiting before the cfg.Armed() check so that an existing config with expired tokens does not block the flow. Seed the persisted URL back into the hyperfleet runtime via PersistentPreRun in main.go so subsequent commands route correctly without repeating the flag. Add unit tests covering: SetURL/Reset in pkg/hyperfleet, hfClusterToMap in describe/cluster, hyperfleet-only login path, whoami hyperfleet-only output, and config error paths (invalid JSON, invalid JWT, missing/ non-string claims). Fix pre-existing ST1005 lint violations in pkg/config/config.go (capitalized error strings).
…fleet fixes - Add subnet and auto-derived IAM instance profile to node pool creation - Show subnet in node pool describe/list output - Add WithAWSOnly() runtime method; fix rosa whoami for hyperfleet-only mode - Add InstanceProfileFromRolesRef and ComputeInstanceProfile helpers - Fix e2e cleanup ordering: hoist IAM/OIDC DeferCleanup before cluster delete so they run after cluster is fully gone (LIFO); add ENI drain and EC2 instance termination wait before VPC resource teardown
- Hoist IAM/OIDC DeferCleanup registrations before cluster delete so roles and OIDC provider are cleaned up after cluster is gone - Wait for worker EC2 instances to terminate before releasing VPC resources - Delete orphaned ENIs left by cluster controllers (ingress, CSI, CCM) - Purge non-NS/SOA records from hosted zone before deletion to prevent stale zone pollution across test runs - Add classic ELB (v1) sweep before ALB/NLB sweep to remove all LBs blocking VPC subnet/SG deletion; vendor elasticloadbalancing SDK - Add **/*.test to .gitignore to exclude compiled Go test binaries
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gdbranco 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
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded HyperFleet support across the CLI. The change adds AWS-backed runtime initialization, HyperFleet URL configuration, cluster and node pool create, list, describe, edit, and delete commands, and HyperFleet-only authentication. It also adds resource resolution and IAM helpers, mock and reporter test utilities, extensive unit coverage, dependency updates, coverage exclusions, Make targets, and an end-to-end AWS lifecycle sanity test. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 6 warnings)
✅ Passed checks (7 passed)
✨ 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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
tests/e2e/hyperfleet_sanity_test.go-1189-1190 (1)
1189-1190: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not discard the
json.Marshalerror.
hfBuildTrustPolicydrops the marshal error. If marshalling ever fails, the function returns""andCreateRolefails later with an opaque AWS error. Surface the failure at the source.🔧 Proposed fix
- b, _ := json.Marshal(doc) - return string(b) + b, err := json.Marshal(doc) + Expect(err).NotTo(HaveOccurred(), "marshalling trust policy for provider %s", oidcProvider) + return string(b) }As per path instructions: "Never ignore error returns".
🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` around lines 1189 - 1190, Update hfBuildTrustPolicy to handle the error returned by json.Marshal instead of discarding it. Surface marshalling failures immediately through the function’s existing error-handling or return contract, ensuring CreateRole does not receive an empty policy string after serialization fails.Source: Path instructions
tests/e2e/hyperfleet_sanity_test.go-791-794 (1)
791-794: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
HYPERFLEET_INSTANCE_TYPEin the Makefile target.The test reads
HYPERFLEET_INSTANCE_TYPEhere, but thee2e-hyperfleetdocumentation block inMakefile(Lines 178 to 187) does not list it, and the target does not forward it toginkgo.ginkgo runpasses the parent environment through, so the variable still reaches the test when exported. Add it to the "Optional" list so the supported inputs are discoverable.🔧 Makefile documentation update
# OPERATOR_ROLES_PREFIX — defaults to CLUSTER_NAME +# HYPERFLEET_INSTANCE_TYPE — node pool instance type; defaults to m5.xlarge # AWS_DEFAULT_REGION — fallback when region cannot be derived from HYPERFLEET_URL🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` around lines 791 - 794, Update the e2e-hyperfleet documentation block in the Makefile to list HYPERFLEET_INSTANCE_TYPE under the Optional inputs, matching the environment variable consumed near instanceType. Do not change the test logic or add explicit ginkgo forwarding, since the inherited environment already provides the variable.tests/e2e/hyperfleet_sanity_test.go-1129-1154 (1)
1129-1154: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winWeak Cryptography (CWE-295): Improper Certificate Validation
Reachability: Internal
Set
MinVersion: tls.VersionTLS12and clarify the chain-validation tradeoff.The function dials the OIDC issuer with
InsecureSkipVerify: trueto extract the SHA-1 thumbprint, then registers it in the test AWS account. An on-path attacker could present a substituted chain and register the attacker's root in IAM. AddMinVersion: tls.VersionTLS12and document why validation is skipped and why the residual risk is acceptable.🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` around lines 1129 - 1154, Update hfOIDCThumbprint to set MinVersion to tls.VersionTLS12 in the tls.Config passed to tls.Dial, and add a clear comment explaining that certificate validation is intentionally skipped only to read the issuer’s raw chain for thumbprint registration. Keep the existing InsecureSkipVerify behavior and SHA-1 thumbprint extraction unchanged, but document the tradeoff and why the residual risk is acceptable in this test-only flow.Source: Linters/SAST tools
cmd/list/cluster/hyperfleet.go-48-48 (1)
48-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle the
tabwriter.Flusherror.If stdout fails during a pipe or redirect,
Flushreturns an error. The command currently reports success after partial output. Report the error and exit with a nonzero status.🤖 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/list/cluster/hyperfleet.go` at line 48, Update the output finalization around writer.Flush in the command flow to check and handle its returned error. Report the flush failure and terminate with a nonzero status instead of allowing the command to report success after partial output.Sources: Coding guidelines, Path instructions
cmd/list/machinepool/hyperfleet_run_test.go-83-89 (1)
83-89: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the original
ocmcluster key in each test.Each test overwrites process-global state and restores the literal value
"cluster1". A preceding spec can set a different key. Capture the prior key before mutation and restore that exact value during cleanup.
cmd/list/machinepool/hyperfleet_run_test.go#L83-L89: restore the captured key instead of"cluster1".cmd/dlt/cluster/hyperfleet_run_test.go#L48-L54: restore the captured key instead of"cluster1".cmd/edit/machinepool/hyperfleet_run_test.go#L133-L149: restore the captured key instead of"cluster1".Based on learnings, tests that override globals must restore the original values to prevent cross-test interference.
🤖 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/list/machinepool/hyperfleet_run_test.go` around lines 83 - 89, Capture the existing cluster key before each test mutates it, then restore that captured value during cleanup instead of the hardcoded "cluster1". Apply this in cmd/list/machinepool/hyperfleet_run_test.go#L83-L89, cmd/dlt/cluster/hyperfleet_run_test.go#L48-L54, and cmd/edit/machinepool/hyperfleet_run_test.go#L133-L149, using the relevant test setup and ocm.SetClusterKey calls.Source: Learnings
cmd/edit/cluster/hyperfleet_run_test.go-65-66 (1)
65-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the expiration timestamp in the update payload.
This test only proves that
Updateexecutes. It does not prove thatupdated.Spec.ExpirationTimestampcontains the requested expiration.Capture the
Updateargument. Assert that the timestamp is non-nil and matches the requested one-hour expiration within a small test tolerance.🤖 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/edit/cluster/hyperfleet_run_test.go` around lines 65 - 66, Update the mock expectation in the hyperfleet run test to capture the payload passed to clusters.Update, then assert that updated.Spec.ExpirationTimestamp is non-nil and matches the requested one-hour expiration within a small test tolerance. Keep the existing Get and Update behavior unchanged while validating the update payload.Source: Coding guidelines
cmd/dlt/machinepool/hyperfleet_run_test.go-93-94 (1)
93-94: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the original cluster key in test cleanup.
Both tests restore a fixed
"cluster1"value instead of the value that existed before the test. A prior or future test that sets another cluster key can leak state into later examples.
cmd/dlt/machinepool/hyperfleet_run_test.go#L93-L94: capture the existing cluster key before clearing it and restore that captured value.cmd/edit/cluster/hyperfleet_run_test.go#L76-L77: capture the existing cluster key before clearing it and restore that captured value.Based on learnings, tests that override shared state must restore the original value.
🤖 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/dlt/machinepool/hyperfleet_run_test.go` around lines 93 - 94, Update the test setup in cmd/dlt/machinepool/hyperfleet_run_test.go at lines 93-94 and cmd/edit/cluster/hyperfleet_run_test.go at lines 76-77: capture the current cluster key before calling ocm.SetClusterKey(""), then have each DeferCleanup restore that captured value instead of the fixed "cluster1".Source: Learnings
cmd/dlt/machinepool/cmd.go-86-88 (1)
86-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winChange error format verb from
%vto%wto preserve error chain.Line 88 wraps the service error using
%v, which preventserrors.Is()anderrors.As()from inspecting the error chain. Change the format verb to%w:Diff
- return fmt.Errorf("error deleting machinepool: %v", err) + return fmt.Errorf("error deleting machinepool: %w", err)The test assertions use
ContainSubstring()to check error messages. The error message text remains identical with%w, so tests will continue to pass. The coding guidelines require "Wrap returned errors with context using%w; do not drop the original error."🤖 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/dlt/machinepool/cmd.go` around lines 86 - 88, Update the error wrapping in the DeleteMachinePool call within the machinepool command to use the wrapping format that preserves the underlying error chain, while keeping the existing contextual message unchanged.Source: Coding guidelines
cmd/create/machinepool/hyperfleet_run_test.go-115-116 (1)
115-116: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the previous cluster key.
Line 116 always restores
"cluster1". This changes shared OCM state when the prior key differs. Read the original key before Line 115 and restore that exact value inDeferCleanup.🤖 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/create/machinepool/hyperfleet_run_test.go` around lines 115 - 116, Update the test setup around ocm.SetClusterKey to capture the existing cluster key before clearing it, then have DeferCleanup restore that captured value instead of hardcoding "cluster1".Source: Learnings
cmd/create/cluster/hyperfleet.go-34-49 (1)
34-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
--versionand--operator-roles-prefixbefore building the cluster spec.Two validation gaps exist in this block:
args.versionis never checked. Line 77 assigns it toRelease.Image. An empty value produces a cluster spec with an empty release image. The HyperFleet node pool path incmd/create/machinepool/hyperfleet.goLines 69-72 rejects the empty case explicitly. Match that behavior.args.operatorRolesPrefixis only checked for emptiness. The OCM path incmd/create/cluster/cmd.goLines 1931-1938 also enforces a 32-character limit andaws.RoleNameRE. Without those checks,ComputeRolesRefbuilds malformed IAM role ARNs and the API call fails with an unclear error.🤖 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/create/cluster/hyperfleet.go` around lines 34 - 49, Extend the validation block before building the cluster spec in the cluster creation flow: reject an empty args.version with the same required-value behavior used by the HyperFleet machine-pool path, and validate args.operatorRolesPrefix against the existing 32-character maximum and aws.RoleNameRE used by the OCM path. Preserve the current required checks and report validation errors before constructing the spec or making the API call.cmd/create/machinepool/hyperfleet.go-95-95 (1)
95-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBounds-check the replica count before narrowing to
int32.
userOptions.Replicasis anint. On a 64-bit platform a value abovemath.MaxInt32wraps and can become negative. The negative value is then written toNodePoolSpec.Replicas.As per path instructions: "Integer overflow: bounds-check user-supplied sizes".🐛 Proposed fix
- replicas := int32(userOptions.Replicas) + if userOptions.Replicas < 0 || userOptions.Replicas > math.MaxInt32 { + r.Reporter.Errorf("--replicas must be between 0 and %d", math.MaxInt32) + exitFn(1) + return + } + replicas := int32(userOptions.Replicas)Add the
mathimport.🤖 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/create/machinepool/hyperfleet.go` at line 95, Bounds-check userOptions.Replicas before converting it to int32 in the machine pool creation flow. Use the math package’s int32 maximum, reject values above that limit with the existing validation/error path, and only then assign the narrowed value to NodePoolSpec.Replicas.Sources: Path instructions, Linters/SAST tools
🧹 Nitpick comments (15)
cmd/login/cmd.go (1)
285-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the
config.Saveerror chain.Line 286 formats
errwith%v. Callers cannot inspect the root cause. Use%winstead. This preserves the current CLI text.Proposed fix
- return fmt.Errorf("failed to save config file: %v", err) + return fmt.Errorf("failed to save config file: %w", err)As per coding guidelines, “Wrap returned errors with context using
%w; do not drop the original error.”🤖 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/login/cmd.go` around lines 285 - 286, Update the error formatting in the config.Save failure branch to use the error-wrapping verb instead of value formatting, preserving the existing context message and allowing callers to inspect the underlying save error.Source: Coding guidelines
tests/e2e/hyperfleet_sanity_test.go (5)
1073-1100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHonor context cancellation in the polling loop.
The loop uses
time.Sleepand checks only the wall-clock deadline. Ifctxis cancelled, the loop keeps polling until the deadline. Select onctx.Done()so the wait stops promptly.🔧 Proposed change
- time.Sleep(15 * time.Second) + select { + case <-ctx.Done(): + GinkgoWriter.Printf("Context cancelled while waiting for VPC %s to drain\n", vpcID) + return + case <-time.After(15 * time.Second): + }As per path instructions: "context.Context for cancellation and timeouts".
🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` around lines 1073 - 1100, Update hfWaitVPCInstancesTerminated to honor ctx cancellation throughout the polling loop: check ctx.Done() before polling and replace the fixed time.Sleep with a context-aware wait that returns promptly when cancellation occurs, while preserving the existing deadline, success, and timeout behavior.Source: Path instructions
152-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttach a deadline to the root context.
context.Background()carries no deadline. The AWS waiters take explicit timeouts, buthfWaitVPCInstancesTerminatedand the direct SDK calls rely on SDK defaults only. The samectxis also captured by everyDeferCleanupclosure, so a hung cleanup call can block the suite until the Ginkgo--timeout 3hfires.Derive a bounded context and register its cancel function.
As per path instructions: "context.Context for cancellation and timeouts".
🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` at line 152, Replace context.Background() in the test setup with a bounded context carrying an appropriate deadline, retain its cancel function, and register cancellation through the test cleanup mechanism. Ensure the derived context is the one passed to hfWaitVPCInstancesTerminated, direct SDK calls, and captured by DeferCleanup closures.Source: Path instructions
195-195: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResolve the availability zone instead of appending
"a".
az := region + "a"assumes the region always exposes an AZ with theasuffix and that the calling account can use it. AWS maps AZ names per account, and some accounts do not have<region>aavailable for every instance type.CreateSubnetthen fails and the test aborts before any cluster is created.Call
DescribeAvailabilityZonesand use the first available zone.🔧 Proposed change
- az := region + "a" + azOut, err := ec2Client.DescribeAvailabilityZones(ctx, &ec2svc.DescribeAvailabilityZonesInput{}) + Expect(err).NotTo(HaveOccurred(), "describing availability zones in %s", region) + Expect(azOut.AvailabilityZones).NotTo(BeEmpty(), "no availability zones in %s", region) + az := awssdk.ToString(azOut.AvailabilityZones[0].ZoneName)Note:
ec2Clientis created at Line 185, so move this block after that line.🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` at line 195, Replace the hard-coded az assignment in the test setup with a DescribeAvailabilityZones call using ec2Client, select the first available zone, and use its name for subnet creation. Move this lookup after ec2Client is initialized and preserve the test’s existing failure handling if no availability zone is returned.
113-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the single
Itinto focused specs withBeforeEach/AfterEach.One
Itblock spans Lines 115 to 933 and covers login,whoami, VPC and IAM provisioning, cluster creation, describe, list, and the full node pool lifecycle. When any assertion fails, Ginkgo reports one spec failure and every later assertion is skipped, so the report does not identify which behavior regressed.Move the shared AWS and Hyperfleet setup into
BeforeAll/BeforeEachinside anOrderedcontainer, then express each behavior as its ownIt: cluster reachesReady,rosa list clustersshows the cluster,rosa describe clustermatches the APIGetresponse, node pools reachReady, and node pool deletion completes.Add a Ginkgo label so the suite can select or exclude this long-running test through the
--label-filterused by thee2e_testtarget.As per coding guidelines: "Ginkgo tests must have single responsibility (one specific behavior per It block), proper setup/cleanup using BeforeEach/AfterEach (especially for cluster-scoped resources)".
🤖 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 `@tests/e2e/hyperfleet_sanity_test.go` around lines 113 - 115, Refactor the “Hyperfleet sanity” suite into an Ordered container with shared AWS/Hyperfleet setup in BeforeAll/BeforeEach and cleanup in AfterEach, then split the monolithic It into focused specs for cluster readiness, listing, description/API consistency, node-pool readiness, and node-pool deletion. Add the suite label used by the e2e_test --label-filter, while preserving the existing resource lifecycle and test ordering.Source: Coding guidelines
941-972: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPaginate and batch Route 53 record deletions.
ListResourceRecordSetsis paginated, so the current code can leave records that causeDeleteHostedZoneto returnHostedZoneNotEmpty. SplitChangeResourceRecordSetsrequests within Route 53 limits, and reportDeleteHostedZoneerrors instead of discarding 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 `@tests/e2e/hyperfleet_sanity_test.go` around lines 941 - 972, Update hfPurgeHostedZoneRecords to paginate ListResourceRecordSets using its continuation markers, collecting deletable records across all pages. Submit ChangeResourceRecordSets requests in batches within Route 53 limits, handling and reporting each batch error. Also update the DeleteHostedZone call site to capture and report its returned error instead of discarding it.Makefile (1)
188-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the
rosabinary before running the Hyperfleet e2e target.The sanity test drives the CLI through
rosacli.NewClient().Runner, which executes the installedrosabinary. The existinge2e_testtarget declaresinstallas a prerequisite.e2e-hyperfleetdoes not, so the test can run against a stale binary or fail when no binary exists.🔧 Add the `install` prerequisite
.PHONY: e2e-hyperfleet -e2e-hyperfleet: +e2e-hyperfleet: install HYPERFLEET_URL="$${HYPERFLEET_URL}" \🤖 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 `@Makefile` around lines 188 - 200, Update the e2e-hyperfleet Make target to depend on the existing install target, ensuring the rosa binary is built before Ginkgo runs while preserving the current environment variables and test arguments.pkg/output/reporter_test.go (1)
138-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the
IsTerminaldelegation test.The fake uses the zero value, so
Terminalisfalse. The assertion passes even ifIsTerminalreturned a constantfalseand never called the inner logger. Add thetruecase to prove delegation.🔧 Proposed test
It("IsTerminal delegates to inner", func() { Expect(NewStructuredReporter(&reportertest.FakeLogger{}).IsTerminal()).To(BeFalse()) + Expect(NewStructuredReporter(&reportertest.FakeLogger{Terminal: true}).IsTerminal()).To(BeTrue()) })Based on path instructions: "Flag weak tests that only restate implementation or changes that weaken existing assertions."
🤖 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 `@pkg/output/reporter_test.go` around lines 138 - 139, Strengthen the IsTerminal test by configuring the reportertest.FakeLogger with Terminal set to true before constructing NewStructuredReporter, then assert IsTerminal returns true. Keep the test focused on proving delegation rather than allowing the zero-value false result to pass without calling the inner logger.Source: Path instructions
cmd/describe/cluster/hyperfleet_run_test.go (1)
39-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the emitted command output.
These tests verify API calls but do not verify text or structured output. Capture stdout. Assert the text output. Decode and assert the structured output.
cmd/describe/cluster/hyperfleet_run_test.go#L39-L61: assert the rendered cluster text.cmd/describe/cluster/hyperfleet_run_test.go#L80-L96: assert the structured cluster fields.cmd/describe/machinepool/hyperfleet_run_test.go#L44-L71: assert the rendered node-pool text.cmd/describe/machinepool/hyperfleet_run_test.go#L95-L118: assert the structured node-pool fields.🤖 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/describe/cluster/hyperfleet_run_test.go` around lines 39 - 61, Extend the describe command tests to capture stdout and assert emitted output: in cmd/describe/cluster/hyperfleet_run_test.go:39-61, assert the rendered cluster text; in cmd/describe/cluster/hyperfleet_run_test.go:80-96, decode the structured output and verify cluster fields; in cmd/describe/machinepool/hyperfleet_run_test.go:44-71, assert the rendered node-pool text; and in cmd/describe/machinepool/hyperfleet_run_test.go:95-118, decode the structured output and verify node-pool fields. Keep the existing API mock expectations and runHyperfleetDescribe flows intact.Source: Coding guidelines
cmd/describe/cluster/hyperfleet.go (1)
30-50: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftPropagate a cancellable context to HyperFleet requests.
Replace
context.Background()in the three handlers with the Cobra command context. Pass the command throughhfDescribeMachinePoolandhfListClusters. Incmd/rosa/main.go, execute the root command with a signal-backed context;root.Execute()currently provides no cancellation context. The HyperFleet client forwards request contexts throughGetandList.🤖 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/describe/cluster/hyperfleet.go` around lines 30 - 50, Propagate a cancellable Cobra context through the HyperFleet describe/list flows instead of using context.Background(): update runHyperfleetDescribe, hfDescribeMachinePool, and hfListClusters to use the command context and thread the command through the helper calls where needed. Also update cmd/rosa/main.go so the root command is executed with a signal-backed context, since root.Execute() does not provide cancellation; keep the existing HyperFleet Get/List call sites using the passed-in request context.Sources: Coding guidelines, Path instructions
cmd/dlt/cluster/hyperfleet.go (1)
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReplace mutable package-level command seams with explicit dependencies.
These variables let tests mutate production dispatch and exit behavior through shared package state. This creates test-order coupling and blocks safe parallel execution.
cmd/dlt/cluster/hyperfleet.go#L14-L22: pass HyperFleet dispatch and exit dependencies through a per-call dependency struct.cmd/dlt/machinepool/hyperfleet.go#L14-L22: pass HyperFleet dispatch and exit dependencies through a per-call dependency struct.cmd/edit/cluster/hyperfleet.go#L17-L25: pass HyperFleet dispatch and exit dependencies through a per-call dependency struct.Based on learnings, “avoid using mutable package-level function variables for new command-level seams.”
🤖 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/dlt/cluster/hyperfleet.go` around lines 14 - 22, Replace the mutable package-level seams around hfEnabled, exitFn, and hfDeleteCluster with a per-call dependency struct in cmd/dlt/cluster/hyperfleet.go lines 14-22, and update the command flow to receive and use those dependencies. Apply the same change to cmd/dlt/machinepool/hyperfleet.go lines 14-22 and cmd/edit/cluster/hyperfleet.go lines 17-25, preserving production defaults while preventing shared test state and enabling safe parallel execution.Source: Learnings
pkg/rosa/runtime.go (1)
96-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared AWS initialization and align the exit style.
WithAWSOnlyduplicates lines 82-92 ofWithAWSexactly. Two copies of the creator lookup will drift.
WithAWSOnlyalso callsos.Exit(1)directly while the neighbouring new functionWithHyperFleetcallshfExitFn(1). Use one exit mechanism across the new runtime code so tests can stub it.As per coding guidelines: "Prefer small focused functions and early returns over deeply nested branches" and the repository DRY expectation.♻️ Proposed refactor
func (r *Runtime) WithAWS() *Runtime { // dependency to ocm client to validate the region r.WithOCM() err := r.OCMClient.ValidateAwsClientRegion() if err != nil { r.Reporter.Errorf("%s", err) os.Exit(1) } - if r.AWSClient == nil { - r.AWSClient = aws.CreateNewClientOrExit(r.Logger, r.Reporter) - } - if r.Creator == nil { - var err error - r.Creator, err = r.AWSClient.GetCreator() - if err != nil { - r.Reporter.Errorf("Failed to get AWS creator: %v", err) - os.Exit(1) - } - } - return r + return r.WithAWSOnly() }Then in
WithAWSOnly, replaceos.Exit(1)withhfExitFn(1)followed byreturn r.🤖 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 `@pkg/rosa/runtime.go` around lines 96 - 111, Refactor WithAWSOnly to reuse the existing shared AWS client and creator initialization performed by WithAWS instead of duplicating the creator lookup. Replace its direct os.Exit(1) call with the shared hfExitFn(1) mechanism, then return r after the exit call so the function remains testable and preserves the current failure flow.Source: Coding guidelines
pkg/hyperfleet/endpoint.go (1)
11-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch the region per hostname label.
awsRegionREruns an unanchored search over the whole hostname. A hostname label such asapi-prod-2.example.commatches the pattern and yieldsapi-prod-2as the region.ExtractRegionthen returns a value that is not an AWS region, and SigV4 signs with it. The request fails later with an opaque signature error instead of the clear "cannot derive AWS region" message.Split the hostname on
.and match each label with an anchored pattern.As per path instructions: "Normalize Unicode and anchor regexes (^$); watch for ReDoS".♻️ Proposed change to match labels
-var awsRegionRE = regexp.MustCompile(`[a-z]+-(?:[a-z]+-)+\d+`) +var awsRegionRE = regexp.MustCompile(`^(?:us|eu|ap|sa|ca|me|af|il|mx)-(?:gov-)?[a-z]+-\d+$`) @@ - region := awsRegionRE.FindString(u.Hostname()) + var region string + for _, label := range strings.Split(u.Hostname(), ".") { + if awsRegionRE.MatchString(label) { + region = label + break + } + }🤖 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 `@pkg/hyperfleet/endpoint.go` around lines 11 - 30, Update awsRegionRE and ExtractRegion so region matching is performed against individual dot-separated hostname labels rather than an unanchored search across the full hostname. Anchor the regex to the complete label, normalize the hostname appropriately before splitting, and return the existing “cannot derive AWS region” error when no entire label matches; preserve valid standard and GovCloud region extraction.Source: Path instructions
pkg/hyperfleet/flags.go (1)
7-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd doc comments to the new exported functions.
AddFlags,Enabled, andExplicitURLare new exported symbols without doc comments.SetURLandResetbelow already have them.As per coding guidelines: "Use exported symbol doc comments when new public types or functions are introduced".♻️ Proposed doc comments
+// AddFlags registers the hidden --hyperfleet-url persistent flag on cmd. func AddFlags(cmd *cobra.Command) { @@ +// Enabled reports whether commands must route to the Platform API instead of OCM. func Enabled() bool { return hyperfleetURL != "" } + +// ExplicitURL returns the configured Platform API endpoint URL. func ExplicitURL() string { return hyperfleetURL }🤖 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 `@pkg/hyperfleet/flags.go` around lines 7 - 18, Add Go doc comments to the exported functions AddFlags, Enabled, and ExplicitURL in pkg/hyperfleet/flags.go. Each comment must begin with the corresponding function name and briefly describe its purpose, matching the existing documentation style used by SetURL and Reset.Source: Coding guidelines
pkg/hyperfleet/hyperfleet_test.go (1)
100-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd GovCloud coverage for
ComputeRolesRef.The suite asserts GovCloud region extraction at Lines 32-33, but
ComputeRolesRefis only tested with the commercial partition. The missing case is exactly the one that hides the hardcodedarn:aws:iam::partition inpkg/hyperfleet/roles.go.Add an entry that builds ARNs for a GovCloud caller and asserts the
aws-us-govpartition.As per coding guidelines: "All code should be covered by tests, using Ginkgo".
🤖 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 `@pkg/hyperfleet/hyperfleet_test.go` around lines 100 - 111, Extend the Ginkgo test for ComputeRolesRef with a GovCloud caller case, using a GovCloud ARN input and asserting every generated role ARN uses the aws-us-gov partition while preserving the cluster prefix and account ID. Add coverage for all seven fields in the existing ComputeRolesRef test.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/create/cluster/hyperfleet.go`:
- Around line 60-65: Update the subnet handling near subnetOut.Subnets in the
hyperfleet creation flow to import and use awssdk.ToString for VpcId and
AvailabilityZone instead of direct pointer dereferences. Validate both converted
values, report an error through r.Reporter.Errorf, and exit when either field is
missing, while preserving the existing empty-subnet handling.
In `@cmd/create/machinepool/hyperfleet_run_test.go`:
- Around line 71-80: Update the success tests around runHyperfleetCreate and the
nodePools.EXPECT().Create mocks to capture the NodePool argument instead of
accepting gomock.Any(). Assert the payload’s name, release image, replica count,
instance profile, subnet, and cluster name, and verify the positional-name case
sets ObjectMeta.Name to "my-np".
In `@cmd/create/machinepool/hyperfleet.go`:
- Around line 40-93: Update the validation and error-handling branches in the
machine pool creation flow to return immediately after every exitFn(1) call,
including the node name, cluster key, cluster resolution, cluster fetch, release
image, subnet, instance profile, and final validation branches. Match the
existing WithHyperFleet pattern so stubbed exitFn calls cannot fall through into
invalid state or dereference a nil cluster.
In `@cmd/dlt/cluster/hyperfleet.go`:
- Around line 42-45: Restore the existing confirmation flow before the
destructive delete calls: in cmd/dlt/cluster/hyperfleet.go lines 42-45, invoke
the established delete-cluster confirmation before Clusters(...).Delete; in
cmd/dlt/machinepool/hyperfleet.go lines 54-57, invoke the established
machine-pool confirmation before NodePools(...).Delete. Preserve --yes behavior
and only call each API after confirmation succeeds.
- Line 28: Replace context.Background in the Hyperfleet command flows with the
Cobra command context, then apply the existing command timeout policy before
Platform API operations: update cmd/dlt/cluster/hyperfleet.go lines 28-28 for
resolving and deleting, cmd/dlt/machinepool/hyperfleet.go lines 25-25 for
resolving and deleting, and cmd/edit/cluster/hyperfleet.go lines 31-31 for
resolving, reading, and updating. Propagate the resulting context through each
relevant API call so cancellation and timeouts terminate blocked requests.
In `@cmd/edit/cluster/hyperfleet.go`:
- Around line 39-42: Extend the validation before clusters.Update in the
HyperFleet edit flow to reject any changed edit flag that HyperFleet does not
support, including combinations of --expiration with OCM-only flags. Preserve
--cluster and selector/output flags as valid, and retain the existing
error-reporting and exit behavior for unsupported combinations.
In `@cmd/edit/machinepool/hyperfleet.go`:
- Around line 70-74: Validate userOptions.replicas in the HyperFleet edit flow
before casting it to int32, since this path bypasses EditMachinepoolOptions.Bind
and can otherwise accept negative values or overflow beyond math.MaxInt32.
Update the logic around the updated := np.DeepCopy() and nodePools.Update call
to reject any replica count below 0 or above int32 limits, and preserve the
existing update behavior for valid inputs. Add boundary tests covering the lower
and upper rejected values as well as a valid in-range case.
In `@cmd/list/cluster/hyperfleet.go`:
- Around line 33-48: Update the HyperFleet cluster-list handler to check
output.HasFlag() before creating the tabwriter and serialize a stable
cluster-list representation for JSON and YAML output, preserving the existing
table behavior when no structured format is requested. Follow the command’s
existing structured-output conventions and add coverage for both JSON and YAML
paths.
In `@cmd/whoami/cmd.go`:
- Around line 61-65: Update run’s endpoint-selection logic to derive the
effective Hyperfleet URL from hyperfleet.ExplicitURL(), falling back to
cfg.HyperfleetURL when no explicit URL is provided. Select WithAWSOnly when that
effective URL exists and cfg is absent or has an invalid/expired OCM token,
using cfg.Armed() or equivalent validity logic instead of config.IsNotValid;
otherwise preserve WithAWS, and add coverage for both explicit-URL and
expired-token scenarios.
In `@go.mod`:
- Around line 30-31: Replace the pseudo-version entries for
github.com/openshift-online/rosa-hyperfleet-api/clientset and
github.com/openshift/hypershift/api in go.mod, including the additional
occurrence, with exact pinned official release versions. If an approved
HyperFleet preview exception applies, document its scope and obtain human review
before shipping.
In `@Makefile`:
- Line 166: Replace the non-portable sed invocation in the mocks target with a
rewrite that works on both GNU and BSD/macOS sed, while preserving the existing
import-path substitution in wrappers_mock.go. Alternatively, configure mockgen
with the required imports/package options and remove the post-processing step.
In `@pkg/config/config.go`:
- Around line 154-166: Replace the %v verbs with %w in the returned fmt.Errorf
calls at pkg/config/config.go lines 154-166, 195-199, 269-270, 305-306, and
329-330, and in pkg/hyperfleet/nodepool.go lines 16-19, preserving each existing
error message and wrapped error value so errors.Is and errors.As can inspect the
underlying errors.
In `@pkg/hyperfleet/cluster.go`:
- Around line 16-19: Update the error formatting in the cluster-listing flow to
wrap the original error with `%w` instead of formatting it with `%v`. Preserve
the existing context message and return behavior in the `Clusters(...).List`
call so callers can use errors.Is or errors.As on the returned error.
In `@pkg/hyperfleet/endpoint.go`:
- Around line 17-30: Update ExtractRegion to reject any URL whose parsed scheme
is not HTTPS, returning a clear validation error before deriving the region.
Ensure the same validation is applied when URLs enter the runtime through
WithHyperFleet and hyperfleet.SetURL, so all HyperFleet client configuration
paths reject cleartext endpoints.
In `@pkg/hyperfleet/roles.go`:
- Around line 12-25: Update ComputeRolesRef in pkg/hyperfleet/roles.go to accept
an AWS partition parameter and use it when constructing role ARNs, then update
its caller in cmd/create/cluster/hyperfleet.go to pass r.Creator.Partition.
Extend the test case in pkg/hyperfleet/hyperfleet_test.go to pass aws-us-gov and
assert the resulting GovCloud role ARNs, while preserving commercial ARN
coverage.
In `@pkg/rosa/runtime_test.go`:
- Around line 78-95: Update the hfNewClient stub in the success test to capture
the supplied *hfrest.Config, control AWS_REGION for a deterministic value, and
assert the captured configuration’s Host, Region, AccountID, and CallerARN
alongside the existing Runtime assertions. Preserve the current successful
client setup and cleanup any environment override after the test.
In `@pkg/rosa/runtime.go`:
- Around line 158-216: Update Runtime.WithHyperFleet to create a bounded context
with a timeout instead of context.Background(), and defer its cancel function
immediately after creation. Reuse this context for awsLoadConfig and
awsGetIdentity so HyperFleet initialization cannot hang indefinitely.
In `@tests/e2e/hyperfleet_sanity_test.go`:
- Around line 309-321: Register cleanup immediately after each AWS create call
in tests/e2e/hyperfleet_sanity_test.go:281-307, 309-321, 358-423, and 425-461:
clean up igwID, natEIPAllocID, publicRTID, privateRTID, and workerSGID before
subsequent fallible calls; make route-table cleanup disassociate only when its
matching association ID is non-empty, and ensure each cleanup tolerates partial
configuration while removing any duplicate later registrations.
---
Minor comments:
In `@cmd/create/cluster/hyperfleet.go`:
- Around line 34-49: Extend the validation block before building the cluster
spec in the cluster creation flow: reject an empty args.version with the same
required-value behavior used by the HyperFleet machine-pool path, and validate
args.operatorRolesPrefix against the existing 32-character maximum and
aws.RoleNameRE used by the OCM path. Preserve the current required checks and
report validation errors before constructing the spec or making the API call.
In `@cmd/create/machinepool/hyperfleet_run_test.go`:
- Around line 115-116: Update the test setup around ocm.SetClusterKey to capture
the existing cluster key before clearing it, then have DeferCleanup restore that
captured value instead of hardcoding "cluster1".
In `@cmd/create/machinepool/hyperfleet.go`:
- Line 95: Bounds-check userOptions.Replicas before converting it to int32 in
the machine pool creation flow. Use the math package’s int32 maximum, reject
values above that limit with the existing validation/error path, and only then
assign the narrowed value to NodePoolSpec.Replicas.
In `@cmd/dlt/machinepool/cmd.go`:
- Around line 86-88: Update the error wrapping in the DeleteMachinePool call
within the machinepool command to use the wrapping format that preserves the
underlying error chain, while keeping the existing contextual message unchanged.
In `@cmd/dlt/machinepool/hyperfleet_run_test.go`:
- Around line 93-94: Update the test setup in
cmd/dlt/machinepool/hyperfleet_run_test.go at lines 93-94 and
cmd/edit/cluster/hyperfleet_run_test.go at lines 76-77: capture the current
cluster key before calling ocm.SetClusterKey(""), then have each DeferCleanup
restore that captured value instead of the fixed "cluster1".
In `@cmd/edit/cluster/hyperfleet_run_test.go`:
- Around line 65-66: Update the mock expectation in the hyperfleet run test to
capture the payload passed to clusters.Update, then assert that
updated.Spec.ExpirationTimestamp is non-nil and matches the requested one-hour
expiration within a small test tolerance. Keep the existing Get and Update
behavior unchanged while validating the update payload.
In `@cmd/list/cluster/hyperfleet.go`:
- Line 48: Update the output finalization around writer.Flush in the command
flow to check and handle its returned error. Report the flush failure and
terminate with a nonzero status instead of allowing the command to report
success after partial output.
In `@cmd/list/machinepool/hyperfleet_run_test.go`:
- Around line 83-89: Capture the existing cluster key before each test mutates
it, then restore that captured value during cleanup instead of the hardcoded
"cluster1". Apply this in cmd/list/machinepool/hyperfleet_run_test.go#L83-L89,
cmd/dlt/cluster/hyperfleet_run_test.go#L48-L54, and
cmd/edit/machinepool/hyperfleet_run_test.go#L133-L149, using the relevant test
setup and ocm.SetClusterKey calls.
In `@tests/e2e/hyperfleet_sanity_test.go`:
- Around line 1189-1190: Update hfBuildTrustPolicy to handle the error returned
by json.Marshal instead of discarding it. Surface marshalling failures
immediately through the function’s existing error-handling or return contract,
ensuring CreateRole does not receive an empty policy string after serialization
fails.
- Around line 791-794: Update the e2e-hyperfleet documentation block in the
Makefile to list HYPERFLEET_INSTANCE_TYPE under the Optional inputs, matching
the environment variable consumed near instanceType. Do not change the test
logic or add explicit ginkgo forwarding, since the inherited environment already
provides the variable.
- Around line 1129-1154: Update hfOIDCThumbprint to set MinVersion to
tls.VersionTLS12 in the tls.Config passed to tls.Dial, and add a clear comment
explaining that certificate validation is intentionally skipped only to read the
issuer’s raw chain for thumbprint registration. Keep the existing
InsecureSkipVerify behavior and SHA-1 thumbprint extraction unchanged, but
document the tradeoff and why the residual risk is acceptable in this test-only
flow.
---
Nitpick comments:
In `@cmd/describe/cluster/hyperfleet_run_test.go`:
- Around line 39-61: Extend the describe command tests to capture stdout and
assert emitted output: in cmd/describe/cluster/hyperfleet_run_test.go:39-61,
assert the rendered cluster text; in
cmd/describe/cluster/hyperfleet_run_test.go:80-96, decode the structured output
and verify cluster fields; in
cmd/describe/machinepool/hyperfleet_run_test.go:44-71, assert the rendered
node-pool text; and in cmd/describe/machinepool/hyperfleet_run_test.go:95-118,
decode the structured output and verify node-pool fields. Keep the existing API
mock expectations and runHyperfleetDescribe flows intact.
In `@cmd/describe/cluster/hyperfleet.go`:
- Around line 30-50: Propagate a cancellable Cobra context through the
HyperFleet describe/list flows instead of using context.Background(): update
runHyperfleetDescribe, hfDescribeMachinePool, and hfListClusters to use the
command context and thread the command through the helper calls where needed.
Also update cmd/rosa/main.go so the root command is executed with a
signal-backed context, since root.Execute() does not provide cancellation; keep
the existing HyperFleet Get/List call sites using the passed-in request context.
In `@cmd/dlt/cluster/hyperfleet.go`:
- Around line 14-22: Replace the mutable package-level seams around hfEnabled,
exitFn, and hfDeleteCluster with a per-call dependency struct in
cmd/dlt/cluster/hyperfleet.go lines 14-22, and update the command flow to
receive and use those dependencies. Apply the same change to
cmd/dlt/machinepool/hyperfleet.go lines 14-22 and cmd/edit/cluster/hyperfleet.go
lines 17-25, preserving production defaults while preventing shared test state
and enabling safe parallel execution.
In `@cmd/login/cmd.go`:
- Around line 285-286: Update the error formatting in the config.Save failure
branch to use the error-wrapping verb instead of value formatting, preserving
the existing context message and allowing callers to inspect the underlying save
error.
In `@Makefile`:
- Around line 188-200: Update the e2e-hyperfleet Make target to depend on the
existing install target, ensuring the rosa binary is built before Ginkgo runs
while preserving the current environment variables and test arguments.
In `@pkg/hyperfleet/endpoint.go`:
- Around line 11-30: Update awsRegionRE and ExtractRegion so region matching is
performed against individual dot-separated hostname labels rather than an
unanchored search across the full hostname. Anchor the regex to the complete
label, normalize the hostname appropriately before splitting, and return the
existing “cannot derive AWS region” error when no entire label matches; preserve
valid standard and GovCloud region extraction.
In `@pkg/hyperfleet/flags.go`:
- Around line 7-18: Add Go doc comments to the exported functions AddFlags,
Enabled, and ExplicitURL in pkg/hyperfleet/flags.go. Each comment must begin
with the corresponding function name and briefly describe its purpose, matching
the existing documentation style used by SetURL and Reset.
In `@pkg/hyperfleet/hyperfleet_test.go`:
- Around line 100-111: Extend the Ginkgo test for ComputeRolesRef with a
GovCloud caller case, using a GovCloud ARN input and asserting every generated
role ARN uses the aws-us-gov partition while preserving the cluster prefix and
account ID. Add coverage for all seven fields in the existing ComputeRolesRef
test.
In `@pkg/output/reporter_test.go`:
- Around line 138-139: Strengthen the IsTerminal test by configuring the
reportertest.FakeLogger with Terminal set to true before constructing
NewStructuredReporter, then assert IsTerminal returns true. Keep the test
focused on proving delegation rather than allowing the zero-value false result
to pass without calling the inner logger.
In `@pkg/rosa/runtime.go`:
- Around line 96-111: Refactor WithAWSOnly to reuse the existing shared AWS
client and creator initialization performed by WithAWS instead of duplicating
the creator lookup. Replace its direct os.Exit(1) call with the shared
hfExitFn(1) mechanism, then return r after the exit call so the function remains
testable and preserves the current failure flow.
In `@tests/e2e/hyperfleet_sanity_test.go`:
- Around line 1073-1100: Update hfWaitVPCInstancesTerminated to honor ctx
cancellation throughout the polling loop: check ctx.Done() before polling and
replace the fixed time.Sleep with a context-aware wait that returns promptly
when cancellation occurs, while preserving the existing deadline, success, and
timeout behavior.
- Line 152: Replace context.Background() in the test setup with a bounded
context carrying an appropriate deadline, retain its cancel function, and
register cancellation through the test cleanup mechanism. Ensure the derived
context is the one passed to hfWaitVPCInstancesTerminated, direct SDK calls, and
captured by DeferCleanup closures.
- Line 195: Replace the hard-coded az assignment in the test setup with a
DescribeAvailabilityZones call using ec2Client, select the first available zone,
and use its name for subnet creation. Move this lookup after ec2Client is
initialized and preserve the test’s existing failure handling if no availability
zone is returned.
- Around line 113-115: Refactor the “Hyperfleet sanity” suite into an Ordered
container with shared AWS/Hyperfleet setup in BeforeAll/BeforeEach and cleanup
in AfterEach, then split the monolithic It into focused specs for cluster
readiness, listing, description/API consistency, node-pool readiness, and
node-pool deletion. Add the suite label used by the e2e_test --label-filter,
while preserving the existing resource lifecycle and test ordering.
- Around line 941-972: Update hfPurgeHostedZoneRecords to paginate
ListResourceRecordSets using its continuation markers, collecting deletable
records across all pages. Submit ChangeResourceRecordSets requests in batches
within Route 53 limits, handling and reporting each batch error. Also update the
DeleteHostedZone call site to capture and report its returned error instead of
discarding it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| func ExtractRegion(rawURL string) (string, error) { | ||
| u, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid --hyperfleet-url %q: %w", rawURL, err) | ||
| } | ||
| region := awsRegionRE.FindString(u.Hostname()) | ||
| if region == "" { | ||
| return "", fmt.Errorf( | ||
| "cannot derive AWS region from --hyperfleet-url %q; use --region to specify it explicitly", | ||
| rawURL, | ||
| ) | ||
| } | ||
| return region, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any HyperFleet code path validates the URL scheme.
rg -nP -C4 '(hyperfleet-url|hyperfleetURL|HyperfleetURL|rawURL)' --type=go -g '!vendor/**'
# Inspect how the vendored client treats Config.Host schemes.
fd -t f -p 'rosa-hyperfleet-api/clientset/rest' vendor | head -50
rg -nP -C4 '(Host|https?://|url\.Parse|Scheme)' --type=go -g 'vendor/github.com/openshift-online/rosa-hyperfleet-api/clientset/rest/**'Repository: openshift/rosa
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- HyperFleet client files ---'
git ls-files | grep -E 'rosa-hyperfleet-api|hyperfleet.*rest|clientset/rest' || true
printf '%s\n' '--- Host and signing references ---'
rg -n -C4 'type Config struct|Host[[:space:]]+string|Config\.Host|SigV4|SignHTTP|Signing|url\.Parse|http://' . \
-g '*.go' -g '!vendor/**' -g 'vendor/**' | head -300
printf '%s\n' '--- URL entry points and setters ---'
rg -n -C4 'SetURL|hfExplicitURL|HyperFleet.*URL|hyperfleet-url' --type=go . | head -250Repository: openshift/rosa
Length of output: 39110
🏁 Script executed:
#!/bin/bash
set -u
for f in \
vendor/github.com/openshift-online/rosa-hyperfleet-api/clientset/rest/config.go \
vendor/github.com/openshift-online/rosa-hyperfleet-api/clientset/transport/sigv4.go \
vendor/github.com/openshift-online/rosa-hyperfleet-api/clientset/hyperfleet.go \
pkg/hyperfleet/flags.go \
cmd/rosa/main.go; do
printf '\n--- %s ---\n' "$f"
cat -n "$f"
doneRepository: openshift/rosa
Length of output: 18724
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
endpoint = Path("pkg/hyperfleet/endpoint.go").read_text()
runtime = Path("pkg/rosa/runtime.go").read_text()
transport = Path("vendor/github.com/openshift-online/rosa-hyperfleet-api/clientset/transport/sigv4.go").read_text()
client = Path("vendor/github.com/openshift-online/rosa-hyperfleet-api/clientset/hyperfleet.go").read_text()
assert "url.Parse(rawURL)" in endpoint
assert "u.Scheme" not in endpoint
assert 'Host: rawURL' in runtime
assert "signer.SignHTTP" in transport
assert "return t.inner.RoundTrip(req)" in transport
assert "Transport: sigv4" in client
assert "Host: cfg.Host" in client
print("ExtractRegion accepts parsed URLs without a scheme check.")
print("WithHyperFleet forwards rawURL as hfrest.Config.Host.")
print("NewForConfig installs the SigV4 transport for requests using cfg.Host.")
print("SigV4RoundTripper signs requests and forwards them unchanged to the inner transport; an http:// Host therefore uses cleartext HTTP.")
PYRepository: openshift/rosa
Length of output: 467
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal · Exploitability: Moderate
Reachability path
● Entry
pkg/rosa/runner_test.go:84
RuntimeWithHyperFleet
│
▼
● Hop
pkg/rosa/runner.go:64
RuntimeWithHyperFleet
│
▼
● Hop
pkg/rosa/runtime.go:158
WithHyperFleet: Resolve region: explicit flag/env takes precedence, then extracted from URL.
│
▼
● Sink
pkg/hyperfleet/endpoint.go
Reject non-HTTPS HyperFleet endpoint URLs.
ExtractRegion does not validate the URL scheme. WithHyperFleet passes rawURL to hfrest.Config.Host, and the client signs requests before forwarding them. An http:// URL therefore sends AWS credentials over cleartext. Validate the scheme when the URL enters the runtime, including URLs loaded through hyperfleet.SetURL.
🤖 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 `@pkg/hyperfleet/endpoint.go` around lines 17 - 30, Update ExtractRegion to
reject any URL whose parsed scheme is not HTTPS, returning a clear validation
error before deriving the region. Ensure the same validation is applied when
URLs enter the runtime through WithHyperFleet and hyperfleet.SetURL, so all
HyperFleet client configuration paths reject cleartext endpoints.
gdbranco
changed the title
|
Testing it |
- Portable `perl -pi -e` in Makefile replacing non-portable `sed -i ''` - `%w` error wrapping in pkg/config/config.go, pkg/hyperfleet/cluster.go, pkg/hyperfleet/nodepool.go - HTTPS validation at WithHyperFleet entry; SetURL and ExtractRegion reject cleartext HTTP - whoami endpoint-selection uses cfg.Armed() + ExplicitURL() fallback via useAWSOnly helper - hyperfleet cluster list checks output.HasFlag() and emits structured JSON/YAML via clusterListItem - edit machinepool validates replicas within [0, math.MaxInt32] before int32 cast - delete cluster/machinepool: add confirmFn stub, call before delete; thread cmd.Context() through dispatch - create machinepool: return after every exitFn(1) to prevent fall-through into invalid state - create machinepool success tests capture NodePool argument and assert all payload fields - create cluster: awssdk.ToString for VpcId/AvailabilityZone with missing-field validation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/dlt/machinepool/hyperfleet.go (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit dependencies for the new command seams.
exitFn,confirmFn, andhfDeleteMachinePooladd mutable package state for test control. Pass these collaborators throughrunHyperfleetDeleteor a dependency struct instead. This keeps command dependencies explicit and prevents cross-test interference.Based on learnings, “Avoid using mutable package-level function variables for new command-level seams.”
🤖 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/dlt/machinepool/hyperfleet.go` around lines 15 - 23, Remove the mutable package-level seams exitFn, confirmFn, and hfDeleteMachinePool, and make these collaborators explicit through runHyperfleetDelete or a dedicated dependency struct. Update the command setup and callers to pass the required exit, confirmation, and delete-handler dependencies directly, preserving existing behavior without shared package state.Sources: Coding guidelines, Learnings
🤖 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/edit/machinepool/hyperfleet_run_test.go`:
- Around line 218-245: Strengthen the test around runHyperfleetEdit by capturing
the NodePool argument passed to nps.EXPECT().Update instead of accepting any
payload, then assert that the updated Spec.NodePool.Replicas equals
math.MaxInt32. Keep the existing setup and execution unchanged, and ensure the
assertion verifies the value was propagated without truncation or leaving the
original replica count.
In `@cmd/list/cluster/hyperfleet_run_test.go`:
- Around line 42-50: Update the captureStdout helper to handle errors from
os.Pipe, w.Close, and io.ReadAll instead of discarding them, using the test’s
established failure mechanism. Register cleanup to restore os.Stdout before
invoking f so it is restored even if the callback panics, while preserving
captured output behavior.
---
Nitpick comments:
In `@cmd/dlt/machinepool/hyperfleet.go`:
- Around line 15-23: Remove the mutable package-level seams exitFn, confirmFn,
and hfDeleteMachinePool, and make these collaborators explicit through
runHyperfleetDelete or a dedicated dependency struct. Update the command setup
and callers to pass the required exit, confirmation, and delete-handler
dependencies directly, preserving existing behavior without shared package
state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 15726836-f131-4e96-ba84-7b00ab43fa04
⛔ Files ignored due to path filters (1)
assets/bindata.gois excluded by!assets/bindata.go
📒 Files selected for processing (31)
Makefilecmd/create/cluster/hyperfleet.gocmd/create/cluster/hyperfleet_test.gocmd/create/machinepool/hyperfleet.gocmd/create/machinepool/hyperfleet_run_test.gocmd/dlt/cluster/cmd.gocmd/dlt/cluster/hyperfleet.gocmd/dlt/cluster/hyperfleet_run_test.gocmd/dlt/cluster/hyperfleet_test.gocmd/dlt/machinepool/cmd.gocmd/dlt/machinepool/hyperfleet.gocmd/dlt/machinepool/hyperfleet_run_test.gocmd/dlt/machinepool/hyperfleet_test.gocmd/edit/cluster/hyperfleet.gocmd/edit/cluster/hyperfleet_run_test.gocmd/edit/machinepool/hyperfleet.gocmd/edit/machinepool/hyperfleet_run_test.gocmd/list/cluster/hyperfleet.gocmd/list/cluster/hyperfleet_run_test.gocmd/whoami/cmd.gocmd/whoami/cmd_test.gopkg/config/config.gopkg/hyperfleet/cluster.gopkg/hyperfleet/endpoint.gopkg/hyperfleet/flags.gopkg/hyperfleet/hyperfleet_test.gopkg/hyperfleet/nodepool.gopkg/hyperfleet/roles.gopkg/rosa/runtime.gopkg/rosa/runtime_test.gotests/e2e/hyperfleet_sanity_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- cmd/dlt/machinepool/hyperfleet_test.go
- cmd/dlt/machinepool/cmd.go
- Makefile
- pkg/hyperfleet/cluster.go
- cmd/dlt/cluster/hyperfleet.go
- cmd/dlt/cluster/hyperfleet_test.go
- pkg/hyperfleet/nodepool.go
- cmd/dlt/cluster/hyperfleet_run_test.go
- cmd/create/machinepool/hyperfleet_run_test.go
- pkg/hyperfleet/hyperfleet_test.go
- cmd/edit/cluster/hyperfleet.go
- cmd/create/machinepool/hyperfleet.go
- cmd/edit/machinepool/hyperfleet.go
- cmd/dlt/cluster/cmd.go
- cmd/create/cluster/hyperfleet.go
- pkg/config/config.go
- cmd/whoami/cmd.go
- pkg/rosa/runtime.go
- tests/e2e/hyperfleet_sanity_test.go
| It("accepts math.MaxInt32 as a valid replica count", func() { | ||
| ctrl := gomock.NewController(GinkgoT()) | ||
| _, nodePools, np := func() (*hfmocks.MockInterface, *hfmocks.MockNodePoolInterface, *v1alpha1.NodePool) { | ||
| hf, clusters, nps := newEditMPMocks(ctrl) | ||
| replicas := int32(3) | ||
| np := &v1alpha1.NodePool{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "my-np", UID: types.UID("np-uid-1")}, | ||
| Spec: v1alpha1.NodePoolSpec{ | ||
| NodePool: hypershiftv1beta1.NodePoolSpec{Replicas: &replicas}, | ||
| }, | ||
| } | ||
| clusters.EXPECT().List(gomock.Any(), gomock.Any()).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "cluster1", UID: types.UID("cluster-uid")}, | ||
| }}}, nil) | ||
| nps.EXPECT().List(gomock.Any(), gomock.Any()).Return( | ||
| &v1alpha1.NodePoolList{Items: []v1alpha1.NodePool{*np}}, nil) | ||
| nps.EXPECT().Get(gomock.Any(), "np-uid-1", gomock.Any()).Return(np, nil) | ||
| nps.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Return(np, nil) | ||
| t.RosaRuntime.HyperFleetClient = hf | ||
| return hf, nps, np | ||
| }() | ||
| _ = nodePools | ||
| _ = np | ||
|
|
||
| runHyperfleetEdit(t.RosaRuntime, | ||
| &EditMachinepoolUserOptions{machinepool: "my-np", replicas: math.MaxInt32}, | ||
| makeEditCmd("5"), nil) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the replica value passed to Update.
The mock accepts any update payload. The test passes if runHyperfleetEdit truncates math.MaxInt32 or leaves the existing value unchanged. Capture the updated NodePool and assert that Spec.NodePool.Replicas equals math.MaxInt32.
As per path instructions, “Flag weak tests that only restate implementation.”
🤖 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/edit/machinepool/hyperfleet_run_test.go` around lines 218 - 245,
Strengthen the test around runHyperfleetEdit by capturing the NodePool argument
passed to nps.EXPECT().Update instead of accepting any payload, then assert that
the updated Spec.NodePool.Replicas equals math.MaxInt32. Keep the existing setup
and execution unchanged, and ensure the assertion verifies the value was
propagated without truncation or leaving the original replica count.
Sources: Coding guidelines, Path instructions
| captureStdout := func(f func()) string { | ||
| r, w, _ := os.Pipe() | ||
| orig := os.Stdout | ||
| os.Stdout = w | ||
| f() | ||
| w.Close() | ||
| os.Stdout = orig | ||
| out, _ := io.ReadAll(r) | ||
| return string(out) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Handle stdout capture errors and always restore os.Stdout.
os.Pipe, w.Close, and io.ReadAll errors are discarded. A callback panic also leaves the process stdout redirected. Check each I/O error and restore stdout with cleanup before invoking the callback.
As per path instructions, “Never ignore error returns.”
🤖 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/list/cluster/hyperfleet_run_test.go` around lines 42 - 50, Update the
captureStdout helper to handle errors from os.Pipe, w.Close, and io.ReadAll
instead of discarding them, using the test’s established failure mechanism.
Register cleanup to restore os.Stdout before invoking f so it is restored even
if the callback panics, while preserving captured output behavior.
Source: Path instructions
…VPC teardown SGs created by the cluster controller and ingress controller are not removed when the cluster is deleted, causing DeleteVpc to fail silently and leak the VPC. Add hfDeleteVPCSecurityGroups (same pattern as hfDeleteAvailableENIs) and call it in the cluster DeferCleanup after ENI cleanup.
…rdown ALB/NLB deletion is async; security groups they reference cannot be deleted until AWS fully removes the load balancer. Add hfWaitVPCClassicLoadBalancersDeleted and hfWaitVPCLoadBalancersDeleted polls after each LB delete step, and add hfDeleteVPCSecurityGroups to remove controller-created SGs before DeleteVpc. Also verify VPC deletion via hfWaitVPCDeleted and log errors on DeleteVpc failure.
2 participants
PR Summary
POC integration with hyperfleet sdk
Detailed Description of the Issue
These commits wire the ROSA CLI into the Hyperfleet Platform API v2, an alternative control plane for managing HyperShift-based clusters and node pools without going through OCM.
terminate, drain orphaned ENIs, delete classic ELBs and ALB/NLBs before releasing the VPC, and purge hosted zone records before deletion.
Why: Enables ROSA CLI users to manage clusters through the new Platform API v2 without OCM as an intermediary, which is the foundation for the Hyperfleet product offering.
Related Issues and PRs
ROSAENG-62084 — Hyperfleet (Platform API v2) integration in the ROSA CLI
Type of Change
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit
New Features
Bug Fixes