-
Notifications
You must be signed in to change notification settings - Fork 85
[HCMPRE-2735] Fixed schema migration for encryption service #744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughA SQL migration script defining tables for symmetric and asymmetric encryption key management was deleted and replaced with a new migration script that recreates these tables with the same schema and constraints. Additionally, a Flyway migration fix was introduced to remove hardcoded references to the Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as DB Migration Tool
participant DB as Database
Admin->>DB: Drop eg_enc_symmetric_keys if exists
Admin->>DB: Drop eg_enc_asymmetric_keys if exists
Admin->>DB: Create eg_enc_symmetric_keys table
Admin->>DB: Create unique index on symmetric key_id
Admin->>DB: Create partial unique index on tenant_id where active is true
Admin->>DB: Create eg_enc_asymmetric_keys table
Admin->>DB: Create unique index on asymmetric key_id
Admin->>DB: Create partial unique index on tenant_id where active is true
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core-services/egov-enc-service/src/main/resources/db/migration/main/V20250519185601__eg_enc.sql (4)
1-10
: Consider strengthening theeg_enc_symmetric_keys
table schema
The current DDL is functional but could be enhanced for data integrity and future scalability:
- Use
BIGSERIAL
instead ofSERIAL
forid
to avoid integer overflow on large key volumes.- Add
created_at
/updated_at
timestamp columns for auditing key lifecycle changes.- If you have a tenants table, add a foreign key constraint on
tenant_id
to enforce referential integrity.Proposed diff for this segment (please confirm the actual tenants table and PK column name before applying):
--- a/V20250519185601__eg_enc.sql +++ b/V20250519185601__eg_enc.sql -CREATE TABLE IF NOT EXISTS eg_enc_symmetric_keys -( - id SERIAL, - key_id integer NOT NULL, - secret_key text NOT NULL, - initial_vector text NOT NULL, - active boolean NOT NULL DEFAULT true, - tenant_id text NOT NULL, - PRIMARY KEY (id) -); +CREATE TABLE IF NOT EXISTS eg_enc_symmetric_keys +( + id BIGSERIAL PRIMARY KEY, + key_id INTEGER NOT NULL, + secret_key TEXT NOT NULL, + initial_vector TEXT NOT NULL, + active BOOLEAN NOT NULL DEFAULT true, + tenant_id TEXT NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + CONSTRAINT fk_enc_sym_tenant FOREIGN KEY (tenant_id) + REFERENCES tenant(tenant_id) ON DELETE CASCADE +);Please verify the target table and column for the FK and adjust names accordingly.
12-13
: Consider using concurrent index creation for large tables
If these tables already contain a significant number of rows in production, creating unique indexes withoutCONCURRENTLY
will block writes. To minimize downtime, consider:CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS eg_symmetric_key_id ON eg_enc_symmetric_keys (key_id); CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS active_tenant_symmetric_keys ON eg_enc_symmetric_keys (tenant_id) WHERE active IS TRUE;Note: concurrent index creation must run outside of an explicit transaction; confirm your Flyway configuration supports this.
15-24
: Apply similar table enhancements toeg_enc_asymmetric_keys
For consistency and auditability, mirror the suggestions above on the asymmetric key table:
- Switch to
BIGSERIAL
.- Add
created_at
/updated_at
.- Enforce a foreign key on
tenant_id
.Example diff:
--- a/V20250519185601__eg_enc.sql +++ b/V20250519185601__eg_enc.sql -CREATE TABLE IF NOT EXISTS eg_enc_asymmetric_keys -( - id SERIAL, - key_id integer NOT NULL, - public_key text NOT NULL, - private_key text NOT NULL, - active boolean NOT NULL DEFAULT true, - tenant_id text NOT NULL, - PRIMARY KEY (id) -); +CREATE TABLE IF NOT EXISTS eg_enc_asymmetric_keys +( + id BIGSERIAL PRIMARY KEY, + key_id INTEGER NOT NULL, + public_key TEXT NOT NULL, + private_key TEXT NOT NULL, + active BOOLEAN NOT NULL DEFAULT true, + tenant_id TEXT NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + CONSTRAINT fk_enc_asym_tenant FOREIGN KEY (tenant_id) + REFERENCES tenant(tenant_id) ON DELETE CASCADE +);
26-27
: Also use concurrent index creation for asymmetric keys
Apply the same index strategy here to avoid write locks on theeg_enc_asymmetric_keys
table:CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS eg_asymmetric_key_id ON eg_enc_asymmetric_keys (key_id); CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS active_tenant_asymmetric_keys ON eg_enc_asymmetric_keys (tenant_id) WHERE active IS TRUE;Ensure your migration runner allows non-transactional scripts when using
CONCURRENTLY
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
core-services/egov-enc-service/src/main/resources/db/migration/main/V20180607185601__eg_enc.sql
(0 hunks)core-services/egov-enc-service/src/main/resources/db/migration/main/V20250519185601__eg_enc.sql
(1 hunks)
💤 Files with no reviewable changes (1)
- core-services/egov-enc-service/src/main/resources/db/migration/main/V20180607185601__eg_enc.sql
Summary by CodeRabbit
Issue & Resolution Summary
To fix flyway migration issue with new breaking change -
Tables - egov_enc_service_schema
Steps -
egov_enc_service
using devops environment variableSCHEMA_TABLE
for the service. ie.egov_enc_service_schema
SELECT * FROM public.egov_enc_service_schema
DELETE FROM public.egov_enc_service_schema
Why it needs to be done?
What if you deploy this in production environment?
CrashLoopBackOff
due to DB migration failure.Summary by CodeRabbit