Repository navigation
Preserve VM network identity on Calico destinations - #6
Merged
Merged
Conversation
… expected behaviour Emit Calico annotations for VM template in vSphere builder use ipAddrs annotation rather than ipAddrsNoIpam. + update some docstrings validation+testing for calico in VSphere, + stubs in other providers. * When implicit-VLAN is used, infer the correct VLAN from the l2Bridge spec, rather than reporting VLAN=0. * Richer error reporting when JSON formatting fails on fetching NAD. adds new CalicoIssue when there IS an l2Bridge spec, but no VLAN inside Factors-out NetworkConfig fetch&parse where duplicated code existed Note: pkg/controller/plan/validation.go seemed to have a bug where, if the last NAD in the list failed to parse, the resultant error would not follow the same (log&continue) codepath as the prior NADs. Instead, the err would bubble out of the for-loop and get retured by `validateUserDefinedNetwork`. This commit makes the error handling consistent for all iterations. separate pure-Calico misconfigurations from VM config mismatches *Caches fetched NADs in mapNetworks to avoid repeated fetches for the same NAD. *Adds new Calico Issue "NAD unreadable" *Increased UT test-case coverage Grant forklift-controller RBAC for Calico Network/IPPool reads Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
A NAD whose Spec.Config is just {"type": "calico"} (no "network" field) is
a valid Calico-CNI configuration — it requests legacy L3 IPAM mode. But
Forklift's identity-preservation work only fires for L2-attached NADs
(gated on ReferencesCalicoNetwork() == "type==calico" AND "network != \"\""),
so a user opting into this without an L2 reference silently loses MAC/IP
preservation at migration time.
This commit surfaces that gap as a Warn-class Plan condition instead of
silently skipping the NAD.
Changes:
* New CalicoIssueKind: NADMissingNetwork.
* New CalicoValidationResult.Warnings slice, distinct from .Issues so the
dispatcher can render Critical and Warn classes independently.
* vSphere ValidateCalicoNADs: emit the warning when type==calico and
network is empty, between NAD parse and the existing non-Calico skip.
* New Plan condition type CalicoNetworkWarning (Category: Warn) alongside
CalicoNetworkInvalid (Category: Critical). Dispatcher logic deduplicated
via a shared buildCalicoNADCondition helper so both classes go through
the same itemize/format path.
* Unit tests: warn-in-isolation case + Critical+Warn coexistence case to
lock in the dispatcher's independence between the two classes.
Stale-condition cleanup is handled by the existing BeginStaging /
EndStaging pattern in controller.go — when the user fixes the NAD the
warning is naturally pruned on the next reconcile.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
* pkg/controller/plan/adapter/base/nad.go: wrap the NAD GET error with
namespace/name context using fmt.Errorf("%w"). Downstream k8serr.IsNotFound
and meta.IsNoMatchError both unwrap through %w, so the validator's
CalicoNetworkNotFound / NoMatchError disambiguation continues to work.
* pkg/lib/client/calico/network_test.go: split the combined
"if got.L2Bridge == nil || len(got.L2Bridge.VLANs) != 3" check into two
distinct t.Fatal calls. The original code would have panicked on a nil
L2Bridge because the t.Fatalf args still dereferenced through it, masking
the real regression with a panic stack trace.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
Migrating a VM onto a Calico cluster gave its pod a fresh MAC and a
fresh IP, breaking workloads keyed to the source identity (MAC-based
licences, IP allowlists, static in-guest addressing). This adds opt-in
identity preservation, driven from the NetworkMap.
A `type: pod` destination gains an optional `calico` block. Its presence
— even empty (`calico: {}`) — opts the VM's primary NIC into
preservation: the source MAC is carried over, and the source IP too when
the Plan sets preserveStaticIPs.
Naming a Calico Network under the block requests an L2 attach.
`calico.network` selects the Network and `calico.vlan` selects the VLAN
within it; the two are required together, and a Network named without a
VLAN is rejected rather than auto-selected.
Secondary NICs keep their `type: multus` shape — a Multus entry pointing
at a Calico-backed NAD now also carries the source NIC's identity.
Before a migration starts, Forklift checks what the destination's Calico
can actually do and blocks the Plan up front when a request can't be
honoured: Calico absent, the Network resource absent, a requested VLAN
missing, a NIC carrying more than one IPv4, or
a source other than vSphere — the only source supported in this delivery.
Review fixes folded in:
- IPPool eligibility is allowedUses-aware. The Calico client parses
disabled and allowedUses (nil-vs-empty is load-bearing: an absent
field means Calico's default ["Workload","Tunnel"]), replacing the
CIDR-only pool helpers: L2 attach requires an L2Workload-allowed
pool inside the VLAN subnet, plain L3 preservation requires a
Workload-allowed pool, and disabled pools never match.
- The plan-level "preserving static IPs on pod networking" warning no
longer fires for a calico-flagged pod entry - preserving the IP on
the pod network is exactly what that entry does.
- NAD-path hardening: the NAD walk caches parsed configs so a
fetch/parse failure is reported once as NADUnreadable instead of
being retried per entry, and the per-VM primary matcher only
considers pod-type entries.
- A source NIC without a MAC no longer produces an empty hwAddr
annotation that would fail the pod at CNI ADD.
- A NetworkMap reference to a Calico Network that is not an l2Bridge
network (e.g. a VRF network) is reported accurately as
NetworkTypeUnsupported instead of a misleading VLAN error: the
Network is fetched and classified before any VLAN handling, so a
missing Network now also reports NetworkNotFound ahead of
VLANRequired.
- L2 attach requires Calico's BPF dataplane: a plan attaching to an
l2Bridge network on a cluster whose FelixConfiguration does not set
bpfEnabled: true is blocked with a Critical condition up front
rather than failing at pod creation. RBAC gains read access to
felixconfigurations.
- Doc and CRD text synced to explicit-VLAN semantics: vlan 0 is
documented as "not set" and rejected whenever a Network is named;
the regenerated CRD and operator bundles carry the same text.
- RBAC comment reworded; test coverage aligned across the vSphere
validator and builder suites.
# Conflicts:
# pkg/controller/plan/adapter/vsphere/validator_test.go
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
A NetworkMap multus entry may now point at a NAD referencing a Calico VRF network. The scoped MAC/IP identity annotations apply unchanged; preserved static IPs are validated against enabled Workload IPPools (VRFs have no VLANs or subnets). VRF networks remain unsupported on the pod-primary entry: a VRF-only primary would sever cluster DNS, the API server, and kubelet probes. Plans referencing a VRF network are checked for viability up front, since the platform validates little of it. Criticals: a non-nftables dataplane, kernel-reserved routing tables, and provable route-table collisions (with another VRF network on overlapping nodes, or with the FelixConfiguration's explicit routeTableRanges). Warnings: hostConfig coverage limited to selected nodes, unprovable route-table collisions, a VLAN named against a VRF network, missing ipv4_pools pinning on plans that assign fresh IPs, hostConfig entries without host interfaces, and no BGPPeer bound to the network (required for cross-node reachability in Local mode). RBAC gains read access to bgppeers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
PrimaryNetworkTypeUnsupported no longer claims only l2Bridge networks are supported - VRF networks attach via multus NADs instead. The "Only Workload covers (not L2)" test case now uses an IP that no L2 pool covers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
An empty hostConfig nodeSelector is the canonical all-nodes form, so all-scoped entries mean a node subset. Without targetNodeSelector or targetAffinity on the plan, VMs may schedule onto uncovered nodes and fail at CNI ADD - nondeterministic, so Critical. With placement set, the new VRFPlacementUnverified warning notes the pin cannot be verified against the network's Calico selectors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
Calico NAD and primary-entry validation now runs in the NetworkMap reconciler against the destination cluster and surfaces conditions on the NetworkMap; Critical ones block the map's Ready condition, which already blocks plans. The plan controller calls the same shared functions to rebuild the per-VM cache and keeps only the plan-scoped checks: VRF placement, pool pinning, the preserveStaticIPs warning, the UDN-namespace conflict, and the per-VM issues. ValidateCalicoNADs and ValidateCalicoPrimary are removed from the adapter Validator interface along with every provider stub; the non-vSphere rejection of the calico block moves to the NetworkMap controller. Signed-off-by: Alex O'Regan <alex.oregan@tigera.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings Calico network identity preservation onto
main, so the next release branch can be cut frommainrather than carried forward fromrel/v3.24.0-2.What it does
Migrating a vSphere VM onto a Calico cluster used to give its pod a fresh MAC and IP, breaking workloads keyed to the source identity (MAC-based licences, IP allowlists, static in-guest addressing). With this, a NetworkMap can opt a VM into keeping them:
type: poddestination gains an optionalcalicoblock. Its presence, even empty, carries the source MAC over to the VM's primary NIC, and the source IP too when the Plan setspreserveStaticIPs.calico.networkandcalico.vlantogether request an L2 attach to a VLAN of a Calico Network.type: multusshape; a Multus entry pointing at a Calico-backed NAD also carries the source NIC's identity. VRF networks are accepted as Multus destinations, with checks that they can actually be reached.How it got here
These are the commits shipped on
rel/v3.24.0-2, re-applied onto currentmain:mainwere additive and resolved keeping both sides:main'sExcludedDisksvalidation alongside the Calico checks in the vSphere validator and its tests, andmain'snetworkIPModechecks alongside the Calico checks in NetworkMap validation.maingained since the release branch was cut, gets the same Calico treatment as Hyper-V, so it compiles and Nutanix sources are rejected for Calico mappings like every non-vSphere source.Independent of #4 (the CVE remediation): the two touch different files and merge together without conflicts, in either order.
Open question
mainadded a per-entrynetworkIPModeto NetworkMaps, while Calico's IP preservation reads only the Plan'spreserveStaticIPs. By default they agree. But an explicitnetworkIPMode: dhcpornoneon a Calico entry isn't honoured by the Calico side, andpreserveon a Calico pod entry raises a NetworkMap warning that the Calico work deliberately suppresses at Plan level. How the two should combine needs deciding; this PR doesn't change either.Verification
go build, the fullmake testsuite (69 packages, none failing), andmake manifests generate-manifestswith no drift, on this branch alone.Successful. An end-to-end Calico migration there is still to run.🤖 Generated with Claude Code