fix(docker): Prevent all possible "silent errors" during `docker build` #644

MaxymVlasov · 2024-03-11T21:28:48Z

Put an x into the box if that apply:

This PR introduces breaking change.
This PR fixes a bug.
This PR adds new functionality.
This PR enhances existing functionality.

Description of your changes

Because docker RUN runs shell w/o set -e I moved all installation logic to standalone bash scripts, as it was suggested in #635 (comment), and:

Rewrite them from sh to bash (because of no set pipefail in sh - https://www.shellcheck.net/wiki/SC3040)
Sort Dockerfile instructions alphabetically

MaxymVlasov

stderr: exec /usr/bin/git: exec format error

from https://github.com/antonbabenko/pre-commit-terraform/actions/runs/8239964468/job/22534539718?pr=644#step:7:16

https://stackoverflow.com/questions/73285601/docker-exec-usr-bin-sh-exec-format-error

So, free arm builders not available for GitHub at all? 🤔

yermulnik

Apart from the suggestions and some bits of nitpicking, this PR looks good to me.

.pre-commit-config.yaml

tools/install/checkov.sh

tools/install/hcledit.sh

tools/install/terrascan.sh

tools/install/trivy.sh

yermulnik · 2024-03-11T22:25:05Z

So, free arm builders not available for GitHub at all? 🤔

Get @antonbabenko to pay for some 😂

MaxymVlasov · 2024-03-11T22:28:38Z

Lol :)
Better he will pay us :D

Btw, ARM runners in private beta from Oct 2023

https://stackoverflow.com/a/77429129

yermulnik · 2024-03-11T22:34:50Z

Btw, ARM runners in private beta from Oct 2023

stackoverflow.com/a/77429129

It's still not released? =( Should we opt in to the waitlist https://resources.github.com/devops/accelerate-your-cicd-with-arm-and-gpu-runners-in-github-actions/#form ? 🤔

MaxymVlasov · 2024-03-11T22:37:53Z

Already joined, but don't think that it will be soon

https://blog.thesparktree.com/docker-multi-arch-github-actions#qemu-support

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

yermulnik · 2024-03-14T22:11:54Z

Dockerfile

+    if [ "$PRE_COMMIT_VERSION" = "false" ]; then echo "PRE_COMMIT_VERSION=latest" >> /.env; fi; \
+    if [ "$TERRAFORM_VERSION" = "false" ]; then echo "TERRAFORM_VERSION=latest" >> /.env; fi


I most probably am missing some context, though why switching from comparing value to latest? Is false assigned for a reason and somewhere else?

Just a few lines above answer to your question :)

Install required tools

If someone disables PRE_COMMIT_VERSION - the image will not work
Same for TF, till we don't add support for OpenTofu or other TF-derivative. I can't guarantee that any of our hooks can work without TF

Just a few lines above answer to your question :)

Oh, I think I got the idea. As always explanatory comment wouldn't come amiss.

Btw why specifically lowercase false has been chosen? What if they choose to set it to off, no, False, or whatever they can imagine may work to switch it off? =)

Btw why specifically lowercase false has been chosen?

Because only false is reserved word in bash

What if they choose to set it to off, no, False, or whatever they can imagine may work to switch it off? =)

Well, I can suggest to that folks RTFM :D
Btw, usually no-one would like to set anything to false, as everything that can be disabled, is set to false by default, if INSTALL_ALL=true not provided

As always explanatory comment wouldn't come amiss.

Write it then. It's too obvious to me that I have no idea what here should be explained

maybe tomorrow (c) you

?

Suggested change

if [ "$PRE_COMMIT_VERSION" = "false" ]; then echo "PRE_COMMIT_VERSION=latest" >> /.env; fi; \

if [ "$TERRAFORM_VERSION" = "false" ]; then echo "TERRAFORM_VERSION=latest" >> /.env; fi

if [ "$PRE_COMMIT_VERSION" = "false" ]; then echo "PRE_COMMIT_VERSION=''" >> /.env; fi; \

if [ "$TERRAFORM_VERSION" = "false" ]; then echo "TERRAFORM_VERSION=''" >> /.env; fi

Or you suggest to just get rid of these if's, and provide folks ability to shoot their legs if they want to?

pre-commit-terraform/tools/install/_common.sh

Lines 18 to 22 in 51cfcd9

# Skip tool installation if the version is set to "false"

if [[ $VERSION == false ]]; then

echo "'$TOOL' skipped"

exit 0

fi

I did not suggest either options. Initially I only asked to add a brief comment explaining why false is silently converted into latest because it wasn't obvious right away.
From my point of view the correct way to implement what you're implementing would be to check whether values of PRE_COMMIT_VERSION and TERRAFORM_VERSION are either latest or match their version formats. Else fail to prevent Docker build from completing successfully.

Now it explicitly fails if set PRE_COMMIT_VERSION or TERRAFORM_VERSION to false.

If there is be provided not-valid version - it will fail when will try to find such version

tools/install/_common.sh

yermulnik · 2024-03-14T22:25:27Z

tools/install/checkov.sh

+apk del gcc libffi-dev musl-dev
+apk del cargo git rust


I might have missed the answer from previous round of review, hence: why these two apk del commands cannot be combined into one apk del command? 🤔 Apologies if you already replied though. In such case it defo makes sense to add an explanatory comment so that I stop asking again and again =) Thanks.

Should always be:

pre-commit-terraform/tools/install/checkov.sh

Lines 11 to 14 in 65ba593

apk add --no-cache \

gcc=~12 \

libffi-dev=~3 \

musl-dev=~1

pre-commit-terraform/tools/install/checkov.sh

Line 33 in 65ba593

apk del gcc libffi-dev musl-dev

Should be removed when the issue in Checkov will be fixed:

pre-commit-terraform/tools/install/checkov.sh

Lines 16 to 25 in 65ba593

# cargo, gcc, git, musl-dev, rust and CARGO envvar required for compilation of rustworkx@0.13.2

# no longer required once checkov version depends on rustworkx >0.14.0

# https://github.com/bridgecrewio/checkov/pull/6045

# gcc libffi-dev musl-dev required for compilation of cffi, until it contains musl aarch64

export CARGO_NET_GIT_FETCH_WITH_CLI=true

apk add --no-cache \

cargo=~1 \

git=~2 \

libgcc=~12 \

rust=~1

pre-commit-terraform/tools/install/checkov.sh

Line 34 in 65ba593

apk del cargo git rust

Should always be:

Should always be what? I'm not following your point.

We have two apk del commands. Is there a reason to not combine them into single one: apk del cargo gcc git libffi-dev musl-dev rust?

Should always be what? I'm not following your point.

Should always exist.

Hmm, I dunno, maybe you not from GHweb, here image for you:

Should always exist.

Heck 🤦🏻 What should exist always? 😲

Hmm, I dunno, maybe you not from GHweb, here image for you:

I see the changes. Posting just links (or screenshots) w/o any explanation is just senseless.
I see the consecutive lines 33 and 34:

They both have the same apk del but for different packages.
What makes you think these two lines should not be combined into single command?
Is it logical separation because these packages are added with separate apk add commands? If yes, then please add an explanatory comment of why this oddness is maintained. Thanks.

It is here
Line 18

# no longer required once checkov version depends on rustworkx >0.14.0

You just ignored it twice.

It is here Line 18

# no longer required once checkov version depends on rustworkx >0.14.0

For me it's line 17.
I also see there cargo, gcc, git, musl-dev, rust and libffi-dev musl-dev are also mentioned in that comment. And this give zero to nothing answer to why the apk del commands can't be combined.
What you feel like is obvious to you, shouldn't be expected to be obvious to others.

You just ignored it twice.

Dude, you please learn to explain your thoughts before accusing others that they don't get what you're not telling.

Added corresponding comment, hope it will help

yermulnik · 2024-03-14T22:27:12Z

tools/install/hcledit.sh

+GH_RELEASE_REGEX_LATEST="https://.+?_${TARGETOS}_${TARGETARCH}.tar.gz"
+GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?${VERSION}_${TARGETOS}_${TARGETARCH}.tar.gz"
+DISTRIBUTED_AS="tar.gz"


Maybe re-use DISTRIBUTED_AS?

Suggested change

GH_RELEASE_REGEX_LATEST="https://.+?_${TARGETOS}_${TARGETARCH}.tar.gz"

GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?${VERSION}_${TARGETOS}_${TARGETARCH}.tar.gz"

DISTRIBUTED_AS="tar.gz"

DISTRIBUTED_AS="tar.gz"

GH_RELEASE_REGEX_LATEST="https://.+?_${TARGETOS}_${TARGETARCH}.${DISTRIBUTED_AS}"

GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?${VERSION}_${TARGETOS}_${TARGETARCH}.${DISTRIBUTED_AS}"

Nope, because:

It will be different for binary

You will know the Distribution type only when you get GH Release link, not otherwise

It will be different for binary

The binary types just don't use the DISTRIBUTED_AS in GH_RELEASE_REGEX_LATEST and GH_RELEASE_REGEX_SPECIFIC_VERSION vars so they are not affected at all by the suggested change.

You will know the Distribution type only when you get GH Release link, not otherwise

This is probably about something else.
I see the code:

GH_RELEASE_REGEX_LATEST="https://.+?_${TARGETOS}_${TARGETARCH}.tar.gz" GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?${VERSION}_${TARGETOS}_${TARGETARCH}.tar.gz" DISTRIBUTED_AS="tar.gz"

and

GH_RELEASE_REGEX_LATEST="https://.+?/${TOOL}_${TARGETOS}_${TARGETARCH}" GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?v${VERSION}/${TOOL}_${TARGETOS}_${TARGETARCH}" DISTRIBUTED_AS="binary"

and I see no reason to not swap these two to the below:

DISTRIBUTED_AS="tar.gz" GH_RELEASE_REGEX_LATEST="https://.+?_${TARGETOS}_${TARGETARCH}.${DISTRIBUTED_AS}" GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?${VERSION}_${TARGETOS}_${TARGETARCH}.${DISTRIBUTED_AS}"

and

DISTRIBUTED_AS="binary" GH_RELEASE_REGEX_LATEST="https://.+?/${TOOL}_${TARGETOS}_${TARGETARCH}" GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?v${VERSION}/${TOOL}_${TARGETOS}_${TARGETARCH}"

No change to binary types and optimization to non-binary types.

Am I missing something non-obvious?

Once again:

You will know the Distribution type only when you get GH Release link, not otherwise

Imagine that you adding a new hook, let's say tfupdate.

You copy-past file, IE tfsec.sh rename it, cleanup pre-populated values and get this:

#... GH_ORG="" GH_RELEASE_REGEX_LATEST="https://.+?/" GH_RELEASE_REGEX_SPECIFIC_VERSION="https://.+?" DISTRIBUTED_AS="" common::install_from_gh_release "$GH_ORG" "$DISTRIBUTED_AS" \ "$GH_RELEASE_REGEX_LATEST" "$GH_RELEASE_REGEX_SPECIFIC_VERSION"

How you should know DISTRIBUTED_AS value till you don't get any of GH_RELEASE_...?

Please, don't make it more complex than it already is.

Also, someone could distribute not .tar.gz but .tgz.

How you should know DISTRIBUTED_AS value till you don't get any of GH_RELEASE_...?

From what I see in each new script for each tool installation, the value of DISTRIBUTED_AS is known and is defined in each of these files. Along with that most of the files have the value of DISTRIBUTED_AS repeated three times. And my suggestion is to reduce it to one time.

Please, don't make it more complex than it already is.

I'm trying to simplify so that new tools are added without a need to copy&paste DISTRIBUTED_AS value three times on a three lines in a row.

Read once again, maybe tomorrow
#644 (comment)

There 0 simplification.

You're wrong, unless you prove opposite 🤝

Read once again, maybe tomorrow

That comment makes no sense to me. Re-write it once again, maybe tomorrow 😏

There is a process of adding a new tool from scratch
Screencast from 24.04.24 21:01:35.webm

Get URLs

Replace URL parts with variables

Simplify URL by replacing not matter parts by regex

Set DISTRIBUTED_AS value based on URL

Also, I see that I'm already stuck to recreate that process from memory for LATEST regex, which means that this flow is already complicated.
That verbosity does not hurt anyone.

Version without stuck (will add it to contributor docs)

Screencast.from.24.04.24.22.00.36.webm

tools/install/terrascan.sh

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

Dockerfile

## [1.89.1](v1.89.0...v1.89.1) (2024-04-25) ### Bug Fixes * **docker:** Prevent all possible "silent errors" during `docker build` ([#644](#644)) ([0340c8d](0340c8d))

antonbabenko · 2024-04-25T15:03:32Z

This PR is included in version 1.89.1 🎉

MaxymVlasov added the area/docker label Mar 11, 2024

MaxymVlasov requested a review from yermulnik as a code owner March 11, 2024 21:28

MaxymVlasov added this to the Features implementing and cleanup without breaking changes milestone Mar 11, 2024

MaxymVlasov commented Mar 11, 2024

View reviewed changes

yermulnik reviewed Mar 11, 2024

View reviewed changes

MaxymVlasov and others added 5 commits March 14, 2024 20:23

fix(docker): Prevent all possible "silent errors" during docker build

4e6a011

ci: Fix platform

5de5a97

Add QEMU wich hope that it will change anything

82cb162

https://blog.thesparktree.com/docker-multi-arch-github-actions#qemu-support

Apply suggestions from code review

35f89ba

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

Apply revie suggestions

0749051

MaxymVlasov force-pushed the docker/improve_error_handling branch from 9da6b98 to 0749051 Compare March 14, 2024 18:23

MaxymVlasov added 3 commits March 14, 2024 22:16

Templating and move out most common part

6ebe879

Use one function to install GH releases

eed0722

Apply review suggestions

cb59613

MaxymVlasov requested a review from yermulnik March 14, 2024 22:02

Fix "redefinition" of global vars in function

65ba593

yermulnik reviewed Mar 14, 2024

View reviewed changes

MaxymVlasov and others added 3 commits March 18, 2024 21:49

Apply suggestions from code review

133fe03

Co-authored-by: George L. Yermulnik <yz@yz.kiev.ua>

Rewrite readonly definitions + fix dumb error

0fbbb5b

Minor style improvements

51cfcd9

MaxymVlasov requested a review from yermulnik March 18, 2024 20:24

MaxymVlasov added 2 commits March 19, 2024 18:45

Fix tests for arm

8c86ac0

fix refactoring error

84b0433

yermulnik reviewed Mar 19, 2024

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

MaxymVlasov mentioned this pull request Apr 5, 2024

Easier execution via docker image #418

Open

Merge branch 'master' into docker/improve_error_handling

8c06a41

MaxymVlasov added 3 commits April 24, 2024 20:53

Apply review suggestions

fd6efa8

Reorder fields

b3436cf

Add example of how to add add a new dependency

6b0c4ee

MaxymVlasov requested a review from yermulnik April 24, 2024 19:19

nfracost changed their version output

8f1e05c

yermulnik approved these changes Apr 24, 2024

View reviewed changes

MaxymVlasov merged commit 0340c8d into master Apr 25, 2024
6 checks passed

MaxymVlasov deleted the docker/improve_error_handling branch April 25, 2024 15:02

		if [ "$PRE_COMMIT_VERSION" = "false" ]; then echo "PRE_COMMIT_VERSION=latest" >> /.env; fi; \
		if [ "$TERRAFORM_VERSION" = "false" ]; then echo "TERRAFORM_VERSION=latest" >> /.env; fi

	# Skip tool installation if the version is set to "false"
	if [[ $VERSION == false ]]; then
	echo "'$TOOL' skipped"
	exit 0
	fi

	# cargo, gcc, git, musl-dev, rust and CARGO envvar required for compilation of rustworkx@0.13.2
	# no longer required once checkov version depends on rustworkx >0.14.0
	# https://github.com/bridgecrewio/checkov/pull/6045
	# gcc libffi-dev musl-dev required for compilation of cffi, until it contains musl aarch64
	export CARGO_NET_GIT_FETCH_WITH_CLI=true
	apk add --no-cache \
	cargo=~1 \
	git=~2 \
	libgcc=~12 \
	rust=~1

fix(docker): Prevent all possible "silent errors" during docker build #644

fix(docker): Prevent all possible "silent errors" during docker build #644

Conversation

MaxymVlasov commented Mar 11, 2024

Description of your changes

MaxymVlasov left a comment • edited Loading

Choose a reason for hiding this comment

yermulnik left a comment

Choose a reason for hiding this comment

yermulnik commented Mar 11, 2024

MaxymVlasov commented Mar 11, 2024

yermulnik commented Mar 11, 2024 • edited Loading

MaxymVlasov commented Mar 11, 2024

Choose a reason for hiding this comment

MaxymVlasov Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxymVlasov Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxymVlasov Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxymVlasov Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

yermulnik Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxymVlasov Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonbabenko commented Apr 25, 2024

fix(docker): Prevent all possible "silent errors" during `docker build` #644

fix(docker): Prevent all possible "silent errors" during `docker build` #644

MaxymVlasov left a comment •

edited

Loading

yermulnik commented Mar 11, 2024 •

edited

Loading

MaxymVlasov Mar 18, 2024 •

edited

Loading

MaxymVlasov Mar 18, 2024 •

edited

Loading

MaxymVlasov Mar 19, 2024 •

edited

Loading

MaxymVlasov Mar 18, 2024 •

edited

Loading

yermulnik Mar 18, 2024 •

edited

Loading

MaxymVlasov Mar 18, 2024 •

edited

Loading