From 40b5187f4868d7314b61c170083ae6acfe1925d6 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 29 May 2018 13:24:30 +0200 Subject: [PATCH] Remove log traces in AzureStorageServiceImpl and fix test This commit removes some log traces in AzureStorageServiceImpl and also fixes the AzureStorageServiceTests so that is uses the real implementation to create Azure clients. --- .../azure/AzureStorageServiceImpl.java | 69 ++++++------- .../azure/AzureStorageServiceTests.java | 98 +++++++++---------- 2 files changed, 76 insertions(+), 91 deletions(-) diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java index f21dbdfd269f4..6f4f8cfea9609 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java @@ -35,6 +35,7 @@ import com.microsoft.azure.storage.blob.ListBlobItem; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.blobstore.BlobMetaData; import org.elasticsearch.common.blobstore.support.PlainBlobMetaData; import org.elasticsearch.common.collect.MapBuilder; @@ -45,6 +46,7 @@ import java.io.InputStream; import java.net.URI; import java.net.URISyntaxException; +import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; import java.util.Map; @@ -52,66 +54,59 @@ public class AzureStorageServiceImpl extends AbstractComponent implements AzureStorageService { final Map storageSettings; - - final Map clients = new HashMap<>(); + final Map clients; public AzureStorageServiceImpl(Settings settings, Map storageSettings) { super(settings); - - this.storageSettings = storageSettings; - if (storageSettings.isEmpty()) { // If someone did not register any settings, they basically can't use the plugin throw new IllegalArgumentException("If you want to use an azure repository, you need to define a client configuration."); } - - logger.debug("starting azure storage client instance"); - - // We register all regular azure clients - for (Map.Entry azureStorageSettingsEntry : this.storageSettings.entrySet()) { - logger.debug("registering regular client for account [{}]", azureStorageSettingsEntry.getKey()); - createClient(azureStorageSettingsEntry.getValue()); - } + this.storageSettings = storageSettings; + this.clients = createClients(storageSettings); } - void createClient(AzureStorageSettings azureStorageSettings) { - try { - logger.trace("creating new Azure storage client using account [{}], key [{}], endpoint suffix [{}]", - azureStorageSettings.getAccount(), azureStorageSettings.getKey(), azureStorageSettings.getEndpointSuffix()); + private Map createClients(final Map storageSettings) { + final Map clients = new HashMap<>(); + for (Map.Entry azureStorageEntry : storageSettings.entrySet()) { + final String clientName = azureStorageEntry.getKey(); + final AzureStorageSettings clientSettings = azureStorageEntry.getValue(); + try { + logger.trace("creating new Azure storage client with name [{}]", clientName); + String storageConnectionString = + "DefaultEndpointsProtocol=https;" + + "AccountName=" + clientSettings.getAccount() + ";" + + "AccountKey=" + clientSettings.getKey(); + + final String endpointSuffix = clientSettings.getEndpointSuffix(); + if (Strings.hasLength(endpointSuffix)) { + storageConnectionString += ";EndpointSuffix=" + endpointSuffix; + } + // Retrieve storage account from connection-string. + CloudStorageAccount storageAccount = CloudStorageAccount.parse(storageConnectionString); - String storageConnectionString = - "DefaultEndpointsProtocol=https;" - + "AccountName=" + azureStorageSettings.getAccount() + ";" - + "AccountKey=" + azureStorageSettings.getKey(); + // Create the blob client. + CloudBlobClient client = storageAccount.createCloudBlobClient(); - String endpointSuffix = azureStorageSettings.getEndpointSuffix(); - if (endpointSuffix != null && !endpointSuffix.isEmpty()) { - storageConnectionString += ";EndpointSuffix=" + endpointSuffix; + // Register the client + clients.put(clientSettings.getAccount(), client); + } catch (Exception e) { + logger.error(() -> new ParameterizedMessage("Can not create azure storage client [{}]", clientName), e); } - // Retrieve storage account from connection-string. - CloudStorageAccount storageAccount = CloudStorageAccount.parse(storageConnectionString); - - // Create the blob client. - CloudBlobClient client = storageAccount.createCloudBlobClient(); - - // Register the client - this.clients.put(azureStorageSettings.getAccount(), client); - } catch (Exception e) { - logger.error("can not create azure storage client: {}", e.getMessage()); } + return Collections.unmodifiableMap(clients); } CloudBlobClient getSelectedClient(String clientName, LocationMode mode) { logger.trace("selecting a client named [{}], mode [{}]", clientName, mode.name()); AzureStorageSettings azureStorageSettings = this.storageSettings.get(clientName); if (azureStorageSettings == null) { - throw new IllegalArgumentException("Can not find named azure client [" + clientName + "]. Check your settings."); + throw new IllegalArgumentException("Unable to find client with name [" + clientName + "]"); } CloudBlobClient client = this.clients.get(azureStorageSettings.getAccount()); - if (client == null) { - throw new IllegalArgumentException("Can not find an azure client named [" + azureStorageSettings.getAccount() + "]"); + throw new IllegalArgumentException("No account defined for client with name [" + clientName + "]"); } // NOTE: for now, just set the location mode in case it is different; diff --git a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureStorageServiceTests.java b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureStorageServiceTests.java index 72cd015f14847..447826dbf833f 100644 --- a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureStorageServiceTests.java +++ b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureStorageServiceTests.java @@ -23,7 +23,6 @@ import com.microsoft.azure.storage.RetryExponentialRetry; import com.microsoft.azure.storage.blob.CloudBlobClient; import com.microsoft.azure.storage.core.Base64; - import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; @@ -36,6 +35,7 @@ import java.net.URISyntaxException; import java.net.UnknownHostException; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.Map; import static org.elasticsearch.repositories.azure.AzureStorageServiceImpl.blobNameFromUri; @@ -49,31 +49,14 @@ public class AzureStorageServiceTests extends ESTestCase { - private MockSecureSettings buildSecureSettings() { - MockSecureSettings secureSettings = new MockSecureSettings(); - secureSettings.setString("azure.client.azure1.account", "myaccount1"); - secureSettings.setString("azure.client.azure1.key", "mykey1"); - secureSettings.setString("azure.client.azure2.account", "myaccount2"); - secureSettings.setString("azure.client.azure2.key", "mykey2"); - secureSettings.setString("azure.client.azure3.account", "myaccount3"); - secureSettings.setString("azure.client.azure3.key", "mykey3"); - return secureSettings; - } - private Settings buildSettings() { - Settings settings = Settings.builder() - .setSecureSettings(buildSecureSettings()) - .build(); - return settings; - } - public void testReadSecuredSettings() { MockSecureSettings secureSettings = new MockSecureSettings(); secureSettings.setString("azure.client.azure1.account", "myaccount1"); - secureSettings.setString("azure.client.azure1.key", "mykey1"); + secureSettings.setString("azure.client.azure1.key", encodeKey("mykey1")); secureSettings.setString("azure.client.azure2.account", "myaccount2"); - secureSettings.setString("azure.client.azure2.key", "mykey2"); + secureSettings.setString("azure.client.azure2.key", encodeKey("mykey2")); secureSettings.setString("azure.client.azure3.account", "myaccount3"); - secureSettings.setString("azure.client.azure3.key", "mykey3"); + secureSettings.setString("azure.client.azure3.key", encodeKey("mykey3")); Settings settings = Settings.builder().setSecureSettings(secureSettings) .put("azure.client.azure3.endpoint_suffix", "my_endpoint_suffix").build(); @@ -88,9 +71,9 @@ public void testReadSecuredSettings() { public void testCreateClientWithEndpointSuffix() { MockSecureSettings secureSettings = new MockSecureSettings(); secureSettings.setString("azure.client.azure1.account", "myaccount1"); - secureSettings.setString("azure.client.azure1.key", Base64.encode("mykey1".getBytes(StandardCharsets.UTF_8))); + secureSettings.setString("azure.client.azure1.key", encodeKey("mykey1")); secureSettings.setString("azure.client.azure2.account", "myaccount2"); - secureSettings.setString("azure.client.azure2.key", Base64.encode("mykey2".getBytes(StandardCharsets.UTF_8))); + secureSettings.setString("azure.client.azure2.key", encodeKey("mykey2")); Settings settings = Settings.builder().setSecureSettings(secureSettings) .put("azure.client.azure1.endpoint_suffix", "my_endpoint_suffix").build(); AzureStorageServiceImpl azureStorageService = new AzureStorageServiceImpl(settings, AzureStorageSettings.load(settings)); @@ -103,7 +86,7 @@ public void testCreateClientWithEndpointSuffix() { public void testGetSelectedClientWithNoPrimaryAndSecondary() { try { - new AzureStorageServiceMockForSettings(Settings.EMPTY); + new AzureStorageServiceImpl(Settings.EMPTY, Collections.emptyMap()); fail("we should have raised an IllegalArgumentException"); } catch (IllegalArgumentException e) { assertThat(e.getMessage(), is("If you want to use an azure repository, you need to define a client configuration.")); @@ -111,11 +94,11 @@ public void testGetSelectedClientWithNoPrimaryAndSecondary() { } public void testGetSelectedClientNonExisting() { - AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(buildSettings()); + AzureStorageServiceImpl azureStorageService = createAzureService(buildSettings()); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { azureStorageService.getSelectedClient("azure4", LocationMode.PRIMARY_ONLY); }); - assertThat(e.getMessage(), is("Can not find named azure client [azure4]. Check your settings.")); + assertThat(e.getMessage(), is("Unable to find client with name [azure4]")); } public void testGetSelectedClientDefaultTimeout() { @@ -123,7 +106,7 @@ public void testGetSelectedClientDefaultTimeout() { .setSecureSettings(buildSecureSettings()) .put("azure.client.azure3.timeout", "30s") .build(); - AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(timeoutSettings); + AzureStorageServiceImpl azureStorageService = createAzureService(timeoutSettings); CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY); assertThat(client1.getDefaultRequestOptions().getTimeoutIntervalInMs(), nullValue()); CloudBlobClient client3 = azureStorageService.getSelectedClient("azure3", LocationMode.PRIMARY_ONLY); @@ -131,13 +114,13 @@ public void testGetSelectedClientDefaultTimeout() { } public void testGetSelectedClientNoTimeout() { - AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(buildSettings()); + AzureStorageServiceImpl azureStorageService = createAzureService(buildSettings()); CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY); assertThat(client1.getDefaultRequestOptions().getTimeoutIntervalInMs(), is(nullValue())); } public void testGetSelectedClientBackoffPolicy() { - AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(buildSettings()); + AzureStorageServiceImpl azureStorageService = createAzureService(buildSettings()); CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY); assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), is(notNullValue())); assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), instanceOf(RetryExponentialRetry.class)); @@ -149,7 +132,7 @@ public void testGetSelectedClientBackoffPolicyNbRetries() { .put("azure.client.azure1.max_retries", 7) .build(); - AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(timeoutSettings); + AzureStorageServiceImpl azureStorageService = createAzureService(timeoutSettings); CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY); assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), is(notNullValue())); assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), instanceOf(RetryExponentialRetry.class)); @@ -159,7 +142,7 @@ public void testNoProxy() { Settings settings = Settings.builder() .setSecureSettings(buildSecureSettings()) .build(); - AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings); + AzureStorageServiceImpl mock = createAzureService(settings); assertThat(mock.storageSettings.get("azure1").getProxy(), nullValue()); assertThat(mock.storageSettings.get("azure2").getProxy(), nullValue()); assertThat(mock.storageSettings.get("azure3").getProxy(), nullValue()); @@ -172,7 +155,7 @@ public void testProxyHttp() throws UnknownHostException { .put("azure.client.azure1.proxy.port", 8080) .put("azure.client.azure1.proxy.type", "http") .build(); - AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings); + AzureStorageServiceImpl mock = createAzureService(settings); Proxy azure1Proxy = mock.storageSettings.get("azure1").getProxy(); assertThat(azure1Proxy, notNullValue()); @@ -192,7 +175,7 @@ public void testMultipleProxies() throws UnknownHostException { .put("azure.client.azure2.proxy.port", 8081) .put("azure.client.azure2.proxy.type", "http") .build(); - AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings); + AzureStorageServiceImpl mock = createAzureService(settings); Proxy azure1Proxy = mock.storageSettings.get("azure1").getProxy(); assertThat(azure1Proxy, notNullValue()); assertThat(azure1Proxy.type(), is(Proxy.Type.HTTP)); @@ -211,7 +194,7 @@ public void testProxySocks() throws UnknownHostException { .put("azure.client.azure1.proxy.port", 8080) .put("azure.client.azure1.proxy.type", "socks") .build(); - AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings); + AzureStorageServiceImpl mock = createAzureService(settings); Proxy azure1Proxy = mock.storageSettings.get("azure1").getProxy(); assertThat(azure1Proxy, notNullValue()); assertThat(azure1Proxy.type(), is(Proxy.Type.SOCKS)); @@ -227,7 +210,7 @@ public void testProxyNoHost() { .put("azure.client.azure1.proxy.type", randomFrom("socks", "http")) .build(); - SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings)); + SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings)); assertEquals("Azure Proxy type has been set but proxy host or port is not defined.", e.getMessage()); } @@ -238,7 +221,7 @@ public void testProxyNoPort() { .put("azure.client.azure1.proxy.type", randomFrom("socks", "http")) .build(); - SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings)); + SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings)); assertEquals("Azure Proxy type has been set but proxy host or port is not defined.", e.getMessage()); } @@ -249,7 +232,7 @@ public void testProxyNoType() { .put("azure.client.azure1.proxy.port", 8080) .build(); - SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings)); + SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings)); assertEquals("Azure Proxy port or host have been set but proxy type is not defined.", e.getMessage()); } @@ -261,26 +244,10 @@ public void testProxyWrongHost() { .put("azure.client.azure1.proxy.port", 8080) .build(); - SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings)); + SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings)); assertEquals("Azure proxy host is unknown.", e.getMessage()); } - /** - * This internal class just overload createClient method which is called by AzureStorageServiceImpl.doStart() - */ - class AzureStorageServiceMockForSettings extends AzureStorageServiceImpl { - AzureStorageServiceMockForSettings(Settings settings) { - super(settings, AzureStorageSettings.load(settings)); - } - - // We fake the client here - @Override - void createClient(AzureStorageSettings azureStorageSettings) { - this.clients.put(azureStorageSettings.getAccount(), - new CloudBlobClient(URI.create("https://" + azureStorageSettings.getAccount()))); - } - } - public void testBlobNameFromUri() throws URISyntaxException { String name = blobNameFromUri(new URI("https://myservice.azure.net/container/path/to/myfile")); assertThat(name, is("path/to/myfile")); @@ -291,4 +258,27 @@ public void testBlobNameFromUri() throws URISyntaxException { name = blobNameFromUri(new URI("https://127.0.0.1/container/path/to/myfile")); assertThat(name, is("path/to/myfile")); } + + private static MockSecureSettings buildSecureSettings() { + MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString("azure.client.azure1.account", "myaccount1"); + secureSettings.setString("azure.client.azure1.key", encodeKey("mykey1")); + secureSettings.setString("azure.client.azure2.account", "myaccount2"); + secureSettings.setString("azure.client.azure2.key", encodeKey("mykey2")); + secureSettings.setString("azure.client.azure3.account", "myaccount3"); + secureSettings.setString("azure.client.azure3.key", encodeKey("mykey3")); + return secureSettings; + } + + private static Settings buildSettings() { + return Settings.builder().setSecureSettings(buildSecureSettings()).build(); + } + + private static AzureStorageServiceImpl createAzureService(final Settings settings) { + return new AzureStorageServiceImpl(settings, AzureStorageSettings.load(settings)); + } + + private static String encodeKey(final String value) { + return Base64.encode(value.getBytes(StandardCharsets.UTF_8)); + } }