feat: use recommended cluster settings for cockroachdb

[martskins]

What does this PR do?

This PR configures the cockroachdb container with the settings recommended by Cockroach Labs for testing clusters.

Why is it important?

It seems reasonable to follow their advise but the why for each of these settings is explained in the link above.

Related issues

None

How to test this PR

I did some rather naive testing by running a test suite with and without these changes and it seemed like it did work in that the the tests took less time to finish with these settings than without them. It's not clear to me whether that improvement will be there in all possible scenarios but, as I said before, I think it's reasonable to follow Cockroach Labs advise on this.

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	ff63c4c
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/6723ae021b3b4300084b7b18
😎 Deploy Preview	https://deploy-preview-2858--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mdelapenya]

LGTM, I left a minor comment.

I wonder if we always want to add those recommendations, or wrap them into a functional opt so users can decide whether to apply them or not 🤔

[mdelapenya]

BTW I'd rename the title to feat:, as we are adding a new behaviour, not fixing a broken one

[martskins]

LGTM, I left a minor comment.

I wonder if we always want to add those recommendations, or wrap them into a functional opt so users can decide whether to apply them or not 🤔

Would you add one single WithRecommendedClusterSettings or one option for each of the settings? I'm leaning towards the former but happy to do either.

[mdelapenya]

Given all of us agree, I'd go with a single functional option for settings, as pointed out by @stevenh. What I'm not sure if we want to append the default settings to those passed by the user (I like this, although it could come with some conflicts)

[stevenh]

Given all of us agree, I'd go with a single functional option for settings, as pointed out by @stevenh. What I'm not sure if we want to append the default settings to those passed by the user (I like this, although it could come with some conflicts)

We could export a variable to be used e.g. cockroachdb.WithSettings(cockroachdb.ClusterDefaults...) to avoid having to deal with the conflicts.

[martskins]

Given all of us agree, I'd go with a single functional option for settings, as pointed out by @stevenh. What I'm not sure if we want to append the default settings to those passed by the user (I like this, although it could come with some conflicts)

We could export a variable to be used e.g. cockroachdb.WithSettings(cockroachdb.ClusterDefaults...) to avoid having to deal with the conflicts.

Would you make those configurable? Thinking that if we expose that as something configurable then a struct would probably be better but it would be weird if we only add these specific settings to the struct and leave out all the other configurable stuff of the cluster.

I suppose, given that these are just statements we run after ready, we could also just expose this as a non-configurable PostReadies function that the user just plugs in there instead of trying to name these as settings. And if they want something else they just specific the function themselves?

[mdelapenya]

I'd go simple: a WithClusterSettings functional opt that is able to execute them in the cluster. As @stevenh suggested, having a variable with default cluster settings seems good, so we can mention about its existence both in the $ROOT_DIR/docs/modules/cockroachdb page and in the functional option comments, so users are aware of its existence. Finally, this option would contribute the result to the PostReadies.

[stevenh]

I'd go simple: a WithClusterSettings functional opt that is able to execute them in the cluster. As @stevenh suggested, having a variable with default cluster settings seems good, so we can mention about its existence both in the $ROOT_DIR/docs/modules/cockroachdb page and in the functional option comments, so users are aware of its existence. Finally, this option would contribute the result to the PostReadies.

Thinking about it I wouldn't call it WithClusterSettings as it could be used to apply all sorts of settings, so something more generic like WithStatements for the functional option and then the specifics are exposed in the variable passed in.

[martskins]

Just updated the PR. Let me know if that's roughly what you had in mind.

[stevenh]

Thanks for making all these changes, we're getting close to a solid solution I think.

[stevenh]

LGTM

[mdelapenya]

LGTM, thank you very much and sorry if the review process took longer than expected 🙏

[stevenh]

Looks like this fails due the example not working with TLS, see here.

[martskins]

Looks like this fails due the example not working with TLS, see here.

So I found two issues... First one was that the user you pass in the options when setting up the cluster needs to have MODIFYCLUSTERSETTING privilege to run the statements. I added a note about that in the DefaultStatements documentation and changed the non-root tests to not use the default statements, but not sure if that's good enough.

Second one was the one you mentioned, it seems like these default statements don't play nicely with TLS. I couldn't find any documentation from Cockroach Labs that would help me understand why this would be the case. I tried running only some of these statements but I couldn't really make it work with any combination that I tried. Should we maybe error if default statements and TLS are used?

[stevenh]

suggestion: This is somewhat confusing as this won't be combined with DefaultStatements which is how I would interpret this currently. We should clarify and ensure we clearly state the default if not configured is DefaultStatements.

[mdelapenya]

If we are always adding the default statements, then I think they must be private, and probably prepended to the user-provided ones.

[martskins]

If we are always adding the default statements, then I think they must be private, and probably prepended to the user-provided ones.

I see what you mean, but given the complications with TLS and users without MODIFYCLUSTERSETTING privilege I think prepending to the user-provided statements is probably going to cause a bit of pain.

Good spot on the comment. I'll hold on that change until we've established what we do about the TLS and user privileges thing though, as it might affect this.

[stevenh]

Private vs public for these is an interesting discussion, here's some ideas from the perspective of making the var private:

• Pro: Others can't mess with them
• Conn: Others can't correct if needed
• Conn: Can't pass them into WithStatements to customise
• Conn: Not self documenting.

[mdelapenya]

One of the goals of any testcontainers module is to be simple, and maybe opinionated, helping users in not committing mistakes. So I'd look for the mechanism that creates this experience.

Whatever you find that matches that, it's fine for me 😊

[stevenh]

Looks like this fails due the example not working with TLS, see here.

So I found two issues... First one was that the user you pass in the options when setting up the cluster needs to have MODIFYCLUSTERSETTING privilege to run the statements. I added a note about that in the DefaultStatements documentation and changed the non-root tests to not use the default statements, but not sure if that's good enough.

Second one was the one you mentioned, it seems like these default statements don't play nicely with TLS. I couldn't find any documentation from Cockroach Labs that would help me understand why this would be the case. I tried running only some of these statements but I couldn't really make it work with any combination that I tried. Should we maybe error if default statements and TLS are used?

I believe TLS issue is down to how the self signed cert is created by the module in that it only supports the default user, which seems like an unnecessary restriction.

[martskins]

Looks like this fails due the example not working with TLS, see here.

So I found two issues... First one was that the user you pass in the options when setting up the cluster needs to have MODIFYCLUSTERSETTING privilege to run the statements. I added a note about that in the DefaultStatements documentation and changed the non-root tests to not use the default statements, but not sure if that's good enough.
Second one was the one you mentioned, it seems like these default statements don't play nicely with TLS. I couldn't find any documentation from Cockroach Labs that would help me understand why this would be the case. I tried running only some of these statements but I couldn't really make it work with any combination that I tried. Should we maybe error if default statements and TLS are used?

I believe TLS issue is down to how the self signed cert is created by the module in that it only supports the default user, which seems like an unnecessary restriction.

I'm not sure I understand. Should it be failing in the main branch if that was the case? I haven't modified those failing tests or anything around TLS that would cause it fail. Also, commenting out the added statements seemed to make the tests pass for me.

Apologies if I'm misunderstanding something here.

[stevenh]

I believe TLS issue is down to how the self signed cert is created by the module in that it only supports the default user, which seems like an unnecessary restriction.

I'm not sure I understand. Should it be failing in the main branch if that was the case? I haven't modified those failing tests or anything around TLS that would cause it fail. Also, commenting out the added statements seemed to make the tests pass for me.

Apologies if I'm misunderstanding something here.

I had a look into this in more detail and the connections handling in this module for TLS is not clear, it actually requires the user to jump through hoops to get it right combining a base URL with TLS setting. You've fallen foul of this in the way you're using the connection string in runStatements.

I've pushed a commit to a separate draft PR which fixes that.

[martskins]

I believe TLS issue is down to how the self signed cert is created by the module in that it only supports the default user, which seems like an unnecessary restriction.

I'm not sure I understand. Should it be failing in the main branch if that was the case? I haven't modified those failing tests or anything around TLS that would cause it fail. Also, commenting out the added statements seemed to make the tests pass for me.
Apologies if I'm misunderstanding something here.

I had a look into this in more detail and the connections handling in this module for TLS is not clear, it actually requires the user to jump through hoops to get it right combining a base URL with TLS setting. You've fallen foul of this in the way you're using the connection string in runStatements.

I've pushed a commit to a separate draft PR which fixes that.

Ooh that's a good find! I'm happy to close this one and go with yours then, sounds like the easier path.

[stevenh]

Closing in favour of #2869