From 3ba54c7e3559359abd8d4734aa969829309a9dab Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Thu, 23 Jul 2015 09:50:10 -0700 Subject: [PATCH] Eliminate some poorly thought out optimizations from the netconf/controller interaction, and go ahead and bump version to 1.0.4. For a while in 1.0.3 -dev I was trying to optimize out repeated network controller requests by using a ratcheting mechanism. If the client received a network config that was indeed different from the one it had, it would respond by instantlly requesting it again. Not sure what I was thinking. It's fundamentally unsafe to respond to a message with another message of the same type -- it risks a race condition. In this case that's exactly what could happen. It just isn't worth the added complexity to avoid a tiny, tiny amount of network overhead, so I've taken this whole path out. A few extra bytes every two minutes isn't worth fretting about, but as I recall the reason for this optimization was to save CPU on the controller. This can be achieved by just caching responses in memory *there* and serving those same responses back out if they haven't changed. I think I developed that 'ratcheting' stuff before I went full time on this. It's hard to develop stuff like this without hours of sustained focus. --- controller/SqliteNetworkController.cpp | 33 +++++++++++++------------- node/IncomingPacket.cpp | 31 ++++++------------------ node/Network.cpp | 13 +++++++++- node/NetworkConfig.cpp | 23 +++++++----------- node/NetworkConfig.hpp | 26 ++++++-------------- node/NetworkController.hpp | 7 +++--- version.h | 2 +- 7 files changed, 55 insertions(+), 80 deletions(-) diff --git a/controller/SqliteNetworkController.cpp b/controller/SqliteNetworkController.cpp index 85dbed001..f64896409 100644 --- a/controller/SqliteNetworkController.cpp +++ b/controller/SqliteNetworkController.cpp @@ -296,6 +296,12 @@ SqliteNetworkController::~SqliteNetworkController() NetworkController::ResultCode SqliteNetworkController::doNetworkConfigRequest(const InetAddress &fromAddr,const Identity &signingId,const Identity &identity,uint64_t nwid,const Dictionary &metaData,uint64_t haveRevision,Dictionary &netconf) { + // Decode some stuff from metaData + const unsigned int clientMajorVersion = (unsigned int)metaData.getHexUInt(ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_MAJOR_VERSION,0); + const unsigned int clientMinorVersion = (unsigned int)metaData.getHexUInt(ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_MINOR_VERSION,0); + const unsigned int clientRevision = (unsigned int)metaData.getHexUInt(ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_REVISION,0); + const bool clientIs104 = (Utils::compareVersion(clientMajorVersion,clientMinorVersion,clientRevision,1,0,4) >= 0); + Mutex::Lock _l(_lock); // Note: we can't reuse prepared statements that return const char * pointers without @@ -418,13 +424,6 @@ NetworkController::ResultCode SqliteNetworkController::doNetworkConfigRequest(co if (!member.authorized) return NetworkController::NETCONF_QUERY_ACCESS_DENIED; - // If netconf is unchanged from client reported revision, just tell client they're up to date - - // Temporarily disabled -- old version didn't do this, and we'll go ahead and - // test more thoroughly before enabling this optimization. - //if ((haveRevision > 0)&&(haveRevision == network.revision)) - // return NetworkController::NETCONF_QUERY_OK_BUT_NOT_NEWER; - // Create and sign netconf netconf.clear(); @@ -581,7 +580,8 @@ NetworkController::ResultCode SqliteNetworkController::doNetworkConfigRequest(co if ((ipNetmaskBits <= 0)||(ipNetmaskBits > 32)) continue; - switch((IpAssignmentType)sqlite3_column_int(_sGetIpAssignmentsForNode,0)) { + const IpAssignmentType ipt = (IpAssignmentType)sqlite3_column_int(_sGetIpAssignmentsForNode,0); + switch(ipt) { case ZT_IP_ASSIGNMENT_TYPE_ADDRESS: haveStaticIpAssignment = true; break; @@ -592,13 +592,15 @@ NetworkController::ResultCode SqliteNetworkController::doNetworkConfigRequest(co continue; } - // We send both routes and IP assignments -- client knows which is - // which by whether address ends in all zeroes after netmask. - char tmp[32]; - Utils::snprintf(tmp,sizeof(tmp),"%d.%d.%d.%d/%d",(int)ip[12],(int)ip[13],(int)ip[14],(int)ip[15],ipNetmaskBits); - if (v4s.length()) - v4s.push_back(','); - v4s.append(tmp); + // 1.0.4 or newer clients support network routes in addition to IPs. + // Older clients only support IP address / netmask entries. + if ((clientIs104)||(ipt == ZT_IP_ASSIGNMENT_TYPE_ADDRESS)) { + char tmp[32]; + Utils::snprintf(tmp,sizeof(tmp),"%d.%d.%d.%d/%d",(int)ip[12],(int)ip[13],(int)ip[14],(int)ip[15],ipNetmaskBits); + if (v4s.length()) + v4s.push_back(','); + v4s.append(tmp); + } } if (!haveStaticIpAssignment) { @@ -1388,7 +1390,6 @@ unsigned int SqliteNetworkController::_doCPGet( const char *result = ""; switch(this->doNetworkConfigRequest(InetAddress(),sigid,memid,nwid,Dictionary(),hr,netconf)) { case NetworkController::NETCONF_QUERY_OK: result = "OK"; break; - case NetworkController::NETCONF_QUERY_OK_BUT_NOT_NEWER: result = "OK_BUT_NOT_NEWER"; break; case NetworkController::NETCONF_QUERY_OBJECT_NOT_FOUND: result = "OBJECT_NOT_FOUND"; break; case NetworkController::NETCONF_QUERY_ACCESS_DENIED: result = "ACCESS_DENIED"; break; case NetworkController::NETCONF_QUERY_INTERNAL_SERVER_ERROR: result = "INTERNAL_SERVER_ERROR"; break; diff --git a/node/IncomingPacket.cpp b/node/IncomingPacket.cpp index ae99352e9..c3d8cc6d0 100644 --- a/node/IncomingPacket.cpp +++ b/node/IncomingPacket.cpp @@ -392,28 +392,7 @@ bool IncomingPacket::_doOK(const RuntimeEnvironment *RR,const SharedPtr &p const unsigned int dictlen = at(ZT_PROTO_VERB_NETWORK_CONFIG_REQUEST__OK__IDX_DICT_LEN); const std::string dict((const char *)field(ZT_PROTO_VERB_NETWORK_CONFIG_REQUEST__OK__IDX_DICT,dictlen),dictlen); if (dict.length()) { - if (nw->setConfiguration(Dictionary(dict)) == 2) { // 2 == accepted and actually new - /* If this configuration was indeed new, we do another - * controller request with its revision. We do this in - * order to (a) tell the network controller we got it (it - * won't send a duplicate if ts == current), and (b) - * get another one if the controller is changing rapidly - * until we finally have the final version. - * - * Note that we don't do this for network controllers with - * versions <= 1.0.3, since those regenerate a new controller - * with a new revision every time. In that case this double - * confirmation would create a race condition. */ - const SharedPtr nc(nw->config2()); - if ((peer->atLeastVersion(1,0,3))&&(nc)&&(nc->revision() > 0)) { - Packet outp(peer->address(),RR->identity.address(),Packet::VERB_NETWORK_CONFIG_REQUEST); - outp.append((uint64_t)nw->id()); - outp.append((uint16_t)0); // no meta-data - outp.append((uint64_t)nc->revision()); - outp.armor(peer->key(),true); - RR->node->putPacket(_remoteAddress,outp.data(),outp.size()); - } - } + nw->setConfiguration(Dictionary(dict)); TRACE("got network configuration for network %.16llx from %s",(unsigned long long)nw->id(),source().toString().c_str()); } } @@ -692,6 +671,7 @@ bool IncomingPacket::_doNETWORK_CONFIG_REQUEST(const RuntimeEnvironment *RR,cons if (RR->localNetworkController) { Dictionary netconf; switch(RR->localNetworkController->doNetworkConfigRequest((h > 0) ? InetAddress() : _remoteAddress,RR->identity,peer->identity(),nwid,metaData,haveRevision,netconf)) { + case NetworkController::NETCONF_QUERY_OK: { const std::string netconfStr(netconf.toString()); if (netconfStr.length() > 0xffff) { // sanity check since field ix 16-bit @@ -712,8 +692,7 @@ bool IncomingPacket::_doNETWORK_CONFIG_REQUEST(const RuntimeEnvironment *RR,cons } } } break; - case NetworkController::NETCONF_QUERY_OK_BUT_NOT_NEWER: // nothing to do -- netconf has not changed - break; + case NetworkController::NETCONF_QUERY_OBJECT_NOT_FOUND: { Packet outp(peer->address(),RR->identity.address(),Packet::VERB_ERROR); outp.append((unsigned char)Packet::VERB_NETWORK_CONFIG_REQUEST); @@ -723,6 +702,7 @@ bool IncomingPacket::_doNETWORK_CONFIG_REQUEST(const RuntimeEnvironment *RR,cons outp.armor(peer->key(),true); RR->node->putPacket(_remoteAddress,outp.data(),outp.size()); } break; + case NetworkController::NETCONF_QUERY_ACCESS_DENIED: { Packet outp(peer->address(),RR->identity.address(),Packet::VERB_ERROR); outp.append((unsigned char)Packet::VERB_NETWORK_CONFIG_REQUEST); @@ -732,12 +712,15 @@ bool IncomingPacket::_doNETWORK_CONFIG_REQUEST(const RuntimeEnvironment *RR,cons outp.armor(peer->key(),true); RR->node->putPacket(_remoteAddress,outp.data(),outp.size()); } break; + case NetworkController::NETCONF_QUERY_INTERNAL_SERVER_ERROR: TRACE("NETWORK_CONFIG_REQUEST failed: internal error: %s",netconf.get("error","(unknown)").c_str()); break; + default: TRACE("NETWORK_CONFIG_REQUEST failed: invalid return value from NetworkController::doNetworkConfigRequest()"); break; + } } else { Packet outp(peer->address(),RR->identity.address(),Packet::VERB_ERROR); diff --git a/node/Network.cpp b/node/Network.cpp index adc8e1b8c..549219d74 100644 --- a/node/Network.cpp +++ b/node/Network.cpp @@ -38,6 +38,8 @@ #include "Buffer.hpp" #include "NetworkController.hpp" +#include "../version.h" + namespace ZeroTier { const ZeroTier::MulticastGroup Network::BROADCAST(ZeroTier::MAC(0xffffffffffffULL),0); @@ -255,9 +257,18 @@ void Network::requestConfiguration() } TRACE("requesting netconf for network %.16llx from controller %s",(unsigned long long)_id,controller().toString().c_str()); + + // TODO: in the future we will include things like join tokens here, etc. + Dictionary metaData; + metaData.setHex(ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_MAJOR_VERSION,ZEROTIER_ONE_VERSION_MAJOR); + metaData.setHex(ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_MINOR_VERSION,ZEROTIER_ONE_VERSION_MINOR); + metaData.setHex(ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_REVISION,ZEROTIER_ONE_VERSION_REVISION); + std::string mds(metaData.toString()); + Packet outp(controller(),RR->identity.address(),Packet::VERB_NETWORK_CONFIG_REQUEST); outp.append((uint64_t)_id); - outp.append((uint16_t)0); // no meta-data + outp.append((uint16_t)mds.length()); + outp.append((const void *)mds.data(),(unsigned int)mds.length()); { Mutex::Lock _l(_lock); if (_config) diff --git a/node/NetworkConfig.cpp b/node/NetworkConfig.cpp index ba4d338bb..7898646cf 100644 --- a/node/NetworkConfig.cpp +++ b/node/NetworkConfig.cpp @@ -47,7 +47,6 @@ SharedPtr NetworkConfig::createTestNetworkConfig(const Address &s nc->_private = false; nc->_enableBroadcast = true; nc->_name = "ZT_TEST_NETWORK"; - nc->_description = "Built-in dummy test network"; // Make up a V4 IP from 'self' in the 10.0.0.0/8 range -- no // guarantee of uniqueness but collisions are unlikely. @@ -111,7 +110,6 @@ void NetworkConfig::_fromDictionary(const Dictionary &d) _name = d.get(ZT_NETWORKCONFIG_DICT_KEY_NAME); if (_name.length() > ZT1_MAX_NETWORK_SHORT_NAME_LENGTH) throw std::invalid_argument("network short name too long (max: 255 characters)"); - _description = d.get(ZT_NETWORKCONFIG_DICT_KEY_DESC,std::string()); // In dictionary IPs are split into V4 and V6 addresses, but we don't really // need that so merge them here. @@ -132,26 +130,22 @@ void NetworkConfig::_fromDictionary(const Dictionary &d) case AF_INET: if ((!addr.netmaskBits())||(addr.netmaskBits() > 32)) continue; - else if (addr.isNetwork()) { - // TODO: add route to network -- this is a route without an IP assignment - continue; - } break; case AF_INET6: if ((!addr.netmaskBits())||(addr.netmaskBits() > 128)) continue; - else if (addr.isNetwork()) { - // TODO: add route to network -- this is a route without an IP assignment - continue; - } break; default: // ignore unrecognized address types or junk/empty fields continue; } - _staticIps.push_back(addr); + if (addr.isNetwork()) + _localRoutes.push_back(addr); + else _staticIps.push_back(addr); } - if (_staticIps.size() > ZT1_MAX_ZT_ASSIGNED_ADDRESSES) - throw std::invalid_argument("too many ZT-assigned IP addresses or routes"); + if (_localRoutes.size() > ZT1_MAX_ZT_ASSIGNED_ADDRESSES) throw std::invalid_argument("too many ZT-assigned routes"); + if (_staticIps.size() > ZT1_MAX_ZT_ASSIGNED_ADDRESSES) throw std::invalid_argument("too many ZT-assigned IP addresses"); + std::sort(_localRoutes.begin(),_localRoutes.end()); + _localRoutes.erase(std::unique(_localRoutes.begin(),_localRoutes.end()),_localRoutes.end()); std::sort(_staticIps.begin(),_staticIps.end()); _staticIps.erase(std::unique(_staticIps.begin(),_staticIps.end()),_staticIps.end()); @@ -201,7 +195,7 @@ bool NetworkConfig::operator==(const NetworkConfig &nc) const if (_private != nc._private) return false; if (_enableBroadcast != nc._enableBroadcast) return false; if (_name != nc._name) return false; - if (_description != nc._description) return false; + if (_localRoutes != nc._localRoutes) return false; if (_staticIps != nc._staticIps) return false; if (_gateways != nc._gateways) return false; if (_activeBridges != nc._activeBridges) return false; @@ -211,4 +205,3 @@ bool NetworkConfig::operator==(const NetworkConfig &nc) const } } // namespace ZeroTier - diff --git a/node/NetworkConfig.hpp b/node/NetworkConfig.hpp index 5c7cdd7c0..6111e65ba 100644 --- a/node/NetworkConfig.hpp +++ b/node/NetworkConfig.hpp @@ -47,59 +47,48 @@ namespace ZeroTier { +// Fields for meta-data sent with network config requests +#define ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_MAJOR_VERSION "majv" +#define ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_MINOR_VERSION "minv" +#define ZT_NETWORKCONFIG_REQUEST_METADATA_KEY_NODE_REVISION "revv" + // These dictionary keys are short so they don't take up much room in // netconf response packets. // integer(hex)[,integer(hex),...] #define ZT_NETWORKCONFIG_DICT_KEY_ALLOWED_ETHERNET_TYPES "et" - // network ID #define ZT_NETWORKCONFIG_DICT_KEY_NETWORK_ID "nwid" - // integer(hex) #define ZT_NETWORKCONFIG_DICT_KEY_TIMESTAMP "ts" - // integer(hex) #define ZT_NETWORKCONFIG_DICT_KEY_REVISION "r" - // address of member #define ZT_NETWORKCONFIG_DICT_KEY_ISSUED_TO "id" - // integer(hex) #define ZT_NETWORKCONFIG_DICT_KEY_MULTICAST_LIMIT "ml" - // 0/1 #define ZT_NETWORKCONFIG_DICT_KEY_PRIVATE "p" - // text #define ZT_NETWORKCONFIG_DICT_KEY_NAME "n" - // text #define ZT_NETWORKCONFIG_DICT_KEY_DESC "d" - // IP/bits[,IP/bits,...] // Note that IPs that end in all zeroes are routes with no assignment in them. #define ZT_NETWORKCONFIG_DICT_KEY_IPV4_STATIC "v4s" - // IP/bits[,IP/bits,...] // Note that IPs that end in all zeroes are routes with no assignment in them. #define ZT_NETWORKCONFIG_DICT_KEY_IPV6_STATIC "v6s" - // serialized CertificateOfMembership #define ZT_NETWORKCONFIG_DICT_KEY_CERTIFICATE_OF_MEMBERSHIP "com" - // 0/1 #define ZT_NETWORKCONFIG_DICT_KEY_ENABLE_BROADCAST "eb" - // 0/1 #define ZT_NETWORKCONFIG_DICT_KEY_ALLOW_PASSIVE_BRIDGING "pb" - // node[,node,...] #define ZT_NETWORKCONFIG_DICT_KEY_ACTIVE_BRIDGES "ab" - // node;IP/port[,node;IP/port] #define ZT_NETWORKCONFIG_DICT_KEY_RELAYS "rl" - // IP/metric[,IP/metric,...] #define ZT_NETWORKCONFIG_DICT_KEY_GATEWAYS "gw" @@ -158,7 +147,7 @@ public: inline bool isPublic() const throw() { return (!_private); } inline bool isPrivate() const throw() { return _private; } inline const std::string &name() const throw() { return _name; } - inline const std::string &description() const throw() { return _description; } + inline const std::vector &localRoutes() const throw() { return _localRoutes; } inline const std::vector &staticIps() const throw() { return _staticIps; } inline const std::vector &gateways() const throw() { return _gateways; } inline const std::vector
&activeBridges() const throw() { return _activeBridges; } @@ -194,7 +183,7 @@ private: bool _private; bool _enableBroadcast; std::string _name; - std::string _description; + std::vector _localRoutes; std::vector _staticIps; std::vector _gateways; std::vector
_activeBridges; @@ -207,4 +196,3 @@ private: } // namespace ZeroTier #endif - diff --git a/node/NetworkController.hpp b/node/NetworkController.hpp index 265ee3d44..bef884de5 100644 --- a/node/NetworkController.hpp +++ b/node/NetworkController.hpp @@ -52,10 +52,9 @@ public: enum ResultCode { NETCONF_QUERY_OK = 0, - NETCONF_QUERY_OK_BUT_NOT_NEWER = 1, - NETCONF_QUERY_OBJECT_NOT_FOUND = 2, - NETCONF_QUERY_ACCESS_DENIED = 3, - NETCONF_QUERY_INTERNAL_SERVER_ERROR = 4 + NETCONF_QUERY_OBJECT_NOT_FOUND = 1, + NETCONF_QUERY_ACCESS_DENIED = 2, + NETCONF_QUERY_INTERNAL_SERVER_ERROR = 3 }; NetworkController() {} diff --git a/version.h b/version.h index f7b253a7b..62f8fb697 100644 --- a/version.h +++ b/version.h @@ -41,6 +41,6 @@ /** * Revision */ -#define ZEROTIER_ONE_VERSION_REVISION 3 +#define ZEROTIER_ONE_VERSION_REVISION 4 #endif