OCPCLOUD-3215: Detect IPv6 clusters and apply correct policy#30938
OCPCLOUD-3215: Detect IPv6 clusters and apply correct policy#30938nrb wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@nrb: This pull request references OCPCLOUD-3215 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 "4.22.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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFetches the cluster Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nrb 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 |
| out, err := oc.AsAdmin().WithoutNamespace().Run("create").Args("-n", oc.Namespace(), "service", "loadbalancer", svcName, "--tcp=80:8080").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to create lb service") | ||
| o.Expect(out).To(o.ContainSubstring("service/" + svcName + " created")) | ||
| defer oc.WithoutNamespace().AsAdmin().Run("delete").Args("service", fmt.Sprintf("%s/%s", oc.Namespace(), svcName), "--ignore-not-found").Execute() |
There was a problem hiding this comment.
This change was because oc delete service loadbalancer actually isn't a valid command; it would work before because of the --ignore-not-found and namespace teardown, but this cleans it up more quickly.
nrb
force-pushed
the
ccm-dual-stack-change
branch
from
6eeb44a to
a5a2a3a
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/cloud_controller_manager/ccm.go`:
- Around line 102-106: The code dereferences infra.Status.PlatformStatus without
checking for nil which can panic; update the isIPv6Primary computation to first
verify infra.Status.PlatformStatus is non-nil (and then
infra.Status.PlatformStatus.AWS is non-nil) before reading .AWS.IPFamily—e.g.
check infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.AWS !=
nil && infra.Status.PlatformStatus.AWS.IPFamily ==
configv1.DualStackIPv6Primary, using the existing
infra/AdminConfigClient/ConfigV1/Infrastructures().Get call and the
isIPv6Primary variable to locate where to apply the guard.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b91f067-e4cd-453b-85f0-db8593c8274c
📒 Files selected for processing (1)
test/extended/cloud_controller_manager/ccm.go
|
Scheduling required tests: |
nrb
force-pushed
the
ccm-dual-stack-change
branch
from
a5a2a3a to
afa7cb5
Compare
|
Scheduling required tests: |
| name: %s | ||
| spec: | ||
| type: LoadBalancer | ||
| ipFamilyPolicy: PreferDualStack |
There was a problem hiding this comment.
Should this be RequireDualStack?
There was a problem hiding this comment.
Yep, you're right, that's what we'd agreed on for IPv6 primary.
We don't have that captured in our API and docs are not yet written. I guess the canonical place for us to reference would be the enhancement proposal.
nrb
force-pushed
the
ccm-dual-stack-change
branch
from
afa7cb5 to
d7bae29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/cloud_controller_manager/ccm.go`:
- Line 131: The oc create invocation is passing "-n" before the resource type so
the literal "service" token is being parsed as the namespace; update the
Args(...) call in test/extended/cloud_controller_manager/ccm.go (the
oc.AsAdmin().WithoutNamespace().Run("create").Args(...) call) so the resource
type and name come before the -n flag — e.g. supply "service", "loadbalancer",
svcName, then "-n", "service", "--tcp=80:8080" — ensuring the create subcommand
receives the correct resource type and name.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f75e13de-9b50-4657-9ca6-7b18e2f10c56
📒 Files selected for processing (1)
test/extended/cloud_controller_manager/ccm.go
|
Scheduling required tests: |
Dual stack load balancers in Kubernetes _must_ specify an IP family policy, or else it will be set to `SingleStack`. On AWS, IPv6 single stack is not supported, so update the test to reflect the new setting. Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com> Assisted-By: Claude Sonnet 4.6 Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
nrb
force-pushed
the
ccm-dual-stack-change
branch
from
d7bae29 to
3a77c41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/cloud_controller_manager/ccm.go`:
- Around line 111-133: The create/delete calls use WithoutNamespace() so the
commands run in the default admin namespace while later lookup uses
oc.Namespace(); update the three call sites (the deferred delete using
oc.WithoutNamespace().AsAdmin().Run("delete").Args(...).Execute(), the YAML
create using oc.AsAdmin().WithoutNamespace().Run("create").Args("-f",
"-").InputString(...).Execute(), and the loadbalancer create using
oc.AsAdmin().WithoutNamespace().Run("create").Args("service", "loadbalancer",
...).Output()) to explicitly target the test namespace by replacing
WithoutNamespace() with Namespace(oc.Namespace()) (or by adding the namespace
via Args("-n", oc.Namespace(), ...)) so creation, deletion, and lookup all use
the same namespace.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b95c9f60-dc1e-4b27-8c8c-008090770cdc
📒 Files selected for processing (1)
test/extended/cloud_controller_manager/ccm.go
| defer oc.WithoutNamespace().AsAdmin().Run("delete").Args("service", svcName, "--ignore-not-found").Execute() | ||
| if isIPv6Primary { | ||
|
|
||
| // IPv6 cannot be single stack on AWS, but kube will default an empty spec.ipFamilyPolicy to SingleStack. | ||
| svcYAML := fmt.Sprintf(`apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: %s | ||
| spec: | ||
| type: LoadBalancer | ||
| ipFamilyPolicy: RequireDualStack | ||
| ports: | ||
| - port: 80 | ||
| targetPort: 8080 | ||
| protocol: TCP | ||
| `, svcName) | ||
| err = oc.AsAdmin().WithoutNamespace().Run("create").Args("-f", "-").InputString(svcYAML).Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to create lb service") | ||
| } else { | ||
| // single stack or IPv4 primary | ||
| out, err := oc.AsAdmin().WithoutNamespace().Run("create").Args("service", "loadbalancer", svcName, "--tcp=80:8080").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to create lb service") | ||
| o.Expect(out).To(o.ContainSubstring("service/" + svcName + " created")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect exutil.CLI namespace behavior (constructor, WithoutNamespace, Run).
rg -n --type=go -C4 'type CLI struct|func NewCLI\(|func \([^)]+CLI\) WithoutNamespace\(|func \([^)]+CLI\) Run\(' test/extended/util
# 2) Inspect this test's create/delete/get namespace usage together.
cat -n test/extended/cloud_controller_manager/ccm.go | sed -n '96,145p'Repository: openshift/origin
Length of output: 7386
🏁 Script executed:
# Get the full Run() function implementation
rg -n --type=go -A 30 'func \(c \*CLI\) Run\(commands' test/extended/util/client.go | head -50Repository: openshift/origin
Length of output: 1317
🏁 Script executed:
# Find getLoadBalancerExternalIP function
rg -n --type=go -B 2 -A 15 'func getLoadBalancerExternalIP' test/extended/cloud_controller_manager/ccm.goRepository: openshift/origin
Length of output: 801
🏁 Script executed:
# Check how namespace flag is injected in the command construction
rg -n --type=go 'withoutNamespace' test/extended/util/client.go | head -20Repository: openshift/origin
Length of output: 289
Add explicit namespace to create/delete operations to match lookup behavior.
Service creation and deletion at lines 111, 127, and 131 use WithoutNamespace() without specifying -n <namespace>, causing the commands to operate in an unspecified namespace (kubectl default). However, the service lookup at line 137 explicitly uses oc.Namespace(). If the admin context default namespace differs from the test namespace, the service may be created/deleted in one namespace and looked up in another, causing test failures or timeouts.
Suggested fix
g.By("Create a cluster scope load balancer")
svcName := "test-lb"
- defer oc.WithoutNamespace().AsAdmin().Run("delete").Args("service", svcName, "--ignore-not-found").Execute()
+ ns := oc.Namespace()
+ defer oc.WithoutNamespace().AsAdmin().Run("delete").Args("service", svcName, "-n", ns, "--ignore-not-found").Execute()
if isIPv6Primary {
@@
- err = oc.AsAdmin().WithoutNamespace().Run("create").Args("-f", "-").InputString(svcYAML).Execute()
+ err = oc.AsAdmin().WithoutNamespace().Run("create").Args("-n", ns, "-f", "-").InputString(svcYAML).Execute()
o.Expect(err).NotTo(o.HaveOccurred(), "failed to create lb service")
} else {
// single stack or IPv4 primary
- out, err := oc.AsAdmin().WithoutNamespace().Run("create").Args("service", "loadbalancer", svcName, "--tcp=80:8080").Output()
+ out, err := oc.AsAdmin().WithoutNamespace().Run("create").Args("-n", ns, "service", "loadbalancer", svcName, "--tcp=80:8080").Output()
o.Expect(err).NotTo(o.HaveOccurred(), "failed to create lb service")
o.Expect(out).To(o.ContainSubstring("service/" + svcName + " created"))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/cloud_controller_manager/ccm.go` around lines 111 - 133, The
create/delete calls use WithoutNamespace() so the commands run in the default
admin namespace while later lookup uses oc.Namespace(); update the three call
sites (the deferred delete using
oc.WithoutNamespace().AsAdmin().Run("delete").Args(...).Execute(), the YAML
create using oc.AsAdmin().WithoutNamespace().Run("create").Args("-f",
"-").InputString(...).Execute(), and the loadbalancer create using
oc.AsAdmin().WithoutNamespace().Run("create").Args("service", "loadbalancer",
...).Output()) to explicitly target the test namespace by replacing
WithoutNamespace() with Namespace(oc.Namespace()) (or by adding the namespace
via Args("-n", oc.Namespace(), ...)) so creation, deletion, and lookup all use
the same namespace.
There was a problem hiding this comment.
I guess this suggestion makes sense? We should pass in "-n", oc.Namespace() in order to use the correct namespace for oc command in test 🤔
The suggested patch looks reasonable...
There was a problem hiding this comment.
@tthvo Yes, the fix is warranted. Looking at the original code before this PR, it already passed -n oc.Namespace() explicitly to both the create and delete commands:
// Old code (before this PR)
defer oc.WithoutNamespace().AsAdmin().Run("delete").Args("-n", oc.Namespace(), "service", "loadbalancer", svcName, "--ignore-not-found").Execute()
out, err := oc.AsAdmin().WithoutNamespace().Run("create").Args("-n", oc.Namespace(), "service", "loadbalancer", svcName, "--tcp=80:8080").Output()The PR inadvertently dropped that -n oc.Namespace() in the refactored code, affecting:
- The deferred
deletecall - The new IPv6 YAML
createcall (also missingnamespace:in the YAML metadata) - The existing
service loadbalancercreatecall (else branch)
The suggested patch restores the correct behavior so that creation, deletion, and the subsequent getLoadBalancerExternalIP lookup (which uses oc.Namespace()) all target the same namespace.
|
Scheduling required tests: |
|
@nrb: The following tests 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. |
4 participants
Dual stack load balancers in Kubernetes must specify an IP family policy, or else it will be set to
SingleStack. On AWS, IPv6 single stack is not supported, so update the test to reflect the required value.Assisted-By: Claude Sonnet 4.6