Skip to content

Commit

Permalink
🐛 Prevent race when informers are started more than once
Browse files Browse the repository at this point in the history
If `Informers` are started a second time, there is a possibility for a
data race because it sets a `ctx` field on itself. This write is
protected by a mutex, but reads from that field are not.
  • Loading branch information
alvaroaleman committed Apr 7, 2024
1 parent a17fd58 commit 8abc43a
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 5 deletions.
6 changes: 6 additions & 0 deletions pkg/cache/cache_test.go
Expand Up @@ -1849,6 +1849,12 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
)
})
Describe("as an Informer", func() {
It("should error when starting the cache a second time", func() {
err := informerCache.Start(context.Background())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Informer already started"))
})

Context("with structured objects", func() {
It("should be able to get informer for the object", func() {
By("getting a shared index informer for a pod")
Expand Down
7 changes: 7 additions & 0 deletions pkg/cache/internal/informers.go
Expand Up @@ -18,6 +18,7 @@ package internal

import (
"context"
"errors"
"fmt"
"math/rand"
"net/http"
Expand Down Expand Up @@ -186,6 +187,12 @@ type Informers struct {
// Start calls Run on each of the informers and sets started to true. Blocks on the context.
// It doesn't return start because it can't return an error, and it's not a runnable directly.
func (ip *Informers) Start(ctx context.Context) error {
select {
case <-ip.startWait:
return errors.New("Informer already started") //nolint:sylecheck

Check failure on line 192 in pkg/cache/internal/informers.go

View workflow job for this annotation

GitHub Actions / lint

ST1005: error strings should not be capitalized (stylecheck)

Check failure on line 192 in pkg/cache/internal/informers.go

View workflow job for this annotation

GitHub Actions / lint

ST1005: error strings should not be capitalized (stylecheck)
default:
// continue
}
func() {
ip.mu.Lock()
defer ip.mu.Unlock()
Expand Down
14 changes: 9 additions & 5 deletions pkg/cache/multi_namespace_cache.go
Expand Up @@ -163,12 +163,13 @@ func (c *multiNamespaceCache) GetInformerForKind(ctx context.Context, gvk schema
}

func (c *multiNamespaceCache) Start(ctx context.Context) error {
errs := make(chan error, 0)

Check failure on line 166 in pkg/cache/multi_namespace_cache.go

View workflow job for this annotation

GitHub Actions / lint

S1019: should use make(chan error) instead (gosimple)

Check failure on line 166 in pkg/cache/multi_namespace_cache.go

View workflow job for this annotation

GitHub Actions / lint

S1019: should use make(chan error) instead (gosimple)
// start global cache
if c.clusterCache != nil {
go func() {
err := c.clusterCache.Start(ctx)
if err != nil {
log.Error(err, "cluster scoped cache failed to start")
errs <- fmt.Errorf("failed to start cluster-scoped cache: %w", err)
}
}()
}
Expand All @@ -177,13 +178,16 @@ func (c *multiNamespaceCache) Start(ctx context.Context) error {
for ns, cache := range c.namespaceToCache {
go func(ns string, cache Cache) {
if err := cache.Start(ctx); err != nil {
log.Error(err, "multi-namespace cache failed to start namespaced informer", "namespace", ns)
errs <- fmt.Errorf("failed to start cache for namespace %s: %w", ns, err)
}
}(ns, cache)
}

<-ctx.Done()
return nil
select {
case <-ctx.Done():
return nil
case err := <-errs:
return err
}
}

func (c *multiNamespaceCache) WaitForCacheSync(ctx context.Context) bool {
Expand Down

0 comments on commit 8abc43a

Please sign in to comment.