e2e/service.go: remove same node nodeport test

[aauren]

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

Remove local pod -> local nodeport test from Services [It] should fallback to local terminating endpoints when there are no ready endpoints with externalTrafficPolicy=Local test.

How local external traffic policy influences behavior has been a hotly contested topic in the past. @danwinship created an issue several years ago for discussing exactly what the sig-network stance on this is here: #108526

In it, several sig-network members went back and forth, but ultimately decided that local pod -> LB IP and local pod -> local NodePort would remain exceptions from the "destination" interpretation of traffic policy.

However, in the case of local pod -> local NodePort the exception for this was primarily made because sig-network didn't want to disrupt kube-proxy's existing functionality. I think that this was ultimately the right call for kube-proxy, but I think that it still makes sense to allow other Kubernetes network providers to interpret this in a way that makes sense for them.

In kube-router's case, interpreting local pod -> local NodePort as external traffic not only keeps it consistent with the "destination" interpretation of traffic policy, but also keeps existing functionality in a decision similar to sig-network's WRT kube-proxy.

Given that:

• This e2e test is supposed to be testing traffic policy's termination logic and not the semantics of local pod -> local NodePort functionality
• That the current exception held by sig-network for local pod -> local NodePort is primarily held to keep kube-proxy consistent and not for it's ideological merits

I think that it would be beneficial to the community to remove this one line from the sig-network e2e battery of tests.

Special notes for your reviewer:

FYI @danwinship @aojea @andrewsykim

Existing conversation on this can be found in Slack thread: https://kubernetes.slack.com/archives/C09QYUH5W/p1709310311449749

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

[k8s-ci-robot]

Hi @aauren. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[aojea]

/ok-to-test

[aojea]

we need to update the comments above to explain what is going on, and also mention that this case is not tested with references to the discussions

[aojea]

ok, so this assumes that a pod serving in in nodeA should be reachable from a pod in nodeB that connectes to a nodePort in the same node .... 😄 a bit of a stretch I'm fine with this change, seems an artifact of kube-proxy implementation and at minimum debatable from a more fine interpretation

[danwinship]

I need to do some more updates to https://github.com/kubernetes-sigs/kpng/blob/master/doc/service-proxy.md. It mentions the LB IP case but doesn't say anything about NodePorts+eTP:Local.

Anyway, I agree that this is testing an artifact of kube-proxy's implementation that I don't think we actually agreed was supposed to be the behavior for all service proxy implementations. (Which means maybe we should make nftables not do it that way?)

Note that we already have a unit test for pod-to-eTP:Local-NodePort in iptables that asserts the same behavior as seen here, so removing this test will not leave us in danger of regressing that.

[aauren]

Updated the comments, let me know what you think.

[aojea]

/lgtm
/approve

#123622 (comment)

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: fa46ccdb88b938511b3c5498b21ff8b8c068ed93

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aauren, aojea

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• test/e2e/network/OWNERS [aojea]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@aauren: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-providerless	d205150	link	false	/test pull-kubernetes-e2e-gce-providerless

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.