tests: quarantine services nodeport w/ L7 policy test.

[tommyp1ckles]

Changes in c1a0dba may have introduced flakes for this set of tests when attempting to reach NodePort from outside the cluster.

A replacement test is being added to cilium cli connectivity tests:

cilium/cilium-cli#1547

Quarantine this test for now until we can remove it.

Addresses: #25119

[tommyp1ckles]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-host-2gs77

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-5.4/1945/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

Then please upload the Jenkins artifacts to that issue.

[tommyp1ckles]

/test-runtime

[tommyp1ckles]

/test-1.24-5.4

[tommyp1ckles]

/test-runtime

[jibi]

perhaps worth reassigning this to sig-policy in CODEOWNERS?

[julianwiedmann]

I don't follow :/.

• How did we determine that this is more than just the usual fallout from adding a new ginkgo test - where CI already runs the latest tests, but the PR branches are outdated and don't include the relevant fixes yet (as would be the case for eg. datapath: Remove 2005 route table for IPv4 #24807) ?
• we now quarantined the test in main, but CI: 1.13 flake: K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside #25119 was about the v1.13 branch. Are we still seeing the flakes in v1.13, and need to quarantine there as well?

(it's not helping that #25119 turned into a catch-all for similar fails in main, for both IPv4 and IPv6 😞)

[joestringer]

@tommyp1ckles @jschwinger233 any comments on @julianwiedmann 's questions above?

It looks to me like c1a0dba could have broken the test, potentially representing a real regression. The PR here doesn't explain why. Should we revert c1a0dba instead?

[jschwinger233]

@joestringer The quarantined ginkgo test in this PR is replaced by connectivity test in cli. As connectivity test passes, the ginkgo test failure could be a flake, instead of a regression, I think?

Should we revert c1a0dba instead?

The quarantined ginkgo test actually was introduced to cover c1a0dba, so reverting that commit won't help to resolve the issue.

Since we have connectivity test to cover the scenario, we can remove this ginkgo test case if that quarantine is bothering.

[joestringer]

👍 OK, then if we added the test to cover that commit and the test is buggy and we have the same coverage in another CLI test, we should just remove the buggy test (on both main and v1.13 branches, we can propose the change to delete the test on main branch and then label it for backport to v1.13). Would you mind following up on that @jschwinger233 ?

sidenote: I would strongly discourage describing something as "could be a flake". All failures are the result of either bugs in production code, or bugs in the testing code, or (rarely) infrastructure issues. When we as developers call something a "flake", in my experience this is often a shorthand that sounds like a reasonable motivation to ignore something and do nothing about it. Over time, ignored bugs in testing infrastructure stack up and make the overall system unreliable, decreasing developer trust in the system, and eventually leading to decreased development velocity because we can't reliably validate the functionality. We need to be vigilant about enforcing a high level quality on the testing code, and if it's buggy, fix it or delete it. It's not good enough to merge a buggy test and impose the cost of those failures on the rest of the development community.