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.

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

Add EC2 credential test for repository-s3 #31918

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ class ClusterConfiguration {
this.project = project
}

Map<String, String> systemProperties = new HashMap<>()
// **Note** for systemProperties, settings, keystoreFiles etc:
// value could be a GString that is evaluated to just a String
// there are cases when value depends on task that is not executed yet on configuration stage
Map<String, Object> systemProperties = new HashMap<>()
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference is easy to overlook , maybe it would be good to have a comment explaining why Object is needed.
We might be able to have Map<String, GString> to make the intention cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯


Map<String, Object> settings = new HashMap<>()

Expand All @@ -157,7 +160,7 @@ class ClusterConfiguration {
List<Object> dependencies = new ArrayList<>()

@Input
void systemProperty(String property, String value) {
void systemProperty(String property, Object value) {
systemProperties.put(property, value)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,6 @@ class ClusterFormationTasks {

/** Adds a task to start an elasticsearch node with the given configuration */
static Task configureStartTask(String name, Project project, Task setup, NodeInfo node) {

// this closure is converted into ant nodes by groovy's AntBuilder
Closure antRunner = { AntBuilder ant ->
ant.exec(executable: node.executable, spawn: node.config.daemonize, dir: node.cwd, taskname: 'elasticsearch') {
Expand All @@ -624,13 +623,6 @@ class ClusterFormationTasks {
node.writeWrapperScript()
}

// we must add debug options inside the closure so the config is read at execution time, as
// gradle task options are not processed until the end of the configuration phase
if (node.config.debug) {
println 'Running elasticsearch in debug mode, suspending until connected on port 8000'
node.env['ES_JAVA_OPTS'] = '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8000'
}

node.getCommandString().eachLine { line -> logger.info(line) }

if (logger.isInfoEnabled() || node.config.daemonize == false) {
Expand All @@ -648,6 +640,27 @@ class ClusterFormationTasks {
}
start.doLast(elasticsearchRunner)
start.doFirst {
// Configure ES JAVA OPTS - adds system properties, assertion flags, remote debug etc
List<String> esJavaOpts = [node.env.get('ES_JAVA_OPTS', '')]
String collectedSystemProperties = node.config.systemProperties.collect { key, value -> "-D${key}=${value}" }.join(" ")
esJavaOpts.add(collectedSystemProperties)
esJavaOpts.add(node.config.jvmArgs)
if (Boolean.parseBoolean(System.getProperty('tests.asserts', 'true'))) {
// put the enable assertions options before other options to allow
// flexibility to disable assertions for specific packages or classes
// in the cluster-specific options
esJavaOpts.add("-ea")
esJavaOpts.add("-esa")
}
// we must add debug options inside the closure so the config is read at execution time, as
// gradle task options are not processed until the end of the configuration phase
if (node.config.debug) {
println 'Running elasticsearch in debug mode, suspending until connected on port 8000'
esJavaOpts.add('-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8000')
}
node.env['ES_JAVA_OPTS'] = esJavaOpts.join(" ")

//
project.logger.info("Starting node in ${node.clusterName} distribution: ${node.config.distribution}")
}
return start
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,15 +180,7 @@ class NodeInfo {
}

args.addAll("-E", "node.portsfile=true")
String collectedSystemProperties = config.systemProperties.collect { key, value -> "-D${key}=${value}" }.join(" ")
String esJavaOpts = config.jvmArgs.isEmpty() ? collectedSystemProperties : collectedSystemProperties + " " + config.jvmArgs
if (Boolean.parseBoolean(System.getProperty('tests.asserts', 'true'))) {
// put the enable assertions options before other options to allow
// flexibility to disable assertions for specific packages or classes
// in the cluster-specific options
esJavaOpts = String.join(" ", "-ea", "-esa", esJavaOpts)
}
env = ['ES_JAVA_OPTS': esJavaOpts]
env = [:]
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed, the map is already initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, map is null, here is the output if I drop this line:

Cannot invoke method put() on null object

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, this class initializes in the constructor, this is ok as it is.

for (Map.Entry<String, String> property : System.properties.entrySet()) {
if (property.key.startsWith('tests.es.')) {
args.add("-E")
Expand Down
40 changes: 35 additions & 5 deletions plugins/repository-s3/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,17 @@ String s3TemporarySessionToken = System.getenv("amazon_s3_session_token_temporar
String s3TemporaryBucket = System.getenv("amazon_s3_bucket_temporary")
String s3TemporaryBasePath = System.getenv("amazon_s3_base_path_temporary")

String s3EC2Bucket = System.getenv("amazon_s3_bucket_ec2")
String s3EC2BasePath = System.getenv("amazon_s3_base_path_ec2")
String s3EC2AccessKey = System.getenv("amazon_s3_access_key_ec2")
String s3EC2Token = System.getenv("amazon_s3_token_ec2")

// If all these variables are missing then we are testing against the internal fixture instead, which has the following
// credentials hard-coded in.

if (!s3PermanentAccessKey && !s3PermanentSecretKey && !s3PermanentBucket && !s3PermanentBasePath
&& !s3TemporaryAccessKey && !s3TemporarySecretKey && !s3TemporaryBucket && !s3TemporaryBasePath && !s3TemporarySessionToken) {
&& !s3TemporaryAccessKey && !s3TemporarySecretKey && !s3TemporaryBucket && !s3TemporaryBasePath && !s3TemporarySessionToken
&& !s3EC2Bucket && !s3EC2BasePath && !s3EC2AccessKey) {

s3PermanentAccessKey = 's3_integration_test_permanent_access_key'
s3PermanentSecretKey = 's3_integration_test_permanent_secret_key'
Expand All @@ -106,9 +112,15 @@ if (!s3PermanentAccessKey && !s3PermanentSecretKey && !s3PermanentBucket && !s3P
s3TemporaryBasePath = 'integration_test'
s3TemporarySessionToken = 's3_integration_test_temporary_session_token'

s3EC2Bucket = 'ec2-bucket-test'
s3EC2BasePath = 'integration_test'
s3EC2AccessKey = 'ec2_accessKey'
s3EC2Token = 'ec2_tkn'

useFixture = true
} else if (!s3PermanentAccessKey || !s3PermanentSecretKey || !s3PermanentBucket || !s3PermanentBasePath
|| !s3TemporaryAccessKey || !s3TemporarySecretKey || !s3TemporaryBucket || !s3TemporaryBasePath || !s3TemporarySessionToken) {
|| !s3TemporaryAccessKey || !s3TemporarySecretKey || !s3TemporaryBucket || !s3TemporaryBasePath || !s3TemporarySessionToken
|| !s3EC2Bucket || !s3EC2BasePath || !s3EC2AccessKey) {
throw new IllegalArgumentException("not all options specified to run against external S3 service")
}

Expand Down Expand Up @@ -271,7 +283,10 @@ if (useFixture && minioDistribution) {
integTestMinioRunner.dependsOn(startMinio)
integTestMinioRunner.finalizedBy(stopMinio)
// Minio only supports a single access key, see https://github.com/minio/minio/pull/5968
integTestMinioRunner.systemProperty 'tests.rest.blacklist', 'repository_s3/30_repository_temporary_credentials/*'
integTestMinioRunner.systemProperty 'tests.rest.blacklist', [
'repository_s3/30_repository_temporary_credentials/*',
'repository_s3/40_repository_ec2_credentials/*'
].join(",")

project.check.dependsOn(integTestMinio)
}
Expand All @@ -280,15 +295,26 @@ if (useFixture && minioDistribution) {
task s3Fixture(type: AntFixture) {
dependsOn testClasses
env 'CLASSPATH', "${ -> project.sourceSets.test.runtimeClasspath.asPath }"
env 'S3FIXTURE_WORKINGDIR', "${baseDir}"
env 'S3FIXTURE_PERMANENT_BUCKET_NAME', "${s3PermanentBucket}"
env 'S3FIXTURE_PERMANENT_KEY', "${s3PermanentAccessKey}"
env 'S3FIXTURE_TEMPORARY_BUCKET_NAME', "${s3TemporaryBucket}"
env 'S3FIXTURE_TEMPORARY_KEY', "${s3TemporaryAccessKey}"
env 'S3FIXTURE_TEMPORARY_SESSION_TOKEN', "${s3TemporarySessionToken}"
env 'S3FIXTURE_EC2_BUCKET_NAME', "${s3EC2Bucket}"
env 'S3FIXTURE_EC2_KEY', "${s3EC2AccessKey}"
env 'S3FIXTURE_EC2_TOKEN', "${s3EC2Token}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not so sure about this, compared with your proposal of configuring the fixture using a file. @tlrx what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason I changed to env variables - I faced some issues on creating a property file - again due to different stages of configuring / running - property file is not visible to fixture

executable = new File(project.runtimeJavaHome, 'bin/java')
args 'org.elasticsearch.repositories.s3.AmazonS3Fixture', baseDir, s3PermanentBucket, s3TemporaryBucket
args 'org.elasticsearch.repositories.s3.AmazonS3Fixture', baseDir
}

Map<String, Object> expansions = [
'permanent_bucket': s3PermanentBucket,
'permanent_base_path': s3PermanentBasePath,
'temporary_bucket': s3TemporaryBucket,
'temporary_base_path': s3TemporaryBasePath
'temporary_base_path': s3TemporaryBasePath,
'ec2_bucket': s3EC2Bucket,
'ec2_base_path': s3EC2BasePath
]

processTestResources {
Expand All @@ -309,6 +335,10 @@ integTestCluster {
/* Use a closure on the string to delay evaluation until tests are executed */
setting 's3.client.integration_test_permanent.endpoint', "http://${-> s3Fixture.addressAndPort}"
setting 's3.client.integration_test_temporary.endpoint', "http://${-> s3Fixture.addressAndPort}"
setting 's3.client.integration_test_ec2.endpoint', "http://${-> s3Fixture.addressAndPort}"

// to redirect InstanceProfileCredentialsProvider to custom auth point
systemProperty "com.amazonaws.sdk.ec2MetadataServiceEndpointOverride", "http://${-> s3Fixture.addressAndPort}"
} else {
println "Using an external service to test the repository-s3 plugin"
}
Expand Down
Loading