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

Don't create endpoint config for MAC addr config migration #47474

Merged
merged 1 commit into from Mar 6, 2024

Conversation

robmry
Copy link
Contributor

@robmry robmry commented Feb 29, 2024

- What I did

When a container-wide MAC address is supplied, it's migrated to the endpoint settings.

Previously, if the network identity supplied in HostConfig.NetworkMode did not match the key for a network in NetworkSettings.Networks, a new EndpointSettings was created to store the MAC address.

But, the mismatch could be due to mixing network name/id/short-id in those fields. In that case, creating a new EndpointSettings resulted in duplicate config for the network, and one set was eventually discarded.

- How I did it

Don't create a new EndpointSettings, unless there are no existing settings.

For the old API, if the API request contains a container-wide MAC address and more than one set of endpoint config, reject the request (not expecting more than one entry in NetworkSettings.Networks in that case). Otherwise, apply the container-wide setting to the single endpoint.

For the new API, if the deprecated container wide field is used in the new API and EndpointsConfig is provided for any network, the NetworkMode and key in NetworkSettings.Networks must be the same - no mixing of ids and names.

- How to verify it

New unit test, and a new integration test that mimics Watchtower container creation.

Without this change, the new integration test fails with Error response from daemon: no available IPv4 addresses on this network's address pools: wtmvl.

- Description for the changelog

Preserve supplied endpoint configuration in a container-create API request, when a container-wide MAC address is specified, but `NetworkMode` name-or-id is not the same as the name-or-id used in `NetworkSettings.Networks`.

api/server/router/container/container_routes.go Outdated Show resolved Hide resolved
integration/networking/mac_addr_test.go Outdated Show resolved Hide resolved
In a container-create API request, HostConfig.NetworkMode (the identity
of the "main" network) may be a name, id or short-id.

The configuration for that network, including preferred IP address etc,
may be keyed on network name or id - it need not match the NetworkMode.

So, when migrating the old container-wide MAC address to the new
per-endpoint field - it is not safe to create a new EndpointSettings
entry unless there is no possibility that it will duplicate settings
intended for the same network (because one of the duplicates will be
discarded later, dropping the settings it contains).

This change introduces a new API restriction, if the deprecated container
wide field is used in the new API, and EndpointsConfig is provided for
any network, the NetworkMode and key under which the EndpointsConfig is
store must be the same - no mixing of ids and names.

Signed-off-by: Rob Murray <rob.murray@docker.com>
@robmry robmry force-pushed the 47441_mac_addr_config_migration branch from 03a850a to a580544 Compare February 29, 2024 17:02
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

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

LGTM

@NonaSuomy
Copy link

NonaSuomy commented Mar 2, 2024

Will this fix watchtower without them having to do anything or will they still have to do something for this to work properly?

I use mac_address: on about 30 containers with v25.0.3 docker compose they get a static MAC just fine. When watchtower runs it gives them random MACs.

In the latest 25.0.3 mac_address needs to be under networks or it fails to set it.

They run like this example, DNSMasq gives them a static IP address from DHCP set in dnsmasq.conf

version: '3.8'
services:
  nginx:
    image: nginx:latest
    networks:
      dhcp:
        mac_address: de:ad:be:ef:00:01
networks:
  dhcp:
    name: dbrv100
    external: true

@robmry
Copy link
Contributor Author

robmry commented Mar 4, 2024

Will this fix watchtower without them having to do anything or will they still have to do something for this to work properly?

I use mac_address: on about 30 containers with v25.0.3 docker compose they get a static MAC just fine. When watchtower runs it gives them random MACs.

In the latest 25.0.3 mac_address needs to be under networks or it fails to set it.

Hi @NonaSuomy ... the intention of the original change was to migrate the MAC address from top level config to endpoint config, for any version of the API. And, when an older version of the API is used, to continue to ignore a MAC address supplied in endpoint configuration. This PR doesn't change that intention, it should just stop us from losing track of settings when a mix of network names and ids are used in the API request. It also drops the per-endpoint config for the old API even when no container-wide setting is supplied.

I think this sounds like a different issue, perhaps related to #47233 but I'm not sure.

If you can catch logs for a failed update, as well as "docker inspect" for the container before and after - and pop them into a new moby issue, I'll take a look.

Instructions for enabling debug logging are at https://docs.docker.com/config/daemon/logs/#enable-debugging ... when the container's updated, you should see lines "Calling POST" - creating the container, then to disconnect and connect the networks, and finally to start the container. The "form data" log lines following each of those show the API request data.

Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

@vvoland vvoland requested a review from thaJeztah March 4, 2024 17:28
@akerouanton akerouanton merged commit 7c7e453 into moby:master Mar 6, 2024
126 checks passed
@robmry robmry deleted the 47441_mac_addr_config_migration branch March 27, 2024 16:35
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.

Docker 25.03 - Updating containers results in IP Error
5 participants