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

Use MSI ClientID as userAssignedIdentityID in azure.json #2214

Merged
merged 1 commit into from
May 13, 2022
Merged

Use MSI ClientID as userAssignedIdentityID in azure.json #2214

merged 1 commit into from
May 13, 2022

Conversation

mboersma
Copy link
Contributor

@mboersma mboersma commented Apr 4, 2022 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Uses a lookup function to translate providerID to ClientID when creating azure.json on nodes.

Which issue(s) this PR fixes:

Fixes #2164

Special notes for your reviewer:

This is a hack and I'm open for suggestions as to how to make it conform better with our service and reconciler models. Also, it needs regression tests to ensure azure.json doesn't revert. (I've tested this interactively.)

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Use MSI ClientID as userAssignedIdentityID in azure.json

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2022
@mboersma mboersma changed the title Use MSI ClientID as userAssignedIdentityID in azure.json [WIP] Use MSI ClientID as userAssignedIdentityID in azure.json Apr 4, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2022
CecileRobertMichon
CecileRobertMichon reviewed Apr 6, 2022
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.

/assign @jackfrancis

@mboersma
Copy link
Contributor Author

mboersma commented Apr 7, 2022

I would really like to validate azure.json in an e2e test, but AFAICT we don't use the user-assigned id flavor in tests yet. I'll try writing a new spec for that. Suggestions welcome!

@CecileRobertMichon CecileRobertMichon mentioned this pull request Apr 11, 2022
Closed
3 tasks
shysank
shysank reviewed Apr 11, 2022
View reviewed changes
Copy link
Contributor

@shysank shysank left a comment

Choose a reason for hiding this comment

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

looks good overall with a couple of nits.

azure/services/identities/client.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
azure/services/identities/client.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2022
@mboersma mboersma force-pushed the uai-test branch 3 times, most recently from 165b664 to 10d5f93 Compare April 29, 2022 17:26
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 29, 2022
@mboersma mboersma changed the title [WIP] Use MSI ClientID as userAssignedIdentityID in azure.json Use MSI ClientID as userAssignedIdentityID in azure.json Apr 29, 2022
@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 29, 2022
@mboersma
Copy link
Contributor Author

See also kubernetes-sigs/cloud-provider-azure#1290.

@mboersma mboersma mentioned this pull request Apr 29, 2022
Merged
3 tasks
@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@mboersma
Copy link
Contributor Author

Updated to validate user-assigned identity in the existing external-cloud-provider and dual-stack (internal cloud-provider) test specs.

This requires a user-assigned identity to exist before running tests, as described in the comments in azure_test.go. This is already true for the CI subscription.

@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-optional

@CecileRobertMichon
Copy link
Contributor

/retest

CecileRobertMichon
CecileRobertMichon reviewed May 13, 2022
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.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 May 13, 2022
@k8s-ci-robot k8s-ci-robot merged commit d6ff841 into kubernetes-sigs:main May 13, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone May 13, 2022
@mboersma mboersma deleted the uai-test branch May 14, 2022 14:52
@CecileRobertMichon
Copy link
Contributor

/cherry-pick release-1.3

@mboersma thoughts on cutting a release to include this one?

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request May 16, 2022
Merged
@k8s-infra-cherrypick-robot
Copy link

@CecileRobertMichon: new pull request created: #2309

In response to this:

/cherry-pick release-1.3

@mboersma thoughts on cutting a release to include this one?

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.

@mboersma
Copy link
Contributor Author

thoughts on cutting a release to include this one?

Yes, I think we should, otherwise user-assigned ID stays broken for most users.

@mboersma mboersma mentioned this pull request Aug 9, 2023
Closed
3 tasks
teamdraftbox added a commit to teamdraftbox/cluster-api-provider-azure that referenced this pull request Aug 11, 2023
@teamdraftbox
⏪ reverted userAssignedIdenties logic based pn PR kubernetes-sigs#2214
d71353c
teamdraftbox added a commit to teamdraftbox/cluster-api-provider-azure that referenced this pull request Aug 15, 2023
@teamdraftbox
⏪ reverted userAssignedIdenties logic based pn PR kubernetes-sigs#2214
77dad5e 
patch

patch

♻️ cleanup

patch

📝  linter fix make
teamdraftbox added a commit to teamdraftbox/cluster-api-provider-azure that referenced this pull request Aug 15, 2023
@teamdraftbox
⏪ reverted userAssignedIdenties logic based pn PR kubernetes-sigs#2214
d35495e 
patch

patch

♻️ cleanup

patch

📝  linter fix make

conflict fix
teamdraftbox added a commit to teamdraftbox/cluster-api-provider-azure that referenced this pull request Aug 15, 2023
@teamdraftbox
⏪ reverted userAssignedIdenties logic based pn PR kubernetes-sigs#2214
48768e4 
patch

patch

♻️ cleanup

patch

📝  linter fix make

conflict fix

conflict fix
mboersma pushed a commit to mboersma/cluster-api-provider-azure that referenced this pull request Aug 18, 2023
@teamdraftbox @mboersma
⏪ reverted userAssignedIdenties logic based pn PR kubernetes-sigs#2214
ab0dadc 
patch

patch

♻️ cleanup

patch

📝  linter fix make

conflict fix

conflict fix
mboersma added a commit to mboersma/cluster-api-provider-azure that referenced this pull request Oct 2, 2023
@mboersma
Revert "⏪ reverted userAssignedIdenties logic based pn PR kubernetes-…
d75fb0a 
…sigs#2214"

This reverts commit 48768e4.
mboersma added a commit to mboersma/cluster-api-provider-azure that referenced this pull request Oct 2, 2023
@mboersma
Revert "⏪ reverted userAssignedIdenties logic based pn PR kubernetes-…
2d60b4c 
…sigs#2214"

This reverts commit 48768e4.
k8s-ci-robot added a commit that referenced this pull request Oct 3, 2023
@k8s-ci-robot
Merge pull request #4064 from mboersma/revert-2214
39024f6 
Revert "⏪ reverted userAssignedIdenties logic based on PR #2214"
k8s-infra-cherrypick-robot pushed a commit to k8s-infra-cherrypick-robot/cluster-api-provider-azure that referenced this pull request Oct 3, 2023
@mboersma
Revert "⏪ reverted userAssignedIdenties logic based pn PR kubernetes-…
f85e1a3 
…sigs#2214"

This reverts commit 48768e4.
k8s-ci-robot added a commit that referenced this pull request Oct 3, 2023
@k8s-ci-robot
Merge pull request #4068 from k8s-infra-cherrypick-robot/cherry-pick-…
7afcfbb 
…4064-to-release-1.11

[release-1.11] Revert "⏪ reverted userAssignedIdenties logic based on PR #2214"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shysank shysank shysank left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

Assignees

@jackfrancis jackfrancis

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

userAssignedIdentityID in azure.json is wrong when using user assinged MSI
6 participants
@mboersma @CecileRobertMichon @k8s-ci-robot @k8s-infra-cherrypick-robot @shysank @jackfrancis