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

feat(elasticsearch): add option to skip cert retrieval #2530

Closed

Conversation

ImFlog
Copy link

@ImFlog ImFlog commented May 3, 2024

What does this PR do?

Add a new option to skip certificate retrieval.

Why is it important?

In some edge cases, it seems that the certificates are not available on the container and even if elasticsearch is working, the containers throw an error.

Related issues

Link related issues below. Insert the issue link or reference after the word "Closes" if merging this should automatically close it.

How to test this PR

There should be no impact because the default behavior is still to retrieve the certs. One can use the WithoutCertRetrieval function in the RunContainer method.

@ImFlog ImFlog requested a review from a team as a code owner May 3, 2024 20:39
Copy link

netlify bot commented May 3, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit e42fd71
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/666828eab46f680008b1e5bb
😎 Deploy Preview https://deploy-preview-2530--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Hi @ImFlog thanks for opening this PR and my apologies for the radio silence. May has been a month with PTO and full focus on an eventual v1 release of testcontainers-go, so please forgive me if this review took that long.

Back to the PR, I do not see any harm in adding it, although it seems a way to bypass the errors getting the certificate, right? Could you elaborate a bit more on this, please?

In some edge cases

If you relate to https://github.com/ImFlog/repro-testcontainer-es/blob/e2647d980919911fa8d652ac42186cdaebdb8e74/main.go#L22, I see that the code is binding the data dir, which lives next to the config (the location of the certs: why then are the certs affected by that binding?

As I said, this PR LGTM, but first I'd like to understand more why the certs are not available in that repro code.

Cheers!

docs/modules/elasticsearch.md Outdated Show resolved Hide resolved
Co-authored-by: Manuel de la Peña <[email protected]>
@ImFlog
Copy link
Author

ImFlog commented Jun 11, 2024

Hello @mdelapenya, no worries for the delay, I am using testcontainers without the module in the meantime :)

The tricky part is I don't understand why the cert is impacted by the fact that I map a data dir. This doesn't really make sense to me and after some debugging I couldn't find anything in the base image that could provoke this behavior.
Having the data bind directly at startup is pretty important to me to load my Elasticsearch with actual data for my tests but I cannot because of this behavior.

In #2207 It seems that another case to trigger this is when xpack is disabled (which makes more sense).

As I don't really care about certificates in my integration tests I thought that we could explicitly bypass the cert retrieval (even if it's not the cleanliest).

@mdelapenya
Copy link
Member

Could you add a test where you're able to preload the index? That will help me debug and review it more in depth

Thanks!

@ImFlog
Copy link
Author

ImFlog commented Jun 14, 2024

In the base issue I created a repository that reproduce the issue : https://github.com/ImFlog/repro-testcontainer-es
Do you need something else ?

@mdelapenya
Copy link
Member

mdelapenya commented Jun 14, 2024

Hi @ImFlog I noticed the repro version uses testcontainers-go v0.27.0, but in #2475 we added the very same feature, but using certain xpack properties. So if you upgrade to the v0.31.0 release, the latest, then you'll have the same behavior you proposed here.

In the repro, apart from bumping to the latest and greatest release, add this functional option to the RunContainer func:

		testcontainers.WithEnv(map[string]string{
			"xpack.security.enabled": "false",
		}),

And I think this is more Elasticsearch-idiomatic, as whoever knowing about Elastic configuration will understand properly what's going on here (ex-Elastic here 🙋 😉)

So if you agree, I think we should close this one.

@ImFlog
Copy link
Author

ImFlog commented Jun 24, 2024

I just tried and indeed it works as expected. I don't understand why I didn't saw this other related issue.
I'll close this as well as the associated Issue.
Thank you for the help 🙏

@ImFlog ImFlog closed this Jun 24, 2024
@ImFlog ImFlog deleted the elasticsearch/skip-cert-retrieval branch June 24, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Adding host config bind breaks the ElasticSearch cert retrieval
2 participants