fix: invalidate cached AuthManager when ProviderConfig contents change

[dfry]

Problem

SharedConnector.Connect caches the AuthManager keyed by ProviderConfig UID alone. A ProviderConfig update keeps its UID, so any change to management-uri, credentials, credentials-type, or oauth-issuer-url keeps returning an AuthManager built for the old values until the provider pod is restarted.

Observed in the field: after a management server moved from plain HTTP in-cluster to a public TLS endpoint (ProviderConfig management-uri updated), every reconcile kept dialing the old URI — all managed resources for that config stuck in ReconcileError (error code 400, error unmarshalling body: invalid character ...) until a manual rollout restart of the provider.

A rotated token has the same failure mode: Connect re-extracts the credential Secret on every call, but discards it on cache hit — and refreshToken reuses the constructor-time credentials, so a rotated token is never picked up either.

Fix

Pair each cache entry with a fingerprint (sha256 over the length-prefixed endpoint, extracted credentials, credentials type, and issuer URL) and rebuild the AuthManager when the fingerprint differs. Unchanged configs still reuse the cached manager, so token-refresh behaviour and the one-manager-per-ProviderConfig model are preserved — the cache continues to prevent re-authenticating on every reconcile.

Tests

• TestConnectReusesManagerWhileConfigUnchanged — unchanged config → same cached manager.
• TestConnectRebuildsManagerOnConfigChange — same UID, updated management-uri → rebuilt manager with the new endpoint, which is then itself cached.

Full test suite passes (go test ./..., go vet ./... clean).

Summary by CodeRabbit

• Bug Fixes
  • Improved connection caching so authentication settings are revalidated when provider configuration details change, even if the identifier stays the same.
  • Prevented reuse of outdated connection state when key endpoint or credential-related values are updated.
• Tests
  • Added coverage for reusing cached connections when settings are unchanged.
  • Added coverage for rebuilding cached connections after configuration changes.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces content-based fingerprinting for cached AuthManager instances in SharedConnector.Connect. A new configFingerprint helper hashes ManagementURI, credentials, CredentialsType, and OauthIssuerUrl, stored alongside the manager in a cacheEntry, invalidating stale cache entries when ProviderConfig content changes despite a constant UID. Tests validate reuse and rebuild behavior.

Changes

Auth Cache Fingerprinting

Layer / File(s)	Summary
Fingerprint computation and cache validation internal/controller/nb/auth.go	Adds crypto/hash imports, a cacheEntry struct pairing manager with fingerprint, a configFingerprint helper hashing ProviderConfig inputs, and updates Connect to validate/store cache entries using the fingerprint.
Cache reuse and rebuild tests internal/controller/nb/auth_test.go	Adds test helpers and two tests verifying Connect reuses a cached manager when config is unchanged and rebuilds/re-caches when ManagementURI changes under the same UID.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Connect
  participant Cache
  participant AuthManager

  Caller->>Connect: Connect(NbGroup, ProviderConfig)
  Connect->>Connect: configFingerprint(endpoint, creds, credType, issuerURL)
  Connect->>Cache: load(pc.UID)
  alt entry found and fingerprint matches
    Cache-->>Connect: cached AuthManager
    Connect-->>Caller: return cached AuthManager
  else no entry or fingerprint mismatch
    Connect->>AuthManager: create new AuthManager
    Connect->>Cache: store cacheEntry{fingerprint, manager}
    Connect-->>Caller: return new AuthManager
  end

Loading

Poem

A rabbit hashes bytes with glee,
ManagementURI, creds, and key,
If nothing's changed, reuse the old,
If something shifts, a fresh one's told! 🐰🔐
Cache stays sharp as carrot stew!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: invalidating cached AuthManager instances when ProviderConfig contents change.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/controller/nb/auth_test.go (1)

80-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider testing credential and credentials-type changes too.

The PR objectives mention fingerprint invalidation for credentials, credentials-type, and oauth-issuer-url changes, but the tests only cover ManagementURI. Adding table-driven cases for the other fields would increase confidence that the fingerprint mechanism covers all inputs correctly.

🧪 Suggested additional test

func TestConnectRebuildsManagerOnCredentialsTypeChange(t *testing.T) {
	c := testConnector()
	mg := &vpnv1alpha1.NbGroup{}

	pc := testProviderConfig("uid-1", "https://mgmt.example.com")
	pc.Spec.CredentialsType = "token"
	first, err := c.Connect(context.Background(), mg, pc)
	if err != nil {
		t.Fatalf("first Connect: %v", err)
	}

	pc.Spec.CredentialsType = "oauth"
	second, err := c.Connect(context.Background(), mg, pc)
	if err != nil {
		t.Fatalf("second Connect: %v", err)
	}
	if first == second {
		t.Fatal("expected a rebuilt AuthManager after CredentialsType changed")
	}
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/nb/auth_test.go` around lines 80 - 113, The existing
Connect rebuild test only covers ManagementURI changes, but the fingerprint
should also invalidate on credentials, credentials-type, and oauth-issuer-url
changes. Extend TestConnectRebuildsManagerOnConfigChange in auth_test.go,
ideally with table-driven subtests, to mutate ProviderConfig.Spec.Credentials,
CredentialsType, and OAuthIssuerURL after an initial Connect and assert the
returned AuthManager is rebuilt and then cached again. Reuse the same Connect,
testConnector, and testProviderConfig helpers so the new cases verify all
fingerprint inputs.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/controller/nb/auth_test.go`:
- Around line 80-113: The existing Connect rebuild test only covers
ManagementURI changes, but the fingerprint should also invalidate on
credentials, credentials-type, and oauth-issuer-url changes. Extend
TestConnectRebuildsManagerOnConfigChange in auth_test.go, ideally with
table-driven subtests, to mutate ProviderConfig.Spec.Credentials,
CredentialsType, and OAuthIssuerURL after an initial Connect and assert the
returned AuthManager is rebuilt and then cached again. Reuse the same Connect,
testConnector, and testProviderConfig helpers so the new cases verify all
fingerprint inputs.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e10b66bd-e2c1-4586-bc7b-3575b8541404

📥 Commits

Reviewing files that changed from the base of the PR and between c45c47b and d86823a.

📒 Files selected for processing (2)

• internal/controller/nb/auth.go
• internal/controller/nb/auth_test.go