OCPBUGS-62267: fix forwarded header for IPv6 on IPv4 stack#713
OCPBUGS-62267: fix forwarded header for IPv6 on IPv4 stack#713jcmoraisjr wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-62267, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-62267, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
Did primary check on the cluster created using the PR. Especially the changes can be seen on the HAproxy template. May check the functionality working also.. |
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
The change looks good to me, option forwarded is a nice addition to simplify the template. My only puzzle is the testing. Normally we add e2e test scenarios to the cluster ingress operator repository and there is a good one already existing: forwarded header policy. It doesn't check Forwarded headers though but it should. The most problematic thing would be to test an IPv6 client IP, we would need to create a new Prow test job which would be a duastack one and only from there we could send some curl -6 requests. I'm not sure this is worth the effort. That is, if we don't go the e2e path, we need to rely on engineering's (manual) and QE's (maybe there is some automated testing for IPv6) testing to make sure we do the fix and don't break any existing behavior.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87 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
|
/assign @Miciah |
|
/test list |
|
/test e2e-metal-ipi-ovn-ipv6 |
1 similar comment
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-serial-ovn-dualstack periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-ovn-dualstack periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-serial-ovn-ipv6-runc periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-serial-ovn-ipv6 periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-upgrade-ovn-ipv6 periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-ovn-ipv6 periodic-ci-openshift-release-master-nightly-4.22-e2e-agent-single-node-ipv6-conformance |
|
@melvinjoseph86: trigger 7 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5c848db0-f6c0-11f0-98ae-94e841c0f30a-0 |
|
/assign |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
@melvinjoseph86: trigger 7 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/32d2f1c0-f876-11f0-8482-70e408e3c668-0 |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-serial-ovn-ipv6 periodic-ci-openshift-release-master-nightly-4.22-e2e-agent-single-node-ipv6-conformance: |
|
@melvinjoseph86: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-serial-ovn-ipv6 periodic-ci-openshift-release-master-nightly-4.22-e2e-agent-single-node-ipv6-conformance |
|
@melvinjoseph86: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7e48eae0-f93b-11f0-9936-b6d18bf6dac5-0 |
|
/test list |
|
Able to test the functionality is working with a fronting LB sending PROXY protocol and adding ipv6 binding in a ipv4 stack cluster. It is difficult to test the same by creating a ipv6 only or a dual stack using this PR before merging. Confirm the cluster accept PROXY protocol (
Hence marking as verified |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: 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. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test e2e-metal-ipi-ovn-ipv6 |
1 similar comment
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/unassign @alebedev87 |
|
/assign @davidesalerno |
|
@jcmoraisjr Changes lgtm, my only question is if we can improve test coverage for the IPv4-stack / IPv6-client / PROXY-protocol case in order to avoid regression. |
Source IP on Forwarded header is built depending on the configured stack: if IPv4 or IPv6 only, no special handling is done. On dual stack, router checks if the source is IPv6 in order to properly format with brackets and double quotes. However if a fronting load balancer has IPv6 or dual stack, a client sends a request on IPv6 and router is configured with PROXY protocol, the source IPv6 will be received on the router, but it'll be handled as IPv4, missing brackets and double quotes. This is being changed in the following way: * `append` or `replace` mode: using `option forwarded` keyword instead, which already handles IPv6 correctly * `if-none` mode: `option forwarded` doesn't support acl, so using manual building and always checking if src is IPv6
jcmoraisjr
force-pushed
the
fix-forward-ipv6
branch
from
4621952 to
6358d91
Compare
openshift-ci-robot
removed
the
verified
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughHAProxy configuration template updated to detect IPv6 addresses dynamically using an ACL instead of relying on static configuration mode for conditionally formatting the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@davidesalerno thanks for the review! The router side of this configuration is pretty static, the tests that really add value here are the e2e ones. Anyway I revisited the changes, updated the comments, and also added a few tests that explores the only condition we are using. |
openshift-ci-robot
added
jira/invalid-bug
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-62267, 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
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-62267, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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)
pkg/router/router_test.go (1)
599-643: Also assert the!ipv6_addr(IPv4) directive for each mode.The three new cases only verify the bracketed/IPv6 directive (
... if ipv6_addr). The template also emits a paired non-bracketed directive (for=%[src]... if !ipv6_addr) which is the default path for IPv4 clients on IPv4 stacks — i.e., the behavior for the vast majority of traffic today. Without explicit coverage, a regression that drops or corrupts the!ipv6_addrline (or the ACL itself) would not be caught here.This also addresses the test-coverage question raised during review about the IPv4-stack / IPv6-client / PROXY-protocol scenario: asserting both branches pins the conditional shape.
🧪 Suggested additional assertions (illustrative)
For each of
fwd1/fwd2/fwd3, add a secondmustCreateWithConfigentry in the same test slice matching the IPv4 directive, e.g. forappend:mustCreateWithConfig{ mustMatchConfig: mustMatchConfig{ section: "backend", sectionName: insecureBackendName(h.namespace, "fwd1"), attribute: "http-request", value: `add-header Forwarded for=%[src];host=%[req.hdr(host)];proto=%[req.hdr(X-Forwarded-Proto)] if !ipv6_addr`, }, },And analogously
set-header ... if !ipv6_addrforreplace, andset-header ... if !ipv6_addr !{ req.hdr(Forwarded) -m found }forif-none. Optionally, also assert the ACL itself:mustMatchConfig{ section: "backend", sectionName: insecureBackendName(h.namespace, "fwd1"), attribute: "acl", value: "ipv6_addr src -m sub :", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/router_test.go` around lines 599 - 643, Add assertions for the IPv4 branch that pairs with the existing IPv6 checks: for each test case using mustCreateWithConfig/mustMatchConfig for fwd1/fwd2/fwd3 (and the backend identified by insecureBackendName(..., "fwdX")), add a second mustMatchConfig that expects the non-bracketed / !ipv6_addr line (e.g. for append expect `add-header Forwarded for=%[src];... if !ipv6_addr`, for replace `set-header ... if !ipv6_addr`, for if-none `set-header ... if !ipv6_addr !{ req.hdr(Forwarded) -m found }`). Optionally also add an ACL assertion using mustMatchConfig with attribute "acl" and value like `ipv6_addr src -m sub :` to ensure the ACL exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/router/haproxy/conf/haproxy-config.template`:
- Around line 658-685: Comment wording: replace "despite of the router IP family
config" with "regardless of the router IP family config" in the three template
comments surrounding the acl ipv6_addr blocks (the comments just above the acl
ipv6_addr src -m sub : occurrences). Test coverage: add or update router_test.go
to exercise the !ipv6_addr branches so the code paths that set Forwarded without
brackets are covered (create test cases that simulate IPv4 src values that do
not contain ':' and assert the Forwarded header uses for=%[src] in
append/replace/if-none modes). Ensure you reference the acl ipv6_addr and the
Forwarded header generation logic when adding tests.
---
Nitpick comments:
In `@pkg/router/router_test.go`:
- Around line 599-643: Add assertions for the IPv4 branch that pairs with the
existing IPv6 checks: for each test case using
mustCreateWithConfig/mustMatchConfig for fwd1/fwd2/fwd3 (and the backend
identified by insecureBackendName(..., "fwdX")), add a second mustMatchConfig
that expects the non-bracketed / !ipv6_addr line (e.g. for append expect
`add-header Forwarded for=%[src];... if !ipv6_addr`, for replace `set-header ...
if !ipv6_addr`, for if-none `set-header ... if !ipv6_addr !{ req.hdr(Forwarded)
-m found }`). Optionally also add an ACL assertion using mustMatchConfig with
attribute "acl" and value like `ipv6_addr src -m sub :` to ensure the ACL
exists.
🪄 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: 40d2e5d2-08d3-44a1-86d6-ef68e45e2564
📒 Files selected for processing (2)
images/router/haproxy/conf/haproxy-config.templatepkg/router/router_test.go
option forwarded is not working as expected on a full setup, for an unknown reason. Tests made with 2.8.10, 2.8.18, 3.2.11, all of them is missing the host field in the forwarded header. For some reason hdr(host) is not being rendered to the actual Host header, although the same sample works on a http-request set-header in the same backend. Because of that, moving from the better optimized option to the manual check and configuration.
jcmoraisjr
force-pushed
the
fix-forward-ipv6
branch
from
6358d91 to
dc7e786
Compare
|
@jcmoraisjr: 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. |
8 participants
Source IP on Forwarded header is built depending on the configured stack: if IPv4 or IPv6 only, no special handling is done. On dual stack, router checks if the source is IPv6 in order to properly format with brackets and double quotes.
However if a fronting load balancer has IPv6 or dual stack, a client sends a request on IPv6 and router is configured with PROXY protocol, the source IPv6 will be received on the router, but it'll be handled as IPv4, missing brackets and double quotes.
This is being changed in the following way:
appendorreplacemode: usingoption forwardedkeyword instead, which already handles IPv6 correctlyif-nonemode:option forwardeddoesn't support acl, so using manual building and always checking if src is IPv6 - EDIT:appendandreplaceas well.For an unknown reason, the router deployment on some environments does not render
hdr(host)correctly fromoption forwarded, neither declaring viahost-exprnor leaving the default option. It is always empty. Moreover it works when running locally, and also on a local single node OCP deployment. To be investigated and improved in the future, since checking via acl and manually building the header has a small penalty on performance.Summary by CodeRabbit
Bug Fixes
Forwardedheader generation across all modes to ensure consistent formatting.Tests
Forwardedheader generation, validating append, replace, and if-none mode behaviors.