Commit 464ea71b authored by Thomas Markwalder's avatar Thomas Markwalder

[#42,!103] Addressed majority of review comments

    Most notable change is "queue-control" is now "dhcp-queue-control"
parent 8c8f4dd4
......@@ -637,7 +637,7 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) {
// Configure packet queue
try {
data::ConstElementPtr qc;
qc = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
qc = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl();
if (!qc) {
// @todo For now we're manually constructing default queue config
// This probably needs to be built into the PQM?
......
This diff is collapsed.
......@@ -706,7 +706,7 @@ ControlCharacterFill [^"\\]|\\{JSONEscapeSequence}
case isc::dhcp::Parser4Context::RESERVATIONS:
case isc::dhcp::Parser4Context::CLIENT_CLASSES:
case isc::dhcp::Parser4Context::CONTROL_SOCKET:
case isc::dhcp::Parser4Context::QUEUE_CONTROL:
case isc::dhcp::Parser4Context::DHCP_QUEUE_CONTROL:
case isc::dhcp::Parser4Context::LOGGERS:
case isc::dhcp::Parser4Context::DHCP_DDNS:
return isc::dhcp::Dhcp4Parser::make_USER_CONTEXT(driver.loc_);
......@@ -727,7 +727,7 @@ ControlCharacterFill [^"\\]|\\{JSONEscapeSequence}
case isc::dhcp::Parser4Context::RESERVATIONS:
case isc::dhcp::Parser4Context::CLIENT_CLASSES:
case isc::dhcp::Parser4Context::CONTROL_SOCKET:
case isc::dhcp::Parser4Context::QUEUE_CONTROL:
case isc::dhcp::Parser4Context::DHCP_QUEUE_CONTROL:
case isc::dhcp::Parser4Context::LOGGERS:
case isc::dhcp::Parser4Context::DHCP_DDNS:
return isc::dhcp::Dhcp4Parser::make_COMMENT(driver.loc_);
......@@ -1241,12 +1241,12 @@ ControlCharacterFill [^"\\]|\\{JSONEscapeSequence}
}
}
\"queue-control\" {
\"dhcp-queue-control\" {
switch(driver.ctx_) {
case isc::dhcp::Parser4Context::DHCP4:
return isc::dhcp::Dhcp4Parser::make_QUEUE_CONTROL(driver.loc_);
return isc::dhcp::Dhcp4Parser::make_DHCP_QUEUE_CONTROL(driver.loc_);
default:
return isc::dhcp::Dhcp4Parser::make_STRING("queue-control", driver.loc_);
return isc::dhcp::Dhcp4Parser::make_STRING("dhcp-queue-control", driver.loc_);
}
}
......
This diff is collapsed.
......@@ -460,7 +460,7 @@ namespace isc { namespace dhcp {
TOKEN_CONTROL_SOCKET = 367,
TOKEN_SOCKET_TYPE = 368,
TOKEN_SOCKET_NAME = 369,
TOKEN_QUEUE_CONTROL = 370,
TOKEN_DHCP_QUEUE_CONTROL = 370,
TOKEN_DHCP_DDNS = 371,
TOKEN_ENABLE_UPDATES = 372,
TOKEN_QUALIFYING_SUFFIX = 373,
......@@ -1082,7 +1082,7 @@ namespace isc { namespace dhcp {
static inline
symbol_type
make_QUEUE_CONTROL (const location_type& l);
make_DHCP_QUEUE_CONTROL (const location_type& l);
static inline
symbol_type
......@@ -2579,9 +2579,9 @@ namespace isc { namespace dhcp {
}
Dhcp4Parser::symbol_type
Dhcp4Parser::make_QUEUE_CONTROL (const location_type& l)
Dhcp4Parser::make_DHCP_QUEUE_CONTROL (const location_type& l)
{
return symbol_type (token::TOKEN_QUEUE_CONTROL, l);
return symbol_type (token::TOKEN_DHCP_QUEUE_CONTROL, l);
}
Dhcp4Parser::symbol_type
......
......@@ -170,7 +170,7 @@ using namespace std;
SOCKET_TYPE "socket-type"
SOCKET_NAME "socket-name"
QUEUE_CONTROL "queue-control"
DHCP_QUEUE_CONTROL "dhcp-queue-control"
DHCP_DDNS "dhcp-ddns"
ENABLE_UPDATES "enable-updates"
......@@ -450,7 +450,7 @@ global_param: valid_lifetime
| expired_leases_processing
| dhcp4o6_port
| control_socket
| queue_control
| dhcp_queue_control
| dhcp_ddns
| echo_client_id
| match_client_id
......@@ -1827,15 +1827,13 @@ control_socket_name: SOCKET_NAME {
};
// --- queue control ---------------------------------------------
// --- dhcp-queue-control ---------------------------------------------
// --- queue-control ---------------------------------------------
queue_control: QUEUE_CONTROL {
dhcp_queue_control: DHCP_QUEUE_CONTROL {
ctx.enter(ctx.NO_KEYWORD);
} COLON map_value {
ElementPtr qc = $4;
ctx.stack_.back()->set("queue-control", qc);
ctx.stack_.back()->set("dhcp-queue-control", qc);
if (!qc->contains("queue-type")) {
std::stringstream msg;
......
......@@ -24,7 +24,7 @@
#include <dhcpsrv/parsers/host_reservations_list_parser.h>
#include <dhcpsrv/parsers/ifaces_config_parser.h>
#include <dhcpsrv/parsers/option_data_parser.h>
#include <dhcpsrv/parsers/queue_control_parser.h>
#include <dhcpsrv/parsers/dhcp_queue_control_parser.h>
#include <dhcpsrv/parsers/simple_parser4.h>
#include <dhcpsrv/parsers/shared_networks_list_parser.h>
#include <dhcpsrv/parsers/sanity_checks_parser.h>
......@@ -381,9 +381,9 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set,
continue;
}
if (config_pair.first == "queue-control") {
QueueControlParser parser(AF_INET);
srv_cfg->setQueueControlInfo(parser.parse(config_pair.second));
if (config_pair.first == "dhcp-queue-control") {
DHCPQueueControlParser parser(AF_INET);
srv_cfg->setDHCPQueueControl(parser.parse(config_pair.second));
continue;
}
......
......@@ -174,8 +174,8 @@ Parser4Context::contextName()
return ("server-id");
case CONTROL_SOCKET:
return ("control-socket");
case QUEUE_CONTROL:
return ("queue-control");
case DHCP_QUEUE_CONTROL:
return ("dhcp-queue-control");
case POOLS:
return ("pools");
case RESERVATIONS:
......
......@@ -267,8 +267,8 @@ public:
/// Used while parsing Dhcp4/control-socket structures.
CONTROL_SOCKET,
/// Used while parsing Dhcp4/queue-control structures.
QUEUE_CONTROL,
/// Used while parsing Dhcp4/dhcp-queue-control structures.
DHCP_QUEUE_CONTROL,
/// Used while parsing Dhcp4/subnet4/pools structures.
POOLS,
......
......@@ -299,7 +299,11 @@ public:
void configure(std::string config, int expected_code,
std::string exp_error = "") {
ConstElementPtr json;
ASSERT_NO_THROW(json = parseDHCP4(config, true));
try {
json = parseDHCP4(config, true);
} catch(const std::exception& ex) {
ADD_FAILURE() << "parseDHCP4 failed: " << ex.what();
}
ConstElementPtr status;
EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
......@@ -6337,61 +6341,86 @@ TEST_F(Dhcp4ParserTest, serverTag) {
ASSERT_THROW(parseDHCP4(bad_tag), std::exception);
}
// Check whether it is possible to configure server-tag
TEST_F(Dhcp4ParserTest, queueControl) {
// Config without server-tag
string config_no_queue = "{ " + genIfaceConfig() + "," +
"\"subnet4\": [ ] "
"}";
string config_with_queue =
"{ " + genIfaceConfig() + ", \n" +
" \"subnet4\": [ ], \n"
" \"queue-control\": { \n"
" \"queue-type\": \"some-type\", \n"
" \"capacity\": 75 \n"
" } \n"
"} \n";
// Check whether it is possible to configure packet queue
TEST_F(Dhcp4ParserTest, dhcpQueueControl) {
struct Scenario {
std::string description_;
std::string json_;
};
string config_with_context =
"{ " + genIfaceConfig() + ", \n" +
" \"subnet4\": [ ], \n"
" \"queue-control\": { \n"
std::vector<Scenario> scenarios = {
{
"no entry",
""
},
{
"valid entry",
"{ \n"
" \"queue-type\": \"some-type\", \n"
" \"capacity\": 75 \n"
"} \n"
},
{
"valid arbitrary content",
"{ \n"
" \"queue-type\": \"some-type\", \n"
" \"capacity\": 90, \n"
" \"user-context\": { \"comment\": \"some text\" } \n"
" } \n"
"} \n";
" \"user-context\": { \"comment\": \"some text\" },\n"
" \"random-bool\" : false, \n"
" \"random-int\" : 1234 \n"
"} \n"
}
};
// Let's check the default. It should be empty.
data::ConstElementPtr control;
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
control = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl();
ASSERT_FALSE(control);
// Configuration with no queue should default to an emtpy control.
configure(config_no_queue, CONTROL_RESULT_SUCCESS, "");
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
ASSERT_FALSE(control);
// Iterate over the incorrect scenarios and verify they
// fail as expected. Note, we use parseDHCP4() directly
// as all of the errors above are enforced by the grammar.
data::ConstElementPtr exp_elems;
for (auto scenario : scenarios) {
SCOPED_TRACE(scenario.description_);
{
// Clear the config
CfgMgr::instance().clear();
// Clear the config
CfgMgr::instance().clear();
// Construct the config JSON
std::stringstream os;
os << "{ " + genIfaceConfig();
if (!scenario.json_.empty()) {
// Configuration with queue should be valid.
configure(config_with_queue, CONTROL_RESULT_SUCCESS, "");
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
ASSERT_TRUE(control);
os << ",\n \"dhcp-queue-control\": " << scenario.json_;
}
// Clear the config
CfgMgr::instance().clear();
os << "} \n";
// Configure the server. This should succeed.
configure(os.str(), CONTROL_RESULT_SUCCESS, "");
// Fetch the queue control info.
control = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl();
// Configuration with queue with context should be valid.
configure(config_with_context, CONTROL_RESULT_SUCCESS, "");
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
ASSERT_TRUE(control);
// If JSON does not contain queue control,
// the pointer stored in staging should be empty.
if (scenario.json_.empty()) {
ASSERT_FALSE(control);
continue;
}
// Make sure the staged config is correct.
ASSERT_TRUE(control);
ASSERT_NO_THROW(exp_elems = Element::fromJSON(scenario.json_))
<< " cannot convert expected JSON, test is broken";
EXPECT_TRUE(control->equals(*exp_elems));
}
}
}
// Check whether it is possible to configure server-tag
TEST_F(Dhcp4ParserTest, queueControlInvalid) {
// Check that we catch invalid dhcp-queue-control content
TEST_F(Dhcp4ParserTest, dhcpQueueControlInvalid) {
struct Scenario {
std::string description_;
std::string json_;
......@@ -6402,14 +6431,14 @@ TEST_F(Dhcp4ParserTest, queueControlInvalid) {
"not a map",
"{ " + genIfaceConfig() + ", \n" +
" \"subnet4\": [ ], \n"
" \"queue-control\": 75 \n"
" \"dhcp-queue-control\": 75 \n"
"} \n"
},
{
"queue type missing",
"{ " + genIfaceConfig() + ", \n" +
" \"subnet4\": [ ], \n"
" \"queue-control\": { \n"
" \"dhcp-queue-control\": { \n"
" \"capacity\": 100 \n"
" } \n"
"} \n"
......@@ -6420,9 +6449,9 @@ TEST_F(Dhcp4ParserTest, queueControlInvalid) {
// fail as expected. Note, we use parseDHCP4() directly
// as all of the errors above are enforced by the grammar.
for (auto scenario : scenarios) {
SCOPED_TRACE((scenario).description_);
SCOPED_TRACE(scenario.description_);
{
EXPECT_THROW(parseDHCP4((scenario).json_), Dhcp4ParseError);
EXPECT_THROW(parseDHCP4(scenario.json_), Dhcp4ParseError);
}
}
}
......
......@@ -653,10 +653,10 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) {
return (isc::config::createAnswer(1, err.str()));
}
// Configure packet queue
// Configure DHCP packet queue
try {
data::ConstElementPtr qc;
qc = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
qc = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl();
if (!qc) {
// @todo For now we're manually constructing default queue config
// This probably needs to be built into the PQM?
......@@ -673,7 +673,7 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) {
} catch (const std::exception& ex) {
std::ostringstream err;
err << "Error setting packet queue controls after server reconfiguration: "
err << "Error setting DHCP packet queue controls after server reconfiguration: "
<< ex.what();
return (isc::config::createAnswer(1, err.str()));
}
......
This diff is collapsed.
......@@ -1597,12 +1597,12 @@ ControlCharacterFill [^"\\]|\\{JSONEscapeSequence}
}
}
\"queue-control\" {
\"dhcp-queue-control\" {
switch(driver.ctx_) {
case isc::dhcp::Parser6Context::DHCP6:
return isc::dhcp::Dhcp6Parser::make_QUEUE_CONTROL(driver.loc_);
return isc::dhcp::Dhcp6Parser::make_DHCP_QUEUE_CONTROL(driver.loc_);
default:
return isc::dhcp::Dhcp6Parser::make_STRING("queue-control", driver.loc_);
return isc::dhcp::Dhcp6Parser::make_STRING("dhcp-queue-control", driver.loc_);
}
}
......
......@@ -3050,7 +3050,7 @@ namespace isc { namespace dhcp {
#line 1930 "dhcp6_parser.yy" // lalr1.cc:859
{
ElementPtr qc = yystack_[0].value.as< ElementPtr > ();
ctx.stack_.back()->set("queue-control", qc);
ctx.stack_.back()->set("dhcp-queue-control", qc);
if (!qc->contains("queue-type")) {
std::stringstream msg;
......@@ -4793,7 +4793,7 @@ namespace isc { namespace dhcp {
"\"max-reclaim-time\"", "\"unwarned-reclaim-cycles\"", "\"server-id\"",
"\"LLT\"", "\"EN\"", "\"LL\"", "\"identifier\"", "\"htype\"", "\"time\"",
"\"enterprise-id\"", "\"dhcp4o6-port\"", "\"control-socket\"",
"\"socket-type\"", "\"socket-name\"", "\"queue-control\"",
"\"socket-type\"", "\"socket-name\"", "\"dhcp-queue-control\"",
"\"dhcp-ddns\"", "\"enable-updates\"", "\"qualifying-suffix\"",
"\"server-ip\"", "\"server-port\"", "\"sender-ip\"", "\"sender-port\"",
"\"max-queue-size\"", "\"ncr-protocol\"", "\"ncr-format\"",
......@@ -4883,8 +4883,8 @@ namespace isc { namespace dhcp {
"server_id_type", "$@98", "duid_type", "htype", "identifier", "$@99",
"time", "enterprise_id", "dhcp4o6_port", "control_socket", "$@100",
"control_socket_params", "control_socket_param", "socket_type", "$@101",
"socket_name", "$@102", "queue_control", "$@103", "dhcp_ddns", "$@104",
"sub_dhcp_ddns", "$@105", "dhcp_ddns_params", "dhcp_ddns_param",
"socket_name", "$@102", "dhcp_queue_control", "$@103", "dhcp_ddns",
"$@104", "sub_dhcp_ddns", "$@105", "dhcp_ddns_params", "dhcp_ddns_param",
"enable_updates", "qualifying_suffix", "$@106", "server_ip", "$@107",
"server_port", "sender_ip", "$@108", "sender_port", "max_queue_size",
"ncr_protocol", "$@109", "ncr_protocol_value", "ncr_format", "$@110",
......
......@@ -462,7 +462,7 @@ namespace isc { namespace dhcp {
TOKEN_CONTROL_SOCKET = 370,
TOKEN_SOCKET_TYPE = 371,
TOKEN_SOCKET_NAME = 372,
TOKEN_QUEUE_CONTROL = 373,
TOKEN_DHCP_QUEUE_CONTROL = 373,
TOKEN_DHCP_DDNS = 374,
TOKEN_ENABLE_UPDATES = 375,
TOKEN_QUALIFYING_SUFFIX = 376,
......@@ -1098,7 +1098,7 @@ namespace isc { namespace dhcp {
static inline
symbol_type
make_QUEUE_CONTROL (const location_type& l);
make_DHCP_QUEUE_CONTROL (const location_type& l);
static inline
symbol_type
......@@ -2618,9 +2618,9 @@ namespace isc { namespace dhcp {
}
Dhcp6Parser::symbol_type
Dhcp6Parser::make_QUEUE_CONTROL (const location_type& l)
Dhcp6Parser::make_DHCP_QUEUE_CONTROL (const location_type& l)
{
return symbol_type (token::TOKEN_QUEUE_CONTROL, l);
return symbol_type (token::TOKEN_DHCP_QUEUE_CONTROL, l);
}
Dhcp6Parser::symbol_type
......
......@@ -172,7 +172,7 @@ using namespace std;
SOCKET_TYPE "socket-type"
SOCKET_NAME "socket-name"
QUEUE_CONTROL "queue-control"
DHCP_QUEUE_CONTROL "dhcp-queue-control"
DHCP_DDNS "dhcp-ddns"
ENABLE_UPDATES "enable-updates"
......@@ -457,7 +457,7 @@ global_param: preferred_lifetime
| server_id
| dhcp4o6_port
| control_socket
| queue_control
| dhcp_queue_control
| dhcp_ddns
| user_context
| comment
......@@ -1923,13 +1923,13 @@ socket_name: SOCKET_NAME {
ctx.leave();
};
// --- queue-control ---------------------------------------------
// --- dhcp-queue-control ---------------------------------------------
queue_control: QUEUE_CONTROL {
dhcp_queue_control: DHCP_QUEUE_CONTROL {
ctx.enter(ctx.NO_KEYWORD);
} COLON map_value {
ElementPtr qc = $4;
ctx.stack_.back()->set("queue-control", qc);
ctx.stack_.back()->set("dhcp-queue-control", qc);
if (!qc->contains("queue-type")) {
std::stringstream msg;
......
......@@ -31,7 +31,7 @@
#include <dhcpsrv/parsers/host_reservations_list_parser.h>
#include <dhcpsrv/parsers/ifaces_config_parser.h>
#include <dhcpsrv/parsers/option_data_parser.h>
#include <dhcpsrv/parsers/queue_control_parser.h>
#include <dhcpsrv/parsers/dhcp_queue_control_parser.h>
#include <dhcpsrv/parsers/simple_parser6.h>
#include <dhcpsrv/parsers/shared_networks_list_parser.h>
#include <dhcpsrv/parsers/sanity_checks_parser.h>
......@@ -483,9 +483,9 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set,
continue;
}
if (config_pair.first == "queue-control") {
QueueControlParser parser(AF_INET);
srv_config->setQueueControlInfo(parser.parse(config_pair.second));
if (config_pair.first == "dhcp-queue-control") {
DHCPQueueControlParser parser(AF_INET);
srv_config->setDHCPQueueControl(parser.parse(config_pair.second));
continue;
}
......
......@@ -174,8 +174,8 @@ Parser6Context::contextName()
return ("duid-type");
case CONTROL_SOCKET:
return ("control-socket");
case QUEUE_CONTROL:
return ("queue-control");
case DHCP_QUEUE_CONTROL:
return ("dhcp-queue-control");
case POOLS:
return ("pools");
case PD_POOLS:
......
......@@ -271,8 +271,8 @@ public:
/// Used while parsing Dhcp6/control-socket structures.
CONTROL_SOCKET,
/// Used while parsing Dhcp4/queue-control structures.
QUEUE_CONTROL,
/// Used while parsing Dhcp4/dhcp-queue-control structures.
DHCP_QUEUE_CONTROL,
/// Used while parsing Dhcp6/subnet6/pools structures.
POOLS,
......
......@@ -6915,61 +6915,86 @@ TEST_F(Dhcp6ParserTest, serverTag) {
ASSERT_THROW(parseDHCP6(bad_tag), std::exception);
}
// Check whether it is possible to configure queue-control
TEST_F(Dhcp6ParserTest, queueControl) {
// Config without server-tag
string config_no_queue = "{ " + genIfaceConfig() + "," +
"\"subnet6\": [ ] "
"}";
string config_with_queue =
"{ " + genIfaceConfig() + ", \n" +
" \"subnet6\": [ ], \n"
" \"queue-control\": { \n"
" \"queue-type\": \"some-type\", \n"
" \"capacity\": 75 \n"
" } \n"
"} \n";
// Check whether it is possible to configure packet queue
TEST_F(Dhcp6ParserTest, dhcpQueueControl) {
struct Scenario {
std::string description_;
std::string json_;
};
string config_with_context =
"{ " + genIfaceConfig() + ", \n" +
" \"subnet6\": [ ], \n"
" \"queue-control\": { \n"
std::vector<Scenario> scenarios = {
{
"no entry",
""
},
{
"valid entry",
"{ \n"
" \"queue-type\": \"some-type\", \n"
" \"capacity\": 75 \n"
"} \n"
},
{
"valid arbitrary content",
"{ \n"
" \"queue-type\": \"some-type\", \n"
" \"capacity\": 90, \n"
" \"user-context\": { \"comment\": \"some text\" } \n"
" } \n"
"} \n";
" \"user-context\": { \"comment\": \"some text\" },\n"
" \"random-bool\" : false, \n"
" \"random-int\" : 1236 \n"
"} \n"
}
};
// Let's check the default. It should be empty.
data::ConstElementPtr control;
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
control = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl();
ASSERT_FALSE(control);
// Configuration with no queue should default to an emtpy control.
configure(config_no_queue, CONTROL_RESULT_SUCCESS, "");
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
ASSERT_FALSE(control);
// Iterate over the incorrect scenarios and verify they
// fail as expected. Note, we use parseDHCP6() directly
// as all of the errors above are enforced by the grammar.
data::ConstElementPtr exp_elems;
for (auto scenario : scenarios) {
SCOPED_TRACE(scenario.description_);
{
// Clear the config
CfgMgr::instance().clear();
// Clear the config
CfgMgr::instance().clear();
// Construct the config JSON
std::stringstream os;
os << "{ " + genIfaceConfig();
if (!scenario.json_.empty()) {
// Configuration with queue should be valid.
configure(config_with_queue, CONTROL_RESULT_SUCCESS, "");
control = CfgMgr::instance().getStagingCfg()->getQueueControlInfo();
ASSERT_TRUE(control);