Merged
Conversation
Add TLSConfigManager.DialWithDialer method. (cherry picked from commit a116576)
Small quality of life improvement by adding TLSConfigManager.UseTLS. Also contains some minor internal refactoring. (cherry picked from commit 3e6fcc0)
* fix: Clone *tls.Config returned by TLSConfigManager.TLSConfig TLSConfigManager.TLSConfig() will now always returned a cloned *tls.Config. This eliminates the need to manually cloning the returned *tls.Config before using it to Dial or Listen. It will also eliminate any bugs in current code which used the returned config without cloning it. * chore: fix issue in test code (cherry picked from commit f8351ea)
* feat: multiple TLS configuration improvements
Externally visible changes:
- New `[http] https-insecure-certificate` configuration allows skipping permission
checking for HTTP TLS certificate.
- New `[opentsdb] insecure-certificate` configuration allows skipping
permission checking for OpenTSDB endpoint certificates.
Internal changes:
- TLSConfigManager
- Allow skipping certificate permission checks with WithIgnoreFilePermissions
- Direct passthrough of TLSCertLoader options suported.
- Add support for client authentication using WithClientAuth
- Custom root CAs supported through WithRootCAIncludeSystem and
WithRootCAFiles
- Custom client CAs supported through WithClientCAIncludeSystem and
WithClientCAFiles
- Refactoring of construction code
- NewClientTLSManager now returns an error
- TLSCertLoader
- GetClientCertificate: Improve client and server side error messages
when the server will not accept the client certificate
- GetClientCertificate: error message improvement
- NewTLSCertLoader no longer waits for certificate monitoring goroutine
to start before returning. Test code that needs to wait for this should
use TLSCertLoader.WaitForMonitorStart.
- TLSCertLoaderOpt constructors include TLSCertLoader in their name
(cherry picked from commit 85353b3)
There was a problem hiding this comment.
Pull request overview
Backports the tlsconfig.TLSConfigManager feature set to the 1.12 branch to centralize TLS config creation, certificate loading/reloading, and related logging/CA configuration, and updates services to use the new manager.
Changes:
- Add
pkg/tlsconfig.TLSConfigManagerwith helpers forListen,Dial, and certificate reload orchestration. - Extend TLS handling with CA pool options, client auth support, and file-permission checking controls; refactor
TLSCertLoaderoption naming and monitor startup behavior. - Wire
httpdandopentsdbservices (and sample config) to use the new manager and add “insecure-certificate” config flags.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/opentsdb/service.go | Switch OpenTSDB listener/cert reload logic to use TLSConfigManager; add insecure cert-permissions flag plumbing. |
| services/opentsdb/config.go | Add insecure-certificate configuration field. |
| services/httpd/service.go | Replace direct TLSCertLoader usage with TLSConfigManager; add insecure cert-permissions flag plumbing. |
| services/httpd/config.go | Add https-insecure-certificate config (but currently has a duplicate field). |
| pkg/tlsconfig/configmanager.go | New TLSConfigManager implementation with options for CA pools, client auth, cert loader passthrough, dialing/listening, and reload support. |
| pkg/tlsconfig/configmanager_test.go | New comprehensive tests for TLSConfigManager behaviors. |
| pkg/tlsconfig/certconfig.go | Extend LoadCertificate with options (ignore perms), refactor TLSCertLoader opts naming, add monitor start coordination, improve errors. |
| pkg/tlsconfig/certconfig_test.go | Update tests for renamed TLSCertLoader opts and monitor-start semantics; adjust GetClientCertificate expectations. |
| etc/config.sample.toml | Document new insecure-certificate knobs and TLS wording updates. |
Comments suppressed due to low confidence (1)
pkg/tlsconfig/certconfig_test.go:299
- These logging tests can become flaky now that the monitor goroutine may emit the same warning before this first assertion runs (depending on scheduling and
testCheckTime). Instead ofrequire.Len(..., 1), consider asserting at least one matching warning (and then clearing the observer) or usingrequire.Eventuallyaround the expected log emission.
checkWarning := func(t *testing.T) {
warning := logs.FilterMessage("Certificate is not valid yet").TakeAll()
require.Len(t, warning, 1)
require.Equal(t, zap.WarnLevel, warning[0].Level)
require.Equal(t, ss.CertPath, warning[0].ContextMap()["cert"])
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
tlsconfig.TLSConfigManagerfor managing TLS configurations, handling certificate reloads, logging certificate expiration warnings, etc.This is backport of the following master-1.x PRs to the 1.12 branch: