Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Storing the user assigned managed identity in the scaleset table #255

Merged
merged 9 commits into from
Nov 5, 2020

Conversation

chkeita
Copy link
Contributor

@chkeita chkeita commented Nov 2, 2020

Summary of the Pull Request

Closes #249

PR Checklist

  • Applies to work item: Throttling issue with calls to user assigned managed identities #249
  • CLA signed. If not, go over here and sign the CLI.
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

What does this include?

Validation Steps Performed

How does someone test & validate?

@chkeita chkeita marked this pull request as ready for review November 3, 2020 04:25
self.save()

def set_identity(self, vmss: Any) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this handle pre-existing scalesets during the upgrade case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing scalesets are handled in the is_authorized function in agent_authorization.py.

  • if the scaleset had a system assigned managed identity it would have been assigned to client_object_id
  • if the scaleset used a user assigned managed identity before this change we fall back to the existing behavior of querying the azure.

Unfortunately we cannot update the object id of the scaleset because the token does not contain the scaleset id.

Copy link
Contributor

Choose a reason for hiding this comment

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

If a user starts the creation of a scaleset and another user starts an upgrade shortly afterwards, this could leave that newly created scaleset in a bad place such that it does not have the client_object_id

Copy link
Contributor Author

@chkeita chkeita Nov 4, 2020

Choose a reason for hiding this comment

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

I Updated the logic to make sure we only set the node to running if we are able to set the object_client_id. your scenario will result in a failed node creation.

@chkeita chkeita requested a review from bmc-msft November 4, 2020 23:29
@bmc-msft bmc-msft merged commit bbee84a into microsoft:main Nov 5, 2020
@chkeita chkeita deleted the chkeita/249 branch March 31, 2021 21:52
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throttling issue with calls to user assigned managed identities
2 participants