From 823cc7cb939c76a7d1a1bbfb4ecc0f23746f3dbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 23 Jan 2024 14:09:33 +0100 Subject: [PATCH 01/31] Implement CloudInit object to write cloud-init files It also keeps its own config so we don't have to pass it around --- windows-agent/internal/cloudinit/cloudinit.go | 202 +++++++++ .../internal/cloudinit/cloudinit_test.go | 400 ++++++++++++++++++ 2 files changed, 602 insertions(+) create mode 100644 windows-agent/internal/cloudinit/cloudinit.go create mode 100644 windows-agent/internal/cloudinit/cloudinit_test.go diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go new file mode 100644 index 000000000..4f996972a --- /dev/null +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -0,0 +1,202 @@ +// Package cloudinit has some helpers to set up cloud-init configuration. +package cloudinit + +import ( + "bytes" + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + + log "github.com/canonical/ubuntu-pro-for-wsl/common/grpc/logstreamer" + "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/config" + "github.com/ubuntu/decorate" + "gopkg.in/ini.v1" + "gopkg.in/yaml.v3" +) + +// Config is a configuration provider for ProToken and the Landscape config. +type Config interface { + Subscription() (string, config.Source, error) + LandscapeClientConfig() (string, config.Source, error) +} + +// CloudInit contains necessary data to drop cloud-init user data files for WSL's data source to pick them up. +type CloudInit struct { + dataDir string + conf Config +} + +// New creates a CloudInit object and attaches it to the configuration notifier. +func New(ctx context.Context, conf Config, publicDir string) (CloudInit, error) { + c := CloudInit{ + dataDir: filepath.Join(publicDir, ".cloud-init"), + conf: conf, + } + + if err := c.WriteAgentData(); err != nil { + return c, err + } + + return c, nil +} + +// Notify is syntax sugar to call WriteAgentData and log any error. +func (c CloudInit) Notify(ctx context.Context) { + if err := c.WriteAgentData(); err != nil { + log.Warningf(ctx, "Cloud-init: %v", err) + } +} + +// WriteAgentData writes the agent's cloud-init data file. +func (c CloudInit) WriteAgentData() (err error) { + defer decorate.OnError(&err, "could not create distro-specific cloud-init file") + + cloudInit, err := marshalConfig(c.conf) + if err != nil { + return err + } + + err = writeFileInDir(c.dataDir, "agent.yaml", cloudInit) + if err != nil { + return err + } + + return nil +} + +// WriteDistroData writes cloud-init user data to be used for a distro in particular. +func (c CloudInit) WriteDistroData(distroName string, cloudInit string) error { + err := writeFileInDir(c.dataDir, distroName+".user-data", []byte(cloudInit)) + if err != nil { + return fmt.Errorf("could not create distro-specific cloud-init file: %v", err) + } + + return nil +} + +// writeFileInDir: +// 1. Creates the directory if it did not exist. +// 2. Creates the file using the temp-then-move pattern. This avoids read/write races. +func writeFileInDir(dir string, file string, contents []byte) error { + if err := os.MkdirAll(dir, 0700); err != nil { + return fmt.Errorf("could not create directory: %v", err) + } + + path := filepath.Join(dir, file) + tmp := path + ".tmp" + + if err := os.WriteFile(tmp, contents, 0600); err != nil { + return fmt.Errorf("could not write: %v", err) + } + + if err := os.Rename(tmp, path); err != nil { + _ = os.Remove(tmp) + return err // Error message already says 'cannot rename' + } + + return nil +} + +// RemoveDistroData removes cloud-init user data to be used for a distro in particular. +// +// No error is returned if the data did not exist. +func (c CloudInit) RemoveDistroData(distroName string) (err error) { + defer decorate.OnError(&err, "could not remove distro-specific cloud-init file") + + path := filepath.Join(c.dataDir, distroName+".user-data") + + err = os.Remove(path) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return err + } + return nil +} + +func marshalConfig(conf Config) ([]byte, error) { + w := &bytes.Buffer{} + + if _, err := fmt.Fprintln(w, "# cloud-init"); err != nil { + return nil, fmt.Errorf("could not write # cloud-init stenza: %v", err) + } + + if _, err := fmt.Fprintln(w, "# This file was generated automatically and must not be edited"); err != nil { + return nil, fmt.Errorf("could not write warning message: %v", err) + } + + contents := make(map[string]interface{}) + + if err := ubuntuAdvantageModule(conf, contents); err != nil { + return nil, err + } + + if err := landscapeModule(conf, contents); err != nil { + return nil, err + } + + out, err := yaml.Marshal(contents) + if err != nil { + return nil, fmt.Errorf("could not Marshal user data as a YAML: %v", err) + } + + if _, err := w.Write(out); err != nil { + return nil, fmt.Errorf("could not write config body: %v", err) + } + + return w.Bytes(), nil +} + +func ubuntuAdvantageModule(c Config, out map[string]interface{}) error { + token, src, err := c.Subscription() + if err != nil { + return err + } + if src == config.SourceNone { + return nil + } + + type uaModule struct { + Token string `yaml:"token"` + } + + out["ubuntu_advantage"] = uaModule{Token: token} + return nil +} + +func landscapeModule(c Config, out map[string]interface{}) error { + conf, src, err := c.LandscapeClientConfig() + if err != nil { + return err + } + if src == config.SourceNone { + return nil + } + + var landcapeModule struct { + Client map[string]string `yaml:"client"` + } + + f, err := ini.Load(strings.NewReader(conf)) + if err != nil { + return fmt.Errorf("could not load Landscape configuration file") + } + + section, err := f.GetSection("client") + if err != nil { + return nil // Empty section + } + + landcapeModule.Client = make(map[string]string) + for _, keyName := range section.KeyStrings() { + landcapeModule.Client[keyName] = section.Key(keyName).String() + } + + out["landscape"] = landcapeModule + return nil +} diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go new file mode 100644 index 000000000..9d19b5048 --- /dev/null +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -0,0 +1,400 @@ +package cloudinit_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/canonical/ubuntu-pro-for-wsl/common/golden" + "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/cloudinit" + "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/config" + "github.com/stretchr/testify/require" +) + +func TestNew(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + breakWriteAgentData bool + wantErr bool + }{ + "Success": {}, + "Error when cloud-init agent file cannot be written": {breakWriteAgentData: true, wantErr: true}, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + publicDir := t.TempDir() + + conf := &mockConfig{ + subcriptionErr: tc.breakWriteAgentData, + } + + _, err := cloudinit.New(ctx, conf, publicDir) + if tc.wantErr { + require.Error(t, err, "Cloud-init creation should have returned an error") + return + } + require.NoError(t, err, "Cloud-init creation should have returned no error") + + // We don't assert on specifics, as they are tested in WriteAgentData tests. + path := filepath.Join(publicDir, ".cloud-init", "agent.yaml") + require.FileExists(t, path, "agent data file was not created when updating the config") + }) + } +} + +func TestWriteAgentData(t *testing.T) { + t.Parallel() + + // All error cases share a golden file so we need to protect it during updates + var sharedGolden goldenMutex + + const landscapeConfigOld string = `[irrelevant] +info=this section should have been omitted + +[client] +data=This is an old data field +info=This is the old configuration +` + + const landscapeConfigNew string = `[irrelevant] +info=this section should have been omitted + +[client] +info = This is the new configuration +url = www.example.com/new/rickroll +` + + testCases := map[string]struct { + // Contents + skipProToken bool + skipLandscapeConf bool + + // Break marshalling + breakSubscription bool + breakLandscape bool + + // Landcape parsing + landscapeNoClientSection bool + badLandscape bool + + // Break writing to file + breakDir bool + breakTempFile bool + breakFile bool + + wantErr bool + }{ + "Success": {}, + "Success without pro token": {skipProToken: true}, + "Success without Landscape": {skipLandscapeConf: true}, + "Success without Landscape [client] section": {landscapeNoClientSection: true}, + "Success with empty contents": {skipProToken: true, skipLandscapeConf: true}, + + "Error obtaining pro token": {breakSubscription: true, wantErr: true}, + "Error obtaining Landscape config": {breakLandscape: true, wantErr: true}, + "Error with erroneous Landscape config": {badLandscape: true, wantErr: true}, + + "Error when the datadir cannot be created": {breakDir: true, wantErr: true}, + "Error when the temp file cannot be written": {breakTempFile: true, wantErr: true}, + "Error when the temp file cannot be renamed": {breakFile: true, wantErr: true}, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + publicDir := t.TempDir() + dir := filepath.Join(publicDir, ".cloud-init") + path := filepath.Join(dir, "agent.yaml") + + conf := &mockConfig{ + proToken: "OLD_PRO_TOKEN", + landscapeConf: landscapeConfigOld, + } + + // Test a clean filesystem (New calls WriteAgentData internally) + ci, err := cloudinit.New(ctx, conf, publicDir) + require.NoError(t, err, "cloudinit.New should return no error") + require.FileExists(t, path, "New() should have created an agent cloud-init file") + + // Test overriding the file: New() created the agent.yaml file + conf.subcriptionErr = tc.breakSubscription + conf.landscapeErr = tc.breakLandscape + + conf.proToken = "NEW_PRO_TOKEN" + if tc.skipProToken { + conf.proToken = "" + } + + conf.landscapeConf = landscapeConfigNew + if tc.badLandscape { + conf.landscapeConf = "This is not valid ini" + } + if tc.landscapeNoClientSection { + conf.landscapeConf = "[irrelevant]\ninfo=This section should be ignored" + } + if tc.skipLandscapeConf { + conf.landscapeConf = "" + } + + if tc.breakTempFile { + require.NoError(t, os.RemoveAll(path+".tmp"), "Setup: Agent cloud-init file should not fail to delete") + require.NoError(t, os.MkdirAll(path+".tmp", 0600), "Setup: could not create directory to mess with cloud-init") + } + + if tc.breakFile { + require.NoError(t, os.RemoveAll(path), "Setup: Agent cloud-init file should not fail to delete") + require.NoError(t, os.MkdirAll(path, 0600), "Setup: could not create directory to mess with cloud-init") + } + + if tc.breakDir { + require.NoError(t, os.RemoveAll(dir), "Setup: Agent cloud-init file should not fail to delete") + require.NoError(t, os.WriteFile(dir, nil, 0600), "Setup: could not create file to mess with cloud-init directory") + } + + err = ci.WriteAgentData() + var opts []golden.Option + if tc.wantErr { + require.Error(t, err, "WriteAgentData should have returned an error") + errorGolden := filepath.Join(golden.TestFamilyPath(t), "golden", "error-cases") + opts = append(opts, golden.WithGoldenPath(errorGolden)) + } else { + require.NoError(t, err, "WriteAgentData should return no errors") + } + + // Assert that the file was updated (success case) or that the old one remains (error case) + if tc.breakFile || tc.breakDir { + // Cannot really assert on anything: we removed the old file + return + } + + got, err := os.ReadFile(path) + require.NoError(t, err, "There should be no error reading the cloud-init agent file") + + sharedGolden.Lock() + defer sharedGolden.Unlock() + + want := golden.LoadWithUpdateFromGolden(t, string(got), opts...) + require.Equal(t, want, string(got), "Agent cloud-init file does not match the golden file") + }) + } +} + +// goldenMutex is a mutex that only works when golden update is enabled. +type goldenMutex struct { + sync.Mutex +} + +func (mu *goldenMutex) Lock() { + if !golden.UpdateEnabled() { + return + } + mu.Mutex.Lock() +} + +func (mu *goldenMutex) Unlock() { + if !golden.UpdateEnabled() { + return + } + mu.Mutex.Unlock() +} + +func TestWriteDistroData(t *testing.T) { + t.Parallel() + + const oldCloudInit = `# cloud-init +# I'm an old piece of user data +data: + is_this_data: Yes, it is + new: false +` + + const newCloudInit = `# cloud-init +# I'm a shiny new piece of user data +data: + new: true +` + + testCases := map[string]struct { + // Break marshalling + emptyData bool + noOldData bool + + // Break writing to file + breakDir bool + breakTempFile bool + breakFile bool + + wantErr bool + }{ + "Success": {}, + "Success with no old data": {noOldData: true}, + "Success with empty data": {emptyData: true}, + + "Error when the datadir cannot be created": {breakDir: true, wantErr: true}, + "Error when the temp file cannot be written": {breakTempFile: true, wantErr: true}, + "Error when the temp file cannot be renamed": {breakFile: true, wantErr: true}, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + distroName := "CoolDistro" + + publicDir := t.TempDir() + dir := filepath.Join(publicDir, ".cloud-init") + path := filepath.Join(dir, distroName+".user-data") + + conf := &mockConfig{} + + // Test a clean filesystem (New calls WriteAgentData internally) + ci, err := cloudinit.New(ctx, conf, publicDir) + require.NoError(t, err, "Setup: cloud-init New should return no errors") + + if !tc.noOldData { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0600), "Setup: could not write old distro data directory") + require.NoError(t, os.WriteFile(path, []byte(oldCloudInit), 0600), "Setup: could not write old distro data") + } + + if tc.breakTempFile { + require.NoError(t, os.RemoveAll(path+".tmp"), "Setup: Distro cloud-init file should not fail to delete") + require.NoError(t, os.MkdirAll(path+".tmp", 0600), "Setup: could not create directory to mess with cloud-init") + } + + if tc.breakFile { + require.NoError(t, os.RemoveAll(path), "Setup: Distro cloud-init file should not fail to delete") + require.NoError(t, os.MkdirAll(path, 0600), "Setup: could not create directory to mess with cloud-init") + } + + if tc.breakDir { + require.NoError(t, os.RemoveAll(dir), "Setup: Distro cloud-init file should not fail to delete") + require.NoError(t, os.WriteFile(dir, nil, 0600), "Setup: could not create file to mess with cloud-init directory") + } + + var input string + if !tc.emptyData { + input = newCloudInit + } + + err = ci.WriteDistroData(distroName, input) + var want string + if tc.wantErr { + require.Error(t, err, "WriteAgentData should have returned an error") + want = oldCloudInit + } else { + require.NoError(t, err, "WriteAgentData should return no errors") + want = input + } + + // Assert that the file was updated (success case) or that the old one remains (error case) + if tc.breakFile || tc.breakDir { + // Cannot really assert on anything: we removed the old file + return + } + + got, err := os.ReadFile(path) + require.NoError(t, err, "There should be no error reading the distro's cloud-init file") + require.Equal(t, want, string(got), "Agent cloud-init file does not match the golden file") + }) + } +} + +func TestRemoveDistroData(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + fileDoesNotExist bool + dirDoesNotExist bool + fileIsDir bool + + wantErr bool + }{ + "Success": {}, + "Success when the file did not exist": {fileDoesNotExist: true}, + "Success when the directory did not exist": {dirDoesNotExist: true}, + + "Error when file cannot be removed": {fileIsDir: true, wantErr: true}, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + distroName := "CoolDistro" + + publicDir := t.TempDir() + dir := filepath.Join(publicDir, ".cloud-init") + path := filepath.Join(dir, distroName+".user-data") + + ci, err := cloudinit.New(ctx, &mockConfig{}, publicDir) + require.NoError(t, err, "Setup: cloud-init New should return no errors") + + if !tc.dirDoesNotExist { + if tc.fileIsDir { + // cloud-init will try to remove the file, but it is a directory + dir = path + // and the directory is not empty, thus remove should fail. + path = filepath.Join(dir, distroName+".user-data") + } + require.NoError(t, os.MkdirAll(dir, 0700), "Setup: could not set up directory") + if !tc.fileDoesNotExist { + require.NoError(t, os.WriteFile(path, []byte("hello, world!"), 0600), "Setup: could not set up directory") + } + } + + err = ci.RemoveDistroData(distroName) + if tc.wantErr { + require.Error(t, err, "RemoveDistroData should return an error") + require.FileExists(t, path, "RemoveDistroData should not have removed the distro cloud-init data file") + return + } + require.NoError(t, err, "RemoveDistroData should return no errors") + require.NoFileExists(t, path, "RemoveDistroData should remove the distro cloud-init data file") + }) + } +} + +type mockConfig struct { + proToken string + subcriptionErr bool + + landscapeConf string + landscapeErr bool +} + +func (c mockConfig) Subscription() (string, config.Source, error) { + if c.subcriptionErr { + return "", config.SourceNone, errors.New("culd not get subscription: mock error") + } + + if c.proToken == "" { + return "", config.SourceNone, nil + } + + return c.proToken, config.SourceUser, nil +} + +func (c mockConfig) LandscapeClientConfig() (string, config.Source, error) { + if c.landscapeErr { + return "", config.SourceNone, errors.New("could not get landscape configuration: mock error") + } + + if c.landscapeConf == "" { + return "", config.SourceNone, nil + } + + return c.landscapeConf, config.SourceUser, nil +} From 52f946047eb96c1136842a60a26d473657f4311e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Mon, 5 Feb 2024 17:19:30 +0100 Subject: [PATCH 02/31] Cloud-init test golden files --- windows-agent/internal/cloudinit/cloudinit_test.go | 14 +++++++------- .../testdata/TestWriteAgentData/golden/error-cases | 8 ++++++++ .../testdata/TestWriteAgentData/golden/success | 8 ++++++++ .../golden/success_with_empty_contents | 3 +++ .../golden/success_without_landscape | 4 ++++ .../success_without_landscape_[client]_section | 4 ++++ .../golden/success_without_pro_token | 6 ++++++ 7 files changed, 40 insertions(+), 7 deletions(-) create mode 100644 windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/error-cases create mode 100644 windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success create mode 100644 windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_with_empty_contents create mode 100644 windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape create mode 100644 windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape_[client]_section create mode 100644 windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_pro_token diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index 9d19b5048..4a45b7a35 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -8,7 +8,7 @@ import ( "sync" "testing" - "github.com/canonical/ubuntu-pro-for-wsl/common/golden" + "github.com/canonical/ubuntu-pro-for-wsl/common/testutils" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/cloudinit" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/config" "github.com/stretchr/testify/require" @@ -163,11 +163,11 @@ url = www.example.com/new/rickroll } err = ci.WriteAgentData() - var opts []golden.Option + var opts []testutils.Option if tc.wantErr { require.Error(t, err, "WriteAgentData should have returned an error") - errorGolden := filepath.Join(golden.TestFamilyPath(t), "golden", "error-cases") - opts = append(opts, golden.WithGoldenPath(errorGolden)) + errorGolden := filepath.Join(testutils.TestFamilyPath(t), "golden", "error-cases") + opts = append(opts, testutils.WithGoldenPath(errorGolden)) } else { require.NoError(t, err, "WriteAgentData should return no errors") } @@ -184,7 +184,7 @@ url = www.example.com/new/rickroll sharedGolden.Lock() defer sharedGolden.Unlock() - want := golden.LoadWithUpdateFromGolden(t, string(got), opts...) + want := testutils.LoadWithUpdateFromGolden(t, string(got), opts...) require.Equal(t, want, string(got), "Agent cloud-init file does not match the golden file") }) } @@ -196,14 +196,14 @@ type goldenMutex struct { } func (mu *goldenMutex) Lock() { - if !golden.UpdateEnabled() { + if !testutils.UpdateEnabled() { return } mu.Mutex.Lock() } func (mu *goldenMutex) Unlock() { - if !golden.UpdateEnabled() { + if !testutils.UpdateEnabled() { return } mu.Mutex.Unlock() diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/error-cases b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/error-cases new file mode 100644 index 000000000..882b62c56 --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/error-cases @@ -0,0 +1,8 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + data: This is an old data field + info: This is the old configuration +ubuntu_advantage: + token: OLD_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success new file mode 100644 index 000000000..20381817e --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success @@ -0,0 +1,8 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + info: This is the new configuration + url: www.example.com/new/rickroll +ubuntu_advantage: + token: NEW_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_with_empty_contents b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_with_empty_contents new file mode 100644 index 000000000..ba33d201a --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_with_empty_contents @@ -0,0 +1,3 @@ +# cloud-init +# This file was generated automatically and must not be edited +{} diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape new file mode 100644 index 000000000..d3be18eea --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape @@ -0,0 +1,4 @@ +# cloud-init +# This file was generated automatically and must not be edited +ubuntu_advantage: + token: NEW_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape_[client]_section b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape_[client]_section new file mode 100644 index 000000000..d3be18eea --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape_[client]_section @@ -0,0 +1,4 @@ +# cloud-init +# This file was generated automatically and must not be edited +ubuntu_advantage: + token: NEW_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_pro_token b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_pro_token new file mode 100644 index 000000000..93af58d59 --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_pro_token @@ -0,0 +1,6 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + info: This is the new configuration + url: www.example.com/new/rickroll From 72fec67c1170d3eeafefb0a7280b82da011615a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Fri, 9 Feb 2024 09:32:40 +0100 Subject: [PATCH 03/31] Use cloudinit in proservices and Landscape --- .../proservices/landscape/executor.go | 26 ++++++++++++++++--- .../proservices/landscape/interfaces.go | 1 + .../internal/proservices/landscape/service.go | 15 ++++++++++- .../internal/proservices/proservices.go | 12 +++++++-- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/windows-agent/internal/proservices/landscape/executor.go b/windows-agent/internal/proservices/landscape/executor.go index 7c747026f..50127cd93 100644 --- a/windows-agent/internal/proservices/landscape/executor.go +++ b/windows-agent/internal/proservices/landscape/executor.go @@ -118,8 +118,10 @@ func (e executor) stop(ctx context.Context, cmd *landscapeapi.Command_Stop) (err } func (e executor) install(ctx context.Context, cmd *landscapeapi.Command_Install) (err error) { - if cmd.GetCloudinit() != "" { - return fmt.Errorf("Cloud Init support is not yet available") + log.Debugf(ctx, "Landscape: received command Install. Target: %s", cmd.GetId()) + + if cmd.GetId() == "" { + return errors.New("empty distro name") } distro := gowsl.NewDistro(ctx, cmd.GetId()) @@ -129,6 +131,10 @@ func (e executor) install(ctx context.Context, cmd *landscapeapi.Command_Install return errors.New("already installed") } + if err := e.cloudInit().WriteDistroData(cmd.GetId(), cmd.GetCloudinit()); err != nil { + return fmt.Errorf("skipped installation: %v", err) + } + if err := gowsl.Install(ctx, distro.Name()); err != nil { return err } @@ -148,6 +154,10 @@ func (e executor) install(ctx context.Context, cmd *landscapeapi.Command_Install return err } + if cmd.GetCloudinit() != "" { + return nil + } + // TODO: The rest of this function will need to be rethought once cloud-init support exists. windowsUser, err := user.Current() if err != nil { @@ -174,10 +184,18 @@ func (e executor) install(ctx context.Context, cmd *landscapeapi.Command_Install func (e executor) uninstall(ctx context.Context, cmd *landscapeapi.Command_Uninstall) (err error) { d, ok := e.database().Get(cmd.GetId()) if !ok { - return fmt.Errorf("distro %q not in database", cmd.GetId()) + return errors.New("distro not in database") + } + + if err := d.Uninstall(ctx); err != nil { + return err } - return d.Uninstall(ctx) + if err := e.cloudInit().RemoveDistroData(d.Name()); err != nil { + log.Warningf(ctx, "Landscape uninstall: distro %q: %v", d.Name(), err) + } + + return nil } func (e executor) setDefault(ctx context.Context, cmd *landscapeapi.Command_SetDefault) error { diff --git a/windows-agent/internal/proservices/landscape/interfaces.go b/windows-agent/internal/proservices/landscape/interfaces.go index 972dc8a3d..67f9c32eb 100644 --- a/windows-agent/internal/proservices/landscape/interfaces.go +++ b/windows-agent/internal/proservices/landscape/interfaces.go @@ -14,6 +14,7 @@ type serviceData interface { config() Config database() *database.DistroDB hostname() string + cloudInit() CloudInit } // serviceConn is an internal interface to manage the connection to the Landscape service. diff --git a/windows-agent/internal/proservices/landscape/service.go b/windows-agent/internal/proservices/landscape/service.go index 2f72fc04e..ca6309eb4 100644 --- a/windows-agent/internal/proservices/landscape/service.go +++ b/windows-agent/internal/proservices/landscape/service.go @@ -41,6 +41,8 @@ type Service struct { // function to try again now (instead of waiting for the retrial // time). Do not use directly. Instead use signalRetryConnection(). connRetrier *retryConnection + + cloudinit CloudInit } // Config is a configuration provider for ProToken and the Landscape URL. @@ -53,6 +55,12 @@ type Config interface { SetLandscapeAgentUID(string) error } +// CloudInit is a cloud-init user data writer. +type CloudInit interface { + WriteDistroData(distroName string, cloudInit string) error + RemoveDistroData(distroName string) error +} + type options struct { hostname string } @@ -61,7 +69,7 @@ type options struct { type Option = func(*options) // New creates a new Landscape service object. -func New(ctx context.Context, conf Config, db *database.DistroDB, args ...Option) (s *Service, err error) { +func New(ctx context.Context, conf Config, db *database.DistroDB, cloudInit CloudInit, args ...Option) (s *Service, err error) { defer decorate.OnError(&err, "could not initizalize Landscape service") var opts options @@ -86,6 +94,7 @@ func New(ctx context.Context, conf Config, db *database.DistroDB, args ...Option db: db, hostName: opts.hostname, connRetrier: newRetryConnection(), + cloudinit: cloudInit, } return s, nil @@ -381,3 +390,7 @@ func (s *Service) sendInfo(info *landscapeapi.HostAgentInfo) error { func (s *Service) isDisabled() bool { return s.disabled.Load() } + +func (s *Service) cloudInit() CloudInit { + return s.cloudinit +} diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index 05fbc4186..61ea16354 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -12,6 +12,7 @@ import ( "github.com/canonical/ubuntu-pro-for-wsl/common/grpc/interceptorschain" "github.com/canonical/ubuntu-pro-for-wsl/common/grpc/logconnections" log "github.com/canonical/ubuntu-pro-for-wsl/common/grpc/logstreamer" + "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/cloudinit" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/config" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/database" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/proservices/landscape" @@ -79,7 +80,12 @@ func New(ctx context.Context, publicDir, privateDir string, args ...Option) (s M conf := config.New(ctx, privateDir) - db, err := database.New(ctx, privateDir, conf) + cloudInit, err := cloudinit.New(ctx, conf, publicDir) + if err != nil { + return s, err + } + + db, err := database.New(ctx, privateDir, s.conf) if err != nil { return s, err } @@ -90,7 +96,7 @@ func New(ctx context.Context, publicDir, privateDir string, args ...Option) (s M s.uiService = ui.New(ctx, conf, s.db) - landscape, err := landscape.New(ctx, conf, s.db) + landscape, err := landscape.New(ctx, conf, s.db, cloudInit) if err != nil { return s, err } @@ -101,10 +107,12 @@ func New(ctx context.Context, publicDir, privateDir string, args ...Option) (s M conf.SetUbuntuProNotifier(func(ctx context.Context, token string) { ubuntupro.Distribute(ctx, s.db, token) landscape.NotifyUbuntuProUpdate(ctx, token) + cloudInit.Notify(ctx) }) conf.SetLandscapeNotifier(func(ctx context.Context, conf, uid string) { landscape.NotifyConfigUpdate(ctx, conf, uid) + cloudInit.Notify(ctx) }) // All notifications have been set up: starting the registry watcher before any services. From 9b7839ada5be2518446ac8d7dc7769da7a31ed3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Mon, 5 Feb 2024 17:20:29 +0100 Subject: [PATCH 04/31] Update Landscape tests --- .../proservices/landscape/executor_test.go | 77 +++++++++++++------ .../proservices/landscape/landscape_test.go | 47 +++++++++-- 2 files changed, 96 insertions(+), 28 deletions(-) diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 68ce88a7b..02a169d03 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -143,19 +143,24 @@ func TestInstall(t *testing.T) { t.Parallel() testCases := map[string]struct { + noCloudInit bool distroAlredyInstalled bool emptyDistroName bool + cloudInitWriteErr bool wslInstallErr bool appxDoesNotExist bool - wantInstalled bool - wantNonRootUser bool + wantCouldInitWriteCalled bool + wantInstalled bool + wantNonRootUser bool }{ - "Success": {wantInstalled: true, wantNonRootUser: true}, + "Success": {wantCouldInitWriteCalled: true, wantInstalled: true}, + "Success with no cloud-init": {noCloudInit: true, wantCouldInitWriteCalled: true, wantInstalled: true, wantNonRootUser: true}, "Error when the distroname is empty": {emptyDistroName: true}, "Error when the Appx does not exist": {appxDoesNotExist: true}, "Error when the distro is already installed": {distroAlredyInstalled: true, wantInstalled: true}, + "Error when cloud-init cannot write": {cloudInitWriteErr: true, wantCouldInitWriteCalled: true}, "Error when the distro fails to install": {wslInstallErr: true}, } @@ -184,12 +189,22 @@ func TestInstall(t *testing.T) { distroName = testBed.distro.Name() } + if tc.cloudInitWriteErr { + testBed.cloudInit.writeErr = true + } + if tc.wslInstallErr { testBed.wslMock.InstallError = true } + var cloudInit *string + if !tc.noCloudInit { + cloudInit = new(string) + *cloudInit = "Hello, this is a cloud-init file" + } + return &landscapeapi.Command{ - Cmd: &landscapeapi.Command_Install_{Install: &landscapeapi.Command_Install{Id: distroName}}, + Cmd: &landscapeapi.Command_Install_{Install: &landscapeapi.Command_Install{Id: distroName, Cloudinit: cloudInit}}, } }, // Test assertions @@ -210,14 +225,17 @@ func TestInstall(t *testing.T) { require.NoError(t, err, "GetConfiguration should return no error") require.NotEqual(t, uint32(0), conf.DefaultUID, "Default user should have been changed from root") } - return - } + } else { + time.Sleep(timeout) - time.Sleep(timeout) + distroExists, err := testBed.distro.IsRegistered() + require.NoError(t, err, "IsRegistered should return no error") + require.False(t, distroExists, "Distro should not have been registered") + } - distroExists, err := testBed.distro.IsRegistered() - require.NoError(t, err, "IsRegistered should return no error") - require.False(t, distroExists, "Distro should not have been registered") + if tc.wantCouldInitWriteCalled { + require.True(t, testBed.cloudInit.writeCalled.Load(), "Cloud-init should have been called to write the user data file") + } }) }) } @@ -229,13 +247,16 @@ func TestUninstall(t *testing.T) { testCases := map[string]struct { distroNotInstalled bool wslUninstallErr bool + cloudInitRemoveErr bool - wantNotRegistered bool + wantNotRegistered bool + wantCloudInitRemoveCalled bool }{ - "Success": {wantNotRegistered: true}, + "Success": {wantNotRegistered: true, wantCloudInitRemoveCalled: true}, - "Error when the distroname does not match any distro": {distroNotInstalled: true, wantNotRegistered: true}, - "Error when the distro fails to uninstall": {wslUninstallErr: true}, + "Error when the distroname does not match any distro": {distroNotInstalled: true, wantNotRegistered: true}, + "Error when the distro fails to uninstall": {wslUninstallErr: true}, + "Error when cloud-init cannot remove the cloud-init file": {cloudInitRemoveErr: true, wantNotRegistered: true, wantCloudInitRemoveCalled: true}, } for name, tc := range testCases { @@ -245,6 +266,10 @@ func TestUninstall(t *testing.T) { testReceiveCommand(t, distroSettings{install: !tc.distroNotInstalled}, // Test setup func(testBed *commandTestBed) *landscapeapi.Command { + if tc.cloudInitRemoveErr { + testBed.cloudInit.writeErr = true + } + if tc.wslUninstallErr { testBed.wslMock.WslUnregisterDistributionError = true } @@ -260,13 +285,17 @@ func TestUninstall(t *testing.T) { if tc.wantNotRegistered { ok, _ := checkEventuallyState(t, testBed.distro, wsl.NonRegistered, maxTimeout, time.Second) require.True(t, ok, "Distro should not be registered") - return + } else { + time.Sleep(maxTimeout) + distroExists, err := testBed.distro.IsRegistered() + require.NoError(t, err, "IsRegistered should return no error") + require.True(t, distroExists, "Existing distro should still have been unregistered") } - time.Sleep(maxTimeout) - distroExists, err := testBed.distro.IsRegistered() - require.NoError(t, err, "IsRegistered should return no error") - require.True(t, distroExists, "Existing distro should still have been unregistered") + if tc.wantCloudInitRemoveCalled { + require.Eventually(t, testBed.cloudInit.removeCalled.Load, time.Second, 100*time.Millisecond, + "Cloud-init should have been called to write the user data file") + } }) }) } @@ -393,9 +422,10 @@ func TestSetShutdownHost(t *testing.T) { type commandTestBed struct { ctx context.Context - conf *mockConfig - distro *wsl.Distro - db *database.DistroDB + conf *mockConfig + distro *wsl.Distro + db *database.DistroDB + cloudInit *mockCloudInit serverService *landscapemockservice.Service clientService *landscape.Service @@ -415,7 +445,7 @@ type distroSettings struct { // // Before testSetup: // - Set up the mock WSL -// - Set up the agent components (config, database...) +// - Set up the agent components (config, database, cloud-init...) // - Set up the mock Landscape server // - Set up the landscape client // - Register a distro to test @@ -463,6 +493,7 @@ func testReceiveCommand(t *testing.T, distrosettings distroSettings, testSetup f require.NoError(t, err, "Setup: database New should not return an error") tb.db = db + tb.cloudInit = &mockCloudInit{} // Set up Landscape client clientService, err := landscape.New(ctx, tb.conf, tb.db, landscape.WithHostname("HOSTNAME")) diff --git a/windows-agent/internal/proservices/landscape/landscape_test.go b/windows-agent/internal/proservices/landscape/landscape_test.go index 0545200a2..3b750b7d6 100644 --- a/windows-agent/internal/proservices/landscape/landscape_test.go +++ b/windows-agent/internal/proservices/landscape/landscape_test.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "text/template" "time" @@ -167,7 +168,9 @@ func TestConnect(t *testing.T) { _, err = db.GetDistroAndUpdateProperties(ctx, distroName, distro.Properties{}) require.NoError(t, err, "Setup: GetDistroAndUpdateProperties should return no errors") - service, err := landscape.New(ctx, conf, db) + var cloudInit mockCloudInit + + service, err := landscape.New(ctx, conf, db, &cloudInit) require.NoError(t, err, "Setup: NewClient should return no errrors") if tc.precancelContext { @@ -312,7 +315,9 @@ func TestSendUpdatedInfo(t *testing.T) { const hostname = "HOSTNAME" - service, err := landscape.New(ctx, conf, db, landscape.WithHostname(hostname)) + var cloudInit mockCloudInit + + service, err := landscape.New(ctx, conf, db, &cloudInit, landscape.WithHostname(hostname)) require.NoError(t, err, "Landscape NewClient should not return an error") ctl := service.Controller() @@ -503,7 +508,9 @@ func TestAutoReconnection(t *testing.T) { const hostname = "HOSTNAME" - service, err := landscape.New(ctx, conf, db, landscape.WithHostname(hostname)) + var cloudInit mockCloudInit + + service, err := landscape.New(ctx, conf, db, &cloudInit, landscape.WithHostname(hostname)) require.NoError(t, err, "Landscape NewClient should not return an error") defer service.Stop(ctx) @@ -733,10 +740,12 @@ func TestReconnect(t *testing.T) { go server.Serve(lis) defer server.Stop() - db, err := database.New(ctx, t.TempDir(), conf) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: database New should not return an error") - service, err := landscape.New(ctx, conf, db) + var cloudInit mockCloudInit + + service, err := landscape.New(ctx, conf, db, &cloudInit) require.NoError(t, err, "Setup: New should not return an error") err = service.Connect() @@ -842,6 +851,34 @@ func setUpLandscapeMock(t *testing.T, ctx context.Context, addr string, certPath return lis, server, service } +type mockCloudInit struct { + writeCalled atomic.Bool + removeCalled atomic.Bool + + writeErr bool + removeErr bool +} + +func (c *mockCloudInit) WriteDistroData(distroName string, cloudInit string) error { + c.writeCalled.Store(true) + + if c.writeErr { + return errors.New("could not write distro cloud-init data: mock error") + } + + return nil +} + +func (c *mockCloudInit) RemoveDistroData(distroName string) error { + c.removeCalled.Store(true) + + if c.removeErr { + return errors.New("could not remove distro cloud-init data: mock error") + } + + return nil +} + type mockConfig struct { proToken string landscapeClientConfig string From 823242fc1fdadecca928e636dbeae7a25a606da0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Wed, 27 Mar 2024 11:04:20 +0100 Subject: [PATCH 05/31] Remove task provisioning Cloud-init is in charge of this now --- windows-agent/internal/config/config.go | 23 ------ windows-agent/internal/config/config_test.go | 76 ++----------------- .../internal/distros/database/database.go | 11 +-- .../distros/database/database_test.go | 14 ++-- .../internal/distros/distro/distro.go | 17 +---- .../internal/distros/distro/distro_test.go | 46 +++++------ .../internal/distros/distro/export_test.go | 4 +- .../internal/distros/worker/worker.go | 42 +--------- .../internal/distros/worker/worker_test.go | 34 +-------- .../proservices/landscape/executor_test.go | 4 +- .../proservices/landscape/landscape_test.go | 11 +-- .../internal/proservices/proservices.go | 2 +- .../registrywatcher/watcher_test.go | 2 +- .../internal/proservices/ui/ui_test.go | 10 +-- .../wslinstance/wslinstance_test.go | 4 +- .../internal/ubuntupro/ubuntupro_test.go | 2 +- 16 files changed, 60 insertions(+), 242 deletions(-) diff --git a/windows-agent/internal/config/config.go b/windows-agent/internal/config/config.go index a38956f62..50361b355 100644 --- a/windows-agent/internal/config/config.go +++ b/windows-agent/internal/config/config.go @@ -13,8 +13,6 @@ import ( log "github.com/canonical/ubuntu-pro-for-wsl/common/grpc/logstreamer" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/database" - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/task" - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/tasks" "github.com/ubuntu/decorate" ) @@ -90,27 +88,6 @@ func (c *Config) Subscription() (token string, source Source, err error) { return token, source, nil } -// ProvisioningTasks returns a slice of all tasks to be submitted upon first contact with a distro. -func (c *Config) ProvisioningTasks(ctx context.Context, distroName string) ([]task.Task, error) { - var taskList []task.Task - - // Refresh data from registry - s, err := c.get() - if err != nil { - return nil, fmt.Errorf("config: could not get provisioning tasks: %v", err) - } - - // Ubuntu Pro attachment - proToken, _ := s.Subscription.resolve() - taskList = append(taskList, tasks.ProAttachment{Token: proToken}) - - // Landscape config - lconf, _ := s.Landscape.resolve() - taskList = append(taskList, tasks.LandscapeConfigure{Config: lconf, HostagentUID: s.Landscape.UID}) - - return taskList, nil -} - // LandscapeClientConfig returns the value of the landscape server URL and // the method it was acquired with (if any). func (c *Config) LandscapeClientConfig() (string, Source, error) { diff --git a/windows-agent/internal/config/config_test.go b/windows-agent/internal/config/config_test.go index ebfed5a22..65b33b22b 100644 --- a/windows-agent/internal/config/config_test.go +++ b/windows-agent/internal/config/config_test.go @@ -9,8 +9,6 @@ import ( config "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/config" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/database" - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/task" - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/tasks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" wsl "github.com/ubuntu/gowsl" @@ -69,7 +67,6 @@ func TestSubscription(t *testing.T) { "Error when the file cannot be read from": {settingsState: untouched, breakFile: true, wantError: true}, } - //nolint: dupl // This is mostly duplicate but de-duplicating with a meta-test worsens readability for name, tc := range testCases { t.Run(name, func(t *testing.T) { ctx := context.Background() @@ -78,7 +75,7 @@ func TestSubscription(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, false) @@ -124,7 +121,6 @@ func TestLandscapeConfig(t *testing.T) { "Error when the file cannot be read from": {settingsState: untouched, breakFile: true, wantError: true}, } - //nolint: dupl // This is mostly duplicate but de-duplicating with a meta-test worsens readability for name, tc := range testCases { t.Run(name, func(t *testing.T) { ctx := context.Background() @@ -133,7 +129,7 @@ func TestLandscapeConfig(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, false) @@ -182,7 +178,7 @@ func TestLandscapeAgentUID(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, false) @@ -213,62 +209,6 @@ func TestLandscapeAgentUID(t *testing.T) { } } -func TestProvisioningTasks(t *testing.T) { - if wsl.MockAvailable() { - t.Parallel() - } - - testCases := map[string]struct { - settingsState settingsState - - wantToken string - wantLandscapeConf string - wantLandscapeUID string - - wantError bool - }{ - "Success when there is no data": {settingsState: untouched}, - "Success when there is an empty config file": {settingsState: fileExists}, - "Success when the file's pro token field exists but is empty": {settingsState: userTokenExists}, - "Success with a user token": {settingsState: userTokenHasValue, wantToken: "user_token"}, - "Success when there is Landscape config": {settingsState: userLandscapeConfigHasValue | landscapeUIDHasValue, wantLandscapeConf: "[client]\nuser=JohnDoe", wantLandscapeUID: "landscapeUID1234"}, - } - - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - ctx := context.Background() - if wsl.MockAvailable() { - t.Parallel() - ctx = wsl.WithMock(ctx, wslmock.New()) - } - - db, err := database.New(ctx, t.TempDir(), nil) - require.NoError(t, err, "Setup: could not create empty database") - - setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, false, false) - conf := config.New(ctx, dir) - setup(t, conf) - - gotTasks, err := conf.ProvisioningTasks(ctx, "UBUNTU") - if tc.wantError { - require.Error(t, err, "ProvisioningTasks should return an error") - return - } - require.NoError(t, err, "ProvisioningTasks should return no error") - - wantTasks := []task.Task{ - tasks.ProAttachment{Token: tc.wantToken}, - tasks.LandscapeConfigure{ - Config: tc.wantLandscapeConf, - HostagentUID: tc.wantLandscapeUID, - }, - } - - require.ElementsMatch(t, wantTasks, gotTasks, "Unexpected contents returned by ProvisioningTasks") - }) - } -} - func TestSetUserSubscription(t *testing.T) { if wsl.MockAvailable() { t.Parallel() @@ -300,7 +240,7 @@ func TestSetUserSubscription(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, tc.cannotWriteFile) @@ -375,7 +315,7 @@ func TestSetStoreSubscription(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, tc.cannotWriteFile) @@ -444,7 +384,7 @@ func TestSetUserLandscapeConfig(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, false) @@ -509,7 +449,7 @@ func TestSetLandscapeAgentUID(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") setup, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakFile, tc.cannotWriteFile) @@ -578,7 +518,7 @@ func TestUpdateRegistryData(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") _, dir := setUpMockSettings(t, ctx, db, tc.settingsState, tc.breakConfigFile, false) diff --git a/windows-agent/internal/distros/database/database.go b/windows-agent/internal/distros/database/database.go index 2eb4f4389..89bf8b758 100644 --- a/windows-agent/internal/distros/database/database.go +++ b/windows-agent/internal/distros/database/database.go @@ -17,7 +17,6 @@ import ( log "github.com/canonical/ubuntu-pro-for-wsl/common/grpc/logstreamer" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/consts" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/distro" - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/worker" "github.com/ubuntu/decorate" "gopkg.in/yaml.v3" ) @@ -35,8 +34,7 @@ type DistroDB struct { scheduleTrigger chan struct{} - storageDir string - provisioning worker.Provisioning + storageDir string ctx context.Context cancelCtx func() @@ -58,7 +56,7 @@ type DistroDB struct { // Every certain amount of times, the database wil purge all distros that // are no longer registered or that have been marked as unreachable. This // cleanup can be triggered on demmand with TriggerCleanup. -func New(ctx context.Context, storageDir string, provisioning worker.Provisioning) (db *DistroDB, err error) { +func New(ctx context.Context, storageDir string) (db *DistroDB, err error) { defer decorate.OnError(&err, "could not initialize database") select { @@ -72,7 +70,6 @@ func New(ctx context.Context, storageDir string, provisioning worker.Provisionin db = &DistroDB{ storageDir: storageDir, scheduleTrigger: make(chan struct{}), - provisioning: provisioning, ctx: ctx, cancelCtx: cancel, } @@ -150,7 +147,7 @@ func (db *DistroDB) GetDistroAndUpdateProperties(ctx context.Context, name strin if !found { log.Debugf(ctx, "Database: cache miss, creating %q and adding it to the database", name) - d, err := distro.New(db.ctx, name, props, db.storageDir, &db.distroStartMu, distro.WithProvisioning(db.provisioning)) + d, err := distro.New(db.ctx, name, props, db.storageDir, &db.distroStartMu) if err != nil { return nil, err } @@ -168,7 +165,7 @@ func (db *DistroDB) GetDistroAndUpdateProperties(ctx context.Context, name strin go d.Cleanup(ctx) delete(db.distros, normalizedName) - d, err := distro.New(db.ctx, name, props, db.storageDir, &db.distroStartMu, distro.WithProvisioning(db.provisioning)) + d, err := distro.New(db.ctx, name, props, db.storageDir, &db.distroStartMu) if err != nil { return nil, err } diff --git a/windows-agent/internal/distros/database/database_test.go b/windows-agent/internal/distros/database/database_test.go index fe88ad66d..ea44e2441 100644 --- a/windows-agent/internal/distros/database/database_test.go +++ b/windows-agent/internal/distros/database/database_test.go @@ -72,7 +72,7 @@ func TestNew(t *testing.T) { databaseFromTemplate(t, dbDir, distroID{distro, guid}) } - db, err := database.New(ctx, dbDir, nil) + db, err := database.New(ctx, dbDir) if err == nil { defer db.Close(ctx) } @@ -114,7 +114,7 @@ func TestDatabaseGetAll(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: database creation should not fail") defer db.Close(ctx) @@ -157,7 +157,7 @@ func TestDatabaseGet(t *testing.T) { distroID{registeredDistroInDB, registeredGUID}, distroID{nonRegisteredDistroInDB, oldGUID}) - db, err := database.New(ctx, databaseDir, nil) + db, err := database.New(ctx, databaseDir) require.NoError(t, err, "Setup: New() should return no error") // Must use Cleanup. If we use defer, it'll run before the subtests are launched. @@ -202,7 +202,7 @@ func TestDatabaseGetAfterClose(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: New() should return no error") db.Close(ctx) @@ -249,7 +249,7 @@ func TestDatabaseDump(t *testing.T) { databaseFromTemplate(t, dbDir, distroID{distro1, guid1}, distroID{distro2, guid2}) } - db, err := database.New(ctx, dbDir, nil) + db, err := database.New(ctx, dbDir) require.NoError(t, err, "Setup: empty database should be created without issue") defer db.Close(ctx) @@ -396,7 +396,7 @@ func TestGetDistroAndUpdateProperties(t *testing.T) { distroID{distroInDB, guids[distroInDB]}, distroID{reRegisteredDistro, guids[reRegisteredDistro]}) - db, err := database.New(ctx, dbDir, nil) + db, err := database.New(ctx, dbDir) require.NoError(t, err, "Setup: New() should return no error") defer db.Close(ctx) @@ -497,7 +497,7 @@ func TestDatabaseCleanup(t *testing.T) { databaseFromTemplate(t, dbDir, distros...) - db, err := database.New(ctx, dbDir, nil) + db, err := database.New(ctx, dbDir) require.NoError(t, err, "Setup: New() should have returned no error") defer db.Close(ctx) diff --git a/windows-agent/internal/distros/distro/distro.go b/windows-agent/internal/distros/distro/distro.go index 22eefcc76..e0de9b498 100644 --- a/windows-agent/internal/distros/distro/distro.go +++ b/windows-agent/internal/distros/distro/distro.go @@ -56,9 +56,8 @@ func (*NotValidError) Error() string { type options struct { guid uuid.UUID - provisioning worker.Provisioning taskProcessingContext context.Context - newWorkerFunc func(context.Context, *Distro, string, worker.Provisioning) (workerInterface, error) + newWorkerFunc func(context.Context, *Distro, string) (workerInterface, error) } // Option is an optional argument for distro.New. @@ -72,14 +71,6 @@ func WithGUID(guid uuid.UUID) Option { } } -// WithProvisioning allows for providing a worker.Provisioning. If that is done, -// it'll be queried for the provisioning tasks and these will be submitted. -func WithProvisioning(c worker.Provisioning) Option { - return func(o *options) { - o.provisioning = c - } -} - // New creates a new Distro object after searching for a distro with the given name. // // - If identity.Name is not registered, a DistroDoesNotExist error is returned. @@ -96,8 +87,8 @@ func New(ctx context.Context, name string, props Properties, storageDir string, opts := options{ guid: nilGUID, taskProcessingContext: context.Background(), - newWorkerFunc: func(ctx context.Context, d *Distro, dir string, provisioning worker.Provisioning) (workerInterface, error) { - return worker.New(ctx, d, dir, worker.WithProvisioning(provisioning)) + newWorkerFunc: func(ctx context.Context, d *Distro, dir string) (workerInterface, error) { + return worker.New(ctx, d, dir) }, } @@ -140,7 +131,7 @@ func New(ctx context.Context, name string, props Properties, storageDir string, }, } - distro.worker, err = opts.newWorkerFunc(opts.taskProcessingContext, distro, storageDir, opts.provisioning) + distro.worker, err = opts.newWorkerFunc(opts.taskProcessingContext, distro, storageDir) if err != nil { return nil, err } diff --git a/windows-agent/internal/distros/distro/distro_test.go b/windows-agent/internal/distros/distro/distro_test.go index fbfb71a47..d62ad7909 100644 --- a/windows-agent/internal/distros/distro/distro_test.go +++ b/windows-agent/internal/distros/distro/distro_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "path/filepath" "sync" "testing" "time" @@ -65,17 +66,16 @@ func TestNew(t *testing.T) { } testCases := map[string]struct { - distro string - withGUID string - withProvisioning bool - nilMutex bool + distro string + withGUID string + preventWorkDirCreation bool + nilMutex bool wantErr bool wantErrType error }{ - "Success with a registered distro": {distro: registeredDistro}, - "Success with a registered distro and its GUID": {distro: registeredDistro, withGUID: registeredGUID}, - "Success with a registered distro with provisioning": {distro: registeredDistro, withProvisioning: true}, + "Success with a registered distro": {distro: registeredDistro}, + "Success with a registered distro and its GUID": {distro: registeredDistro, withGUID: registeredGUID}, // Error cases "Error when a constructing a distro with another distro's GUID": {distro: nonRegisteredDistro, withGUID: anotherRegisteredGUID, wantErr: true, wantErrType: &distro.NotValidError{}}, @@ -98,8 +98,11 @@ func TestNew(t *testing.T) { args = append(args, distro.WithGUID(GUID)) } - if tc.withProvisioning { - args = append(args, distro.WithProvisioning(&mockProvisioning{})) + workDir := t.TempDir() + if tc.preventWorkDirCreation { + workDir = filepath.Join(workDir, "workdir") + err := os.WriteFile(workDir, []byte("I'm here to interfere"), 0600) + require.NoError(t, err, "Setup: could not write file to interfere with distro's MkDir") } mu := startupMutex() @@ -557,7 +560,6 @@ func TestWorkerConstruction(t *testing.T) { withMockWorker, worker := mockWorkerInjector(tc.constructorReturnErr) workDir := t.TempDir() - provisioning := mockProvisioning{} d, err := distro.New(ctx, distroName, @@ -565,7 +567,6 @@ func TestWorkerConstruction(t *testing.T) { workDir, startupMutex(), distro.WithTaskProcessingContext(ctx), - distro.WithProvisioning(provisioning), withMockWorker) defer d.Cleanup(context.Background()) @@ -579,7 +580,6 @@ func TestWorkerConstruction(t *testing.T) { require.NotNil(t, (*worker).newCtx.Value(testContextMarker(42)), "Worker's constructor should be called with the distro's context or a child of it") require.Equal(t, d, (*worker).newDistro, "Worker's constructor should be called with the distro it is attached to") require.Equal(t, workDir, (*worker).newDir, "Worker's constructor should be called with the same workdir as the distro's") - require.Equal(t, provisioning, (*worker).newProvisioning, "Worker's constructor should be called with the config passed to the distro") }) } } @@ -766,10 +766,9 @@ func TestUninstall(t *testing.T) { } type mockWorker struct { - newCtx context.Context - newDistro *distro.Distro - newDir string - newProvisioning worker.Provisioning + newCtx context.Context + newDistro *distro.Distro + newDir string isActiveCalled bool connectionCalled bool @@ -780,12 +779,11 @@ type mockWorker struct { func mockWorkerInjector(constructorReturnsError bool) (distro.Option, **mockWorker) { mock := new(*mockWorker) - newMockWorker := func(ctx context.Context, d *distro.Distro, tmpDir string, conf worker.Provisioning) (distro.Worker, error) { + newMockWorker := func(ctx context.Context, d *distro.Distro, tmpDir string) (distro.Worker, error) { w := &mockWorker{ - newCtx: ctx, - newDistro: d, - newDir: tmpDir, - newProvisioning: conf, + newCtx: ctx, + newDistro: d, + newDir: tmpDir, } *mock = w if constructorReturnsError { @@ -828,12 +826,6 @@ func (w *mockWorker) Stop(context.Context) { w.stopCalled = true } -type mockProvisioning struct{} - -func (c mockProvisioning) ProvisioningTasks(ctx context.Context, distroName string) ([]task.Task, error) { - return nil, nil -} - type mockConnection struct{} func (c *mockConnection) SendProAttachment(proToken string) error { diff --git a/windows-agent/internal/distros/distro/export_test.go b/windows-agent/internal/distros/distro/export_test.go index 4baf1206f..958367235 100644 --- a/windows-agent/internal/distros/distro/export_test.go +++ b/windows-agent/internal/distros/distro/export_test.go @@ -2,8 +2,6 @@ package distro import ( "context" - - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/worker" ) func WithTaskProcessingContext(ctx context.Context) Option { @@ -16,7 +14,7 @@ func WithTaskProcessingContext(ctx context.Context) Option { // WithNewWorker is an optional parameter for distro.New that allows for overriding // the worker.New constructor. It is meant for dependency injection. -func WithNewWorker(newWorkerFunc func(context.Context, *Distro, string, worker.Provisioning) (workerInterface, error)) Option { +func WithNewWorker(newWorkerFunc func(context.Context, *Distro, string) (workerInterface, error)) Option { return func(o *options) { o.newWorkerFunc = newWorkerFunc } diff --git a/windows-agent/internal/distros/worker/worker.go b/windows-agent/internal/distros/worker/worker.go index ab10624bc..bb19d022d 100644 --- a/windows-agent/internal/distros/worker/worker.go +++ b/windows-agent/internal/distros/worker/worker.go @@ -44,37 +44,12 @@ type Worker struct { connMu sync.RWMutex } -// Provisioning is an interface which provides provisioning tasks. -type Provisioning interface { - ProvisioningTasks(context.Context, string) ([]task.Task, error) -} - -type options struct { - provisioning Provisioning -} - -// Option is an optional argument for worker.New. -type Option func(*options) - -// WithProvisioning is an optional parameter for worker.New that allows for -// conditionally importing the provisioning tasks. -func WithProvisioning(provisioning Provisioning) Option { - return func(o *options) { - o.provisioning = provisioning - } -} - // New creates a new worker and starts it. Call Stop when you're done to avoid leaking the task execution goroutine. -func New(ctx context.Context, d distro, storageDir string, args ...Option) (w *Worker, err error) { +func New(ctx context.Context, d distro, storageDir string) (w *Worker, err error) { defer decorate.OnError(&err, "distro %q: could not create worker", d.Name()) storagePath := filepath.Join(storageDir, d.Name()+".tasks") - var opts options - for _, f := range args { - f(&opts) - } - tm, err := newTaskManager(storagePath) if err != nil { return nil, err @@ -87,21 +62,6 @@ func New(ctx context.Context, d distro, storageDir string, args ...Option) (w *W w.start(ctx) - // load and submit provisioning tasks. (case of first contact with distro) - if opts.provisioning == nil { - return w, nil - } - - provisioning, err := opts.provisioning.ProvisioningTasks(ctx, d.Name()) - if err != nil { - return w, err - } - - if err := w.SubmitTasks(provisioning...); err != nil { - w.Stop(ctx) - return nil, err - } - return w, nil } diff --git a/windows-agent/internal/distros/worker/worker_test.go b/windows-agent/internal/distros/worker/worker_test.go index 056f33b55..c09214b4f 100644 --- a/windows-agent/internal/distros/worker/worker_test.go +++ b/windows-agent/internal/distros/worker/worker_test.go @@ -53,10 +53,6 @@ func TestNew(t *testing.T) { taskFile taskFileState fillUpQueue bool - withProvisioning bool - emptyProvisioning bool - provisioningTasksErr bool - wantErr bool wantNTasks int }{ @@ -65,14 +61,10 @@ func TestNew(t *testing.T) { "Success with task file containing a single task": {taskFile: fileHasOneTask, wantNTasks: 1}, "Success with task file containing multiple tasks": {taskFile: fileHasTwoTasks, wantNTasks: 2}, - "Success with empty provisioning": {withProvisioning: true, emptyProvisioning: true}, - "Success with single-task provisioning": {withProvisioning: true, wantNTasks: 1}, - // Error "Error when task file reads non-registered task type": {taskFile: fileHasNonRegisteredTask, wantErr: true}, "Error when task file has bad syntax": {taskFile: fileHasBadSyntax, wantErr: true}, "Error when task file is unreadable": {taskFile: fileIsDir, wantErr: true}, - "Error when ProvisioningTasks fails": {withProvisioning: true, provisioningTasksErr: true, wantErr: true}, } for name, tc := range testCases { @@ -112,20 +104,11 @@ func TestNew(t *testing.T) { require.NoError(t, err, "Setup: could not make a directory in task file's location") } - var args []worker.Option - if tc.withProvisioning { - c := &mockProvisioning{ - provisioningTasksErr: tc.provisioningTasksErr, - privisioningTasksReturnsNil: tc.emptyProvisioning, - } - args = append(args, worker.WithProvisioning(c)) - } - // We pass a cancelled context so that no tasks are popped // and we can accurately assert on the task queue length. cancel() - w, err := worker.New(ctx, distro, distroDir, args...) + w, err := worker.New(ctx, distro, distroDir) if err == nil { defer w.Stop(ctx) } @@ -806,21 +789,6 @@ func taskfileFromTemplate[T task.Task](t *testing.T) []byte { return w.Bytes() } -type mockProvisioning struct { - provisioningTasksErr bool - privisioningTasksReturnsNil bool -} - -func (c mockProvisioning) ProvisioningTasks(ctx context.Context, distroName string) ([]task.Task, error) { - if c.provisioningTasksErr { - return nil, errors.New("mock error") - } - if c.privisioningTasksReturnsNil { - return nil, nil - } - return []task.Task{&testTask{}}, nil -} - type mockConnection struct { proAttachmentCount atomic.Int32 LandscapeConfigCount atomic.Int32 diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 02a169d03..426df0775 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -489,14 +489,14 @@ func testReceiveCommand(t *testing.T, distrosettings distroSettings, testSetup f } } - db, err := database.New(ctx, t.TempDir(), tb.conf) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: database New should not return an error") tb.db = db tb.cloudInit = &mockCloudInit{} // Set up Landscape client - clientService, err := landscape.New(ctx, tb.conf, tb.db, landscape.WithHostname("HOSTNAME")) + clientService, err := landscape.New(ctx, tb.conf, tb.db, tb.cloudInit, landscape.WithHostname("HOSTNAME")) require.NoError(t, err, "Landscape NewClient should not return an error") err = clientService.Connect() diff --git a/windows-agent/internal/proservices/landscape/landscape_test.go b/windows-agent/internal/proservices/landscape/landscape_test.go index 3b750b7d6..bdc04d243 100644 --- a/windows-agent/internal/proservices/landscape/landscape_test.go +++ b/windows-agent/internal/proservices/landscape/landscape_test.go @@ -24,7 +24,6 @@ import ( "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/config" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/database" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/distro" - "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/distros/task" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/proservices/landscape" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -161,7 +160,7 @@ func TestConnect(t *testing.T) { defer server.Stop() } - db, err := database.New(ctx, t.TempDir(), conf) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: database New should not return an error") distroName, _ := wsltestutils.RegisterDistro(t, ctx, true) @@ -292,7 +291,7 @@ func TestSendUpdatedInfo(t *testing.T) { go server.Serve(lis) defer server.Stop() - db, err := database.New(ctx, t.TempDir(), conf) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: database New should not return an error") var d *distro.Distro @@ -503,7 +502,7 @@ func TestAutoReconnection(t *testing.T) { landscapeClientConfig: executeLandscapeConfigTemplate(t, defaultLandscapeConfig, "", lis.Addr()), } - db, err := database.New(ctx, t.TempDir(), conf) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: database New should not return an error") const hostname = "HOSTNAME" @@ -902,10 +901,6 @@ func (m *mockConfig) LandscapeClientConfig() (string, config.Source, error) { return m.landscapeClientConfig, config.SourceUser, nil } -func (m *mockConfig) ProvisioningTasks(ctx context.Context, distroName string) ([]task.Task, error) { - return nil, nil -} - func (m *mockConfig) Subscription() (string, config.Source, error) { m.mu.Lock() defer m.mu.Unlock() diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index 61ea16354..851c3f08f 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -85,7 +85,7 @@ func New(ctx context.Context, publicDir, privateDir string, args ...Option) (s M return s, err } - db, err := database.New(ctx, privateDir, s.conf) + db, err := database.New(ctx, privateDir) if err != nil { return s, err } diff --git a/windows-agent/internal/proservices/registrywatcher/watcher_test.go b/windows-agent/internal/proservices/registrywatcher/watcher_test.go index 5a9ee9bb3..e758e73f9 100644 --- a/windows-agent/internal/proservices/registrywatcher/watcher_test.go +++ b/windows-agent/internal/proservices/registrywatcher/watcher_test.go @@ -60,7 +60,7 @@ func TestRegistryWatcher(t *testing.T) { conf := &mockConfig{} - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty DB") reg := registry.NewMock() diff --git a/windows-agent/internal/proservices/ui/ui_test.go b/windows-agent/internal/proservices/ui/ui_test.go index 6cd48f3a6..74340e140 100644 --- a/windows-agent/internal/proservices/ui/ui_test.go +++ b/windows-agent/internal/proservices/ui/ui_test.go @@ -28,7 +28,7 @@ func TestNew(t *testing.T) { t.Parallel() dir := t.TempDir() - db, err := database.New(ctx, dir, nil) + db, err := database.New(ctx, dir) require.NoError(t, err, "Setup: empty database New() should return no error") defer db.Close(ctx) @@ -71,7 +71,7 @@ func TestAttachPro(t *testing.T) { t.Parallel() dir := t.TempDir() - db, err := database.New(ctx, dir, nil) + db, err := database.New(ctx, dir) require.NoError(t, err, "Setup: empty database New() should return no error") defer db.Close(ctx) @@ -164,7 +164,7 @@ func TestGetConfigSources(t *testing.T) { ctx := context.Background() dir := t.TempDir() - db, err := database.New(ctx, dir, nil) + db, err := database.New(ctx, dir) require.NoError(t, err, "Setup: empty database New() should return no error") config := tc.config service := ui.New(ctx, &config, db) @@ -210,7 +210,7 @@ func TestNotifyPurchase(t *testing.T) { ctx := context.Background() dir := t.TempDir() - db, err := database.New(ctx, dir, nil) + db, err := database.New(ctx, dir) require.NoError(t, err, "Setup: empty database New() should return no error") opts, stop := setupMockContracts(t, ctx) @@ -265,7 +265,7 @@ func TestApplyLandscapeConfig(t *testing.T) { landscapeConfig := "look at me! I am a Landscape config" dir := t.TempDir() - db, err := database.New(ctx, dir, nil) + db, err := database.New(ctx, dir) require.NoError(t, err, "Setup: empty database New() should return no error") defer db.Close(ctx) diff --git a/windows-agent/internal/proservices/wslinstance/wslinstance_test.go b/windows-agent/internal/proservices/wslinstance/wslinstance_test.go index 96fcae71c..44acd1ad3 100644 --- a/windows-agent/internal/proservices/wslinstance/wslinstance_test.go +++ b/windows-agent/internal/proservices/wslinstance/wslinstance_test.go @@ -77,7 +77,7 @@ func TestServe(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") landscape := &landscapeCtlMock{} @@ -206,7 +206,7 @@ func TestSendCommands(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: could not create empty database") landscape := &landscapeCtlMock{} diff --git a/windows-agent/internal/ubuntupro/ubuntupro_test.go b/windows-agent/internal/ubuntupro/ubuntupro_test.go index 21ec0cf84..70e732639 100644 --- a/windows-agent/internal/ubuntupro/ubuntupro_test.go +++ b/windows-agent/internal/ubuntupro/ubuntupro_test.go @@ -43,7 +43,7 @@ func TestDistribute(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - db, err := database.New(ctx, t.TempDir(), nil) + db, err := database.New(ctx, t.TempDir()) require.NoError(t, err, "Setup: Database creation should return no error") distroName, _ := wsltestutils.RegisterDistro(t, ctx, false) From 92fb914c1597cf7259e25ccb0db8d790a316c2ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Fri, 16 Feb 2024 16:13:22 +0100 Subject: [PATCH 06/31] Add end-to-end test validating cloud-init functionality --- end-to-end/cloud_init_test.go | 116 ++++++++++++++++++ end-to-end/landscape_utils_test.go | 11 +- end-to-end/manual_token_input_test.go | 4 +- end-to-end/organization_token_test.go | 4 +- end-to-end/purchase_test.go | 4 +- .../TestCloudInitIntegration/user-data.yaml | 13 ++ .../ubuntupro/end_to_end/end_to_end_test.dart | 1 + 7 files changed, 143 insertions(+), 10 deletions(-) create mode 100644 end-to-end/cloud_init_test.go create mode 100644 end-to-end/testdata/TestCloudInitIntegration/user-data.yaml diff --git a/end-to-end/cloud_init_test.go b/end-to-end/cloud_init_test.go new file mode 100644 index 000000000..c84106642 --- /dev/null +++ b/end-to-end/cloud_init_test.go @@ -0,0 +1,116 @@ +package endtoend_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + landscapeapi "github.com/canonical/landscape-hostagent-api" + "github.com/canonical/ubuntu-pro-for-wsl/common/testutils" + "github.com/stretchr/testify/require" + wsl "github.com/ubuntu/gowsl" +) + +func TestCloudInitIntegration(t *testing.T) { + currentFuncName := t.Name() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + testSetup(t) + defer logWindowsAgentOnError(t) + + landscape := NewLandscape(t, ctx) + writeUbuntuProRegistry(t, "LandscapeConfig", landscape.ClientConfig) + + go landscape.Serve() + defer landscape.LogOnError(t) + defer landscape.Stop() + + hostname, err := os.Hostname() + require.NoError(t, err, "Setup: could not test machine's hostname") + + proToken := os.Getenv(proTokenEnv) + require.NotEmptyf(t, proToken, "Setup: environment variable %q should contain a valid pro token, but is empty", proTokenEnv) + writeUbuntuProRegistry(t, "UbuntuProToken", proToken) + + cleanup := startAgent(t, ctx, currentFuncName) + defer cleanup() + + info := landscape.RequireReceivedInfo(t, proToken, nil, hostname) + + out, err := os.ReadFile(filepath.Join(testutils.TestFixturePath(t), "user-data.yaml")) + require.NoError(t, err, "Setup: could not read cloud-init file") + cloudInitUserData := string(out) + + err = landscape.service.SendCommand(ctx, info.UID, &landscapeapi.Command{ + Cmd: &landscapeapi.Command_Install_{ + Install: &landscapeapi.Command_Install{ + Id: strings.Split(referenceDistroAppx, ".")[1], // CanonicalGroupLimited.[UbuntuPreview] + Cloudinit: &cloudInitUserData, + }, + }, + }) + require.NoError(t, err, "Setup: could not send install command") + + distro := wsl.NewDistro(ctx, referenceDistro) + + //nolint:errcheck // Nothing we can do about it + defer distro.Unregister() + + require.Eventually(t, func() bool { + ok, err := distro.IsRegistered() + if err != nil { + t.Logf("could not determine if distro is registered: %v", err) + return false + } + return ok + }, time.Minute, time.Second, "Distro should have been registered") + + defer logWslProServiceOnError(t, ctx, distro) + + runCommand(t, ctx, time.Minute, distro, "cloud-init status --wait") + + require.Eventually(t, func() bool { + attached, err := distroIsProAttached(t, ctx, distro) + if err != nil { + t.Logf("could not determine if distro is attached: %v", err) + return false + } + return attached + }, 10*time.Second, time.Second, "distro should have been Pro attached") + + userName := runCommand(t, ctx, 10*time.Second, distro, "whoami") + require.Equal(t, "testuser", userName, "cloud-init should have configured the default user") + + landscape.RequireReceivedInfo(t, proToken, []wsl.Distro{distro}, hostname) + landscape.RequireUninstallCommand(t, ctx, distro, info) +} + +//nolint:revive // t always goes before ctx +func runCommand(t *testing.T, ctx context.Context, timeout time.Duration, distro wsl.Distro, comand string) string { + t.Helper() + + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + out, err := distro.Command(ctx, comand).CombinedOutput() + if err == nil { + return string(out) + } + + // We check the context to see if it was a timeout. + // This makes the error message easier to understand. + + select { + case <-ctx.Done(): + require.Fail(t, "Timed out waiting for cloud-init to finish") + default: + } + + require.NoError(t, err, "could not determine if cloud-init is done: %s. Output: %s", out, out) + return "" +} diff --git a/end-to-end/landscape_utils_test.go b/end-to-end/landscape_utils_test.go index 0331bdcec..759357348 100644 --- a/end-to-end/landscape_utils_test.go +++ b/end-to-end/landscape_utils_test.go @@ -109,7 +109,7 @@ func (l landscape) Stop() { } // RequireReceivedInfo checks that a connection to Landscape was made and the proper information was sent. -func (l landscape) RequireReceivedInfo(t *testing.T, wantToken string, wantDistro gowsl.Distro, wantHostname string) landscapemockservice.HostInfo { +func (l landscape) RequireReceivedInfo(t *testing.T, wantToken string, wantDistros []gowsl.Distro, wantHostname string) landscapemockservice.HostInfo { t.Helper() require.Eventually(t, func() bool { @@ -122,9 +122,12 @@ func (l landscape) RequireReceivedInfo(t *testing.T, wantToken string, wantDistr // Validate token require.Equal(t, wantToken, info.Token, "Landscape did not receive the right pro token") - // Validate distro - require.Len(t, info.Instances, 1, "Landscape did not receive the right number of distros") - require.Equal(t, wantDistro.Name(), info.Instances[0].ID, "Landscape did not receive the right distro name from the agent") + // Validate distros + gotDistros := make([]string, 0) + for _, instance := range info.Instances { + gotDistros = append(gotDistros, instance.ID) + } + require.ElementsMatch(t, wantDistros, gotDistros, "Landscape did not receive the right distros") // Validate hostname require.Equal(t, wantHostname, info.Hostname, "Landscape did not receive the right hostname from the agent") diff --git a/end-to-end/manual_token_input_test.go b/end-to-end/manual_token_input_test.go index b2ed54577..3c43b01fc 100644 --- a/end-to-end/manual_token_input_test.go +++ b/end-to-end/manual_token_input_test.go @@ -63,7 +63,7 @@ func TestManualTokenInput(t *testing.T) { defer logWslProServiceOnError(t, ctx, d) - out, err := d.Command(ctx, "exit 0").CombinedOutput() + out, err := d.Command(ctx, "cloud-init status --wait").CombinedOutput() require.NoErrorf(t, err, "Setup: could not wake distro up: %v. %s", err, out) // ... or after registration, but never both. @@ -94,7 +94,7 @@ func TestManualTokenInput(t *testing.T) { return attached }, maxTimeout, time.Second, "distro should have been Pro attached") - info := landscape.RequireReceivedInfo(t, os.Getenv(proTokenEnv), d, hostname) + info := landscape.RequireReceivedInfo(t, os.Getenv(proTokenEnv), []wsl.Distro{d}, hostname) landscape.RequireUninstallCommand(t, ctx, d, info) }) } diff --git a/end-to-end/organization_token_test.go b/end-to-end/organization_token_test.go index 232d721fc..2d5680f19 100644 --- a/end-to-end/organization_token_test.go +++ b/end-to-end/organization_token_test.go @@ -65,7 +65,7 @@ func TestOrganizationProvidedToken(t *testing.T) { defer logWslProServiceOnError(t, ctx, d) - out, err := d.Command(ctx, "exit 0").CombinedOutput() + out, err := d.Command(ctx, "cloud-init status --wait").CombinedOutput() require.NoErrorf(t, err, "Setup: could not wake distro up: %v. %s", err, out) if tc.whenToken == afterDistroRegistration { @@ -99,7 +99,7 @@ func TestOrganizationProvidedToken(t *testing.T) { return attached }, maxTimeout, time.Second, "distro should have been Pro attached") - info := landscape.RequireReceivedInfo(t, proToken, d, hostname) + info := landscape.RequireReceivedInfo(t, proToken, []wsl.Distro{d}, hostname) landscape.RequireUninstallCommand(t, ctx, d, info) }) } diff --git a/end-to-end/purchase_test.go b/end-to-end/purchase_test.go index e8950c87c..4506e1fd6 100644 --- a/end-to-end/purchase_test.go +++ b/end-to-end/purchase_test.go @@ -121,7 +121,7 @@ func TestPurchase(t *testing.T) { defer logWslProServiceOnError(t, ctx, d) - out, err := d.Command(ctx, "exit 0").CombinedOutput() + out, err := d.Command(ctx, "cloud-init status --wait").CombinedOutput() require.NoErrorf(t, err, "Setup: could not wake distro up: %v. %s", err, out) // ... or after registration, but never both. @@ -153,7 +153,7 @@ func TestPurchase(t *testing.T) { return attached }, maxTimeout, time.Second, "distro should have been Pro attached") - landscape.RequireReceivedInfo(t, token, d, hostname) + landscape.RequireReceivedInfo(t, token, []wsl.Distro{d}, hostname) // Skipping because we know it to be broken // See https://warthogs.atlassian.net/browse/UDENG-1810 // diff --git a/end-to-end/testdata/TestCloudInitIntegration/user-data.yaml b/end-to-end/testdata/TestCloudInitIntegration/user-data.yaml new file mode 100644 index 000000000..2857b3486 --- /dev/null +++ b/end-to-end/testdata/TestCloudInitIntegration/user-data.yaml @@ -0,0 +1,13 @@ +#cloud-config +users: + - name: testuser + gecos: Test User + sudo: ALL=(ALL) NOPASSWD:ALL + groups: [adm,dialout,cdrom,floppy,sudo,audio,dip,video,plugdev,users,netdev] + shell: /bin/bash +write_files: +- path: /etc/wsl.conf + append: true + content: | + [user] + default=testuser diff --git a/gui/packages/ubuntupro/end_to_end/end_to_end_test.dart b/gui/packages/ubuntupro/end_to_end/end_to_end_test.dart index bd89cbb04..4d6ee04ff 100644 --- a/gui/packages/ubuntupro/end_to_end/end_to_end_test.dart +++ b/gui/packages/ubuntupro/end_to_end/end_to_end_test.dart @@ -26,6 +26,7 @@ void main(List args) { const testCases = { 'TestOrganizationProvidedToken': testOrganizationProvidedToken, + 'TestCloudInitIntegration': testOrganizationProvidedToken, 'TestManualTokenInput': testManualTokenInput, 'TestPurchase': testPurchase, }; From a99ba06f22219bca8760b61340951efe6c8cb6f9 Mon Sep 17 00:00:00 2001 From: Carlos Date: Wed, 3 Apr 2024 11:06:38 -0300 Subject: [PATCH 07/31] Fix wrong decorate message That function is about agent data, not user's. --- windows-agent/internal/cloudinit/cloudinit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go index 4f996972a..3917f31b8 100644 --- a/windows-agent/internal/cloudinit/cloudinit.go +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -53,7 +53,7 @@ func (c CloudInit) Notify(ctx context.Context) { // WriteAgentData writes the agent's cloud-init data file. func (c CloudInit) WriteAgentData() (err error) { - defer decorate.OnError(&err, "could not create distro-specific cloud-init file") + defer decorate.OnError(&err, "could not create agent's cloud-init file") cloudInit, err := marshalConfig(c.conf) if err != nil { From 455f6d2cb98b6384ced0bc2bf9a507b4c6bdf020 Mon Sep 17 00:00:00 2001 From: Carlos Date: Thu, 4 Apr 2024 23:05:41 -0300 Subject: [PATCH 08/31] Add a test case in proservices breaking cloud-init initialization --- windows-agent/internal/proservices/proservices_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index 3ff547b46..2fd69fa99 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -37,6 +37,7 @@ func TestNew(t *testing.T) { breakNewDistroDB bool breakCertificatesDir bool breakCA bool + breakCloudInit bool wantErr bool }{ @@ -46,6 +47,7 @@ func TestNew(t *testing.T) { "Error when database cannot create its dump file": {breakNewDistroDB: true, wantErr: true}, "Error when certificates directory cannot be created": {breakCertificatesDir: true, wantErr: true}, "Error when CA certificate cannot be created": {breakCA: true, wantErr: true}, + "Error when cloud-init dir cannot be created": {breakCloudInit: true, wantErr: true}, } for name, tc := range testCases { @@ -73,6 +75,12 @@ func TestNew(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(publicDir, common.CertificatesDir, common.RootCACertFileName), 0700), "Setup: could not break the root CA certificate file") } + if tc.breakCloudInit { + f, err := os.Create(filepath.Join(publicDir, ".cloud-init")) + require.NoError(t, err, "Setup: could not write the file that replaces cloud-init data directory") + f.Close() + } + s, err := proservices.New(ctx, publicDir, privateDir, proservices.WithRegistry(reg)) if err == nil { defer s.Stop(ctx) From 0c0c0389a582ea2bd138fd58a20945c46e7e0196 Mon Sep 17 00:00:00 2001 From: Carlos Date: Thu, 4 Apr 2024 23:28:41 -0300 Subject: [PATCH 09/31] Fix removeErr logic in TestUninstall it's about cloud-init failing to remove a file, not to write it. --- windows-agent/internal/proservices/landscape/executor_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 426df0775..6c45777d6 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -267,7 +267,7 @@ func TestUninstall(t *testing.T) { // Test setup func(testBed *commandTestBed) *landscapeapi.Command { if tc.cloudInitRemoveErr { - testBed.cloudInit.writeErr = true + testBed.cloudInit.removeErr = true } if tc.wslUninstallErr { From 9edc2ad5ad2edc96889359c168d0903cf66c4023 Mon Sep 17 00:00:00 2001 From: Carlos Date: Thu, 4 Apr 2024 23:30:04 -0300 Subject: [PATCH 10/31] Marshall the cloud-init data heading in a single line There is no specific reason to split that writing. Also, less lines for codecov to complain about. --- windows-agent/internal/cloudinit/cloudinit.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go index 3917f31b8..0e4c66d16 100644 --- a/windows-agent/internal/cloudinit/cloudinit.go +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -122,12 +122,8 @@ func (c CloudInit) RemoveDistroData(distroName string) (err error) { func marshalConfig(conf Config) ([]byte, error) { w := &bytes.Buffer{} - if _, err := fmt.Fprintln(w, "# cloud-init"); err != nil { - return nil, fmt.Errorf("could not write # cloud-init stenza: %v", err) - } - - if _, err := fmt.Fprintln(w, "# This file was generated automatically and must not be edited"); err != nil { - return nil, fmt.Errorf("could not write warning message: %v", err) + if _, err := fmt.Fprintln(w, "# cloud-init\n# This file was generated automatically and must not be edited"); err != nil { + return nil, fmt.Errorf("could not write # cloud-init stenza and warning message: %v", err) } contents := make(map[string]interface{}) From 870fe9c2794bb6e9f87a1573e6ea82526d225e30 Mon Sep 17 00:00:00 2001 From: Carlos Date: Thu, 4 Apr 2024 23:28:04 -0300 Subject: [PATCH 11/31] Test Landscape assign host with an empty UID Since we are passing nearby the codecov is unhappy --- .../internal/proservices/landscape/executor_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 6c45777d6..1f128820e 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -28,10 +28,12 @@ func TestAssignHost(t *testing.T) { testCases := map[string]struct { confErr bool + uid string wantErr bool }{ - "Success ": {}, + "Success with some uid": {uid: "HostUID123"}, + "Error with an empty uid": {uid: "", wantErr: true}, "Error when config returns an error": {confErr: true, wantErr: true}, } @@ -47,7 +49,7 @@ func TestAssignHost(t *testing.T) { } return &landscapeapi.Command{ - Cmd: &landscapeapi.Command_AssignHost_{AssignHost: &landscapeapi.Command_AssignHost{Uid: "HostUID123"}}, + Cmd: &landscapeapi.Command_AssignHost_{AssignHost: &landscapeapi.Command_AssignHost{Uid: tc.uid}}, } }, // Test assertions @@ -55,7 +57,7 @@ func TestAssignHost(t *testing.T) { const maxTimeout = time.Second if tc.wantErr { time.Sleep(maxTimeout) - require.NotEqual(t, "HostUID123", testBed.conf.landscapeAgentUID, "Landscape UID should not have been assigned") + require.NotEqual(t, tc.uid, testBed.conf.landscapeAgentUID, "Landscape UID should not have been assigned") return } @@ -63,7 +65,7 @@ func TestAssignHost(t *testing.T) { testBed.conf.mu.Lock() defer testBed.conf.mu.Unlock() - return testBed.conf.landscapeAgentUID == "HostUID123" + return testBed.conf.landscapeAgentUID == tc.uid }, maxTimeout, 100*time.Millisecond, "Landscape client should have overridden the initial UID sent by the server") }) }) From 57060d70dc1340047e62ef2dcc804d22bd962117 Mon Sep 17 00:00:00 2001 From: Carlos Date: Fri, 5 Apr 2024 10:10:54 -0300 Subject: [PATCH 12/31] t.Skip() cloud-init end-to-end test case --- end-to-end/cloud_init_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/end-to-end/cloud_init_test.go b/end-to-end/cloud_init_test.go index c84106642..2e19af631 100644 --- a/end-to-end/cloud_init_test.go +++ b/end-to-end/cloud_init_test.go @@ -15,6 +15,9 @@ import ( ) func TestCloudInitIntegration(t *testing.T) { + // TODO: Remove this line when cloud-init support for UP4W is released. + // Follow this PR for more information: https://github.com/canonical/cloud-init/pull/5116 + t.Skip("This test depends on cloud-init support for UP4W being released.") currentFuncName := t.Name() ctx, cancel := context.WithCancel(context.Background()) From 230532ba1de630dd76089dc332f38e55851db36a Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 14:46:50 -0300 Subject: [PATCH 13/31] Ensures landscape server stops at t.Cleanup --- end-to-end/cloud_init_test.go | 11 +++++++++-- end-to-end/manual_token_input_test.go | 11 +++++++++-- end-to-end/organization_token_test.go | 11 +++++++++-- end-to-end/purchase_test.go | 11 +++++++++-- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/end-to-end/cloud_init_test.go b/end-to-end/cloud_init_test.go index 2e19af631..6030855a6 100644 --- a/end-to-end/cloud_init_test.go +++ b/end-to-end/cloud_init_test.go @@ -29,9 +29,16 @@ func TestCloudInitIntegration(t *testing.T) { landscape := NewLandscape(t, ctx) writeUbuntuProRegistry(t, "LandscapeConfig", landscape.ClientConfig) - go landscape.Serve() + serverDone := make(chan struct{}) + go func() { + defer close(serverDone) + landscape.Serve() + }() + t.Cleanup(func() { + landscape.Stop() + <-serverDone + }) defer landscape.LogOnError(t) - defer landscape.Stop() hostname, err := os.Hostname() require.NoError(t, err, "Setup: could not test machine's hostname") diff --git a/end-to-end/manual_token_input_test.go b/end-to-end/manual_token_input_test.go index 3c43b01fc..e237c6af4 100644 --- a/end-to-end/manual_token_input_test.go +++ b/end-to-end/manual_token_input_test.go @@ -44,9 +44,16 @@ func TestManualTokenInput(t *testing.T) { landscape := NewLandscape(t, ctx) writeUbuntuProRegistry(t, "LandscapeConfig", landscape.ClientConfig) - go landscape.Serve() + serverDone := make(chan struct{}) + go func() { + defer close(serverDone) + landscape.Serve() + }() + t.Cleanup(func() { + landscape.Stop() + <-serverDone + }) defer landscape.LogOnError(t) - defer landscape.Stop() hostname, err := os.Hostname() require.NoError(t, err, "Setup: could not test machine's hostname") diff --git a/end-to-end/organization_token_test.go b/end-to-end/organization_token_test.go index 2d5680f19..433c52001 100644 --- a/end-to-end/organization_token_test.go +++ b/end-to-end/organization_token_test.go @@ -43,9 +43,16 @@ func TestOrganizationProvidedToken(t *testing.T) { landscape := NewLandscape(t, ctx) writeUbuntuProRegistry(t, "LandscapeConfig", landscape.ClientConfig) - go landscape.Serve() + serverDone := make(chan struct{}) + go func() { + defer close(serverDone) + landscape.Serve() + }() + t.Cleanup(func() { + landscape.Stop() + <-serverDone + }) defer landscape.LogOnError(t) - defer landscape.Stop() hostname, err := os.Hostname() require.NoError(t, err, "Setup: could not test machine's hostname") diff --git a/end-to-end/purchase_test.go b/end-to-end/purchase_test.go index 4506e1fd6..f01c1b917 100644 --- a/end-to-end/purchase_test.go +++ b/end-to-end/purchase_test.go @@ -58,9 +58,16 @@ func TestPurchase(t *testing.T) { landscape := NewLandscape(t, ctx) writeUbuntuProRegistry(t, "LandscapeConfig", landscape.ClientConfig) - go landscape.Serve() + serverDone := make(chan struct{}) + go func() { + defer close(serverDone) + landscape.Serve() + }() + t.Cleanup(func() { + landscape.Stop() + <-serverDone + }) defer landscape.LogOnError(t) - defer landscape.Stop() hostname, err := os.Hostname() require.NoError(t, err, "Setup: could not test machine's hostname") From 570d2253e079130ca05c123816550ee3db1a1cd4 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 15:08:33 -0300 Subject: [PATCH 14/31] Removes "Success" prefix from test case names on the testcases definitions we touched on in this PR. --- .../internal/cloudinit/cloudinit_test.go | 16 ++++++++-------- ...s_with_empty_contents => with_empty_contents} | 0 ...ccess_without_landscape => without_landscape} | 0 ...ection => without_landscape_[client]_section} | 0 ...ccess_without_pro_token => without_pro_token} | 0 .../proservices/landscape/executor_test.go | 10 +++++----- .../internal/proservices/proservices_test.go | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-) rename windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/{success_with_empty_contents => with_empty_contents} (100%) rename windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/{success_without_landscape => without_landscape} (100%) rename windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/{success_without_landscape_[client]_section => without_landscape_[client]_section} (100%) rename windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/{success_without_pro_token => without_pro_token} (100%) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index 4a45b7a35..8bd48c4c6 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -93,11 +93,11 @@ url = www.example.com/new/rickroll wantErr bool }{ - "Success": {}, - "Success without pro token": {skipProToken: true}, - "Success without Landscape": {skipLandscapeConf: true}, - "Success without Landscape [client] section": {landscapeNoClientSection: true}, - "Success with empty contents": {skipProToken: true, skipLandscapeConf: true}, + "Success": {}, + "Without pro token": {skipProToken: true}, + "Without Landscape": {skipLandscapeConf: true}, + "Without Landscape [client] section": {landscapeNoClientSection: true}, + "With empty contents": {skipProToken: true, skipLandscapeConf: true}, "Error obtaining pro token": {breakSubscription: true, wantErr: true}, "Error obtaining Landscape config": {breakLandscape: true, wantErr: true}, @@ -237,9 +237,9 @@ data: wantErr bool }{ - "Success": {}, - "Success with no old data": {noOldData: true}, - "Success with empty data": {emptyData: true}, + "Success": {}, + "With no old data": {noOldData: true}, + "With empty data": {emptyData: true}, "Error when the datadir cannot be created": {breakDir: true, wantErr: true}, "Error when the temp file cannot be written": {breakTempFile: true, wantErr: true}, diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_with_empty_contents b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/with_empty_contents similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_with_empty_contents rename to windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/with_empty_contents diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape rename to windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape_[client]_section b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape_[client]_section similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_landscape_[client]_section rename to windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape_[client]_section diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_pro_token b/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_pro_token similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success_without_pro_token rename to windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_pro_token diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 1f128820e..13f97aa85 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -31,7 +31,7 @@ func TestAssignHost(t *testing.T) { uid string wantErr bool }{ - "Success with some uid": {uid: "HostUID123"}, + "With some uid": {uid: "HostUID123"}, "Error with an empty uid": {uid: "", wantErr: true}, "Error when config returns an error": {confErr: true, wantErr: true}, @@ -92,8 +92,8 @@ func TestReceiveCommandStartStop(t *testing.T) { wantState wsl.State wantErr bool }{ - "Success with command Start": {cmd: start, wantState: wsl.Running}, - "Success with command Stop": {cmd: stop, wantState: wsl.Stopped}, + "With command Start": {cmd: start, wantState: wsl.Running}, + "With command Stop": {cmd: stop, wantState: wsl.Stopped}, "Error with Start when the distro does not exist": {cmd: start, dontRegisterDistro: true, wantErr: true}, "Error with Stop when the distro does not exist": {cmd: stop, dontRegisterDistro: true, wantErr: true}, @@ -156,8 +156,8 @@ func TestInstall(t *testing.T) { wantInstalled bool wantNonRootUser bool }{ - "Success": {wantCouldInitWriteCalled: true, wantInstalled: true}, - "Success with no cloud-init": {noCloudInit: true, wantCouldInitWriteCalled: true, wantInstalled: true, wantNonRootUser: true}, + "Success": {wantCouldInitWriteCalled: true, wantInstalled: true}, + "With no cloud-init": {noCloudInit: true, wantCouldInitWriteCalled: true, wantInstalled: true, wantNonRootUser: true}, "Error when the distroname is empty": {emptyDistroName: true}, "Error when the Appx does not exist": {appxDoesNotExist: true}, diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index 2fd69fa99..85dd08db6 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -41,8 +41,8 @@ func TestNew(t *testing.T) { wantErr bool }{ - "Success when the subscription stays empty": {}, - "Success when the config cannot check if it is read-only": {breakConfig: true}, + "When the subscription stays empty": {}, + "When the config cannot check if it is read-only": {breakConfig: true}, "Error when database cannot create its dump file": {breakNewDistroDB: true, wantErr: true}, "Error when certificates directory cannot be created": {breakCertificatesDir: true, wantErr: true}, From 3d6482677ea56c1c1dc12980db29d4cb9e640ed3 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 15:15:52 -0300 Subject: [PATCH 15/31] Makes WriteAgentData private and testing Update (Notify) - Removing common golden; - Generating new goldens for each error case -- even if they are equal - Renaming Notify into Update; - Renaming golden directory - to match the new test case name - Asserting on the goldens instead of on err. - There is no tc.wantErr anymore. --- windows-agent/internal/cloudinit/cloudinit.go | 12 ++--- .../internal/cloudinit/cloudinit_test.go | 53 ++++--------------- .../golden/error-cases | 0 .../golden/error_obtaining_landscape_config | 8 +++ .../golden/error_obtaining_pro_token | 8 +++ ...error_when_the_temp_file_cannot_be_written | 8 +++ .../error_with_erroneous_landscape_config | 8 +++ .../golden/success | 0 .../golden/with_empty_contents | 0 .../golden/without_landscape | 0 .../golden/without_landscape_[client]_section | 0 .../golden/without_pro_token | 0 .../internal/proservices/proservices.go | 4 +- 13 files changed, 49 insertions(+), 52 deletions(-) rename windows-agent/internal/cloudinit/testdata/{TestWriteAgentData => TestUpdate}/golden/error-cases (100%) create mode 100644 windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_landscape_config create mode 100644 windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_pro_token create mode 100644 windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_when_the_temp_file_cannot_be_written create mode 100644 windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_with_erroneous_landscape_config rename windows-agent/internal/cloudinit/testdata/{TestWriteAgentData => TestUpdate}/golden/success (100%) rename windows-agent/internal/cloudinit/testdata/{TestWriteAgentData => TestUpdate}/golden/with_empty_contents (100%) rename windows-agent/internal/cloudinit/testdata/{TestWriteAgentData => TestUpdate}/golden/without_landscape (100%) rename windows-agent/internal/cloudinit/testdata/{TestWriteAgentData => TestUpdate}/golden/without_landscape_[client]_section (100%) rename windows-agent/internal/cloudinit/testdata/{TestWriteAgentData => TestUpdate}/golden/without_pro_token (100%) diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go index 0e4c66d16..39f3db786 100644 --- a/windows-agent/internal/cloudinit/cloudinit.go +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -37,22 +37,22 @@ func New(ctx context.Context, conf Config, publicDir string) (CloudInit, error) conf: conf, } - if err := c.WriteAgentData(); err != nil { + if err := c.writeAgentData(); err != nil { return c, err } return c, nil } -// Notify is syntax sugar to call WriteAgentData and log any error. -func (c CloudInit) Notify(ctx context.Context) { - if err := c.WriteAgentData(); err != nil { +// Update is syntax sugar to call writeAgentData and log any error. +func (c CloudInit) Update(ctx context.Context) { + if err := c.writeAgentData(); err != nil { log.Warningf(ctx, "Cloud-init: %v", err) } } -// WriteAgentData writes the agent's cloud-init data file. -func (c CloudInit) WriteAgentData() (err error) { +// writeAgentData writes the agent's cloud-init data file. +func (c CloudInit) writeAgentData() (err error) { defer decorate.OnError(&err, "could not create agent's cloud-init file") cloudInit, err := marshalConfig(c.conf) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index 8bd48c4c6..a57a434b0 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -5,7 +5,6 @@ import ( "errors" "os" "path/filepath" - "sync" "testing" "github.com/canonical/ubuntu-pro-for-wsl/common/testutils" @@ -51,12 +50,9 @@ func TestNew(t *testing.T) { } } -func TestWriteAgentData(t *testing.T) { +func TestUpdate(t *testing.T) { t.Parallel() - // All error cases share a golden file so we need to protect it during updates - var sharedGolden goldenMutex - const landscapeConfigOld string = `[irrelevant] info=this section should have been omitted @@ -90,8 +86,6 @@ url = www.example.com/new/rickroll breakDir bool breakTempFile bool breakFile bool - - wantErr bool }{ "Success": {}, "Without pro token": {skipProToken: true}, @@ -99,13 +93,13 @@ url = www.example.com/new/rickroll "Without Landscape [client] section": {landscapeNoClientSection: true}, "With empty contents": {skipProToken: true, skipLandscapeConf: true}, - "Error obtaining pro token": {breakSubscription: true, wantErr: true}, - "Error obtaining Landscape config": {breakLandscape: true, wantErr: true}, - "Error with erroneous Landscape config": {badLandscape: true, wantErr: true}, + "Error obtaining pro token": {breakSubscription: true}, + "Error obtaining Landscape config": {breakLandscape: true}, + "Error with erroneous Landscape config": {badLandscape: true}, - "Error when the datadir cannot be created": {breakDir: true, wantErr: true}, - "Error when the temp file cannot be written": {breakTempFile: true, wantErr: true}, - "Error when the temp file cannot be renamed": {breakFile: true, wantErr: true}, + "Error when the datadir cannot be created": {breakDir: true}, + "Error when the temp file cannot be written": {breakTempFile: true}, + "Error when the temp file cannot be renamed": {breakFile: true}, } for name, tc := range testCases { @@ -162,15 +156,7 @@ url = www.example.com/new/rickroll require.NoError(t, os.WriteFile(dir, nil, 0600), "Setup: could not create file to mess with cloud-init directory") } - err = ci.WriteAgentData() - var opts []testutils.Option - if tc.wantErr { - require.Error(t, err, "WriteAgentData should have returned an error") - errorGolden := filepath.Join(testutils.TestFamilyPath(t), "golden", "error-cases") - opts = append(opts, testutils.WithGoldenPath(errorGolden)) - } else { - require.NoError(t, err, "WriteAgentData should return no errors") - } + ci.Update(ctx) // Assert that the file was updated (success case) or that the old one remains (error case) if tc.breakFile || tc.breakDir { @@ -181,34 +167,13 @@ url = www.example.com/new/rickroll got, err := os.ReadFile(path) require.NoError(t, err, "There should be no error reading the cloud-init agent file") - sharedGolden.Lock() - defer sharedGolden.Unlock() + want := testutils.LoadWithUpdateFromGolden(t, string(got)) - want := testutils.LoadWithUpdateFromGolden(t, string(got), opts...) require.Equal(t, want, string(got), "Agent cloud-init file does not match the golden file") }) } } -// goldenMutex is a mutex that only works when golden update is enabled. -type goldenMutex struct { - sync.Mutex -} - -func (mu *goldenMutex) Lock() { - if !testutils.UpdateEnabled() { - return - } - mu.Mutex.Lock() -} - -func (mu *goldenMutex) Unlock() { - if !testutils.UpdateEnabled() { - return - } - mu.Mutex.Unlock() -} - func TestWriteDistroData(t *testing.T) { t.Parallel() diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/error-cases b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error-cases similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/error-cases rename to windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error-cases diff --git a/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_landscape_config b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_landscape_config new file mode 100644 index 000000000..882b62c56 --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_landscape_config @@ -0,0 +1,8 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + data: This is an old data field + info: This is the old configuration +ubuntu_advantage: + token: OLD_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_pro_token b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_pro_token new file mode 100644 index 000000000..882b62c56 --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_obtaining_pro_token @@ -0,0 +1,8 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + data: This is an old data field + info: This is the old configuration +ubuntu_advantage: + token: OLD_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_when_the_temp_file_cannot_be_written b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_when_the_temp_file_cannot_be_written new file mode 100644 index 000000000..882b62c56 --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_when_the_temp_file_cannot_be_written @@ -0,0 +1,8 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + data: This is an old data field + info: This is the old configuration +ubuntu_advantage: + token: OLD_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_with_erroneous_landscape_config b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_with_erroneous_landscape_config new file mode 100644 index 000000000..882b62c56 --- /dev/null +++ b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/error_with_erroneous_landscape_config @@ -0,0 +1,8 @@ +# cloud-init +# This file was generated automatically and must not be edited +landscape: + client: + data: This is an old data field + info: This is the old configuration +ubuntu_advantage: + token: OLD_PRO_TOKEN diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/success similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/success rename to windows-agent/internal/cloudinit/testdata/TestUpdate/golden/success diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/with_empty_contents b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/with_empty_contents similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/with_empty_contents rename to windows-agent/internal/cloudinit/testdata/TestUpdate/golden/with_empty_contents diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/without_landscape similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape rename to windows-agent/internal/cloudinit/testdata/TestUpdate/golden/without_landscape diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape_[client]_section b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/without_landscape_[client]_section similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_landscape_[client]_section rename to windows-agent/internal/cloudinit/testdata/TestUpdate/golden/without_landscape_[client]_section diff --git a/windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_pro_token b/windows-agent/internal/cloudinit/testdata/TestUpdate/golden/without_pro_token similarity index 100% rename from windows-agent/internal/cloudinit/testdata/TestWriteAgentData/golden/without_pro_token rename to windows-agent/internal/cloudinit/testdata/TestUpdate/golden/without_pro_token diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index 851c3f08f..6429315a2 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -107,12 +107,12 @@ func New(ctx context.Context, publicDir, privateDir string, args ...Option) (s M conf.SetUbuntuProNotifier(func(ctx context.Context, token string) { ubuntupro.Distribute(ctx, s.db, token) landscape.NotifyUbuntuProUpdate(ctx, token) - cloudInit.Notify(ctx) + cloudInit.Update(ctx) }) conf.SetLandscapeNotifier(func(ctx context.Context, conf, uid string) { landscape.NotifyConfigUpdate(ctx, conf, uid) - cloudInit.Notify(ctx) + cloudInit.Update(ctx) }) // All notifications have been set up: starting the registry watcher before any services. From 298c712561979b8b812db73ca47b1450e3ac41fd Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:02:46 -0300 Subject: [PATCH 16/31] Log if we leave undesired temp files in writeFileInDir --- windows-agent/internal/cloudinit/cloudinit.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go index 39f3db786..e56d1d09d 100644 --- a/windows-agent/internal/cloudinit/cloudinit.go +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -94,7 +94,9 @@ func writeFileInDir(dir string, file string, contents []byte) error { } if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) + if r := os.Remove(tmp); r != nil { + log.Warningf(context.Background(), "could not remove temporary file: %v", r) + } return err // Error message already says 'cannot rename' } From 9f67749fca0d7799b225486f30ace47a67472a1a Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:04:27 -0300 Subject: [PATCH 17/31] Stylistic else if in RemoveDistroData --- windows-agent/internal/cloudinit/cloudinit.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go index e56d1d09d..6fe01b4fb 100644 --- a/windows-agent/internal/cloudinit/cloudinit.go +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -114,8 +114,7 @@ func (c CloudInit) RemoveDistroData(distroName string) (err error) { err = os.Remove(path) if errors.Is(err, fs.ErrNotExist) { return nil - } - if err != nil { + } else if err != nil { return err } return nil From 7894e8ee7afe058bbda5f6a2db422eddb875adc8 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:07:27 -0300 Subject: [PATCH 18/31] Cloudinit's TestNew to check if returned is nil. It's implied that if New returns an error, the other return value shouldn't be used, yet it's good to return nil so any naive attempt to use it would panic instead of behaving unpredicatably. --- windows-agent/internal/cloudinit/cloudinit_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index a57a434b0..bc7d7f1b3 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -36,12 +36,14 @@ func TestNew(t *testing.T) { subcriptionErr: tc.breakWriteAgentData, } - _, err := cloudinit.New(ctx, conf, publicDir) + ci, err := cloudinit.New(ctx, conf, publicDir) if tc.wantErr { require.Error(t, err, "Cloud-init creation should have returned an error") + require.Nil(t, ci, "Cloud-init creation should not have returned a CloudInit object") return } require.NoError(t, err, "Cloud-init creation should have returned no error") + require.NotNil(t, ci, "Cloud-init creation should have returned a CloudInit object") // We don't assert on specifics, as they are tested in WriteAgentData tests. path := filepath.Join(publicDir, ".cloud-init", "agent.yaml") From 42c55e788b95f9f2617619537d4bff861e7bc5e6 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:15:12 -0300 Subject: [PATCH 19/31] Ensures cloudinit.New returns an empty object on failure What that does even mean? That only returns an error if writeAgentData does so. that could be for a number of errors, all serialization-related. If we returned a usable object under such circunstance and registered it to be notified of changes in the agent's config, silent failures would be logged on update, while the agent would stay ill-running. --- windows-agent/internal/cloudinit/cloudinit.go | 2 +- windows-agent/internal/cloudinit/cloudinit_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/windows-agent/internal/cloudinit/cloudinit.go b/windows-agent/internal/cloudinit/cloudinit.go index 6fe01b4fb..b577ad842 100644 --- a/windows-agent/internal/cloudinit/cloudinit.go +++ b/windows-agent/internal/cloudinit/cloudinit.go @@ -38,7 +38,7 @@ func New(ctx context.Context, conf Config, publicDir string) (CloudInit, error) } if err := c.writeAgentData(); err != nil { - return c, err + return CloudInit{}, err } return c, nil diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index bc7d7f1b3..dbed88f48 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -39,11 +39,11 @@ func TestNew(t *testing.T) { ci, err := cloudinit.New(ctx, conf, publicDir) if tc.wantErr { require.Error(t, err, "Cloud-init creation should have returned an error") - require.Nil(t, ci, "Cloud-init creation should not have returned a CloudInit object") + require.Empty(t, ci, "Cloud-init creation should not have returned a CloudInit object") return } require.NoError(t, err, "Cloud-init creation should have returned no error") - require.NotNil(t, ci, "Cloud-init creation should have returned a CloudInit object") + require.NotEmpty(t, ci, "Cloud-init creation should have returned a CloudInit object") // We don't assert on specifics, as they are tested in WriteAgentData tests. path := filepath.Join(publicDir, ".cloud-init", "agent.yaml") From 5cc063f566cacca86d92ebeb08d56803e5d18a57 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:17:34 -0300 Subject: [PATCH 20/31] Adds missing "Setup: " prefix in require assertions. --- windows-agent/internal/cloudinit/cloudinit_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index dbed88f48..2433486a0 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -120,8 +120,8 @@ url = www.example.com/new/rickroll // Test a clean filesystem (New calls WriteAgentData internally) ci, err := cloudinit.New(ctx, conf, publicDir) - require.NoError(t, err, "cloudinit.New should return no error") - require.FileExists(t, path, "New() should have created an agent cloud-init file") + require.NoError(t, err, "Setup: cloudinit.New should return no error") + require.FileExists(t, path, "Setup: New() should have created an agent cloud-init file") // Test overriding the file: New() created the agent.yaml file conf.subcriptionErr = tc.breakSubscription From 3d573f75360a401f736c5787a55c95821fca7d23 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:20:07 -0300 Subject: [PATCH 21/31] Fix function name in TestWriteDistroData's require clauses --- windows-agent/internal/cloudinit/cloudinit_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index 2433486a0..f3a6345ec 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -258,10 +258,10 @@ data: err = ci.WriteDistroData(distroName, input) var want string if tc.wantErr { - require.Error(t, err, "WriteAgentData should have returned an error") + require.Error(t, err, "WriteDistroData should have returned an error") want = oldCloudInit } else { - require.NoError(t, err, "WriteAgentData should return no errors") + require.NoError(t, err, "WriteDistroData should return no errors") want = input } From d637d0da30b4a0bd86ed503f53552d5ed315b21a Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:29:27 -0300 Subject: [PATCH 22/31] Remove the emptyData test case field And replace it by a want string. --- .../internal/cloudinit/cloudinit_test.go | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index f3a6345ec..9a27100ff 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -194,7 +194,6 @@ data: testCases := map[string]struct { // Break marshalling - emptyData bool noOldData bool // Break writing to file @@ -202,15 +201,16 @@ data: breakTempFile bool breakFile bool + want string wantErr bool }{ - "Success": {}, - "With no old data": {noOldData: true}, - "With empty data": {emptyData: true}, + "Success": {}, + "With no old data": {want: newCloudInit, noOldData: true}, + "With new valid data": {want: newCloudInit}, - "Error when the datadir cannot be created": {breakDir: true, wantErr: true}, - "Error when the temp file cannot be written": {breakTempFile: true, wantErr: true}, - "Error when the temp file cannot be renamed": {breakFile: true, wantErr: true}, + "Error when the datadir cannot be created": {breakDir: true, want: oldCloudInit, wantErr: true}, + "Error when the temp file cannot be written": {breakTempFile: true, want: oldCloudInit, wantErr: true}, + "Error when the temp file cannot be renamed": {breakFile: true, want: oldCloudInit, wantErr: true}, } for name, tc := range testCases { @@ -250,19 +250,11 @@ data: require.NoError(t, os.WriteFile(dir, nil, 0600), "Setup: could not create file to mess with cloud-init directory") } - var input string - if !tc.emptyData { - input = newCloudInit - } - - err = ci.WriteDistroData(distroName, input) - var want string + err = ci.WriteDistroData(distroName, tc.want) if tc.wantErr { require.Error(t, err, "WriteDistroData should have returned an error") - want = oldCloudInit } else { require.NoError(t, err, "WriteDistroData should return no errors") - want = input } // Assert that the file was updated (success case) or that the old one remains (error case) @@ -273,7 +265,7 @@ data: got, err := os.ReadFile(path) require.NoError(t, err, "There should be no error reading the distro's cloud-init file") - require.Equal(t, want, string(got), "Agent cloud-init file does not match the golden file") + require.Equal(t, tc.want, string(got), "Agent cloud-init file does not match the golden file") }) } } From c64f2b62e6459eb7793a009f5b079aad089aa491 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:34:50 -0300 Subject: [PATCH 23/31] Rename test case in executor TestInstall --- windows-agent/internal/proservices/landscape/executor_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 13f97aa85..1cc930798 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -162,7 +162,7 @@ func TestInstall(t *testing.T) { "Error when the distroname is empty": {emptyDistroName: true}, "Error when the Appx does not exist": {appxDoesNotExist: true}, "Error when the distro is already installed": {distroAlredyInstalled: true, wantInstalled: true}, - "Error when cloud-init cannot write": {cloudInitWriteErr: true, wantCouldInitWriteCalled: true}, + "Error when cannot write cloud-init file": {cloudInitWriteErr: true, wantCouldInitWriteCalled: true}, "Error when the distro fails to install": {wslInstallErr: true}, } From ad997f90c2e897a032542e26eb9ef7945f4d8460 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:47:17 -0300 Subject: [PATCH 24/31] Avoid pointer to string in the middle of executor's TestInstall --- .../internal/proservices/landscape/executor_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/windows-agent/internal/proservices/landscape/executor_test.go b/windows-agent/internal/proservices/landscape/executor_test.go index 1cc930798..543bbe1be 100644 --- a/windows-agent/internal/proservices/landscape/executor_test.go +++ b/windows-agent/internal/proservices/landscape/executor_test.go @@ -199,14 +199,13 @@ func TestInstall(t *testing.T) { testBed.wslMock.InstallError = true } - var cloudInit *string + var cloudInit string if !tc.noCloudInit { - cloudInit = new(string) - *cloudInit = "Hello, this is a cloud-init file" + cloudInit = "Hello, this is a cloud-init file" } return &landscapeapi.Command{ - Cmd: &landscapeapi.Command_Install_{Install: &landscapeapi.Command_Install{Id: distroName, Cloudinit: cloudInit}}, + Cmd: &landscapeapi.Command_Install_{Install: &landscapeapi.Command_Install{Id: distroName, Cloudinit: &cloudInit}}, } }, // Test assertions From d62b25b1b9e6f4e811ec17fc33453f2745ae7070 Mon Sep 17 00:00:00 2001 From: Carlos Date: Mon, 15 Apr 2024 16:53:05 -0300 Subject: [PATCH 25/31] Skipping all end-to-end test cases that depend on cloud-init All cases, since cloudinit is now the provisioning method. We still run the end-to-end test workflow to assert that all artifacts can be built. --- end-to-end/manual_token_input_test.go | 4 ++++ end-to-end/organization_token_test.go | 4 ++++ end-to-end/purchase_test.go | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/end-to-end/manual_token_input_test.go b/end-to-end/manual_token_input_test.go index e237c6af4..58e06f949 100644 --- a/end-to-end/manual_token_input_test.go +++ b/end-to-end/manual_token_input_test.go @@ -12,6 +12,10 @@ import ( ) func TestManualTokenInput(t *testing.T) { + // TODO: Remove this line when cloud-init support for UP4W is released. + // Follow this PR for more information: https://github.com/canonical/cloud-init/pull/5116 + t.Skip("This test depends on cloud-init support for UP4W being released.") + type whenToken int const ( never whenToken = iota diff --git a/end-to-end/organization_token_test.go b/end-to-end/organization_token_test.go index 433c52001..6f3f8b2f1 100644 --- a/end-to-end/organization_token_test.go +++ b/end-to-end/organization_token_test.go @@ -11,6 +11,10 @@ import ( ) func TestOrganizationProvidedToken(t *testing.T) { + // TODO: Remove this line when cloud-init support for UP4W is released. + // Follow this PR for more information: https://github.com/canonical/cloud-init/pull/5116 + t.Skip("This test depends on cloud-init support for UP4W being released.") + type whenToken int const ( never whenToken = iota diff --git a/end-to-end/purchase_test.go b/end-to-end/purchase_test.go index f01c1b917..6932493a4 100644 --- a/end-to-end/purchase_test.go +++ b/end-to-end/purchase_test.go @@ -23,6 +23,10 @@ const ( ) func TestPurchase(t *testing.T) { + // TODO: Remove this line when cloud-init support for UP4W is released. + // Follow this PR for more information: https://github.com/canonical/cloud-init/pull/5116 + t.Skip("This test depends on cloud-init support for UP4W being released.") + type whenToken int const ( never whenToken = iota From 924ea420786b85f21040b1c690d2f41f88d8ba31 Mon Sep 17 00:00:00 2001 From: Carlos Date: Tue, 16 Apr 2024 16:02:59 -0300 Subject: [PATCH 26/31] Cloudinit's TestNew to check if returned is not nil On the happy path. --- windows-agent/internal/cloudinit/cloudinit_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/windows-agent/internal/cloudinit/cloudinit_test.go b/windows-agent/internal/cloudinit/cloudinit_test.go index 9a27100ff..b63cfaef1 100644 --- a/windows-agent/internal/cloudinit/cloudinit_test.go +++ b/windows-agent/internal/cloudinit/cloudinit_test.go @@ -39,7 +39,6 @@ func TestNew(t *testing.T) { ci, err := cloudinit.New(ctx, conf, publicDir) if tc.wantErr { require.Error(t, err, "Cloud-init creation should have returned an error") - require.Empty(t, ci, "Cloud-init creation should not have returned a CloudInit object") return } require.NoError(t, err, "Cloud-init creation should have returned no error") From 097109df68ae47793f3b893b98d1d74be866c8be Mon Sep 17 00:00:00 2001 From: Carlos Date: Tue, 16 Apr 2024 16:11:42 -0300 Subject: [PATCH 27/31] Adds missing test case in distro_tesrt :) --- windows-agent/internal/distros/distro/distro_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/windows-agent/internal/distros/distro/distro_test.go b/windows-agent/internal/distros/distro/distro_test.go index d62ad7909..820bec014 100644 --- a/windows-agent/internal/distros/distro/distro_test.go +++ b/windows-agent/internal/distros/distro/distro_test.go @@ -84,6 +84,7 @@ func TestNew(t *testing.T) { "Error when the distro is not registered, but the GUID is": {distro: nonRegisteredDistro, withGUID: registeredGUID, wantErr: true, wantErrType: &distro.NotValidError{}}, "Error when neither the distro nor the GUID are registered": {distro: nonRegisteredDistro, withGUID: fakeGUID, wantErr: true, wantErrType: &distro.NotValidError{}}, "Error when the startup mutex is nil": {distro: registeredDistro, nilMutex: true, wantErr: true}, + "Error when the distro working directory cannot be created": {distro: nonRegisteredDistro, preventWorkDirCreation: true, wantErr: true, wantErrType: &distro.NotValidError{}}, } for name, tc := range testCases { From b920c2dff0391f07736577ce9cf408f37f6b0f02 Mon Sep 17 00:00:00 2001 From: Carlos Date: Tue, 16 Apr 2024 16:50:33 -0300 Subject: [PATCH 28/31] Proservices TestNew to exercise the notification closures We force both closures to run by writing into the registry mock. That mock defers notifying the registry watcher of the changes. The information finally hits cloud-init and we assert on the produced agent.yaml against the goldens herein introduced. --- .../internal/proservices/proservices_test.go | 21 ++++++++++++++----- ...the_config_cannot_check_if_it_is_read-only | 3 +++ .../golden/when_the_subscription_stays_empty | 3 +++ 3 files changed, 22 insertions(+), 5 deletions(-) create mode 100644 windows-agent/internal/proservices/testdata/TestNew/golden/when_the_config_cannot_check_if_it_is_read-only create mode 100644 windows-agent/internal/proservices/testdata/TestNew/golden/when_the_subscription_stays_empty diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index 85dd08db6..5f8b67036 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -12,6 +12,7 @@ import ( agentapi "github.com/canonical/ubuntu-pro-for-wsl/agentapi/go" "github.com/canonical/ubuntu-pro-for-wsl/common" + "github.com/canonical/ubuntu-pro-for-wsl/common/testutils" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/consts" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/proservices" "github.com/canonical/ubuntu-pro-for-wsl/windows-agent/internal/proservices/registrywatcher/registry" @@ -61,7 +62,7 @@ func TestNew(t *testing.T) { reg := registry.NewMock() k, err := reg.HKCUCreateKey("Software/Canonical/UbuntuPro") require.NoError(t, err, "Setup: could not create Ubuntu Pro registry key") - reg.CloseKey(k) + defer reg.CloseKey(k) if tc.breakNewDistroDB { dbFile := filepath.Join(privateDir, consts.DatabaseFileName) @@ -82,15 +83,25 @@ func TestNew(t *testing.T) { } s, err := proservices.New(ctx, publicDir, privateDir, proservices.WithRegistry(reg)) - if err == nil { - defer s.Stop(ctx) - } - if tc.wantErr { require.Error(t, err, "New should return an error") return } require.NoError(t, err, "New should return no error") + defer s.Stop(ctx) + + // defer is needed here because the registry mock will defer notifying the registry watcher after the following writes. + defer func() { + got, err := os.ReadFile(filepath.Join(publicDir, ".cloud-init", "agent.yaml")) + require.NoError(t, err, "Setup: could not read agent.yaml file post test completion") + want := testutils.LoadWithUpdateFromGolden(t, string(got)) + require.Equal(t, want, string(got), "agent.yaml file should be the same as the golden file") + }() + err = reg.WriteValue(k, "LandscapeConfig", "[client]\nuser=JohnDoe", true) + require.NoError(t, err, "Setup: could not write LandscapeConfig to the registry mock") + err = reg.WriteValue(k, "UbuntuProToken", "test-token", false) + require.NoError(t, err, "Setup: could not write UbuntuProToken to the registry mock") + // registry notifications are triggered after this point and if proservices.New() made the right connections thye will cause cloudinit to update the agent.yaml file. }) } } diff --git a/windows-agent/internal/proservices/testdata/TestNew/golden/when_the_config_cannot_check_if_it_is_read-only b/windows-agent/internal/proservices/testdata/TestNew/golden/when_the_config_cannot_check_if_it_is_read-only new file mode 100644 index 000000000..ba33d201a --- /dev/null +++ b/windows-agent/internal/proservices/testdata/TestNew/golden/when_the_config_cannot_check_if_it_is_read-only @@ -0,0 +1,3 @@ +# cloud-init +# This file was generated automatically and must not be edited +{} diff --git a/windows-agent/internal/proservices/testdata/TestNew/golden/when_the_subscription_stays_empty b/windows-agent/internal/proservices/testdata/TestNew/golden/when_the_subscription_stays_empty new file mode 100644 index 000000000..ba33d201a --- /dev/null +++ b/windows-agent/internal/proservices/testdata/TestNew/golden/when_the_subscription_stays_empty @@ -0,0 +1,3 @@ +# cloud-init +# This file was generated automatically and must not be edited +{} From a4fb4f6bd8baae107d10e4d0bb1b7a98696d6adb Mon Sep 17 00:00:00 2001 From: Carlos Date: Wed, 17 Apr 2024 11:42:35 -0300 Subject: [PATCH 29/31] Stop proservices before require clauses in test --- windows-agent/internal/proservices/proservices_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index 5f8b67036..fc81e47a4 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -83,6 +83,7 @@ func TestNew(t *testing.T) { } s, err := proservices.New(ctx, publicDir, privateDir, proservices.WithRegistry(reg)) + defer s.Stop(ctx) if tc.wantErr { require.Error(t, err, "New should return an error") return From 0cb6f356f595c037915cbd2a2f17d24141edbdc9 Mon Sep 17 00:00:00 2001 From: Carlos Date: Wed, 17 Apr 2024 11:44:01 -0300 Subject: [PATCH 30/31] Remove the golden assertions out of the defer clause Still need to find a way other than time-based programming to give cloudinit time to write the agent.yaml --- .../internal/proservices/proservices_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index fc81e47a4..27f808027 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -89,20 +89,17 @@ func TestNew(t *testing.T) { return } require.NoError(t, err, "New should return no error") - defer s.Stop(ctx) - // defer is needed here because the registry mock will defer notifying the registry watcher after the following writes. - defer func() { - got, err := os.ReadFile(filepath.Join(publicDir, ".cloud-init", "agent.yaml")) - require.NoError(t, err, "Setup: could not read agent.yaml file post test completion") - want := testutils.LoadWithUpdateFromGolden(t, string(got)) - require.Equal(t, want, string(got), "agent.yaml file should be the same as the golden file") - }() + // Those lines are just to trigger the registry watcher callbacks, they don't write anything to the agent.yaml file by the time we check, that's why goldens are empty. err = reg.WriteValue(k, "LandscapeConfig", "[client]\nuser=JohnDoe", true) require.NoError(t, err, "Setup: could not write LandscapeConfig to the registry mock") err = reg.WriteValue(k, "UbuntuProToken", "test-token", false) require.NoError(t, err, "Setup: could not write UbuntuProToken to the registry mock") - // registry notifications are triggered after this point and if proservices.New() made the right connections thye will cause cloudinit to update the agent.yaml file. + + got, err := os.ReadFile(filepath.Join(publicDir, ".cloud-init", "agent.yaml")) + require.NoError(t, err, "Setup: could not read agent.yaml file post test completion") + want := testutils.LoadWithUpdateFromGolden(t, string(got)) + require.Equal(t, want, string(got), "agent.yaml file should be the same as the golden file") }) } } From d3426c8379c9f31c585279987ca8b8d1791fdf84 Mon Sep 17 00:00:00 2001 From: Carlos Date: Fri, 19 Apr 2024 11:32:50 -0300 Subject: [PATCH 31/31] [NEW!!!] Only stop proservices if err is nil It'd be safe today to do so, but might not be tomorrow. The good practice is always to not touch the returned object if err is not nil. --- windows-agent/internal/proservices/proservices_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index 27f808027..051433cd6 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -83,7 +83,9 @@ func TestNew(t *testing.T) { } s, err := proservices.New(ctx, publicDir, privateDir, proservices.WithRegistry(reg)) - defer s.Stop(ctx) + if err == nil { + defer s.Stop(ctx) + } if tc.wantErr { require.Error(t, err, "New should return an error") return