aksd: fix: return full cluster list for AKS cluster registration#296
aksd: fix: return full cluster list for AKS cluster registration#296tejhan wants to merge 1 commit intoAzure:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the "Register AKS Cluster" feature only returned the first 100 AKS clusters (alphabetically) when a subscription contained more than 100 clusters. The root cause was that the Azure Resource Graph query didn't specify the --first flag (which defaults to 100) and didn't handle pagination using --skip-token.
Changes:
- Created a new
fetchGraphPagehelper function that executes Azure Resource Graph queries with pagination support (using--first 1000and--skip-token) - Refactored
getClustersViaGraphto use the new helper function and loop through all pages to fetch the complete list of clusters - Added proper error handling for authentication and query failures in the pagination logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Nice fix.
Can you please address the copilot comments?
tejhan
force-pushed
the
fix/paginate-aks-clusters
branch
2 times, most recently
from
d436b13 to
fd06a4a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tejhan
force-pushed
the
fix/paginate-aks-clusters
branch
from
fd06a4a to
2eab1f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tejhan
force-pushed
the
fix/paginate-aks-clusters
branch
from
2eab1f9 to
f03a99d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks. Nice improvement.
I left a note and a couple of questions.
| * skip token if applicable to handle pagination to fetch all clusters in larger subscriptions. | ||
| */ | ||
| async function fetchGraphPage( | ||
| query: string, |
There was a problem hiding this comment.
Can you please add param docs?
| // Fetch remaining pages if the subscription has more clusters than one page holds | ||
| const MAX_PAGES = 100; // 100,000 cluster limit. | ||
| let pageCount = 1; | ||
| while (page.skipToken && pageCount < MAX_PAGES) { |
There was a problem hiding this comment.
Does this always go through 100 pages?
I don't understand how it knows to stop when there are no more pages to go through?
There was a problem hiding this comment.
It won't, since the graph query response object contains a skipToken only if more pages exist, otherwise it returns a null skipToken, (thus checking for existence of the skipToken here halts the loop on the final 'page' response object which won't have a skipToken present.
| throw error; | ||
| const query = ` | ||
| Resources | ||
| | where type =~ 'microsoft.containerservice/managedclusters' |
There was a problem hiding this comment.
How does this affect working with non-automatic clusters?
cc @gambtho
There was a problem hiding this comment.
The microsoft.containerservice/managedclusters resource type is used for both Standard & Automatic SKU's. Upon testing both types are returned by the graph query.
tejhan
force-pushed
the
fix/paginate-aks-clusters
branch
from
f03a99d to
4e4a09c
Compare
Description
When using the "Register AKS Cluster" feature, subscriptions with more than 100 AKS clusters would return only the first 100 clusters (alphabetically). Any cluster that alphabetically proceeded these isn't returned or searchable.
The root of issue is that
getClustersViaGraphcalled az graph query without specifying the --first flag, which defaults to 100 results & also didn't paginate excess results with theskip-tokens.Type of Change
Related Issues
Closes #295
Related to #274
Changes Made
Created a
fetchGraphPagehelper that calls az graph query with--first1000 (max) and returns the result data and the skip_token pagination cursor.Updated
getClustersViaGraphto paginate through all results using skip_token, so all clusters are successfully fetched.Testing
Since my largest subscriptions were in the hundreds but not thousands, pagination was validated by temporarily lowering the page size to 100 to confirm multi-page fetching works correctly. Verified all clusters appear when cluster amount exceeds page amount, which indicates pagination works.
Notes:
This is related to the umbrella issue of pagination for az-cli commands throughout. Currently looking into implementing this logic for other resource types so we can share utility.