From 1b2ddb59b851a5c8c0c71975374ea309883216e8 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Wed, 4 Mar 2026 15:18:25 -0500 Subject: [PATCH 01/17] Update the logic for `shouldReportStatus` shouldReportStatus should be true whenever there was at least one executed action. If there's no action, then it should be true if the status is transitioning. --- main/main.go | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/main/main.go b/main/main.go index 00d3507..9c4ee3a 100644 --- a/main/main.go +++ b/main/main.go @@ -214,21 +214,19 @@ func customEnable(ext *vmextensionhelper.VMExtension, hostgaCommunicator hostgac return errors.Wrapf(err, "Could not get package registry") } - // write success status if requested sequence number is newer - shouldReportStatus := false - - if ext.CurrentSequenceNumber == nil || requestedSequenceNumber > *ext.CurrentSequenceNumber { - shouldReportStatus = true - } else if requestedSequenceNumber == *ext.CurrentSequenceNumber { - statusType, err := utils.GetStatusType(ext.HandlerEnv, requestedSequenceNumber) - if err != nil || strings.EqualFold(string(statusType), string(status.StatusTransitioning)) { - // either something is wrong with the status file - // or its a transitioning status file - // overwrite it in either case - shouldReportStatus = true - } - } - if shouldReportStatus { + // The status file needs to be updated whenever there is some VM App actions because + // all VM Apps are always processed. It also needed to be updated if the status is + // Transitioning even if there's no VM App actions to do. The only time update isn't + // needed is when VM is rebooted and there are no changes to the desired packages, + // and no VM App re-run after rebooting. + // + // This matters even if the extension already has a non-transitioning status file + // for the requested sequence number because an app might have failed earlier + // but become successful in the current run or vice versa. When an app reaches MaxReboot + // or when it runs successfully, the number of reboot is reset to 0, allowing it to + // run again if the VM is rebooted by any other process, which is why its new run + // status needs to be reflected in the status file. + if shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) { var statusResult status.StatusType statusMessage := getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, result) if executeError.GetErrorIfDeploymentFailed() == nil { // treatFailureAsDeploymentFailure @@ -258,7 +256,20 @@ func customEnable(ext *vmextensionhelper.VMExtension, hostgaCommunicator hostgac return nil } -// Callback indicating the extension is being removed +func shouldReportStatus(ext *vmextensionhelper.VMExtension, requestedSequenceNumber uint, vmAppResults *actionplan.PackageOperationResults) bool { + // Report status if any VM App operations were executed + if vmAppResults != nil && len(*vmAppResults) > 0 { + return true + } + // Also report status if the current status is Transitioning (to update it to Success/Error) + // or if there is something wrong with the status file + statusType, err := utils.GetStatusType(ext.HandlerEnv, requestedSequenceNumber) + if err != nil || strings.EqualFold(string(statusType), string(status.StatusTransitioning)) { + return true + } + return false +} + func vmAppUninstallCallback(ext *vmextensionhelper.VMExtension) error { ext.ExtensionEvents.LogInformationalEvent("Uninstalling", "VmApplications extension - removing all applications for uninstall") hostGaCommunicator := hostgacommunicator.HostGaCommunicator{} From c9e437290e9902530dae148cba3786110e2ab5d9 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Wed, 4 Mar 2026 19:51:57 -0500 Subject: [PATCH 02/17] Fixed test setup and cleanup --- main/main_test.go | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/main/main_test.go b/main/main_test.go index ddf848f..e43f523 100644 --- a/main/main_test.go +++ b/main/main_test.go @@ -73,15 +73,15 @@ func nopLog() *logging.ExtensionLogger { var maintestdir string -func TestMain(m *testing.M) { - testdir, err := ioutil.TempDir("", "maintest") +func setupTest(t *testing.T) { + testdir, err := os.MkdirTemp("", "maintest") if err != nil { - return + t.Fatalf("Failed to create temp dir: %v", err) } err = os.MkdirAll(testdir, constants.FilePermissions_UserOnly_ReadWriteExecute) if err != nil { - return + t.Fatalf("Failed to create test dir: %v", err) } setSequenceNumberFunc = func(extName, extVersion string, seqNo uint) error { @@ -90,13 +90,14 @@ func TestMain(m *testing.M) { } maintestdir = testdir - exitVal := m.Run() - os.RemoveAll(maintestdir) - os.Exit(exitVal) + t.Cleanup(func() { + os.RemoveAll(maintestdir) + }) } func Test_settingsFailToInit(t *testing.T) { + setupTest(t) ExtensionVersion = "" defer resetExtensionVersion() err := getExtensionAndRun([]string{"vm-application-manager", "enable"}) @@ -104,18 +105,21 @@ func Test_settingsFailToInit(t *testing.T) { } func Test_failToCreateExtension(t *testing.T) { + setupTest(t) // This will fail automatically because Guest Agent hasn't set the required sequence numbers err := getExtensionAndRun([]string{"vm-application-manager", "enable"}) require.Error(t, err) } func Test_getVMPackageData_noSettings(t *testing.T) { + setupTest(t) ext := createTestVMExtension(t, nil) err := customEnable(ext, noopHostGaCommunicator, 0) require.Error(t, err) } func Test_getVMPackageData_cannotDeserialize(t *testing.T) { + setupTest(t) vmPackages := "yabasnarfle {}" ext := createTestVMExtension(t, vmPackages) @@ -124,6 +128,7 @@ func Test_getVMPackageData_cannotDeserialize(t *testing.T) { } func Test_getVMPackageData_noApplications(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) @@ -132,6 +137,7 @@ func Test_getVMPackageData_noApplications(t *testing.T) { } func Test_getVMPackageData_valid(t *testing.T) { + setupTest(t) order := 1 vmApplications := []extdeserialization.VmAppSetting{ { @@ -148,6 +154,7 @@ func Test_getVMPackageData_valid(t *testing.T) { } func Test_getVMAppProtectedSettings_valid(t *testing.T) { + setupTest(t) order := 1 actions := extdeserialization.ActionSetting{ ActionName: "logging", @@ -181,6 +188,7 @@ func Test_getVMAppProtectedSettings_valid(t *testing.T) { } func Test_getVMAppProtectedSettings_valid_no_custom_actions(t *testing.T) { + setupTest(t) order := 1 appSettings := extdeserialization.VmAppSetting{ @@ -201,6 +209,7 @@ func Test_getVMAppProtectedSettings_valid_no_custom_actions(t *testing.T) { } func Test_getVMPackageData_noVersion(t *testing.T) { + setupTest(t) order := 1 vmApplications := []extdeserialization.VmAppSetting{ { @@ -217,6 +226,7 @@ func Test_getVMPackageData_noVersion(t *testing.T) { } func Test_GetApplicationMetadataWithInvalidRebootBehavior_DefaultsToNone(t *testing.T) { + setupTest(t) order := 1 vmApplications := []extdeserialization.VmAppSetting{ { @@ -247,6 +257,7 @@ func Test_GetApplicationMetadataWithInvalidRebootBehavior_DefaultsToNone(t *test } func Test_getVMPackageDataCustomAction_valid(t *testing.T) { + setupTest(t) order := 1 actions := extdeserialization.ActionSetting{ ActionName: "Action1", @@ -309,6 +320,7 @@ func Test_getVMPackageDataCustomAction_valid(t *testing.T) { } func Test_getVMPackageDataCustomAction_CriticalError(t *testing.T) { + setupTest(t) order := 1 actions := extdeserialization.ActionSetting{ ActionName: "Action1", @@ -333,6 +345,7 @@ func Test_getVMPackageDataCustomAction_CriticalError(t *testing.T) { } func Test_getVMPackageData_noApplicationName(t *testing.T) { + setupTest(t) order := 1 vmApplications := []extdeserialization.VmAppSetting{ { @@ -349,6 +362,7 @@ func Test_getVMPackageData_noApplicationName(t *testing.T) { } func Test_main_statusIsWrittenForCriticalErrors(t *testing.T) { + setupTest(t) order := 1 vmApplications := []extdeserialization.VmAppSetting{ { @@ -384,6 +398,7 @@ func Test_main_statusIsWrittenForCriticalErrors(t *testing.T) { } func Test_main_statusIsNotWrittenForFileLockErrors(t *testing.T) { + setupTest(t) order := 1 vmApplications := []extdeserialization.VmAppSetting{ { @@ -417,6 +432,7 @@ func Test_main_statusIsNotWrittenForFileLockErrors(t *testing.T) { } func Test_main_nothingToProcess_noStatusUpdate(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) @@ -434,6 +450,7 @@ func Test_main_nothingToProcess_noStatusUpdate(t *testing.T) { } func Test_main_transitioningStatusIsUpdated(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) @@ -451,6 +468,7 @@ func Test_main_transitioningStatusIsUpdated(t *testing.T) { } func Test_main_nothingToProcess_withStatus(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) hostGaCommunicator := NoopHostGaCommunicator{} @@ -467,6 +485,7 @@ func Test_main_nothingToProcess_withStatus(t *testing.T) { } func Test_uninstall_cannotCreatePackageRegistry(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) hostGaCommunicator := NoopHostGaCommunicator{} @@ -480,6 +499,7 @@ func Test_uninstall_cannotCreatePackageRegistry(t *testing.T) { } func Test_uninstall_cannotReadPackageRegistry(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) hostGaCommunicator := NoopHostGaCommunicator{} @@ -495,6 +515,7 @@ func Test_uninstall_cannotReadPackageRegistry(t *testing.T) { } func Test_uninstall_noAppsToUninstall(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) hostGaCommunicator := NoopHostGaCommunicator{} @@ -536,6 +557,7 @@ func Test_uninstall_noAppsToUninstall(t *testing.T) { } func Test_uninstall_uninstallApps(t *testing.T) { + setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} ext := createTestVMExtension(t, vmApplications) hostGaCommunicator := NoopHostGaCommunicator{} From d6d50b990dc298f47baeea279c8b02346a49dd7b Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Wed, 4 Mar 2026 20:26:10 -0500 Subject: [PATCH 03/17] Added unit tests for the updated shouldReportStatus function --- main/main_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/main/main_test.go b/main/main_test.go index e43f523..7a751ad 100644 --- a/main/main_test.go +++ b/main/main_test.go @@ -484,6 +484,50 @@ func Test_main_nothingToProcess_withStatus(t *testing.T) { require.Equal(t, requestedSequenceNumber, currentSequenceNumber) } +// Test that shouldReportStatus returns true when vmAppResults has elements, +// even if there's an existing error status file +func Test_shouldReportStatus_withVmAppResults_existingErrorStatus(t *testing.T) { + setupTest(t) + vmApplications := []extdeserialization.VmAppSetting{} + ext := createTestVMExtension(t, vmApplications) + requestedSequenceNumber := *ext.CurrentSequenceNumber + + // Pre-create a status file with error status + err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, status.StatusError, vmextension.EnableOperation.ToStatusName(), "previous failure") + require.NoError(t, err) + + // Create vmAppResults with one result (simulating an app was executed) + vmAppResults := &actionplan.PackageOperationResults{ + {PackageName: "testapp", Operation: "install", Result: actionplan.Success}, + } + + // shouldReportStatus should return true because vmAppResults has elements + result := shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) + require.True(t, result, "shouldReportStatus should return true when vmAppResults has elements") +} + +// Test that shouldReportStatus returns true when vmAppResults has elements, +// even if there's an existing success status file +func Test_shouldReportStatus_withVmAppResults_existingSuccessStatus(t *testing.T) { + setupTest(t) + vmApplications := []extdeserialization.VmAppSetting{} + ext := createTestVMExtension(t, vmApplications) + requestedSequenceNumber := *ext.CurrentSequenceNumber + + // Pre-create a status file with success status + err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, status.StatusSuccess, vmextension.EnableOperation.ToStatusName(), "previous success") + require.NoError(t, err) + + // Create vmAppResults with one result (simulating an app was executed) + vmAppResults := &actionplan.PackageOperationResults{ + {PackageName: "testapp", Operation: "install", Result: "Error: command failed"}, + } + + // shouldReportStatus should return true because vmAppResults has elements + result := shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) + require.True(t, result, "shouldReportStatus should return true when vmAppResults has elements") +} + func Test_uninstall_cannotCreatePackageRegistry(t *testing.T) { setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} From 4f743d629010c3b2bf6966c90bab7292145598d7 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Wed, 4 Mar 2026 20:37:06 -0500 Subject: [PATCH 04/17] Update deprecated references of ioutil in main_test.go --- main/main_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/main/main_test.go b/main/main_test.go index 7a751ad..ac7fe3c 100644 --- a/main/main_test.go +++ b/main/main_test.go @@ -7,7 +7,6 @@ import ( "encoding/json" "errors" "fmt" - "io/ioutil" "os" "path" "path/filepath" @@ -296,7 +295,7 @@ func Test_getVMPackageDataCustomAction_valid(t *testing.T) { require.Contains(t, currentpackages[vmApplications[0].ApplicationName].Result, actionplan.Success) // test contents of the status file statusFilePath := filepath.Join(ext.HandlerEnv.StatusFolder, fmt.Sprintf("%d.status", requestedSequenceNumber)) - fileBytes, err := ioutil.ReadFile(statusFilePath) + fileBytes, err := os.ReadFile(statusFilePath) require.NoError(t, err) statusReport := status.StatusReport{} err = json.Unmarshal(fileBytes, &statusReport) @@ -386,7 +385,7 @@ func Test_main_statusIsWrittenForCriticalErrors(t *testing.T) { err := getExtensionAndRun([]string{"vm-application-manager", vmextension.EnableOperation.ToString()}) require.NoError(t, err) statusFilePath := filepath.Join(ext.HandlerEnv.StatusFolder, fmt.Sprintf("%d.status", requestedSequenceNumber)) - fileBytes, err := ioutil.ReadFile(statusFilePath) + fileBytes, err := os.ReadFile(statusFilePath) require.NoError(t, err) fileString := string(fileBytes) require.Contains(t, fileString, vmextension.EnableOperation.ToStatusName()) @@ -476,7 +475,7 @@ func Test_main_nothingToProcess_withStatus(t *testing.T) { err := customEnable(ext, &hostGaCommunicator, requestedSequenceNumber) require.NoError(t, err) statusFilePath := filepath.Join(ext.HandlerEnv.StatusFolder, fmt.Sprintf("%d.status", requestedSequenceNumber)) - fileBytes, err := ioutil.ReadFile(statusFilePath) + fileBytes, err := os.ReadFile(statusFilePath) require.NoError(t, err) fileString := string(fileBytes) require.Contains(t, fileString, vmextension.EnableOperation.ToStatusName()) @@ -550,7 +549,7 @@ func Test_uninstall_cannotReadPackageRegistry(t *testing.T) { // Write an invalid registry so we can't create a package registry appRegistryFilePath := path.Join(ext.HandlerEnv.ConfigFolder, packageregistry.LocalApplicationRegistryFileName) - ioutil.WriteFile(appRegistryFilePath, []byte("}"), 0644) + os.WriteFile(appRegistryFilePath, []byte("}"), 0644) defer os.Remove(appRegistryFilePath) err := doVmAppUninstallCallback(ext, &hostGaCommunicator) From 00b373ecd6cbfea0fbd783952c363f91a00aae0b Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Mon, 9 Mar 2026 15:49:33 -0400 Subject: [PATCH 05/17] Edit code comment --- main/main.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/main/main.go b/main/main.go index 9c4ee3a..4229044 100644 --- a/main/main.go +++ b/main/main.go @@ -219,13 +219,6 @@ func customEnable(ext *vmextensionhelper.VMExtension, hostgaCommunicator hostgac // Transitioning even if there's no VM App actions to do. The only time update isn't // needed is when VM is rebooted and there are no changes to the desired packages, // and no VM App re-run after rebooting. - // - // This matters even if the extension already has a non-transitioning status file - // for the requested sequence number because an app might have failed earlier - // but become successful in the current run or vice versa. When an app reaches MaxReboot - // or when it runs successfully, the number of reboot is reset to 0, allowing it to - // run again if the VM is rebooted by any other process, which is why its new run - // status needs to be reflected in the status file. if shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) { var statusResult status.StatusType statusMessage := getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, result) From 586085dbda1f263af131309dd6827c4e4e997323 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 13 Mar 2026 11:11:26 -0400 Subject: [PATCH 06/17] Added custom error for hostgacommunicator --- .../hostgacommunicator/hostgacommunicator.go | 46 +++++++++++++++++-- .../hostgacommunicator_test.go | 4 +- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/internal/hostgacommunicator/hostgacommunicator.go b/internal/hostgacommunicator/hostgacommunicator.go index e9f6887..088c526 100644 --- a/internal/hostgacommunicator/hostgacommunicator.go +++ b/internal/hostgacommunicator/hostgacommunicator.go @@ -18,6 +18,37 @@ import ( const hostGaPluginPort = "32526" const WireProtocolAddress = "AZURE_GUEST_AGENT_WIRE_PROTOCOL_ADDRESS" const wireServerFallbackAddress = "http://168.63.129.16:32526" +const HostGaMetadataErrorPrefix = "HostGaCommunicator GetVMAppInfo error" + +type HostGaCommunicatorError int + +const ( + InitializationError HostGaCommunicatorError = iota + MetadataRequestFailedWithRetries + MetadataRequestFailedInvalidResponseBody +) + +func (hostGaCommunicatorError HostGaCommunicatorError) ToString() string { + switch hostGaCommunicatorError { + case InitializationError: + return "InitializationError" + case MetadataRequestFailedWithRetries: + return "MetadataRequestFailedWithRetries" + case MetadataRequestFailedInvalidResponseBody: + return "MetadataRequestFailedInvalidResponseBody" + default: + return "UnknownError" + } +} + +type HostGaCommunicatorGetVMAppInfoError struct { + errorMessage string + errorType HostGaCommunicatorError +} + +func (e *HostGaCommunicatorGetVMAppInfoError) Error() string { + return fmt.Sprintf("%s: %s, error type: %s", HostGaMetadataErrorPrefix, e.errorMessage, e.errorType.ToString()) +} type IHostGaCommunicator interface { DownloadPackage(el *logging.ExtensionLogger, appName string, dst string) error @@ -33,7 +64,10 @@ type HostGaCommunicator struct{} func (*HostGaCommunicator) GetVMAppInfo(el *logging.ExtensionLogger, appName string) (*VMAppMetadata, error) { requestManager, isArc, err := getMetadataRequestManager(el, appName) if err != nil { - return nil, errors.Wrapf(err, "Could not create the request manager") + return nil, &HostGaCommunicatorGetVMAppInfoError{ + errorMessage: fmt.Sprintf("Could not create the request manager: %v", err), + errorType: InitializationError, + } } var resp *http.Response @@ -47,7 +81,10 @@ func (*HostGaCommunicator) GetVMAppInfo(el *logging.ExtensionLogger, appName str } if err != nil { - return nil, errors.Wrapf(err, "Metadata request failed with retries.") + return nil, &HostGaCommunicatorGetVMAppInfoError{ + errorMessage: fmt.Sprintf("Metadata request failed after retries: %v", err), + errorType: MetadataRequestFailedWithRetries, + } } body := resp.Body @@ -56,7 +93,10 @@ func (*HostGaCommunicator) GetVMAppInfo(el *logging.ExtensionLogger, appName str var target VMAppMetadataReceiver err = json.NewDecoder(body).Decode(&target) if err != nil { - return nil, errors.Wrapf(err, "failed to decode response body") + return nil, &HostGaCommunicatorGetVMAppInfoError{ + errorMessage: fmt.Sprintf("Failed to decode response body: %v", err), + errorType: MetadataRequestFailedInvalidResponseBody, + } } return target.MapToVMAppMetadata(), nil diff --git a/internal/hostgacommunicator/hostgacommunicator_test.go b/internal/hostgacommunicator/hostgacommunicator_test.go index bed0ee8..0d05e13 100644 --- a/internal/hostgacommunicator/hostgacommunicator_test.go +++ b/internal/hostgacommunicator/hostgacommunicator_test.go @@ -60,7 +60,7 @@ func TestGetVmAppInfo_RequestFailed(t *testing.T) { hgc := &HostGaCommunicator{} _, err := hgc.GetVMAppInfo(nopLog(), myAppName) require.NotNil(t, err, "did not fail") - require.Contains(t, err.Error(), "Metadata request failed with retries.", "Wrong message for failed request") + require.Contains(t, err.Error(), "Metadata request failed after retries:", "Wrong message for failed request") } func TestGetVmAppInfo_CouldNotDecodeResponse(t *testing.T) { @@ -75,7 +75,7 @@ func TestGetVmAppInfo_CouldNotDecodeResponse(t *testing.T) { hgc := &HostGaCommunicator{} _, err := hgc.GetVMAppInfo(nopLog(), myAppName) require.NotNil(t, err, "did not fail") - require.Contains(t, err.Error(), "failed to decode response body", "Wrong message for invalid response") + require.Contains(t, err.Error(), "Failed to decode response body:", "Wrong message for invalid response") } func TestGetVmAppInfo_MissingProperties(t *testing.T) { From adefc61380b925d0188d0d8b739131b33bc197a7 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 13 Mar 2026 11:39:53 -0400 Subject: [PATCH 07/17] Update status computation Switch back status after encountering a transient error --- launcher/main.go | 2 +- main/main.go | 136 ++++++++++++++++++------ main/main_test.go | 221 ++++++++++++++++++++++++++++++--------- pkg/utils/status.go | 43 ++++++-- pkg/utils/status_test.go | 39 ++++++- 5 files changed, 350 insertions(+), 91 deletions(-) diff --git a/launcher/main.go b/launcher/main.go index 8f79159..9c868ae 100644 --- a/launcher/main.go +++ b/launcher/main.go @@ -85,7 +85,7 @@ func main() { if requestedSequenceNumber >= currentSequenceNumber { // attempt to write a transitioning status file if it doesn't exist - _, getStatusError := utils.GetStatusType(handlerEnv, requestedSequenceNumber) + _, getStatusError := utils.GetStatus(handlerEnv, requestedSequenceNumber) if getStatusError != nil { // either no transitioning status file was found, or the status file was malformed // either way create a new transitioning status file diff --git a/main/main.go b/main/main.go index 4229044..131b2d3 100644 --- a/main/main.go +++ b/main/main.go @@ -27,7 +27,6 @@ import ( var ( ExtensionName string // assign at compile time ExtensionVersion = "1.0.10" // should be assigned at compile time, do not edit in code - reportStatusFunc = utils.ReportStatus getVMExtensionFunc = getVMExtension customEnableFunc = customEnable setSequenceNumberFunc = seqno.SetSequenceNumber @@ -95,15 +94,24 @@ func getExtensionAndRun(arguments []string) error { ext.ExtensionLogger.Error(errorMessage) ext.ExtensionEvents.LogErrorEvent("Enable Failed", errorMessage) default: + if _, ok := enableError.(*hostgacommunicator.HostGaCommunicatorGetVMAppInfoError); ok { + // Preserve the last good status file if it exists and isn't already a + // HostGA network error + if statusObj, err := utils.GetStatus(ext.HandlerEnv, requestedSequenceNumber); err == nil { + msg := statusObj.FormattedMessage.Message + if !strings.HasPrefix(msg, hostgacommunicator.HostGaMetadataErrorPrefix) { + if err := utils.BackupStatusFile(ext.HandlerEnv.StatusFolder, requestedSequenceNumber); err != nil { + ext.ExtensionLogger.Warn("Failed to back up status file for sequence %d: %v", requestedSequenceNumber, err) + } + } + } + } ext.ExtensionLogger.Error(enableError.Error()) ext.ExtensionEvents.LogErrorEvent("Enable Failed", enableError.Error()) // try to save status file statusMessage := enableError.Error() - err := reportStatusFunc(ext.HandlerEnv, requestedSequenceNumber, status.StatusError, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) + err := reportStatusFunc(ext, requestedSequenceNumber, status.StatusError, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) if err != nil { - errorMessage := fmt.Sprintf("Failed to save status file: %s", err.Error()) - ext.ExtensionLogger.Error(errorMessage) - ext.ExtensionEvents.LogErrorEvent("Save Status", errorMessage) return err } } @@ -214,28 +222,18 @@ func customEnable(ext *vmextensionhelper.VMExtension, hostgaCommunicator hostgac return errors.Wrapf(err, "Could not get package registry") } - // The status file needs to be updated whenever there is some VM App actions because - // all VM Apps are always processed. It also needed to be updated if the status is - // Transitioning even if there's no VM App actions to do. The only time update isn't - // needed is when VM is rebooted and there are no changes to the desired packages, - // and no VM App re-run after rebooting. - if shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) { - var statusResult status.StatusType - statusMessage := getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, result) - if executeError.GetErrorIfDeploymentFailed() == nil { // treatFailureAsDeploymentFailure - statusResult = status.StatusSuccess - } else { - statusResult = status.StatusError - } - err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, statusResult, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) + statusUpdated, statusResult, statusMessage := computeStatus(ext, requestedSequenceNumber, ¤tPackageRegistry, executeError, result, vmAppResults) + + if statusUpdated { + err := reportStatusFunc(ext, requestedSequenceNumber, statusResult, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) if err != nil { - errorMessage := fmt.Sprintf("Failed to save status file: %s", err.Error()) - ext.ExtensionLogger.Error(errorMessage) - ext.ExtensionEvents.LogErrorEvent("Save Status", errorMessage) return err } + // update the sequence number that has been executed - if err := setSequenceNumberFunc(ExtensionName, ExtensionVersion, requestedSequenceNumber); err != nil { + err = setSequenceNumberFunc(ExtensionName, ExtensionVersion, requestedSequenceNumber) + if err != nil { + // log but not return the error errorMessage := fmt.Sprintf("Failed to update sequence number to %d: %s", requestedSequenceNumber, err.Error()) ext.ExtensionLogger.Error(errorMessage) ext.ExtensionEvents.LogErrorEvent("Update Sequence Number", errorMessage) @@ -249,18 +247,92 @@ func customEnable(ext *vmextensionhelper.VMExtension, hostgaCommunicator hostgac return nil } -func shouldReportStatus(ext *vmextensionhelper.VMExtension, requestedSequenceNumber uint, vmAppResults *actionplan.PackageOperationResults) bool { - // Report status if any VM App operations were executed +func computeStatus( + ext *vmextensionhelper.VMExtension, + requestedSequenceNumber uint, + currentPackageRegistry *packageregistry.CurrentPackageRegistry, + executeError *actionplan.ExecuteError, + customActionResult actionplan.IResult, + vmAppResults *actionplan.PackageOperationResults, +) (bool, status.StatusType, string) { + statusUpdated := false + var statusResult status.StatusType + var statusMessage string + if vmAppResults != nil && len(*vmAppResults) > 0 { - return true + // executeError is only meaningful if there are VM App operations, otherwise + // it is the equivalent of no error (i.e success). + statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) + if executeError.GetErrorIfDeploymentFailed() == nil { // treatFailureAsDeploymentFailure + statusResult = status.StatusSuccess + } else { + statusResult = status.StatusError + } + statusUpdated = true + } else { + // These next cases are dependent on the existing status + statusObj, err := utils.GetStatus(ext.HandlerEnv, requestedSequenceNumber) + if err != nil { + // Existing status file maybe corrupted or missing. The existing behavior is + // to write a success status. + statusMessage = fmt.Sprintf("Failed to read existing status file: %v. Writing success status.", err) + statusResult = status.StatusSuccess + statusUpdated = true + } else if strings.EqualFold(string(statusObj.Status), string(status.StatusTransitioning)) { + // If the status is Transitioning whenever there's a new requested sequence number, + // as the Transitioning status is saved by launcher program. + if ext.CurrentSequenceNumber == nil || requestedSequenceNumber == *ext.CurrentSequenceNumber { + // If this the very first time this extension is Enabled, or we're processing + // the same sequence number, and there is no VM App to process, save the status as Success + // It is also possible that a VM App cause a reboot but is not re-run again leading to + // no VM App operations. We should also set the status to success in this case. + statusMessage = "No VM App operations to perform, but current status is Transitioning. Updating status to Success." + statusResult = status.StatusSuccess + } else { + // If this is a new sequence number with no VM App operations, then report + // the same status as the last sequence number. + // This case can happen if the user changes the ordering inside applicationProfile + // without changing any app or their installation order. + prevStatusObj, prevStatusErr := utils.GetStatus(ext.HandlerEnv, *ext.CurrentSequenceNumber) + if prevStatusErr != nil { + // No existing status save, should record as success since there are no VM App operations + statusMessage = fmt.Sprintf("No existing status file found for sequence %d, and no VM App operations. Writing success status.", *ext.CurrentSequenceNumber) + statusResult = status.StatusSuccess + } else { + statusMessage = prevStatusObj.FormattedMessage.Message + statusResult = prevStatusObj.Status + } + } + statusUpdated = true + } else if strings.Contains(statusObj.FormattedMessage.Message, hostgacommunicator.HostGaMetadataErrorPrefix) { + // If there is no VM App operations, but the current status is a transient host GA + // communication error, the status should be the same as the last stable status + prevStatusObj, prevStatusErr := utils.GetLastStableStatus(ext.HandlerEnv, *ext.CurrentSequenceNumber) + if prevStatusErr != nil { + // No last stable status save, should record as success since the hostGA issue + // is gone + statusResult = status.StatusSuccess + statusMessage = "No last stable status found, and no VM App operations." + } else { + statusResult = prevStatusObj.Status + statusMessage = prevStatusObj.FormattedMessage.Message + } + statusUpdated = true + } } - // Also report status if the current status is Transitioning (to update it to Success/Error) - // or if there is something wrong with the status file - statusType, err := utils.GetStatusType(ext.HandlerEnv, requestedSequenceNumber) - if err != nil || strings.EqualFold(string(statusType), string(status.StatusTransitioning)) { - return true + + return statusUpdated, statusResult, statusMessage +} + +// A wrapper for utils.ReportStatus to log any errors occurring in that function +func reportStatusFunc(ext *vmextensionhelper.VMExtension, requestedSequenceNumber uint, statusType status.StatusType, operationName string, message string) error { + err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, statusType, operationName, message) + if err != nil { + errorMessage := fmt.Sprintf("Failed to save status file: %s", err.Error()) + ext.ExtensionLogger.Error(errorMessage) + ext.ExtensionEvents.LogErrorEvent("Save Status", errorMessage) } - return false + return err } func vmAppUninstallCallback(ext *vmextensionhelper.VMExtension) error { diff --git a/main/main_test.go b/main/main_test.go index ac7fe3c..7610b93 100644 --- a/main/main_test.go +++ b/main/main_test.go @@ -442,9 +442,10 @@ func Test_main_nothingToProcess_noStatusUpdate(t *testing.T) { err = customEnable(ext, &hostGaCommunicator, requestedSequenceNumber) require.NoError(t, err) // ensure stautus file is not overwritten - statusType, err := utils.GetStatusType(ext.HandlerEnv, requestedSequenceNumber) + statusObj, err := utils.GetStatus(ext.HandlerEnv, requestedSequenceNumber) require.NoError(t, err) - require.Equal(t, status.StatusError, statusType) + require.NotNil(t, statusObj) + require.Equal(t, status.StatusError, statusObj.Status) require.Equal(t, requestedSequenceNumber, currentSequenceNumber) } @@ -460,9 +461,10 @@ func Test_main_transitioningStatusIsUpdated(t *testing.T) { err = customEnable(ext, &hostGaCommunicator, requestedSequenceNumber) require.NoError(t, err) // ensure error stautus file is not overwritten - statusType, err := utils.GetStatusType(ext.HandlerEnv, requestedSequenceNumber) + statusObj, err := utils.GetStatus(ext.HandlerEnv, requestedSequenceNumber) require.NoError(t, err) - require.Equal(t, status.StatusSuccess, statusType) + require.NotNil(t, statusObj) + require.Equal(t, status.StatusSuccess, statusObj.Status) require.Equal(t, requestedSequenceNumber, currentSequenceNumber) } @@ -483,50 +485,6 @@ func Test_main_nothingToProcess_withStatus(t *testing.T) { require.Equal(t, requestedSequenceNumber, currentSequenceNumber) } -// Test that shouldReportStatus returns true when vmAppResults has elements, -// even if there's an existing error status file -func Test_shouldReportStatus_withVmAppResults_existingErrorStatus(t *testing.T) { - setupTest(t) - vmApplications := []extdeserialization.VmAppSetting{} - ext := createTestVMExtension(t, vmApplications) - requestedSequenceNumber := *ext.CurrentSequenceNumber - - // Pre-create a status file with error status - err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, status.StatusError, vmextension.EnableOperation.ToStatusName(), "previous failure") - require.NoError(t, err) - - // Create vmAppResults with one result (simulating an app was executed) - vmAppResults := &actionplan.PackageOperationResults{ - {PackageName: "testapp", Operation: "install", Result: actionplan.Success}, - } - - // shouldReportStatus should return true because vmAppResults has elements - result := shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) - require.True(t, result, "shouldReportStatus should return true when vmAppResults has elements") -} - -// Test that shouldReportStatus returns true when vmAppResults has elements, -// even if there's an existing success status file -func Test_shouldReportStatus_withVmAppResults_existingSuccessStatus(t *testing.T) { - setupTest(t) - vmApplications := []extdeserialization.VmAppSetting{} - ext := createTestVMExtension(t, vmApplications) - requestedSequenceNumber := *ext.CurrentSequenceNumber - - // Pre-create a status file with success status - err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, status.StatusSuccess, vmextension.EnableOperation.ToStatusName(), "previous success") - require.NoError(t, err) - - // Create vmAppResults with one result (simulating an app was executed) - vmAppResults := &actionplan.PackageOperationResults{ - {PackageName: "testapp", Operation: "install", Result: "Error: command failed"}, - } - - // shouldReportStatus should return true because vmAppResults has elements - result := shouldReportStatus(ext, requestedSequenceNumber, vmAppResults) - require.True(t, result, "shouldReportStatus should return true when vmAppResults has elements") -} - func Test_uninstall_cannotCreatePackageRegistry(t *testing.T) { setupTest(t) vmApplications := []extdeserialization.VmAppSetting{} @@ -672,3 +630,170 @@ func createTestVMExtension(t *testing.T, settings interface{}) *vmextension.VMEx ExtensionEvents: eem, } } + +// Test computeStatus when vmAppResults has items and no deployment failure +func Test_computeStatus_WithVMAppResults_Success(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + vmAppResults := actionplan.PackageOperationResults{ + actionplan.PackageOperationResult{ + PackageName: "testApp", + AppVersion: "1.0.0", + Operation: "install", + Result: "0", + }, + } + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + + updated, statusType, _ := computeStatus(ext, 1, ¤tPkgReg, executeError, &vmAppResults, &vmAppResults) + + assert.True(t, updated) + assert.Equal(t, status.StatusSuccess, statusType) +} + +// Test computeStatus when no vmAppResults and status file doesn't exist +func Test_computeStatus_NoVMAppResults_NoStatusFile(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + emptyResults := actionplan.PackageOperationResults{} + + updated, statusType, _ := computeStatus(ext, 1, ¤tPkgReg, executeError, &emptyResults, nil) + + assert.True(t, updated) + assert.Equal(t, status.StatusSuccess, statusType) +} + +// Test computeStatus when no vmAppResults and current status has HostGA error prefix but no last stable +func Test_computeStatus_NoVMAppResults_HostGAError_NoLastStable(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + // Write a status file with HostGA error prefix + // ReportStatus adds "Enable failed: " prefix, but Contains still finds the HostGaMetadataErrorPrefix + err := utils.ReportStatus(ext.HandlerEnv, 1, status.StatusError, "Enable", hostgacommunicator.HostGaMetadataErrorPrefix+" some error") + require.NoError(t, err) + + seqNo := uint(1) + ext.CurrentSequenceNumber = &seqNo + + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + emptyResults := actionplan.PackageOperationResults{} + + updated, statusType, _ := computeStatus(ext, 1, ¤tPkgReg, executeError, &emptyResults, nil) + + // HostGA error detected, no last stable status exists, so update to success + assert.True(t, updated) + assert.Equal(t, status.StatusSuccess, statusType) +} + +// Test computeStatus when no vmAppResults and current status has HostGA error with last stable status +func Test_computeStatus_NoVMAppResults_HostGAError_WithLastStable(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + // First write a stable (lastgood) status file + err := utils.ReportStatus(ext.HandlerEnv, 1, status.StatusSuccess, "Enable", "last good message") + require.NoError(t, err) + err = utils.BackupStatusFile(ext.HandlerEnv.StatusFolder, 1) + require.NoError(t, err) + + // Now write a status file with HostGA error prefix + // ReportStatus adds "Enable failed: " prefix, but Contains still finds the HostGaMetadataErrorPrefix + err = utils.ReportStatus(ext.HandlerEnv, 1, status.StatusError, "Enable", hostgacommunicator.HostGaMetadataErrorPrefix+" some error") + require.NoError(t, err) + + seqNo := uint(1) + ext.CurrentSequenceNumber = &seqNo + + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + emptyResults := actionplan.PackageOperationResults{} + + updated, statusType, msg := computeStatus(ext, 1, ¤tPkgReg, executeError, &emptyResults, nil) + + // HostGA error detected, last stable status restored + assert.True(t, updated) + assert.Equal(t, status.StatusSuccess, statusType) + assert.Contains(t, msg, "last good message") +} + +// Test computeStatus when status is Transitioning and CurrentSequenceNumber is nil (first Enable) +func Test_computeStatus_StatusTransitioning_FirstEnable(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + // Write a transitioning status file + err := utils.ReportStatus(ext.HandlerEnv, 1, status.StatusTransitioning, "Enable", "transitioning") + require.NoError(t, err) + + // Set CurrentSequenceNumber to nil (first time Enable) + ext.CurrentSequenceNumber = nil + + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + emptyResults := actionplan.PackageOperationResults{} + + updated, statusType, msg := computeStatus(ext, 1, ¤tPkgReg, executeError, &emptyResults, nil) + + assert.True(t, updated) + assert.Equal(t, status.StatusSuccess, statusType) + assert.Contains(t, msg, "No VM App operations to perform, but current status is Transitioning") +} + +// Test computeStatus when status is Transitioning with previous sequence having a status file +func Test_computeStatus_StatusTransitioning_WithPreviousStatus(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + // Write a success status for previous sequence number (1) + err := utils.ReportStatus(ext.HandlerEnv, 1, status.StatusError, "Enable", "previous sequence failed message") + require.NoError(t, err) + + // Write a transitioning status for new sequence number (2) + err = utils.ReportStatus(ext.HandlerEnv, 2, status.StatusTransitioning, "Enable", "transitioning") + require.NoError(t, err) + + // Set CurrentSequenceNumber to 1 (previous) + seqNo := uint(1) + ext.CurrentSequenceNumber = &seqNo + + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + emptyResults := actionplan.PackageOperationResults{} + + updated, statusType, msg := computeStatus(ext, 2, ¤tPkgReg, executeError, &emptyResults, nil) + + assert.True(t, updated) + assert.Equal(t, status.StatusError, statusType) + // The code returns the message from the previous sequence's status + assert.Contains(t, msg, "previous sequence failed message") +} + +// Test computeStatus when status is Transitioning but previous sequence status file is missing +func Test_computeStatus_StatusTransitioning_NoPreviousStatus(t *testing.T) { + setupTest(t) + ext := createTestVMExtension(t, nil) + + // Write a transitioning status for new sequence number (2) only + err := utils.ReportStatus(ext.HandlerEnv, 2, status.StatusTransitioning, "Enable", "transitioning") + require.NoError(t, err) + + // Set CurrentSequenceNumber to 1 (previous) - but no status file exists for it + seqNo := uint(1) + ext.CurrentSequenceNumber = &seqNo + + executeError := &actionplan.ExecuteError{} + currentPkgReg := packageregistry.CurrentPackageRegistry{} + emptyResults := actionplan.PackageOperationResults{} + + updated, statusType, _ := computeStatus(ext, 2, ¤tPkgReg, executeError, &emptyResults, nil) + + assert.True(t, updated) + assert.Equal(t, status.StatusSuccess, statusType) +} diff --git a/pkg/utils/status.go b/pkg/utils/status.go index b9a3732..43b0dac 100644 --- a/pkg/utils/status.go +++ b/pkg/utils/status.go @@ -6,7 +6,7 @@ package utils import ( "encoding/json" "fmt" - "io/ioutil" + "os" "path/filepath" "github.com/Azure/azure-extension-platform/pkg/handlerenv" @@ -14,6 +14,8 @@ import ( "github.com/pkg/errors" ) +const BackupStatusFileSuffix = ".lastStableStatus" + type StatusSaveError struct { Err error } @@ -22,19 +24,29 @@ func (statusServerError *StatusSaveError) Error() string { return statusServerError.Err.Error() } -func GetStatusType(handlerEnv *handlerenv.HandlerEnvironment, sequenceNumber uint) (status.StatusType, error) { - fn := fmt.Sprintf("%d.status", sequenceNumber) - path := filepath.Join(handlerEnv.StatusFolder, fn) - statusBytes, err := ioutil.ReadFile(path) +func readStatusFileHelper(path string) (*status.Status, error) { + statusBytes, err := os.ReadFile(path) if err != nil { - return "", err + return nil, err } statusReport := make(status.StatusReport, 1) err = json.Unmarshal(statusBytes, &statusReport) if err != nil { - return "", err + return nil, err } - return statusReport[0].Status.Status, nil + return &statusReport[0].Status, nil +} + +func GetStatus(handlerEnv *handlerenv.HandlerEnvironment, sequenceNumber uint) (*status.Status, error) { + fn := fmt.Sprintf("%d.status", sequenceNumber) + path := filepath.Join(handlerEnv.StatusFolder, fn) + return readStatusFileHelper(path) +} + +func GetLastStableStatus(handlerEnv *handlerenv.HandlerEnvironment, sequenceNumber uint) (*status.Status, error) { + fn := fmt.Sprintf("%d%s", sequenceNumber, BackupStatusFileSuffix) + path := filepath.Join(handlerEnv.StatusFolder, fn) + return readStatusFileHelper(path) } func ReportStatus(handlerEnv *handlerenv.HandlerEnvironment, requestedSequenceNumber uint, statusType status.StatusType, operationName string, message string) error { @@ -47,3 +59,18 @@ func ReportStatus(handlerEnv *handlerenv.HandlerEnvironment, requestedSequenceNu } return nil } + +// BackupStatusFile renames the current status file so it can be restored later. +// If there is no existing status file, this function returns without error because +// there's nothing to back up. +func BackupStatusFile(statusFolder string, sequenceNumber uint) error { + current := filepath.Join(statusFolder, fmt.Sprintf("%d.status", sequenceNumber)) + backup := filepath.Join(statusFolder, fmt.Sprintf("%d%s", sequenceNumber, BackupStatusFileSuffix)) + if _, err := os.Stat(current); err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + return os.Rename(current, backup) +} diff --git a/pkg/utils/status_test.go b/pkg/utils/status_test.go index b45fb4e..8fa3650 100644 --- a/pkg/utils/status_test.go +++ b/pkg/utils/status_test.go @@ -4,7 +4,9 @@ package utils import ( + "os" "path" + "path/filepath" "strings" "testing" @@ -15,7 +17,40 @@ import ( func TestStatusParsing(t *testing.T) { handlerEnv := handlerenv.HandlerEnvironment{StatusFolder: path.Join(".", "testFiles")} - statusType, err := GetStatusType(&handlerEnv, 1) + statusObj, err := GetStatus(&handlerEnv, 1) require.NoError(t, err) - require.True(t, strings.EqualFold(string(statusType), string(platformstatus.StatusTransitioning))) + require.NotNil(t, statusObj) + require.True(t, strings.EqualFold(string(statusObj.Status), string(platformstatus.StatusTransitioning))) +} + +func TestBackupStatusFile(t *testing.T) { + t.Run("successful backup when status file exists", func(t *testing.T) { + // Create temp directory + tmpDir := t.TempDir() + statusFile := filepath.Join(tmpDir, "1.status") + backupFile := filepath.Join(tmpDir, "1"+BackupStatusFileSuffix) + + // Create a status file + err := os.WriteFile(statusFile, []byte(`[{"status":{"status":"success"}}]`), 0644) + require.NoError(t, err) + + // Backup the status file + err = BackupStatusFile(tmpDir, 1) + require.NoError(t, err) + + // Verify original file no longer exists + _, err = os.Stat(statusFile) + require.True(t, os.IsNotExist(err)) + + // Verify backup file exists + _, err = os.Stat(backupFile) + require.NoError(t, err) + }) + + t.Run("successful backup when status file does not exist", func(t *testing.T) { + tmpDir := t.TempDir() + + err := BackupStatusFile(tmpDir, 999) + require.NoError(t, err) + }) } From 469251adffa7c7b104815850c35204a4ee77f870 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Thu, 19 Mar 2026 09:59:21 -0400 Subject: [PATCH 08/17] Update reportStatusFunc to reportStatusWrapper --- main/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/main/main.go b/main/main.go index 131b2d3..400fbf7 100644 --- a/main/main.go +++ b/main/main.go @@ -110,7 +110,7 @@ func getExtensionAndRun(arguments []string) error { ext.ExtensionEvents.LogErrorEvent("Enable Failed", enableError.Error()) // try to save status file statusMessage := enableError.Error() - err := reportStatusFunc(ext, requestedSequenceNumber, status.StatusError, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) + err := reportStatusWrapper(ext, requestedSequenceNumber, status.StatusError, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) if err != nil { return err } @@ -225,7 +225,7 @@ func customEnable(ext *vmextensionhelper.VMExtension, hostgaCommunicator hostgac statusUpdated, statusResult, statusMessage := computeStatus(ext, requestedSequenceNumber, ¤tPackageRegistry, executeError, result, vmAppResults) if statusUpdated { - err := reportStatusFunc(ext, requestedSequenceNumber, statusResult, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) + err := reportStatusWrapper(ext, requestedSequenceNumber, statusResult, vmextensionhelper.EnableOperation.ToStatusName(), statusMessage) if err != nil { return err } @@ -325,7 +325,7 @@ func computeStatus( } // A wrapper for utils.ReportStatus to log any errors occurring in that function -func reportStatusFunc(ext *vmextensionhelper.VMExtension, requestedSequenceNumber uint, statusType status.StatusType, operationName string, message string) error { +func reportStatusWrapper(ext *vmextensionhelper.VMExtension, requestedSequenceNumber uint, statusType status.StatusType, operationName string, message string) error { err := utils.ReportStatus(ext.HandlerEnv, requestedSequenceNumber, statusType, operationName, message) if err != nil { errorMessage := fmt.Sprintf("Failed to save status file: %s", err.Error()) From e306815983874bb37e05d9279127a805e8341b41 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Thu, 19 Mar 2026 10:39:20 -0400 Subject: [PATCH 09/17] Fix statusMessage --- main/main.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/main/main.go b/main/main.go index 400fbf7..a400db8 100644 --- a/main/main.go +++ b/main/main.go @@ -275,7 +275,7 @@ func computeStatus( if err != nil { // Existing status file maybe corrupted or missing. The existing behavior is // to write a success status. - statusMessage = fmt.Sprintf("Failed to read existing status file: %v. Writing success status.", err) + statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) statusResult = status.StatusSuccess statusUpdated = true } else if strings.EqualFold(string(statusObj.Status), string(status.StatusTransitioning)) { @@ -286,7 +286,7 @@ func computeStatus( // the same sequence number, and there is no VM App to process, save the status as Success // It is also possible that a VM App cause a reboot but is not re-run again leading to // no VM App operations. We should also set the status to success in this case. - statusMessage = "No VM App operations to perform, but current status is Transitioning. Updating status to Success." + statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) statusResult = status.StatusSuccess } else { // If this is a new sequence number with no VM App operations, then report @@ -296,7 +296,7 @@ func computeStatus( prevStatusObj, prevStatusErr := utils.GetStatus(ext.HandlerEnv, *ext.CurrentSequenceNumber) if prevStatusErr != nil { // No existing status save, should record as success since there are no VM App operations - statusMessage = fmt.Sprintf("No existing status file found for sequence %d, and no VM App operations. Writing success status.", *ext.CurrentSequenceNumber) + statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) statusResult = status.StatusSuccess } else { statusMessage = prevStatusObj.FormattedMessage.Message @@ -312,7 +312,7 @@ func computeStatus( // No last stable status save, should record as success since the hostGA issue // is gone statusResult = status.StatusSuccess - statusMessage = "No last stable status found, and no VM App operations." + statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) } else { statusResult = prevStatusObj.Status statusMessage = prevStatusObj.FormattedMessage.Message From c4b717819c49813b0c5c1df47873e61c27f81caa Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Thu, 19 Mar 2026 11:22:44 -0400 Subject: [PATCH 10/17] Fix unit test --- main/main_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/main/main_test.go b/main/main_test.go index 7610b93..7792339 100644 --- a/main/main_test.go +++ b/main/main_test.go @@ -739,11 +739,10 @@ func Test_computeStatus_StatusTransitioning_FirstEnable(t *testing.T) { currentPkgReg := packageregistry.CurrentPackageRegistry{} emptyResults := actionplan.PackageOperationResults{} - updated, statusType, msg := computeStatus(ext, 1, ¤tPkgReg, executeError, &emptyResults, nil) + updated, statusType, _ := computeStatus(ext, 1, ¤tPkgReg, executeError, &emptyResults, nil) assert.True(t, updated) assert.Equal(t, status.StatusSuccess, statusType) - assert.Contains(t, msg, "No VM App operations to perform, but current status is Transitioning") } // Test computeStatus when status is Transitioning with previous sequence having a status file From cf17000d76127cf8beb0584c56c813c027fafbf7 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 20 Mar 2026 11:12:18 -0400 Subject: [PATCH 11/17] Remove check for new requested sequence without vm app update --- main/main.go | 28 ++++--------------------- main/main_test.go | 52 ----------------------------------------------- 2 files changed, 4 insertions(+), 76 deletions(-) diff --git a/main/main.go b/main/main.go index a400db8..5d71e30 100644 --- a/main/main.go +++ b/main/main.go @@ -279,30 +279,10 @@ func computeStatus( statusResult = status.StatusSuccess statusUpdated = true } else if strings.EqualFold(string(statusObj.Status), string(status.StatusTransitioning)) { - // If the status is Transitioning whenever there's a new requested sequence number, - // as the Transitioning status is saved by launcher program. - if ext.CurrentSequenceNumber == nil || requestedSequenceNumber == *ext.CurrentSequenceNumber { - // If this the very first time this extension is Enabled, or we're processing - // the same sequence number, and there is no VM App to process, save the status as Success - // It is also possible that a VM App cause a reboot but is not re-run again leading to - // no VM App operations. We should also set the status to success in this case. - statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) - statusResult = status.StatusSuccess - } else { - // If this is a new sequence number with no VM App operations, then report - // the same status as the last sequence number. - // This case can happen if the user changes the ordering inside applicationProfile - // without changing any app or their installation order. - prevStatusObj, prevStatusErr := utils.GetStatus(ext.HandlerEnv, *ext.CurrentSequenceNumber) - if prevStatusErr != nil { - // No existing status save, should record as success since there are no VM App operations - statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) - statusResult = status.StatusSuccess - } else { - statusMessage = prevStatusObj.FormattedMessage.Message - statusResult = prevStatusObj.Status - } - } + // If status is Transitioning and there's no VM App operations, + // then record a succes status. + statusMessage = getStatusMessage(currentPackageRegistry.GetPackageCollection(), executeError, customActionResult) + statusResult = status.StatusSuccess statusUpdated = true } else if strings.Contains(statusObj.FormattedMessage.Message, hostgacommunicator.HostGaMetadataErrorPrefix) { // If there is no VM App operations, but the current status is a transient host GA diff --git a/main/main_test.go b/main/main_test.go index 7792339..314e618 100644 --- a/main/main_test.go +++ b/main/main_test.go @@ -744,55 +744,3 @@ func Test_computeStatus_StatusTransitioning_FirstEnable(t *testing.T) { assert.True(t, updated) assert.Equal(t, status.StatusSuccess, statusType) } - -// Test computeStatus when status is Transitioning with previous sequence having a status file -func Test_computeStatus_StatusTransitioning_WithPreviousStatus(t *testing.T) { - setupTest(t) - ext := createTestVMExtension(t, nil) - - // Write a success status for previous sequence number (1) - err := utils.ReportStatus(ext.HandlerEnv, 1, status.StatusError, "Enable", "previous sequence failed message") - require.NoError(t, err) - - // Write a transitioning status for new sequence number (2) - err = utils.ReportStatus(ext.HandlerEnv, 2, status.StatusTransitioning, "Enable", "transitioning") - require.NoError(t, err) - - // Set CurrentSequenceNumber to 1 (previous) - seqNo := uint(1) - ext.CurrentSequenceNumber = &seqNo - - executeError := &actionplan.ExecuteError{} - currentPkgReg := packageregistry.CurrentPackageRegistry{} - emptyResults := actionplan.PackageOperationResults{} - - updated, statusType, msg := computeStatus(ext, 2, ¤tPkgReg, executeError, &emptyResults, nil) - - assert.True(t, updated) - assert.Equal(t, status.StatusError, statusType) - // The code returns the message from the previous sequence's status - assert.Contains(t, msg, "previous sequence failed message") -} - -// Test computeStatus when status is Transitioning but previous sequence status file is missing -func Test_computeStatus_StatusTransitioning_NoPreviousStatus(t *testing.T) { - setupTest(t) - ext := createTestVMExtension(t, nil) - - // Write a transitioning status for new sequence number (2) only - err := utils.ReportStatus(ext.HandlerEnv, 2, status.StatusTransitioning, "Enable", "transitioning") - require.NoError(t, err) - - // Set CurrentSequenceNumber to 1 (previous) - but no status file exists for it - seqNo := uint(1) - ext.CurrentSequenceNumber = &seqNo - - executeError := &actionplan.ExecuteError{} - currentPkgReg := packageregistry.CurrentPackageRegistry{} - emptyResults := actionplan.PackageOperationResults{} - - updated, statusType, _ := computeStatus(ext, 2, ¤tPkgReg, executeError, &emptyResults, nil) - - assert.True(t, updated) - assert.Equal(t, status.StatusSuccess, statusType) -} From e9473c08a87cd6bf252ec67fe5701b51078b669b Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 20 Mar 2026 15:56:38 -0400 Subject: [PATCH 12/17] Update to use requestedSeqNumber --- main/main.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/main/main.go b/main/main.go index 5d71e30..fa37b0a 100644 --- a/main/main.go +++ b/main/main.go @@ -285,9 +285,10 @@ func computeStatus( statusResult = status.StatusSuccess statusUpdated = true } else if strings.Contains(statusObj.FormattedMessage.Message, hostgacommunicator.HostGaMetadataErrorPrefix) { - // If there is no VM App operations, but the current status is a transient host GA - // communication error, the status should be the same as the last stable status - prevStatusObj, prevStatusErr := utils.GetLastStableStatus(ext.HandlerEnv, *ext.CurrentSequenceNumber) + // If there is no VM App operations, but the requested sequence's status is + // a transient host GA communication error, the status should be the same as + // its last stable status. + prevStatusObj, prevStatusErr := utils.GetLastStableStatus(ext.HandlerEnv, requestedSequenceNumber) if prevStatusErr != nil { // No last stable status save, should record as success since the hostGA issue // is gone From 5e90dd176186d8b2f64b2f42f75f7f895bb97c8d Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 20 Mar 2026 16:33:00 -0400 Subject: [PATCH 13/17] Add a check for when status file path is a directory too --- pkg/utils/status.go | 6 +++++- pkg/utils/status_test.go | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg/utils/status.go b/pkg/utils/status.go index 43b0dac..82617ab 100644 --- a/pkg/utils/status.go +++ b/pkg/utils/status.go @@ -66,11 +66,15 @@ func ReportStatus(handlerEnv *handlerenv.HandlerEnvironment, requestedSequenceNu func BackupStatusFile(statusFolder string, sequenceNumber uint) error { current := filepath.Join(statusFolder, fmt.Sprintf("%d.status", sequenceNumber)) backup := filepath.Join(statusFolder, fmt.Sprintf("%d%s", sequenceNumber, BackupStatusFileSuffix)) - if _, err := os.Stat(current); err != nil { + info, err := os.Stat(current) + if err != nil { if os.IsNotExist(err) { return nil } return err } + if info.IsDir() { + return fmt.Errorf("expected a file but found a directory: %s", current) + } return os.Rename(current, backup) } diff --git a/pkg/utils/status_test.go b/pkg/utils/status_test.go index 8fa3650..9367db7 100644 --- a/pkg/utils/status_test.go +++ b/pkg/utils/status_test.go @@ -53,4 +53,17 @@ func TestBackupStatusFile(t *testing.T) { err := BackupStatusFile(tmpDir, 999) require.NoError(t, err) }) + + t.Run("error when the status file path is a directory", func(t *testing.T) { + tmpDir := t.TempDir() + statusFileAsDir := filepath.Join(tmpDir, "1.status") + + // Create a directory with the same name as the status file + err := os.Mkdir(statusFileAsDir, 0755) + require.NoError(t, err) + + // Backup should fail because the path is a directory, not a file + err = BackupStatusFile(tmpDir, 1) + require.Error(t, err) + }) } From 69300b4192d4412f473caab782a4448500320600 Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 20 Mar 2026 18:06:27 -0400 Subject: [PATCH 14/17] Update tests for hostgacommunicator to use newly defined error --- internal/hostgacommunicator/hostgacommunicator_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/hostgacommunicator/hostgacommunicator_test.go b/internal/hostgacommunicator/hostgacommunicator_test.go index 0d05e13..f56ae4e 100644 --- a/internal/hostgacommunicator/hostgacommunicator_test.go +++ b/internal/hostgacommunicator/hostgacommunicator_test.go @@ -47,6 +47,9 @@ func TestGetVmAppInfo_InvalidUri(t *testing.T) { hgc := &HostGaCommunicator{} _, err := hgc.GetVMAppInfo(nopLog(), myAppName) require.NotNil(t, err, "did not fail") + _, ok := err.(*HostGaCommunicatorGetVMAppInfoError) + require.True(t, ok, "expected error to be of type *HostGaCommunicatorGetVMAppInfoError") + require.Contains(t, err.Error(), InitializationError.ToString(), "Wrong error code") require.Contains(t, err.Error(), "Could not parse the HostGA URI", "Wrong message for invalid uri") } @@ -60,6 +63,9 @@ func TestGetVmAppInfo_RequestFailed(t *testing.T) { hgc := &HostGaCommunicator{} _, err := hgc.GetVMAppInfo(nopLog(), myAppName) require.NotNil(t, err, "did not fail") + _, ok := err.(*HostGaCommunicatorGetVMAppInfoError) + require.True(t, ok, "expected error to be of type *HostGaCommunicatorGetVMAppInfoError") + require.Contains(t, err.Error(), MetadataRequestFailedWithRetries.ToString(), "Wrong error code") require.Contains(t, err.Error(), "Metadata request failed after retries:", "Wrong message for failed request") } @@ -75,6 +81,9 @@ func TestGetVmAppInfo_CouldNotDecodeResponse(t *testing.T) { hgc := &HostGaCommunicator{} _, err := hgc.GetVMAppInfo(nopLog(), myAppName) require.NotNil(t, err, "did not fail") + _, ok := err.(*HostGaCommunicatorGetVMAppInfoError) + require.True(t, ok, "expected error to be of type *HostGaCommunicatorGetVMAppInfoError") + require.Contains(t, err.Error(), MetadataRequestFailedInvalidResponseBody.ToString(), "Wrong error code") require.Contains(t, err.Error(), "Failed to decode response body:", "Wrong message for invalid response") } From b175aa490be4f6962b9bc1767f305481630a75df Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 20 Mar 2026 18:48:29 -0400 Subject: [PATCH 15/17] Create custom DownloadPackageError --- .../hostgacommunicator/hostgacommunicator.go | 28 +++++++++++++++++-- .../hostgacommunicator_test.go | 22 +++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/internal/hostgacommunicator/hostgacommunicator.go b/internal/hostgacommunicator/hostgacommunicator.go index 088c526..f10a9d5 100644 --- a/internal/hostgacommunicator/hostgacommunicator.go +++ b/internal/hostgacommunicator/hostgacommunicator.go @@ -26,6 +26,8 @@ const ( InitializationError HostGaCommunicatorError = iota MetadataRequestFailedWithRetries MetadataRequestFailedInvalidResponseBody + DownloadPackageRequestFactoryError + DownloadPackageFileError ) func (hostGaCommunicatorError HostGaCommunicatorError) ToString() string { @@ -36,6 +38,10 @@ func (hostGaCommunicatorError HostGaCommunicatorError) ToString() string { return "MetadataRequestFailedWithRetries" case MetadataRequestFailedInvalidResponseBody: return "MetadataRequestFailedInvalidResponseBody" + case DownloadPackageRequestFactoryError: + return "DownloadPackageRequestFactoryError" + case DownloadPackageFileError: + return "DownloadPackageFileError" default: return "UnknownError" } @@ -50,6 +56,15 @@ func (e *HostGaCommunicatorGetVMAppInfoError) Error() string { return fmt.Sprintf("%s: %s, error type: %s", HostGaMetadataErrorPrefix, e.errorMessage, e.errorType.ToString()) } +type DownloadPackageError struct { + errorMessage string + errorType HostGaCommunicatorError +} + +func (e *DownloadPackageError) Error() string { + return fmt.Sprintf("DownloadPackage error: %s, error type: %s", e.errorMessage, e.errorType.ToString()) +} + type IHostGaCommunicator interface { DownloadPackage(el *logging.ExtensionLogger, appName string, dst string) error DownloadConfig(el *logging.ExtensionLogger, appName string, dst string) error @@ -108,11 +123,20 @@ func (*HostGaCommunicator) GetVMAppInfo(el *logging.ExtensionLogger, appName str func (*HostGaCommunicator) DownloadPackage(el *logging.ExtensionLogger, appName string, dst string) error { requestFactory, err := newPackageDownloadRequestFactory(el, appName) if err != nil { - return errors.Wrapf(err, "Could not create the request factory") + return &DownloadPackageError{ + errorMessage: fmt.Sprintf("Could not create the request factory: %v", err), + errorType: DownloadPackageRequestFactoryError, + } } err = requestFactory.downloadFile(el, dst) - return err + if err != nil { + return &DownloadPackageError{ + errorMessage: fmt.Sprintf("Failed to download file: %v", err), + errorType: DownloadPackageFileError, + } + } + return nil } // DownloadConfig downloads the application config through HostGaPlugin to the specified diff --git a/internal/hostgacommunicator/hostgacommunicator_test.go b/internal/hostgacommunicator/hostgacommunicator_test.go index f56ae4e..9f6a1a8 100644 --- a/internal/hostgacommunicator/hostgacommunicator_test.go +++ b/internal/hostgacommunicator/hostgacommunicator_test.go @@ -164,9 +164,22 @@ func TestDownloadPackage_CannotRemoveExistingFile(t *testing.T) { hgc := &HostGaCommunicator{} err = hgc.DownloadPackage(nopLog(), myAppName, filePath) require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadPackageError) + require.True(t, ok, "expected error to be of type *DownloadPackageError") + require.Contains(t, err.Error(), "DownloadPackageFileError", "Wrong error code") require.Contains(t, err.Error(), "Could not remove the existing file", "Wrong message for failing to remove locked file") } +func TestDownloadPackage_InvalidUri(t *testing.T) { + os.Setenv(WireProtocolAddress, "htt!p:notgoingtohappen!") + hgc := &HostGaCommunicator{} + err := hgc.DownloadPackage(nopLog(), myAppName, "somepath") + require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadPackageError) + require.True(t, ok, "expected error to be of type *DownloadPackageError") + require.Contains(t, err.Error(), DownloadPackageRequestFactoryError.ToString(), "Wrong error type") +} + func TestDownloadPackage_InvalidPath(t *testing.T) { filePath := string(make([]byte, 5)) // null characters in file names are invalid in both windows and linux @@ -179,6 +192,9 @@ func TestDownloadPackage_InvalidPath(t *testing.T) { hgc := &HostGaCommunicator{} err := hgc.DownloadPackage(nopLog(), myAppName, filePath) require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadPackageError) + require.True(t, ok, "expected error to be of type *DownloadPackageError") + require.Contains(t, err.Error(), "DownloadPackageFileError", "Wrong error code") require.Contains(t, err.Error(), "Cannot retrieve file information", "Wrong message for invalid file path") } @@ -229,6 +245,9 @@ func TestDownloadPackage_TooManyTries(t *testing.T) { hgc := &HostGaCommunicator{} err := hgc.DownloadPackage(nopLog(), myAppName, filePath) require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadPackageError) + require.True(t, ok, "expected error to be of type *DownloadPackageError") + require.Contains(t, err.Error(), "DownloadPackageFileError", "Wrong error code") require.Contains(t, err.Error(), "Failed to completely download the file", "Wrong message for incomplete file") } @@ -258,6 +277,9 @@ func TestDownloadPackage_IntermediateCallFails(t *testing.T) { hgc := &HostGaCommunicator{} err := hgc.DownloadPackage(nopLog(), myAppName, filePath) require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadPackageError) + require.True(t, ok, "expected error to be of type *DownloadPackageError") + require.Contains(t, err.Error(), "DownloadPackageFileError", "Wrong error code") require.Contains(t, err.Error(), "Unrecoverable error while downloading the file", "Wrong message for failure mid-retries") } From 9729754cc710eff8dc0f1662d29e48f3fc8fc8ff Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Fri, 20 Mar 2026 19:14:49 -0400 Subject: [PATCH 16/17] Added custom error DownloadConfigError --- .../hostgacommunicator/hostgacommunicator.go | 28 +++++++++++++++++-- .../hostgacommunicator_test.go | 28 +++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/internal/hostgacommunicator/hostgacommunicator.go b/internal/hostgacommunicator/hostgacommunicator.go index f10a9d5..8940392 100644 --- a/internal/hostgacommunicator/hostgacommunicator.go +++ b/internal/hostgacommunicator/hostgacommunicator.go @@ -28,6 +28,8 @@ const ( MetadataRequestFailedInvalidResponseBody DownloadPackageRequestFactoryError DownloadPackageFileError + DownloadConfigRequestFactoryError + DownloadConfigFileError ) func (hostGaCommunicatorError HostGaCommunicatorError) ToString() string { @@ -42,6 +44,10 @@ func (hostGaCommunicatorError HostGaCommunicatorError) ToString() string { return "DownloadPackageRequestFactoryError" case DownloadPackageFileError: return "DownloadPackageFileError" + case DownloadConfigRequestFactoryError: + return "DownloadConfigRequestFactoryError" + case DownloadConfigFileError: + return "DownloadConfigFileError" default: return "UnknownError" } @@ -65,6 +71,15 @@ func (e *DownloadPackageError) Error() string { return fmt.Sprintf("DownloadPackage error: %s, error type: %s", e.errorMessage, e.errorType.ToString()) } +type DownloadConfigError struct { + errorMessage string + errorType HostGaCommunicatorError +} + +func (e *DownloadConfigError) Error() string { + return fmt.Sprintf("DownloadConfig error: %s, error type: %s", e.errorMessage, e.errorType.ToString()) +} + type IHostGaCommunicator interface { DownloadPackage(el *logging.ExtensionLogger, appName string, dst string) error DownloadConfig(el *logging.ExtensionLogger, appName string, dst string) error @@ -145,11 +160,20 @@ func (*HostGaCommunicator) DownloadPackage(el *logging.ExtensionLogger, appName func (*HostGaCommunicator) DownloadConfig(el *logging.ExtensionLogger, appName string, dst string) error { requestFactory, err := newConfigDownloadRequestFactory(el, appName) if err != nil { - return errors.Wrapf(err, "Could not create the request factory") + return &DownloadConfigError{ + errorMessage: fmt.Sprintf("Could not create the request factory: %v", err), + errorType: DownloadConfigRequestFactoryError, + } } err = requestFactory.downloadFile(el, dst) - return err + if err != nil { + return &DownloadConfigError{ + errorMessage: fmt.Sprintf("Failed to download file: %v", err), + errorType: DownloadConfigFileError, + } + } + return nil } func getOperationURI(el *logging.ExtensionLogger, appName string, operation string) (string, error) { diff --git a/internal/hostgacommunicator/hostgacommunicator_test.go b/internal/hostgacommunicator/hostgacommunicator_test.go index 9f6a1a8..900bce6 100644 --- a/internal/hostgacommunicator/hostgacommunicator_test.go +++ b/internal/hostgacommunicator/hostgacommunicator_test.go @@ -319,6 +319,34 @@ func TestDownloadPackage_MultipleCallDownload(t *testing.T) { verifyFileContents(t, filePath, expected) } +func TestDownloadConfig_InvalidUri(t *testing.T) { + os.Setenv(WireProtocolAddress, "htt!p:notgoingtohappen!") + hgc := &HostGaCommunicator{} + err := hgc.DownloadConfig(nopLog(), myAppName, "somepath") + require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadConfigError) + require.True(t, ok, "expected error to be of type *DownloadConfigError") + require.Contains(t, err.Error(), DownloadConfigRequestFactoryError.ToString(), "Wrong error code") +} + +func TestDownloadConfig_InvalidPath(t *testing.T) { + filePath := string(make([]byte, 5)) // null characters in file names are invalid in both windows and linux + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + os.Setenv(WireProtocolAddress, srv.URL) + hgc := &HostGaCommunicator{} + err := hgc.DownloadConfig(nopLog(), myAppName, filePath) + require.NotNil(t, err, "did not fail") + _, ok := err.(*DownloadConfigError) + require.True(t, ok, "expected error to be of type *DownloadConfigError") + require.Contains(t, err.Error(), DownloadConfigFileError.ToString(), "Wrong error code") + require.Contains(t, err.Error(), "Cannot retrieve file information", "Wrong message for invalid file path") +} + func TestDownloadConfig_SingeCallDownload(t *testing.T) { expected := "file contents don't matter" createTestDir(t) From f7786fc5f6feaa82578be1c0b97019f824e9648c Mon Sep 17 00:00:00 2001 From: Haley Bui-Nguyen Date: Thu, 26 Mar 2026 20:42:49 -0400 Subject: [PATCH 17/17] Replace the Rename with Copy when backing up status file --- pkg/utils/status.go | 40 +++++++++++++++++++++++++++++++++++++++- pkg/utils/status_test.go | 4 ++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/pkg/utils/status.go b/pkg/utils/status.go index 82617ab..b0960da 100644 --- a/pkg/utils/status.go +++ b/pkg/utils/status.go @@ -6,6 +6,7 @@ package utils import ( "encoding/json" "fmt" + "io" "os" "path/filepath" @@ -60,6 +61,43 @@ func ReportStatus(handlerEnv *handlerenv.HandlerEnvironment, requestedSequenceNu return nil } +// copyFile copies a file from src to dst. If dst already exists, it will be overwritten. +// The file permissions of the destination file will be the same as the source file. +func copyFile(src, dst string) error { + // Open the source file + sourceFile, err := os.Open(src) + if err != nil { + return fmt.Errorf("failed to open source file: %w", err) + } + defer sourceFile.Close() // Get source file info + + sourceInfo, err := sourceFile.Stat() + if err != nil { + return fmt.Errorf("failed to stat source file: %w", err) + } + + // Create the destination file with same mode + destinationFile, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, sourceInfo.Mode()) + if err != nil { + return fmt.Errorf("failed to create destination file: %w", err) + } + defer destinationFile.Close() + + // Copy the content + _, err = io.Copy(destinationFile, sourceFile) + if err != nil { + return fmt.Errorf("failed to copy file: %w", err) + } + + // Flush file metadata to disk + err = destinationFile.Sync() + if err != nil { + return fmt.Errorf("failed to sync destination file: %w", err) + } + + return nil +} + // BackupStatusFile renames the current status file so it can be restored later. // If there is no existing status file, this function returns without error because // there's nothing to back up. @@ -76,5 +114,5 @@ func BackupStatusFile(statusFolder string, sequenceNumber uint) error { if info.IsDir() { return fmt.Errorf("expected a file but found a directory: %s", current) } - return os.Rename(current, backup) + return copyFile(current, backup) } diff --git a/pkg/utils/status_test.go b/pkg/utils/status_test.go index 9367db7..ad1d90f 100644 --- a/pkg/utils/status_test.go +++ b/pkg/utils/status_test.go @@ -38,9 +38,9 @@ func TestBackupStatusFile(t *testing.T) { err = BackupStatusFile(tmpDir, 1) require.NoError(t, err) - // Verify original file no longer exists + // Verify original file is still there (to be overwritten later) _, err = os.Stat(statusFile) - require.True(t, os.IsNotExist(err)) + require.NoError(t, err) // Verify backup file exists _, err = os.Stat(backupFile)