CAE support #966
CAE support #966
Conversation
| @@ -19,7 +19,7 @@ | |||
|
|
|||
| def _get_credential_default_policy_type_has_async_version(credential_default_policy_type: str) -> bool: | |||
| mapping = { | |||
| "BearerTokenCredentialPolicy": True, | |||
| "ARMChallengeAuthenticationPolicy": True, | |||
There was a problem hiding this comment.
hey @msyyc, since we're trying to not make breaking changes, this change would be breaking for anyone who explictly passes in BearerTokenCredentialPolicy.
I realize mgmt plane wants to switch completely over to ARMChallengeAuthenticationPolicy, and since we check the authentication policy also in the templates, one idea is to have a @property on the CodeModel, i.e. challenge_auth_policy
This getter would replace every single `ARMChallengeAuthenticationPolicy that you currently have, and should have the following behavior
- If users explicitly pass in
--credential-default-policy-type=BearerTokenCredentialPolicy, it returnsBearerTokenCredentialPolicy - If users are generating mgmt plane (i.e.
code_model.options['azure_arm'] == True, returnARMChallengeAuthenticationPolicy. - Else, return
BearerTokenCredentialPolicy
There was a problem hiding this comment.
Hi @iscai-msft Thank you for the detailed advice.
(1) I add ARMChallengeAuthenticationPolicy as one of allowed policy type instead of deleting BearerTokenCredentialPolicy with your advice .
(2) By comparing ARMChallengeAuthenticationPolicy with BearerTokenCredentialPolicy (https://github.com/Azure/azure-sdk-for-python/blob/16f66ce00af0435b05bd7b88e045ba8ff42736ed/sdk/core/azure-mgmt-core/azure/mgmt/core/policies/_authentication.py#L36), we could find that the only difference is the on_challenge and it will also return 'false' if service does not support CAE.
In one word, for users who don't input policy, ARMChallengeAuthenticationPolicy almost has same behavior with BearerTokenCredentialPolicy. So to simplify code, how about change the default policy directly?
There was a problem hiding this comment.
Yeah, but ARMChallengeAuthenticationPolicy is arm specific, and it's in azure-mgmt-core. So it makes sense to switch the default auth policy in the case of mgmt plane to ARMChallengeAuthenticationPolicy. However, for data-plane, we don't have users install azure-mgmt-core at all, so this is breaking for data-plane SDKs
There was a problem hiding this comment.
Got it. For mgmt-plan, the default policy could become ARMChallengeAuthenticationPolicy; for data-plan, the dafault policy keeps same with before. How about this commit : 5d2821e
| @@ -18,7 +18,10 @@ def config_imports(code_model, global_parameters: ParameterList, async_mode: boo | |||
| for gp in global_parameters: | |||
| file_import.merge(gp.imports()) | |||
| if code_model.options["azure_arm"]: | |||
| file_import.add_from_import("azure.mgmt.core.policies", "ARMHttpLoggingPolicy", ImportType.AZURECORE) | |||
| async_policy = "ARMHttpLoggingPolicy, AsyncARMChallengeAuthenticationPolicy" | |||
There was a problem hiding this comment.
One thing to note is that the base azure-mgmt-core version in the basic-setup-py needs to be updated to the azure-mgmt-core version that added ARMChallengeAuthenticationPolicy. Please update [here](https://github.com/Azure/autorest.python/blob/autorestv3/autorest/codegen/templates/setup.py.jinja2. At the same time, can you update the minimum azure-mgmt-core req in this doc?
There was a problem hiding this comment.
sure. setup.py.jinjia2 has been updated. And I will update the this doc after new autorest.python is published.
There was a problem hiding this comment.
sounds good, it's also possible to update in this PR if you want to keep everything in one PR
| @@ -142,15 +143,16 @@ def _get_credential_param(self, azure_arm, credential, credential_default_policy | |||
| credential_scopes = self._get_credential_scopes(credential) | |||
| credential_key_header_name = self._autorestapi.get_value('credential-key-header-name') | |||
|
|
|||
| if credential_default_policy_type == "BearerTokenCredentialPolicy": | |||
| if credential_default_policy_type in ["ARMChallengeAuthenticationPolicy", "BearerTokenCredentialPolicy"]: | |||
There was a problem hiding this comment.
Preferably, we only have "ARMChallengeAuthenticationPolicy" and "BearerTokenCredentialPolicy" in one place.
Implementing the property I talked about earlier will get you this behavior
There was a problem hiding this comment.
| if credential_default_policy_type in ["ARMChallengeAuthenticationPolicy", "BearerTokenCredentialPolicy"]: | |
| if (azure_arm, credential_default_policy_type ) in [(True, "ARMChallengeAuthenticationPolicy"), (False, "BearerTokenCredentialPolicy")]: |
Do you mean to distinguish data-plan and mgmt-plan like up?
rely on : Azure/azure-sdk-for-python#19365