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

fix: set envs only when passed #405

Merged
merged 3 commits into from
Oct 11, 2024
Merged

fix: set envs only when passed #405

merged 3 commits into from
Oct 11, 2024

Conversation

knqyf263
Copy link
Contributor

@knqyf263 knqyf263 commented Oct 9, 2024

Problem

Currently, all TRIVY_XXX environment variables are always passed to entrypoint.sh, which prevents configurations set in trivy.yaml from being correctly applied. This occurs because environment variables take precedence over configuration files.

Proposed Solution

This PR implements a workaround to set environment variables only when values are explicitly provided. However, there's currently no reliable method to determine if a value has been passed in GitHub Actions (see actions/runner#924).

Implementation Details

  1. We now compare input values against their defaults and only set the corresponding environment variables if they differ.
  2. TRIVY_CACHE_DIR is an exception and is always set, as it requires a value different from Trivy's default.
    • Updated the README to clarify that TRIVY_CACHE_DIR cannot be controlled via trivy.yaml due to this implementation.

Testing

https://github.com/knqyf263/trivy-action-test/actions/runs/11250664411/job/31280013016

Signed-off-by: knqyf263 <knqyf263@gmail.com>
set_env_var_if_provided "TRIVY_EXIT_CODE" "${{ inputs.exit-code }}" ""
set_env_var_if_provided "TRIVY_IGNORE_UNFIXED" "${{ inputs.ignore-unfixed }}" "false"
set_env_var_if_provided "TRIVY_PKG_TYPES" "${{ inputs.vuln-type }}" "os,library"
set_env_var_if_provided "TRIVY_SEVERITY" "${{ inputs.severity }}" "UNKNOWN,LOW,MEDIUM,HIGH,CRITICAL"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking for it, but I couldn't. I'll try it. Thanks.

Copy link
Contributor Author

@knqyf263 knqyf263 Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is just a document about syntax of action.yaml. I don't think it's available, but I'll give it a shot.

Copy link
Contributor Author

@knqyf263 knqyf263 Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work. An empty string is filled.
1a12292

CleanShot 2024-10-09 at 13 59 27

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks that you checked that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bummer.... 🫤

Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 marked this pull request as ready for review October 9, 2024 10:17
@knqyf263 knqyf263 requested a review from DmitriyLewen October 9, 2024 10:17
@knqyf263 knqyf263 changed the title fix: set envs when passed fix: set envs only when passed Oct 9, 2024
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

# Set environment variables, handling those with default values
# cf. https://aquasecurity.github.io/trivy/latest/docs/configuration/#environment-variables
set_env_var_if_provided "TRIVY_INPUT" "${{ inputs.input }}" ""
set_env_var_if_provided "TRIVY_EXIT_CODE" "${{ inputs.exit-code }}" ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it isn't set, is the default equivalent to a zero for this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware that would be the default Trivy behavior (if exit code is not specified), I am just not sure how GitHub Actions interprets an empty string in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it isn't set, is the default equivalent to a zero for this case?

The action will not set TRIVY_EXIT_CODE and Trivy CLI just uses the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just not sure how GitHub Actions interprets an empty string in this case.

If the input is empty or the same as a default value, it doesn't set an environment variable.

@Roy-Gal-Git
Copy link

Roy-Gal-Git commented Oct 10, 2024

@knqyf263 Does your fix work for all TRIVY_XXX variables or you have to use set_env_var_if_provided for every one of them?

I'm encountering the TOOMANYREQUESTS issue #389.
I tried bumping trivy-action to v0.25.0 as people suggested and it didn't solve the problem.
Yesterday v0.26.0 was released where the caching mechanism was added by default but it didn't solve the problem either (well, makes sense as I'm still past the rate limit..).
When I tried adding env variables to set TRIVY_DB_REPOSITORY and TRIVY_JAVA_DB_REPOSITORY (on v0.26.0) I encountered this exact problem.

I don't see that TRIVY_DB_REPOSITORY and TRIVY_JAVA_DB_REPOSITORY are addressed in the code.

@knqyf263
Copy link
Contributor Author

TRIVY_DB_REPOSITORY and TRIVY_JAVA_DB_REPOSITORY shouldn't be affected by this problem.

@Roy-Gal-Git
Copy link

Roy-Gal-Git commented Oct 10, 2024

@knqyf263 Then how come I'm experiencing the problem when I set them?

      - name: Trivy vulnerability scan
        uses: aquasecurity/trivy-action@0.26.0
        with:
          scan-type: image
          image-ref: 'FILTERED'
          trivy-config: .trivy/trivy.yaml
        env:
          TRIVY_DB_REPOSITORY: public.ecr.aws/aquasecurity/trivy-db:2
          TRIVY_JAVA_DB_REPOSITORY: public.ecr.aws/aquasecurity/trivy-java-db:1

My trivy-config is ignored.

@Roy-Gal-Git
Copy link

What I understand is that whenever any TRIVY_XXX variable is set trivy-action would completely ignore action inputs.

@knqyf263
Copy link
Contributor Author

What I understand is that whenever any TRIVY_XXX variable is set trivy-action would completely ignore action inputs.

No, only variables defined in input are affected.

@knqyf263
Copy link
Contributor Author

I tested and it worked.
https://github.com/knqyf263/trivy-action-test/actions/runs/11272540264/workflow

CleanShot 2024-10-10 at 14 58 45

@Roy-Gal-Git
Copy link

@knqyf263 Can you test it with trivy-config set too?

@knqyf263
Copy link
Contributor Author

Worked.
https://github.com/knqyf263/trivy-action-test/actions/runs/11273222891/workflow#L16

Please create a small repository to reproduce your issue.

@Roy-Gal-Git
Copy link

No need if it worked :) I'll wait for your merge. Thanks!

@simar7 simar7 self-requested a review October 11, 2024 04:48
@simar7 simar7 merged commit 5681af8 into master Oct 11, 2024
3 checks passed
@simar7 simar7 deleted the fix/config_file branch October 11, 2024 04:48
@Roy-Gal-Git
Copy link

I upgraded to 0.27.0 and now a new error was raised...

Error: Bad request - jaxxstorm/action-install-gh-release@v1.10.0 is not allowed to be used in FILTERED. Actions in this workflow must be: within a repository owned by FILTERED, created by GitHub, or verified in the GitHub Marketplace.

I know that there's an open issue (#409) about this bug.
Do you have a time estimation for this fix? I'm in the loop of bumping trivy-action versions and being held by new bugs that appear in each new version.

@knqyf263
Copy link
Contributor Author

knqyf263 commented Oct 14, 2024

I don't think it's a bug. It just depends on your configuration. We'll fix it anyway, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants