Allow Kubernetes Agent to be registered as a worker #655

rain-on · 2024-06-25T06:54:12Z

This change is similar to work conducted when registering the k8s agent as a deployment target.

I.e. this change simply allows for a worker to be created in an OctopusDpeloy instance - it is not responsible for the actual helm installation.

THis change means that a worker-object can be created and initialized in an Octopus instance, and the values (certificate/name etc) can then be reused during the helm installation process (which is performed by the helm's TF provider).

When the helm install is performed, the installed worker sees that the required object in Ocotpus already exists, and does not register a second time.

This change means that the worker object, which represents the installed helm agent - is accessible from within the TF script if it requires further modification (enable/disable etc).

kevjt · 2024-07-31T01:26:52Z

docs/resources/kubernetes_agent_worker.md

+
+### Required
+
+- `environments` (List of String) A list of environment IDs this Kubernetes agent can deploy to.


These are fields for the agent as deployment target. I think you may need to regenerate the docs

yup - needs new docs.

octopusdeploy/schema_kubernetes_agent_worker.go

kevjt · 2024-07-31T01:34:55Z

octopusdeploy/schema_kubernetes_agent_worker.go

+			Default:     false,
+			Type:        schema.TypeBool,
+		},
+		"worker_pool_ids": {


This is missing a description

kevjt · 2024-07-31T01:39:50Z

octopusdeploy/schema_worker.go

+
+	return map[string]*schema.Schema{
+		"communication_styles": getQueryCommunicationStyles(),
+		"workers": {


Why is there a list of workers in the schema for a worker?

This follows the same pattern we use in other schemas (eg schema_worker_pool) - the other fields represent filters you can specify when searching for worker pools - the workers object is the computed response of what was found in the search.

kevjt · 2024-07-31T01:42:45Z

octopusdeploy/schema_kubernetes_agent_worker.go

+		Type:        schema.TypeList,
+	}
+
+	delete(WorkerDataSchema, "communication_styles")


Curious why these fields need to be deleted from the worker data schema

This is again somewhat of a copy-paste.
But - while you can search for workers via comms-style - that doesn't work for a kubernetes_agent_workers it just has no meaning - so there's no need to ask for it.
Why do we remove the workers? THAT is a different question and I need to investigate that :)
I suspect its because we've removed "workers" and replaced it with "kubernetes_agent_workers" insetad.

kevjt · 2024-07-31T02:03:21Z

octopusdeploy/schema_kubernetes_agent_worker.go

+		"id":       getIDSchema(),
+		"space_id": getSpaceIDSchema(),
+		"name":     getNameSchema(true),
+		"communication_mode": {


Nit: Can we just exclude this field until there is support for more than one communication mode? We are going to have to come back and edit the description once that happens anyway

In an attempt to keep it aligned with the k8s_deployment_target, I was going to leave it here.

kevjt · 2024-07-31T02:11:37Z

integration_test.go

+			t.Fatalf("Space must have two workers (both KubernetesTentacles), instead found %v", resources.Items)
+		}
+
+		optionalAgentName := "minimum-agent"


minimalAgent_ is a better variable name since the other agent has the name "agent-with-optionals" and it can be confusing

kevjt · 2024-07-31T02:15:21Z

terraform/58-kubernetesagentworker/kubernetes_agent_workers.tf

@@ -0,0 +1,17 @@
+resource "octopusdeploy_kubernetes_agent_worker" "agent_with_minimum" {
+  name         = "minimum-agent"
+  worker_pool_ids = [octopusdeploy_static_worker_pool.workerpool_docker.id]


Perhaps it's overkill, but it would be nice to set more than one worker pool here and assert that the created worker has all the right worker pool ids associated with it.

I do also acknowledge the fact that these integration tests are more or less a sanity check to see if it all 'compiles and runs', and not meant to be an exhaustive check of all the field mappings..

kevjt · 2024-07-31T02:18:47Z

integration_test.go

@@ -3848,3 +3848,64 @@ func TestTentacleCertificateResource(t *testing.T) {
 		return nil
 	})
 }
+
+func TestKubernetesAgentWorkerResource(t *testing.T) {


I think you'd want a test for the agent as worker data_source too. Have a look at the other integration tests that have a part two to them (e.g. 2-projectgroup and 2a-projectgroupds). The part two is for testing the data_source.

kevjt · 2024-07-31T02:22:38Z

docs/resources/kubernetes_agent_worker.md

Do we have docs in other places that give examples on how to use this terraform provider in conjunction with the helm provider to actually create the kubernetes agent/worker?

no - not yet - we'll make docs along the lines of those used for the deployment-target docs

IsaacCalligeros95 · 2024-08-22T23:43:16Z

octopusdeploy/schema_kubernetes_agent_worker.go

+		Computed:    true,
+		Description: "A list of kubernetes agent workers that match the filter(s).",
+		Elem:        &schema.Resource{Schema: dataSchema},
+		Optional:    true,


There was a recent issue around deferring the reads for datasources. This only applies to the return list, TLDR is that optional should be false here.

There's more details on this PR

Thanks for the heads up!

IsaacCalligeros95 · 2024-08-22T23:55:27Z

octopusdeploy/schema_kubernetes_agent_worker.go

+
+	communicationsMode := kubernetesAgent.Get("communication_mode").(string)
+	upgradeLocked := kubernetesAgent.Get("upgrade_locked").(bool)
+	var endpoint machines.IEndpoint = machines.NewKubernetesTentacleEndpoint(uri, thumbprint, upgradeLocked, communicationsMode, "")


I know very little about the Kubernetes agent, should the namespace be set here?

The defaultNamespace is only applicable to the Kubernetes agent when it is operating as a deployment target. It defines the Kubernetes namespace to which applications get deployed.
In this case the Kubernetes agent is operating as a worker which means that it does not have this concept. The namespace that the worker itself resides in is defined separately using the namespace of the Helm installation.

IsaacCalligeros95 · 2024-08-23T00:01:23Z

docs/resources/kubernetes_agent_worker.md

+### Required
+
+- `name` (String) The name of this resource.
+- `thumbprint` (String) The thumbprint of the Kubernetes agent's certificate used by server to verify the identity of the agent. This is the same thumbprint that was used when installing the agent.


Most other targets have thumbprint as optional. This is probably required for the agent vs other targets but just want to raise it to double check this is correct.

It is required here because of the way the Kubernetes agent is installed as a polling Tentacle. Normally the agent sends its own thumbprint to Octopus server during installation where it registers itself as a target/worker. However, in the case of Terraform the resource in Octopus server is being created first so the exact thumbprint must be known ahead of time.

IsaacCalligeros95

LGTM, a couple of minor comments. I'm not too familiar with the agent so the actual shape of the resource/datasource. It would be good to get another review from someone who is more familiar with the SDK side of things but from what I can tell it all looks 👌 other than the few minor comments.

Meant to comment

domenicsim1

Nice! Do note we will be moving this to the new terraform framework in the near future, therefore behaviour may change slightly. 🤷
As this change was in progress before the migration work for framework started let's merge it and we will target all future changes for framework. 🚀

rain-on added 2 commits June 17, 2024 16:33

add k8s worker to terraform

e44242f

Merge remote-tracking branch 'origin/main' into tmm/k8gent_worker

4bfd2d8

APErebus changed the title ~~Allow Kuberentes Agent to be registered as a worker~~ Allow Kubernetes Agent to be registered as a worker Jun 25, 2024

rain-on added 9 commits June 26, 2024 11:37

create schema_worker

74eb654

adding examples I think

cc23624

might be working

e3b972f

don't lose file

cea6f0f

personal review of production code

e60707a

move files around for testing

72fd4c7

updated integration test

46c29f2

trying to get workerpool_ids working again

c183325

force worker to on in int tests

cb50746

rain-on marked this pull request as ready for review June 28, 2024 01:28

rain-on added 13 commits July 1, 2024 16:35

no idea

949a1b6

pre merge

b04c7ea

Merge remote-tracking branch 'origin/main' into tmm/k8gent_worker

577db6a

use newer test framework

6432d3b

new framework for test

ae82ee0

add some help output

3955a95

change comms style

9215277

fix enabled/disabled

3153a42

fix naming

ea9b891

flip it all around again

142eeeb

ensure upgrade locked is false by default

1631a56

revert workflow file

fcd96a8

Merge remote-tracking branch 'origin/main' into tmm/k8gent_worker

c73f5e4

kevjt reviewed Jul 31, 2024

View reviewed changes

rain-on added 3 commits August 5, 2024 11:10

Merge remote-tracking branch 'origin/main' into tmm/k8gent_worker

133eb2b

not sure what was done in this commit - maybe drop?

7ef0bb6

Merge remote-tracking branch 'origin/main' into tmm/k8gent_worker

8bdcb0b

rain-on and others added 7 commits August 16, 2024 11:02

hack container creation into int test

1d7e1cd

Merge branch 'main' into tmm/k8gent_worker

b316ba8

update test to conform with the new way of integration testing

8c7d0c0

regenerate docs

140a672

edit

fbae2c2

remove random char

a4ffcd1

fix

653b443

kevjt mentioned this pull request Aug 21, 2024

Add space delimited CRUD operations to Workers using the new client OctopusDeploy/go-octopusdeploy#269

Merged

kevjt added 5 commits August 21, 2024 13:48

use space aware go client functions

63b0412

Merge branch 'main' into tmm/k8gent_worker

8db84aa

regenerate docs to pass the doc validation github action

5cecf88

try add newline

0dc91ee

isolate test container

7e4a4d7

IsaacCalligeros95 reviewed Aug 22, 2024

View reviewed changes

IsaacCalligeros95 reviewed Aug 23, 2024

View reviewed changes

IsaacCalligeros95 previously approved these changes Aug 23, 2024

View reviewed changes

IsaacCalligeros95 self-requested a review August 23, 2024 00:03

kevjt added 2 commits August 23, 2024 10:10

data source result must not be optional

318d6b9

put newline back so validate docs is happy

a7cfbf9

domenicsim1 approved these changes Aug 23, 2024

View reviewed changes

kevjt merged commit 2058858 into main Aug 23, 2024
22 checks passed

kevjt deleted the tmm/k8gent_worker branch August 23, 2024 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Kubernetes Agent to be registered as a worker #655

Allow Kubernetes Agent to be registered as a worker #655

rain-on commented Jun 25, 2024 •

edited

Loading

kevjt Jul 31, 2024

rain-on Aug 16, 2024

kevjt Jul 31, 2024

kevjt Jul 31, 2024

rain-on Aug 5, 2024

kevjt Jul 31, 2024

rain-on Aug 5, 2024 •

edited

Loading

kevjt Jul 31, 2024

rain-on Aug 5, 2024

kevjt Jul 31, 2024

kevjt Jul 31, 2024

kevjt Jul 31, 2024

kevjt Jul 31, 2024

rain-on Aug 2, 2024

IsaacCalligeros95 Aug 22, 2024

kevjt Aug 22, 2024

IsaacCalligeros95 Aug 22, 2024

kevjt Aug 23, 2024

IsaacCalligeros95 Aug 23, 2024

kevjt Aug 23, 2024

IsaacCalligeros95 left a comment •

edited

Loading

domenicsim1 left a comment


		### Required

		- `environments` (List of String) A list of environment IDs this Kubernetes agent can deploy to.

Allow Kubernetes Agent to be registered as a worker #655

Allow Kubernetes Agent to be registered as a worker #655

Conversation

rain-on commented Jun 25, 2024 • edited Loading

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

rain-on Aug 5, 2024 • edited Loading

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

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

IsaacCalligeros95 left a comment • edited Loading

Choose a reason for hiding this comment

domenicsim1 left a comment

Choose a reason for hiding this comment

rain-on commented Jun 25, 2024 •

edited

Loading

rain-on Aug 5, 2024 •

edited

Loading

IsaacCalligeros95 left a comment •

edited

Loading