Skip to content

Commit

Permalink
wip
Browse files Browse the repository at this point in the history
  • Loading branch information
@watson
watson committed Aug 28, 2024
1 parent 736b625 commit 46ec990
Show file tree
Hide file tree
Showing 8 changed files with 121 additions and 31 deletions.
78 changes: 72 additions & 6 deletions integration-tests/debugger/index.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
})
Expand All @@ -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',
Expand All @@ -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()
Expand All @@ -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).
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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)
Expand All @@ -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 = [[
Expand All @@ -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',
Expand All @@ -200,14 +262,18 @@ describe('Dynamic Instrumentation', function () {
assert.typeOf(diagnostics.exception.stacktrace, 'string')
}

if (expectedPayloads.length === 0) done()
endIfDone()
})

agent.addRemoteConfig({
product: 'LIVE_DEBUGGING',
id: `logProbe_${config.id}`,
config
})

function endIfDone () {
if (receivedAckUpdate && expectedPayloads.length === 0) done()
}
})
}
})
Expand Down Expand Up @@ -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).
Expand Down
20 changes: 13 additions & 7 deletions integration-tests/helpers/fake-agent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
}

Expand Down
5 changes: 3 additions & 2 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 @@ -106,7 +107,7 @@ function disableWafUpdate () {
}
}

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

module.exports = {
enable,
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
4 changes: 3 additions & 1 deletion packages/dd-trace/src/debugger/devtools_client/remote_config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
Expand Down
16 changes: 12 additions & 4 deletions packages/dd-trace/src/debugger/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
{
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 @@ -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 {
Expand All @@ -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') {
Expand All @@ -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

Expand Down
2 changes: 1 addition & 1 deletion 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

0 comments on commit 46ec990

Please sign in to comment.