Implement Terraform State API

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/207345

app/models/terraform/state.rb

@@ -2,14 +2,26 @@
 
 module Terraform
   class State < ApplicationRecord
+    DEFAULT = '{"version":1}'.freeze
+    HEX_REGEXP = %r{\A\h+\z}.freeze
+    UUID_LENGTH = 32
+
     belongs_to :project
-    belongs_to :locked_by, class_name: 'User'
+    belongs_to :locked_by_user, class_name: 'User'
 
     validates :project_id, presence: true
+    validates :uuid, presence: true, uniqueness: true, length: { is: UUID_LENGTH },
+              format: { with: HEX_REGEXP, message: 'only allows hex characters' }
+
+    default_value_for(:uuid, allows_nil: false) { SecureRandom.hex(UUID_LENGTH / 2) }
+
+    after_save :update_file_store, if: :saved_change_to_file?
 
     mount_uploader :file, StateUploader
 
-    def update_file_store!
+    default_value_for(:file) { CarrierWaveStringFile.new(DEFAULT) }
+
+    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)

app/services/terraform/remote_state_handler.rb

@@ -2,8 +2,22 @@
 
 module Terraform
   class RemoteStateHandler < BaseService
+    include Gitlab::OptimisticLocking
+
     StateLockedError = Class.new(StandardError)
 
+    # rubocop: disable CodeReuse/ActiveRecord
+    def find_with_lock
+      raise ArgumentError unless params[:name].present?
+
+      state = Terraform::State.find_by(project: project, name: params[:name])
+      raise ActiveRecord::RecordNotFound.new("Couldn't find state") unless state
+
+      retry_optimistic_lock(state) { |state| yield state } if state && block_given?
+      state
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
     def create_or_find!
       raise ArgumentError unless params[:name].present?
 
@@ -16,8 +30,7 @@ module Terraform
 
         yield state if block_given?
 
-        state.save!
-        state.update_file_store!
+        state.save! unless state.destroyed?
       end
     end
 
@@ -28,7 +41,7 @@ module Terraform
         raise StateLockedError if state.locked?
 
         state.lock_xid = params[:lock_id]
-        state.locked_by = current_user
+        state.locked_by_user = current_user
         state.locked_at = Time.now
 
         state.save!
@@ -36,13 +49,12 @@ module Terraform
     end
 
     def unlock!
-      raise ArgumentError if params[:lock_id].blank?
-
       retrieve_with_lock do |state|
-        raise StateLockedError unless lock_matches?(state)
+        # force-unlock does not pass ID, so we ignore it if it is missing
+        raise StateLockedError unless params[:lock_id].nil? || lock_matches?(state)
 
         state.lock_xid = nil
-        state.locked_by = nil
+        state.locked_by_user = nil
         state.locked_at = nil
 
         state.save!
@@ -52,7 +64,7 @@ module Terraform
     private
 
     def retrieve_with_lock
-      create_or_find!.tap { |state| state.with_lock { yield state } }
+      create_or_find!.tap { |state| retry_optimistic_lock(state) { |state| yield state } }
     end
 
     def lock_matches?(state)

app/uploaders/terraform/state_uploader.rb

@@ -12,7 +12,7 @@ module Terraform
     encrypt(key: :key)
 
     def filename
-      "#{model.id}.tfstate"
+      "#{model.uuid}.tfstate"
     end
 
     def store_dir

changelogs/unreleased/feature-gb-terraform-state-locking.yml

+---
+title: Implement Terraform State API with locking
+merge_request: 28692
+author:
+type: added

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/migrate/20200416120128_add_columns_to_terraform_state.rb

+# frozen_string_literal: true
+
+class AddColumnsToTerraformState < 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_column :terraform_states, :locked_by_user_id, :bigint
+    add_column :terraform_states, :uuid, :string, limit: 32, null: false # rubocop:disable Rails/NotNullColumn (table not used yet)
+    add_column :terraform_states, :name, :string, limit: 255
+    add_index :terraform_states, :locked_by_user_id # rubocop:disable Migration/AddIndex (table not used yet)
+    add_index :terraform_states, :uuid, unique: true # rubocop:disable Migration/AddIndex (table not used yet)
+    add_index :terraform_states, [:project_id, :name], unique: true # rubocop:disable Migration/AddIndex (table not used yet)
+    remove_index :terraform_states, :project_id # rubocop:disable Migration/RemoveIndex (table not used yet)
+  end
+end

db/migrate/20200416120354_add_locked_by_user_id_foreign_key_to_terraform_state.rb

+# frozen_string_literal: true
+
+class AddLockedByUserIdForeignKeyToTerraformState < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_foreign_key :terraform_states, :users, column: :locked_by_user_id # rubocop:disable Migration/AddConcurrentForeignKey
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :terraform_states, column: :locked_by_user_id
+    end
+  end
+end

db/structure.sql

@@ -6137,7 +6137,8 @@ CREATE TABLE public.terraform_states (
     file character varying(255),
     lock_xid character varying(255),
     locked_at timestamp with time zone,
-    locked_by_id bigint,
+    locked_by_user_id bigint,
+    uuid character varying(32) NOT NULL,
     name character varying(255)
 );
 
@@ -10236,12 +10237,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 INDEX index_terraform_states_on_locked_by_user_id ON public.terraform_states USING btree (locked_by_user_id);
 
 CREATE UNIQUE INDEX index_terraform_states_on_project_id_and_name ON public.terraform_states USING btree (project_id, name);
 
+CREATE UNIQUE INDEX index_terraform_states_on_uuid ON public.terraform_states USING btree (uuid);
+
 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);
@@ -11419,6 +11420,9 @@ ALTER TABLE ONLY public.geo_node_namespace_links
 ALTER TABLE ONLY public.clusters_applications_knative
     ADD CONSTRAINT fk_rails_54fc91e0a0 FOREIGN KEY (cluster_id) REFERENCES public.clusters(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.terraform_states
+    ADD CONSTRAINT fk_rails_558901b030 FOREIGN KEY (locked_by_user_id) REFERENCES public.users(id);
+
 ALTER TABLE ONLY public.issue_user_mentions
     ADD CONSTRAINT fk_rails_57581fda73 FOREIGN KEY (issue_id) REFERENCES public.issues(id) ON DELETE CASCADE;
 
@@ -11800,9 +11804,6 @@ 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;
 
@@ -13168,9 +13169,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200402123926
 20200402124802
 20200402135250
-20200402171949
 20200402185044
-20200403095403
 20200403184110
 20200403185127
 20200403185422
@@ -13204,5 +13203,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200415161021
 20200415161206
 20200415192656
+20200416120128
+20200416120354
 \.
 

lib/api/terraform/state.rb

 # frozen_string_literal: true
 
+require_dependency 'api/validations/validators/limit'
+
 module API
   module Terraform
     class State < Grape::API
-      before { authenticate! }
-      before { authorize! :admin_terraform_state, user_project }
+      include ::Gitlab::Utils::StrongMemoize
+
+      default_format :json
+
+      before do
+        authenticate!
+        authorize! :admin_terraform_state, user_project
+      end
 
       params do
         requires :id, type: String, desc: 'The ID of a project'
       end
+
       resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+        namespace ':id/terraform/state/:name' do
           params do
-          requires :name, type: String, desc: 'The name of a terraform state'
+            requires :name, type: String, desc: 'The name of a Terraform state'
+            optional :ID, type: String, limit: 255, desc: 'Terraform state lock ID'
           end
-        namespace ':id/terraform/state/:name' do
+
+          helpers do
+            def remote_state_handler
+              ::Terraform::RemoteStateHandler.new(user_project, current_user, name: params[:name], lock_id: params[:ID])
+            end
+          end
+
           desc 'Get a terraform state by its name'
           route_setting :authentication, basic_auth_personal_access_token: true
           get do
-            status 501
-            content_type 'text/plain'
-            body 'not implemented'
+            remote_state_handler.find_with_lock do |state|
+              no_content! unless state.file.exists?
+
+              env['api.format'] = :binary # this bypasses json serialization
+              body state.file.read
+              status :ok
+            end
           end
 
           desc 'Add a new terraform state or update an existing one'
           route_setting :authentication, basic_auth_personal_access_token: true
           post do
-            status 501
-            content_type 'text/plain'
-            body 'not implemented'
+            data = request.body.string
+            no_content! if data.empty?
+
+            remote_state_handler.handle_with_lock do |state|
+              state.file = CarrierWaveStringFile.new(data)
+              state.save!
+              status :ok
+            end
           end
 
           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'
+            remote_state_handler.handle_with_lock do |state|
+              state.destroy!
+              status :ok
+            end
           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'
+            requires :ID, type: String, limit: 255, desc: 'Terraform state lock ID'
+            requires :Operation, type: String, desc: 'Terraform operation'
+            requires :Info, type: String, desc: 'Terraform info'
+            requires :Who, type: String, desc: 'Terraform state lock owner'
+            requires :Version, type: String, desc: 'Terraform version'
+            requires :Created, type: String, desc: 'Terraform state lock timestamp'
+            requires :Path, type: String, desc: 'Terraform path'
+          end
+          post '/lock' do
+            status_code = :ok
+            lock_info = {
+              'Operation' => params[:Operation],
+              'Info' => params[:Info],
+              'Version' => params[:Version],
+              'Path' => params[:Path]
+            }
+
+            begin
+              remote_state_handler.lock!
+            rescue ::Terraform::RemoteStateHandler::StateLockedError
+              status_code = :conflict
+            end
+
+            remote_state_handler.find_with_lock do |state|
+              lock_info['ID'] = state.lock_xid
+              lock_info['Who'] = state.locked_by_user.username
+              lock_info['Created'] = state.locked_at
+
+              env['api.format'] = :binary # this bypasses json serialization
+              body lock_info.to_json
+              status status_code
             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
+          params do
+            optional :ID, type: String, limit: 255, desc: 'Terraform state lock ID'
+          end
           delete '/lock' do
-            status 501
-            content_type 'text/plain'
-            body 'UNLOCK not implemented'
+            remote_state_handler.unlock!
+            status :ok
+          rescue ::Terraform::RemoteStateHandler::StateLockedError
+            status :conflict
           end
         end
       end

lib/api/validations/validators/limit.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Validators
+      class Limit < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          value = params[attr_name]
+
+          return if value.size <= @option
+
+          raise Grape::Exceptions::Validation,
+            params: [@scope.full_name(attr_name)],
+            message: "#{@scope.full_name(attr_name)} must be less than #{@option} characters"
+        end
+      end
+    end
+  end
+end

spec/factories/terraform/state.rb

@@ -4,8 +4,10 @@ FactoryBot.define do
   factory :terraform_state, class: 'Terraform::State' do
     project { create(:project) }
 
+    sequence(:name) { |n| "state-#{n}" }
+
     trait :with_file do
-      file { fixture_file_upload('spec/fixtures/terraform/terraform.tfstate') }
+      file { fixture_file_upload('spec/fixtures/terraform/terraform.tfstate', 'application/json') }
     end
   end
 end

spec/factories/users.rb

@@ -87,12 +87,17 @@ FactoryBot.define do
 
     transient do
       developer_projects { [] }
+      maintainer_projects { [] }
     end
 
     after(:create) do |user, evaluator|
       evaluator.developer_projects.each do |project|
         project.add_developer(user)
       end
+
+      evaluator.maintainer_projects.each do |project|
+        project.add_maintainer(user)
+      end
     end
 
     factory :omniauth_user do

spec/lib/api/validations/validators/limit_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Validations::Validators::Limit do
+  include ApiValidatorsHelpers
+
+  subject do
+    described_class.new(['test'], 255, false, scope.new)
+  end
+
+  context 'valid limit param' do
+    it 'does not raise a validation error' do
+      expect_no_validation_error('test' => '123-456')
+      expect_no_validation_error('test' => '00000000-ffff-0000-ffff-000000000000')
+      expect_no_validation_error('test' => "#{'a' * 255}")
+    end
+  end
+
+  context 'longer than limit param' do
+    it 'raises a validation error' do
+      expect_validation_error('test' => "#{'a' * 256}")
+    end
+  end
+end

spec/models/terraform/state_spec.rb

@@ -5,24 +5,35 @@ require 'spec_helper'
 describe Terraform::State do
   subject { create(:terraform_state, :with_file) }
 
+  let(:terraform_state_file) { fixture_file('terraform/terraform.tfstate') }
+
   it { is_expected.to belong_to(:project) }
 
+  it { is_expected.to belong_to(:locked_by_user).class_name('User') }
+
   it { is_expected.to validate_presence_of(:project_id) }
 
   before do
     stub_terraform_state_object_storage(Terraform::StateUploader)
   end
 
-  describe '#file_store' do
-    context 'when no value is set' do
-      it 'returns the default store of the uploader' do
-        [ObjectStorage::Store::LOCAL, ObjectStorage::Store::REMOTE].each do |store|
-          expect(Terraform::StateUploader).to receive(:default_store).and_return(store)
-          expect(described_class.new.file_store).to eq(store)
+  describe '#file' do
+    context 'when a file exists' do
+      it 'does not use the default file' do
+        expect(subject.file.read).to eq(terraform_state_file)
+      end
+    end
+
+    context 'when no file exists' do
+      subject { create(:terraform_state) }
+
+      it 'creates a default file' do
+        expect(subject.file.read).to eq('{"version":1}')
       end
     end
   end
 
+  describe '#file_store' do
     context 'when a value is set' do
       it 'returns the value' do
         [ObjectStorage::Store::LOCAL, ObjectStorage::Store::REMOTE].each do |store|

spec/requests/api/terraform/state_spec.rb

@@ -3,117 +3,231 @@
 require 'spec_helper'
 
 describe API::Terraform::State do
-  def auth_header_for(user)
-    auth_header = ActionController::HttpAuthentication::Basic.encode_credentials(
-      user.username,
-      create(:personal_access_token, user: user).token
-    )
-    { 'HTTP_AUTHORIZATION' => auth_header }
-  end
+  let_it_be(:project) { create(:project) }
+  let_it_be(:developer) { create(:user, developer_projects: [project]) }
+  let_it_be(:maintainer) { create(:user, maintainer_projects: [project]) }
+
+  let!(:state) { create(:terraform_state, :with_file, project: project) }
 
-  let!(:project) { create(:project) }
-  let(:developer) { create(:user) }
-  let(:maintainer) { create(:user) }
-  let(:state_name) { 'state' }
+  let(:current_user) { maintainer }
+  let(:auth_header) { basic_auth_header(current_user) }
+  let(:project_id) { project.id }
+  let(:state_name) { state.name }
+  let(:state_path) { "/projects/#{project_id}/terraform/state/#{state_name}" }
 
   before do
-    project.add_maintainer(maintainer)
+    stub_terraform_state_object_storage(Terraform::StateUploader)
   end
 
   describe 'GET /projects/:id/terraform/state/:name' do
-    it 'returns 401 if user is not authenticated' do
-      headers = { 'HTTP_AUTHORIZATION' => 'failing_token' }
+    subject(:request) { get api(state_path), headers: auth_header }
 
-      get api("/projects/#{project.id}/terraform/state/#{state_name}"), headers: headers
+    context 'without authentication' do
+      let(:auth_header) { basic_auth_header('failing_token') }
+
+      it 'returns 401 if user is not authenticated' do
+        request
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
+    end
+
+    context 'with maintainer permissions' do
+      let(:current_user) { maintainer }
 
       it 'returns terraform state belonging to a project of given state name' do
-      get api("/projects/#{project.id}/terraform/state/#{state_name}"), headers: auth_header_for(maintainer)
+        request
 
-      expect(response).to have_gitlab_http_status(:not_implemented)
-      expect(response.body).to eq('not implemented')
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response.body).to eq(state.file.read)
       end
 
-    it 'returns not found if the project does not exists' do
-      get api("/projects/0000/terraform/state/#{state_name}"), headers: auth_header_for(maintainer)
+      context 'for a project that does not exist' do
+        let(:project_id) { '0000' }
+
+        it 'returns not found' do
+          request
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
+      end
+    end
 
-    it 'returns forbidden if the user cannot access the state' do
-      project.add_developer(developer)
+    context 'with developer permissions' do
+      let(:current_user) { developer }
 
-      get api("/projects/#{project.id}/terraform/state/#{state_name}"), headers: auth_header_for(developer)
+      it 'returns forbidden if the user cannot access the state' do
+        request
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
+  end
 
   describe 'POST /projects/:id/terraform/state/:name' do
+    let(:params) { { 'instance': 'example-instance' } }
+
+    subject(:request) { post api(state_path), headers: auth_header, as: :json, params: params }
+
     context 'when terraform state with a given name is already present' do
+      context 'with maintainer permissions' do
+        let(:current_user) { maintainer }
+
         it 'updates the state' do
-        post api("/projects/#{project.id}/terraform/state/#{state_name}"),
-          params: '{ "instance": "example-instance" }',
-          headers: { 'Content-Type' => 'text/plain' }.merge(auth_header_for(maintainer))
+          expect { request }.to change { Terraform::State.count }.by(0)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'without body' do
+        let(:params) { nil }
 
-        expect(response).to have_gitlab_http_status(:not_implemented)
-        expect(response.body).to eq('not implemented')
+        it 'returns no content if no body is provided' do
+          request
+
+          expect(response).to have_gitlab_http_status(:no_content)
+        end
       end
 
-      it 'returns forbidden if the user cannot access the state' do
-        project.add_developer(developer)
+      context 'with developer permissions' do
+        let(:current_user) { developer }
 
-        post api("/projects/#{project.id}/terraform/state/#{state_name}"), headers: auth_header_for(developer)
+        it 'returns forbidden' do
+          request
 
           expect(response).to have_gitlab_http_status(:forbidden)
         end
       end
+    end
 
     context 'when there is no terraform state of a given name' do
+      let(:state_name) { 'example2' }
+
+      context 'with maintainer permissions' do
+        let(:current_user) { maintainer }
+
         it 'creates a new state' do
-        post api("/projects/#{project.id}/terraform/state/example2"),
-          headers: auth_header_for(maintainer),
-          params: '{ "database": "example-database" }'
+          expect { request }.to change { Terraform::State.count }.by(1)
 
-        expect(response).to have_gitlab_http_status(:not_implemented)
-        expect(response.body).to eq('not implemented')
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'without body' do
+        let(:params) { nil }
+
+        it 'returns no content if no body is provided' do
+          request
+
+          expect(response).to have_gitlab_http_status(:no_content)
+        end
+      end
+
+      context 'with developer permissions' do
+        let(:current_user) { developer }
+
+        it 'returns forbidden' do
+          request
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
       end
     end
   end
 
   describe 'DELETE /projects/:id/terraform/state/:name' do
+    subject(:request) { delete api(state_path), headers: auth_header }
+
+    context 'with maintainer permissions' do
+      let(:current_user) { maintainer }
+
       it 'deletes the state' do
-      delete api("/projects/#{project.id}/terraform/state/#{state_name}"), headers: auth_header_for(maintainer)
+        expect { request }.to change { Terraform::State.count }.by(-1)
 
-      expect(response).to have_gitlab_http_status(:not_implemented)
+        expect(response).to have_gitlab_http_status(:ok)
+      end
     end
 
-    it 'returns forbidden if the user cannot access the state' do
-      project.add_developer(developer)
+    context 'with developer permissions' do
+      let(:current_user) { developer }
 
-      delete api("/projects/#{project.id}/terraform/state/#{state_name}"), headers: auth_header_for(developer)
+      it 'returns forbidden' do
+        expect { request }.to change { Terraform::State.count }.by(0)
 
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
+  end
 
   describe 'PUT /projects/:id/terraform/state/:name/lock' do
+    let(:params) do
+      {
+        ID: '123-456',
+        Version: '0.1',
+        Operation: 'OperationTypePlan',
+        Info: '',
+        Who: "#{current_user.username}",
+        Created: Time.now.utc.iso8601(6),
+        Path: ''
+      }
+    end
+
+    subject(:request) { post api("#{state_path}/lock"), headers: auth_header, params: params }
+
     it 'locks the terraform state' do
-      put api("/projects/#{project.id}/terraform/state/#{state_name}/lock?ID=123-456"), headers: auth_header_for(maintainer)
+      request
 
-      expect(response).to have_gitlab_http_status(:not_implemented)
-      expect(response.body).to include('LOCK not implemented')
+      expect(response).to have_gitlab_http_status(:ok)
     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)
+    before do
+      state.lock_xid = '123-456'
+      state.save!
+    end
+
+    subject(:request) { delete api("#{state_path}/lock"), headers: auth_header, params: params }
+
+    context 'with the correct lock id' do
+      let(:params) { { ID: '123-456' } }
+
+      it 'removes the terraform state lock' do
+        request
 
-      expect(response).to have_gitlab_http_status(:not_implemented)
-      expect(response.body).to include('LOCK not implemented')
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
+
+    context 'with no lock id (force-unlock)' do
+      let(:params) { {} }
+
+      it 'removes the terraform state lock' do
+        request
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
+
+    context 'with an incorrect lock id' do
+      let(:params) { { ID: '456-789' } }
+
+      it 'returns an error' do
+        request
+
+        expect(response).to have_gitlab_http_status(:conflict)
+      end
+    end
+
+    context 'with a longer than 255 character lock id' do
+      let(:params) { { ID: '0' * 256 } }
+
+      it 'returns an error' do
+        request
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
     end
   end
 end

spec/services/terraform/remote_state_handler_spec.rb

@@ -6,6 +6,34 @@ describe Terraform::RemoteStateHandler do
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
 
+  describe '#find_with_lock' do
+    context 'without a state name' do
+      subject { described_class.new(project, user) }
+
+      it 'raises an exception' do
+        expect { subject.find_with_lock }.to raise_error(ArgumentError)
+      end
+    end
+
+    context 'with a state name' do
+      subject { described_class.new(project, user, name: 'state') }
+
+      context 'with no matching state' do
+        it 'raises an exception' do
+          expect { subject.find_with_lock }.to raise_error(ActiveRecord::RecordNotFound)
+        end
+      end
+
+      context 'with a matching state' do
+        let!(:state) { create(:terraform_state, project: project, name: 'state') }
+
+        it 'returns the state' do
+          expect(subject.find_with_lock).to eq(state)
+        end
+      end
+    end
+  end
+
   describe '#create_or_find!' do
     it 'requires passing a state name' do
       handler = described_class.new(project, user)
@@ -57,12 +85,6 @@ describe Terraform::RemoteStateHandler 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

spec/support/helpers/api_helpers.rb

@@ -40,6 +40,17 @@ module ApiHelpers
     end
   end
 
+  def basic_auth_header(user = nil)
+    return { 'HTTP_AUTHORIZATION' => user } unless user.respond_to?(:username)
+
+    {
+      'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Basic.encode_credentials(
+        user.username,
+        create(:personal_access_token, user: user).token
+      )
+    }
+  end
+
   def expect_empty_array_response
     expect_successful_response_with_paginated_array
     expect(json_response.length).to eq(0)

spec/uploaders/terraform/state_uploader_spec.rb

@@ -5,15 +5,15 @@ require 'spec_helper'
 describe Terraform::StateUploader do
   subject { terraform_state.file }
 
-  let(:terraform_state) { create(:terraform_state, file: fixture_file_upload('spec/fixtures/terraform/terraform.tfstate')) }
+  let(:terraform_state) { create(:terraform_state, :with_file) }
 
   before do
     stub_terraform_state_object_storage
   end
 
   describe '#filename' do
-    it 'contains the ID of the terraform state record' do
-      expect(subject.filename).to include(terraform_state.id.to_s)
+    it 'contains the UUID of the terraform state record' do
+      expect(subject.filename).to include(terraform_state.uuid)
     end
   end