Skip to content

Commit

Permalink
Fix intermittent failure in ApiKeyIntegTests (elastic#38627)
Browse files Browse the repository at this point in the history
Few tests failed intermittently and most of the
times due to invalidated or expired keys that were
deleted were still reported in search results.
This commit removes the test and adds enhancements
to other tests testing different scenario's.

When ExpiredApiKeysRemover is triggered, the tests
did not await its termination thereby sometimes
the results would be wrong for a search operation.

DELETE_INTERVAL setting has been further reduced to
100ms so we can trigger ExpiredApiKeysRemover faster.

Closes elastic#38408
  • Loading branch information
bizybot authored and Yogesh Gaikwad committed Feb 15, 2019
1 parent b1f829f commit 1be7de6
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 120 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import javax.crypto.SecretKeyFactory;
import java.io.Closeable;
import java.io.IOException;
import java.io.UncheckedIOException;
Expand All @@ -92,6 +91,8 @@
import java.util.function.Function;
import java.util.stream.Collectors;

import javax.crypto.SecretKeyFactory;

import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING;
import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
Expand Down Expand Up @@ -692,7 +693,6 @@ private void findApiKeys(final BoolQueryBuilder boolQuery, boolean filterOutInva
expiredQuery.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time")));
boolQuery.filter(expiredQuery);
}

final SearchRequest request = client.prepareSearch(SecurityIndexManager.SECURITY_INDEX_NAME)
.setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings))
.setQuery(boolQuery)
Expand Down Expand Up @@ -852,10 +852,16 @@ private <E extends Throwable> E traceLog(String action, E exception) {
return exception;
}

// pkg scoped for testing
boolean isExpirationInProgress() {
return expiredApiKeysRemover.isExpirationInProgress();
}

// pkg scoped for testing
long lastTimeWhenApiKeysRemoverWasTriggered() {
return lastExpirationRunMs;
}

private void maybeStartApiKeyRemover() {
if (securityIndex.isAvailable()) {
if (client.threadPool().relativeTimeInMillis() - lastExpirationRunMs > deleteInterval.getMillis()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.bulk.BulkItemResponse;
import org.elasticsearch.client.Client;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
Expand All @@ -25,8 +24,8 @@
import org.elasticsearch.threadpool.ThreadPool.Names;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.concurrent.atomic.AtomicBoolean;

import static org.elasticsearch.action.support.TransportActions.isShardNotAvailableException;
Expand All @@ -37,6 +36,8 @@
* Responsible for cleaning the invalidated and expired API keys from the security index.
*/
public final class ExpiredApiKeysRemover extends AbstractRunnable {
public static final Duration EXPIRED_API_KEYS_RETENTION_PERIOD = Duration.ofDays(7L);

private static final Logger logger = LogManager.getLogger(ExpiredApiKeysRemover.class);

private final Client client;
Expand All @@ -60,11 +61,10 @@ public void doRun() {
.setQuery(QueryBuilders.boolQuery()
.filter(QueryBuilders.termsQuery("doc_type", "api_key"))
.should(QueryBuilders.termsQuery("api_key_invalidated", true))
.should(QueryBuilders.rangeQuery("expiration_time").lte(now.minus(7L, ChronoUnit.DAYS).toEpochMilli()))
.should(QueryBuilders.rangeQuery("expiration_time").lte(now.minus(EXPIRED_API_KEYS_RETENTION_PERIOD).toEpochMilli()))
.minimumShouldMatch(1)
);

logger.trace(() -> new ParameterizedMessage("Removing old api keys: [{}]", Strings.toString(expiredDbq)));
executeAsyncWithOrigin(client, SECURITY_ORIGIN, DeleteByQueryAction.INSTANCE, expiredDbq,
ActionListener.wrap(r -> {
debugDbqResponse(r);
Expand Down
Loading

0 comments on commit 1be7de6

Please sign in to comment.