-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Send HTTP Warning Header(s) for any Deprecation Usage from a REST request #17804
Send HTTP Warning Header(s) for any Deprecation Usage from a REST request #17804
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think that it should be used in other places.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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())); |
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 too is confusing.
/** | ||
* Attempts to do a scatter/gather request that expects unique responses per sub-request. | ||
*/ | ||
@AwaitsFix(bugUrl = "") |
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.
can we add text here/open up a issue?
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.
Created #19222 (and placed it appropriately)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be simpler to have the node remove its threadcontext from here on close
left one comment, other than that LGTM |
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.
dd44506
to
b927cfe
Compare
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. |
This adds support for
ThreadContext
s 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 headersThread
aware 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
DeprecationRestHandler
that logs a deprecation warning whenever it is used, but it otherwise behaves identically to normalRestHandler
s. 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