Apply ssh askpass flow for the workspace container #1307
Conversation
|
Hi @vinokurig. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
/test v8-devworkspace-operator-e2e, v8-che-happy-path |
|
@vinokurig: Cannot trigger testing until a trusted user reviews the PR and leaves an 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-sigs/prow repository. |
There was a problem hiding this comment.
Looks awesome to me so far @vinokurig, great work 😁
I left some small suggestions as to how we inject the SSH ask pass script into the workspace deployment, as well as removing some functionality from #1295 that would be rendered redundant from this PR.
pkg/provision/automount/common.go
Outdated
| @@ -109,7 +109,12 @@ func getAutomountResources(api sync.ClusterAPI, namespace string) (*Resources, e | |||
| return nil, err | |||
| } | |||
|
|
|||
| return mergeAutomountResources(gitCMAutoMountResources, mergedResources, pvcAutoMountResources), nil | |||
| sshCMAutoMountResources, err := ProvisionSshAskPassScript(api, namespace) | |||
There was a problem hiding this comment.
I'm a bit hesitant for us to add the SSH ask pass script as part of the automount resources provisioning flow. From my understanding, automount resources are supposed to be optional, user-provided resources. In contrast, the SSH ask pass script is a static file that is automatically injected into the workspace without user intervention.
Instead, I'd suggest making a new package ssh in /pkg/provision/ssh/ to store all the ssh askpass related code. There, you can have a function like ssh.InjectSSHAskPassScript(podAdditions *v1alpha1.PodAdditions, api sync.ClusterAPI, namespace string) that:
- Creates the SSH ask pass configmap script on the cluster (you already implemented this here)
- Modifies the workspaces podAdditions to add the additional SSH ask pass configmap volume and volumeMounts
Basically, you'd inject the SSH ask pass configmap volume and volumeMount into the podAdditions, instead of the automount resources. We do a similar approach for ServiceAccountTokens.
Your new ssh.InjectSSHAskPassScript(podAdditions function could get called in the devworkspace-controller's reconcile function, just before we provision the automount resources. This way, we'll check whether the SSH ask pass volume and volumeMount conflict with any automount resources when checkAutomountVolumesForCollisions() is called.
| @@ -58,7 +58,7 @@ func TestProvisionAutomountResourcesIntoPersistentHomeEnabled(t *testing.T) { | |||
|
|
|||
| err := ProvisionAutoMountResourcesInto(podAdditions, testAPI, testNamespace, true) | |||
|
|
|||
| if !assert.NoError(t, err, "Unexpected error") { | |||
| if assert.ErrorContains(t, err, "Created object") || !assert.NoError(t, err, "Unexpected error") { | |||
There was a problem hiding this comment.
I assume this was added because ProvisionAutoMountResourcesInto() now creates the ssh ask pass configmap on the cluster, and we don't want to fail the test for this expected "error" right?
There was a problem hiding this comment.
Reworked to intercept the error when initialising the configmap
pkg/provision/automount/ssh.go
Outdated
| return nil, dwerrors.WrapSyncError(err) | ||
| } | ||
| resources := flattenAutomountResources([]Resources{ | ||
| getAutomountConfigmap(SshAskPassMountPath, constants.DevWorkspaceMountAsSubpath, pointer.Int32(0755), sshAskPassConfigMap), |
There was a problem hiding this comment.
As mentioned in my earlier comment, I don't know if we should be creating an automount configmap in order to have DWO provision the volume and volumeMount in the workspace for us, rather than just creating and adding the volume and volumeMount to the workspace's podAdditions.
On one hand, it's nice to reuse existing code and functionality: here we're creating an automount configmap as if we were a user.
On the other hand, it seems to blur the responsibilities of the automount package, as it is no longer only responsible for automatically mounting user-provided data (since we are now mounting static data that is not provided by users).
@dkwon17 do you have any opinions here?
There was a problem hiding this comment.
reworked to initialise the resources without using the automount package
pkg/provision/automount/ssh.go
Outdated
| "k8s.io/utils/pointer" | ||
| ) | ||
|
|
||
| const SshAskPassMountPath = "/.ssh-askpass/" |
There was a problem hiding this comment.
In the project clone container, we mount the SSH askpass script to /usr/local/bin/ssh-askpass.sh. Is there a reason for now mounting it to /.ssh-askpass/? Was it to have it as a hidden file in the filesystem?
There was a problem hiding this comment.
It does not matter where to keep the script file but I changed it to be inline with
project-clone/Dockerfile
Outdated
| @@ -49,7 +49,7 @@ ENV USER_UID=1001 \ | |||
| SSH_ASKPASS=/usr/local/bin/ssh-askpass.sh | |||
|
|
|||
| COPY build/bin /usr/local/bin | |||
| COPY project-clone/ssh-askpass.sh /usr/local/bin | |||
| COPY pkg/provision/automount/ssh-askpass.sh /usr/local/bin | |||
There was a problem hiding this comment.
Regardless of whether we use the current approach of using DWO's automount functionality, or directly add to the podAdditions, the SSH ask pass script should actually be injected into the project clone init container already when we create the workspace deployment.
This means that we shouldn't have to manually COPY the SSH ask pass script to the project clone image from the Dockerfile anymore (which is nice 🥳) -- so we should be able to remove this line altogether.
There was a problem hiding this comment.
Since the project clone container also gets environment variables from the commonEnvironmentVariables() function, we no longer have to setup the SSH askpass related environment variables in the project clone container's Dockerfile.
There was a problem hiding this comment.
Awesome thank you! Just leaving a comment here as a reminder to myself: it's worth re-testing the functionality of #1291 since we're modifying how it's implemented now.
pkg/library/env/workspaceenv.go
Outdated
| Value: ":0", | ||
| }, { | ||
| Name: constants.SSHAskPass, | ||
| Value: automount.SshAskPassMountPath + automount.SshAskPassScriptFileName, |
There was a problem hiding this comment.
In the rest of DWO's codebase, we tend to use fmt.Sprintf() instead of using + to concatenate strings. In my opinion, it makes it more clear that we are concatenating two strings and not summing two numbers.
Admittedly, fmt.Sprintf("%s%s", automount.SshAskPassMountPath, automount.SshAskPassScriptFileName) does look a bit weird however, since we're just concatenating two strings with no extra formatting.
|
@vinokurig Also, please run |
|
@AObuchow Thank you for the review, I've added another commit to add a post start event to initialise |
vinokurig
force-pushed
the
dwo-1295
branch
6 times, most recently
from
6a6cd5e
to
e641d7b
Compare
There was a problem hiding this comment.
Thanks for addressing my earlier comments @vinokurig, this is looking great :)
I left some minor comments but I think this is about ready for some testing on my end.
pkg/library/env/workspaceenv.go
Outdated
|
|
||
| return envvars | ||
| } | ||
|
|
||
| func GetSshAskPassEnvVars() []corev1.EnvVar { |
There was a problem hiding this comment.
Thank you for putting this into a function :)
We probably don't need to have this exported (i.e. the function name should start with a lower case letter). since it's only being called in the same file.
It's actually odd that GetProxyEnvVars() is also exported but not used elsewhere.
Would you mind making both the GetSshAskPassEnvVars() and the GetProxyEnvVars() functions not exported please?
There was a problem hiding this comment.
Awesome thank you! Just leaving a comment here as a reminder to myself: it's worth re-testing the functionality of #1291 since we're modifying how it's implemented now.
pkg/library/ssh/event.go
Outdated
| spec.Events = &v1alpha2.Events{} | ||
| } | ||
|
|
||
| var commandLine = `SSH_ENV_PATH=/home/user/ssh-environment \ |
There was a problem hiding this comment.
A few comments:
- The
commandLinevariable could probably be a constant to make the rest of the code a little easier to read (like we did here) - It might be better to use
$HOMEinstead of hard-coding/home/user/since the users workspace might not be using the UDI. I'm not sure how common thessh-agentbinary is, but hopefully your implementation here could also work for other workspace images. - This is very subjective: I find using
sourceis more readable than.. This is relevant IMO because we are modifying the users.bashrc.
The only non-straightforward thing here is the ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH. My understanding is that there's some echo commands as part of ssh-agent's output that we don't want to be executed everytime the .bashrc is sourced?
There was a problem hiding this comment.
- The commandLine variable could probably be a constant to make the rest of the code a little easier to read (like we did here)
- It might be better to use $HOME instead of hard-coding /home/user/ since the users workspace might not be using the UDI. I'm not sure how common the ssh-agent binary is, but hopefully your implementation here could also work for other workspace images.
- This is very subjective: I find using source is more readable than .. This is relevant IMO because we are modifying the users .bashrc.
done
The only non-straightforward thing here is the ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH. My understanding is that there's some echo commands as part of ssh-agent's output that we don't want to be executed everytime the .bashrc is sourced?
Right, we comment the echo Agent pid ****; line of the ssh-agent command output, so we have a clear terminal start. As an alternative we can add >/dev/null to the source ${SSH_ENV_PATH} bachrc command. WDYT?
There was a problem hiding this comment.
I actually like your current solution of not echoing the Agent pid **** over doing a >/dev/null since this would probably let any errors related to sourcing the ssh-agent environment variables get logged in the user's terminal and let them know something is not working correctly.
There was a problem hiding this comment.
Also @vinokurig it seems we're still referencing /home/user/ in two lines of the postStart event: here and here. Could you please change these to $HOME or ~?
pkg/library/ssh/event.go
Outdated
| spec.Events = &v1alpha2.Events{} | ||
| } | ||
|
|
||
| var commandLine = `SSH_ENV_PATH=/home/user/ssh-environment \ |
There was a problem hiding this comment.
I actually like your current solution of not echoing the Agent pid **** over doing a >/dev/null since this would probably let any errors related to sourcing the ssh-agent environment variables get logged in the user's terminal and let them know something is not working correctly.
pkg/library/ssh/event.go
Outdated
| spec.Events = &v1alpha2.Events{} | ||
| } | ||
|
|
||
| var commandLine = `SSH_ENV_PATH=/home/user/ssh-environment \ |
There was a problem hiding this comment.
Also @vinokurig it seems we're still referencing /home/user/ in two lines of the postStart event: here and here. Could you please change these to $HOME or ~?
vinokurig
force-pushed
the
dwo-1295
branch
2 times, most recently
from
30d5490
to
405ec53
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1307 +/- ##
==========================================
+ Coverage 52.52% 52.63% +0.10%
==========================================
Files 84 85 +1
Lines 7642 7976 +334
==========================================
+ Hits 4014 4198 +184
- Misses 3335 3477 +142
- Partials 293 301 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I re-built the project-clone image with the changes from this PR and verified this current PR resolves #1295 without breaking the functionality of #1291.
I still need to do one last pass-through of this PR, and @dkwon17 should take a look as well, but IMO this PR should be good to merge very soon (ideally, before the end of this week).
Great work @vinokurig 😁 And sorry for taking so long with my review.
|
/ok-to-test |
|
LGTMl, I just have one comment |
| "github.com/devfile/devworkspace-operator/pkg/library/lifecycle" | ||
| ) | ||
|
|
||
| const commandLine = `SSH_ENV_PATH=$HOME/ssh-environment \ |
There was a problem hiding this comment.
If we keep the current approach of modifying the .bashrc, it's probably worth making the ssh-environment file hidden with a . i.e. SSH_ENV_PATH=$HOME/.ssh-environment
pkg/library/ssh/event.go
Outdated
| then ssh-agent | sed 's/^echo/#echo/' > $SSH_ENV_PATH \ | ||
| && chmod 600 $SSH_ENV_PATH && source $SSH_ENV_PATH \ | ||
| && ssh-add /etc/ssh/dwo_ssh_key < /etc/ssh/passphrase \ | ||
| && if [ -f $HOME/.bashrc ]; then echo "source ${SSH_ENV_PATH}" >> $HOME/.bashrc; fi; fi` |
There was a problem hiding this comment.
Modifying the .bashrc works, but if a user provides their own custom .bashrc through an automount configmap, this feature will actually break. Files that are mounted via the automount configmap mechanism cannot be written to.
Instead, we could place the SSH_ENV_PATH in /etc/profile.d/, which contains scripts that should be run when sourcing the .bashrc, e.g. SSH_ENV_PATH=/etc/profile.d/.ssh-environment. This way, any .bashrc will load the ssh-agent script.
$ cat ~/.bashrc
# .bashrc
# Source global definitions
if [ -f /etc/bashrc ]; then
. /etc/bashrc
fi
(...)
$ cat /etc/bashrc
(...)
SHELL=/bin/bash
# Only display echos from profile.d scripts if we are no login shell
# and interactive - otherwise just process them to set envvars
for i in /etc/profile.d/*.sh; do
if [ -r "$i" ]; then
if [ "$PS1" ]; then
. "$i" # Here is where all the scripts in /etc/profile.d/ get sourced
else
. "$i" >/dev/null
fi
fi
doneA caveat is that the /etc/profile.d/ scripts only get run for interactive shells, see here for more explanation. But for #1295 this is fine.
I do understand that this is a rather big change to request from your PR @vinokurig, so I'm perfectly fine with keeping the current approach that modifies the .bashrc and making a new issue/PR in the future to implement my suggestions. I haven't actually tested my suggestion, so it might not actually work (though I did something similar for the UDI a while back)
There was a problem hiding this comment.
I do understand that this is a rather big change to request from your PR @vinokurig, so I'm perfectly fine with keeping the current approach that modifies the .bashrc and making a new issue/PR in the future to implement my suggestions.
I think we should take the current approach for now since there isn't a straight forward alternative since the /etc/profile.d folder cannot be written by regular users by default in the UDI. I suggest we keep the functionality how it is for now, and document that if .bashrc is being mounted using a configmap, something like [ -f /home/user/ssh-environment ] && source /home/user/ssh-environment should be added to it.
I can make a new PR documenting this after this PR is merged.
Any thoughts?
There was a problem hiding this comment.
Good catch :) I agree that it's probably worth documenting this somewhere in the near future (maybe in the Che Docs as well as DWO additional docs)?
| package workspace | ||
|
|
||
| import ( | ||
| _ "embed" |
There was a problem hiding this comment.
Is there a reason why the blank identifier _ is being used? Couldn't we just have
import (
"embed"
...
)I've never used the embed directive, but examples of it don't seem to be using the blank identifier.
There was a problem hiding this comment.
Importing the embed explicitly causes an error: pkg/provision/workspace/ssh.go:19:2: "embed" imported and not used
pkg/provision/workspace/ssh.go
Outdated
| //go:embed ssh-askpass.sh | ||
| var data string | ||
|
|
||
| func ProvisionSshSshAskPass(api sync.ClusterAPI, namespace string, podAdditions *v1alpha1.PodAdditions) error { |
There was a problem hiding this comment.
I believe there's a typo in this function name: ProvisionSshSshAskPass -> ProvisionSshAskPass
There was a problem hiding this comment.
LGTM, awesome work @vinokurig 😁
Please squash all your commits into a single commit (I think you could squash all your commits into your first commit)
You could also add "Fixes #1295" to the commit description.
Signed-off-by: ivinokur <[email protected]> Fixes devfile#1295
thanks, done |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AObuchow, dkwon17, vinokurig 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 |
What does this PR do?
DISPLAYandSSH_ASKPASSenvironment variables to the workspace container for the ssh askpass flow.ssh-askpass.shfile in the project clone Dockerfile to the one in theprovision/automountpackageWhat issues does this PR fix or reference?
fixes #1295
Is it tested? How?
DISPLAYandSSH_ASKPASSenvironment variables are set/.ssh-askpass/ssh-askpass.shfile is present and have the right content.PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che