Optional isolated environment

[fmoehler]

This is the same as PR #663 . The creator of the PR is currently OOO. Therefore this PR is opened.

The bosh-cli wipes parts of the environment provided by the host-system which are in some cases essential for compilation of bosh-releases. This PR follows a conservative fix-approach by adding an option to opt-out of this default. This can (and maybe should) be extended by a fix of the “isolation” from the host-environment. For more details, see #660

[anshrupani]

looks good

[aramprice]

I'd prefer to keep this complexity out of the bosh cli since the issue this appears to be addressing could also be side-stepped by running the bosh CLI in a docker container.

Perhaps there is some broader issue I'm missing here though?

Regarding the code changes, it seems like there are a number of additions, specifically for nix, beyond a "don't isolate my environment" flag, I'm wondering if those need to be in the bosh-cli repo?

Regarding the feature I am concerned that allowing more variation in the local environment will increase the surface area we will have to consider when triaging issues with the CLI.

[aramprice]

Perhaps a default value of "/usr/local/bin:/usr/bin:/bin:/sbin" should remain here and we allow an override with something like BOSH_CPI_COMAND_PATH.

[aramprice]

I think there are two core changes lurking in this PR

1. Allowing the current ENV to be passed to the CPI command

• My inclination is for this logic to live exclusively in the bosh-cli codebase, and not rely on the bosh-utils' system.execCmdRunner.buildComplexCommand() handling of cmd.UseIsolatedEnv at all.

It seems like this change alone might be sufficient for the requestors needs.

2. Allowing the the PATH value passed to the CPI to be overridden

• This could be handled by using BOSH_CPI_COMAND_PATH or defaulting to the existing value. However this is accomplished the CLI's default behavior should be the same (e.g. PATH=/usr/local/bin:/usr/bin:/bin:/sbin).

This might be a nice additional change if there is a need for a mostly isolated ENV except with modifications to PATH.

The other nix specific files don't belong in this repo.

Side note: bosh-cli appears to be the only project in the cloudfoundry org which sets system.Command.UseIsolatedEnv perhaps this capability could be removed if we handle the "isolated or not" logic into the bosh-cli?

[FloThinksPi]

@fmoehler @aramprice
indeed there are quite some files not relevant here imho. They should be removed like the nix files and envrc etc.
Also i think removal of some code parts is not neccecary e.g. the BOSH_CPI_USE_ISOLATED_ENV variable seems to be removed here. Dont know if thats intended this variable is also not documented in the bosh docs.

broader issue I'm missing here though?

The broader issue as far as i remember is that the isolation feature in case of create-release is somewhat pointless on systems other than ubuntu. It basically boils down to bosh-cli does not accept the environment variable the OS gives it - however then it proceeds calling OS packages (gcc etc.) and fails either not finding those as it looks not in the path the OS told the process to look in but in a hardcoded one. Or It cannot find the proper libraries development headers when compiling with GCC since it also does not pass the information the OS has given via ENV variables to the GCC process.

In this state afaik the current create-release with enabled isolation(default?) does not work on every linux/unix system that has all the needed tools/libraries installed, it just works on systems that have the tools installed how ubuntu places them into its filesystem specifically.

One way would also be to state that just ubuntu is supported and one should run this inside a ubuntu container/vm - however compared to whats needed to make it work on all unix systems it might be easy to enable all linux/unix systems when holding on to basic POSIX concepts - but the exclusion exists for windows it seems already #626

However now looking into the code i saw there is also BOSH_CPI_USE_ISOLATED_ENV - maybe it can be used to disable isolation of env entirely then building a bosh release would also work on a non ubuntu linux as bosh and all build tools behind it would respect the OS env. WDYT ?

Also disabling that feature and using all OS Envs would be ok as a solution maybe BOSH_CPI_USE_ISOLATED_ENV does this need to test that maybe.

[jpalermo]

We talked about this at the FIWG meeting.

We think a good direction is to just always enable the isolated environment within the bosh cli. Even removing the current flag that allows you to disable it. And then have the CLI pass in the full set of ENV variables that a CPI might need to run (PATH, LIBRARY_PATH, etc)

This might cause some breakages, but we can just deal with those if they arise.

[jpalermo]

@beyhan and @rkoster , do either of you remember what we talked about in the FIWG meeting regarding this? I can't find the video for the 12/19 meeting and my comment above only confuses me.

I think what my comment is saying is: "Always enable isolated environment, but then pass through ENV that includes PATH and things that might be needed"

But wouldn't it be more simple if we instead "Always disabled isolated environment, and then just let it use whatever is already in the ENV"?

[beyhan]

But wouldn't it be more simple if we instead "Always disabled isolated environment, and then just let it use whatever is already in the ENV"?

@jpalermo I can't follow what you mean. If we don't want to relay on the bosh-utils option UseIsolatedEnv we could just define a new option with UseHostOSEnv or UseOSEnv (because UseIsolatedEnv is a little bit confusing IMO and doesn't describe what happens really) and based on that option we decide in https://github.com/cloudfoundry/bosh-cli/blob/main/cloud/cpi_cmd_runner.go#L112-L122 whether we initialise the env as it is happening currently or use the OS env as done here https://github.com/cloudfoundry/bosh-utils/blob/c837f5a2ec9037ca1459b49dee470b9d6e4014a5/system/exec_cmd_runner.go#L85. Personally, I would prefer a solution which doesn't change the current behaviour for the default usage.

If allowing an override of the passed PATH value to the CPI is still needed we can talk about introducing a BOSH_CPI_COMAND_PATH as Aram suggest but it is not clear to me whether it is needed if OS env usage is implemented.

[jpalermo]

I'm just wondering if we actually get anything of value from UseIsolatedEnv.

We've had it configured this way since 2014, but it's not clear why we're doing this. The original commit doesn't describe any problem that it is solving. It seems like more of a "this sounds like a good idea" sort of thing.

Up until now, it hasn't really caused any major problems, other than the PR in 2022 to add the flag to disabled parts of isolated env (but PATH is still overridden).

But the BOSH_CPI_USE_ISOLATED_ENV and this new PR kind of show that the IsolatedEnv is problematic. And if we can't describe why we need it, does it really make sense to be adding a bunch of flags and conditional behavior to work around it?

Why not just rip isolated env out of the bosh-cli entirely?

[a-hassanin]

I'm just wondering if we actually get anything of value from UseIsolatedEnv.

We've had it configured this way since 2014, but it's not clear why we're doing this. The original commit doesn't describe any problem that it is solving. It seems like more of a "this sounds like a good idea" sort of thing.

Up until now, it hasn't really caused any major problems, other than the PR in 2022 to add the flag to disabled parts of isolated env (but PATH is still overridden).

But the BOSH_CPI_USE_ISOLATED_ENV and this new PR kind of show that the IsolatedEnv is problematic. And if we can't describe why we need it, does it really make sense to be adding a bunch of flags and conditional behavior to work around it?

Why not just rip isolated env out of the bosh-cli entirely?

If that is the case, i guess we can create a 2nd PR to just rip it out entirely (Y)

[fmoehler]

I also agree with @jpalermo , but am just wondering if removing this entirely could be a security risk? Don't really know if that is something to consider, just came to my mind.
If it is (or could be), then I think the initial solution (also including this change) to "hide" this feature behind a flag as proposed here is fine.

[jpalermo]

Update from FIWG meeting today. Plan is to just remove isolated environment from the cli and let it use whatever is in the ENV