Skip to content

Conversation

@avara1986
Copy link
Member

@avara1986 avara1986 commented Mar 6, 2023

Remote Configuration should be able to store previous products target file content due to ASM needs to update all content/rules, the new once and the previous once.

further more, if a RC target file is disabled, RC should remove all configuration related to this target_file.

(Note: this bug was introduced with the new products in 1.10 so there is no need to backport).

e.g:

Actual behavior:

  • Payload1:
    • Product ASM_DD. New target file ASM_DD/file_12. Content: {"exclusions": [rule1]}
    • ASM WAF updates with: {"exclusions": [rule1]}
  • Payload2:
    • Product ASM_DD. New target file ASM_DD/file_34. Content: {"exclusions": [rule2]}
    • ASM WAF updates with: {"exclusions": [rule2]}
  • Payload3:
    • Product ASM_DD. Disable Target File ASM_DD/file_34. Content: {"exclusions": [rule2]}
    • ASM WAF updates with: {"exclusions": []}

With this PR the new behavior is:

  • Payload1:
    • Product ASM_DD. New target file ASM_DD/file_12. Content: {"exclusions": [rule1]}
    • ASM WAF updates with: {"exclusions": [rule1]}
  • Payload2:
    • Product ASM_DD. New target file ASM_DD/file_34. Content: {"exclusions": [rule2]}
    • ASM WAF updates with: {"exclusions": [rule1, rule2]}
  • Payload3:
    • Product ASM_DD. Disable Target File ASM_DD/file_34. Content: {"exclusions": [rule2]}
    • ASM WAF updates with: {"exclusions": [rule1]}

Affected versions

1.10.rcX

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Author is aware of the performance implications of this PR as reported in the benchmarks PR comment.

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer is aware of, and discussed the performance implications of this PR as reported in the benchmarks PR comment.

@avara1986 avara1986 added changelog/no-changelog A changelog entry is not required for this PR. ASM Application Security Monitoring labels Mar 6, 2023
@juanjux juanjux added this to the v1.10.0 milestone Mar 6, 2023
@pr-commenter
Copy link

pr-commenter bot commented Mar 6, 2023

Benchmarks

Comparing candidate commit 9b57b98 in PR branch avara1986/rcm-store-payloads with baseline commit 5755849 in branch 1.x.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 56 cases.

@avara1986 avara1986 changed the title feat(rcm): store payloads target files content fix(rcm): store payloads target files content Mar 6, 2023
@avara1986 avara1986 marked this pull request as ready for review March 6, 2023 14:26
@avara1986 avara1986 requested review from a team as code owners March 6, 2023 14:26
gnufede
gnufede previously approved these changes Mar 6, 2023
juanjux
juanjux previously approved these changes Mar 6, 2023
@avara1986 avara1986 dismissed stale reviews from juanjux, christophe-papazian, and gnufede via cae9f57 March 6, 2023 15:54
gnufede
gnufede previously approved these changes Mar 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2023

Codecov Report

Merging #5270 (9b57b98) into 1.x (5755849) will increase coverage by 0.09%.
The diff coverage is 95.67%.

@@            Coverage Diff             @@
##              1.x    #5270      +/-   ##
==========================================
+ Coverage   75.84%   75.93%   +0.09%     
==========================================
  Files         872      872              
  Lines       68572    68871     +299     
==========================================
+ Hits        52007    52296     +289     
- Misses      16565    16575      +10     
Impacted Files Coverage Δ
ddtrace/appsec/_remoteconfiguration.py 65.95% <78.78%> (-1.11%) ⬇️
ddtrace/internal/remoteconfig/client.py 54.51% <78.94%> (+1.29%) ⬆️
tests/appsec/test_remoteconfiguration.py 100.00% <100.00%> (ø)
.../internal/remoteconfig/test_remoteconfig_client.py 95.98% <100.00%> (+3.35%) ⬆️
...ernal/remoteconfig/test_remoteconfig_client_e2e.py 100.00% <100.00%> (ø)

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

nizox
nizox previously approved these changes Mar 6, 2023
juanjux
juanjux previously approved these changes Mar 6, 2023
@avara1986 avara1986 dismissed stale reviews from juanjux, nizox, and gnufede via ba17480 March 7, 2023 12:36
@juanjux juanjux enabled auto-merge (squash) March 7, 2023 17:54
@juanjux juanjux merged commit bdb1805 into 1.x Mar 7, 2023
@juanjux juanjux deleted the avara1986/rcm-store-payloads branch March 7, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants