Add higher level environment handling #30

Malax · 2021-07-07T14:51:09Z

Adds higher level environment handling, decoupling layer environment variables from the filesystem. This is the first step towards a declarative API for layer environment variables where buildpack authors don't have to write environment variables to disk, greatly improving testability due to no side-effects.

In a nutshell, this implements the layer environment variable handling that is done in the CNB lifecycle at runtime. I ported over the relevant tests from the CNB lifecycle source to ensure testing parity.

This will be integrated into layer lifecycles in a dedicated PR (let's face it, this PR is already too large).

Please refer to the included RustDoc for an overview how this works and is used. I copied the RustDoc for LayerEnv below:

LayerEnv RustDoc

Represents environment variable modifications of a Cloud Native Buildpack layer.

Cloud Native Buildpacks can add a special directory to their layer directories to modify the
environment of subsequent buildpacks, the running container or specific processes at launch.
The rules for these modifications are described in the relevant section of the specification.

This type decouples this information from the file system, providing a type-safe in-memory
representation of the environment delta that is specified in the env/* directories of a layer.
Using this type, libcnb can provide declarative APIs that enable buildpack authors to easily
test their layer environment variable logic since they no longer write them to disk manually.

One use-case are environment variables that are modified by a layer that are required by the
same buildpack in later stages of the build process. For example, a buildpack might install a
build tool (i.e. Apache Maven) in one layer and adding the main binary to PATH via the env
directory of that layer. The same buildpack then wants to execute Maven to download dependencies
to a different layer. By using LayerEnv, the buildpack can encode these changes in a
type and, in addition to passing it to libcnb which will persist it to disk, pass it to the
logic that uses the build tool to download dependencies. The download process does not need to
know the layer name or any logic how to construct PATH.

Applying the delta

LayerEnv is not a static set of environment variables, but a delta. Layers can modify existing
variables by appending or prepending or setting new ones only conditionally. If you only need a
static set of environment variables, see [Env].

To apply a LayerEnv delta to a given Env, use [LayerEnv::apply] like so:

use libcnb::layer_env::{LayerEnv, TargetLifecycle, ModificationBehavior};
use libcnb::Env;

let mut layer_env = LayerEnv::empty();
layer_env.insert(TargetLifecycle::All, ModificationBehavior::Append, "VAR", "bar");
layer_env.insert(TargetLifecycle::All, ModificationBehavior::Default, "VAR2", "default");

let mut env = Env::empty();
env.insert("VAR", "foo");
env.insert("VAR2", "previous-value");

let modified_env = layer_env.apply(TargetLifecycle::Build, &env);
assert_eq!(modified_env.get("VAR").unwrap(), "foobar");
assert_eq!(modified_env.get("VAR2").unwrap(), "previous-value");

Implicit Entries

Some directories in a layer directory are be implicitly added to the layer environment if they
exist. The prime example for this behaviour is the bin directory. If it exists, its path will
be automatically appended to PATH using the operating systems path delimiter as the delimiter.

A full list of these special directories can be found in the
Cloud Native Buildpack specification.

libcnb supports these, including all precedence and lifecycle rules, when a LayerEnv is read
from disk:

use libcnb::layer_env::{LayerEnv, TargetLifecycle};
use tempfile::tempdir;
use libcnb::Env;
use std::fs;

// Create a bogus layer directory
let temp_dir = tempdir().unwrap();
let layer_dir = temp_dir.path();
fs::create_dir_all(layer_dir.join("bin")).unwrap();
fs::create_dir_all(layer_dir.join("include")).unwrap();

let layer_env = LayerEnv::read_from_layer_dir(&layer_dir).unwrap();

let env = Env::empty();
let modified_env = layer_env.apply(TargetLifecycle::Launch, &env);

assert_eq!(modified_env.get("PATH").unwrap(), layer_dir.join("bin"));
assert_eq!(modified_env.get("CPATH"), None); // None, because CPATH is only added during build

schneems · 2021-08-02T15:39:31Z

Gus-W-9528533

src/env.rs

src/layer_env.rs

LayerEnv becomes LayerEnvDelta as it can only handle one set of env entries. For layer paths, we have a separate, implicit, list of deltas that must not be overwritten directly by user provided entries. LayerEnvDelta is also required to support env/ env.build/ env.launch/ as well as process specific layer env entries.

Code in the same module requires the entries to have a consistent order for both iterating over and comparing it. To ensure that consistency, we now use the appropiate collection type: BTreeSet.

To make it more consistent with the ecosystem. Also adds Default implementions for both types.

edmorley

Thank you for working on this!

src/env.rs

src/layer_env.rs

schneems

This was a beast to review. I'm betting much more so to actually write. Great job!

It was easier to review than I was originally anticipating. Credit to you and Rust.

I left some comments as I went. Nothing blocking except maybe for the case of mis-spelling a process.

For style, I generally like a space after markdown headers here are some docs for diesel.rs that have an extra space after headers https://docs.diesel.rs/src/diesel/query_builder/functions.rs.html#80-82. If you want I could send you suggestions for the rest of them.

schneems · 2021-08-30T20:10:32Z

src/layer_env.rs

+/// logic that uses the build tool to download dependencies. The download process does not need to
+/// know the layer name or any logic how to construct `PATH`.
+///
+/// # Applying the delta


Extremely pedantic, but can we get whitespace after headers. It helps me pick them out in the source.

Suggested change

/// # Applying the delta

/// # Applying the delta

///

I'm not against adding newlines after markdown headings, but I'm hesitant to do the changes to this PR as we have other headings outside of this PR as well. If you feel strongly about this, I suggest we make DR about this, figure out how to enforce this automatically via a lint and ship a dedicated PR with the lint and the fixes. Is that okay for you?

I was curious to see what rustdocs/markdown related lints Clippy had already - there are a few listed at https://rust-lang.github.io/rust-clippy/stable/index.html but none that cover this case.

I did find rust-lang/rust-clippy#1007 which covers adding more docs related lints.

src/layer_env.rs

schneems · 2021-08-30T20:22:24Z

src/layer_env.rs

+///
+/// let mut layer_env = LayerEnv::new();
+/// layer_env.insert(TargetLifecycle::All, ModificationBehavior::Append, "VAR", "bar");
+/// layer_env.insert(TargetLifecycle::All, ModificationBehavior::Default, "VAR2", "default");


As I'm looking at this code, I'm curious if there's a more concise way to represent this logic:

layer_env.insert(TargetLifecycle::All, ModificationBehavior::Append, "VAR", "bar");

For example (without implementation constraints) could be:

layer_env.all.append("VAR", "bar"); layer_env.build.append("VAR", "bar"); layer_env.launch.append("VAR", "bar");

Or something like:

layer_env.append_all("VAR", "bar"); layer_env.append_build("VAR", "bar"); layer_env.append_launch("VAR", "bar");

What you've got now is great to move forward. As I'm trying to figure out what makes a "good" interface in Rust, I would be interested to hear your thoughts (Again, non-blocking...we can take it offline to chat).

Filed as #73

src/layer_env.rs

schneems · 2021-08-30T20:36:58Z

src/layer_env.rs

+    /// env.insert("VAR", "foo");
+    /// env.insert("VAR2", "previous-value");
+    ///
+    /// let modified_env = layer_env.apply(TargetLifecycle::Build, &env);


I was confused by this TargetLifecycle::Build here. I couldn't figure out why TargetLifecycle::All was being specified above, but just build here. If I'm understanding correctly it's because we're currently in a build environment so we only want to apply modifications to build to the current env.

That makes sense as I type it out, but it was unclear on first pass. Right now I'm trying to think of cases where we would want to assign to the current env but have it not be build. I'm not sure I can come up with any.

I'm also left somewhat wanting to compress/combine LayerEnv and Env updates. It's a two step process but seems like I always want to do them in lockstep. I would need to see it utilized in a buildpack, but I bet that I'll insert the env var and then forget to update the current env.

Maybe some interface like this would help prevent that situation:

layer_env.insert_and_apply(TargetLifecycle::All, ModificationBehavior::Default, "VAR2", "default", &env);

I've added this comment to #73

schneems · 2021-08-30T20:51:23Z

src/layer_env.rs

+                let mut process_deltas = vec![&self.all];
+                if let Some(process_specific_delta) = self.process.get(&process) {
+                    process_deltas.push(process_specific_delta);
+                }


This can fail silently if someone passes in a process that they've typo-d TargetLifecycle::Process("w3b") instead of "web". It looks like if that happens then the process would get ALL of the env vars which is likely not want they want (but maybe better than nothing). We should warn or error (panic).

We've got 2 cases, either someone typo-d when creating the delta or when applying the delta. To be on the safe side we shouldn't allow applying to a process we've not inserted into (I think). Alternatively, maybe issue a warning. I guess I'm suggesting we panic in that situation.

It looks like if that happens then the process would get ALL of the env vars

I can see why you think that, but that's not the case. self.all contains all env var deltas that apply to all target lifecycles, not all deltas across all lifecycles.

With that out of the way, I don't think panicking is a good idea here. There is nothing wrong to apply the deltas for a process that doesn't have specific environment variables. If the user of this library passes in the wrong process type string, this is nothing the library should attempt to fix.

I guess I'm struggling to see the use-case where someone would be using process-specific .apply() during the build, to understand what the user expectations might be? (I can see why we need to write out process-specific env files, but in my mind they mostly would be useful for runtime.)

If there is a use-case during build it's most likely an edge-case. This PR includes support for applying them because it is mostly free in terms of required code. If we'd remove the ability to apply process specific environment deltas, we'd need more code as TargetLifecycle would be split up into two: one for specifying, one for applying. I can imagine the code being harder to follow too.

src/layer_env.rs

Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>

Malax · 2021-08-31T08:27:15Z

Thank you @edmorley for the spelling and sentence structure corrections!

hone · 2021-08-31T20:02:21Z

src/env.rs

+
+    /// Returns a cloned value corresponding to the given key.
+    pub fn get(&self, key: impl AsRef<OsStr>) -> Option<OsString> {
+        self.inner.get(key.as_ref()).cloned()


@Malax Isn't it usually common here to return a borrowed and not cloned from an object?

hone · 2021-08-31T20:03:11Z

src/env.rs

+    }
+}
+
+mod test {


shouldn't this be in #[cfg(test)] so it's not built for non test envs?

This is where rust-lang/rust-clippy#1719 would be useful :-)

edmorley · 2021-09-28T17:36:23Z

I've created GUS-W-9959585 to track adding the next part of this.

Base automatically changed from malax/0.2.0 to main July 13, 2021 17:30

edmorley reviewed Aug 4, 2021

View reviewed changes

src/env.rs Show resolved Hide resolved

src/env.rs Show resolved Hide resolved

src/layer_env.rs Outdated Show resolved Hide resolved

src/layer_env.rs Show resolved Hide resolved

Malax force-pushed the malax/layer-env branch 2 times, most recently from 71d4b76 to 57dce3f Compare August 26, 2021 14:27

Malax added 17 commits August 26, 2021 16:29

Initial environment implementation

d6cb58d

Re-introduce insert function to modify a LayerEnv

e18c548

Improve insert implementation

acd9dce

Implement LayerEnvDelta.write_to_env_dir

725ac1e

Add Ordering

530a486

Rename variable #type to modification_behavior

05784d3

Store LayerEnvDeltaEntries in BTreeSet

44c58dd

Code in the same module requires the entries to have a consistent order for both iterating over and comparing it. To ensure that consistency, we now use the appropiate collection type: BTreeSet.

Add newline between comments and use in env.rs

6261a67

Generalize LayerEnv.apply_for_build for any TargetLifecycle

fff207d

Remove automatic type conversion from Env::get

ce334ca

Add LayerEnv RustDoc

e5e726a

Rearrange order of types in layer_env.rs

4992f50

Fix override behaviour for LayerEnv entries

9d9e091

Fix clippy warnings

2596a02

Fix RustDoc typo

ecc4585

Add write_to_layer_dir for LayerEnv

b8c5959

Malax force-pushed the malax/layer-env branch from 57dce3f to b8c5959 Compare August 26, 2021 14:29

Malax added 2 commits August 26, 2021 17:10

Add support for launch layer paths, fix build layer paths

a7ca1b6

Add minor docs

66246d5

Malax force-pushed the malax/layer-env branch from af33844 to 66246d5 Compare August 26, 2021 15:29

Malax requested review from hone and edmorley August 26, 2021 15:38

Rename Env::empty and LayerEnv::empty to ::new

803d4fa

To make it more consistent with the ecosystem. Also adds Default implementions for both types.

Malax marked this pull request as ready for review August 27, 2021 09:42

edmorley approved these changes Aug 30, 2021

View reviewed changes

schneems approved these changes Aug 30, 2021

View reviewed changes

Fix documentation spelling/typos

28eed13

Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>

Malax merged commit 7fe6993 into main Aug 31, 2021

Malax deleted the malax/layer-env branch August 31, 2021 16:49

hone reviewed Aug 31, 2021

View reviewed changes

This was referenced Sep 8, 2021

Explore ways to make the LayerEnv API more consise #73

Closed

Return a borrowed not cloned object from Env::get #74

Closed

Add missing #[cfg(test)] to Env's tests #75

Closed

edmorley assigned Malax Nov 10, 2021

This was referenced Nov 15, 2021

Remove PlatformEnv #157

Merged

Refactor LayerLifecycle #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add higher level environment handling #30

Add higher level environment handling #30

Malax commented Jul 7, 2021 •

edited

Loading

schneems commented Aug 2, 2021

edmorley left a comment

schneems left a comment

schneems Aug 30, 2021

Malax Aug 31, 2021

edmorley Aug 31, 2021

schneems Aug 30, 2021

edmorley Sep 8, 2021

schneems Aug 30, 2021

edmorley Sep 8, 2021

schneems Aug 30, 2021

Malax Aug 31, 2021

edmorley Aug 31, 2021

Malax Aug 31, 2021

Malax commented Aug 31, 2021

hone Aug 31, 2021

edmorley Sep 8, 2021

hone Aug 31, 2021

edmorley Sep 1, 2021

edmorley Sep 8, 2021

edmorley commented Sep 28, 2021

Add higher level environment handling #30

Add higher level environment handling #30

Conversation

Malax commented Jul 7, 2021 • edited Loading

LayerEnv RustDoc

Applying the delta

Implicit Entries

schneems commented Aug 2, 2021

edmorley left a comment

Choose a reason for hiding this comment

schneems left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Malax commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley commented Sep 28, 2021

Malax commented Jul 7, 2021 •

edited

Loading