Skip to content

Commit

Permalink
[#3049] Addressed review comments
Browse files Browse the repository at this point in the history
Spelling, clean ups, doxygen
Split DdnsParams out to its own files:
	new file:   src/lib/dhcpsrv/ddns_params.cc
	new file:   src/lib/dhcpsrv/ddns_params.h
  • Loading branch information
tmarkwalder committed Jan 22, 2025
1 parent b9372f4 commit d202904
Show file tree
Hide file tree
Showing 25 changed files with 429 additions and 367 deletions.
2 changes: 1 addition & 1 deletion src/bin/dhcp4/tests/fqdn_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3100,7 +3100,7 @@ TEST_F(NameDhcpv4SrvTest, ddnsTtlMaxTest) {
TEST_F(NameDhcpv4SrvTest, poolDdnsParametersTest) {
// A configuration with following pools:
// 1. Specifies a qualifying suffix
// 2. Specifes no DDNS parameters
// 2. Specifies no DDNS parameters
// 3. Disables DDNS updates
// 4. Specifies a qualifying suffix but disables DDNS updates
std::string config = R"(
Expand Down
8 changes: 4 additions & 4 deletions src/bin/dhcp6/dhcp6_srv.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5093,11 +5093,11 @@ Dhcpv6Srv::checkPostAssignmentChanges(const Pkt6Ptr& question, Pkt6Ptr& answer,
}
}

// Check if the subnet was dynamnically changed by the allocation engine.
// Check if the subnet was dynamically changed by the allocation engine.
if (ctx.subnet_ && orig_subnet && (orig_subnet->getID() != ctx.subnet_->getID())) {
// We get the network for logging only. It should always be set as this a dynamic
// change should only happen within shared-networks. Not having one might not be
// an error if a hook changed the subnet?
// We get the network for logging only. It should always be set as
// this a dynamic change should only happen within shared-networks.
// Not having one might not be an error if a hook changed the subnet?
SharedNetwork6Ptr network;
orig_subnet->getSharedNetwork(network);
LOG_DEBUG(packet6_logger, DBG_DHCP6_BASIC_DATA, DHCP6_SUBNET_DYNAMICALLY_CHANGED)
Expand Down
2 changes: 1 addition & 1 deletion src/bin/dhcp6/dhcp6_srv.h
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,7 @@ class Dhcpv6Srv : public process::Daemon {
///
/// Updates the context DDNS parameters to include those from the pool
/// associated with the first active NA lease address and then checks
/// to see if the subnet has been dynamicaly changed. If either the
/// to see if the subnet has been dynamically changed. If either the
/// pool has DDNS parameters or the subnet has changed the FQDN and
/// DDNS flags are recalculated in the event the pool or subnet change
/// introduced different parameter values otherwise the function returns.
Expand Down
47 changes: 27 additions & 20 deletions src/bin/dhcp6/tests/fqdn_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@ class FqdnDhcpv6SrvTest : public Dhcpv6SrvTest {
///
/// @param iaid IAID
/// @param pkt A DHCPv6 message to which the IA option should be added.
/// @param cxt allocation engine context to which IA option should be
/// added.
/// @param cxt allocation engine context to which IA option should be
/// added.
void addIA(const uint32_t iaid, const IOAddress& addr, Pkt6Ptr& pkt,
AllocEngine::ClientContext6& ctx) {
Option6IAPtr opt_ia = generateIA(D6O_IA_NA, iaid, 1500, 3000);
Expand All @@ -275,8 +275,8 @@ class FqdnDhcpv6SrvTest : public Dhcpv6SrvTest {
/// @param iaid IAID
/// @param status_code Status code
/// @param pkt A DHCPv6 message to which the option should be added.
/// @param cxt allocation engine context to which IA option should be
/// added.
/// @param cxt allocation engine context to which IA option should be
/// added.
void addIA(const uint32_t iaid, const uint16_t status_code, Pkt6Ptr& pkt,
AllocEngine::ClientContext6& ctx) {
Option6IAPtr opt_ia = generateIA(D6O_IA_NA, iaid, 1500, 3000);
Expand Down Expand Up @@ -696,7 +696,7 @@ class FqdnDhcpv6SrvTest : public Dhcpv6SrvTest {
ASSERT_TRUE(pool);
}

/// @brief Verifies that DDNS TTL parameters are used when specified.
/// @brief Verifies that DDNS TTL parameters are used when specified.
///
/// @param valid_flt lease life time
/// @param ddns_ttl_percent expected configured value for ddns-ttl-percent
Expand Down Expand Up @@ -725,7 +725,7 @@ class FqdnDhcpv6SrvTest : public Dhcpv6SrvTest {

// Create an IA.
Option6IAPtr opt_ia = generateIA(D6O_IA_NA, 1234, 1500, 3000);
Option6IAAddrPtr opt_iaaddr(new Option6IAAddr(D6O_IAADDR,
Option6IAAddrPtr opt_iaaddr(new Option6IAAddr(D6O_IAADDR,
IOAddress("2001:db8:1::1"),
valid_lft, valid_lft));
opt_ia->addOption(opt_iaaddr);
Expand Down Expand Up @@ -2239,15 +2239,15 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsTtlTest) {
testDdnsTtlParameters(2100, // valid lft
Optional<double>(), // percent
999, // ttl
Optional<uint32_t>(), // min
Optional<uint32_t>(), // min
Optional<uint32_t>()); // max
}

// Verify the ddns-ttl-min is used when specified.
TEST_F(FqdnDhcpv6SrvTest, ddnsTtlMinTest) {
testDdnsTtlParameters(2100, // valid lft
Optional<double>(), // percent
Optional<uint32_t>(), // ttl
Optional<uint32_t>(), // ttl
800, // ttl-min
Optional<uint32_t>()); // ttl-max
}
Expand All @@ -2256,19 +2256,19 @@ TEST_F(FqdnDhcpv6SrvTest, ddnsTtlMinTest) {
TEST_F(FqdnDhcpv6SrvTest, ddnsTtlMaxTest) {
testDdnsTtlParameters(2100, // valid lft
Optional<double>(), // percent
Optional<uint32_t>(), // ttl
Optional<uint32_t>(), // ttl
Optional<uint32_t>(), // ttl-min
500); // ttl-max
500); // ttl-max
}

// Verify pool-level DDNS pararmeters are used.
// Verify pool-level DDNS parameters are used.
// We don't verify all of them, just enough
// enough to ensure proper scoping of values.
TEST_F(FqdnDhcpv6SrvTest, poolDdnsParametersTest) {

// A configuration with following pools:
// 1. Specifies a qualifying suffix
// 2. Specifes no DDNS parameters
// 2. Specifies no DDNS parameters
// 3. Disables DDNS updates
// 4. Specifies a qualifying suffix but disables DDNS updates
std::string config = R"(
Expand Down Expand Up @@ -2358,18 +2358,23 @@ TEST_F(FqdnDhcpv6SrvTest, poolDdnsParametersTest) {
query->setIndex(ETH0_INDEX);

// Add the client id.
OptionPtr client_id(new Option(Option::V6, D6O_CLIENTID, scenario.raw_duid_));
OptionPtr client_id(new Option(Option::V6, D6O_CLIENTID,
scenario.raw_duid_));
query->addOption(client_id);
query->addOption(srv_->getServerID());

// Add an IA requesting the expected address.
Option6IAPtr ia = generateIA(D6O_IA_NA, 234, 1500, 3000);
OptionPtr hint_opt(new Option6IAAddr(D6O_IAADDR, scenario.expected_address_, 300, 500));
OptionPtr hint_opt(new Option6IAAddr(D6O_IAADDR,
scenario.expected_address_,
300, 500));
ia->addOption(hint_opt);
query->addOption(ia);

// Add an FQDN option. We set it to partial to ensure we qualify it with a suffix.
query->addOption(createClientFqdn(Option6ClientFqdn::FLAG_S, scenario.client_fqdn_,
// Add an FQDN option. We set it to partial to ensure we qualify
// it with a suffix.
query->addOption(createClientFqdn(Option6ClientFqdn::FLAG_S,
scenario.client_fqdn_,
Option6ClientFqdn::PARTIAL));

// Process the REQUEST.
Expand All @@ -2392,16 +2397,18 @@ TEST_F(FqdnDhcpv6SrvTest, poolDdnsParametersTest) {

// Check that we got the address we requested.
tmp = ia->getOption(D6O_IAADDR);
boost::shared_ptr<Option6IAAddr> addr = boost::dynamic_pointer_cast<Option6IAAddr>(tmp);
boost::shared_ptr<Option6IAAddr> addr
= boost::dynamic_pointer_cast<Option6IAAddr>(tmp);
ASSERT_TRUE(addr);
EXPECT_EQ(addr->getAddress(), scenario.expected_address_);

// Check that the lease exists with the correct FDQN.
Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, addr->getAddress());
Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA,
addr->getAddress());
ASSERT_TRUE(lease);
EXPECT_EQ(lease->hostname_, scenario.expected_fqdn_);

// Verfiy the FQDN in the response is correct.
// Verify the FQDN in the response is correct.
Option6ClientFqdnPtr fqdn;
ASSERT_TRUE(fqdn = boost::dynamic_pointer_cast<
Option6ClientFqdn>(reply->getOption(D6O_CLIENT_FQDN)));
Expand All @@ -2415,7 +2422,7 @@ TEST_F(FqdnDhcpv6SrvTest, poolDdnsParametersTest) {
verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
scenario.expected_address_.toText(),
scenario.expected_dhcid_,
0, 4000,
0, 4000,
scenario.expected_fqdn_);
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/lib/dhcpsrv/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ libkea_dhcpsrv_la_SOURCES += timer_mgr.cc timer_mgr.h
libkea_dhcpsrv_la_SOURCES += tracking_lease_mgr.cc tracking_lease_mgr.h
libkea_dhcpsrv_la_SOURCES += utils.h
libkea_dhcpsrv_la_SOURCES += writable_host_data_source.h
libkea_dhcpsrv_la_SOURCES += ddns_params.cc ddns_params.h

# Configuration parsers
libkea_dhcpsrv_la_SOURCES += parsers/base_network_parser.cc
Expand Down Expand Up @@ -348,7 +349,8 @@ libkea_dhcpsrv_include_HEADERS = \
subnet_selector.h \
timer_mgr.h \
utils.h \
writable_host_data_source.h
writable_host_data_source.h \
ddns_params.h

if FUZZING
libkea_dhcpsrv_include_HEADERS += \
Expand Down
2 changes: 1 addition & 1 deletion src/lib/dhcpsrv/cfg_hosts_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void CfgHostsList::internalize(ConstElementPtr list) {
isc_throw(BadValue, "internal error: CfgHostsList::internalize: "
"no reservations for subnet ID " << subnet_id);
}
map_.insert(std::make_pair(subnet_id,
map_.insert(std::make_pair(subnet_id,
boost::const_pointer_cast<Element>(resvs)));
}
}
Expand Down
30 changes: 15 additions & 15 deletions src/lib/dhcpsrv/database_backends.dox
Original file line number Diff line number Diff line change
Expand Up @@ -128,26 +128,26 @@

- <b>address</b> - IPv4 address \n
type: string \n
example: 192.0.2.1
example: 192.0.2.1

- <b>hwaddr</b> - hardware address (without the hardware type) \n
type: string \n
- <b>hwaddr</b> - hardware address (without the hardware type) \n
type: string \n
example: 00:01:02:03:04:05
- <b>client_id</b> - client identifier \n

- <b>client_id</b> - client identifier \n
type: string \n
example: 01:02:03:03:04:aa:bb:cc

- <b>valid_lifetime</b> - valid lifetime \n
type: uint32_t \n
type: uint32_t \n
example: 200

- <b>expire</b> - expiration date \n
type: uint64_t (cltt + valid lifetime) \n
example: 1733517739

- <b>subnet_id</b> - DHCPv4 subnet identifier \n
type: int32_t (0 and max excluded) \n
- <b>subnet_id</b> - DHCPv4 subnet identifier \n
type: int32_t (0 and max excluded) \n
example: 1

- <b>fqdn_fwd</b> - FQDN forward DNS RR update flag \n
Expand All @@ -161,17 +161,17 @@
- <b>hostname</b> - hostname \n
type: string (separators are escaped) \n
example: foo.bar

- <b>state</b> - lease state \n
type: int32_t (0 = assigned, 1 = declined, 2 = expired-reclaimed, 3 = released) \n
example: 0

- <b>user_context</b> - user context \n
type: string (separators are escaped) \n
type: string (separators are escaped) \n
example: <tt>{ \"foo\": true }</tt>

- <b>pool_id</b> - pool identifier \n
type: uint32_t \n
- <b>pool_id</b> - pool identifier \n
type: uint32_t \n
example: 0

for instance:
Expand Down Expand Up @@ -244,11 +244,11 @@
example: <tt>{ \"foo\": true }</tt>

- <b>hwtype</b> - hardware type \n
type: uint16_t (can be empty/omitted) \n
type: uint16_t (can be empty/omitted) \n
example: 1

- <b>hwaddr_source</b> - source of hardware address and type \n
tyoe: uint32_t (one bit from an 8 bit mask, can be empty/omitted) \n
tyoe: uint32_t (one bit from an 8 bit mask, can be empty/omitted) \n
example: 0

- <b>pool_id</b> - pool identifier \n
Expand All @@ -259,7 +259,7 @@
\verbatim
2001:db8::1,00:01:02:03:04:05:06:0f,200,800,8,100,0,7,128,1,1,,,1,{ \"foo\": true },,,0\n
\endverbatim

More examples can be found in unit tests.

@section dhcpdb-host Host Backends
Expand Down
Loading

0 comments on commit d202904

Please sign in to comment.