Creds-init writes to fixed location when HOME override is disabled #2180
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/L
ghost
mentioned this pull request
|
/hold Want to look at this again with fresh eyes on Monday before committing. |
tekton-robot
added
the
do-not-merge/hold
ghost
added
the
needs-cherry-pick
docs/auth.md
Outdated
| @@ -24,6 +24,12 @@ To solve this, before any `PipelineResources` are retrieved, all `pods` execute | |||
| a credential initialization process that accesses each of its secrets and | |||
| aggregates them into their respective files in `$HOME`. | |||
|
|
|||
| Please note: If the `disable-home-env-overwrite` feature flag has been set to | |||
There was a problem hiding this comment.
So, in the case of disable-home-env-overwrite (and by default in the future), do we decide that /tekton/home is where we write anything that "would be in $HOME" ? (like .gitconfig, …) I am +:100: on this as we will document it, and we are sure it's always written at the same place.
There was a problem hiding this comment.
Interesting idea! I think this might make sense. I was considering using a more explicit name like /tekton/creds since we'd originally planned for /tekton/home to be removed in future. But if we see this as reusable then I think it makes sense to leave it as-is.
I'm going to write up a one-page design doc and will present it to the WG on wednesday just to quickly get community feedback on that plan.
There was a problem hiding this comment.
We should probably also expose this as a variable so that users don't have to hard-code it.
There was a problem hiding this comment.
+:100: on both your comments :wink: I really like /tekton/creds and having a variable for it is 😻
There was a problem hiding this comment.
👍 awesome, i've implemented both of these now.
tekton-robot
added
the
approved
|
I've written up a very short design doc covering the solution in this PR and a slight alternative. Doc here: https://docs.google.com/document/d/1SVuDt-SXPHymz41dveSXFSPrx5Z-Wzb9eHliJAyYg4o |
|
This didn't end up being true because the entrypoint now copies the creds into $HOME. So the behaviour is the same regardless of the state of the flag: credentials from a Secret will end up in the PipelineResource container's $HOME dir. |
|
I've added variable replacement from I've updated the |
|
This change should be pretty much invisible to users now, no extra copying in their Steps should be necessary. The entrypoint binary now copies the creds into $HOME instead of creds-init (when the disable-home-env-overwrite feature flag is true) so the net result is the same regardless of the feature flag: creds end up in container HOME dirs. |
|
/hold cancel I think this is probably ready for review now. |
tekton-robot
removed
the
do-not-merge/hold
| @@ -125,6 +125,15 @@ func ApplyTaskResults(spec *v1alpha1.TaskSpec) *v1alpha1.TaskSpec { | |||
| return ApplyReplacements(spec, stringReplacements, map[string][]string{}) | |||
| } | |||
|
|
|||
| // ApplyCredentialsPath applies a substitution of the key $(credentials.path) with the path that the creds-init | |||
| // helper will write its credentials to. | |||
| func ApplyCredentialsPath(spec *v1alpha1.TaskSpec, path string) *v1alpha1.TaskSpec { | |||
There was a problem hiding this comment.
Is this only usable as internal ? (because I think it's gonna be invalid if using directly in a spec)
There was a problem hiding this comment.
Could you tell me more about it being invalid if used directly in spec? It's intended for users to be able to work with as well.
There was a problem hiding this comment.
I think this might fail at validation saying credential.path is an invalid variable (not 100% sure but my feeling is that not having anything about it in the validation code means it will fail — maybe I am wrong though, a tests showing this would be nice 😝 )
There was a problem hiding this comment.
Aaaaah. Lemme check. Great catch.
There was a problem hiding this comment.
Initial manual test proved positive - I was able to use $(credential.path) in a TaskRun w/ embedded TaskSpec. I didn't hit a validation error and the Task ran successfully.
I'm going to add a test for this right now to make sure it doesn't fail validation in future.
There was a problem hiding this comment.
Updated with tests for task and taskrun validations in both v1beta1 and v1alpha1 packages.
tekton-robot
added
size/XL
When the disable-home-env-overwrite flag is set to "true" each Step in a Task can conceivably have its own HOME directory. The concept of "HOME" is further muddied in systems that randomize the UID of containers. So now creds-init will write to a shared volumeMount, /tekton/creds, when the disable-home-env-overwrite flag is "true". When the flag is "false" creds-init will behave exactly the same as before, writing the credentials to /tekton/home, and no extra volume mount will be needed. This change should be mostly transparent to users: the entrypoint binary in each Step will now try and copy credentials out of /tekton/creds into $HOME/. The net result is the same as before the flag was introduced, it's just that entrypoint does the final copy into $HOME instead of creds-init. To support users who were in some way depending on the location of credentials, the path to where creds-init writes is now exposed for Tasks via the $(credentials.path) variable. This will be replaced with the directory that creds-init writes to: either "/tekton/home" or "/tekton/creds" depending on the state of the disable-home-env-overwrite flag.
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
|
/test pull-tekton-pipeline-integration-tests |
1 similar comment
|
/test pull-tekton-pipeline-integration-tests |
|
/test pull-tekton-pipeline-build-tests |
ghost
removed
the
needs-cherry-pick
Changes
Fixes #2165
Relates to #2013
When the disable-home-env-overwrite flag is set to "true" each Step in a Task can conceivably have its own HOME directory. The concept of "HOME" is further muddied in systems that randomize the UID of containers.
When the disanle-home-env-overwite flag is set to "true", creds-init will write its credentials to
/tekton/credsinstead of/tekton/home. Then the entrypoint binary will copy the credentials from/tekton/credsinto$HOMEbefore the Task's Steps are allowed to run. This change should be completely transparent to the user.For those users who were for some very specific reason actually relying on the credentials being in
/tekton/homewe'll now expose a new variable substitution:$(credentials.path)will get replaced with the directory that creds-init writes to - either/tekton/homeor/tekton/credsdepending on the state of the disable-home-env-overwrite flag.This is a stop-gap measure as we work towards beta. It seems that we will probably need to revisit our recommendations around auth and the contract we're making with creds-init.
$(credentials.path)Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes