Confusing behavior when append/prepend to PATH with no delimiter

[schneems]

Context

I was building a simple Ruby CNB when I came across this behavior. I prepended /layers/heroku_ruby/ruby/bin to the PATH, but found that echo $PATH ended up being the whole layer path twice:

$ docker run --entrypoint='/cnb/lifecycle/launcher' my-image 'echo $PATH'
/layers/heroku_ruby/ruby/bin/layers/heroku_ruby/ruby/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

That was confusing. I thought perhaps it was expecting a relative path instead of absolute, so I tried pretending bin and got this output:

$ docker run --entrypoint='/cnb/lifecycle/launcher' my-image 'echo $PATH'
bin/layers/heroku_ruby/ruby/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

That behavior was also confusing. After a lot of debugging, the cause was that I was pretending to PATH without specifying a delimiter.

Observations

• In https://github.com/buildpacks/spec/blob/main/buildpack.md#delimiter, the spec does not describe what happens when there is no delimiter set
• The spec on the lifecycle seems to indicate that <layerpath>/bin will always be on the PATH. However, it's possible to modify the PATH by accident, so that's no longer true https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-paths
• In the spec "The lifecycle MUST separate each path with the OS path list separator (e.g. : on Linux, ; on Windows)." this behavior does not hold for PATH values in prepend.

Paths forward

• The specification should state what behavior we expect when there's no delimiter as this behavior is currently undefined (by the spec)

In addition, I would like to propose some possible future changes to the spec:

• Make env vars using prepend and append an error unless a delimiter is specified.
• Possibly: Default the delimiter for PATH environment variables when none is present.
• Possibly: Always place bin of a layer on the PATH even if a CNB modifies the PATH.

[jromero]

@buildpacks/buildpack-authors-tooling-maintainers care to provide some input here?

[ekcasey]

@schneems

In addition, I would like to propose some possible future changes to the spec:

Make env vars using prepend and append an error unless a delimiter is specified.
Possibly: Default the delimiter for PATH environment variables when none is present.
Possibly: Always place bin of a layer on the PATH even if a CNB modifies the PATH.

The bin of a layer is in fact automatically prepended to the PATH with the correct delimiter. This is why you are seeing "the whole layer path twice" in the first example, and /layers/heroku_ruby/ruby/bin after bin in your second example.

I could still see a case for default the PATH delimiter in cases where buildpacks explicitly prepend or append to the PATH but given the above functionality those cases are less common than one might imagine.

[schneems]

The bin of a layer is in fact automatically prepended to the PATH with the correct delimiter

Correct, but if you have any prepend values without a delimiter, that PATH that was auto added is no longer valid as it’s treated as one whole path.

I could still see a case for default the PATH delimiter in cases where buildpacks explicitly prepend or append to the PATH but given the above functionality those cases are less common than one might imagine.

You’re saying appending and prepending is not common? I use them a bunch. Even if a failure mode isn’t common I think we should at minimum warn when there is likely incorrect behavior, ideally error or auto fix the problem (with an escape valve). The problem is less about the specific failure mode expectations, and more about the difficulty of debugging this failure mode when things go wrong.

[hone]

We talked about this a bit at the core team sync today. From what I understand we don't want to special case PATH and deal with edge cases. What do we do if a buildpack sets this to NOT : for unix?

I do concur with @schneems, there is a default delimiter if we do nothing that the spec is already doing with every <layer>/bin. This defaults to the OS Path separator. If we do nothing here, then every buildpack author will have to implement this logic. libcnb can of course paper over this. Right now, we're encouraging everyone to just shove everything into <layer>/bin to have things "just work".

[dmikusa]

I have been hit by this before too. I don't think it was with PATH, but with appending to other env variables. The default of no delimiter was confusing and took me a minute to figure out what was going on. I think if it had defaulted to an actual value, like :, it would have been easier to spot the problem. i.e. why is it a:b:c:d? Oh, right I need a custom delimiter cause I want a,b,c,d. Whereas abcd is unexpected.

This is just my experience and other's may have different experiences but, I can't think of a case where I'm appending/prepending values in a buildpack and I actually want to just concat all the values together. I most commonly want a path delimiter, followed by space or comma. If we defaulted to the path separator, I think that would provide a more natural behavior. Even if it's not right in all cases, it's right for many and in the cases where it's not right, it's pretty obviously not right.

For the Windows/Unix scenario, it seems like the platform should know that and be able to adjust automatically. So the spec could say it defaults to the OS path separator.

[schneems]

One way to think of this is that there is a default now, it's just effectively the same as a zero-width string.

If we treat this as a current default then a possible proposal going forward could be to deprecate and replace it with a new default:

• Deprecate the current "default" (zero width string): If someone is using prepend/append without setting a value, emit a warning saying the behavior will change and if they want to preserve behavior they must write an empty delimiter file. The presence of the file will let us know unambiguously that they intended that behavior.
• In a new release change the default delimiter to be system delimiter by default for all env vars (don't special-case PATH variants). If someone wants the delimiter to be something different, then they'll see the : as part of their prepend value when they inspect the environment variable and that will give them a breadcrumb that will hopefully lead to the discovery of how to manually change the delimiter.

What do you think?

[ekcasey]

@schneems and @dmikusa-pivotal

You make some good points.

I would prefer requiring a delimiter to changing the default b/c

1. If buildpack authors miss the warnings (say for example they skip a version when upgrade the buildpack API) they could silently get unexpected behavior
2. If the assumption is that "" is almost never a good default, failing is more explicit than picking a default like : and requiring folks to follow the breadcrumbs

[dmikusa]

@ekcasey I'm OK with that.

So if you have no delimiter set, then it fails. If you have a delimiter of ``, that's OK. Otherwise, you need to set your own delimiter to something useful. The buildpack author would be required to handle ; vs `:` OS path delimiter differences.

[hone]

@ekcasey I'm OK with that.

So if you have no delimiter set, then it fails. If you have a delimiter of ``, that's OK. Otherwise, you need to set your own delimiter to something useful. The buildpack author would be required to handle ; vs `:` OS path delimiter differences.

yeah, that kind of sucks :(, though not sure how many windows + unix buildpacks we have out in the wild.

[schneems]

In general, I don't want to know about OS differences, I want to use APIs that abstract those differences for me. My preference as a buildpack author would be to default to the OS delimiter.

Brainstorming an alternative: We could have CNB write out the delimiter to a file for us so we could read that in and use it instead of having to implement our own OS switching detection. That's one way that I wouldn't have to think about it, but I would still prefer to not have to jump through that hoop by using a default.

Regarding the worries around the deprecation cycle: I would implement the deprecation warning in the current major version, then change the behavior in the next major version rev. I would expect developers to upgrade to the latest patch release of a major version before trying to jump major versions. Basically, if they do that, it's a little on them that they didn't see the warning and skipped the major version breaking changes changelog.

A more difficult, but safer alternative to relying on the developer to upgrade versions in a safe manner could be to track prior versions of the spec being used (somehow) and emit a diff of deprecations since the prior stored version. For example if you jumped from 0.2 to 1.2 then that could be detected and you could get a list of everything deprecated between those versions. I think that such tracking and warning could be very user friendly, but I'm betting it's overly complicated compared to a simple deprecate/change cycle (which is already a bit time consuming).

[edmorley]

I would love to resolve this.

At the very least we should fix the spec to defined the existing behaviour - since having it undocumented (as well as an unhelpful default), doesn't help with user confusion.

Next, we need to decide whether to:

1. Still have a default delimiter, but change it to the path separator
2. Or, require that an explicit delimiter be set (and error if not)

In addition, there were questions above about whether anyone was relying on the current default of the empty string, and so would be broken by (1).

To help answer this, I took a look at usages in the wild using GitHub code search.

Starting with Heroku's libcnb.rs powered buildpacks, I found zero usages of the default empty string delimiter, a handful of cases of a single space being used as a delimiter (for JAVA_TOOL_OPTIONS, MAVEN_OPTS, SBT_OPTS), and the majority being the path separator:
https://github.com/search?q=org%3Aheroku+%2F%5CbModificationBehavior%3A%3A%28Append%7CPrepend%29%5Cb%2F&type=code

Looking at the Paketo buildpacks, which use either packit or the Go based libcnb env APIs, I similarly found zero usages of the default empty string delimiter, a handful of cases of a single space being used as a delimiter (for JAVA_TOOL_OPTIONS, PIP_FIND_LINKS), and the majority being the path separator:
https://github.com/search?q=org%3Apaketo-buildpacks+%28%22.Prepend%28%22+OR+%22.Append%28%22%29&type=code

Finally, looking at the GCP buildpacks, I again found zero usages of the default empty string delimiter, one case of a single space being used as a delimiter (for NODE_OPTIONS), and all others being the path separator:
https://github.com/search?q=repo%3AGoogleCloudPlatform%2Fbuildpacks+%2FEnvironment%5C.%28Prepend%7CAppend%29%5C%28%2F&type=code

As such:

• It seems the current empty-string default delimiter is definitely not helpful
• We shouldn't need to worry about breaking lots of people relying on the empty string delimiter (given we're allowed breaking changes in new versions of the spec anyway)
• IMO there's a strong case for making the OS path separator the default value (rather than requiring an explicit value), given it's used in the majority of cases. Plus having it as a default means that end users don't need to all reimplement the "calculate the OS path separator for the current OS" behaviour.

[natalieparellano]

IMO there's a strong case for making the OS path separator the default value (rather than requiring an explicit value), given it's used in the majority of cases

Just to be clear, OS path separator would be the default delimiter for ALL env vars, not just PATH, right?

@edmorley would you be willing to open an RFC for this?