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

Elasticsearch: Don't throw exception on missing CA cert file #5265

Merged
merged 5 commits into from
May 8, 2022

Conversation

spinscale
Copy link
Contributor

The current check throws an exception if the version is supposed to be
greater than 8 and the ca cert file is missing. This posed two problems:

  1. A custom docker image might have a 'latest' version, and thus is
    newer than version 8
  2. A user may have configured their own CA in a different path

In order to accomodate for this, a missing copy of a the ca crt file
will log a warning, but not throw an exception and leave the container
unstarted. There is also another public method, allowing to read the
cert, called extractCert.

Closes #5264

The current check throws an exception if the version is supposed to be
greater than 8 and the ca cert file is missing. This posed two problems:

1. A custom docker image might have a 'latest' version, and thus is
   newer than version 8
2. A user may have configured their own CA in a different path

In order to accomodate for this, a missing copy of a the ca crt file
will log a warning, but not throw an exception and leave the container
unstarted. There is also another public method, allowing to read the
cert, called `extractCert`.

Closes testcontainers#5264
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.

Thanks for the prompt investigation and fix @spinscale, much appreciated 🙌
I added a comment regarding the API.

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, thanks!
Only thing I was wondering if we should maybe also add a test utilizing withCertPath if it is not too involved.

@spinscale
Copy link
Contributor Author

Hm, I could copy another http_ca.crt into the container, but that would need to be done before Elasticsearch starts, is this possible? container.copyFileToContainer requires a running container IIRC

@kiview
Copy link
Member

kiview commented Apr 14, 2022

withCopyFileToContainer will perform the copying on startup 🙂

@spinscale
Copy link
Contributor Author

oooh... of course. thx. I took a small shortcut and added a http_ca.crt file, that will not match the one created by Elasticsearch on startup and therefore fails on purpose when trying to connect to Elasticsearch using that for a client with a handshake exception.

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

could you please add a test that verifies that #5264 works correctly now?

@spinscale
Copy link
Contributor Author

done, tagged an older image as latest and started the container. If there is a better way, let me know, I stole a little code from DockerComposeLocalImageTest

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

💯

@bsideup bsideup added this to the next milestone May 5, 2022
@kiview kiview merged commit b3b2a46 into testcontainers:master May 8, 2022
@kiview
Copy link
Member

kiview commented May 8, 2022

Merged, thanks a lot for this high-quality PR @spinscale, a pleasure to work with you 🥳

@vmassol
Copy link
Contributor

vmassol commented May 11, 2022

Hey guys (cc @spinscale), I'm using tag 8.2.0for ES and v1.17.1 for TC and I'm getting this exception:

13:16:56.855 [main] ERROR ?.e.c.2.0] - Could not start container
com.github.dockerjava.api.exception.NotFoundException: Status 404: {"message":"Could not find the file /usr/share/elasticsearch/config/certs/http_ca.crt in container 8edfaa2eef41d2bc12dae07291f3796973ce3670440d918fb7ec90a6a5410a96"}

I understand that this PR (when released), will not throw this exception anymore but that ES will still not start.

I'm starting it with the following code:

        DockerImageName imageName =
            DockerImageName.parse("docker.elastic.co/elasticsearch/elasticsearch").withTag("8.2.0");
        container = new ElasticsearchContainer(imageName);
        // - single node since we don't need a cluster for testing
        // - disabled security to avoid the warning messages and because security is not needed for testing as
        //   the data is discarded and the testing environment is secure.
        container.setEnv(List.of(
            "discovery.type=single-node",
            "xpack.security.enabled=false",
            "xpack.security.http.ssl.enabled=false",
            "xpack.security.transport.ssl.enabled=false"));
        container.start();

As you can see, I've tried to turn off security so I'm not sure why it doesn't work in 8.2.0. It works fine in 7.17.3.

Also, I'd be interested to understand how I can start ES 8.2.0 with TC in a secure mode. I've tried to read https://www.elastic.co/guide/en/elasticsearch/reference/current/docker.html but I'm not sure I understand everything. Am I supposed to provide the http_ca.crt file or is generated by the docker image? Step 4 suggests it's generated. So how are we supposed to use ES with TC? Do we have to start the container once, copy the crt file on the host, stop the container, restart it again with a volume mapping on the host for the cert file? Sounds pretty complex :)

Thx!

@spinscale
Copy link
Contributor Author

Can you try #5264 (comment) as a workaround until the next version is released?

@vmassol
Copy link
Contributor

vmassol commented May 11, 2022

@spinscale that works fine, thx!

@vmassol
Copy link
Contributor

vmassol commented May 24, 2022

I confirm it's now working OOB with TC 1.17.2. Thanks for that!

However, I still get a warning:

10:38:15.887 [main] WARN  o.t.e.ElasticsearchContainer - CA cert under /usr/share/elasticsearch/config/certs/http_ca.crt not found.

I have turned security off and thus wasn't expecting any warning. Is that something that could be improved? Should I create an issue for it @spinscale ?

       DockerImageName imageName =
           DockerImageName.parse("docker.elastic.co/elasticsearch/elasticsearch").withTag(ELASTICSEARCH_VERSION);
       ElasticsearchContainer container = new ElasticsearchContainer(imageName);
       // - single node since we don't need a cluster for testing
       // - disabled security to avoid the warning messages and because security is not needed for testing as
       //   the data is discarded and the testing environment is secure.
       container.setEnv(List.of("discovery.type=single-node", "xpack.security.enabled=false"));
       container.start();

Thx

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.

ElasticsearchContainer in 1.17.x does not handle latest tags correctly
5 participants