SHIP-0004: Build Environment Variables #10
Conversation
There was a problem hiding this comment.
Hm, certainly less complex. But, it forces the build user to know the names of the steps in the build strategy. Today ...
- most of our sample build strategies (actually all beside s2i) today have only one real step (ignoring helper steps like prepare and results) where environment variable overrides make sense
- as far as I can see it, no build strategy has any step combination where an environment variable needs to be overwritten for one of the steps but must not be set on the others because it would otherwise break them. (or is there any env variable that makes sense for s2i that would break kaniko or buildah; or vice versa?)
So, is it really necessary that our build users need to know the names of the strategy steps?
I think establishing a community convention where "build" is the main step in any BuildStrategy would help here. Users wouldn't need to know this particularly detail offhand to inject environment variables in 80-90% of build strategies.
Proxy environment variables could fall into this bucket. I'd rather err on the side of caution here, than have a case down the road where we need to twist our API to apply environment variables to only certain steps in a build strategy. |
adambkaplan
changed the title
|
Hm, I do understand your mindset of being prepared for future edge cases. But, we should take care to not let those pay the price that are on the 95 % path. Question now is: can we find an easy solution for those use cases without restricting those who need to go for the edge case:
I do not have the perfect suggestion for this, but some ideas:
|
|
@SaschaSchwarze0 I've added a CLI enhancement to the proposal so that users can easily see the steps of a build strategy. I wish we could have surfaced this via |
This makes a lot of sense.
Another good idea - enhance the cli so developers can pass env vars into all build steps. |
fwiw the still need to make a pass at this EP @adambkaplan @SaschaSchwarze0 before I can comment with some level of intelligence on the other points, but this specific point was "makeable now" :-) |
There was a problem hiding this comment.
Just a minor potential update suggestion (I want balk much if you disagree @adambkaplan ) and a question around the current non-goals and the initial discussions we had around the long term potential wrt other apects of the step/container field in the strategy.
+1 on the its simpler, more conservative (which is admittedly safer at this point wrt user input into the project)
Otherwise, from the reviewers of my PR, aside from @SaschaSchwarze0 and whether he and his users can at least progress with the needing to know the build step element, I'm curious what @sbose78 thinks, as he was the one who prompted me about "allow" list type functionality. Among other things, I don't recall if his input stemmed from any early customer discussion or not.
Certainly you have a explicit way of declaring a finite set of what is allowed with this approach. An an implicit way of declaring what is not allowed (i.e. if it ain't in a step already, it is not allowed).
But sure, let's see what feedback we get once we implement this, and how much of a call for decoupling from strategy build step knowledge, and as a result, potential need for explicit not allowed / deny lists, and then add that later on if need be.
|
/approve |
There was a problem hiding this comment.
I´m late to the party. Adding minor and major request for changes. I know you folks have been very diligent on this particular topic, so kudos for that. I read @gabemontero proposal in advance, to get the right rational for this one.
|
|
||
| ## Proposal | ||
|
|
||
| ### User Stories |
There was a problem hiding this comment.
It would be good if we can layout more elaborated user stories. For example, I took a look on the Tekton/Catalog, as this should provide real examples on how people uses env support there. The majority of the use cases are a combination of:
- [1] env vars + the
scriptfield per step - [2] env vars defined because someone knows in advance that the image in the step will read it ( very specific knowledge )
Because today we dont support the script field, our use case today for envs vars is merely related to [2]. That does not mean that we cannot support [1] in the future, so some ideas for more user stories:
- Using env vars allows in the future to provide values to env variables defined under the script field of a step. Note: See Move BuildStep out of corev1.Container build#742 (comment) for more info on the decision of the script field adoption
- Using env vars to support the definition of CNB_PLATFORM_API. See example on how Tekton uses this in related doc. Today we use whatever default we get from the binaries.
There was a problem hiding this comment.
Interesting to note that your example with the Tekton catalog entry for buildpacks, they expose build time environment variables as a Tekton parameter: https://github.com/tektoncd/catalog/blob/main/task/buildpacks/0.3/buildpacks.yaml#L32-L34
This IMO validates the problem statement (developers need to be able to manipulate arbitrary environment variables) and our solution (provide an API to append environment variables)
adambkaplan
force-pushed
the
ship-step-overrides
branch
2 times, most recently
from
f1ae8cd
to
b3d620b
Compare
|
@gabemontero @SaschaSchwarze0 @qu1queee @sbose78 this is ready for final review as an Implementable proposal. |
There was a problem hiding this comment.
@adambkaplan nice work! I'm requesting two minor things from my side, I'm happy to approve after that!
|
tell you what @qu1queee I'll /approve and let you lgtm once @adambkaplan updates basaed on you comments yesterday |
ah never mind :-) I'll lgtm if need be after your comments are addressed @qu1queee |
adambkaplan
force-pushed
the
ship-step-overrides
branch
from
f70f47c
to
cfe9909
Compare
This proposal provides a mechanism for developers to append environment variables to a Build or BuildRun. Many programming languages and frameworks use environment variables to tune the behavior of their tooling. For image building tools that aggregate dev frameworks like Cloud Native Buildpacks, this capaiblity is important because the number of environment variables that can be tuned is unbounded. Other developer tools like maven use environment variables to inject private repository credentials. This proposal defines the behavior of how environment variables are defined at the BuildStrategy, Build, and BuildRun levels. It also proposes enhancements to the shp CLI that make it easier for developers to set environment variables at build time.
adambkaplan
force-pushed
the
ship-step-overrides
branch
from
cfe9909
to
b137f63
Compare
|
@qu1queee @gabemontero updated |
|
/lgtm |
|
/lgtm @sbose78 you are marked as an approver on this EP. Do you mind giving your final thoughts pls. |
| #### Story: Alter build behavior via environment variables | ||
|
|
||
| As a nodejs developer using buildpacks | ||
| I want to set the `NPM_INCLUDE` environment variable in my Build and BuildRuns |
|
|
||
| As a build strategy author | ||
| I want to keep fixed values for some environment variables in my build strategy | ||
| So that I don't allow end users to enable insecure or potentially harmful features. |
|
|
||
| As a Java developer using maven to build my application (via buildpacks or source-to-image) | ||
| I want to use environment variables to pass usernames and passwords to values defined in my `pom.xml` file | ||
| So that I don't store sensitive information in source control. |
| ### Risks and Mitigations | ||
|
|
||
| There is a risk that developers will want an environment variable to be set in one build step, but unset or restored to a default value in a different build step. | ||
| This could be addressed in a future enhancemenent via separate API field (ex: `excludeFromSteps`). |
| ## Drawbacks | ||
|
|
||
| The primary drawback is that build strategy authors need to provide fixed or default values for environment variables they want to control. | ||
| A strategy author cannot simply declare that an environment variable is "off limits" - they must declare when a controlled env var is used and what value it should have. |
There was a problem hiding this comment.
👍 This is a concern I've had, but we can start with what we have in this proposal.
Also, when should we really care about off-limit environment variables ? When a user uses an environment variable to break out of the sandbox of a container. However, a secure container platform wouldn't let you do that if the pod isn't being run with elevated privileges/capabilities.
There was a problem hiding this comment.
CVE-2021-20182 is a truly compelling case for a build author to declare an environment variable off limits. Setting the BUILDAH_ISOLATION env var to chroot AND running with the privileged: true security context exposes devices on the node (storage, network, etc.).
|
|
||
| This proposal won't allow developers to set environment variables in some build strategy steps, but not others. | ||
| This capability implies that the developer knows some information about the build strategy, which the community consideres an advanced skill. | ||
| The Shipwright community does not expect developers to know the names of the steps declared in build strategies. |
| A strategy author cannot simply declare that an environment variable is "off limits" - they must declare when a controlled env var is used and what value it should have. | ||
| This is a fair compromise to establish a clear contract between a build strategy author and the developer augmenting the build strategy's behavior. | ||
|
|
||
| This proposal won't allow developers to set environment variables in some build strategy steps, but not others. |
There was a problem hiding this comment.
This is fine. The likelihood of an environment variable being useful in more than one step is extremely low since the steps would ideally accomplish pretty distinct tasks.
| In that proposal, it was incumbent on the build strategy authors to declare which environment variables could be overrode, and which environment variables could not be set. | ||
| This was accomplished through allow/block lists, as well as a separate list of environment variables that could be declared with default values (and which were not declared in build steps). | ||
| The downside of this proposal was that it severed the relationship of an environment variable from the place where the environment variable is used. | ||
| Its complexity also led to competing paths to implement similar capabilites. |
There was a problem hiding this comment.
Fair enough. I did see a value in that. However, it did add complexities, agreed.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabemontero, sbose78 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
This proposal introduces the concept of build strategy overrides,
whereby a developer can override the behavior declared in a build
strategy. Augmenting the environment variables in a build step is
presented as the first type of override that is made available.
The proposal includes rules and behavior for environment variables that
are declared in a build strategy. Specifically, builds cannot overwrite
declared environment variables unless said env var is exposed as a build
parameter.