Merge branch 'caw-rd-termination-is-permanent' into 'master'

Remote Dev: Prevent update of terminated workspaces

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132088



Merged-by: Alper Akgun <aakgun@gitlab.com>
Approved-by: Alper Akgun <aakgun@gitlab.com>
Reviewed-by: Alper Akgun <aakgun@gitlab.com>
Reviewed-by: Chad Woolley <cwoolley@gitlab.com>
Co-authored-by: Chad Woolley <cwoolley@gitlab.com>

ee/app/models/remote_development/remote_development_agent_config.rb

@@ -14,7 +14,9 @@ class RemoteDevelopmentAgentConfig < ApplicationRecord
     validates :agent, presence: true
     validates :dns_zone, hostname: true
 
-    # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/409772 - Make this a type:enum
+    # NOTE: We do NOT want to use `enum` in the ActiveRecord models, because they break the `ActiveRecord#save` contract
+    #       by throwing an `ArgumentError` on `#save`, instead of `#save!`.
+    #       See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129708#note_1538946504 for more context.
     validates :enabled, inclusion: { in: [true], message: 'is currently immutable, and must be set to true' }
     # noinspection RubyResolve - likely due to https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31540
     before_validation :prevent_dns_zone_update, if: ->(record) { record.persisted? && record.dns_zone_changed? }

ee/app/models/remote_development/workspace.rb

@@ -31,14 +31,13 @@ class Workspace < ApplicationRecord
 
     # See https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/blob/main/doc/architecture.md?plain=0#workspace-states
     # for state validation rules
-    # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/409773
-    #       Add validation preventing desired_state change if it is TERMINATED; you can't restart a terminated workspace
-    #       Also reflect this in GraphQL API and Vue component UI
     validates :desired_state, inclusion: { in: VALID_DESIRED_STATES }
     validates :actual_state, inclusion: { in: VALID_ACTUAL_STATES }
     validates :editor, inclusion: { in: ['webide'], message: "'webide' is currently the only supported editor" }
     validates :max_hours_before_termination, numericality: { less_than_or_equal_to: MAX_HOURS_BEFORE_TERMINATION_LIMIT }
 
+    validate :enforce_permanent_termination
+
     ignore_column :force_full_reconciliation, remove_with: '16.7', remove_after: '2023-11-22'
 
     scope :with_desired_state_updated_more_recently_than_last_response_to_agent, -> do
@@ -83,6 +82,12 @@ def validate_agent_config_presence
       errors.add(:agent, _('for Workspace must have an associated RemoteDevelopmentAgentConfig'))
     end
 
+    def enforce_permanent_termination
+      return unless persisted? && desired_state_changed? && desired_state_was == Workspaces::States::TERMINATED
+
+      errors.add(:desired_state, "is 'Terminated', and cannot be updated. Create a new workspace instead.")
+    end
+
     def touch_desired_state_updated_at
       self.desired_state_updated_at = Time.current.utc
     end

ee/spec/models/remote_development/workspace_spec.rb

@@ -2,14 +2,19 @@
 
 require 'spec_helper'
 
+# noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542
 RSpec.describe RemoteDevelopment::Workspace, feature_category: :remote_development do
   let_it_be(:user) { create(:user) }
   let_it_be(:agent) { create(:ee_cluster_agent, :with_remote_development_agent_config) }
   let_it_be(:project) { create(:project, :in_group) }
   let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
 
+  let(:desired_state) { ::RemoteDevelopment::Workspaces::States::STOPPED }
+
   subject do
-    create(:workspace, user: user, agent: agent, project: project, personal_access_token: personal_access_token)
+    create(:workspace,
+      user: user, agent: agent, project: project,
+      personal_access_token: personal_access_token, desired_state: desired_state)
   end
 
   describe 'associations' do
@@ -66,7 +71,6 @@
 
       it 'sets desired_state_updated_at' do
         subject.save!
-        # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542
         expect(subject.desired_state_updated_at).to eq(Time.current)
       end
     end
@@ -74,7 +78,6 @@
     describe 'when updating desired_state' do
       it 'sets desired_state_updated_at' do
         expect { subject.update!(desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) }.to change {
-          # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542
           subject.desired_state_updated_at
         }
       end
@@ -83,7 +86,6 @@
     describe 'when updating a field other than desired_state' do
       it 'does not set desired_state_updated_at' do
         expect { subject.update!(actual_state: ::RemoteDevelopment::Workspaces::States::RUNNING) }.not_to change {
-          # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542
           subject.desired_state_updated_at
         }
       end
@@ -91,7 +93,6 @@
   end
 
   describe 'validations' do
-    # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542
     it 'validates max_hours_before_termination is no more than 120' do
       subject.max_hours_before_termination = described_class::MAX_HOURS_BEFORE_TERMINATION_LIMIT
       expect(subject).to be_valid
@@ -120,6 +121,20 @@
         expect(subject.errors[:agent]).to include('for Workspace must have an associated RemoteDevelopmentAgentConfig')
       end
     end
+
+    context 'when desired_state is Terminated' do
+      let(:desired_state) { ::RemoteDevelopment::Workspaces::States::TERMINATED }
+
+      before do
+        subject.desired_state = ::RemoteDevelopment::Workspaces::States::STOPPED
+      end
+
+      it 'prevents changes to desired_state' do
+        expect(subject).not_to be_valid
+        expect(subject.errors[:desired_state])
+          .to include("is 'Terminated', and cannot be updated. Create a new workspace instead.")
+      end
+    end
   end
 
   describe 'scopes' do