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

[SYSE-370 master] June template application #6331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alephnull
Copy link
Contributor

@alephnull alephnull commented Jun 10, 2024

User description

  • Implement custom rules for enterprise artifacts in package promotion
  • Test controller UI
  • Update combination matrix of config used in API/UI pipelines

PR Type

Enhancement, Configuration changes


Description

  • Updated the CI/CD workflow to enhance Docker support, including multi-architecture builds, improved metadata, and tagging.
  • Added a new build configuration for FIPS-compliant binaries in the GoReleaser configuration.
  • Enhanced the Dockerfile for the distroless image by removing FIPS-related deb files before installation.
  • Refined the test-controller-api job steps and environment variables for better integration and reliability.
  • Removed the distroless job and integrated its steps into other jobs for a more streamlined workflow.

Changes walkthrough 📝

Relevant files
Enhancement
release.yml
Update CI/CD workflow for enhanced Docker support and metadata

.github/workflows/release.yml

  • Updated the commit_author output step.
  • Added new outputs and environment variables for branch and commit
    author.
  • Modified cache key and restore-keys for Go modules.
  • Updated Docker build and push steps, including multi-architecture
    support.
  • Enhanced metadata and tagging for Docker images.
  • Refined test-controller-api job steps and environment variables.
  • Removed the distroless job and integrated its steps into other jobs.
  • +96/-101
    Dockerfile.distroless
    Enhance Dockerfile for distroless image                                   

    ci/Dockerfile.distroless

  • Added a step to remove FIPS-related deb files before installation.
  • Minor formatting changes.
  • +2/-1     
    goreleaser.yml
    Add FIPS build configuration and update package metadata 

    ci/goreleaser/goreleaser.yml

  • Added a new build configuration for FIPS-compliant binaries.
  • Updated package metadata and contents for FIPS builds.
  • Removed redundant Docker build configurations.
  • +75/-116

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

    Copy link
    Contributor

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    4

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The change in environment variable name from NFPM_STD_PASSPHRASE to NFPM_PASSPHRASE might cause issues if not handled properly in all places where it is used.

    Configuration Consistency:
    The removal of distroless job and its integration into other jobs needs careful review to ensure all intended functionalities are preserved.

    Dependency Management:
    Changes in cache keys for Go modules might affect the build reproducibility and need to be reviewed to ensure they align with the project's dependency management strategies.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Assign a unique binary name for the FIPS build to prevent conflicts

    Ensure that the fips-linux build configuration specifies a unique binary name to avoid
    conflicts with other builds, especially since the current configuration duplicates the
    binary name used in other profiles.

    ci/goreleaser/goreleaser.yml [39]

    -binary: tyk
    +binary: tyk-fips
     
    Suggestion importance[1-10]: 10

    Why: Assigning a unique binary name for the FIPS build is crucial to prevent conflicts with other builds. This change addresses a potential bug that could cause build issues.

    10
    Improve the reliability of capturing the commit_author

    Ensure that the commit_author is correctly captured by using the correct git command. The
    current implementation might fail if the HEAD pointer is not properly set, especially in
    different CI contexts where HEAD may not point to the expected commit.

    .github/workflows/release.yml [56]

    -echo "commit_author=$(git show -s --format='%ae' HEAD)" >> $GITHUB_OUTPUT
    +echo "commit_author=$(git log -1 --pretty=format:'%ae')" >> $GITHUB_OUTPUT
     
    Suggestion importance[1-10]: 8

    Why: The suggestion improves the reliability of capturing the commit_author by using a more robust git command, which is crucial in CI contexts where the HEAD pointer might not be set correctly.

    8
    Security
    Sanitize the BASE_REF variable to prevent potential security risks

    To avoid potential script injection or execution errors, validate or sanitize the BASE_REF
    environment variable before using it in the curl command within the GitHub Actions
    workflow.

    .github/workflows/release.yml [222]

    -endpoint="http://tui.internal.dev.tyk.technology/api/tyk/$BASE_REF/${{ github.event_name}}/api"
    +sanitized_base_ref=$(echo "$BASE_REF" | sed 's/[^a-zA-Z0-9_\-]//g')
    +endpoint="http://tui.internal.dev.tyk.technology/api/tyk/$sanitized_base_ref/${{ github.event_name}}/api"
     
    Suggestion importance[1-10]: 10

    Why: Sanitizing the BASE_REF variable is a crucial security improvement to prevent potential script injection or execution errors, which is highly important in CI/CD pipelines.

    10
    Possible issue
    Remove duplicate build flags to clean up the configuration

    Remove the duplicate -tags=goplugin flag from the flags list for the fips-linux build
    configuration to avoid redundancy and potential confusion.

    ci/goreleaser/goreleaser.yml [26-27]

     - -trimpath
    -- -tags=goplugin
     - -tags=goplugin,fips,boringcrypto
     
    Suggestion importance[1-10]: 9

    Why: Removing the duplicate -tags=goplugin flag is important for avoiding redundancy and potential confusion in the build configuration. This change improves clarity and maintainability.

    9
    Enhancement
    Consolidate build tags into a single entry for clarity

    Combine the -tags flags into a single entry to streamline the build configuration and
    ensure clarity in the build process.

    ci/goreleaser/goreleaser.yml [26-27]

     - -trimpath
    -- -tags=goplugin
     - -tags=goplugin,fips,boringcrypto
     
    Suggestion importance[1-10]: 8

    Why: Consolidating the -tags flags into a single entry enhances readability and reduces the risk of misconfiguration. This is a good practice for maintaining clear and concise build configurations.

    8
    Update the conditional check to use current matrix strategy variables

    Replace the deprecated matrix.golang_cross with a new environment variable or matrix
    strategy that reflects the current configurations, as the old variable is no longer
    visible in the new code context.

    .github/workflows/release.yml [141]

    -if: ${{ matrix.golang_cross == '1.21-bullseye' }}
    +if: ${{ matrix.go_version == '1.21' && matrix.os == 'bullseye' }}
     
    Suggestion importance[1-10]: 7

    Why: This suggestion updates the conditional check to reflect the current matrix strategy variables, improving code clarity and future-proofing against deprecated variables.

    7
    Maintainability
    Improve the Dockerfile's clarity and maintainability by separating the RUN commands

    Ensure that the removal of specific .deb files and the installation process are handled
    more cleanly by separating the commands for better readability and maintenance.

    ci/Dockerfile.distroless [9]

    -RUN rm -f /*fips*.deb && dpkg -i /tyk-gateway*${TARGETARCH}.deb && rm /*.deb
    +RUN rm -f /*fips*.deb
    +RUN dpkg -i /tyk-gateway*${TARGETARCH}.deb
    +RUN rm /*.deb
     
    Suggestion importance[1-10]: 6

    Why: Separating the RUN commands enhances the readability and maintainability of the Dockerfile, although it is a minor improvement in terms of functionality.

    6

    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    @alephnull alephnull force-pushed the releng/master branch 2 times, most recently from 3c20672 to 1ac29f2 Compare June 10, 2024 11:41
    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    Copy link

    sonarcloud bot commented Jul 3, 2024

    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.

    None yet

    1 participant