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

Inject HTTPS_PROXY/HTTP_PROXY env var into endpoint elasticsearch output units as proxy_url #5044

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

michel-laterman
Copy link
Contributor

@michel-laterman michel-laterman commented Jul 2, 2024

What does this PR do?

Inject proxy_url value into endpoint's elasticsearch output configuration, and enpoint/apm's fleet configuration if the attribute is missing and HTTPS_PROXY/HTTP_PROXY env var is set.

The first host value is used to determine if the HTTPS_PROXY, or HTTP_PROXY value is injected.
If that can't be used to determine then the HTTPS_PROXY is preferred.
No Injection occurs if the proxy_url key exists, proxy_disable: true is set, or the env vars are empty.

Why is it important?

Go HTTP clients automatically use various proxy env vars. This leads to inconsistent behaviour where some components get config from the environment, but others do not.
This effects endpoint specifically as it is not a go process.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

Related issues

@michel-laterman michel-laterman added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Jul 2, 2024
Copy link
Contributor

mergify bot commented Jul 2, 2024

This pull request does not have a backport label. Could you fix it @michel-laterman? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip label Jul 2, 2024
@michel-laterman michel-laterman changed the title wip wip: inject HTTPS_PROXY/HTTP_PROXY env var into output units as proxy_url Jul 2, 2024
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Reading the issue this seems like this should also be endpoint specific, so you should really be filter this only to the endpoint component. I don't think we want this to be done across the board, as passing the ENV down to the subprocesses already does the correct thing.

I believe there is already a endpoint specific component modifier, it might be better to integrate this there so multiple modifiers are not needed, each looping through all components and units each time.

See comment amount elasticsearch specific, we should ensure it also works for logstash. Format might be different.

@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@michel-laterman michel-laterman changed the title wip: inject HTTPS_PROXY/HTTP_PROXY env var into output units as proxy_url Inject HTTPS_PROXY/HTTP_PROXY env var into endpoint elasticsearch output units as proxy_url Jul 3, 2024
Copy link
Contributor

@blakerouse blakerouse 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 scoping this to endpoint and elasticsearch only. Good test coverage as well, nice!

@michel-laterman
Copy link
Contributor Author

@intxgo @ferullo
Any concerns about this approach?

@pierrehilbert pierrehilbert added the backport-8.15 Automated backport to the 8.15 branch with mergify label Jul 4, 2024
@mergify mergify bot removed the backport-skip label Jul 4, 2024
@intxgo
Copy link
Contributor

intxgo commented Jul 5, 2024

@intxgo @ferullo Any concerns about this approach?

The PR is incomplete, Endpoint requires fleet proxy as well. I guess that environment proxy also overrides fleet connection on Agent.

However, I'm concerned about the configuration update flow. Endpoint is caching permanently received config so that even if Agent gets compromised the machine remains protected by Endpoint (using last known configuration).

The configuration update protocol is using state index value to determine if the Expected and Actual config are the same or different, so I guess to have the proxy override applied the index would have to be bumped. Is the index solely managed by Agent or is it a Fleet policy property?
https:/elastic/elastic-agent-client/blob/94f110545198e910527a9db7e1a21f8553d70ea2/elastic-agent-client.proto#L185-L201

I've reviewed the configuration update flow, the config state index is only reported back, any configuration change will be applied.

@michel-laterman
Copy link
Contributor Author

Thanks @intxgo,
I've added a proxy_url injection into InjectFleetConfigComponentModifier to handle that area

@michel-laterman michel-laterman merged commit 097787f into elastic:main Jul 8, 2024
13 checks passed
@michel-laterman michel-laterman deleted the inject-proxy branch July 8, 2024 20:09
mergify bot pushed a commit that referenced this pull request Jul 8, 2024
…utput units as proxy_url (#5044)

Inject proxy_url value into endpoint's elasticsearch output configuration, and enpoint/apm's fleet configuration if the attribute is missing and HTTPS_PROXY/HTTP_PROXY env var is set.

The first host value is used to determine if the HTTPS_PROXY, or HTTP_PROXY value is injected.
If that can't be used to determine then the HTTPS_PROXY is preferred.
No Injection occurs if the proxy_url key exists, proxy_disable:  true is set, or the env vars are empty.

(cherry picked from commit 097787f)
michel-laterman added a commit that referenced this pull request Jul 8, 2024
…utput units as proxy_url (#5044) (#5083)

Inject proxy_url value into endpoint's elasticsearch output configuration, and enpoint/apm's fleet configuration if the attribute is missing and HTTPS_PROXY/HTTP_PROXY env var is set.

The first host value is used to determine if the HTTPS_PROXY, or HTTP_PROXY value is injected.
If that can't be used to determine then the HTTPS_PROXY is preferred.
No Injection occurs if the proxy_url key exists, proxy_disable:  true is set, or the env vars are empty.

(cherry picked from commit 097787f)

Co-authored-by: Michel Laterman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.15 Automated backport to the 8.15 branch with mergify enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forward HTTP_PROXY/HTTPS_PROXY/NO_PROXY to components.
5 participants