search

FlowFuse / nr-launcher

A Laucher for Node-RED that allows settings to be loaded at startup.
Apache License 2.0
5 stars 6 forks source link

Editor Node-RED app environment variables: allow controllable propagation of env set to process to Node-RED app #145

Closed elenaviter closed 1 year ago

elenaviter commented 1 year ago

Description

Environment variables, sources

In the environment that the Node-RED process operates, there are 3 sources for the environmental variables:

  1. Variables set on the project settings in FlowForge.
  2. Variables related to FlowForge project identification.
  3. Variables that come into play when the Editor instance (and by extension, the Device instance, though that's a separate discussion) is deployed via a container orchestrator (dynamically provisioned)

Share env variables to software running with Node-RED

To forward environment variables to Node-RED (as well as software solutions running in Node-RED), we need to imbue the launcher with the ability to augment the application's environment with env from source 3 - e.g. those that are set on process.env.

Way to share env

Instead of the usual way that Node-RED process will assume env includes:

E.g. instead

const appEnv = Object.assign({}, this.env, this.settings.env)

We propose to filter the process.env to exclude variables starting with "FORGE", which stands for servicing FORGE secrets:

const filter_env = (env) =>
    Object.entries(env).reduce((acc, [key, value]) =>
          key.startsWith('FORGE') ? acc : {...acc, [key]: value}, {});
const appEnv = Object.assign({}, filter_env(process.env), this.env, this.settings.env)

Or alternatively, in a more controlled approach, we propose using a documented environment variable, such as FORGE_ALLOW_MANAGED_ENV_IN_EDITOR. This variable should be set on the project setting to unlock this propagation. This approach ensures that users are aware of the implications and potential benefits of the propagation.

const filter_env = (env) =>
    Object.entries(env).reduce((acc, [key, value]) =>
          key.startsWith('FORGE') ? acc : {...acc, [key]: value}, {});

const appEnv = Object.assign({},
    this.settings.env.FORGE_ALLOW_MANAGED_ENV_IN_EDITOR ? filter_env(process.env) : {}, 
    this.env, 
    this.settings.env)

Related Issue(s)

Editor environment variables: allow controllable propagation of env set to Editor process with the hosting operators

Checklist

Labels

knolleary commented 1 year ago

Hi @elenaviter

Can I suggest that when you are raising an issue for discussion and a PR soon after, please link to the issue directly rather than copying it all over into the PR description.

The issue contains the background information and we can discuss the requirements and needs it addresses The PR can link to the issue for context, and keep its description focussed on the technical changes the PR is introducing.

That makes it easier for us to focus the discussion in the right place. We want to make sure we understand the why of an issue to help us understand the what of a PR - as there could be a number of different ways to solve the problem posed in the issue.

But that said, thank you for your contributions. I'm out of time today to get into the fine details of it, but I'll pick this up on Monday.

elenaviter commented 1 year ago

Hi @knolleary, thank you for helping me sort it out 🙂 I've updated the description of this PR, so left the short summary of env vars source and a description of the approach this PR is trying to solve the problem.

elenaviter commented 1 year ago

Hi, @knolleary,

regarding your point about the need for a mechanism to control the setting of environment variable FORGE_EXPOSE_HOST_ENV: I agree that it's essential to have control over such environment variable settings.

3 parts of this problem I see:

  1. Ability to issue declarative preset of FORGE_EXPOSE_HOST_ENV on Admin level
  2. Prevent any attempt to override what's defined by Admin for this feature in any way other than initiated by Admin.
  3. Ability to scope the Teams and stacks/templates. E.g. there are might be teams who work with the cloud and are authorized to leverage cloud access, and there're teams who only work on public APIs integrations (w/o need an access to cloud), and thus probably in contrary, should be sandboxed from direct access to cloud.

How to accomplish 1 and 2?

Everything which is defined in stacks / templates is already under Admin-only scope, so that if there's a way to lock some preset on that level then we solve "modification risk" part of the problem.

  1. Through a template that already offers predefined environment variables and even the ability to "lock" some of them.
  2. Alternatively, this could be done at the k8s stack level if
    • there's a feature similar to one which exists in template over there, e.g. allowing predefined environment variables (and "locking" some)
    • or an "enable expose host env" option that would set FORGE_EXPOSE_HOST_ENV to True/False based on selection.
  3. This can also be achieved at the Docker image level of the chosen stack. However, {maybe I am missing something) this approach isn't different from 2 (in 2 at least it would be indicated by ticked "enable expose host env" or env var), while in addition doesn't provide any indication in the admin interface that a particular stack declares such a special image. (Might be not intuitive. There should be an indication of a special resource that controls this unique aspect, which is crucial for those who work on Node-RED based cloud solutions).

In general, any of these methods would be appropriate under the policy "host environment variable permissions apply to all FlowForge projects across all teams" (if I am mistaken, please correct me). E.g. could cover problems 1 and 2. Currently, since all templates are visible to all team owners, and all stacks are available to all teams, regardless of where the admin defines FORGE_EXPOSE_HOST_ENV (stack, template, or image specified for a stack), any team owner can create a project that allows host environment variables as long as such a stack (or template, depending on where the flag is set) exists in the stack list (or the template is in the template list). Which means that problem 3 is not covered.

knolleary commented 1 year ago

I had been assuming this was going to be a platform wide flag, rather than being able to administratively set it on some instances but not others. This is why using an env var as a flag made sense - because that could be setup in the container itself.

But to have this be more selectively applied does make it more complicated to achieve.

I can see a couple possible ways forward given what we have in place today.

We recently introduced the TeamType concept. This can be used to make different things available to different teams. That includes what InstanceTypes are available to which teams. The InstanceType (aka ProjectType) is what identifies the container that should be deployed for an instance of that type.

You could:

  1. Create a container for 'regular' users
  2. Create a container for your 'elevated' users - this should set the env var to enable pass-through.
  3. Create an InstanceType using the 'regular' container
  4. Create an InstanceType using the 'elevated' container
  5. Create a TeamType for 'regular' teams. Configure it to only provide access to the 'regular' InstanceType
  6. Create a TeamType for 'elevated' teams. Configure it to only provide access to the 'elevated' InstanceType

That then gives you a way to provide different levels of access to different teams. However, it isn't a perfect solution. The TeamType implementation is very much aimed at what we required in FF Cloud. I can see some things you might want to behave differently and we'd have to think about how to achieve that:

  1. If an InstanceType is unavailable to a given TeamType, it sill gets listed, just not selectable - we'd need an option to hide unavailable types, rather than make them unselectable.
  2. There are no restrictions on Team Owners being able to change their team type to any of those available - we'd need an option that somehow restricts team type changes - this could be as simple as a single top-level option to disable the ability to change TeamType.

There may well be other things to consider.

A slightly different approach is, as you mention, to do this at the Template level. As above, you'd still create two different TeamTypes, 'regular' and 'elevated'. We'd then need the ability to limit a Template to a particular TeamType. You'd then be able to create two Templates, one with the flag set, one without. You'd still also need the ability to lock the value etc, and also the ability to disable changing TeamType.

Again, there may well be other things to consider that I have overlooked.

elenaviter commented 1 year ago

Thank you @knolleary, if we go with

A slightly different approach is, as you mention, to do this at the Template level. As above, you'd still create two different TeamTypes, 'regular' and 'elevated'. We'd then need the ability to limit a Template to a particular TeamType. You'd then be able to create two Templates, one with the flag set, one without. You'd still also need the ability to lock the value etc, and also the ability to disable changing TeamType.

what are our next actions is order to solve this story?

knolleary commented 1 year ago

I think there are a number of steps needed to be able to achieve what you want. Step one is getting separate user stories raised to cover the different pieces once we understand the requirements.

As mentioned, there are some UX questions around TeamTypes and how they would work in your situation. It's a bit tricky for me to tell you what is needed here - I can make some observations as to the current UX, but we do need your input on the desired UX.

I believe the following items are things you'd want to be able to do, but not currently possible.

elenaviter commented 1 year ago

Hey @knolleary, Firstly, I appreciate the thoroughness of the discussion, the long-term impacts and the broader application are important. However, I wanted to express an immediate need on our end. We're eager to start using the licensed version for our internal development teams as soon as possible. The feature introduced in this PR (and in Device Node-RED app environment variables) are essential for us to progress using the original, non-patched versions of the FlowFuse libraries.

Our goal is to start with this foundational set of features, understand its impacts, and then certainly expand to wider teams with the extended capabilities you are discussing.

Given this, could we perhaps focus on the minimal steps or changes required to get this PR to a state where it can be merged and used by our teams? This would tremendously aid our current development cycle, and we remain committed to collaborating on future enhancements based on the ongoing discussion.

Thank you for your understanding and looking forward to your guidance on this.

knolleary commented 1 year ago

Given this, could we perhaps focus on the minimal steps or changes required to get this PR to a state where it can be merged and used by our teams? This would tremendously aid our current development cycle, and we remain committed to collaborating on future enhancements based on the ongoing discussion.

Looping in @MarianRaphael as our Product Owner.

It depends on which of your requirements you are prepared to drop for the initial iteration.

In FF Cloud we do not want to start exposing all env vars to the Node-RED instances. If this behaviour is controlled by a flag that any Team Owner is able to set via an env var on the Instance, then we effectively loose the ability to prevent users from bypassing this restriction however they want.

If, for a first iteration, you have happy for all instances to be able to access the host env vars, then we can update this PR to use an env var flag (not one provided by Instance settings), and you can create a custom Node-RED container with this flag set.

If you want some teams to have this behaviour and some teams to not have this behaviour, then we need some mechanism to give different teams a different configuration. We do not have that today and will require additional development work.

elenaviter commented 1 year ago

@knolleary, this definitely unblocks us now:

If, for a first iteration, you have happy for all instances to be able to access the host env vars, then we can update this PR to use an env var flag (not one provided by Instance settings), and you can create a custom Node-RED container with this flag set.

Do you want me to make the change in this PR to achieve this?

knolleary commented 1 year ago

@elenaviter yes please, I think it just needs this.settings.env.FORGE_EXPOSE_HOST_ENV changing to process.env.FORGE....

elenaviter commented 1 year ago

@knolleary - done, now we read env-unlocker from container env.

May I ask - can I apply the same changes in similar PR for Device?

knolleary commented 1 year ago

@elenaviter yes, lets keep the device consistent with this behaviour.

elenaviter commented 1 year ago

@knolleary fixed the problems found by linter