Address review comments

DataDog · Sep 4, 2024 · 2f52be1 · 2f52be1
1 parent 2b732e4
commit 2f52be1
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 54 deletions.
diff --git a/packages/dd-trace/src/appsec/remote_config/manager.js b/packages/dd-trace/src/appsec/remote_config/manager.js
@@ -33,6 +33,9 @@ class RemoteConfigManager extends EventEmitter {
       port: config.port
     }))
 
+    this._handlers = new Map()
+    const appliedConfigs = this.appliedConfigs = new Map()
+
     this.scheduler = new Scheduler((cb) => this.poll(cb), pollInterval)
 
     this.state = {
@@ -70,9 +73,6 @@ class RemoteConfigManager extends EventEmitter {
       },
       cached_target_files: [] // updated by `parseConfig()`
     }
-
-    this._handlers = new Map()
-    const appliedConfigs = this.appliedConfigs = new Map()
   }
 
   updateCapabilities (mask, value) {
@@ -231,15 +231,11 @@ class RemoteConfigManager extends EventEmitter {
       this.dispatch(toApply, 'apply')
       this.dispatch(toModify, 'modify')
 
-      this.state.cached_target_files = []
-
-      for (const conf of this.appliedConfigs.values()) {
-        this.state.cached_target_files.push({
-          path: conf.path,
-          length: conf.length,
-          hashes: Object.entries(conf.hashes).map((entry) => ({ algorithm: entry[0], hash: entry[1] }))
-        })
-      }
+      this.state.cached_target_files = Array.from(this.appliedConfigs.values()).map((conf) => ({
+        path: conf.path,
+        length: conf.length,
+        hashes: Object.entries(conf.hashes).map((entry) => ({ algorithm: entry[0], hash: entry[1] }))
+      }))
     }
   }
 
@@ -248,46 +244,7 @@ class RemoteConfigManager extends EventEmitter {
       // TODO: we need a way to tell if unapply configs were handled by kPreUpdate or not, because they're always
       // emitted unlike the apply and modify configs
 
-      // in case the item was already handled by kPreUpdate
-      if (item.apply_state === UNACKNOWLEDGED || action === 'unapply') {
-        const handler = this._handlers.get(item.product)
-
-        if (handler) {
-          try {
-            if (supportsAckCallback(handler)) {
-              // If the handler accepts an `ack` callback, expect that to be called and set `apply_state` accordinly
-              // TODO: do we want to pass old and new config ?
-              handler(action, item.file, item.id, (err) => {
-                if (err) {
-                  item.apply_state = ERROR
-                  item.apply_error = err.toString()
-                } else if (item.apply_state !== ERROR) {
-                  item.apply_state = ACKNOWLEDGED
-                }
-              })
-            } else {
-              // If the handler doesn't accept an `ack` callback, assume `apply_state` is `ACKNOWLEDGED`,
-              // unless it returns a promise, in which case we wait for the promise to be resolved or rejected.
-              // TODO: do we want to pass old and new config ?
-              const result = handler(action, item.file, item.id)
-              if (result instanceof Promise) {
-                result.then(
-                  () => { item.apply_state = ACKNOWLEDGED },
-                  (err) => {
-                    item.apply_state = ERROR
-                    item.apply_error = err.toString()
-                  }
-                )
-              } else {
-                item.apply_state = ACKNOWLEDGED
-              }
-            }
-          } catch (err) {
-            item.apply_state = ERROR
-            item.apply_error = err.toString()
-          }
-        }
-      }
+      callHandlerFor(action, item)
 
       if (action === 'unapply') {
         this.appliedConfigs.delete(item.path)
@@ -319,6 +276,49 @@ function parseConfigPath (configPath) {
   }
 }
 
+function callHandlerFor (action, item) {
+  // in case the item was already handled by kPreUpdate
+  if (item.apply_state !== UNACKNOWLEDGED && action !== 'unapply') return
+
+  const handler = this._handlers.get(item.product)
+
+  if (!handler) return
+
+  try {
+    if (supportsAckCallback(handler)) {
+      // If the handler accepts an `ack` callback, expect that to be called and set `apply_state` accordinly
+      // TODO: do we want to pass old and new config ?
+      handler(action, item.file, item.id, (err) => {
+        if (err) {
+          item.apply_state = ERROR
+          item.apply_error = err.toString()
+        } else if (item.apply_state !== ERROR) {
+          item.apply_state = ACKNOWLEDGED
+        }
+      })
+    } else {
+      // If the handler doesn't accept an `ack` callback, assume `apply_state` is `ACKNOWLEDGED`,
+      // unless it returns a promise, in which case we wait for the promise to be resolved or rejected.
+      // TODO: do we want to pass old and new config ?
+      const result = handler(action, item.file, item.id)
+      if (result instanceof Promise) {
+        result.then(
+          () => { item.apply_state = ACKNOWLEDGED },
+          (err) => {
+            item.apply_state = ERROR
+            item.apply_error = err.toString()
+          }
+        )
+      } else {
+        item.apply_state = ACKNOWLEDGED
+      }
+    }
+  } catch (err) {
+    item.apply_state = ERROR
+    item.apply_error = err.toString()
+  }
+}
+
 function supportsAckCallback (handler) {
   if (kSupportsAckCallback in handler) return handler[kSupportsAckCallback]
 

diff --git a/packages/dd-trace/test/appsec/remote_config/manager.spec.js b/packages/dd-trace/test/appsec/remote_config/manager.spec.js
@@ -151,10 +151,12 @@ describe('RemoteConfigManager', () => {
 
   describe('setProductHandler/removeProductHandler', () => {
     it('should update the product list and autostart or autostop', () => {
+      expect(rc.scheduler.start).to.not.have.been.called
+
       rc.setProductHandler('ASM_FEATURES', noop)
 
       expect(rc.state.client.products).to.deep.equal(['ASM_FEATURES'])
-      expect(rc.scheduler.start).to.have.been.calledOnce
+      expect(rc.scheduler.start).to.have.been.called
 
       rc.setProductHandler('ASM_DATA', noop)
       rc.setProductHandler('ASM_DD', noop)
@@ -171,7 +173,7 @@ describe('RemoteConfigManager', () => {
 
       rc.removeProductHandler('ASM_DD')
 
-      expect(rc.scheduler.stop).to.have.been.calledOnce
+      expect(rc.scheduler.stop).to.have.been.called
       expect(rc.state.client.products).to.be.empty
     })
   })