matchish / laravel-scout-elasticsearch

Search among multiple models with ElasticSearch and Laravel Scout
MIT License
702 stars 113 forks source link

Add ssl verify config option #255

Closed danielsimkus closed 1 year ago

danielsimkus commented 1 year ago

This adds the ability to disable SSL Verification using the environment variable: ELASTICSEARCH_SSL_VERIFICATION=false

Notes

When trying to test that the SSL verification is actually disabled correctly, I couldn't really compare against the following as it's all private:

dd($client->getTransport()->getClient());
Symfony\Component\HttpClient\Psr18Client^ {#8779
  -client: Symfony\Component\HttpClient\CurlHttpClient^ {#8771
    -defaultOptions: array:31 [
      "verify_host" => false
      "verify_peer" => false

I ended up needing to add the ClientBuilder to be loaded by the container so that I can verify the sslVerification method is being called correctly using mocks

danielsimkus commented 1 year ago

Had a million issues with the local test environment (doesn't support ARM, getting DB setup errors on linux), and we don't actually need this I just thought it may be useful. Closing for now..

hulkur commented 2 months ago

@matchish please consider merging this

Our client has internal install with self-signed cert.

matchish commented 2 months ago

I'm not sure we should always configure the ES client directly from the app config. For larger projects, it's likely you'll end up with a custom ES client service provider configured to your specific project needs. Basic ES client included just for quicker start with the package It is described in the README https://github.com/matchish/laravel-scout-elasticsearch?tab=readme-ov-file#rocket-installation

The package uses \ElasticSearch\Client from official package, but does not try to configure it, so feel free do it in your app service provider. But if you don't want to do it right now, you can use Matchish\ElasticSearchServiceProvider from the package. Register the provider, adding to config/app.php 'providers' => [ // Other Service Providers \Matchish\ScoutElasticSearch\ElasticSearchServiceProvider::class ],

Of course If you still think we should be able to configure it on package config level, I would like to hear your arguments

hulkur commented 2 months ago

can't speak for large projects

for less complex projects it seems redundant to duplicate whole service provider just to add ->setSSLVerification(false)

Can't selectively change things as AppServiceProvider is called first and ElasticSearchProvider would overwrite bindings.

matchish commented 2 months ago

@hulkur Maybe you're right but as it is I would like to not merge it. Tests should be removed(in this case mocks are not welcome) as well as ClientBuilder part.

matchish commented 2 months ago

Feel free to adjust the PR and I'll merge it