Fix server-side apply on BGPConfiguration object lists

[skoryk-oleksandr]

The serviceLoadBalancerIPs, serviceExternalIPs, serviceClusterIPs, communities, and prefixAdvertisements fields on BGPConfiguration are tagged +listType=set, but their elements are objects. Server-side apply only permits listType=set on lists of scalars, so any SSA-based tool (FluxCD, kubectl apply --server-side, ArgoCD) fails with:

failed to convert new object to smd typed: .spec.serviceLoadBalancerIPs: element 0: associative list without keys has an element that's a map type

This regressed in v3.32.0 when the +listType=set markers were added during the v3 CRD work in projectcalico#10447. Pre-3.32 CRDs had no list-type annotation, which SSA treats as atomic.

Switching these five fields to +listType=atomic restores the pre-3.32 SSA behavior. IgnoredInterfaces is []string so its +listType=set annotation is correct and is left alone.

Why atomic and not map

+listType=map with +listMapKey=cidr (or name for communities) would be the more expressive long-term shape — it lets multiple controllers manage different elements of the list independently. I went with atomic here because:

• Pre-3.32 CRDs had no listType annotation, which SSA treats as atomic. atomic is a true restoration of the prior behavior; map introduces new SSA semantics that nobody has been running against.
• listType=map requires the key field to be required, immutable, and non-empty. The cidr field on ServiceLoadBalancerIPBlock (and friends) is currently omitempty and not required, so converting cleanly would mean tightening the schema — a breaking change on a stable v3 API.
• This needs to cherry-pick to release branches; the smallest behavior-preserving fix is the right shape for a backport.

The listType=map conversion is a worthwhile follow-up for master only, alongside making the key fields required.

Fixes projectcalico#12700

Fix server-side apply (FluxCD, ArgoCD, `kubectl apply --server-side`) failures on BGPConfiguration resources that set serviceLoadBalancerIPs, serviceExternalIPs, serviceClusterIPs, communities, or prefixAdvertisements.

[github-actions]

🔎 Council of Claudes — Correctness

Note

Correctness lens · bugs · completeness · concurrency · edge cases

Cannot verify correctness — the annotated diff only shows a new empty file and does not contain the described CRD/type-marker changes. Based on the intent, here are correctness/completeness items to verify:

• Ensure the five fields on BGPConfiguration that are lists of objects are all switched from listType=set to listType=atomic: serviceLoadBalancerIPs, serviceExternalIPs, serviceClusterIPs, communities, prefixAdvertisements. Verify there are no remaining listType=set on any list-of-object fields.
• Keep IgnoredInterfaces (a []string) as listType=set; do not change it.
• Regenerate and commit all generated artifacts:
  • OpenAPI/CRD schema from the api types (make generate).
  • The rendered CRD YAMLs in charts/manifests (make gen-manifests), so that Helm installs contain the corrected structural schema.
• Confirm no schema tightening was introduced inadvertently (e.g., making cidr or name required) — the goal is a behavior-preserving backport.
• Validate SSA behavior end-to-end on a live cluster: kubectl apply --server-side and a representative SSA-based controller (e.g., Flux/Argo) against a BGPConfiguration using each of the five fields now marked atomic.
• Sanity-check all Calico distributions that ship CRDs (charts/crd.projectcalico.org.v1 and any operator-managed CRDs in this repo) now carry the updated listType=atomic so users don’t get mixed schemas depending on install path.

No line-specific findings (the relevant changes are not present in the provided diff).

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

🧪 Council of Claudes — Maintainability & Tests

Tip

Maintainability & Tests lens · simplicity · tests · docs · idioms

Missing test coverage and generated artifacts; likely stray file committed

• The diff only shows an empty new file (.casey-mirror-12702-v2) and none of the schema/tag changes described in the PR body. If that reflects the actual PR, it’s effectively a no-op and won’t fix the SSA issue. If it’s an excerpting error, the points below still apply to the intended change set.
• Tests: This change needs a regression test to prevent reintroducing invalid listType on object lists. Prefer a fast unit test that loads the generated CRD and asserts x-kubernetes-list-type for the five fields is atomic, and that IgnoredInterfaces remains set. This meaningfully exercises the exact surface that broke SSA.
• Generated files: Changing kubebuilder markers requires regenerating and committing the CRDs/OpenAPI and manifests. Please run make generate and make gen-manifests and include the generated changes. Without them, downstream users (and our Helm charts) won’t pick up the fix.
• Documentation/comment budget: The rationale in the PR body (why atomic and not map) is excellent institutional knowledge. Add a brief code comment above each affected field in the Go type (BGPConfigurationSpec) noting that listType is atomic to preserve pre-3.32 SSA semantics and that map may be considered on master only with required key fields. This will help future maintainers avoid “fixing” it back to set.
• Maintenance guardrail: Consider adding a small schema sanity test that fails if any listType=set is applied to a list of objects in our v3 API types. That would catch similar regressions early without relying on people remembering SSA rules.
• Repo hygiene: The new empty file .casey-mirror-12702-v2 looks accidental and increases maintenance noise. Please remove it before merge.

No line-specific anchors are provided because the annotated diff contains no content lines to anchor to. If a fuller diff is available, I’m happy to add precise anchors for test and comment locations.

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

🛡️ Council of Claudes — Security

Caution

Security lens · validation · secrets · authz · isolation

No security concerns found in the provided diff. The only change shown is the addition of an empty file; there are no functional or schema changes to assess for security impact. If the PR does update CRD listType annotations as described, that is not visible here; from a security perspective, I would want to verify in the actual CRD diff that:

• No RBAC permissions or admission policies are loosened for BGPConfiguration writes.
• The schema changes do not remove validation constraints (e.g., CIDR formats) that could allow malformed inputs to reach controllers.

No line-specific findings.

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

Council of Claudes — Nell

Important

Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments

Potentially missing intended changes; 1 likely stray file checked in

• IIUC this PR should change kubebuilder markers on BGPConfiguration and regenerate CRDs/manifests, but the diff only adds an empty dotfile. Did we miss committing the actual API/type edits and generated artifacts? I think we’ll need to run make generate and include the updated CRDs (and any downstream chart/manifests changes) in this PR.
• Looks like a stray development artefact slipped in: “.casey-mirror-12702-v2”. OK to drop that?
• Tests: would it be reasonable to add a small UT that inspects the generated CRD/OpenAPI for BGPConfiguration and asserts x-kubernetes-list-type=atomic on the five fields? If that’s heavy in this area, happy to defer, but let’s track it.
• Release note reads well; no DESIGN.md update needed for a schema bugfix. This seems like a good cherry-pick candidate, no?

No line-specific findings (the only file in the diff is empty).

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

Council of Claudes — Casey

Warning

Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness

1 potential correctness/process issue; otherwise the approach (switching to atomic to restore pre-3.32 SSA semantics) seems right to me for a backport. I am a little bit surprised though — the diff here is effectively empty, so I can’t verify the actual marker changes or the generated CRDs. A few whole-PR observations:

• I think atomic is the right short-term fix for backports — matches the pre-3.32 behavior and avoids tightening the schema in a patch release. Long-term, I agree we should move these to listType=map on master with required keys.
• Do we have the actual marker changes committed in api/pkg/apis/projectcalico/v3 (and any versioned copies), and the regenerated CRDs checked in? Per repo norms, after changing kubebuilder markers we need to run make generate and commit the generated artifacts (CRDs under manifests/ and chart templates via make gen-manifests).
• Given this regressed once (projectcalico#10447), I wonder if we can add a tiny guardrail test or check so we don’t slip back. Even a lightweight validation that fails CI if those five fields aren’t x-kubernetes-list-type: atomic in the generated CRD would carry its weight. If we can’t reasonably add a UT here, I’d still like an explicit rationale in the PR for skipping tests per the “tests required” rule.
• Minor ask: a brief comment next to each affected field explaining why atomic (SSA limitation on object lists and backport rationale) would help future readers avoid reintroducing set.

No line-specific findings — the provided diff doesn’t include the actual changes, so I can’t anchor comments to code lines.

_{🤖 Council of Claudes · 0 inline comment(s)}