Merge branch '235108-remove-legacy-terraform-file-uploader' into 'master'

Remove unused object storage config from Terraform::State

See merge request !45113

app/models/terraform/state.rb

@@ -3,7 +3,6 @@
 module Terraform
   class State < ApplicationRecord
     include UsageStatistics
-    include FileStoreMounter
     include IgnorableColumns
     # These columns are being removed since geo replication falls to the versioned state
     # Tracking in https://gitlab.com/gitlab-org/gitlab/-/issues/258262
@@ -36,18 +35,8 @@ class State < ApplicationRecord
 
     default_value_for(:uuid, allows_nil: false) { SecureRandom.hex(UUID_LENGTH / 2) }
 
-    mount_file_store_uploader StateUploader
-
-    def file_store
-      super || StateUploader.default_store
-    end
-
     def latest_file
-      if versioning_enabled?
-        latest_version&.file
-      else
-        latest_version&.file || file
-      end
+      latest_version&.file
     end
 
     def locked?
@@ -55,13 +44,14 @@ def locked?
     end
 
     def update_file!(data, version:, build:)
+      # This check is required to maintain backwards compatibility with
+      # states that were created prior to versioning being supported.
+      # This can be removed in 14.0 when support for these states is dropped.
+      # See https://gitlab.com/gitlab-org/gitlab/-/issues/258960
       if versioning_enabled?
         create_new_version!(data: data, version: version, build: build)
-      elsif latest_version.present?
-        migrate_legacy_version!(data: data, version: version, build: build)
       else
-        self.file = data
-        save!
+        migrate_legacy_version!(data: data, version: version, build: build)
       end
     end
 

app/models/terraform/state_version.rb

@@ -10,9 +10,9 @@ class StateVersion < ApplicationRecord
 
     scope :ordered_by_version_desc, -> { order(version: :desc) }
 
-    default_value_for(:file_store) { VersionedStateUploader.default_store }
+    default_value_for(:file_store) { StateUploader.default_store }
 
-    mount_file_store_uploader VersionedStateUploader
+    mount_file_store_uploader StateUploader
 
     delegate :project_id, :uuid, to: :terraform_state, allow_nil: true
 

app/uploaders/terraform/state_uploader.rb

@@ -6,17 +6,33 @@ class StateUploader < GitlabUploader
 
     storage_options Gitlab.config.terraform_state
 
-    delegate :project_id, to: :model
+    delegate :terraform_state, :project_id, to: :model
 
     # Use Lockbox to encrypt/decrypt the stored file (registers CarrierWave callbacks)
     encrypt(key: :key)
 
     def filename
-      "#{model.uuid}.tfstate"
+      # This check is required to maintain backwards compatibility with
+      # states that were created prior to versioning being supported.
+      # This can be removed in 14.0 when support for these states is dropped.
+      # See https://gitlab.com/gitlab-org/gitlab/-/issues/258960
+      if terraform_state.versioning_enabled?
+        "#{model.version}.tfstate"
+      else
+        "#{model.uuid}.tfstate"
+      end
     end
 
     def store_dir
-      project_id.to_s
+      # This check is required to maintain backwards compatibility with
+      # states that were created prior to versioning being supported.
+      # This can be removed in 14.0 when support for these states is dropped.
+      # See https://gitlab.com/gitlab-org/gitlab/-/issues/258960
+      if terraform_state.versioning_enabled?
+        Gitlab::HashedPath.new(model.uuid, root_hash: project_id)
+      else
+        project_id.to_s
+      end
     end
 
     def key

app/uploaders/terraform/versioned_state_uploader.rb

-# frozen_string_literal: true
-
-module Terraform
-  class VersionedStateUploader < StateUploader
-    delegate :terraform_state, to: :model
-
-    def filename
-      if terraform_state.versioning_enabled?
-        "#{model.version}.tfstate"
-      else
-        "#{model.uuid}.tfstate"
-      end
-    end
-
-    def store_dir
-      if terraform_state.versioning_enabled?
-        Gitlab::HashedPath.new(model.uuid, root_hash: project_id)
-      else
-        project_id.to_s
-      end
-    end
-  end
-end

ee/spec/models/ee/terraform/state_version_spec.rb

@@ -17,7 +17,7 @@
     end
 
     it 'excludes states without local storage' do
-      stub_terraform_state_version_object_storage
+      stub_terraform_state_object_storage
 
       create_list(:terraform_state_version, 5)
 
@@ -53,7 +53,7 @@
 
       before do
         stub_current_geo_node(secondary)
-        stub_terraform_state_version_object_storage if terraform_object_storage_enabled
+        stub_terraform_state_object_storage if terraform_object_storage_enabled
 
         create_list(:terraform_state_version, 5, terraform_state: create(:terraform_state, project: project))
         create_list(:terraform_state_version, 5, terraform_state: create(:terraform_state, project: create(:project)))

spec/factories/terraform/state.rb

@@ -6,11 +6,6 @@
 
     sequence(:name) { |n| "state-#{n}" }
 
-    trait :with_file do
-      versioning_enabled { false }
-      file { fixture_file_upload('spec/fixtures/terraform/terraform.tfstate', 'application/json') }
-    end
-
     trait :locked do
       sequence(:lock_xid) { |n| "lock-#{n}" }
       locked_at { Time.current }
@@ -22,8 +17,5 @@
         create(:terraform_state_version, 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/models/terraform/state_spec.rb

@@ -3,21 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe Terraform::State do
-  subject { create(:terraform_state, :with_file) }
-
-  let(:terraform_state_file) { fixture_file('terraform/terraform.tfstate') }
-
-  it { is_expected.to be_a FileStoreMounter }
+  subject { create(:terraform_state, :with_version) }
 
   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
-  end
-
   describe 'scopes' do
     describe '.ordered_by_name' do
       let_it_be(:project) { create(:project) }
@@ -35,64 +27,18 @@
     end
   end
 
-  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
-  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|
-          expect(build(:terraform_state, file_store: store).file_store).to eq(store)
-        end
-      end
-    end
-  end
-
-  describe '#update_file_store' do
-    context 'when file is stored in object storage' do
-      it_behaves_like 'mounted file in object store'
-    end
-
-    context 'when file is stored locally' do
-      before do
-        stub_terraform_state_object_storage(enabled: false)
-      end
-
-      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) }
+    let(:terraform_state) { create(:terraform_state, :with_version) }
+    let(:latest_version) { terraform_state.latest_version }
 
-        it { is_expected.to be_nil }
-      end
-    end
-
-    context 'versioning is disabled' do
-      let(:terraform_state) { create(:terraform_state, :with_file) }
+    subject { terraform_state.latest_file }
 
-      it { is_expected.to eq terraform_state.file }
+    it { is_expected.to eq latest_version.file }
 
-      context 'and a version exists (migration to versioned in progress)' do
-        let!(:migrated_version) { create(:terraform_state_version, terraform_state: terraform_state) }
+    context 'but no version exists yet' do
+      let(:terraform_state) { create(:terraform_state) }
 
-        it { is_expected.to eq terraform_state.latest_version.file }
-      end
+      it { is_expected.to be_nil }
     end
   end
 
@@ -115,39 +61,30 @@
       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 }
+    context 'versioning is disabled (migration to versioned in progress)' do
+      let(:terraform_state) { create(:terraform_state, versioning_enabled: false) }
+      let!(:migrated_version) { create(:terraform_state_version, terraform_state: terraform_state, version: 0) }
 
-        expect(terraform_state.latest_file.read).to eq(data)
-      end
-
-      context 'and a version exists (migration to versioned in progress)' do
-        let!(:migrated_version) { create(:terraform_state_version, terraform_state: terraform_state, version: 0) }
+      it 'creates a new version, corrects the migrated version number, and marks the state as versioned' do
+        expect { subject }.to change { Terraform::StateVersion.count }
 
-        it 'creates a new version, corrects the migrated version number, and marks the state as versioned' do
-          expect { subject }.to change { Terraform::StateVersion.count }
+        expect(migrated_version.reload.version).to eq(1)
+        expect(migrated_version.file.read).to eq(fixture_file('terraform/terraform.tfstate'))
 
-          expect(migrated_version.reload.version).to eq(1)
-          expect(migrated_version.file.read).to eq(terraform_state_file)
+        expect(terraform_state.reload.latest_version.version).to eq(version)
+        expect(terraform_state.latest_version.file.read).to eq(data)
+        expect(terraform_state).to be_versioning_enabled
+      end
 
-          expect(terraform_state.reload.latest_version.version).to eq(version)
-          expect(terraform_state.latest_version.file.read).to eq(data)
-          expect(terraform_state).to be_versioning_enabled
+      context 'the current version cannot be determined' do
+        before do
+          migrated_version.update!(file: CarrierWaveStringFile.new('invalid-json'))
         end
 
-        context 'the current version cannot be determined' do
-          before do
-            migrated_version.update!(file: CarrierWaveStringFile.new('invalid-json'))
-          end
-
-          it 'uses version - 1 to correct the migrated version number' do
-            expect { subject }.to change { Terraform::StateVersion.count }
+        it 'uses version - 1 to correct the migrated version number' do
+          expect { subject }.to change { Terraform::StateVersion.count }
 
-            expect(migrated_version.reload.version).to eq(2)
-          end
+          expect(migrated_version.reload.version).to eq(2)
         end
       end
     end

spec/support/helpers/stub_object_storage.rb

@@ -93,13 +93,6 @@ def stub_uploads_object_storage(uploader = described_class, **params)
   end
 
   def stub_terraform_state_object_storage(**params)
-    stub_object_storage_uploader(config: Gitlab.config.terraform_state.object_store,
-                                 uploader: Terraform::VersionedStateUploader,
-                                 remote_directory: 'terraform',
-                                 **params)
-  end
-
-  def stub_terraform_state_version_object_storage(**params)
     stub_object_storage_uploader(config: Gitlab.config.terraform_state.object_store,
                                  uploader: Terraform::StateUploader,
                                  remote_directory: 'terraform',

spec/uploaders/terraform/state_uploader_spec.rb

@@ -3,23 +3,45 @@
 require 'spec_helper'
 
 RSpec.describe Terraform::StateUploader do
-  subject { terraform_state.file }
+  subject { state_version.file }
 
-  let(:terraform_state) { create(:terraform_state, :with_file) }
+  let(:state_version) { create(:terraform_state_version) }
 
   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 include(terraform_state.uuid)
+    it 'contains the version of the terraform state record' do
+      expect(subject.filename).to eq("#{state_version.version}.tfstate")
+    end
+
+    context 'legacy state with versioning disabled' do
+      let(:state) { create(:terraform_state, versioning_enabled: false) }
+      let(:state_version) { create(:terraform_state_version, terraform_state: state) }
+
+      it 'contains the UUID of the terraform state record' do
+        expect(subject.filename).to eq("#{state_version.uuid}.tfstate")
+      end
     end
   end
 
   describe '#store_dir' do
-    it 'contains the ID of the project' do
-      expect(subject.store_dir).to include(terraform_state.project_id.to_s)
+    it 'hashes the project ID and UUID' do
+      expect(Gitlab::HashedPath).to receive(:new)
+        .with(state_version.uuid, root_hash: state_version.project_id)
+        .and_return(:store_dir)
+
+      expect(subject.store_dir).to eq(:store_dir)
+    end
+
+    context 'legacy state with versioning disabled' do
+      let(:state) { create(:terraform_state, versioning_enabled: false) }
+      let(:state_version) { create(:terraform_state_version, terraform_state: state) }
+
+      it 'contains the ID of the project' do
+        expect(subject.store_dir).to include(state_version.project_id.to_s)
+      end
     end
   end
 
@@ -27,7 +49,7 @@
     it 'creates a digest with a secret key and the project id' do
       expect(OpenSSL::HMAC)
         .to receive(:digest)
-        .with('SHA256', Gitlab::Application.secrets.db_key_base, terraform_state.project_id.to_s)
+        .with('SHA256', Gitlab::Application.secrets.db_key_base, state_version.project_id.to_s)
         .and_return('digest')
 
       expect(subject.key).to eq('digest')

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 version of the terraform state record' do
-      expect(subject.filename).to eq("#{model.version}.tfstate")
-    end
-
-    context 'legacy state with versioning disabled' do
-      let(:state) { create(:legacy_terraform_state) }
-      let(:model) { create(:terraform_state_version, terraform_state: state) }
-
-      it 'contains the UUID of the terraform state record' do
-        expect(subject.filename).to eq("#{model.uuid}.tfstate")
-      end
-    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
-
-    context 'legacy state with versioning disabled' do
-      let(:state) { create(:legacy_terraform_state) }
-      let(:model) { create(:terraform_state_version, terraform_state: state) }
-
-      it 'contains the ID of the project' do
-        expect(subject.store_dir).to include(model.project_id.to_s)
-      end
-    end
-  end
-end