Merge branch 'windns-improve-error-handling'

8 files changed, 139 insertions, 83 deletions

diff --git a/windows/windns/src/windns/confineoperation.cpp b/windows/windns/src/windns/confineoperation.cpp
index 5c16de71bf..a43bbd0d2e 100644
--- a/windows/windns/src/windns/confineoperation.cpp
+++ b/windows/windns/src/windns/confineoperation.cpp
@@ -50,6 +50,24 @@ bool ConfineOperation
 	}
 }
 
+bool ConfineOperation
+(
+	const char *literalOperation,
+	ILogSink *logSink,
+	std::function<void()> operation
+)
+{
+	auto ForwardError = [logSink](const char *error, const char **details, uint32_t numDetails)
+	{
+		if (nullptr != logSink)
+		{
+			logSink->error(error, details, numDetails);
+		}
+	};
+
+	return ConfineOperation(literalOperation, ForwardError, operation);
+}
+
 std::vector<uint8_t> CreateRawStringArray(const std::vector<std::string> &arr)
 {
 	//
diff --git a/windows/windns/src/windns/confineoperation.h b/windows/windns/src/windns/confineoperation.h
index 21fe7dd996..4c37058da2 100644
--- a/windows/windns/src/windns/confineoperation.h
+++ b/windows/windns/src/windns/confineoperation.h
@@ -1,5 +1,6 @@
 #pragma once
 
+#include "ilogsink.h"
 #include <functional>
 #include <vector>
 #include <string>
@@ -12,6 +13,13 @@ bool ConfineOperation
 	std::function<void()> operation
 );
 
+bool ConfineOperation
+(
+	const char *literalOperation,
+	ILogSink *logSink,
+	std::function<void()> operation
+);
+
 //
 // The returned buffer looks like this:
 //
diff --git a/windows/windns/src/windns/dnsagent.cpp b/windows/windns/src/windns/dnsagent.cpp
index 6ff8ee9b39..e9b3a4246d 100644
--- a/windows/windns/src/windns/dnsagent.cpp
+++ b/windows/windns/src/windns/dnsagent.cpp
@@ -2,8 +2,10 @@
 #include "dnsagent.h"
 #include "registrypaths.h"
 #include "netsh.h"
+#include "confineoperation.h"
 #include <libcommon/trace/xtrace.h>
 #include <libcommon/error.h>
+#include <libcommon/string.h>
 #include <process.h>
 #include <algorithm>
 
@@ -214,15 +216,23 @@ void DnsAgent::processServerSourceEvent()
 		return interfaceData.interfaceGuid;
 	});
 
-	const auto updatedSnaps = createSnaps(interfaces);
 	const auto enforcedServers = m_nameServerSource->getNameServers(m_protocol);
 
-	for (const auto snap : updatedSnaps)
+	for (const auto &iface : interfaces)
 	{
-		if (snap.needsOverriding(enforcedServers))
+		const auto literalOperation = std::wstring(L"Verifying settings on interface ").append(iface);
+
+		XTRACE(literalOperation);
+
+		ConfineOperation(common::string::ToAnsi(literalOperation).c_str(), m_logSink, [&]()
 		{
-			setNameServers(snap.interfaceGuid(), enforcedServers);
-		}
+			InterfaceSnap snap(m_protocol, iface);
+
+			if (snap.needsOverriding(enforcedServers))
+			{
+				setNameServers(iface, enforcedServers);
+			}
+		});
 	}
 }
 
@@ -293,36 +303,32 @@ DnsAgent::ProcessingResult DnsAgent::processInterfaceEvent(const HANDLE *interfa
 			continue;
 		}
 
-		auto &interface = m_trackedInterfaces[i];
+		auto &iface = m_trackedInterfaces[i];
+
+		const auto literalOperation = std::wstring(L"Processing event for interface ").append(iface.interfaceGuid);
 
-		XTRACE(L"Processing event for interface ", interface.interfaceGuid);
+		XTRACE(literalOperation);
 
-		try
+		ConfineOperation(common::string::ToAnsi(literalOperation).c_str(), m_logSink, [&]()
 		{
-			InterfaceSnap updatedSnap(m_protocol, interface.interfaceGuid);
+			InterfaceSnap updatedSnap(m_protocol, iface.interfaceGuid);
 
-			if (updatedSnap.needsOverriding(enforcedNameServers))
+			if ((iface.preservedSettings.internalInterface() && updatedSnap.internalInterface())
+				|| updatedSnap.nameServers() == enforcedNameServers)
 			{
-				result = ProcessingResult::TrackingUpdated;
-
-				interface.preservedSettings = std::move(updatedSnap);
-				setNameServers(interface.interfaceGuid, enforcedNameServers);
+				return;
 			}
-		}
-		catch (std::exception &err)
-		{
-			const char *what = err.what();
 
-			m_logSink->error("Could not fetch updated interface settings. Probably because the interface was removed.", &what, 1);
+			const auto shouldOverride = updatedSnap.needsOverriding(enforcedNameServers);
 
-			continue;
-		}
-		catch (...)
-		{
-			m_logSink->error("Could not fetch updated interface settings. Probably because the interface was removed.");
+			result = ProcessingResult::TrackingUpdated;
+			iface.preservedSettings = std::move(updatedSnap);
 
-			continue;
-		}
+			if (shouldOverride)
+			{
+				setNameServers(iface.interfaceGuid, enforcedNameServers);
+			}
+		});
 	}
 
 	return result;
@@ -345,63 +351,68 @@ std::vector<std::wstring> DnsAgent::discoverInterfaces()
 	return interfaces;
 }
 
-std::vector<InterfaceSnap> DnsAgent::createSnaps(const std::vector<std::wstring> &interfaces)
+void DnsAgent::setNameServers(const std::wstring &interfaceGuid, const std::vector<std::wstring> &enforcedServers)
 {
-	std::vector<InterfaceSnap> snaps;
+	XTRACE(L"Overriding name servers for interface ", interfaceGuid);
 
-	snaps.reserve(interfaces.size());
+	uint32_t interfaceIndex = 0;
 
-	for (const auto &interface : interfaces)
+	try
 	{
-		snaps.emplace_back(m_protocol, interface);
+		interfaceIndex = NetSh::ConvertInterfaceGuidToIndex(interfaceGuid);
 	}
+	catch (...)
+	{
+		//
+		// The interface cannot be linked to a virtual or physical adapter.
+		//
 
-	return snaps;
-}
-
-void DnsAgent::setNameServers(const std::wstring &interfaceGuid, const std::vector<std::wstring> &enforcedServers)
-{
-	XTRACE(L"Overriding name servers for interface ", interfaceGuid);
+		XTRACE(L"Ignoring floating interface ", interfaceGuid);
+		return;
+	}
 
 	if (Protocol::IPv4 == m_protocol)
 	{
-		NetSh::Instance().SetIpv4StaticDns(NetSh::ConvertInterfaceGuidToIndex(interfaceGuid), enforcedServers);
+		NetSh::Instance().SetIpv4StaticDns(interfaceIndex, enforcedServers);
 	}
 	else
 	{
-		NetSh::Instance().SetIpv6StaticDns(NetSh::ConvertInterfaceGuidToIndex(interfaceGuid), enforcedServers);
+		NetSh::Instance().SetIpv6StaticDns(interfaceIndex, enforcedServers);
 	}
 }
 
 void DnsAgent::startTrackingInterfaces(const std::vector<std::wstring> &interfaces)
 {
-	const auto snaps = createSnaps(interfaces);
-
-	//
-	// Override configured name servers on all interfaces, as necessary.
-	//
-
 	const auto enforcedServers = m_nameServerSource->getNameServers(m_protocol);
 
-	for (const auto &snap : snaps)
+	for (const auto &iface : interfaces)
 	{
-		if (snap.needsOverriding(enforcedServers))
+		const auto literalOperation = std::wstring(L"Start tracking interface ").append(iface);
+
+		XTRACE(literalOperation);
+
+		ConfineOperation(common::string::ToAnsi(literalOperation).c_str(), m_logSink, [&]()
 		{
-			setNameServers(snap.interfaceGuid(), enforcedServers);
-		}
-	}
+			InterfaceSnap snap(m_protocol, iface);
 
-	//
-	// Create a tracking record for each interface.
-	//
+			const auto shouldOverride = snap.needsOverriding(enforcedServers);
 
-	for (const auto &snap : snaps)
-	{
-		const auto interfaceGuid = snap.interfaceGuid();
+			//
+			// Create a tracking record.
+			//
+
+			m_trackedInterfaces.emplace_back(iface, std::move(snap),
+				std::make_unique<InterfaceMonitor>(m_protocol, iface));
 
-		XTRACE(L"Creating tracking entry for interface ", interfaceGuid);
+			//
+			// Override configured name servers, as necessary.
+			//
 
-		m_trackedInterfaces.emplace_back(interfaceGuid, snap, std::make_unique<InterfaceMonitor>(m_protocol, interfaceGuid));
+			if (shouldOverride)
+			{
+				setNameServers(iface, enforcedServers);
+			}
+		});
 	}
 }
 
diff --git a/windows/windns/src/windns/dnsagent.h b/windows/windns/src/windns/dnsagent.h
index 48dd12c52d..db73257330 100644
--- a/windows/windns/src/windns/dnsagent.h
+++ b/windows/windns/src/windns/dnsagent.h
@@ -73,7 +73,6 @@ private:
 	ProcessingResult processInterfaceEvent(const HANDLE *interfaceEvents, size_t startIndex);
 
 	std::vector<std::wstring> discoverInterfaces();
-	std::vector<InterfaceSnap> createSnaps(const std::vector<std::wstring> &interfaces);
 
 	void setNameServers(const std::wstring &interfaceGuid, const std::vector<std::wstring> &enforcedServers);
 
diff --git a/windows/windns/src/windns/interfacesnap.cpp b/windows/windns/src/windns/interfacesnap.cpp
index 309296897f..1840776ed3 100644
--- a/windows/windns/src/windns/interfacesnap.cpp
+++ b/windows/windns/src/windns/interfacesnap.cpp
@@ -81,7 +81,7 @@ InterfaceSnap::InterfaceSnap(Protocol protocol, const std::wstring &interfaceGui
 	// DHCP name servers are the servers most recently supplied by DHCP.
 	// An adapter can be configured for DHCP and static name servers at the same time.
 	// Static name servers always have precedence.
-	m_dhcpNameServers = GetNameServers(m_interfaceGuid, m_protocol, NameServerType::Dhcp);
+	//m_dhcpNameServers = GetNameServers(m_interfaceGuid, m_protocol, NameServerType::Dhcp);
 }
 
 InterfaceSnap::InterfaceSnap(common::serialization::Deserializer &deserializer)
@@ -99,7 +99,6 @@ InterfaceSnap::InterfaceSnap(common::serialization::Deserializer &deserializer)
 	d >> m_interfaceGuid;
 	d >> (uint8_t &)m_configuredForDhcp;
 	d >> m_staticNameServers;
-	d >> m_dhcpNameServers;
 }
 
 void InterfaceSnap::serialize(common::serialization::Serializer &serializer) const
@@ -110,7 +109,6 @@ void InterfaceSnap::serialize(common::serialization::Serializer &serializer) con
 	s << m_interfaceGuid;
 	s << (uint8_t &)m_configuredForDhcp;
 	s << m_staticNameServers;
-	s << m_dhcpNameServers;
 }
 
 bool InterfaceSnap::needsOverriding(const std::vector<std::wstring> &enforcedServers) const
diff --git a/windows/windns/src/windns/interfacesnap.h b/windows/windns/src/windns/interfacesnap.h
index 4d1156b615..5159f581eb 100644
--- a/windows/windns/src/windns/interfacesnap.h
+++ b/windows/windns/src/windns/interfacesnap.h
@@ -29,7 +29,5 @@ private:
 	std::wstring m_interfaceGuid;
 
 	bool m_configuredForDhcp;
-
 	std::vector<std::wstring> m_staticNameServers;
-	std::vector<std::wstring> m_dhcpNameServers;
 };
diff --git a/windows/windns/src/windns/recoverylogic.cpp b/windows/windns/src/windns/recoverylogic.cpp
index 0c630a89b2..70fe0b8a91 100644
--- a/windows/windns/src/windns/recoverylogic.cpp
+++ b/windows/windns/src/windns/recoverylogic.cpp
@@ -14,16 +14,11 @@ void RecoveryLogic::RestoreInterfaces(const RecoveryFormatter::Unpacked &data,
 		throw std::runtime_error("Invalid logger sink");
 	}
 
-	auto forwardError = [logSink](const char *msg, const char **details, uint32_t numDetails)
-	{
-		logSink->error(msg, details, numDetails);
-	};
-
 	bool success = true;
 
 	for (const auto &snap : data.v4Snaps)
 	{
-		const auto status = ConfineOperation("Reset interface DNS settings", forwardError, [&snap, &timeout]()
+		const auto status = ConfineOperation("Reset interface DNS settings", logSink, [&snap, &timeout]()
 		{
 			if (snap.internalInterface())
 			{
@@ -34,15 +29,32 @@ void RecoveryLogic::RestoreInterfaces(const RecoveryFormatter::Unpacked &data,
 				return;
 			}
 
-			XTRACE("Resetting name server configuration for interface ", snap.interfaceGuid());
+			XTRACE(L"Resetting name server configuration for interface ", snap.interfaceGuid());
+
+			uint32_t interfaceIndex = 0;
+
+			try
+			{
+				interfaceIndex = NetSh::ConvertInterfaceGuidToIndex(snap.interfaceGuid());
+			}
+			catch (...)
+			{
+				//
+				// The interface cannot be linked to a virtual or physical adapter.
+				// It's either floating or has been removed.
+				//
+
+				XTRACE(L"Ignoring floating/invalid interface ", snap.interfaceGuid());
+				return;
+			}
 
 			if (snap.nameServers().empty())
 			{
-				NetSh::Instance().SetIpv4DhcpDns(NetSh::ConvertInterfaceGuidToIndex(snap.interfaceGuid()), timeout);
+				NetSh::Instance().SetIpv4DhcpDns(interfaceIndex, timeout);
 			}
 			else
 			{
-				NetSh::Instance().SetIpv4StaticDns(NetSh::ConvertInterfaceGuidToIndex(snap.interfaceGuid()), snap.nameServers(), timeout);
+				NetSh::Instance().SetIpv4StaticDns(interfaceIndex, snap.nameServers(), timeout);
 			}
 		});
 
@@ -54,7 +66,7 @@ void RecoveryLogic::RestoreInterfaces(const RecoveryFormatter::Unpacked &data,
 
 	for (const auto &snap : data.v6Snaps)
 	{
-		const auto status = ConfineOperation("Reset interface DNS settings", forwardError, [&snap, &timeout]()
+		const auto status = ConfineOperation("Reset interface DNS settings", logSink, [&snap, &timeout]()
 		{
 			if (snap.internalInterface())
 			{
@@ -65,15 +77,32 @@ void RecoveryLogic::RestoreInterfaces(const RecoveryFormatter::Unpacked &data,
 				return;
 			}
 
-			XTRACE("Resetting name server configuration for interface ", snap.interfaceGuid());
+			XTRACE(L"Resetting name server configuration for interface ", snap.interfaceGuid());
+
+			uint32_t interfaceIndex = 0;
+
+			try
+			{
+				interfaceIndex = NetSh::ConvertInterfaceGuidToIndex(snap.interfaceGuid());
+			}
+			catch (...)
+			{
+				//
+				// The interface cannot be linked to a virtual or physical adapter.
+				// It's either floating or has been removed.
+				//
+
+				XTRACE(L"Ignoring floating/invalid interface ", snap.interfaceGuid());
+				return;
+			}
 
 			if (snap.nameServers().empty())
 			{
-				NetSh::Instance().SetIpv6DhcpDns(NetSh::ConvertInterfaceGuidToIndex(snap.interfaceGuid()), timeout);
+				NetSh::Instance().SetIpv6DhcpDns(interfaceIndex, timeout);
 			}
 			else
 			{
-				NetSh::Instance().SetIpv6StaticDns(NetSh::ConvertInterfaceGuidToIndex(snap.interfaceGuid()), snap.nameServers(), timeout);
+				NetSh::Instance().SetIpv6StaticDns(interfaceIndex, snap.nameServers(), timeout);
 			}
 		});
 
diff --git a/windows/windns/src/windns/windnscontext.cpp b/windows/windns/src/windns/windnscontext.cpp
index cf80c0e619..7c95358652 100644
--- a/windows/windns/src/windns/windnscontext.cpp
+++ b/windows/windns/src/windns/windnscontext.cpp
@@ -18,12 +18,7 @@ WinDnsContext::WinDnsContext(ILogSink *logSink)
 
 WinDnsContext::~WinDnsContext()
 {
-	auto forwardError = [this](const char *msg, const char **details, uint32_t numDetails)
-	{
-		m_logSink->error(msg, details, numDetails);
-	};
-
-	ConfineOperation("Reset DNS settings", forwardError, [this]()
+	ConfineOperation("Reset DNS settings", m_logSink, [this]()
 	{
 		this->reset();
 	});