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

[potential-bug] raw_exec artifact not set as executable #4809

Closed
jrasell opened this issue Oct 26, 2018 · 7 comments · Fixed by #5511
Closed

[potential-bug] raw_exec artifact not set as executable #4809

jrasell opened this issue Oct 26, 2018 · 7 comments · Fixed by #5511

Comments

@jrasell
Copy link
Member

jrasell commented Oct 26, 2018

Nomad version

Nomad v0.8.3 (c85483da3471f4bd3a7c3de112e95f551071769f)

Operating system and Environment details

Distributor ID:	CentOS
Description:	CentOS Linux release 7.5.1804 (Core)
Release:	7.5.1804
Codename:	Core

Issue

When using the raw_exec driver and downloading an artifact binary, the binary is not getting set as executable.

The file permissions:

-rw-r--r-- 1 root root 13234400 Oct 26 09:34 nomad-toast_linux_amd64

The job error message:

Error: fork/exec /srv/nomad-client/alloc/2cd4397f-83ff-967a-049c-17d44593825e/nomad-toast-deployments/local/nomad-toast_linux_amd64: permission denied

As far as I am aware when using the raw_exec driver and downloading a binary, nomad/go-getter should be setting the file as executable.

Job file (if appropriate)

artifact {
        source = "https://github.com/jrasell/nomad-toast/releases/download/v0.0.1-rc1/nomad-toast_linux_amd64"

        options {
          checksum = "sha256:4fd86d228e539fad57de3d154c86ccab02d38a6e3d1b815fa34775351f3c287a"
        }
      }
config {
        command = ${NOMAD_TASK_DIR}/nomad-toast_linux_amd64
}
@preetapan
Copy link
Contributor

@jrasell from the following reproduction steps it looks like go-getter does not make the file executable. Nomad does not directly change file permissions in the artifact stanza.

preetha@preetha-work ~ $go-getter https://github.com/jrasell/nomad-toast/releases/download/v0.0.1-rc1/nomad-toast_linux_amd64 .
2018/10/26 14:51:15 Success!
preetha@preetha-work ~ $ls -l nomad-toast_linux_amd64 
-rw-rw-r-- 1 preetha preetha 13234400 Oct 26 14:51 nomad-toast_linux_amd64

@preetapan
Copy link
Contributor

preetapan commented Oct 26, 2018

@schmichael pointed out that our executor should mark the file as executable if its not already (

if err := makeExecutable(absPath); err != nil {
). any other errors or log messages from the client you can share before it gets to this state?

@jrasell
Copy link
Member Author

jrasell commented Oct 26, 2018

@preetapan thanks for the quick responses; i'll run it again tomorrow and capture any relevant logs from the client to add here.

@preetapan
Copy link
Contributor

I was able to reproduce this with the following job spec file, adapted from https://github.com/hashicorp/nomad/blob/community-call-0/community/specs/hello.nomad but using raw_exec instead of exec driver

job "hello2" {
  datacenters = ["dc1"]

  update {
    max_parallel = 1
    min_healthy_time = "15s"
    auto_revert = true
  }

  group "hello" {

    count = 3

    task "hello" {
      driver = "raw_exec"

      config {
        command = "${NOMAD_TASK_DIR}/hellov1"
      }

      artifact {
         source = "https://s3.amazonaws.com/nomad-community-demo/hellov1"
         mode = "file"
         destination = "local/hellov1"
      }

      resources {
        cpu    = 500
        memory = 256
        network {
          mbits = 10
          port "web" {}
        }
      }

      service {
        name = "hello"
        tags = ["urlprefix-/"]
        port = "web"
        check {
          name     = "alive"
          type     = "http"
          path     = "/"
          interval = "10s"
          timeout  = "2s"
        }
      }
    }
  }
}

@preetapan
Copy link
Contributor

After playing around with the above spec file some more, got it to work after changing the command to the following change:

config {
 command = "local/hellov1"
}

That's the relative path to the task directory, use that instead of ${NOMAD_TASK_DIR} and it works.

If that doesn't work for you, please re-open this.

@preetapan
Copy link
Contributor

We can make a fully interpolated path like `${NOMAD_TASK_DIR}/binary' work with a small patch to

func lookupBin(taskDir string, bin string) (string, error) {
, so reopening this.

@preetapan preetapan reopened this Oct 26, 2018
notnoop pushed a commit that referenced this issue Apr 2, 2019
Follow up to #4809, so we only
return bin path if it's inside task dir; otherwise, use old mechanism.

We should aim priortize bin path inside task dir, otherwise, we'd be
marking the wrong file as executable in later steps.
notnoop pushed a commit that referenced this issue Apr 2, 2019
notnoop pushed a commit that referenced this issue Apr 2, 2019
notnoop added a commit that referenced this issue Apr 3, 2019
This is a follow up work to #4813 to fix #4809 and fix a regression introduced in 0.9 in marking files in libcontainer executable.

#4809 bug is that `lookupBin` uses `exec.LookPath` when not inspecting task dir files.  `exec.LookPath` only returns a file if it's already marked as an executable path in https://github.com/golang/go/blob/go1.12.1/src/os/exec/lp_unix.go#L24-L27 .  This affects raw exec as if passed an absolute path to file, `lookupBin` returns an error if file isn't already an executable.  This explains why the error manifests when an absolute interpolated path is used (e.g. `${NOMAD_TASK_DIR}/hellov1`) but not when using a task rel dir (e.g. `local/hellov1`) in the above examples used in ticket.

PR #4813 remedied this problem for raw exec but inadvertably broke libcontainer executor, as it made `lookupBin` returns the paths to host files rather than ones found inside chroot.

This PR reorders the evaluation, so we go back to 0.8 behavior of looking up task directories first, but then check for host paths before using `exec.LookPath`.

This PR is broken into three commits to illustrate evolution and confirming hypothesis:
1. 9adab75 : Adding a test illustrating how libcontainer executor fails at marking processes as executable in https://travis-ci.org/hashicorp/nomad/jobs/514942694 - note that the test doesn't depend on artifacts or interpolated paths
2. d441cdd: reverting PR #4809 and showing the test fail now with raw_exec case (as expected) in https://travis-ci.org/hashicorp/nomad/jobs/514944065
2. 244544b: in where we add the check in appropriate place next to `exec.LookPath(...)` for absolute paths and have a green job in https://travis-ci.org/hashicorp/nomad/jobs/514945024

## Future work

Inspecting `lookupBin` in 0.8 and 0.9 case, we have a bug in using `exec.LookPath` for the libcontainer executor case.  We should be looking up paths based on the container chroot and container PATH rather than the host's.  However, this is not a 0.9.0 regression and was present in 0.8; so punting to fix it post 0.9.
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants