Merge branch 'master' into enhancement/node_client_setting_removal

Original commit: elastic/x-pack-elasticsearch@4276ae3192

elasticsearch/x-pack/build.gradle

@@ -1,6 +1,8 @@
 import org.elasticsearch.gradle.MavenFilteringHack
 import org.elasticsearch.gradle.test.NodeInfo
 
+group 'org.elasticsearch.plugin'
+
 apply plugin: 'elasticsearch.esplugin'
 esplugin {
   name 'xpack'

elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/MarvelSettings.java

@@ -32,6 +32,10 @@ public class MarvelSettings extends AbstractComponent {
     public static final String LEGACY_DATA_INDEX_NAME = ".marvel-es-data";
 
     public static final String HISTORY_DURATION_SETTING_NAME = "history.duration";
+    /**
+     * The minimum amount of time allowed for the history duration.
+     */
+    public static final TimeValue HISTORY_DURATION_MINIMUM = TimeValue.timeValueHours(24);
     public static final TimeValue MAX_LICENSE_GRACE_PERIOD = TimeValue.timeValueHours(7 * 24);
 
     /**
@@ -101,10 +105,21 @@ public class MarvelSettings extends AbstractComponent {
             listSetting(key("agent.collectors"), Collections.emptyList(), Function.identity(), Property.NodeScope);
 
     /**
-     * The default retention duration of the monitoring history data
+     * The default retention duration of the monitoring history data.
+     * <p>
+     * Expected values:
+     * <ul>
+     * <li>Default: 7 days</li>
+     * <li>Minimum: 1 day</li>
+     * </ul>
+     *
+     * @see #HISTORY_DURATION_MINIMUM
      */
     public static final Setting<TimeValue> HISTORY_DURATION =
-            timeSetting(key(HISTORY_DURATION_SETTING_NAME), TimeValue.timeValueHours(7 * 24), Property.Dynamic, Property.NodeScope);
+            timeSetting(key(HISTORY_DURATION_SETTING_NAME),
+                        TimeValue.timeValueHours(7 * 24), // default value (7 days)
+                        HISTORY_DURATION_MINIMUM,         // minimum value
+                        Property.Dynamic, Property.NodeScope);
 
     /**
      * The index setting that holds the template version
@@ -221,7 +236,13 @@ public class MarvelSettings extends AbstractComponent {
         this.indices = indices.toArray(new String[0]);
     }
 
-    private static String key(String key) {
+    /**
+     * Prefix the {@code key} with the Monitoring prefix.
+     *
+     * @param key The key to prefix
+     * @return The key prefixed by the product prefixes.
+     */
+    static String key(String key) {
         return XPackPlugin.featureSettingPrefix(Marvel.NAME) + "." + key;
     }
 

elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/local/LocalExporter.java

@@ -254,19 +254,6 @@ public class LocalExporter extends Exporter implements ClusterStateListener, Cle
         }
 
         if (clusterService.localNode().masterNode()) {
-
-            // Retention duration can be overridden at exporter level
-            TimeValue exporterRetention = config.settings().getAsTime(MarvelSettings.HISTORY_DURATION_SETTING_NAME, null);
-            if (exporterRetention != null) {
-                try {
-                    cleanerService.validateRetention(exporterRetention);
-                    retention = exporterRetention;
-                } catch (IllegalArgumentException e) {
-                    logger.warn("local exporter [{}] - unable to use custom history duration [{}]: {}", name(), exporterRetention,
-                            e.getMessage());
-                }
-            }
-
             // Reference date time will be compared to index.creation_date settings,
             // that's why it must be in UTC
             DateTime expiration = new DateTime(DateTimeZone.UTC).minus(retention.millis());

elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/cleaner/CleanerService.java

@@ -10,7 +10,7 @@ import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.ClusterSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.common.util.concurrent.AbstractRunnable;
+import org.elasticsearch.common.util.concurrent.AbstractLifecycleRunnable;
 import org.elasticsearch.common.util.concurrent.FutureUtils;
 import org.elasticsearch.marvel.MarvelSettings;
 import org.elasticsearch.marvel.license.MarvelLicensee;
@@ -23,18 +23,17 @@ import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ScheduledFuture;
 
 /**
- * CleanerService takes care of deleting old monitoring indices.
+ * {@code CleanerService} takes care of deleting old monitoring indices.
  */
 public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
 
-
     private final MarvelLicensee licensee;
     private final ThreadPool threadPool;
     private final ExecutionScheduler executionScheduler;
     private final List<Listener> listeners = new CopyOnWriteArrayList<>();
 
     private volatile IndicesCleaner runnable;
-    private volatile TimeValue retention;
+    private volatile TimeValue globalRetention;
 
     CleanerService(Settings settings, ClusterSettings clusterSettings, MarvelLicensee licensee, ThreadPool threadPool,
                    ExecutionScheduler executionScheduler) {
@@ -42,7 +41,10 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         this.licensee = licensee;
         this.threadPool = threadPool;
         this.executionScheduler = executionScheduler;
-        clusterSettings.addSettingsUpdateConsumer(MarvelSettings.HISTORY_DURATION, this::setRetention, this::validateRetention);
+        this.globalRetention = MarvelSettings.HISTORY_DURATION.get(settings);
+
+        // the validation is performed by the setting's object itself
+        clusterSettings.addSettingsUpdateConsumer(MarvelSettings.HISTORY_DURATION, this::setGlobalRetention);
     }
 
     @Inject
@@ -77,32 +79,56 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         return ThreadPool.Names.GENERIC;
     }
 
-    TimeValue getRetention() {
+    /**
-        return retention;
+     * Get the retention that can be used.
-    }
+     * <p>
-
+     * This will ignore the global retention if the license does not allow retention updates.
-    public void setRetention(TimeValue retention) {
+     *
-        validateRetention(retention);
+     * @return Never {@code null}
-        this.retention = retention;
+     * @see MarvelLicensee#allowUpdateRetention()
-    }
+     */
-
+    public TimeValue getRetention() {
-    public void validateRetention(TimeValue retention) {
+        // we only care about their value if they are allowed to set it
-        if (retention == null) {
+        if (licensee.allowUpdateRetention() && globalRetention != null) {
-            throw new IllegalArgumentException("history duration setting cannot be null");
+            return globalRetention;
         }
-        if ((retention.getMillis() <= 0) && (retention.getMillis() != -1)) {
+        else {
-            throw new IllegalArgumentException("invalid history duration setting value");
+            return MarvelSettings.HISTORY_DURATION.getDefault(Settings.EMPTY);
-        }
-        if (!licensee.allowUpdateRetention()) {
-            throw new IllegalArgumentException("license does not allow the history duration setting to be updated to value ["
-                    + retention + "]");
         }
     }
 
+    /**
+     * Set the global retention. This is expected to be used by the cluster settings to dynamically control the global retention time.
+     * <p>
+     * Even if the current license prevents retention updates, it will accept the change so that they do not need to re-set it if they
+     * upgrade their license (they can always unset it).
+     *
+     * @param globalRetention The global retention to use dynamically.
+     */
+    public void setGlobalRetention(TimeValue globalRetention) {
+        // notify the user that their setting will be ignored until they get the right license
+        if (licensee.allowUpdateRetention() == false) {
+            logger.warn("[{}] setting will be ignored until an appropriate license is applied", MarvelSettings.HISTORY_DURATION.getKey());
+        }
+
+        this.globalRetention = globalRetention;
+    }
+
+    /**
+     * Add a {@code listener} that is executed by the internal {@code IndicesCleaner} given the {@link #getRetention() retention} time.
+     *
+     * @param listener A listener used to control retention
+     */
     public void add(Listener listener) {
         listeners.add(listener);
     }
 
+    /**
+     * Remove a {@code listener}.
+     *
+     * @param listener A listener used to control retention
+     * @see #add(Listener)
+     */
     public void remove(Listener listener) {
         listeners.remove(listener);
     }
@@ -121,49 +147,56 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         void onCleanUpIndices(TimeValue retention);
     }
 
-    class IndicesCleaner extends AbstractRunnable {
+    /**
+     * {@code IndicesCleaner} runs and reschedules itself in order to automatically clean (delete) indices that are outside of the
+     * {@link #getRetention() retention} period.
+     */
+    class IndicesCleaner extends AbstractLifecycleRunnable {
 
         private volatile ScheduledFuture<?> future;
 
+        /**
+         * Enable automatic logging and stopping of the runnable based on the {@link #lifecycle}.
+         */
+        public IndicesCleaner() {
+            super(lifecycle, logger);
+        }
+
         @Override
-        protected void doRun() throws Exception {
+        protected void doRunInLifecycle() throws Exception {
-            if (lifecycle.stoppedOrClosed()) {
+            if (licensee.cleaningEnabled() == false) {
-                logger.trace("cleaning service is stopping, exiting");
-                return;
-            }
-            if (!licensee.cleaningEnabled()) {
                 logger.debug("cleaning service is disabled due to invalid license");
                 return;
             }
 
-            TimeValue globalRetention = retention;
+            // fetch the retention, which is depends on a bunch of rules
-            if (globalRetention == null) {
+            TimeValue retention = getRetention();
+
+            logger.trace("cleaning up indices with retention [{}]", retention);
+
+            // Note: listeners are free to override the retention
+            for (Listener listener : listeners) {
                 try {
-                    globalRetention = MarvelSettings.HISTORY_DURATION.get(settings);
+                    listener.onCleanUpIndices(retention);
-                    validateRetention(globalRetention);
+                } catch (Throwable t) {
-                } catch (IllegalArgumentException e) {
+                    logger.error("listener failed to clean indices", t);
-                    globalRetention = MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY);
                 }
             }
 
+            logger.trace("done cleaning up indices");
+        }
+
+        /**
+         * Reschedule the cleaner if the service is not stopped.
+         */
+        @Override
+        protected void onAfterInLifecycle() {
             DateTime start = new DateTime(ISOChronology.getInstance());
-            if (globalRetention.millis() > 0) {
+            TimeValue delay = executionScheduler.nextExecutionDelay(start);
-                logger.trace("cleaning up indices with retention [{}]", globalRetention);
 
-                for (Listener listener : listeners) {
+            logger.debug("scheduling next execution in [{}] seconds", delay.seconds());
-                    try {
-                        listener.onCleanUpIndices(globalRetention);
-                    } catch (Throwable t) {
-                        logger.error("listener failed to clean indices", t);
-                    }
-                }
-            }
 
-            if (!lifecycle.stoppedOrClosed()) {
+            future = threadPool.schedule(delay, executorName(), this);
-                TimeValue delay = executionScheduler.nextExecutionDelay(start);
-                logger.debug("scheduling next execution in [{}] seconds", delay.seconds());
-                future = threadPool.schedule(delay, executorName(), this);
-            }
         }
 
         @Override
@@ -171,6 +204,13 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
             logger.error("failed to clean indices", t);
         }
 
+        /**
+         * Cancel/stop the cleaning service.
+         * <p>
+         * This will kill any scheduled {@link #future} from running. It's possible that this will be executed concurrently with the
+         * {@link #onAfter() rescheduling code}, at which point it will be stopped during the next execution <em>if</em> the service is
+         * stopped.
+         */
         public void cancel() {
             FutureUtils.cancel(future);
         }
@@ -179,8 +219,7 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
     interface ExecutionScheduler {
 
         /**
-         * Calculates the delay in millis between "now" and
+         * Calculates the delay in millis between "now" and the next execution.
-         * the next execution.
          *
          * @param now the current time
          * @return the delay in millis
@@ -189,7 +228,7 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
     }
 
     /**
-     * Schedule task so that it will be executed everyday at 01:00 AM
+     * Schedule task so that it will be executed everyday at the next 01:00 AM.
      */
     static class DefaultExecutionScheduler implements ExecutionScheduler {
 
@@ -197,7 +236,8 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         public TimeValue nextExecutionDelay(DateTime now) {
             // Runs at 01:00 AM today or the next day if it's too late
             DateTime next = now.withTimeAtStartOfDay().plusHours(1);
-            if (next.isBefore(now) || next.equals(now)) {
+            // if it's not after now, then it needs to be the next day!
+            if (next.isAfter(now) == false) {
                 next = next.plusDays(1);
             }
             return TimeValue.timeValueMillis(next.getMillis() - now.getMillis());

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/MarvelSettingsTests.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.marvel;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.test.ESTestCase;
+
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;
+
+/**
+ * Tests {@link MarvelSettings}
+ */
+public class MarvelSettingsTests extends ESTestCase {
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
+    public void testHistoryDurationDefaults7Days() {
+        TimeValue sevenDays = TimeValue.timeValueHours(7 * 24);
+
+        // 7 days
+        assertEquals(sevenDays, MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY));
+        // Note: this verifies the semantics because this is taken for granted that it never returns null!
+        assertEquals(sevenDays, MarvelSettings.HISTORY_DURATION.get(buildSettings(MarvelSettings.HISTORY_DURATION.getKey(), null)));
+    }
+
+    public void testHistoryDurationMinimum24Hours() {
+        // hit the minimum
+        assertEquals(MarvelSettings.HISTORY_DURATION_MINIMUM,
+                     MarvelSettings.HISTORY_DURATION.get(buildSettings(MarvelSettings.HISTORY_DURATION.getKey(), "24h")));
+    }
+
+    public void testHistoryDurationMinimum24HoursBlocksLower() {
+        expectedException.expect(IllegalArgumentException.class);
+
+        // 1 ms early!
+        String oneSecondEarly = (MarvelSettings.HISTORY_DURATION_MINIMUM.millis() - 1) + "ms";
+
+        MarvelSettings.HISTORY_DURATION.get(buildSettings(MarvelSettings.HISTORY_DURATION.getKey(), oneSecondEarly));
+    }
+
+    private Settings buildSettings(String key, String value) {
+        return Settings.builder().put(key, value).build();
+    }
+}

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/AbstractIndicesCleanerTestCase.java

@@ -7,9 +7,6 @@ package org.elasticsearch.marvel.cleaner;
 
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.license.core.License;
-import org.elasticsearch.license.plugin.core.LicenseState;
-import org.elasticsearch.license.plugin.core.Licensee;
 import org.elasticsearch.marvel.MarvelSettings;
 import org.elasticsearch.marvel.MonitoredSystem;
 import org.elasticsearch.marvel.agent.exporter.Exporter;
@@ -17,7 +14,6 @@ import org.elasticsearch.marvel.agent.exporter.Exporters;
 import org.elasticsearch.marvel.agent.exporter.MarvelTemplateUtils;
 import org.elasticsearch.marvel.agent.exporter.MonitoringDoc;
 import org.elasticsearch.marvel.agent.resolver.MonitoringIndexNameResolver;
-import org.elasticsearch.marvel.license.MarvelLicensee;
 import org.elasticsearch.marvel.test.MarvelIntegTestCase;
 import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
 import org.elasticsearch.test.VersionUtils;
@@ -27,8 +23,6 @@ import org.joda.time.DateTimeZone;
 import java.util.Locale;
 
 import static org.elasticsearch.test.ESIntegTestCase.Scope.TEST;
-import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
-import static org.hamcrest.Matchers.lessThanOrEqualTo;
 
 @ClusterScope(scope = TEST, numDataNodes = 0, numClientNodes = 0, transportClientRatio = 0.0)
 public abstract class AbstractIndicesCleanerTestCase extends MarvelIntegTestCase {
@@ -37,8 +31,7 @@ public abstract class AbstractIndicesCleanerTestCase extends MarvelIntegTestCase
     protected Settings nodeSettings(int nodeOrdinal) {
         Settings.Builder settings = Settings.builder()
                 .put(super.nodeSettings(nodeOrdinal))
-                .put(MarvelSettings.INTERVAL.getKey(), "-1")
+                .put(MarvelSettings.INTERVAL.getKey(), "-1");
-                .put(MarvelSettings.HISTORY_DURATION.getKey(), "-1");
         return settings.build();
     }
 
@@ -165,46 +158,6 @@ public abstract class AbstractIndicesCleanerTestCase extends MarvelIntegTestCase
         assertIndicesCount(retention);
     }
 
-    public void testRetentionAsExporterSetting() throws Exception {
-        final int max = 10;
-
-        // Default retention is between 3 and max days
-        final int defaultRetention = randomIntBetween(3, max);
-        internalCluster().startNode(Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(),
-                String.format(Locale.ROOT, "%dd", defaultRetention)));
-
-        final DateTime now = now();
-        for (int i = 0; i < max; i++) {
-            createTimestampedIndex(MarvelTemplateUtils.TEMPLATE_VERSION, now.minusDays(i));
-        }
-        assertIndicesCount(max);
-
-        // Exporter retention is between 0 and the default retention
-        final int exporterRetention = randomIntBetween(1, defaultRetention);
-        assertThat(exporterRetention, lessThanOrEqualTo(defaultRetention));
-
-        // Updates the retention setting for the exporter
-        Exporters exporters = internalCluster().getInstance(Exporters.class);
-        for (Exporter exporter : exporters) {
-            Settings transientSettings = Settings.builder().put("xpack.monitoring.agent.exporters." + exporter.name() + "." +
-                    MarvelSettings.HISTORY_DURATION_SETTING_NAME, String.format(Locale.ROOT, "%dd", exporterRetention)).build();
-            assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(transientSettings));
-        }
-
-        // Move to GOLD license
-        for (MarvelLicensee licensee : internalCluster().getInstances(MarvelLicensee.class)) {
-            licensee.onChange(new Licensee.Status(License.OperationMode.GOLD, LicenseState.ENABLED));
-        }
-
-        // Try to clean indices using the global setting
-        CleanerService.Listener listener = getListener();
-        listener.onCleanUpIndices(days(defaultRetention));
-
-        // Checks that indices have been deleted according to
-        // the retention configured at exporter level
-        assertIndicesCount(exporterRetention);
-    }
-
     protected CleanerService.Listener getListener() {
         Exporters exporters = internalCluster().getInstance(Exporters.class);
         for (Exporter exporter : exporters) {

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/CleanerServiceTests.java

@@ -16,26 +16,30 @@ import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;
 
 import java.util.Collections;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
-import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 public class CleanerServiceTests extends ESTestCase {
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
 
+    private final MarvelLicensee licensee = mock(MarvelLicensee.class);
     private ClusterSettings clusterSettings;
-    private TimeValue defaultRetention;
     private ThreadPool threadPool;
 
     @Before
     public void start() {
         clusterSettings = new ClusterSettings(Settings.EMPTY, Collections.singleton(MarvelSettings.HISTORY_DURATION));
-        defaultRetention = TimeValue.parseTimeValue("7d", null, "");
         threadPool = new ThreadPool("CleanerServiceTests");
     }
 
@@ -44,64 +48,81 @@ public class CleanerServiceTests extends ESTestCase {
         terminate(threadPool);
     }
 
-    public void testRetentionDefaultValue() {
+    public void testConstructorWithInvalidRetention() {
-        MarvelLicensee licensee = mock(MarvelLicensee.class);
+        // invalid setting
-        when(licensee.allowUpdateRetention()).thenReturn(false);
+        expectedException.expect(IllegalArgumentException.class);
-        assertNull(new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee).getRetention());
+
+        TimeValue expected = TimeValue.timeValueHours(1);
+        Settings settings = Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(), expected.getStringRep()).build();
+
+        new CleanerService(settings, clusterSettings, threadPool, licensee);
     }
 
-    public void testRetentionUpdateAllowed() {
+    public void testGetRetentionWithSettingWithUpdatesAllowed() {
-        MarvelLicensee licensee = mock(MarvelLicensee.class);
+        TimeValue expected = TimeValue.timeValueHours(25);
+        Settings settings = Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(), expected.getStringRep()).build();
+
+        when(licensee.allowUpdateRetention()).thenReturn(true);
+
+        assertEquals(expected, new CleanerService(settings, clusterSettings, threadPool, licensee).getRetention());
+
+        verify(licensee).allowUpdateRetention();
+    }
+
+    public void testGetRetentionDefaultValueWithNoSettings() {
+        when(licensee.allowUpdateRetention()).thenReturn(true);
+
+        assertEquals(MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY),
+                     new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee).getRetention());
+
+        verify(licensee).allowUpdateRetention();
+    }
+
+    public void testGetRetentionDefaultValueWithSettingsButUpdatesNotAllowed() {
+        TimeValue notExpected = TimeValue.timeValueHours(25);
+        Settings settings = Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(), notExpected.getStringRep()).build();
+
+        when(licensee.allowUpdateRetention()).thenReturn(false);
+
+        assertEquals(MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY),
+                     new CleanerService(settings, clusterSettings, threadPool, licensee).getRetention());
+
+        verify(licensee).allowUpdateRetention();
+    }
+
+    public void testSetGlobalRetention() {
+        // Note: I used this value to ensure we're not double-validating the setter; the cluster state should be the
+        // only thing calling this method and it will use the settings object to validate the time value
+        TimeValue expected = TimeValue.timeValueHours(2);
+
         when(licensee.allowUpdateRetention()).thenReturn(true);
 
         CleanerService service = new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee);
-        service.setRetention(TimeValue.parseTimeValue("-1", null, ""));
-        assertThat(service.getRetention().getMillis(), equalTo(-1L));
 
-        TimeValue randomRetention = TimeValue.parseTimeValue(randomIntBetween(1, 1000) + "ms", null, "");
+        service.setGlobalRetention(expected);
-        service.setRetention(randomRetention);
-        assertThat(service.getRetention(), equalTo(randomRetention));
 
-        try {
+        assertEquals(expected, service.getRetention());
-            service.validateRetention(randomRetention);
+
-        } catch (IllegalArgumentException e) {
+        verify(licensee, times(2)).allowUpdateRetention(); // once by set, once by get
-            fail("fail to validate new value of retention");
-        }
     }
 
-    public void testRetentionUpdateBlocked() {
+    public void testSetGlobalRetentionAppliesEvenIfLicenseDisallows() {
-        MarvelLicensee licensee = mock(MarvelLicensee.class);
+        // Note: I used this value to ensure we're not double-validating the setter; the cluster state should be the
-        when(licensee.allowUpdateRetention()).thenReturn(true);
+        // only thing calling this method and it will use the settings object to validate the time value
+        TimeValue expected = TimeValue.timeValueHours(2);
+
+        // required to be true on the second call for it to see it take effect
+        when(licensee.allowUpdateRetention()).thenReturn(false).thenReturn(true);
+
         CleanerService service = new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee);
-        try {
-            service.setRetention(TimeValue.parseTimeValue("-5000ms", null, ""));
-            fail("exception should have been thrown: negative retention are not allowed");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("invalid history duration setting value"));
-        }
-        try {
-            service.setRetention(null);
-            fail("exception should have been thrown: null retention is not allowed");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("history duration setting cannot be null"));
-        }
 
-        TimeValue randomRetention = TimeValue.parseTimeValue(randomIntBetween(1, 1000) + "ms", null, "");
+        // uses allow=false
-        when(licensee.allowUpdateRetention()).thenReturn(false);
+        service.setGlobalRetention(expected);
-        try {
-            service.setRetention(randomRetention);
-            fail("exception should have been thrown");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("license does not allow the history duration setting to be updated to value"));
-            assertNull(service.getRetention());
-        }
 
-        try {
+        // uses allow=true
-            service.validateRetention(randomRetention);
+        assertEquals(expected, service.getRetention());
-            fail("exception should have been thrown");
+
-        } catch (IllegalArgumentException e) {
+        verify(licensee, times(2)).allowUpdateRetention();
-            assertThat(e.getMessage(), containsString("license does not allow the history duration setting to be updated to value"));
-        }
     }
 
     public void testNextExecutionDelay() {

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/support/VersionUtilsTests.java

@@ -18,7 +18,7 @@ public class VersionUtilsTests extends ESTestCase {
     public void testParseVersion() {
         List<Version> versions = randomSubsetOf(9, Version.V_2_0_0_beta1, Version.V_2_0_0_beta2, Version.V_2_0_0_rc1, Version.V_2_0_0,
                 Version.V_2_0_1, Version.V_2_0_2, Version.V_2_1_0, Version.V_2_1_1, Version.V_2_1_2, Version.V_2_2_0, Version.V_2_3_0,
-                Version.V_5_0_0);
+                Version.V_5_0_0_alpha1);
         for (Version version : versions) {
             String output = createOutput(VersionUtils.VERSION_NUMBER_FIELD, version.toString());
             assertThat(VersionUtils.parseVersion(output.getBytes(StandardCharsets.UTF_8)), equalTo(version));

elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/user/PutUserRequestBuilder.java

@@ -11,12 +11,14 @@ import org.elasticsearch.client.ElasticsearchClient;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseFieldMatcher;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.shield.User;
 import org.elasticsearch.shield.authc.support.Hasher;
 import org.elasticsearch.shield.authc.support.SecuredString;
+import org.elasticsearch.shield.support.Validation;
 import org.elasticsearch.xpack.common.xcontent.XContentUtils;
 
 import java.io.IOException;
@@ -46,7 +48,17 @@ public class PutUserRequestBuilder extends ActionRequestBuilder<PutUserRequest,
     }
 
     public PutUserRequestBuilder password(@Nullable char[] password) {
-        request.passwordHash(password == null ? null : hasher.hash(new SecuredString(password)));
+        if (password != null) {
+            Validation.Error error = Validation.ESUsers.validatePassword(password);
+            if (error != null) {
+                ValidationException validationException = new ValidationException();
+                validationException.addValidationError(error.toString());
+                throw validationException;
+            }
+            request.passwordHash(hasher.hash(new SecuredString(password)));
+        } else {
+            request.passwordHash(null);
+        }
         return this;
     }
 

elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authc/esnative/ESNativeUsersStore.java

@@ -679,12 +679,13 @@ public class ESNativeUsersStore extends AbstractComponent implements ClusterStat
             final ObjectLongMap<String> map = new ObjectLongHashMap<>();
             SearchResponse response = null;
             try {
+                client.admin().indices().prepareRefresh(ShieldTemplateService.SECURITY_INDEX_NAME).get();
                 SearchRequest request = client.prepareSearch(ShieldTemplateService.SECURITY_INDEX_NAME)
                         .setScroll(scrollKeepAlive)
                         .setQuery(QueryBuilders.typeQuery(USER_DOC_TYPE))
                         .setSize(scrollSize)
                         .setVersion(true)
-                        .setFetchSource(true)
+                        .setFetchSource(false) // we only need id and version
                         .request();
                 response = client.search(request).actionGet();
 

elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/esnative/ESNativeRolesStore.java

@@ -496,6 +496,7 @@ public class ESNativeRolesStore extends AbstractComponent implements RolesStore,
             // create a copy of the keys in the cache since we will be modifying this list
             final Set<String> existingRoles = new HashSet<>(roleCache.keySet());
             try {
+                client.admin().indices().prepareRefresh(ShieldTemplateService.SECURITY_INDEX_NAME);
                 SearchRequest request = client.prepareSearch(ShieldTemplateService.SECURITY_INDEX_NAME)
                         .setScroll(scrollKeepAlive)
                         .setQuery(QueryBuilders.typeQuery(ROLE_DOC_TYPE))
@@ -538,7 +539,8 @@ public class ESNativeRolesStore extends AbstractComponent implements RolesStore,
                 // check to see if we had roles that do not exist in the index
                 if (existingRoles.isEmpty() == false) {
                     for (String roleName : existingRoles) {
-                        invalidate(roleName);
+                        logger.trace("role [{}] does not exist anymore, removing from cache", roleName);
+                        roleCache.remove(roleName);
                     }
                 }
             } catch (IndexNotFoundException e) {

elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java

@@ -23,7 +23,6 @@ import org.elasticsearch.shield.authz.esnative.ESNativeRolesStore;
 import org.elasticsearch.shield.client.SecurityClient;
 import org.elasticsearch.test.NativeRealmIntegTestCase;
 import org.elasticsearch.test.ShieldSettingsSource;
-import org.elasticsearch.test.junit.annotations.TestLogging;
 import org.elasticsearch.test.rest.client.http.HttpResponse;
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -39,7 +38,6 @@ import static org.hamcrest.Matchers.notNullValue;
  * Test for the Shield clear roles API that changes the polling aspect of shield to only run once an hour in order to
  * test the cache clearing APIs.
  */
-@TestLogging("shield.authc.esnative:TRACE,shield.authz.esnative:TRACE,integration:DEBUG")
 public class ClearRolesCacheTests extends NativeRealmIntegTestCase {
 
     private static String[] roles;
@@ -76,9 +74,11 @@ public class ClearRolesCacheTests extends NativeRealmIntegTestCase {
 
     @Override
     public Settings nodeSettings(int nodeOrdinal) {
+        TimeValue pollerInterval = TimeValue.timeValueMillis((long) randomIntBetween(2, 2000));
+        logger.debug("using poller interval [{}]", pollerInterval);
         return Settings.builder()
                 .put(super.nodeSettings(nodeOrdinal))
-                .put("shield.authc.native.reload.interval", TimeValue.timeValueSeconds(2L))
+                .put("shield.authc.native.reload.interval", pollerInterval)
                 .put(NetworkModule.HTTP_ENABLED.getKey(), true)
                 .build();
     }
@@ -90,11 +90,13 @@ public class ClearRolesCacheTests extends NativeRealmIntegTestCase {
         int modifiedRolesCount = randomIntBetween(1, roles.length);
         List<String> toModify = randomSubsetOf(modifiedRolesCount, roles);
         logger.debug("--> modifying roles {} to have run_as", toModify);
+        final boolean refresh = randomBoolean();
         for (String role : toModify) {
             PutRoleResponse response = securityClient.preparePutRole(role)
                     .cluster("none")
                     .addIndices(new String[] { "*" }, new String[] { "ALL" }, null, null)
                     .runAs(role)
+                    .refresh(refresh)
                     .get();
             assertThat(response.isCreated(), is(false));
             logger.debug("--> updated role [{}] with run_as", role);
@@ -107,10 +109,12 @@ public class ClearRolesCacheTests extends NativeRealmIntegTestCase {
         int modifiedRolesCount = randomIntBetween(1, roles.length);
         List<String> toModify = randomSubsetOf(modifiedRolesCount, roles);
         logger.debug("--> modifying roles {} to have run_as", toModify);
+        final boolean refresh = randomBoolean();
         for (String role : toModify) {
             UpdateResponse response = internalClient().prepareUpdate().setId(role).setIndex(ShieldTemplateService.SECURITY_INDEX_NAME)
                     .setType(ESNativeRolesStore.ROLE_DOC_TYPE)
                     .setDoc("run_as", new String[] { role })
+                    .setRefresh(refresh)
                     .get();
             assertThat(response.isCreated(), is(false));
             logger.debug("--> updated role [{}] with run_as", role);
@@ -150,8 +154,11 @@ public class ClearRolesCacheTests extends NativeRealmIntegTestCase {
         RoleDescriptor[] foundRoles = securityClient.prepareGetRoles().names(role).get().roles();
         assertThat(foundRoles.length, is(1));
         logger.debug("--> deleting role [{}]", role);
+        final boolean refresh = randomBoolean();
         DeleteResponse response = internalClient()
-                .prepareDelete(ShieldTemplateService.SECURITY_INDEX_NAME, ESNativeRolesStore.ROLE_DOC_TYPE, role).get();
+                .prepareDelete(ShieldTemplateService.SECURITY_INDEX_NAME, ESNativeRolesStore.ROLE_DOC_TYPE, role)
+                .setRefresh(refresh)
+                .get();
         assertThat(response.isFound(), is(true));
 
         assertBusy(new Runnable() {

elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/VersionCompatibilityTests.java

@@ -37,6 +37,6 @@ public class VersionCompatibilityTests extends ESTestCase {
          *
          */
         assertThat("Remove workaround in LicenseService class when es core supports merging cluster level custom metadata",
-                Version.CURRENT.onOrBefore(Version.V_5_0_0), is(true));
+                Version.CURRENT.onOrBefore(Version.V_5_0_0_alpha1), is(true));
     }
 }

elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/authc/esnative/ESNativeTests.java

@@ -10,6 +10,7 @@ import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.shield.ShieldTemplateService;
@@ -386,4 +387,14 @@ public class ESNativeTests extends NativeRealmIntegTestCase {
                 .admin().cluster().prepareHealth().get();
         assertFalse(response.isTimedOut());
     }
+
+    public void testCannotCreateUserWithShortPassword() throws Exception {
+        SecurityClient client = securityClient();
+        try {
+            client.preparePutUser("joe", randomAsciiOfLengthBetween(0, 5).toCharArray(), "admin_role").get();
+            fail("cannot create a user without a password < 6 characters");
+        } catch (ValidationException v) {
+            assertThat(v.getMessage().contains("password"), is(true));
+        }
+    }
 }

elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/actions

@@ -64,7 +64,6 @@ indices:data/read/script/get
 indices:data/read/scroll
 indices:data/read/scroll/clear
 indices:data/read/search
-indices:data/read/suggest
 indices:data/read/tv
 indices:data/write/bulk
 indices:data/write/delete

elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/handlers

@@ -51,7 +51,6 @@ indices:data/read/search[phase/query/id]
 indices:data/read/search[phase/query/query+fetch]
 indices:data/read/search[phase/query/scroll]
 indices:data/read/search[phase/query]
-indices:data/read/suggest[s]
 indices:data/read/tv[s]
 indices:data/write/bulk[s]
 indices:data/write/bulk[s][p]