-
-
Notifications
You must be signed in to change notification settings - Fork 466
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
[Feature] Docker machine #71
Conversation
The kubeconfig.yaml generated by K3S uses local host as the host name by default. It won't work when running with docker machine. This patch detects if the docker environment is set up with docker machine. If it is, then replace the host name to the IP address of the docker machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it and the code looks good to me, though I couldn't test it on Windows yet.
I'll test this on windows and report back |
cli/docker-machine.go
Outdated
return "", err | ||
} | ||
|
||
out, err := exec.Command(dockerMachinePath, "ip").Output() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the machine
variable from L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think 'machine' here is necessary. Without it, the machine name will be picked up from the environment variable, which will end up being the same. Never the less, I will make the suggested change.
I tested this on my machine and after my comment is addressed it worked great, but I think maybe we can extend this by using |
@zeerorg , Thanks for the review, test and suggestion. This is an interesting idea, I like it. It probably needs a bit more thoughts:
On the other hand, your suggestion is more general and more powerful. May be we should consider it for some future releases? for example, post 2.0.0 Personally, I also hope it can be offered when it enables use cases the community cares about, not for technical reasons alone. At any rate, I will merge the PR as is because it solves a problem at hand. |
When running on a docker machine, the default X598 certificate does not allow access via docker machine's IP. This patch fixes this by adding "--tls-san <docker machine IP>" to the K3S server argument list.
@andyz-dev FYI I had to solve a similar problem - communicating with the machine that Docker Daemon is running in general. The most clean and correct way I could find was STDIO port-forwarding using socat. It works in any situation and doesn't require a heuristic detection. https://gist.github.com/kkimdev/534669eea45b856dbd523faba7f16c4a |
return "", nil | ||
} | ||
|
||
dockerMachinePath, err := exec.LookPath("docker-machine") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also check docker-machine.exe
? "docker toolbox on windows" installs it as .exe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. It worked for me since I tested using the shell provided by docker toolbox on windows.
Do you mind send a PR that implements this idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to this patch, exec.LookPath() on windows should attempt to find executables with .exe or .com extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info. Seems like fairly simple but personally I never used Go before so I'll leave it to others.
) | ||
|
||
func getDockerMachineIp() (string, error) { | ||
machine := os.ExpandEnv("$DOCKER_MACHINE_NAME") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calling docker-machine active
to get the machine name would be more reliable as environmental variables might not be set or overridden by users.
Also we're calling docker-machine ip
instead of DOCKER_HOST already anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried doing docker-machine active
and didn't get any output (even though one of the machines is running). I am not familiar with this command can you describe more of what needs to be done ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting. Mine outputs default
and the explanation is that
$ docker-machine
...
Commands:
active Print which machine is active
..
What's your output of docker-machine env
? Does it have the following line?
SET DOCKER_MACHINE_NAME=default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output for docker-machine env dvm
"dvm" is the name of the docker-machine
PS C:\WINDOWS\system32> docker-machine env dvm
$Env:DOCKER_TLS_VERIFY = "1"
$Env:DOCKER_HOST = "tcp://172.17.209.142:2376"
$Env:DOCKER_CERT_PATH = "C:\Users\user\.docker\machine\machines\dvm"
$Env:DOCKER_MACHINE_NAME = "dvm"
$Env:COMPOSE_CONVERT_WINDOWS_PATHS = "true"
# Run this command to configure your shell:
# & "C:\Program Files\Docker\Docker\Resources\bin\docker-machine.exe" env dvm | Invoke-Expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about docker-machine ls
Does "dvm" line has an active mark on the left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really strange then. docker-machine ls
reports dvm
is active while docker-machine active
reports nothing is active. I can't think of anything else than it's a docker-machine bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to find similar issues in their repo. docker/machine#1651 docker/machine#3448
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, definitely that issue. What do you suggest ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess we don't have any choice other than DOCKER_HOST. I just looked up their active check, and it's also just a combination of [DOCKER_HOST + current status] https://github.com/docker/machine/blob/19973f2b1bbcb7b1c07a3de572177bcaf8dedcd5/commands/ls.go#L490
Also, docker-machine repository hasn't been updated for several months so we can't expect it to be fixed anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we should leave it like this for now or create a new issue regarding this.
Also should fix #68.
Docker Toolbox uses a docker-machine to run docker containers. This bug exposes the fact k3d does not recognize docker machine in general. This PR adds support for docker machine, thus should also fix #68.
I don't usually use Docker Toolbox, or other docker machine set up. This PR could benefit from more testing from those who do use them.