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

Generate a ssh port from 1025..9999 range #68

Merged
merged 1 commit into from
Dec 18, 2022
Merged

Generate a ssh port from 1025..9999 range #68

merged 1 commit into from
Dec 18, 2022

Conversation

jay7x
Copy link
Member

@jay7x jay7x commented Dec 18, 2022

It was possible to generate a ssh port <= 1024 before. This is ok for rootful containers but it doesn't work for rootless. That causes another re-provisioning attempt until the port above 1024 is generated. With this change the generated port is always within 1025..9999 inclusive range.

It was possible to generate a ssh port <= 1024 before. This is ok for rootful containers but it doesn't work for rootless. That causes another re-provisioning attempt until the port above 1024 is generated. With this change the generated port is always within 1025..9999 inclusive range.
@codecov
Copy link

codecov bot commented Dec 18, 2022

Codecov Report

Base: 84.05% // Head: 84.05% // No change to project coverage 👍

Coverage data is based on head (77b54d7) compared to base (179f77b).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   84.05%   84.05%           
=======================================
  Files           1        1           
  Lines         301      301           
=======================================
  Hits          253      253           
  Misses         48       48           
Impacted Files Coverage Δ
lib/beaker/hypervisor/docker.rb 84.05% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Having an explicit range is so much better than taking a substring on a random number. I do wonder if 9999 should be the max, or 65535.

Another thing I wonder about is whether Docker can't do this natively. So create the container and then inspect to find out whatever Docker assigned to it, but I guess we can leave that for another day.

@trevor-vaughan
Copy link
Contributor

This looks like a great idea.

From a brief dig through the docker docs, if you want to use a specific port, you need to map it. I do like the idea of telling it that you want a mapping and letting it decide, but I don't think I've ever seen that done anywhere.

We should probably improve this at some point to validate that the port we're trying to use isn't already in use but that may be done elsewhere, I don't quite recall.

@bastelfreak bastelfreak merged commit 3ff2753 into voxpupuli:master Dec 18, 2022
@jay7x
Copy link
Member Author

jay7x commented Dec 19, 2022

Having an explicit range is so much better than taking a substring on a random number. I do wonder if 9999 should be the max, or 65535.

I thought about this but decided to keep the port to have 4 digits as before. Another idea was to make the range configurable (via env vars e.g.)

@jay7x jay7x deleted the patch-1 branch December 19, 2022 02:29
@jay7x
Copy link
Member Author

jay7x commented Dec 19, 2022

Wrt inspecting the container it's quite possible:

➜  ~ docker container inspect 7341d1b9fd52 | jq '.[0].NetworkSettings.Ports'
{
  "22/tcp": [
    {
      "HostIp": "0.0.0.0",
      "HostPort": "1611"
    }
  ]
}
➜  ~ docker container inspect 7341d1b9fd52 | jq '.[0].NetworkSettings.Ports["22/tcp"][0]["HostPort"]' 
"1611"

N.B. docker is actually podman.lima which is podman running in a lima VM.

@jay7x
Copy link
Member Author

jay7x commented Dec 19, 2022

Well.. I tried to remove the following lines: https://github.com/voxpupuli/beaker-docker/blob/master/lib/beaker/hypervisor/docker.rb#L80-L82

          'PortBindings' => {
            '22/tcp' => [{ 'HostPort' => rand(1025..9999).to_s, 'HostIp' => '0.0.0.0'}]
          },

.. and get_ssh_connection_info() function was still able to get the mapped port (thanks to PublishAllPorts: true)! I.e. it works for me.

➜  ~ docker container ls -a                                                 
CONTAINER ID  IMAGE                                                             COMMAND     CREATED         STATUS             PORTS                  NAMES
cfe2dc3e58f9  4a1b665b8afcf7641fcde01f56009704d18ecbaeeb784a7b0b0844d18ca7852a  /sbin/init  23 seconds ago  Up 23 seconds ago  0.0.0.0:37225->22/tcp  beaker-ubuntu2004-64-1-50434a0ae574
➜  ~ docker container inspect cfe2dc3e58f9 | jq '.[0].NetworkSettings.Ports'    
{
  "22/tcp": [
    {
      "HostIp": "",
      "HostPort": "37225"
    }
  ]
}
Relevant debug output from `rake beaker`
[...]
Creating container from image 4a1b665b8afc
post
/containers/create
{"name"=>"beaker-ubuntu2004-64-1-50434a0ae574"}
{"Image":"4a1b665b8afc","Hostname":"ubuntu2004-64-1","HostConfig":{"PublishAllPorts":true,"RestartPolicy":{"Name":"always"},"Privileged":true}}
[...]
Starting container cfe2dc3e58f913dfb9be304f85dd707a583dfaca5d8d98eab3b85c6c17b57658
[...]
Using container connection at :37225
node available as ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no root@ -p 37225
[...]

@trevor-vaughan
Copy link
Contributor

We'll need to give it a go from "real" docker as well. podman implements the docker spec more precisely than docker itself :-/ (but this is good news!)

@jay7x
Copy link
Member Author

jay7x commented Dec 19, 2022

I'll try with docker and nerdctl later.. though I'm not sure about Docker Swarm which is kinda supported too.. I'd expect biggest problems from this side as there are multiple hosts involved

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.

None yet

4 participants