agents: add root certs API and use it in OTLPAgent#340
agents: add root certs API and use it in OTLPAgent#340santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
Conversation
WalkthroughThe changes centralize root CA certificates by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant OTLPAgent
participant OTLPMetrics
participant ExporterOptions
participant RootCerts
Main->>OTLPAgent: Construct OTLPAgent()
OTLPAgent->>RootCerts: GetRootCerts() and GetRootCertsCount()
RootCerts-->>OTLPAgent: Return embedded root cert strings
OTLPAgent->>OTLPAgent: Concatenate root certs into cacert_
Main->>OTLPAgent: config_otlp_endpoint(config)
OTLPAgent->>OTLPMetrics: Construct OTLPMetrics(..., cacert_)
OTLPMetrics->>ExporterOptions: Set SSL/TLS CA cert to cacert_
OTLPAgent->>ExporterOptions: Set CA cert for trace exporters to cacert_
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
agents/otlp/src/otlp_agent.cc (1)
104-107: Consider optimizing string concatenation.The current implementation is correct but could be optimized for better performance.
Consider using a more efficient approach for string concatenation:
- for (size_t i = 0; i < sizeof(root_certs) / sizeof(root_certs[0]); i++) { - cacert_ += root_certs[i]; - cacert_ += "\n"; - } + size_t total_size = 0; + constexpr size_t cert_count = sizeof(root_certs) / sizeof(root_certs[0]); + for (size_t i = 0; i < cert_count; i++) { + total_size += strlen(root_certs[i]) + 1; // +1 for newline + } + + cacert_.reserve(total_size); + for (size_t i = 0; i < cert_count; i++) { + cacert_ += root_certs[i]; + cacert_ += "\n"; + }This reserves the required capacity upfront, reducing memory reallocations during concatenation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
agents/otlp/src/otlp_agent.cc(5 hunks)agents/otlp/src/otlp_agent.h(1 hunks)agents/otlp/src/otlp_metrics.cc(3 hunks)agents/otlp/src/otlp_metrics.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: coverage-linux
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-windows
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: build-docs
- GitHub Check: coverage-linux-without-intl
🔇 Additional comments (5)
agents/otlp/src/otlp_agent.h (1)
153-153: LGTM! Clean addition of certificate storage member.The addition of the
cacert_member is well-placed and appropriately typed for storing the embedded root CA certificates.agents/otlp/src/otlp_metrics.h (1)
34-35: LGTM! Consistent constructor signature updates.The constructors are correctly updated to accept the
cacertparameter usingconst std::string&, which avoids unnecessary copying and follows C++ best practices.Also applies to: 41-42
agents/otlp/src/otlp_metrics.cc (1)
63-64: LGTM! Proper SSL certificate configuration implementation.The constructor implementations correctly apply the
cacertparameter to the appropriate SSL configuration fields for both HTTP (ssl_ca_cert_string) and gRPC (ssl_credentials_cacert_as_string) exporters, ensuring consistent certificate usage across all OTLP metrics exporters.Also applies to: 69-69, 74-74, 83-84, 93-93, 98-98
agents/otlp/src/otlp_agent.cc (2)
48-50: LGTM! Proper certificate embedding setup.The inclusion of
node_root_certs.hand static array declaration provides a clean way to embed root certificates directly in the binary.
550-550: LGTM! Consistent certificate usage across all OTLP exporters.The implementation correctly applies the embedded root certificates to both metrics and trace exporters (HTTP and gRPC variants), ensuring consistent SSL/TLS configuration across all OTLP communications.
Also applies to: 575-575, 594-594, 601-601
santigimeno
force-pushed
the
santi/otlp_cacert_load
branch
from
80842c5 to
496b992
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
agents/src/http_client.cc (1)
42-45: Optimize loop performance by caching the certificate count.The loop logic is correct, but calling
GetRootCertsCount()in each iteration is inefficient. Cache the count in a variable for better performance.- for (size_t i = 0; i < GetRootCertsCount(); i++) { + size_t cert_count = GetRootCertsCount(); + for (size_t i = 0; i < cert_count; i++) { cacert_ += GetRootCerts()[i]; cacert_ += "\n"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
agents/grpc/src/grpc_agent.cc(2 hunks)agents/otlp/src/otlp_agent.cc(5 hunks)agents/otlp/src/otlp_agent.h(1 hunks)agents/otlp/src/otlp_metrics.cc(3 hunks)agents/otlp/src/otlp_metrics.h(1 hunks)agents/src/http_client.cc(2 hunks)agents/src/root_certs.cc(1 hunks)agents/src/root_certs.h(1 hunks)node.gyp(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- node.gyp
- agents/src/root_certs.cc
- agents/src/root_certs.h
- agents/grpc/src/grpc_agent.cc
🚧 Files skipped from review as they are similar to previous changes (4)
- agents/otlp/src/otlp_agent.h
- agents/otlp/src/otlp_metrics.cc
- agents/otlp/src/otlp_metrics.h
- agents/otlp/src/otlp_agent.cc
🧰 Additional context used
🧬 Code Graph Analysis (1)
agents/src/http_client.cc (2)
agents/src/root_certs.cc (4)
GetRootCertsCount(14-16)GetRootCertsCount(14-14)GetRootCerts(10-12)GetRootCerts(10-10)agents/src/root_certs.h (2)
GetRootCertsCount(12-12)GetRootCerts(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test-macOS
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-linux
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: build-tarball
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-windows
🔇 Additional comments (1)
agents/src/http_client.cc (1)
2-2: LGTM: Include change aligns with centralized root certificate management.The change from
"util.h"to"root_certs.h"correctly reflects the new centralized interface for root certificate access.
Also, make all our existing agents code to use it. Ideally having an API like this being exposed from the `node::crypto` namespace (crypto_context specifically) would be ideal, but for the time being this should do.
santigimeno
changed the title
santigimeno
force-pushed
the
santi/otlp_cacert_load
branch
from
496b992 to
d543dd2
Compare
Also, make all our existing agents code to use it. Ideally having an API like this being exposed from the `node::crypto` namespace (crypto_context specifically) would be ideal, but for the time being this should do. PR-URL: #340 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Also, make all our existing agents code to use it. Ideally having an API like this being exposed from the `node::crypto` namespace (crypto_context specifically) would be ideal, but for the time being this should do. PR-URL: #340 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Also, make all our existing agents code to use it. Ideally having an API like this being exposed from the `node::crypto` namespace (crypto_context specifically) would be ideal, but for the time being this should do. PR-URL: #340 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Also, make all our existing agents code to use it. Ideally having an API like this being exposed from the `node::crypto` namespace (crypto_context specifically) would be ideal, but for the time being this should do. PR-URL: #340 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
3 participants
Also, make all our existing agents code to use it.
Ideally having an API like this being exposed from the
node::cryptonamespace (crypto_context specifically) would be ideal, but for the time
being this should do.
Summary by CodeRabbit
Summary by CodeRabbit