Skip to content
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

Added namespace configurator for existing user SSH keys #192

Merged
merged 5 commits into from
Dec 13, 2021

Conversation

mshaposhnik
Copy link
Contributor

@mshaposhnik mshaposhnik commented Dec 8, 2021

Signed-off-by: Max Shaposhnik mshaposh@redhat.com

What does this PR do?

Added namespace configurator for mounting existing user SSH keys into namespace during provision

Screenshot/screencast of this PR

Generated secret data will looks like:

image

What issues does this PR fix or reference?


eclipse-che/che#20832

How to test this PR?

  • Deploy Che with devworkspaces enabled;
  • Add SSH key via Swagger API (since SSH plugin in Theia is disabled on DVW mode)
  • Remove user namespace and refresh dashboard to force namespace provision again
  • Check secret is created

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Dec 8, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Test product:

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a DWO perspective.

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Dec 9, 2021

❌ E2E Happy path tests failed ❗

See Details

Test product:

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

@skabashnyuk
Copy link
Contributor

skabashnyuk commented Dec 9, 2021

@amisevsk @JPinkney can you confirm that the path of ssh key should be /.ssh/ as described here devfile/devworkspace-operator#613 (comment) we used to have it /etc/ssh see

throws InfrastructureException {

var client = clientFactory.create();
List<SshPairImpl> vcsSshPairs = getVcsSshPairs(namespaceResolutionContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amisevsk wondering if we need to process the internal ssh keys for async-storage. I'm not sure how the DevWorkspace handles the key management for it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
@mshaposhnik
Copy link
Contributor Author

Yes, correct path should be /etc/ssh/ssh_config, pushed the fix.

@amisevsk
Copy link
Contributor

amisevsk commented Dec 9, 2021

/etc/ssh should work, however note that currently async storage mounts its own ssh key to /etc/ssh/private for the async storage sidecar, so that's a sticking point we may need to work around in some way.

@mshaposhnik
Copy link
Contributor Author

Maybe separate secret with different mount path-s for them ? (but ssh_config file should remain the same IMHO....)

@amisevsk
Copy link
Contributor

amisevsk commented Dec 9, 2021

Maybe separate secret with different mount path-s for them ? (but ssh_config file should remain the same IMHO....)

I think DWO needs to isolate the async-storage sidecar from automount, etc. It only has a single responsibility (communicate with async server) and so should not be impacted by anything else. I've created devfile/devworkspace-operator#707 to track this issue.

@skabashnyuk
Copy link
Contributor

[crw-ci-test --rebuild]

@skabashnyuk
Copy link
Contributor

@amisevsk @ibuziuk does it make sense to handle async-storage keys as a separate task? Because it is not really clear what to do now.

@mshaposhnik
Copy link
Contributor Author

@skabashnyuk Yes that's why devfile/devworkspace-operator#707 created AFAIU

@che-bot
Copy link
Contributor

che-bot commented Dec 10, 2021

✅ E2E Happy path tests succeed 🎉

See Details

Test product:

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

@amisevsk
Copy link
Contributor

To clarify, DWO currently creates the keypair for the async storage usecase, so no intervention from Che Server is needed to make that happen (Che should not have to do anything for the async storage case). The issue is that currently the keypair provisioned by DWO will collide with an automount to that directory, which is what's described in devfile/devworkspace-operator#707.

@mshaposhnik mshaposhnik merged commit ff26653 into eclipse-che:main Dec 13, 2021
@mshaposhnik mshaposhnik deleted the ssh-configurator branch December 13, 2021 07:00
@che-bot che-bot added this to the 7.41 milestone Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants