feat: AWS VCP Scanner #275
Conversation
WalkthroughThis pull request introduces support for scanning and importing Amazon Virtual Private Cloud (VPC) resources alongside existing Amazon Elastic Kubernetes Service (EKS) cluster scanning capabilities. The changes encompass updates to documentation, database schema, frontend dialogs, and backend scanning logic. Users can now toggle VPC resource import through new configuration options, with default settings configured to maintain existing behavior while enhancing flexibility for resource management. Changes
Sequence DiagramsequenceDiagram
participant User
participant WebUI
participant Backend
participant AWS
User->>WebUI: Configure AWS Resource Provider
WebUI->>User: Present Import Options
User->>WebUI: Select VPC/EKS Import Preferences
WebUI->>Backend: Submit Configuration
Backend->>AWS: Assume Role
AWS->>Backend: Retrieve VPC/EKS Resources
Backend->>Backend: Transform Resources
Backend->>WebUI: Return Imported Resources
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/event-worker/src/resource-scan/aws/vpc.ts (1)
68-74: Consider avoiding the spread operator on accumulators for better performance.Using spread syntax (
{ ...acc }) within.reduce()can introduce an O(n²) complexity ifvpc.Tagsgrows larger. Consider mutating the accumulator object directly:- ...(vpc.Tags?.reduce( - (acc, tag) => ({ - ...acc, - [`aws/tag/${tag.Key}`]: tag.Value, - }), - {}, - ) ?? {}) + ...(vpc.Tags?.reduce((acc, tag) => { + acc[`aws/tag/${tag.Key}`] = tag.Value; + return acc; + }, {}) ?? {})🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
apps/docs/pages/integrations/aws/compute-scanner.mdx (1)
9-9: Documentation updated to include VPC.
Having a separate bullet point for VPC is clear and user-friendly. Encouraging clarity on any specific permissions or constraints would be helpful for new users.packages/db/drizzle/meta/_journal.json (1)
355-360: Consider deployment strategy and performance implications.Since this migration adds new configuration columns for AWS resource imports:
Database Migration Strategy:
- Consider adding a deployment guide for existing installations
- Document the default behavior for existing AWS providers
- Include rollback procedures
Performance Considerations:
- VPC scanning might significantly increase the resource discovery time
- Consider implementing pagination or batch processing for VPC resources
- Monitor AWS API usage to avoid hitting rate limits
Would you like me to help create a deployment guide or documentation for these considerations?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/docs/pages/integrations/aws/compute-scanner.mdx(1 hunks)apps/event-worker/src/resource-scan/aws/eks.ts(1 hunks)apps/event-worker/src/resource-scan/aws/index.ts(2 hunks)apps/event-worker/src/resource-scan/aws/vpc.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/UpdateAwsProviderDialog.tsx(4 hunks)packages/db/drizzle/0050_bizarre_red_shift.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/resource-provider.ts(1 hunks)packages/validators/src/resources/cloud-v1.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
packages/db/src/schema/resource-provider.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/resource-scan/aws/eks.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/resource-scan/aws/index.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/validators/src/resources/cloud-v1.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/UpdateAwsProviderDialog.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/resource-scan/aws/vpc.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🪛 Biome (1.9.4)
apps/event-worker/src/resource-scan/aws/vpc.ts
[error] 70-70: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (15)
apps/event-worker/src/resource-scan/aws/vpc.ts (2)
22-77: Robust conversion function; looks good overall.
Your approach to converting an AWS Vpc object into the CloudVPCV1 schema is clean and well-structured. Adding console links for AWS fosters user-friendly navigation, and the partition-based logic appropriately handles GovCloud distinctions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
165-204: Validate edge cases when importing VPCs.
If config.awsRoleArns is empty or if an assumed role lacks permission to describe VPCs, this logic might yield empty resources without logging warnings. Consider logging a message or handling potential permission failures more explicitly to aid debugging.
apps/event-worker/src/resource-scan/aws/index.ts (1)
5-5: Seamless integration of EKS and VPC resource imports.
Importing getVpcResources and combining its results with EKS resources aligns well with the new multi-resource scanning approach. This maintains clarity while expanding AWS coverage.
Also applies to: 18-28
packages/validators/src/resources/cloud-v1.ts (1)
13-29: Schema expansion supports comprehensive VPC details.
Adding accountId, optional subnet properties, and secondaryCidrs offers flexibility for AWS-based VPC representations. The new fields are well-structured and maintain consistency with existing patterns.
packages/db/src/schema/resource-provider.ts (1)
85-87: New flags for controlling VPC and EKS imports look correct.
Defining importEks and importVpc in the schema with default false values aligns well with the pull request objective. They are clearly integrated into the cascading references in the codebase.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx (2)
52-53: Form schema and defaults effectively handle EKS/VPC toggles.
Introducing importEks and importVpc in the schema with sensible defaults helps ensure form data integrity. Initializing importEks to true highlights its common usage, while disabling VPC scanning by default is prudent for gradual adoption.
Also applies to: 65-66
205-245: Clear toggle UI for EKS and VPC import.
The two Switch components and descriptive FormDescription fields provide a straightforward user experience. This clarity helps guide users to enable or disable importing EKS clusters and VPCs per their needs.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/UpdateAwsProviderDialog.tsx (5)
26-26: Newly imported FormDescription confirmed.
The addition of FormDescription from @ctrlplane/ui/form looks consistent and is appropriately used below.
36-36: Switch import for toggle fields.
Looks good. The Switch component import is aligned with the UI library usage.
76-77: Inclusion of importEks and importVpc in mutation payload.
The new fields appear consistent with the form schema. Verify that these fields have default values in form definitions or schemas so that the mutation can gracefully handle their absence.
231-250: EKS Import Toggle Implementation.
Good job using a dedicated UI section to selectively enable EKS cluster imports. This straightforward toggle approach fosters clarity for end users.
252-271: VPC Import Toggle Implementation.
Similarly, the VPC toggle is well-structured. Ensure that the user is aware of any potential time or cost impacts from scanning large VPCs in multiple regions.
packages/db/drizzle/0050_bizarre_red_shift.sql (1)
1-2: Add columns for import_eks and import_vpc.
The DDL changes look correct. As a safeguard, consider documenting any rollback or migration-down instructions if needed.
apps/event-worker/src/resource-scan/aws/eks.ts (1)
164-179: Conditional scanning for EKS clusters (importEks).
The logic to skip scanning when importEks is false is a concise way to reduce unnecessary work. Ensure potential errors from invalid AWS roles are handled, especially when scanning is bypassed.
packages/db/drizzle/meta/_journal.json (1)
355-360: LGTM! The migration journal entry follows the established pattern.
The new entry maintains consistency with existing entries in terms of structure and versioning.
Let's verify the related migration files and default values:
✅ Verification successful
Migration files and default values are properly configured
The verification confirms:
- The SQL migration file
0050_bizarre_red_shift.sqlexists in the expected location - Both new columns
import_eksandimport_vpcare added with:DEFAULT falseensuring existing rows get proper default valuesNOT NULLconstraint preventing null valuesbooleantype matching the expected data type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify migration files and default values
# Check if the corresponding SQL migration file exists
echo "Checking for migration SQL file..."
fd -t f "0050_bizarre_red_shift.sql" packages/db/drizzle
# Check for default values in migration
echo "Checking default values in migration..."
rg -A 5 "ALTER TABLE.*resource_provider_aws" packages/db/drizzle/0050_bizarre_red_shift.sql
Length of output: 573
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/validators/src/resources/cloud-v1.ts (1)
28-30: Consider enhancing the secondaryCidrs schema.While the implementation is functional, consider these improvements:
- Add CIDR format validation
- Use an enum for the
statefield to restrict it to valid AWS VPC CIDR statesHere's a suggested enhancement:
secondaryCidrs: z // for AWS .array(z.object({ - cidr: z.string(), + cidr: z.string().regex(/^(\d{1,3}\.){3}\d{1,3}\/\d{1,2}$/), - state: z.string() + state: z.enum(["associating", "associated", "disassociating", "disassociated", "failing", "failed"]) })) .optional(),apps/event-worker/src/resource-scan/aws/vpc.ts (3)
88-120: Enhance subnet null value handlingWhile the implementation is solid, the subnet mapping could be more robust with null coalescing.
Consider this improvement:
return Subnets.map((subnet) => ({ - name: subnet.SubnetId ?? "", + name: subnet.Tags?.find(tag => tag.Key === "Name")?.Value ?? subnet.SubnetId ?? "", region, cidr: subnet.CidrBlock ?? "", type: subnet.MapPublicIpOnLaunch ? "public" : "private", - availabilityZone: subnet.AvailabilityZone ?? "", + availabilityZone: subnet.AvailabilityZone ?? region, }));
122-145: Consider batch processing for large VPC collectionsThe current implementation processes all VPCs in parallel, which could lead to memory pressure with a large number of VPCs.
Consider implementing batch processing:
const batchSize = 10; const vpcResources = []; for (let i = 0; i < vpcs.length; i += batchSize) { const batch = vpcs.slice(i, i + batchSize); const results = await Promise.all( batch.map(async (vpc) => { if (!vpc.VpcId) return null; const subnets = await getSubnets(client, region, vpc.VpcId); return convertVpcToCloudResource(accountId, region, vpc, subnets); }) ); vpcResources.push(...results.filter(isPresent)); }
168-207: Enhance error handling and loggingWhile the implementation is solid, it could benefit from more detailed error handling and logging.
Consider wrapping the main processing in a try-catch block:
export const getVpcResources = async ( workspace: Workspace, config: ResourceProviderAws, ) => { const { awsRoleArn: workspaceRoleArn } = workspace; if (workspaceRoleArn == null) return []; log.info( `Scanning for VPCs with assumed role arns ${config.awsRoleArns.join(", ")} using role ${workspaceRoleArn}`, { workspaceId: workspace.id, config, workspaceRoleArn, }, ); + try { const credentials = await assumeWorkspaceRole(workspaceRoleArn); const workspaceStsClient = credentials.sts(); const resources = config.importVpc ? await _.chain(config.awsRoleArns) // ... rest of the chain ... : []; log.info(`Found ${resources.length} VPC resources`); return resources; + } catch (error) { + log.error('Failed to scan VPC resources', { + error, + workspaceId: workspace.id, + workspaceRoleArn, + }); + throw error; + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/resource-scan/aws/vpc.ts(1 hunks)packages/validators/src/resources/cloud-v1.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/validators/src/resources/cloud-v1.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/resource-scan/aws/vpc.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🪛 Biome (1.9.4)
apps/event-worker/src/resource-scan/aws/vpc.ts
[error] 73-73: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (5)
packages/validators/src/resources/cloud-v1.ts (2)
10-10: LGTM! Verify AWS-specific field usage.
The new optional fields id and accountId are correctly typed and documented for AWS configurations.
Let's verify the consistent usage of these fields:
Also applies to: 14-14
✅ Verification successful
AWS VPC fields are used consistently across the codebase
The verification shows that the AWS-specific fields (id and accountId) in the VPC schema are used consistently:
- In
apps/event-worker/src/resource-scan/aws/vpc.ts, both fields are properly populated during VPC resource conversion:id: vpc.VpcId!is set from AWS VPC dataaccountId: accountIdis extracted from AWS role ARN and passed through
The fields are used as part of the resource identifier pattern aws/${accountId}/vpc/${vpc.VpcId}, indicating a well-structured and consistent approach to AWS resource handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of AWS-specific VPC fields
# Search for VPC-related AWS configurations
rg -A 5 "accountId|vpc.*aws" --type ts
# Check for any potential AWS VPC ID handling
ast-grep --pattern 'id: $_'
Length of output: 7275
18-27: Verify impact of optional subnets array.
The subnets array is now optional, which is a potentially breaking change. While the new fields (cidr, type, availabilityZone) are correctly modeled for AWS subnets, existing code might assume subnets are always present.
Let's verify the impact:
✅ Verification successful
Optional subnets array is safe to implement
Based on the codebase analysis, making the subnets array optional is safe because:
-
The only consumer of this schema is the AWS VPC scanner (
apps/event-worker/src/resource-scan/aws/vpc.ts), which:- Always provides subnets through the
getSubnetsfunction - Returns an empty array as fallback:
const { Subnets = [] } = await ec2Client.send(...)
- Always provides subnets through the
-
The GCP VPC scanner (
apps/event-worker/src/resource-scan/google/vpc.ts) safely handles subnets using optional chaining:network.subnetworks?.map(...)
The new optional fields (cidr, type, availabilityZone) align with the actual implementation in both AWS and GCP scanners.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for code that might assume subnets are required
# Search for direct subnet access patterns
rg -A 5 "subnets\." --type ts
# Look for array operations on subnets that might fail if undefined
ast-grep --pattern 'subnets.map($_)'
ast-grep --pattern 'subnets.forEach($_)'
ast-grep --pattern 'subnets.filter($_)'
Length of output: 155
Script:
#!/bin/bash
# Let's try a broader search to find any references to subnets
rg -i "subnet" --type ts -A 5
# Also search for potential array access patterns without the dot
rg "\[.*subnet" --type ts -A 2
# Look for variable declarations or assignments involving subnets
ast-grep --pattern 'const $_ = $$.subnets'
ast-grep --pattern 'let $_ = $$.subnets'
Length of output: 5094
apps/event-worker/src/resource-scan/aws/vpc.ts (3)
1-20: LGTM: Well-organized imports and logger setup
The imports are properly structured with clear type imports and specific AWS SDK components. The logger setup follows best practices with appropriate labeling.
82-87: LGTM: Clean implementation of region fetching
The function is concise and effectively uses promise chaining for error propagation.
147-166: LGTM: Well-structured multi-region scanning
The function effectively combines role assumption, region scanning, and result aggregation using lodash chain operations.
.../src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx
Outdated
Show resolved
Hide resolved
…urce-providers/integrations/aws/AwsDialog.tsx Co-authored-by: Justin Brooks <[email protected]> Signed-off-by: Zachary Blasczyk <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx (1)
204-244: LGTM: Well-structured form fields with clear UXThe new form fields for EKS and VPC import options are well-implemented with:
- Clear labels and descriptions
- Proper form control binding
- Consistent styling with the rest of the form
Consider adding
aria-labelattributes to the switches for better accessibility.<Switch checked={field.value} onCheckedChange={field.onChange} + aria-label={`Toggle ${field.name === "importEks" ? "EKS cluster" : "VPC"} import`} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
🔇 Additional comments (3)
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/aws/AwsDialog.tsx (3)
Line range hint 27-36: LGTM: Required imports added correctly
The new imports for FormDescription and Switch components are properly added and necessary for the new form fields.
52-53: LGTM: Schema updated with proper typing
The schema changes appropriately add support for EKS and VPC import toggles with sensible defaults.
93-95: LGTM: Properly implemented role ARN mapping
The implementation follows the suggested approach for handling AWS role ARNs.
| importEks: true, | ||
| importVpc: false, |
There was a problem hiding this comment.
Align form defaults with schema defaults
There's an inconsistency between the schema defaults and form defaults:
- Schema: both
importEksandimportVpcdefault tofalse - Form:
importEksdefaults totrue,importVpctofalse
This could lead to unexpected behavior. Consider aligning these values.
defaultValues: {
name: "",
awsRoleArns: [{ value: "" }],
- importEks: true,
+ importEks: false,
importVpc: false,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| importEks: true, | |
| importVpc: false, | |
| importEks: false, | |
| importVpc: false, |
Summary by CodeRabbit
Release Notes: AWS Resource Management Enhancement
New Features
Improvements
Documentation