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 registry scanning system test #511

Merged
merged 1 commit into from
Dec 4, 2024
Merged

add registry scanning system test #511

merged 1 commit into from
Dec 4, 2024

Conversation

refaelm92
Copy link
Contributor

@refaelm92 refaelm92 commented Dec 4, 2024

User description

Signed-off-by: refaelm [email protected]


PR Type

Tests, Enhancement


Description

  • Added a new RegistryTests class to the system tests to support registry scanning.
  • Implemented new API endpoints and methods for registry management, including CRUD operations.
  • Introduced RegistryChecker class for handling registry operations and validations.
  • Added configuration files for registry checks and creation for AWS, Azure, Google, and Quay.
  • Updated system test mapping to include registry scanning tests.

Changes walkthrough 📝

Relevant files
Tests
4 files
tests.py
Integrate RegistryTests into system tests framework           

configurations/system/tests.py

  • Added import for RegistryTests.
  • Included RegistryTests in the all_tests_names function.
  • Integrated RegistryTests in the get_test function.
  • +4/-0     
    registry_tests.py
    Add RegistryTests class for registry scanning                       

    configurations/system/tests_cases/registry_tests.py

  • Introduced RegistryTests class.
  • Added test_registry_scanning method for registry scanning.
  • +20/-0   
    base_registry.py
    Introduce BaseRegistry class for registry tests                   

    tests_scripts/registry/base_registry.py

    • Created BaseRegistry class for registry tests.
    +14/-0   
    registry_connectors.py
    Add RegistryChecker class for registry operations               

    tests_scripts/registry/registry_connectors.py

  • Added RegistryChecker class for registry operations.
  • Implemented methods for checking and managing registries.
  • +138/-0 
    Enhancement
    1 files
    backend_api.py
    Implement registry management API endpoints and methods   

    infrastructure/backend_api.py

  • Added API endpoints for registry management.
  • Implemented methods for registry CRUD operations.
  • +50/-0   
    Configuration changes
    7 files
    statics.py
    Add default registry paths configuration                                 

    systest_utils/statics.py

    • Defined DEFAULT_REGISTRY_PATHS for registry configurations.
    +3/-0     
    check_aws.json
    Add AWS registry check configuration                                         

    configurations/registry/check_aws.json

    • Added AWS registry check configuration.
    +4/-0     
    check_azure.json
    Add Azure registry check configuration                                     

    configurations/registry/check_azure.json

    • Added Azure registry check configuration.
    +4/-0     
    check_google.json
    Add Google registry check configuration                                   

    configurations/registry/check_google.json

    • Added Google registry check configuration.
    +3/-0     
    check_quay.json
    Add Quay registry check configuration                                       

    configurations/registry/check_quay.json

    • Added Quay registry check configuration.
    +4/-0     
    create_quay.json
    Add Quay registry creation configuration                                 

    configurations/registry/create_quay.json

    • Added Quay registry creation configuration.
    +6/-0     
    system_test_mapping.json
    Update system test mapping with registry scanning               

    system_test_mapping.json

    • Added test_registry_scanning entry for system test mapping.
    +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 AWS access key ID is hardcoded in check_aws.json (line 3). This should be moved to a secure configuration management system or environment variable like the other registry credentials.

    ⚡ Recommended focus areas for review

    Security Validation
    The registry connection check should validate SSL certificates and implement proper error handling for failed connections

    Error Handling
    The registry scan completion check uses a fixed timeout of 60 retries which may not be sufficient for large registries

    Input Validation
    The registry management API endpoints need input validation for the provider parameter and request payloads

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Remove hardcoded cloud service credentials from configuration files to prevent security vulnerabilities

    Remove hardcoded AWS access key ID from the configuration file. Access keys should
    be injected via environment variables or secure configuration management systems to
    prevent credential exposure.

    configurations/registry/check_aws.json [1-4]

     {
       "registryURI": "015253967648.dkr.ecr.eu-central-1.amazonaws.com",
    -  "accessKeyID": "AKIAQHDJVCMQAN4N4I55"
    +  "accessKeyID": ""
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Hardcoded AWS access key IDs in source code pose a critical security risk as they could be exploited for unauthorized access. Moving credentials to environment variables is essential for security best practices.

    10
    Possible issue
    Improve error handling in asynchronous operation status checks to catch and report failures properly

    Add error handling for failed API responses in the registry scan completion check to
    avoid silent failures and provide better debugging information.

    tests_scripts/registry/registry_connectors.py [112-123]

     def assert_registry_scan_completed(self, provider, guid):
         TestUtil.sleep(30, "waiting for registry scan to complete")
         for i in range(60):
             try:
                 registry = self.backend.get_registry(provider, guid)
    -            assert registry, "Expected registry"
    -            assert registry.json()["scanStatus"] == "Completed"
    -            break
    -        except AssertionError:
    +            if not registry:
    +                raise AssertionError(f"Failed to get registry status for provider {provider}")
    +            status = registry.json().get("scanStatus")
    +            if status == "Failed":
    +                raise AssertionError(f"Registry scan failed for provider {provider}")
    +            if status == "Completed":
    +                break
    +        except Exception as e:
                 if i == 59:
    -                raise
    +                raise AssertionError(f"Registry scan did not complete in time: {str(e)}")
                 TestUtil.sleep(2, "waiting for registry scan to complete")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The improved error handling provides better visibility into failure modes and specific error conditions during registry scanning, making debugging and maintenance easier.

    7

    💡 Need additional feedback ? start a PR chat

    Copy link

    github-actions bot commented Dec 4, 2024

    Failed to generate code suggestions for PR

    @refaelm92 refaelm92 merged commit dac7117 into master Dec 4, 2024
    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