diff --git a/elasticsearch/x-pack/build.gradle b/elasticsearch/x-pack/build.gradle index 8e68915e7b0..c2584564d5b 100644 --- a/elasticsearch/x-pack/build.gradle +++ b/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' diff --git a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/MarvelSettings.java b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/MarvelSettings.java index 37cf6ee90d4..90dcf37dbde 100644 --- a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/MarvelSettings.java +++ b/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. + *

+ * Expected values: + *

+ * + * @see #HISTORY_DURATION_MINIMUM */ public static final Setting 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; } diff --git a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/local/LocalExporter.java b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/local/LocalExporter.java index c547114b5df..06e500309b3 100644 --- a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/local/LocalExporter.java +++ b/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()); diff --git a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/cleaner/CleanerService.java b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/cleaner/CleanerService.java index b36a91c598e..ff85ce73a43 100644 --- a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/cleaner/CleanerService.java +++ b/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 { - private final MarvelLicensee licensee; private final ThreadPool threadPool; private final ExecutionScheduler executionScheduler; private final List 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 { 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 { return ThreadPool.Names.GENERIC; } - TimeValue getRetention() { - return retention; - } - - public void setRetention(TimeValue retention) { - validateRetention(retention); - this.retention = retention; - } - - public void validateRetention(TimeValue retention) { - if (retention == null) { - throw new IllegalArgumentException("history duration setting cannot be null"); + /** + * Get the retention that can be used. + *

+ * This will ignore the global retention if the license does not allow retention updates. + * + * @return Never {@code null} + * @see MarvelLicensee#allowUpdateRetention() + */ + public TimeValue getRetention() { + // we only care about their value if they are allowed to set it + if (licensee.allowUpdateRetention() && globalRetention != null) { + return globalRetention; } - if ((retention.getMillis() <= 0) && (retention.getMillis() != -1)) { - throw new IllegalArgumentException("invalid history duration setting value"); - } - if (!licensee.allowUpdateRetention()) { - throw new IllegalArgumentException("license does not allow the history duration setting to be updated to value [" - + retention + "]"); + else { + return MarvelSettings.HISTORY_DURATION.getDefault(Settings.EMPTY); } } + /** + * Set the global retention. This is expected to be used by the cluster settings to dynamically control the global retention time. + *

+ * 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 { 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 { - if (lifecycle.stoppedOrClosed()) { - logger.trace("cleaning service is stopping, exiting"); - return; - } - if (!licensee.cleaningEnabled()) { + protected void doRunInLifecycle() throws Exception { + if (licensee.cleaningEnabled() == false) { logger.debug("cleaning service is disabled due to invalid license"); return; } - TimeValue globalRetention = retention; - if (globalRetention == null) { + // fetch the retention, which is depends on a bunch of rules + 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); - validateRetention(globalRetention); - } catch (IllegalArgumentException e) { - globalRetention = MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY); + listener.onCleanUpIndices(retention); + } catch (Throwable t) { + logger.error("listener failed to clean indices", t); } } + 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) { - logger.trace("cleaning up indices with retention [{}]", globalRetention); + TimeValue delay = executionScheduler.nextExecutionDelay(start); - for (Listener listener : listeners) { - try { - listener.onCleanUpIndices(globalRetention); - } catch (Throwable t) { - logger.error("listener failed to clean indices", t); - } - } - } + logger.debug("scheduling next execution in [{}] seconds", delay.seconds()); - if (!lifecycle.stoppedOrClosed()) { - TimeValue delay = executionScheduler.nextExecutionDelay(start); - logger.debug("scheduling next execution in [{}] seconds", delay.seconds()); - future = threadPool.schedule(delay, executorName(), this); - } + future = threadPool.schedule(delay, executorName(), this); } @Override @@ -171,6 +204,13 @@ public class CleanerService extends AbstractLifecycleComponent { logger.error("failed to clean indices", t); } + /** + * Cancel/stop the cleaning service. + *

+ * 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 if the service is + * stopped. + */ public void cancel() { FutureUtils.cancel(future); } @@ -179,8 +219,7 @@ public class CleanerService extends AbstractLifecycleComponent { interface ExecutionScheduler { /** - * Calculates the delay in millis between "now" and - * the next execution. + * Calculates the delay in millis between "now" and the next execution. * * @param now the current time * @return the delay in millis @@ -189,7 +228,7 @@ public class CleanerService extends AbstractLifecycleComponent { } /** - * 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 { 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()); diff --git a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/MarvelSettingsTests.java b/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/MarvelSettingsTests.java new file mode 100644 index 00000000000..82f1a9d2abd --- /dev/null +++ b/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(); + } +} diff --git a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/AbstractIndicesCleanerTestCase.java b/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/AbstractIndicesCleanerTestCase.java index 522c520de43..1d56ea411bb 100644 --- a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/AbstractIndicesCleanerTestCase.java +++ b/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.HISTORY_DURATION.getKey(), "-1"); + .put(MarvelSettings.INTERVAL.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) { diff --git a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/CleanerServiceTests.java b/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/CleanerServiceTests.java index e8365de3267..8476a92e69b 100644 --- a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/CleanerServiceTests.java +++ b/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() { - MarvelLicensee licensee = mock(MarvelLicensee.class); - when(licensee.allowUpdateRetention()).thenReturn(false); - assertNull(new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee).getRetention()); + public void testConstructorWithInvalidRetention() { + // invalid setting + expectedException.expect(IllegalArgumentException.class); + + 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() { - MarvelLicensee licensee = mock(MarvelLicensee.class); + public void testGetRetentionWithSettingWithUpdatesAllowed() { + 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.setRetention(randomRetention); - assertThat(service.getRetention(), equalTo(randomRetention)); + service.setGlobalRetention(expected); - try { - service.validateRetention(randomRetention); - } catch (IllegalArgumentException e) { - fail("fail to validate new value of retention"); - } + assertEquals(expected, service.getRetention()); + + verify(licensee, times(2)).allowUpdateRetention(); // once by set, once by get } - public void testRetentionUpdateBlocked() { - MarvelLicensee licensee = mock(MarvelLicensee.class); - when(licensee.allowUpdateRetention()).thenReturn(true); + public void testSetGlobalRetentionAppliesEvenIfLicenseDisallows() { + // 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); + + // 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, ""); - when(licensee.allowUpdateRetention()).thenReturn(false); - 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()); - } + // uses allow=false + service.setGlobalRetention(expected); - try { - service.validateRetention(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")); - } + // uses allow=true + assertEquals(expected, service.getRetention()); + + verify(licensee, times(2)).allowUpdateRetention(); } public void testNextExecutionDelay() { diff --git a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/support/VersionUtilsTests.java b/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/support/VersionUtilsTests.java index 722008a37b2..9f37cab21ac 100644 --- a/elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/support/VersionUtilsTests.java +++ b/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 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)); diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/user/PutUserRequestBuilder.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/user/PutUserRequestBuilder.java index 0225329421d..d3d2fc74320 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/user/PutUserRequestBuilder.java +++ b/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 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(); diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/esnative/ESNativeRolesStore.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/esnative/ESNativeRolesStore.java index 98396187539..9aa527fa4e5 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/esnative/ESNativeRolesStore.java +++ b/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 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) { diff --git a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java b/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java index 8ba77b32d07..065326da13f 100644 --- a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/integration/ClearRolesCacheTests.java +++ b/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 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 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() { diff --git a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/VersionCompatibilityTests.java b/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/VersionCompatibilityTests.java index cc4a145ee61..a02ac83c819 100644 --- a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/VersionCompatibilityTests.java +++ b/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)); } } diff --git a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/authc/esnative/ESNativeTests.java b/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/authc/esnative/ESNativeTests.java index 0700b09d5f4..b139a589628 100644 --- a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/authc/esnative/ESNativeTests.java +++ b/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)); + } + } } diff --git a/elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/actions b/elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/actions index 0075c73310e..52104851534 100644 --- a/elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/actions +++ b/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 diff --git a/elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/handlers b/elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/handlers index 4f8a63074b7..d4960a2091d 100644 --- a/elasticsearch/x-pack/shield/src/test/resources/org/elasticsearch/transport/handlers +++ b/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]