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

agent spec bypass proxy for cloud metadata #785

Merged
merged 3 commits into from
Apr 19, 2023

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Apr 12, 2023

  • May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
    • n/a
  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
    To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents) : Do not use proxy when querying cloud metadata from APM agents #786
  • If this spec adds a new dynamic config option, add it to central config. n/a

@SylvainJuge SylvainJuge marked this pull request as ready for review April 13, 2023 07:09
@SylvainJuge SylvainJuge requested review from a team as code owners April 13, 2023 07:09
@SylvainJuge SylvainJuge removed request for a team April 13, 2023 07:09
specs/agents/metadata.md Outdated Show resolved Hide resolved
specs/agents/metadata.md Outdated Show resolved Hide resolved
specs/agents/metadata.md Outdated Show resolved Hide resolved
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Maybe add a small blurb as to why the agents need to do this? If I explicitly configure a proxy I would be surprised if we actively dismiss this configuration. I'm probably missing a lot of context though.

Also should we fallback to a call over the proxy?

@SylvainJuge
Copy link
Member Author

Maybe add a small blurb as to why the agents need to do this? If I explicitly configure a proxy I would be surprised if we actively dismiss this configuration. I'm probably missing a lot of context though.

Yes, it's definitely missing some context, would something like this provide enough ?

In the case where a proxy is configured on the application, the agents SHOULD attempt to make
the calls to the metadata endpoint directly, without using the proxy.
This is recommended as those HTTP calls could be caller-sensitive and have to be made directly
 by the virtual machine where the APM agent executes, also, the `169.254.x.x` IP address range
is reserved for "link-local" addresses that are not routed.

Regarding the caller-sensitivity I am just making an assumption, but I guess if you have an app host and a proxy, then doing this call from the proxy or the app host might definitely return different results, for example in the extreme case where they aren't in the same availability zone.

Also should we fallback to a call over the proxy?

You mean if it fails by calling it directly AND there is a proxy configured, then we do a fallback attempt to call it from the proxy ?
I think that would probably be a very small corner case, and given the fact that those addresses are not routed, then if they are not available is a good heuristic for "we are not running the expected cloud provider".

Also, it could even return invalid data if my caller-sensitivity assumption holds, for example with a self-hosted app host using a proxy deployed on AWS, in this case we don't want the app host to report being on AWS. It would also probably be dependent on the proxy implementation too, an AWS managed HTTP proxy service would not return its own reply for example.

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Thanks that makes total sense @SylvainJuge !

The added context LGTM too.

@jackshirazi
Copy link
Contributor

I can see a scenario where there is a proxy for testing which fakes the cloud provider endpoint, and the user would want the endpoint hit through the proxy. But also an edge case, I don't think worth catering for unless we get a customer request

@SylvainJuge SylvainJuge merged commit ecf4ce6 into elastic:main Apr 19, 2023
@SylvainJuge SylvainJuge deleted the metadata-avoid-proxy branch April 19, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants