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

support ephemeral containers #97

Merged
merged 2 commits into from
Apr 5, 2024
Merged

support ephemeral containers #97

merged 2 commits into from
Apr 5, 2024

Conversation

amirmalka
Copy link
Contributor

@amirmalka amirmalka commented Apr 4, 2024

Type

enhancement


Description

  • Introduced handling for ephemeral container instances including validation, listing, and ID generation.
  • Added unit tests for new functions and methods related to ephemeral container instances.
  • Enhanced IWorkload interface and workload methods to include ephemeral containers retrieval.
  • Updated instance ID generation to support ephemeral containers.

Changes walkthrough

Relevant files
Enhancement
7 files
helpers.go
Add Ephemeral Container Instance Handling                               

instanceidhandler/v1/ephemeralcontainerinstance/helpers.go

  • Introduced package ephemeralcontainerinstance for handling ephemeral
    container instances.
  • Added validateInstanceID function to validate the instance ID fields.
  • Added listInstanceIDs function to list instance IDs for ephemeral
    containers.
  • +63/-0   
    initializers.go
    Ephemeral Container Instance ID Generation Functions         

    instanceidhandler/v1/ephemeralcontainerinstance/initializers.go

  • Added functions to generate instance IDs from workload, pod, and
    string for ephemeral containers.
  • +73/-0   
    instanceidhandler.go
    Implement Ephemeral Container InstanceID Handling               

    instanceidhandler/v1/ephemeralcontainerinstance/instanceidhandler.go

  • Implemented InstanceID struct and methods for ephemeral container
    instances.
  • Added constants and methods for string formatting and hashing of
    instance IDs.
  • +101/-0 
    keys.go
    Add Ephemeral Container Name Metadata Key                               

    instanceidhandler/v1/helpers/keys.go

  • Added EphemeralContainerNameMetadataKey constant for ephemeral
    container name metadata.
  • +1/-0     
    initializers.go
    Integrate Ephemeral Container Instance ID Generation         

    instanceidhandler/v1/initializers.go

  • Integrated ephemeral container instance ID generation into
    GenerateInstanceID functions.
  • +29/-1   
    interface.go
    Add Ephemeral Containers Retrieval to IWorkload Interface

    workloadinterface/interface.go

  • Added GetEphemeralContainers method to the IWorkload interface.
  • +1/-0     
    workloadmethods.go
    Implement Ephemeral Containers Retrieval Method                   

    workloadinterface/workloadmethods.go

    • Implemented GetEphemeralContainers method for workload objects.
    +17/-0   
    Tests
    5 files
    helpers_test.go
    Unit Tests for Ephemeral Container Instance Helpers           

    instanceidhandler/v1/ephemeralcontainerinstance/helpers_test.go

  • Added tests for validateInstanceID function.
  • Added tests for listInstanceIDs function.
  • +340/-0 
    initializers_test.go
    Tests for Ephemeral Container Instance ID Generators         

    instanceidhandler/v1/ephemeralcontainerinstance/initializers_test.go

  • Added tests for ephemeral container instance ID generation functions.
  • +135/-0 
    instanceidhandler_test.go
    Unit Tests for Ephemeral Container InstanceID Methods       

    instanceidhandler/v1/ephemeralcontainerinstance/instanceidhandler_test.go

  • Added unit tests for InstanceID methods in ephemeral container
    instance handling.
  • +216/-0 
    initializers_test.go
    Update Tests for Instance ID Generation with Ephemeral Containers

    instanceidhandler/v1/initializers_test.go

  • Updated tests to include ephemeral container instance ID generation.
  • +4/-1     
    workloadmock.go
    Mock Implementation for Ephemeral Containers Retrieval     

    workloadinterface/workloadmock.go

    • Added mock implementation for GetEphemeralContainers method.
    +5/-0     

    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 codiumai-pr-agent bot added the enhancement New feature or request label Apr 4, 2024
    Copy link

    PR Description updated to latest commit (e50dbfa)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR introduces significant changes across multiple files, including new functionalities for handling ephemeral containers, extensive unit tests, and modifications to existing interfaces and methods. The complexity and breadth of these changes require a thorough review to ensure correctness, compatibility, and adherence to best practices.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The listInstanceIDs function in helpers.go returns an empty slice and nil error when ephemeralContainers are empty. This behavior is correct, but it's important to ensure that calling functions handle this case appropriately to avoid potential nil pointer dereferences or unexpected behavior.

    Performance Concern: The listInstanceIDs function iterates over all ephemeral containers and validates each instance ID. In scenarios with a large number of ephemeral containers, this could introduce performance bottlenecks. Consider optimizing this process or documenting potential performance implications.

    🔒 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                                                                                                                                                       
    Enhancement
    Refactor Test_validateInstanceID to use table-driven tests with subtests.

    Consider using table-driven tests with subtests for Test_validateInstanceID to improve
    code readability and maintainability. This approach allows you to define test cases in a
    structured format and run them as subtests, which can make it easier to identify which
    test case fails without having to navigate through multiple test functions.

    instanceidhandler/v1/ephemeralcontainerinstance/helpers_test.go [11-112]

    -func Test_validateInstanceID(t *testing.T) {
    -  type args struct {
    +func TestValidateInstanceID(t *testing.T) {
    +  tests := []struct {
    +    name      string
         instanceID *InstanceID
    +    wantErr   bool
    +  }{
    +    // Test cases here
       }
    -  tests := []struct {
    -    name    string
    -    args    args
    -    wantErr bool
    -  }{
    +  for _, tt := range tests {
    +    t.Run(tt.name, func(t *testing.T) {
    +      if err := validateInstanceID(tt.instanceID); (err != nil) != tt.wantErr {
    +        t.Errorf("validateInstanceID() error = %v, wantErr %v", err, tt.wantErr)
    +      }
    +    })
    +  }
     
    Improve error handling in tests for better readability and consistency.

    Instead of using t.Fatalf directly with err.Error(), you can use require.NoError(t, err,
    "custom error message") for better readability and consistency. This also applies to other
    error handling in tests.

    instanceidhandler/v1/ephemeralcontainerinstance/initializers_test.go [18]

    -if err != nil {
    -    t.Fatalf(err.Error())
    -}
    +require.NoError(t, err, "can't create instance ID from pod")
     
    Use more descriptive error messages for better debugging.

    Consider using a more descriptive error message that includes the kind of workload when
    the kind is not "Pod". This will help in debugging and understanding the context of the
    error.

    instanceidhandler/v1/ephemeralcontainerinstance/initializers.go [17]

    -return nil, fmt.Errorf("CreateInstanceID: workload kind must be Pod for create instance ID")
    +return nil, fmt.Errorf("GenerateInstanceID: workload kind must be Pod, got %s", w.GetKind())
     
    Implement the GetEphemeralContainers method in the IBasicWorkload interface.

    Consider implementing the GetEphemeralContainers method to ensure that the interface
    IBasicWorkload fully supports ephemeral containers, as this feature is crucial for
    temporary or debugging workloads in Kubernetes.

    workloadinterface/interface.go [60]

    -GetEphemeralContainers() ([]corev1.EphemeralContainer, error)
    +GetEphemeralContainers() ([]corev1.EphemeralContainer, error) {
    +  // Implementation goes here
    +}
     
    Add descriptive labels to the service for better observability.

    To enhance the observability of the service, consider adding more descriptive labels that
    reflect the service's purpose, version, and other relevant metadata.

    instanceidhandler/v1/ephemeralcontainerinstance/testdata/service.json [6-8]

     "labels": {
       "app": "armo-vuln-scan",
    -  "app.kubernetes.io/managed-by": "Helm"
    +  "app.kubernetes.io/managed-by": "Helm",
    +  "app.kubernetes.io/name": "armo-vuln-scan",
    +  "app.kubernetes.io/version": "1.0.0"
     }
     
    Best practice
    Use mocks for external dependencies in Test_listInstanceIDs.

    For the listInstanceIDs function tests, consider using a mock for external dependencies to
    ensure that your unit tests are not dependent on the external state. This will make your
    tests more reliable and faster.

    instanceidhandler/v1/ephemeralcontainerinstance/helpers_test.go [115-339]

    -func Test_listInstanceIDs(t *testing.T) {
    -  type args struct {
    -    ownerReferences []metav1.OwnerReference
    -    containers      []core1.EphemeralContainer
    -    apiVersion      string
    -    namespace       string
    -    kind            string
    -    name            string
    -  }
    +func TestListInstanceIDs(t *testing.T) {
    +  // Use a mocking framework to mock external dependencies
    +  // Define your tests here
    +}
     
    Use errors.New for simple error messages.

    Avoid using fmt.Errorf for errors that do not format strings. Instead, use errors.New for
    simple error messages to improve code readability.

    instanceidhandler/v1/ephemeralcontainerinstance/instanceidhandler_test.go [30-31]

     if err != nil {
    -  return fmt.Errorf(err.Error())
    +  return errors.New(err.Error())
     }
     
    Use consistent imagePullPolicy across all container types for reliability.

    For consistency and to avoid potential image mismatches, consider using the same
    imagePullPolicy across all container types, including ephemeralContainers and
    initContainers.

    instanceidhandler/v1/testdata/deployment.json [27-31]

     "ephemeralContainers": [
         {
             "image": "nginx@sha256:f7988fb6c02e0ce69257d9bd9cf37ae20a60f1df7563c3a2a6abe24160306b8d",
    -        "imagePullPolicy": "IfNotPresent",
    +        "imagePullPolicy": "Always",
             "name": "abc"
         }
     ]
     
    Set resource limits for the ephemeral container to manage resource usage.

    To ensure that the ephemeral container is only used for its intended short-lived
    operations, it's recommended to set a resource limit to prevent it from consuming
    excessive cluster resources.

    instanceidhandler/v1/ephemeralcontainerinstance/testdata/jobPod.json [30-45]

     "ephemeralContainers": [
         {
             "image": "nginx",
             "imagePullPolicy": "Always",
             "name": "nginx-job",
    -        "resources": {},
    +        "resources": {
    +            "limits": {
    +                "cpu": "100m",
    +                "memory": "200Mi"
    +            },
    +            "requests": {
    +                "cpu": "50m",
    +                "memory": "100Mi"
    +            }
    +        },
             "terminationMessagePath": "/dev/termination-log",
             "terminationMessagePolicy": "File",
             "volumeMounts": [
                 {
                     "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount",
                     "name": "kube-api-access-8b7q6",
                     "readOnly": true
                 }
             ]
         }
     ]
     
    Maintainability
    Use constants for repeated string literals.

    Consider using constants for repeated string literals to make the code more maintainable
    and to avoid typos in string values.

    instanceidhandler/v1/ephemeralcontainerinstance/instanceidhandler_test.go [110-116]

    +const (
    +  apiGroup = "apps"
    +  apiVersion = "v1"
    +  namespace = "default"
    +  kind = "ReplicaSet"
    +  name = "nginx-84f5585d68"
    +  containerName = "nginx"
    +)
     expectedLabels := map[string]string{
    -  helpers.ApiGroupMetadataKey:               "apps",
    -  helpers.ApiVersionMetadataKey:             "v1",
    -  helpers.NamespaceMetadataKey:              "default",
    -  helpers.KindMetadataKey:                   "ReplicaSet",
    -  helpers.NameMetadataKey:                   "nginx-84f5585d68",
    -  helpers.EphemeralContainerNameMetadataKey: "nginx",
    +  helpers.ApiGroupMetadataKey:               apiGroup,
    +  helpers.ApiVersionMetadataKey:             apiVersion,
    +  helpers.NamespaceMetadataKey:              namespace,
    +  helpers.KindMetadataKey:                   kind,
    +  helpers.NameMetadataKey:                   name,
    +  helpers.EphemeralContainerNameMetadataKey: containerName,
     }
     
    Break down checkAllsFunctions into smaller test helper functions.

    For better test readability and maintenance, consider breaking down checkAllsFunctions
    into smaller, more focused test helper functions. Each helper function should aim to
    assert a specific aspect of the InstanceID object.

    instanceidhandler/v1/ephemeralcontainerinstance/instanceidhandler_test.go [27-98]

    -func checkAllsFunctions(object string, apiversion, namespace, kind, name, containerName, formattedString, expectedHash string, expectedLabels map[string]string) error {
    +// Example of breaking down into smaller functions
    +func assertAPIVersion(t *testing.T, instanceID *InstanceID, expected string) {
    +  if instanceID.GetAPIVersion() != expected {
    +    t.Errorf("APIVersion got = %s, want %s", instanceID.GetAPIVersion(), expected)
    +  }
    +}
    +// Additional functions for other assertions
     
    Refactor parsing and validation logic for better maintainability.

    To improve maintainability, consider refactoring the GenerateInstanceIDFromString function
    to split the parsing and validation logic into separate functions. This will make the code
    easier to read and maintain.

    instanceidhandler/v1/ephemeralcontainerinstance/initializers.go [39]

     func GenerateInstanceIDFromString(input string) (*InstanceID, error) {
    +    // Refactored parsing logic into parseInput function
    +    fields, err := parseInput(input)
    +    if err != nil {
    +        return nil, err
    +    }
    +    // Refactored instance ID creation into createInstanceID function
    +    instanceID, err := createInstanceID(fields)
    +    if err != nil {
    +        return nil, err
    +    }
    +    return instanceID, nil
    +}
     
    Security
    Enhance security by using a stronger hash function.

    To enhance security, consider using a stronger hash function than SHA-256 for generating
    the hashed instance ID, such as SHA-384 or SHA-512.

    instanceidhandler/v1/ephemeralcontainerinstance/instanceidhandler.go [82]

    -hash := sha256.Sum256([]byte(id.GetStringFormatted()))
    +hash := sha512.Sum512([]byte(id.GetStringFormatted()))
     
    Add security contexts to ephemeralContainers to enhance pod security.

    Ensure that the ephemeralContainers field includes necessary security contexts to maintain
    the security posture of the pod, especially when running in production environments.

    instanceidhandler/v1/ephemeralcontainerinstance/testdata/deployment.json [27-47]

     "ephemeralContainers": [
         {
             "env": [
                 {
                     "name": "DEMO_GREETING",
                     "value": "Hellofromtheenvironment"
                 }
             ],
             "image": "nginx@sha256:f7988fb6c02e0ce69257d9bd9cf37ae20a60f1df7563c3a2a6abe24160306b8d",
             "imagePullPolicy": "IfNotPresent",
             "name": "nginx",
             "resources": {},
             "terminationMessagePath": "/dev/termination-log",
             "terminationMessagePolicy": "File",
             "volumeMounts": [
                 {
                     "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount",
                     "name": "kube-api-access-g42z4",
                     "readOnly": true
                 }
    -        ]
    +        ],
    +        "securityContext": {
    +            "runAsUser": 1000,
    +            "allowPrivilegeEscalation": false
    +        }
         }
     ]
     
    Performance
    Pre-allocate slices for better performance and reduced memory usage.

    For better performance and reduced memory usage, consider pre-allocating the slice
    instanceIDs with the expected capacity, since the length of ephemeralContainers is known.

    instanceidhandler/v1/ephemeralcontainerinstance/initializers.go [36]

    -instanceIDs := make([]InstanceID, 0)
    +instanceIDs := make([]InstanceID, 0, len(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.

    Copy link

    github-actions bot commented Apr 4, 2024

    Summary:

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

    1 similar comment
    Copy link

    github-actions bot commented Apr 4, 2024

    Summary:

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

    …ral-containers
    
    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: failure
    • Unit test: success
    • Go linting: success

    @amirmalka amirmalka merged commit beb10a4 into main Apr 5, 2024
    7 of 8 checks passed
    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.

    None yet

    2 participants