-
Notifications
You must be signed in to change notification settings - Fork 3.4k
CI: Use fake external target in LVH-based workflows #40640
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.
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
Conversation
gentoo-root
commented
Jul 22, 2025
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you for the work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments below, but none of them blocking.
You have a merge conflict to address, though.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably have less background on the recent work for external targets than other reviewers and, maybe because of that, I'm currently unable to review the main changes here.
I think this pull request and its commits need more context, to help me review and future debuggers understand. I've left comments to try and state what is unclear to me, but in general I'd expect the Why to be stated for each commit and, if the changes don't trivially relate to the commit's title/description, additional explanation for what's non-obvious (cf. Quentin's comment for an example).
152914a
to
6312e43
Compare
9f0b54b
to
7dfe134
Compare
/test |
7dfe134
to
1576a7b
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Maxim! The new commit descriptions help a lot for review and it all looks good to me :)
.github/actions/kind-external-network/action.sh passes KIND_EXPERIMENTAL_DOCKER_NETWORK to kind, but our wrapper script overwrites it unconditionally and doesn't provide a way to change it. Make the script check whether KIND_EXPERIMENTAL_DOCKER_NETWORK might be already set. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
Canonicalize IP6EXTERNALRANGE to fix this warning: [=] [cilium-test-2] Test [all-egress-deny-knp] [3/21] I0526 13:40:09.838874 1 warnings.go:110] "Warning: spec.egress[0].to[1].ipBlock.cidr: IPv6 CIDR value \"fd00:10:64::ffff:00/112\" should be in RFC 5952 canonical format (\"fd00:10:64::ffff:0/112\")" Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
Recently, we started replacing real external targets (such as one.one.one.one) in test workflows with fake ones to increase robustness in case the servers that we don't control go down. Commit 6982ed9 (".github/workflows: Use fake targets in conformance-kind-proxy-embedded") mentions that the new GitHub actions kind-external-network and kind-external-targets don't work with little-vm-helper. The reason is that those actions, as implemented initially, called into Docker from the GHA context, but Docker runs inside a VM in LVH-based workflows. The change made here allows those GitHub actions to SSH into the VM before running Docker commands. To support Docker volume mounts both on the host and inside the VM, use relative paths and cd /host in the VM to get into the same directory. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
Enabling fake external targets in LVH-based workflows makes this IPv6 NodePort test fail: north-south-loadbalancing-with-l7-policy-port-range/outside-to-nodeport tcp_checksum_complete fails in tcp_v6_rcv, and TCP_MIB_CSUMERRORS increases with every IPv6 SYNACK. While it's not clear why it passed in other workflows and how setting the bridge name explicitly affects the checksums, adding this magic line is sufficient to make this test pass in LVH-based workflows. The bridge name chosen is not arbitrary, and it matches the config used by contrib/scripts/kind.sh. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
check-encryption-leaks.bt treats everything under fd00::/8 as pod traffic. This CIDR includes the actual external and node-to-node traffic in configurations with fake external target. Change the IPv6 subnet in those configurations to match the default used by contrib/scripts/kind.sh. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
.github/actions/kind-external-targets/action.sh patches the cluster-wide DNS to be able to resolve the hostnames of the fake external targets in the cilium TLD. Some LVH-based workflows use node-local DNS that doesn't know about the cilium TLD, therefore, it'll fail to resolve the fake external targets once they are enabled for LVH-based workflows. Patch the node-local DNS config to forward requests in the cilium TLD to the cluster-wide DNS server, which is capable of resolving the hostnames of the fake external targets. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
EXTRA_CLI_FLAGS is formed as an array, each element corresponding to a command line argument. However, if an argument contains spaces, we have to quote the element of the array manually. Alleviate this burden by applying shell quoting with @q when expanding the array into a string. Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
1576a7b
to
6c96b33
Compare
/test |