-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Changes from 9 commits
ef2a7de
f4283be
deb79ce
478100b
014f2ef
9cbd24f
247c5f7
a9f8cdd
16b5a63
97fc266
4d2694f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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() { | ||
} | ||
|
||
public boolean doesContainerExist() | ||
{ | ||
return this.client.doesContainerExist(this.clientName, this.locMode, container); | ||
public boolean doesContainerExist() { | ||
logger.trace("containerExists({})", container); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this trace logging necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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'"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this an exception instead of an assert? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.