Skip to content

Commit

Permalink
Fix Encrypted Repo Test and RepositoriesService Logging (elastic#68469)…
Browse files Browse the repository at this point in the history 
… (elastic#68527)

This test creates a strange situation when it restarts the
two master nodes back to back without strong guarantees about
whether or not the cluster has cleanly formed again. Since we really just want to
make sure that the master is after the restart is actually running
with the changed password, it seemed easiest to simply do a full restart rather than the
current loop.

Also, this commit fixes the logging around repository creation and deletion which made this
hard to debug and by logging before a CS update has actually been processed would also
log incorrectly.
Lastly, I removed the now unnecessary hacks of using the generic pool on a node to fetch
repository data. This logic is not needed any longer and the repository will now automatically
use the correct thread under the hood. The hack effectively just adds another possible failure
mode around node restarts since the generic pool quietly rejects tasks after shutdown.

Closes elastic#67834
  • Loading branch information
@original-brownbear
original-brownbear authored Feb 4, 2021
1 parent 211cd8e commit eab1bda
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 30 deletions.
26 changes: 21 additions & 5 deletions server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,15 @@ public void registerRepository(final PutRepositoryRequest request, final ActionL
clusterService.submitStateUpdateTask("put_repository [" + request.name() + "]",
new AckedClusterStateUpdateTask(request, acknowledgementStep) {

private boolean found = false;
private boolean changed = false;

@Override
public ClusterState execute(ClusterState currentState) {
ensureRepositoryNotInUse(currentState, request.name());
Metadata metadata = currentState.metadata();
Metadata.Builder mdBuilder = Metadata.builder(currentState.metadata());
RepositoriesMetadata repositories = metadata.custom(RepositoriesMetadata.TYPE, RepositoriesMetadata.EMPTY);
boolean found = false;
List<RepositoryMetadata> repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1);
for (RepositoryMetadata repositoryMetadata : repositories.repositories()) {
if (repositoryMetadata.name().equals(newRepositoryMetadata.name())) {
Expand All @@ -189,13 +191,11 @@ public ClusterState execute(ClusterState currentState) {
}
}
if (found == false) {
logger.info("put repository [{}]", request.name());
repositoriesMetadata.add(new RepositoryMetadata(request.name(), request.type(), request.settings()));
} else {
logger.info("update repository [{}]", request.name());
}
repositories = new RepositoriesMetadata(repositoriesMetadata);
mdBuilder.putCustom(RepositoriesMetadata.TYPE, repositories);
changed = true;
return ClusterState.builder(currentState).metadata(mdBuilder).build();
}

Expand All @@ -214,6 +214,13 @@ public boolean mustAck(DiscoveryNode discoveryNode) {

@Override
public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) {
if (changed) {
if (found) {
logger.info("updated repository [{}]", request.name());
} else {
logger.info("put repository [{}]", request.name());
}
}
publicationStep.onResponse(null);
}
});
Expand Down Expand Up @@ -288,6 +295,8 @@ public void unregisterRepository(final DeleteRepositoryRequest request, final Ac
clusterService.submitStateUpdateTask("delete_repository [" + request.name() + "]",
new AckedClusterStateUpdateTask(request, listener) {

private final List<String> deletedRepositories = new ArrayList<>();

@Override
public ClusterState execute(ClusterState currentState) {
Metadata metadata = currentState.metadata();
Expand All @@ -299,7 +308,7 @@ public ClusterState execute(ClusterState currentState) {
for (RepositoryMetadata repositoryMetadata : repositories.repositories()) {
if (Regex.simpleMatch(request.name(), repositoryMetadata.name())) {
ensureRepositoryNotInUse(currentState, repositoryMetadata.name());
logger.info("delete repository [{}]", repositoryMetadata.name());
deletedRepositories.add(repositoryMetadata.name());
changed = true;
} else {
repositoriesMetadata.add(repositoryMetadata);
Expand All @@ -317,6 +326,13 @@ public ClusterState execute(ClusterState currentState) {
throw new RepositoryMissingException(request.name());
}

@Override
public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) {
if (deletedRepositories.isEmpty() == false) {
logger.info("deleted repositories [{}]", deletedRepositories);
}
}

@Override
public boolean mustAck(DiscoveryNode discoveryNode) {
// repository was created on both master and data nodes
Expand Down
5 changes: 1 addition & 4 deletions ...va/org/elasticsearch/repositories/encrypted/EncryptedFSBlobStoreRepositoryIntegTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/
package org.elasticsearch.repositories.encrypted;

import org.elasticsearch.action.ActionRunnable;
import org.elasticsearch.action.support.PlainActionFuture;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -158,9 +157,7 @@ public void testTamperedEncryptionMetadata() throws Exception {
assertThat(
expectThrows(
RepositoryException.class,
() -> PlainActionFuture.<RepositoryData, Exception>get(
f -> blobStoreRepository.threadPool().generic().execute(ActionRunnable.wrap(f, blobStoreRepository::getRepositoryData))
)
() -> PlainActionFuture.<RepositoryData, Exception>get(blobStoreRepository::getRepositoryData)
).getMessage(),
containsString("the encryption metadata in the repository has been corrupted")
);
Expand Down
27 changes: 6 additions & 21 deletions ...st/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package org.elasticsearch.repositories.encrypted;

import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.action.ActionRunnable;
import org.elasticsearch.action.admin.cluster.repositories.verify.VerifyRepositoryResponse;
import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse;
Expand Down Expand Up @@ -40,7 +39,6 @@

import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
Expand Down Expand Up @@ -549,7 +547,6 @@ public void testWrongRepositoryPassword() throws Exception {
internalCluster().startNodes(2, Settings.builder().setSecureSettings(secureSettingsWithPassword).build());
ensureStableCluster(2);
createRepository(repositoryName, repositorySettings, true);
// create empty smapshot
final String snapshotName = randomName();
logger.info("--> create empty snapshot {}:{}", repositoryName, snapshotName);
CreateSnapshotResponse createSnapshotResponse = client().admin()
Expand All @@ -575,14 +572,9 @@ public void testWrongRepositoryPassword() throws Exception {
EncryptedRepositoryPlugin.ENCRYPTION_PASSWORD_SETTING.getConcreteSettingForNamespace(repositoryName).getKey(),
wrongPassword
);
Set<String> nodesWithWrongPassword = new HashSet<>();
do {
String masterNodeName = internalCluster().getMasterName();
logger.info("--> restart master node {}", masterNodeName);
internalCluster().restartNode(masterNodeName, new InternalTestCluster.RestartCallback());
nodesWithWrongPassword.add(masterNodeName);
ensureStableCluster(2);
} while (false == nodesWithWrongPassword.contains(internalCluster().getMasterName()));

internalCluster().fullRestart();
ensureStableCluster(2);
// maybe recreate the repository
if (randomBoolean()) {
deleteRepository(repositoryName);
Expand All @@ -594,9 +586,7 @@ public void testWrongRepositoryPassword() throws Exception {
).repository(repositoryName);
RepositoryException e = expectThrows(
RepositoryException.class,
() -> PlainActionFuture.<RepositoryData, Exception>get(
f -> blobStoreRepository.threadPool().generic().execute(ActionRunnable.wrap(f, blobStoreRepository::getRepositoryData))
)
() -> PlainActionFuture.<RepositoryData, Exception>get(blobStoreRepository::getRepositoryData)
);
assertThat(e.getCause().getMessage(), containsString("repository password is incorrect"));
e = expectThrows(
Expand All @@ -621,13 +611,8 @@ public void testWrongRepositoryPassword() throws Exception {
EncryptedRepositoryPlugin.ENCRYPTION_PASSWORD_SETTING.getConcreteSettingForNamespace(repositoryName).getKey(),
goodPassword
);
do {
String masterNodeName = internalCluster().getMasterName();
logger.info("--> restart master node {}", masterNodeName);
internalCluster().restartNode(masterNodeName, new InternalTestCluster.RestartCallback());
nodesWithWrongPassword.remove(masterNodeName);
ensureStableCluster(2);
} while (nodesWithWrongPassword.contains(internalCluster().getMasterName()));
internalCluster().fullRestart();
ensureStableCluster(2);
// ensure get snapshot works
GetSnapshotsResponse getSnapshotResponse = client().admin().cluster().prepareGetSnapshots(repositoryName).get();
assertThat(getSnapshotResponse.getSnapshots(), hasSize(1));
Expand Down

0 comments on commit eab1bda

Please sign in to comment.