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.

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

OSDOCS#5384: Document the explicit list of required credential permissions for Azure #56274

Merged

Conversation

xenolinux
Copy link
Contributor

@xenolinux xenolinux commented Feb 22, 2023

Document the list of required permissions to create and delete the OCP cluster on the Azure Public Cloud for installer-provisioned infrastructure and user-provisioned infrastructure.

Version(s): 4.12+

Issue: https://issues.redhat.com/browse/OSDOCS-5384

Link to docs preview:

The note in modules/installation-azure-create-resource-group-and-identity.adoc: https://56274--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_azure/installing-azure-user-infra.html#installation-azure-create-resource-group-and-identity_installing-azure-user-infra

Changes in the file modules/installation-azure-finalizing-encryption.adoc:
https://56274--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_azure/installing-azure-customizations.html#finalizing-encryption_installing-azure-customizations

Changes in file modules/installation-azure-permissions.adoc:
https://56274--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_azure/installing-azure-user-infra.html#installation-azure-permissions_installing-azure-user-infra

A note in the file modules/installation-azure-service-principal.adoc:
https://56274--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_azure/installing-azure-account.html#installation-azure-service-principal_installing-azure-account

Preview for IPI related module modules/minimum-required-permissions-ipi-azure.adoc:
https://56274--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_azure/installing-azure-account.html#minimum-required-azure-permissions_installing-azure-account

Preview for UPI related module modules/minimum-required-permissions-upi-azure.adoc : https://56274--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_azure/installing-azure-user-infra.html#minimum-required-azure-permissions_installing-azure-user-infra

QE review:

  • QE has approved this change.

Additional information:

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 22, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Feb 22, 2023

🤖 Updated build preview is available at:
https://56274--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/13288

@xenolinux xenolinux changed the title OSDOCS#5384: Document the explicit list of required credential permissions for Azure [WIP] OSDOCS#5384: Document the explicit list of required credential permissions for Azure Feb 22, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 22, 2023
@xenolinux xenolinux force-pushed the minm-required-permissions-azure branch from 2d89786 to 519c2d8 Compare February 28, 2023 05:09
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 28, 2023
@xenolinux xenolinux force-pushed the minm-required-permissions-azure branch 3 times, most recently from bfa1b60 to 353b791 Compare February 28, 2023 05:44
@xenolinux xenolinux changed the title [WIP] OSDOCS#5384: Document the explicit list of required credential permissions for Azure OSDOCS#5384: Document the explicit list of required credential permissions for Azure Feb 28, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2023
@xenolinux xenolinux force-pushed the minm-required-permissions-azure branch 11 times, most recently from caaaa02 to 80e4395 Compare February 28, 2023 06:43
Copy link
Member

@chiragkyal chiragkyal left a comment

Choose a reason for hiding this comment

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

I've suggested a few changes for IPI part, we can do similar changes for UPI also.

[id="minimum-required-azure-permissions_{context}"]
= Required Azure permissions in installer-provisioned infrastructure

To deploy and destroy an {product-title} cluster with installer-provisioned infrastructure, the service account requires the following permissions.
Copy link
Member

Choose a reason for hiding this comment

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

Service Principal in-place of service account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@@ -11,6 +11,4 @@
* `User Access Administrator`
* `Owner`
Copy link
Member

Choose a reason for hiding this comment

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

This should be Contributor. I will create a separate bug to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Copy link
Member

Choose a reason for hiding this comment

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

@xenolinux We can address this as part of separate PR, may be through a Bug, as this is not part of this work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Ack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting the changes

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created the PR for above bug #56648

[id="minimum-required-azure-permissions_{context}"]
= Required Azure permissions in installer-provisioned infrastructure

To deploy and destroy an {product-title} cluster with installer-provisioned infrastructure, the service account requires the following permissions.
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it little more informative to have a link from the previous section Required Azure roles ?
may be like this :

When you assign the Contributor and User Access Administrator roles to the service principal, you automatically grant all of the required permissions. Specifically, to deploy and destroy an OpenShift Container Platform cluster with installer-provisioned infrastructure, the service principal requires the following permissions.

^^ Please change any wordings / formatting as per convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

* `Microsoft.Storage/storageAccounts/listKeys/action`
====

.Additional permissions for creating marketplace virtual machine resources
Copy link
Member

Choose a reason for hiding this comment

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

There are some ongoing discussion regarding marketplace image. Adding this comment as a place holder to look back. cc @jinyunma

Copy link

Choose a reason for hiding this comment

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

got it, I will update the status once there is any progress for marketplace image testing.





Copy link
Member

Choose a reason for hiding this comment

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

we should add a sentence here about custom role creation. Something like :

You can create a Custom Role having all the above mentioned permissions and assign that custom role to the service principal. To create a custom role in Azure portal, see the Azure custom roles in the Azure documentation.

^^ Please change any wordings / formatting as per convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

* `Microsoft.Network/virtualNetworks/delete`
====

.Required permissions for deleting health resources
Copy link
Member

Choose a reason for hiding this comment

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

Required permissions for checking health of resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

* `Microsoft.Resourcehealth/healthevent/Updated/action`
====

.Required permissions for deleting subscription resources
Copy link
Member

Choose a reason for hiding this comment

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

Required permissions for deleting resource group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

* `Microsoft.MarketplaceOrdering/offertypes/publishers/offers/plans/agreements/write`
====

.Additional permissions for creating compute resources
Copy link
Member

Choose a reason for hiding this comment

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

Should we call them additional or optional ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optional sounds appropriate. Addressed

modules/minimum-required-permissions-ipi-azure.adoc Outdated Show resolved Hide resolved
modules/minimum-required-permissions-ipi-azure.adoc Outdated Show resolved Hide resolved
@xenolinux xenolinux force-pushed the minm-required-permissions-azure branch 6 times, most recently from e0f1e30 to 5d982fd Compare March 1, 2023 08:24
@jinyunma
Copy link

jinyunma commented Apr 4, 2023

@xenolinux thanks for addressing, the update looks good to me

@bscott-rh
Copy link
Contributor

LGTM.

/remove-label peer-review-in-progress
/remove-label peer-review-needed
/label peer-review-done

@openshift-ci openshift-ci bot added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 4, 2023
@xenolinux
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Apr 4, 2023
@ousleyp ousleyp added merge-review-in-progress Signifies that the merge review team is reviewing this PR branch/enterprise-4.12 branch/enterprise-4.13 and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Apr 4, 2023
@ousleyp ousleyp added this to the Continuous Release milestone Apr 4, 2023
Copy link
Member

@ousleyp ousleyp left a comment

Choose a reason for hiding this comment

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

Great work overall! However, the module IDs are incorrect on both new modules, so please fix those before re-requesting merge review. Thanks!

PS. You might want to get a final merge review from someone who usually works on these install docs, but as far as I can tell, everything other than the module IDs LGTM.

//
// * installing/installing_azure/installing-azure-user-infra.adoc

[id="minimum-required-azure-permissions_{context}"]
Copy link
Member

Choose a reason for hiding this comment

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

This ID is the same as the one in modules/minimum-required-permissions-ipi-azure.adoc. Please change both module IDs so that they match the corresponding filename. This one should be: [id="minimum-required-permissions-upi-azure_{context}"]

Thanks!

//
// * installing/installing_azure/installing-azure-account.adoc

[id="minimum-required-azure-permissions_{context}"]
Copy link
Member

Choose a reason for hiding this comment

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

This ID is the same as the one in modules/minimum-required-permissions-upi-azure.adoc. Please change both module IDs so that they match the corresponding filename. Ideally, they would also match the module title, but having them match the filename is more important.

This one should be: [id="minimum-required-permissions-ipi-azure_{context}"]

@ousleyp ousleyp removed the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Apr 4, 2023
@xenolinux xenolinux force-pushed the minm-required-permissions-azure branch from 3f8d8b3 to eafc6ee Compare April 5, 2023 00:49
@xenolinux
Copy link
Contributor Author

Great work overall! However, the module IDs are incorrect on both new modules, so please fix those before re-requesting merge review. Thanks!

Thanks. I corrected the module IDs.

PS. You might want to get a final merge review from someone who usually works on these install docs, but as far as I can tell, everything other than the module IDs LGTM.

Yes. I have got it reviewed by @bscott-rh before setting the merge-review-needed label.

@xenolinux
Copy link
Contributor Author

Acknowledged the comments. Setting the merge-review-needed label again.

@xenolinux
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Apr 5, 2023
@sheriff-rh sheriff-rh added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Apr 5, 2023
Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

Pan's requests were addressed, content lgtm otherwise. Merging.

@sheriff-rh sheriff-rh merged commit 40e012b into openshift:main Apr 5, 2023
1 check passed
@sheriff-rh
Copy link
Contributor

/cherrypick enterprise-4.13

@sheriff-rh
Copy link
Contributor

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

@sheriff-rh: new pull request created: #58247

In response to this:

/cherrypick enterprise-4.13

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.

@openshift-cherrypick-robot

@sheriff-rh: new pull request created: #58248

In response to this:

/cherrypick enterprise-4.12

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.12 branch/enterprise-4.13 merge-review-in-progress Signifies that the merge review team is reviewing this PR peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants