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

Please add support for AWS SDK v2 #1442

Closed
xenomachina opened this issue Apr 30, 2019 · 15 comments
Closed

Please add support for AWS SDK v2 #1442

xenomachina opened this issue Apr 30, 2019 · 15 comments
Labels
help wanted type/breaking-api-change Public APIs are being changed

Comments

@xenomachina
Copy link

LocalStackContainer uses the AWS SDK for Java v1.

The AWS SDK for Java v2 API has renamed everything. In particular, all of the packages have changed, and so the AWSCredentialsProvider and AwsClientBuilder.EndpointConfiguration returned by LocalStackContainer are not usable as-is in the AWS SDK for Java v2. It is possible to adapt them with some work (and we can share our wrapper if you like, though it is written in Kotlin) however, it would be nice if clients using one version of the AWS SDK for Java did not need to depend on artifacts from the other version, even transitively.

I imagine you'll want to retain support for the v1 SDK at least until AWS deprecates it.

@rnorth
Copy link
Member

rnorth commented May 21, 2019

I've used Testcontainers for testing code that uses AWS SDKv2, and I'd agree that LocalStackContainer's coupling to the v1 SDK makes it a rough fit.

The AWS SDK coupling in general is, I think, fairly important to retain as it removes some pain points that would be exposed if we just provided a thin wrapper around Localstack.

I think that we probably need a separate module for AWS SDKv2 compatibility in this case. Let me think and discuss with @bsideup and @kiview...

@ashleyfrieze
Copy link

@rnorth - it might be possible to make two subclasses of a LocalStackContainerBase, one for v1 and one for v2, with provided dependencies. It would be a shame to have to duplicate the code across two modules.

Perhaps some of the issue here is that the local stack container doesn't really need to do a HUGE amount of AWS stuff itself. We only really need the URL to pass to our SDK client library.

If @bsideup and @kiview and yourself can propose how this should be done, I don't mind hacking a PR together. It would be useful in my current project.

@rnorth
Copy link
Member

rnorth commented Aug 3, 2019

Thanks @ashleyfrieze - sorry for the slow reply.

Perhaps some of the issue here is that the local stack container doesn't really need to do a HUGE amount of AWS stuff itself. We only really need the URL to pass to our SDK client library.

You're quite right; this container only faintly interacts with the AWS SDK, but just enough to cause us a compatibility issue.

We since learned that this is a bit of a mistaken pattern to use for our modules, and now recommend that new modules only expose JDK types in their public APIs. Too late for this module, though 😬.

Thinking about whether this should be within the same module:

Pros:

  • We don't have to publish and document a new module (though really, it's not much of a burden for us)
  • Users do not have to learn of the existence of a new module when they want to use/migrate to AWSDKv2
  • We avoid an embarassingly asymmetric naming situation of having a localstack module and a localstack-awssdk2 (?) module 😁

Cons:

  • Code supporting both versions will be on the classpath, but one will be broken depending on which version of the AWS SDK is also present. I feel that this could be very confusing and frustrating for users.

In the long run, IMHO it would be good to have a localstack-awssdk-v1 module and a localstack-awssdk-v2 module. Renaming the current module to localstack-awssdk-v1 would of course be a breaking change, so something that we should really wait to do along with a major version bump for Testcontainers...

But that wouldn't stop us from taking the first step in that direction: creation of localstack-awssdk-v2.

Thoughts?

(Aside: it's amusing that this is a prime example of 'naming things' being the hardest part of the problem 😄)

@rnorth rnorth added help wanted type/breaking-api-change Public APIs are being changed labels Aug 3, 2019
@oscarvarto
Copy link

oscarvarto commented Aug 26, 2019

I've used Testcontainers for testing code that uses AWS SDKv2, and I'd agree that LocalStackContainer's coupling to the v1 SDK makes it a rough fit.

Would you mind explaining your strategy to use Testcontainers (currently coupled to SDK V1) and code written with AWS SDK V2?
That might be helpful for a project I am currently working on.
Thanks in advance @rnorth

@ashleyfrieze, @bsideup, @kiview, and @rnorth I am craving for SDK V2 support in Testcontainers. Any news?

@xenomachina
Copy link
Author

@oscarvarto we have something like this (in Kotlin):

import software.amazon.awssdk.awscore.client.builder.AwsClientBuilder
...
fun <T: AwsClientBuilder<*,*>> T.localStack(
    container: LocalStackContainer,
    service: LocalStackContainer.Service
): T {
    endpointOverride(URI(container.getEndpointConfiguration(service).serviceEndpoint))
    credentialsProvider(container.defaultCredentialsProvider?.let { v1Provider ->
        val v1Credentials = v1Provider.credentials
        AwsCredentialsProvider {
            object : AwsCredentials {
                override fun accessKeyId(): String = v1Credentials.awsAccessKeyId
                override fun secretAccessKey(): String = v1Credentials.awsSecretKey
            }
        }
    })
    return this
}

And then build SDK v2 clients that connect to a LocalStackContainer by doing something like:

val client = DynamoDbClient.builder()
    .localStack(localStackContainer, DYNAMODB))
    .build()

@guizmaii
Copy link

Scala version of the @xenomachina code:

import java.net.URI

import org.testcontainers.containers.localstack.LocalStackContainer
import software.amazon.awssdk.auth.credentials.{AwsCredentials, AwsCredentialsProvider}
import software.amazon.awssdk.awscore.client.builder.AwsClientBuilder
import software.amazon.awssdk.regions.Region

object AwsClientBuilderOps {

  implicit final class AwsClientBuilderOps[T <: AwsClientBuilder[_, _]](val builder: T) extends AnyVal {
    def localStack(container: LocalStackContainer, service: LocalStackContainer.Service): T = {
      builder.endpointOverride(new URI(container.getEndpointConfiguration(service).getServiceEndpoint))
      builder.region(Region.US_EAST_1)
      builder.credentialsProvider {
        val v1Credentials = container.getDefaultCredentialsProvider.getCredentials
        new AwsCredentialsProvider {
          override def resolveCredentials(): AwsCredentials = new AwsCredentials {
            override def accessKeyId(): String = v1Credentials.getAWSAccessKeyId

            override def secretAccessKey(): String = v1Credentials.getAWSSecretKey
          }
        }
      }
      builder
    }
  }

}

I added the .region because you need it for SQS.

Cheers
😉

@hdurix
Copy link

hdurix commented Feb 27, 2020

And now a Java version of the temporary solution:

@Bean
public S3Client s3Client(LocalStackContainer localStackContainer) throws URISyntaxException {
  return S3Client
    .builder()
    .endpointOverride(new URI(localStackContainer.getEndpointConfiguration(LocalStackContainer.Service.S3).getServiceEndpoint()))
    .credentialsProvider(new CrossAwsCredentialsProvider(localStackContainer.getDefaultCredentialsProvider()))
    .build();
}

private class CrossAwsCredentialsProvider implements AwsCredentialsProvider {
  private final AWSCredentials credentials;

  public CrossAwsCredentialsProvider(AWSCredentialsProvider provider) {
    this.credentials = provider.getCredentials();
  }

  @Override
  public AwsCredentials resolveCredentials() {
    return AwsBasicCredentials.create(credentials.getAWSAccessKeyId(), credentials.getAWSSecretKey());
  }
}

@dbveeva
Copy link

dbveeva commented Jul 15, 2020

When will the fix committed by @rnorth be released? I am using v1.14.3.

@TylerJaacks
Copy link

Is this issue solved because I am on 15 and it seems to still be an issue.

@jakobbraun
Copy link

I'am also experiencing issues with version 1.14.3, when using AWS SDK v2:

java.lang.NoClassDefFoundError: com/amazonaws/auth/AWSCredentials
at com.exasol.adapter.document.files.DocumentFilesAdapterS3IT.<init>

The reason for that error is, that the localstack testconainers module internally still uses the AWS SDK v1. While that would be no problem, the issue is, that it declares it as compileOnly dependency (see build.gradle)

A quick fix for this issue is to also provide the AWS SDK in test scop.
For maven you can use:

        <dependency>
            <groupId>com.amazonaws</groupId>
            <artifactId>aws-java-sdk-s3</artifactId>
            <version>1.11.860</version>
            <scope>test</scope>
        </dependency>

I do however no understand, why this dependecy is set to compileOnly. At least some documentation would be very helpful.

@brizzbuzz
Copy link

Hey so I wasn't happy with just adding the v1 dependency, even in test, it felt too hacky for me, so I put together a LocalstackContainerV2 that will use the Aws V2 api natively... I think some tricky work work might need to be done to get v1 and v2 to sit nicely together, but I will leave the broader discussion on how to really support this to the TestContainer team, but this solution fits my needs. Also this makes no attempt to support localstack < 11.5.0 which relies on individual ports for each service.

import org.rnorth.ducttape.Preconditions
import org.testcontainers.DockerClientFactory
import org.testcontainers.containers.GenericContainer
import org.testcontainers.containers.wait.strategy.Wait
import org.testcontainers.utility.DockerImageName
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider
import java.net.InetAddress
import java.net.URI

class LocalstackContainerV2(
  dockerImageName: DockerImageName,
  // can be ignored, personally I need to leverage the localstack pro offerings
  val apiKey: String? = null
) : GenericContainer<LocalstackContainerV2>(dockerImageName) {

  companion object {
    private const val PORT = 4566
    const val ACCESS_KEY = "accessKey"
    const val SECRET_KEY = "secretKey"
    const val REGION = "us-east-1"
  }

  // TODO override to prevent version < 0.11.5?
  private val defaultImageName = DockerImageName.parse("localstack/localstack")
  private var services = emptyList<Service>()

  init {
    dockerImageName.assertCompatibleWith(defaultImageName)
    withFileSystemBind(DockerClientFactory.instance().remoteDockerUnixSocketPath, "/var/run/docker.sock")
    waitingFor(Wait.forLogMessage(".*Ready\\.\n", 1))
  }

  override fun configure() {
    super.configure()
    setPreconditions()
    configureEnv()
    exposePorts()
  }

  fun withServices(vararg services: Service): LocalstackContainerV2 = apply {
    this.services = this.services.plus(services)
  }

  fun getEndpointOverride(): URI {
    val hostAddress = InetAddress.getByName(host).hostAddress
    return URI("http://$hostAddress:${this.firstMappedPort}")
  }

  fun getDefaultCredentialsProvider(): AwsCredentialsProvider = StaticCredentialsProvider.create(
    AwsBasicCredentials.create(ACCESS_KEY, SECRET_KEY)
  )

  private fun setPreconditions() {
    Preconditions.check("services list must not be empty", services.isNotEmpty())
  }

  private fun configureEnv() {
    withEnv("SERVICES", services.joinToString(separator = ",") { it.serviceName })
    if (apiKey != null) {
      withEnv("LOCALSTACK_API_KEY", apiKey)
    }
  }

  private fun exposePorts() {
    addExposedPort(PORT)
  }

  enum class Service(val serviceName: String) {
    S3("s3"),
    ECR("ecr")
  // etc... more services go here
  }
}

Then an abstract class instantiating the container can be something like

abstract class LocalstackIntegrationTest {

  companion object {
    private val region = Region.of(LocalstackContainerV2.REGION)
    private val services = arrayOf(LocalstackContainerV2.Service.S3, LocalstackContainerV2.Service.ECR)
    private val LOCALSTACK_IMAGE = DockerImageName.parse("localstack/localstack:0.12.2")
    lateinit var s3Consumer: S3Consumer
    lateinit var ecrConsumer: EcrConsumer

    val localstack = LocalstackContainerV2(LOCALSTACK_IMAGE).apply {
      withServices(*services)
    }
  }

  init {
    localstack.start()
  }

  @BeforeAll
  fun setup() {
    val s3client = setupS3Client()
    val ecrClient = setupEcrClient()
    s3Consumer = S3Consumer(s3client)
    ecrConsumer = EcrConsumer(ecrClient)
  }

  private fun setupS3Client() = S3Client.builder()
    .region(region)
    .endpointOverride(localstack.getEndpointOverride())
    .credentialsProvider(localstack.getDefaultCredentialsProvider())
    .build()

  private fun setupEcrClient() = EcrClient.builder()
    .region(region)
    .endpointOverride(localstack.getEndpointOverride())
    .credentialsProvider(localstack.getDefaultCredentialsProvider())
    .build()
}

@ashleyfrieze
Copy link

@rnorth - perhaps the long-term solution is to make the SDK v1 class (keeping its current name) a subclass of an SDK independent LocalstackTestContainer - say LocalstackAPIContainer. Perhaps this can also use composition to allow for version-specifics of the Localstack API - e.g. service endpoints.

The existing class can become a subclass of the above, adding in the SDK v1 dependencies. It would then be a case of either producing documentation showing how to use API v2, or producing an APIv2 specific subclass if necessary. I suspect the first of these would be better.

If there's interest in this, I could look at submitting a PR along these lines...?

@rfelgent
Copy link

rfelgent commented Jan 1, 2021

IMHO this issue should not be closed, as the code of #3557 was not merged at all.

Could we reopen this issue, as support for aws sdk v2 is still lacking ?

@elenigen
Copy link

@rnorth Is it fine to reopen it?

@eddumelendez
Copy link
Member

Hi, just an update on this topic.

Methods using aws sdk v1 have been deprecated getDefaultCredentialsProvider() and getEndpointConfiguration() and they are going to be removed in the future. Although, this is not solving the current issue, the way to address it is by importing the aws sdk v1 to make the module compile (for aws sdk v2 users). In the future, it is expected to use the following methods getEndpointOverride(), getRegion(), getAccessKey() and getSecretKey() already provided by LocalStackContainer and build the client which in this case can be sdk v1 or v2.

As mentioned in the checklist for a new module

[ ] The module's public API should only handle standard JDK data types and MUST not expose data types that come from compileOnly dependencies. This is to reduce the risk of compatibility problems with future versions of third party libraries.

That said, the issue can remain closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted type/breaking-api-change Public APIs are being changed
Projects
None yet
Development

No branches or pull requests