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

Set the required configuration for processing a Yarn request #360

Merged

Conversation

brunoapimentel
Copy link
Contributor

@brunoapimentel brunoapimentel commented Oct 17, 2023

The following .yarnrc.yml options are strictly necessary in order to process a yarn request:

  • checksumBehavior: set to 'throw', so an exception is raised in case of
    checksum mismatches
  • enableImmutableInstalls: makes 'yarn install' fail in case a change to
    the lockfile is necessary
  • plugins: set to an emtpy array, so that no plugins will be used in the
    prefetch/cache checking. This avoids arbitrary code execution.
  • ignorePath: set to 'true'. This will make the local yarn executable to be ignored, and fallback to Corepack, which will install the corresponding version. This way, we protect the request from a potentially tampered executable file.

For zero-installs:

  • enableImmutableCache: makes 'yarn install' fail if any modification in
    the cache folder would be made.

For regular workflow:

  • enableMirror: set to true to ensure the globalFolder will be used.
  • globalFolder: set to the request's output folder. The prefetched
    packages will be placed in it.
  • enableScripts: set to false to avoid the unplugging (unzipping and placing the code in the ./yarn/unplugged folder) of dependencies that have postinstall scripts.

We're also setting these "nice to have" options:

  • enableStrictSsl and unsafeHttpWhitelist: forces the all the traffic while fetching packages to happen over HTTPS
  • disableTelemetry: less useless requests created by yarn

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

@brunoapimentel
Copy link
Contributor Author

I focused on the yarnrc attributes we are sure we need to change based on the design doc, I still need to investigate the ones under "maybe" and do more real-world testing.

@brunoapimentel brunoapimentel force-pushed the set-yarn-config branch 2 times, most recently from d93088d to ccd7cc9 Compare October 23, 2023 13:17
@brunoapimentel
Copy link
Contributor Author

New changes: Split the first commit in two, removed the WIP status from the first three commits, added a new WIP commit to investigate additional yarnrc configuration options.

@brunoapimentel brunoapimentel marked this pull request as ready for review October 25, 2023 00:10
@taylormadore
Copy link
Contributor

Sadly, I think we're going to need ignorePath: true 🥲
I believe the corepack-downloaded version of yarn will execute yarnPath if yarnPath is present and ignorePath is false

@@ -22,6 +22,15 @@
DEFAULT_CACHE_FOLDER = "./.yarn/cache"
DEFAULT_REGISTRY = "https://registry.yarnpkg.com"

ChecksumBehavior = Literal["throw", "update", "ignore"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't "reset" also a possible value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@lkolacek lkolacek Oct 31, 2023

Choose a reason for hiding this comment

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

Should we make a note to yarn v4 story/epic that reset should be added there?

cachi2/core/package_managers/yarn/main.py Outdated Show resolved Hide resolved
@brunoapimentel
Copy link
Contributor Author

Sadly, I think we're going to need ignorePath: true 🥲 I believe the corepack-downloaded version of yarn will execute yarnPath if yarnPath is present and ignorePath is false

You're absolutely right. We want to ignore the commited yarn executable if it exists, and that requires the ignorePath option set.

Copy link
Contributor

@lkolacek lkolacek left a comment

Choose a reason for hiding this comment

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

Looks amazing Bruno. I left here a few nitpicks, feel free to ignore them. :) I assume those TBD commits will be resolved in upcoming PRs. 👍

cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/yarn/project.py Show resolved Hide resolved
cachi2/core/package_managers/yarn/main.py Show resolved Hide resolved
tests/unit/package_managers/yarn/test_main.py Show resolved Hide resolved
@brunoapimentel brunoapimentel force-pushed the set-yarn-config branch 7 times, most recently from e23c6b4 to 60c7226 Compare November 7, 2023 00:47
Since we will always write some configuration options into the
.yarnrc.yml when processing a Yarn request, we can safely instantiate an
empty YarnRc object whenever a Project is loaded.

Signed-off-by: Bruno Pimentel <[email protected]>
This will be needed later on so that the configuration in the
.yarnrc.yaml file can be changed for the prefetch.

Signed-off-by: Bruno Pimentel <[email protected]>
The name was changed from 'write_to_file' to simply 'write', making it
consistent to a similar method present in the YarnRc class.

Signed-off-by: Bruno Pimentel <[email protected]>
STONEBLD-1777

Set the following options in the .yarnrc.yml file:

- checksumBehavior: set to 'throw', so an exception is raised in case of
  checksum mismatches.
- enableImmutableInstalls: makes 'yarn install' fail in case a change to
  the lockfile is necessary.
- plugins: set to an empty array, so that no plugins will be used in the
  prefetch/cache checking. This avoids arbitrary code execution.
- pnpMode: set to 'strict', to disallow modules to require packages they
  don't explicitly list in their own dependencies.

For zero-installs:
- enableImmutableCache: makes 'yarn install' fail if any modification in
  the cache folder would be made.

For regular workflow:
- enableMirror: set to true to ensure the globalFolder will be used.
- globalFolder: set to the request's output folder. The prefetched
  packages will be placed in it.

More details at: https://v3.yarnpkg.com/configuration/yarnrc

Signed-off-by: Bruno Pimentel <[email protected]>
Sets the "enableStrictSsl" to true and "unsafeHttpWhitelist" to an empty
list in .yarnrc.yml.

See https://v3.yarnpkg.com/configuration/yarnrc for more info.

Signed-off-by: Bruno Pimentel <[email protected]>
Set the .yarnrc.yml option "enableTelemetry" to false to prevent the
yarn commands from making unnecessary requests.

See https://v3.yarnpkg.com/configuration/yarnrc for more info.

Signed-off-by: Bruno Pimentel <[email protected]>
Set the ignorePath option to true, which makes yarn ignore the
executable defined yarnPath and fallback to the global one. This makes
Corepack fetch a new yarn executable, which will then be used.

We don't want to rely on the executable that is commited to the
repository, since there's no way to know if it has been tampered.

Signed-off-by: Bruno Pimentel <[email protected]>
Set the "enableScripts" .yarnrc.yml option to false, which avoids the
unplugging of dependencies that have postinstall scripts.

The unplugged content is necessary for the Yarn project to run, but
since a "yarn install" is expected to happen during the build phase for
a regular workflow project, this content will still be generated as
expected.

For more info, see:
- https://v3.yarnpkg.com/cli/install (--mode=skip-build section)

Signed-off-by: Bruno Pimentel <[email protected]>
@brunoapimentel brunoapimentel force-pushed the set-yarn-config branch 3 times, most recently from c849c6d to 258e04e Compare November 7, 2023 17:47
Copy link
Contributor

@taylormadore taylormadore left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@brunoapimentel brunoapimentel added this pull request to the merge queue Nov 7, 2023
Merged via the queue into containerbuildsystem:main with commit abc89ac Nov 7, 2023
@brunoapimentel brunoapimentel deleted the set-yarn-config branch December 5, 2023 19:31
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