Merge branch '383194-deployments-hooksworker-args-do-not-serialize-to-json-safely' into 'master'

Resolve "Deployments::HooksWorker args do not serialize to JSON safely"

See merge request !108988



Merged-by: Bala Kumar <sbalakumar@gitlab.com>
Approved-by: Shinya Maeda <shinya@gitlab.com>
Approved-by: Bala Kumar <sbalakumar@gitlab.com>
Reviewed-by: Shinya Maeda <shinya@gitlab.com>
Co-authored-by: Halil Coban <hcoban@gitlab.com>

app/models/deployment.rb

@@ -105,7 +105,13 @@ class Deployment < ApplicationRecord
 
     after_transition any => :running do |deployment, transition|
       deployment.run_after_commit do
-        Deployments::HooksWorker.perform_async(deployment_id: id, status: transition.to, status_changed_at: Time.current)
+        perform_params = { deployment_id: id, status: transition.to, status_changed_at: Time.current }
+
+        if Feature.enabled?(:improve_deployment_hooksworker_serialization, deployment.project)
+          serialize_params_for_sidekiq!(perform_params)
+        end
+
+        Deployments::HooksWorker.perform_async(perform_params)
       end
     end
 
@@ -119,7 +125,13 @@ class Deployment < ApplicationRecord
 
     after_transition any => FINISHED_STATUSES do |deployment, transition|
       deployment.run_after_commit do
-        Deployments::HooksWorker.perform_async(deployment_id: id, status: transition.to, status_changed_at: Time.current)
+        perform_params = { deployment_id: id, status: transition.to, status_changed_at: Time.current }
+
+        if Feature.enabled?(:improve_deployment_hooksworker_serialization, deployment.project)
+          serialize_params_for_sidekiq!(perform_params)
+        end
+
+        Deployments::HooksWorker.perform_async(perform_params)
       end
     end
 
@@ -464,6 +476,11 @@ def update_status!(status)
     end
   end
 
+  def serialize_params_for_sidekiq!(perform_params)
+    perform_params[:status_changed_at] = perform_params[:status_changed_at].to_s
+    perform_params.stringify_keys!
+  end
+
   def self.last_deployment_group_associations
     {
       deployable: {

config/feature_flags/development/improve_deployment_hooksworker_serialization.yml

+---
+name: improve_deployment_hooksworker_serialization
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108988
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/388126
+milestone: '15.8'
+type: development
+group: group::release
+default_enabled: false

spec/models/ci/build_spec.rb

@@ -1565,13 +1565,30 @@
         it 'transitions to running and calls webhook' do
           freeze_time do
             expect(Deployments::HooksWorker)
-            .to receive(:perform_async).with(deployment_id: deployment.id, status: 'running', status_changed_at: Time.current)
+              .to receive(:perform_async).with(hash_including({ 'deployment_id' => deployment.id, 'status' => 'running', 'status_changed_at' => Time.current.to_s }))
 
             subject
           end
 
           expect(deployment).to be_running
         end
+
+        context 'when improve_deployment_hooksworker_serialization is disabled' do
+          before do
+            stub_feature_flags(improve_deployment_hooksworker_serialization: false)
+          end
+
+          it 'transitions to running and calls webhook' do
+            freeze_time do
+              expect(Deployments::HooksWorker)
+              .to receive(:perform_async).with(hash_including({ deployment_id: deployment.id, status: 'running', status_changed_at: Time.current }))
+
+              subject
+            end
+
+            expect(deployment).to be_running
+          end
+        end
       end
     end
   end

spec/models/deployment_spec.rb

@@ -164,12 +164,30 @@
         freeze_time do
           expect(Deployments::HooksWorker)
             .to receive(:perform_async)
-            .with(deployment_id: deployment.id, status: 'running', status_changed_at: Time.current)
+                  .with(hash_including({ 'deployment_id' => deployment.id, 'status' => 'running',
+                                         'status_changed_at' => Time.current.to_s }))
 
           deployment.run!
         end
       end
 
+      context 'when improve_deployment_hooksworker_serialization is disabled' do
+        before do
+          stub_feature_flags(improve_deployment_hooksworker_serialization: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+              .to receive(:perform_async)
+                    .with(hash_including({ deployment_id: deployment.id, status: 'running',
+status_changed_at: Time.current }))
+
+            deployment.run!
+          end
+        end
+      end
+
       it 'does not execute Deployments::DropOlderDeploymentsWorker' do
         expect(Deployments::DropOlderDeploymentsWorker)
           .not_to receive(:perform_async).with(deployment.id)
@@ -200,12 +218,29 @@
       it 'executes Deployments::HooksWorker asynchronously' do
         freeze_time do
           expect(Deployments::HooksWorker)
-          .to receive(:perform_async)
-          .with(deployment_id: deployment.id, status: 'success', status_changed_at: Time.current)
+            .to receive(:perform_async)
+                  .with(hash_including({ 'deployment_id' => deployment.id, 'status' => 'success',
+                                         'status_changed_at' => Time.current.to_s }))
 
           deployment.succeed!
         end
       end
+
+      context 'when improve_deployment_hooksworker_serialization is disabled' do
+        before do
+          stub_feature_flags(improve_deployment_hooksworker_serialization: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+            .to receive(:perform_async)
+            .with(hash_including({ deployment_id: deployment.id, status: 'success', status_changed_at: Time.current }))
+
+            deployment.succeed!
+          end
+        end
+      end
     end
 
     context 'when deployment failed' do
@@ -231,11 +266,29 @@
         freeze_time do
           expect(Deployments::HooksWorker)
             .to receive(:perform_async)
-            .with(deployment_id: deployment.id, status: 'failed', status_changed_at: Time.current)
+                  .with(hash_including({ 'deployment_id' => deployment.id, 'status' => 'failed',
+                                         'status_changed_at' => Time.current.to_s }))
 
           deployment.drop!
         end
       end
+
+      context 'when improve_deployment_hooksworker_serialization is disabled' do
+        before do
+          stub_feature_flags(improve_deployment_hooksworker_serialization: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+              .to receive(:perform_async)
+                    .with(hash_including({ deployment_id: deployment.id, status: 'failed',
+status_changed_at: Time.current }))
+
+            deployment.drop!
+          end
+        end
+      end
     end
 
     context 'when deployment was canceled' do
@@ -261,11 +314,28 @@
         freeze_time do
           expect(Deployments::HooksWorker)
             .to receive(:perform_async)
-            .with(deployment_id: deployment.id, status: 'canceled', status_changed_at: Time.current)
-
+                  .with(hash_including({ 'deployment_id' => deployment.id, 'status' => 'canceled',
+                                         'status_changed_at' => Time.current.to_s }))
           deployment.cancel!
         end
       end
+
+      context 'when improve_deployment_hooksworker_serialization is disabled' do
+        before do
+          stub_feature_flags(improve_deployment_hooksworker_serialization: false)
+        end
+
+        it 'executes Deployments::HooksWorker asynchronously' do
+          freeze_time do
+            expect(Deployments::HooksWorker)
+              .to receive(:perform_async)
+                    .with(hash_including({ deployment_id: deployment.id, status: 'canceled',
+status_changed_at: Time.current }))
+
+            deployment.cancel!
+          end
+        end
+      end
     end
 
     context 'when deployment was skipped' do