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

Update guide_azure.rst #54401

Merged
merged 3 commits into from
Apr 5, 2019
Merged

Update guide_azure.rst #54401

merged 3 commits into from
Apr 5, 2019

Conversation

ubruns
Copy link
Contributor

@ubruns ubruns commented Mar 26, 2019

+label: docsite_pr

SUMMARY

Added a note to the 'Storing in a file' section, that the secret value of the ini file should be UrlEncoded.
Otherwise the user might get login errors (depends on the characters used in the secret).

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

guide_azure.rst

ADDITIONAL INFORMATION

If the secret of the azure service principal contains unusual characters, the login will fail.
The secret must be urlencoded() to avoid this login error.

See also this discussion, wich got me to the solution:
https://stackoverflow.com/questions/42477266/aadsts50012-invalid-client-secret-is-provided-when-moving-from-a-test-app-to-pr


Added a note to the 'Storing in a file' section, that the secret value of the ini file should be UrlEncoded.
Otherwise the user might get login errors (depends on the characters used in the secret).

+label: docsite_pr
@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 26, 2019
@Akasurde
Copy link
Member

cc @yuwzho @zikalino

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 27, 2019
@yuwzho
Copy link
Contributor

yuwzho commented Mar 27, 2019

+label azure

Hi @ubruns Thanks for your reporting. My thinking is that user no need to do the url encode for no matter username or secret, if so, it should be a code bug. @lmazuel Would you give some information about this from SDK side?

@ansibot ansibot added the azure label Mar 27, 2019
@acozine
Copy link
Contributor

acozine commented Mar 27, 2019

@yuwzho would it be correct to say If your secrets contain non-ASCII characters, you must URL Encode them to avoid login errors.?

@yuwzho
Copy link
Contributor

yuwzho commented Mar 28, 2019

@acozine Actually I am thinking about handling this in code level. It doesn't make sense from user's perspective to do the encoding in no matter .ini file or environment variable, right?

@acozine
Copy link
Contributor

acozine commented Mar 28, 2019

@yuwzho handling this in the code seems like a smart approach - then users don't need to worry about encoding. What's the timeframe on changing the code? It might make sense to merge this docs PR now if the code fix is going to take days or weeks.

@ansibot
Copy link
Contributor

ansibot commented Mar 28, 2019

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/scenario_guides/guide_azure.rst:112:0: warning: Unknown target name: "url encode<https://www.w3schools.com/tags/ref_urlencode.asp>".

The test ansible-test sanity --test rstcheck [explain] failed with 1 error:

docs/docsite/rst/scenario_guides/guide_azure.rst:112:0: Unknown target name: "url encode<https://www.w3schools.com/tags/ref_urlencode.asp>".

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 28, 2019
@yuwzho
Copy link
Contributor

yuwzho commented Mar 29, 2019

I am a bit busy these time, would you like to propose a change? The code snippet should be at these lines around @acozine

self.azure_credentials = ServicePrincipalCredentials(client_id=self.credentials['client_id'],
secret=self.credentials['secret'],
tenant=self.credentials['tenant'],
cloud_environment=self._cloud_environment,
verify=self._cert_validation_mode == 'validate')

@dagwieers dagwieers added the docsite_pr This PR is created from documentation using the "Edit on GitHub" link. label Apr 2, 2019
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. small_patch labels Apr 5, 2019
@acozine
Copy link
Contributor

acozine commented Apr 5, 2019

@yuwzho I'm a docs person, not a developer, so I'll leave the code changes for someone else to do. Once this PR passes Shippable, I'm going to merge it, then open an issue for the code change.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 5, 2019
@acozine
Copy link
Contributor

acozine commented Apr 5, 2019

Thanks @ubruns for the docs fix.

I've created issue #54914 for the code changes @yuwzho.

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 azure core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite_pr This PR is created from documentation using the "Edit on GitHub" link. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants