✨ Add support for managed identity in machine pool #1004

shysank · 2020-10-20T21:30:24Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds support for managed identity in machine pool.

Ports ✨ Add support for user-assigned managed identities in AzureMachine #616 changes to machine pool

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 #960

TODOs:

squashed commits
includes documentation
adds unit tests

Release Note:

Add support for managed identity in machine pool

k8s-ci-robot · 2020-10-20T21:30:34Z

Welcome @shysank!

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. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-10-20T21:30:34Z

Hi @shysank. Thanks for your PR.

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

nader-ziada · 2020-10-21T17:53:47Z

/cc @devigned

nader-ziada

Thanks for the PR @shysank

a couple of quick questions

cloud/services/virtualmachines/virtualmachines.go

nader-ziada · 2020-10-21T17:58:02Z

cloud/converters/identity.go

+
+// ErrUserAssignedIdentitiesNotFound is the error thrown when user assigned identities is not passed with the identity type being UserAssigned
+var ErrUserAssignedIdentitiesNotFound = errors.New("the user-assigned identity provider ids must not be null or empty for 'UserAssigned' identity type")
+


is this converter to just generate the resource id in the azure format?

Yes, this is just a helper for converting []UserIdentity to map[string]VmOrVmssIdentityStruct

nader-ziada · 2020-10-21T20:48:28Z

/ok-to-test

nader-ziada · 2020-10-22T02:36:27Z

@devigned can you take a look please

nader-ziada · 2020-10-22T02:38:42Z

@shysank if you think the PR is ready, can you please remove the [WIP] and add a release note in the PR description.

devigned

Looks pretty good. I have some light feedback about the code and documentation.

One thing I would like to see out of this PR is a tutorial in the CAPZ book to describe how to create a MachinePool with a user assigned identity with a link to building a user assigned identity.

templates/flavors/machinepool-user-assigned-identity/patches/user-assigned-identity.yaml

templates/flavors/machinepool-user-assigned-identity/kustomization.yaml

templates/flavors/machinepool-system-assigned-identity/patches/system-assigned-identity.yaml

templates/flavors/machinepool-system-assigned-identity/kustomization.yaml

config/crd/bases/exp.infrastructure.cluster.x-k8s.io_azuremachinepools.yaml

devigned · 2020-10-22T16:09:05Z

cloud/services/scalesets/vmss_test.go

@@ -717,6 +717,188 @@ func TestReconcileVMSS(t *testing.T) {
 				s.SetProvisioningState(infrav1.VMStateSucceeded)
 			},
 		},
+		{
+			name:          "can create a vmss with user assigned identity",


No action in the PR, but more for maintainers.

Dang... these tests are verbose with only like 2 lines that need to be updated. It would be nice to dry these up.

cc: @CecileRobertMichon and @nader-ziada

cloud/converters/identity.go

cloud/converters/identity_test.go

CecileRobertMichon · 2020-10-22T16:38:04Z

re: documentation, you could add onto the existing Identity doc https://capz.sigs.k8s.io/topics/identity.html

shysank · 2020-10-22T20:26:37Z

/retitle ✨ Add support for managed identity in machine pool

k8s-ci-robot · 2020-10-22T20:26:44Z

@shysank: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle ✨ Add support for managed identity in machine pool

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.

shysank · 2020-10-22T20:30:41Z

@devigned I've addressed the review comments, PTAL.
cc: @CecileRobertMichon @nader-ziada

devigned

lgtm

/assign @nader-ziada @CecileRobertMichon

nader-ziada · 2020-10-23T16:14:41Z

lgtm pending squashing commits

cloud/services/scalesets/vmss.go

CecileRobertMichon · 2020-10-26T17:00:20Z

/retest

looks like an unrelated flake

shysank · 2020-10-26T20:56:20Z

@CecileRobertMichon I have made the role assignment changes for system assigned identity, PTAL. The api-diff script is failing because I modified the Validate method to accept old machine pool spec as a parameter.

nader-ziada · 2020-10-26T23:16:19Z

I think its lgtm pending squashing commits

CecileRobertMichon

lgtm, please squash the commits @shysank

Awesome work, thanks!

k8s-ci-robot · 2020-10-26T23:45:32Z

@shysank: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`b463637`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

shysank · 2020-10-26T23:52:06Z

@nader-ziada @CecileRobertMichon Thanks for the review, I've squashed the commits.

CecileRobertMichon · 2020-10-27T01:02:13Z

/lgtm

devigned

/lgtm
/approve

k8s-ci-robot · 2020-10-27T12:53:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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~~ [devigned]

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 requested review from alexeldeib and nader-ziada October 20, 2020 21:30

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 20, 2020

shysank force-pushed the fix/960 branch from 4fd31bb to 47d4027 Compare October 21, 2020 02:10

shysank marked this pull request as ready for review October 21, 2020 16:52

k8s-ci-robot requested a review from devigned October 21, 2020 17:53

nader-ziada reviewed Oct 21, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2020

devigned suggested changes Oct 22, 2020

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 22, 2020

shysank changed the title ~~[WIP] ✨ Add support for managed identity in machine pool~~ ✨ Add support for managed identity in machine pool Oct 22, 2020

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 Oct 22, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 22, 2020

devigned approved these changes Oct 23, 2020

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon and nader-ziada Oct 23, 2020

CecileRobertMichon reviewed Oct 23, 2020

View reviewed changes

cloud/services/scalesets/vmss.go Show resolved Hide resolved

shysank force-pushed the fix/960 branch 2 times, most recently from 87d91b7 to af862a5 Compare October 26, 2020 18:09

CecileRobertMichon approved these changes Oct 26, 2020

View reviewed changes

Add support for managed identity in machine pool

b463637

shysank force-pushed the fix/960 branch from dd0304f to b463637 Compare October 26, 2020 23:43

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2020

devigned approved these changes Oct 27, 2020

View reviewed changes

k8s-ci-robot assigned devigned Oct 27, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2020

k8s-ci-robot merged commit 7aae523 into kubernetes-sigs:master Oct 27, 2020

k8s-ci-robot added this to the v0.4.10 milestone Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add support for managed identity in machine pool #1004

✨ Add support for managed identity in machine pool #1004

shysank commented Oct 20, 2020 •

edited

Loading

k8s-ci-robot commented Oct 20, 2020

k8s-ci-robot commented Oct 20, 2020

nader-ziada commented Oct 21, 2020

nader-ziada left a comment

nader-ziada Oct 21, 2020

shysank Oct 21, 2020

nader-ziada commented Oct 21, 2020

nader-ziada commented Oct 22, 2020

nader-ziada commented Oct 22, 2020

devigned left a comment

devigned Oct 22, 2020

CecileRobertMichon commented Oct 22, 2020

shysank commented Oct 22, 2020

k8s-ci-robot commented Oct 22, 2020

shysank commented Oct 22, 2020

devigned left a comment

nader-ziada commented Oct 23, 2020

CecileRobertMichon commented Oct 26, 2020

shysank commented Oct 26, 2020

nader-ziada commented Oct 26, 2020

CecileRobertMichon left a comment

k8s-ci-robot commented Oct 26, 2020

shysank commented Oct 26, 2020

CecileRobertMichon commented Oct 27, 2020

devigned left a comment

k8s-ci-robot commented Oct 27, 2020


		// ErrUserAssignedIdentitiesNotFound is the error thrown when user assigned identities is not passed with the identity type being UserAssigned
		var ErrUserAssignedIdentitiesNotFound = errors.New("the user-assigned identity provider ids must not be null or empty for 'UserAssigned' identity type")

✨ Add support for managed identity in machine pool #1004

✨ Add support for managed identity in machine pool #1004

Conversation

shysank commented Oct 20, 2020 • edited Loading

k8s-ci-robot commented Oct 20, 2020

k8s-ci-robot commented Oct 20, 2020

nader-ziada commented Oct 21, 2020

nader-ziada left a comment

Choose a reason for hiding this comment

nader-ziada Oct 21, 2020

Choose a reason for hiding this comment

shysank Oct 21, 2020

Choose a reason for hiding this comment

nader-ziada commented Oct 21, 2020

nader-ziada commented Oct 22, 2020

nader-ziada commented Oct 22, 2020

devigned left a comment

Choose a reason for hiding this comment

devigned Oct 22, 2020

Choose a reason for hiding this comment

CecileRobertMichon commented Oct 22, 2020

shysank commented Oct 22, 2020

k8s-ci-robot commented Oct 22, 2020

shysank commented Oct 22, 2020

devigned left a comment

Choose a reason for hiding this comment

nader-ziada commented Oct 23, 2020

CecileRobertMichon commented Oct 26, 2020

shysank commented Oct 26, 2020

nader-ziada commented Oct 26, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 26, 2020

shysank commented Oct 26, 2020

CecileRobertMichon commented Oct 27, 2020

devigned left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2020

shysank commented Oct 20, 2020 •

edited

Loading