From e924ed85f70219fe045136c0ca6599d1972255d7 Mon Sep 17 00:00:00 2001 From: Hari Date: Tue, 11 Aug 2015 15:53:34 +0530 Subject: [PATCH] Use the next-hop specified in host route DHCP option. 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 --- src/vnsw/agent/oper/oper_dhcp_options.h | 37 ++++++++++------ src/vnsw/agent/oper/vn.cc | 4 +- src/vnsw/agent/oper/vn.h | 2 +- src/vnsw/agent/services/dhcp_handler.cc | 6 +-- src/vnsw/agent/services/dhcp_handler_base.cc | 46 ++++++++++---------- src/vnsw/agent/services/dhcp_handler_base.h | 2 +- src/vnsw/agent/services/test/dhcp_test.cc | 44 +++++++++++-------- src/vnsw/agent/test/test_util.cc | 17 ++++++-- 8 files changed, 92 insertions(+), 66 deletions(-) diff --git a/src/vnsw/agent/oper/oper_dhcp_options.h b/src/vnsw/agent/oper/oper_dhcp_options.h index a923af236c2..8c423cef339 100644 --- a/src/vnsw/agent/oper/oper_dhcp_options.h +++ b/src/vnsw/agent/oper/oper_dhcp_options.h @@ -18,23 +18,28 @@ class OperDhcpOptions { typedef std::vector DhcpOptionsList; typedef std::vector 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(); } }; @@ -46,7 +51,7 @@ class OperDhcpOptions { virtual ~OperDhcpOptions() {} const DhcpOptionsList &dhcp_options() const { return dhcp_options_; } - const std::vector &host_routes() const { return host_routes_; } + const std::vector &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(); @@ -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); } } @@ -75,7 +84,7 @@ class OperDhcpOptions { private: DhcpOptionsList dhcp_options_; - std::vector host_routes_; + std::vector host_routes_; }; #endif // vnsw_agent_oper_dhcp_options_h_ diff --git a/src/vnsw/agent/oper/vn.cc b/src/vnsw/agent/oper/vn.cc index 11ba051c5e5..a7b14ce5a7c 100644 --- a/src/vnsw/agent/oper/vn.cc +++ b/src/vnsw/agent/oper/vn.cc @@ -121,7 +121,7 @@ AgentDBTable *VnEntry::DBToTable() const { } bool VnEntry::GetVnHostRoutes(const std::string &ipam, - std::vector *routes) const { + std::vector *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(); @@ -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 &host_routes = + const std::vector &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( diff --git a/src/vnsw/agent/oper/vn.h b/src/vnsw/agent/oper/vn.h index 121060eef12..8bee25a402f 100644 --- a/src/vnsw/agent/oper/vn.h +++ b/src/vnsw/agent/oper/vn.h @@ -138,7 +138,7 @@ class VnEntry : AgentRefCount, public AgentOperDBEntry { const std::vector &GetVnIpam() const { return ipam_; }; const VnIpam *GetIpam(const IpAddress &ip) const; bool GetVnHostRoutes(const std::string &ipam, - std::vector *routes) const; + std::vector *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; diff --git a/src/vnsw/agent/services/dhcp_handler.cc b/src/vnsw/agent/services/dhcp_handler.cc index 68c5d34f16b..97c3a2051f5 100644 --- a/src/vnsw/agent/services/dhcp_handler.cc +++ b/src/vnsw/agent/services/dhcp_handler.cc @@ -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 diff --git a/src/vnsw/agent/services/dhcp_handler_base.cc b/src/vnsw/agent/services/dhcp_handler_base.cc index 12bd5478745..42e9f7107c4 100644 --- a/src/vnsw/agent/services/dhcp_handler_base.cc +++ b/src/vnsw/agent/services/dhcp_handler_base.cc @@ -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 "); 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); } } @@ -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 host_routes; + std::vector host_routes; do { // Port specific host route options // TODO: should we remove host routes at port level from schema ? @@ -429,18 +433,10 @@ uint16_t DhcpHandlerBase::AddClasslessRouteOption(uint16_t opt_len) { } // IPAM level host route options - const std::vector &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_); @@ -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); } @@ -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, diff --git a/src/vnsw/agent/services/dhcp_handler_base.h b/src/vnsw/agent/services/dhcp_handler_base.h index eaab5d8fe34..3860b515cb4 100644 --- a/src/vnsw/agent/services/dhcp_handler_base.h +++ b/src/vnsw/agent/services/dhcp_handler_base.h @@ -149,7 +149,7 @@ class DhcpHandlerBase : public ProtoHandler { std::string routers_; // parse and store the host routes coming in config - std::vector host_routes_; + std::vector host_routes_; DhcpOptionLevel host_routes_level_; std::string ipam_name_; diff --git a/src/vnsw/agent/services/test/dhcp_test.cc b/src/vnsw/agent/services/test/dhcp_test.cc index 34bd25db56d..b4f4bff85f1 100644 --- a/src/vnsw/agent/services/test/dhcp_test.cc +++ b/src/vnsw/agent/services/test/dhcp_test.cc @@ -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 { \ @@ -1093,9 +1093,9 @@ TEST_F(DhcpTest, IpamSpecificDhcpOptions) { \ \ 10.1.1.0/24 \ - 10.1.2.0/24 \ - 150.25.75.0/24 \ - 192.168.1.128/28 \ + 10.1.2.0/24 junk \ + 150.25.75.0/24 150.25.75.254 \ + 192.168.1.128/28 0.0.0.0 \ \ "; @@ -1204,10 +1204,14 @@ TEST_F(DhcpTest, SubnetSpecificDhcpOptions) { 4\ 3.2.14.5\ \ + \ + 3\ + 12.13.14.15 23.24.25.26\ + \ \ \ 1.2.3.0/24 \ - 4.5.0.0/16 \ + 4.5.0.0/16 4.5.0.254 \ \ "; char add_subnet_tags[] = @@ -1220,14 +1224,18 @@ TEST_F(DhcpTest, SubnetSpecificDhcpOptions) { 15\ subnet.com\ \ + \ + 3\ + 1.2.3.4 5.6.7.8\ + \ "; // option 4 from ipam and other options from subnet should be used std::vector 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(); @@ -1309,8 +1317,8 @@ TEST_F(DhcpTest, PortSpecificDhcpOptions) { \ \ \ - 1.2.3.0/24 \ - 4.5.0.0/16 \ + 1.2.3.0/24 0.0.0.0 \ + 4.5.0.0/16 4.5.0.254 \ \ "; @@ -1334,8 +1342,8 @@ TEST_F(DhcpTest, PortSpecificDhcpOptions) { \ \ \ - 99.2.3.0/24 \ - 99.5.0.0/16 \ + 99.2.3.0/24 0.0.0.0 \ + 99.5.0.0/16 99.5.0.1 \ "; // option 6 from vm interface, option 4 from subnet and option 15 // from ipam should be used @@ -1911,11 +1919,11 @@ TEST_F(DhcpTest, ClasslessOption) { "\ \ classless-static-routes\ - 10.1.2.0/24 1.2.3.4 20.20.20.0/24 20.20.20.1\ + 10.1.2.0/24 0.0.0.0 20.20.20.0/24 20.20.20.1\ \ "; - #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, ""); } @@ -1926,7 +1934,7 @@ TEST_F(DhcpTest, ClasslessOptionError) { "\ \ classless-static-routes\ - 10.1.2.0/24 1.2.3.4 abcd\ + 10.1.2.0/24 0.0.0.0 abcd\ \ \ classless-static-routes\ diff --git a/src/vnsw/agent/test/test_util.cc b/src/vnsw/agent/test/test_util.cc index 68d52a680df..c5098a6dd12 100644 --- a/src/vnsw/agent/test/test_util.cc +++ b/src/vnsw/agent/test/test_util.cc @@ -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 << " \n"; for (unsigned int i = 0; i < vm_host_routes->size(); i++) { + std::string prefix, nexthop; + std::vector tokens; + boost::split(tokens, (*vm_host_routes)[i], boost::is_any_of(" \t")); + vector::iterator it = tokens.begin(); + if (it != tokens.end()) { + prefix = *it; + it++; + } + if (it != tokens.end()) { + nexthop = *it; + } str << " \n"; - str << " " << (*vm_host_routes)[i] << "\n"; - str << " \n"; + str << " " << prefix << "\n"; + str << " " << nexthop << "\n"; str << " \n"; str << " \n"; } @@ -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 *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;