From 22026c284a54dae5a7ea24cb72c463dee263adfd Mon Sep 17 00:00:00 2001 From: Scott Merchant <97430840+scme0@users.noreply.github.com> Date: Thu, 16 Nov 2023 18:21:04 +1030 Subject: [PATCH] Ensure k8s reregistration is correctly (#802) * add tests * add implementation * Make it anonymous * Make it obvious why we are checking if id != null --- ...gisterKubernetesClusterOperationFixture.cs | 149 ++++++++++++++++++ .../RegisterKubernetesClusterOperation.cs | 6 + .../Operations/RegisterMachineOperation.cs | 75 ++++++--- 3 files changed, 206 insertions(+), 24 deletions(-) diff --git a/source/Octopus.Client.Tests/Operations/RegisterKubernetesClusterOperationFixture.cs b/source/Octopus.Client.Tests/Operations/RegisterKubernetesClusterOperationFixture.cs index e2550c16d..3d7c34689 100644 --- a/source/Octopus.Client.Tests/Operations/RegisterKubernetesClusterOperationFixture.cs +++ b/source/Octopus.Client.Tests/Operations/RegisterKubernetesClusterOperationFixture.cs @@ -1,10 +1,12 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading; using System.Threading.Tasks; using FluentAssertions; using NSubstitute; using NUnit.Framework; +using Octopus.Client.Exceptions; using Octopus.Client.Extensibility; using Octopus.Client.Model; using Octopus.Client.Model.Endpoints; @@ -22,6 +24,7 @@ public class RegisterKubernetesClusterOperationFixture ResourceCollection environments; ResourceCollection machines; ResourceCollection machinePolicies; + private ResourceCollection proxies; [SetUp] public void SetUp() @@ -32,6 +35,7 @@ public void SetUp() operation = new RegisterKubernetesClusterOperation(clientFactory); serverEndpoint = new OctopusServerEndpoint("http://octopus", "ABC123"); + proxies = new ResourceCollection(new ProxyResource[0], LinkCollection.Self("/foo")); environments = new ResourceCollection(new EnvironmentResource[0], LinkCollection.Self("/foo")); machines = new ResourceCollection(new MachineResource[0], LinkCollection.Self("/foo")); machinePolicies = new ResourceCollection(new MachinePolicyResource[0], LinkCollection.Self("/foo")); @@ -45,6 +49,7 @@ public void SetUp() .Add("MachinePolicies", "/api/machinepolicies") .Add("CurrentUser", "/api/users/me") .Add("SpaceHome", "/api/spaces") + .Add("Proxies", "/api/proxies") }; client.Get(Arg.Any(), Arg.Any()).Returns(rootDocument); @@ -57,6 +62,8 @@ public void SetUp() .Do(ci => ci.Arg, bool>>()(machines)); client.When(x => x.Paginate(Arg.Any(), Arg.Any(), Arg.Any, bool>>(), Arg.Any())) .Do(ci => ci.Arg, bool>>()(machinePolicies)); + client.When(x => x.Paginate(Arg.Any(), Arg.Any(), Arg.Any, bool>>(), Arg.Any())) + .Do(ci => ci.Arg, bool>>()(proxies)); } [Test] @@ -101,5 +108,147 @@ public async Task ShouldCreateNewPollingKubernetesTentacle() machineResources.Should().ContainSingle().Which.Endpoint.Should().BeOfType().Which.Should().BeEquivalentTo(new KubernetesTentacleEndpointResource(new PollingTentacleEndpointConfigurationResource("ABCDEF", "poll://ckyhfyfkcbmzjl8sfgch/"))); } + + [Test] + public async Task ShouldOnlyUpdateEndpointConnectionConfigurationOnExistingMachine() + { + var updatedMachines = new List(); + await client.Update("/machines/whatever/1", Arg.Do(m => updatedMachines.Add(m)), Arg.Any(), Arg.Any()); + + environments.Items.Add(new EnvironmentResource {Id = "environments-1", Name = "UAT", Links = LinkCollection.Self("/api/environments/environments-1").Add("Machines", "/api/environments/environments-1/machines")}); + environments.Items.Add(new EnvironmentResource {Id = "environments-2", Name = "Production", Links = LinkCollection.Self("/api/environments/environments-2").Add("Machines", "/api/environments/environments-2/machines")}); + + machines.Items.Add(new MachineResource + { + Id = "machines/84", EnvironmentIds = new ReferenceCollection(new[] { "environments-1" }), Name = "Mymachine", Links = LinkCollection.Self("/machines/whatever/1"), + Endpoint = new KubernetesTentacleEndpointResource(new ListeningTentacleEndpointConfigurationResource("123456", "myMachine.test.com") { ProxyId = "proxy-2" }) + }); + + proxies.Items.Add(new ProxyResource { Id = "proxy-1", Name = "MyNewProxy", Links = LinkCollection.Self("/api/proxies/proxy-1") }); + + operation.MachineName = "Mymachine"; + operation.TentaclePort = 10930; + operation.TentacleThumbprint = "ABCDEF"; + operation.TentacleHostname = "my-new-machine.test.com"; + operation.CommunicationStyle = CommunicationStyle.TentaclePassive; + operation.EnvironmentNames = new[] {"Production"}; + operation.ProxyName = "MyNewProxy"; + + await operation.ExecuteAsync(serverEndpoint).ConfigureAwait(false); + + var thing = updatedMachines.Should().ContainSingle().Which; + + thing.Should().BeEquivalentTo(new + { + Id = "machines/84", + Name = "Mymachine", + EnvironmentIds = new ReferenceCollection("environments-1"), + Endpoint = new KubernetesTentacleEndpointResource(new ListeningTentacleEndpointConfigurationResource("ABCDEF", "https://my-new-machine.test.com:10930/") {ProxyId = "proxy-1"}), + Links = LinkCollection.Self("/machines/whatever/1") + }); + } + + [Test] + public async Task ShouldUpdateExistingMachineWhenForceIsEnabled() + { + environments.Items.Add(new EnvironmentResource {Id = "environments-1", Name = "UAT", Links = LinkCollection.Self("/api/environments/environments-1").Add("Machines", "/api/environments/environments-1/machines")}); + environments.Items.Add(new EnvironmentResource {Id = "environments-2", Name = "Production", Links = LinkCollection.Self("/api/environments/environments-2").Add("Machines", "/api/environments/environments-2/machines")}); + + machines.Items.Add(new MachineResource {Id = "machines/84", EnvironmentIds = new ReferenceCollection(new[] {"environments-1"}), Name = "Mymachine", Links = LinkCollection.Self("/machines/whatever/1")}); + + operation.TentacleThumbprint = "ABCDEF"; + operation.TentaclePort = 10930; + operation.MachineName = "Mymachine"; + operation.TentacleHostname = "Mymachine.test.com"; + operation.CommunicationStyle = CommunicationStyle.TentaclePassive; + operation.EnvironmentNames = new[] {"Production"}; + operation.AllowOverwrite = true; + + await operation.ExecuteAsync(serverEndpoint).ConfigureAwait(false); + + await client.Received().Update("/machines/whatever/1", Arg.Is(m => + m.Id == "machines/84" + && m.Name == "Mymachine" + && m.EnvironmentIds.First() == "environments-2"), + Arg.Any(), Arg.Any()).ConfigureAwait(false); + } + + [Test] + public async Task ShouldCreateWhenCantDeserializeMachines() + { + client.When(x => x.Paginate(Arg.Any(), Arg.Any(), Arg.Any, bool>>())) + .Throw(new OctopusDeserializationException(1, "Can not deserialize")); + + environments.Items.Add(new EnvironmentResource { Id = "environments-1", Name = "UAT", Links = LinkCollection.Self("/api/environments/environments-1").Add("Machines", "/api/environments/environments-1/machines") }); + environments.Items.Add(new EnvironmentResource { Id = "environments-2", Name = "Production", Links = LinkCollection.Self("/api/environments/environments-2").Add("Machines", "/api/environments/environments-2/machines") }); + + operation.TentacleThumbprint = "ABCDEF"; + operation.TentaclePort = 10930; + operation.MachineName = "Mymachine"; + operation.TentacleHostname = "Mymachine.test.com"; + operation.CommunicationStyle = CommunicationStyle.TentaclePassive; + operation.EnvironmentNames = new[] { "Production" }; + + await operation.ExecuteAsync(serverEndpoint).ConfigureAwait(false); + + await client.Received().Create("/api/machines", Arg.Is(m => + m.Name == "Mymachine" + && ((KubernetesTentacleEndpointResource)m.Endpoint).TentacleEndpointConfiguration.Uri == "https://mymachine.test.com:10930/" + && m.EnvironmentIds.First() == "environments-2"), + Arg.Any(), Arg.Any()).ConfigureAwait(false); + } + + [Test] + public async Task ShouldNotOverwriteMachinePolicyToNull() + { + environments.Items.Add(new EnvironmentResource { Id = "environments-1", Name = "UAT", Links = LinkCollection.Self("/api/environments/environments-1").Add("Machines", "/api/environments/environments-1/machines") }); + environments.Items.Add(new EnvironmentResource { Id = "environments-2", Name = "Production", Links = LinkCollection.Self("/api/environments/environments-2").Add("Machines", "/api/environments/environments-2/machines") }); + + machines.Items.Add(new MachineResource { Id = "machines/84", MachinePolicyId = "MachinePolicies-1", EnvironmentIds = new ReferenceCollection(new[] { "environments-1" }), Name = "Mymachine", Links = LinkCollection.Self("/machines/whatever/1") }); + + operation.TentacleThumbprint = "ABCDEF"; + operation.TentaclePort = 10930; + operation.MachineName = "Mymachine"; + operation.TentacleHostname = "Mymachine.test.com"; + operation.CommunicationStyle = CommunicationStyle.TentaclePassive; + operation.EnvironmentNames = new[] { "Production" }; + operation.AllowOverwrite = true; + + await operation.ExecuteAsync(serverEndpoint).ConfigureAwait(false); + + await client.Received().Update("/machines/whatever/1", Arg.Is(m => + m.Id == "machines/84" + && m.Name == "Mymachine" + && m.EnvironmentIds.First() == "environments-2" + && m.MachinePolicyId == "MachinePolicies-1"), + Arg.Any(), Arg.Any()).ConfigureAwait(false); + } + + [Test] + public async Task ShouldOverwriteMachinePolicyWhenPassed() + { + environments.Items.Add(new EnvironmentResource { Id = "environments-1", Name = "UAT", Links = LinkCollection.Self("/api/environments/environments-1").Add("Machines", "/api/environments/environments-1/machines") }); + environments.Items.Add(new EnvironmentResource { Id = "environments-2", Name = "Production", Links = LinkCollection.Self("/api/environments/environments-2").Add("Machines", "/api/environments/environments-2/machines") }); + + machines.Items.Add(new MachineResource { Id = "machines/84", MachinePolicyId = "MachinePolicies-1", EnvironmentIds = new ReferenceCollection(new[] { "environments-1" }), Name = "Mymachine", Links = LinkCollection.Self("/machines/whatever/1") }); + machinePolicies.Items.Add(new MachinePolicyResource {Id = "MachinePolicies-2", Name = "Machine Policy 2"}); + operation.TentacleThumbprint = "ABCDEF"; + operation.TentaclePort = 10930; + operation.MachineName = "Mymachine"; + operation.TentacleHostname = "Mymachine.test.com"; + operation.CommunicationStyle = CommunicationStyle.TentaclePassive; + operation.EnvironmentNames = new[] { "Production" }; + operation.AllowOverwrite = true; + operation.MachinePolicy = "Machine Policy 2"; + + await operation.ExecuteAsync(serverEndpoint).ConfigureAwait(false); + + await client.Received().Update("/machines/whatever/1", Arg.Is(m => + m.Id == "machines/84" + && m.Name == "Mymachine" + && m.EnvironmentIds.First() == "environments-2" + && m.MachinePolicyId == "MachinePolicies-2"), + Arg.Any(), Arg.Any()).ConfigureAwait(false); + } } } \ No newline at end of file diff --git a/source/Octopus.Server.Client/Operations/RegisterKubernetesClusterOperation.cs b/source/Octopus.Server.Client/Operations/RegisterKubernetesClusterOperation.cs index 16675175e..900e0494d 100644 --- a/source/Octopus.Server.Client/Operations/RegisterKubernetesClusterOperation.cs +++ b/source/Octopus.Server.Client/Operations/RegisterKubernetesClusterOperation.cs @@ -1,3 +1,4 @@ +using System.Threading.Tasks; using Octopus.Client.Model; using Octopus.Client.Model.Endpoints; @@ -13,6 +14,11 @@ public RegisterKubernetesClusterOperation(IOctopusClientFactory clientFactory) : { } + protected override void PrepareMachineForReRegistration(MachineResource machine, string proxyId) + { + machine.Endpoint = GenerateEndpoint(proxyId); + } + protected override EndpointResource GenerateEndpoint(string proxyId) { return CommunicationStyle switch diff --git a/source/Octopus.Server.Client/Operations/RegisterMachineOperation.cs b/source/Octopus.Server.Client/Operations/RegisterMachineOperation.cs index 43fc99a72..33a3af3d9 100644 --- a/source/Octopus.Server.Client/Operations/RegisterMachineOperation.cs +++ b/source/Octopus.Server.Client/Operations/RegisterMachineOperation.cs @@ -1,11 +1,9 @@ using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Threading.Tasks; using Octopus.Client.Exceptions; using Octopus.Client.Model; -using Octopus.Client.Model.Endpoints; namespace Octopus.Client.Operations { @@ -65,17 +63,32 @@ public RegisterMachineOperation(IOctopusClientFactory clientFactory) : base(clie /// public override void Execute(IOctopusSpaceRepository repository) { - var selectedEnvironments = GetEnvironments(repository); - var machinePolicy = GetMachinePolicy(repository); var machine = GetMachine(repository); - var tenants = GetTenants(repository); - ValidateTenantTags(repository); var proxy = GetProxy(repository); - ApplyBaseChanges(machine, machinePolicy, proxy); - ApplyDeploymentTargetChanges(machine, selectedEnvironments, tenants); + if (!IsExistingMachine(machine) || AllowOverwrite) + { + var machinePolicy = GetMachinePolicy(repository); + ValidateTenantTags(repository); + ApplyBaseChanges(machine, machinePolicy, proxy); + ApplyDeploymentTargetChanges(machine, GetEnvironments(repository), GetTenants(repository)); + } + else + { + PrepareMachineForReRegistration(machine, proxy?.Id); + } + + ModifyOrCreateMachine(repository, machine); + } + + static bool IsExistingMachine(MachineResource machine) + { + return machine.Id != null; + } - if (machine.Id != null) + static void ModifyOrCreateMachine(IOctopusSpaceRepository repository, MachineResource machine) + { + if (IsExistingMachine(machine)) repository.Machines.Modify(machine); else repository.Machines.Create(machine); @@ -130,12 +143,11 @@ MachineResource GetMachine(IOctopusSpaceRepository repository) try { existing = repository.Machines.FindByName(MachineName); - if (!AllowOverwrite && existing?.Id != null) - throw new InvalidRegistrationArgumentsException($"A machine named '{MachineName}' already exists on the Octopus Server in the target space. Use the 'force' parameter if you intended to update the existing machine."); } - catch (OctopusDeserializationException) // eat it, probably caused by resource incompatability between versions + catch (OctopusDeserializationException) // eat it, probably caused by resource incompatibility between versions { } + return existing ?? new MachineResource(); } @@ -147,18 +159,35 @@ MachineResource GetMachine(IOctopusSpaceRepository repository) /// public override async Task ExecuteAsync(IOctopusSpaceAsyncRepository repository) { - var selectedEnvironments = GetEnvironments(repository).ConfigureAwait(false); - var machinePolicy = GetMachinePolicy(repository).ConfigureAwait(false); - var machineTask = GetMachine(repository).ConfigureAwait(false); - var tenants = GetTenants(repository).ConfigureAwait(false); - await ValidateTenantTags(repository).ConfigureAwait(false); - var proxy = GetProxy(repository).ConfigureAwait(false); + var machine = await GetMachine(repository).ConfigureAwait(false); + var proxy = await GetProxy(repository).ConfigureAwait(false); + + if (!IsExistingMachine(machine) || AllowOverwrite) + { + var machinePolicy = GetMachinePolicy(repository).ConfigureAwait(false); + await ValidateTenantTags(repository).ConfigureAwait(false); + ApplyBaseChanges(machine, await machinePolicy, proxy); + var selectedEnvironments = await GetEnvironments(repository).ConfigureAwait(false); + var tenants = await GetTenants(repository).ConfigureAwait(false); + ApplyDeploymentTargetChanges(machine, selectedEnvironments, tenants); + } + else + { + PrepareMachineForReRegistration(machine, proxy?.Id); + } + + await ModifyOrCreateMachine(repository, machine).ConfigureAwait(false); + } - var machine = await machineTask; - ApplyBaseChanges(machine, await machinePolicy, await proxy); - ApplyDeploymentTargetChanges(machine, await selectedEnvironments, await tenants); + protected virtual void PrepareMachineForReRegistration(MachineResource machineResource, string proxyId) + { + throw new InvalidRegistrationArgumentsException( + $"A machine named '{MachineName}' already exists in the environment. Use the 'force' parameter if you intended to update the existing machine."); + } - if (machine.Id != null) + static async Task ModifyOrCreateMachine(IOctopusSpaceAsyncRepository repository, MachineResource machine) + { + if (IsExistingMachine(machine)) await repository.Machines.Modify(machine).ConfigureAwait(false); else await repository.Machines.Create(machine).ConfigureAwait(false); @@ -226,8 +255,6 @@ async Task GetMachine(IOctopusSpaceAsyncRepository repository) try { existing = await repository.Machines.FindByName(MachineName).ConfigureAwait(false); - if (!AllowOverwrite && existing?.Id != null) - throw new InvalidRegistrationArgumentsException($"A machine named '{MachineName}' already exists in the environment. Use the 'force' parameter if you intended to update the existing machine."); } catch (OctopusDeserializationException) // eat it, probably caused by resource incompatability between versions {