add stop job for aws #96
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces multi-cloud support for the free-tier shutdown job, specifically extending functionality to include AWS EKS alongside existing GCP GKE support. The changes span multiple files in the backend jobs directory, adding AWS SDK dependencies, modifying repository methods to handle cloud provider-specific credentials, and updating schemas and deployment configurations to accommodate the new cloud provider context. Changes
Sequence DiagramsequenceDiagram
participant OmnistrateRepo as OmnistrateRepository
participant K8sRepo as K8sRepository
participant EKSClient as AWS EKS Client
participant GKEClient as GCP GKE Client
OmnistrateRepo->>OmnistrateRepo: Extract cloud provider
OmnistrateRepo->>K8sRepo: Call with cloud provider
alt Cloud Provider is AWS
K8sRepo->>EKSClient: Get AWS Credentials
else Cloud Provider is GCP
K8sRepo->>GKEClient: Get GCP Credentials
end
K8sRepo-->>OmnistrateRepo: Return Cluster Information
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
CI Feedback 🧐(Feedback updated until commit 8ea9627)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.ts (3)
32-85: Add robust error handling and consider EKS token import location.
- Validate edge cases for
axioscalls, STS, and EKS commands, to gracefully handle failures such as invalid tokens or insufficient AWS permissions.- Since
aws-eks-tokenis a direct dependency, a top-levelimportstatement could be more conventional thanrequire, unless lazy loading is intentional.
87-95: Allow easy extension for future cloud providers.Using a simple ternary for
'gcp'vs.'aws'is sufficient, but if you anticipate supporting more providers, consider a more extensible pattern or switch case for improved maintainability.
279-287: Use cloudProvider enum for type safety.Leveraging
cloudProvider: 'gcp' | 'aws'is cohesive, though referencing theCloudProvidersenum type directly might add clarity while aligning with the schema. This method’s logic is otherwise well-structured and consistent withgetFalkorDBLastQueryTime.backend/jobs/free-tier-shutdown/src/index.ts (1)
26-32: Consider adding specific error handling for cloud provider-specific failures.The function call looks good, but consider adding specific error handling for cloud provider-specific failures to improve debugging and error reporting.
- const lastUsedTime = await k8sRepo.getFalkorDBLastQueryTime( - cloudProvider, - clusterId, - region, - instanceId, - instance.tls, - ); + let lastUsedTime; + try { + lastUsedTime = await k8sRepo.getFalkorDBLastQueryTime( + cloudProvider, + clusterId, + region, + instanceId, + instance.tls, + ); + } catch (error) { + logger.error( + { error, cloudProvider, clusterId, region, instanceId }, + `Failed to get last query time for ${cloudProvider} instance`, + ); + throw error; + }backend/jobs/free-tier-shutdown/package.json (1)
22-23: Consider pinning AWS SDK versions.For better stability and predictability, consider pinning the AWS SDK versions instead of using caret ranges. This helps prevent unexpected breaking changes during deployments.
Apply this diff to pin the versions:
- "@aws-sdk/client-eks": "^3.735.0", - "@aws-sdk/client-sts": "^3.734.0", + "@aws-sdk/client-eks": "3.735.0", + "@aws-sdk/client-sts": "3.734.0", - "aws-eks-token": "^1.0.5", + "aws-eks-token": "1.0.5",Also applies to: 32-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
backend/jobs/free-tier-shutdown/cloudbuild.dev.yaml(2 hunks)backend/jobs/free-tier-shutdown/cloudbuild.prod.yaml(2 hunks)backend/jobs/free-tier-shutdown/package.json(2 hunks)backend/jobs/free-tier-shutdown/src/index.ts(1 hunks)backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.test.ts(1 hunks)backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.ts(4 hunks)backend/jobs/free-tier-shutdown/src/repositories/omnistrate/OmnistrateRepository.ts(2 hunks)backend/jobs/free-tier-shutdown/src/schemas/OmnistrateInstance.ts(2 hunks)
🔇 Additional comments (14)
backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.ts (6)
7-9: Nice addition of AWS and axios imports.Introducing STS/EKS clients and axios is appropriate for retrieving AWS credentials and cluster details. Make sure you have consistent error-handling strategies across all cloud clients (GCP and AWS) for better resilience.
22-22: Verify cluster naming assumptions.Appending
"c-"and removing dashes from the cluster ID is a specialized naming pattern. Ensure downstream references or naming constraints won't break if cluster IDs deviate from this pattern.
26-26: Good practice forcing HTTPS.Appending
https://to the cluster endpoint ensures secure communication with GKE. This is a sound approach for GCP clusters.
103-103: Confirm endpoint correctness for AWS clusters.For AWS-based clusters, verify that
k8sCredentials.endpointis valid. It might need"https://"prefix for the server field if the EKS endpoint lacks it.
109-109: Check if settingauthProviderto undefined is intended.When cloudProvider is
'aws',authProviderbecomesundefined. This is valid, but confirm that no custom auth provider is needed for AWS contexts or consider a descriptive name (e.g.,"aws-iam").
242-258: Clean integration of the new cloud provider parameter.The updated signature introducing
cloudProviderlooks consistent. The conditional logic to retrieve AWS or GCP credentials is straightforward, and fallback togetFalkorDBInfoupon no graphs found remains lexically clear.backend/jobs/free-tier-shutdown/src/schemas/OmnistrateInstance.ts (2)
3-6: Enum provides clarity for cloud provider selection.Defining
CloudProviderswith'gcp'and'aws'is straightforward and ensures maintainability and type safety. Consider adding doc comments if you plan to expand the enum in the future.
18-19: Great addition of a strict enum field to the schema.Using
Type.Enum(CloudProviders)ensures that thecloudProvidervalue is validated at runtime, reducing the risk of invalid inputs.backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.test.ts (1)
10-10: Test update aligns with new method signature.Including
'gcp'as the first argument matches the repository’s updated parameter order and ensures the test accurately reflects multi-cloud support.backend/jobs/free-tier-shutdown/cloudbuild.prod.yaml (2)
36-36: LGTM! Environment variable addition looks good.The AWS role ARN is properly configured as an environment variable, consistent with the existing pattern.
61-61: Verify AWS IAM role permissions.Please ensure that the AWS IAM role
FreeTierJobRolefollows the principle of least privilege and has only the necessary permissions to perform the free-tier shutdown operations.Run the following script to check the role's permissions:
✅ Verification successful
AWS IAM role implementation follows security best practices
The AWS role
FreeTierJobRoleis:
- Properly scoped for EKS cluster access only
- Uses secure OIDC federation with GCP service accounts
- Maintains separate roles for development and production environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check AWS IAM role permissions using the AWS CLI # Get role details and policies aws iam get-role --role-name FreeTierJobRole aws iam list-role-policies --role-name FreeTierJobRole aws iam list-attached-role-policies --role-name FreeTierJobRole # Get policy details for attached policies for policy in $(aws iam list-attached-role-policies --role-name FreeTierJobRole --query 'AttachedPolicies[*].PolicyArn' --output text); do echo "Policy: $policy" aws iam get-policy-version --policy-arn $policy --version-id $(aws iam get-policy --policy-arn $policy --query 'Policy.DefaultVersionId' --output text) doneLength of output: 667
Script:
#!/bin/bash # Search for AWS SDK usage and IAM-related files echo "=== Searching for AWS SDK imports ===" rg -l "require\(['\"](aws-sdk|@aws-sdk)" echo -e "\n=== Searching for AWS API calls ===" rg "aws-sdk.*\.(EC2|IAM|Organizations)" -A 5 echo -e "\n=== Looking for IAM role definitions ===" fd -e tf -e yaml -e json . | grep -i "iam\|role"Length of output: 411
Script:
#!/bin/bash echo "=== Checking free-tier-shutdown job files ===" fd . backend/jobs/free-tier-shutdown -t f -x cat {} echo -e "\n=== Searching for AWS configurations ===" rg -l "AWS" backend/jobs/free-tier-shutdown echo -e "\n=== Looking for environment variables ===" rg "AWS_" backend/jobs/free-tier-shutdownLength of output: 34049
backend/jobs/free-tier-shutdown/cloudbuild.dev.yaml (2)
36-36: LGTM! Environment variable addition looks good.The AWS role ARN is properly configured as an environment variable, consistent with the production configuration.
36-36: Verify configuration consistency across environments.The development configuration correctly mirrors the production setup with environment-specific values:
- Development AWS account: 637423310747
- Production AWS account: 148761665810
Please ensure that:
- The role permissions are identical across both environments
- The development role has been tested with the same operations that will be performed in production
Run the following script to compare role configurations:
Also applies to: 61-61
backend/jobs/free-tier-shutdown/package.json (1)
22-23: Verify AWS SDK version numbers.The specified versions (
3.735.0and3.734.0) appear to be incorrect as they are higher than the current latest AWS SDK versions. Please verify and update to the correct versions.
| @@ -20,10 +20,16 @@ async function handleFreeInstance( | |||
| logger.info(`Handling free instance ${instance.id}`); | |||
| try { | |||
| // 2. For each instance, get the last used time from k8s | |||
| const { clusterId, region, id: instanceId } = instance; | |||
| const { cloudProvider, clusterId, region, id: instanceId } = instance; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for cloudProvider value.
Consider validating the cloudProvider value against allowed values to prevent runtime errors.
- const { cloudProvider, clusterId, region, id: instanceId } = instance;
+ const { cloudProvider, clusterId, region, id: instanceId } = instance;
+ if (!['AWS', 'GCP'].includes(cloudProvider)) {
+ throw new Error(`Unsupported cloud provider: ${cloudProvider}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { cloudProvider, clusterId, region, id: instanceId } = instance; | |
| const { cloudProvider, clusterId, region, id: instanceId } = instance; | |
| if (!['AWS', 'GCP'].includes(cloudProvider)) { | |
| throw new Error(`Unsupported cloud provider: ${cloudProvider}`); | |
| } |
| @@ -96,6 +96,7 @@ export class OmnistrateRepository { | |||
| resourceId: Object.entries(d?.['consumptionResourceInstanceResult']?.['detailedNetworkTopology']).filter( | |||
| (ob) => (ob[1] as unknown)?.['main'], | |||
| )[0][0], | |||
| cloudProvider: d?.['consumptionResourceInstanceResult']?.['cloud_provider'], | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for cloudProvider field.
Consider adding type validation for the cloudProvider field to ensure type safety and prevent runtime errors.
+import { CloudProviders } from '../../schemas/OmnistrateInstance';
+
export class OmnistrateRepository {
// ... existing code ...
async getInstancesFromTier(
serviceId: string,
environmentId: string,
tierId: string,
): Promise<OmnistrateInstanceSchemaType[]> {
// ... existing code ...
return response.data['resourceInstances']
.map((d: unknown) => {
+ const cloudProvider = d?.['consumptionResourceInstanceResult']?.['cloud_provider'];
+ if (!Object.values(CloudProviders).includes(cloudProvider)) {
+ this._options.logger.warn(
+ { cloudProvider },
+ 'Unknown cloud provider, instance will be filtered out',
+ );
+ return null;
+ }
return ({
// ... other fields ...
- cloudProvider: d?.['consumptionResourceInstanceResult']?.['cloud_provider'],
+ cloudProvider,
});
})
+ .filter(Boolean)
.filter(
(instance) => instance.productTierId === tierId && instance.status === 'RUNNING',
) as OmnistrateInstanceSchemaType[];
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -84,7 +84,7 @@ | |||
| return response.data['resourceInstances'] | |||
| .map((d: unknown) => ({ | |||
| id: d?.['consumptionResourceInstanceResult']?.['id'], | |||
| clusterId: 'c-' + d?.['deploymentCellID']?.replace('-', ''), | |||
| clusterId: d?.['deploymentCellID'], | |||
There was a problem hiding this comment.
💡 Codebase verification
The clusterId format change breaks GCP cluster name construction
The change from concatenated string to raw deploymentCellID breaks the GCP cluster name construction in K8sRepository._getGKECredentials. The method expects to transform the clusterId by adding 'c-' prefix, but now receives raw deploymentCellID which results in incorrect cluster name format.
backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.ts: Update_getGKECredentialsto handle rawdeploymentCellIDformat.
🔗 Analysis chain
Verify the impact of clusterId format change.
The clusterId format has changed from a concatenated string to using the raw deploymentCellID. Please verify that this change doesn't break existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of clusterId to verify compatibility with the new format
# Search for clusterId usage patterns
rg -A 5 'clusterId'
# Search for specific GCP cluster ID patterns to ensure we haven't missed any legacy format dependencies
rg -A 5 'c-[a-zA-Z0-9-]+'
Length of output: 66332
dudizimber
deleted the
95-add-aws-support-for-the-free-tier-shutdown-job
branch
User description
fix #95
PR Type
Enhancement, Tests
Description
Added AWS EKS and STS client integration for Kubernetes credential retrieval in
K8sRepository.Introduced multi-cloud support by adding
cloudProviderparameter across various methods and schemas.Updated
OmnistrateRepositoryto includecloudProviderfield and consistentclusterIdformatting.Enhanced
handleFreeInstancelogic to handle cloud provider-specific operations.Added
CloudProvidersenum and schema field for defining supported cloud providers.Updated tests to validate multi-cloud support in
K8sRepository.Added AWS-related dependencies (
@aws-sdk/client-eks,@aws-sdk/client-sts,aws-eks-token) and updatedpnpm-lock.yaml.Updated Cloud Build configurations to include
AWS_ROLE_ARNfor AWS integration.Changes walkthrough 📝
4 files
K8sRepository.ts
Added AWS EKS support and multi-cloud handling in K8sRepository.backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.ts
credentials.
_getEKSCredentialsmethod to handle AWS-specific credentialfetching.
_getK8sConfigto support both GCP and AWS cloud providers.cloudProvideras a parameter for multi-cloudsupport.
OmnistrateRepository.ts
Added cloud provider field to OmnistrateRepository.backend/jobs/free-tier-shutdown/src/repositories/omnistrate/OmnistrateRepository.ts
cloudProviderfield to the resource instance mapping.clusterIdformatting logic for consistency.index.ts
Integrated cloud provider parameter in free instance handling.backend/jobs/free-tier-shutdown/src/index.ts
handleFreeInstanceto includecloudProviderwhen fetching lastused time.
cloudProviderfor multi-cloud support.OmnistrateInstance.ts
Added cloud provider enum and schema field.backend/jobs/free-tier-shutdown/src/schemas/OmnistrateInstance.ts
CloudProvidersenum for defining supported cloud providers.cloudProviderfield to theOmnistrateInstanceSchema.1 files
K8sRepository.test.ts
Updated K8sRepository test for multi-cloud support.backend/jobs/free-tier-shutdown/src/repositories/k8s/K8sRepository.test.ts
cloudProviderparameter forgetFalkorDBInfo.2 files
cloudbuild.dev.yaml
Updated dev Cloud Build configuration for AWS integration.backend/jobs/free-tier-shutdown/cloudbuild.dev.yaml
AWS_ROLE_ARNenvironment variable to the dev Cloud Buildconfiguration.
cloudbuild.prod.yaml
Updated prod Cloud Build configuration for AWS integration.backend/jobs/free-tier-shutdown/cloudbuild.prod.yaml
AWS_ROLE_ARNenvironment variable to the prod Cloud Buildconfiguration.
2 files
package.json
Added AWS SDK and EKS token dependencies.backend/jobs/free-tier-shutdown/package.json
@aws-sdk/client-eksand@aws-sdk/client-stsdependencies for AWSintegration.
aws-eks-tokendependency for generating EKS tokens.pnpm-lock.yaml
Added AWS SDK dependencies and utility libraries.backend/pnpm-lock.yaml
@aws-sdk/client-eksand@aws-sdk/client-sts.aws-eks-tokenandcrypto-js.libraries.
Summary by CodeRabbit
New Features
Dependencies
Configuration Updates
Schema Changes