-
Notifications
You must be signed in to change notification settings - Fork 1.8k
OSDOCS-13798 Adding user-specified identities to Azure #93377
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.
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
Conversation
🤖 Wed May 21 19:42:34 - Prow CI generated the docs preview: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the fine-grained permissions updates, we should update the docs to remove the mentions of User Access Administrator
, which is no longer required by default. This permission (or perhaps the less powerful RBAC Access Admin) is only required when users specify a User-Assigned Identity in the install config.
In the docs in 2.4.1 it says:
The Azure account that you use to create the identity is assigned the User Access Administrator and Contributor roles. These roles are required when:
Creating a service principal or user-assigned managed identity.
Enabling a system-assigned managed identity on a virtual machine.
We could probably change that to just.
The Azure account that you use to create the identity is assigned the Contributor role.
And then in 2.4.3 where we document how to create the service principal we can remove step 2 which adds the User Access Admin role.
Or, instead of removing it, we can add a note saying this is only necessary when users supply the values in the install config... I hope this makes sense, but I'm sure it's confusing so please LMK if you have questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that two parts are missed.
- The permission of service principal or managed identity (attached to azure virtual machine and installer run in that VM) for IPI installation with user-assigned identity configured in install-config (attached to cluster nodes), which you added into installation-configuration-parameters.adoc.
- Without minimal permission installation, both 'Contributor' role and 'User Access Administrator' are required.
- With minimal permission installation, following optional permissions are required.
"Microsoft.ManagedIdentity/userAssignedIdentities/assign/action"
"Microsoft.ManagedIdentity/userAssignedIdentities/read"
- UPI doc:
- Remove the identity creation and role assignment steps (procedure 2 & 3) from Creating the Azure resource group , because user-assigned identity is also removed from UPI ARM template (openshift/installer#9625)
- Same permissions you removed from IPI doc also need to be removed from minimal permission list in UPI doc
modules/installation-azure-create-resource-group-and-identity.adoc
Outdated
Show resolved
Hide resolved
Thanks @jinyunma I have incorporated your feedback, PTAL |
@bscott-rh thanks for your update. Following parts need to be updated in UPI installation doc.
|
subscription: | ||
|The subscription that contains the user-assigned identity. | ||
|String. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon Field "identity" is also under compute and defaultMachinePlatform, and specified single user-assigned identity can also be attached to worker nodes.
Do we need to add them into doc? Or anything I missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think so. @bscott-rh jinyun is correct, we do indeed support setting this on compute nodes (and default machine platform.) so I assume these fields need to be added to those machine pools?
Thanks, I have incorporated these changes. On point 3, the ARM templates are imported from the repo like this: |
type: | ||
|The type of identity used for control plane virtual machine. | ||
The `UserAssigned` identity is a standalone Azure resource provided by the user and assigned to control plane virtual machines. | ||
Identity can only be set for control plane nodes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this line. It is inaccurate (out of date). I have updated openshift/installer#9717 to remove it from the installer code.
* You have installed or updated the link:https://docs.microsoft.com/en-us/cli/azure/install-azure-cli-yum?view=azure-cli-latest[{azure-short} CLI]. | ||
* You have an {azure-short} subscription ID. | ||
* If you are not assigning the `Contributor` and `User Administrator Access` roles to the service principal, you have created a custom role with the required {azure-short} permissions. | ||
* If you are not assigning the `Contributor` role to the service principal, you have created a custom role with the required {azure-short} permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now by default the installer creds need User Admin Access
. Perhaps we want to stet this change? I'm not sure how to best handle the credentials as now the requirements are conditional.
@bscott-rh: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bscott-rh @jinyunma Once the future upstream changes land, the installer will no longer create an identity and the docs should look like they are currently (which is simpler), but until then it seems like the docs will get a bit more complex due to the various configurations.
.Required permissions for creating identity management resources | ||
[%collapsible] | ||
==== | ||
* `Microsoft.ManagedIdentity/userAssignedIdentities/assign/action` | ||
* `Microsoft.ManagedIdentity/userAssignedIdentities/read` | ||
* `Microsoft.ManagedIdentity/userAssignedIdentities/write` | ||
==== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With openshift/installer#9735, the permissions are required with the default configuration, but unnecessary if identity is set to None... So would we want to call them optional? Optional (Required with default config)? I'm also fine leaving this as is (not deleting it) and then adding a separate note saying they are not required when Identity: none...
The following options are available to you: | ||
|
||
* You can assign the identity the `Contributor` and `User Access Administrator` roles. Assigning these roles is the quickest way to grant all of the required permissions. | ||
* You can assign the identity the `Contributor` role. Assigning this roles is the quickest way to grant all of the required permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to stet this, especially as this is suppoed to be the "quickest" way... We could also add something to the effect of "If defaultMachinePool.platform.azure.identity.type
is set to None
only the Contributor
role is needed."
* `Microsoft.Authorization/roleAssignments/read` | ||
* `Microsoft.Authorization/roleAssignments/write` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now these two perms are technically "optional" but are required with the default install config settings...
* `Microsoft.Resources/subscriptions/resourcegroups/write` | ||
==== | ||
.Optional permissions for using a user-assigned identity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
.Optional permissions for using a user-assigned identity | |
.Optional permissions for attaching an existing user-assigned identity to a node |
.Required permissions for deleting authorization resources | ||
[%collapsible] | ||
==== | ||
* `Microsoft.Authorization/roleAssignments/delete` | ||
==== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other comments, this is now needed when the installer creates an identity (default)
Perhaps we should add a Known Issue release note for OCPBUGS-56008 that ACR image pulls will not work (without workarounds) when identity is set to none? |
Closing in favor of #93959 |
Version(s):
4.19
Issue:
https://issues.redhat.com/browse/OSDOCS-13798
Link to docs preview:
Required permissions for IPI
Additional Azure configuration parameters
QE review: