DHCP6: lastlease behavior after Confim non-response #387
Conversation
| dhcp6_bind(ifp, NULL, NULL); | ||
| return; |
There was a problem hiding this comment.
Why return?
We couldn't find any DHCPv6 server, so we do use the last lease, but we should retry and DISCOVER yes?
There was a problem hiding this comment.
Soliciting for a lease while using the current one could lead to problems if the DHCPv6 server has ping checking enabled, in the case where you come back up on the same network. We've seen it fail the ping check (ie, gets a response) and issue a new IP address when that happens, thinking there's an address conflict. I think making this behavior optional with the lastlease option allows everybody to choose which way they want to go, since it is kind of a couple of competing requirements we're trying to untangle here. What do you think?
There was a problem hiding this comment.
Does it matter it gets a new IP address? I mean, we have just failed to CONFIRM it?
This behavior is long standing in the DHCPv4 side of things and am just trying to get the same behavior both sides.
There was a problem hiding this comment.
For the product I work on, it would for sure. We've had customer complaints due to address changes like this in the past. It wouldn't fly for us at all.
There was a problem hiding this comment.
Well, DHCP address can and do change. Shouldn't count on them being static. Even if you get a new address, the old one will still persist here.
Anyway, how would you feel instead with moving to the REBIND state immediately then?
There was a problem hiding this comment.
That sounds like a very good idea. Let me try it out against the Ready Logo tests to make sure it doesn't cause any unexpected behaviors, though I don't really expect it to, and I'll update this PR.
Is this the right way to do it?
dhcp6_fail()
} else if (state->state == DH6S_CONFIRM && ifp->options->options & DHCPCD_LASTLEASE) {
dhcp6_bind(ifp, NULL, NULL);
state->state = DH6S_REBIND;
dhcp6_startrebind(ifp);
return;
}
src/dhcp6.c
Outdated
| @@ -1760,8 +1760,9 @@ dhcp6_fail(struct interface *ifp, bool drop) | |||
| dhcp_unlink(ifp->ctx, state->leasefile); | |||
| dhcp6_addrequestedaddrs(ifp); | |||
| eloop_timeout_delete(ifp->ctx->eloop, NULL, ifp); | |||
| } else if (state->recv && ifp->options->options & DHCPCD_LASTLEASE) { | |||
| } else if (state->state == DH6S_CONFIRM && ifp->options->options & DHCPCD_LASTLEASE) { | |||
There was a problem hiding this comment.
Can we also allow the state DH6S_REBIND please?
IF you ask for a Prefix Delegation you REBIND, otherwise CONFIRM and ideally we want the behavior in both states.
And we should still check state->recv to ensure we ARE using the last lease in this state.
There was a problem hiding this comment.
Sure, I'm not seeing any issues with those changes in my own testing.
There was a problem hiding this comment.
I still feel we should check if we have recv still. Why did you remove it?
There was a problem hiding this comment.
I've got it in my local branch, just haven't pushed it yet. It'll be correct in my next push along with the rebind change.
There was a problem hiding this comment.
Testing this some more, I'm seeing situations where state->recv is null when it's in the CONFIRM state. It looks like in dhcp6_bind, it gets set to null after the Solicit/Adv/Request/Reply transaction. So if the link bounces, and dhcpcd sends a Confirm the check fails. I see the same behavior if I restart dhcpcd while it has a valid lease. Is that expected?
There was a problem hiding this comment.
OK, leave it then and I'll have a look at it later as it won't work with Prefix Delegation as that will be in the REBIND state but with CONFIRM timings - it's silly, but that's the RFC.
There was a problem hiding this comment.
Thanks! I've updated the commit now.
jvfranklin
force-pushed
the
lastlease3
branch
from
e7370f8
to
a28b875
Compare
If lastlease is enabled, and dhcpcd is unable to confirm its prior lease, after timeout, bind the lease and move to the REBIND state. Confine lastlease behavior to the CONFIRM and REBIND states.
jvfranklin
force-pushed
the
lastlease3
branch
from
a28b875
to
eee603b
Compare
If lastlease is enabled, and dhcpcd is unable to confirm its prior lease, after timeout, don't continue to solicit. Confine lastlease behavior to only the CONFIRM state.
This cleans up some minor issues with the initial commit for DHCPv6 lastlease.