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

contrib: stop caching appsec.Enabled() value for gin/echo #1732

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Feb 13, 2023

What does this PR do?

Explicitely call appsec.Enabled() every time we want to check that appsec is enabled, instead of calling
it once and only using the cached value.

Motivation

Appsec can now be enabled at runtime through remote configuration meaning that the value returned by appsec.Enabled()
can change during execution. Explicitely calling appsec.Enabled() makes sure the actual value used in our conditions is accurate.

Describe how to test/QA your changes

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

@Hellzy Hellzy requested a review from a team February 13, 2023 14:53
@pr-commenter
Copy link

pr-commenter bot commented Feb 13, 2023

Benchmarks

Comparing candidate commit e030b3e in PR branch francois.mazeau/appsec-enabled-check with baseline commit f98f668 in branch main.

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

@Hellzy Hellzy added this to the v1.48.0 milestone Feb 13, 2023
@Julio-Guerra
Copy link
Contributor

Thanks, remote activation is saved ;-p

@Hellzy Hellzy merged commit f1d81e0 into main Feb 14, 2023
@Hellzy Hellzy deleted the francois.mazeau/appsec-enabled-check branch February 14, 2023 08:38
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