perf(networking): Cilium ENI native routing as opt-in --cni=cilium datapath

[rustyrazorblade]

Closes #805

Draft — implementation in progress. The unit tier runs locally; the full suite runs in CI on each push.

Lands Cilium ENI IPAM native routing as an opt-in datapath, selected via --cni=<cilium|flannel> (default flannel). Default-to-Cilium flip is tracked in #819.

Scope update (post-rebase onto #858): the base-AMI packer script (configure_cilium_eni_networkd.sh + base.pkr.hcl/docker-compose.yml/README.md wiring), the IMDS hop-limit-2 fix applied to all node types (EC2InstanceService.kt), and the --cni=<cilium|flannel> enum flag itself (CniMode in ClusterState.kt, PicoCniModeConverter, Init.kt/Up.kt wiring) landed separately via #858 and are no longer part of this diff.

What remains here is just the actual, still-unvalidated Cilium install logic:

• CiliumService.install()'s ENI native-routing flag set (ipam.mode=eni, routingMode=native, ipv4NativeRoutingCIDR, kubeProxyReplacement=false pinning, iptables masquerade, the Hubble metrics brace-quoting fix)
• the two-argument install(controlHost, vpcCidr) signature
• Up.installCilium()'s VPC-CIDR threading into that call
• CiliumServiceTest.kt and the Up/Init test coverage for the above
• the openspec/changes/cilium-native-routing/ design docs

Live cross-AZ validation (design doc task 6.2) is still outstanding — that's the risk this PR hasn't yet proven out.

[github-actions]

Code Coverage

File	Coverage [88.75%]	✅
src/main/kotlin/com/rustyrazorblade/easydblab/commands/Init.kt	78.00%	❌
src/main/kotlin/com/rustyrazorblade/easydblab/commands/Up.kt	85.95%	✅
src/main/kotlin/com/rustyrazorblade/easydblab/configuration/ClusterState.kt	100.00%	✅
src/main/kotlin/com/rustyrazorblade/easydblab/services/CiliumService.kt	100.00%	✅

Total Project Coverage	73.52%	❌

[claude]

Review: cilium-native-routing OpenSpec proposal

This PR only adds OpenSpec artifacts (proposal/design/spec/tasks) - no code changes yet, consistent with the draft status. Overall the design is well-researched: I spot-checked the concrete claims against the current worktree (CiliumService.kt, EC2InstanceService.kt, ClusterState.kt, Up.kt, start-k3s-server.sh) and they all check out - the unquoted hubble.metrics.enabled={...} brace-expansion bug, the if (serverType == ServerType.Control) IMDS gating, ciliumEnabled: Boolean = false at ClusterState.kt:111, resolveCidr/startK3sOnAllNodes line references, and the --flannel-backend=none --disable-network-policy args in start-k3s-server.sh are all exactly as described.

Bug: proposal.md contradicts itself (and design.md/tasks.md) on the default CniMode

The Why section and What Changes bullet (line 16) are explicit that the default stays flannel: '--cni=flannel (the default) selects K3s built-in Flannel. The default-to-Cilium flip is deferred to #819.'

This matches design.md Decision 1 ('Default stays flannel this patch (owner decision)') and tasks.md 1.2/1.3 (InitConfig.cni: CniMode = CniMode.Flannel, Init.cni: CniMode = CniMode.Flannel, default flannel).

But the Impact section of proposal.md says the opposite in two places:

• Line 30: commands/Init.kt - --cilium boolean replaced by --cni=<cilium|flannel> (default cilium)
• Line 31: configuration/ClusterState.kt - new CniMode enum; InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium; ...

Since proposal.md is what gets carried forward into implementation (and archived into the spec), this is worth fixing before merge so whoever picks up tasks.md does not cross-reference the Impact section and flip the default by accident. Should be a one-line find/replace (CniMode.Cilium to CniMode.Flannel, default cilium to default flannel) on lines 30-31.

Other notes (non-blocking):

• design.md's Verified no-change section explicitly calls out the missing ICMP security-group rule and updates the acceptance scenarios (spec.md) to use curl/nc instead of ping - good catch, keeps the spec honest about what live validation can actually exercise.
• The CLAUDE.md correction task (5.1: fix the K3s uses Cilium (not Flannel) line in the root CLAUDE.md) is a real, verified doc bug independent of this change - good to see it's in scope rather than left dangling.
• Decision 5 (keep kubeProxyReplacement off) and Decision 3's explicit rejection of autoDirectNodeRoutes/egressMasqueradeInterfaces are well-justified and appropriately scoped - good use of non-goals to keep this change reviewable.
• tasks.md 4.4 only mentions asserting the IMDS metadata options for db/app/control specs - worth double-checking during implementation whether there are other ServerType values (e.g. any monitoring/bastion node type) that should also get hop-limit 2, since the fix's whole point is every node type, no exceptions.

No security, performance, or test-coverage concerns beyond the above - this is a docs-only PR and the actual code changes will need their own review once implemented per tasks.md.

[claude]

Reviewed the diff (Init/Up/CiliumService/EC2InstanceService + tests + openspec artifacts). Overall this is a well-scoped, well-tested change — the enum-based --cni option is a clean replacement for the boolean flag, the Hubble brace-expansion fix and IMDS hop-limit fix are both real bugs worth fixing regardless of the ENI work, and the new/updated tests (CiliumServiceTest, EC2InstanceServiceTest, InitTest) actually exercise the new behavior rather than echoing mocks. A few things worth addressing before merge:

1. proposal.md Impact section contradicts the rest of the change (and the code)
openspec/changes/cilium-native-routing/proposal.md Impact says commands/Init.kt replaces --cilium boolean with --cni=<cilium|flannel> "(default cilium)", and that InitConfig.ciliumEnabled: Boolean is replaced by InitConfig.cni: CniMode = CniMode.Cilium.

This directly contradicts design.md Decision 1 ("default flannel... Default stays flannel this patch"), tasks.md 1.2/1.3, the new specs own scenario ("Default provision uses Flannel"), and the actual code (CniMode.Flannel is the real default). Looks like a leftover from an earlier draft of the proposal before the default-flip was deferred to #819. Since specs are the source of truth here, worth fixing so a future reader does not get the wrong idea about what shipped.

2. PicoCniModeConverter has no dedicated test
The three sibling converters in the same package — PicoAZConverter, PicoArchConverter, PicoServerTypeConverter — each have a matching test in src/test/.../commands/converters/. PicoCniModeConverter does not. It has real logic worth covering (case-insensitive match, trim, and the TypeConversionException message on an invalid value) — none of which is exercised anywhere, since InitTest sets command.cni directly in Kotlin and never goes through PicoCLI parsing/the converter at all.

3. Worth confirming before flipping the default (not a blocker for this opt-in patch)
The old VXLAN comment attributed avoiding the K3s-agent-bootstrap eBPF-interception deadlock to the tunnel mode itself ("VXLAN avoids this by keeping host-to-host traffic outside eBPF interception"). The new comment reattributes safety entirely to the k8sServiceHost/k8sServicePort direct-endpoint trick, which existed in both versions. It is not obvious from the diff alone that native routing mode does not reintroduce the original deadlock. Task 6.2 ("Live cross-AZ validation... all agents Ready without deadlock") is correctly left unchecked — just flagging that this is the load-bearing open question for the change, worth calling out explicitly as a precondition for #819 (default flip), not just an item on the task list.

Minor/non-blocking: enum class CniMode { Cilium, Flannel } lists Cilium first even though Flannel is the default — purely cosmetic, but reordering could reduce the chance of a future reader assuming enum-declaration-order implies default.

Nice fixes on the IMDS hop-limit generalization (EC2InstanceService.kt) — lifting it out of the Control-only guard is the right root-cause fix per the "configuration problems require configuration fixes" principle, and the new parameterized test (@EnumSource(ServerType::class)) covers all node types cleanly.

[claude]

Review of the Cilium ENI native routing change.

Solid, well-scoped change. The default-stays-Flannel decision, the two bug fixes (Hubble brace-expansion, IMDS hop-limit on all node types), and the CIDR-threading approach (reading from workingState.initConfig?.cidr rather than plumbing a new param through the call stack) are all sound, and the tests exercise real behavior (asserting exact flags present/absent, single-quoting, CIDR threading, hop-limit across all ServerTypes via @EnumSource) rather than mock-echoing.

Findings:

1. openspec/changes/cilium-native-routing/proposal.md contradicts itself and the actual code on the default.
   The Capabilities section (line 24) and design.md/tasks.md correctly state the default stays flannel this patch. But the Impact section directly below it says the opposite twice:

• Line 30: "commands/Init.kt — --cilium boolean replaced by --cni=<cilium|flannel> (default cilium)"
• Line 31: "InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium"
  Both are stale — the shipped code defaults to CniMode.Flannel (Init.kt:273, ClusterState.kt:125), matching the rest of the doc and design.md Decision 1. Since "Specs are the source of truth" here, worth fixing before merge so a future reader does not get whiplash from the same document disagreeing with itself.

2. Minor: user-supplied --cidr is interpolated unescaped into a remote shell command.
   CiliumService.install() builds ipv4NativeRoutingCIDR=$vpcCidr and joins it into a command string executed via remoteOps.executeRemotely -> SSH exec (shell-interpreted). vpcCidr traces back to Up.resolveCidr(initConfig.cidr), and cidr is a free-form user-supplied --cidr string option in Init.kt. A value containing shell metacharacters would be interpreted by the remote shell. This is consistent with existing patterns in this file (k8sServiceHost=${controlHost.private} etc. are built the same way) and the blast radius is limited — the operator already has full remote command execution on their own ephemeral control node via this exact mechanism — so I would call this low severity / not blocking, just flagging since it is new user-controlled input flowing into a shell string.

3. EC2InstanceService: hop-limit-2 / IMDSv2-required is now applied unconditionally to every node, even on Flannel (the default) clusters.
   This is intentional per the design doc (the operator can land on any node), and hop-limit 2 is strictly more permissive than 1 so it should not break anything — just noting the scope is now always, not only when Cilium is selected, in case that is worth calling out explicitly in a comment (the current comment already explains why, so this is a non-issue, just confirming it is deliberate).

Nit: openspec/changes/cilium-native-routing/proposal.md line 36 says "add REQ-NET-007" but the spec delta file itself does not carry that ID — fine per tasks.md 5.2 (numbering deferred to archive), just flagging so it does not get missed at archive time.

Nothing here blocks merging as a draft/opt-in change — finding 1 is the only one I would actually want fixed before this leaves draft, since it is a factual self-contradiction in the spec doc itself.

[claude]

Review: Cilium ENI native routing (opt-in --cni)

Solid change overall — the datapath rewrite is well-reasoned (VXLAN → ENI native routing is the right call for cross-AZ correctness), the two root-cause fixes (Hubble brace-expansion, IMDS hop-limit on all node types) are genuine bug fixes rather than workarounds, and the CniMode enum is a clean replacement for the old boolean. Tests are meaningful — they assert real command content/config decisions rather than mock-echoes (e.g. CiliumServiceTest's single-quote regression test, the new UpTest cases that stub onServerReady to actually exercise the install side effect, the parameterized EC2InstanceServiceTest across all ServerTypes).

I traced the CIDR threading end-to-end (provisionInfrastructure → resolveCidr → persisted to workingState.initConfig → installCilium() → CiliumService.install) and the useCustomCni/onServerReady wiring in Up.kt — both check out, and the new requireNotNull guard in installCilium() is effectively unreachable through a normal up run (correctly documented as such in the new test). Confirmed no other ciliumEnabled/--cilium references were left behind anywhere in src/main or docs/.

Finding: stale/contradictory OpenSpec proposal.md

openspec/changes/cilium-native-routing/proposal.md Impact section (lines 30, 31, 35) says:

• commands/Init.kt — --cilium boolean replaced by --cni=<cilium|flannel> (default cilium)
• InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium
• fromInit default cilium

This directly contradicts:

• The same file's own Why/What Changes sections (lines 5, 16): "the default stays Flannel... Flipping the default to Cilium... is the deliberately-separate follow-up perf(networking): make Cilium ENI native routing the DEFAULT CNI (post-stabilization flip) #819."
• design.md Decision 1: "Default stays flannel this patch (owner decision)."
• tasks.md 1.2/1.3: "default Flannel this patch."
• The actual shipped code: Init.kt:273 (var cni: CniMode = CniMode.Flannel) and ClusterState.kt:375 (val cni: CniMode = CniMode.Flannel).

Looks like leftover text from an earlier research pass that wasn't reconciled after the owner-decision to default to Flannel. Since CLAUDE.md treats specs as the source of truth, this is worth a quick fixup commit before merge — otherwise it'll misinform whoever reads the proposal during the #819 default-flip follow-up.

Everything else

• CiliumService.kt: ENI native --set flags match the design doc exactly; VXLAN flags correctly removed; single-quoting of the Hubble metrics list is a correct fix for the brace-expansion bug (confirmed executeRemotely sends the command string as a single SSH exec payload interpreted by one remote shell — no double-quoting-layer risk).
• EC2InstanceService.kt: lifting metadataOptions out of the Control-only guard is the right root-cause fix (the non-hostNetwork cilium-operator can land on any node type). IMDSv2 (httpTokens(REQUIRED)) stays enforced, which keeps the SSRF blast radius from the wider hop-limit reasonable.
• PicoCniModeConverter: clean, case-insensitive, trims whitespace, well-tested.
• No test-tier violations spotted — nothing here needs TestContainers/Docker, and the unit tests don't touch anything that would require the integration tier.

Nice work — just the proposal.md doc fix needed.

[claude]

Review: Cilium ENI native routing (opt-in --cni=cilium)

Overall this is a well-structured change: the CNI selection is modeled correctly as an enum (extensible, mutually exclusive by construction), the two blocking bugs described in the proposal (Hubble brace-expansion and IMDS hop-limit) are both genuinely fixed in the code, and keeping the default at Flannel is a sensible way to de-risk the rollout. Test coverage is strong - the new UpTest cases stub setupCluster to actually invoke onServerReady, which is the right way to exercise the install side effect rather than just asserting a mock was wired up.

Issue

openspec/changes/cilium-native-routing/proposal.md still claims the wrong default (Impact section):
The Impact bullet for configuration/ClusterState.kt says: InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium

This says the new default is CniMode.Cilium, but the actual code, design.md, and every other part of this PR (including the dedicated commit "docs: fix proposal Impact bullet - --cni default is flannel not cilium") say the default is CniMode.Flannel. It looks like that fix commit corrected the root CLAUDE.md line but missed this second occurrence in proposal.md. Worth a follow-up since specs are meant to be the source of truth here and this one is currently self-contradictory.

Nitpick (non-blocking)

PicoCniModeConverter hand-matches "cilium"/"flannel" string literals rather than deriving from CniMode.entries(). The design doc frames the enum as extensible to future CNIs with no new flags, but as written a third CniMode value would need updates in both the enum and this converter's when block. Not a problem today - just something to keep in sync if a future CNI is added.

What looks solid

• CiliumService: ENI native --set flags match the design (ipam.mode=eni, routingMode=native, ipv4NativeRoutingCIDR=$vpcCidr, etc.), VXLAN/tunnel flags are fully removed, and the Hubble metrics list is now single-quoted (hubble.metrics.enabled='{...}'). Traced this through DefaultRemoteOperationsService/SSHClient - the whole command string is handed to the remote shell as one exec, so quoting there is exactly the right fix for the brace-expansion bug.
• CIDR threading: Up.installCilium()'s requireNotNull(workingState.initConfig?.cidr) looked risky in isolation, but tracing provisionInfrastructure() confirms the CIDR is always resolved and persisted into workingState.initConfig before setupInstancesIfNeeded() -> startK3sOnAllNodes() -> installCilium() ever runs, so the guard can't actually fire during a real up. The UpTest case covering the auto-resolved-CIDR path documents this well.
• EC2InstanceService: lifting the IMDS metadataOptions (hop-limit 2, IMDSv2 required) out of the Control-only guard is correct and, if anything, tightens security for db/app nodes (they previously had no explicit metadata hardening). The @EnumSource(ServerType::class) parameterized test gets full coverage without hardcoding each server type.
• Docs: the CLAUDE.md CNI line update accurately reflects the new default-Flannel/opt-in-Cilium reality.

No security, performance, or correctness concerns beyond the doc inconsistency above. Good test discipline throughout - tests exercise real decision points rather than mock-echo assertions.

[claude]

Review

Solid piece of work — the OpenSpec docs are thorough (including the live-AWS-test corrections baked into the design doc), the CniMode enum is a clean replacement for the boolean flag, and the new tests genuinely exercise behavior (command-content assertions, CIDR threading, hop-limit-per-node-type, server-ready hook wiring) rather than mock-echoing. A few things worth a look before this comes out of draft:

Correctness

ipv4NativeRoutingCIDR can be wrong when reusing an existing VPC without --cidr. Up.provisionInfrastructure() always calls resolveCidr(initConfig.cidr), and when cidr is null this auto-selects a "non-conflicting" block via CidrBlock.selectAvailable(vpcService.listAllVpcCidrs()) — a value that has no relationship to the real CIDR of an existing VPC (createOrValidateVpc just returns existingVpcId when one is set; there's no getVpcCidr(vpcId) lookup anywhere in VpcService). That resolved-but-arbitrary value is what now flows into installCilium() → CiliumService.install(..., vpcCidr) → ipv4NativeRoutingCIDR=$vpcCidr.

Previously this mismatch was harmless (the stored cidr was only ever used when creating a new VPC). This PR is the first place the value becomes load-bearing for correctness: ipv4NativeRoutingCIDR tells Cilium which destination ranges to treat as directly routable (skip masquerade). If it doesn't match the VPC's real range, either legitimate intra-VPC/cross-AZ pod traffic gets masqueraded (defeating the point of native routing) or, worse, an unrelated CIDR gets marked non-masqueraded. Worth at minimum a requireNotNull/validation when --cni=cilium is combined with --vpc <id> and no explicit --cidr, or a real VPC CIDR lookup for the existing-VPC path.

Docs

openspec/changes/cilium-native-routing/proposal.md's "Impact" section says:

InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium

That contradicts design.md Decision 1, tasks.md 1.2, and the actual shipped code (CniMode.Flannel is the default). Minor, but since specs are supposed to be the source of truth here (and #819 will presumably diff against this doc when flipping the default), worth fixing so it doesn't mislead later.

Minor / nice-to-have

• docs/reference/commands.md's init options table doesn't get a --cni row (though --cidr is also missing there already, so this is a pre-existing gap, not a regression).
• I wasn't able to execute ./gradlew test in this review environment to confirm the new/changed tests actually pass — worth confirming CI is green (and the still-unchecked task 6.2 live cross-AZ validation) before merging out of draft.

Nothing here blocks the shape of the design — the CIDR issue is scoped to a specific combination (--cni=cilium + --vpc <existing> + no --cidr) and the rest of the datapath logic (masquerade interface, kube-proxy retention, IMDS hop-limit, Hubble quoting) all looks carefully reasoned and backed by the live-test notes in the design doc.

[claude]

Review

Solid, well-documented change. The openspec proposal/design/tasks docs are thorough, and the live-AWS-test-driven corrections baked into the design (named egressMasqueradeInterfaces, pinning kubeProxyReplacement=false, bpf.hostLegacyRouting=true) show real debugging rigor rather than guessing at Cilium flags. The CniMode enum over a boolean is the right call for extensibility, and fixing the Hubble brace-expansion bug + IMDS hop-limit gap as root causes (rather than working around them) matches the project's fail-fast/no-band-aid philosophy. Test coverage for the new branches (CNI selection, onServerReady wiring, CIDR threading, IMDS on all node types) is good and exercises real decision points rather than mock-echoing.

A few things worth a look before this comes out of draft:

1. Docs inconsistency: proposal.md still says the new default is Cilium

openspec/changes/cilium-native-routing/proposal.md (Impact section) says:

InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium

This contradicts design.md's explicit "Default stays flannel this patch (owner decision)" and the actual code (ClusterState.kt: val cni: CniMode = CniMode.Flannel). There's already a commit in this PR titled "docs: fix proposal Impact bullet — --cni default is flannel not cilium" but this particular bullet still has the stale value — looks like it slipped through.

2. Security consideration: IMDS hop-limit 2 now applies to db/app (worker) nodes too

EC2InstanceService.kt previously gated httpPutResponseHopLimit(2) behind serverType == ServerType.Control; now it applies unconditionally. This is the correct fix for the cilium-operator-on-a-worker-node deadlock, but it's also a real expansion of blast radius: any pod scheduled on a db/app node — including arbitrary kit/workload pods, not just system components — can now reach the node's IMDS at 2 hops and pull the instance's IAM role credentials, where previously that was only possible from pods on the control node. Given AWSPolicy.kt already grants a broad ec2:* to the instance profile (noted in the design doc as "verified no-change"), this may be an acceptable trade-off for a lab tool, but it'd be worth a line in the design doc calling out the widened credential-exposure surface explicitly (the design doc mentions the rejected nodeSelector-pinning alternative but doesn't discuss this angle).

3. Weak assertion in one new UpTest case

fun `up auto-resolves an unset VPC CIDR and installs Cilium with the selected block`() {
    ...
    assertThat(cidrCaptor.firstValue).isNotBlank()
}

Since mockVpcService.listAllVpcCidrs() is stubbed to return an empty list, CidrBlock.selectAvailable deterministically returns "10.0.0.0/16". Asserting the exact value (isEqualTo("10.0.0.0/16")) would make this a stronger regression guard than "non-blank."

4. Minor test gap

CiliumServiceTest asserts the absence of tunnelProtocol=vxlan, routingMode=tunnel, and bpf.masquerade, but design.md explicitly calls out autoDirectNodeRoutes as a "must NOT set" (it only works within one L2 domain and breaks cross-AZ). A doesNotContain("autoDirectNodeRoutes") assertion would guard against a future regression there too.

5. Draft status / live validation still outstanding

tasks.md §6.2 ("Live cross-AZ validation") is unchecked, and the PR itself is marked draft. Given that the whole premise here (ENI IPAM correctness across AZs, the systemd-networkd unmanaged-ENI drop-ins actually preventing the competing-default-route problem) can only really be confirmed on a live multi-AZ cluster, it'd be good to see that validation land before merging — especially since the AMI provisioning change is baked into every future image built from this base.

Nothing here blocks the design — mostly documentation/test polish plus one security trade-off worth stating explicitly in the design doc.

[claude]

Review: Cilium ENI native routing (opt-in --cni)

Went through the full diff (Kotlin, Packer/systemd, and the OpenSpec change docs). Overall this is a well-scoped, well-tested change — opt-in by default, good rationale comments tying config flags back to the specific live-AWS failures that motivated them (brace-expansion bug, cilium/cilium#46010 blackhole), and solid regression coverage.

Findings

1. Doc inconsistency in the OpenSpec proposal (nit)
openspec/changes/cilium-native-routing/proposal.md (Impact section) says:

InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium

This contradicts the actual default everywhere else in the same PR — design.md, tasks.md, and the real code (ClusterState.kt:125, Init.kt:417) all correctly default to CniMode.Flannel. Since specs are meant to be the source of truth here, worth a one-line fix so the proposal doesn't contradict its own implementation.

Things I verified and are fine (no action needed)

• CIDR threading is safe. vpcCidr flows into the cilium install shell command via string interpolation (ipv4NativeRoutingCIDR=$vpcCidr), but it's always sourced from InitConfig.cidr, which is validated through the CidrBlock value class (require(isValidCidr(value))) before it's ever persisted — so there's no injection surface even though it's built as a raw command string.
• requireNotNull(workingState.initConfig?.cidr) in Up.installCilium() is correctly unreachable in the real flow — provisionInfrastructure() resolves and persists the CIDR (line ~307) before startK3sOnAllNodes() runs (line ~649), and the new up auto-resolves an unset VPC CIDR... test documents exactly why.
• No stray references to the removed ciliumEnabled/--cilium boolean remain anywhere in src/ (grepped for both).
• IMDS hop-limit widened to all node types (EC2InstanceService.kt) is a behavior change that reaches beyond Flannel clusters too, not just Cilium ones — but it's called out explicitly in the design doc as "independently correct," and IMDSv2-required + hop-limit 2 is a reasonable security default regardless of CNI. Not flagging as a problem, just noting the blast radius is slightly wider than the PR title suggests.
• configure_cilium_eni_networkd.sh is careful: ens[6-9] ens[1-9][0-9] correctly matches ens6–ens99 without overlapping the ens5 primary match, the 05-/06- prefix ordering is well-justified against 10-netplan-*, and the self-verifying grep -q checks after writing the drop-ins mean a malformed unit file fails the packer build loudly instead of silently.
• Test quality is solid and follows the repo's "no mock-echo" rule — UpTest's new Cilium-path tests stub setupCluster to actually invoke onServerReady (rather than just asserting a mock was called), CiliumServiceTest asserts against the real generated command string, and EC2InstanceServiceTest is parameterized over every ServerType to catch a regression to the old if (serverType == Control) guard.

Status note

Task 6.2 (live cross-AZ validation) is still unchecked in tasks.md, consistent with the PR being marked draft — flagging only so it doesn't get lost before this moves out of draft, not a code issue.

No blocking issues found.

[claude]

Review: Cilium ENI native routing (opt-in --cni=cilium)

Overall this is a well-scoped, well-tested change — the flag ordering in CiliumService.install() is correct (every --set has its paired value), the VXLAN→native flags match what's documented in design.md/proposal.md, and the Hubble brace-expansion fix is a real bug fix (single-quoting prevents the shell from brace-expanding before Helm/Cilium ever sees the value). Nice test coverage: CIDR threading, ENI-flag presence, VXLAN-absence, and the Up wiring for both the Cilium and Flannel branches (including the 'CIDR is always resolved before Cilium installs' case) — no mock-echo patterns, each test exercises a real transformation.

A couple of small things worth a look before merge:

1. Stale default in proposal.md's Impact section
openspec/changes/cilium-native-routing/proposal.md (Impact bullet, ~line 152) still reads:

InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium

This contradicts the rest of the same PR — design.md Decision 1 ('Default stays flannel this patch (owner decision)'), tasks.md 1.2, the actual code/tests, and even the proposal's own ## Why/## What Changes sections all correctly say the default is Flannel. There's already a commit in this PR titled 'docs: fix proposal Impact bullet — --cni default is flannel not cilium' but this particular line wasn't caught by it. Worth fixing so whoever runs openspec-archive-change later doesn't get misled.

2. Duplicated rationale comments in CiliumService.kt
The explanation for egressMasqueradeInterfaces=ens+, kubeProxyReplacement=false, and bpf.hostLegacyRouting=true is written twice: once in the large block comment above the buildList { } call, and again as an inline comment directly above each corresponding add(...) line. Not a bug, but the two copies can drift out of sync on a future edit — consider keeping the rationale in one place (either the block comment or the inline ones, not both).

3. Not a blocker, just flagging per the PR description
Task 6.2 (live cross-AZ validation) is still unchecked and the PR is marked Draft — the author already calls this out as the outstanding risk ('this PR hasn't yet proven out'). Since the default stays flannel this patch, that's a reasonable place to land it as opt-in, but worth holding off on flipping #819 until that validation actually runs.

Nothing else stood out — EC2InstanceService's hop-limit fix and the --cni flag/enum plumbing already landed via #858 as noted, and I confirmed hopLimit(2) is indeed unconditional on main now, so this diff is scoped correctly to just the Cilium install logic.

[claude]

Review: Cilium ENI native routing (opt-in --cni=cilium)

Nice, focused diff — the scope trim after the #858 rebase makes this easy to review in isolation, and the design doc's live-AWS-test corrections (named egressMasqueradeInterfaces, explicit kubeProxyReplacement=false, bpf.hostLegacyRouting=true) read as genuinely earned from a real failure, not speculative hardening.

Code quality / correctness

• CiliumService.install()'s flag set and inline rationale comments (services/CiliumService.kt:34-59) are clear and will save the next person a trip through the Cilium/AWS docs. Good call keeping the "why" (Cilium 1.19.x with kubeProxyReplacement=true kills Talos host networking during endpoint/veth/BPF initialization cilium/cilium#46010, the brace-expansion bug) rather than just the "what."
• Up.installCilium()'s requireNotNull(workingState.initConfig?.cidr) (Up.kt:705-708) is correctly unreachable in the normal up flow — provisionInfrastructure() (Up.kt:304-311) always resolves and persists the CIDR into workingState.initConfig before setupInstancesIfNeeded() triggers the K3s/Cilium path. Good defensive fail-fast rather than a silent NPE-shaped bug later.
• egressMasqueradeInterfaces=ens+ — confirmed + isn't a shell metacharacter, so passing it unquoted through the SSH command string is safe as claimed.
• Minor: openspec/changes/cilium-native-routing/proposal.md's "Impact" section (line ~152) says InitConfig.ciliumEnabled: Boolean is "replaced by InitConfig.cni: CniMode = CniMode.Cilium" — the actual default (and the rest of the design/tasks docs) is CniMode.Flannel, which matches the shipped code (ClusterState.kt:125). Just a stray doc typo, worth a quick fix since specs are supposed to be the source of truth.

Test coverage

• CiliumServiceTest is thorough: asserts the full ENI flag set, absence of the old VXLAN flags, the CIDR threading with a distinct value (not just the fixture default — good, that would have passed even with vpcCidr hardcoded), and the Hubble brace-quoting regression specifically. This is exactly the kind of test that would have caught the original bug.
• UpTest's new setupClusterInvokingServerReadyHook() helper is a nice touch — it documents why the mock needs to invoke the callback (otherwise the Cilium-install branch is silently unexercised), which is the kind of note that prevents someone "simplifying" it later and reintroducing a coverage gap.
• The up auto-resolves an unset VPC CIDR and installs Cilium test is a good example of testing the actual integration point (state resolution → Cilium call) rather than just asserting mock wiring.
• All tests follow the no-mock-echo guidance — each asserts a real transformation (command string contents, captured args) rather than "mock was called."

Design/scope

• Correctly opt-in (--cni=cilium, default flannel) per the design doc's own stated risk posture, given task 6.2 (live cross-AZ validation) is still outstanding. Good discipline not merging the default flip in the same PR as the unvalidated datapath change.
• kubeProxyReplacement=false is now pinned explicitly rather than relying on an implicit default — good, since the design doc says the previous unpinned state is what caused the SSH/apiserver blackhole in live testing.

Nothing blocking

No bugs, security issues, or missing coverage found. The one doc inconsistency above (proposal.md's Impact section default) is cosmetic and easy to fix in a follow-up or amend.

[claude]

Review: Cilium ENI native routing (opt-in --cni=cilium)

Reviewed the diff for services/CiliumService.kt, commands/Up.kt, configuration/ClusterState.kt, commands/Init.kt, the OpenSpec change docs, and the test files. Overall this is a solid, well-scoped change — good separation between the mechanical --cni plumbing (landed in #858) and the actual ENI native-routing logic here.

Strengths

• The two live-AWS-discovered bugs are real fixes, well explained. The egressMasqueradeInterfaces=ens+ panic and the kubeProxyReplacement=false / bpf.hostLegacyRouting=true SSH-blackhole fix (Cilium 1.19.x with kubeProxyReplacement=true kills Talos host networking during endpoint/veth/BPF initialization cilium/cilium#46010) are exactly the kind of thing that's easy to get wrong and hard to debug remotely — the inline comments in CiliumService.kt:34-56 explaining why each flag is set (not just what) are genuinely useful for the next person touching this.
• CIDR threading is correct. Verified the call chain: provisionInfrastructure() (Up.kt:307-310) resolves and persists initConfig.cidr into workingState unconditionally on every up, before startK3sOnAllNodes() (Up.kt:713) wires the onServerReady hook that calls installCilium(). So the requireNotNull(workingState.initConfig?.cidr) guard (Up.kt:706) can't actually fire in the real flow — good, and the new UpTest case for the auto-resolved-CIDR path confirms this ordering explicitly.
• Docs correction is good hygiene: the CLAUDE.md line claiming "K3s uses Cilium (not Flannel)" was actually false (Flannel is the default) — good catch fixing that rather than leaving stale docs.
• Tests are meaningful, not mock-echoes — CiliumServiceTest asserts on the actual --set flag strings (presence of ENI flags, explicit absence of VXLAN flags and bpf.masquerade), and the new UpTest cases exercise the onServerReady hook via a real callback invocation rather than just checking it was passed.

Minor nits

• UpTest.kt — up auto-resolves an unset VPC CIDR and installs Cilium with the selected block only asserts cidrCaptor.firstValue).isNotBlank(). Since CidrBlock.selectAvailable(emptyList()) is deterministic, this could assert the exact expected CIDR value instead — as written it would still pass if CIDR selection returned garbage, as long as it's non-empty.
• CiliumService.kt — vpcCidr (ultimately sourced from the user's --cidr flag or auto-resolution) is interpolated unescaped into the remote command string (ipv4NativeRoutingCIDR=$vpcCidr), same pattern as the existing k8sServiceHost=${controlHost.private}. Not a new anti-pattern and low-risk given this is a single-operator CLI against the operator's own infra, but worth a mental flag since it's a new value flowing through that position.

One process note

I wasn't able to execute ./gradlew test in this review environment to independently confirm the new/changed tests pass (sandboxed, no gradle permission here) — please make sure CI is green before this comes out of draft, especially given the PR description itself flags task 6.2 (live cross-AZ validation) as the main outstanding risk. That's the right thing to gate on before flipping any default.

No blocking issues found in the static review.

[claude]

Review: Cilium ENI native routing (opt-in --cni=cilium)

Reviewed via gh pr diff 820 against the checked-out tree (which already reflects this branch). I could not execute ./gradlew test/detekt in this sandboxed session (Gradle invocations were blocked at the approval layer), so the notes below are from static review only -- please confirm CI is green before merge.

Overview

Replaces the VXLAN-tunnel Cilium install with ENI IPAM native routing, gated behind the existing --cni=<cilium|flannel> enum (default stays flannel, matching the PR's stated scope). Core change is CiliumService.install() gaining a vpcCidr parameter and a corrected --set flag list, plus threading the resolved VPC CIDR through Up.installCilium().

Correctness

• CIDR injection is a non-issue: vpcCidr is interpolated directly into a shell command string (services/CiliumService.kt), but it always originates from CidrBlock, whose init block requires strict ddd.ddd.ddd.ddd/dd shape with numeric-only octets/prefix (network/CidrBlock.kt:22-27,116-136). No shell metacharacters can reach the command. Good -- worth a one-line comment noting this invariant, since a future refactor that lets cidr bypass CidrBlock validation would silently reopen it.
• Up.installCilium()'s requireNotNull(workingState.initConfig?.cidr) is correctly unreachable in the real flow -- resolveCidr() runs and is persisted back to initConfig in provisionInfrastructure() (Up.kt:307-311) well before onServerReady fires during K3s setup. The new "up auto-resolves an unset VPC CIDR..." test actually exercises this path rather than just asserting the guard exists, which is the right test to write here.
• egressMasqueradeInterfaces=ens+, kubeProxyReplacement=false, and the single-quoted Hubble metrics list are all justified with specific failure modes (agent panic, Cilium 1.19.x with kubeProxyReplacement=true kills Talos host networking during endpoint/veth/BPF initialization cilium/cilium#46010 blackhole, brace expansion) rather than asserted without reasoning -- good rationale-in-comment discipline per the fail-fast/root-cause culture here.

Documentation inconsistency

openspec/changes/cilium-native-routing/proposal.md (Impact section) states:

InitConfig.ciliumEnabled: Boolean replaced by InitConfig.cni: CniMode = CniMode.Cilium

This contradicts design.md Decision 1, tasks.md 1.2, and the actual shipped code (ClusterState.kt:125, val cni: CniMode = CniMode.Flannel), all of which correctly default to Flannel. Since specs are the source of truth here, worth fixing before archive so a future reader of proposal.md alone doesn't get the default backwards.

Test coverage

The new tests in CiliumServiceTest, UpTest, InitTest, and PicoCniModeConverterTest assert actual command content / config wiring (e.g., presence of ipam.mode=eni, absence of tunnelProtocol=vxlan, single-quoted Hubble value, CIDR threaded verbatim) rather than mock-echoing setup values -- they'd fail if the underlying logic regressed, which is the right bar per this repo's "no mock-echo tests" rule. The setupClusterInvokingServerReadyHook() helper with its explanatory KDoc is a nice touch -- without it the onServerReady branch would be silently unexercised.

Minor

• CiliumService/DefaultCiliumService still lack class-level KDoc (pre-existing, not introduced by this diff, but since the interface signature is touched here it's a low-cost opportunity to add per the root CLAUDE.md's "always add class-level KDoc" rule).

Risk (already flagged by the author)

Live cross-AZ validation (design doc task 6.2) is explicitly called out as outstanding -- that remains the real open risk here (the egressMasqueradeInterfaces/kubeProxyReplacement flags were already corrected once by a prior live test catching agent panics/blackholes, so a second unvalidated pass on a real multi-AZ cluster before flipping the default in #819 seems warranted).