-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add SSH agent postStart event only if SSH key has a passphrase & experimental features enabled #1341
Conversation
Skipping CI for Draft Pull Request. |
err = ssh.AddSshAgentPostStartEvent(&workspace.Spec.Template) | ||
if err != nil { | ||
return r.failWorkspace(workspace, "Failed to add ssh-agent post start event", metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus), nil | ||
if needsSSHAgentPostStartEvent, err := ssh.NeedsSSHPostStartEvent(clusterAPI, workspace.Namespace); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the standup's discussion I like the idea of enabling it based on
apiVersion: controller.devfile.io/v1alpha1
config:
enableExperimentalFeatures: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll make the appropriate changes in an additional commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the additional commit I created for guarding the SSH agent initialization postStart event injection with enableExperimentalFeatures
got merged into my original commit while I was cleaning up the git log. However, this change is present.
8864d06
to
648ce69
Compare
@vinokurig: changing LGTM is restricted to collaborators 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. |
return r.failWorkspace(workspace, "Failed to add ssh-agent post start event", metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus), nil | ||
if workspace.Config.EnableExperimentalFeatures != nil && *workspace.Config.EnableExperimentalFeatures { | ||
if needsSSHAgentPostStartEvent, err := ssh.NeedsSSHPostStartEvent(clusterAPI, workspace.Namespace); err != nil { | ||
// TODO: Should we fail the workspace? Or log the error and continue on? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards just logging the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Given the fact that this PR aims to prevent breaking any workspaces that would otherwise work fine prior to the SSH agent postStart event, we should probably just log an error rather than failing the workspace.
It would be nice to add a documentation note e.g.
|
+1 will add an extra commit for this |
@dkwon17 thank you for the review :) will squash my fixup commits tomorrow & have this merged |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AObuchow, dkwon17, ibuziuk, 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 |
Only add the SSH agent initialization postStart event if an SSH key with a passphrase is being used & experimental features are enabled. We don't use the config package's ExperimentalFeaturesEnabled function so that the SSH agent initialization postStart event injection can be enabled from an external DWOC, or the global DWOC if no external DWOC is used. fix devfile#1340 Signed-off-by: Andrew Obuchowicz <[email protected]>
Signed-off-by: Andrew Obuchowicz <[email protected]>
Signed-off-by: Andrew Obuchowicz <[email protected]>
48c3464
to
bf43b39
Compare
New changes are detected. LGTM label has been removed. |
What does this PR do?
The SSH agent initialization postStart event is now only injected under the following conditions:
git-ssh-key
exists in the workspace's namespacegit-ssh-key
contains a data key calledpassphrase
config.enableExperimentalFeatures: true
is set in an external DWOC used by the workspace, or in the global DWOC.The intention of this PR is to ensure the SSH agent initialization postStart event is only injected if user's opt-in by configuring the DWOC accordingly, and provide a passphrase in their SSH key.
However, this is only a temporary workaround for DWO 0.31.2. After this PR, we should reconsider how this postStart event should be injected. I've mentioned 2 potential ideas in the long-term solution section of #1340
What issues does this PR fix or reference?
#1340
Is it tested? How?
First deploy DWO with the changes from this PR.
There are 4 scenarios to test:
I recommend testing all 4 scenarios in order.
Scenario 1: no SSH secret configured; experimental features disabled
oc get pod <workspace-pod-name> -n $NAMESPACE -o json | jq '.spec.containers[0].lifecycle.postStart'
should benull
.oc delete dw plain-devworkspace -n $NAMESPACE
Scenario 2: SSH secret configured with no passphrase; experimental features disabled
$PASSPHRASE
environment variable when creating the SSH secret.oc delete dw plain-devworkspace -n $NAMESPACE
Scenario 3: SSH secret configured with a passphrase; experimental features disabled
oc delete secret git-ssh-key -n $NAMESPACE
$PASSPHRASE
environment variable when creating the SSH secret.oc delete dw plain-devworkspace -n $NAMESPACE
Scenario 4: SSH secret configured with a passphrase; experimental features enabled
oc edit dwoc -n $NAMESPACE
kind: DevWorkspaceOperatorConfig apiVersion: controller.devfile.io/v1alpha1 config: + enableExperimentalFeatures: true routing: clusterHostSuffix: 192.168.49.2.nip.io defaultRoutingClass: basic workspace: imagePullPolicy: Always
oc get pod <workspace-pod-name> -n $NAMESPACE -o json | jq '.spec.containers[0].lifecycle.postStart'
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-path
to trigger)v8-devworkspace-operator-e2e
: DevWorkspace e2e testv8-che-happy-path
: Happy path for verification integration with Che