Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add null and empty string validation to S3 bucket #107883

Merged
merged 3 commits into from
Apr 25, 2024
Merged

Add null and empty string validation to S3 bucket #107883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36ba2f7
Add null and empty string validation to S3 bucket
mhl-b Apr 25, 2024
ba5d62e
Add null and empty string validation to S3 bucket
mhl-b Apr 25, 2024
49940f1
inline s3 repo setting builder
mhl-b Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,8 @@ class S3Repository extends MeteredBlobStoreRepository {

// Parse and validate the user's S3 Storage Class setting
this.bucket = BUCKET_SETTING.get(metadata.settings());
if (bucket == null) {
throw new RepositoryException(metadata.name(), "No bucket defined for s3 repository");
if (Strings.hasLength(bucket) == false) {
throw new IllegalArgumentException("Invalid S3 bucket name, cannot be null or empty");
}

this.bufferSize = BUFFER_SIZE_SETTING.get(metadata.settings());
Expand Down
9 changes: 7 additions & 2 deletions ...sitory-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ protected Settings nodeSettings() {
public void testRepositoryCredentialsOverrideSecureCredentials() {
final String repositoryName = "repo-creds-override";
final Settings.Builder repositorySettings = Settings.builder()
.put(S3Repository.BUCKET_SETTING.getKey(), "bucket")
// repository settings for credentials override node secure settings
.put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key")
.put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret");
Expand Down Expand Up @@ -115,7 +116,7 @@ public void testRepositoryCredentialsOverrideSecureCredentials() {
public void testReinitSecureCredentials() {
final String clientName = randomFrom("default", "other");

final Settings.Builder repositorySettings = Settings.builder();
final Settings.Builder repositorySettings = Settings.builder().put(S3Repository.BUCKET_SETTING.getKey(), "bucket");
final boolean hasInsecureSettings = randomBoolean();
if (hasInsecureSettings) {
// repository settings for credentials override node secure settings
Expand Down Expand Up @@ -153,7 +154,10 @@ public void testReinitSecureCredentials() {
final MockSecureSettings newSecureSettings = new MockSecureSettings();
newSecureSettings.setString("s3.client." + clientName + ".access_key", "new_secret_aws_key");
newSecureSettings.setString("s3.client." + clientName + ".secret_key", "new_secret_aws_secret");
final Settings newSettings = Settings.builder().setSecureSettings(newSecureSettings).build();
final Settings newSettings = Settings.builder()
.put(S3Repository.BUCKET_SETTING.getKey(), "bucket")
.setSecureSettings(newSecureSettings)
.build();
// reload S3 plugin settings
final PluginsService plugins = getInstanceFromNode(PluginsService.class);
final ProxyS3RepositoryPlugin plugin = plugins.filterPlugins(ProxyS3RepositoryPlugin.class).findFirst().get();
Expand Down Expand Up @@ -202,6 +206,7 @@ public void testInsecureRepositoryCredentials() throws Exception {
createRepository(
repositoryName,
Settings.builder()
.put(S3Repository.BUCKET_SETTING.getKey(), "bucket")
.put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key")
.put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret")
.build()
Expand Down
24 changes: 22 additions & 2 deletions modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public void testInvalidChunkBufferSizeSettings() {

private Settings bufferAndChunkSettings(long buffer, long chunk) {
return Settings.builder()
.put(S3Repository.BUCKET_SETTING.getKey(), "bucket")
.put(S3Repository.BUFFER_SIZE_SETTING.getKey(), new ByteSizeValue(buffer, ByteSizeUnit.MB).getStringRep())
.put(S3Repository.CHUNK_SIZE_SETTING.getKey(), new ByteSizeValue(chunk, ByteSizeUnit.MB).getStringRep())
.build();
Expand All @@ -102,15 +103,22 @@ public void testBasePathSetting() {
final RepositoryMetadata metadata = new RepositoryMetadata(
"dummy-repo",
"mock",
Settings.builder().put(S3Repository.BASE_PATH_SETTING.getKey(), "foo/bar").build()
Settings.builder()
.put(S3Repository.BUCKET_SETTING.getKey(), "bucket")
.put(S3Repository.BASE_PATH_SETTING.getKey(), "foo/bar")
.build()
);
try (S3Repository s3repo = createS3Repo(metadata)) {
assertEquals("foo/bar/", s3repo.basePath().buildAsString());
}
}

public void testDefaultBufferSize() {
final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", Settings.EMPTY);
final RepositoryMetadata metadata = new RepositoryMetadata(
"dummy-repo",
"mock",
Settings.builder().put(S3Repository.BUCKET_SETTING.getKey(), "bucket").build()
);
try (S3Repository s3repo = createS3Repo(metadata)) {
assertThat(s3repo.getBlobStore(), is(nullValue()));
s3repo.start();
Expand All @@ -121,6 +129,17 @@ public void testDefaultBufferSize() {
}
}

public void testMissingBucketName() {
final var metadata = new RepositoryMetadata("repo", "mock", Settings.EMPTY);
assertThrows(IllegalArgumentException.class, () -> createS3Repo(metadata));
}

public void testEmptyBucketName() {
final var settings = Settings.builder().put(S3Repository.BUCKET_SETTING.getKey(), "").build();
final var metadata = new RepositoryMetadata("repo", "mock", settings);
assertThrows(IllegalArgumentException.class, () -> createS3Repo(metadata));
}

private S3Repository createS3Repo(RepositoryMetadata metadata) {
return new S3Repository(
metadata,
Expand All @@ -132,4 +151,5 @@ private S3Repository createS3Repo(RepositoryMetadata metadata) {
S3RepositoriesMetrics.NOOP
);
}

}
50 changes: 40 additions & 10 deletions ...es/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,46 @@
# Integration tests for repository-s3
#
"Module repository-s3 is loaded":
- skip:
reason: "contains is a newly added assertion"
features: contains
- do:
cluster.state: {}
- skip:
reason: "contains is a newly added assertion"
features: contains
- do:
cluster.state: { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: cleaning up stuff like whitespace in YAML tests is ok but I'd generally prefer them in a separate PR - it makes git bisect easier to use and avoids cluttering up git annotate output etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep it mind in next PR's. I applied project formatting to entire file, it's relatively small one.


# Get master node id
- set: { master_node: master }
# Get master node id
- set: { master_node: master }

- do:
nodes.info: {}
- do:
nodes.info: { }

- contains: { nodes.$master.modules: { name: repository-s3 } }
- contains: { nodes.$master.modules: { name: repository-s3 } }

---
"Create S3 snapshot repository":
- do:
snapshot.create_repository:
repository: test-snapshot-repo
verify: false
body:
type: s3
settings:
bucket: test-bucket

---
"Create S3 snapshot repository without bucket":
- do:
catch: /illegal_argument_exception/
snapshot.create_repository:
repository: test-snapshot-repo-without-bucket
verify: false
body:
type: s3
- do:
catch: /illegal_argument_exception/
snapshot.create_repository:
repository: test-snapshot-repo-without-bucket
verify: false
body:
type: s3
settings:
bucket: ""