Set CNI installMode to CalicoOnly for Windows FV#236
Conversation
There was a problem hiding this comment.
🔎 Council of Claudes — Correctness
Note
Correctness lens · bugs · completeness · concurrency · edge cases
1 major correctness/completeness issue: the diff contains no functional change implementing the stated intent. As-is, this PR will not set installMode: CalicoOnly for Windows FV and will not fix the ImagePullBackOff described.
- The only change in the diff is the addition of an empty file (.casey-mirror-13014-v2). There is no modification to the Windows FV installation manifests or Helm values to set Installation.spec.cni.installMode: CalicoOnly.
- To meet the PR’s intent, you need to update the Installation CR (or Helm values) that the Windows FV harness applies so that the operator renders without the third-party CNI init container. For example, ensure the Installation used during Windows FV includes:
spec:
cni:
installMode: CalicoOnly - Verify that the operator version used by the Windows FV path supports the installMode field and that the field path/name matches that version; otherwise the change will be ignored.
- Scope the change to the Windows FV flow only to avoid unintended impact on other test flows that may require the upstream plugins.
No line-specific findings (the provided annotated diff has no relevant changed lines to anchor).
🤖 Council of Claudes · 0 inline comment(s)
There was a problem hiding this comment.
🧪 Council of Claudes — Maintainability & Tests
Tip
Maintainability & Tests lens · simplicity · tests · docs · idioms
Missing actual change; stray file committed; suggest adding a scoped comment and a small guard in the Windows FV setup
- The annotated diff contains only a new empty file (.casey-mirror-13014-v2) and does not include the described change to set installMode: CalicoOnly. From a maintainability and tests perspective, this looks like an unintended commit and the substantive change is missing. Please verify the correct file(s) are updated and committed (likely the Windows FV bring-up manifests/values under hack/test/kind or the FV-specific Installation resource).
- Avoid leaving repo-top ad hoc marker files (.casey-mirror-*) around — they add noise and future confusion. Prefer local notes or a PR description link; if such files are required for tooling, document and ignore them via .gitignore with a brief comment.
- When you add the intended change, include a brief code comment near the place where installMode: CalicoOnly is set, explaining it’s intentionally scoped to Windows FV to avoid pulling upstream CNI plugins now rendered as a separate initContainer by the operator. Link to the operator PR or a short rationale so future maintainers don’t “optimize” it away.
- Scope containment: ensure the change applies only to the Windows FV path and does not affect general kind bring-up or other FVs. If you’re modifying shared values/manifests, consider parameterizing them or adding a Windows-FV-specific overlay.
- Tests/regression: since this fixes a break in the Windows FV setup, the FVs themselves will exercise the path. For resilience, consider adding a lightweight guard in the Windows FV environment setup that asserts one of:
- The Installation in the test cluster has installMode: CalicoOnly; or
- The calico-node DaemonSet in Windows FV clusters has no cni-plugins initContainer.
This turns a future operator behavior change into a fast-fail with a clear message, rather than a long timeout when nodes never become Ready.
- If any manifests are generated as part of this change, regenerate and commit them per repo rules. Also sanity-check the Windows FV docs/instructions to ensure they no longer mention importing the third-party-cni-plugins image.
🤖 Council of Claudes · 0 inline comment(s)
There was a problem hiding this comment.
🛡️ Council of Claudes — Security
Caution
Security lens · validation · secrets · authz · isolation
No security concerns found. The change described (setting installMode: CalicoOnly for Windows FV) reduces dependencies in a test-only path and does not weaken runtime isolation or policy enforcement. Please ensure the installMode change is scoped strictly to the FV/test harness and not propagated to production/operator defaults or user-facing manifests.
- Verify that disabling upstream CNI plugins is only applied in Windows FV context and cannot leak into non-test deployments.
- Confirm no tests rely on upstream plugins’ behavior (e.g., portmap) in a way that might accidentally mask a security regression. Skipping third-party plugins generally reduces attack surface in the FV environment.
No line-specific findings.
🤖 Council of Claudes · 0 inline comment(s)
There was a problem hiding this comment.
Council of Claudes — Nell
Important
Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments
No functional changes in diff; likely missing the intended change and one stray file.
I think the PR description makes sense and sounds like the right fix for the Windows FV flow, but the diff only adds an empty file .casey-mirror-13014-v2 and doesn’t include any change setting installMode: CalicoOnly. Suspect something didn’t get committed/pushed?
A few checks/questions while you update the diff:
- Where are we setting
installMode: CalicoOnly— Helm values under hack/test/kind/infra, or the Installation CR that the FV harness applies? Let’s scope it so it only affects the Windows FV path and doesn’t change Linux FV/kind defaults. - Do we need to regen any generated manifests if this touches charts? If it’s only test harness config, then no.
- Tests: since this is FV infra, I don’t think we need a new test, but would it be worth a tiny harness assertion (non-blocking) that the calico-node DaemonSet for the Windows FV does not include the upstream CNI init container? Feel free to defer.
- Release note “None” and docs-not-required both seem appropriate here.
nit: Please drop the .casey-mirror-13014-v2 file from the PR — looks like local scaffolding?
No line-specific findings (no [L] lines in the diff).
🤖 Council of Claudes · 0 inline comment(s)
There was a problem hiding this comment.
Council of Claudes — Casey
Warning
Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness
No effective changes in this diff — I think the actual change is missing; a single empty file was added.
- I think something went wrong with the commit here — the description talks about setting Installation CNI installMode=CalicoOnly for the Windows FV flow, but the diff only adds an empty file. Can you double-check and push the intended changes? I’d expect to see something under hack/test/kind/infra (operator values), the Windows FV harness, or wherever we render the operator Installation for FV.
- On the approach: I’m not necessarily against using CalicoOnly for the Windows FV — seems reasonable to avoid pulling an image we don’t build/import for that job. A couple of questions to make sure we don’t mask issues:
- Which file actually sets installMode: CalicoOnly, and is it scoped solely to the Windows FV path so we don’t impact other FV or kind flows?
- Do we have any Linux-side pods in the Windows FV run that rely on upstream plugins (e.g., host-local/portmap) that would break under CalicoOnly? If not, great — just want to sanity check the coupling.
- If the operator API field is new-ish (CNI.InstallMode?), is it compatible with the operator version we pin in this repo? If there’s a version floor, can we enforce/fail fast in the FV harness with a clear message rather than silently continuing and timing out?
- If the prior behavior caused calico-node to sit Pending due to ImagePullBackOff until a global timeout, I think we should tighten that loop in the FV harness: fail loudly with a specific reason when we detect an unexpected init container or missing image, rather than waiting for a generic readiness timeout. WDYT?
- If this change alters the Windows FV bring-up instructions, a brief note in the relevant CLAUDE/AGENTS doc or hack/docs for Windows FV might help future you (not blocking).
No line-specific findings — there are no substantive changed lines in the diff.
🤖 Council of Claudes · 0 inline comment(s)
The operator now renders the upstream CNI plugins as a separate cni-plugins init container on the calico-node DaemonSet. The Windows FV local-build flow only imports calico/calico, calico/node, and calico/operator onto the Linux node, so the new init container hits ImagePullBackOff, calico-node never goes Ready, and the install times out before any Felix Windows FV spec runs.
The Windows FV doesn't need the upstream plugins on the Linux node, so set
installMode: CalicoOnlyon the Installation to skip the init container altogether. This avoids having to build and import the third-party-cni-plugins image.Alternative to projectcalico#13013, which fixes the same breakage by building and importing the image instead.