fix(agent): No empty cloudinit (#886)

When the config is empty, the yaml serialization outputs {}. We still
added some commentary in the agent.yaml, resulting in a bunch of nothing
that confused cloud-init, because it's enablement is based on the
existence of the files, their contents are only checked later. That
wouldn't be a problem per se, as the WSL datasource would recognize
there's nothing to do and bail out, but that would have costed
unnecesary CPU cicles, and worse than that, the current implementation
of cloud-init makes it log as an error the fact that there is nothing to
do at this point, which results in more noise if the user see the output
of cloud-init status --long.

The fix is rather simple: do not write an empty object into agent.yaml.
If the config is empty we delete that file. This way, if there is
nothing from the user nor the agent's perspective, cloud-init will
remain trully disabled.

I updated some tests that relied on the fact that cloud-init would
create the agent.yaml file unconditionally, which is no longer the case.

UDENG-4169

windows-agent/internal/cloudinit/cloudinit.go

@@ -37,6 +37,12 @@ func New(ctx context.Context, conf Config, publicDir string) (CloudInit, error)
 		conf:    conf,
 	}
 
+	// c.writeAgentData() is no longer guaranteed to create the cloud-init directory,
+	// so let's check now if we have permission to do so.
+	if err := os.MkdirAll(c.dataDir, 0700); err != nil {
+		return CloudInit{}, fmt.Errorf("could not create cloud-init directory: %v", err)
+	}
+
 	if err := c.writeAgentData(); err != nil {
 		return CloudInit{}, err
 	}
@@ -60,6 +66,11 @@ func (c CloudInit) writeAgentData() (err error) {
 		return err
 	}
 
+	// Nothing to write, we don't want an empty agent.yaml confusing the real cloud-init.
+	if cloudInit == nil {
+		return removeFileInDir(c.dataDir, "agent.yaml")
+	}
+
 	err = writeFileInDir(c.dataDir, "agent.yaml", cloudInit)
 	if err != nil {
 		return err
@@ -78,6 +89,15 @@ func (c CloudInit) WriteDistroData(distroName string, cloudInit string) error {
 	return nil
 }
 
+// removeFileInDir attempts to remove the file 'dir/file' if it exists. Missing file is not an error.
+func removeFileInDir(dir, file string) error {
+	err := os.Remove(filepath.Join(dir, file))
+	if err != nil && !os.IsNotExist(err) {
+		return 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.
@@ -121,12 +141,6 @@ func (c CloudInit) RemoveDistroData(distroName string) (err error) {
 }
 
 func marshalConfig(conf Config) ([]byte, error) {
-	w := &bytes.Buffer{}
-
-	if _, err := fmt.Fprintln(w, "#cloud-config\n# This file was generated automatically and must not be edited"); err != nil {
-		return nil, fmt.Errorf("could not write #cloud-config stenza and warning message: %v", err)
-	}
-
 	contents := make(map[string]interface{})
 
 	if err := ubuntuProModule(conf, contents); err != nil {
@@ -137,11 +151,22 @@ func marshalConfig(conf Config) ([]byte, error) {
 		return nil, err
 	}
 
+	// If there is no config to write, then let's not write an empty object with comments to avoid confusing cloud-init.
+	if len(contents) == 0 {
+		return nil, nil
+	}
+
 	out, err := yaml.Marshal(contents)
 	if err != nil {
 		return nil, fmt.Errorf("could not Marshal user data as a YAML: %v", err)
 	}
 
+	w := &bytes.Buffer{}
+
+	if _, err := fmt.Fprintln(w, "#cloud-config\n# This file was generated automatically and must not be edited"); err != nil {
+		return nil, fmt.Errorf("could not write #cloud-config stenza and warning message: %v", err)
+	}
+
 	if _, err := w.Write(out); err != nil {
 		return nil, fmt.Errorf("could not write config body: %v", err)
 	}

windows-agent/internal/cloudinit/cloudinit_test.go

@@ -19,9 +19,13 @@ func TestNew(t *testing.T) {
 
 	testCases := map[string]struct {
 		breakWriteAgentData bool
-		wantErr             bool
+		emptyConfig         bool
+
+		wantErr         bool
+		wantNoAgentYaml bool
 	}{
 		"Success": {},
+		"No file if there is no config to write into":        {emptyConfig: true, wantNoAgentYaml: true},
 		"Error when cloud-init agent file cannot be written": {breakWriteAgentData: true, wantErr: true},
 	}
 
@@ -33,7 +37,13 @@ func TestNew(t *testing.T) {
 
 			publicDir := t.TempDir()
 
+			proToken := "test token"
+			if tc.emptyConfig {
+				proToken = ""
+			}
+
 			conf := &mockConfig{
+				proToken:       proToken,
 				subcriptionErr: tc.breakWriteAgentData,
 			}
 
@@ -47,6 +57,10 @@ func TestNew(t *testing.T) {
 
 			// We don't assert on specifics, as they are tested in WriteAgentData tests.
 			path := filepath.Join(publicDir, ".cloud-init", "agent.yaml")
+			if tc.wantNoAgentYaml {
+				require.NoFileExists(t, path, "there should be no agent data file if there is no config to write into")
+				return
+			}
 			require.FileExists(t, path, "agent data file was not created when updating the config")
 		})
 	}
@@ -87,9 +101,12 @@ hostagent_uid = landscapeUID1234
 		badLandscape             bool
 
 		// Break writing to file
-		breakDir      bool
-		breakTempFile bool
-		breakFile     bool
+		breakDir          bool
+		breakTempFile     bool
+		breakFile         bool
+		breakRemovingFile bool
+
+		wantAgentYamlAsDir bool
 	}{
 		"Success":                            {},
 		"Without hostagent UID":              {skipHostAgentUID: true},
@@ -98,6 +115,7 @@ hostagent_uid = landscapeUID1234
 		"Without Landscape [client] section": {landscapeNoClientSection: true},
 		"With empty contents":                {skipProToken: true, skipLandscapeConf: true},
 
+		"Error to remove existing agent.yaml":   {skipProToken: true, skipLandscapeConf: true, breakRemovingFile: true, wantAgentYamlAsDir: true},
 		"Error obtaining pro token":             {breakSubscription: true},
 		"Error obtaining Landscape config":      {breakLandscape: true},
 		"Error with erroneous Landscape config": {badLandscape: true},
@@ -164,6 +182,11 @@ hostagent_uid = landscapeUID1234
 				require.NoError(t, os.WriteFile(dir, nil, 0600), "Setup: could not create file to mess with cloud-init directory")
 			}
 
+			if tc.breakRemovingFile {
+				_ = os.RemoveAll(path)
+				require.NoError(t, os.MkdirAll(path, 0750), "Creating the directory that breaks removing agent.yaml should not fail")
+				require.NoError(t, os.WriteFile(filepath.Join(path, "child.txt"), nil, 0600), "Setup: could not create file to mess with agent.yaml")
+			}
 			ci.Update(ctx)
 
 			// Assert that the file was updated (success case) or that the old one remains (error case)
@@ -172,6 +195,17 @@ hostagent_uid = landscapeUID1234
 				return
 			}
 
+			if tc.wantAgentYamlAsDir {
+				require.DirExists(t, path, "There should be a directory instead of agent.yaml")
+				return
+			}
+
+			golden := testutils.Path(t)
+			if _, err = os.Stat(golden); err != nil && os.IsNotExist(err) {
+				// golden file doesn't exist
+				require.NoFileExists(t, path, "There should not be cloud-init agent file without useful contents")
+				return
+			}
 			got, err := os.ReadFile(path)
 			require.NoError(t, err, "There should be no error reading the cloud-init agent file")
 
@@ -237,7 +271,7 @@ data:
 			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.MkdirAll(filepath.Dir(path), 0700), "Setup: could not write old distro data directory")
 				require.NoError(t, os.WriteFile(path, []byte(oldCloudInit), 0600), "Setup: could not write old distro data")
 			}
 

windows-agent/internal/proservices/proservices_test.go

@@ -92,12 +92,16 @@ func TestNew(t *testing.T) {
 			}
 			require.NoError(t, err, "New should return no error")
 
-			// 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")
 
+			agentYamlPath := filepath.Join(publicDir, ".cloud-init", "agent.yaml")
+
+			// Wait for the agent.yaml to be written
+			require.Eventually(t, checkFileExists(agentYamlPath), 5*time.Second, 200*time.Millisecond, "agent.yaml file should have been created with registry data")
+
 			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))
@@ -106,6 +110,13 @@ func TestNew(t *testing.T) {
 	}
 }
 
+func checkFileExists(path string) func() bool {
+	return func() bool {
+		s, err := os.Stat(path)
+		return err == nil && !s.IsDir()
+	}
+}
+
 func TestRegisterGRPCServices(t *testing.T) {
 	t.Parallel()
 

windows-agent/internal/proservices/testdata/TestNew/golden/when_the_config_cannot_check_if_it_is_read-only

@@ -1,3 +1,8 @@
 #cloud-config
 # This file was generated automatically and must not be edited
-{}
+landscape:
+    client:
+        tags: wsl
+        user: JohnDoe
+ubuntu_pro:
+    token: test-token

windows-agent/internal/proservices/testdata/TestNew/golden/when_the_subscription_stays_empty

@@ -1,3 +1,8 @@
 #cloud-config
 # This file was generated automatically and must not be edited
-{}
+landscape:
+    client:
+        tags: wsl
+        user: JohnDoe
+ubuntu_pro:
+    token: test-token