Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: template custom userdata #4504

Closed
wants to merge 2 commits into from

Conversation

ekristen
Copy link

Fixes #N/A

Description

Allows Custom User Data for amiFamily Custom to be templated. This basically allows labels and taints to be templated into the user data which is useful for custom deployments. For example I am using RKE1 (from Rancher) with Rancher on EC2 instances and bootstrap the ec2 nodes using custom data.

Example template (pseudo code, not full)

sudo docker run -d --privileged --restart=unless-stopped --net=host -v /etc/kubernetes:/etc/kubernetes -v /var/run:/var/run rancher/rancher-agent:v2.6.9 \
      --server https://rancher2 \
      --token token \
      --ca-checksum checksum \
      {{ with .Labels }}{{ range $k, $v := . }}--label {{ $k }}={{ $v }} {{ end }} \{{ end }}
      --worker

How was this change tested?

Deployment into clusters, struggled to get the test suite working with lack of docs (or couldn't locate it)

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

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

@ekristen ekristen requested a review from a team as a code owner August 26, 2023 20:02
@ekristen ekristen requested a review from tzneal August 26, 2023 20:02
@netlify
Copy link

netlify bot commented Aug 26, 2023

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 1b55c2b
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/64ea5a653246d20008f4e8c5
😎 Deploy Preview https://deploy-preview-4504--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

)

type Custom struct {
Options
}

type TemplateData struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool! I'm curious on a couple of things:

  1. Do you think that this kind of templating might be useful for other AMIFamilies within Karpenter?
  2. Karpenter currently has a "node registration flow" which adds all of the labels that Karpenter is tracking on its Machine CustomResource onto the Node when the Node joins the cluster. Is this mechanism sufficient to get the labels onto the node or do you also need these labels passed through into the userData?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be useful for sure. It all depends on how each registers though. I’m not as familiar with the others.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at like bottlerocket, it wouldn't be useful because it's already passing in taints and labels via it's special toml configuration https://github.com/aws/karpenter/blob/main/pkg/providers/amifamily/bootstrap/bottlerocket.go#L47-L49

So I feel it's likely to be implementation specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you think this templating plays with the ideas presented here: https://github.com/aws/karpenter/issues/4459? One thought that we had was to drop a file onto the filesystem that you could modify and pull data from rather than having to template the userData like this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a different problem entirely as it assumes EKS. This simple just makes the data available to the custom user data. At least with the custom user data it's expected to be passed through the CRD definition. For the others the file already exists or is generated it seems depending on the family.

Copy link
Contributor

@ellistarn ellistarn Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On first glance, this is a brilliant idea imo. It gives the user access to karpenters state (runtime interface) at launch time, without assuming anything about the file system. It's even user data format agnostic, i.e. could work with bash or toml.

To Jonathan's point, I want us to be very careful about what exactly we expose in this template, as it will become something we support forever.

Specifically on labels and taints, we've been hoping to shift label and taints registration to the controller, instead of the bootstrap. Does that mean you should be able to know the labels at bootstrap time? I don't know -- maybe.

Edit: we're gonna need a security review on this, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To Ellis's point @ekristen, do you need these taints and labels added in the userData script if they are added to the Node at node-join time when the the node first starts up?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I guess in my initial testing this wasn't happening but for different reasons at least labels work. I'll need to verify taints.

I don't use cloud provider operators so I had to write a shim to get the cloud provider id to be set so it would link up to karpenter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use cloud provider operators so I had to write a shim to get the cloud provider id to be set so it would link up to karpenter.

I could still see this being extremely useful for things like the kubeletConfiguration if you are trying to inject the kubelet args that Karpenter is aware of into your userData script. I think, in general, I'm supportive of this templating mechanism since we can expand it out to other areas of Karpenter configuration that might affect userData.

@ekristen
Copy link
Author

ekristen commented Sep 7, 2023

@ellistarn let me know. it would be nice to have.

@ellistarn
Copy link
Contributor

@jonathan-innis to take the lead here

)

type Custom struct {
Options
}

type TemplateData struct {
Taints []v1.Taint `hash:"set"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check, what are the hash annotations here for?

@@ -15,15 +15,47 @@ limitations under the License.
package bootstrap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some test cases into the User Data section of the launch template file (see https://github.com/aws/karpenter/blob/main/pkg/providers/launchtemplate/suite_test.go#L842)

@github-actions
Copy link
Contributor

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants