Upgrade to non-legacy Azure SDK

minio / kes

Key Managament Server for Object Storage and more

https://min.io/docs/kes/concepts/

GNU Affero General Public License v3.0

456 stars 94 forks source link

Upgrade to non-legacy Azure SDK #459

Closed ramondeklein closed 3 months ago

ramondeklein commented 4 months ago

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

harshavardhana commented 4 months ago

There is a PR here doing this already @ramondeklein

harshavardhana commented 4 months ago

https://github.com/minio/kes/pull/430

ramondeklein commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

ramondeklein commented 4 months ago

@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 commented 4 months ago

@ramondeklein fix lint plz.

ramondeklein commented 4 months ago

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 commented 4 months ago

@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.

ramondeklein commented 4 months ago

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

harshavardhana commented 3 months ago

PTAL @aead