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

Add Support for SAS keys in Azure Blob #738

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

somtochiama
Copy link
Member

This pull request adds support for SAS Keys in Azure Blob.
The SAS token is specified in the secret under sasKey field.

Fixes: #719

Signed-off-by: Somtochi Onyekwere somtochionyekwere@gmail.com

@pjbgf pjbgf added the area/storage Storage related issues and pull requests label May 26, 2022
pkg/azure/blob.go Outdated Show resolved Hide resolved
docs/spec/v1beta2/buckets.md Outdated Show resolved Hide resolved
@stefanprodan stefanprodan added area/bucket Bucket related issues and pull requests and removed area/storage Storage related issues and pull requests labels May 26, 2022
@somtochiama somtochiama requested a review from pjbgf June 10, 2022 14:54
pkg/azure/blob.go Outdated Show resolved Hide resolved
pkg/azure/blob.go Outdated Show resolved Hide resolved
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

A few nits. And I think we need some safeguards to ensure SAS tokens would leak into errors.

pkg/azure/blob.go Outdated Show resolved Hide resolved
docs/spec/v1beta2/buckets.md Outdated Show resolved Hide resolved
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise LGTM.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM

thanks for getting this implemented @somtochiama! 🙇‍♂️

@pjbgf pjbgf requested a review from stefanprodan June 30, 2022 09:02
@somtochiama somtochiama requested a review from pjbgf July 7, 2022 19:05
@pjbgf
Copy link
Member

pjbgf commented Jul 8, 2022

@somtochiama do you mind rebasing please?

Comment on lines 535 to 536
The query values from the `sasKey` data field in the Secrets gets merged with the `spec.endpoint` of the `Bucket`.
If the same key is present in the both of them, the token takes precedence.
Copy link
Member

@hiddeco hiddeco Jul 8, 2022

Choose a reason for hiding this comment

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

I feel this could use more explanation. In combination with looking at the doc page of the added ref, I had at least the following questions:

  • Does the sasKey only contain the "SAS token"?

  • If so, is it expected to be prefixed with ? as suggested in the documentation?

  • What does

    If the same key is present in the both of them, the token takes precedence.

    entail precisely?

@pjbgf pjbgf added this to the GA milestone Jul 26, 2022
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@pjbgf
Copy link
Member

pjbgf commented Aug 12, 2022

@somtochiama thank you for addressing @hiddeco 's point. Can you rebase please?

@pjbgf pjbgf merged commit fc5dc4d into fluxcd:main Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bucket Bucket related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add support for SAS Keys for Azure Blob
4 participants