Allow user userdata #135

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

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-aws#651
kubernetes-sigs/cluster-api#721
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:05:24Z

Welcome @enxebre!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-03-13T15:05:25Z

@enxebre: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2019-03-13T15:05:36Z

[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: feiskyer

If they are not already assigned, you can assign the PR to them by writing /assign @feiskyer 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:05:37Z

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:12:54Z

cc @justaugustus @vincepri @derekwaynecarr @ncdc

frobware · 2019-03-13T15:17:11Z

pkg/cloud/azure/services/compute/virtualmachines.go

@@ -138,7 +141,34 @@ func (s *Service) MachineExists(machine *actuators.MachineScope) (bool, error) {
 	return vm != nil, nil
 }

+func getUserDataFromSecret(machine *actuators.MachineScope) (string, error) {
+	if machine.MachineConfig.UserDataSecret != nil {


You could reduce the indentation by returning early here.

frobware · 2019-03-13T15:18:36Z

pkg/cloud/azure/services/compute/virtualmachines.go

+		} else {
+			return "", fmt.Errorf("secret %q does not have field %q", machine.MachineConfig.UserDataSecret.Name, "userData")
+		}
+		encodedUserData := base64.StdEncoding.EncodeToString(userData)


nit: drop the temporary, return base64.StdEncoding.EncodeToString(userData), nil.

frobware · 2019-03-13T15:18:53Z

pkg/cloud/azure/services/compute/virtualmachines.go

 func (s *Service) getVMStartupScript(machine *actuators.MachineScope, bootstrapToken string) (string, error) {
+


nit; drop the blank line.

frobware · 2019-03-13T15:20:02Z

pkg/cloud/azure/services/compute/virtualmachines.go

+
+	userData, err := getUserDataFromSecret(machine)
+	if err != nil {
+		return "", errors.Errorf("failed getting userData from secret %v", err)


Not sure what the convention is here, but do we separate error message with a preceding :?

justaugustus · 2019-03-14T21:16:49Z

On hold, echoing @ncdc's request for a design doc/justification (ref: kubernetes-sigs/cluster-api-provider-aws#651 (comment))

/hold
/ok-to-test

tariq1890 · 2019-03-19T23:56:36Z

pkg/apis/azureprovider/v1alpha1/azuremachineproviderconfig_types.go

@@ -41,6 +42,10 @@ type AzureMachineProviderSpec struct {
 	OSDisk        OSDisk        `json:"osDisk"`
 	SSHPublicKey  string        `json:"sshPublicKey"`
 	SSHPrivateKey string        `json:"sshPrivateKey"`
+	// UserDataSecret contains a local reference to a secret that contains the
+	// UserData to apply to the instance


Suggested change

// UserData to apply to the instance

// UserData to apply to the instance.

tariq1890 · 2019-03-19T23:56:56Z

pkg/cloud/azure/actuators/machine/actuator.go

-	scope, err := actuators.NewMachineScope(actuators.MachineScopeParams{Machine: machine, Cluster: cluster, Client: a.client})
+	coreClient, bootstrapErr := a.coreV1Client(cluster)
+	if bootstrapErr != nil {
+		return errors.Wrapf(bootstrapErr, "failed to retrieve corev1 client for cluster %q", cluster.Name)


Suggested change

return errors.Wrapf(bootstrapErr, "failed to retrieve corev1 client for cluster %q", cluster.Name)

return errors.Wrapf(bootstrapErr, "failed to retrieve corev1 client for cluster %s", cluster.Name)

tariq1890 · 2019-03-19T23:57:40Z

pkg/cloud/azure/actuators/machine_scope.go

@@ -20,6 +20,7 @@ import (
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	v1 "k8s.io/client-go/kubernetes/typed/core/v1"


Suggested change

v1 "k8s.io/client-go/kubernetes/typed/core/v1"

v1 "corev1 "k8s.io/api/core/v1"

tariq1890 · 2019-03-19T23:58:04Z

pkg/cloud/azure/services/compute/virtualmachines.go

+		userData := []byte{}
+		userDataSecret, err := machine.CoreClient.Secrets(machine.Namespace()).Get(machine.MachineConfig.UserDataSecret.Name, metav1.GetOptions{})
+		if err != nil {
+			return "", fmt.Errorf("failed to get userData secret %q", machine.MachineConfig.UserDataSecret.Name)


tariq1890 · 2019-03-19T23:58:38Z

pkg/cloud/azure/services/compute/virtualmachines.go

+
+	userData, err := getUserDataFromSecret(machine)
+	if err != nil {
+		return "", errors.Errorf("failed getting userData from secret %v", err)


Suggested change

return "", errors.Errorf("failed getting userData from secret %v", err)

return "", errors.Wrap(err, "failed getting userData from secret")

k8s-ci-robot · 2019-03-31T20:49:48Z

@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.

fejta-bot · 2019-06-29T21:38:41Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

alexeldeib · 2019-06-29T22:48:57Z

/remove-lifecycle stale

alexeldeib · 2019-06-29T22:51:56Z

This ties in closely to bootstrap/infrastructure provider changes in the CAPI v1alpha2 proposals, feels like there will be some tight interplay there. Marking as frozen since this is important but the direction is a bit hazy at the moment.

/lifecycle frozen

justaugustus · 2019-06-29T23:00:34Z

Closing this (xref @ncdc's comment on the same PR for CAPA):

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-29T23:00:36Z

@justaugustus: Closed this PR.

In response to this:

Closing this (xref @ncdc's comment on the same PR for CAPA):

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 2 commits March 13, 2019 15:23

Add support for user userdata

f19112f

Run generate and gazelle

818dd0e

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/provider/azure Issues or PRs related to azure provider labels 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

k8s-ci-robot requested review from andyzhangx and roberthbailey March 13, 2019 15:05

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2019

enxebre mentioned this pull request Mar 13, 2019

Allow user to use arbitraty userdata kubernetes-sigs/cluster-api-provider-aws#651

Closed

frobware reviewed Mar 13, 2019

View reviewed changes

tariq1890 reviewed Mar 19, 2019

View reviewed changes

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 29, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 29, 2019

k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jun 29, 2019

k8s-ci-robot closed this Jun 29, 2019

		func (s Service) getVMStartupScript(machine actuators.MachineScope, bootstrapToken string) (string, error) {

	// UserData to apply to the instance
	// UserData to apply to the instance.

	return errors.Wrapf(bootstrapErr, "failed to retrieve corev1 client for cluster %q", cluster.Name)
	return errors.Wrapf(bootstrapErr, "failed to retrieve corev1 client for cluster %s", cluster.Name)

	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
	v1 "corev1 "k8s.io/api/core/v1"

	return "", errors.Errorf("failed getting userData from secret %v", err)
	return "", errors.Wrap(err, "failed getting userData from secret")

Allow user userdata #135

Allow user userdata #135

Uh oh!

Conversation

enxebre commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 13, 2019

Uh oh!

k8s-ci-robot commented Mar 13, 2019

Uh oh!

k8s-ci-robot commented Mar 13, 2019

Uh oh!

k8s-ci-robot commented Mar 13, 2019

Uh oh!

enxebre commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justaugustus commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 31, 2019

Uh oh!

fejta-bot commented Jun 29, 2019

Uh oh!

alexeldeib commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexeldeib commented Jun 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justaugustus commented Jun 29, 2019

Uh oh!

k8s-ci-robot commented Jun 29, 2019

Uh oh!

Uh oh!

enxebre commented Mar 13, 2019 •

edited

Loading

enxebre commented Mar 13, 2019 •

edited

Loading

alexeldeib commented Jun 29, 2019 •

edited

Loading

alexeldeib commented Jun 29, 2019 •

edited

Loading