From 84d27b52be7649ef526c18ac5a8a178c423d3522 Mon Sep 17 00:00:00 2001 From: Areek Zillur Date: Wed, 23 Mar 2016 16:19:36 -0400 Subject: [PATCH 01/10] fix for removing suggest transport action in core elasticsearchelastic/elasticsearch#17198 Original commit: elastic/x-pack-elasticsearch@c8a742c9e47e036f445c225509eb2d5093a4b832 --- .../src/test/resources/org/elasticsearch/transport/actions | 1 - .../src/test/resources/org/elasticsearch/transport/handlers | 1 - 2 files changed, 2 deletions(-) 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] From 2f267530a639a6db9731915fb40b4cbe3f6667e7 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 24 Mar 2016 08:36:43 +0100 Subject: [PATCH 02/10] Set version 5.0.0-alpha1 Helping commit for changes in the core Original commit: elastic/x-pack-elasticsearch@73c8e19a292d44375eca12c792c2f6be6211fc9f --- .../org/elasticsearch/marvel/support/VersionUtilsTests.java | 2 +- .../org/elasticsearch/shield/VersionCompatibilityTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/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)); } } From 8e3e19d8c653506a727e6081d48f0d9ca8c0e462 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 23 Mar 2016 12:13:02 -0400 Subject: [PATCH 03/10] Handle method rename in core Original commit: elastic/x-pack-elasticsearch@43b5edbff07f8bdbcc76c4333bd23f0293df4556 --- .../org/elasticsearch/marvel/action/MonitoringBulkResponse.java | 2 +- .../org/elasticsearch/marvel/agent/exporter/MonitoringDoc.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/action/MonitoringBulkResponse.java b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/action/MonitoringBulkResponse.java index 070f10f5e2a..e7544a99434 100644 --- a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/action/MonitoringBulkResponse.java +++ b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/action/MonitoringBulkResponse.java @@ -63,7 +63,7 @@ public class MonitoringBulkResponse extends ActionResponse { public void readFrom(StreamInput in) throws IOException { super.readFrom(in); tookInMillis = in.readVLong(); - error = in.readOptionalWritable(Error::new); + error = in.readOptionalWriteable(Error::new); } @Override diff --git a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/MonitoringDoc.java b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/MonitoringDoc.java index b2199b7f417..e439b4758f2 100644 --- a/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/MonitoringDoc.java +++ b/elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/MonitoringDoc.java @@ -38,7 +38,7 @@ public class MonitoringDoc implements Writeable { this(in.readOptionalString(), in.readOptionalString()); clusterUUID = in.readOptionalString(); timestamp = in.readVLong(); - sourceNode = in.readOptionalWritable(Node::new); + sourceNode = in.readOptionalWriteable(Node::new); } public String getClusterUUID() { From 43de1ff8da687fc72b25bf9c94097bd11a2aa08c Mon Sep 17 00:00:00 2001 From: Chris Earle Date: Mon, 22 Feb 2016 15:17:25 -0500 Subject: [PATCH 04/10] Modify the CleanerService to use a minimum Users running the `CleanerService` should not be able to disable it (via a `-1` as the time setting) because they'll just shoot themselves in the foot. This PR changes the behavior to allow extensive amounts via the setting (e.g., they could set it to 2 years). By doing this via the `Setting`, we can avoid a lot of boilerplate code for verification as well. If we decide to allow it to be disabled, then the setting should be explicit. I've found that users tend to not understand setting times to `-1`. With the internal `IndicesCleaner` runnable, I have also moved the rescheduling code to `onAfter` so that it always happens, even if the license makes it temporarily invalid. I also think that we should allow the user to dynamically set the setting regardless of it being allowed -- and warn on it. This way they can set it when it's expired or during the trial, but it will take effect when they apply the paid license. I think that this will provide a better user experience so that they do not have to remember to re-set it later. This also removes the `LocalExporter`-specific setting that allowed it to override the global retention. If we ever add another listener, then we should add exporter-specific settings to support this kind of functionality. Adds some tests for the settings as well as for the service, while also removing now unneeded ones. Original commit: elastic/x-pack-elasticsearch@3abd41807e2f24859dd990cdb393e8c576083f23 --- .../elasticsearch/marvel/MarvelSettings.java | 29 +++- .../agent/exporter/local/LocalExporter.java | 13 -- .../marvel/cleaner/CleanerService.java | 130 ++++++++++++------ .../marvel/MarvelSettingsTests.java | 49 +++++++ .../AbstractIndicesCleanerTestCase.java | 49 +------ .../marvel/cleaner/CleanerServiceTests.java | 119 +++++++++------- 6 files changed, 232 insertions(+), 157 deletions(-) create mode 100644 elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/MarvelSettingsTests.java 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..f1fab8813b2 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: + *

    + *
  • Default: 7 days
    • + *
  • Minimum: 1 day
    • + *
+ * + * @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,15 @@ 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. + *

+ * The {@code key} is prefixed by {@link XPackPlugin#featureSettingPrefix(String)}, {@link Marvel#NAME}, and {@code "."}. + * + * @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..67f022010bb 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 @@ -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,60 @@ public class CleanerService extends AbstractLifecycleComponent { return ThreadPool.Names.GENERIC; } - TimeValue getRetention() { + /** + * 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() { + TimeValue retention; + + // we only care about their value if they are allowed to set it + if (licensee.allowUpdateRetention() && globalRetention != null) { + retention = globalRetention; + } + else { + retention = MarvelSettings.HISTORY_DURATION.getDefault(Settings.EMPTY); + } + 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"); - } - 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 + "]"); - } + /** + * 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,6 +151,10 @@ public class CleanerService extends AbstractLifecycleComponent { void onCleanUpIndices(TimeValue retention); } + /** + * {@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 AbstractRunnable { private volatile ScheduledFuture future; @@ -131,37 +165,39 @@ public class CleanerService extends AbstractLifecycleComponent { logger.trace("cleaning service is stopping, exiting"); return; } - if (!licensee.cleaningEnabled()) { + 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); } } - DateTime start = new DateTime(ISOChronology.getInstance()); - if (globalRetention.millis() > 0) { - logger.trace("cleaning up indices with retention [{}]", globalRetention); + logger.trace("done cleaning up indices"); + } - for (Listener listener : listeners) { - try { - listener.onCleanUpIndices(globalRetention); - } catch (Throwable t) { - logger.error("listener failed to clean indices", t); - } - } - } - - if (!lifecycle.stoppedOrClosed()) { + /** + * Reschedule the cleaner if the service is not stopped. + */ + @Override + public void onAfter() { + if (lifecycle.stoppedOrClosed() == false) { + DateTime start = new DateTime(ISOChronology.getInstance()); TimeValue delay = executionScheduler.nextExecutionDelay(start); + logger.debug("scheduling next execution in [{}] seconds", delay.seconds()); + future = threadPool.schedule(delay, executorName(), this); } } @@ -171,6 +207,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 +222,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 +231,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 01:00 AM on the next day. */ static class DefaultExecutionScheduler implements ExecutionScheduler { 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() { From 87c37302447910978e3b86de4f673bdd9adff083 Mon Sep 17 00:00:00 2001 From: Chris Earle Date: Wed, 24 Feb 2016 14:21:44 -0500 Subject: [PATCH 05/10] Removing unnecessary JavaDoc Original commit: elastic/x-pack-elasticsearch@083f5529ac8f511fd92ca3e7c292d2a5e34682d9 --- .../src/main/java/org/elasticsearch/marvel/MarvelSettings.java | 2 -- 1 file changed, 2 deletions(-) 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 f1fab8813b2..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 @@ -238,8 +238,6 @@ public class MarvelSettings extends AbstractComponent { /** * Prefix the {@code key} with the Monitoring prefix. - *

- * The {@code key} is prefixed by {@link XPackPlugin#featureSettingPrefix(String)}, {@link Marvel#NAME}, and {@code "."}. * * @param key The key to prefix * @return The key prefixed by the product prefixes. From ac6b5b7c25628dd78c17382471841d99a9e2255a Mon Sep 17 00:00:00 2001 From: Chris Earle Date: Sun, 6 Mar 2016 22:36:55 -0500 Subject: [PATCH 06/10] Modifying based on review comments Original commit: elastic/x-pack-elasticsearch@8e3b5f4c171468d02a969085d6a360e1ec02fee8 --- .../marvel/cleaner/CleanerService.java | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) 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 67f022010bb..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; @@ -88,17 +88,13 @@ public class CleanerService extends AbstractLifecycleComponent { * @see MarvelLicensee#allowUpdateRetention() */ public TimeValue getRetention() { - TimeValue retention; - // we only care about their value if they are allowed to set it if (licensee.allowUpdateRetention() && globalRetention != null) { - retention = globalRetention; + return globalRetention; } else { - retention = MarvelSettings.HISTORY_DURATION.getDefault(Settings.EMPTY); + return MarvelSettings.HISTORY_DURATION.getDefault(Settings.EMPTY); } - - return retention; } /** @@ -155,16 +151,19 @@ public class CleanerService extends AbstractLifecycleComponent { * {@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 AbstractRunnable { + 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; - } + protected void doRunInLifecycle() throws Exception { if (licensee.cleaningEnabled() == false) { logger.debug("cleaning service is disabled due to invalid license"); return; @@ -191,15 +190,13 @@ public class CleanerService extends AbstractLifecycleComponent { * Reschedule the cleaner if the service is not stopped. */ @Override - public void onAfter() { - if (lifecycle.stoppedOrClosed() == false) { - DateTime start = new DateTime(ISOChronology.getInstance()); - TimeValue delay = executionScheduler.nextExecutionDelay(start); + protected void onAfterInLifecycle() { + DateTime start = new DateTime(ISOChronology.getInstance()); + TimeValue delay = executionScheduler.nextExecutionDelay(start); - logger.debug("scheduling next execution in [{}] seconds", delay.seconds()); + logger.debug("scheduling next execution in [{}] seconds", delay.seconds()); - future = threadPool.schedule(delay, executorName(), this); - } + future = threadPool.schedule(delay, executorName(), this); } @Override @@ -231,7 +228,7 @@ public class CleanerService extends AbstractLifecycleComponent { } /** - * Schedule task so that it will be executed everyday at 01:00 AM on the next day. + * Schedule task so that it will be executed everyday at the next 01:00 AM. */ static class DefaultExecutionScheduler implements ExecutionScheduler { @@ -239,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()); From ca9ebf535130f293aa16d5b2cef5c137972c7b25 Mon Sep 17 00:00:00 2001 From: jaymode Date: Thu, 24 Mar 2016 12:55:35 -0400 Subject: [PATCH 07/10] security: refresh before searching in pollers This commit is the forward port of fixes made in 2.3 for the roles and users pollers. The pollers now refresh since not all operations are guaranteed to refresh. The clear roles tests are also made more evil since the poller runs at different intervals on each node and can sometimes run almost continuously. The modification requests now randomize if they refresh or not as well. Original commit: elastic/x-pack-elasticsearch@f61159c40a4c558d908d7773a5c77d1c7bce7364 --- .../authc/esnative/ESNativeUsersStore.java | 3 ++- .../authz/esnative/ESNativeRolesStore.java | 19 ++++++++++++++++--- .../integration/ClearRolesCacheTests.java | 15 +++++++++++---- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authc/esnative/ESNativeUsersStore.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authc/esnative/ESNativeUsersStore.java index babbb5fd773..01b6f6b2765 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authc/esnative/ESNativeUsersStore.java +++ b/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 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..4a024198bc3 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)) @@ -513,12 +514,23 @@ public class ESNativeRolesStore extends AbstractComponent implements RolesStore, for (SearchHit hit : response.getHits().getHits()) { final String roleName = hit.getId(); final long version = hit.version(); - existingRoles.remove(roleName); + final boolean existed = existingRoles.remove(roleName); // we use the locking mechanisms provided by the map/cache to help protect against concurrent operations // that will leave the cache in a bad state - roleCache.computeIfPresent(roleName, new BiFunction() { + roleCache.compute(roleName, new BiFunction() { @Override public RoleAndVersion apply(String roleName, RoleAndVersion existing) { + if (existing == null) { + if (existed) { + // the cache doesn't have this role anymore, it got cleared by something else, do nothing. + return null; + } else { + // it is new, we can cache it + RoleDescriptor rd = transformRole(hit.getId(), hit.getSourceRef()); + return new RoleAndVersion(rd, version); + } + } + if (version > existing.getVersion()) { RoleDescriptor rd = transformRole(hit.getId(), hit.getSourceRef()); if (rd != null) { @@ -538,7 +550,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() { From cf6cadf19fa8bd9ec6aeaaf9ce6074fdcb18923e Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 24 Mar 2016 09:02:41 +0100 Subject: [PATCH 08/10] Build: Move xpack to plugin group This is needed in order to make `bin/elasticsearch-plugin install xpack` work, as it expects the plugin in a certain path. Original commit: elastic/x-pack-elasticsearch@252c55e5a873518af3b8fc51604c5482710f6772 --- elasticsearch/x-pack/build.gradle | 2 ++ 1 file changed, 2 insertions(+) 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' From 929e1791503840655d4941576a5d192efc60d4d3 Mon Sep 17 00:00:00 2001 From: jaymode Date: Thu, 24 Mar 2016 14:56:11 -0400 Subject: [PATCH 09/10] shield: put user should validate password length This changes the put user request builder to validate password length when a password is provided. The validation is the same as what we use in the file based realm. Closes elastic/elasticsearch#1800 Original commit: elastic/x-pack-elasticsearch@fde1d6c6856b5d90540aaaf7c845838f0d6deee7 --- .../shield/action/user/PutUserRequestBuilder.java | 14 +++++++++++++- .../shield/authc/esnative/ESNativeTests.java | 11 +++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) 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 Date: Fri, 25 Mar 2016 07:24:26 -0400 Subject: [PATCH 10/10] security: roles store poller should only update existing entries Original commit: elastic/x-pack-elasticsearch@6573f4d6896caff284af23f52172d3810db4485f --- .../shield/authz/esnative/ESNativeRolesStore.java | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) 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 4a024198bc3..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 @@ -514,23 +514,12 @@ public class ESNativeRolesStore extends AbstractComponent implements RolesStore, for (SearchHit hit : response.getHits().getHits()) { final String roleName = hit.getId(); final long version = hit.version(); - final boolean existed = existingRoles.remove(roleName); + existingRoles.remove(roleName); // we use the locking mechanisms provided by the map/cache to help protect against concurrent operations // that will leave the cache in a bad state - roleCache.compute(roleName, new BiFunction() { + roleCache.computeIfPresent(roleName, new BiFunction() { @Override public RoleAndVersion apply(String roleName, RoleAndVersion existing) { - if (existing == null) { - if (existed) { - // the cache doesn't have this role anymore, it got cleared by something else, do nothing. - return null; - } else { - // it is new, we can cache it - RoleDescriptor rd = transformRole(hit.getId(), hit.getSourceRef()); - return new RoleAndVersion(rd, version); - } - } - if (version > existing.getVersion()) { RoleDescriptor rd = transformRole(hit.getId(), hit.getSourceRef()); if (rd != null) {