Attach docker nodes to their declared networks - #104
Merged
Conversation
networks: on a docker node was decoded and dropped. Containers were created with an empty networking configuration, so every node landed on the default bridge: a documented static ip: had no effect, containers could not resolve each other by name, and a suite asserting that two nodes were isolated passed because they had never been separated. The declared networks now reach the container. Docker accepts one endpoint at creation, so the first entry is applied there and the rest are connected immediately afterwards, preserving list order. A static address is passed as the endpoint's IPAM configuration. subnet on a node-level entry is now a configuration error pointing at docker.networks, which is what creates the network — a node joins one rather than defining it. This matches the rule already applied to lxd nodes. Wrapper.cli is now the client interface rather than the concrete type, so the call sequence can be asserted without a daemon; GetClient and RemoveImage widen to match, both of which only ever needed the interface. Note: verified against a recording client rather than a live daemon — no Docker was reachable from this machine or the datacenter hosts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #93.
The problem
networks:on a docker node was decoded intoDockerNodeOpts.Networksand then never read. Containers were created with an emptyNetworkingConfig, so every node landed on Docker's default bridge.Three consequences, in increasing order of seriousness:
ip:had no effect.The shipped
examples/docker/docker.yamlrelies on it: two containers ontest_netat192.168.200.10and.100. Those addresses were silently ignored.The fix
Declared networks now reach the container. Docker accepts a single endpoint at creation time, so the first entry is applied there and any others are connected immediately afterwards with
NetworkConnect, preserving list order. A static address is passed as the endpoint's IPAM configuration.subneton a node-level entry is now a configuration error pointing atdocker.networks— a node joins a network rather than defining one. That matches the rule already applied to lxd nodes in #97.Testability change
Wrapper.cliwas the concrete*client.Client, which made the create path impossible to test without a daemon. It is nowclient.APIClient;GetClientandRemoveImagewiden to match, and both only ever needed the interface —NewComposeStackalready took it.Verification — please read
No Docker daemon was reachable from this machine or from any datacenter host I tried (
services-01/02/03,control-ws), so this is not verified end to end against a real daemon. Given that, I tested the wiring against a recording client that captures the create call and follow-up connects:EndpointsConfigand needs no follow-up connectIPAMConfig.IPv4AddressThat covers my logic but not Docker's acceptance of it. The parts I would want confirmed on a real daemon before trusting this in anger:
examples/docker/docker.yamlnow actually reaches.10and.100and its tests pass.If you have a host with Docker, that example is the fastest way to confirm both. Build, vet, gofmt, and the full test suite are green, and every example still passes
--check.