feat(api): associate VPC prefixes with its SitePrefix "parents" - #4634
Conversation
Summary by CodeRabbit
WalkthroughVPC prefixes now support optional SitePrefix lineage across storage, APIs, CLI, and REST. Creation validates attachment and concurrency rules. Startup verifies lineage. Allocation supports IPv4 ChangesVPC prefix lineage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant VpcPrefixHandler
participant SitePrefixQueries
participant VpcPrefixDatabase
Client->>VpcPrefixHandler: create VPC prefix with site_prefix_id
VpcPrefixHandler->>SitePrefixQueries: validate and lock SitePrefix
SitePrefixQueries->>VpcPrefixDatabase: persist lineage and prefix
VpcPrefixDatabase-->>VpcPrefixHandler: return created VPC prefix
VpcPrefixHandler-->>Client: return created prefix
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
🔐 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-05 22:46:52 UTC | Commit: bf79e9b |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sql (1)
12-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the
VALIDATE CONSTRAINTinto a separate migration.
sqlx::migrate!()runs each migration in one transaction. Adding the foreign key asNOT VALIDand validating it in the same transaction produces the same lock profile as adding a validated constraint directly, so the two-step form provides no benefit here. This repository already applies the split pattern:20260804153414_remove_secondary_vtep_data.sqlperforms the data change and20260804171948_validate_secondary_vtep_constraint.sqlvalidates afterwards.Two options are acceptable:
- Move line 38-39 into a follow-up migration, matching the existing precedent.
- Drop
NOT VALIDand add the constraint as validated, which makes the single-transaction lock behaviour explicit.The current table size makes either choice safe today; the concern is clarity for future readers and larger deployments.
🤖 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 `@crates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sql` around lines 12 - 39, Separate validation of network_vpc_prefixes_site_prefix_id_fkey from the migration that adds and backfills it, creating a follow-up migration that runs VALIDATE CONSTRAINT and removing the in-transaction validation from the current migration. Follow the existing split pattern used by the secondary VTEP constraint migrations.Source: Linters/SAST tools
crates/api-core/src/setup.rs (1)
327-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass the pool directly to the read-only preflight.
find_unassigned_vpc_prefix_site_prefix_idsacceptsimpl DbReader<'_>, so this read-only check does not need a transaction. The current form opens a transaction, and on theeyre::ensure!failure path it never reachestxn.rollback(), relying on drop instead. Using the pool removes both the transaction and that asymmetric cleanup path.Consider also gating on
carbide_config.listen_onlyinstead ofseed_data.is_none(), which states the intent directly.♻️ Proposed simplification
- if seed_data.is_none() && !carbide_config.site_fabric_prefixes.is_empty() { - let mut txn = Transaction::begin(&db_pool).await?; - let unassigned = - db::site_prefix::find_unassigned_vpc_prefix_site_prefix_ids(&mut txn).await?; + if carbide_config.listen_only && !carbide_config.site_fabric_prefixes.is_empty() { + let unassigned = + db::site_prefix::find_unassigned_vpc_prefix_site_prefix_ids(&db_pool).await?; eyre::ensure!( unassigned.is_empty(), "VpcPrefix SitePrefix lineage preflight failed: unassigned VpcPrefix IDs: {:?}", unassigned, ); - txn.rollback().await?; }As per coding guidelines: "For read-only database functions, accept
impl DbReaderso callers can use a pool without unnecessarily opening and committing a transaction."🤖 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 `@crates/api-core/src/setup.rs` around lines 327 - 337, Update the read-only preflight around find_unassigned_vpc_prefix_site_prefix_ids to pass db_pool directly, removing the explicit Transaction::begin, rollback, and related mutable transaction handling. Gate this check on carbide_config.listen_only instead of seed_data.is_none() to express the intended mode directly, while preserving the existing unassigned-ID validation and error behavior.Source: Coding guidelines
🤖 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 `@crates/admin-cli/src/vpc_prefix/create/args.rs`:
- Line 113: Add an optional site_prefix_id argument to the create command’s
argument definitions and pass it through instead of hardcoding site_prefix_id:
None. Document the new --site-prefix-id option in the EXAMPLES section and
regenerate the CLI reference using the project’s standard generation workflow.
---
Nitpick comments:
In `@crates/api-core/src/setup.rs`:
- Around line 327-337: Update the read-only preflight around
find_unassigned_vpc_prefix_site_prefix_ids to pass db_pool directly, removing
the explicit Transaction::begin, rollback, and related mutable transaction
handling. Gate this check on carbide_config.listen_only instead of
seed_data.is_none() to express the intended mode directly, while preserving the
existing unassigned-ID validation and error behavior.
In
`@crates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sql`:
- Around line 12-39: Separate validation of
network_vpc_prefixes_site_prefix_id_fkey from the migration that adds and
backfills it, creating a follow-up migration that runs VALIDATE CONSTRAINT and
removing the in-transaction validation from the current migration. Follow the
existing split pattern used by the secondary VTEP constraint migrations.
🪄 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: f34a69c0-f978-4353-bcd9-ba25aba0fd6f
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (20)
crates/admin-cli/src/devenv/config/apply/cmd.rscrates/admin-cli/src/vpc_prefix/create/args.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/handlers/vpc_prefix.rscrates/api-core/src/network_segment/allocate.rscrates/api-core/src/setup.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/vpc_prefix.rscrates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sqlcrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rscrates/api-db/src/site_prefix.rscrates/api-db/src/vpc_prefix.rscrates/api-model/src/vpc_prefix.rscrates/api-web/src/tests/vpc.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc_prefix.rscrates/vpc-prefix-controller/src/io.rsrest-api/proto/core/src/v1/nico_nico.proto
There was a problem hiding this comment.
🧹 Nitpick comments (7)
crates/api-db/src/vpc_prefix.rs (1)
583-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept
impl DbReaderfor this read-only lookup.
has_tenant_managed_site_prefixperforms a singleEXISTSread. The api-db convention reserves&mut PgConnectionfor writes and mutation-scoped reads, so a read-only helper should acceptimpl DbReader<'_>. The signature still accepts&mut *txnfrom the existing caller incrates/api-core/src/handlers/vpc.rs, which keeps the locked-snapshot semantics intact, while a pool-based caller becomes possible without opening a transaction.Note that
impl DbReader<'_>consumes the executor, so the caller must passtxn.as_mut()or&mut *txnrather than&mut txn.♻️ Proposed signature change
pub async fn has_tenant_managed_site_prefix( - txn: &mut PgConnection, + txn: impl DbReader<'_>, vpc_id: VpcId, ) -> Result<bool, DatabaseError> {As per coding guidelines: "For read-only database functions, accept
impl DbReaderso callers can use a pool without unnecessarily opening and committing a transaction."🤖 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 `@crates/api-db/src/vpc_prefix.rs` around lines 583 - 586, Update the signature of has_tenant_managed_site_prefix to accept impl DbReader<'_> instead of &mut PgConnection, preserving its read-only EXISTS behavior and Result<bool, DatabaseError> return type. Adjust the existing caller to pass the executor as txn.as_mut() or &mut *txn, not &mut txn, so locked-snapshot semantics remain unchanged while pool-based callers are supported.Source: Coding guidelines
crates/rpc/src/model/vpc_prefix.rs (1)
313-368: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStrong contract guard. Consider extending it to the proto mirror.
This test pins the
forgetag numbers, the retainedreserved 5range, and the legacytenant_prefix_id = 2. That is the right level of enforcement for wire compatibility.The same three tags also exist in
rest-api/proto/core/src/v1/nico_nico.protoat 11, 8, and 7. That file is not part of this crate's descriptor set, so nothing here prevents the two definitions from drifting apart. If a mirror-consistency check does not already exist elsewhere, consider adding one so a future tag change to one file cannot silently diverge from the other.🤖 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 `@crates/rpc/src/model/vpc_prefix.rs` around lines 313 - 368, Extend the contract coverage around the test site_prefix_fields_use_fresh_protobuf_tags to validate the corresponding VpcPrefix, VpcPrefixCreationRequest, and VpcPrefixSearchQuery definitions in the mirrored nico_nico.proto schema. Ensure their site_prefix_id tags remain 11, 8, and 7 respectively, and fail when the mirror diverges from the forge definitions; reuse any existing proto mirror-consistency mechanism if available.crates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sql (1)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting
VALIDATE CONSTRAINTinto a follow-up migration.This migration adds the constraint as
NOT VALIDand validates it in the same transaction. The repository already established the split pattern with20260804153414_remove_secondary_vtep_data.sqland20260804171948_validate_secondary_vtep_constraint.sql.The practical impact is limited here. The preceding
ALTER TABLE ... ADD COLUMNalready holdsACCESS EXCLUSIVEonnetwork_vpc_prefixesuntil commit, andnetwork_vpc_prefixesis a small inventory table. Treat this as consistency with the established convention rather than a lock-duration fix.🤖 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 `@crates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sql` around lines 38 - 39, Remove the VALIDATE CONSTRAINT statement from this migration’s network_vpc_prefixes changes, leaving the foreign key added as NOT VALID. Add a subsequent migration dedicated to validating network_vpc_prefixes_site_prefix_id_fkey, following the established split pattern used by the secondary VTEP migrations.Source: Linters/SAST tools
crates/api-core/src/setup.rs (1)
328-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the needless transaction around a read-only query.
db::site_prefix::find_unassigned_vpc_prefix_site_prefix_idsacceptsimpl DbReader<'_>. Pass&db_pooldirectly. The current code opens a transaction and then rolls it back, and the early return on theeyre::ensure!failure path relies onDropto release it.♻️ Proposed simplification
if seed_data.is_none() && !carbide_config.site_fabric_prefixes.is_empty() { - let mut txn = Transaction::begin(&db_pool).await?; - let unassigned = - db::site_prefix::find_unassigned_vpc_prefix_site_prefix_ids(&mut txn).await?; + let unassigned = + db::site_prefix::find_unassigned_vpc_prefix_site_prefix_ids(&db_pool).await?; eyre::ensure!( unassigned.is_empty(), "VpcPrefix SitePrefix lineage preflight failed: unassigned VpcPrefix IDs: {:?}", unassigned, ); - txn.rollback().await?; }As per coding guidelines: "For read-only database functions, accept
impl DbReaderso callers can use a pool without unnecessarily opening and committing a transaction."🤖 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 `@crates/api-core/src/setup.rs` around lines 328 - 336, Remove the unnecessary Transaction::begin and rollback flow around the read-only preflight query; call db::site_prefix::find_unassigned_vpc_prefix_site_prefix_ids directly with &db_pool, preserving the existing eyre::ensure! validation and error message.Source: Coding guidelines
crates/api-core/src/tests/vpc_prefix.rs (2)
1164-1389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this test so a failure identifies the broken property.
This single test now asserts eight independent contracts: successful attachment, get and search round-tripping, tenant-ownership rejection, omitted-ID rejection under a tenant root, three lifecycle rejections, containment rejection, non-FNN rejection,
/31allocation and exhaustion, and legacy operator-root precedence. The function spans about 225 lines and shares oneenvacross all of them.Two consequences. First, the first failing assertion hides every later contract, so one regression masks the rest. Second, a reported failure names only a line number, not the property.
Extract at least the two self-contained tails into their own tests: the
/31allocation case at lines 1322-1354 and the legacy operator-root precedence case at lines 1356-1386. Keep the existing table at lines 1253-1282 as it is; it already groups the lifecycle cases well.🤖 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 `@crates/api-core/src/tests/vpc_prefix.rs` around lines 1164 - 1389, Split the `/31` allocation and exhaustion scenario into a separate test, and move the legacy operator-root precedence scenario into another self-contained test with its own environment and fixture setup. Keep the existing lifecycle-state table in `exact_site_prefix_attachment_enforces_lineage_and_round_trips` unchanged, while preserving each extracted test’s assertions and required setup.
140-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the lock-wait match so the synchronisation stays trustworthy.
wait_until_query_is_blocked_bymatches the blocked statement withILIKE '%' || $2 || '%'. Two callers pass very broad fragments:"site_prefixes"at line 1597 and"vpcs"at line 1721. Today only the spawned request can wait on those locks, so the tests are correct. If a later change introduces any other lock wait touching those tables, the helper returns the wrong pid and the test continues before the intended statement blocks. The failure would then be a confusing assertion mismatch rather than a clear timeout.Pass the distinctive statement text instead, for example the
FOR UPDATE/FOR SHAREfragment or the advisory-lock function name that the code under test actually issues. The other two callers already do this with"site_prefixes:tenant:"and"pg_advisory_xact_lock_shared".🤖 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 `@crates/api-core/src/tests/vpc_prefix.rs` around lines 140 - 170, Narrow the query fragments passed to wait_until_query_is_blocked_by at the callers currently using broad "site_prefixes" and "vpcs" matches. Replace them with distinctive SQL text issued by the blocked statements, such as their FOR UPDATE/FOR SHARE clause or advisory-lock function name, while preserving the existing specific fragments used by the other callers.crates/vpc-prefix-controller/src/io.rs (1)
55-62: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd
list_objectscoverage for both lineage cases.site_prefix_id: Noneis intentionally unfiltered becausesearchadds that predicate only forSome(...). An integration assertion should ensure reconciliation includes prefixes with and withoutsite_prefix_id.🤖 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 `@crates/vpc-prefix-controller/src/io.rs` around lines 55 - 62, Add integration coverage for list_objects reconciliation that includes two VPC prefixes: one with a site_prefix_id and one without it. Assert both lineage cases are returned, preserving site_prefix_id: None as an intentionally unfiltered search input in the db::vpc_prefix::search call.
🤖 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.
Nitpick comments:
In `@crates/api-core/src/setup.rs`:
- Around line 328-336: Remove the unnecessary Transaction::begin and rollback
flow around the read-only preflight query; call
db::site_prefix::find_unassigned_vpc_prefix_site_prefix_ids directly with
&db_pool, preserving the existing eyre::ensure! validation and error message.
In `@crates/api-core/src/tests/vpc_prefix.rs`:
- Around line 1164-1389: Split the `/31` allocation and exhaustion scenario into
a separate test, and move the legacy operator-root precedence scenario into
another self-contained test with its own environment and fixture setup. Keep the
existing lifecycle-state table in
`exact_site_prefix_attachment_enforces_lineage_and_round_trips` unchanged, while
preserving each extracted test’s assertions and required setup.
- Around line 140-170: Narrow the query fragments passed to
wait_until_query_is_blocked_by at the callers currently using broad
"site_prefixes" and "vpcs" matches. Replace them with distinctive SQL text
issued by the blocked statements, such as their FOR UPDATE/FOR SHARE clause or
advisory-lock function name, while preserving the existing specific fragments
used by the other callers.
In
`@crates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sql`:
- Around line 38-39: Remove the VALIDATE CONSTRAINT statement from this
migration’s network_vpc_prefixes changes, leaving the foreign key added as NOT
VALID. Add a subsequent migration dedicated to validating
network_vpc_prefixes_site_prefix_id_fkey, following the established split
pattern used by the secondary VTEP migrations.
In `@crates/api-db/src/vpc_prefix.rs`:
- Around line 583-586: Update the signature of has_tenant_managed_site_prefix to
accept impl DbReader<'_> instead of &mut PgConnection, preserving its read-only
EXISTS behavior and Result<bool, DatabaseError> return type. Adjust the existing
caller to pass the executor as txn.as_mut() or &mut *txn, not &mut txn, so
locked-snapshot semantics remain unchanged while pool-based callers are
supported.
In `@crates/rpc/src/model/vpc_prefix.rs`:
- Around line 313-368: Extend the contract coverage around the test
site_prefix_fields_use_fresh_protobuf_tags to validate the corresponding
VpcPrefix, VpcPrefixCreationRequest, and VpcPrefixSearchQuery definitions in the
mirrored nico_nico.proto schema. Ensure their site_prefix_id tags remain 11, 8,
and 7 respectively, and fail when the mirror diverges from the forge
definitions; reuse any existing proto mirror-consistency mechanism if available.
In `@crates/vpc-prefix-controller/src/io.rs`:
- Around line 55-62: Add integration coverage for list_objects reconciliation
that includes two VPC prefixes: one with a site_prefix_id and one without it.
Assert both lineage cases are returned, preserving site_prefix_id: None as an
intentionally unfiltered search input in the db::vpc_prefix::search call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f43a2c95-b3db-48d0-9a46-622c6c5b0d57
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (20)
crates/admin-cli/src/devenv/config/apply/cmd.rscrates/admin-cli/src/vpc_prefix/create/args.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/handlers/vpc_prefix.rscrates/api-core/src/network_segment/allocate.rscrates/api-core/src/setup.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/vpc_prefix.rscrates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sqlcrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rscrates/api-db/src/site_prefix.rscrates/api-db/src/vpc_prefix.rscrates/api-model/src/vpc_prefix.rscrates/api-web/src/tests/vpc.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc_prefix.rscrates/vpc-prefix-controller/src/io.rsrest-api/proto/core/src/v1/nico_nico.proto
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/site_prefix.rs (1)
592-621: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a single set-based update instead of one statement per row.
The loop issues one
UPDATE ... RETURNINGround trip per resolvableVpcPrefix. On a site with many legacy rows this multiplies startup latency inside an already open transaction. A single statement withUNNEST($1::uuid[], $2::uuid[])would assign all unique matches at once and return the assigned IDs.🤖 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 `@crates/api-db/src/site_prefix.rs` around lines 592 - 621, The lineage assignment currently performs one database update per resolvable VPC prefix; replace the per-row loop and `update_query` execution with a single set-based update using `UNNEST` over paired VPC-prefix and site-prefix ID arrays, preserving the existing `site_prefix_id IS NULL` guard and collecting all returned IDs into `report.assigned_vpc_prefix_ids`. Keep missing and ambiguous candidate reporting unchanged.
🤖 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.
Nitpick comments:
In `@crates/api-db/src/site_prefix.rs`:
- Around line 592-621: The lineage assignment currently performs one database
update per resolvable VPC prefix; replace the per-row loop and `update_query`
execution with a single set-based update using `UNNEST` over paired VPC-prefix
and site-prefix ID arrays, preserving the existing `site_prefix_id IS NULL`
guard and collecting all returned IDs into `report.assigned_vpc_prefix_ids`.
Keep missing and ambiguous candidate reporting unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea8094de-2575-4748-861b-78531b452afe
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (21)
crates/admin-cli/src/devenv/config/apply/cmd.rscrates/admin-cli/src/vpc_prefix/create/args.rscrates/admin-cli/src/vpc_prefix/tests.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/handlers/vpc_prefix.rscrates/api-core/src/network_segment/allocate.rscrates/api-core/src/setup.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/vpc_prefix.rscrates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sqlcrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rscrates/api-db/src/site_prefix.rscrates/api-db/src/vpc_prefix.rscrates/api-model/src/vpc_prefix.rscrates/api-web/src/tests/vpc.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc_prefix.rscrates/vpc-prefix-controller/src/io.rsrest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (17)
- crates/vpc-prefix-controller/src/io.rs
- crates/api-web/src/tests/vpc.rs
- crates/api-core/src/handlers/vpc.rs
- crates/api-core/src/setup.rs
- crates/rpc/proto/forge.proto
- crates/api-core/src/handlers/vpc_prefix.rs
- crates/rpc/src/model/vpc_prefix.rs
- crates/api-model/src/vpc_prefix.rs
- crates/api-db/src/migrations/mod.rs
- crates/api-core/src/tests/instance.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/api-core/src/tests/instance_config_update.rs
- crates/api-db/src/vpc_prefix.rs
- crates/api-db/src/network_prefix.rs
- crates/api-core/src/tests/vpc_prefix.rs
- crates/api-core/src/network_segment/allocate.rs
- crates/admin-cli/src/devenv/config/apply/cmd.rs
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/admin-cli/src/vpc_prefix/create/args.rs (1)
52-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the extended explanation into
long_help.The
helpvalue contains three sentences. Clap rendershelpinline in the--helptable, so this argument will produce a very wide row next to the single-line help of every sibling argument. Clap renderslong_helponly for--helpon the subcommand in long form, which is the correct place for the selection rules.♻️ Proposed split
#[clap( long, name = "site-prefix-id", value_name = "SitePrefixId", - help = "The exact parent SitePrefix ID. Required for tenant-managed SitePrefixes or when multiple operator-managed SitePrefixes contain this VPC prefix. When omitted, Core selects the unique containing operator-managed SitePrefix when one exists" + help = "The exact parent SitePrefix ID", + long_help = "The exact parent SitePrefix ID. Required for tenant-managed SitePrefixes, or when multiple operator-managed SitePrefixes contain this VPC prefix. When omitted, Core selects the unique containing operator-managed SitePrefix if one exists" )] site_prefix_id: Option<SitePrefixId>,As per path instructions: "Review CLI changes for clap behavior, actionable operator-facing error messages, realistic help examples".
🤖 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 `@crates/admin-cli/src/vpc_prefix/create/args.rs` around lines 52 - 58, Update the site_prefix_id clap attribute so help contains only a concise single-line description suitable for the inline options table, and move the full parent SitePrefix selection rules into long_help. Preserve the existing argument name, type, and semantics while ensuring detailed guidance appears only in the subcommand’s long-form help.Source: Path instructions
crates/api-core/src/setup.rs (1)
300-309: 🩺 Stability & Availability | 🔵 TrivialEmit the unresolved lineage before the process aborts.
The writer path fails startup when any VPC prefix remains unresolved. The rollback discards the partial assignments, so the next boot repeats the same work. The only signal is the
eyrechain, which is easy to truncate in aggregated logs.Log the report at
errorlevel with structured fields beforeeyre::ensure!, so operators can identify the affected rows without re-running startup. Example fields:unresolved_vpc_prefix_count,missing_vpc_prefix_ids,ambiguous_vpc_prefix_count.🤖 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 `@crates/api-core/src/setup.rs` around lines 300 - 309, The site-prefix lineage preflight must emit unresolved details before aborting. In the setup block around backfill_vpc_prefix_site_prefix_lineage, when unresolved_vpc_prefix_count() is nonzero, log an error with structured fields for the unresolved count, missing_vpc_prefix_ids, and ambiguous VPC prefix count, then retain the existing eyre::ensure! failure behavior.crates/api-core/src/tests/vpc_prefix.rs (2)
1592-1602: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a more specific blocked-query fragment.
The fragment
"site_prefixes"matches any blocked statement that mentions the table. In this test the creation path issues both an advisory-lock call and a row read againstsite_prefixes. The helper can therefore return as soon as the first of them waits, which is not necessarily the lock this test intends to observe. That makes the ordering assertion sensitive to future changes in the handler.Match the exact statement the test targets, as the sibling tests already do with
"site_prefixes:tenant:"and"pg_advisory_xact_lock_shared".♻️ Suggested tightening
- wait_until_query_is_blocked_by(&env.pool, blocker_pid, "site_prefixes").await; + // The handler resolves an explicit parent with a shared row lock. + wait_until_query_is_blocked_by(&env.pool, blocker_pid, "FROM site_prefixes WHERE id =").await;🤖 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 `@crates/api-core/src/tests/vpc_prefix.rs` around lines 1592 - 1602, Update the wait_until_query_is_blocked_by call in this VPC prefix creation test to use a specific fragment identifying the intended blocked advisory-lock statement, rather than the broad "site_prefixes" table fragment. Match the existing sibling-test convention, such as the tenant-specific site-prefix fragment together with "pg_advisory_xact_lock_shared", while leaving the retirement and assertion flow unchanged.
1164-1389: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit this test into focused cases.
exact_site_prefix_attachment_enforces_lineage_and_round_tripscovers eight independent behaviors: successful attachment, persistence, search filtering, tenant ownership, omitted-ID rejection, lifecycle admission, containment, virtualization type,/31allocation, and legacy operator-root inference. A single failure reports one test name, so the failing behavior is not identifiable from the test result alone.The lifecycle loop at lines 1253-1282 already uses the table form. Extend that approach: keep one test per behavior, or move the remaining single-assertion cases into
value_scenarios!/scenarios!tables. At minimum, add a short comment above the block at lines 1284-1294, which currently relies on the reader noticing that10.46.1.0/24is outside10.40.0.0/16.As per coding guidelines: "Use table-driven tests for operations mapping inputs to outputs or errors."
🤖 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 `@crates/api-core/src/tests/vpc_prefix.rs` around lines 1164 - 1389, Split exact_site_prefix_attachment_enforces_lineage_and_round_trips into focused tests or table-driven scenarios so each attachment behavior reports independently, covering successful round-trip/search, tenant ownership, omitted IDs, lifecycle states, containment, virtualization type, /31 allocation, and legacy operator-root inference. Preserve the existing assertions and setup semantics; at minimum, add a concise comment explaining the out-of-range 10.46.1.0/24 containment case if it remains in a combined block.Source: Coding guidelines
crates/api-core/src/handlers/vpc_prefix.rs (1)
129-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the legacy resolution branch into a named function.
The
elseblock spans roughly 55 lines and nests a three-armmatchwith two lock acquisitions, a VPC lookup, and two early returns inside aletbinding.createis already a long function, and this block is the densest part of it.An extraction such as
resolve_legacy_site_prefix(txn, &new_prefix) -> CarbideResult<Option<SitePrefix>>would keepcreatelinear and give the legacy compatibility rule a name that matches the comment at Line 173. The behavior is correct as written, so this is a readability improvement only.🤖 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 `@crates/api-core/src/handlers/vpc_prefix.rs` around lines 129 - 185, Extract the entire legacy-resolution else branch from create into a named helper such as resolve_legacy_site_prefix, accepting the transaction and new prefix and returning CarbideResult<Option<SitePrefix>>. Move its locks, VPC lookup, tenant-managed containment check, candidate matching, and existing errors unchanged; have create call the helper so its control flow remains linear.
🤖 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.
Nitpick comments:
In `@crates/admin-cli/src/vpc_prefix/create/args.rs`:
- Around line 52-58: Update the site_prefix_id clap attribute so help contains
only a concise single-line description suitable for the inline options table,
and move the full parent SitePrefix selection rules into long_help. Preserve the
existing argument name, type, and semantics while ensuring detailed guidance
appears only in the subcommand’s long-form help.
In `@crates/api-core/src/handlers/vpc_prefix.rs`:
- Around line 129-185: Extract the entire legacy-resolution else branch from
create into a named helper such as resolve_legacy_site_prefix, accepting the
transaction and new prefix and returning CarbideResult<Option<SitePrefix>>. Move
its locks, VPC lookup, tenant-managed containment check, candidate matching, and
existing errors unchanged; have create call the helper so its control flow
remains linear.
In `@crates/api-core/src/setup.rs`:
- Around line 300-309: The site-prefix lineage preflight must emit unresolved
details before aborting. In the setup block around
backfill_vpc_prefix_site_prefix_lineage, when unresolved_vpc_prefix_count() is
nonzero, log an error with structured fields for the unresolved count,
missing_vpc_prefix_ids, and ambiguous VPC prefix count, then retain the existing
eyre::ensure! failure behavior.
In `@crates/api-core/src/tests/vpc_prefix.rs`:
- Around line 1592-1602: Update the wait_until_query_is_blocked_by call in this
VPC prefix creation test to use a specific fragment identifying the intended
blocked advisory-lock statement, rather than the broad "site_prefixes" table
fragment. Match the existing sibling-test convention, such as the
tenant-specific site-prefix fragment together with
"pg_advisory_xact_lock_shared", while leaving the retirement and assertion flow
unchanged.
- Around line 1164-1389: Split
exact_site_prefix_attachment_enforces_lineage_and_round_trips into focused tests
or table-driven scenarios so each attachment behavior reports independently,
covering successful round-trip/search, tenant ownership, omitted IDs, lifecycle
states, containment, virtualization type, /31 allocation, and legacy
operator-root inference. Preserve the existing assertions and setup semantics;
at minimum, add a concise comment explaining the out-of-range 10.46.1.0/24
containment case if it remains in a combined block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1afdef69-5dd7-461b-8b53-3d0b71d76776
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (21)
crates/admin-cli/src/devenv/config/apply/cmd.rscrates/admin-cli/src/vpc_prefix/create/args.rscrates/admin-cli/src/vpc_prefix/tests.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/handlers/vpc_prefix.rscrates/api-core/src/network_segment/allocate.rscrates/api-core/src/setup.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/instance_config_update.rscrates/api-core/src/tests/vpc_prefix.rscrates/api-db/migrations/20260805131227_associate_vpc_prefix_site_prefix.sqlcrates/api-db/src/migrations/mod.rscrates/api-db/src/network_prefix.rscrates/api-db/src/site_prefix.rscrates/api-db/src/vpc_prefix.rscrates/api-model/src/vpc_prefix.rscrates/api-web/src/tests/vpc.rscrates/rpc/proto/forge.protocrates/rpc/src/model/vpc_prefix.rscrates/vpc-prefix-controller/src/io.rsrest-api/proto/core/src/v1/nico_nico.proto
CIDR containment was enough to find a VpcPrefix's SitePrefix while every prefix was globally unique. Tenant-managed SitePrefixes change that: the same address space can belong to different tenants, so the exact resource ID needs to be part of the relationship. So, this adds a nullable `site_prefix_id` to VpcPrefix persistence and the gRPC surface, backfills rows only when they have one operator-managed match, and makes startup yell about missing or ambiguous relationships. New create requests lock and validate the selected SitePrefix; legacy callers that omit the ID attach automatically when there is one operator-managed match, while sites without any SitePrefix authority keep their old behavior. NetworkPrefix allocation and capacity now use the exact `vpc_prefix_id`. Unassigned NetworkPrefixes still reserve space globally until NVIDIA#3892 relaxes the global exclusion constraint, and an IPv4 `/31` VpcPrefix now represents one usable generated linknet. This supports NVIDIA#3886 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4634.docs.buildwithfern.com/infra-controller |
chet
changed the title
2 participants
Historically, SitePrefixes were only a configured containment guardrail. When Core created a
VpcPrefix, it only needed to ask whether the CIDR fit inside any configured site root. VPC prefixes were globally unique, so there was no reason to retain which root matched.Tenant-managed SitePrefixes change that. Different tenants may own separate SitePrefix resources with the same RFC1918 CIDR, and an operator-managed root may overlap a tenant-managed one. Containment can tell us that a VpcPrefix fits, but it can no longer tell us which SitePrefix authorized it.
This adds an optional
site_prefix_idrelationship to VpcPrefix persistence and gRPC. Core locks and validates the selected SitePrefix for tenant ownership, readiness, routing scope, containment, and FNN compatibility. NetworkPrefix allocation also uses the exactvpc_prefix_idinstead of rediscovering its VpcPrefix from the CIDR.The rollout remains additive:
site_prefix_idand automatically use one uniquely matching operator-managed root.site_prefix_idremains nullable in PostgreSQL and optional in protobuf for mixed-version rollouts.This establishes the
SitePrefix -> VpcPrefix -> NetworkPrefixrelationship needed by the tenant-managed workflow. It does not relax overlap, change peering or routing policy, or complete tenant-managed SitePrefix enablement by itself.It also makes an IPv4
/31VpcPrefix represent one usable generated linknet, without allowing directly created/31NetworkSegments.Related issues
This supports #3886.
It is part of #3883.
Type of Change
Breaking Changes
Testing
Tested with:
cargo test -p carbide-api-db -p carbide-api-core --lib-- 1,599 runnable Core tests passed with 4 ignored, and all 357 database tests passed.cargo make check-format-nightlycargo make clippycargo make carbide-lintscargo make --no-workspace generate-rest-core-protogo test ./proto/core/gen/v1fromrest-api/bash scripts/check-migration-filenames.sh --base origin/main