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

Add ssh access to VMs #352

Merged
merged 1 commit into from
Dec 12, 2020

Conversation

claudious96
Copy link
Contributor

@claudious96 claudious96 commented Nov 24, 2020

Description

This PR presents a possible implementation of an SSH bastion using a sidecar approach. Each pod is composed of an ssh-bastion running sshd and a sidecar operator that whatches the Tenant resource updating the authorized_keys file at each tenant related event (creation/deletion/update of a Tenant resource).

Currently the operator gets a List of Tenants in the default namespace, this is good for testing but probably should be managed differently.
Assuming that a single deployment would manage all incoming ssh incoming traffic for the whole crownlabs cluster, since the Tenant resource could belong to one or more workspaces, a possible approach could be:

  • establish (somehow) a list of workspaces that are going to use this feature i.e. Cloud Computing course
  • perform a List operation on the Tenant CRD belonging to those workspaces
  • update the authorized_keys file

Otherwise if the plan is to deploy it per workspace, the operator could just perform the List on the related workspace and the operator should be modified in order to be aware of the workspace (namespace might be useful).

Notes:

  • bastion's host keys are generated in the Dockerfile but they should probably be set using a secret
  • sshd configuration is created using a RUN command, could be created as file to be copied at build time
  • give the possibility to set parameters such as port number, namespace name, etc (just like it's done for the lab operator)
  • default message in /etc/motd/ in case a user connects to the bastion without the -J jump option
  • Ginkgo tests

@claudious96 claudious96 force-pushed the vm-ssh-access branch 3 times, most recently from ad0c989 to 9a3e059 Compare November 28, 2020 09:59
@claudious96 claudious96 force-pushed the vm-ssh-access branch 3 times, most recently from 7d8d5e3 to 5e6cfc3 Compare November 29, 2020 18:02
@claudious96 claudious96 changed the title Vm ssh access Add ssh access to VMs Nov 29, 2020
@frisso
Copy link
Member

frisso commented Nov 30, 2020

Quoting from the PR description:

Resource updating the authorized_keys file at each tenant creation/deletion.

I would suggest to update the authorized_keys also when the tenant is updated. It would be useful to allow users to update the list of authorized keys in their profile.

Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks @claudious96. At a first sight, this PR seems very good!

A couple of comments with regard to your notes (and a few very minor inline):

  1. Tenant resources are not namespaced
  2. Totally agree about using the keys from a secret and not statically generate them in the docker
  3. From my point of view, both generating the configuration using RUN and using an external file is fine. In the first case, you could use only one echo instruction instead of multiples.
  4. Fine to have a separate manifest with the cluster role (makes it easier to apply the others from the CI)
  5. Yes, it would be nice to have a bit of templating for the namespace, image version and so on

infrastructure/ssh-bastion/motd Outdated Show resolved Hide resolved
log.Info("reconciling bastion")

// Get tenants resources
var list crownlabsalpha1.TenantList
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var list crownlabsalpha1.TenantList
var tenants crownlabsalpha1.TenantList

Just a proposal, to make the code more readable

@giorio94 giorio94 added the kind/feature New feature or request label Nov 30, 2020
@claudious96
Copy link
Contributor Author

Thanks for the input @frisso , I've just tested the case (updated the keys on an existing Tenant resource) and the authorized_keys file is correctly updated.
I'll update the description.

@claudious96
Copy link
Contributor Author

@giorio94 I updated the PR with your suggestions.

The manifest can now be generated like the lab operator, setting desired values, then exporting the envs and substituting them in the template. In case of using the default namespace the result of the kubectl apply is namespace/default configured, I'm not sure if actually changes something though.

I wrote some instructions in the README inside the infrastructure/ssh-bastion folder to generate host keys and create the relative secret. I'm not sure if it's okay to have the inscructions there since the kubectl command depends on the namespace the deployment is going to run.

I have also updated the var you suggested in the bastion controller and the message in motd.

@claudious96 claudious96 requested a review from giorio94 December 1, 2020 10:34
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

/lgtm

Just a couple of minor comments inline.
Could you also add the bastion-operator to this list in order to automatically generate the Docker images?

matrix:
component:
- laboratory-operator
- crownlabs-image-list

operators/pkg/controllers/bastion_controller.go Outdated Show resolved Hide resolved
operators/deploy/bastion-operator/k8s-cluster-role.yaml Outdated Show resolved Hide resolved
operators/deploy/bastion-operator/k8s-manifest.yaml.tmpl Outdated Show resolved Hide resolved
add bastion dockerfile and bastion-operator
tenant's public keys injection to vms with cloud-init
tests with ginkgo
docs
@claudious96 claudious96 marked this pull request as ready for review December 12, 2020 09:17
@palexster
Copy link
Member

/merge

@kingmakerbot kingmakerbot merged commit 948d9df into netgroup-polito:master Dec 12, 2020
@kingmakerbot
Copy link
Collaborator

Your staging environment has been correctly teared-down!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants