From 403a1534949a969c3d2886cb03642f12fabde859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 24 Oct 2023 15:40:33 +0200 Subject: [PATCH 1/8] Refactor proService's updateSubscriptions in anticipation of Landscape --- .../internal/proservices/proservices.go | 137 ++++++++++++------ 1 file changed, 92 insertions(+), 45 deletions(-) diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index c9b2ec12b..a457cc409 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -14,6 +14,7 @@ import ( "github.com/canonical/ubuntu-pro-for-windows/common" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/database" + "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/task" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/grpc/interceptorschain" log "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/grpc/logstreamer" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/proservices/landscape" @@ -102,7 +103,7 @@ func New(ctx context.Context, args ...Option) (s Manager, err error) { }() go func() { - err := updateSubscriptions(ctx, opts.cacheDir, conf, db) + err := updateRegistrySettings(ctx, opts.cacheDir, conf, db) if err != nil { log.Warningf(ctx, "Could not update subscriptions: %v", err) } @@ -154,71 +155,117 @@ func (m Manager) RegisterGRPCServices(ctx context.Context) *grpc.Server { return grpcServer } -// updateSubscriptions checks if the subscription has changed since the last time it was called. If so, the new subscription -// is pushed to all distros as a deferred task. -func updateSubscriptions(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) error { - proToken, _, err := conf.Subscription(ctx) - if err != nil { - return fmt.Errorf("could not retrieve current subscription: %v", err) - } +// updateRegistrySettings checks if any of the registry settings have changed since this function was last called. +// If so, updated settings are pushed to the distros. +func updateRegistrySettings(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) error { + type getTask = func(context.Context, string, *config.Config, *database.DistroDB) (task.Task, error) - if !subscriptionIsNew(ctx, cacheDir, proToken) { - return nil + // Collect tasks for updated settings + var acc error + var taskList []task.Task + for _, f := range []getTask{getNewSubscription} { + task, err := f(ctx, cacheDir, conf, db) + if err != nil { + errors.Join(acc, err) + continue + } + if task != nil { + taskList = append(taskList, task) + } } - task := tasks.ProAttachment{Token: proToken} + if acc != nil { + log.Warningf(ctx, "Could not obtain some updated registry settings: %v", acc) + } + // Apply tasks for updated settings + acc = nil for _, d := range db.GetAll() { - err = errors.Join(err, d.SubmitDeferredTasks(task)) + acc = errors.Join(acc, d.SubmitDeferredTasks(taskList...)) } - if err != nil { - return fmt.Errorf("could not submit new token to certain distros: %v", err) + if acc != nil { + return fmt.Errorf("could not submit new token to certain distros: %v", acc) } return nil } -// subscriptionIsNew detects if the current subscription is different from the last time it was called. -func subscriptionIsNew(ctx context.Context, cacheDir string, newSubscription string) (new bool) { - cachePath := filepath.Join(cacheDir, "subscription.csum") - newCheckSum := sha512.Sum512([]byte(newSubscription)) +// getNewSubscription checks if the subscription has changed since the last time it was called. If so, the new subscription +// is returned in the form of a task. +func getNewSubscription(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) (task.Task, error) { + proToken, _, err := conf.Subscription(ctx) + if err != nil { + return nil, fmt.Errorf("could not retrieve current subscription: %v", err) + } - // Update cache on exit - defer func() { - if newSubscription == "" { - // If there is no subscription, we remove the file. - // This preserves this function's idempotency. - err := os.Remove(cachePath) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - log.Warningf(ctx, "Could not write new subscription to cache: %v", err) - } - return - } + isNew, err := valueIsNew(filepath.Join(cacheDir, "subscription.csum"), []byte(proToken)) + if err != nil { + log.Warningf(ctx, "could not update checksum for Ubuntu Pro subscription: %v", err) + } + if isNew { + return nil, nil + } - if !new { - return - } + log.Debug(ctx, "New Ubuntu Pro subscription settings detected in registry") + return tasks.ProAttachment{Token: proToken}, nil +} - err := os.WriteFile(cachePath, newCheckSum[:], 0600) - if err != nil { - log.Warningf(ctx, "Could not write new subscription to cache: %v", err) - } - }() +// valueIsNew detects if the current value is different from the last time it was used. +// The return value is usable even if error is returned. +func valueIsNew(cachePath string, newValue []byte) (new bool, err error) { + var newCheckSum []byte + if len(newValue) != 0 { + tmp := sha512.Sum512(newValue) + newCheckSum = tmp[:] + } + + defer decorateUpdateCache(&new, &err, cachePath, newCheckSum) oldChecksum, err := os.ReadFile(cachePath) if errors.Is(err, fs.ErrNotExist) { - // File not found: there was no subscription before - // (Lack of) subscription is new only if the new subscription non-empty. - return newSubscription != "" + // File not found: there was no value before + oldChecksum = nil } else if err != nil { - log.Warningf(ctx, "Could not read old subscription, assuming subscription is new. Error: %v", err) - return true + return true, fmt.Errorf("could not read old value: %v", err) } - if slices.Equal(oldChecksum, newCheckSum[:]) { - return false + if slices.Equal(oldChecksum, newCheckSum) { + return false, nil } - return true + return true, nil +} + +// decorateUpdateCache acts depending on caller's return values (hence decorate). +// It stores the new checksum to the cachefile. Any errors are joined to *err. +func decorateUpdateCache(new *bool, err *error, cachePath string, newCheckSum []byte) { + writeCacheErr := func() error { + // If the value is empty, we remove the file. + // This preserves this function's idempotency. + if len(newCheckSum) == 0 { + err := os.Remove(cachePath) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("could not remove old checksum: %v", err) + } + return nil + } + + // Value is unchanged: don't write to file + if !*new { + return nil + } + + // Update to file + if err := os.WriteFile(cachePath, newCheckSum[:], 0600); err != nil { + return fmt.Errorf("could not write checksum to cache: %v", err) + } + + return nil + }() + + *err = errors.Join(*err, writeCacheErr) } From da117c856c09c000cc085d6b800ba6de28f42c6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 24 Oct 2023 15:41:24 +0200 Subject: [PATCH 2/8] Add Landscape to the list of checksumed updated to check --- .../internal/proservices/proservices.go | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index a457cc409..055e98872 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -163,7 +163,7 @@ func updateRegistrySettings(ctx context.Context, cacheDir string, conf *config.C // Collect tasks for updated settings var acc error var taskList []task.Task - for _, f := range []getTask{getNewSubscription} { + for _, f := range []getTask{getNewSubscription, getNewLandscape} { task, err := f(ctx, cacheDir, conf, db) if err != nil { errors.Join(acc, err) @@ -211,6 +211,42 @@ func getNewSubscription(ctx context.Context, cacheDir string, conf *config.Confi return tasks.ProAttachment{Token: proToken}, nil } +// getNewLandscape checks if the Landscape settings has changed since the last time it was called. If so, the +// new Landscape settings are returned in the form of a task. +func getNewLandscape(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) (task.Task, error) { + landscapeConf, err := conf.LandscapeClientConfig(ctx) + if err != nil { + return nil, fmt.Errorf("could not retrieve current landscape config: %v", err) + } + + landscapeUID, err := conf.LandscapeAgentUID(ctx) + if err != nil { + return nil, fmt.Errorf("could not retrieve current landscape UID: %v", err) + } + + // We append them just so we can compute a combined checksum + serialized := fmt.Sprintf("%s\n%s", landscapeUID, landscapeConf) + + isNew, err := valueIsNew(filepath.Join(cacheDir, "landscape.csum"), []byte(serialized)) + if err != nil { + log.Warningf(ctx, "could not update checksum for Landscape configuration: %v", err) + } + + if isNew { + return nil, nil + } + + log.Debug(ctx, "New Landscape settings detected in registry") + + // We must not register to landscape if we have no Landscape UID + if landscapeConf != "" && landscapeUID == "" { + log.Debug(ctx, "Ignoring new landscape settings: no Landscape agent UID") + return nil, nil + } + + return tasks.LandscapeConfigure{Config: landscapeConf, HostagentUID: landscapeUID}, nil +} + // valueIsNew detects if the current value is different from the last time it was used. // The return value is usable even if error is returned. func valueIsNew(cachePath string, newValue []byte) (new bool, err error) { From 59edb602f86ea053f5ab542273c6d8f7d5ecc3c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 24 Oct 2023 15:52:24 +0200 Subject: [PATCH 3/8] Move UpdateRegistrySettings to config --- windows-agent/internal/config/config.go | 157 +++++++++++++++++ .../internal/proservices/proservices.go | 159 +----------------- 2 files changed, 158 insertions(+), 158 deletions(-) diff --git a/windows-agent/internal/config/config.go b/windows-agent/internal/config/config.go index e76a72d7c..ec4568dda 100644 --- a/windows-agent/internal/config/config.go +++ b/windows-agent/internal/config/config.go @@ -4,16 +4,22 @@ package config import ( "context" + "crypto/sha512" "errors" "fmt" + "io/fs" + "os" + "path/filepath" "sync" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config/registry" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/contracts" + "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/database" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/task" log "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/grpc/logstreamer" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/tasks" "github.com/ubuntu/decorate" + "golang.org/x/exp/slices" ) const ( @@ -415,3 +421,154 @@ func (c *Config) FetchMicrosoftStoreSubscription(ctx context.Context) (err error return nil } + +// UpdateRegistrySettings checks if any of the registry settings have changed since this function was last called. +// If so, new settings are pushed to the distros. +func (c *Config) UpdateRegistrySettings(ctx context.Context, cacheDir string, db *database.DistroDB) error { + type getTask = func(*Config, context.Context, string, *database.DistroDB) (task.Task, error) + + // Collect tasks for updated settings + var acc error + var taskList []task.Task + for _, f := range []getTask{(*Config).getTaskOnNewSubscription, (*Config).getTaskOnNewLandscape} { + task, err := f(c, ctx, cacheDir, db) + if err != nil { + errors.Join(acc, err) + continue + } + if task != nil { + taskList = append(taskList, task) + } + } + + if acc != nil { + log.Warningf(ctx, "Could not obtain some updated registry settings: %v", acc) + } + + // Apply tasks for updated settings + acc = nil + for _, d := range db.GetAll() { + acc = errors.Join(acc, d.SubmitDeferredTasks(taskList...)) + } + + if acc != nil { + return fmt.Errorf("could not submit new token to certain distros: %v", acc) + } + + return nil +} + +// getTaskOnNewSubscription checks if the subscription has changed since the last time it was called. If so, the new subscription +// is returned in the form of a task. +func (c *Config) getTaskOnNewSubscription(ctx context.Context, cacheDir string, db *database.DistroDB) (task.Task, error) { + proToken, _, err := c.Subscription(ctx) + if err != nil { + return nil, fmt.Errorf("could not retrieve current subscription: %v", err) + } + + isNew, err := valueIsNew(filepath.Join(cacheDir, "subscription.csum"), []byte(proToken)) + if err != nil { + log.Warningf(ctx, "could not update checksum for Ubuntu Pro subscription: %v", err) + } + if isNew { + return nil, nil + } + + log.Debug(ctx, "New Ubuntu Pro subscription settings detected in registry") + return tasks.ProAttachment{Token: proToken}, nil +} + +// getTaskOnNewLandscape checks if the Landscape settings has changed since the last time it was called. If so, the +// new Landscape settings are returned in the form of a task. +func (c *Config) getTaskOnNewLandscape(ctx context.Context, cacheDir string, db *database.DistroDB) (task.Task, error) { + landscapeConf, err := c.LandscapeClientConfig(ctx) + if err != nil { + return nil, fmt.Errorf("could not retrieve current landscape config: %v", err) + } + + landscapeUID, err := c.LandscapeAgentUID(ctx) + if err != nil { + return nil, fmt.Errorf("could not retrieve current landscape UID: %v", err) + } + + // We append them just so we can compute a combined checksum + serialized := fmt.Sprintf("%s\n%s", landscapeUID, landscapeConf) + + isNew, err := valueIsNew(filepath.Join(cacheDir, "landscape.csum"), []byte(serialized)) + if err != nil { + log.Warningf(ctx, "could not update checksum for Landscape configuration: %v", err) + } + + if isNew { + return nil, nil + } + + log.Debug(ctx, "New Landscape settings detected in registry") + + // We must not register to landscape if we have no Landscape UID + if landscapeConf != "" && landscapeUID == "" { + log.Debug(ctx, "Ignoring new landscape settings: no Landscape agent UID") + return nil, nil + } + + return tasks.LandscapeConfigure{Config: landscapeConf, HostagentUID: landscapeUID}, nil +} + +// valueIsNew detects if the current value is different from the last time it was used. +// The return value is usable even if error is returned. +func valueIsNew(cachePath string, newValue []byte) (new bool, err error) { + var newCheckSum []byte + if len(newValue) != 0 { + tmp := sha512.Sum512(newValue) + newCheckSum = tmp[:] + } + + defer decorateUpdateCache(&new, &err, cachePath, newCheckSum) + + oldChecksum, err := os.ReadFile(cachePath) + if errors.Is(err, fs.ErrNotExist) { + // File not found: there was no value before + oldChecksum = nil + } else if err != nil { + return true, fmt.Errorf("could not read old value: %v", err) + } + + if slices.Equal(oldChecksum, newCheckSum) { + return false, nil + } + + return true, nil +} + +// decorateUpdateCache acts depending on caller's return values (hence decorate). +// It stores the new checksum to the cachefile. Any errors are joined to *err. +func decorateUpdateCache(new *bool, err *error, cachePath string, newCheckSum []byte) { + writeCacheErr := func() error { + // If the value is empty, we remove the file. + // This preserves this function's idempotency. + if len(newCheckSum) == 0 { + err := os.Remove(cachePath) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("could not remove old checksum: %v", err) + } + return nil + } + + // Value is unchanged: don't write to file + if !*new { + return nil + } + + // Update to file + if err := os.WriteFile(cachePath, newCheckSum[:], 0600); err != nil { + return fmt.Errorf("could not write checksum to cache: %v", err) + } + + return nil + }() + + *err = errors.Join(*err, writeCacheErr) +} diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index 055e98872..b680e453f 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -3,10 +3,7 @@ package proservices import ( "context" - "crypto/sha512" "errors" - "fmt" - "io/fs" "os" "path/filepath" @@ -14,14 +11,11 @@ import ( "github.com/canonical/ubuntu-pro-for-windows/common" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/database" - "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/task" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/grpc/interceptorschain" log "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/grpc/logstreamer" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/proservices/landscape" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/proservices/ui" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/proservices/wslinstance" - "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/tasks" - "golang.org/x/exp/slices" "google.golang.org/grpc" ) @@ -103,7 +97,7 @@ func New(ctx context.Context, args ...Option) (s Manager, err error) { }() go func() { - err := updateRegistrySettings(ctx, opts.cacheDir, conf, db) + err := conf.UpdateRegistrySettings(ctx, opts.cacheDir, db) if err != nil { log.Warningf(ctx, "Could not update subscriptions: %v", err) } @@ -154,154 +148,3 @@ func (m Manager) RegisterGRPCServices(ctx context.Context) *grpc.Server { return grpcServer } - -// updateRegistrySettings checks if any of the registry settings have changed since this function was last called. -// If so, updated settings are pushed to the distros. -func updateRegistrySettings(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) error { - type getTask = func(context.Context, string, *config.Config, *database.DistroDB) (task.Task, error) - - // Collect tasks for updated settings - var acc error - var taskList []task.Task - for _, f := range []getTask{getNewSubscription, getNewLandscape} { - task, err := f(ctx, cacheDir, conf, db) - if err != nil { - errors.Join(acc, err) - continue - } - if task != nil { - taskList = append(taskList, task) - } - } - - if acc != nil { - log.Warningf(ctx, "Could not obtain some updated registry settings: %v", acc) - } - - // Apply tasks for updated settings - acc = nil - for _, d := range db.GetAll() { - acc = errors.Join(acc, d.SubmitDeferredTasks(taskList...)) - } - - if acc != nil { - return fmt.Errorf("could not submit new token to certain distros: %v", acc) - } - - return nil -} - -// getNewSubscription checks if the subscription has changed since the last time it was called. If so, the new subscription -// is returned in the form of a task. -func getNewSubscription(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) (task.Task, error) { - proToken, _, err := conf.Subscription(ctx) - if err != nil { - return nil, fmt.Errorf("could not retrieve current subscription: %v", err) - } - - isNew, err := valueIsNew(filepath.Join(cacheDir, "subscription.csum"), []byte(proToken)) - if err != nil { - log.Warningf(ctx, "could not update checksum for Ubuntu Pro subscription: %v", err) - } - if isNew { - return nil, nil - } - - log.Debug(ctx, "New Ubuntu Pro subscription settings detected in registry") - return tasks.ProAttachment{Token: proToken}, nil -} - -// getNewLandscape checks if the Landscape settings has changed since the last time it was called. If so, the -// new Landscape settings are returned in the form of a task. -func getNewLandscape(ctx context.Context, cacheDir string, conf *config.Config, db *database.DistroDB) (task.Task, error) { - landscapeConf, err := conf.LandscapeClientConfig(ctx) - if err != nil { - return nil, fmt.Errorf("could not retrieve current landscape config: %v", err) - } - - landscapeUID, err := conf.LandscapeAgentUID(ctx) - if err != nil { - return nil, fmt.Errorf("could not retrieve current landscape UID: %v", err) - } - - // We append them just so we can compute a combined checksum - serialized := fmt.Sprintf("%s\n%s", landscapeUID, landscapeConf) - - isNew, err := valueIsNew(filepath.Join(cacheDir, "landscape.csum"), []byte(serialized)) - if err != nil { - log.Warningf(ctx, "could not update checksum for Landscape configuration: %v", err) - } - - if isNew { - return nil, nil - } - - log.Debug(ctx, "New Landscape settings detected in registry") - - // We must not register to landscape if we have no Landscape UID - if landscapeConf != "" && landscapeUID == "" { - log.Debug(ctx, "Ignoring new landscape settings: no Landscape agent UID") - return nil, nil - } - - return tasks.LandscapeConfigure{Config: landscapeConf, HostagentUID: landscapeUID}, nil -} - -// valueIsNew detects if the current value is different from the last time it was used. -// The return value is usable even if error is returned. -func valueIsNew(cachePath string, newValue []byte) (new bool, err error) { - var newCheckSum []byte - if len(newValue) != 0 { - tmp := sha512.Sum512(newValue) - newCheckSum = tmp[:] - } - - defer decorateUpdateCache(&new, &err, cachePath, newCheckSum) - - oldChecksum, err := os.ReadFile(cachePath) - if errors.Is(err, fs.ErrNotExist) { - // File not found: there was no value before - oldChecksum = nil - } else if err != nil { - return true, fmt.Errorf("could not read old value: %v", err) - } - - if slices.Equal(oldChecksum, newCheckSum) { - return false, nil - } - - return true, nil -} - -// decorateUpdateCache acts depending on caller's return values (hence decorate). -// It stores the new checksum to the cachefile. Any errors are joined to *err. -func decorateUpdateCache(new *bool, err *error, cachePath string, newCheckSum []byte) { - writeCacheErr := func() error { - // If the value is empty, we remove the file. - // This preserves this function's idempotency. - if len(newCheckSum) == 0 { - err := os.Remove(cachePath) - if errors.Is(err, fs.ErrNotExist) { - return nil - } - if err != nil { - return fmt.Errorf("could not remove old checksum: %v", err) - } - return nil - } - - // Value is unchanged: don't write to file - if !*new { - return nil - } - - // Update to file - if err := os.WriteFile(cachePath, newCheckSum[:], 0600); err != nil { - return fmt.Errorf("could not write checksum to cache: %v", err) - } - - return nil - }() - - *err = errors.Join(*err, writeCacheErr) -} From 1ed57cd42b07b26b2cf4829fbdeec860d91ea2b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 24 Oct 2023 16:19:16 +0200 Subject: [PATCH 4/8] Fix issues in UpdateRegistrySettings This should be a fixup to the commit before the previous one, but with the migration from proServices to Config, it becomes too messy. Fixes: - Rename acc -> errs - Rename valueIsNew to hasChanged - Fix typo in config - Remove endline when serializing Landscape We treat 'empty string' as a special value. With that \n there, the string would never be empty! --- windows-agent/internal/config/config.go | 31 +++++++++++++------------ 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/windows-agent/internal/config/config.go b/windows-agent/internal/config/config.go index ec4568dda..61eab7e45 100644 --- a/windows-agent/internal/config/config.go +++ b/windows-agent/internal/config/config.go @@ -428,12 +428,12 @@ func (c *Config) UpdateRegistrySettings(ctx context.Context, cacheDir string, db type getTask = func(*Config, context.Context, string, *database.DistroDB) (task.Task, error) // Collect tasks for updated settings - var acc error + var errs error var taskList []task.Task for _, f := range []getTask{(*Config).getTaskOnNewSubscription, (*Config).getTaskOnNewLandscape} { task, err := f(c, ctx, cacheDir, db) if err != nil { - errors.Join(acc, err) + errs = errors.Join(errs, err) continue } if task != nil { @@ -441,18 +441,18 @@ func (c *Config) UpdateRegistrySettings(ctx context.Context, cacheDir string, db } } - if acc != nil { - log.Warningf(ctx, "Could not obtain some updated registry settings: %v", acc) + if errs != nil { + log.Warningf(ctx, "Could not obtain some updated registry settings: %v", errs) } // Apply tasks for updated settings - acc = nil + errs = nil for _, d := range db.GetAll() { - acc = errors.Join(acc, d.SubmitDeferredTasks(taskList...)) + errs = errors.Join(errs, d.SubmitDeferredTasks(taskList...)) } - if acc != nil { - return fmt.Errorf("could not submit new token to certain distros: %v", acc) + if errs != nil { + return fmt.Errorf("could not submit new task to certain distros: %v", errs) } return nil @@ -466,11 +466,12 @@ func (c *Config) getTaskOnNewSubscription(ctx context.Context, cacheDir string, return nil, fmt.Errorf("could not retrieve current subscription: %v", err) } - isNew, err := valueIsNew(filepath.Join(cacheDir, "subscription.csum"), []byte(proToken)) + isNew, err := hasChanged(filepath.Join(cacheDir, "subscription.csum"), []byte(proToken)) if err != nil { log.Warningf(ctx, "could not update checksum for Ubuntu Pro subscription: %v", err) } - if isNew { + + if !isNew { return nil, nil } @@ -492,14 +493,14 @@ func (c *Config) getTaskOnNewLandscape(ctx context.Context, cacheDir string, db } // We append them just so we can compute a combined checksum - serialized := fmt.Sprintf("%s\n%s", landscapeUID, landscapeConf) + serialized := fmt.Sprintf("%s%s", landscapeUID, landscapeConf) - isNew, err := valueIsNew(filepath.Join(cacheDir, "landscape.csum"), []byte(serialized)) + isNew, err := hasChanged(filepath.Join(cacheDir, "landscape.csum"), []byte(serialized)) if err != nil { log.Warningf(ctx, "could not update checksum for Landscape configuration: %v", err) } - if isNew { + if !isNew { return nil, nil } @@ -514,9 +515,9 @@ func (c *Config) getTaskOnNewLandscape(ctx context.Context, cacheDir string, db return tasks.LandscapeConfigure{Config: landscapeConf, HostagentUID: landscapeUID}, nil } -// valueIsNew detects if the current value is different from the last time it was used. +// hasChanged detects if the current value is different from the last time it was used. // The return value is usable even if error is returned. -func valueIsNew(cachePath string, newValue []byte) (new bool, err error) { +func hasChanged(cachePath string, newValue []byte) (new bool, err error) { var newCheckSum []byte if len(newValue) != 0 { tmp := sha512.Sum512(newValue) From 8aca498b2ec7b5f5b4e2616fa77d9d84a6a08c15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 24 Oct 2023 15:56:55 +0200 Subject: [PATCH 5/8] Simplify proservices.go Say goodbye to a weird deferred function and an unnecessary goroutine --- windows-agent/internal/proservices/proservices.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/windows-agent/internal/proservices/proservices.go b/windows-agent/internal/proservices/proservices.go index b680e453f..ec79c3ec6 100644 --- a/windows-agent/internal/proservices/proservices.go +++ b/windows-agent/internal/proservices/proservices.go @@ -90,18 +90,11 @@ func New(ctx context.Context, args ...Option) (s Manager, err error) { if err != nil { return s, err } - defer func() { - if err != nil { - db.Close(ctx) - } - }() + defer db.Close(ctx) - go func() { - err := conf.UpdateRegistrySettings(ctx, opts.cacheDir, db) - if err != nil { - log.Warningf(ctx, "Could not update subscriptions: %v", err) - } - }() + if err := conf.UpdateRegistrySettings(ctx, opts.cacheDir, db); err != nil { + log.Warningf(ctx, "Could not update registry settings: %v", err) + } uiService := ui.New(ctx, conf, db) From 3269c68e28859d8656714f0db80b465cb5ae5c2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Thu, 26 Oct 2023 10:50:10 +0200 Subject: [PATCH 6/8] Remove checksum tests from proServices --- .../internal/proservices/proservices_test.go | 49 ------------------- 1 file changed, 49 deletions(-) diff --git a/windows-agent/internal/proservices/proservices_test.go b/windows-agent/internal/proservices/proservices_test.go index a0c0b52c2..235c195a4 100644 --- a/windows-agent/internal/proservices/proservices_test.go +++ b/windows-agent/internal/proservices/proservices_test.go @@ -2,12 +2,9 @@ package proservices_test import ( "context" - "crypto/sha512" - "io/fs" "os" "path/filepath" "testing" - "time" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config/registry" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/consts" @@ -26,13 +23,6 @@ func TestMain(m *testing.M) { func TestNew(t *testing.T) { t.Parallel() - type subscription = string - const ( - tokenA = "Token123" - tokenB = "TokenXYZ" - tokenNone = "" - ) - testCases := map[string]struct { configIsReadOnly bool @@ -40,24 +30,12 @@ func TestNew(t *testing.T) { breakMkDir bool breakNewDistroDB bool - newSubscription subscription - oldSubscription subscription - checksumIsEmpty bool - wantErr bool }{ "Success when the subscription stays empty": {}, "Success with a read-only registry": {configIsReadOnly: true}, "Success when the config cannot check if it is read-only": {breakConfig: true}, - "Success when the subscription stays the same": {oldSubscription: tokenA, newSubscription: tokenA}, - "Success when the subscription changes tokens": {oldSubscription: tokenA, newSubscription: tokenB}, - "Success when the subscription changes from empty": {oldSubscription: tokenNone, newSubscription: tokenA}, - "Success when the subscription changes to empty": {oldSubscription: tokenA, newSubscription: tokenNone}, - - "Success when the subscription was an empty file (subscribed)": {checksumIsEmpty: true, newSubscription: tokenA}, - "Success when the subscription was an empty file (non-subscribed)": {checksumIsEmpty: true, newSubscription: tokenNone}, - "Error when Manager cannot create its cache dir": {breakMkDir: true, wantErr: true}, "Error when database cannot create its dump file": {breakNewDistroDB: true, wantErr: true}, } @@ -69,22 +47,8 @@ func TestNew(t *testing.T) { ctx := context.Background() dir := t.TempDir() - subscriptionChecksumFile := filepath.Join(dir, "subscription.csum") - if tc.checksumIsEmpty { - err := os.WriteFile(subscriptionChecksumFile, []byte{}, 0600) - require.NoError(t, err, "Setup: could not write empty checksum file") - } else if tc.oldSubscription != "" { - oldChecksum := sha512.Sum512([]byte(tc.oldSubscription)) - err := os.WriteFile(subscriptionChecksumFile, oldChecksum[:], 0600) - require.NoError(t, err, "Setup: could not write subscription checksum file") - } - reg := registry.NewMock() reg.KeyExists = true - reg.UbuntuProData["ProTokenUser"] = tc.newSubscription - if tc.breakConfig { - reg.Errors = registry.MockErrOnCreateKey - } if tc.configIsReadOnly { reg.KeyExists = true @@ -113,19 +77,6 @@ func TestNew(t *testing.T) { return } require.NoError(t, err, "New should return no error") - - // Subscriptions are updated asyncronously - time.Sleep(time.Second) - - out, err := os.ReadFile(subscriptionChecksumFile) - if tc.newSubscription == "" { - require.ErrorIs(t, err, fs.ErrNotExist, "The subscription file should have been removed.") - return - } - require.NoError(t, err, "Could not read subscription checksum file: %v", err) - - newChecksum := sha512.Sum512([]byte(tc.newSubscription)) - require.Equal(t, out, newChecksum[:], "Checksum does no match the new subscription's") }) } } From 1c3c58a9d0dfa86e04e06a12d8dc311f61f71b3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Thu, 26 Oct 2023 11:50:22 +0200 Subject: [PATCH 7/8] Implement TestUpdateRegistrySettings --- windows-agent/internal/config/config_test.go | 92 ++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/windows-agent/internal/config/config_test.go b/windows-agent/internal/config/config_test.go index be42e4775..7298d13cc 100644 --- a/windows-agent/internal/config/config_test.go +++ b/windows-agent/internal/config/config_test.go @@ -2,14 +2,23 @@ package config_test import ( "context" + "errors" + "io/fs" + "os" + "path/filepath" "testing" + "github.com/canonical/ubuntu-pro-for-windows/common/wsltestutils" config "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config/registry" + "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/database" + "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/distro" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/distros/task" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/tasks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + wsl "github.com/ubuntu/gowsl" + wslmock "github.com/ubuntu/gowsl/mock" ) // registryState represents how much data is in the registry. @@ -432,6 +441,89 @@ func TestFetchMicrosoftStoreSubscription(t *testing.T) { } } +func TestUpdateRegistrySettings(t *testing.T) { + if wsl.MockAvailable() { + t.Parallel() + } + + testCases := map[string]struct { + valueToChange string + registryState registryState + breakTaskfile bool + + wantTasks []string + unwantedTasks []string + wantErr bool + }{ + "Success changing Pro token": {valueToChange: "ProTokenOrg", registryState: keyExists, wantTasks: []string{"tasks.ProAttachment"}, unwantedTasks: []string{"tasks.LandscapeConfigure"}}, + "Success changing Landscape without a UID": {valueToChange: "LandscapeClientConfig", registryState: keyExists, unwantedTasks: []string{"tasks.ProAttachment", "tasks.LandscapeConfigure"}}, + "Success changing Landscape with a UID": {valueToChange: "LandscapeClientConfig", registryState: landscapeAgentUIDHasValue, wantTasks: []string{"tasks.LandscapeConfigure"}, unwantedTasks: []string{"tasks.ProAttachment"}}, + "Success changing the Landscape UID": {valueToChange: "LandscapeAgentUID", registryState: keyExists, wantTasks: []string{"tasks.LandscapeConfigure"}, unwantedTasks: []string{"tasks.ProAttachment"}}, + + // Very implementation-detailed, but it's the only thing that actually triggers an error + "Error when the tasks cannot be submitted": {valueToChange: "ProTokenOrg", registryState: keyExists, breakTaskfile: true, wantErr: true}, + } + + for name, tc := range testCases { + tc := tc + t.Run(name, func(t *testing.T) { + ctx := context.Background() + if wsl.MockAvailable() { + t.Parallel() + ctx = wsl.WithMock(ctx, wslmock.New()) + } + + dir := t.TempDir() + + distroName, _ := wsltestutils.RegisterDistro(t, ctx, false) + taskFilePath := filepath.Join(dir, distroName+".tasks") + + db, err := database.New(ctx, dir, nil) + require.NoError(t, err, "Setup: could create empty database") + + _, err = db.GetDistroAndUpdateProperties(ctx, distroName, distro.Properties{}) + require.NoError(t, err, "Setup: could not add dummy distro to database") + + r := setUpMockRegistry(0, tc.registryState, false) + require.NoError(t, err, "Setup: could not create empty database") + + c := config.New(ctx, config.WithRegistry(r)) + + // Update value in registry + r.UbuntuProData[tc.valueToChange] = "NEW_VALUE!" + + if tc.breakTaskfile { + err := os.MkdirAll(taskFilePath, 0600) + require.NoError(t, err, "could not create directory to interfere with task file") + } + + err = c.UpdateRegistrySettings(ctx, dir, db) + if tc.wantErr { + require.Error(t, err, "UpdateRegistrySettings should return an error") + return + } + require.NoError(t, err, "UpdateRegistrySettings should return no error") + + out, err := readFileOrEmpty(taskFilePath) + require.NoError(t, err, "Could not read distro taskfile") + for _, task := range tc.wantTasks { + assert.Containsf(t, out, task, "Distro should have received a %s task", task) + } + for _, task := range tc.unwantedTasks { + assert.NotContainsf(t, out, task, "Distro should have received a %s task", task) + } + }) + } +} + +func readFileOrEmpty(path string) (string, error) { + out, err := os.ReadFile(path) + if errors.Is(err, fs.ErrNotExist) { + return "", nil + } + return string(out), err +} + // is defines equality between flags. It is convenience function to check if a registryState matches a certain state. func (state registryState) is(flag registryState) bool { return state&flag == flag From eec60192cda3a737141ae030d77ade0dce98631e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Thu, 26 Oct 2023 12:09:22 +0200 Subject: [PATCH 8/8] Go mod tidy --- wsl-pro-service/go.sum | 2 -- 1 file changed, 2 deletions(-) diff --git a/wsl-pro-service/go.sum b/wsl-pro-service/go.sum index 89ddee454..95bc88253 100644 --- a/wsl-pro-service/go.sum +++ b/wsl-pro-service/go.sum @@ -42,8 +42,6 @@ github.com/canonical/ubuntu-pro-for-windows/agentapi v0.0.0-20231023183501-458a1 github.com/canonical/ubuntu-pro-for-windows/agentapi v0.0.0-20231023183501-458a10db4c16/go.mod h1:yZFCC1gVaFNjIJ7Glk3FBiaCokcV/Jk5B6tFIlHBgd8= github.com/canonical/ubuntu-pro-for-windows/common v0.0.0-20231023183501-458a10db4c16 h1:u7afWNLgssNSEHKxldRbgCobFnGlILpECBeAPiWyb0c= github.com/canonical/ubuntu-pro-for-windows/common v0.0.0-20231023183501-458a10db4c16/go.mod h1:a/RSoEHIs9RmXxL/+68oDpXi8+d+TGptM/9N5ULw2Kw= -github.com/canonical/ubuntu-pro-for-windows/wslserviceapi v0.0.0-20231023183501-458a10db4c16 h1:B/iSer/eNknIEp9TnNYC56zdc6SLY6K7fXxkeFl15s8= -github.com/canonical/ubuntu-pro-for-windows/wslserviceapi v0.0.0-20231023183501-458a10db4c16/go.mod h1:gjEBJv2vc1VUqqrv1ZTDcNmd3TBBCPIlmBaR63YSX9Y= github.com/canonical/ubuntu-pro-for-windows/wslserviceapi v0.0.0-20231025153609-b3e7db95a9ce h1:zuJOIrd+T+J5UVEiCHT9lK7sYS4VDJ5ZxI81Yvdr6jY= github.com/canonical/ubuntu-pro-for-windows/wslserviceapi v0.0.0-20231025153609-b3e7db95a9ce/go.mod h1:gjEBJv2vc1VUqqrv1ZTDcNmd3TBBCPIlmBaR63YSX9Y= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=