feat: Refactor Helm chart #1145

stevehipwell · 2022-01-14T14:21:42Z

1. Issue, if available:
This PR addresses part of #781.

2. Description of changes:

This is a breaking change.

Improve chart metadata
Make chart follow idiomatic Helm patterns
Use K8s recommended labelling to distinguish between components and improve selector logic
Use a different ServiceAccount for controller and webhook, this is more secure
Recommend creating service accounts rather than using the namespace default, this is more secure

3. How was this change tested?

The template output was compared to the pervious version.

4. Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: link to issue
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2022-01-14T14:21:50Z

✔️ Deploy Preview for karpenter-docs-prod ready!

🔨 Explore the source changes: 24f2876

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6203f7d61e7bed00079be390

😎 Browse the preview: https://deploy-preview-1145--karpenter-docs-prod.netlify.app

stevehipwell · 2022-01-14T14:30:42Z

@ellistarn sorry it took me so long to come through on this one, but with the holidays and the chart refactor I did for NTH taking longer than expected I've got quite a backlog of pending OSS commitments. I'm about to go through the issues and comment on the ones that this PR is related to.

charts/karpenter/Chart.yaml

stevehipwell · 2022-01-20T09:39:24Z

@ellistarn have you had a chance to review this yet?

bwagner5 · 2022-01-20T19:15:21Z

I'll take a look at this today @stevehipwell ! Looks familiar ;)

bwagner5

I haven't quite finished reviewing all the files, but had a question on the serviceAccount

bwagner5 · 2022-01-20T19:17:04Z

Makefile

@@ -71,7 +71,6 @@ apply: ## Deploy the controller into your ~/.kube/config cluster
 delete: ## Delete the controller from your ~/.kube/config cluster
 	helm template karpenter charts/karpenter --namespace karpenter \
 		$(HELM_OPTS) \
-		--set serviceAccount.create=false \


this was set to prevent the service account annotated for IRSA to be deleted in the dev workflow.

Shouldn't the annotation be passed in with --set flag? Otherwise you're using the namespace default Service Account and giving it all the access. I guess for development it doesn't really matter, but this pattern was also copied everywhere as is often the case.

We thought users would be using the getting started guide eksctl command to create the IAM Role for Service Accounts, which creates the service account and annotates with the role arn (https://karpenter.sh/v0.5.5/getting-started/#create-the-karpentercontroller-iam-role). That might be a bad assumption for production installs, I'm not sure. But for dev installs, it's probably accurate and so leaving the --set serviceAccount.controller.create=false seems like the best dev workflow experience to me. Otherwise, we'd need some way of persisting the role arn in an env var or something and then using a set to annotate every time we make apply.

That guide really needs re-writing to promote better practice, although from memory one of the other tabs has a better way of creating the roles and getting the ARN back? It's idiomatic for a Helm chart to create it's own service account(s), so I'd suggest documenting the externally created one, but with a name specified so it's not the default namespace one.

For the dev workflow, once the role has been created it persists outside of the K8s resources. It'd be simplest to set it in the env.

The docs that need updating are here (I think that's all of the places):

https://karpenter.sh/v0.6.0/getting-started/#install-karpenter-helm-chart (may want to move the helm install docs above the spot service linked role docs since we need to pass thru the annotation now. Probably want to add --role-only to the eksctl cmd to prevent creating the service account too)

https://karpenter.sh/v0.6.0/getting-started-with-terraform/#install-karpenter-helm-chart

https://karpenter.sh/v0.6.0/development-guide/#environment-specific-setup

@bwagner5 shouldn't we be waiting until there is a release to make the changes to the docs for that release? The v0.6.0 chart has already been released so I don't think the docs shouldn't change for that version? This is one of the problems of directly linking your chart releases to your package releases.

While I wait for a response I'll update the non-docs references and undo the change to docs I accidentally made previously.

@bwagner5 @ellistarn how do the docs get updated for a release?

All docs before a release go in the preview directory https://github.com/aws/karpenter/tree/main/website/content/en/preview

When we cut a release, the preview directory is copied to the version we are releasing, so it's fine to make the doc updates to preview in this PR. We shouldn't ever modify a specific version docs outside of a release or a bug fix in the docs.

These should be updated now.

ellistarn · 2022-01-21T18:05:17Z

FYI related: https://github.com/aws/karpenter/pull/914/files

charts/karpenter/templates/controller/serviceaccount.yaml

bwagner5 · 2022-02-01T23:15:56Z

website/content/en/v0.5.5/getting-started/_index.md

@@ -142,7 +142,7 @@ eksctl. Thus, we don't need the helm chart to do that.
 helm repo add karpenter https://charts.karpenter.sh
 helm repo update
 helm upgrade --install karpenter karpenter/karpenter --namespace karpenter \
-  --create-namespace --set serviceAccount.create=false --version v0.5.5 \


IIUC this should include the IRSA annotations if we remove this, or else it will override the service account created above with eksctl?

bwagner5 · 2022-02-01T23:20:28Z

Makefile

@@ -71,7 +71,6 @@ apply: ## Deploy the controller into your ~/.kube/config cluster
 delete: ## Delete the controller from your ~/.kube/config cluster
 	helm template karpenter charts/karpenter --namespace karpenter \
 		$(HELM_OPTS) \
-		--set serviceAccount.create=false \


The instance profile shouldn't make a difference since it's for the nodes that karpenter provisions and not the karpenter controller instance role.

charts/karpenter/templates/controller/serviceaccount.yaml

bwagner5

Nice! Just one comment on a mistyped command in the docs

website/content/en/preview/getting-started/_index.md

Signed-off-by: Steve Hipwell <[email protected]>

stevehipwell · 2022-02-08T09:13:55Z

@bwagner5 thanks for the review, I've fixed the typo you found. Hopefully this can be merged soon.

stevehipwell · 2022-02-08T15:37:35Z

@bwagner5 the CI is failing but I don't think it's related to any changes I made?

bwagner5 · 2022-02-08T19:59:58Z

I pulled this down and did some manual testing. I posted a PR to your fork with a few changes: stevehipwell#1

We're very near a merge! I love the single karpenter pod!

stevehipwell · 2022-02-09T09:49:48Z

@bwagner5 thanks for the review and the PR.

mbevc1 · 2022-02-09T10:12:31Z

Nice work and also glad to see consolidating Pod too 👍

stevehipwell · 2022-02-09T17:21:13Z

@bwagner5 all merged and in this PR.

bwagner5

LGTM! Thanks!!

… broken down to after #1145 (#1301)

… broken down to after #1145 for v0.6.1 (#1305)

Skarlso · 2022-02-16T17:14:13Z

In the future please note that refactors like these break eksctl's Karpenter installation flow. A heads up would have been nice.

stevehipwell · 2022-02-16T17:16:43Z

@Skarlso I'm not a maintainer just a contributor. That said I was expecting this to go out as v0.7.0 to clearly show that it was a breaking change, but as per SemVer 2.0 all pre v1 versions are considered breaking.

metanerd · 2022-02-18T13:43:41Z

Hi! I just want to mention that this refactor also broke the values for this aws sample: https://github.com/aws-samples/karpenter-terraform/blob/main/karpenter/templates/values.yaml.tpl

stevehipwell · 2022-02-18T14:25:16Z

@metanerd I'm just a contributor so just my opinion, but all samples SHOULD use fixed versions and all samples MUST fix the version of any pre v1.0.0 components.

stevehipwell mentioned this pull request Jan 14, 2022

RBAC is too permissive #973

Closed

mbevc1 reviewed Jan 14, 2022

View reviewed changes

charts/karpenter/Chart.yaml Outdated Show resolved Hide resolved

stevehipwell force-pushed the refactor-helmp-chart branch from a5f2e17 to 0258fd4 Compare January 20, 2022 09:37

bwagner5 self-requested a review January 20, 2022 19:12

bwagner5 reviewed Jan 21, 2022

View reviewed changes

charts/karpenter/templates/controller/serviceaccount.yaml Outdated Show resolved Hide resolved

charts/karpenter/templates/controller/serviceaccount.yaml Outdated Show resolved Hide resolved

stevehipwell force-pushed the refactor-helmp-chart branch 3 times, most recently from f30e5f6 to c46f599 Compare February 1, 2022 12:13

stevehipwell mentioned this pull request Feb 1, 2022

Decouple Helm chart release from Karpenter release #1253

Closed

bwagner5 reviewed Feb 1, 2022

View reviewed changes

stevehipwell force-pushed the refactor-helmp-chart branch 2 times, most recently from af2aa1a to 0e12bb4 Compare February 4, 2022 12:41

stevehipwell mentioned this pull request Feb 4, 2022

feat: added annotations and debug level to the helm chart #1274

Closed

3 tasks

stevehipwell force-pushed the refactor-helmp-chart branch 4 times, most recently from 30103c0 to 7ef9e4b Compare February 7, 2022 16:57

bwagner5 requested changes Feb 8, 2022

View reviewed changes

website/content/en/preview/getting-started/_index.md Outdated Show resolved Hide resolved

stevehipwell force-pushed the refactor-helmp-chart branch 2 times, most recently from 2f5990b to d68e66a Compare February 8, 2022 09:07

feat: Refactor Helm chart

2d5656d

Signed-off-by: Steve Hipwell <[email protected]>

stevehipwell force-pushed the refactor-helmp-chart branch from d68e66a to 2d5656d Compare February 8, 2022 09:11

felix-zhe-huang mentioned this pull request Feb 8, 2022

Add support for custom resources annotations on helm chart #1263

Closed

bwagner5 added 3 commits February 9, 2022 17:20

helm fixes

080be6d

use a regular helm install w/ make apply and make delete

b4eed34

karpenter service name is configurable for webhook cert now

24f2876

bwagner5 approved these changes Feb 9, 2022

View reviewed changes

bwagner5 merged commit 7e95b62 into aws:main Feb 9, 2022

stevehipwell deleted the refactor-helmp-chart branch February 9, 2022 17:32

spring1843 mentioned this pull request Feb 9, 2022

Correct broken links to rbac.yaml #1301

Merged

3 tasks

bwagner5 pushed a commit that referenced this pull request Feb 9, 2022

Replaces the link to non-existent rbac.yaml into the four files it is…

0d1c5d0

… broken down to after #1145 (#1301)

spring1843 mentioned this pull request Feb 9, 2022

Correct broken links to rbac.yaml for v0.6.1 #1305

Merged

3 tasks

bwagner5 pushed a commit that referenced this pull request Feb 9, 2022

Replaces the link to non-existent rbac.yaml into the four files it is…

5ddf71b

… broken down to after #1145 for v0.6.1 (#1305)

Skarlso mentioned this pull request Feb 16, 2022

Error: failed to install Karpenter: failed to install Karpenter chart: failed to install chart: create: failed to create: namespaces "karpenter" not found eksctl-io/eksctl#4804

Closed

flf2ko mentioned this pull request Jul 5, 2022

fix: hostNetwork variable is not aligned with docs in Helm chart (#2043) #2044

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor Helm chart #1145

feat: Refactor Helm chart #1145

stevehipwell commented Jan 14, 2022

netlify bot commented Jan 14, 2022 •

edited

Loading

stevehipwell commented Jan 14, 2022

stevehipwell commented Jan 20, 2022

bwagner5 commented Jan 20, 2022

bwagner5 left a comment

bwagner5 Jan 20, 2022

stevehipwell Jan 21, 2022

bwagner5 Jan 21, 2022

stevehipwell Jan 21, 2022

stevehipwell Jan 21, 2022

bwagner5 Feb 1, 2022

stevehipwell Feb 4, 2022

stevehipwell Feb 4, 2022

bwagner5 Feb 4, 2022 •

edited

Loading

stevehipwell Feb 7, 2022

ellistarn commented Jan 21, 2022

bwagner5 Feb 1, 2022

bwagner5 Feb 1, 2022

bwagner5 left a comment

stevehipwell commented Feb 8, 2022

stevehipwell commented Feb 8, 2022

bwagner5 commented Feb 8, 2022

stevehipwell commented Feb 9, 2022

mbevc1 commented Feb 9, 2022

stevehipwell commented Feb 9, 2022

bwagner5 left a comment

Skarlso commented Feb 16, 2022

stevehipwell commented Feb 16, 2022

metanerd commented Feb 18, 2022

stevehipwell commented Feb 18, 2022

feat: Refactor Helm chart #1145

feat: Refactor Helm chart #1145

Conversation

stevehipwell commented Jan 14, 2022

netlify bot commented Jan 14, 2022 • edited Loading

stevehipwell commented Jan 14, 2022

stevehipwell commented Jan 20, 2022

bwagner5 commented Jan 20, 2022

bwagner5 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwagner5 Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwagner5 left a comment

Choose a reason for hiding this comment

stevehipwell commented Feb 8, 2022

stevehipwell commented Feb 8, 2022

bwagner5 commented Feb 8, 2022

stevehipwell commented Feb 9, 2022

mbevc1 commented Feb 9, 2022

stevehipwell commented Feb 9, 2022

bwagner5 left a comment

Choose a reason for hiding this comment

Skarlso commented Feb 16, 2022

stevehipwell commented Feb 16, 2022

metanerd commented Feb 18, 2022

stevehipwell commented Feb 18, 2022

netlify bot commented Jan 14, 2022 •

edited

Loading

bwagner5 Feb 4, 2022 •

edited

Loading