Skip to content

2PC on CI branch

Adam Hegyi requested to merge ci-split-transaction-test into rails-6-1-ci-split

What does this MR do?

Proof of concept. This MR tries to detect cross-database writes within a transaction during CI test run and raise errors.

Example: INSERT, UPDATE or DELETE query is triggered for both ci_* and non-ci tables.

Issue.transaction do
  Issue.create(...)
  Ci::Pipeline.create(...)
end

How does it work?

In the spec_helper.rb class, we have the following option: use_transactional_fixtures = true. This makes each test case run its own transaction. During the test run, a test case can be wrapped in multiple transactions (using savepoints):

  1. Transaction created by use_transactional_fixtures
  2. Transaction created by let_it_be
  3. Test case is running
  4. Application code may create transactions (savepoints), this is what we're looking for
  5. Rollback

Note: this works for schemas (single DB), for multiple databases, we'll need a slightly different logic (track savepoints per DB)

The TransactionTracker functionality does the following for each test case:

  1. Detect the point where the savepoints created by the test frameworks are finished.
  2. Start collecting the created savepoints.
  3. Collect UPDATE, INSERT, DELETE DB queries and extract the referenced DB tables.
  4. Check for offenses.

The tracking is disabled for the following cases:

  • InternalId
  • FactoryBot record creation

Example from the log:

  TRANSACTION (0.1ms)  SAVEPOINT active_record_3 /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
  ↳ config/initializers/00_rails_disable_joins.rb:17:in `scope'
-- Savepoint created, name: active_record_3
  ProjectMember Destroy (0.1ms)  DELETE FROM "members" WHERE "members"."type" = 'ProjectMember' AND "members"."source_id" = 2 AND "members"."source_type" = 'Project' AND "members"."requested_at" IS NOT NULL /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
  ↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
DB accessed with savepoint(active_record_3): main
  NotificationSetting Destroy (0.1ms)  DELETE FROM "notification_settings" WHERE "notification_settings"."source_id" = 2 AND "notification_settings"."source_type" = 'Project' /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
  ↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
  ContainerRepository Load (0.2ms)  SELECT "container_repositories".* FROM "container_repositories" WHERE "container_repositories"."project_id" = 2 /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
  ↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
DB accessed with savepoint(active_record_3): ci
  Ci::Pipeline Destroy (0.7ms)  DELETE FROM "ci_pipelines" WHERE "ci_pipelines"."project_id" = 2 AND ("ci_pipelines"."source" IN (1, 2, 3, 4, 5, 6, 7, 8, 10, 11) OR "ci_pipelines"."source" IS NULL) /*application:test,correlation_id:662b936c6988c202435bce3ee6f2d79e*/
  ↳ spec/models/project_spec.rb:3265:in `block (4 levels) in <top (required)>'
-- CI table(s) and Main table(s) were referenced within a transaction: ci_pipelines

Screenshots or Screencasts (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Adam Hegyi

Merge request reports