Refactor managed cluster cache helper

[srstack]

Summary

• fold the RBAC cache path into listAllManagedClusters
• remove the redundant cached helper, cache toggle, cache-origin return value, and unreachable bypass metric
• add regression coverage confirming repeated managed-cluster lists use the RBAC cache

Test plan

• go test ./pkg/server -run "^(TestListAllManagedClustersUsesRBACCache|TestFirstManagedOrganizationUsesRBACCache)$" -count=1
• make lint
• git diff --check

Summary by CodeRabbit

• Bug Fixes

  • Improved admin tenant authorization and listing by using more reliable managed-cluster data.
  • Ensured tenant credential flows consistently resolve the correct managed organization.
  • Reduced unnecessary provider requests when retrieving managed-cluster information.

• Tests

  • Added coverage confirming managed-cluster results are reused correctly across repeated requests.
  • Verified cached data remains accurate during organization and tenant authorization flows.

[coderabbitai]

📝 Walkthrough

Walkthrough

Managed cluster listing now encapsulates RBAC cache handling and returns only cluster data and errors. Admin tenant authorization, listing, and pool organization resolution use the updated function, with tests verifying cache reuse.

Changes

Managed cluster cache consolidation

Layer / File(s)	Summary
Consolidate managed cluster listing and caching pkg/server/admin_tenants.go	listAllManagedClusters now handles cache lookups, metrics, termination, and cluster-list persistence without returning a cache-status flag.
Update admin tenant and pool callers pkg/server/admin_tenants.go, pkg/server/admin_tenant_pool.go, pkg/server/admin_tenant_pool_cache_test.go	Admin tenant and pool flows use the simplified listing contract, and tests verify repeated calls reuse the RBAC cache.

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

Possibly related PRs

• mem9-ai/drive9#723: Overlaps with managed-cluster listing and RBAC cache behavior.
• mem9-ai/drive9#764: Modifies the same admin tenant listing and authorization flow.
• mem9-ai/drive9#765: Touches the same pool organization path and cache reuse tests.

Suggested reviewers: copilot, mornyx

🚥 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 matches the main change: refactoring the managed cluster cache helper and folding cache logic into listAllManagedClusters.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/managed-cluster-cache-helper

---

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.}

[Copilot]

Pull request overview

Refactors the server’s managed-cluster listing helper to always route through the TiDB Cloud RBAC cache, removing redundant wrapper/bypass plumbing and simplifying call sites in the admin tenant and tenant-pool flows.

Changes:

• Inlined the RBAC cache lookup path into listAllManagedClusters and removed the redundant cached helper, cache toggle, and cache-origin return value.
• Updated admin tenant and tenant-pool call sites to use the simplified listAllManagedClusters signature.
• Added a regression test verifying repeated managed-cluster list calls reuse the RBAC cache (avoiding extra TiDB Cloud list calls).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
pkg/server/admin_tenants.go	Simplifies managed-cluster listing helper to directly include RBAC cache behavior; updates admin tenant endpoints to match new return signature.
pkg/server/admin_tenant_pool.go	Updates org resolution path to use the refactored managed-cluster listing helper.
pkg/server/admin_tenant_pool_cache_test.go	Adds regression coverage ensuring repeated managed-cluster lists hit the RBAC cache (single upstream list call).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/server/admin_tenants.go (1)

531-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use errors.New for static error messages.

Since there are no formatting arguments provided, prefer using errors.New over fmt.Errorf.

♻️ Proposed refactor

 	lister, ok := s.provisioner.(tenant.ManagedClusterLister)
 	if !ok {
-		return nil, fmt.Errorf("managed cluster list not enabled")
+		return nil, errors.New("managed cluster list not enabled")
 	}

🤖 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 `@pkg/server/admin_tenants.go` around lines 531 - 534, Update the error
construction in the managed cluster lister type-assertion branch of the server
handler to use errors.New for the static “managed cluster list not enabled”
message instead of fmt.Errorf, and add or reuse the required errors import.

🤖 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 `@pkg/server/admin_tenants.go`:
- Around line 531-534: Update the error construction in the managed cluster
lister type-assertion branch of the server handler to use errors.New for the
static “managed cluster list not enabled” message instead of fmt.Errorf, and add
or reuse the required errors import.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0c65ce99-6d03-466d-aaa1-a17cf3b570b7

📥 Commits

Reviewing files that changed from the base of the PR and between db562b7 and 50ac198.

📒 Files selected for processing (3)

• pkg/server/admin_tenant_pool.go
• pkg/server/admin_tenant_pool_cache_test.go
• pkg/server/admin_tenants.go

[mornyx]

Reviewed locally (checked out the branch, built, ran the targeted tests).

Verdict: approve — this is a behavior-preserving simplification. Verified the two claims behind it:

1. The bypass path was unreachable. On main, the only direct listAllManagedClustersCached caller passed allowCache=true (authorizedAdminTenant), and the listAllManagedClusters wrapper hard-coded true. So removing the allowCache parameter and the bypass metric label loses nothing — bypass could never be emitted at runtime, and no dashboard under docs/grafana references it.
2. The cache-origin bool return was discarded at every call site (clusters, _, err × 3). Dropping it from the signature is safe.

Also confirmed:

• All 4 call sites updated; no dangling references to listAllManagedClustersCached; go build ./pkg/server clean.
• TestListAllManagedClustersUsesRBACCache + TestFirstManagedOrganizationUsesRBACCache pass locally; CI green.
• Cache semantics unchanged: scoped lookup still only serves a hit when the cached cluster has a non-empty OrganizationID; unscoped miss still populates via rememberClusterList; the deleting-missing-cluster fallback in authorizedAdminTenant behaves exactly as before.

Two non-blocking notes:

• The branch history contains the accidental add managed-cluster-cache commit and its revert (net-zero). Harmless under squash merge — just flagging so nobody is confused by the commit list.
• Process FYI: that accidental commit and its revert were pushed directly to main (bypassing PR/CI). The revert did restore main's tree byte-for-byte to the post-#765 state (verified: git diff cd8e004 origin/main is empty), so no harm done — but it may be worth tightening branch protection on main to require PRs.

[srstack]

Reviewed (read-only diff). Clean behavior-preserving refactor and a natural follow-up to #764. LGTM.

This folds listAllManagedClustersCached(cred, clusterID, allowCache, metricPath) (clusters, fromCache, err) into listAllManagedClusters(cred, clusterID, metricPath) (clusters, err). Verified it's purely subtractive with no behavior change:

• allowCache is gone because it had no remaining false caller. Keep quota reads side-effect free and tune MySQL pool lifetimes #764 removed the last bypass path (the backfill fresh-read), so the only surviving usage was allowCache=true. Dropping the param + the unreachable "bypass" metric branch is correct.
• fromCache bool is gone because nothing reads it. After Keep quota reads side-effect free and tune MySQL pool lifetimes #764 dropped the clustersFromCache-driven backfill, every call site already discarded it (_,). All four sites (firstManagedOrganization, handleAdminTenantList, authorizedAdminTenant + its deleting-missing-cluster fallback) are updated to the two-value signature.
• Cache logic is line-for-line equivalent to the old allowCache=true branch: clusterID != "" → getCluster (hit only when OrganizationID != ""), else getClusterList, else record miss and fall through to the lister. I checked the edge case — a getCluster hit with empty OrganizationID still falls through to the list call in both old and new code (inner if OrganizationID != "" doesn't return), so no regression there.
• The remember-on-miss paths (rememberClusterList for full list, rememberTiDBCloudRBAC per-cluster on the single-page cluster-scoped path) are unchanged.

TestListAllManagedClustersUsesRBACCache pins the intended property: two calls, second served from cache, listCalls == 1. Combined with the existing TestFirstManagedOrganizationUsesRBACCache (which also checks invalidation via forget → 2 calls), the cache + invalidation behavior stays covered.

Already carries an approval; no concerns from me. LGTM.