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.

Sign up for GitHub

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

CAE support #966

Closed
wants to merge 10 commits into from
Closed

CAE support #966

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3f72e81
cae support
msyyc Jun 22, 2021
82c9dbc
pylint
msyyc Jun 29, 2021
a6fa001
pylint
msyyc Jun 29, 2021
71038ec
Merge branch 'autorestv3' into cae-support-2021-06-22
msyyc Jul 2, 2021
17b7d50
update dependency version
msyyc Jul 7, 2021
21904a0
review
msyyc Jul 7, 2021
30f917b
fix test
msyyc Jul 7, 2021
f7e7407
fix test
msyyc Jul 7, 2021
8bbbf97
fix test
msyyc Jul 7, 2021
5d2821e
set different default policy between data-plan and mgmt-plan
msyyc Jul 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions autorest/codegen/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

def _get_credential_default_policy_type_has_async_version(credential_default_policy_type: str) -> bool:
mapping = {
"BearerTokenCredentialPolicy": True,
"ARMChallengeAuthenticationPolicy": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. If users explicitly pass in --credential-default-policy-type=BearerTokenCredentialPolicy, it returns BearerTokenCredentialPolicy
  2. If users are generating mgmt plane (i.e. code_model.options['azure_arm'] == True, return ARMChallengeAuthenticationPolicy.
  3. Else, return BearerTokenCredentialPolicy

Copy link
Member Author

@msyyc msyyc Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

"AzureKeyCredentialPolicy": False
}
return mapping[credential_default_policy_type]
Expand Down Expand Up @@ -142,15 +142,15 @@ 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 == "ARMChallengeAuthenticationPolicy":
if not credential_scopes:
if azure_arm:
credential_scopes = ["https://management.azure.com/.default"]
elif credential:
# If add-credential is specified, we still want to add a credential_scopes variable.
# Will make it an empty list so we can differentiate between this case and None
_LOGGER.warning(
"You have default credential policy BearerTokenCredentialPolicy"
"You have default credential policy ARMChallengeAuthenticationPolicy"
"but not the --credential-scopes flag set while generating non-management plane code. "
"This is not recommend because it forces the customer to pass credential scopes "
"through kwargs if they want to authenticate."
Expand All @@ -159,8 +159,8 @@ def _get_credential_param(self, azure_arm, credential, credential_default_policy
if credential_key_header_name:
raise ValueError(
"You have passed in a credential key header name with default credential policy type "
"BearerTokenCredentialPolicy. This is not allowed, since credential key header name is tied with "
"AzureKeyCredentialPolicy. Instead, with this policy it is recommend you pass in "
"ARMChallengeAuthenticationPolicy. This is not allowed, since credential key header name is tied "
"with AzureKeyCredentialPolicy. Instead, with this policy it is recommend you pass in "
"--credential-scopes."
)
else:
Expand All @@ -169,7 +169,7 @@ def _get_credential_param(self, azure_arm, credential, credential_default_policy
raise ValueError(
"You have passed in credential scopes with default credential policy type "
"AzureKeyCredentialPolicy. This is not allowed, since credential scopes is tied with "
"BearerTokenCredentialPolicy. Instead, with this policy you must pass in "
"ARMChallengeAuthenticationPolicy. Instead, with this policy you must pass in "
"--credential-key-header-name."
)
if not credential_key_header_name:
Expand All @@ -182,19 +182,19 @@ def _get_credential_param(self, azure_arm, credential, credential_default_policy
def _handle_default_authentication_policy(self, azure_arm, credential):

passed_in_credential_default_policy_type = (
self._autorestapi.get_value("credential-default-policy-type") or "BearerTokenCredentialPolicy"
self._autorestapi.get_value("credential-default-policy-type") or "ARMChallengeAuthenticationPolicy"
)

# right now, we only allow BearerTokenCredentialPolicy and AzureKeyCredentialPolicy
allowed_policies = ["BearerTokenCredentialPolicy", "AzureKeyCredentialPolicy"]
# right now, we only allow ARMChallengeAuthenticationPolicy and AzureKeyCredentialPolicy
allowed_policies = ["ARMChallengeAuthenticationPolicy", "AzureKeyCredentialPolicy"]
try:
credential_default_policy_type = [
cp for cp in allowed_policies if cp.lower() == passed_in_credential_default_policy_type.lower()
][0]
except IndexError:
raise ValueError(
"The credential you pass in with --credential-default-policy-type must be either "
"BearerTokenCredentialPolicy or AzureKeyCredentialPolicy"
"ARMChallengeAuthenticationPolicy or AzureKeyCredentialPolicy"
)

credential_scopes, credential_key_header_name = self._get_credential_param(
Expand Down
2 changes: 1 addition & 1 deletion autorest/codegen/models/code_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def add_credential_global_parameter(self) -> None:
:rtype: None
"""
credential_schema: Union[AzureKeyCredentialSchema, TokenCredentialSchema]
if self.options["credential_default_policy_type"] == "BearerTokenCredentialPolicy":
if self.options["credential_default_policy_type"] == "ARMChallengeAuthenticationPolicy":
credential_schema = TokenCredentialSchema(async_mode=False)
else:
credential_schema = AzureKeyCredentialSchema()
Expand Down
9 changes: 6 additions & 3 deletions autorest/codegen/serializers/general_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. setup.py.jinjia2 has been updated. And I will update the this doc after new autorest.python is published.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, it's also possible to update in this PR if you want to keep everything in one PR

policy = "ARMHttpLoggingPolicy, ARMChallengeAuthenticationPolicy"
file_import.add_from_import("azure.mgmt.core.policies", async_policy if async_mode else policy,
ImportType.AZURECORE)
return file_import


Expand Down Expand Up @@ -53,7 +56,7 @@ def _service_client_imports() -> FileImport:

if (
self.code_model.options['credential'] and
self.code_model.options['credential_default_policy_type'] == "BearerTokenCredentialPolicy"
self.code_model.options['credential_default_policy_type'] == "ARMChallengeAuthenticationPolicy"
):
self._correct_credential_parameter()

Expand All @@ -75,7 +78,7 @@ def serialize_config_file(self) -> str:

if (
self.code_model.options['credential'] and
self.code_model.options['credential_default_policy_type'] == "BearerTokenCredentialPolicy"
self.code_model.options['credential_default_policy_type'] == "ARMChallengeAuthenticationPolicy"
):
self._correct_credential_parameter()

Expand Down
2 changes: 1 addition & 1 deletion autorest/codegen/serializers/metadata_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def _is_paging(operation):
async_global_parameters = self.code_model.global_parameters
if (
self.code_model.options['credential'] and
self.code_model.options['credential_default_policy_type'] == "BearerTokenCredentialPolicy"
self.code_model.options['credential_default_policy_type'] == "ARMChallengeAuthenticationPolicy"
):
# this ensures that the TokenCredentialSchema showing up in the list of code model's global parameters
# is sync. This way we only have to make a copy for an async_credential
Expand Down
4 changes: 2 additions & 2 deletions autorest/codegen/templates/config.py.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,6 @@ class {{ code_model.class_name }}Configuration(Configuration):
{% endif %}
if self.credential and not self.authentication_policy:
{% set credential_default_policy_type = ("Async" if (async_mode and code_model.options['credential_default_policy_type_has_async_version']) else "") + code_model.options['credential_default_policy_type'] %}
{% set credential_param_type = ("'" + code_model.options['credential_key_header_name'] + "', ") if code_model.options['credential_key_header_name'] else ("*self.credential_scopes, " if "BearerTokenCredentialPolicy" in credential_default_policy_type else "") %}
self.authentication_policy = policies.{{ credential_default_policy_type }}(self.credential, {{ credential_param_type if credential_param_type }}**kwargs)
{% set credential_param_type = ("'" + code_model.options['credential_key_header_name'] + "', ") if code_model.options['credential_key_header_name'] else ("*self.credential_scopes, " if "ARMChallengeAuthenticationPolicy" in credential_default_policy_type else "") %}
self.authentication_policy = {{ credential_default_policy_type }}(self.credential, {{ credential_param_type if credential_param_type }}**kwargs)
{% endif %}
4 changes: 2 additions & 2 deletions autorest/multiapi/templates/multiapi_config.py.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,6 @@ class {{ code_model.service_client.name }}Configuration(Configuration):
{% endif %}
if self.credential and not self.authentication_policy:
{% set credential_default_policy_type = ("Async" if (async_mode and code_model.config.credential_default_policy_type_has_async_version) else "") + code_model.config.credential_default_policy_type %}
{% set credential_param_type = ("'" + code_model.config.credential_key_header_name + "', ") if code_model.config.credential_key_header_name else ("*self.credential_scopes, " if "BearerTokenCredentialPolicy" in credential_default_policy_type else "") %}
self.authentication_policy = policies.{{ credential_default_policy_type }}(self.credential, {{ credential_param_type if credential_param_type }}**kwargs)
{% set credential_param_type = ("'" + code_model.config.credential_key_header_name + "', ") if code_model.config.credential_key_header_name else ("*self.credential_scopes, " if "ARMChallengeAuthenticationPolicy" in credential_default_policy_type else "") %}
self.authentication_policy = {{ credential_default_policy_type }}(self.credential, {{ credential_param_type if credential_param_type }}**kwargs)
{% endif %}