[WIP] OCPBUGS-63219: Support clientIPPreservationMode for AWS NLB#1426
Open
gcs278 wants to merge 2 commits into
Open
[WIP] OCPBUGS-63219: Support clientIPPreservationMode for AWS NLB#1426gcs278 wants to merge 2 commits into
gcs278 wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-important
Contributor
|
@gcs278: This pull request references Jira Issue OCPBUGS-63219, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
📝 WalkthroughWalkthroughAdds a 🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Consume the new clientIPPreservationMode field on AWSNetworkLoadBalancerParameters to control how client IP addresses are preserved by NLBs. When set to ProxyProtocol, the operator configures the NLB target group with preserve_client_ip.enabled=false and proxy_protocol_v2.enabled=true via the target-group-attributes Service annotation. It also sets ROUTER_USE_PROXY_PROTOCOL=true on the router deployment so HAProxy parses PROXY protocol headers. This avoids hairpin connection failures on internal NLBs. When set to Native (or omitted on existing IngressControllers), the NLB uses AWS's native client IP preservation, which is the current default behavior. New IngressControllers default to ProxyProtocol via controller-managed defaulting in setDefaultProviderParameters, gated with !alreadyAdmitted so that existing IngressControllers are not modified on upgrade. https://redhat.atlassian.net/browse/OCPBUGS-63219 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gcs278
force-pushed
the
OCPBUGS-63219-proxy-protocol
branch
from
4f32483 to
c3b1e27
Compare
gcs278
force-pushed
the
OCPBUGS-63219-proxy-protocol
branch
2 times, most recently
from
7ca9715 to
6a46693
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
gcs278
changed the title
Contributor
Author
|
/jira refresh |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-bug
Contributor
|
@gcs278: This pull request references Jira Issue OCPBUGS-63219, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci
Bot
requested review from
Thealisyed,
anuragthehatter and
jcmoraisjr
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
manifests/00-custom-resource-definition-OKD.yaml (1)
2760-2763: Optional: clarify status wording.Line 2760–2763 says “the user has no opinion,” which is spec-oriented language. For the status schema, consider wording that reflects observed/effective state instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/00-custom-resource-definition-OKD.yaml` around lines 2760 - 2763, Update the status schema description text that currently reads “the user has no opinion” to language that reflects observed/effective state — e.g., replace that phrase with “not specified by the user; the platform may choose a default (currently 'ProxyProtocol')” and ensure any other status-related descriptions use present-tense, observed wording rather than spec-oriented phrasing so the status describes the effective value rather than intent.test/e2e/nlb_client_ip_preservation_test.go (1)
37-38: Prefer generated IC names to reduce collision risk in repeated runs.Using fixed names can cause intermittent
AlreadyExistsfailures in reruns or partially cleaned environments.♻️ Suggested change
- name := types.NamespacedName{Namespace: operatorNamespace, Name: "nlb-pp-test"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("nlb-pp-")} ... - name := types.NamespacedName{Namespace: operatorNamespace, Name: "nlb-default"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: names.SimpleNameGenerator.GenerateName("nlb-default-")}Also applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/nlb_client_ip_preservation_test.go` around lines 37 - 38, The test currently uses a fixed NamespacedName (variable name := types.NamespacedName{Namespace: operatorNamespace, Name: "nlb-pp-test"}) and derived domain, which risks AlreadyExists on reruns; change the creation to generate a unique resource name (e.g., append a short random/UUID/timestamp suffix) when constructing the types.NamespacedName and update the derived domain assignment (domain := name.Name + "." + dnsConfig.Spec.BaseDomain) accordingly; apply the same change to the other fixed NamespacedName usage referenced around lines 125-126 so all test resources use generated unique names to avoid collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 229: The go.mod contains a replace directive "replace
github.com/openshift/api => github.com/gcs278/api
v0.0.0-20260429000454-cff0427099ea" that introduces a fork without explanation;
either remove this replace to use the upstream github.com/openshift/api, or if
the fork is required keep the replace but add a nearby comment explaining why
the fork is necessary (reference the commit cff0427099ea and the specific
issue/bug it fixes), mark it as TODO with a target removal date/version, and
ensure the rationale is committed so reviewers know it’s intentional.
In `@test/e2e/nlb_client_ip_preservation_test.go`:
- Around line 169-171: The wait timeout in
waitForIngressControllerClientIPPreservationMode is set to 2 minutes causing
intermittent flakes; update the timeout argument in the
wait.PollUntilContextTimeout call from 2*time.Minute to 5*time.Minute to match
other readiness waits so reconciliation has more time to settle (reference
function name waitForIngressControllerClientIPPreservationMode and the
wait.PollUntilContextTimeout invocation).
---
Nitpick comments:
In `@manifests/00-custom-resource-definition-OKD.yaml`:
- Around line 2760-2763: Update the status schema description text that
currently reads “the user has no opinion” to language that reflects
observed/effective state — e.g., replace that phrase with “not specified by the
user; the platform may choose a default (currently 'ProxyProtocol')” and ensure
any other status-related descriptions use present-tense, observed wording rather
than spec-oriented phrasing so the status describes the effective value rather
than intent.
In `@test/e2e/nlb_client_ip_preservation_test.go`:
- Around line 37-38: The test currently uses a fixed NamespacedName (variable
name := types.NamespacedName{Namespace: operatorNamespace, Name: "nlb-pp-test"})
and derived domain, which risks AlreadyExists on reruns; change the creation to
generate a unique resource name (e.g., append a short random/UUID/timestamp
suffix) when constructing the types.NamespacedName and update the derived domain
assignment (domain := name.Name + "." + dnsConfig.Spec.BaseDomain) accordingly;
apply the same change to the other fixed NamespacedName usage referenced around
lines 125-126 so all test resources use generated unique names to avoid
collisions.
🪄 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: 7aaec5b4-af35-4500-903f-2af108380f15
⛔ Files ignored due to path filters (49)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
go.modmanifests/00-custom-resource-definition-CustomNoUpgrade.yamlmanifests/00-custom-resource-definition-DevPreviewNoUpgrade.yamlmanifests/00-custom-resource-definition-OKD.yamlmanifests/00-custom-resource-definition-TechPreviewNoUpgrade.yamlmanifests/00-custom-resource-definition.yamlpkg/operator/controller/ingress/controller.gopkg/operator/controller/ingress/controller_test.gopkg/operator/controller/ingress/load_balancer_service.gopkg/operator/controller/ingress/load_balancer_service_test.gopkg/operator/controller/ingress/status.gotest/e2e/all_test.gotest/e2e/nlb_client_ip_preservation_test.go
| // Replace needed because the fork keeps upstream module name for compatibility. | ||
| replace github.com/istio-ecosystem/sail-operator => github.com/openshift-service-mesh/sail-operator v0.0.0-20260327145107-d4059b965c68 | ||
|
|
||
| replace github.com/openshift/api => github.com/gcs278/api v0.0.0-20260429000454-cff0427099ea |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking replacement in go.mod:"
rg -n 'replace github.com/openshift/api' go.mod
FORK_VERSION="$(awk '/replace github.com\/openshift\/api =>/{print $5}' go.mod)"
FORK_COMMIT="${FORK_VERSION##*-}"
echo "Fork pseudo-version: ${FORK_VERSION}"
echo "Derived commit: ${FORK_COMMIT}"
echo
echo "Commit presence in upstream openshift/api:"
git ls-remote https://github.com/openshift/api.git "${FORK_COMMIT}" || true
echo
echo "Commit presence in fork gcs278/api:"
git ls-remote https://github.com/gcs278/api.git "${FORK_COMMIT}" || trueRepository: openshift/cluster-ingress-operator
Length of output: 373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for comments around the replacement in go.mod
echo "=== Context around line 229 in go.mod ==="
sed -n '225,235p' go.mod
echo
echo "=== Full go.mod to find any comments about this replacement ==="
grep -B 5 -A 5 "replace github.com/openshift/api" go.mod || echo "(No additional context found)"
echo
echo "=== Check if there are TODOs or FIXMEs related to this ==="
rg -i "gcs278|openshift/api|replace.*api" go.mod go.sum 2>/dev/null || echo "(No matches)"Repository: openshift/cluster-ingress-operator
Length of output: 1361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to verify the commit hash with different approaches
COMMIT_HASH="cff0427099ea"
FORK_REPO="gcs278/api"
UPSTREAM_REPO="openshift/api"
echo "=== Attempting to verify commit in fork repository ==="
# Try fetching via GitHub API instead of git ls-remote
curl -s "https://api.github.com/repos/${FORK_REPO}/commits/${COMMIT_HASH}" | jq -r '.sha, .message, .commit.message' 2>/dev/null || echo "Fork commit lookup failed or repo not found"
echo
echo "=== Attempting to verify commit in upstream repository ==="
curl -s "https://api.github.com/repos/${UPSTREAM_REPO}/commits/${COMMIT_HASH}" | jq -r '.sha, .message, .commit.message' 2>/dev/null || echo "Upstream commit lookup failed or commit doesn't exist"Repository: openshift/cluster-ingress-operator
Length of output: 2007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any other references to this fork or temporary replacements
echo "=== Search for any documentation about this replacement ==="
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "CHANGES*" \) -exec grep -l "gcs278\|openshift/api" {} \; 2>/dev/null | head -20 || echo "(No documentation found)"
echo
echo "=== Check recent git history for context about this change ==="
git log --oneline --all -20 -- go.mod 2>/dev/null | head -10 || echo "(git log unavailable)"
echo
echo "=== Check git blame for this specific line ==="
git blame -L 229,229 go.mod 2>/dev/null || echo "(git blame unavailable)"Repository: openshift/cluster-ingress-operator
Length of output: 1300
Remove the fork replacement or document why it's necessary.
The replacement to github.com/gcs278/api introduces a supply-chain maintenance risk for a core dependency, especially because the commit (cff0427099ea) exists in the upstream github.com/openshift/api repository. There is no explanation in the code for why the fork is required instead of using the upstream version directly. If the fork is temporary, mark it with a TODO and target a removal date. If it serves a specific purpose, document that rationale in a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 229, The go.mod contains a replace directive "replace
github.com/openshift/api => github.com/gcs278/api
v0.0.0-20260429000454-cff0427099ea" that introduces a fork without explanation;
either remove this replace to use the upstream github.com/openshift/api, or if
the fork is required keep the replace but add a nearby comment explaining why
the fork is necessary (reference the commit cff0427099ea and the specific
issue/bug it fixes), mark it as TODO with a target removal date/version, and
ensure the rationale is committed so reviewers know it’s intentional.
Contributor
Author
There was a problem hiding this comment.
@CodeRabbit I know - I'm waiting for openshift/api#2661 to merge
There was a problem hiding this comment.
@gcs278, thanks for the context! That makes sense. Once openshift/api#2661 merges, this replace directive should be removed and the dependency updated to the official upstream version.
In the meantime, it would be helpful to add a comment above the replace directive (similar to the other replace stanzas in the file) so reviewers understand it's intentional and temporary:
// Temporary: replace with upstream once https://github.com/openshift/api/pull/2661 merges.
replace github.com/openshift/api => github.com/gcs278/api v0.0.0-20260429000454-cff0427099ea📝 Learnings were identified and not saved (knowledge base disabled). Enable
Contributor
Author
|
/retest |
Add two E2E tests: TestAWSNLBClientIPPreservationMode creates an NLB with ProxyProtocol, verifies the target-group-attributes annotation, router env var, and status are correct, tests connectivity, then switches to Native and verifies the annotation and env var are removed. TestAWSNLBDefaultClientIPPreservationMode creates an NLB without specifying clientIPPreservationMode and verifies the controller defaults it to ProxyProtocol. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gcs278
force-pushed
the
OCPBUGS-63219-proxy-protocol
branch
from
6a46693 to
dbb29d0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/nlb_client_ip_preservation_test.go (1)
37-38: Use generated IngressController names to reduce rerun collision flakes.Since these tests run in parallel, fixed names (
"nlb-pp-test","nlb-default") can conflict with leftovers from interrupted runs. Prefer generated names.♻️ Suggested change
- name := types.NamespacedName{Namespace: operatorNamespace, Name: "nlb-pp-test"} + name := types.NamespacedName{ + Namespace: operatorNamespace, + Name: names.SimpleNameGenerator.GenerateName("nlb-pp-test-"), + } domain := name.Name + "." + dnsConfig.Spec.BaseDomain- name := types.NamespacedName{Namespace: operatorNamespace, Name: "nlb-default"} + name := types.NamespacedName{ + Namespace: operatorNamespace, + Name: names.SimpleNameGenerator.GenerateName("nlb-default-"), + } domain := name.Name + "." + dnsConfig.Spec.BaseDomainAlso applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/nlb_client_ip_preservation_test.go` around lines 37 - 38, Replace the fixed IngressController names with generated, unique names to avoid parallel test collisions: instead of using the literal "nlb-pp-test" when constructing the NamespacedName stored in the variable name (and building domain from name.Name), generate a unique name (e.g., using a random/suffix or Kubernetes GenerateName pattern) and use that value for name.Name and domain; do the same replacement for the other hardcoded "nlb-default" occurrence (lines around where that literal is used). Update any assertions or cleanup that reference those literals to use the generated name variables (name and domain) so tests don't collide across runs.
🤖 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/e2e/nlb_client_ip_preservation_test.go`:
- Around line 37-38: Replace the fixed IngressController names with generated,
unique names to avoid parallel test collisions: instead of using the literal
"nlb-pp-test" when constructing the NamespacedName stored in the variable name
(and building domain from name.Name), generate a unique name (e.g., using a
random/suffix or Kubernetes GenerateName pattern) and use that value for
name.Name and domain; do the same replacement for the other hardcoded
"nlb-default" occurrence (lines around where that literal is used). Update any
assertions or cleanup that reference those literals to use the generated name
variables (name and domain) so tests don't collide across runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d9f49a9a-0e30-4b40-b195-94d238657d59
📒 Files selected for processing (2)
test/e2e/all_test.gotest/e2e/nlb_client_ip_preservation_test.go
Member
|
/assign |
Contributor
|
@gcs278: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
clientIPPreservationModefield onAWSNetworkLoadBalancerParametersto control how client IP addresses are preserved by NLBsProxyProtocol, configures the NLB target group withpreserve_client_ip.enabled=falseandproxy_protocol_v2.enabled=true, and enablesROUTER_USE_PROXY_PROTOCOL=trueon the router — this fixes hairpin connection failures on internal NLBs (OCPBUGS-63219)Native(or omitted on existing IngressControllers), preserves current behavior using AWS's native client IP preservationProxyProtocolvia controller-managed defaulting (not CRD default), so existing ICs are not modified on upgradeDependencies
clientIPPreservationModefield to the APIservice.beta.kubernetes.io/aws-load-balancer-target-group-attributesannotationTest plan
desiredLoadBalancerServicewith ProxyProtocol and Native modesIsProxyProtocolNeededwith NLB ProxyProtocol/Native🤖 Generated with Claude Code