Skip to content

DRAFT: feature: enable manual rollback #381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
bfjelds wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

DRAFT: feature: enable manual rollback #381

bfjelds wants to merge 31 commits into from

Conversation

@bfjelds
Copy link
Member

@bfjelds bfjelds commented Dec 5, 2025

🔍 Description

Enable trident rollback to undo the last update, with options:

  • --ab: user expects rollback will undo an a/b update, trident return error otherwise
  • --runtime: user expects rollback will undo a runtime update, trident return error otherwise
  • --show validation|target|chain: list rollback details

@bfjelds bfjelds mentioned this pull request Dec 5, 2025
Closed
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
bfjelds
bfjelds commented Dec 5, 2025
View reviewed changes
@bfjelds
Copy link
Member Author

bfjelds commented Dec 5, 2025

/AzurePipelines run [GITHUB]-trident-pr-e2e

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@bfjelds
Copy link
Member Author

bfjelds commented Dec 5, 2025

/AzurePipelines run [GITHUB]-trident-pr-e2e

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 5, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@bfjelds
Copy link
Member Author

bfjelds commented Dec 6, 2025

/AzurePipelines run [GITHUB]-trident-pr-e2e

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 6, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@bfjelds
Copy link
Member Author

bfjelds commented Dec 6, 2025

/AzurePipelines run [GITHUB]-trident-pr-e2e

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 6, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@bfjelds bfjelds force-pushed the user/bfjelds/manual-rollback branch from 5fc0ede to 08fe5bd Compare December 11, 2025 22:37
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 11, 2025
View reviewed changes
tools/storm/rollback/tests/rollback.go
return fmt.Errorf("failed to write host config file locally: %w", err)
}
logrus.Tracef("Putting updated Host Configuration on VM")
logrus.Tracef("Putting updated Host Configuration on VM:\n%s", string(hostConfigBytes))

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High test

Sensitive data returned by an access to DebugPassword
Loading
flows to a logging call.

Copilot Autofix

AI 4 days ago

The recommended fix is to prevent sensitive information—specifically, the password in testConfig.DebugPassword—from being included in logs. In this code, logging the full YAML host configuration (hostConfigBytes) will expose the password if DebugPassword is set.

To fix the issue:

  • Redact or remove the password-containing field(s) from the copy being logged, or avoid logging the full config altogether.
  • It's common and safe to create a copy of the hostConfig map and redact any sensitive fields (in this case, the password set in the scripts->postProvision->content path) before marshalling and logging.
  • This copy can then be marshaled and logged, while the original (with the sensitive data) continues to be used for all functional purposes.

To do this safely:

  • Clone the hostConfig structure, recursively if needed.
  • Redact or mask ("***REDACTED***") the password in the appropriate field.
  • Marshal the redacted copy and log only that.

Required changes:

  • Update the logging logic on line 335 to perform the redacting before logging.
  • Add a helper function to create a redacted copy of the hostConfig for logging purposes.
  • No new dependencies are needed.
Suggested changeset 1
tools/storm/rollback/tests/rollback.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/tools/storm/rollback/tests/rollback.go b/tools/storm/rollback/tests/rollback.go
--- a/tools/storm/rollback/tests/rollback.go
+++ b/tools/storm/rollback/tests/rollback.go
@@ -305,6 +305,101 @@
 	return validateRollbacksAvailable(testConfig, vmConfig, vmIP, expectedAvailableRollbacks, expectedFirstRollbackNeedsReboot)
 }
 
+// redactHostConfigForLogging returns a copy of the hostConfig with sensitive fields (like DebugPassword) redacted.
+func redactHostConfigForLogging(hostConfig map[string]interface{}) map[string]interface{} {
+	// Deep copy the map
+	redacted := make(map[string]interface{})
+	for k, v := range hostConfig {
+		redacted[k] = deepCopyWithRedaction(k, v)
+	}
+	return redacted
+}
+
+// Helper function to deep copy and redact as needed
+func deepCopyWithRedaction(key string, value interface{}) interface{} {
+	if key == "scripts" {
+		// Redact passwords in postProvision scripts
+		if scriptsMap, ok := value.(map[string]interface{}); ok {
+			newScriptsMap := make(map[string]interface{})
+			for sk, sv := range scriptsMap {
+				if sk == "postProvision" {
+					if postProvisionArr, okArr := sv.([]map[string]interface{}); okArr {
+						newArr := []map[string]interface{}{}
+						for _, elem := range postProvisionArr {
+							newElem := make(map[string]interface{})
+							for ek, ev := range elem {
+								// Redact password in the "content" field of "set-password-script"
+								if ek == "name" && ev == "set-password-script" {
+									newElem[ek] = ev
+									// Copy all fields, but redact "content"
+									for nk, nv := range elem {
+										if nk == "content" {
+											newElem[nk] = "***REDACTED***"
+										} else {
+											newElem[nk] = nv
+										}
+									}
+									break
+								}
+								newElem[ek] = ev
+							}
+							newArr = append(newArr, newElem)
+						}
+						newScriptsMap[sk] = newArr
+					} else if postProvisionArr, okArr2 := sv.([]interface{}); okArr2 {
+						newArr := []interface{}{}
+						for _, elemIf := range postProvisionArr {
+							if elem, okElem := elemIf.(map[string]interface{}); okElem {
+								newElem := make(map[string]interface{})
+								for ek, ev := range elem {
+									if ek == "name" && ev == "set-password-script" {
+										newElem[ek] = ev
+										for nk, nv := range elem {
+											if nk == "content" {
+												newElem[nk] = "***REDACTED***"
+											} else {
+												newElem[nk] = nv
+											}
+										}
+										break
+									}
+									newElem[ek] = ev
+								}
+								newArr = append(newArr, newElem)
+							} else {
+								newArr = append(newArr, elemIf)
+							}
+						}
+						newScriptsMap[sk] = newArr
+					} else {
+						newScriptsMap[sk] = sv
+					}
+				} else {
+					newScriptsMap[sk] = sv
+				}
+			}
+			return newScriptsMap
+		}
+	}
+	// Deep copy for nested maps
+	if m, ok := value.(map[string]interface{}); ok {
+		newMap := make(map[string]interface{})
+		for nk, nv := range m {
+			newMap[nk] = deepCopyWithRedaction(nk, nv)
+		}
+		return newMap
+	}
+	// Deep copy for nested slices
+	if arr, ok := value.([]interface{}); ok {
+		newArr := make([]interface{}, len(arr))
+		for i, v := range arr {
+			newArr[i] = deepCopyWithRedaction("", v)
+		}
+		return newArr
+	}
+	return value
+}
+
 func doUpdateTest(
 	testConfig stormrollbackconfig.TestConfig,
 	vmConfig stormvmconfig.AllVMConfig,
@@ -332,7 +427,13 @@
 	if err != nil {
 		return fmt.Errorf("failed to write host config file locally: %w", err)
 	}
-	logrus.Tracef("Putting updated Host Configuration on VM:\n%s", string(hostConfigBytes))
+	// Redact sensitive fields from the hostConfig before logging
+	redactedHostConfig := redactHostConfigForLogging(hostConfig)
+	redactedHostConfigBytes, err := yaml.Marshal(redactedHostConfig)
+	if err != nil {
+		return fmt.Errorf("failed to marshal redacted Host Configuration: %w", err)
+	}
+	logrus.Tracef("Putting updated Host Configuration on VM:\n%s", string(redactedHostConfigBytes))
 	err = stormssh.ScpUploadFile(vmConfig.VMConfig, vmIP, localTmpFile.Name(), vmHostConfigPath)
 	if err != nil {
 		return fmt.Errorf("failed to put updated Host Configuration on VM (%w)", err)
EOF
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bfjelds