From 92d9c4d62dbfe804d4e4f6800e5e8a2062a347f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 14 Dec 2023 20:30:02 +0100 Subject: [PATCH 1/3] testutils: Move broker into brokertestutils package to avoid importing cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When using testutils inside internal/brokers we were getting this: ❯ go build -C ~/Dev/authd/cmd/authd -tags withexamplebroker package github.com/ubuntu/authd/cmd/authd imports github.com/ubuntu/authd/cmd/authd/daemon imports github.com/ubuntu/authd/internal/services imports github.com/ubuntu/authd/internal/brokers imports github.com/ubuntu/authd/internal/testutils imports github.com/ubuntu/authd/internal/brokers: import cycle not allowed --- internal/brokers/broker_test.go | 5 +++-- internal/brokers/manager_test.go | 13 +++++++++---- internal/services/pam/pam_test.go | 7 ++++--- internal/testutils/{ => broker}/broker.go | 3 ++- 4 files changed, 18 insertions(+), 10 deletions(-) rename internal/testutils/{ => broker}/broker.go (99%) diff --git a/internal/brokers/broker_test.go b/internal/brokers/broker_test.go index a4276399e..60bbf2fb9 100644 --- a/internal/brokers/broker_test.go +++ b/internal/brokers/broker_test.go @@ -13,6 +13,7 @@ import ( "github.com/ubuntu/authd/internal/brokers" "github.com/ubuntu/authd/internal/brokers/responses" "github.com/ubuntu/authd/internal/testutils" + brokertestutils "github.com/ubuntu/authd/internal/testutils/broker" ) var supportedLayouts = map[string]map[string]string{ @@ -325,7 +326,7 @@ func newBrokerForTests(t *testing.T, cfgDir, brokerName string) (b brokers.Broke brokerName = strings.ReplaceAll(t.Name(), "/", "_") } - cfgPath, cleanup, err := testutils.StartBusBrokerMock(cfgDir, brokerName) + cfgPath, cleanup, err := brokertestutils.StartBusBrokerMock(cfgDir, brokerName) require.NoError(t, err, "Setup: could not start bus broker mock") t.Cleanup(cleanup) @@ -342,5 +343,5 @@ func newBrokerForTests(t *testing.T, cfgDir, brokerName string) (b brokers.Broke // prefixID is a helper function that prefixes the given ID with the test name to avoid conflicts. func prefixID(t *testing.T, id string) string { t.Helper() - return t.Name() + testutils.IDSeparator + id + return t.Name() + brokertestutils.IDSeparator + id } diff --git a/internal/brokers/manager_test.go b/internal/brokers/manager_test.go index d1b890648..f93f4cd75 100644 --- a/internal/brokers/manager_test.go +++ b/internal/brokers/manager_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ubuntu/authd/internal/brokers" "github.com/ubuntu/authd/internal/testutils" + brokertestutils "github.com/ubuntu/authd/internal/testutils/broker" ) var ( @@ -328,10 +329,14 @@ func TestStartAndEndSession(t *testing.T) { require.NoError(t, *firstErr, "First NewSession should not return an error, but did") require.NoError(t, *secondErr, "Second NewSession should not return an error, but did") - require.Equal(t, b1.ID+"-"+testutils.GenerateSessionID("user1"), *firstID, "First NewSession should return the expected session ID, but did not") - require.Equal(t, testutils.GenerateEncryptionKey(b1.Name), *firstKey, "First NewSession should return the expected encryption key, but did not") - require.Equal(t, b2.ID+"-"+testutils.GenerateSessionID("user2"), *secondID, "Second NewSession should return the expected session ID, but did not") - require.Equal(t, testutils.GenerateEncryptionKey(b2.Name), *secondKey, "Second NewSession should return the expected encryption key, but did not") + require.Equal(t, b1.ID+"-"+brokertestutils.GenerateSessionID("user1"), + *firstID, "First NewSession should return the expected session ID, but did not") + require.Equal(t, brokertestutils.GenerateEncryptionKey(b1.Name), + *firstKey, "First NewSession should return the expected encryption key, but did not") + require.Equal(t, b2.ID+"-"+brokertestutils.GenerateSessionID("user2"), + *secondID, "Second NewSession should return the expected session ID, but did not") + require.Equal(t, brokertestutils.GenerateEncryptionKey(b2.Name), + *secondKey, "Second NewSession should return the expected encryption key, but did not") assignedBroker, err := m.BrokerFromSessionID(*firstID) require.NoError(t, err, "First NewSession should have assigned a broker for the session, but did not") diff --git a/internal/services/pam/pam_test.go b/internal/services/pam/pam_test.go index 73a0427bd..71a9423ac 100644 --- a/internal/services/pam/pam_test.go +++ b/internal/services/pam/pam_test.go @@ -19,6 +19,7 @@ import ( cachetests "github.com/ubuntu/authd/internal/cache/tests" "github.com/ubuntu/authd/internal/services/pam" "github.com/ubuntu/authd/internal/testutils" + brokertestutils "github.com/ubuntu/authd/internal/testutils/broker" usertests "github.com/ubuntu/authd/internal/users/tests" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" @@ -154,7 +155,7 @@ func TestSelectBroker(t *testing.T) { } if tc.username != "" { - tc.username = t.Name() + testutils.IDSeparator + tc.username + tc.username = t.Name() + brokertestutils.IDSeparator + tc.username } sbRequest := &authd.SBRequest{ @@ -613,7 +614,7 @@ func initBrokers() (brokerConfigPath string, cleanup func(), err error) { _ = os.RemoveAll(tmpDir) return "", nil, err } - _, brokerCleanup, err := testutils.StartBusBrokerMock(brokersConfPath, "BrokerMock") + _, brokerCleanup, err := brokertestutils.StartBusBrokerMock(brokersConfPath, "BrokerMock") if err != nil { _ = os.RemoveAll(tmpDir) return "", nil, err @@ -682,7 +683,7 @@ func startSession(t *testing.T, client authd.PAMClient, username string) string t.Helper() // Prefixes the username to avoid concurrency issues. - username = t.Name() + testutils.IDSeparator + username + username = t.Name() + brokertestutils.IDSeparator + username sbResp, err := client.SelectBroker(context.Background(), &authd.SBRequest{ BrokerId: mockBrokerGeneratedID, diff --git a/internal/testutils/broker.go b/internal/testutils/broker/broker.go similarity index 99% rename from internal/testutils/broker.go rename to internal/testutils/broker/broker.go index 46087689d..024d2c3ac 100644 --- a/internal/testutils/broker.go +++ b/internal/testutils/broker/broker.go @@ -1,4 +1,5 @@ -package testutils +// Package brokertestutils provides utility functions and behaviors for testing brokers. +package brokertestutils import ( "bytes" From 01fc400ba3641314f02aa214e4d552635b87edb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 14 Dec 2023 20:41:20 +0100 Subject: [PATCH 2/3] brokers/withexamples: Reuse testutils function to start a mock system bus --- internal/brokers/withexamples.go | 65 +------------------------------- 1 file changed, 2 insertions(+), 63 deletions(-) diff --git a/internal/brokers/withexamples.go b/internal/brokers/withexamples.go index 2c1dc4c00..c11698de4 100644 --- a/internal/brokers/withexamples.go +++ b/internal/brokers/withexamples.go @@ -4,20 +4,17 @@ package brokers import ( "context" - "errors" - "fmt" "os" - "os/exec" - "path/filepath" "time" "github.com/ubuntu/authd/internal/brokers/examplebroker" "github.com/ubuntu/authd/internal/log" + "github.com/ubuntu/authd/internal/testutils" ) // useExampleBrokers starts a mock system bus and exports the examplebroker in it. func useExampleBrokers() (string, func(), error) { - busCleanup, err := startSystemBusMock() + busCleanup, err := testutils.StartSystemBusMock() if err != nil { return "", nil, err } @@ -50,65 +47,7 @@ func useExampleBrokers() (string, func(), error) { }, nil } -// startSystemBusMock starts a mock dbus daemon and returns a cancel function to stop it. -func startSystemBusMock() (func(), error) { - busDir := filepath.Join(os.TempDir(), "authd-system-bus-mock") - if err := os.MkdirAll(busDir, 0750); err != nil { - return nil, err - } - - cfgPath := filepath.Join(busDir, "bus.conf") - listenPath := filepath.Join(busDir, "bus.sock") - - err := os.WriteFile(cfgPath, []byte(fmt.Sprintf(localSystemBusCfg, listenPath)), 0600) - if err != nil { - err = errors.Join(err, os.RemoveAll(busDir)) - return nil, err - } - - busCtx, busCancel := context.WithCancel(context.Background()) - //#nosec:G204 // This is only for manual testing purposes and won't be in production code. - cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath) - if err := cmd.Start(); err != nil { - busCancel() - err = errors.Join(err, os.RemoveAll(busDir)) - return nil, err - } - // Give some time for the daemon to start. - time.Sleep(500 * time.Millisecond) - - prev, set := os.LookupEnv("DBUS_SYSTEM_BUS_ADDRESS") - os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path="+listenPath) - - return func() { - busCancel() - _ = cmd.Wait() - _ = os.RemoveAll(busDir) - - if !set { - os.Unsetenv("DBUS_SYSTEM_BUS_ADDRESS") - } else { - os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", prev) - } - }, nil -} - // Stop calls the function responsible for cleaning up the examplebrokers. func (m *Manager) Stop() { m.cleanup() } - -var localSystemBusCfg = ` - - system - - unix:path=%s - - - - - - - -` From fafc8bd0cf000ed76bbc1f852c50e680f1398ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 13 Dec 2023 21:39:34 +0100 Subject: [PATCH 3/3] dbus: Wait for the dbus-daemon to be ready using its output dbus-daemon can print out its listen address when it's ready, by waiting so instead of just guessing how much to wait. Doing this through some goroutines although being this test code we could very likely just hang. Co-authored-by: Gabriel Nagy --- internal/testutils/dbus.go | 40 ++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/internal/testutils/dbus.go b/internal/testutils/dbus.go index 3843ad837..fde1df7bd 100644 --- a/internal/testutils/dbus.go +++ b/internal/testutils/dbus.go @@ -2,12 +2,14 @@ package testutils import ( + "bufio" "context" "errors" "fmt" "os" "os/exec" "path/filepath" + "strings" "testing" "time" @@ -56,17 +58,47 @@ func StartSystemBusMock() (func(), error) { busCtx, busCancel := context.WithCancel(context.Background()) //#nosec:G204 // This is a test helper and we are in control of the arguments. - cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath) + cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath, "--print-address=1") + dbusStdout, err := cmd.StdoutPipe() + if err != nil { + busCancel() + return nil, errors.Join(err, os.RemoveAll(tmp)) + } if err := cmd.Start(); err != nil { busCancel() err = errors.Join(err, os.RemoveAll(tmp)) return nil, err } - // Give some time for the daemon to start. - time.Sleep(500 * time.Millisecond) + waitDone := make(chan struct{}) + var busAddress string + + go func() { + scanner := bufio.NewScanner(dbusStdout) + for scanner.Scan() { + busAddress = scanner.Text() + close(waitDone) + break + } + }() + + select { + case <-time.After(10 * time.Second): + busCancel() + err = errors.New("dbus-daemon failed to start in 10 seconds") + return nil, errors.Join(err, os.RemoveAll(tmp)) + case <-waitDone: + } + + if !strings.HasPrefix(busAddress, "unix:path=") { + busCancel() + err = fmt.Errorf("invalid bus path: %s", busAddress) + return nil, errors.Join(err, os.RemoveAll(tmp)) + } + + busAddress, _, _ = strings.Cut(busAddress, ",") prev, set := os.LookupEnv("DBUS_SYSTEM_BUS_ADDRESS") - os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path="+listenPath) + os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", busAddress) return func() { busCancel()