Support changes for nginx-datadog/waf-rem-cfg #143

cataphract · 2024-08-06T17:43:42Z

Description

See individual commits

Motivation

The implemetation originally assumed that service_target was included in all configuration files. However,, this assumption is incorrect; for instance, Appsec configuration files does not include this field. This PR removes that unnecessary restriction.

pr-commenter · 2024-08-06T17:45:44Z

Benchmarks

Benchmark execution time: 2024-08-20 09:33:15

Comparing candidate commit 383400b in PR branch glopes/waf-rem-cfg with baseline commit dc5e762 in branch main.

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

dgoffredo · 2024-08-07T00:09:29Z

Re: this commit...

It looks like Damien added src/datadog as an include directory to support his refactoring of remote config here. The structure of includes was no longer flat, so he chose to keep the #include "..." discipline flat by adding src/datadog to the compiler's list of include directories to search.

You then saw this causes problems with the POSIX header glob.h and so you removed src/datadog from the list of include directories (good idea) and started using relative includes instead, like #include "../glob.h".

Here's an alternative workaround that I like better: Change the #include style throughout all of dd-trace-cpp to use angle-bracketed "qualified" includes. For example, in src/datadog/remote_config/listener.h you have #include "../optional.h". Consider changing it instead to #include <datadog/optional.h>. In existing headers, everywhere there is #include "foo.h", it would instead be #include <datadog/foo.h> or possibly #include <datadog/remote_config/foo.h>. The relative locations of the headers and sources that include each other would no longer be relevant -- it would always be relative to the exposed include directory src/ and made unambiguous by the constant prefix datadog/.

This change would affect every file in the repository, but now that the structure of includes is not flat, I think it's The Right Way™.

The distinction really is just a matter of style. The advantage that comes to mind for what I propose is that someone interested in using the library programmatically can copy the include style without having to grok the examples. That and it makes clearer that the file included is relative to some build-configured search directory, rather than being that or relative to the current file (and not knowing which is the case).

src/datadog/remote_config/remote_config.cpp

See #143 (comment)

cataphract · 2024-08-19T19:29:52Z

This change would affect every file in the repository, but now that the structure of includes is not flat, I think it's The Right Way™.

The distinction really is just a matter of style. The advantage that comes to mind for what I propose is that someone interested in using the library programmatically can copy the include style without having to grok the examples. That and it makes clearer that the file included is relative to some build-configured search directory, rather than being that or relative to the current file (and not knowing which is the case).

@dgoffredo I'm not sold on this being a good idea, style aside because angled includes privilege include directories over local files (and hence you will have problems building another version of the library if you have another version installed globally), but anyway I've added a commit for it.

dmehala · 2024-08-20T09:07:57Z

@cataphract don't mind fixing the include issue. This issue is being addressed in #144

EDIT: @cataphract I merged #144, the include issue is fixed. I have DataDog/nginx-datadog#106 pending to support changes introduced in dd-trace-cpp.

May I suggest you to rebase your branch?

codecov-commenter · 2024-08-21T14:26:32Z

Codecov Report

Attention: Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.45%. Comparing base (d354e96) to head (1621fdb).
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
src/datadog/config_manager.cpp	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   94.46%   94.45%   -0.02%     
==========================================
  Files          72       72              
  Lines        3797     3805       +8     
==========================================
+ Hits         3587     3594       +7     
- Misses        210      211       +1

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

cataphract · 2024-08-21T14:29:17Z

I've rebased this and updated nginx-datadog to match. I didn't use your PR in nginx-datadog, though; removing nlhomann::json is a lot more work than you have there.

dmehala

Left a few comments. Thank you.

include/datadog/logger.h

src/datadog/remote_config/remote_config.cpp

dmehala · 2024-08-23T19:10:11Z

src/datadog/remote_config/remote_config.cpp

+      // TODO: should be moved to the listener
+      if (product == product::Flag::APM_TRACING) {
+        const auto config_json = nlohmann::json::parse(new_config.content);
+        new_config.version = config_json.at("revision");
+
+        const auto& targeted_service = config_json.at("service_target");
+        if (targeted_service.at("service").get<StringView>() !=
+                tracer_signature_.default_service ||
+            targeted_service.at("env").get<StringView>() !=
+                tracer_signature_.default_environment) {
+          new_config.state = Configuration::State::error;
+          new_config.error_message = "Wrong service targeted";
+          goto skip_listeners;
+        }
+      }


TODO.
[genuine question] Are you planning to move the code to the listener?

No, not really. It's not really trivial because the listener currently has no access to tracer_signature_.

[question] Do you mind if I push a commit to remove that technical debt?

sure go ahead

done in d11cd52

dmehala · 2024-08-23T19:22:54Z

src/datadog/tracer.cpp

    {"span_sampler", span_sampler_->config_json()},
-    {"injection_styles", join_propagation_styles(injection_styles_)},
-    {"extraction_styles", join_propagation_styles(extraction_styles_)},
+    {"injection_styles", to_string_vec(injection_styles_)},


Can you confirm the change?

Before After

"tracecontext,datadog" ["tracecontext", "datadog"]

If so, thank you—this is indeed an improvement. However, the best approach would be to leverage the JSON library.

The code should look something like:

void to_json(nlohmann::json& j, const PropagationStyle& style) { j = to_string_view(style); }

And the usage:

... {"injection_styles", injection_styles_}, ...

Yes, that's the change. This also fixes the test TestConfiguration.test_in_http in nginx. With the changes made in this PR it now looks normal:

{ "collector": { "config": { "event_scheduler": { "type": "datadog::nginx::NgxEventScheduler" }, "flush_interval_milliseconds": 2000, "http_client": { "type": "datadog::tracing::Curl" }, "remote_configuration_url": "http://bogus:1234/v0.7/config", "request_timeout_milliseconds": 2000, "shutdown_timeout_milliseconds": 2000, "telemetry_url": "http://bogus:1234/telemetry/proxy/api/v2/apmtelemetry", "traces_url": "http://bogus:1234/v0.4/traces" }, "type": "datadog::tracing::DatadogAgent" }, "defaults": { "environment": "fooment", "service": "foosvc", "service_type": "web" }, "environment_variables": { "DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS": "1" }, "extraction_styles": [ "B3", "Datadog" ], "injection_styles": [ "B3", "Datadog" ], "report_traces": true, "runtime_id": "31998223-6dc3-4fd6-b4c2-cd1ded711143", "span_sampler": { "rules": [] }, "tags_header_size": 512, "trace_sampler": { "max_per_second": 200, "rules": [] }, "version": "[dd-trace-cpp version v0.2.2]" }

Indeed. Thanks, I appreciate.

dmehala

LGTM. Before merging, could you update the commit message to be more descriptive? Something like, "fix(remote_config): remove unnecessary restriction on configuration content" would be more appropriate.

id and version are not taken from the config body (which doesn't have to be json, and even less has to have specific fields). See https://github.com/DataDog/dd-trace-java/blob/dddda34e07973024fcf7457692ce1c3f892f2ef2/remote-config/remote-config-core/src/main/java/datadog/remoteconfig/state/ProductState.java#L209-L210

cataphract mentioned this pull request Aug 6, 2024

Remote config support for the WAF DataDog/nginx-datadog#71

Merged

cataphract force-pushed the glopes/waf-rem-cfg branch 2 times, most recently from fc25f95 to 5d69bf5 Compare August 19, 2024 18:24

cataphract marked this pull request as ready for review August 19, 2024 18:24

cataphract requested a review from a team as a code owner August 19, 2024 18:24

cataphract requested review from dmehala and removed request for a team August 19, 2024 18:24

dmehala reviewed Aug 19, 2024

View reviewed changes

src/datadog/remote_config/remote_config.cpp Outdated Show resolved Hide resolved

cataphract added a commit that referenced this pull request Aug 19, 2024

Use only lib header include style

5b50509

See #143 (comment)

cataphract added a commit that referenced this pull request Aug 19, 2024

Use only lib header include style

c1ebd40

See #143 (comment)

cataphract force-pushed the glopes/waf-rem-cfg branch from 5b50509 to c1ebd40 Compare August 19, 2024 19:22

cataphract added a commit that referenced this pull request Aug 19, 2024

Use only lib header include style

58c595a

See #143 (comment)

cataphract force-pushed the glopes/waf-rem-cfg branch from c1ebd40 to 58c595a Compare August 19, 2024 19:24

cataphract force-pushed the glopes/waf-rem-cfg branch 5 times, most recently from ca7299e to 930f9ea Compare August 21, 2024 14:21

cataphract force-pushed the glopes/waf-rem-cfg branch 2 times, most recently from c847275 to a42b961 Compare August 22, 2024 08:00

cataphract requested a review from dmehala August 23, 2024 13:08

dmehala reviewed Aug 23, 2024

View reviewed changes

dmehala approved these changes Aug 27, 2024

View reviewed changes

cataphract and others added 2 commits August 27, 2024 11:17

Fix json dump of configuration

7d94f4a

move logic to config_manager

1621fdb

cataphract force-pushed the glopes/waf-rem-cfg branch from d11cd52 to 1621fdb Compare August 27, 2024 10:18

cataphract merged commit 8e11e46 into main Aug 27, 2024
21 of 22 checks passed

cataphract deleted the glopes/waf-rem-cfg branch August 27, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support changes for nginx-datadog/waf-rem-cfg #143

Support changes for nginx-datadog/waf-rem-cfg #143

cataphract commented Aug 6, 2024 •

edited by dmehala

Loading

pr-commenter bot commented Aug 6, 2024 •

edited

Loading

dgoffredo commented Aug 7, 2024 •

edited

Loading

cataphract commented Aug 19, 2024

dmehala commented Aug 20, 2024 •

edited

Loading

codecov-commenter commented Aug 21, 2024 •

edited

Loading

cataphract commented Aug 21, 2024

dmehala left a comment

dmehala Aug 23, 2024

cataphract Aug 26, 2024

dmehala Aug 26, 2024

cataphract Aug 26, 2024

dmehala Aug 27, 2024

dmehala Aug 23, 2024

cataphract Aug 26, 2024

dmehala Aug 26, 2024

dmehala left a comment

Support changes for nginx-datadog/waf-rem-cfg #143

Support changes for nginx-datadog/waf-rem-cfg #143

Conversation

cataphract commented Aug 6, 2024 • edited by dmehala Loading

Description

Motivation

pr-commenter bot commented Aug 6, 2024 • edited Loading

Benchmarks

dgoffredo commented Aug 7, 2024 • edited Loading

cataphract commented Aug 19, 2024

dmehala commented Aug 20, 2024 • edited Loading

codecov-commenter commented Aug 21, 2024 • edited Loading

Codecov Report

cataphract commented Aug 21, 2024

dmehala left a comment

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

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

Choose a reason for hiding this comment

dmehala left a comment

Choose a reason for hiding this comment

cataphract commented Aug 6, 2024 •

edited by dmehala

Loading

pr-commenter bot commented Aug 6, 2024 •

edited

Loading

dgoffredo commented Aug 7, 2024 •

edited

Loading

dmehala commented Aug 20, 2024 •

edited

Loading

codecov-commenter commented Aug 21, 2024 •

edited

Loading