[CrOS Network] Safely handle IKEv2 and unknown VPN providers in metrics.

This change updates the metrics reporting related to VPNs so that both
an IKEv2 and "unknown" provider can be reported.

Bug: b/211046884,b/207688917
Test: Ran affected unit tests.

Change-Id: I4efd4a2299580d11c9839c2eee3110786edb8b1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3404721
Reviewed-by: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Chad Duffin <chadduffin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#985519}

chromeos/network/metrics/network_metrics_helper.cc

@@ -41,6 +41,7 @@ const char kTether[] = "Tether";
 const char kVPN[] = "VPN";
 const char kVPNBuiltIn[] = "VPN.TypeBuiltIn";
 const char kVPNThirdParty[] = "VPN.TypeThirdParty";
+const char kVPNUnknown[] = "VPN.TypeUnknown";
 
 const char kWifi[] = "WiFi";
 const char kWifiOpen[] = "WiFi.SecurityOpen";
@@ -123,12 +124,14 @@ const std::vector<std::string> GetVpnNetworkTypeHistograms(
   if (vpn_provider_type == shill::kProviderThirdPartyVpn ||
       vpn_provider_type == shill::kProviderArcVpn) {
     vpn_histograms.emplace_back(kVPNThirdParty);
-  } else if (vpn_provider_type == shill::kProviderL2tpIpsec ||
+  } else if (vpn_provider_type == shill::kProviderIKEv2 ||
+             vpn_provider_type == shill::kProviderL2tpIpsec ||
              vpn_provider_type == shill::kProviderOpenVpn ||
              vpn_provider_type == shill::kProviderWireGuard) {
     vpn_histograms.emplace_back(kVPNBuiltIn);
   } else {
     NOTREACHED();
+    vpn_histograms.emplace_back(kVPNUnknown);
   }
   return vpn_histograms;
 }

chromeos/network/metrics/network_metrics_helper_unittest.cc

@@ -14,6 +14,7 @@
 #include "chromeos/network/network_handler_test_helper.h"
 #include "chromeos/network/network_ui_data.h"
 #include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest-spi.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
@@ -40,6 +41,8 @@ const char kVpnBuiltInConnectResultAllHistogram[] =
     "Network.Ash.VPN.TypeBuiltIn.ConnectionResult.All";
 const char kVpnThirdPartyConnectResultAllHistogram[] =
     "Network.Ash.VPN.TypeThirdParty.ConnectionResult.All";
+const char kVpnUnknownConnectResultAllHistogram[] =
+    "Network.Ash.VPN.TypeUnknown.ConnectionResult.All";
 
 // LogAllConnectionResult() WiFi histograms.
 const char kWifiConnectResultAllHistogram[] =
@@ -74,6 +77,8 @@ const char kVpnBuiltInConnectResultUserInitiatedHistogram[] =
     "Network.Ash.VPN.TypeBuiltIn.ConnectionResult.UserInitiated";
 const char kVpnThirdPartyConnectResultUserInitiatedHistogram[] =
     "Network.Ash.VPN.TypeThirdParty.ConnectionResult.UserInitiated";
+const char kVpnUnknownConnectResultUserInitiatedHistogram[] =
+    "Network.Ash.VPN.TypeUnknown.ConnectionResult.UserInitiated";
 
 // LogUserInitiatedConnectionResult() WiFi histograms.
 const char kWifiConnectResultUserInitiatedHistogram[] =
@@ -108,6 +113,8 @@ const char kVpnBuiltInConnectionStateHistogram[] =
     "Network.Ash.VPN.TypeBuiltIn.DisconnectionsWithoutUserAction";
 const char kVpnThirdPartyConnectionStateHistogram[] =
     "Network.Ash.VPN.TypeThirdParty.DisconnectionsWithoutUserAction";
+const char kVpnUnknownConnectionStateHistogram[] =
+    "Network.Ash.VPN.TypeUnknown.DisconnectionsWithoutUserAction";
 
 // LogConnectionStateResult() WiFi histograms.
 const char kWifiConnectionStateHistogram[] =
@@ -152,6 +159,25 @@ const char kTestServicePath1[] = "/service/network1";
 const char kTestDevicePath[] = "/device/network";
 const char kTestName[] = "network_name";
 const char kTestVpnHost[] = "test host";
+const char kTestUnknownVpn[] = "test_unknown_vpn";
+
+void LogVpnResult(const std::string& provider,
+                  base::RepeatingClosure func,
+                  bool* failed_to_log_result) {
+  ASSERT_NE(failed_to_log_result, nullptr);
+
+// Emitting a metric for an unknown VPN provider will always cause a NOTREACHED
+// to be hit. This can cause a CHECK to fail, depending on the build flags. We
+// catch any failing CHECK below by asserting that we will crash when emitting.
+#if !BUILDFLAG(ENABLE_LOG_ERROR_NOT_REACHED)
+  if (provider == kTestUnknownVpn) {
+    ASSERT_DEATH({ func.Run(); }, "");
+    *failed_to_log_result = true;
+    return;
+  }
+#endif  // !BUILDFLAG(ENABLE_LOG_ERROR_NOT_REACHED)
+  func.Run();
+}
 
 }  // namespace
 
@@ -334,19 +360,35 @@ TEST_F(NetworkMetricsHelperTest, CellularPSim) {
 
 TEST_F(NetworkMetricsHelperTest, VPN) {
   const std::vector<const std::string> kProviders{{
+      shill::kProviderIKEv2,
       shill::kProviderL2tpIpsec,
       shill::kProviderArcVpn,
       shill::kProviderOpenVpn,
       shill::kProviderThirdPartyVpn,
       shill::kProviderWireGuard,
+      kTestUnknownVpn,
   }};
 
   size_t expected_all_count = 0;
   size_t expected_user_initiated_count = 0;
   size_t expected_built_in_count = 0;
   size_t expected_third_party_count = 0;
+  size_t expected_unknown_count = 0;
+
+  base::RepeatingClosure log_all_connection_result =
+      base::BindRepeating(&NetworkMetricsHelper::LogAllConnectionResult,
+                          kTestGuid, shill::kErrorNotRegistered);
+  base::RepeatingClosure log_user_initiated_connection_result =
+      base::BindRepeating(
+          &NetworkMetricsHelper::LogUserInitiatedConnectionResult, kTestGuid,
+          shill::kErrorNotRegistered);
+  base::RepeatingClosure log_connection_state_result = base::BindRepeating(
+      &NetworkMetricsHelper::LogConnectionStateResult, kTestGuid,
+      NetworkMetricsHelper::ConnectionState::kConnected);
 
   for (const auto& provider : kProviders) {
+    bool failed_to_log_result = false;
+
     shill_service_client_->AddService(kTestServicePath, kTestGuid, kTestName,
                                       shill::kTypeVPN, shill::kStateIdle,
                                       /*visible=*/true);
@@ -357,26 +399,36 @@ TEST_F(NetworkMetricsHelperTest, VPN) {
                                               base::Value(kTestVpnHost));
     base::RunLoop().RunUntilIdle();
 
-    if (provider == shill::kProviderThirdPartyVpn ||
-        provider == shill::kProviderArcVpn) {
-      ++expected_third_party_count;
-    } else {
-      ++expected_built_in_count;
+    LogVpnResult(provider, log_all_connection_result, &failed_to_log_result);
+    LogVpnResult(provider, log_user_initiated_connection_result,
+                 &failed_to_log_result);
+    LogVpnResult(provider, log_connection_state_result, &failed_to_log_result);
+
+    if (!failed_to_log_result) {
+      if (provider == shill::kProviderThirdPartyVpn ||
+          provider == shill::kProviderArcVpn) {
+        ++expected_third_party_count;
+      } else if (provider == shill::kProviderIKEv2 ||
+                 provider == shill::kProviderL2tpIpsec ||
+                 provider == shill::kProviderOpenVpn ||
+                 provider == shill::kProviderWireGuard) {
+        ++expected_built_in_count;
+      } else {
+        ++expected_unknown_count;
+      }
+      ++expected_all_count;
+      ++expected_user_initiated_count;
     }
-    ++expected_all_count;
-    ++expected_user_initiated_count;
 
-    NetworkMetricsHelper::LogAllConnectionResult(kTestGuid,
-                                                 shill::kErrorNotRegistered);
     histogram_tester_->ExpectTotalCount(kVpnConnectResultAllHistogram,
                                         expected_all_count);
     histogram_tester_->ExpectTotalCount(kVpnBuiltInConnectResultAllHistogram,
                                         expected_built_in_count);
     histogram_tester_->ExpectTotalCount(kVpnThirdPartyConnectResultAllHistogram,
                                         expected_third_party_count);
+    histogram_tester_->ExpectTotalCount(kVpnUnknownConnectResultAllHistogram,
+                                        expected_unknown_count);
 
-    NetworkMetricsHelper::LogUserInitiatedConnectionResult(
-        kTestGuid, shill::kErrorNotRegistered);
     histogram_tester_->ExpectTotalCount(kVpnConnectResultUserInitiatedHistogram,
                                         expected_user_initiated_count);
     histogram_tester_->ExpectTotalCount(
@@ -385,15 +437,17 @@ TEST_F(NetworkMetricsHelperTest, VPN) {
     histogram_tester_->ExpectTotalCount(
         kVpnThirdPartyConnectResultUserInitiatedHistogram,
         expected_third_party_count);
+    histogram_tester_->ExpectTotalCount(
+        kVpnUnknownConnectResultUserInitiatedHistogram, expected_unknown_count);
 
-    NetworkMetricsHelper::LogConnectionStateResult(
-        kTestGuid, NetworkMetricsHelper::ConnectionState::kConnected);
     histogram_tester_->ExpectTotalCount(kVpnConnectionStateHistogram,
                                         expected_user_initiated_count);
     histogram_tester_->ExpectTotalCount(kVpnBuiltInConnectionStateHistogram,
                                         expected_built_in_count);
     histogram_tester_->ExpectTotalCount(kVpnThirdPartyConnectionStateHistogram,
                                         expected_third_party_count);
+    histogram_tester_->ExpectTotalCount(kVpnUnknownConnectionStateHistogram,
+                                        expected_unknown_count);
 
     shill_service_client_->RemoveService(kTestServicePath);
     base::RunLoop().RunUntilIdle();

chromeos/network/metrics/vpn_network_metrics_helper.cc

@@ -20,6 +20,8 @@ namespace {
 // sources of created VPNs.
 const char kVpnConfigurationSourceBucketArc[] =
     "Network.Ash.VPN.ARC.ConfigurationSource";
+const char kVpnConfigurationSourceBucketIKEv2[] =
+    "Network.Ash.VPN.IKEv2.ConfigurationSource";
 const char kVpnConfigurationSourceBucketL2tpIpsec[] =
     "Network.Ash.VPN.L2TPIPsec.ConfigurationSource";
 const char kVpnConfigurationSourceBucketOpenVpn[] =
@@ -28,10 +30,14 @@ const char kVpnConfigurationSourceBucketThirdParty[] =
     "Network.Ash.VPN.ThirdParty.ConfigurationSource";
 const char kVpnConfigurationSourceBucketWireGuard[] =
     "Network.Ash.VPN.WireGuard.ConfigurationSource";
+const char kVpnConfigurationSourceBucketUnknown[] =
+    "Network.Ash.VPN.Unknown.ConfigurationSource";
 
 const char* GetBucketForVpnProviderType(const std::string& vpn_provider_type) {
   if (vpn_provider_type == shill::kProviderArcVpn) {
     return kVpnConfigurationSourceBucketArc;
+  } else if (vpn_provider_type == shill::kProviderIKEv2) {
+    return kVpnConfigurationSourceBucketIKEv2;
   } else if (vpn_provider_type == shill::kProviderL2tpIpsec) {
     return kVpnConfigurationSourceBucketL2tpIpsec;
   } else if (vpn_provider_type == shill::kProviderOpenVpn) {
@@ -41,7 +47,8 @@ const char* GetBucketForVpnProviderType(const std::string& vpn_provider_type) {
   } else if (vpn_provider_type == shill::kProviderWireGuard) {
     return kVpnConfigurationSourceBucketWireGuard;
   }
-  return nullptr;
+  NOTREACHED();
+  return kVpnConfigurationSourceBucketUnknown;
 }
 
 }  // namespace
@@ -71,10 +78,7 @@ void VpnNetworkMetricsHelper::OnConfigurationCreated(
   const char* vpn_provider_type_bucket =
       GetBucketForVpnProviderType(network_state->GetVpnProviderType());
 
-  if (!vpn_provider_type_bucket) {
-    NOTREACHED();
-    return;
-  }
+  DCHECK(vpn_provider_type_bucket);
 
   base::UmaHistogramEnumeration(
       vpn_provider_type_bucket,

chromeos/network/metrics/vpn_network_metrics_helper_unittest.cc

@@ -19,6 +19,7 @@
 #include "chromeos/network/network_ui_data.h"
 #include "chromeos/network/shill_property_util.h"
 #include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest-spi.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
@@ -28,6 +29,8 @@ namespace {
 
 const char kVpnHistogramConfigurationSourceArc[] =
     "Network.Ash.VPN.ARC.ConfigurationSource";
+const char kVpnHistogramConfigurationSourceIKEv2[] =
+    "Network.Ash.VPN.IKEv2.ConfigurationSource";
 const char kVpnHistogramConfigurationSourceL2tpIpsec[] =
     "Network.Ash.VPN.L2TPIPsec.ConfigurationSource";
 const char kVpnHistogramConfigurationSourceOpenVpn[] =
@@ -36,11 +39,45 @@ const char kVpnHistogramConfigurationSourceThirdParty[] =
     "Network.Ash.VPN.ThirdParty.ConfigurationSource";
 const char kVpnHistogramConfigurationSourceWireGuard[] =
     "Network.Ash.VPN.WireGuard.ConfigurationSource";
+const char kVpnHistogramConfigurationSourceUnknown[] =
+    "Network.Ash.VPN.Unknown.ConfigurationSource";
+
+const char kTestUnknownVpn[] = "test_unknown_vpn";
 
 void ErrorCallback(const std::string& error_name) {
   ADD_FAILURE() << "Unexpected error: " << error_name;
 }
 
+// Helper function to create a VPN network using NetworkConfigurationHandler.
+void CreateTestShillConfiguration(const std::string& vpn_provider_type,
+                                  bool is_managed) {
+  base::Value properties(base::Value::Type::DICTIONARY);
+
+  properties.SetKey(shill::kGuidProperty, base::Value("vpn_guid"));
+  properties.SetKey(shill::kTypeProperty, base::Value(shill::kTypeVPN));
+  properties.SetKey(shill::kStateProperty, base::Value(shill::kStateIdle));
+  properties.SetKey(shill::kProviderHostProperty, base::Value("vpn_host"));
+  properties.SetKey(shill::kProviderTypeProperty,
+                    base::Value(vpn_provider_type));
+  properties.SetKey(shill::kProfileProperty,
+                    base::Value(NetworkProfileHandler::GetSharedProfilePath()));
+
+  if (is_managed) {
+    properties.SetKey(shill::kONCSourceProperty,
+                      base::Value(shill::kONCSourceDevicePolicy));
+    std::unique_ptr<NetworkUIData> ui_data = NetworkUIData::CreateFromONC(
+        ::onc::ONCSource::ONC_SOURCE_DEVICE_POLICY);
+    properties.SetKey(shill::kUIDataProperty,
+                      base::Value(ui_data->GetAsJson()));
+  }
+
+  NetworkHandler::Get()
+      ->network_configuration_handler()
+      ->CreateShillConfiguration(properties, base::DoNothing(),
+                                 base::BindOnce(&ErrorCallback));
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace
 
 class VpnNetworkMetricsHelperTest : public testing::Test {
@@ -68,37 +105,6 @@ class VpnNetworkMetricsHelperTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
-  // Helper function to create a VPN network using NetworkConfigurationHandler.
-  void CreateTestShillConfiguration(const char* vpn_provider_type,
-                                    bool is_managed) {
-    base::Value properties(base::Value::Type::DICTIONARY);
-
-    properties.SetKey(shill::kGuidProperty, base::Value("vpn_guid"));
-    properties.SetKey(shill::kTypeProperty, base::Value(shill::kTypeVPN));
-    properties.SetKey(shill::kStateProperty, base::Value(shill::kStateIdle));
-    properties.SetKey(shill::kProviderHostProperty, base::Value("vpn_host"));
-    properties.SetKey(shill::kProviderTypeProperty,
-                      base::Value(vpn_provider_type));
-    properties.SetKey(
-        shill::kProfileProperty,
-        base::Value(NetworkProfileHandler::GetSharedProfilePath()));
-
-    if (is_managed) {
-      properties.SetKey(shill::kONCSourceProperty,
-                        base::Value(shill::kONCSourceDevicePolicy));
-      std::unique_ptr<NetworkUIData> ui_data = NetworkUIData::CreateFromONC(
-          ::onc::ONCSource::ONC_SOURCE_DEVICE_POLICY);
-      properties.SetKey(shill::kUIDataProperty,
-                        base::Value(ui_data->GetAsJson()));
-    }
-
-    NetworkHandler::Get()
-        ->network_configuration_handler()
-        ->CreateShillConfiguration(properties, base::DoNothing(),
-                                   base::BindOnce(&ErrorCallback));
-    base::RunLoop().RunUntilIdle();
-  }
-
   void ExpectConfigurationSourceCounts(const char* histogram,
                                        size_t manual_count,
                                        size_t policy_count) {
@@ -122,8 +128,9 @@ class VpnNetworkMetricsHelperTest : public testing::Test {
 };
 
 TEST_F(VpnNetworkMetricsHelperTest, LogVpnVPNConfigurationSource) {
-  const std::vector<std::pair<const char*, const char*>>
+  const std::vector<std::pair<const std::string, const char*>>
       kProvidersAndHistograms{{
+          {shill::kProviderIKEv2, kVpnHistogramConfigurationSourceIKEv2},
           {shill::kProviderL2tpIpsec,
            kVpnHistogramConfigurationSourceL2tpIpsec},
           {shill::kProviderArcVpn, kVpnHistogramConfigurationSourceArc},
@@ -132,12 +139,36 @@ TEST_F(VpnNetworkMetricsHelperTest, LogVpnVPNConfigurationSource) {
            kVpnHistogramConfigurationSourceThirdParty},
           {shill::kProviderWireGuard,
            kVpnHistogramConfigurationSourceWireGuard},
+          {kTestUnknownVpn, kVpnHistogramConfigurationSourceUnknown},
       }};
 
   for (const auto& it : kProvidersAndHistograms) {
-    ExpectConfigurationSourceCounts(it.first, /*manual_count=*/0,
+    ExpectConfigurationSourceCounts(it.first.c_str(), /*manual_count=*/0,
                                     /*policy_count=*/0);
 
+// Emitting a metric for an unknown VPN provider will always cause a NOTREACHED
+// to be hit. This can cause a CHECK to fail, depending on the build flags. We
+// catch any failing CHECK below by asserting that we will crash when emitting.
+#if !BUILDFLAG(ENABLE_LOG_ERROR_NOT_REACHED)
+    if (it.first == kTestUnknownVpn) {
+      ASSERT_DEATH(
+          {
+            CreateTestShillConfiguration(kTestUnknownVpn, /*is_managed=*/false);
+          },
+          "");
+      ClearServices();
+      ASSERT_DEATH(
+          {
+            CreateTestShillConfiguration(kTestUnknownVpn, /*is_managed=*/true);
+          },
+          "");
+      ClearServices();
+      ExpectConfigurationSourceCounts(it.second, /*manual_count=*/0,
+                                      /*policy_count=*/0);
+      continue;
+    }
+#endif  // !BUILDFLAG(ENABLE_LOG_ERROR_NOT_REACHED)
+
     CreateTestShillConfiguration(it.first, /*is_managed=*/false);
     ExpectConfigurationSourceCounts(it.second, /*manual_count=*/1,
                                     /*policy_count=*/0);