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 namespace argument support to control scripts and update attack c… #582

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 16, 2025

User description

…hain tests


PR Type

Enhancement, Tests


Description

  • Added support for namespace-specific operations in attack chain scenarios.

  • Introduced a new test configuration attackchains_all for multiple scenarios.

  • Enhanced backend API to filter attack chains by namespace.

  • Updated scripts and configurations to handle namespaces dynamically.


Changes walkthrough 📝

Relevant files
Tests
1 files
ks_microservice_test.py
Added `attackchains_all` test configuration for multiple scenarios
+25/-0   
Enhancement
8 files
backend_api.py
Enhanced attack chain API with namespace filtering             
+12/-6   
scenarios_manager.py
Improved scenario management with namespace support           
+41/-10 
ks_microservice.py
Added multi-scenario attack chain testing functionality   
+176/-1 
fix_control
Made fix script namespace-aware for alpine service             
+13/-2   
fix_image
Made fix script namespace-aware for alpine image                 
+13/-2   
fix_control
Made fix script namespace-aware for attack-chain-8             
+15/-2   
02-service-unauthnticated.yaml
Removed hardcoded namespace from attack-chain-9 configuration
+0/-4     
fix_control
Made fix script namespace-aware for attack-chain-9             
+15/-4   
Documentation
1 files
readme.md
Documented new `attackchains_all` test configuration         
+1/-0     
Configuration changes
1 files
system_test_mapping.json
Added mapping for attackchains_all and updated repository references
+24/-11 

💡 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
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The verify_fixes() method does not properly handle the case where timeout is reached - it continues execution after setting succeded=False instead of breaking immediately

# break if timeout is reached
if timeout < 0:
    verification_report[scenario_key] = "Timeout reached"
    succeded = False
    break
Missing Validation

The get_attack_chains() method does not validate that namespace parameter is a valid k8s namespace name before using it in API request

    Logger.logger.info(
        "Found {} sessions of container '{}' and component '{}'".format(len(sessions), container, component))

    return sessions

def get_posture_clusters_overtime(self, cluster_name: str, framework_name: str = ""):
    params = {"pageNum": 1, "pageSize": 1, "orderBy": "timestamp:desc", "innerFilters": [{
        "clusterName": cluster_name, "frameworkName": framework_name}]}

    if framework_name in statics.SECURITY_FRAMEWORKS:
        params["innerFilters"][0]["typeTags"] = statics.SECURITY_FRAMEWORK_TYPETAG

    r = self.post(API_POSTURE_CLUSTERSOVERTIME,
                  json=params)
    if not 200 <= r.status_code < 300:
        raise Exception(
Race Condition

The verify_scenario() method may have race conditions when multiple scenarios are being verified concurrently due to shared timeout calculation

    assert response["securityRiskID"] == security_risk_id, f"Failed to edit exception, expected securityRiskID: {security_risk_id}, got: {response['securityRiskID']}"
    assert len(response["resources"]) == len(exceptions_resources), f"Failed to add exception, expected resources: {exceptions_resources}, got: {response['resources']}"
    return response

def get_security_risks_resources(self, security_risk_id):
    """
    get_security_risks_resources get the security risks resources from the backend
    """
    r = self.backend.get_security_risks_resources(self.cluster, self.namespace, security_risk_id)
    response = json.loads(r.text)
    if "response" not in response or response["response"] ==  None:
        response["response"] = []
    return response

def get_security_risks_list(self, security_risk_ids=[]):
    """
    get_security_risks_list get the security risks list from the backend
    """
    r = self.backend.get_security_risks_list(self.cluster, self.namespace, security_risk_ids)
    response = json.loads(r.text)
    return response


def check_security_risks_resources_results(self, result, expected):
    """
    check_security_risks_resources_results - Validate the input content with the expected one of security risks resources

    :param result: content retrieved from backend.
    """

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Fix variable reference before definition to prevent undefined variable error

The fix_command variable is used before it's defined in the apply_fix method. Move
the variable definition before its usage.

systest_utils/scenarios_manager.py [72]

+fix_command = "bash " + os.path.join(self.scenario_path, self.test_scenario, 'fix_' + fix_type) + ' --namespace ' + self.namespace
 TestUtil.run_command(command_args=fix_command, display_stdout=True, timeout=300)
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion identifies a critical bug where fix_command is used before being defined, which would cause a NameError at runtime. Moving the variable definition before its usage is essential for proper code execution.

8
Return boolean value instead of using assertion to match function's return type annotation

The has_active_attack_chains method asserts that total value is 0 but doesn't return
a boolean value as indicated by the return type annotation.

infrastructure/backend_api.py [2022-2026]

 def has_active_attack_chains(self, cluster_name=None, namespace=None) -> bool:
     r = self.get_attack_chains(cluster_name, namespace)
     response = json.loads(r.text)
-    assert response['total']['value'] == 0, f"attack-chains not fixed yet"
+    return response['total']['value'] == 0
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion improves code correctness by making the function return a boolean value as specified in its type annotation, instead of using an assertion that could raise an exception. This makes the function more reliable and type-safe.

7

Copy link

Failed to generate code suggestions for PR

… including stack traces in exception messages
@kooomix kooomix merged commit 2fc7b8d into master Jan 16, 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.

1 participant