Add raw endpoints for terraform state locking / unlocking

Add lock_id and locked_by to terraform state model

Add locked_at column to terraform states table

Add terraform state name with proper locking columns

Add first version of terraform state handler service

This adds initial version of Terraform state handler that supports
locking, and is going to be used in our Terraform Remote Backend state
API implementation.

Update terraform state migrations

Improve terraform state handler and related specs

Rename terraform lock id column to lock_xid

The rename is consistent with our recent guidelines to avoid using `_id`
suffixes for things that are not relations. It is recommended to use
`_xid` is such situations.

app/models/terraform/state.rb

@@ -3,14 +3,13 @@
 module Terraform
   class State < ApplicationRecord
     belongs_to :project
+    belongs_to :locked_by, class_name: 'User'
 
     validates :project_id, presence: true
 
-    after_save :update_file_store, if: :saved_change_to_file?
-
     mount_uploader :file, StateUploader
 
-    def update_file_store
+    def update_file_store!
       # The file.object_store is set during `uploader.store!`
       # which happens after object is inserted/updated
       self.update_column(:file_store, file.object_store)
@@ -19,5 +18,9 @@ module Terraform
     def file_store
       super || StateUploader.default_store
     end
+
+    def locked?
+      self.lock_xid.present?
+    end
   end
 end

app/services/terraform/remote_state_handler.rb

+# frozen_string_literal: true
+
+module Terraform
+  class RemoteStateHandler < BaseService
+    StateLockedError = Class.new(StandardError)
+
+    def create_or_find!
+      raise ArgumentError unless params[:name].present?
+
+      Terraform::State.create_or_find_by(project: project, name: params[:name])
+    end
+
+    def handle_with_lock
+      retrieve_with_lock do |state|
+        raise StateLockedError unless lock_matches?(state)
+
+        yield state if block_given?
+
+        state.save!
+        state.update_file_store!
+      end
+    end
+
+    def lock!
+      raise ArgumentError if params[:lock_id].blank?
+
+      retrieve_with_lock do |state|
+        raise StateLockedError if state.locked?
+
+        state.lock_xid = params[:lock_id]
+        state.locked_by = current_user
+        state.locked_at = Time.now
+
+        state.save!
+      end
+    end
+
+    def unlock!
+      raise ArgumentError if params[:lock_id].blank?
+
+      retrieve_with_lock do |state|
+        raise StateLockedError unless lock_matches?(state)
+
+        state.lock_xid = nil
+        state.locked_by = nil
+        state.locked_at = nil
+
+        state.save!
+      end
+    end
+
+    private
+
+    def retrieve_with_lock
+      create_or_find!.tap { |state| state.with_lock { yield state } }
+    end
+
+    def lock_matches?(state)
+      return true if state.lock_xid.nil? && params[:lock_id].nil?
+
+      ActiveSupport::SecurityUtils
+        .secure_compare(state.lock_xid.to_s, params[:lock_id].to_s)
+    end
+  end
+end

db/migrate/20200402171949_add_lock_id_to_terraform_state.rb

+# frozen_string_literal: true
+
+class AddLockIdToTerraformState < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :terraform_states, :lock_xid, :string, limit: 255
+    add_column :terraform_states, :locked_at, :datetime_with_timezone
+    add_reference :terraform_states, :locked_by, foreign_key: { to_table: :users } # rubocop:disable Migration/AddReference (table not used yet)
+  end
+end

db/migrate/20200403095403_add_name_to_terraform_state.rb

+# frozen_string_literal: true
+
+class AddNameToTerraformState < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :terraform_states, :name, :string, limit: 255
+    add_index :terraform_states, [:project_id, :name], unique: true # rubocop:disable Migration/AddIndex (table not used yet)
+  end
+end

db/structure.sql

@@ -6134,7 +6134,11 @@ CREATE TABLE public.terraform_states (
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     file_store smallint,
-    file character varying(255)
+    file character varying(255),
+    lock_xid character varying(255),
+    locked_at timestamp with time zone,
+    locked_by_id bigint,
+    name character varying(255)
 );
 
 CREATE SEQUENCE public.terraform_states_id_seq
@@ -10232,8 +10236,12 @@ CREATE INDEX index_term_agreements_on_term_id ON public.term_agreements USING bt
 
 CREATE INDEX index_term_agreements_on_user_id ON public.term_agreements USING btree (user_id);
 
+CREATE INDEX index_terraform_states_on_locked_by_id ON public.terraform_states USING btree (locked_by_id);
+
 CREATE INDEX index_terraform_states_on_project_id ON public.terraform_states USING btree (project_id);
 
+CREATE UNIQUE INDEX index_terraform_states_on_project_id_and_name ON public.terraform_states USING btree (project_id, name);
+
 CREATE INDEX index_timelogs_on_issue_id ON public.timelogs USING btree (issue_id);
 
 CREATE INDEX index_timelogs_on_merge_request_id ON public.timelogs USING btree (merge_request_id);
@@ -11792,6 +11800,9 @@ ALTER TABLE ONLY public.resource_label_events
 ALTER TABLE ONLY public.packages_build_infos
     ADD CONSTRAINT fk_rails_b18868292d FOREIGN KEY (package_id) REFERENCES public.packages_packages(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.terraform_states
+    ADD CONSTRAINT fk_rails_b1c810a8d8 FOREIGN KEY (locked_by_id) REFERENCES public.users(id);
+
 ALTER TABLE ONLY public.merge_trains
     ADD CONSTRAINT fk_rails_b29261ce31 FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE;
 
@@ -13157,7 +13168,9 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200402123926
 20200402124802
 20200402135250
+20200402171949
 20200402185044
+20200403095403
 20200403184110
 20200403185127
 20200403185422

lib/api/terraform/state.rb

@@ -30,13 +30,32 @@ module API
             body 'not implemented'
           end
 
-          desc 'Delete a terraform state of certain name'
+          desc 'Delete a terraform state of a certain name'
           route_setting :authentication, basic_auth_personal_access_token: true
           delete do
             status 501
             content_type 'text/plain'
             body 'not implemented'
           end
+
+          desc 'Lock a terraform state of a certain name'
+          route_setting :authentication, basic_auth_personal_access_token: true
+          params do
+            optional :ID, type: String, desc: 'Terraform state lock ID'
+          end
+          put '/lock' do
+            status 501
+            content_type 'text/plain'
+            body 'LOCK not implemented'
+          end
+
+          desc 'Unlock a terraform state of a certain name'
+          route_setting :authentication, basic_auth_personal_access_token: true
+          delete '/lock' do
+            status 501
+            content_type 'text/plain'
+            body 'UNLOCK not implemented'
+          end
         end
       end
     end

spec/requests/api/terraform/state_spec.rb

@@ -98,4 +98,22 @@ describe API::Terraform::State do
       expect(response).to have_gitlab_http_status(:forbidden)
     end
   end
+
+  describe 'PUT /projects/:id/terraform/state/:name/lock' do
+    it 'locks the terraform state' do
+      put api("/projects/#{project.id}/terraform/state/#{state_name}/lock?ID=123-456"), headers: auth_header_for(maintainer)
+
+      expect(response).to have_gitlab_http_status(:not_implemented)
+      expect(response.body).to include('LOCK not implemented')
+    end
+  end
+
+  describe 'DELETE /projects/:id/terraform/state/:name/lock' do
+    it 'remove the terraform state lock' do
+      delete api("/projects/#{project.id}/terraform/state/#{state_name}/lock?ID=123-456"), headers: auth_header_for(maintainer)
+
+      expect(response).to have_gitlab_http_status(:not_implemented)
+      expect(response.body).to include('LOCK not implemented')
+    end
+  end
 end

spec/services/terraform/remote_state_handler_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Terraform::RemoteStateHandler do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  describe '#create_or_find!' do
+    it 'requires passing a state name' do
+      handler = described_class.new(project, user)
+
+      expect { handler.create_or_find! }.to raise_error(ArgumentError)
+    end
+
+    it 'allows to create states with same name in different projects' do
+      project_b =  create(:project)
+
+      state_a = described_class.new(project, user, name: 'my-state').create_or_find!
+      state_b = described_class.new(project_b, user, name: 'my-state').create_or_find!
+
+      expect(state_a).to be_persisted
+      expect(state_b).to be_persisted
+      expect(state_a.id).not_to eq state_b.id
+    end
+
+    it 'loads the same state upon subsequent call in the project scope' do
+      state_a = described_class.new(project, user, name: 'my-state').create_or_find!
+      state_b = described_class.new(project, user, name: 'my-state').create_or_find!
+
+      expect(state_a).to be_persisted
+      expect(state_a.id).to eq state_b.id
+    end
+  end
+
+  context 'when state locking is not being used' do
+    subject { described_class.new(project, user, name: 'my-state') }
+
+    describe '#handle_with_lock' do
+      it 'allows to modify a state using database locking' do
+        state = subject.handle_with_lock do |state|
+          state.name = 'updated-name'
+        end
+
+        expect(state.name).to eq 'updated-name'
+      end
+
+      it 'returns the state object itself' do
+        state = subject.create_or_find!
+
+        expect(state.name).to eq 'my-state'
+      end
+    end
+
+    describe '#lock!' do
+      it 'raises an error' do
+        expect { subject.lock! }.to raise_error(ArgumentError)
+      end
+    end
+
+    describe '#unlock!' do
+      it 'raises an error' do
+        expect { subject.unlock! }.to raise_error(ArgumentError)
+      end
+    end
+  end
+
+  context 'when using locking' do
+    describe '#handle_with_lock' do
+      it 'handles a locked state using exclusive read lock' do
+        handler = described_class
+          .new(project, user, name: 'new-state', lock_id: 'abc-abc')
+
+        handler.lock!
+
+        state = handler.handle_with_lock do |state|
+          state.name = 'new-name'
+        end
+
+        expect(state.name).to eq 'new-name'
+      end
+    end
+
+    it 'raises exception if lock has not been acquired before' do
+      handler = described_class
+        .new(project, user, name: 'new-state', lock_id: 'abc-abc')
+
+      expect { handler.handle_with_lock }
+        .to raise_error(described_class::StateLockedError)
+    end
+
+    describe '#lock!' do
+      it 'allows to lock state if it does not exist yet' do
+        handler = described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc')
+
+        state = handler.lock!
+
+        expect(state).to be_persisted
+        expect(state.name).to eq 'new-state'
+      end
+
+      it 'allows to lock state if it exists and is not locked' do
+        state = described_class.new(project, user, name: 'new-state').create_or_find!
+        handler = described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc')
+
+        handler.lock!
+
+        expect(state.reload.lock_xid).to eq 'abc-abc'
+        expect(state).to be_locked
+      end
+
+      it 'raises an exception when trying to unlocked state locked by someone else' do
+        described_class.new(project, user, name: 'new-state', lock_id: 'abc-abc').lock!
+
+        handler = described_class.new(project, user, name: 'new-state', lock_id: '12a-23f')
+
+        expect { handler.lock! }.to raise_error(described_class::StateLockedError)
+      end
+    end
+  end
+end