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

Azure (ARM): Fix GovCloud Support #45187

Merged

Conversation

Projects
None yet
5 participants
@nicholasmhughes
Copy link
Contributor

commented Dec 27, 2017

What does this PR do?

This PR fixes the ability of the Azure ARM cloud module to communicate with Azure GovCloud endpoints. Some other issues with the cloud module were also fixed, along with general cleanup from linting, etc.

What issues does this PR fix or reference?

N/A

Previous Behavior

  • Cloud module was unable to hit endpoints for anything other than the Public Cloud.
  • Userdata was not able to be passed during instance creation... it silently failed.
  • Some of the functions didn't work from the command line (salt-cloud -f <function>)

New Behavior

  • Broke out some functionality into a utils module which can also be used by some execution and state modules which are currently being worked.
  • Cloud module can now hit endpoints for any Microsoft hosted endpoint or custom Azure Stack endpoint
  • Custom scripts can now be passed via VM extensions. Added some documentation to the main docstring outlining the uses.
  • Fixed functions to work from the command line in order to list various resources accessible to the cloud provider configuration.
  • Fixed some logic to ensure the cloud module works as expected.
  • Rolled in start/stop actions from PR 45119
  • Linted the modules to ensure prettyness...

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

nicholasmhughes added some commits Dec 27, 2017

@gtmanfred gtmanfred requested a review from techhat Dec 28, 2017

@gtmanfred
Copy link
Contributor

left a comment

Can you change the name of the option for the public key to ssh_key_file so that it matches what is used in our other cloud providers.

Also if you could add a note about it to the getting started doc for azurearm

https://docs.saltstack.com/en/latest/topics/cloud/azurearm.html

Thanks,
Daniel

nicholasmhughes added some commits Dec 28, 2017

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2017

Actually, sorry, i am wrong, you were right. it can be ssh_publickeyfile. ssh_key_file should be the private key that is used to match that public key and log into the server.

nicholasmhughes added some commits Dec 28, 2017

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

@gtmanfred I was too quick on the draw... Net effect is updated documentation. Is that correct now?

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2017

It looks like you just need to add using the ssh_key_file as the identity file for the ssh command.

it should be passed to the bootstrap() function as key_filename

Thanks,
Daniel

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

@gtmanfred bootstrapping works as expected using the ssh_keyfile parameter, which is pulled in via saltify.py. I updated the documentation to reflect this... is that acceptable?

https://github.com/saltstack/salt/blob/develop/salt/cloud/clouds/saltify.py#L383

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2017

Yup, that works for me.

Thanks,
Daniel

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2018

re-run lint

nicholasmhughes added some commits Jan 5, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

re-run docs

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

@techhat Can you review this?

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2018

@rallytime @techhat Anything I can do to help with testing/acceptance?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

@nicholasmhughes We're still waiting on a review from @techhat. But in the mean time, this PR has a merge conflict in it. Can you fix that and rebase this?

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

@rallytime I think the conflicts should be resolved. Please let me know if there are any other actions.

@drawsmcgraw

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

@rallytime @techhat anything more we can do to assist with the testing/merging?I can buy @techhat a coffee if it helps :)

@techhat
Copy link
Member

left a comment

This is a really great set of changes! Thanks @nicholasmhughes!

@@ -95,7 +97,7 @@ Set up an initial profile at ``/etc/salt/cloud.profiles``:
size: Standard_D1_v2
location: eastus
ssh_username: azureuser
ssh_password: verybadpass

This comment has been minimized.

Copy link
@techhat

techhat Jan 25, 2018

Member

ssh_password should not be removed from the example, since it is still supported. How about another example block for ssh_publickeyfile?

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@techhat is the documentation change in line with what you were thinking?

@techhat

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

@nicholasmhughes perfect, thanks!

@rallytime rallytime merged commit 6b49508 into saltstack:develop Jan 26, 2018

3 of 10 checks passed

jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #19352 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #15791 — ABORTED
Details
codeclimate 29 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #14139 — FAILURE
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #1764 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #6317 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #21770 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #18746 — SUCCESS
Details

@nicholasmhughes nicholasmhughes deleted the nicholasmhughes:add-azurearm-govcloud-support branch Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.