migrate from adal to azidentity #83
Conversation
sp98
changed the title
sp98
force-pushed
the
use-azidentity
branch
2 times, most recently
from
b0d2f1d
to
b33f0b1
Compare
sp98
changed the title
sp98
changed the title
sp98
force-pushed
the
use-azidentity
branch
3 times, most recently
from
3c002ec
to
a202e93
Compare
sp98
changed the title
|
@dahuang-purestorage Hi, can you please take a look at this PR. The change to support certificates based auth was very small to included in the same PR. If you want, I can open a different PR for that as well. Thanks. cc @adityadani |
azure/azure_kv.go
Outdated
| if strings.EqualFold(cloudEnv, "AzureUSGovernment") { | ||
| opts.Cloud = cloud.AzureGovernment | ||
| } else if strings.EqualFold(cloudEnv, "AzureChinaCloud") { | ||
| opts.Cloud = cloud.AzureChina | ||
| } else { | ||
| opts.Cloud = cloud.AzurePublic | ||
| } |
There was a problem hiding this comment.
Do we need to do this? Can we not pass a user-defined string to the libopenstorage/secrets lib here so that Rook doesn't need to do custom processing?
There was a problem hiding this comment.
The current version in the master is using this. So if we don't do this it might be a breaking change for users.
I'll check again if we really need this or not.
There was a problem hiding this comment.
Maybe we can add this logic or these "AzureUSGovernment"/"AzureChinaCloud" strings to the libopenstorage library. But in general, I am concerned about hard coding these values.
It's entirely possible and reasonable that Azure might add other clouds like "AzureEuropeCloud", "AzureRussiaCloud", or any other reasonably big use-case where data security needs to be different from other clouds, and if we force any unrecognized option to use the public cloud, those users might be broken. It seems best to me just to pass in the value given to Rook into the lib without trying to validate it.
There was a problem hiding this comment.
I was using AzurePublicCloud for any unrecognized option. That's wrong. I've updated to use only AzurePublicCloud only if the user has set it or if user does not provide an option.
There was a problem hiding this comment.
It seems best to me just to pass in the value given to Rook into the lib without trying to validate it.
I think this would result in breaking changes to the users already using this library. Because they are only expected to provide the AZURE_ENVIRONMENT (string) and then libopenshift/secrets finds out the exact environment based on the AZURE_ENVIRONMENT
sp98
force-pushed
the
use-azidentity
branch
3 times, most recently
from
8f7772b
to
5a08606
Compare
| @@ -109,7 +132,7 @@ func (az *azureSecrets) GetSecret( | |||
| return nil, secrets.NoVersion, err | |||
| } | |||
|
|
|||
| secretResp, ok := resp.(keyvault.SecretBundle) | |||
| secretResp, ok := resp.(azsecrets.GetSecretResponse) | |||
There was a problem hiding this comment.
Azure now returns secret version.
can we return version as well?
secretResp.ID.Version()There was a problem hiding this comment.
No other KMS in the library is returning the version currently. So I think for now, we can keep it as it is and revisit this later.
There was a problem hiding this comment.
AWS Secrets Manager returns the version.The only reason all providers have not been updated to return the version is lack of bandwidth to check and test if each one of them If we are modifying this provider and you are able to verify its correctness we can modify it
adal is is out of support since March 31, 2023. This PR migrates from adal to azidentity for azure key vault Signed-off-by: sp98 <sapillai@redhat.com>
| // don't retry if the secret key was not found | ||
| responseError, ok := err.(*azcore.ResponseError) | ||
| if ok && responseError.StatusCode == 404 { | ||
| return nil, false, secrets.ErrSecretNotFound |
There was a problem hiding this comment.
Can you return this error instead. - ErrInvalidSecretId since all the other providers are returning it when a secret is not found.
There was a problem hiding this comment.
@adityadani Sorry for the delay. I'll update the PR this week.
| @@ -109,7 +132,7 @@ func (az *azureSecrets) GetSecret( | |||
| return nil, secrets.NoVersion, err | |||
| } | |||
|
|
|||
| secretResp, ok := resp.(keyvault.SecretBundle) | |||
| secretResp, ok := resp.(azsecrets.GetSecretResponse) | |||
There was a problem hiding this comment.
AWS Secrets Manager returns the version.The only reason all providers have not been updated to return the version is lack of bandwidth to check and test if each one of them If we are modifying this provider and you are able to verify its correctness we can modify it
|
@adityadani Hi. This got delayed a bit due to other commitments. I plan to address your review comments this week. Hope we can get this merged soon. Thanks. |
adal is out of support since March 31, 2023.
This PR migrates from adal to azidentity for azure key vault.
Migration guide was used as a reference.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional)
Closes #84
Special notes for your reviewer: