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 MSI Support for Azure plugin. #6938

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

yanggangtony
Copy link
Contributor

@yanggangtony yanggangtony commented Oct 10, 2023

Thank you for contributing to Velero!

Please add a summary of your change

Add MSI Support for Azure plugin.

Does your change fix a particular issue?

Fixes #(#6931)

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.

pkg/util/azure/credential.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #6938 (c8ffce8) into main (ed441de) will increase coverage by 0.01%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main    #6938      +/-   ##
==========================================
+ Coverage   60.75%   60.77%   +0.01%     
==========================================
  Files         250      250              
  Lines       26632    26639       +7     
==========================================
+ Hits        16180    16189       +9     
+ Misses       9302     9299       -3     
- Partials     1150     1151       +1     
Files Coverage Δ
pkg/util/azure/credential.go 69.41% <57.14%> (-4.95%) ⬇️

... and 1 file with indirect coverage changes

@yanggangtony
Copy link
Contributor Author

@ywk253100
How about like this change?

@yanggangtony
Copy link
Contributor Author

@ywk253100
thanks for quick review.
rebased.

@anshulahuja98
Copy link
Collaborator

@yanggangtony do you have an environment to test this?
Or should I test and update you.

Can you also make a PR for azure plugin consuming this, I'll pull the commit from your branch to test out in cluster.

@yanggangtony
Copy link
Contributor Author

yanggangtony commented Oct 11, 2023

@yanggangtony do you have an environment to test this? Or should I test and update you.

@anshulahuja98
No , i have no azure env in my local test. your help to test is much appreciate.

Can you also make a PR for azure plugin consuming this, I'll pull the commit from your
branch to test out in cluster.

Ok , i will go to the azure plugin repo , and found the call to consume that .


There is seems some unit test wrong with func TestNewCredential(t *testing.T) {

@yanggangtony
Copy link
Contributor Author

@anshulahuja98
vmware-tanzu/velero-plugin-for-microsoft-azure#212 .
Please help to test that in azure cluster.
Thanks.

@anshulahuja98
Copy link
Collaborator

@anshulahuja98 vmware-tanzu/velero-plugin-for-microsoft-azure#212 . Please help to test that in azure cluster. Thanks.

Ack
will test and get back.

@anshulahuja98
Copy link
Collaborator

Update: I tried to run some automated tests yesterday, but did some mistakes

will try again today, and get back once done.

@yanggangtony
Copy link
Contributor Author

thanks .
@anshulahuja98

@anshulahuja98
Copy link
Collaborator

Just completed validation, it is working as expected.
@yanggangtony can you please check the failing tests in the CI. Also see if it is feasible to add test for this scenario.

@yanggangtony
Copy link
Contributor Author

ok , i will check it later.

@yanggangtony yanggangtony force-pushed the add-msi-support branch 3 times, most recently from a81fa53 to 4ad957a Compare October 13, 2023 12:09
@yanggangtony
Copy link
Contributor Author

i will continue test..
still show the errors..

Signed-off-by: yanggang <gang.yang@daocloud.io>
@yanggangtony
Copy link
Contributor Author

yanggangtony commented Oct 13, 2023

interesting!! @anshulahuja98
i debug the code , found the NewCredential will always return ok, not error shows..
but when you use it to GetToken , will shows the details errors.

image

@yanggangtony
Copy link
Contributor Author

the ci happy now..

@anshulahuja98 anshulahuja98 merged commit 7ca33f8 into vmware-tanzu:main Oct 16, 2023
@anshulahuja98
Copy link
Collaborator

@yanggangtony please update the azure PR as well with latest reference

@yanggangtony yanggangtony deleted the add-msi-support branch October 16, 2023 05:32
@yanggangtony
Copy link
Contributor Author

Ack
@anshulahuja98

blackpiglet pushed a commit to blackpiglet/velero that referenced this pull request Oct 19, 2023
Signed-off-by: yanggang <gang.yang@daocloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants