Skip to content

Commit

Permalink
Use the next-hop specified in host route DHCP option.
Browse files Browse the repository at this point in the history 
The nexthop provided in host route DHCP option is not being used, as we always
used the subnet's GW address. With L2 networks, contrail may not have GW
configured and external GW may have to be used. Changing this behaviour such
that the specified GW is sent and when the GW is 0.0.0.0, the subnet's GW
is used.

Change-Id: Idf603d6beabc61558a227fc1f1d5046feb26502a
closes-bug: 1483105
  • Loading branch information
@haripk
haripk committed Aug 11, 2015
1 parent 2bac9f3 commit e924ed8
Show file tree
Hide file tree
Showing 8 changed files with 92 additions and 66 deletions.
37 changes: 23 additions & 14 deletions src/vnsw/agent/oper/oper_dhcp_options.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,28 @@ class OperDhcpOptions {
typedef std::vector<autogen::DhcpOptionType> DhcpOptionsList;
typedef std::vector<autogen::RouteType> HostOptionsList;

struct Subnet {
struct HostRoute {
Ip4Address prefix_;
uint32_t plen_;
Ip4Address gw_;

Subnet() : prefix_(), plen_(0) {}
bool operator<(const Subnet &rhs) const {
HostRoute() : prefix_(), plen_(0), gw_() {}
bool operator<(const HostRoute &rhs) const {
if (prefix_ != rhs.prefix_)
return prefix_ < rhs.prefix_;
return plen_ < rhs.plen_;
if (plen_ != rhs.plen_)
return plen_ < rhs.plen_;
return gw_ < rhs.gw_;
}
bool operator==(const Subnet &rhs) const {
return prefix_ == rhs.prefix_ && plen_ == rhs.plen_;
bool operator==(const HostRoute &rhs) const {
return prefix_ == rhs.prefix_ && plen_ == rhs.plen_ &&
gw_ == rhs.gw_;
}
std::string ToString() const {
char len[32];
snprintf(len, sizeof(len), "%u", plen_);
return prefix_.to_string() + "/" + std::string(len);
return prefix_.to_string() + "/" + std::string(len) +
" -> " + gw_.to_string();
}
};

Expand All @@ -46,7 +51,7 @@ class OperDhcpOptions {
virtual ~OperDhcpOptions() {}

const DhcpOptionsList &dhcp_options() const { return dhcp_options_; }
const std::vector<Subnet> &host_routes() const { return host_routes_; }
const std::vector<HostRoute> &host_routes() const { return host_routes_; }
void set_options(const DhcpOptionsList &options) { dhcp_options_ = options; }
void set_host_routes(const HostOptionsList &host_routes) {
host_routes_.clear();
Expand All @@ -55,14 +60,18 @@ class OperDhcpOptions {
void update_host_routes(const HostOptionsList &host_routes) {
host_routes_.clear();
for (unsigned int i = 0; i < host_routes.size(); ++i) {
Subnet subnet;
HostRoute host_route;
boost::system::error_code ec = Ip4PrefixParse(host_routes[i].prefix,
&subnet.prefix_,
(int *)&subnet.plen_);
if (ec || subnet.plen_ > 32) {
&host_route.prefix_,
(int *)&host_route.plen_);
if (ec || host_route.plen_ > 32) {
continue;
}
host_routes_.push_back(subnet);
host_route.gw_ = Ip4Address::from_string(host_routes[i].next_hop, ec);
if (ec) {
host_route.gw_ = Ip4Address();
}
host_routes_.push_back(host_route);
}
}

Expand All @@ -75,7 +84,7 @@ class OperDhcpOptions {

private:
DhcpOptionsList dhcp_options_;
std::vector<Subnet> host_routes_;
std::vector<HostRoute> host_routes_;
};

#endif // vnsw_agent_oper_dhcp_options_h_
4 changes: 2 additions & 2 deletions src/vnsw/agent/oper/vn.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ AgentDBTable *VnEntry::DBToTable() const {
}

bool VnEntry::GetVnHostRoutes(const std::string &ipam,
std::vector<OperDhcpOptions::Subnet> *routes) const {
std::vector<OperDhcpOptions::HostRoute> *routes) const {
VnData::VnIpamDataMap::const_iterator it = vn_ipam_data_.find(ipam);
if (it != vn_ipam_data_.end()) {
*routes = it->second.oper_dhcp_options_.host_routes();
Expand Down Expand Up @@ -1075,7 +1075,7 @@ bool VnEntry::DBEntrySandesh(Sandesh *sresp, std::string &name) const {
it != vn_ipam_data_.end(); ++it) {
VnIpamHostRoutes vn_ipam_host_routes;
vn_ipam_host_routes.ipam_name = it->first;
const std::vector<OperDhcpOptions::Subnet> &host_routes =
const std::vector<OperDhcpOptions::HostRoute> &host_routes =
it->second.oper_dhcp_options_.host_routes();
for (uint32_t i = 0; i < host_routes.size(); ++i) {
vn_ipam_host_routes.host_routes.push_back(
Expand Down
2 changes: 1 addition & 1 deletion src/vnsw/agent/oper/vn.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class VnEntry : AgentRefCount<VnEntry>, public AgentOperDBEntry {
const std::vector<VnIpam> &GetVnIpam() const { return ipam_; };
const VnIpam *GetIpam(const IpAddress &ip) const;
bool GetVnHostRoutes(const std::string &ipam,
std::vector<OperDhcpOptions::Subnet> *routes) const;
std::vector<OperDhcpOptions::HostRoute> *routes) const;
bool GetIpamName(const IpAddress &vm_addr, std::string *ipam_name) const;
bool GetIpamData(const IpAddress &vm_addr, std::string *ipam_name,
autogen::IpamType *ipam_type) const;
Expand Down
6 changes: 1 addition & 5 deletions src/vnsw/agent/services/dhcp_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1104,11 +1104,7 @@ bool DhcpHandler::IsOptionRequested(uint8_t option) {

bool DhcpHandler::IsRouterOptionNeeded() {
// If GW is not configured, dont include
if (config_.gw_addr.is_unspecified())
return false;

// If router option is already included, nothing to do
if (is_flag_set(DHCP_OPTION_ROUTER))
if (config_.gw_addr.is_unspecified() && routers_.empty())
return false;

// When client requests Classless Static Routes option and this is
Expand Down
46 changes: 24 additions & 22 deletions src/vnsw/agent/services/dhcp_handler_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,18 +361,22 @@ void DhcpHandlerBase::ReadClasslessRoute(uint32_t option, uint16_t opt_len,
while (value.good()) {
std::string snetstr;
value >> snetstr;
OperDhcpOptions::Subnet subnet;
boost::system::error_code ec = Ip4PrefixParse(snetstr, &subnet.prefix_,
(int *)&subnet.plen_);
if (ec || subnet.plen_ > 32 || !value.good()) {
OperDhcpOptions::HostRoute host_route;
boost::system::error_code ec = Ip4PrefixParse(snetstr, &host_route.prefix_,
(int *)&host_route.plen_);
if (ec || host_route.plen_ > 32 || !value.good()) {
DHCP_BASE_TRACE("Invalid Classless route DHCP option -"
"has to be list of <subnet/plen gw>");
break;
}
host_routes_.push_back(subnet);

// ignore the gw, as we use always use subnet's gw
value >> snetstr;
host_route.gw_ = Ip4Address::from_string(snetstr, ec);
if (ec) {
host_route.gw_ = Ip4Address();
}

host_routes_.push_back(host_route);
}
}

Expand All @@ -385,7 +389,7 @@ void DhcpHandlerBase::ReadClasslessRoute(uint32_t option, uint16_t opt_len,
// 6) options at IPAM level (classless routes from IPAM dhcp options)
// Add the options defined at the highest level in priority
uint16_t DhcpHandlerBase::AddClasslessRouteOption(uint16_t opt_len) {
std::vector<OperDhcpOptions::Subnet> host_routes;
std::vector<OperDhcpOptions::HostRoute> host_routes;
do {
// Port specific host route options
// TODO: should we remove host routes at port level from schema ?
Expand Down Expand Up @@ -429,18 +433,10 @@ uint16_t DhcpHandlerBase::AddClasslessRouteOption(uint16_t opt_len) {
}

// IPAM level host route options
const std::vector<autogen::RouteType> &routes =
ipam_type_.host_routes.route;
for (unsigned int i = 0; i < routes.size(); ++i) {
OperDhcpOptions::Subnet subnet;
boost::system::error_code ec = Ip4PrefixParse(routes[i].prefix,
&subnet.prefix_,
(int *)&subnet.plen_);
if (ec || subnet.plen_ > 32) {
continue;
}
host_routes.push_back(subnet);
}
OperDhcpOptions oper_dhcp_options;
oper_dhcp_options.update_host_routes(ipam_type_.host_routes.route);
host_routes = oper_dhcp_options.host_routes();

// Host route options from IPAM level DHCP options
if (!host_routes.size() && host_routes_.size()) {
host_routes.swap(host_routes_);
Expand All @@ -455,13 +451,18 @@ uint16_t DhcpHandlerBase::AddClasslessRouteOption(uint16_t opt_len) {
for (uint32_t i = 0; i < host_routes.size(); ++i) {
uint32_t prefix = host_routes[i].prefix_.to_ulong();
uint32_t plen = host_routes[i].plen_;
uint32_t gw = host_routes[i].gw_.to_ulong();
*ptr++ = plen;
len++;
for (unsigned int i = 0; plen && i <= (plen - 1) / 8; ++i) {
*ptr++ = (prefix >> 8 * (3 - i)) & 0xFF;
for (unsigned int j = 0; plen && j <= (plen - 1) / 8; ++j) {
*ptr++ = (prefix >> 8 * (3 - j)) & 0xFF;
len++;
}
*(uint32_t *)ptr = htonl(config_.gw_addr.to_v4().to_ulong());
// if GW is not specified, set it to subnet's GW
if (gw)
*(uint32_t *)ptr = htonl(gw);
else
*(uint32_t *)ptr = htonl(config_.gw_addr.to_v4().to_ulong());
ptr += sizeof(uint32_t);
len += sizeof(uint32_t);
}
Expand Down Expand Up @@ -559,6 +560,7 @@ uint16_t DhcpHandlerBase::AddDhcpOptions(
if (option == DHCP_OPTION_ROUTER) {
// Router option is added later
routers_ = options[i].dhcp_option_value;
set_flag(DHCP_OPTION_ROUTER);
} else {
opt_len = AddIpv4Option(option, opt_len,
options[i].dhcp_option_value,
Expand Down
2 changes: 1 addition & 1 deletion src/vnsw/agent/services/dhcp_handler_base.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class DhcpHandlerBase : public ProtoHandler {
std::string routers_;

// parse and store the host routes coming in config
std::vector<OperDhcpOptions::Subnet> host_routes_;
std::vector<OperDhcpOptions::HostRoute> host_routes_;
DhcpOptionLevel host_routes_level_;

std::string ipam_name_;
Expand Down
44 changes: 26 additions & 18 deletions src/vnsw/agent/services/test/dhcp_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,17 @@
#define CLIENT_REQ_IP "1.2.3.4"
#define CLIENT_REQ_PREFIX "1.2.3.0"
#define CLIENT_REQ_GW "1.2.3.1"
#define MAX_WAIT_COUNT 1000
#define MAX_WAIT_COUNT 3000
#define BUF_SIZE 8192
MacAddress src_mac(0x00, 0x01, 0x02, 0x03, 0x04, 0x05);
MacAddress dest_mac(0x00, 0x11, 0x12, 0x13, 0x14, 0x15);
#define DHCP_RESPONSE_STRING "Server : 1.1.1.200; Lease time : 4294967295; Subnet mask : 255.255.255.0; Broadcast : 1.1.1.255; Gateway : 1.1.1.200; Host Name : vm1; DNS : 1.1.1.200; Domain Name : test.contrail.juniper.net; "
#define HOST_ROUTE_STRING "Host Routes : 10.1.1.0/24 -> 1.1.1.200;10.1.2.0/24 -> 1.1.1.200;150.25.75.0/24 -> 1.1.1.200;192.168.1.128/28 -> 1.1.1.200;"
#define HOST_ROUTE_STRING "Host Routes : 10.1.1.0/24 -> 1.1.1.200;10.1.2.0/24 -> 1.1.1.200;150.25.75.0/24 -> 150.25.75.254;192.168.1.128/28 -> 1.1.1.200;"
#define CHANGED_HOST_ROUTE_STRING "Host Routes : 150.2.2.0/24 -> 1.1.1.200;192.1.1.1/28 -> 1.1.1.200;"
#define IPAM_DHCP_OPTIONS_STRING "DNS : 1.2.3.4; Domain Name : test.com; Time Server : 3.2.14.5"
#define SUBNET_DHCP_OPTIONS_STRING "DNS : 11.12.13.14; Domain Name : subnet.com; Time Server : 3.2.14.5;"
#define SUBNET_DHCP_OPTIONS_STRING "DNS : 11.12.13.14; Domain Name : subnet.com; Time Server : 3.2.14.5; Host Routes : 10.1.1.0/24 -> 1.1.1.200;10.1.2.0/24 -> 1.1.1.200;150.25.75.0/24 -> 150.25.75.254;192.168.1.128/28 -> 1.1.1.200;Gateway : 1.2.3.4; Gateway : 5.6.7.8; Gateway : 1.1.1.200;"
#define PORT_DHCP_OPTIONS_STRING "DNS : 21.22.23.24; Time Server : 13.12.14.15; Domain Name : test.com;"
#define PORT_HOST_ROUTE_STRING "Host Routes : 99.2.3.0/24 -> 1.1.1.200;99.5.0.0/16 -> 1.1.1.200;"
#define PORT_HOST_ROUTE_STRING "Host Routes : 99.2.3.0/24 -> 1.1.1.200;99.5.0.0/16 -> 99.5.0.1;"

#define DHCP_CHECK(condition) \
do { \
Expand Down Expand Up @@ -1093,9 +1093,9 @@ TEST_F(DhcpTest, IpamSpecificDhcpOptions) {
</dhcp-option-list>\
<host-routes>\
<route><prefix>10.1.1.0/24</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>10.1.2.0/24</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>150.25.75.0/24</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>192.168.1.128/28</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>10.1.2.0/24</prefix> <next-hop>junk</next-hop> <next-hop-type /></route>\
<route><prefix>150.25.75.0/24</prefix> <next-hop>150.25.75.254</next-hop> <next-hop-type /></route>\
<route><prefix>192.168.1.128/28</prefix> <next-hop>0.0.0.0</next-hop> <next-hop-type /></route>\
</host-routes>\
</network-ipam-mgmt>";

Expand Down Expand Up @@ -1204,10 +1204,14 @@ TEST_F(DhcpTest, SubnetSpecificDhcpOptions) {
<dhcp-option-name>4</dhcp-option-name>\
<dhcp-option-value>3.2.14.5</dhcp-option-value>\
</dhcp-option>\
<dhcp-option>\
<dhcp-option-name>3</dhcp-option-name>\
<dhcp-option-value>12.13.14.15 23.24.25.26</dhcp-option-value>\
</dhcp-option>\
</dhcp-option-list>\
<host-routes>\
<route><prefix>1.2.3.0/24</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>4.5.0.0/16</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>4.5.0.0/16</prefix> <next-hop>4.5.0.254</next-hop> <next-hop-type /></route>\
</host-routes>\
</network-ipam-mgmt>";
char add_subnet_tags[] =
Expand All @@ -1220,14 +1224,18 @@ TEST_F(DhcpTest, SubnetSpecificDhcpOptions) {
<dhcp-option-name>15</dhcp-option-name>\
<dhcp-option-value>subnet.com</dhcp-option-value>\
</dhcp-option>\
<dhcp-option>\
<dhcp-option-name>3</dhcp-option-name>\
<dhcp-option-value>1.2.3.4 5.6.7.8</dhcp-option-value>\
</dhcp-option>\
</dhcp-option-list>";
// option 4 from ipam and other options from subnet should be used

std::vector<std::string> vm_host_routes;
vm_host_routes.push_back("10.1.1.0/24");
vm_host_routes.push_back("10.1.2.0/24");
vm_host_routes.push_back("150.25.75.0/24");
vm_host_routes.push_back("192.168.1.128/28");
vm_host_routes.push_back("10.1.2.0/24 junk");
vm_host_routes.push_back("150.25.75.0/24 150.25.75.254");
vm_host_routes.push_back("192.168.1.128/28 0.0.0.0");

CreateVmportEnv(input, 2, 0);
client->WaitForIdle();
Expand Down Expand Up @@ -1309,8 +1317,8 @@ TEST_F(DhcpTest, PortSpecificDhcpOptions) {
</dhcp-option>\
</dhcp-option-list>\
<host-routes>\
<route><prefix>1.2.3.0/24</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>4.5.0.0/16</prefix> <next-hop /> <next-hop-type /></route>\
<route><prefix>1.2.3.0/24</prefix> <next-hop>0.0.0.0</next-hop> <next-hop-type /></route>\
<route><prefix>4.5.0.0/16</prefix> <next-hop>4.5.0.254</next-hop> <next-hop-type /></route>\
</host-routes>\
</network-ipam-mgmt>";

Expand All @@ -1334,8 +1342,8 @@ TEST_F(DhcpTest, PortSpecificDhcpOptions) {
</dhcp-option>\
</virtual-machine-interface-dhcp-option-list>\
<virtual-machine-interface-host-routes>\
<route><prefix>99.2.3.0/24</prefix> <next-hop /> <next-hop-type /> </route>\
<route><prefix>99.5.0.0/16</prefix> <next-hop /> <next-hop-type /> </route>\
<route><prefix>99.2.3.0/24</prefix> <next-hop>0.0.0.0</next-hop> <next-hop-type /> </route>\
<route><prefix>99.5.0.0/16</prefix> <next-hop>99.5.0.1</next-hop> <next-hop-type /> </route>\
</virtual-machine-interface-host-routes>";
// option 6 from vm interface, option 4 from subnet and option 15
// from ipam should be used
Expand Down Expand Up @@ -1911,11 +1919,11 @@ TEST_F(DhcpTest, ClasslessOption) {
"<virtual-machine-interface-dhcp-option-list>\
<dhcp-option>\
<dhcp-option-name>classless-static-routes</dhcp-option-name>\
<dhcp-option-value>10.1.2.0/24 1.2.3.4 20.20.20.0/24 20.20.20.1</dhcp-option-value>\
<dhcp-option-value>10.1.2.0/24 0.0.0.0 20.20.20.0/24 20.20.20.1</dhcp-option-value>\
</dhcp-option>\
</virtual-machine-interface-dhcp-option-list>";

#define OPTION_CATEGORY_CLASSLESS "Host Routes : 10.1.2.0/24 -> 1.1.1.200;20.20.20.0/24 -> 1.1.1.200;"
#define OPTION_CATEGORY_CLASSLESS "Host Routes : 10.1.2.0/24 -> 1.1.1.200;20.20.20.0/24 -> 20.20.20.1;"
DhcpOptionCategoryTest(vm_interface_attr, true, OPTION_CATEGORY_CLASSLESS,
false, "");
}
Expand All @@ -1926,7 +1934,7 @@ TEST_F(DhcpTest, ClasslessOptionError) {
"<virtual-machine-interface-dhcp-option-list>\
<dhcp-option>\
<dhcp-option-name>classless-static-routes</dhcp-option-name>\
<dhcp-option-value>10.1.2.0/24 1.2.3.4 abcd</dhcp-option-value>\
<dhcp-option-value>10.1.2.0/24 0.0.0.0 abcd</dhcp-option-value>\
</dhcp-option>\
<dhcp-option>\
<dhcp-option-name>classless-static-routes</dhcp-option-name>\
Expand Down
17 changes: 14 additions & 3 deletions src/vnsw/agent/test/test_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,20 @@ void AddNodeString(char *buff, int &len, const char *nodename, const char *name,
if (vm_host_routes && vm_host_routes->size()) {
str << " <host-routes>\n";
for (unsigned int i = 0; i < vm_host_routes->size(); i++) {
std::string prefix, nexthop;
std::vector<std::string> tokens;
boost::split(tokens, (*vm_host_routes)[i], boost::is_any_of(" \t"));
vector<string>::iterator it = tokens.begin();
if (it != tokens.end()) {
prefix = *it;
it++;
}
if (it != tokens.end()) {
nexthop = *it;
}
str << " <route>\n";
str << " <prefix>" << (*vm_host_routes)[i] << "</prefix>\n";
str << " <next-hop />\n";
str << " <prefix>" << prefix << "</prefix>\n";
str << " <next-hop>" << nexthop << "</next-hop>\n";
str << " <next-hop-type />\n";
str << " </route>\n";
}
Expand Down Expand Up @@ -1810,7 +1821,7 @@ void DelVmPortVrf(const char *name) {
void AddIPAM(const char *name, IpamInfo *ipam, int ipam_size, const char *ipam_attr,
const char *vdns_name, const std::vector<std::string> *vm_host_routes,
const char *add_subnet_tags) {
char buff[8192];
char buff[10240];
char node_name[128];
char ipam_name[128];
int len = 0;
Expand Down

0 comments on commit e924ed8

Please sign in to comment.