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

Bug: plugin / tool installation can result in corrupt binaries #4467

Closed
Walther opened this issue May 30, 2023 · 4 comments
Closed

Bug: plugin / tool installation can result in corrupt binaries #4467

Walther opened this issue May 30, 2023 · 4 comments
Labels

Comments

@Walther
Copy link
Contributor

Walther commented May 30, 2023

Bug

Current Behavior

Currently, tools downloaded by Garden might be unzipped wrong, resulting in a corrupt executable. This affects at least terraform, but might also affect e.g. helm, mutagen, and so on.

Example output locally when running the terraform-plugin tests:

 16) Terraform action type
       apply-action command
         calls terraform apply for the action:
     Error: Command "/Users/walther/.garden/tools/terraform-0-13-3/692e8c519d913625/terraform init" failed with code Unknown system error -8:
      at ChildProcess.spawn (node:internal/child_process:413:11)
      at Object.spawn (node:child_process:757:9)
      at execa (/Users/walther/git/garden/node_modules/execa/index.js:80:26)
      at exec (/Users/walther/git/garden/core/src/util/util.ts:216:16)
      at PluginTool.exec (/Users/walther/git/garden/core/src/util/ext-tools.ts:78:16)
      at getWorkspaces (common.ts:262:3)
      at setWorkspace (common.ts:297:36)
      at Object.handler (commands.ts:95:7)
      at Context.<anonymous> (test/terraform.ts:379:9)

Example of corrupt executables in our CI: https://app.circleci.com/pipelines/github/garden-io/garden/17991/workflows/f023b92f-2435-443c-94fd-1bd0b57cf084/jobs/314086

Expected behavior

Tools get downloaded and extracted correctly, resulting in executables that work.

Reproducible example

rm -rf ~/.garden/tools/terraform-*
cd garden/plugins/terraform/test
yarn run test
# all tests fail

ls -halF ~/.garden/tools/terraform-*/*/terraform
file ~/.garden/tools/terraform-*/*/terraform 
# the files are "binary data", not recognized as valid executables

Workaround

Manually download the relevant .zip or .tar.gz, manually extract it, and move the executable to the location garden expects it within ~/.garden/tools/*

Suggested solution(s)

  • Wait for a fix in the upstream unzipper library
  • Debug the library to find a fix, submit PR
  • Switch to another library or libraries (we use both .zip and .tar.gz formats, some libraries only support one or the other)

Additional context

This is an adverse interaction between the unzipper library and Node version >= 18.16.0 which we use.

See the upstream issue here: ZJONSSON/node-unzipper#271
The issue description also has a minimal repro for local testing.

Your environment

This happens both on local macOS as well as in our CI on Linux.
This affects both 0.13 as well as 0.12 versions released after Jan 30th.

@stefreak
Copy link
Member

Good catch!

@Orzelius Orzelius moved this from Candidate to Todo in Core Weekly May 30, 2023
@Orzelius Orzelius moved this from Todo to Candidate in Core Weekly May 30, 2023
Walther added a commit that referenced this issue Jun 1, 2023
The tests have been fixed and they pass locally. These tests fail in CI
because of #4467
Temporarily skipping these tests in order to unblock other PRs for the
other plugins and fixing their test setups, while keeping our CI green
for the test-plugins step.
@shumailxyz
Copy link
Contributor

@Walther Should we may be try using a lower node version for executing test-plugins in CI until we find a solution to this?

Walther added a commit that referenced this issue Jun 1, 2023
* test(terraform): run tests for multiple tf versions

* improvement: remove terraform lock files from git tree

* ci(circleci): re-enable plugins tests

* chore(terraform): fix hash for terraform 0.13.3 darwin amd64

* chore: terraform-plugin: 0.13 config updates for test projects

* chore: terraform-plugin: test updates for 0.13 config

* chore: terraform-plugin: fix provider tests

* chore: terraform-plugin: fix some action tests

* chore: terraform-plugin: fix 'sets the workspace before destroying' test

Co-authored-by: Steffen Neubauer <[email protected]>

* chore: terraform-plugin: fix rest of the skipped tests

Co-authored-by: Steffen Neubauer <[email protected]>

* chore: conftest plugin: temporarily disable tests, fix in another pr

* chore: conftest plugin: temporarily disable tests, fix in another pr

* chore: jib plugin: temporarily disable tests, fix in another pr

* chore: conftest test: fix import

* chore: jib plugin: temporarily disable tests, fix in another pr

* chore: terraform-plugin common tests: init before workspace commands

* chore: terraform-plugin: lockfile for both test projects

* chore: update unzipper

* refactor: improve log message methods for testing purposes

based on the feedback provided at #4457 (comment)

* chore: return the separate getRootLogMessages and getLogMessages

other unrelated tests were depending on the specific behavior.
we can always come back to refactor further.

* chore: disable terraform-plugin tests

The tests have been fixed and they pass locally. These tests fail in CI
because of #4467
Temporarily skipping these tests in order to unblock other PRs for the
other plugins and fixing their test setups, while keeping our CI green
for the test-plugins step.

* chore: terraform-plugin: remove lockfile

* chore: terraform-plugin tests: restore test-project-module

---------

Co-authored-by: Walther <[email protected]>
@Orzelius Orzelius removed this from Core Weekly Jun 12, 2023
@Walther
Copy link
Contributor Author

Walther commented Jul 4, 2023

Adding more context to this issue:

  • We use node 18.15 in some places of our codebase, (e.g. circleci-runner docker image) which does not run into this error
  • We use node 18 in some other places of the codebase (e.g. dockerfiles for building our public garden docker images), resulting in this error in the final images, as node >=18.16 is used
  • We should unify our codebase to use 18.15 specifically everywhere
  • We should update unzipper to a newer version when a fix arrives, or alternatively, switch to using a different library

@vvagaytsev
Copy link
Collaborator

This will be followed in #4467. Closing this.

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

Successfully merging a pull request may close this issue.

4 participants