Skip to content

Commit 6a0531a

Browse files
authored
Fix intermittent failure in ApiKeyIntegTests (#38627)
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 #38408
1 parent 70b6134 commit 6a0531a

File tree

3 files changed

+144
-120
lines changed

3 files changed

+144
-120
lines changed

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@
6868
import org.elasticsearch.xpack.core.security.user.User;
6969
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
7070

71-
import javax.crypto.SecretKeyFactory;
7271
import java.io.Closeable;
7372
import java.io.IOException;
7473
import java.io.UncheckedIOException;
@@ -92,6 +91,8 @@
9291
import java.util.function.Function;
9392
import java.util.stream.Collectors;
9493

94+
import javax.crypto.SecretKeyFactory;
95+
9596
import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING;
9697
import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
9798
import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
@@ -692,7 +693,6 @@ private void findApiKeys(final BoolQueryBuilder boolQuery, boolean filterOutInva
692693
expiredQuery.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time")));
693694
boolQuery.filter(expiredQuery);
694695
}
695-
696696
final SearchRequest request = client.prepareSearch(SecurityIndexManager.SECURITY_INDEX_NAME)
697697
.setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings))
698698
.setQuery(boolQuery)
@@ -852,10 +852,16 @@ private <E extends Throwable> E traceLog(String action, E exception) {
852852
return exception;
853853
}
854854

855+
// pkg scoped for testing
855856
boolean isExpirationInProgress() {
856857
return expiredApiKeysRemover.isExpirationInProgress();
857858
}
858859

860+
// pkg scoped for testing
861+
long lastTimeWhenApiKeysRemoverWasTriggered() {
862+
return lastExpirationRunMs;
863+
}
864+
859865
private void maybeStartApiKeyRemover() {
860866
if (securityIndex.isAvailable()) {
861867
if (client.threadPool().relativeTimeInMillis() - lastExpirationRunMs > deleteInterval.getMillis()) {

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ExpiredApiKeysRemover.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import org.elasticsearch.action.ActionListener;
1313
import org.elasticsearch.action.bulk.BulkItemResponse;
1414
import org.elasticsearch.client.Client;
15-
import org.elasticsearch.common.Strings;
1615
import org.elasticsearch.common.settings.Settings;
1716
import org.elasticsearch.common.unit.TimeValue;
1817
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
@@ -25,8 +24,8 @@
2524
import org.elasticsearch.threadpool.ThreadPool.Names;
2625
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
2726

27+
import java.time.Duration;
2828
import java.time.Instant;
29-
import java.time.temporal.ChronoUnit;
3029
import java.util.concurrent.atomic.AtomicBoolean;
3130

3231
import static org.elasticsearch.action.support.TransportActions.isShardNotAvailableException;
@@ -37,6 +36,8 @@
3736
* Responsible for cleaning the invalidated and expired API keys from the security index.
3837
*/
3938
public final class ExpiredApiKeysRemover extends AbstractRunnable {
39+
public static final Duration EXPIRED_API_KEYS_RETENTION_PERIOD = Duration.ofDays(7L);
40+
4041
private static final Logger logger = LogManager.getLogger(ExpiredApiKeysRemover.class);
4142

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

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

0 commit comments

Comments
 (0)