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

misc: e2e tests for s3 mrap #1193

Merged
merged 24 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from 22 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
20 changes: 20 additions & 0 deletions services/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,26 @@ subprojects {
description = "Run e2e service tests"
group = "verification"

if (project.name == "s3") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this works but this is getting ugly. I'm thinking we'll be revisiting the structure around e2e tests soon.

Copy link
Contributor Author

@0marperez 0marperez Feb 22, 2024

Choose a reason for hiding this comment

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

I agree,I think we could refactor some of this into functions if it starts getting used more.

dependencies {
val services = project.parent?.subprojects

if (services?.any { it.name == "s3control" } == true) {
implementation(project(":services:s3control"))
} else {
implementation("aws.sdk.kotlin:s3control:+")
}

if (services?.any { it.name == "sts" } == true) {
implementation(project(":services:sts"))
} else {
implementation("aws.sdk.kotlin:sts:+")
}

implementation(libs.smithy.kotlin.aws.signing.crt)
}
}

// Run the tests with the classpath containing the compile dependencies (including 'main'),
// runtime dependencies, and the outputs of this compilation:
classpath = compileDependencyFiles + runtimeDependencyFiles + output.allOutputs
Expand Down
109 changes: 109 additions & 0 deletions services/s3/e2eTest/src/MutliRegionAccessPointTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
package aws.sdk.kotlin.e2etest

import aws.sdk.kotlin.e2etest.S3TestUtils.createMultiRegionAccessPoint
import aws.sdk.kotlin.e2etest.S3TestUtils.deleteBucketAndAllContents
import aws.sdk.kotlin.e2etest.S3TestUtils.deleteMultiRegionAccessPoint
import aws.sdk.kotlin.e2etest.S3TestUtils.getAccountId
import aws.sdk.kotlin.e2etest.S3TestUtils.getMultiRegionAccessPointArn
import aws.sdk.kotlin.e2etest.S3TestUtils.getTestBucket
import aws.sdk.kotlin.e2etest.S3TestUtils.multiRegionAccessPointWasCreated
import aws.sdk.kotlin.services.s3.S3Client
import aws.sdk.kotlin.services.s3.deleteObject
import aws.sdk.kotlin.services.s3.putObject
import aws.sdk.kotlin.services.s3.withConfig
import aws.sdk.kotlin.services.s3control.S3ControlClient
import aws.smithy.kotlin.runtime.auth.awssigning.UnsupportedSigningAlgorithmException
import aws.smithy.kotlin.runtime.auth.awssigning.crt.CrtAwsSigner
import aws.smithy.kotlin.runtime.http.auth.SigV4AsymmetricAuthScheme
import kotlinx.coroutines.runBlocking
import org.junit.jupiter.api.AfterAll
import org.junit.jupiter.api.BeforeAll
import org.junit.jupiter.api.TestInstance
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFailsWith

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class MutliRegionAccessPointTest {
private val s3West = S3Client { region = "us-west-2" }
private val s3East = s3West.withConfig { region = "us-east-2" }
private val s3SigV4a = s3West.withConfig { authSchemes = listOf(SigV4AsymmetricAuthScheme(CrtAwsSigner)) }
private val s3Control = S3ControlClient { region = "us-west-2" }

private val multiRegionAccessPoint = "aws-sdk-for-kotlin-test-multi-region-access-point"
private val objectKey = "test.txt"

private lateinit var accountId: String
private lateinit var multiRegionAccessPointArn: String
private lateinit var usWestBucket: String
private lateinit var usEastBucket: String

@BeforeAll
private fun setUpMrapTest(): Unit = runBlocking {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: setup and cleanup are one word.

Copy link
Contributor

Choose a reason for hiding this comment

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

Counter-nit: "setup" and "cleanup" are nouns. "Set up" and "Clean up" are verbs, which is the preferred form of method names.

Copy link
Member

Choose a reason for hiding this comment

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

nit: My thought on this is to just call it setup, the test class is enough of a hint of what you are setting up

accountId = getAccountId()
usWestBucket = getTestBucket(s3West, "us-west-2", accountId)
usEastBucket = getTestBucket(s3East, "us-east-2", accountId)

createMultiRegionAccessPoint(
s3Control,
multiRegionAccessPoint,
usWestBucket,
usEastBucket,
accountId,
)

multiRegionAccessPointArn =
getMultiRegionAccessPointArn(
s3Control,
multiRegionAccessPoint,
accountId,
)
}

@AfterAll
private fun cleanUpMrapTest(): Unit = runBlocking {
if (multiRegionAccessPointWasCreated(s3Control, multiRegionAccessPoint, accountId)) {
deleteMultiRegionAccessPoint(s3Control, multiRegionAccessPoint, accountId)
}

deleteBucketAndAllContents(s3West, usWestBucket)
deleteBucketAndAllContents(s3East, usEastBucket)

s3West.close()
s3East.close()
s3SigV4a.close()
s3Control.close()
}

@Test
fun testMultiRegionAccessPointOperation(): Unit = runBlocking {
s3SigV4a.putObject {
bucket = multiRegionAccessPointArn
key = objectKey
}

s3SigV4a.deleteObject {
bucket = multiRegionAccessPointArn
key = objectKey
}
}

@Test
fun testUnsupportedSigningAlgorithm(): Unit = runBlocking {
val ex = assertFailsWith<UnsupportedSigningAlgorithmException> {
s3West.putObject {
bucket = multiRegionAccessPointArn
key = objectKey
}
}

assertEquals(
ex.message,
"SIGV4A support is not yet implemented for the default signer. For more information on how to enable it with the CRT signer, please refer to: https://a.co/3sf8533",
)
}
}
162 changes: 157 additions & 5 deletions services/s3/e2eTest/src/S3TestUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,61 @@
package aws.sdk.kotlin.e2etest

import aws.sdk.kotlin.services.s3.*
import aws.sdk.kotlin.services.s3.S3Client
import aws.sdk.kotlin.services.s3.model.*
import aws.sdk.kotlin.services.s3.model.BucketLocationConstraint
import aws.sdk.kotlin.services.s3.model.ExpirationStatus
import aws.sdk.kotlin.services.s3.model.LifecycleRule
import aws.sdk.kotlin.services.s3.model.LifecycleRuleFilter
import aws.sdk.kotlin.services.s3.paginators.listObjectsV2Paginated
import aws.sdk.kotlin.services.s3.waiters.waitUntilBucketExists
import aws.sdk.kotlin.services.s3control.*
import aws.sdk.kotlin.services.s3control.model.*
import aws.sdk.kotlin.services.sts.StsClient
import aws.smithy.kotlin.runtime.http.request.HttpRequest
import kotlinx.coroutines.*
import kotlinx.coroutines.flow.*
import kotlinx.coroutines.withTimeout
import java.io.OutputStreamWriter
import java.net.URL
import java.util.*
import javax.net.ssl.HttpsURLConnection
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.seconds

object S3TestUtils {

const val DEFAULT_REGION = "us-west-2"

// The E2E test account only has permission to operate on buckets with the prefix
private const val TEST_BUCKET_PREFIX = "s3-test-bucket-"

suspend fun getTestBucket(client: S3Client): String = getBucketWithPrefix(client, TEST_BUCKET_PREFIX)
suspend fun getTestBucket(
client: S3Client,
region: String? = null,
accountId: String? = null,
): String = getBucketWithPrefix(client, TEST_BUCKET_PREFIX, region, accountId)

private suspend fun getBucketWithPrefix(client: S3Client, prefix: String): String = withTimeout(60.seconds) {
var testBucket = client.listBuckets()
private suspend fun getBucketWithPrefix(
client: S3Client,
prefix: String,
region: String?,
accountId: String?,
): String = withTimeout(60.seconds) {
val buckets = client.listBuckets()
.buckets
?.mapNotNull { it.name }
?.firstOrNull { it.startsWith(prefix) }

var testBucket = if (region != null) {
buckets?.firstOrNull {
client.getBucketLocation {
bucket = it
expectedBucketOwner = accountId
}.locationConstraint?.value == region && it.startsWith(prefix)
}
} else {
buckets?.firstOrNull { it.startsWith(prefix) }
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: possible simplification

var testBucket = buckets?.firstOrNull { bucketName -> 
    bucketName.startsWith(prefix) && 
    region?.let { 
        client.getBucketLocation { 
            bucket = bucketName
            expectedBucketOwner = accountId
        }.locationConstraint?.value == region 
    } ?: true 
}


if (testBucket == null) {
testBucket = prefix + UUID.randomUUID()
Expand All @@ -38,7 +68,7 @@ object S3TestUtils {
client.createBucket {
bucket = testBucket
createBucketConfiguration {
locationConstraint = BucketLocationConstraint.fromValue(client.config.region!!)
locationConstraint = BucketLocationConstraint.fromValue(region ?: client.config.region!!)
}
}

Expand Down Expand Up @@ -115,4 +145,126 @@ object S3TestUtils {

return connection.responseCode
}

internal suspend fun getAccountId(): String {
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 couldn't think of a better place for this function. It seemed out of place in the tests class instead of here with the other utils. Placing it in a STS test utils folder could be another option but it seems a little odd since it's used for S3

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this seems fine, especially now that S3 E2E tests are guaranteed to have a dependency on STS

println("Getting account ID")
Copy link
Member

Choose a reason for hiding this comment

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

nit: seems like an unnecessary print to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a couple of other print statements in the utils, why does this one seem unnecessary?

Copy link
Member

Choose a reason for hiding this comment

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

Because all the other prints have debuggable information like the bucket name, operation, etc. This one has nothing, so I think it's not super useful. It's totally not a blocking comment, just something I noticed


val accountId = StsClient {
region = "us-west-2"
}.use {
it.getCallerIdentity().account
}

return checkNotNull(accountId) { "Unable to get AWS account ID" }
}

internal suspend fun createMultiRegionAccessPoint(
s3ControlClient: S3ControlClient,
multiRegionAccessPointName: String,
regionOneBucket: String,
regionTwoBucket: String,
testAccountId: String,
) {
println("Creating multi region access point: $multiRegionAccessPointName")

val createRequestToken = s3ControlClient.createMultiRegionAccessPoint {
accountId = testAccountId
details {
name = multiRegionAccessPointName
regions = listOf(
Region { bucket = regionOneBucket },
Region { bucket = regionTwoBucket },
)
}
}

waitUntilMultiRegionAccessPointOperationCompletes(
s3ControlClient,
checkNotNull(createRequestToken.requestTokenArn) { "Unable to get request token ARN" },
1000 * 60 * 10, // 10 minutes
testAccountId,
"createMultiRegionAccessPoint",
)
}

internal suspend fun getMultiRegionAccessPointArn(
s3ControlClient: S3ControlClient,
multiRegionAccessPointName: String,
testAccountId: String,
): String {
println("Getting multi region access point arn for: $multiRegionAccessPointName")

s3ControlClient.getMultiRegionAccessPoint {
accountId = testAccountId
name = multiRegionAccessPointName
}.accessPoint?.alias?.let { alias ->
return "arn:aws:s3::$testAccountId:accesspoint/$alias"
}
throw Exception("Unable to get multi region access point arn")
}

internal suspend fun deleteMultiRegionAccessPoint(
s3ControlClient: S3ControlClient,
multiRegionAccessPointName: String,
testAccountId: String,
) {
println("Deleting multi region access point: $multiRegionAccessPointName")

val deleteRequestToken = s3ControlClient.deleteMultiRegionAccessPoint {
accountId = testAccountId
details {
name = multiRegionAccessPointName
}
}

waitUntilMultiRegionAccessPointOperationCompletes(
s3ControlClient,
checkNotNull(deleteRequestToken.requestTokenArn) { "Unable to get request token ARN" },
1000 * 60 * 5, // 5 minutes
testAccountId,
"deleteMultiRegionAccessPoint",
)
}

private suspend fun waitUntilMultiRegionAccessPointOperationCompletes(
s3ControlClient: S3ControlClient,
request: String,
duration: Int,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: withTimeout also accepts a Duration, instead of passing this as an integer you could just be writing 5.minutes in the calling code and accept a Duration. Also probably rename to something like timeoutAfter

testAccountId: String,
operation: String,
) {
withTimeout(duration.milliseconds) {
while (true) {
val status = s3ControlClient.describeMultiRegionAccessPointOperation {
accountId = testAccountId
requestTokenArn = request
}.asyncOperation?.requestStatus

println("Waiting on $operation operation. Status: $status ")

if (status == "SUCCEEDED") {
println("$operation operation succeeded.")
return@withTimeout
}

if (status == "FAILED") throw Exception("$operation operation failed.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

style:

check(status != "FAILED") { "$operation operation failed")


delay(10.seconds) // Avoid constant status checks
}
}
}

internal suspend fun multiRegionAccessPointWasCreated(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be done using a get operation and then looking at the error we get if the MRAP doesn't exist. The only issue is it returns a rather generic S3ControlException and all we would have to differentiate a non existent MRAP from a failure would be the error message. Which doesn't seem right as it could change. Either way I can take that approach if that's what everyone thinks

s3Control: S3ControlClient,
multiRegionAccessPointName: String,
testAccountId: String,
): Boolean {
println("Checking if multi region access point was created: $multiRegionAccessPointName")

val search = s3Control.listMultiRegionAccessPoints {
accountId = testAccountId
}.accessPoints?.find { it.name == multiRegionAccessPointName }

return search != null
Copy link
Member

Choose a reason for hiding this comment

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

nit: simplification

return s3Control.listMultiRegionAccessPoints {
    accountId = testAccountId
}.accessPoints?.any { it.name == multiRegionAccessPointName }

}
}
Loading