DNM/TEST: force IPv4 for idle test in dual-stack + BGP environments#30813
DNM/TEST: force IPv4 for idle test in dual-stack + BGP environments#30813jluhrsen wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/test e2e-metal-ipi-ovn-dualstack-bgp |
WalkthroughThe waitHTTPGetStatus helper in the extended router tests now reads and inspects HTTP response bodies and improves logging: it logs read errors, conditionally logs headers and a truncated body preview for early failures, and otherwise preserves previous status-check behavior and return logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.5.0)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: jluhrsen 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/router/idle.go (1)
102-109: LGTM! Clear fix for the dual-stack IPv6 preference issue.The
IPFamiliesandIPFamilyPolicyconfiguration correctly forces IPv4-only behavior, and the comment explains the rationale well (HAProxy IPv6 preference + K8s 1.34 KEP-4633 restrictions).Optional: The IIFE for pointer creation is functional but verbose. Consider using
ptr.To()fromk8s.io/utils/ptrfor cleaner code:♻️ Optional simplification using ptr.To()
Add import:
"k8s.io/utils/ptr"Then simplify:
- IPFamilyPolicy: func() *corev1.IPFamilyPolicy { - policy := corev1.IPFamilyPolicySingleStack - return &policy - }(), + IPFamilyPolicy: ptr.To(corev1.IPFamilyPolicySingleStack),,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/idle.go` around lines 102 - 109, Replace the anonymous IIFE used to create a pointer for IPFamilyPolicy with the helper from k8s.io/utils/ptr: import "k8s.io/utils/ptr" and change the IPFamilyPolicy value from the inline func that returns &policy to ptr.To(corev1.IPFamilyPolicySingleStack) so the block uses IPFamilyPolicy: ptr.To(corev1.IPFamilyPolicySingleStack) while leaving IPFamilies and the surrounding comment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/router/idle.go`:
- Around line 102-109: Replace the anonymous IIFE used to create a pointer for
IPFamilyPolicy with the helper from k8s.io/utils/ptr: import "k8s.io/utils/ptr"
and change the IPFamilyPolicy value from the inline func that returns &policy to
ptr.To(corev1.IPFamilyPolicySingleStack) so the block uses IPFamilyPolicy:
ptr.To(corev1.IPFamilyPolicySingleStack) while leaving IPFamilies and the
surrounding comment unchanged.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/router/idle.go
|
@jluhrsen: 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. |
The router idling test is failing with HTTP 403 Forbidden on metal-ipi-ovn-dualstack-bgp jobs starting Feb 20, 2026. Previous hypothesis (IPv6 endpoint preference) was disproven when forcing IPv4-only service still resulted in HTTP 403. This commit: - Reverts the IPv4-only service configuration (didn't help) - Adds logging of HTTP response body and headers for the first 10 non-200 responses - Limits body preview to 500 chars to avoid log spam This will help identify what component is returning 403 and why. Affects: e2e-metal-ipi-ovn-dualstack-bgp Related: openshift#30813
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/router/idle.go (2)
314-331: Useio.ReadAllinstead of deprecatedioutil.ReadAll.
ioutil.ReadAllis deprecated since Go 1.16. Theiopackage is already imported, so switch toio.ReadAll.The diagnostic logging logic itself looks good for debugging the 403 errors in dual-stack BGP environments.
♻️ Proposed fix
// Read response body to help debug unexpected status codes - bodyBytes, readErr := ioutil.ReadAll(resp.Body) + bodyBytes, readErr := io.ReadAll(resp.Body) if readErr != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/idle.go` around lines 314 - 331, Replace the deprecated ioutil.ReadAll call with io.ReadAll: in the block that reads the response body (where bodyBytes, readErr are assigned from ioutil.ReadAll(resp.Body)), call io.ReadAll(resp.Body) instead; keep the existing error handling and subsequent logging logic that uses bodyBytes, readErr, resp.StatusCode, resp.Header and attempt unchanged so behavior remains identical while removing the deprecated import usage.
8-8: Remove deprecatedio/ioutilimport.The
io/ioutilpackage has been deprecated since Go 1.16. Sinceiois already imported on line 7, this import can be removed after updating theioutil.ReadAllcall on line 316.♻️ Proposed fix
- "io/ioutil"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/idle.go` at line 8, Remove the deprecated io/ioutil import and replace its use with the io package: delete the "io/ioutil" import entry and change the ioutil.ReadAll(...) call (the ReadAll invocation in this file, e.g., where ioutil.ReadAll is used) to io.ReadAll(...); ensure the io import remains and run go vet/go test to verify there are no remaining references to io/ioutil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/router/idle.go`:
- Around line 314-331: Replace the deprecated ioutil.ReadAll call with
io.ReadAll: in the block that reads the response body (where bodyBytes, readErr
are assigned from ioutil.ReadAll(resp.Body)), call io.ReadAll(resp.Body)
instead; keep the existing error handling and subsequent logging logic that uses
bodyBytes, readErr, resp.StatusCode, resp.Header and attempt unchanged so
behavior remains identical while removing the deprecated import usage.
- Line 8: Remove the deprecated io/ioutil import and replace its use with the io
package: delete the "io/ioutil" import entry and change the ioutil.ReadAll(...)
call (the ReadAll invocation in this file, e.g., where ioutil.ReadAll is used)
to io.ReadAll(...); ensure the io import remains and run go vet/go test to
verify there are no remaining references to io/ioutil.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/router/idle.go
The router idling test is failing with HTTP 403 Forbidden on metal-ipi-ovn-dualstack-bgp jobs starting Feb 20, 2026. Root cause is still unknown. This commit adds diagnostic logging to capture the actual HTTP response body and headers for the first 10 failed requests. Changes: - Log full HTTP headers on non-200 responses (first 10 attempts) - Log response body preview (up to 500 chars) - After attempt 10, just log status code to avoid log spam This will help identify: - What component is returning 403 (HAProxy, proxy, auth service) - Why it's forbidden (error message in response body) - Any relevant headers (forwarding, auth, server info) Affects: e2e-metal-ipi-ovn-dualstack-bgp Related: Investigation of Feb 20, 2026 test failures
jluhrsen
force-pushed
the
fix-idle-test-k8s-1.34-auth
branch
from
f8070c3 to
8256187
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/extended/router/idle.go (2)
98-110:⚠️ Potential issue | 🟠 MajorImplement the IPv4 SingleStack service config described by the PR objective.
idleService.Speccurrently does not setIPFamilyPolicy/IPFamilies. On dual-stack+BGP, this can still expose IPv6 endpoints and keep the 403 path active instead of enforcing IPv4.💡 Proposed fix
Spec: corev1.ServiceSpec{ + IPFamilyPolicy: utilpointer.To(corev1.IPFamilyPolicySingleStack), + IPFamilies: []corev1.IPFamily{ + corev1.IPv4Protocol, + }, Selector: map[string]string{ "app": "idle-test", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/idle.go` around lines 98 - 110, The Service spec for the test's idleService is missing IPFamilyPolicy/IPFamilies and must be configured for IPv4 single-stack; update the corev1.ServiceSpec (the object created as idleService / the inline Spec block) to set IPFamilyPolicy to corev1.IPFamilyPolicySingleStack and IPFamilies to []corev1.IPFamily{corev1.IPv4Protocol} so the test always exposes IPv4 endpoints only.
7-9:⚠️ Potential issue | 🔴 CriticalSwitch to
io.ReadAlland removeio/ioutilimport.The
iopackage is imported at line 7 but never used. Line 316 callsioutil.ReadAll, which is deprecated since Go 1.16. Useio.ReadAllinstead and remove the unusedio/ioutilimport. Unused imports cause compilation failures.💡 Proposed fix
- "io" - "io/ioutil" + "io" ... - bodyBytes, readErr := ioutil.ReadAll(resp.Body) + bodyBytes, readErr := io.ReadAll(resp.Body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/idle.go` around lines 7 - 9, Replace the deprecated call to ioutil.ReadAll with io.ReadAll and remove the unused "io/ioutil" import: change the call to ioutil.ReadAll(...) to io.ReadAll(...), delete the "io/ioutil" entry from the import block, and run goimports/gofmt to clean up imports so the existing "io" import is used (this fixes the unused-import compile error referencing ioutil.ReadAll).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/extended/router/idle.go`:
- Around line 98-110: The Service spec for the test's idleService is missing
IPFamilyPolicy/IPFamilies and must be configured for IPv4 single-stack; update
the corev1.ServiceSpec (the object created as idleService / the inline Spec
block) to set IPFamilyPolicy to corev1.IPFamilyPolicySingleStack and IPFamilies
to []corev1.IPFamily{corev1.IPv4Protocol} so the test always exposes IPv4
endpoints only.
- Around line 7-9: Replace the deprecated call to ioutil.ReadAll with io.ReadAll
and remove the unused "io/ioutil" import: change the call to ioutil.ReadAll(...)
to io.ReadAll(...), delete the "io/ioutil" entry from the import block, and run
goimports/gofmt to clean up imports so the existing "io" import is used (this
fixes the unused-import compile error referencing ioutil.ReadAll).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/router/idle.go
|
/close |
|
@jluhrsen: Closed this PR. 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 kubernetes-sigs/prow repository. |
2 participants
Root cause: HAProxy prefers IPv6 endpoints in dual-stack configurations, but the IPv6 path hits Kubernetes 1.34 anonymous authentication restrictions (KEP-4633), resulting in HTTP 403 Forbidden errors.
This issue is specific to bare metal + BGP + dual-stack because:
Virtual/cloud environments don't exhibit this because their traffic path doesn't involve the same BGP speaker routing.
Fix: Configure the test service with IPFamilyPolicy: SingleStack and IPFamilies: [IPv4] to prevent HAProxy from seeing IPv6 endpoints, forcing all traffic through the IPv4 path which works with anonymous auth.
Timeline:
Related:
Affects: e2e-metal-ipi-ovn-dualstack-bgp
Fixes: Consistent HTTP 403 errors on route access
Summary by CodeRabbit