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 entrypoint routing, which supports routing string-based commands to different executables, with special logic for pwsh #398

Open
TravisEz13 opened this issue Mar 27, 2020 · 13 comments

Comments

@TravisEz13
Copy link
Member

TravisEz13 commented Mar 27, 2020

Goal

Add entrypoint routing, which supports routing string-based commands to different executables, with special logic for pwsh. In order to:

  • Provide actionable error messages. Users should know that the entrypoint syntax they used failed in PowerShell, in Bash (or other shell), or in the script itself.
  • Support on Linux and Windows (a number of these appear like general concerns, not Linux-specific).
  • This avoids needing to
    • use the --entrypoint syntax to specify an executable, and
    • split the entrypoint (an argument to docker run) from its arguments (arguments to the container; they show up last).

Background

The best practice in docker is for an image to run the product in contains.
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#entrypoint

Currently our images only run PowerShell when no command is specified. This is specified in the CMD entry in the Dockerfile.

Unfortunately, making the change causes a breaking change to the image in what is hopefully a corner case.

Customer requests for this are in #274 and #394

Proof of concept is here: #399

Other Language images

  • Work similar to the proposal
    • php
    • node
  • Like our current image
    • perl (perldebug)
    • python
  • Dumps you in bash
    • golang

Proposal

  1. Add a shell script that will detect if you are running pwsh (or similar), if so, send it to bash, otherwise run pwsh -nologo -l -c "$@"
  2. set that script as the ENTRYPOINT
  3. SetCMD to [ "pwsh" ]
  • Maintain the most features
  • Achieves the goals of the best practices

New Scenario

Note the lack of pwsh -c.

docker  run --rm  mcr.microsoft.com/powershell:7.1.0-preview.1-ubuntu-18.04 '$psversiontable.PSVersion.ToString()'

Will continue to work

docker  run --rm  mcr.microsoft.com/powershell:7.1.0-preview.1-ubuntu-18.04 pwsh -c '$psversiontable.PSVersion.ToString()'

Breaking change

Scenario - Running PowerShell using container

Working command with current images

docker  run --rm  mcr.microsoft.com/powershell:7.1.0-preview.1-ubuntu-18.04 /opt/microsoft/powershell/7-preview/pwsh -c '$psversiontable.PSVersion.ToString()'

Scenario - Expected result (with current image)

7.1.0-preview.1

Actual result (with private build with change)

ParserError: 
Line |
   1 |  7.1.0-preview.1
     |       ~~~~~~~~
     | Unexpected token '-preview' in expression or statement.

Workaround

Use --% to prevent the new PowerShell entrypoint from parsing the command.

docker  run --rm  mcr.microsoft.com/powershell:7.1.0-preview.1-ubuntu-18.04 /opt/microsoft/powershell/7-preview/pwsh --% -c '$psversiontable.PSVersion.ToString()'

Entry point script - Ubuntu

#!/bin/sh
set -e

# first arg is `pwsh` or similar
if ! { [ $1 = 'pwsh' ] || [ $1 = 'pwsh-preview' ]; }; then
 set -- pwsh -nologo -l -c "$@"
fi

exec "$@"
@RDIL
Copy link
Collaborator

RDIL commented Mar 27, 2020

I feel like this would be ideal for the refactor we were discussing a month or so ago, and I feel like in between these releases is the perfect time to get that in motion.

@TravisEz13
Copy link
Member Author

TravisEz13 commented Apr 1, 2020

I found this, which would break, but it is our code and no longer used.
https://github.com/O330oei/vlea/blob/b7295deaf506d55c590491132dc6cb16e3d99355/test/nanoserver/nanoserver.tests.ps1

GitHub
Contribute to O330oei/vlea development by creating an account on GitHub.

@TravisEz13
Copy link
Member Author

add support for
/bin/bash
/bin/sh

@TravisEz13
Copy link
Member Author

1/3 of the docker files define entry point and definitely won't be affected.

Most executions of the image where either pwsh -c or interactive.

@MichaelSimons
Copy link

MichaelSimons commented Apr 2, 2020

When I first saw this issue I thought it was pretty cut and dried but the more I think about it, that definitely does not seem to be the case 😸.

FWIW, I have a couple questions/comments

1. If I understand the proposal correctly, both a CMD and an ENTRYPOINT would be defined. If you docker run the resulting image you would be launching pwsh twice in a nested format as docker run will pass the exec form CMD as parameters to the ENTRYPOINT. Is the CMD defined just so that the parameterless docker run scenario works e.g. docker run -it --rm test? Edit: I realize this is not an issue with the proposal because the CMD is passed into the script which results in the CMD being executed.

  1. The entrypoint script feels strange to me. The differing behavior based on the docker run args is what feels strange. Is being done only for compatibility reasons? Do you not want to make this breaking change during a major/minor release? It feels like in the long term, you would want the entry point to be simply be pwsh -nologo -l. Have you measured the performance impact to startup?

Maybe it would be helpful to capture the usage scenarios and the expected behavior in this issue.

@joeyaiello
Copy link

joeyaiello commented Apr 8, 2020

So I spoke to @TravisEz13 offline and spent a bunch more time with Docker entrypoint scripts in some home projects, and think I can accurately describe the behavior now.

  1. docker run -it mcr.microsoft.com/powershell:foo pwsh
    For this one, the entrypoint script is checking to see if it's getting pwsh or pwsh-preview so that it doesn't run pwsh inside of pwsh.

  2. docker run -it mcr.microsoft.com/powershell:foo 'Do-Stuff -Bar'
    In this case, the entrypoint script doesn't see pwsh or pwsh-preview, so it passes the script block to the default pwsh in the container. This is the scenario that's being lit up by this PR.

  3. docker run -it mcr.microsoft.com/powershell:foo bash
    In this case, the user expects to be dropped into bash, as the default behavior of Docker is to look for bash on the PATH of powershell:foo and execute it. However, because the entrypoint script isn't detecting pwsh or pwsh-preview, bash is actually being executed from within PowerShell as a script block. The implication being that startup time for non-pwsh binaries could be slower, but I'm okay with that tradeoff given that the point of the powershell container is to run PowerShell.

  4. docker run -it mcr.microsoft.com/powershell:foo 'bash -c 1'
    Today, this doesn't work. With the new entrypoint script, this should now work because bash -c 1 is being passed as a script block to PowerShell.

  5. docker run -it mcr.microsoft.com/powershell:foo '/bar.ps1'
    In this example, running the script bar.ps1 on the root of the filesystem requires an absolute path or a ./ because it's being executed as a script block.

  6. docker run -it mcr.microsoft.com/powershell:foo bar.ps1
    This only works today if bar.ps1 has a properly set shebang, a +x attribute, and is on the PATH. I believe that should still hold true today (though I believe it also means that shebang is no longer required as PowerShell trusts just the .ps1 file extension).

  7. docker run -it mcr.microsoft.com/powershell:foo '/usr/bin/pwsh'
    This will actually run a PowerShell inside PowerShell because the full path is not parsed as pwsh or pwsh-preview by the entrypoint script.

@joeyaiello
Copy link

@PowerShell/powershell-committee reviewed the behavior I outlined above, there's a couple other cases without quotes around the last arg of the run, but none of them are problematic either. We're marking as approved.

@MichaelSimons
Copy link

@joeyaiello - Which of these scenarios require entry entrypoint script versus simply defining the ENTRYPOINT to be pwsh? In the long term, it feels like avoiding the entrypoint script would be the preferred solution. The entrypoint script makes it difficult for users to self discover what the entrypoint behavior is. You can't rely on docker inspect or looking at the Dockerfile. You have to inspect the script itself to figure out the behavior.

@richlander
Copy link

Interesting topic. Few thoughts for you to consider:

  • You may feel compelled over time to expand the scenarios in the script and it could become a long-term compat burden. Obviously, the script growing would be a bad thing. It isn't your product, but a convenience crutch.
  • Related, we have never felt (on my team) that we need to make Docker easier to use than it is. Certainly, we've tried to make good choices, but we don't try to paper over Docker-isms. We don't see this product approach as an adoption problem. If a design choice will make things easier, but won't improve adoption (for our product as a whole), and might be thought of as weird (not a standard pattern), we usually won't do it. At the end of the day, we only care about (growing) adoption.
  • I'm not a scripting expert, but the examples provided at the top don't all seem like close comparisons to PowerShell. For example, golang is the furthest off (note: .NET images have the same behavior as golang). Neither expose interactive experiences so that makes sense. Even if .NET had an interactive experience, I wouldn't expose it as the ENTRYPOINT. I suspect that perl and python are the closest to PowerShell. PHP and node seem less close. That's interesting.
  • I totally get the desire to present a default interactive PowerShell experience from docker run, although all of this goes away as a problem if you don't expose an entrypoint at all. That's how you use PowerShell with the .NET Core SDK image, as you know.
  • The list of experiences you've provided demonstrates that you cannot cover all scenarios. It's a slippery slope, and that brings us back to the first point. My belief is that the winning proposition is to define a simple standard model, and then document how people can do other things, in standard ways. It's incredibly liberating when you do that. People can ask for things, and you just say "You can do that via this standard pattern." They may say "but you could make it easier". And then you can say "We're not in the business of making Docker easier to use than it is."
  • Oh, and compatibility bites.

@TravisEz13
Copy link
Member Author

TravisEz13 commented Apr 9, 2020

@MichaelSimons

Which of these scenarios require entry entrypoint script versus simply defining the ENTRYPOINT to be pwsh?

I'll test all of these and verify I have tested all of these and added some important other scenarios. I corrected the paths to the ones in the container where relevant.

Tested and works:

  1. docker run -it mcr.microsoft.com/powershell:foo pwsh
  2. docker run -it mcr.microsoft.com/powershell:foo 'Do-Stuff -Bar'
    This is the new scenario
  3. docker run -it mcr.microsoft.com/powershell:foo '/bar.ps1'
    This is the new scenario
  4. docker run -it mcr.microsoft.com/powershell:foo '/opt/microsoft/powershell/7-preview/pwsh'
  5. docker run -it mcr.microsoft.com/powershell:foo '/opt/microsoft/powershell/7-preview/pwsh --% -c $psversiontable.psversion.tostring()'
    This does not work before the change. See note 1
  6. docker run -it mcr.microsoft.com/powershell:foo bash
  7. docker run -it mcr.microsoft.com/powershell:foo 'bash -c env'
    This would not work before the change. See note 1
  8. docker run -it mcr.microsoft.com/powershell:foo 'bash -c 1'
    This would not work before the change. See note 1
  9. docker run -it -v <mapping> -w <workDir> mcr.microsoft.com/powershell:foo pwsh -f bar.ps1

This will break with a entry point of ["pwsh", "-c"] instead of the proposed entrypoint

  1. docker run -it mcr.microsoft.com/powershell:foo pwsh -c '$psversiontable.psversion.tostring()'
    Note this is the major way the container is currently automated.
  2. docker run -it mcr.microsoft.com/powershell:foo /usr/bin/pwsh -c '$psversiontable.psversion.tostring()'
  3. docker run -it mcr.microsoft.com/powershell:foo '/usr/bin/pwsh -c $psversiontable.psversion.tostring()'
    This would not work before the change. . See note 1
  4. docker run -it -v <mapping> -w <workDir> mcr.microsoft.com/powershell:foo bar.ps1
    This also does not work interactively, even with shebang. Must be ./bar.ps1 or use pwsh -f bar.ps1

Note 1 - Implicit Entrypoint behavior for quoted cmd parameters

The the default entrypoint treats a quoted cmd parameter as a single parameter to the entrypoint and the first parameter is the executable. So, for example, '/usr/bin/pwsh -c $psversiontable.psversion.tostring()' would look for the executable /usr/bin/pwsh -c $psversiontable.psversion.tostring() which is not the intended behavior.

@richlander
Copy link

I was thinking more about this. This issue has the wrong title. It very much underplays what is being asked for. This is the feature description, and scope, I think:

  • Add entrypoint routing, which supports routing string-based commands to different executables, with special logic for pwsh. This avoids needing to (A) use the --entrypoint syntax to specify an executable, and (B) split the entrypoint (an argument to docker run) from its arguments (arguments to the container; they show up last).
  • Provide actionable error messages. Users should know that the entrypoint syntax they used failed in PowerShell, in Bash (or other shell), or in the script itself.
  • Support on Linux and Windows (a number of these appear like general concerns, not Linux-specific).

@TravisEz13 TravisEz13 changed the title Changing the EntryPoint Add entrypoint routing, which supports routing string-based commands to different executables, with special logic for pwsh Apr 11, 2020
@TravisEz13 TravisEz13 changed the title Add entrypoint routing, which supports routing string-based commands to different executables, with special logic for pwsh Add entrypoint routing, which supports routing string-based commands to different executables, with special logic for pwsh Apr 11, 2020
@TravisEz13
Copy link
Member Author

@richlander I updated the title and added a goals section to the description based on your comments

@joeyaiello
Copy link

@MichaelSimons The entrypoint behavior we've defined in the script is basically: pwsh -c except if you're calling pwsh or pwsh-preview in your -c, in which case, don't run PowerShell inside of PowerShell. The goal here is that folks already using the existing exec/bash behavior (no idea which Docker defaults to under the hood, but it's somewhat irrelevant to my point) to docker run mcr.microsoft.com/powershell pwsh -c Get-Foo (which is likely fairly common) don't double(-ish) their startup time.

If we want to make the docker inspect behavior better, we could simply add a comment to the Dockerfile ENTRYPOINT line that says "This is basically pwsh -c".

@richlander

Re: the long-term compat burden, I agree, but I don't think this has to be a slippery slope. Conceptually, this is a very thin wrapper around pwsh -c, and I believe fairly easy to communicate as such. We should be explicit about not wanting to add additional, complex logic here for any other reason.

I agree with you that a few of those original languages aren't 1:1 comparisons, and @TravisEz13 and I spoke pretty extensively about it, agreeing that there really wasn't a clear answer. The most apt comparison in my mind is Bash, where you're wanting to execute both .sh scripts / arbitrary executables as well as in-lined script, and where doing the former can be done via the latter. The big difference, as I noted above, is that opening Bash from Bash is very, very fast, and opening PowerShell from PowerShell is not.

I also agree we should address this on Windows as well, and I think @TravisEz13 is working on the .bat/.cmd equivalent already.

@TravisEz13 TravisEz13 added this to To do in Planned PowerShell Team work via automation May 17, 2020
@TravisEz13 TravisEz13 moved this from To do to In progress in Planned PowerShell Team work May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

5 participants