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

Make Kopia support Azure AD #6686

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Conversation

ywk253100
Copy link
Contributor

@ywk253100 ywk253100 commented Aug 22, 2023

This commit introduces our own Azure storage provider by wrapping Kopia's implementation rather than contributing to upstream based on the following considerations:

  1. Velero needs the capability to interact with the repository concurrently while Kopia doesn't, this will increase the complexity of Kopia if we contribute to upstream
  2. The configuration items provided by Velero and Kopia are conflict, e.g. Velero supports customizing storage account URI which is a full path while Kopia supports customizing storage account domain which is part of the URI. We need to consider the backward compatibility and upgrade case if we contribute to upstream which needs extra efforts
  3. Contribute to upstream is a longer cycle when we need to introduce new changes. With this commit, we no longer depends on upstream for the Azure storage provider part and is easy for us to maintain

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@github-actions github-actions bot added Dependencies Pull requests that update a dependency file has-changelog has-unit-tests labels Aug 22, 2023
@anshulahuja98
Copy link
Collaborator

anshulahuja98 commented Aug 22, 2023

" Kopia supports customizing storage account domain"

Should we also make azure plugin conform to that?
I was not aware of the kopia side of things for handling storage endpoints, we can also aim to be inline with them.

@ywk253100 ywk253100 force-pushed the 230612_kopia branch 2 times, most recently from 31f21c9 to 29db4d0 Compare August 22, 2023 06:00
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #6686 (b598150) into main (5af664d) will increase coverage by 0.28%.
The diff coverage is 75.82%.

@@            Coverage Diff             @@
##             main    #6686      +/-   ##
==========================================
+ Coverage   60.47%   60.75%   +0.28%     
==========================================
  Files         242      245       +3     
  Lines       26040    26255     +215     
==========================================
+ Hits        15747    15952     +205     
+ Misses       9187     9171      -16     
- Partials     1106     1132      +26     
Files Changed Coverage Δ
pkg/repository/udmrepo/kopialib/backend/azure.go 54.54% <50.00%> (-36.76%) ⬇️
pkg/util/azure/storage.go 68.20% <68.20%> (ø)
pkg/util/azure/credential.go 74.35% <74.35%> (ø)
pkg/repository/config/azure.go 66.66% <83.33%> (+39.83%) ⬆️
pkg/util/azure/util.go 90.69% <90.69%> (ø)
pkg/repository/provider/unified_repo.go 87.39% <100.00%> (-0.18%) ⬇️
pkg/repository/udmrepo/kopialib/backend/s3.go 90.47% <100.00%> (ø)

... and 1 file with indirect coverage changes

ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this pull request Aug 22, 2023
Both Kopia repository and Azure plugin need to interact with Azure storage. In order to keep in sync with the new features for both these two places and remove duplicated code, PR vmware-tanzu/velero#6686 makes the common code as util functions in Velero repository.

This commit refactors the Azure plugin based on these util functions.

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this pull request Aug 22, 2023
Both Kopia repository and Azure plugin need to interact with Azure storage. In order to keep in sync with the new features for both these two places and remove duplicated code, PR vmware-tanzu/velero#6686 makes the common code as util functions in Velero repository.

This commit refactors the Azure plugin based on these util functions.

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
@ywk253100 ywk253100 force-pushed the 230612_kopia branch 3 times, most recently from 1867843 to 0cde964 Compare August 22, 2023 07:50
@ywk253100
Copy link
Contributor Author

" Kopia supports customizing storage account domain"

Should we also make azure plugin conform to that? I was not aware of the kopia side of things for handling storage endpoints, we can also aim to be inline with them.

@anshulahuja98 We don't need to. The storageAccountURI is flexible enough to address #6163 while Kopia's storage account domain isn't.

BTW, I also submitted a draft PR vmware-tanzu/velero-plugin-for-microsoft-azure#206 to refactor the Azure plugin to use the util functions introduced by this PR and remove the duplicated code. Any new features for the Azure storage related need to be implemented on the Velero side first after this PR is merged. This will avoid implementing new features only work for Azure plugin but not for Kopia repository and keep features consistent.

@ywk253100
Copy link
Contributor Author

I have done some manual test to cover:

  • Auth with client secret and useAAD=false:
    • B/R with native snapshot
    • B/R with Kopia fs backup
    • B/R with Restic fs backup
    • B/R with data mover
  • Auth with client secret and useAAD=true:
    • B/R with native snapshot
    • B/R with Kopia fs backup
    • B/R with data mover
  • Auth with Azure AD Workload Identity and useAAD=true:
    • B/R with native snapshot
    • B/R with data mover
  • Auth with storage account access key:
    • B/R with data mover

@anshulahuja98
Copy link
Collaborator

" Kopia supports customizing storage account domain"
Should we also make azure plugin conform to that? I was not aware of the kopia side of things for handling storage endpoints, we can also aim to be inline with them.

@anshulahuja98 We don't need to. The storageAccountURI is flexible enough to address #6163 while Kopia's storage account domain isn't.

BTW, I also submitted a draft PR vmware-tanzu/velero-plugin-for-microsoft-azure#206 to refactor the Azure plugin to use the util functions introduced by this PR and remove the duplicated code. Any new features for the Azure storage related need to be implemented on the Velero side first after this PR is merged. This will avoid implementing new features only work for Azure plugin but not for Kopia repository and keep features consistent.

okay got it

@kaovilai
Copy link
Contributor

kaovilai commented Sep 6, 2023

our own Azure storage provider by copying Kopia's Azure storage provider code and wrapping it

I think it will be harder to maintain compatibility if we include this code separately. I suggests that we fork, rather than copy.

That way we have commit history specific to this and we are able to easily rebase with upstream kopia when needed.
There maybe more projects in the future that velero needs to fork in this way. So I suggest that we get conversations started on how velero as a project should consume and maintain it's modified version of externally originated code.

@sseago
Copy link
Collaborator

sseago commented Sep 6, 2023

The other advantage of a fork is it provides the opportunity to submit the changes upstream. While they may not always be accepted (especially when goals of the projects differ), when a commit is accepted upstream, then we can remove the local commit in the future and reduce our maintenance burden. Also, we already have a velero fork of kopia for a prior bug fix (although we should still eventually get it into an official velero repo), so we could just make that change in the same repo.

@kaovilai
Copy link
Contributor

kaovilai commented Sep 6, 2023

If we aren't forking.. The alternative IMO is to use git submodules which will keep history of original code + patches.

ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this pull request Sep 8, 2023
Both Kopia repository and Azure plugin need to interact with Azure storage. In order to keep in sync with the new features for both these two places and remove duplicated code, PR vmware-tanzu/velero#6686 makes the common code as util functions in Velero repository.

This commit refactors the Azure plugin based on these util functions.

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
@ywk253100 ywk253100 changed the title Make Kopia support Azure AD [WIP]Make Kopia support Azure AD Sep 13, 2023
ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this pull request Sep 13, 2023
Both Kopia repository and Azure plugin need to interact with Azure storage. In order to keep in sync with the new features for both these two places and remove duplicated code, PR vmware-tanzu/velero#6686 makes the common code as util functions in Velero repository.

This commit refactors the Azure plugin based on these util functions.

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
@ywk253100 ywk253100 force-pushed the 230612_kopia branch 4 times, most recently from a81b779 to 4d7a00b Compare September 18, 2023 01:42
@ywk253100 ywk253100 changed the title [WIP]Make Kopia support Azure AD Make Kopia support Azure AD Sep 18, 2023
@ywk253100
Copy link
Contributor Author

@Lyndon-Li @sseago @kaovilai @blackpiglet I have removed the copied code, please review this PR again.

This commit introduces our own Azure storage provider by wrapping Kopia's implementation rather than contributing to upstream based on the following considerations:
1. Velero needs the capability to interact with the repository concurrently while Kopia doesn't, this will increase the complexity of Kopia if we contribute to upstream
2. The configuration items provided by Velero and Kopia are conflict, e.g. Velero supports customizing storage account URI which is a full path while Kopia supports customizing storage account domain which is part of the URI. We need to consider the backward compatibility and upgrade case if we contribute to upstream which needs extra efforts
3. Contribute to upstream is a longer cycle when we need to introduce new changes. With this commit, we no longer depends on upstream for the Azure storage provider part and is easy for us to maintain

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
Support setting CA cert for BSL

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
@ywk253100 ywk253100 merged commit 63c6a48 into vmware-tanzu:main Sep 19, 2023
24 checks passed
ywk253100 added a commit to ywk253100/velero-plugin-for-microsoft-azure that referenced this pull request Sep 20, 2023
Both Kopia repository and Azure plugin need to interact with Azure storage. In order to keep in sync with the new features for both these two places and remove duplicated code, PR vmware-tanzu/velero#6686 makes the common code as util functions in Velero repository.

This commit refactors the Azure plugin based on these util functions.

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Pull requests that update a dependency file has-changelog has-unit-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants