Allow user to use arbitraty userdata #651

enxebre · 2019-03-13T15:05:54Z

What this PR does / why we need it:

This PR removes tight coupling between machine resource and kubeadm while keeping the current behaviour pristine.
This allows flexibility for the API consumers to support broader use cases by injecting arbitrary userdata into a secret optionally referenced by the spec.
While being a fairly scoped change this would favour adoption of the project extremely.

Related:
kubernetes-sigs/cluster-api-provider-azure#135
kubernetes-sigs/cluster-api#721 and https://docs.google.com/document/d/1pzXtwYWRsOzq5Ftu03O5FcFAlQE26nD3bjYBPenbhjg/edit#heading=h.etxqa0k5waej

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2019-03-13T15:06:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enxebre
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: ingvagabund

If they are not already assigned, you can assign the PR to them by writing /assign @ingvagabund in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-03-13T15:06:11Z

Hi @enxebre. Thanks for your PR.

I'm waiting for a kubernetes-sigs or 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

enxebre · 2019-03-13T15:13:00Z

cc @vincepri @derekwaynecarr @justaugustus @ncdc

chuckha · 2019-03-13T15:20:23Z

where is the branch on your fork to check this out? I can't seem to locate a branch named allow-user-userdata

edit: 🤦‍♂️

enxebre · 2019-03-13T15:22:43Z

@chuckha https://github.com/enxebre/cluster-api-provider-aws-2/tree/allow-user-userdata
See also https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/actuators/machine/actuator.go#L203-L214

frobware · 2019-03-13T15:31:01Z

pkg/cloud/aws/services/userdata/userdata.go

@@ -90,3 +93,21 @@ func funcMap(funcs map[string]interface{}) template.FuncMap {

 	return funcMap
 }
+
+func GetUserDataFromSecret(machine *actuators.MachineScope) (string, error) {


Is it possible to create a unit test case for this function?

chuckha

Before I get too far into code reviewing let me make sure I understand what this is doing:

This allows someone to add their own script to be run at node-boot time. It's not in addition to the default installation, but entirely replaces it.

Can you outline what your use case is so I can better understand where you're coming from?

chuckha · 2019-03-13T15:37:50Z

pkg/cloud/aws/services/userdata/userdata.go

@@ -90,3 +93,21 @@ func funcMap(funcs map[string]interface{}) template.FuncMap {

 	return funcMap
 }
+
+func GetUserDataFromSecret(machine *actuators.MachineScope) (string, error) {


A couple of suggestions and questions:

flip around the nil check and return early if the UserDataSecret is nil

the userData var is unnecessary, directly encode and return at line 105 and remove the else statement

why is it an error if the secret doesn't contain the userData key? My expectation would be to log an error and return nothing

please add godoc and a test to this function

ashish-amarnath · 2019-03-13T16:04:43Z

pkg/cloud/aws/services/ec2/instances.go

@@ -158,15 +158,73 @@ func (s *Service) createInstance(machine *actuators.MachineScope, bootstrapToken
 	}

 	// apply values based on the role of the machine


is this comment still valid?

ashish-amarnath · 2019-03-13T16:09:27Z

pkg/cloud/aws/services/userdata/userdata.go

+func GetUserDataFromSecret(machine *actuators.MachineScope) (string, error) {
+	if machine.MachineConfig.UserDataSecret != nil {
+		userData := []byte{}
+		userDataSecret, err := machine.CoreClient.Secrets(machine.Namespace()).Get(machine.MachineConfig.UserDataSecret.Name, metav1.GetOptions{})


How will this secret be created?
If the secret doesn't exist, the machine reconciliation will not make progress until the secret is created. Correct?

Correct. If you set https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/651/files#diff-7fbfb526ca988417add16e1058fcd340R85
You are assuming the secret with that name to exist.
If you don't set the spec field, current behaviour remains
https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/651/files#diff-e7748766dc5475de766bbb0d40ef7fd1R98

enxebre · 2019-03-13T19:26:22Z

Before I get too far into code reviewing let me make sure I understand what this is doing:

This allows someone to add their own script to be run at node-boot time. It's not in addition to the default installation, but entirely replaces it.

Can you outline what your use case is so I can better understand where you're coming from?

Hey @chuckha for our use case in OpenShift we have a clearer separation between machine (hardware) and software/kubelet. The software/kubelet is managed orthogonally by a different component.
We pre-populate a secret with an ignition config containing the kubelet configuration (among other things) which then is fetched via the userdata API field https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/actuators/machine/actuator.go#L203-L214.
Then the kubelet and software upgrades/lifecycle management is oblivious to this API. Please let me know if more context is needed

ncdc · 2019-03-13T19:53:13Z

As discussed in today's Cluster API meeting, we'd like to see a document outlining requirements and use cases. Going to mark this on hold for now. Thanks!

/hold

enxebre · 2019-03-13T20:29:30Z

As discussed in today's Cluster API meeting, we'd like to see a document outlining requirements and use cases. Going to mark this on hold for now. Thanks!

Unfortunately attending the meeting is not always feasible being in CET. @ncdc I can't see any action item in the meeting notes https://docs.google.com/document/d/1Ys-DOR5UsgbMEeciuG0HOgDQc8kZsaWIWJeKJ1-UfbY/edit#heading=h.h3b651pv3rlt I'm happy to get some doc in place otherwise.

This relates to kubernetes-sigs/cluster-api#721
The case this PR is adding is allowing the user to put arbitrary userdata in machines

ncdc · 2019-03-13T20:33:23Z

@enxebre understood about the time zone issue. The relevant section in the meeting notes is Ignition support - node bootstrap commonalities between all providers?, specifically the part about doing a design doc.

ncdc · 2019-03-13T20:34:53Z

We would like to go through the design process to come up with something that works well for everyone, and would like adequate time to review/comment/revise etc.

k8s-ci-robot · 2019-04-27T19:17:35Z

@enxebre: PR needs rebase.

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.

ncdc · 2019-06-24T20:33:31Z

As part of the Cluster API v1alpha2 milestone that we just started working on, we're separating bootstrapping from infrastructure provisioning, and you'll be able to bring your own user data. As such, I'm going to close this in favor of that effort.

/close

k8s-ci-robot · 2019-06-24T20:33:32Z

@ncdc: Closed this PR.

In response to this:

As part of the Cluster API v1alpha2 milestone that we just started working on, we're separating bootstrapping from infrastructure provisioning, and you'll be able to bring your own user data. As such, I'm going to close this in favor of that effort.

/close

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.

enxebre added 3 commits March 13, 2019 12:40

Refactor for getting userdata and security group

6f419fa

Add support for user userdata

48d9fe9

Run generate and gazelle

1d766e0

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2019

k8s-ci-robot requested review from d-nishi and roberthbailey March 13, 2019 15:06

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2019

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 Mar 13, 2019

enxebre mentioned this pull request Mar 13, 2019

Allow user userdata kubernetes-sigs/cluster-api-provider-azure#135

Closed

frobware reviewed Mar 13, 2019

View reviewed changes

chuckha reviewed Mar 13, 2019

View reviewed changes

ashish-amarnath reviewed Mar 13, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2019

enxebre changed the title ~~Allow user userdata~~ Allow user to use arbitraty userdata Mar 13, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2019

k8s-ci-robot closed this Jun 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to use arbitraty userdata #651

Allow user to use arbitraty userdata #651

enxebre commented Mar 13, 2019 •

edited

Loading

k8s-ci-robot commented Mar 13, 2019

k8s-ci-robot commented Mar 13, 2019

enxebre commented Mar 13, 2019 •

edited

Loading

chuckha commented Mar 13, 2019 •

edited

Loading

enxebre commented Mar 13, 2019

frobware Mar 13, 2019

chuckha left a comment

chuckha Mar 13, 2019

ashish-amarnath Mar 13, 2019

ashish-amarnath Mar 13, 2019 •

edited

Loading

enxebre Mar 13, 2019 •

edited

Loading

enxebre commented Mar 13, 2019 •

edited

Loading

ncdc commented Mar 13, 2019

enxebre commented Mar 13, 2019 •

edited

Loading

ncdc commented Mar 13, 2019

ncdc commented Mar 13, 2019

k8s-ci-robot commented Apr 27, 2019

ncdc commented Jun 24, 2019

k8s-ci-robot commented Jun 24, 2019

		@@ -158,15 +158,73 @@ func (s Service) createInstance(machine actuators.MachineScope, bootstrapToken
		}

		// apply values based on the role of the machine

Allow user to use arbitraty userdata #651

Allow user to use arbitraty userdata #651

Conversation

enxebre commented Mar 13, 2019 • edited Loading

k8s-ci-robot commented Mar 13, 2019

k8s-ci-robot commented Mar 13, 2019

enxebre commented Mar 13, 2019 • edited Loading

chuckha commented Mar 13, 2019 • edited Loading

enxebre commented Mar 13, 2019

frobware Mar 13, 2019

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

chuckha Mar 13, 2019

Choose a reason for hiding this comment

ashish-amarnath Mar 13, 2019

Choose a reason for hiding this comment

ashish-amarnath Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

enxebre Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

enxebre commented Mar 13, 2019 • edited Loading

ncdc commented Mar 13, 2019

enxebre commented Mar 13, 2019 • edited Loading

ncdc commented Mar 13, 2019

ncdc commented Mar 13, 2019

k8s-ci-robot commented Apr 27, 2019

ncdc commented Jun 24, 2019

k8s-ci-robot commented Jun 24, 2019

enxebre commented Mar 13, 2019 •

edited

Loading

enxebre commented Mar 13, 2019 •

edited

Loading

chuckha commented Mar 13, 2019 •

edited

Loading

ashish-amarnath Mar 13, 2019 •

edited

Loading

enxebre Mar 13, 2019 •

edited

Loading

enxebre commented Mar 13, 2019 •

edited

Loading

enxebre commented Mar 13, 2019 •

edited

Loading