-
-
Notifications
You must be signed in to change notification settings - Fork 330
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
[feature] request blocking by http headers #2409
[feature] request blocking by http headers #2409
Conversation
aacdc7b
to
25714c5
Compare
internal/db/bundb/migrations/20231212144715_add_header_filters.go
Outdated
Show resolved
Hide resolved
internal/headerfilter/filter.go
Outdated
*regexp.Regexp | ||
|
||
// match count. | ||
n atomic.Uint64 |
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.
This can probably be dropped, if we're going to use prometheus/otel metrics for this then we can use a counter for this instead when we get around to it.
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.
oh I was just going to return this as a counter value, as I'm pretty sure that's what it uses under the hood is atomic ints etc. the plan is to make that stats function export Prometheus compatible data at some point. also this way I have some stat collection enabled required for one of the milestone bulletpoints
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.
The way counters work in OTel is I think to just call Add(i) on the counter when a value must be incremented, and let OTel handle the rest; you don't actually need to keep track in the application of the counter's current value afaik. That's one thing that makes OTel so nice :) I mean we could implement it ourself and serve that raw value, but it seems unnecessary unless there's some specific benefit / bug workaround in doing so, I think.
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.
gotcha. maybe in the tracing / metric package i'll wrap it so it works with our enabling / disabling mechanism (otherwise this will make it a hard dependency in all cases)
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.
so for now i've kept-in an initial statistics tracking implementation, but i have rejigged the header matching so the matched header key and regular expression are returned which should make it easier to instrument with otel in the future. see: eaa29f9
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.
It's looking great! Coming along nicely!
…ch on header value > MaxHeaderValue
b48fd07
to
78bfd08
Compare
… rejig to return matched header + regex
Description
HeaderFilter()
middleware to filter requests by their header content/docs/admin/header_filtering_modes.md
documentation laying out the logic of the http request filtering modesChecklist
go fmt ./...
andgolangci-lint run
.