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 for https in expvar scraping #1213

Merged
merged 5 commits into from
Jan 30, 2025

Conversation

cmetz100
Copy link
Contributor

@cmetz100 cmetz100 commented Jan 21, 2025

What does this PR do?

Adds support for https requests for scraping expvar. Currently I am allowing any certificate with no tls verification. This is most definitely a bad practice in general. For this specific use case it may be fine. After testing this I should be able to pass in the pem path used for tls validation if we dont want to allow any certificate.

Motivation

Error seen in this notebook

Update as of Jan 22:
The change to set trace agents debug endpoint to https has been reverted for now so this change is not needed yet.

Update as of Jan 29:
The change has landed. These errors will be present from now. I need to update expvar endpoint for quality gates in agent repo and finish this change and get a lading release out

Testing:
job_id:03321fcd-6cdb-45ee-b07d-0ca48e4848d0

  • comparison image is nightly from jan 10 (was hitting api limit when trying to pull todays, jan 30)
  • This version has the trace https change that was reapplied here
    -quality_gate_idle has expvar configured for https
    -quality_gate_all_features has expvar configured for http
    -quality_gate_logs has no expvar configured
    I expect to see the following:
  • comparison + quality_gate_idle => no errors (logs)
  • baseline + quality_gate_idle => cant get expvar endpoint since we are sending request to https port and 7.58 will expose over http (logs)
  • comparison + quality_gate_all_features => Http request sent to an https server error (logs)
  • baseline + quality_gate_all_features => no errors (logs)

SMPTNG-587

@GeorgeHahn
Copy link
Contributor

The mentioned PR DataDog/datadog-agent#32355 looks like it configures expvar to not use HTTPS, am I missing other context?

@cmetz100
Copy link
Contributor Author

cmetz100 commented Jan 21, 2025

The mentioned PR DataDog/datadog-agent#32355 looks like it configures expvar to not use HTTPS, am I missing other context?

This line I believe configures the expvar endpoint to be https. It turns out that the mentioned PR is most likely the root cause of the two other issues I have investigated in these two notebooks: 1 2. I also ssm'd into a machine to verify that expvars are only accessible over https not http

So in summary the next agent release will require us to configure the expvar path in lading.yaml to use https instead of http. However the previous releases all expose over http. In order to support the https request we either need to grab the cert it uses or allow no verification which is what i currently got going. Ideally we could keep the config in lading.yaml at http and automatically upgrade the request to https if the client can discern it is needed but that seems difficult as of now

@GeorgeHahn
Copy link
Contributor

@cmetz100 Ah, I follow now. I misunderstood the changes in that PR, thanks for the explanation.

@cmetz100 cmetz100 force-pushed the cmetz/enable_https_no_tls_verification branch 2 times, most recently from a65c8e7 to baeda34 Compare January 29, 2025 21:37
@cmetz100 cmetz100 marked this pull request as ready for review January 30, 2025 15:47
@cmetz100 cmetz100 requested a review from a team as a code owner January 30, 2025 15:47
Copy link
Contributor

@scottopell scottopell left a comment

Choose a reason for hiding this comment

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

Worth a small changelog entry I think, but fine as-is, up to you

@cmetz100
Copy link
Contributor Author

Worth a small changelog entry I think, but fine as-is, up to you

Agreed, I will add that now

@cmetz100 cmetz100 merged commit 89fbdb6 into main Jan 30, 2025
18 checks passed
@cmetz100 cmetz100 deleted the cmetz/enable_https_no_tls_verification branch January 30, 2025 16:40
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.

3 participants