Export http helper functions #774
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## v3/dev #774 +/- ##
=======================================
Coverage 81.85% 81.85%
=======================================
Files 153 153
Lines 8188 8188
=======================================
Hits 6702 6702
Misses 1267 1267
Partials 219 219
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for this @redradrat. I am not too happy with exposing internal details in such a way. I'd really prefer to learn the reasons on why you can't use middleware (i.e. what is missing) and how we could improve it so you can use it. This change sounds like an early abstraction and we did not do that for coraza-caddy (where the code is almost identical but we can't reuse this middleware due to a signature difference) as we wanted to collect use cases and then come up with the right abstraction. Request for the feature, not for the implementation.
See
|
@jcchavezs really our first use case would have been to inject logic when an interruption was triggered. The simple example would be to report what happened with a prometheus gauge for example. I can adapt the PR to utilise functions as args for WrapHandler maybe? From my perspective the point here is, that coraza is a WAF framework, flexible for integration no? So having hooks at points within the http handler wrapper would make sense to inject logic, instead of replicating the handler. |
|
Yeah exactly, having hook functions would do the trick (see a similar
pattern in
https://github.com/openzipkin/zipkin-go/blob/master/middleware/http/server.go#L78).
We could have one to deal with the interruption, something like
func(types.Transaction, types.Interruption) so people can access the
transaction and the interruption (although you can access the interruption
from the transaction). In the future I also see ourselves writing one which
would access the request and the transaction but let's hold it until it is
the right time. The wrap method should accept a Options struct.
…On Wed, 12 Apr 2023, 15:24 Ralph Kühnert, ***@***.***> wrote:
@jcchavezs <https://github.com/jcchavezs> really our first use case would
have been to inject logic when an interruption was triggered. The simple
example would be to report what happened with a prometheus gauge for
example. I can adapt the PR to utilise functions as args for WrapHandler
maybe?
From my perspective the point here is, that coraza is a WAF framework,
flexible for integration no? So having hooks at points within the http
handler wrapper would make sense to inject logic, instead of replicating
the handler.
—
Reply to this email directly, view it on GitHub
<#774 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAVSHSAGQPZGUGQCU4DXA2UHZANCNFSM6AAAAAAW3MDAZ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
awesome... thanks for your thoughts. I'll give it a shot @jcchavezs |
|
Giving a second thought maybe we can accept a observer interface which would report metrics for this? Something like type Observer interface {
// onRequest is invoked when a request is started
func onRequest(*http.Request, types.Transaction)
// onInterruption is invoked when a request is interrupted
func onInterruption(*http.Request, types.Transaction)
}Ping @anuraaga |
|
Those hooks seem to make sense to me - I'm a bit hesitant on exposing |
|
Good point @anuraaga. Maybe something like TransactionState is what we are
looking for? Or just the ID?
…On Thu, 13 Apr 2023, 05:01 Anuraag Agrawal, ***@***.***> wrote:
Those hooks seem to make sense to me - I'm a bit hesitant on exposing
Transaction though as it would give a lot of surface for footgunning.
Ideally the information passed to the hook is something immutable - unless
we know of use cases that need mutability in hooks?
—
Reply to this email directly, view it on GitHub
<#774 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYASSQNKSKBTEBADH4STXA5UBBANCNFSM6AAAAAAW3MDAZ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
TransactionState is also mutable - basically we currently have Transaction as a view for use in middleware and TransactionState as a view for use in plugins. This would be more of a view for use in management or something like that - maybe having yet another view is overengineering though 🤔 |
|
Yeah, after looking at the transaction we might not need to access the
transaction at all? I was thinking that transaction ID would be useful for
logging purposes but you can achieve the same with seclang itself and when
it comes to metrics transaction ID is high cardinality so not worth I think
(feedback is welcome) so I would just skip transaction for now.
…On Thu, 13 Apr 2023, 07:11 Anuraag Agrawal, ***@***.***> wrote:
TransactionState is also mutable - basically we currently have Transaction
as a view for use in middleware and TransactionState as a view for use in
plugins. This would be more of a view for use in management or something
like that - maybe having yet another view is overengineering though 🤔
—
Reply to this email directly, view it on GitHub
<#774 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAXQ7RPZCO353HJPHJTXA6DGTANCNFSM6AAAAAAW3MDAZ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@redradrat so something like type TransactionCallback interface {
// onRequest is invoked when a request is started
func onRequest(*http.Request)
// onInterruption is invoked when a request is interrupted
func onInterruption(*http.Request, *types.Interruption)
}might do the trick. I wonder if matched rules (hold by transaction) is something we should also pass cc @piyushroshan and finally if we should include the Anyways, any feedback on the metrics needed would be great. Ping @RedXanadu as he might have insightful feedback. |
|
Any movement on this @redradrat ? |
|
Hey @jcchavezs. Actually, yes. We did quite some rewriting on the http package internally, I'll be cleaning that up to open another PR here. Hope you agree to the changes. We had to split up the Wrapper into a request and a response section because the interceptor would just not work for us on response... Long story short, I'm still up for pushing sthg here :) |
|
@jcchavezs @redradrat so... what's next here? |
|
Any movement on this @redradrat ? |
The http handler wrapper is great, but when compiling a custom transaction flow doesn't allow access to
ProcessRequestandProcessResponseanymore. The wrapper itself is cool, but in a lot of cases access to the actual transaction flow would be required. An example would be taking actions if an interruption occurred. e.g. producing metrics, notifying something, etc.So in general to have an easier time writing a custom, wrapper-like flow with the nice
Processhelpers, would be great to have them exported again.Make sure that you've checked the boxes below before you submit PR:
Thanks for your contribution ❤️