Skip to content

ginkgo: remove ClusterIP can be accessed when external access is enabled#44193

Merged
julianwiedmann merged 3 commits intomainfrom
pr/smagnani96/ginkgo-2
Feb 20, 2026
Merged

ginkgo: remove ClusterIP can be accessed when external access is enabled#44193
julianwiedmann merged 3 commits intomainfrom
pr/smagnani96/ginkgo-2

Conversation

@smagnani96
Copy link
Copy Markdown
Contributor

@smagnani96 smagnani96 commented Feb 4, 2026

Complementary PR of #44192 ✂️

Pleaser refer to commit messages:

  1. Add script test for the complementary behavior (SVC ClusterIP routable)
  2. Add bpf unit test for IPv4/6
  3. Remove Ginkgo test.

Related: #44168.

@smagnani96 smagnani96 self-assigned this Feb 4, 2026
@smagnani96 smagnani96 added kind/enhancement This would improve or streamline existing functionality. area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. labels Feb 4, 2026
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch 2 times, most recently from cd02664 to 05279f1 Compare February 4, 2026 16:10
@smagnani96
Copy link
Copy Markdown
Contributor Author

/test

@julianwiedmann
Copy link
Copy Markdown
Member

Should this come with a matching BPF test, which plugs in such a SVC entry and validates the datapath?

@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch from 05279f1 to 295f29b Compare February 10, 2026 16:09
@smagnani96 smagnani96 changed the base branch from main to pr/smagnani96/ginkgo-1 February 10, 2026 16:09
@smagnani96
Copy link
Copy Markdown
Contributor Author

Should this come with a matching BPF test, which plugs in such a SVC entry and validates the datapath?

I checked and I believe we already cover this, basically in every BPF unit test where we create the service with SVC_FLAG_ROUTABLE (e.g., tc_nodeport_lb4_nat_backend.c).

@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-1 branch from 48a699f to 5a5654a Compare February 10, 2026 16:40
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch 2 times, most recently from eeeca1f to b474c2b Compare February 10, 2026 19:30
@julianwiedmann
Copy link
Copy Markdown
Member

Should this come with a matching BPF test, which plugs in such a SVC entry and validates the datapath?

I checked and I believe we already cover this, basically in every BPF unit test where we create the service with SVC_FLAG_ROUTABLE (e.g., tc_nodeport_lb4_nat_backend.c).

ack, makes sense! That means those tests are currently all testing the ClusterIP case (that's a bit unexpected ...), since nothing is setting SVC_FLAG_NODEPORT or similar.

But that's a future improvement for the test suite, nothing to worry about right now. Did you also find a test for the negative path (drops in the N/S path when the ROUTABLE flag is not set) ?

@smagnani96
Copy link
Copy Markdown
Contributor Author

ack, makes sense! That means those tests are currently all testing the ClusterIP case (that's a bit unexpected ...), since nothing is setting SVC_FLAG_NODEPORT or similar.

I think the SVC_FLAG_NODEPORT is relevant only for bpf_sock and bpf_lxc.
In the DP, we only check if SVC_FLAG_ROUTABLE is set https://github.com/cilium/cilium/blob/v1.19/bpf/lib/nodeport.h#L2766-L2767, otherwise we return DROP_IS_CLUSTER_IP (even if the dst addr was not a cluster IP).

But that's a future improvement for the test suite, nothing to worry about right now. Did you also find a test for the negative path (drops in the N/S path when the ROUTABLE flag is not set) ?

I'm adding in the 1st PR of this patch series #44192, both IPv4/6.

@julianwiedmann
Copy link
Copy Markdown
Member

I think the SVC_FLAG_NODEPORT is relevant only for bpf_sock and bpf_lxc.
In the DP, we only check if SVC_FLAG_ROUTABLE is set https://github.com/cilium/cilium/blob/v1.19/bpf/lib/nodeport.h#L2766-L2767, otherwise we return DROP_IS_CLUSTER_IP (even if the dst addr was not a cluster IP).

oh yeah absolutely. I just would have expected most of our existing NS BPF tests to actually insert proper Nodeport services. Even if that shouldn't make much difference in the current code behavior.

This sort of subtle drift between controlplane and datapath test behavior is hard to avoid of course.

@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-1 branch from 5a5654a to 2cea2c0 Compare February 11, 2026 14:24
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch from b474c2b to 4ea0b13 Compare February 11, 2026 14:28
@julianwiedmann julianwiedmann self-requested a review February 11, 2026 14:30
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-1 branch 2 times, most recently from be419c9 to 7df91c6 Compare February 11, 2026 21:19
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch 2 times, most recently from 8074fb5 to 545cee4 Compare February 11, 2026 22:40
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-1 branch 2 times, most recently from 2302321 to bed38a9 Compare February 16, 2026 11:58
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch from 545cee4 to 0eefe2c Compare February 16, 2026 12:20
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-1 branch 2 times, most recently from 0d56d14 to 262bf7f Compare February 16, 2026 17:59
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch from 0eefe2c to 7b9a43d Compare February 16, 2026 18:06
@smagnani96 smagnani96 added the dont-merge/blocked Another PR must be merged before this one. label Feb 17, 2026
Base automatically changed from pr/smagnani96/ginkgo-1 to main February 17, 2026 18:19
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch from 7b9a43d to 22611ac Compare February 18, 2026 14:14
@smagnani96 smagnani96 removed the dont-merge/blocked Another PR must be merged before this one. label Feb 18, 2026
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch 2 times, most recently from 6fa9df7 to ece09b6 Compare February 18, 2026 14:46
This commit adds a testdata file for the ClusterIP when external access
is enabled. This basically takes the first steps of `clusterip.txtar`,
but enabled the `--bpf-lb-external-clusterip=true` flag, so we expect
the ClusterIP to be routable and have the `ClusterIP+sessionAffinity` flag
when displayed. In addition, we enable Maglev as LB algorithm, as we
are doing in the Ginkgo test.

We therefore expect from the LB map something like:

```
...
MAGLEV: ID=1 INNER=[1(1021)]
SVC: ID=1 ADDR=10.96.50.104:80/TCP SLOT=0 LBALG=undef AFFTimeout=42 COUNT=1 QCOUNT=0 FLAGS=ClusterIP+sessionAffinity
...
```

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit adds a BPF unit test for loadbalancing of external packets
destined to ClusterIP service. This aims at replacing the Ginkgo
"ClusterIP can be accessed when external access is enabled" test.

Differently than testing a non-routable ClusterIP service, in this BPF
test we enable the SVC_FLAG_ROUTABLE flag when creating the SVC in the
bpf map. Therefore, we expect the packet to be correctly handled, DNATed,
and conntrack to track the connections.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
…bled`

This commit removed the Ginkgo test in favor of the script test and BPF
tests introduced in the previous commits.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/ginkgo-2 branch from ece09b6 to f79c43f Compare February 19, 2026 18:58
@smagnani96
Copy link
Copy Markdown
Contributor Author

/test

@smagnani96 smagnani96 marked this pull request as ready for review February 19, 2026 22:25
@smagnani96 smagnani96 requested review from a team as code owners February 19, 2026 22:26
Copy link
Copy Markdown
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

yay, thank you!

@julianwiedmann julianwiedmann added area/loadbalancing Impacts load-balancing and Kubernetes service implementations area/kpr Anything related to our kube-proxy replacement. ci/hyperjump Relates to 2022 test improvement initiative. labels Feb 20, 2026
@julianwiedmann julianwiedmann added this pull request to the merge queue Feb 20, 2026
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 20, 2026
Merged via the queue into main with commit b14ea88 Feb 20, 2026
491 checks passed
@julianwiedmann julianwiedmann deleted the pr/smagnani96/ginkgo-2 branch February 20, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/CI Continuous Integration testing issue or flake area/kpr Anything related to our kube-proxy replacement. area/loadbalancing Impacts load-balancing and Kubernetes service implementations ci/hyperjump Relates to 2022 test improvement initiative. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants