Skip to content

Commit

Permalink
Changing HeaderWarning to always use 299 as the warning code (#80304) (
Browse files Browse the repository at this point in the history
…#80503)

* 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.

* fixing compilation errors from merge
  • Loading branch information
masseyke authored Nov 8, 2021
1 parent c5ca20e commit e4271b6
Show file tree
Hide file tree
Showing 16 changed files with 42 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.CheckedSupplier;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.network.InetAddresses;
import org.elasticsearch.common.network.NetworkAddress;
Expand Down Expand Up @@ -464,7 +463,6 @@ public GeoIpProcessor create(
boolean valid = metadata.isValid(currentState.metadata().settings());
if (valid && metadata.isCloseToExpiration()) {
HeaderWarning.addWarning(
DeprecationLogger.CRITICAL,
"database [{}] was not updated for over 25 days, geoip processor"
+ " will stop working if there is no update for 30 days",
databaseFile
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
"Test put and reset transient settings":
- skip:
version: " - 7.15.99"
version: " - 7.99.99"
reason: "transient settings deprecation"
features: "warnings"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.Setting;
Expand Down Expand Up @@ -1954,7 +1953,7 @@ static void validateDataStreams(SortedMap<String, IndexAbstraction> indicesLooku
// log as debug, this method is executed each time a new cluster state is created and
// could result in many logs:
logger.debug(warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
HeaderWarning.addWarning(warning);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.IndexScopedSettings;
Expand Down Expand Up @@ -560,7 +559,7 @@ public ClusterState addIndexTemplateV2(
name
);
logger.warn(warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
HeaderWarning.addWarning(warning);
}

ComposableIndexTemplate finalIndexTemplate = template;
Expand Down Expand Up @@ -949,7 +948,7 @@ static ClusterState innerPutTemplate(
request.name
);
logger.warn(warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
HeaderWarning.addWarning(warning);
}

templateBuilder.order(request.order);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

package org.elasticsearch.common.logging;

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

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

Expand Down Expand Up @@ -313,21 +310,16 @@ public static String getXOpaqueId() {
.orElse("");
}

public static void addWarning(Level level, String message, Object... params) {
addWarning(THREAD_CONTEXT, level, message, params);
public static void addWarning(String message, Object... params) {
addWarning(THREAD_CONTEXT, message, params);
}

// package scope for testing
static void addWarning(Set<ThreadContext> threadContexts, String message, Object... params) {
addWarning(threadContexts, DeprecationLogger.CRITICAL, message, params);
}

// package scope for testing
static void addWarning(Set<ThreadContext> threadContexts, Level level, String message, Object... params) {
final Iterator<ThreadContext> iterator = threadContexts.iterator();
if (iterator.hasNext()) {
final String formattedMessage = LoggerMessageFormat.format(message, params);
final String warningHeaderValue = formatWarning(level, formattedMessage);
final String warningHeaderValue = formatWarning(formattedMessage);
assert WARNING_HEADER_PATTERN.matcher(warningHeaderValue).matches();
assert extractWarningValueFromWarningHeader(warningHeaderValue, false).equals(escapeAndEncode(formattedMessage));
while (iterator.hasNext()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ public void append(LogEvent event) {
String messagePattern = esLogMessage.getMessagePattern();
Object[] arguments = esLogMessage.getArguments();

HeaderWarning.addWarning(event.getLevel(), messagePattern, arguments);
HeaderWarning.addWarning(messagePattern, arguments);
} else {
final String formattedMessage = event.getMessage().getFormattedMessage();
HeaderWarning.addWarning(event.getLevel(), formattedMessage);
HeaderWarning.addWarning(formattedMessage);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator;

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

public void testWarningValueFromWarningHeader() {
final String s = randomAlphaOfLength(16);
final String first = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, s);
final String first = HeaderWarning.formatWarning(s);
assertThat(HeaderWarning.extractWarningValueFromWarningHeader(first, false), equalTo(s));

final String withPos = "[context][1:11] Blah blah blah";
final String formatted = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, withPos);
final String formatted = HeaderWarning.formatWarning(withPos);
assertThat(HeaderWarning.extractWarningValueFromWarningHeader(formatted, true), equalTo("Blah blah blah"));

final String withNegativePos = "[context][-1:-1] Blah blah blah";
assertThat(
HeaderWarning.extractWarningValueFromWarningHeader(
HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, withNegativePos),
true
),
HeaderWarning.extractWarningValueFromWarningHeader(HeaderWarning.formatWarning(withNegativePos), true),
equalTo("Blah blah blah")
);
}
Expand Down Expand Up @@ -289,15 +285,15 @@ public void testAddWarningNonDefaultLogLevel() {
Settings settings = Settings.builder().put("http.max_warning_header_count", maxWarningHeaderCount).build();
ThreadContext threadContext = new ThreadContext(settings);
final Set<ThreadContext> threadContexts = Collections.singleton(threadContext);
HeaderWarning.addWarning(threadContexts, Level.WARN, "A simple message 1");
HeaderWarning.addWarning(threadContexts, "A simple message 1");
final Map<String, List<String>> responseHeaders = threadContext.getResponseHeaders();

assertThat(responseHeaders.size(), equalTo(1));
final List<String> responses = responseHeaders.get("Warning");
assertThat(responses, hasSize(1));
assertThat(responses.get(0), warningValueMatcher);
assertThat(responses.get(0), containsString("\"A simple message 1\""));
assertThat(responses.get(0), containsString(Integer.toString(Level.WARN.intLevel())));
assertThat(responses.get(0), containsString(Integer.toString(299)));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package org.elasticsearch.common.util.concurrent;

import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -279,11 +278,11 @@ public void testResponseHeaders() {
threadContext.addResponseHeader("foo", "bar");
}

final String value = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "qux");
final String value = HeaderWarning.formatWarning("qux");
threadContext.addResponseHeader("baz", value, s -> HeaderWarning.extractWarningValueFromWarningHeader(s, false));
// pretend that another thread created the same response at a different time
if (randomBoolean()) {
final String duplicateValue = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "qux");
final String duplicateValue = HeaderWarning.formatWarning("qux");
threadContext.addResponseHeader("baz", duplicateValue, s -> HeaderWarning.extractWarningValueFromWarningHeader(s, false));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.apache.lucene.search.Query;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.index.query.AbstractQueryBuilder;
import org.elasticsearch.index.query.SearchExecutionContext;
Expand Down Expand Up @@ -82,7 +81,7 @@ protected Query doToQuery(SearchExecutionContext context) throws IOException {
}
final String header = "[" + context.index().getName() + "][" + context.getShardId() + "]";
if (error.getErrorType() == IndexError.ERROR_TYPE.WARNING) {
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, header + " " + error.getMessage());
HeaderWarning.addWarning(header + " " + error.getMessage());
return new MatchAllDocsQuery();
} else {
throw new RuntimeException(header + " " + error.getMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1740,7 +1740,7 @@ protected static InetAddress randomIp(boolean v4) {
}

public static final class DeprecationWarning {
private final Level level;
private final Level level; // Intentionally ignoring level for the sake of equality for now
private final String message;

public DeprecationWarning(Level level, String message) {
Expand All @@ -1750,20 +1750,20 @@ public DeprecationWarning(Level level, String message) {

@Override
public int hashCode() {
return Objects.hash(level, message);
return Objects.hash(message);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
DeprecationWarning that = (DeprecationWarning) o;
return Objects.equals(level, that.level) && Objects.equals(message, that.message);
return Objects.equals(message, that.message);
}

@Override
public String toString() {
return String.format(Locale.ROOT, "%s (%s): %s", level.name(), level.intLevel(), message);
return String.format(Locale.ROOT, "%s: %s", level.name(), message);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import org.elasticsearch.client.Node;
import org.elasticsearch.client.NodeSelector;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.test.rest.yaml.ClientYamlTestExecutionContext;
import org.elasticsearch.test.rest.yaml.ClientYamlTestResponse;
Expand Down Expand Up @@ -52,10 +51,10 @@ public void testWarningHeaders() {
section.checkWarningHeaders(emptyList(), Version.CURRENT);
}

final String testHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "test");
final String anotherHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "another \"with quotes and \\ backslashes\"");
final String someMoreHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "some more");
final String catHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "cat");
final String testHeader = HeaderWarning.formatWarning("test");
final String anotherHeader = HeaderWarning.formatWarning("another \"with quotes and \\ backslashes\"");
final String someMoreHeader = HeaderWarning.formatWarning("some more");
final String catHeader = HeaderWarning.formatWarning("cat");
// Any warning headers fail
{
final DoSection section = new DoSection(new XContentLocation(1, 1));
Expand Down Expand Up @@ -141,17 +140,13 @@ public void testWarningHeaders() {

public void testWarningHeadersRegex() {

final String testHeader = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "test");
final String testHeader = HeaderWarning.formatWarning("test");
final String realisticTestHeader = HeaderWarning.formatWarning(
DeprecationLogger.CRITICAL,
"index template [my-it] has index "
+ "patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template "
+ "[my-it] will take precedence during new index creation"
);
final String testHeaderWithQuotesAndBackslashes = HeaderWarning.formatWarning(
DeprecationLogger.CRITICAL,
"test \"with quotes and \\ backslashes\""
);
final String testHeaderWithQuotesAndBackslashes = HeaderWarning.formatWarning("test \"with quotes and \\ backslashes\"");

// require header and it matches (basic example)
DoSection section = new DoSection(new XContentLocation(1, 1));
Expand Down Expand Up @@ -224,9 +219,8 @@ public void testWarningHeadersRegex() {
}

public void testIgnoreTypesWarnings() {
String legitimateWarning = HeaderWarning.formatWarning(DeprecationLogger.CRITICAL, "warning");
String legitimateWarning = HeaderWarning.formatWarning("warning");
String typesWarning = HeaderWarning.formatWarning(
DeprecationLogger.CRITICAL,
"[types removal] " + "The endpoint /{index}/{type}/_count is deprecated, use /{index}/_count instead."
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
package org.elasticsearch.license;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.logging.LoggerMessageFormat;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -443,7 +442,7 @@ boolean isAllowed(LicensedFeature feature) {
void checkExpiry() {
String warning = status.expiryWarning;
if (warning != null) {
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
HeaderWarning.addWarning(warning);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.license.LicenseUtils;
Expand Down Expand Up @@ -163,7 +162,7 @@ protected void masterOperation(
String warning = Messages.getMessage(TRAINED_MODEL_INPUTS_DIFFER_SIGNIFICANTLY, request.getModelId(), oldModelId);
auditor.warning(oldModelId, warning);
logger.warn("[{}] {}", oldModelId, warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
HeaderWarning.addWarning(warning);
}
}
clusterService.submitStateUpdateTask("update-model-alias", new AckedClusterStateUpdateTask(request, listener) {
Expand Down
Loading

0 comments on commit e4271b6

Please sign in to comment.