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

Feature: Add mac_address parameter #293

Open
the-maldridge opened this issue Oct 22, 2023 · 5 comments · May be fixed by #359
Open

Feature: Add mac_address parameter #293

the-maldridge opened this issue Oct 22, 2023 · 5 comments · May be fixed by #359

Comments

@the-maldridge
Copy link

The docker task driver allows specifying the container mac address. When combined with CNI networks this buidls a powerful ability to use administrator assigned MAC ranges to dynamically assign container addresses using DHCP.

If this would be accepted, I can send a patch.

@lgfa29
Copy link
Contributor

lgfa29 commented Nov 25, 2023

Hi @the-maldridge 👋

I think parity with the Docker driver whenever possible is always welcomed.

Thank you for the interest in working on this. Let us know if you have any question!

@the-maldridge
Copy link
Author

So the further into this I get the more I'm actually convinced that this is a bug that it isn't working already.

In api/structs.go the field StaticMAC is already defined, and should take a MAC address. From what I can ascertain, this is consumed in driver.go in Driver.StartTask() where when network is defined as bridge something should be putting the mac_address into the createOpts structure. There is no code that performs this mapping, or even reads from the mac_address field for that matter. It seems like in theory there should be something doing a mapping.

I'm not entirely sure why this is restricted to only bridge network mode, as it should work fine in anything except host mode where there would be a pre-existing conflicting address.

I'll see if I have cycles to get to this, but given the generally abysmal support for Podman, both in weird bugs I've noticed when using the driver in Nomad as well as trying to get a not-ancient version of it on Debian based systems, I've gone back to docker for the time being.

@lgfa29
Copy link
Contributor

lgfa29 commented Nov 28, 2023

Thank you for looking into this @the-maldridge.

I suspect Podman API v4 significantly modified the network container configuration to the point where the spec we currently have in api/structs.go no longer works for some container network configuration.

From their v4.0.0 Breaking Changes section:

The Podman APIs for Manifest List and Network operations have been completely rewritten to address issues and inconsistencies in the previous APIs. Incompatible APIs should warn if they are used with an older Podman client.

And looking at the latest content for their specs, the `StaticMAC field is marked as deprecated and only used to maintain database schema backwards compatibility:
https://github.com/containers/podman/blob/e4cdd4b35ad2dc617f5d33bb92abf2d433704ad4/libpod/container_config.go#L260-L261

So now the question becomes: how does it work now? And after looking into it a bit, I'm not sure 😞

The API reference seems to be broken as I can't really see the request schema from https://docs.podman.io/en/latest/_static/api.html#tag/containers/operation/ContainerCreateLibpod.

Looking at the OpenAPI source file (https://storage.googleapis.com/libpod-master-releases/swagger-latest.yaml) SpecGen points back to https://github.com/containers/podman/blob/main/pkg/specgen/specgen.go, which is where our api/structs.go file was sourced from, but it's the updated version where ContainerNetworkConfig does not have StaticMAC field.

But reading through the over values, we see this:

	// Map of networks names or ids that the container should join.
	// You can request additional settings for each network, you can
	// set network aliases, static ips, static mac address  and the
	// network interface name for this container on the specific network.
	// If the map is empty and the bridge network mode is set the container
	// will be joined to the default network.
	Networks map[string]nettypes.PerNetworkOptions

https://github.com/containers/podman/blob/e4cdd4b35ad2dc617f5d33bb92abf2d433704ad4/pkg/specgen/specgen.go#L458-L464

So it seems like static MAC addresses should be set in the Networks map?

At this point I gave up trying to understand the API and went looking for how it's consumed by the podman container run command and indeed the --mac-address flag does seem to be mapped to Networks:

https://github.com/containers/podman/blob/e4cdd4b35ad2dc617f5d33bb92abf2d433704ad4/cmd/podman/common/netflags.go#L250-L263

And yes, this option does seem restricted to only bridge network mode, but I'm not sure why and this requirement doesn't seem to be documented or explained anywhere.

So, I think the way to get this to work would be:

  1. Add that Networks field to api/structs.go.
  2. Add PerNetworkOptions struct to api/structs.go, which, I think, comes from here?
    https://github.com/containers/common/blob/3cec82a3710580e7e545b23c47534f5cb2682917/libnetwork/types/network.go#L246-L263
  3. Create a new task configuration for mac_address.

And now is the weird part. We try to keep as much compatibility as possible, so we would need to detect which version of Podman is being used. Luckily this is already done by the fingerprinter, so I think we could store that apiVersion into the driver itself so we can later access it in StartTask() (since fingerprints and tasks start concurrently, this value would need to be protected by a mutex).

func (d *Driver) buildFingerprint() *drivers.Fingerprint {
attrs := map[string]*pstructs.Attribute{}
// Ping podman api
apiVersion, err := d.podman.Ping(d.ctx)

Now that we know the Podman version being used we do one of the following:

4.a. If Podman version is 3.x, populate StaticMAC as you did.
4.b. If podman version is 4.x, create the Networks with a "default" key and a PerNetworkOptions with the static mac address set. This part I'm not totally sure would work like this, but I'm going by the CLI code again (https://github.com/containers/podman/blob/e4cdd4b35ad2dc617f5d33bb92abf2d433704ad4/cmd/podman/common/netflags.go#L206-L211)

Now, that's a lot of changes, and I'm probably missing some extra details, so I totally understand if you don't have the time to take on this.

Moreover, it may be good for us to figure out our Podman version compatibility and update to use the API v4, potentially dropping v3 support when incompatible (and hope V5 doesn't break too much stuff again...)

@the-maldridge
Copy link
Author

That's kind of what I was afraid of. I currently manage a bunch of podman containers via the ansible collection, and getting static MAC addresses to work in there involved descending pretty far into CNI mutating template parsers, so I'm not surprised that that complexity percolates all the way back up to the top.

I'm a little concerned about blindly setting up a default network unless there's some way to guarantee that in the context that gets parsed in it can only match the network that nomad expects, otherwise this might inadvertently attach the container to an extant and unexpected network.

I haven't got cycles to get that far into the API right now, and I'm largely reconsidering if the static MAC addresses are the right way to achieve what I'm doing anyway due to some really strange boundary conditions that occur when running the CNI dhcp driver (which was the only motivation for having known mac addresses in the first place). I'm increasingly considering just provisioning services behind a proxy which I can advertise via an internal bgp announcement to move its service VIP around. I'll chew on this some, but right now I'm increasingly thinking this will cause more problems than it solves.

@zandeez
Copy link

zandeez commented Jul 2, 2024

I needed Static IPv6 addressing (which I have sorted in a branch, PR to come shortly) #358 so I have added static MAC support as part of that.

zandeez pushed a commit to zandeez/nomad-driver-podman that referenced this issue Jul 2, 2024
…ress, static_ips and static_mac options for podman tasks.

Podman API version checks whether to set the properties directly or whether to use the new PerNetworkOptions framework.

ipv6_address and ipv4_address are provided for backwards compatibility, and for compatibility with docker. For Podman 4.0.0 these are merged into static_ips and sent as a PerNetorkOptions entry for the default network.
Added API version check, static mac support, new array-based option for static IPs to match the podman API

Trying moving the object to ContanerNetworkConfig
@zandeez zandeez linked a pull request Jul 2, 2024 that will close this issue
zandeez pushed a commit to zandeez/nomad-driver-podman that referenced this issue Jul 2, 2024
…ress, static_ips and static_mac options for podman tasks.

Podman API version checks whether to set the properties directly or whether to use the new PerNetworkOptions framework.

ipv6_address and ipv4_address are provided for backwards compatibility, and for compatibility with docker. For Podman 4.0.0 these are merged into static_ips and sent as a PerNetorkOptions entry for the default network.
Added API version check, static mac support, new array-based option for static IPs to match the podman API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants