Draft: Calico Secondary-NIC L2 & L3 Annotation Support#6936
Draft
aaaaaaaalex wants to merge 3 commits into
Draft
Draft: Calico Secondary-NIC L2 & L3 Annotation Support#6936aaaaaaaalex wants to merge 3 commits into
aaaaaaaalex wants to merge 3 commits into
Conversation
aaaaaaaalex
requested review from
Hazanel,
mnecas,
mrnold,
solenoci and
yaacov
as code owners
aaaaaaaalex
changed the title
aaaaaaaalex
force-pushed
the
rebase-exp-against-main
branch
from
15911b7 to
66b57ac
Compare
📝 WalkthroughWalkthroughThis PR adds Calico network validation for migration plans and preserves Calico MAC and static IP annotations on migrated VMs. It introduces plan-level NAD/network/IPPool validation to detect configuration issues, per-VM checks to ensure static IPs match Calico subnets and IPPools, NAD config parsing for Calico detection, and vSphere builder integration to collect and annotate VMs with Calico network metadata. ChangesCalico Network Validation and Preservation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/plan/adapter/vsphere/builder_test.go (1)
1135-1145: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMigrate this test client bootstrap to the shared k8s testutil helpers.
createBuildercurrently hand-rolls scheme/client setup; please wire it throughpkg/provider/testutil/k8s.goto keep test client construction consistent across the repo.As per coding guidelines,
**/*_test.go: Unit tests should use shared fake client helpers frompkg/provider/testutil/k8s.goincludingNewScheme()andNewFakeClient(objs...)before hand-rolling custom fakes.🤖 Prompt for 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. In `@pkg/controller/plan/adapter/vsphere/builder_test.go` around lines 1135 - 1145, The test currently hand-rolls a scheme and fake client in createBuilder; replace that with the shared k8s testutil helpers by calling testutil.NewScheme() to get a scheme, register the same APIs (v1, core, rbacv1, k8snet, v1beta1) onto that scheme, then create the fake client via testutil.NewFakeClient(objs...) (or NewFakeClient(scheme, objs...) if your helper takes a scheme) and assign it to the client used to construct the Builder so createBuilder uses testutil.NewScheme() and testutil.NewFakeClient instead of manual runtime.NewScheme()/fake.NewClientBuilder().
🧹 Nitpick comments (2)
pkg/controller/plan/adapter/vsphere/validator_test.go (1)
693-700: ⚡ Quick winUse the shared Kubernetes fake-client test helpers instead of hand-rolled setup.
Please switch this local scheme/client construction to
pkg/provider/testutil/k8s.gohelpers (NewScheme()andNewFakeClient(objs...)) so these tests follow the repo’s standard test harness.As per coding guidelines, unit tests should use shared fake client helpers from
pkg/provider/testutil/k8s.goincludingNewScheme()andNewFakeClient(objs...)before hand-rolling custom fakes.🤖 Prompt for 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. In `@pkg/controller/plan/adapter/vsphere/validator_test.go` around lines 693 - 700, Replace the hand-rolled scheme/client setup that calls runtime.NewScheme(), k8snet.AddToScheme, scheme.AddKnownTypeWithName(... calicoclient.NetworkGVK/IPPoolGVK ...) and fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(k8sObjs...).Build() with the repository test helpers: call NewScheme() instead of runtime.NewScheme and use NewFakeClient(k8sObjs...) to obtain the fake client (replace variables scheme and c accordingly); remove the manual calico/k8snet type registrations and adjust imports to use pkg/provider/testutil/k8s.go's NewScheme and NewFakeClient so tests use the shared fake-client harness.pkg/lib/client/calico/ippool_test.go (1)
42-66: ⚡ Quick winAdd a negative-path test for malformed or missing
spec.cidrinListIPPools.Current coverage is mostly happy-path; adding explicit malformed
spec.cidrcases would harden confidence in parse/error behavior used by Calico validation.As per coding guidelines,
**/*_test.go: Coverage: Strive for high test coverage on critical logic paths.🤖 Prompt for 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. In `@pkg/lib/client/calico/ippool_test.go` around lines 42 - 66, Add a negative-path unit test for ListIPPools to exercise malformed or missing spec.cidr: create a new test (e.g., TestListIPPools_MalformedCIDR) that uses newFakeClientWithIPPools and makeIPPool variants where spec.cidr is empty or contains invalid CIDR strings, call ListIPPools(context.Background(), c), assert that the function returns the expected error or skips/filters the bad entries (matching the package behavior), and validate that valid pools are still returned correctly; reference the existing TestListIPPools_Multiple, ListIPPools, newFakeClientWithIPPools, and makeIPPool to model setup and assertions.
🤖 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 `@pkg/controller/plan/adapter/base/nad_test.go`:
- Around line 15-25: Replace the custom test helper newFakeClientWithNADs with
the shared test utilities: call the shared NewScheme() to build the scheme and
NewFakeClient(objs...) to create the fake client instead of using
runtime.NewScheme(), k8snet.AddToScheme, and fake.NewClientBuilder; update tests
that call newFakeClientWithNADs to pass the NetworkAttachmentDefinition objects
into NewFakeClient and remove the custom builder function so the test uses
pkg/provider/testutil/k8s.go's NewScheme and NewFakeClient helpers.
In `@pkg/controller/plan/adapter/base/nad.go`:
- Around line 18-20: The raw error from the client Get call in the NAD retrieval
must be wrapped with the NAD namespace/name for context: replace the plain
"return nil, err" after "if err := c.Get(ctx, key, nad); err != nil {" with a
wrapped error that includes key.Namespace and key.Name (e.g., using
fmt.Errorf("failed to get NetworkAttachmentDefinition %s/%s: %w", key.Namespace,
key.Name, err) or errors.Wrapf) so callers receive namespace/name context;
ensure the fmt or errors package is imported and maintain the same return
signature (return nil, wrappedErr).
In `@pkg/lib/client/calico/ippool_test.go`:
- Around line 20-29: Replace the hand-rolled fake client in
newFakeClientWithIPPools by using the shared test util helpers: call
pkg/provider/testutil/k8s.NewScheme() to obtain the scheme, register the IPPool
GVK types on that scheme (using IPPoolGVK and
IPPoolGVK.GroupVersion().WithKind("IPPoolList") as needed), then create the fake
client with pkg/provider/testutil/k8s.NewFakeClient(objs...) instead of manual
fake.NewClientBuilder code; update newFakeClientWithIPPools to return the shared
NewFakeClient result (or a builder-compatible object) and remove the manual
runtime.NewScheme()/fake.NewClientBuilder() logic.
In `@pkg/lib/client/calico/network_test.go`:
- Around line 142-144: The test currently checks "if got.L2Bridge == nil ||
len(got.L2Bridge.VLANs) != 3" which still evaluates len(got.L2Bridge.VLANs) when
L2Bridge is nil; change the assertion in the test (where "got" and
"got.L2Bridge.VLANs" are referenced) to first explicitly fail if got.L2Bridge ==
nil (e.g. t.Fatalf("got.L2Bridge is nil")) and only afterwards check
len(got.L2Bridge.VLANs) != 3 and fail with a message including the actual
length; this prevents a nil dereference while preserving the existing failure
messages.
- Around line 25-37: Replace the local test helper newFakeClientWith with the
shared test utilities: call testutil.NewScheme() to obtain the scheme and use
testutil.NewFakeClient(objs...) to create the fake client instead of building a
fake.ClientBuilder manually; ensure NetworkGVK and its list-kind are registered
on the returned scheme if NewScheme() does not already include them, and update
tests to use the returned client from NewFakeClient rather than the builder.
---
Outside diff comments:
In `@pkg/controller/plan/adapter/vsphere/builder_test.go`:
- Around line 1135-1145: The test currently hand-rolls a scheme and fake client
in createBuilder; replace that with the shared k8s testutil helpers by calling
testutil.NewScheme() to get a scheme, register the same APIs (v1, core, rbacv1,
k8snet, v1beta1) onto that scheme, then create the fake client via
testutil.NewFakeClient(objs...) (or NewFakeClient(scheme, objs...) if your
helper takes a scheme) and assign it to the client used to construct the Builder
so createBuilder uses testutil.NewScheme() and testutil.NewFakeClient instead of
manual runtime.NewScheme()/fake.NewClientBuilder().
---
Nitpick comments:
In `@pkg/controller/plan/adapter/vsphere/validator_test.go`:
- Around line 693-700: Replace the hand-rolled scheme/client setup that calls
runtime.NewScheme(), k8snet.AddToScheme, scheme.AddKnownTypeWithName(...
calicoclient.NetworkGVK/IPPoolGVK ...) and
fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(k8sObjs...).Build()
with the repository test helpers: call NewScheme() instead of runtime.NewScheme
and use NewFakeClient(k8sObjs...) to obtain the fake client (replace variables
scheme and c accordingly); remove the manual calico/k8snet type registrations
and adjust imports to use pkg/provider/testutil/k8s.go's NewScheme and
NewFakeClient so tests use the shared fake-client harness.
In `@pkg/lib/client/calico/ippool_test.go`:
- Around line 42-66: Add a negative-path unit test for ListIPPools to exercise
malformed or missing spec.cidr: create a new test (e.g.,
TestListIPPools_MalformedCIDR) that uses newFakeClientWithIPPools and makeIPPool
variants where spec.cidr is empty or contains invalid CIDR strings, call
ListIPPools(context.Background(), c), assert that the function returns the
expected error or skips/filters the bad entries (matching the package behavior),
and validate that valid pools are still returned correctly; reference the
existing TestListIPPools_Multiple, ListIPPools, newFakeClientWithIPPools, and
makeIPPool to model setup and assertions.
🪄 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: aad89d37-fe44-4448-b712-986f8bb239ab
⛔ Files ignored due to path filters (1)
CLAUDE.mdis excluded by!**/*.md
📒 Files selected for processing (27)
operator/.downstream_manifestsoperator/.upstream_manifestsoperator/config/rbac/forklift-controller_role.yamlpkg/controller/plan/adapter/base/calico.gopkg/controller/plan/adapter/base/calico_test.gopkg/controller/plan/adapter/base/calico_validation.gopkg/controller/plan/adapter/base/doc.gopkg/controller/plan/adapter/base/nad.gopkg/controller/plan/adapter/base/nad_test.gopkg/controller/plan/adapter/hyperv/validator.gopkg/controller/plan/adapter/ocp/validator.gopkg/controller/plan/adapter/openstack/validator.gopkg/controller/plan/adapter/ovfbase/validator.gopkg/controller/plan/adapter/ovirt/validator.gopkg/controller/plan/adapter/vsphere/builder.gopkg/controller/plan/adapter/vsphere/builder_test.gopkg/controller/plan/adapter/vsphere/validator.gopkg/controller/plan/adapter/vsphere/validator_test.gopkg/controller/plan/validation.gopkg/controller/provider/model/ocp/model.gopkg/controller/provider/model/ocp/model_test.gopkg/controller/provider/web/ocp/base.gopkg/lib/client/calico/ippool.gopkg/lib/client/calico/ippool_test.gopkg/lib/client/calico/network.gopkg/lib/client/calico/network_test.gopkg/provider/ec2/controller/validator/noop.go
👮 Files not reviewed due to content moderation or server errors (1)
- operator/.upstream_manifests
Comment on lines
+15
to
+25
| func newFakeClientWithNADs(nads ...*k8snet.NetworkAttachmentDefinition) (*fake.ClientBuilder, error) { | ||
| scheme := runtime.NewScheme() | ||
| if err := k8snet.AddToScheme(scheme); err != nil { | ||
| return nil, err | ||
| } | ||
| b := fake.NewClientBuilder().WithScheme(scheme) | ||
| for _, nad := range nads { | ||
| b = b.WithRuntimeObjects(nad) | ||
| } | ||
| return b, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use shared k8s fake-client test helpers instead of a custom builder helper.
newFakeClientWithNADs hand-rolls scheme/client setup that should come from the shared test utility to keep test scaffolding consistent across adapters.
As per coding guidelines **/*_test.go: "Unit tests should use shared fake client helpers from pkg/provider/testutil/k8s.go including NewScheme() and NewFakeClient(objs...) before hand-rolling custom fakes."
🤖 Prompt for 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.
In `@pkg/controller/plan/adapter/base/nad_test.go` around lines 15 - 25, Replace
the custom test helper newFakeClientWithNADs with the shared test utilities:
call the shared NewScheme() to build the scheme and NewFakeClient(objs...) to
create the fake client instead of using runtime.NewScheme(), k8snet.AddToScheme,
and fake.NewClientBuilder; update tests that call newFakeClientWithNADs to pass
the NetworkAttachmentDefinition objects into NewFakeClient and remove the custom
builder function so the test uses pkg/provider/testutil/k8s.go's NewScheme and
NewFakeClient helpers.
Comment on lines
+18
to
+20
| if err := c.Get(ctx, key, nad); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Wrap NAD GET errors with namespace/name context.
Returning the raw client error here hides which NAD failed and makes reconciliation failures harder to triage.
Proposed fix
import (
"context"
+ "fmt"
@@
if err := c.Get(ctx, key, nad); err != nil {
- return nil, err
+ return nil, fmt.Errorf("get NetworkAttachmentDefinition %s/%s: %w", namespace, name, err)
}As per coding guidelines **/*.go: "Errors must be checked, wrapped with context, and propagated."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := c.Get(ctx, key, nad); err != nil { | |
| return nil, err | |
| } | |
| if err := c.Get(ctx, key, nad); err != nil { | |
| return nil, fmt.Errorf("get NetworkAttachmentDefinition %s/%s: %w", namespace, name, err) | |
| } |
🤖 Prompt for 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.
In `@pkg/controller/plan/adapter/base/nad.go` around lines 18 - 20, The raw error
from the client Get call in the NAD retrieval must be wrapped with the NAD
namespace/name for context: replace the plain "return nil, err" after "if err :=
c.Get(ctx, key, nad); err != nil {" with a wrapped error that includes
key.Namespace and key.Name (e.g., using fmt.Errorf("failed to get
NetworkAttachmentDefinition %s/%s: %w", key.Namespace, key.Name, err) or
errors.Wrapf) so callers receive namespace/name context; ensure the fmt or
errors package is imported and maintain the same return signature (return nil,
wrappedErr).
Comment on lines
+20
to
+29
| func newFakeClientWithIPPools(objs ...runtime.Object) *fake.ClientBuilder { | ||
| scheme := runtime.NewScheme() | ||
| scheme.AddKnownTypeWithName(IPPoolGVK, &unstructured.Unstructured{}) | ||
| scheme.AddKnownTypeWithName(IPPoolGVK.GroupVersion().WithKind("IPPoolList"), &unstructured.UnstructuredList{}) | ||
| b := fake.NewClientBuilder().WithScheme(scheme) | ||
| for _, o := range objs { | ||
| b = b.WithRuntimeObjects(o) | ||
| } | ||
| return b | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace the local fake-client constructor with shared testutil helpers.
This test helper should use pkg/provider/testutil/k8s.go (NewScheme() / NewFakeClient(objs...)) instead of a hand-rolled fake setup.
As per coding guidelines, **/*_test.go: Unit tests should use shared fake client helpers from pkg/provider/testutil/k8s.go including NewScheme() and NewFakeClient(objs...) before hand-rolling custom fakes.
🤖 Prompt for 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.
In `@pkg/lib/client/calico/ippool_test.go` around lines 20 - 29, Replace the
hand-rolled fake client in newFakeClientWithIPPools by using the shared test
util helpers: call pkg/provider/testutil/k8s.NewScheme() to obtain the scheme,
register the IPPool GVK types on that scheme (using IPPoolGVK and
IPPoolGVK.GroupVersion().WithKind("IPPoolList") as needed), then create the fake
client with pkg/provider/testutil/k8s.NewFakeClient(objs...) instead of manual
fake.NewClientBuilder code; update newFakeClientWithIPPools to return the shared
NewFakeClient result (or a builder-compatible object) and remove the manual
runtime.NewScheme()/fake.NewClientBuilder() logic.
Comment on lines
+25
to
+37
| func newFakeClientWith(objs ...runtime.Object) (*fake.ClientBuilder, error) { | ||
| scheme := runtime.NewScheme() | ||
| // Register list-kind so the fake client knows how to map GVK -> Go type | ||
| // for unstructured Network objects. For Gets without a list-kind, the | ||
| // fake client falls back to using the GVK from the object itself. | ||
| scheme.AddKnownTypeWithName(NetworkGVK, &unstructured.Unstructured{}) | ||
| scheme.AddKnownTypeWithName(NetworkGVK.GroupVersion().WithKind("NetworkList"), &unstructured.UnstructuredList{}) | ||
| b := fake.NewClientBuilder().WithScheme(scheme) | ||
| for _, o := range objs { | ||
| b = b.WithRuntimeObjects(o) | ||
| } | ||
| return b, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use shared fake-client test utilities instead of a local builder helper.
Please switch this helper to pkg/provider/testutil/k8s.go (NewScheme() / NewFakeClient(objs...)) for consistency with the repository’s test harness conventions.
As per coding guidelines, **/*_test.go: Unit tests should use shared fake client helpers from pkg/provider/testutil/k8s.go including NewScheme() and NewFakeClient(objs...) before hand-rolling custom fakes.
🤖 Prompt for 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.
In `@pkg/lib/client/calico/network_test.go` around lines 25 - 37, Replace the
local test helper newFakeClientWith with the shared test utilities: call
testutil.NewScheme() to obtain the scheme and use
testutil.NewFakeClient(objs...) to create the fake client instead of building a
fake.ClientBuilder manually; ensure NetworkGVK and its list-kind are registered
on the returned scheme if NewScheme() does not already include them, and update
tests to use the returned client from NewFakeClient rather than the builder.
Comment on lines
+142
to
+144
| if got.L2Bridge == nil || len(got.L2Bridge.VLANs) != 3 { | ||
| t.Fatalf("VLANs len = %d, want 3", len(got.L2Bridge.VLANs)) | ||
| } |
There was a problem hiding this comment.
Avoid nil dereference in the failure assertion path.
When got.L2Bridge == nil, the current t.Fatalf still evaluates len(got.L2Bridge.VLANs), which can panic and hide the real regression.
Suggested fix
- if got.L2Bridge == nil || len(got.L2Bridge.VLANs) != 3 {
- t.Fatalf("VLANs len = %d, want 3", len(got.L2Bridge.VLANs))
- }
+ if got.L2Bridge == nil {
+ t.Fatal("L2Bridge = nil, want non-nil")
+ }
+ if len(got.L2Bridge.VLANs) != 3 {
+ t.Fatalf("VLANs len = %d, want 3", len(got.L2Bridge.VLANs))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if got.L2Bridge == nil || len(got.L2Bridge.VLANs) != 3 { | |
| t.Fatalf("VLANs len = %d, want 3", len(got.L2Bridge.VLANs)) | |
| } | |
| if got.L2Bridge == nil { | |
| t.Fatal("L2Bridge = nil, want non-nil") | |
| } | |
| if len(got.L2Bridge.VLANs) != 3 { | |
| t.Fatalf("VLANs len = %d, want 3", len(got.L2Bridge.VLANs)) | |
| } |
🤖 Prompt for 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.
In `@pkg/lib/client/calico/network_test.go` around lines 142 - 144, The test
currently checks "if got.L2Bridge == nil || len(got.L2Bridge.VLANs) != 3" which
still evaluates len(got.L2Bridge.VLANs) when L2Bridge is nil; change the
assertion in the test (where "got" and "got.L2Bridge.VLANs" are referenced) to
first explicitly fail if got.L2Bridge == nil (e.g. t.Fatalf("got.L2Bridge is
nil")) and only afterwards check len(got.L2Bridge.VLANs) != 3 and fail with a
message including the actual length; this prevents a nil dereference while
preserving the existing failure messages.
66b57ac to
21b64ce
Compare
Contributor
|
Linking to #6203 The core of this seems straightforward enough, setting some annotations for a particular network type. I have read only a little bit about Calico though, and couldn't quite figure out how it works with forklift. Is the idea to do a regular migration from VMware to a Kubernetes cluster with Calico installed, and then having these annotations enables Calico to do something special with the migrated VMs? Or does Calico work with VMware/Kubernetes in some other way? |
Author
|
Hiya @mrnold, thanks for having a look, and sorry for the delay in replying. Some of the features I referenced in this code (e.g. NIC-specific annotations) are still unreleased so documentation is scarce at the moment, sorry if there was any confusion! Yes this PR would aim to facilitate migrations into Calico clusters with Kubevirt as the virtualization orchestrator, and Multus for multi-NIC support. Calico has support for requesting a particular IP address with our I'm still very much deep in WIP territory, but with this PR we'd like to detect the variant of Calico running, and depending on the answer, do one / many / all of the following:
I haven't addressed all of these points in this PR yet (only naively programmed per-interface annotations), and will hold the PR in draft until they're all addressed. |
… 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 Claude summary of this branch 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>
aaaaaaaalex
force-pushed
the
rebase-exp-against-main
branch
from
21b64ce to
3feff37
Compare
|
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
2 participants
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.
Augments vSphere builder to preserve NIC addresses for later emission of Calico annotations on VM template.
Adds a (unstructured) client for fetching and sparsely-reading Calico datatypes.
Validates (when a NAD has type: calico) Network mappings make sense.
Unit testing for new codepaths.