Deletion race condition when fetching secret from the Kubernetes secrets provider cache

[cmacknz]

• Relates Make it possible to view and debug the lifecycle of a secret populated by the kubernetes_secrets provider #6187

The Kubernetes secrets provider caches secrets and updates them on a configurable schedule to avoid placing unnecessary load on the Kubernetes API.

The cache is updated in a goroutine:

elastic-agent/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

Lines 88 to 90 in 54932dc

	if !p.config.DisableCache {
	go p.updateSecrets(ctx, comm)
	}

The cache is copied, the latest value for each non-expired secret is fetched, and then the copy is merged into the active map.

elastic-agent/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

Lines 164 to 199 in 54932dc

	// to not hold the lock for long, we copy the current state of the cache map
	copyMap := make(map[string]secretsData)
	p.secretsCacheMx.RLock()
	for name, data := range p.secretsCache {
	copyMap[name] = *data
	}
	p.secretsCacheMx.RUnlock()
	// The only way to update an entry in the cache is through the last access time (to delete the key)
	// or if the value gets updated.
	for name, data := range copyMap {
	diff := time.Since(data.lastAccess)
	if diff < p.config.TTLDelete {
	value, ok := p.fetchSecretWithTimeout(name)
	if ok {
	newData := &secretsData{
	value: value,
	lastAccess: data.lastAccess,
	}
	cacheTmp[name] = newData
	if value != data.value {
	updatedCache = true
	}
	}
	} else {
	updatedCache = true
	}
	}
	// While the cache was updated, it is possible that some secret was added through another go routine.
	// We need to merge the updated map with the current cache map to catch the new entries and avoid
	// loss of data.
	var updated bool
	p.secretsCacheMx.Lock()
	p.secretsCache, updated = p.mergeWithCurrent(cacheTmp)
	p.secretsCacheMx.Unlock()

The logic to get a value from the cache follows below.

elastic-agent/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

Lines 204 to 225 in 54932dc

	func (p *contextProviderK8sSecrets) getFromCache(key string) (string, bool) {
	p.secretsCacheMx.RLock()
	_, ok := p.secretsCache[key]
	p.secretsCacheMx.RUnlock()
	// if value is still not present in cache, it is possible we haven't tried to fetch it yet
	if !ok {
	value, ok := p.addToCache(key)
	// if it was not possible to fetch the secret, return
	if !ok {
	return value, ok
	}
	}
	p.secretsCacheMx.Lock()
	data, ok := p.secretsCache[key]
	data.lastAccess = time.Now()
	pass := data.value
	p.secretsCacheMx.Unlock()
	return pass, ok
	}

1. A read lock on the cache is taken to check if the value exists in the cache.
2. If it isn't, the secret is fetched which involves holding a write lock again to update the cache.
3. A write lock on the cache is taken and the cache is updated with the value from step 2 and the last access time is set.

The race condition is that after 1 and 2 have completed, the updateCache() method that can delete secrets from the cache could have run and deleted the secret right before 3 below where the value is returned directly from the cache with no check for whether the secret still exists.

elastic-agent/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

Lines 218 to 222 in 54932dc

	p.secretsCacheMx.Lock()
	data, ok := p.secretsCache[key]
	data.lastAccess = time.Now()
	pass := data.value
	p.secretsCacheMx.Unlock()

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[cmacknz]

There is another potential problem here, where time since last access is evaluated twice:

Once here where we refresh the value if the TTL hasn't expired, but the problem is if the value changed and agent should update the component model with the new value, we don't update the last access.

elastic-agent/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

Lines 174 to 182 in 54932dc

	for name, data := range copyMap {
	diff := time.Since(data.lastAccess)
	if diff < p.config.TTLDelete {
	value, ok := p.fetchSecretWithTimeout(name)
	if ok {
	newData := &secretsData{
	value: value,
	lastAccess: data.lastAccess,
	}

The TTL is then evaluated again when merging the maps which can lead to deletion even though the value updated:

elastic-agent/internal/pkg/composable/providers/kubernetessecrets/kubernetes_secrets.go

Lines 128 to 130 in 54932dc

	for name, data := range p.secretsCache {
	diff := time.Since(data.lastAccess)
	if diff < p.config.TTLDelete {

This would only matter if the TTLDelete and the secret rotation time were very close but we have an internal example of that.

[swiatekm]

The design of this provider doesn't sit well with me. What we're trying to do is very simple: Keep an up-to-date local cache of a small number of Secrets. Intuitively, it'd be best to have an informer for these. However, informers (and watches in general) don't have great support for selecting resources based on field names. It's possible to create an informer which tracks a particular singular Secret by using a field selector, but if we want multiple Secrets, we need to have an informer for each of them, which puts some undue strain on the Kubernetes API Server.

If this was anything other than Secrets, I'd strongly consider using a single informer and just filtering within the application, but cluster operators get understandably touchy about applications wanting to access a lot of Secrets, so this doesn't sound like the best of ideas either.

Maybe we should at least use a battle-tested off-the-shelf component for this instead of implementing an expiration cache ourselves and opening ourselves up to concurrency bugs. https://pkg.go.dev/k8s.io/client-go/tools/cache#ExpirationCache looks like it would fit.

@pkoutsovasilis maybe you'll have a better idea than me here. 🙏

[pkoutsovasilis]

The design of this provider doesn't sit well with me. What we're trying to do is very simple: Keep an up-to-date local cache of a small number of Secrets. Intuitively, it'd be best to have an informer for these. However, informers (and watches in general) don't have great support for selecting resources based on field names. It's possible to create an informer which tracks a particular singular Secret by using a field selector, but if we want multiple Secrets, we need to have an informer for each of them, which puts some undue strain on the Kubernetes API Server.

yes AFAIK multiple FieldSelectors (aka selecting by name) is not a thing in k8s. Chained selectors is the closest thing to "multi-values" fieldselectors but they are chained with logical "and" and we need logical "or" here, thus unusable.

If this was anything other than Secrets, I'd strongly consider using a single informer and just filtering within the application, but cluster operators get understandably touchy about applications wanting to access a lot of Secrets, so this doesn't sound like the best of ideas either.

As is now the agent RBAC requires just a "get" for just the secrets this provider has to "watch"; where with a single informer we would need "list", "watch" (and "get") for every secret out there. So just echoing the same message as the quote, not the best option

Maybe we should at least use a battle-tested off-the-shelf component for this instead of implementing an expiration cache ourselves and opening ourselves up to concurrency bugs. https://pkg.go.dev/k8s.io/client-go/tools/cache#ExpirationCache looks like it would fit.

Utilising a battle-tested off-the-shelf component seems like the best way forward 🙂

[cmacknz]

Utilising a battle-tested off-the-shelf component seems like the best way forward 🙂

+1 from me. TIL https://pkg.go.dev/k8s.io/client-go/tools/cache#ExpirationCache exists, thanks.