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

Add account ID extraction and cleanup functionality in CSPM tests #600

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 29, 2025

PR Type

Enhancement, Tests


Description

  • Added functionality to extract AWS account ID from ARNs.

  • Implemented cleanup of existing AWS cloud accounts in CSPM tests.

  • Enabled nodeProfileService in cluster test configurations.

  • Updated system test mapping to skip CSPM tests in production-us environment.


Changes walkthrough 📝

Relevant files
Enhancement
aws.py
Add AWS account ID extraction function                                     

infrastructure/aws.py

  • Added a function extract_account_id to extract AWS account ID from
    ARNs.
  • Utilized regex for parsing ARNs.
  • +17/-1   
    cspm.py
    Add AWS account cleanup and CSPM enhancements                       

    tests_scripts/accounts/cspm.py

  • Added stack_manager initialization.
  • Implemented cleanup_existing_aws_cloud_accounts for AWS account
    cleanup.
  • Integrated account ID extraction and cleanup into CSPM tests.
  • Enhanced error handling in cloud account creation.
  • +42/-2   
    Tests
    clusters.py
    Enable nodeProfileService in cluster tests                             

    tests_scripts/accounts/clusters.py

    • Changed nodeProfileService capability from "disable" to "enable".
    +1/-1     
    Configuration changes
    system_test_mapping.json
    Update CSPM test environment configuration                             

    system_test_mapping.json

  • Updated CSPM test configuration to skip in production-us environment.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @kooomix kooomix merged commit bfc0aaf into master Jan 29, 2025
    3 checks passed
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The extract_account_id function does not handle the case where match is None, which could raise an AttributeError when calling group() on None

    match = re.search(r"arn:aws:iam::(\d+):", arn)
    return match.group(1) if match else None
    Potential Bug

    The stack_manager is initialized as None but used in cleanup() without checking if it was properly initialized elsewhere

    if self.stack_manager:
        self.stack_manager.delete_stack()

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add input validation for ARN

    Add error handling for when the ARN format is invalid or None. Currently, accessing
    group(1) on a None match will raise an AttributeError.

    infrastructure/aws.py [137-138]

    +if not arn:
    +    return None
     match = re.search(r"arn:aws:iam::(\d+):", arn)
     return match.group(1) if match else None
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential runtime error by adding null check for the ARN parameter, preventing AttributeError when accessing group(1) on None. This is a critical defensive programming practice.

    8
    Handle missing API response data

    Add error handling for the API response in cleanup_existing_aws_cloud_accounts. The
    code assumes 'response' key exists in res dictionary.

    tests_scripts/accounts/cspm.py [157-160]

    -if "response" in res:
    -    if len(res["response"]) == 0:
    -        Logger.logger.info(f"No existing aws cloud accounts to cleanup for account_id {account_id}")
    -        return
    +if not res or "response" not in res:
    +    Logger.logger.warning(f"Unexpected API response format for account_id {account_id}")
    +    return
    +if len(res["response"]) == 0:
    +    Logger.logger.info(f"No existing aws cloud accounts to cleanup for account_id {account_id}")
    +    return
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by checking for both None response and missing 'response' key, preventing potential KeyError exceptions and adding appropriate logging. This enhances the robustness of the cleanup process.

    7

    Copy link

    Failed to generate code suggestions for PR

    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.

    1 participant