From b86c294771d50ab238f96fbcd0c7cef759fac998 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Thu, 30 Apr 2026 22:15:29 +0200 Subject: [PATCH] fix(agent): merge bootstrap-config.json on --install instead of clobbering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from the TenantIdAwaiter PR review: * HIGH: A redeploy that passed only --tenant-id-wait (PS1's default invocation today) was overwriting bootstrap-config.json with BootstrapToken=null / TenantId=null, pushing devices to cert-auth before their MDM cert was usable. The install path now reads the existing file, merges only fields explicitly given on this invocation, and writes the merged result back. Field-given booleans drive the merge so that prior values survive an absent CLI flag. * MEDIUM: --tenant-id-wait 0 is now a real opt-out. Previously the persist block was gated on tenantIdWaitSeconds > 0, so an admin who flipped the PS1 default to 0 and redeployed kept the old non-zero wait persisted. The gate is now "any flag explicitly set" and the merge writes 0 over the prior value when 0 is what was given. * LOW: --tenant-id-wait is now in --help under a new "TenantId resolution" section so the start-reliability switch is discoverable. The merge logic is extracted as Program.MergeBootstrapConfig — a pure function — and covered by 7 InstallModeMergeTests cases (wait-only preserve, explicit-zero override, tenant-only preserve, token-only preserve, first-install path, PS1 default invocation, legacy existing-config without the new field). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Program/InstallModeMergeTests.cs | 158 ++++++++++++++++++ .../Program.InstallMode.cs | 81 +++++++-- .../AutopilotMonitor.Agent.V2/Program.cs | 7 + .../buildcounter.txt | 2 +- .../AutopilotMonitor.Agent/buildcounter.txt | 2 +- 5 files changed, 235 insertions(+), 15 deletions(-) create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/InstallModeMergeTests.cs diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/InstallModeMergeTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/InstallModeMergeTests.cs new file mode 100644 index 00000000..d4ef44f1 --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/InstallModeMergeTests.cs @@ -0,0 +1,158 @@ +using AutopilotMonitor.Agent.V2; +using AutopilotMonitor.Agent.V2.Core.Configuration; +using Xunit; + +namespace AutopilotMonitor.Agent.V2.Core.Tests.Program +{ + /// + /// Pinpoints the --install bootstrap-config merge semantics. The earlier + /// implementation overwrote the file from CLI args alone, so a redeploy with only + /// --tenant-id-wait wiped a previously-persisted BootstrapToken / TenantId + /// and pushed devices to cert-auth before their cert was usable. The merge now + /// preserves prior fields when their corresponding CLI flag is absent on this + /// invocation, and honours an explicit value (including empty / 0) when present. + /// + public sealed class InstallModeMergeTests + { + [Fact] + public void Wait_only_redeploy_preserves_existing_token_and_tenantid() + { + var existing = new BootstrapConfigFile + { + BootstrapToken = "tok-abc", + TenantId = "tenant-xyz", + TenantIdWaitSeconds = 0, + }; + + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing, + bootstrapTokenArg: null!, bootstrapTokenGiven: false, + tenantIdArg: null!, tenantIdGiven: false, + tenantIdWaitSeconds: 600, tenantIdWaitGiven: true); + + Assert.Equal("tok-abc", merged.BootstrapToken); + Assert.Equal("tenant-xyz", merged.TenantId); + Assert.Equal(600, merged.TenantIdWaitSeconds); + } + + [Fact] + public void Explicit_zero_wait_overrides_existing_nonzero() + { + var existing = new BootstrapConfigFile + { + BootstrapToken = "tok-abc", + TenantId = "tenant-xyz", + TenantIdWaitSeconds = 600, + }; + + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing, + bootstrapTokenArg: null!, bootstrapTokenGiven: false, + tenantIdArg: null!, tenantIdGiven: false, + tenantIdWaitSeconds: 0, tenantIdWaitGiven: true); + + // Token + tenantId untouched, wait explicitly disabled. + Assert.Equal("tok-abc", merged.BootstrapToken); + Assert.Equal("tenant-xyz", merged.TenantId); + Assert.Equal(0, merged.TenantIdWaitSeconds); + } + + [Fact] + public void Tenant_only_redeploy_preserves_existing_wait() + { + var existing = new BootstrapConfigFile + { + BootstrapToken = "tok-abc", + TenantId = "tenant-old", + TenantIdWaitSeconds = 1500, + }; + + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing, + bootstrapTokenArg: null!, bootstrapTokenGiven: false, + tenantIdArg: "tenant-new", tenantIdGiven: true, + tenantIdWaitSeconds: 0, tenantIdWaitGiven: false); + + Assert.Equal("tok-abc", merged.BootstrapToken); + Assert.Equal("tenant-new", merged.TenantId); + Assert.Equal(1500, merged.TenantIdWaitSeconds); + } + + [Fact] + public void Token_only_redeploy_preserves_existing_wait_and_tenant() + { + var existing = new BootstrapConfigFile + { + BootstrapToken = "tok-old", + TenantId = "tenant-xyz", + TenantIdWaitSeconds = 300, + }; + + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing, + bootstrapTokenArg: "tok-new", bootstrapTokenGiven: true, + tenantIdArg: null!, tenantIdGiven: false, + tenantIdWaitSeconds: 0, tenantIdWaitGiven: false); + + Assert.Equal("tok-new", merged.BootstrapToken); + Assert.Equal("tenant-xyz", merged.TenantId); + Assert.Equal(300, merged.TenantIdWaitSeconds); + } + + [Fact] + public void First_install_with_no_existing_config_uses_args_only() + { + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing: null, + bootstrapTokenArg: "tok-fresh", bootstrapTokenGiven: true, + tenantIdArg: "tenant-fresh", tenantIdGiven: true, + tenantIdWaitSeconds: 600, tenantIdWaitGiven: true); + + Assert.Equal("tok-fresh", merged.BootstrapToken); + Assert.Equal("tenant-fresh", merged.TenantId); + Assert.Equal(600, merged.TenantIdWaitSeconds); + } + + [Fact] + public void First_install_with_only_wait_arg_writes_null_token_and_tenant() + { + // PS1's default invocation: agent --install --tenant-id-wait 600 + // No existing config, no token, no tenantId. Result: nullable fields stay null, + // wait gets the new value. Subsequent runtime config-load falls back to the + // registry resolver for TenantId — that's the OOBE happy path. + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing: null, + bootstrapTokenArg: null!, bootstrapTokenGiven: false, + tenantIdArg: null!, tenantIdGiven: false, + tenantIdWaitSeconds: 600, tenantIdWaitGiven: true); + + Assert.Null(merged.BootstrapToken); + Assert.Null(merged.TenantId); + Assert.Equal(600, merged.TenantIdWaitSeconds); + } + + [Fact] + public void Existing_config_without_wait_field_treated_as_zero() + { + // BackCompat: pre-fix bootstrap-config.json has no TenantIdWaitSeconds field; + // Newtonsoft deserialises that to 0. New install with --tenant-id-wait 600 + // overrides cleanly; no extra migration logic required. + var existing = new BootstrapConfigFile + { + BootstrapToken = "tok-legacy", + TenantId = "tenant-legacy", + TenantIdWaitSeconds = 0, + }; + + var merged = AutopilotMonitor.Agent.V2.Program.MergeBootstrapConfig( + existing, + bootstrapTokenArg: null!, bootstrapTokenGiven: false, + tenantIdArg: null!, tenantIdGiven: false, + tenantIdWaitSeconds: 600, tenantIdWaitGiven: true); + + Assert.Equal("tok-legacy", merged.BootstrapToken); + Assert.Equal("tenant-legacy", merged.TenantId); + Assert.Equal(600, merged.TenantIdWaitSeconds); + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs b/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs index 7ed20878..9885386b 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs @@ -65,35 +65,66 @@ namespace AutopilotMonitor.Agent.V2 // Persist bootstrap config if any of --bootstrap-token / --tenant-id / // --tenant-id-wait was provided. The Scheduled Task command line has no args — // the agent picks the persisted values up on the first post-install run. + // + // Read-merge-write: only fields explicitly set on this install invocation + // overwrite the persisted file. This protects against a redeploy that passes + // only --tenant-id-wait clobbering an existing BootstrapToken / TenantId, and + // lets an admin opt out of the wait by passing --tenant-id-wait 0 explicitly + // (which writes 0 over an old non-zero value). var bootstrapTokenArg = GetArgValue(args, "--bootstrap-token"); var tenantIdArg = GetArgValue(args, "--tenant-id"); var tenantIdWaitArg = GetArgValue(args, "--tenant-id-wait"); + + bool bootstrapTokenGiven = !string.IsNullOrEmpty(bootstrapTokenArg); + bool tenantIdGiven = !string.IsNullOrEmpty(tenantIdArg); + bool tenantIdWaitGiven = !string.IsNullOrEmpty(tenantIdWaitArg); + int tenantIdWaitSeconds = 0; - if (!string.IsNullOrEmpty(tenantIdWaitArg) && !int.TryParse(tenantIdWaitArg, out tenantIdWaitSeconds)) + if (tenantIdWaitGiven && !int.TryParse(tenantIdWaitArg, out tenantIdWaitSeconds)) { logger.Warning($"Install: --tenant-id-wait '{tenantIdWaitArg}' is not a valid integer — ignoring."); + tenantIdWaitGiven = false; tenantIdWaitSeconds = 0; } - if (!string.IsNullOrEmpty(bootstrapTokenArg) || - !string.IsNullOrEmpty(tenantIdArg) || - tenantIdWaitSeconds > 0) + if (bootstrapTokenGiven || tenantIdGiven || tenantIdWaitGiven) { var bootstrapConfigPath = Path.Combine( Environment.ExpandEnvironmentVariables(Constants.AgentDataDirectory), BootstrapConfigFileName); - var bootstrapConfig = new BootstrapConfigFile + + BootstrapConfigFile existing = null; + if (File.Exists(bootstrapConfigPath)) { - BootstrapToken = bootstrapTokenArg, - TenantId = tenantIdArg, - TenantIdWaitSeconds = tenantIdWaitSeconds, - }; - File.WriteAllText(bootstrapConfigPath, JsonConvert.SerializeObject(bootstrapConfig)); + try + { + existing = JsonConvert.DeserializeObject( + File.ReadAllText(bootstrapConfigPath)); + } + catch (Exception ex) + { + logger.Warning( + $"Existing bootstrap-config.json could not be parsed " + + $"({ex.GetType().Name}: {ex.Message}) — overwriting fresh."); + } + } + + var merged = MergeBootstrapConfig( + existing, + bootstrapTokenArg, bootstrapTokenGiven, + tenantIdArg, tenantIdGiven, + tenantIdWaitSeconds, tenantIdWaitGiven); + + File.WriteAllText(bootstrapConfigPath, JsonConvert.SerializeObject(merged)); logger.Info( $"Bootstrap config persisted for Scheduled Task " + - $"(token={(string.IsNullOrEmpty(bootstrapTokenArg) ? "no" : "yes")}, " + - $"tenantId={(string.IsNullOrEmpty(tenantIdArg) ? "no" : "yes")}, " + - $"tenantIdWait={tenantIdWaitSeconds}s)."); + $"(token={(string.IsNullOrEmpty(merged.BootstrapToken) ? "no" : "yes")}" + + $"{(bootstrapTokenGiven ? "*" : "")}, " + + $"tenantId={(string.IsNullOrEmpty(merged.TenantId) ? "no" : "yes")}" + + $"{(tenantIdGiven ? "*" : "")}, " + + $"tenantIdWait={merged.TenantIdWaitSeconds}s" + + $"{(tenantIdWaitGiven ? "*" : "")})." + + " Star = explicitly set on this --install invocation, others were preserved from prior config."); } // Persist await-enrollment config if requested. @@ -260,6 +291,30 @@ namespace AutopilotMonitor.Agent.V2 logger.Info($"Payload deployment completed: '{sourceDir}' -> '{targetDir}'"); } + /// + /// Pure-function merge for bootstrap-config.json. Each ...Given flag tells + /// us whether the corresponding CLI arg was explicitly set on this --install + /// invocation. When given, the new value wins (including explicit zero / empty) — this + /// is what makes --tenant-id-wait 0 a real opt-out. When not given, the prior + /// persisted value is preserved — this is what stops a redeploy that only carries + /// --tenant-id-wait from clobbering a previously-set BootstrapToken / TenantId. + /// + internal static BootstrapConfigFile MergeBootstrapConfig( + BootstrapConfigFile existing, + string bootstrapTokenArg, bool bootstrapTokenGiven, + string tenantIdArg, bool tenantIdGiven, + int tenantIdWaitSeconds, bool tenantIdWaitGiven) + { + return new BootstrapConfigFile + { + BootstrapToken = bootstrapTokenGiven ? bootstrapTokenArg : existing?.BootstrapToken, + TenantId = tenantIdGiven ? tenantIdArg : existing?.TenantId, + TenantIdWaitSeconds = tenantIdWaitGiven + ? tenantIdWaitSeconds + : (existing?.TenantIdWaitSeconds ?? 0), + }; + } + private static void TryWriteDeploymentMarker(AgentLogger logger) { try diff --git a/src/Agent/AutopilotMonitor.Agent.V2/Program.cs b/src/Agent/AutopilotMonitor.Agent.V2/Program.cs index 0e81067d..ee64a9af 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/Program.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2/Program.cs @@ -415,6 +415,13 @@ namespace AutopilotMonitor.Agent.V2 Console.Out.WriteLine(" --await-enrollment Wait for MDM certificate before starting monitoring"); Console.Out.WriteLine(" --await-enrollment-timeout Timeout in minutes (default: 480)"); Console.Out.WriteLine(); + Console.Out.WriteLine("TenantId resolution:"); + Console.Out.WriteLine(" --tenant-id-wait On miss, wait up to N seconds for the registry to"); + Console.Out.WriteLine(" populate (RegistryWatcher on Enrollments + CloudDomainJoin)."); + Console.Out.WriteLine(" 0 = disabled (legacy fast-fail). At --install time the value"); + Console.Out.WriteLine(" is persisted to bootstrap-config.json so the Scheduled Task"); + Console.Out.WriteLine(" picks it up on each run."); + Console.Out.WriteLine(); Console.Out.WriteLine("Overrides:"); Console.Out.WriteLine(" --api-url Override backend API base URL (alias: --backend-api)"); Console.Out.WriteLine(" --ime-log-path Override IME logs directory"); diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index 1c599d4f..9a26b94d 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -516 +518 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index 939b0cbc..81c545ef 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1233 +1234