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

Azure Cosmos DB: Introduce CosmosDBEmulatorContainer #4303

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

okohub
Copy link
Contributor

@okohub okohub commented Jul 23, 2021

Azure Cosmos DB is a fully managed NoSQL database service for modern app development. I want to introduce it as a test container. Azure Cosmos DB has its own emulator and docker image provided by Microsoft.

The critical point in implementing this container is mandatory SSL connection. Azure Java SDK (https://github.com/Azure/azure-sdk-for-java) has no option to override SSL check, so I added some configurable KeyStore codes to prepare SSL context properly instead of overriding it via reflection or something else.

Please feel free to guide me about contributing this emulator at its best.

Default docker image - Checked

Module dependencies - Checked

API - Checked

Documentation (don't know what can I do extra?)

@okohub
Copy link
Contributor Author

okohub commented Jul 23, 2021

Fixes #4283

@okohub
Copy link
Contributor Author

okohub commented Jul 28, 2021

Kindly ping :) @bsideup @kiview @rnorth

@okohub okohub force-pushed the f-azure-cosmosdb branch 2 times, most recently from 237ed74 to 03c8825 Compare July 30, 2021 22:50
@okohub okohub changed the title Azure Cosmosdb: Introduce CosmosDbEmulatorContainer Azure Cosmos DB: Introduce CosmosDBEmulatorContainer Jul 30, 2021
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Hi @okohub,

thanks a lot for this module, this will be a great addition and it is very much appreciated!
I have some general remarks, afterwards, it would be good, if someone with more CosmosDB background looks at it as well.

@okohub
Copy link
Contributor Author

okohub commented Aug 3, 2021

@kiview @rnorth ready to another review :) thanks for your help.

@kiview
Copy link
Member

kiview commented Aug 3, 2021

Thanks for the quick turnover @okohub, this looks much better, great work!
I will check in detail tomorrow.

@bsideup bsideup added this to the next milestone Aug 4, 2021
@okohub
Copy link
Contributor Author

okohub commented Aug 4, 2021

@kiview @bsideup done with latest reviews.

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks a lot.

Maybe @bsideup can have a final look as well.

@okohub
Copy link
Contributor Author

okohub commented Aug 4, 2021

@kiview @bsideup

ok so, why not have both :)

I added a KeyStoreUtils as before I did, with ssl disabled okhttp. Now we can also build key store from downloaded pem file. I kept both buildKeyStore methods to give flexibility to developer.

We can also describe both methods in documentations by the way.

@rnorth
Copy link
Member

rnorth commented Sep 27, 2021

I was going to suggest adding a quick bit of code to make sure we cleanup our system properties within our tests, as I have the fear that one day some tests within our wider test suite could break after the trust store modifications.

When trying out a quick patch locally, I encountered something very surprising: the tests now fail.

ERROR 🐳 [mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator:2.11.13-c5c131e3] - Log output from the failed container:
Error: The evaluation period has expired.
./palrun: ERROR: PAL initialization failed. Error: 104

This obviously worked previously, and doesn't happen when using the latest tag of the image, so I can only imagine that the pinned tagged images have some kind of expiry built in.

For the first time it seems that we can't pin an image to a specific version, which is quite alarming from a reproduceability perspective. Clearly the real CosmosDB service is itself evolving, but not being able to pin the version used in testing will be a headache.

Is there any possibility that I'm wrong? Can we somehow pin the image and not have it expire?

@okohub
Copy link
Contributor Author

okohub commented Sep 30, 2021

@bsideup I added Junit TemporaryFolder rule, I didn't know that thank you.

@rnorth I checked latest tags here:

https://mcr.microsoft.com/v2/cosmosdb/linux/azure-cosmos-emulator/tags/list

and re-read documents here

https://docs.microsoft.com/en-us/azure/cosmos-db/linux-emulator

but I didn't see a way to workaround expire problem. Looks like all emulators have 164 days to expire after they built. May be we should ask it to @JonathanGiles

I was going to suggest adding a quick bit of code to make sure we cleanup our system properties within our tests, as I have the fear that one day some tests within our wider test suite could break after the trust store modifications.

When trying out a quick patch locally, I encountered something very surprising: the tests now fail.

ERROR 🐳 [mcr.microsoft.com/cosmosdb/linux/azure-cosmos-emulator:2.11.13-c5c131e3] - Log output from the failed container:
Error: The evaluation period has expired.
./palrun: ERROR: PAL initialization failed. Error: 104

This obviously worked previously, and doesn't happen when using the latest tag of the image, so I can only imagine that the pinned tagged images have some kind of expiry built in.

For the first time it seems that we can't pin an image to a specific version, which is quite alarming from a reproduceability perspective. Clearly the real CosmosDB service is itself evolving, but not being able to pin the version used in testing will be a headache.

Is there any possibility that I'm wrong? Can we somehow pin the image and not have it expire?

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

It's a shame that we have to use a :latest image for our tests, but I don't think this should be a blocker. LGTM, thanks for your hard work!

@rnorth
Copy link
Member

rnorth commented Oct 4, 2021

Ohoh, this is unfortunate:

Execution failed for task ':azure:japicmp'.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.
> Could not resolve all files for configuration ':azure:baseline'.

   > Could not find org.testcontainers:azure:1.16.0.
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
     Searched in the following locations:     

We need to work out a way to prevent the japicmp checks from failing on new modules...

@bsideup
Copy link
Member

bsideup commented Oct 4, 2021

@rnorth on a second thought (after the review)... Does it mean our users will have to use :latest as well? 🥲

@rnorth
Copy link
Member

rnorth commented Oct 4, 2021

We can see that the azure:test task passed and then azure:japicmp then failed for entirely predictable reasons.

I was going to say: with this in mind I'm inclined to merge and then revisit japicmp's resolution of new modules if/when necessary.

However, I realise that this will also break master branch until the next release, so we probably don't have the option to defer this 😬

@rnorth
Copy link
Member

rnorth commented Oct 4, 2021

@rnorth on a second thought (after the review)... Does it mean our users will have to use :latest as well? 🥲

@bsideup yes it does indeed 😬

The slight silver lining compared to modules for 'self hosted' services is that the real prod cloud service is ever-evolving. However, being unable to pin versions and have repeatable tests does add quite a bit of displeasure.

@bsideup
Copy link
Member

bsideup commented Oct 4, 2021

Hey @JonathanGiles! Is there absolutely nothing that can be done to stop CosmosDB Docker images from "expiring"?
-SNAPSHOT style test dependencies tend to make test suites very unreliable, and we wanted to make sure that our Azure support is solid 🦾 Thanks in advance!

@okohub
Copy link
Contributor Author

okohub commented Oct 6, 2021

@rnorth after reading again your review, this sentence drew my attention

I was going to suggest adding a quick bit of code to make sure we cleanup our system properties within our tests, as I have the fear that one day some tests within our wider test suite could break after the trust store modifications.

May be we can cover test method with try/finally and set parameters to default state.

Something like that:

String defaultTrustStore = null;
String defaultTrustStorePassword = null;
String defaultTrustStoreType = null;
try {
defaultTrustStore = getprop
defaultTrustStorePassword = getprop
defaultTrustStoreType = getprop

//do some serious thing

} finally {
 if (defaultTrustStore != null) {
    setprop defaultTrustStore
  }
  //and others
}

@rnorth
Copy link
Member

rnorth commented Oct 8, 2021

@okohub thanks for remembering!

I'll add a suggested change, which I think reduces the amount of noise inside the test method.

@rnorth
Copy link
Member

rnorth commented Oct 8, 2021

Actually please could you have a look at this commit and use that?

@okohub
Copy link
Contributor Author

okohub commented Oct 8, 2021

@rnorth Of course! I will give it a try, btw I want to squash these commit flood to single one. Is it ok?

@rnorth
Copy link
Member

rnorth commented Oct 9, 2021

@okohub Sure, if you want! We will squash before merging anyway, so you only need to squash commits if it helps you.

@okohub
Copy link
Contributor Author

okohub commented Oct 9, 2021

@rnorth thanks! Also I added your prop suggestion and rebased into one commit.

@rnorth
Copy link
Member

rnorth commented Oct 11, 2021

@rnorth thanks! Also I added your prop suggestion and rebased into one commit.

Thanks @okohub !

@kiview kiview merged commit 1ad2a05 into testcontainers:master Oct 12, 2021
@kiview
Copy link
Member

kiview commented Oct 12, 2021

Merged, thanks a lot @okohub for your patience and your commitment to getting this PR merged, much appreciated 🙇

@RikEnde
Copy link

RikEnde commented Feb 9, 2022

hi @okohub do you have an example of a working project using this container with the latest version of the image?

@kiview
Copy link
Member

kiview commented Feb 10, 2022

Check the code example in our docs, which are executed as part of our CI:
https://www.testcontainers.org/modules/azure/#cosmosdb

@okohub okohub deleted the f-azure-cosmosdb branch January 6, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants