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

Patch apt source typo #8

Closed

Conversation

asoehnlein
Copy link

@asoehnlein asoehnlein commented Apr 17, 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

bug_fix, enhancement


Description

  • Refactored RBAC enforcement and project fetching in the application server to improve error handling and messaging.
  • Updated CLI commands to use sourcePosition instead of sourceIndex for clarity in multi-source applications.
  • Enhanced error handling with a new error type for unauthorized application project access.
  • Updated Kubernetes tested versions documentation for Argo CD v2.11.
  • Updated Swagger JSON and ProtoBuf definitions to support new application manifest query parameters.
  • Updated Dockerfile base image source URL.
  • Updated Argo CD image tags to v2.11.0-rc1 across various manifests.

Changes walkthrough

Relevant files
Enhancement
8 files
application.go
Refactor RBAC enforcement and project fetching in application server

server/application/application.go

  • Refactored getAppEnforceRBAC to return *appv1.AppProject along with
    *appv1.Application and error.
  • Updated getApplicationEnforceRBACInformer and
    getApplicationEnforceRBACClient to support the new return type from
    getAppEnforceRBAC.
  • Added getAppProject method to fetch the application's project and
    enforce project restrictions.
  • Modified validateAndNormalizeApp to accept *appv1.AppProject as an
    additional parameter.
  • +95/-104
    app.go
    Update source index handling to source position in CLI commands

    cmd/argocd/commands/app.go

  • Changed sourceIndex to sourcePosition to clarify the meaning in
    multi-source applications.
  • Adjusted the handling of source positions in NewApplicationSetCommand
    and NewApplicationUnsetCommand.
  • +88/-88 
    openapi_generated.go
    Generate OpenAPI definitions for new application project error type

    pkg/apis/application/v1alpha1/openapi_generated.go

  • Generated OpenAPI definitions for new
    ErrApplicationNotAllowedToUseProject type.
  • +35/-0   
    zz_generated.deepcopy.go
    Add deep copy method for new application project error type

    pkg/apis/application/v1alpha1/zz_generated.deepcopy.go

    • Added deep copy method for ErrApplicationNotAllowedToUseProject.
    +16/-0   
    app_project_types.go
    Introduce error type for unauthorized application project access

    pkg/apis/application/v1alpha1/app_project_types.go

  • Added ErrApplicationNotAllowedToUseProject type to represent errors
    related to application access to projects.
  • +18/-0   
    argo.go
    Use new error type for unauthorized project access in utility
    functions

    util/argo/argo.go

  • Utilize ErrApplicationNotAllowedToUseProject for unauthorized project
    access errors.
  • +1/-2     
    generated.proto
    Add ProtoBuf message for new application project error type

    pkg/apis/application/v1alpha1/generated.proto

    • Added ErrApplicationNotAllowedToUseProject message.
    +3/-0     
    application.proto
    Update ProtoBuf for application manifest query enhancements

    server/application/application.proto

  • Added sourcePositions and revisions fields to
    ApplicationManifestQuery.
  • +2/-1     
    Tests
    5 files
    application_test.go
    Update application server tests for project access control

    server/application/application_test.go

  • Updated test cases to reflect new error messaging for unauthorized
    project access.
  • Added tests for application sync window and list of links in namespace
    with project access control.
  • +114/-4 
    app_test.go
    Update app utility tests for source position handling       

    cmd/util/app_test.go

    • Updated tests to use sourcePosition instead of sourceIndex.
    +18/-18 
    declarative_test.go
    Adjust e2e test for application with invalid project         

    test/e2e/declarative_test.go

  • Commented out deletion test for application with invalid project due
    to new enforcement logic.
  • +8/-6     
    app_management_ns_test.go
    Update e2e namespace test for new project access error messaging

    test/e2e/app_management_ns_test.go

  • Updated test expectation to match new error messaging for unauthorized
    project access.
  • +1/-1     
    app_management_test.go
    Update e2e test for new project access error messaging     

    test/e2e/app_management_test.go

  • Updated test expectation to match new error messaging for unauthorized
    project access.
  • +1/-1     
    Configuration changes
    6 files
    namespace-install.yaml
    Update Argo CD image tags in HA namespace install manifest

    manifests/ha/namespace-install.yaml

    • Updated Argo CD image tags to v2.11.0-rc1.
    +7/-7     
    namespace-install.yaml
    Update Argo CD image tags in namespace install manifest   

    manifests/namespace-install.yaml

    • Updated Argo CD image tags to v2.11.0-rc1.
    +7/-7     
    Dockerfile
    Update base image source in Dockerfile                                     

    Dockerfile

    • Updated base image source URL.
    +1/-1     
    kustomization.yaml
    Update Argo CD image tags in HA base kustomization             

    manifests/ha/base/kustomization.yaml

    • Updated Argo CD image tags to v2.11.0-rc1.
    +1/-1     
    kustomization.yaml
    Update Argo CD image tags in base kustomization                   

    manifests/base/kustomization.yaml

    • Updated Argo CD image tags to v2.11.0-rc1.
    +1/-1     
    kustomization.yaml
    Update Argo CD image tags in core install kustomization   

    manifests/core-install/kustomization.yaml

    • Updated Argo CD image tags to v2.11.0-rc1.
    +1/-1     
    Documentation
    8 files
    argocd_app_diff.md
    Update app diff command documentation for source position changes

    docs/user-guide/commands/argocd_app_diff.md

  • Updated documentation to reflect changes from source-indexes to
    source-positions.
  • +12/-12 
    argocd_app_manifests.md
    Update app manifests command documentation for source position changes

    docs/user-guide/commands/argocd_app_manifests.md

  • Updated documentation to reflect changes from source-indexes to
    source-positions.
  • +8/-8     
    argocd_app_set.md
    Update app set command documentation for source position changes

    docs/user-guide/commands/argocd_app_set.md

  • Updated documentation to reflect changes from source-index to
    source-position.
  • +3/-3     
    argocd_app_unset.md
    Update app unset command documentation for source position changes

    docs/user-guide/commands/argocd_app_unset.md

  • Updated documentation to reflect changes from source-index to
    source-position.
  • +3/-3     
    argocd_app_remove-source.md
    Update app remove-source command documentation for source position
    changes

    docs/user-guide/commands/argocd_app_remove-source.md

  • Updated documentation to reflect changes from source-index to
    source-position.
  • +4/-4     
    argocd_app.md
    Update app command documentation for source position changes

    docs/user-guide/commands/argocd_app.md

  • Updated documentation to reflect changes from source-index to
    source-position.
  • +1/-1     
    tested-kubernetes-versions.md
    Update tested Kubernetes versions for Argo CD v2.11           

    docs/operator-manual/tested-kubernetes-versions.md

    • Updated tested Kubernetes versions for Argo CD v2.11.
    +3/-4     
    swagger.json
    Update Swagger JSON for application manifest query parameters

    assets/swagger.json

  • Updated Swagger JSON to include new query parameters for application
    manifest query.
  • +19/-0   
    Other
    1 files
    VERSION
    Update Argo CD version to v2.11.0-rc1                                       

    VERSION

    • Updated Argo CD version to v2.11.0-rc1.
    +1/-1     

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

    github-actions bot and others added 6 commits April 5, 2024 15:08
    Signed-off-by: GitHub <[email protected]>
    Co-authored-by: pasha-codefresh <[email protected]>
    …goproj#17753)
    
    * chore: rename source-indexes to source-positions
    
    
    
    * update documentation
    
    
    
    ---------
    
    Signed-off-by: ishitasequeira <[email protected]>
    Co-authored-by: Ishita Sequeira <[email protected]>
    …y fields in swagger (argoproj#17804) (argoproj#17820)
    
    * use arrays instead of map to display ApplicationManifetQuery fields in swagger
    
    
    
    * fix equality conditions for souce-position check
    
    
    
    ---------
    
    Signed-off-by: ishitasequeira <[email protected]>
    Co-authored-by: Ishita Sequeira <[email protected]>
    * sec: validate a project before execute an action
    
    Signed-off-by: pashakostohrys <[email protected]>
    
    * sec: validate a project before execute an action
    
    Signed-off-by: pashakostohrys <[email protected]>
    
    ---------
    
    Signed-off-by: pashakostohrys <[email protected]>
    Signed-off-by: pashakostohrys <[email protected]>
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request bug_fix labels Apr 17, 2024
    Copy link

    PR Description updated to latest commit (47b26cf)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR introduces a significant number of changes across various files, including updates to documentation, Dockerfiles, Kubernetes manifests, and Go source files. The complexity of the changes, especially those related to the application and project APIs, security concerns, and the introduction of new error handling, requires a thorough review to ensure compatibility, security, and adherence to best practices.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The changes introduce a new error type ErrApplicationNotAllowedToUseProject but it's not clear if all instances where this error should be used have been updated. This requires a thorough search and update of all relevant error handling to ensure consistency.

    Security Concern: The modifications in Dockerfile and Kubernetes manifests involve changes to base images and versions. It's crucial to ensure that these new images are free from vulnerabilities and are compatible with the existing system requirements.

    Compatibility Issue: The update in Kubernetes versions supported by different Argo CD versions in the documentation needs to be cross-verified with actual compatibility tests to ensure that users do not face deployment issues.

    🔒 Security concerns

    - Base Image Update: The update to the base image in the Dockerfile needs to be checked for any known vulnerabilities. It's important to ensure that the new base image is secure and does not introduce any new security risks.

    • Hardcoded Versions: The hardcoded version changes in Kubernetes manifests and Dockerfiles might lead to potential security risks if these versions contain vulnerabilities. It's recommended to have a dynamic way of updating these versions to the latest secure versions automatically.

    ✨ 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                                                                                                                                                       
    Possible issue
    Add nil check for error before accessing its properties.

    Consider checking if err is not nil before calling err.Error() to avoid potential nil
    pointer dereference.

    server/application/application_test.go [1822]

    -assert.Contains(t, err.Error(), "not exist")
    +if err != nil {
    +    assert.Contains(t, err.Error(), "not exist")
    +}
     
    Address API rule violations for ErrApplicationNotAllowedToUseProject.

    Review the newly added API rule violations related to ErrApplicationNotAllowedToUseProject
    to ensure they adhere to Kubernetes API conventions, possibly requiring renaming or
    additional documentation.

    pkg/apis/api-rules/violation_exceptions.list [102-104]

    -API rule violation: names_match,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ErrApplicationNotAllowedToUseProject,application
    -API rule violation: names_match,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ErrApplicationNotAllowedToUseProject,namespace
    -API rule violation: names_match,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ErrApplicationNotAllowedToUseProject,project
    +// Potential resolution could involve renaming fields or adding necessary documentation to clear the API rule violation.
     
    Maintainability
    Use constants for repeated string literals.

    Use a constant for the repeated string literals like "argocd-1", "other-ns", and "*" to
    improve maintainability and reduce typos.

    server/application/application_test.go [2533-2540]

    -testApp.Namespace = "argocd-1"
    -testApp.Spec.Project = "other-ns"
    +const (
    +    namespaceArgoCD1 = "argocd-1"
    +    projectOtherNS = "other-ns"
    +    wildcard = "*"
    +)
    +testApp.Namespace = namespaceArgoCD1
    +testApp.Spec.Project = projectOtherNS
     otherNsProj := &appsv1.AppProject{
    -    ObjectMeta: metav1.ObjectMeta{Name: "other-ns", Namespace: "default"},
    +    ObjectMeta: metav1.ObjectMeta{Name: projectOtherNS, Namespace: "default"},
         Spec: appsv1.AppProjectSpec{
    -        SourceRepos:      []string{"*"},
    -        Destinations:     []appsv1.ApplicationDestination{{Server: "*", Namespace: "*"}},
    -        SourceNamespaces: []string{"argocd-1"},
    +        SourceRepos:      []string{wildcard},
    +        Destinations:     []appsv1.ApplicationDestination{{Server: wildcard, Namespace: wildcard}},
    +        SourceNamespaces: []string{namespaceArgoCD1},
         },
     }
     
    Best practice
    Handle errors returned by function calls instead of ignoring them.

    Instead of ignoring the error returned by SetAppSpecOptions, consider handling it to catch
    potential issues early.

    cmd/util/app_test.go [182]

    -_ = SetAppSpecOptions(f.command.Flags(), f.spec, f.options, sourcePosition)
    +if err := SetAppSpecOptions(f.command.Flags(), f.spec, f.options, sourcePosition); err != nil {
    +    return fmt.Errorf("failed to set app spec options: %w", err)
    +}
     
    Add cleanup logic for consistent test environment setup.

    Since deletion of applications with invalid projects is not allowed, consider adding a
    cleanup step in the test setup to ensure consistent test environment.

    test/e2e/declarative_test.go [58-59]

    -// TODO: you can`t delete application with invalid project due to enforcment that was recently added,
    -// in https://github.com/argoproj/argo-cd/security/advisories/GHSA-2gvw-w6fj-7m3c
    +// Cleanup logic here to ensure consistent test environment
     
    Use a stable version of the Argo CD image for production environments.

    Consider using a stable version of the Argo CD image instead of a release candidate for
    production environments to ensure stability and reliability.

    manifests/ha/namespace-install.yaml [1671]

    -image: quay.io/argoproj/argocd:v2.11.0-rc1
    +image: quay.io/argoproj/argocd:v2.11.0
     
    Use the original Debian repository URL for buster-backports to ensure up-to-date and secure packages.

    Consider using the original Debian repository URL (http://deb.debian.org/debian) instead
    of the archive URL for the buster-backports source. The archive URL is typically used for
    older releases that are no longer supported, which might not receive security updates or
    might contain outdated packages.

    Dockerfile [9]

    -RUN echo 'deb http://archive.debian.org/debian buster-backports main' >> /etc/apt/sources.list
    +RUN echo 'deb http://deb.debian.org/debian buster-backports main' >> /etc/apt/sources.list
     
    Document the usage and format of new fields in ApplicationManifestQuery.

    When adding sourcePositions and revisions fields to the ApplicationManifestQuery message,
    ensure that their usage is clear and documented. For revisions, consider specifying the
    expected format of the strings, especially if they are expected to follow a particular
    versioning scheme or pattern.

    server/application/application.proto [72-73]

    +// Positions of the sources in the application's source list. Counting starts at 1.
     repeated int64 sourcePositions = 5;
    +// Revisions of the application's sources, expected to follow semantic versioning.
     repeated string revisions = 6;
     
    Enhancement
    Export struct fields to enable external access.

    Make the fields of ErrApplicationNotAllowedToUseProject exported to allow access from
    outside the package.

    pkg/apis/application/v1alpha1/app_project_types.go [20-23]

     type ErrApplicationNotAllowedToUseProject struct {
    -    application string
    -    namespace   string
    -    project     string
    +    Application string
    +    Namespace   string
    +    Project     string
     }
     
    Add explicit handling for when an application namespace is permitted to use a project.

    Consider handling the case where proj.IsAppNamespacePermitted(app, ns) returns true
    explicitly to improve code readability and maintainability.

    util/argo/argo.go [702-704]

    -if !proj.IsAppNamespacePermitted(app, ns) {
    +if proj.IsAppNamespacePermitted(app, ns) {
    +    // Handle the permitted case explicitly here
    +} else {
         return nil, argoappv1.NewErrApplicationNotAllowedToUseProject(app.Name, app.Namespace, proj.Name)
     }
     
    Use "Index starts with 1" for clarity and consistency in command documentation.

    To maintain consistency and clarity in documentation, consider reverting the term
    "Counting starts with 1" back to "Index starts with 1" in the description of the argocd
    app remove-source command. This change would align with common terminology used in
    programming and documentation, making it easier for users to understand.

    docs/user-guide/commands/argocd_app_remove-source.md [5]

    -Remove a source from multiple sources application. Counting starts with 1. Default value is -1.
    +Remove a source from multiple sources application. Index starts with 1. Default value is -1.
     
    Add a field to the error message for more informative debugging.

    For the new ErrApplicationNotAllowedToUseProject message, consider adding at least one
    field to provide more context about the error, such as a string describing the reason.
    Empty messages can be less informative for developers trying to debug or handle errors in
    their applications.

    pkg/apis/application/v1alpha1/generated.proto [907-908]

     message ErrApplicationNotAllowedToUseProject {
    +  string reason = 1;
     }
     
    Documentation
    Clarify the documentation for the --source-positions option.

    Update the documentation to clarify the use of --source-positions option, ensuring it's
    clear how it differs from the previous --source-indexes and its impact on command
    behavior.

    docs/user-guide/commands/argocd_app_diff.md [32]

    ---source-positions int64Slice   List of source positions. Default is empty array. Counting start at 1. (default [])
    +--source-positions int64Slice   List of source positions, specifying the order of sources as defined in the application. Counting starts at 1. This option is useful for multi-source applications. (default [])
     
    Update documentation for the --source-position flag to ensure clarity and consistency.

    Ensure consistency in documentation by updating examples and descriptions to reflect the
    new --source-position flag, providing clear guidance on its usage.

    docs/user-guide/commands/argocd_app_set.md [82]

    ---source-position int                        Position of the source from the list of sources of the app. Counting starts at 1. (default -1)
    +--source-position int                        Specifies the position of the source within the application's source list, starting from 1. Use this flag to target specific sources in multi-source applications. (default -1)
     
    Add descriptions to new Swagger parameters for better API documentation.

    For the new sourcePositions and revisions parameters in the Swagger definition, ensure
    that their descriptions are added to the Swagger JSON to improve API documentation. Clear
    descriptions help API consumers understand the purpose and usage of these parameters.

    assets/swagger.json [986-997]

     "name": "sourcePositions",
    -"in": "query"
    +"in": "query",
    +"description": "Positions of the sources in the application's source list, counting starts at 1."
     "name": "revisions",
    -"in": "query"
    +"in": "query",
    +"description": "Revisions of the application's sources, expected to follow semantic versioning."
     

    ✨ 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.

    @asoehnlein
    Copy link
    Author

    Fixed argoproj#17886

    @asoehnlein asoehnlein closed this Apr 19, 2024
    @asoehnlein asoehnlein deleted the patch-apt-source-typo branch April 19, 2024 13:25
    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