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 ability to start & stop ssh-agent process #16761

Merged
merged 6 commits into from
Jun 30, 2023

Conversation

spowelljr
Copy link
Member

@spowelljr spowelljr commented Jun 23, 2023

This PR adds an sshagent package that allows starting and stoping an ssh-agent process. Once a process is started it's auth socket and agent pid are added to the cluster config. When running docker-env, if the auth socket and agent pid are populated in the cluster config they'll be included in the command output. On cluster delete the process is killed.

When creating this output I added sshagent.Start() to the beginning of cmd/minikube/cmd/docker-env.go

sshagent.Start() will be implemented in #15452

$ pgrep ssh-agent
1219

$ minikube docker-env
export DOCKER_TLS_VERIFY="1"
export DOCKER_HOST="tcp://127.0.0.1:59146"
export DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs"
export MINIKUBE_ACTIVE_DOCKERD="minikube"
export SSH_AUTH_SOCK="/var/folders/9l/6wpxv6wd1b901m1146r579wc00rqw3/T//ssh-KCQt1sNqrCPI/agent.29227"
export SSH_AGENT_PID="29228"

# To point your shell to minikube's docker-daemon, run:
# eval $(minikube -p minikube docker-env)

$ pgrep ssh-agent
1219
29228

$ minikube delete
🔥  Deleting "minikube" in docker ...
🔥  Deleting container "minikube" ...
🔥  Removing /Users/powellsteven/.minikube/machines/minikube ...
💀  Removed all traces of the "minikube" cluster.

$ pgrep ssh-agent
1219

Before:

$ minikube docker-env
export DOCKER_TLS_VERIFY="1"
export DOCKER_HOST="tcp://127.0.0.1:51030"
export DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs"
export MINIKUBE_ACTIVE_DOCKERD="minikube"

# To point your shell to minikube's docker-daemon, run:
# eval $(minikube -p minikube docker-env)

After w/ agent stopped:

$ minikube docker-env
export DOCKER_TLS_VERIFY="1"
export DOCKER_HOST="tcp://127.0.0.1:51030"
export DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs"
export MINIKUBE_ACTIVE_DOCKERD="minikube"

# To point your shell to minikube's docker-daemon, run:
# eval $(minikube -p minikube docker-env)

After w/ agent started:

$ minikube docker-env
export DOCKER_TLS_VERIFY="1"
export DOCKER_HOST="tcp://127.0.0.1:51030"
export DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs"
export MINIKUBE_ACTIVE_DOCKERD="minikube"
export SSH_AUTH_SOCK="/var/folders/9l/6wpxv6wd1b901m1146r579wc00rqw3/T//ssh-rrVWjcEBi4pO/agent.37343"
export SSH_AGENT_PID="37344"

# To point your shell to minikube's docker-daemon, run:
# eval $(minikube -p minikube docker-env)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 23, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 23, 2023
@medyagh
Copy link
Member

medyagh commented Jun 23, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 23, 2023
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

two questions
1-
does this PR make an ssh-agent by default even if it is not used by docker-env ?

2-
can we make minikube use this SSH agent ? I bet this would make our command runner faster
after we create a KIC container we switch from docker exec to SSH that made minikube faster.

here are some relevent code:

@spowelljr
Copy link
Member Author

@medyagh

  1. This PR doesn't start the ssh-agent at all, I just implemented the functionality, I'm leaving it up to feat: direct image build on containerd via docker-env #15452 to call Start when necessary.

  2. I don't see why not, would need additional modifications though

@medyagh
Copy link
Member

medyagh commented Jun 23, 2023

  1. I don't see why not, would need additional modifications though

would you be interested to plug it in to make our SSH_RUNNER to use this agent and benchmark the Command Runner metrics ( we could see how many miliseconds or seconds it takes to run pwd or ls commands using with or without agent..

because without agent I assume it would have to load the SSH key on each connection to memory

@minikube-pr-bot

This comment has been minimized.

@spowelljr
Copy link
Member Author

  1. I don't see why not, would need additional modifications though

would you be interested to plug it in to make our SSH_RUNNER to use this agent and benchmark the Command Runner metrics ( we could see how many miliseconds or seconds it takes to run pwd or ls commands using with or without agent..

because without agent I assume it would have to load the SSH key on each connection to memory

Could do that, would do it in a follow up PR though

@minikube-pr-bot

This comment has been minimized.

@k8s-ci-robot
Copy link
Contributor

@spowelljr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-minikube-build ca7e0fd link true /test pull-minikube-build

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16761) |
+----------------+----------+---------------------+
| minikube start | 51.4s    | 51.1s               |
| enable ingress | 28.0s    | 26.7s               |
+----------------+----------+---------------------+

Times for minikube (PR 16761) start: 51.9s 52.4s 51.6s 47.2s 52.3s
Times for minikube start: 54.3s 52.8s 51.7s 48.5s 49.9s

Times for minikube (PR 16761) ingress: 27.8s 27.2s 28.2s 25.6s 24.7s
Times for minikube ingress: 28.6s 28.7s 28.2s 27.3s 27.2s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16761) |
+----------------+----------+---------------------+
| minikube start | 24.3s    | 23.1s               |
| enable ingress | 49.3s    | 48.9s               |
+----------------+----------+---------------------+

Times for minikube start: 25.1s 25.1s 22.3s 24.2s 24.9s
Times for minikube (PR 16761) start: 21.7s 24.3s 25.1s 22.0s 22.4s

Times for minikube ingress: 49.3s 49.8s 48.8s 48.3s 50.3s
Times for minikube (PR 16761) ingress: 48.3s 49.3s 48.8s 48.3s 49.8s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16761) |
+----------------+----------+---------------------+
| minikube start | 22.3s    | 21.7s               |
| enable ingress | 31.1s    | 28.9s               |
+----------------+----------+---------------------+

Times for minikube ingress: 31.3s 31.3s 31.3s 30.3s 31.3s
Times for minikube (PR 16761) ingress: 31.3s 31.3s 31.3s 31.3s 19.4s

Times for minikube start: 19.7s 20.5s 24.2s 23.2s 23.6s
Times for minikube (PR 16761) start: 19.8s 19.8s 21.0s 24.2s 23.6s

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot
Copy link

These are the flake rates of all failed tests.

Environment Failed Tests Flake Rate (%)
Hyperkit_macOS TestStartStop/group/no-preload/serial/DeployApp (gopogh) 0.61 (chart)
Hyperkit_macOS TestStartStop/group/no-preload/serial/EnableAddonWhileActive (gopogh) 0.61 (chart)
Hyperkit_macOS TestStartStop/group/no-preload/serial/FirstStart (gopogh) 0.61 (chart)
Docker_Linux_docker_arm64 TestFunctional/parallel/MountCmd/specific-port (gopogh) 1.18 (chart)
Docker_Linux_docker_arm64 TestRunningBinaryUpgrade (gopogh) 1.76 (chart)
KVM_Linux_crio TestNoKubernetes/serial/StartNoArgs (gopogh) 1.80 (chart)
Hyperkit_macOS TestPreload (gopogh) 1.83 (chart)
Docker_Linux_crio TestFunctional/parallel/ImageCommands/ImageTagAndLoadDaemon (gopogh) 2.35 (chart)
QEMU_macOS TestFunctional/parallel/NonActiveRuntimeDisabled (gopogh) 5.23 (chart)
QEMU_macOS TestFunctional/parallel/UpdateContextCmd/no_changes (gopogh) 5.23 (chart)
QEMU_macOS TestFunctional/parallel/UpdateContextCmd/no_clusters (gopogh) 5.23 (chart)
QEMU_macOS TestFunctional/parallel/UpdateContextCmd/no_minikube_cluster (gopogh) 5.23 (chart)
QEMU_macOS TestFunctional/parallel/Version/components (gopogh) 5.23 (chart)
QEMU_macOS TestFunctional/serial/LogsCmd (gopogh) 5.84 (chart)
QEMU_macOS TestFunctional/parallel/CertSync (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/CpCmd (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/DashboardCmd (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/DockerEnv/bash (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/FileSync (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageBuild (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageListTable (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageLoadDaemon (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageLoadFromFile (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageReloadDaemon (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageSaveToFile (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ImageCommands/ImageTagAndLoadDaemon (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/NodeLabels (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/DeployApp (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/Format (gopogh) 5.88 (chart)
QEMU_macOS TestFunctional/parallel/ServiceCmd/HTTPS (gopogh) 5.88 (chart)
More tests... Continued...

Too many tests failed - See test logs for more details.

To see the flake rates of all tests by environment, click here.

@medyagh
Copy link
Member

medyagh commented Jun 26, 2023

@spowelljr could I plz see the output of docker-env before/after this PR (both incase the agent is started and incase the agent is not started)

@spowelljr
Copy link
Member Author

spowelljr commented Jun 27, 2023

@spowelljr could I plz see the output of docker-env before/after this PR (both incase the agent is started and incase the agent is not started)

Added before/after to the PR description

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: medyagh, spowelljr

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@medyagh
Copy link
Member

medyagh commented Jun 29, 2023

looks good to me @spowelljr my only question is, for docker-env on docker runtime, does the AFTER need to export the regular docker-env envs too ?

$ minikube docker-env
export DOCKER_TLS_VERIFY="1"
export DOCKER_HOST="tcp://127.0.0.1:51030"
export DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs"
export MINIKUBE_ACTIVE_DOCKERD="minikube"
export SSH_AUTH_SOCK="/var/folders/9l/6wpxv6wd1b901m1146r579wc00rqw3/T//ssh-rrVWjcEBi4pO/agent.37343"
export SSH_AGENT_PID="37344"

or is there a way to make regular docker-env to use that SSH_AGENT (kind of adding DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs" to the ssh agent ? ) maybe not

@spowelljr
Copy link
Member Author

looks good to me @spowelljr my only question is, for docker-env on docker runtime, does the AFTER need to export the regular docker-env envs too ?

$ minikube docker-env export DOCKER_TLS_VERIFY="1" export DOCKER_HOST="tcp://127.0.0.1:51030" export DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs" export MINIKUBE_ACTIVE_DOCKERD="minikube" export SSH_AUTH_SOCK="/var/folders/9l/6wpxv6wd1b901m1146r579wc00rqw3/T//ssh-rrVWjcEBi4pO/agent.37343" export SSH_AGENT_PID="37344"

or is there a way to make regular docker-env to use that SSH_AGENT (kind of adding DOCKER_CERT_PATH="/Users/powellsteven/.minikube/certs" to the ssh agent ? ) maybe not

Responded offline:
The ssh-agent is only going to get exported if it's started, and in my current PR it's not started at all. I'm leaving that up to #15452 to start when the user calls docker-env and they're using containerd, so if they're using docker CRI then the agent won't be started and it won't be included in the export

@spowelljr spowelljr merged commit 7ba7f7b into kubernetes:master Jun 30, 2023
@spowelljr spowelljr deleted the sshAgent branch June 30, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants