From fe2b3e382d691b946dc2aacb7fa236d52a2c8c44 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 21 Apr 2017 12:20:05 -0700 Subject: [PATCH 01/58] Restart tasks on upgrade with script checks and old executors --- client/alloc_runner.go | 11 ++++++++-- client/task_runner.go | 50 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/client/alloc_runner.go b/client/alloc_runner.go index 8ca6f671eae..7da40c67814 100644 --- a/client/alloc_runner.go +++ b/client/alloc_runner.go @@ -186,13 +186,20 @@ func (r *AllocRunner) RestoreState() error { continue } - if err := tr.RestoreState(); err != nil { - r.logger.Printf("[ERR] client: failed to restore state for alloc %s task '%s': %v", r.alloc.ID, name, err) + if restartReason, err := tr.RestoreState(); err != nil { + r.logger.Printf("[ERR] client: failed to restore state for alloc %s task %q: %v", r.alloc.ID, name, err) mErr.Errors = append(mErr.Errors, err) } else if !r.alloc.TerminalStatus() { // Only start if the alloc isn't in a terminal status. go tr.Run() + + // Restart task runner if RestoreState gave a reason + if restartReason != "" { + r.logger.Printf("[INFO] client: restarting alloc %s task %q due to upgrade: %s", r.alloc.ID, name, restartReason) + tr.Restart("upgrade", restartReason) + } } + } return mErr.ErrorOrNil() diff --git a/client/task_runner.go b/client/task_runner.go index c578ca5ea0b..a9f1d744292 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -16,6 +16,7 @@ import ( "github.com/golang/snappy" "github.com/hashicorp/consul-template/signals" "github.com/hashicorp/go-multierror" + version "github.com/hashicorp/go-version" "github.com/hashicorp/nomad/client/allocdir" "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/driver" @@ -234,17 +235,20 @@ func (r *TaskRunner) stateFilePath() string { return path } -// RestoreState is used to restore our state -func (r *TaskRunner) RestoreState() error { +// RestoreState is used to restore our state. If a non-empty string is returned +// the task is restarted with the string as the reason. This is useful for +// backwards incompatible upgrades that need to restart tasks with a new +// executor. +func (r *TaskRunner) RestoreState() (string, error) { // Load the snapshot var snap taskRunnerState if err := restoreState(r.stateFilePath(), &snap); err != nil { - return err + return "", err } // Restore fields if snap.Task == nil { - return fmt.Errorf("task runner snapshot includes nil Task") + return "", fmt.Errorf("task runner snapshot includes nil Task") } else { r.task = snap.Task } @@ -255,7 +259,7 @@ func (r *TaskRunner) RestoreState() error { r.setCreatedResources(snap.CreatedResources) if err := r.setTaskEnv(); err != nil { - return fmt.Errorf("client: failed to create task environment for task %q in allocation %q: %v", + return "", fmt.Errorf("client: failed to create task environment for task %q in allocation %q: %v", r.task.Name, r.alloc.ID, err) } @@ -265,7 +269,7 @@ func (r *TaskRunner) RestoreState() error { data, err := ioutil.ReadFile(tokenPath) if err != nil { if !os.IsNotExist(err) { - return fmt.Errorf("failed to read token for task %q in alloc %q: %v", r.task.Name, r.alloc.ID, err) + return "", fmt.Errorf("failed to read token for task %q in alloc %q: %v", r.task.Name, r.alloc.ID, err) } // Token file doesn't exist @@ -276,10 +280,11 @@ func (r *TaskRunner) RestoreState() error { } // Restore the driver + restartReason := "" if snap.HandleID != "" { d, err := r.createDriver() if err != nil { - return err + return "", err } ctx := driver.NewExecContext(r.taskDir) @@ -289,7 +294,11 @@ func (r *TaskRunner) RestoreState() error { if err != nil { r.logger.Printf("[ERR] client: failed to open handle to task %q for alloc %q: %v", r.task.Name, r.alloc.ID, err) - return nil + return "", nil + } + + if pre06ScriptCheck(snap.Version, r.task.Services) { + restartReason = "upgrading pre-0.6 script checks" } if err := r.registerServices(d, handle); err != nil { @@ -308,7 +317,30 @@ func (r *TaskRunner) RestoreState() error { r.running = true r.runningLock.Unlock() } - return nil + return restartReason, nil +} + +var ver06 = version.Must(version.NewVersion("0.6.0dev")) + +// pre06ScriptCheck returns true if version is prior to 0.6.0dev. +func pre06ScriptCheck(ver string, services []*structs.Service) bool { + v, err := version.NewVersion(ver) + if err != nil { + // Treat it as old + return true + } + if !v.LessThan(ver06) { + // >= 0.6.0dev + return false + } + for _, service := range services { + for _, check := range service.Checks { + if check.Type == "script" { + return true + } + } + } + return false } // SaveState is used to snapshot our state From ba1c2e7936d8f6d9b6945d9823e9b251f8baec85 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 21 Apr 2017 12:20:48 -0700 Subject: [PATCH 02/58] Upgrade go-version to one compatible with Nomad versioning --- .../hashicorp/go-version/.travis.yml | 11 -- .../github.com/hashicorp/go-version/README.md | 2 +- .../hashicorp/go-version/constraint.go | 24 +++- .../hashicorp/go-version/version.go | 115 ++++++++++++++---- vendor/vendor.json | 4 +- 5 files changed, 120 insertions(+), 36 deletions(-) delete mode 100644 vendor/github.com/hashicorp/go-version/.travis.yml diff --git a/vendor/github.com/hashicorp/go-version/.travis.yml b/vendor/github.com/hashicorp/go-version/.travis.yml deleted file mode 100644 index 9f30eecd73f..00000000000 --- a/vendor/github.com/hashicorp/go-version/.travis.yml +++ /dev/null @@ -1,11 +0,0 @@ -language: go - -go: - - 1.0 - - 1.1 - - 1.2 - - 1.3 - - 1.4 - -script: - - go test diff --git a/vendor/github.com/hashicorp/go-version/README.md b/vendor/github.com/hashicorp/go-version/README.md index 1d50070f131..6f3a15ce772 100644 --- a/vendor/github.com/hashicorp/go-version/README.md +++ b/vendor/github.com/hashicorp/go-version/README.md @@ -1,5 +1,5 @@ # Versioning Library for Go -[![Build Status](https://travis-ci.org/hashicorp/go-version.svg?branch=master)](https://travis-ci.org/hashicorp/go-version) +[![Build Status](https://travis-ci.org/hashicorp/go-version.svg?branch=master)](https://travis-ci.org/hashicorp/go-version) go-version is a library for parsing versions and version constraints, and verifying versions against a set of constraints. go-version diff --git a/vendor/github.com/hashicorp/go-version/constraint.go b/vendor/github.com/hashicorp/go-version/constraint.go index 091cfab389f..8c73df0602b 100644 --- a/vendor/github.com/hashicorp/go-version/constraint.go +++ b/vendor/github.com/hashicorp/go-version/constraint.go @@ -37,7 +37,7 @@ func init() { } ops := make([]string, 0, len(constraintOperators)) - for k, _ := range constraintOperators { + for k := range constraintOperators { ops = append(ops, regexp.QuoteMeta(k)) } @@ -142,15 +142,37 @@ func constraintLessThanEqual(v, c *Version) bool { } func constraintPessimistic(v, c *Version) bool { + // If the version being checked is naturally less than the constraint, then there + // is no way for the version to be valid against the constraint if v.LessThan(c) { return false } + // We'll use this more than once, so grab the length now so it's a little cleaner + // to write the later checks + cs := len(c.segments) + // If the version being checked has less specificity than the constraint, then there + // is no way for the version to be valid against the constraint + if cs > len(v.segments) { + return false + } + + // Check the segments in the constraint against those in the version. If the version + // being checked, at any point, does not have the same values in each index of the + // constraints segments, then it cannot be valid against the constraint. for i := 0; i < c.si-1; i++ { if v.segments[i] != c.segments[i] { return false } } + // Check the last part of the segment in the constraint. If the version segment at + // this index is less than the constraints segment at this index, then it cannot + // be valid against the constraint + if c.segments[cs-1] > v.segments[cs-1] { + return false + } + + // If nothing has rejected the version by now, it's valid return true } diff --git a/vendor/github.com/hashicorp/go-version/version.go b/vendor/github.com/hashicorp/go-version/version.go index c7147ff7bf5..dfe509caa0c 100644 --- a/vendor/github.com/hashicorp/go-version/version.go +++ b/vendor/github.com/hashicorp/go-version/version.go @@ -14,8 +14,8 @@ var versionRegexp *regexp.Regexp // The raw regular expression string used for testing the validity // of a version. -const VersionRegexpRaw string = `v?([0-9]+(\.[0-9]+){0,2})` + - `(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?` + +const VersionRegexpRaw string = `v?([0-9]+(\.[0-9]+)*?)` + + `(-?([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?` + `(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?` + `?` @@ -23,7 +23,7 @@ const VersionRegexpRaw string = `v?([0-9]+(\.[0-9]+){0,2})` + type Version struct { metadata string pre string - segments []int + segments []int64 si int } @@ -38,20 +38,23 @@ func NewVersion(v string) (*Version, error) { if matches == nil { return nil, fmt.Errorf("Malformed version: %s", v) } - segmentsStr := strings.Split(matches[1], ".") - segments := make([]int, len(segmentsStr), 3) + segments := make([]int64, len(segmentsStr)) si := 0 for i, str := range segmentsStr { - val, err := strconv.ParseInt(str, 10, 32) + val, err := strconv.ParseInt(str, 10, 64) if err != nil { return nil, fmt.Errorf( "Error parsing version: %s", err) } - segments[i] = int(val) - si += 1 + segments[i] = int64(val) + si++ } + + // Even though we could support more than three segments, if we + // got less than three, pad it with 0s. This is to cover the basic + // default usecase of semver, which is MAJOR.MINOR.PATCH at the minimum for i := len(segments); i < 3; i++ { segments = append(segments, 0) } @@ -86,8 +89,8 @@ func (v *Version) Compare(other *Version) int { return 0 } - segmentsSelf := v.Segments() - segmentsOther := other.Segments() + segmentsSelf := v.Segments64() + segmentsOther := other.Segments64() // If the segments are the same, we must compare on prerelease info if reflect.DeepEqual(segmentsSelf, segmentsOther) { @@ -106,21 +109,56 @@ func (v *Version) Compare(other *Version) int { return comparePrereleases(preSelf, preOther) } + // Get the highest specificity (hS), or if they're equal, just use segmentSelf length + lenSelf := len(segmentsSelf) + lenOther := len(segmentsOther) + hS := lenSelf + if lenSelf < lenOther { + hS = lenOther + } // Compare the segments - for i := 0; i < len(segmentsSelf); i++ { + // Because a constraint could have more/less specificity than the version it's + // checking, we need to account for a lopsided or jagged comparison + for i := 0; i < hS; i++ { + if i > lenSelf-1 { + // This means Self had the lower specificity + // Check to see if the remaining segments in Other are all zeros + if !allZero(segmentsOther[i:]) { + // if not, it means that Other has to be greater than Self + return -1 + } + break + } else if i > lenOther-1 { + // this means Other had the lower specificity + // Check to see if the remaining segments in Self are all zeros - + if !allZero(segmentsSelf[i:]) { + //if not, it means that Self has to be greater than Other + return 1 + } + break + } lhs := segmentsSelf[i] rhs := segmentsOther[i] - if lhs == rhs { continue } else if lhs < rhs { return -1 - } else { - return 1 } + // Otherwis, rhs was > lhs, they're not equal + return 1 } - panic("should not be reached") + // if we got this far, they're equal + return 0 +} + +func allZero(segs []int64) bool { + for _, s := range segs { + if s != 0 { + return false + } + } + return true } func comparePart(preSelf string, preOther string) int { @@ -128,24 +166,38 @@ func comparePart(preSelf string, preOther string) int { return 0 } + selfNumeric := true + _, err := strconv.ParseInt(preSelf, 10, 64) + if err != nil { + selfNumeric = false + } + + otherNumeric := true + _, err = strconv.ParseInt(preOther, 10, 64) + if err != nil { + otherNumeric = false + } + // if a part is empty, we use the other to decide if preSelf == "" { - _, notIsNumeric := strconv.ParseInt(preOther, 10, 64) - if notIsNumeric == nil { + if otherNumeric { return -1 } return 1 } if preOther == "" { - _, notIsNumeric := strconv.ParseInt(preSelf, 10, 64) - if notIsNumeric == nil { + if selfNumeric { return 1 } return -1 } - if preSelf > preOther { + if selfNumeric && !otherNumeric { + return -1 + } else if !selfNumeric && otherNumeric { + return 1 + } else if preSelf > preOther { return 1 } @@ -226,12 +278,25 @@ func (v *Version) Prerelease() string { return v.pre } -// Segments returns the numeric segments of the version as a slice. +// Segments returns the numeric segments of the version as a slice of ints. // // This excludes any metadata or pre-release information. For example, // for a version "1.2.3-beta", segments will return a slice of // 1, 2, 3. func (v *Version) Segments() []int { + segmentSlice := make([]int, len(v.segments)) + for i, v := range v.segments { + segmentSlice[i] = int(v) + } + return segmentSlice +} + +// Segments64 returns the numeric segments of the version as a slice of int64s. +// +// This excludes any metadata or pre-release information. For example, +// for a version "1.2.3-beta", segments will return a slice of +// 1, 2, 3. +func (v *Version) Segments64() []int64 { return v.segments } @@ -239,7 +304,13 @@ func (v *Version) Segments() []int { // and metadata information. func (v *Version) String() string { var buf bytes.Buffer - fmt.Fprintf(&buf, "%d.%d.%d", v.segments[0], v.segments[1], v.segments[2]) + fmtParts := make([]string, len(v.segments)) + for i, s := range v.segments { + // We can ignore err here since we've pre-parsed the values in segments + str := strconv.FormatInt(s, 10) + fmtParts[i] = str + } + fmt.Fprintf(&buf, strings.Join(fmtParts, ".")) if v.pre != "" { fmt.Fprintf(&buf, "-%s", v.pre) } diff --git a/vendor/vendor.json b/vendor/vendor.json index ec1b1787731..1c4d1fc6431 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -743,8 +743,10 @@ "revision": "42a2b573b664dbf281bd48c3cc12c086b17a39ba" }, { + "checksumSHA1": "tUGxc7rfX0cmhOOUDhMuAZ9rWsA=", "path": "github.com/hashicorp/go-version", - "revision": "2e7f5ea8e27bb3fdf9baa0881d16757ac4637332" + "revision": "03c5bf6be031b6dd45afec16b1cf94fc8938bc77", + "revisionTime": "2017-02-02T08:07:59Z" }, { "checksumSHA1": "d9PxF1XQGLMJZRct2R8qVM/eYlE=", From 90f5e232a5f6a07c3d52a37b97ddd6df17e33170 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 21 Apr 2017 16:20:37 -0700 Subject: [PATCH 03/58] Switch java/exec to use Exec in Executor --- client/alloc_runner.go | 1 - client/driver/exec.go | 7 ++++- client/driver/executor/executor.go | 46 ++++++++++++++++++++++++++++-- client/driver/executor_plugin.go | 36 +++++++++++++++++++++++ client/driver/java.go | 7 ++++- client/task_runner.go | 8 ++++-- 6 files changed, 98 insertions(+), 7 deletions(-) diff --git a/client/alloc_runner.go b/client/alloc_runner.go index 7da40c67814..35a58e1cd05 100644 --- a/client/alloc_runner.go +++ b/client/alloc_runner.go @@ -195,7 +195,6 @@ func (r *AllocRunner) RestoreState() error { // Restart task runner if RestoreState gave a reason if restartReason != "" { - r.logger.Printf("[INFO] client: restarting alloc %s task %q due to upgrade: %s", r.alloc.ID, name, restartReason) tr.Restart("upgrade", restartReason) } } diff --git a/client/driver/exec.go b/client/driver/exec.go index bc6ee3aee25..fdc3bbc0186 100644 --- a/client/driver/exec.go +++ b/client/driver/exec.go @@ -259,7 +259,12 @@ func (h *execHandle) Update(task *structs.Task) error { } func (h *execHandle) Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error) { - return execChroot(ctx, h.taskDir.Dir, cmd, args) + deadline, ok := ctx.Deadline() + if !ok { + // No deadline set on context; default to 1 minute + deadline = time.Now().Add(time.Minute) + } + return h.executor.Exec(deadline, cmd, args) } func (h *execHandle) Signal(s os.Signal) error { diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index 90797fbefa2..295dc89283a 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -1,6 +1,7 @@ package executor import ( + "context" "fmt" "io/ioutil" "log" @@ -15,6 +16,7 @@ import ( "syscall" "time" + "github.com/armon/circbuf" "github.com/hashicorp/go-multierror" "github.com/mitchellh/go-ps" "github.com/shirou/gopsutil/process" @@ -57,6 +59,7 @@ type Executor interface { Version() (*ExecutorVersion, error) Stats() (*cstructs.TaskResourceUsage, error) Signal(s os.Signal) error + Exec(deadline time.Time, cmd string, args []string) ([]byte, int, error) } // ExecutorContext holds context to configure the command user @@ -203,8 +206,8 @@ func (e *UniversalExecutor) SetContext(ctx *ExecutorContext) error { return nil } -// LaunchCmd launches a process and returns it's state. It also configures an -// applies isolation on certain platforms. +// LaunchCmd launches the main process and returns its state. It also +// configures an applies isolation on certain platforms. func (e *UniversalExecutor) LaunchCmd(command *ExecCommand) (*ProcessState, error) { e.logger.Printf("[DEBUG] executor: launching command %v %v", command.Cmd, strings.Join(command.Args, " ")) @@ -283,6 +286,45 @@ func (e *UniversalExecutor) LaunchCmd(command *ExecCommand) (*ProcessState, erro return &ProcessState{Pid: e.cmd.Process.Pid, ExitCode: -1, IsolationConfig: ic, Time: time.Now()}, nil } +// Exec a command inside a container for exec and java drivers. +func (e *UniversalExecutor) Exec(deadline time.Time, name string, args []string) ([]byte, int, error) { + ctx, cancel := context.WithDeadline(context.Background(), deadline) + defer cancel() + + name = e.ctx.TaskEnv.ReplaceEnv(name) + cmd := exec.CommandContext(ctx, name, e.ctx.TaskEnv.ParseAndReplace(args)...) + + // Copy runtime environment from the main command + cmd.SysProcAttr = e.cmd.SysProcAttr + cmd.Dir = e.cmd.Dir + cmd.Env = e.ctx.TaskEnv.EnvList() + + // Capture output + buf, _ := circbuf.NewBuffer(int64(dstructs.CheckBufSize)) + cmd.Stdout = buf + cmd.Stderr = buf + + if err := cmd.Run(); err != nil { + exitErr, ok := err.(*exec.ExitError) + if !ok { + // Non-exit error, return it and let the caller treat + // it as a critical failure + return nil, 0, err + } + + // Some kind of error happened; default to critical + exitCode := 2 + if status, ok := exitErr.Sys().(syscall.WaitStatus); ok { + exitCode = status.ExitStatus() + } + + // Don't return the exitError as the caller only needs the + // output and code. + return buf.Bytes(), exitCode, nil + } + return buf.Bytes(), 0, nil +} + // configureLoggers sets up the standard out/error file rotators func (e *UniversalExecutor) configureLoggers() error { e.rotatorLock.Lock() diff --git a/client/driver/executor_plugin.go b/client/driver/executor_plugin.go index 7c4074feffc..01e63343d28 100644 --- a/client/driver/executor_plugin.go +++ b/client/driver/executor_plugin.go @@ -6,6 +6,7 @@ import ( "net/rpc" "os" "syscall" + "time" "github.com/hashicorp/go-plugin" "github.com/hashicorp/nomad/client/driver/executor" @@ -33,6 +34,17 @@ type LaunchCmdArgs struct { Cmd *executor.ExecCommand } +type ExecCmdArgs struct { + Deadline time.Time + Name string + Args []string +} + +type ExecCmdReturn struct { + Output []byte + Code int +} + func (e *ExecutorRPC) LaunchCmd(cmd *executor.ExecCommand) (*executor.ProcessState, error) { var ps *executor.ProcessState err := e.client.Call("Plugin.LaunchCmd", LaunchCmdArgs{Cmd: cmd}, &ps) @@ -91,6 +103,20 @@ func (e *ExecutorRPC) Signal(s os.Signal) error { return e.client.Call("Plugin.Signal", &s, new(interface{})) } +func (e *ExecutorRPC) Exec(deadline time.Time, name string, args []string) ([]byte, int, error) { + req := ExecCmdArgs{ + Deadline: deadline, + Name: name, + Args: args, + } + var resp *ExecCmdReturn + err := e.client.Call("Plugin.Exec", req, &resp) + if resp == nil { + return nil, 0, err + } + return resp.Output, resp.Code, err +} + type ExecutorRPCServer struct { Impl executor.Executor logger *log.Logger @@ -165,6 +191,16 @@ func (e *ExecutorRPCServer) Signal(args os.Signal, resp *interface{}) error { return e.Impl.Signal(args) } +func (e *ExecutorRPCServer) Exec(args ExecCmdArgs, result *ExecCmdReturn) error { + out, code, err := e.Impl.Exec(args.Deadline, args.Name, args.Args) + ret := &ExecCmdReturn{ + Output: out, + Code: code, + } + *result = *ret + return err +} + type ExecutorPlugin struct { logger *log.Logger Impl *ExecutorRPCServer diff --git a/client/driver/java.go b/client/driver/java.go index c215e6882d5..5801d2e83aa 100644 --- a/client/driver/java.go +++ b/client/driver/java.go @@ -390,7 +390,12 @@ func (h *javaHandle) Update(task *structs.Task) error { } func (h *javaHandle) Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error) { - return execChroot(ctx, h.taskDir, cmd, args) + deadline, ok := ctx.Deadline() + if !ok { + // No deadline set on context; default to 1 minute + deadline = time.Now().Add(time.Minute) + } + return h.executor.Exec(deadline, cmd, args) } func (h *javaHandle) Signal(s os.Signal) error { diff --git a/client/task_runner.go b/client/task_runner.go index a9f1d744292..b88bc6a7d20 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -297,7 +297,7 @@ func (r *TaskRunner) RestoreState() (string, error) { return "", nil } - if pre06ScriptCheck(snap.Version, r.task.Services) { + if pre06ScriptCheck(snap.Version, r.task.Driver, r.task.Services) { restartReason = "upgrading pre-0.6 script checks" } @@ -323,7 +323,11 @@ func (r *TaskRunner) RestoreState() (string, error) { var ver06 = version.Must(version.NewVersion("0.6.0dev")) // pre06ScriptCheck returns true if version is prior to 0.6.0dev. -func pre06ScriptCheck(ver string, services []*structs.Service) bool { +func pre06ScriptCheck(ver, driver string, services []*structs.Service) bool { + if driver != "exec" && driver != "java" { + // Only exec and java are affected + return false + } v, err := version.NewVersion(ver) if err != nil { // Treat it as old From 29f222d4615117e9b26d1ce91e91a9e144fb5162 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Fri, 21 Apr 2017 16:50:20 -0700 Subject: [PATCH 04/58] Change raw_exec to use simplified exec wrapper --- client/driver/raw_exec.go | 10 +++++++++- client/driver/rkt.go | 9 ++++++++- client/driver/utils.go | 18 +++++++++--------- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/client/driver/raw_exec.go b/client/driver/raw_exec.go index 5e602f47bb1..b799a32041d 100644 --- a/client/driver/raw_exec.go +++ b/client/driver/raw_exec.go @@ -10,7 +10,9 @@ import ( "time" "github.com/hashicorp/go-plugin" + "github.com/hashicorp/nomad/client/allocdir" "github.com/hashicorp/nomad/client/config" + "github.com/hashicorp/nomad/client/driver/env" "github.com/hashicorp/nomad/client/driver/executor" dstructs "github.com/hashicorp/nomad/client/driver/structs" "github.com/hashicorp/nomad/client/fingerprint" @@ -48,6 +50,8 @@ type rawExecHandle struct { logger *log.Logger waitCh chan *dstructs.WaitResult doneCh chan struct{} + taskEnv *env.TaskEnvironment + taskDir *allocdir.TaskDir } // NewRawExecDriver is used to create a new raw exec driver @@ -165,6 +169,8 @@ func (d *RawExecDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandl logger: d.logger, doneCh: make(chan struct{}), waitCh: make(chan *dstructs.WaitResult, 1), + taskEnv: d.taskEnv, + taskDir: ctx.TaskDir, } go h.run() return h, nil @@ -212,6 +218,8 @@ func (d *RawExecDriver) Open(ctx *ExecContext, handleID string) (DriverHandle, e version: id.Version, doneCh: make(chan struct{}), waitCh: make(chan *dstructs.WaitResult, 1), + taskEnv: d.taskEnv, + taskDir: ctx.TaskDir, } go h.run() return h, nil @@ -247,7 +255,7 @@ func (h *rawExecHandle) Update(task *structs.Task) error { } func (h *rawExecHandle) Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error) { - return execChroot(ctx, "", cmd, args) + return execScript(ctx, h.taskDir.Dir, h.taskEnv, cmd, args) } func (h *rawExecHandle) Signal(s os.Signal) error { diff --git a/client/driver/rkt.go b/client/driver/rkt.go index a8cff3c0249..930d6672894 100644 --- a/client/driver/rkt.go +++ b/client/driver/rkt.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/go-version" "github.com/hashicorp/nomad/client/allocdir" "github.com/hashicorp/nomad/client/config" + "github.com/hashicorp/nomad/client/driver/env" "github.com/hashicorp/nomad/client/driver/executor" dstructs "github.com/hashicorp/nomad/client/driver/structs" cstructs "github.com/hashicorp/nomad/client/structs" @@ -87,6 +88,8 @@ type RktDriverConfig struct { // rktHandle is returned from Start/Open as a handle to the PID type rktHandle struct { uuid string + env *env.TaskEnvironment + taskDir *allocdir.TaskDir pluginClient *plugin.Client executorPid int executor executor.Executor @@ -474,6 +477,8 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e maxKill := d.DriverContext.config.MaxKillTimeout h := &rktHandle{ uuid: uuid, + env: d.taskEnv, + taskDir: ctx.TaskDir, pluginClient: pluginClient, executor: execIntf, executorPid: ps.Pid, @@ -514,6 +519,8 @@ func (d *RktDriver) Open(ctx *ExecContext, handleID string) (DriverHandle, error // Return a driver handle h := &rktHandle{ uuid: id.UUID, + env: d.taskEnv, + taskDir: ctx.TaskDir, pluginClient: pluginClient, executorPid: id.ExecutorPid, executor: exec, @@ -566,7 +573,7 @@ func (h *rktHandle) Exec(ctx context.Context, cmd string, args []string) ([]byte enterArgs[1] = h.uuid enterArgs[2] = cmd copy(enterArgs[3:], args) - return execChroot(ctx, "", rktCmd, enterArgs) + return execScript(ctx, h.taskDir.Dir, h.env, rktCmd, enterArgs) } func (h *rktHandle) Signal(s os.Signal) error { diff --git a/client/driver/utils.go b/client/driver/utils.go index def668548ff..07864bae411 100644 --- a/client/driver/utils.go +++ b/client/driver/utils.go @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-plugin" "github.com/hashicorp/nomad/client/config" + "github.com/hashicorp/nomad/client/driver/env" "github.com/hashicorp/nomad/client/driver/executor" cstructs "github.com/hashicorp/nomad/client/driver/structs" "github.com/hashicorp/nomad/helper/discover" @@ -182,18 +183,17 @@ func getExecutorUser(task *structs.Task) string { return task.User } -// execChroot executes cmd with args inside chroot if set and returns the -// output, exit code, and error. If chroot is an empty string the command is -// executed on the host. -func execChroot(ctx context.Context, chroot, name string, args []string) ([]byte, int, error) { - buf, _ := circbuf.NewBuffer(int64(cstructs.CheckBufSize)) +// execScript executes cmd with args and returns the output, exit code, and +// error. Output is truncated to client/driver/structs.CheckBufSize +func execScript(ctx context.Context, dir string, env *env.TaskEnvironment, name string, args []string) ([]byte, int, error) { + name = env.ReplaceEnv(name) + args = env.ParseAndReplace(args) cmd := exec.CommandContext(ctx, name, args...) - cmd.Dir = "/" + cmd.Dir = dir + cmd.Env = env.EnvList() + buf, _ := circbuf.NewBuffer(int64(cstructs.CheckBufSize)) cmd.Stdout = buf cmd.Stderr = buf - if chroot != "" { - setChroot(cmd, chroot) - } if err := cmd.Run(); err != nil { exitErr, ok := err.(*exec.ExitError) if !ok { From 8f518c4c1f2d14feeb7e0d858bf884d04d94d5e8 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 25 Apr 2017 11:13:06 -0700 Subject: [PATCH 05/58] Test env+cgroups for exec driver checks --- client/driver/exec_test.go | 53 +++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/client/driver/exec_test.go b/client/driver/exec_test.go index 6a580f24352..5760b175163 100644 --- a/client/driver/exec_test.go +++ b/client/driver/exec_test.go @@ -283,7 +283,8 @@ func TestExecDriverUser(t *testing.T) { } } -// TestExecDriver_HandlerExec ensures the exec driver's handle properly executes commands inside the chroot. +// TestExecDriver_HandlerExec ensures the exec driver's handle properly +// executes commands inside the container. func TestExecDriver_HandlerExec(t *testing.T) { ctestutils.ExecCompatible(t) task := &structs.Task{ @@ -315,20 +316,60 @@ func TestExecDriver_HandlerExec(t *testing.T) { t.Fatalf("missing handle") } - // Exec a command that should work - out, code, err := handle.Exec(context.TODO(), "/usr/bin/stat", []string{"/alloc"}) + // Exec a command that should work and dump the environment + out, code, err := handle.Exec(context.Background(), "/bin/sh", []string{"-c", "env | grep NOMAD"}) if err != nil { t.Fatalf("error exec'ing stat: %v", err) } if code != 0 { t.Fatalf("expected `stat /alloc` to succeed but exit code was: %d", code) } - if expected := 100; len(out) < expected { - t.Fatalf("expected at least %d bytes of output but found %d:\n%s", expected, len(out), out) + + // Assert exec'd commands are run in a task-like environment + scriptEnv := make(map[string]string) + for _, line := range strings.Split(string(out), "\n") { + if line == "" { + continue + } + parts := strings.SplitN(string(line), "=", 2) + if len(parts) != 2 { + t.Fatalf("Invalid env var: %q", line) + } + scriptEnv[parts[0]] = parts[1] + } + if v, ok := scriptEnv["NOMAD_SECRETS_DIR"]; !ok || v != "/secrets" { + t.Errorf("Expected NOMAD_SECRETS_DIR=/secrets but found=%t value=%q", ok, v) + } + if v, ok := scriptEnv["NOMAD_ALLOC_ID"]; !ok || v != ctx.DriverCtx.allocID { + t.Errorf("Expected NOMAD_SECRETS_DIR=%q but found=%t value=%q", ok, v) + } + + // Assert cgroup membership + out, code, err = handle.Exec(context.Background(), "/bin/cat", []string{"/proc/self/cgroup"}) + if err != nil { + t.Fatalf("error exec'ing cat /proc/self/cgroup: %v", err) + } + if code != 0 { + t.Fatalf("expected `cat /proc/self/cgroup` to succeed but exit code was: %d", code) + } + found := false + for _, line := range strings.Split(string(out), "\n") { + // Every cgroup entry should be /nomad/$ALLOC_ID + if line == "" { + continue + } + if !strings.Contains(line, ":/nomad/") { + t.Errorf("Not a member of the alloc's cgroup: expected=...:/nomad/... -- found=%q", line) + continue + } + found = true + } + if !found { + t.Errorf("exec'd command isn't in the task's cgroup") } // Exec a command that should fail - out, code, err = handle.Exec(context.TODO(), "/usr/bin/stat", []string{"lkjhdsaflkjshowaisxmcvnlia"}) + out, code, err = handle.Exec(context.Background(), "/usr/bin/stat", []string{"lkjhdsaflkjshowaisxmcvnlia"}) if err != nil { t.Fatalf("error exec'ing stat: %v", err) } From 2cc492c95e06a74b8564146c230d26dc024977f2 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 25 Apr 2017 11:41:03 -0700 Subject: [PATCH 06/58] Test pre-0.6 script check upgrade path --- client/alloc_runner_test.go | 79 +++++++++++++++++++++++++++++++++++++ client/task_runner.go | 10 ++++- client/task_runner_test.go | 2 +- 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/client/alloc_runner_test.go b/client/alloc_runner_test.go index f1bc828b807..99b03f65989 100644 --- a/client/alloc_runner_test.go +++ b/client/alloc_runner_test.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "os" "path/filepath" + "strings" "testing" "text/template" "time" @@ -476,6 +477,84 @@ func TestAllocRunner_SaveRestoreState_TerminalAlloc(t *testing.T) { }) } +// TestAllocRunner_SaveRestoreState_Upgrade asserts that pre-0.6 exec tasks are +// restarted on upgrade. +func TestAllocRunner_SaveRestoreState_Upgrade(t *testing.T) { + alloc := mock.Alloc() + task := alloc.Job.TaskGroups[0].Tasks[0] + task.Driver = "mock_driver" + task.Config = map[string]interface{}{ + "exit_code": "0", + "run_for": "10s", + } + + upd, ar := testAllocRunnerFromAlloc(alloc, false) + // Hack in old version to cause an upgrade on RestoreState + origConfig := ar.config.Copy() + ar.config.Version = "0.5.6" + go ar.Run() + + // Snapshot state + testutil.WaitForResult(func() (bool, error) { + return len(ar.tasks) == 1, nil + }, func(err error) { + t.Fatalf("task never started: %v", err) + }) + + err := ar.SaveState() + if err != nil { + t.Fatalf("err: %v", err) + } + + // Create a new alloc runner + l2 := prefixedTestLogger("----- ar2: ") + ar2 := NewAllocRunner(l2, origConfig, upd.Update, + &structs.Allocation{ID: ar.alloc.ID}, ar.vaultClient, + ar.consulClient) + err = ar2.RestoreState() + if err != nil { + t.Fatalf("err: %v", err) + } + go ar2.Run() + + testutil.WaitForResult(func() (bool, error) { + if len(ar2.tasks) != 1 { + return false, fmt.Errorf("Incorrect number of tasks") + } + + if upd.Count < 3 { + return false, nil + } + + for _, ev := range ar2.alloc.TaskStates["web"].Events { + if strings.HasSuffix(ev.RestartReason, pre06ScriptCheckReason) { + return true, nil + } + } + return false, fmt.Errorf("no restart with proper reason found") + }, func(err error) { + t.Fatalf("err: %v\nAllocs: %#v\nWeb State: %#v", err, upd.Allocs, ar2.alloc.TaskStates["web"]) + }) + + // Destroy and wait + ar2.Destroy() + start := time.Now() + + testutil.WaitForResult(func() (bool, error) { + alloc := ar2.Alloc() + if alloc.ClientStatus != structs.AllocClientStatusComplete { + return false, fmt.Errorf("Bad client status; got %v; want %v", alloc.ClientStatus, structs.AllocClientStatusComplete) + } + return true, nil + }, func(err error) { + t.Fatalf("err: %v %#v %#v", err, upd.Allocs[0], ar.alloc.TaskStates) + }) + + if time.Since(start) > time.Duration(testutil.TestMultiplier()*5)*time.Second { + t.Fatalf("took too long to terminate") + } +} + // Ensure pre-#2132 state files containing the Context struct are properly // migrated to the new format. // diff --git a/client/task_runner.go b/client/task_runner.go index b88bc6a7d20..f3715f338bf 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -298,7 +298,7 @@ func (r *TaskRunner) RestoreState() (string, error) { } if pre06ScriptCheck(snap.Version, r.task.Driver, r.task.Services) { - restartReason = "upgrading pre-0.6 script checks" + restartReason = pre06ScriptCheckReason } if err := r.registerServices(d, handle); err != nil { @@ -320,11 +320,16 @@ func (r *TaskRunner) RestoreState() (string, error) { return restartReason, nil } +// ver06 is used for checking for pre-0.6 script checks var ver06 = version.Must(version.NewVersion("0.6.0dev")) +// pre06ScriptCheckReason is the restart reason given when a pre-0.6 script +// check is found on an exec/java task. +const pre06ScriptCheckReason = "upgrading pre-0.6 script checks" + // pre06ScriptCheck returns true if version is prior to 0.6.0dev. func pre06ScriptCheck(ver, driver string, services []*structs.Service) bool { - if driver != "exec" && driver != "java" { + if driver != "exec" && driver != "java" && driver != "mock_driver" { // Only exec and java are affected return false } @@ -352,6 +357,7 @@ func (r *TaskRunner) SaveState() error { r.persistLock.Lock() defer r.persistLock.Unlock() + r.logger.Printf("[XXX] task_runner: %q", r.config.Version) snap := taskRunnerState{ Task: r.task, Version: r.config.Version, diff --git a/client/task_runner_test.go b/client/task_runner_test.go index ede8cb1647c..b373d6db874 100644 --- a/client/task_runner_test.go +++ b/client/task_runner_test.go @@ -369,7 +369,7 @@ func TestTaskRunner_SaveRestoreState(t *testing.T) { tr2 := NewTaskRunner(ctx.tr.logger, ctx.tr.config, ctx.upd.Update, ctx.tr.taskDir, ctx.tr.alloc, task2, ctx.tr.vaultClient, ctx.tr.consul) tr2.restartTracker = noRestartsTracker() - if err := tr2.RestoreState(); err != nil { + if _, err := tr2.RestoreState(); err != nil { t.Fatalf("err: %v", err) } go tr2.Run() From 17decf9f66556b5907f152fec85b0508a2a50c6b Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Tue, 25 Apr 2017 23:29:43 +0100 Subject: [PATCH 07/58] Add verification options to TLS config struct --- nomad/structs/config/tls.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index aea3cc4dfd4..48a7ef0211d 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -28,6 +28,12 @@ type TLSConfig struct { // KeyFile is used to provide a TLS key that is used for serving TLS connections. // Must be provided to serve TLS connections. KeyFile string `mapstructure:"key_file"` + + // VerifyIncoming + VerifyIncoming bool `mapstructure:"verify_incoming"` + + // VerifyOutgoing + VerifyOutgoing bool `mapstructure:"verify_outgoing"` } // Merge is used to merge two TLS configs together @@ -52,6 +58,12 @@ func (t *TLSConfig) Merge(b *TLSConfig) *TLSConfig { if b.KeyFile != "" { result.KeyFile = b.KeyFile } + if b.VerifyIncoming { + result.VerifyIncoming = true + } + if b.VerifyOutgoing { + result.VerifyOutgoing = true + } return &result } From 44a91c395f2833b50deb7b0e206cbb0ace03dc68 Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Tue, 25 Apr 2017 23:33:12 +0100 Subject: [PATCH 08/58] Copy TLSConfig verification flags in server create --- command/agent/http.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/http.go b/command/agent/http.go index 8dbfca78eeb..70fef37db46 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -65,8 +65,8 @@ func NewHTTPServer(agent *Agent, config *Config) (*HTTPServer, error) { // If TLS is enabled, wrap the listener with a TLS listener if config.TLSConfig.EnableHTTP { tlsConf := &tlsutil.Config{ - VerifyIncoming: false, - VerifyOutgoing: true, + VerifyIncoming: config.TLSConfig.VerifyIncoming, + VerifyOutgoing: config.TLSConfig.VerifyOutgoing, VerifyServerHostname: config.TLSConfig.VerifyServerHostname, CAFile: config.TLSConfig.CAFile, CertFile: config.TLSConfig.CertFile, From 54e5dd6ff153e2e8bb237d7e2a55f662d2f18965 Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Tue, 25 Apr 2017 23:35:47 +0100 Subject: [PATCH 09/58] Verification options allowed in TLS config --- command/agent/config_parse.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/command/agent/config_parse.go b/command/agent/config_parse.go index 43e9e5b22e2..da14f6fd12a 100644 --- a/command/agent/config_parse.go +++ b/command/agent/config_parse.go @@ -689,6 +689,8 @@ func parseTLSConfig(result **config.TLSConfig, list *ast.ObjectList) error { "ca_file", "cert_file", "key_file", + "verify_incoming", + "verify_outgoing", } if err := checkHCLKeys(listVal, valid); err != nil { From dadfc95bec631ba1f9d4f27dc1675ee404171941 Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Wed, 26 Apr 2017 18:58:19 +0100 Subject: [PATCH 10/58] apply gofmt --- nomad/structs/config/tls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index 48a7ef0211d..694400e61cd 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -28,10 +28,10 @@ type TLSConfig struct { // KeyFile is used to provide a TLS key that is used for serving TLS connections. // Must be provided to serve TLS connections. KeyFile string `mapstructure:"key_file"` - + // VerifyIncoming VerifyIncoming bool `mapstructure:"verify_incoming"` - + // VerifyOutgoing VerifyOutgoing bool `mapstructure:"verify_outgoing"` } From 6ca05757eebf0c7ab8aa18e496d6922ce2be7049 Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Wed, 26 Apr 2017 21:13:54 +0100 Subject: [PATCH 11/58] fix config parse test --- command/agent/config-test-fixtures/basic.hcl | 2 ++ command/agent/config_parse_test.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/command/agent/config-test-fixtures/basic.hcl b/command/agent/config-test-fixtures/basic.hcl index cd741295df6..65c0b874eef 100644 --- a/command/agent/config-test-fixtures/basic.hcl +++ b/command/agent/config-test-fixtures/basic.hcl @@ -138,4 +138,6 @@ tls { ca_file = "foo" cert_file = "bar" key_file = "pipe" + verify_incoming = true + verify_outgoing = true } diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index 19fccdf64a6..cec5cbef20d 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -154,6 +154,8 @@ func TestConfig_Parse(t *testing.T) { CAFile: "foo", CertFile: "bar", KeyFile: "pipe", + VerifyIncoming: true, + VerifyOutgoing: true, }, HTTPAPIResponseHeaders: map[string]string{ "Access-Control-Allow-Origin": "*", From 06f3aac9de9d5263271d6900266beecc5da607ad Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Thu, 27 Apr 2017 23:27:58 +0100 Subject: [PATCH 12/58] clean up consul earlier when destroying a task --- client/task_runner.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/client/task_runner.go b/client/task_runner.go index c578ca5ea0b..be32215ecf4 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -918,6 +918,7 @@ func (r *TaskRunner) run() { select { case success := <-prestartResultCh: if !success { + r.consulCleanup() r.cleanup() r.setState(structs.TaskStateDead, nil) return @@ -1025,6 +1026,11 @@ func (r *TaskRunner) run() { r.runningLock.Lock() running := r.running r.runningLock.Unlock() + + // Remove from consul before killing the task so that traffic + // can be rerouted + r.consulCleanup() + if !running { r.cleanup() r.setState(structs.TaskStateDead, r.destroyEvent) @@ -1059,6 +1065,7 @@ func (r *TaskRunner) run() { // shouldRestart will block if the task should restart after a delay. restart := r.shouldRestart() if !restart { + r.consulCleanup() r.cleanup() r.setState(structs.TaskStateDead, nil) return @@ -1073,12 +1080,14 @@ func (r *TaskRunner) run() { } } -// cleanup removes Consul entries and calls Driver.Cleanup when a task is -// stopping. Errors are logged. -func (r *TaskRunner) cleanup() { +func (r *TaskRunner) consulCleanup() { // Remove from Consul r.consul.RemoveTask(r.alloc.ID, r.task) +} +// cleanup calls Driver.Cleanup when a task is +// stopping. Errors are logged. +func (r *TaskRunner) cleanup() { drv, err := r.createDriver() if err != nil { r.logger.Printf("[ERR] client: error creating driver to cleanup resources: %v", err) From aa2da9ea50384ed7f780a67e760bb13429c76d7f Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Fri, 28 Apr 2017 09:19:15 +0100 Subject: [PATCH 13/58] address feedback --- client/task_runner.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/client/task_runner.go b/client/task_runner.go index be32215ecf4..2bb98922fd5 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -918,7 +918,6 @@ func (r *TaskRunner) run() { select { case success := <-prestartResultCh: if !success { - r.consulCleanup() r.cleanup() r.setState(structs.TaskStateDead, nil) return @@ -1026,17 +1025,16 @@ func (r *TaskRunner) run() { r.runningLock.Lock() running := r.running r.runningLock.Unlock() - - // Remove from consul before killing the task so that traffic - // can be rerouted - r.consulCleanup() - if !running { r.cleanup() r.setState(structs.TaskStateDead, r.destroyEvent) return } + // Remove from consul before killing the task so that traffic + // can be rerouted + r.consul.RemoveTask(r.alloc.ID, r.task) + // Store the task event that provides context on the task // destroy. The Killed event is set from the alloc_runner and // doesn't add detail @@ -1065,7 +1063,6 @@ func (r *TaskRunner) run() { // shouldRestart will block if the task should restart after a delay. restart := r.shouldRestart() if !restart { - r.consulCleanup() r.cleanup() r.setState(structs.TaskStateDead, nil) return @@ -1080,14 +1077,12 @@ func (r *TaskRunner) run() { } } -func (r *TaskRunner) consulCleanup() { +// cleanup removes Consul entries and calls Driver.Cleanup when a task is +// stopping. Errors are logged. +func (r *TaskRunner) cleanup() { // Remove from Consul r.consul.RemoveTask(r.alloc.ID, r.task) -} -// cleanup calls Driver.Cleanup when a task is -// stopping. Errors are logged. -func (r *TaskRunner) cleanup() { drv, err := r.createDriver() if err != nil { r.logger.Printf("[ERR] client: error creating driver to cleanup resources: %v", err) From a4ad6eb31993dd7cbd1dd1a134a7dfc764df6236 Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Fri, 28 Apr 2017 10:45:09 +0100 Subject: [PATCH 14/58] reduce to one configuration option There should be just one option, verify_https_client, which controls incoming and outgoing validation for the HTTPS wrapper --- command/agent/config-test-fixtures/basic.hcl | 3 +-- command/agent/config_parse.go | 3 +-- command/agent/http.go | 4 ++-- nomad/structs/config/tls.go | 15 ++++----------- 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/command/agent/config-test-fixtures/basic.hcl b/command/agent/config-test-fixtures/basic.hcl index 65c0b874eef..8d4880a7d27 100644 --- a/command/agent/config-test-fixtures/basic.hcl +++ b/command/agent/config-test-fixtures/basic.hcl @@ -138,6 +138,5 @@ tls { ca_file = "foo" cert_file = "bar" key_file = "pipe" - verify_incoming = true - verify_outgoing = true + verify_https_client = true } diff --git a/command/agent/config_parse.go b/command/agent/config_parse.go index da14f6fd12a..403f5b75b5c 100644 --- a/command/agent/config_parse.go +++ b/command/agent/config_parse.go @@ -689,8 +689,7 @@ func parseTLSConfig(result **config.TLSConfig, list *ast.ObjectList) error { "ca_file", "cert_file", "key_file", - "verify_incoming", - "verify_outgoing", + "verify_https_client", } if err := checkHCLKeys(listVal, valid); err != nil { diff --git a/command/agent/http.go b/command/agent/http.go index 70fef37db46..787f02e662f 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -65,8 +65,8 @@ func NewHTTPServer(agent *Agent, config *Config) (*HTTPServer, error) { // If TLS is enabled, wrap the listener with a TLS listener if config.TLSConfig.EnableHTTP { tlsConf := &tlsutil.Config{ - VerifyIncoming: config.TLSConfig.VerifyIncoming, - VerifyOutgoing: config.TLSConfig.VerifyOutgoing, + VerifyIncoming: config.TLSConfig.VerifyHTTPSClient, + VerifyOutgoing: config.TLSConfig.VerifyHTTPSClient, VerifyServerHostname: config.TLSConfig.VerifyServerHostname, CAFile: config.TLSConfig.CAFile, CertFile: config.TLSConfig.CertFile, diff --git a/nomad/structs/config/tls.go b/nomad/structs/config/tls.go index 694400e61cd..2baa76a07f7 100644 --- a/nomad/structs/config/tls.go +++ b/nomad/structs/config/tls.go @@ -29,11 +29,8 @@ type TLSConfig struct { // Must be provided to serve TLS connections. KeyFile string `mapstructure:"key_file"` - // VerifyIncoming - VerifyIncoming bool `mapstructure:"verify_incoming"` - - // VerifyOutgoing - VerifyOutgoing bool `mapstructure:"verify_outgoing"` + // Verify connections to the HTTPS API + VerifyHTTPSClient bool `mapstructure:"verify_https_client"` } // Merge is used to merge two TLS configs together @@ -58,12 +55,8 @@ func (t *TLSConfig) Merge(b *TLSConfig) *TLSConfig { if b.KeyFile != "" { result.KeyFile = b.KeyFile } - if b.VerifyIncoming { - result.VerifyIncoming = true - } - if b.VerifyOutgoing { - result.VerifyOutgoing = true + if b.VerifyHTTPSClient { + result.VerifyHTTPSClient = true } - return &result } From 36f595480eee3d36765cf0e0b0bf95022e5147bf Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Sat, 29 Apr 2017 08:26:12 +0100 Subject: [PATCH 15/58] address feedback --- command/agent/http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/http.go b/command/agent/http.go index 787f02e662f..bdae5ee337c 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -66,7 +66,7 @@ func NewHTTPServer(agent *Agent, config *Config) (*HTTPServer, error) { if config.TLSConfig.EnableHTTP { tlsConf := &tlsutil.Config{ VerifyIncoming: config.TLSConfig.VerifyHTTPSClient, - VerifyOutgoing: config.TLSConfig.VerifyHTTPSClient, + VerifyOutgoing: true, VerifyServerHostname: config.TLSConfig.VerifyServerHostname, CAFile: config.TLSConfig.CAFile, CertFile: config.TLSConfig.CertFile, From 3d2213e3b9816b3945b8c3c6dffdeaaa4e3b700c Mon Sep 17 00:00:00 2001 From: Aaron Kunz Date: Sun, 30 Apr 2017 09:42:17 +0200 Subject: [PATCH 16/58] Fix wrong text for 'Guides' item in sidebar --- website/source/layouts/_sidebar.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/layouts/_sidebar.erb b/website/source/layouts/_sidebar.erb index 19bd5f1d78f..b0268b7f55d 100644 --- a/website/source/layouts/_sidebar.erb +++ b/website/source/layouts/_sidebar.erb @@ -7,7 +7,7 @@
  • Intro
    • -
  • Docs
    • +
  • Guides
  • Docs
  • Community
  • Security
    • From b16409ac54f8d37beecb73edcad478d80eefdb55 Mon Sep 17 00:00:00 2001 From: Pete Wildsmith Date: Sun, 30 Apr 2017 15:40:04 +0100 Subject: [PATCH 17/58] update test --- command/agent/config_parse_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index cec5cbef20d..9774b0db3fd 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -154,8 +154,7 @@ func TestConfig_Parse(t *testing.T) { CAFile: "foo", CertFile: "bar", KeyFile: "pipe", - VerifyIncoming: true, - VerifyOutgoing: true, + VerifyHTTPSClient: true, }, HTTPAPIResponseHeaders: map[string]string{ "Access-Control-Allow-Origin": "*", From 93d7626ac5c1ea92663b68ebe89b4e90a027a275 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Mon, 1 May 2017 17:36:20 -0700 Subject: [PATCH 18/58] fix old arch --- website/source/docs/job-specification/group.html.md | 2 +- website/source/docs/runtime/interpolation.html.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/website/source/docs/job-specification/group.html.md b/website/source/docs/job-specification/group.html.md index 22c38915073..d5b075e9486 100644 --- a/website/source/docs/job-specification/group.html.md +++ b/website/source/docs/job-specification/group.html.md @@ -81,7 +81,7 @@ will restrict the tasks to 64-bit operating systems. ```hcl group "example" { constraint { - attribute = "${attr.arch}" + attribute = "${attr.cpu.arch}" value = "amd64" } diff --git a/website/source/docs/runtime/interpolation.html.md b/website/source/docs/runtime/interpolation.html.md index 6e9fd2ce2fa..b23917967aa 100644 --- a/website/source/docs/runtime/interpolation.html.md +++ b/website/source/docs/runtime/interpolation.html.md @@ -94,7 +94,7 @@ driver. ${attr.<property>} Property given by property on the client - ${attr.arch} => amd64 + ${attr.cpu.arch} => amd64 ${meta.<key>} @@ -111,7 +111,7 @@ Below is a table documenting common node properties: Description - ${attr.arch} + ${attr.cpu.arch} CPU architecture of the client (e.g. amd64, 386) @@ -166,7 +166,7 @@ Here are some examples of using node attributes and properties in a job file: job "docs" { # This will constrain this job to only run on 64-bit clients. constraint { - attribute = "${attr.arch}" + attribute = "${attr.cpu.arch}" value = "amd64" } From 43fd0e28f0c4730b4dd24ffb78322d145f86420a Mon Sep 17 00:00:00 2001 From: Aaron Kunz Date: Tue, 2 May 2017 07:27:11 +0200 Subject: [PATCH 19/58] Fix wrong link to GitHub project in sidebar --- website/source/layouts/_sidebar.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/layouts/_sidebar.erb b/website/source/layouts/_sidebar.erb index 19bd5f1d78f..72103c2b283 100644 --- a/website/source/layouts/_sidebar.erb +++ b/website/source/layouts/_sidebar.erb @@ -23,7 +23,7 @@
  • - + <%= inline_svg "github.svg" %> GitHub
    • From 7776b8d9af49c7b2486cad7719567d6c561774ae Mon Sep 17 00:00:00 2001 From: Trevor Hartman Date: Tue, 2 May 2017 07:54:46 -0600 Subject: [PATCH 20/58] Update K8S max nodes According to http://blog.kubernetes.io/2017/03/scalability-updates-in-kubernetes-1.6.html --- website/source/intro/vs/kubernetes.html.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/intro/vs/kubernetes.html.md b/website/source/intro/vs/kubernetes.html.md index 94c50ca90e1..a59f0491206 100644 --- a/website/source/intro/vs/kubernetes.html.md +++ b/website/source/intro/vs/kubernetes.html.md @@ -34,7 +34,7 @@ Nomad combines a lightweight resource manager and a sophisticated scheduler into a single system. By default, Nomad is distributed, highly available, and operationally simple. -Kubernetes documentation states they can support clusters greater than 1,000 nodes +Kubernetes documentation states they can support clusters greater than 5,000 nodes and they support a multi-AZ/multi-region configuration. Nomad has been tested on clusters up to 5,000 nodes, but is expected to work on much larger clusters as well. Nomad also supports multi-datacenter and multi-region configurations. From 26609bc832b57eae21ccaa41c4e38d9f37f47fcc Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 16:48:16 -0700 Subject: [PATCH 21/58] Extensively test verify_https_client behavior verify_https_client support added in #2587 --- command/agent/http_test.go | 124 +++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/command/agent/http_test.go b/command/agent/http_test.go index de27c47c463..379bb06a273 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -2,12 +2,16 @@ package agent import ( "bytes" + "crypto/tls" + "crypto/x509" "encoding/json" "fmt" "io" "io/ioutil" + "net" "net/http" "net/http/httptest" + "net/url" "os" "strconv" "testing" @@ -15,6 +19,7 @@ import ( "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/nomad/structs/config" "github.com/hashicorp/nomad/testutil" ) @@ -337,6 +342,125 @@ func TestParseRegion(t *testing.T) { } } +// TestHTTP_VerifyHTTPSClient asserts that a client certificate signed by the +// appropriate CA is required when VerifyHTTPSClient=true. +func TestHTTP_VerifyHTTPSClient(t *testing.T) { + const ( + cafile = "../../helper/tlsutil/testdata/ca.pem" + foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" + fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" + ) + s := makeHTTPServer(t, func(c *Config) { + c.Region = "foo" // match the region on foocert + c.TLSConfig = &config.TLSConfig{ + EnableHTTP: true, + VerifyHTTPSClient: true, + CAFile: cafile, + CertFile: foocert, + KeyFile: fookey, + } + }) + defer s.Cleanup() + + reqURL := fmt.Sprintf("https://%s/v1/agent/self", s.Agent.config.AdvertiseAddrs.HTTP) + + // FAIL: Requests that expect 127.0.0.1 as the name should fail + resp, err := http.Get(reqURL) + if err == nil { + resp.Body.Close() + t.Fatalf("expected non-nil error but received: %v", resp.StatusCode) + } + urlErr, ok := err.(*url.Error) + if !ok { + t.Fatalf("expected a *url.Error but received: %T -> %v", err, err) + } + hostErr, ok := urlErr.Err.(x509.HostnameError) + if !ok { + t.Fatalf("expected a x509.HostnameError but received: %T -> %v", urlErr.Err, urlErr.Err) + } + if expected := "127.0.0.1"; hostErr.Host != expected { + t.Fatalf("expected hostname on error to be %q but found %q", expected, hostErr.Host) + } + + // FAIL: Requests that specify a valid hostname but not the CA should + // fail + tlsConf := &tls.Config{ + ServerName: "client.regionFoo.nomad", + } + transport := &http.Transport{TLSClientConfig: tlsConf} + client := &http.Client{Transport: transport} + req, err := http.NewRequest("GET", reqURL, nil) + if err != nil { + t.Fatalf("error creating request: %v", err) + } + resp, err = client.Do(req) + if err == nil { + resp.Body.Close() + t.Fatalf("expected non-nil error but received: %v", resp.StatusCode) + } + urlErr, ok = err.(*url.Error) + if !ok { + t.Fatalf("expected a *url.Error but received: %T -> %v", err, err) + } + _, ok = urlErr.Err.(x509.UnknownAuthorityError) + if !ok { + t.Fatalf("expected a x509.UnknownAuthorityError but received: %T -> %v", urlErr.Err, urlErr.Err) + } + + // FAIL: Requests that specify a valid hostname and CA cert but lack a + // client certificate should fail + cacertBytes, err := ioutil.ReadFile(cafile) + if err != nil { + t.Fatalf("error reading cacert: %v", err) + } + tlsConf.RootCAs = x509.NewCertPool() + tlsConf.RootCAs.AppendCertsFromPEM(cacertBytes) + req, err = http.NewRequest("GET", reqURL, nil) + if err != nil { + t.Fatalf("error creating request: %v", err) + } + resp, err = client.Do(req) + if err == nil { + resp.Body.Close() + t.Fatalf("expected non-nil error but received: %v", resp.StatusCode) + } + urlErr, ok = err.(*url.Error) + if !ok { + t.Fatalf("expected a *url.Error but received: %T -> %v", err, err) + } + opErr, ok := urlErr.Err.(*net.OpError) + if !ok { + t.Fatalf("expected a *net.OpErr but received: %T -> %v", urlErr.Err, urlErr.Err) + } + const badCertificate = "tls: bad certificate" // from crypto/tls/alert.go:52 and RFC 5246 ยง A.3 + if opErr.Err.Error() != badCertificate { + t.Fatalf("expected tls.alert bad_certificate but received: %q", opErr.Err.Error()) + } + + // PASS: Requests that specify a valid hostname, CA cert, and client + // certificate succeed. + tlsConf.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { + c, err := tls.LoadX509KeyPair(foocert, fookey) + if err != nil { + t.Logf("error loading client certificate: %v", err) + return nil, err + } + return &c, nil + } + req, err = http.NewRequest("GET", reqURL, nil) + if err != nil { + t.Fatalf("error creating request: %v", err) + } + resp, err = client.Do(req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resp.Body.Close() + if resp.StatusCode != 200 { + t.Fatalf("expected 200 status code but got: %d", resp.StatusCode) + } +} + // assertIndex tests that X-Nomad-Index is set and non-zero func assertIndex(t *testing.T, resp *httptest.ResponseRecorder) { header := resp.Header().Get("X-Nomad-Index") From 731995e34b79cf42cb92819914cc3158c6d30b0c Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 17:10:16 -0700 Subject: [PATCH 22/58] Document verify_https_client --- CHANGELOG.md | 2 ++ website/source/docs/agent/configuration/tls.html.md | 4 ++++ website/source/docs/agent/encryption.html.md | 12 +++++++++++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 228e7528cf9..c120c73bcba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ IMPROVEMENTS: * core: Back-pressure when evaluations are nacked and ensure scheduling progress on evaluation failures [GH-2555] * core: Track multiple job versions and add a stopped state for jobs [GH-2566] + * api: Add `verify_https_client` to require certificates from HTTP clients + [GH-2587] * api/job: Ability to revert job to older versions [GH-2575] * client: Fingerprint all routable addresses on an interface including IPv6 addresses [GH-2536] diff --git a/website/source/docs/agent/configuration/tls.html.md b/website/source/docs/agent/configuration/tls.html.md index 80c3042d5c6..5534eef7ab4 100644 --- a/website/source/docs/agent/configuration/tls.html.md +++ b/website/source/docs/agent/configuration/tls.html.md @@ -54,6 +54,10 @@ the [Agent's Gossip and RPC Encryption](/docs/agent/encryption.html). a Nomad client makes the client use TLS for making RPC requests to the Nomad servers. +- `verify_https_client` `(bool: false)` - Specifies agents should require + client certificates for all incoming HTTPS requests. The client certificates + must be signed by the same CA as Nomad. + - `verify_server_hostname` `(bool: false)` - Specifies if outgoing TLS connections should verify the server's hostname. diff --git a/website/source/docs/agent/encryption.html.md b/website/source/docs/agent/encryption.html.md index 99068eba70f..f21ea8c6fcb 100644 --- a/website/source/docs/agent/encryption.html.md +++ b/website/source/docs/agent/encryption.html.md @@ -69,9 +69,19 @@ export NOMAD_CACERT=/path/to/ca.pem Run any command except `agent` with `-h` to see all environment variables and flags. For example: `nomad status -h` -Since HTTPS currently does not validate client certificates you do not need to +By default HTTPS does not validate client certificates, so you do not need to give the command line tool access to any private keys. +### Network Isolation with TLS + +If you want to isolate Nomad agents on a network with TLS you need to enable +both [`verify_https_client`][tls] and [`verify_server_hostname`][tls]. This +will cause agents to require client certificates for all incoming HTTPS +connections as well as verify proper names on all other certificates. + +Consul will not attempt to health check agents with `verify_https_client` set +as it is unable to use client certificates. + ## Encryption Examples ### TLS Configuration using `cfssl` From a2d453ad142b2e0edd5ead88b832c3d400faa3d6 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 17:24:45 -0700 Subject: [PATCH 23/58] Fix error check for consul skip tls verify support --- command/agent/agent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 8080591e694..967740ce08b 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -691,7 +691,7 @@ func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error { } // Determine version for TLSSkipVerify - if self, err := client.Agent().Self(); err != nil { + if self, err := client.Agent().Self(); err == nil { a.consulSupportsTLSSkipVerify = consulSupportsTLSSkipVerify(self) } From 4e11242225e4644cee445fa818d73f96f624303d Mon Sep 17 00:00:00 2001 From: Aaron Kunz Date: Wed, 3 May 2017 19:45:03 +0200 Subject: [PATCH 24/58] Fix typo --- website/source/docs/operating-a-job/configuring-tasks.html.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/docs/operating-a-job/configuring-tasks.html.md b/website/source/docs/operating-a-job/configuring-tasks.html.md index 7d6f69af91b..3023385d9b7 100644 --- a/website/source/docs/operating-a-job/configuring-tasks.html.md +++ b/website/source/docs/operating-a-job/configuring-tasks.html.md @@ -70,7 +70,7 @@ Nomad has many [drivers](/docs/drivers/index.html), and most support passing arguments to their tasks via the `args` parameter. This option also optionally accepts [Nomad interpolation](/docs/runtime/interpolation.html). For example, if you wanted Nomad to dynamically allocate a high port to bind the service on -intead of relying on a static port for the previous job: +instead of relying on a static port for the previous job: ```hcl job "docs" { From 77d9b417c1a18320f4cd7606c47700fbac3f2199 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 May 2017 17:37:09 -0700 Subject: [PATCH 25/58] Skip https health check if verify_https_client is true --- command/agent/agent.go | 89 ++++++++++++++++------------------- command/agent/agent_test.go | 94 +++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 49 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 967740ce08b..d09ee4147b9 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -351,43 +351,22 @@ func (a *Agent) setupServer() error { a.server = server // Consul check addresses default to bind but can be toggled to use advertise - httpCheckAddr := a.config.normalizedAddrs.HTTP rpcCheckAddr := a.config.normalizedAddrs.RPC serfCheckAddr := a.config.normalizedAddrs.Serf if *a.config.Consul.ChecksUseAdvertise { - httpCheckAddr = a.config.AdvertiseAddrs.HTTP rpcCheckAddr = a.config.AdvertiseAddrs.RPC serfCheckAddr = a.config.AdvertiseAddrs.Serf } // Create the Nomad Server services for Consul - // TODO re-introduce HTTP/S checks when Consul 0.7.1 comes out if *a.config.Consul.AutoAdvertise { httpServ := &structs.Service{ Name: a.config.Consul.ServerServiceName, PortLabel: a.config.AdvertiseAddrs.HTTP, Tags: []string{consul.ServiceTagHTTP}, - Checks: []*structs.ServiceCheck{ - &structs.ServiceCheck{ - Name: "Nomad Server HTTP Check", - Type: "http", - Path: "/v1/status/peers", - Protocol: "http", - Interval: serverHttpCheckInterval, - Timeout: serverHttpCheckTimeout, - PortLabel: httpCheckAddr, - }, - }, } - if conf.TLSConfig.EnableHTTP { - if a.consulSupportsTLSSkipVerify { - httpServ.Checks[0].Protocol = "https" - httpServ.Checks[0].TLSSkipVerify = true - } else { - // No TLSSkipVerify support, don't register https check - a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2") - httpServ.Checks = []*structs.ServiceCheck{} - } + if check := a.agentHTTPCheck(); check != nil { + httpServ.Checks = []*structs.ServiceCheck{check} } rpcServ := &structs.Service{ Name: a.config.Consul.ServerServiceName, @@ -482,39 +461,15 @@ func (a *Agent) setupClient() error { } a.client = client - // Resolve the http check address - httpCheckAddr := a.config.normalizedAddrs.HTTP - if *a.config.Consul.ChecksUseAdvertise { - httpCheckAddr = a.config.AdvertiseAddrs.HTTP - } - // Create the Nomad Client services for Consul if *a.config.Consul.AutoAdvertise { httpServ := &structs.Service{ Name: a.config.Consul.ClientServiceName, PortLabel: a.config.AdvertiseAddrs.HTTP, Tags: []string{consul.ServiceTagHTTP}, - Checks: []*structs.ServiceCheck{ - &structs.ServiceCheck{ - Name: "Nomad Client HTTP Check", - Type: "http", - Path: "/v1/agent/servers", - Protocol: "http", - Interval: clientHttpCheckInterval, - Timeout: clientHttpCheckTimeout, - PortLabel: httpCheckAddr, - }, - }, } - if conf.TLSConfig.EnableHTTP { - if a.consulSupportsTLSSkipVerify { - httpServ.Checks[0].Protocol = "https" - httpServ.Checks[0].TLSSkipVerify = true - } else { - // No TLSSkipVerify support, don't register https check - a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2") - httpServ.Checks = []*structs.ServiceCheck{} - } + if check := a.agentHTTPCheck(); check != nil { + httpServ.Checks = []*structs.ServiceCheck{check} } if err := a.consulService.RegisterAgent(consulRoleClient, []*structs.Service{httpServ}); err != nil { return err @@ -524,6 +479,42 @@ func (a *Agent) setupClient() error { return nil } +// agentHTTPCheck returns a health check for the agent's HTTP API if possible. +// If no HTTP health check can be supported nil is returned. +func (a *Agent) agentHTTPCheck() *structs.ServiceCheck { + // Resolve the http check address + httpCheckAddr := a.config.normalizedAddrs.HTTP + if *a.config.Consul.ChecksUseAdvertise { + httpCheckAddr = a.config.AdvertiseAddrs.HTTP + } + check := structs.ServiceCheck{ + Name: "Nomad Client HTTP Check", + Type: "http", + Path: "/v1/agent/servers", + Protocol: "http", + Interval: clientHttpCheckInterval, + Timeout: clientHttpCheckTimeout, + PortLabel: httpCheckAddr, + } + if !a.config.TLSConfig.EnableHTTP { + // No HTTPS, return a plain http check + return &check + } + if !a.consulSupportsTLSSkipVerify { + a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2") + return nil + } + if a.config.TLSConfig.VerifyHTTPSClient { + a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because verify_https_client enabled") + return nil + } + + // HTTPS enabled; skip verification + check.Protocol = "https" + check.TLSSkipVerify = true + return &check +} + // reservePortsForClient reserves a range of ports for the client to use when // it creates various plugins for log collection, executors, drivers, etc func (a *Agent) reservePortsForClient(conf *clientconfig.Config) error { diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 24d45ee8d1e..c1803d0711a 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -4,12 +4,14 @@ import ( "encoding/json" "fmt" "io/ioutil" + "log" "net" "os" "strings" "testing" "time" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad" sconfig "github.com/hashicorp/nomad/nomad/structs/config" ) @@ -360,6 +362,98 @@ func TestAgent_ClientConfig(t *testing.T) { } } +// TestAgent_HTTPCheck asserts Agent.agentHTTPCheck properly alters the HTTP +// API health check depending on configuration. +func TestAgent_HTTPCheck(t *testing.T) { + logger := log.New(ioutil.Discard, "", 0) + if testing.Verbose() { + logger = log.New(os.Stdout, "[TestAgent_HTTPCheck] ", log.Lshortfile) + } + agent := func() *Agent { + return &Agent{ + logger: logger, + config: &Config{ + AdvertiseAddrs: &AdvertiseAddrs{HTTP: "advertise:4646"}, + normalizedAddrs: &Addresses{HTTP: "normalized:4646"}, + Consul: &sconfig.ConsulConfig{ + ChecksUseAdvertise: helper.BoolToPtr(false), + }, + TLSConfig: &sconfig.TLSConfig{EnableHTTP: false}, + }, + } + } + + t.Run("Plain HTTP Check", func(t *testing.T) { + a := agent() + check := a.agentHTTPCheck() + if check == nil { + t.Fatalf("expected non-nil check") + } + if check.Type != "http" { + t.Errorf("expected http check not: %q", check.Type) + } + if expected := "/v1/agent/servers"; check.Path != expected { + t.Errorf("expected %q path not: %q", expected, check.Path) + } + if check.Protocol != "http" { + t.Errorf("expected http proto not: %q", check.Protocol) + } + if expected := a.config.normalizedAddrs.HTTP; check.PortLabel != expected { + t.Errorf("expected normalized addr not %q", check.PortLabel) + } + }) + + t.Run("Plain HTTP + ChecksUseAdvertise", func(t *testing.T) { + a := agent() + a.config.Consul.ChecksUseAdvertise = helper.BoolToPtr(true) + check := a.agentHTTPCheck() + if check == nil { + t.Fatalf("expected non-nil check") + } + if expected := a.config.AdvertiseAddrs.HTTP; check.PortLabel != expected { + t.Errorf("expected advertise addr not %q", check.PortLabel) + } + }) + + t.Run("HTTPS + consulSupportsTLSSkipVerify", func(t *testing.T) { + a := agent() + a.consulSupportsTLSSkipVerify = true + a.config.TLSConfig.EnableHTTP = true + + check := a.agentHTTPCheck() + if check == nil { + t.Fatalf("expected non-nil check") + } + if !check.TLSSkipVerify { + t.Errorf("expected tls skip verify") + } + if check.Protocol != "https" { + t.Errorf("expected https not: %q", check.Protocol) + } + }) + + t.Run("HTTPS w/o TLSSkipVerify", func(t *testing.T) { + a := agent() + a.consulSupportsTLSSkipVerify = false + a.config.TLSConfig.EnableHTTP = true + + if check := a.agentHTTPCheck(); check != nil { + t.Fatalf("expected nil check not: %#v", check) + } + }) + + t.Run("HTTPS + VerifyHTTPSClient", func(t *testing.T) { + a := agent() + a.consulSupportsTLSSkipVerify = true + a.config.TLSConfig.EnableHTTP = true + a.config.TLSConfig.VerifyHTTPSClient = true + + if check := a.agentHTTPCheck(); check != nil { + t.Fatalf("expected nil check not: %#v", check) + } + }) +} + func TestAgent_ConsulSupportsTLSSkipVerify(t *testing.T) { assertSupport := func(expected bool, blob string) { self := map[string]map[string]interface{}{} From 9a4f8a6de6cdf1b19d21f7aac85dac57330dd0c9 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 3 May 2017 12:40:27 -0700 Subject: [PATCH 26/58] better change_mode explanation for templates --- website/source/docs/job-specification/template.html.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/website/source/docs/job-specification/template.html.md b/website/source/docs/job-specification/template.html.md index 0c3588a4915..1a14c8e8047 100644 --- a/website/source/docs/job-specification/template.html.md +++ b/website/source/docs/job-specification/template.html.md @@ -49,7 +49,9 @@ README][ct]. ## `template` Parameters - `change_mode` `(string: "restart")` - Specifies the behavior Nomad should take - if the rendered template changes. The possible values are: + if the rendered template changes. Nomad will always write the new contents of + the template to the specified destination. The possible values below describe + Nomad's action after writing the template to disk. - `"noop"` - take no action (continue running the task) - `"restart"` - restart the task From cf1b298f72b47c72235e4b291b07a014e9378c30 Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Wed, 8 Mar 2017 17:57:38 -0800 Subject: [PATCH 27/58] starting the jelly bean trail for two new env vars --- client/driver/env/env.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/client/driver/env/env.go b/client/driver/env/env.go index 845453b0094..d2a6372563b 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -48,6 +48,12 @@ const ( // AllocIndex is the environment variable for passing the allocation index. AllocIndex = "NOMAD_ALLOC_INDEX" + // Datacenter is the environment variable for passing the datacenter in which the alloc is running. + Datacenter = "NOMAD_DC" + + // Region is the environment variable for passing the region in which the alloc is running. + Region = "NOMAD_REGION" + // AddrPrefix is the prefix for passing both dynamic and static port // allocations to tasks. // E.g $NOMAD_ADDR_http=127.0.0.1:80 @@ -94,6 +100,8 @@ type TaskEnvironment struct { MemLimit int TaskName string AllocIndex int + Datacenter string + Region string AllocId string AllocName string Node *structs.Node From aeec7b35910ee978508ed2230b9dfad1348271e9 Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Wed, 29 Mar 2017 15:21:21 -0700 Subject: [PATCH 28/58] add dc and region to task env building function. flipped order of a couple functions to be consistent with the ordering of the rest of that section. --- client/driver/env/env.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/client/driver/env/env.go b/client/driver/env/env.go index d2a6372563b..08f8968e9bb 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -203,6 +203,12 @@ func (t *TaskEnvironment) Build() *TaskEnvironment { if t.JobName != "" { t.TaskEnv[JobName] = t.JobName } + if t.Datacenter != "" { + t.TaskEnv[Datacenter] = t.Datacenter + } + if t.Region != "" { + t.TaskEnv[Region] = t.Region + } // Build the addr of the other tasks if t.Alloc != nil { @@ -496,13 +502,13 @@ func (t *TaskEnvironment) SetTaskName(name string) *TaskEnvironment { return t } -func (t *TaskEnvironment) SetJobName(name string) *TaskEnvironment { - t.JobName = name +func (t *TaskEnvironment) ClearTaskName() *TaskEnvironment { + t.TaskName = "" return t } -func (t *TaskEnvironment) ClearTaskName() *TaskEnvironment { - t.TaskName = "" +func (t *TaskEnvironment) SetJobName(name string) *TaskEnvironment { + t.JobName = name return t } From a5c4663efbeb496de246f7a01872de1d5b93333a Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Wed, 29 Mar 2017 16:20:10 -0700 Subject: [PATCH 29/58] add helper functions to set and clear dc, region names. --- client/driver/env/env.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/client/driver/env/env.go b/client/driver/env/env.go index 08f8968e9bb..90b7d2cfe65 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -517,6 +517,26 @@ func (t *TaskEnvironment) ClearJobName() *TaskEnvironment { return t } +func (t *TaskEnvironment) SetDatacenterName(name string) *TaskEnvironment { + t.JobName = name + return t +} + +func (t *TaskEnvironment) ClearDatacenterName() *TaskEnvironment { + t.JobName = "" + return t +} + +func (t *TaskEnvironment) SetRegionName(name string) *TaskEnvironment { + t.JobName = name + return t +} + +func (t *TaskEnvironment) ClearRegionName() *TaskEnvironment { + t.JobName = "" + return t +} + func (t *TaskEnvironment) SetVaultToken(token string, inject bool) *TaskEnvironment { t.VaultToken = token t.InjectVaultToken = inject From d10bfb46c19e846939479ca672fd94044f0dc068 Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Wed, 29 Mar 2017 16:27:08 -0700 Subject: [PATCH 30/58] just found out dc and region are going to be a bit different, as Region didn't previously exist in the Node struct. --- client/driver/driver.go | 2 ++ nomad/structs/structs.go | 3 +++ 2 files changed, 5 insertions(+) diff --git a/client/driver/driver.go b/client/driver/driver.go index ee28888cfad..d48fb007a67 100644 --- a/client/driver/driver.go +++ b/client/driver/driver.go @@ -295,6 +295,8 @@ func GetTaskEnv(taskDir *allocdir.TaskDir, node *structs.Node, SetJobName(alloc.Job.Name). SetEnvvars(task.Env). SetTaskName(task.Name) + SetDatacenterName(node.Datacenter) + SetRegionName(node.Region) // Vary paths by filesystem isolation used drv, err := NewDriver(task.Driver, NewEmptyDriverContext()) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 924c39009e0..357c953327d 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -735,6 +735,9 @@ type Node struct { // Datacenter for this node Datacenter string + // Region for this node + Region string + // Node name Name string From 530cf0317766f9b75bbe909b0b69e7091a4d2ced Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Wed, 29 Mar 2017 16:31:23 -0700 Subject: [PATCH 31/58] allow region to also be interpolated, like datacenter. --- client/driver/env/env.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/client/driver/env/env.go b/client/driver/env/env.go index 90b7d2cfe65..a8ccca2497d 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -78,10 +78,11 @@ const ( // The node values that can be interpreted. const ( - nodeIdKey = "node.unique.id" - nodeDcKey = "node.datacenter" - nodeNameKey = "node.unique.name" - nodeClassKey = "node.class" + nodeIdKey = "node.unique.id" + nodeDcKey = "node.datacenter" + nodeRegionKey = "node.region" + nodeNameKey = "node.unique.name" + nodeClassKey = "node.class" // Prefixes used for lookups. nodeAttributePrefix = "attr." @@ -241,6 +242,7 @@ func (t *TaskEnvironment) Build() *TaskEnvironment { // Set up the node values. t.NodeValues[nodeIdKey] = t.Node.ID t.NodeValues[nodeDcKey] = t.Node.Datacenter + t.NodeValues[nodeRegionKey] = t.Node.Region t.NodeValues[nodeNameKey] = t.Node.Name t.NodeValues[nodeClassKey] = t.Node.NodeClass From 4a4417d556fa413ff7c7e53cfc98d0a075452b2e Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Thu, 30 Mar 2017 14:15:06 -0700 Subject: [PATCH 32/58] I think I did it. --- client/driver/driver.go | 4 ++-- client/driver/env/env.go | 8 ++++---- command/agent/agent.go | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/client/driver/driver.go b/client/driver/driver.go index d48fb007a67..8c03284a7c6 100644 --- a/client/driver/driver.go +++ b/client/driver/driver.go @@ -293,10 +293,10 @@ func GetTaskEnv(taskDir *allocdir.TaskDir, node *structs.Node, env := env.NewTaskEnvironment(node). SetTaskMeta(alloc.Job.CombinedTaskMeta(alloc.TaskGroup, task.Name)). SetJobName(alloc.Job.Name). + SetDatacenterName(node.Datacenter). + SetRegionName(node.Region). SetEnvvars(task.Env). SetTaskName(task.Name) - SetDatacenterName(node.Datacenter) - SetRegionName(node.Region) // Vary paths by filesystem isolation used drv, err := NewDriver(task.Driver, NewEmptyDriverContext()) diff --git a/client/driver/env/env.go b/client/driver/env/env.go index a8ccca2497d..f0c12fc591f 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -520,22 +520,22 @@ func (t *TaskEnvironment) ClearJobName() *TaskEnvironment { } func (t *TaskEnvironment) SetDatacenterName(name string) *TaskEnvironment { - t.JobName = name + t.Datacenter = name return t } func (t *TaskEnvironment) ClearDatacenterName() *TaskEnvironment { - t.JobName = "" + t.Datacenter = "" return t } func (t *TaskEnvironment) SetRegionName(name string) *TaskEnvironment { - t.JobName = name + t.Region = name return t } func (t *TaskEnvironment) ClearRegionName() *TaskEnvironment { - t.JobName = "" + t.Region = "" return t } diff --git a/command/agent/agent.go b/command/agent/agent.go index 8080591e694..6c962067cba 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -280,6 +280,7 @@ func (a *Agent) clientConfig() (*clientconfig.Config, error) { // Setup the node conf.Node = new(structs.Node) conf.Node.Datacenter = a.config.Datacenter + conf.Node.Region = a.config.Region conf.Node.Name = a.config.NodeName conf.Node.Meta = a.config.Client.Meta conf.Node.NodeClass = a.config.Client.NodeClass From 9eac667642097c987265f418067ebac1fc16053e Mon Sep 17 00:00:00 2001 From: Kate Taggart Date: Thu, 30 Mar 2017 15:43:39 -0700 Subject: [PATCH 33/58] mention the two newly available env vars in the docs. --- website/source/docs/runtime/environment.html.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/website/source/docs/runtime/environment.html.md b/website/source/docs/runtime/environment.html.md index 1b62d7af98f..b11d075d6df 100644 --- a/website/source/docs/runtime/environment.html.md +++ b/website/source/docs/runtime/environment.html.md @@ -60,6 +60,14 @@ environment variables. `NOMAD_JOB_NAME` The job's name + + `NOMAD_DC` + The datacenter in which the job is running. + + + `NOMAD_REGION` + The region in which the job is running. + `NOMAD_IP_