fix(e2e): keep localdns reply-direction NOTRACK rules allowlisted - #9258
Conversation
PR #9247 reverted #9230, which removed the reply-direction localdns NOTRACK rules from localdns.sh *and* removed the matching entry from the eBPF host-routing iptables allowlist in e2e. Those two things do not take effect at the same time. localdns.sh is baked into the VHD by the packer `file` provisioner, it is not delivered through CSE custom data. So every VHD published while #9230 was on main still installs the reply-direction rules, and those VHDs remain in support for 6 months. The e2e allowlist, by contrast, changed the moment the revert merged. The result is that ValidateIPTablesCompatibleWithCiliumEBPF (run from ValidateCommonLinux) fails on any scenario booting one of those VHDs: Rule in table raw did not match any pattern: -A OUTPUT -s 169.254.10.10/32 -p tcp -m comment --comment "localdns: skip conntrack" -m tcp --sport 53 -j NOTRACK That took out all 18 real-node GPU scenarios in Agentbaker GPU E2E build 177266063 (A100, H100, A10, NC, MIG, DRA, device-plugin, ...), starting with build 177223847, the first run after the revert landed. Changes: * e2e/validation.go: restore the reply-direction pattern in the "raw" table. The allowlist describes what a node may legally have, not what main currently installs, so it has to cover the whole supported VHD window. This also means the entry is already in place if #9230 is re-landed. * e2e/validators.go: extract the matching loop into iptablesRuleMatchesAnyPattern and stop discarding the regexp.MatchString error. Previously a malformed allowlist entry silently became "no match", which surfaces as a confusing "unsupported iptables rule" failure on every scenario instead of pointing at the bad pattern. * e2e/validation_iptables_test.go: unit tests that assert every allowlist pattern compiles, that both localdns rule directions are allowlisted, that the allowlist still fails closed for an unknown rule, and that the helper behaves as documented. Removing the restored pattern makes Test_iptablesAllowlist_localdnsRules fail with the exact rules seen in the pipeline, so this is a real regression guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Windows Unit Test Results 3 files 13 suites 1m 21s ⏱️ Results for commit ebc1453. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Restores compatibility for supported VHDs containing LocalDNS reply-direction NOTRACK rules and improves allowlist diagnostics.
Changes:
- Restores the LocalDNS reply-rule allowlist pattern.
- Extracts matching logic with explicit regex error handling.
- Adds regression and helper unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
e2e/validation.go |
Restores reply-direction NOTRACK allowlisting. |
e2e/validators.go |
Adds reusable, error-aware rule matching. |
e2e/validation_iptables_test.go |
Adds allowlist regression and matching tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses two review findings. 1. The previous commit changed behaviour for patterns that are not valid regular expressions. regexp.MatchString returns (false, err) on a compile error, so the original inline loop fell through to the strings.Contains check and such a pattern could still match literally. Returning early on the error removed that fallback, which matters because this allowlist is deliberately fed rules pasted out of `iptables -S` -- a comment such as --comment "kube-proxy (v1.31" is not valid regex syntax. iptablesRuleMatchesAnyPattern now records the first compile error but keeps evaluating, so the literal-substring path still runs. The error is only returned when the rule went unmatched overall, which is exactly the case where the malformed pattern is the likely culprit. Three test cases cover this: literal match despite an invalid regex, no error when a later pattern matches, and the error surfacing when nothing matches. 2. e2e is a separate Go module, so `make test` in the root module never runs it and check-coverage.yml explicitly filters it out. The new tests could therefore only fail inside the ADO E2E pipeline they exist to protect, and could not block the PR that caused the regression. Added an e2e-unit-test job to the Go Unit Tests workflow, following the existing precedent in validate-components.yml, scoped to tests that need neither Azure credentials nor network. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
E2E evidence: the fix works
Every one of the 18 failures on The 2 remaining failures are unrelated infrastructure flakesBoth are
The other two pipelines are likewise unrelated to this change: All three pipelines have been re-queued (GPU 177378215, E2E 177378257, Windows 177378268). |
Re-run results — the two non-GPU pipelines are now explained
GPU E2E: 18 → 1, and the iptables signature is still gone
The single remaining failure is
Windows E2E failure is pre-existing on
|
Final CI evidence across three GPU E2E runsThe iptables regression this PR fixes is gone in every run. The residual failures are an Azure Bastion infrastructure problem that is also present on
Two independent signals:
The set of affected tests also shuffles between runs ( Other pipelines: |
Does this re-introduce what #9247 reverted? No — and here is the proofReasonable objection, so stating it explicitly. #9247 removed four things. This PR restores exactly one of them, and it is the only one with permissive rather than prescriptive semantics.
The polarity is the whole pointAn allowlist entry cannot cause a node to install a rule. It only says "if a node has this rule, do not fail it." Restoring it is incapable of re-enabling #9230's product behaviour. What actually triggered the revert was the other validatorBuild 177145051 ( So #9230 broke CI via This PR fixes the mirror image of that same VHD-skew bug: the test now forbids rules that VHDs still do have. The two fixes are complementary, not opposed:
Both are needed because |
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it
Agentbaker GPU E2Ehas been red onmainsince #9247 merged. All 18 real-node GPU scenarios (A100 x2, H100, A10 x2, NC, RTXPro6000, GridDriver, NoDriver, NvidiaDevicePlugin, MIG, DRA) fail on the same assertion fromValidateIPTablesCompatibleWithCiliumEBPF:Root cause
#9247 reverted #9230. That revert removed the reply-direction NOTRACK rules from
localdns.shand removed the matching entry from the e2e iptables allowlist. Those two removals do not take effect at the same time:localdns.shis baked into the VHD by the packerfileprovisioner (vhdbuilder/packer/*.json). It is not delivered through CSE custom data. A change to it only reaches a node when a new VHD is published.So every VHD published while #9230 was on
mainstill installs the reply-direction rules, and those VHDs stay in support for 6 months — but the test stopped accepting them immediately.Timeline (UTC):
ea63b859a0(#9230) adds reply rules + allowlist entry1.1787040551.32430published — rules baked in7716637881(#9247) reverts bothmainmay partially self-heal as post-revert VHDs roll out, but any run pinned toVHD_BUILD_IDin the 08-18 → 08-19 window, and any release branch still on those images, stays red. The allowlist is also required again the moment #9230 is re-landed.Changes
e2e/validation.go— restore the reply-direction pattern in therawtable. The allowlist describes what a node is permitted to have, not whatmaincurrently installs, so it must cover the whole supported VHD window.e2e/validators.go— extract the matching loop intoiptablesRuleMatchesAnyPatternand stop discarding theregexp.MatchStringerror. Previously a malformed allowlist entry silently degraded to "no match", which surfaces as a confusing "unsupported iptables rule" failure on every scenario rather than pointing at the bad pattern. The loop also assigned to the outermatchedvariable; the extraction removes that hazard. Matching semantics (regex OR literal substring, blank patterns skipped) are unchanged — note in particular that a pattern which is not valid regex syntax still gets its literal-substring chance, since the allowlist is deliberately fed rules pasted out ofiptables -S(a comment like--comment "kube-proxy (v1.31"is not a valid regex). The compile error is only surfaced when the rule went unmatched overall, i.e. exactly when the malformed pattern is the likely culprit..github/workflows/go-test.yml—e2e/is a separate Go module, somake testin the root module never reaches it andcheck-coverage.ymlexplicitly filters it out. Without this, the new tests could only fail inside the ADO E2E pipeline they exist to protect, and could not block the PR causing the regression. Added ane2e-unit-testjob modelled on the existingversion-consistencyjob invalidate-components.yml, scoped to tests needing neither Azure credentials nor network.e2e/validation_iptables_test.go(new) — pure unit tests, no Azure resources needed:Does this PR introduce a user-facing change?
Testing / verification
Product behaviour is unchanged — this only touches e2e test code.
Negative check — deleting the restored pattern reproduces the pipeline failure exactly:
Refs #9230, #9247. ADO build 177266063 (definition 443878,
Agentbaker GPU E2E).