Makes interface gateway configuration optional. #156
Conversation
jglogan
force-pushed
the
users/jglogan/single-gateway
branch
from
f72115e to
9a82cc0
Compare
| // Configure the gateway only on the first interface | ||
| if !self.interfaces.isEmpty { | ||
| try await agent.routeAddDefault(name: "eth0", gateway: self.interfaces[0].gateway) |
There was a problem hiding this comment.
I feel like we should just be configuring whatever is given to us here. Like if the network has a gateway on it, we configure it. There could be cases where we want all the gateways configured as routes and only a single one as the default gateway.
There was a problem hiding this comment.
That makes sense, thanks! IMO the default policy should prevent folks from shooting themselves in the foot with multiple default routes, but it makes sense that that policy should exist in the sandbox service and not here. I'll look into how to rework the PR to do that without blowing up the scope of this change.
There was a problem hiding this comment.
I agree with making the gateway optional and only configuring it when provided. That still is not a great design because of the "default" route. I don't know if we should add any additional information to the interface type that would denote the "default" route.
There was a problem hiding this comment.
Agree, let's do the minimum in this one and get the mechanism/policy split correct. We can consider how Interface evolves over time elsewhere.
jglogan
force-pushed
the
users/jglogan/single-gateway
branch
2 times, most recently
from
39e79ba to
a582c71
Compare
jglogan
changed the title
jglogan
force-pushed
the
users/jglogan/single-gateway
branch
3 times, most recently
from
8366de5 to
ae69e9c
Compare
jglogan
force-pushed
the
users/jglogan/single-gateway
branch
from
ae69e9c to
98b509e
Compare
crosbymichael
left a comment
crosbymichael
left a comment
There was a problem hiding this comment.
LGTM. Lets make sure we have an issue so we don't drop the second half of this work.
See discussion below for example. For multiple network interfaces in a single container we'll want to integrate against a containerization that includes apple/containerization#156. The change bumps the containerization dependency to 0.2.0 and addresses the breaking API changes. ```console % container network OVERVIEW: Manage container networks USAGE: container network <subcommand> OPTIONS: --version Show the version. -h, --help Show help information. SUBCOMMANDS: create Create a new network delete, rm Delete one or more networks list, ls List networks inspect Display information about one or more networks See 'container help network <subcommand>' for detailed help. ```
3 participants
Here we implement the mechanism for ensuring that
containerdoesn't set up two default routes on containers that connect to multiple networks. We'll implement the policy in apple/container#243.