Disallow negative TimeValues (elastic#53913)

This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.

Author: gwbrown

client/rest-high-level/src/test/java/org/elasticsearch/client/ccr/CcrStatsResponseTests.java

@@ -109,7 +109,7 @@ static FollowStatsAction.StatsResponses createStatsResponse() {
                 randomNonNegativeLong(),
                 randomNonNegativeLong(),
                 Collections.emptyNavigableMap(),
-                randomLong(),
+                randomNonNegativeLong(),
                 randomBoolean() ? new ElasticsearchException("fatal error") : null);
             responses.add(new FollowStatsAction.StatsResponse(status));
         }

client/rest-high-level/src/test/java/org/elasticsearch/client/enrich/StatsResponseTests.java

@@ -87,7 +87,7 @@ private static TaskInfo randomTaskInfo() {
         String action = randomAlphaOfLength(5);
         String description = randomAlphaOfLength(5);
         long startTime = randomLong();
-        long runningTimeNanos = randomLong();
+        long runningTimeNanos = randomNonNegativeLong();
         boolean cancellable = randomBoolean();
         TaskId parentTaskId = TaskId.EMPTY_TASK_ID;
         Map<String, String> headers = randomBoolean() ?

client/rest-high-level/src/test/java/org/elasticsearch/client/indexlifecycle/IndexLifecycleExplainResponseTests.java

@@ -57,7 +57,7 @@ private static IndexLifecycleExplainResponse randomManagedIndexExplainResponse()
         boolean stepNull = randomBoolean();
         return IndexLifecycleExplainResponse.newManagedIndexResponse(randomAlphaOfLength(10),
             randomAlphaOfLength(10),
-            randomBoolean() ? null : randomNonNegativeLong(),
+            randomBoolean() ? null : randomLongBetween(0, System.currentTimeMillis()),
             stepNull ? null : randomAlphaOfLength(10),
             stepNull ? null : randomAlphaOfLength(10),
             stepNull ? null : randomAlphaOfLength(10),

libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java

@@ -47,6 +47,9 @@ public TimeValue(long millis) {
     }
 
     public TimeValue(long duration, TimeUnit timeUnit) {
+        if (duration < -1) {
+            throw new IllegalArgumentException("duration cannot be negative, was given [" + duration + "]");
+        }
         this.duration = duration;
         this.timeUnit = timeUnit;
     }
@@ -201,7 +204,7 @@ public double getDaysFrac() {
      * Returns a {@link String} representation of the current {@link TimeValue}.
      *
      * Note that this method might produce fractional time values (ex 1.6m) which cannot be
-     * parsed by method like {@link TimeValue#parse(String, String, String)}.
+     * parsed by method like {@link TimeValue#parse(String, String, String, String)}.
      *
      * Also note that the maximum string value that will be generated is
      * {@code 106751.9d} due to the way that values are internally converted
@@ -216,7 +219,7 @@ public String toString() {
      * Returns a {@link String} representation of the current {@link TimeValue}.
      *
      * Note that this method might produce fractional time values (ex 1.6m) which cannot be
-     * parsed by method like {@link TimeValue#parse(String, String, String)}. The number of
+     * parsed by method like {@link TimeValue#parse(String, String, String, String)}. The number of
      * fractional decimals (up to 10 maximum) are truncated to the number of fraction pieces
      * specified.
      *
@@ -358,20 +361,20 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St
         }
         final String normalized = sValue.toLowerCase(Locale.ROOT).trim();
         if (normalized.endsWith("nanos")) {
-            return new TimeValue(parse(sValue, normalized, "nanos"), TimeUnit.NANOSECONDS);
+            return new TimeValue(parse(sValue, normalized, "nanos", settingName), TimeUnit.NANOSECONDS);
         } else if (normalized.endsWith("micros")) {
-            return new TimeValue(parse(sValue, normalized, "micros"), TimeUnit.MICROSECONDS);
+            return new TimeValue(parse(sValue, normalized, "micros", settingName), TimeUnit.MICROSECONDS);
         } else if (normalized.endsWith("ms")) {
-            return new TimeValue(parse(sValue, normalized, "ms"), TimeUnit.MILLISECONDS);
+            return new TimeValue(parse(sValue, normalized, "ms", settingName), TimeUnit.MILLISECONDS);
         } else if (normalized.endsWith("s")) {
-            return new TimeValue(parse(sValue, normalized, "s"), TimeUnit.SECONDS);
+            return new TimeValue(parse(sValue, normalized, "s", settingName), TimeUnit.SECONDS);
         } else if (sValue.endsWith("m")) {
             // parsing minutes should be case-sensitive as 'M' means "months", not "minutes"; this is the only special case.
-            return new TimeValue(parse(sValue, normalized, "m"), TimeUnit.MINUTES);
+            return new TimeValue(parse(sValue, normalized, "m", settingName), TimeUnit.MINUTES);
         } else if (normalized.endsWith("h")) {
-            return new TimeValue(parse(sValue, normalized, "h"), TimeUnit.HOURS);
+            return new TimeValue(parse(sValue, normalized, "h", settingName), TimeUnit.HOURS);
         } else if (normalized.endsWith("d")) {
-            return new TimeValue(parse(sValue, normalized, "d"), TimeUnit.DAYS);
+            return new TimeValue(parse(sValue, normalized, "d", settingName), TimeUnit.DAYS);
         } else if (normalized.matches("-0*1")) {
             return TimeValue.MINUS_ONE;
         } else if (normalized.matches("0+")) {
@@ -383,10 +386,16 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St
         }
     }
 
-    private static long parse(final String initialInput, final String normalized, final String suffix) {
+    private static long parse(final String initialInput, final String normalized, final String suffix, String settingName) {
         final String s = normalized.substring(0, normalized.length() - suffix.length()).trim();
         try {
-            return Long.parseLong(s);
+            final long value = Long.parseLong(s);
+            if (value < -1) {
+                // -1 is magic, but reject any other negative values
+                throw new IllegalArgumentException("failed to parse setting [" + settingName + "] with value [" + initialInput +
+                    "] as a time value: negative durations are not supported");
+            }
+            return value;
         } catch (final NumberFormatException e) {
             try {
                 @SuppressWarnings("unused") final double ignored = Double.parseDouble(s);

libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java

@@ -238,4 +238,26 @@ public void testConversionHashCode() {
         TimeValue secondValue = new TimeValue(firstValue.getSeconds(), TimeUnit.SECONDS);
         assertEquals(firstValue.hashCode(), secondValue.hashCode());
     }
+
+    public void testRejectsNegativeValuesDuringParsing() {
+        final String settingName = "test-value";
+        final long negativeValue = randomLongBetween(Long.MIN_VALUE, -2);
+        final String negativeTimeValueString = Long.toString(negativeValue) + randomTimeUnit();
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
+            () -> TimeValue.parseTimeValue(negativeTimeValueString, settingName));
+        assertThat(ex.getMessage(),
+            equalTo("failed to parse setting [" + settingName + "] with value [" + negativeTimeValueString +
+                "] as a time value: negative durations are not supported"));
+    }
+
+    public void testRejectsNegativeValuesAtCreation() {
+        final long duration = randomLongBetween(Long.MIN_VALUE, -2);
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new TimeValue(duration, randomTimeUnitObject()));
+        assertThat(ex.getMessage(), containsString("duration cannot be negative"));
+    }
+
+    private TimeUnit randomTimeUnitObject() {
+        return randomFrom(TimeUnit.NANOSECONDS, TimeUnit.MICROSECONDS, TimeUnit.MILLISECONDS, TimeUnit.SECONDS,
+            TimeUnit.MINUTES, TimeUnit.HOURS, TimeUnit.DAYS);
+    }
 }

server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java

@@ -133,7 +133,9 @@ public long getDeleteCount() {
         /**
          * The total amount of time spend on executing delete operations.
          */
-        public TimeValue getDeleteTime() { return new TimeValue(deleteTimeInMillis); }
+        public TimeValue getDeleteTime() {
+            return new TimeValue(deleteTimeInMillis);
+        }
 
         /**
          * Returns the currently in-flight delete operations

server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java

@@ -28,8 +28,8 @@
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.monitor.jvm.JvmStats.GarbageCollector;
-import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.threadpool.Scheduler.Cancellable;
+import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.threadpool.ThreadPool.Names;
 
 import java.util.HashMap;
@@ -165,13 +165,20 @@ public JvmGcMonitorService(Settings settings, ThreadPool threadPool) {
     }
 
     private static TimeValue getValidThreshold(Settings settings, String key, String level) {
-        TimeValue threshold = settings.getAsTime(level, null);
+        final TimeValue threshold;
+
+        try {
+            threshold = settings.getAsTime(level, null);
+        } catch (RuntimeException ex) {
+            final String settingValue = settings.get(level);
+            throw new IllegalArgumentException("failed to parse setting [" + getThresholdName(key, level) + "] with value [" +
+                settingValue + "] as a time value", ex);
+        }
+
         if (threshold == null) {
             throw new IllegalArgumentException("missing gc_threshold for [" + getThresholdName(key, level) + "]");
         }
-        if (threshold.nanos() <= 0) {
-            throw new IllegalArgumentException("invalid gc_threshold [" + threshold + "] for [" + getThresholdName(key, level) + "]");
-        }
+
         return threshold;
     }
 

server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java

@@ -530,7 +530,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa
         if (verbose || endTime != 0) {
             builder.field(END_TIME, DATE_TIME_FORMATTER.format(Instant.ofEpochMilli(endTime).atZone(ZoneOffset.UTC)));
             builder.field(END_TIME_IN_MILLIS, endTime);
-            builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(endTime - startTime));
+            builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(Math.max(0L, endTime - startTime)));
         }
         if (verbose || !shardFailures.isEmpty()) {
             builder.startArray(FAILURES);

server/src/test/java/org/elasticsearch/action/search/SearchResponseMergerTests.java

@@ -94,12 +94,12 @@ private void awaitResponsesAdded() throws InterruptedException {
     }
 
     public void testMergeTookInMillis() throws InterruptedException {
-        long currentRelativeTime = randomLong();
+        long currentRelativeTime = randomNonNegativeLong();
         SearchTimeProvider timeProvider = new SearchTimeProvider(randomLong(), 0, () -> currentRelativeTime);
         SearchResponseMerger merger = new SearchResponseMerger(randomIntBetween(0, 1000), randomIntBetween(0, 10000),
             SearchContext.TRACK_TOTAL_HITS_ACCURATE, timeProvider, emptyReduceContextBuilder());
         for (int i = 0; i < numResponses; i++) {
-            SearchResponse searchResponse = new SearchResponse(InternalSearchResponse.empty(), null, 1, 1, 0, randomLong(),
+            SearchResponse searchResponse = new SearchResponse(InternalSearchResponse.empty(), null, 1, 1, 0, randomNonNegativeLong(),
                 ShardSearchFailure.EMPTY_ARRAY, SearchResponseTests.randomClusters());
             addResponse(merger, searchResponse);
         }
@@ -399,7 +399,7 @@ public void testMergeAggs() throws InterruptedException {
     }
 
     public void testMergeSearchHits() throws InterruptedException {
-        final long currentRelativeTime = randomLong();
+        final long currentRelativeTime = randomNonNegativeLong();
         final SearchTimeProvider timeProvider = new SearchTimeProvider(randomLong(), 0, () -> currentRelativeTime);
         final int size = randomIntBetween(0, 100);
         final int from = size > 0 ? randomIntBetween(0, 100) : 0;
@@ -557,7 +557,7 @@ public void testMergeSearchHits() throws InterruptedException {
     }
 
     public void testMergeNoResponsesAdded() {
-        long currentRelativeTime = randomLong();
+        long currentRelativeTime = randomNonNegativeLong();
         final SearchTimeProvider timeProvider = new SearchTimeProvider(randomLong(), 0, () -> currentRelativeTime);
         SearchResponseMerger merger = new SearchResponseMerger(0, 10, Integer.MAX_VALUE, timeProvider, emptyReduceContextBuilder());
         SearchResponse.Clusters clusters = SearchResponseTests.randomClusters();

server/src/test/java/org/elasticsearch/common/RoundingTests.java

@@ -129,8 +129,8 @@ public void testDayRounding() {
         Rounding tzRounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH)
             .timeZone(ZoneOffset.ofHours(timezoneOffset)).build();
         assertThat(tzRounding.round(0), equalTo(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()));
-        assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(0L - TimeValue
-            .timeValueHours(timezoneOffset).millis()));
+        assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(TimeValue
+            .timeValueHours(-timezoneOffset).millis()));
 
         ZoneId tz = ZoneId.of("-08:00");
         tzRounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH).timeZone(tz).build();

server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java

@@ -128,8 +128,8 @@ public void testDayRounding() {
         Rounding tzRounding = Rounding.builder(DateTimeUnit.DAY_OF_MONTH).timeZone(DateTimeZone.forOffsetHours(timezoneOffset))
                 .build();
         assertThat(tzRounding.round(0), equalTo(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()));
-        assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(0L - TimeValue
-                .timeValueHours(timezoneOffset).millis()));
+        assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(TimeValue
+                .timeValueHours(-timezoneOffset).millis()));
 
         DateTimeZone tz = DateTimeZone.forID("-08:00");
         tzRounding = Rounding.builder(DateTimeUnit.DAY_OF_MONTH).timeZone(tz).build();

server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java

@@ -253,15 +253,17 @@ public void testNegativeInterval() {
         Exception e = expectThrows(IllegalArgumentException.class,
                                    () -> new MockController(Settings.builder()
                                                             .put("indices.memory.interval", "-42s").build()));
-        assertEquals("failed to parse value [-42s] for setting [indices.memory.interval], must be >= [0ms]", e.getMessage());
+        assertEquals("failed to parse setting [indices.memory.interval] with value " +
+            "[-42s] as a time value: negative durations are not supported", e.getMessage());
 
     }
 
     public void testNegativeShardInactiveTime() {
         Exception e = expectThrows(IllegalArgumentException.class,
                                    () -> new MockController(Settings.builder()
                                                             .put("indices.memory.shard_inactive_time", "-42s").build()));
-        assertEquals("failed to parse value [-42s] for setting [indices.memory.shard_inactive_time], must be >= [0ms]", e.getMessage());
+        assertEquals("failed to parse setting [indices.memory.shard_inactive_time] with value " +
+            "[-42s] as a time value: negative durations are not supported", e.getMessage());
 
     }
 

server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java

@@ -33,8 +33,8 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 
-import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.instanceOf;
 
 public class JvmGcMonitorServiceSettingsTests extends ESTestCase {
@@ -62,11 +62,12 @@ public void testDisabledSetting() throws InterruptedException {
 
     public void testNegativeSetting() throws InterruptedException {
         String collector = randomAlphaOfLength(5);
-        Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", "-" + randomTimeValue()).build();
+        final String timeValue = "-" + randomTimeValue();
+        Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", timeValue).build();
         execute(settings, (command, interval, name) -> null, e -> {
             assertThat(e, instanceOf(IllegalArgumentException.class));
-            assertThat(e.getMessage(), allOf(containsString("invalid gc_threshold"),
-                containsString("for [monitor.jvm.gc.collector." + collector + ".")));
+            assertThat(e.getMessage(), equalTo("failed to parse setting [monitor.jvm.gc.collector." + collector + ".warn] " +
+                "with value [" + timeValue + "] as a time value"));
         }, true, null);
     }
 

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java

@@ -61,7 +61,7 @@ protected ShardFollowNodeTaskStatus createTestInstance() {
                 randomNonNegativeLong(),
                 randomNonNegativeLong(),
                 randomReadExceptions(),
-                randomLong(),
+                randomNonNegativeLong(),
                 randomBoolean() ? new ElasticsearchException("fatal error") : null);
     }
 

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java

@@ -59,7 +59,7 @@ static FollowStatsAction.StatsResponses createStatsResponse() {
                 randomNonNegativeLong(),
                 randomNonNegativeLong(),
                 Collections.emptyNavigableMap(),
-                randomLong(),
+                randomNonNegativeLong(),
                 randomBoolean() ? new ElasticsearchException("fatal error") : null);
             responses.add(new FollowStatsAction.StatsResponse(status));
         }