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

policy: cleanup to allow snapshotting #42191

Closed
wants to merge 9 commits into from
Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Mar 2, 2022

This is just some cleanup/refactoring to move getOptionValue away as much as possible for snapshotting reasons and some general startup cleaning.

Since this makes it always at least init an empty policy I thought adding it to the snapshot makes sense but I'm fine leaving it out as well.

CC: @joyeecheung

@bmeck bmeck added policy Issues and PRs related to the policy subsystem. snapshot Issues and PRs related to the startup snapshot labels Mar 2, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 2, 2022
@bmeck
Copy link
Member Author

bmeck commented Mar 7, 2022

will wait and rebase after #42203

@aduh95
Copy link
Contributor

aduh95 commented Mar 12, 2022

#42203 have landed, this needs a rebase.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit, thanks

lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
@bmeck bmeck added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 22, 2022
@nodejs-github-bot
Copy link
Collaborator

@bmeck bmeck removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 22, 2022
@bmeck
Copy link
Member Author

bmeck commented Mar 22, 2022

looks like i've got to dance around no crypto builds

@bmeck
Copy link
Member Author

bmeck commented Mar 22, 2022

@joyeecheung it looks like we still cannot include crypto in the snapshot via require('crypto') due to non-crypto builds. I can move all the stuff back to inside lazyBits?

@joyeecheung
Copy link
Member

@bmeck hmm, I don’t think that is snapshot-specific - if policy still needs crypto to check integrity in non-crypto builds, how is that supposed to work? It seems some code should be added in policy to throw an error when the build has no OpenSSL instead?

@bmeck
Copy link
Member Author

bmeck commented Mar 24, 2022

@joyeecheung anything using crypto transitively like policies has to add guards so anything in turn using policies would have to add guards. The goal overall here is to allow policies in the snapshot and the lazyBits would throw the error when it is actually used rather than during snapshotting. If we add guards like we did in bootstrap/node.js to each usage of policy that would mitigate things but be much more a burden on the codebase than throwing an error on first using the feature at runtime.

@bmeck
Copy link
Member Author

bmeck commented Mar 24, 2022

To clarify a bit, the overall goal is to get ESM/CJS loading into the snapshot and they have a transitive dependency on crypto through policy. It would be a mess to add guards everywhere.

@aduh95
Copy link
Contributor

aduh95 commented Mar 24, 2022

Maybe we need to rewrite

const crypto = require('crypto');
const { Buffer } = require('buffer');
const { URL } = require('internal/url');
const { createHash, timingSafeEqual } = crypto;
const HashUpdate = uncurryThis(crypto.Hash.prototype.update);
const HashDigest = uncurryThis(crypto.Hash.prototype.digest);
into

let createHash, timingSafeEqual, HashUpdate, HashDigest;
if (config.hasSSL) {
  const crypto = require('crypto');
  ({createHash, timingSafeEqual} = crypto);
  HashUpdate = uncurryThis(crypto.Hash.prototype.update); 
  HashDigest = uncurryThis(crypto.Hash.prototype.digest); 
}

That should solve the issue at hand I think.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 28, 2022

I think it should be fine to revert back to something like lazyBits - crypto is already eager loaded into the snapshot in builds with crypto anyway so doing another require('crypto') only incurs a map lookup which isn't expensive, though IMO ideally code that makes use of policy should guard against non-crypto builds instead of simply assuming the existence of a feature where it's not functional (I don't think it needs to be done in this patch, to be clear)

@bmeck
Copy link
Member Author

bmeck commented Mar 28, 2022

@joyeecheung the amount of checks would not be in policy itself, but anything consuming policy as well. I don't want to spider web out the checks throughout the codebase.

@RedYetiDev
Copy link
Member

Hi, I'm sure you've heard that the policy mechanism was deprecated. A commit removing it entirely was just pushed, so I'm closing this. If you disagree, please re-open.

Thank you!

@RedYetiDev RedYetiDev closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. policy Issues and PRs related to the policy subsystem. snapshot Issues and PRs related to the startup snapshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants