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

added support to read proxy settings from env vars #2571

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chris-gunawardena
Copy link

@chris-gunawardena chris-gunawardena commented May 20, 2024

part of #2017

This solves the issue of the connectors ignoring system proxy settings in env vars.

Added trust_env=True according to https://docs.aiohttp.org/en/stable/client_advanced.html#aiohttp-client-proxy-support

The use of proxy is needed when running the connector alongside elastic cluster behind a corporate firewall, a proxy is used to expose outgoing traffic. For example, to externally SaaS products like ServiceNow, Sharepoint intranet, Teams and Confluence

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Considered corresponding documentation changes
  • Contributed any configuration settings changes to the configuration reference
  • [] if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana

Changes Requiring Extra Attention

  • Security-related changes (encryption, TLS, SSRF, etc)
  • New external service dependencies added.

Related Pull Requests

#2266

Work around

In case this doesn't go through, it's possible to do this in the docker build with RUN sed -i 's/aiohttp.ClientSession(/aiohttp.ClientSession(trust_env=True,/g' /app/connectors/sources/sharepoint_online.py

@chris-gunawardena chris-gunawardena requested a review from a team May 20, 2024 17:53
Copy link

cla-checker-service bot commented May 20, 2024

❌ Author of the following commits did not sign a Contributor Agreement:
8dab873,

Please, read and sign the above mentioned agreement if you want to contribute to this project

@navarone-feekery
Copy link
Contributor

@chris-gunawardena thanks for the contribution! I think this is a good change to add to Connectors. There are a few things we need to work on before merging first.

  1. I think it will be important that it is a configurable value. Ideally this would be configurable from the config.yml file. I'll try and find an example of how this could be done.
  2. This doesn't completely solve the proxy issue linked, because ENV vars can't be set on cloud. We still think it's worth merging for users running on self-managed infrastructure. Can you change the PR link to the issue to Relates to instead of Closes?
  3. Also, do you have any links to documentation that explains which ENV vars can be used like this?

@MatheusGelinskiPires
Copy link

@chris-gunawardena thanks for the contribution! I think this is a good change to add to Connectors. There are a few things we need to work on before merging first.

  1. I think it will be important that it is a configurable value. Ideally this would be configurable from the config.yml file. I'll try and find an example of how this could be done.
  2. This doesn't completely solve the proxy issue linked, because ENV vars can't be set on cloud. We still think it's worth merging for users running on self-managed infrastructure. Can you change the PR link to the issue to Relates to instead of Closes?
  3. Also, do you have any links to documentation that explains which ENV vars can be used like this?

We also need to add proxy support between the Connector and the Elasticsearch endpoint.

@seanstory
Copy link
Member

We also need to add proxy support between the Connector and the Elasticsearch endpoint.

I think that's why @navarone-feekery asked to change the PR description from closes to relates to. This change will have value on its own, even without also adding proxy support for Elasticsearch, which can be added in a separate PR.

@seanstory
Copy link
Member

buildkite test this

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.

4 participants