From 46ec9905bb3d12057cee2011510f9e2ee730140e Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Wed, 28 Aug 2024 18:41:52 +0200 Subject: [PATCH] wip --- integration-tests/debugger/index.spec.js | 78 +++++++++++++++++-- integration-tests/helpers/fake-agent.js | 20 +++-- .../src/appsec/remote_config/index.js | 5 +- .../src/appsec/remote_config/manager.js | 18 +++-- .../debugger/devtools_client/remote_config.js | 4 +- packages/dd-trace/src/debugger/index.js | 16 +++- packages/dd-trace/src/proxy.js | 9 ++- .../test/appsec/remote_config/manager.spec.js | 2 +- 8 files changed, 121 insertions(+), 31 deletions(-) diff --git a/integration-tests/debugger/index.spec.js b/integration-tests/debugger/index.spec.js index 538b460d118..1f0c05f82bc 100644 --- a/integration-tests/debugger/index.spec.js +++ b/integration-tests/debugger/index.spec.js @@ -6,9 +6,11 @@ const getPort = require('get-port') const Axios = require('axios') const { assert } = require('chai') const { assertObjectContains, assertUUID, createSandbox, FakeAgent, spawnProc } = require('../helpers') +const { ACKNOWLEDGED, ERROR } = require('../../packages/dd-trace/src/appsec/remote_config/apply_states') const probeFile = 'debugger/target-app/index.js' const probeLineNo = 13 +const pollInterval = 1 describe('Dynamic Instrumentation', function () { let axios, sandbox, cwd, appPort, appFile, agent, proc, probeConfig @@ -37,6 +39,7 @@ describe('Dynamic Instrumentation', function () { env: { APP_PORT: appPort, DD_TRACE_AGENT_PORT: agent.port, + DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS: pollInterval, DD_EXPERIMENTAL_DYNAMIC_INSTRUMENTATION_ENABLED: true } }) @@ -58,6 +61,7 @@ describe('Dynamic Instrumentation', function () { describe('diagnostics messages', function () { it('should send expected diagnostics messages if probe is received and triggered', function (done) { + let receivedAckUpdate = false const probeId = probeConfig.config.id const expectedPayloads = [{ ddsource: 'dd_debugger', @@ -73,6 +77,16 @@ describe('Dynamic Instrumentation', function () { debugger: { diagnostics: { probeId, version: 0, status: 'EMITTING' } } }] + agent.on('remote-config-ack-update', (id, version, state, error) => { + assert.strictEqual(id, probeConfig.id) + assert.strictEqual(version, 1) + assert.strictEqual(state, ACKNOWLEDGED) + assert.notOk(error) // falsy check since error will be an empty string, but that's an implementation detail + + receivedAckUpdate = true + endIfDone() + }) + agent.on('debugger-diagnostics', async ({ payload }) => { try { const expected = expectedPayloads.shift() @@ -85,7 +99,7 @@ describe('Dynamic Instrumentation', function () { assert.deepStrictEqual(response.data, { hello: 'foo' }) } - if (expectedPayloads.length === 0) done() + endIfDone() } catch (err) { // Nessecary hack: Any errors thrown inside of an async function is invisible to Mocha unless the outer `it` // callback is also `async` (which we can't do in this case since we rely on the `done` callback). @@ -94,9 +108,14 @@ describe('Dynamic Instrumentation', function () { }) agent.addRemoteConfig(probeConfig) + + function endIfDone () { + if (receivedAckUpdate && expectedPayloads.length === 0) done() + } }) it('should send expected diagnostics messages if probe is first received and then updated', function (done) { + let receivedAckUpdates = 0 const probeId = probeConfig.config.id const expectedPayloads = [{ ddsource: 'dd_debugger', @@ -123,18 +142,33 @@ describe('Dynamic Instrumentation', function () { () => {} ] + agent.on('remote-config-ack-update', (id, version, state, error) => { + assert.strictEqual(id, probeConfig.id) + assert.strictEqual(version, ++receivedAckUpdates) + assert.strictEqual(state, ACKNOWLEDGED) + assert.notOk(error) // falsy check since error will be an empty string, but that's an implementation detail + + endIfDone() + }) + agent.on('debugger-diagnostics', ({ payload }) => { const expected = expectedPayloads.shift() assertObjectContains(payload, expected) assertUUID(payload.debugger.diagnostics.runtimeId) if (payload.debugger.diagnostics.status === 'INSTALLED') triggers.shift()() - if (expectedPayloads.length === 0) done() + endIfDone() }) agent.addRemoteConfig(probeConfig) + + function endIfDone () { + if (receivedAckUpdates === 2 && expectedPayloads.length === 0) done() + } }) it('should send expected diagnostics messages if probe is first received and then deleted', function (done) { + let receivedAckUpdate = false + let payloadsProcessed = false const probeId = probeConfig.config.id const expectedPayloads = [{ ddsource: 'dd_debugger', @@ -146,6 +180,16 @@ describe('Dynamic Instrumentation', function () { debugger: { diagnostics: { probeId, version: 0, status: 'INSTALLED' } } }] + agent.on('remote-config-ack-update', (id, version, state, error) => { + assert.strictEqual(id, probeConfig.id) + assert.strictEqual(version, 1) + assert.strictEqual(state, ACKNOWLEDGED) + assert.notOk(error) // falsy check since error will be an empty string, but that's an implementation detail + + receivedAckUpdate = true + endIfDone() + }) + agent.on('debugger-diagnostics', ({ payload }) => { const expected = expectedPayloads.shift() assertObjectContains(payload, expected) @@ -154,11 +198,18 @@ describe('Dynamic Instrumentation', function () { if (payload.debugger.diagnostics.status === 'INSTALLED') { agent.removeRemoteConfig(probeConfig.id) // Wait a little to see if we get any follow-up `debugger-diagnostics` messages - setTimeout(done, 5000) + setTimeout(() => { + payloadsProcessed = true + endIfDone() + }, pollInterval * 2 * 1000) // wait twice as long as the RC poll interval } }) agent.addRemoteConfig(probeConfig) + + function endIfDone () { + if (receivedAckUpdate && payloadsProcessed) done() + } }) const unsupporedOrInvalidProbes = [[ @@ -174,8 +225,19 @@ describe('Dynamic Instrumentation', function () { ]] for (const [title, config, costumErrorDiagnosticsObj] of unsupporedOrInvalidProbes) { - // TODO: Test that we report the error via the RC client as well it(title, function (done) { + let receivedAckUpdate = false + + agent.on('remote-config-ack-update', (id, version, state, error) => { + assert.strictEqual(id, `logProbe_${config.id}`) + assert.strictEqual(version, 1) + assert.strictEqual(state, ERROR) + assert.strictEqual(error.slice(0, 6), 'Error:') + + receivedAckUpdate = true + endIfDone() + }) + const probeId = config.id const expectedPayloads = [{ ddsource: 'dd_debugger', @@ -200,7 +262,7 @@ describe('Dynamic Instrumentation', function () { assert.typeOf(diagnostics.exception.stacktrace, 'string') } - if (expectedPayloads.length === 0) done() + endIfDone() }) agent.addRemoteConfig({ @@ -208,6 +270,10 @@ describe('Dynamic Instrumentation', function () { id: `logProbe_${config.id}`, config }) + + function endIfDone () { + if (receivedAckUpdate && expectedPayloads.length === 0) done() + } }) } }) @@ -295,7 +361,7 @@ describe('Dynamic Instrumentation', function () { // We want to wait enough time to see if the client triggers on the breakpoint so that the test can fail // if it does, but not so long that the test times out. // TODO: Is there some signal we can use instead of a timer? - setTimeout(done, 5000) + setTimeout(done, pollInterval * 2 * 1000) // wait twice as long as the RC poll interval } catch (err) { // Nessecary hack: Any errors thrown inside of an async function is invisible to Mocha unless the outer // `it` callback is also `async` (which we can't do in this case since we rely on the `done` callback). diff --git a/integration-tests/helpers/fake-agent.js b/integration-tests/helpers/fake-agent.js index fa6394f7c96..05a59fa17b2 100644 --- a/integration-tests/helpers/fake-agent.js +++ b/integration-tests/helpers/fake-agent.js @@ -13,8 +13,7 @@ module.exports = class FakeAgent extends EventEmitter { constructor (port = 0) { super() this.port = port - this._rcFiles = {} - this._rcTargetsVersion = 0 + this.resetRemoteConfig() } async start () { @@ -95,11 +94,12 @@ module.exports = class FakeAgent extends EventEmitter { } /** - * Remove any existing config added by calls to FakeAgent#addRemoteConfig. + * Reset any existing Remote Config state. Usefull in `before` and `beforeEach` blocks. */ resetRemoteConfig () { this._rcFiles = {} - this._rcTargetsVersion++ + this._rcTargetsVersion = 0 + this._rcSeenStates = new Set() } // **resolveAtFirstSuccess** - specific use case for Next.js (or any other future libraries) @@ -216,10 +216,16 @@ function buildExpressServer (agent) { console.error(state.error) // eslint-disable-line no-console } - for (const { apply_error: error } of state.config_states) { - if (error) { + for (const cs of state.config_states) { + const uniqueState = `${cs.id}-${cs.version}-${cs.apply_state}` + if (!agent._rcSeenStates.has(uniqueState)) { + agent.emit('remote-config-ack-update', cs.id, cs.version, cs.apply_state, cs.apply_error) + agent._rcSeenStates.add(uniqueState) + } + + if (cs.apply_error) { // Print the error sent by the client in case it's useful in debugging tests - console.error(error) // eslint-disable-line no-console + console.error(cs.apply_error) // eslint-disable-line no-console } } diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 169e5c2dff7..64020f2aa48 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) { @@ -106,7 +107,7 @@ function disableWafUpdate () { } } -function noop () {} +function noop (a, b, c, ack) { ack() } module.exports = { enable, 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/debugger/devtools_client/remote_config.js b/packages/dd-trace/src/debugger/devtools_client/remote_config.js index 928db448394..4b61429b2da 100644 --- a/packages/dd-trace/src/debugger/devtools_client/remote_config.js +++ b/packages/dd-trace/src/debugger/devtools_client/remote_config.js @@ -35,10 +35,12 @@ let sessionStarted = false // sampling: { snapshotsPerSecond: 5000 }, // evaluateAt: 'EXIT' // only used for method probes // } -rcPort.on('message', async ({ action, conf: probe }) => { +rcPort.on('message', async ({ action, conf: probe, ackId }) => { try { await processMsg(action, probe) + rcPort.postMessage({ ackId }) } catch (err) { + rcPort.postMessage({ ackId, error: err }) ackError(err, probe) } }) diff --git a/packages/dd-trace/src/debugger/index.js b/packages/dd-trace/src/debugger/index.js index 18019c16a83..9e5cf22fd34 100644 --- a/packages/dd-trace/src/debugger/index.js +++ b/packages/dd-trace/src/debugger/index.js @@ -17,13 +17,21 @@ function start (config, rc) { log.debug('Starting Dynamic Instrumentation client...') - rc.on('LIVE_DEBUGGING', (action, conf) => { - rcChannel.port2.postMessage({ action, conf }) - }) - + const rcAckCallbacks = new Map() const rcChannel = new MessageChannel() configChannel = new MessageChannel() + rc.on('LIVE_DEBUGGING', (action, conf, id, ack) => { + const ackId = `${id}-${conf.version}` + rcAckCallbacks.set(ackId, ack) + rcChannel.port2.postMessage({ action, conf, ackId }) + }) + + rcChannel.port2.on('message', ({ ackId, error }) => { + rcAckCallbacks.get(ackId)(error) + rcAckCallbacks.delete(ackId) + }) + worker = new Worker( join(__dirname, 'devtools_client', 'index.js'), { diff --git a/packages/dd-trace/src/proxy.js b/packages/dd-trace/src/proxy.js index 241141588c1..9c679e77045 100644 --- a/packages/dd-trace/src/proxy.js +++ b/packages/dd-trace/src/proxy.js @@ -84,7 +84,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 { @@ -93,7 +94,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') { @@ -104,7 +106,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/manager.spec.js b/packages/dd-trace/test/appsec/remote_config/manager.spec.js index 8e5fdc6b516..166d2f0bf49 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