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.

Sign up for GitHub

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
Closed

feat: template custom userdata #4504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00ec087
template custom userdata
ekristen Aug 25, 2023
1b55c2b
remove instance types
ekristen Aug 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions pkg/providers/amifamily/bootstrap/custom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)


import (
"bytes"
"encoding/base64"

"github.com/aws/aws-sdk-go/aws"
"github.com/samber/lo"
v1 "k8s.io/api/core/v1"
"text/template"
)

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 •
edited
Loading

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.

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?

Labels map[string]string `hash:"set"`
}

func (e Custom) Script() (string, error) {
return base64.StdEncoding.EncodeToString([]byte(aws.StringValue(e.Options.CustomUserData))), nil
userData, err := e.templateUserData(lo.FromPtr(e.Options.CustomUserData))
if err != nil {
return "", err
}

return base64.StdEncoding.EncodeToString([]byte(userData)), nil
}

func (e Custom) templateUserData(rawUserData string) (string, error) {
tmpl, err := template.New("custom").Parse(rawUserData)
if err != nil {
return "", err
}

data := TemplateData{
Taints: e.Options.Taints,
Labels: e.Options.Labels,
}

var buf bytes.Buffer

if err := tmpl.Execute(&buf, data); err != nil {
return "", err
}

return buf.String(), nil
}
4 changes: 3 additions & 1 deletion pkg/providers/amifamily/custom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ type Custom struct {
}

// UserData returns the default userdata script for the AMI Family
func (c Custom) UserData(_ *v1alpha5.KubeletConfiguration, _ []v1.Taint, _ map[string]string, _ *string, _ []*cloudprovider.InstanceType, customUserData *string) bootstrap.Bootstrapper {
func (c Custom) UserData(_ *v1alpha5.KubeletConfiguration, taints []v1.Taint, labels map[string]string, _ *string, _ []*cloudprovider.InstanceType, customUserData *string) bootstrap.Bootstrapper {
return bootstrap.Custom{
Options: bootstrap.Options{
Taints: taints,
Labels: labels,
CustomUserData: customUserData,
},
}
Expand Down