Skip to content

[IAST] fix tainting values stored in the db#6389

Merged
robertpi merged 20 commits into
masterfrom
robert/asm/test-stored-xxs
Dec 13, 2024
Merged

[IAST] fix tainting values stored in the db#6389
robertpi merged 20 commits into
masterfrom
robert/asm/test-stored-xxs

Conversation

@robertpi

@robertpi robertpi commented Dec 3, 2024

Copy link
Copy Markdown
Member

Summary of changes

Add tests and corrections to support for various database platforms.

Reason for change

Some bugs were reported relating to database tainting not working correctly for some dbs.

Test coverage

Adds new integration tests dependent on docker db instances.

@robertpi
robertpi force-pushed the robert/asm/test-stored-xxs branch from e381678 to 17c314e Compare December 3, 2024 14:29
@github-actions

github-actions Bot commented Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

1 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,
[...]
-      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.IastController.StoredSqli (Samples.Security.AspNetCore5),
+      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController.StoredSqli (Samples.Security.AspNetCore5.DatabaseIntegration),

1 occurrences of :

-      http.url: http://localhost:00000/Iast/StoredSqli?useMicrosoftDataDb=...,
+      http.url: http://localhost:00000/Iast/StoredSqli?database=...,

1 occurrences of :

-        "path": "Samples.Security.AspNetCore5.Controllers.IastController",
+        "path": "Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController",

3 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,

1 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,
[...]
-      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.IastController.StoredXss (Samples.Security.AspNetCore5),
+      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController.StoredXss (Samples.Security.AspNetCore5.DatabaseIntegration),

1 occurrences of :

-      http.url: http://localhost:00000/Iast/StoredXss?param=%3Cb%3ERawValue%3C/b%3E&useMicrosoftDataDb=...,
+      http.url: http://localhost:00000/Iast/StoredXss?param=%3Cb%3ERawValue%3C/b%3E&database=...,

1 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,
[...]
-      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.IastController.StoredXssEscaped (Samples.Security.AspNetCore5),
+      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController.StoredXssEscaped (Samples.Security.AspNetCore5.DatabaseIntegration),

1 occurrences of :

-      http.url: http://localhost:00000/Iast/StoredXssEscaped?useMicrosoftDataDb=...,
+      http.url: http://localhost:00000/Iast/StoredXssEscaped?database=...,

@andrewlock

andrewlock commented Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (68ms)  : 65, 71
     .   : milestone, 68,
    master - mean (68ms)  : 64, 73
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (979ms)  : 957, 1000
     .   : milestone, 979,
    master - mean (974ms)  : 948, 999
     .   : milestone, 974,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (107ms)  : 104, 110
     .   : milestone, 107,
    master - mean (106ms)  : 104, 108
     .   : milestone, 106,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (679ms)  : 666, 693
     .   : milestone, 679,
    master - mean (674ms)  : 660, 689
     .   : milestone, 674,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (91ms)  : 89, 93
     .   : milestone, 91,
    master - mean (90ms)  : 88, 92
     .   : milestone, 90,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (635ms)  : 620, 650
     .   : milestone, 635,
    master - mean (629ms)  : 614, 644
     .   : milestone, 629,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (194ms)  : 190, 199
     .   : milestone, 194,
    master - mean (194ms)  : 190, 198
     .   : milestone, 194,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (1,103ms)  : 1070, 1137
     .   : milestone, 1103,
    master - mean (1,098ms)  : 1074, 1123
     .   : milestone, 1098,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (279ms)  : 273, 285
     .   : milestone, 279,
    master - mean (277ms)  : 273, 281
     .   : milestone, 277,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (878ms)  : 844, 913
     .   : milestone, 878,
    master - mean (869ms)  : 838, 901
     .   : milestone, 869,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (267ms)  : 263, 272
     .   : milestone, 267,
    master - mean (266ms)  : 262, 271
     .   : milestone, 266,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (853ms)  : 819, 887
     .   : milestone, 853,
    master - mean (850ms)  : 811, 890
     .   : milestone, 850,
Loading

@andrewlock

andrewlock commented Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for appsec 🐌

Benchmarks for #6389 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.172
  • 1 benchmarks are slower, with geometric mean 1.130
  • 1 benchmarks have fewer allocations
  • 2 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6389

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net6.0 1.130 188,463.30 212,878.99

Faster 🎉 in #6389

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 1.172 229.02 195.35

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 187μs 117ns 437ns 2.62 0 0 188.52 KB
master AllCycleSimpleBody netcoreapp3.1 281μs 159ns 597ns 2.66 0 0 195.8 KB
master AllCycleSimpleBody net472 250μs 153ns 574ns 35.8 2 0 225.32 KB
master AllCycleMoreComplexBody net6.0 189μs 80.4ns 290ns 2.74 0 0 192.02 KB
master AllCycleMoreComplexBody netcoreapp3.1 298μs 639ns 2.48μs 2.63 0 0 199.22 KB
master AllCycleMoreComplexBody net472 254μs 425ns 1.64μs 36.4 2.02 0 228.83 KB
master ObjectExtractorSimpleBody net6.0 138ns 0.184ns 0.69ns 0.00394 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 229ns 0.339ns 1.31ns 0.00366 0 0 272 B
master ObjectExtractorSimpleBody net472 163ns 0.264ns 1.02ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 2.89μs 1.4ns 5.22ns 0.0522 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.73μs 3.7ns 13.8ns 0.0508 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.6μs 2.19ns 8.18ns 0.603 0.00543 0 3.8 KB
#6389 AllCycleSimpleBody net6.0 202μs 106ns 395ns 2.65 0 0 188.38 KB
#6389 AllCycleSimpleBody netcoreapp3.1 305μs 116ns 417ns 2.58 0 0 195.66 KB
#6389 AllCycleSimpleBody net472 267μs 320ns 1.15μs 35.7 2 0 225.16 KB
#6389 AllCycleMoreComplexBody net6.0 213μs 88.5ns 343ns 2.64 0 0 191.88 KB
#6389 AllCycleMoreComplexBody netcoreapp3.1 308μs 225ns 870ns 2.76 0 0 199.07 KB
#6389 AllCycleMoreComplexBody net472 273μs 452ns 1.63μs 36.3 2.05 0 228.67 KB
#6389 ObjectExtractorSimpleBody net6.0 137ns 0.109ns 0.392ns 0.00391 0 0 280 B
#6389 ObjectExtractorSimpleBody netcoreapp3.1 195ns 0.0694ns 0.269ns 0.00374 0 0 272 B
#6389 ObjectExtractorSimpleBody net472 163ns 0.113ns 0.44ns 0.0446 0 0 281 B
#6389 ObjectExtractorMoreComplexBody net6.0 2.91μs 2.71ns 10.5ns 0.0538 0 0 3.78 KB
#6389 ObjectExtractorMoreComplexBody netcoreapp3.1 3.97μs 2.34ns 9.05ns 0.0517 0 0 3.69 KB
#6389 ObjectExtractorMoreComplexBody net472 3.59μs 3.04ns 11.4ns 0.602 0.00538 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 37.2μs 26.4ns 98.6ns 0.445 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 54.7μs 56.4ns 219ns 0.432 0 0 32.4 KB
master EncodeArgs net472 67.7μs 43.1ns 161ns 5.16 0.067 0 32.5 KB
master EncodeLegacyArgs net6.0 74.2μs 107ns 416ns 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 104μs 163ns 630ns 0 0 0 2.14 KB
master EncodeLegacyArgs net472 154μs 128ns 497ns 0.306 0 0 2.15 KB
#6389 EncodeArgs net6.0 36.9μs 23.5ns 81.3ns 0.443 0 0 32.4 KB
#6389 EncodeArgs netcoreapp3.1 54.4μs 54.1ns 203ns 0.433 0 0 32.4 KB
#6389 EncodeArgs net472 67.9μs 125ns 484ns 5.15 0.0673 0 32.5 KB
#6389 EncodeLegacyArgs net6.0 73.1μs 37.4ns 140ns 0.0364 0 0 2.14 KB
#6389 EncodeLegacyArgs netcoreapp3.1 106μs 171ns 638ns 0 0 0 2.14 KB
#6389 EncodeLegacyArgs net472 154μs 96.7ns 362ns 0.307 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 181μs 84ns 291ns 0 0 0 2.44 KB
master RunWafRealisticBenchmark netcoreapp3.1 192μs 249ns 964ns 0 0 0 2.39 KB
master RunWafRealisticBenchmark net472 209μs 180ns 696ns 0.309 0 0 2.46 KB
master RunWafRealisticBenchmarkWithAttack net6.0 122μs 119ns 461ns 0 0 0 1.47 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 129μs 162ns 583ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack net472 139μs 56.6ns 219ns 0.209 0 0 1.49 KB
#6389 RunWafRealisticBenchmark net6.0 190μs 162ns 626ns 0 0 0 2.44 KB
#6389 RunWafRealisticBenchmark netcoreapp3.1 194μs 192ns 718ns 0 0 0 2.39 KB
#6389 RunWafRealisticBenchmark net472 207μs 126ns 471ns 0.31 0 0 2.46 KB
#6389 RunWafRealisticBenchmarkWithAttack net6.0 123μs 108ns 419ns 0 0 0 1.47 KB
#6389 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 116ns 451ns 0 0 0 1.46 KB
#6389 RunWafRealisticBenchmarkWithAttack net472 141μs 96.5ns 361ns 0.211 0 0 1.49 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6389

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 257.94 KB 264.13 KB 6.18 KB 2.40%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 253.05 KB 255.14 KB 2.09 KB 0.83%

Fewer allocations 🎉 in #6389

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 61.66 KB 60.06 KB -1.6 KB -2.59%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 52.3μs 249ns 932ns 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 53.4μs 176ns 660ns 0 0 0 42.64 KB
master StringConcatBenchmark net472 36.4μs 80.4ns 290ns 0 0 0 61.66 KB
master StringConcatAspectBenchmark net6.0 310μs 1.09μs 3.92μs 0 0 0 257.94 KB
master StringConcatAspectBenchmark netcoreapp3.1 333μs 1.74μs 9.37μs 0 0 0 253.05 KB
master StringConcatAspectBenchmark net472 284μs 6.2μs 59.5μs 0 0 0 278.53 KB
#6389 StringConcatBenchmark net6.0 51.5μs 244ns 944ns 0 0 0 43.44 KB
#6389 StringConcatBenchmark netcoreapp3.1 58μs 794ns 7.86μs 0 0 0 42.64 KB
#6389 StringConcatBenchmark net472 37.4μs 87.5ns 328ns 0 0 0 60.06 KB
#6389 StringConcatAspectBenchmark net6.0 291μs 6.09μs 60.3μs 0 0 0 264.13 KB
#6389 StringConcatAspectBenchmark netcoreapp3.1 345μs 1.77μs 10.8μs 0 0 0 255.14 KB
#6389 StringConcatAspectBenchmark net472 272μs 4.5μs 42.9μs 0 0 0 278.53 KB

@datadog-ddstaging

datadog-ddstaging Bot commented Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: robert/asm/test-stored-xxs
Commit report: e9072d2
Test service: dd-trace-dotnet

✅ 0 Failed, 466351 Passed, 3694 Skipped, 33h 5m 16.77s Total Time

@andrewlock

andrewlock commented Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for tracer 🐌

Benchmarks for #6389 compared to master:

  • 1 benchmarks are slower, with geometric mean 1.121
  • All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 8μs 44.8ns 297ns 0.0159 0.00794 0 5.61 KB
master StartStopWithChild netcoreapp3.1 9.82μs 47.8ns 203ns 0.0242 0.00969 0 5.8 KB
master StartStopWithChild net472 16.2μs 49.4ns 191ns 1.04 0.3 0.0974 6.21 KB
#6389 StartStopWithChild net6.0 8.26μs 46.7ns 313ns 0.0163 0.00815 0 5.61 KB
#6389 StartStopWithChild netcoreapp3.1 10.3μs 54.7ns 310ns 0.0199 0.00497 0 5.81 KB
#6389 StartStopWithChild net472 16.4μs 43.1ns 167ns 1.06 0.341 0.106 6.2 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 480μs 735ns 2.85μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 647μs 309ns 1.2μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 853μs 419ns 1.62μs 0.425 0 0 3.3 KB
#6389 WriteAndFlushEnrichedTraces net6.0 489μs 347ns 1.34μs 0 0 0 2.7 KB
#6389 WriteAndFlushEnrichedTraces netcoreapp3.1 701μs 629ns 2.44μs 0 0 0 2.7 KB
#6389 WriteAndFlushEnrichedTraces net472 877μs 359ns 1.29μs 0.434 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 150μs 900ns 8.91μs 0.136 0 0 14.47 KB
master SendRequest netcoreapp3.1 169μs 1.02μs 9.81μs 0.181 0 0 17.27 KB
master SendRequest net472 0.00166ns 0.000575ns 0.00215ns 0 0 0 0 b
#6389 SendRequest net6.0 152μs 887ns 7.73μs 0.142 0 0 14.47 KB
#6389 SendRequest netcoreapp3.1 166μs 945ns 7.14μs 0.225 0 0 17.27 KB
#6389 SendRequest net472 0.00082ns 0.000466ns 0.00168ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 570μs 3.09μs 18μs 0.579 0 0 41.64 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 677μs 3.69μs 21.2μs 0.347 0 0 41.78 KB
master WriteAndFlushEnrichedTraces net472 833μs 3.19μs 11.9μs 8.33 2.5 0.417 53.26 KB
#6389 WriteAndFlushEnrichedTraces net6.0 556μs 2.16μs 8.06μs 0.534 0 0 41.57 KB
#6389 WriteAndFlushEnrichedTraces netcoreapp3.1 658μs 2.24μs 8.38μs 0.321 0 0 41.78 KB
#6389 WriteAndFlushEnrichedTraces net472 849μs 3.14μs 11.3μs 8.08 2.55 0.425 53.27 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.3μs 0.717ns 2.78ns 0.0142 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.76μs 1.82ns 7.04ns 0.0139 0 0 1.02 KB
master ExecuteNonQuery net472 2.01μs 1.64ns 6.34ns 0.156 0.00101 0 987 B
#6389 ExecuteNonQuery net6.0 1.35μs 1.52ns 5.87ns 0.0143 0 0 1.02 KB
#6389 ExecuteNonQuery netcoreapp3.1 1.79μs 3.58ns 13.4ns 0.0142 0 0 1.02 KB
#6389 ExecuteNonQuery net472 2.14μs 2.28ns 8.54ns 0.157 0.00107 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.27μs 0.541ns 2.1ns 0.0133 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.59μs 0.682ns 2.64ns 0.0128 0 0 976 B
master CallElasticsearch net472 2.59μs 1.04ns 3.88ns 0.157 0 0 995 B
master CallElasticsearchAsync net6.0 1.39μs 0.653ns 2.53ns 0.0132 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.74μs 0.608ns 2.27ns 0.0139 0 0 1.02 KB
master CallElasticsearchAsync net472 2.58μs 0.841ns 3.15ns 0.167 0 0 1.05 KB
#6389 CallElasticsearch net6.0 1.23μs 1.25ns 4.86ns 0.0135 0 0 976 B
#6389 CallElasticsearch netcoreapp3.1 1.54μs 0.428ns 1.6ns 0.0132 0 0 976 B
#6389 CallElasticsearch net472 2.64μs 1.23ns 4.76ns 0.158 0 0 995 B
#6389 CallElasticsearchAsync net6.0 1.29μs 0.624ns 2.33ns 0.0136 0 0 952 B
#6389 CallElasticsearchAsync netcoreapp3.1 1.65μs 0.453ns 1.69ns 0.014 0 0 1.02 KB
#6389 CallElasticsearchAsync net472 2.63μs 1.65ns 6.17ns 0.166 0 0 1.05 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net6.0 1.31μs 3.72ns 14.4ns 0.0129 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.72μs 0.578ns 2.16ns 0.0129 0 0 952 B
master ExecuteAsync net472 1.79μs 0.544ns 1.96ns 0.145 0 0 915 B
#6389 ExecuteAsync net6.0 1.42μs 0.413ns 1.6ns 0.0135 0 0 952 B
#6389 ExecuteAsync netcoreapp3.1 1.68μs 0.876ns 3.28ns 0.0125 0 0 952 B
#6389 ExecuteAsync net472 1.8μs 0.411ns 1.59ns 0.145 0 0 915 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.37μs 1.03ns 3.99ns 0.0326 0 0 2.31 KB
master SendAsync netcoreapp3.1 5.27μs 2.36ns 9.13ns 0.0396 0 0 2.85 KB
master SendAsync net472 7.34μs 1.57ns 5.87ns 0.495 0 0 3.12 KB
#6389 SendAsync net6.0 4.49μs 1.7ns 6.57ns 0.0315 0 0 2.31 KB
#6389 SendAsync netcoreapp3.1 5.38μs 3.19ns 12.4ns 0.0376 0 0 2.85 KB
#6389 SendAsync net472 7.4μs 2.61ns 10.1ns 0.492 0 0 3.12 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.48μs 2.98ns 11.6ns 0.0234 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.2μs 1.92ns 7.43ns 0.022 0 0 1.64 KB
master EnrichedLog net472 2.71μs 1.8ns 6.48ns 0.249 0 0 1.57 KB
#6389 EnrichedLog net6.0 1.46μs 1.16ns 4.5ns 0.0232 0 0 1.64 KB
#6389 EnrichedLog netcoreapp3.1 2.14μs 0.755ns 2.93ns 0.0217 0 0 1.64 KB
#6389 EnrichedLog net472 2.68μs 1.27ns 4.74ns 0.25 0 0 1.57 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 119μs 269ns 1.04μs 0.0594 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 124μs 258ns 1μs 0 0 0 4.28 KB
master EnrichedLog net472 151μs 250ns 970ns 0.669 0.223 0 4.46 KB
#6389 EnrichedLog net6.0 119μs 211ns 818ns 0 0 0 4.28 KB
#6389 EnrichedLog netcoreapp3.1 123μs 206ns 797ns 0.0623 0 0 4.28 KB
#6389 EnrichedLog net472 154μs 136ns 527ns 0.69 0.23 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.13μs 1.35ns 5.22ns 0.0312 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.18μs 2.66ns 10.3ns 0.0291 0 0 2.2 KB
master EnrichedLog net472 4.9μs 0.728ns 2.72ns 0.319 0 0 2.02 KB
#6389 EnrichedLog net6.0 2.99μs 0.888ns 3.32ns 0.0314 0 0 2.2 KB
#6389 EnrichedLog netcoreapp3.1 4.23μs 1.75ns 6.77ns 0.0296 0 0 2.2 KB
#6389 EnrichedLog net472 4.89μs 1.19ns 4.63ns 0.32 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.32μs 1.55ns 5.98ns 0.0158 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.83μs 1.73ns 6.7ns 0.0153 0 0 1.14 KB
master SendReceive net472 2.11μs 1.3ns 5.03ns 0.184 0 0 1.16 KB
#6389 SendReceive net6.0 1.42μs 1.07ns 4.16ns 0.016 0 0 1.14 KB
#6389 SendReceive netcoreapp3.1 1.74μs 1.3ns 5.03ns 0.0155 0 0 1.14 KB
#6389 SendReceive net472 2.07μs 1.32ns 5.11ns 0.183 0 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.74μs 2.57ns 9.95ns 0.022 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.81μs 1.28ns 4.78ns 0.021 0 0 1.65 KB
master EnrichedLog net472 4.44μs 0.633ns 2.19ns 0.324 0 0 2.04 KB
#6389 EnrichedLog net6.0 2.65μs 0.561ns 2.02ns 0.0228 0 0 1.6 KB
#6389 EnrichedLog netcoreapp3.1 3.91μs 1.31ns 4.91ns 0.0215 0 0 1.65 KB
#6389 EnrichedLog net472 4.33μs 3.73ns 14.4ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 398ns 0.261ns 0.977ns 0.00814 0 0 576 B
master StartFinishSpan netcoreapp3.1 589ns 0.702ns 2.72ns 0.00767 0 0 576 B
master StartFinishSpan net472 649ns 0.436ns 1.69ns 0.0918 0 0 578 B
master StartFinishScope net6.0 560ns 0.241ns 0.901ns 0.00982 0 0 696 B
master StartFinishScope netcoreapp3.1 723ns 0.791ns 3.06ns 0.0094 0 0 696 B
master StartFinishScope net472 875ns 0.528ns 2.04ns 0.104 0 0 658 B
#6389 StartFinishSpan net6.0 393ns 0.249ns 0.965ns 0.00797 0 0 576 B
#6389 StartFinishSpan netcoreapp3.1 580ns 0.567ns 2.2ns 0.00775 0 0 576 B
#6389 StartFinishSpan net472 648ns 0.349ns 1.21ns 0.0916 0 0 578 B
#6389 StartFinishScope net6.0 568ns 0.551ns 2.13ns 0.00965 0 0 696 B
#6389 StartFinishScope netcoreapp3.1 749ns 0.545ns 2.11ns 0.00931 0 0 696 B
#6389 StartFinishScope net472 870ns 0.475ns 1.84ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6389

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 1.121 609.08 682.66

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 609ns 0.524ns 2.03ns 0.00977 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 927ns 0.491ns 1.9ns 0.00934 0 0 696 B
master RunOnMethodBegin net472 1.12μs 1.19ns 4.47ns 0.104 0 0 658 B
#6389 RunOnMethodBegin net6.0 683ns 0.43ns 1.67ns 0.00973 0 0 696 B
#6389 RunOnMethodBegin netcoreapp3.1 968ns 0.376ns 1.36ns 0.00925 0 0 696 B
#6389 RunOnMethodBegin net472 1.1μs 0.504ns 1.95ns 0.104 0 0 658 B

@robertpi
robertpi force-pushed the robert/asm/test-stored-xxs branch from a5ed297 to fc2be2d Compare December 3, 2024 16:25
@andrewlock

andrewlock commented Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Throughput/Crank Report ⚡

Throughput results for AspNetCoreSimpleController comparing the following branches/commits:

Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.

Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!

gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6389) (11.192M)   : 0, 11192159
    master (11.195M)   : 0, 11194535
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6389) (7.278M)   : 0, 7278323
    master (7.216M)   : 0, 7216361
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.611M)   : 0, 7610504

    section Manual
    master (11.163M)   : 0, 11162548

    section Manual + Automatic
    This PR (6389) (6.775M)   : 0, 6774762
    master (6.753M)   : 0, 6752572

    section DD_TRACE_ENABLED=0
    master (10.216M)   : 0, 10215642
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6389) (9.270M)   : 0, 9270052
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6389) (6.433M)   : 0, 6432814

    section Manual + Automatic
    This PR (6389) (5.802M)   : 0, 5801580
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6389) (9.513M)   : 0, 9512762
    master (9.988M)   : 0, 9987589
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6389) (6.176M)   : 0, 6175629
    master (6.338M)   : 0, 6338249
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (7.137M)   : 0, 7136842

    section Manual
    master (9.965M)   : 0, 9965094

    section Manual + Automatic
    This PR (6389) (5.699M)   : 0, 5699227
    master (5.791M)   : 0, 5790893

    section DD_TRACE_ENABLED=0
    master (9.274M)   : 0, 9274318
Loading

@robertpi
robertpi marked this pull request as ready for review December 10, 2024 10:12
@robertpi
robertpi requested review from a team as code owners December 10, 2024 10:12
NachoEchevarria
NachoEchevarria reviewed Dec 10, 2024
View reviewed changes
Comment thread .../test/test-applications/security/Samples.Security.AspNetCore5/Properties/launchSettings.json Outdated
"DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED": "true",
"DD_APPSEC_BLOCKING_ENABLED": "false",
"DD_DOTNET_TRACER_HOME": "$(SolutionDir)shared\\bin\\monitoring-home",
"DD_DOTNET_TRACER_HOME": "c:\\code\\dd-trace-dotnet\\shared\\bin\\monitoring-home",

@NachoEchevarria NachoEchevarria Dec 10, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, changes in this file were not intended to be included in the PR

@robertpi robertpi Dec 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! Fixed in d266b58

NachoEchevarria
NachoEchevarria reviewed Dec 10, 2024
View reviewed changes
Comment thread ...-applications/security/aspnet/Samples.Security.AspNetMvc5/Samples.Security.AspNetMvc5.csproj Outdated
<ErrorReport>prompt</ErrorReport>
</PropertyGroup>
<ItemGroup>
<Reference Include="BouncyCastle.Crypto, Version=1.9.0.0, Culture=neutral, PublicKeyToken=0e99375e54769942, processorArchitecture=MSIL">

@NachoEchevarria NachoEchevarria Dec 10, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The c# code from this mvc web does not seemed to have changed. Do we need these new references?

@robertpi robertpi Dec 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to have been added automatically at some point, thanks! Fixed in d266b58

NachoEchevarria
NachoEchevarria reviewed Dec 10, 2024
View reviewed changes
Comment thread ...ions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Controllers/HomeController.cs
@@ -0,0 +1,33 @@
using Microsoft.AspNetCore.Mvc;

@NachoEchevarria NachoEchevarria Dec 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully get why we need a new sample page instead of reusing/expanding the old one. I am pretty sure that there is a strong reason, though.

@dromanol dromanol Dec 10, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't get why either. It feels like this could be added to the Samples.Security.AspNetCore5 one

@robertpi robertpi Dec 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be a whole new sample, because it has dependencies databases run in docker. The way the integration tests currently work is samples are either run with docker dependencies or without them - you can build in both tests runs.

So if we want to have these endpoints to Samples.Security.AspNetCore5 it would mean moving all the tests in that run with docker dependency. We could so that, but I don't think it's what we want as the with docker run is more flaky.

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could so that, but I don't think it's what we want as the with docker run is more flaky.

Personally, I would actually have gone the other way - I'm very in favour of fewer samples. If anything, the docker run is less flakey actually 😄

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, somewhat separate, but it would be preferable to link the files into the sample (if we really need one) rather than copying. Every extra file slows down CI a small amount 😄 + it's more duplication to deal with etc

NachoEchevarria
NachoEchevarria reviewed Dec 10, 2024
View reviewed changes
Comment thread ...tions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Data/IastControllerHelper.cs
return res;
}
}
}

@NachoEchevarria NachoEchevarria Dec 10, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should binary app.db file be included?

@robertpi robertpi Dec 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's needed for sqlite tests.

dromanol
dromanol approved these changes Dec 10, 2024
View reviewed changes

@dromanol dromanol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this.

@robertpi
robertpi force-pushed the robert/asm/test-stored-xxs branch from 094a66c to 194b611 Compare December 10, 2024 15:16
NachoEchevarria
NachoEchevarria approved these changes Dec 10, 2024
View reviewed changes

@NachoEchevarria NachoEchevarria left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!!

andrewlock
andrewlock approved these changes Dec 11, 2024
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM from the integration side. Just a few nits and suggestions. Personally I would favour merging the samples tbh (but could be done in a separate PR instead if you agree and that's easier)

Comment thread tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/MySql/MySqlDefinitions.cs
MaximumVersion = "9.*.*",
IntegrationName = nameof(IntegrationId.MySql),
DataReaderType = "MySql.Data.MySqlClient.MySqlDataReader",
DataReaderTaskType = "System.Threading.Tasks.Task`1[MySql.Data.MySqlClient.MySqlDataReader]",

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned elsewhere we should mark these integration as being IAST only - may require an update to the source generator though

Comment thread tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/ReaderCloseNpgsqlIntegration.cs

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we should move this into the Npgsql sub-folder

@robertpi robertpi Dec 12, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in c59e762

Comment thread tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/ReaderCloseNpgsqlIntegration.cs Outdated
ParameterTypeNames = new[] { ClrNames.Bool, ClrNames.Bool, ClrNames.Bool, },
MinimumVersion = "4.0.0",
MaximumVersion = "8.*.*",
IntegrationName = nameof(IntegrationId.Npgsql))]

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can definitely set the InstrumentationCategory for this one!

Suggested change
IntegrationName = nameof(IntegrationId.Npgsql))]
IntegrationName = nameof(IntegrationId.Npgsql),
InstrumentationCategory = InstrumentationCategory.Iast)]

@robertpi robertpi Dec 12, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in c59e762

Comment thread tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/ReaderCloseNpgsqlIntegration.cs Outdated
Comment on lines +37 to +47
/// <summary>
/// OnMethodEnd callback
/// </summary>
/// <typeparam name="TTarget">Type of the target</typeparam>
/// <typeparam name="TReturn">Type of the return</typeparam>
/// <param name="instance">Instance value, aka `this` of the instrumented method.</param>
/// <param name="returnValue">Instance of the return value</param>
/// <param name="exception">Exception instance in case the original code threw an exception.</param>
/// <param name="state">Calltarget state value</param>
/// <returns>A response value, in an async scenario will be T of Task of T</returns>
// internal static CallTargetReturn OnMethodEnd<TTarget>(TTarget instance, Exception? exception, in CallTargetState state)

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we tend to remove these comments as being just noise these days

Suggested change
/// <summary>
/// OnMethodEnd callback
/// </summary>
/// <typeparam name="TTarget">Type of the target</typeparam>
/// <typeparam name="TReturn">Type of the return</typeparam>
/// <param name="instance">Instance value, aka `this` of the instrumented method.</param>
/// <param name="returnValue">Instance of the return value</param>
/// <param name="exception">Exception instance in case the original code threw an exception.</param>
/// <param name="state">Calltarget state value</param>
/// <returns>A response value, in an async scenario will be T of Task of T</returns>
// internal static CallTargetReturn OnMethodEnd<TTarget>(TTarget instance, Exception? exception, in CallTargetState state)

@robertpi robertpi Dec 12, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in c59e762

Comment thread tracer/src/Datadog.Trace/Iast/IastModule.cs Outdated
Comment on lines +907 to +910
// note: the return value this method indicates if the value could have been tainted
// if a iast context is available. It will return true if iast is enabled and the row
// count conditions are met. The result is used in the unit tests, don't use it
// anywhere else

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we add this comment to the doc comments? 🙂

Suggested change
// note: the return value this method indicates if the value could have been tainted
// if a iast context is available. It will return true if iast is enabled and the row
// count conditions are met. The result is used in the unit tests, don't use it
// anywhere else
/// <summary>
/// Add a DB value to be tainted
/// </summary>
/// <returns>The return value this method indicates if the value could have been tainted
/// if a iast context is available. It will return true if iast is enabled and the row
/// count conditions are met. The result is used in the unit tests, don't use it
/// anywhere else
/// </returns>

@robertpi robertpi Dec 12, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in c59e762

Comment thread tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp
Comment thread ...ions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Controllers/HomeController.cs
@@ -0,0 +1,33 @@
using Microsoft.AspNetCore.Mvc;

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could so that, but I don't think it's what we want as the with docker run is more flaky.

Personally, I would actually have gone the other way - I'm very in favour of fewer samples. If anything, the docker run is less flakey actually 😄

Comment thread ...ions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Controllers/HomeController.cs
@@ -0,0 +1,33 @@
using Microsoft.AspNetCore.Mvc;

@andrewlock andrewlock Dec 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, somewhat separate, but it would be preferable to link the files into the sample (if we really need one) rather than copying. Every extra file slows down CI a small amount 😄 + it's more duplication to deal with etc

@robertpi

robertpi commented Dec 11, 2024

Copy link
Copy Markdown
Member Author

Mostly LGTM from the integration side. Just a few nits and suggestions. Personally I would favour merging the samples tbh (but could be done in a separate PR instead if you agree and that's easier)

Okay, I agree, we can do a follow up PR to remerge the sample and move it completely over the docker dependency.

We could so that, but I don't think it's what we want as the with docker run is more flaky.

Personally, I would actually have gone the other way - I'm very in favour of fewer samples. If anything, the docker run is less flakey actually 😄

Doesn't start the depended database images failing to start create more flake? If the docker depend tests really are more stable, why not run everything there.

@robertpi
robertpi force-pushed the robert/asm/test-stored-xxs branch from c59e762 to e9072d2 Compare December 13, 2024 12:48
@robertpi
robertpi merged commit 0e50622 into master Dec 13, 2024
@robertpi
robertpi deleted the robert/asm/test-stored-xxs branch December 13, 2024 14:52
@github-actions github-actions Bot added this to the vNext-v3 milestone Dec 13, 2024
veerbia pushed a commit that referenced this pull request Dec 16, 2024
@robertpi @veerbia
[IAST] fix tainting values stored in the db (#6389)
7a250af 
## Summary of changes

Add tests and corrections to support for various database platforms.

## Reason for change

Some bugs were reported relating to database tainting not working
correctly for some dbs.

## Test coverage

Adds new integration tests dependent on docker db instances.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dromanol dromanol dromanol approved these changes
@andrewlock andrewlock andrewlock approved these changes
@NachoEchevarria NachoEchevarria NachoEchevarria approved these changes

Assignees

No one assigned

Labels

area:asm area:asm-iast type:bug

Projects

None yet

Milestone

3.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@robertpi @andrewlock @NachoEchevarria @dromanol