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

Don't use storage account key for object storage if SP or MI is provided #111

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

yvespp
Copy link
Contributor

@yvespp yvespp commented Nov 18, 2021

Implements vmware-tanzu/velero#4267

The plugin no longer uses the storage access key to access the blobs by default. The key is only used if it's configured via AZURE_STORAGE_ACCOUNT_ACCESS_KEY. Therefore "Allow storage account key access" can be disabled on the storage account if using MI or SP.

Changes:

  • Switched object_store to the newer azblobclient
  • When the storage account access key is not provided delegation SAS instead of service SAS is used.
    • delegation SAS isn't implemented in the azblob client yet so I implemented a simple version in delegationsas.go. This code can be delete once it's implemented in azblob
    • resourceGroupConfigKey and subscriptionIDConfigKey are no longer needed for the object store
  • Added an integration test for the object_storage
    • Only runs when the tag integration is specified e.g. go test ./... -tags=integration -v
    • The storage account for the test has to be created separatly.
  • Updated README
  • Updated deps
    • go 1.17
    • go dependencies
    • busybox image

Copy link
Collaborator

@sseago sseago left a comment

Choose a reason for hiding this comment

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

Just a question around the golang version for now -- I'll look at this in more detail a bit later.

@@ -13,10 +13,10 @@ jobs:
runs-on: ubuntu-latest
steps:

- name: Set up Go 1.14
- name: Set up Go 1.17
Copy link
Collaborator

Choose a reason for hiding this comment

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

velero still uses go 1.16 on the main branch. We should probably do the same here unless there is a specific need for 1.17 for this plugin.

Dockerfile Outdated
@@ -12,12 +12,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM golang:1.13-buster AS build
FROM golang:1.17-buster AS build
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, velero still uses go 1.16 on the main branch. We should probably do the same here unless there is a specific need for 1.17 for this plugin.

@ywk253100 ywk253100 self-requested a review November 19, 2021 03:11
@yvespp
Copy link
Contributor Author

yvespp commented Nov 19, 2021

I downgraded it to 1.16 and also added a go setup step to pr.yaml, it seems it was using the preinstalled version before.

@yvespp
Copy link
Contributor Author

yvespp commented Nov 26, 2021

Hi!

Has someone time to review this?
BTW: is there a way to run integration tests against Azure in this repo?

Thanks,
Yves

@yvespp yvespp force-pushed the delegation_sas branch 2 times, most recently from 34066b0 to b43a9b8 Compare February 16, 2022 10:13
@yvespp
Copy link
Contributor Author

yvespp commented Feb 16, 2022

Rebased to master.
Any chance this can be merged?

@nxf5025
Copy link

nxf5025 commented Feb 17, 2022

This is exactly what we need as well and makes a lot more sense than dealing with access keys when they aren't necessary.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #111 (e7efb9e) into main (73d1530) will increase coverage by 1.29%.
The diff coverage is 7.00%.

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   12.77%   14.07%   +1.29%     
==========================================
  Files           4        4              
  Lines         626      668      +42     
==========================================
+ Hits           80       94      +14     
- Misses        542      571      +29     
+ Partials        4        3       -1     
Impacted Files Coverage Δ
velero-plugin-for-microsoft-azure/object_store.go 2.38% <0.54%> (-0.27%) ⬇️
...o-plugin-for-microsoft-azure/volume_snapshotter.go 23.68% <4.44%> (+0.28%) ⬆️
velero-plugin-for-microsoft-azure/common.go 24.59% <55.55%> (+24.59%) ⬆️

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

@vzabawski
Copy link

Does the PR also addresses this issue? vmware-tanzu/velero#4930

@yvespp
Copy link
Contributor Author

yvespp commented Jul 14, 2022

Does the PR also addresses this issue? vmware-tanzu/velero#4930

Yes, ListKeys will be removed with this PR.

@vzabawski
Copy link

Awesome! Thank you for your contribution.

@chrisob
Copy link

chrisob commented Jul 28, 2022

Reliance on Azure storage account shared keys is preventing my org from using Velero, would be great to see this merged ❤️ @ywk253100 @eleanor-millman

@anshulahuja98
Copy link
Collaborator

HI @yvespp
I was trying to test out this PR
1 basic flow I was not able to validate - I created a BSL with incorrect params(SA didn't exist)
But the BSL is showing as available. Possibly some underlying layers are not returning errors?
The logs also did not help much debugging

When I created a backup, the following error came in the Backup Status. The BSL was still showing as available

│ completionTimestamp: "2022-09-08T05:12:03Z" ││ expiration: "2022-10-08T05:12:03Z" ││ failureReason: | ││ error checking if backup already exists in object storage: rpc error: code = Unknown desc = ===== RESPONSE ERROR (ErrorCode=AuthorizationPermissionMismatch) ===== ││ Description=, Details: (none) ││ formatVersion: 1.1.0 ││ phase: Failed ││ startTimestamp: "2022-09-08T05:12:03Z" ││ version: 1

@ywk253100
Copy link
Contributor

@yvespp Sorry for the late review.

I have added some comments but haven't reviewed the delegationsas.go yet and I will continue to add comments.

@mimmi-gjems
Copy link

Hi! Would be really great if this could be merged sometime soon :) @sseago @ywk253100 @dsu-igeek

@vzabawski
Copy link

I second that. This PR will allow me to start using Velero.

@yvespp yvespp force-pushed the delegation_sas branch 2 times, most recently from 5a45768 to 96b6d31 Compare September 22, 2022 18:09
@yvespp
Copy link
Contributor Author

yvespp commented Sep 26, 2022

HI @yvespp I was trying to test out this PR 1 basic flow I was not able to validate - I created a BSL with incorrect params(SA didn't exist) But the BSL is showing as available. Possibly some underlying layers are not returning errors? The logs also did not help much debugging

When I created a backup, the following error came in the Backup Status. The BSL was still showing as available

│ completionTimestamp: "2022-09-08T05:12:03Z" ││ expiration: "2022-10-08T05:12:03Z" ││ failureReason: | ││ error checking if backup already exists in object storage: rpc error: code = Unknown desc = ===== RESPONSE ERROR (ErrorCode=AuthorizationPermissionMismatch) ===== ││ Description=, Details: (none) ││ formatVersion: 1.1.0 ││ phase: Failed ││ startTimestamp: "2022-09-08T05:12:03Z" ││ version: 1

Hi @anshulahuja98, do you know how the backup location is verified? I was looking through the code but couldn't find it.

@ywk253100
Copy link
Contributor

ywk253100 commented Jan 9, 2023

@yvespp I added a few comments about the document, please take a look.
And please also resolve the conflicts, rebase and squash the commits

yvespp added 2 commits January 9, 2023 12:58
Switches the to newer blob client
Add integration test

Signed-off-by: Yves Peter <[email protected]>
@yvespp
Copy link
Contributor Author

yvespp commented Jan 9, 2023

@yvespp I added a few comments about the document, please take a look. And please also resolve the conflicts, rebase and squash the commits

@ywk253100 I addressed your comments, rebased and squashed.

@ywk253100
Copy link
Contributor

The changes have passed the end-to-end testing running locally. Thanks @yvespp

@sseago Could you take a look at this PR when you are available?

@blackpiglet blackpiglet requested review from blackpiglet and removed request for dsu-igeek January 12, 2023 09:05
@reasonerjt reasonerjt merged commit dc34ed3 into vmware-tanzu:main Jan 16, 2023
@jkroepke
Copy link

jkroepke commented Feb 8, 2023

Hi all, could the PR released? It looks like this PR is not port of the 1.6.1 version.

Current workaround: Add Reader and Data Access Role in addition to Storage Blob Data Contributor on the storage account to avoid the error ... does not have authorization to perform action 'Microsoft.Storage/storageAccounts/listKeys/action' over scope ...

@yvespp
Copy link
Contributor Author

yvespp commented Feb 15, 2023

Hi all, could the PR released? It looks like this PR is not port of the 1.6.1 version.

Current workaround: Add Reader and Data Access Role in addition to Storage Blob Data Contributor on the storage account to avoid the error ... does not have authorization to perform action 'Microsoft.Storage/storageAccounts/listKeys/action' over scope ...

I think the plan is to release this with the next minor version 1.7.0 and Velero 1.11: #111 (comment)

If you want to try it out you can use my image which includes this pull request: https://hub.docker.com/r/yvespp/velero-plugin-for-microsoft-azure
It's still on 1.6.0 though.

@ywk253100
Copy link
Contributor

ywk253100 commented Feb 15, 2023

@yvespp Thanks
@jkroepke The fix will be available in 1.7. And as the changes introduced by this PR are quite fundamental, we'll not back-port the changes to 1.6

ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this pull request Mar 15, 2023
We introduced changes in vmware-tanzu#111 to remove the logic of listing storage account access key, the Velero Azure plugin supports auth via Azure AD directly after the changes, but that isn't enough as Restic/Kopia still doesn't support auth via Azure AD at this moment, this will cause filesystem backup failure on Azure.

So we revert the doc change in this commit and Velero still needs the permission of listing storage access key to work as expected. But as we keep the code changes, users can workaround the permission issue by refer to vmware-tanzu/velero#5984

Signed-off-by: Wenkai Yin(尹文开) <[email protected]>
reasonerjt added a commit that referenced this pull request Mar 19, 2023
Revert the changes made to doc in #111
@vikrantoct7
Copy link

If this PR is already merged, may i know the plugin version in which this PR is available?

@mkemmerz
Copy link

mkemmerz commented Apr 4, 2023

Hello everyone - is there any release date planned for this feature yet? This feature is essential for Azure and I would like to use it :)

@yvespp yvespp deleted the delegation_sas branch April 4, 2023 15:13
@yvespp yvespp restored the delegation_sas branch April 4, 2023 15:13
@ywk253100
Copy link
Contributor

@vikrantoct7 @mkemmerz This change isn't available in Velero 1.11, please see vmware-tanzu/velero#5984 for details.

But if you guys take backup without Restic/Kopia, the Velero Azure plugin can be configured as work without listing storage account access keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.