Update Azure SDK and support additional authentication schemes #3839

kirat-singh · 2023-02-14T03:31:35Z

Microsoft has updated the golang Azure SDK significantly. Update the azure storage driver to use the new SDK. Add support for client secret and MSI authentication schemes in addition to shared key authentication.

Implement rootDirectory support for the azure storage driver to mirror the S3 driver.

Also support AWS_CA_BUNDLE for the S3 driver.

milosgajdos · 2023-02-14T09:23:20Z

Thanks, this is awesome! Couple of things:

Can we please split the S3 change from the Azure SDK change into separate PR
Can you make sure you signed every commit as per the contributor guidelines

kirat-singh · 2023-02-14T14:45:43Z

@milosgajdos oh sure, apologies I was thinking I should have split them out, my bad. appreciate you taking a look.

kirat-singh · 2023-02-14T17:24:38Z

Hello,

This is done. I split out the AWS_CA_BUNDLE change into a separate pull request. Amended this commit, and signed it.

Appreciate your help.

best,
-Kirat

kirat-singh · 2023-02-14T19:34:56Z

sorry I accidentally assumed gpg signing was the requirement, now properly signed off and go fmt run. Apologies.

milosgajdos

Can we group direct and indirect dependencies into their dedicated required stanzas? We have like 4 groups in go.mod now

kirat-singh · 2023-02-22T20:56:17Z

@milosgajdos good point, done.

kirat-singh · 2023-02-24T21:12:10Z

Hi, I see there's a CodeQL warning which seems to have nothing to do with my change. Not quite sure what I can do to address it.

milosgajdos · 2023-02-24T23:06:28Z

@kirat-singh can you rebase

codecov-commenter · 2023-02-27T16:25:34Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e5d5810) 56.58% compared to head (6f15461) 56.58%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3839   +/-   ##
=======================================
  Coverage   56.58%   56.58%           
=======================================
  Files         105      105           
  Lines       10636    10636           
=======================================
  Hits         6018     6018           
  Misses       3946     3946           
  Partials      672      672

Impacted Files	Coverage Δ
registry/storage/driver/storagedriver.go	`0.00% <ø> (ø)`
registry/storage/blobwriter.go	`33.33% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

milosgajdos · 2023-02-27T18:23:32Z

Seems like this would fix these?

Registry with azure blob storage as backend: storage path #1247
azure fix empty dir name from leading slash, allow setting rootdirectory #3583 (close this once this one's merged)

kirat-singh · 2023-02-27T18:54:04Z

yes looks like those would be resolved. and if you prefer I split out the rootdirectory changes, I'm happy to do so. I really care about the azure auth changes since the existing mechanism is very insecure. It requires you to expose your account key to the registry. This is basically impossible to revoke so it's a really bad idea to distribute it in any way. The token based auth and secret based auth is much much better.

milosgajdos · 2023-02-27T20:54:43Z

No, this is fine. This will take me a bit of time to review as I'm not familiar with Azure API and SDK 😓

milosgajdos · 2023-03-13T11:07:27Z

@kirat-singh not sure what CodQL's problem here is but it seems to have a beef with this PR 😬 I only skimmed through it -- it seems to be crying about logging user-provided values in parts of codebase that havent been touched for ages 🤔

milosgajdos · 2023-03-20T11:51:28Z

I will close this PR and reopen because I suspect during the original CI run CodeQL had a bit of a fit. Let's see if re-triggering the build does the trick (CodeQL folks reported a bug that was accidentally pushed out last week)

registry/storage/driver/azure/parser.go

registry/storage/driver/azure/azure_auth.go

corhere · 2023-04-13T22:12:24Z

registry/storage/driver/azure/azure_auth.go

+	accountName string
+	container   string
+	serviceURL  string


Neither the accountName nor serviceURL fields are accessed. It looks like the values which are assigned to these fields are needed to construct cred and client, but are not needed afterwards.

duh, sorted

registry/storage/driver/azure/azure_auth.go

docs/configuration.md

corhere · 2023-04-13T23:11:00Z

registry/storage/driver/azure/azure.go

 	}
-	info := blobRef.Properties
-	size := info.ContentLength
+	size := *props.ContentLength


Suggested change

size := *props.ContentLength

size := props.ContentLength

FYI: the . field-access operator behaves like the -> operator in the C-family languages when applied to a pointer. Pretty much the only time a pointer-to-struct needs to be explicitly dereferenced is when you want to make a shallow copy. E.g.:

copied := *original

I agree, but in this case ContentLength itself is a pointer to an int64. Go figure. The azure apis are just horrible. So I left it as is.

Whoops, I naively assumed that ContentLength was a scalar without checking the GoDoc.

In idiomatic Go, pointer-to-scalar arguments and fields are often used as a poor man's Option<T> when unset needs to be distinguished from zero. How can we be certain that ContentLength is guaranteed to be != nil? Do we need an if props.ContentLength != nil guard to avoid dereferencing nil?

Hi @corhere, I added defensive checks around these, please let me know if that's reasonable. Honestly I think these are required properties and don't think they can be nil unless there's some serious failure on the azure level, but agree that it's good to guard against.

I agree with all your points @kirat-singh. Returning an error if a property required by the storage driver is missing from the azure API response is much more reasonable than panicking.

registry/storage/driver/azure/azure.go

kirat-singh · 2023-04-18T18:39:29Z

@corhere thank you for the review, I made changes as you suggest. Sorry for the delay, I was a bit busy last week with other stuff.

milosgajdos · 2023-04-18T18:49:34Z

@kirat-singh can you sign your commit, please

kirat-singh · 2023-04-18T19:04:39Z

@milosgajdos oops, sorry I keep forgetting! done.

kirat-singh · 2023-04-19T01:08:22Z

@corhere and pushed a final change with some more fixes. for some reason github had 3 'hidden' conversations. I replied to your comments, but did not resolve them. Let me know if you want me to change anything else.

milosgajdos

I think we are almost there. Just have a few comments

registry/storage/driver/azure/azure.go

registry/storage/driver/azure/parser.go

registry/storage/driver/storagedriver.go

corhere

LGTM, aside from a style nitpick and a follow-up question regarding dereferencing pointer-typed struct fields in the Azure API responses. (Repeated here for visibility: what guarantees do we have that the fields which are unconditionally dereferenced will always be != nil?)

Please squash down the merge and "chore: address feedback" commits to get this PR ready to merge.

corhere · 2023-04-20T19:59:07Z

registry/storage/driver/azure/azure_auth.go

+	UdcGracePeriod = 30.0 * time.Minute
+	UdcExpiryTime  = 48.0 * time.Hour


Nit: https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Suggested change

UdcGracePeriod = 30.0 * time.Minute

UdcExpiryTime = 48.0 * time.Hour

UDCGracePeriod = 30.0 * time.Minute

UDCExpiryTime = 48.0 * time.Hour

Addressed style nitpick. Squashed down commits.

I poked around the Azure APIs, the pointers seem to be an artifact of the code generator they use for the API bindings.
There's some internal code there that parses response headers and sets these pointers if the corresponding headers are present.

So I suppose it is possible that they can be nil. I added a bunch of defensive checks around them.

kirat-singh · 2023-04-24T13:17:12Z

Hi, sorry to chase, but I've made all the requested changes.

milosgajdos · 2023-04-24T13:34:23Z

Ping @corhere

corhere

Almost there!

corhere · 2023-04-24T15:20:42Z

registry/storage/driver/azure/azure.go

 	}
-	info := blobRef.Properties
-	size := info.ContentLength
+	size := *props.ContentLength


I agree with all your points @kirat-singh. Returning an error if a property required by the storage driver is missing from the azure API response is much more reasonable than panicking.

corhere · 2023-04-24T15:22:38Z

registry/storage/driver/azure/azure.go

-	info := blobRef.Properties
-	size := info.ContentLength
+	if props.ContentLength == nil {
+		return nil, fmt.Errorf("Failed to get ContentLength for path: %s", path)


Suggested change

return nil, fmt.Errorf("Failed to get ContentLength for path: %s", path)

return nil, fmt.Errorf("failed to get ContentLength for path: %s", path)

https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Ping @kirat-singh – this error string also needs updating

registry/storage/driver/azure/azure.go

milosgajdos · 2023-04-25T16:29:06Z

@kirat-singh when you accept commit suggestions Github doesnt propagate git signatures, so you'll have to rebase and sign. I know it's silly and has been raised with GH on gazillion occasions to no avail.

kirat-singh · 2023-04-25T16:48:18Z

@milosgajdos thanks! noticed that. squashed, signed, committed. Thanks @corhere as well.

Microsoft has updated the golang Azure SDK significantly. Update the azure storage driver to use the new SDK. Add support for client secret and MSI authentication schemes in addition to shared key authentication. Implement rootDirectory support for the azure storage driver to mirror the S3 driver. Signed-off-by: Kirat Singh <kirat.singh@beacon.io> Co-authored-by: Cory Snider <corhere@gmail.com>

corhere

LGTM!

milosgajdos · 2023-04-25T18:36:05Z

Awesome work @kirat-singh

kirat-singh force-pushed the feature.azure-sdk-update branch from 0b207b5 to 263bf0c Compare February 14, 2023 17:20

kirat-singh force-pushed the feature.azure-sdk-update branch from 263bf0c to 7bae9ab Compare February 14, 2023 19:33

kirat-singh force-pushed the feature.azure-sdk-update branch from 7bae9ab to 9341366 Compare February 15, 2023 13:33

milosgajdos reviewed Feb 22, 2023

View reviewed changes

kirat-singh force-pushed the feature.azure-sdk-update branch from 9341366 to 7b7d77d Compare February 22, 2023 20:55

milosgajdos requested review from deleteriousEffect and wy65701436 February 27, 2023 20:55

milosgajdos closed this Mar 20, 2023

milosgajdos reopened this Mar 20, 2023

This was referenced Mar 28, 2023

Updated Azure Storage Driver with Latest SDK #3029

Closed

error manifest blob unknown: blob unknown to registry #3868

Closed

milosgajdos added this to the Registry/3.0.0 milestone Apr 13, 2023

milosgajdos requested review from corhere and thaJeztah April 13, 2023 10:55

corhere requested changes Apr 14, 2023

View reviewed changes

kirat-singh force-pushed the feature.azure-sdk-update branch from 49ab310 to 88ef150 Compare April 18, 2023 18:42

kirat-singh force-pushed the feature.azure-sdk-update branch from 88ef150 to c62551a Compare April 18, 2023 19:03

milosgajdos reviewed Apr 20, 2023

View reviewed changes

corhere reviewed Apr 20, 2023

View reviewed changes

kirat-singh force-pushed the feature.azure-sdk-update branch 2 times, most recently from 4bfce34 to 0c703ef Compare April 20, 2023 21:18

milosgajdos requested a review from corhere April 21, 2023 13:26

milosgajdos approved these changes Apr 21, 2023

View reviewed changes

corhere reviewed Apr 24, 2023

View reviewed changes

kirat-singh force-pushed the feature.azure-sdk-update branch 2 times, most recently from 63cf8ea to 8df98b5 Compare April 25, 2023 16:47

milosgajdos requested a review from corhere April 25, 2023 17:33

kirat-singh force-pushed the feature.azure-sdk-update branch from 8df98b5 to ba4a6bb Compare April 25, 2023 18:24

corhere approved these changes Apr 25, 2023

View reviewed changes

milosgajdos merged commit 2fb8dbd into distribution:main Apr 25, 2023
12 checks passed

This was referenced Apr 25, 2023

azure fix empty dir name from leading slash, allow setting rootdirectory #3583

Closed

Registry with azure blob storage as backend: storage path #1247

Closed

Update GCS dependencies and fix GCS driver #3702

Closed

marcusirgens mentioned this pull request May 18, 2023

StorageDriver gcs not registered when updating 2.8.1 -> 2.8.2 #3919

Closed

This was referenced Aug 13, 2023

Azure storage driver spent too much resources on SSL connection to storage server #2246

Closed

Azure storage driver IAM support #3273

Closed

jkroepke mentioned this pull request Feb 1, 2024

[Feat]: Support for Azure Storage Account / Blob project-zot/zot#2221

Open

		UdcGracePeriod = 30.0 * time.Minute
		UdcExpiryTime = 48.0 * time.Hour

	return nil, fmt.Errorf("Failed to get ContentLength for path: %s", path)
	return nil, fmt.Errorf("failed to get ContentLength for path: %s", path)

Update Azure SDK and support additional authentication schemes #3839

Update Azure SDK and support additional authentication schemes #3839

Conversation

kirat-singh commented Feb 14, 2023

milosgajdos commented Feb 14, 2023

kirat-singh commented Feb 14, 2023

kirat-singh commented Feb 14, 2023

kirat-singh commented Feb 14, 2023

milosgajdos left a comment

Choose a reason for hiding this comment

kirat-singh commented Feb 22, 2023

kirat-singh commented Feb 24, 2023

milosgajdos commented Feb 24, 2023

codecov-commenter commented Feb 27, 2023 • edited

Codecov Report

milosgajdos commented Feb 27, 2023

kirat-singh commented Feb 27, 2023 • edited

milosgajdos commented Feb 27, 2023

milosgajdos commented Mar 13, 2023

milosgajdos commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirat-singh commented Apr 18, 2023

milosgajdos commented Apr 18, 2023

kirat-singh commented Apr 18, 2023

kirat-singh commented Apr 19, 2023

milosgajdos left a comment

Choose a reason for hiding this comment

corhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirat-singh commented Apr 24, 2023

milosgajdos commented Apr 24, 2023

corhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosgajdos commented Apr 25, 2023

kirat-singh commented Apr 25, 2023

corhere left a comment

Choose a reason for hiding this comment

milosgajdos commented Apr 25, 2023

codecov-commenter commented Feb 27, 2023 •

edited

kirat-singh commented Feb 27, 2023 •

edited