OCPBUGS-63219: Support NLB protocol to configure proxy protocol and client IP preservation - #1426
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-important
|
@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
|
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:
📝 WalkthroughWalkthroughAdds 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4f32483 to
c3b1e27
Compare
7ca9715 to
6a46693
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
gcs278
changed the title
|
/jira refresh |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-bug
|
@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. |
![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
|
/retest |
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
|
/assign |
dbb29d0 to
2c0d8a4
Compare
|
Actionable comments posted: 0 |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@gcs278: 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. |
|
/pipeline required |
|
Scheduling tests matching the |
…safety Add TestAWSNLBProtocol and TestAWSNLBDefaultProtocol to verify NLB protocol annotation, router PROXY protocol env var, connectivity, and defaulting behavior for new IngressControllers. Add TestAWSNLBUpgradeAnnotationPreservation to simulate upgrading a pre-existing NLB IngressController and verify the operator does not stomp user-set target-group-attributes annotations. Add TestAWSLBTypeTransitionSafety to verify that CLB/NLB type transitions do not break traffic or stomp proxy protocol annotations while the service is in a pending transition state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lers Add an informational Prometheus alert that fires when an IngressController uses an internal AWS NLB with an empty NLB protocol in status. An empty NLB protocol means the operator is not managing the target-group- attributes annotation for this IC, so it may be using AWS's native client IP preservation and could be affected by hairpin connection failures. The alert nudges admins to set protocol to PROXY (to fix hairpin) or TCP (to acknowledge current behavior and silence the alert). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… for NLB protocol defaulting Replace the !alreadyAdmitted gate on NLB protocol defaulting with isNewIngressController (ic.Status.EndpointPublishingStrategy == nil). The alreadyAdmitted flag can flip when an IC is re-admitted after a validation failure, which would incorrectly re-default the protocol to PROXY for existing NLBs. Checking whether status has been initialized is a reliable signal for truly new IngressControllers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Refactor the proxy protocol annotation switch to use effectiveLBType as the outer switch with proxyNeeded for value selection, guarded by len(nlbProtocol) > 0 for upgrade safety. Use t.Context() instead of context.TODO() in e2e tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add upgrade context, traffic disruption warning, and TCP trade-off explanation to the alert description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ring auto-delete When auto-delete is set, the service will be deleted and recreated in the same reconcile. Pass nil for currentService so that desiredLoadBalancerService and IsProxyProtocolNeeded compute state from status (the intended state) rather than from the soon-to-be-deleted service. This prevents NLB-specific annotations from being set on a new CLB service, which the CCM rejects with "target group attributes annotation is only supported for NLB." Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@jcmoraisjr Sorry for the extra review round: the proxy annotation switch refactor you suggested actually exposed a bug in the auto-delete workflow. When auto-delete is set and the LB type changes from NLB→CLB, The fix: when auto-delete is set, pass nil for Also reduced |
|
/pipeline required |
|
Scheduling tests matching the |
|
I don't know whether I'm glad to help the finding or sorry to have added the noise 🙂 /lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-upgrade |
|
@gcs278: trigger 1 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/1f49a5d0-8c3d-11f1-835b-bfe75395c83d-0 |
|
install failure |
|
|
|
this job is known to be flaky today: /test hypershfit-e2e-aks |
|
/test hypershift-e2e-aks |
|
@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: 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. |
|
Copying @melvinjoseph86's verified: |
|
@gcs278: This PR has been marked to be verified later 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. |
|
/test e2e-gcp-operator |
|
|
|
I think we need to proceed merging with this one as is. It's not a perfect solution, but it is a pragmatic one, this fix is in high demand. In the future, I may try to add some sort of one-off migration logic so that we don't have these subtle status behaviors. I also would like to add E2E tests for the next metric/alert, but since we are out of time, we can merge as is, and I'll provide a follow up. /unhold |
|
@gcs278: Jira Issue OCPBUGS-63219: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-63219 has not been moved to the MODIFIED state. This PR is marked as verified-later. Jira issue(s) in the title of this PR will require post-merge verification. After testing, it must be manually moved to the 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. |
|
Fix included in release 5.0.0-0.nightly-2026-08-04-172547 |
6 participants
Summary
AWS NLBs have
preserve_client_ip.enabled=trueby default on their target groups. This causes hairpin connection failures on internal NLBs: when a pod sends traffic through the NLB and it routes back to the same node, the return packet's destination matches the pod's own IP, breaking the connection. This impacts ROSA private deployments and any internal NLB setup where the client pod and router pod share a node.This PR adds a
protocolfield toAWSNetworkLoadBalancerParameterswith two values:PROXY— disables native client IP preservation and enables PROXY protocol v2 (preserve_client_ip.enabled=false,proxy_protocol_v2.enabled=true). HAProxy parses PROXY protocol headers to obtain the original client IP. Hairpin connections work.TCP— uses AWS's native client IP preservation (preserve_client_ip.enabled=true). This is the current behavior. Hairpin connections may fail on internal NLBs.The field reuses the same name and type already used by four other endpoint publishing strategies (HostNetwork, NodePort, Private, IBM), providing a consistent API surface: regardless of strategy or platform,
protocol: PROXYmeans "enable PROXY protocol on the router and configure the infrastructure to match."What this PR does
PROXY, setsservice.beta.kubernetes.io/aws-load-balancer-target-group-attributes: preserve_client_ip.enabled=false,proxy_protocol_v2.enabled=trueon the NLB Service. WhenTCP, setspreserve_client_ip.enabled=true,proxy_protocol_v2.enabled=false. When protocol is empty (pre-existing NLB), no annotation is set — the operator does not manage the annotation for these ICs.PROXY, setsROUTER_USE_PROXY_PROTOCOL=trueon the router so HAProxy parses PROXY protocol headers. The existingIsProxyProtocolNeededfunction is updated to return true for NLBs with PROXY protocol.aws-load-balancer-proxy-protocol: "*") is now only set for CLBs. NLBs use the target-group-attributes annotation instead. A switch statement indesiredLoadBalancerServiceroutes to the right annotation based on effective LB type and protocol.getEffectiveAWSLoadBalancerTypeto read the LB type from the current service annotation (not status) during pending CLB↔NLB transitions, preventing annotation stomping on the still-live service.IngressStatusesEqualis updated to compare protocol, andupdatePublishingStrategysyncs spec changes to status. It also handles two additional scenarios:previousStatusLBType). Pre-existing NLBs on upgrade are excluded because their status type was already NLB (isPreExistingNLB).setDefaultProviderParametersdefaults new NLB IngressControllers toPROXY, gated byisNewIngressController(ic.Status.EndpointPublishingStrategy == nil). This is more reliable than!alreadyAdmittedbecausealreadyAdmittedcan flip on re-admission after a validation failure.auto-delete-load-balancerannotation is set, the desired service and proxy protocol determination are computed from status rather than the current service. This prevents NLB-specific annotations from being applied to a new CLB service during LB type transitions, which the CCM would reject.NLBHairpinRiskalert for existing internal NLB IngressControllers that have protocol unset, nudging admins to set PROXY (fix hairpin) or TCP (acknowledge current behavior). The alert description includes anoc editcommand with the IC name populated from{{ $labels.name }}.Upgrade compatibility
isNewIngressControllercheck (status not yet initialized) ensures the PROXY default is only applied to brand new IngressControllers. Re-admitted ICs are not affected because their status is already initialized.IsProxyProtocolNeededreturns false, the desired service gets no target-group-attributes annotation, and the one-directional reconciliation inloadBalancerServiceChangedonly adds or updates the annotation — never removes it. Any user-set annotation (e.g., the KCS hairpin workaround) is left untouched.updatePublishingStrategydetects the type transition viapreviousStatusLBTypeand defaults to PROXY. This does not fire on upgrade because the status type was already NLB.Design
Dependencies
protocolfield to the APIservice.beta.kubernetes.io/aws-load-balancer-target-group-attributesannotation