Skip to content

feat(packer): bake Cilium ENI networkd config into base AMI; apply IMDS hop-limit 2 to all node types - #858

Merged
rustyrazorblade merged 2 commits into
mainfrom
857-cilium-ami-networkd-hop-limit
Jul 24, 2026
Merged

feat(packer): bake Cilium ENI networkd config into base AMI; apply IMDS hop-limit 2 to all node types#858
rustyrazorblade merged 2 commits into
mainfrom
857-cilium-ami-networkd-hop-limit

Conversation

@rustyrazorblade

@rustyrazorblade rustyrazorblade commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Closes #857

This extracts three safe, decoupled pieces from the in-progress #805 Cilium ENI native-routing work (PR #820) so they can land now, independently of that still-under-validation effort. Nothing else from #805/#820 is included here.

1. Bake Cilium ENI networkd config into the base AMI

New packer/base/install/configure_cilium_eni_networkd.sh, wired into packer/base/base.pkr.hcl (packer provisioner) and packer/docker-compose.yml (local script testing), plus a new packer/README.md section.

In Cilium ENI IPAM native-routing mode, the cilium-operator attaches a second ENI (ens6) to a node at runtime once it runs enough pods. Without this change, systemd-networkd (fed by cloud-init/netplan) DHCPs that secondary ENI and installs a competing default route, multi-homing the host and breaking IMDS/egress/kubelet. The script bakes two systemd-networkd drop-ins into the image: 05-cilium-eni-primary.network keeps ens5 OS-managed via DHCP, and 06-cilium-eni-unmanaged.network marks ens6+ Unmanaged=yes so Cilium owns them. It's a no-op on Flannel clusters, which never attach a secondary ENI, so it's safe to bake in unconditionally.

2. IMDS hop-limit 2 for all node types

EC2InstanceService previously only set httpPutResponseHopLimit(2) for Control nodes. The non-hostNetwork cilium-operator pod can schedule on a db or app node and needs two hops to reach IMDS for ENI-allocation credentials, so this now applies to every ServerType. Independently correct for IMDSv2 regardless of the CNI-selection feature landing later.

Covered by EC2InstanceServiceTest (parameterized over every ServerType) and ./gradlew testPackerBase.

3. --cni=<cilium|flannel> flag (replaces the old --cilium boolean)

Pure CLI flag surface, no behavior change beyond how the flag is spelled and stored:

  • New CniMode enum (Cilium / Flannel) in configuration/ClusterState.kt, replacing InitConfig.ciliumEnabled: Boolean with InitConfig.cni: CniMode (default Flannel).
  • New PicoCniModeConverter for case-insensitive --cni parsing.
  • Init.kt: --cni replaces --cilium.
  • Up.kt: reads initConfig?.cni == CniMode.Cilium instead of the old boolean.

Cilium continues to install exactly as it does on main today — the existing single-argument, VXLAN-tunnel-mode CiliumService.install(controlHost) call is unchanged. Only the flag syntax and the persisted state field changed; the ENI native-routing install logic itself stays exclusively on #805/PR #820, still on hold.

Covered by PicoCniModeConverterTest, and adapted InitTest/UpTest cases asserting the cni field persists correctly and that up still installs Cilium via the existing onServerReady callback when cni == Cilium.

…DS hop-limit 2 to all node types

Splits two safe, decoupled pieces out of the in-progress #805 Cilium ENI
native-routing work so they can land independently while the rest of that
PR is still being validated:

- New packer/base/install/configure_cilium_eni_networkd.sh bakes
  systemd-networkd drop-ins into the base AMI so the OS leaves Cilium's
  runtime-attached secondary ENIs (ens6+) unmanaged, preventing a
  competing DHCP default route from multi-homing the host. Inert on
  Flannel.
- EC2InstanceService now sets IMDSv2 hop-limit 2 for every node type, not
  just Control nodes, so the non-hostNetwork cilium-operator pod can reach
  IMDS regardless of which node it schedules on.

Closes #857
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review

Clean, well-scoped extraction from #820 — both pieces are independently justified and each is covered by a test (EC2InstanceServiceTest parameterized over ServerType, testPackerBase). A few notes:

Worth a second look

Hardcoded ens5/ens6+ interface naming may not hold for every instance family the tool allows. configure_cilium_eni_networkd.sh assumes the primary ENI is always ens5 and any ens6+ is a Cilium-attached secondary. That convention comes from AWS Nitro's PCI slot reservation and does generally hold across current Nitro families, but it's baked into the AMI once and then applied to whatever instance type the user picks at cluster-create time — which isn't the AMI-build instance type (c6i.2xlarge/c8g.2xlarge in base.pkr.hcl). Defaults are m5d.xlarge (control), i4i.xlarge (db), c6id.2xlarge (app) — all Nitro, so likely fine — but since users can pass an arbitrary --instance-type, it'd be worth either:

  • a one-time validation against the actual default instance families before merge (boot each, check ip link / networkctl naming), or
  • a defensive note in the script header that this assumes Nitro's conventional PCI slot layout and could misfire on an instance type where it doesn't hold.

The failure mode if the assumption is wrong is not graceful: if a node's real primary NIC ever matched the ens[6-9]|ens[1-9][0-9] pattern instead of ens5, 06-cilium-eni-unmanaged.network would mark it Unmanaged=yes and the node would lose DHCP/SSH/IMDS entirely — and this wouldn't be caught by testPackerBase, since that only checks the drop-in files are written with the right content in a Docker container, not actual interface naming on real EC2 hardware. Not a blocker given the Nitro convention is well established, but flagging since the blast radius (total network loss on the node) is high if it's ever wrong for a less-common instance family.

Looks solid

  • EC2InstanceService.kt: lifting metadataOptions out of the Control-only guard is a straightforward, correct change — ServerType only has three variants (Cassandra/Stress/Control), all of which run K3s pods, so applying hop-limit 2 uniformly is low risk and matches the stated rationale (non-hostNetwork cilium-operator can land on any node type).
  • The parameterized test (@EnumSource(ServerType::class)) is a real improvement over the old single-case test — it actually exercises the change (previously-conditional → unconditional) rather than restating a mock.
  • Provisioner ordering in base.pkr.hcl and docker-compose.yml is consistent between the two, so testPackerBase genuinely exercises the same sequence as the real build.
  • The script is self-verifying (grep -q checks after writing each drop-in) and set -euo pipefail + sudo usage matches the conventions of the other packer/base/install/*.sh scripts.
  • Marking it "inert on Flannel" is accurate — the 06- match only fires on ens6+, which only appears once Cilium ENI-IPAM attaches a secondary ENI at runtime.

No security or test-coverage concerns beyond the above.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Adds the CniMode enum-based flag surface ahead of the Cilium ENI
native-routing work, which stays on #805/PR #820. This is plumbing only:
Cilium still installs in the existing single-argument VXLAN-tunnel mode
via CiliumService.install(controlHost) — unchanged. Only how the
install-or-not decision gets computed changes, from a boolean field to
InitConfig.cni == CniMode.Cilium.

- CniMode enum in configuration/ClusterState.kt, replacing
  InitConfig.ciliumEnabled with InitConfig.cni (default Flannel)
- PicoCniModeConverter for case-insensitive --cni parsing
- Init.kt: --cni option replaces --cilium
- Up.kt: reads workingState.initConfig?.cni == CniMode.Cilium instead of
  the old boolean
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review: Clean, well-scoped PR -- does exactly what the description promises (three decoupled, safe extractions from #805/#820), and each piece is independently justifiable. No backwards-compatibility concerns apply here per project convention (ephemeral clusters), and I did not flag the AMI/config changes on those grounds. || Strengths: (1) IMDS hop-limit fix is a real bug fix, not just plumbing -- the old code only set httpPutResponseHopLimit(2) for ServerType.Control, which would have deadlocked ENI IPAM for the non-hostNetwork cilium-operator pod scheduled on a db/app node. The new parameterized test (EC2InstanceServiceTest, EnumSource(ServerType)) actually asserts hop-limit 2 for every node type -- good, meaningful coverage of the exact regression this fixes. (2) CniMode enum over boolean is the right call -- self-documenting, extensible for future datapaths, and the migration (ciliumEnabled: Boolean -> cni: CniMode) is clean. No stray references to the old --cilium flag or ciliumEnabled field remain anywhere in code or docs. (3) PicoCniModeConverter follows the existing converter pattern in the codebase (c.f. PicoAZConverter) -- small, single-purpose, well-documented KDoc, case-insensitive/whitespace-tolerant with a clear error message. (4) UpTest additions are genuinely exercising behavior, not mock-echo: the Cilium-path test actually invokes the captured onServerReady callback and verifies CiliumService.install is called with the correct host, rather than just asserting a mock was configured. Same for the Flannel-default test asserting CiliumService.install is never called. (5) configure_cilium_eni_networkd.sh is unusually well-documented for a provisioning script -- the WHY/PRECEDENCE/ROBUSTNESS/TIMING/INERT-ON-FLANNEL comment structure clearly explains the lexical-ordering trick (05-/06- prefixes beating cloud-init's 10-netplan-*) and why cloud-init's network management is deliberately left enabled rather than disabled. The self-verifying grep checks at the end are a nice touch given the packer test harness only checks exit code. sudo usage and set -euo pipefail are consistent with sibling scripts in packer/base/install/. (6) Wiring into base.pkr.hcl and docker-compose.yml is correctly ordered and matches the existing provisioner pattern. || Minor / non-blocking notes: (1) Name=ens[6-9] ens[1-9][0-9] in the secondary-ENI match only covers ens6-ens99 -- not a real concern (EC2 instance types don't come close to 100 ENIs), just noting the pattern has an implicit ceiling. (2) The self-verify grep assertions in the shell script re-derive the exact heredoc content by hand -- if the heredoc body changes later these need to be kept in sync manually. Low risk given the file is small and self-contained. (3) I was not able to execute ./gradlew test / ktlintCheck in this review environment (sandboxed -- Gradle/Java invocations were blocked by a permission gate for both direct execution and a subagent), so I read the new/changed tests manually rather than confirming they pass. They look logically sound and consistent with the project's no-mock-echo testing guidance, but please make sure CI is green before merge. || No correctness, security, or performance issues found. LGTM.

@rustyrazorblade
rustyrazorblade merged commit 9b69a92 into main Jul 24, 2026
12 checks passed
@rustyrazorblade
rustyrazorblade deleted the 857-cilium-ami-networkd-hop-limit branch July 24, 2026 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(packer): bake Cilium ENI-mode networkd config into base AMI + apply IMDS hop-limit 2 to all node types

1 participant