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.

Sign up for GitHub

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

DHCP6: lastlease behavior after Confim non-response #387

Merged
merged 1 commit into from
Oct 29, 2024
+6 −1
Merged

DHCP6: lastlease behavior after Confim non-response #387

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/dhcp6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ static const char * const dhcp6_statuses[] = {

static void dhcp6_bind(struct interface *, const char *, const char *);
static void dhcp6_failinform(void *);
static void dhcp6_startrebind(void *arg);
static void dhcp6_recvaddr(void *, unsigned short);
static void dhcp6_startdecline(struct interface *);
static void dhcp6_startrequest(struct interface *);
Expand Down Expand Up @@ -1760,8 +1761,12 @@ 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 || state->state == DH6S_REBIND) &&
ifp->options->options & DHCPCD_LASTLEASE) {
dhcp6_bind(ifp, NULL, NULL);
state->state = DH6S_REBIND;
dhcp6_startrebind(ifp);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return?

We couldn't find any DHCPv6 server, so we do use the last lease, but we should retry and DISCOVER yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok here.

} else if (state->new) {
script_runreason(ifp, "TIMEOUT6");
// We need to keep the expire timeout alive
Expand Down
Loading