Allow passing in extra user-data to cloud-init #3633

rdrgmnzs · 2017-10-16T00:13:31Z

This allows for the utilization of multipart mime to pass extra data to cloud-init.

This gives you the ability to utilize cloud-init features such as setting up a CM or monitoring so that existing infrastructure components can be leveraged.

rdrgmnzs · 2017-10-16T00:16:17Z

/assign @justinsb

KashifSaadat · 2017-10-16T13:48:32Z

Hey @brdude. The PR looks good, however is there specific behaviour you're looking to achieve by adding to the user-data that couldn't already be done via use of Hooks within the Cluster Spec? https://github.com/kubernetes/kops/blob/master/docs/cluster_spec.md#hooks

I think we should be cautious with allowing people to modify the user-data, as there is a 16KB limit with AWS so you could potentially break the cluster provisioning process.

rdrgmnzs · 2017-10-16T17:26:21Z

@KashifSaadat it's not so much that it could not be done but the fact that the level of engineering required to port everything that currently exists in the infrastructure that uses user-data would be a huge burden and take an immense amount of engineering.

Examples of what currently is done by user-data and what it's used for in my infra:

Configuration management setup.
Monitoring setup
Amongst others things.

Would checking the size of user-data at creation time and failing if it exceeds the size be an appropriate solution here?

chrislovecnm · 2017-10-16T20:35:38Z

/assign @justinsb

PTAL when you have a chance. I am kinda on the fence with this one. If it goes through we need to not have the user blow up there user data.

rdrgmnzs · 2017-10-16T20:42:14Z

@chrislovecnm when you say blow up their user data, are you referring to the size of the user data or if there is an error in their user data?

For the first we can add a check to ensure the user-data does not exceed 16KB, for the second if a part of the user data is broken cloud-init will just fail for that piece but still run the other cloud-init data componets passed to it.

chrislovecnm · 2017-10-16T20:43:53Z

For the first we can add a check to ensure the user-data does not exceed 16KB

^ yes

Let @justinsb take a look and get his feedback. Not saying no, it just appears we are giving the user the same functionality to do the same thing. Just my thought.

justinsb · 2017-10-21T03:08:06Z

I think we're trying to encourage hooks and daemonsets. cloud-init isn't something I think we can support for the long-haul (e.g. the differences between what coreos supports & debian supports).

As a compromise, how about we produce those example hooks or daemonsets for the use cases you're trying to achieve? e.g. there's a daemonset for NewRelic at https://github.com/kubernetes/examples/blob/master/staging/newrelic/README.md It is different from cloud-init, but it lets us use kubernetes management for these things, rather than having two classes of components.

rdrgmnzs · 2017-10-21T19:38:40Z

@justinsb at least for my use case unfortunately the NewRelic daemon set wouldn't solve our issues. We actually use user-data for other things besides cloud-init. We have daemon sets on boxes that do access management based on the user-data, since it cannot be changed without a reboot. We also have another daemon that uses other parts of user-data for identity management and ownership identification. These are just a couple examples.

The reason I used cloud-init as the factor in the title is because that is what most people use it for.

Unfortunately I had not realized that coreOS does not support multi-part mime. Would setting this up so that we only encode the user-data as a multipart mime if the user passes in EXTRA_USER_DATA be an acceptable middle ground? This way support for both types of user-data (Debian and CoreOS) would be possible.

justinsb · 2017-11-04T05:44:46Z

docs/extra_user-data.md

+
+### Adding additonal user-data
+
+Aditional user-user data can be passed to the host provissioning by utilizing the environment variable EXTRA_USER_DATA. By setting `EXTRA_USER_DATA` to `<file>:<content-type>` kops will load these aditional files and pass them on to the hosts. A list of valid content-types can be found [here](http://cloudinit.readthedocs.io/en/latest/topics/format.html#mime-multi-part-archive) 


Nit: typo provisioning

justinsb · 2017-11-04T05:49:43Z

So actually I think this is OK, but with one tweak: I think we should not set up multipart if EXTRA_USER_DATA is not passed. Then for most people, there will be no change, and if you opt-in, you get multipart.

Then a few suggestions:

I suggest calling the env var KOPS_EXTRA_USER_DATA, just in case someone has EXTRA_USER_DATA set (not sure how likely this is, but the KOPS_ prefix makes this reasonable IMO). But...
... I would also consider that this may belong in the cluster spec or instancegroup spec? Otherwise you have to pass KOPS_EXTRA_USER_DATA to every kops command. But on the other hand for your use case I'm guessing that you don't want the user data in the instance group spec maybe?

Sorry this took so long for me to get my head around. If we can change it so that we don't do multipart unless the variable is set, lgtm!

justinsb · 2017-11-04T05:49:57Z

Going to close & reopen to retrigger e2e as well

rdrgmnzs · 2017-11-07T22:53:00Z

Going to close & reopen to retrigger e2e

justinsb · 2017-11-09T15:57:59Z

pkg/apis/kops/validation/instancegroup.go

+
+func validateExtraUserData(userData *kops.ExtraUserDataSpec) error {
+	if userData.Name == "" {
+		return field.Required(field.NewPath("ExtraUserData Name"), "extraUserData Name field must be set")


So we are supposed to pass the parent field.Path into a validate function, and then it is fieldPath.Child("Name") i.e. just the name of the field.

justinsb · 2017-11-09T15:59:28Z

pkg/model/bootstrapscript.go

@@ -160,7 +160,12 @@ func (b *BootstrapScript) ResourceNodeUp(ig *kops.InstanceGroup, cs *kops.Cluste
 		},
 	}

-	templateResource, err := NewTemplateResource("nodeup", resources.AWSNodeUpTemplate, functions, nil)
+	AWSNodeUpTemplate, err := resources.AWSNodeUpTemplate(ig)


Nit: typically we prefer lower case for local variables, e.g. awsNodeUpTempler, err...

justinsb · 2017-11-09T15:59:51Z

pkg/model/resources/nodeup.go

@@ -170,3 +180,70 @@ __EOF_KUBE_ENV
 download-release
 echo "== nodeup node config done =="
 `
+
+// AWSNodeUpTemplate returns a Mime Multi Part Archive container the nodeup (bootstrap) script
+// and any aditional User Data passed to it in files using the env variable KOPS_EXTRA_USER_DATA


Nit: comment is out of date (KOPS_EXTRA_USER_DATA)

justinsb · 2017-11-09T16:00:21Z

pkg/model/resources/nodeup.go

+
+		mimeWriter := multipart.NewWriter(writer)
+
+		// we explicitly set the buoudaries to make testing easier.


Nit: typo in boundaries

justinsb · 2017-11-09T16:01:34Z

pkg/apis/kops/validation/instancegroup.go

+	case "text/cloud-config":
+	case "text/part-handler":
+	case "text/x-shellscript":
+	case "text/cloud-boothook":


Do you need all of these? My concern is that if we specify e.g. x-shellscript, it might interfere with normal boot in non-trivial ways.

The only one I need personally is "text/cloud-config". Just though I'd add all the valid types to have inclusive support since we're already putting support in. However I have nothibg against removing x-shellscript.

I think we probably should, for safety, but I also want to get this PR in, so maybe we look at that in a separate PR :-)

(We can always add things to the set of permitted types, much harder to remove things)

justinsb · 2017-11-09T16:02:15Z

pkg/client/clientset_generated/clientset/scheme/register.go

@@ -17,15 +17,14 @@ limitations under the License.
 package scheme

 import (
-	os "os"


We're trying to move to goimports formatted code... not sure what happened here :-)

Yeah not sure what happened either I believe this was done when I generated the API for userData :-/

justinsb · 2017-11-09T16:03:19Z

pkg/model/resources/nodeup.go

+// and any aditional User Data passed to it in files using the env variable KOPS_EXTRA_USER_DATA
+func AWSNodeUpTemplate(ig *kops.InstanceGroup) (string, error) {
+
+	UserDataTemplate := NodeUpTemplate


Nit: userDataTemplate

justinsb · 2017-11-09T16:06:21Z

pkg/apis/kops/instancegroup.go

@@ -109,6 +109,18 @@ type InstanceGroupSpec struct {
 	Kubelet *KubeletConfigSpec `json:"kubelet,omitempty"`
 	// Taints indicates the kubernetes taints for nodes in this group
 	Taints []string `json:"taints,omitempty"`
+	// ExtraUserData is any extra user-data to be passed to the host
+	ExtraUserData []ExtraUserDataSpec `json:"extraUserData,omitempty"`


For better or worse, we've called this additionalX previously...

How do you feel about:

AdditionalUserData []UserDataSpec json:"additionalUserData,omitempty"``

justinsb · 2017-11-09T16:07:34Z

pkg/apis/kops/instancegroup.go

+}
+
+// ExtraUserDataSpec defines a user-data section
+type ExtraUserDataSpec struct {


This can also be AdditionalUserDataSpec or just UserDataSpec, or maybe even UserData.

My 2c: UserData

justinsb · 2017-11-09T16:08:40Z

Looks great - API is the one thing we really can't change easily, so how would you feel about naming the field additionalUserData - I think we've used "additional" rather than "extra" everywhere else. Everything else is just nits / suggestions for the future :-)

rdrgmnzs · 2017-11-09T21:17:27Z

Quick note, looks like AdditionalSANs did not have API machinery ran on it so this includes changes for that as well.

chrislovecnm · 2017-11-09T21:29:26Z

@brdude I will PR api machinery update ... and open api as well.

chrislovecnm · 2017-11-09T21:35:12Z

@brdude #3815 will do the update. Thanks for the catch. We have an open issue to test API machinery generation during CI.

justinsb · 2017-11-10T04:30:11Z

/lgtm

k8s-github-robot · 2017-11-10T04:30:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-10T04:30:58Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-10T05:03:14Z

Automatic merge from submit-queue.

Looking at https://godoc.org/k8s.io/kops/pkg/apis/kops#InstanceGroupSpec and kubernetes#3633, the field is `AdditionalUserData` not `ExtraUserData`

Automatic merge from submit-queue. Update Additional user-data per #3853 Looking at https://godoc.org/k8s.io/kops/pkg/apis/kops#InstanceGroupSpec and #3633, the field is `AdditionalUserData` not `ExtraUserData` This addresses #3853

Allow passing in extra user-data to cloud-init

e77cda1

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2017

k8s-ci-robot assigned justinsb Oct 16, 2017

Merge remote-tracking branch 'upstream/master' into extra_user-data

75aa120

rdrgmnzs added 2 commits October 17, 2017 14:10

Merge remote-tracking branch 'upstream/master' into extra_user-data

469b735

Merge remote-tracking branch 'upstream/master' into extra_user-data

50c7094

KashifSaadat mentioned this pull request Oct 19, 2017

Support for adding user snippets to the user-data -- Perhaps a systemd unit abstraction ? #789

Closed

rdrgmnzs added 2 commits October 24, 2017 12:52

Merge remote-tracking branch 'upstream/master' into extra_user-data

d803480

Merge remote-tracking branch 'origin/master' into extra_user-data

58faa71

justinsb reviewed Nov 4, 2017

View reviewed changes

justinsb closed this Nov 4, 2017

justinsb reopened this Nov 4, 2017

rdrgmnzs changed the title ~~Allow passing in extra user-data to cloud-init~~ WIP: Allow passing in extra user-data to cloud-init Nov 4, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2017

rdrgmnzs added 2 commits November 4, 2017 17:06

Merge remote-tracking branch 'upstream/master' into extra_user-data

565afae

Move user-data to IG spec

1fe56a1

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 7, 2017

rdrgmnzs closed this Nov 7, 2017

rdrgmnzs reopened this Nov 7, 2017

rdrgmnzs changed the title ~~Allow passing in extra user-data to cloud-init~~ WIP: Allow passing in extra user-data to cloud-init Nov 8, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2017

rdrgmnzs changed the title ~~WIP: Allow passing in extra user-data to cloud-init~~ Allow passing in extra user-data to cloud-init Nov 8, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2017

Merge remote-tracking branch 'upstream/master' into extra_user-data

3cfa67c

justinsb reviewed Nov 9, 2017

View reviewed changes

rdrgmnzs force-pushed the extra_user-data branch 2 times, most recently from f077e79 to 6aa7de8 Compare November 9, 2017 21:27

rdrgmnzs force-pushed the extra_user-data branch from 6aa7de8 to d85ea8e Compare November 9, 2017 22:06

Fixing naming convention and adding running API machinery.

ef24cec

rdrgmnzs force-pushed the extra_user-data branch from d85ea8e to ef24cec Compare November 9, 2017 22:30

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2017

k8s-github-robot merged commit 8739632 into kubernetes:master Nov 10, 2017

dgem added a commit to dgem/kops that referenced this pull request Nov 14, 2017

Update Additional user-data example

f0c5b6a

Looking at https://godoc.org/k8s.io/kops/pkg/apis/kops#InstanceGroupSpec and kubernetes#3633, the field is `AdditionalUserData` not `ExtraUserData`

dgem mentioned this pull request Nov 14, 2017

Update Additional user-data per #3853 #3854

Merged

rdrgmnzs deleted the extra_user-data branch April 19, 2018 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing in extra user-data to cloud-init #3633

Allow passing in extra user-data to cloud-init #3633

rdrgmnzs commented Oct 16, 2017

rdrgmnzs commented Oct 16, 2017

KashifSaadat commented Oct 16, 2017

rdrgmnzs commented Oct 16, 2017 •

edited

Loading

chrislovecnm commented Oct 16, 2017

rdrgmnzs commented Oct 16, 2017

chrislovecnm commented Oct 16, 2017

justinsb commented Oct 21, 2017

rdrgmnzs commented Oct 21, 2017 •

edited

Loading

justinsb Nov 4, 2017

justinsb commented Nov 4, 2017

justinsb commented Nov 4, 2017

rdrgmnzs commented Nov 7, 2017

justinsb Nov 9, 2017

justinsb Nov 9, 2017

justinsb Nov 9, 2017

justinsb Nov 9, 2017

justinsb Nov 9, 2017

rdrgmnzs Nov 9, 2017

justinsb Nov 10, 2017

justinsb Nov 10, 2017

justinsb Nov 9, 2017

rdrgmnzs Nov 9, 2017

justinsb Nov 9, 2017

justinsb Nov 9, 2017

justinsb Nov 9, 2017

justinsb commented Nov 9, 2017

rdrgmnzs commented Nov 9, 2017

chrislovecnm commented Nov 9, 2017

chrislovecnm commented Nov 9, 2017

justinsb commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017


		### Adding additonal user-data

		Aditional user-user data can be passed to the host provissioning by utilizing the environment variable EXTRA_USER_DATA. By setting `EXTRA_USER_DATA` to `<file>:<content-type>` kops will load these aditional files and pass them on to the hosts. A list of valid content-types can be found [here](http://cloudinit.readthedocs.io/en/latest/topics/format.html#mime-multi-part-archive)


		mimeWriter := multipart.NewWriter(writer)

		// we explicitly set the buoudaries to make testing easier.

Allow passing in extra user-data to cloud-init #3633

Allow passing in extra user-data to cloud-init #3633

Conversation

rdrgmnzs commented Oct 16, 2017

rdrgmnzs commented Oct 16, 2017

KashifSaadat commented Oct 16, 2017

rdrgmnzs commented Oct 16, 2017 • edited Loading

chrislovecnm commented Oct 16, 2017

rdrgmnzs commented Oct 16, 2017

chrislovecnm commented Oct 16, 2017

justinsb commented Oct 21, 2017

rdrgmnzs commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

justinsb commented Nov 4, 2017

justinsb commented Nov 4, 2017

rdrgmnzs commented Nov 7, 2017

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

justinsb commented Nov 9, 2017

rdrgmnzs commented Nov 9, 2017

chrislovecnm commented Nov 9, 2017

chrislovecnm commented Nov 9, 2017

justinsb commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

rdrgmnzs commented Oct 16, 2017 •

edited

Loading

rdrgmnzs commented Oct 21, 2017 •

edited

Loading