Using a container action requires an entrypoint to be specified

[AlexLukasT]

Hey,

I'm using the k8s hooks together with the Actions Runner Controller.

When I'm using a container action like this:

jobs:
  test:
    container: ubuntu:latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3
      - name: Custom action
        uses: ./.github/actions/test-action

with the custom action

runs:
  using: docker
  image: "docker://debian:buster"
  entrypoint: "bin/bash"  # this line is required!
  args:
    - "-c"
    - "ls"

it will correctly create the Debian container but requires the entrypoint to be specified.

I checked the input the hook receives and the entrypoint there is empty when it is not specified in the action explicitly. This leads to it being overridden by the default one (tail -f /dev/null) which causes the container and the job to hang indefinitely. I would expect that it falls back to the default endpoint of the image.

This problem blocks any usage of public actions that use containers without specifying an entrypoint.

To be honest, I'm also not 100% sure if this is an issue with this project or the ARC. Or is there a way to distinguish if the job is from a container action and use the default entrypoint of the image?

I'm using the latest version of the k8s hooks (v0.3.1) and the ARC (v0.27.3).

Thanks a lot for your help!

[AlexLukasT]

Update:

I was able to solve my issue by overriding the entrypoint of the action like this: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepswithentrypoint.

However, I would still be interested to figure out if there is an issue with handling the default entrypoint.

[nikola-jokic]

Hey @AlexLukasT,

Thanks for submitting this issue! We can't really fall back to the default entrypoint because we need to provide prependPath, change the directory, expose environment variables and then execute the entrypoint (https://github.com/actions/runner-container-hooks/blob/c37c5ca584154e58d50b143f1a960eeef1cd3317/packages/k8s/src/k8s/utils.ts#L137). Now, working directory and env variables could be applied in container spec, but PATH can't be dynamically modified.

That is one limitation here is that you need to provide an entry point in order to run a container step. Nevertheless, we can maybe throw from the hook saying that the entrypoint is a requirement in order to run a container action, instead of applying the default entrypoint as tail -f /dev/null.

[AlexLukasT]

Hey @nikola-jokic,

thanks for the explanation.

I think throwing out a warning or something like that would be really helpful. And maybe also to include it in the known limitations: https://github.com/actions/runner-container-hooks/tree/main/packages/k8s#limitations.

I spent a few hours trying to figure this out and it would be nice to prevent other people from having the same experience :)

[nikola-jokic]

Of course! Thank you for pointing it out :relaxed:.

[nikola-jokic]

Hey @AlexLukasT

I have looked into the entrypoint and it might not be a requirement for container actions. I updated the PR fixing it, but can you double-check if it is going to work in your use-case? I don't see why not, and it can eliminate the limitation on entrypoint. The only difference is that we were throwing in case working-directory does not exist on the pod, while now, I think the one would be created. Anyway, if you can, please try it out because I think we can eliminate this limitation :relaxed:

[AlexLukasT]

Hey @nikola-jokic ,

I had an extensive look and needed to apply more changes to make it work. For some reason the entryPointArgs were wrongly formatted. I printed the input the hook receives from stdin and it was already like this: entryPointArgs: [ '', '"-c"', '"ls"' ]. Does the ARC not parse the arguments correctly?

After fixing this and checking if entryPointArgs are provided it worked for me. Here is the full patch:

diff --git a/packages/k8s/src/hooks/run-container-step.ts b/packages/k8s/src/hooks/run-container-step.ts
index 1920045..08eabcf 100644
--- a/packages/k8s/src/hooks/run-container-step.ts
+++ b/packages/k8s/src/hooks/run-container-step.ts
@@ -79,7 +79,9 @@ function createPodSpec(
   podContainer.workingDir = container.workingDirectory
   if (container.entryPoint) {
     podContainer.command = [container.entryPoint]
-    podContainer.args = container.entryPointArgs
+  }
+  if (container.entryPointArgs) {
+    podContainer.args = trimEntryPointArgs(container.entryPointArgs)
   }

   if (secretName) {
@@ -96,3 +98,8 @@ function createPodSpec(

   return podContainer
 }
+
+function trimEntryPointArgs(entryPointArgs: string[]): string[] {
+  // remove empty arguments and all quotes
+  return entryPointArgs.filter(arg => arg).map(arg => arg.replace(/"/g, ''))
+}

[nikola-jokic]

Hey :wave: I will try to reproduce this. What is confusing to me is the first argument being empty string. That should not have happened... To reproduce this, can I use your workflow from the issue and just remove an entrypoint and args?

[AlexLukasT]

Yes this happens with the workflow from me either with the endpoint or when removing it (and the -c in the args).

[nikola-jokic]

Hey @AlexLukasT,

I have tried setting up container hooks and the following action executed successfully: workflow:

name: Container Hook no entrypoint args for docker action

on:
  workflow_dispatch:

jobs:
  test:
    runs-on: main-scaleset
    container: ubuntu:latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3
      - name: Custom action
        uses: ./dockeraction

dockeraction

name: 'Hello World'
description: 'Echo docker'
runs:
  using: 'docker'
  image: 'docker://{myimage}'

Can you create an example repo from which I can reproduce this?

[AlexLukasT]

Hey @nikola-jokic, sorry I got a bit confused here. Let me try to clarfiy:

When not specifying an entrypoint it will work fine, also for me, due to this if statement, right?

If I add args now, like this:

runs:
  using: docker
  image: "docker://debian:buster"
  args: ["pwd"]

it will still work, but the args are already wrong: (I'm showing the args from stdin here)

Adding also a custom entrypoint

runs:
  using: docker
  image: "docker://debian:buster"
  entrypoint: "/bin/bash"
  args: ["pwd"]

will cause an error, because now the malformed args are used in the created pod:

Are you able to reproduce this?

[nikola-jokic]

Hey @AlexLukasT,

Thanks, I was able to reproduce it. This might be something to do with the runner so I'm currently investigating why the runner passes an empty entrypoint as the first argument.

[mhuijgen]

@nikola-jokic What about the other issue @AlexLukasT described that all arguments get extra "" around them?

Im running into this issue when using the docker container hooks from this actions/runner-container-hooks repo on my self-hosted runner.

Workflow to reproduce:

name: "sarif-tool"

on:
  push:
    branches: [ "develop", master, support/* ]
  pull_request:
    # The branches below must be a subset of the branches above
    branches: [ "develop" ]

jobs:
  analyze:
    name: Analyze
    runs-on: [ self-hosted, Linux ]

    steps:
    - name: SARIF Multitool
      uses: microsoft/sarif-actions@v0.1
      with:
        command: "convert"

With container hooks enabled it fails to run the multitool with the proper args:

SARIF Multitool 4.2.1
© Microsoft Corporation. All rights reserved.

ERROR(S):
  Verb '"convert"' is not recognized.

  --help       Display this help screen.

  --version    Display version information.

When I disable the hooks on the runner and run the same flow it gives the following output:

SARIF Multitool 4.2.0
© Microsoft Corporation. All rights reserved.

ERROR(S):
  Required option 't, tool' is missing.
  A required value not bound to option name is missing.

  -t, --tool                    Required. The tool format of the input file.
                                Must be one of: AndroidStudio, ClangAnalyzer,
                                ClangTidy, CppCheck, ContrastSecurity,
                                FlawFinder, Fortify, FortifyFpr, FxCop, Hdf,
                                PREfast, Pylint, SemmleQL, StaticDriverVerifier,
                                TSLint, or a tool format for which a plugin
                                assembly provides the converter.

  -a, --plugin-assembly-path    Path to plugin assembly containing converter
                                types.

  --normalize-for-github        (Deprecated. Use --normalize-for-ghas instead.)
                                Normalize converted output to conform to GitHub
                                Advanced Security (GHAS) code scanning ingestion
                                requirements.

  --normalize-for-ghas          Normalize converted output to conform to GitHub
                                Advanced Security (GHAS) code scanning ingestion
                                requirements.

  -o, --output                  A file path to the generated SARIF log.

  --log                         Optionally present data, expressed as a
                                semicolon-delimited list enclosed in double
                                quotes, for governing output files. Valid values
                                include ForceOverwrite, Inline, PrettyPrint,
                                Minify or Optimize.

  --insert                      Optionally present data, expressed as a
                                semicolon-delimited list enclosed in double
                                quotes, that should be inserted into the log
                                file. Valid values include Hashes, TextFiles,
                                BinaryFiles, EnvironmentVariables,
                                RegionSnippets, ContextRegionSnippets,
                                ContextRegionSnippetPartialFingerprints, Guids,
                                VersionControlDetails, and
                                NondeterministicProperties.

  --remove                      Optionally present data, expressed as a
                                semicolon-delimited list enclosed in double
                                quotes, that should be not be persisted to or
                                which should be removed from the log file. Valid
                                values include Hashes, TextFiles, BinaryFiles,
                                EnvironmentVariables, RegionSnippets,
                                ContextRegionSnippets, Guids,
                                VersionControlDetails, and
                                NondeterministicProperties.

  -u, --uriBaseIds              Key + value pairs, expressed as a
                                semicolon-delimited list enclosed in double
                                quotes, that defines a uriBaseId and its
                                corresponding local file path. E.g.,
                                SRC=c:\src;TEST=c:\test

  -v, --sarif-output-version    (Default: Current) The SARIF version of the
                                output log file. Valid values are OneZeroZero
                                and Current

  --threads                     A count of threads that should be used for
                                multithreaded operations.

  --insert-property             JSON path + property values, expressed as a
                                semicolon-delimited list enclosed in double
                                quotes, that should be inserted into the output
                                log. Currently, only paths that point to a
                                version control provenance property bag is
                                supported, e.g.,
                                'runs[0].invocations[1].versionControlProvenance
                                .properties.myProperty=myValue'.

  --automation-id               An id that will be persisted to the
                                'Run.AutomationDetails.Id' property. See section
                                '3.17.3' of the SARIF specification for more
                                information.

  --automation-guid             A guid that will be persisted to the
                                'Run.AutomationDetails.Guid' property. See
                                section '3.17.4' of the SARIF specification for
                                more information.

  --help                        Display this help screen.

  --version                     Display version information.

  <inputFile> (pos. 0)          Required. A path to a file to process.

As you can tell from this in with the hooks it executes the entrypoint with arg '"convert"' And without the hooks it executes the entrypoint with arg 'convert' (the tool does now complain that the other options like -t tool is not supplied which I intentionally omitted. But the point is that the sarif-too in the hooks case literally gets "convert" as its first arg including the "" and does not recognize that as a valid verb/command for the tool.

So it seems a workaround like

+function trimEntryPointArgs(entryPointArgs: string[]): string[] {
+  // remove empty arguments and all quotes
+  return entryPointArgs.filter(arg => arg).map(arg => arg.replace(/"/g, ''))
+}

Is still needed to strip the extra "" from the args

Also it does not matter if I do or do not remove the "" around convert in the workflow. The arg given to the container still is "convert"

[mhuijgen]

Actually its a bit more complicated I noticed just now. The following is echoed in the job output for both with and without the container hooks (just the order of all the args to docker are a little bit different with or without hooks).

/usr/bin/docker run --rm --name 3dcd0d --workdir=/github/workspace --label=4f4d522d4465764f70732d4769744875622d52756e6e65722d3031 --network=github_network_a760d6f4-5fe6-4533-933f-a36d65ced997 xxx..Cut out all -e and -v options..xxx 4f4d522d4465764f70732d4769744875622d52756e6e65722d3031:3dcd0d "convert --tool CppCheck build-debian-buster-x86_64/cppcheck.result.xml -o cppcheck.sarif"

Without hooks it works. It seems "convert --tool CppCheck build-debian-buster-x86_64/cppcheck.result.xml -o cppcheck.sarif" is given to entrypoint.sh as $1.

However with the hooks, $1 in the entrypoint.sh is just "convert and the rest of the args end up in $2 $3 and so on with the last arg being cppcheck.sarif"

It seems that in the function

export async function containerRun(

args.entryPointArgs

has already been split into separate args, where it should have been just one arg, the full string convert --tool CppCheck build-debian-buster-x86_64/cppcheck.result.xml -o cppcheck.sarif

It seems the github/runner already did this when handing over the args to the hooks.

[nikola-jokic]

Hey @mhuijgen,

You are exactly right, and that is why I always try to evaluate what would be the expected way. The way you would specify the command is essentially the way you would do it in bash, but to invoke the tool as another process, we would need to do argument splitting the same way the bash does it.

And it is not always as easy due to ' and " mixed together. We can implement it based on the spec, but that is why I would prefer to use a third-party library that is handling many edge cases we could bump into. This is the reason why I wanted to investigate other approaches :relaxed:

[mhuijgen]

What I don't get just yet is why if the runner directly executes docker run ...... "convert --tool CppCheck */cppcheck.result.xml -o cppcheck.sarif" it works and via hooks it somehow scrambles the arguments given to docker process.

Both seem to execute docker command directly without first going through a shell.

In the runner case the quoted string ends up without quotes as a whole in the first argument. Im not familiar with cs code however, but from what I read a $"" interpolates any cs code between it, so my guess this line somehow converts the args to a string in a way that puts "convert --tool CppCheck */cppcheck.result.xml -o cppcheck.sarif" into one arg as it should be. dockerOptions.Add($"{container.ContainerEntryPointArgs}");

In the hooks case the hook already receives a cut up version of the string split on spaces and then ends up putting every piece as a separate argument to the docker process including a quote in front of the first arg and the trailing quote in the last arg.

The runner code somehow converts the container object to a JSON to send off to the hook. Isn't the problem then in this conversion where the string "convert --tool CppCheck */cppcheck.result.xml -o cppcheck.sarif" seems to be cut up into separate bits by the code in the hook:

  if (args.entryPointArgs) {
    for (const entryPointArg of args.entryPointArgs) {
      if (!entryPointArg) {
        continue
      }
      dockerArgs.push(entryPointArg)
    }
  }

"convert --tool CppCheck */cppcheck.result.xml -o cppcheck.sarif"

?