crossplane-contrib / crossplane-contrib/provider-upjet-github
Provider config cache is keyed by name only, serving stale credentials after a ProviderConfig changes
- 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