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

Ephemeral containers #227

Merged
merged 6 commits into from
Apr 7, 2024
Merged

Ephemeral containers #227

merged 6 commits into from
Apr 7, 2024

Conversation

amirmalka
Copy link
Contributor

@amirmalka amirmalka commented Apr 4, 2024

User description

Overview


Type

enhancement


Description

  • Added support for ephemeral containers across various components including utility functions, relevancy manager, rule engine helpers, and application profile manager.
  • Updated dependencies in go.mod.
  • Added a test case for patching ApplicationProfile with ephemeralContainers.
  • Minor formatting adjustment in storage_mock.go.

Changes walkthrough

Relevant files
Enhancement
utils.go
Add Ephemeral Container Support in Utility Functions         

pkg/utils/utils.go

  • Imported ephemeralcontainerinstance package.
  • Added support for EphemeralContainer in
    GetApplicationProfileContainer, InsertApplicationProfileContainer, and
    SetContainerType functions.
  • Implemented ToInstanceType function to support EphemeralContainer.
  • +24/-1   
    relevancy_manager.go
    Support Ephemeral Containers in Relevancy Manager               

    pkg/relevancymanager/v1/relevancy_manager.go

  • Added handling for EphemeralContainer in findImageID and findImageTag
    functions.
  • +13/-0   
    helpers.go
    Enhance Helpers with Ephemeral Container Support                 

    pkg/ruleengine/v1/helpers.go

  • Added support for fetching ApplicationProfileContainer and mount paths
    for EphemeralContainers.
  • +21/-0   
    applicationprofile_manager.go
    Handle Ephemeral Containers in Application Profile Manager

    pkg/applicationprofilemanager/v1/applicationprofile_manager.go

    • Added handling for EphemeralContainers in saveProfile function.
    +3/-1     
    Formatting
    storage_mock.go
    Import Formatting Adjustment                                                         

    pkg/storage/storage_mock.go

    • Minor import formatting change.
    +2/-7     
    Tests
    storage_test.go
    Test Patching with Ephemeral Containers                                   

    pkg/storage/v1/storage_test.go

  • Added a test case for patching ApplicationProfile with
    ephemeralContainers.
  • +29/-0   
    Dependencies
    go.mod
    Update Dependencies                                                                           

    go.mod

  • Updated several dependencies including github.com/cenkalti/backoff/v4,
    github.com/kubescape/k8s-interface, and github.com/kubescape/storage.
  • +86/-66 

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

    Signed-off-by: Amir Malka <amirm@armosec.io>
    @codiumai-pr-agent-free codiumai-pr-agent-free bot added the enhancement New feature or request label Apr 4, 2024
    Copy link

    PR Description updated to latest commit (28b8f98)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR introduces a significant feature with changes across multiple components, including utility functions, relevancy manager, rule engine helpers, and application profile manager. The addition of ephemeral container support touches core functionalities and requires careful review of the integration points, potential edge cases, and the overall impact on existing functionalities.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The implementation assumes the presence of ephemeral containers in all workload types without null checks. This could lead to runtime errors in workloads that do not support ephemeral containers.

    Performance Concern: The addition of ephemeral container checks in hot paths (e.g., relevancy manager and rule engine helpers) might introduce performance regressions, especially in large clusters with many workloads.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Handle errors when fetching ephemeral containers to improve debugging.

    Consider handling the error returned by wl.GetEphemeralContainers() instead of silently
    returning. This will help in debugging and understanding if there are any issues fetching
    ephemeral containers.

    pkg/utils/utils.go [285-287]

     ephemeralContainers, err := wl.GetEphemeralContainers()
     if err != nil {
    +    log.Errorf("Error fetching ephemeral containers: %v", err)
         return
     }
     
    Use descriptive test case names for clarity.

    Ensure that the test case names are descriptive and reflect what is being tested,
    especially when adding new test cases for ephemeral containers.

    pkg/storage/v1/storage_test.go [311-316]

     {
    -    name: "test",
    +    name: "PatchApplicationProfile_AddEphemeralContainers",
         args: args{
             name:  storage.NginxKey,
             patch: []byte(`[{"op":"add","path":"/spec/ephemeralContainers","value":[{},{},{"name":"abc"}]}]`),
         },
     }
     
    Enhancement
    Improve error handling for ephemeral container statuses.

    When adding support for EphemeralContainer in findImageID and findImageTag, ensure
    consistency in error handling and return specific error messages for clarity.

    pkg/relevancymanager/v1/relevancy_manager.go [191-192]

     case utils.EphemeralContainer:
    +    if podStatus.EphemeralContainerStatuses == nil {
    +        return "", fmt.Errorf("ephemeral container statuses are not available")
    +    }
         containerStatuses = podStatus.EphemeralContainerStatuses
     
    Performance
    Break out of the loop once the target container is found to improve performance.

    Optimize the search for containers by breaking out of the loop once the target container
    is found to improve performance.

    pkg/ruleengine/v1/helpers.go [29-31]

     for i := range ap.Spec.EphemeralContainers {
         if ap.Spec.EphemeralContainers[i].Name == containerName {
             return ap.Spec.EphemeralContainers[i], nil
         }
     }
    +// No need to continue if the container is found
    +break
     
    Maintainability
    Refactor conditional structure to switch statement for clarity.

    Consider refactoring the conditional structure for setting existingContainers to use a
    switch statement for better readability and maintainability.

    pkg/applicationprofilemanager/v1/applicationprofile_manager.go [376-381]

    -if watchedContainer.ContainerType == utils.Container {
    +switch watchedContainer.ContainerType {
    +case utils.Container:
         existingContainers = existingProfile.Spec.Containers
    -} else if watchedContainer.ContainerType == utils.InitContainer {
    +case utils.InitContainer:
         existingContainers = existingProfile.Spec.InitContainers
    -} else {
    +case utils.EphemeralContainer:
         existingContainers = existingProfile.Spec.EphemeralContainers
     }
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    go.mod Show resolved Hide resolved
    go.mod Show resolved Hide resolved
    Signed-off-by: Amir Malka <amirm@armosec.io>
    Signed-off-by: Amir Malka <amirm@armosec.io>
    Copy link

    github-actions bot commented Apr 4, 2024

    Summary:

    • License scan: failure
    • Credentials scan: failure
    • Vulnerabilities scan: success
    • Unit test: success
    • Go linting: success

    Signed-off-by: Amir Malka <amirm@armosec.io>
    @amirmalka amirmalka changed the title WIP: Ephemeral containers Ephemeral containers Apr 4, 2024
    Copy link

    github-actions bot commented Apr 4, 2024

    Summary:

    • License scan: failure
    • Credentials scan: failure
    • Vulnerabilities scan: success
    • Unit test: success
    • Go linting: success

    Signed-off-by: Amir Malka <amirm@armosec.io>
    Copy link

    github-actions bot commented Apr 7, 2024

    Summary:

    • License scan: failure
    • Credentials scan: failure
    • Vulnerabilities scan: success
    • Unit test: success
    • Go linting: success

    @dwertent dwertent merged commit 9a70fcb into rule-manager Apr 7, 2024
    6 checks passed
    @matthyx matthyx deleted the ephemeral-containers branch September 17, 2024 09:09
    This pull request was closed.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants