Skip to content

OSAC-1344: implement private gRPC servers for bare metal resources#683

Merged
openshift-merge-bot[bot] merged 13 commits into
osac-project:mainfrom
adriengentil:feat/OSAC-1343
Jun 15, 2026
Merged

OSAC-1344: implement private gRPC servers for bare metal resources#683
openshift-merge-bot[bot] merged 13 commits into
osac-project:mainfrom
adriengentil:feat/OSAC-1343

Conversation

@adriengentil

@adriengentil adriengentil commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add three private gRPC servers (PrivateBareMetalInstancesServer, PrivateBareMetalInstanceTemplatesServer, PrivateBareMetalInstanceCatalogItemsServer) backed by GenericDAO
  • SSH public key validation (OpenSSH format) and 64 KB user data cap enforced at create time
  • PATCH immutability for catalog_item, ssh_key, user_data on bare metal instances
  • Delete on catalog items blocked while referenced by a bare metal instance
  • Migration 54 creates the three database tables with correct creator/tenant schema and tenant FK constraints
  • Event type proto extended with bare metal payload fields (27–29); setPayload updated accordingly
  • All three servers registered in the gRPC server startup

Test plan

  • Unit tests added alongside each server file; all 64 internal test suites pass
  • ginkgo run -r internal green locally
  • go build ./... clean

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features
    • Added private gRPC services for bare metal instance templates, catalog items, and instances.
    • Extended event payloads to include bare metal instance-related types.
  • Enhancements
    • Enforced immutability for bare metal instance spec fields (catalog item, SSH key, user data).
    • Added OpenSSH public key validation and user-data size limits during instance creation.
    • Catalog item deletion is blocked while still referenced by existing instances.
  • Database Migration
    • Added bare-metal schema tables (active and archived) with indexes and constraints.
  • Tests
    • Added migration and server test coverage for new behavior and validations.

@openshift-ci-robot

openshift-ci-robot commented Jun 12, 2026

Copy link
Copy Markdown

@adriengentil: This pull request references OSAC-1344 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 task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

  • Add three private gRPC servers (PrivateBareMetalInstancesServer, PrivateBareMetalInstanceTemplatesServer, PrivateBareMetalInstanceCatalogItemsServer) backed by GenericDAO
  • SSH public key validation (OpenSSH format) and 64 KB user data cap enforced at create time
  • PATCH immutability for catalog_item, ssh_key, user_data on bare metal instances
  • Delete on catalog items blocked while referenced by a bare metal instance
  • Migration 54 creates the three database tables with correct creator/tenant schema and tenant FK constraints
  • Event type proto extended with bare metal payload fields (27–29); setPayload updated accordingly
  • All three servers registered in the gRPC server startup

Test plan

  • Unit tests added alongside each server file; all 64 internal test suites pass
  • ginkgo run -r internal green locally
  • go build ./... clean

🤖 Generated with Claude Code

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.

@openshift-ci

openshift-ci Bot commented Jun 12, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This PR introduces a complete bare metal instance management system with three gRPC-based servers (templates, catalog items, and instances), database schema with archive tables, immutability constraints on critical fields, reference integrity checking, SSH key validation, and event system integration. The implementation enforces create-time constraints (SSH validation, user data size, catalog item verification) and prevents update-time tampering with immutable fields via field mask inspection.

Changes

Bare Metal Instance Management System

Layer / File(s) Summary
Proto schema and field behavior contracts
proto/private/osac/private/v1/baremetal_instance_type.proto, proto/public/osac/public/v1/baremetal_instance_type.proto
BareMetalInstanceSpec fields catalog_item, ssh_key, and user_data are marked IMMUTABLE using field behavior annotations in both private and public API schemas.
Event payload support and notification wiring
proto/private/osac/private/v1/event_type.proto, internal/servers/generic_server.go
Event proto oneof payload is extended with three bare metal variants (fields 27–29); GenericServer.setPayload adds type-switch cases to map bare metal objects to event fields.
SSH public key validation
internal/servers/ssh_validation.go
Helper validates OpenSSH keys using ssh.ParseAuthorizedKey, rejects trailing content, and wraps parse errors for enforced use at create time.
Database schema and migration
internal/database/migrations/54_create_baremetal_tables.up.sql, internal/database/migrations/54_create_baremetal_tables_test.go
Migration creates active and archived tables for templates, catalog items, and instances with metadata, JSONB data/labels/annotations, btree indexes on name/creator/tenant, GIN indexes on labels, expression index on catalog item references, and tenant FK constraints; test validates insertion and foreign key enforcement.
Bare Metal Instance Templates Server
internal/servers/private_baremetal_instance_templates_server.go, internal/servers/private_baremetal_instance_templates_server_test.go
Builder validates required logger and tenancy logic; constructs typed GenericServer for BareMetalInstanceTemplate; all CRUD and Signal RPC operations delegate to generic server; tests verify builder dependencies and full CRUD flow.
Bare Metal Instance Catalog Items Server
internal/servers/private_baremetal_instance_catalog_items_server.go, internal/servers/private_baremetal_instance_catalog_items_server_test.go
Builder includes optional reference checker; Delete operation pre-checks via DAO whether catalog item is referenced by bare metal instances, returning gRPC FailedPrecondition if referenced; other CRUD and Signal operations delegate to generic server; tests verify reference-blocking delete and field definition return behavior.
Bare Metal Instances Server
internal/servers/private_baremetal_instances_server.go, internal/servers/private_baremetal_instances_server_test.go
Create validates OpenSSH keys, enforces 64 KiB user data limit, verifies catalog item existence via DAO with publication status and access control checks, and applies catalog field definitions; Update enforces immutability on spec.catalog_item, spec.ssh_key, and spec.user_data by comparing against stored values when update mask present; tests cover SSH validation, catalog constraints, size boundaries, and immutability blocking for PATCH and full replace operations.
gRPC server registration and startup
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
Registers and initializes all three bare metal servers with shared logger, notifier, attribution/tenancy logic, and metrics registerer; returns error if any server build fails.

Sequence Diagram

sequenceDiagram
  participant Client
  participant InstancesServer
  participant GenericServer
  participant CatalogItemsDAO
  participant Database
  Client->>InstancesServer: Create(BareMetalInstancesCreateRequest)
  InstancesServer->>InstancesServer: validateCreateSpec(ssh_key, user_data)
  InstancesServer->>CatalogItemsDAO: List(spec.catalog_item filter)
  CatalogItemsDAO->>Database: SELECT FROM bare_metal_instance_catalog_items
  Database-->>CatalogItemsDAO: CatalogItem
  CatalogItemsDAO-->>InstancesServer: CatalogItem or error
  InstancesServer->>GenericServer: Create(instance)
  GenericServer->>Database: INSERT INTO bare_metal_instances
  Database-->>GenericServer: id
  GenericServer-->>InstancesServer: CreateResponse
  InstancesServer-->>Client: CreateResponse
  Client->>InstancesServer: Update(BareMetalInstancesUpdateRequest with FieldMask)
  InstancesServer->>GenericServer: Get(instance_id)
  GenericServer->>Database: SELECT FROM bare_metal_instances
  Database-->>GenericServer: existing instance
  GenericServer-->>InstancesServer: Instance
  InstancesServer->>InstancesServer: validateImmutability(mask, new_spec, old_spec)
  alt Immutable fields changed
    InstancesServer-->>Client: InvalidArgument error
  else Immutable fields unchanged
    InstancesServer->>GenericServer: Update(instance)
    GenericServer->>Database: UPDATE bare_metal_instances
    Database-->>GenericServer: success
    GenericServer-->>InstancesServer: UpdateResponse
    InstancesServer-->>Client: UpdateResponse
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • osac-project/fulfillment-service#625: Implements reference-checking delete guards for catalog items referenced by other resources using the same DAO-backed pattern for reference validation.
  • osac-project/fulfillment-service#520: Extends event payload oneof variants and GenericServer.setPayload type-switch in the same files and patterns.
  • osac-project/fulfillment-service#542: Introduces catalog-item field-definition transformation primitives (applyFieldDefinitions, validateCatalogItemAccess) that this PR leverages in bare metal instance creation.

Suggested labels

lgtm

Suggested reviewers

  • tzumainn
  • jhernand

Poem

🔧 Bare metal blueprint rises high,
Three servers guard their catalog supply.
SSH keys and sizes checked with care,
Immutable fields no PATCH can dare.
References protected—integrity's prayer. ⚙️


Security Assessment

Risk Severity: Medium | Impact: Containment & Validation

This PR introduces critical validation checkpoints that prevent invalid or unsafe configurations:

  1. SSH Key Validation (Risk: Medium) — validateOpenSSHPublicKey enforces syntactic correctness via ssh.ParseAuthorizedKey, preventing storage of malformed keys that could cause authentication failures or injection vulnerabilities. Missing validation would allow arbitrary strings to be persisted as SSH keys, potentially enabling remote access with invalid credentials.

  2. Immutability Enforcement (Risk: Medium) — Three core fields (catalog_item, ssh_key, user_data) are marked immutable and enforced at update time via mask inspection. Without this, an attacker with update permissions could pivot from one catalog item to another or inject malicious user data post-creation, escalating from configuration drift to potential code execution or privilege escalation in the instance.

  3. Reference Integrity Guard (Risk: Low) — Delete on catalog items checks for active references before removal. Allows administrators to prevent accidental deletion of in-use templates, maintaining operational integrity. Low risk if deletion is soft (flagged) rather than cascading.

  4. Catalog Item Access Control (Risk: Medium) — validateCatalogItemAccess is called during instance creation, ensuring users cannot reference inaccessible or unpublished catalogs. Without this check, users could bypass tenancy isolation by specifying catalog items outside their scope.

  5. User Data Size Limit (Risk: Low) — 64 KiB cap on user_data mitigates DoS via unbounded cloud-init/userData payloads. Reasonable defensive boundary but not a critical vulnerability.

Recommendation: Verify that validateCatalogItemAccess includes tenant isolation checks and that update mask inspection correctly handles nested field paths for spec mutations.

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main objective of the PR: implementing three private gRPC servers for bare metal resources, which aligns with the substantial changes across database migrations, server implementations, and proto definitions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed No hardcoded secrets detected. The SSH public key constant in tests is safe—public keys are designed for sharing and the test key clearly identified as example data.
No-Weak-Crypto ✅ Passed No weak cryptographic algorithms (MD5, SHA1, DES, RC4, etc.) found. Non-constant-time string comparisons only apply to non-secret fields (ssh_key, user_data). This is consistent with prior art in c...
No-Injection-Vectors ✅ Passed No injection vectors detected in PR. Database migration uses static DDL with no dynamic SQL. All DAO queries use parameterized placeholders. CEL filter construction properly escapes user input via...
Container-Privileges ✅ Passed No container or Kubernetes manifest files modified in PR. Changes are limited to Go source, SQL migrations, and protobuf definitions with no privilege escalation configurations present.
No-Sensitive-Data-In-Logs ✅ Passed No sensitive data (SSH keys, user data payloads, credentials, tokens, or PII) is logged in the PR. Two error logs only capture DAO infrastructure errors from database operations, not business logic...
Ai-Attribution ✅ Passed AI tool (Claude Code) is properly attributed via Assisted-by trailer in commit message and PR description disclosure; no improper Co-Authored-By usage detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@adriengentil

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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 `@internal/database/migrations/54_create_baremetal_tables_test.go`:
- Around line 24-46: The test in DescribeTable (the anonymous func used by Entry
for bare_metal_instance_templates/catalog_items/instances) only inserts tenant
"system" so it won't catch dropped tenant FKs; after the successful insert and
count check, add a negative insert using conn.Exec that attempts to insert a row
with a nonexistent tenant (e.g., "no-such-tenant") into the same table and
assert it fails with a foreign-key constraint error
(Expect(err).To(HaveOccurred()) and optionally assert error text/SQLState),
ensuring the tenant FK constraints (bare_metal_instance_templates_tenant_fk,
bare_metal_instance_catalog_items_tenant_fk, bare_metal_instances_tenant_fk) are
enforced by the migration.

In `@internal/servers/private_baremetal_instance_catalog_items_server.go`:
- Around line 89-101: The Build() methods for
PrivateBareMetalInstanceCatalogItemsServerBuilder and
PrivateBareMetalInstancesServerBuilder are missing the attributionLogic
precondition; update each Build() (e.g.,
PrivateBareMetalInstanceCatalogItemsServerBuilder.Build and
PrivateBareMetalInstancesServerBuilder.Build) to validate b.attributionLogic is
non-nil and return err = errors.New("attribution logic is mandatory") before
proceeding to construct the GenericServer (i.e., add the same nil-check pattern
used for b.tenancyLogic immediately prior to the NewGenericServer(...) chain).

In `@internal/servers/private_baremetal_instance_templates_server_test.go`:
- Around line 67-172: Add a round-trip test that exercises
PrivateBareMetalInstanceTemplatesServer.Signal: construct a
privatev1.BareMetalInstanceTemplatesSignalRequest (using
BareMetalInstanceTemplatesSignalRequest_builder), call server.Signal(ctx, req),
assert no error and that the returned response contains the expected fields (or
proto.Equal matches an expected response) to verify GenericServer
request/response resolution wiring; place the test alongside the other
Describe("Behaviour") specs so it uses the same
NewPrivateBareMetalInstanceTemplatesServer setup and validates the Signal method
and its contract with GenericServer.

In `@internal/servers/private_baremetal_instances_server.go`:
- Around line 241-248: The code only checks hasMaskPrefix(mask, "...") and
returns early when none match, allowing full-replace updates (empty/nil
update_mask) to bypass immutability checks; change the logic in the update
handler that uses request.GetUpdateMask() so that when the update mask is nil or
empty you treat it as a full-field update (i.e. set
updatingCatalogItem/updatingSshKey/updatingUserData to true or generate an
implicit mask including "spec.catalog_item", "spec.ssh_key", "spec.user_data")
and then enforce immutability, and apply the same fix to the second block that
mirrors this logic (the other check around lines 274-287). Ensure you still use
hasMaskPrefix(...) for explicit masks but do not return early for empty/nil
masks.
- Around line 250-287: The validateImmutability() logic dereferences newSpec
(bmi.GetSpec()) without checking for nil, which can panic if the client includes
spec fields in the update mask but omits object.spec; modify the function to
first retrieve newSpec := bmi.GetSpec() and if newSpec == nil and any of the
flags updatingCatalogItem/updatingSshKey/updatingUserData are true, return a
grpc InvalidArgument describing that object.spec is required when those spec
fields are targeted; similarly guard existingSpec (existing.GetSpec()) before
comparisons to avoid nil derefs and perform comparisons only when both specs are
non-nil.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: c6952d9d-3991-41b0-8966-a74de40da68a

📥 Commits

Reviewing files that changed from the base of the PR and between c385789 and 296b06b.

⛔ Files ignored due to path filters (6)
  • internal/api/osac/private/v1/baremetal_instance_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/event_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/event_type_protoopaque.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/public/v1/baremetal_instance_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/public/v1/baremetal_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (13)
  • internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
  • internal/database/migrations/54_create_baremetal_tables.up.sql
  • internal/database/migrations/54_create_baremetal_tables_test.go
  • internal/servers/generic_server.go
  • internal/servers/private_baremetal_instance_catalog_items_server.go
  • internal/servers/private_baremetal_instance_catalog_items_server_test.go
  • internal/servers/private_baremetal_instance_templates_server.go
  • internal/servers/private_baremetal_instance_templates_server_test.go
  • internal/servers/private_baremetal_instances_server.go
  • internal/servers/private_baremetal_instances_server_test.go
  • proto/private/osac/private/v1/baremetal_instance_type.proto
  • proto/private/osac/private/v1/event_type.proto
  • proto/public/osac/public/v1/baremetal_instance_type.proto

Comment thread internal/database/migrations/54_create_baremetal_tables_test.go
Comment thread internal/servers/private_baremetal_instance_catalog_items_server.go
Comment thread internal/servers/private_baremetal_instances_server.go Outdated
Comment thread internal/servers/private_baremetal_instances_server.go
@adriengentil

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@adriengentil

Copy link
Copy Markdown
Contributor Author

No-Weak-Crypto: false positive

The check flagged ssh_key and user_data immutability comparisons as timing side-channels. Neither field is a secret:

  • ssh_key is an SSH public key — by definition not sensitive.
  • user_data is cloud-init/ignition configuration — treated as regular data, not a credential. ComputeInstance has identical ssh_key and user_data fields and uses plain equality throughout without any redaction or constant-time treatment.

Constant-time comparison is only meaningful when an attacker can probe a secret value byte-by-byte via timing. That threat model doesn't apply here: an immutability check compares the client-supplied value against the stored value — the stored value is already accessible to the authenticated caller via a GET. There is no secret to probe.

Reverting to plain != (same as ComputeInstance) and requesting the check be suppressed or tuned for this case.

@adriengentil adriengentil force-pushed the feat/OSAC-1343 branch 2 times, most recently from bba43fd to 82a8545 Compare June 12, 2026 15:08
@adriengentil adriengentil marked this pull request as ready for review June 12, 2026 15:11
@openshift-ci openshift-ci Bot requested review from tzumainn and tzvatot June 12, 2026 15:11
@adriengentil

Copy link
Copy Markdown
Contributor Author

/cc @carbonin

@openshift-ci openshift-ci Bot requested a review from carbonin June 12, 2026 15:12
Comment thread internal/servers/private_baremetal_instance_catalog_items_server.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@internal/servers/private_baremetal_instances_server.go`:
- Around line 137-145: The validation order in the Create method at line 137-145
is incorrect: validateSpec is called before validateAndApplyCatalogItem, which
means validation happens on the original spec before catalog defaults are
applied. This allows invalid data introduced by catalog field definitions (such
as invalid ssh_key or oversized user_data) to bypass validation and be
persisted. Reorder the method calls in the Create method of
PrivateBareMetalInstancesServer so that validateSpec is called after
validateAndApplyCatalogItem, ensuring the final spec (after all catalog defaults
have been applied) is validated against the contract requirements. Apply the
same fix at line 237 where this pattern is also present.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 38e7388b-a1de-4947-bfac-2ea2d8ae655d

📥 Commits

Reviewing files that changed from the base of the PR and between 296b06b and 3f29056.

⛔ Files ignored due to path filters (6)
  • internal/api/osac/private/v1/baremetal_instance_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/event_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/event_type_protoopaque.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/public/v1/baremetal_instance_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/public/v1/baremetal_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (14)
  • internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
  • internal/database/migrations/54_create_baremetal_tables.up.sql
  • internal/database/migrations/54_create_baremetal_tables_test.go
  • internal/servers/generic_server.go
  • internal/servers/private_baremetal_instance_catalog_items_server.go
  • internal/servers/private_baremetal_instance_catalog_items_server_test.go
  • internal/servers/private_baremetal_instance_templates_server.go
  • internal/servers/private_baremetal_instance_templates_server_test.go
  • internal/servers/private_baremetal_instances_server.go
  • internal/servers/private_baremetal_instances_server_test.go
  • internal/servers/ssh_validation.go
  • proto/private/osac/private/v1/baremetal_instance_type.proto
  • proto/private/osac/private/v1/event_type.proto
  • proto/public/osac/public/v1/baremetal_instance_type.proto

Comment thread internal/servers/private_baremetal_instances_server.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/servers/private_baremetal_instances_server.go (1)

233-237: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Major: prevent catalog defaults from rewriting the validated catalog reference.

validateCatalogItemAccess checks the item resolved from the original ref, but applyFieldDefinitions can then mutate spec.catalog_item. A misconfigured catalog item can persist a bare metal instance that references a different or empty catalog item than the one that was access-checked, breaking immutable-reference and delete-guard integrity.

Suggested fix
 	if err := validateCatalogItemAccess(item, ref); err != nil {
 		return err
 	}
 
-	return applyFieldDefinitions(bmi.GetSpec(), item.GetFieldDefinitions())
+	spec := bmi.GetSpec()
+	if err := applyFieldDefinitions(spec, item.GetFieldDefinitions()); err != nil {
+		return err
+	}
+	if spec.GetCatalogItem() != ref {
+		return grpcstatus.Errorf(grpccodes.InvalidArgument,
+			"catalog item field definitions cannot change spec.catalog_item")
+	}
+	return nil
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/servers/private_baremetal_instances_server.go` around lines 233 -
237, The validateCatalogItemAccess function validates the user's access to the
catalog item referenced in the original ref, but applyFieldDefinitions can
subsequently mutate the spec.catalog_item field with defaults from the catalog
item, allowing a different or empty catalog item to be persisted. To fix this,
preserve the validated catalog item reference by saving the original
spec.catalog_item value before calling applyFieldDefinitions, then restore it
after the field definitions are applied, ensuring the access-checked reference
is not overwritten by field definition defaults.
🤖 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.

Outside diff comments:
In `@internal/servers/private_baremetal_instances_server.go`:
- Around line 233-237: The validateCatalogItemAccess function validates the
user's access to the catalog item referenced in the original ref, but
applyFieldDefinitions can subsequently mutate the spec.catalog_item field with
defaults from the catalog item, allowing a different or empty catalog item to be
persisted. To fix this, preserve the validated catalog item reference by saving
the original spec.catalog_item value before calling applyFieldDefinitions, then
restore it after the field definitions are applied, ensuring the access-checked
reference is not overwritten by field definition defaults.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: f8aed40e-f2e2-4c97-ab35-6f3714f9b79f

📥 Commits

Reviewing files that changed from the base of the PR and between 3f29056 and c84f775.

📒 Files selected for processing (1)
  • internal/servers/private_baremetal_instances_server.go

@openshift-ci

openshift-ci Bot commented Jun 15, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, carbonin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add three private gRPC server implementations backed by GenericDAO:
- PrivateBareMetalInstancesServer: List/Get/Create/Delete/Update/Signal
  with SSH public key format validation (OpenSSH), user data size cap
  (64 KB), and PATCH immutability for catalog_item, ssh_key, user_data
- PrivateBareMetalInstanceTemplatesServer: full CRUD + Signal
- PrivateBareMetalInstanceCatalogItemsServer: full CRUD + Signal,
  blocks delete when referenced by a bare metal instance

Also adds:
- Event type proto fields (fields 27-29) for bare metal types and
  corresponding cases in GenericServer.setPayload
- Migration 54 creating the three database tables with tenant FK
- Unit tests for all three servers
- Registration of all three servers in the gRPC server startup

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
Add validateAndApplyCatalogItem to PrivateBareMetalInstancesServer.
On create, the server now verifies that the referenced catalog item
exists, is published, and is not deleted, mirroring the validation
that ComputeInstancesServer performs. Field definitions from the
catalog item are also applied to the spec.

Update tests to create a real catalog item before each test so the
catalog item reference validation passes.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
Add (google.api.field_behavior) = IMMUTABLE annotation to the three
immutable fields in BareMetalInstanceSpec, matching the pattern used
in virtual_network_type.proto.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
…ic proto

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
- Add attributionLogic nil-check to catalog items and instances Build()
- Fix validateImmutability to treat nil/empty update_mask as full replace
- Guard newSpec and existingSpec against nil in validateImmutability
- Add Signal round-trip test for BareMetalInstanceTemplates server
- Add negative tenant FK test to migration 54

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
- No-Weak-Crypto: use subtle.ConstantTimeCompare for ssh_key and user_data
  immutability comparisons to avoid timing side-channels
- No-Injection-Vectors: replace SQL identifier concatenation in migration
  test with pgx.Identifier.Sanitize() safe quoting
- No-Sensitive-Data-In-Logs: remove catalog ref, instance id, and tenant
  fields from error/warn log lines in instances server and generic server

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
ssh_key is an SSH public key (not secret) and user_data is cloud-init
configuration — neither warrants constant-time comparison. The
No-Weak-Crypto check is a false positive here; ComputeInstance uses
plain equality for the same fields.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
Reverts accidental removal of slog.String("requested", requestTenant)
from generic_server.go — that code is not part of this PR's change.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
Move validateOpenSSHPublicKey to shared ssh_validation.go. Use the
existing updateIncludesField helper (field_mask.go) in the baremetal
instances server instead of hasMaskPrefix, removing the redundant
fullReplace logic.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
…item

The hasReference check filters instances by spec.catalog_item using the
CEL expression `this.spec.catalog_item == <id>`, which translates to
`data->'spec'->>'catalog_item' = $1` in SQL. Without an index on this
expression, every catalog item deletion triggers a full sequential scan.

Add a functional B-tree index on the extracted JSONB field so PostgreSQL
can satisfy the equality lookup efficiently.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>
applyFieldDefinitions can overwrite ssh_key and user_data with catalog
defaults. Running validateSpec before that meant defaults injected by
the catalog bypassed OpenSSH key format and 64 KB user_data checks.
Swap the order so validateSpec always sees the final, post-catalog spec.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Adrien Gentil <agentil@redhat.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go (1)

378-384: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

High risk: unary auth runs after transaction creation.

Severity: Major. Impact: unauthenticated traffic can open DB transactions before auth rejection, increasing pool exhaustion/DoS surface and violating the documented interceptor contract.

Suggested fix
 		grpc.ChainUnaryInterceptor(
 			panicInterceptor.UnaryServer,
 			metricsInterceptor.UnaryServer,
 			loggingInterceptor.UnaryServer,
-			txInterceptor.UnaryServer,
 			authUnaryInterceptor,
+			txInterceptor.UnaryServer,
 		),

As per coding guidelines, the gRPC interceptor chain should be panic recovery → metrics → logging → authentication → database transaction management.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines
378 - 384, The interceptor chain in the grpc.ChainUnaryInterceptor call has
authentication and transaction management in the wrong order. Currently
authUnaryInterceptor runs after txInterceptor.UnaryServer, but it should run
before to prevent unauthenticated requests from opening database transactions.
Reorder the interceptors in the ChainUnaryInterceptor call so that
authUnaryInterceptor comes before txInterceptor.UnaryServer, resulting in the
correct sequence: panicInterceptor.UnaryServer, metricsInterceptor.UnaryServer,
loggingInterceptor.UnaryServer, authUnaryInterceptor, txInterceptor.UnaryServer.

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 `@internal/servers/private_baremetal_instances_server_test.go`:
- Around line 354-374: The test starting at line 354 only validates the success
path where a full-replace PATCH preserves the same immutable fields. Add a new
test case that attempts to perform a full-replace PATCH while changing an
immutable field (the CatalogItem in the BareMetalInstanceSpec) and assert that
the Update call fails with an error. This regression test will ensure that
future changes do not accidentally allow immutable field mutations during
full-replace operations.

In `@internal/servers/private_baremetal_instances_server.go`:
- Around line 261-270: The error handling in the immutability check after the
`s.generic.dao.Get().SetId(id).Do(ctx)` call only checks for `*dao.ErrNotFound`
errors and treats all other errors as internal failures. However, the DAO can
also return `*dao.ErrDenied` errors which should be mapped to
`grpccodes.PermissionDenied` instead of `grpccodes.Internal`. Add a second error
type check using `errors.As` for `*dao.ErrDenied` (following the same pattern as
the existing `*dao.ErrNotFound` check) and return a gRPC error with the
appropriate `PermissionDenied` status code, placing this check before the
fallback to `Internal` error mapping.

---

Outside diff comments:
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 378-384: The interceptor chain in the grpc.ChainUnaryInterceptor
call has authentication and transaction management in the wrong order. Currently
authUnaryInterceptor runs after txInterceptor.UnaryServer, but it should run
before to prevent unauthenticated requests from opening database transactions.
Reorder the interceptors in the ChainUnaryInterceptor call so that
authUnaryInterceptor comes before txInterceptor.UnaryServer, resulting in the
correct sequence: panicInterceptor.UnaryServer, metricsInterceptor.UnaryServer,
loggingInterceptor.UnaryServer, authUnaryInterceptor, txInterceptor.UnaryServer.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: bf71ca4d-ba8f-428e-9dbb-cbbc20b96b85

📥 Commits

Reviewing files that changed from the base of the PR and between c84f775 and 6a48ad4.

⛔ Files ignored due to path filters (6)
  • internal/api/osac/private/v1/baremetal_instance_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/event_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/private/v1/event_type_protoopaque.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/public/v1/baremetal_instance_type.pb.go is excluded by !**/*.pb.go
  • internal/api/osac/public/v1/baremetal_instance_type_protoopaque.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (14)
  • internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
  • internal/database/migrations/54_create_baremetal_tables.up.sql
  • internal/database/migrations/54_create_baremetal_tables_test.go
  • internal/servers/generic_server.go
  • internal/servers/private_baremetal_instance_catalog_items_server.go
  • internal/servers/private_baremetal_instance_catalog_items_server_test.go
  • internal/servers/private_baremetal_instance_templates_server.go
  • internal/servers/private_baremetal_instance_templates_server_test.go
  • internal/servers/private_baremetal_instances_server.go
  • internal/servers/private_baremetal_instances_server_test.go
  • internal/servers/ssh_validation.go
  • proto/private/osac/private/v1/baremetal_instance_type.proto
  • proto/private/osac/private/v1/event_type.proto
  • proto/public/osac/public/v1/baremetal_instance_type.proto

Comment thread internal/servers/private_baremetal_instances_server_test.go
Comment thread internal/servers/private_baremetal_instances_server.go
@carbonin

Copy link
Copy Markdown

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Jun 15, 2026
@openshift-merge-bot openshift-merge-bot Bot merged commit 93fa82b into osac-project:main Jun 15, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants