-
Notifications
You must be signed in to change notification settings - Fork 375
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
Introduce AppSec::Instrumentation::Gateway::Argument #2648
Conversation
9b9987d
to
da2e304
Compare
- Replace contrib Rack::Request and Rack::Response with Gateway::Request and Gateway::Response
da2e304
to
c8a200f
Compare
9a64baa
to
f55473c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except usage of AppSec::Processor.active_context
(which uses a thread local) should be made minimal when it can be accessed reliably otherwise.
module Rails | ||
module Gateway | ||
# Gateway Request argument. Normalized extration of data from ActionDispatch::Request | ||
class Request < Instrumentation::Gateway::Argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to inherit from Contrib::Rack::Gateway::Request
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I thought about, but since I saw we use the Rack::RequestBody
directly in the Sinatra contrib folder I thought it would be ok.
I'm happy to change it.
20abc18
to
9043bcd
Compare
Codecov Report
@@ Coverage Diff @@
## master #2648 +/- ##
==========================================
- Coverage 98.08% 98.08% -0.01%
==========================================
Files 1160 1166 +6
Lines 63676 63832 +156
Branches 2849 2849
==========================================
+ Hits 62457 62608 +151
- Misses 1219 1224 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This PR introduces
AppSec::Instrumentation::Gateway::Argument
class.This class is the expected object to be provided when passing information to
Gateway#push
Each contrib integration can define their own
Argument
objects. The idea is to encapsulate all the logic regarding argument handling inside those classes. This helps with encapsulation and ease of testing each argument individually.For now, the parent class
AppSec::Instrumentation::Gateway::Argument
has no method defined, as there isn't any shared logic among the types of arguments. This could change in the future.The different classes added in this PR are:
Datadog::AppSec::Instrumentation::Gateway::User
Datadog::AppSec::Contrib::Rack::Gateway::Request
Datadog::AppSec::Contrib::Rack::Gateway::Response
Datadog::AppSec::Contrib::Sinatra::Gateway::Request
Datadog::AppSec::Contrib::Sinatra::Gateway::RouteParams
Datadog::AppSec::Contrib::Rails::Gateway::Request
AppSec::Instrumentation::Gateway::User
is a particular case since it is framework agnostic and currently is only used inkit/identity.rb
For that reason, the definition of the class is inside
lib/datadog/appsec/instrumentation/gateway/argument.rb
and not in anycontrib
folder.