chore: Replace deprecated autorest SDK with azidentity #1904
Conversation
Codecov ReportAttention: Patch coverage is
|
shahramk64
changed the title
shahramk64
force-pushed
the
skalantari/Refactor-Azure-authentication-support-to-use-azidentity
branch
6 times, most recently
from
6e5c9bd
to
e8c85c4
Compare
shahramk64
requested review from
akashsinghal,
binbin-li,
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
shahramk64
force-pushed
the
skalantari/Refactor-Azure-authentication-support-to-use-azidentity
branch
from
e8c85c4
to
2ba81a3
Compare
There was a problem hiding this comment.
Thanks for the PR @shahramk64 , looks great overall. Would you be able to include all automated and manual validation in the PR description? our automated test may have switched to using kmp, we might need to manually validate certificateprovider path to ensure this provider continues to be functional.
|
Manual Validation test failed. I'll continue working on this PR and will update you once it's fixed |
shahramk64
force-pushed
the
skalantari/Refactor-Azure-authentication-support-to-use-azidentity
branch
from
8d9b220
to
db0f2a9
Compare
|
@susanshi @binbin-li @akashsinghal |
There was a problem hiding this comment.
Thankyou @shahramk64 ! code change looks good to me. I have two ask:
- thank you for providing the validation result for kmp CR, since this change also updated certificate provider CR, would you be able to validate certificateprovider CR and paste the result?
- would you be to review @duffney 's PR to help understand if there will be any updates required to resolve any merge conflict.
| @@ -207,30 +210,42 @@ func parseAzureEnvironment(cloudName string) (*azure.Environment, error) { | |||
| return &env, err | |||
| } | |||
|
|
|||
| func initializeKvClient(ctx context.Context, keyVaultEndpoint, tenantID, clientID string) (*kv.BaseClient, error) { | |||
| kvClient := kv.New() | |||
| func initializeKvClient(ctx context.Context, keyVaultEndpoint, tenantID, clientID string, credProvider azcore.TokenCredential) (*azsecrets.Client, error) { | |||
There was a problem hiding this comment.
correct me if I'm wrong, but I didn't see any caller pass in a non-nil credprovider.
There was a problem hiding this comment.
That's correct. It only is non-nill in unit tests for mocking purposes.
There was a problem hiding this comment.
I see. I feel it's not the best way to add this nil credProvider for testing purpose, but may need some refactoring on how we invoke initKVClient function. It's out of the scope of this PR, could you add some TODO comments so that we can address it later.
| @@ -207,30 +210,42 @@ func parseAzureEnvironment(cloudName string) (*azure.Environment, error) { | |||
| return &env, err | |||
| } | |||
|
|
|||
| func initializeKvClient(ctx context.Context, keyVaultEndpoint, tenantID, clientID string) (*kv.BaseClient, error) { | |||
| kvClient := kv.New() | |||
| func initializeKvClient(ctx context.Context, keyVaultEndpoint, tenantID, clientID string, credProvider azcore.TokenCredential) (*azsecrets.Client, error) { | |||
There was a problem hiding this comment.
I see. I feel it's not the best way to add this nil credProvider for testing purpose, but may need some refactoring on how we invoke initKVClient function. It's out of the scope of this PR, could you add some TODO comments so that we can address it later.
| @@ -79,11 +79,6 @@ func (s *akvCertProvider) GetCertificates(ctx context.Context, attrib map[string | |||
| return nil, nil, re.ErrorCodeConfigInvalid.NewError(re.CertProvider, providerName, re.AKVLink, nil, "clientID is not set", re.HideStackTrace) | |||
| } | |||
|
|
|||
| azureCloudEnv, err := parseAzureEnvironment(cloudName) | |||
There was a problem hiding this comment.
do we not need cloud env information now? Is this because the new SDK can infer cloud type based on the URI?
There was a problem hiding this comment.
I couldn't find any usecases for the couldName variable, other than extracting azureenv which is removed here because the new SDK only needs the vault's URI when creating a client. So I removed it.
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to get secret objectName:%s, objectVersion:%s, error: %w", keyVaultCert.CertificateName, keyVaultCert.CertificateVersion, err) | ||
| } | ||
| secretBundle := secretResponse.SecretBundle |
There was a problem hiding this comment.
nil check required for secretReponse?
There was a problem hiding this comment.
Go newbie question:
Is this the right way to compare the secretResponse struct with nil:
if reflect.DeepEqual(secretResponse, azsecrets.GetSecretResponse{}) {
return nil, nil, fmt.Errorf("failed to get secret objectName:%s, objectVersion:%s, secret response is nil", keyVaultCert.Name, keyVaultCert.Version)
}
There was a problem hiding this comment.
is if secretResponse == nil working in this case?
There was a problem hiding this comment.
@binbin-li doesn't seem like it:
invalid operation: secretResponse == nil (mismatched types azsecrets.GetSecretResponse and untyped nil)compiler[MismatchedTypes](https://pkg.go.dev/golang.org/x/tools/internal/typesinternal#MismatchedTypes)
| webKey.Kty = kv.EC | ||
| case kv.RSAHSM: | ||
| webKey.Kty = kv.RSA | ||
| case azkeys.JSONWebKeyTypeECHSM: |
There was a problem hiding this comment.
have we verified the new values for each case exact the existing values? Just want to double check
There was a problem hiding this comment.
Here is how the new consts are defined in azkeys:
const (
JSONWebKeyTypeEC JSONWebKeyType = "EC"
JSONWebKeyTypeECHSM JSONWebKeyType = "EC-HSM"
JSONWebKeyTypeOct JSONWebKeyType = "oct"
JSONWebKeyTypeOctHSM JSONWebKeyType = "oct-HSM"
JSONWebKeyTypeRSA JSONWebKeyType = "RSA"
JSONWebKeyTypeRSAHSM JSONWebKeyType = "RSA-HSM"
)
And here is how they were defined in keyvault (kv):
const (
EC = "EC"
ECHSM = "EC-HSM"
Oct = "oct"
RSA = "RSA"
RSAHSM = "RSA-HSM"
)
I replicated the same two conditions ( both in the condition and the body of the if statements). Is that whatever you are concerned about or something else?
shahramk64
force-pushed
the
skalantari/Refactor-Azure-authentication-support-to-use-azidentity
branch
4 times, most recently
from
af4254c
to
8666a69
Compare
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
shahramk64
force-pushed
the
skalantari/Refactor-Azure-authentication-support-to-use-azidentity
branch
from
8666a69
to
cbabb9c
Compare
|
@duffney Can you please have a look at this PR and see if the merge makes sense to you? |
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Signed-off-by: Shahram Kalantari <[email protected]>
Description
What this PR does / why we need it:
Azure authentication currently uses go-autorest SDK. It's now deprecated and using MSAL and azidentity SDKs are now recommended. This PR replaces the deprecated SDK with azidentity.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #1630
Type of change
Please delete options that are not relevant.
mainbranch)How Has This Been Tested?
This tutorial was followed to test authenticating AKS with AKV and retrieval of certificates.
1- KeyManagementProvider scenario: After deploying the following kmp resource using
kubectl apply -f kmp_config.yaml, the Ratify pod log shows successful retrieval of the certificate.kmp_config.yaml:
Ratify logs:
It was also verified by the following command:
kubectl describe KeyManagementProvider keymanagementprovider-akv2- CertificateStore scenario: After deploying the following certificate store resource using
kubectl apply -f cs_config.yaml, the Ratify pod log shows successful retrieval of the certificate.cs_config.yaml:
Ratify logs:
It was also verified by the following command:
kubectl describe CertificateStore certstore-akvChecklist:
Post Merge Requirements
Helm Chart Change