Skip to content

Commit

Permalink
Allow Remote Config acknowledgements to be async
Browse files Browse the repository at this point in the history
  • Loading branch information
@watson
watson committed Aug 28, 2024
1 parent c8e6070 commit 07e9704
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 47 deletions.
10 changes: 6 additions & 4 deletions packages/dd-trace/src/appsec/remote_config/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -106,6 +107,7 @@ function disableWafUpdate () {
}
}

function ackNoop (a, b, c, ack) { ack() }
function noop () {}

module.exports = {
Expand Down
18 changes: 11 additions & 7 deletions packages/dd-trace/src/appsec/remote_config/manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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()
Expand Down
9 changes: 6 additions & 3 deletions packages/dd-trace/src/proxy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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') {
Expand All @@ -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

Expand Down
37 changes: 27 additions & 10 deletions packages/dd-trace/test/appsec/remote_config/index.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down Expand Up @@ -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()
})
})

Expand Down Expand Up @@ -165,49 +176,54 @@ 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', () => {
listener('apply', {
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', () => {
listener('apply', {
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', () => {
listener('apply', {
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', () => {
listener('apply', {
api_security: {
request_sample_rate: 'not_a_number'
}
})
}, id, ack)

expect(apiSecuritySampler.configure).to.not.be.called
expect(ack).to.have.been.calledOnceWithExactly()
})
})

Expand All @@ -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()
})
})
})
Expand Down
45 changes: 31 additions & 14 deletions packages/dd-trace/test/appsec/remote_config/manager.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = [
{
Expand All @@ -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', () => {
Expand All @@ -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')
}
})
})

Expand Down
Loading

0 comments on commit 07e9704

Please sign in to comment.