fix: Parse aks cluster cert info from kubeconfig #282
Conversation
WalkthroughThe changes involve enhancing the Azure Kubernetes Service (AKS) resource scanning process in the event worker. The modifications focus on improving the asynchronous handling of cluster resource conversion by introducing a new method to retrieve certificate authority data and updating the resource conversion function to work with an Azure Container Service client. The implementation now supports more robust error handling and provides more comprehensive cluster resource information. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts (1)
21-25: Schema definition for kubeConfig may need enhancementThe current Zod schema only validates the presence of
"certificate-authority-data"andserverin the first cluster of theclustersarray. Consider enhancing the schema to validate all clusters or ensure the specific cluster relevant to the context is validated.You could modify the schema to validate multiple clusters or select the appropriate cluster based on additional identifiers.
apps/event-worker/src/resource-scan/azure/aks.ts (1)
50-54: Handle rejections in promise array when converting clustersUsing
Promise.allwill reject immediately if any promise rejects. SinceconvertManagedClusterToResourcehandles errors and returnsnull, unintended rejections are unlikely. However, for robustness, consider usingPromise.allSettledto handle any unexpected rejections without halting the entire process.Modify the code to use
Promise.allSettled:-const resources = await Promise.all(resourcePromises); -return resources.filter(isPresent); +const settledPromises = await Promise.allSettled(resourcePromises); +const resources = settledPromises + .filter(result => result.status === 'fulfilled' && isPresent(result.value)) + .map(result => result.value); +return resources;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/resource-scan/azure/aks.ts(1 hunks)apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/event-worker/src/resource-scan/azure/aks.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (4)
apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts (4)
1-4: Imports are appropriate and necessaryThe addition of
ContainerServiceClientandManagedClusterimports from@azure/arm-containerserviceis correct and required for the new Azure service interactions.
6-7: Imports for YAML parsing and schema validation are appropriateImporting
js-yamlfor YAML parsing andzodfor schema validation is appropriate for processing and validating the Kubernetes configuration.
27-58: Error handling and logging ingetCertificateAuthorityDataare well-implementedThe function correctly implements a
try-catchblock for error handling and logs detailed errors, including cluster name and ID. This aids in debugging and maintaining robustness.
77-77: Handle potential null values fromgetCertificateAuthorityDataEnsure that the rest of the code gracefully handles cases where
caisnull, to avoid runtime errors.Review the usage of
cain the subsequent code to confirm that all accesses are null-safe.
| const resourceGroup = cluster.id.split("/resourcegroups/")[1]?.split("/")[0]; | ||
| if (!resourceGroup) { | ||
| log.error("Invalid cluster", { cluster }); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parsing resourceGroup from cluster.id could be more robust
Using string splitting to extract the resourceGroup may be error-prone if the ID format changes. Consider using a more reliable method to parse the resource group.
You can use Azure's ResourceIdentifier class or a regular expression for parsing:
-import resourceGroup from cluster.id.split("/resourcegroups/")[1]?.split("/")[0];
+import { parseResourceId } from "@azure/arm-resourcegraph";
+const parsedId = parseResourceId(cluster.id);
+const resourceGroup = parsedId.resourceGroupName;
if (!resourceGroup) {
log.error("Invalid cluster ID format", { cluster });
return null;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
that function simply does not exist
There was a problem hiding this comment.
@adityachoudhari26 I apologize for my mistake. You are correct - I suggested using a non-existent function. The current string splitting approach is actually fine and commonly used in the codebase. Please disregard my previous suggestion.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Azure SDK packages in package.json
rg "@azure/" apps/event-worker/package.json
# Search for any resource ID parsing utilities in the codebase
rg -i "parseresource|resourceid" --type ts
Length of output: 41358
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#282
File: apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts:71-75
Timestamp: 2025-01-18T06:38:00.912Z
Learning: The Azure resource ID parsing in the codebase is done using string splitting (e.g., `id.split("/resourcegroups/")[1]?.split("/")[0]`) rather than any SDK utility functions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| auth: { method: "azure/aks", clusterName: cluster.name, resourceGroup }, | ||
| status: cluster.provisioningState ?? "UNKNOWN", | ||
| server: { | ||
| endpoint: cluster.fqdn ?? "", | ||
| certificateAuthorityData: cluster.servicePrincipalProfile?.clientId, | ||
| }, | ||
| server: { ...ca, endpoint: ca?.endpoint ?? cluster.fqdn ?? "" }, |
There was a problem hiding this comment.
💡 Codebase verification
Explicit null handling recommended for server configuration
The review suggestion is correct. The current spread pattern silently drops properties when ca is null, and differs from similar implementations in AWS EKS and Google GKE providers which use explicit property assignments. The suggested ternary operator approach is safer and more explicit.
🔗 Analysis chain
Ensure server configuration is correctly constructed
The server configuration merges ca data. If ca is null, the spread operator will not add any properties. Confirm that endpoint is always correctly assigned.
Consider explicitly handling the scenario where ca is null:
-server: { ...ca, endpoint: ca?.endpoint ?? cluster.fqdn ?? "" },
+server: ca
+ ? { ...ca }
+ : { endpoint: cluster.fqdn ?? "" },🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find type definitions and usage of ca variable
ast-grep --pattern 'interface $_ { endpoint: $_ }'
rg "ca\s*:\s*{.*endpoint" -A 5
rg "server:\s*{.*endpoint" -A 5
# Find the assignment of ca variable
ast-grep --pattern 'const ca = $_'
Length of output: 532
Script:
#!/bin/bash
# Find type definitions and implementations
rg "interface.*Server" -A 5
rg "type.*Server" -A 5
# Find getCertificateAuthorityData implementation
ast-grep --pattern 'async function getCertificateAuthorityData($_) {
$$$
}'
# Find similar server configurations with fixed regex
rg "server: \{" -A 3
rg "ca: \{" -A 3
Length of output: 4393
Script:
#!/bin/bash
# Find getCertificateAuthorityData implementation with a different pattern
rg "getCertificateAuthorityData" -A 10
rg "function getCertificateAuthorityData" -A 10
# Find any type definitions for the server object
rg "certificateAuthorityData.*endpoint" -A 3
Length of output: 2278
There was a problem hiding this comment.
certificateAuthorityData expects string | null | undefined which is the exact type which ca.certificateAuthorityData has, so this spread should be safe
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right. I've verified that certificateAuthorityData is defined as z.string().nullish() in the validators package, which means it accepts string | null | undefined. The current spread operation is type-safe and consistent with how certificate authority data is handled across other providers like AWS EKS and Google GKE.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find type definitions for ClusterResource and KubernetesClusterAPIV1
rg "type ClusterResource" -A 10
rg "interface KubernetesClusterAPIV1" -A 10
rg "certificateAuthorityData.*:" -A 1
# Find the return type and implementation of getCertificateAuthorityData
ast-grep --pattern 'const getCertificateAuthorityData = async ($_) => {
$$$
}'
Length of output: 2773
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#282
File: apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts:87-89
Timestamp: 2025-01-18T06:38:46.011Z
Learning: In the AKS provider, certificateAuthorityData is type-safe with string | null | undefined, allowing the spread operation of ca object in server configuration. This is consistent with AWS EKS and Google GKE implementations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
New Features
Improvements
Dependencies
js-yamlfor YAML parsing.@types/js-yamlfor TypeScript support.