Merge pull request #108 from flatcar-linux/invidian/dbus-improvements

D-Bus improvements

pkg/agent/agent.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/klog/v2"
 
 	"github.com/flatcar-linux/flatcar-linux-update-operator/pkg/constants"
+	"github.com/flatcar-linux/flatcar-linux-update-operator/pkg/dbus"
 	"github.com/flatcar-linux/flatcar-linux-update-operator/pkg/k8sutil"
 	"github.com/flatcar-linux/flatcar-linux-update-operator/pkg/updateengine"
 )
@@ -67,7 +68,7 @@ func New(node string, reapTimeout time.Duration) (*Klocksmith, error) {
 	nc := kc.CoreV1().Nodes()
 
 	// Set up update_engine client.
-	updateEngineClient, err := updateengine.New(updateengine.DBusSystemPrivateConnector)
+	updateEngineClient, err := updateengine.New(dbus.SystemPrivateConnector)
 	if err != nil {
 		return nil, fmt.Errorf("establishing connection to update_engine dbus: %w", err)
 	}

pkg/dbus/conn.go

@@ -0,0 +1,64 @@
+// Package dbus provides a helper function for creating new D-Bus client.
+package dbus
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+
+	godbus "github.com/godbus/dbus/v5"
+)
+
+// Client is an interface describing capabilities of internal D-Bus client.
+type Client interface {
+	AddMatchSignal(matchOptions ...godbus.MatchOption) error
+	Signal(ch chan<- *godbus.Signal)
+	Object(dest string, path godbus.ObjectPath) godbus.BusObject
+	Close() error
+}
+
+// Connection is an interface describing how much functionality we need from object providing D-Bus connection.
+type Connection interface {
+	Auth(authMethods []godbus.Auth) error
+	Hello() error
+
+	Client
+}
+
+// Connector is a constructor function providing D-Bus connection.
+type Connector func() (Connection, error)
+
+// SystemPrivateConnector is a standard connector using system bus.
+func SystemPrivateConnector() (Connection, error) {
+	return godbus.SystemBusPrivate()
+}
+
+// New creates new D-Bus client using given connector.
+func New(connector Connector) (Client, error) {
+	if connector == nil {
+		return nil, fmt.Errorf("no connection creator given")
+	}
+
+	conn, err := connector()
+	if err != nil {
+		return nil, fmt.Errorf("connecting to D-Bus: %w", err)
+	}
+
+	methods := []godbus.Auth{godbus.AuthExternal(strconv.Itoa(os.Getuid()))}
+
+	if err := conn.Auth(methods); err != nil {
+		// Best effort closing the connection.
+		_ = conn.Close()
+
+		return nil, fmt.Errorf("authenticating to D-Bus: %w", err)
+	}
+
+	if err := conn.Hello(); err != nil {
+		// Best effort closing the connection.
+		_ = conn.Close()
+
+		return nil, fmt.Errorf("sending hello to D-Bus: %w", err)
+	}
+
+	return conn, nil
+}

pkg/dbus/conn_integration_test.go

@@ -0,0 +1,30 @@
+//go:build integration
+// +build integration
+
+package dbus_test
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/flatcar-linux/flatcar-linux-update-operator/pkg/dbus"
+)
+
+const (
+	testDbusSocketEnv = "FLUO_TEST_DBUS_SOCKET"
+)
+
+//nolint:paralleltest // This test use environment variables.
+func Test_System_private_connector_successfully_connects_to_running_system_bus(t *testing.T) {
+	t.Setenv("DBUS_SYSTEM_BUS_ADDRESS", fmt.Sprintf("unix:path=%s", os.Getenv(testDbusSocketEnv)))
+
+	client, err := dbus.New(dbus.SystemPrivateConnector)
+	if err != nil {
+		t.Fatalf("Failed creating client: %v", err)
+	}
+
+	if client == nil {
+		t.Fatalf("Expected not nil client when new succeeds")
+	}
+}

pkg/dbus/conn_test.go

@@ -0,0 +1,189 @@
+package dbus_test
+
+import (
+	"encoding/hex"
+	"errors"
+	"fmt"
+	"os"
+	"strconv"
+	"testing"
+
+	godbus "github.com/godbus/dbus/v5"
+
+	"github.com/flatcar-linux/flatcar-linux-update-operator/pkg/dbus"
+)
+
+func Test_Creating_client_authenticates_using_user_id(t *testing.T) {
+	t.Parallel()
+
+	authCheckingConnection := &mockConnection{
+		authF: func(authMethods []godbus.Auth) error {
+			uidAuthFound := false
+
+			for i, method := range authMethods {
+				_, data, _ := method.FirstData()
+
+				decodedData, err := hex.DecodeString(string(data))
+				if err != nil {
+					t.Fatalf("Received auth method %d has bad hex data %v: %v", i, data, err)
+				}
+
+				potentialUID, err := strconv.Atoi(string(decodedData))
+				if err != nil {
+					t.Logf("Data %q couldn't be converted to UID: %v", string(decodedData), err)
+				}
+
+				if potentialUID == os.Getuid() {
+					uidAuthFound = true
+				}
+			}
+
+			if !uidAuthFound {
+				t.Fatalf("Expected auth method with user id")
+			}
+
+			return nil
+		},
+	}
+
+	client, err := dbus.New(func() (dbus.Connection, error) { return authCheckingConnection, nil })
+	if err != nil {
+		t.Fatalf("Unexpected error creating client: %v", err)
+	}
+
+	if client == nil {
+		t.Fatalf("When new succeeds, returned client should not be nil")
+	}
+}
+
+//nolint:funlen // Just many subtests.
+func Test_Creating_client_returns_error_when(t *testing.T) {
+	t.Parallel()
+
+	t.Run("no_connector_is_given", func(t *testing.T) {
+		t.Parallel()
+
+		testNewError(t, nil, nil)
+	})
+
+	t.Run("connecting_to_D-Bus_socket_fails", func(t *testing.T) {
+		t.Parallel()
+
+		expectedErr := fmt.Errorf("connection error")
+
+		failingConnectionConnector := func() (dbus.Connection, error) { return nil, expectedErr }
+
+		testNewError(t, failingConnectionConnector, expectedErr)
+	})
+
+	t.Run("authenticating_to_D-Bus_fails", func(t *testing.T) {
+		t.Parallel()
+
+		expectedErr := fmt.Errorf("auth error")
+
+		closeCalled := false
+
+		failingAuthConnection := &mockConnection{
+			authF: func([]godbus.Auth) error {
+				return expectedErr
+			},
+			closeF: func() error {
+				closeCalled = true
+
+				return fmt.Errorf("closing error")
+			},
+		}
+
+		testNewError(t, func() (dbus.Connection, error) { return failingAuthConnection, nil }, expectedErr)
+
+		t.Run("and_tries_to_close_the_client_while_ignoring_closing_error", func(t *testing.T) {
+			if !closeCalled {
+				t.Fatalf("Expected close function to be called")
+			}
+		})
+	})
+
+	t.Run("sending_hello_to_D-Bus_fails", func(t *testing.T) {
+		t.Parallel()
+
+		expectedErr := fmt.Errorf("hello error")
+
+		closeCalled := false
+
+		failingHelloConnection := &mockConnection{
+			helloF: func() error {
+				return expectedErr
+			},
+			closeF: func() error {
+				closeCalled = true
+
+				return fmt.Errorf("closing error")
+			},
+		}
+
+		testNewError(t, func() (dbus.Connection, error) { return failingHelloConnection, nil }, expectedErr)
+
+		t.Run("and_tries_to_close_the_client_while_ignoring_closing_error", func(t *testing.T) {
+			if !closeCalled {
+				t.Fatalf("Expected close function to be called")
+			}
+		})
+	})
+}
+
+func testNewError(t *testing.T, connector dbus.Connector, expectedErr error) {
+	t.Helper()
+
+	client, err := dbus.New(connector)
+	if err == nil {
+		t.Fatalf("Expected error creating client")
+	}
+
+	if client != nil {
+		t.Fatalf("Client should not be returned when creation error occurs")
+	}
+
+	if expectedErr != nil && !errors.Is(err, expectedErr) {
+		t.Fatalf("Unexpected error occurred, expected %q, got %q", expectedErr, err)
+	}
+}
+
+type mockConnection struct {
+	authF  func(authMethods []godbus.Auth) error
+	helloF func() error
+	closeF func() error
+}
+
+func (m *mockConnection) Auth(authMethods []godbus.Auth) error {
+	if m.authF == nil {
+		return nil
+	}
+
+	return m.authF(authMethods)
+}
+
+func (m *mockConnection) Hello() error {
+	if m.helloF == nil {
+		return nil
+	}
+
+	return m.helloF()
+}
+
+func (m *mockConnection) AddMatchSignal(matchOptions ...godbus.MatchOption) error {
+	return nil
+}
+
+func (m *mockConnection) Signal(ch chan<- *godbus.Signal) {}
+
+func (m *mockConnection) Object(dest string, path godbus.ObjectPath) godbus.BusObject {
+	return nil
+}
+
+func (m *mockConnection) Close() error {
+	if m.closeF == nil {
+		return nil
+	}
+
+	return m.closeF()
+}