Use EKS Addon to manage `kube-proxy` and `coredns` #1357

corymhall · 2024-09-12T16:30:24Z

Proposed changes

This PR switches the coredns and kube-proxy addons from self-managed to managed. By default the latest compatible version will be used.

This also introduces two new top level arguments to ClusterOptions for configuring these new addons.

corednsAddonOptions
kubeProxyAddonOptions

BREAKING CHANGE: creating an eks.Cluster will now also create the coredns and kube-proxy addons. If you are currently already managing these you will need to disable the creation of these through the new arguments ClusterOptions.corednsAddonOptions.enabled = false and ClusterOptions.kubeProxyAddonOptions.enabled = false

Related issues (optional)

closes #1261, closes #1254

github-actions · 2024-09-12T16:32:49Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

closes #1261

corymhall · 2024-09-17T17:29:06Z

examples/cluster/index.ts

@@ -39,6 +39,9 @@ const cluster2 = new eks.Cluster(`${projectName}-2`, {
 const cluster3 = new eks.Cluster(`${projectName}-3`, {
    vpcId: vpc.vpcId,
    publicSubnetIds: vpc.publicSubnetIds,
+    corednsAddonOptions: {


We need to add this to the migration guide.

flostadler · 2024-09-18T08:04:52Z

nodejs/eks/cluster.ts

@@ -627,6 +624,74 @@ export function createCore(
        },
    );

+    pulumi.output(args.fargate).apply((fargate) => {


What do you think about lifting the addons out of the apply and always creating them?
If fargate is enabled we set the compute type config.

flostadler · 2024-09-18T08:05:38Z

nodejs/eks/cluster.ts

+                        args.kubeProxyAddonOptions?.resolveConflictsOnCreate ?? "OVERWRITE",
+                    resolveConflictsOnUpdate:
+                        args.kubeProxyAddonOptions?.resolveConflictsOnUpdate ?? "OVERWRITE",
+                    addonVersion: kubeProxyVersion,


we should set the preserve property to true. Otherwise this would delete the resources from the cluster when users toggle the enabled setting.

We should also set the tags

That brings up a question - Is enabled a thing that only works on initial deployment?

The addons can be created/deleted at any time

flostadler · 2024-09-18T08:06:43Z

provider/cmd/pulumi-gen-eks/main.go

@@ -1798,6 +1812,108 @@ func generateSchema() schema.PackageSpec {
 					Required: []string{"content", "contentType"},
 				},
 			},
+			"eks:index:ResolveConflictsOnUpdate": {


flostadler · 2024-09-18T08:11:04Z

provider/cmd/pulumi-gen-eks/main.go

+					},
+				},
+			},
+			"eks:index:CoreDnsAddonOptions": {


We should add the serviceAccountRoleArn prop as well.

I'm also wondering if we should allow users to pass configuration values and then merge those with the ones the provider sets.

Modifying the resource requests/limits or replica count of coredns is a common config for example.

But we can exclude that from this PR here. I've added it to the VPC CNI and if we both start adding those settings we're just gonna step on each others toes

We should add the serviceAccountRoleArn prop as well.

I don't think either coredns or kube-proxy use a role. At least according to this doc

Ah nice! Didn't know that

t0yv0 · 2024-09-18T16:05:00Z

provider/cmd/pulumi-gen-eks/main.go

@@ -663,6 +663,20 @@ func generateSchema() schema.PackageSpec {
 						Description: "The authentication mode of the cluster. Valid values are `CONFIG_MAP`, `API` or `API_AND_CONFIG_MAP`.\n\n" +
 							"See for more details:\nhttps://docs.aws.amazon.com/eks/latest/userguide/grant-k8s-access.html#set-cam",
 					},
+					"corednsAddonOptions": {
+						TypeSpec: schema.TypeSpec{
+							Plain: true,


Do you need Plain: true? I think what it does is make it slightly less nice for the user but supposedly easier for the implementation. Wanted to make sure it's an informed decision here.

I think this is actually nicer for the user. Otherwise previews wouldn't be accurate because this decides whether a resource is created or not.

Ah yes, unsolved problem of conditional creates.

Why does it make it less nice for the user? I thought you use plain when it doesn't need to accept output values. Is that not the case?

Generally if I'm a user, I want to be able to call resources by passing Output<X> values directly. If I have plain values, great, I can lift them to outputs. If I have however computed Output values, and the resource does not accept them, I'm really stuck and I can only provision inside an apply which defeats plans. So generally Plain: true is bad for users. However the fact that this property is used for conditional resource creation inside is a good reason for having Plain: true.

t0yv0 · 2024-09-18T16:06:30Z

provider/cmd/pulumi-gen-eks/main.go

+				ObjectTypeSpec: schema.ObjectTypeSpec{
+					Type: "object",
+					Properties: map[string]schema.PropertySpec{
+						"enabled": {


The Go design would be disabled with default: false, so missing and false are the same. But perhaps we intentionally prefer the extra "enabled: true" marker.

Those resources are deployed by default on EKS

Most people will be using it in TypeScript 😀

I'm thinking Go has good style in here :) But deferring to y'all.

I just had a look through other settings in the provider and more are called enable... then disable.... Many of those are rooted in AWS settings, so I'd vote for enabled here to stay more consistent with what the provider is already doing

Yeah if this is what AWS users expect it's perfect.

t0yv0 · 2024-09-18T16:09:57Z

nodejs/eks/cluster.ts

+                  )
+                  .apply((addonVersion) => addonVersion.version);
+
+        const kubeProxyAddon = new aws.eks.Addon(


Conditional resource, interesting. Good thing args.kubeProxyAddonOptions?.enabled are not outputty and cannot be unknown.

t0yv0 · 2024-09-18T16:11:17Z

nodejs/eks/cluster.ts

+                        });
+                    }
+                    return undefined;
+                }) as any,


Nit: disabling type-checking via any here avoids some pain perhaps?

any is gonna get removed here when we expose the configurationValues. That's a follow up item.

The referenced issue is pulumi/pulumi#6175

t0yv0 · 2024-09-18T16:11:49Z

nodejs/eks/cluster.ts

-                            fs.writeFileSync(tmpKubeconfig.fd, JSON.stringify(kconfig));
-
-                            // Determine if the CoreDNS deployment has a computeType annotation.
-                            const cmdGetAnnos = `kubectl get deployment coredns -n kube-system -o jsonpath='{.spec.template.metadata.annotations}'`;


Very nice removal.

t0yv0 · 2024-09-18T16:13:22Z

Left some nits but looking very healthy overall! Reviewing at the low-level mostly. Trusting this is the right default we want: "creating an eks.Cluster will now also create the coredns and kube-proxy addons"

flostadler

LGTM!

Now that the EKS addons are added we need to align them and do some cleanup. This involves: - adding the enums introduced in #1357 to the VPC CNI - exposing configurationValues for coredns and kube-proxy - removing kubectl from the provider - deeply sort addon configuration keys to guarantee stable json serialization - remove deepmerge again. It caused issues during unit tests (voodoocreation/ts-deepmerge#22) and when used on outputs. Additionally I discovered and fixed an old bug that luckily never surfaced. The VPC CNI configuration did incorrectly handle outputs and called `toString` on them in a couple of places. The increased type safety and tests around addon configuration uncovered this. Closes #1369

### Proposed changes  This PR switches the `coredns` and `kube-proxy` addons from self-managed to managed. By default the latest compatible version will be used. This also introduces two new top level arguments to `ClusterOptions` for configuring these new addons. - `corednsAddonOptions` - `kubeProxyAddonOptions` BREAKING CHANGE: creating an `eks.Cluster` will now also create the `coredns` and `kube-proxy` addons. If you are currently already managing these you will need to disable the creation of these through the new arguments `ClusterOptions.corednsAddonOptions.enabled = false` and `ClusterOptions.kubeProxyAddonOptions.enabled = false` ### Related issues (optional) closes #1261, closes #1254

Now that the EKS addons are added we need to align them and do some cleanup. This involves: - adding the enums introduced in #1357 to the VPC CNI - exposing configurationValues for coredns and kube-proxy - removing kubectl from the provider - deeply sort addon configuration keys to guarantee stable json serialization - remove deepmerge again. It caused issues during unit tests (voodoocreation/ts-deepmerge#22) and when used on outputs. Additionally I discovered and fixed an old bug that luckily never surfaced. The VPC CNI configuration did incorrectly handle outputs and called `toString` on them in a couple of places. The increased type safety and tests around addon configuration uncovered this. Closes #1369

### Proposed changes  This PR switches the `coredns` and `kube-proxy` addons from self-managed to managed. By default the latest compatible version will be used. This also introduces two new top level arguments to `ClusterOptions` for configuring these new addons. - `corednsAddonOptions` - `kubeProxyAddonOptions` BREAKING CHANGE: creating an `eks.Cluster` will now also create the `coredns` and `kube-proxy` addons. If you are currently already managing these you will need to disable the creation of these through the new arguments `ClusterOptions.corednsAddonOptions.enabled = false` and `ClusterOptions.kubeProxyAddonOptions.enabled = false` ### Related issues (optional) closes #1261, closes #1254

Now that the EKS addons are added we need to align them and do some cleanup. This involves: - adding the enums introduced in #1357 to the VPC CNI - exposing configurationValues for coredns and kube-proxy - removing kubectl from the provider - deeply sort addon configuration keys to guarantee stable json serialization - remove deepmerge again. It caused issues during unit tests (voodoocreation/ts-deepmerge#22) and when used on outputs. Additionally I discovered and fixed an old bug that luckily never surfaced. The VPC CNI configuration did incorrectly handle outputs and called `toString` on them in a couple of places. The increased type safety and tests around addon configuration uncovered this. Closes #1369

corymhall marked this pull request as draft September 12, 2024 16:30

mjeffryes assigned corymhall Sep 13, 2024

corymhall marked this pull request as ready for review September 17, 2024 17:17

corymhall changed the base branch from master to release-3.x.x September 17, 2024 17:18

corymhall added 7 commits September 17, 2024 13:19

Use EKS Addon to manage kube-proxy and coredns

afb908c

closes #1261

prefix name

8f13984

trying to fix test

e32aaa7

Adding ClusterOptions for configuring addons

326b4dd

Fixing linting

eca0e92

wrong cluster...

fc2f0b4

updating default

286d86f

corymhall force-pushed the corymhall/cluster-addons branch from 2c099ee to 286d86f Compare September 17, 2024 17:25

corymhall requested review from flostadler and t0yv0 September 17, 2024 17:26

corymhall commented Sep 17, 2024

View reviewed changes

flostadler reviewed Sep 18, 2024

View reviewed changes

flostadler mentioned this pull request Sep 18, 2024

Move VPC CNI to EKS addon #1358

Merged

pulling resource creation out of apply

cdcd7b9

corymhall requested a review from flostadler September 18, 2024 14:55

adding tags to addons

0121214

t0yv0 reviewed Sep 18, 2024

View reviewed changes

t0yv0 self-requested a review September 18, 2024 16:13

t0yv0 approved these changes Sep 18, 2024

View reviewed changes

flostadler mentioned this pull request Sep 18, 2024

Clean up after addons were introduced #1369

Closed

flostadler approved these changes Sep 18, 2024

View reviewed changes

corymhall merged commit c3622dd into release-3.x.x Sep 18, 2024
34 checks passed

corymhall deleted the corymhall/cluster-addons branch September 18, 2024 17:56

flostadler mentioned this pull request Sep 19, 2024

Align addon configuration #1370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use EKS Addon to manage `kube-proxy` and `coredns` #1357

Use EKS Addon to manage `kube-proxy` and `coredns` #1357

corymhall commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024

corymhall Sep 17, 2024

flostadler Sep 18, 2024

corymhall Sep 18, 2024

flostadler Sep 18, 2024 •

edited

Loading

flostadler Sep 18, 2024

corymhall Sep 18, 2024

flostadler Sep 18, 2024

corymhall Sep 18, 2024

flostadler Sep 18, 2024

flostadler Sep 18, 2024

flostadler Sep 18, 2024

corymhall Sep 18, 2024

flostadler Sep 18, 2024

t0yv0 Sep 18, 2024

flostadler Sep 18, 2024

t0yv0 Sep 18, 2024

corymhall Sep 18, 2024

t0yv0 Sep 18, 2024

t0yv0 Sep 18, 2024

flostadler Sep 18, 2024

corymhall Sep 18, 2024

t0yv0 Sep 18, 2024

flostadler Sep 18, 2024

t0yv0 Sep 18, 2024

t0yv0 Sep 18, 2024

t0yv0 Sep 18, 2024

flostadler Sep 18, 2024

flostadler Sep 18, 2024

corymhall Sep 18, 2024

t0yv0 Sep 18, 2024

t0yv0 commented Sep 18, 2024

flostadler left a comment

Use EKS Addon to manage kube-proxy and coredns #1357

Use EKS Addon to manage kube-proxy and coredns #1357

Conversation

corymhall commented Sep 12, 2024 • edited Loading

Proposed changes

Related issues (optional)

github-actions bot commented Sep 12, 2024

Does the PR have any schema changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flostadler Sep 18, 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

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

t0yv0 commented Sep 18, 2024

flostadler left a comment

Choose a reason for hiding this comment

Use EKS Addon to manage `kube-proxy` and `coredns` #1357

Use EKS Addon to manage `kube-proxy` and `coredns` #1357

corymhall commented Sep 12, 2024 •

edited

Loading

flostadler Sep 18, 2024 •

edited

Loading