policy: cleanup to allow snapshotting

[bmeck]

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

[nodejs-github-bot]

Review requested:

• @nodejs/modules
• @nodejs/startup

[bmeck]

will wait and rebase after #42203

[aduh95]

#42203 have landed, this needs a rebase.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43149/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43154/

[joyeecheung]

LGTM with a nit, thanks

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43172/

[bmeck]

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

[bmeck]

@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]

@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]

@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]

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]

Maybe we need to rewrite

node/lib/internal/policy/manifest.js

Lines 32 to 37 in ceadb47

	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]

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]

@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.

[avivkeller]

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!