[wip] OSAC-1610: NetBox Inventory Backend Design - #289
mennyaboush wants to merge 5 commits into
Conversation
|
@mennyaboush: This pull request references OSAC-1610 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
AI EP Review: EP-289Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with clear user-facing need, concrete justification, focused scope, and fully testable requirements; held back from a perfect score only by design leakage in the Assumptions section and one In Scope item that prescribes implementation mechanics. Feedback: Move the field-level specifics from Assumptions (osac_instance_id, osac_managed, status=staged/active) into the design document — the PRD should say the admin configures pool membership and OSAC tracks allocation state, without prescribing field names or status values. Rewrite the In Scope credential bullet to describe what the admin does ('provides an API token through deployment configuration') rather than how it's stored ('rendered into a Helm-created Secret'). These changes would bring User-Facing Focus to a 2. Critical (0)None. Important (2)
Suggestions (1)
Structural notes (0)None. Review costModel: claude-opus-4-6 |
AI Design Review: EP-289Score: 8/8 | Verdict: PASS
Verdict: A comprehensive, well-structured design that thoroughly addresses architecture, implementation, scope, and testing with exceptional depth across all four criteria. Feedback: This is a strong design ready for implementation. Minor suggestions: the Observability section could mention whether the two new metrics follow the existing BMF Prometheus naming conventions and whether dashboards/alerts need updating. The Version Skew Strategy could be more specific about minimum supported OSAC-5618 API version requirements. Consider adding a brief note on operational runbook integration for the quarantine workflow (admin restoring staged status after Secret repair). Critical (0)None. Important (0)None. Suggestions (3)
Structural notes (0)None. Review costModel: claude-opus-4-6 |
Test Plan Review: TP-289Score: 6/10 | Verdict: Revise
Verdict: Well-structured test plan with strong specificity but undermined by field name and status value mismatches against the design, missing BMC lifecycle test coverage, and lack of test infrastructure grounding. Feedback: Align the custom field name to 'osac_instance_id' per the design (currently 'osac_assignment_id' throughout) and fix device status from 'active' to 'staged' to match the design's FindFreeHost query. Add unit tests for the BMC credential extraction, BMC Secret creation, and BMH creation steps in AssignHost — these are core to the design's workflow but completely absent from the test plan. Ground the plan in specific test files, fixtures, and helper patterns from bare-metal-fulfillment-operator (e.g., reference existing BCM test files by path, name the mock server setup helpers, point to test patterns to follow). Critical (2)
Important (4)
Suggestions (4)
Review costModel: claude-opus-4-6 |
| - On success: record `ExternalHostID` in BareMetalInstance status; proceed to power/provisioning | ||
|
|
||
| 3. **Host Provisioning** (existing Metal3/BMH workflow): | ||
| - Create or update Metal3 BareMetalHost for power management |
There was a problem hiding this comment.
we need to extract credentials from netbox and create the bmc secret as well ?
There was a problem hiding this comment.
Yes. I think that in any inventory case that will involved metal3 as management this will be the flow
| - Rationale: custom field queries via API are cumbersome; client-side filtering is simpler and more flexible | ||
| - Pagination: handle large device counts via limit/offset (e.g., fetch 100 at a time) | ||
|
|
||
| ### Idempotency and Crash Recovery |
There was a problem hiding this comment.
I think this section is redundant with "Operator reconciliation" in "Tenant User: Request and Release Bare-Metal Host" section
|
|
||
| ## Security Considerations | ||
|
|
||
| ### Credential Handling |
There was a problem hiding this comment.
this section is almost the same as "TLS and Credential Security" section
|
|
||
| No changes to RBAC or tenancy model. Existing BareMetalInstance RBAC applies unchanged. | ||
|
|
||
| NetBox backend records assignment identifier (BareMetalInstance UID) in NetBox, not tenant name. Tenant isolation is enforced at the BareMetalInstance API level (fulfillment-service OPA policies); NetBox stores no tenant-identifying data. |
There was a problem hiding this comment.
same info as in "Tenant Isolation" section
d0f7914 to
f359bab
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds requirements, design documentation, deployment decisions, lifecycle behavior, and a 53-scenario test plan for an in-tree NetBox inventory backend. ChangesNetBox inventory backend
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested labels: Merge Risk: 🟡 Moderate · up to The approved design could permit duplicate or premature host reuse and defines tests that cannot reliably validate key behavior. These issues should be resolved before merge. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (9 passed)
Full details: No-Hardcoded-SecretsExplanation The pull request adds concrete credential-shaped token values to Full details: No-Sensitive-Data-In-LogsExplanation The new design explicitly proposes logs that can expose sensitive data. Resolution Remove raw tenant label selectors and configured endpoint URLs/hostnames from logs. Log only safe counts and generic error categories. Do not log raw resource or assignment identifiers unless the implementation guarantees that they are non-sensitive and appropriately redacted. Update the structured-log examples and tests to assert that tenant-provided values, internal hostnames, credentials, and identifiers are absent. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
|
||
| ## Open Questions | ||
|
|
||
| ### NetBox Version Compatibility — CRITICAL: Minor Versions Have Breaking Changes |
There was a problem hiding this comment.
does the api as a endpint to extract netbox version? It can be queried and enable different code paths in the inventory backend
Test Plan Review: TP-289Score: 7/10 | Verdict: Revise
Verdict: Solid test plan with specific scenarios and clear structure, but count mismatches, missing BMC credential lifecycle tests, and unaddressed NetBox version compatibility testing prevent a Ready verdict. Feedback: Fix the coverage summary table to match the 48 actually documented tests — the 5 missing tests (AssignHost concurrent scenarios, UnassignHost edge case, stress variants) should either be documented or the table corrected. Add unit tests for BMC credential extraction from NetBox custom fields and BMC Secret create/delete lifecycle, which the design describes as part of AssignHost/UnassignHost but the test plan omits. Ground the plan in existing test patterns: reference specific files in bare-metal-fulfillment-operator (e.g., the BCM client tests) as implementation templates, and name concrete fixtures or helpers the new tests should reuse. Critical (2)
Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
f359bab to
5394734
Compare
| - If unassigned → proceed to step 2 | ||
| 2. Extract BMC credentials: read `bmc_username`, `bmc_password`, `bmc_address` from device custom fields | ||
| 3. PATCH assignment: update NetBox device with `osac_instance_id = bareMetalInstanceID`. **Include `If-Match: <etag>` header from step 1.** If NetBox returns **412 Precondition Failed** → another process modified the device since our read; return (nil, nil) — treat as race loss. | ||
| 4. Verify write (sanity check): read device back; confirm `osac_instance_id` matches. This is now a safety net rather than the primary race detection mechanism — the ETag in step 3 prevents concurrent overwrites. |
There was a problem hiding this comment.
do we really to do it then?
There was a problem hiding this comment.
no. you right if we used the IFf-Match the get after write seems redundant. will fix it
Align the NetBox selector contract, Helm Secret lifecycle, allocation state, and test coverage with the reviewed design. Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb> Assisted-by: Claude Haiku 4.5 <noreply@anthropic.com> Assisted-by: Codex <noreply@openai.com>
251331b to
59b724b
Compare
|
|
||
| ### Input Validation | ||
|
|
||
| Label selectors from BareMetalInstance API are user-provided key=value pairs. These are converted to tag slugs and included in NetBox API query URLs. Input validation requirements: |
There was a problem hiding this comment.
BareMetalInstanceTypes where label selectors lives are created by the cloud admin who is owner on the system, not by tenant users/admins.
The user-facing API is not aware of the inventory system.
There was a problem hiding this comment.
you right. the tenant do not supposed to be aware to the inventory and current design
|
please avoid to rebase the history, it makes harder to review between changes |
Remove the redundant post-write verification step from host allocation flows and clarify the admin-managed selector source. Assisted-by: Codex <noreply@openai.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
|
@jkilzi: changing LGTM is restricted to collaborators DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jkilzi, mennyaboush The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| `device_type` filter, and it does not extract selectors from the instance | ||
| type's hardware fields. Any hardware distinction needed for placement must | ||
| be represented by a tag key in the resolved HostSelector. | ||
| 2. `FindFreeHost` returns an inventory host ID in the existing `<namespace>/<name>` form expected by the controller and Metal3 management client: `<metal3 namespace>/netbox-device-<NetBox device ID>`. The reconciler persists that ID in `ExternalHostID` before calling `AssignHost`, as it does for the other inventory backends. This persisted ID is the recovery pointer: when it is already set, the controller skips `FindFreeHost` and lets `AssignHost` reconcile the NetBox state. |
There was a problem hiding this comment.
Why add metal3 namespace to the external host id here?
There was a problem hiding this comment.
maybe it should be discribe at the design more clearly but metal3 identify the BMH with namespace/name while netbox do not use the namespace so we need to make the same adaptation we did at bcm and save the hostId with the namespace for the BMH to continens the correct values at the management client
There was a problem hiding this comment.
Okay, maybe this is already handled some specific way in BCM that I need to review, but I figured the metal3 namespace for this was fixed at the config level so there was no reason to store it anywhere else.
| #### Selector data flow and lifetime | ||
|
|
||
| `hostSelector` is a persistent field in the BareMetalInstance **spec**, not a | ||
| temporary argument created by the NetBox adapter. Before the CR is created, | ||
| fulfillment-service copies the referenced instance type's | ||
| `host_label_selector.match_labels` into `spec.selector.hostSelector`. The CRD | ||
| requires at least one entry and makes the selector immutable, so the map stays | ||
| with that BareMetalInstance for its lifetime, including allocation, provisioning, | ||
| deallocation, and controller restarts. The controller uses it for | ||
| `FindFreeHost` while no `ExternalHostID` is recorded; after a candidate ID is | ||
| persisted, retries use that ID with `AssignHost` and do not select by tags again. | ||
| The NetBox client does not fetch or reconstruct the instance type at allocation | ||
| time. |
There was a problem hiding this comment.
Am I missing something or is none of this new? This is how it works today and will be unchanged by this proposal, why mention it?
|
|
||
| **Credential Management:** | ||
| - API token stored in the Helm-created Kubernetes Secret `osac-netbox-api-token`, key `token` | ||
| - Secret is mounted read-only at `/etc/osac/secrets/<tokenSecret>/`; the client reads `/etc/osac/secrets/<tokenSecret>/token` |
There was a problem hiding this comment.
Where does this happen? Earlier in the doc there is a struct like this:
type NetBoxOptions struct {
Endpoint string `json:"endpoint"` // https://netbox.example.com
TokenSecret string `json:"tokenSecret"` // Kubernetes Secret name/key token
CACertSecret string `json:"caCertSecret"` // Optional Secret/key ca.crt
}I think we can do it either by mounting or with the secret ref in the struct, but not both.
|
|
||
| **TLS Configuration:** | ||
| - System CA bundle used by default | ||
| - Optional custom CA cert provided in the Helm-created `osac-netbox-ca` Secret, key `ca.crt`, and mounted read-only at `/etc/osac/secrets/<caCertSecret>/ca.crt` alongside the token |
There was a problem hiding this comment.
Same comment here. Is it mounted or is the secret being read based on the struct info?
| management configuration. The existing chart uses `metal3.enabled` for both | ||
| Metal3 inventory and management templates, so the implementation must avoid | ||
| rendering two objects with the same `secrets.inventoryConfig` name: when both | ||
| `netbox.enabled` and `metal3.enabled` are true, the NetBox template owns |
There was a problem hiding this comment.
I would expect them to get wrapped together. I don't understand why someone would enable both here.
There was a problem hiding this comment.
There no need and supposed to failed in case we enabled 2 inventory at the same time.
this section maybe confusing but I see that as both enabled 1 for the inventory and 1 management as it should be.
What is mean to expect them get wrapped together?
There was a problem hiding this comment.
Like I wouldn't expect to see netbox.enabled and metal3.enabled with sections for both. I expected to see the config section as you wrote it with just netbox.enabled = true
Specifically the line that's confusing is "when both netbox.enabled and metal3.enabled are true" - I would expect this never to happen and have no reason to happen because the netbox section already has everything we need for metal3 management.
mennyaboush
changed the title
Use provider-created custom fields, preserve device names with durable numeric identity, clarify mounted credentials and Metal3 wiring, and specify restart-safe allocation cleanup with aligned coverage. Assisted-by: Codex <noreply@openai.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
Assisted-by: Codex <noreply@openai.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
55a7ad3 to
10df395
Compare
Align the NetBox test plan with system-scoped, device-label BMC Secret resolution and credential failure handling. Assisted-by: Codex <noreply@openai.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
| configuration. | ||
| - NetBox device names are optional metadata. The stable OSAC identity is | ||
| derived from the numeric device ID as netbox-<id>; endpoint or Metal3 | ||
| namespace changes require draining affected instances. |
There was a problem hiding this comment.
metal3 namespace change is not expected. What means "endpoint change"?
| Starting state: the provider has a reachable NetBox deployment and a configured | ||
| OSAC Secret API. | ||
|
|
||
| 1. The administrator creates the required NetBox device and scalar capability |
| address and boot MAC to each corresponding NetBox device. Raw credentials | ||
| and Secret IDs are never entered in NetBox. |
There was a problem hiding this comment.
| address and boot MAC to each corresponding NetBox device. Raw credentials | |
| and Secret IDs are never entered in NetBox. | |
| address and boot MAC to each corresponding NetBox device. |
| address and boot MAC to each corresponding NetBox device. Raw credentials | ||
| and Secret IDs are never entered in NetBox. | ||
| 3. The administrator configures the NetBox endpoint, API token, CA material, | ||
| Secret API endpoint, and controller credentials in the deployment values. |
There was a problem hiding this comment.
the secret API endpoint might be configured at install time, we should know the URL of the fulfillment-service at install time.
adriengentil
left a comment
adriengentil
left a comment
There was a problem hiding this comment.
few comments, but I think it's going to the right direction
| address and boot MAC to each corresponding NetBox device. Raw credentials | ||
| and Secret IDs are never entered in NetBox. | ||
| 3. The administrator configures the NetBox endpoint, API token, CA material, | ||
| Secret API endpoint, and controller credentials in the deployment values. |
There was a problem hiding this comment.
| Secret API endpoint, and controller credentials in the deployment values. | |
| Secret API endpoint, and controller credentials in the OSAC deployment values. |
| 3. The administrator configures the NetBox endpoint, API token, CA material, | ||
| Secret API endpoint, and controller credentials in the deployment values. | ||
| 4. The operator validates connectivity and fixed NetBox fields when it starts. | ||
| Capability fields are validated when they are used by a selector. |
There was a problem hiding this comment.
not sure I understand this sentence
| | osac_managed=true | Provider | Enrolls a device in the OSAC pool | | ||
| | status=staged/active/failed | OSAC | Available, claimed, or quarantined state | | ||
| | osac_instance_id | OSAC | BMI UID owning an active claim | | ||
| | osac_bmc_address | Provider | Metal3-compatible BMC address | |
There was a problem hiding this comment.
we might need to add the equivalent of "osac_interface_macs" https://redhat.atlassian.net/browse/OSAC-5810?focusedCommentId=18731725
| created for that allocation, waits for both resources to disappear, and then | ||
| conditionally restores status staged and clears the owner. The source OSAC | ||
| Secret is not deleted. | ||
|
|
There was a problem hiding this comment.
we also need to describe GetHostNICs, it should behave like BCM integration.
| The BMF uses an authenticated private Secret API client. It does not connect | ||
| directly to Vault, Thales KMS, or another provider backend. OSAC-5618 provides | ||
| the system-scoped Secret authorization; tenant users cannot retrieve the source | ||
| Secret data. |
There was a problem hiding this comment.
| The BMF uses an authenticated private Secret API client. It does not connect | |
| directly to Vault, Thales KMS, or another provider backend. OSAC-5618 provides | |
| the system-scoped Secret authorization; tenant users cannot retrieve the source | |
| Secret data. | |
| The BMF uses the authenticated private OSAC Secret API client. |
that should be enough
| Secret deletion and automatic rotation are not part of this enhancement; | ||
| providers must coordinate rotation by their normal secret lifecycle and, | ||
| where needed, drain and reassign hosts. | ||
|
|
There was a problem hiding this comment.
can we state what is the data expected in the secret (json, yaml) and what are the fields ?
5 participants
Summary
Design document for NetBox as a pluggable inventory backend for OSAC bare-metal fulfillment.
Design Highlights
osac_instance_idwith read-after-write verificationtag=managed_by:osac) + staged statusDocuments
Related
🤖 Generated with Claude Code
Summary
NetBoxConfig,NetBoxClient, and Metal3/BareMetalHost lifecycle workflows.5.1.0.Compatibility
This PR adds documentation only. It does not change executable code, runtime configuration, public APIs, stored data, or controller behavior.
The proposed backend would use NetBox for inventory and allocation tracking. Metal3/BareMetalHost would continue to manage provisioning and power operations. NetBox version compatibility remains unresolved because the design identifies minor-version breaking changes and does not select a supported version range.
Risk classification
risk:ship — The PR changes documentation and test plans only. It does not change production behavior, deployment configuration, public interfaces, or stored data.
The PR is close to risk:show because it specifies a new backend, allocation state, Metal3 integration, authentication, and deployment configuration. It does not qualify because the implementation is not included.