Add container ifName to the dhcp clientID, making the clientID value #217
Conversation
plugins/ipam/dhcp/daemon.go
Outdated
| if !ok { | ||
| return nil | ||
| } | ||
| return l | ||
| } | ||
|
|
||
| func (d *DHCP) setLease(contID, netName string, l *DHCPLease) { | ||
| //func (d *DHCP) setLease(contID, netName string, ifName string, l *DHCPLease) { |
plugins/ipam/dhcp/daemon.go
Outdated
| @@ -50,6 +50,12 @@ func newDHCP() *DHCP { | |||
| } | |||
| } | |||
|
|
|||
| func generateClientID(containerID string, netName string, ifName string) string { | |||
| clientID := containerID + "/" + netName + "/" + ifName | |||
There was a problem hiding this comment.
suppose we just make it one line as
return containerID + "/" + netName + "/" + ifName
|
On further inspection we figured out that the DHCP client code doesn't actually pass the Client ID in the packet, so even though we've fixed the internal code we have more to do. Related: d2g/dhcp4client#24 |
now equal to: ContainerID / Host Network / ifname inside container Use clientID as a key for the lease
plugins/ipam/dhcp/client.go
Outdated
|
|
||
| /* | ||
| //Create Discover Packet | ||
| func DhcpDiscoverPacket(c *dhcp4client.Client, options dhcp4.Options) dhcp4.Packet { |
There was a problem hiding this comment.
I'd just remove all the commented-out code.
|
@mccv1r0 thanks for the update; one small nit otherwise LGTM |
plugins/ipam/dhcp/dhcp_test.go
Outdated
| var br *netlink.Bridge | ||
|
|
||
| BeforeEach(func() { | ||
| dhcpServerStopCh = make(chan bool) |
There was a problem hiding this comment.
The next 100+ lines look very similar to lines 49 onwards in dhcp2_test.go - if they are different please add some comments indicating what is going on, otherwise pull out the common code to a shared setup function.
There was a problem hiding this comment.
I created a shared setup function as suggested
…le CNI interfaces using dhcp ipam. Vendor latest dhcp4server, dhcp4client, dhcp4 Added additional tests for new functionality in dhcp2_test.go Wrap d2g dhcp4client calls with our own which add clientID to packet.
|
I was wondering what happens if someone was running the previous version, and they upgrade to this version: will the IDs sent to DHCP change? But @squeed worked out that the daemon doesn't persist this information so all the leases are forgotten on every restart so this isn't a new problem. |
now equal to: ContainerID / Host Network / ifname inside container
Use clientID as a key for the lease
Fixes #214