Remove apk go requirement when possible #2318

echoix · 2023-02-06T05:24:23Z

Fixes #2315 (continued optimization)

Installing go from apk is like installing gcc or build requirements. Removing this package install completely will at least remove at least 384 MB in the usr/bin/lib/go folder (for the ci_light flavor). Go programs are very portable, they are statically compiled and a file for the appropriate OS type and CPU arch will run without problems. Thus, if a Go linter binary is available, it is better to use it directly.

Furthermore, if when I checked at a linter, they had a published docker image of a single file, and it was kept up to date by some CI (so it won't get not updated later on), I changed the installation method to a pair of dockerfile "FROM" and "COPY" instructions. Building with this is way faster, especially with buildx that is now used, since these can be done in parallel. A further optimisation should be done by using a dockerfile frontend (adding # syntax=docker/dockerfile:1 as the first line in the dockerfiles, starting with the # character) in order to use COPY --link. This would enable the buildkit to not invalidate the rest of the cache is this changes, and it could, if possible, rebase an image without pulling, pushing or building layers from the registry. See https://docs.docker.com/engine/reference/builder/#benefits-of-using---link. Adding that magic comment should be done in another PR later on. In the meantime, the new copy instructions that I added include the full file path on destination in order to make sure that there won't be overwriting problems when using COPY --link, since the destination handling has a little difference. I confirm by inspecting the images with dive that it is a file and not a folder on the destination.

Proposed Changes

Remove apk go package from go descriptor, installing revive from Docker image by copying binary.
Install shellcheck from Docker image by copying binary.
Remove apk go package from action descriptor, installing actionlint Docker image by copying binary.
- Shellcheck and pyflakes are dependencies of actionlint.
- pyflakes is installed by apk: py3-pyflakes
- Shellcheck is installed by Docker image by copying binary. It is added again directly inside the action descriptor. Duplicating the from and COPY operations adds a 0B layer, and has no real impact, except for me making sure that any future use of the actionlint in a flavor will have the dependencies generated.
- (Super-Linter installs it the same way as I figured out to do here)

Install checkmake from Docker image by copying binary.

   # make is included in the mrtazz/checkmake:latest image 
   # (DIGEST:sha256:eb6919b20b22d1701a976856e4a224627df0a74b118246101fb6cf5c2e03049f)
   # It may not be a real dependency, like their pandoc mention in the README,
   # that is not included in the docker image they provide.

Remove apk go package from go descriptor:
- install revive as a binary from a build stage (temporarly until the version returned is fixed Missing version output in revive -version in goreleaser released files mgechev/revive#787 and that a :latest tag can be used Add tags to docker images built by GoReleaser mgechev/revive#794 Missing tag "latest" on container images uploaded to ghcr.io mgechev/revive#786)
- add apk go as an install requirement for golangci-lint
Remove apk go install for repository dustilock, installing dustilock by copying binary from a Docker build stage since no container image is published but supports it.
Use COPY --link when copying a single file in order to not invalidate caches (it adds them in a layer that is independent of the preceding, and can thus be kept the same if not changed). That implied adding a # syntax=docker/dockerfile:1 at the first line of the Dockerfiles.
See a test image built to illustrate that duplicating from and copy --link isn't that bad.

To be continued, so it is not ready to merge yet, there are missing some linters with Go to change, like dustilock that doesn't include any prebuilt binaries.

Readiness Checklist

Author/Contributor

Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

Label as breaking if this is a large fundamental change
Label as either automation, bug, documentation, enhancement, infrastructure, or performance

echoix · 2023-02-06T05:45:54Z

There was a test failure in my fork with revive version. However, I don't see why it could fail, as the latest image, and my temporary tag that was pushed to the registry seems to give the same result:

nvuillam · 2023-02-11T10:49:24Z

@echoix for some reason revive does not return a version, that's why it fails :/

nvuillam · 2023-02-11T10:50:52Z

About the rest of the PR... if you could use latest tags of docker images, it would prevent to have dozens of PRs at each version upgrade, and let auto-upgrade-linters job do his nightly work with just a merge from us after ^^

echoix · 2023-02-11T15:47:06Z

About the rest of the PR... if you could use latest tags of docker images, it would prevent to have dozens of PRs at each version upgrade, and let auto-upgrade-linters job do his nightly work with just a merge from us after ^^

@nvuillam How do you keep track of what tool version is in which MegaLinter release then, if it is only tagged "latest"? Doesn't it mean if you run docker build on the same Dockerfile at two different times apart (weeks) with the repo at the same commit we won't get the same image? (or start including a broken version without us checking it, or worse, start including a vulnerable version, ie: supply chain attacks) Isn't the dependabot there to help us allow when we want each linter to change version?

nvuillam · 2023-02-11T16:36:33Z

@echoix new versions arrive in beta :)

About vulnerable versions... we have OX Security itself :)

nvuillam · 2023-02-11T16:37:57Z

And about trace, auto-update-linters job (trigger at each new commit in main, + schduled daily) calls for the version during unit tests and store the result in a JSON, which itself is reused for documentation :)

echoix · 2023-02-11T16:43:17Z

I encountered something really strange when simply pulling the go flavour image. (Trying to understand why the revive call to version bugs)

failed to register layer: ApplyLayer exit status 1 stdout: stderr: lchown /node-deps/node_modules/character-parser/LICENSE: invalid argument

I never heard about that and, seems related to rootless docker (gitpod is running my environment in a container, on a shared machine). moby/moby#43576 Did you ever see something like this?

echoix · 2023-02-11T16:48:34Z

About vulnerable versions... we have OX Security itself :)

Where do we see that they monitor, or keep monitoring? Relying on someone else seems a little easy.

And about trace, auto-update-linters job (trigger at each new commit in main, + schduled daily) calls for the version during unit tests and store the result in a JSON, which itself is reused for documentation :)

So we document, but the code is not stateful?

nvuillam · 2023-02-11T16:59:24Z

@echoix there is a dashboard on ox.security, where I monitor all my repos (not only MegaLinter)
They are a third party, but supply chain attack is the heart of their business and I trust them :)
You should try the free tier to see what I'm talking about :)

About that... @llaville please could you add 2FA auth to your Github account ? It would remove the last high severity issue found :)

The code is stateful within an official released version of MegaLinter, we don't call docker images while linting, we just use them when building then copy their executables within MegaLinter image

The safest way to use MegaLinter is indeed to use a released version, not the beta one (even if the beta one is also monitored)

echoix · 2023-02-11T17:05:49Z

The new beta image of a few minutes ago still jams, but not the released one (latest is still v6.19.0):

nvuillam · 2023-02-11T17:07:49Z

Latest beta image is ok on my computer, and I never saw such error

But since yesterday, all images are built using docker-build-push action, so maybe something changed somewhere :/

echoix · 2023-02-11T17:11:21Z

Nope, main oxsecurity/megalinter:beta doesn't work while oxsecurity/megalinter:v6.19.0 does.

You are on a windows platform, so the UID stuff doesn't seem to affect.
Its a permission thing that we need to check in what files are included. moby/moby#43576 (comment)

echoix · 2023-02-11T17:13:13Z

But since yesterday, all images are built using docker-build-push action, so maybe something changed somewhere :/

I imagine it might have hidden something on our side.
Users that run CI using containers on shared machine might stumble on this I imagine.

echoix · 2023-02-11T17:14:08Z

Ok, lets file a real bug to look at this. Until we don't understand what it is, I consider it blocking the release. It's not normal.

nvuillam · 2023-02-11T17:16:31Z

Would this solution work on your context ? moby/moby#43576 (comment)

We always knew that MegaLinter has some issues on some contexts because of its root user :/

echoix · 2023-02-13T02:26:37Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ BASH	bash-exec	6		0	0.01s
✅ BASH	shellcheck	6		0	0.14s
✅ BASH	shfmt	6	0	0	0.05s
✅ COPYPASTE	jscpd	yes		no	2.75s
✅ DOCKERFILE	hadolint	114		0	15.36s
✅ JSON	eslint-plugin-jsonc	21	0	0	1.9s
✅ JSON	jsonlint	19		0	0.25s
✅ JSON	npm-package-json-lint	yes		no	0.66s
✅ JSON	v8r	21		0	12.42s
⚠️ MARKDOWN	markdownlint	309	2	230	6.06s
✅ MARKDOWN	markdown-link-check	309		0	5.64s
✅ MARKDOWN	markdown-table-formatter	309	2	0	16.93s
✅ OPENAPI	spectral	1		0	1.75s
⚠️ PYTHON	bandit	183		47	2.07s
✅ PYTHON	black	183	0	0	3.35s
✅ PYTHON	flake8	183		0	1.89s
✅ PYTHON	isort	183	0	0	0.47s
✅ PYTHON	mypy	183		0	7.22s
✅ PYTHON	pylint	183		0	12.09s
⚠️ PYTHON	pyright	183		244	18.75s
✅ REPOSITORY	checkov	yes		no	29.77s
⚠️ REPOSITORY	devskim	yes		61	1.41s
✅ REPOSITORY	dustilock	yes		no	3.26s
✅ REPOSITORY	git_diff	yes		no	0.04s
✅ REPOSITORY	secretlint	yes		no	9.32s
✅ REPOSITORY	syft	yes		no	0.98s
✅ REPOSITORY	trivy	yes		no	20.18s
✅ SPELL	cspell	745		0	18.8s
✅ SPELL	misspell	566	2	0	0.58s
✅ XML	xmllint	3	0	0	0.03s
✅ YAML	prettier	81	2	0	2.58s
✅ YAML	v8r	23		0	57.33s
✅ YAML	yamllint	82		0	1.15s

See detailed report in MegaLinter reports

You could have same capabilities but better runtime performances if you request a new MegaLinter flavor.

Click here to request the new flavor

MegaLinter is graciously provided by

echoix · 2023-02-13T03:09:12Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ BASH	bash-exec	6		0	0.01s
✅ BASH	shellcheck	6		0	0.14s
✅ BASH	shfmt	6	0	0	0.37s
✅ COPYPASTE	jscpd	yes		no	2.58s
✅ DOCKERFILE	hadolint	114		0	14.24s
✅ JSON	eslint-plugin-jsonc	21	0	0	2.01s
✅ JSON	jsonlint	19		0	0.23s
✅ JSON	v8r	21		0	12.55s
⚠️ MARKDOWN	markdownlint	309	0	230	6.18s
✅ MARKDOWN	markdown-link-check	309		0	5.17s
✅ MARKDOWN	markdown-table-formatter	309	0	0	14.66s
✅ OPENAPI	spectral	1		0	1.63s
⚠️ PYTHON	bandit	183		47	1.99s
✅ PYTHON	black	183	0	0	4.24s
✅ PYTHON	flake8	183		0	1.72s
✅ PYTHON	isort	183	0	0	0.85s
✅ PYTHON	mypy	183		0	6.19s
✅ PYTHON	pylint	183		0	10.39s
⚠️ PYTHON	pyright	183		246	15.42s
✅ REPOSITORY	checkov	yes		no	26.57s
✅ REPOSITORY	git_diff	yes		no	0.37s
✅ REPOSITORY	secretlint	yes		no	11.48s
✅ REPOSITORY	trivy	yes		no	26.43s
✅ SPELL	cspell	745		0	17.56s
✅ SPELL	misspell	566	0	0	0.9s
✅ XML	xmllint	3	0	0	0.39s
✅ YAML	prettier	81	2	0	2.76s
✅ YAML	v8r	23		0	56.69s
✅ YAML	yamllint	82		0	1.22s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

echoix · 2023-02-13T04:38:40Z

So theres a test failure with golangci-lint. I wasn't exactly sure if it was safe to remove the GOROOT and GOPATH env since there wasn't go anymore, but it seems that it might need it still. Especially once I read the related issue mentionned in their Dockerfile
https://github.com/golangci/golangci-lint/blob/dd45e44838ed636965c9d2fd1b1abb78f3b7cca9/build/alpine.Dockerfile#L17-L27

And maybe that linter would still need go installed, even if it comes with binaries. To be continued...

… version number

Unneeded since we are not building go packages from source anymore

echoix · 2023-02-19T00:23:36Z

If this passes, it should be ready to merge once I add the changelog entry, I forgot. For now, golangci-lint really needs go installed, so it must be present. I added it in the apk install key for that linter only.

I tested locally with the following commands (used a filter here since the tests are wayyyyyy too long to run.

docker buildx build --pull --rm -f "flavors/go/Dockerfile" -t megalinter-go:test-echoix .

TEST_KEYWORDS_TO_USE="go_golangci_lint"
docker run -e TEST_CASE_RUN=true -e OUTPUT_DETAIL=detailed -e TEST_KEYWORDS="${TEST_KEYWORDS_TO_USE}" -e MEGALINTER_VOLUME_ROOT="." -v "/var/run/docker.sock:/var/run/docker.sock:rw" -v $(pwd):/tmp/lint megalinter-go:test-echoix

and

TEST_KEYWORDS_TO_USE="go"
docker run -e TEST_CASE_RUN=true -e OUTPUT_DETAIL=detailed -e TEST_KEYWORDS="${TEST_KEYWORDS_TO_USE}" -e MEGALINTER_VOLUME_ROOT="." -v "/var/run/docker.sock:/var/run/docker.sock:rw" -v $(pwd):/tmp/lint megalinter-go:test-echoix

echoix · 2023-02-19T16:26:35Z

@nvuillam I think it is done!

echoix · 2023-02-19T16:27:06Z

There are some changes in the build.py that were required for hadolint

nvuillam

Great :)
Let's see how it decreases the docker mage sizes ^^

echoix · 2023-02-19T18:34:20Z

It's sad that golangci-lint really needs go. Without it, it would be even smaller, since it was only really removed for flavors: documentation, dotnet, java, javascript, php, python, ruby, rust, salesforce, security, swift, terraform. Oh, finally it might not be as bad...

bdovaz · 2023-02-19T18:44:30Z

@echoix I am a little concerned that you have changed the installation of several linters to be through the docker image. Have you checked if with that change we lose compatibility in #2273? The fact of installing them in through bash scripts gives that possibility in most cases but installing through docker images not because not all publish the version in arm64, most are in amd64 only....

echoix · 2023-02-19T19:09:12Z

My rationale was to have the current state working as best as it could, and that linter specific overrides can be added when it will be a blocker for arm64 images.
What I made sure before changing them to a Docker image was that their repos had their docker images CI-released so they are published at the same time.

The linters changed that way were written in Go, so they can be compiled to arm64 super easily.
If ever a PR to the linter's repo to add these won't be merged in time, we could add an override with the following install (like dustilock or revive):

    install:
      dockerfile:
          # The golang image used as a builder is a temporary workaround 
        - |
          FROM golang:alpine as revive
          RUN GOBIN=/usr/bin go install github.com/mgechev/revive@latest
        - COPY --link --from=revive /usr/bin/revive /usr/bin/revive

If we would want, we could use that for all platforms, since it would work, but there are faster ways than compiling in docker as the target platform.

actionlint is released with platforms linux/amd64,linux/arm64
shellcheck is released with platforms linux/amd64,linux/arm/v6,linux/arm64
checkmake is released with platforms linux/amd64 -> so are the github releases, unofficial docker images exist, but we could go without it.

These are the only ones which the installation method "changed".
For dustilock, it was build in a RUN command, it is still built, but is now built in a stage then copied (that repo is not in good shape/active, but they have a dockerfile and showed how it could be done).
For revive, it was build in a RUN command, it is still built, but is now built in a stage then copied.

Thats pretty much it.

echoix added the performance label Feb 6, 2023

echoix self-assigned this Feb 6, 2023

echoix requested a review from nvuillam as a code owner February 6, 2023 05:24

echoix mentioned this pull request Feb 11, 2023

v6.20.0 container images errors when pulling #2348

Closed

echoix added 8 commits February 18, 2023 22:32

Install revive from ghcr docker binary

ddd9fba

Install actionlint from docker image's binary to remove apk go

0e25186

Install checkmake from docker image binary

5fbd211

Use latest instead of tags for modified docker images

a0f6626

Use a builder image for revive as the released binaries return broken…

1984bbc

… version number

Edit comment in actionlint Dockerfile instructions

295a4c1

Build dustilock in a stage and copy go binary

56ffd40

Specify GOBIN folder for revive install in builder

0e960c8

echoix added 6 commits February 18, 2023 22:35

Remove unneeded GOPATH and GOROOT variables

a10231f

Unneeded since we are not building go packages from source anymore

Include fixes from linters

6ffb7de

Add COPY --link --from by using dockerfile frontend syntax

91a19c7

Revert: Remove unneeded GOPATH and GOROOT variables

2f6ca9f

Run build.sh

3ac4086

Run ./build.sh after rebase

4c149d7

echoix force-pushed the dev/remove-apk-go branch from ea5c767 to 4c149d7 Compare February 18, 2023 22:45

Include apk go requirement for golangci-lint

62036cd

echoix changed the base branch from dev/remove-apk-go to main February 19, 2023 00:26

echoix added 3 commits February 19, 2023 00:55

Add changelog entry

4300a91

Handle duplicated FROM and COPY instructions

4a86e92

cSpell fixes

2f130b0

nvuillam approved these changes Feb 19, 2023

View reviewed changes

nvuillam merged commit fb924c7 into oxsecurity:main Feb 19, 2023

bdovaz mentioned this pull request Feb 20, 2023

Bump golang from 1.19-alpine to 1.20-alpine #2380

Merged

echoix mentioned this pull request Nov 30, 2023

Can't run megalinter container image on OpenShift since v7.4.0 #3176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove apk go requirement when possible #2318

Remove apk go requirement when possible #2318

echoix commented Feb 6, 2023 •

edited

echoix commented Feb 6, 2023

nvuillam commented Feb 11, 2023

nvuillam commented Feb 11, 2023

echoix commented Feb 11, 2023 •

edited

nvuillam commented Feb 11, 2023

nvuillam commented Feb 11, 2023 •

edited

echoix commented Feb 11, 2023

echoix commented Feb 11, 2023 •

edited

nvuillam commented Feb 11, 2023 •

edited

echoix commented Feb 11, 2023

nvuillam commented Feb 11, 2023 •

edited

echoix commented Feb 11, 2023

echoix commented Feb 11, 2023

echoix commented Feb 11, 2023

nvuillam commented Feb 11, 2023

echoix commented Feb 13, 2023 •

edited

echoix commented Feb 13, 2023 •

edited

echoix commented Feb 13, 2023

echoix commented Feb 19, 2023 •

edited

echoix commented Feb 19, 2023

echoix commented Feb 19, 2023

nvuillam left a comment

echoix commented Feb 19, 2023

bdovaz commented Feb 19, 2023

echoix commented Feb 19, 2023 •

edited

Remove apk go requirement when possible #2318

Remove apk go requirement when possible #2318

Conversation

echoix commented Feb 6, 2023 • edited

Proposed Changes

Readiness Checklist

Author/Contributor

Reviewing Maintainer

echoix commented Feb 6, 2023

nvuillam commented Feb 11, 2023

nvuillam commented Feb 11, 2023

echoix commented Feb 11, 2023 • edited

nvuillam commented Feb 11, 2023

nvuillam commented Feb 11, 2023 • edited

echoix commented Feb 11, 2023

echoix commented Feb 11, 2023 • edited

nvuillam commented Feb 11, 2023 • edited

echoix commented Feb 11, 2023

nvuillam commented Feb 11, 2023 • edited

echoix commented Feb 11, 2023

echoix commented Feb 11, 2023

echoix commented Feb 11, 2023

nvuillam commented Feb 11, 2023

echoix commented Feb 13, 2023 • edited

🦙 MegaLinter status: ⚠️ WARNING

echoix commented Feb 13, 2023 • edited

🦙 MegaLinter status: ⚠️ WARNING

echoix commented Feb 13, 2023

echoix commented Feb 19, 2023 • edited

echoix commented Feb 19, 2023

echoix commented Feb 19, 2023

nvuillam left a comment

Choose a reason for hiding this comment

echoix commented Feb 19, 2023

bdovaz commented Feb 19, 2023

echoix commented Feb 19, 2023 • edited

echoix commented Feb 6, 2023 •

edited

echoix commented Feb 11, 2023 •

edited

nvuillam commented Feb 11, 2023 •

edited

echoix commented Feb 11, 2023 •

edited

nvuillam commented Feb 11, 2023 •

edited

nvuillam commented Feb 11, 2023 •

edited

echoix commented Feb 13, 2023 •

edited

echoix commented Feb 13, 2023 •

edited

echoix commented Feb 19, 2023 •

edited

echoix commented Feb 19, 2023 •

edited