feat: Google VPC Scanner last mile #276
Conversation
|
Warning Rate limit exceeded@zacharyblasczyk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces enhancements to the Google Cloud resource scanning capabilities, focusing on expanding the Compute Scanner's functionality. The changes include adding support for importing new resource types like Namespaces, VClusters, and Google Virtual Private Cloud (VPC). Additionally, the VPC resource scanning module is updated with detailed subnet information, and the user interface is modified to allow configuration of VPC imports across multiple components of the application. Changes
Sequence DiagramsequenceDiagram
participant User
participant WebUI
participant ResourceScanner
participant CloudProvider
User->>WebUI: Configure Google Cloud Provider
WebUI->>User: Present VPC Import Option
User->>WebUI: Enable VPC Import
WebUI->>ResourceScanner: Submit Configuration
ResourceScanner->>CloudProvider: Fetch VPC Resources
CloudProvider-->>ResourceScanner: Return VPC and Subnet Details
ResourceScanner->>ResourceScanner: Process and Store 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: 1
🧹 Nitpick comments (1)
apps/event-worker/src/resource-scan/google/vpc.ts (1)
78-129: General approach is correct, focus on potential performance improvement.
The logic to map and gather subnets asynchronously looks correct. However, in lines 115-125, there's a performance hint from static analysis regarding spreading accumulators:...network.peerings?.reduce( (acc, peering) => ({ - ...acc, [`google/peering/${peering.name}`]: JSON.stringify({ network: peering.network, state: peering.state, autoCreateRoutes: peering.autoCreateRoutes, }), }), {}, )Using the spread operator in a reduce callback can degrade performance for large arrays. Consider mutating the accumulator object directly:
🧰 Tools
🪛 Biome (1.9.4)
[error] 117-117: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/docs/pages/integrations/google-cloud/compute-scanner.mdx(1 hunks)apps/event-worker/src/resource-scan/google/index.ts(2 hunks)apps/event-worker/src/resource-scan/google/vpc.ts(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/google/GoogleDialog.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/google/UpdateGoogleProviderDialog.tsx(2 hunks)packages/db/drizzle/0051_fat_iron_lad.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)
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0051_fat_iron_lad.sql
🧰 Additional context used
📓 Path-based instructions (6)
apps/event-worker/src/resource-scan/google/index.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/google/UpdateGoogleProviderDialog.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/resource-provider.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/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/google/GoogleDialog.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/google/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/google/vpc.ts
[error] 117-117: 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 (18)
apps/event-worker/src/resource-scan/google/vpc.ts (6)
8-8: Imports look good.
No issues found with these newly introduced import statements.
Also applies to: 10-10
20-28: Subnet details type aligns well with potential usage.
This definition sets a clear structure for the Google subnet metadata, and it appears consistent with the expanded schema in the validators.
29-41: Async client creation is well-structured.
Returning both networksClient and subnetsClient in a single object is a neat approach for cohesive usage.
Line range hint 132-164: Pagination logic is clear and robust.
Fetching resources in a loop with pageToken is handled properly. No issues found.
165-172: Permission error handling is well-defined.
The error logging provides sufficient context on missing privileges.
191-200: Conditional VPC import logic is correctly implemented.
This respects user configuration and keeps the code maintainable.
apps/event-worker/src/resource-scan/google/index.ts (3)
5-5: New import for VPC resources.
Straightforward addition to bring in the new VPC functionality.
18-18: Descriptive variable naming.
Renaming to gkeResources clarifies the nature of the returned data.
23-28: Combining VPC and GKE resources.
Returning both helps ensure thorough Google resource coverage in a single pass.
packages/validators/src/resources/cloud-v1.ts (1)
23-35: Extended enum and new fields for GCP subnets.
The added “internal”/“external” types, gatewayAddress, and secondaryCidrs fields extend coverage for GCP.
packages/db/src/schema/resource-provider.ts (2)
54-54: New boolean importVpc for Google providers.
Setting default to false is a sensible choice.
Line range hint 69-69: New boolean importVpc for AWS providers.
Similar to Google providers, this supports multi-cloud VPC imports.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/google/GoogleDialog.tsx (3)
46-46: Added importVpc field in validation schema.
Ensures form data verifies this new boolean field.
60-60: Default value set to false.
Good practice to disable new functionalities by default to avoid unexpected resource imports.
259-276: Form integration for importVpc.
The new switch field parallels existing toggles (GKE, namespaces, vClusters). Implementation is consistent and user-friendly.
apps/webservice/src/app/[workspaceSlug]/(app)/(resources)/resource-providers/integrations/google/UpdateGoogleProviderDialog.tsx (1)
289-306: Addition of importVpc field is well-structured and consistent.
This new form field aligns well with your overall approach, using a Switch for toggling. The naming is consistent with the rest of the file, and validation appears covered within the same schema. No further changes needed.
apps/docs/pages/integrations/google-cloud/compute-scanner.mdx (1)
9-11: Documentation update aligns with new import features.
The added entries for Namespaces, vClusters, and VPCs are consistent with the code updates. The language is clear and matches the newly introduced functionality.
packages/db/drizzle/meta/_journal.json (1)
361-367: New journal entry correctly logged.
The newly added entry for versioning and migration traces is correct and follows the existing structure. No immediate issues noted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/event-worker/src/resource-scan/google/vpc.ts (1)
43-76:⚠️ Potential issueHandle the case when subnet is not found.
The current implementation may throw runtime errors if the subnet is not found. This issue was previously identified but hasn't been addressed.
Consider this safer implementation:
const getSubnetDetails = ( subnetsClient: SubnetworksClient, project: string, subnetSelfLink: string, ): Promise<GoogleSubnetDetails | null> => { const parts = subnetSelfLink.split("/"); const region = parts.at(-3) ?? ""; const name = parts.at(-1) ?? ""; return subnetsClient .list({ project, region, filter: `name eq ${name}`, }) .then(([subnets]) => { - _.chain(subnets) - .find((subnet) => subnet.name === name) - .thru( - (subnet): GoogleSubnetDetails => ({ + const subnet = subnets.find((s) => s.name === name); + return subnet + ? { name, region, gatewayAddress: subnet.gatewayAddress ?? "", cidr: subnet.ipCidrRange ?? "", type: subnet.purpose === "INTERNAL" ? "internal" : "external", secondaryCidrs: subnet.secondaryIpRanges?.map((r) => ({ name: r.rangeName ?? "", cidr: r.ipCidrRange ?? "", })), - }), - ) - .value(), + } + : null; }); };
🧹 Nitpick comments (2)
apps/event-worker/src/resource-scan/google/vpc.ts (2)
29-41: Consider enhancing error handling for client initialization.While the implementation is clean, consider handling potential initialization failures more gracefully:
const getNetworksClient = async (targetPrincipal?: string | null) => { + try { const [networksClient] = await getGoogleClient( NetworksClient, targetPrincipal, "Networks Client", ); const [subnetsClient] = await getGoogleClient( SubnetworksClient, targetPrincipal, "Subnets Client", ); return { networksClient, subnetsClient }; + } catch (error) { + log.error("Failed to initialize Google Cloud clients", { error, targetPrincipal }); + throw new Error("Failed to initialize Google Cloud clients"); + } };
188-197: Consider enhancing logging for better observability.While the implementation is solid, consider adding more detailed logging for better debugging and monitoring.
const clients = await getNetworksClient(googleServiceAccountEmail); + log.debug("Successfully initialized Google Cloud clients"); const resources: InsertResource[] = config.importVpc ? await _.chain(config.projectIds) .map((id) => fetchProjectNetworks(clients, id, workspaceId, resourceProviderId), ) .thru((promises) => Promise.all(promises)) .value() .then((results) => results.flat()) : []; - log.info(`Found ${resources.length} VPC resources`); + log.info(`Found ${resources.length} VPC resources`, { + projectCount: config.projectIds.length, + resourceCount: resources.length, + workspaceId, + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/resource-scan/google/vpc.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/event-worker/src/resource-scan/google/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/google/vpc.ts
[error] 117-117: 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 (2)
apps/event-worker/src/resource-scan/google/vpc.ts (2)
20-27: LGTM! Well-structured type definition.
The GoogleSubnetDetails type is comprehensive and properly typed with all necessary fields for subnet information.
Line range hint 131-173: LGTM! Well-implemented pagination and error handling.
The implementation properly handles:
- Pagination with page tokens
- Specific permission error detection
- Informative error messages with role requirements
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/event-worker/src/resource-scan/google/vpc.ts (3)
29-41: Consider enhancing error handlingWhile the implementation is clean, consider adding explicit error handling for scenarios where client initialization fails. This would help with debugging and provide clearer error messages.
const getNetworksClient = async (targetPrincipal?: string | null) => { + try { const [networksClient] = await getGoogleClient( NetworksClient, targetPrincipal, "Networks Client", ); const [subnetsClient] = await getGoogleClient( SubnetworksClient, targetPrincipal, "Subnets Client", ); return { networksClient, subnetsClient }; + } catch (error) { + log.error("Failed to initialize Google Cloud clients", { error, targetPrincipal }); + throw new Error("Failed to initialize Google Cloud clients: " + (error as Error).message); + } };
161-164: Enhance error message with specific IAM role requirementsWhile the error message mentions the "Compute Network Viewer" role, it would be helpful to include the specific IAM role ID and a link to the documentation.
- ? 'Missing required permissions. Please ensure the service account has the "Compute Network Viewer" role.' + ? 'Missing required permissions. Please ensure the service account has the "Compute Network Viewer" role (roles/compute.networkViewer). See: https://cloud.google.com/compute/docs/access/iam#compute.networkViewer'
185-194: Add detailed logging for better observabilityConsider adding more detailed logging to track the progress of VPC scanning across multiple projects.
const clients = await getNetworksClient(googleServiceAccountEmail); + log.debug("Successfully initialized Google Cloud clients"); const resources: InsertResource[] = config.importVpc ? await _.chain(config.projectIds) .map((id) => - fetchProjectNetworks(clients, id, workspaceId, resourceProviderId), + fetchProjectNetworks(clients, id, workspaceId, resourceProviderId) + .then((networks) => { + log.debug(`Fetched ${networks.length} networks from project ${id}`); + return networks; + }), ) .thru((promises) => Promise.all(promises)) .value() .then((results) => results.flat()) : [];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/event-worker/src/resource-scan/google/vpc.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/event-worker/src/resource-scan/google/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.
🔇 Additional comments (3)
apps/event-worker/src/resource-scan/google/vpc.ts (3)
20-27: LGTM: Well-structured type definition
The GoogleSubnetDetails type is well-defined with clear field types and optional fields properly marked.
113-123: LGTM: Performance optimization implemented
The reducer implementation has been optimized as suggested in the previous review, avoiding the spread operator for better performance.
48-50: Handle edge cases in self-link parsing
The current implementation assumes a specific format for subnetSelfLink. Consider adding validation for the URL format and handling malformed links.
- const parts = subnetSelfLink.split("/");
- const region = parts.at(-3) ?? "";
- const name = parts.at(-1) ?? "";
+ if (!subnetSelfLink.includes("/regions/")) {
+ log.warn("Invalid subnet self-link format", { subnetSelfLink });
+ return null;
+ }
+ const parts = subnetSelfLink.split("/");
+ const region = parts.at(-3);
+ const name = parts.at(-1);
+ if (!region || !name) {
+ log.warn("Could not extract region or name from self-link", { subnetSelfLink });
+ return null;
+ }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/validators/src/resources/cloud-v1.ts (1)
3-20: LGTM! Consider adding JSDoc comments.The subnet schema is well-structured with clear provider-specific fields. The type field correctly accommodates both AWS ("public"/"private") and GCP ("internal"/"external") values.
Consider adding JSDoc comments to document the schema, especially to clarify the provider-specific fields:
/** * Schema for subnet resources across cloud providers. * @property {string} name - The name of the subnet * @property {string} region - The region where the subnet is located * @property {string} [cidr] - The CIDR range of the subnet * @property {("public"|"private"|"internal"|"external")} [type] - The type of subnet * - "public"/"private": Used for AWS subnets * - "internal"/"external": Used for GCP subnets * @property {string} [gatewayAddress] - The gateway IP address (GCP only) * @property {string} [availabilityZone] - The availability zone * @property {Array<{name: string, cidr: string}>} [secondaryCidrs] - Secondary CIDR ranges (GCP only) */ const subnet = z.object({ // ... existing schema });apps/event-worker/src/resource-scan/google/vpc.ts (2)
23-35: Consider using array destructuring for cleaner code.The client initialization can be simplified using array destructuring.
const getNetworksClient = async (targetPrincipal?: string | null) => { - const [networksClient] = await getGoogleClient( + const [[networksClient], [subnetsClient]] = await Promise.all([ + getGoogleClient( NetworksClient, targetPrincipal, "Networks Client", - ); - const [subnetsClient] = await getGoogleClient( + ), + getGoogleClient( SubnetworksClient, targetPrincipal, "Subnets Client", - ); + ), + ]); return { networksClient, subnetsClient }; };
154-165: Consider using a custom error type for better error handling.While the current error handling is good, consider creating a custom error type for better type safety and handling of Google API errors.
type GoogleApiError = { message?: string; code?: number; errors?: Array<{ message: string; domain: string; reason: string }>; }; // Usage: } catch (err) { const error = err as GoogleApiError; const errorMessage = error.code === 403 || error.message?.includes("PERMISSION_DENIED") ? 'Missing required permissions. Please ensure the service account has the "Compute Network Viewer" role.' : (error.errors?.[0]?.message ?? error.message ?? "Unknown error"); // ... rest of the error handling }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/resource-scan/google/vpc.ts(4 hunks)packages/validators/src/resources/cloud-v1.ts(3 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/google/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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build
- GitHub Check: Lint
🔇 Additional comments (5)
packages/validators/src/resources/cloud-v1.ts (2)
36-36: LGTM! Good use of schema reusability.The update to use the new
subnetschema improves code maintainability and ensures consistent subnet validation across the codebase.
45-45: LGTM! Type export enhances type safety.The exported
CloudSubnetV1type ensures type safety by maintaining alignment with the schema definition.apps/event-worker/src/resource-scan/google/vpc.ts (3)
37-68: LGTM! Good error handling implementation.The function properly handles the case when a subnet is not found and uses appropriate null coalescing for optional fields.
70-122: LGTM! Good performance optimizations.The implementation shows good practices:
- Parallel processing with Promise.all
- Efficient metadata handling with Object.fromEntries
- Proper null checks and optional chaining
180-189: LGTM! Good use of lodash chain for readability.The implementation shows good practices:
- Readable data transformation using lodash chain
- Proper handling of the importVpc flag
- Appropriate logging
Summary by CodeRabbit
New Features
Documentation
Enhancements