Skip to content

[smartagent] Use system cert pool as default on Windows #6240

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

Merged
merged 5 commits into from
May 21, 2025

Conversation

pjanotti
Copy link
Contributor

Description:
The system certificate pool should be the default on Windows, the same behavior of the other supported OSes.

Testing:
Added 2 new tests to cover the new default.

Documentation:
N/A this should be correct default.

@pjanotti pjanotti requested review from a team as code owners May 20, 2025 22:01
@pjanotti pjanotti requested a review from Copilot May 20, 2025 22:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adjusts the default behavior on Windows to use the system certificate pool, aligning Windows with other supported operating systems.

  • Added tests to validate the behavior in both HTTP and TLS contexts
  • Removed the explicit Windows branch in the TLS configuration code
  • Updated the changelog to document the bug fix

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
internal/signalfx-agent/pkg/monitors/http/http_test.go Added test to verify HTTP monitor metrics when checking Microsoft
internal/signalfx-agent/pkg/core/common/auth/tls_test.go Added test to ensure CertPool returns the system certificate pool
internal/signalfx-agent/pkg/core/common/auth/tls.go Updated CertPool to always use the system certificate pool, including on Windows
CHANGELOG.md Documented change in behavior for Windows certificate pool usage
Comments suppressed due to low confidence (1)

internal/signalfx-agent/pkg/core/common/auth/tls.go:57

  • Update the function comment for CertPool to reflect that it now returns the system certificate pool on all platforms, including Windows.
func CertPool() (*x509.CertPool, error) {

Copy link

codecov bot commented May 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.79%. Comparing base (8049f0f) to head (f8dc019).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6240      +/-   ##
==========================================
+ Coverage   44.42%   44.79%   +0.37%     
==========================================
  Files         390      390              
  Lines       26986    26983       -3     
==========================================
+ Hits        11988    12088     +100     
+ Misses      14156    14022     -134     
- Partials      842      873      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pjanotti pjanotti merged commit 7e5cb58 into signalfx:main May 21, 2025
233 checks passed
@pjanotti pjanotti deleted the fix-default-certpool-on-windows branch May 21, 2025 14:11
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants