Validate monitoring header overrides at parse time #47848
Conversation
|
Pinging @elastic/es-core-features (:Core/Features/Monitoring) |
|
Thanks, @jakelandis, there are existing tests for this setting. |
|
@elastic/es-core-infra, this work covers both monitoring and settings if you want to review it. |
| final Set<String> names = settings.names(); | ||
| for (String name : names) { | ||
| final String fullSetting = key + "." + name; | ||
| if (HttpExporter.BLACKLISTED_HEADERS.stream().anyMatch(name::equalsIgnoreCase)) { |
There was a problem hiding this comment.
Why use equalsIgnoreCase? I don't see where we've been case insensitive in setting names before, and I'm not sure we should change behavior here while trying to add validation.
There was a problem hiding this comment.
HTTP headers are supposed to be case insensitive, so without the case insensitive check, a user could supply an override for "content-length" even though "Content-Length" is on the header override blacklist. If that's not a concern here or this is the wrong place to check that, I can make it a case sensitive check.
There was a problem hiding this comment.
@rjernst, would you prefer the HTTP header name check here remain case-sensitive?
There was a problem hiding this comment.
I'm open to it being case-insensitive, but changing that behavior should be separate PR. This PR is just about adding validation to the parsing as it currently exists.
There was a problem hiding this comment.
@rjernst, I've restored the original case-sensitive check here.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
Provides parse-time validation for
HEADERS_SETTINGas described in #47711.Also changes the check for blacklisted header overrides to be case-insensitive.