test(e2e): make the in-sandbox Talos image cache reachable by tenant workers - #3254
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChanges the Talos image factory mirror selection to use a tenant-namespace reachability probe validating HTTP 206 byte-range support against the mirror's Service ClusterIP, replacing a localhost-based range check. Adds a Cilium egress policy, updated manifest/test documentation, and a new Bats test suite covering the probe and manifest split. ChangesTenant Egress Reachability Probe
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5966b98 to
1c779cf
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses deterministic failures in e2e tests caused by network reachability issues between tenant workers and the in-sandbox Talos image cache. By implementing a targeted network policy and a more accurate reachability probe, the changes ensure that the mirror is only utilized when it is fully accessible, falling back to the public factory otherwise to maintain CI stability. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances the E2E Talos image cache mechanism by verifying reachability from the tenant namespace before using the mirror. It introduces a CiliumClusterwideNetworkPolicy to allow tenant CDI importer Pods to egress to the mirror, applies this policy dynamically once Cilium is up, and runs a tenant-scoped probe Pod to verify end-to-end connectivity (specifically checking for a 206 Partial Content response). Additionally, it adds comprehensive unit tests in hack/talos-image-cache_test.bats to validate the manifest splitting and probe logic. The reviewer suggested performing the probe Pod deletion asynchronously using --wait=false to avoid blocking the E2E test runner.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The in-sandbox Talos image mirror was pointed at by tenant Kubernetes CRs whenever its Deployment was Available, but "Available" only proves the seed finished and the server answers on localhost. It does not prove a worker's CDI importer can reach the Service. Tenant namespaces run under a default-deny Cilium egress that permits the outside world and kube-dns but not an arbitrary kube-system Service, so the importer resolves the name yet its TCP connect to the ClusterIP is silently dropped. The disk import then never starts, no tenant node joins within the 12-minute deadline, and the kubernetes-* tests fail deterministically -- worse than the intermittent public-factory flake the mirror was meant to relieve. Replace the localhost range-probe with one that faces the exact same network path as a worker importer: a throwaway Pod in the tenant namespace, labelled cdi.kubevirt.io=importer, that fetches the seeded image from the Service ClusterIP with a Range request and expects a 206. Point tenants at the mirror only when that end-to-end check passes; otherwise fall back to the public factory, restoring the guarantee that the mirror can only help, never make CI worse. The resolver also installs the importer egress allow at point-of-use, because Cilium's CRDs do not exist yet when the mirror manifest is first applied before install. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
1c779cf to
c57b685
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/talos-image-cache_test.bats`:
- Around line 124-137: The overrides builder test only verifies part of the
restricted PSA security context, so it can miss regressions in
_talos_image_cache_probe_overrides. Update the test in the talos-image-cache
probe overrides suite to assert the remaining restricted fields emitted by the
builder, including runAsUser = 1000, seccompProfile.type = RuntimeDefault, and
capabilities.drop containing ALL, alongside the existing image, command,
runAsNonRoot, and allowPrivilegeEscalation checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68f9cc93-5a6b-4ed1-bae6-0b6e255eed55
📒 Files selected for processing (4)
hack/e2e-apps/talos-image-cache.shhack/e2e-install-cozystack.batshack/e2e-talos-image-cache.yamlhack/talos-image-cache_test.bats
Tenant namespaces run under a default-deny Cilium egress installed by the tenant chart, which lets a worker's CDI importer reach the outside world and kube-dns but nothing in kube-system. The in-sandbox image mirror is a kube-system Service, so without an explicit allow the importer's connect to its ClusterIP is dropped and the worker disk import never starts. Ship a tightly-scoped CiliumClusterwideNetworkPolicy in the mirror manifest that lets importer Pods in tenant-test egress to the mirror Pod in kube-system. Cilium unions allow rules, so this only adds a hole; scoping it to tenant-test (already under default-deny egress) means it never flips another namespace's posture, and the mirror Pod needs no ingress rule because it is selected by no ingress policy. The policy is kept out of the pre-install apply because that runs before Cilium's CRDs exist; the resolver in hack/e2e-apps/talos-image-cache.sh applies it at point-of-use once Cilium is up. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
c57b685 to
1a0680b
Compare
Supersedes the earlier kube-system probe on this branch, which shared the original localhost probe's flaw: it ran OUTSIDE the tenant egress policy, so it would report the ClusterIP reachable while a real importer in tenant-test stayed blocked (a false positive that would not have prevented the failure). Root cause (from #3254 by @lexfrei, verified against the failing run's crust-gather snapshot): tenant namespaces run a default-deny Cilium egress (allow-internal/external-communication + the <tenant>-egress CCNP) that permits the world entity and the tenant tree but NOT an arbitrary kube-system Service, so the CDI importer's TCP connect to the cache ClusterIP is silently dropped and no worker disk imports. Port of #3254 to the Chainsaw layout (hack/e2e-chainsaw/_lib/): - ship a tightly-scoped CiliumClusterwideNetworkPolicy (importer pods in tenant-test -> mirror pod in kube-system), applied at point-of-use once Cilium CRDs exist; excluded from the pre-Cilium install apply via yq. - replace the readiness probe with a throwaway pod in tenant-test labelled cdi.kubevirt.io=importer that curls the ClusterIP with a range request and must get 206 - the exact egress path a real importer faces. - port the unit test pinning the manifest split, the probe/policy label agreement, the strict 206 gate, and the --overrides JSON builder. Kept from this branch's prior commit: the (a2) DataVolume import-stage diagnostics in the node-join failure block (importer logs + a ClusterIP re-probe via talos_image_cache_diagnose), complementary to #3254. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
) The in-sandbox Talos image mirror (#3244) is unreachable by the tenant workers it serves, so the kubernetes-* e2e tests fail deterministically at the 12m node-join wait instead of relieving the public-factory flake (#3231). Root cause (from #3254 by @lexfrei, verified against the failing run's crust-gather snapshot): tenant namespaces run a default-deny Cilium egress (allow-internal/external-communication + the <tenant>-egress CCNP) that permits the world entity and the tenant tree but NOT an arbitrary kube-system Service, so a worker's CDI importer resolves talos-image-cache.kube-system.svc yet its TCP connect to the ClusterIP is silently dropped and no disk imports. #3244's gate never caught this because it probed the server over localhost inside the serve pod, which passes regardless of cross-pod reachability. Port of #3254 to the Chainsaw layout (hack/e2e-chainsaw/_lib/): - ship a tightly-scoped CiliumClusterwideNetworkPolicy (importer pods in tenant-test -> mirror pod in kube-system), applied at point-of-use once Cilium CRDs exist; excluded from the pre-Cilium install apply via yq. - replace the readiness probe with a throwaway pod in tenant-test labelled cdi.kubevirt.io=importer that curls the ClusterIP with a range request and must get 206 - the exact egress path a real importer faces; tenants use the mirror only when that end-to-end check passes, else fall back to the public factory, so it can only help, never make CI worse. - unit test pinning the manifest split, probe/policy label agreement, the strict 206 gate, and the --overrides JSON builder. Also adds (a2) DataVolume import-stage diagnostics to the node-join failure block in run-kubernetes.sh (importer logs + a ClusterIP re-probe via talos_image_cache_diagnose) so the import-stage failure mode is legible. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Reconciles the talos-image-cache conflicts created when #3254 merged to main: keep the Chainsaw _lib/ port (hack/e2e-chainsaw/_lib/talos-image-cache.sh, a superset of #3254's logic + the node-join diagnose helper), drop the BATS-era hack/e2e-apps/talos-image-cache.sh, and keep the _lib/-path versions of the mirror manifest, install bats, and unit test. Also picks up main's zizmor action-pinning (#3223, unblocks pre-commit/zizmor) and Talos v1.13.6 (#3240). Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
What this PR does
The in-sandbox Talos image cache added in #3244 is unreachable by the tenant workers it was meant to serve, so it makes the
kubernetes-latest/kubernetes-previouse2e tests fail deterministically instead of relieving the intermittent public-factory flake it targets (#3231). Every branch off currentmaininherits this — for example #3240 has failed its tenant-Kubernetes e2e repeatedly for this reason alone, despite being unrelated (it only touches host boot assets).Root cause: tenant namespaces run under a default-deny Cilium egress installed by the tenant chart. A worker's CDI importer is allowed to reach the outside world (so the public factory works) and kube-dns (so the name resolves), but nothing permits egress to an arbitrary
kube-systemService. The importer therefore resolvestalos-image-cache.kube-system.svcyet its TCP connect to the ClusterIP is silently dropped (dial tcp <clusterip>:80: i/o timeout), the DataVolume import never starts, the worker VM staysDataVolumeError, and no tenant node joins within the 12-minute deadline. #3244's readiness gate never caught this because it probed the server overlocalhostinside the serve Pod, which passes regardless of cross-Pod reachability.Two changes:
CiliumClusterwideNetworkPolicythat lets importer Pods intenant-testegress to the mirror Pod inkube-system. Cilium unions allow rules, so this only adds a hole; scoping it to a namespace that is already default-deny egress means it never flips another namespace's posture, and the mirror Pod needs no ingress rule because no ingress policy selects it. The policy is applied at point-of-use (not with the rest of the mirror manifest, which is applied before Cilium's CRDs exist).cdi.kubevirt.io=importer, that fetches the seeded image from the Service ClusterIP with a Range request and must get a206. Tenants are pointed at the mirror only when that end-to-end check passes; otherwise the harness falls back to the public factory. This restores the invariant feat(apps/kubernetes): overridable Talos image factory URL + in-sandbox e2e mirror #3244 intended — the mirror can only help, never make CI worse.Not a retry/timeout bump: it fixes the real network path and adds a genuine reachability gate.
Screenshots
N/A — CI-only change, no user-facing surface.
Release note
Summary by CodeRabbit
206) before using it.206gating/fallback scenarios.