Add versioning support to Terraform backend

Creates a new `terraform_state_versions` table to
store a separate file every time a Terraform state
is updated, instead of overwriting the same file each
time.

Existing Terraform states are unchanged and will need
to be migrated at a later date, at which point we can
remove the (now duplicated) `file` and `file_store`
columns on `terraform_states`.

app/models/terraform/state.rb

@@ -5,27 +5,34 @@ module Terraform
     include UsageStatistics
     include FileStoreMounter
 
-    DEFAULT = '{"version":1}'.freeze
     HEX_REGEXP = %r{\A\h+\z}.freeze
     UUID_LENGTH = 32
 
     belongs_to :project
     belongs_to :locked_by_user, class_name: 'User'
 
+    has_many :versions, class_name: 'Terraform::StateVersion', foreign_key: :terraform_state_id
+    has_one :latest_version, -> { ordered_by_version_desc }, class_name: 'Terraform::StateVersion', foreign_key: :terraform_state_id
+
+    scope :versioning_not_enabled, -> { where(versioning_enabled: false) }
+
     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) }
+    default_value_for(:versioning_enabled, true)
 
     mount_file_store_uploader StateUploader
 
-    default_value_for(:file) { CarrierWaveStringFile.new(DEFAULT) }
-
     def file_store
       super || StateUploader.default_store
     end
 
+    def latest_file
+      versioning_enabled ? latest_version&.file : file
+    end
+
     def local?
       file_store == ObjectStorage::Store::LOCAL
     end
@@ -33,6 +40,17 @@ module Terraform
     def locked?
       self.lock_xid.present?
     end
+
+    def update_file!(data, version:)
+      if versioning_enabled?
+        new_version = versions.build(version: version)
+        new_version.assign_attributes(created_by_user: locked_by_user, file: data)
+        new_version.save!
+      else
+        self.file = data
+        save!
+      end
+    end
   end
 end
 

app/models/terraform/state_version.rb

+# frozen_string_literal: true
+
+module Terraform
+  class StateVersion < ApplicationRecord
+    include FileStoreMounter
+
+    belongs_to :terraform_state, class_name: 'Terraform::State', optional: false
+    belongs_to :created_by_user, class_name: 'User', optional: true
+
+    scope :ordered_by_version_desc, -> { order(version: :desc) }
+
+    default_value_for(:file_store) { VersionedStateUploader.default_store }
+
+    mount_file_store_uploader VersionedStateUploader
+
+    delegate :project_id, :uuid, to: :terraform_state, allow_nil: true
+  end
+end

app/uploaders/terraform/versioned_state_uploader.rb

+# frozen_string_literal: true
+
+module Terraform
+  class VersionedStateUploader < StateUploader
+    def filename
+      "#{model.version}.tfstate"
+    end
+
+    def store_dir
+      Gitlab::HashedPath.new(model.uuid, root_hash: project_id)
+    end
+  end
+end

changelogs/unreleased/207347-add-terraform-state-versioning.yml

+---
+title: Add versioning support to Terraform state backend
+merge_request: 35211
+author:
+type: added

db/migrate/20200610033228_create_terraform_state_versions.rb

+# frozen_string_literal: true
+
+class CreateTerraformStateVersions < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    create_table :terraform_state_versions, if_not_exists: true do |t|
+      t.references :terraform_state, index: false, null: false, foreign_key: { on_delete: :cascade }
+      t.references :created_by_user, foreign_key: false
+      t.timestamps_with_timezone null: false
+      t.integer :version, null: false
+      t.integer :file_store, limit: 2, null: false
+      t.text :file, null: false
+
+      t.index [:terraform_state_id, :version], unique: true, name: 'index_terraform_state_versions_on_state_id_and_version'
+    end
+
+    add_text_limit :terraform_state_versions, :file, 255
+  end
+
+  def down
+    drop_table :terraform_state_versions
+  end
+end

db/migrate/20200610040615_add_versioning_enabled_to_terraform_states.rb

+# frozen_string_literal: true
+
+class AddVersioningEnabledToTerraformStates < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :terraform_states, :versioning_enabled, :boolean, null: false, default: false
+  end
+end

db/migrate/20200611013227_add_users_foreign_key_to_terraform_state_versions.rb

+# frozen_string_literal: true
+
+class AddUsersForeignKeyToTerraformStateVersions < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :terraform_state_versions, :users, column: :created_by_user_id, on_delete: :nullify
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key_if_exists :terraform_state_versions, :users, column: :created_by_user_id
+    end
+  end
+end

db/schema_migrations/20200610033228

+354524319f4c426328c7485619e248d00df323842873eaf7a2b3fbd2ad93048f
\ No newline at end of file

db/schema_migrations/20200610040615

+1f698671f226289fa1eabbb988b94ecd6486038f4922076bb981e44ee2356b25
\ No newline at end of file

db/schema_migrations/20200611013227

+d0ede6c4a28988494b0e18c073e56c1d985de73c443cc6b6d99e0b34a7f37642
\ No newline at end of file

db/structure.sql

@@ -15994,6 +15994,27 @@ CREATE SEQUENCE public.term_agreements_id_seq
 
 ALTER SEQUENCE public.term_agreements_id_seq OWNED BY public.term_agreements.id;
 
+CREATE TABLE public.terraform_state_versions (
+    id bigint NOT NULL,
+    terraform_state_id bigint NOT NULL,
+    created_by_user_id bigint,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    version integer NOT NULL,
+    file_store smallint NOT NULL,
+    file text NOT NULL,
+    CONSTRAINT check_0824bb7bbd CHECK ((char_length(file) <= 255))
+);
+
+CREATE SEQUENCE public.terraform_state_versions_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.terraform_state_versions_id_seq OWNED BY public.terraform_state_versions.id;
+
 CREATE TABLE public.terraform_states (
     id bigint NOT NULL,
     project_id bigint NOT NULL,
@@ -16011,6 +16032,7 @@ CREATE TABLE public.terraform_states (
     verification_retry_count smallint,
     verification_checksum bytea,
     verification_failure text,
+    versioning_enabled boolean DEFAULT false NOT NULL,
     CONSTRAINT check_21a47163ea CHECK ((char_length(verification_failure) <= 255))
 );
 
@@ -17563,6 +17585,8 @@ ALTER TABLE ONLY public.tags ALTER COLUMN id SET DEFAULT nextval('public.tags_id
 
 ALTER TABLE ONLY public.term_agreements ALTER COLUMN id SET DEFAULT nextval('public.term_agreements_id_seq'::regclass);
 
+ALTER TABLE ONLY public.terraform_state_versions ALTER COLUMN id SET DEFAULT nextval('public.terraform_state_versions_id_seq'::regclass);
+
 ALTER TABLE ONLY public.terraform_states ALTER COLUMN id SET DEFAULT nextval('public.terraform_states_id_seq'::regclass);
 
 ALTER TABLE ONLY public.timelogs ALTER COLUMN id SET DEFAULT nextval('public.timelogs_id_seq'::regclass);
@@ -18871,6 +18895,9 @@ ALTER TABLE ONLY public.tags
 ALTER TABLE ONLY public.term_agreements
     ADD CONSTRAINT term_agreements_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.terraform_state_versions
+    ADD CONSTRAINT terraform_state_versions_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY public.terraform_states
     ADD CONSTRAINT terraform_states_pkey PRIMARY KEY (id);
 
@@ -21115,6 +21142,10 @@ 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_state_versions_on_created_by_user_id ON public.terraform_state_versions USING btree (created_by_user_id);
+
+CREATE UNIQUE INDEX index_terraform_state_versions_on_state_id_and_version ON public.terraform_state_versions USING btree (terraform_state_id, version);
+
 CREATE INDEX index_terraform_states_on_file_store ON public.terraform_states USING btree (file_store);
 
 CREATE INDEX index_terraform_states_on_locked_by_user_id ON public.terraform_states USING btree (locked_by_user_id);
@@ -21913,6 +21944,9 @@ ALTER TABLE ONLY public.geo_event_log
 ALTER TABLE ONLY public.projects
     ADD CONSTRAINT fk_6e5c14658a FOREIGN KEY (pool_repository_id) REFERENCES public.pool_repositories(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.terraform_state_versions
+    ADD CONSTRAINT fk_6e81384d7f FOREIGN KEY (created_by_user_id) REFERENCES public.users(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY public.protected_branch_push_access_levels
     ADD CONSTRAINT fk_7111b68cdb FOREIGN KEY (group_id) REFERENCES public.namespaces(id) ON DELETE CASCADE;
 
@@ -22336,6 +22370,9 @@ ALTER TABLE ONLY public.events
 ALTER TABLE ONLY public.ip_restrictions
     ADD CONSTRAINT fk_rails_04a93778d5 FOREIGN KEY (group_id) REFERENCES public.namespaces(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.terraform_state_versions
+    ADD CONSTRAINT fk_rails_04f176e239 FOREIGN KEY (terraform_state_id) REFERENCES public.terraform_states(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.ci_build_report_results
     ADD CONSTRAINT fk_rails_056d298d48 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 

ee/app/models/ee/terraform/state.rb

@@ -15,7 +15,9 @@ module EE
 
       class_methods do
         def replicables_for_geo_node(node = ::Gitlab::Geo.current_node)
-          selective_sync_scope(node).merge(object_storage_scope(node))
+          selective_sync_scope(node)
+            .merge(object_storage_scope(node))
+            .versioning_not_enabled
         end
 
         private

ee/app/replicators/geo/terraform_state_replicator.rb

@@ -8,6 +8,18 @@ module Geo
       model_record.file
     end
 
+    def handle_after_create_commit
+      return if model_record.versioning_enabled?
+
+      super
+    end
+
+    def handle_after_destroy
+      return if model_record.versioning_enabled?
+
+      super
+    end
+
     def self.model
       ::Terraform::State
     end

ee/spec/models/ee/terraform/state_spec.rb

@@ -56,13 +56,28 @@ RSpec.describe Terraform::State do
         stub_current_geo_node(secondary)
         stub_terraform_state_object_storage(Terraform::StateUploader) if terraform_object_storage_enabled
 
-        create_list(:terraform_state, 5, project: project)
-        create_list(:terraform_state, 5, project: create(:project))
+        create_list(:terraform_state, 5, :with_file, project: project)
+        create_list(:terraform_state, 5, :with_file, project: create(:project))
       end
 
       it 'returns the proper number of terraform states' do
         expect(Terraform::State.replicables_for_geo_node.count).to eq(synced_states)
       end
     end
+
+    context 'state versioning' do
+      let(:secondary) { create(:geo_node, sync_object_storage: true) }
+
+      before do
+        stub_current_geo_node(secondary)
+        stub_terraform_state_object_storage(Terraform::StateUploader)
+
+        create_list(:terraform_state, 5, project: project)
+      end
+
+      it 'excludes versioned states' do
+        expect(Terraform::State.replicables_for_geo_node.count).to be_zero
+      end
+    end
   end
 end

ee/spec/replicators/geo/terraform_state_replicator_spec.rb

@@ -3,7 +3,28 @@
 require 'spec_helper'
 
 RSpec.describe Geo::TerraformStateReplicator do
-  let(:model_record) { build(:terraform_state) }
+  let(:model_record) { build(:terraform_state, :with_file) }
 
   it_behaves_like 'a blob replicator'
+
+  context 'Terraform state versioning is enabled' do
+    let(:model_record) { build(:terraform_state, :with_version) }
+    let(:replicator) { model_record.replicator }
+
+    describe '#handle_after_create_commit' do
+      subject { replicator.handle_after_create_commit }
+
+      it 'does not create a Geo::Event' do
+        expect { subject }.not_to change { ::Geo::Event.count }
+      end
+    end
+
+    describe '#handle_after_destroy' do
+      subject { replicator.handle_after_destroy }
+
+      it 'does not create a Geo::Event' do
+        expect { subject }.not_to change { ::Geo::Event.count }
+      end
+    end
+  end
 end

ee/spec/services/geo/registry_consistency_service_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe Geo::RegistryConsistencyService, :geo, :use_clean_rails_memory_st
     { Geo::DesignRegistry => :project_with_design,
       Geo::MergeRequestDiffRegistry => :external_merge_request_diff,
       Geo::PackageFileRegistry => :package_file_with_file,
-      Geo::TerraformStateRegistry => :terraform_state }
+      Geo::TerraformStateRegistry => :legacy_terraform_state }
       .fetch(registry_class, default_factory_name)
   end
 

ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb

@@ -85,7 +85,7 @@ RSpec.describe Geo::Secondary::RegistryConsistencyWorker, :geo do
       lfs_object = create(:lfs_object)
       merge_request_diff = create(:merge_request_diff, :external)
       package_file = create(:conan_package_file, :conan_package)
-      terraform_state = create(:terraform_state, project: project)
+      terraform_state = create(:terraform_state, :with_file, project: project)
       upload = create(:upload)
 
       expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(0)

lib/api/terraform/state.rb

@@ -35,10 +35,10 @@ module API
           route_setting :authentication, basic_auth_personal_access_token: true, job_token_allowed: :basic_auth
           get do
             remote_state_handler.find_with_lock do |state|
-              no_content! unless state.file.exists?
+              no_content! unless state.latest_file && state.latest_file.exists?
 
               env['api.format'] = :binary # this bypasses json serialization
-              body state.file.read
+              body state.latest_file.read
               status :ok
             end
           end
@@ -52,8 +52,7 @@ module API
             no_content! if data.empty?
 
             remote_state_handler.handle_with_lock do |state|
-              state.file = CarrierWaveStringFile.new(data)
-              state.save!
+              state.update_file!(CarrierWaveStringFile.new(data), version: params[:serial])
               status :ok
             end
           end

spec/factories/terraform/state.rb

@@ -7,6 +7,7 @@ FactoryBot.define do
     sequence(:name) { |n| "state-#{n}" }
 
     trait :with_file do
+      versioning_enabled { false }
       file { fixture_file_upload('spec/fixtures/terraform/terraform.tfstate', 'application/json') }
     end
 
@@ -25,5 +26,14 @@ FactoryBot.define do
       with_file
       verification_failure { 'Could not calculate the checksum' }
     end
+
+    trait :with_version do
+      after(:create) do |state|
+        create(:terraform_state_version, :with_file, terraform_state: state)
+      end
+    end
+
+    # Remove with https://gitlab.com/gitlab-org/gitlab/-/issues/235108
+    factory :legacy_terraform_state, parent: :terraform_state, traits: [:with_file]
   end
 end

spec/factories/terraform/state_version.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :terraform_state_version, class: 'Terraform::StateVersion' do
+    terraform_state factory: :terraform_state
+    created_by_user factory: :user
+
+    sequence(:version)
+    file { fixture_file_upload('spec/fixtures/terraform/terraform.tfstate', 'application/json') }
+  end
+end

spec/models/terraform/state_spec.rb

@@ -7,8 +7,9 @@ RSpec.describe Terraform::State do
 
   let(:terraform_state_file) { fixture_file('terraform/terraform.tfstate') }
 
-  it { is_expected.to belong_to(:project) }
+  it { is_expected.to be_a FileStoreMounter }
 
+  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) }
@@ -23,14 +24,6 @@ RSpec.describe Terraform::State 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
@@ -56,4 +49,55 @@ RSpec.describe Terraform::State do
       it_behaves_like 'mounted file in local store'
     end
   end
+
+  describe '#latest_file' do
+    subject { terraform_state.latest_file }
+
+    context 'versioning is enabled' do
+      let(:terraform_state) { create(:terraform_state, :with_version) }
+      let(:latest_version) { terraform_state.latest_version }
+
+      it { is_expected.to eq latest_version.file }
+
+      context 'but no version exists yet' do
+        let(:terraform_state) { create(:terraform_state) }
+
+        it { is_expected.to be_nil }
+      end
+    end
+
+    context 'versioning is disabled' do
+      let(:terraform_state) { create(:terraform_state, :with_file) }
+
+      it { is_expected.to eq terraform_state.file }
+    end
+  end
+
+  describe '#update_file!' do
+    let(:version) { 2 }
+    let(:data) { Hash[terraform_version: '0.12.21'].to_json }
+
+    subject { terraform_state.update_file!(CarrierWaveStringFile.new(data), version: version) }
+
+    context 'versioning is enabled' do
+      let(:terraform_state) { create(:terraform_state) }
+
+      it 'creates a new version' do
+        expect { subject }.to change { Terraform::StateVersion.count }
+
+        expect(terraform_state.latest_version.version).to eq(version)
+        expect(terraform_state.latest_version.file.read).to eq(data)
+      end
+    end
+
+    context 'versioning is disabled' do
+      let(:terraform_state) { create(:terraform_state, :with_file) }
+
+      it 'modifies the existing state record' do
+        expect { subject }.not_to change { Terraform::StateVersion.count }
+
+        expect(terraform_state.latest_file.read).to eq(data)
+      end
+    end
+  end
 end

spec/models/terraform/state_version_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Terraform::StateVersion do
+  it { is_expected.to be_a FileStoreMounter }
+
+  it { is_expected.to belong_to(:terraform_state).required }
+  it { is_expected.to belong_to(:created_by_user).class_name('User').optional }
+
+  describe 'scopes' do
+    describe '.ordered_by_version_desc' do
+      let(:terraform_state) { create(:terraform_state) }
+      let(:versions) { [4, 2, 5, 1, 3] }
+
+      subject { described_class.ordered_by_version_desc }
+
+      before do
+        versions.each do |version|
+          create(:terraform_state_version, terraform_state: terraform_state, version: version)
+        end
+      end
+
+      it { expect(subject.map(&:version)).to eq(versions.sort.reverse) }
+    end
+  end
+
+  context 'file storage' do
+    subject { create(:terraform_state_version) }
+
+    before do
+      stub_terraform_state_object_storage(Terraform::StateUploader)
+    end
+
+    describe '#file' do
+      let(:terraform_state_file) { fixture_file('terraform/terraform.tfstate') }
+
+      before do
+        subject.file = CarrierWaveStringFile.new(terraform_state_file)
+        subject.save!
+      end
+
+      it 'returns the saved file' do
+        expect(subject.file.read).to eq(terraform_state_file)
+      end
+    end
+
+    describe '#file_store' do
+      it 'returns the value' do
+        [ObjectStorage::Store::LOCAL, ObjectStorage::Store::REMOTE].each do |store|
+          subject.update!(file_store: store)
+
+          expect(subject.file_store).to eq(store)
+        end
+      end
+    end
+
+    describe '#update_file_store' do
+      context 'when file is stored in object storage' do
+        it 'sets file_store to remote' do
+          expect(subject.file_store).to eq(ObjectStorage::Store::REMOTE)
+        end
+      end
+
+      context 'when file is stored locally' do
+        before do
+          stub_terraform_state_object_storage(enabled: false)
+        end
+
+        it 'sets file_store to local' do
+          expect(subject.file_store).to eq(ObjectStorage::Store::LOCAL)
+        end
+      end
+    end
+  end
+end

spec/requests/api/terraform/state_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe API::Terraform::State do
   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!(:state) { create(:terraform_state, :with_version, project: project) }
 
   let(:current_user) { maintainer }
   let(:auth_header) { user_basic_auth_header(current_user) }
@@ -42,7 +42,7 @@ RSpec.describe API::Terraform::State do
           request
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(response.body).to eq(state.file.read)
+          expect(response.body).to eq(state.reload.latest_file.read)
         end
 
         context 'for a project that does not exist' do
@@ -63,7 +63,7 @@ RSpec.describe API::Terraform::State do
           request
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(response.body).to eq(state.file.read)
+          expect(response.body).to eq(state.reload.latest_file.read)
         end
       end
     end
@@ -78,7 +78,7 @@ RSpec.describe API::Terraform::State do
           request
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(response.body).to eq(state.file.read)
+          expect(response.body).to eq(state.reload.latest_file.read)
         end
 
         it 'returns unauthorized if the the job is not running' do
@@ -106,14 +106,14 @@ RSpec.describe API::Terraform::State do
           request
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(response.body).to eq(state.file.read)
+          expect(response.body).to eq(state.reload.latest_file.read)
         end
       end
     end
   end
 
   describe 'POST /projects/:id/terraform/state/:name' do
-    let(:params) { { 'instance': 'example-instance' } }
+    let(:params) { { 'instance': 'example-instance', 'serial': '1' } }
 
     subject(:request) { post api(state_path), headers: auth_header, as: :json, params: params }
 

spec/uploaders/terraform/versioned_state_uploader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Terraform::VersionedStateUploader do
+  subject { model.file }
+
+  let(:model) { create(:terraform_state_version, :with_file) }
+
+  before do
+    stub_terraform_state_object_storage
+  end
+
+  describe '#filename' do
+    it 'contains the UUID of the terraform state record' do
+      expect(subject.filename).to eq("#{model.version}.tfstate")
+    end
+  end
+
+  describe '#store_dir' do
+    it 'hashes the project ID and UUID' do
+      expect(Gitlab::HashedPath).to receive(:new)
+        .with(model.uuid, root_hash: model.project_id)
+        .and_return(:store_dir)
+
+      expect(subject.store_dir).to eq(:store_dir)
+    end
+  end
+end