Allow defining proxy settings for connection

In the current build (8.0.0-alpha2), I see no implementation that uses Drupal's HTTP proxy settings. This version of Elasticsearch Connector uses elasticsearch/elasticsearch 8, which should support proxies through the ClientBuilder's setHttpClientOptions() method, as described here: https://www.elastic.co/guide/en/elasticsearch/client/php-api/8.15/http-client.html.

Is it correct to say that proxy support is missing from this module?

Changing the version number

@murrow, you're correct there's currently no support for proxy connection.

@sokru, I have updated the issue summary with a proposed solution. The dependency injection part will disturb the code a bit, but I don't think the other tasks will have much impact. What are your thoughts?

Further update to the summary problem section

Adding test task

I pushed a solution using ClientBuilder::setHttpClient.
I also made it possible for Connector extending the StandardConnector to alter the Client without having to redefine it completely.

Sorry for the noise; I haven't had a chance to review the change yet.

I rebased onto the latest 8.0.x, so the phpstan pipeline wouldn't warn us about a deprecation

@murrow, @hexaki, thank you both very much for your contributions!

I've had a chance to look at the code in the merge request, and I have some feedback.

Before I give my feedback, note that I am assuming that, because you moved the Issue Status to "Needs review", you have finished implementing the full solution that you want. If that's not the case (i.e.: you still have work to do), please move it back to "Needs work" until you're done! Thank you! (it's hard for me to tell because I don't have the context that you do — I've never needed to connect to Elasticsearch through a proxy!)

Without further ado, here is my feedback...

1. The Issue Title says "Allow defining proxy settings for connection", so I was expecting to see some configuration and/or UI in the Merge Request to do that.
   (note that I may have misunderstood the Issue Title, and my expectation may be wrong!)
   1. The code in the Merge Request seems to change the specifics of how the Elasticsearch Client object gets built. Has the Issue Title become out-of-sync with the solution? If so, can you update the Issue Title?
      • For example, If you're using some other module (e.g.: cURL HTTP Request) or service (e.g.: Guzzle) to change the proxy settings, please explain what you're doing, so that I understand! (this will also document a solution for other community members who need to do the same thing that you are doing but don't know how!)
2. In the Issue Summary, under Proposed resolution, it says "We should be able to use the elasticsearch/elasticsearch ClientBuilder::setHttpClientOptions method to add any Drupal HTTP proxy settings into the call if we have Guzzle and Curl as our transports." so I was expecting to see a call to setHttpClientOptions() in the merge request, but I don't see anything like that.
   (note that I may have misunderstood the issue summary and my expectation may be wrong)
   1. If this is no longer the solution we're going for, can you update the issue summary to explain what the new solution is?
3. Would you be able to update the Remaining tasks in the Issue Summary to explain what else needs to be done? @sokru added the Needs issue summary update tag, please remove when you're done updating the issue summary.
4. The merge request doesn't have any automated tests. Automated tests ultimately benefit you, because they ensure that future changes to the module contributed by other people won't break the functionality that your site depends on. If you need help writing tests, please ask us! I've added the Needs tests tag, please remove it when you've added some tests.

Since I have some questions for you, I'm changing the status to Postponed (maintainer needs more info). When you've answered the questions, please move the issue back to Needs review if you're done, or Needs work if you have more work to do!

Some other notes...

I fixed a spelling mistake in a docblock in the code.

I've updated the Issue Summary a bit based on my understanding of the problem and your solution. Please review it, and fix it if I've made a mistake.

I removed the proxy and proxy variables issue tags, as those are non-standard. See this list of standard tags.

I changed this to a "Feature request" to reflect that you're asking for a feature that doesn't currently exist in the module. See also @sokru's comment in #4.

Hello, thanks @mparker17 for your feedback.

1. My understanding was that the proxy is usually an environment configuration and Drupal already has the means to configure it via the settings.php. So I didn't see the need for the proxy to be redefined in the server configuration.
I'm not used to contrib modules, and I have access to the entire site configuration (config and settings), so it seemed the easiest solution.

Do you rather like a full config solution ?

2. My propsed solution is : Use Drupal's "HttpClient" as in the \Elastic\Elasticsearch\ClientBuilder. This way the proxy is defined by $settings['http_client_config']['proxy']

3. Next step is validating this solution and writing tests.

4. I could use help writing a test for this solution (any guideline is fine).

MR96 at commit c0ce23116b39467d4c2bb857d7a75a5d3037c0ef works for me.

settings.php

<?php
$settings['http_client_config'] = [
  'proxy' => [
    'http' => getenv('HTTP_PROXY') ?: NULL,
    'https' => getenv('HTTPS_PROXY') ?: NULL,
    'no' => array_filter(explode(',', getenv('NO_PROXY') ?: '')),
  ],
];

I rebased onto 8.0.x. I added the ability to use Drupal core's http_client service (which knows about the proxy) from the newer ElasticCloudEndpointConnector and ElasticCloudIdConnector connectors.

I was on the fence about adding a getClientBuilder() method to the Standard connector (or presumably, all connector plugins), but I ultimately decided that, to keep it simple, I wouldn't add it (which adds a bit of duplication). I think it's worth exploring if it would be worth adding a connector base class, but I daresay that's out-of-scope for this issue.

This merge request also changes nearly the same part of the code as #3529647: Fix logging in BasicAuthConnector, meaning there's a chance those two changes will conflict... so I set the HTTP Client before the Hosts: hopefully Git can still figure out how to auto-merge them. If not, it should be relatively simple to resolve a merge conflict if that happens anyway.

I'm not sure how we can test this so I'm removing the "Needs tests" tag. Happily, it seems like pipeline 801875 passed, so that tells me we didn't break anything, although our current CI environment doesn't have a proxy configured.

I would appreciate it if @sabsbecool, @murrow, @hexaki, @sweetchuck, and/or @sokru could test out the updated changes and let me know if it still works.

Created merge request !204 for the 9.0.x branch; updated this issue's version, and added the "Needs backport" tag so I don't forget to merge the 8.0.x MR.

I haven't heard back from anyone in 6 days, but I want to make this issue part of the next release, so I'm going to merge this one now.

Had to rebase and fix a merge conflict; then made a follow-up change to make merges easier in the future.

D10 an D11 tests pass, so I'm going to merge.

Merged to 9.0.x; merging to 8.0.x next.

Merged to 8.0.x. I will update this when I make a the next release.

The changes in this issue were released in elasticsearch_connector-9.0.0-alpha3, and elasticsearch_connector-8.0.0-alpha7