Remove log traces in AzureStorageServiceImpl and fix test (#30924)

This commit removes some log traces in AzureStorageServiceImpl and also
fixes the AzureStorageServiceTests so that is uses the real
implementation to create Azure clients.

plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageServiceImpl.java

@@ -35,6 +35,7 @@ import com.microsoft.azure.storage.blob.DeleteSnapshotsOption;
 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 org.elasticsearch.repositories.RepositoryException;
 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 @@ import java.util.Map;
 public class AzureStorageServiceImpl extends AbstractComponent implements AzureStorageService {
 
     final Map<String, AzureStorageSettings> storageSettings;
-
-    final Map<String, CloudBlobClient> clients = new HashMap<>();
+    final Map<String, CloudBlobClient> clients;
 
     public AzureStorageServiceImpl(Settings settings, Map<String, AzureStorageSettings> 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<String, AzureStorageSettings> 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<String, CloudBlobClient> createClients(final Map<String, AzureStorageSettings> storageSettings) {
+        final Map<String, CloudBlobClient> clients = new HashMap<>();
+        for (Map.Entry<String, AzureStorageSettings> 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;

plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureStorageServiceTests.java

@@ -23,7 +23,6 @@ import com.microsoft.azure.storage.LocationMode;
 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.URI;
 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 @@ import static org.hamcrest.Matchers.nullValue;
 
 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 class AzureStorageServiceTests extends ESTestCase {
     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 class AzureStorageServiceTests extends ESTestCase {
 
     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 class AzureStorageServiceTests extends ESTestCase {
     }
 
     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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
     }
 
     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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
         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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
             .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 class AzureStorageServiceTests extends ESTestCase {
         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));
+    }
 }