add EP for environment variable API #726
Conversation
openshift-ci-robot
added
the
release-note-none
There was a problem hiding this comment.
Nice that you take care of that @gabemontero. I just by coincidence in our internal grooming session yesterday explained how important that feature is for Buildpacks users. Did a first quick read through it.
docs/proposals/env-vars.md
Outdated
| direct line to the Kubernetes Container API in their API. As as result, environment variable fields from `Steps`/`Containers` | ||
| are directly accessible from Tekton API. | ||
|
|
||
| Specifically [Tekton TaskRuns](https://github.com/tektoncd/pipeline/blob/main/docs/taskruns.md) reference |
There was a problem hiding this comment.
Steps are defined in the spec of a Task, not a TaskRun.
There was a problem hiding this comment.
yes but there is a ptr to a TaskSpec in the TaskRun, and the Steps are in the TaskSpec definition.
if that is not clear my text above I'll add more verbiage
There was a problem hiding this comment.
For me is okay but as you point out: TaskRuns do not reference the steps directory but they are defined in the referenced Task or in the (like in Shipwright) inlined TaskSpec.
There was a problem hiding this comment.
ok finally back to this today .... even if you and I are okay, I'm making the relationship chain more explicit and clear in my next update
docs/proposals/env-vars.md
Outdated
| } | ||
| ``` | ||
|
|
||
| Then, the specs of all `BuildStrategy` (both cluster and namespaced scope), `Build`, and `BuildRun` will be |
There was a problem hiding this comment.
In the BuildRun, there would not anymore be a need for a default value. At the latest at this point, the value needs to be specified by the user.
There was a problem hiding this comment.
reusing the type across the board is attractive to me here
a bit of a stretch, but if a user was uncertain about the contents of a configmap or secret used with valueFrom (perhaps they reconcile as blank) having the default could a safe fallback
so I'm inclined to keep as is ... but if that reasoning is not sufficient for you, please elaborate and we'll go from there
There was a problem hiding this comment.
I was coming from the assumption that we always use this ordering to determine a value:
BuildRun overwrites Build overwrites BuildStrategy.
Based on that, the BuildRun would define the final value. A default therefore would not have made sense.
The valueFrom might make sense but seems to be complex to implement. So, if I understand you correctly, your scenario would be:
env:
- name: USERNAME
valueFrom:
secretKeyRef:
name: my-secret
key: username
default: NOT_DEFINEDThe designated behavior would be that if the secret exists and if it contains the key username, then the value of that key would be used, otherwise the value would have to be NOT_DEFINED?
This would be fairly complex for us to implement, I think. When creating the TaskRun, we would need to go through all environment variables. We would need to perform the evaluation that Kubernetes would do = we would need to parse the secretKeyRef, load the secret and check if it has the username key. Only if all that is the case, we would define the environment variable on the TaskSpec step using the secretKeyRef. Otherwise we would directly set the value?
That would be fairly complex given valueFrom supports some more options.
Thinking through this even more ... let's maybe take a step back: do we have use cases for valueFrom ? Or would a simple name / value setting be good enough for a build user?
I think we also need to distinguish build/buildrun and buildstrategy at one aspect: in the build and buildrun, you probably plan to introduce an env section in their spec? But in a buildstrategy, the author is defining the steps=containers where environment variables can be defined today already. Can you clarify how this comes together?
There was a problem hiding this comment.
So a lot of good discourse to unpack there @SaschaSchwarze0. Bottom line, I believe I need to make some adjustments.
But how to convey those so we can reach consensus? Here we go ... but while I was finishing this post, @SaschaSchwarze0 and I chatted on slack and also agreed to set up a video conf for us to discuss.
So while we can all continue to talk here (I also have comments from @otaviof I need to get to), this can also serve as additional input into the conversation he and I will have.
-
Overall, the notion of conveying a staging plan wrt this EP is a possibility. I think we could incrementally add to an env var API as part of Shipwright if need be. I'll circle back to that based on where we land. Then, if need be, a stage 1 could be simple k/v pairing, a stage 2 maybe could be say
valueFrom(though I think it should be a stage 1 item, with possible pull out if something unexpected arises), followed by a probablenofordefaults. More on defaults in a bit, but perhaps I've overstretched wrt that. -
IMO, we do have uses cases for
valueFrom, again with the caveat that we do not have to necessarily address them in an "initial" stage if when we get to implementation there is uncertainty on the nature of the change.
Those use cases?
First, I already have a link for this in this EP, but Tekton's specific example at https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#using-a-secret-as-an-environment-source where they seed the GITHUB_TOKEN based on what is pulled from a k8s secret, is pervasive enough IMO that I at least could see Shipwright users liking this form.
A lot of tools like git, maven, etc. used in building are big on environment variables, and in the case of sensitive data, storing that data and retrieving it from k8s Secrets is a de facto mode of operator at this point.
And a first blush, it seems like the reconciliation of the BuildRun to TaskRun is straight forward, as we map the analogous BuildRun API to the TaskRun API seen in that link.
Hopefully with that, along with cleaning things up around the strategy already has steps/containers point you made (more on that below), that will remove the complexity concern.
But I think it is fair to say that a path to Shipwright will be Tekton users that desire the value in standardizing image building. And even though Tekton is a Shipwright implementation detail, analogous semantics like this, that leverage core k8s resources, seems like a distinct possibility.
Second, we also have this analogous scenario wrt build tools and this precise feature with OpenShift Builds. Please see https://docs.openshift.com/container-platform/4.7/cicd/builds/creating-build-inputs.html#builds-using-secrets-as-environment-variables_creating-build-inputs
And from a historical perspective, that feature arose from customer feedback (again, how they are use to supplying tokens to build tools via env vars). Long term, as we migrate OpenShift Build users to Shipwright, not having this could be a feature gap and obstruction to migration.
Now, back to the staging, again, perhaps we do not do this initially based on how the initial implementation goes, but I think we should strive for doing it initially, and only back it out if we see encounter something unexpected in mapping from BuildRun to TaskRun during the implementation PR.
But minimally though, if we don't have direct mapping of this features for these two potential user sets, we then have a more complex migration of "how you do this in a different form via Shipwright" story that we'll need to convey.
- Defaulting ... yeah, my motivation here stemmed from my exploration of Tekton Triggers, and their use of defaults with the parameters for
TriggerTemplates. Fixed some bugs in this space, and overall the feature resonated with me.
That said, none of the existing stakes in the ground wrt environment variables I noted above applied the defaulting concept.
Based on that and your initial interpretation, I'm going to back off of it in the main proposal section. I'll probably add some mention of it in alternatives, where I'll mention the idea was entertained, but we moved off of it.
- How it comes together .... strategy already having steps/containers. Yeah I missed the need for more detail here, thanks. And I think your comment hit on the choice we have here.
Setting env var's on the build step(s) is already available, so is that sufficient for setting things at the BuildStrategy level?
Do we allow setting of env vars at the BuildStrategySpec level, that apply to all steps/containers?
I could see staging coming into play here, where we at least initially we do not add a new field for multi container override, and then wait for user feedback.
Then, separate from those choices, the precedence decision I've noted in the doc, where the admin and dev sort out whether the BuildStrategy or BuildRun take precedence, or if we do not provide that choice and decide on an order, at least initially builds on top of those choices above.
Controlling the precedence order could also be another staging item. Where initially we just pick an order, perhaps "developer" focused, where the BuildRun wins, and then we let administrators block that with kyverno / gatekeeperm though I could go the other way as well, and then add configuration / control around the order later.
docs/proposals/env-vars.md
Outdated
| `BuildStrategy` would also have a `EnvironmentVariable` array named `DisallowedEnvironmentVariables` that lists `EnvVar` settings which are not allowed | ||
| and will be filtered in the final application onto the `TaskSpec`. |
There was a problem hiding this comment.
Without having deeply thought about this ...
- black listing might also be something on the deployment level.
- white listing might be interesting as well, for example in Paketo, environment variables are typically prefixed with
BP
You may also add some words about what happens if a Build or BuildRun contains some environment variables that are not allowed.
There was a problem hiding this comment.
ah yes I only implied here that filtering would occur from the unallowed list
do we update status/conditions?
do we add logs / events?
do we prevent execution (seems extreme ... but maybe an opt in for the administrator)?
I'll ponder and update, thanks. Of course chime in with any immediate opinions on how to answer those questions.
There was a problem hiding this comment.
I think the behavior should be the same as for params, there I had this exchange with Enrique: https://github.com/shipwright-io/build/pull/697/files#r604954782. So, I in general prefer that we do not just silently filter. I consider writing to the controller's container log or events silently because that's not where a build user would necessarily look at. I would prefer the validation to happen as early as possible - with webhooks, this would means to prevent the creation of the build or buildrun. With our current approach, their status would be not registered or not succeeded with reasonable reasons. Anyway, when we create the TaskRun, we always need to validate again because the build strategy might have changed after a build was created (and successfully validated by an eventually existing webhook or registered by today's approach).
There was a problem hiding this comment.
I need to re-review the merged parameters EP in details, but on the surface, I'm good with being consistent and updating this EP as needeed for consistency.
+1 on non-silent filtering, the earlier the better, all that.
@SaschaSchwarze0 and I can dive into this more when we chat.
docs/proposals/env-vars.md
Outdated
| Then, via the [current global config of controller env vars](https://github.com/shipwright-io/build/blob/v0.4.0/docs/configuration.md), | ||
| or if that current approach gets converted to a `ConfigMap`, or if the community decides to create a `ConfigMap` for each | ||
| logical piece of "feature configuration", we have a setting that allows a switch to "ADMINISTRATOR_CENTRIC" order of precedence, | ||
| say: | ||
|
|
||
| - apply `BuildRun` envs first | ||
| - then `Build` | ||
| - then `BuildStrategy` |
There was a problem hiding this comment.
What would be the use case for this?
There was a problem hiding this comment.
The use case I mentioned above that the administrator wants to enforce specific settings for env vars
There was a problem hiding this comment.
Good, agree. For me this is not a global configuration but rather a case-by-case decision for every environment variable. Earlier I commented that we will need to distinguish - in the build strategy - between spec.env (what you propose to add) vs spec.buildSteps[].env. A reasonable approach imo would be that those environment variables explicitly defined on a step cannot be overwritten. An example would be the BUILDKITD_FLAGS environment variable of the BuildKit build strategy.
There was a problem hiding this comment.
Yeah perhaps avoiding a global config and supporting case by case would be an argument for having both spec.env and spec.buildSteps[].env .... where the latter is the final override, and the former is the "set up an initial value, but allow the BuildRun to override.
We can settle on this during our discussion @SaschaSchwarze0
| - then `Build` | ||
| - then `BuildStrategy` | ||
|
|
||
|
|
There was a problem hiding this comment.
Looking for some more information on where we set those environment variables eventually. I assume on all steps defined in the build strategy = not including those steps/containers that we dynamically define like to retrieve sources ?
There was a problem hiding this comment.
Yeah I can add more details here based on how we resolve items above after our conversation.
| - Any needed change to integrate Shipwright with Tekton parameters is handled by the [current 'Parameterize Build Strategies' EP](https://github.com/shipwright-io/build/pull/697) | ||
| or follow up work that stems from that EP. Until then, only static content of any environment variable value related | ||
| fields is supported. In other words, the `$(params...)` syntax seen in Tekton today. | ||
|
|
There was a problem hiding this comment.
To make it clear, it may make sense to add a statement that this EP is not about environment variables that will be set in the final container image.
There was a problem hiding this comment.
sure that distinction has value ... I'll add
There was a problem hiding this comment.
this EP is not about environment variables that will be set in the final container image.
Agree as well. Perhaps we rename this proposal to "execution environment variables"?
There was a problem hiding this comment.
Interesting perspective about the environment-variables, @gabemontero. Although, one thing I missed is yaml based examples on how the final API would look like, in addition to the Golang snippets.
Thanks!
docs/proposals/env-vars.md
Outdated
| Then, the specs of all `BuildStrategy` (both cluster and namespaced scope), `Build`, and `BuildRun` will be | ||
| updated with an array of `EnvironmentVariable` | ||
|
|
||
| `BuildStrategy` would also have a `EnvironmentVariable` array named `DisallowedEnvironmentVariables` that lists `EnvVar` settings which are not allowed |
There was a problem hiding this comment.
Thinking out loud, I wonder if we should invert this logic and let administrators define "allowed environment variables" instead. Like for instance HTTP_PROXY, an administrator would like to leave a list of proxy servers that are allowed; in comparison, would be difficult to add a list of servers that aren't allowed.
There was a problem hiding this comment.
Yeah I could see both allow and deny lists being desirable @otaviof .... I tried to indicate that providing some form of both may be a item to strive fo, though maybe we stage one before the other wrt to the notion of a staging plan I proposed in response to some of @SaschaSchwarze0 's comments. Though based on some of the exchanges up above, I have some optimism we devise and order of precedence that gets us suitable allow/deny capabilities.
Bottom line though, I need to add more details to the portion you've highlighted here.
docs/proposals/env-vars.md
Outdated
| - then `BuildRun` | ||
|
|
||
| Then, via the [current global config of controller env vars](https://github.com/shipwright-io/build/blob/v0.4.0/docs/configuration.md), | ||
| or if that current approach gets converted to a `ConfigMap`, or if the community decides to create a `ConfigMap` for each |
There was a problem hiding this comment.
Would you please add more on why a ConfigMap would be useful/needed on this process?
There was a problem hiding this comment.
I'd rather not detail why that shift might occur as part of this EP, but defer to whichever EP is created to dive into that, if the change in fact occurs.
#651 was opened an issue to track this potential change. I'll add a qualifier here to reference that issue and potential/subsequent EP for details.
That sound OK to you @otaviof ?
There was a problem hiding this comment.
Yes, it does. I'll watch out for #651, thanks!
Yep I'll look to compliment the golang with yaml @otaviof thanks |
|
OK @SaschaSchwarze0 and I are meeting on Friday to discuss things face-to-face / voice-to-voice and live iterate to zero in on what we want Reach out to @SaschaSchwarze0 if you want to me added to the invite I've pushed a preliminary update in a separate commit with my attempt to rework things based on feedback from @SaschaSchwarze0 and @otaviof that hopefully helps progress our call on Friday. Of course additional comment in the interim is welcomed, where we'll try to factor that into the Friday discussion. |
|
fyi I hope to push new changes today that add yaml examples of valueFrom secret usage i.e. GITHUB_TOKEN and/or some of "popular" maven envs as well as using parameters |
pushed as separate commit for now |
docs/proposals/env-vars.md
Outdated
| `BuildStrategy` needs some further consideration, in that `BuildStep` already inlines `Container`, which allows specification | ||
| of `EnvVar`. | ||
|
|
||
| Those existing `BuildStep` / `Container` environment variables cab be the way an administrator could ensure that |
| become a preferred method when building images in k8s native environments. | ||
|
|
||
| Next, the question of whether to use the k8s `EnvVar` directly exists. This proposal suggests creating a new | ||
| Shipwright type that inlines the k8s type, in case we want to extend and add new features later that require |
There was a problem hiding this comment.
Counter point - we don't need a separate type right now, and a future type will need to extend corev1.EnvVar to be backwards-compatible. Without a concrete, motivating feature why not keep things simple?
docs/proposals/env-vars.md
Outdated
| There are also a set of features in Tekton around setting values for environment variables in a dynamic fashion. | ||
|
|
||
| While the list of scenarios Shipwright goes after is a subset of what Tekton goes after, as the [Motivation](#motivation) | ||
| section will explain, manipulation of environment variable settings on the resulting k8s Container objects exits |
| underlying Tekton `Step`/`Container` is sufficient for an initial implementation and scenarios we want to support. | ||
| That will help the initial implementation in that the explicit "name" index field helps with default kubernetes | ||
| "path" API operations that tend to occur with devops/gitops flows. Also, `ValueFrom *EnvVarSource` still allows | ||
| us to pull content from `Secrets`, like authentication token. Many of the tools when building images (including |
| name: something-that-uses-docker-buildrun | ||
| spec: | ||
| environmentVariables: | ||
| - name: DOCKER_API_VERSION |
There was a problem hiding this comment.
Is this the only environment variable the Build/BuildRun is allowed to set in this example ?
There was a problem hiding this comment.
nope per our exchange below
docs/proposals/env-vars.md
Outdated
| - then from `BuildRunSpec.[]EnvironmentVariable`, assuming any names from `BuildStrategySpec.([]string)DisallowedEnvironmentVariable` are not used, | ||
| where values are replaced from any entries previously seeded | ||
| - then from `BuildStrategySpec.[]BuildStep`, assuming any names from `BuildStrategySpec.([]string)DisallowedEnvironmentVariable` are not used, | ||
| where values are replaced from any entries previously seeded |
There was a problem hiding this comment.
I was thinking of another approach:
-
Everything specified in
BuildStrategySpec.[]BuildStepmay be overriden in Build/BuildRun unless specified otherwise in.spec.[]DisallowEnvironmentVariableOverride. -
.spec.[]DisallowEnvironmentVariableOverridewould contain the list of environment variables that a user is not allowed to be set irrespective of whether there was a default specified inBuildStrategySpec.[]BuildStep. -
.spec.[]EnvironmentVariableis not needed anymore because defaults could be specified in theBuildStepitself.
( did you intend BuildRunSpec.[]EnvironmentVariable to be an allow-list of what env vars can be specified in build/buildRun ?)
There was a problem hiding this comment.
yep good question @sbose78 .... need some more work here
BuildRunSpec.[]EnvironmentVariablein intended to be defaults essentially- but that said, admins might want to start with an "allow no envs, then list a small set of envs that can be used" vs. "allow all evns, then lis a small set of envs that cannot be used"
I'll add a .spec.[]AllowedEnvironmentVariables field, along with some thought / explanation on how Allow... and Disallow... work together.
Spoiler: Allow... and "allow no envs, then list a small set of envs that can be used" will take precedence.
There was a problem hiding this comment.
Agreed @gabemontero , sounds good!
There was a problem hiding this comment.
update pushed @sbose78
gabemontero
force-pushed
the
env-var-ep
branch
2 times, most recently
from
ff4c692
to
8138dec
Compare
docs/proposals/env-vars.md
Outdated
|
|
||
| If `AllowedEnvironmentVariables` is empty, assume all are allowed, except for those mentioned in `DisallowedEnvironmentVariables`. | ||
| Otherwise, the list is the list, and any environment variables in `BuildStep`, `Build`, and `BuildRun` must be vetted against this list. | ||
| Initially, no wildcards or regexp, but perhaps a feature to consider in the future. |
There was a problem hiding this comment.
I think you can remove this line as you later mention
Each entry of the allow and disallow arrays need to minimally support wildcards, if not regular expressions.
|
suggestions pulled in and commits squashed @SaschaSchwarze0 - thanks ! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SaschaSchwarze0 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-robot
added
the
approved
There was a problem hiding this comment.
With regards to the personas this EP is targeting, there are two that I feel have overlapping concerns:
- Cluster admins (secure builds)
- Build strategy authors (define best practices)
I'd like to see us focus on the build strategy author's perspective vs. cluster admin. To me, the strategy author's aim is to define the best practice for using their tool on Kubernetes. We're heading in the right direction with the strategy allow/block list. However, the separate environmentVariables spec confuses me, since strategy authors define environment variable defaults in their build steps.
| - Any needed change to integrate Shipwright with Tekton parameters is handled by the [current 'Parameterize Build Strategies' EP](https://github.com/shipwright-io/build/pull/697) | ||
| or follow up work that stems from that EP. Until then, only static content of any environment variable value related | ||
| fields is supported. In other words, the `$(params...)` syntax seen in Tekton today. | ||
|
|
There was a problem hiding this comment.
this EP is not about environment variables that will be set in the final container image.
Agree as well. Perhaps we rename this proposal to "execution environment variables"?
docs/proposals/env-vars.md
Outdated
| That said, one could also see cluster administrators wanting certain environment variables to never be set in their | ||
| cluster, don't use certain values, or that certain environment variables always use certain values. |
There was a problem hiding this comment.
For this EP I fall back on our experience with OpenShift builds. We have default environment variables that can be applied to all builds in a cluster, but we don't force environment variables to have a specific value unless it is explicitly used in the build process. I would not consider overrides/block lists at the cluster level as in scope.
There was a problem hiding this comment.
My initial thought here was the black box nature of openshift builds actually shielded its exposure of environment variables from some of the more complex concerns that can occur as the box is opened up with tekton based shipwright.
I agree with cycling through each feature / use case on its own to see if it remains, but I am not in entire agreement that the openshift build experience is 100% applicable.
docs/proposals/env-vars.md
Outdated
|
|
||
| ## Release Signoff Checklist | ||
|
|
||
| - [ ] Enhancement is `implementable` |
There was a problem hiding this comment.
Are we calling this implementable?
There was a problem hiding this comment.
updating the check list will be part of the next push
| - Shipwright API needs to articulate a first class way for specifying those environment variables approach for both | ||
| administrators and individual developers (i.e. authors/owners of the BuildStrategies vs. authors owners of the Builds | ||
| and BuildRuns) to specify those environment variables. |
There was a problem hiding this comment.
For the sake of simplicity I would focus on the experience as individual developers and build strategy authors.
Administrative control is something that should be additive and based on demonstrated need. I would consider it a non-goal.
There was a problem hiding this comment.
@sbose78 @SaschaSchwarze0 refresh my memory on your respective takes on administrative controls. IIRC, between the two of you, goals you desired included
- prevention of overriding in build/buildruns of env var k/v defined in the strategy
- specification of finite sets of env vars that could be speficied
Are those incorrect? Or did I miss something?
As I noted in #726 (comment) @adambkaplan perhaps a suitable compromise is laying out a long term road map of a complete set of features in this space in this EP, but implementing them in stages.
But some of that is dependent on making sure of the desire amongst the other reviewers on these administrative controls.
There was a problem hiding this comment.
Correct, there are environment variables specified in a build strategy that must not be overwritten in a build or buildrun. And yes, it makes sense that build strategy authors define which environment variables can be set. I mainly focus on the Buildpacks scenario where the different Buildpacks are configurable using environment variables that start with BP, such as BP_MRI_VERSION for Ruby.
There was a problem hiding this comment.
Sounds like we are in agreement that we need allow/block lists as a basic primitive for build strategy authors. IMO higher level "cluster admin" controls can be built on top of these primitives in the future.
docs/proposals/env-vars.md
Outdated
|
|
||
| First, some role / actor terminology and detail (nothing new most likely for Shipwright community members): | ||
|
|
||
| - A `developer` or `end user` is the person who has been provisioned a namespace, or shares a namespace with some teammates, |
There was a problem hiding this comment.
Format nit - backticks are for code, quotes are better here.
| - A `developer` or `end user` is the person who has been provisioned a namespace, or shares a namespace with some teammates, | |
| - A "developer" or "end user" is the person who has been provisioned a namespace, or shares a namespace with some teammates, |
|
|
||
| Then, via the [current global config of controller env vars](https://github.com/shipwright-io/build/blob/v0.4.0/docs/configuration.md), | ||
| or if that current approach gets converted to a `ConfigMap`, per (https://github.com/shipwright-io/build/issues/651)[https://github.com/shipwright-io/build/issues/651], or if the community decides to create a `ConfigMap` for each | ||
| logical piece of "feature configuration", we have a setting that allows a switch to "ADMINISTRATOR_CENTRIC" order of precedence, |
There was a problem hiding this comment.
Agree that we shouldn't do this - stick to one behavior only.
docs/proposals/env-vars.md
Outdated
| - `AllowedEnvironmentVariables` | ||
| - `DisallowedEnvironmentVariables` |
There was a problem hiding this comment.
This is perhaps a bit too misleading, since this controls env var overrides at the Build/BuildRun level. Perhaps rename as:
allowedEnvVarOverridesblockedEnvVarOverrides
We should validate that only one of the two are set for a given BuildStrategy/ClusterBuildStrategy. This can be done directly on the generated CRD via OpenAPI - no need for validating admission webhooks.
There was a problem hiding this comment.
No those are suppose to be general allow / block lists. Though initial reaction I do like blocked... better than disallowed...
Prevention of overrides at the moment is specifying k/v pairs in the existing BuildStep/k8s Container env var array vs. the new array (which in response to another comment you had, minimally needs a rename, if we keep at all).
At this point, it perhaps makes sense to have a common voice to voice / face to face conversation with the current set of reviewers on the "administrative control" themed features.
Based on where we land with that, we can resume this thread as needed. Where even if we keep these, field names and the surrounding text in the EP perhaps need to be clearer.
There was a problem hiding this comment.
webhook
on the no need for validating webhooks, we may need to cross reference your take here with @SaschaSchwarze0 's #726 (comment)
where part of that I think arose in the recent parameters EP
| additional fields or new types from k8s or Tekton. So with that, we start with | ||
|
|
||
| ```go | ||
| type EnvironmentVariable struct { |
There was a problem hiding this comment.
Shorten to EnvVar (and envVar in our YAML representation?)
There was a problem hiding this comment.
I was worried about confusion with the existing k8s structs, but I'll acquiesce, unless others jump in with that similar concern. I'll give a day or so to see if that occurs before making the switch.
There was a problem hiding this comment.
We should use k8s namings if they correspond directly to what gets added to a pod spec. Developers won't need to context switch.
docs/proposals/env-vars.md
Outdated
|
|
||
| Each string array list the `EnvVar` names/keys to consider. | ||
|
|
||
| If `AllowedEnvironmentVariables` is empty, assume all are allowed, except for those mentioned in `DisallowedEnvironmentVariables`. |
There was a problem hiding this comment.
| If `AllowedEnvironmentVariables` is empty, assume all are allowed, except for those mentioned in `DisallowedEnvironmentVariables`. | |
| If `AllowedEnvironmentVariables` is empty, assume all environment variable overrides are allowed, except for those mentioned in `DisallowedEnvironmentVariables`. |
docs/proposals/env-vars.md
Outdated
| disallowedEnvironmentVariables: | ||
| - 'DOCKER_TLS_VERIFY' | ||
| environmentVariables: | ||
| # for convenience, we add the version most our users can use, but we'll allow you to override | ||
| - name: DOCKER_API_VERSION | ||
| value: '1.19' | ||
| buildSteps: | ||
| - command: | ||
| - /usr/local/bin/docker | ||
| - build | ||
| env: | ||
| - name: DOCKER_CONFIG | ||
| value: /tekton/home/.docker |
There was a problem hiding this comment.
I am a bit confused by this API. Are environmentVariables applied to every build step with the value provided? Can a developer override the DOCKER_CONFIG env var?
I would almost rather have build strategy authors define the env vars in line with the build steps, and not worry about (redundantly) declaring default values in environmentVariables. Values that can't be overrode should be declared clearly and not rely on hidden behavior. As I understand the behavior in the EP, I think the following would provide more clarity:
| disallowedEnvironmentVariables: | |
| - 'DOCKER_TLS_VERIFY' | |
| environmentVariables: | |
| # for convenience, we add the version most our users can use, but we'll allow you to override | |
| - name: DOCKER_API_VERSION | |
| value: '1.19' | |
| buildSteps: | |
| - command: | |
| - /usr/local/bin/docker | |
| - build | |
| env: | |
| - name: DOCKER_CONFIG | |
| value: /tekton/home/.docker | |
| disallowedEnvironmentVariables: | |
| # Don't allow docker to push to an insecure registry | |
| - 'DOCKER_TLS_VERIFY' | |
| # Don't allow the pull/push secrets to be overrode from what Shipwright provides | |
| - 'DOCKER_CONFIG' | |
| buildSteps: | |
| - command: | |
| - /usr/local/bin/docker | |
| - build | |
| env: | |
| - name: DOCKER_CONFIG | |
| value: /tekton/home/.docker | |
| environmentVariables: | |
| # for convenience, we add the version most our users can use, but developers can override at build time. | |
| - name: DOCKER_API_VERSION | |
| value: '1.19' |
There was a problem hiding this comment.
Yes they are applied to build steps, but then those k/v paris could be overwritten by the same keys specified in builds or build runs.
Where as the items specified directly in the BuildStep cannot be overridden.
But assuming we go with this "administrative control", let's dive into the alternative noted by @SaschaSchwarze0 in #726 (comment)
steps:
- name: my-build-tool
env:
- name: MY_FIXED_ENVVAR
value: "fixed value"
overwritable: false # default: true
- name: MY_DEFAULT_VALUE_ENVVAR
value: "default value"
Well, if you consider the current def of BuildStep:
type BuildStep struct {
corev1.Container `json:",inline"`
}
To achieve that yaml, we need to either
a) create a shipwright.Container type, with all the existing corev1.Container fields, but with the envVar array field change to have to be an array field of a new type that has the overrwritable flag.
That seems like a more disruptive change. And fragile given the field complexity of corev1.Container.
or
b) create a
type BuildStep struct {
corev1.Container `json:",inline"`
EnvVarOverwritable map[string]bool
}
Where the key to that map is the env var name, and then true/false indicates whether the build run or build can overwrite.
Of those two, at least for me b) is the more tolerable of the two. But it is a slight departure from the yaml @SaschaSchwarze0 noted.
When I originally wrote this, b) was not as palatable as what is currently in the EP, but I could be talked into switching to b) now if the current form is too lacking in intuitiveness.
There was a problem hiding this comment.
@adambkaplan and I have had a detailed conversation.
We agree on going with
type BuildStep struct {
corev1.Container `json:",inline"`
EnvVarOverwritable map[string]bool
}
And losing environmentVariable array that was added to the strategy. I'll be making that update later today.
There was a problem hiding this comment.
actually I think I'm going to change that EnvVarOverwritable map[string]bool to EnvVarOverwritable []stringl
if the key is not in the list, it is not overwritable.
so by default, any existing env vars prior to our change to BuildStep are not overwritable
docs/proposals/env-vars.md
Outdated
| ## Drawbacks | ||
|
|
||
|
|
There was a problem hiding this comment.
What are the drawbacks to this approach? Does our API and planned implementation preclude some future behavior?
| Assuming this gets delivered while Shipwright builds is still v1alpha1, we just have to announce to users | ||
| what new API is provided. | ||
|
|
||
| But if the implementation of this is delayed to the point that we are at say v1beta1, then we have to |
There was a problem hiding this comment.
I don't think we can confidently ship a v1beta1 API without this implemented.
|
thanks for all the feedback @adambkaplan I've pushed a commit with updates from your review that I could accept with little effort. PTAL if you have cycles in the short term. The remaining comments are items I want to ponder on some more before addressing specifically. |
After going over this one a bit @adambkaplan, I'm not on the same page with you yet. My initial issue with this is that if build strategy author is not tied to more universal k8s or ci/cd roles, or described independently in such a way that existing shops which have cluster admins and the like can sort out who should be a build strategy author, it has little meaning to users coming in looking to adopt. If build strategies were only namespaced scope, I may be more amenable. But we have cluster scoped ones too. There will invariably be required RBAC to decide who gets to author clustered build strategies. In k8s land, admins of some level define such RBAC. The first question we'll get is "who should author build strategies". And at least after searching on author and writer in the docs dir, I see no reference to such a role yet there. I'm fine with introducing such titles for roles, but there needs to be a clear definition of it in existing docs before I think we start referencing them in EPs. If that exists somewhere and I'm just overlooking it, please help me with a pointer. Otherwise, I don't think this EP is the place for establishing the "build strategy author" role. I do think it is fair that after proper definition of a build strategy author, build/build run author, etc., going back and updating this EP or others which have not been implemented yet, that reference some semblance of those roles, should be updated. |
There are some inline response I still need to make, but in general what you've raised here @adambkaplan makes me think I do minimally need some more work here if we are to keep this at all. Now, some of that "what we keep" will probably depend on where we land with the roles discussion and what we in fact focus on there. But even assuming we keep some for of What I was contending with when devising this:
And contending with the existing fact that buildstep wrappers a container which allows specification of env vars The But in discussing his team's scenarios, some notion of controlling which env vars could be overridden from the strategy / build / buildrun perspective is desired. Positioning the existing BuildStep / container stuff as providing 3) is the other piece wrt this with current form of this EP. Lastly, I did mention "staging" wrt to this EP early on in discussions with @SaschaSchwarze0 . Minimally, that could mean just moving elements of what is here to the alternative section. Perhaps likely. But besides just moving elements there, I think we could get a little more detailed and simply list all the possible features in stages, but call for initial implementation of only an initial stage. Again, if I were predict, the roles discussion will have some bearing on whether this flavor of change makes sense at all. |
You remember this correctly. Just for reference, here is the text file I had shown in our meeting, with the scenarios I had thought about (and some sample ideas on how this could look): envvars.txt |
thanks for the confirmatino @SaschaSchwarze0 I just posted a bit of a dive into how to precisely achieve that (with a preference for a slightly different variant on your sample yaml) at #726 (comment) |
|
OK @adambkaplan @SaschaSchwarze0 @sbose78 @otaviof I've pushed a new commit with updates following the verbal agreement Adam and I reached wrt the various comment threads here. Still a separate commit vs. squashed to help with identifying deltas since you all last looked at this. PTAL as your cycles of availability allow. thanks |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SaschaSchwarze0 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 |
There was a problem hiding this comment.
I greatly appreciate the thought and effort that has gone into the proposal so far. Reconciling the desire for end user flexibility and administrative/author control is no easy task, and the current API design earnestly tries to achieve that.
That said, at this point I have a fundamental objection to the complexity of this proposal - particularly the combination of the allow/block lists with the EnvVarOverwriteable field. In my critique I have hinted at an alternative approach; I think it is only fair that I work through this with my own proposal, and then we as a community can weigh the merits of each.
| name: something-that-uses-docker | ||
| spec: | ||
| # nobody is allowed to change DOCKER_TLS_VERIFY | ||
| BlockedEnvironmentVariables: |
There was a problem hiding this comment.
nit - lower camelCase for YAML
| BlockedEnvironmentVariables: | |
| blockedEnvironmentVariables: |
| } | ||
| ``` | ||
|
|
||
| This field will be added to the `Build` and `BuildRun` objects. |
There was a problem hiding this comment.
Please provide API context. Personally, I think having envVars or env represent the array of type EnvironmentVariable makes sense:
type BuildSpec struct {
// existing fields
...
// Env contains environment variables provided by the end user, and set on all steps in the executing build
Env []EnvironmentVariable `json:"env,omitempty"`
}| become a preferred method when building images in k8s native environments. | ||
|
|
||
| Next, the question of whether to use the k8s `EnvVar` directly exists. This proposal suggests creating a new | ||
| Shipwright type that inlines the k8s type, in case we want to extend and add new features later that require |
There was a problem hiding this comment.
Counter point - we don't need a separate type right now, and a future type will need to extend corev1.EnvVar to be backwards-compatible. Without a concrete, motivating feature why not keep things simple?
| To facilitate both mindsets, we'll add two string arrays to the `BuildStrategySpec` | ||
| - `AllowedEnvironmentVariables` | ||
| - `BlockedEnvironmentVariables` |
There was a problem hiding this comment.
With the EnvVarOverwriteable option, we are providing competing paths for the same objective. If the goal is "prevent a user from overwriting an environment variable", there are two approaches:
- Add the env var key to the BlockedEnvironmentVariables slice
- Add the env var key to the EnvVarOverwriteable slice
Simple things have one clear, obvious path to achieving the objective. If the goal of Shipwright is to strive for simplicity, then we should adhere to that.
| ### User Stories [optional] | ||
|
|
||
| #### Story 1 | ||
|
|
||
| As an image developer using Shipwright, I need to be able to control environment variables passed to the underlying | ||
| tools used for image management, source code management, or data transfer, when I build image using Shipwright, assuming | ||
| agreement with the cluster administrator has been obtained on which environment variables I can control. | ||
|
|
||
| #### Story 2 | ||
|
|
||
| As someone in charge of maintaining `BuildStrategies` and most likely the use of Shipwright across the cluster, | ||
| I need to make sure that the users of my cluster do not use any unsafe environment variable enabled | ||
| features that exist with the underlying tools used for image management, source code management, or data transfer, when | ||
| Shipwright runs in the cluster. | ||
|
|
||
| #### Story 3 | ||
|
|
||
| As someone in charge of maintaining `BuildStrategies` and most likely the use of Shipwright across the cluster, | ||
| I need to make sure that only approved values for certain environment variables, which in turn control | ||
| features that exist with the underlying tools used for image management, source code management, or data transfer, are used when | ||
| Shipwright runs in the cluster. |
There was a problem hiding this comment.
A general nit on the structure, and one that I want to try and enforce with the SHIPs repo.
User stories should go first in the proposal section. These provide the overall context for the proposal - who needs a certain feature, and why. Reading these first sets the table for the implementation that follows.
| Second, if Shipwright users end up preferring the use of third party policy engines for restricting, the parts of this | ||
| API that attempt to provide controls around environment variable specification will appear redundant. More detail on | ||
| these third party policy engines are in the [Alternatives](#alternatives) section below. |
There was a problem hiding this comment.
My take based the latest discussion/sessions at KubeCon is that 3rd party policy engines are going to become a predominant means that production Kubernetes clusters are secured. Tabitha Sable and Ellen Korbes more or less implied this with their Security 101 talk [1].
| API that attempt to provide controls around environment variable specification will appear redundant. More detail on | ||
| these third party policy engines are in the [Alternatives](#alternatives) section below. | ||
|
|
||
| ## Alternatives |
There was a problem hiding this comment.
Consider the following alternative approach that doesn't use the allow/block lists:
- Env vars in build strategy steps are extended to have the
overridePolicy: <Deny|Allow>field (if empty, interpret asAllow).
spec:
steps:
- name: build
...
env:
- key: FOO
value: bar
overridePolicy: Deny- BuildStrategy's openAPI def is updated so when a user does
$ kubectl describe buildstrategy.shipwright.io buildpacks, the names of the build steps are clear. - In the
BuildandBuildRunobjects, we provide astepOverridesfield where the env vars can be overrode for each step
spec:
stepOverrides:
build:
env:
- key: FOO
value: biz # this would be lead to a failed BuildRun
- key: FIZZ
value: buzz # this would be allowed, as FIZZ was not specified in the build strategyWhat are the drawbacks here? Are there use cases that we can't satisfy with this approach?
If desired, I can author a competing proposal with this design.
understood @adambkaplan I'm going to defer processing any of your suggestions from today until your alternative is posted and the community makes a high level decision. |
One minor reiteration wrt having both allow and block ... having both was not what was originally proposed, but stemmed from input from different reviewers / view points. |
|
I think we can now close this in favor of shipwright-io/community#10 |
Changes
And enhancement proposal for #224 for adding environment variable API.
I've initially called out some dependency on #697
Per comments in the EP, I was a minimalist when it came to initially listing reviewers and approvers. My thought is I'll update those based on who reviews this PR and discussion in the community meeting / slack on who should approve.
Also, I've set release not to none. We can have a release not when this is implemented.
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes