Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update secure settings for the repository azure repository plugin #29319

Original file line number Diff line number Diff line change
Expand Up @@ -20,45 +20,43 @@
package org.elasticsearch.repositories.azure;

import com.microsoft.azure.storage.LocationMode;

import com.microsoft.azure.storage.StorageException;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.blobstore.BlobContainer;
import org.elasticsearch.common.blobstore.BlobMetaData;
import org.elasticsearch.common.blobstore.BlobPath;
import org.elasticsearch.common.blobstore.BlobStore;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.settings.Settings;

import java.io.IOException;
import java.io.InputStream;
import java.net.URISyntaxException;
import java.util.Locale;
import java.util.Map;

import static java.util.Collections.emptyMap;

import static org.elasticsearch.repositories.azure.AzureRepository.Repository;

public class AzureBlobStore extends AbstractComponent implements BlobStore {

private final AzureStorageService client;
private final AzureStorageService service;

private final String clientName;
private final LocationMode locMode;
private final String container;
private final LocationMode locationMode;

public AzureBlobStore(RepositoryMetaData metadata, Settings settings,
AzureStorageService client) throws URISyntaxException, StorageException {
public AzureBlobStore(RepositoryMetaData metadata, Settings settings, AzureStorageService service)
throws URISyntaxException, StorageException {
super(settings);
this.client = client;
this.container = Repository.CONTAINER_SETTING.get(metadata.settings());
this.clientName = Repository.CLIENT_NAME.get(metadata.settings());

String modeStr = Repository.LOCATION_MODE_SETTING.get(metadata.settings());
if (Strings.hasLength(modeStr)) {
this.locMode = LocationMode.valueOf(modeStr.toUpperCase(Locale.ROOT));
} else {
this.locMode = LocationMode.PRIMARY_ONLY;
}
this.service = service;
// locationMode is set per repository, not per client
this.locationMode = Repository.LOCATION_MODE_SETTING.get(metadata.settings());
final Map<String, AzureStorageSettings> prevSettings = this.service.updateClientsSettings(emptyMap());
final Map<String, AzureStorageSettings> newSettings = AzureStorageSettings.overrideLocationMode(prevSettings, this.locationMode);
this.service.updateClientsSettings(newSettings);
}

@Override
Expand All @@ -70,7 +68,11 @@ public String toString() {
* Gets the configured {@link LocationMode} for the Azure storage requests.
*/
public LocationMode getLocationMode() {
return locMode;
return locationMode;
}

public String getClientName() {
return clientName;
}

@Override
Expand All @@ -79,50 +81,52 @@ public BlobContainer blobContainer(BlobPath path) {
}

@Override
public void delete(BlobPath path) {
String keyPath = path.buildAsString();
public void delete(BlobPath path) throws IOException {
final String keyPath = path.buildAsString();
try {
this.client.deleteFiles(this.clientName, this.locMode, container, keyPath);
service.deleteFiles(clientName, container, keyPath);
} catch (URISyntaxException | StorageException e) {
logger.warn("can not remove [{}] in container {{}}: {}", keyPath, container, e.getMessage());
logger.warn("cannot access [{}] in container {{}}: {}", keyPath, container, e.getMessage());
throw new IOException(e);
}
}

@Override
public void close() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not something you changed but I think we can remove this override?

Copy link
Contributor Author

@albertzaharovits albertzaharovits Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's needed to implement Closeable#close.

}

public boolean doesContainerExist()
{
return this.client.doesContainerExist(this.clientName, this.locMode, container);
public boolean doesContainerExist() {
logger.trace("containerExists({})", container);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this trace logging necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I have removed it. Trace logging statements have been sprinkled without much restrain. Surely some nasty bug fighting went on here 🐛

try {
return service.doesContainerExist(clientName, container);
} catch (URISyntaxException | StorageException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every other method throws these exceptions, is there a reason why this one cannot also throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adjusted it to also throw exception.

I guess the original reasoning was that boolean exists should swallow the exception and return false, making code simpler and presumably because if indeed there is a connection problem the exception will be rethrown later, when trying the create/access associated operation. Yet, this is not consistent across the plugin code, and your suggestion is indeed in tone with the surrounding.

logger.warn("cannot access container {{}}: {}", container, e.getMessage());
}
return false;
}

public boolean blobExists(String blob) throws URISyntaxException, StorageException
{
return this.client.blobExists(this.clientName, this.locMode, container, blob);
public boolean blobExists(String blob) throws URISyntaxException, StorageException {
return service.blobExists(clientName, container, blob);
}

public void deleteBlob(String blob) throws URISyntaxException, StorageException
{
this.client.deleteBlob(this.clientName, this.locMode, container, blob);
public void deleteBlob(String blob) throws URISyntaxException, StorageException {
service.deleteBlob(clientName, container, blob);
}

public InputStream getInputStream(String blob) throws URISyntaxException, StorageException, IOException
{
return this.client.getInputStream(this.clientName, this.locMode, container, blob);
public InputStream getInputStream(String blob) throws URISyntaxException, StorageException, IOException {
return service.getInputStream(clientName, container, blob);
}

public Map<String,BlobMetaData> listBlobsByPrefix(String keyPath, String prefix)
throws URISyntaxException, StorageException {
return this.client.listBlobsByPrefix(this.clientName, this.locMode, container, keyPath, prefix);
return service.listBlobsByPrefix(clientName, container, keyPath, prefix);
}

public void moveBlob(String sourceBlob, String targetBlob) throws URISyntaxException, StorageException
{
this.client.moveBlob(this.clientName, this.locMode, container, sourceBlob, targetBlob);
public void moveBlob(String sourceBlob, String targetBlob) throws URISyntaxException, StorageException {
service.moveBlob(clientName, container, sourceBlob, targetBlob);
}

public void writeBlob(String blobName, InputStream inputStream, long blobSize) throws URISyntaxException, StorageException {
this.client.writeBlob(this.clientName, this.locMode, container, blobName, inputStream, blobSize);
service.writeBlob(clientName, container, blobName, inputStream, blobSize);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import com.microsoft.azure.storage.LocationMode;
import com.microsoft.azure.storage.StorageException;

import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.cluster.metadata.RepositoryMetaData;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -60,19 +62,19 @@ public class AzureRepository extends BlobStoreRepository {
public static final String TYPE = "azure";

public static final class Repository {

@Deprecated // Replaced by client
public static final Setting<String> ACCOUNT_SETTING = new Setting<>("account", "default", Function.identity(),
Property.NodeScope, Property.Deprecated);
public static final Setting<String> CLIENT_NAME = new Setting<>("client", ACCOUNT_SETTING, Function.identity());

public static final Setting<String> CONTAINER_SETTING =
new Setting<>("container", "elasticsearch-snapshots", Function.identity(), Property.NodeScope);
public static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path", Property.NodeScope);
public static final Setting<String> LOCATION_MODE_SETTING = Setting.simpleString("location_mode", Property.NodeScope);
public static final Setting<LocationMode> LOCATION_MODE_SETTING = new Setting<>("location_mode",
s -> LocationMode.PRIMARY_ONLY.toString(), s -> LocationMode.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope);
public static final Setting<ByteSizeValue> CHUNK_SIZE_SETTING =
Setting.byteSizeSetting("chunk_size", MAX_CHUNK_SIZE, MIN_CHUNK_SIZE, MAX_CHUNK_SIZE, Property.NodeScope);
public static final Setting<Boolean> COMPRESS_SETTING = Setting.boolSetting("compress", false, Property.NodeScope);
public static final Setting<Boolean> READONLY_SETTING = Setting.boolSetting("readonly", false, Property.NodeScope);
}

private final AzureBlobStore blobStore;
Expand All @@ -81,45 +83,32 @@ public static final class Repository {
private final boolean compress;
private final boolean readonly;

public AzureRepository(RepositoryMetaData metadata, Environment environment,
NamedXContentRegistry namedXContentRegistry, AzureStorageService storageService)
throws IOException, URISyntaxException, StorageException {
public AzureRepository(RepositoryMetaData metadata, Environment environment, NamedXContentRegistry namedXContentRegistry,
AzureStorageService storageService) throws IOException, URISyntaxException, StorageException {
super(metadata, environment.settings(), namedXContentRegistry);

blobStore = new AzureBlobStore(metadata, environment.settings(), storageService);
String container = Repository.CONTAINER_SETTING.get(metadata.settings());
this.blobStore = new AzureBlobStore(metadata, environment.settings(), storageService);
this.chunkSize = Repository.CHUNK_SIZE_SETTING.get(metadata.settings());
this.compress = Repository.COMPRESS_SETTING.get(metadata.settings());
String modeStr = Repository.LOCATION_MODE_SETTING.get(metadata.settings());
Boolean forcedReadonly = metadata.settings().getAsBoolean("readonly", null);
// If the user explicitly did not define a readonly value, we set it by ourselves depending on the location mode setting.
// For secondary_only setting, the repository should be read only
if (forcedReadonly == null) {
if (Strings.hasLength(modeStr)) {
LocationMode locationMode = LocationMode.valueOf(modeStr.toUpperCase(Locale.ROOT));
this.readonly = locationMode == LocationMode.SECONDARY_ONLY;
} else {
this.readonly = false;
}
if (Repository.READONLY_SETTING.exists(metadata.settings())) {
this.readonly = Repository.READONLY_SETTING.get(metadata.settings());
} else {
readonly = forcedReadonly;
this.readonly = this.blobStore.getLocationMode() == LocationMode.SECONDARY_ONLY;
}

String basePath = Repository.BASE_PATH_SETTING.get(metadata.settings());

final String basePath = Strings.trimLeadingCharacter(Repository.BASE_PATH_SETTING.get(metadata.settings()), '/');
if (Strings.hasLength(basePath)) {
// Remove starting / if any
basePath = Strings.trimLeadingCharacter(basePath, '/');
BlobPath path = new BlobPath();
for(String elem : basePath.split("/")) {
for(final String elem : basePath.split("/")) {
path = path.add(elem);
}
this.basePath = path;
} else {
this.basePath = BlobPath.cleanPath();
}
logger.debug("using container [{}], chunk_size [{}], compress [{}], base_path [{}]",
container, chunkSize, compress, basePath);
logger.debug((org.apache.logging.log4j.util.Supplier<?>) () -> new ParameterizedMessage(
"using container [{}], chunk_size [{}], compress [{}], base_path [{}]", blobStore, chunkSize, compress, basePath));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.env.Environment;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.ReInitializablePlugin;
import org.elasticsearch.plugins.RepositoryPlugin;
import org.elasticsearch.repositories.Repository;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand All @@ -35,24 +35,20 @@
/**
* A plugin to add a repository type that writes to and from the Azure cloud storage service.
*/
public class AzureRepositoryPlugin extends Plugin implements RepositoryPlugin {

private final Map<String, AzureStorageSettings> clientsSettings;
public class AzureRepositoryPlugin extends Plugin implements RepositoryPlugin, ReInitializablePlugin {

// overridable for tests
protected AzureStorageService createStorageService(Settings settings) {
return new AzureStorageServiceImpl(settings, clientsSettings);
}
// protected for testing
final AzureStorageService azureStoreService;

public AzureRepositoryPlugin(Settings settings) {
// eagerly load client settings so that secure settings are read
clientsSettings = AzureStorageSettings.load(settings);
this.azureStoreService = new AzureStorageServiceImpl(settings);
}

@Override
public Map<String, Repository.Factory> getRepositories(Environment env, NamedXContentRegistry namedXContentRegistry) {
return Collections.singletonMap(AzureRepository.TYPE,
(metadata) -> new AzureRepository(metadata, env, namedXContentRegistry, createStorageService(env.settings())));
(metadata) -> new AzureRepository(metadata, env, namedXContentRegistry, azureStoreService));
}

@Override
Expand All @@ -67,4 +63,16 @@ public List<Setting<?>> getSettings() {
AzureStorageSettings.PROXY_PORT_SETTING
);
}

@Override
public boolean reinit(Settings settings) {
// secure settings should be readable
final Map<String, AzureStorageSettings> clientsSettings = AzureStorageSettings.load(settings);
if (clientsSettings.isEmpty()) {
throw new IllegalArgumentException("If you want to use an azure repository, you need to define a client configuration.");
}
assert clientsSettings.containsKey("default") : "always have 'default'";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this an exception instead of an assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the check as suggested in the below comment, but kept the assert there since it would truly be a code problem instead of an external one.

azureStoreService.updateClientsSettings(clientsSettings);
return true;
}
}

This file was deleted.

This file was deleted.

Loading