Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set appsec disabled when ddtrace is not enabled #245

Merged
merged 8 commits into from
Apr 27, 2023

Conversation

estringana
Copy link
Collaborator

Description

There are cases that customers disabled ddtrace but not appsec. On those cases we don't want appsec extension to be enabled.

Motivation

Additional Notes

Describe how to test your changes

Readiness checklist

  • Unit tests have been updated and pass
  • If known, an appropriate milestone has been selected
  • All new source files include the required notice

Release checklist

  • The CHANGELOG.md has been updated

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2023

Codecov Report

Merging #245 (4f1c1f0) into master (5dda370) will increase coverage by 7.58%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   63.54%   71.13%   +7.58%     
==========================================
  Files          94       25      -69     
  Lines        5635     3236    -2399     
  Branches     1802      721    -1081     
==========================================
- Hits         3581     2302    -1279     
+ Misses        974      560     -414     
+ Partials     1080      374     -706     
Flag Coverage Δ
extension 71.13% <83.33%> (-0.07%) ⬇️
helper ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/extension/ddappsec.c 69.91% <0.00%> (-0.76%) ⬇️
src/extension/ddtrace.c 63.39% <100.00%> (+0.73%) ⬆️
src/extension/tags.c 83.61% <100.00%> (-0.33%) ⬇️
src/extension/user_tracking.c 67.92% <100.00%> (-1.31%) ⬇️

... and 69 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@estringana estringana marked this pull request as ready for review April 17, 2023 12:48
@estringana estringana requested a review from Anilm3 April 17, 2023 12:48
Copy link
Collaborator

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, the solution provided would only work if DDTrace is not loaded. What if it's loaded but disabled through DD_TRACE_ENABLED = false?

@estringana estringana force-pushed the estringana/disable-appsec-if-no-tracer branch from 4494ff5 to cadf016 Compare April 18, 2023 15:45
src/extension/ddtrace.c Outdated Show resolved Hide resolved
src/extension/ddtrace.c Outdated Show resolved Hide resolved
src/extension/user_tracking.c Show resolved Hide resolved
@estringana estringana force-pushed the estringana/disable-appsec-if-no-tracer branch from 0ec843a to 3e14f05 Compare April 25, 2023 14:09
@estringana estringana force-pushed the estringana/disable-appsec-if-no-tracer branch from 3e14f05 to a216b1d Compare April 26, 2023 14:15
@estringana estringana force-pushed the estringana/disable-appsec-if-no-tracer branch from 1a1d852 to 4f1c1f0 Compare April 26, 2023 15:51
src/extension/ddtrace.h Outdated Show resolved Hide resolved
Co-authored-by: Anil Mahtani <929854+Anilm3@users.noreply.github.com>
@estringana estringana merged commit c8a7a7c into master Apr 27, 2023
28 checks passed
@estringana estringana deleted the estringana/disable-appsec-if-no-tracer branch April 27, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants