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 new plugin - kubectl sshd #462

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

ottoyiu
Copy link
Contributor

@ottoyiu ottoyiu commented Jan 30, 2020

Requesting to add a new plugin: kubectl sshd.

Thanks for the consideration :)

A kubectl plugin that utilize dropbear to run a temporary SSH server on any Running Pod in your Kubernetes cluster.

It can be useful to run a SSH server in an existing Pod to securely copy files off the Pod itself using scp or rsync, without having to re-deploy a Pod with a SSH sidecar.

The use of this plugin should eventually be deprecated in favour of Ephemeral Containers (alpha since Kubernetes v1.16): https://kubernetes.io/docs/concepts/workloads/pods/ephemeral-containers/

Caution: In most cases, using SSH to gain access to a Pod is considered an anti-pattern. Please consider using kubectl exec for executing commands and kubectl cp for copying files between a Pod and your local workstation.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 30, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @ottoyiu!

It looks like this is your first PR to kubernetes-sigs/krew-index 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew-index has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ottoyiu
To complete the pull request process, please assign corneliusweig
You can assign the PR to them by writing /assign @corneliusweig in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 30, 2020
@ahmetb
Copy link
Member

ahmetb commented Jan 30, 2020

I'll defer to @corneliusweig as he's reviewed similar plugins recently.

@corneliusweig
Copy link
Contributor

Hey @ottoyiu, thanks for submitting this plugin to krew. As a first step, I'd like to understand what is the use-case for this plugin. Your landing page rightfully says that it should not be (ab)used for a remote shell or copying files. Instead, users should use kubectl exec and kubectl cp (I agree!).
So what problem does kubectl-sshd solve that is not already solved by kubectl?

In particular, for your plugin to work the container already needs to be capable of kubectl exec and kubectl cp (so it doesn't work for distroless containers).

Additionally, you return the podIp to talk to sshd. My understanding is that this IP is a k8s-network private IP, so that it can't be reached from outside. I may have missed something, but I wonder how this works.

@ottoyiu
Copy link
Contributor Author

ottoyiu commented Jan 30, 2020

Hi @corneliusweig!

Thanks for taking a look and reviewing my plugin. We have multiple Kubernetes clusters on-premise, that have podIPs that are first-class citizens and are fully route-able within and across our datacenters. From my understanding, it is common place for on-premise setups to use a fully routed layer 3 network for pod networking. Some cloud environments may accommodate this by using IP aliasing. However, it will depend on the environment, and so perhaps another feature that can be implemented for the script is to setup a local tunnel using kubectl proxy.

One of the use-cases for this plugin for us, is having the ability to transfer existing Pod's data from one cluster to another using rsync over SSH. Sometimes, it is difficult for us to inject a sidecar that has access to the filePath/volume path in order for us to pull data off the Pod.

An example of this is, an application dumping a coredump in a filepath that's not stored in a volume/filepath that a sidecar can access, yet we want to remotely pull it off the Pod and aggregate it without having to download it locally using kubectl cp.

Hope that clarifies the plugin somewhat.

@corneliusweig
Copy link
Contributor

Thanks for clarifying this. Are there any other important use-cases that you want to solve with this plugin?

If what you want to achieve is rsync from one pod to another, I'd suggest to focus on exactly that use-case for a plugin. This will be much more useful for end-users, than a generic kubectl-sshd plugin which requires extra effort to set up rsync. Besides, it may not be obvious for users how to achieve this and also tedious to get done.

My concern with the current plugin is that it looks too much like a kubectl exec replacement (one that will not even work in all setups). By focusing on the real goal (rsync?) you could greatly improve the UX for this task.

@ottoyiu
Copy link
Contributor Author

ottoyiu commented Feb 1, 2020

The main use-case is to enable any users to use an SSH client straight up to remote connect to a Pod, and also to allow any applications that use the SSH protocol to complete its task.

rsync is only one of many numerous applications that rely on SSH as the base protocol and the risk of making it more niche of a plugin is the prevention of others to making kubectl sshd a building block for scripting - thus making it even more rare that you would need such a plugin.

I plan to add examples of how to use kubectl sshd in scripts to do tasks such as rsync, rdiff-backup, or even one-off restic backups. If you believe that that's not enough to make the cut as a krew-plugin, I will understand.

Overall, I draw parallels of making this plugin more specific for a use-case like rsync to something as having kubectl implement specific exec commands instead, like: kubectl exec-ls or kubectl exec-cat instead of just the kubectl exec command itself. Yes, the individual exec commands will serve that specific use-case very well, but it does not allow the user to fully exercise their creativity and freedom to do anything else.

@ahmetb
Copy link
Member

ahmetb commented Feb 2, 2020

I think increasingly it's becoming more clear the line is hard to draw, but in this space, there's increasingly more and more plugins –and this one is distinct enough.

Since it's valid to a considerable subset of users, I feel like we should have this in krew-index. Many other plugins are also applicable to small set of users.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Alright, then let's add this to krew-index.

Your manifest already looks quite good. There are just minor nits that need attention (see inline comments).

Finally, can you make sure that all text blocks wrap at ~80 chars, so that it prints nicely on smaller screens?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 4, 2020
@ottoyiu
Copy link
Contributor Author

ottoyiu commented Feb 4, 2020

@ahmetb @corneliusweig thank you for accepting the plugin! I have made the requested changes.

@ahmetb
Copy link
Member

ahmetb commented Feb 4, 2020

@alonyb It looks like something is wrong with out GitHub Workflow check:
https://github.com/kubernetes-sigs/krew-index/pull/462/checks?check_run_id=426529087 shows:
image

This probably happened because:

force-pushed the ottoyiu:add-plugin-sshd branch from 7c6691d to 307f680 7 minutes ago

@corneliusweig
Copy link
Contributor

So the action does the following:

git diff --name-only --diff-filter=AM ${{ github.event.before }} ${{ github.sha }}

Looks like github.event.before becomes the sha of the previous tip when doing a force-push.

@alonyb Would this also work?

git diff --name-only --diff-filter=AM origin/master ${{ github.sha }}

@corneliusweig
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2020
@alonyb
Copy link
Contributor

alonyb commented Feb 5, 2020

@corneliusweig @ahmetb
In fact, the problem is the force-pushed, it is also not good practice. It's trying to get diff with a replaced hash that shouldn't be there. Actually git diff command brings the differences between the currently latest commit and the last commit, we should keep that configuration. Since I don't think it's a good idea to always compare against the master branch, it wouldn't work.
Origin is a "remote". A remote is just a reference to another remote clone of the repository and git diff cannot make comparisons between whole repositories, only between revisions/commits. That's what the error message is telling you.

For these cases, I could make a validation for PR(maybe or sth to identify force-pushed) to comparing only the new hash ${{ github.sha }}, but this is only for force-pushed such is rarely given. When it is a normal push, run the current flow.

So the action does the following:

git diff --name-only --diff-filter=AM ${{ github.event.before }} ${{ github.sha }}

Looks like github.event.before becomes the sha of the previous tip when doing a force-push.

@alonyb Would this also work?

git diff --name-only --diff-filter=AM origin/master ${{ github.sha }}

@ottoyiu
Copy link
Contributor Author

ottoyiu commented Feb 5, 2020

@alonyb

In fact, the problem is the force-pushed, it is also not good practice

Perhaps I'm misunderstanding this statement. Squashing commits is rather standard for PRs to remove intermediate commits that pollute the commit history with unimportant commit messages. After a squash/rebase, it makes sense to force-push it back on the same branch as the opened PR. It feels pretty important to me that a workflow check would support this.

If there's a better practice, I would love to know about it so that I can be a better contributor. Thanks :)

@alonyb
Copy link
Contributor

alonyb commented Feb 5, 2020

This #476 should fix this, it's the solution I mentioned before

@corneliusweig @ahmetb
For these cases, I could make a validation for PR(maybe or sth to identify force-pushed) to comparing only the new hash ${{ github.sha }}, but this is only for force-pushed such is rarely given. When it is a normal push, run the current flow.

@ahmetb
Copy link
Member

ahmetb commented Feb 5, 2020

“Force pushes is a not a good practice” is simply inaccurate. :)
The CI system should not care about this, as it should be able to tell what’s the base ref vs HEAD ref.

@corneliusweig corneliusweig changed the title add new plugin - kubectl sshd Add new plugin - kubectl sshd Feb 5, 2020
@corneliusweig corneliusweig merged commit c9106a4 into kubernetes-sigs:master Feb 5, 2020
@corneliusweig
Copy link
Contributor

Merged with admin privileges as the failing check is due to a bug in our CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gray-area lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants