Cluster / InstanceGroup File Assets #3090

gambol99 · 2017-07-30T20:35:45Z

The current implementation does not make it ease to fully customize nodes before kube install. This PR adds the ability to include file assets in the cluster and instaneGroup spec which can be consumed by nodeup. Allowing those whom need (i.e. me :-)) greater flexibilty around their nodes. @note, nothing is enforced, so unless you've specified anything everything is as the same

updated the cluster_spec.md to reflect the changes
permit users to place inline files into the cluster and instance group specs
added the ability to template the files, the Cluster and InstanceGroup specs are passed into context
cleaned up and missed comment, unordered imports etc along the journey

notes: In addition to this; need to look at the detecting the changes in the cluster and instance group spec. Think out loud perhaps using a last_known_configuration annotation, similar to kubernetes

k8s-ci-robot · 2017-07-30T20:35:53Z

Hi @gambol99. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gambol99 · 2017-07-31T00:06:47Z

/assign @justinsb

gambol99 · 2017-07-31T11:31:17Z

nodeup/pkg/model/file_assets.go

+		}
+
+		// @check if the directory structure exist or create it
+		if err := os.MkdirAll(filepath.Dir(asset.Path), 0755); err != nil {


just realized i could use instead

c.AddTask(&nodetasks.File{ Path: filepath.Dir(asset.Path), Type: nodetasks.FileType_Directory, Mode: s("0700"), Owner: s("root"), Group: s("root"), })

gambol99 · 2017-08-02T19:47:08Z

a friendly ping :-) @justinsb ... Note we've also raised #3120 so changes would be detectable

justinsb · 2017-08-06T23:04:53Z

So not saying "no" here, but the long term direction is towards immutable infrastructure and OS independence, and I worry that this is setting us up for something we don't really want to encourage.

Would you mind describing some of the use-cases (you can always let me know privately if they are sensitive). Hooks are still early, but they are likely to be a better way IMO, and it does seem that we can do anything in them!

For example, rather than do iptables-restore, the hook could install your rules even when we swap out iptables. Or a daemonset could not apply the iptables rules, but also continue to synchronize them even if they somehow got out of sync - and also makes it easier to update the ruleset because you can then use kubernetes to manage it.

That said I recognize that just dropping a file is the path of least resistance, and we don't want to make things unnecessarily hard for you (or anyone!)

k8s-github-robot · 2017-08-08T00:03:14Z

@gambol99 PR needs rebase

gambol99 · 2017-08-09T21:55:56Z

hi @justinsb ... similar lines to the #3063 PR ... the driving force provide this was a concrete, versioned, immutable and tagged version for production. I becomes difficult to test and ultimately provide assurance if you have a bunch on third party includes via daemonset's and external pipelines.

gambol99 · 2017-08-11T11:37:06Z

this also permit's us to use a token webhook and place the config on box without having to go via an external hook

gambol99 · 2017-08-14T09:57:24Z

hey @justinsb ... is there any other chances you want made to this? ...

justinsb · 2017-08-17T13:07:37Z

docs/cluster_spec.md

@@ -249,6 +249,43 @@ spec:
      image: busybox
 ```

+### fileAssets
+
+FileAssets permit you to place inline file content into the cluster and instanceGroup specification, which can be consumed on the nodes


I would be much more comfortable if this was marked as an alpha feature or similar.

justinsb · 2017-08-17T13:09:57Z

nodeup/pkg/model/file_assets.go

+			return fmt.Errorf("The file asset is invalid, name: %s, error: %q", asset.Name, err)
+		}
+		// @check if the file asset applys to us. If no roles applied we assume its applied to all roles
+		// @todo: use the containsRole when the hooks PR is merged


Looks like we merged containsRole :-)

justinsb · 2017-08-17T13:15:55Z

pkg/apis/kops/v1alpha1/cluster.go

+	// Path is the location this file should reside
+	Path string `json:"path,omitempty"`
+	// Mode is unix filemode for the file - defaults to 0400
+	Mode string `json:"mode,omitempty"`


Do you need the mode? I'm more worried about execute permissions and noexec than anything else here...

justinsb · 2017-08-17T13:16:18Z

pkg/apis/kops/v1alpha1/cluster.go

+	// Name is a shortened reference to the asset
+	Name string `json:"name,omitempty"`
+	// Path is the location this file should reside
+	Path string `json:"path,omitempty"`


Would you be OK with putting the assets all into the same directory i.e. dropping this field? I'm thinking then we don't have to worry about immutable base images, and we also could in theory deliver the assets via another mechanism, i.e. just like configmap in a pod.

justinsb · 2017-08-17T13:17:54Z

pkg/apis/kops/v1alpha1/cluster.go

+	// Content is the contents of the file
+	Content string `json:"content,omitempty"`
+	// Templated indicates the content is templated
+	Templated bool `json:"templated,omitempty"`


If we do want to allow Templated, we have to be really careful - it ties us to a particular version of the API objects. So we should be specifying the API version somehow. But I think I would rather not include Templated at all, and push people to hooks, because our experience of go templating is that they rapidly become unmaintainable.

justinsb · 2017-08-17T13:19:03Z

pkg/apis/kops/instancegroup.go

@@ -94,6 +94,8 @@ type InstanceGroupSpec struct {
 	CloudLabels map[string]string `json:"cloudLabels,omitempty"`
 	// NodeLabels indicates the kubernetes labels for nodes in this group
 	NodeLabels map[string]string `json:"nodeLabels,omitempty"`
+	// FileAssets is a collection of file assets for this instance group
+	FileAssets []*FileAssetSpec `json:"fileAssets,omitempty"`


Can we change all the API []*FileAssetSpec to []FileAssetSpec please? I know we have a lot of []*T but API machinery is happier with []T

justinsb · 2017-08-17T13:42:24Z

OK, I've been thinking about this a lot, and I think we should do this PR, but I'd suggest a few tweaks:

The mental model I'm using is secrets or configmap, which exist independently, can be mounted on the filesystem, but could also be delivered by other mechanisms today or in future (e.g. a secure secret channel, or env vars)
If we can, I would like to keep the files in a particular directory, e.g. /etc/kubernetes/assets or similar. Then this will work on immutable images. Not sure whether that works for your use cases. The would imply not having the Path field - not sure if that works for you.
Similarly, allowing the executable mode is a tricky one. Do you actually need customizable modes?
In a different way, templates are also worrying to me. They tend to evolve into "real programming", and then the limitations of the templating system become very apparent vs go code. That's what we found with kops anyway.

I guess the question is which of your use cases would this break? And can I instead volunteer to write you some hooks or extends kops/nodeup (particularly for the ones that need to be executable)?

BTW, for webhook tokens, we did merge initial support for "kopeio" authentication, so explicit support might also be an option (search for writeAuthenticationConfig).

gambol99 · 2017-08-18T13:01:33Z

hi @justinsb ... i've removed the Mode and Template (we are templating on the outside anyhow, so one more thing to template isn't a big deal. It was introduced as figured given the API scheme remained static is was ok to use template, but i get the concern). In regard to the Path could we compromise and say default /srv/kubenetes/assets/ and then allow an override a user override

@note

The current implementation does not make it ease to fully customize nodes before kube install. This PR adds the ability to include file assets in the cluster and instaneGroup spec which can be consumed by nodeup. Allowing those whom need (i.e. me :-)) greater flexibilty around their nodes. @note, nothing is enforced, so unless you've specified anything everything is as the same - updated the cluster_spec.md to reflect the changes - permit users to place inline files into the cluster and instance group specs - added the ability to template the files, the Cluster and InstanceGroup specs are passed into context - cleaned up and missed comment, unordered imports etc along the journey

- removed the Mode field from the FileAsset spec - removed the ability to template the content - removed the need to specify the Path and instead default to /srv/kubernetes/assets/<name> - change the FileAssets from []*FileAssets to []FileAssets

justinsb · 2017-08-22T10:52:48Z

nodeup/pkg/model/file_assets.go

+}
+
+// @perhaps a path finder?
+var templateFuncs = template.FuncMap{


Unused now I think

justinsb · 2017-08-22T11:01:53Z

pkg/apis/kops/validation/validation.go

+		allErrs = append(allErrs, field.Required(fieldPath.Child("Content"), ""))
+	}
+
+	return allErrs


Would be good to validate the base64 value here as otherwise will fail in nodeup

indeed .. that's a good shout .. I'll add it to the PR

justinsb · 2017-08-22T11:04:07Z

/lgtm

justinsb · 2017-08-22T11:04:34Z

/ok-to-test

k8s-github-robot · 2017-08-22T11:04:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99, 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

justinsb · 2017-08-22T22:02:19Z

/retest

justinsb · 2017-08-22T23:15:36Z

Flakiness is being tracked in upstream issue: kubernetes/kubernetes#51128, hopefully fixed by kubernetes/kubernetes#51144

eparis · 2017-08-23T00:03:27Z

/retest
merged the referenced fix

k8s-github-robot · 2017-08-23T02:15:35Z

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

k8s-github-robot · 2017-08-23T02:49:48Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 30, 2017

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

gambol99 force-pushed the file_assets branch from e382846 to c1aff51 Compare July 30, 2017 20:54

k8s-ci-robot assigned justinsb Jul 31, 2017

gambol99 commented Jul 31, 2017

View reviewed changes

gambol99 mentioned this pull request Jul 31, 2017

Feature Request: Write files per instance group #2907

Closed

gambol99 force-pushed the file_assets branch from c1aff51 to 5e7c7a9 Compare August 2, 2017 18:31

gambol99 force-pushed the file_assets branch 2 times, most recently from 12f863f to 9d11c92 Compare August 3, 2017 21:13

gambol99 mentioned this pull request Aug 3, 2017

Customize Kops Platform UKHomeOffice/kops#1

Closed

gambol99 force-pushed the file_assets branch 3 times, most recently from 8652865 to 2a0c10d Compare August 6, 2017 22:29

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017

gambol99 force-pushed the file_assets branch from 2a0c10d to 7dc8999 Compare August 9, 2017 19:43

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

gambol99 force-pushed the file_assets branch 3 times, most recently from a1539c0 to 2d5a4d2 Compare August 12, 2017 00:12

gambol99 force-pushed the file_assets branch from 2d5a4d2 to f775aa9 Compare August 14, 2017 12:13

justinsb reviewed Aug 17, 2017

View reviewed changes

gambol99 force-pushed the file_assets branch 2 times, most recently from 17e0524 to f1e43ac Compare August 18, 2017 13:00

gambol99 force-pushed the file_assets branch from f1e43ac to 0e716b6 Compare August 18, 2017 13:29

gambol99 added 4 commits August 18, 2017 18:06

- using the nodetask rather than the direct MkdirAll

2c9b273

- adding a Roles filter to the file assets to choose who gets the asset

c15221c

Request Changes - File Assets

494671e

- removed the Mode field from the FileAsset spec - removed the ability to template the content - removed the need to specify the Path and instead default to /srv/kubernetes/assets/<name> - change the FileAssets from []*FileAssets to []FileAssets

gambol99 force-pushed the file_assets branch from 0e716b6 to 494671e Compare August 18, 2017 17:06

justinsb approved these changes Aug 22, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 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 Aug 22, 2017

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

k8s-github-robot merged commit 4edc267 into kubernetes:master Aug 23, 2017

gambol99 deleted the file_assets branch August 24, 2017 23:49

chrislovecnm mentioned this pull request Oct 14, 2017

How to customize cloud-config in KOPS for AWS? #2779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster / InstanceGroup File Assets #3090

Cluster / InstanceGroup File Assets #3090

gambol99 commented Jul 30, 2017

k8s-ci-robot commented Jul 30, 2017

gambol99 commented Jul 31, 2017

gambol99 Jul 31, 2017

gambol99 commented Aug 2, 2017 •

edited

Loading

justinsb commented Aug 6, 2017

k8s-github-robot commented Aug 8, 2017

gambol99 commented Aug 9, 2017 •

edited

Loading

gambol99 commented Aug 11, 2017 •

edited

Loading

gambol99 commented Aug 14, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb commented Aug 17, 2017

gambol99 commented Aug 18, 2017

justinsb Aug 22, 2017

justinsb Aug 22, 2017

gambol99 Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

eparis commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017

Cluster / InstanceGroup File Assets #3090

Cluster / InstanceGroup File Assets #3090

Conversation

gambol99 commented Jul 30, 2017

k8s-ci-robot commented Jul 30, 2017

gambol99 commented Jul 31, 2017

Choose a reason for hiding this comment

gambol99 commented Aug 2, 2017 • edited Loading

justinsb commented Aug 6, 2017

k8s-github-robot commented Aug 8, 2017

gambol99 commented Aug 9, 2017 • edited Loading

gambol99 commented Aug 11, 2017 • edited Loading

gambol99 commented Aug 14, 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

justinsb commented Aug 17, 2017

gambol99 commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

eparis commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017

gambol99 commented Aug 2, 2017 •

edited

Loading

gambol99 commented Aug 9, 2017 •

edited

Loading

gambol99 commented Aug 11, 2017 •

edited

Loading