Send HTTP Warning Header(s) for any Deprecation Usage from a REST request #17804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pickypg
added
>enhancement
:Core/Infra/Logging
pickypg
force-pushed
the
feature/deprecated-rest-handler-17687
branch
from
8235156
to
f6e5a18
Compare
| * | ||
| * @return {@code true} if the {@code value} is not obviously wrong. | ||
| */ | ||
| public static boolean validHeaderValue(String value) { |
There was a problem hiding this comment.
I don't think this method belongs on Strings, especially since it's only used in only place. I'd be fine with it as a static package-private method on DeprecationRestChannel so it's visible for testing.
There was a problem hiding this comment.
I honestly think that it should be used in other places.
There was a problem hiding this comment.
That's probably correct, but right now it's not and I don't think Strings is the right place anyway.
There was a problem hiding this comment.
Moved to DeprecationRestChannel and also added String requireValidHeader(String value) that behaves similar to Objects.requireNonNull to use for all creations. I did leave it as public as it's used by DeprecationRestHandler (even though they're in the same package).
pickypg
force-pushed
the
feature/deprecated-rest-handler-17687
branch
from
f6e5a18
to
c1d086a
Compare
| @@ -1404,7 +1404,7 @@ private final EngineConfig newEngineConfig(EngineConfig.OpenMode openMode, Trans | |||
| return new EngineConfig(openMode, shardId, | |||
| threadPool, indexSettings, warmer, store, deletionPolicy, indexSettings.getMergePolicy(), | |||
| mapperService.indexAnalyzer(), similarityService.similarity(mapperService), codecService, shardEventListener, translogRecoveryPerformer, indexCache.query(), cachingPolicy, translogConfig, | |||
| indexSettings.getSettings().getAsTime(IndexingMemoryController.SHARD_INACTIVE_TIME_SETTING, IndexingMemoryController.SHARD_DEFAULT_INACTIVE_TIME)); | |||
| IndexingMemoryController.SHARD_INACTIVE_TIME_SETTING.get(indexSettings.getSettings())); | |||
| /** | ||
| * Attempts to do a scatter/gather request that expects unique responses per sub-request. | ||
| */ | ||
| @AwaitsFix(bugUrl = "") |
There was a problem hiding this comment.
can we add text here/open up a issue?
There was a problem hiding this comment.
Created #19222 (and placed it appropriately)
pickypg
force-pushed
the
feature/deprecated-rest-handler-17687
branch
2 times, most recently
from
4f44659
to
af4fc01
Compare
|
@jaymode Please take a second pass now that I have rebased it back onto master. I think it's ready to merge with all tests passing. |
| } | ||
|
|
||
| /** | ||
| * Note: {@code threadContexts} <em>must</em> be a modifiable {@link Set} so that stale contexts can be removed. |
There was a problem hiding this comment.
I think it would be simpler to have the node remove its threadcontext from here on close
|
left one comment, other than that LGTM |
pickypg
force-pushed
the
feature/deprecated-rest-handler-17687
branch
from
af4fc01
to
8079da3
Compare
This adds a new proxy for RestHandlers and RestControllers so that requests made to deprecated REST APIs can be automatically logged in the ES logs via the DeprecationLogger as well as via a "Warning" header (RFC-7234) for all responses.
pickypg
force-pushed
the
feature/deprecated-rest-handler-17687
branch
from
dd44506
to
b927cfe
Compare
pickypg
changed the title
|
In case anyone in the internet superfuture runs into this like we did, this can pretty easily cause a long enough set of headers that nginx cannot even parse the response, and thus can cause pretty serious outages. Would be great if it could be disabled entirely, but you can set |
|
@frioux Can you share the contents of these large responses? I would be interested in seeing if there are headers that we need to de-duplicate (we do this already to some extent, this is part of what pushes my interest here). |
Of course instead of |
|
Okay; it is because it's a distinct field. Thanks for that, I will see what, if anything, can be done (I am doubtful because of the way these are collected). |
|
It would be good to limit this, because this is pretty gnarly to debug (I could only do it with a network trace) and anyone who puts |
|
I agree with you. I opened #28301 and this work is assigned. |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds support for
ThreadContexts to maintain unique Response headers (repeated header values are ignored) in addition to Request headers. In particular, this enables REST requests (e.g.,PUT /test) that hop across the nodes to properly return headers as part of the overall request. With the response headersThreadaware thanks to theThreadContext, we can associate individual requests with deprecation logging and therefore warn developers when they unwittingly use deprecated features:The headers that would get returned by this would be:
Alongside that feature, this adds a
DeprecationRestHandlerthat logs a deprecation warning whenever it is used, but it otherwise behaves identically to normalRestHandlers. This allows REST APIs to be deprecated in a consistent manner instead of dropped without any non-documentation-level deprecation warnings in advance.Closes #17687