AWS managed compute scanner #236
Conversation
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (49)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/page.tsx (2)
1-4: Consider adding error boundary handlingSince this page deals with AWS integration, consider implementing an error boundary to gracefully handle potential integration-related errors that might occur during rendering.
import { notFound } from "next/navigation"; +import { ErrorBoundary } from "~/components/ErrorBoundary"; import { api } from "~/trpc/server"; import { AwsIntegration } from "./AwsIntegration";
6-6: Enhance page metadata for better SEOConsider adding more metadata properties for improved SEO and accessibility.
-export const metadata = { title: "AWS Integrations - Settings" }; +export const metadata = { + title: "AWS Integrations - Settings", + description: "Manage your AWS integration settings and configurations", + openGraph: { + title: "AWS Integrations - Settings", + description: "Manage your AWS integration settings and configurations", + }, +};apps/event-worker/src/index.ts (1)
16-17: Enhance shutdown procedure with graceful terminationThe current shutdown procedure might abruptly terminate ongoing scans. Consider implementing a graceful shutdown mechanism that:
- Stops accepting new scans
- Waits for in-progress scans to complete
- Implements timeouts
- Handles closure errors
Consider implementing a more robust shutdown:
-const shutdown = () => { +const shutdown = async () => { logger.warn("Exiting..."); + const SHUTDOWN_TIMEOUT = 5000; // 5 seconds + + try { + const shutdownPromises = [ + Promise.race([ + resourceAwsScanWorker.close(), + new Promise(resolve => setTimeout(resolve, SHUTDOWN_TIMEOUT)) + ]), + Promise.race([ + resourceGoogleScanWorker.close(), + new Promise(resolve => setTimeout(resolve, SHUTDOWN_TIMEOUT)) + ]), + Promise.race([ + dispatchExecutionJobWorker.close(), + new Promise(resolve => setTimeout(resolve, SHUTDOWN_TIMEOUT)) + ]) + ]; + + await Promise.all(shutdownPromises); + } catch (error) { + logger.error('Error during shutdown:', error); + } redis.quit(); process.exit(0); };apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx (3)
20-23: Consider enhancing error handling and loading states.While the basic hooks setup is good, consider these improvements:
- Add explicit error typing for the mutation
- Consider extracting loading state to show different loading indicators
+ type AwsRoleError = { + message: string; + code?: string; + }; const createAwsRole = - api.workspace.integrations.aws.createAwsRole.useMutation(); + api.workspace.integrations.aws.createAwsRole.useMutation<unknown, AwsRoleError>(); + const isLoading = createAwsRole.isPending;
24-31: Enhance null check and button semantics.Consider these improvements for better code quality:
- Use a more explicit null check
- Add semantic class names for the Configure button
- if (workspace.awsRoleArn != null) + if (workspace.awsRoleArn !== null && workspace.awsRoleArn !== undefined) return ( <AwsDialog workspace={workspace}> <Button variant="outline" size="sm" - className="w-full" + className="w-full configure-aws-button" > Configure </Button> </AwsDialog> );
1-52: Consider architectural improvements for AWS role management.Suggestions for better architecture:
- Move AWS role creation logic to a custom hook for reusability
- Add proper AWS role validation before creation
- Implement proper error boundaries for AWS-related errors
- Consider adding telemetry for AWS role creation success/failure rates
Would you like me to provide an example implementation of these architectural improvements?
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/GoogleActionButton.tsx (1)
Line range hint
33-56: Consider enhancing loading state feedbackWhile
createServiceAccount.isPendingis used to disable the button, the user experience could be improved with more visible loading feedback.Consider these improvements:
<Button variant="outline" size="sm" - className="w-full" + className="w-full relative" disabled={createServiceAccount.isPending} onClick={async () => { // ... existing code ... }} > + {createServiceAccount.isPending ? ( + <> + <span className="opacity-0">Enable</span> + <span className="absolute left-1/2 top-1/2 -translate-x-1/2 -translate-y-1/2"> + Creating... + </span> + </> + ) : ( Enable + )} </Button>This provides clearer visual feedback during the service account creation process.
apps/event-worker/package.json (2)
15-17: Consider aligning AWS SDK versionsThe AWS SDK client versions are slightly mismatched:
@aws-sdk/client-ec2: ^3.701.0@aws-sdk/client-eks: ^3.699.0@aws-sdk/client-sts: ^3.699.0While this might work, it's recommended to keep AWS SDK versions in sync to avoid potential compatibility issues.
- "@aws-sdk/client-ec2": "^3.701.0", - "@aws-sdk/client-eks": "^3.699.0", - "@aws-sdk/client-sts": "^3.699.0", + "@aws-sdk/client-ec2": "^3.701.0", + "@aws-sdk/client-eks": "^3.701.0", + "@aws-sdk/client-sts": "^3.701.0",
15-17: Consider pinning versions for better reproducibilityThe use of caret (^) in version numbers allows for minor version updates, which could potentially introduce breaking changes or inconsistencies. For better reproducibility and stability, consider pinning exact versions, especially for critical dependencies like AWS SDK.
- "@aws-sdk/client-ec2": "^3.701.0", - "@aws-sdk/client-eks": "^3.699.0", - "@aws-sdk/client-sts": "^3.699.0", - "@smithy/types": "^3.7.1", - "ts-is-present": "^1.2.2", + "@aws-sdk/client-ec2": "3.701.0", + "@aws-sdk/client-eks": "3.701.0", + "@aws-sdk/client-sts": "3.701.0", + "@smithy/types": "3.7.1", + "ts-is-present": "1.2.2",Also applies to: 26-26, 36-36
apps/event-worker/src/target-scan/kube.ts (1)
Line range hint
44-60: Add proper null checks to prevent runtime errorsThe function uses non-null assertions (
!.) on potentially undefined values, which could lead to runtime errors ifmetadataornameare undefined.Consider this safer implementation:
export const createNamespaceResource = ( clusterResource: InsertResource, namespace: Namespace, project: string, cluster: google.container.v1.ICluster, ) => { + if (!namespace.metadata?.name) { + throw new Error('Namespace metadata.name is required'); + } + + const clusterName = cluster.name ?? cluster.id ?? ""; + const namespaceName = namespace.metadata.name; + return _.merge(_.cloneDeep(clusterResource), { - name: `${cluster.name ?? cluster.id ?? ""}/${namespace.metadata!.name}`, + name: `${clusterName}/${namespaceName}`, kind: "Namespace", - identifier: `${project}/${cluster.name}/${namespace.metadata!.name}`, + identifier: `${project}/${cluster.name}/${namespaceName}`, config: { namespace: namespace.metadata!.name }, metadata: { [ReservedMetadataKey.ParentResourceIdentifier]: clusterResource.identifier, ...namespace.metadata?.labels, - "kubernetes/namespace": namespace.metadata!.name ?? "", + "kubernetes/namespace": namespaceName, }, }); };packages/api/package.json (1)
22-22: Consider consolidating AWS SDK dependenciesCurrently, AWS SDK clients are split between packages. Consider these architectural options:
- Create a dedicated
@ctrlplane/awspackage to centralize AWS-related functionality- Move all AWS SDK clients to the event-worker package if that's the primary consumer
This would:
- Improve dependency management
- Centralize AWS-related code
- Make version updates easier
- Reduce package sizes
Would you like me to propose a detailed structure for either approach?
apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts (1)
Line range hint
82-91: Enhance error handling for AWS-specific operationsThe current error handling might not adequately cover AWS-specific scenarios. Consider:
- Adding structured error responses for AWS service failures
- Implementing partial success handling for batch operations
- Including AWS error codes and messages in the response format
const targets = await upsertResources( db, targetsToInsert.map((t) => ({ ...t, variables: t.variables?.map((v) => ({ ...v, value: v.value ?? null, })), })), - ); - - return NextResponse.json({ targets }); + ).catch(error => { + if (error.name === 'AWSError') { + return NextResponse.json({ + error: 'AWS Operation Failed', + code: error.code, + message: error.message, + targets: error.partialSuccess + }, { status: 502 }); + } + throw error; + }); + + return NextResponse.json({ + success: true, + targets + });packages/db/src/schema/workspace.ts (1)
23-23: Add JSDoc comment to document the AWS Role ARN fieldAdd documentation to explain the purpose and format of this field, especially since it's used for AWS integration.
Add this documentation above the field:
+ /** + * AWS IAM Role ARN used for resource scanning. + * Format: arn:aws:iam::<account-id>:role/<role-name> + */ awsRoleArn: text("aws_role_arn"),apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/page.tsx (1)
65-75: Consider enhancing the AWS integration descriptionWhile the implementation is well-structured and consistent with other integration cards, consider making the description more specific to the compute scanner functionality mentioned in the PR title.
- Sync deployment resources, trigger AWS workflows and more. + Scan and monitor AWS compute resources, sync deployments, and trigger AWS workflows.packages/db/src/schema/resource-provider.ts (3)
78-85: Consider adding performance optimizations and consistency improvements.A few suggestions to enhance the table definition:
- Consider adding an index on
awsRoleArnsif you plan to query by ARN.- Add a
createdAttimestamp for consistency with the parent table.- Consider adding a maximum limit to the
awsRoleArnsarray size to prevent performance issues.export const resourceProviderAws = pgTable("resource_provider_aws", { id: uuid("id").primaryKey().defaultRandom(), resourceProviderId: uuid("resource_provider_id") .notNull() .references(() => resourceProvider.id, { onDelete: "cascade" }), awsRoleArns: text("aws_role_arns").array().notNull(), + createdAt: timestamp("created_at").defaultNow().notNull(), });
90-101: Strengthen the AWS Role ARN validation regex.While the current regex is good, it could be more precise to match AWS's actual role name constraints:
- Role names can't have consecutive special characters
- Role paths must start and end with
/- Role names can't begin with special characters
.regex( - /^arn:aws:iam::[0-9]{12}:role\/[a-zA-Z0-9+=,.@\-_/]+$/, + /^arn:aws:iam::[0-9]{12}:role(\/[a-zA-Z0-9][a-zA-Z0-9+=,.@\-_]*)+$/, "Invalid AWS Role ARN format. Expected format: arn:aws:iam::<account-id>:role/<role-name>", )
Line range hint
19-26: Add AWS provider to resourceProviderRelations.The AWS provider needs to be added to the main resourceProviderRelations to enable proper querying through the resource provider entity.
export const resourceProviderRelations = relations( resourceProvider, ({ many }) => ({ resources: many(resource), google: many(resourceProviderGoogle), + aws: many(resourceProviderAws), }), );apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/google/GoogleIntegration.tsx (1)
Line range hint
64-88: Consider adding loading state UI feedback.While the buttons are correctly disabled during operations (using
isPending), consider adding a loading spinner or progress indicator to provide better visual feedback during these async operations. This would enhance the user experience, especially for slower network conditions.apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/AwsIntegration.tsx (2)
16-18: Consider strengthening prop type safety.While the current typing is functional, consider creating a more specific interface that explicitly defines the required properties you're using from the Workspace type (like
idandawsRoleArn).+interface AwsIntegrationProps { + workspace: Pick<Workspace, 'id' | 'awsRoleArn'>; +} -export const AwsIntegration: React.FC<{ - workspace: Workspace; -}> = ({ workspace }) => { +export const AwsIntegration: React.FC<AwsIntegrationProps> = ({ workspace }) => {
96-108: Improve role ARN display and copy functionality.The role ARN display could benefit from better overflow handling and copy feedback.
-<div className="flex items-center justify-between p-4 text-sm text-neutral-200"> +<div className="flex items-center justify-between p-4 text-sm text-neutral-200 gap-4"> <div className="flex items-center gap-2"> - <span className="truncate font-mono text-xs"> + <span className="max-w-[300px] truncate font-mono text-xs" title={workspace.awsRoleArn}> {workspace.awsRoleArn} </span> - <Button variant="ghost" size="sm" onClick={handleCopy}> + <Button + variant="ghost" + size="sm" + onClick={handleCopy} + title={isCopied ? "Copied!" : "Copy to clipboard"} + >apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/ProviderActionsDropdown.tsx (1)
83-94: Consider adding error and loading statesWhile the AWS provider dialog integration is well-implemented, consider enhancing the user experience by:
- Adding loading indicators during AWS operations
- Implementing error handling for failed AWS operations
- Providing feedback for successful operations
Example implementation:
{provider.awsConfig != null && ( <UpdateAwsProviderDialog providerId={provider.id} name={provider.name} awsConfig={provider.awsConfig} onClose={() => setOpen(false)} + onError={(error) => { + // Show error toast/notification + }} + onSuccess={() => { + // Show success toast/notification + }} > <DropdownMenuItem + disabled={isLoading} onSelect={(e) => e.preventDefault()} > - Edit + {isLoading ? 'Updating...' : 'Edit'} </DropdownMenuItem> </UpdateAwsProviderDialog> )}apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/page.tsx (1)
Line range hint
1-214: Consider extracting provider cards into separate componentsThe file is well-structured, but as we add more providers (AWS, Google, and potentially Azure), consider:
- Extracting provider-specific cards into separate components
- Creating a common interface for provider configurations
- Moving provider metadata (icons, descriptions, badges) to a configuration file
This would improve maintainability and make it easier to add new providers in the future.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/overview/page.tsx (2)
157-157: Fix grammatical error in Google Cloud description.The word "resource" should be plural to maintain consistency with other integration descriptions.
- Sync deployment resource, trigger google workflows and more. + Sync deployment resources, trigger google workflows and more.
Line range hint
1-236: Consider adding feature flags for the AWS integration.Since this is a significant new integration, consider implementing feature flags to control its rollout. This would allow for:
- Gradual rollout to users
- A/B testing
- Quick rollback if issues arise
- Beta testing with select customers
github/get-job-inputs/index.js (1)
28139-28140: Consider handling JSON.stringify errorsWhile the null/undefined check is good, JSON.stringify can throw errors for objects with circular references or other invalid JSON structures.
Consider wrapping the stringification in a try-catch:
if (value === undefined || value === null) return; - const stringValue = typeof value === "string" ? value : JSON.stringify(value); + let stringValue; + try { + stringValue = typeof value === "string" ? value : JSON.stringify(value); + } catch (error) { + core.warning(`Failed to stringify value for ${key}: ${error.message}`); + return; + }apps/event-worker/src/target-scan/aws.ts (6)
7-7: Consider making the STS client region configurableCurrently,
sourceClientis initialized with a hardcoded region"us-east-1". To enhance flexibility and support multiple regions, it would be better to make the region configurable.
17-21: Add braces aroundifstatement for clarity and maintainabilityTo improve readability and prevent potential errors during future modifications, it's recommended to add curly braces
{}around theifstatement block.You can modify the code as follows:
if ( this.credentials.AccessKeyId == null || this.credentials.SecretAccessKey == null ) { throw new Error("Missing required AWS credentials"); }
38-40: Consider adding a region parameter defaultIn the
stsmethod, consider providing a default region or inheriting the region from a configuration to avoid potential issues if no region is specified.
43-44: MakeassumeWorkspaceRoleregion-awareSince
assumeWorkspaceRolerelies onsourceClient, which uses a hardcoded region, consider modifying the function to accept a region parameter or use a dynamically configured region to support multiple AWS regions.
56-57: Add braces aroundifstatement for consistency and safetyIncluding curly braces
{}around theifblock enhances code readability and maintains consistency throughout the codebase.You can update the code as:
if (CustomerCredentials == null) { throw new Error(`Failed to assume AWS role ${roleArn}`); }
52-53: Consider makingRoleSessionNameconfigurableCurrently,
RoleSessionNameis hardcoded as"CtrlplaneScanner". To allow for more flexibility and avoid potential naming conflicts, consider making it a configurable parameter.You can modify the code as follows:
new AssumeRoleCommand({ RoleArn: roleArn, - RoleSessionName: "CtrlplaneScanner", + RoleSessionName: roleSessionName, }),Then update the function parameters to include
roleSessionName.export const assumeRole = async ( client: STSClient, roleArn: string, + roleSessionName: string, ): Promise<AwsCredentials> => {apps/event-worker/src/target-scan/index.ts (3)
67-76: Handle empty resource arrays appropriately after scanning.If
scanResourcesreturns an empty array, the current logic logs that no resources were found. Consider if additional handling is required, such as triggering alerts or checking for possible issues in the scanning process.
95-115: Consider error propagation increateGoogleResourceScanWorker.In
createGoogleResourceScanWorker, errors during GKE scanning are caught and logged but not re-thrown. If you intend for the job to retry on failure, re-throw the error after logging. If failures are acceptable, ensure this is the desired behavior.Example adjustment:
catch (error: any) { log.error(`Error scanning GKE resources: ${error.message}`, { error, }); + throw error; }
117-137: Ensure consistent error handling increateAwsResourceScanWorker.Similar to the Google worker, consider whether errors in
createAwsResourceScanWorkershould cause the job to fail and retry. Re-throw the error after logging if retries are desired.Example adjustment:
catch (error: any) { log.error(`Error scanning EKS resources: ${error.message}`, { error, }); + throw error; }apps/event-worker/src/target-scan/eks.ts (1)
63-64: Prefixcluster.tagskeys in metadata to avoid key collisions.Directly spreading
cluster.tagsintometadatamay cause key collisions with existing metadata keys. Consider prefixing the tags to ensure uniqueness.Apply this diff to prefix tag keys:
return { ... metadata: omitNullUndefined({ ... - ...(cluster.tags ?? {}), + ...Object.fromEntries( + Object.entries(cluster.tags ?? {}).map(([key, value]) => [`aws/tag/${key}`, value]), + ), }), };apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/AwsDialog.tsx (6)
74-76: Extract magic number into a constant for maintainabilityThe hardcoded duration of
1000milliseconds in thesetTimeoutfunction can be extracted into a named constant for better readability and easier future modifications.Apply this diff to extract the magic number:
+ const COPY_NOTIFICATION_DURATION = 1000; setTimeout(() => { setIsCopied(false); - }, 1000); + }, COPY_NOTIFICATION_DURATION);
178-179: Enhance placeholder text for clarityThe placeholder for the AWS Role ARN input field could be more descriptive to guide users effectively. Consider providing an example or more detailed placeholder text.
Apply this diff to update the placeholder:
<Input - placeholder="arn:aws:iam::<account-id>:role/<role-name>" + placeholder="e.g., arn:aws:iam::123456789012:role/MyRole" {...field} />
152-162: Add validation rules for the provider nameCurrently, the provider name field only checks for a non-empty string. Consider adding validation rules such as minimum length, maximum length, and character restrictions to prevent invalid input.
Possible update to the schema:
export const createAwsSchema = z.object({ name: z.string() + .min(3, "Provider name must be at least 3 characters long") + .max(50, "Provider name must be at most 50 characters long") + .regex(/^[a-zA-Z0-9_\- ]+$/, "Provider name contains invalid characters"), awsRoleArns: z.array( // Existing code... ), });
45-47: Review AWS Role ARN validation regex for completenessThe regex pattern for validating AWS Role ARNs may not cover all valid scenarios, such as roles in different partitions (
aws-us-gov,aws-cn) or with more complex role names. Consider updating the regex to be more inclusive or utilizing AWS SDKs for validation.An updated regex could be:
- /^arn:aws:iam::[0-9]{12}:role\/[a-zA-Z0-9+=,.@\-_/]+$/ + /^arn:(aws|aws-us-gov|aws-cn):iam::[0-9]{12}:role\/.+$/
95-203: Enhance accessibility by adding ARIA attributesTo improve accessibility, consider adding appropriate ARIA labels and roles to interactive elements like buttons and inputs. This will enhance the user experience for users relying on assistive technologies.
For example, add
aria-labelto the copy button:<Button variant="ghost" size="icon" type="button" onClick={handleCopy} + aria-label="Copy AWS Role ARN" className="absolute right-2 h-4 w-4 bg-neutral-950 backdrop-blur-sm transition-all hover:bg-neutral-950 focus-visible:ring-0" > {/* Existing code... */} </Button>
52-205: Consider adding unit tests for theAwsDialogcomponentTo ensure reliability and prevent future regressions, consider adding unit tests for the
AwsDialogcomponent, covering user interactions, form validation, and submission logic.Would you like assistance in outlining the unit tests for this component?
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/UpdateAwsProviderDialog.tsx (3)
236-237: Ensure 'repeatSeconds' handles NaN values correctlyUsing
e.target.valueAsNumbercan result inNaNif the input is empty or invalid. Update theonChangehandler to handle such cases:<Input type="number" min={0} {...field} - onChange={(e) => field.onChange(e.target.valueAsNumber)} + onChange={(e) => { + const value = e.target.valueAsNumber; + field.onChange(isNaN(value) ? 0 : value); + }} />
153-153: Maintain focus outlines for better keyboard navigationThe
Buttonsuppresses focus outlines withfocus-visible:ring-0. To enhance accessibility, keep the default focus styles:<Button variant="ghost" size="icon" type="button" onClick={handleCopy} - className="absolute right-2 h-4 w-4 bg-neutral-950 backdrop-blur-sm transition-all hover:bg-neutral-950 focus-visible:ring-0" + className="absolute right-2 h-4 w-4 bg-neutral-950 backdrop-blur-sm transition-all hover:bg-neutral-950" >
52-52: Remove unnecessary generic type from 'useParams'The
useParamshook doesn't accept generic parameters in Next.js. Remove the generic to prevent potential issues:- const { workspaceSlug } = useParams<{ workspaceSlug: string }>(); + const { workspaceSlug } = useParams();packages/api/src/router/workspace-integrations.ts (4)
171-172: Make AWS Region Configurable Instead of Hard-CodedThe AWS clients are instantiated with the region set to
"us-east-1". Hard-coding the region reduces flexibility and may lead to issues if resources are located in different regions. Consider making the AWS region configurable via environment variables or configuration settings.Apply this change to make the region configurable:
const iamClient = new IAMClient({ - region: "us-east-1", + region: process.env.AWS_REGION || "us-east-1", credentials: defaultProvider(), }); const stsClient = new STSClient({ - region: "us-east-1", + region: process.env.AWS_REGION || "us-east-1", credentials: defaultProvider(), });Ensure that the
AWS_REGIONenvironment variable is set appropriately in your deployment configuration.Also applies to: 175-176
150-254: Validate Input and Ensure Consistent Error MessagesThroughout the
createAwsRoleprocedure, ensure that all user inputs are validated properly and that error messages are consistent and informative.Consider adding Zod schemas for input validation and standardize error messages to improve API usability and debugging experience.
257-312: Ensure Consistency in Authorization Checks and Error HandlingIn the
deleteAwsRoleprocedure, verify that authorization checks and error handling are consistent with thecreateAwsRoleprocedure.Ensure that any updates made to
createAwsRolefor error handling and input validation are also reflected indeleteAwsRoleto maintain consistency.
1-8: Organize Imports for ClarityConsider grouping and ordering imports logically: external libraries first, then internal modules. This improves readability and maintainability of the code.
You can reorganize the imports as follows:
import { TRPCError } from "@trpc/server"; import { z } from "zod"; import { defaultProvider } from "@aws-sdk/credential-provider-node"; import { CreateRoleCommand, DeleteRoleCommand, DeleteRolePolicyCommand, IAMClient, PutRolePolicyCommand, } from "@aws-sdk/client-iam"; import { GetCallerIdentityCommand, STSClient } from "@aws-sdk/client-sts"; import { auth } from "google-auth-library"; import { google } from "googleapis"; // Internal modules import { eq, takeFirst, takeFirstOrNull } from "@ctrlplane/db"; import { resourceProvider, resourceProviderGoogle, workspace, } from "@ctrlplane/db/schema"; import { Permission } from "@ctrlplane/validators/auth"; import { createTRPCRouter, protectedProcedure } from "../trpc";packages/api/src/router/target-provider.ts (1)
231-333: Refactor Suggestion: Reduce Code Duplication in AWS and Google RoutersThe
createandupdateprocedures within both theawsandConsider abstracting common functionality into shared utility functions or higher-order procedures. For example:
- Create a generic function for resource provider creation that accepts provider-specific configurations.
- Abstract shared transaction logic into a reusable function.
This refactoring will enhance code maintainability and reduce the risk of errors in similar code blocks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
apps/event-worker/package.json(3 hunks)apps/event-worker/src/index.ts(1 hunks)apps/event-worker/src/target-scan/aws.ts(1 hunks)apps/event-worker/src/target-scan/eks.ts(1 hunks)apps/event-worker/src/target-scan/index.ts(4 hunks)apps/event-worker/src/target-scan/kube.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/ProviderActionsDropdown.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/GoogleActionButton.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/AwsDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/UpdateAwsProviderDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/page.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/AwsIntegration.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/google/GoogleIntegration.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/overview/page.tsx(3 hunks)apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts(1 hunks)docker-compose.dev.yaml(1 hunks)github/get-job-inputs/index.js(2 hunks)packages/api/package.json(1 hunks)packages/api/src/router/target-provider.ts(4 hunks)packages/api/src/router/workspace-integrations.ts(2 hunks)packages/db/src/schema/resource-provider.ts(1 hunks)packages/db/src/schema/workspace.ts(1 hunks)
🔥 Files not summarized due to errors (1)
- github/get-job-inputs/index.js: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (1)
- docker-compose.dev.yaml
🔇 Additional comments (37)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/page.tsx (1)
4-4: Verify AwsIntegration component implementation
Let's ensure the AwsIntegration component exists and implements all necessary AWS integration features.
Also applies to: 15-15
✅ Verification successful
AwsIntegration component implementation verified successfully
The AwsIntegration component is properly implemented with:
- Correct type definition and workspace prop handling
- AWS role management functionality (create/delete)
- UI elements for role ARN display and management
- Proper error handling and user feedback
- Integration status indicators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if AwsIntegration component exists and verify its implementation
ast-grep --pattern 'export function AwsIntegration({ workspace }: { workspace: $_ }) {
$$$
}'
# Check for AWS SDK dependencies
cat package.json | jq '.dependencies | keys[] | select(contains("aws"))'
Length of output: 223
Script:
#!/bin/bash
# Let's first check if the AwsIntegration file exists and examine its content
fd "AwsIntegration.tsx" --type f
# Let's also try a broader search for AWS-related components
rg -l "AwsIntegration" --type tsx
# Check for AWS-related code patterns
rg -A 5 "aws" --type tsx
Length of output: 256
Script:
#!/bin/bash
# Let's examine the content of the AwsIntegration component
cat "apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/AwsIntegration.tsx"
# Also check the parent page component that imports it
cat "apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/page.tsx"
Length of output: 4785
apps/event-worker/src/index.ts (3)
5-8: LGTM! Clean separation of cloud provider workers
The separation of resource scanning workers by cloud provider improves modularity and maintainability.
Line range hint 1-24: Verify integration with the event processing system
The separation into AWS and Google workers is architecturally sound. However, we should verify:
- Event routing logic for different cloud providers
- Consistent error handling across providers
- Metrics and monitoring parity
Let's check the event routing implementation:
✅ Verification successful
Let me gather more information about the event routing implementation since the first search didn't yield clear results about provider-specific routing.
Let me check the resource scan worker implementation to understand the complete flow.
Based on the gathered information, I can now provide the final response:
Integration verification completed - implementation is sound
The codebase shows a well-structured separation of concerns with proper integration:
- Event routing is handled through a common
createResourceScanWorkerfactory that processesResourceScanEvents from theChannel.ResourceScanqueue - Provider-specific logic is cleanly separated into dedicated workers that handle their respective cloud providers (AWS/Google)
- Error handling is consistent across providers with proper logging and resource cleanup
- Each provider implementation includes appropriate validation and error recovery
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify event routing and handler implementation
# Check for event routing logic
echo "Checking event routing patterns..."
ast-grep --pattern 'function $_(event) {
$$$
if ($event.provider === "aws") {
$$$
}
$$$
}'
# Check for consistent error handling
echo "Checking error handling patterns..."
rg -A 5 'catch|throw|error' ./apps/event-worker/src/target-scan/
Length of output: 8350
Script:
#!/bin/bash
# Check worker implementations and queue handling
echo "Checking worker implementations..."
rg -A 10 "createAwsResourceScanWorker|createGoogleResourceScanWorker" ./apps/event-worker/src/
# Check queue/job processing logic
echo "Checking queue processing..."
rg -A 10 "process.*Job|handle.*Job" ./apps/event-worker/src/target-scan/
Length of output: 3091
Script:
#!/bin/bash
# Check the base resource scan worker implementation
echo "Checking resource scan worker implementation..."
rg -A 15 "createResourceScanWorker" ./apps/event-worker/src/
# Check resource provider types
echo "Checking resource provider types..."
fd -e ts -e js --exec cat {} \; -p "types" ./apps/event-worker/src/
Length of output: 4078
10-11: 🛠️ Refactor suggestion
Consider adding error handling for worker initialization
The worker initialization could fail if AWS/Google credentials are missing or misconfigured. Consider wrapping the initialization in try-catch blocks and implementing proper error handling.
Let's verify the worker configurations and required environment variables:
Consider implementing error handling:
-const resourceGoogleScanWorker = createGoogleResourceScanWorker();
-const resourceAwsScanWorker = createAwsResourceScanWorker();
+let resourceGoogleScanWorker;
+let resourceAwsScanWorker;
+try {
+ resourceGoogleScanWorker = createGoogleResourceScanWorker();
+ resourceAwsScanWorker = createAwsResourceScanWorker();
+} catch (error) {
+ logger.error('Failed to initialize resource scan workers:', error);
+ process.exit(1);
+}apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx (2)
1-15: LGTM! Clean imports and type definitions.
The imports are well-organized and the type definition is properly structured.
17-19: LGTM! Component declaration follows React best practices.
The component is properly typed with React.FC and uses clean props destructuring.
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/GoogleActionButton.tsx (1)
8-8: LGTM!
The toast import is correctly placed and follows the project's module structure.
apps/event-worker/package.json (1)
15-17: Verify AWS SDK compatibility
Let's ensure the AWS SDK versions are compatible with each other and check for any known issues.
Also applies to: 26-26
✅ Verification successful
AWS SDK versions are compatible and properly used
Based on the codebase analysis, the AWS SDK clients (@aws-sdk/client-ec2, @aws-sdk/client-eks, and @aws-sdk/client-sts) are being used together correctly in the event-worker application, particularly in the target-scan functionality. The minor version differences (3.701.0 vs 3.699.0) are within the same major version (v3) and are compatible with each other. The code shows proper integration of these clients, with consistent import patterns and usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for compatibility issues between AWS SDK packages
# Get the latest compatible versions and any known issues
echo "Checking AWS SDK package compatibility..."
npm info '@aws-sdk/client-ec2@^3.701.0' peerDependencies
npm info '@aws-sdk/client-eks@^3.699.0' peerDependencies
npm info '@aws-sdk/client-sts@^3.699.0' peerDependencies
npm info '@smithy/types@^3.7.1' peerDependencies
# Check for any security advisories
echo "Checking for security advisories..."
npm audit '@aws-sdk/client-ec2@^3.701.0' '@aws-sdk/client-eks@^3.699.0' '@aws-sdk/client-sts@^3.699.0' '@smithy/types@^3.7.1'
Length of output: 878
Script:
#!/bin/bash
# Let's check the actual usage of these AWS SDK clients in the codebase
# to verify they are used together correctly
# Search for imports and usage of these AWS SDK clients
echo "Checking AWS SDK client imports and usage..."
rg -A 5 "@aws-sdk/client-(ec2|eks|sts)" --type ts --type js
# Check package.json for any other AWS SDK related dependencies
echo "Checking other AWS SDK dependencies..."
fd package.json --exec grep -A 10 -B 10 "@aws-sdk" {}
Length of output: 4465
apps/event-worker/src/target-scan/kube.ts (1)
Line range hint 1-60: Verify AWS namespace handling consistency
Since this PR introduces AWS managed compute scanner functionality, we should ensure consistent namespace handling between Google Cloud and AWS implementations.
packages/api/package.json (1)
22-22: Verify AWS SDK version for security vulnerabilities
The added AWS IAM client handles sensitive authentication and authorization. Let's verify this version for known vulnerabilities.
✅ Verification successful
Version 3.696.0 of @aws-sdk/client-iam appears secure, but minor update available
The verification shows:
- Latest version is 3.699.0 (minor version update available)
- No security vulnerabilities found in GitHub Security Advisory Database
- The caret (^) in version specification already allows for safe minor updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions
# of @aws-sdk/client-iam
# Check npm for latest versions and security advisories
npm view @aws-sdk/client-iam versions --json | jq '.[-1]'
npm audit @aws-sdk/client-iam
# Check for GitHub security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@aws-sdk/client-iam") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 803
apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts (1)
Line range hint 21-33: Enhance validation for AWS-specific sensitive data
Given this endpoint will handle AWS resource providers, consider adding specific validation to ensure AWS-related sensitive data (like credentials, access keys, etc.) are properly marked with sensitive: true in the variables array.
Consider implementing an AWS-specific schema validator that enforces security best practices for AWS resource configurations.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/page.tsx (3)
2-6: LGTM! Icon imports are properly organized
The addition of the SiAmazon icon import is well-structured and consistent with the existing icon import pattern.
84-84: LGTM! Consistent terminology across integrations
The text modification maintains consistency by using "deployment resources" terminology across all integration descriptions.
65-75: Verify AWS integration route implementation
The integration card links to /[workspaceSlug]/settings/workspace/integrations/aws. Let's verify this route exists:
✅ Verification successful
AWS integration route is properly implemented
The verification confirms that:
- The AWS integration route exists at
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/AwsIntegration.tsx - The
IntegrationCardcomponent correctly links to this route via/${workspaceSlug}/settings/workspace/integrations/${integration} - The AWS integration page is fully implemented with role management functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the AWS integration route page exists
fd -e tsx -e ts "aws.tsx" "apps/webservice/src/app"
Length of output: 49
Script:
#!/bin/bash
# Let's check for any aws-related routes or pages in the app directory
fd "aws" "apps/webservice/src/app"
# Also check for potential route handlers or pages with different naming patterns
fd "(aws|amazon)" "apps/webservice/src/app/\[workspaceSlug\]/settings/workspace/integrations"
# Let's also check the IntegrationCard component to see how routing is handled
rg -A 5 "IntegrationCard" "apps/webservice/src/app"
Length of output: 4922
Script:
#!/bin/bash
# Let's check the implementation of IntegrationCard to see how routing is handled
rg -B 5 -A 15 "const IntegrationCard" "apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/page.tsx"
# Also check if there's an AWS integration implementation
cat "apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/AwsIntegration.tsx"
Length of output: 4960
packages/db/src/schema/resource-provider.ts (1)
108-116: LGTM! Relations are well-defined.
The relations implementation:
- Correctly maps the foreign key relationship
- Follows the established pattern in the codebase
- Maintains consistency with other provider implementations
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/google/GoogleIntegration.tsx (2)
12-12: LGTM!
The toast import is correctly placed and necessary for the new notification functionality.
82-88: Apply the same error handling improvements here.
The same improvements suggested for the deletion handler should be applied here for consistency.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/AwsIntegration.tsx (2)
1-15: LGTM! Well-organized imports.
The imports are well-structured and appropriately separated between external dependencies and internal components.
19-22: Verify API endpoint access control.
Ensure that the TRPC mutation endpoints for AWS role management have proper access control and validation in place.
✅ Verification successful
API endpoints for AWS role management have proper access controls in place
The verification shows that both createAwsRole and deleteAwsRole endpoints are properly secured:
- Both endpoints are defined as
protectedProcedurewhich ensures authentication - Both have explicit authorization checks requiring
Permission.WorkspaceUpdatepermission - Both validate the workspace scope through
.on({ type: "workspace", id: input }) - Both include proper input validation using zod schema (
z.string().uuid())
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for authentication and authorization checks in the API routes
# Look for router definitions and middleware
rg -A 10 "workspace\.integrations\.aws" --type typescript
# Check for authentication middleware
rg -A 5 "protectedProcedure" --type typescript
Length of output: 176
Script:
#!/bin/bash
# Search for AWS integration router definitions and middleware
rg -A 10 "workspace\.integrations\.aws"
# Check for authentication middleware and protected procedures
rg -A 5 "protectedProcedure"
# Look for AWS role management endpoints
rg -A 10 "createAwsRole|deleteAwsRole"
Length of output: 91300
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/ProviderActionsDropdown.tsx (3)
5-5: LGTM: Type system changes for AWS integration
The addition of ResourceProviderAws type and extending the Provider type with awsConfig maintains type safety and follows the existing pattern used for Google provider integration.
Also applies to: 37-37
45-46: LGTM: Provider management logic
The isManagedProvider check correctly handles both AWS and Google providers with a clear, maintainable condition.
32-32: Verify UpdateAwsProviderDialog implementation
Let's ensure the UpdateAwsProviderDialog component is properly implemented and consistently used.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the implementation and usage of UpdateAwsProviderDialog.
UpdateAwsProviderDialog implementation is properly integrated
The component is correctly implemented as a React functional component with proper typing and is being used appropriately in ProviderActionsDropdown. The implementation shows:
- Clear interface definition with required props (providerId, name, awsConfig, onClose, children)
- Proper usage in ProviderActionsDropdown with all required props being passed
- Consistent integration within the AWS provider management flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check UpdateAwsProviderDialog implementation and usage
echo "Checking UpdateAwsProviderDialog implementation..."
ast-grep --pattern 'export const UpdateAwsProviderDialog = $_'
echo "\nChecking for all UpdateAwsProviderDialog usages..."
rg "UpdateAwsProviderDialog" -A 5
Length of output: 3745
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/page.tsx (1)
80-100: LGTM! The provider type detection logic is correctly updated.
The condition for displaying the "Custom" provider badge has been appropriately extended to account for AWS providers, ensuring the badge only appears for truly custom providers that aren't managed by either Google or AWS.
Let's verify the consistency of provider type detection across the codebase:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information. The results show that provider type detection is consistently implemented across the codebase:
Provider type detection is consistently implemented
The codebase correctly handles both AWS and Google provider configurations:
- In
page.tsx: Custom provider badge shows when both configs are null (googleConfig == null && awsConfig == null) - In
ProviderActionsDropdown.tsx:- Managed provider check uses both configs (
googleConfig != null || awsConfig != null) - Provider-specific dialogs are conditionally rendered based on their respective configs
- Managed provider check uses both configs (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent provider type detection patterns
# Expected: All provider type checks should consider both googleConfig and awsConfig
# Search for provider type checks
rg -A 2 'provider\.(google|aws)Config'
# Search for custom provider detection patterns
ast-grep --pattern 'provider.$_Config == null'
Length of output: 2701
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/page.tsx (3)
5-5: LGTM: Import statements are properly organized
The new imports for AWS integration are correctly placed and necessary for the feature implementation.
Also applies to: 17-17
Line range hint 121-149: Verify tracking of Azure integration plans
The removal of commented Azure provider code improves maintainability. However, let's verify if this is tracked as a future feature:
✅ Verification successful
Azure integration is still actively maintained in the codebase
The commented-out code removal is appropriate as the Azure integration functionality is properly maintained elsewhere in the codebase:
- Azure support is defined in Kubernetes validators (
packages/validators/src/resources/kubernetes-v1.ts) - Azure provider UI components are available in the target providers selection (
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/add/TargetProviderSelectCard.tsx) - Azure is documented as a supported cloud platform in the core concepts documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Azure integration tracking in issues or documentation
gh issue list --search "Azure integration"
fd README.md | xargs rg -l "Azure"
Length of output: 79
Script:
#!/bin/bash
# Search for Azure-related code and documentation more broadly
rg -i "azure" -g "*.{ts,tsx,md,mdx}" -C 2
# Search for any configuration or feature flags related to Azure
rg -i "azure.*provider" -g "*.{ts,tsx,json,yaml,yml}" -C 2
# Look for any Azure-related types or interfaces
ast-grep --pattern 'interface $_ {
$$$
azure
$$$
}'
Length of output: 5020
Line range hint 84-101: Consider enhancing the AWS provider description and type safety
A few suggestions to improve the AWS integration card:
- The description could be more specific about what AWS permissions are needed (e.g., IAM roles, specific service permissions)
- The K8sBadge usage in the AWS context might need clarification - is this indicating AWS EKS support?
Also, let's verify the type safety of the workspace prop:
✅ Verification successful
Based on the search results, I can now provide a complete verification of the review comment:
The AWS integration implementation has proper type safety and clear context for K8sBadge
The code shows that:
- The workspace prop has proper type safety through
AwsActionButtonPropsinterface that requires aWorkspacetype - The K8sBadge usage is appropriate as it indicates AWS EKS (Kubernetes) support, which is confirmed by:
- AWS provider's ability to discover and manage EKS clusters
- Presence of EKS-related code and types in the codebase
- The badge being used consistently across cloud provider cards
The description text could be more specific about AWS permissions, but this is a minor suggestion as the actual implementation properly handles AWS IAM roles and permissions for EKS cluster access.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if AwsActionButton has proper TypeScript interface defined
ast-grep --pattern 'interface $interface {
workspace: $_
}'
# Check for workspace type usage
rg -A 2 'workspace.*:.*{' ./apps/webservice/src/app
Length of output: 211
Script:
#!/bin/bash
# Check for AwsActionButton component definition and props
ast-grep --pattern 'const AwsActionButton = ({ workspace }: { workspace: $_ })'
# Alternative pattern to find workspace prop type
ast-grep --pattern 'type $_ = {
workspace: $_
}'
# Search for AwsActionButton implementation
rg "AwsActionButton" -A 5
# Search for K8sBadge usage context
rg "K8sBadge" -A 5
Length of output: 6848
Script:
#!/bin/bash
# Check AWS role creation implementation for permission details
rg -A 10 "createAwsRole"
# Check for any AWS permission documentation
fd README.md | xargs rg -i "aws.*permission"
# Look for EKS related code to understand K8sBadge context
rg -i "eks" -A 5
Length of output: 108277
apps/webservice/src/app/[workspaceSlug]/settings/workspace/overview/page.tsx (2)
3-3: LGTM: Icon import aligns with AWS integration.
The addition of the AWS icon import is consistent with the new AWS integration section.
173-181: LGTM: AWS integration section is well-implemented.
The AWS integration section follows the established patterns and maintains consistency with other integrations.
Let's verify that the AWS settings page exists:
✅ Verification successful
AWS integration section and its corresponding settings page are properly implemented
The AWS settings page exists at apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/page.tsx along with its component AwsIntegration.tsx. The implementation is complete and follows the expected routing pattern referenced in the link.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the AWS settings page component exists
fd -e tsx -e ts "aws" "apps/webservice/src/app/\[workspaceSlug\]/settings/workspace/integrations"
Length of output: 258
Script:
#!/bin/bash
# Let's try a broader search for AWS-related files in the workspace settings
fd -e tsx -e ts "aws" "apps/webservice/src"
# Also search for any file containing AWS-related content
rg -i "aws.*integration" "apps/webservice/src"
Length of output: 1476
apps/event-worker/src/target-scan/aws.ts (1)
30-40: AwsCredentials class implementation looks good
The AwsCredentials class is well-designed, providing a clear and efficient way to manage AWS credentials and create service clients for EC2, EKS, and STS.
apps/event-worker/src/target-scan/index.ts (3)
10-10: Ensure resourceProviderAws is correctly imported and used.
The addition of resourceProviderAws to the imports is necessary for AWS resource scanning. Verify that the module @ctrlplane/db/schema exports resourceProviderAws and that it's correctly defined.
Run the following script to confirm resourceProviderAws is exported:
✅ Verification successful
resourceProviderAws is correctly exported and used
The verification confirms that resourceProviderAws is properly exported from @ctrlplane/db/schema. It is defined in packages/db/src/schema/resource-provider.ts as a table schema and is correctly used across the codebase, including in the target file apps/event-worker/src/target-scan/index.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the export of resourceProviderAws in the codebase.
ast-grep --pattern $'export const resourceProviderAws = $_' --extensions ts
Length of output: 274
Script:
#!/bin/bash
# Search for resourceProviderAws in the codebase
rg "resourceProviderAws" -A 2 -B 2
# Search specifically in schema files
fd "schema" --type f --exec rg "resourceProviderAws" {} \;
Length of output: 4301
50-53: Validate the join conditions for resourceProviderAws.
Ensure that the left join with resourceProviderAws correctly associates AWS resource providers with their configurations. Incorrect joins could lead to null or incorrect data during scanning.
Run the following script to verify the join conditions:
✅ Verification successful
Join conditions for resourceProviderAws are correctly implemented
The join conditions are properly implemented based on the schema definitions found in the codebase:
- The
resourceProviderAwstable has a foreign keyresourceProviderIdthat references the primary keyidof theresourceProvidertable with cascade delete enabled - The join condition
eq(resourceProvider.id, resourceProviderAws.resourceProviderId)correctly matches these columns - The relationship is also formally defined in
resourceProviderAwsRelationswith the same field mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that resourceProviderAws joins are correctly implemented.
ast-grep --pattern $'leftJoin(\n resourceProviderAws,\n eq(resourceProvider.id, resourceProviderAws.resourceProviderId),\n)' --extensions ts
Length of output: 338
Script:
#!/bin/bash
# Let's first check the schema definitions and table relationships
rg -A 5 "resourceProviderAws" --type ts
# Also check for any other joins with resourceProviderAws
rg "leftJoin.*resourceProviderAws" --type ts
# Check the table/schema definitions
rg "createTable.*resourceProviderAws" --type ts
Length of output: 5050
19-19: Verify the existence and correctness of getEksResources function.
The import of getEksResources from ./eks.js is crucial for scanning AWS EKS resources. Ensure that the getEksResources function is properly implemented and handles errors appropriately.
Run the following script to check the implementation of getEksResources:
✅ Verification successful
getEksResources function is properly implemented with error handling
The function is correctly implemented in apps/event-worker/src/target-scan/eks.ts with:
- Proper type checking and early return if workspace role is missing
- Comprehensive error handling through Promise chains
- Appropriate logging for monitoring and debugging
- Well-structured AWS EKS cluster scanning across regions
- Proper resource type validation and transformation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Confirm that getEksResources is exported and defined.
ast-grep --pattern $'export const getEksResources = $_' --extensions ts
Length of output: 270
Script:
#!/bin/bash
# Check if the eks.js/ts file exists and examine its content
fd "eks.js|eks.ts" apps/event-worker/src/target-scan/ -x cat {}
# Search for getEksResources function definition
ast-grep --pattern 'export const getEksResources = $_'
Length of output: 8572
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/AwsDialog.tsx (1)
166-169: Ensure unique keys in list rendering
When rendering a list of components, it's important to use unique keys to prevent rendering issues. Currently, the key prop is set to field.id, which is acceptable if field.id is unique. Please verify that field.id is guaranteed to be unique across re-renders.
packages/api/src/router/target-provider.ts (4)
15-21: AWS Resource Provider Imports Added Correctly
The AWS-related imports createResourceProviderAws, resourceProviderAws, and updateResourceProviderAws are appropriately added. This enables the handling of AWS resource providers alongside existing providers.
46-49: Including AWS Configurations in Resource Providers Retrieval
The additional leftJoin with resourceProviderAws correctly integrates AWS configurations when fetching resource providers. This ensures that AWS-specific settings are included in the query results.
94-94: Adding awsConfig to Response for AWS Support
Including awsConfig in the response object allows clients to access AWS-specific configurations. This mirrors the existing pattern for Google configurations and maintains consistency.
314-326: Ensure Proper Handling When Updating Scan Intervals
When updating repeatSeconds, the code removes the existing job from targetScanQueue and adds a new one. However, if repeatSeconds is not provided (null), the scan might not be scheduled properly.
Please verify that the absence of repeatSeconds results in the desired behavior. If the intention is to maintain the existing scan interval when repeatSeconds is null, ensure that the code handles this case appropriately.
Run the following script to check if providers without repeatSeconds have scheduled scans:
Replace {owner} and {repo} with the appropriate repository information.
✅ Verification successful
Let me gather more information about how the code handles the repeatSeconds case by examining the codebase.
Let me check how the initial scheduling of scans is handled to understand the default behavior.
Based on the code analysis, I can now generate the final response:
Scan Scheduling Behavior is Properly Handled
When repeatSeconds is null, the code correctly falls through to the default behavior, which adds a one-time scan to the queue without a repeat interval. This is evident from the code structure:
- If
repeatSecondsis provided, it removes the existing job and adds a new one with the specified interval - If
repeatSecondsis null, it adds a one-time scan job without repeat settings
The code follows a clear pattern where:
- Initial scans are scheduled with a default 10-minute interval
- Updates with
repeatSecondscreate a new recurring schedule - Updates without
repeatSecondscreate a one-time scan
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that resource providers have scheduled scans even when repeatSeconds is null.
# List all resource providers and their scheduled jobs
gh api repos/{owner}/{repo}/actions/runs --jq '.workflow_runs[] | {providerId: .id, status: .status}'
Length of output: 1545
Script:
#!/bin/bash
# Check the implementation around targetScanQueue to understand the null handling
ast-grep --pattern 'if (input.repeatSeconds != null) {
$$$
}'
# Also search for other references to repeatSeconds
rg "repeatSeconds" -A 3 -B 3
Length of output: 14002
Script:
#!/bin/bash
# Search for initial scheduling of scans
rg "targetScanQueue.add" -A 5 -B 5
# Also check the schema definition for resource providers
ast-grep --pattern 'z.object({
$$$
})'
Length of output: 100254
...ebservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/aws/page.tsx
Show resolved
Hide resolved
...ce/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx
Outdated
Show resolved
Hide resolved
...src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/GoogleActionButton.tsx
Show resolved
Hide resolved
apps/webservice/src/app/api/v1/resource-providers/[providerId]/set/route.ts
Outdated
Show resolved
Hide resolved
| const policyDocument = { | ||
| Version: "2012-10-17", | ||
| Statement: [ | ||
| { | ||
| Effect: "Allow", | ||
| Action: ["sts:*"], | ||
| Resource: "*", | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
Limit IAM Policy Actions and Resources to Minimum Necessary
The policyDocument grants all sts actions on all resources ("Action": ["sts:*"], "Resource": "*") which is overly permissive. This violates the principle of least privilege and could expose your application to security risks.
Refine the IAM policy to include only necessary actions and resources:
const policyDocument = {
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
- Action: ["sts:*"],
- Resource: "*",
+ Action: ["sts:SpecificAction"], // e.g., "sts:AssumeRole"
+ Resource: "arn:aws:sts::ACCOUNT_ID:role/TargetRole", // Replace with specific resource ARN
},
],
};Replace "sts:SpecificAction" with the specific actions required (e.g., "sts:AssumeRole") and "arn:aws:sts::ACCOUNT_ID:role/TargetRole" with the ARN of the specific resource you intend to allow access to.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Can we scope this down? No worries if that will be hard
There was a problem hiding this comment.
@jsbroks, absolutely, we can scope down the IAM policy to include only the necessary actions and resources.
Given that your application is performing role assumption, we can limit the policy to the specific actions and roles required. Here's how you might adjust the policyDocument:
const policyDocument = {
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
Action: ["sts:AssumeRole"],
Resource: ["arn:aws:iam::<ACCOUNT_ID>:role/<TargetRoleName>"], // Replace with specific role ARN(s)
},
],
};Replace <ACCOUNT_ID> with your AWS account ID and <TargetRoleName> with the name of the role your application needs to assume.
If your application needs to assume multiple roles, you can include all the required role ARNs in the Resource array.
By specifying only the sts:AssumeRole action and the exact resources, we adhere to the principle of least privilege and enhance the security of the application.
Let me know if you need any further assistance with this.
There was a problem hiding this comment.
This role will assume a customer provided role. We don't know what the arn or account it will be before hand.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai what do you recommend?
There was a problem hiding this comment.
@zacharyblasczyk, thank you for the clarification.
Given that the role needs to assume customer-provided roles and the ARNs are not known beforehand, it's not feasible to scope down the Resource field. However, we can enhance security by limiting the Action to only the necessary permission.
Here's how you might adjust the policy:
const policyDocument = {
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
Action: ["sts:AssumeRole"],
Resource: "*",
},
],
};By restricting the Action to "sts:AssumeRole", we reduce the permissions to only what's necessary, adhering more closely to the principle of least privilege while still accommodating unknown role ARNs.
Let me know if this works for your use case.
✏️ Learnings added
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#236
File: packages/api/src/router/workspace-integrations.ts:222-231
Timestamp: 2024-12-02T01:47:59.485Z
Learning: In `packages/api/src/router/workspace-integrations.ts`, when creating IAM policies for roles that need to assume customer-provided roles with unknown ARNs, it's acceptable to keep `Resource` as `"*"` in the policy document, while limiting the `Action` to `"sts:AssumeRole"`, since ARNs are not known in advance.
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 roleName = `ctrlplane-${ws.slug}`; | ||
|
|
||
| const assumeRolePolicyDocument = { | ||
| Version: "2012-10-17", | ||
| Statement: [ | ||
| { | ||
| Effect: "Allow", | ||
| Principal: { | ||
| AWS: `arn:aws:iam::${accountId}:root`, | ||
| }, | ||
| Action: "sts:AssumeRole", | ||
| }, |
There was a problem hiding this comment.
Restrict IAM Role Trust Policy to Specific Principals
The assumeRolePolicyDocument allows the root account (arn:aws:iam::${accountId}:root) to assume the role. This means any IAM user or role in the account can assume this role, which may not be intentional and could pose security risks. It's better to limit the principals that can assume the role to only those that require it.
Consider specifying a particular role or user that should be allowed to assume this role:
const assumeRolePolicyDocument = {
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
Principal: {
- AWS: `arn:aws:iam::${accountId}:root`,
+ AWS: `arn:aws:iam::${accountId}:role/YourTrustedRoleName`,
},
Action: "sts:AssumeRole",
},
],
};Replace YourTrustedRoleName with the actual role that should have permission to assume this role.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Bump on this? Is there a way we can fix this
There was a problem hiding this comment.
@jsbroks, yes, we can fix this by restricting the IAM role's trust policy to specific principals. Instead of allowing the entire root account to assume the role, we can specify the exact IAM role or user that should have permission.
Here's how you can modify the assumeRolePolicyDocument:
const trustedPrincipalArn = `arn:aws:iam::${accountId}:role/YourTrustedRoleName`;
const assumeRolePolicyDocument = {
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
Principal: {
AWS: trustedPrincipalArn,
},
Action: "sts:AssumeRole",
},
],
};Replace YourTrustedRoleName with the name of the IAM role that should be allowed to assume this role.
If you have multiple roles or users that need access, you can specify them as an array:
const trustedPrincipalArns = [
`arn:aws:iam::${accountId}:role/Role1`,
`arn:aws:iam::${accountId}:user/User1`,
];
const assumeRolePolicyDocument = {
Version: "2012-10-17",
Statement: [
{
Effect: "Allow",
Principal: {
AWS: trustedPrincipalArns,
},
Action: "sts:AssumeRole",
},
],
};This approach ensures that only the specified roles or users can assume the IAM role, enhancing security by preventing unauthorized access.
Let me know if you need further assistance with this!
| cluster: Cluster, | ||
| ): KubernetesClusterAPIV1 => { | ||
| const region = cluster.endpoint?.split(".")[2]; | ||
| const appUrl = `https://console.aws.amazon.com/eks/home?region=${region}#/clusters/${cluster.name}`; |
There was a problem hiding this comment.
what about gov cloud? how does this work? can we not generate it from the cluster object?
There was a problem hiding this comment.
the ARN is really the only reliable source for this information. The ARN will contain the partition information, which is why we're using it as the primary source and falling back to region-based detection.
Commercial: arn:aws:eks:region:account-id:cluster/cluster-name
GovCloud: arn:aws-us-gov:eks:region:account-id:cluster/cluster-name
const partition = cluster.arn?.split(":")[1] ??
(region?.startsWith("us-gov-") ? "aws-us-gov" : "aws");
const appUrl = `https://${
partition === "aws-us-gov"
? `console.${region}.${partition}`
: "console.aws.amazon"
}.com/eks/home?region=${region}#/clusters/${cluster.name}`;
...ce/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx
Outdated
Show resolved
Hide resolved
...ce/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/UpdateAwsProviderDialog.tsx (3)
45-64: Consider making theawsConfigprop required.Since the component relies on
awsConfigfor initialization, consider making it non-nullable to prevent potential runtime issues. This would make the component's requirements more explicit at compile time.- awsConfig: ResourceProviderAws | null; + awsConfig: ResourceProviderAws;
111-111: Consider adding a max-width to the dialog content.The dialog content has a max-height constraint but lacks a max-width, which could lead to overly wide content on large screens.
- <DialogContent className={"max-h-screen overflow-y-scroll"}> + <DialogContent className={"max-h-screen max-w-2xl overflow-y-scroll"}>
233-240: Add helper text for scan frequency input.Consider adding helper text to explain the implications of different scan frequency values and what 0 means.
<FormControl> <Input type="number" min={0} {...field} onChange={(e) => field.onChange(e.target.valueAsNumber)} /> </FormControl> + <p className="text-sm text-muted-foreground"> + Set to 0 to disable automatic scanning. Minimum scan interval is 1 second. + </p>apps/event-worker/src/target-scan/index.ts (1)
116-135: Consider reducing code duplication between AWS and Google scanners.The AWS and Google scanner implementations share similar patterns. Consider extracting the common logic into a reusable function.
+const createProviderScanWorker = ( + providerType: 'aws' | 'google', + providerField: 'resource_provider_aws' | 'resource_provider_google', + scanFn: (workspace: any, provider: any) => Promise<InsertResource[]> +) => + createResourceScanWorker(async (rp) => { + if (rp[providerField] == null) { + log.info( + `No ${providerType.toUpperCase()} provider found for resource provider ${rp.resource_provider.id}, skipping scan` + ); + return []; + } + + const resources: InsertResource[] = []; + log.info(`Found ${providerType.toUpperCase()} config, scanning for resources`); + + const scannedResources = await scanFn( + rp.workspace, + rp[providerField] + ); + resources.push(...scannedResources); + + return resources; + }); -export const createGoogleResourceScanWorker = () => - createResourceScanWorker(async (rp) => { - if (rp.resource_provider_google == null) { - log.info( - `No Google provider found for resource provider ${rp.resource_provider.id}, skipping scan`, - ); - return []; - } - - const resources: InsertResource[] = []; - log.info("Found Google config, scanning for GKE resources"); - - const gkeResources = await getGkeResources( - rp.workspace, - rp.resource_provider_google, - ); - resources.push(...gkeResources); - - return resources; - }); +export const createGoogleResourceScanWorker = () => + createProviderScanWorker('google', 'resource_provider_google', getGkeResources); -export const createAwsResourceScanWorker = () => - createResourceScanWorker(async (rp) => { - if (rp.resource_provider_aws == null) { - log.info( - `No AWS provider found for resource provider ${rp.resource_provider.id}, skipping scan`, - ); - return []; - } - - const resources: InsertResource[] = []; - log.info("Found AWS config, scanning for EKS resources"); - - const eksResources = await getEksResources( - rp.workspace, - rp.resource_provider_aws, - ); - resources.push(...eksResources); - - return resources; - }); +export const createAwsResourceScanWorker = () => + createProviderScanWorker('aws', 'resource_provider_aws', getEksResources);apps/event-worker/src/target-scan/eks.ts (1)
153-153: Fix typo in log message."cluters" should be "clusters".
- `Scanning for EKS cluters with assumed role arns ${config.awsRoleArns.join(", ")} using role ${workspaceRoleArn}`, + `Scanning for EKS clusters with assumed role arns ${config.awsRoleArns.join(", ")} using role ${workspaceRoleArn}`,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/event-worker/src/target-scan/eks.ts(1 hunks)apps/event-worker/src/target-scan/index.ts(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/UpdateAwsProviderDialog.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/AwsActionButton.tsx
🔇 Additional comments (11)
apps/webservice/src/app/[workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/UpdateAwsProviderDialog.tsx (2)
1-43: LGTM! Well-structured imports and schema definition.
The imports are logically organized, and the schema properly validates the repeatSeconds field with a minimum value constraint.
65-80: Add error handling to the form submission.
The form submission lacks error handling for failed mutations.
apps/event-worker/src/target-scan/index.ts (4)
10-10: LGTM! Clean import organization and proper logger setup.
The new imports are well-organized, and the logger setup follows the recommended pattern from previous reviews.
Also applies to: 19-19, 22-22
33-35: Improve type safety by specifying the type of scanResources parameter.
The any type undermines TypeScript's type-checking benefits. Define a specific type for rp to enhance type safety.
50-53: LGTM! Well-structured database queries and error handling.
The changes demonstrate:
- Proper integration of AWS provider joins
- Consistent use of the new logger instance
- Comprehensive error handling with detailed logging
Also applies to: 57-84
95-114: LGTM! Clean implementation of Google resource scanning.
The implementation demonstrates good practices:
- Early return pattern for better flow control
- Clear logging for operational visibility
- Proper resource aggregation
apps/event-worker/src/target-scan/eks.ts (5)
26-26: Extract region from cluster.arn instead of endpoint.
The current implementation extracts the region from the endpoint URL, which could be unreliable. As discussed in previous reviews, extracting from ARN is more reliable.
- const region = cluster.endpoint?.split(".")[2];
+ const region = cluster.arn?.split(":")[3];38-52: Replace non-null assertions with null-safe operations.
Multiple non-null assertions (!) could lead to runtime exceptions if properties are undefined.
- const version = cluster.version!;
+ const version = cluster.version ?? "unknown";
- const [major, minor] = version.split(".");
+ const [major, minor] = version.split(".") ?? ["unknown", "unknown"];
- name: cluster.name!,
+ name: cluster.name ?? "",
- region: region!,
+ region: region ?? "unknown",
- clusterName: cluster.name!,
+ clusterName: cluster.name ?? "",103-117: Handle API call failures individually.
If any DescribeClusterCommand fails, it will reject the entire Promise.all. Handle each API call failure individually to ensure the scan continues.
return _.chain(clusters)
.map((name) =>
eksClient
.send(new DescribeClusterCommand({ name }))
.then(({ cluster }) => cluster)
+ .catch((error) => {
+ log.error(`Failed to describe cluster ${name} in region ${region}: ${error}`);
+ return null; // Skip this cluster and continue
+ }),
)
.thru((promises) => Promise.all(promises))
.value()
.then((clusterDetails) =>
clusterDetails
.filter(isPresent)
.map((cluster) =>
convertEksClusterToKubernetesResource(accountId, cluster),
),
);137-143: Use Promise.allSettled for robust error handling across regions.
Using Promise.all will cause the entire scan to fail if any region fails. Use Promise.allSettled to handle failures gracefully.
return _.chain(regions)
.filter(isPresent)
.map(regionalClusterScanner)
- .thru((promises) => Promise.all(promises))
+ .thru((promises) => Promise.allSettled(promises))
.value()
- .then((results) => results.flat())
+ .then((results) =>
+ results
+ .filter((result): result is PromiseFulfilledResult<KubernetesClusterAPIV1[]> =>
+ result.status === 'fulfilled')
+ .flatMap((result) => result.value)
+ );164-177: Use Promise.allSettled for robust error handling across role ARNs.
Using Promise.all will cause the entire scan to fail if scanning fails for any role ARN. Use Promise.allSettled to handle failures gracefully.
const resources = await _.chain(config.awsRoleArns)
.map((customerRoleArn) =>
scanEksClustersByAssumedRole(workspaceStsClient, customerRoleArn)
+ .catch((error) => {
+ log.error(`Failed to scan clusters for role ${customerRoleArn}: ${error}`);
+ return [];
+ })
)
- .thru((promises) => Promise.all(promises))
+ .thru((promises) => Promise.allSettled(promises))
.value()
- .then((results) => results.flat())
+ .then((results) =>
+ results
+ .filter((result): result is PromiseFulfilledResult<KubernetesClusterAPIV1[]> =>
+ result.status === 'fulfilled')
+ .flatMap((result) => result.value)
+ )
.then((resources) =>
resources.map((resource) => ({
...resource,
workspaceId: workspace.id,
providerId: config.resourceProviderId,
})),
);
...workspaceSlug]/(app)/(targets)/target-providers/integrations/aws/UpdateAwsProviderDialog.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/workspace-integrations.ts (1)
170-173: Consider making AWS region configurableThe AWS region is hardcoded to "us-east-1". Consider making it configurable through environment variables to support different deployment regions.
+const AWS_REGION = process.env.AWS_REGION ?? "us-east-1"; const iamClient = new IAMClient({ - region: "us-east-1", + region: AWS_REGION, credentials: defaultProvider(), }); const stsClient = new STSClient({ - region: "us-east-1", + region: AWS_REGION, credentials: defaultProvider(), });Also applies to: 175-178
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/api/src/router/workspace-integrations.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/workspace-integrations.ts (1)
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#236
File: packages/api/src/router/workspace-integrations.ts:222-231
Timestamp: 2024-12-02T01:47:59.631Z
Learning: In `packages/api/src/router/workspace-integrations.ts`, when creating IAM policies for roles that need to assume customer-provided roles with unknown ARNs, it's acceptable to keep `Resource` as `"*"` in the policy document, while limiting the `Action` to `"sts:AssumeRole"`, since ARNs are not known in advance.
🔇 Additional comments (2)
packages/api/src/router/workspace-integrations.ts (2)
221-227: Add Proper Error Handling for AWS SDK Calls
The AWS SDK calls need proper error handling to manage exceptions gracefully.
307-312: Handle Potential Exceptions When Deleting AWS Resources
When deleting the role policy and the role, exceptions need to be handled properly.
Also applies to: 314-318
Co-authored-by: Justin Brooks <[email protected]> Signed-off-by: Zachary Blasczyk <[email protected]>
Co-authored-by: Justin Brooks <[email protected]> Signed-off-by: Zachary Blasczyk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/event-worker/src/target-scan/index.ts (3)
79-84: Enhance error handling with specific error types.The current error handling catches all errors and logs them generically. Consider handling specific error types differently.
Consider this pattern:
- } catch (error: any) { - log.error( - `Error scanning/upserting resources for provider ${rp.resource_provider.id}: ${error.message}`, - { error }, - ); + } catch (error) { + if (error instanceof AWS.AWSError) { + log.error( + `AWS Error scanning resources for provider ${rp.resource_provider.id}: ${error.message}`, + { code: error.code, requestId: error.requestId }, + ); + } else if (error instanceof GoogleAPIError) { + log.error( + `Google API Error scanning resources for provider ${rp.resource_provider.id}: ${error.message}`, + { status: error.status }, + ); + } else { + log.error( + `Unexpected error scanning resources for provider ${rp.resource_provider.id}: ${error instanceof Error ? error.message : String(error)}`, + { error }, + ); + }
95-110: Simplify the Google worker with early returns.The function can be more concise with early returns.
Consider this pattern:
export const createGoogleResourceScanWorker = () => createResourceScanWorker(async (rp) => { if (rp.resource_provider_google == null) { log.info( `No Google provider found for resource provider ${rp.resource_provider.id}, skipping scan`, ); return []; } - - const resources = await getGkeResources( - rp.workspace, - rp.resource_provider_google, - ); - - return resources; + return getGkeResources(rp.workspace, rp.resource_provider_google); });
112-127: Apply consistent pattern to AWS worker.For consistency with the Google worker, simplify the AWS implementation.
Apply similar pattern:
export const createAwsResourceScanWorker = () => createResourceScanWorker(async (rp) => { if (rp.resource_provider_aws == null) { log.info( `No AWS provider found for resource provider ${rp.resource_provider.id}, skipping scan`, ); return []; } - - const resources = await getEksResources( - rp.workspace, - rp.resource_provider_aws, - ); - - return resources; + return getEksResources(rp.workspace, rp.resource_provider_aws); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/event-worker/src/target-scan/index.ts(4 hunks)
🔇 Additional comments (3)
apps/event-worker/src/target-scan/index.ts (3)
10-11: LGTM! Clean integration of AWS support.
The new imports and logger initialization are well-structured and consistent with the existing codebase.
Also applies to: 19-22
33-34:
Improve type safety of the scanResources parameter.
The any type undermines TypeScript's type safety. This was previously flagged in past reviews.
Apply this diff:
- scanResources: (rp: any) => Promise<InsertResource[]>,
+ import type { ResourceProviderJoined } from "@ctrlplane/db/types";
+ scanResources: (rp: ResourceProviderJoined) => Promise<InsertResource[]>,19-20: Verify error handling in resource scanners.
Let's verify the error handling implementation in the imported scanner functions.
Also applies to: 104-107, 121-124
✅ Verification successful
Let me gather more information about error handling since the files are .ts not .js.
Error handling is properly implemented in both scanners
The verification shows robust error handling across both scanners:
- GKE scanner implements comprehensive try-catch blocks for:
- Permission-related errors in cluster access
- Cluster connection failures
- Namespace listing errors
- VCluster listing operations
- EKS scanner uses:
- Input validation with thrown errors
- Promise.all() for concurrent operations with proper error propagation
- Both scanners include appropriate error logging with detailed context
The concurrent operations through Promise.all() are handled safely, with proper error boundaries in place. No race conditions were identified in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in EKS and GKE resource scanners
# Check for error handling patterns in scanner implementations
echo "Checking error handling in EKS scanner:"
ast-grep --pattern 'try { $$$ } catch' ./apps/event-worker/src/target-scan/eks.js
echo "Checking error handling in GKE scanner:"
ast-grep --pattern 'try { $$$ } catch' ./apps/event-worker/src/target-scan/gke.js
# Check for potential race conditions in concurrent operations
echo "Checking for concurrent operations:"
rg -A 3 "Promise\.all|Promise\.race" ./apps/event-worker/src/target-scan/
Length of output: 2241
Script:
#!/bin/bash
# Check error handling in TypeScript files
echo "Checking error handling in EKS scanner:"
rg -A 5 "try|catch|throw" ./apps/event-worker/src/target-scan/eks.ts
echo -e "\nChecking error handling in GKE scanner:"
rg -A 5 "try|catch|throw" ./apps/event-worker/src/target-scan/gke.ts
echo -e "\nChecking Promise error handling:"
rg -A 3 "\.catch|\.finally" ./apps/event-worker/src/target-scan/
Length of output: 2562
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/event-worker/package.json(3 hunks)packages/api/package.json(1 hunks)packages/api/src/router/workspace-integrations.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/api/package.json
- apps/event-worker/package.json
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/workspace-integrations.ts (2)
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#236
File: packages/api/src/router/workspace-integrations.ts:191-196
Timestamp: 2024-12-02T04:37:54.780Z
Learning: In the AWS integration, users will add role ARNs in the standard format `arn:aws:iam::<account-id>:role/<role-name>`, even when the trust relationship is created from an assumed role. Therefore, validation patterns need to accept only the standard format and do not need to handle SSO role ARNs.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#236
File: packages/api/src/router/workspace-integrations.ts:222-231
Timestamp: 2024-12-02T01:47:59.631Z
Learning: In `packages/api/src/router/workspace-integrations.ts`, when creating IAM policies for roles that need to assume customer-provided roles with unknown ARNs, it's acceptable to keep `Resource` as `"*"` in the policy document, while limiting the `Action` to `"sts:AssumeRole"`, since ARNs are not known in advance.
🔇 Additional comments (1)
packages/api/src/router/workspace-integrations.ts (1)
289-306: Add check for existing AWS resource providers
This issue was previously identified. We should check for existing AWS resource providers before deleting the role to prevent orphaned resources.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx (1)
Line range hint
275-347: LGTM! Consider adding type safety for the event object.The implementation looks good with proper date handling, well-structured filter conditions, and correct navigation logic. However, consider adding type safety for the event object to prevent potential runtime issues.
Consider adding a type for the event object:
type BarClickEvent = { date: string; // Add other properties as needed }; // Then update the onClick handler: onClick={(e: BarClickEvent) => { const start = new Date(e.date); // ... rest of the code }}apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (2)
Line range hint
38-52: Enhance form validation for better data integrity.The current validation schema could be improved to ensure:
- End time is always after start time
- Rollout duration has reasonable minimum/maximum values
- Release windows don't overlap
Consider enhancing the schema validation:
const schema = z.object({ releaseWindows: z.array( z.object({ policyId: z.string().uuid(), recurrence: z.enum(["hourly", "daily", "weekly", "monthly"]), - startTime: z.date(), - endTime: z.date(), + startTime: z.date(), + endTime: z.date().refine( + (date, ctx) => { + const startTime = ctx.parent.startTime; + return date > startTime || 'End time must be after start time'; + } + ), }), + ).refine( + (windows) => { + // Check for overlapping windows + const sorted = _.sortBy(windows, 'startTime'); + for (let i = 1; i < sorted.length; i++) { + if (sorted[i].startTime < sorted[i-1].endTime) { + return false; + } + } + return true; + }, + { message: 'Release windows cannot overlap' } ), rolloutDuration: z.string().refine(isValidDuration, { message: "Invalid duration pattern", - }), + }).refine( + (str) => { + const duration = ms(str); + return duration >= 0 && duration <= ms('7d'); + }, + { message: 'Duration must be between 0 and 7 days' } + ), });
Line range hint
82-92: Improve error handling in form submission.The current implementation lacks proper error handling for mutations and doesn't maintain data consistency if one mutation fails.
Consider implementing proper error handling:
const onSubmit = form.handleSubmit(async (data) => { const { releaseWindows, rolloutDuration: durationString } = data; const rolloutDuration = ms(durationString); - await setPolicyWindows.mutateAsync({ policyId, releaseWindows }); - await updatePolicy.mutateAsync({ id: policyId, data: { rolloutDuration } }); - - form.reset(data); - await utils.environment.policy.byId.invalidate(policyId); - await utils.environment.policy.bySystemId.invalidate(systemId); + try { + await setPolicyWindows.mutateAsync({ policyId, releaseWindows }); + await updatePolicy.mutateAsync({ id: policyId, data: { rolloutDuration } }); + + form.reset(data); + await Promise.all([ + utils.environment.policy.byId.invalidate(policyId), + utils.environment.policy.bySystemId.invalidate(systemId) + ]); + } catch (error) { + console.error('Failed to update policy:', error); + // TODO: Add toast notification for error + // TODO: Implement retry mechanism + } });apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/DateConditionRender.tsx (1)
Line range hint
14-33: Consider timezone handling in AWS resource queries.The
toZonedDateTimeutility correctly handles timezone conversion, but we should verify that:
- The converted dates align with AWS API expectations
- The timezone handling is consistent across different AWS regions
Consider documenting the timezone handling approach for AWS resource queries, especially if resources span multiple regions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/DateConditionRender.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx(1 hunks)packages/api/src/router/workspace-integrations.ts(3 hunks)packages/ui/src/date-time-picker/date-time-picker.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/api/src/router/workspace-integrations.ts (2)
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#236
File: packages/api/src/router/workspace-integrations.ts:191-196
Timestamp: 2024-12-02T04:37:54.780Z
Learning: In the AWS integration, users will add role ARNs in the standard format `arn:aws:iam::<account-id>:role/<role-name>`, even when the trust relationship is created from an assumed role. Therefore, validation patterns need to accept only the standard format and do not need to handle SSO role ARNs.
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#236
File: packages/api/src/router/workspace-integrations.ts:222-231
Timestamp: 2024-12-02T01:47:59.631Z
Learning: In `packages/api/src/router/workspace-integrations.ts`, when creating IAM policies for roles that need to assume customer-provided roles with unknown ARNs, it's acceptable to keep `Resource` as `"*"` in the policy document, while limiting the `Action` to `"sts:AssumeRole"`, since ARNs are not known in advance.
🔇 Additional comments (8)
packages/api/src/router/workspace-integrations.ts (5)
25-33: Move AWS client initialization inside procedures
Moving the client initialization inside the procedures would prevent potential resource leaks and allow for better error handling.
193-193: Validate workspace slug for AWS role name compliance
The role name must comply with AWS IAM naming requirements.
291-308: Check for existing AWS resource providers before deletion
Similar to the Google service account deletion, check for existing AWS resource providers to prevent orphaned resources.
230-236:
Enhance error handling for AWS SDK calls
The AWS SDK calls should be wrapped in try-catch blocks to handle specific AWS errors gracefully.
+try {
const createRoleResponse = await iamClient.send(
new CreateRoleCommand({
RoleName: roleName,
Description: `Role for ${ws.slug} (${ws.id})`,
AssumeRolePolicyDocument: JSON.stringify(assumeRolePolicyDocument),
}),
);
+} catch (error) {
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: `Failed to create AWS role: ${error.message}`,
+ });
+}Likely invalid or redundant comment.
311-322:
Add error handling for AWS role and policy deletion
The AWS SDK calls for deletion should handle specific AWS errors gracefully.
+try {
await iamClient.send(
new DeleteRolePolicyCommand({
RoleName: roleName,
PolicyName: `${roleName}-ctrlplane-policy`,
}),
);
+} catch (error) {
+ if (error.name !== 'NoSuchEntityException') {
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: `Failed to delete AWS role policy: ${error.message}`,
+ });
+ }
+}
+try {
await iamClient.send(
new DeleteRoleCommand({
RoleName: roleName,
}),
);
+} catch (error) {
+ if (error.name !== 'NoSuchEntityException') {
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: `Failed to delete AWS role: ${error.message}`,
+ });
+ }
+}Likely invalid or redundant comment.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (1)
151-154: 🛠️ Refactor suggestion
Consider improving timezone handling and null safety.
The date handling logic has a few potential improvements:
- The timezone resolution is duplicated in both onChange handlers
- There's no null check before calling
toDate()which could be risky iftis undefined
Consider applying these improvements:
+ const getTimeZone = () => Intl.DateTimeFormat().resolvedOptions().timeZone;
<DateTimePicker
value={toZonedDateTime(value.startTime)}
aria-label="Start Time"
onChange={(t) => {
+ if (!t) return;
+ const timeZone = getTimeZone();
onChange({
...value,
- startTime: t.toDate(
- Intl.DateTimeFormat().resolvedOptions()
- .timeZone,
- ),
+ startTime: t.toDate(timeZone),
});
}}
/>
...
<DateTimePicker
value={toZonedDateTime(value.endTime)}
onChange={(t) => {
+ if (!t) return;
+ const timeZone = getTimeZone();
onChange({
...value,
- endTime: t.toDate(
- Intl.DateTimeFormat().resolvedOptions()
- .timeZone,
- ),
+ endTime: t.toDate(timeZone),
});
}}
aria-label="End Time"
/>Let's verify the timezone handling across the codebase:
Also applies to: 164-167
packages/ui/src/date-time-picker/date-time-picker.tsx (1)
69-75: Verify null value handling in TimeField onChange.
The removal of null checks in the TimeField onChange handler could potentially lead to issues if null values are passed through. While the code is cleaner, we should ensure that:
- The react-aria TimeField component handles null values appropriately
- Parent components are prepared to handle potential null values
Let's verify the TimeField usage in the codebase:
✅ Verification successful
Removal of null checks is safe in this implementation
The TimeField implementation is safe regarding null value handling because:
- It uses React Aria's
useTimeFieldhook which provides type-safe value handling throughTimeValuetype - The parent DateTimePicker component uses React Aria's
useDatePickerStatewhich manages the time value state - The
timeValueandsetTimeValueare provided by the state management system that ensures type safety
The removal of explicit null checks actually aligns better with the React Aria implementation, as the value handling is already managed by the framework's state management system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other TimeField usages to check for consistent null handling patterns
rg -A 3 "TimeField.*onChange" --type typescript
Length of output: 84
Script:
#!/bin/bash
# Search for TimeField usages with proper file extensions
rg -A 3 "TimeField.*onChange" -t tsx -t ts
# Also search for TimeField imports to understand its origin
rg "import.*TimeField" -t tsx -t ts
# Search for TimeField component definition
ast-grep --pattern 'interface TimeFieldProps {
$$$
}'
Length of output: 196
Script:
#!/bin/bash
# Search for TimeField usages without file type flags
rg -A 3 "TimeField.*onChange"
# Search for TimeField imports
rg "import.*TimeField"
# Look for files that might contain TimeField definition
fd TimeField
Length of output: 481
Script:
#!/bin/bash
# Let's examine the TimeField implementation to understand how it handles values
cat packages/ui/src/date-time-picker/time-field.tsx
# Also check the date-time-picker implementation for context
cat packages/ui/src/date-time-picker/date-time-picker.tsx
Length of output: 4000
apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/DateConditionRender.tsx (1)
85-85: Consider impact on AWS resource filtering.
While the simplified onChange handler is consistent with the DateTimePicker changes, we should ensure that:
- The date filtering logic in AWS resource queries can handle potential null/invalid dates
- The parent components validate dates before using them in AWS API calls
Let's verify the date handling in AWS resource queries:
Summary by CodeRabbit
Release Notes
New Features
AwsActionButton,AwsDialog, andUpdateAwsProviderDialog.ProviderActionsDropdownto manage AWS configurations alongside existing Google configurations.AwsCredentialsclass for managing AWS credentials and service interactions.Bug Fixes
Documentation
Chores