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

salt/utils/cloud: Allow to customize ssh gateway command/options #48062

Merged

Conversation

@icy
Copy link
Contributor

@icy icy commented Jun 12, 2018

What does this PR do?

By default, nc command (nc -q0 %h %p) is used to create gateway. This is not
always possible in some restricted environments. An alternative is to use
native support from ssh: -W %h:%p.

This commit allows user to provide a customize option for gateway. Please note
the (wait_for_port) process may need another patch.

What issues does this PR fix or reference?

Previous Behavior

Users are forced to use nc -q0 %h %p command to create ssh proxy for salt-cloud

New Behavior

Users can use a customized command to create ssh proxy for salt-cloud

Tests written?

NO

Commits signed with GPG?

Yes

By default, nc command (nc -q0 %h %p) is used to create gateway. This is not
always possible in some restricted environments. An alternative is to use
native support from ssh: -W %h:%p.

This commit allows user to provide a customize option for gateway. Please note
the (wait_for_port) process may need another patch.
@ghost ghost self-requested a review Jun 12, 2018
Copy link
Contributor

@gtmanfred gtmanfred left a comment

This should be documented in salt/cloud/clouds/ec2.py, as well as adding the gateway stuff to ./doc/topics/cloud/misc.rst

doc/man/salt.7 Outdated
# Default to nc -q0 %h %p
# Optional
ssh_gateway_command: "-W %h:%p"

Copy link
Contributor

@gtmanfred gtmanfred Jun 12, 2018

Can you remove this change, we compile these every time we build, so this will not remain.

Copy link
Contributor Author

@icy icy Jun 12, 2018

thanks a lot. I will update ec2.py and misc.rst instead :)

@icy
Copy link
Contributor Author

@icy icy commented Jun 13, 2018

Thanks @gtmanfred and @cachedout for your reviews and approvals.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Jun 15, 2018

Thanks @icy and congrats on your first salt PR. :)

@rallytime rallytime merged commit bf1b2e7 into saltstack:develop Jun 15, 2018
6 of 10 checks passed
rallytime added a commit that referenced this issue Jun 19, 2018
Minor improvements and fixes for related PR: #48062
rklaren
Copy link
Contributor

rklaren commented on 10c61be Aug 5, 2018

@icy shouldn't this log.info line be inside the if? It crashes if no gateway is defined....

icy
Copy link
Contributor

icy commented on 10c61be Aug 6, 2018

Yes I agree :) All my manual tests were done with gateway settings. I will fix that. Thanks a lot for the catch.

rklaren
Copy link
Contributor

rklaren commented on 10c61be Aug 6, 2018

You're welcome :) Good addition to make the gateway command configurable!

@icy
Copy link
Contributor Author

@icy icy commented Aug 6, 2018

Hi @rallytime and @gtmanfred,

Is there any good start to write and improve unit tests (e.g., in my PRs)?

At my laptop, the tests are passed as seen here https://gist.github.com/icy/a59f975ba54224b7d941b2e38ccc74a1 , I don't think these tests are working fine. I did some smoke tests actually and it's not a recommended way (expensive?)

icy added a commit to icyfork/salt that referenced this issue Aug 6, 2018
Some variables are not in their right scope and makes salt crashed
when user settings do not include some gateway settings.

Cf: saltstack#48062
icy added a commit to icyfork/salt that referenced this issue Aug 6, 2018
Some variables are not in their right scope; that makes salt crashed
when user settings do not include some gateway settings.

Cf: saltstack#48062
rallytime added a commit to rallytime/salt that referenced this issue Aug 23, 2018
Some variables are not in their right scope; that makes salt crashed
when user settings do not include some gateway settings.

Cf: saltstack#48062
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants