Skip to content

Commit 08df1cb

Browse files
masseykemasseyke
authored
Changing HeaderWarning to always use 299 as the warning code (#80304)
This commit changes the deprecation logger so that all messages (critical or warning) are written out with "299" as the level at the beginning of the header in order to be compliant with https://www.rfc-editor.org/rfc/rfc7234.html#section-5.5.7. In #79107 we mistakenly began logging warning-level messages with the 300 code, which is not valid in the RFC.
1 parent d90fa4e commit 08df1cb

File tree

15 files changed

+41
-67
lines changed

15 files changed

+41
-67
lines changed

‎modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpProcessor.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -486,7 +486,6 @@ public Processor create(
486486
boolean valid = metadata.isValid(currentState.metadata().settings());
487487
if (valid && metadata.isCloseToExpiration()) {
488488
HeaderWarning.addWarning(
489-
DeprecationLogger.CRITICAL,
490489
"database [{}] was not updated for over 25 days, geoip processor"
491490
+ " will stop working if there is no update for 30 days",
492491
databaseFile

‎rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
"Test put and reset transient settings":
33
- skip:
4-
version: " - 7.15.99"
4+
version: " - 7.99.99"
55
reason: "transient settings deprecation"
66
features: "warnings"
77

‎server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import org.elasticsearch.common.ValidationException;
3131
import org.elasticsearch.common.compress.CompressedXContent;
3232
import org.elasticsearch.common.inject.Inject;
33-
import org.elasticsearch.common.logging.DeprecationLogger;
3433
import org.elasticsearch.common.logging.HeaderWarning;
3534
import org.elasticsearch.common.regex.Regex;
3635
import org.elasticsearch.common.settings.IndexScopedSettings;
@@ -570,7 +569,7 @@ public ClusterState addIndexTemplateV2(
570569
name
571570
);
572571
logger.warn(warning);
573-
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
572+
HeaderWarning.addWarning(warning);
574573
}
575574

576575
ComposableIndexTemplate finalIndexTemplate = template;
@@ -959,7 +958,7 @@ static ClusterState innerPutTemplate(
959958
request.name
960959
);
961960
logger.warn(warning);
962-
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
961+
HeaderWarning.addWarning(warning);
963962
} else {
964963
// Otherwise, this is a hard error, the user should use V2 index templates instead
965964
String error = String.format(

‎server/src/main/java/org/elasticsearch/common/logging/HeaderWarning.java‎

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
package org.elasticsearch.common.logging;
1010

11-
import org.apache.logging.log4j.Level;
1211
import org.elasticsearch.Build;
1312
import org.elasticsearch.Version;
1413
import org.elasticsearch.common.util.concurrent.ThreadContext;
@@ -33,10 +32,10 @@
3332
public class HeaderWarning {
3433
/**
3534
* Regular expression to test if a string matches the RFC7234 specification for warning headers. This pattern assumes that the warn code
36-
* is always 299 or 300. Further, this pattern assumes that the warn agent represents a version of Elasticsearch including the build
35+
* is always 299. Further, this pattern assumes that the warn agent represents a version of Elasticsearch including the build
3736
* hash.
3837
*/
39-
public static final Pattern WARNING_HEADER_PATTERN = Pattern.compile("(?:299|300) " + // log level code
38+
public static final Pattern WARNING_HEADER_PATTERN = Pattern.compile("299 " + // log level code
4039
"Elasticsearch-" + // warn agent
4140
"\\d+\\.\\d+\\.\\d+(?:-(?:alpha|beta|rc)\\d+)?(?:-SNAPSHOT)?-" + // warn agent
4241
"(?:[a-f0-9]{7}(?:[a-f0-9]{33})?|unknown) " + // warn agent
@@ -54,15 +53,14 @@ public class HeaderWarning {
5453

5554
/*
5655
* RFC7234 specifies the warning format as warn-code <space> warn-agent <space> "warn-text" [<space> "warn-date"]. Here, warn-code is a
57-
* three-digit number with various standard warn codes specified, and is left off of this static prefix so that it can be added based
58-
* on the log level received. The warn code will be either 299 or 300 at runtime, which are apt for our purposes as
59-
* they represent miscellaneous persistent warnings (can be presented to a human, or logged, and must not be removed by a cache).
60-
* The warn-agent is an arbitrary token; here we use the Elasticsearch version and build hash. The warn text must be quoted. The
61-
* warn-date is an optional quoted field that can be in a variety of specified date formats; here we use RFC 1123 format.
56+
* three-digit number with various standard warn codes specified. The warn code 299 is apt for our purposes as it represents a
57+
* miscellaneous persistent warning (can be presented to a human, or logged, and must not be removed by a cache). The warn-agent is an
58+
* arbitrary token; here we use the Elasticsearch version and build hash. The warn text must be quoted. The warn-date is an optional
59+
* quoted field that can be in a variety of specified date formats; here we use RFC 1123 format.
6260
*/
6361
private static final String WARNING_PREFIX = String.format(
6462
Locale.ROOT,
65-
" Elasticsearch-%s%s-%s",
63+
"299 Elasticsearch-%s%s-%s",
6664
Version.CURRENT.toString(),
6765
Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : "",
6866
Build.CURRENT.hash()
@@ -191,15 +189,14 @@ private static boolean assertWarningValue(final String s, final String warningVa
191189
* Format a warning string in the proper warning format by prepending a warn code, warn agent, wrapping the warning string in quotes,
192190
* and appending the RFC 7231 date.
193191
*
194-
* @param level the level of the warning - Level.WARN or DeprecationLogger.CRITICAL
195192
* @param s the warning string to format
196193
* @return a warning value formatted according to RFC 7234
197194
*/
198-
public static String formatWarning(final Level level, final String s) {
195+
public static String formatWarning(final String s) {
199196
// Assume that the common scenario won't have a string to escape and encode.
200197
int length = WARNING_PREFIX.length() + s.length() + 6;
201198
final StringBuilder sb = new StringBuilder(length);
202-
sb.append(level.intLevel() + WARNING_PREFIX).append(" \"").append(escapeAndEncode(s)).append("\"");
199+
sb.append(WARNING_PREFIX).append(" \"").append(escapeAndEncode(s)).append("\"");
203200
return sb.toString();
204201
}
205202

@@ -313,21 +310,16 @@ public static String getXOpaqueId() {
313310
.orElse("");
314311
}
315312

316-
public static void addWarning(Level level, String message, Object... params) {
317-
addWarning(THREAD_CONTEXT, level, message, params);
313+
public static void addWarning(String message, Object... params) {
314+
addWarning(THREAD_CONTEXT, message, params);
318315
}
319316

320317
// package scope for testing
321318
static void addWarning(Set<ThreadContext> threadContexts, String message, Object... params) {
322-
addWarning(threadContexts, DeprecationLogger.CRITICAL, message, params);
323-
}
324-
325-
// package scope for testing
326-
static void addWarning(Set<ThreadContext> threadContexts, Level level, String message, Object... params) {
327319
final Iterator<ThreadContext> iterator = threadContexts.iterator();
328320
if (iterator.hasNext()) {
329321
final String formattedMessage = LoggerMessageFormat.format(message, params);
330-
final String warningHeaderValue = formatWarning(level, formattedMessage);
322+
final String warningHeaderValue = formatWarning(formattedMessage);
331323
assert WARNING_HEADER_PATTERN.matcher(warningHeaderValue).matches();
332324
assert extractWarningValueFromWarningHeader(warningHeaderValue, false).equals(escapeAndEncode(formattedMessage));
333325
while (iterator.hasNext()) {

‎server/src/main/java/org/elasticsearch/common/logging/HeaderWarningAppender.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ public void append(LogEvent event) {
3535
String messagePattern = esLogMessage.getMessagePattern();
3636
Object[] arguments = esLogMessage.getArguments();
3737

38-
HeaderWarning.addWarning(event.getLevel(), messagePattern, arguments);
38+
HeaderWarning.addWarning(messagePattern, arguments);
3939
} else {
4040
final String formattedMessage = event.getMessage().getFormattedMessage();
41-
HeaderWarning.addWarning(event.getLevel(), formattedMessage);
41+
HeaderWarning.addWarning(formattedMessage);
4242
}
4343
}
4444

‎server/src/test/java/org/elasticsearch/common/logging/HeaderWarningTests.java‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator;
1111

12-
import org.apache.logging.log4j.Level;
1312
import org.elasticsearch.common.settings.Settings;
1413
import org.elasticsearch.common.util.concurrent.ThreadContext;
1514
import org.elasticsearch.test.ESTestCase;
@@ -191,19 +190,16 @@ public void testFailsWhenRemovingUnknownThreadContext() throws IOException {
191190

192191
public void testWarningValueFromWarningHeader() {
193192
final String s = randomAlphaOfLength(16);
194-
final String first = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, s);
193+
final String first = HeaderWarning.formatWarning(s);
195194
assertThat(HeaderWarning.extractWarningValueFromWarningHeader(first, false), equalTo(s));
196195

197196
final String withPos = "[context][1:11] Blah blah blah";
198-
final String formatted = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, withPos);
197+
final String formatted = HeaderWarning.formatWarning(withPos);
199198
assertThat(HeaderWarning.extractWarningValueFromWarningHeader(formatted, true), equalTo("Blah blah blah"));
200199

201200
final String withNegativePos = "[context][-1:-1] Blah blah blah";
202201
assertThat(
203-
HeaderWarning.extractWarningValueFromWarningHeader(
204-
HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, withNegativePos),
205-
true
206-
),
202+
HeaderWarning.extractWarningValueFromWarningHeader(HeaderWarning.formatWarning(withNegativePos), true),
207203
equalTo("Blah blah blah")
208204
);
209205
}
@@ -289,15 +285,15 @@ public void testAddWarningNonDefaultLogLevel() {
289285
Settings settings = Settings.builder().put("http.max_warning_header_count", maxWarningHeaderCount).build();
290286
ThreadContext threadContext = new ThreadContext(settings);
291287
final Set<ThreadContext> threadContexts = Collections.singleton(threadContext);
292-
HeaderWarning.addWarning(threadContexts, Level.WARN, "A simple message 1");
288+
HeaderWarning.addWarning(threadContexts, "A simple message 1");
293289
final Map<String, List<String>> responseHeaders = threadContext.getResponseHeaders();
294290

295291
assertThat(responseHeaders.size(), equalTo(1));
296292
final List<String> responses = responseHeaders.get("Warning");
297293
assertThat(responses, hasSize(1));
298294
assertThat(responses.get(0), warningValueMatcher);
299295
assertThat(responses.get(0), containsString("\"A simple message 1\""));
300-
assertThat(responses.get(0), containsString(Integer.toString(Level.WARN.intLevel())));
296+
assertThat(responses.get(0), containsString(Integer.toString(299)));
301297
}
302298

303299
}

‎server/src/test/java/org/elasticsearch/common/util/concurrent/ThreadContextTests.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
package org.elasticsearch.common.util.concurrent;
99

1010
import org.elasticsearch.common.io.stream.BytesStreamOutput;
11-
import org.elasticsearch.common.logging.DeprecationLogger;
1211
import org.elasticsearch.common.logging.HeaderWarning;
1312
import org.elasticsearch.common.settings.Settings;
1413
import org.elasticsearch.test.ESTestCase;
@@ -279,11 +278,11 @@ public void testResponseHeaders() {
279278
threadContext.addResponseHeader("foo", "bar");
280279
}
281280

282-
final String value = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "qux");
281+
final String value = HeaderWarning.formatWarning("qux");
283282
threadContext.addResponseHeader("baz", value, s -> HeaderWarning.extractWarningValueFromWarningHeader(s, false));
284283
// pretend that another thread created the same response at a different time
285284
if (randomBoolean()) {
286-
final String duplicateValue = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "qux");
285+
final String duplicateValue = HeaderWarning.formatWarning("qux");
287286
threadContext.addResponseHeader("baz", duplicateValue, s -> HeaderWarning.extractWarningValueFromWarningHeader(s, false));
288287
}
289288

‎test/external-modules/error-query/src/main/java/org/elasticsearch/test/errorquery/ErrorQueryBuilder.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import org.apache.lucene.search.Query;
1313
import org.elasticsearch.common.io.stream.StreamInput;
1414
import org.elasticsearch.common.io.stream.StreamOutput;
15-
import org.elasticsearch.common.logging.DeprecationLogger;
1615
import org.elasticsearch.common.logging.HeaderWarning;
1716
import org.elasticsearch.index.query.AbstractQueryBuilder;
1817
import org.elasticsearch.index.query.SearchExecutionContext;
@@ -82,7 +81,7 @@ protected Query doToQuery(SearchExecutionContext context) throws IOException {
8281
}
8382
final String header = "[" + context.index().getName() + "][" + context.getShardId() + "]";
8483
if (error.getErrorType() == IndexError.ERROR_TYPE.WARNING) {
85-
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, header + " " + error.getMessage());
84+
HeaderWarning.addWarning(header + " " + error.getMessage());
8685
return new MatchAllDocsQuery();
8786
} else {
8887
throw new RuntimeException(header + " " + error.getMessage());

‎test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1684,7 +1684,7 @@ protected static InetAddress randomIp(boolean v4) {
16841684
}
16851685

16861686
public static final class DeprecationWarning {
1687-
private final Level level;
1687+
private final Level level; // Intentionally ignoring level for the sake of equality for now
16881688
private final String message;
16891689

16901690
public DeprecationWarning(Level level, String message) {
@@ -1694,20 +1694,20 @@ public DeprecationWarning(Level level, String message) {
16941694

16951695
@Override
16961696
public int hashCode() {
1697-
return Objects.hash(level, message);
1697+
return Objects.hash(message);
16981698
}
16991699

17001700
@Override
17011701
public boolean equals(Object o) {
17021702
if (this == o) return true;
17031703
if (o == null || getClass() != o.getClass()) return false;
17041704
DeprecationWarning that = (DeprecationWarning) o;
1705-
return Objects.equals(level, that.level) && Objects.equals(message, that.message);
1705+
return Objects.equals(message, that.message);
17061706
}
17071707

17081708
@Override
17091709
public String toString() {
1710-
return String.format(Locale.ROOT, "%s (%s): %s", level.name(), level.intLevel(), message);
1710+
return String.format(Locale.ROOT, "%s: %s", level.name(), message);
17111711
}
17121712
}
17131713
}

‎test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/DoSectionTests.java‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import org.elasticsearch.client.Node;
1414
import org.elasticsearch.client.NodeSelector;
1515
import org.elasticsearch.common.ParsingException;
16-
import org.elasticsearch.common.logging.DeprecationLogger;
1716
import org.elasticsearch.common.logging.HeaderWarning;
1817
import org.elasticsearch.test.rest.yaml.ClientYamlTestExecutionContext;
1918
import org.elasticsearch.test.rest.yaml.ClientYamlTestResponse;
@@ -52,10 +51,10 @@ public void testWarningHeaders() {
5251
section.checkWarningHeaders(emptyList());
5352
}
5453

55-
final String testHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "test");
56-
final String anotherHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "another \"with quotes and \\ backslashes\"");
57-
final String someMoreHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "some more");
58-
final String catHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "cat");
54+
final String testHeader = HeaderWarning.formatWarning("test");
55+
final String anotherHeader = HeaderWarning.formatWarning("another \"with quotes and \\ backslashes\"");
56+
final String someMoreHeader = HeaderWarning.formatWarning("some more");
57+
final String catHeader = HeaderWarning.formatWarning("cat");
5958
// Any warning headers fail
6059
{
6160
final DoSection section = new DoSection(new XContentLocation(1, 1));
@@ -135,17 +134,13 @@ public void testWarningHeaders() {
135134

136135
public void testWarningHeadersRegex() {
137136

138-
final String testHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "test");
137+
final String testHeader = HeaderWarning.formatWarning("test");
139138
final String realisticTestHeader = HeaderWarning.formatWarning(
140-
DeprecationLogger.CRITICAL,
141139
"index template [my-it] has index "
142140
+ "patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template "
143141
+ "[my-it] will take precedence during new index creation"
144142
);
145-
final String testHeaderWithQuotesAndBackslashes = HeaderWarning.formatWarning(
146-
DeprecationLogger.CRITICAL,
147-
"test \"with quotes and \\ backslashes\""
148-
);
143+
final String testHeaderWithQuotesAndBackslashes = HeaderWarning.formatWarning("test \"with quotes and \\ backslashes\"");
149144

150145
// require header and it matches (basic example)
151146
DoSection section = new DoSection(new XContentLocation(1, 1));

0 commit comments

Comments
 (0)