cilium-cli: Validate NodePort readiness on all node IP addresses by pillai-ashwin · Pull Request #40812 · cilium/cilium

pillai-ashwin · 2025-07-30T08:19:15Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer's Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

Description

Fixes intermittent pod-to-local-nodeport connectivity test failures (exit code 28 - connection timeout) in dual-stack clusters.

Root Cause

The upfront NodePort readiness check in deployment.go only validated HostIP (one IP per node, typically IPv4), but tests iterate through all addresses in node.Status.Addresses[] which includes both IPv4 and IPv6 in dual-stack clusters.

This meant IPv6 NodePorts were never validated for readiness, causing timeouts when tests tried them.

Fix

Enhance the upfront NodePort validation to check all node IP addresses (NodeInternalIP and NodeExternalIP types) instead of just HostIP. This ensures all addresses that will be tested are validated during setup.

Fixes: #39793

Fix intermittent NodePort connectivity test timeouts in dual-stack clusters by validating NodePort readiness on all node IP addresses during test setup.

dylandreimerink

Insufficient timeouts: 2-second connect timeout was too aggressive for IPv6 NodePort connections

IPv6 networking delays: IPv6 connections can take longer to establish than IPv4

I am a bit skeptical about these findings. Establishing a connection should not take 2 seconds, especially when all of it is effective on the same node (no internet involved). If they do, then it seems to me that something is going wrong (packets being dropped, causing TCP level retires, or something related) and increasing the timeout may just mask that.

I also have never heard of IPv6 being slower than IPv4 to establish connections. I don't think we should accept that statement without something to back that up.

Missing retry logic: Single-attempt connections failed on transient network issues

This is essentially saying that we expect Cilium, the thing we are testing to fail. I don't think that is right. These sorts of tests are exactly here to catch issues in Cilium. If the first connection we make always fails or frequently fails then we should investigate why and not plaster over it with retries. We want all connections to work, not just at least one out of three.

Premature testing: Tests started before NodePorts were fully ready

This sounds more like the right direction. I think this is more likely to be the real fix.

cilium-cli/connectivity/tests/service.go

pillai-ashwin · 2025-09-03T22:13:02Z

/test

viktor-kurchenko

Thank you!

pillai-ashwin · 2025-09-12T19:55:25Z

@viktor-kurchenko are we good to go ahead with merge?

viktor-kurchenko · 2025-09-13T07:30:33Z

/test

viktor-kurchenko · 2025-09-13T09:56:00Z

@viktor-kurchenko are we good to go ahead with merge?

Yes, it's marked as ready-to-merge now.
Thanks for the contribution!

joestringer · 2025-12-01T18:02:09Z

I see there's been some updates, but the PR title and description (release note) both talk about dropping IPv6-specific timeouts and I don't see that in the code change.

Related, in the commit message:

This removes the need for per-test readiness checks.

If the need is removed, does that mean there should be another change to remove them? Does it make sense to track that with an issue and follow up to remove those checks or were you planning to look into that as part of this PR?

pillai-ashwin · 2025-12-09T04:55:20Z

@joestringer

I see there's been some updates, but the PR title and description (release note) both talk about dropping IPv6-specific timeouts and I don't see that in the code change.

Updated the pr title, description, commit message and release note. The current approach is simpler:

The fix enhances the existing upfront NodePort validation to check all node IP addresses (NodeInternalIP and NodeExternalIP) instead of just HostIP. This covers IPv6 addresses in dual-stack clusters that were previously unchecked and were causing the intermittent timeouts.

If the need is removed, does that mean there should be another change to remove them? Does it make sense to track that with an issue and follow up to remove those checks or were you planning to look into that as part of this PR?

In an earlier version of this PR I added per-test WaitForNodePorts() calls before each curl, but I dropped that approach to favor fixing the upfront check directly. There are no per-test checks in main to remove, so no follow-up needed.

viktor-kurchenko · 2025-12-09T16:54:04Z

/test

The upfront NodePort readiness check only validated HostIP (typically one IP per node), but tests target all addresses in node.Status.Addresses which can include multiple IPs per node (e.g., IPv4 and IPv6 in dual-stack clusters). This caused intermittent failures (exit code 28) when tests tried IPv6 NodePorts that were never validated for readiness. Fix by enhancing the upfront validation in deployment.go to check NodePort readiness on all node IP addresses (InternalIP and ExternalIP types) during the setup phase, ensuring all addresses that will be tested are ready before tests execute. Fixes: cilium#39793 Signed-off-by: Ashwin Pillai <[email protected]>

pillai-ashwin · 2025-12-13T03:36:39Z

/test

julianwiedmann · 2026-01-26T10:11:44Z

Looks like this resulted in #43749.

PR cilium#40812 added validation for NodePorts on all node IP addresses (internal and external) to fix dual-stack clusters. However, on GKE, external IPs are not reachable from inside the cluster due to firewall restrictions. The connectivity tests already skip external IPs on GKE (service.go), but the upfront validation added by cilium#40812 did not, causing timeouts during test setup. This commit adds the same GKE-aware external IP skip logic to the NodePort validation phase, making it consistent with the actual test behavior. Fixes: cilium#43749 See-also: cilium#39793 Signed-off-by: Ash <[email protected]>

PR cilium#40812 added validation for NodePorts on all node IP addresses (internal and external) to fix dual-stack clusters. However, on GKE, external IPs are not reachable from inside the cluster due to firewall restrictions. The connectivity tests already skip external IPs on GKE, but the upfront validation added by cilium#40812 did not, causing timeouts during test setup. This commit adds the same GKE-aware external IP skip logic to the NodePort validation phase, making it consistent with the actual test behavior. Fixes: cilium#43749 See-also: cilium#39793 Signed-off-by: Ashwin Pillai <[email protected]>

PR #40812 added validation for NodePorts on all node IP addresses (internal and external) to fix dual-stack clusters. However, on GKE, external IPs are not reachable from inside the cluster due to firewall restrictions. The connectivity tests already skip external IPs on GKE, but the upfront validation added by #40812 did not, causing timeouts during test setup. This commit adds the same GKE-aware external IP skip logic to the NodePort validation phase, making it consistent with the actual test behavior. Fixes: #43749 See-also: #39793 Signed-off-by: Ashwin Pillai <[email protected]>

pillai-ashwin requested review from a team as code owners July 30, 2025 08:19

pillai-ashwin requested review from dylandreimerink and viktor-kurchenko July 30, 2025 08:19

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 30, 2025

pillai-ashwin requested a review from derailed July 30, 2025 08:19

github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary labels Jul 30, 2025

dylandreimerink requested changes Aug 1, 2025

View reviewed changes

cilium-cli/connectivity/tests/service.go Outdated Show resolved Hide resolved

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from 4538a18 to 4527a2f Compare August 8, 2025 00:32

pillai-ashwin changed the title ~~cilium-cli: Fix IPv6 NodePort connectivity test timeouts~~ cilium-cli: NodePort tests wait for readiness; drop IPv6-specific timeouts Aug 8, 2025

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch 2 times, most recently from a2d1e38 to 083d5d1 Compare August 8, 2025 00:55

pchaigno added the release-note/ci This PR makes changes to the CI. label Aug 15, 2025

pchaigno requested a review from dylandreimerink August 15, 2025 16:00

pchaigno removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 15, 2025

dylandreimerink approved these changes Aug 25, 2025

View reviewed changes

derailed approved these changes Aug 25, 2025

View reviewed changes

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from 083d5d1 to d60f535 Compare August 26, 2025 21:01

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from d60f535 to ab2dc2a Compare September 3, 2025 21:34

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from ab2dc2a to 4ee5bec Compare September 4, 2025 16:59

viktor-kurchenko approved these changes Sep 5, 2025

View reviewed changes

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from 4ee5bec to c0ea472 Compare September 6, 2025 19:49

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from c0ea472 to 707c054 Compare September 12, 2025 19:55

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 Sep 13, 2025

pillai-ashwin changed the title ~~cilium-cli: NodePort tests wait for readiness; drop IPv6-specific timeouts~~ cilium-cli: Validate NodePort readiness on all node IP addresses Dec 9, 2025

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from 2b00fb3 to 2c0b93c Compare December 9, 2025 04:57

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from 2c0b93c to 6f7772e Compare December 12, 2025 05:05

pillai-ashwin force-pushed the fix/ci-flake-nodeport-ipv6-timeout-39793 branch from 6f7772e to 9607dac Compare December 13, 2025 02:59

joestringer approved these changes Jan 5, 2026

View reviewed changes

joestringer removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Jan 5, 2026

joestringer enabled auto-merge January 5, 2026 01:18

joestringer added this pull request to the merge queue Jan 5, 2026

Merged via the queue into cilium:main with commit 0b64c92 Jan 5, 2026
76 checks passed

maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jan 5, 2026

gandro mentioned this pull request Jan 20, 2026

cilium-cli: Fix NodePort deployment check in dual-stack clusters #43888

Merged

julianwiedmann mentioned this pull request Jan 26, 2026

Conformance GKE: timeout reached waiting for NodePort #43749

Closed

pillai-ashwin mentioned this pull request Jan 26, 2026

cilium-cli: Skip external IP NodePort validation on GKE #44014

Merged

8 tasks

cilium-release-bot bot added this to cilium v1.19.0 Feb 3, 2026

cilium-release-bot bot moved this to Released in cilium v1.19.0 Feb 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cilium-cli: Validate NodePort readiness on all node IP addresses#40812

cilium-cli: Validate NodePort readiness on all node IP addresses#40812
joestringer merged 1 commit intocilium:mainfrom
pillai-ashwin:fix/ci-flake-nodeport-ipv6-timeout-39793

pillai-ashwin commented Jul 30, 2025 •

edited

Loading

Uh oh!

dylandreimerink left a comment •

edited

Loading

Uh oh!

Uh oh!

pillai-ashwin commented Sep 3, 2025

Uh oh!

viktor-kurchenko left a comment

Uh oh!

pillai-ashwin commented Sep 12, 2025

Uh oh!

viktor-kurchenko commented Sep 13, 2025

Uh oh!

viktor-kurchenko commented Sep 13, 2025

Uh oh!

joestringer commented Dec 1, 2025

Uh oh!

pillai-ashwin commented Dec 9, 2025 •

edited

Loading

Uh oh!

viktor-kurchenko commented Dec 9, 2025

Uh oh!

pillai-ashwin commented Dec 13, 2025

Uh oh!

Uh oh!

julianwiedmann commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

pillai-ashwin commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Root Cause

Fix

Uh oh!

dylandreimerink left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pillai-ashwin commented Sep 3, 2025

Uh oh!

viktor-kurchenko left a comment

Choose a reason for hiding this comment

Uh oh!

pillai-ashwin commented Sep 12, 2025

Uh oh!

viktor-kurchenko commented Sep 13, 2025

Uh oh!

viktor-kurchenko commented Sep 13, 2025

Uh oh!

joestringer commented Dec 1, 2025

Uh oh!

pillai-ashwin commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viktor-kurchenko commented Dec 9, 2025

Uh oh!

pillai-ashwin commented Dec 13, 2025

Uh oh!

Uh oh!

julianwiedmann commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pillai-ashwin commented Jul 30, 2025 •

edited

Loading

dylandreimerink left a comment •

edited

Loading

pillai-ashwin commented Dec 9, 2025 •

edited

Loading