Engine port 2376 should not be hardcoded

docker / machine

Machine management for a container-centric world

https://docs.docker.com/machine/

Apache License 2.0

6.63k stars 1.97k forks source link

Engine port 2376 should not be hardcoded #3361

Open nathanleclaire opened 8 years ago

nathanleclaire commented 8 years ago

In many various places (drivers, provisioning, etc.) engine port :2376 is hardcoded. It should not be.

tpires commented 8 years ago

For docker-machine version 0.7.0, engine port is hardcoded at :

[ ] drivers/google/google.go
[ ] drivers/virtualbox/virtualbox.go
[ ] drivers/softlayer/driver.go
[ ] drivers/vmwarefusion/fusion_darwin.go
[ ] drivers/vmwarevsphere/vsphere.go
[ ] drivers/digitalocean/digitalocean.go
[ ] drivers/openstack/openstack.go
[ ] drivers/exoscale/exoscale.go
[ ] drivers/hyperv/hyperv.go
[ ] drivers/amazonec2/amazonec2.go
[ ] libmachine/provision/boot2docker.go
[ ] libmachine/provision/suse.go

tpires commented 8 years ago

In my opinion, Docker engine port shouldn't be hardcoded anywhere but libmachine/engine/engine.go as DefaultPort.

fsoppelsa commented 8 years ago

Agree @tpires and let users to overwrite somehow (and storing it in config.json).

I would be personally in favor of some --port CLI general option.

wdullaer commented 7 years ago

I'm thinking of taking a stab at this. Would this be an option at the subcommand level (like create and provision), or at the docker-machine level?

fsoppelsa commented 7 years ago

I think at driver level.

nathanleclaire commented 7 years ago

I think you might be able to use a create flag and use it passed in SetConfigFromFlags e.g., flags.Int("engine-port") (seems --generic-engine-port already exists...), although I forget if we filter it out to only driver-specific flags before we pass it to that method.

provision is a somewhat tricky though, since the drivers themselves don't really have a Provision method, it's all external, so you wouldn't be able to do any tricks like edit config.json and call provision and expect it to work (granted, that's kind of an unsupported/unofficial workflow anyway).

wdullaer commented 7 years ago

I'm still learning the codebase, so bear with me, but it seems like only fake_provisioner and the suse provisioner have a hardcoded reference to the port. All the other ones use a dockerPort variable. I still have to see where that variable comes from, but I get the impression that in the provisioner everything has already been properly factored out (in most cases), so I'll just have to tackle the create subcommand.

wdullaer commented 7 years ago

The azure and generic driver already expose driver level flags for this functionality. How would you like me to handle that? Right now I've just removed these flags, but that could be considered a semver major change. I can also make these flags output a deprecated message. The final option is to leave them functional, but then the question becomes: how do they interact with the flag defined at subcommand level? Which flag takes precedence?

I'm personally leaning towards disabling them and have them output a deprecation warning, so people using them right now know what's going on. They can then be removed in a future version.

wdullaer commented 7 years ago

I've made a PR that implements this. I'm trying to rebase it against master as often as I can, but it would be nice if someone can take a look at it.

wdullaer commented 7 years ago

Bimonthly ping to see if anyone is actually interested in the PR.

wdullaer commented 7 years ago

@fsoppelsa @nathanleclaire is anyone interested in taking a look at the PR?

nathanleclaire commented 7 years ago

I am not maintainer anymore -- ping @shin-