Add runtime guardrail for SSI #4319

bengl · 2024-05-16T19:52:38Z

When in SSI, bail out if our version of Node.js is incompatible.

github-actions · 2024-05-16T19:53:18Z

Overall package size

Self size: 6.73 MB
Deduped: 61.99 MB
No deduping: 62.27 MB

Dependency sizes

name	version	self size	total size
@datadog/native-appsec	8.0.1	15.59 MB	15.6 MB
@datadog/native-iast-taint-tracking	2.1.0	14.91 MB	14.92 MB
@datadog/pprof	5.3.0	9.85 MB	10.22 MB
protobufjs	7.2.5	2.77 MB	6.56 MB
@datadog/native-iast-rewriter	2.3.1	2.15 MB	2.24 MB
@opentelemetry/core	1.14.0	872.87 kB	1.47 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
@opentelemetry/api	1.8.0	1.21 MB	1.21 MB
import-in-the-middle	1.8.1	71.67 kB	741.34 kB
msgpack-lite	0.1.26	201.16 kB	281.59 kB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.5.4	93.4 kB	123.8 kB
pprof-format	2.1.0	111.69 kB	111.69 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ignore	5.2.4	51.22 kB	51.22 kB
int64-buffer	0.1.10	49.18 kB	49.18 kB
shell-quote	1.8.1	44.96 kB	44.96 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
tlhunter-sorted-set	0.1.0	24.94 kB	24.94 kB
limiter	1.1.5	23.17 kB	23.17 kB
dc-polyfill	0.1.4	23.1 kB	23.1 kB
retry	0.13.1	18.85 kB	18.85 kB
jest-docblock	29.7.0	8.99 kB	12.76 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

pr-commenter · 2024-05-16T19:59:44Z

Benchmarks

Benchmark execution time: 2024-06-25 19:25:41

Comparing candidate commit acb6c1e in PR branch bengl/runtime-guardrails with baseline commit bc36d27 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics.

simon-id · 2024-05-17T15:01:26Z

integration-tests/init.spec.js

-    it('should initialize the tracer, if DD_INJECTION_ENABLED', () => {
-      return runTest(cwd, { NODE_OPTIONS, DD_INJECTION_ENABLED }, 'true\n')
+    context('when node version is more than engines field', () => {
+      const FAKE_VERSION = '50000.0.0'


make sure to update this in april of the year 27002, when node 50000 releases

Maybe I should use Number.MAX_SAFE_INTEGER instead?

codecov · 2024-06-17T04:33:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.19%. Comparing base (b1f1f85) to head (077923f).
Report is 15 commits behind head on master.

❗ Current head 077923f differs from pull request most recent head acb6c1e

Please upload reports for the commit acb6c1e to get more accurate results.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4319       +/-   ##
===========================================
- Coverage   92.64%   69.19%   -23.46%     
===========================================
  Files         116        1      -115     
  Lines        4173      198     -3975     
  Branches       33       33               
===========================================
- Hits         3866      137     -3729     
+ Misses        307       61      -246

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/dd-trace/src/telemetry/init-telemetry.js

rochdev · 2024-06-18T18:51:59Z

integration-tests/init/setversion.js

+
+const desc = Reflect.getOwnPropertyDescriptor(process.versions, 'node')
+
+desc.value = process.env.FAKE_VERSION


Let's use a real old version instead. The version number is not what would typically cause issues, but rather things that we do that could be unsupported in older versions, and that includes the bailout logic.

@rochdev Would one version prior to the current bottom of the engines field range suffice?

To be honest I'd probably go with as low as we possibly can for the odd Node 4/6/8 that may try installing this, especially since it should be easy to support these versions for just the bailout script.

New code looks good to me, but I'd say the minimum Node version should be lowered to at the very least 8, but if we can push 4 then that's even better. Both of those won't work on Apple Silicon though, so maybe the choice could be platform specific? In any case, this is not a blocker, but definitely something we should do, otherwise we have no idea if this works on older versions which may still fail.

integration-tests/init.spec.js

szegedi

LGTM, one small nit.

szegedi · 2024-06-19T10:12:27Z

packages/dd-trace/src/telemetry/init-telemetry.js

+  if (points.length === 0) {
+    return
+  }


You could move this before forEach

szegedi · 2024-06-20T09:08:23Z

init.js


 let initBailout = false
+let clobberBailout = false
+const forced = ['1', 'true', 'True'].includes(process.env.DD_INJECT_FORCE)


I often see this ['1', 'true', 'True'].includes(…) idiom. We do have an isTrue function in util.js though – could you use that instead for consistency? It also accepts the word "true" in any combination of letter cases and while "tRuE" is arguably weird, "TRUE" is reasonable.

Suggested change

const forced = ['1', 'true', 'True'].includes(process.env.DD_INJECT_FORCE)

const forced = isTrue(process.env.DD_INJECT_FORCE)

Even better, since you're creating a Config object on line 10 anyway you could let it read the env variable and expose its evaluation result as a property of the config object. That way the config object is the only source of configuration for all of the rest of the codebase. There's already some processing in there for e.g. DD_INJECTION_ENABLED that I added for profiler's needs, but you'd be welcome to generalize it, and then you could use a Config property for that as well.

* Add guardrails for SSI * PR feedback * test on a swath of versions, in CI only

bengl requested a review from a team as a code owner May 16, 2024 19:52

bengl added the semver-minor label May 16, 2024

bengl force-pushed the bengl/runtime-guardrails branch from 74ff0c6 to 7a88c4f Compare May 16, 2024 20:08

Qard previously approved these changes May 17, 2024

View reviewed changes

simon-id reviewed May 17, 2024

View reviewed changes

simon-id previously approved these changes May 17, 2024

View reviewed changes

bengl marked this pull request as draft May 17, 2024 20:46

bengl dismissed stale reviews from simon-id and Qard via aefbd02 June 3, 2024 18:48

bengl force-pushed the bengl/runtime-guardrails branch from aefbd02 to 947e5d1 Compare June 3, 2024 19:26

bengl force-pushed the bengl/runtime-guardrails branch 3 times, most recently from dea462d to 95948d8 Compare June 17, 2024 04:27

bengl force-pushed the bengl/runtime-guardrails branch 3 times, most recently from 7b20ebf to 077923f Compare June 17, 2024 18:27

Qard force-pushed the master branch from 7539556 to 5be6314 Compare June 17, 2024 23:42

bengl force-pushed the bengl/runtime-guardrails branch from 077923f to eb145ba Compare June 18, 2024 03:39

bengl marked this pull request as ready for review June 18, 2024 17:42

bengl requested a review from a team as a code owner June 18, 2024 17:42

rochdev requested changes Jun 18, 2024

View reviewed changes

bengl force-pushed the bengl/runtime-guardrails branch 2 times, most recently from 944755a to 98620da Compare June 18, 2024 21:11

szegedi previously approved these changes Jun 19, 2024

View reviewed changes

rochdev previously approved these changes Jun 19, 2024

View reviewed changes

szegedi reviewed Jun 20, 2024

View reviewed changes

Qard previously approved these changes Jun 21, 2024

View reviewed changes

bengl dismissed stale reviews from Qard, rochdev, and szegedi via 146dbd8 June 21, 2024 21:26

bengl force-pushed the bengl/runtime-guardrails branch 3 times, most recently from adc1630 to 0b093b4 Compare June 25, 2024 16:52

bengl added 3 commits June 25, 2024 15:17

Add guardrails for SSI

61ba040

PR feedback

afe9a4d

test on a swath of versions, in CI only

acb6c1e

bengl force-pushed the bengl/runtime-guardrails branch from 0b093b4 to acb6c1e Compare June 25, 2024 19:17

rochdev approved these changes Jun 26, 2024

View reviewed changes

bengl merged commit 413df1e into master Jun 26, 2024
144 checks passed

bengl deleted the bengl/runtime-guardrails branch June 26, 2024 14:22

juan-fernandez pushed a commit that referenced this pull request Jul 10, 2024

Add runtime guardrail for SSI (#4319)

3f0e5bb

* Add guardrails for SSI * PR feedback * test on a swath of versions, in CI only

juan-fernandez pushed a commit that referenced this pull request Jul 10, 2024

Add runtime guardrail for SSI (#4319)

a800afe

* Add guardrails for SSI * PR feedback * test on a swath of versions, in CI only

juan-fernandez pushed a commit that referenced this pull request Jul 11, 2024

Add runtime guardrail for SSI (#4319)

128e4e8

* Add guardrails for SSI * PR feedback * test on a swath of versions, in CI only

juan-fernandez pushed a commit that referenced this pull request Jul 11, 2024

Add runtime guardrail for SSI (#4319)

43f7974

* Add guardrails for SSI * PR feedback * test on a swath of versions, in CI only

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add runtime guardrail for SSI #4319

Add runtime guardrail for SSI #4319

bengl commented May 16, 2024

github-actions bot commented May 16, 2024 •

edited

Loading

pr-commenter bot commented May 16, 2024 •

edited

Loading

simon-id May 17, 2024

bengl May 17, 2024

Qard May 19, 2024

codecov bot commented Jun 17, 2024 •

edited

Loading

rochdev Jun 18, 2024

bengl Jun 18, 2024

rochdev Jun 18, 2024

rochdev Jun 19, 2024

szegedi left a comment

szegedi Jun 19, 2024

szegedi Jun 20, 2024


		const desc = Reflect.getOwnPropertyDescriptor(process.versions, 'node')

		desc.value = process.env.FAKE_VERSION

	const forced = ['1', 'true', 'True'].includes(process.env.DD_INJECT_FORCE)
	const forced = isTrue(process.env.DD_INJECT_FORCE)

Add runtime guardrail for SSI #4319

Add runtime guardrail for SSI #4319

Conversation

bengl commented May 16, 2024

github-actions bot commented May 16, 2024 • edited Loading

Overall package size

Dependency sizes

pr-commenter bot commented May 16, 2024 • edited Loading

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szegedi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 16, 2024 •

edited

Loading

pr-commenter bot commented May 16, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading