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

migrate network tests to NetworkNeighborhoods #450

Merged
merged 1 commit into from
Aug 26, 2024
Merged

migrate network tests to NetworkNeighborhoods #450

merged 1 commit into from
Aug 26, 2024

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Aug 22, 2024

PR Type

enhancement, tests


Description

  • Refactored code to replace NetworkNeighbors with NetworkNeighborhood across multiple files.
  • Updated test scripts to reflect the new network neighborhood terminology.
  • Modified JSON and YAML configurations to align with the new structure and naming conventions.
  • Updated documentation and descriptions to use network neighborhood terminology.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
ks_vuln_scan_tests.py
Update variable name to neighborhood_map                                 

configurations/system/tests_cases/ks_vuln_scan_tests.py

  • Renamed neighbors_map to neighborhood_map.
+1/-1     
network_policy_tests.py
Rename network neighbors to network neighborhood                 

configurations/system/tests_cases/network_policy_tests.py

  • Renamed expected_network_neighbors to expected_network_neighborhood.
  • Updated paths for expected network neighborhood files.
  • +19/-19 
    kubectl_wrapper.py
    Update kind name in kubectl wrapper                                           

    infrastructure/kubectl_wrapper.py

  • Changed NetworkNeighbors to NetworkNeighborhood in
    kubescape_namespaced_kinds.
  • +1/-1     
    statics.py
    Update plural form for network neighborhood                           

    systest_utils/statics.py

    • Changed NETWORK_NEIGHBOR_PLURAL to networkneighborhoods.
    +1/-1     
    base_network_policy.py
    Refactor network neighbors to network neighborhood             

    tests_scripts/helm/base_network_policy.py

  • Renamed methods and variables from network_neighbors to
    network_neighborhood.
  • +35/-35 
    base_k8s.py
    Rename method for network neighborhood retrieval                 

    tests_scripts/kubernetes/base_k8s.py

  • Renamed method get_network_neighbors to get_network_neighborhood.
  • +3/-3     
    networkneighborhood.yaml
    Update CRD kind and structure for network neighborhood     

    configurations/kubescape-crds/supported/networkneighborhood.yaml

  • Changed kind from NetworkNeighbors to NetworkNeighborhood.
  • Added containers field with ingress and egress.
  • +5/-3     
    busybox-known-server.json
    Update JSON structure for network neighborhood                     

    configurations/network-policy/expected-network-neighbors/busybox-known-server.json

  • Changed kind from NetworkNeighbors to NetworkNeighborhood.
  • Added containers field with ingress and egress.
  • +57/-52 
    Tests
    1 files
    network_policy.py
    Update test scripts for network neighborhood terminology 

    tests_scripts/helm/network_policy.py

  • Updated test plan comments to reflect neighborhood terminology.
  • Renamed variables from network_neighbors to network_neighborhood.
  • +53/-53 
    Documentation
    1 files
    system_test_mapping.json
    Update test mapping descriptions for network neighborhood

    system_test_mapping.json

    • Updated descriptions to use network neighborhood terminology.
    +6/-6     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-free codiumai-pr-agent-free bot added enhancement New feature or request Tests labels Aug 22, 2024
    Copy link

    Preparing PR description...

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Duplication
    The functions validate_expected_network_neighborhood_list and validate_expected_network_neighborhood are very similar to their network_neighbors counterparts. Consider refactoring to reduce duplication.

    Repeated Code
    Multiple instances of similar code blocks for validating network neighborhood and generated network policies. Consider extracting this into a reusable function.

    Copy link

    Preparing review...

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use a more specific exception type for better error handling

    Consider using a more specific exception type instead of a general ValueError. This
    would make error handling more precise and informative.

    tests_scripts/helm/base_network_policy.py [169]

    -raise ValueError("expected entry is not valid, it should contain either dns, ipAddress or namespaceSelector/podSelector")
    +class InvalidNetworkEntryError(ValueError):
    +    pass
     
    +raise InvalidNetworkEntryError("Expected entry is not valid. It should contain either dns, ipAddress, or namespaceSelector/podSelector.")
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Defining a specific exception class improves error handling by providing more precise and informative error messages, which is beneficial for debugging and maintenance.

    8
    Add example values for ingress and egress fields in the NetworkNeighborhood template

    Consider adding example values for the ingress and egress fields to provide a more
    comprehensive template for users.

    configurations/kubescape-crds/supported/networkneighborhood.yaml [7-9]

     containers:
       - name: nginx
         ingress:
    +      - identifier: example-ingress
    +        type: internal
    +        ports:
    +          - name: HTTP
    +            protocol: TCP
    +            port: 80
         egress:
    +      - identifier: example-egress
    +        type: external
    +        dns: example.com
    +        ports:
    +          - name: HTTPS
    +            protocol: TCP
    +            port: 443
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Providing example values for ingress and egress fields enhances the template's usability and helps users understand how to configure these fields effectively.

    8
    Add a "dnsNames" field to internal egress rules for consistency and potential future use

    Consider adding a "dnsNames" field to the internal egress rules, similar to the
    external ones. This can help in cases where DNS resolution is needed for internal
    services.

    configurations/network-policy/expected-network-neighbors/deployment-wikijs.json [63-71]

     "type": "internal",
     "dns": "",
    +"dnsNames": [],
     "ports": [
       {
         "name": "UDP-53",
         "protocol": "UDP",
         "port": 53
       }
     ],
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a "dnsNames" field to internal egress rules can improve consistency and future-proof the configuration for potential DNS resolution needs, although it is not immediately necessary.

    7
    Maintainability
    Replace hardcoded sleep duration multiplier with a constant

    Consider using a constant or configuration variable for the sleep duration instead
    of hardcoding the value 6. This would make the code more maintainable and easier to
    adjust in the future.

    tests_scripts/helm/network_policy.py [55]

    -TestUtil.sleep(6 * int(update_period_in_seconds), "wait for node-agent update period", "info")
    +SLEEP_MULTIPLIER = 6
    +TestUtil.sleep(SLEEP_MULTIPLIER * int(update_period_in_seconds), "wait for node-agent update period", "info")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a constant for the sleep duration multiplier improves maintainability by making it easier to adjust the value in the future without searching through the code.

    7
    Best practice
    Use a more descriptive variable name for better code readability

    Consider using a more descriptive variable name for nn_json. Something like
    network_neighborhood_json would be clearer and more self-explanatory.

    tests_scripts/helm/network_policy.py [64]

    -nn_json = json.dumps(actual_network_neighborhood)
    +network_neighborhood_json = json.dumps(actual_network_neighborhood)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: A more descriptive variable name enhances code readability and understanding, which is a good practice for maintainability.

    6
    Implement or remove the TODO comment to avoid technical debt

    The commented out TODO block should be removed or implemented. Leaving TODO comments
    in the code can lead to technical debt if not addressed promptly.

    tests_scripts/helm/base_network_policy.py [271]

    -# TODO rewrite to use networkneighborhood for the graph
    -# self.validate_expected_network_neighborhood(namespace=namespace, actual_network_neighborhood=graph, expected_network_neighborhood=expected_network_neighborhood_list[i])
    +self.validate_expected_network_neighborhood(namespace=namespace, actual_network_neighborhood=graph, expected_network_neighborhood=expected_network_neighborhood_list[i])
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Addressing TODO comments is important to prevent technical debt, but the suggestion assumes the implementation is straightforward without additional context.

    5

    @matthyx matthyx force-pushed the fix branch 11 times, most recently from 1c5c1dc to b4f28fa Compare August 23, 2024 09:33
    @matthyx matthyx merged commit d73e9ff into master Aug 26, 2024
    3 checks passed
    @matthyx matthyx deleted the fix branch August 26, 2024 05:14
    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