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

Allow AzureBlobHealthCheck to work with "Storage Blob Data Contributor" #806

Conversation

Nisden
Copy link
Contributor

@Nisden Nisden commented Jul 19, 2021

What this PR does / why we need it:

This pull request allows for the AzureBlobHealthCheck to use Azure AD roles for authentication.
Specially it allows an user with the role Storage Blob Data Contributor to run the health check.

Which issue(s) this PR fixes:

This will fix the issue #783

Special notes for your reviewer:

Storage Blob Data Contributor is an Azure AD RBAC role that can read and write data to storage containers. Its typically used together with Managed Identities. The Storage Blob Data Contributor only have a very limited set of permissions (See image)

image

The GetServiceProperties requires account ownership, that you generally don't wanna delegate to your account.
As a note, an Storage Account Key equals full account ownership.

Does this PR introduce a user-facing change?:
There should be no noticeable changes.

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

Copy link
Contributor

@Meertman Meertman left a comment

Choose a reason for hiding this comment

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

Is it necessary to loop over all containers? Would it not be just enough to see that the operation returns?

@MrKevHunter
Copy link
Contributor

MrKevHunter commented Dec 13, 2021

Can this get merged? We really need it

@Nisden
Copy link
Contributor Author

Nisden commented Feb 16, 2022

Any chance you can review and merge this pull request @sungam3r its been pending for quite a while, and is a minor change.

@sungam3r
Copy link
Collaborator

I'm OK to review/merge this one when you rebase your changes onto actual master. The reason is to run CI tests that were added some time ago into master.

@sungam3r sungam3r added the enhancement New feature or request label Feb 16, 2022
@Nisden Nisden force-pushed the AzureBlobStorageHealthCheck_AllowBlobDataContributor branch from 38ae9a4 to 8999056 Compare February 16, 2022 13:20
@Nisden
Copy link
Contributor Author

Nisden commented Feb 16, 2022

@sungam3r its been rebased, but you need to run/approve the workflow.

@sungam3r
Copy link
Collaborator

sungam3r commented Feb 16, 2022

@CarlosLanderas @unaizorrilla global.json looks like this
{ "projects": ["src", "tests", "samples"], "sdk": { "version": "6.0.100" } }

Now all CI will fail because they use 6.0.x version and current version is 6.0.200.
изображение

Options:

  1. Configure global.json - https://docs.microsoft.com/en-us/dotnet/core/tools/global-json?tabs=netcore3x#rollforward
  2. Remove global.json

Honestly, I would prefer the second, I have never seen a lot of benefit in it.

@Nisden Nisden force-pushed the AzureBlobStorageHealthCheck_AllowBlobDataContributor branch from 8999056 to a55a974 Compare February 17, 2022 08:38
@unaizorrilla
Copy link
Collaborator

Hi @sungam3r

What about add rollForward to global.json?

{
  "sdk": {
    "version": "3.1.102",
    "rollForward": "latestPatch"
  }
}

@carlosrecuero carlosrecuero self-assigned this Feb 17, 2022
@carlosrecuero
Copy link
Collaborator

Hi @unaizorrilla , i think is a good and quick option.

@sungam3r
Copy link
Collaborator

@dependabot rebase

@sungam3r sungam3r merged commit 01d9179 into Xabaril:master Feb 23, 2022
@Nisden Nisden deleted the AzureBlobStorageHealthCheck_AllowBlobDataContributor branch February 24, 2022 05:06
@sungam3r sungam3r mentioned this pull request Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants