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.

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

Ensure trailing slash on base_url #984

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

ChandlerSwift
Copy link
Contributor

SUMMARY

Ensure trailing slash on base_url

The AZURE_CHINA_CLOUD.endpoints.resource_manager and
AZURE_GERMAN_CLOUD.endpoints.resource_manager URLs don't end with a
trailing slash, but the AZURE_PUBLIC_CLOUD and AZURE_US_GOV_CLOUD URLs
do. This commit ensures consistency despite that.

https://github.com/Azure/msrestazure-for-python/blob/8849f398b6ebd4607de63c2f5d1318f44ec1d822/msrestazure/azure_cloud.py#L137

Without this, base_url is set to 'https://management.chinacloudapi.cn':

base_url = self.azure_auth._cloud_environment.endpoints.resource_manager

And then credential_scopes is set to [base_url + ".default"]:

subscription_id=mgmt_subscription_id, base_url=base_url, credential_scopes=[base_url + ".default"])

Which results in an invalid credential_scopes of
["https://management.chinacloudapi.cn.default"] rather than
["https://management.chinacloudapi.cn/.default"].

Fix also submitted upstream:
Azure/msrestazure-for-python#169

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

azure_rm_common

# https://github.com/Azure/msrestazure-for-python/pull/169
# China's base_url doesn't end in a trailing slash, though others do,
# and we need a trailing slash when generating credential_scopes below.
if base_url[-1] != "/":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the string end with a '/'? Isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

kinlly ping!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right -- if the string doesn't end with a slash, then we add one. I'm realizing, now that I look back at this, that python has str.endswith(). I can rewrite this commit to use that instead, if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, Thanks for your reply!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right -- if the string doesn't end with a slash, then we add one. I'm realizing, now that I look back at this, that python has str.endswith(). I can rewrite this commit to use that instead, if you prefer.

This comments, Please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks for your patience; I wanted to make sure I'd tested everything, to not accidentally re-introduce some regression.

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Sep 26, 2022
@Fred-sun
Copy link
Collaborator

@ChandlerSwift Any update for this ?

@ChandlerSwift
Copy link
Contributor Author

@ChandlerSwift Any update for this ?

I'm sorry -- I didn't realize this was waiting on me. Please let me know what I can do to get this ready to merge!

The `AZURE_CHINA_CLOUD.endpoints.resource_manager` and
`AZURE_GERMAN_CLOUD.endpoints.resource_manager` URLs don't end with a
trailing slash, but the `AZURE_PUBLIC_CLOUD` and `AZURE_US_GOV_CLOUD` URLs
do. This commit ensures consistency despite that.

https://github.com/Azure/msrestazure-for-python/blob/8849f398b6ebd4607de63c2f5d1318f44ec1d822/msrestazure/azure_cloud.py#L137

Without this, `base_url` is set to `'https://management.chinacloudapi.cn'`:

https://github.com/ansible-collections/azure/blob/256ed011ea3d2e15616529034f53078dcefd9d2d/plugins/module_utils/azure_rm_common.py#L877

And then `credential_scopes` is set to `[base_url + ".default"]`:

https://github.com/ansible-collections/azure/blob/256ed011ea3d2e15616529034f53078dcefd9d2d/plugins/module_utils/azure_rm_common.py#L892

Which results in an invalid `credential_scopes` of
`["https://management.chinacloudapi.cn.default"]` rather than
`["https://management.chinacloudapi.cn/.default"]`.

Fix also submitted upstream:
Azure/msrestazure-for-python#169
@Fred-sun Fred-sun added the ready_for_review The PR has been modified and can be reviewed and merged label Nov 25, 2022
@xuzhang3
Copy link
Collaborator

@ChandlerSwift LGTM

@xuzhang3 xuzhang3 merged commit 142e2dc into ansible-collections:dev Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority ready_for_review The PR has been modified and can be reviewed and merged work in In trying to solve, or in working with contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants