feat: propagate featureflags to plugins #4992
Conversation
| This pull request adds or modifies JavaScript ( |
packages/build/src/steps/plugin.js Outdated
| event, | ||
| error, | ||
| envChanges, | ||
| featureFlags, |
There was a problem hiding this comment.
we're providing the flags to every single plugin with this, no matter who wrote it and wether we trust it. I don't think these flags are super secret, so this should be fine - if not, is there some guard we could put in place? e.g. use the pluginPackageJson that's passed into the function, and check it against an allow-list?
There was a problem hiding this comment.
How much work do you think it would be to build that allow-list functionality? Just thinking from a perspective of being conservative in what we expose to plugins before we've fully settled on how we expose feature flags in general.
There was a problem hiding this comment.
The easiest approach would probably be to check pluginPackageJson.name and compare it to an allowlist. That's trivial to build, but won't stop people from accessing them if they're determined to do so (i'm sure there's ways of mangling with the package.json)
There was a problem hiding this comment.
but if we're only trying to hide this while figuring out the details, then that's probably a good solution.
There was a problem hiding this comment.
These flags are passed down from buildbot (not filtered or anything). This means that we will also get secret values down the road here!
Instead, we might only pass them to the "core plugins"? Which is the next runtime etc.? instead of to all build plugins?
Furthermore, we should now change in buildbot the way how we gather feature flags and use the newly added tag filter to only pass feature flags for @netlify/build.
Currently all Netlify flags are passed down (backend, frontend, etc...)
There was a problem hiding this comment.
Instead, we might only pass them to the "core plugins"?
Right, that's what I mean by "allow list". How would you recommend we check wether we can trust a plugin?
There was a problem hiding this comment.
I would recommend adding a property to the pluginsList https://list-v2--netlify-plugins.netlify.app/plugins.json
https://github.com/netlify/plugins/blob/main/site/plugins.json
There was a problem hiding this comment.
or if the npm package starts with @netlify/ as then it's one of our trusted npm packages
There was a problem hiding this comment.
but anyways I would suggest first limiting the list of plugins that are passed by buildbot via a tag in devcycle.
There was a problem hiding this comment.
I've implemented the @netlify/-based filter in 12c45da.
but anyways I would suggest first limiting the list of plugins that are passed by buildbot via a tag in devcycle.
Could you elaborate on how this would help? You mentioned that we have "secret values" in the flags, could you give an example for a featureflag that we don't want to expose to trusted plugins?
packages/build/src/steps/plugin.js Outdated
| event, | ||
| error, | ||
| envChanges, | ||
| featureFlags, |
There was a problem hiding this comment.
How much work do you think it would be to build that allow-list functionality? Just thinking from a perspective of being conservative in what we expose to plugins before we've fully settled on how we expose feature flags in general.
| featureFlags: { test_flag: true }, | ||
| }) | ||
| .runWithBuild() | ||
| t.true(output.includes('test_flag')) |
There was a problem hiding this comment.
This doesn't actually assert the shape of featureFlags (e.g. comma-separated array vs. object). Could we make the test a bit more specific?
| @@ -0,0 +1,3 @@ | |||
| export const onBuild = function ({ featureFlags }) { | |||
| console.log(Object.keys(featureFlags)) | |||
There was a problem hiding this comment.
you could leverage node builtin assertion to test it with:
import { deepEqual } from 'node:assert' export const onBuild = function ({ featureFlags }) { assert.deepEqual(featureFlags, { test_flag: true} ); console.log('passed'); };There was a problem hiding this comment.
I'd like to keep the assertions in the test file for readability, so I went with 5909c3d instead
| @@ -0,0 +1,2 @@ | |||
| name: test | |||
| inputs: [] | |||
There was a problem hiding this comment.
I think you don't need the inputs here or?
3 participants
Summary
Addresses https://github.com/netlify/pod-compute/issues/491 and https://github.com/netlify/pod-ecosystem-frameworks/issues/471.
This PR propagates the
featureFlagsobject that's passed into@netlify/builddown towards plugins. The Netlify Team can use this to roll out changes to plugins more precisely.There's discussions around changing where those values come from (BitBalloon vs Buildbot, see e.g. https://github.com/netlify/pod-compute/issues/491#issuecomment-1523024232), but with this PR i'd like to just address that they're usable from plugins, not where they're sourced from.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)