Skip to content

Validate the uniqueness of pipeline variables

Marius Bobin requested to merge ci-validate-uniquness-of-pipeline-variables into master

What does this MR do?

Prevents Projects::PipelinesController#create from returning 500 errors when there are duplicate pipeline variables: https://log.gprd.gitlab.net/goto/2998692616fc89c188b46ff726630523

We have a uniqueness validation on the pipeline variables model, but that doesn't work because the validations are executed on all the records before we start inserting them:

[8] pry(main)> pipeline = Ci::Pipeline.last.dup; nil
  Ci::Pipeline Load (0.5ms)  SELECT "ci_pipelines".* FROM "ci_pipelines" ORDER BY "ci_pipelines"."id" DESC LIMIT 1 
=> nil
[9] pry(main)> pipeline.iid = nil
=> nil
[10] pry(main)> pipeline.variables_attributes = [{ key: 'a', value: 'a' }, { key: 'a', value: 'b' }]
=> [{:key=>"a", :value=>"a"}, {:key=>"a", :value=>"b"}]
[11] pry(main)> pipeline.save
  TRANSACTION (0.2ms)  BEGIN
  Ci::PipelineVariable Exists? (0.3ms)  SELECT 1 AS one FROM "ci_pipeline_variables" WHERE "ci_pipeline_variables"."key" = 'a' AND "ci_pipeline_variables"."pipeline_id" IS NULL LIMIT 1
  Ci::PipelineVariable Exists? (0.3ms)  SELECT 1 AS one FROM "ci_pipeline_variables" WHERE "ci_pipeline_variables"."key" = 'a' AND "ci_pipeline_variables"."pipeline_id" IS NULL LIMIT 1
  Project Load (0.8ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 13 LIMIT 1
  Feature::FlipperGate Load (2.8ms)  SELECT "feature_gates".* FROM "feature_gates" WHERE "feature_gates"."feature_key" = 'generate_iids_without_explicit_locking'
  InternalId Load (6.7ms)  SELECT "internal_ids".* FROM "internal_ids" WHERE "internal_ids"."project_id" = 13 AND "internal_ids"."usage" = 5 LIMIT 1 
  InternalId Load (0.3ms)  SELECT "internal_ids".* FROM "internal_ids" WHERE "internal_ids"."id" = 23 LIMIT 1 FOR UPDATE 
  InternalId Update (0.3ms)  UPDATE "internal_ids" SET "last_value" = 9 WHERE "internal_ids"."id" = 23 
  Ci::Pipeline Create (4.9ms)  INSERT INTO "ci_pipelines" ("ref", "sha", "before_sha", "created_at", "updated_at", "project_id", "status", "started_at", "finished_at", "duration", "user_id", "lock_version", "source", "config_source", "protected", "iid", "ci_ref_id") VALUES ('main', 'f72367b97387a8f1b7b381a3521f699d83708f56', '0000000000000000000000000000000000000000', '2021-08-09 13:21:41.844206', '2021-08-09 13:21:41.844206', 13, 'failed', '2021-07-22 12:47:43.206252', '2021-07-22 12:49:11.598515', 40, 1, 5, 2, 1, TRUE, 9, 4) RETURNING "id" 
  Ci::PipelineVariable Create (1.5ms)  INSERT INTO "ci_pipeline_variables" ("key", "value", "encrypted_value", "encrypted_value_salt", "encrypted_value_iv", "pipeline_id") VALUES ('a', NULL, 'l/M+tii2dBlbyKHqO9Ug9g==
', '_CCbDl3MfR5AvEyv6/ByjTg==
', 'kZsukkHfZNHe/T9N8AvuIg==
', 36) RETURNING "id"
  Ci::PipelineVariable Create (0.5ms)  INSERT INTO "ci_pipeline_variables" ("key", "value", "encrypted_value", "encrypted_value_salt", "encrypted_value_iv", "pipeline_id") VALUES ('a', NULL, 'HL0/vjvMoDj11wY+UGJLVw==
', '_qH/yOP0PY8Dx+7hK4a3zrg==
', 'o6WYLvZxHoQ4P1HdnA6+5w==
', 36) RETURNING "id" 
  TRANSACTION (0.2ms)  ROLLBACK
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_ci_pipeline_variables_on_pipeline_id_and_key"
DETAIL:  Key (pipeline_id, key)=(36, a) already exists.

from /Users/marius/.gem/ruby/2.7.2/gems/activerecord-6.1.3.2/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `exec_params'
Caused by PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_ci_pipeline_variables_on_pipeline_id_and_key"
DETAIL:  Key (pipeline_id, key)=(36, a) already exists.

from /Users/marius/.gem/ruby/2.7.2/gems/activerecord-6.1.3.2/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `exec_params'

Screenshots or Screencasts (strongly suggested)

image

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 Marius Bobin

Merge request reports