Policy conditions #34414

bmeck · 2020-07-17T20:23:39Z

This PR is to add resolve conditions to policies. This helps to align the use policies as a configuration of the capabilities of the default resolver. There is some minor refactoring to match naming a bit better across the code base. I did not see a clean place to actually traverse conditions and maybe we should make one? Additionally, this fixes policy dependency redirects to be working with ESM.

Tests not yet done as we may have minor alterations needed due to my less experience with "exports"/"imports" and actual condition resolution.

CC: @nodejs/modules-active-members due to the inconsistency with node: vs nodejs: prefix for builtin resolution vs policies. I think moving to nodejs: is fine, but would prefer node: personally. The only important bit is that they match.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

jkrems

Makes sense to me in general. For my own understanding: Is it fair to sum up the new semantics of these mappings as (roughly) "like exports but without the arrays"?

lib/internal/policy/manifest.js

lib/internal/modules/cjs/helpers.js

jkrems · 2020-07-17T20:40:16Z

lib/internal/modules/esm/resolve.js

+    const redirects = manifest.getRedirector(parentURL);
+    if (redirects) {
+      const { resolve, reaction } = redirects;
+      const destination = resolve(specifier, new SafeSet(conditions));


Is the copy necessarily because of potential mutations? If so, would it make sense to freeze the conditions set? Or is that a perf concern?

just to keep the signature of resolve consistent

lib/internal/modules/cjs/helpers.js

lib/internal/modules/esm/resolve.js

bmeck · 2020-07-18T00:08:29Z

@jkrems

Makes sense to me in general. For my own understanding: Is it fair to sum up the new semantics of these mappings as (roughly) "like exports but without the arrays"?

Correct. I don't think we should at least for now do fallbacks. Policies are meant to be static resolution. We do have multi-integrity for resources though so multi-paths might be fine? Each value in the array would still need to be a full absolute URL though.

guybedford · 2020-07-18T12:05:13Z

I would also prefer using node: over nodejs:. This is still internal only so we can change it, and that could be worth considering in this or a separate PR. I didn't realise policy already used node: or would have aligned with that previously.

It is great to see the unification here. I'm not sure I understand the use cases behind the conditions though - can you perhaps clarify in more detail why you want to split on these conditions?

bmeck · 2020-07-18T14:30:49Z

@guybedford

This is generally just to allow policies to model:

require('foo');
import('foo');

Which can resolve to 2 different locations. Leaving it to match how "exports" works because of the cascading and in order iteration means that logic is involved in non-trivial ways and I'm unclear if only supporting a subset of conditions makes sense (and the potential for things like #33171 to appear in the future).

guybedford · 2020-07-18T14:39:03Z

Import maps do not permit conditional variations, rather conditions have to be handled by having the import map be a function of the environment.

The "require" and "import" conditions are unique in that they apply within the same environment unlike all other conditions.

If creating an import map for Node.js this problem is avoided by the nature of scoping permitting "import" and "require" to naturally diverage as appropriate based on the importer.

This same technique should be able to apply to policy.

For this reason I would prefer to use scoping-based rules over explicit conditions in mappings.

bmeck · 2020-07-18T15:30:32Z

If creating an import map for Node.js this problem is avoided by the nature of scoping permitting "import" and "require" to naturally diverge as appropriate based on the importer.

This same technique should be able to apply to policy.

Policies are not import maps; currently in stable Node 12 and 14 you can have code that diverges when you have the code above. Policies are meant to model the resolution of the application and needs to model this divergence within the same source text as can be seen in both, unflagged. We have 1 importer with the same import specifier but 2 different conditions (usage of import vs require). We cannot model such using import maps.

For this reason I would prefer to use scoping-based rules over explicit conditions in mappings.

Scopes are a different feature that I have a branch to work on but are for usability not for feature parity. This PR is to enable feature parity of which import maps cannot supply.

guybedford · 2020-07-18T15:45:08Z

I suppose the missing case would be a CommonJS file that both has a require() to an export and an import() to the same package export, where that package export diverges on the import / require condition.

I have voiced my concern that this seems a lot of functionality to support an edge case, but I'll leave it at that.

bmeck · 2020-07-18T16:02:17Z

@guybedford it also should affect the currently broken regarding policies createRequire. Which should be fixed in a different PR. I'm mostly just going around getting things patched up/interoperated since ESM is mostly stable rather than the large churn we had at the start of this year.

guybedford · 2020-07-18T16:04:49Z

I'm 100% behind the unification, my concern is more that this conditions model doesn't integrate well with loaders since the loader is the authoritative point of truth as to what is resolved (and loaders can bypass policy).

bmeck · 2020-07-18T16:20:52Z

@guybedford the loader is still the authority, policies just configure the actions of the default loader. Policies are not meant to override any loader behavior. I think we are in agreement there. The conditions are just the static actions the default loader should take when given the listed conditions (by an application or another loader), not that loaders see specific conditions.

guybedford

Could we restrict policy to only act on conditions that Node.js explicitly varies on? That is right now it would just be "import" and "require". Possibly in future we might include things like "main" or "worker" etc. My concern is that it makes no sense for other conditions here while still allowing external loaders to drive these variations.

I suppose one argument against that would be the "development" and "production" conditions if we open up that feature in future via a flag or otherwise, but even then I would still prefer to have eg policy.dev and policy.production as separate and only have a within-environment policy handle within-environment condition variations.

Let me know your thoughts.

Also, "default" is hard-coded I believe, so needs to be supported explictly otherwise (but double-check).

bmeck · 2020-07-19T14:05:46Z

Could we restrict policy to only act on conditions that Node.js explicitly varies on? That is right now it would just be "import" and "require". Possibly in future we might include things like "main" or "worker" etc. My concern is that it makes no sense for other conditions here while still allowing external loaders to drive these variations.

Currently the default loader appears to handle anything a user loader passes to it. That means that if you use a policy and a user loader it could still see a different kind of condition (who knows what [ no-native-modules ?]). I think if we do limit it to default, import, node, and require it would prevent user loaders from using custom conditions when using policies and doing the normal delegation workflow. That seems odd to me and makes me lean toward not limiting it without reason to drop that parity.

I suppose one argument against that would be the "development" and "production" conditions if we open up that feature in future via a flag or otherwise, but even then I would still prefer to have eg policy.dev and policy.production as separate and only have a within-environment policy handle within-environment condition variations.

I think this is very nuanced, I agree for development/production you likely would have separated files. I don't necessarily think we should focus on mixing those in the same file as that seems to be less likely to co-exist in a single file if we did support such a feature. However, if there is runtime reflection we likely would want to ensure that a policy does support both. This is all future speculation though. Currently conditions are not runtime reflected anywhere.

Also, "default" is hard-coded I believe, so needs to be supported explictly otherwise (but double-check).

Good catch, will fix.

guybedford · 2020-07-19T15:45:18Z

My point specifically is that environment-level configurations interacting with policy makes policy support multiple environment invocations. Consider if we support node --conditions=x,y for any x,y. Policy then captures all those condition variations. In contrast to how one might picture policy as locking down only a specific environment execution invocation.

For example, in my mind, policy generation for a specific invocation would be automated via eg node x.js --output-policy=policy.json and then having the ability to run that same policy against the app provides the guarantees. Multi-environment policy breaks the simplicity and determinism of this model.

bmeck · 2020-07-19T20:31:06Z

@guybedford but doesn't that mean that if a user land loader passes a condition to the default loader that isn't in that list it will never work with policies? That doesn't seem to make sense to have that limit if so. What do we get from that limit?

bmeck · 2020-07-19T20:31:29Z

I'd note that the policy remains deterministic and static either way.

guybedford · 2020-07-19T20:39:58Z

The limit just ensures the process of creating a policy is well-defined since otherwise how do you know the full possible set of conditions loaders might pass given arbitrary runtime branches.

Let me just state outright the end to end model that makes the most sense to me:

Policy as a loader in the first place would allow it to be more authoritative thatn existing in core this way because you could make it the authoritative loader in composition.
Policy as a loader would allow easy policy generation since policy generation is a direct function of the hook outputs.
Such a model would work best under exact condition branching, or if handling import / require branching to do that by literally branching only on the require / import resolvers and not on other conditions.

All of the above could be achieved if we had loader composition landed first and then made policy a "core loader" perhaps via node --loader node:policy or similar where core loaders could exist to be called upon, or where node --policy automatically injects the loader authoritatively in chaining. This would in turn full abstract policy from the resolver internals.

That's my full say here... just wanted to share the thoughts... but I'm not against anything here.

bmeck · 2020-07-19T20:58:30Z

The limit just ensures the process of creating a policy is well-defined since otherwise how do you know the full possible set of conditions loaders might pass given arbitrary runtime branches.

You would never know once a runtime loader is involved. Policies just state what the default is allowed to return. They are not a limiter/constraint system to loaders.

Policy as a loader in the first place would allow it to be more authoritative than existing in core this way because you could make it the authoritative loader in composition.

Policies apply to loaders currently, this would not be possible to recreate unless we have loaders instrumenting each other.

Policy as a loader would allow easy policy generation since policy generation is a direct function of the hook outputs.

I don't understand this. A loader can currently do that by monitoring the input/output of the default loader already. You would likely not want to run this in the same situation as when the policy is applied.

Such a model would work best under exact condition branching, or if handling import / require branching to do that by literally branching only on the require / import resolvers and not on other conditions.

I think there are a ton of interesting cases which wouldn't match up with the idea that conditions are part of the environment. Things like loader conditions that are only set before the main entry point to the application to ensure that various hardening APIs are only loaded in a higher integrity set of globals etc.

Overall since there are no complaints about landing this PR as is I think we should move forward. I think if people want to expose policies / use them from loaders we can enable a reflective API somehow in a different PR. I'm not sure I understand the last few points. We do have write ups on various policy intents in https://docs.google.com/presentation/d/153ME48cGiIo7RZpORjbbtogMbA4TeBZXv2EnEeKdZGg/edit#slide=id.p and I hope to update it when we finish landing various other features since some of the points are out of date (like data: being a way to load from in-memory and no longer a next step)

bmeck · 2020-07-29T14:11:46Z

fixed a bug with localMappings that only mapped based upon specifier instead of specifier and conditions. test is permutation based, i figured a depth of 3 was a reasonable stopping point. this did introduce a compositeKey API for internal usage due to some complexity with creating that map key.

CC: @devsnek

bmeck · 2020-08-13T14:10:39Z

Fresh CI then will land in a bit after

nodejs-github-bot · 2020-08-13T14:10:49Z

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

nodejs-github-bot · 2020-08-13T14:15:23Z

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

nodejs-github-bot · 2020-08-13T14:36:19Z

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

nodejs-github-bot · 2020-08-13T17:20:45Z

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

PR-URL: #34414 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

bmeck · 2020-08-13T18:18:50Z

Landed in e155d96

PR-URL: #34414 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

nodejs/node#34414

MylesBorins · 2020-11-03T16:48:14Z

@bmeck should we land this on 12.x? Seems like it would be relatively straight forward to fix the conflicts.

bmeck · 2020-11-03T17:16:58Z

@MylesBorins +0 on landing it on 12.x, the feature is nice but not worth wrangling any conflicts to me personally. if you want to, that seems fine as likely we will continue to update policies

bmeck requested review from jkrems and guybedford July 17, 2020 20:23

bmeck self-assigned this Jul 17, 2020

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 17, 2020

jkrems approved these changes Jul 17, 2020

View reviewed changes

ljharb reviewed Jul 17, 2020

View reviewed changes

lib/internal/modules/cjs/helpers.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

guybedford reviewed Jul 19, 2020

View reviewed changes

mhdawson mentioned this pull request Jul 27, 2020

Node.js Modules Team Meeting 2020-07-29 nodejs/modules#536

Closed

guybedford approved these changes Jul 27, 2020

View reviewed changes

bmeck added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed modules-agenda Issues and PRs to discuss during the meetings of the Modules team. labels Aug 13, 2020

bmeck force-pushed the policy-conditions branch from d463fe4 to f2183d9 Compare August 13, 2020 14:14

policy: support conditions for redirects

52c7ed6

bmeck force-pushed the policy-conditions branch from 0ac7513 to 52c7ed6 Compare August 13, 2020 17:12

bmeck added a commit that referenced this pull request Aug 13, 2020

policy: support conditions for redirects

e155d96

PR-URL: #34414 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

bmeck closed this Aug 13, 2020

MylesBorins pushed a commit that referenced this pull request Aug 17, 2020

policy: support conditions for redirects

c279092

PR-URL: #34414 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

This was referenced Aug 18, 2020

CI Reliability 2020-08-18 nodejs/reliability#47

Open

CI Reliability 2020-08-19 nodejs/reliability#48

Open

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

BethGriggs pushed a commit that referenced this pull request Aug 20, 2020

policy: support conditions for redirects

8403118

PR-URL: #34414 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update filename JSON

1f13243

nodejs/node#34414

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update filename JSON

198a757

nodejs/node#34414

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update filename JSON

2fb1d77

nodejs/node#34414

codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020

Update filename JSON

be2732e

nodejs/node#34414

bmeck deleted the policy-conditions branch February 3, 2022 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Policy conditions #34414

Policy conditions #34414

bmeck commented Jul 17, 2020 •

edited

Loading

jkrems left a comment

jkrems Jul 17, 2020

bmeck Jul 17, 2020

bmeck commented Jul 18, 2020

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020 •

edited

Loading

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020

guybedford left a comment

bmeck commented Jul 19, 2020

guybedford commented Jul 19, 2020

bmeck commented Jul 19, 2020

bmeck commented Jul 19, 2020

guybedford commented Jul 19, 2020

bmeck commented Jul 19, 2020

bmeck commented Jul 29, 2020

bmeck commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

bmeck commented Aug 13, 2020

MylesBorins commented Nov 3, 2020

bmeck commented Nov 3, 2020

Policy conditions #34414

Policy conditions #34414

Conversation

bmeck commented Jul 17, 2020 • edited Loading

Checklist

jkrems left a comment

Choose a reason for hiding this comment

jkrems Jul 17, 2020

Choose a reason for hiding this comment

bmeck Jul 17, 2020

Choose a reason for hiding this comment

bmeck commented Jul 18, 2020

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020 • edited Loading

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020

guybedford commented Jul 18, 2020

bmeck commented Jul 18, 2020

guybedford left a comment

Choose a reason for hiding this comment

bmeck commented Jul 19, 2020

guybedford commented Jul 19, 2020

bmeck commented Jul 19, 2020

bmeck commented Jul 19, 2020

guybedford commented Jul 19, 2020

bmeck commented Jul 19, 2020

bmeck commented Jul 29, 2020

bmeck commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

nodejs-github-bot commented Aug 13, 2020

bmeck commented Aug 13, 2020

MylesBorins commented Nov 3, 2020

bmeck commented Nov 3, 2020

bmeck commented Jul 17, 2020 •

edited

Loading

bmeck commented Jul 18, 2020 •

edited

Loading