Merge branch '14108-instance-level-ci-variables-controller' into 'master'

Add admin controller actions for instance variables

See merge request gitlab-org/gitlab!30385

app/controllers/admin/ci/variables_controller.rb

+# frozen_string_literal: true
+
+class Admin::Ci::VariablesController < Admin::ApplicationController
+  def show
+    respond_to do |format|
+      format.json { render_instance_variables }
+    end
+  end
+
+  def update
+    service = Ci::UpdateInstanceVariablesService.new(variables_params)
+
+    if service.execute
+      respond_to do |format|
+        format.json { render_instance_variables }
+      end
+    else
+      respond_to do |format|
+        format.json { render_error(service.errors) }
+      end
+    end
+  end
+
+  private
+
+  def variables
+    @variables ||= Ci::InstanceVariable.all
+  end
+
+  def render_instance_variables
+    render status: :ok,
+      json: {
+        variables: Ci::InstanceVariableSerializer.new.represent(variables)
+      }
+  end
+
+  def render_error(errors)
+    render status: :bad_request, json: errors
+  end
+
+  def variables_params
+    params.permit(variables_attributes: [*variable_params_attributes])
+  end
+
+  def variable_params_attributes
+    %i[id variable_type key secret_value protected masked _destroy]
+  end
+end

app/serializers/ci/basic_variable_entity.rb

+# frozen_string_literal: true
+
+module Ci
+  class BasicVariableEntity < Grape::Entity
+    expose :id
+    expose :key
+    expose :value
+    expose :variable_type
+
+    expose :protected?, as: :protected
+    expose :masked?, as: :masked
+  end
+end

app/serializers/ci/instance_variable_serializer.rb

+# frozen_string_literal: true
+
+module Ci
+  class InstanceVariableSerializer < BaseSerializer
+    entity BasicVariableEntity
+  end
+end

app/serializers/group_variable_entity.rb

 # frozen_string_literal: true
 
-class GroupVariableEntity < Grape::Entity
-  expose :id
-  expose :key
-  expose :value
-  expose :variable_type
-
-  expose :protected?, as: :protected
-  expose :masked?, as: :masked
+class GroupVariableEntity < Ci::BasicVariableEntity
 end

app/serializers/variable_entity.rb

 # frozen_string_literal: true
 
-class VariableEntity < Grape::Entity
-  expose :id
-  expose :key
-  expose :value
-  expose :variable_type
-
-  expose :protected?, as: :protected
-  expose :masked?, as: :masked
+class VariableEntity < Ci::BasicVariableEntity
   expose :environment_scope
 end

app/services/ci/update_instance_variables_service.rb

+# frozen_string_literal: true
+
+# This class is a simplified version of assign_nested_attributes_for_collection_association from ActiveRecord
+# https://github.com/rails/rails/blob/v6.0.2.1/activerecord/lib/active_record/nested_attributes.rb#L466
+
+module Ci
+  class UpdateInstanceVariablesService
+    UNASSIGNABLE_KEYS = %w(id _destroy).freeze
+
+    def initialize(params)
+      @params = params[:variables_attributes]
+    end
+
+    def execute
+      instantiate_records
+      persist_records
+    end
+
+    def errors
+      @records.to_a.flat_map { |r| r.errors.full_messages }
+    end
+
+    private
+
+    attr_reader :params
+
+    def existing_records_by_id
+      @existing_records_by_id ||= Ci::InstanceVariable
+        .all
+        .index_by { |var| var.id.to_s }
+    end
+
+    def instantiate_records
+      @records = params.map do |attributes|
+        find_or_initialize_record(attributes).tap do |record|
+          record.assign_attributes(attributes.except(*UNASSIGNABLE_KEYS))
+          record.mark_for_destruction if has_destroy_flag?(attributes)
+        end
+      end
+    end
+
+    def find_or_initialize_record(attributes)
+      id = attributes[:id].to_s
+
+      if id.blank?
+        Ci::InstanceVariable.new
+      else
+        existing_records_by_id.fetch(id) { raise ActiveRecord::RecordNotFound }
+      end
+    end
+
+    def persist_records
+      Ci::InstanceVariable.transaction do
+        success = @records.map do |record|
+          if record.marked_for_destruction?
+            record.destroy
+          else
+            record.save
+          end
+        end.all?
+
+        raise ActiveRecord::Rollback unless success
+
+        success
+      end
+    end
+
+    def has_destroy_flag?(hash)
+      Gitlab::Utils.to_boolean(hash['_destroy'])
+    end
+  end
+end

changelogs/unreleased/14108-instance-level-ci-variables-controller.yml

+---
+title: Add admin controller actions for interacting with instance variables
+merge_request: 30385
+author:
+type: added

config/routes/admin.rb

@@ -154,6 +154,10 @@ namespace :admin do
     end
   end
 
+  namespace :ci do
+    resource :variables, only: [:show, :update]
+  end
+
   concerns :clusterable
 
   get '/dashboard/stats', to: 'dashboard#stats'

spec/controllers/admin/ci/variables_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Admin::Ci::VariablesController do
+  let_it_be(:variable) { create(:ci_instance_variable) }
+
+  before do
+    sign_in(user)
+  end
+
+  describe 'GET #show' do
+    subject do
+      get :show, params: {}, format: :json
+    end
+
+    context 'when signed in as admin' do
+      let(:user) { create(:admin) }
+
+      include_examples 'GET #show lists all variables'
+    end
+
+    context 'when signed in as regular user' do
+      let(:user) { create(:user) }
+
+      it 'returns 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
+  describe 'PATCH #update' do
+    subject do
+      patch :update,
+        params: {
+          variables_attributes: variables_attributes
+        },
+        format: :json
+    end
+
+    context 'when signed in as admin' do
+      let(:user) { create(:admin) }
+
+      include_examples 'PATCH #update updates variables' do
+        let(:variables_scope) { Ci::InstanceVariable.all }
+        let(:file_variables_scope) { variables_scope.file }
+      end
+    end
+
+    context 'when signed in as regular user' do
+      let(:user) { create(:user) }
+
+      let(:variables_attributes) do
+        [{
+          id: variable.id,
+          key: variable.key,
+          secret_value: 'new value'
+        }]
+      end
+
+      it 'returns 404' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+end

spec/services/ci/update_instance_variables_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::UpdateInstanceVariablesService do
+  let(:params) { { variables_attributes: variables_attributes } }
+
+  subject { described_class.new(params) }
+
+  describe '#execute' do
+    context 'without variables' do
+      let(:variables_attributes) { [] }
+
+      it { expect(subject.execute).to be_truthy }
+    end
+
+    context 'with insert only variables' do
+      let(:variables_attributes) do
+        [
+          { key: 'var_a', secret_value: 'dummy_value_for_a', protected: true },
+          { key: 'var_b', secret_value: 'dummy_value_for_b', protected: false }
+        ]
+      end
+
+      it { expect(subject.execute).to be_truthy }
+
+      it 'persists all the records' do
+        expect { subject.execute }
+          .to change { Ci::InstanceVariable.count }
+          .by variables_attributes.size
+      end
+
+      it 'persists attributes' do
+        subject.execute
+
+        expect(Ci::InstanceVariable.all).to contain_exactly(
+          have_attributes(key: 'var_a', secret_value: 'dummy_value_for_a', protected: true),
+          have_attributes(key: 'var_b', secret_value: 'dummy_value_for_b', protected: false)
+        )
+      end
+    end
+
+    context 'with update only variables' do
+      let!(:var_a) { create(:ci_instance_variable) }
+      let!(:var_b) { create(:ci_instance_variable, protected: false) }
+
+      let(:variables_attributes) do
+        [
+          {
+            id: var_a.id,
+            key: var_a.key,
+            secret_value: 'new_dummy_value_for_a',
+            protected: var_a.protected?.to_s
+          },
+          {
+            id: var_b.id,
+            key: 'var_b_key',
+            secret_value: 'new_dummy_value_for_b',
+            protected: 'true'
+          }
+        ]
+      end
+
+      it { expect(subject.execute).to be_truthy }
+
+      it 'does not change the count' do
+        expect { subject.execute }
+          .not_to change { Ci::InstanceVariable.count }
+      end
+
+      it 'updates the records in place', :aggregate_failures do
+        subject.execute
+
+        expect(var_a.reload).to have_attributes(secret_value: 'new_dummy_value_for_a')
+
+        expect(var_b.reload).to have_attributes(
+          key: 'var_b_key', secret_value: 'new_dummy_value_for_b', protected: true)
+      end
+    end
+
+    context 'with insert and update variables' do
+      let!(:var_a) { create(:ci_instance_variable) }
+
+      let(:variables_attributes) do
+        [
+          {
+            id: var_a.id,
+            key: var_a.key,
+            secret_value: 'new_dummy_value_for_a',
+            protected: var_a.protected?.to_s
+          },
+          {
+            key: 'var_b',
+            secret_value: 'dummy_value_for_b',
+            protected: true
+          }
+        ]
+      end
+
+      it { expect(subject.execute).to be_truthy }
+
+      it 'inserts only one record' do
+        expect { subject.execute }
+          .to change { Ci::InstanceVariable.count }.by 1
+      end
+
+      it 'persists all the records', :aggregate_failures do
+        subject.execute
+        var_b = Ci::InstanceVariable.find_by(key: 'var_b')
+
+        expect(var_a.reload.secret_value).to eq('new_dummy_value_for_a')
+        expect(var_b.secret_value).to eq('dummy_value_for_b')
+      end
+    end
+
+    context 'with insert, update, and destroy variables' do
+      let!(:var_a) { create(:ci_instance_variable) }
+      let!(:var_b) { create(:ci_instance_variable) }
+
+      let(:variables_attributes) do
+        [
+          {
+            id: var_a.id,
+            key: var_a.key,
+            secret_value: 'new_dummy_value_for_a',
+            protected: var_a.protected?.to_s
+          },
+          {
+            id: var_b.id,
+            key: var_b.key,
+            secret_value: 'dummy_value_for_b',
+            protected: var_b.protected?.to_s,
+            '_destroy' => 'true'
+          },
+          {
+            key: 'var_c',
+            secret_value: 'dummy_value_for_c',
+            protected: true
+          }
+        ]
+      end
+
+      it { expect(subject.execute).to be_truthy }
+
+      it 'persists all the records', :aggregate_failures do
+        subject.execute
+        var_c = Ci::InstanceVariable.find_by(key: 'var_c')
+
+        expect(var_a.reload.secret_value).to eq('new_dummy_value_for_a')
+        expect { var_b.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        expect(var_c.secret_value).to eq('dummy_value_for_c')
+      end
+    end
+
+    context 'with invalid variables' do
+      let!(:var_a) { create(:ci_instance_variable, secret_value: 'dummy_value_for_a') }
+
+      let(:variables_attributes) do
+        [
+          {
+            key: '...?',
+            secret_value: 'nice_value'
+          },
+          {
+            id: var_a.id,
+            key: var_a.key,
+            secret_value: 'new_dummy_value_for_a',
+            protected: var_a.protected?.to_s
+          },
+          {
+            key: var_a.key,
+            secret_value: 'other_value'
+          }
+        ]
+      end
+
+      it { expect(subject.execute).to be_falsey }
+
+      it 'does not insert any records' do
+        expect { subject.execute }
+          .not_to change { Ci::InstanceVariable.count }
+      end
+
+      it 'does not update existing records' do
+        subject.execute
+
+        expect(var_a.reload.secret_value).to eq('dummy_value_for_a')
+      end
+
+      it 'returns errors' do
+        subject.execute
+
+        expect(subject.errors).to match_array(
+          [
+            "Key (#{var_a.key}) has already been taken",
+            "Key can contain only letters, digits and '_'."
+          ])
+      end
+    end
+
+    context 'when deleting non existing variables' do
+      let(:variables_attributes) do
+        [
+          {
+            id: 'some-id',
+            key: 'some_key',
+            secret_value: 'other_value',
+            '_destroy' => 'true'
+          }
+        ]
+      end
+
+      it { expect { subject.execute }.to raise_error(ActiveRecord::RecordNotFound) }
+    end
+
+    context 'when updating non existing variables' do
+      let(:variables_attributes) do
+        [
+          {
+            id: 'some-id',
+            key: 'some_key',
+            secret_value: 'other_value'
+          }
+        ]
+      end
+
+      it { expect { subject.execute }.to raise_error(ActiveRecord::RecordNotFound) }
+    end
+  end
+end

spec/support/shared_examples/controllers/variables_shared_examples.rb

@@ -27,6 +27,9 @@ RSpec.shared_examples 'PATCH #update updates variables' do
       protected: 'false' }
   end
 
+  let(:variables_scope) { owner.variables }
+  let(:file_variables_scope) { owner.variables.file }
+
   context 'with invalid new variable parameters' do
     let(:variables_attributes) do
       [
@@ -40,7 +43,7 @@ RSpec.shared_examples 'PATCH #update updates variables' do
     end
 
     it 'does not create the new variable' do
-      expect { subject }.not_to change { owner.variables.count }
+      expect { subject }.not_to change { variables_scope.count }
     end
 
     it 'returns a bad request response' do
@@ -63,7 +66,7 @@ RSpec.shared_examples 'PATCH #update updates variables' do
     end
 
     it 'does not create the new variable' do
-      expect { subject }.not_to change { owner.variables.count }
+      expect { subject }.not_to change { variables_scope.count }
     end
 
     it 'returns a bad request response' do
@@ -86,7 +89,7 @@ RSpec.shared_examples 'PATCH #update updates variables' do
     end
 
     it 'creates the new variable' do
-      expect { subject }.to change { owner.variables.count }.by(1)
+      expect { subject }.to change { variables_scope.count }.by(1)
     end
 
     it 'returns a successful response' do
@@ -106,7 +109,7 @@ RSpec.shared_examples 'PATCH #update updates variables' do
     let(:variables_attributes) { [variable_attributes.merge(_destroy: 'true')] }
 
     it 'destroys the variable' do
-      expect { subject }.to change { owner.variables.count }.by(-1)
+      expect { subject }.to change { variables_scope.count }.by(-1)
       expect { variable.reload }.to raise_error ActiveRecord::RecordNotFound
     end
 
@@ -123,6 +126,18 @@ RSpec.shared_examples 'PATCH #update updates variables' do
     end
   end
 
+  context 'with missing variable' do
+    let(:variables_attributes) do
+      [variable_attributes.merge(_destroy: 'true', id: 'some-id')]
+    end
+
+    it 'returns not found response' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+
   context 'for variables of type file' do
     let(:variables_attributes) do
       [
@@ -131,7 +146,7 @@ RSpec.shared_examples 'PATCH #update updates variables' do
     end
 
     it 'creates new variable of type file' do
-      expect { subject }.to change { owner.variables.file.count }.by(1)
+      expect { subject }.to change { file_variables_scope.count }.by(1)
     end
   end
 end