Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/add aws #25

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Feat/add aws #25

merged 2 commits into from
Sep 30, 2024

Conversation

NichArchA82
Copy link
Collaborator

@NichArchA82 NichArchA82 commented Sep 29, 2024

PR Type

enhancement, dependencies


Description

  • Integrated AWS server management capabilities, including server creation, deletion, start, stop, and listing.
  • Enhanced VM command handling to support both AWS and Hetzner platforms.
  • Updated button registration to support regex for VM creation.
  • Added AWS SDK as a new dependency to manage AWS EC2 instances.
  • Refactored Hetzner utility imports for consistency.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
buttons.js
Update button registration for VM creation                             

command-handler/src/cmd-handler/buttons.js

  • Removed button_create_vm without regex.
  • Added button_create_vm with regex.
  • +5/-4     
    vm.js
    Integrate AWS server management and enhance VM commands   

    command-handler/src/commands/vm.js

  • Added AWS server management commands.
  • Updated Hetzner server commands.
  • Introduced platform selection for VM creation.
  • +82/-13 
    aws-server.js
    Add AWS server management utilities                                           

    command-handler/src/util/aws/aws-server.js

  • Implemented AWS server management functions.
  • Added server creation, deletion, start, stop, and list
    functionalities.
  • +397/-0 
    get-aws-images.js
    Implement AWS image retrieval function                                     

    command-handler/src/util/aws/get-aws-images.js

    • Added function to retrieve AWS images.
    +15/-0   
    get-instances.js
    Implement AWS instance description function                           

    command-handler/src/util/aws/get-instances.js

    • Added function to describe AWS instances.
    +37/-0   
    get-user-data.js
    Add user data configuration for cloud-init                             

    command-handler/src/util/get-user-data.js

    • Added function to configure user data for cloud-init.
    +11/-0   
    get-hetzner-images.js
    Refactor Hetzner image retrieval imports                                 

    command-handler/src/util/hetzner/get-hetzner-images.js

    • Refactored imports for logger and error handler.
    +2/-2     
    get-servers.js
    Refactor Hetzner server retrieval imports                               

    command-handler/src/util/hetzner/get-servers.js

    • Refactored imports for logger and error handler.
    +2/-2     
    hetzner-servers.js
    Enhance Hetzner server management and configuration           

    command-handler/src/util/hetzner/hetzner-servers.js

  • Updated server type and user data configuration.
  • Added chat message for listing servers.
  • +17/-18 
    Dependencies
    2 files
    package-lock.json
    Update package-lock for AWS SDK dependencies                         

    command-handler/package-lock.json

    • Added @aws-sdk/client-ec2 and its dependencies.
    +1315/-0
    package.json
    Add AWS SDK dependency                                                                     

    command-handler/package.json

    • Added @aws-sdk/client-ec2 to dependencies.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 Security concerns

    Hardcoded Security Group ID:
    In the file 'command-handler/src/util/aws/aws-server.js', a hardcoded security group ID (sg-0b5b0e1fb1f73dbba) is used when creating a new server. This could potentially expose the server to security risks if the security group is not properly configured or if it's accidentally made public. It's recommended to use a more secure method of specifying security groups, such as using environment variables or a configuration file.

    ⚡ Key issues to review

    Error Handling
    The createServer function has inconsistent error handling. Some errors are logged and reported to the user, while others are silently caught without proper logging or user notification.

    Security Concern
    The code is using a hardcoded security group ID (sg-0b5b0e1fb1f73dbba) when creating a new server. This could potentially expose the server to security risks if the security group is not properly configured.

    Potential Performance Issue
    The function retrieves all images owned by 'self' and then sorts and slices them. For accounts with many images, this could be inefficient. Consider adding filters to the API call to limit the results.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Remove unnecessary break statement to allow proper server state checking

    The createServer function contains a loop that breaks after the first iteration,
    regardless of the server state. This may lead to premature termination of the loop.
    Consider removing the break statement to allow the loop to continue checking the
    server state for the full number of attempts.

    command-handler/src/util/aws/aws-server.js [125-137]

     for (attempts = 1; attempts <= maxRetries; attempts++) {
         //wait 30 seconds
         await delay(1000 * 30);
     
         const server = await getInstance({ serverName });
     
         if (server[0].State.Name === 'running') {
    -    break;
    +        break;
         } else {
    -    log.info(`Attempt ${attempts} Failed. Backing off for 30 seconds`);
    +        log.info(`Attempt ${attempts} Failed. Backing off for 30 seconds`);
         }
    -    break;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical bug where the loop prematurely exits, preventing proper server state checking. Removing the unnecessary break statement ensures the loop functions as intended.

    9
    Possible issue
    Add a check for deviceId before attempting to delete from Tailscale

    In the deleteServer function, consider adding a check to ensure that deviceId is not
    undefined before attempting to delete the device from Tailscale. This will prevent
    potential errors if the device is not found in Tailscale.

    command-handler/src/util/aws/aws-server.js [205-212]

     //get servers from tailscale
     const { deviceId } = await getDevices(serverName)
     
    -//delete the device from tailscale
    -await axios.delete(`https://api.tailscale.com/api/v2/device/${deviceId}`, {
    -  headers: {
    -    'Authorization': `Bearer ${process.env.TAILSCALE_API_TOKEN}`
    -  }
    -})
    +if (deviceId) {
    +  //delete the device from tailscale
    +  await axios.delete(`https://api.tailscale.com/api/v2/device/${deviceId}`, {
    +    headers: {
    +      'Authorization': `Bearer ${process.env.TAILSCALE_API_TOKEN}`
    +    }
    +  })
    +} else {
    +  log.warn(`Device ${serverName} not found in Tailscale`);
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion prevents potential runtime errors by ensuring that the deviceId is valid before attempting deletion, improving the robustness of the deleteServer function.

    8
    Error handling
    Improve error handling by catching and logging specific AWS SDK errors

    Consider using a more specific error type for the catch block in the
    getSecurityGroups function. Instead of logging a generic error, you could catch and
    handle specific AWS SDK errors, providing more informative error messages.

    command-handler/src/util/aws/aws-server.js [43-45]

     } catch (err) {
    -    console.error(err);
    +    if (err.name === 'AccessDeniedException') {
    +        console.error('Access denied. Check your AWS credentials.');
    +    } else if (err.name === 'InvalidParameterException') {
    +        console.error('Invalid parameter in the request.');
    +    } else {
    +        console.error('An error occurred while fetching security groups:', err.message);
    +    }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances error handling by providing more informative messages for specific AWS SDK errors, which can aid in debugging and improve user feedback.

    7
    Best practice
    Use a specific version number for the AWS SDK EC2 client dependency

    Consider specifying a more precise version for the @aws-sdk/client-ec2 package
    instead of using the caret (^) notation. This can help ensure consistency across
    different environments and prevent potential issues caused by minor version updates.

    command-handler/package.json [14]

    -"@aws-sdk/client-ec2": "^3.658.1",
    +"@aws-sdk/client-ec2": "3.658.1",
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a specific version number instead of the caret (^) notation can help ensure consistency across different environments by preventing unexpected changes from minor version updates. This is a good practice for maintaining stability in dependencies.

    7
    Enhancement
    Limit and sort the displayed images to improve user experience

    In the selectImage function, consider adding pagination or limiting the number of
    images displayed to prevent overwhelming the user interface if there are many
    images. You could also add sorting to show the most recent or relevant images first.

    command-handler/src/util/aws/aws-server.js [374-396]

    -for (const image of images) {
    +const sortedImages = images.sort((a, b) => new Date(b.CreationDate) - new Date(a.CreationDate));
    +const limitedImages = sortedImages.slice(0, 5); // Limit to 5 most recent images
    +
    +for (const image of limitedImages) {
       app.client.chat.postEphemeral({
       channel: `${body.channel.id}`,
       user: `${body.user.id}`,
       blocks: [
           {
           "type": "actions",
           "elements": [
               {
               "type": "button",
               "text": {
                   "type": "plain_text",
    -              "text": `${image.Name}`
    +              "text": `${image.Name} (${new Date(image.CreationDate).toLocaleDateString()})`
               },
               "action_id": `button_create_image_aws_${image.Name}_${image.ImageId}`
               },
           ]
           }
       ],
       text: "Select an image:"
       });
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion improves user experience by limiting and sorting the images displayed, preventing UI overload and ensuring the most relevant images are shown first.

    6

    💡 Need additional feedback ? start a PR chat

    Copy link
    Contributor

    @venkatamutyala venkatamutyala left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Feel free to merge. And look at what i tagged in the next version

    @NichArchA82 NichArchA82 merged commit cf6b2c5 into main Sep 30, 2024
    4 checks passed
    @NichArchA82 NichArchA82 deleted the feat/add-aws branch September 30, 2024 06:07
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants