From 2f52be13d36e5621d899385cdc011e18018d5e76 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 4 Sep 2024 11:03:30 +0200 Subject: [PATCH] Address review comments --- .../src/appsec/remote_config/manager.js | 104 +++++++++--------- .../test/appsec/remote_config/manager.spec.js | 6 +- 2 files changed, 56 insertions(+), 54 deletions(-) diff --git a/packages/dd-trace/src/appsec/remote_config/manager.js b/packages/dd-trace/src/appsec/remote_config/manager.js index b2f6babc1a9..76e3c41d0c2 100644 --- 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 index b0883049f1a..f9aea97ce08 100644 --- 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 }) })