Observe/Update hardening: adopt-by-name fallback, durable identity via status, error propagation, NbNetworkRouter.Update - #4
Merged
Conversation
…ining controllers Extends PR netbirdio#3's pattern (originally applied to NbNetwork / NbNetworkResource / NbNetworkRouter) to the eight controllers that were still vulnerable to the orphan-creation bug: - nbaccesstoken (critical: orphan PATs are leaked credentials) - nbsetupkey (critical: orphan setup keys are leaked credentials) - nbpolicy (high: duplicate ACL policies silently broaden access) - nbgroup (high: duplicate groups misroute peers) - nbnameserver (medium) - nbuser (medium) - nbaccount (low — singleton, consistency only) - nbdnssetting (low — singleton, consistency only) For each non-singleton controller: 1. isXNotFoundError(err) discriminates the netbird REST API's plaintext "<type>: <id> not found" / "<type> not found" messages from transient errors. Observe only returns ResourceExists: false on explicit not-found; other errors are wrapped so Crossplane requeues instead of invoking Create. 2. resolveXLookupID(cr) prefers the external-name annotation but falls back to cr.Status.AtProvider.Id when external-name is empty or was defaulted to the K8s object name by an older reconcile. 3. managed.WithInitializers() disables Crossplane's NameAsExternalName default so the K8s object name doesn't get used as a netbird ID. 4. Stale external-name annotations are repaired in-place after a successful by-ID lookup. 5. The external struct's authManager field is now an authClient interface, enabling future httptest-driven Observe tests in the same shape as the PR netbirdio#3 tests. 6. Update and Delete switched to the resolver. Controller-specific care: - nbsetupkey and nbaccesstoken Update intentionally deletes the upstream resource and clears external-name so the next Observe drives Create (key rotation). With the new resolver this also clears cr.Status.AtProvider.Id — otherwise the resolver would fall back to the now-deleted ID and Observe would loop on isXNotFoundError. - nbuser's third Observe branch uses List + local filter (the netbird SDK has no Users.Get), so it never sees a 404. The fix here is to stop swallowing List errors — they now propagate as wrapped errors. Adoption branches are also error-wrapped consistently. - nbpolicy NbPolicyObservation.Id is *string; the resolver nil-checks before dereferencing. - nbaccount and nbdnssetting are singletons with no-op Create; they receive only the wrapped-error fix for consistency. Builds clean and the existing test stubs still pass; httptest-backed Observe tests will land in a follow-up to keep this diff focused on the controller fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…p netbird SDK to v0.71.4)
The NbAccount CRD could not manage user_approval_required because the vendored
netbird SDK (v0.50.1) predates the field — its generated api.AccountExtraSettings
had no such member, so the value never reached the wire on PUT /api/accounts/{id}
nor was read back on GET. Properly supporting it requires the field to exist in
the SDK's generated model, so this bumps the dependency rather than hand-rolling
a parallel request type.
SDK bump v0.50.1 -> v0.71.4:
- Import paths moved under shared/:
management/server/http/api -> shared/management/http/api
management/client/rest -> shared/management/client/rest
Rewritten across all 14 controllers + apis + mocks.
- netbird's go.mod replace directives are not inherited by consumers, so the
relevant ones (dex fork, wireguard-go, ice, circl, easyjson, systray,
service) are mirrored into our go.mod. Without the dex replace, module
resolution picks dexidp/dex@latest which lacks server/signer and breaks
`go mod tidy`.
- Only one source-level API drift across the whole provider: the
AccountExtraSettings layout changed (NetworkTrafficLogsGroups and
UserApprovalRequired added), which broke the pointer-cast in Observe.
user_approval_required wiring (NbAccount):
- v1alpha1.AccountExtraSettings gains UserApprovalRequired (the feature) and
NetworkTrafficLogsGroups (also new in v0.71.4 — mapped so the wholesale PUT
doesn't silently clear it).
- Replaced the fragile (*v1alpha1.AccountExtraSettings)(p.Extra) struct cast
with an explicit ApitoNbAccountExtraSettings mapper, decoupling the CRD type
from upstream field-layout changes.
- NbToApiAccountSettings now sends both new fields; isUpToDate compares them
(slice compare treats nil/empty as equal to avoid drift flapping).
- Regenerated deepcopy + CRD (go generate ./apis/...). Both new fields are
optional in the CRD.
Builds clean, go vet clean, all controller unit tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…a status, swallowed Update errors, implement NbNetworkRouter.Update Observe across NbNetwork/NbNetworkResource/NbNetworkRouter/NbPolicy/NbSetupKey/ NbAccessToken now treats the external-name annotation as untrusted: compositions stamp it with the netbird display name (adoption hint) and the composite controller keeps re-applying that value, so a name-valued or stale external-name must fall back to adoption (by name, scoped to the parent network for network resources; by PeerGroupName/Peer for routers; by name under the resolved user for PATs) instead of reporting not-exists and driving a duplicate Create. Adoption now also records status.atProvider.id: external-name set during Observe is not persisted by crossplane-runtime (only late-init/Create paths persist metadata), so status is the durable identity and the lookup resolvers already prefer it. Mint-only resources (setup keys, PATs) adopt as up-to-date so the rotation-style Update (delete + re-mint) stays reserved for the expired/revoked path; setup key adoption skips revoked/expired keys. Also: - NbNetworkResource.Update returned the wrong (nil) error variable, and NbPolicy.Update discarded the API error entirely - both reported Synced while the remote was unchanged. Errors now propagate wrapped. - NbNetworkRouter.Update was an unimplemented no-op; in-place re-points (peer group / peer / metric / masquerade / enabled) now PUT to the API, and Observe gained a real up-to-date check to drive them. - NbNetworkResource Observe gained a spec-vs-API up-to-date check (name/address/enabled/description/groups) so in-place edits reconcile. - Router adoption previously matched ANY router with peer groups; it now requires the resolved peer group id to be present on the router. - Remaining swallowed transient list errors in Observe adopt paths (networks/resources/routers) now propagate so Crossplane requeues instead of calling Create. - NbNetworkRouter.Create no longer requires a peer group for peer-typed routers (peer and peer_groups are mutually exclusive in the API). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughMigrates imports to NetBird shared management packages, bumps Go/module versions, adds two NbAccount extra settings, and refactors controllers to use lookup-ID-first observation with adoption fallbacks, improved error handling, and additional adoption/unit tests. ChangesNetBird Shared Management Migration & Controller Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (3)
internal/controller/nbaccount/nbaccount.go (2)
176-196:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHandle nil
Extrasafely in compare and update conversion paths.Both paths dereference
Extrawithout nil checks. Ifapiaccount.Settings.Extraorp.Extrais nil, Observe/Update can panic.Suggested fix
func isUpToDate(nbaccount v1alpha1.NbAccount, apiaccount api.Account, c *external) bool { - if !cmp.Equal(nbaccount.Spec.ForProvider.Settings.Extra.NetworkTrafficLogsEnabled, apiaccount.Settings.Extra.NetworkTrafficLogsEnabled) { + desiredExtra := v1alpha1.AccountExtraSettings{} + if nbaccount.Spec.ForProvider.Settings.Extra != nil { + desiredExtra = *nbaccount.Spec.ForProvider.Settings.Extra + } + actualExtra := api.AccountExtraSettings{} + if apiaccount.Settings.Extra != nil { + actualExtra = *apiaccount.Settings.Extra + } + if !cmp.Equal(desiredExtra.NetworkTrafficLogsEnabled, actualExtra.NetworkTrafficLogsEnabled) { c.log.Info("extra settings NetworkTrafficLogsEnabled not equal") return false } - if !cmp.Equal(nbaccount.Spec.ForProvider.Settings.Extra.NetworkTrafficPacketCounterEnabled, apiaccount.Settings.Extra.NetworkTrafficPacketCounterEnabled) { + if !cmp.Equal(desiredExtra.NetworkTrafficPacketCounterEnabled, actualExtra.NetworkTrafficPacketCounterEnabled) { c.log.Info("extra settings NetworkTrafficPacketCounterEnabled not equal") return false } - if !cmp.Equal(nbaccount.Spec.ForProvider.Settings.Extra.PeerApprovalEnabled, apiaccount.Settings.Extra.PeerApprovalEnabled) { + if !cmp.Equal(desiredExtra.PeerApprovalEnabled, actualExtra.PeerApprovalEnabled) { c.log.Info("extra settings PeerApprovalEnabled not equal") return false } - if !cmp.Equal(nbaccount.Spec.ForProvider.Settings.Extra.UserApprovalRequired, apiaccount.Settings.Extra.UserApprovalRequired) { + if !cmp.Equal(desiredExtra.UserApprovalRequired, actualExtra.UserApprovalRequired) { c.log.Info("extra settings UserApprovalRequired not equal") return false } - if !stringSlicesEqual(nbaccount.Spec.ForProvider.Settings.Extra.NetworkTrafficLogsGroups, apiaccount.Settings.Extra.NetworkTrafficLogsGroups) { + if !stringSlicesEqual(desiredExtra.NetworkTrafficLogsGroups, actualExtra.NetworkTrafficLogsGroups) { c.log.Info("extra settings NetworkTrafficLogsGroups not equal") return false }func NbToApiAccountSettings(p v1alpha1.AccountSettings) *api.AccountSettings { - extrasettings := api.AccountExtraSettings{ - NetworkTrafficLogsEnabled: p.Extra.NetworkTrafficLogsEnabled, - NetworkTrafficLogsGroups: p.Extra.NetworkTrafficLogsGroups, - NetworkTrafficPacketCounterEnabled: p.Extra.NetworkTrafficPacketCounterEnabled, - PeerApprovalEnabled: p.Extra.PeerApprovalEnabled, - UserApprovalRequired: p.Extra.UserApprovalRequired, - } + var extrasettings *api.AccountExtraSettings + if p.Extra != nil { + extrasettings = &api.AccountExtraSettings{ + NetworkTrafficLogsEnabled: p.Extra.NetworkTrafficLogsEnabled, + NetworkTrafficLogsGroups: p.Extra.NetworkTrafficLogsGroups, + NetworkTrafficPacketCounterEnabled: p.Extra.NetworkTrafficPacketCounterEnabled, + PeerApprovalEnabled: p.Extra.PeerApprovalEnabled, + UserApprovalRequired: p.Extra.UserApprovalRequired, + } + } accountsettings := api.AccountSettings{ - Extra: &extrasettings, + Extra: extrasettings,Also applies to: 337-344
🤖 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 `@internal/controller/nbaccount/nbaccount.go` around lines 176 - 196, The comparison code in isUpToDate (and the Observe/Update conversion paths) dereferences nbaccount.Spec.ForProvider.Settings.Extra and apiaccount.Settings.Extra (and p.Extra) without nil checks which can cause panics; update isUpToDate and the conversion/observe/update functions to first guard against nil by treating nil and empty as equivalent (e.g., consider a nil Extra as an empty struct or short-circuit comparisons) before accessing fields like NetworkTrafficLogsEnabled, NetworkTrafficPacketCounterEnabled, PeerApprovalEnabled, UserApprovalRequired and NetworkTrafficLogsGroups, and reuse stringSlicesEqual only after ensuring the slice refs are non-nil to avoid panics.
129-137:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard empty account list before indexing.
accounts[0]on Line 157 can panic when the API returns an empty list. Return a controlled error (or explicit not-found behavior) instead of crashing reconcile.Suggested fix
accounts, err := client.Accounts.List(ctx) if err != nil { if auth.IsTokenInvalidError(err) { c.authManager.ForceRefresh(ctx) return managed.ExternalObservation{}, err } // Don't swallow transient errors — Crossplane should requeue, not call Create. return managed.ExternalObservation{}, errors.Wrap(err, "failed to list accounts") } +if len(accounts) == 0 { + return managed.ExternalObservation{}, errors.New("failed to observe account: API returned no accounts") +} accountusers, err := client.Users.List(ctx)Also applies to: 157-157
🤖 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 `@internal/controller/nbaccount/nbaccount.go` around lines 129 - 137, The code assumes accounts[0] exists after calling client.Accounts.List and can panic on an empty response; add a guard after the call to client.Accounts.List that checks len(accounts) == 0 and return a controlled error or explicit not-found observation (e.g., return managed.ExternalObservation{}, errors.New("no accounts found") or the Crossplane NotFound flow) instead of indexing into accounts; keep the existing auth token handling (auth.IsTokenInvalidError and c.authManager.ForceRefresh) intact and reference the accounts slice and the managed.ExternalObservation{} return to locate where to add the check.internal/controller/nbaccesstoken/nbaccesstoken.go (1)
149-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify user-resolution rules across Observe and mutate paths.
Line 162 restricts Observe to
IsServiceUser == true, butgetUserID(used by Update) and Create/Delete resolve by name without that filter. This can reconcile one user while rotating/deleting against another in ambiguous-name cases.Also applies to: 294-309
🤖 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 `@internal/controller/nbaccesstoken/nbaccesstoken.go` around lines 149 - 173, The Observe path currently filters users by IsServiceUser when resolving a username, which differs from getUserID (used by Create/Update/Delete) and can cause mismatches; change Observe to reuse the same resolution logic as getUserID (or call getUserID directly) so name-based lookup does not require IsServiceUser == true, preserve the existing fallback to cr.Spec.ForProvider.UserId when provided, and make the same change in the other similar observe block to ensure Create/Update/Delete and Observe all resolve the same user identifier (affecting functions Observe and any duplicate resolution around the later block).
🧹 Nitpick comments (4)
internal/controller/nbnetworkresource/nbnetworkresource.go (1)
254-271: 💤 Low valueAsymmetric group comparison may miss drift when API has extra groups.
The comparison checks that every spec group exists in the API response, but doesn't verify the reverse. If the API resource has additional groups not present in the spec (e.g., added out-of-band), this function still returns
true(up-to-date), preventing an Update that would remove the extra groups.If this is intentional (e.g., to allow external additions), consider adding a comment. Otherwise, compare lengths first or check both directions.
🤖 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 `@internal/controller/nbnetworkresource/nbnetworkresource.go` around lines 254 - 271, The current group comparison only ensures every spec group exists in res.Groups but not the reverse, allowing extra API groups to be ignored; update the check in nbnetworkresource.go (the block referencing spec.Groups and res.Groups) to perform a symmetric comparison: first verify lengths match, then ensure every group in spec has a matching group in res (by Id or Name) and every group in res has a matching group in spec (by Id or Name); alternatively build lookup maps keyed by Id/Name and compare keys both ways so any extra groups in res are detected and cause a false (not up-to-date) result.internal/controller/nbnetworkrouter/nbnetworkrouter.go (1)
364-368: 💤 Low valueMissing
breakafter finding the network.The
Updatemethod (line 435) includes abreakafter finding the matching network, butCreatedoes not. While network names are expected to be unique, addingbreakhere would be consistent withUpdateand avoid unnecessary iterations.♻️ Suggested fix
for _, network := range networks { if network.Name == cr.Spec.ForProvider.NetworkName { apinetwork = &network + break } }🤖 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 `@internal/controller/nbnetworkrouter/nbnetworkrouter.go` around lines 364 - 368, In the Create logic where you loop "for _, network := range networks" to locate the network matching cr.Spec.ForProvider.NetworkName and assign apinetwork = &network, add a break immediately after the assignment so the loop stops once a match is found (mirroring the Update method behavior) to avoid unnecessary iterations; this change targets the loop that sets apinetwork in the Create flow.internal/controller/nbsetupkey/nbsetupkey_test.go (1)
105-105: ⚡ Quick winVariable
authshadows the package import.The local variable
authshadows the imported packageauth "github.com/crossplane/netbird-crossplane-provider/internal/controller/nb", which can cause confusion when reading the code.♻️ Rename local variable to avoid shadowing
t.Run("EmptyLookupAdoptsValidKeyByName", func(t *testing.T) { - auth, srv := newFakeAuth(t, func(w http.ResponseWriter, r *http.Request) { + fakeAuth, srv := newFakeAuth(t, func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/setup-keys" && r.Method == http.MethodGet { w.Header().Set("Content-Type", "application/json") w.Write([]byte(`[{"id":"sk-1","name":"bootstrap-key","revoked":false,"expires":"2099-01-01T00:00:00Z","last_used":"2026-01-01T00:00:00Z","state":"valid","type":"reusable","auto_groups":[]}]`)) return } t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) }) defer srv.Close() - e := external{authManager: auth} + e := external{authManager: fakeAuth}Apply the same rename in the second subtest.
Also applies to: 135-135
🤖 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 `@internal/controller/nbsetupkey/nbsetupkey_test.go` at line 105, The local test variable named auth shadows the imported package alias auth; rename the local variable returned by newFakeAuth (e.g., fakeAuth or authSrv) and update all references (including the companion srv variable use) in both subtests to avoid shadowing; ensure you apply the same rename in the second subtest where newFakeAuth is called so the imported package auth remains unshadowed.internal/controller/nbsetupkey/nbsetupkey.go (1)
326-341: 💤 Low valueConsider ignoring not-found errors when deleting during rotation.
If the setup key was already deleted externally, the Delete call will fail, causing unnecessary retries. Since the goal is to clear the key so a new one can be created, a not-found error could be treated as success.
♻️ Optional: swallow not-found on rotation delete
- if err := client.SetupKeys.Delete(ctx, setupKeyId); err != nil { - return managed.ExternalUpdate{}, errors.Wrap(err, "failed to delete expired setupkey") + if err := client.SetupKeys.Delete(ctx, setupKeyId); err != nil && !isSetupKeyNotFoundError(err) { + return managed.ExternalUpdate{}, errors.Wrap(err, "failed to delete expired setupkey") }🤖 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 `@internal/controller/nbsetupkey/nbsetupkey.go` around lines 326 - 341, The Delete call in the rotation path should ignore "not found" errors so an already-removed setup key doesn't cause retries: inside the block where you call client.SetupKeys.Delete(ctx, setupKeyId) (after resolveSetupKeyLookupID returns a non-empty id), detect the provider's not-found error type or value and treat it as a successful delete (i.e., continue clearing external-name and cr.Status.AtProvider.Id and return the managed.ExternalUpdate) instead of returning an error; keep the existing meta.SetExternalName and status-clearing logic and only wrap/return non-not-found errors as failures.
🤖 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 `@apis/vpn/v1alpha1/nbaccount_types.go`:
- Around line 54-56: The UserApprovalRequired field in nbaccount_types.go is
currently a non-pointer bool so it cannot represent "unset"; change the field
type to *bool and update the JSON tag to include omitempty (i.e.,
UserApprovalRequired *bool `json:"user_approval_required,omitempty"`), so
controllers can distinguish omitted vs explicit false; update any
construction/usage sites (e.g., where New NBAccount or defaulting/validation
logic touches UserApprovalRequired) to handle nil pointers appropriately.
In `@internal/controller/nbaccesstoken/nbaccesstoken_test.go`:
- Around line 113-124: The test handler passed into newFakeAuth must not call
t.Fatalf from the server goroutine; replace the default case so the handler
writes an HTTP error (e.g., w.WriteHeader(http.StatusInternalServerError) and a
small message) and send the unexpected-request details to the test goroutine via
a channel (e.g., create errCh := make(chan string,1) in the test and in the
handler do errCh <- fmt.Sprintf("unexpected request: %s %s", r.Method,
r.URL.Path)); after exercising the client, read from errCh (non-blocking or with
close) in the test goroutine and call t.Fatalf/t.Errorf there if a message was
received. Ensure you update the code around newFakeAuth and the inline handler
to use the channel and remove the t.Fatalf from the handler.
In `@internal/controller/nbaccesstoken/nbaccesstoken.go`:
- Around line 400-403: In the Delete handler, do not hard-fail when
resolveAccessTokenLookupID(cr) returns an empty string; instead treat an empty
lookupID as already-deleted/no-op by skipping any external-provider deletion and
returning nil (optionally log a debug/info message). Update the logic in the
method that calls resolveAccessTokenLookupID so that when lookupID == "" it does
not return errors.New(...) but returns nil after ensuring any finalizer removal
or local cleanup still occurs.
In `@internal/controller/nbgroup/nbgroup.go`:
- Around line 226-234: The observe logic in nbgroup.go currently returns
ResourceExists:false when isGroupNotFoundError(err) is true, which prevents the
adoption-by-name fallback; change the control flow in the Observe method so that
when isGroupNotFoundError(err) is true you do not return immediately but
continue into the adoption-by-name branch (the same pattern used in nbpolicy.go
around lines 163-169): remove the early return, allow the code to attempt a
name-based lookup using lookupID/name, and only return ResourceExists:false
after both ID and name adoption attempts fail; keep the existing wrapped error
return for non-not-found errors.
In `@internal/controller/nbuser/nbuser.go`:
- Around line 297-307: The resolver resolveUserLookupID currently treats an
external name equal to the Kubernetes object name as a legacy default and
returns status.AtProvider.Id, but it fails to treat an external name equal to
spec.forProvider.name the same way; update resolveUserLookupID so the recovery
branch checks if cr.Status.AtProvider.Id != "" && (externalName == cr.GetName()
|| externalName == cr.Spec.ForProvider.Name) && cr.Status.AtProvider.Id !=
externalName and then return cr.Status.AtProvider.Id; ensure callers
(Users.Update and Users.Delete) continue to use resolveUserLookupID so they
receive the real ID rather than spec.forProvider.name.
---
Outside diff comments:
In `@internal/controller/nbaccesstoken/nbaccesstoken.go`:
- Around line 149-173: The Observe path currently filters users by IsServiceUser
when resolving a username, which differs from getUserID (used by
Create/Update/Delete) and can cause mismatches; change Observe to reuse the same
resolution logic as getUserID (or call getUserID directly) so name-based lookup
does not require IsServiceUser == true, preserve the existing fallback to
cr.Spec.ForProvider.UserId when provided, and make the same change in the other
similar observe block to ensure Create/Update/Delete and Observe all resolve the
same user identifier (affecting functions Observe and any duplicate resolution
around the later block).
In `@internal/controller/nbaccount/nbaccount.go`:
- Around line 176-196: The comparison code in isUpToDate (and the Observe/Update
conversion paths) dereferences nbaccount.Spec.ForProvider.Settings.Extra and
apiaccount.Settings.Extra (and p.Extra) without nil checks which can cause
panics; update isUpToDate and the conversion/observe/update functions to first
guard against nil by treating nil and empty as equivalent (e.g., consider a nil
Extra as an empty struct or short-circuit comparisons) before accessing fields
like NetworkTrafficLogsEnabled, NetworkTrafficPacketCounterEnabled,
PeerApprovalEnabled, UserApprovalRequired and NetworkTrafficLogsGroups, and
reuse stringSlicesEqual only after ensuring the slice refs are non-nil to avoid
panics.
- Around line 129-137: The code assumes accounts[0] exists after calling
client.Accounts.List and can panic on an empty response; add a guard after the
call to client.Accounts.List that checks len(accounts) == 0 and return a
controlled error or explicit not-found observation (e.g., return
managed.ExternalObservation{}, errors.New("no accounts found") or the Crossplane
NotFound flow) instead of indexing into accounts; keep the existing auth token
handling (auth.IsTokenInvalidError and c.authManager.ForceRefresh) intact and
reference the accounts slice and the managed.ExternalObservation{} return to
locate where to add the check.
---
Nitpick comments:
In `@internal/controller/nbnetworkresource/nbnetworkresource.go`:
- Around line 254-271: The current group comparison only ensures every spec
group exists in res.Groups but not the reverse, allowing extra API groups to be
ignored; update the check in nbnetworkresource.go (the block referencing
spec.Groups and res.Groups) to perform a symmetric comparison: first verify
lengths match, then ensure every group in spec has a matching group in res (by
Id or Name) and every group in res has a matching group in spec (by Id or Name);
alternatively build lookup maps keyed by Id/Name and compare keys both ways so
any extra groups in res are detected and cause a false (not up-to-date) result.
In `@internal/controller/nbnetworkrouter/nbnetworkrouter.go`:
- Around line 364-368: In the Create logic where you loop "for _, network :=
range networks" to locate the network matching cr.Spec.ForProvider.NetworkName
and assign apinetwork = &network, add a break immediately after the assignment
so the loop stops once a match is found (mirroring the Update method behavior)
to avoid unnecessary iterations; this change targets the loop that sets
apinetwork in the Create flow.
In `@internal/controller/nbsetupkey/nbsetupkey_test.go`:
- Line 105: The local test variable named auth shadows the imported package
alias auth; rename the local variable returned by newFakeAuth (e.g., fakeAuth or
authSrv) and update all references (including the companion srv variable use) in
both subtests to avoid shadowing; ensure you apply the same rename in the second
subtest where newFakeAuth is called so the imported package auth remains
unshadowed.
In `@internal/controller/nbsetupkey/nbsetupkey.go`:
- Around line 326-341: The Delete call in the rotation path should ignore "not
found" errors so an already-removed setup key doesn't cause retries: inside the
block where you call client.SetupKeys.Delete(ctx, setupKeyId) (after
resolveSetupKeyLookupID returns a non-empty id), detect the provider's not-found
error type or value and treat it as a successful delete (i.e., continue clearing
external-name and cr.Status.AtProvider.Id and return the managed.ExternalUpdate)
instead of returning an error; keep the existing meta.SetExternalName and
status-clearing logic and only wrap/return non-not-found errors as failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a4347a5-40c9-492a-a46b-9dc88f491536
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.gitignoreapis/vpn/v1alpha1/nbaccount_types.goapis/vpn/v1alpha1/nbgroup_types.goapis/vpn/v1alpha1/zz_generated.deepcopy.gogo.modinternal/controller/nb/auth.gointernal/controller/nb/mock/mock_auth.gointernal/controller/nbaccesstoken/nbaccesstoken.gointernal/controller/nbaccesstoken/nbaccesstoken_test.gointernal/controller/nbaccount/nbaccount.gointernal/controller/nbdnssetting/nbdnssetting.gointernal/controller/nbgroup/nbgroup.gointernal/controller/nbnameserver/nbnameserver.gointernal/controller/nbnetwork/nbnetwork.gointernal/controller/nbnetwork/nbnetwork_test.gointernal/controller/nbnetworkresource/nbnetworkresource.gointernal/controller/nbnetworkresource/nbnetworkresource_test.gointernal/controller/nbnetworkrouter/nbnetworkrouter.gointernal/controller/nbnetworkrouter/nbnetworkrouter_test.gointernal/controller/nbpolicy/nbpolicy.gointernal/controller/nbsetupkey/nbsetupkey.gointernal/controller/nbsetupkey/nbsetupkey_test.gointernal/controller/nbuser/nbuser.gopackage/crds/vpn.netbird.crossplane.io_nbaccounts.yaml
Comment on lines
+54
to
+56
| // UserApprovalRequired Enables manual approval for new users joining via domain matching. When enabled, users are blocked with pending approval status until explicitly approved by an admin. | ||
| // +optional | ||
| UserApprovalRequired bool `json:"user_approval_required"` |
There was a problem hiding this comment.
Optional scalar cannot preserve “unset” semantics
UserApprovalRequired is marked optional, but bool without omitempty cannot distinguish omitted vs explicit false. This can cause unintended state overwrites during reconcile/update flows.
Suggested fix
- // UserApprovalRequired Enables manual approval for new users joining via domain matching. When enabled, users are blocked with pending approval status until explicitly approved by an admin.
- // +optional
- UserApprovalRequired bool `json:"user_approval_required"`
+ // UserApprovalRequired Enables manual approval for new users joining via domain matching. When enabled, users are blocked with pending approval status until explicitly approved by an admin.
+ // +optional
+ UserApprovalRequired *bool `json:"user_approval_required,omitempty"`🤖 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 `@apis/vpn/v1alpha1/nbaccount_types.go` around lines 54 - 56, The
UserApprovalRequired field in nbaccount_types.go is currently a non-pointer bool
so it cannot represent "unset"; change the field type to *bool and update the
JSON tag to include omitempty (i.e., UserApprovalRequired *bool
`json:"user_approval_required,omitempty"`), so controllers can distinguish
omitted vs explicit false; update any construction/usage sites (e.g., where New
NBAccount or defaulting/validation logic touches UserApprovalRequired) to handle
nil pointers appropriately.
Comment on lines
+113
to
+124
| auth, srv := newFakeAuth(t, func(w http.ResponseWriter, r *http.Request) { | ||
| switch { | ||
| case r.URL.Path == "/api/users" && r.Method == http.MethodGet: | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte(`[{"id":"u-1","name":"svc-user","is_service_user":true,"email":"","role":"admin","auto_groups":[],"is_blocked":false,"issued":"api","status":"active"}]`)) | ||
| case r.URL.Path == "/api/users/u-1/tokens" && r.Method == http.MethodGet: | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte(`[{"id":"t-1","name":"bootstrap-pat","expiration_date":"2099-01-01T00:00:00Z","created_by":"u-1","created_at":"2026-01-01T00:00:00Z"}]`)) | ||
| default: | ||
| t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's testing package, can t.Fatalf (or FailNow) be safely called from a goroutine spawned outside the test function, such as an httptest server handler?
💡 Result:
No, calling t.Fatalf (or t.FailNow, t.Fatal, and related methods) from a goroutine spawned outside the test function is not safe and is explicitly prohibited by the Go testing package [1][2]. According to the official documentation, these methods must be called only from the goroutine running the Test function [1]. When you call t.Fatalf, it invokes t.FailNow, which in turn calls runtime.Goexit to terminate the execution of the current goroutine [1][3]. If called from a spawned goroutine, it will only terminate that specific goroutine, not the test itself, often leading to confusing test results, silent failures, or unexpected panics if the test function continues or completes prematurely [4][5]. To handle errors within concurrent code or goroutines (such as an httptest server handler), you should use thread-safe reporting methods that do not terminate the test execution immediately, such as t.Error or t.Errorf [1][6]. These methods allow you to record the failure while permitting the test function to continue and manage its own synchronization [1]. You can then synchronize the completion of your goroutines with the test function (e.g., using sync.WaitGroup or channels) to ensure all errors are reported before the test finishes [6]. The Go toolchain includes an analyzer called testinggoroutine (often run as part of go vet) that specifically detects and warns against incorrect calls to t.Fatal/Fatalf from non-test goroutines [7][8].
Citations:
- 1: https://pkg.go.dev/testing
- 2: https://deepsource.com/directory/go/issues/GO-W6004
- 3: testing: t.Fatal does not fatal tests if defers do not complete golang/go#22989
- 4: testing: detect t.FailNow() called from wrong goroutine golang/go#24678
- 5: testing: nested t.Fatalf hides errors golang/go#22882
- 6: How to handle failed expectations inside of a goroutine? stretchr/testify#772
- 7: proposal: cmd/vet: include testinggoroutine analyzer in 'go test' suite golang/go#76739
- 8: https://golang.bg/pkg/cmd/vendor/golang.org/x/tools/go/analysis/passes/testinggoroutine/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/controller/nbaccesstoken/nbaccesstoken_test.go"
python3 - <<'PY'
import itertools
path="internal/controller/nbaccesstoken/nbaccesstoken_test.go"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
for start,end in [(90,140),(140,190)]:
for i in range(start-1,end):
if 0<=i<len(lines):
print(f"{i+1}\t{lines[i].rstrip()}")
PY
echo "----"
rg -n "t\.Fatalf\(" internal/controller/nbaccesstoken/nbaccesstoken_test.go || trueRepository: netbirdio/netbird-crossplane-provider
Length of output: 1996
Don’t call t.Fatalf from httptest HTTP handlers
default: t.Fatalf(...) runs in the HTTP server goroutine; t.Fatalf/FailNow is explicitly not safe from non-test goroutines and can cause flaky/incorrect test results. In the handler, return an error response instead and report the failure from the test goroutine (e.g., via t.Errorf or by sending the error over a channel/WaitGroup).
🤖 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 `@internal/controller/nbaccesstoken/nbaccesstoken_test.go` around lines 113 -
124, The test handler passed into newFakeAuth must not call t.Fatalf from the
server goroutine; replace the default case so the handler writes an HTTP error
(e.g., w.WriteHeader(http.StatusInternalServerError) and a small message) and
send the unexpected-request details to the test goroutine via a channel (e.g.,
create errCh := make(chan string,1) in the test and in the handler do errCh <-
fmt.Sprintf("unexpected request: %s %s", r.Method, r.URL.Path)); after
exercising the client, read from errCh (non-blocking or with close) in the test
goroutine and call t.Fatalf/t.Errorf there if a message was received. Ensure you
update the code around newFakeAuth and the inline handler to use the channel and
remove the t.Fatalf from the handler.
Comment on lines
+400
to
403
| lookupID := resolveAccessTokenLookupID(cr) | ||
| if lookupID == "" { | ||
| return errors.New("can't find access token id") | ||
| } |
There was a problem hiding this comment.
Avoid hard-failing Delete when provider lookup ID is absent.
At Line 401, returning an error for empty lookupID can leave the MR stuck in deletion (finalizer never clears) when no external ID was ever persisted. Treat this as already-deleted/no-op.
Suggested fix
lookupID := resolveAccessTokenLookupID(cr)
if lookupID == "" {
- return errors.New("can't find access token id")
+ c.log.Info("no access token id recorded; treating delete as no-op")
+ return nil
}📝 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
| lookupID := resolveAccessTokenLookupID(cr) | |
| if lookupID == "" { | |
| return errors.New("can't find access token id") | |
| } | |
| lookupID := resolveAccessTokenLookupID(cr) | |
| if lookupID == "" { | |
| c.log.Info("no access token id recorded; treating delete as no-op") | |
| return nil | |
| } |
🤖 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 `@internal/controller/nbaccesstoken/nbaccesstoken.go` around lines 400 - 403,
In the Delete handler, do not hard-fail when resolveAccessTokenLookupID(cr)
returns an empty string; instead treat an empty lookupID as
already-deleted/no-op by skipping any external-provider deletion and returning
nil (optionally log a debug/info message). Update the logic in the method that
calls resolveAccessTokenLookupID so that when lookupID == "" it does not return
errors.New(...) but returns nil after ensuring any finalizer removal or local
cleanup still occurs.
Comment on lines
+226
to
234
| if isGroupNotFoundError(err) { | ||
| c.log.Info("group not found", "lookup-id", lookupID) | ||
| return managed.ExternalObservation{ | ||
| ResourceExists: false, | ||
| }, nil | ||
| } | ||
| // Don't swallow transient errors — Crossplane should requeue, not call Create. | ||
| return managed.ExternalObservation{}, errors.Wrapf(err, "failed to observe group %q", lookupID) | ||
| } |
There was a problem hiding this comment.
Missing adopt-by-name fallback after ID not-found.
When isGroupNotFoundError is true, this branch returns ResourceExists: false immediately without falling back to adoption by name. This is inconsistent with the pattern in nbpolicy.go (lines 163-169), which continues to the adoption-by-name path when the ID lookup fails.
If a group's ID becomes stale (e.g., the group was deleted and recreated externally), this controller will attempt to create a duplicate instead of adopting the existing group with the matching name.
Consider restructuring to match the policy controller's pattern: try ID lookup first, then fall back to adoption-by-name on not-found.
Suggested approach
c.log.Info("external name set, fetching by ID", "lookupID", lookupID)
-apigroup, err := client.Groups.Get(ctx, lookupID)
+var group *nbapi.Group
+apigroup, err := client.Groups.Get(ctx, lookupID)
if err != nil {
if auth.IsTokenInvalidError(err) {
c.authManager.ForceRefresh(ctx)
return managed.ExternalObservation{}, err
}
- if isGroupNotFoundError(err) {
- c.log.Info("group not found", "lookup-id", lookupID)
- return managed.ExternalObservation{
- ResourceExists: false,
- }, nil
+ if !isGroupNotFoundError(err) {
+ return managed.ExternalObservation{}, errors.Wrapf(err, "failed to observe group %q", lookupID)
}
- // Don't swallow transient errors — Crossplane should requeue, not call Create.
- return managed.ExternalObservation{}, errors.Wrapf(err, "failed to observe group %q", lookupID)
+ c.log.Info("group not found by id, attempting adoption by name", "lookup-id", lookupID)
+} else {
+ group = apigroup
+}
+
+if group == nil {
+ // Adoption by Name fallback
+ groups, err := client.Groups.List(ctx)
+ if err != nil {
+ if auth.IsTokenInvalidError(err) {
+ c.authManager.ForceRefresh(ctx)
+ return managed.ExternalObservation{}, err
+ }
+ return managed.ExternalObservation{}, errors.Wrap(err, "failed to list groups for adoption")
+ }
+ for _, apigroup := range groups {
+ if apigroup.Name == cr.Spec.ForProvider.Name {
+ c.log.Info("found existing group for adoption", "groupid", apigroup.Id)
+ group = &apigroup
+ break
+ }
+ }
+ if group == nil {
+ return managed.ExternalObservation{ResourceExists: false}, nil
+ }
}
-group := *apigroup🤖 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 `@internal/controller/nbgroup/nbgroup.go` around lines 226 - 234, The observe
logic in nbgroup.go currently returns ResourceExists:false when
isGroupNotFoundError(err) is true, which prevents the adoption-by-name fallback;
change the control flow in the Observe method so that when
isGroupNotFoundError(err) is true you do not return immediately but continue
into the adoption-by-name branch (the same pattern used in nbpolicy.go around
lines 163-169): remove the early return, allow the code to attempt a name-based
lookup using lookupID/name, and only return ResourceExists:false after both ID
and name adoption attempts fail; keep the existing wrapped error return for
non-not-found errors.
Comment on lines
+297
to
+307
| func resolveUserLookupID(cr *v1alpha1.NbUser) string { | ||
| externalName := meta.GetExternalName(cr) | ||
| switch { | ||
| case externalName == "": | ||
| return cr.Status.AtProvider.Id | ||
| case cr.Status.AtProvider.Id != "" && externalName == cr.GetName() && cr.Status.AtProvider.Id != externalName: | ||
| // Recover from older reconciles that defaulted the external name to the Kubernetes object name. | ||
| return cr.Status.AtProvider.Id | ||
| default: | ||
| return externalName | ||
| } |
There was a problem hiding this comment.
Recover from spec.forProvider.name in resolveUserLookupID to prevent invalid Update/Delete IDs.
Line 302 only recovers when external-name equals the Kubernetes object name. When external-name equals spec.forProvider.name, the resolver returns the name instead of status.atProvider.id, so Lines 349 and 379 can call Users.Update/Delete with a non-ID value.
Suggested fix
func resolveUserLookupID(cr *v1alpha1.NbUser) string {
externalName := meta.GetExternalName(cr)
switch {
case externalName == "":
return cr.Status.AtProvider.Id
- case cr.Status.AtProvider.Id != "" && externalName == cr.GetName() && cr.Status.AtProvider.Id != externalName:
- // Recover from older reconciles that defaulted the external name to the Kubernetes object name.
+ case cr.Status.AtProvider.Id != "" &&
+ cr.Status.AtProvider.Id != externalName &&
+ (externalName == cr.GetName() || externalName == cr.Spec.ForProvider.Name):
+ // Recover when external-name is object name (older reconciles)
+ // or spec name (adoption hint), using durable provider ID from status.
return cr.Status.AtProvider.Id
default:
return externalName
}
}Also applies to: 349-352, 379-383
🤖 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 `@internal/controller/nbuser/nbuser.go` around lines 297 - 307, The resolver
resolveUserLookupID currently treats an external name equal to the Kubernetes
object name as a legacy default and returns status.AtProvider.Id, but it fails
to treat an external name equal to spec.forProvider.name the same way; update
resolveUserLookupID so the recovery branch checks if cr.Status.AtProvider.Id !=
"" && (externalName == cr.GetName() || externalName == cr.Spec.ForProvider.Name)
&& cr.Status.AtProvider.Id != externalName and then return
cr.Status.AtProvider.Id; ensure callers (Users.Update and Users.Delete) continue
to use resolveUserLookupID so they receive the real ID rather than
spec.forProvider.name.
The netbird SDK v0.71.4 bump (92df245) raised the go.mod go directive to 1.25.5, but CI still pinned GO_VERSION=1.21. setup-go then installed 1.21 and the 1.25.5 directive forced a toolchain auto-download whose `covdata` tool is not present, so the coverage step (`go test -covermode=count -coverprofile`) failed with `go: no such tool "covdata"` even though every test passed. Pinning GO_VERSION to the go.mod version makes setup-go install the matching toolchain directly. Verified locally: CI's exact coverage command is green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Generalize comments and test data for upstream readability — no behavior
change:
- Comments framed the name-valued external-name case specifically around
compositions/composite controllers stamping the display name. Reword to
describe the general mechanism (external-name may hold a defaulted object
name, or a display name used as an adoption hint).
- Test fixtures used environment-specific group names ("bao-routers");
replace with generic names ("routers"/"clients", "g-routers").
- Drop a stray cluster reference in a test comment and rename the subtest
accordingly.
Comment/fixture-only; all controller tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Follow-up to #3, broadening the Observe recovery pattern across the remaining controllers and fixing several Update-path bugs. Three commits:
1.
fix(nb*): Observe error handling + external-name recovery across remaining controllersRolls out the #3 pattern (lookup-ID resolver with status fallback, transient errors propagated instead of swallowed into
ResourceExists: false,WithInitializers()to disable NameAsExternalName) to NbGroup, NbSetupKey, NbPolicy, NbNameserver, NbUser, NbAccessToken, NbNetwork.2.
feat(nbaccount): support settings.extra.user_approval_requiredBumps the netbird SDK to v0.71.4 and exposes
settings.extra.user_approval_requiredon NbAccount, so an account's default approval gate for new SSO users can be managed declaratively.3.
fix(nb*): adopt-by-name fallback after not-found, persist identity via status, swallowed Update errors, implement NbNetworkRouter.Updatecrossplane.io/external-nameannotation as untrusted: it may legitimately hold a display name (set by a user or by tooling as an adoption hint) or a stale ID. After a not-found on the ID lookup, Observe falls back to adoption — by name scoped to the parent network for network resources, by PeerGroupName/Peer for routers, by name under the resolved user for PATs — instead of reporting not-exists and driving a duplicate Create. Live failure mode this fixes: a Create succeeds remotely but the external-name persist is lost, after which every reconcile re-Creates and fails withresource with name "..." already exists, wedging the MR permanently.status.atProvider.id. crossplane-runtime does not persist metadata changes made during Observe (only the late-init and Create paths persist), so status is the durable identity; the lookup resolvers already prefer it.NbNetworkResource.Updatereturned the wrong (nil) error variable andNbPolicy.Updatediscarded the API error entirely — both reported Synced while the remote write had failed. Errors now propagate wrapped.NbNetworkRouter.Updatewas an unimplemented no-op; it now PUTs the desired spec (enabled/masquerade/metric/peer/peer-groups), and Observe gained real up-to-date checks for routers and network resources so in-place changes actually reconcile.NbNetworkRouter.Createno longer requires a peer group for peer-identified routers (peerandpeer_groupsare mutually exclusive in the API).Tests: table-driven httptest coverage added for the adoption fallbacks (stale-ID and display-name external-names), drift detection, Update error propagation, and the router Update implementation.
make generateproduces no CRD diff except the NbAccount field from commit 2.Validated on a live cluster: three MRs wedged on the duplicate-Create loop adopted their existing remote resources and went Ready/Synced with no manual intervention after rolling out this build.
Notes for reviewers
Dependency footprint. Commit 2 bumps
github.com/netbirdio/netbirdfrom v0.50.1 to v0.71.4. This moves the client import paths undershared/and — because Go does not inherit a dependency'sreplacedirectives — requires mirroring netbird'sreplaceset into thisgo.mod(dex fork, wireguard-go, ice, circl, easyjson, systray, service). Consuming the SDK at this version also pulls a large slice of the netbird management-server dependency tree transitively (redis, gorm, the SQL drivers, dex, etc.), even though the provider only uses the REST client — a build-time, image-size, and CVE-surface cost worth weighing. If netbird exposes a lighter client-only module, consuming that instead would be preferable; happy to adjust to maintainer preference. Thegodirective moves to 1.25.5 to match the SDK and its transitive requirements (golang.org/x/*,go.opentelemetry.io/otel/*), and CI'sGO_VERSIONis bumped to match so the coverage step's toolchain hascovdata.Behavior change (upgrade note). Adding
managed.WithInitializers()(empty) disables crossplane-runtime'sNameAsExternalNameinitializer, so a managed resource'scrossplane.io/external-nameis no longer defaulted to the Kubernetes object name. Existing resources whose external-name was auto-defaulted recover automatically on the first Observe after upgrade — falling back tostatus.atProvider.idand then to adopt-by-name — but operators should be aware the annotation semantics change. No CRD/spec changes are required; the one new field (NbAccountsettings.extra.user_approval_required) is optional.Summary by CodeRabbit
New Features
Bug Fixes
Tests