Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ add support for azure system assigned identities #565

Merged
merged 1 commit into from
Apr 30, 2020
Merged

✨ add support for azure system assigned identities #565

merged 1 commit into from
Apr 30, 2020

Conversation

nader-ziada
Copy link
Contributor

What this PR does / why we need it:
Add support for azure managed system assigned identities when created a VM

  • add new field for IdentityType in AzureMachineSpec
  • add new flavor for VMs with system assigned identity

Which issue(s) this PR fixes
Ref #312

Release note:

Add a new IdentityType field on AzureMachineSpec to support Azure managed Identities 
Add a new flavor  in templates that uses the Azure system assigned identity

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 22, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 22, 2020
@nader-ziada
Copy link
Contributor Author

@devigned please take a look when you get a chance! Thanks

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-verify

@nader-ziada nader-ziada force-pushed the identity branch 2 times, most recently from 8f1ad61 to d0cd420 Compare April 23, 2020 14:47
@nader-ziada nader-ziada changed the title [WIP] ✨ add support for azure system assigned identities ✨ add support for azure system assigned identities Apr 23, 2020
@k8s-ci-robot 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 Apr 23, 2020
@nader-ziada
Copy link
Contributor Author

I'm not sure why the verify fails, everything seems to be updated, will look into it more

@CecileRobertMichon
Copy link
Contributor

@nader-ziada it looks like it's complaining that make generate-flavors is generating a diff

@nader-ziada
Copy link
Contributor Author

@CecileRobertMichon Thanks, seems my local kustomize is different than on ci, but I think I found something, trying it now

@nader-ziada nader-ziada force-pushed the identity branch 3 times, most recently from 4ab94cc to 46f01bc Compare April 23, 2020 17:49
@nader-ziada
Copy link
Contributor Author

@CecileRobertMichon @devigned PR ready for another look. Thanks

@devigned
Copy link
Contributor

@nader-ziada, the code looks awesome ✨ . I have one question though.

What rights does the identity have? I don't see any RBAC rights assigned to the identity. For example, the control plane machines will likely need subscription contributor access and the worker nodes might need resource group contributor and possibly subscription contributor too.

I don't think this will work correctly with the Azure cloud provider with the current RBAC rights.

@nader-ziada
Copy link
Contributor Author

@devigned I was able to to create a cluster and made sure the VMs has an identity of System Identity turned on, I also deployed a sample app on the workload cluster and was able to connect to that. But can you share an example or a doc of what I need to add from an Azure perspective?

@nader-ziada
Copy link
Contributor Author

/retest

1 similar comment
@nader-ziada
Copy link
Contributor Author

/retest

@nader-ziada
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020
@nader-ziada nader-ziada force-pushed the identity branch 2 times, most recently from 56276ef to 45cafc9 Compare April 30, 2020 12:29
@nader-ziada
Copy link
Contributor Author

@CecileRobertMichon addressed the last comments

CecileRobertMichon
CecileRobertMichon approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/hold
/assign @devigned

Awesome work @nader-ziada !

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, nader-ziada

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2020
devigned
devigned suggested changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry to do this, but there is one last thing. Please update https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/docs/getting-started.md#Prerequisites to specify --role owner, so that developers creating a new service principal will have the correct rights to assign roles.

@nader-ziada
add support for azure system assigned identities
ac4ee9e 
 - add new field for IdentityType  in AzureMachineSpec
 - add new flavor for VMs with system assigned identity
 - add a role assognment to the system generated identity
@nader-ziada
Copy link
Contributor Author

@devigned no worries, I made the change

@devigned
Copy link
Contributor

/lgtm

Thank you, @nader-ziada. Great work! 🔥

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2020
@CecileRobertMichon
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2020
@k8s-ci-robot k8s-ci-robot merged commit 6efc4da into kubernetes-sigs:master Apr 30, 2020
@k8s-ci-robot k8s-ci-robot added this to the v0.5 milestone Apr 30, 2020
@nader-ziada
Copy link
Contributor Author

Thanks @devigned for helping me understand how all this works on Azure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon approved these changes

@devigned devigned devigned approved these changes

@justaugustus justaugustus Awaiting requested review from justaugustus

Assignees

@devigned devigned

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4.x
Development

Successfully merging this pull request may close these issues.
4 participants
@nader-ziada @CecileRobertMichon @devigned @k8s-ci-robot