List objects when checking if bucket exists to allow use of container-level SAS token #906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
somtochiama
force-pushed
the
sas-docs
branch
2 times, most recently
from
68a02ac to
d029140
Compare
somtochiama
changed the title
hiddeco
reviewed
Oct 5, 2022
pkg/azure/blob.go
Outdated
Comment on lines
184
to
187
| var max int32 = 1 | ||
| items := container.ListBlobsFlat(&azblob.ContainerListBlobsFlatOptions{ | ||
| MaxResults: &max, | ||
| }) |
There was a problem hiding this comment.
The Azure SDK has a helper for this, see: https://github.com/Azure/azure-sdk-for-go/blob/b386adc15c2e60e3812956f68d244b608330c479/sdk/azcore/to/to.go#L10
Given this, I would likely replace this with:
Suggested change
| var max int32 = 1 | |
| items := container.ListBlobsFlat(&azblob.ContainerListBlobsFlatOptions{ | |
| MaxResults: &max, | |
| }) | |
| items := container.ListBlobsFlat(&azblob.ContainerListBlobsFlatOptions{ | |
| MaxResults: to.Ptr(1), | |
| }) |
hiddeco
reviewed
Oct 5, 2022
docs/spec/v1beta2/buckets.md
Outdated
Comment on lines
540
to
548
| **Note:** SAS Token has an expiry date and it should be updated before it expires so that Flux can | ||
| continue to access Azure Storage. Also, The source-controller can use a account-level SAS token or a container-level SAS token. | ||
| The minimum permissions for an account-level SAS Token is: | ||
| - Allowed services: Blob | ||
| - Allowed resource types: Container, Object | ||
| - Allowed permission: Read, List | ||
|
|
||
| The minimum permissions for a container-level SAS Token is: | ||
| - Permission: Read, List |
There was a problem hiding this comment.
Suggested change
| **Note:** SAS Token has an expiry date and it should be updated before it expires so that Flux can | |
| continue to access Azure Storage. Also, The source-controller can use a account-level SAS token or a container-level SAS token. | |
| The minimum permissions for an account-level SAS Token is: | |
| - Allowed services: Blob | |
| - Allowed resource types: Container, Object | |
| - Allowed permission: Read, List | |
| The minimum permissions for a container-level SAS Token is: | |
| - Permission: Read, List | |
| **Note:** The SAS token has an expiry date and it must be updated before it expires to allow Flux to | |
| continue to access Azure Storage. It is allowed to use an account-level or container-level SAS token. | |
| The minimum permissions for an account-level SAS token are: | |
| - Allowed services: `Blob` | |
| - Allowed resource types: `Container`, `Object` | |
| - Allowed permissions: `Read`, `List` | |
| The minimum permissions for a container-level SAS token are: | |
| - Allowed permissions: `Read`, `List` | |
| Refer to the [Azure documentation](https://learn.microsoft.com/en-us/rest/api/storageservices/create-account-sas#blob-service) for a full overview on permissions. |
pjbgf
reviewed
Oct 5, 2022
pkg/azure/blob.go
Outdated
|
|
||
| // For a container-level SASToken, we get an AuthenticationFailed when the bucket doesn't exist | ||
| if respErr.ErrorCode == string(azblob.StorageErrorCodeAuthenticationFailed) { | ||
| return false, fmt.Errorf("(Bucket might not exist) %w", err) |
There was a problem hiding this comment.
Suggested change
| return false, fmt.Errorf("(Bucket might not exist) %w", err) | |
| return false, fmt.Errorf("%w (Bucket name may be incorrect or non-existent)", err) |
There was a problem hiding this comment.
The error message is a bit long. I was thinking it might not be easily seen by users if placed at the end.
There was a problem hiding this comment.
e.g
3ab-4b60-b393-094065902f97","error":"failed to confirm existence of 'fluxes' bucket: GET https://testfluxsaas.blob.core.windows.net/fluxes\n--------------------------------------------------------------------------------\nRESPONSE 403: 403 Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature.\nERROR CODE: AuthenticationFailed\n--------------------------------------------------------------------------------\n<?xml version=\"1.0\" encoding=\"utf-8\"?><Error><Code>AuthenticationFailed</Code><Message>Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature.\nRequestId:daf54521-101e-003f-5ceb-d8649e000000\nTime:2022-10-05T18:50:34.3475175Z</Message><AuthenticationErrorDetail>Signature did not match. String to sign used was rl\n2022-10-05T10:53:36Z\n2022-10-05T18:53:36Z\n/blob/testfluxsaas/fluxes\n\n\nhttps\n2021-06-08\nc\n\n\n\n\n\n\n</AuthenticationErrorDetail></Error>\n--------------------------------------------------------------------------------\n (Bucket might not exist) "
There was a problem hiding this comment.
Thank you for pointing that out. Starting the error within parenthesis does not feel right, so potentially?
return false, fmt.Errorf("Bucket name may be incorrect, it does not exist or caller does not have enough permissions: %w", err)
hiddeco
reviewed
Oct 6, 2022
| err = respErr | ||
|
|
||
| // For a container-level SASToken, we get an AuthenticationFailed when the bucket doesn't exist | ||
| if respErr.ErrorCode == string(azblob.StorageErrorCodeAuthenticationFailed) { |
There was a problem hiding this comment.
The library seems to have a helper method for this since v0.5.0: Azure/azure-sdk-for-go@0e6a11e#diff-2c126bccb5ce0d58738ebd81c556270990a2f9f71a5bbfe8021f458c919c72aaR18
There was a problem hiding this comment.
The upgrade to v0.5.0 introduces some other changes that I think should come in a different PR.
Wdyt?
hiddeco
reviewed
Oct 7, 2022
docs/spec/v1beta2/buckets.md
Outdated
Comment on lines
543
to
549
| The minimum permissions for an account-level SAS token are: | ||
| - Allowed services: `Blob` | ||
| - Allowed resource types: `Container`, `Object` | ||
| - Allowed permissions: `Read`, `List` | ||
|
|
||
| The minimum permissions for a container-level SAS token are: | ||
| - Allowed permissions: `Read`, `List` |
There was a problem hiding this comment.
Suggested change
| The minimum permissions for an account-level SAS token are: | |
| - Allowed services: `Blob` | |
| - Allowed resource types: `Container`, `Object` | |
| - Allowed permissions: `Read`, `List` | |
| The minimum permissions for a container-level SAS token are: | |
| - Allowed permissions: `Read`, `List` | |
| The minimum permissions for an account-level SAS token are: | |
| - Allowed services: `Blob` | |
| - Allowed resource types: `Container`, `Object` | |
| - Allowed permissions: `Read`, `List` | |
| The minimum permissions for a container-level SAS token are: | |
| - Allowed permissions: `Read`, `List` |
hiddeco
approved these changes
Oct 7, 2022
hiddeco
added
enhancement
Signed-off-by: Somtochi Onyekwere <[email protected]>
Signed-off-by: Somtochi Onyekwere <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #902
Signed-off-by: Somtochi Onyekwere [email protected]