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

[Serverless Mini Agent] Use Web PKI certificates for HTTPS Connections for Serverless Mini Agent #523

Merged
merged 11 commits into from
Jul 11, 2024

Conversation

duncanpharvey
Copy link
Contributor

@duncanpharvey duncanpharvey commented Jul 9, 2024

What does this PR do?

Use Web PKI certificates when establishing an HTTPS connection for the Serverless Mini Agent.

Motivation

Resolve certificate errors when running the Serverless Mini Agent in an Azure Function on Windows.

Error when on premium plan

2024-07-08T17:02:01Z   [Information]   thread 'tokio-runtime-worker' panicked at /Users/duncan.harvey/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-rustls-0.23.2/src/config.rs:31:62:
2024-07-08T17:02:01Z   [Information]   could not load platform certs: Os { code: 5, kind: PermissionDenied, message: "Access is denied." }

Error when on consumption plan

2024-07-10T12:23:06Z   [Information]   [2024-07-10T12:23:06Z ERROR datadog_trace_mini_agent::stats_flusher] Error sending stats: Failed to send trace stats: error trying to connect: invalid peer certificate: UnknownIssuer

Additional Notes

Adds feature use_webpki_roots to ddcommon to use Web PKI root certificates instead of native certifcates when establishing an HTTPS connection. Currently this is only enabled for the Serverless Mini Agent.

How to test the change?

  • Build mini agent locally
  • Deploy Azure Function or Google Cloud Function with mini agent binary in root path
  • Set DD_MINI_AGENT_PATH
    • Azure: /home/site/wwwroot/datadog-serverless-trace-mini-agent
    • Google: /workspace/datadog-serverless-trace-mini-agent

Azure

  • Node / Linux / Consumption Plan ✅
  • Node / Linux / Premium Plan ✅
  • .NET / Linux / Consumption Plan ✅
  • .NET / Linux / Premium Plan ▶️ tracer does not start serverless mini agent
  • Python / Linux / Consumption Plan ✅
  • Python / Linux / Premium Plan ▶️ tracer does not start serverless mini agent
  • Node / Windows / Consumption Plan ✅
  • Node / Windows / Premium Plan ⚠️ Only works for one function on a plan due to port conflicts, will address in a future PR
  • .NET / Windows / Consumption Plan ✅
  • .NET / Windows / Premium Plan ▶️ tracer does not start serverless mini agent

Google

  • Node ✅
  • Python ✅

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.08%. Comparing base (54708da) to head (a1fbde8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #523      +/-   ##
==========================================
- Coverage   70.25%   70.08%   -0.17%     
==========================================
  Files         206      206              
  Lines       27823    27810      -13     
==========================================
- Hits        19546    19491      -55     
- Misses       8277     8319      +42     
Components Coverage Δ
crashtracker 16.86% <ø> (ø)
datadog-alloc 98.73% <ø> (ø)
data-pipeline 51.15% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.41% <100.00%> (-0.28%) ⬇️
ddcommon-ffi 75.31% <ø> (ø)
ddtelemetry 59.02% <ø> (ø)
ipc 84.13% <ø> (ø)
profiling 78.04% <ø> (-0.65%) ⬇️
profiling-ffi 58.26% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 35.69% <ø> (ø)
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 70.93% <ø> (ø)
trace-normalization 98.24% <ø> (ø)
trace-obfuscation 95.73% <ø> (ø)
trace-protobuf 77.16% <ø> (ø)
trace-utils 89.91% <ø> (ø)

@ivoanjo
Copy link
Member

ivoanjo commented Jul 9, 2024

Hey! One thing that came to mind 🤔 : Since this change affects ddcommon, I suspect this will increase the size of libdatadog binaries even outside the serverless mini agent (e.g. for profiling).

It may be worth checking the libdatadog release sizes produced by GitLab CI with this PR to see if it's something we need to care about or not?

@pr-commenter
Copy link

pr-commenter bot commented Jul 9, 2024

Benchmarks

This comment was omitted because it was over 65536 characters.Please check the Gitlab Job logs to see its output.

@ivoanjo
Copy link
Member

ivoanjo commented Jul 10, 2024

Thanks for addressing my concern ❤️ 🙇

@duncanpharvey duncanpharvey marked this pull request as draft July 10, 2024 12:31
@duncanpharvey duncanpharvey marked this pull request as ready for review July 10, 2024 18:42
@duncanpharvey
Copy link
Contributor Author

@bwoebi @ivoanjo I added the use_webpki_roots feature to ddcommon to avoid affecting other code that uses the connector. Let me know if this is what you had in mind and if you think any other changes are necessary.

@duncanpharvey duncanpharvey changed the title [Serverless Mini Agent] Fall back to Web PKI Certificates if Unable to Load Platform Certificates [Serverless Mini Agent] Use Web PKI certificates for HTTPS Connections for Serverless Mini Agent Jul 10, 2024
Copy link

@joeyzhao2018 joeyzhao2018 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Yep, that's exactly what we had in mind :-)

@ivoanjo
Copy link
Member

ivoanjo commented Jul 11, 2024

Looks reasonable to me too! :)

@duncanpharvey duncanpharvey merged commit ccc5efc into main Jul 11, 2024
32 checks passed
@duncanpharvey duncanpharvey deleted the duncan-harvey/azure-function-windows-certs branch July 11, 2024 12:40
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.

5 participants