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

Accounts tests #596

Merged
merged 4 commits into from
Jan 28, 2025
Merged

Accounts tests #596

merged 4 commits into from
Jan 28, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 28, 2025

PR Type

Enhancement, Tests


Description

  • Introduced CSPM and Clusters functionalities with dedicated classes and methods.

    • Added CSPM class for cloud security posture management tests.
    • Added Clusters class for Kubernetes cluster-related tests.
  • Refactored AccountsTests to integrate new CSPM and Clusters functionalities.

  • Added AWS CloudFormation management utility for stack operations.

  • Enhanced backend API with CSPM link retrieval and feature deletion methods.


Changes walkthrough 📝

Relevant files
Enhancement
accounts_tests.py
Refactor AccountsTests to include CSPM and Clusters           

configurations/system/tests_cases/accounts_tests.py

  • Renamed accounts method to cspm and added clusters method.
  • Updated test object mappings for CSPM and Clusters.
  • +11/-3   
    aws.py
    Add AWS CloudFormation management utility                               

    infrastructure/aws.py

  • Added CloudFormationManager class for AWS stack operations.
  • Implemented methods for stack creation, deletion, and output
    retrieval.
  • +123/-0 
    backend_api.py
    Enhance backend API with CSPM and feature management         

    infrastructure/backend_api.py

  • Added methods for retrieving CSPM links and deleting account features.
  • Introduced new API endpoints for CSPM and feature management.
  • +24/-2   
    accounts.py
    Simplify and refactor Accounts class                                         

    tests_scripts/accounts/accounts.py

  • Refactored Accounts class to simplify and remove redundant methods.
  • Updated base class to BaseTest and removed Helm-specific logic.
  • +29/-213
    Tests
    clusters.py
    Introduce Clusters class for Kubernetes tests                       

    tests_scripts/accounts/clusters.py

  • Added Clusters class for Kubernetes cluster-related tests.
  • Implemented methods for validating Kubernetes accounts and unique
    values.
  • +113/-0 
    cspm.py
    Add CSPM class for cloud security posture tests                   

    tests_scripts/accounts/cspm.py

  • Added CSPM class for cloud security posture management tests.
  • Implemented methods for CSPM link validation and cloud account
    management.
  • +240/-0 
    Configuration changes
    system_test_mapping.json
    Update system test mappings for CSPM and Clusters               

    system_test_mapping.json

  • Updated test mappings to include CSPM and Clusters functionalities.
  • Adjusted descriptions and targets for new test cases.
  • +18/-6   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The code contains hardcoded AWS ARN values and template URLs in tests_scripts/accounts/cspm.py. While these appear to be test values, they should be moved to environment variables or secure configuration management to prevent potential exposure of production resources.

    ⚡ Recommended focus areas for review

    Security Configuration

    The CloudFormationManager uses AWS credentials and permissions to create/delete stacks. Validate that proper IAM role restrictions and least privilege access are implemented.

    def __init__(self, url, aws_access_key_id=None, aws_secret_access_key=None, aws_session_token=None):
        self.stack_name, self.template_url, self.region, self.parameters = self.extract_parameters_from_url(url)
        self.cloudformation = boto3.client(
            "cloudformation",
            region_name=self.region,
            aws_access_key_id=aws_access_key_id,
            aws_secret_access_key=aws_secret_access_key,
            aws_session_token=aws_session_token
        )
    Hardcoded Values

    The code contains hardcoded ARN values and template URLs that should be moved to environment variables or configuration files for better maintainability and security.

    GOOD_ARN = "arn:aws:iam::015253967648:role/armo-scan-role-015253967648"
    
    # cspm template url - consider moving to an env var?
    CSPM_TEMPLATE_URL = "https://armo-scan-user-stack.s3.us-east-1.amazonaws.com/cspm-template-dev.yaml"
    Error Handling

    The AWS stack creation and deletion operations could benefit from more robust error handling and cleanup in case of failures.

    stack_id =  self.stack_manager.create_stack(stack_name=test_stack_name)
    assert stack_id, f"failed to create stack for cspm, stack name is {test_stack_name}"
    Logger.logger.info(f"Stack creation initiated for: {test_stack_name}, stack id is {stack_id}")
    
    self.stack_manager.wait_for_stack_creation()
    test_arn =  self.stack_manager.get_stack_output_role_arn()
    assert test_arn, f"failed to get stack output arn for cspm, stack name is {test_stack_name}"
    Logger.logger.info(f"Got stack output arn for cspm, arn is {test_arn}")

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure resource cleanup on failure

    Add cleanup of the CloudFormation stack even if test fails. Currently, if test fails
    before reaching cleanup(), the stack remains in AWS.

    tests_scripts/accounts/cspm.py [124-126]

     def cleanup(self, **kwargs):
    -    self.stack_manager.delete_stack()
    -    return super().cleanup(**kwargs)
    +    try:
    +        if hasattr(self, 'stack_manager'):
    +            self.stack_manager.delete_stack()
    +    except Exception as e:
    +        Logger.logger.error(f"Failed to delete stack during cleanup: {str(e)}")
    +    finally:
    +        return super().cleanup(**kwargs)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion prevents resource leaks by ensuring AWS stack cleanup occurs even when tests fail, which is crucial for maintaining clean test environments.

    9
    Add timeout handling for operations

    Add timeout handling in the stack creation waiter to prevent indefinite waiting.
    Currently, if the stack creation gets stuck, the code will keep waiting until max
    attempts is reached.

    infrastructure/aws.py [69-73]

    -waiter.wait(StackName=self.stack_name,
    -            WaiterConfig={
    -                "Delay": delay,  # Polling interval in seconds
    -                "MaxAttempts": max_attempts  # Maximum number of attempts (total time = Delay * MaxAttempts)
    -            })
    +try:
    +    waiter.wait(StackName=self.stack_name,
    +                WaiterConfig={
    +                    "Delay": delay,
    +                    "MaxAttempts": max_attempts
    +                })
    +except boto3.exceptions.WaiterError as e:
    +    Logger.logger.error(f"Stack creation timed out after {delay * max_attempts} seconds")
    +    raise TimeoutError(f"Stack creation for {self.stack_name} timed out") from e
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds critical error handling for stack creation timeouts, preventing potential hanging operations and providing better error reporting.

    8
    General
    Improve error handling specificity

    Add error handling for the case when required parameters are missing in the URL
    query string. Currently, the code raises a generic ValueError which may not provide
    enough context for debugging.

    infrastructure/aws.py [41-42]

    -if not stack_name or not template_url or not region:
    -    raise ValueError("The URL does not contain the required parameters 'stackName', 'templateUrl', or 'region'.")
    +missing_params = []
    +if not stack_name:
    +    missing_params.append("stackName")
    +if not template_url:
    +    missing_params.append("templateUrl")
    +if not region:
    +    missing_params.append("region")
    +if missing_params:
    +    raise ValueError(f"Missing required URL parameters: {', '.join(missing_params)}")
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion improves error message clarity by specifically identifying which required parameters are missing, making debugging easier for users.

    6

    Copy link

    Failed to generate code suggestions for PR

    @kooomix kooomix merged commit a20c242 into master Jan 28, 2025
    3 checks passed
    @kooomix kooomix deleted the accountstests branch February 3, 2025 12:25
    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