crossplane-contrib / crossplane-contrib/provider-upjet-github

Provider config cache is keyed by name only, serving stale credentials after a ProviderConfig changes

Open
#327 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
56
Forks
46
Avg merge
6d 7h
Merged PRs (30d)
2

Description

We noticed that the provider targets a wrong organization after deleting and re-creating our configuration without a longer delay between the operations. The `ProviderConfig` is cached and the provider uses the cached version for about 30 minutes even if you only change the contents but keep the name. Below is an AI generated analysis:

## Summary

`TerraformSetupBuilder` caches a fully configured `terraform.Setup` keyed **only by ProviderConfig name**, and invalidates it **only on a time-based TTL**. When a ProviderConfig's content changes while its name stays the same, the provider keeps authenticating with the superseded credentials until the TTL expires or the pod restarts.

The cached `terraform.Setup` carries both `Configuration` (including `owner`) and `Meta` (the authenticated GitHub client), so a stale entry does not merely use an old token — it also targets the **old organization**.

Verified against `v0.20.0`.

## Observed symptom

A ProviderConfig was deleted and re-created under the same name, resolving to a different organization with a different credentials secret. Every GitHub managed resource then failed with:

```
POST https:///orgs//teams: 401 Bad credentials
```

The giveaway is that `` appeared in **no** object on the cluster. The managed resources, the ProviderConfig and the referenced secret all named the *current* organization. The only place the old value still existed was the provider's in-memory cache. Restarting the provider pod cleared it immediately.

That mismatch is a useful diagnostic in general: a value that appears in no spec on the cluster can only be coming from provider memory.

## Root cause

`internal/clients/github.go`:

- L148-151 — `CachedTerraformSetup` stores `setup` + `expiry` only, with no record of what the setup was built from.
- L233 / L247 / L255 — the cache is read and written as `cache[configRefName]`. The ProviderConfig **name** is the entire key.
- L257 — `expiry: now().Add(ttl)` with `tfSetupCacheTTL = githubInstallationTokenLifetime - tfSetupMaxHold` = 30m (L168). Expiry is anchored to the **build**; a cache hit at L235 returns without extending it.

So an entry is replaced only when it ages out. Content is never consulted, and a ProviderConfig whose credentials or `owner` changed in place is indistinguishable from one that did not.

## When this bites

The stale window is bounded by the TTL measured from the last *build*, so a long enough gap is safe. Two cases are not:

- **Recreating a ProviderConfig under the same name within the TTL.** Realistic whenever a failed provisioning is torn down and immediately retried, and reliably reproducible in automated provision/teardown test loops. Note the window can extend past the obvious: managed resources keep reconciling during teardown, so the entry may be rebuilt late in the teardown and the clock restarts from there.
- **In-place credential rotation — no deletion involved at all.** Rotating the GitHub App private key, or repointing `spec.credentials.secretRef` at a new secret, leaves the ProviderConfig name unchanged, so the provider keeps using the superseded credentials for up to the TTL. This is the most likely way to hit it in day-to-day operation, and the failure is silent until something 401s.

## Reproducer

Drop into `internal/clients/`. Fails on `v0.20.0` as-is:

```go
func TestGetOrBuildTerraformSetup_StaleAfterCredentialsChange(t *testing.T) {
var lock sync.RWMutex
cache := map[string]CachedTerraformSetup{}
frozen := func() time.Time { return time.Unix(0, 0) }

build := func(owner string) func() (terraform.Setup, error) {
return func() (terraform.Setup, error) {
return terraform.Setup{
Configuration: terraform.ProviderConfiguration{"owner": owner},
}, nil
}
}

if _, err := getOrBuildTerraformSetup(&lock, cache, "pc", frozen, tfSetupCacheTTL, build("org-old")); err != nil {
t.Fatal(err)
}

// "pc" is deleted and re-created, now resolving to a different org with
// different credentials. Time has not advanced, so the cached entry is
// still well inside its TTL.
ps, err := getOrBuildTerraformSetup(&lock, cache, "pc", frozen, tfSetupCacheTTL, build("org-new"))
if err != nil {
t.Fatal(err)
}

if got := ps.Configuration["owner"]; got != "org-new" {
t.Errorf("owner = %q, want %q: cache served a Setup built from superseded credentials", got, "org-new")
}
}
```

Actual output:

```
--- FAIL: TestGetOrBuildTerraformSetup_StaleAfterCredentialsChange (0.00s)
owner = "org-old", want "org-new": cache served a Setup built from superseded credentials
```

which is exactly the production symptom.

## Suggested fix

Resolve the ProviderConfig and its credentials on every call, digest the resulting provider configuration, and treat a cached entry as usable only when the digest matches:

```go
type CachedTerraformSetup struct {
setup *terraform.Setup
fingerprint string
expiry time.Time
}

func (c CachedTerraformSetup) usableAt(t time.Time, fingerprint string) bool {
return c.setup != nil && c.fingerprint == fingerprint && c.expiry.After(t)
}
```

This keeps the optimisation the cache exists for. Resolving the ProviderConfig and reading the secret are served from the controller-runtime cache and never touch GitHub; only `configureNoForkGithubClient` mints an installation token, and that stays behind the cache. Since `encoding/json` sorts map keys, a `sha256` over the marshalled `terraform.ProviderConfiguration` is a stable digest. It must not be logged — the configuration contains the App private key.

A per-ProviderConfig `singleflight.Group` (`golang.org/x/sync` is already an indirect dependency) would be a reasonable companion change: it would stop a slow or hanging `Configure` for one organization from stalling reconciles for every other organization, which the current single global lock does not.

I am happy to open a PR with the fix and the regression test if that is welcome.

## Workaround

Restart the provider pod, or wait out the TTL:

```bash
kubectl delete -n crossplane-system \
$(kubectl get pods -n crossplane-system -o name | grep provider-upjet-github)
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in internal/clients/github.go, especially CachedTerraformSetup and getOrBuildTerraformSetup, then reproduce the stale-entry behavior with the internal/clients test described in the issue. Trace how ProviderConfig credentials and owner reach the cache, and verify that changing them causes a fresh setup while unchanged configurations still use the cache; the regression test should pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
github, go, kubernetes
Domain
authentication, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.