feat(rest-api): add iPXE template + templated-OS data model & proto - #3160
Conversation
PR 1/3 (foundations). Adds DB models and site-agent inventory proto for iPXE templates and templated iPXE Operating Systems: - IpxeTemplate + IpxeTemplateSiteAssociation models (+ tests) - OperatingSystem iPXE/templated fields and scope handling (+ tests) - OperatingSystemSiteAssociation updates - StatusDetailDAO.CreateFromParams helper - Migration 20260623150000_ipxe_os_and_templates - site-agent workflows v1 inventory proto: OperatingSystem/template types
|
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 (4)
Summary by CodeRabbit
WalkthroughAdds iPXE template persistence, operating-system templating support, controller-state tracking for site associations, a schema migration, and new inventory protobuf messages. ChangesiPXE Template Support
Estimated code review effort: 4 (Complex) | ~60 minutes Related Issues: None Related PRs: None Suggested labels: database, migration, feature Suggested reviewers: Reviewers familiar with the Bun DAO layer and iPXE/OperatingSystem persistence flow 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
🔐 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-07-06 22:23:57 UTC | Commit: 011d2a3 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
rest-api/db/pkg/db/model/ipxetemplatesiteassociation.go (1)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComposite uniqueness constraint lives only in the migration, not the model.
The
(ipxe_template_id, site_id)uniqueness invariant is applied entirely via raw SQL in the migration (and duplicated again in the test setup), but nothing in the model itself documents or enforces it — unlike the FK constraints, which are colocated inBeforeCreateTable. Anyone usingResetModel(as the tests do) outside of this specific test helper would miss the constraint entirely. Consider adding theUNIQUEconstraint insideBeforeCreateTablealongside theForeignKeycalls so the invariant travels with the model.Also applies to: 79-86
🤖 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/model/ipxetemplatesiteassociation.go` around lines 59 - 64, The composite uniqueness for the ipxe_template/site association is currently only enforced in migration SQL and test setup, so it is easy to miss when the model is reset elsewhere. Update IpxeTemplateSiteAssociation’s BeforeCreateTable method to declare the (ipxe_template_id, site_id) UNIQUE constraint alongside the existing ForeignKey calls, so the invariant is defined with the model rather than only in migrations.rest-api/db/pkg/migrations/20260623150000_ipxe_os_and_templates.go (1)
40-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant index on
ipxe_template.name.
IpxeTemplate.Nameis taggeduniquein the Bun model (ipxetemplate.goLine 51), sotx.NewCreateTable()already produces aUNIQUEconstraint onname, which Postgres backs with an implicit unique b-tree index (confirmed by the test files re-creatingipxe_template_name_key, the default Postgres constraint name for that exact column). The subsequentCREATE INDEX ipxe_template_name_idx ON ipxe_template(name)therefore duplicates that index, adding unnecessary write overhead and storage with no query benefit.♻️ Proposed fix
- _, err = tx.Exec("CREATE INDEX IF NOT EXISTS ipxe_template_name_idx ON ipxe_template(name)") - handleError(tx, err) _, err = tx.Exec("CREATE INDEX IF NOT EXISTS ipxe_template_scope_idx ON ipxe_template(scope)")As per path instructions, "Review database changes for Bun/pgx query correctness, transaction boundaries, migration ordering, data-model compatibility."
🤖 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/migrations/20260623150000_ipxe_os_and_templates.go` around lines 40 - 50, Remove the redundant name index from the ipxe template migration: the IpxeTemplate model already makes name unique, so tx.NewCreateTable() will create the implicit unique index for it. In the migration logic around the IpxeTemplate table creation, keep only the non-duplicate indexes (scope, created, updated) and drop the extra CREATE INDEX on name to avoid unnecessary storage and write overhead.Source: Path instructions
rest-api/db/pkg/db/model/ipxetemplatesiteassociation_test.go (1)
17-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated schema-bootstrap SQL across test files.
The
ipxe_template_name_keydrop/add block (Lines 25-28) exactly duplicates the identical statements inipxetemplate_test.go'stestIpxeTemplateSetupSchema. Consider extracting a shared helper (e.g.testIpxeTemplateAddUniqueConstraint) to avoid the two copies drifting apart if the constraint definition ever changes.🤖 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/model/ipxetemplatesiteassociation_test.go` around lines 17 - 33, The schema bootstrap in testIpxeTemplateSiteAssociationSetupSchema duplicates the ipxe_template unique-constraint drop/add logic already used in testIpxeTemplateSetupSchema; extract that repeated ALTER TABLE sequence into a shared helper with a clear name such as a constraint bootstrap helper, then call it from both test setup functions so the ipxe_template_name_key definition stays consistent in one place.rest-api/db/pkg/db/model/operatingsystem.go (2)
129-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
IsIPXETypeinstead of re-listing the iPXE type set inline.
GetAll's scope filter hardcodes[]string{OperatingSystemTypeIPXE, OperatingSystemTypeTemplatedIPXE}, duplicating the exact set already encapsulated by the newly-addedIsIPXEType. If a third iPXE variant is ever introduced, only one of the two call sites is likely to be updated, silently breaking scope filtering for the new type.♻️ Proposed fix
+var operatingSystemIPXETypes = []string{OperatingSystemTypeIPXE, OperatingSystemTypeTemplatedIPXE} + // IsIPXEType returns true if the given OS type is any iPXE variant (raw script or templated). func IsIPXEType(osType string) bool { - return osType == OperatingSystemTypeIPXE || osType == OperatingSystemTypeTemplatedIPXE + return slices.Contains(operatingSystemIPXETypes, osType) }query = query.Where( "os.type IN (?) AND COALESCE(os.ipxe_os_scope, ?) IN (?)", - bun.In([]string{OperatingSystemTypeIPXE, OperatingSystemTypeTemplatedIPXE}), + bun.In(operatingSystemIPXETypes), OperatingSystemScopeLocal, bun.In(filter.Scopes), )Also applies to: 629-639
🤖 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/model/operatingsystem.go` around lines 129 - 132, The scope filter in GetAll is duplicating the iPXE type list inline instead of reusing IsIPXEType, which risks drift if more iPXE variants are added. Update the filtering logic in GetAll to use IsIPXEType as the single source of truth for identifying iPXE OS types, and remove the hardcoded []string{OperatingSystemTypeIPXE, OperatingSystemTypeTemplatedIPXE} set so both call sites stay consistent.
62-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a named
CacheStrategytype instead of raw strings + package-level maps.
OperatingSystemIpxeArtifact.CacheStrategyis a barestring, with proto round-tripping handled by two package-level maps invoked inline insideFromProto/ToProto. The repo's established pattern for exactly this situation—a string field that round-trips with a proto enum—is a dedicated named type owning its ownToProto/FromProtomethods.♻️ Proposed refactor sketch
-type OperatingSystemIpxeArtifact struct { - Name string `json:"name"` - URL string `json:"url"` - SHA *string `json:"sha"` - AuthType *string `json:"authType"` - AuthToken *string `json:"authToken"` - CacheStrategy string `json:"cacheStrategy"` -} +type OperatingSystemIpxeArtifactCacheStrategy string + +func (s OperatingSystemIpxeArtifactCacheStrategy) ToProto() cwssaws.IpxeTemplateArtifactCacheStrategy { + return OperatingSystemIpxeArtifactCacheStrategyToProtoMap[string(s)] +} + +func (s *OperatingSystemIpxeArtifactCacheStrategy) FromProto(p cwssaws.IpxeTemplateArtifactCacheStrategy) { + v := OperatingSystemIpxeArtifactCacheStrategyFromProtoMap[p] + if v == "" { + v = OperatingSystemIpxeArtifactCacheStrategyCacheAsNeeded + } + *s = OperatingSystemIpxeArtifactCacheStrategy(v) +} + +type OperatingSystemIpxeArtifact struct { + Name string `json:"name"` + URL string `json:"url"` + SHA *string `json:"sha"` + AuthType *string `json:"authType"` + AuthToken *string `json:"authToken"` + CacheStrategy OperatingSystemIpxeArtifactCacheStrategy `json:"cacheStrategy"` +}As per path instructions,
db/pkg/db/model.MachineCapabilityType"type X stringwith(X).ToProto() cwssaws.X... are the reference for typed-string domain enums that round-trip with proto enum values."Also applies to: 159-209
🤖 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/model/operatingsystem.go` around lines 62 - 69, Refactor OperatingSystemIpxeArtifact.CacheStrategy away from a bare string and the package-level proto maps into a dedicated named CacheStrategy type that owns its own ToProto and FromProto methods, matching the existing MachineCapabilityType pattern. Update the enum constants in operatingSystem.go to be typed CacheStrategy values, and move the proto round-trip logic currently embedded in OperatingSystemIpxeArtifact.FromProto and ToProto onto the CacheStrategy type so conversion stays localized and type-safe.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 `@rest-api/db/pkg/db/model/ipxetemplate.go`:
- Around line 198-217: GetAll handles an explicit empty IDs filter by returning
no rows, but it does not do the same for Names. Update IpxeTemplateSQLDAO.GetAll
to mirror the IDs check for filter.Names so a non-nil empty Names slice returns
an empty result set before building the query. Make sure the behavior stays
consistent with setQueryWithFilter and the existing early-return logic in
GetAll.
In `@rest-api/db/pkg/db/model/ipxetemplatesiteassociation.go`:
- Around line 192-234: In IpxeTemplateSiteAssociationSQLDAO.GetAll, treat
explicit empty association filters as a no-match instead of ignoring them. Add
the same guard used by IpxeTemplateSQLDAO.GetAll so that when
filter.IpxeTemplateIDs or filter.SiteIDs is non-nil but has length 0, the method
short-circuits and returns no rows before building the query. Keep the existing
behavior for nil filters, and apply the check alongside the current query setup
in IpxeTemplateSiteAssociationSQLDAO.GetAll.
---
Nitpick comments:
In `@rest-api/db/pkg/db/model/ipxetemplatesiteassociation_test.go`:
- Around line 17-33: The schema bootstrap in
testIpxeTemplateSiteAssociationSetupSchema duplicates the ipxe_template
unique-constraint drop/add logic already used in testIpxeTemplateSetupSchema;
extract that repeated ALTER TABLE sequence into a shared helper with a clear
name such as a constraint bootstrap helper, then call it from both test setup
functions so the ipxe_template_name_key definition stays consistent in one
place.
In `@rest-api/db/pkg/db/model/ipxetemplatesiteassociation.go`:
- Around line 59-64: The composite uniqueness for the ipxe_template/site
association is currently only enforced in migration SQL and test setup, so it is
easy to miss when the model is reset elsewhere. Update
IpxeTemplateSiteAssociation’s BeforeCreateTable method to declare the
(ipxe_template_id, site_id) UNIQUE constraint alongside the existing ForeignKey
calls, so the invariant is defined with the model rather than only in
migrations.
In `@rest-api/db/pkg/db/model/operatingsystem.go`:
- Around line 129-132: The scope filter in GetAll is duplicating the iPXE type
list inline instead of reusing IsIPXEType, which risks drift if more iPXE
variants are added. Update the filtering logic in GetAll to use IsIPXEType as
the single source of truth for identifying iPXE OS types, and remove the
hardcoded []string{OperatingSystemTypeIPXE, OperatingSystemTypeTemplatedIPXE}
set so both call sites stay consistent.
- Around line 62-69: Refactor OperatingSystemIpxeArtifact.CacheStrategy away
from a bare string and the package-level proto maps into a dedicated named
CacheStrategy type that owns its own ToProto and FromProto methods, matching the
existing MachineCapabilityType pattern. Update the enum constants in
operatingSystem.go to be typed CacheStrategy values, and move the proto
round-trip logic currently embedded in OperatingSystemIpxeArtifact.FromProto and
ToProto onto the CacheStrategy type so conversion stays localized and type-safe.
In `@rest-api/db/pkg/migrations/20260623150000_ipxe_os_and_templates.go`:
- Around line 40-50: Remove the redundant name index from the ipxe template
migration: the IpxeTemplate model already makes name unique, so
tx.NewCreateTable() will create the implicit unique index for it. In the
migration logic around the IpxeTemplate table creation, keep only the
non-duplicate indexes (scope, created, updated) and drop the extra CREATE INDEX
on name to avoid unnecessary storage and write overhead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4daaa15-1006-44d3-9e1f-dcc689f770d6
⛔ Files ignored due to path filters (1)
rest-api/workflow-schema/schema/site-agent/workflows/v1/inventory.pb.gois excluded by!**/*.pb.go,!rest-api/**/*.pb.go
📒 Files selected for processing (9)
rest-api/db/pkg/db/model/ipxetemplate.gorest-api/db/pkg/db/model/ipxetemplate_test.gorest-api/db/pkg/db/model/ipxetemplatesiteassociation.gorest-api/db/pkg/db/model/ipxetemplatesiteassociation_test.gorest-api/db/pkg/db/model/operatingsystem.gorest-api/db/pkg/db/model/operatingsystem_ipxe_test.gorest-api/db/pkg/db/model/operatingsystemsiteassociation.gorest-api/db/pkg/migrations/20260623150000_ipxe_os_and_templates.gorest-api/workflow-schema/site-agent/workflows/v1/inventory.proto
| Deleted *time.Time `bun:"deleted,soft_delete"` | ||
| CreatedBy uuid.UUID `bun:"created_by,type:uuid,notnull"` | ||
| // ControllerState mirrors the tenant state reported by nico-core for this OS at this site. | ||
| ControllerState *string `bun:"controller_state"` |
There was a problem hiding this comment.
To check whether Status would be sufficient.
There was a problem hiding this comment.
OS like 'machine' has bi-directional updates: the source of truth changes based on the direction. Going single state would not allow to detect drift/desync and intent vs reported state if we have a need for it.
Address review feedback on the iPXE template DAOs: - GetAll now treats a non-nil but empty filter slice as a no-match and short-circuits before building the query, for both IpxeTemplate (Names) and IpxeTemplateSiteAssociation (IpxeTemplateIDs / SiteIDs), matching the existing IDs guard. - Rename IpxeTemplateFilterInput.IDs to IpxeTemplateIDs for consistency with the other filter structs. Adds table cases covering the empty-slice early returns.
…ies (#3232) Adds the synchronization layer for the Templated iPXE Operating System feature: the cloud-side inbound reconcile plus the site-agent / site-workflow inventory collectors that keep Operating System definitions and iPXE templates consistent between on-site NICo Core and NICo REST. Additive workflow/collector wiring only; no schema or API changes. Site-agent collectors discover Operating Systems and iPXE templates on-site and publish them as inventory; cloud inventory workflows reconcile that inventory into the DB. ## Related issues - follow-up to #3160 ## Type of Change - [x] **Add** - New feature or capability ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated Signed-off-by: Patrice Breton <pbreton@nvidia.com>
Summary
First PR in a clean, reviewable stack that ports the Templated iPXE Operating System feature onto REST API to match 'core' implementation. This PR lands only the data-model + proto foundation; the sync workflow and the API/SDK/OpenAPI layers follow in subsequent stacked PRs (see Stack below). It introduces an iPXE template–based OS variant (alongside Image and raw iPXE) with a per-OS scope (Local / Global / Limited) and the schema that lets OS definitions stay in sync with on-site NICo Core.
This PR is data-layer only: no handler, workflow, or route wiring is activated yet, so it is additive and inert on its own.
Stack
What's implemented
operating_systemscope + iPXE template columns; newipxe_templateandipxe_template_site_associationtables;operating_system_site_association.controller_state; additive migration (20260623150000_ipxe_os_and_templates).ipxetemplateandipxetemplatesiteassociationDAOs, extendedoperatingsystemmodel (scope + templated-iPXE type), and proto conversions.OperatingSystemInventory+IpxeTemplateInventoryadded toinventory.proto(regenerated; onlyinventory.pb.gochanged) so the downstream inbound-inventory PR has the types it needs.ipxetemplate,ipxetemplatesiteassociation, and the templated/scope additions tooperatingsystem(table-driven).Verification
cd rest-api && make testNotes for review
controller_stateonoperating_system_site_associationis introduced here but is exercised by the later workflow/API PRs.inventory.pb.gochanges in the generated proto; no other generated files are touched in this PR.Type of Change
feat:)Services Affected
Breaking Changes
None expected — additive schema + new proto messages only.