Skip to content

Commit

Permalink
Exposing the ability to log deprecated settings at non-critical level(#…
Browse files Browse the repository at this point in the history
…79107) (#79492)

A recent change for the deprecation logs provided the capability to emit deprecation's at critical vs. warning levels, #77482.
However deprecated settings always log at critical level without the ability to express that the setting deprecation is only a
warning.

This commit exposes the ability to set the deprecation level when deprecating a setting.
Relates #78781
  • Loading branch information
masseyke authored Oct 19, 2021
1 parent f58a31b commit 1f0e85b
Show file tree
Hide file tree
Showing 30 changed files with 323 additions and 127 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,14 @@ public HttpEntity getEntity() {

/**
* Tests if a string matches the RFC 7234 specification for warning headers.
* This assumes that the warn code is always 299 and the warn agent is always
* Elasticsearch.
* This assumes that the warn code is always 299 or 300 and the warn agent is
* always Elasticsearch.
*
* @param s the value of a warning header formatted according to RFC 7234
* @return {@code true} if the input string matches the specification
*/
private static boolean matchWarningHeaderPatternByPrefix(final String s) {
return s.startsWith("299 Elasticsearch-");
return s.startsWith("299 Elasticsearch-") || s.startsWith("300 Elasticsearch-");
}

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

import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.xcontent.XContentParserUtils;
import org.elasticsearch.common.Strings;
import org.elasticsearch.core.CheckedFunction;
Expand Down Expand Up @@ -213,7 +214,8 @@ class TestStruct {
objectParser.declareField((i, v, c) -> v.test = i.text(), new ParseField("test", "old_test"), ObjectParser.ValueType.STRING);
objectParser.parse(parser, s, null);
assertEquals("foo", s.test);
assertWarnings(false, "[foo][1:15] Deprecated field [old_test] used, expected [test] instead");
assertWarnings(false, new DeprecationWarning(DeprecationLogger.CRITICAL, "[foo][1:15] Deprecated field [old_test] used, " +
"expected [test] instead"));
}

public void testFailOnValueType() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
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 @@ -447,8 +448,8 @@ public GeoIpProcessor create(

boolean valid = metadata.isValid(currentState.metadata().settings());
if (valid && metadata.isCloseToExpiration()) {
HeaderWarning.addWarning("database [{}] was not updated for over 25 days, geoip processor will stop working if there " +
"is no update for 30 days", databaseFile);
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);
}

return valid;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
import com.amazonaws.auth.AWSCredentialsProvider;
import com.amazonaws.auth.BasicSessionCredentials;
import com.amazonaws.auth.DefaultAWSCredentialsProviderChain;

import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -59,8 +62,9 @@ public void testDeprecationOfLoneAccessKey() {
Ec2ClientSettings.getClientSettings(Settings.builder().setSecureSettings(secureSettings).build())).getCredentials();
assertThat(credentials.getAWSAccessKeyId(), is("aws_key"));
assertThat(credentials.getAWSSecretKey(), is(""));
assertSettingDeprecationsAndWarnings(new String[]{},
"Setting [discovery.ec2.access_key] is set but [discovery.ec2.secret_key] is not, which will be unsupported in future");
assertSettingDeprecationsAndWarnings(new Setting<?>[]{},
new DeprecationWarning(DeprecationLogger.CRITICAL, "Setting [discovery.ec2.access_key] is set but " +
"[discovery.ec2.secret_key] is not, which will be unsupported in future"));
}

public void testDeprecationOfLoneSecretKey() {
Expand All @@ -70,8 +74,9 @@ public void testDeprecationOfLoneSecretKey() {
Ec2ClientSettings.getClientSettings(Settings.builder().setSecureSettings(secureSettings).build())).getCredentials();
assertThat(credentials.getAWSAccessKeyId(), is(""));
assertThat(credentials.getAWSSecretKey(), is("aws_secret"));
assertSettingDeprecationsAndWarnings(new String[]{},
"Setting [discovery.ec2.secret_key] is set but [discovery.ec2.access_key] is not, which will be unsupported in future");
assertSettingDeprecationsAndWarnings(new Setting<?>[]{},
new DeprecationWarning(DeprecationLogger.CRITICAL, "Setting [discovery.ec2.secret_key] is set but " +
"[discovery.ec2.access_key] is not, which will be unsupported in future"));
}

public void testRejectionOfLoneSessionToken() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void testDeprecationWarnMessage() throws IOException {
);
}

assertWarnings("deprecated warn message1");
assertWarnings(true, new DeprecationWarning(Level.WARN, "deprecated warn message1")) ;
}

public void testDeprecatedMessageWithoutXOpaqueId() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
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 @@ -1785,7 +1786,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(warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateUpdateTask;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -490,7 +491,7 @@ public ClusterState addIndexTemplateV2(final ClusterState currentState, final bo
.collect(Collectors.joining(",")),
name);
logger.warn(warning);
HeaderWarning.addWarning(warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
}

ComposableIndexTemplate finalIndexTemplate = template;
Expand Down Expand Up @@ -816,7 +817,7 @@ static ClusterState innerPutTemplate(final ClusterState currentState, PutRequest
.collect(Collectors.joining(",")),
request.name);
logger.warn(warning);
HeaderWarning.addWarning(warning);
HeaderWarning.addWarning(DeprecationLogger.CRITICAL, warning);
}

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

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 @@ -32,10 +33,11 @@
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. Further, this pattern assumes that the warn agent represents a version of Elasticsearch including the build hash.
* is always 299 or 300. 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 " + // warn code
"(?:299|300) " + // 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 @@ -53,15 +55,16 @@ 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. 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.
* 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.
*/
private static final String WARNING_PREFIX =
String.format(
Locale.ROOT,
"299 Elasticsearch-%s%s-%s",
" Elasticsearch-%s%s-%s",
Version.CURRENT.toString(),
Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : "",
Build.CURRENT.hash());
Expand Down Expand Up @@ -189,14 +192,15 @@ 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 String s) {
public static String formatWarning(final Level level, final String s) {
// Assume that the common scenario won't have a string to escape and encode.
int length = WARNING_PREFIX.length() + s.length() + 3;
int length = WARNING_PREFIX.length() + s.length() + 6;
final StringBuilder sb = new StringBuilder(length);
sb.append(WARNING_PREFIX).append(" \"").append(escapeAndEncode(s)).append("\"");
sb.append(level.intLevel() + WARNING_PREFIX).append(" \"").append(escapeAndEncode(s)).append("\"");
return sb.toString();
}

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

public static void addWarning(String message, Object... params) {
addWarning(THREAD_CONTEXT, message, params);
public static void addWarning(Level level, String message, Object... params) {
addWarning(THREAD_CONTEXT, level, 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(formattedMessage);
final String warningHeaderValue = formatWarning(level, formattedMessage);
assert WARNING_HEADER_PATTERN.matcher(warningHeaderValue).matches();
assert extractWarningValueFromWarningHeader(warningHeaderValue, false)
.equals(escapeAndEncode(formattedMessage));
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(messagePattern, arguments);
HeaderWarning.addWarning(event.getLevel(), messagePattern, arguments);
} else {
final String formattedMessage = event.getMessage().getFormattedMessage();
HeaderWarning.addWarning(formattedMessage);
HeaderWarning.addWarning(event.getLevel(), formattedMessage);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,15 @@ public enum Property {
Final,

/**
* mark this setting as deprecated
* mark this setting as deprecated (critical level)
*/
Deprecated,

/**
* mark this setting as deprecated (warning level)
*/
DeprecatedWarning,

/**
* Node scope
*/
Expand Down Expand Up @@ -170,6 +175,9 @@ private Setting(Key key, @Nullable Setting<T> fallbackSetting, Function<Settings
if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.OperatorDynamic)) {
throw new IllegalArgumentException("setting [" + key + "] cannot be both dynamic and operator dynamic");
}
if (propertiesAsSet.contains(Property.Deprecated) && propertiesAsSet.contains(Property.DeprecatedWarning)) {
throw new IllegalArgumentException("setting [" + key + "] cannot be deprecated at both critical and warning levels");
}
checkPropertyRequiresIndexScope(propertiesAsSet, Property.NotCopyableOnResize);
checkPropertyRequiresIndexScope(propertiesAsSet, Property.InternalIndex);
checkPropertyRequiresIndexScope(propertiesAsSet, Property.PrivateIndex);
Expand Down Expand Up @@ -382,7 +390,11 @@ public boolean hasIndexScope() {
* Returns <code>true</code> if this setting is deprecated, otherwise <code>false</code>
*/
public boolean isDeprecated() {
return properties.contains(Property.Deprecated);
return properties.contains(Property.Deprecated) || properties.contains(Property.DeprecatedWarning);
}

private boolean isDeprecatedWarningOnly() {
return properties.contains(Property.DeprecatedWarning);
}

/**
Expand Down Expand Up @@ -561,13 +573,16 @@ void checkDeprecation(Settings settings) {
if (this.isDeprecated() && this.exists(settings)) {
// It would be convenient to show its replacement key, but replacement is often not so simple
final String key = getKey();

DeprecationCategory category = this.isSecure(settings) ? DeprecationCategory.SECURITY : DeprecationCategory.SETTINGS;
List<String> skipTheseDeprecations = settings.getAsList("deprecation.skip_deprecated_settings");
if (Regex.simpleMatch(skipTheseDeprecations, key) == false) {
Settings.DeprecationLoggerHolder.deprecationLogger
.critical(category, key, "[{}] setting was deprecated in Elasticsearch and will be removed in a future release! "
+ "See the breaking changes documentation for the next major version.", key);
String message = "[{}] setting was deprecated in Elasticsearch and will be removed in a future release! "
+ "See the breaking changes documentation for the next major version.";
if (this.isDeprecatedWarningOnly()) {
Settings.DeprecationLoggerHolder.deprecationLogger.warn(category, key, message, key);
} else {
Settings.DeprecationLoggerHolder.deprecationLogger.critical(category, key, message, key);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
package org.elasticsearch.common.logging;

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 @@ -190,16 +192,16 @@ public void testFailsWhenRemovingUnknownThreadContext() throws IOException {

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

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

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

public void testEscapeBackslashesAndQuotes() {
Expand Down Expand Up @@ -285,4 +287,22 @@ private String range(int lowerInclusive, int upperInclusive) {
.toString();
}

public void testAddWarningNonDefaultLogLevel() {
final int maxWarningHeaderCount = 2;
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");
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())));
}

}
Loading

0 comments on commit 1f0e85b

Please sign in to comment.