OCPBUGS-84508: Fix NLB name parsing for EKS Auto Mode hostnames - #8343
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@typeid: This pull request references Jira Issue OCPBUGS-84508, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
📝 WalkthroughWalkthroughA new package-level function 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
/jira refresh |
|
@typeid: This pull request references Jira Issue OCPBUGS-84508, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@typeid: This pull request references Jira Issue OCPBUGS-84508, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go (1)
914-942: Add an empty-hostname edge case to lock parser behavior.Given parser output is persisted into spec, include a case like
hostname: ""→expected: ""(or whatever behavior you want to guarantee) to prevent silent regressions around uninitialized ingress data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go` around lines 914 - 942, Add an explicit empty-hostname test to TestExtractNLBName to lock down parser behavior: in the testCases slice inside TestExtractNLBName, add a case with hostname: "" and expected: "" so extractNLBName("") is asserted to return the empty string; this prevents regressions when persisted spec contains uninitialized/empty ingress hostnames and clearly documents the intended behavior of extractNLBName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go`:
- Line 173: Check and validate the LoadBalancer hostname before
deriving/persisting an NLB name: ensure svc.Status.LoadBalancer != nil,
len(svc.Status.LoadBalancer.Ingress) > 0 and
svc.Status.LoadBalancer.Ingress[0].Hostname is non-empty before calling
extractNLBName; if the hostname is empty, avoid calling extractNLBName and do
not set/persist NetworkLoadBalancerName (or return/error out of the reconcile
path) and log a clear warning. Update the code around the extractNLBName call
and any subsequent assignment to NetworkLoadBalancerName to use this guard so
DescribeLoadBalancers is never invoked with an empty name.
---
Nitpick comments:
In
`@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go`:
- Around line 914-942: Add an explicit empty-hostname test to TestExtractNLBName
to lock down parser behavior: in the testCases slice inside TestExtractNLBName,
add a case with hostname: "" and expected: "" so extractNLBName("") is asserted
to return the empty string; this prevents regressions when persisted spec
contains uninitialized/empty ingress hostnames and clearly documents the
intended behavior of extractNLBName.
🪄 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: c599f7ef-b59b-46ac-9fa4-6ca87ac9dc5e
📒 Files selected for processing (2)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go
|
@typeid: This pull request references Jira Issue OCPBUGS-84508, which is valid. 3 validation(s) were run on this bug
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8343 +/- ##
=======================================
Coverage 36.08% 36.09%
=======================================
Files 767 767
Lines 93486 93493 +7
=======================================
+ Hits 33737 33744 +7
Misses 57042 57042
Partials 2707 2707
🚀 New features to boost your workflow:
|
19b27cc to
8421390
Compare
EKS Auto Mode provisions NLBs via the AWS LB Controller, which generates names containing hyphens (e.g. k8s-ns-svc-hash). The previous parsing split on the first hyphen, extracting only the first segment. Use strings.LastIndex to strip only the AWS-assigned ID suffix, which works correctly for both standard and EKS Auto Mode NLB hostnames. Signed-off-by: Claudio Busse <cbusse@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8421390 to
4e22e73
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go (1)
173-173:⚠️ Potential issue | 🟠 MajorValidate
Ingress[0].Hostnamebefore deriving NLB name.At Line 173, an empty hostname still produces an empty
NetworkLoadBalancerName, which can later fail AWS load balancer lookup by name.💡 Proposed fix
- lbName := extractNLBName(svc.Status.LoadBalancer.Ingress[0].Hostname) + hostname := strings.TrimSpace(svc.Status.LoadBalancer.Ingress[0].Hostname) + if hostname == "" { + r.log.Info("load balancer hostname not provisioned yet") + return ctrl.Result{}, nil + } + lbName := extractNLBName(hostname) + if lbName == "" { + return ctrl.Result{}, fmt.Errorf("failed to extract NLB name from hostname %q", hostname) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go` at line 173, The code calls extractNLBName(...) on svc.Status.LoadBalancer.Ingress[0].Hostname without validating the Ingress slice or Hostname, which can produce an empty lbName and break subsequent AWS lookups; update the reconcile logic to first check that svc.Status.LoadBalancer != nil, len(svc.Status.LoadBalancer.Ingress) > 0 and that svc.Status.LoadBalancer.Ingress[0].Hostname is non-empty before calling extractNLBName, and if any check fails, log a clear error via the controller logger and return/requeue (don’t proceed with using lbName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go`:
- Line 173: The code calls extractNLBName(...) on
svc.Status.LoadBalancer.Ingress[0].Hostname without validating the Ingress slice
or Hostname, which can produce an empty lbName and break subsequent AWS lookups;
update the reconcile logic to first check that svc.Status.LoadBalancer != nil,
len(svc.Status.LoadBalancer.Ingress) > 0 and that
svc.Status.LoadBalancer.Ingress[0].Hostname is non-empty before calling
extractNLBName, and if any check fails, log a clear error via the controller
logger and return/requeue (don’t proceed with using lbName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 18e9634c-912c-484b-91db-4c343f3ff999
📒 Files selected for processing (2)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, typeid 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
|
/retest |
|
/test e2e-aks e2e-aws e2e-aws-upgrade-hypershift-operator e2e-azure-self-managed e2e-kubevirt-aws-ovn-reduced e2e-v2-aws |
|
/verified by @typeid Tested on EKS - names are recognized and cluster installation proceeds with this build instead of hanging on failing endpoint service reconciles. E2E tests here cover that there's no regression: we're able to still recognize the OpenShift NLB names as not being able to would mean clusters in E2Es don't come up. |
openshift-ci-robot
added
the
verified
|
@typeid: This PR has been marked as verified by 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. |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aks
e2e-aws
|
|
/test e2e-aws e2e-azure-self-managed |
|
/test e2e-aws |
|
/test e2e-azure-self-managed |
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-azure-self-managed |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a KubeVirt-hosted guest cluster infrastructure flake — not a code regression from PR #8343. What happened:
Why it's not related to PR #8343:
Why pods were stuck: Recommendations
Evidence
|
|
@typeid: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
@typeid: Jira Issue Verification Checks: Jira Issue OCPBUGS-84508 Jira Issue OCPBUGS-84508 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-05-01-102026 |
6 participants
Summary
PrivateServiceObserverextracts the NLB name from the Kubernetes Service's load balancer hostname to populateAWSEndpointService.Spec.NetworkLoadBalancerName. The current parsing splits on the first-, which fails for NLBs provisioned by the AWS Load Balancer Controller (EKS Auto Mode), where names contain hyphens (e.g.k8s-clusters-kubeapis-db6fee3a62).Background
AWS NLB DNS hostnames follow the format
{name}-{id}.elb.{region}.amazonaws.com, where{name}is the exact value passed toCreateLoadBalancerand{id}is an AWS-assigned hex suffix (AWS docs).Backwards compatibility: The
{id}suffix is always hex (no hyphens), as shown in every AWS API example. This is also structurally required — since{name}may contain hyphens, a hyphenated{id}would make the hostname format ambiguous.LastIndex("-")therefore always lands on the boundary between{name}and{id}, producing the same result asSplit("-")[0]when the name has no hyphens.The two NLB name generation paths:
"a" + serviceUIDwith all hyphens stripped — purely alphanumeric, so both old and new parsing are identical.k8s-{ns}-{svc}-{hash}— always contains hyphens, where the oldSplit("-")[0]extracted onlyk8s.Jira
https://issues.redhat.com/browse/OCPBUGS-84508
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests