Skip to content

[WIP] OCPBUGS-63219: Support clientIPPreservationMode for AWS NLB #1426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gcs278 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[WIP] OCPBUGS-63219: Support clientIPPreservationMode for AWS NLB #1426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3b1e27
OCPBUGS-63219: Support clientIPPreservationMode for AWS NLB
gcs278 Apr 29, 2026
dbb29d0
OCPBUGS-63219: Add E2E tests for clientIPPreservationMode
gcs278 Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,3 +225,5 @@ replace github.com/imdario/mergo => github.com/imdario/mergo v0.3.5
// Use OSSM 3.3.1 for Sail Library integration.
// 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
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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}" || true

Repository: 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.

Copy link
Copy Markdown
Contributor Author

@gcs278 gcs278 Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeRabbit I know - I'm waiting for openshift/api#2661 to merge

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sa
github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ=
github.com/gabriel-vasile/mimetype v1.4.3 h1:in2uUcidCuFcDKtdcBxlR0rJ1+fsokWf+uqxgUFjbI0=
github.com/gabriel-vasile/mimetype v1.4.3/go.mod h1:d8uq/6HKRL6CGdk+aubisF/M5GcPfT7nKyLpA0lbSSk=
github.com/gcs278/api v0.0.0-20260429000454-cff0427099ea h1:eFEpRHG2JZcHQZ8VpqXvnAgRaWyFWbystXFMD/m/Ckw=
github.com/gcs278/api v0.0.0-20260429000454-cff0427099ea/go.mod h1:pyVjK0nZ4sRs4fuQVQ4rubsJdahI1PB94LnQ8sGdvxo=
github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04=
github.com/go-errors/errors v1.5.1 h1:ZwEMSLRCapFLflTpT7NKaAc7ukJ8ZPEjzlxt8rPN8bk=
github.com/go-errors/errors v1.5.1/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og=
Expand Down Expand Up @@ -497,8 +499,6 @@ github.com/opencontainers/image-spec v1.1.1 h1:y0fUlFfIZhPF1W537XOLg0/fcx6zcHCJw
github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M=
github.com/openshift-service-mesh/sail-operator v0.0.0-20260327145107-d4059b965c68 h1:qbJWOmvwtzZJT6u7b2HHMF3g0wWvbcYmv9keKcJlb+k=
github.com/openshift-service-mesh/sail-operator v0.0.0-20260327145107-d4059b965c68/go.mod h1:l5/9fIFLMnrArPGvg49DBrWZzi8LojS5OcGe9nJeeI4=
github.com/openshift/api v0.0.0-20260327065519-582dc3d316b7 h1:7AmoMSqTryaZu65nij6EACe8+DmlMlmR1giaUx5S5sQ=
github.com/openshift/api v0.0.0-20260327065519-582dc3d316b7/go.mod h1:pyVjK0nZ4sRs4fuQVQ4rubsJdahI1PB94LnQ8sGdvxo=
github.com/openshift/client-go v0.0.0-20260317180604-743f664b82d1 h1:Hr/R38eg5ZJXfbiaHumjJIN1buDZwhsm4ys4npVCXH0=
github.com/openshift/client-go v0.0.0-20260317180604-743f664b82d1/go.mod h1:Za51LlH76ALiQ/aKGBYJXmyJNkA//IDJ+I///30CA2M=
github.com/openshift/library-go v0.0.0-20251021141706-f489e811f030 h1:dbv8ZYDWIl22A5WBjQJTKeENM08f8HwMBuv8glDXO/0=
Expand Down
50 changes: 50 additions & 0 deletions manifests/00-custom-resource-definition-CustomNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down Expand Up @@ -2749,6 +2774,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down
50 changes: 50 additions & 0 deletions manifests/00-custom-resource-definition-DevPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down Expand Up @@ -2749,6 +2774,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down
50 changes: 50 additions & 0 deletions manifests/00-custom-resource-definition-OKD.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down Expand Up @@ -2718,6 +2743,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down
50 changes: 50 additions & 0 deletions manifests/00-custom-resource-definition-TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down Expand Up @@ -2749,6 +2774,31 @@ spec:
networkLoadBalancerParameters holds configuration parameters for an AWS
network load balancer. Present only if type is NLB.
properties:
clientIPPreservationMode:
description: |-
clientIPPreservationMode specifies how client IP addresses are
preserved by the load balancer.

Valid values are "Native" and "ProxyProtocol".

When set to "Native", the NLB uses AWS's native client IP preservation,
which may cause hairpin connection failures for internal load balancers when
connections are made from pods to router pods on the same node.

When set to "ProxyProtocol", the NLB uses PROXY protocol v2 to preserve
client IP addresses. This avoids hairpin connection failures.

When omitted, this means the user has no opinion and the value is left
to the platform to choose a good default, which is subject to change
over time. The current default is "ProxyProtocol".

Note that changing this field may cause brief connection failures during
the transition as the NLB attribute change and router rollout occur
independently.
enum:
- Native
- ProxyProtocol
type: string
eipAllocations:
description: |-
eipAllocations is a list of IDs for Elastic IP (EIP) addresses that
Expand Down
Loading