Repository: code-ready/crc
Pull request #1819 (2021-01-06 11:34:15)

fedora33: Workaround NetworkManager bug with systemd-resolved
crc DNS configuration on systems using systemd-resolved is done through
a NetworkManager dispatcher file which gets run when the crc bridge is
created by libvirt. This file then uses resolvectl to configure the
bridge as the interface to use for resolving names in the .testing
domain.

However, NetworkManager will overwrite this resolvectl configuration
every time a network connection goes up/down. This is a NetworkManager
bug which is fixed in version 1.26.6 by this commit:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ee4e679bc7479de42780ebd8e3a4d74afa2b2ebe

This commit changes the dispatcher script to run every time any network
interface changes state so that we restore crc DNS configuration after
NM overwrites it. This workaround can be removed in a few weeks/months
once the NetworkManager package is upgraded in Fedora (I did not check
if ubuntu is impacted).

This fixes https://github.com/code-ready/crc/issues/1773

## Testing

Using a fedora 33 system with NetworkManager older than 1.26.6
1. after `crc setup`, `resolvectl domain` shows Link x (crc): ~testing
2. start/stop a NM connection with a search domain set (nmcli conn, nmcli conn show $conn |grep ipv4.dns-search, nmcli conn modify $conn ipv4.dns-search ~example.com, nmcli conn down $conn, nmcli conn up $conn) - just stopping/starting the VPN does the tric on my system
3. check `resolvectl domain` again - before this patch, `~testing` is gone, after this patch `~testing` is still present

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1819#" title="Author self-approved">cfergeau</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [cfergeau]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/cmd/crc/cmd/root.go b/cmd/crc/cmd/root.go
index 36e7a13b2..1c2b96adf 100644
--- a/cmd/crc/cmd/root.go
+++ b/cmd/crc/cmd/root.go
@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"strings"
+	"time"
 
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
@@ -175,13 +176,12 @@ func addForceFlag(cmd *cobra.Command) {
 func executeWithLogging(fullCmd string, input func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Command, args []string) error {
 	return func(cmd *cobra.Command, args []string) error {
 		logging.Debugf("Running '%s'", fullCmd)
-		if err := input(cmd, args); err != nil {
-			if serr := segmentClient.Upload(fullCmd, err); serr != nil {
-				logging.Debugf("Cannot send data to telemetry: %v", serr)
-			}
-			return err
+		startTime := time.Now()
+		err := input(cmd, args)
+		if serr := segmentClient.Upload(fullCmd, time.Since(startTime), err); serr != nil {
+			logging.Debugf("Cannot send data to telemetry: %v", serr)
 		}
-		return nil
+		return err
 	}
 }
 
diff --git a/pkg/crc/preflight/preflight_checks_network_linux.go b/pkg/crc/preflight/preflight_checks_network_linux.go
index bc78d6802..ca22b3ea9 100644
--- a/pkg/crc/preflight/preflight_checks_network_linux.go
+++ b/pkg/crc/preflight/preflight_checks_network_linux.go
@@ -72,22 +72,27 @@ server=/crc.testing/192.168.130.11
 dns=dnsmasq
 `
 
-	crcNetworkManagerDispatcherPath   = filepath.Join(crcNetworkManagerRootPath, "dispatcher.d", "pre-up.d", "99-crc.sh")
-	crcNetworkManagerDispatcherConfig = `#!/bin/sh
+	crcNetworkManagerOldDispatcherPath = filepath.Join(crcNetworkManagerRootPath, "dispatcher.d", "pre-up.d", "99-crc.sh")
+	crcNetworkManagerDispatcherPath    = filepath.Join(crcNetworkManagerRootPath, "dispatcher.d", "99-crc.sh")
+	crcNetworkManagerDispatcherConfig  = `#!/bin/sh
 # This is a NetworkManager dispatcher script to configure split DNS for
 # the 'crc' libvirt network.
-# The corresponding crc bridge is recreated each time the system reboots, so
-# it cannot be configured permanently through NetworkManager.
-# Changing DNS settings with nmcli requires the connection to go down/up,
-# so we directly make the change using resolvectl
+#
+# The corresponding crc bridge is not created through NetworkManager, so
+# it cannot be configured permanently through NetworkManager. We make the
+# change directly using resolvectl instead.
+#
+# NetworkManager will overwrite this resolvectl configuration every time a
+# network connection goes up/down, so we run this script on each of these events
+# to restore our settings. This is a NetworkManager bug which is fixed in
+# version 1.26.6 by this commit:
+# https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ee4e679bc7479de42780ebd8e3a4d74afa2b2ebe
 
 export LC_ALL=C
 
-if [ "$1" = crc ]; then
-        resolvectl domain "$1" ~testing
-        resolvectl dns "$1" 192.168.130.11
-        resolvectl default-route "$1" false
-fi
+resolvectl domain crc ~testing
+resolvectl dns crc 192.168.130.11
+resolvectl default-route crc false
 
 exit 0
 `
@@ -266,6 +271,10 @@ func checkCrcNetworkManagerDispatcherFile() error {
 
 func fixCrcNetworkManagerDispatcherFile() error {
 	logging.Debug("Fixing NetworkManager dispatcher configuration")
+
+	// Remove dispatcher script which was used in crc 1.20 - it's been moved to a new location
+	_ = removeNetworkManagerConfigFile(crcNetworkManagerOldDispatcherPath)
+
 	err := fixNetworkManagerConfigFile(crcNetworkManagerDispatcherPath, crcNetworkManagerDispatcherConfig, 0755)
 	if err != nil {
 		return err
@@ -276,6 +285,9 @@ func fixCrcNetworkManagerDispatcherFile() error {
 }
 
 func removeCrcNetworkManagerDispatcherFile() error {
+	// Remove dispatcher script which was used in crc 1.20 - it's been moved to a new location
+	_ = removeNetworkManagerConfigFile(crcNetworkManagerOldDispatcherPath)
+
 	return removeNetworkManagerConfigFile(crcNetworkManagerDispatcherPath)
 }
 
diff --git a/pkg/crc/segment/segment.go b/pkg/crc/segment/segment.go
index 254dfb386..e9ac423c4 100644
--- a/pkg/crc/segment/segment.go
+++ b/pkg/crc/segment/segment.go
@@ -4,7 +4,9 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
+	"time"
 
 	"github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
@@ -53,24 +55,35 @@ func (c *Client) Close() error {
 	return c.segmentClient.Close()
 }
 
-func (c *Client) Upload(action string, err error) error {
+func (c *Client) Upload(action string, duration time.Duration, err error) error {
 	if !c.config.Get(config.ConsentTelemetry).AsBool() {
 		return nil
 	}
-	logging.Debug("Uploading the error to segment")
 
 	anonymousID, uerr := getUserIdentity(c.telemetryFilePath)
 	if uerr != nil {
 		return uerr
 	}
 
-	t := analytics.NewTraits().
-		Set("action", action).
-		Set("error", err.Error())
+	if err := c.segmentClient.Enqueue(analytics.Identify{
+		AnonymousId: anonymousID,
+		Traits: analytics.NewTraits().
+			Set("os", runtime.GOOS),
+	}); err != nil {
+		return err
+	}
+
+	properties := analytics.NewProperties().
+		Set("success", err == nil).
+		Set("duration", duration.Milliseconds())
+	if err != nil {
+		properties = properties.Set("error", err.Error())
+	}
 
-	return c.segmentClient.Enqueue(analytics.Identify{
+	return c.segmentClient.Enqueue(analytics.Track{
 		AnonymousId: anonymousID,
-		Traits:      t,
+		Event:       action,
+		Properties:  properties,
 	})
 }
 


Test patch:
diff --git a/pkg/crc/segment/segment_test.go b/pkg/crc/segment/segment_test.go
index cdf4faa22..abc799f97 100644
--- a/pkg/crc/segment/segment_test.go
+++ b/pkg/crc/segment/segment_test.go
@@ -8,7 +8,9 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
+	"time"
 
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
@@ -22,8 +24,11 @@ type segmentResponse struct {
 		AnonymousID string `json:"anonymousId"`
 		MessageID   string `json:"messageId"`
 		Traits      struct {
-			Error string `json:"error"`
+			OS string `json:"os"`
 		} `json:"traits"`
+		Properties struct {
+			Error string `json:"error"`
+		} `json:"properties"`
 		Type string `json:"type"`
 	} `json:"batch"`
 	Context struct {
@@ -81,14 +86,17 @@ func TestClientUploadWithConsent(t *testing.T) {
 	c, err := newCustomClient(config, filepath.Join(dir, "telemetry"), server.URL)
 	require.NoError(t, err)
 
-	require.NoError(t, c.Upload("start", errors.New("an error occurred")))
+	require.NoError(t, c.Upload("start", time.Minute, errors.New("an error occurred")))
 	require.NoError(t, c.Close())
 
 	select {
 	case x := <-body:
 		s := segmentResponse{}
 		require.NoError(t, json.Unmarshal(x, &s))
-		require.Equal(t, s.Batch[0].Traits.Error, "an error occurred")
+		require.Equal(t, s.Batch[0].Type, "identify")
+		require.Equal(t, s.Batch[0].Traits.OS, runtime.GOOS)
+		require.Equal(t, s.Batch[1].Type, "track")
+		require.Equal(t, s.Batch[1].Properties.Error, "an error occurred")
 		require.Equal(t, s.Context.App.Name, "crc")
 		require.Equal(t, s.Context.App.Version, version.GetCRCVersion())
 	default:
@@ -111,7 +119,7 @@ func TestClientUploadWithOutConsent(t *testing.T) {
 	c, err := newCustomClient(config, filepath.Join(dir, "telemetry"), server.URL)
 	require.NoError(t, err)
 
-	require.NoError(t, c.Upload("start", errors.New("an error occurred")))
+	require.NoError(t, c.Upload("start", time.Second, errors.New("an error occurred")))
 	require.NoError(t, c.Close())
 
 	select {