Make pipelining work with su/sudo+requiretty #13200

Merged
merged 1 commit into from Dec 1, 2015

Conversation

Projects
None yet
10 participants
@amenonsen
Contributor

amenonsen commented Nov 18, 2015

While writing http://toroid.org/ansible-ssh-pipelining it occurred to me that it's possible to make pipelining work with su and sudo+requiretty. Here's a working solution. I've been using this for several days now, and I can say that it's very pleasant to use this with pipelining and the recently-speeded-up v2.

Thanks to @mgedmin for coming up with the Python one-liner that I substituted for sed. (If the maintainers consider sed an acceptable dependency—as I do—the sed version in the commit message is shorter and easier to understand. Note that this is a change to the sh/ssh plugins, so it's not relevant to Windows.)

cc: @abadger

@mgedmin

View changes

lib/ansible/plugins/connection/ssh.py
@@ -253,6 +253,8 @@ def _send_initial_data(self, fh, in_data):
try:
fh.write(in_data)
+ if tty:
+ fh.write("__EOF__\n")

This comment has been minimized.

@mgedmin

mgedmin Nov 18, 2015

Contributor

What are the chances this string occurs inside in_data?

@mgedmin

mgedmin Nov 18, 2015

Contributor

What are the chances this string occurs inside in_data?

This comment has been minimized.

@amenonsen

amenonsen Nov 18, 2015

Contributor

AFAIK it's not valid Python, so I'd think zero? But if you have a better suggestion, I'm happy to change it.

@amenonsen

amenonsen Nov 18, 2015

Contributor

AFAIK it's not valid Python, so I'd think zero? But if you have a better suggestion, I'm happy to change it.

This comment has been minimized.

@amenonsen

amenonsen Nov 18, 2015

Contributor

I've pushed another commit to add a random suffix to reduce the probability of collisions. Thanks for the suggestion.

@amenonsen

amenonsen Nov 18, 2015

Contributor

I've pushed another commit to add a random suffix to reduce the probability of collisions. Thanks for the suggestion.

This comment has been minimized.

@mgedmin

mgedmin Nov 18, 2015

Contributor

I mentioned this on IRC, but for archival purposes I'll repeat myself here:

#!/usr/bin/python

... python code ...

SOME_SHELL_SCRIPT = '''
#!/bin/sh
...

cat << __EOF__
hello
world
__EOF__

... more shell code ...
'''

... more python code...

is valid Python code.

@mgedmin

mgedmin Nov 18, 2015

Contributor

I mentioned this on IRC, but for archival purposes I'll repeat myself here:

#!/usr/bin/python

... python code ...

SOME_SHELL_SCRIPT = '''
#!/bin/sh
...

cat << __EOF__
hello
world
__EOF__

... more shell code ...
'''

... more python code...

is valid Python code.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Nov 18, 2015

Contributor

@abadger asked me to confirm that the raw and script modules will continue to work even if the remote machine doesn't have Python installed. Those commands are constructed differently, so this change should not affect them at all. I tested both and they do continue to work fine.

Contributor

amenonsen commented Nov 18, 2015

@abadger asked me to confirm that the raw and script modules will continue to work even if the remote machine doesn't have Python installed. Those commands are constructed differently, so this change should not affect them at all. I tested both and they do continue to work fine.

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Nov 19, 2015

Member

Took a closer look at this, this morning --

  • it looks like it changes the signature of connection plugins so we'll have to modify the other ones too?
  • Does this implementation makes pipelining always happen? Does that break the keep remote files functionality?
  • I'm not sure if we ever pass things in on stdin in the current code to non-python commands but it seems like a possibility (for instance with custom non-python modules). If so i think the shell.build_command() method would need to get its python interpreter from the caller rather than taking it out of shebang. You can take a look at how the shell.checksum() code (and the method that calls it) does that.
Member

abadger commented Nov 19, 2015

Took a closer look at this, this morning --

  • it looks like it changes the signature of connection plugins so we'll have to modify the other ones too?
  • Does this implementation makes pipelining always happen? Does that break the keep remote files functionality?
  • I'm not sure if we ever pass things in on stdin in the current code to non-python commands but it seems like a possibility (for instance with custom non-python modules). If so i think the shell.build_command() method would need to get its python interpreter from the caller rather than taking it out of shebang. You can take a look at how the shell.checksum() code (and the method that calls it) does that.

@abadger abadger added this to the next milestone Nov 19, 2015

@abadger abadger self-assigned this Nov 19, 2015

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Nov 19, 2015

Contributor

Thanks for taking a look.

  • it looks like it changes the signature of connection plugins so
    we'll have to modify the other ones too?

It does not. _run and _send_initial_data are internal to ssh.py.

  • Does this implementation makes pipelining always happen? Does that
    break the keep remote files functionality?

It doesn't, and doesn't. It changes how pipelining works when we decide
to use it, which decision is taken in plugins/action/init.py based
on whether the transport supports it and so on. We change the default,
and we allow pipelining with su, but KEEP_REMOTE_FILES is tested and
taken into consideration separately.

See the diff to action/init.py for the full picture.

  • I'm not sure if we ever pass things in on stdin in the current code
    to non-python commands but it seems like a possibility (for instance
    with custom non-python modules).

The current code has no support for this, and I expect that non-Python
modules would break with pipelining turned on. (As I said earlier, raw
and script commands are built elsewhere, and are not affected by this
change at all.)

I agree that it would be nice to make this work; but the code that uses
shebang was already there, I just copied it verbatim to make sure that I
maintained compatibility above all, to give this a chance to be accepted
for 2.0 as a significant performance benefit.

But if you want me to clean it up further, I will.

(P.S. If this already has no chance of being accepted for 2.0, I would
really appreciate it if you said so, so that I can adjust my priorities
accordingly. Thanks.)

-- Abhijit

Contributor

amenonsen commented Nov 19, 2015

Thanks for taking a look.

  • it looks like it changes the signature of connection plugins so
    we'll have to modify the other ones too?

It does not. _run and _send_initial_data are internal to ssh.py.

  • Does this implementation makes pipelining always happen? Does that
    break the keep remote files functionality?

It doesn't, and doesn't. It changes how pipelining works when we decide
to use it, which decision is taken in plugins/action/init.py based
on whether the transport supports it and so on. We change the default,
and we allow pipelining with su, but KEEP_REMOTE_FILES is tested and
taken into consideration separately.

See the diff to action/init.py for the full picture.

  • I'm not sure if we ever pass things in on stdin in the current code
    to non-python commands but it seems like a possibility (for instance
    with custom non-python modules).

The current code has no support for this, and I expect that non-Python
modules would break with pipelining turned on. (As I said earlier, raw
and script commands are built elsewhere, and are not affected by this
change at all.)

I agree that it would be nice to make this work; but the code that uses
shebang was already there, I just copied it verbatim to make sure that I
maintained compatibility above all, to give this a chance to be accepted
for 2.0 as a significant performance benefit.

But if you want me to clean it up further, I will.

(P.S. If this already has no chance of being accepted for 2.0, I would
really appreciate it if you said so, so that I can adjust my priorities
accordingly. Thanks.)

-- Abhijit

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Nov 19, 2015

Contributor

(Sorry, I take that back about non-Python modules breaking with pipelining enabled. I can't say whether they would or not without trying it out, and I'm not in a position to do so easily.)

Contributor

amenonsen commented Nov 19, 2015

(Sorry, I take that back about non-Python modules breaking with pipelining enabled. I can't say whether they would or not without trying it out, and I'm not in a position to do so easily.)

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Nov 19, 2015

Contributor

I've rebased and added a commit to address @abadger's comment about the python interpreter.

Contributor

amenonsen commented Nov 19, 2015

I've rebased and added a commit to address @abadger's comment about the python interpreter.

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Dec 1, 2015

Member

Reviewed and tested. Looks good. Want to squash this before I merge it to devel?

Member

abadger commented Dec 1, 2015

Reviewed and tested. Looks good. Want to squash this before I merge it to devel?

Make sudo+requiretty and ANSIBLE_PIPELINING work together
Pipelining is a *significant* performance benefit, because each task can
be completed with a single SSH connection (vs. one ssh connection at the
start to mkdir, plus one sftp and one ssh per task).

Pipelining is disabled by default in Ansible because it conflicts with
the use of sudo if 'Defaults requiretty' is set in /etc/sudoers (as it
is on Red Hat) and su (which always requires a tty).

We can (and already do) make sudo/su happy by using "ssh -t" to allocate
a tty, but then the python interpreter goes into interactive mode and is
unhappy with module source being written to its stdin, per the following
comment from connections/ssh.py:

        # we can only use tty when we are not pipelining the modules.
        # piping data into /usr/bin/python inside a tty automatically
        # invokes the python interactive-mode but the modules are not
        # compatible with the interactive-mode ("unexpected indent"
        # mainly because of empty lines)

Instead of the (current) drastic solution of turning off pipelining when
we use a tty, we can instead use a tty but suppress the behaviour of the
Python interpreter to switch to interactive mode. The easiest way to do
this is to make its stdin *not* be a tty, e.g. with cat|python.

This works, but there's a problem: ssh will ignore -t if its input isn't
really a tty. So we could open a pseudo-tty and use that as ssh's stdin,
but if we then write Python source into it, it's all echoed back to us
(because we're a tty). So we have to use -tt to force tty allocation; in
that case, however, ssh puts the tty into "raw" mode (~ICANON), so there
is no good way for the process on the other end to detect EOF on stdin.
So if we do:

    echo -e "print('hello world')\n"|ssh -tt someho.st "cat|python"

…it hangs forever, because cat keeps on reading input even after we've
closed our pipe into ssh's stdin. We can get around this by writing a
special __EOF__ marker after writing in_data, and doing this:

    echo -e "print('hello world')\n__EOF__\n"|ssh -tt someho.st "sed -ne '/__EOF__/q' -e p|python"

This works fine, but in fact I use a clever python one-liner by mgedmin
to achieve the same effect without depending on sed (at the expense of a
much longer command line, alas; Python really isn't one-liner-friendly).

We also enable pipelining by default as a consequence.
@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Dec 1, 2015

Contributor

Done.

Contributor

amenonsen commented Dec 1, 2015

Done.

abadger added a commit that referenced this pull request Dec 1, 2015

Merge pull request #13200 from amenonsen/pipelining
Make pipelining work with su/sudo+requiretty

@abadger abadger merged commit bebd2c5 into ansible:devel Dec 1, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Dec 1, 2015

Member

Thanks @amenonsen !

Member

abadger commented Dec 1, 2015

Thanks @amenonsen !

alanorth added a commit to alanorth/ansible-personal that referenced this pull request Aug 22, 2016

Add ansible.cfg
Pipelining makes ansible run tasks much faster, and as of Ansible
2.0 or 2.1 or so, it now works on older hosts that have requiretty
in their sudo config[0]. Also, disable the creation of those stupid
retry files.

[0] ansible/ansible#13200

alanorth added a commit to ilri/rmg-ansible-public that referenced this pull request Aug 22, 2016

Add ansible.cfg
Pipelining makes ansible run tasks much faster, and as of Ansible
2.0 or 2.1 or so, it now works on older hosts that have requiretty
in their sudo config[0]. Also, disable the creation of those stupid
retry files.

[0] ansible/ansible#13200

@alanorth alanorth referenced this pull request in ilri/rmg-ansible-public Aug 22, 2016

Merged

Add ansible.cfg #50

@orenwolf

This comment has been minimized.

Show comment
Hide comment
@orenwolf

orenwolf Nov 18, 2016

@abadger Anything we can do to get this re-merged or investigated? I understand it was pulled from the build, which has lead to no end of confusion on our part since it shows merged here. :)

@abadger Anything we can do to get this re-merged or investigated? I understand it was pulled from the build, which has lead to no end of confusion on our part since it shows merged here. :)

@sourcejedi

This comment has been minimized.

Show comment
Hide comment
@sourcejedi

sourcejedi Oct 27, 2017

"Instead the pipelining patch should modify the other connection plugins to handle the new EOF marker." #13409 (comment)

"Instead the pipelining patch should modify the other connection plugins to handle the new EOF marker." #13409 (comment)

@Dmitriusan

This comment has been minimized.

Show comment
Hide comment
@Dmitriusan

Dmitriusan Nov 3, 2017

@amenonsen / @sourcejedi / @abadger,

Looks like this PR is merged
But latest Ansible documentation still statest that I should disable requiretty

http://docs.ansible.com/ansible/latest/intro_configuration.html#pipelining

What is the current status? Is documentation outdated?

Dmitriusan commented Nov 3, 2017

@amenonsen / @sourcejedi / @abadger,

Looks like this PR is merged
But latest Ansible documentation still statest that I should disable requiretty

http://docs.ansible.com/ansible/latest/intro_configuration.html#pipelining

What is the current status? Is documentation outdated?

@tkeeler33

This comment has been minimized.

Show comment
Hide comment
@tkeeler33

tkeeler33 Dec 14, 2017

@Dmitriusan - It doesn't look like it's in the current 2.4.2 release.

Looking at the 2.5 develop branch it does appear to be supported using the use_tty option in ssh settings. I have not tried this with pipelining however.

@Dmitriusan - It doesn't look like it's in the current 2.4.2 release.

Looking at the 2.5 develop branch it does appear to be supported using the use_tty option in ssh settings. I have not tried this with pipelining however.

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

@x-yuri

This comment has been minimized.

Show comment
Hide comment
@x-yuri

x-yuri May 6, 2018

From the other issue:

note for people finding this from a web search: currently, the change introduced in PR #13200 has been reverted until we can find fixes for this and other issues. stemming from it.

I understand it's still reverted. But judging from this conversation, I take it it's safe to disable it, like:

    - name: sudo | disable requiretty (to enable pipelining + become)
      lineinfile:
        path: /etc/sudoers
        regexp: "^(\\s*Defaults\\s+requiretty\\s*)$"
        backrefs: yes
        line: '# \1'

x-yuri commented May 6, 2018

From the other issue:

note for people finding this from a web search: currently, the change introduced in PR #13200 has been reverted until we can find fixes for this and other issues. stemming from it.

I understand it's still reverted. But judging from this conversation, I take it it's safe to disable it, like:

    - name: sudo | disable requiretty (to enable pipelining + become)
      lineinfile:
        path: /etc/sudoers
        regexp: "^(\\s*Defaults\\s+requiretty\\s*)$"
        backrefs: yes
        line: '# \1'

samatjain added a commit to samatjain/dotfiles that referenced this pull request May 31, 2018

ansible: Enabling pipelining for SSH
Requires ansible 2.5 or later. Further info:

 - Background: http://toroid.org/ansible-ssh-pipelining
 - Feature PR: ansible/ansible#13200
@aioue

This comment has been minimized.

Show comment
Hide comment
@aioue

aioue Jun 14, 2018

Contributor

Equivalent of @x-yuri's workaround on the official AWS CentOS AMIs:

- name: allow sudo without tty
  become: yes
  lineinfile:
    create: yes
    dest: /etc/sudoers.d/99-centos-cloud-init-requiretty
    line: 'Defaults:centos !requiretty'
    validate: 'visudo -cf %s'
    owner: root
    group: root
    mode: 0440

After running the above in it's own play with:

vars:
  ansible_ssh_pipelining: False

All subsequent runs can be actioned normally.

Contributor

aioue commented Jun 14, 2018

Equivalent of @x-yuri's workaround on the official AWS CentOS AMIs:

- name: allow sudo without tty
  become: yes
  lineinfile:
    create: yes
    dest: /etc/sudoers.d/99-centos-cloud-init-requiretty
    line: 'Defaults:centos !requiretty'
    validate: 'visudo -cf %s'
    owner: root
    group: root
    mode: 0440

After running the above in it's own play with:

vars:
  ansible_ssh_pipelining: False

All subsequent runs can be actioned normally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment