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

@@ -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;

node/IncomingPacket.cpp

@@ -392,28 +392,7 @@ bool IncomingPacket::_doOK(const RuntimeEnvironment *RR,const SharedPtr<Peer> &p
 					const unsigned int dictlen = at<uint16_t>(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<NetworkConfig> 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);

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)

node/NetworkConfig.cpp

@@ -47,7 +47,6 @@ SharedPtr<NetworkConfig> 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
-

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<InetAddress> &localRoutes() const throw() { return _localRoutes; }
 	inline const std::vector<InetAddress> &staticIps() const throw() { return _staticIps; }
 	inline const std::vector<InetAddress> &gateways() const throw() { return _gateways; }
 	inline const std::vector<Address> &activeBridges() const throw() { return _activeBridges; }
@@ -194,7 +183,7 @@ private:
 	bool _private;
 	bool _enableBroadcast;
 	std::string _name;
-	std::string _description;
+	std::vector<InetAddress> _localRoutes;
 	std::vector<InetAddress> _staticIps;
 	std::vector<InetAddress> _gateways;
 	std::vector<Address> _activeBridges;
@@ -207,4 +196,3 @@ private:
 } // namespace ZeroTier
 
 #endif
-

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() {}

version.h

@@ -41,6 +41,6 @@
 /**
  * Revision
  */
-#define ZEROTIER_ONE_VERSION_REVISION 3
+#define ZEROTIER_ONE_VERSION_REVISION 4
 
 #endif