Add settings to control size and count of warning headers in responses #28427

mayya-sharipova · 2018-01-29T18:41:57Z

Add a static cluster level setting "http.max_warning_header_count"
to control the maximum number of warning headers in client HTTP responses.
Defaults to unbounded.

Add a static cluster level setting "http.max_warning_header_size"
to control the maximum total size of warning headers in client HTTP responses.
Defaults to unbounded.

Closes #28301

jasontedor · 2018-02-15T19:31:23Z

server/src/main/java/org/elasticsearch/http/HttpTransportSettings.java

+    public static final Setting<Integer> SETTING_HTTP_MAX_WARNING_HEADER_COUNT =
+        Setting.intSetting("http.max_warning_header_count", 62, 0, Setting.Property.Dynamic, Property.NodeScope);
+    // Setting the default max count of warning header size to 7Kb, as default max size for all types of headers is 8Kb.
+    // This leaves 1Kb for  headers of content-type and content-length.


I am not sure what default you're referring to here? As far as I know, there is no default limit enforced by Elasticsearch on header count nor aggregate header size on HTTP responses?

@jasontedor Thanks for the comment. Yes, you are right, there is no default limits enforced by ES. These were the default settings used by Elastic Cloud, and I thought they are more or less universal. What defaults would you recommend?

Philosophically I do not think our defaults should be based on Cloud constraints. It’s a separate product with specific limitations because of the nature of the infrastructure there. Additionally, their constraints can change on a lifecycle independent of our own. Instead, we should choose what makes sense for the wider user base while Cloud can utilize these knobs to tune to their infrastructure. I think about a few things here:

a bounded default is a behavioral breaking change in a minor release; we should probably not do that unless we consider unbounded a bug

I think it is a mistake to silently truncate

if a client application blows up because Elasticsearch sends it too large of response headers, this is a problem that is easily searchable for solutions to (where developers of such application would stumble on these settings and tune their Elasticsearch instance in response)

Therefore, I lean towards the default being unbounded. What do you think?

DaveCTurner · 2018-02-16T08:46:22Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

+            if (key.equals("Warning")) {
+                // check if we can add another warning header, if max count or size exceeded
+                final int warningHeaderCount = this.responseHeaders.containsKey("Warning")? this.responseHeaders.get("Warning").size() : 0;
+                if (warningHeaderCount >= maxWarningHeaderCount) return this; //can't add max count reached


I think it'd be useful to get one more warning when the limit is hit, saying that there were more warnings but we dropped them because http.max_warning_header_count is set to <n>, and similarly for the size limit.

This is a great suggestion, I think it should be a warning in the server logs as opposed to an additional warning header that eats into our budgets.

I think it's more useful if we spend the last of the budget saying the list is truncated, in anticipation of a time when someone forgets that the list they're looking at might be truncated and makes a poor decision on the assumption that it's complete. These excessive warnings are often repetitive, so the specifics of the 62nd warning seem less valuable to me.

(Not that it shouldn't be in the server logs too.)

My concern here is if a user sets the budget to H headers and B bytes because they can not handle more than that (e.g., the common case being a proxy) then we have to subtract a header (or possibly many) to stay under the (H, B) budget after we include the missed warnings warning.

…responses Add a dynamic persistent cluster level setting "http.max_warning_header_count" to control the maximum number of warning headers in client HTTP responses. Defaults to unbounded. Add a dynamic persistent cluster level setting "http.max_warning_header_size" to control the maximum total size of warning headers in client HTTP responses. Defaults to unbounded. Closes elastic#28301

mayya-sharipova · 2018-03-09T16:12:33Z

@jasontedor When you have time, can you please continue to review. Sorry that I have to rebase the code, I could not run it otherwise with new x-pack.

jasontedor · 2018-03-09T16:33:44Z

@mayya-sharipova Did you push new code, or only rebase? I have not received any notification of a push of new code?

mayya-sharipova · 2018-03-09T17:00:32Z

@jasontedor I have pushed the new code. But I have to overwrite my previous commit, sorry for that.

jasontedor · 2018-03-09T17:01:35Z

@mayya-sharipova I would ask that you please merge master in instead of rebasing, and push changes to the PR as separate commits. It makes it easier on reviewers.

jasontedor

Thanks @mayya-sharipova; I left some comments.

jasontedor · 2018-03-17T19:15:17Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -81,6 +88,8 @@
    private static final ThreadContextStruct DEFAULT_CONTEXT = new ThreadContextStruct();
    private final Map<String, String> defaultHeader;
    private final ContextThreadLocal threadLocal;
+    private static volatile int maxWrnHeaderCount;
+    private static volatile long maxWrnHeaderSize;


maxWrnHeaderSize -> maxWarningHeaderSize

jasontedor · 2018-03-17T19:16:15Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -81,6 +88,8 @@
    private static final ThreadContextStruct DEFAULT_CONTEXT = new ThreadContextStruct();
    private final Map<String, String> defaultHeader;
    private final ContextThreadLocal threadLocal;
+    private static volatile int maxWrnHeaderCount;


Let's use traditional Java variable style names here (e.g., maxWarningHeaderCount).

jasontedor · 2018-03-17T19:16:45Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

    }

    @Override
    public void close() throws IOException {
        threadLocal.close();
    }

+    public static void setMaxWarningHeaderCount(int newMaxWrnHeaderCount){


Nit: spacing between the ) and {: ){ -> ) {

jasontedor · 2018-03-17T19:17:21Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

    }

    @Override
    public void close() throws IOException {
        threadLocal.close();
    }

+    public static void setMaxWarningHeaderCount(int newMaxWrnHeaderCount){


newMaxWrnHeaderCount -> maxWarningHeaderCount(and reference the member field by dereferencingthis` then).

jasontedor · 2018-03-17T19:17:35Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

+        maxWrnHeaderCount = newMaxWrnHeaderCount;
+    }
+
+    public static void setMaxWarningHeaderSize(ByteSizeValue newMaxWarningHeaderSize){


){ -> ) {

jasontedor · 2018-03-17T19:27:03Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

-            return new ThreadContextStruct(requestHeaders, newResponseHeaders, transientHeaders, isSystemContext);
+        //replace last warning header(s) with "headers limit reached" warning
+        //respecting limitations on headers size if it is set by user
+        private ThreadContextStruct addWrnLmtReachedHeader(){


The user already expects if they have set this setting that warning headers will be truncated. Therefore, I don't think we should do this, losing a warning header (or more) at the expense of a warning header saying that there are more warnings that can be found in the logs. Instead I think that we should log (in the main Elasticsearch log) when we truncate.

jasontedor · 2018-03-17T19:41:39Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

+                if (isWrnLmtReached) return this; //can't add warning headers - limit reached
+                if (maxWrnHeaderCount != -1) { //if count is NOT unbounded, check its limits
+                    int wrnHeaderCount = this.responseHeaders.containsKey("Warning") ? this.responseHeaders.get("Warning").size() : 0;
+                    if (wrnHeaderCount >= maxWrnHeaderCount) return addWrnLmtReachedHeader();


Take care, the warning headers are de-duplicated below. The accounting should happen after de-duplication, otherwise we truncate when a warning header would not have been added because it is a duplicate of an existing warning header.

jasontedor · 2018-03-17T19:42:06Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -359,7 +378,8 @@ default void restore() {
        private final Map<String, Object> transientHeaders;
        private final Map<String, List<String>> responseHeaders;
        private final boolean isSystemContext;
-
+        private long wrnHeadersSize; //saving current warning headers' size not to recalculate the size with every new warning header


wrnHeaderSize -> warningHeaderSize

jasontedor · 2018-03-17T19:42:31Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -359,7 +378,8 @@ default void restore() {
        private final Map<String, Object> transientHeaders;
        private final Map<String, List<String>> responseHeaders;
        private final boolean isSystemContext;
-
+        private long wrnHeadersSize; //saving current warning headers' size not to recalculate the size with every new warning header
+        private boolean isWrnLmtReached;


Is this field strictly needed?

I think yes. this field once the limit is reached ensures that we don't check and log "warnings limit reached" again and again with a new warning header.

jasontedor · 2018-03-17T19:42:54Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -442,6 +478,19 @@ private ThreadContextStruct putResponseHeaders(Map<String, List<String>> headers

        private ThreadContextStruct putResponse(final String key, final String value, final Function<String, String> uniqueValue) {
            assert value != null;
+            long curWrnHeaderSize = 0;


curWrnHeaderSize -> currentWarningHeaderSize

Add a dynamic persistent cluster level setting "http.max_warning_header_count" to control the maximum number of warning headers in client HTTP responses. Defaults to unbounded. Add a dynamic persistent cluster level setting "http.max_warning_header_size" to control the maximum total size of warning headers in client HTTP responses. Defaults to unbounded. Once any of these limits is exceeded this will be logged in the main ES log, and any more warning headers for this response will be ignored. Closes elastic#28301

mayya-sharipova · 2018-03-21T20:17:42Z

@jasontedor Hi Jason! I have tried to address you comments in my last commit. Can you please continue the review, when you have available time

jasontedor

I left a comment about the docs, and I left two comments that tie together to make me thing that a larger change is needed here. I think that what we want to do is push the current value of the maxWarningHeaderCount and maxWarningHeaderSize into the ThreadContextStruct and not update them, but only new instances would see the updated values.

jasontedor · 2018-03-27T19:38:54Z

docs/reference/modules/http.asciidoc

+almost all of them are not dynamically updatable. For them to take effect
+they should be set in `elasticsearch.yml`. The only dynamically updatable
+settings are `http.max_warning_header_count` and
+`http.max_warning_header_size`.


Let us reword this slightly. Let us leave out the specific settings that are dynamically updatable (and notate the ones that are dynamically updatable right alongside the setting itself) otherwise I could see these easily drifting away from being accurate if a new dynamic setting is added. Let us also say:

The settings in the table below can be configured for HTTP. Note that some of these settings are dynamic and can be updated via the <<cluster-update-settings, cluster update settings API>> and the remainder must be set in the Elasticsearch <<settings, configuration file>>.

jasontedor · 2018-03-27T20:34:30Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

-            return new ThreadContextStruct(requestHeaders, newResponseHeaders, transientHeaders, isSystemContext);
+            //check if we can add another warning header - if max count within limits
+            if ((key.equals("Warning")) && (maxWarningHeaderCount != -1)) { //if count is NOT unbounded, check its limits
+                final int warningHeaderCount = newResponseHeaders.containsKey("Warning") ? newResponseHeaders.get("Warning").size() : 0;


I think there is a problem here. Imagine that maxWarningHeaderCount is set (not equal to -1) and then a user dynamically sets it to unbounded before the if statement on the following line (i.e., after we have passed the set condition). Then we will see the updated value and warningHeaderCount will always be greater than maxWarningHeaderCount. We want to avoid these sorts of race conditions.

jasontedor · 2018-03-27T20:35:35Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -98,13 +107,23 @@ public ThreadContext(Settings settings) {
            this.defaultHeader = Collections.unmodifiableMap(defaultHeader);
        }
        threadLocal = new ContextThreadLocal();
+        maxWarningHeaderCount = SETTING_HTTP_MAX_WARNING_HEADER_COUNT.get(settings);
+        maxWarningHeaderSize = SETTING_HTTP_MAX_WARNING_HEADER_SIZE.get(settings).getBytes();


This is a code smell, setting static fields from an instance constructor. I think that we want to do something different here, so that these fields are instance fields. I can offer a suggestion on how to do this, but I prefer to let you spend some time coming up with a solution on your own (I don't want to take the joy of problem solving away from you).

Add a static persistent cluster level setting "http.max_warning_header_count" to control the maximum number of warning headers in client HTTP responses. Defaults to unbounded. Add a static persistent cluster level setting "http.max_warning_header_size" to control the maximum total size of warning headers in client HTTP responses. Defaults to unbounded. Once any of these limits is exceeded this will be logged in the main ES log, and any more warning headers for this response will be ignored.

…ntrol-warning-headers

mayya-sharipova · 2018-03-29T21:40:44Z

@jasontedor I have made these settings static. Can you please continue the review when you have time. Thanks a lot.

jasontedor

I left a comment about logging when we drop.

jasontedor · 2018-04-04T01:13:27Z

server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

+            long newWarningHeaderSize = warningHeadersSize;
+            //check if we can add another warning header - if max size within limits
+            if (key.equals("Warning")) {
+                if (isWarningLimitReached) return this; // can't add warning headers - limit reached


I think that we want to log each time that we drop a warning header, not only the first time for a given request. Also we can be more precise than the current implementation which says one or the other condition is met, but we always know exactly which condition it is so we can help the user more by letting them know.

…ntrol-warning-headers

Add a static persistent cluster level setting "http.max_warning_header_count" to control the maximum number of warning headers in client HTTP responses. Defaults to unbounded. Add a static persistent cluster level setting "http.max_warning_header_size" to control the maximum total size of warning headers in client HTTP responses. Defaults to unbounded. With every warning header that exceeds these limits, a message will be logged in the main ES log, and any more warning headers for this response will be ignored.

mayya-sharipova · 2018-04-12T19:51:06Z

@elasticmachine run sample packaging tests

jasontedor

LGTM.

* master: Enable skipping fetching latest for BWC builds (elastic#29497) Add remote cluster client (elastic#29495) Ensure flush happens on shard idle Adds SpanGapQueryBuilder in the query DSL (elastic#28636) Control max size and count of warning headers (elastic#28427) Make index APIs work without types. (elastic#29479) Deprecate filtering on `_type`. (elastic#29468) Fix auto-generated ID example format (elastic#29461) Fix typo in max number of threads check docs (elastic#29469) Add primary term to translog header (elastic#29227) Add a helper method to get a random java.util.TimeZone (elastic#29487) Move TimeValue into elasticsearch-core project (elastic#29486)

* es/master: Add remote cluster client (#29495) Ensure flush happens on shard idle Adds SpanGapQueryBuilder in the query DSL (#28636) Control max size and count of warning headers (#28427) Make index APIs work without types. (#29479) Deprecate filtering on `_type`. (#29468) Fix auto-generated ID example format (#29461) Fix typo in max number of threads check docs (#29469) Add primary term to translog header (#29227) Add a helper method to get a random java.util.TimeZone (#29487) Move TimeValue into elasticsearch-core project (#29486) Fix NPE in InternalGeoCentroidTests#testReduceRandom (#29481) Build: introduce keystoreFile for cluster config (#29491) test: Index more docs, so that it is less likely the search request does not time out.

Add a static persistent cluster level setting "http.max_warning_header_count" to control the maximum number of warning headers in client HTTP responses. Defaults to unbounded. Add a static persistent cluster level setting "http.max_warning_header_size" to control the maximum total size of warning headers in client HTTP responses. Defaults to unbounded. With every warning header that exceeds these limits, a message will be logged in the main ES log, and any more warning headers for this response will be ignored.

* 6.x: [TEST] REST client request without leading '/' (#29471) Prevent accidental changes of default values (#29528) [Docs] Add definitions to glossary (#29127) Avoid self-deadlock in the translog (#29520) Minor cleanup in NodeInfo.groovy Lazy configure build tasks that require older JDKs (#29519) Fix usage of NodeInfo#nodeVersion in build Control max size/count of warning headers (#28427) (#29516) Simplify snapshot check in root build file Make NodeInfo#nodeVersion strongly-typed as Version (#29515) Enable license header exclusions (#29379) Use proper Java version for BWC builds (#29493) Mute TranslogTests#testFatalIOExceptionsWhileWritingConcurrently

mayya-sharipova added :Core/Infra/Core Core issues without another label >enhancement v6.3.0 v7.0.0 labels Jan 29, 2018

jasontedor reviewed Feb 15, 2018

View reviewed changes

DaveCTurner reviewed Feb 16, 2018

View reviewed changes

nik9000 mentioned this pull request Feb 26, 2018

max_analyzed_offset warnings in header part of ES response #28830

Closed

mayya-sharipova force-pushed the add-setting-to-control-warning-headers branch from bfd9872 to eb992fb Compare March 8, 2018 23:48

mayya-sharipova force-pushed the add-setting-to-control-warning-headers branch from eb992fb to 9a3f8ad Compare March 9, 2018 03:28

jasontedor reviewed Mar 17, 2018

View reviewed changes

mayya-sharipova added 2 commits March 20, 2018 15:47

Merge branch 'master' into add-setting-to-control-warning-headers

48a5768

jasontedor requested changes Mar 27, 2018

View reviewed changes

mayya-sharipova added 2 commits March 29, 2018 11:09

Merge remote-tracking branch 'upstream/master' into add-setting-to-co…

e2310f0

…ntrol-warning-headers

jasontedor requested changes Apr 4, 2018

View reviewed changes

mayya-sharipova added 2 commits April 5, 2018 14:39

Merge remote-tracking branch 'upstream/master' into add-setting-to-co…

9e6de75

…ntrol-warning-headers

jasontedor approved these changes Apr 13, 2018

View reviewed changes

mayya-sharipova merged commit 5dcfdb0 into elastic:master Apr 13, 2018

mayya-sharipova deleted the add-setting-to-control-warning-headers branch April 13, 2018 09:55

jasontedor mentioned this pull request Sep 6, 2018

Set a bounded default for http.max_warning_header_count #33479

Closed

This was referenced Dec 5, 2018

Support Travis VM Infrastructure pelias/schema#337

Closed

Backport deprecation warning header limits to 5.6 #36243

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add settings to control size and count of warning headers in responses #28427

Add settings to control size and count of warning headers in responses #28427

mayya-sharipova commented Jan 29, 2018 •

edited

jasontedor Feb 15, 2018

mayya-sharipova Feb 15, 2018

jasontedor Feb 16, 2018 •

edited

DaveCTurner Feb 16, 2018

jasontedor Feb 16, 2018

DaveCTurner Feb 16, 2018

jasontedor Feb 16, 2018

mayya-sharipova commented Mar 9, 2018

jasontedor commented Mar 9, 2018

mayya-sharipova commented Mar 9, 2018

jasontedor commented Mar 9, 2018

jasontedor left a comment

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

jasontedor Mar 17, 2018

mayya-sharipova Mar 19, 2018

jasontedor Mar 17, 2018

mayya-sharipova commented Mar 21, 2018

jasontedor left a comment

jasontedor Mar 27, 2018

jasontedor Mar 27, 2018

jasontedor Mar 27, 2018

mayya-sharipova commented Mar 29, 2018

jasontedor left a comment

jasontedor Apr 4, 2018

mayya-sharipova commented Apr 12, 2018

jasontedor left a comment

Add settings to control size and count of warning headers in responses #28427

Add settings to control size and count of warning headers in responses #28427

Conversation

mayya-sharipova commented Jan 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor Feb 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Mar 9, 2018

jasontedor commented Mar 9, 2018

mayya-sharipova commented Mar 9, 2018

jasontedor commented Mar 9, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Mar 21, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Mar 29, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented Apr 12, 2018

jasontedor left a comment

Choose a reason for hiding this comment

mayya-sharipova commented Jan 29, 2018 •

edited

jasontedor Feb 16, 2018 •

edited