Merge pull request #2820 from DataDog/appsec-fix-flaky-test

fix flaky test by using expect API to make sure the original code doe…

lib/datadog/appsec/component.rb

@@ -53,7 +53,7 @@ def reconfigure(ruleset:)
           if new && new.ready?
             old = @processor
             @processor = new
-            old.finalize
+            old.finalize if old
           end
         end
       end

lib/datadog/appsec/contrib/rack/request_middleware.rb

@@ -23,12 +23,11 @@ def initialize(app, opt = {})
             @oneshot_tags_sent = false
           end
 
-          # rubocop:disable Metrics/AbcSize,Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength
+          # rubocop:disable Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength
           def call(env)
             return @app.call(env) unless Datadog::AppSec.enabled?
 
             Datadog::Core::Remote.active_remote.barrier(:once) unless Datadog::Core::Remote.active_remote.nil?
-            processor = Datadog::AppSec.processor
 
             processor = nil
             ready = false
@@ -89,7 +88,7 @@ def call(env)
               processor.deactivate_context
             end
           end
-          # rubocop:enable Metrics/AbcSize,Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength
+          # rubocop:enable Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength
 
           private
 

spec/datadog/appsec/component_spec.rb

@@ -70,6 +70,49 @@
   end
 
   describe '#reconfigure' do
+    let(:ruleset) do
+      {
+        'exclusions' => [{
+          'conditions' => [{
+            'operator' => 'ip_match',
+            'parameters' => {
+              'inputs' => [{
+                'address' => 'http.client_ip'
+              }]
+            }
+          }]
+        }],
+        'metadata' => {
+          'rules_version' => '1.5.2'
+        },
+        'rules' => [{
+          'conditions' => [{
+            'operator' => 'ip_match',
+            'parameters' => {
+              'data' => 'blocked_ips',
+              'inputs' => [{
+                'address' => 'http.client_ip'
+              }]
+            }
+          }],
+          'id' => 'blk-001-001',
+          'name' => 'Block IP Addresses',
+          'on_match' => ['block'],
+          'tags' => {
+            'category' => 'security_response', 'type' => 'block_ip'
+          },
+          'transformers' => []
+        }],
+        'rules_data' => [{
+          'data' => [{
+            'expiration' => 1678972458,
+            'value' => '42.42.42.1'
+          }]
+        }],
+        'version' => '2.2'
+      }
+    end
+
     context 'lock' do
       it 'makes sure to synchronize' do
         mutex = Mutex.new
@@ -88,47 +131,6 @@
 
         old_processor = component.processor
 
-        ruleset = {
-          'exclusions' => [{
-            'conditions' => [{
-              'operator' => 'ip_match',
-              'parameters' => {
-                'inputs' => [{
-                  'address' => 'http.client_ip'
-                }]
-              }
-            }]
-          }],
-          'metadata' => {
-            'rules_version' => '1.5.2'
-          },
-          'rules' => [{
-            'conditions' => [{
-              'operator' => 'ip_match',
-              'parameters' => {
-                'data' => 'blocked_ips',
-                'inputs' => [{
-                  'address' => 'http.client_ip'
-                }]
-              }
-            }],
-            'id' => 'blk-001-001',
-            'name' => 'Block IP Addresses',
-            'on_match' => ['block'],
-            'tags' => {
-              'category' => 'security_response', 'type' => 'block_ip'
-            },
-            'transformers' => []
-          }],
-          'rules_data' => [{
-            'data' => [{
-              'expiration' => 1678972458,
-              'value' => '42.42.42.1'
-            }]
-          }],
-          'version' => '2.2'
-        }
-
         expect(old_processor).to receive(:finalize)
         component.reconfigure(ruleset: ruleset)
         new_processor = component.processor
@@ -137,6 +139,21 @@
       end
     end
 
+    context 'when the new processor is ready, and old processor is nil' do
+      it 'swaps the processor instance and do not finalize the old processor' do
+        processor = nil
+        component = described_class.new(processor: processor)
+
+        old_processor = component.processor
+
+        expect(old_processor).to_not receive(:finalize)
+        component.reconfigure(ruleset: ruleset)
+        new_processor = component.processor
+        expect(new_processor).to_not eq(old_processor)
+        new_processor.finalize
+      end
+    end
+
     context 'when the new processor is not ready' do
       it 'does not swap the processor instance and finalize the old processor' do
         processor = instance_double(Datadog::AppSec::Processor)

spec/datadog/appsec/remote_spec.rb

@@ -158,18 +158,24 @@
           }
 
           expect(Datadog::AppSec).to receive(:reconfigure).with(ruleset: expected_ruleset)
+            .and_return(nil)
           changes = transaction
           receiver.call(repository, changes)
         end
 
         context 'when there is no ASM_DD information' do
           let(:transaction) { repository.transaction { |repository, transaction| } }
+
           it 'uses the rules from the appsec settings' do
+            ruleset = 'foo'
+
             expect(Datadog::AppSec::Processor::RuleLoader).to receive(:load_rules).with(
               ruleset: Datadog.configuration.appsec.ruleset
-            ).at_least(:once).and_call_original
+            ).and_return(ruleset)
 
             changes = transaction
+            expect(Datadog::AppSec).to receive(:reconfigure).with(ruleset: ruleset)
+              .and_return(nil)
             receiver.call(repository, changes)
           end