OSAC-2158: Implement ClusterVersion delete protection - #157
Conversation
|
@sk-ilya: This pull request references OSAC-2158 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 story to target the "5.0.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. |
WalkthroughChangesCluster version integrity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PrivateClusterVersionsServer
participant cluster_versions
participant ActiveReferences
PrivateClusterVersionsServer->>cluster_versions: Request soft-delete
cluster_versions->>ActiveReferences: Check active clusters, templates, and catalog items
ActiveReferences-->>cluster_versions: Return matching reference
cluster_versions-->>PrivateClusterVersionsServer: Return FailedPrecondition
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger.up.sql (2)
26-29: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex only covers one of the three outbound lookups.
check_cluster_version_not_in_use()probes three tables. The migration indexesclustersonly. Thecluster_templatesprobe ondata->'spec_defaults'->>'version_name'and thecluster_catalog_itemslateral unnest both fall back to sequential scans on every version soft-delete. Add a matching partial expression index forcluster_templates. Forcluster_catalog_items, a plain expression index cannot serve the unnest, so consider a GIN index ondata->'field_definitions'plus a containment predicate, or accept the scan and document the cost.♻️ Add the template index
create index clusters_version_name on clusters ((data->'spec'->>'version_name')) where data->'spec'->>'version_name' is not null; + +create index cluster_templates_version_name on cluster_templates + ((data->'spec_defaults'->>'version_name')) + where data->'spec_defaults'->>'version_name' is not null;🤖 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 `@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger.up.sql` around lines 26 - 29, Extend the migration’s indexing for check_cluster_version_not_in_use() by adding a partial expression index on cluster_templates for data->'spec_defaults'->>'version_name', matching the existing clusters index pattern and excluding null values. The cluster_catalog_items lookup is not covered by this requested change.
27-29: 🚀 Performance & Scalability | 🔵 TrivialIndex build takes ACCESS EXCLUSIVE on
clusters.Squawk flags the missing
concurrently. If the migration runner wraps each file in a transaction,create index concurrentlyis not usable, so this may be unavoidable. Confirm the expectedclustersrow count at rollout time. If the table is large, split the index into a separate non-transactional migration step.🤖 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 `@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger.up.sql` around lines 27 - 29, Confirm the expected clusters row count and migration transaction behavior before creating clusters_version_name. If the table is large, move this index creation into a separate non-transactional migration and build it concurrently; otherwise retain the current definition with documented justification for the locking behavior.Source: Linters/SAST tools
fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger_test.go (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
softDeletebuilds SQL by concatenation.The three call sites pass constant literals, so this is not exploitable. The coding guidelines still ask for concatenation-free SQL. Use
pgx.Identifier{table}.Sanitize()to make the intent explicit and keep the helper safe if a caller ever passes a variable.Based on the coding guideline "Flag SQL string concatenation".
🤖 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 `@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger_test.go` around lines 51 - 55, Update the softDelete helper to construct the table reference with pgx.Identifier{table}.Sanitize() instead of concatenating table into the SQL string, while preserving the parameterized id argument and existing error assertion.Source: Coding guidelines
fulfillment-service/internal/servers/private_cluster_versions_server_test.go (1)
222-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing helpers instead of repeating Create and the status assertion.
The three new specs re-implement what
createCValready does, and each repeats the same four-lineFailedPreconditionassertion. The file already establishedexpectInvalidArgumentfor that pattern. AddexpectFailedPreconditionnext to it, usecreateCV(versionName, "4.17.0"), and build the DAOs once inBeforeEach. That removes roughly 40 lines across the three specs.♻️ Sketch
+ expectFailedPrecondition = func(err error, substring string) { + Expect(err).To(HaveOccurred()) + status, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(status.Code()).To(Equal(grpccodes.FailedPrecondition)) + Expect(status.Message()).To(ContainSubstring(substring)) + }It("Blocks delete when referenced by an active cluster", func() { versionName := "del-prot-4-17-0" - response, err := server.Create(ctx, privatev1.ClusterVersionsCreateRequest_builder{ - ... - }.Build()) - Expect(err).ToNot(HaveOccurred()) - cv := response.GetObject() + cv := createCV(versionName, "4.17.0")🤖 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 `@fulfillment-service/internal/servers/private_cluster_versions_server_test.go` around lines 222 - 264, Refactor the affected delete specs to reuse the existing createCV helper instead of duplicating ClusterVersion creation. Add an expectFailedPrecondition helper alongside expectInvalidArgument for the shared gRPC assertion, and update the specs to use it. Initialize the clusters DAO once in BeforeEach and reuse it across the tests, removing the repeated DAO construction and assertion code.
🤖 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
`@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger_test.go`:
- Around line 37-43: Add a `shared` tenant fixture in the test setup before the
`insertVersion` helper inserts into `cluster_versions`, ensuring the existing
`cluster_versions_tenant_fk` reference resolves while preserving the current
`test-tenant` fixture and insertion logic.
---
Nitpick comments:
In
`@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger_test.go`:
- Around line 51-55: Update the softDelete helper to construct the table
reference with pgx.Identifier{table}.Sanitize() instead of concatenating table
into the SQL string, while preserving the parameterized id argument and existing
error assertion.
In
`@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger.up.sql`:
- Around line 26-29: Extend the migration’s indexing for
check_cluster_version_not_in_use() by adding a partial expression index on
cluster_templates for data->'spec_defaults'->>'version_name', matching the
existing clusters index pattern and excluding null values. The
cluster_catalog_items lookup is not covered by this requested change.
- Around line 27-29: Confirm the expected clusters row count and migration
transaction behavior before creating clusters_version_name. If the table is
large, move this index creation into a separate non-transactional migration and
build it concurrently; otherwise retain the current definition with documented
justification for the locking behavior.
In
`@fulfillment-service/internal/servers/private_cluster_versions_server_test.go`:
- Around line 222-264: Refactor the affected delete specs to reuse the existing
createCV helper instead of duplicating ClusterVersion creation. Add an
expectFailedPrecondition helper alongside expectInvalidArgument for the shared
gRPC assertion, and update the specs to use it. Initialize the clusters DAO once
in BeforeEach and reuse it across the tests, removing the repeated DAO
construction and assertion code.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: daaf4166-cdce-49ab-a08a-f9bae052b222
📒 Files selected for processing (4)
fulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger.up.sqlfulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger_test.gofulfillment-service/internal/servers/private_cluster_versions_server_test.go
| insertVersion := func(ctx context.Context, id, name string) { | ||
| _, err := conn.Exec(ctx, | ||
| `insert into cluster_versions (id, name, tenant, data) | ||
| values ($1, $2, 'shared', $3::jsonb)`, | ||
| id, name, `{"spec":{"version":"4.17.0","image":"quay.io/test:4.17.0"}}`) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the cluster_versions table definition and any tenant constraint.
fd -e sql . fulfillment-service/internal/database/migrations | xargs rg -nP -C6 'create table (if not exists )?cluster_versions|references tenants'Repository: osac-project/osac
Length of output: 27496
Add the shared tenant fixture before inserting cluster_versions. The cluster_versions_tenant_fk constraint references tenants(name), so the shared insert fails when setup creates only test-tenant.
🤖 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
`@fulfillment-service/internal/database/migrations/90_add_cluster_version_delete_protection_trigger_test.go`
around lines 37 - 43, Add a `shared` tenant fixture in the test setup before the
`insertVersion` helper inserts into `cluster_versions`, ensuring the existing
`cluster_versions_tenant_fk` reference resolves while preserving the current
`test-tenant` fixture and insertion logic.
0da0e1d to
2ce109d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger_test.go (1)
51-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace dynamic SQL table concatenation with a closed allowlist.
softDeleteinterpolatestableintoconn.Exec. The current calls are fixed literals, but keep table selection in aswitchthat maps each supported table to a constant query.As per coding guidelines, “Flag SQL string concatenation.” As per path instructions, “SQL: parameterized queries only; no string concatenation”.
🤖 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 `@fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger_test.go` around lines 51 - 54, The softDelete helper builds SQL with dynamic table concatenation; replace it with a switch on table that maps each supported table name to a constant update query, then execute the selected query with id as the parameter. Preserve the existing error assertion and handle unsupported table values explicitly.Sources: Coding guidelines, Path instructions
🤖 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
`@fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql`:
- Around line 134-139: The update trigger conditions in
fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql
at lines 134-139, 150-155, and 190-195 must also validate when restoring a
previously soft-deleted resource. Update each trigger’s WHEN condition to
trigger if old.deletion_timestamp <> 'epoch' or the corresponding reference data
changes: spec.version_name at lines 134-139, spec_defaults.version_name at lines
150-155, and field_definitions at lines 190-195.
---
Nitpick comments:
In
`@fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger_test.go`:
- Around line 51-54: The softDelete helper builds SQL with dynamic table
concatenation; replace it with a switch on table that maps each supported table
name to a constant update query, then execute the selected query with id as the
parameter. Preserve the existing error assertion and handle unsupported table
values explicitly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a77d63f5-e6aa-43b2-88ef-22d9fe7e52c6
📒 Files selected for processing (4)
fulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sqlfulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger_test.gofulfillment-service/internal/servers/private_cluster_versions_server_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- fulfillment-service/internal/database/migrations.sha256
- fulfillment-service/internal/servers/private_cluster_versions_server_test.go
| create trigger check_cluster_version_name_ref_on_update | ||
| before update on clusters | ||
| for each row | ||
| when (new.deletion_timestamp = 'epoch' and | ||
| (new.data->'spec'->>'version_name') is distinct from (old.data->'spec'->>'version_name')) | ||
| execute function check_cluster_version_ref('spec', 'version_name'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate references when a soft-deleted resource becomes active.
Each update trigger skips validation if its JSONB reference data is unchanged. A resource can be soft-deleted, its version can then be soft-deleted, and the resource can be restored with an invalid active reference.
fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql#L134-L139: validate whenold.deletion_timestamp <> 'epoch'orspec.version_namechanges.fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql#L150-L155: validate whenold.deletion_timestamp <> 'epoch'orspec_defaults.version_namechanges.fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql#L190-L195: validate whenold.deletion_timestamp <> 'epoch'orfield_definitionschanges.
📍 Affects 1 file
fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql#L134-L139(this comment)fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql#L150-L155fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql#L190-L195
🤖 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
`@fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql`
around lines 134 - 139, The update trigger conditions in
fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql
at lines 134-139, 150-155, and 190-195 must also validate when restoring a
previously soft-deleted resource. Update each trigger’s WHEN condition to
trigger if old.deletion_timestamp <> 'epoch' or the corresponding reference data
changes: spec.version_name at lines 134-139, spec_defaults.version_name at lines
150-155, and field_definitions at lines 190-195.
There was a problem hiding this comment.
the resource can be restored - how?
There was a problem hiding this comment.
@sk-ilya, restoration means an UPDATE that changes the resource from soft-deleted to active:
update clusters
set deletion_timestamp = 'epoch'
where id = ...;The same applies to cluster_templates and cluster_catalog_items.
For example:
- A cluster references version
v1. - The cluster is soft-deleted. Its
deletion_timestampbecomes non-'epoch'. - Version
v1is soft-deleted. The outbound trigger permits this because the cluster is inactive. - The cluster is restored by setting
deletion_timestampback to'epoch'.
In step 4, spec.version_name does not change. The current update trigger does not run. The restored active cluster can therefore reference deleted v1.
This is a database-level integrity case. It applies even if the current service does not expose a restore operation, because an update or a future restore path can perform that transition. 🐇🔍
You are interacting with an AI system.
There was a problem hiding this comment.
are there any other cases of such a reference protection in this codebase
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate migration files =='
fd -e sql . fulfillment-service/internal/database/migrations \
| xargs rg -l -i -n -C 2 \
'(create\s+(or\s+replace\s+)?function|create\s+trigger|deletion_timestamp|for\s+share|foreign\s+key|does not exist|has been deleted|not_in_use|_ref)' \
|| true
printf '%s\n' '== Trigger and soft-delete conditions =='
rg -n -i -C 4 \
'(create\s+trigger|before\s+(insert|update)|after\s+(insert|update)|when\s*\(|deletion_timestamp\s*[!<>=]+|is\s+distinct\s+from)' \
fulfillment-service/internal/database/migrations -g '*.sql' \
|| true
printf '%s\n' '== Reference validation and protection functions =='
rg -n -i -C 5 \
'(for\s+share|for\s+key\s+share|ensure_.*active|check_.*(ref|reference|in_use)|referenc(e|ed)|does not exist|has been deleted|cannot delete)' \
fulfillment-service/internal/database/migrations -g '*.sql' \
|| trueLength of output: 201411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current reference-validation implementation candidates =='
rg -n -i -C 3 \
'(reference validation interceptor|reference.*validat|validate.*reference|Z0002|does not exist or has been deleted)' \
fulfillment-service -g '*.go' \
| head -n 1200 || true
printf '%s\n' '== Candidate Go files =='
fd -e go . fulfillment-service \
| xargs rg -l -i \
'(reference validation interceptor|reference.*validat|validate.*reference)' \
|| true
printf '%s\n' '== Current post-90 inbound database reference triggers =='
rg -n -i -C 3 \
'(create\s+(or\s+replace\s+)?function.*(ref|reference)|create\s+trigger.*(ref|reference)|execute function.*(ref|reference)|errcode\s*=\s*''Z0002'')' \
fulfillment-service/internal/database/migrations \
-g '*.up.sql' \
| awk '
/migrations\/9[1-9]_/ || /migrations\/[1-9][0-9][0-9]_/ { print }
' || trueLength of output: 141996
@sk-ilya, yes. The repository has several earlier examples of bidirectional soft-delete reference protection:
- Migration 52:
Subnet↔ComputeInstance - Migration 55:
VirtualNetwork↔SubnetandSecurityGroup - Migration 56:
InstanceType↔ComputeInstance - Migration 59: catalog items ↔
ClusterandComputeInstance - Migration 73:
NATGateway→VirtualNetwork - Migration 76:
StorageTier→StorageBackend - Migration 81:
ClusterVersion.allowed_upgrades→ClusterVersion - Migration 87:
Cluster→Subnet
Migration 56 validates on every update of an active ComputeInstance. It would validate a restore.
Migration 59 and migration 81 used the same “validate only when the reference changes” pattern as this PR. Those historical definitions also do not validate a restore when the reference is unchanged.
However, migration 90 removed those inbound database triggers. The current ReferenceValidator gRPC interceptor validates all Create and Update requests. It therefore validates a restore through the API.
Migration 93 is the only current inbound database trigger after migration 90. A direct SQL restore bypasses the gRPC interceptor. Therefore, the reported condition matters only if this migration intends to enforce the invariant for direct database writes, which its DAO-level trigger tests suggest.
You are interacting with an AI system.
2ce109d to
94831cc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
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
`@fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql`:
- Around line 27-29: Move the clusters version-name index creation out of the
current migration into a standalone migration that runs CREATE INDEX
CONCURRENTLY, preserving the existing expression and non-null predicate. Ensure
the original migration no longer creates this index.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1bb1abae-9b4b-4665-8353-b9a733295aa1
📒 Files selected for processing (4)
fulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sqlfulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger_test.gofulfillment-service/internal/servers/private_cluster_versions_server_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- fulfillment-service/internal/database/migrations.sha256
- fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger_test.go
- fulfillment-service/internal/servers/private_cluster_versions_server_test.go
| create index clusters_version_name on clusters | ||
| ((data->'spec'->>'version_name')) | ||
| where data->'spec'->>'version_name' is not null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find migration-runner configuration and existing concurrent-index patterns.
rg -n -C 3 --glob '*.{go,sql,yaml,yml,md}' \
'CREATE INDEX CONCURRENTLY|NoTransaction|transaction|migrations' \
fulfillment-serviceRepository: osac-project/osac
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration ---'
cat -n fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql
printf '%s\n' '--- migration tool references ---'
rg -n -C 4 --glob '*.go' \
'dbTool|Migrate\(|golang-migrate|migrate\.New|New.*Migration|migration' \
fulfillment-service/internal/cmd fulfillment-service/internal/database | \
grep -E 'dbTool|Migrate\(|golang-migrate|migrate\.New|New.*Migration|migration' | head -250
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 \
'golang-migrate|migration|migrate' \
fulfillment-service/go.mod fulfillment-service/go.sum fulfillment-service/internal/databaseRepository: osac-project/osac
Length of output: 36835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration command implementation ---'
cat -n fulfillment-service/internal/cmd/service/migrate/migrate_cmd.go | sed -n '1,110p'
cat -n fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go | sed -n '245,275p'
printf '%s\n' '--- database migration package files ---'
git ls-files fulfillment-service/internal/database | grep -Ei 'migrat|database_tool|db_tool'
rg -n -C 5 --glob '*.go' \
'type .*Tool|func .*Migrate|New.*Tool|Set.*Migration|migrate\.New|iofs|source\.iofs' \
fulfillment-service/internal/database fulfillment-service/internal/cmd | head -300
printf '%s\n' '--- migration dependency context ---'
grep -n -C 4 -E 'migrate|postgres' fulfillment-service/go.modRepository: osac-project/osac
Length of output: 44980
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all migration API symbols in the repository ---'
rg -n --glob '!**/vendor/**' --glob '!**/node_modules/**' \
'Migrate\(|migrate\.|migration' fulfillment-service | \
grep -E 'Migrate\(|migrate\.|migration' | head -400
printf '%s\n' '--- module dependency entries ---'
grep -n -E 'migrate|postgres|pgx' fulfillment-service/go.mod fulfillment-service/go.sum | head -120
printf '%s\n' '--- migration directory metadata and nearby migrations ---'
find fulfillment-service/internal/database/migrations -maxdepth 1 -type f -printf '%f\n' | sort -V | tail -20
for f in $(find fulfillment-service/internal/database/migrations -maxdepth 1 -type f -name '*.up.sql' | sort -V | tail -5); do
echo "--- $f"
sed -n '1,80p' "$f"
doneRepository: osac-project/osac
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve the migration runner without executing repository code.
printf '%s\n' '--- migration runner declarations ---'
rg -n -C 8 --glob '*.go' \
'dbTool|DatabaseTool|Migrate|Migration' \
fulfillment-service/internal/cmd/service fulfillment-service/internal/database | \
grep -E 'dbTool|DatabaseTool|Migrate|Migration' | head -220
printf '%s\n' '--- migration library in module files ---'
grep -n -E 'migrate|migration' fulfillment-service/go.mod fulfillment-service/go.sum || true
printf '%s\n' '--- index statements in migrations ---'
rg -n -C 2 --glob '*.sql' \
'CREATE( UNIQUE)? INDEX|create( unique)? index' \
fulfillment-service/internal/database/migrationsRepository: osac-project/osac
Length of output: 29212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- imports and command setup ---'
sed -n '1,90p' fulfillment-service/internal/cmd/service/migrate/migrate_cmd.go
sed -n '1,90p' fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
sed -n '1,120p' fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go | grep -n -C 3 -E 'database|migrat'
printf '%s\n' '--- exact module migration references ---'
grep -n -i -C 2 'migrat' fulfillment-service/go.mod || true
grep -n -i -C 2 'migrat' fulfillment-service/go.sum | head -80 || trueRepository: osac-project/osac
Length of output: 7619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- database tool migration call ---'
sed -n '348,425p' fulfillment-service/internal/database/database_tool.go
printf '%s\n' '--- local Go module cache for golang-migrate ---'
gopath="$(go env GOPATH 2>/dev/null || true)"
if [ -n "$gopath" ] && [ -d "$gopath/pkg/mod" ]; then
find "$gopath/pkg/mod" -path '*golang-migrate*migrate*' -maxdepth 6 -type f \
\( -path '*database/postgres*' -o -path '*database/pgx*' -o -name 'postgres.go' -o -name 'pgx.go' \) \
-print | head -40
else
echo 'Go module cache is unavailable'
fi
printf '%s\n' '--- repository transaction and non-transaction migration markers ---'
rg -n --glob '*.sql' --glob '*.go' \
'StatementBegin|StatementEnd|NoTransaction|DisableTransaction|CREATE INDEX CONCURRENTLY|create index concurrently' \
fulfillment-service || trueRepository: osac-project/osac
Length of output: 2306
🌐 Web query:
golang-migrate migrate/v4 PostgreSQL driver Run transaction CREATE INDEX CONCURRENTLY transaction block
💡 Result:
You cannot execute CREATE INDEX CONCURRENTLY within a PostgreSQL transaction block [1][2]. Because the golang-migrate PostgreSQL driver often executes migration files wrapped in a transaction, any attempt to run this command will result in an error: ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block [3][4]. To resolve this when using golang-migrate, you have several options: 1. Use separate migration files: Put the CREATE INDEX CONCURRENTLY statement in its own migration file, separate from other SQL statements that might trigger a transaction [5][6][7]. 2. Avoid multi-statement mode: If you have enabled multi-statement mode (which executes statements inside a transaction), you must either disable it or ensure your concurrent index creation is isolated from other commands [5][6]. 3. Configuration: In some cases, you may be able to configure the driver to avoid wrapping migrations in transactions, though the recommended approach remains isolating the concurrent operation [5][6]. PostgreSQL mandates this restriction because CREATE INDEX CONCURRENTLY performs multiple table scans and manages its own internal transactions; it cannot function while nested inside an existing user-defined transaction [1][4]. If you must include it in a migration, it must be the only command in that migration and executed without a surrounding transaction block [5][4].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-createindex.html
- 2: https://www.postgresql.org/docs/19/sql-createindex.html
- 3: https://pgfence.com/blog/concurrently-in-a-transaction-silent-footgun/
- 4: https://thesev1database.com/errors/msg-create-index-concurrently-cannot-run-inside-a-transaction-block/
- 5: https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md
- 6: Support multi-statement execution for PostgreSQL golang-migrate/migrate#495
- 7: https://github.com/golang-migrate/migrate/tree/master/database/postgres
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/golang-migrate/migrate/v4.19.1/database/postgres/postgres.go'
curl --fail --silent --show-error "$url" |
sed -n '/func (p \*Postgres) Run/,/func (p \*Postgres) SetVersion/p'Repository: osac-project/osac
Length of output: 2523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import urllib.request
url = "https://api.github.com/repos/golang-migrate/migrate/contents/database/postgres/postgres.go?ref=v4.19.1"
with urllib.request.urlopen(url) as response:
payload = json.load(response)
source = __import__("base64").b64decode(payload["content"]).decode()
lines = source.splitlines()
for i, line in enumerate(lines):
if "func (p *Postgres) Run" in line:
for n in range(i, min(i + 70, len(lines))):
print(f"{n+1:4}: {lines[n]}")
break
PYRepository: osac-project/osac
Length of output: 2441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
base='https://raw.githubusercontent.com/golang-migrate/migrate/v4.19.1/database/pgx/v5'
for file in pgx5.go pgx.go; do
echo "--- $file ---"
if curl --fail --silent --show-error "$base/$file" > "/tmp/$file"; then
grep -n -A45 -B5 -E 'func \(p \*|func \(.*\) Run|MultiStatementEnabled|runStatement|Config' "/tmp/$file" | head -240
else
echo "not found"
fi
done
printf '%s\n' '--- repository migration driver construction ---'
rg -n -C 5 \
'database/pgx/v5|NewWithSourceInstance|MultiStatementEnabled|multi_statement|StatementTimeout' \
fulfillment-service/internal/database fulfillment-service/go.modRepository: osac-project/osac
Length of output: 8038
Move the index to a standalone concurrent migration.
When startup migrates a live database, regular CREATE INDEX blocks writes on clusters. Place only CREATE INDEX CONCURRENTLY in a separate migration file, or schedule a write outage.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 27-29: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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
`@fulfillment-service/internal/database/migrations/93_add_cluster_version_delete_protection_trigger.up.sql`
around lines 27 - 29, Move the clusters version-name index creation out of the
current migration into a standalone migration that runs CREATE INDEX
CONCURRENTLY, preserving the existing expression and non-null predicate. Ensure
the original migration no longer creates this index.
Source: Linters/SAST tools
94831cc to
e5745ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger_test.go`:
- Around line 51-55: Update the softDelete helper to remove table-name SQL
concatenation by selecting fixed, parameterized UPDATE statements for each
supported table. Keep binding id as the $1 parameter and preserve the existing
error assertion; reject or handle unsupported table values explicitly.
In
`@fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger.up.sql`:
- Around line 22-24: Update the migration around the clusters_version index to
build it with CREATE INDEX CONCURRENTLY and ensure it runs without
golang-migrate transaction wrapping via x-no-tx-wrap=true. Since the same
migration creates triggers, move the concurrent index creation into a separate
migration or deployment step while preserving the existing index definition.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95d68666-616a-42bf-a28c-5f72c0770e3c
📒 Files selected for processing (4)
fulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger.up.sqlfulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger_test.gofulfillment-service/internal/servers/private_cluster_versions_server_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- fulfillment-service/internal/database/migrations.sha256
- fulfillment-service/internal/servers/private_cluster_versions_server_test.go
| softDelete := func(ctx context.Context, table, id string) { | ||
| _, err := conn.Exec(ctx, | ||
| `update `+table+` set deletion_timestamp = now() where id = $1`, id) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove dynamic SQL construction from softDelete.
Line 53 concatenates table into the SQL statement. The current callers use literals, but this helper accepts an unconstrained identifier. Use fixed statements for the supported tables, then continue to bind id as a parameter.
As per coding guidelines, “Flag SQL string concatenation.” As per path instructions, “SQL: parameterized queries only; no string concatenation.”
Proposed fix
-softDelete := func(ctx context.Context, table, id string) {
+softDelete := func(ctx context.Context, statement, id string) {
_, err := conn.Exec(ctx,
- `update `+table+` set deletion_timestamp = now() where id = $1`, id)
+ statement, id)
Expect(err).ToNot(HaveOccurred())
}
...
-softDelete(ctx, "clusters", "cluster-2")
+softDelete(ctx, `update clusters set deletion_timestamp = now() where id = $1`, "cluster-2")
...
-softDelete(ctx, "cluster_templates", "template-2")
+softDelete(ctx, `update cluster_templates set deletion_timestamp = now() where id = $1`, "template-2")
...
-softDelete(ctx, "cluster_catalog_items", "cci-2")
+softDelete(ctx, `update cluster_catalog_items set deletion_timestamp = now() where id = $1`, "cci-2")🤖 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
`@fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger_test.go`
around lines 51 - 55, Update the softDelete helper to remove table-name SQL
concatenation by selecting fixed, parameterized UPDATE statements for each
supported table. Keep binding id as the $1 parameter and preserve the existing
error assertion; reject or handle unsupported table values explicitly.
Sources: Coding guidelines, Path instructions
| create index clusters_version on clusters | ||
| ((data->'spec'->'version'->>'name')) | ||
| where data->'spec'->'version'->>'name' is not null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the migration executor and its transaction handling.
rg -n -C 5 --glob '*.go' 'func .*Migrate|\.Migrate\(|Begin\(|BeginTx|Transaction|transaction' fulfillment-service/internal/database
# Find established handling for concurrent index creation.
rg -n -C 4 --glob '*.sql' 'CREATE INDEX CONCURRENTLY|CREATE INDEX' fulfillment-service/internal/database/migrationsRepository: osac-project/osac
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration-related Go files ---'
rg -l --glob '*.go' 'func .*Migrate|Migrate\(|golang-migrate|goose|tern|migration' fulfillment-service/internal/database | head -80
printf '%s\n' '--- migration executor definitions and transaction handling ---'
rg -n -C 8 --glob '*.go' 'func [A-Za-z0-9_]*Migrate|type .*Migration|BeginTx|Begin\(|Commit\(|Rollback\(|RunMigrations|Migrate\(' fulfillment-service/internal/database --glob '!**/*_test.go' | head -300
printf '%s\n' '--- index statements in migration SQL ---'
rg -n --glob '*.sql' -i 'create[[:space:]]+(unique[[:space:]]+)?index([[:space:]]+concurrently)?' fulfillment-service/internal/database/migrations | head -200
printf '%s\n' '--- target migration ---'
cat -n fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger.up.sql | sed -n '1,120p'Repository: osac-project/osac
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration imports and implementation ---'
sed -n '1,80p' fulfillment-service/internal/database/database_tool.go
sed -n '350,430p' fulfillment-service/internal/database/database_tool.go
printf '%s\n' '--- module dependency ---'
rg -n -C 2 'golang-migrate|migrate' go.mod fulfillment-service/go.mod fulfillment-service/go.sum 2>/dev/null | head -100
printf '%s\n' '--- migration file headers and transaction directives ---'
rg -n -i --glob '*.sql' 'transaction|no.?tx|concurrently' fulfillment-service/internal/database/migrations | head -100
printf '%s\n' '--- migration test helpers and setup ---'
sed -n '1,180p' fulfillment-service/internal/database/migrations/migrations_suite_test.go
sed -n '1,180p' fulfillment-service/internal/database/database_migrations_test.goRepository: osac-project/osac
Length of output: 5668
🌐 Web query:
golang-migrate migrate v4.19.1 PostgreSQL CREATE INDEX CONCURRENTLY transaction no transaction option
💡 Result:
To perform a CREATE INDEX CONCURRENTLY operation in PostgreSQL using golang-migrate, you must avoid running the statement within an implicit transaction, as PostgreSQL forbids concurrent index creation inside transaction blocks [1][2][3]. You can achieve this in golang-migrate by disabling the implicit transaction wrapping for your migration. Use the x-no-tx-wrap query parameter in your database connection string [4]: postgres://user:password@hostname:port/db_name?sslmode=disable&x-no-tx-wrap=true If you are configuring the driver programmatically using the postgres.Config struct, you can set the NoTxWrap option to true [4]: import ( "github.com/golang-migrate/migrate/v4/database/postgres") //... inside your configuration driver, err:= postgres.WithInstance(db, &postgres.Config{ NoTxWrap: true, }) When x-no-tx-wrap or NoTxWrap is enabled, the driver will not wrap the migration in a transaction [4]. Note that this shifts the responsibility to you for handling partial migrations or cleanup if a migration fails, as there will be no automatic rollback [4]. If you are unable to disable transactions globally, an alternative approach recommended in the official documentation is to place the CREATE INDEX CONCURRENTLY statement into its own separate migration file, independent of other SQL statements that might trigger transaction wrapping [1][2].
Citations:
- 1: https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md
- 2: https://github.com/golang-migrate/migrate/tree/master/database/postgres
- 3: Support creating indexes concurrently in postgresql golang-migrate/migrate#137
- 4: https://docsearch.algolia.com/mcp/docs/repo/golang-migrate/migrate
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration test files ---'
fd -t f 'migrations.*_test.go|database_migrations_test.go' fulfillment-service/internal/database fulfillment-service/internal/database/migrations
printf '%s\n' '--- migration test setup ---'
for f in $(fd -t f 'migrations.*_test.go|database_migrations_test.go' fulfillment-service/internal/database fulfillment-service/internal/database/migrations | head -5); do
echo "--- $f"
sed -n '1,180p' "$f"
done
printf '%s\n' '--- migration URL options and driver configuration ---'
rg -n -C 5 'pgx5|migrateURL|x-|NoTx|no.?transaction|WithInstance|NewWith' fulfillment-service/internal/database fulfillment-service/go.modRepository: osac-project/osac
Length of output: 27280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
sql = Path("fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger.up.sql").read_text()
go = Path("fulfillment-service/internal/database/database_tool.go").read_text()
index = re.search(r"(?im)^\s*create\s+index\b[^\n]*", sql)
print("target_index:", index.group(0).strip() if index else "missing")
print("uses_concurrently:", bool(re.search(r"(?i)\bcreate\s+index\s+concurrently\b", sql)))
print("has_following_trigger_sql:", bool(re.search(r"(?i)\bcreate\s+(or\s+replace\s+)?function\b|\bcreate\s+trigger\b", sql)))
print("uses_golang_migrate:", "github.com/golang-migrate/migrate/v4" in go)
print("rewrites_url_scheme_to_pgx5:", 'parsed.Scheme = "pgx5"' in go)
print("configures_no_tx_wrap:", bool(re.search(r"(?i)(x-no-tx-wrap|NoTxWrap)", go)))
PYRepository: osac-project/osac
Length of output: 357
Avoid blocking writes during the index build.
If production writes can overlap this migration, use CREATE INDEX CONCURRENTLY without golang-migrate transaction wrapping (x-no-tx-wrap=true). Because this migration also creates triggers, place the index creation in a separate migration or deployment step.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 22-24: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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
`@fulfillment-service/internal/database/migrations/94_add_cluster_version_delete_protection_trigger.up.sql`
around lines 22 - 24, Update the migration around the clusters_version index to
build it with CREATE INDEX CONCURRENTLY and ensure it runs without
golang-migrate transaction wrapping via x-no-tx-wrap=true. Since the same
migration creates triggers, move the concurrent index creation into a separate
migration or deployment step while preserving the existing index definition.
Source: Linters/SAST tools
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Clean, focused change — the Z0003 trigger follows the established pattern, exists(select 1 ...) avoids leaking referencing IDs (consistent with migration 72), and the tests cover all three referencing resource types at both migration and server layers.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 2 |
| 💡 Suggestion | 1 |
🟡 PR description is stale
The description mentions "inbound triggers (Z0002) with FOR SHARE locking", ensure_active_cluster_version(), check_cluster_version_ref(), and "23 specs" — none of which exist in the current diff. The actual PR contains only the outbound Z0003 trigger and 10 test specs (7 migration + 3 server). Should be updated to match the actual scope.
| -- This migration adds a BEFORE UPDATE trigger on cluster_versions that prevents soft-deleting a version while an | ||
| -- active cluster, cluster template, or cluster catalog item still references it. Inbound (forward-reference) | ||
| -- validation is handled by the gRPC reference validation interceptor, not by database triggers. | ||
| -- |
There was a problem hiding this comment.
🟡 Inbound reference validation — does it exist?
This comment states inbound validation is handled by the gRPC reference validation interceptor. However, I couldn't find a ClusterVersionReference lookup registered with the ReferenceValidator in the gRPC server setup (start_grpc_server_cmd.go:436). The validator is created but .Register() is never called for any reference type in production code.
Without inbound validation, users could create clusters/templates referencing non-existent or deleted cluster versions — the outbound protection would be one-sided.
The established pattern (migrations 56, 76, 81) uses bidirectional DB triggers with FOR SHARE locking to eliminate TOCTOU races. Is the inbound side handled elsewhere (separate PR, server-level check), or is this a known gap?
| ((data->'spec'->'version'->>'name')) | ||
| where data->'spec'->'version'->>'name' is not null; | ||
|
|
||
| -- Trigger function that checks whether any active cluster, cluster template, or cluster catalog item references the |
There was a problem hiding this comment.
💡 Consider adding expression index for cluster_templates
The migration indexes the clusters table lookup but not cluster_templates (on data->'spec_defaults'->'version'->>'name'). While version deletion is rare and the table is small, adding the index would be consistent. The cluster_catalog_items lateral unnest can't be served by a plain expression index, so that one is fine without.
There was a problem hiding this comment.
I don't think adding the index would necessarily make this more consistent, since, as you mentioned, cluster_catalog_items doesn't have one either. Given how small cluster_templates is, I don't think there is much benefit in adding an index here.
e5745ec to
db9bdc4
Compare
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Re-review - PR has been significantly reworked since my last review.
Previous Findings Status
| # | Finding | Status | Reason |
|---|---|---|---|
| 1 | 🟡 Inbound reference validation - does it exist? | ACCEPTED | PR #183 added ClusterVersionReference lookup to reference_lookups.go, now merged. PR description correctly updated. |
| 2 | 💡 Expression index for cluster_templates | ACCEPTED | Author's reasoning is sound - table is small, consistent with cluster_catalog_items also being unindexed. |
New Issues
None. The reworked PR correctly narrows scope to outbound-only Z0003 trigger. Trigger function follows established patterns, JSONB path matching is correct, catalog item array scan handles edge cases, and the test coverage (7 migration + 3 server specs) is comprehensive.
/lgtm
Assisted-by: Claude <noreply@anthropic.com>
db9bdc4 to
c29405f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omer-vishlitzky, sk-ilya, tzvatot 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 |
4 participants
Summary
cluster_versionsthat blocks soft-deletion when active clusters, templates, or catalog items reference the version by name.Test plan
Assisted-by: Claude noreply@anthropic.com