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

Fix verify pods and jira rate limit handling #553

Merged
merged 11 commits into from
Jan 1, 2025
Merged

Conversation

kooomix
Copy link
Contributor

@kooomix kooomix commented Jan 1, 2025

PR Type

Enhancement, Bug fix


Description

  • Added namespace filtering safeguards in pod operations

  • Replaced kubectl shell commands with API calls

  • Implemented wait mechanism for security risk reports

  • Enhanced pod state logging and visibility


Changes walkthrough 📝

Relevant files
Enhancement
jira_integration.py
Add wait mechanism for security risk report retrieval       

tests_scripts/helm/jira_integration.py

  • Added wait mechanism with timeout and retry logic for security risk
    report retrieval
  • Replaced direct resource fetch with wait_for_report method for better
    reliability
  • +9/-1     
    Bug fix
    base_k8s.py
    Improve pod retrieval safety and logging capabilities       

    tests_scripts/kubernetes/base_k8s.py

  • Added namespace filtering safeguards to pod retrieval methods
  • Replaced shell-based kubectl pod listing with direct API calls
  • Added new method get_all_pods_printable_details for improved logging
  • Refactored pod readiness check logic for better clarity
  • +25/-16 

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Redundant Filter

    The namespace filtering is done twice - once in get_pods() and again in get_ready_pods(). The second filter in get_ready_pods() is redundant since get_pods() already filters by namespace.

    # Safeguard: Ensure namespace consistency
    pods = [pod for pod in pods if pod.metadata.namespace == namespace]
    Error Handling

    The wait_for_report call in create_jira_issue_for_security_risks() should include error handling in case the timeout is reached without finding the resource.

                return True
            Logger.logger.info(
                "Cluster '{}' wasn't confirmed as already deleted from backend. Error: {}".format(cluster_name, ex))
        Logger.logger.info("Deleting cluster '{}' from backend".format(cluster_name))
        self.backend.delete_cluster(cluster_name=cluster_name)
    except requests.ReadTimeout as e:

    Copy link

    codiumai-pr-agent-free bot commented Jan 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for failed report retrieval to prevent potential null reference exceptions

    Add error handling for the case when the wait_for_report call times out or fails.
    Currently, the code continues to use the 'resource' variable without checking if it
    was successfully retrieved.

    tests_scripts/helm/jira_integration.py [213-221]

     resource, t = self.wait_for_report(
         timeout=120, 
         sleep_interval=10,
         report_type=self.backend.get_security_risks_list,
         security_risk_id=security_risk_id,
     )
    +if not resource:
    +    raise Exception(f"Failed to retrieve security risk report for ID {security_risk_id}")
     
    -# resource = self.get_security_risks_resource(security_risk_id)
     resourceHash = resource['k8sResourceHash']
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical error handling gap that could lead to runtime exceptions if the report retrieval fails. Adding this check is important for system stability and debugging.

    8
    Add null check for pods list to prevent potential NoneType exceptions during list operations

    The verify_running_pods method should check if total_pods is None before attempting
    list operations to prevent potential NoneType exceptions.

    tests_scripts/kubernetes/base_k8s.py [793-794]

     total_pods = self.get_pods(namespace=namespace, name=name, include_terminating=False)
    +if total_pods is None:
    +    total_pods = []
     non_running_pods = [i for i in total_pods if i not in running_pods]
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This is a valuable defensive programming suggestion that prevents potential NoneType exceptions, especially important in Kubernetes operations where API calls might fail.

    7
    Fix potential undefined variable reference by directly returning the filtered pods list

    The get_ready_pods method is missing a return value assignment for ready_pods, which
    could lead to undefined behavior.

    tests_scripts/kubernetes/base_k8s.py [712-726]

     def get_ready_pods(self, namespace, name: str = None):
         """
         :return: list of running pods with all containers ready
         """
    -
         pods = self.get_pods(namespace=namespace, name=name)
     
         # Safeguard: Ensure namespace consistency
         pods = [pod for pod in pods if pod.metadata.namespace == namespace]
     
    -    ready_pods = [
    -    pod for pod in pods
    -    if all(container.ready for container in (pod.status.container_statuses or []))
    -]
    -    return ready_pods
    +    return [
    +        pod for pod in pods
    +        if all(container.ready for container in (pod.status.container_statuses or []))
    +    ]
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    Why: The suggestion is technically valid but unnecessary, as the code already correctly assigns and returns ready_pods. The proposed change is merely a style preference that doesn't fix any actual issues.

    2

    Copy link

    github-actions bot commented Jan 1, 2025

    Failed to generate code suggestions for PR

    @kooomix kooomix changed the title Fix verify pods Fix verify pods and jira rate limit handling Jan 1, 2025
    …_limit methods for improved clarity and reliability
    @kooomix kooomix merged commit ef23e89 into master Jan 1, 2025
    2 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