Skip to content

Commit bfa784e

Browse files
tlrxtlrx
authored
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.
1 parent 3c918d7 commit bfa784e

File tree

2 files changed

+76
-91
lines changed

2 files changed

+76
-91
lines changed

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

Lines changed: 32 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.microsoft.azure.storage.blob.ListBlobItem;
3636
import org.apache.logging.log4j.message.ParameterizedMessage;
3737
import org.apache.logging.log4j.util.Supplier;
38+
import org.elasticsearch.common.Strings;
3839
import org.elasticsearch.common.blobstore.BlobMetaData;
3940
import org.elasticsearch.common.blobstore.support.PlainBlobMetaData;
4041
import org.elasticsearch.common.collect.MapBuilder;
@@ -45,73 +46,67 @@
4546
import java.io.InputStream;
4647
import java.net.URI;
4748
import java.net.URISyntaxException;
49+
import java.util.Collections;
4850
import java.util.EnumSet;
4951
import java.util.HashMap;
5052
import java.util.Map;
5153

5254
public class AzureStorageServiceImpl extends AbstractComponent implements AzureStorageService {
5355

5456
final Map<String, AzureStorageSettings> storageSettings;
55-
56-
final Map<String, CloudBlobClient> clients = new HashMap<>();
57+
final Map<String, CloudBlobClient> clients;
5758

5859
public AzureStorageServiceImpl(Settings settings, Map<String, AzureStorageSettings> storageSettings) {
5960
super(settings);
60-
61-
this.storageSettings = storageSettings;
62-
6361
if (storageSettings.isEmpty()) {
6462
// If someone did not register any settings, they basically can't use the plugin
6563
throw new IllegalArgumentException("If you want to use an azure repository, you need to define a client configuration.");
6664
}
67-
68-
logger.debug("starting azure storage client instance");
69-
70-
// We register all regular azure clients
71-
for (Map.Entry<String, AzureStorageSettings> azureStorageSettingsEntry : this.storageSettings.entrySet()) {
72-
logger.debug("registering regular client for account [{}]", azureStorageSettingsEntry.getKey());
73-
createClient(azureStorageSettingsEntry.getValue());
74-
}
65+
this.storageSettings = storageSettings;
66+
this.clients = createClients(storageSettings);
7567
}
7668

77-
void createClient(AzureStorageSettings azureStorageSettings) {
78-
try {
79-
logger.trace("creating new Azure storage client using account [{}], key [{}], endpoint suffix [{}]",
80-
azureStorageSettings.getAccount(), azureStorageSettings.getKey(), azureStorageSettings.getEndpointSuffix());
69+
private Map<String, CloudBlobClient> createClients(final Map<String, AzureStorageSettings> storageSettings) {
70+
final Map<String, CloudBlobClient> clients = new HashMap<>();
71+
for (Map.Entry<String, AzureStorageSettings> azureStorageEntry : storageSettings.entrySet()) {
72+
final String clientName = azureStorageEntry.getKey();
73+
final AzureStorageSettings clientSettings = azureStorageEntry.getValue();
74+
try {
75+
logger.trace("creating new Azure storage client with name [{}]", clientName);
76+
String storageConnectionString =
77+
"DefaultEndpointsProtocol=https;"
78+
+ "AccountName=" + clientSettings.getAccount() + ";"
79+
+ "AccountKey=" + clientSettings.getKey();
80+
81+
final String endpointSuffix = clientSettings.getEndpointSuffix();
82+
if (Strings.hasLength(endpointSuffix)) {
83+
storageConnectionString += ";EndpointSuffix=" + endpointSuffix;
84+
}
85+
// Retrieve storage account from connection-string.
86+
CloudStorageAccount storageAccount = CloudStorageAccount.parse(storageConnectionString);
8187

82-
String storageConnectionString =
83-
"DefaultEndpointsProtocol=https;"
84-
+ "AccountName=" + azureStorageSettings.getAccount() + ";"
85-
+ "AccountKey=" + azureStorageSettings.getKey();
88+
// Create the blob client.
89+
CloudBlobClient client = storageAccount.createCloudBlobClient();
8690

87-
String endpointSuffix = azureStorageSettings.getEndpointSuffix();
88-
if (endpointSuffix != null && !endpointSuffix.isEmpty()) {
89-
storageConnectionString += ";EndpointSuffix=" + endpointSuffix;
91+
// Register the client
92+
clients.put(clientSettings.getAccount(), client);
93+
} catch (Exception e) {
94+
logger.error(() -> new ParameterizedMessage("Can not create azure storage client [{}]", clientName), e);
9095
}
91-
// Retrieve storage account from connection-string.
92-
CloudStorageAccount storageAccount = CloudStorageAccount.parse(storageConnectionString);
93-
94-
// Create the blob client.
95-
CloudBlobClient client = storageAccount.createCloudBlobClient();
96-
97-
// Register the client
98-
this.clients.put(azureStorageSettings.getAccount(), client);
99-
} catch (Exception e) {
100-
logger.error("can not create azure storage client: {}", e.getMessage());
10196
}
97+
return Collections.unmodifiableMap(clients);
10298
}
10399

104100
CloudBlobClient getSelectedClient(String clientName, LocationMode mode) {
105101
logger.trace("selecting a client named [{}], mode [{}]", clientName, mode.name());
106102
AzureStorageSettings azureStorageSettings = this.storageSettings.get(clientName);
107103
if (azureStorageSettings == null) {
108-
throw new IllegalArgumentException("Can not find named azure client [" + clientName + "]. Check your settings.");
104+
throw new IllegalArgumentException("Unable to find client with name [" + clientName + "]");
109105
}
110106

111107
CloudBlobClient client = this.clients.get(azureStorageSettings.getAccount());
112-
113108
if (client == null) {
114-
throw new IllegalArgumentException("Can not find an azure client named [" + azureStorageSettings.getAccount() + "]");
109+
throw new IllegalArgumentException("No account defined for client with name [" + clientName + "]");
115110
}
116111

117112
// 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‎

Lines changed: 44 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import com.microsoft.azure.storage.RetryExponentialRetry;
2424
import com.microsoft.azure.storage.blob.CloudBlobClient;
2525
import com.microsoft.azure.storage.core.Base64;
26-
2726
import org.elasticsearch.common.settings.MockSecureSettings;
2827
import org.elasticsearch.common.settings.Settings;
2928
import org.elasticsearch.common.settings.SettingsException;
@@ -36,6 +35,7 @@
3635
import java.net.URISyntaxException;
3736
import java.net.UnknownHostException;
3837
import java.nio.charset.StandardCharsets;
38+
import java.util.Collections;
3939
import java.util.Map;
4040

4141
import static org.elasticsearch.repositories.azure.AzureStorageServiceImpl.blobNameFromUri;
@@ -49,31 +49,14 @@
4949

5050
public class AzureStorageServiceTests extends ESTestCase {
5151

52-
private MockSecureSettings buildSecureSettings() {
53-
MockSecureSettings secureSettings = new MockSecureSettings();
54-
secureSettings.setString("azure.client.azure1.account", "myaccount1");
55-
secureSettings.setString("azure.client.azure1.key", "mykey1");
56-
secureSettings.setString("azure.client.azure2.account", "myaccount2");
57-
secureSettings.setString("azure.client.azure2.key", "mykey2");
58-
secureSettings.setString("azure.client.azure3.account", "myaccount3");
59-
secureSettings.setString("azure.client.azure3.key", "mykey3");
60-
return secureSettings;
61-
}
62-
private Settings buildSettings() {
63-
Settings settings = Settings.builder()
64-
.setSecureSettings(buildSecureSettings())
65-
.build();
66-
return settings;
67-
}
68-
6952
public void testReadSecuredSettings() {
7053
MockSecureSettings secureSettings = new MockSecureSettings();
7154
secureSettings.setString("azure.client.azure1.account", "myaccount1");
72-
secureSettings.setString("azure.client.azure1.key", "mykey1");
55+
secureSettings.setString("azure.client.azure1.key", encodeKey("mykey1"));
7356
secureSettings.setString("azure.client.azure2.account", "myaccount2");
74-
secureSettings.setString("azure.client.azure2.key", "mykey2");
57+
secureSettings.setString("azure.client.azure2.key", encodeKey("mykey2"));
7558
secureSettings.setString("azure.client.azure3.account", "myaccount3");
76-
secureSettings.setString("azure.client.azure3.key", "mykey3");
59+
secureSettings.setString("azure.client.azure3.key", encodeKey("mykey3"));
7760
Settings settings = Settings.builder().setSecureSettings(secureSettings)
7861
.put("azure.client.azure3.endpoint_suffix", "my_endpoint_suffix").build();
7962

@@ -88,9 +71,9 @@ public void testReadSecuredSettings() {
8871
public void testCreateClientWithEndpointSuffix() {
8972
MockSecureSettings secureSettings = new MockSecureSettings();
9073
secureSettings.setString("azure.client.azure1.account", "myaccount1");
91-
secureSettings.setString("azure.client.azure1.key", Base64.encode("mykey1".getBytes(StandardCharsets.UTF_8)));
74+
secureSettings.setString("azure.client.azure1.key", encodeKey("mykey1"));
9275
secureSettings.setString("azure.client.azure2.account", "myaccount2");
93-
secureSettings.setString("azure.client.azure2.key", Base64.encode("mykey2".getBytes(StandardCharsets.UTF_8)));
76+
secureSettings.setString("azure.client.azure2.key", encodeKey("mykey2"));
9477
Settings settings = Settings.builder().setSecureSettings(secureSettings)
9578
.put("azure.client.azure1.endpoint_suffix", "my_endpoint_suffix").build();
9679
AzureStorageServiceImpl azureStorageService = new AzureStorageServiceImpl(settings, AzureStorageSettings.load(settings));
@@ -103,41 +86,41 @@ public void testCreateClientWithEndpointSuffix() {
10386

10487
public void testGetSelectedClientWithNoPrimaryAndSecondary() {
10588
try {
106-
new AzureStorageServiceMockForSettings(Settings.EMPTY);
89+
new AzureStorageServiceImpl(Settings.EMPTY, Collections.emptyMap());
10790
fail("we should have raised an IllegalArgumentException");
10891
} catch (IllegalArgumentException e) {
10992
assertThat(e.getMessage(), is("If you want to use an azure repository, you need to define a client configuration."));
11093
}
11194
}
11295

11396
public void testGetSelectedClientNonExisting() {
114-
AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(buildSettings());
97+
AzureStorageServiceImpl azureStorageService = createAzureService(buildSettings());
11598
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
11699
azureStorageService.getSelectedClient("azure4", LocationMode.PRIMARY_ONLY);
117100
});
118-
assertThat(e.getMessage(), is("Can not find named azure client [azure4]. Check your settings."));
101+
assertThat(e.getMessage(), is("Unable to find client with name [azure4]"));
119102
}
120103

121104
public void testGetSelectedClientDefaultTimeout() {
122105
Settings timeoutSettings = Settings.builder()
123106
.setSecureSettings(buildSecureSettings())
124107
.put("azure.client.azure3.timeout", "30s")
125108
.build();
126-
AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(timeoutSettings);
109+
AzureStorageServiceImpl azureStorageService = createAzureService(timeoutSettings);
127110
CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY);
128111
assertThat(client1.getDefaultRequestOptions().getTimeoutIntervalInMs(), nullValue());
129112
CloudBlobClient client3 = azureStorageService.getSelectedClient("azure3", LocationMode.PRIMARY_ONLY);
130113
assertThat(client3.getDefaultRequestOptions().getTimeoutIntervalInMs(), is(30 * 1000));
131114
}
132115

133116
public void testGetSelectedClientNoTimeout() {
134-
AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(buildSettings());
117+
AzureStorageServiceImpl azureStorageService = createAzureService(buildSettings());
135118
CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY);
136119
assertThat(client1.getDefaultRequestOptions().getTimeoutIntervalInMs(), is(nullValue()));
137120
}
138121

139122
public void testGetSelectedClientBackoffPolicy() {
140-
AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(buildSettings());
123+
AzureStorageServiceImpl azureStorageService = createAzureService(buildSettings());
141124
CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY);
142125
assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), is(notNullValue()));
143126
assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), instanceOf(RetryExponentialRetry.class));
@@ -149,7 +132,7 @@ public void testGetSelectedClientBackoffPolicyNbRetries() {
149132
.put("azure.client.azure1.max_retries", 7)
150133
.build();
151134

152-
AzureStorageServiceImpl azureStorageService = new AzureStorageServiceMockForSettings(timeoutSettings);
135+
AzureStorageServiceImpl azureStorageService = createAzureService(timeoutSettings);
153136
CloudBlobClient client1 = azureStorageService.getSelectedClient("azure1", LocationMode.PRIMARY_ONLY);
154137
assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), is(notNullValue()));
155138
assertThat(client1.getDefaultRequestOptions().getRetryPolicyFactory(), instanceOf(RetryExponentialRetry.class));
@@ -159,7 +142,7 @@ public void testNoProxy() {
159142
Settings settings = Settings.builder()
160143
.setSecureSettings(buildSecureSettings())
161144
.build();
162-
AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings);
145+
AzureStorageServiceImpl mock = createAzureService(settings);
163146
assertThat(mock.storageSettings.get("azure1").getProxy(), nullValue());
164147
assertThat(mock.storageSettings.get("azure2").getProxy(), nullValue());
165148
assertThat(mock.storageSettings.get("azure3").getProxy(), nullValue());
@@ -172,7 +155,7 @@ public void testProxyHttp() throws UnknownHostException {
172155
.put("azure.client.azure1.proxy.port", 8080)
173156
.put("azure.client.azure1.proxy.type", "http")
174157
.build();
175-
AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings);
158+
AzureStorageServiceImpl mock = createAzureService(settings);
176159
Proxy azure1Proxy = mock.storageSettings.get("azure1").getProxy();
177160

178161
assertThat(azure1Proxy, notNullValue());
@@ -192,7 +175,7 @@ public void testMultipleProxies() throws UnknownHostException {
192175
.put("azure.client.azure2.proxy.port", 8081)
193176
.put("azure.client.azure2.proxy.type", "http")
194177
.build();
195-
AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings);
178+
AzureStorageServiceImpl mock = createAzureService(settings);
196179
Proxy azure1Proxy = mock.storageSettings.get("azure1").getProxy();
197180
assertThat(azure1Proxy, notNullValue());
198181
assertThat(azure1Proxy.type(), is(Proxy.Type.HTTP));
@@ -211,7 +194,7 @@ public void testProxySocks() throws UnknownHostException {
211194
.put("azure.client.azure1.proxy.port", 8080)
212195
.put("azure.client.azure1.proxy.type", "socks")
213196
.build();
214-
AzureStorageServiceMockForSettings mock = new AzureStorageServiceMockForSettings(settings);
197+
AzureStorageServiceImpl mock = createAzureService(settings);
215198
Proxy azure1Proxy = mock.storageSettings.get("azure1").getProxy();
216199
assertThat(azure1Proxy, notNullValue());
217200
assertThat(azure1Proxy.type(), is(Proxy.Type.SOCKS));
@@ -227,7 +210,7 @@ public void testProxyNoHost() {
227210
.put("azure.client.azure1.proxy.type", randomFrom("socks", "http"))
228211
.build();
229212

230-
SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings));
213+
SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings));
231214
assertEquals("Azure Proxy type has been set but proxy host or port is not defined.", e.getMessage());
232215
}
233216

@@ -238,7 +221,7 @@ public void testProxyNoPort() {
238221
.put("azure.client.azure1.proxy.type", randomFrom("socks", "http"))
239222
.build();
240223

241-
SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings));
224+
SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings));
242225
assertEquals("Azure Proxy type has been set but proxy host or port is not defined.", e.getMessage());
243226
}
244227

@@ -249,7 +232,7 @@ public void testProxyNoType() {
249232
.put("azure.client.azure1.proxy.port", 8080)
250233
.build();
251234

252-
SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings));
235+
SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings));
253236
assertEquals("Azure Proxy port or host have been set but proxy type is not defined.", e.getMessage());
254237
}
255238

@@ -261,26 +244,10 @@ public void testProxyWrongHost() {
261244
.put("azure.client.azure1.proxy.port", 8080)
262245
.build();
263246

264-
SettingsException e = expectThrows(SettingsException.class, () -> new AzureStorageServiceMockForSettings(settings));
247+
SettingsException e = expectThrows(SettingsException.class, () -> createAzureService(settings));
265248
assertEquals("Azure proxy host is unknown.", e.getMessage());
266249
}
267250

268-
/**
269-
* This internal class just overload createClient method which is called by AzureStorageServiceImpl.doStart()
270-
*/
271-
class AzureStorageServiceMockForSettings extends AzureStorageServiceImpl {
272-
AzureStorageServiceMockForSettings(Settings settings) {
273-
super(settings, AzureStorageSettings.load(settings));
274-
}
275-
276-
// We fake the client here
277-
@Override
278-
void createClient(AzureStorageSettings azureStorageSettings) {
279-
this.clients.put(azureStorageSettings.getAccount(),
280-
new CloudBlobClient(URI.create("https://" + azureStorageSettings.getAccount())));
281-
}
282-
}
283-
284251
public void testBlobNameFromUri() throws URISyntaxException {
285252
String name = blobNameFromUri(new URI("https://myservice.azure.net/container/path/to/myfile"));
286253
assertThat(name, is("path/to/myfile"));
@@ -291,4 +258,27 @@ public void testBlobNameFromUri() throws URISyntaxException {
291258
name = blobNameFromUri(new URI("https://127.0.0.1/container/path/to/myfile"));
292259
assertThat(name, is("path/to/myfile"));
293260
}
261+
262+
private static MockSecureSettings buildSecureSettings() {
263+
MockSecureSettings secureSettings = new MockSecureSettings();
264+
secureSettings.setString("azure.client.azure1.account", "myaccount1");
265+
secureSettings.setString("azure.client.azure1.key", encodeKey("mykey1"));
266+
secureSettings.setString("azure.client.azure2.account", "myaccount2");
267+
secureSettings.setString("azure.client.azure2.key", encodeKey("mykey2"));
268+
secureSettings.setString("azure.client.azure3.account", "myaccount3");
269+
secureSettings.setString("azure.client.azure3.key", encodeKey("mykey3"));
270+
return secureSettings;
271+
}
272+
273+
private static Settings buildSettings() {
274+
return Settings.builder().setSecureSettings(buildSecureSettings()).build();
275+
}
276+
277+
private static AzureStorageServiceImpl createAzureService(final Settings settings) {
278+
return new AzureStorageServiceImpl(settings, AzureStorageSettings.load(settings));
279+
}
280+
281+
private static String encodeKey(final String value) {
282+
return Base64.encode(value.getBytes(StandardCharsets.UTF_8));
283+
}
294284
}

0 commit comments

Comments
 (0)