Skip to content

CNTRLPLANE-3342: Extract support/netutil package from support/util - #8360

Merged
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
bryan-cox:CNTRLPLANE-3342
Apr 30, 2026
Merged

CNTRLPLANE-3342: Extract support/netutil package from support/util#8360
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
bryan-cox:CNTRLPLANE-3342

Conversation

@bryan-cox

@bryan-cox bryan-cox commented Apr 28, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it:

Extracts network visibility, route management, service publishing, networking, and IP utility functions from the monolithic support/util/ package (632 importers) into a new focused support/netutil/ package (~60 callers).

This improves discoverability by name and bounds the blast radius of network-related changes. The extracted functions form a cohesive cluster covering:

  • visibility: IsPrivateHCP, IsPublicHCP, IsPrivateHC, IsPublicHC, LabelHCPRoutes
  • route: ReconcileExternalRoute, ReconcileInternalRoute, HCPRouteLabel, ShortenName, etc.
  • expose: ServicePublishingStrategyByTypeForHCP, IsLBKAS, IsRouteKAS, UseDedicatedDNS, etc.
  • networking: MachineCIDRs, ServiceCIDRs, KASPodPort, AdvertiseAddress, etc.
  • public: ConnectsThroughInternetToControlplane
  • iputil: IsIPv4CIDR, IsIPv4Address, FirstUsableIP, ResolveDNSHostname, HostFromURL

Clean break — no forwarding functions left in support/util/.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/CNTRLPLANE-3342

Special notes for your reviewer:

  • Two commits: (1) structural file moves + function extraction, (2) caller updates across 65 files
  • Files moved via git mv to preserve history
  • 79 files changed total, net diff is ~zero lines (pure refactoring)
  • No behavioral changes — only package/import changes
  • go build ./..., go test ./support/netutil/..., make lint, and make run-gitlint all pass

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Summary by CodeRabbit

  • Refactor

    • Network-related helpers consolidated into a dedicated netutil package; many controllers and components now use the new networking helpers for public/private cluster, route labeling, and CIDR/host/address decisions.
  • New Features

    • Introduced networking helpers for CIDR/IP checks, first-usable IP, DNS resolution, and host extraction from URLs.
  • Tests

    • Added comprehensive tests for IP/CIDR parsing and URL host extraction.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

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

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

This repository is configured in: LGTM mode

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 28, 2026
@openshift-ci

openshift-ci Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot

openshift-ci-robot commented Apr 28, 2026

Copy link
Copy Markdown

@bryan-cox: This pull request references CNTRLPLANE-3342 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

What this PR does / why we need it:

Extracts network visibility, route management, service publishing, networking, and IP utility functions from the monolithic support/util/ package (632 importers) into a new focused support/netutil/ package (~60 callers).

This improves discoverability by name and bounds the blast radius of network-related changes. The extracted functions form a cohesive cluster covering:

  • visibility: IsPrivateHCP, IsPublicHCP, IsPrivateHC, IsPublicHC, LabelHCPRoutes
  • route: ReconcileExternalRoute, ReconcileInternalRoute, HCPRouteLabel, ShortenName, etc.
  • expose: ServicePublishingStrategyByTypeForHCP, IsLBKAS, IsRouteKAS, UseDedicatedDNS, etc.
  • networking: MachineCIDRs, ServiceCIDRs, KASPodPort, AdvertiseAddress, etc.
  • public: ConnectsThroughInternetToControlplane
  • iputil: IsIPv4CIDR, IsIPv4Address, FirstUsableIP, ResolveDNSHostname, HostFromURL

Clean break — no forwarding functions left in support/util/.

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/CNTRLPLANE-3342

Special notes for your reviewer:

  • Two commits: (1) structural file moves + function extraction, (2) caller updates across 65 files
  • Files moved via git mv to preserve history
  • 79 files changed total, net diff is ~zero lines (pure refactoring)
  • No behavioral changes — only package/import changes
  • go build ./..., go test ./support/netutil/..., make lint, and make run-gitlint all pass

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-ci Bot added do-not-merge/needs-area needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 28, 2026
@coderabbitai

coderabbitai Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This pull request refactors network-related utility functions by establishing a new support/netutil package and migrating network-specific helpers (IPv4/CIDR detection, DNS resolution, URL parsing, routing, visibility, and service-publishing logic) from the general support/util package. All call sites across the control-plane-operator, hypershift-operator, shared ingress components, and e2e tests are systematically updated to import and invoke these functions from support/netutil instead of support/util. The original implementations are removed from support/util and repackaged under the new netutil namespace, with no changes to function signatures, behavior, or control flow.

🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the main change: extracting network-related functionality from support/util into a new support/netutil package.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The PR does not introduce or modify any Ginkgo-style tests with dynamic names. All modified tests use standard Go testing with table-driven patterns and static string constants for test names.
Test Structure And Quality ✅ Passed PR adds standard Go unit tests with table-driven patterns and Gomega assertions, updates e2e test imports; no Ginkgo tests introduced or modified.
Microshift Test Compatibility ✅ Passed This PR is a pure refactoring that extracts networking functionality from support/util into a new support/netutil package. No new Ginkgo e2e tests are added.
Single Node Openshift (Sno) Test Compatibility ✅ Passed This PR does not introduce any new Ginkgo e2e tests. The e2e test files have minimal changes to reference netutil instead of util, and the new test file is a standard Go unit test using the testing package, not a Ginkgo e2e test.
Topology-Aware Scheduling Compatibility ✅ Passed Pure refactoring extracting network-utility functions into new support/netutil package with file moves and import updates across 65 files; no behavioral changes.
Ote Binary Stdout Contract ✅ Passed PR introduces netutil package by extracting network utilities without adding process-level stdout-writing code. All new files contain utility functions only, tests use standard patterns, and existing stdout writes are intended tool behavior.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR does not add new Ginkgo e2e tests. Modified e2e test files use traditional Go unit/integration tests with *testing.T. New unit tests in support/netutil/iputil_test.go have comprehensive IPv6 coverage and do not require external connectivity.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci openshift-ci Bot added area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/platform/aws PR/issue for AWS (AWSPlatform) platform area/platform/gcp PR/issue for GCP (GCPPlatform) platform area/platform/openstack PR/issue for OpenStack (OpenStackPlatform) platform area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels Apr 28, 2026
@openshift-ci

openshift-ci Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bryan-cox

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

The pull request process is described here

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

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2026
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2026
@bryan-cox
bryan-cox marked this pull request as ready for review April 28, 2026 18:32
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2026
@openshift-ci
openshift-ci Bot requested review from Nirshal and enxebre April 28, 2026 18:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
support/pki/kas.go (1)

14-19: ⚠️ Potential issue | 🟡 Minor

Use serviceCIDR in the error message.

serviceIP is still empty when FirstUsableIP returns an error, so the diagnostic loses the CIDR that actually failed.

Suggested fix
-			return nil, nil, fmt.Errorf("cannot get the first usable IP from CIDR %s: %w", serviceIP, err)
+			return nil, nil, fmt.Errorf("cannot get the first usable IP from CIDR %s: %w", serviceCIDR, err)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@support/pki/kas.go` around lines 14 - 19, The error message inside the loop
that calls netutil.FirstUsableIP currently prints serviceIP (which will be empty
on error) instead of the failing CIDR; update the fmt.Errorf call in the loop
that iterates serviceCIDRs (where serviceIP, err :=
netutil.FirstUsableIP(serviceCIDR) is used) to include serviceCIDR in the
formatted message so the diagnostic reports the actual CIDR that caused the
error.
control-plane-operator/controllers/hostedcontrolplane/kas/service.go (1)

256-260: ⚠️ Potential issue | 🔴 Critical

Pass the HCP name to netutil.ReconcileInternalRoute instead of an empty string.

The call at line 260 currently passes "" for the HCP name parameter, but the function signature expects hcName string. The wrapper function receives owner *metav1.OwnerReference and already uses owner.Name at line 258, so it should be passed here as well. Other similar functions throughout the codebase (oauth/route.go, ignitionserver/route.go, metrics_proxy/route.go) correctly pass the HCP name.

🛠️ Suggested fix
 func ReconcileInternalRoute(route *routev1.Route, owner *metav1.OwnerReference) error {
 	util.EnsureOwnerRef(route, owner)
 	route.Spec.Host = fmt.Sprintf("api.%s.hypershift.local", owner.Name)
 	// Assumes owner is the HCP
-	return netutil.ReconcileInternalRoute(route, "", manifests.KubeAPIServerService("").Name)
+	return netutil.ReconcileInternalRoute(route, owner.Name, manifests.KubeAPIServerService("").Name)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@control-plane-operator/controllers/hostedcontrolplane/kas/service.go` around
lines 256 - 260, The wrapper ReconcileInternalRoute sets route.Spec.Host using
owner.Name but calls netutil.ReconcileInternalRoute with an empty HCP name;
change the call to pass owner.Name instead of "" so the hcName parameter
receives the hosted control plane name. Locate the call to
netutil.ReconcileInternalRoute in ReconcileInternalRoute and replace the empty
string argument with owner.Name (keeping manifests.KubeAPIServerService("").Name
unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@support/netutil/iputil.go`:
- Around line 65-85: HostFromURL currently returns bracketed IPv6 hosts like
"[::1]" when no port is present; update HostFromURL to strip surrounding
brackets for bare IPv6 addresses before returning. Specifically, after obtaining
hostPort from parsedURL.Host and after the hasPortRegex check that determines
there is no port, detect if hostPort starts with "[" and ends with "]" and if so
remove the leading "[" and trailing "]" (return the unbracketed value); keep
existing error handling for parsing and SplitHostPort, and continue to validate
hostName where SplitHostPort is used. Ensure this fix is applied in the
HostFromURL function (and consider hasPortRegex logic) so callers receive
unbracketed IPv6 addresses for DNS/dial usage.

---

Outside diff comments:
In `@control-plane-operator/controllers/hostedcontrolplane/kas/service.go`:
- Around line 256-260: The wrapper ReconcileInternalRoute sets route.Spec.Host
using owner.Name but calls netutil.ReconcileInternalRoute with an empty HCP
name; change the call to pass owner.Name instead of "" so the hcName parameter
receives the hosted control plane name. Locate the call to
netutil.ReconcileInternalRoute in ReconcileInternalRoute and replace the empty
string argument with owner.Name (keeping manifests.KubeAPIServerService("").Name
unchanged).

In `@support/pki/kas.go`:
- Around line 14-19: The error message inside the loop that calls
netutil.FirstUsableIP currently prints serviceIP (which will be empty on error)
instead of the failing CIDR; update the fmt.Errorf call in the loop that
iterates serviceCIDRs (where serviceIP, err :=
netutil.FirstUsableIP(serviceCIDR) is used) to include serviceCIDR in the
formatted message so the diagnostic reports the actual CIDR that caused the
error.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: dcf16027-3f6f-4ca8-87fa-7a01368ca400

📥 Commits

Reviewing files that changed from the base of the PR and between cc51a9c and 56b45c8.

📒 Files selected for processing (79)
  • cmd/cluster/core/dump.go
  • cmd/kubeconfig/create.go
  • cmd/oadp/common.go
  • cmd/oadp/restore.go
  • control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
  • control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go
  • control-plane-operator/controllers/hostedcontrolplane/cloud/openstack/providerconfig.go
  • control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
  • control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
  • control-plane-operator/controllers/hostedcontrolplane/infra/infra.go
  • control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go
  • control-plane-operator/controllers/hostedcontrolplane/ingress/router.go
  • control-plane-operator/controllers/hostedcontrolplane/ingress/router_test.go
  • control-plane-operator/controllers/hostedcontrolplane/kas/params.go
  • control-plane-operator/controllers/hostedcontrolplane/kas/service.go
  • control-plane-operator/controllers/hostedcontrolplane/multus_disable_test.go
  • control-plane-operator/controllers/hostedcontrolplane/oauth/route.go
  • control-plane-operator/controllers/hostedcontrolplane/pki/params.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/openstack/config.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cno/component.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/route.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/oapi/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/config.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/config_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/reconcile.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
  • control-plane-operator/main.go
  • hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
  • hypershift-operator/controllers/hostedcluster/network_policies.go
  • hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go
  • hypershift-operator/controllers/nodepool/manifests.go
  • hypershift-operator/controllers/nodepool/nto.go
  • hypershift-operator/controllers/nodepool/nto_test.go
  • hypershift-operator/controllers/sharedingress/sharedingress_controller.go
  • hypershift-operator/main.go
  • konnectivity-https-proxy/cmd.go
  • sharedingress-config-generator/config.go
  • sharedingress-config-generator/config_test.go
  • sharedingress-config-generator/controller.go
  • support/conditions/conditions.go
  • support/globalconfig/infrastructure.go
  • support/globalconfig/installconfig.go
  • support/globalconfig/network.go
  • support/netutil/expose.go
  • support/netutil/expose_test.go
  • support/netutil/iputil.go
  • support/netutil/iputil_test.go
  • support/netutil/networking.go
  • support/netutil/networking_test.go
  • support/netutil/public.go
  • support/netutil/public_test.go
  • support/netutil/route.go
  • support/netutil/route_test.go
  • support/netutil/visibility.go
  • support/netutil/visibility_test.go
  • support/pki/kas.go
  • support/upsert/apply.go
  • support/upsert/apply_test.go
  • support/util/util.go
  • support/util/util_test.go
  • test/e2e/nodepool_mirrorconfigs_test.go
  • test/e2e/nodepool_nto_performanceprofile_test.go
  • test/e2e/util/hypershift_framework.go
  • test/e2e/util/oauth.go
  • test/e2e/util/util.go
💤 Files with no reviewable changes (2)
  • support/util/util_test.go
  • support/util/util.go

Comment thread support/netutil/iputil.go
@codecov

codecov Bot commented Apr 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.91371% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.42%. Comparing base (5eaee74) to head (c55ad86).
⚠️ Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
support/netutil/iputil.go 72.72% 13 Missing and 2 partials ⚠️
...ator/controllers/hostedcontrolplane/kas/service.go 28.57% 5 Missing ⚠️
...ontrollers/hostedcontrolplane/v2/kas/kubeconfig.go 0.00% 5 Missing ⚠️
...ator/controllers/hostedcontrolplane/oauth/route.go 0.00% 4 Missing ⚠️
...ollers/awsprivatelink/awsprivatelink_controller.go 25.00% 3 Missing ⚠️
...ostedcontrolplane/hostedcontrolplane_controller.go 57.14% 3 Missing ⚠️
...rator/controllers/hostedcontrolplane/pki/params.go 0.00% 3 Missing ⚠️
...ontrollers/hostedcontrolplane/v2/cno/deployment.go 0.00% 3 Missing ⚠️
...ontrollers/hostedcontrolplane/v2/kas/deployment.go 25.00% 3 Missing ⚠️
...trollers/sharedingress/sharedingress_controller.go 0.00% 3 Missing ⚠️
... and 24 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8360   +/-   ##
=======================================
  Coverage   36.42%   36.42%           
=======================================
  Files         765      766    +1     
  Lines       93302    93302           
=======================================
  Hits        33981    33981           
  Misses      56606    56606           
  Partials     2715     2715           
Files with missing lines Coverage Δ
cmd/oadp/common.go 89.65% <100.00%> (ø)
cmd/oadp/restore.go 36.33% <100.00%> (ø)
...stedcontrolplane/cloud/openstack/providerconfig.go 45.65% <100.00%> (ø)
...ne/v2/cloud_controller_manager/openstack/config.go 33.82% <100.00%> (ø)
...rs/hostedcontrolplane/v2/ignitionserver/service.go 90.32% <100.00%> (ø)
...tedcontrolplane/v2/ignitionserver_proxy/service.go 100.00% <100.00%> (ø)
...or/controllers/hostedcontrolplane/v2/kas/params.go 88.80% <100.00%> (ø)
...ontrollers/hostedcontrolplane/v2/kcm/deployment.go 91.30% <100.00%> (ø)
...ostedcontrolplane/v2/oauth_apiserver/deployment.go 100.00% <100.00%> (ø)
...trollers/hostedcontrolplane/v2/router/util/util.go 100.00% <100.00%> (ø)
... and 46 more
Flag Coverage Δ
cmd-support 30.37% <66.66%> (ø)
cpo-hostedcontrolplane 37.08% <59.57%> (ø)
cpo-other 35.69% <54.54%> (ø)
hypershift-operator 47.88% <52.63%> (ø)
other 27.76% <50.00%> (ø)

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

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

@bryan-cox

Copy link
Copy Markdown
Member Author

/verified by UT and e2e

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Apr 28, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@bryan-cox: This PR has been marked as verified by UT and e2e.

Details

In response to this:

/verified by UT and e2e

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
support/netutil/iputil.go (1)

74-76: ⚠️ Potential issue | 🟡 Minor

Return unbracketed host for bare IPv6 URLs.

At Line 74, the no-port path returns hostPort verbatim, so https://[::1] yields [::1] instead of ::1. This is the same unresolved edge case previously raised and can break hostname/IP consumers.

Proposed fix
 	if !hasPortRegex.MatchString(hostPort) {
+		if len(hostPort) >= 2 && hostPort[0] == '[' && hostPort[len(hostPort)-1] == ']' {
+			return hostPort[1 : len(hostPort)-1], nil
+		}
 		return hostPort, nil
 	}
In Go's net/url package, what do URL.Host and URL.Hostname() return for:
1) https://[::1]
2) https://[::1]:8443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@support/netutil/iputil.go` around lines 74 - 76, The no-port branch currently
returns hostPort verbatim (when hasPortRegex fails), which leaves bracketed IPv6
hosts like "[::1]" unchanged; update that branch so when hostPort starts with
'[' and ends with ']' you strip the surrounding brackets and return the inner
address (e.g., "::1") instead. Locate the code using hasPortRegex and hostPort
in iputil.go and add a simple check using strings.HasPrefix(hostPort, "[") &&
strings.HasSuffix(hostPort, "]") to return hostPort[1:len(hostPort)-1],
otherwise return hostPort as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@support/netutil/iputil.go`:
- Around line 74-76: The no-port branch currently returns hostPort verbatim
(when hasPortRegex fails), which leaves bracketed IPv6 hosts like "[::1]"
unchanged; update that branch so when hostPort starts with '[' and ends with ']'
you strip the surrounding brackets and return the inner address (e.g., "::1")
instead. Locate the code using hasPortRegex and hostPort in iputil.go and add a
simple check using strings.HasPrefix(hostPort, "[") &&
strings.HasSuffix(hostPort, "]") to return hostPort[1:len(hostPort)-1],
otherwise return hostPort as before.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 4d2b4263-fdc8-45e0-9a2a-615b828ea4fa

📥 Commits

Reviewing files that changed from the base of the PR and between 56b45c8 and 0ab199f.

📒 Files selected for processing (79)
  • cmd/cluster/core/dump.go
  • cmd/kubeconfig/create.go
  • cmd/oadp/common.go
  • cmd/oadp/restore.go
  • control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
  • control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go
  • control-plane-operator/controllers/hostedcontrolplane/cloud/openstack/providerconfig.go
  • control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
  • control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
  • control-plane-operator/controllers/hostedcontrolplane/infra/infra.go
  • control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go
  • control-plane-operator/controllers/hostedcontrolplane/ingress/router.go
  • control-plane-operator/controllers/hostedcontrolplane/ingress/router_test.go
  • control-plane-operator/controllers/hostedcontrolplane/kas/params.go
  • control-plane-operator/controllers/hostedcontrolplane/kas/service.go
  • control-plane-operator/controllers/hostedcontrolplane/multus_disable_test.go
  • control-plane-operator/controllers/hostedcontrolplane/oauth/route.go
  • control-plane-operator/controllers/hostedcontrolplane/pki/params.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/openstack/config.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cno/component.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/route.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/oapi/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/config.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/config_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/reconcile.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
  • control-plane-operator/main.go
  • hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
  • hypershift-operator/controllers/hostedcluster/network_policies.go
  • hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go
  • hypershift-operator/controllers/nodepool/manifests.go
  • hypershift-operator/controllers/nodepool/nto.go
  • hypershift-operator/controllers/nodepool/nto_test.go
  • hypershift-operator/controllers/sharedingress/sharedingress_controller.go
  • hypershift-operator/main.go
  • konnectivity-https-proxy/cmd.go
  • sharedingress-config-generator/config.go
  • sharedingress-config-generator/config_test.go
  • sharedingress-config-generator/controller.go
  • support/conditions/conditions.go
  • support/globalconfig/infrastructure.go
  • support/globalconfig/installconfig.go
  • support/globalconfig/network.go
  • support/netutil/expose.go
  • support/netutil/expose_test.go
  • support/netutil/iputil.go
  • support/netutil/iputil_test.go
  • support/netutil/networking.go
  • support/netutil/networking_test.go
  • support/netutil/public.go
  • support/netutil/public_test.go
  • support/netutil/route.go
  • support/netutil/route_test.go
  • support/netutil/visibility.go
  • support/netutil/visibility_test.go
  • support/pki/kas.go
  • support/upsert/apply.go
  • support/upsert/apply_test.go
  • support/util/util.go
  • support/util/util_test.go
  • test/e2e/nodepool_mirrorconfigs_test.go
  • test/e2e/nodepool_nto_performanceprofile_test.go
  • test/e2e/util/hypershift_framework.go
  • test/e2e/util/oauth.go
  • test/e2e/util/util.go
💤 Files with no reviewable changes (2)
  • support/util/util_test.go
  • support/util/util.go
✅ Files skipped from review due to trivial changes (43)
  • support/netutil/public.go
  • support/netutil/networking.go
  • support/netutil/visibility.go
  • support/netutil/visibility_test.go
  • support/netutil/networking_test.go
  • support/netutil/route.go
  • support/globalconfig/installconfig.go
  • support/netutil/expose_test.go
  • support/netutil/public_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/config.go
  • support/netutil/route_test.go
  • test/e2e/nodepool_mirrorconfigs_test.go
  • control-plane-operator/controllers/hostedcontrolplane/cloud/openstack/providerconfig.go
  • support/netutil/expose.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/openstack/config.go
  • konnectivity-https-proxy/cmd.go
  • hypershift-operator/controllers/nodepool/manifests.go
  • control-plane-operator/controllers/hostedcontrolplane/kas/params.go
  • cmd/oadp/restore.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/reconcile.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/service.go
  • support/conditions/conditions.go
  • support/pki/kas.go
  • control-plane-operator/controllers/hostedcontrolplane/ingress/router_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go
  • support/globalconfig/infrastructure.go
  • control-plane-operator/controllers/hostedcontrolplane/multus_disable_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver_proxy/service.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/config_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.go
  • sharedingress-config-generator/config.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/route.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/router/util/util.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
  • control-plane-operator/controllers/hostedcontrolplane/infra/infra_test.go
  • test/e2e/util/hypershift_framework.go
  • control-plane-operator/controllers/hostedcontrolplane/oauth/route.go
  • control-plane-operator/controllers/hostedcontrolplane/pki/params.go
  • hypershift-operator/controllers/nodepool/nto_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go
  • support/upsert/apply_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
  • control-plane-operator/controllers/hostedcontrolplane/v2/oapi/deployment.go
  • sharedingress-config-generator/config_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/cno/component.go
  • test/e2e/util/oauth.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/params.go
  • support/upsert/apply.go
  • test/e2e/nodepool_nto_performanceprofile_test.go
  • sharedingress-config-generator/controller.go
  • hypershift-operator/controllers/nodepool/nto.go
  • cmd/cluster/core/dump.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/route.go
  • support/globalconfig/network.go
  • cmd/kubeconfig/create.go
  • control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go
  • control-plane-operator/main.go
  • control-plane-operator/controllers/hostedcontrolplane/kas/service.go
  • control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
  • control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
  • test/e2e/util/util.go
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go

bryan-cox and others added 2 commits April 30, 2026 07:41
Move network visibility, route management, service publishing, networking,
and IP utility functions into a new cohesive support/netutil package.

Files moved via git mv: visibility.go, route.go, expose.go, networking.go,
public.go and their corresponding test files.
Functions extracted from util.go into new iputil.go: IsIPv4CIDR,
IsIPv4Address, FirstUsableIP, ResolveDNSHostname, HostFromURL.
Tests extracted from util_test.go into new iputil_test.go.

Signed-off-by: Bryan Cox <brcox@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update all callers across the codebase to import and reference symbols
from the new support/netutil package instead of support/util.

Files that only used extracted symbols have their support/util import
removed entirely. Files using both extracted and remaining symbols
retain both imports.

Signed-off-by: Bryan Cox <brcox@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Apr 30, 2026
@enxebre

enxebre commented Apr 30, 2026

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aks-4-22
/test e2e-aws-4-22
/test e2e-aks
/test e2e-aws
/test e2e-aws-upgrade-hypershift-operator
/test e2e-azure-self-managed
/test e2e-kubevirt-aws-ovn-reduced
/test e2e-v2-aws

@bryan-cox

Copy link
Copy Markdown
Member Author

/verified by UT & e2e

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Apr 30, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@bryan-cox: This PR has been marked as verified by UT & e2e.

Details

In response to this:

/verified by UT & e2e

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.

@cwbotbot

cwbotbot commented Apr 30, 2026

Copy link
Copy Markdown

Test Results

e2e-aks

e2e-aws

@hypershift-jira-solve-ci

Copy link
Copy Markdown
Contributor

AI Test Failure Analysis

Job: pull-ci-openshift-hypershift-main-e2e-aws | Build: 2049818635194601472 | Cost: $5.096510499999999 | Failed step: hypershift-aws-run-e2e-nested

View full analysis report


Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6

@bryan-cox

Copy link
Copy Markdown
Member Author

/test e2e-aws

@hypershift-jira-solve-ci

Copy link
Copy Markdown
Contributor

AI Test Failure Analysis

Job: pull-ci-openshift-hypershift-main-e2e-aws | Build: 2049871249047293952 | Cost: $2.3446862000000004 | Failed step: hypershift-aws-run-e2e-nested

View full analysis report


Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6

@bryan-cox

Copy link
Copy Markdown
Member Author

/test e2e-aws

@bryan-cox

Copy link
Copy Markdown
Member Author

/override "ci/prow/verify-workflows"

@openshift-ci

openshift-ci Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/verify-workflows

Details

In response to this:

/override "ci/prow/verify-workflows"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci

openshift-ci Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@bryan-cox: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot
openshift-merge-bot Bot merged commit afd63bd into openshift:main Apr 30, 2026
43 checks passed
@bryan-cox
bryan-cox deleted the CNTRLPLANE-3342 branch April 30, 2026 22:54
@hypershift-jira-solve-ci

hypershift-jira-solve-ci Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Test Failure Analysis Complete

Job Information

Test Failure Analysis

Error

MISSING: .github/workflows/codespell-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/cpo-container-sync-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/docs-build-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/envtest-kube-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/envtest-ocp-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/gitlint-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/lint-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/test-reusable.yaml exists on main but not on this branch.
MISSING: .github/workflows/verify-reusable.yaml exists on main but not on this branch.
OUTDATED: .github/workflows/envtest-kube.yaml has been updated on main since this branch diverged.
OUTDATED: .github/workflows/envtest-ocp.yaml has been updated on main since this branch diverged.

Rebase your branch on main: git fetch upstream main && git rebase upstream/main

Summary

The verify-workflows CI check failed because PR #8360's branch was missing 9 *-reusable.yaml GitHub Actions workflow files and had 2 outdated workflow files compared to main. These reusable workflow files were introduced to main by a separate PR #8386 ("CNTRLPLANE-3352: Add reusable GHA workflow definitions", merged at 20:29:38 UTC) after PR #8360's branch had already diverged from main. When Tide re-triggered the CI job at 21:22:11 UTC, the verify-workflows script detected the branch was out of date and failed. This is a stale-branch issue, not a product bug. The PR was subsequently merged at 22:14:42 UTC regardless.

Root Cause

The root cause is a stale PR branch — a race condition between two PRs from the same author (bryan-cox):

  1. PR CNTRLPLANE-3342: Extract support/netutil package from support/util #8360 (this PR) branched from main at commit 5eaee747fb5 (merged 2026-04-30T05:31:19Z), before the reusable workflow files existed.
  2. PR CNTRLPLANE-3352: Add reusable GHA workflow definitions #8386 ("CNTRLPLANE-3352: Add reusable GHA workflow definitions") merged to main at 20:29:38 UTC, adding 9 new *-reusable.yaml files under .github/workflows/ and updating envtest-kube.yaml and envtest-ocp.yaml.
  3. Tide then re-tested PR CNTRLPLANE-3342: Extract support/netutil package from support/util #8360 against the updated main (base SHA 5e3afb8557b = merge commit of PR CNTRLPLANE-3352: Add reusable GHA workflow definitions #8386) at 21:22:11 UTC.
  4. The verify-workflows CI step compares all .github/workflows/ files on the PR branch (HEAD^2) against the current main (FETCH_HEAD). Since the PR branch was never rebased, it was missing all 9 newly-added *-reusable.yaml files and had stale versions of envtest-kube.yaml and envtest-ocp.yaml.

The verify-workflows check is working as designed — it exists specifically to catch this situation where a PR branch has drifted from main with respect to GitHub Actions workflow files. The fix is simply to rebase the PR branch on the latest main.

Recommendations
Evidence
Evidence Detail
Failing step verify-workflows (container test, exit code 1)
PR branch HEAD c55ad86bc27e71c3b3bbbc7ef17b02a09ceacd05
Main at CI time 5e3afb8557b05cd735d969f62be6c35f283f578a (merge of PR #8386)
Merge-base (branch divergence point) 5eaee747fb5e2d37cda2d7caa796526eefbed01b
PR #8386 merge time 2026-04-30T20:29:38Z (added 9 reusable workflow files)
CI job start time 2026-04-30T21:22:11Z (52 minutes after #8386 merged)
Missing files (9) codespell-reusable.yaml, cpo-container-sync-reusable.yaml, docs-build-reusable.yaml, envtest-kube-reusable.yaml, envtest-ocp-reusable.yaml, gitlint-reusable.yaml, lint-reusable.yaml, test-reusable.yaml, verify-reusable.yaml
Outdated files (2) envtest-kube.yaml, envtest-ocp.yaml
PR #8360 merged despite failure 2026-04-30T22:14:42Z
verify-workflows script source ci-operator/config/openshift/hypershift/openshift-hypershift-main.yaml (inline commands block)

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/platform/aws PR/issue for AWS (AWSPlatform) platform area/platform/gcp PR/issue for GCP (GCPPlatform) platform area/platform/openstack PR/issue for OpenStack (OpenStackPlatform) platform area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants