Opened 11 months ago

Closed 5 months ago

#5404 closed enhancement (complete)

implement relay port

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea1.4
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The I-D is in its final phase so IANA should assign option codes just in time for 1.4...

Subtickets

Change History (18)

comment:1 Changed 10 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4
  • Priority changed from medium to low

The draft is expected to go through IESG by end of Nov.

We could implement this in 1.4 due to parity with ISC DHCP, which will get this feature in 4.4.0.

comment:2 Changed 9 months ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

Doing the code in parallel with ISC DHCP (easier for Kea as it is server only).

comment:3 Changed 9 months ago by fdupont

Code done. Keeping the ticket until the ISC DHCP review is done.

comment:4 Changed 9 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

ISC DHCP code was merged. Note I didn't add the DHCPv4-over-DHCPv6 stuff here believing that Kea 4o6 doesn't support relays but in fact it does so:

1- first option: give the ticket back so I add the 4o6 + relay port support
2- create a child of this ticket in 1.4
3- create a child of this ticket in proposed
4- give up (i.e. create a child of this ticket in outstanding)

Ready for review (and tested with ISC DHCP, cf RT 44535).

comment:5 Changed 9 months ago by fdupont

Finally I took benefit of the DHCPv4-over-DHCPv6 + relay port testbed to finish and test the 4o6 support.
Still ready for review.

comment:6 follow-up: Changed 9 months ago by fdupont

Before merge update comments about 19 and 135 code points (IANA assigned but no RFC number yet).

comment:7 Changed 6 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:8 follow-up: Changed 6 months ago by tmark

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc -
TEST_F(Dhcpv4SrvTest, adjustIfaceDataRelayPort)

  1. It might be good to verify the original values are as expected

prior to calling NakedDhcpv4Srv::adjustIfaceData(ex);

  1. The following comment appears to be wrong, it says the port must be 67,

yet you're testing for the remote port value

+ The query has been relayed, so the response must be sent to the port 67.
+ EXPECT_EQ(1234, resp->getRemotePort());


src/bin/dhcp6/dhcp6_srv.*

testRelaySourcePort() - the v4 analog is called "checkRelayPort" where this
is called "testRelaySourcePort", perhaps for symmetry you could call this one
"checkRealySourcePort"?


You should probably coordinate with Wlodek on creating Forge tests for this.

Have you done any system testing with this?

comment:9 Changed 6 months ago by tmark

  • Owner changed from tmark to fdupont

comment:10 in reply to: ↑ 6 Changed 6 months ago by fdupont

Replying to fdupont:

Before merge update comments about 19 and 135 code points (IANA assigned but no RFC number yet).

RFC 8357 with the right code points.

comment:11 in reply to: ↑ 8 Changed 6 months ago by fdupont

Replying to tmark:

=> I was a bit light on the tests: the effort was on the ISC DHCP code, ie
on the relay code....

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc -
TEST_F(Dhcpv4SrvTest, adjustIfaceDataRelayPort)

  1. It might be good to verify the original values are as expected

prior to calling NakedDhcpv4Srv::adjustIfaceData(ex);

=> good idea but as the value is copied from the request I prefer to
set it on 67 so the side effect of adjustIfaceData is more visible.

  1. The following comment appears to be wrong, it says the port must be 67,

yet you're testing for the remote port value

+ The query has been relayed, so the response must be sent to the port 67.
+ EXPECT_EQ(1234, resp->getRemotePort());

=> oops, the comment must be updated.

src/bin/dhcp6/dhcp6_srv.*

testRelaySourcePort() - the v4 analog is called "checkRelayPort" where this
is called "testRelaySourcePort", perhaps for symmetry you could call this one
"checkRealySourcePort"?

=> I think you mean checkRelaySourcePort. Done.

You should probably coordinate with Wlodek on creating Forge tests for this.

=> now that the RFC was published it is something we can do but I propose
to discuss it because I don't know the priority (low or very low IMHO).

Have you done any system testing with this?

=> yes and it is not trivial because the complex case uses 2 relays so 4 VMs.
Fortunately the server side is simple...

I take the occasion to update comments referring to the draft.
When it will be finished I'll give you back the ticket.

comment:12 Changed 6 months ago by fdupont

  • Owner changed from fdupont to tmark

Ready for final review if you'd like.

comment:13 Changed 6 months ago by tmark

  • Owner changed from tmark to fdupont

Changes are fine. Merge at will.

comment:14 Changed 6 months ago by fdupont

  • Owner changed from fdupont to UnAssigned

Unfortunately as this changed the DHCPv4-over-DHCPv6 inter-server protocol I believe a ChangeLog is required.
What about:
Added RFC 8357 "Generalized UDP Source Port for DHCP Relay" support for DHCPv4, DHCPv6 and DHCPv4-over-DHCPv6 (RFC 7341, in a not backward compatible way).
As soon as the wording is good enough (so please improve it) I'll merge.

comment:15 follow-up: Changed 6 months ago by tomek

  • Owner changed from UnAssigned to fdupont

Did this change update user's guide sections 8.10 and 9.15?

Proposed changelog:

13xx.	[func]		fdupont
	Added support for generalized UDP Source Port for DHCP Relay
	(RFC 8357) for DHCPv4, DHCPv6 and DHCPv4-over-DHCPv6. Note
	the change in DHCPv4-over-DHCPv6 is not backward compatible.
	(Trac #5404, git tbd)

Could you expand upon the last sentence? How is it not backwardly compatible?

Last edited 6 months ago by tmark (previous) (diff)

comment:16 in reply to: ↑ 15 ; follow-up: Changed 6 months ago by fdupont

  • Owner changed from fdupont to UnAssigned

Replying to tomek:

Did this change update user's guide sections 8.10 and 9.15?

=> in fact no because we do not have a section of DHCP standards,
only one for DHCPv4 and another one for DHCPv6...

Proposed changelog:

13xx.	[func]		fdupont
	Added support for generalized UDP Source Port for DHCP Relay
	(RFC 8357) for DHCPv4, DHCPv6 and DHCPv4-over-DHCPv6. Note
	the change in DHCPv4-over-DHCPv6 is not backward compatible.
	(Trac #5404, git tbd)

Could you expand upon the last sentence? How is it not backwardly compatible?

=> DHCPv4-over-DHCPv6 uses a private inter-server protocol which was
changed. Perhaps it is only confusing and as DHCPv4-over-DHCPv6 support
is not heavily used (I think it is marked as experimental) it is not a real issue.

So I have a simpler proposal:
Added support for generalized UDP Source Port for DHCP Relay (RFC 8357).

Should be enough for a ChangeLog entry.

comment:17 in reply to: ↑ 16 Changed 6 months ago by tmark

  • Owner changed from UnAssigned to fdupont

Replying to fdupont:

Replying to tomek:

Did this change update user's guide sections 8.10 and 9.15?

=> in fact no because we do not have a section of DHCP standards,
only one for DHCPv4 and another one for DHCPv6...

Proposed changelog:

13xx.	[func]		fdupont
	Added support for generalized UDP Source Port for DHCP Relay
	(RFC 8357) for DHCPv4, DHCPv6 and DHCPv4-over-DHCPv6. Note
	the change in DHCPv4-over-DHCPv6 is not backward compatible.
	(Trac #5404, git tbd)

Could you expand upon the last sentence? How is it not backwardly compatible?

=> DHCPv4-over-DHCPv6 uses a private inter-server protocol which was
changed. Perhaps it is only confusing and as DHCPv4-over-DHCPv6 support
is not heavily used (I think it is marked as experimental) it is not a real issue.

So I have a simpler proposal:
Added support for generalized UDP Source Port for DHCP Relay (RFC 8357).

Should be enough for a ChangeLog entry.

Ok, so Kea 1.3 4o6 is not compatible with Kea 1.4 4o6. I belive we have to state this in the ChangeLog?, whether its widely used or not. Perhaps just a bit more information, like
this?

    Added support for generalized UDP Source Port for DHCP Relay
    (RFC 8357) for DHCPv4, DHCPv6 and DHCPv4-over-DHCPv6. Note
    this required changes to the inter-server protocol used by
    our 4o6 implementation, and is therefore not backwardly
    compatible.

Is 4o6 documented in the Admin guide anywhere? If so that would most definitely need to be updated.

comment:18 Changed 5 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. 4o6 is documented with a note saying both servers must run the same version. I added a statement giving this as an example of changes...

Note: See TracTickets for help on using tickets.