Conversation
WalkthroughThis pull request introduces comprehensive Azure resource management capabilities across the CodeRabbit platform. The changes span multiple components, including event workers, web services, database schemas, and documentation. The implementation enables scanning, importing, and managing Azure Kubernetes Service (AKS) clusters, adding Azure as a first-class resource provider alongside existing Google and AWS integrations. The changes include new API routes, database migrations, frontend components, and backend resource scanning logic. Changes
Sequence DiagramsequenceDiagram
participant User
participant WebUI
participant AzureAPI
participant ResourceScanner
participant Database
User->>WebUI: Configure Azure Provider
WebUI->>AzureAPI: Request Consent URL
AzureAPI-->>WebUI: Return Consent URL
User->>AzureAPI: Authorize Application
AzureAPI->>Database: Store Tenant & Provider
Database-->>ResourceScanner: Trigger Resource Scan
ResourceScanner->>AzureAPI: Fetch AKS Clusters
ResourceScanner->>Database: Upsert Cluster Resources
Possibly related PRs
Suggested reviewers
Poem
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts (2)
16-29: Consider unifying promise usage
This file mixesasync/awaitwith.then()chaining, which can make the flow harder to trace. Adopting a uniformasync/awaitapproach or consistently using.then()would improve readability and error handling consistency.- const resourceProvider = await db - .insert(...) - .values({ ... }) - .returning() - .then(takeFirstOrNull); + const resourceProvider = (await db + .insert(...) + .values({ ... }) + .returning()) + .map(takeFirstOrNull)
83-105: Avoid returning final response from a .then() chain
Relying on multiple.then()calls for the final redirect can complicate debugging if an error arises in a subsequent step. Using a singleawaitblock ensures the function’s final response is more predictable.apps/webservice/src/app/api/azure/consent/route.ts (1)
30-42: Clear error paths
The early returns for missing or invalid state are concise. Consider returning an error object or message for easier client debugging instead of the minimal JSON.- if (!state) return NextResponse.json({ status: BAD_REQUEST }); + if (!state) { + return NextResponse.json( + { error: "Missing 'state' parameter" }, + { status: BAD_REQUEST }, + ); + }apps/event-worker/src/resource-scan/index.ts (2)
1-33: Use typed resource provider models
To improve clarity, consider defining a stricter type or interface for the resource provider object, instead of usinganywhen checkingrp.resource_provider_google,rp.resource_provider_aws, etc.
34-43: Guard checks for each cloud vendor
The sequential checks for Google, AWS, and Azure providers work fine, but an explicitswitch-like approach (or a well-defined type union) might be more declarative and safer if more cloud providers are added in the future.apps/event-worker/src/resource-scan/azure/index.ts (1)
1-7: Prefer consistent logging overconsole.log
At line 10, consider using the injected logger instead of rawconsole.logfor better logging consistency.- console.log("GETTING AKS RESOURCES"); + log.info("Getting AKS resources");apps/event-worker/src/resource-scan/azure/aks.ts (5)
15-17: Check environment variable clarity
AZURE_CLIENT_IDandAZURE_CLIENT_SECRETmight benefit from prefix consistency (e.g.,AZURE_APP_CLIENT_IDandAZURE_APP_CLIENT_SECRET) to avoid confusion or mismatched usage in other code segments.
22-25: Graceful handling of missing credentials
Skipping the scan if credentials are not provided is appropriate. Confirm if logging the error alone is sufficient or if you'd like to notify an external system or raise a dedicated error for auditing.
33-36: Tenant not found scenario
Similarly to the missing credentials case, confirm whether an error log is enough or if this scenario should generate a workflow notification or alert for misconfigurations.
44-47: Regional or Gov cloud usage
If you anticipate Azure Gov cloud or region-specific endpoints, confirm that the client constructor domain or credential scope can adapt. Currently, it defaults to commercial Azure.
49-54: Potential improvement in listing clusters
Consider loading clusters in parallel if you foresee large subscription usage, though the existing approach is simpler. If performance is a concern, explore parallel iteration patterns.apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/AzureDialog.tsx (2)
25-25: Naming clarity
TheAzureDialogPropstype is straightforward. Consider adding docstrings clarifying its fields if more props are introduced later.
46-49: User discoverability
The button labeled “Configure” might benefit from a short explanation or icon to indicate Azure specifically, ensuring that multi-cloud contexts remain obvious in the UI.packages/db/src/schema/resource-provider.ts (1)
130-139: Check for potential orphan records inresource_provider_azure.While
resourceProviderIdhasonDelete: "cascade", the reference toazureTenant.iddoesn't specify cascading behavior. Consider whether you'd like to cascade deletions here as well, or handle orphaned records manually via a cleanup process.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
apps/event-worker/package.json(1 hunks)apps/event-worker/src/index.ts(1 hunks)apps/event-worker/src/resource-scan/aws/index.ts(0 hunks)apps/event-worker/src/resource-scan/azure/aks.ts(1 hunks)apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts(1 hunks)apps/event-worker/src/resource-scan/azure/index.ts(1 hunks)apps/event-worker/src/resource-scan/google/index.ts(0 hunks)apps/event-worker/src/resource-scan/index.ts(1 hunks)apps/event-worker/src/resource-scan/utils.ts(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/ProviderActionsDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/AzureDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/page.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx(4 hunks)apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts(1 hunks)apps/webservice/src/app/api/azure/consent/route.ts(1 hunks)apps/webservice/src/env.ts(1 hunks)packages/api/src/router/resource-provider.ts(3 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/resource-provider.ts(1 hunks)packages/validators/src/auth/index.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/event-worker/src/resource-scan/aws/index.ts
- apps/event-worker/src/resource-scan/google/index.ts
- apps/event-worker/src/resource-scan/utils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/meta/_journal.json
🧰 Additional context used
📓 Path-based instructions (15)
packages/validators/src/auth/index.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/index.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/webservice/src/env.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/page.tsx (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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/AzureDialog.tsx (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.
packages/db/src/schema/resource-provider.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.
packages/api/src/router/resource-provider.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/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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/ProviderActionsDropdown.tsx (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/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.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.
apps/event-worker/src/resource-scan/index.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/index.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (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/webservice/src/app/api/azure/consent/route.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.
🔇 Additional comments (45)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (8)
1-11: Imports look consistent and purposeful.
You are properly separating and naming your icon imports, which helps keep the code organized and clear. Good job.
36-38: Metadata export and ResourceProvider type definition are well-defined.
Exporting themetadataobject aligns with Next.js patterns, and referencing theRouterOutputs["resource"]["provider"]["byWorkspaceId"]type helps maintain strong typing.
40-43: Check for potential null references inisCustomProvider.
Although you are verifyinggoogleConfig,awsConfig, andazureConfig, consider confirming thatprovideritself is valid in places outside this function.
45-63:CustomProviderTooltipBadgeoffers good clarity for custom providers.
Using a detailed tooltip is a nice UX improvement, ensuring users understand custom providers.
65-96:ManagedProviderBadgeis cleanly implemented.
Conditionally rendering provider badges for Google, AWS, and Azure keeps the UI straightforward. Nicely done.
143-149: Conditional rendering for custom vs. managed providers is well-handled.
This improves readability by offloading each UI variant to dedicated components.
162-179: Responsive handling ofprovider.kinds.
Using separate logic for empty and existing resource kinds ensures clarity for each scenario.
182-184: Date formatting is concise.
Displaying the date withtoLocaleDateString()is user-friendly and avoids time zone complexity.apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts (3)
1-15: Use consistent dependency imports and environment usage
The setup of environment variables and third-party dependencies looks good overall. Ensure that future expansions keep the import style consistent throughout the codebase (e.g., consistently using named vs. default imports).
31-45: Check workspace existence
Good job returning a clear 404 when the workspace does not exist. This prevents forwarding requests that can’t be fulfilled. Simply ensure the response shape is consistent across the entire codebase (some places return{ error: "..." }, others return{ status: 400 }, etc.).
46-66: Redirect logic is well-handled
The code properly handles tenant creation and consenting flow. The 403 Forbidden guard is also consistent with preventing cross-tenant access. No issues found here.apps/webservice/src/app/api/azure/consent/route.ts (2)
45-57: Graceful workspace checks
The workspace retrieval logic is correct. Nice use oftakeFirstOrNullto avoid array indexing. Ensure that the new logic is well-tested for typical and edge cases.
58-104: Transaction usage is beneficial
Wrapping these inserts in a transaction is a good approach to ensure no partial data if an operation fails mid-creation. Good job.apps/event-worker/src/resource-scan/index.ts (1)
44-105: Adequate concurrency and job lifecycle
A concurrency of 10 is reasonable for scanning tasks. Also, removing repeatable jobs when a resource provider is missing prevents infinite loops. Ensure you handle re-creation in the future if that resource provider arises.apps/event-worker/src/index.ts (1)
5-12: Single unified worker
Consolidating AWS, Google, and Azure scanning intoresourceScanWorkersimplifies initiation and shutdown. Good architectural move.apps/event-worker/src/resource-scan/azure/index.ts (1)
8-26: Extend resource scanning logic
The logic correctly handles absent Azure resource provider references. If advanced validations or additional Azure resources are added, ensuregetAksResourcesis flexible or made extensible.apps/webservice/src/env.ts (1)
32-33: Confirm necessity of optional vs. required environment variables
ProvidingAZURE_APP_CLIENT_IDas optional andREDIS_URLas required makes sense if your application can run without Azure integration but always requires Redis. Ensure that any dependent logic checks availability ofAZURE_APP_CLIENT_IDbefore use.apps/event-worker/src/resource-scan/azure/aks.ts (3)
27-31: Ensure synchronous concurrency
When querying the database for the tenant, concurrency or simultaneous scans might happen if multiple jobs run in parallel. Confirm that no concurrency issues or locks are needed in this flow.
38-42: Security check forClientSecretCredentialinstantiation
Check that you are not accidentally logging secrets. Ensure no unintentional debug logs expose sensitive info, especially if you expand the project’s logging in the future.
57-61: Resource mapping
The usage ofisPresentis clear for filtering. EnsureconvertManagedClusterToResourceis tested for all relevant cluster states, including partially-defined or error states.apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/AzureDialog.tsx (4)
33-33: Exporting the functional component
export const AzureDialogis a good approach. If usage grows, you may want to index multiple related components under anazurefolder for clarity.
37-41: Router flow
Usingrouter.pushpost-validation is fine. Just ensure that any API errors after the user is redirected are handled gracefully so users receive clear feedback in the new page.
54-55: Form usage
Wrapping<form>in<Form>from your custom UI framework is a neat approach, but confirm that all form state and error handling are consistent with your codebase guidelines.
97-103: Conditional disable
Disabling the “Save” button unless the form is valid and not submitting is a good user experience. Remember to handle special scenarios (e.g., offline, partial loads) that might prevent form validation.packages/validators/src/auth/index.ts (1)
54-54: Extending the permission set
AddingResourceProviderCreateis useful for resource management. Ensure that your role definitions have appropriate coverage for this permission to avoid unintended elevating of privileges.packages/db/src/schema/resource-provider.ts (2)
118-128: Consider whether tenant uniqueness should be scoped by workspace.Currently, there's a unique index only on
tenantId, without includingworkspaceId. If you plan to allow the same tenant ID in different workspaces, a composite unique index on(workspaceId, tenantId)might be more appropriate. Otherwise, the current schema will enforce a global uniqueness, preventing the same tenant ID from appearing in more than one workspace.
141-145: LGTM on Azure-related type exports.These type definitions appear consistent with the table schemas and naming conventions, providing a clear interface for consumers of the Azure data.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/ProviderActionsDropdown.tsx (3)
3-3: Import alignment looks good.Using
RouterOutputsfrom@ctrlplane/apiensures the provider type remains in sync with the backend query outputs.
31-31: Switched to the RouterOutputs type.Changing to
Provider = RouterOutputs["resource"]["provider"]["byWorkspaceId"][number]is a clean approach that stabilizes type definitions against API changes.
39-41: ExtendedisManagedProviderto include Azure.This succinctly maintains the existing pattern and ensures that Azure providers are recognized as managed resources. No issues found.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/page.tsx (5)
10-10: Introducing Azure icon import.Good use of a distinct Azure icon, improving the UI consistency for multi-cloud support.
16-16: Environment usage for Azure client ID.Fetching environment variables is a straightforward approach. Just ensure that
envis defined at build time to avoid runtime undefined references.
19-19: AzureDialog import looks consistent.This addition complements the new Azure integration flow, keeping structure parallel to AWS/Google.
77-77: Conditionally rendering the Azure card.Ensuring the card only appears when
AZURE_APP_CLIENT_IDis present prevents confusion if Azure integration isn’t configured. Good design.
124-142: New Azure resource provider card.The layout and content align with the existing cards for AWS/Google. It’s readable, consistent in styling, and meets the multi-provider approach.
apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts (6)
1-3: Imports for Azure containerservice and validator types.These references look correct, ensuring strong type support for the Azure SDK objects and your custom resource schema.
4-10: Logger usage and reserved metadata key import are appropriate.Logging provides clear diagnostics for cluster processing. No issues noted.
11-14: Augmented type ensures clarity.Extending
KubernetesClusterAPIV1withworkspaceIdandproviderIdmerges your app’s domain data with resource data effectively.
16-24: Error handling for missing cluster name or ID.The early return on invalid data is straightforward, preventing further issues. Good practice.
26-27: Tracking the app URL from the cluster.Storing
cluster.azurePortalFqdnin a local variable is clean and keeps the return object logic simpler below.
28-172: Comprehensive metadata population for Azure clusters.The aggregator pattern ensures your final resource object is complete. Omitting null or undefined keys keeps the data clean, and coverage of potential cluster fields appears thorough. This is an excellent approach for robust multi-cloud resource representation.
packages/api/src/router/resource-provider.ts (3)
20-20: Include Azure Provider Schema in Imports
Nicely integrated import ofresourceProviderAzure. This aligns well with the existing pattern for other providers (e.g., AWS, Google).
51-54: Confirm Left Join on Azure Provider
The left join withresourceProviderAzurealigns with the existing join workflow for other providers. Make sure that any data-specific constraints or optional fields inresourceProviderAzureare correctly handled downstream (especially if null or missing).
100-100: Add Null-Safe Checks forazureConfig
When mapping providers, consider gracefully handling scenarios whereprovider.resource_provider_azuremight be null or partially populated. A subtle runtime error or unexpected UI behavior might occur if the data is malformed.apps/event-worker/package.json (1)
18-19: Validate Added Azure Dependencies
The newly added@azure/arm-containerserviceand@azure/identitydependencies appear correct for interacting with AKS and Azure authentication. As a best practice, confirm these versions meet your current compatibility requirements and do not introduce security vulnerabilities.
| const resourceProvider = await db | ||
| .insert(SCHEMA.resourceProvider) | ||
| .values({ | ||
| workspaceId, | ||
| name, | ||
| }) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
|
|
||
| if (resourceProvider == null) | ||
| return NextResponse.json( | ||
| { error: "Failed to create resource provider" }, | ||
| { status: INTERNAL_SERVER_ERROR }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transactional inconsistency
Creating the resourceProvider in a separate statement (outside a transaction with the subsequent resourceProviderAzure insert) can lead to partial data creation if an error occurs later. Consider wrapping both inserts in a single transaction (e.g., db.transaction()) to ensure atomicity.
| import type { ResourceScanEvent } from "@ctrlplane/validators/events"; | ||
| import type { NextRequest } from "next/server"; | ||
| import { NextResponse } from "next/server"; | ||
| import { Queue } from "bullmq"; | ||
| import { BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_FOUND } from "http-status"; | ||
| import IORedis from "ioredis"; | ||
| import ms from "ms"; | ||
| import { z } from "zod"; | ||
|
|
||
| import { eq, takeFirstOrNull } from "@ctrlplane/db"; | ||
| import { db } from "@ctrlplane/db/client"; | ||
| import * as SCHEMA from "@ctrlplane/db/schema"; | ||
| import { Channel } from "@ctrlplane/validators/events"; | ||
|
|
||
| import { env } from "~/env"; | ||
|
|
||
| const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null }); | ||
|
|
||
| const resourceScanQueue = new Queue<ResourceScanEvent>(Channel.ResourceScan, { | ||
| connection: redis, | ||
| }); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
HTTP status codes are incorrectly set in some NextResponse.json calls
The code review reveals that there are indeed issues with HTTP status code usage in the route handler:
- Three instances where
NextResponse.json({ status: BAD_REQUEST })is called without the proper status code in the second argument:- Line 33:
if (!state) return NextResponse.json({ status: BAD_REQUEST }) - Line 36:
if (configJSON == null) return NextResponse.json({ status: BAD_REQUEST }) - Line 40:
if (!parsedConfig.success) return NextResponse.json({ status: BAD_REQUEST })
- Line 33:
These calls should be updated to include the status code in the second argument, similar to how other error responses are handled in the same file:
NextResponse.json({ status: BAD_REQUEST }, { status: BAD_REQUEST })Note that other error responses in the file correctly set the status code:
- Line 54:
{ status: NOT_FOUND } - Line 65 and 77:
{ status: INTERNAL_SERVER_ERROR }
🔗 Analysis chain
Validate HTTP status usage
Several places return NextResponse.json({ status: BAD_REQUEST }) without specifying the second argument for the status code. This might default to 200 OK in some environments. Verify that you truly return HTTP 400.
Let me verify the usage of HTTP status codes in NextResponse.json calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking for NextResponse.json calls that lack {status: ...} in second argument
rg --after-context 3 "NextResponse\.json\s*\(\s*\{ status: BAD_REQUEST \}"
Length of output: 1106
Script:
#!/bin/bash
# Let's check the actual implementation of the route handler to see all NextResponse usage
cat apps/webservice/src/app/api/azure/consent/route.ts
Length of output: 3244
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/db/src/schema/resource-provider.ts (1)
Line range hint
22-27: Update resourceProvider relations to include Azure providersThe
resourceProviderRelationsneeds to be updated to include the new Azure provider relationship.export const resourceProviderRelations = relations( resourceProvider, ({ many }) => ({ resources: many(resource), google: many(resourceProviderGoogle), + azure: many(resourceProviderAzure), }), );
🧹 Nitpick comments (8)
apps/event-worker/src/resource-scan/aws/eks.ts (2)
149-149: Log the skipped scan scenario
Early returning an empty array is correct whenimportEksis disabled. However, consider adding a debug log statement here so users can trace why EKS scanning was skipped.if (!config.importEks) { + log.debug("Skipping EKS scanning because importEks is disabled"); return []; }
165-178: Simplify promise chaining
Chaining with Lodash.chainplus multiple.then()calls can hinder clarity and debugging. Consider relying on native async/await or directly callingPromise.allfor better readability.- const resources = await _.chain(config.awsRoleArns) - .map((customerRoleArn) => scanEksClustersByAssumedRole(workspaceStsClient, customerRoleArn)) - .thru((promises) => Promise.all(promises)) - .value() - .then((results) => results.flat()) - .then((resources) => - resources.map((resource) => ({ - ...resource, - workspaceId: workspace.id, - providerId: config.resourceProviderId, - })), - ); + const scanResults = await Promise.all( + config.awsRoleArns.map((customerRoleArn) => + scanEksClustersByAssumedRole(workspaceStsClient, customerRoleArn), + ), + ); + const resources = scanResults + .flat() + .map((resource) => ({ + ...resource, + workspaceId: workspace.id, + providerId: config.resourceProviderId, + }));apps/event-worker/src/resource-scan/aws/vpc.ts (2)
172-173: Log the skipped scan scenario
Similar to the EKS logic, consider adding a log entry to confirm the function was intentionally skipped whenimportVpcis false.if (!config.importVpc) { + log.debug("Skipping VPC scanning because importVpc is disabled"); return []; }
189-202: Improve readability and error handling
Using Lodash.chainplus.then()can be replaced with clearer async/await logic, benefiting maintainability and debugging.- const resources = await _.chain(config.awsRoleArns) - .map((customerRoleArn) => scanVpcsByAssumedRole(workspaceStsClient, customerRoleArn)) - .thru((promises) => Promise.all(promises)) - .value() - .then((results) => results.flat()) - .then((resources) => - resources.map((resource) => ({ - ...resource, - workspaceId: workspace.id, - providerId: config.resourceProviderId, - })), - ); + const scanResults = await Promise.all( + config.awsRoleArns.map((customerRoleArn) => + scanVpcsByAssumedRole(workspaceStsClient, customerRoleArn), + ), + ); + const flattened = scanResults.flat(); + const resources = flattened.map((resource) => ({ + ...resource, + workspaceId: workspace.id, + providerId: config.resourceProviderId, + }));packages/db/drizzle/0051_little_swordsman.sql (3)
7-12: Confirm table naming consistency
Check thatresource_provider_azurealigns with existing naming patterns and that it remains consistent if you rename other tables for standardization.
14-18: Validate foreign key constraints
Ensure the foreign key onazure_tenant.workspace_idbehaves as intended. If a workspace row is removed, do we want to automatically remove associated tenants?
20-24: Use consistent foreign key actions
Here,ON DELETE CASCADEis used forresource_provider_azure→resource_provider. Evaluate whether to keep all foreign key constraints in sync (e.g., use CASCADE or NO ACTION) for uniform handling of deletions.apps/event-worker/src/resource-scan/index.ts (1)
18-23: Centralized resource import
Centralizing all providers (AWS, Azure, Google) in a single file is convenient, but ensure that with multiple new cloud integrations, these import paths remain maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/resource-scan/aws/eks.ts(2 hunks)apps/event-worker/src/resource-scan/aws/vpc.ts(2 hunks)apps/event-worker/src/resource-scan/index.ts(1 hunks)packages/db/drizzle/0051_little_swordsman.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/resource-provider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/drizzle/meta/_journal.json
🧰 Additional context used
📓 Path-based instructions (4)
apps/event-worker/src/resource-scan/aws/vpc.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/aws/eks.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/index.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.
packages/db/src/schema/resource-provider.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.
🔇 Additional comments (7)
packages/db/drizzle/0051_little_swordsman.sql (3)
1-5: Consider on-delete behavior
Creating anazure_tenanttable withON DELETE NO ACTIONonworkspace_idcan leave orphaned rows if the corresponding workspace is deleted. Confirm this is desired or switch to a cascading delete.
26-30: Check referential integrity
The foreign key ontenant_idreferences the newazure_tenanttable withNO ACTION; ensure that partial deletions of tenants won’t break any references inresource_provider_azure.
32-32: Index usage
Creating a unique index onazure_tenant.tenant_idis appropriate. Confirm no collisions or need for composite indexes if multi-tenant scenarios become more complex.apps/event-worker/src/resource-scan/index.ts (2)
1-4: Check concurrency vs. performance constraints
You’re importing BullMQ and setting concurrency to 10 below. Ensure adhering to any resource usage limits for each environment if scanning is CPU/network-intensive.
58-119: Cohesive worker design
ThecreateResourceScanWorkerincludes robust logging, removing repeatable jobs, and upserting resources. This is well-structured. Just ensure any future expansions do not bloat the function.packages/db/src/schema/resource-provider.ts (2)
144-148: LGTM!The type exports are correctly defined and maintain consistency with other provider implementations.
121-148: Verify consistency with existing Azure implementationsLet's verify if there are any existing Azure-related implementations in the codebase to ensure consistency.
✅ Verification successful
Let me try another search strategy to gather more information about Azure-related implementations.
Azure implementation is consistent with existing patterns
The schema implementation for Azure resources is consistent with the existing codebase patterns. The verification shows:
- The
azure_tenantandresource_provider_azuretables are properly referenced throughout the codebase, particularly in:
- API routes (
apps/webservice/src/app/api/azure/)- Resource scanning logic (
apps/event-worker/src/resource-scan/azure/)- Frontend components (
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/)- The schema structure aligns with the application's Azure integration flow, which handles tenant management, subscription IDs, and resource provider configurations
- The implementation follows the same pattern used for other cloud providers in the system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing Azure-related implementations echo "Searching for Azure-related files..." fd -e ts -e tsx azure echo "Searching for Azure-related code patterns..." rg -i "azure|aks" --type ts --type tsx echo "Searching for similar table definitions..." ast-grep --pattern 'pgTable($_, { $$$ subscriptionId: $_ $$$ })'Length of output: 554
Script:
#!/bin/bash # Search for Azure-related code with ripgrep, fixing the file type syntax echo "Searching for Azure-related code patterns..." rg -i "azure|aks" -t ts # Search for Azure-related table definitions and schemas echo "Searching for Azure table definitions..." rg "pgTable\(.*azure" -t ts # Search for subscription-related code echo "Searching for subscription-related patterns..." rg "subscription" -t tsLength of output: 19592
| const getResource = async (rp: any) => { | ||
| if (rp.resource_provider_google != null) { | ||
| const [gkeResources, vpcResources] = await Promise.all([ | ||
| getGkeResources(rp.workspace, rp.resource_provider_google), | ||
| getGoogleVpcResources(rp.workspace, rp.resource_provider_google), | ||
| ]); | ||
| return [...gkeResources, ...vpcResources]; | ||
| } | ||
|
|
||
| if (rp.resource_provider_aws != null) { | ||
| const [eksResources, vpcResources] = await Promise.all([ | ||
| getEksResources(rp.workspace, rp.resource_provider_aws), | ||
| getAwsVpcResources(rp.workspace, rp.resource_provider_aws), | ||
| ]); | ||
| return [...eksResources, ...vpcResources]; | ||
| } | ||
|
|
||
| if (rp.resource_provider_azure != null) | ||
| return getAksResources(rp.workspace, rp.resource_provider_azure); | ||
| throw new Error("Invalid resource provider"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle partial scanning results
If an error occurs in one provider’s resource retrieval, getResource throws globally. Consider partial results or structured error handling to avoid losing data from healthy providers.
| export const azureTenant = pgTable( | ||
| "azure_tenant", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| workspaceId: uuid("workspace_id") | ||
| .notNull() | ||
| .references(() => workspace.id), | ||
| tenantId: text("tenant_id").notNull(), | ||
| }, | ||
| (t) => ({ uniq: uniqueIndex().on(t.tenantId) }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing schema components for Azure tenant
The azureTenant table definition needs additional components to maintain consistency with other provider implementations:
- Add relations definition
- Add create/update schema with validation
- Add validation for Azure tenant ID format
- Consider adding cascade delete for workspace reference
Here's the suggested implementation:
// Add relations
export const azureTenantRelations = relations(azureTenant, ({ many }) => ({
resourceProviders: many(resourceProviderAzure),
}));
// Add create schema with validation
export const createAzureTenant = createInsertSchema(azureTenant, {
tenantId: z
.string()
.regex(
/^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$/,
"Invalid Azure Tenant ID format. Expected format: GUID",
),
}).omit({ id: true });
export const updateAzureTenant = createAzureTenant.partial();Also, consider modifying the workspace reference to include cascade delete:
- .references(() => workspace.id),
+ .references(() => workspace.id, { onDelete: "cascade" }),| export const resourceProviderAzure = pgTable("resource_provider_azure", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| resourceProviderId: uuid("resource_provider_id") | ||
| .notNull() | ||
| .references(() => resourceProvider.id, { onDelete: "cascade" }), | ||
| tenantId: uuid("tenant_id") | ||
| .notNull() | ||
| .references(() => azureTenant.id), | ||
| subscriptionId: text("subscription_id").notNull(), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing schema components and import flags for Azure provider
The resourceProviderAzure table definition needs additional components to maintain consistency with other provider implementations:
- Add relations definition
- Add create/update schema with validation
- Add validation for Azure subscription ID format
- Consider adding cascade delete for tenant reference
- Add import flags similar to AWS/Google providers (e.g., for AKS, Azure Container Apps)
Here's the suggested implementation:
// Modify the table to include import flags
export const resourceProviderAzure = pgTable("resource_provider_azure", {
id: uuid("id").primaryKey().defaultRandom(),
resourceProviderId: uuid("resource_provider_id")
.notNull()
.references(() => resourceProvider.id, { onDelete: "cascade" }),
tenantId: uuid("tenant_id")
.notNull()
.references(() => azureTenant.id, { onDelete: "cascade" }),
subscriptionId: text("subscription_id").notNull(),
importAks: boolean("import_aks").notNull().default(false),
importContainerApps: boolean("import_container_apps").notNull().default(false),
});
// Add relations
export const resourceProviderAzureRelations = relations(
resourceProviderAzure,
({ one }) => ({
provider: one(resourceProvider, {
fields: [resourceProviderAzure.resourceProviderId],
references: [resourceProvider.id],
}),
tenant: one(azureTenant, {
fields: [resourceProviderAzure.tenantId],
references: [azureTenant.id],
}),
}),
);
// Add create schema with validation
export const createResourceProviderAzure = createInsertSchema(
resourceProviderAzure,
{
subscriptionId: z
.string()
.regex(
/^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$/,
"Invalid Azure Subscription ID format. Expected format: GUID",
),
},
).omit({ id: true });
export const updateResourceProviderAzure = createResourceProviderAzure.partial();There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (2)
40-43: Consider a more maintainable approach for provider type checking.The current implementation requires updating the function whenever a new provider type is added. Consider refactoring to make it more maintainable.
-const isCustomProvider = (provider: ResourceProvider) => - provider.googleConfig == null && - provider.awsConfig == null && - provider.azureConfig == null; +const isCustomProvider = (provider: ResourceProvider) => { + const managedConfigs = ['googleConfig', 'awsConfig', 'azureConfig']; + return managedConfigs.every(config => provider[config as keyof ResourceProvider] == null); +};
65-96: Consider reducing duplication in provider badge rendering.The component logic is correct, but there's repetition in the provider checks. Consider refactoring to reduce duplication.
-const ManagedProviderBadge: React.FC<{ - provider: ResourceProvider; -}> = ({ provider }) => ( +const ManagedProviderBadge: React.FC<{ + provider: ResourceProvider; +}> = ({ provider }) => { + const providerConfig = { + googleConfig: { Icon: SiGooglecloud, label: 'Google', className: 'bg-red-400/20 text-red-400' }, + awsConfig: { Icon: SiAmazon, label: 'AWS', className: 'bg-orange-400/20 text-orange-400' }, + azureConfig: { Icon: IconBrandAzure, label: 'Azure', className: 'bg-blue-400/20 text-blue-400' } + }; + + const activeConfig = Object.entries(providerConfig).find( + ([key]) => provider[key as keyof ResourceProvider] != null + ); + + if (!activeConfig) return null; + const [_, { Icon, label, className }] = activeConfig; + + return ( <Badge variant="secondary" - className={cn( - "flex h-6 w-fit items-center gap-1 rounded-full px-2 py-0 text-xs", - provider.googleConfig != null && "bg-red-400/20 text-red-400", - provider.awsConfig != null && "bg-orange-400/20 text-orange-400", - provider.azureConfig != null && "bg-blue-400/20 text-blue-400", - )} + className={cn("flex h-6 w-fit items-center gap-1 rounded-full px-2 py-0 text-xs", className)} > - {provider.googleConfig != null && ( - <> - <SiGooglecloud className="h-3 w-3" /> - Google - </> - )} - {provider.awsConfig != null && ( - <> - <SiAmazon className="h-3 w-3" /> - AWS - </> - )} - {provider.azureConfig != null && ( - <> - <IconBrandAzure className="h-3 w-3" /> - Azure - </> - )} + <Icon className="h-3 w-3" /> + {label} - </Badge> -); + </Badge> +)};apps/webservice/src/app/api/azure/consent/route.ts (1)
48-107: Improve transaction handling and configurationSeveral improvements could enhance the robustness of this code:
- The transaction rollback isn't explicitly handled
- The subscription ID format isn't validated
- The scan interval is hard-coded
Consider these improvements:
+const SCAN_INTERVAL_MS = ms('10m'); + +// Add subscription ID validation to schema const configSchema = z.object({ workspaceId: z.string(), tenantId: z.string(), - subscriptionId: z.string(), + subscriptionId: z.string().regex(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, + 'Invalid subscription ID format'), name: z.string(), }); return db.transaction(async (db) => { try { // ... existing code ... return db .insert(SCHEMA.resourceProviderAzure) .values({ resourceProviderId, tenantId: tenant.id, subscriptionId }) .then(() => resourceScanQueue.add( resourceProviderId, { resourceProviderId }, - { repeat: { every: ms("10m"), immediately: true } }, + { repeat: { every: SCAN_INTERVAL_MS, immediately: true } }, ), ) } catch (error) { + await db.rollback(); throw error; } });apps/event-worker/src/resource-scan/index.ts (1)
58-119: Optimize worker configuration and error handlingThe worker implementation could be improved in several areas:
- Cleanup settings are hard-coded
- Error handling could be more detailed
- Database query could be optimized
+const WORKER_CONFIG = { + removeOnComplete: { age: 1 * 60 * 60, count: 5000 }, + removeOnFail: { age: 12 * 60 * 60, count: 5000 }, + concurrency: 10, +}; + export const createResourceScanWorker = () => new Worker<ResourceScanEvent>( Channel.ResourceScan, async (job) => { const { resourceProviderId } = job.data; + let rp; - const rp = await db - .select() - .from(resourceProvider) - // ... existing joins ... - .then(takeFirstOrNull); + try { + rp = await db + .select() + .from(resourceProvider) + .where(eq(resourceProvider.id, resourceProviderId)) + .innerJoin(workspace, eq(resourceProvider.workspaceId, workspace.id)) + .$dynamic() // Use dynamic joins based on provider type + .then(takeFirstOrNull); + } catch (error) { + log.error(`Database query failed for provider ${resourceProviderId}:`, error); + throw error; + } // ... rest of the implementation ... }, - { - connection: redis, - removeOnComplete: { age: 1 * 60 * 60, count: 5000 }, - removeOnFail: { age: 12 * 60 * 60, count: 5000 }, - concurrency: 10, - }, + { connection: redis, ...WORKER_CONFIG }, );Also, consider implementing a circuit breaker pattern for resource scanning to prevent overwhelming cloud provider APIs:
import CircuitBreaker from 'opossum'; const breaker = new CircuitBreaker(getResources, { timeout: 30000, // 30 seconds errorThresholdPercentage: 50, resetTimeout: 30000 }); breaker.fallback(() => []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts(1 hunks)apps/event-worker/src/resource-scan/index.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx(4 hunks)apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts(1 hunks)apps/webservice/src/app/api/azure/consent/route.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts
- apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/event-worker/src/resource-scan/index.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/webservice/src/app/api/azure/consent/route.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (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.
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (3)
1-11: LGTM! Clean imports and type definitions.The imports are well-organized, and the
ResourceProvidertype is correctly derived from the API output.Also applies to: 37-38
45-63: LGTM! Well-structured tooltip component.The
CustomProviderTooltipBadgecomponent is well-implemented with clear tooltip content and consistent styling.
143-149: LGTM! Well-structured UI improvements.The changes improve the UI organization with:
- Clear provider type indication
- Proper empty state handling for provider kinds
- Simplified date formatting
Also applies to: 162-179, 182-184
| const configJSON = await redis.get(`azure_consent_state:${state}`); | ||
| if (configJSON == null) | ||
| return NextResponse.json({ error: "Bad request" }, { status: BAD_REQUEST }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add state cleanup and TTL for Redis keys
The Redis state is neither cleaned up after successful processing nor has a TTL set. This could lead to memory leaks.
-const configJSON = await redis.get(`azure_consent_state:${state}`);
+const stateKey = `azure_consent_state:${state}`;
+const configJSON = await redis.get(stateKey);
+
+// Clean up state after successful processing
+await redis.del(stateKey);Additionally, when setting the state (in the consent initiation endpoint), add a TTL:
// Add this where the state is initially set
await redis.set(stateKey, JSON.stringify(config), 'EX', 3600); // 1 hour expiry|
|
||
| import { env } from "~/env"; | ||
|
|
||
| const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null }); |
There was a problem hiding this comment.
Add retry limits and error handling for Redis connection
The Redis connection is configured without retry limits (maxRetriesPerRequest: null), which could lead to infinite retries. Consider adding:
- A reasonable retry limit
- Error handling for connection failures
-const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null });
+const redis = new IORedis(env.REDIS_URL, {
+ maxRetriesPerRequest: 3,
+ retryStrategy: (times) => {
+ if (times > 3) return null;
+ return Math.min(times * 100, 3000);
+ }
+});
+
+redis.on('error', (error) => {
+ logger.error('Redis connection error:', error);
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null }); | |
| const redis = new IORedis(env.REDIS_URL, { | |
| maxRetriesPerRequest: 3, | |
| retryStrategy: (times) => { | |
| if (times > 3) return null; | |
| return Math.min(times * 100, 3000); | |
| } | |
| }); | |
| redis.on('error', (error) => { | |
| logger.error('Redis connection error:', error); | |
| }); |
| const getResources = async (rp: any) => { | ||
| if (rp.resource_provider_google != null) { | ||
| const [gkeResources, vpcResources] = await Promise.all([ | ||
| getGkeResources(rp.workspace, rp.resource_provider_google), | ||
| getGoogleVpcResources(rp.workspace, rp.resource_provider_google), | ||
| ]); | ||
| return [...gkeResources, ...vpcResources]; | ||
| } | ||
|
|
||
| if (rp.resource_provider_aws != null) { | ||
| const [eksResources, vpcResources] = await Promise.all([ | ||
| getEksResources(rp.workspace, rp.resource_provider_aws), | ||
| getAwsVpcResources(rp.workspace, rp.resource_provider_aws), | ||
| ]); | ||
| return [...eksResources, ...vpcResources]; | ||
| } | ||
|
|
||
| if (rp.resource_provider_azure != null) | ||
| return getAksResources(rp.workspace, rp.resource_provider_azure); | ||
| throw new Error("Invalid resource provider"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in resource retrieval
The current implementation has several type-safety issues:
- Uses
anytype for the provider parameter - Lacks proper type narrowing
- Could benefit from more granular error handling
+type ResourceProvider = {
+ workspace: any;
+ resource_provider_google?: {
+ // Add specific type properties
+ };
+ resource_provider_aws?: {
+ // Add specific type properties
+ };
+ resource_provider_azure?: {
+ // Add specific type properties
+ };
+};
+
-const getResources = async (rp: any) => {
+const getResources = async (rp: ResourceProvider) => {
try {
if (rp.resource_provider_google != null) {
const [gkeResources, vpcResources] = await Promise.all([
getGkeResources(rp.workspace, rp.resource_provider_google),
getGoogleVpcResources(rp.workspace, rp.resource_provider_google),
- ]);
+ ]).catch(error => {
+ log.error('Error fetching Google resources:', error);
+ return [[], []];
+ });
return [...gkeResources, ...vpcResources];
}
// Similar changes for AWS and Azure
- throw new Error("Invalid resource provider");
+ throw new Error("No valid resource provider configuration found");
+ } catch (error) {
+ log.error('Error in getResources:', error);
+ throw error;
+ }
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
apps/webservice/src/app/api/azure/consent/route.ts (2)
17-17:⚠️ Potential issueAdd retry limits and error handling for Redis connection.
The Redis connection is configured without retry limits (
maxRetriesPerRequest: null), which could lead to infinite retries.Apply this diff to add retry limits and error handling:
-const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null }); +const redis = new IORedis(env.REDIS_URL, { + maxRetriesPerRequest: 3, + retryStrategy: (times) => { + if (times > 3) return null; + return Math.min(times * 100, 3000); + } +}); + +redis.on('error', (error) => { + logger.error('Redis connection error:', error); +});
94-94:⚠️ Potential issueAdd state cleanup and TTL for Redis keys.
The Redis state is neither cleaned up after successful processing nor has a TTL set.
Apply this diff to add state cleanup and TTL:
-await redis.set(`azure_consent_state:${state}`, configJSON, "EX", 900); +const stateKey = `azure_consent_state:${state}`; +await redis.set(stateKey, configJSON, "EX", 900); + +// Clean up state after successful processing +await redis.del(stateKey);
🧹 Nitpick comments (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx (2)
51-64: Add form validation feedback and loading state.The form lacks visual feedback during submission and clear validation messages.
Consider adding:
- Loading spinner on the submit button
- Explicit validation messages for each field
- Form-level error message display
- <form onSubmit={onSubmit} className="space-y-4"> + <form onSubmit={onSubmit} className="space-y-4"> + {form.formState.errors._form && ( + <div className="text-red-500 text-sm"> + {form.formState.errors._form.message} + </div> + )}
93-98: Add loading state to submit button.The submit button should indicate when the form is being submitted.
<Button type="submit" disabled={!form.formState.isValid || form.formState.isSubmitting} + className="w-full" > - Save + {form.formState.isSubmitting ? ( + <span className="flex items-center gap-2"> + <LoadingSpinner className="h-4 w-4" /> + Saving... + </span> + ) : ( + "Save" + )} </Button>apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/UpdateAzureProviderDialog.tsx (1)
53-60: Improve dialog accessibility.The dialog should handle keyboard events and focus management properly.
<Dialog open={open} onOpenChange={(o) => { setOpen(o); if (!o) onClose?.(); }} + onEscapeKeyDown={() => setOpen(false)} + onInteractOutside={(e) => e.preventDefault()} >apps/webservice/src/app/api/azure/consent/route.ts (1)
128-131: Enhance error messages with more details.The error messages are generic and don't provide enough context for debugging.
Apply this diff to enhance error messages:
return NextResponse.json( - { error: "Failed to update resource provider" }, + { + error: "Failed to update resource provider", + details: error instanceof Error ? error.message : 'Unknown error', + providerId: resourceProviderId + }, { status: INTERNAL_SERVER_ERROR }, ); return NextResponse.json( - { error: "Failed to create resource provider" }, + { + error: "Failed to create resource provider", + details: error instanceof Error ? error.message : 'Unknown error', + workspaceId, + tenantId: tenant.id + }, { status: INTERNAL_SERVER_ERROR }, );Also applies to: 144-147
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (3)
40-43: Improve maintainability of provider type checking.The
isCustomProviderfunction requires modification each time a new provider type is added.Consider using an array of config keys for more maintainable code:
-const isCustomProvider = (provider: ResourceProvider) => - provider.googleConfig == null && - provider.awsConfig == null && - provider.azureConfig == null; +const PROVIDER_CONFIG_KEYS = ['googleConfig', 'awsConfig', 'azureConfig'] as const; + +const isCustomProvider = (provider: ResourceProvider) => + PROVIDER_CONFIG_KEYS.every((key) => provider[key] == null);
49-54: Extract common badge styles to improve reusability.Both
CustomProviderTooltipBadgeandManagedProviderBadgehave similar badge styling patterns.Consider extracting common styles:
+const COMMON_BADGE_STYLES = "h-6 gap-1.5 rounded-full"; + +const PROVIDER_STYLES = { + custom: "bg-blue-500/10 text-blue-300", + google: "bg-red-400/20 text-red-400", + aws: "bg-orange-400/20 text-orange-400", + azure: "bg-blue-400/20 text-blue-400" +} as const; + <Badge variant="outline" - className="h-6 gap-1.5 rounded-full border-none bg-blue-500/10 pl-2 pr-3 text-xs text-blue-300" + className={cn(COMMON_BADGE_STYLES, "border-none pl-2 pr-3 text-xs", PROVIDER_STYLES.custom)} > <Badge variant="secondary" className={cn( - "flex h-6 w-fit items-center gap-1 rounded-full px-2 py-0 text-xs", + COMMON_BADGE_STYLES, "flex w-fit items-center px-2 py-0 text-xs", provider.googleConfig != null && PROVIDER_STYLES.google, provider.awsConfig != null && PROVIDER_STYLES.aws, provider.azureConfig != null && PROVIDER_STYLES.azure )} >Also applies to: 68-75
Line range hint
152-158: Improve accessibility of resource count display.The resource count display lacks proper ARIA labels and could be confusing for screen readers.
Apply this diff to improve accessibility:
<Badge variant="outline" + role="status" + aria-label={`${provider.resourceCount} ${ + provider.resourceCount === 1 ? "resource" : "resources" + }`} className="flex h-6 items-center gap-1.5 rounded-full border-none bg-neutral-800/50 px-2 text-xs text-muted-foreground" > <IconExternalLink className="h-4 w-4" /> - {provider.resourceCount}{" "} - {provider.resourceCount === 1 ? "resource" : "resources"} + <span aria-hidden="true"> + {provider.resourceCount}{" "} + {provider.resourceCount === 1 ? "resource" : "resources"} + </span> </Badge>packages/api/src/router/resource-provider.ts (3)
41-53: Consider handling missing Azure tenant data gracefully.The inner join with
azureTenantmight exclude Azure providers if tenant data is missing. Consider using a left join if you want to preserve Azure providers even when tenant data is unavailable.- .innerJoin( + .leftJoin( azureTenant, eq(resourceProviderAzure.tenantId, azureTenant.id), )
355-375: Add missing CRUD operations for Azure provider.The Azure router section only implements the
byProviderIdquery. Consider addingcreateandupdateoperations similar to AWS and Google Cloud providers to maintain feature parity.Would you like me to help generate the missing CRUD operations for the Azure provider?
364-374: Enhance error handling in the Azure provider query.The query could benefit from better error handling for cases where the provider doesn't exist or when database operations fail.
.query(({ ctx, input }) => ctx.db .select() .from(resourceProviderAzure) .innerJoin( azureTenant, eq(resourceProviderAzure.tenantId, azureTenant.id), ) .where(eq(resourceProviderAzure.resourceProviderId, input)) - .then(takeFirstOrNull), + .then(takeFirstOrNull) + .catch((error) => { + // Log the error for debugging + console.error('Failed to fetch Azure provider:', error); + throw new Error('Failed to fetch Azure provider configuration'); + }), ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/ProviderActionsDropdown.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/UpdateAzureProviderDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/page.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx(5 hunks)apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts(1 hunks)apps/webservice/src/app/api/azure/consent/route.ts(1 hunks)apps/webservice/src/env.ts(1 hunks)packages/api/src/router/resource-provider.ts(5 hunks)packages/db/src/schema/resource-provider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/page.tsx
🧰 Additional context used
📓 Path-based instructions (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx (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.
packages/api/src/router/resource-provider.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/webservice/src/env.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx (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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/UpdateAzureProviderDialog.tsx (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/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/ProviderActionsDropdown.tsx (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/webservice/src/app/api/azure/consent/route.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (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.
packages/db/src/schema/resource-provider.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 (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build
🔇 Additional comments (13)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx (1)
33-37: Consider adding error handling for navigation failures.The form submission directly pushes to a new route without handling potential navigation failures.
Add error handling:
const onSubmit = form.handleSubmit((data) => - router.push( - `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}`, - ), + router + .push( + `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}`, + ) + .catch((error) => { + console.error('Navigation failed:', error); + // TODO: Show error message to user + }), );apps/webservice/src/env.ts (1)
32-35: Review security implications of Azure credentials.The Azure credentials are marked as optional, which might lead to runtime issues if the Azure integration is enabled but credentials are missing.
Consider adding conditional validation:
- AZURE_APP_CLIENT_ID: z.string().optional(), - AZURE_APP_CLIENT_SECRET: z.string().optional(), - AZURE_TENANT_ID: z.string().optional(), + AZURE_APP_CLIENT_ID: z.string().optional().refine( + (val) => !process.env.AZURE_ENABLED || val != null, + "AZURE_APP_CLIENT_ID is required when Azure is enabled" + ), + AZURE_APP_CLIENT_SECRET: z.string().optional().refine( + (val) => !process.env.AZURE_ENABLED || val != null, + "AZURE_APP_CLIENT_SECRET is required when Azure is enabled" + ), + AZURE_TENANT_ID: z.string().optional().refine( + (val) => !process.env.AZURE_ENABLED || val != null, + "AZURE_TENANT_ID is required when Azure is enabled" + ),apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx (2)
60-60: Complete the implementation of Step 3.The Step 3 section appears to be incomplete, with an empty
<h3>tag.Please provide the necessary instructions for configuring the role assignment or remove the incomplete section.
19-22: 🛠️ Refactor suggestionImprove error handling for provider fetch.
The code only handles the null case but doesn't handle potential fetch errors.
- const provider = - await api.resource.provider.managed.azure.byProviderId(resourceProviderId); - - if (provider == null) return notFound(); + try { + const provider = + await api.resource.provider.managed.azure.byProviderId(resourceProviderId); + + if (provider == null) return notFound(); + } catch (error) { + console.error('Failed to fetch provider:', error); + throw new Error('Failed to load Azure provider configuration'); + }Likely invalid or redundant comment.
apps/webservice/src/app/api/azure/consent/route.ts (1)
30-32: 🛠️ Refactor suggestionAdd error handling for queue operations.
The resource scan queue initialization lacks error handling. Queue operations could fail silently.
Add error handling for queue operations:
const resourceScanQueue = new Queue<ResourceScanEvent>(Channel.ResourceScan, { connection: redis, }); + +resourceScanQueue.on('error', (error) => { + logger.error('Resource scan queue error:', error); +}); + +resourceScanQueue.on('failed', (job, error) => { + logger.error(`Job ${job.id} failed:`, error); +});Likely invalid or redundant comment.
packages/api/src/router/resource-provider.ts (2)
14-24: LGTM! Azure schema imports are properly integrated.The new Azure-related schema imports (
azureTenant,resourceProviderAzure) are correctly added alongside existing cloud provider schemas.
115-115: LGTM! Azure config is properly integrated in the response.The Azure configuration is correctly added to the provider response mapping, maintaining consistency with other cloud providers.
packages/db/src/schema/resource-provider.ts (3)
121-131: Add missing schema components for Azure tenant.The table definition needs additional components to maintain consistency with other provider implementations:
- Add relations definition for
resourceProviders- Add create/update schema with validation
- Add validation for Azure tenant ID format (GUID)
- Consider adding cascade delete for workspace reference
export const azureTenant = pgTable( "azure_tenant", { id: uuid("id").primaryKey().defaultRandom(), workspaceId: uuid("workspace_id") .notNull() - .references(() => workspace.id), + .references(() => workspace.id, { onDelete: "cascade" }), tenantId: text("tenant_id").notNull(), }, (t) => ({ uniq: uniqueIndex().on(t.tenantId) }), ); +export const azureTenantRelations = relations(azureTenant, ({ many }) => ({ + resourceProviders: many(resourceProviderAzure), +})); + +export const createAzureTenant = createInsertSchema(azureTenant, { + tenantId: z + .string() + .regex( + /^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$/, + "Invalid Azure Tenant ID format. Expected format: GUID", + ), +}).omit({ id: true }); + +export const updateAzureTenant = createAzureTenant.partial();
133-142: Add missing schema components and import flags for Azure provider.The table definition needs additional components to maintain consistency with other provider implementations:
- Add import flags for Azure services (AKS, Container Apps)
- Add relations definition
- Add validation for Azure subscription ID format (GUID)
export const resourceProviderAzure = pgTable("resource_provider_azure", { id: uuid("id").primaryKey().defaultRandom(), resourceProviderId: uuid("resource_provider_id") .notNull() .references(() => resourceProvider.id, { onDelete: "cascade" }), tenantId: uuid("tenant_id") .notNull() .references(() => azureTenant.id), subscriptionId: text("subscription_id").notNull(), + importAks: boolean("import_aks").notNull().default(false), + importContainerApps: boolean("import_container_apps") + .notNull() + .default(false), }); +export const resourceProviderAzureRelations = relations( + resourceProviderAzure, + ({ one }) => ({ + provider: one(resourceProvider, { + fields: [resourceProviderAzure.resourceProviderId], + references: [resourceProvider.id], + }), + tenant: one(azureTenant, { + fields: [resourceProviderAzure.tenantId], + references: [azureTenant.id], + }), + }), +);
144-165: 🛠️ Refactor suggestionAdd validation for Azure subscription ID.
The
createResourceProviderAzureschema should include validation for the subscription ID format, similar to how AWS and Google provider schemas include validation for their respective identifiers.export const createResourceProviderAzure = createInsertSchema( resourceProviderAzure, + { + subscriptionId: z + .string() + .regex( + /^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$/, + "Invalid Azure Subscription ID format. Expected format: GUID", + ), + } ) .omit({ id: true, resourceProviderId: true }) .extend({ name: z.string().min(1) });Likely invalid or redundant comment.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/ProviderActionsDropdown.tsx (3)
3-3: LGTM! Type definition improvement.The change to use
RouterOutputsfor the Provider type improves type safety by ensuring alignment with the API's type definitions.Also applies to: 32-32
40-42: LGTM! Consistent provider type checking.The addition of
provider.azureConfigcheck maintains consistency with existing provider type checks.
91-102: LGTM! Consistent Azure provider integration.The Azure provider edit dialog implementation follows the same pattern as AWS and Google providers, maintaining UI consistency.
| const onSubmit = form.handleSubmit((data: UpdateResourceProviderAzure) => { | ||
| setOpen(false); | ||
| router.push( | ||
| `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}?resourceProviderId=${resourceProvider.id}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and success feedback for form submission.
The form submission lacks error handling and success feedback.
const onSubmit = form.handleSubmit((data: UpdateResourceProviderAzure) => {
- setOpen(false);
- router.push(
- `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}?resourceProviderId=${resourceProvider.id}`,
- );
+ return router
+ .push(
+ `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}?resourceProviderId=${resourceProvider.id}`,
+ )
+ .then(() => {
+ setOpen(false);
+ onClose?.();
+ })
+ .catch((error) => {
+ console.error('Update failed:', error);
+ // TODO: Show error message to user
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onSubmit = form.handleSubmit((data: UpdateResourceProviderAzure) => { | |
| setOpen(false); | |
| router.push( | |
| `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}?resourceProviderId=${resourceProvider.id}`, | |
| ); | |
| }); | |
| const onSubmit = form.handleSubmit((data: UpdateResourceProviderAzure) => { | |
| return router | |
| .push( | |
| `/api/azure/${workspaceId}/${data.tenantId}/${data.subscriptionId}/${data.name}?resourceProviderId=${resourceProvider.id}`, | |
| ) | |
| .then(() => { | |
| setOpen(false); | |
| onClose?.(); | |
| }) | |
| .catch((error) => { | |
| console.error('Update failed:', error); | |
| // TODO: Show error message to user | |
| }); | |
| }); |
|
|
||
| if (provider == null) return notFound(); | ||
|
|
||
| const portalUrl = `https://portal.azure.com/#@${provider.azure_tenant.tenantId}/resource/subscriptions/${provider.resource_provider_azure.subscriptionId}/users`; |
There was a problem hiding this comment.
Ensure URL parameters are properly encoded.
The Azure portal URL construction should encode the tenant and subscription IDs to prevent potential URL injection.
- const portalUrl = `https://portal.azure.com/#@${provider.azure_tenant.tenantId}/resource/subscriptions/${provider.resource_provider_azure.subscriptionId}/users`;
+ const portalUrl = `https://portal.azure.com/#@${encodeURIComponent(provider.azure_tenant.tenantId)}/resource/subscriptions/${encodeURIComponent(provider.resource_provider_azure.subscriptionId)}/users`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const portalUrl = `https://portal.azure.com/#@${provider.azure_tenant.tenantId}/resource/subscriptions/${provider.resource_provider_azure.subscriptionId}/users`; | |
| const portalUrl = `https://portal.azure.com/#@${encodeURIComponent(provider.azure_tenant.tenantId)}/resource/subscriptions/${encodeURIComponent(provider.resource_provider_azure.subscriptionId)}/users`; |
| .values({ workspaceId, name }) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
|
|
||
| if (resourceProvider == null) | ||
| return NextResponse.json( | ||
| { error: "Failed to create resource provider" }, |
There was a problem hiding this comment.
Encode URL parameters to prevent injection.
The consent URL construction doesn't encode parameters, which could lead to URL injection vulnerabilities.
Apply this diff to encode URL parameters:
- const redirectUrl = `${baseUrl}/api/azure/consent?state=${state}`;
- const consentUrlExtension =
- resourceProviderId == null
- ? ""
- : `&resourceProviderId=${resourceProviderId}`;
- const consentUrl = `https://login.microsoftonline.com/${tenantId}/adminconsent?client_id=${clientId}&redirect_uri=${redirectUrl}${consentUrlExtension}`;
+ const redirectUrl = new URL(`${baseUrl}/api/azure/consent`);
+ redirectUrl.searchParams.set('state', state);
+ if (resourceProviderId != null) {
+ redirectUrl.searchParams.set('resourceProviderId', resourceProviderId);
+ }
+
+ const consentUrl = new URL(`https://login.microsoftonline.com/${encodeURIComponent(tenantId)}/adminconsent`);
+ consentUrl.searchParams.set('client_id', clientId);
+ consentUrl.searchParams.set('redirect_uri', redirectUrl.toString());Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
encoded in the frontend forms, since this is internal we can guarantee that these are already safe
There was a problem hiding this comment.
@adityachoudhari26 Understood. Since the parameters undergo schema validation and the route is internal, I'll make a note of this design decision.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#277
File: apps/webservice/src/app/api/azure/consent/route.ts:95-101
Timestamp: 2025-01-07T00:03:48.836Z
Learning: URL parameters in the Azure consent route (`/api/azure/consent`) are considered safe without additional encoding because:
1. Parameters are encoded in frontend forms
2. Route is internal and requires authentication
3. Values undergo schema validation with Zod before use
4. Database queries use prepared statements
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.
| const createResourceProvider = async ( | ||
| db: Tx, | ||
| workspaceId: string, | ||
| tenantId: string, | ||
| subscriptionId: string, | ||
| name: string, | ||
| ) => { | ||
| const resourceProvider = await db | ||
| .insert(SCHEMA.resourceProvider) | ||
| .values({ workspaceId, name }) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
|
|
||
| if (resourceProvider == null) | ||
| throw new Error("Failed to create resource provider"); | ||
|
|
||
| await db.insert(SCHEMA.resourceProviderAzure).values({ | ||
| resourceProviderId: resourceProvider.id, | ||
| tenantId, | ||
| subscriptionId, | ||
| }); | ||
|
|
||
| await resourceScanQueue.add( | ||
| resourceProvider.id, | ||
| { resourceProviderId: resourceProvider.id }, | ||
| { repeat: { every: ms("10m"), immediately: true } }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The createResourceProvider function needs:
- Input parameter validation
- Error handling for queue operations
const createResourceProvider = async (
db: Tx,
workspaceId: string,
tenantId: string,
subscriptionId: string,
name: string,
) => {
+ // Validate input parameters
+ if (!workspaceId || !tenantId || !subscriptionId || !name) {
+ throw new Error("Missing required parameters");
+ }
+
const resourceProvider = await db
.insert(SCHEMA.resourceProvider)
.values({ workspaceId, name })
.returning()
.then(takeFirstOrNull);
if (resourceProvider == null)
throw new Error("Failed to create resource provider");
await db.insert(SCHEMA.resourceProviderAzure).values({
resourceProviderId: resourceProvider.id,
tenantId,
subscriptionId,
});
- await resourceScanQueue.add(
- resourceProvider.id,
- { resourceProviderId: resourceProvider.id },
- { repeat: { every: ms("10m"), immediately: true } },
- );
+ try {
+ await resourceScanQueue.add(
+ resourceProvider.id,
+ { resourceProviderId: resourceProvider.id },
+ { repeat: { every: ms("10m"), immediately: true } },
+ );
+ } catch (error) {
+ logger.error("Failed to schedule resource scan:", error);
+ // Continue without failing the transaction as this is not critical
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| const baseUrl = env.BASE_URL; | ||
| const clientId = env.AZURE_APP_CLIENT_ID; | ||
|
|
||
| const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null }); | ||
| const resourceScanQueue = new Queue<ResourceScanEvent>(Channel.ResourceScan, { | ||
| connection: redis, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Redis operations.
The Redis client initialization and queue setup lack error handling. Consider:
- Adding error handling for Redis connection failures
- Implementing cleanup for expired consent states
+const handleRedisError = (error: Error) => {
+ logger.error("Redis operation failed:", error);
+ throw new Error("Service temporarily unavailable");
+};
-const redis = new IORedis(env.REDIS_URL, { maxRetriesPerRequest: null });
+const redis = new IORedis(env.REDIS_URL, {
+ maxRetriesPerRequest: null,
+ retryStrategy: (times) => Math.min(times * 50, 2000),
+});
+redis.on("error", handleRedisError);
const resourceScanQueue = new Queue<ResourceScanEvent>(Channel.ResourceScan, {
connection: redis,
+ defaultJobOptions: {
+ attempts: 3,
+ backoff: {
+ type: "exponential",
+ delay: 1000,
+ },
+ },
});Committable suggestion skipped: line range outside the PR's diff.
| if (tenant == null) { | ||
| const state = randomUUID(); | ||
| const config = { workspaceId, tenantId, subscriptionId, name }; | ||
| const configJSON = JSON.stringify(config); | ||
| await redis.set(`azure_consent_state:${state}`, configJSON, "EX", 900); | ||
| const redirectUrl = `${baseUrl}/api/azure/consent?state=${state}`; | ||
| const consentUrlExtension = | ||
| resourceProviderId == null | ||
| ? "" | ||
| : `&resourceProviderId=${resourceProviderId}`; | ||
| const consentUrl = `https://login.microsoftonline.com/${tenantId}/adminconsent?client_id=${clientId}&redirect_uri=${redirectUrl}${consentUrlExtension}`; | ||
| return NextResponse.redirect(consentUrl); | ||
| } |
There was a problem hiding this comment.
Enhance security of consent flow state management.
The consent flow state handling needs improvements:
- Use a more secure state parameter (e.g., signed JWT)
- Add rate limiting for consent requests
- Consider shorter expiration time for state parameter
if (tenant == null) {
- const state = randomUUID();
+ const state = await generateSecureState(config);
const configJSON = JSON.stringify(config);
- await redis.set(`azure_consent_state:${state}`, configJSON, "EX", 900);
+ await redis.set(`azure_consent_state:${state}`, configJSON, "EX", 300);
const redirectUrl = `${baseUrl}/api/azure/consent?state=${state}`;
const consentUrlExtension =
resourceProviderId == null
? ""
: `&resourceProviderId=${resourceProviderId}`;
- const consentUrl = `https://login.microsoftonline.com/${tenantId}/adminconsent?client_id=${clientId}&redirect_uri=${redirectUrl}${consentUrlExtension}`;
+ const consentUrl = new URL(`https://login.microsoftonline.com/${tenantId}/adminconsent`);
+ consentUrl.searchParams.set("client_id", clientId);
+ consentUrl.searchParams.set("redirect_uri", redirectUrl);
+ if (consentUrlExtension) {
+ consentUrl.searchParams.set("resourceProviderId", resourceProviderId);
+ }
return NextResponse.redirect(consentUrl);
}Committable suggestion skipped: line range outside the PR's diff.
| const { workspaceId, tenantId, subscriptionId, name } = params; | ||
| const { searchParams } = new URL(request.url); | ||
| const resourceProviderId = searchParams.get("resourceProviderId"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input parameter validation and sanitization.
The handler should validate and sanitize input parameters before use:
+const validateParams = (params: Params) => {
+ const { workspaceId, tenantId, subscriptionId, name } = params;
+
+ if (!/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(workspaceId)) {
+ throw new Error("Invalid workspace ID format");
+ }
+ if (!/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(tenantId)) {
+ throw new Error("Invalid tenant ID format");
+ }
+ if (!/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(subscriptionId)) {
+ throw new Error("Invalid subscription ID format");
+ }
+ if (!/^[\w-]{1,64}$/.test(name)) {
+ throw new Error("Invalid name format");
+ }
+};
const { workspaceId, tenantId, subscriptionId, name } = params;
+validateParams(params);
const { searchParams } = new URL(request.url);
const resourceProviderId = searchParams.get("resourceProviderId");
+if (resourceProviderId && !/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(resourceProviderId)) {
+ throw new Error("Invalid resource provider ID format");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { workspaceId, tenantId, subscriptionId, name } = params; | |
| const { searchParams } = new URL(request.url); | |
| const resourceProviderId = searchParams.get("resourceProviderId"); | |
| const validateParams = (params: Params) => { | |
| const { workspaceId, tenantId, subscriptionId, name } = params; | |
| if (!/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(workspaceId)) { | |
| throw new Error("Invalid workspace ID format"); | |
| } | |
| if (!/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(tenantId)) { | |
| throw new Error("Invalid tenant ID format"); | |
| } | |
| if (!/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(subscriptionId)) { | |
| throw new Error("Invalid subscription ID format"); | |
| } | |
| if (!/^[\w-]{1,64}$/.test(name)) { | |
| throw new Error("Invalid name format"); | |
| } | |
| }; | |
| const { workspaceId, tenantId, subscriptionId, name } = params; | |
| validateParams(params); | |
| const { searchParams } = new URL(request.url); | |
| const resourceProviderId = searchParams.get("resourceProviderId"); | |
| if (resourceProviderId && !/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/i.test(resourceProviderId)) { | |
| throw new Error("Invalid resource provider ID format"); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx (1)
29-30:⚠️ Potential issueEnsure URL parameters are properly encoded.
The Azure portal URL construction should encode the tenant and subscription IDs to prevent potential URL injection.
- const portalUrl = `https://portal.azure.com/#@${provider.azure_tenant.tenantId}/resource/subscriptions/${provider.resource_provider_azure.subscriptionId}/users`; - const applicationPortalUrl = `https://portal.azure.com/#@${provider.azure_tenant.tenantId}/blade/Microsoft_AAD_RegisteredApps/ApplicationMenuBlade/Overview/appId/${azureAppClientId}`; + const portalUrl = `https://portal.azure.com/#@${encodeURIComponent(provider.azure_tenant.tenantId)}/resource/subscriptions/${encodeURIComponent(provider.resource_provider_azure.subscriptionId)}/users`; + const applicationPortalUrl = `https://portal.azure.com/#@${encodeURIComponent(provider.azure_tenant.tenantId)}/blade/Microsoft_AAD_RegisteredApps/ApplicationMenuBlade/Overview/appId/${encodeURIComponent(azureAppClientId)}`;
🧹 Nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/PermissionsButton.tsx (3)
13-18: Consider adding error handling for the mutation.While the setup is clean, consider handling potential errors from the sync mutation to provide better user feedback.
export const PermissionsButton: React.FC<PermissionsButtonProps> = ({ resourceProviderId, }) => { const router = useRouter(); - const sync = api.resource.provider.managed.sync.useMutation(); + const sync = api.resource.provider.managed.sync.useMutation({ + onError: (error) => { + console.error('Sync failed:', error); + // Consider adding a toast notification here + } + }); const { workspaceSlug } = useParams<{ workspaceSlug: string }>();
20-24: Simplify async/await usage.The current implementation mixes async/await with Promise chains. Consider simplifying to use consistent async/await syntax.
const handleClick = async () => { - await sync - .mutateAsync(resourceProviderId) - .then(() => router.push(`/${workspaceSlug}/resource-providers`)); + try { + await sync.mutateAsync(resourceProviderId); + router.push(`/${workspaceSlug}/resource-providers`); + } catch (error) { + console.error('Failed to sync:', error); + // Handle error appropriately + } };
26-31: Enhance button feedback during sync.Consider showing a loading state in the button text to provide better user feedback.
return ( <Button onClick={handleClick} disabled={sync.isPending}> - Permissions granted + {sync.isPending ? "Syncing..." : "Permissions granted"} </Button> );apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx (2)
1-14: Consider stronger environment variable validation.The environment variable check could be more explicit about the required format.
-const azureAppClientId = env.AZURE_APP_CLIENT_ID; +const azureAppClientId = env.AZURE_APP_CLIENT_ID?.trim(); +if (!azureAppClientId || !/^[0-9a-f-]{36}$/i.test(azureAppClientId)) { + throw new Error('Invalid AZURE_APP_CLIENT_ID format'); +}
16-28: Enhance error handling for provider fetch.Consider handling potential errors from the provider fetch operation more gracefully.
- const provider = - await api.resource.provider.managed.azure.byProviderId(resourceProviderId); - if (provider == null) return notFound(); + try { + const provider = + await api.resource.provider.managed.azure.byProviderId(resourceProviderId); + if (provider == null) return notFound(); + } catch (error) { + console.error('Failed to fetch provider:', error); + throw new Error('Failed to load Azure provider details'); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/PermissionsButton.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx (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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/PermissionsButton.tsx (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 (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/PermissionsButton.tsx (1)
1-11: LGTM! Clean imports and type definitions.The imports are well-organized and the prop type is clearly defined.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/[resourceProviderId]/page.tsx (1)
32-100: LGTM! Well-structured UI implementation.The UI is well-organized with clear step-by-step instructions and proper security attributes for external links.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx (2)
80-91: Add missing FormMessage component.The
subscriptionIdfield is missing theFormMessagecomponent that's present in other fields.Add the missing component:
<FormItem> <FormLabel>Subscription ID</FormLabel> <FormControl> <Input {...field} /> </FormControl> + <FormMessage /> </FormItem>
39-102: Enhance dialog UX with additional controls.Consider the following UX improvements:
- Add a cancel button to dismiss the dialog
- Add placeholder text to input fields for guidance
- Add helper text explaining the required Azure IDs
Example improvement for the dialog actions:
<Button type="submit" disabled={!form.formState.isValid || form.formState.isSubmitting} > Save </Button> + <Button + type="button" + variant="outline" + onClick={() => form.reset()} + > + Cancel + </Button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/UpdateAzureProviderDialog.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/UpdateAzureProviderDialog.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx (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 (1)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/azure/CreateAzureProviderDialog.tsx (2)
1-25: LGTM! Clean imports and type definitions.The imports are well-organized and the props type is properly defined.
27-32: Verify form validation schema requirements.The form is initialized with
createResourceProviderAzureschema, but we should verify the validation requirements for each field.Let's check the schema definition:
✅ Verification successful
Form validation schema requirements are properly defined.
The form is correctly initialized with a schema that validates:
- Required name field (min length 1)
- Required tenantId (must be a valid Azure tenant UUID)
- Required subscriptionId
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find and analyze the schema definition for Azure provider validation # Search for the schema definition rg -A 10 "createResourceProviderAzure.*=.*{" --type typescriptLength of output: 194
Script:
#!/bin/bash # Try multiple approaches to find the schema definition # Search for the variable name rg "createResourceProviderAzure" --type typescript # Search for potential schema files fd "azure.*schema" --type f # Use ast-grep to find potential type/const definitions ast-grep --pattern 'const createResourceProviderAzure = $_' # Search for files with both "azure" and "schema" in content rg -l "azure.*schema" --type typescriptLength of output: 672
Script:
#!/bin/bash # Get complete schema definitions # View the full schema file cat packages/db/src/schema/resource-provider.ts # Search for createInsertSchema definition rg "createInsertSchema" -A 5 --no-filenameLength of output: 15109
| const onSubmit = form.handleSubmit((data) => | ||
| router.push( | ||
| `/api/azure/${workspaceId}/${encodeURIComponent(data.tenantId)}/${encodeURIComponent(data.subscriptionId)}/${encodeURIComponent(data.name)}`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve URL construction.
The current implementation has several potential improvements:
- Add error handling for failed navigation
- Consider using a URL builder utility for complex paths
- Add loading state management
Consider this improvement:
const onSubmit = form.handleSubmit((data) =>
- router.push(
- `/api/azure/${workspaceId}/${encodeURIComponent(data.tenantId)}/${encodeURIComponent(data.subscriptionId)}/${encodeURIComponent(data.name)}`,
- ),
+ router.push(
+ `/api/azure/${workspaceId}/${encodeURIComponent(data.tenantId)}/${encodeURIComponent(data.subscriptionId)}/${encodeURIComponent(data.name)}`,
+ ).catch((error) => {
+ console.error('Navigation failed:', error);
+ // TODO: Add user feedback for navigation failure
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onSubmit = form.handleSubmit((data) => | |
| router.push( | |
| `/api/azure/${workspaceId}/${encodeURIComponent(data.tenantId)}/${encodeURIComponent(data.subscriptionId)}/${encodeURIComponent(data.name)}`, | |
| ), | |
| ); | |
| const onSubmit = form.handleSubmit((data) => | |
| router.push( | |
| `/api/azure/${workspaceId}/${encodeURIComponent(data.tenantId)}/${encodeURIComponent(data.subscriptionId)}/${encodeURIComponent(data.name)}`, | |
| ).catch((error) => { | |
| console.error('Navigation failed:', error); | |
| // TODO: Add user feedback for navigation failure | |
| }), | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/event-worker/src/config.ts (1)
15-16: LGTM! Consider adding documentation for Azure configuration.The Azure credential configuration follows the established pattern and security practices. However, adding documentation about the required Azure App Registration setup would be helpful.
Consider adding a comment block above these variables explaining:
/** * Azure App Registration credentials used for Azure resource management. * To configure: * 1. Create an App Registration in Azure Portal * 2. Generate a client secret * 3. Set appropriate API permissions */apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (3)
40-43: Consider a more maintainable approach for custom provider detection.The current implementation might become fragile as new cloud providers are added. Consider using a more explicit approach.
-const isCustomProvider = (provider: ResourceProvider) => - provider.googleConfig == null && - provider.awsConfig == null && - provider.azureConfig == null; +const isCustomProvider = (provider: ResourceProvider) => { + const managedConfigs = ['googleConfig', 'awsConfig', 'azureConfig']; + return managedConfigs.every(config => provider[config] == null); +};
45-96: LGTM! Well-structured UI components with clear responsibilities.The components are well-implemented with proper typing and consistent styling. Consider adding aria-labels for better accessibility.
<Badge variant="outline" + aria-label="Custom provider badge" className="h-6 gap-1.5 rounded-full border-none bg-blue-500/10 pl-2 pr-3 text-xs text-blue-300" >
181-182: Consider standardizing date formatting.The current date formatting might vary across different locales. Consider using a date formatting library or explicit format options for consistency.
-{new Date(provider.createdAt).toLocaleDateString()} +{new Date(provider.createdAt).toLocaleDateString('en-US', { + year: 'numeric', + month: 'short', + day: 'numeric' +})}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/config.ts(1 hunks)apps/event-worker/src/resource-scan/azure/aks.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx(5 hunks)apps/webservice/src/app/api/azure/consent/route.ts(1 hunks)apps/webservice/src/env.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/env.ts
- apps/event-worker/src/resource-scan/azure/aks.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/event-worker/src/config.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/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (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/webservice/src/app/api/azure/consent/route.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.
📓 Learnings (1)
apps/webservice/src/app/api/azure/consent/route.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#277
File: apps/webservice/src/app/api/azure/consent/route.ts:95-101
Timestamp: 2025-01-07T00:03:48.992Z
Learning: URL parameters in the Azure consent route (`/api/azure/consent`) are considered safe without additional encoding because:
1. Parameters are encoded in frontend forms
2. Route is internal and requires authentication
3. Values undergo schema validation with Zod before use
4. Database queries use prepared statements
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build
🔇 Additional comments (5)
apps/webservice/src/app/api/azure/consent/route.ts (3)
17-17: Add retry limits and error handling for Redis connectionThe Redis connection is configured without retry limits (
maxRetriesPerRequest: null), which could lead to infinite retries.
23-28: LGTM! Schema validation is well-defined.The schema correctly validates all required fields for Azure configuration.
30-48: Great improvements in error handling and Redis state management!The code now properly:
- Sets HTTP status codes in error responses
- Cleans up Redis state after use
apps/event-worker/src/config.ts (1)
15-16: Verify Azure credential configuration across the codebase.Let's ensure the Azure credentials are properly utilized across the codebase.
✅ Verification successful
Azure credentials are properly configured and utilized ✅
The Azure credentials are correctly implemented across the codebase with proper separation of concerns:
- Web service only uses CLIENT_ID for frontend operations
- Event worker securely handles both CLIENT_ID and CLIENT_SECRET for AKS operations
- No hardcoded credentials or insecure patterns found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Azure credential usage patterns # Check for Azure credential initialization echo "Checking Azure credential initialization patterns..." rg "AZURE_APP_CLIENT" -A 5 # Check for potential hardcoded Azure credentials echo "Checking for potential hardcoded Azure credentials..." rg -i "azure.*client.*secret" --type ts # Look for Azure SDK initialization patterns echo "Checking Azure SDK initialization patterns..." ast-grep --pattern 'new DefaultAzureCredential($$$)'Length of output: 4569
Script:
#!/bin/bash # Check AKS implementation and other Azure services echo "Checking AKS implementation..." cat apps/event-worker/src/resource-scan/azure/aks.ts echo -e "\nChecking for other Azure service clients..." rg "from '@azure/" --type ts -B 2 -A 2Length of output: 2090
Script:
#!/bin/bash # Check for Azure service imports with a more specific pattern echo "Checking Azure service imports..." rg "from '@azure/(arm-|identity)" --type ts echo -e "\nChecking Azure credential usage..." rg "ClientSecretCredential|DefaultAzureCredential" --type tsLength of output: 478
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/page.tsx (1)
1-11: LGTM! Well-organized imports and type definitions.The imports are logically grouped and the ResourceProvider type is properly derived from the API output type.
Also applies to: 36-38
| .insert(SCHEMA.resourceProviderAzure) | ||
| .values({ | ||
| resourceProviderId: resourceProvider.id, | ||
| tenantId: tenant.id, |
There was a problem hiding this comment.
Fix inconsistent tenant ID usage
The code uses tenant.id here but tenantId (from request) in the earlier insert at line 65. This seems incorrect as tenant.id is the database ID while tenantId is the Azure tenant ID.
- tenantId: tenant.id,
+ tenantId: tenantId,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tenantId: tenant.id, | |
| tenantId: tenantId, |
| .then(() => | ||
| resourceScanQueue.add( | ||
| resourceProvider.id, | ||
| { resourceProviderId: resourceProvider.id }, | ||
| { repeat: { every: ms("10m"), immediately: true } }, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle queue job rollback on transaction failure
The queue job is created after the transaction commits, but if the final database operation fails, the job will still exist. Consider moving the queue operation inside the transaction or implementing a cleanup mechanism.
- .then(() =>
- resourceScanQueue.add(
- resourceProvider.id,
- { resourceProviderId: resourceProvider.id },
- { repeat: { every: ms("10m"), immediately: true } },
- ),
- )
+ .then(async () => {
+ try {
+ await resourceScanQueue.add(
+ resourceProvider.id,
+ { resourceProviderId: resourceProvider.id },
+ { repeat: { every: ms("10m"), immediately: true } },
+ );
+ } catch (error) {
+ // If queue job creation fails, rollback the transaction
+ throw error;
+ }
+ })Committable suggestion skipped: line range outside the PR's diff.
| {provider.kinds.length === 0 && ( | ||
| <span className="text-xs italic text-muted-foreground"> | ||
| No resources | ||
| </span> | ||
| )} | ||
| {provider.kinds.length > 0 && ( | ||
| <div className="flex gap-2 overflow-x-auto"> | ||
| {provider.kinds.map((kind) => ( | ||
| <Badge | ||
| key={kind.kind} | ||
| variant="outline" | ||
| className="h-6 gap-1.5 rounded-full border-none bg-neutral-800/50 px-2 text-xs text-muted-foreground" | ||
| > | ||
| {kind.version}:{kind.kind} | ||
| </Badge> | ||
| ))} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
💡 Codebase verification
Follow established badge list pattern
The codebase already has a proven pattern for handling badge lists in ReleaseBadgeList.tsx. Let's maintain consistency by using the same approach:
-<div className="flex gap-2 overflow-x-auto">
+<div className="flex gap-2">
{provider.kinds.map((kind, index) => (
index < 3 ? (
<Badge
key={kind.kind}
variant="outline"
className="h-6 gap-1.5 rounded-full border-none bg-neutral-800/50 px-2 text-xs text-muted-foreground"
>
- {kind.version}:{kind.kind}
+ <span className="max-w-32 truncate">
+ {kind.version}:{kind.kind}
+ </span>
</Badge>
) : index === 3 ? (
<Badge
variant="outline"
className="h-6 gap-1.5 rounded-full border-none bg-neutral-800/50 px-2 text-xs text-muted-foreground"
>
+{provider.kinds.length - 3}
</Badge>
) : null
))}
</div>🔗 Analysis chain
Verify overflow behavior in the kinds column.
While the overflow handling is implemented, the horizontal scrolling within a table cell might lead to poor user experience, especially with many kinds.
Consider these alternatives:
- Truncate with ellipsis and show full list in tooltip
- Stack badges vertically with a "show more" button
- Use a popover for displaying all kinds
Example implementation of option 1:
-<div className="flex gap-2 overflow-x-auto">
+<div className="flex gap-2 max-w-[300px] overflow-hidden">
{provider.kinds.map((kind, index) => (
index < 3 ? (
<Badge
key={kind.kind}
variant="outline"
className="h-6 gap-1.5 rounded-full border-none bg-neutral-800/50 px-2 text-xs text-muted-foreground"
>
{kind.version}:{kind.kind}
</Badge>
) : index === 3 ? (
<TooltipProvider>
<Tooltip>
<TooltipTrigger>
<Badge>+{provider.kinds.length - 3} more</Badge>
</TooltipTrigger>
<TooltipContent>
{provider.kinds.slice(3).map(k => `${k.version}:${k.kind}`).join(', ')}
</TooltipContent>
</Tooltip>
</TooltipProvider>
) : null
))}
</div>🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Look for provider data structure and mock data
rg -g "*.ts*" "provider.*kinds.*" -B2 -A2
# Find similar badge/tag implementations
rg -g "*.tsx" "Badge.*variant.*outline.*" -B2 -A2
# Look for tooltip/popover usage patterns
ast-grep --pattern 'TooltipProvider { $$$ }'
Length of output: 3657
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/db/drizzle/0052_groovy_zuras.sql (3)
1-5: Consider adding CASCADE and indexes for better data integrity and performance.While the table structure is correct, consider the following improvements:
- Add ON DELETE CASCADE to the workspace_id foreign key to automatically clean up tenant data when a workspace is deleted
- Add an index on workspace_id for better query performance on foreign key joins
CREATE INDEX IF NOT EXISTS "azure_tenant_workspace_id_index" ON "azure_tenant" ("workspace_id");
7-12: Add indexes to improve query performance.Consider adding the following indexes to improve query performance:
- Index on tenant_id for foreign key lookups
- Index on resource_provider_id for foreign key lookups
- Compound index on (tenant_id, subscription_id) for filtering resources by tenant and subscription
CREATE INDEX IF NOT EXISTS "resource_provider_azure_tenant_id_index" ON "resource_provider_azure" ("tenant_id"); CREATE INDEX IF NOT EXISTS "resource_provider_azure_resource_provider_id_index" ON "resource_provider_azure" ("resource_provider_id"); CREATE INDEX IF NOT EXISTS "resource_provider_azure_tenant_subscription_index" ON "resource_provider_azure" ("tenant_id", "subscription_id");
1-32: Consider adding a down migration script.While the migration successfully sets up the Azure resource management tables, consider adding a corresponding down migration script to handle rollback scenarios. This ensures that changes can be safely reversed if needed.
-- Down migration example DROP TABLE IF EXISTS "resource_provider_azure"; DROP TABLE IF EXISTS "azure_tenant";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/drizzle/0052_groovy_zuras.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/drizzle/meta/_journal.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/db/drizzle/0052_groovy_zuras.sql (2)
14-30: Review ON DELETE behavior consistency.The foreign key constraints have inconsistent ON DELETE behaviors:
- resource_provider_id uses CASCADE
- workspace_id and tenant_id use NO ACTION
This might lead to orphaned records. Consider standardizing the deletion behavior based on your data integrity requirements.
✅ Verification successful
Foreign key constraints follow established patterns
The different ON DELETE behaviors are intentional and consistent with the codebase patterns:
- NO ACTION prevents accidental deletion of core configurations (workspace, tenant)
- CASCADE automatically cleans up dependent records (provider configurations)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other similar foreign key patterns in the schema rg -A 2 "ON DELETE cascade" packages/db/drizzle/ rg -A 2 "ON DELETE no action" packages/db/drizzle/Length of output: 41726
32-32: Verify unique tenant_id requirement.The unique index on tenant_id implies that an Azure tenant can only be associated with one workspace. Please verify if this is the intended business requirement, as it prevents the same Azure tenant from being used across multiple workspaces.
✅ Verification successful
Unique tenant_id constraint is intentional
The unique constraint on tenant_id is a deliberate design choice, enforced consistently across the system:
- Database schema ensures each Azure tenant belongs to exactly one workspace
- API layer validates tenant-workspace relationship with proper error handling
- Resource providers are linked to azure_tenant through foreign key constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related business logic or tests that verify this constraint rg -A 5 "tenant_id.*unique" apps/ rg -A 5 "tenant.*workspace" apps/Length of output: 803
Script:
#!/bin/bash # Search for azure tenant creation and workspace relationship ast-grep --pattern 'async function create$_($$$) { $$$ }' | rg -A 10 'azure.*tenant' rg -A 5 "azure.*tenant.*create" rg -A 5 "INSERT.*azure_tenant"Length of output: 153
Script:
#!/bin/bash # Search for azure tenant related code with broader patterns rg -A 5 "azure_tenant" --type sql rg -A 5 "class.*AzureTenant" rg -A 5 "interface.*AzureTenant" rg -A 5 "type.*AzureTenant"Length of output: 2141
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts (4)
29-32:⚠️ Potential issueAdd error handling for Redis operations.
The Redis client initialization and queue setup need error handling for connection failures and cleanup for expired consent states.
41-54:⚠️ Potential issueEnsure atomicity with transaction handling.
The resource provider creation and Azure-specific data insertion should be wrapped in a transaction to maintain data consistency.
69-72:⚠️ Potential issueAdd input parameter validation.
The handler should validate and sanitize input parameters before use, including format validation for UUIDs and name restrictions.
92-104:⚠️ Potential issueEnhance security of consent flow.
The consent flow state handling needs improvements:
- Use a more secure state parameter
- Add rate limiting
- Consider shorter expiration time
- Use URL constructor for safer URL building
🧹 Nitpick comments (3)
apps/docs/pages/integrations/azure/compute-scanner.mdx (2)
6-8: Add comma after 'Currently' and consider rewording for clarity.-Currently the Azure Compute Scanner supports importing the following resources: +Currently, the Azure Compute Scanner supports importing the following resources:🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...they are created and deleted in Azure. Currently the Azure Compute Scanner supports impo...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
12-14: Consider rewording for conciseness.-The managed Azure Compute Scanner is built into the Ctrlplane solution. In order to configure it, -you must consent to our Azure AD application in your Azure tenant and give it the necessary permissions -to scan your Azure resources. +The managed Azure Compute Scanner is built into the Ctrlplane solution. To configure it, +you must consent to our Azure AD application in your Azure tenant and grant necessary permissions +for resource scanning.🧰 Tools
🪛 LanguageTool
[style] ~12-~12: Consider a shorter alternative to avoid wordiness.
Context: ...r is built into the Ctrlplane solution. In order to configure it, you must consent to our A...(IN_ORDER_TO_PREMIUM)
apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts (1)
73-153: Consider breaking down the transaction handler.The transaction handler is quite long and handles multiple responsibilities. Consider extracting the tenant verification and resource provider update/create logic into separate functions for better maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
apps/docs/public/integrations/azure/azure-add-member.pngis excluded by!**/*.pngapps/docs/public/integrations/azure/azure-reader-role.pngis excluded by!**/*.pngapps/docs/public/integrations/azure/azure-role-assignment.pngis excluded by!**/*.pngapps/docs/public/integrations/azure/azure-scanner-ctrlplane.pngis excluded by!**/*.pngapps/docs/public/integrations/azure/azure-select-members.pngis excluded by!**/*.png
📒 Files selected for processing (3)
apps/docs/pages/integrations/_meta.ts(1 hunks)apps/docs/pages/integrations/azure/compute-scanner.mdx(1 hunks)apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/docs/pages/integrations/_meta.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/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.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.
🪛 LanguageTool
apps/docs/pages/integrations/azure/compute-scanner.mdx
[uncategorized] ~6-~6: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...they are created and deleted in Azure. Currently the Azure Compute Scanner supports impo...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~12-~12: Consider a shorter alternative to avoid wordiness.
Context: ...r is built into the Ctrlplane solution. In order to configure it, you must consent to our A...
(IN_ORDER_TO_PREMIUM)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ations/azure/azure-select-members.png) In the select members page, search for `ct...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/docs/pages/integrations/_meta.ts (1)
5-5: LGTM!The Azure integration entry is correctly added and maintains alphabetical ordering.
apps/docs/pages/integrations/azure/compute-scanner.mdx (1)
1-68: Well-structured documentation with clear instructions!The documentation is comprehensive and well-organized, with:
- Clear explanation of the scanner's purpose
- Detailed setup instructions
- Step-by-step consent process
- Helpful screenshots
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...they are created and deleted in Azure. Currently the Azure Compute Scanner supports impo...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~12-~12: Consider a shorter alternative to avoid wordiness.
Context: ...r is built into the Ctrlplane solution. In order to configure it, you must consent to our A...(IN_ORDER_TO_PREMIUM)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ations/azure/azure-select-members.png) In the select members page, search for `ct...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
apps/webservice/src/app/api/azure/[workspaceId]/[tenantId]/[subscriptionId]/[name]/route.ts (1)
56-60:⚠️ Potential issueAdd error handling for queue operations.
- await resourceScanQueue.add( - resourceProvider.id, - { resourceProviderId: resourceProvider.id }, - { repeat: { every: ms("10m"), immediately: true } }, - ); + try { + await resourceScanQueue.add( + resourceProvider.id, + { resourceProviderId: resourceProvider.id }, + { + repeat: { every: ms("10m"), immediately: true }, + attempts: 3, + backoff: { + type: "exponential", + delay: 1000, + }, + }, + ); + } catch (error) { + logger.error("Failed to schedule resource scan:", error); + throw new Error("Failed to schedule resource scan"); + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/db/drizzle/0054_loud_amphibian.sql (2)
1-5: Consider adding audit columns to azure_tenant table.For better tracking and debugging capabilities, consider adding:
created_atTIMESTAMP WITH TIME ZONEupdated_atTIMESTAMP WITH TIME ZONECREATE TABLE IF NOT EXISTS "azure_tenant" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "workspace_id" uuid NOT NULL, "tenant_id" text NOT NULL, + "created_at" timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP, + "updated_at" timestamptz NOT NULL DEFAULT CURRENT_TIMESTAMP );
32-32: Add index for subscription_id lookups.Consider adding an index on
resource_provider_azure.subscription_idto improve query performance when looking up resources by subscription ID.CREATE UNIQUE INDEX IF NOT EXISTS "azure_tenant_tenant_id_index" ON "azure_tenant" USING btree ("tenant_id"); +CREATE INDEX IF NOT EXISTS "resource_provider_azure_subscription_id_index" ON "resource_provider_azure" USING btree ("subscription_id");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/db/drizzle/0054_loud_amphibian.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/validators/src/auth/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/validators/src/auth/index.ts
- packages/db/drizzle/meta/_journal.json
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Typecheck
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: build
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
| CREATE TABLE IF NOT EXISTS "resource_provider_azure" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "resource_provider_id" uuid NOT NULL, | ||
| "tenant_id" uuid NOT NULL, | ||
| "subscription_id" text NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unique constraint to prevent duplicate Azure subscriptions.
To maintain data integrity and prevent the same Azure subscription from being registered multiple times under the same resource provider, add a unique constraint on (resource_provider_id, subscription_id).
CREATE TABLE IF NOT EXISTS "resource_provider_azure" (
"id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL,
"resource_provider_id" uuid NOT NULL,
"tenant_id" uuid NOT NULL,
"subscription_id" text NOT NULL,
+ UNIQUE ("resource_provider_id", "subscription_id")
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE IF NOT EXISTS "resource_provider_azure" ( | |
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | |
| "resource_provider_id" uuid NOT NULL, | |
| "tenant_id" uuid NOT NULL, | |
| "subscription_id" text NOT NULL | |
| ); | |
| CREATE TABLE IF NOT EXISTS "resource_provider_azure" ( | |
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | |
| "resource_provider_id" uuid NOT NULL, | |
| "tenant_id" uuid NOT NULL, | |
| "subscription_id" text NOT NULL, | |
| UNIQUE ("resource_provider_id", "subscription_id") | |
| ); |
| DO $$ BEGIN | ||
| ALTER TABLE "azure_tenant" ADD CONSTRAINT "azure_tenant_workspace_id_workspace_id_fk" FOREIGN KEY ("workspace_id") REFERENCES "public"."workspace"("id") ON DELETE no action ON UPDATE no action; | ||
| EXCEPTION | ||
| WHEN duplicate_object THEN null; | ||
| END $$; | ||
| --> statement-breakpoint | ||
| DO $$ BEGIN | ||
| ALTER TABLE "resource_provider_azure" ADD CONSTRAINT "resource_provider_azure_resource_provider_id_resource_provider_id_fk" FOREIGN KEY ("resource_provider_id") REFERENCES "public"."resource_provider"("id") ON DELETE cascade ON UPDATE no action; | ||
| EXCEPTION | ||
| WHEN duplicate_object THEN null; | ||
| END $$; | ||
| --> statement-breakpoint | ||
| DO $$ BEGIN | ||
| ALTER TABLE "resource_provider_azure" ADD CONSTRAINT "resource_provider_azure_tenant_id_azure_tenant_id_fk" FOREIGN KEY ("tenant_id") REFERENCES "public"."azure_tenant"("id") ON DELETE no action ON UPDATE no action; | ||
| EXCEPTION | ||
| WHEN duplicate_object THEN null; | ||
| END $$; |
There was a problem hiding this comment.
💡 Codebase verification
Confirm using CASCADE for Azure tenant foreign key.
The current NO ACTION behavior for the Azure tenant foreign key could lead to orphaned records. Using CASCADE is more appropriate because:
- Azure resources cannot function without their tenant
- It maintains consistency with how other cloud provider resources are handled
- It prevents referential integrity issues
🔗 Analysis chain
Review inconsistent foreign key deletion behaviors.
The current foreign key constraints have inconsistent deletion behaviors:
resource_provider_iduses CASCADEtenant_idandworkspace_iduse NO ACTION
This could lead to orphaned records when an Azure tenant is deleted. Consider using CASCADE for the tenant_id foreign key to maintain referential integrity.
-ALTER TABLE "resource_provider_azure" ADD CONSTRAINT "resource_provider_azure_tenant_id_azure_tenant_id_fk" FOREIGN KEY ("tenant_id") REFERENCES "public"."azure_tenant"("id") ON DELETE no action ON UPDATE no action;
+ALTER TABLE "resource_provider_azure" ADD CONSTRAINT "resource_provider_azure_tenant_id_azure_tenant_id_fk" FOREIGN KEY ("tenant_id") REFERENCES "public"."azure_tenant"("id") ON DELETE cascade ON UPDATE no action;Let's verify the existing foreign key behaviors in the database:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check foreign key behaviors in the database schema
# Search for other related foreign key constraints and their deletion behaviors
rg -A 5 "FOREIGN KEY.*REFERENCES.*workspace|resource_provider|azure"
Length of output: 95798
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/event-worker/src/resource-scan/azure/aks.ts (2)
16-30: Enhance error handling with more descriptive messages.While the validation logic is good, consider:
- Adding error handling for database query failures
- Including more context in error messages
if (!AZURE_CLIENT_ID || !AZURE_CLIENT_SECRET) - throw new Error("Invalid azure credentials"); + throw new Error("Missing required Azure credentials: AZURE_APP_CLIENT_ID or AZURE_APP_CLIENT_SECRET"); const tenant = await db .select() .from(SCHEMA.azureTenant) .where(eq(SCHEMA.azureTenant.id, azureProvider.tenantId)) - .then(takeFirstOrNull); + .then(takeFirstOrNull) + .catch((error) => { + throw new Error(`Failed to query Azure tenant: ${error.message}`); + }); -if (!tenant) throw new Error("Tenant not found"); +if (!tenant) throw new Error(`Azure tenant not found for ID: ${azureProvider.tenantId}`);
42-55: Add error handling and optimize memory usage for cluster retrieval.Consider adding error handling for the async iteration and optimizing memory usage:
- const res = client.managedClusters.list(); - - const clusters: ManagedCluster[] = []; - for await (const cluster of res) { - clusters.push(cluster); - } + try { + const clusters: ManagedCluster[] = []; + for await (const cluster of client.managedClusters.list()) { + clusters.push(cluster); + } + + const { resourceProviderId: providerId } = azureProvider; + return clusters + .map((cluster) => + convertManagedClusterToResource(workspace.id, providerId, cluster), + ) + .filter(isPresent); + } catch (error) { + throw new Error(`Failed to list AKS clusters: ${error.message}`); + }apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts (2)
16-45: Enhance cluster validation and simplify optional chaining.Consider adding more comprehensive validation and simplifying the code:
- if (!cluster.name || !cluster.id) { + if (!cluster?.name || !cluster?.id || !cluster?.nodeResourceGroup) { log.error("Invalid cluster", { cluster }); return null; } return { workspaceId, providerId, name: cluster.name, identifier: cluster.id, version: "kubernetes/v1", kind: "ClusterAPI", config: { name: cluster.name, auth: { method: "azure/aks", clusterName: cluster.name, - resourceGroup: cluster.nodeResourceGroup ?? "", + resourceGroup: cluster.nodeResourceGroup, }, status: cluster.provisioningState ?? "UNKNOWN", server: { endpoint: cluster.fqdn ?? "", - certificateAuthorityData: cluster.servicePrincipalProfile?.clientId, + certificateAuthorityData: cluster.servicePrincipalProfile?.clientId ?? null, }, },
46-171: Optimize metadata handling and improve maintainability.Consider the following improvements:
- Define metadata keys as constants to prevent typos
- Optimize JSON.stringify usage
- Simplify deeply nested property access
+const AZURE_METADATA_KEYS = { + SELF_LINK: "azure/self-link", + RESOURCE_GROUP: "azure/resource-group", + // ... define other keys +} as const; +const safeStringify = (value: unknown) => + value ? JSON.stringify(value) : undefined; metadata: omitNullUndefined({ [ReservedMetadataKey.Links]: cluster.azurePortalFqdn ? JSON.stringify({ "Azure Portal": cluster.azurePortalFqdn }) : undefined, [ReservedMetadataKey.ExternalId]: cluster.id ?? "", [ReservedMetadataKey.KubernetesFlavor]: "aks", [ReservedMetadataKey.KubernetesVersion]: cluster.currentKubernetesVersion, - "azure/self-link": cluster.id, - "azure/resource-group": cluster.nodeResourceGroup, + [AZURE_METADATA_KEYS.SELF_LINK]: cluster.id, + [AZURE_METADATA_KEYS.RESOURCE_GROUP]: cluster.nodeResourceGroup, // Simplify deeply nested properties - "azure/addon-profiles/ingress-application-gateway/config/application-gateway-id": - cluster.addonProfiles?.ingressApplicationGateway?.config - ?.applicationGatewayId, + "azure/addon-profiles/ingress-application-gateway/config/application-gateway-id": + cluster.addonProfiles?.ingressApplicationGateway?.config?.applicationGatewayId ?? undefined, // Optimize JSON.stringify usage - "azure/agent-pool-profiles": JSON.stringify(cluster.agentPoolProfiles), + "azure/agent-pool-profiles": safeStringify(cluster.agentPoolProfiles),
📜 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(1 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 (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/event-worker/src/resource-scan/azure/aks.ts (2)
1-15: LGTM! Good security practices for credential handling.The code properly uses environment variables for sensitive Azure credentials and has appropriate imports for Azure SDK packages.
31-41: LGTM! Proper Azure client initialization.The code follows Azure SDK best practices for client initialization and maintains type safety.
apps/event-worker/src/resource-scan/azure/cluster-to-resource.ts (1)
1-15: LGTM! Well-structured type definitions and logging setup.The code has clear type definitions and proper logging configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
apps/docs/pages/integrations/azure/compute-scanner.mdx (8)
3-6: Add specificity about sync behavior.Consider clarifying the sync behavior by specifying if it's real-time, periodic, or event-driven. This helps set correct expectations for users.
-and creates corresponding resources in Ctrlplane. It will keep resources in -sync, creating and deleting resources as they are created and deleted in Azure. +and creates corresponding resources in Ctrlplane. It maintains real-time synchronization, +automatically creating and deleting resources as changes occur in Azure.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...they are created and deleted in Azure. Currently the Azure Compute Scanner supports impo...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
7-9: Make resource support limitations more explicit.Consider adding a note about active development to set expectations about future resource type support.
-Currently the Azure Compute Scanner supports importing the following resources: +Currently, the Azure Compute Scanner supports importing the following resources (with more resource types planned for future releases):
21-21: Improve URL template formatting.Consider using a more distinct format for the placeholder to make it clearer that users need to replace it.
-`https://app.ctrlplane.dev/{workspace-slug}/resource-providers/integrations`. +`https://app.ctrlplane.dev/<YOUR_WORKSPACE_SLUG>/resource-providers/integrations`
29-33: Highlight tenant configuration constraints.Consider using a callout or warning block to emphasize these important tenant configuration constraints.
-Once a tenant has been configured in a workspace, it cannot be added to another -workspace. However, you can add multiple tenants to a workspace, and the same -tenant can be used for multiple scanners within the same workspace (for example -adding multiple scanners for different subscriptions). +> **Important:** A tenant can only be configured in one workspace. However: +> - You can add multiple tenants to a single workspace +> - The same tenant can be used for multiple scanners within that workspace (e.g., for different subscriptions)
49-49: Standardize URL placeholder format and add explanation.The URL template uses inconsistent placeholder format and lacks explanation of where to find the IDs.
-`https://portal.azure.com/#@{tenant-id}/resource/subscriptions/{subscription-id}/users` +`https://portal.azure.com/#@<TENANT_ID>/resource/subscriptions/<SUBSCRIPTION_ID>/users` + +You can find your Tenant ID and Subscription ID in the Azure Portal under Azure Active Directory > Properties.
73-74: Fix typo and clarify verification step.There's a typo in "assignmet" and the verification step could be clearer.
-Once the role assignmet is completed, return to the next steps page and click -`Permissions granted`. The setup process is complete. +Once the role assignment is completed, return to the next steps page and click +`Permissions granted`. Ctrlplane will verify the permissions and complete the setup process.
27-27: Enhance screenshot accessibility with descriptive alt text.The screenshots should have more descriptive alt text for better accessibility.
- + - + - + - + - +Also applies to: 55-55, 59-59, 63-63, 68-68
1-2: Add prerequisites section.Consider adding a prerequisites section at the beginning of the document to help users prepare.
# Azure Compute Scanner +## Prerequisites + +Before configuring the Azure Compute Scanner, ensure you have: +- An active Azure subscription +- Administrator access to your Azure Active Directory tenant +- Permissions to assign IAM roles in your subscription +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/docs/pages/integrations/azure/compute-scanner.mdx(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/drizzle/meta/_journal.json
🧰 Additional context used
🪛 LanguageTool
apps/docs/pages/integrations/azure/compute-scanner.mdx
[uncategorized] ~6-~6: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...they are created and deleted in Azure. Currently the Azure Compute Scanner supports impo...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~13-~13: Consider a shorter alternative to avoid wordiness.
Context: ...r is built into the Ctrlplane solution. In order to configure it, you must consent to our A...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~19-~19: Possible missing comma found.
Context: ...isit the Resource Provider Integrations page which can be found at `https://app.ctr...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~64-~64: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ations/azure/azure-select-members.png) In the select members page, search for `ct...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation