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

Sec/helm index max size #4

Closed
wants to merge 2 commits into from
Closed

Conversation

pasha-codefresh
Copy link
Owner

@pasha-codefresh pasha-codefresh commented Mar 26, 2024

User description

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Type

enhancement


Description

  • Introduced a new configuration option helm-registry-max-index-size to limit the size of the helm registry index file.
  • Updated the helm client to enforce this maximum index size limit when fetching the index file.
  • Extended the RepoServerInitConstants struct to include the new HelmRegistryMaxIndexSize parameter.
  • Modified tests to accommodate the new parameter in the helm client's GetIndex method and added a test case for size limit enforcement.

Changes walkthrough

Relevant files
Enhancement
argocd_repo_server.go
Add helm registry max index size configuration                     

cmd/argocd-repo-server/commands/argocd_repo_server.go

  • Added a new flag helm-registry-max-index-size to specify the maximum
    size of the helm registry index file.
  • Integrated the new helmRegistryMaxIndexSize parameter into the
    reposerver.NewServer function.
  • +6/-0     
    repository.go
    Extend RepoServerInitConstants with HelmRegistryMaxIndexSize

    reposerver/repository/repository.go

  • Added HelmRegistryMaxIndexSize field to RepoServerInitConstants struct
    to hold the maximum index size configuration.
  • +1/-0     
    client.go
    Enforce max index size in helm client                                       

    util/helm/client.go

  • Modified GetIndex method to accept maxIndexSize parameter to enforce
    maximum index file size.
  • Updated loadRepoIndex method to limit the index file size based on
    maxIndexSize.
  • +5/-5     
    Tests
    client_test.go
    Update tests for helm client with max index size                 

    util/helm/client_test.go

  • Updated tests to pass maxIndexSize parameter to GetIndex method.
  • Added a new test case to verify the behavior when the index size
    exceeds the specified limit.
  • +10/-4   

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

    Copy link

    PR-Agent was enabled for this repository. To use it, please link your git user with your CodiumAI identity here.

    CI Failure Feedback

    Action: Validate PR Title

    Failed stage: Run thehanimo/pr-title-checker@0cf5902181e78341bb97bb06646396e5bd354b3f [❌]

    Failure summary:

  • The action failed because it attempted to add a label ("title needs formatting") to the PR but
    encountered an HttpError: "Resource not accessible by integration".
  • This error typically occurs when the GitHub token provided does not have the necessary permissions
    to modify PR labels.
  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    22:  Prepare all required actions
    23:  Getting action download info
    24:  Download action repository 'thehanimo/pr-title-checker@0cf5902181e78341bb97bb06646396e5bd354b3f' (SHA:0cf5902181e78341bb97bb06646396e5bd354b3f)
    25:  Complete job name: Validate PR Title
    26:  ##[group]Run thehanimo/pr-title-checker@0cf5902181e78341bb97bb06646396e5bd354b3f
    27:  with:
    28:  GITHUB_TOKEN: ***
    29:  configuration_path: .github/pr-title-checker-config.json
    30:  pass_on_octokit_error: false
    31:  ##[endgroup]
    32:  Using config file .github/pr-title-checker-config.json from repo pasha-codefresh/argo-cd [ref: 8bb41eda04d9a41a4fe7375888d77f91a68fd429]
    33:  Creating label (title needs formatting)...
    34:  Label (title needs formatting) already created.
    35:  ##[notice]PR Title needs to pass regex '^(feat|fix|docs|test|ci|chore)!?(\(.*\))?!?:.*
    36:  Adding label (title needs formatting) to PR...
    37:  HttpError: Resource not accessible by integration
    38:  ##[error]Failed to add label (title needs formatting) to PR
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added the enhancement New feature or request label Mar 26, 2024
    Copy link

    PR Description updated to latest commit (9cfa9a7)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and localized to specific functionalities related to handling Helm registry index sizes. The modifications are clear, and the logic is not overly complex. However, understanding the context and implications of these changes requires familiarity with the Argo CD codebase and Helm registry operations.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The implementation uses io.LimitReader to limit the size of the index file read from the Helm registry. This approach effectively prevents reading beyond the specified limit but does not handle scenarios where the actual index file size exceeds the limit gracefully. It would be beneficial to include a more descriptive error message or handling mechanism to inform the user about the specific issue (index file size exceeds the maximum allowed size) rather than a generic "unexpected end of stream" error.

    Error Handling: The error handling for loadRepoIndex(maxIndexSize int64) could be improved by providing more context-specific error messages. For instance, when the index size exceeds the maximum allowed size, a specific error message indicating this issue would be more helpful to users for troubleshooting.

    🔒 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

    codiumai-pr-agent-pro bot commented Mar 26, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Improve error handling for parsing quantity.

    Consider handling the error from resource.ParseQuantity more gracefully instead of using
    errors.CheckError(err), which might terminate the program. It's better to log the error
    and continue, or return the error to the caller if it's critical.

    cmd/argocd-repo-server/commands/argocd_repo_server.go [114-115]

     helmRegistryMaxIndexSizeQuantity, err := resource.ParseQuantity(helmRegistryMaxIndexSize)
    -errors.CheckError(err)
    +if err != nil {
    +    log.Errorf("Failed to parse helmRegistryMaxIndexSize: %v", err)
    +    return
    +}
     
    Add test case for exceeding maximum index size.

    In tests where client.GetIndex is called with a maxIndexSize parameter, consider adding a
    test case to verify behavior when the index size exceeds maxIndexSize. This would ensure
    the limit is enforced as expected.

    util/helm/client_test.go [39]

    -_, err := client.GetIndex(false, 10000)
    +_, err := client.GetIndex(false, 10000) // Existing test case for normal behavior
    +// Suggested new test case
    +_, err = client.GetIndex(false, 100000000) // Exceeds limit
    +assert.Error(t, err) // Verify that an error is returned
     
    Rename environment variable for clarity.

    The environment variable name ARGOCD_REPO_SERVER_HELM_MANIFEST_MAX_INDEX_SIZE used in
    env.StringFromEnv for helmRegistryMaxIndexSize might be more accurately named
    ARGOCD_REPO_SERVER_HELM_REGISTRY_MAX_INDEX_SIZE to reflect its purpose more clearly.

    cmd/argocd-repo-server/commands/argocd_repo_server.go [216]

    -command.Flags().StringVar(&helmRegistryMaxIndexSize, "helm-registry-max-index-size", env.StringFromEnv("ARGOCD_REPO_SERVER_HELM_MANIFEST_MAX_INDEX_SIZE", "1G"), "Maximum size of registry index file")
    +command.Flags().StringVar(&helmRegistryMaxIndexSize, "helm-registry-max-index-size", env.StringFromEnv("ARGOCD_REPO_SERVER_HELM_REGISTRY_MAX_INDEX_SIZE", "1G"), "Maximum size of registry index file")
     
    Best practice
    Ensure to close response body to avoid resource leaks.

    When reading the index file with io.ReadAll(io.LimitReader(resp.Body, maxIndexSize)),
    ensure to close resp.Body to avoid leaking resources. This can be done by adding a
    deferred close after checking the status code.

    util/helm/client.go [332]

    +defer resp.Body.Close()
     return io.ReadAll(io.LimitReader(resp.Body, maxIndexSize))
     
    Validate maxIndexSize is positive to prevent misuse.

    Consider adding a check for maxIndexSize being positive in GetIndex and loadRepoIndex
    functions to prevent potential misuse or misconfiguration that could lead to unexpected
    behavior.

    util/helm/client.go [230]

     func (c *nativeHelmChart) GetIndex(noCache bool, maxIndexSize int64) (*Index, error) {
    +    if maxIndexSize <= 0 {
    +        return nil, fmt.Errorf("maxIndexSize must be positive")
    +    }
     

    ✨ 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

    PR-Agent was enabled for this repository. To use it, please link your git user with your CodiumAI identity here.

    CI Failure Feedback

    Action: Validate PR Title

    Failed stage: Run thehanimo/pr-title-checker@0cf5902181e78341bb97bb06646396e5bd354b3f [❌]

    Failure summary:

    The action failed because it attempted to add a label "(title needs formatting)" to the PR but
    encountered an HttpError with the message "Resource not accessible by integration". This error
    suggests that the GitHub Action did not have the necessary permissions to modify PR labels.

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    22:  Prepare all required actions
    23:  Getting action download info
    24:  Download action repository 'thehanimo/pr-title-checker@0cf5902181e78341bb97bb06646396e5bd354b3f' (SHA:0cf5902181e78341bb97bb06646396e5bd354b3f)
    25:  Complete job name: Validate PR Title
    26:  ##[group]Run thehanimo/pr-title-checker@0cf5902181e78341bb97bb06646396e5bd354b3f
    27:  with:
    28:  GITHUB_TOKEN: ***
    29:  configuration_path: .github/pr-title-checker-config.json
    30:  pass_on_octokit_error: false
    31:  ##[endgroup]
    32:  Using config file .github/pr-title-checker-config.json from repo pasha-codefresh/argo-cd [ref: 8bb41eda04d9a41a4fe7375888d77f91a68fd429]
    33:  Creating label (title needs formatting)...
    34:  Label (title needs formatting) already created.
    35:  ##[notice]PR Title needs to pass regex '^(feat|fix|docs|test|ci|chore)!?(\(.*\))?!?:.*
    36:  Adding label (title needs formatting) to PR...
    37:  HttpError: Resource not accessible by integration
    38:  ##[error]Failed to add label (title needs formatting) to PR
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    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.

    1 participant