MGMT-24693: Normalize unbracketed IPv6 ignition endpoint URLs before …#10570
MGMT-24693: Normalize unbracketed IPv6 ignition endpoint URLs before …#10570andrej1991 wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@andrej1991: This pull request references MGMT-24693 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR normalizes unbracketed IPv6 HTTP URLs for ignition endpoint handling and updates disconnected operator mirroring commands to run through a retry wrapper. ChangesIPv6 URL normalization
Mirror command retries
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MirrorCaller
participant run_mirror_command_with_retry
participant mirror_command_succeeded
participant MirrorCommand
MirrorCaller->>run_mirror_command_with_retry: mirror command
run_mirror_command_with_retry->>MirrorCommand: execute and tee output
MirrorCommand-->>run_mirror_command_with_retry: exit status and log output
run_mirror_command_with_retry->>mirror_command_succeeded: inspect output for failures
mirror_command_succeeded-->>run_mirror_command_with_retry: success or failure
run_mirror_command_with_retry-->>MirrorCaller: return after success or retries
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/M
|
/test edge-e2e-ai-operator-disconnected-capi |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andrej1991 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/test edge-e2e-ai-operator-disconnected-capi |
4 similar comments
|
/test edge-e2e-ai-operator-disconnected-capi |
|
/test edge-e2e-ai-operator-disconnected-capi |
|
/test edge-e2e-ai-operator-disconnected-capi |
|
/test edge-e2e-ai-operator-disconnected-capi |
openshift-ci
Bot
added
size/L
|
/test edge-e2e-ai-operator-disconnected-capi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/bminventory/inventory_ignition_endpoint_unit_test.go (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew file mixes plain
testing.Twith the package's existing Ginkgo/Gomega convention.This adds a stdlib-style unit test in a package that otherwise uses Ginkgo/Gomega for tests (see
internal/host/hostutil/host_utils_test.gofor a similar pattern). Consider folding this coverage into the existing Ginkgo suite for consistency, unless there's a specific reason to keep it framework-independent.🤖 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 `@internal/bminventory/inventory_ignition_endpoint_unit_test.go` around lines 1 - 22, The new validation coverage in TestValidateIgnitionEndpointNormalizesUnbracketedIPv6 is using plain testing.T, but this package’s tests follow the Ginkgo/Gomega style. Move this case into the existing bminventory Ginkgo suite and express the same validateIgnitionEndpoint and IgnitionEndpoint URL normalization assertions with Gomega, keeping the behavior coverage while matching the surrounding test convention.internal/host/hostutil/host_utils_ignition_url_test.go (1)
1-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test coverage with
host_utils_test.go(lines 160-168).This file's
TestGetIgnitionEndpointAndCertUnbracketedIPv6CustomEndpointand the newIt("for cluster with unbracketed IPv6 custom IgnitionEndpoint", ...)block inhost_utils_test.gotest the same input/output using two different test frameworks (stdlibtestinghere vs. Ginkgo/Gomega there). Consider dropping one to avoid duplicate maintenance and mixed test-framework conventions in the same package.🤖 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 `@internal/host/hostutil/host_utils_ignition_url_test.go` around lines 1 - 31, The new unbracketed IPv6 custom IgnitionEndpoint scenario is duplicated between TestGetIgnitionEndpointAndCertUnbracketedIPv6CustomEndpoint and the Ginkgo case in host_utils_test.go, so remove one of the two tests to avoid redundant coverage and mixed testing styles in the hostutil package. Keep the remaining test aligned with GetIgnitionEndpointAndCert behavior and ensure it still covers the same URL normalization and nil cert expectation.deploy/operator/capi/deploy_capi_cluster.sh (1)
83-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve substitution failures and quote path expansions.
Line 83 masks
get_image_repository_onlyfailures throughexport, and Line 101 can split$PWDwhen constructing the workspace URL.Proposed fix
- export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi" + provider_image_repo="$(get_image_repository_only "${PROVIDER_IMAGE}")" + export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/${provider_image_repo}:capi" ... - run_mirror_command_with_retry oc-mirror --config isc.yaml --authfile "${PULL_SECRET_FILE}" --workspace file://$PWD/mirror docker://"${OCP_MIRROR_REGISTRY}" --v2 + run_mirror_command_with_retry oc-mirror --config isc.yaml --authfile "${PULL_SECRET_FILE}" --workspace "file://${PWD}/mirror" docker://"${OCP_MIRROR_REGISTRY}" --v2Also applies to: 101-101
🤖 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 `@deploy/operator/capi/deploy_capi_cluster.sh` at line 83, The provider image assignment currently hides failures from get_image_repository_only because the command substitution is wrapped directly in export; assign the substitution to PROVIDER_LOCAL_IMAGE first (or otherwise preserve its exit status) and then export it so errors are not masked. Also update the workspace URL construction to quote the PWD expansion so the path in the URL is not split or globbed. Use the existing get_image_repository_only and PROVIDER_LOCAL_IMAGE / workspace URL setup in deploy_capi_cluster.sh to make the change.Source: Linters/SAST tools
deploy/operator/setup_hive.sh (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuote the sourced helper path.
This avoids word splitting if the checkout path contains spaces and clears the ShellCheck finding.
Proposed fix
-source ${__dir}/mirror_utils.sh +# shellcheck source=deploy/operator/mirror_utils.sh +source "${__dir}/mirror_utils.sh"🤖 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 `@deploy/operator/setup_hive.sh` at line 6, The helper source statement in setup_hive.sh should quote the path to avoid word splitting when __dir contains spaces. Update the source call that pulls in mirror_utils.sh to use a quoted path, keeping the same __dir variable and sourced helper name so the shellcheck warning is resolved.Source: Linters/SAST tools
🤖 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 `@deploy/operator/mirror_utils.sh`:
- Line 19: The mirror attempt log in the helper that prints "Mirror attempt" is
exposing full command arguments via $*, which can leak internal hostnames and
customer data. Update the logging in the mirror utility to keep only the attempt
counters and the command name, or redact the arguments before printing, so the
mirror call sites do not surface local registry or environment details.
- Around line 21-24: The logging helper is changing the caller’s shell options
by always re-enabling pipefail after the command runs. In the script’s
`mirror_utils.sh` helper around the `"$@" 2>&1 | tee "${log_file}"` block,
capture whether pipefail was set before modifying it, then restore that original
state after recording `PIPESTATUS[0]` so sourced callers keep their prior shell
settings.
---
Nitpick comments:
In `@deploy/operator/capi/deploy_capi_cluster.sh`:
- Line 83: The provider image assignment currently hides failures from
get_image_repository_only because the command substitution is wrapped directly
in export; assign the substitution to PROVIDER_LOCAL_IMAGE first (or otherwise
preserve its exit status) and then export it so errors are not masked. Also
update the workspace URL construction to quote the PWD expansion so the path in
the URL is not split or globbed. Use the existing get_image_repository_only and
PROVIDER_LOCAL_IMAGE / workspace URL setup in deploy_capi_cluster.sh to make the
change.
In `@deploy/operator/setup_hive.sh`:
- Line 6: The helper source statement in setup_hive.sh should quote the path to
avoid word splitting when __dir contains spaces. Update the source call that
pulls in mirror_utils.sh to use a quoted path, keeping the same __dir variable
and sourced helper name so the shellcheck warning is resolved.
In `@internal/bminventory/inventory_ignition_endpoint_unit_test.go`:
- Around line 1-22: The new validation coverage in
TestValidateIgnitionEndpointNormalizesUnbracketedIPv6 is using plain testing.T,
but this package’s tests follow the Ginkgo/Gomega style. Move this case into the
existing bminventory Ginkgo suite and express the same validateIgnitionEndpoint
and IgnitionEndpoint URL normalization assertions with Gomega, keeping the
behavior coverage while matching the surrounding test convention.
In `@internal/host/hostutil/host_utils_ignition_url_test.go`:
- Around line 1-31: The new unbracketed IPv6 custom IgnitionEndpoint scenario is
duplicated between TestGetIgnitionEndpointAndCertUnbracketedIPv6CustomEndpoint
and the Ginkgo case in host_utils_test.go, so remove one of the two tests to
avoid redundant coverage and mixed testing styles in the hostutil package. Keep
the remaining test aligned with GetIgnitionEndpointAndCert behavior and ensure
it still covers the same URL normalization and nil cert expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ffcaec83-1085-4e5d-8bef-fa4f6ae04a39
📒 Files selected for processing (8)
deploy/operator/capi/deploy_capi_cluster.shdeploy/operator/mirror_utils.shdeploy/operator/setup_hive.shinternal/bminventory/inventory.gointernal/bminventory/inventory_ignition_endpoint_unit_test.gointernal/host/hostutil/host_utils.gointernal/host/hostutil/host_utils_ignition_url_test.gointernal/host/hostutil/host_utils_test.go
|
/test edge-e2e-ai-operator-disconnected-capi |
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 213bb6c to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #10570 +/- ##
==========================================
+ Coverage 44.48% 44.51% +0.02%
==========================================
Files 423 423
Lines 73327 73353 +26
==========================================
+ Hits 32622 32652 +30
+ Misses 37798 37794 -4
Partials 2907 2907
🚀 New features to boost your workflow:
|
…validation The edge CAPI disconnected test pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-disconnected-capi started failing after PR openshift#10396 upgraded assisted-service to Go 1.26. Go 1.26 tightened net/url.Parse: URLs with an unbracketed IPv6 host and port are now rejected (urlstrictcolons). Hypershift builds ignition endpoints as host:port without brackets (e.g. fd2e:6f44:5dd8:c956::14:31187), and cluster-api-provider-agent copies that into AgentClusterInstall. The operator then syncs the URL to the backend, where ValidateHTTPFormat fails: parse "https://fd2e:6f44:5dd8:c956::14:31187": invalid port ":6f44:5dd8:c956::14:31187" after host That surfaces as ACI SpecSynced=False (InputError), the worker agent stays insufficient (ignition-downloadable pending), and the test times out waiting for a day-2 worker node. The same IPv6 lab and URL format passed on Go 1.25 (e.g. PR openshift#10319, Jun 2026); the regression correlates with the toolchain bump, not with the OCP 5.0 payload shift. Add NormalizeHTTPURL to bracket IPv6 literal hosts via net.JoinHostPort. Apply it in ValidateHTTPFormat and in parseIgnitionEndpoint so URLs from ACI are normalized before API validation and storage. The root cause is upstream in Hypershift URL construction; this change is a defensive fix on the assisted-service side. A companion fix in cluster-api-provider-agent normalizes the URL when writing ACI.
andrej1991
force-pushed
the
ipv6UrlFormating
branch
from
2adef12 to
fd6c432
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@deploy/operator/capi/deploy_capi_cluster.sh`:
- Line 101: The oc-mirror invocation in deploy_capi_cluster.sh uses an unquoted
$PWD in the --workspace argument, which triggers shellcheck SC2086. Update the
run_mirror_command_with_retry call to quote the workspace path so the shell
treats the full file URL as a single argument; use the existing oc-mirror
command in deploy_capi_cluster.sh as the place to apply the fix.
- Line 83: The PROVIDER_LOCAL_IMAGE export combines command substitution with an
unquoted PROVIDER_IMAGE expansion, triggering shellcheck SC2155/SC2086. Update
the assignment in deploy_capi_cluster.sh so the image repository lookup is
performed safely with PROVIDER_IMAGE quoted inside get_image_repository_only,
and avoid masking the command substitution result by separating the export from
the variable construction if needed; use the existing get_image_repository_only
and PROVIDER_LOCAL_IMAGE symbols to locate the fix.
In `@deploy/operator/setup_hive.sh`:
- Line 6: The source statement in setup_hive.sh uses __dir unquoted, which can
break when the path contains spaces or special characters. Update the source
line to quote the __dir expansion so mirror_utils.sh is loaded safely, and keep
the change localized to the source invocation that references __dir.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79364b9e-4d67-4431-a9ea-0fc727962457
📒 Files selected for processing (11)
deploy/operator/capi/deploy_capi_cluster.shdeploy/operator/mirror_utils.shdeploy/operator/setup_hive.shinternal/bminventory/inventory.gointernal/bminventory/inventory_ignition_endpoint_unit_test.gointernal/controller/controllers/clusterdeployments_controller.gointernal/host/hostutil/host_utils.gointernal/host/hostutil/host_utils_ignition_url_test.gointernal/host/hostutil/host_utils_test.gopkg/validations/validations.gopkg/validations/validations_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/validations/validations_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/bminventory/inventory.go
- internal/host/hostutil/host_utils_test.go
- internal/bminventory/inventory_ignition_endpoint_unit_test.go
- internal/host/hostutil/host_utils_ignition_url_test.go
- internal/host/hostutil/host_utils.go
- internal/controller/controllers/clusterdeployments_controller.go
- deploy/operator/mirror_utils.sh
- pkg/validations/validations.go
| # 3. mirrored CAPI provider agent image to local mirror registry | ||
| if [ ! -z "$PROVIDER_IMAGE" ] | ||
| then | ||
| export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix shellcheck SC2155/SC2086 on PROVIDER_LOCAL_IMAGE assignment.
Command substitution's return value is masked by the export assignment, and ${PROVIDER_IMAGE} is unquoted inside $(...), risking word-splitting/globbing.
🔧 Proposed fix
- export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi"
+ provider_repo=$(get_image_repository_only "${PROVIDER_IMAGE}")
+ export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/${provider_repo}:capi"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/$(get_image_repository_only ${PROVIDER_IMAGE}):capi" | |
| provider_repo=$(get_image_repository_only "${PROVIDER_IMAGE}") | |
| export PROVIDER_LOCAL_IMAGE="${LOCAL_REGISTRY}/${provider_repo}:capi" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 83-83: Declare and assign separately to avoid masking return values.
(SC2155)
[info] 83-83: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@deploy/operator/capi/deploy_capi_cluster.sh` at line 83, The
PROVIDER_LOCAL_IMAGE export combines command substitution with an unquoted
PROVIDER_IMAGE expansion, triggering shellcheck SC2155/SC2086. Update the
assignment in deploy_capi_cluster.sh so the image repository lookup is performed
safely with PROVIDER_IMAGE quoted inside get_image_repository_only, and avoid
masking the command substitution result by separating the export from the
variable construction if needed; use the existing get_image_repository_only and
PROVIDER_LOCAL_IMAGE symbols to locate the fix.
Source: Linters/SAST tools
| __dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| source ${__dir}/common.sh | ||
| source ${__dir}/utils.sh | ||
| source ${__dir}/mirror_utils.sh |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote ${__dir} in the source line.
🔧 Proposed fix
-source ${__dir}/mirror_utils.sh
+source "${__dir}/mirror_utils.sh"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| source ${__dir}/mirror_utils.sh | |
| source "${__dir}/mirror_utils.sh" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 6-6: Not following: ./mirror_utils.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 6-6: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@deploy/operator/setup_hive.sh` at line 6, The source statement in
setup_hive.sh uses __dir unquoted, which can break when the path contains spaces
or special characters. Update the source line to quote the __dir expansion so
mirror_utils.sh is loaded safely, and keep the change localized to the source
invocation that references __dir.
Source: Linters/SAST tools
|
/retest |
andrej1991
force-pushed
the
ipv6UrlFormating
branch
from
fd6c432 to
213bb6c
Compare
2 participants
…validation
The edge CAPI disconnected test
pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-disconnected-capi started failing after PR #10396 upgraded assisted-service to Go 1.26.
Go 1.26 tightened net/url.Parse: URLs with an unbracketed IPv6 host and port are now rejected (urlstrictcolons). Hypershift builds ignition endpoints as host:port without brackets (e.g. fd2e:6f44:5dd8:c956::14:31187), and cluster-api-provider-agent copies that into AgentClusterInstall. The operator then syncs the URL to the backend, where ValidateHTTPFormat fails:
parse "https://fd2e:6f44:5dd8:c956::14:31187": invalid port
":6f44:5dd8:c956::14:31187" after host
That surfaces as ACI SpecSynced=False (InputError), the worker agent stays insufficient (ignition-downloadable pending), and the test times out waiting for a day-2 worker node. The same IPv6 lab and URL format passed on Go 1.25 (e.g. PR #10319, Jun 2026); the regression correlates with the toolchain bump, not with the OCP 5.0 payload shift.
Add NormalizeHTTPURL to bracket IPv6 literal hosts via net.JoinHostPort. Apply it in ValidateHTTPFormat and in parseIgnitionEndpoint so URLs from ACI are normalized before API validation and storage.
The root cause is upstream in Hypershift URL construction; this change is a defensive fix on the assisted-service side. A companion fix in cluster-api-provider-agent normalizes the URL when writing ACI.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit