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 - cloud and posture #546

Merged
merged 3 commits into from
Jan 2, 2025
Merged

Accounts - cloud and posture #546

merged 3 commits into from
Jan 2, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Dec 30, 2024

PR Type

Enhancement, Tests


Description

  • Added comprehensive accounts management functionality with new API endpoints for cloud and Kubernetes accounts
  • Implemented new test suite (AccountsTests) with extensive test coverage:
    • Cloud accounts CRUD operations
    • Kubernetes accounts validation
    • AWS regions validation
    • Unique values endpoints testing
  • Added API endpoints for:
    • Cloud accounts list/create/update/delete
    • Kubernetes accounts list
    • Unique values retrieval
    • AWS regions
  • Integrated accounts testing into the system test framework

Changes walkthrough 📝

Relevant files
Enhancement
tests.py
Add AccountsTests to system test suite                                     

configurations/system/tests.py

  • Added AccountsTests to the test suite
  • Integrated AccountsTests into test discovery and execution
  • +4/-0     
    backend_api.py
    Add accounts management API endpoints and methods               

    infrastructure/backend_api.py

  • Added new API endpoints for accounts management
  • Implemented methods for cloud and Kubernetes accounts operations
  • Added methods for unique values retrieval
  • Added AWS regions endpoint
  • +113/-0 
    Tests
    accounts_tests.py
    Create AccountsTests test class                                                   

    configurations/system/tests_cases/accounts_tests.py

  • Created new test class AccountsTests
  • Added accounts() test method that returns KubescapeConfiguration
  • +14/-0   
    accounts.py
    Implement accounts testing scenarios                                         

    tests_scripts/accounts/accounts.py

  • Created Accounts test class with comprehensive test scenarios
  • Implemented cloud accounts CRUD operations testing
  • Added Kubernetes accounts validation
  • Added AWS regions validation
  • +309/-0 
    Configuration changes
    system_test_mapping.json
    Add accounts test mapping configuration                                   

    system_test_mapping.json

  • Added accounts test configuration
  • Specified target repositories and owner
  • +13/-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 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The code contains a hard-coded AWS ARN (line 13 in accounts.py) which could potentially expose account information. While it's noted as a test account, it's better to move such values to secure configuration management.

    ⚡ Recommended focus areas for review

    Error Handling

    The cloud account creation error handling could be improved. Currently it only checks if an exception was raised but doesn't validate the specific error message or type.

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

    The error handling code is duplicated across multiple account-related methods. Consider extracting common error handling logic into a helper method.

        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error accessing cloud accounts. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    def get_kubernetes_accounts(self,  body=None, **kwargs):
        url = API_ACCOUNTS_KUBERNETES_LIST
        if body is None:
            body = {"pageSize": 150, "pageNum": 1}
    
        params = {"customerGUID": self.selected_tenant_id}
        if kwargs:
            params.update(**kwargs)
        r = self.post(url, params=params, json=body)
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error accessing cloud accounts. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    
    def get_cloud_accounts_uniquevalues(self, body):
        params = {"customerGUID": self.selected_tenant_id}
    
        Logger.logger.info("get_cloud_accounts_uniquevalues body: %s" % body)
    
        r = self.post(API_UNIQUEVALUES_ACCOUNTS_CLOUD, params=params, json=body)
    
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error accessing dashboard. Request: get_cloud_accounts_uniquevalues "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    def get_kubernetes_accounts_uniquevalues(self, body):
        params = {"customerGUID": self.selected_tenant_id}
    
        Logger.logger.info("get_kubernetes_accounts_uniquevalues body: %s" % body)
    
        r = self.post(API_UNIQUEVALUES_ACCOUNTS_KUBERNETES, params=params, json=body)
    
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error accessing dashboard. Request: get_kubernetes_accounts_uniquevalues "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    def create_cloud_account(self, body, provider):
        url = API_ACCOUNTS
        params = {"customerGUID": self.selected_tenant_id,
                  "provider": provider}
        r = self.post(url, params=params, json=body)
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error creating cloud account. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    def delete_cloud_account(self, guid):
        url = API_ACCOUNTS
        params = {"customerGUID": self.selected_tenant_id}
        body = {
            "innerFilters": [
                {
                    "guid": guid
                }
            ]
        }
        r = self.delete(url, params=params, json=body)
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error deleting cloud account. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    
    
    def update_cloud_account(self, body, provider):
        url = API_ACCOUNTS
        params = {"customerGUID": self.selected_tenant_id,
                  "provider": provider}
        r = self.put(url, params=params, json=body)
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error updating cloud account. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    
    
    def get_aws_regions(self):
        url = API_ACCOUNTS_AWS_REGIONS
        r = self.get(url, params={"customerGUID": self.selected_tenant_id})
        if not 200 <= r.status_code < 300:
            raise Exception(
                'Error accessing AWS regions. Customer: "%s" (code: %d, message: %s)' % (
                    self.customer, r.status_code, r.text))
        return r.json()
    Hard-coded Values

    The test uses hard-coded AWS ARN values and regions. Consider moving these to configuration files or environment variables for better maintainability.

    GOOD_ARN = "arn:aws:iam::015253967648:role/armo-scan-role-cross-with_customer-015253967648"

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Move sensitive credentials from hardcoded values to environment variables

    The test is using hardcoded AWS ARN values which should be moved to environment
    variables or configuration files for better security and maintainability.

    tests_scripts/accounts/accounts.py [13]

    -GOOD_ARN = "arn:aws:iam::015253967648:role/armo-scan-role-cross-with_customer-015253967648"
    +GOOD_ARN = os.getenv('AWS_TEST_ARN', '')  # ARN should be provided via environment variable
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Hardcoding AWS ARN credentials in source code is a security risk. Moving them to environment variables follows security best practices and improves configuration flexibility.

    9
    Possible issue
    Add error handling to cleanup process to ensure all resources are properly handled

    The cleanup method should handle exceptions when deleting cloud accounts to ensure
    all cleanup attempts are made even if one fails.

    tests_scripts/accounts/accounts.py [121-125]

     def cleanup(self, **kwargs):
         for guid in self.test_cloud_accounts_guids:
    -        self.backend.delete_cloud_account(guid=guid)
    -        Logger.logger.info(f"Deleted cloud account with guid {guid}")
    +        try:
    +            self.backend.delete_cloud_account(guid=guid)
    +            Logger.logger.info(f"Deleted cloud account with guid {guid}")
    +        except Exception as e:
    +            Logger.logger.error(f"Failed to delete cloud account {guid}: {str(e)}")
         return super().cleanup(**kwargs)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The cleanup method lacks error handling, which could leave resources undeleted if one deletion fails. Adding try-catch ensures more robust cleanup and better error reporting.

    8
    General
    Fix incorrect error message text to accurately reflect the API being called

    The error message in get_kubernetes_accounts() incorrectly refers to "cloud
    accounts" instead of "kubernetes accounts". Update the error message to accurately
    reflect the API being called.

    infrastructure/backend_api.py [3121-3123]

     raise Exception(
    -    'Error accessing cloud accounts. Customer: "%s" (code: %d, message: %s)' % (
    +    'Error accessing kubernetes accounts. Customer: "%s" (code: %d, message: %s)' % (
             self.customer, r.status_code, r.text))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The error message incorrectly refers to "cloud accounts" in the Kubernetes accounts API method, which could be misleading for debugging. This fix improves error clarity and maintainability.

    7

    Copy link

    Failed to generate code suggestions for PR

    @kooomix kooomix changed the title Accounts Accounts - cloud and posture Dec 30, 2024
    PROVIDER_AZURE = "azure"
    PROVIDER_GCP = "gcp"

    # a generated good arn from Eran aws dev account - consider moving to an env var?
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    why not?

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    how we handle these kind of "secrets"

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    it's not really a secret, maybe i'll add it as env var.

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    and it manged in github not in code?

    "capabilities.vulnerabilityScan": "disable",
    "grypeOfflineDB.enabled": "false",
    "capabilities.relevancy": "disabled",
    # "capabilities.runtimeObservability": "disable",
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    why cooment?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    no need for this in the this test

    Copy link
    Contributor

    @idohuber idohuber Jan 1, 2025

    Choose a reason for hiding this comment

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

    so just leave it disabled or delete it

    self.delete_and_validate_cloud_account(guid)
    self.test_cloud_accounts_guids.remove(guid)

    Logger.logger.info('Stage 7: Validate cspm results apis - TODO')
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    this is for the scan results? or just for the scanState updates

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    whatever you'll need to test here, but we will need to test anything related with the scan itself.

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    ok,
    i think it will be different flow

    guid = res["response"][0]["guid"]
    return guid

    def validate_accounts_cloud_uniquevalues(self, cloud_account_name:str):
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    why validating only name and not validate all abviable fields for more cover?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I might expand it later it, for now just to make sure the api works

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    ok

    @kooomix kooomix merged commit 8b53d2b into master Jan 2, 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