Repository navigation
NO-ISSUE: Add builtin system and shared organizations - #596
Conversation
|
@jhernand: This pull request explicitly references no jira issue. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
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:
WalkthroughThis PR introduces builtin tenants ( ChangesBuiltin Tenants Migration and Tests
CI Ginkgo Timeout Configuration
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR applies consistent context-threading pattern across many migration test files (refactoring), introduces a new migration with seed data and validation, and adds integration tests. The changes are heterogeneous (context updates, SQL migration, Go tests, CI workflow) but follow clear, predictable patterns. Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🔕 Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
245a285 to
4eb465c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/47_add_builtin_tenants_test.go`:
- Around line 28-33: The test currently selects and asserts only name and tenant
from the seeded organization row; extend the QueryRow/row.Scan call to also scan
into variables for creator and data (e.g., add creator and data variables), then
add Expect assertions verifying creator equals the expected creator value and
that data contains or equals the expected payload (use
Expect(data).ToNot(BeNil()) or a more specific equality/match as appropriate);
update the row.Scan(&name, &tenant) to row.Scan(&name, &tenant, &creator, &data)
and add corresponding Expect(...) checks to fully cover all inserted migration
fields.
- Around line 25-31: The migration test currently uses an unbounded context
which can hang CI; create a bounded context with a timeout (e.g., via
context.WithTimeout) and use that context when calling tool.Migrate(ctx, 47) and
conn.QueryRow(ctx, ...) (and any subsequent row.Scan operations), ensuring you
call cancel() with defer to clean up; update references in this test (functions:
tool.Migrate, conn.QueryRow, row.Scan) to use the new timeout context so the
test will abort rather than hang.
In `@it/it_builtin_tenants_test.go`:
- Around line 32-35: Replace the unbounded context in the BeforeEach setup with
a timeout-scoped context by calling context.WithTimeout(context.Background(),
<reasonable duration>) and assign both ctx and its cancel function (ensure
cancel is invoked appropriately e.g., in AfterEach or via defer in the test) so
private API RPCs (used by privatev1.NewOrganizationsClient) cannot hang CI;
additionally, after calling client.Get(...) add an assertion
Expect(response).ToNot(BeNil()) before any use of response.GetObject() to avoid
dereferencing a nil response (mirror the nil-check already done for List calls).
🪄 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: d1b5aa7d-0e32-45d3-8523-add47c898331
📒 Files selected for processing (3)
internal/database/migrations/47_add_builtin_tenants.up.sqlinternal/database/migrations/47_add_builtin_tenants_test.goit/it_builtin_tenants_test.go
4eb465c to
d476bea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/check-pull-request.yaml (2)
55-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing explicit permissions block violates least-privilege principle.
The workflow uses default GITHUB_TOKEN permissions, which grant broad read-write access to repository contents, issues, pull requests, and other scopes. These jobs only require read access to checkout code and potentially read cache.
Risk severity: Major
Impact: If the workflow or its dependencies are compromised (e.g., through supply chain attack or malicious PR exploit), an attacker gains unnecessary write permissions, allowing them to push commits, create releases, modify issues/PRs, or exfiltrate secrets from other jobs.Add a top-level permissions block to enforce least privilege across all jobs. As per coding guidelines, CI/CD workflows must minimize GITHUB_TOKEN permissions.
🔒 Proposed fix to add least-privilege permissions
Add this block after line 21 (after the
on:trigger section):branches: - main +permissions: + contents: read + jobs:This restricts all jobs to read-only access. Jobs that need additional permissions can override at the job level.
Also applies to: 74-90, 92-108
🤖 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 @.github/workflows/check-pull-request.yaml around lines 55 - 62, Add a top-level permissions block to the workflow to enforce least-privilege for GITHUB_TOKEN (e.g., set permissions: contents: read, actions: read, checks: read as appropriate) so all jobs including the run-unit-tests job default to read-only access; if specific jobs require extra scopes, override the permissions at the job level (such as within the run-unit-tests job) rather than relying on broad default permissions. Ensure the new permissions block is placed at the top level of the YAML (after the on: trigger) so it applies globally and only loosen permissions per-job when explicitly necessary.
28-30: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftActions should be pinned by full SHA commit, not by tag.
Multiple actions use mutable tag references (e.g.,
@v6,@v7,@v3.0.1) instead of immutable SHA pins. Tags can be moved or deleted, allowing attackers to substitute malicious code if an action's repository is compromised.Risk severity: Major
Impact: Supply chain attack vector. If an upstream action repository is compromised, attackers can repoint tags to malicious commits, executing arbitrary code in your CI with GITHUB_TOKEN permissions.Pin actions by their full commit SHA (e.g.,
actions/checkout@0123456789abcdef...) to ensure immutable references. As per coding guidelines, CI/CD workflows must pin actions by SHA, not tag.Example transformation:
# Before (vulnerable to tag repointing) - uses: actions/checkout@v6 # After (pinned to immutable SHA) - uses: actions/checkout@a1b2c3d4e5f6... # v6You can use tools like
pin-github-actionor Dependabot to automate SHA pinning and updates.Also applies to: 36-36, 46-47, 59-59, 68-70, 78-78, 86-86, 96-96, 104-104
🤖 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 @.github/workflows/check-pull-request.yaml around lines 28 - 30, Replace all mutable action tags with immutable commit SHAs: update each occurrence of actions/checkout@v6, actions/setup-python@v6, pre-commit/action@v3.0.1 (and the other listed action usages) to use the corresponding full commit SHA for that release (e.g., actions/checkout@<full-sha> # v6), keeping the human-readable tag as a comment; ensure every occurrence referenced in the review (the multiple lines mentioned) is changed so no actions use tag references anymore.
♻️ Duplicate comments (1)
it/it_builtin_tenants_test.go (1)
36-41: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd defensive nil check for
responsebefore dereferencing.The
client.Getcall returnsresponse, errand the code checkserr, then immediately callsresponse.GetObject()without verifyingresponse != nil. While the gRPC contract implieserr == nilguaranteesresponse != nil, theListtest on line 55 already includes this defensive check (Expect(response).ToNot(BeNil())), creating an inconsistency.Risk: If a gRPC implementation bug violates the contract and returns
(nil, nil), the test will panic on nil dereference. Severity: minor defensive gap.🛡️ Proposed fix to match List test pattern
response, err := client.Get(ctx, privatev1.OrganizationsGetRequest_builder{ Id: id, }.Build()) Expect(err).ToNot(HaveOccurred()) +Expect(response).ToNot(BeNil()) object := response.GetObject()🤖 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 `@it/it_builtin_tenants_test.go` around lines 36 - 41, The test calls client.Get and checks err but then dereferences response via response.GetObject() without ensuring response != nil; add a defensive Expect(response).ToNot(BeNil()) immediately after Expect(err).ToNot(HaveOccurred()) (matching the List test pattern) before calling response.GetObject() to prevent a nil dereference if a (nil, nil) gRPC response occurs.
🤖 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 `@it/it_builtin_tenants_test.go`:
- Around line 48-49: Replace the hardcoded tenant ID strings in the test Entry
calls with the auth package constants to ensure consistency with the migration
tests: change the Entry("System", "system") and Entry("Shared", "shared") usages
to use auth.SystemTenant and auth.SharedTenant respectively (refer to the Entry
calls in it_builtin_tenants_test.go) and update any related assertions or
iterations to reference those constants as well.
---
Outside diff comments:
In @.github/workflows/check-pull-request.yaml:
- Around line 55-62: Add a top-level permissions block to the workflow to
enforce least-privilege for GITHUB_TOKEN (e.g., set permissions: contents: read,
actions: read, checks: read as appropriate) so all jobs including the
run-unit-tests job default to read-only access; if specific jobs require extra
scopes, override the permissions at the job level (such as within the
run-unit-tests job) rather than relying on broad default permissions. Ensure the
new permissions block is placed at the top level of the YAML (after the on:
trigger) so it applies globally and only loosen permissions per-job when
explicitly necessary.
- Around line 28-30: Replace all mutable action tags with immutable commit SHAs:
update each occurrence of actions/checkout@v6, actions/setup-python@v6,
pre-commit/action@v3.0.1 (and the other listed action usages) to use the
corresponding full commit SHA for that release (e.g.,
actions/checkout@<full-sha> # v6), keeping the human-readable tag as a comment;
ensure every occurrence referenced in the review (the multiple lines mentioned)
is changed so no actions use tag references anymore.
---
Duplicate comments:
In `@it/it_builtin_tenants_test.go`:
- Around line 36-41: The test calls client.Get and checks err but then
dereferences response via response.GetObject() without ensuring response != nil;
add a defensive Expect(response).ToNot(BeNil()) immediately after
Expect(err).ToNot(HaveOccurred()) (matching the List test pattern) before
calling response.GetObject() to prevent a nil dereference if a (nil, nil) gRPC
response occurs.
🪄 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: 53068a9e-69ed-459a-b0a4-33be73adffb9
📒 Files selected for processing (13)
.github/workflows/check-pull-request.yamlinternal/database/migrations/39_move_hub_fields_to_spec_test.gointernal/database/migrations/40_rename_tenants_to_tenant_test.gointernal/database/migrations/41_rename_creators_to_creator_test.gointernal/database/migrations/42_singular_tenant_and_creator_in_public_ip_attachments_tables_test.gointernal/database/migrations/43_drop_leases_tables_test.gointernal/database/migrations/44_add_public_ip_attachments_unique_indexes_test.gointernal/database/migrations/45_fix_tables_test.gointernal/database/migrations/46_add_immutable_column_trigger_test.gointernal/database/migrations/47_add_builtin_tenants.up.sqlinternal/database/migrations/47_add_builtin_tenants_test.gointernal/database/migrations/migrations_suite_test.goit/it_builtin_tenants_test.go
d476bea to
e4cb77c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/database/migrations/44_add_public_ip_attachments_unique_indexes_test.go (1)
24-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse UUIDv7 fixture IDs for
public_ip/compute_instancevalues (minor severity, contract-drift risk).Using slug-style IDs (
pip-1,ci-1) in these changed migration fixtures weakens parity with production ID formats and can mask failures if stricter ID validation is introduced.Suggested fixture pattern update
- err := insert(ctx, "a1", "pip-1", "ci-1") + err := insert(ctx, "a1", "019728a4-3f5c-7def-8abc-1234567890ab", "019728a4-3f5c-7e01-8abc-1234567890ab") - err = insert(ctx, "a2", "pip-1", "ci-2") + err = insert(ctx, "a2", "019728a4-3f5c-7def-8abc-1234567890ab", "019728a4-3f5c-7e02-8abc-1234567890ab")Based on learnings: In this repository, resource IDs (including PublicIP and ComputeInstance in test fixtures) should use UUIDv7-format strings.
Also applies to: 43-45, 50-51, 55-56, 61-62, 66-67, 74-75, 79-80, 87-88, 92-93, 98-99
🤖 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/database/migrations/44_add_public_ip_attachments_unique_indexes_test.go` around lines 24 - 31, Update the test fixture values to use UUIDv7-format IDs instead of slug-style IDs in the insert helper: inside the insert function (and its call sites) replace test values passed as publicIP and computeInstance (currently like "pip-1"/"ci-1") with UUIDv7-like strings (e.g., 16-byte timestamp-prefixed UUIDs used elsewhere in tests) so fixtures mirror production ID format; apply the same change to all related test cases referenced (the other migration tests mentioned) to keep parity with production ID formats and avoid contract drift.
🤖 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 @.github/workflows/check-pull-request.yaml:
- Line 62: Add an explicit least-privilege permissions block for the workflow or
for the specific job that runs the tests (the job invoking "ginkgo run --timeout
1h -r internal"), setting GITHUB_TOKEN permissions only to the minimal scopes
required (e.g., contents: read and any other specific read-only scopes your
tests need) and remove reliance on default broad scopes; if any step needs extra
privileges, grant them only on that job by adding a job-level permissions stanza
for GITHUB_TOKEN with the narrower scopes.
---
Outside diff comments:
In
`@internal/database/migrations/44_add_public_ip_attachments_unique_indexes_test.go`:
- Around line 24-31: Update the test fixture values to use UUIDv7-format IDs
instead of slug-style IDs in the insert helper: inside the insert function (and
its call sites) replace test values passed as publicIP and computeInstance
(currently like "pip-1"/"ci-1") with UUIDv7-like strings (e.g., 16-byte
timestamp-prefixed UUIDs used elsewhere in tests) so fixtures mirror production
ID format; apply the same change to all related test cases referenced (the other
migration tests mentioned) to keep parity with production ID formats and avoid
contract drift.
🪄 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: c41b8c1f-7ccf-45a1-9b70-1e649ef0578e
📒 Files selected for processing (13)
.github/workflows/check-pull-request.yamlinternal/database/migrations/39_move_hub_fields_to_spec_test.gointernal/database/migrations/40_rename_tenants_to_tenant_test.gointernal/database/migrations/41_rename_creators_to_creator_test.gointernal/database/migrations/42_singular_tenant_and_creator_in_public_ip_attachments_tables_test.gointernal/database/migrations/43_drop_leases_tables_test.gointernal/database/migrations/44_add_public_ip_attachments_unique_indexes_test.gointernal/database/migrations/45_fix_tables_test.gointernal/database/migrations/46_add_immutable_column_trigger_test.gointernal/database/migrations/47_add_builtin_tenants.up.sqlinternal/database/migrations/47_add_builtin_tenants_test.gointernal/database/migrations/migrations_suite_test.goit/it_builtin_tenants_test.go
This is a preparatory step towards making the tenant field mandatory and enforcing that every tenant references an existing organization via a database foreign key constraint. For that to work the `system` and `shared` tenants, which are already used by convention throughout the codebase, must exist as rows in the `organizations` table. Migration 46 inserts both organizations with matching `id`, `name`, and `tenant` columns (e.g. `system`/`system`/`system`) and their status set to `ORGANIZATION_STATE_SYNCED` since they are always operational and do not require IDP reconciliation. The list tests in `organizations_server_test.go` and `private_organizations_server_test.go` are updated to filter by name so they remain deterministic now that the table is no longer empty at test start. A migration test verifies the rows are created correctly, and an integration test confirms both organizations are retrievable through the private gRPC API. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com> Assisted-by: Cursor
e4cb77c to
889af76
Compare
2 participants
Summary
This is a preparatory step towards making the tenant field mandatory and enforcing that every
tenant references an existing organization via a database foreign key constraint. For that to
work the
systemandsharedtenants, which are already used by convention throughout thecodebase, must exist as rows in the
organizationstable.id,name, andtenantcolumns(e.g.
system/system/system).deterministic now that the table is no longer empty at test start.
Test plan
ginkgo run --focus="Add builtin tenants" internal/database/migrations)ginkgo run internal/database/migrations)go vet ./it/...)Summary by CodeRabbit
Release Notes
New Features
Tests