diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 169e5c2dff7..35066d139e2 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -28,7 +28,8 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) } - rc.on('ASM_FEATURES', (action, rcConfig) => { + rc.on('ASM_FEATURES', (action, rcConfig, id, ack) => { + ack() if (!rcConfig) return if (activation === Activation.ONECLICK) { @@ -76,9 +77,9 @@ function enableWafUpdate (appsecConfig) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_CUSTOM_BLOCKING_RESPONSE, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_TRUSTED_IPS, true) - rc.on('ASM_DATA', noop) - rc.on('ASM_DD', noop) - rc.on('ASM', noop) + rc.on('ASM_DATA', ackNoop) + rc.on('ASM_DD', ackNoop) + rc.on('ASM', ackNoop) rc.on(RemoteConfigManager.kPreUpdate, RuleManager.updateWafFromRC) } @@ -106,6 +107,7 @@ function disableWafUpdate () { } } +function ackNoop (a, b, c, ack) { ack() } function noop () {} module.exports = { diff --git a/packages/dd-trace/src/appsec/remote_config/manager.js b/packages/dd-trace/src/appsec/remote_config/manager.js index 5b0044e2c71..667d64dc73c 100644 --- a/packages/dd-trace/src/appsec/remote_config/manager.js +++ b/packages/dd-trace/src/appsec/remote_config/manager.js @@ -236,8 +236,9 @@ class RemoteConfigManager extends EventEmitter { id: conf.id, version: conf.version, product: conf.product, - apply_state: conf.apply_state, - apply_error: conf.apply_error + // Use getters so `conf.apply_*` can be updated async and still affect the new object pushed to config_states + get apply_state () { return conf.apply_state }, + get apply_error () { return conf.apply_error } }) this.state.cached_target_files.push({ @@ -258,11 +259,14 @@ class RemoteConfigManager extends EventEmitter { if (item.apply_state === UNACKNOWLEDGED || action === 'unapply') { try { // TODO: do we want to pass old and new config ? - const hadListeners = this.emit(item.product, action, item.file, item.id) - - if (hadListeners) { - item.apply_state = ACKNOWLEDGED - } + this.emit(item.product, 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 + } + }) } catch (err) { item.apply_state = ERROR item.apply_error = err.toString() diff --git a/packages/dd-trace/src/proxy.js b/packages/dd-trace/src/proxy.js index c3b865226ed..2a8218c5e5c 100644 --- a/packages/dd-trace/src/proxy.js +++ b/packages/dd-trace/src/proxy.js @@ -83,7 +83,8 @@ class Tracer extends NoopProxy { if (config.remoteConfig.enabled && !config.isCiVisibility) { const rc = remoteConfig.enable(config, this._modules.appsec) - rc.on('APM_TRACING', (action, conf) => { + rc.on('APM_TRACING', (action, conf, id, ack) => { + ack() if (action === 'unapply') { config.configure({}, true) } else { @@ -92,7 +93,8 @@ class Tracer extends NoopProxy { this._enableOrDisableTracing(config) }) - rc.on('AGENT_CONFIG', (action, conf) => { + rc.on('AGENT_CONFIG', (action, conf, id, ack) => { + ack() if (!conf?.name?.startsWith('flare-log-level.')) return if (action === 'unapply') { @@ -103,7 +105,8 @@ class Tracer extends NoopProxy { } }) - rc.on('AGENT_TASK', (action, conf) => { + rc.on('AGENT_TASK', (action, conf, id, ack) => { + ack() if (action === 'unapply' || !conf) return if (conf.task_type !== 'tracer_flare' || !conf.args) return diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index d954e41e15b..e436f261ad2 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -50,6 +50,13 @@ describe('Remote Config index', () => { }) describe('enable', () => { + let ack + const id = 1 + + beforeEach(() => { + ack = sinon.spy() + }) + it('should listen to remote config when appsec is not explicitly configured', () => { config.appsec = { enabled: undefined } @@ -116,28 +123,32 @@ describe('Remote Config index', () => { }) it('should enable appsec when listener is called with apply and enabled', () => { - listener('apply', { asm: { enabled: true } }) + listener('apply', { asm: { enabled: true } }, id, ack) expect(appsec.enable).to.have.been.called + expect(ack).to.have.been.calledOnceWithExactly() }) it('should enable appsec when listener is called with modify and enabled', () => { - listener('modify', { asm: { enabled: true } }) + listener('modify', { asm: { enabled: true } }, id, ack) expect(appsec.enable).to.have.been.called + expect(ack).to.have.been.calledOnceWithExactly() }) it('should disable appsec when listener is called with unnaply and enabled', () => { - listener('unnaply', { asm: { enabled: true } }) + listener('unnaply', { asm: { enabled: true } }, id, ack) expect(appsec.disable).to.have.been.calledOnce + expect(ack).to.have.been.calledOnceWithExactly() }) it('should not do anything when listener is called with apply and malformed data', () => { - listener('apply', {}) + listener('apply', {}, id, ack) expect(appsec.enable).to.not.have.been.called expect(appsec.disable).to.not.have.been.called + expect(ack).to.have.been.calledOnceWithExactly() }) }) @@ -165,9 +176,10 @@ describe('Remote Config index', () => { api_security: { request_sample_rate: 0.5 } - }) + }, id, ack) expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0.5) + expect(ack).to.have.been.calledOnceWithExactly() }) it('should update apiSecuritySampler config and disable it', () => { @@ -175,9 +187,10 @@ describe('Remote Config index', () => { api_security: { request_sample_rate: 0 } - }) + }, id, ack) expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0) + expect(ack).to.have.been.calledOnceWithExactly() }) it('should not update apiSecuritySampler config with values greater than 1', () => { @@ -185,9 +198,10 @@ describe('Remote Config index', () => { api_security: { request_sample_rate: 5 } - }) + }, id, ack) expect(apiSecuritySampler.configure).to.not.be.called + expect(ack).to.have.been.calledOnceWithExactly() }) it('should not update apiSecuritySampler config with values less than 0', () => { @@ -195,9 +209,10 @@ describe('Remote Config index', () => { api_security: { request_sample_rate: -0.4 } - }) + }, id, ack) expect(apiSecuritySampler.configure).to.not.be.called + expect(ack).to.have.been.calledOnceWithExactly() }) it('should not update apiSecuritySampler config with incorrect values', () => { @@ -205,9 +220,10 @@ describe('Remote Config index', () => { api_security: { request_sample_rate: 'not_a_number' } - }) + }, id, ack) expect(apiSecuritySampler.configure).to.not.be.called + expect(ack).to.have.been.calledOnceWithExactly() }) }) @@ -234,9 +250,10 @@ describe('Remote Config index', () => { api_security: { request_sample_rate: 0.5 } - }) + }, id, ack) expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0.5) + expect(ack).to.have.been.calledOnceWithExactly() }) }) }) 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 8e5fdc6b516..d0858583737 100644 --- a/packages/dd-trace/test/appsec/remote_config/manager.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/manager.spec.js @@ -3,7 +3,7 @@ const Capabilities = require('../../../src/appsec/remote_config/capabilities') const { UNACKNOWLEDGED, ACKNOWLEDGED, ERROR } = require('../../../src/appsec/remote_config/apply_states') -const noop = () => {} +const noop = (a, b, c, ack) => { ack() } describe('RemoteConfigManager', () => { let uuid @@ -557,9 +557,10 @@ describe('RemoteConfigManager', () => { }) it('should call emit for each config, catch errors, and update the state', () => { - rc.emit.onFirstCall().returns(true) - rc.emit.onSecondCall().throws(new Error('Unable to apply config')) - rc.emit.onThirdCall().returns(true) + rc.emit + .onCall(0).yields() + .onCall(1).yields(new Error('foo')) + .onCall(2).throws(new Error('bar')) const list = [ { @@ -585,27 +586,38 @@ describe('RemoteConfigManager', () => { apply_state: UNACKNOWLEDGED, apply_error: '', file: { rules: [4, 5, 6] } + }, + { + id: 'asm_dd_rules', + path: 'datadog/42/ASM_DD_RULES/confId/config', + product: 'ASM_DD_RULES', + apply_state: UNACKNOWLEDGED, + apply_error: '', + file: { rules: [7, 8, 9] } } ] rc.dispatch(list, 'apply') - expect(rc.emit).to.have.been.calledThrice - expect(rc.emit.firstCall).to.have.been - .calledWithExactly('ASM_FEATURES', 'apply', { asm: { enabled: true } }, 'asm_features') - expect(rc.emit.secondCall).to.have.been.calledWithExactly('ASM_DATA', 'apply', { data: [1, 2, 3] }, 'asm_data') - expect(rc.emit.thirdCall).to.have.been.calledWithExactly('ASM_DD', 'apply', { rules: [4, 5, 6] }, 'asm_dd') + expect(rc.emit).to.have.callCount(4) + assertRCEmitCallArguments(rc.emit.getCall(0), 'ASM_FEATURES', 'apply', { asm: { enabled: true } }, 'asm_features') + assertRCEmitCallArguments(rc.emit.getCall(1), 'ASM_DATA', 'apply', { data: [1, 2, 3] }, 'asm_data') + assertRCEmitCallArguments(rc.emit.getCall(2), 'ASM_DD', 'apply', { rules: [4, 5, 6] }, 'asm_dd') + assertRCEmitCallArguments(rc.emit.getCall(3), 'ASM_DD_RULES', 'apply', { rules: [7, 8, 9] }, 'asm_dd_rules') expect(list[0].apply_state).to.equal(ACKNOWLEDGED) expect(list[0].apply_error).to.equal('') expect(list[1].apply_state).to.equal(ERROR) - expect(list[1].apply_error).to.equal('Error: Unable to apply config') - expect(list[2].apply_state).to.equal(ACKNOWLEDGED) - expect(list[2].apply_error).to.equal('') + expect(list[1].apply_error).to.equal('Error: foo') + expect(list[2].apply_state).to.equal(ERROR) + expect(list[2].apply_error).to.equal('Error: bar') + expect(list[3].apply_state).to.equal(UNACKNOWLEDGED) + expect(list[3].apply_error).to.equal('') expect(rc.appliedConfigs.get('datadog/42/ASM_FEATURES/confId/config')).to.equal(list[0]) expect(rc.appliedConfigs.get('datadog/42/ASM_DATA/confId/config')).to.equal(list[1]) expect(rc.appliedConfigs.get('datadog/42/ASM_DD/confId/config')).to.equal(list[2]) + expect(rc.appliedConfigs.get('datadog/42/ASM_DD_RULES/confId/config')).to.equal(list[3]) }) it('should delete config from state when action is unapply', () => { @@ -620,10 +632,15 @@ describe('RemoteConfigManager', () => { rc.dispatch([rc.appliedConfigs.get('datadog/42/ASM_FEATURES/confId/config')], 'unapply') - expect(rc.emit).to.have.been - .calledOnceWithExactly('ASM_FEATURES', 'unapply', { asm: { enabled: true } }, 'asm_data') + assertRCEmitCallArguments(rc.emit.firstCall, 'ASM_FEATURES', 'unapply', { asm: { enabled: true } }, 'asm_data') expect(rc.appliedConfigs).to.be.empty }) + + function assertRCEmitCallArguments (call, ...expectedArgs) { + expect(call).to.have.been.calledWith(...expectedArgs) + expect(call.args.length).to.equal(expectedArgs.length + 1) + expect(call.args[call.args.length - 1]).to.be.a('function') + } }) }) diff --git a/packages/dd-trace/test/proxy.spec.js b/packages/dd-trace/test/proxy.spec.js index b7df0c6a647..b371636b8a0 100644 --- a/packages/dd-trace/test/proxy.spec.js +++ b/packages/dd-trace/test/proxy.spec.js @@ -250,18 +250,23 @@ describe('TracerProxy', () => { it('should support applying remote config', () => { const conf = {} + const id = 1 + const ack = sinon.spy() proxy.init() - rc.emit('APM_TRACING', 'apply', { lib_config: conf }) + rc.emit('APM_TRACING', 'apply', { lib_config: conf }, id, ack) expect(config.configure).to.have.been.calledWith(conf) expect(tracer.configure).to.have.been.calledWith(config) expect(pluginManager.configure).to.have.been.calledWith(config) + expect(ack).to.have.been.calledOnceWithExactly() }) it('should support enabling debug logs for tracer flares', () => { const logLevel = 'debug' + const id = 1 + const ack = sinon.spy() proxy.init() @@ -270,10 +275,11 @@ describe('TracerProxy', () => { log_level: logLevel }, name: 'flare-log-level.debug' - }) + }, id, ack) expect(flare.enable).to.have.been.calledWith(config) expect(flare.prepare).to.have.been.calledWith(logLevel) + expect(ack).to.have.been.calledOnceWithExactly() }) it('should support sending tracer flares', () => { @@ -282,6 +288,8 @@ describe('TracerProxy', () => { hostname: 'myhostname', user_handle: 'user.name@datadoghq.com' } + const id = 1 + const ack = sinon.spy() proxy.init() @@ -289,10 +297,11 @@ describe('TracerProxy', () => { args: task, task_type: 'tracer_flare', uuid: 'd53fc8a4-8820-47a2-aa7d-d565582feb81' - }) + }, id, ack) expect(flare.enable).to.have.been.calledWith(config) expect(flare.send).to.have.been.calledWith(task) + expect(ack).to.have.been.calledOnceWithExactly() }) it('should cleanup flares when the config is removed', () => { @@ -302,13 +311,17 @@ describe('TracerProxy', () => { }, name: 'flare-log-level.debug' } + const id = 1 + const ack = sinon.spy() proxy.init() - rc.emit('AGENT_CONFIG', 'apply', conf) - rc.emit('AGENT_CONFIG', 'unapply', conf) + rc.emit('AGENT_CONFIG', 'apply', conf, id, ack) + rc.emit('AGENT_CONFIG', 'unapply', conf, id, ack) expect(flare.disable).to.have.been.called + expect(ack).to.have.callCount(2) + expect(ack).to.have.been.calledWithExactly() }) it('should support applying remote config', () => { @@ -319,6 +332,8 @@ describe('TracerProxy', () => { './appsec/remote_config': remoteConfig, './appsec/sdk': AppsecSdk }) + const id = 1 + const ack = sinon.spy() const remoteConfigProxy = new RemoteConfigProxy() remoteConfigProxy.init() @@ -328,16 +343,19 @@ describe('TracerProxy', () => { expect(iast.enable).to.not.have.been.called let conf = { tracing_enabled: false } - rc.emit('APM_TRACING', 'apply', { lib_config: conf }) + rc.emit('APM_TRACING', 'apply', { lib_config: conf }, id, ack) expect(appsec.disable).to.not.have.been.called expect(iast.disable).to.not.have.been.called + expect(ack).to.have.been.calledOnceWithExactly() conf = { tracing_enabled: true } - rc.emit('APM_TRACING', 'apply', { lib_config: conf }) + ack.resetHistory() + rc.emit('APM_TRACING', 'apply', { lib_config: conf }, id, ack) expect(DatadogTracer).to.have.been.calledOnce expect(AppsecSdk).to.have.been.calledOnce expect(appsec.enable).to.not.have.been.called expect(iast.enable).to.not.have.been.called + expect(ack).to.have.been.calledOnceWithExactly() }) it('should support applying remote config (only call disable if enabled before)', () => { @@ -349,6 +367,8 @@ describe('TracerProxy', () => { './appsec/remote_config': remoteConfig, './appsec/sdk': AppsecSdk }) + const id = 1 + const ack = sinon.spy() config.telemetry = {} config.appsec.enabled = true @@ -364,16 +384,19 @@ describe('TracerProxy', () => { expect(iast.enable).to.have.been.calledOnceWithExactly(config, tracer) let conf = { tracing_enabled: false } - rc.emit('APM_TRACING', 'apply', { lib_config: conf }) + rc.emit('APM_TRACING', 'apply', { lib_config: conf }, id, ack) expect(appsec.disable).to.have.been.called expect(iast.disable).to.have.been.called + expect(ack).to.have.been.calledOnceWithExactly() conf = { tracing_enabled: true } - rc.emit('APM_TRACING', 'apply', { lib_config: conf }) + ack.resetHistory() + rc.emit('APM_TRACING', 'apply', { lib_config: conf }, id, ack) expect(appsec.enable).to.have.been.calledTwice expect(appsec.enable.secondCall).to.have.been.calledWithExactly(config) expect(iast.enable).to.have.been.calledTwice expect(iast.enable.secondCall).to.have.been.calledWithExactly(config, tracer) + expect(ack).to.have.been.calledOnceWithExactly() }) it('should start capturing runtimeMetrics when configured', () => {