honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix S3ClientSettings Leak (#56703) (#56862) 

Fixes the fact that repository metadata with the same settings still results in
multiple settings instances being cached as well as leaking settings on closing
a repository.

Closes #56702
This commit is contained in:
Armin Braun 2020-05-17 09:18:20 +02:00 committed by GitHub
parent 31f54c934e
commit c02850f335
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 67 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java
View File

@ -21,7 +21,6 @@ package org.elasticsearch.repositories.s3;
import com.amazonaws.ClientConfiguration;
import com.amazonaws.Protocol;
import org.elasticsearch.cluster.metadata.RepositoryMetadata;
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
@ -179,14 +178,13 @@ final class S3ClientSettings {
/**
* Overrides the settings in this instance with settings found in repository metadata.
*
* @param metadata RepositoryMetadata
* @param repositorySettings found in repository metadata
* @return S3ClientSettings
*/
S3ClientSettings refine(RepositoryMetadata metadata) {
final Settings repoSettings = metadata.settings();
S3ClientSettings refine(Settings repositorySettings) {
// Normalize settings to placeholder client settings prefix so that we can use the affix settings directly
final Settings normalizedSettings =
Settings.builder().put(repoSettings).normalizePrefix(PREFIX + PLACEHOLDER_CLIENT + '.').build();
Settings.builder().put(repositorySettings).normalizePrefix(PREFIX + PLACEHOLDER_CLIENT + '.').build();
final String newEndpoint = getRepoSettingOrDefault(ENDPOINT_SETTING, normalizedSettings, endpoint);
final Protocol newProtocol = getRepoSettingOrDefault(PROTOCOL_SETTING, normalizedSettings, protocol);
@ -200,8 +198,8 @@ final class S3ClientSettings {
final boolean newDisableChunkedEncoding = getRepoSettingOrDefault(
DISABLE_CHUNKED_ENCODING, normalizedSettings, disableChunkedEncoding);
final S3BasicCredentials newCredentials;
if (checkDeprecatedCredentials(repoSettings)) {
newCredentials = loadDeprecatedCredentials(repoSettings);
if (checkDeprecatedCredentials(repositorySettings)) {
newCredentials = loadDeprecatedCredentials(repositorySettings);
} else {
newCredentials = credentials;
}

28
plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java
View File

@ -58,7 +58,7 @@ class S3Service implements Closeable {
* Client settings derived from those in {@link #staticClientSettings} by combining them with settings
* in the {@link RepositoryMetadata}.
*/
private volatile Map<S3ClientSettings, Map<RepositoryMetadata, S3ClientSettings>> derivedClientSettings = emptyMap();
private volatile Map<Settings, S3ClientSettings> derivedClientSettings = emptyMap();
/**
* Refreshes the settings for the AmazonS3 clients and clears the cache of
@ -107,26 +107,23 @@ class S3Service implements Closeable {
* @return S3ClientSettings
*/
S3ClientSettings settings(RepositoryMetadata repositoryMetadata) {
final String clientName = S3Repository.CLIENT_NAME.get(repositoryMetadata.settings());
final Settings settings = repositoryMetadata.settings();
{
final S3ClientSettings existing = derivedClientSettings.get(settings);
if (existing != null) {
return existing;
}
}
final String clientName = S3Repository.CLIENT_NAME.get(settings);
final S3ClientSettings staticSettings = staticClientSettings.get(clientName);
if (staticSettings != null) {
{
final S3ClientSettings existing = derivedClientSettings.getOrDefault(staticSettings, emptyMap()).get(repositoryMetadata);
if (existing != null) {
return existing;
}
}
synchronized (this) {
final Map<RepositoryMetadata, S3ClientSettings> derivedSettings =
derivedClientSettings.getOrDefault(staticSettings, emptyMap());
final S3ClientSettings existing = derivedSettings.get(repositoryMetadata);
final S3ClientSettings existing = derivedClientSettings.get(settings);
if (existing != null) {
return existing;
}
final S3ClientSettings newSettings = staticSettings.refine(repositoryMetadata);
derivedClientSettings = MapBuilder.newMapBuilder(derivedClientSettings).put(
staticSettings, MapBuilder.newMapBuilder(derivedSettings).put(repositoryMetadata, newSettings).immutableMap()
).immutableMap();
final S3ClientSettings newSettings = staticSettings.refine(settings);
derivedClientSettings = MapBuilder.newMapBuilder(derivedClientSettings).put(settings, newSettings).immutableMap();
return newSettings;
}
}
@ -213,6 +210,7 @@ class S3Service implements Closeable {
}
// clear previously cached clients, they will be build lazily
clientsCache = emptyMap();
derivedClientSettings = emptyMap();
// shutdown IdleConnectionReaper background thread
// it will be restarted on new client usage
IdleConnectionReaper.shutdown();

9
plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java
View File

@ -22,7 +22,6 @@ package org.elasticsearch.repositories.s3;
import com.amazonaws.ClientConfiguration;
import com.amazonaws.Protocol;
import com.amazonaws.services.s3.AmazonS3Client;
import org.elasticsearch.cluster.metadata.RepositoryMetadata;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
@ -130,14 +129,13 @@ public class S3ClientSettingsTests extends ESTestCase {
Settings.builder().setSecureSettings(secureSettings).build()).get("default");
{
final S3ClientSettings refinedSettings = baseSettings.refine(new RepositoryMetadata("name", "type", Settings.EMPTY));
final S3ClientSettings refinedSettings = baseSettings.refine(Settings.EMPTY);
assertSame(refinedSettings, baseSettings);
}
{
final String endpoint = "some.host";
final S3ClientSettings refinedSettings = baseSettings.refine(new RepositoryMetadata("name", "type",
Settings.builder().put("endpoint", endpoint).build()));
final S3ClientSettings refinedSettings = baseSettings.refine(Settings.builder().put("endpoint", endpoint).build());
assertThat(refinedSettings.endpoint, is(endpoint));
S3BasicSessionCredentials credentials = (S3BasicSessionCredentials) refinedSettings.credentials;
assertThat(credentials.getAWSAccessKeyId(), is("access_key"));
@ -146,8 +144,7 @@ public class S3ClientSettingsTests extends ESTestCase {
}
{
final S3ClientSettings refinedSettings = baseSettings.refine(new RepositoryMetadata("name", "type",
Settings.builder().put("path_style_access", true).build()));
final S3ClientSettings refinedSettings = baseSettings.refine(Settings.builder().put("path_style_access", true).build());
assertThat(refinedSettings.pathStyleAccess, is(true));
S3BasicSessionCredentials credentials = (S3BasicSessionCredentials) refinedSettings.credentials;
assertThat(credentials.getAWSAccessKeyId(), is("access_key"));

46
plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java Normal file
View File

@ -0,0 +1,46 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.repositories.s3;
import org.elasticsearch.cluster.metadata.RepositoryMetadata;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
public class S3ServiceTests extends ESTestCase {
public void testCachedClientsAreReleased() {
final S3Service s3Service = new S3Service();
final Settings settings = Settings.builder().put("endpoint", "http://first").build();
final RepositoryMetadata metadata1 = new RepositoryMetadata("first", "s3", settings);
final RepositoryMetadata metadata2 = new RepositoryMetadata("second", "s3", settings);
final S3ClientSettings clientSettings = s3Service.settings(metadata2);
final S3ClientSettings otherClientSettings = s3Service.settings(metadata2);
assertSame(clientSettings, otherClientSettings);
final AmazonS3Reference reference = s3Service.client(metadata1);
reference.close();
s3Service.close();
final AmazonS3Reference referenceReloaded = s3Service.client(metadata1);
assertNotSame(referenceReloaded, reference);
referenceReloaded.close();
s3Service.close();
final S3ClientSettings clientSettingsReloaded = s3Service.settings(metadata1);
assertNotSame(clientSettings, clientSettingsReloaded);
}
}