feat(rest-api): Recovery of missing VPC Prefixes from Site inventory - #4913
feat(rest-api): Recovery of missing VPC Prefixes from Site inventory#4913hwadekar-nv wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughThe change adds exact child-CIDR acquisition, soft-delete clearing, and Site inventory recovery for VPC Prefix records. Recovery validates ownership and CIDRs, performs transactional IPAM allocation, restores matching records, and strengthens deletion cleanup handling. ChangesVPC Prefix Recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟡 Moderate · up to VPC Prefix recovery may mishandle IPAM claims when the reported prefix matches a FullGrant block at equal length, potentially creating incorrect REST/IPAM state. Merge should wait for this bounded correctness issue to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant SiteInventory
participant VpcPrefixWorkflow
participant VpcPrefixDAO
participant IPAM
SiteInventory->>VpcPrefixWorkflow: Report controller prefix and CIDR
VpcPrefixWorkflow->>VpcPrefixDAO: Find active or soft-deleted VPC Prefix
VpcPrefixWorkflow->>IPAM: Acquire or claim exact child CIDR
IPAM-->>VpcPrefixWorkflow: Return allocation result
VpcPrefixWorkflow->>VpcPrefixDAO: Create or clear deletion marker
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-13 02:05:41 UTC | Commit: 45e0f0a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
rest-api/workflow/pkg/activity/vpcprefix/vpcprefix.go (2)
527-534: 🩺 Stability & Availability | 🔵 TrivialPlan an escape hatch for a permanently blocked cleanup.
Returning an error when
IPBlockIDis set but the relation is nil correctly prevents a leaked child CIDR. It also creates a terminal state: if the parent IP Block row is soft-deleted, the relation never loads again, and the caller at lines 184-215 retries the same failing delete on every inventory cycle. The VPC Prefix then stays inDeletingforever with no operator signal beyond a repeating error log.Recommended operational follow-up: emit a distinct metric or alert for this branch so the stuck prefix is visible, and provide a documented remediation path, for example an administrative reconciliation that releases the CIDR using the stored
Prefixand the soft-deleted block record.🤖 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 `@rest-api/workflow/pkg/activity/vpcprefix/vpcprefix.go` around lines 527 - 534, Add a distinct operational signal in the default branch of the VPC Prefix deletion flow when IPBlockID is set but the relation is missing, such as a dedicated metric or alert, while preserving the existing error return that prevents CIDR leakage. Document an administrative remediation path that can release the CIDR using the stored Prefix and soft-deleted IP block record.
242-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider decomposing
createOrUpdateVpcPrefixFromSite.The function spans roughly 235 lines and mixes proto validation, CIDR canonicalization, IP Block resolution, drift checks, IPAM allocation, undelete, and creation. Three cohesive helpers would isolate the invariants and make the transaction body readable:
resolveContainingIPBlock,restoreSoftDeletedVpcPrefix, andcreateRecoveredVpcPrefix. Behavior stays identical.Note also line 472:
logger.Warn().Err(err).Msg(err.Error())records the same text twice, once as theerrorfield and once as the message. A static message such as"failed to recover VPC Prefix from Site inventory"reads better in structured logs.Also applies to: 470-476
🤖 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 `@rest-api/workflow/pkg/activity/vpcprefix/vpcprefix.go` around lines 242 - 246, Refactor createOrUpdateVpcPrefixFromSite into cohesive helpers for resolveContainingIPBlock, restoreSoftDeletedVpcPrefix, and createRecoveredVpcPrefix while preserving all existing validation, canonicalization, drift, allocation, undelete, and creation behavior. In the recovery logging near the undelete path, update logger.Warn().Err(err) to use a static descriptive message instead of duplicating err.Error().rest-api/workflow/pkg/activity/vpcprefix/vpcprefix_test.go (1)
856-880: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the precondition explicit instead of branching on prior subtest state.
This subtest reads the current row and then branches: it soft-deletes the prefix if it is active, or asserts a status if it is already deleted. The branch encodes a dependency on whether the preceding
TERMINATINGsubtest ran. Running this subtest alone with-runtakes the other branch, so the test does not verify a fixed scenario, and a regression in the preceding subtest can silently change what this one exercises.Assert the required starting state, then perform the setup unconditionally.
💚 Suggested precondition handling
- if existing[0].Deleted == nil { - _, err = vpcPrefixDAO.Update(ctx, nil, cdbm.VpcPrefixUpdateInput{ - VpcPrefixID: controllerVpcPrefixID, - Status: cutil.GetPtr(cdbm.VpcPrefixStatusDeleting), - IsMissingOnSite: cutil.GetPtr(true), - }) - require.NoError(t, err) - require.NoError(t, ipam.DeleteChildIpamEntryFromCidr( - ctx, nil, dbSession, ipamStorage, ipBlock, controllerVpcPrefix.Config.Prefix, - )) - require.NoError(t, vpcPrefixDAO.Delete(ctx, nil, controllerVpcPrefixID)) - } else { - assert.Equal(t, cdbm.VpcPrefixStatusDeleting, existing[0].Status) - } + // The preceding TERMINATING subtest leaves the row soft-deleted with Status=Deleting. + require.NotNil(t, existing[0].Deleted, "expected the prefix to be soft-deleted by the preceding subtest") + require.Equal(t, cdbm.VpcPrefixStatusDeleting, existing[0].Status)🤖 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 `@rest-api/workflow/pkg/activity/vpcprefix/vpcprefix_test.go` around lines 856 - 880, Update the “inventory restores soft-deleted VPC Prefix” subtest to require that the fetched prefix is active/not deleted with the expected initial state, rather than branching on existing[0].Deleted. After asserting that precondition, perform the status update, child IPAM deletion, and vpcPrefixDAO.Delete setup unconditionally so the test is independent of the preceding TERMINATING subtest.rest-api/db/pkg/db/ipam/ipam.go (1)
186-208: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject an equal-length
childCidrexplicitly.
CreateChildIpamEntryForIPBlocktreatschildBlockSize == parentIPBlock.PrefixLengthas a full grant and recordsFullGrant = truein the REST database. This helper has no such branch. If a caller passes achildCidrwhose length equals the parent length, the allocation happens in IPAM whileFullGrantstaysfalse. The comment block correctly states thatFullGrantexists only in the REST database, so this asymmetry is exactly the kind of divergence the guard above is meant to prevent.The current caller in
rest-api/workflow/pkg/activity/vpcprefix/vpcprefix.goline 399 branches on prefix length before calling this helper, so no live path is affected. A guard keeps the exported contract self-enforcing.♻️ Suggested guard
if parentIPBlock.FullGrant { return nil, errors.New(fmt.Sprintf("parent IPBlock : %s already has a full-grant", parentIPBlock.ID.String())) } ipamer := cipam.NewWithStorage(ipamDB) namespace := GetIpamNamespaceForIPBlock(ctx, parentIPBlock.RoutingType, parentIPBlock.InfrastructureProviderID.String(), parentIPBlock.SiteID.String()) ipamer.SetNamespace(namespace) parentCidr := GetCidrForIPBlock(ctx, parentIPBlock.Prefix, parentIPBlock.PrefixLength) + // A child equal to the parent is a full grant. That path must go through + // CreateChildIpamEntryForIPBlock so the REST DB FullGrant flag stays consistent. + if childCidr == parentCidr { + return nil, errors.New(fmt.Sprintf("childCidr: %s equals parentCidr for IPBlock: %s, use CreateChildIpamEntryForIPBlock", childCidr, parentIPBlock.ID.String())) + } childPrefix, err := ipamer.AcquireSpecificChildPrefix(ctx, parentCidr, childCidr)🤖 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 `@rest-api/db/pkg/db/ipam/ipam.go` around lines 186 - 208, Update AcquireSpecificChildIpamEntryForIPBlock to reject childCidr values whose prefix length equals parentIPBlock.PrefixLength, before invoking IPAM, using the existing full-grant error behavior or an equivalent error. Keep the existing nil-parent and already-FullGrant checks unchanged, and ensure equal-length allocations are never persisted in IPAM without the REST FullGrant state.
🤖 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 `@rest-api/workflow/pkg/activity/vpcprefix/vpcprefix.go`:
- Around line 316-326: Align the cross-site duplicate handling in the VPC prefix
lookup: use a global primary-key lookup by removing the SiteIDs filter from
vpcPrefixDAO.GetAll, then detect an existing match whose SiteID differs from
site.ID and return nil, nil before resolving IP blocks or mutating IPAM. Update
the surrounding comment if needed to accurately describe this behavior.
- Around line 390-395: Use the tenant/IPBlock advisory-lock key consistently in
both VPC-prefix deletion paths, matching the key used by REST create and
inventory recovery. In each deletion flow, acquire that shared lock before
mutating IPAM or IPBlock.FullGrant, then load or refresh IPBlock.FullGrant while
the lock is held before applying changes.
---
Nitpick comments:
In `@rest-api/db/pkg/db/ipam/ipam.go`:
- Around line 186-208: Update AcquireSpecificChildIpamEntryForIPBlock to reject
childCidr values whose prefix length equals parentIPBlock.PrefixLength, before
invoking IPAM, using the existing full-grant error behavior or an equivalent
error. Keep the existing nil-parent and already-FullGrant checks unchanged, and
ensure equal-length allocations are never persisted in IPAM without the REST
FullGrant state.
In `@rest-api/workflow/pkg/activity/vpcprefix/vpcprefix_test.go`:
- Around line 856-880: Update the “inventory restores soft-deleted VPC Prefix”
subtest to require that the fetched prefix is active/not deleted with the
expected initial state, rather than branching on existing[0].Deleted. After
asserting that precondition, perform the status update, child IPAM deletion, and
vpcPrefixDAO.Delete setup unconditionally so the test is independent of the
preceding TERMINATING subtest.
In `@rest-api/workflow/pkg/activity/vpcprefix/vpcprefix.go`:
- Around line 527-534: Add a distinct operational signal in the default branch
of the VPC Prefix deletion flow when IPBlockID is set but the relation is
missing, such as a dedicated metric or alert, while preserving the existing
error return that prevents CIDR leakage. Document an administrative remediation
path that can release the CIDR using the stored Prefix and soft-deleted IP block
record.
- Around line 242-246: Refactor createOrUpdateVpcPrefixFromSite into cohesive
helpers for resolveContainingIPBlock, restoreSoftDeletedVpcPrefix, and
createRecoveredVpcPrefix while preserving all existing validation,
canonicalization, drift, allocation, undelete, and creation behavior. In the
recovery logging near the undelete path, update logger.Warn().Err(err) to use a
static descriptive message instead of duplicating err.Error().
🪄 Autofix
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: Enterprise
Run ID: aabb8957-8a1f-4e54-97a9-e6950bf942ff
📒 Files selected for processing (5)
rest-api/db/pkg/db/ipam/ipam.gorest-api/db/pkg/db/model/vpcprefix.gorest-api/db/pkg/db/model/vpcprefix_test.gorest-api/workflow/pkg/activity/vpcprefix/vpcprefix.gorest-api/workflow/pkg/activity/vpcprefix/vpcprefix_test.go
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
|
@hwadekar-nv why this PR has milestone 2.0? |
|
@AlexGridnev Removing it and updating it to v2.2 |
3 participants
Description
Site inventory previously skipped VPC Prefixes that existed on Site but had no active REST database record, leaving Site and REST state inconsistent. This PR reconciles those VPC Prefixes using their controller ID
Soft-deleted VPC Prefixes are restored when reported by a newer inventory snapshot, while true orphans are auto-created after validating parent VPC ownership, a containing Ready tenant IPBlock, IPAM claim of the Site-reported CIDR, and name conflicts. Recovery writes are transactional and retry-safe.
Key behaviors:
Related issues
This PR is part of the issue (#3436)
Type of Change
Testing
Tests executed:
go test ./workflow/pkg/activity/vpcprefix/ -count=1
go test ./db/pkg/db/model/ -run VpcPrefix -count=1
Additional Notes