Add 'body' field to service's 'check' stanza

[nick96]

This adds a body field to the service check stanza.

As described in #10084 Consul has this field so it's just a matter of parsing it out of the job config and plumbing it through.

Currently there is no validation that the method and body make sense together (e.g.method can be "GET" and the body field can exist). I couldn't find this validation in the Consul codebase either.

I'm also still having a look around the code to see where different bits are tested.

New jobspec Entry Checklist

Code

• Consider similar features in Consul, Kubernetes, and other tools. Is there prior art we should match? Terminology, structure, etc? (adding something already in Consul)
• Add structs/fields to api/ package
  • api/ structs usually have Canonicalize and Copy methods
  • New fields should be added to existing Canonicalize, Copy methods
  • Test the structs/fields via methods mentioned above
• Add structs/fields to nomad/structs package
  • structs/ structs usually have Copy, Equals, and Validate methods
  • Validation happens in this package and must be implemented
  • Note that analogous struct field names should match with api/ package
  • Test the structs/fields via methods mentioned above
  • Implement and test other logical methods
• Add conversion between api/ and nomad/structs in command/agent/job_endpoint.go
  • Add test for conversion
• Implement diff logic for new structs/fields in nomad/structs/diff.go - This seems to just be handled by the generic diff function because body is a primitive
  • Note that fields must be listed in alphabetical order in FieldDiff slices in nomad/structs/diff_test.go
  • Add test for diff of new structs/fields
• Add change detection for new structs/feilds in scheduler/util.go/tasksUpdated - I think this is already handle in the equality check
  • Might be covered by .Equals but might not be, check.
  • Should return true if the task must be replaced as a result of the change.

HCL1 (deprecated)

New jobspec entries should only be added to jobspec2. It makes use of HCL2
and the api package for automatic parsing. Before, additional parsing was
required in the original jobspec package.

• Parse in jobspec/parse.go (HCL1 only)
• Test in jobspec/parse_test.go (preferably with a
  jobspec/text-fixtures/<feature>.hcl test file) (HCL1 only)

Docs

• Changelog
• Jobspec entry https://www.nomadproject.io/docs/job-specification/index.html
• Jobspec sidebar entry https://github.com/hashicorp/nomad/blob/main/website/data/docs-navigation.js
• Job JSON API entry https://www.nomadproject.io/api/json-jobs.html
• Sample Response output in API https://www.nomadproject.io/api/jobs.html
• Consider if it needs a guide https://www.nomadproject.io/guides/index.html

[hashicorp-cla]

All committers have signed the CLA.

[nick96]

I've manually tested this using this job: https://gist.github.com/nick96/02b29844856de7ecafa9e1c2e8bc7c09

The CLI returns the expected health checks, including the Body field

$ ./bin/nomad inspect whoami | jq '.Job.TaskGroups[0].Services[0].Checks[]'
{
  "AddressMode": "",
  "Args": null,
  "Body": "{}",
  "CheckRestart": null,
  "Command": "",
  "Expose": false,
  "FailuresBeforeCritical": 0,
  "GRPCService": "",
  "GRPCUseTLS": false,
  "Header": null,
  "Id": "",
  "InitialStatus": "",
  "Interval": 5000000000,
  "Method": "POST",
  "Name": "service: \"whoami\" check",
  "OnUpdate": "require_healthy",
  "Path": "/",
  "PortLabel": "",
  "Protocol": "",
  "SuccessBeforePassing": 0,
  "TLSSkipVerify": false,
  "TaskName": "",
  "Timeout": 5000000000,
  "Type": "http"
}
{
  "AddressMode": "",
  "Args": null,
  "Body": "",
  "CheckRestart": null,
  "Command": "",
  "Expose": false,
  "FailuresBeforeCritical": 0,
  "GRPCService": "",
  "GRPCUseTLS": false,
  "Header": null,
  "Id": "",
  "InitialStatus": "",
  "Interval": 5000000000,
  "Method": "GET",
  "Name": "service: \"whoami\" check",
  "OnUpdate": "require_healthy",
  "Path": "/health",
  "PortLabel": "",
  "Protocol": "",
  "SuccessBeforePassing": 0,
  "TLSSkipVerify": false,
  "TaskName": "",
  "Timeout": 5000000000,
  "Type": "http"
}

API result is the same, except it doesn't contain the the Id field:

$ curl http://localhost:4646/v1/job/whoami | jq -S '.TaskGroups[0].Services[0].Checks[]'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3484    0  3484    0     0  1134k      0 --:--:-- --:--:-- --:--:-- 1134k
{
  "AddressMode": "",
  "Args": null,
  "Body": "{}",
  "CheckRestart": null,
  "Command": "",
  "Expose": false,
  "FailuresBeforeCritical": 0,
  "GRPCService": "",
  "GRPCUseTLS": false,
  "Header": null,
  "InitialStatus": "",
  "Interval": 5000000000,
  "Method": "POST",
  "Name": "service: \"whoami\" check",
  "OnUpdate": "require_healthy",
  "Path": "/",
  "PortLabel": "",
  "Protocol": "",
  "SuccessBeforePassing": 0,
  "TLSSkipVerify": false,
  "TaskName": "",
  "Timeout": 5000000000,
  "Type": "http"
}
{
  "AddressMode": "",
  "Args": null,
  "Body": "",
  "CheckRestart": null,
  "Command": "",
  "Expose": false,
  "FailuresBeforeCritical": 0,
  "GRPCService": "",
  "GRPCUseTLS": false,
  "Header": null,
  "InitialStatus": "",
  "Interval": 5000000000,
  "Method": "GET",
  "Name": "service: \"whoami\" check",
  "OnUpdate": "require_healthy",
  "Path": "/health",
  "PortLabel": "",
  "Protocol": "",
  "SuccessBeforePassing": 0,
  "TLSSkipVerify": false,
  "TaskName": "",
  "Timeout": 5000000000,
  "Type": "http"
}

The health check is successfully registered in Consul:

[nick96]

Just playing around with this if I change the body and run ./bin/nomad run whoami.hcl again it doesn't update the body field but it works for all other fields.

[tgross]

Hi @nick96! This is looking great so far!

[nick96]

@tgross I think I've fixed all your comments. PTAL when you get the chance

[tgross]

Thanks @nick96. I'm in a bit of crunch today but will try to give this another pass by end-of-week.

[tgross]

Hi @nick96 sorry about the delay on this review. I just wrote the following job spec file and tested it end-to-end and it looks good (including updates):

echo server jobspec

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

  group "web" {

    network {
      mode = "bridge"
      port "www" {
        to = 8000
      }
    }

    service {
      port = "www"

      check {
        type = "http"
        port = "www"
        method = "POST"
        path = "/"
        # body = "check, please"
        body = "make sure updates work, please"
        interval = "10s"
        timeout = "2s"
      }
    }

    task "httpd" {
      driver = "docker"

      config {
        image   = "python:3.8-alpine"
        command = "python"
        args    = ["/local/job.py"]
        ports   = ["www"]
      }

      template {
        data        = <<EOT
import http.server

class Handler(http.server.BaseHTTPRequestHandler):

    def do_GET(self):
        self.send_response(200)
        self.send_header('Content-type', 'text/html')
        self.end_headers()

    def do_POST(self):
        content_length = int(self.headers['Content-Length'])
        post_data = self.rfile.read(content_length)
        post_data.decode('utf-8')
        print(post_data)
        self.send_response(200)
        self.send_header('Content-type', 'text/html')
        self.end_headers()
        self.wfile.write("{}\n".format(post_data).encode('utf-8'))

def run(server_class=http.server.HTTPServer, handler_class=Handler):
    server_address = ('', 8000)
    httpd = server_class(server_address, handler_class)
    httpd.serve_forever()

if __name__ == '__main__':
    run()

EOT
        destination = "local/job.py"
      }

      resources {
        cpu    = 128
        memory = 128
      }
    }
  }
}

I'm going to take one more pass over this tomorrow just for style or testing issues but I think we should be good to merge shortly. There's currently a merge conflict but I can clean that up real quick. Thanks for your patience!

[nick96]

No worries! I'm away for the rest of this week so won't be able to address anything but will get onto it ASAP when I'm back.

[tgross]

Merged! Thanks again @nick96!

[github-actions]

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