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

Cspm #595

Merged
merged 2 commits into from
Jan 28, 2025
Merged

Cspm #595

merged 2 commits into from
Jan 28, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 26, 2025

PR Type

Enhancement, Tests


Description

  • Introduced CSPM-specific functionality in backend API and tests.

  • Added new CSPM class for CSPM-related system tests.

  • Refactored existing Accounts tests to use CSPM.

  • Updated system test mapping to rename accounts to cspm.


Changes walkthrough 📝

Relevant files
Tests
accounts_tests.py
Refactor accounts tests to use CSPM                                           

configurations/system/tests_cases/accounts_tests.py

  • Renamed accounts method to cspm.
  • Updated import path to use cspm instead of accounts.
  • Added create_test_tenant parameter to KubescapeConfiguration.
  • +4/-3     
    cspm.py
    Add CSPM class for CSPM-specific tests                                     

    tests_scripts/accounts/cspm.py

  • Created new CSPM class inheriting from Accounts.
  • Added CSPM-specific test methods and logic.
  • Implemented CSPM link validation and cloud account management.
  • +260/-0 
    Enhancement
    backend_api.py
    Add CSPM API endpoints and methods                                             

    infrastructure/backend_api.py

  • Added API endpoints for CSPM link and feature deletion.
  • Implemented get_cspm_link and delete_accounts_feature methods.
  • Updated imports and removed unused TestUtil.
  • +24/-2   
    accounts.py
    Refactor Accounts class to remove CSPM logic                         

    tests_scripts/accounts/accounts.py

  • Removed CSPM-specific logic from Accounts class.
  • Simplified cloud account validation methods.
  • Updated Helm configuration parameters.
  • +29/-139
    Configuration changes
    system_test_mapping.json
    Update system test mapping for CSPM                                           

    system_test_mapping.json

  • Renamed accounts mapping to cspm.
  • Updated target and description fields accordingly.
  • +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.
  • 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 CSPM template URLs that could potentially expose sensitive infrastructure information. These should be moved to secure configuration management.

    ⚡ Recommended focus areas for review

    Hardcoded Values

    Several hardcoded values like ARN, CSPM template URL and region should be moved to environment variables or configuration files for better maintainability and flexibility

    # a generated good arn from Eran aws dev account - consider moving to an env var?
    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 create_and_validate_cloud_account_with_cspm method catches all exceptions generically without proper error handling or logging of the specific error

    try:
        res = self.backend.create_cloud_account(body=body, provider=provider)
    except Exception as e:
        failed = True
    Input Validation

    The delete_accounts_feature method accepts parameters without validation of account_guid and feature_name

    def delete_accounts_feature(self, account_guid, feature_name):
        url = API_ACCOUNTS_DELETE_FEATURE
        params = {"customerGUID": self.selected_tenant_id}
        body = {
            "guid": account_guid,
            "featureName": feature_name
        }
        r = self.delete(url, params=params, json=body)
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error deleting account feature. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add response validation for API call

    Add error handling and validation for empty or malformed response from get_cspm_link
    API call. The current code assumes response will always be valid.

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

     res = self.backend.get_cspm_link(region=region)
    +if not res or not isinstance(res, str):
    +    raise ValueError(f"Invalid response from get_cspm_link: {res}")
     expected_link = f"https://{region}.console.aws.amazon.com/cloudformation/home?region={region}#/stacks/quickcreate?param_AccountID={tenant}\u0026stackName=create-armo-scan-user\u0026templateUrl={parsed_cspm_template}"
     assert res == expected_link,  f"failed to get cspm link, link is {res}, expected link is {expected_link}"
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important error handling for the API response, which could prevent cryptic errors if the API returns unexpected data. This improves the robustness and reliability of the test.

    7
    Validate region parameter before API call

    Add validation for region parameter to ensure it's a valid AWS region before making
    the API call.

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

     def get_and_validate_cspm_link(self, region): 
         """
         Get and validate cspm link.
         """
    +    if not region or not isinstance(region, str):
    +        raise ValueError(f"Invalid region parameter: {region}")
         tenant = self.backend.get_selected_tenant()
         parsed_cspm_template = quote(CSPM_TEMPLATE_URL, safe='')
         res = self.backend.get_cspm_link(region=region)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding input validation for the region parameter helps catch potential issues early and prevents invalid API calls. This improves the test's reliability and error reporting.

    6

    Copy link

    Failed to generate code suggestions for PR

    @@ -138,6 +136,8 @@ class NotExistingCustomer(Exception):
    API_ACCOUNTS_CLOUD_LIST = "/api/v1/accounts/cloud/list"
    API_ACCOUNTS_KUBERNETES_LIST = "/api/v1/accounts/kubernetes/list"
    API_ACCOUNTS_AWS_REGIONS = "/api/v1/accounts/aws/regions"
    API_ACCOUNTS_CSPM_LINK = "/api/v1/accounts/aws/cspmstack"
    API_ACCOUNTS_DELETE_FEATURE = "/api/v1/accounts/feature"
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    you dont use if for more than just delete?

    @kooomix kooomix merged commit 4b31950 into master Jan 28, 2025
    3 checks passed
    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