Update Kubernetes Tentacle to use ConfigMap for configuration storage #674

scme0 · 2023-11-16T21:56:05Z

Background

Part of #project-k8s-agent

To make sure that the Kubernetes Tentacle is able to handle restarts (including when the nfs pod is restarted) we need to update Tentacle to be able to store its configuration into a ConfigMap.

Results

I've add a new IKeyValueStore and IWritableKeyValueStore implementation which stores data in a Kubernetes ConfigMap. It requires a namespace to make sure that each Kubernetes Tentacle in the cluster has a unique ConfigMap.

I've also added an IsRegistered value to the store so that we can skip registration for Kubernetes Tentacles. We also needed to update the configuration process to remove the "remove trust" step which breaks the configuration process if we're already registered. I thought about updating that command to not clear if we're already registered but that seems to defeat the purpose of the command.

How to review this PR

Quality ✔️

Pre-requisites

I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
I have considered informing or consulting the right people, according to the ownership map.
I have considered appropriate testing for my change.

scme0 · 2023-11-16T21:56:54Z

[sc-60480]

shortcut-integration · 2023-11-16T21:56:57Z

This pull request has been linked to Shortcut Story #60480: When Tentacle is running in Kubernetes, store configuration in ConfigMap.

scme0 · 2024-01-17T07:32:23Z

docker/kubernetes-tentacle/scripts/configure-tentacle.sh

@@ -115,9 +115,6 @@ function configureTentacle() {
        tentacle configure --instance "$instanceName" --port $internalListeningPort --noListen "False"
    fi

-    echo "Updating trust ..."
-    tentacle configure --instance "$instanceName" --reset-trust


This clears out the TrustedOctopusServers which is set when registering. If we skip registering when we've already registered, we effectively clear out the trusted servers and then never replace them meaning a restart of the container fails.

I've remove this because we set the trusted server once and then never clear it. Is this going to cause us issues?

Rather than removing it, could we skip some of this configuration if we know it's configured? Or do we need to set some information before we start the tentacle app?

If we just skip it when "isRegistered" is true, the only time it would be executed is when "isRegistered" is false, which is when there would be no trusted servers configured anyway. So I thought it didn't really make sense to have it when running in kubernetes...

scme0 · 2024-01-17T07:32:55Z

source/Octopus.Tentacle.Tests.Integration/TentacleCommandLineTests.cs

@@ -596,9 +596,6 @@ public async Task ShowConfigurationCommandLooksSensibleToHumans(TentacleConfigur
        {{
          ""Thumbprint"": ""{clientAndTentacle.Server.Thumbprint}"",
          ""CommunicationStyle"": 2,
-          ""KubernetesTentacleCommunicationMode"": {{
-            ""Value"": ""Polling""
-          }},


This property only shows up on serialisation if the CommunicationStyle is KubernetesTentacle now.

I did this because the example below is using CommunicationStyle of Passive but the KubernetesTentacleCommunicationMode says Polling which is confusing.

scme0 · 2024-01-17T07:38:54Z

source/Octopus.Tentacle/Commands/ShowConfigurationCommand.cs

@@ -86,26 +87,14 @@ async Task StartAsync()

        internal async Task CollectConfigurationSettings(DictionaryKeyValueStore outputStore)


This method was a PITA - The HomeConfiguration and TentacleConfiguration classes would take a Configuration store (IKeyValueStore) but also an instance selector which is what you're meant to use to get the IKeyValueStore. But it was used to collect the properties to output them in JSON format. It's just very unclear where values are sourced from and where values are set. I think I've made it clearer by using the WriteTo method so we know that the IKeyValueStore from the selector is used to source the data and it's written to the outputStore.

source/Octopus.Tentacle/Commands/ShowConfigurationCommand.cs

APErebus

Broadly I like the approach, there is just a few things that need cleaning up/addressing

APErebus · 2024-01-18T02:31:24Z

docker/kubernetes-tentacle/scripts/configure-tentacle.sh

@@ -115,9 +115,6 @@ function configureTentacle() {
        tentacle configure --instance "$instanceName" --port $internalListeningPort --noListen "False"
    fi

-    echo "Updating trust ..."
-    tentacle configure --instance "$instanceName" --reset-trust


Rather than removing it, could we skip some of this configuration if we know it's configured? Or do we need to set some information before we start the tentacle app?

source/Octopus.Tentacle/Configuration/Instances/ConfigMapKeyValueStore.cs

source/Octopus.Tentacle/Kubernetes/KubernetesConfig.cs

source/Octopus.Tentacle/Configuration/Instances/ApplicationInstanceSelector.cs

APErebus

Looks good! Couple of minor things that aren't blocking. The only other piece missing is the setting encryption when ProectionLevel.MachineKey using a value in a secret

APErebus · 2024-01-19T01:34:06Z

source/Octopus.Tentacle/Configuration/Instances/ApplicationInstanceSelector.cs

+            if (appInstance is { instanceName: not null, configurationpath: null } &&
+                KubernetesConfig.Namespace is {} @namespace)
            {
-                var message = !string.IsNullOrEmpty(instanceName)
-                    ? $"The configuration file for instance {instanceName} could not be located at the specified location {configurationPath}. " +
-                    "The file might have been manually removed without properly removing the instance and as such it is still listed as present." +
-                    "The instance must be created again before you can interact with it."
-                    : $"The configuration file at {configurationPath} could not be located at the specified location.";
-
-                throw new ControlledFailureException(message);
+                log.Verbose($"Loading configuration from ConfigMap for namespace {@namespace}");
+                var configMapWritableStore = configMapStoreFactory.Value;
+                return (ContributeAdditionalConfiguration(configMapWritableStore), configMapWritableStore);
            }


This check seems weird (checking the namespace). Should we be using the PlatformDetection here?

Ah yep, good call!

APErebus · 2024-01-19T01:35:29Z

source/Octopus.Tentacle/Kubernetes/KubernetesConfigMapService.cs

+        Task<V1ConfigMap> Patch(string name, IDictionary<string, string> data, CancellationToken cancellationToken);
+    }
+
+    public class KubernetesV1ConfigMapService : KubernetesService, IKubernetesV1ConfigMapService


You can drop the V1 here. I doubt we'll need to worry about V2 K8s apis/types for a while

Removed 👍

… if set

… configuration

…n configuring kubernetes tentacle

…tection implementation

… already registered

…espace

# Conflicts: # source/Octopus.Tentacle.Tests/Configuration/ApplicationInstanceSelectorFixture.cs # source/Octopus.Tentacle/Configuration/Instances/ConfigMapKeyValueStore.cs # source/Octopus.Tentacle/Kubernetes/KubernetesModule.cs

LukeButters · 2024-01-22T05:01:31Z

source/Octopus.Tentacle/Configuration/Instances/ApplicationInstanceSelector.cs

        }

-        AggregatedKeyValueStore ContributeAdditionalConfiguration(XmlFileKeyValueStore writableConfig)
+        bool ConfigurationFileExists([NotNullWhen(true)] string? configurationPath)


nit: Did these methods need to be pulled out? It may be best to leave what we can alone to avoid any chance of introducing inadvertent changes.

ultra nit: Also since this just throws we could have pulled out the entire if into a single method.

scme0 changed the title ~~Spiked out the ability for Tentacle to save changes in a config map in kubernetes~~ Spike: Add ability for Tentacle to save changes to a config map in kubernetes Nov 20, 2023

scme0 force-pushed the scme/spikes/store-tentacle-config-in-configmap branch 3 times, most recently from 4240cfa to d8b14a5 Compare January 16, 2024 04:47

scme0 changed the title ~~Spike: Add ability for Tentacle to save changes to a config map in kubernetes~~ Update Kubernetes Tentacle to use ConfigMap for configuration storage Jan 16, 2024

scme0 commented Jan 17, 2024

View reviewed changes

source/Octopus.Tentacle/Commands/ShowConfigurationCommand.cs Outdated Show resolved Hide resolved

scme0 marked this pull request as ready for review January 17, 2024 07:58

scme0 force-pushed the scme/spikes/store-tentacle-config-in-configmap branch from c5d8cbf to feb31b8 Compare January 17, 2024 07:58

scme0 requested a review from a team as a code owner January 17, 2024 07:58

APErebus requested changes Jan 18, 2024

View reviewed changes

scme0 requested a review from APErebus January 18, 2024 07:03

APErebus approved these changes Jan 19, 2024

View reviewed changes

scme0 mentioned this pull request Jan 19, 2024

Add encryption to configmap store #772

Merged

3 tasks

scme0 added 12 commits January 19, 2024 15:33

Initial spike

91860f9

Update configure and run tentacle scripts to use InstanceName env var…

58e1c9c

… if set

Fix bad rebase

50e58c4

Add "IsRegistered" configuration property

bccabcf

Update HomeConfiguration to make it work correctly with the configmap…

f2cf368

… configuration

More updates

25b82d8

Add ToWrite method for config stores

e07a7b5

Undo instance name in configure and run scripts

adae479

Fix up WriteTo functionality

6c54f20

Fix bug

232e426

Update ShowConfigurationCommand

3b030d7

Comment out log statements

87df524

scme0 added 23 commits January 19, 2024 15:33

Remove WriteTo from KeyValueStores

c664f24

Add ShouldSerialize method for KubernetesTentacleCommunicationMode

2410166

Add "WriteTo" methods to HomeConfiguration and TentacleConfiguration

28b75d7

Remove unneeded console.Writelines part 1

03c4efe

More clean ups

0d97038

Neaten up ApplicationInstanceSelector

46ebf5f

Update logging

dbabf35

Move EnvironmentVariable to KubernetesConfig.cs

734077c

Fix build

b24ec40

Remove "Updating trust" step for configuration as it's not needed whe…

59efa4c

…n configuring kubernetes tentacle

Add logs

3d568d5

Tidy up ApplicationInstanceManager

54872e2

Make excludes code clearer

8b7cb18

Fix build

4219f48

Remove "WriteTo" method from ConfigMapKeyValueStore

857124a

Update IsRunningInKubernetesCluster usages to use existing PlatformDe…

0fc820e

…tection implementation

Create KubernetesConfigMapService and use it in ConfigMapKeyValueStore

10f4df2

Review updates

d84a471

Update configure script to skip all configuration and registration if…

f216424

… already registered

Add missing cancellation token parameter to ConfigMapKeyValueStore

c186f80

Update check to use PlatformDetection instead of KubernetesConfig.Nam…

e6c3ac4

…espace

Remove V1 from ConfigMapService

0b81806

# Conflicts: # source/Octopus.Tentacle.Tests/Configuration/ApplicationInstanceSelectorFixture.cs # source/Octopus.Tentacle/Configuration/Instances/ConfigMapKeyValueStore.cs # source/Octopus.Tentacle/Kubernetes/KubernetesModule.cs

Update ConfigMapKeyValueStore to use lazy initialisation of ConfigMap

44f4e32

scme0 force-pushed the scme/spikes/store-tentacle-config-in-configmap branch from 815a4de to 44f4e32 Compare January 19, 2024 05:05

Fix nullable build error

f079060

scme0 merged commit 913b571 into main Jan 22, 2024
47 of 48 checks passed

scme0 deleted the scme/spikes/store-tentacle-config-in-configmap branch January 22, 2024 00:48

LukeButters reviewed Jan 22, 2024

View reviewed changes

LukeButters mentioned this pull request Jan 23, 2024

Add required type registration for Tentacle Manager #779

Merged

3 tasks

acodrington mentioned this pull request Apr 5, 2024

Remove usage of ApplicationInstanceSelector from Tentacle Manager #874

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Kubernetes Tentacle to use ConfigMap for configuration storage #674

Update Kubernetes Tentacle to use ConfigMap for configuration storage #674

scme0 commented Nov 16, 2023 •

edited

Loading

scme0 commented Nov 16, 2023

shortcut-integration bot commented Nov 16, 2023

scme0 Jan 17, 2024

APErebus Jan 18, 2024

scme0 Jan 18, 2024

scme0 Jan 17, 2024

scme0 Jan 17, 2024 •

edited

Loading

scme0 Jan 17, 2024 •

edited

Loading

APErebus left a comment

APErebus Jan 18, 2024

APErebus left a comment

APErebus Jan 19, 2024

scme0 Jan 19, 2024

scme0 Jan 19, 2024

APErebus Jan 19, 2024

scme0 Jan 19, 2024

LukeButters Jan 22, 2024

		@@ -86,26 +87,14 @@ async Task StartAsync()

		internal async Task CollectConfigurationSettings(DictionaryKeyValueStore outputStore)

Update Kubernetes Tentacle to use ConfigMap for configuration storage #674

Update Kubernetes Tentacle to use ConfigMap for configuration storage #674

Conversation

scme0 commented Nov 16, 2023 • edited Loading

Background

Results

How to review this PR

Pre-requisites

scme0 commented Nov 16, 2023

shortcut-integration bot commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scme0 Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

scme0 Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

APErebus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APErebus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scme0 commented Nov 16, 2023 •

edited

Loading

scme0 Jan 17, 2024 •

edited

Loading

scme0 Jan 17, 2024 •

edited

Loading