fix: use env prefix in collector config for typecasting #534
Conversation
WalkthroughThis pull request includes updates to several Helm chart files, primarily focusing on version increments and enhancements to configuration templates for the OpenTelemetry Collector. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Signed-off-by: Prashant Shahi <prashant@signoz.io>
![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
🧹 Outside diff range and nitpick comments (1)
charts/signoz/values.yaml (1)
1971-1972: Improved security and configurability in OpenTelemetry Collector exportersThe changes in the OpenTelemetry Collector exporters configuration are a significant improvement in terms of security and configurability. By using environment variables (
${env:VARIABLE_NAME}) for sensitive information such as usernames, passwords, and host details, the deployment becomes more secure and flexible.This approach allows for easier management of credentials and connection details across different environments without exposing sensitive information in the configuration files.
Consider applying this pattern consistently across other parts of the configuration where sensitive information or environment-specific values are used. This will further enhance the overall security and maintainability of the deployment.
Also applies to: 1974-1979, 2369-2371
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- charts/k8s-infra/Chart.yaml (1 hunks)
- charts/k8s-infra/templates/_config.tpl (1 hunks)
- charts/k8s-infra/values.yaml (1 hunks)
- charts/signoz/Chart.yaml (1 hunks)
- charts/signoz/values.yaml (2 hunks)
✅ Files skipped from review due to trivial changes (2)
- charts/k8s-infra/Chart.yaml
- charts/signoz/Chart.yaml
🧰 Additional context used
🔇 Additional comments (5)
charts/k8s-infra/values.yaml (2)
205-205: LGTM: Improved environment variable handlingThe change from
${K8S_HOST_IP}:10250to${env:K8S_HOST_IP}:10250enhances the configuration by explicitly markingK8S_HOST_IPas an environment variable. This aligns with the PR objective of using an env prefix in the collector config for typecasting, which should improve clarity and potentially prevent issues related to variable resolution.
Line range hint
1-1037: Verify the impact on the overall configurationThe change to use the
env:prefix forK8S_HOST_IPis the only modification in this file. While this change is beneficial for clarity and proper environment variable handling, it's important to ensure that this modification doesn't have any unintended consequences on the rest of the configuration or the system's behavior.To confirm that this change doesn't affect other parts of the system, please run the following verification script:
This script will help ensure that the change is consistent across the configuration and that the environment variable is set correctly in the Kubernetes resources.
charts/k8s-infra/templates/_config.tpl (3)
137-140: Ensure Environment Variables for OTLP Exporter are Properly SetThe OTLP exporter configuration now relies on environment variables for
endpoint,insecure, andinsecure_skip_verify. Please confirm that these environment variables (OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_INSECURE, andOTEL_EXPORTER_OTLP_INSECURE_SKIP_VERIFY) are correctly set in the deployment environment to ensure the exporter functions as intended.
142-145: Verify TLS Certificate Paths and Environment Variable ConfigurationThe TLS settings reference certificate files using the
OTEL_SECRETS_PATHenvironment variable:
cert_file:${env:OTEL_SECRETS_PATH}/cert.pemkey_file:${env:OTEL_SECRETS_PATH}/key.pemca_file:${env:OTEL_SECRETS_PATH}/ca.pem(if applicable)Please ensure that
OTEL_SECRETS_PATHis properly defined and that the certificate files exist at the specified locations. This is crucial for establishing secure connections.
149-149: Confirm Secure Handling ofSIGNOZ_API_KEYEnvironment VariableThe
signoz-access-tokenheader is set using the environment variable${env:SIGNOZ_API_KEY}. Ensure that theSIGNOZ_API_KEYis securely stored and managed according to best practices, and that it's correctly provided in the deployment environment to authenticate with the SigNoz backend.
Signed-off-by: Prashant Shahi <prashant@signoz.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
charts/k8s-infra/templates/otel-agent/daemonset.yaml (1)
125-131: Approve volume mount configuration with suggestions for documentation.The volume mount configuration for
hostfsis well-structured with appropriate security considerations:
- Read-only access limits the potential for accidental or malicious modifications.
- HostToContainer propagation is necessary for comprehensive metrics collection.
Consider adding inline comments or documentation to explain:
- The purpose of this mount and its relation to host metrics collection.
- The implications of HostToContainer propagation.
- Any specific metrics or data this mount enables the agent to collect.
This will help future maintainers understand the reasoning behind this configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- charts/k8s-infra/templates/otel-agent/daemonset.yaml (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
charts/k8s-infra/templates/otel-agent/daemonset.yaml (1)
56-61: Overall impact: Improved flexibility for host metrics collection.The changes successfully introduce the ability to collect host metrics when enabled, aligning with the PR objective. The conditional nature of these changes provides good flexibility for different deployment scenarios.
To ensure the changes work as intended and don't introduce any regressions:
- Test the DaemonSet deployment with
presets.hostMetrics.enabledset to bothtrueandfalse.- Verify that host metrics are correctly collected when enabled.
- Ensure that the DaemonSet functions correctly without host metrics when disabled.
#!/bin/bash # Check for any existing tests related to host metrics collection grep -r "hostMetrics" charts/k8s-infra/tests/If no existing tests are found, consider adding new ones to cover this functionality.
Also applies to: 125-131
Fixes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
k8s-infraandSigNozHelm charts to reflect recent updates.Documentation