- 
                Notifications
    
You must be signed in to change notification settings  - Fork 508
 
Add --mac-address flag to set custom MAC addresses for containers #753
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
Conversation
| 
           @DSS3113 Thank you! Got a bit of a backlog but let's get the workflow going and I'll try to look it over no later than early next week.  | 
    
| 
           @DSS3113 however, an immediate comment relating to the multiple networks aspect. What do you think about handling attachment properties similarly to mount options for filesystems? This has the attractive property of being extensible without cluttering the CLI with a bunch of options. The drawback is discovery/documentation isn't as simple; there's only so much you can include in the help for a single option. Example: container run --network default --network backend,mac=addr1 --network backend2,mac=addr2 | 
    
          
 @jglogan Sure, I can start adjusting it to match this format.  | 
    
| 
           @DSS3113 Building now but there are conflicts (probably because of the just merged  I'll also pull down a patch into my local and test it out, looking fwd to trying it!  | 
    
| 
           @jglogan Resolved merge conflicts!  | 
    
| 
           @DSS3113 I'm seeing the following with my local build: % container run -it --name foo --network default,mac=c1:2f:c2:fd:ce:0b alpine:latest sh
Warning! Running debug build. Performance may be degraded.
Error: internalError: "failed to bootstrap container" (cause: "internalError: "failed to bootstrap container foo (cause: "unknown: "internal error (13): ip-link-set: netlink response indicates error, rc = -99"")"")Are you seeing that it's working without issue on your system?  | 
    
| 
           Ah, that's a multicast address (I just generated a random one). Let me try with a different one...  | 
    
| 
           @DSS3113 Yep, looks good. Lemme take one last look at the code. Could you look at the one comment I posted above? % container run -it --rm --network default,mac=c2:2f:c2:fd:ce:0b alpine:latest ip link
Warning! Running debug build. Performance may be degraded.
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,UP,LOWER_UP> mtu 1280 qdisc pfifo_fast state UP qlen 1000
    link/ether c2:2f:c2:fd:ce:0b brd ff:ff:ff:ff:ff:ff
3: teql0: <NOARP> mtu 1500 qdisc noop state DOWN qlen 100
    link/void 
4: tunl0@NONE: <NOARP> mtu 1480 qdisc noop state DOWN qlen 1000
    link/ipip 0.0.0.0 brd 0.0.0.0
5: sit0@NONE: <NOARP> mtu 1480 qdisc noop state DOWN qlen 1000
    link/sit 0.0.0.0 brd 0.0.0.0
6: ip6tnl0@NONE: <NOARP> mtu 1452 qdisc noop state DOWN qlen 1000
    link/tunnel6 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 brd 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 | 
    
| 
           @DSS3113 Also it looks like some of your commits in this PR might not have verified sigs. Could you rebase your branch, reset it back to main, do a single squashed, verified commit, and force push that? I think we're good to go after that.  | 
    
2de8746    to
    bfab5ec      
    Compare
  
    | 
           @jglogan Thanks for reviewing! I've done the needful.  | 
    
f921339    to
    4652e2f      
    Compare
  
    | 
           Any idea why 4652e2f got pulled into this PR? That's already down in main.  | 
    
c03cce1    to
    a38bc9e      
    Compare
  
    | 
           @jglogan I believe I cherry-picked the commit and that created a duplicate. My bad, it should be fixed now.  | 
    
| 
           @DSS3113 It looks like there are   | 
    
dfcf7f7    to
    b8283f5      
    Compare
  
    | 
           @jglogan My sincere apologies! I think it should be good now.  | 
    
d41b25d    to
    9c9fa40      
    Compare
  
    9c9fa40    to
    a7b21ae      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, only now noticed some seemingly unnecessary diffs in command-reference.md, see comments.
| **Usage** | ||
| 
               | 
          ||
| ```bash | ||
| container run [<options>] <image> [<arguments> ...] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that there are a lot of changes compared to current main that don't need to be made, along with at least one regression. Could you fix this so that only the PR only updates the material relating to the MAC address? Apologies for the hassle!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for the trouble. I somehow missed these. Should be fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! I appreciate your persistence.
Not an excuse but a self-criticism 😄 I've been paging out of PR and issue reviews a lot and I fear it's cost contributors several more round trips than the PRs really need. Something for me to work on!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s alright, I understand! Thanks again!
        
          
                docs/command-reference.md
              
                Outdated
          
        
      | Builds an OCI image from a local build context. It reads a Dockerfile (default `Dockerfile`) or Containerfile and produces an image tagged with `-t` option. The build runs in isolation using BuildKit, and resource limits may be set for the build process itself. | ||
| 
               | 
          ||
| When no `-f/--file` is specified, the build command will look for `Dockerfile` first, then fall back to `Containerfile` if `Dockerfile` is not found. | ||
| Builds an OCI image from a local build context. It reads a Dockerfile (default `Dockerfile`) and produces an image tagged with `-t` option. The build runs in isolation using BuildKit, and resource limits may be set for the build process itself. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SAA.
4ed4ac9    to
    cd7a14f      
    Compare
  
    This commit adds support for customizing MAC addresses for containers.
cd7a14f    to
    c468fa9      
    Compare
  
    | } | ||
| 
               | 
          ||
| let hostname = try message.hostname() | ||
| let macAddress = message.string(key: NetworkKeys.macAddress.rawValue) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note to myself and any interested parties here, no change required for this PR.
@dcantah reworked the client, API server, and Sandbox Service so that the client only deals with the API server for sandbox management (instead of the doing bootstrap through API server and the rest directly with the sandbox service) for the 0.5.0 release. This gives us the opportunity to make the API server responsible for allocating IP addresses at container start, and freeing the allocations when a container stops for any reason. Currently if container-linux-runtime gets terminated (SIGKILL) or faults, the IP will be orphaned.
When we make that change we might want simplify this by only passing what's needed for IP allocation and hostname resolution, and see if we can take the returned Attachment and integrate it with the MAC address and other caller-specified properties not related to the core network service functions to produce and Attachment that we can transfer over to the sandbox service at container start time. We trade a simpler interface between NetworkService and its caller for a little more complexity creating the Attachment that contains the user-specified network attributes, the allocated attributes, and the vmnet_network_ref.
Type of Change
Motivation and Context
[Why is this change needed?]
Currently, there is no way to specify a custom MAC address for a container's network interface and the MAC address is auto-generated by the system.
Use Cases
Testing
Issue
closes #752