-
-
Notifications
You must be signed in to change notification settings - Fork 549
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(docker): Checkov installation silently fails on docker build in arm64. Workaround till issue will be fixed in checkov itself
#635
Merged
MaxymVlasov
merged 20 commits into
antonbabenko:master
from
antm-pp:fix/docker-checkov-install-aarch64
Mar 11, 2024
Merged
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
5cf0ca5
fix-workaround: Checkov install fails aarch64. Awaiting checkov versi…
antm-pp 63f4bef
fix: Docker Checkov - Keep libgcc and remove gcc for compatibility wi…
antm-pp 7014fa0
fix:checkob install - pinned apk versions
antm-pp 3d85490
Add comments on package dependencies and re-order
antm-pp a13a983
fix: Docker Checkov install: Added gcc container check
antm-pp 42d77d9
fix: Docker checkov install: reorder values and comments on package i…
antm-pp f46e8f8
fix: Docker Checkov Install: Container structure test: regex excape c…
antm-pp 9b58092
Update .github/.container-structure-test-config.yaml
antm-pp 3e0d679
fix: Docker Checkov Install: Use git fetch for crates for consistency…
antm-pp b5c6379
Merge branch 'master' into fix/docker-checkov-install-aarch64
MaxymVlasov 54e5ea2
fix(dockerfile): Deal with "successful silent fails" during `docker b…
MaxymVlasov 8798d3b
fix Checkov?
MaxymVlasov c3322d0
Try use checkov binary. Checkov still needs all deps installed by pip
MaxymVlasov 875999c
Revert "Try use checkov binary. Checkov still needs all deps installe…
MaxymVlasov 3481201
Revert "fix Checkov?"
MaxymVlasov c4c1b16
Merge remote-tracking branch 'upstream/fix/dockerfile' into fix/docke…
MaxymVlasov 2f00dcf
fix silent fail
MaxymVlasov 5e98ae3
test
MaxymVlasov 1ca976a
Apply suggestions from code review
MaxymVlasov 01d0868
Apply suggestions from code review
MaxymVlasov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions: Did you try to remove each of these dependencies, build an image, and confirm that is a minimal setup?
Please add check for GCC in https://github.com/antonbabenko/pre-commit-terraform/blob/master/.github/.container-structure-test-config.yaml
Also, I think that we need to find a way to run these tests on arm64 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is minimal dependencies. So gcc was already present in the failing build, as used for same approach by cffi compile. Tried adding only rust, and it errored cargo still missing.
Guidance for rustworkx indicates need for compiler including rust and cargo (or to use rustup a cross platform installer).
Noticed when the pre-existing purge of gcc occurred it caused exception in running checkov in the build container, therefore applied libgcc seperately to minimise that dependency and it executes ok (for the version check). When referring to the final image I noted that gcc in full is already a dependency for the pre-commit hooks. I couldn't see obviously where it's documented what hook that dependency is for. I could add a further comment to highlight it's at least needed for checkov.
Happy to add a gcc check as requested, although I've not added the gcc dependency in the final image.
Just to note, the linux/arm64 builds have been failing for sometime, an example from 2 months ago: https://github.com/antonbabenko/pre-commit-terraform/actions/runs/7183944518/job/19563861227#step:9:741
I'm just running through some tests, it looks like the linux/arm64 build is failing because it can't pull crates.io. I tried one recommended test saying to set env-var CARGO_NET_GIT_FETCH_WITH_CLI=true which then created a dependency on git (which again is already in the final image but not the builder). Just trying a run with that, so that darwin/arm64 and linux/arm64 can both compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, really? I see removal of rust and cargo, but not libgcc. How it works then 🤔
If there no package at the end - then there nothing to test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I'm missing some bits of context:
gcc!=libcc. The former is the compiler collection and the latter is runtime libs only — https://pkgs.alpinelinux.org/package/edge/main/x86/gcc vs https://pkgs.alpinelinux.org/package/edge/main/x86/libgccThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, we can't just add tests for macos, as Structure Test currently not support macos
#636
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the issue is the structure of the '||' command for the install. When the rust compiler call fails it generates a false allowing the other part of the command to run (intended for managing checkov==latest vs checkov==). The 2nd part has its own failure mode that doesn't actually generated an exit1. So the error gets buried and the build passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all these combinations should be rewritten to vanilla if-then-else statements? @yermulnik
pre-commit-terraform/Dockerfile
Lines 70 to 71 in c29bdb1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yep, that sort of "ternary" in Bash is not vanilla
if/elseand has this kind of "discomfortable" hiccups 😿And, yes, these need to be re-written 😢 Either using vanilla
if/else, or like below:ps: I probably can try and do that, though I will need help building it and testing resulting images (@MaxymVlasov, that would be super great if you already had that automation so that I can push changes and you test build/run 🤪).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or like below 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yermulnik done in this PR. Also, I can confirm that without @antm-pp changes to rust-cargo, it fails in arm64, when with - checks passes.
And now we have prevention of silent fails for checkov - #635 (review)