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

Add Windows-compatible testing for interacting with CLI over TTY #49340

Open
williamrandolph opened this issue Nov 19, 2019 · 10 comments
Open

Add Windows-compatible testing for interacting with CLI over TTY #49340

williamrandolph opened this issue Nov 19, 2019 · 10 comments
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests

Comments

@williamrandolph
Copy link
Contributor

In #44775 , we added some tests that use the expect utility to provide a password to Elasticsearch using an interactive terminal. This was easy to install in Unix environments, but to do so in Windows would be more complex.

I want to look into using a Java-native pseudo-terminal (e.g., https://github.com/JetBrains/pty4j) before asking the infra team to update our windows images. If that doesn't pan out, I'll file a ticket with infra to work on providing expect on Windows at test time.

@williamrandolph williamrandolph added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure labels Nov 19, 2019
@williamrandolph williamrandolph self-assigned this Nov 19, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Build)

@williamrandolph
Copy link
Contributor Author

williamrandolph commented Jan 10, 2020

I've looked at pty4j and some of the libraries linked on its page. None of them seem like widely used projects, and I'm not sure if we want to take any of them on as a dependency.

I'd recommend that for the sake of simplicity, we move ahead with trying to get expect installed on Windows. I'm going to issue a ticket with infra to see if we can make this happen.

EDIT: Infra ticket is here: https://github.com/elastic/infra/issues/17124

@williamrandolph
Copy link
Contributor Author

The story on Expect for Windows is that there's a Windows port of Expect, but it's not FOSS and you have to get it as part of the ActiveState TCL distribution. Even then, it's not clear to me that it does exactly the same thing that expect on unix-like systems does, because the Windows architecture for command line apps is so different from the Unix model. In Windows, a program can't create and manage its own pseudo-tty channels; it can only inherit the parent process's channels or use new ones created by the OS.

Anyway, part of the concern here is that the Elasticsearch Java code might operate differently when connected to a TTY than when it's just hooked up to standard input. I wrote a little test class and used it to verify that both our Bash and Windows Batch startup scripts handle prompting for a password (based on whether or not the keystore is password-protected) and will always pipe standard input to the Elasticsearch process, even if standard input is only an empty string.

For example, in Bash, our invocation is:

  exec \
    "$JAVA" \
    "$XSHARE" \
    $ES_JAVA_OPTS \
    […various es flags…] \
    -cp "$ES_CLASSPATH" \
    org.elasticsearch.bootstrap.Elasticsearch \
    "$@" <<<"$KEYSTORE_PASSWORD"

That <<< is a "Here String", and passes the contents of the string to the process's standard input.

In Windows Batch, we always echo either a password or an empty string:

ECHO.!KEYSTORE_PASSWORD!| %JAVA% %ES_JAVA_OPTS% -Delasticsearch ^
  […various es flags…] ^
  -cp "%ES_CLASSPATH%" "org.elasticsearch.bootstrap.Elasticsearch" !newparams!

I used the following simple Java test class to verify that these shell scripts always prevent Elasticsearch from being attached to the console rather than to standard input:

import java.io.Console;

class ConsoleTest {
    public static void main(String[] args) {

        Console console = System.console();
        if (console == null) {
            System.out.println("No console");
        } else {
            System.out.println("There is a console");
        }
    }
}

In summary, if we were to test providing a password over a TTY, we would be testing our bash and batch scripts on the target systems, not our Java code.

There are other Elasticsearch tools, e.g. the KeyStoreCli, that don't have the same shell-script wrapper and might use either a ConsoleTerminal or a SystemTerminal object for output. But the main Elasticsearch application, when started with bin/elasticsearch or bin\elasticsearch.bat, will always use the SystemTerminal for I/O.

@rjernst
Copy link
Member

rjernst commented May 28, 2020

Thanks for continuing this investigation, William.

we would be testing our bash and batch scripts on the target systems

Isn't that exactly what we want to test? The expect tests that we want to also work on windows are specifically for testing starting elasticsearch, with a password, at a system level, right?

@williamrandolph
Copy link
Contributor Author

Yes, agreed. I'm hoping that my notes here will help me (or someone else) either verify that Expect for Windows in fact does what we need it to, if we can get it installed, or help us find the right tool for the job otherwise. As a next step, I'm going to check out the behavior of Expect for Windows.

@williamrandolph
Copy link
Contributor Author

It looks like Expect for Windows didn't make it into the latest ActiveTCL release, and I'm not sure how else to install expect. There is a Node package that uses modern Windows methods for pty behavior (https://github.com/Microsoft/node-pty) and an older C++ library that uses an off-screen window and a screen-scraper to create pty-like behavior for Cygwin and MSYS (https://github.com/rprichard/winpty).

The pty4j library for Java uses the winpty library under the hood, so I'm going to take another shot at using that and see what I learn.

@williamrandolph
Copy link
Contributor Author

I found an issue with the expect script I'm using on centos-6. We won't support centos-6 for master, but if I can replace the expect script with something that works on Windows, maybe this problem will go away: #58200

@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
@mark-vieira
Copy link
Contributor

@williamrandolph is this still an issue?

@williamrandolph
Copy link
Contributor Author

@mark-vieira Yes, this one is still on my back-burner and I'm hoping to take another pass at it the next time I'm working on Windows stuff.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

No branches or pull requests

5 participants