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

Upgrade to non-legacy Azure SDK #459

Merged
merged 9 commits into from
May 3, 2024

Conversation

ramondeklein
Copy link
Contributor

This PR upgrades from the legacy Azure SDK to the current Azure SDK for Go.

@ramondeklein ramondeklein marked this pull request as draft April 8, 2024 19:25
@harshavardhana
Copy link
Member

There is a PR here doing this already @ramondeklein

@harshavardhana
Copy link
Member

#430

@ramondeklein
Copy link
Contributor Author

ramondeklein commented Apr 8, 2024

Didn't see that one... Will check next time, before implementation.

I will take the best parts of both PRs and merge it into a single PR. This PR has some advantages:

  • Allows for multiple credential methods (client/secret, environment, CLI, managed identity, workload identity, ...). The existing client/secret configuration is possible, but otherwise it tries to fall back to other Azure authentication methods. Using workload or managed identity is a good alternative to client/secret, because it doesn't need any secrets (much safer).
  • No runtime inspections needed to check for errors (using the azcore.ResponseError error type).

I do like the integration tests of the other PR, so I'll bring these over to this PR.

@ramondeklein
Copy link
Contributor Author

ramondeklein commented Apr 8, 2024

I started off with https://github.com/jiuker/kes/tree/upgrade_azserect_client and made some changes:

  • If no explicit Azure credentials are specified (client/secret or managed identity), then it falls back to the default Azure credentials that could be stored in the environment, filesystem, ... This also allows the use of workload identifier and using Azure Keyvault during development by simply running az login.
  • No runtime inspections needed to check for errors (using the azcore.ResponseError error type).
  • Code optimization (no need for intermediate errorResponse type, because we already have the status type).
  • Extended test-case.

During testing, I also found an issue that deleting secrets wasn't always handled properly. Recreating a secret with the same name after deleting failed sometimes. That has been fixed.

@ramondeklein ramondeklein force-pushed the upgrade-azure-sdk branch 2 times, most recently from 7767c58 to e59983b Compare April 8, 2024 22:26
@ramondeklein
Copy link
Contributor Author

ramondeklein commented Apr 8, 2024

@harshavardhana I have rebased @jiuker changes to our current master and applied my changes in commit e59983b768645b7bcd7e53f82ecbb6c04e934562. This could supersede #430.

@ramondeklein ramondeklein marked this pull request as ready for review April 8, 2024 22:29
@ramondeklein
Copy link
Contributor Author

ramondeklein commented Apr 8, 2024

@aead Why does it explicitly try to load the first version? If no version is supplied, then the default Azure KeyVault behavior is to fetch the most recent version. Because we don't update secrets, there is always only one version, so the actual behaviour wouldn't change. It would make implementation a bit simpler though.

I could add that in this PR.

@jiuker jiuker requested review from harshavardhana, aead and ravindk89 and removed request for harshavardhana and aead April 9, 2024 03:26
@jiuker
Copy link

jiuker commented Apr 9, 2024

@ramondeklein fix lint plz.

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

Do you have access to Azure Vault to test this?

internal/keystore/azure/error.go Outdated Show resolved Hide resolved
@ramondeklein
Copy link
Contributor Author

Do you have access to Azure Vault to test this?

I tested this in my private Azure account, but I could create instructions on how to test it in your own Azure account. As an alternative, I can sent you the credentials (via Slack) on how to test this in my account.

@ramondeklein
Copy link
Contributor Author

@harshavardhana I have added a script in internal/keystore/azure/create-keyvault.sh that will create an Azure key-vault in your current subscription and add the required permission to use it in the unit-test. If you don't have an Azure account, then I can sent you credentials via Slack if you would like to test for yourself.

internal/keystore/azure/error.go Outdated Show resolved Hide resolved
internal/keystore/azure/key-vault.go Show resolved Hide resolved
internal/keystore/azure/key-vault.go Show resolved Hide resolved
@ramondeklein
Copy link
Contributor Author

Processed review comment (only for reporting error for JSON deserialization issue) and rebased on the current master branch.

@harshavardhana
Copy link
Member

PTAL @aead

@harshavardhana harshavardhana merged commit 4b6f9dc into minio:master May 3, 2024
7 checks passed
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.

None yet

3 participants