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 release-5.3] June template application #6332

Open
wants to merge 1 commit into
base: release-5.3
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 workflow in .github/workflows/release.yml to improve Docker handling and parameter fetching:
    • Replaced the fetch-author step with set_outputs to set commit author and branch.
    • Updated environment variable names for NFPM passphrase.
    • Changed Docker build and push steps to use docker/build-push-action@v5.
    • Modified the test-controller-api job to use curl for fetching parameters.
    • Updated matrix configurations and environment variables in the api-tests job.
  • Added new build configuration for FIPS-compliant binaries in ci/goreleaser/goreleaser.yml.
  • Included new NFPM package configuration for FIPS builds in ci/goreleaser/goreleaser.yml.

Changes walkthrough 📝

Relevant files
Configuration changes
release.yml
Update CI workflow for improved Docker handling and parameter
fetching.

.github/workflows/release.yml

  • Replaced fetch-author step with set_outputs to set commit author and
    branch.
  • Updated environment variable names for NFPM passphrase.
  • Changed Docker build and push steps to use
    docker/build-push-action@v5.
  • Modified test-controller-api job to use curl for fetching parameters.
  • Updated matrix configurations and environment variables in api-tests
    job.
  • +61/-99 
    Enhancement
    goreleaser.yml
    Add FIPS build configuration and packaging.                           

    ci/goreleaser/goreleaser.yml

  • Added new build configuration for FIPS-compliant binaries.
  • Included new NFPM package configuration for FIPS builds.
  • +75/-0   

    💡 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 environment variable name change from NFPM_STD_PASSPHRASE to NFPM_PASSPHRASE might cause issues if not handled properly in all places where it's used.

    Configuration Consistency:
    Ensure that the new configurations and environment variables introduced in the api-tests job are correctly used and propagated throughout the system.

    Dependency Management:
    The changes in Docker build and push steps to use docker/build-push-action@v5 need careful review to ensure that all parameters and configurations are correctly set up.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Mask sensitive data in the GitHub Actions logs

    Ensure that sensitive data such as NFPM_PASSPHRASE is not logged or exposed in the build
    logs. Consider masking this value in the GitHub Actions logs.

    .github/workflows/release.yml [98]

    -echo "NFPM_PASSPHRASE=$NFPM_PASSPHRASE" >> $GITHUB_OUTPUT
    +echo "NFPM_PASSPHRASE=***" >> $GITHUB_OUTPUT
     
    Suggestion importance[1-10]: 10

    Why: Masking sensitive data in logs is crucial for security. Exposing NFPM_PASSPHRASE in logs can lead to security vulnerabilities. This suggestion addresses a significant security concern.

    10
    Validate or sanitize environment variables used in shell scripts to prevent script injection

    To avoid potential script injection vulnerabilities, validate or sanitize inputs used in
    shell scripts. Ensure that HEAD_REF and other environment variables are safe to use in the
    context of shell commands.

    .github/workflows/release.yml [54-57]

     HEAD_REF: ${{github.head_ref}}
     run: |
    -  echo "branch=${HEAD_REF##*/}" >> $GITHUB_OUTPUT
    +  # Ensure HEAD_REF is safe to use
    +  sanitized_HEAD_REF=$(echo "${HEAD_REF}" | sed 's/[^a-zA-Z0-9_\-]//g')
    +  echo "branch=${sanitized_HEAD_REF##*/}" >> $GITHUB_OUTPUT
     
    Suggestion importance[1-10]: 9

    Why: Sanitizing environment variables used in shell scripts is important to prevent script injection vulnerabilities. This suggestion enhances the security of the script by ensuring that HEAD_REF is safe to use.

    9
    Best practice
    Use a specific version for the runs-on attribute to ensure consistent environments

    It's recommended to use a more specific tag than ubuntu-latest for the runs-on attribute
    to ensure consistent environments across different runs. Using a specific version helps
    avoid unexpected failures due to environment updates.

    .github/workflows/release.yml [172]

    -runs-on: ubuntu-latest
    +runs-on: ubuntu-20.04
     
    Suggestion importance[1-10]: 8

    Why: Using a specific version for the runs-on attribute ensures a consistent environment, reducing the risk of unexpected failures due to updates in the ubuntu-latest tag. This is a best practice for CI/CD pipelines.

    8
    Performance
    Use the default sh shell unless bash-specific features are required

    The shell attribute is set to bash but the run command uses curl which might not require
    bash specifically. Consider using the default sh unless bash-specific features are needed
    to optimize the performance.

    .github/workflows/release.yml [52-56]

    -shell: bash
    +shell: sh
     run: |
       echo "commit_author=$(git show -s --format='%ae' HEAD)" >> $GITHUB_OUTPUT
     
    Suggestion importance[1-10]: 5

    Why: While using sh instead of bash can optimize performance slightly, the current script uses bash-specific syntax. The suggestion is minor and does not address a critical issue.

    5

    Copy link

    sonarcloud bot commented Jun 10, 2024

    @alephnull alephnull force-pushed the releng/release-5.3 branch 2 times, most recently from 54582d1 to 27641ca Compare June 10, 2024 12:44
    @buger buger force-pushed the releng/release-5.3 branch 2 times, most recently from b1c05b5 to 8c013fd Compare June 11, 2024 04:11
    @buger buger force-pushed the releng/release-5.3 branch 2 times, most recently from 14f1a3a to d673fe6 Compare July 3, 2024 07:10
    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