Skip to content

Commit 084267c

Browse files
Merge branch 'main' into patch-1
2 parents 6a39c07 + 0ffbf34 commit 084267c

30 files changed

Lines changed: 253 additions & 184 deletions

File tree

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. receiver/otlp)
7+
component: pkg/config/configtls
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Fix goroutine and file descriptor leak when `client_ca_file_reload` is enabled
11+
12+
# One or more tracking issues or pull requests related to the change
13+
issues: [9221]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext: |
19+
Every call to `ServerConfig.LoadTLSConfig` with `client_ca_file_reload` enabled started a
20+
file watcher goroutine that could never be stopped, since the reloader was not reachable
21+
from the returned `*tls.Config`. The client CA file is now checked for changes while TLS
22+
handshakes are served, at most once per second, matching how `reload_interval` already
23+
reloads the server certificate. No background goroutine is started, so nothing is left
24+
behind when a server is torn down and recreated.
25+
26+
# Optional: The change log or logs in which this entry should be included.
27+
# e.g. '[user]' or '[user, api]'
28+
# Include 'user' if the change is relevant to end users.
29+
# Include 'api' if there is a change to a library API.
30+
# Default: '[user]'
31+
change_logs: [user]

cmd/otelcorecol/go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ require (
4444
github.com/ebitengine/purego v0.10.0 // indirect
4545
github.com/felixge/httpsnoop v1.0.4 // indirect
4646
github.com/foxboron/go-tpm-keyfiles v0.0.0-20251226215517-609e4778396f // indirect
47-
github.com/fsnotify/fsnotify v1.10.1 // indirect
4847
github.com/go-logr/logr v1.4.3 // indirect
4948
github.com/go-logr/stdr v1.2.2 // indirect
5049
github.com/go-ole/go-ole v1.2.6 // indirect

cmd/otelcorecol/go.sum

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

config/configgrpc/go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ require (
3535
github.com/cespare/xxhash/v2 v2.3.0 // indirect
3636
github.com/davecgh/go-spew v1.1.1 // indirect
3737
github.com/foxboron/go-tpm-keyfiles v0.0.0-20251226215517-609e4778396f // indirect
38-
github.com/fsnotify/fsnotify v1.10.1 // indirect
3938
github.com/go-logr/logr v1.4.3 // indirect
4039
github.com/go-logr/stdr v1.2.2 // indirect
4140
github.com/go-viper/mapstructure/v2 v2.5.0 // indirect

config/configgrpc/go.sum

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

config/confighttp/go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ require (
4242
github.com/davecgh/go-spew v1.1.1 // indirect
4343
github.com/felixge/httpsnoop v1.0.4 // indirect
4444
github.com/foxboron/go-tpm-keyfiles v0.0.0-20251226215517-609e4778396f // indirect
45-
github.com/fsnotify/fsnotify v1.10.1 // indirect
4645
github.com/go-logr/logr v1.4.3 // indirect
4746
github.com/go-logr/stdr v1.2.2 // indirect
4847
github.com/go-viper/mapstructure/v2 v2.5.0 // indirect

config/confighttp/go.sum

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

config/confighttp/xconfighttp/go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ require (
1818
github.com/davecgh/go-spew v1.1.1 // indirect
1919
github.com/felixge/httpsnoop v1.0.4 // indirect
2020
github.com/foxboron/go-tpm-keyfiles v0.0.0-20251226215517-609e4778396f // indirect
21-
github.com/fsnotify/fsnotify v1.10.1 // indirect
2221
github.com/go-logr/logr v1.4.3 // indirect
2322
github.com/go-logr/stdr v1.2.2 // indirect
2423
github.com/go-viper/mapstructure/v2 v2.5.0 // indirect

config/confighttp/xconfighttp/go.sum

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

config/configtls/clientcasfilereloader.go

Lines changed: 84 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -4,49 +4,93 @@
44
package configtls // import "go.opentelemetry.io/collector/config/configtls"
55

66
import (
7+
"crypto/sha256"
78
"crypto/tls"
89
"crypto/x509"
9-
"errors"
1010
"fmt"
11+
"os"
12+
"path/filepath"
1113
"sync"
12-
13-
"github.com/fsnotify/fsnotify"
14+
"time"
1415
)
1516

17+
// defaultClientCAsReloadInterval bounds how often the client CA file is checked
18+
// for modifications. The check only happens while TLS handshakes are being
19+
// served, so an idle server does no work at all.
20+
const defaultClientCAsReloadInterval = time.Second
21+
1622
type clientCAsFileReloader struct {
17-
clientCAsFile string
23+
clientCAsFile string
24+
loader clientCAsFileLoader
25+
// reloadInterval is the minimum duration between two checks of
26+
// clientCAsFile. Overridden by tests.
27+
reloadInterval time.Duration
28+
29+
lock sync.Mutex
1830
certPool *x509.CertPool
1931
lastReloadError error
20-
lock sync.RWMutex
21-
loader clientCAsFileLoader
22-
watcher *fsnotify.Watcher
23-
shutdownCH chan bool
32+
// lastCheck is when clientCAsFile was most recently checked for changes.
33+
lastCheck time.Time
34+
// fileID identifies the contents of clientCAsFile as of the last check.
35+
fileID fileIdentity
36+
}
37+
38+
// fileIdentity identifies the contents of a file by hash.
39+
//
40+
// Hashing the contents rather than comparing os.Stat metadata keeps detection
41+
// independent of how the file came to be updated. In particular, Kubernetes
42+
// projects ConfigMap and Secret volumes as a symlink to a timestamped directory
43+
// and swaps that symlink on update, so the path being read resolves to a
44+
// different underlying inode each time; and file modification timestamps carry
45+
// filesystem-dependent granularity. Reading the file each check sidesteps both.
46+
// The read is bounded by reloadInterval and CA bundles are small.
47+
type fileIdentity struct {
48+
exists bool
49+
sum [sha256.Size]byte
50+
}
51+
52+
// identifyFile returns the identity of the file at path, following symlinks. A
53+
// path that cannot be read yields the zero identity, which never compares equal
54+
// to that of a readable file, so a file that disappears or reappears is always
55+
// treated as a change.
56+
func identifyFile(path string) fileIdentity {
57+
contents, err := os.ReadFile(filepath.Clean(path))
58+
if err != nil {
59+
return fileIdentity{}
60+
}
61+
return fileIdentity{exists: true, sum: sha256.Sum256(contents)}
2462
}
2563

2664
type clientCAsFileLoader interface {
2765
loadClientCAFile() (*x509.CertPool, error)
2866
}
2967

3068
func newClientCAsReloader(clientCAsFile string, loader clientCAsFileLoader) (*clientCAsFileReloader, error) {
69+
// Identify the file before loading it, so that a change racing with the
70+
// initial load is detected by the next check rather than being missed.
71+
fileID := identifyFile(clientCAsFile)
72+
3173
certPool, err := loader.loadClientCAFile()
3274
if err != nil {
3375
return nil, fmt.Errorf("failed to load client CA CertPool: %w", err)
3476
}
3577

36-
reloader := &clientCAsFileReloader{
37-
clientCAsFile: clientCAsFile,
38-
certPool: certPool,
39-
loader: loader,
40-
shutdownCH: nil,
41-
watcher: nil,
42-
}
43-
44-
return reloader, nil
78+
return &clientCAsFileReloader{
79+
clientCAsFile: clientCAsFile,
80+
loader: loader,
81+
reloadInterval: defaultClientCAsReloadInterval,
82+
certPool: certPool,
83+
lastCheck: time.Now(),
84+
fileID: fileID,
85+
}, nil
4586
}
4687

4788
func (r *clientCAsFileReloader) getClientConfig(original *tls.Config) (*tls.Config, error) {
48-
r.lock.RLock()
49-
defer r.lock.RUnlock()
89+
r.lock.Lock()
90+
defer r.lock.Unlock()
91+
92+
r.reloadIfModified(time.Now())
93+
5094
return &tls.Config{
5195
RootCAs: original.RootCAs,
5296
GetCertificate: original.GetCertificate,
@@ -59,84 +103,35 @@ func (r *clientCAsFileReloader) getClientConfig(original *tls.Config) (*tls.Conf
59103
}, nil
60104
}
61105

62-
func (r *clientCAsFileReloader) reload() {
63-
r.lock.Lock()
64-
defer r.lock.Unlock()
106+
// reloadIfModified reloads the client CA CertPool if clientCAsFile has changed
107+
// since the last check. Checks are rate limited to one per reloadInterval. A
108+
// failed reload is recorded and leaves the previously loaded CertPool in place,
109+
// so that a truncated or malformed file cannot break client authentication.
110+
//
111+
// The caller must hold r.lock.
112+
func (r *clientCAsFileReloader) reloadIfModified(now time.Time) {
113+
if now.Sub(r.lastCheck) < r.reloadInterval {
114+
return
115+
}
116+
r.lastCheck = now
117+
118+
fileID := identifyFile(r.clientCAsFile)
119+
if fileID == r.fileID {
120+
return
121+
}
122+
r.fileID = fileID
123+
65124
certPool, err := r.loader.loadClientCAFile()
66125
if err != nil {
67126
r.lastReloadError = err
68-
} else {
69-
r.certPool = certPool
70-
r.lastReloadError = nil
127+
return
71128
}
129+
r.certPool = certPool
130+
r.lastReloadError = nil
72131
}
73132

74133
func (r *clientCAsFileReloader) getLastError() error {
75134
r.lock.Lock()
76135
defer r.lock.Unlock()
77136
return r.lastReloadError
78137
}
79-
80-
func (r *clientCAsFileReloader) startWatching() error {
81-
if r.shutdownCH != nil {
82-
return errors.New("client CA file watcher already started")
83-
}
84-
85-
watcher, err := fsnotify.NewWatcher()
86-
if err != nil {
87-
return fmt.Errorf("failed to create watcher to reload client CA CertPool: %w", err)
88-
}
89-
r.watcher = watcher
90-
91-
err = watcher.Add(r.clientCAsFile)
92-
if err != nil {
93-
return fmt.Errorf("failed to add client CA file to watcher: %w", err)
94-
}
95-
96-
r.shutdownCH = make(chan bool)
97-
go r.handleWatcherEvents()
98-
99-
return nil
100-
}
101-
102-
func (r *clientCAsFileReloader) handleWatcherEvents() {
103-
defer r.watcher.Close()
104-
for {
105-
select {
106-
case _, ok := <-r.shutdownCH:
107-
_ = ok
108-
return
109-
case event, ok := <-r.watcher.Events:
110-
if !ok {
111-
continue
112-
}
113-
// NOTE: k8s configmaps uses symlinks, we need this workaround.
114-
// original configmap file is removed.
115-
// SEE: https://martensson.io/go-fsnotify-and-kubernetes-configmaps/
116-
if event.Has(fsnotify.Remove) || event.Has(fsnotify.Chmod) {
117-
// remove the watcher since the file is removed
118-
if err := r.watcher.Remove(event.Name); err != nil {
119-
r.lastReloadError = err
120-
}
121-
// add a new watcher pointing to the new symlink/file
122-
if err := r.watcher.Add(r.clientCAsFile); err != nil {
123-
r.lastReloadError = err
124-
}
125-
r.reload()
126-
}
127-
if event.Has(fsnotify.Write) {
128-
r.reload()
129-
}
130-
}
131-
}
132-
}
133-
134-
func (r *clientCAsFileReloader) shutdown() error {
135-
if r.shutdownCH == nil {
136-
return errors.New("client CAs file watcher is not running")
137-
}
138-
r.shutdownCH <- true
139-
close(r.shutdownCH)
140-
r.shutdownCH = nil
141-
return nil
142-
}

0 commit comments

Comments
 (0)