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

Fix multiple networks with separately specified ip and mac #867

Merged
merged 10 commits into from
Apr 8, 2024

Conversation

baszoetekouw
Copy link
Contributor

Currently, podman-compose incorrectly parses docker-compose file with multiple networks, for which the ipv4_address, ipv6_address or mac_address is specified for each network (see eg #817). There is a partial fix in devel, which solves this issue for the case that the networks addresses are explicitly specified for all defined addresses. However, two cases still fail:

  • if the mac_address is specified (either globally or per interface)
  • if one interface has no explicit ipv4 address, and one does; this happens for example when defining an internal net (which should use dynamic addresses) and a macvlan interface with a fixed ip on the host network.

To be more specific, current HEAD of podman-compose, using this configuration:

---
version: "3"
services:
  number_1:
    image: busybox
    networks:
      internal:
  number_2:
    image: busybox
    networks:
      external:
        ipv4_address: "192.168.1.242"
        mac_address: "02:01:42:00:01:03"
      internal:

networks:
  internal:
    driver: "bridge"
    internal: true
    dns_enabled: true
  external:
    driver: "macvlan"
    driver_opts:
      parent: "eth0"
    ipam:
      driver: "host-local"
      config:
        - subnet: "192.168.1.0/24

fails with:

podman-compose version: 1.0.7
['podman', '--version', '']
using podman version: 4.3.1
** excluding:  set()
['podman', 'ps', '--filter', 'label=io.podman.compose.project=tmp', '-a', '--format', '{{ index .Labels "io.podman.compose.config-hash"}}']
podman pod create --name=pod_tmp --infra=false --share=
49e035ad3839ef2d6a6a4d4c1174fb0e0e7fccf3f9dcedfc9b8dd3edcd8f5bfc
exit code: 0
['podman', 'network', 'exists', 'tmp_internal']
podman create --name=tmp_number_1_1 --pod=pod_tmp --label io.podman.compose.config-hash=eef2914b01b6c8cee877f8ffb53431be4086f78a986ece813948eac9bf7d93ef --label io.podman.compose.project=tmp --label io.podman.compose.version=1.0.7 --label PODMAN_SYSTEMD_UNIT=podman-compose@tmp.service --label com.docker.compose.project=tmp --label com.docker.compose.project.working_dir=/tmp --label com.docker.compose.project.config_files=test1.yml --label com.docker.compose.container-number=1 --label com.docker.compose.service=number_1 --net tmp_internal --network-alias number_1 busybox
a35b3df3e0497580f9b6d9ca3d07026200e2b477e7b63f12d048210c31702fac
exit code: 0
['podman', 'network', 'exists', 'tmp_external']
['podman', 'network', 'exists', 'tmp_internal']
podman create --name=tmp_number_2_1 --pod=pod_tmp --label io.podman.compose.config-hash=eef2914b01b6c8cee877f8ffb53431be4086f78a986ece813948eac9bf7d93ef --label io.podman.compose.project=tmp --label io.podman.compose.version=1.0.7 --label PODMAN_SYSTEMD_UNIT=podman-compose@tmp.service --label com.docker.compose.project=tmp --label com.docker.compose.project.working_dir=/tmp --label com.docker.compose.project.config_files=test1.yml --label com.docker.compose.container-number=1 --label com.docker.compose.service=number_2 --net tmp_internal,tmp_external --network-alias number_2 --ip=192.168.1.254 busybox
Error: --ip can only be set for a single network: invalid argument
exit code: 125
[number_1] podman start -a tmp_number_1_1
[number_2] podman start -a tmp_number_2_1
[number_2] | Error: no container with name or ID "tmp_number_2_1" found: no such container
[number_2] exit code: 125
[number_1] exit code: 0

This is caused by podman-compose parsing the compose file network config to

--net tmp_internal,tmp_external --ip=192.168.1.254

which in invalid.

With this PR, it executes

podman create --name=tmp_number_2_1 --pod=pod_tmp --label io.podman.compose.config-hash=eef2914b01b6c8cee877f8ffb53431be4086f78a986ece813948eac9bf7d93ef --label io.podman.compose.project=tmp --label io.podman.compose.version=1.0.7 --label PODMAN_SYSTEMD_UNIT=podman-compose@tmp.service --label com.docker.compose.project=tmp --label com.docker.compose.project.working_dir=/tmp --label com.docker.compose.project.config_files=test1.yml --label com.docker.compose.container-number=1 --label com.docker.compose.service=number_2 --network tmp_external:ip=192.168.1.254,mac=02:01:42:00:01:03 --network tmp_internal busybox

so the network arguments are

--network tmp_external:ip=192.168.1.254,mac=02:01:42:00:01:03 --network tmp_internal

instead.

Note that I've added a test case for this issue in tests/test_podman_compose_networks.py. I'm not entirely sure this this is the correct way to integrate the tests, so please let me know if I should adjust that.

Closes #817

Copy link
Collaborator

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It looks good in principle.

The test suite has moved to unittest in main branch, so this PR needs to be updated to match.

Also I think it would be useful to add explicit unit tests to get_net_args function. See e.g. what I did in this PR: https://github.com/containers/podman-compose/pull/819/files.

With unit tests we can cover way more special cases as they run so fast.

@baszoetekouw
Copy link
Contributor Author

Hi @p12tic ! I'm working on the unit tests as you suggested, but I encountered a bit of a problem.

It turns out that docker-compose doesn't support specifying a mac_address for a specific network interface. Instead, it only allows specification of mac_address per container, and seems to randomly apply it to one or more of the specified network interfaces (haven't tried this myself as I don't currently have access to a machine running docker, but this seems to be the consensus on stackexchange).

Of course, podman is much nicer than docker and properly supports specification of mac_addressper network (with similar semantics as for ipv4 and ipv6 addresses), so it would be nice if podman-compose could also support this.

There are two choices to make here. First, how to handle the container-level mac_address option. I see three options:

  1. apply the mac_address the to all specified networks in the container
  2. apply the mac_address to the first network interface specified
  3. apply the mac_address to the first macvlan network interface specified

I would expect that option 1 might lead to network headaches inside the container, that option 2 is probably most like the original docker-compose behaviour, and option 3 might mostly do what people might expect (as there is usually no reason to specify mac_address unless you're using a macvlan interface).

And then there's the question if you would like podman-compose to support "extended" podman functionality and allow specification of the mac_addreess per interface, like this:

services:
    some_service:
        image: busybox
        hostname: myhost
        command: ["/bin/busybox", "httpd", "-f", "-h", "/var/www/html", "-p", "8001"]
        working_dir: /var/www/html
        networks:
            shared-network:
                ipv4_address: "172.19.1.10"
                mac_address: "02:01:01:00:01:01"
            internal-network:
                ipv4_address: "172.19.2.10"
                mac_address: "02:01:01:00:02:01"

Please let me know how you would like me to implement this.

@p12tic
Copy link
Collaborator

p12tic commented Mar 11, 2024

docker-compose spec says it just passes the mac address as --mac-address option to docker run. In the docker CLI code there's this logic:

  • if there's per-network mac-address and container mac address, exit with error
  • otherwise, apply mac address to the attachment to the first network if there are multiple networks, or to the attachment to the default network.

You can see this here https://github.com/docker/cli/blob/a2f3f40233bbe9cd119bed9048973366789de8b5/cli/command/container/opts.go#L810

I think it makes sense to preserve this behavior for the container level mac_address.

As for network-level mac_address, we can add this option as podman.mac_address. If you end up adding it, then please also document it as a podman-compose specific extension in a new file in docs/.

@baszoetekouw
Copy link
Contributor Author

I've made the changes as you requested:

  • mac_address support is now identical to docker-composer; the podman-specific per-network mac_address settings are now called podman.mac_address and this extension is documented in docs/Extensions.md
  • I've added unit tests as you suggested. As a bonus, get_net_args() now has 100% coverage
  • I've fixed a few bugs in the handling of the network_mode parameter that I encountered while writing the unittests
  • I've added the last missing podman-specific network_mode values (private and pasta), and documented those (together with the pre-existing podman-specific values slirp4netns and ns:) also in docs/Extensions.md
  • synced with updated test suites in main

@baszoetekouw baszoetekouw requested a review from p12tic March 21, 2024 19:34
self.assertListEqual(expected_args, args)

@parameterized.expand([
# TODO: check is this behaviour introduced by this patch?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftover that probably shouldn't be in a PR?

Copy link
Collaborator

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Just one small nit and we can merge this in. Thanks a lot!

@baszoetekouw
Copy link
Contributor Author

Indeed, I forgot to remove that comment. Fixed now!

@baszoetekouw baszoetekouw requested a review from p12tic March 28, 2024 09:44
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
… addresses

Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Specifically:
  - use "--network=foo" instead of "--network foo"
  - specify "--network-alias" multiple times instead of concatenating values

Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
@p12tic
Copy link
Collaborator

p12tic commented Mar 28, 2024

Rebased and will merge soon. Thanks!

@p12tic p12tic merged commit e893d06 into containers:main Apr 8, 2024
8 checks passed
@p12tic p12tic mentioned this pull request May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

With multiple networks: Unable to specify per-network static ip addresses
2 participants