From 3aadee86741968cf92fff7fa7b969d7b40864f23 Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Tue, 5 Jan 2021 01:07:35 +0000 Subject: [PATCH 01/36] Allow setting of headers in api client --- api/api.go | 9 +++++++++ api/api_test.go | 16 ++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/api/api.go b/api/api.go index e54d37bb983..f0d4022b86b 100644 --- a/api/api.go +++ b/api/api.go @@ -155,6 +155,8 @@ type Config struct { // // TLSConfig is ignored if HttpClient is set. TLSConfig *TLSConfig + + Headers map[string]string } // ClientConfig copies the configuration with a new client address, region, and @@ -527,6 +529,7 @@ type request struct { body io.Reader obj interface{} ctx context.Context + header http.Header } // setQueryOptions is used to annotate the request with @@ -621,6 +624,7 @@ func (r *request) toHTTP() (*http.Request, error) { req.SetBasicAuth(r.config.HttpAuth.Username, r.config.HttpAuth.Password) } + req.Header = r.header req.Header.Add("Accept-Encoding", "gzip") if r.token != "" { req.Header.Set("X-Nomad-Token", r.token) @@ -649,6 +653,7 @@ func (c *Client) newRequest(method, path string) (*request, error) { Path: u.Path, RawPath: u.RawPath, }, + header: make(http.Header), params: make(map[string][]string), } if c.config.Region != "" { @@ -671,6 +676,10 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } + for key, value := range c.config.Headers { + r.header.Set(key, value) + } + return r, nil } diff --git a/api/api_test.go b/api/api_test.go index d460987963a..e2dcd632106 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -341,6 +341,22 @@ func TestParseWriteMeta(t *testing.T) { } } +func TestClientHeader(t *testing.T) { + t.Parallel() + c, s := makeClient(t, func(c *Config) { + c.Headers = map[string]string{ + "Hello": "World", + } + }, nil) + defer s.Stop() + + r, _ := c.newRequest("GET", "/v1/jobs") + + if r.header.Get("Hello") != "World" { + t.Fatalf("bad: %v", r.header) + } +} + func TestQueryString(t *testing.T) { t.Parallel() c, s := makeClient(t, nil, nil) From 679864ea05c53f96474e5c394acf10f7cfc2b90c Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Tue, 5 Jan 2021 15:43:24 +0000 Subject: [PATCH 02/36] Prefer http.Header over map[string]string to allow for multi-valued headers --- api/api.go | 6 ++---- api/api_test.go | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/api/api.go b/api/api.go index f0d4022b86b..0efb10bbb6e 100644 --- a/api/api.go +++ b/api/api.go @@ -156,7 +156,7 @@ type Config struct { // TLSConfig is ignored if HttpClient is set. TLSConfig *TLSConfig - Headers map[string]string + Headers http.Header } // ClientConfig copies the configuration with a new client address, region, and @@ -676,9 +676,7 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } - for key, value := range c.config.Headers { - r.header.Set(key, value) - } + r.header = c.config.Headers return r, nil } diff --git a/api/api_test.go b/api/api_test.go index e2dcd632106..0048ecdfe67 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -344,8 +344,8 @@ func TestParseWriteMeta(t *testing.T) { func TestClientHeader(t *testing.T) { t.Parallel() c, s := makeClient(t, func(c *Config) { - c.Headers = map[string]string{ - "Hello": "World", + c.Headers = http.Header{ + "Hello": []string{"World"}, } }, nil) defer s.Stop() From c7cc71e2e058da5ab5b161f7e48045c9bb1c2d08 Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Tue, 5 Jan 2021 15:49:03 +0000 Subject: [PATCH 03/36] Headers -> Header --- api/api.go | 4 ++-- api/api_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/api.go b/api/api.go index 0efb10bbb6e..0e96043b8ea 100644 --- a/api/api.go +++ b/api/api.go @@ -156,7 +156,7 @@ type Config struct { // TLSConfig is ignored if HttpClient is set. TLSConfig *TLSConfig - Headers http.Header + Header http.Header } // ClientConfig copies the configuration with a new client address, region, and @@ -676,7 +676,7 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } - r.header = c.config.Headers + r.header = c.config.Header return r, nil } diff --git a/api/api_test.go b/api/api_test.go index 0048ecdfe67..ccfc2b38e28 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -344,7 +344,7 @@ func TestParseWriteMeta(t *testing.T) { func TestClientHeader(t *testing.T) { t.Parallel() c, s := makeClient(t, func(c *Config) { - c.Headers = http.Header{ + c.Header = http.Header{ "Hello": []string{"World"}, } }, nil) From b2724ba64dec69fb1b0d5ef746d9d6a7a2ca10b9 Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Tue, 5 Jan 2021 16:02:59 +0000 Subject: [PATCH 04/36] Ensure set headers have lower precedence than basic auth headers --- api/api.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/api.go b/api/api.go index 0e96043b8ea..d037d3bc25c 100644 --- a/api/api.go +++ b/api/api.go @@ -615,6 +615,8 @@ func (r *request) toHTTP() (*http.Request, error) { return nil, err } + req.Header = r.header + // Optionally configure HTTP basic authentication if r.url.User != nil { username := r.url.User.Username() @@ -624,7 +626,6 @@ func (r *request) toHTTP() (*http.Request, error) { req.SetBasicAuth(r.config.HttpAuth.Username, r.config.HttpAuth.Password) } - req.Header = r.header req.Header.Add("Accept-Encoding", "gzip") if r.token != "" { req.Header.Set("X-Nomad-Token", r.token) From 214f4ad4d323ecdeba0bbd5bbe32bb7115b2012a Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Tue, 5 Jan 2021 16:03:16 +0000 Subject: [PATCH 05/36] Only override headers if they're set --- api/api.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/api.go b/api/api.go index d037d3bc25c..ed433398429 100644 --- a/api/api.go +++ b/api/api.go @@ -677,7 +677,9 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } - r.header = c.config.Header + if c.config.Header != nil { + r.header = c.config.Header + } return r, nil } From e18eac3ff4c759fa302334babcdd1305ee4962b7 Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Tue, 5 Jan 2021 16:15:38 +0000 Subject: [PATCH 06/36] make sync --- vendor/github.com/hashicorp/nomad/api/api.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/vendor/github.com/hashicorp/nomad/api/api.go b/vendor/github.com/hashicorp/nomad/api/api.go index e54d37bb983..ed433398429 100644 --- a/vendor/github.com/hashicorp/nomad/api/api.go +++ b/vendor/github.com/hashicorp/nomad/api/api.go @@ -155,6 +155,8 @@ type Config struct { // // TLSConfig is ignored if HttpClient is set. TLSConfig *TLSConfig + + Header http.Header } // ClientConfig copies the configuration with a new client address, region, and @@ -527,6 +529,7 @@ type request struct { body io.Reader obj interface{} ctx context.Context + header http.Header } // setQueryOptions is used to annotate the request with @@ -612,6 +615,8 @@ func (r *request) toHTTP() (*http.Request, error) { return nil, err } + req.Header = r.header + // Optionally configure HTTP basic authentication if r.url.User != nil { username := r.url.User.Username() @@ -649,6 +654,7 @@ func (c *Client) newRequest(method, path string) (*request, error) { Path: u.Path, RawPath: u.RawPath, }, + header: make(http.Header), params: make(map[string][]string), } if c.config.Region != "" { @@ -671,6 +677,10 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } + if c.config.Header != nil { + r.header = c.config.Header + } + return r, nil } From 79a4895ad7d9f794fae7a86fd3073de4c0081398 Mon Sep 17 00:00:00 2001 From: Conor Mongey Date: Wed, 6 Jan 2021 01:39:18 +0000 Subject: [PATCH 07/36] Revert "Headers -> Header" This reverts commit 71396fa721945e55f51bc90ed02522936450209b. --- api/api.go | 6 +++--- api/api_test.go | 2 +- vendor/github.com/hashicorp/nomad/api/api.go | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/api.go b/api/api.go index ed433398429..bb553b64576 100644 --- a/api/api.go +++ b/api/api.go @@ -156,7 +156,7 @@ type Config struct { // TLSConfig is ignored if HttpClient is set. TLSConfig *TLSConfig - Header http.Header + Headers http.Header } // ClientConfig copies the configuration with a new client address, region, and @@ -677,8 +677,8 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } - if c.config.Header != nil { - r.header = c.config.Header + if c.config.Headers != nil { + r.header = c.config.Headers } return r, nil diff --git a/api/api_test.go b/api/api_test.go index ccfc2b38e28..0048ecdfe67 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -344,7 +344,7 @@ func TestParseWriteMeta(t *testing.T) { func TestClientHeader(t *testing.T) { t.Parallel() c, s := makeClient(t, func(c *Config) { - c.Header = http.Header{ + c.Headers = http.Header{ "Hello": []string{"World"}, } }, nil) diff --git a/vendor/github.com/hashicorp/nomad/api/api.go b/vendor/github.com/hashicorp/nomad/api/api.go index ed433398429..bb553b64576 100644 --- a/vendor/github.com/hashicorp/nomad/api/api.go +++ b/vendor/github.com/hashicorp/nomad/api/api.go @@ -156,7 +156,7 @@ type Config struct { // TLSConfig is ignored if HttpClient is set. TLSConfig *TLSConfig - Header http.Header + Headers http.Header } // ClientConfig copies the configuration with a new client address, region, and @@ -677,8 +677,8 @@ func (c *Client) newRequest(method, path string) (*request, error) { } } - if c.config.Header != nil { - r.header = c.config.Header + if c.config.Headers != nil { + r.header = c.config.Headers } return r, nil From da74d8c54967bbae50fc8b7002c89fe56f46adab Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 12 Jan 2021 11:55:41 -0500 Subject: [PATCH 08/36] move config files to terraform --- .../packer/ubuntu-bionic-amd64.pkr.hcl | 5 --- .../packer/ubuntu-bionic-amd64/setup.sh | 1 - .../packer/windows-2016-amd64.pkr.hcl | 5 --- e2e/terraform/provision-nomad/main.tf | 34 +++++++++---------- 4 files changed, 16 insertions(+), 29 deletions(-) diff --git a/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl b/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl index 3712b8206a1..790c359f6b8 100644 --- a/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl +++ b/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl @@ -36,11 +36,6 @@ build { source = "./ubuntu-bionic-amd64" } - provisioner "file" { - destination = "/tmp/config" - source = "../config" - } - // cloud-init modifies the apt sources, so we need to wait // before running our setup provisioner "shell-local" { diff --git a/e2e/terraform/packer/ubuntu-bionic-amd64/setup.sh b/e2e/terraform/packer/ubuntu-bionic-amd64/setup.sh index a20ba5a88b7..f1157001d79 100755 --- a/e2e/terraform/packer/ubuntu-bionic-amd64/setup.sh +++ b/e2e/terraform/packer/ubuntu-bionic-amd64/setup.sh @@ -77,7 +77,6 @@ mkdir_for_root $NOMAD_PLUGIN_DIR sudo mv /tmp/linux/nomad.service /etc/systemd/system/nomad.service echo "Install Nomad" -sudo mv /tmp/config /opt/ sudo mv /tmp/linux/provision.sh /opt/provision.sh sudo chmod +x /opt/provision.sh /opt/provision.sh --nomad_version $NOMADVERSION --nostart diff --git a/e2e/terraform/packer/windows-2016-amd64.pkr.hcl b/e2e/terraform/packer/windows-2016-amd64.pkr.hcl index 942a9d1e6be..e9e66126d4a 100644 --- a/e2e/terraform/packer/windows-2016-amd64.pkr.hcl +++ b/e2e/terraform/packer/windows-2016-amd64.pkr.hcl @@ -37,11 +37,6 @@ build { ] } - provisioner "file" { - destination = "/opt" - source = "../config" - } - provisioner "file" { destination = "/opt/provision.ps1" source = "./windows-2016-amd64/provision.ps1" diff --git a/e2e/terraform/provision-nomad/main.tf b/e2e/terraform/provision-nomad/main.tf index a53a124fb5f..0bb9dae78ea 100644 --- a/e2e/terraform/provision-nomad/main.tf +++ b/e2e/terraform/provision-nomad/main.tf @@ -1,20 +1,14 @@ locals { provision_script = var.platform == "windows_amd64" ? "C:/opt/provision.ps1" : "/opt/provision.sh" - custom_path = dirname("${path.root}/config/custom/") - - custom_config_files = compact(setunion( - fileset(local.custom_path, "nomad/*.hcl"), - fileset(local.custom_path, "nomad/${var.role}/*.hcl"), - fileset(local.custom_path, "nomad/${var.role}/indexed/*${var.index}.hcl"), - fileset(local.custom_path, "consul/*.json"), - fileset(local.custom_path, "consul/${var.role}/*.json"), - fileset(local.custom_path, "consul${var.role}indexed/*${var.index}*.json"), - fileset(local.custom_path, "vault/*.hcl"), - fileset(local.custom_path, "vault${var.role}*.hcl"), - fileset(local.custom_path, "vault${var.role}indexed/*${var.index}.hcl"), + config_path = dirname("${path.root}/config/") + + config_files = compact(setunion( + fileset(local.config_path, "**"), )) + update_config_command = var.platform == "windows_amd64" ? "if (test-path /opt/config) { Remove-Item -Path /opt/config -Force -Recurse }; cp -r /tmp/config /opt/config" : "sudo rm -rf /opt/config; sudo mv /tmp/config /opt/config" + # abstract-away platform-specific parameter expectations _arg = var.platform == "windows_amd64" ? "-" : "--" } @@ -22,7 +16,7 @@ locals { resource "null_resource" "provision_nomad" { depends_on = [ - null_resource.upload_custom_configs, + null_resource.upload_configs, null_resource.upload_nomad_binary ] @@ -85,7 +79,7 @@ data "template_file" "arg_index" { resource "null_resource" "upload_nomad_binary" { count = var.nomad_local_binary != "" ? 1 : 0 - depends_on = [null_resource.upload_custom_configs] + depends_on = [null_resource.upload_configs] triggers = { nomad_binary_sha = filemd5(var.nomad_local_binary) } @@ -105,11 +99,10 @@ resource "null_resource" "upload_nomad_binary" { } } -resource "null_resource" "upload_custom_configs" { +resource "null_resource" "upload_configs" { - count = var.profile == "custom" ? 1 : 0 triggers = { - hashes = join(",", [for file in local.custom_config_files : filemd5("${local.custom_path}/${file}")]) + hashes = join(",", [for file in local.config_files : filemd5("${local.config_path}/${file}")]) } connection { @@ -122,7 +115,12 @@ resource "null_resource" "upload_custom_configs" { } provisioner "file" { - source = local.custom_path + source = local.config_path destination = "/tmp/" } + + provisioner "local-exec" { + command = "until ssh -o PasswordAuthentication=no -o KbdInteractiveAuthentication=no -o LogLevel=ERROR -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i ${var.connection.private_key} -p ${var.connection.port} ${var.connection.user}@${var.connection.host} '${local.update_config_command}'; do sleep 5; done" + } + } From 21f77f576dd643e9ed5b7964bbc3152a44092f4d Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 12 Jan 2021 13:30:30 -0500 Subject: [PATCH 09/36] change ami naming --- e2e/terraform/compute.tf | 8 ++++++-- e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl | 3 ++- e2e/terraform/packer/windows-2016-amd64.pkr.hcl | 7 +++++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/e2e/terraform/compute.tf b/e2e/terraform/compute.tf index ea5b0874bd5..c8f27212cc9 100644 --- a/e2e/terraform/compute.tf +++ b/e2e/terraform/compute.tf @@ -1,3 +1,7 @@ +locals { + ami_prefix = "nomad-e2e-v2" +} + resource "aws_instance" "server" { ami = data.aws_ami.ubuntu_bionic_amd64.image_id instance_type = var.instance_type @@ -60,7 +64,7 @@ data "aws_ami" "ubuntu_bionic_amd64" { filter { name = "name" - values = ["nomad-e2e-ubuntu-bionic-amd64-*"] + values = ["${local.ami_prefix}-ubuntu-bionic-amd64-*"] } filter { @@ -75,7 +79,7 @@ data "aws_ami" "windows_2016_amd64" { filter { name = "name" - values = ["nomad-e2e-windows-2016-amd64-*"] + values = ["${local.ami_prefix}-windows-2016-amd64-*"] } filter { diff --git a/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl b/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl index 790c359f6b8..14e5a140b51 100644 --- a/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl +++ b/e2e/terraform/packer/ubuntu-bionic-amd64.pkr.hcl @@ -1,10 +1,11 @@ locals { timestamp = regex_replace(timestamp(), "[- TZ:]", "") distro = "ubuntu-bionic-18.04-amd64-server-*" + version = "v2" } source "amazon-ebs" "latest_ubuntu_bionic" { - ami_name = "nomad-e2e-ubuntu-bionic-amd64-${local.timestamp}" + ami_name = "nomad-e2e-${local.version}-ubuntu-bionic-amd64-${local.timestamp}" iam_instance_profile = "packer_build" // defined in nomad-e2e repo instance_type = "t2.medium" region = "us-east-1" diff --git a/e2e/terraform/packer/windows-2016-amd64.pkr.hcl b/e2e/terraform/packer/windows-2016-amd64.pkr.hcl index e9e66126d4a..8e03fc787e8 100644 --- a/e2e/terraform/packer/windows-2016-amd64.pkr.hcl +++ b/e2e/terraform/packer/windows-2016-amd64.pkr.hcl @@ -1,7 +1,10 @@ -locals { timestamp = regex_replace(timestamp(), "[- TZ:]", "") } +locals { + timestamp = regex_replace(timestamp(), "[- TZ:]", "") + version = "v2" +} source "amazon-ebs" "latest_windows_2016" { - ami_name = "nomad-e2e-windows-2016-amd64-${local.timestamp}" + ami_name = "nomad-e2e-${local.version}-windows-2016-amd64-${local.timestamp}" communicator = "ssh" instance_type = "t2.medium" region = "us-east-1" From 9fdd9a5428f14a97f4fd4917dd750dbe4ccdaf7a Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 12 Jan 2021 13:15:33 -0500 Subject: [PATCH 10/36] set sha --- e2e/terraform/.terraform.lock.hcl | 17 +++++++++++++++++ e2e/terraform/compute.tf | 19 +++++++++++++++++++ .../packer/ubuntu-bionic-amd64.pkr.hcl | 10 ++++++++-- .../packer/windows-2016-amd64.pkr.hcl | 8 +++++++- 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/e2e/terraform/.terraform.lock.hcl b/e2e/terraform/.terraform.lock.hcl index cd48bf697a7..9b8e3c492dd 100755 --- a/e2e/terraform/.terraform.lock.hcl +++ b/e2e/terraform/.terraform.lock.hcl @@ -19,6 +19,23 @@ provider "registry.terraform.io/hashicorp/aws" { ] } +provider "registry.terraform.io/hashicorp/external" { + version = "2.0.0" + hashes = [ + "h1:6S7hqjmUnoAZ5D/0F1VlJZKSJsUIBh7Ro0tLjGpKO0g=", + "zh:07949780dd6a1d43e7b46950f6e6976581d9724102cb5388d3411a1b6f476bde", + "zh:0a4f4636ff93f0644affa8474465dd8c9252946437ad025b28fc9f6603534a24", + "zh:0dd7e05a974c649950d1a21d7015d3753324ae52ebdd1744b144bc409ca4b3e8", + "zh:2b881032b9aa9d227ac712f614056d050bcdcc67df0dc79e2b2cb76a197059ad", + "zh:38feb4787b4570335459ca75a55389df1a7570bdca8cdf5df4c2876afe3c14b4", + "zh:40f7e0aaef3b1f4c2ca2bb1189e3fe9af8c296da129423986d1d99ccc8cfb86c", + "zh:56b361f64f0f0df5c4f958ae2f0e6f8ba192f35b720b9d3ae1be068fabcf73d9", + "zh:5fadb5880cd31c2105f635ded92b9b16f918c1dd989627a4ce62c04939223909", + "zh:61fa0be9c14c8c4109cfb7be8d54a80c56d35dbae49d3231cddb59831e7e5a4d", + "zh:853774bf97fbc4a784d5af5a4ca0090848430781ae6cfc586adeb48f7c44af79", + ] +} + provider "registry.terraform.io/hashicorp/local" { version = "2.0.0" hashes = [ diff --git a/e2e/terraform/compute.tf b/e2e/terraform/compute.tf index c8f27212cc9..49d1c9bf6ce 100644 --- a/e2e/terraform/compute.tf +++ b/e2e/terraform/compute.tf @@ -58,6 +58,15 @@ resource "aws_instance" "client_windows_2016_amd64" { } } +data "external" "packer_sha" { + program = ["/bin/sh", "-c", < Date: Tue, 12 Jan 2021 13:32:49 -0500 Subject: [PATCH 11/36] add helper for building ami --- e2e/terraform/packer/build | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100755 e2e/terraform/packer/build diff --git a/e2e/terraform/packer/build b/e2e/terraform/packer/build new file mode 100755 index 00000000000..4b9bc947403 --- /dev/null +++ b/e2e/terraform/packer/build @@ -0,0 +1,35 @@ +#!/bin/sh + +set -e + +usage() { + cat < + +Build an AMI for the target configuration + +Examples + build ubuntu-bionic-amd64 + +EOF + + exit 2 +} + +if [[ $# -ne 1 ]]; then + usage +fi + +target=${1/%.pkr.hcl/} + +directory=$(dirname "$0") +cd ${directory} + +if ! test -f "${target}.pkr.hcl"; then + echo "${target}.pkr.hcl is not present" >&2 + exit 1 +fi + +sha=$(git log -n 1 --pretty=format:%H ${dirname}) +echo packer build --var "build_sha=${sha}" ${target}.pkr.hcl +packer build --var "build_sha=${sha}" ${target}.pkr.hcl From 1f71e9dae445b19189085586555632f72bad9567 Mon Sep 17 00:00:00 2001 From: zzhai Date: Mon, 25 Jan 2021 00:13:02 -0800 Subject: [PATCH 12/36] Update syntax.mdx "one label" should be the singular form. --- website/content/docs/job-specification/hcl2/syntax.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/docs/job-specification/hcl2/syntax.mdx b/website/content/docs/job-specification/hcl2/syntax.mdx index 651b5da1573..cf9f24d782f 100644 --- a/website/content/docs/job-specification/hcl2/syntax.mdx +++ b/website/content/docs/job-specification/hcl2/syntax.mdx @@ -64,7 +64,7 @@ task "webserver" { A block has a _type_ (`task` in this example). Each block type defines how many _labels_ must follow the type keyword. The `task` block type -expects one labels, which is `webserver` in the example above. +expects one label, which is `webserver` in the example above. A particular block type may have any number of required labels, or it may require none as with the nested `config` block type. From ae91c836f58ee81e01b2db416941139c99a001c1 Mon Sep 17 00:00:00 2001 From: Steven Collins Date: Mon, 25 Jan 2021 16:15:36 +0100 Subject: [PATCH 13/36] Adds community USB plugin to documentation site --- .../{community.mdx => external/index.mdx} | 10 ++ website/content/docs/devices/external/usb.mdx | 131 ++++++++++++++++++ website/data/docs-navigation.js | 10 +- 3 files changed, 150 insertions(+), 1 deletion(-) rename website/content/docs/devices/{community.mdx => external/index.mdx} (70%) create mode 100644 website/content/docs/devices/external/usb.mdx diff --git a/website/content/docs/devices/community.mdx b/website/content/docs/devices/external/index.mdx similarity index 70% rename from website/content/docs/devices/community.mdx rename to website/content/docs/devices/external/index.mdx index f6e894e1f8e..b2b08375417 100644 --- a/website/content/docs/devices/community.mdx +++ b/website/content/docs/devices/external/index.mdx @@ -11,6 +11,15 @@ If you have authored a device plugin that you believe will be useful to the broader Nomad community and you are committed to maintaining the plugin, please file a PR to add your plugin to this page. +## Device Plugins + +Nomad has a plugin system for defining task drivers. External device driver +plugins will have the same user experience as built in devices. + +Below is a list of community-supported task drivers you can use with Nomad: + +- [USB][usb] + ## Authoring Device Plugins Nomad has a plugin system for defining device drivers. External device plugins @@ -19,3 +28,4 @@ authoring a device plugin, please refer to the [plugin authoring guide][plugin_guide]. [plugin_guide]: /docs/internals/plugins +[usb]: /docs/devices/external/usb diff --git a/website/content/docs/devices/external/usb.mdx b/website/content/docs/devices/external/usb.mdx new file mode 100644 index 00000000000..a285599bc7c --- /dev/null +++ b/website/content/docs/devices/external/usb.mdx @@ -0,0 +1,131 @@ +--- +layout: docs +page_title: 'Devices: USB' +sidebar_title: USB Beta +description: The USB Device Plugin detects and makes USB devices available to tasks +--- + +# USB Device Plugin + +Name: `usb` + +The `usb` device plugin is used to expose USB devices to Nomad. The USB plugin +is not included within Nomad and requires downloading and providing on each +node. + +## Fingerprinted Attributes + +| Attribute | Unit | Description | +|------------------|--------|-------------------------------------------------| +| `vendor_id` | int | USB VID in decimal format | +| `product_id` | int | USB PID in decimal format | +| `class_id` | int | USB Class code in decimal format | +| `sub_class_id` | int | USB SubClass code in decimal format | +| `protocol_id` | int | USB Protocol code in decimal format | +| `vendor` | string | Human readable vendor | +| `product` | string | Human readable product | +| `description` | string | Human readable description of the USB device | +| `classification` | string | Human readable classification of the USB device | + +## Runtime Environment + +The `usb` device plugin exposes the following environment variables: + +- `USB_VISIBLE_DEVICES` - List of USB IDs (from the plugin) available to the task. + +## Installation Requirements + +In order to use the `usb` device plugin the following prerequisites must be +met: + +1. GNU/Linux i368/amd64/armv7/arm64 (Other Architectures and OS may work but + have not been tested) +2. [libusb-1.0](https://github.com/libusb/libusb/wiki) installed + +## Installation + +To install the `usb` device plugin you will first need to download the binary +for your Nomad client. You can find the latest release of these binaries on +the [release +page](https://gitlab.com/CarbonCollins/nomad-usb-device-plugin/-/releases). + +This binary needs to be downloaded into the plugins directory of the Nomad +node and the plugin stanza then needs to be added. A full example of the +plugin stanza needed for the `usb` driver can be found under the plugin +configuration section. + +## Plugin Configuration + +```hcl +plugin "usb" { + config { + enabled = true + + included_vendor_ids = [0x1CF1, 0x067b] + excluded_vendor_ids = [] + + included_product_ids = [0x0030, 0x2303] + excluded_product_ids = [] + + fingerprint_period = "1m" + } +} +``` + +The `usb` device plugin supports the following configuration in the agent config: + +- `enabled` `(bool: true)` - the `usb` driver may be disabled on clients by + setting this option to `false` (defaults to `true`). + +- `included_vendor_ids` (`array: []`): An array of VIDs to pass to + nomad if found + +- `excluded_vendor_ids` (`array: []`): An array of VIDs to not pass to + nomad if found (this takes priority over the include list) + +- `included_product_ids` (`array: []`): An array of PIDs to pass to + nomad if found + +- `excluded_product_ids` (`array: []`): An array of PIDs to not pass + to nomad if found (this takes priority over the include list) + +- `fingerprint_period` `(string: "1m")` - The period in which to fingerprint + for device changes. + +## Limitations + +Currently the `usb` driver does not automatically provide dev paths for the +USB devices into the task, this means in containerized applications you will +still have to supply the dev path manually e.g. `/dev/ttyUSB0` however this +plugin does allow containerized tasks to only be deployed on nodes which have +specified USB devices connected. + +## Other Information + +The `usb` device plugin is currently in beta and thus is not recommended for +production use. This will remain the case until the current limitations have +been solved with the plugin. If you find any issues with the `usb` plugin +please raise an issue on the plugins [issues +page](https://gitlab.com/CarbonCollins/nomad-usb-device-plugin/-/issues/new?issue) + +### USB Codes + +For a full list of available class, subclass, and protocol codes you can see +the [usb-if-defined-class-codes](https://www.usb.org/defined-class-codes) page +on the usb + +### USB Vendors & Products + +There are several sources where you can find known vendor and product ids for +USB devices. These are independently maintained lists from third parties but +can be useful as a reference. + +- [linux-usb.org](http://www.linux-usb.org/) +- [devicehunt.com](https://devicehunt.com/all-usb-vendors) + +### Source Code & Compiled Binaries + +The source code for this plugin can be found at +[CarbonCollins/nomad-usb-device-plugin](https://gitlab.com/CarbonCollins/nomad-usb-device-plugin). You +will also find pre built binaries on the [releases +page](https://gitlab.com/CarbonCollins/nomad-usb-device-plugin/-/releases). diff --git a/website/data/docs-navigation.js b/website/data/docs-navigation.js index 2787685dafa..0b2779c364a 100644 --- a/website/data/docs-navigation.js +++ b/website/data/docs-navigation.js @@ -397,7 +397,15 @@ export default [ }, { category: 'devices', - content: ['nvidia', 'community'], + content: [ + 'nvidia', + { + category: 'external', + content: [ + 'usb' + ] + } + ], }, 'schedulers', { category: 'runtime', content: ['environment', 'interpolation'] }, From 85129bb7943142d991122afcb890ab0baa598603 Mon Sep 17 00:00:00 2001 From: Drew Bailey Date: Mon, 25 Jan 2021 10:34:27 -0500 Subject: [PATCH 14/36] ignore setting job summary when oldstatus == newstatus (#9884) --- nomad/state/state_store.go | 13 +++----- nomad/state/state_store_test.go | 57 +++++++++++++-------------------- 2 files changed, 27 insertions(+), 43 deletions(-) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index bcbc6c16787..19f088ed778 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -4428,7 +4428,6 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn, // Capture the current status so we can check if there is a change oldStatus := job.Status - firstPass := index == job.CreateIndex newStatus := forceStatus // If forceStatus is not set, compute the jobs status. @@ -4440,12 +4439,8 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn, } } - // Fast-path if the job has changed. - // Still update the job summary if necessary. + // Fast-path if the job has not changed. if oldStatus == newStatus { - if err := s.setJobSummary(txn, job, index, oldStatus, newStatus, firstPass); err != nil { - return err - } return nil } @@ -4463,13 +4458,13 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn, } // Update the children summary - if err := s.setJobSummary(txn, updated, index, oldStatus, newStatus, firstPass); err != nil { + if err := s.setJobSummary(txn, updated, index, oldStatus, newStatus); err != nil { return fmt.Errorf("job summary update failed %w", err) } return nil } -func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64, oldStatus, newStatus string, firstPass bool) error { +func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64, oldStatus, newStatus string) error { if updated.ParentID == "" { return nil } @@ -4493,7 +4488,7 @@ func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64, children := pSummary.Children // Decrement old status - if !firstPass { + if oldStatus != "" { switch oldStatus { case structs.JobStatusPending: children.Pending-- diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index 1c42e677f1c..8f01cac7835 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -1659,6 +1659,7 @@ func TestStateStore_UpsertJob_ChildJob(t *testing.T) { } child := mock.Job() + child.Status = "" child.ParentID = parent.ID if err := state.UpsertJob(structs.MsgTypeTestSetup, 1001, child); err != nil { t.Fatalf("err: %v", err) @@ -1996,6 +1997,7 @@ func TestStateStore_DeleteJob_ChildJob(t *testing.T) { } child := mock.Job() + child.Status = "" child.ParentID = parent.ID if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil { @@ -4022,6 +4024,7 @@ func TestStateStore_UpsertEvals_Eval_ChildJob(t *testing.T) { } child := mock.Job() + child.Status = "" child.ParentID = parent.ID if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil { @@ -4246,6 +4249,7 @@ func TestStateStore_DeleteEval_ChildJob(t *testing.T) { } child := mock.Job() + child.Status = "" child.ParentID = parent.ID if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil { @@ -4498,6 +4502,7 @@ func TestStateStore_UpdateAllocsFromClient(t *testing.T) { } child := mock.Job() + child.Status = "" child.ParentID = parent.ID if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil { t.Fatalf("err: %v", err) @@ -5017,16 +5022,13 @@ func TestStateStore_UpsertAlloc_ChildJob(t *testing.T) { state := testStateStore(t) parent := mock.Job() - if err := state.UpsertJob(structs.MsgTypeTestSetup, 998, parent); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 998, parent)) child := mock.Job() + child.Status = "" child.ParentID = parent.ID - if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 999, child)) alloc := mock.Alloc() alloc.JobID = child.ID @@ -5034,40 +5036,27 @@ func TestStateStore_UpsertAlloc_ChildJob(t *testing.T) { // Create watchsets so we can test that delete fires the watch ws := memdb.NewWatchSet() - if _, err := state.JobSummaryByID(ws, parent.Namespace, parent.ID); err != nil { - t.Fatalf("bad: %v", err) - } + _, err := state.JobSummaryByID(ws, parent.Namespace, parent.ID) + require.NoError(t, err) - err := state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc}) - if err != nil { - t.Fatalf("err: %v", err) - } + err = state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc}) + require.NoError(t, err) - if !watchFired(ws) { - t.Fatalf("bad") - } + require.True(t, watchFired(ws)) ws = memdb.NewWatchSet() summary, err := state.JobSummaryByID(ws, parent.Namespace, parent.ID) - if err != nil { - t.Fatalf("err: %v", err) - } - if summary == nil { - t.Fatalf("nil summary") - } - if summary.JobID != parent.ID { - t.Fatalf("bad summary id: %v", parent.ID) - } - if summary.Children == nil { - t.Fatalf("nil children summary") - } - if summary.Children.Pending != 0 || summary.Children.Running != 1 || summary.Children.Dead != 0 { - t.Fatalf("bad children summary: %v", summary.Children) - } + require.NoError(t, err) + require.NotNil(t, summary) - if watchFired(ws) { - t.Fatalf("bad") - } + require.Equal(t, parent.ID, summary.JobID) + require.NotNil(t, summary.Children) + + require.Equal(t, int64(0), summary.Children.Pending) + require.Equal(t, int64(1), summary.Children.Running) + require.Equal(t, int64(0), summary.Children.Dead) + + require.False(t, watchFired(ws)) } func TestStateStore_UpdateAlloc_Alloc(t *testing.T) { From ceae8ad1cf3963e56a2b60ba5f54a27d66cecf3b Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Tue, 15 Dec 2020 14:38:33 -0600 Subject: [PATCH 15/36] consul/connect: Add support for Connect terminating gateways This PR implements Nomad built-in support for running Consul Connect terminating gateways. Such a gateway can be used by services running inside the service mesh to access "legacy" services running outside the service mesh while still making use of Consul's service identity based networking and ACL policies. https://www.consul.io/docs/connect/gateways/terminating-gateway These gateways are declared as part of a task group level service definition within the connect stanza. service { connect { gateway { proxy { // envoy proxy configuration } terminating { // terminating-gateway configuration entry } } } } Currently Envoy is the only supported gateway implementation in Consul. The gateay task can be customized by configuring the connect.sidecar_task block. When the gateway.terminating field is set, Nomad will write/update the Configuration Entry into Consul on job submission. Because CEs are global in scope and there may be more than one Nomad cluster communicating with Consul, there is an assumption that any terminating gateway defined in Nomad for a particular service will be the same among Nomad clusters. Gateways require Consul 1.8.0+, checked by a node constraint. Closes #9445 --- CHANGELOG.md | 4 +- api/services.go | 87 +++- api/services_test.go | 53 +++ .../taskrunner/envoy_bootstrap_hook.go | 23 +- command/agent/consul/connect.go | 10 +- command/agent/consul/connect_test.go | 8 +- command/agent/consul/service_client.go | 15 +- command/agent/job_endpoint.go | 42 +- command/agent/job_endpoint_test.go | 145 ++++++- contributing/checklist-jobspec.md | 24 +- e2e/connect/acls.go | 29 ++ e2e/connect/connect.go | 17 + e2e/connect/input/terminating-gateway.nomad | 109 +++++ nomad/consul.go | 49 ++- nomad/consul_test.go | 52 ++- nomad/job_endpoint.go | 12 +- nomad/job_endpoint_hook_connect.go | 235 ++++++----- nomad/job_endpoint_hook_connect_test.go | 94 ++++- nomad/node_endpoint_test.go | 2 +- nomad/structs/connect.go | 33 ++ nomad/structs/diff.go | 103 ++++- nomad/structs/diff_test.go | 104 +++++ nomad/structs/services.go | 246 +++++++++++- nomad/structs/services_test.go | 362 ++++++++++++++++- nomad/structs/structs.go | 28 +- nomad/structs/structs_test.go | 16 + .../hashicorp/nomad/api/services.go | 87 +++- .../docs/job-specification/gateway.mdx | 378 +++++++++++++----- 28 files changed, 1986 insertions(+), 381 deletions(-) create mode 100644 e2e/connect/input/terminating-gateway.nomad create mode 100644 nomad/structs/connect.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f751124cfc9..7e3d9943d5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,12 @@ ## 1.0.3 (Unreleased) +FEATURES: + * **Terminating Gateways**: Adds built-in support for running Consul Connect terminating gateways [[GH-9829](https://github.com/hashicorp/nomad/pull/9829)] + IMPROVEMENTS: * consul/connect: Made handling of sidecar task container image URLs consistent with the `docker` task driver. [[GH-9580](https://github.com/hashicorp/nomad/issues/9580)] BUG FIXES: - * consul: Fixed a bug where failing tasks with group services would only cause the allocation to restart once instead of respecting the `restart` field. [[GH-9869](https://github.com/hashicorp/nomad/issues/9869)] * consul/connect: Fixed a bug where gateway proxy connection default timeout not set [[GH-9851](https://github.com/hashicorp/nomad/pull/9851)] * consul/connect: Fixed a bug preventing more than one connect gateway per Nomad client [[GH-9849](https://github.com/hashicorp/nomad/pull/9849)] diff --git a/api/services.go b/api/services.go index f7e54011dee..f85e113cdfd 100644 --- a/api/services.go +++ b/api/services.go @@ -302,8 +302,8 @@ type ConsulGateway struct { // Ingress represents the Consul Configuration Entry for an Ingress Gateway. Ingress *ConsulIngressConfigEntry `hcl:"ingress,block"` - // Terminating is not yet supported. - // Terminating *ConsulTerminatingConfigEntry + // Terminating represents the Consul Configuration Entry for a Terminating Gateway. + Terminating *ConsulTerminatingConfigEntry `hcl:"terminating,block"` // Mesh is not yet supported. // Mesh *ConsulMeshConfigEntry @@ -315,6 +315,7 @@ func (g *ConsulGateway) Canonicalize() { } g.Proxy.Canonicalize() g.Ingress.Canonicalize() + g.Terminating.Canonicalize() } func (g *ConsulGateway) Copy() *ConsulGateway { @@ -323,8 +324,9 @@ func (g *ConsulGateway) Copy() *ConsulGateway { } return &ConsulGateway{ - Proxy: g.Proxy.Copy(), - Ingress: g.Ingress.Copy(), + Proxy: g.Proxy.Copy(), + Ingress: g.Ingress.Copy(), + Terminating: g.Terminating.Copy(), } } @@ -335,8 +337,8 @@ type ConsulGatewayBindAddress struct { } var ( - // defaultConnectTimeout is the default amount of time a connect gateway will - // wait for a response from an upstream service (same as consul) + // defaultGatewayConnectTimeout is the default amount of time connections to + // upstreams are allowed before timing out. defaultGatewayConnectTimeout = 5 * time.Second ) @@ -349,6 +351,7 @@ type ConsulGatewayProxy struct { EnvoyGatewayBindTaggedAddresses bool `mapstructure:"envoy_gateway_bind_tagged_addresses" hcl:"envoy_gateway_bind_tagged_addresses,optional"` EnvoyGatewayBindAddresses map[string]*ConsulGatewayBindAddress `mapstructure:"envoy_gateway_bind_addresses" hcl:"envoy_gateway_bind_addresses,block"` EnvoyGatewayNoDefaultBind bool `mapstructure:"envoy_gateway_no_default_bind" hcl:"envoy_gateway_no_default_bind,optional"` + EnvoyDNSDiscoveryType string `mapstructure:"envoy_dns_discovery_type" hcl:"envoy_dns_discovery_type,optional"` Config map[string]interface{} `hcl:"config,block"` // escape hatch envoy config } @@ -397,6 +400,7 @@ func (p *ConsulGatewayProxy) Copy() *ConsulGatewayProxy { EnvoyGatewayBindTaggedAddresses: p.EnvoyGatewayBindTaggedAddresses, EnvoyGatewayBindAddresses: binds, EnvoyGatewayNoDefaultBind: p.EnvoyGatewayNoDefaultBind, + EnvoyDNSDiscoveryType: p.EnvoyDNSDiscoveryType, Config: config, } } @@ -549,9 +553,74 @@ func (e *ConsulIngressConfigEntry) Copy() *ConsulIngressConfigEntry { } } -// ConsulTerminatingConfigEntry is not yet supported. -// type ConsulTerminatingConfigEntry struct { -// } +type ConsulLinkedService struct { + Name string `hcl:"name,optional"` + CAFile string `hcl:"ca_file,optional"` + CertFile string `hcl:"cert_file,optional"` + KeyFile string `hcl:"key_file,optional"` + SNI string `hcl:"sni,optional"` +} + +func (s *ConsulLinkedService) Canonicalize() { + // nothing to do for now +} + +func (s *ConsulLinkedService) Copy() *ConsulLinkedService { + if s == nil { + return nil + } + + return &ConsulLinkedService{ + Name: s.Name, + CAFile: s.CAFile, + CertFile: s.CertFile, + KeyFile: s.KeyFile, + SNI: s.SNI, + } +} + +// ConsulTerminatingConfigEntry represents the Consul Configuration Entry type +// for a Terminating Gateway. +// +// https://www.consul.io/docs/agent/config-entries/terminating-gateway#available-fields +type ConsulTerminatingConfigEntry struct { + // Namespace is not yet supported. + // Namespace string + + Services []*ConsulLinkedService `hcl:"service,block"` +} + +func (e *ConsulTerminatingConfigEntry) Canonicalize() { + if e == nil { + return + } + + if len(e.Services) == 0 { + e.Services = nil + } + + for _, service := range e.Services { + service.Canonicalize() + } +} + +func (e *ConsulTerminatingConfigEntry) Copy() *ConsulTerminatingConfigEntry { + if e == nil { + return nil + } + + var services []*ConsulLinkedService = nil + if n := len(e.Services); n > 0 { + services = make([]*ConsulLinkedService, n) + for i := 0; i < n; i++ { + services[i] = e.Services[i].Copy() + } + } + + return &ConsulTerminatingConfigEntry{ + Services: services, + } +} // ConsulMeshConfigEntry is not yet supported. // type ConsulMeshConfigEntry struct { diff --git a/api/services_test.go b/api/services_test.go index 6bfeaed4e8b..737b8b87ec4 100644 --- a/api/services_test.go +++ b/api/services_test.go @@ -291,7 +291,10 @@ func TestService_ConsulGateway_Canonicalize(t *testing.T) { } cg.Canonicalize() require.Equal(t, timeToPtr(5*time.Second), cg.Proxy.ConnectTimeout) + require.True(t, cg.Proxy.EnvoyGatewayBindTaggedAddresses) require.Nil(t, cg.Proxy.EnvoyGatewayBindAddresses) + require.True(t, cg.Proxy.EnvoyGatewayNoDefaultBind) + require.Empty(t, cg.Proxy.EnvoyDNSDiscoveryType) require.Nil(t, cg.Proxy.Config) require.Nil(t, cg.Ingress.Listeners) }) @@ -314,6 +317,7 @@ func TestService_ConsulGateway_Copy(t *testing.T) { "listener2": {Address: "10.0.0.1", Port: 2001}, }, EnvoyGatewayNoDefaultBind: true, + EnvoyDNSDiscoveryType: "STRICT_DNS", Config: map[string]interface{}{ "foo": "bar", "baz": 3, @@ -334,6 +338,11 @@ func TestService_ConsulGateway_Copy(t *testing.T) { }}, }, }, + Terminating: &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "linked-service1", + }}, + }, } t.Run("complete", func(t *testing.T) { @@ -418,3 +427,47 @@ func TestService_ConsulIngressConfigEntry_Copy(t *testing.T) { require.Equal(t, entry, result) }) } + +func TestService_ConsulTerminatingConfigEntry_Canonicalize(t *testing.T) { + t.Parallel() + + t.Run("nil", func(t *testing.T) { + c := (*ConsulTerminatingConfigEntry)(nil) + c.Canonicalize() + require.Nil(t, c) + }) + + t.Run("empty services", func(t *testing.T) { + c := &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{}, + } + c.Canonicalize() + require.Nil(t, c.Services) + }) +} + +func TestService_ConsulTerminatingConfigEntry_Copy(t *testing.T) { + t.Parallel() + + t.Run("nil", func(t *testing.T) { + result := (*ConsulIngressConfigEntry)(nil).Copy() + require.Nil(t, result) + }) + + entry := &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "servic1", + }, { + Name: "service2", + CAFile: "ca_file.pem", + CertFile: "cert_file.pem", + KeyFile: "key_file.pem", + SNI: "sni.terminating.consul", + }}, + } + + t.Run("complete", func(t *testing.T) { + result := entry.Copy() + require.Equal(t, entry, result) + }) +} diff --git a/client/allocrunner/taskrunner/envoy_bootstrap_hook.go b/client/allocrunner/taskrunner/envoy_bootstrap_hook.go index 1ae3e19659a..0b286138dff 100644 --- a/client/allocrunner/taskrunner/envoy_bootstrap_hook.go +++ b/client/allocrunner/taskrunner/envoy_bootstrap_hook.go @@ -110,13 +110,16 @@ func (envoyBootstrapHook) Name() string { return envoyBootstrapHookName } +func isConnectKind(kind string) bool { + kinds := []string{structs.ConnectProxyPrefix, structs.ConnectIngressPrefix, structs.ConnectTerminatingPrefix} + return helper.SliceStringContains(kinds, kind) +} + func (_ *envoyBootstrapHook) extractNameAndKind(kind structs.TaskKind) (string, string, error) { - serviceKind := kind.Name() serviceName := kind.Value() + serviceKind := kind.Name() - switch serviceKind { - case structs.ConnectProxyPrefix, structs.ConnectIngressPrefix: - default: + if !isConnectKind(serviceKind) { return "", "", errors.New("envoy must be used as connect sidecar or gateway") } @@ -350,13 +353,15 @@ func (h *envoyBootstrapHook) newEnvoyBootstrapArgs( proxyID string // gateway only ) - if service.Connect.HasSidecar() { + switch { + case service.Connect.HasSidecar(): sidecarForID = h.proxyServiceID(group, service) - } - - if service.Connect.IsGateway() { - gateway = "ingress" // more types in the future + case service.Connect.IsIngress(): + proxyID = h.proxyServiceID(group, service) + gateway = "ingress" + case service.Connect.IsTerminating(): proxyID = h.proxyServiceID(group, service) + gateway = "terminating" } h.logger.Debug("bootstrapping envoy", diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 1fcf3f568bc..68904070f6d 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -26,6 +26,7 @@ func newConnect(serviceName string, nc *structs.ConsulConnect, networks structs. return &api.AgentServiceConnect{Native: true}, nil case nc.HasSidecar(): + // must register the sidecar for this service sidecarReg, err := connectSidecarRegistration(serviceName, nc.SidecarService, networks) if err != nil { return nil, err @@ -33,6 +34,7 @@ func newConnect(serviceName string, nc *structs.ConsulConnect, networks structs. return &api.AgentServiceConnect{SidecarService: sidecarReg}, nil default: + // a non-nil but empty connect block makes no sense return nil, fmt.Errorf("Connect configuration empty for service %s", serviceName) } } @@ -64,6 +66,10 @@ func newConnectGateway(serviceName string, connect *structs.ConsulConnect) *api. envoyConfig["envoy_gateway_bind_tagged_addresses"] = true } + if proxy.EnvoyDNSDiscoveryType != "" { + envoyConfig["envoy_dns_discovery_type"] = proxy.EnvoyDNSDiscoveryType + } + if proxy.ConnectTimeout != nil { envoyConfig["connect_timeout_ms"] = proxy.ConnectTimeout.Milliseconds() } @@ -89,7 +95,7 @@ func connectSidecarRegistration(serviceName string, css *structs.ConsulSidecarSe return nil, err } - proxy, err := connectProxy(css.Proxy, cPort.To, networks) + proxy, err := connectSidecarProxy(css.Proxy, cPort.To, networks) if err != nil { return nil, err } @@ -102,7 +108,7 @@ func connectSidecarRegistration(serviceName string, css *structs.ConsulSidecarSe }, nil } -func connectProxy(proxy *structs.ConsulProxy, cPort int, networks structs.Networks) (*api.AgentServiceConnectProxyConfig, error) { +func connectSidecarProxy(proxy *structs.ConsulProxy, cPort int, networks structs.Networks) (*api.AgentServiceConnectProxyConfig, error) { if proxy == nil { proxy = new(structs.ConsulProxy) } diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index e3d08d3e97a..d42f639957c 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -119,7 +119,7 @@ func TestConnect_connectProxy(t *testing.T) { // If the input proxy is nil, we expect the output to be a proxy with its // config set to default values. t.Run("nil proxy", func(t *testing.T) { - proxy, err := connectProxy(nil, 2000, testConnectNetwork) + proxy, err := connectSidecarProxy(nil, 2000, testConnectNetwork) require.NoError(t, err) require.Equal(t, &api.AgentServiceConnectProxyConfig{ LocalServiceAddress: "", @@ -134,7 +134,7 @@ func TestConnect_connectProxy(t *testing.T) { }) t.Run("bad proxy", func(t *testing.T) { - _, err := connectProxy(&structs.ConsulProxy{ + _, err := connectSidecarProxy(&structs.ConsulProxy{ LocalServiceAddress: "0.0.0.0", LocalServicePort: 2000, Upstreams: nil, @@ -149,7 +149,7 @@ func TestConnect_connectProxy(t *testing.T) { }) t.Run("normal", func(t *testing.T) { - proxy, err := connectProxy(&structs.ConsulProxy{ + proxy, err := connectSidecarProxy(&structs.ConsulProxy{ LocalServiceAddress: "0.0.0.0", LocalServicePort: 2000, Upstreams: nil, @@ -453,6 +453,7 @@ func TestConnect_newConnectGateway(t *testing.T) { }, }, EnvoyGatewayNoDefaultBind: true, + EnvoyDNSDiscoveryType: "STRICT_DNS", Config: map[string]interface{}{ "foo": 1, }, @@ -470,6 +471,7 @@ func TestConnect_newConnectGateway(t *testing.T) { }, }, "envoy_gateway_no_default_bind": true, + "envoy_dns_discovery_type": "STRICT_DNS", "foo": 1, }, }, result) diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 798e2e451bf..762d3154c33 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -891,10 +891,21 @@ func (c *ServiceClient) serviceRegs(ops *operations, service *structs.Service, w // This enables the consul UI to show that Nomad registered this service meta["external-source"] = "nomad" - // Explicitly set the service kind in case this service represents a Connect gateway. + // Explicitly set the Consul service Kind in case this service represents + // one of the Connect gateway types. kind := api.ServiceKindTypical - if service.Connect.IsGateway() { + switch { + case service.Connect.IsIngress(): kind = api.ServiceKindIngressGateway + case service.Connect.IsTerminating(): + kind = api.ServiceKindTerminatingGateway + // set the default port if bridge / default listener set + if defaultBind, exists := service.Connect.Gateway.Proxy.EnvoyGatewayBindAddresses["default"]; exists { + portLabel := fmt.Sprintf("%s-%s", structs.ConnectTerminatingPrefix, service.Name) + if dynPort, ok := workload.Ports.Get(portLabel); ok { + defaultBind.Port = dynPort.Value + } + } } // Build the Consul Service registration request diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 7f73453ca82..6c3c4cd4020 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1335,8 +1335,9 @@ func apiConnectGatewayToStructs(in *api.ConsulGateway) *structs.ConsulGateway { } return &structs.ConsulGateway{ - Proxy: apiConnectGatewayProxyToStructs(in.Proxy), - Ingress: apiConnectIngressGatewayToStructs(in.Ingress), + Proxy: apiConnectGatewayProxyToStructs(in.Proxy), + Ingress: apiConnectIngressGatewayToStructs(in.Ingress), + Terminating: apiConnectTerminatingGatewayToStructs(in.Terminating), } } @@ -1360,6 +1361,7 @@ func apiConnectGatewayProxyToStructs(in *api.ConsulGatewayProxy) *structs.Consul EnvoyGatewayBindTaggedAddresses: in.EnvoyGatewayBindTaggedAddresses, EnvoyGatewayBindAddresses: bindAddresses, EnvoyGatewayNoDefaultBind: in.EnvoyGatewayNoDefaultBind, + EnvoyDNSDiscoveryType: in.EnvoyDNSDiscoveryType, Config: helper.CopyMapStringInterface(in.Config), } } @@ -1432,6 +1434,42 @@ func apiConnectIngressServiceToStructs(in *api.ConsulIngressService) *structs.Co } } +func apiConnectTerminatingGatewayToStructs(in *api.ConsulTerminatingConfigEntry) *structs.ConsulTerminatingConfigEntry { + if in == nil { + return nil + } + + return &structs.ConsulTerminatingConfigEntry{ + Services: apiConnectTerminatingServicesToStructs(in.Services), + } +} + +func apiConnectTerminatingServicesToStructs(in []*api.ConsulLinkedService) []*structs.ConsulLinkedService { + if len(in) == 0 { + return nil + } + + services := make([]*structs.ConsulLinkedService, len(in)) + for i, service := range in { + services[i] = apiConnectTerminatingServiceToStructs(service) + } + return services +} + +func apiConnectTerminatingServiceToStructs(in *api.ConsulLinkedService) *structs.ConsulLinkedService { + if in == nil { + return nil + } + + return &structs.ConsulLinkedService{ + Name: in.Name, + CAFile: in.CAFile, + CertFile: in.CertFile, + KeyFile: in.KeyFile, + SNI: in.SNI, + } +} + func apiConnectSidecarServiceToStructs(in *api.ConsulSidecarService) *structs.ConsulSidecarService { if in == nil { return nil diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 166c995ba95..43f395a5f4f 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -3061,26 +3061,131 @@ func TestConversion_apiConnectSidecarServiceToStructs(t *testing.T) { })) } -func TestConversion_ApiConsulConnectToStructs_legacy(t *testing.T) { +func TestConversion_ApiConsulConnectToStructs(t *testing.T) { t.Parallel() - require.Nil(t, ApiConsulConnectToStructs(nil)) - require.Equal(t, &structs.ConsulConnect{ - Native: false, - SidecarService: &structs.ConsulSidecarService{Port: "myPort"}, - SidecarTask: &structs.SidecarTask{Name: "task"}, - }, ApiConsulConnectToStructs(&api.ConsulConnect{ - Native: false, - SidecarService: &api.ConsulSidecarService{Port: "myPort"}, - SidecarTask: &api.SidecarTask{Name: "task"}, - })) -} -func TestConversion_ApiConsulConnectToStructs_native(t *testing.T) { - t.Parallel() - require.Nil(t, ApiConsulConnectToStructs(nil)) - require.Equal(t, &structs.ConsulConnect{ - Native: true, - }, ApiConsulConnectToStructs(&api.ConsulConnect{ - Native: true, - })) + t.Run("nil", func(t *testing.T) { + require.Nil(t, ApiConsulConnectToStructs(nil)) + }) + + t.Run("sidecar", func(t *testing.T) { + require.Equal(t, &structs.ConsulConnect{ + Native: false, + SidecarService: &structs.ConsulSidecarService{Port: "myPort"}, + SidecarTask: &structs.SidecarTask{Name: "task"}, + }, ApiConsulConnectToStructs(&api.ConsulConnect{ + Native: false, + SidecarService: &api.ConsulSidecarService{Port: "myPort"}, + SidecarTask: &api.SidecarTask{Name: "task"}, + })) + }) + + t.Run("gateway proxy", func(t *testing.T) { + require.Equal(t, &structs.ConsulConnect{ + Gateway: &structs.ConsulGateway{ + Proxy: &structs.ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(3 * time.Second), + EnvoyGatewayBindTaggedAddresses: true, + EnvoyGatewayBindAddresses: map[string]*structs.ConsulGatewayBindAddress{ + "service": { + Address: "10.0.0.1", + Port: 9000, + }}, + EnvoyGatewayNoDefaultBind: true, + EnvoyDNSDiscoveryType: "STRICT_DNS", + Config: map[string]interface{}{ + "foo": "bar", + }, + }, + }, + }, ApiConsulConnectToStructs(&api.ConsulConnect{ + Gateway: &api.ConsulGateway{ + Proxy: &api.ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(3 * time.Second), + EnvoyGatewayBindTaggedAddresses: true, + EnvoyGatewayBindAddresses: map[string]*api.ConsulGatewayBindAddress{ + "service": { + Address: "10.0.0.1", + Port: 9000, + }, + }, + EnvoyGatewayNoDefaultBind: true, + EnvoyDNSDiscoveryType: "STRICT_DNS", + Config: map[string]interface{}{ + "foo": "bar", + }, + }, + }, + })) + }) + + t.Run("gateway ingress", func(t *testing.T) { + require.Equal(t, &structs.ConsulConnect{ + Gateway: &structs.ConsulGateway{ + Ingress: &structs.ConsulIngressConfigEntry{ + TLS: &structs.ConsulGatewayTLSConfig{Enabled: true}, + Listeners: []*structs.ConsulIngressListener{{ + Port: 1111, + Protocol: "http", + Services: []*structs.ConsulIngressService{{ + Name: "ingress1", + Hosts: []string{"host1"}, + }}, + }}, + }, + }, + }, ApiConsulConnectToStructs( + &api.ConsulConnect{ + Gateway: &api.ConsulGateway{ + Ingress: &api.ConsulIngressConfigEntry{ + TLS: &api.ConsulGatewayTLSConfig{Enabled: true}, + Listeners: []*api.ConsulIngressListener{{ + Port: 1111, + Protocol: "http", + Services: []*api.ConsulIngressService{{ + Name: "ingress1", + Hosts: []string{"host1"}, + }}, + }}, + }, + }, + }, + )) + }) + + t.Run("gateway terminating", func(t *testing.T) { + require.Equal(t, &structs.ConsulConnect{ + Gateway: &structs.ConsulGateway{ + Terminating: &structs.ConsulTerminatingConfigEntry{ + Services: []*structs.ConsulLinkedService{{ + Name: "linked-service", + CAFile: "ca.pem", + CertFile: "cert.pem", + KeyFile: "key.pem", + SNI: "linked.consul", + }}, + }, + }, + }, ApiConsulConnectToStructs(&api.ConsulConnect{ + Gateway: &api.ConsulGateway{ + Terminating: &api.ConsulTerminatingConfigEntry{ + Services: []*api.ConsulLinkedService{{ + Name: "linked-service", + CAFile: "ca.pem", + CertFile: "cert.pem", + KeyFile: "key.pem", + SNI: "linked.consul", + }}, + }, + }, + })) + }) + + t.Run("native", func(t *testing.T) { + require.Equal(t, &structs.ConsulConnect{ + Native: true, + }, ApiConsulConnectToStructs(&api.ConsulConnect{ + Native: true, + })) + }) } diff --git a/contributing/checklist-jobspec.md b/contributing/checklist-jobspec.md index 315b00f4320..24c86a226da 100644 --- a/contributing/checklist-jobspec.md +++ b/contributing/checklist-jobspec.md @@ -2,21 +2,25 @@ ## Code -* [ ] Consider similar features in Consul, Kubernetes, and other tools. Is - there prior art we should match? Terminology, structure, etc? +* [ ] Consider similar features in Consul, Kubernetes, and other tools. Is there prior art we should match? Terminology, structure, etc? * [ ] Add structs/fields to `api/` package - * structs usually have Canonicalize, Copy, and Merge methods - * New fields should be added to existing Canonicalize, Copy, and Merge - methods - * Test the struct/field via all methods mentioned above + * `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 - * Validation happens in this package and must be implemented - * Implement other methods and tests from `api/` 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 check for job diff in `nomad/structs/diff.go` + * Add test for conversion +* [ ] Implement diff logic for new structs/fields in `nomad/structs/diff.go` * Note that fields must be listed in alphabetical order in `FieldDiff` slices in `nomad/structs/diff_test.go` -* [ ] Test conversion + * Add test for diff of new structs/fields +* [ ] Add change detection for new structs/feilds in `scheduler/util.go/tasksUpdated` + * 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) diff --git a/e2e/connect/acls.go b/e2e/connect/acls.go index 3441c7066c6..1f3cbc4b404 100644 --- a/e2e/connect/acls.go +++ b/e2e/connect/acls.go @@ -325,6 +325,35 @@ func (tc *ConnectACLsE2ETest) TestConnectACLsConnectIngressGatewayDemo(f *framew t.Log("connect ingress gateway job with ACLs enabled finished") } +func (tc *ConnectACLsE2ETest) TestConnectACLsConnectTerminatingGatewayDemo(f *framework.F) { + t := f.T() + + t.Log("test register Connect Terminating Gateway job w/ ACLs enabled") + + // setup ACL policy and mint operator token + + policyID := tc.createConsulPolicy(consulPolicy{ + Name: "nomad-operator-policy", + Rules: `service "api-gateway" { policy = "write" } service "count-dashboard" { policy = "write" }`, + }, f) + operatorToken := tc.createOperatorToken(policyID, f) + t.Log("created operator token:", operatorToken) + + jobID := connectJobID() + tc.jobIDs = append(tc.jobIDs, jobID) + + allocs := e2eutil.RegisterAndWaitForAllocs(t, tc.Nomad(), demoConnectTerminatingGateway, jobID, operatorToken) + allocIDs := e2eutil.AllocIDsFromAllocationListStubs(allocs) + e2eutil.WaitForAllocsRunning(t, tc.Nomad(), allocIDs) + + foundSITokens := tc.countSITokens(t) + f.Equal(2, len(foundSITokens), "expected 2 SI tokens total: %v", foundSITokens) + f.Equal(1, foundSITokens["connect-terminating-api-gateway"], "expected 1 SI token for connect-terminating-api-gateway: %v", foundSITokens) + f.Equal(1, foundSITokens["connect-proxy-count-dashboard"], "expected 1 SI token for count-dashboard: %v", foundSITokens) + + t.Log("connect terminating gateway job with ACLs enabled finished") +} + var ( siTokenRe = regexp.MustCompile(`_nomad_si \[[\w-]{36}] \[[\w-]{36}] \[([\S]+)]`) ) diff --git a/e2e/connect/connect.go b/e2e/connect/connect.go index 6cdc663f0fc..3379a750b27 100644 --- a/e2e/connect/connect.go +++ b/e2e/connect/connect.go @@ -23,6 +23,9 @@ const ( // demoConnectMultiIngressGateway is the example multi ingress gateway job useful for testing demoConnectMultiIngressGateway = "connect/input/multi-ingress.nomad" + + // demoConnectTerminatingGateway is the example terminating gateway job useful for testing + demoConnectTerminatingGateway = "connect/input/terminating-gateway.nomad" ) type ConnectE2ETest struct { @@ -117,6 +120,20 @@ func (tc *ConnectE2ETest) TestConnectMultiIngressGatewayDemo(f *framework.F) { tc.jobIds = append(tc.jobIds, jobID) allocs := e2eutil.RegisterAndWaitForAllocs(t, tc.Nomad(), demoConnectMultiIngressGateway, jobID, "") + + allocIDs := e2eutil.AllocIDsFromAllocationListStubs(allocs) + e2eutil.WaitForAllocsRunning(t, tc.Nomad(), allocIDs) +} + +func (tc *ConnectE2ETest) TestConnectTerminatingGatewayDemo(f *framework.F) { + + t := f.T() + + jobID := connectJobID() + tc.jobIds = append(tc.jobIds, jobID) + + allocs := e2eutil.RegisterAndWaitForAllocs(t, tc.Nomad(), demoConnectTerminatingGateway, jobID, "") + allocIDs := e2eutil.AllocIDsFromAllocationListStubs(allocs) e2eutil.WaitForAllocsRunning(t, tc.Nomad(), allocIDs) } diff --git a/e2e/connect/input/terminating-gateway.nomad b/e2e/connect/input/terminating-gateway.nomad new file mode 100644 index 00000000000..ff8ea276e8b --- /dev/null +++ b/e2e/connect/input/terminating-gateway.nomad @@ -0,0 +1,109 @@ +job "countdash-terminating" { + + datacenters = ["dc1"] + + constraint { + attribute = "${attr.kernel.name}" + value = "linux" + } + + group "api" { + network { + mode = "host" + port "port" { + static = "9001" + } + } + + service { + name = "count-api" + port = "port" + } + + task "api" { + driver = "docker" + + config { + image = "hashicorpnomad/counter-api:v3" + network_mode = "host" + } + } + } + + group "gateway" { + network { + mode = "bridge" + } + + service { + name = "api-gateway" + + connect { + gateway { + proxy { + # The following options are automatically set by Nomad if not explicitly + # configured with using bridge networking. + # + # envoy_gateway_no_default_bind = true + # envoy_gateway_bind_addresses "default" { + # address = "0.0.0.0" + # port = + # } + # Additional options are documented at + # https://www.nomadproject.io/docs/job-specification/gateway#proxy-parameters + } + + terminating { + # Nomad will automatically manage the Configuration Entry in Consul + # given the parameters in the terminating block. + # + # Additional options are documented at + # https://www.nomadproject.io/docs/job-specification/gateway#terminating-parameters + service { + name = "count-api" + } + } + } + } + } + } + + group "dashboard" { + network { + mode = "bridge" + + port "http" { + static = 9002 + to = 9002 + } + } + + service { + name = "count-dashboard" + port = "9002" + + connect { + sidecar_service { + proxy { + upstreams { + destination_name = "count-api" + local_bind_port = 8080 + } + } + } + } + } + + task "dashboard" { + driver = "docker" + + env { + COUNTING_SERVICE_URL = "http://${NOMAD_UPSTREAM_ADDR_count_api}" + } + + config { + image = "hashicorpnomad/counter-dashboard:v3" + } + } + } +} diff --git a/nomad/consul.go b/nomad/consul.go index f6bd1960c0e..a541e1ec2d4 100644 --- a/nomad/consul.go +++ b/nomad/consul.go @@ -451,9 +451,13 @@ func (s *Server) purgeSITokenAccessors(accessors []*structs.SITokenAccessor) err // Removing the entries is not particularly safe, given that multiple Nomad clusters // may be writing to the same config entries, which are global in the Consul scope. type ConsulConfigsAPI interface { - // SetIngressGatewayConfigEntry adds the given ConfigEntry to Consul, overwriting + // SetIngressCE adds the given ConfigEntry to Consul, overwriting // the previous entry if set. - SetIngressGatewayConfigEntry(ctx context.Context, service string, entry *structs.ConsulIngressConfigEntry) error + SetIngressCE(ctx context.Context, service string, entry *structs.ConsulIngressConfigEntry) error + + // SetTerminatingCE adds the given ConfigEntry to Consul, overwriting + // the previous entry if set. + SetTerminatingCE(ctx context.Context, service string, entry *structs.ConsulTerminatingConfigEntry) error // Stop is used to stop additional creations of Configuration Entries. Intended to // be used on Nomad Server shutdown. @@ -491,13 +495,18 @@ func (c *consulConfigsAPI) Stop() { c.stopped = true } -func (c *consulConfigsAPI) SetIngressGatewayConfigEntry(ctx context.Context, service string, entry *structs.ConsulIngressConfigEntry) error { - configEntry := convertIngressGatewayConfig(service, entry) - return c.setConfigEntry(ctx, configEntry) +func (c *consulConfigsAPI) SetIngressCE(ctx context.Context, service string, entry *structs.ConsulIngressConfigEntry) error { + return c.setCE(ctx, convertIngressCE(service, entry)) } -// setConfigEntry will set the Configuration Entry of any type Consul supports. -func (c *consulConfigsAPI) setConfigEntry(ctx context.Context, entry api.ConfigEntry) error { +func (c *consulConfigsAPI) SetTerminatingCE(ctx context.Context, service string, entry *structs.ConsulTerminatingConfigEntry) error { + return c.setCE(ctx, convertTerminatingCE(service, entry)) +} + +// also mesh + +// setCE will set the Configuration Entry of any type Consul supports. +func (c *consulConfigsAPI) setCE(ctx context.Context, entry api.ConfigEntry) error { defer metrics.MeasureSince([]string{"nomad", "consul", "create_config_entry"}, time.Now()) // make sure the background deletion goroutine has not been stopped @@ -518,14 +527,14 @@ func (c *consulConfigsAPI) setConfigEntry(ctx context.Context, entry api.ConfigE return err } -func convertIngressGatewayConfig(service string, entry *structs.ConsulIngressConfigEntry) api.ConfigEntry { +func convertIngressCE(service string, entry *structs.ConsulIngressConfigEntry) api.ConfigEntry { var listeners []api.IngressListener = nil for _, listener := range entry.Listeners { var services []api.IngressService = nil - for _, service := range listener.Services { + for _, s := range listener.Services { services = append(services, api.IngressService{ - Name: service.Name, - Hosts: helper.CopySliceString(service.Hosts), + Name: s.Name, + Hosts: helper.CopySliceString(s.Hosts), }) } listeners = append(listeners, api.IngressListener{ @@ -547,3 +556,21 @@ func convertIngressGatewayConfig(service string, entry *structs.ConsulIngressCon Listeners: listeners, } } + +func convertTerminatingCE(service string, entry *structs.ConsulTerminatingConfigEntry) api.ConfigEntry { + var linked []api.LinkedService = nil + for _, s := range entry.Services { + linked = append(linked, api.LinkedService{ + Name: s.Name, + CAFile: s.CAFile, + CertFile: s.CertFile, + KeyFile: s.KeyFile, + SNI: s.SNI, + }) + } + return &api.TerminatingGatewayConfigEntry{ + Kind: api.TerminatingGateway, + Name: service, + Services: linked, + } +} diff --git a/nomad/consul_test.go b/nomad/consul_test.go index 3eacbfe65bf..c57a0e32be3 100644 --- a/nomad/consul_test.go +++ b/nomad/consul_test.go @@ -20,36 +20,54 @@ var _ ConsulACLsAPI = (*consulACLsAPI)(nil) var _ ConsulACLsAPI = (*mockConsulACLsAPI)(nil) var _ ConsulConfigsAPI = (*consulConfigsAPI)(nil) -func TestConsulConfigsAPI_SetIngressGatewayConfigEntry(t *testing.T) { +func TestConsulConfigsAPI_SetCE(t *testing.T) { t.Parallel() - try := func(t *testing.T, expErr error) { + try := func(t *testing.T, expect error, f func(ConsulConfigsAPI) error) { logger := testlog.HCLogger(t) - configsAPI := consul.NewMockConfigsAPI(logger) // agent - configsAPI.SetError(expErr) + configsAPI := consul.NewMockConfigsAPI(logger) + configsAPI.SetError(expect) c := NewConsulConfigsAPI(configsAPI, logger) + err := f(c) // set the config entry - ctx := context.Background() - err := c.SetIngressGatewayConfigEntry(ctx, "service1", &structs.ConsulIngressConfigEntry{ - TLS: nil, - Listeners: nil, - }) - - if expErr != nil { - require.Equal(t, expErr, err) - } else { + switch expect { + case nil: require.NoError(t, err) + default: + require.Equal(t, expect, err) } } - t.Run("set ingress CE success", func(t *testing.T) { - try(t, nil) + ctx := context.Background() + + ingressCE := new(structs.ConsulIngressConfigEntry) + t.Run("ingress ok", func(t *testing.T) { + try(t, nil, func(c ConsulConfigsAPI) error { + return c.SetIngressCE(ctx, "ig", ingressCE) + }) }) - t.Run("set ingress CE failure", func(t *testing.T) { - try(t, errors.New("consul broke")) + t.Run("ingress fail", func(t *testing.T) { + try(t, errors.New("consul broke"), func(c ConsulConfigsAPI) error { + return c.SetIngressCE(ctx, "ig", ingressCE) + }) + }) + + terminatingCE := new(structs.ConsulTerminatingConfigEntry) + t.Run("terminating ok", func(t *testing.T) { + try(t, nil, func(c ConsulConfigsAPI) error { + return c.SetTerminatingCE(ctx, "tg", terminatingCE) + }) }) + + t.Run("terminating fail", func(t *testing.T) { + try(t, errors.New("consul broke"), func(c ConsulConfigsAPI) error { + return c.SetTerminatingCE(ctx, "tg", terminatingCE) + }) + }) + + // also mesh } type revokeRequest struct { diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 245e56da7f2..5d35675d6d7 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -289,11 +289,19 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis // Every job update will re-write the Configuration Entry into Consul. ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - for service, entry := range args.Job.ConfigEntries() { - if err := j.srv.consulConfigEntries.SetIngressGatewayConfigEntry(ctx, service, entry); err != nil { + entries := args.Job.ConfigEntries() + for service, entry := range entries.Ingress { + if err := j.srv.consulConfigEntries.SetIngressCE(ctx, service, entry); err != nil { return err } } + for service, entry := range entries.Terminating { + fmt.Println("SH JE set terminating CE", service) + if err := j.srv.consulConfigEntries.SetTerminatingCE(ctx, service, entry); err != nil { + return err + } + } + // also mesh // Enforce Sentinel policies. Pass a copy of the job to prevent // sentinel from altering it. diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index e7249097f71..fbd444fa275 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -18,77 +18,75 @@ const ( defaultConnectTimeout = 5 * time.Second ) -var ( - // connectSidecarResources returns the set of resources used by default for - // the Consul Connect sidecar task - connectSidecarResources = func() *structs.Resources { - return &structs.Resources{ - CPU: 250, - MemoryMB: 128, - } +// connectSidecarResources returns the set of resources used by default for +// the Consul Connect sidecar task +func connectSidecarResources() *structs.Resources { + return &structs.Resources{ + CPU: 250, + MemoryMB: 128, } +} - // connectSidecarDriverConfig is the driver configuration used by the injected - // connect proxy sidecar task. - connectSidecarDriverConfig = func() map[string]interface{} { - return map[string]interface{}{ - "image": envoy.SidecarConfigVar, - "args": []interface{}{ - "-c", structs.EnvoyBootstrapPath, - "-l", "${meta.connect.log_level}", - "--concurrency", "${meta.connect.proxy_concurrency}", - "--disable-hot-restart", - }, - } +// connectSidecarDriverConfig is the driver configuration used by the injected +// connect proxy sidecar task. +func connectSidecarDriverConfig() map[string]interface{} { + return map[string]interface{}{ + "image": envoy.SidecarConfigVar, + "args": []interface{}{ + "-c", structs.EnvoyBootstrapPath, + "-l", "${meta.connect.log_level}", + "--concurrency", "${meta.connect.proxy_concurrency}", + "--disable-hot-restart", + }, } +} - // connectGatewayDriverConfig is the Docker driver configuration used by the - // injected connect proxy sidecar task. - // - // A gateway may run in a group with bridge or host networking, and if host - // networking is being used the network_mode driver configuration is set here. - connectGatewayDriverConfig = func(hostNetwork bool) map[string]interface{} { - m := map[string]interface{}{ - "image": envoy.GatewayConfigVar, - "args": []interface{}{ - "-c", structs.EnvoyBootstrapPath, - "-l", "${meta.connect.log_level}", - "--concurrency", "${meta.connect.proxy_concurrency}", - "--disable-hot-restart", - }, - } - - if hostNetwork { - m["network_mode"] = "host" - } +// connectGatewayDriverConfig is the Docker driver configuration used by the +// injected connect proxy sidecar task. +// +// A gateway may run in a group with bridge or host networking, and if host +// networking is being used the network_mode driver configuration is set here. +func connectGatewayDriverConfig(hostNetwork bool) map[string]interface{} { + m := map[string]interface{}{ + "image": envoy.GatewayConfigVar, + "args": []interface{}{ + "-c", structs.EnvoyBootstrapPath, + "-l", "${meta.connect.log_level}", + "--concurrency", "${meta.connect.proxy_concurrency}", + "--disable-hot-restart", + }, + } - return m + if hostNetwork { + m["network_mode"] = "host" } - // connectMinimalVersionConstraint is used when building the sidecar task to ensure - // the proper Consul version is used that supports the necessary Connect - // features. This includes bootstrapping envoy with a unix socket for Consul's - // gRPC xDS API. - connectMinimalVersionConstraint = func() *structs.Constraint { - return &structs.Constraint{ - LTarget: "${attr.consul.version}", - RTarget: ">= 1.6.0-beta1", - Operand: structs.ConstraintSemver, - } + return m +} + +// connectSidecarVersionConstraint is used when building the sidecar task to ensure +// the proper Consul version is used that supports the necessary Connect +// features. This includes bootstrapping envoy with a unix socket for Consul's +// gRPC xDS API. +func connectSidecarVersionConstraint() *structs.Constraint { + return &structs.Constraint{ + LTarget: "${attr.consul.version}", + RTarget: ">= 1.6.0-beta1", + Operand: structs.ConstraintSemver, } +} - // connectGatewayVersionConstraint is used when building a connect gateway - // task to ensure proper Consul version is used that supports Connect Gateway - // features. This includes making use of Consul Configuration Entries of type - // {ingress,terminating,mesh}-gateway. - connectGatewayVersionConstraint = func() *structs.Constraint { - return &structs.Constraint{ - LTarget: "${attr.consul.version}", - RTarget: ">= 1.8.0", - Operand: structs.ConstraintSemver, - } +// connectGatewayVersionConstraint is used when building a connect gateway +// task to ensure proper Consul version is used that supports Connect Gateway +// features. This includes making use of Consul Configuration Entries of type +// {ingress,terminating,mesh}-gateway. +func connectGatewayVersionConstraint() *structs.Constraint { + return &structs.Constraint{ + LTarget: "${attr.consul.version}", + RTarget: ">= 1.8.0", + Operand: structs.ConstraintSemver, } -) +} // jobConnectHook implements a job Mutating and Validating admission controller type jobConnectHook struct{} @@ -97,7 +95,7 @@ func (jobConnectHook) Name() string { return "connect" } -func (jobConnectHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error, err error) { +func (jobConnectHook) Mutate(job *structs.Job) (*structs.Job, []error, error) { for _, g := range job.TaskGroups { // TG isn't validated yet, but validation // may depend on mutation results. @@ -116,13 +114,13 @@ func (jobConnectHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error return job, nil, nil } -func (jobConnectHook) Validate(job *structs.Job) (warnings []error, err error) { +func (jobConnectHook) Validate(job *structs.Job) ([]error, error) { + var warnings []error + for _, g := range job.TaskGroups { - w, err := groupConnectValidate(g) - if err != nil { + if w, err := groupConnectValidate(g); err != nil { return nil, err - } - if w != nil { + } else if w != nil { warnings = append(warnings, w...) } } @@ -149,9 +147,11 @@ func hasGatewayTaskForService(tg *structs.TaskGroup, svc string) bool { for _, t := range tg.Tasks { switch { case isIngressGatewayForService(t, svc): - // also terminating and mesh in the future + return true + case isTerminatingGatewayForService(t, svc): return true } + // mesh later } return false } @@ -160,6 +160,10 @@ func isIngressGatewayForService(t *structs.Task, svc string) bool { return t.Kind == structs.NewTaskKind(structs.ConnectIngressPrefix, svc) } +func isTerminatingGatewayForService(t *structs.Task, svc string) bool { + return t.Kind == structs.NewTaskKind(structs.ConnectTerminatingPrefix, svc) +} + // getNamedTaskForNativeService retrieves the Task with the name specified in the // group service definition. If the task name is empty and there is only one task // in the group, infer the name from the only option. @@ -179,6 +183,24 @@ func getNamedTaskForNativeService(tg *structs.TaskGroup, serviceName, taskName s return nil, errors.Errorf("task %s named by Consul Connect Native service %s->%s does not exist", taskName, tg.Name, serviceName) } +func injectPort(group *structs.TaskGroup, label string) { + // check that port hasn't already been defined before adding it to tg + for _, p := range group.Networks[0].DynamicPorts { + if p.Label == label { + return + } + } + + // inject a port of label that maps inside the bridge namespace + group.Networks[0].DynamicPorts = append(group.Networks[0].DynamicPorts, structs.Port{ + Label: label, + // -1 is a sentinel value to instruct the + // scheduler to map the host's dynamic port to + // the same port in the netns. + To: -1, + }) +} + // probably need to hack this up to look for checks on the service, and if they // qualify, configure a port for envoy to use to expose their paths. func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { @@ -205,7 +227,7 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // If the task doesn't already exist, create a new one and add it to the job if task == nil { - task = newConnectTask(service.Name) + task = newConnectSidecarTask(service.Name) // If there happens to be a task defined with the same name // append an UUID fragment to the task name @@ -225,24 +247,8 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // Canonicalize task since this mutator runs after job canonicalization task.Canonicalize(job, g) - makePort := func(label string) { - // check that port hasn't already been defined before adding it to tg - for _, p := range g.Networks[0].DynamicPorts { - if p.Label == label { - return - } - } - g.Networks[0].DynamicPorts = append(g.Networks[0].DynamicPorts, structs.Port{ - Label: label, - // -1 is a sentinel value to instruct the - // scheduler to map the host's dynamic port to - // the same port in the netns. - To: -1, - }) - } - // create a port for the sidecar task's proxy port - makePort(fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name)) + injectPort(g, fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name)) case service.Connect.IsNative(): // find the task backing this connect native service and set the kind @@ -259,18 +265,31 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // a name of an injected gateway task service.Name = env.ReplaceEnv(service.Name) + // detect whether the group is in host networking mode, which will + // require tweaking the default gateway task config netHost := g.Networks[0].Mode == "host" - if !netHost && service.Connect.Gateway.Ingress != nil { + + if !netHost && service.Connect.IsGateway() { // Modify the gateway proxy service configuration to automatically // do the correct envoy bind address plumbing when inside a net // namespace, but only if things are not explicitly configured. service.Connect.Gateway.Proxy = gatewayProxyForBridge(service.Connect.Gateway) } + // Inject a port whether bridge or host network (if not already set). + // This port is accessed by the magic of Connect plumbing so it seems + // reasonable to keep the magic alive here. + if service.Connect.IsTerminating() && service.PortLabel == "" { + // Inject a dynamic port for the terminating gateway. + portLabel := fmt.Sprintf("%s-%s", structs.ConnectTerminatingPrefix, service.Name) + service.PortLabel = portLabel + injectPort(g, portLabel) + } + // inject the gateway task only if it does not yet already exist if !hasGatewayTaskForService(g, service.Name) { - task := newConnectGatewayTask(service.Name, netHost) - + prefix := service.Connect.Gateway.Prefix() + task := newConnectGatewayTask(prefix, service.Name, netHost) g.Tasks = append(g.Tasks, task) // the connect.sidecar_task stanza can also be used to configure @@ -327,6 +346,7 @@ func gatewayProxyForBridge(gateway *structs.ConsulGateway) *structs.ConsulGatewa proxy := new(structs.ConsulGatewayProxy) if gateway.Proxy != nil { proxy.ConnectTimeout = gateway.Proxy.ConnectTimeout + proxy.EnvoyDNSDiscoveryType = gateway.Proxy.EnvoyDNSDiscoveryType proxy.Config = gateway.Proxy.Config } @@ -335,15 +355,28 @@ func gatewayProxyForBridge(gateway *structs.ConsulGateway) *structs.ConsulGatewa proxy.ConnectTimeout = helper.TimeToPtr(defaultConnectTimeout) } - // magically set the fields where Nomad knows what to do - proxy.EnvoyGatewayNoDefaultBind = true - proxy.EnvoyGatewayBindTaggedAddresses = false - proxy.EnvoyGatewayBindAddresses = gatewayBindAddresses(gateway.Ingress) + // magically configure bind address(es) for bridge networking, per gateway type + // non-default configuration is gated above + switch { + case gateway.Ingress != nil: + proxy.EnvoyGatewayNoDefaultBind = true + proxy.EnvoyGatewayBindTaggedAddresses = false + proxy.EnvoyGatewayBindAddresses = gatewayBindAddressesIngress(gateway.Ingress) + case gateway.Terminating != nil: + proxy.EnvoyGatewayNoDefaultBind = true + proxy.EnvoyGatewayBindTaggedAddresses = false + proxy.EnvoyGatewayBindAddresses = map[string]*structs.ConsulGatewayBindAddress{ + "default": { + Address: "0.0.0.0", + Port: -1, // filled in later with dynamic port + }} + } + // later: mesh return proxy } -func gatewayBindAddresses(ingress *structs.ConsulIngressConfigEntry) map[string]*structs.ConsulGatewayBindAddress { +func gatewayBindAddressesIngress(ingress *structs.ConsulIngressConfigEntry) map[string]*structs.ConsulGatewayBindAddress { if ingress == nil || len(ingress.Listeners) == 0 { return make(map[string]*structs.ConsulGatewayBindAddress) } @@ -361,11 +394,11 @@ func gatewayBindAddresses(ingress *structs.ConsulIngressConfigEntry) map[string] return addresses } -func newConnectGatewayTask(serviceName string, netHost bool) *structs.Task { +func newConnectGatewayTask(prefix, service string, netHost bool) *structs.Task { return &structs.Task{ // Name is used in container name so must start with '[A-Za-z0-9]' - Name: fmt.Sprintf("%s-%s", structs.ConnectIngressPrefix, serviceName), - Kind: structs.NewTaskKind(structs.ConnectIngressPrefix, serviceName), + Name: fmt.Sprintf("%s-%s", prefix, service), + Kind: structs.NewTaskKind(prefix, service), Driver: "docker", Config: connectGatewayDriverConfig(netHost), ShutdownDelay: 5 * time.Second, @@ -380,11 +413,11 @@ func newConnectGatewayTask(serviceName string, netHost bool) *structs.Task { } } -func newConnectTask(serviceName string) *structs.Task { +func newConnectSidecarTask(service string) *structs.Task { return &structs.Task{ // Name is used in container name so must start with '[A-Za-z0-9]' - Name: fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, serviceName), - Kind: structs.NewTaskKind(structs.ConnectProxyPrefix, serviceName), + Name: fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service), + Kind: structs.NewTaskKind(structs.ConnectProxyPrefix, service), Driver: "docker", Config: connectSidecarDriverConfig(), ShutdownDelay: 5 * time.Second, @@ -398,7 +431,7 @@ func newConnectTask(serviceName string) *structs.Task { Sidecar: true, }, Constraints: structs.Constraints{ - connectMinimalVersionConstraint(), + connectSidecarVersionConstraint(), }, } } diff --git a/nomad/job_endpoint_hook_connect_test.go b/nomad/job_endpoint_hook_connect_test.go index 494dca56c17..4b197aedce8 100644 --- a/nomad/job_endpoint_hook_connect_test.go +++ b/nomad/job_endpoint_hook_connect_test.go @@ -86,8 +86,8 @@ func TestJobEndpointConnect_groupConnectHook(t *testing.T) { // Expected tasks tgExp := job.TaskGroups[0].Copy() tgExp.Tasks = []*structs.Task{ - newConnectTask("backend"), - newConnectTask("admin"), + newConnectSidecarTask("backend"), + newConnectSidecarTask("admin"), } tgExp.Services[0].Name = "backend" tgExp.Services[1].Name = "admin" @@ -129,7 +129,7 @@ func TestJobEndpointConnect_groupConnectHook_IngressGateway(t *testing.T) { expTG := job.TaskGroups[0].Copy() expTG.Tasks = []*structs.Task{ // inject the gateway task - newConnectGatewayTask("my-gateway", false), + newConnectGatewayTask(structs.ConnectIngressPrefix, "my-gateway", false), } expTG.Services[0].Name = "my-gateway" expTG.Tasks[0].Canonicalize(job, expTG) @@ -328,16 +328,27 @@ func TestJobEndpointConnect_groupConnectGatewayValidate(t *testing.T) { } func TestJobEndpointConnect_newConnectGatewayTask_host(t *testing.T) { - task := newConnectGatewayTask("service1", true) - require.Equal(t, "connect-ingress-service1", task.Name) - require.Equal(t, "connect-ingress:service1", string(task.Kind)) - require.Equal(t, ">= 1.8.0", task.Constraints[0].RTarget) - require.Equal(t, "host", task.Config["network_mode"]) - require.Nil(t, task.Lifecycle) + t.Run("ingress", func(t *testing.T) { + task := newConnectGatewayTask(structs.ConnectIngressPrefix, "foo", true) + require.Equal(t, "connect-ingress-foo", task.Name) + require.Equal(t, "connect-ingress:foo", string(task.Kind)) + require.Equal(t, ">= 1.8.0", task.Constraints[0].RTarget) + require.Equal(t, "host", task.Config["network_mode"]) + require.Nil(t, task.Lifecycle) + }) + + t.Run("terminating", func(t *testing.T) { + task := newConnectGatewayTask(structs.ConnectTerminatingPrefix, "bar", true) + require.Equal(t, "connect-terminating-bar", task.Name) + require.Equal(t, "connect-terminating:bar", string(task.Kind)) + require.Equal(t, ">= 1.8.0", task.Constraints[0].RTarget) + require.Equal(t, "host", task.Config["network_mode"]) + require.Nil(t, task.Lifecycle) + }) } func TestJobEndpointConnect_newConnectGatewayTask_bridge(t *testing.T) { - task := newConnectGatewayTask("service1", false) + task := newConnectGatewayTask(structs.ConnectIngressPrefix, "service1", false) require.NotContains(t, task.Config, "network_mode") } @@ -353,19 +364,27 @@ func TestJobEndpointConnect_hasGatewayTaskForService(t *testing.T) { require.False(t, result) }) - t.Run("has gateway task", func(t *testing.T) { + t.Run("has ingress task", func(t *testing.T) { result := hasGatewayTaskForService(&structs.TaskGroup{ Name: "group", Tasks: []*structs.Task{{ - Name: "task1", - Kind: "", - }, { Name: "ingress-gateway-my-service", Kind: structs.NewTaskKind(structs.ConnectIngressPrefix, "my-service"), }}, }, "my-service") require.True(t, result) }) + + t.Run("has terminating task", func(t *testing.T) { + result := hasGatewayTaskForService(&structs.TaskGroup{ + Name: "group", + Tasks: []*structs.Task{{ + Name: "terminating-gateway-my-service", + Kind: structs.NewTaskKind(structs.ConnectTerminatingPrefix, "my-service"), + }}, + }, "my-service") + require.True(t, result) + }) } func TestJobEndpointConnect_gatewayProxyIsDefault(t *testing.T) { @@ -411,17 +430,18 @@ func TestJobEndpointConnect_gatewayProxyIsDefault(t *testing.T) { func TestJobEndpointConnect_gatewayBindAddresses(t *testing.T) { t.Run("nil", func(t *testing.T) { - result := gatewayBindAddresses(nil) + + result := gatewayBindAddressesIngress(nil) require.Empty(t, result) }) t.Run("no listeners", func(t *testing.T) { - result := gatewayBindAddresses(&structs.ConsulIngressConfigEntry{Listeners: nil}) + result := gatewayBindAddressesIngress(&structs.ConsulIngressConfigEntry{Listeners: nil}) require.Empty(t, result) }) t.Run("simple", func(t *testing.T) { - result := gatewayBindAddresses(&structs.ConsulIngressConfigEntry{ + result := gatewayBindAddressesIngress(&structs.ConsulIngressConfigEntry{ Listeners: []*structs.ConsulIngressListener{{ Port: 3000, Protocol: "tcp", @@ -439,7 +459,7 @@ func TestJobEndpointConnect_gatewayBindAddresses(t *testing.T) { }) t.Run("complex", func(t *testing.T) { - result := gatewayBindAddresses(&structs.ConsulIngressConfigEntry{ + result := gatewayBindAddressesIngress(&structs.ConsulIngressConfigEntry{ Listeners: []*structs.ConsulIngressListener{{ Port: 3000, Protocol: "tcp", @@ -503,7 +523,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }, result) }) - t.Run("fill in defaults", func(t *testing.T) { + t.Run("ingress set defaults", func(t *testing.T) { result := gatewayProxyForBridge(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), @@ -532,7 +552,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }, result) }) - t.Run("leave as-is", func(t *testing.T) { + t.Run("ingress leave as-is", func(t *testing.T) { result := gatewayProxyForBridge(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ Config: map[string]interface{}{"foo": 1}, @@ -555,4 +575,38 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { EnvoyGatewayBindAddresses: nil, }, result) }) + + t.Run("terminating set defaults", func(t *testing.T) { + result := gatewayProxyForBridge(&structs.ConsulGateway{ + Proxy: &structs.ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(2 * time.Second), + EnvoyDNSDiscoveryType: "STRICT_DNS", + }, + Terminating: &structs.ConsulTerminatingConfigEntry{ + Services: []*structs.ConsulLinkedService{{ + Name: "service1", + CAFile: "/cafile.pem", + CertFile: "/certfile.pem", + KeyFile: "/keyfile.pem", + SNI: "", + }}, + }, + }) + require.Equal(t, &structs.ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(2 * time.Second), + EnvoyGatewayNoDefaultBind: true, + EnvoyGatewayBindTaggedAddresses: false, + EnvoyDNSDiscoveryType: "STRICT_DNS", + EnvoyGatewayBindAddresses: map[string]*structs.ConsulGatewayBindAddress{ + "default": { + Address: "0.0.0.0", + Port: -1, + }, + }, + }, result) + }) + + t.Run("terminating leave as-is", func(t *testing.T) { + // + }) } diff --git a/nomad/node_endpoint_test.go b/nomad/node_endpoint_test.go index b1bc89c356c..fbc82d8b7a0 100644 --- a/nomad/node_endpoint_test.go +++ b/nomad/node_endpoint_test.go @@ -3178,7 +3178,7 @@ func TestClientEndpoint_taskUsesConnect(t *testing.T) { t.Run("task uses connect", func(t *testing.T) { try(t, &structs.Task{ - // see nomad.newConnectTask for how this works + // see nomad.newConnectSidecarTask for how this works Name: "connect-proxy-myservice", Kind: "connect-proxy:myservice", }, true) diff --git a/nomad/structs/connect.go b/nomad/structs/connect.go new file mode 100644 index 00000000000..79703876bfe --- /dev/null +++ b/nomad/structs/connect.go @@ -0,0 +1,33 @@ +package structs + +// ConsulConfigEntries represents Consul ConfigEntry definitions from a job. +type ConsulConfigEntries struct { + Ingress map[string]*ConsulIngressConfigEntry + Terminating map[string]*ConsulTerminatingConfigEntry + // Mesh later +} + +// ConfigEntries accumulates the Consul Configuration Entries defined in task groups +// of j. +func (j *Job) ConfigEntries() *ConsulConfigEntries { + entries := &ConsulConfigEntries{ + Ingress: make(map[string]*ConsulIngressConfigEntry), + Terminating: make(map[string]*ConsulTerminatingConfigEntry), + // Mesh later + } + + for _, tg := range j.TaskGroups { + for _, service := range tg.Services { + if service.Connect.IsGateway() { + gateway := service.Connect.Gateway + if ig := gateway.Ingress; ig != nil { + entries.Ingress[service.Name] = ig + } else if tg := gateway.Terminating; tg != nil { + entries.Terminating[service.Name] = tg + } // mesh later + } + } + } + + return entries +} diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 2a30d2f6360..6a08e7c4e7b 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -827,12 +827,18 @@ func connectGatewayDiff(prev, next *ConsulGateway, contextual bool) *ObjectDiff diff.Objects = append(diff.Objects, gatewayProxyDiff) } - // Diff the ConsulGatewayIngress fields. + // Diff the ingress gateway fields. gatewayIngressDiff := connectGatewayIngressDiff(prev.Ingress, next.Ingress, contextual) if gatewayIngressDiff != nil { diff.Objects = append(diff.Objects, gatewayIngressDiff) } + // Diff the terminating gateway fields. + gatewayTerminatingDiff := connectGatewayTerminatingDiff(prev.Terminating, next.Terminating, contextual) + if gatewayTerminatingDiff != nil { + diff.Objects = append(diff.Objects, gatewayTerminatingDiff) + } + return diff } @@ -874,6 +880,99 @@ func connectGatewayIngressDiff(prev, next *ConsulIngressConfigEntry, contextual return diff } +func connectGatewayTerminatingDiff(prev, next *ConsulTerminatingConfigEntry, contextual bool) *ObjectDiff { + diff := &ObjectDiff{Type: DiffTypeNone, Name: "Terminating"} + var oldPrimitiveFlat, newPrimitiveFlat map[string]string + + if reflect.DeepEqual(prev, next) { + return nil + } else if prev == nil { + prev = new(ConsulTerminatingConfigEntry) + diff.Type = DiffTypeAdded + newPrimitiveFlat = flatmap.Flatten(next, nil, true) + } else if next == nil { + next = new(ConsulTerminatingConfigEntry) + diff.Type = DiffTypeDeleted + oldPrimitiveFlat = flatmap.Flatten(prev, nil, true) + } else { + diff.Type = DiffTypeEdited + oldPrimitiveFlat = flatmap.Flatten(prev, nil, true) + newPrimitiveFlat = flatmap.Flatten(next, nil, true) + } + + // Diff the primitive fields. + diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, contextual) + + // Diff the Services lists. + gatewayLinkedServicesDiff := connectGatewayTerminatingLinkedServicesDiff(prev.Services, next.Services, contextual) + if gatewayLinkedServicesDiff != nil { + diff.Objects = append(diff.Objects, gatewayLinkedServicesDiff...) + } + + return diff +} + +// connectGatewayTerminatingLinkedServicesDiff diffs are a set of services keyed +// by service name. These objects contain only fields. +func connectGatewayTerminatingLinkedServicesDiff(prev, next []*ConsulLinkedService, contextual bool) []*ObjectDiff { + // create maps, diff the maps, key by linked service name + + prevMap := make(map[string]*ConsulLinkedService, len(prev)) + nextMap := make(map[string]*ConsulLinkedService, len(next)) + + for _, s := range prev { + prevMap[s.Name] = s + } + for _, s := range next { + nextMap[s.Name] = s + } + + var diffs []*ObjectDiff + for k, prevS := range prevMap { + // Diff the same, deleted, and edited + if diff := connectGatewayTerminatingLinkedServiceDiff(prevS, nextMap[k], contextual); diff != nil { + diffs = append(diffs, diff) + } + } + for k, nextS := range nextMap { + // Diff the added + if old, ok := prevMap[k]; !ok { + if diff := connectGatewayTerminatingLinkedServiceDiff(old, nextS, contextual); diff != nil { + diffs = append(diffs, diff) + } + } + } + + sort.Sort(ObjectDiffs(diffs)) + return diffs +} + +func connectGatewayTerminatingLinkedServiceDiff(prev, next *ConsulLinkedService, contextual bool) *ObjectDiff { + diff := &ObjectDiff{Type: DiffTypeNone, Name: "Service"} + var oldPrimitiveFlat, newPrimitiveFlat map[string]string + + if reflect.DeepEqual(prev, next) { + return nil + } else if prev == nil { + diff.Type = DiffTypeAdded + newPrimitiveFlat = flatmap.Flatten(next, nil, true) + } else if next == nil { + diff.Type = DiffTypeDeleted + oldPrimitiveFlat = flatmap.Flatten(prev, nil, true) + } else { + diff.Type = DiffTypeEdited + oldPrimitiveFlat = flatmap.Flatten(prev, nil, true) + newPrimitiveFlat = flatmap.Flatten(next, nil, true) + } + + // Diff the primitive fields. + diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, contextual) + + // No objects today. + + return diff +} + func connectGatewayTLSConfigDiff(prev, next *ConsulGatewayTLSConfig, contextual bool) *ObjectDiff { diff := &ObjectDiff{Type: DiffTypeNone, Name: "TLS"} var oldPrimitiveFlat, newPrimitiveFlat map[string]string @@ -900,7 +999,7 @@ func connectGatewayTLSConfigDiff(prev, next *ConsulGatewayTLSConfig, contextual // connectGatewayIngressListenersDiff diffs are a set of listeners keyed by "protocol/port", which is // a nifty workaround having slices instead of maps. Presumably such a key will be unique, because if -// it is not the config entry is not going to work anyway. +// if is not the config entry is not going to work anyway. func connectGatewayIngressListenersDiff(prev, next []*ConsulIngressListener, contextual bool) []*ObjectDiff { // create maps, diff the maps, keys are fields, keys are (port+protocol) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index be83b64917d..46c94a8828f 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -2630,6 +2630,7 @@ func TestTaskGroupDiff(t *testing.T) { Port: 2001, }, }, + EnvoyDNSDiscoveryType: "STRICT_DNS", EnvoyGatewayNoDefaultBind: false, Config: map[string]interface{}{ "foo": 1, @@ -2647,6 +2648,15 @@ func TestTaskGroupDiff(t *testing.T) { }}, }}, }, + Terminating: &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "linked1", + CAFile: "ca1.pem", + CertFile: "cert1.pem", + KeyFile: "key1.pem", + SNI: "linked1.consul", + }}, + }, }, }, }, @@ -2705,6 +2715,7 @@ func TestTaskGroupDiff(t *testing.T) { Port: 2002, }, }, + EnvoyDNSDiscoveryType: "LOGICAL_DNS", EnvoyGatewayNoDefaultBind: true, Config: map[string]interface{}{ "foo": 2, @@ -2723,6 +2734,15 @@ func TestTaskGroupDiff(t *testing.T) { }}, }}, }, + Terminating: &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "linked2", + CAFile: "ca2.pem", + CertFile: "cert2.pem", + KeyFile: "key2.pem", + SNI: "linked2.consul", + }}, + }, }, }, }, @@ -3031,6 +3051,12 @@ func TestTaskGroupDiff(t *testing.T) { Old: "1s", New: "2s", }, + { + Type: DiffTypeEdited, + Name: "EnvoyDNSDiscoveryType", + Old: "STRICT_DNS", + New: "LOGICAL_DNS", + }, { Type: DiffTypeEdited, Name: "EnvoyGatewayBindTaggedAddresses", @@ -3173,6 +3199,84 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, + { + Type: DiffTypeEdited, + Name: "Terminating", + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "CAFile", + Old: "", + New: "ca2.pem", + }, + { + Type: DiffTypeAdded, + Name: "CertFile", + Old: "", + New: "cert2.pem", + }, + { + Type: DiffTypeAdded, + Name: "KeyFile", + Old: "", + New: "key2.pem", + }, + { + Type: DiffTypeAdded, + Name: "Name", + Old: "", + New: "linked2", + }, + { + Type: DiffTypeAdded, + Name: "SNI", + Old: "", + New: "linked2.consul", + }, + }, + }, + { + Type: DiffTypeDeleted, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "CAFile", + Old: "ca1.pem", + New: "", + }, + { + Type: DiffTypeDeleted, + Name: "CertFile", + Old: "cert1.pem", + New: "", + }, + { + Type: DiffTypeDeleted, + Name: "KeyFile", + Old: "key1.pem", + New: "", + }, + { + Type: DiffTypeDeleted, + Name: "Name", + Old: "linked1", + New: "", + }, + { + Type: DiffTypeDeleted, + Name: "SNI", + Old: "linked1.consul", + New: "", + }, + }, + }, + }, + }, }, }, }, diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 2ca1dd81f2d..073577b3c1d 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -733,11 +733,23 @@ func (c *ConsulConnect) IsNative() bool { return c != nil && c.Native } -// IsGateway checks if the service is a Connect gateway. +// IsGateway checks if the service is any type of connect gateway. func (c *ConsulConnect) IsGateway() bool { return c != nil && c.Gateway != nil } +// IsIngress checks if the service is an ingress gateway. +func (c *ConsulConnect) IsIngress() bool { + return c.IsGateway() && c.Gateway.Ingress != nil +} + +// IsTerminating checks if the service is a terminating gateway. +func (c *ConsulConnect) IsTerminating() bool { + return c.IsGateway() && c.Gateway.Terminating != nil +} + +// also mesh + // Validate that the Connect block represents exactly one of: // - Connect non-native service sidecar proxy // - Connect native service @@ -1231,21 +1243,32 @@ type ConsulGateway struct { // Ingress represents the Consul Configuration Entry for an Ingress Gateway. Ingress *ConsulIngressConfigEntry - // Terminating is not yet supported. - // Terminating *ConsulTerminatingConfigEntry + // Terminating represents the Consul Configuration Entry for a Terminating Gateway. + Terminating *ConsulTerminatingConfigEntry // Mesh is not yet supported. // Mesh *ConsulMeshConfigEntry } +func (g *ConsulGateway) Prefix() string { + switch { + case g.Ingress != nil: + return ConnectIngressPrefix + default: + return ConnectTerminatingPrefix + } + // also mesh +} + func (g *ConsulGateway) Copy() *ConsulGateway { if g == nil { return nil } return &ConsulGateway{ - Proxy: g.Proxy.Copy(), - Ingress: g.Ingress.Copy(), + Proxy: g.Proxy.Copy(), + Ingress: g.Ingress.Copy(), + Terminating: g.Terminating.Copy(), } } @@ -1262,6 +1285,10 @@ func (g *ConsulGateway) Equals(o *ConsulGateway) bool { return false } + if !g.Terminating.Equals(o.Terminating) { + return false + } + return true } @@ -1270,18 +1297,30 @@ func (g *ConsulGateway) Validate() error { return nil } - if g.Proxy != nil { - if err := g.Proxy.Validate(); err != nil { - return err - } + if err := g.Proxy.Validate(); err != nil { + return err } - // eventually one of: ingress, terminating, mesh - if g.Ingress != nil { - return g.Ingress.Validate() + if err := g.Ingress.Validate(); err != nil { + return err + } + + if err := g.Terminating.Validate(); err != nil { + return err } - return fmt.Errorf("Consul Gateway ingress Configuration Entry must be set") + // Exactly 1 of ingress/terminating/mesh(soon) must be set. + count := 0 + if g.Ingress != nil { + count++ + } + if g.Terminating != nil { + count++ + } + if count != 1 { + return fmt.Errorf("One Consul Gateway Configuration Entry must be set") + } + return nil } // ConsulGatewayBindAddress is equivalent to Consul's api/catalog.go ServiceAddress @@ -1328,7 +1367,7 @@ func (a *ConsulGatewayBindAddress) Validate() error { return fmt.Errorf("Consul Gateway Bind Address must be set") } - if a.Port <= 0 { + if a.Port <= 0 && a.Port != -1 { // port -1 => nomad autofill return fmt.Errorf("Consul Gateway Bind Address must set valid Port") } @@ -1344,6 +1383,7 @@ type ConsulGatewayProxy struct { EnvoyGatewayBindTaggedAddresses bool EnvoyGatewayBindAddresses map[string]*ConsulGatewayBindAddress EnvoyGatewayNoDefaultBind bool + EnvoyDNSDiscoveryType string Config map[string]interface{} } @@ -1352,20 +1392,29 @@ func (p *ConsulGatewayProxy) Copy() *ConsulGatewayProxy { return nil } - bindAddresses := make(map[string]*ConsulGatewayBindAddress, len(p.EnvoyGatewayBindAddresses)) - for k, v := range p.EnvoyGatewayBindAddresses { - bindAddresses[k] = v.Copy() - } - return &ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(*p.ConnectTimeout), EnvoyGatewayBindTaggedAddresses: p.EnvoyGatewayBindTaggedAddresses, - EnvoyGatewayBindAddresses: bindAddresses, + EnvoyGatewayBindAddresses: p.copyBindAddresses(), EnvoyGatewayNoDefaultBind: p.EnvoyGatewayNoDefaultBind, + EnvoyDNSDiscoveryType: p.EnvoyDNSDiscoveryType, Config: helper.CopyMapStringInterface(p.Config), } } +func (p *ConsulGatewayProxy) copyBindAddresses() map[string]*ConsulGatewayBindAddress { + if len(p.EnvoyGatewayBindAddresses) == 0 { + return nil + } + + bindAddresses := make(map[string]*ConsulGatewayBindAddress, len(p.EnvoyGatewayBindAddresses)) + for k, v := range p.EnvoyGatewayBindAddresses { + bindAddresses[k] = v.Copy() + } + + return bindAddresses +} + func (p *ConsulGatewayProxy) equalBindAddresses(o map[string]*ConsulGatewayBindAddress) bool { if len(p.EnvoyGatewayBindAddresses) != len(o) { return false @@ -1401,6 +1450,10 @@ func (p *ConsulGatewayProxy) Equals(o *ConsulGatewayProxy) bool { return false } + if p.EnvoyDNSDiscoveryType != o.EnvoyDNSDiscoveryType { + return false + } + if !opaqueMapsEqual(p.Config, o.Config) { return false } @@ -1408,6 +1461,11 @@ func (p *ConsulGatewayProxy) Equals(o *ConsulGatewayProxy) bool { return true } +const ( + strictDNS = "STRICT_DNS" + logicalDNS = "LOGICAL_DNS" +) + func (p *ConsulGatewayProxy) Validate() error { if p == nil { return nil @@ -1417,6 +1475,14 @@ func (p *ConsulGatewayProxy) Validate() error { return fmt.Errorf("Consul Gateway Proxy connection_timeout must be set") } + switch p.EnvoyDNSDiscoveryType { + case "", strictDNS, logicalDNS: + // Consul defaults to logical DNS, suitable for large scale workloads. + // https://www.envoyproxy.io/docs/envoy/v1.16.1/intro/arch_overview/upstream/service_discovery + default: + return fmt.Errorf("Consul Gateway Proxy Envoy DNS Discovery type must be %s or %s", strictDNS, logicalDNS) + } + for _, bindAddr := range p.EnvoyGatewayBindAddresses { if err := bindAddr.Validate(); err != nil { return err @@ -1671,3 +1737,143 @@ COMPARE: // order does not matter } return true } + +type ConsulLinkedService struct { + Name string + CAFile string + CertFile string + KeyFile string + SNI string +} + +func (s *ConsulLinkedService) Copy() *ConsulLinkedService { + if s == nil { + return nil + } + + return &ConsulLinkedService{ + Name: s.Name, + CAFile: s.CAFile, + CertFile: s.CertFile, + KeyFile: s.KeyFile, + SNI: s.SNI, + } +} + +func (s *ConsulLinkedService) Equals(o *ConsulLinkedService) bool { + if s == nil || o == nil { + return s == o + } + + switch { + case s.Name != o.Name: + return false + case s.CAFile != o.CAFile: + return false + case s.CertFile != o.CertFile: + return false + case s.KeyFile != o.KeyFile: + return false + case s.SNI != o.SNI: + return false + } + + return true +} + +func (s *ConsulLinkedService) Validate() error { + if s == nil { + return nil + } + + if s.Name == "" { + return fmt.Errorf("Consul Linked Service requires Name") + } + + caSet := s.CAFile != "" + certSet := s.CertFile != "" + keySet := s.KeyFile != "" + sniSet := s.SNI != "" + + if (certSet || keySet) && !caSet { + return fmt.Errorf("Consul Linked Service TLS requires CAFile") + } + + if certSet != keySet { + return fmt.Errorf("Consul Linked Service TLS Cert and Key must both be set") + } + + if sniSet && !caSet { + return fmt.Errorf("Consul Linked Service TLS SNI requires CAFile") + } + + return nil +} + +func linkedServicesEqual(servicesA, servicesB []*ConsulLinkedService) bool { + if len(servicesA) != len(servicesB) { + return false + } + +COMPARE: // order does not matter + for _, serviceA := range servicesA { + for _, serviceB := range servicesB { + if serviceA.Equals(serviceB) { + continue COMPARE + } + } + return false + } + return true +} + +type ConsulTerminatingConfigEntry struct { + // Namespace is not yet supported. + // Namespace string + + Services []*ConsulLinkedService +} + +func (e *ConsulTerminatingConfigEntry) Copy() *ConsulTerminatingConfigEntry { + if e == nil { + return nil + } + + var services []*ConsulLinkedService = nil + if n := len(e.Services); n > 0 { + services = make([]*ConsulLinkedService, n) + for i := 0; i < n; i++ { + services[i] = e.Services[i].Copy() + } + } + + return &ConsulTerminatingConfigEntry{ + Services: services, + } +} + +func (e *ConsulTerminatingConfigEntry) Equals(o *ConsulTerminatingConfigEntry) bool { + if e == nil || o == nil { + return e == o + } + + return linkedServicesEqual(e.Services, o.Services) +} + +func (e *ConsulTerminatingConfigEntry) Validate() error { + if e == nil { + return nil + } + + if len(e.Services) == 0 { + return fmt.Errorf("Consul Terminating Gateway requires at least one service") + } + + for _, service := range e.Services { + if err := service.Validate(); err != nil { + return err + } + } + + return nil +} diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go index 493c6fefcc2..68182bb21e7 100644 --- a/nomad/structs/services_test.go +++ b/nomad/structs/services_test.go @@ -577,8 +577,8 @@ var ( ConnectTimeout: helper.TimeToPtr(1 * time.Second), EnvoyGatewayBindTaggedAddresses: true, EnvoyGatewayBindAddresses: map[string]*ConsulGatewayBindAddress{ - "listener1": &ConsulGatewayBindAddress{Address: "10.0.0.1", Port: 2001}, - "listener2": &ConsulGatewayBindAddress{Address: "10.0.0.1", Port: 2002}, + "listener1": {Address: "10.0.0.1", Port: 2001}, + "listener2": {Address: "10.0.0.1", Port: 2002}, }, EnvoyGatewayNoDefaultBind: true, Config: map[string]interface{}{ @@ -608,8 +608,41 @@ var ( }}, }, } + + consulTerminatingGateway1 = &ConsulGateway{ + Proxy: &ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(1 * time.Second), + EnvoyDNSDiscoveryType: "STRICT_DNS", + EnvoyGatewayBindAddresses: nil, + }, + Terminating: &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "linked-service1", + CAFile: "ca.pem", + CertFile: "cert.pem", + KeyFile: "key.pem", + SNI: "service1.consul", + }, { + Name: "linked-service2", + }}, + }, + } ) +func TestConsulGateway_Prefix(t *testing.T) { + t.Run("ingress", func(t *testing.T) { + result := (&ConsulGateway{Ingress: new(ConsulIngressConfigEntry)}).Prefix() + require.Equal(t, ConnectIngressPrefix, result) + }) + + t.Run("terminating", func(t *testing.T) { + result := (&ConsulGateway{Terminating: new(ConsulTerminatingConfigEntry)}).Prefix() + require.Equal(t, ConnectTerminatingPrefix, result) + }) + + // also mesh +} + func TestConsulGateway_Copy(t *testing.T) { t.Parallel() @@ -625,6 +658,13 @@ func TestConsulGateway_Copy(t *testing.T) { require.True(t, result.Equals(consulIngressGateway1)) require.True(t, consulIngressGateway1.Equals(result)) }) + + t.Run("as terminating", func(t *testing.T) { + result := consulTerminatingGateway1.Copy() + require.Equal(t, consulTerminatingGateway1, result) + require.True(t, result.Equals(consulTerminatingGateway1)) + require.True(t, consulTerminatingGateway1.Equals(result)) + }) } func TestConsulGateway_Equals_ingress(t *testing.T) { @@ -640,8 +680,8 @@ func TestConsulGateway_Equals_ingress(t *testing.T) { original := consulIngressGateway1.Copy() - type gway = ConsulGateway - type tweaker = func(g *gway) + type cg = ConsulGateway + type tweaker = func(g *cg) t.Run("reflexive", func(t *testing.T) { require.True(t, original.Equals(original)) @@ -658,27 +698,27 @@ func TestConsulGateway_Equals_ingress(t *testing.T) { // proxy stanza equality checks t.Run("mod gateway timeout", func(t *testing.T) { - try(t, func(g *gway) { g.Proxy.ConnectTimeout = helper.TimeToPtr(9 * time.Second) }) + try(t, func(g *cg) { g.Proxy.ConnectTimeout = helper.TimeToPtr(9 * time.Second) }) }) t.Run("mod gateway envoy_gateway_bind_tagged_addresses", func(t *testing.T) { - try(t, func(g *gway) { g.Proxy.EnvoyGatewayBindTaggedAddresses = false }) + try(t, func(g *cg) { g.Proxy.EnvoyGatewayBindTaggedAddresses = false }) }) t.Run("mod gateway envoy_gateway_bind_addresses", func(t *testing.T) { - try(t, func(g *gway) { + try(t, func(g *cg) { g.Proxy.EnvoyGatewayBindAddresses = map[string]*ConsulGatewayBindAddress{ - "listener3": &ConsulGatewayBindAddress{Address: "9.9.9.9", Port: 9999}, + "listener3": {Address: "9.9.9.9", Port: 9999}, } }) }) t.Run("mod gateway envoy_gateway_no_default_bind", func(t *testing.T) { - try(t, func(g *gway) { g.Proxy.EnvoyGatewayNoDefaultBind = false }) + try(t, func(g *cg) { g.Proxy.EnvoyGatewayNoDefaultBind = false }) }) t.Run("mod gateway config", func(t *testing.T) { - try(t, func(g *gway) { + try(t, func(g *cg) { g.Proxy.Config = map[string]interface{}{ "foo": 2, } @@ -688,40 +728,95 @@ func TestConsulGateway_Equals_ingress(t *testing.T) { // ingress config entry equality checks t.Run("mod ingress tls", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.TLS = nil }) - try(t, func(g *gway) { g.Ingress.TLS.Enabled = false }) + try(t, func(g *cg) { g.Ingress.TLS = nil }) + try(t, func(g *cg) { g.Ingress.TLS.Enabled = false }) }) t.Run("mod ingress listeners count", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners = g.Ingress.Listeners[:1] }) + try(t, func(g *cg) { g.Ingress.Listeners = g.Ingress.Listeners[:1] }) }) t.Run("mod ingress listeners port", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners[0].Port = 7777 }) + try(t, func(g *cg) { g.Ingress.Listeners[0].Port = 7777 }) }) t.Run("mod ingress listeners protocol", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners[0].Protocol = "tcp" }) + try(t, func(g *cg) { g.Ingress.Listeners[0].Protocol = "tcp" }) }) t.Run("mod ingress listeners services count", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners[0].Services = g.Ingress.Listeners[0].Services[:1] }) + try(t, func(g *cg) { g.Ingress.Listeners[0].Services = g.Ingress.Listeners[0].Services[:1] }) }) t.Run("mod ingress listeners services name", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners[0].Services[0].Name = "serviceX" }) + try(t, func(g *cg) { g.Ingress.Listeners[0].Services[0].Name = "serviceX" }) }) t.Run("mod ingress listeners services hosts count", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners[0].Services[0].Hosts = g.Ingress.Listeners[0].Services[0].Hosts[:1] }) + try(t, func(g *cg) { g.Ingress.Listeners[0].Services[0].Hosts = g.Ingress.Listeners[0].Services[0].Hosts[:1] }) }) t.Run("mod ingress listeners services hosts content", func(t *testing.T) { - try(t, func(g *gway) { g.Ingress.Listeners[0].Services[0].Hosts[0] = "255.255.255.255" }) + try(t, func(g *cg) { g.Ingress.Listeners[0].Services[0].Hosts[0] = "255.255.255.255" }) + }) +} + +func TestConsulGateway_Equals_terminating(t *testing.T) { + t.Parallel() + + original := consulTerminatingGateway1.Copy() + + type cg = ConsulGateway + type tweaker = func(c *cg) + + t.Run("reflexive", func(t *testing.T) { + require.True(t, original.Equals(original)) + }) + + try := func(t *testing.T, tweak tweaker) { + modifiable := original.Copy() + tweak(modifiable) + require.False(t, original.Equals(modifiable)) + require.False(t, modifiable.Equals(original)) + require.True(t, modifiable.Equals(modifiable)) + } + + // proxy stanza equality checks + + t.Run("mod dns discovery type", func(t *testing.T) { + try(t, func(g *cg) { g.Proxy.EnvoyDNSDiscoveryType = "LOGICAL_DNS" }) + }) + + // terminating config entry equality checks + + t.Run("mod terminating services count", func(t *testing.T) { + try(t, func(g *cg) { g.Terminating.Services = g.Terminating.Services[:1] }) + }) + + t.Run("mod terminating services name", func(t *testing.T) { + try(t, func(g *cg) { g.Terminating.Services[0].Name = "foo" }) + }) + + t.Run("mod terminating services ca_file", func(t *testing.T) { + try(t, func(g *cg) { g.Terminating.Services[0].CAFile = "foo.pem" }) + }) + + t.Run("mod terminating services cert_file", func(t *testing.T) { + try(t, func(g *cg) { g.Terminating.Services[0].CertFile = "foo.pem" }) + }) + + t.Run("mod terminating services key_file", func(t *testing.T) { + try(t, func(g *cg) { g.Terminating.Services[0].KeyFile = "foo.pem" }) + }) + + t.Run("mod terminating services sni", func(t *testing.T) { + try(t, func(g *cg) { g.Terminating.Services[0].SNI = "foo.consul" }) }) } func TestConsulGateway_ingressServicesEqual(t *testing.T) { + t.Parallel() + igs1 := []*ConsulIngressService{{ Name: "service1", Hosts: []string{"host1", "host2"}, @@ -731,6 +826,7 @@ func TestConsulGateway_ingressServicesEqual(t *testing.T) { }} require.False(t, ingressServicesEqual(igs1, nil)) + require.True(t, ingressServicesEqual(igs1, igs1)) reversed := []*ConsulIngressService{ igs1[1], igs1[0], // services reversed @@ -750,6 +846,8 @@ func TestConsulGateway_ingressServicesEqual(t *testing.T) { } func TestConsulGateway_ingressListenersEqual(t *testing.T) { + t.Parallel() + ils1 := []*ConsulIngressListener{{ Port: 2000, Protocol: "http", @@ -775,6 +873,8 @@ func TestConsulGateway_ingressListenersEqual(t *testing.T) { } func TestConsulGateway_Validate(t *testing.T) { + t.Parallel() + t.Run("bad proxy", func(t *testing.T) { err := (&ConsulGateway{ Proxy: &ConsulGatewayProxy{ @@ -793,9 +893,48 @@ func TestConsulGateway_Validate(t *testing.T) { }).Validate() require.EqualError(t, err, "Consul Ingress Gateway requires at least one listener") }) + + t.Run("bad terminating config entry", func(t *testing.T) { + err := (&ConsulGateway{ + Terminating: &ConsulTerminatingConfigEntry{ + Services: nil, + }, + }).Validate() + require.EqualError(t, err, "Consul Terminating Gateway requires at least one service") + }) + + t.Run("no config entry set", func(t *testing.T) { + err := (&ConsulGateway{ + Ingress: nil, + Terminating: nil, + }).Validate() + require.EqualError(t, err, "One Consul Gateway Configuration Entry must be set") + }) + + t.Run("multiple config entries set", func(t *testing.T) { + err := (&ConsulGateway{ + Ingress: &ConsulIngressConfigEntry{ + Listeners: []*ConsulIngressListener{{ + Port: 1111, + Protocol: "tcp", + Services: []*ConsulIngressService{{ + Name: "service1", + }}, + }}, + }, + Terminating: &ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "linked-service1", + }}, + }, + }).Validate() + require.EqualError(t, err, "One Consul Gateway Configuration Entry must be set") + }) } func TestConsulGatewayBindAddress_Validate(t *testing.T) { + t.Parallel() + t.Run("no address", func(t *testing.T) { err := (&ConsulGatewayBindAddress{ Address: "", @@ -822,6 +961,8 @@ func TestConsulGatewayBindAddress_Validate(t *testing.T) { } func TestConsulGatewayProxy_Validate(t *testing.T) { + t.Parallel() + t.Run("no timeout", func(t *testing.T) { err := (&ConsulGatewayProxy{ ConnectTimeout: nil, @@ -841,6 +982,14 @@ func TestConsulGatewayProxy_Validate(t *testing.T) { require.EqualError(t, err, "Consul Gateway Bind Address must set valid Port") }) + t.Run("invalid dns discovery type", func(t *testing.T) { + err := (&ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(1 * time.Second), + EnvoyDNSDiscoveryType: "RANDOM_DNS", + }).Validate() + require.EqualError(t, err, "Consul Gateway Proxy Envoy DNS Discovery type must be STRICT_DNS or LOGICAL_DNS") + }) + t.Run("ok with nothing set", func(t *testing.T) { err := (&ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(1 * time.Second), @@ -864,6 +1013,8 @@ func TestConsulGatewayProxy_Validate(t *testing.T) { } func TestConsulIngressService_Validate(t *testing.T) { + t.Parallel() + t.Run("invalid name", func(t *testing.T) { err := (&ConsulIngressService{ Name: "", @@ -903,6 +1054,8 @@ func TestConsulIngressService_Validate(t *testing.T) { } func TestConsulIngressListener_Validate(t *testing.T) { + t.Parallel() + t.Run("invalid port", func(t *testing.T) { err := (&ConsulIngressListener{ Port: 0, @@ -958,6 +1111,8 @@ func TestConsulIngressListener_Validate(t *testing.T) { } func TestConsulIngressConfigEntry_Validate(t *testing.T) { + t.Parallel() + t.Run("no listeners", func(t *testing.T) { err := (&ConsulIngressConfigEntry{}).Validate() require.EqualError(t, err, "Consul Ingress Gateway requires at least one listener") @@ -989,3 +1144,172 @@ func TestConsulIngressConfigEntry_Validate(t *testing.T) { require.NoError(t, err) }) } + +func TestConsulLinkedService_Validate(t *testing.T) { + t.Parallel() + + t.Run("nil", func(t *testing.T) { + err := (*ConsulLinkedService)(nil).Validate() + require.Nil(t, err) + }) + + t.Run("missing name", func(t *testing.T) { + err := (&ConsulLinkedService{}).Validate() + require.EqualError(t, err, "Consul Linked Service requires Name") + }) + + t.Run("missing cafile", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + CertFile: "cert_file.pem", + KeyFile: "key_file.pem", + }).Validate() + require.EqualError(t, err, "Consul Linked Service TLS requires CAFile") + }) + + t.Run("mutual cert key", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + CAFile: "ca_file.pem", + CertFile: "cert_file.pem", + }).Validate() + require.EqualError(t, err, "Consul Linked Service TLS Cert and Key must both be set") + }) + + t.Run("sni without cafile", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + SNI: "service.consul", + }).Validate() + require.EqualError(t, err, "Consul Linked Service TLS SNI requires CAFile") + }) + + t.Run("minimal", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + }).Validate() + require.NoError(t, err) + }) + + t.Run("tls minimal", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + CAFile: "ca_file.pem", + }).Validate() + require.NoError(t, err) + }) + + t.Run("tls mutual", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + CAFile: "ca_file.pem", + CertFile: "cert_file.pem", + KeyFile: "key_file.pem", + }).Validate() + require.NoError(t, err) + }) + + t.Run("tls sni", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + CAFile: "ca_file.pem", + SNI: "linked-service.consul", + }).Validate() + require.NoError(t, err) + }) + + t.Run("tls complete", func(t *testing.T) { + err := (&ConsulLinkedService{ + Name: "linked-service1", + CAFile: "ca_file.pem", + CertFile: "cert_file.pem", + KeyFile: "key_file.pem", + SNI: "linked-service.consul", + }).Validate() + require.NoError(t, err) + }) +} + +func TestConsulLinkedService_Copy(t *testing.T) { + t.Parallel() + + require.Nil(t, (*ConsulLinkedService)(nil).Copy()) + require.Equal(t, &ConsulLinkedService{ + Name: "service1", + CAFile: "ca.pem", + CertFile: "cert.pem", + KeyFile: "key.pem", + SNI: "service1.consul", + }, (&ConsulLinkedService{ + Name: "service1", + CAFile: "ca.pem", + CertFile: "cert.pem", + KeyFile: "key.pem", + SNI: "service1.consul", + }).Copy()) +} + +func TestConsulLinkedService_linkedServicesEqual(t *testing.T) { + t.Parallel() + + services := []*ConsulLinkedService{{ + Name: "service1", + CAFile: "ca.pem", + }, { + Name: "service2", + CAFile: "ca.pem", + }} + + require.False(t, linkedServicesEqual(services, nil)) + require.True(t, linkedServicesEqual(services, services)) + + reversed := []*ConsulLinkedService{ + services[1], services[0], // reversed + } + + require.True(t, linkedServicesEqual(services, reversed)) + + different := []*ConsulLinkedService{ + services[0], &ConsulLinkedService{ + Name: "service2", + CAFile: "ca.pem", + SNI: "service2.consul", + }, + } + + require.False(t, linkedServicesEqual(services, different)) +} + +func TestConsulTerminatingConfigEntry_Validate(t *testing.T) { + t.Parallel() + + t.Run("nil", func(t *testing.T) { + err := (*ConsulTerminatingConfigEntry)(nil).Validate() + require.NoError(t, err) + }) + + t.Run("no services", func(t *testing.T) { + err := (&ConsulTerminatingConfigEntry{ + Services: make([]*ConsulLinkedService, 0), + }).Validate() + require.EqualError(t, err, "Consul Terminating Gateway requires at least one service") + }) + + t.Run("service invalid", func(t *testing.T) { + err := (&ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "", + }}, + }).Validate() + require.EqualError(t, err, "Consul Linked Service requires Name") + }) + + t.Run("ok", func(t *testing.T) { + err := (&ConsulTerminatingConfigEntry{ + Services: []*ConsulLinkedService{{ + Name: "service1", + }}, + }).Validate() + require.NoError(t, err) + }) +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index e8824371dc5..88a2731334b 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4385,25 +4385,6 @@ func (j *Job) ConnectTasks() []TaskKind { return kinds } -// ConfigEntries accumulates the Consul Configuration Entries defined in task groups -// of j. -// -// Currently Nomad only supports entries for connect ingress gateways. -func (j *Job) ConfigEntries() map[string]*ConsulIngressConfigEntry { - igEntries := make(map[string]*ConsulIngressConfigEntry) - for _, tg := range j.TaskGroups { - for _, service := range tg.Services { - if service.Connect.IsGateway() { - if ig := service.Connect.Gateway.Ingress; ig != nil { - igEntries[service.Name] = ig - } - // imagine also accumulating other entry types in the future - } - } - } - return igEntries -} - // RequiredSignals returns a mapping of task groups to tasks to their required // set of signals func (j *Job) RequiredSignals() map[string]map[string][]string { @@ -7129,10 +7110,16 @@ func (k TaskKind) IsConnectIngress() bool { return k.hasPrefix(ConnectIngressPrefix) } +func (k TaskKind) IsConnectTerminating() bool { + return k.hasPrefix(ConnectTerminatingPrefix) +} + func (k TaskKind) IsAnyConnectGateway() bool { switch { case k.IsConnectIngress(): return true + case k.IsConnectTerminating(): + return true default: return false } @@ -7154,8 +7141,7 @@ const ( // ConnectTerminatingPrefix is the prefix used for fields referencing a Consul // Connect Terminating Gateway Proxy. // - // Not yet supported. - // ConnectTerminatingPrefix = "connect-terminating" + ConnectTerminatingPrefix = "connect-terminating" // ConnectMeshPrefix is the prefix used for fields referencing a Consul Connect // Mesh Gateway Proxy. diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index f622250ca9b..a055d4a1a34 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -638,6 +638,12 @@ func TestJob_ConnectTasks(t *testing.T) { Name: "generator", Kind: "connect-native:uuid-api", }}, + }, { + Name: "tg5", + Tasks: []*Task{{ + Name: "t1000", + Kind: "connect-terminating:t1000", + }}, }}, } @@ -650,6 +656,7 @@ func TestJob_ConnectTasks(t *testing.T) { NewTaskKind(ConnectIngressPrefix, "ingress"), NewTaskKind(ConnectNativePrefix, "uuid-fe"), NewTaskKind(ConnectNativePrefix, "uuid-api"), + NewTaskKind(ConnectTerminatingPrefix, "t1000"), } r.Equal(exp, connectTasks) @@ -847,6 +854,15 @@ func TestTask_UsesConnect(t *testing.T) { usesConnect := task.UsesConnect() require.True(t, usesConnect) }) + + t.Run("terminating gateway", func(t *testing.T) { + task := &Task{ + Name: "task1", + Kind: NewTaskKind(ConnectTerminatingPrefix, "task1"), + } + usesConnect := task.UsesConnect() + require.True(t, usesConnect) + }) } func TestTaskGroup_UsesConnect(t *testing.T) { diff --git a/vendor/github.com/hashicorp/nomad/api/services.go b/vendor/github.com/hashicorp/nomad/api/services.go index f7e54011dee..f85e113cdfd 100644 --- a/vendor/github.com/hashicorp/nomad/api/services.go +++ b/vendor/github.com/hashicorp/nomad/api/services.go @@ -302,8 +302,8 @@ type ConsulGateway struct { // Ingress represents the Consul Configuration Entry for an Ingress Gateway. Ingress *ConsulIngressConfigEntry `hcl:"ingress,block"` - // Terminating is not yet supported. - // Terminating *ConsulTerminatingConfigEntry + // Terminating represents the Consul Configuration Entry for a Terminating Gateway. + Terminating *ConsulTerminatingConfigEntry `hcl:"terminating,block"` // Mesh is not yet supported. // Mesh *ConsulMeshConfigEntry @@ -315,6 +315,7 @@ func (g *ConsulGateway) Canonicalize() { } g.Proxy.Canonicalize() g.Ingress.Canonicalize() + g.Terminating.Canonicalize() } func (g *ConsulGateway) Copy() *ConsulGateway { @@ -323,8 +324,9 @@ func (g *ConsulGateway) Copy() *ConsulGateway { } return &ConsulGateway{ - Proxy: g.Proxy.Copy(), - Ingress: g.Ingress.Copy(), + Proxy: g.Proxy.Copy(), + Ingress: g.Ingress.Copy(), + Terminating: g.Terminating.Copy(), } } @@ -335,8 +337,8 @@ type ConsulGatewayBindAddress struct { } var ( - // defaultConnectTimeout is the default amount of time a connect gateway will - // wait for a response from an upstream service (same as consul) + // defaultGatewayConnectTimeout is the default amount of time connections to + // upstreams are allowed before timing out. defaultGatewayConnectTimeout = 5 * time.Second ) @@ -349,6 +351,7 @@ type ConsulGatewayProxy struct { EnvoyGatewayBindTaggedAddresses bool `mapstructure:"envoy_gateway_bind_tagged_addresses" hcl:"envoy_gateway_bind_tagged_addresses,optional"` EnvoyGatewayBindAddresses map[string]*ConsulGatewayBindAddress `mapstructure:"envoy_gateway_bind_addresses" hcl:"envoy_gateway_bind_addresses,block"` EnvoyGatewayNoDefaultBind bool `mapstructure:"envoy_gateway_no_default_bind" hcl:"envoy_gateway_no_default_bind,optional"` + EnvoyDNSDiscoveryType string `mapstructure:"envoy_dns_discovery_type" hcl:"envoy_dns_discovery_type,optional"` Config map[string]interface{} `hcl:"config,block"` // escape hatch envoy config } @@ -397,6 +400,7 @@ func (p *ConsulGatewayProxy) Copy() *ConsulGatewayProxy { EnvoyGatewayBindTaggedAddresses: p.EnvoyGatewayBindTaggedAddresses, EnvoyGatewayBindAddresses: binds, EnvoyGatewayNoDefaultBind: p.EnvoyGatewayNoDefaultBind, + EnvoyDNSDiscoveryType: p.EnvoyDNSDiscoveryType, Config: config, } } @@ -549,9 +553,74 @@ func (e *ConsulIngressConfigEntry) Copy() *ConsulIngressConfigEntry { } } -// ConsulTerminatingConfigEntry is not yet supported. -// type ConsulTerminatingConfigEntry struct { -// } +type ConsulLinkedService struct { + Name string `hcl:"name,optional"` + CAFile string `hcl:"ca_file,optional"` + CertFile string `hcl:"cert_file,optional"` + KeyFile string `hcl:"key_file,optional"` + SNI string `hcl:"sni,optional"` +} + +func (s *ConsulLinkedService) Canonicalize() { + // nothing to do for now +} + +func (s *ConsulLinkedService) Copy() *ConsulLinkedService { + if s == nil { + return nil + } + + return &ConsulLinkedService{ + Name: s.Name, + CAFile: s.CAFile, + CertFile: s.CertFile, + KeyFile: s.KeyFile, + SNI: s.SNI, + } +} + +// ConsulTerminatingConfigEntry represents the Consul Configuration Entry type +// for a Terminating Gateway. +// +// https://www.consul.io/docs/agent/config-entries/terminating-gateway#available-fields +type ConsulTerminatingConfigEntry struct { + // Namespace is not yet supported. + // Namespace string + + Services []*ConsulLinkedService `hcl:"service,block"` +} + +func (e *ConsulTerminatingConfigEntry) Canonicalize() { + if e == nil { + return + } + + if len(e.Services) == 0 { + e.Services = nil + } + + for _, service := range e.Services { + service.Canonicalize() + } +} + +func (e *ConsulTerminatingConfigEntry) Copy() *ConsulTerminatingConfigEntry { + if e == nil { + return nil + } + + var services []*ConsulLinkedService = nil + if n := len(e.Services); n > 0 { + services = make([]*ConsulLinkedService, n) + for i := 0; i < n; i++ { + services[i] = e.Services[i].Copy() + } + } + + return &ConsulTerminatingConfigEntry{ + Services: services, + } +} // ConsulMeshConfigEntry is not yet supported. // type ConsulMeshConfigEntry struct { diff --git a/website/content/docs/job-specification/gateway.mdx b/website/content/docs/job-specification/gateway.mdx index 96a70aaefb2..941b14ad835 100644 --- a/website/content/docs/job-specification/gateway.mdx +++ b/website/content/docs/job-specification/gateway.mdx @@ -25,100 +25,10 @@ same network. For public ingress products like [NGINX](https://learn.hashicorp.c provide more suitable features. ```hcl -job "ingress-demo" { - - datacenters = ["dc1"] - - # This group will have a task providing the ingress gateway automatically - # created by Nomad. The ingress gateway is based on the Envoy proxy being - # managed by the docker driver. - group "ingress-group" { - - network { - mode = "bridge" - - # This example will enable plain HTTP traffic to access the uuid-api connect - # native example service on port 8080. - port "inbound" { - static = 8080 - to = 8080 - } - } - - service { - name = "my-ingress-service" - port = "8080" - - connect { - gateway { - - # Consul gateway [envoy] proxy options. - proxy { - # The following options are automatically set by Nomad if not - # explicitly configured when using bridge networking. - # - # envoy_gateway_no_default_bind = true - # envoy_gateway_bind_addresses "uuid-api" { - # address = "0.0.0.0" - # port = - # } - # - # Additional options are documented at - # https://www.nomadproject.io/docs/job-specification/gateway#proxy-parameters - } - - # Consul Ingress Gateway Configuration Entry. - ingress { - # Nomad will automatically manage the Configuration Entry in Consul - # given the parameters in the ingress block. - # - # Additional options are documented at - # https://www.nomadproject.io/docs/job-specification/gateway#ingress-parameters - listener { - port = 8080 - protocol = "tcp" - service { - name = "uuid-api" - } - } - } - } - } - } - } - - # The UUID generator from the connect-native demo is used as an example service. - # The ingress gateway above makes access to the service possible over normal HTTP. - # For example, - # - # $ curl $(dig +short @127.0.0.1 -p 8600 uuid-api.ingress.dc1.consul. ANY):8080 - group "generator" { - network { - mode = "host" - port "api" {} - } - - service { - name = "uuid-api" - port = "${NOMAD_PORT_api}" - - connect { - native = true - } - } - - task "generate" { - driver = "docker" - - config { - image = "hashicorpnomad/uuid-api:v3" - network_mode = "host" - } - - env { - BIND = "0.0.0.0" - PORT = "${NOMAD_PORT_api}" - } +service { + connect { + gateway { + # ... } } } @@ -126,10 +36,14 @@ job "ingress-demo" { ## `gateway` Parameters +Exactly one of `ingress` or `terminating` must be configured. + - `proxy` ([proxy]: nil) - Configuration of the Envoy proxy that will be injected into the task group. - `ingress` ([ingress]: nil) - Configuration Entry of type `ingress-gateway` that will be associated with the service. +- `terminating` ([terminating]: nil) - Configuration Entry of type `terminating-gateway` + that will be associated with the service. ### `proxy` Parameters @@ -159,6 +73,11 @@ envoy_gateway_bind_addresses "" { to configure the gateway's bind addresses. If `bridge` networking is in use, this value will default to `true` since the Envoy proxy does not need to bind to the service address from inside the network namespace. +- `envoy_dns_discovery_type` `(string: optional)` - Determintes how Envoy will + resolve hostnames. Defaults to `LOGICAL_DNS`. Must be one of `STRICT_DNS` or + `LOGICAL_DNS`. Details for each type are available in the [Envoy Documentation](https://www.envoyproxy.io/docs/envoy/v1.16.1/intro/arch_overview/upstream/service_discovery). + This option applies to terminating gateways that route to services addressed by a + hostname. - `config` `(map: nil)` - Escape hatch for [Advanced Configuration] of Envoy. #### `address` Parameters @@ -219,6 +138,35 @@ envoy_gateway_bind_addresses "" { field as well. TLS verification only matches against the hostname of the incoming connection, and does not take into account the port. +### `terminating` Parameters + +- `service` (array<[linked-service]>: required) - One or more services to be + linked with the gateway. The gateway will proxy traffic to these services. These + linked services must be registered with Consul for the gateway to discover their + addresses. They must also be registered in the same Consul datacenter as the + terminating gateway. + +#### `service` Parameters + +- `name` `(string: required)` - The name of the service to link with the gateway. + If the wildcard specifier `*` is provided, then ALL services within the Consul + namespace wil lbe linked with the gateway. + +- `ca_file` `(string: )` - A file path to a PEM-encoded certificate + authority. The file must be accessible by the gateway task. The certificate authority + is used to verify the authenticity of the service linked with the gateway. It + can be provided along with a `cert_file` and `key_file` for mutual TLS + authentication, or on its own for one-way TLS authentication. If none is provided + the gateway **will not** encrypt traffic to the destination. +- `cert_file` `(string: )` - A file path to a PEM-encoded certificate. + The file must be accessible by the gateway task. The certificate is provided to servers + to verify the gateway's authenticity. It must be provided if a `key_file` is provided. +- `key_file` `(string: )` - A file path to a PEM-encoded private key. + The file must be accessible by the gateway task. The key is used with the certificate + to verify the gateway's authenticity. It must be provided if a `cert_file` is provided. +- `sni` `(string: )` - An optional hostname or domain name to specify during + the TLS handshake. + ### Gateway with host networking Nomad supports running gateways using host networking. A static port must be allocated @@ -259,14 +207,244 @@ connect { } ``` -[proxy]: /docs/job-specification/gateway#proxy-parameters +### Examples + +#### ingress gateway + +```hcl +job "ingress-demo" { + + datacenters = ["dc1"] + + # This group will have a task providing the ingress gateway automatically + # created by Nomad. The ingress gateway is based on the Envoy proxy being + # managed by the docker driver. + group "ingress-group" { + + network { + mode = "bridge" + + # This example will enable plain HTTP traffic to access the uuid-api connect + # native example service on port 8080. + port "inbound" { + static = 8080 + to = 8080 + } + } + + service { + name = "my-ingress-service" + port = "8080" + + connect { + gateway { + + # Consul gateway [envoy] proxy options. + proxy { + # The following options are automatically set by Nomad if not + # explicitly configured when using bridge networking. + # + # envoy_gateway_no_default_bind = true + # envoy_gateway_bind_addresses "uuid-api" { + # address = "0.0.0.0" + # port = + # } + # + # Additional options are documented at + # https://www.nomadproject.io/docs/job-specification/gateway#proxy-parameters + } + + # Consul Ingress Gateway Configuration Entry. + ingress { + # Nomad will automatically manage the Configuration Entry in Consul + # given the parameters in the ingress block. + # + # Additional options are documented at + # https://www.nomadproject.io/docs/job-specification/gateway#ingress-parameters + listener { + port = 8080 + protocol = "tcp" + service { + name = "uuid-api" + } + } + } + } + } + } + } + + # The UUID generator from the connect-native demo is used as an example service. + # The ingress gateway above makes access to the service possible over normal HTTP. + # For example, + # + # $ curl $(dig +short @127.0.0.1 -p 8600 uuid-api.ingress.dc1.consul. ANY):8080 + group "generator" { + network { + mode = "host" + port "api" {} + } + + service { + name = "uuid-api" + port = "${NOMAD_PORT_api}" + + connect { + native = true + } + } + + task "generate" { + driver = "docker" + + config { + image = "hashicorpnomad/uuid-api:v3" + network_mode = "host" + } + + env { + BIND = "0.0.0.0" + PORT = "${NOMAD_PORT_api}" + } + } + } +} +``` + +#### terminating gateway + +```hcl +job "countdash-terminating" { + + datacenters = ["dc1"] + + # This group provides the service that exists outside of the Consul Connect + # service mesh. It is using host networking and listening to a statically + # allocated port. + group "api" { + network { + mode = "host" + port "port" { + static = "9001" + } + } + + # This example will enable services in the service mesh to make requests + # to this service which is not in the service mesh by making requests + # through the terminating gateway. + service { + name = "count-api" + port = "port" + } + + task "api" { + driver = "docker" + + config { + image = "hashicorpnomad/counter-api:v3" + network_mode = "host" + } + } + } + + group "gateway" { + network { + mode = "bridge" + } + + service { + name = "api-gateway" + + connect { + gateway { + # Consul gateway [envoy] proxy options. + proxy { + # The following options are automatically set by Nomad if not explicitly + # configured with using bridge networking. + # + # envoy_gateway_no_default_bind = true + # envoy_gateway_bind_addresses "default" { + # address = "0.0.0.0" + # port = + # } + # Additional options are documented at + # https://www.nomadproject.io/docs/job-specification/gateway#proxy-parameters + } + + # Consul Terminating Gateway Configuration Entry. + terminating { + # Nomad will automatically manage the Configuration Entry in Consul + # given the parameters in the terminating block. + # + # Additional options are documented at + # https://www.nomadproject.io/docs/job-specification/gateway#terminating-parameters + service { + name = "count-api" + } + } + } + } + } + } + + # The dashboard service is in the service mesh, making use of bridge network + # mode and connect.sidecar_service. When running, the dashboard should be + # available from a web browser at localhost:9002. + group "dashboard" { + network { + mode = "bridge" + + port "http" { + static = 9002 + to = 9002 + } + } + + service { + name = "count-dashboard" + port = "9002" + + connect { + sidecar_service { + proxy { + upstreams { + # By configuring an upstream destination to the linked service of + # the terminating gateway, the dashboard is able to make requests + # through the gateway to the count-api service. + destination_name = "count-api" + local_bind_port = 8080 + } + } + } + } + } + + task "dashboard" { + driver = "docker" + + env { + COUNTING_SERVICE_URL = "http://${NOMAD_UPSTREAM_ADDR_count_api}" + } + + config { + image = "hashicorpnomad/counter-dashboard:v3" + } + } + } +} +``` + +[address]: /docs/job-specification/gateway#address-parameters +[advanced configuration]: https://www.consul.io/docs/connect/proxies/envoy#advanced-configuration +[connect_timeout_ms]: https://www.consul.io/docs/agent/config-entries/service-resolver#connecttimeout +[envoy docker]: https://hub.docker.com/r/envoyproxy/envoy/tags [ingress]: /docs/job-specification/gateway#ingress-parameters -[tls]: /docs/job-specification/gateway#tls-parameters +[proxy]: /docs/job-specification/gateway#proxy-parameters +[linked-service]: /docs/job-specification/gateway#service-parameters-1 [listener]: /docs/job-specification/gateway#listener-parameters [service]: /docs/job-specification/gateway#service-parameters [service-default]: https://www.consul.io/docs/agent/config-entries/service-defaults [sidecar_task]: /docs/job-specification/sidecar_task -[connect_timeout_ms]: https://www.consul.io/docs/agent/config-entries/service-resolver#connecttimeout -[address]: /docs/job-specification/gateway#address-parameters -[advanced configuration]: https://www.consul.io/docs/connect/proxies/envoy#advanced-configuration -[envoy docker]: https://hub.docker.com/r/envoyproxy/envoy/tags +[terminating]: /docs/job-specification/gateway#terminating-parameters +[tls]: /docs/job-specification/gateway#tls-parameters + From 37544ca86a9efcebc4562734aabf907bd2a55807 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Fri, 22 Jan 2021 10:09:05 -0600 Subject: [PATCH 16/36] consul/connect: remove debug line --- nomad/job_endpoint.go | 1 - 1 file changed, 1 deletion(-) diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 5d35675d6d7..07e7ea0aac7 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -296,7 +296,6 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis } } for service, entry := range entries.Terminating { - fmt.Println("SH JE set terminating CE", service) if err := j.srv.consulConfigEntries.SetTerminatingCE(ctx, service, entry); err != nil { return err } From 74780f08bf854dc7b08b62816dfcdad1246b73d5 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 25 Jan 2021 10:33:50 -0600 Subject: [PATCH 17/36] consul/connect: copy bind address map if empty This parameter is now supposed to be non-nil even if empty, and the Copy method should also maintain that invariant. --- nomad/structs/services.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 073577b3c1d..63107654ae1 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -1403,7 +1403,7 @@ func (p *ConsulGatewayProxy) Copy() *ConsulGatewayProxy { } func (p *ConsulGatewayProxy) copyBindAddresses() map[string]*ConsulGatewayBindAddress { - if len(p.EnvoyGatewayBindAddresses) == 0 { + if p.EnvoyGatewayBindAddresses == nil { return nil } From c45c8e8bb681a40aab6d2ac47645ec1419d701f8 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Mon, 25 Jan 2021 11:39:50 -0500 Subject: [PATCH 18/36] update readme about profiles and packer build --- e2e/terraform/README.md | 10 +++++----- e2e/terraform/packer/README.md | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/e2e/terraform/README.md b/e2e/terraform/README.md index 8b3675d7651..9cdd95a6c2e 100644 --- a/e2e/terraform/README.md +++ b/e2e/terraform/README.md @@ -73,7 +73,7 @@ If you want to bootstrap Nomad ACLs, include `-var 'nomad_acls=true'`. The `profile` field selects from a set of configuration files for Nomad, Consul, and Vault by uploading the files found in `./config/`. The -profiles are as follows: +standard profiles are as follows: * `full-cluster`: This profile is used for nightly E2E testing. It assumes at least 3 servers and includes a unique config for each Nomad client. @@ -81,10 +81,10 @@ profiles are as follows: set of clients. It assumes at least 3 servers but uses the one config for all the Linux Nomad clients and one config for all the Windows Nomad clients. -* `custom`: This profile is used for one-off developer testing of more complex - interactions between features. You can build your own custom profile by - writing config files to the `./config/custom` directory, which are protected - by `.gitignore` + +You may create additional profiles for testing more complex interactions between features. +You can build your own custom profile by writing config files to the +`./config/` directory. For each profile, application (Nomad, Consul, Vault), and agent type (`server`, `client_linux`, or `client_windows`), the agent gets the following diff --git a/e2e/terraform/packer/README.md b/e2e/terraform/packer/README.md index f5d25c3c00f..a6b843107d3 100644 --- a/e2e/terraform/packer/README.md +++ b/e2e/terraform/packer/README.md @@ -17,10 +17,10 @@ $ packer --version 1.6.4 # build Ubuntu Bionic AMI -$ packer build ubuntu-bionic-amd64.pkr.hcl +$ ./build ubuntu-bionic-amd64 # build Windows AMI -$ packer build windows-2016-amd64.pkr.hcl +$ ./build windows-2016-amd64 ``` ## Debugging Packer Builds From 0d795bdac17d5c56373c62b6723dbdc2edefee10 Mon Sep 17 00:00:00 2001 From: James Rasell Date: Tue, 26 Jan 2021 12:29:58 +0100 Subject: [PATCH 19/36] docs: clarify where variables can be placed with HCLv2. --- website/content/docs/job-specification/hcl2/variables.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/website/content/docs/job-specification/hcl2/variables.mdx b/website/content/docs/job-specification/hcl2/variables.mdx index 23d3c33303c..c2d180bf33d 100644 --- a/website/content/docs/job-specification/hcl2/variables.mdx +++ b/website/content/docs/job-specification/hcl2/variables.mdx @@ -12,8 +12,8 @@ description: |- Input variables serve as parameters for a Nomad job, allowing aspects of the job to be customized without altering the job's own source code. -When you declare variables in the job configuration, you can set -their values using CLI options and environment variables. +When you declare variables in the same file as the job specification, you +can set their values using CLI options and environment variables. -> **Note:** For brevity, input variables are often referred to as just "variables" or "Nomad variables" when it is clear from context what sort of From fcb7e160dab6a7ef18576158da337e1a8dea7b64 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:11:37 -0500 Subject: [PATCH 20/36] e2e: Fix build script and pass shellcheck --- e2e/terraform/packer/build | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/e2e/terraform/packer/build b/e2e/terraform/packer/build index 4b9bc947403..9782d549095 100755 --- a/e2e/terraform/packer/build +++ b/e2e/terraform/packer/build @@ -1,5 +1,6 @@ -#!/bin/sh +#!/usr/bin/bash +set -u set -e usage() { @@ -20,16 +21,16 @@ if [[ $# -ne 1 ]]; then usage fi -target=${1/%.pkr.hcl/} +target="${1/%.pkr.hcl/}" -directory=$(dirname "$0") -cd ${directory} +directory="$(dirname "$0")" +cd "${directory}" if ! test -f "${target}.pkr.hcl"; then echo "${target}.pkr.hcl is not present" >&2 exit 1 fi -sha=$(git log -n 1 --pretty=format:%H ${dirname}) -echo packer build --var "build_sha=${sha}" ${target}.pkr.hcl -packer build --var "build_sha=${sha}" ${target}.pkr.hcl +sha=$(git log -n 1 --pretty=format:%H "${directory}") +echo packer build --var "build_sha=${sha}" "${target}.pkr.hcl" +packer build --var "build_sha=${sha}" "${target}.pkr.hcl" From 30573f048ed8e3c98aa1f9c9ff9084ca72264063 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:12:20 -0500 Subject: [PATCH 21/36] e2e: deflake consul/CheckRestart test Ensure we pass the alloc ID to status. Otherwise, the test may fail if there is another spurious allocation running from another test. --- e2e/consul/check_restart.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/e2e/consul/check_restart.go b/e2e/consul/check_restart.go index 63afe5d2022..c7756fc1308 100644 --- a/e2e/consul/check_restart.go +++ b/e2e/consul/check_restart.go @@ -47,8 +47,6 @@ func (tc *CheckRestartE2ETest) TestGroupCheckRestart(f *framework.F) { f.NoError(e2e.Register(jobID, "consul/input/checks_group_restart.nomad")) tc.jobIds = append(tc.jobIds, jobID) - var allocID string - f.NoError( e2e.WaitForAllocStatusComparison( func() ([]string, error) { return e2e.AllocStatuses(jobID, ns) }, @@ -56,6 +54,11 @@ func (tc *CheckRestartE2ETest) TestGroupCheckRestart(f *framework.F) { &e2e.WaitConfig{Interval: time.Second * 10, Retries: 30}, )) + allocs, err := e2e.AllocsForJob(jobID, ns) + f.NoError(err) + f.Len(allocs, 1) + + allocID := allocs[0]["ID"] expected := "Exceeded allowed attempts 2 in interval 5m0s and mode is \"fail\"" out, err := e2e.Command("nomad", "alloc", "status", allocID) From f7acda4260f500b3e0785ba7742dec09513d3cd5 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:14:47 -0500 Subject: [PATCH 22/36] e2e: use testify requires instead of t.Fatal testify requires offer better error message that is easier to notice when seeing a wall of text in the builds. --- e2e/consul/consul.go | 10 +++++----- e2e/consultemplate/consultemplate.go | 2 +- e2e/e2eutil/e2ejob.go | 2 +- e2e/framework/framework.go | 8 ++++---- e2e/lifecycle/lifecycle.go | 2 +- e2e/taskevents/taskevents.go | 2 +- e2e/vaultcompat/vault_test.go | 16 ++++++++-------- 7 files changed, 21 insertions(+), 21 deletions(-) diff --git a/e2e/consul/consul.go b/e2e/consul/consul.go index 20212480ffa..dc61b730607 100644 --- a/e2e/consul/consul.go +++ b/e2e/consul/consul.go @@ -176,7 +176,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { return true, nil }, func(err error) { - t.Fatalf("error while waiting for deploys: %v", err) + require.NoError(t, err, "error while waiting for deploys") }) allocID := activeDeploy.TaskGroups["consul_canary_test"].PlacedCanaries[0] @@ -196,7 +196,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { return *alloc.DeploymentStatus.Healthy, fmt.Errorf("expected healthy canary but found: %#v", alloc.DeploymentStatus) }, func(err error) { - t.Fatalf("error waiting for canary to be healthy: %v", err) + require.NoError(t, err, "error waiting for canary to be healthy") }) // Check Consul for canary tags @@ -212,7 +212,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { } return false, fmt.Errorf(`could not find service tags {"canary", "foo"}: %#v`, consulServices) }, func(err error) { - t.Fatalf("error waiting for canary tags: %v", err) + require.NoError(t, err, "error waiting for canary tags") }) // Promote canary @@ -230,7 +230,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { } return !alloc.DeploymentStatus.Canary, fmt.Errorf("still a canary") }, func(err error) { - t.Fatalf("error waiting for canary to be promoted: %v", err) + require.NoError(t, err, "error waiting for canary to be promoted") }) // Verify that no instances have canary tags @@ -248,7 +248,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { } return true, nil }, func(err error) { - t.Fatalf("error waiting for non-canary tags: %v", err) + require.NoError(t, err, "error waiting for non-canary tags") }) } diff --git a/e2e/consultemplate/consultemplate.go b/e2e/consultemplate/consultemplate.go index 174fe9d6dfb..bb60ab59f5b 100644 --- a/e2e/consultemplate/consultemplate.go +++ b/e2e/consultemplate/consultemplate.go @@ -277,7 +277,7 @@ func (tc *ConsulTemplateTest) TestTemplatePathInterpolation_Bad(f *framework.F) return alloc.ClientStatus == structs.AllocClientStatusFailed, fmt.Errorf("expected status failed, but was: %s", alloc.ClientStatus) }, func(err error) { - f.T().Fatalf("failed to wait on alloc: %v", err) + f.NoError(err, "failed to wait on alloc") }) // Assert the "source escapes" error occurred to prevent false diff --git a/e2e/e2eutil/e2ejob.go b/e2e/e2eutil/e2ejob.go index fe48add16b4..2ffff8b97de 100644 --- a/e2e/e2eutil/e2ejob.go +++ b/e2e/e2eutil/e2ejob.go @@ -116,7 +116,7 @@ func (j *e2eServiceJob) Run(f *framework.F) { return alloc.ClientStatus == structs.AllocClientStatusRunning, fmt.Errorf("expected status running, but was: %s", alloc.ClientStatus) }, func(err error) { - t.Fatalf("failed to keep alloc running: %v", err) + require.NoError(t, err, "failed to keep alloc running") }) scriptPath := filepath.Join(filepath.Dir(j.jobfile), j.script) diff --git a/e2e/framework/framework.go b/e2e/framework/framework.go index 9667dc34044..aff77ac3f4d 100644 --- a/e2e/framework/framework.go +++ b/e2e/framework/framework.go @@ -7,6 +7,8 @@ import ( "reflect" "strings" "testing" + + "github.com/stretchr/testify/require" ) const frameworkHelp = ` @@ -119,7 +121,7 @@ func AddSuites(s ...*TestSuite) { func (f *Framework) Run(t *testing.T) { info, err := f.provisioner.SetupTestRun(t, SetupOptions{}) if err != nil { - t.Fatalf("could not provision cluster: %v", err) + require.NoError(t, err, "could not provision cluster") } defer f.provisioner.TearDownTestRun(t, info.ID) @@ -178,9 +180,7 @@ func (f *Framework) runSuite(t *testing.T, s *TestSuite) (skip bool, err error) ExpectConsul: s.Consul, ExpectVault: s.Vault, }) - if err != nil { - t.Fatalf("could not provision cluster: %v", err) - } + require.NoError(t, err, "could not provision cluster") defer f.provisioner.TearDownTestSuite(t, info.ID) for _, c := range s.Cases { diff --git a/e2e/lifecycle/lifecycle.go b/e2e/lifecycle/lifecycle.go index a0150f2fb74..ab13c2eae6f 100644 --- a/e2e/lifecycle/lifecycle.go +++ b/e2e/lifecycle/lifecycle.go @@ -102,7 +102,7 @@ func (tc *LifecycleE2ETest) TestServiceJob(f *framework.F) { return true, nil }, func(err error) { - t.Fatalf("failed to wait on alloc: %v", err) + require.NoError(err, "failed to wait on alloc") }) alloc, _, err := nomadClient.Allocations().Info(allocID, nil) diff --git a/e2e/taskevents/taskevents.go b/e2e/taskevents/taskevents.go index 47f5b114eb9..26751c05294 100644 --- a/e2e/taskevents/taskevents.go +++ b/e2e/taskevents/taskevents.go @@ -107,7 +107,7 @@ func (tc *TaskEventsTest) waitUntilEvents(f *framework.F, jobName string, numEve return true, nil }, func(err error) { - t.Fatalf("task events error: %v", err) + require.NoError(t, err, "task events error") }) return alloc, taskState diff --git a/e2e/vaultcompat/vault_test.go b/e2e/vaultcompat/vault_test.go index 0e1ff4a2660..3c166ae0c56 100644 --- a/e2e/vaultcompat/vault_test.go +++ b/e2e/vaultcompat/vault_test.go @@ -62,7 +62,7 @@ func syncVault(t *testing.T) ([]*version.Version, map[string]string) { case err := <-errCh: require.NoError(t, err) case <-time.After(5 * time.Minute): - t.Fatalf("timed out downloading Vault binaries") + require.Fail(t, "timed out downloading Vault binaries") } } if n := len(missing); n > 0 { @@ -361,7 +361,7 @@ func testVaultCompatibility(t *testing.T, vault string, version string) { case 1: default: // exit early - t.Fatalf("too many allocations; something failed") + require.Fail("too many allocations; something failed") } alloc := allocs[0] //allocID = alloc.ID @@ -371,7 +371,7 @@ func testVaultCompatibility(t *testing.T, vault string, version string) { return false, fmt.Errorf("client status %q", alloc.ClientStatus) }, func(err error) { - t.Fatalf("allocation did not finish: %v", err) + require.NoError(err, "allocation did not finish") }) } @@ -392,14 +392,14 @@ func setupVault(t *testing.T, client *vapi.Client, vaultVersion string) string { } request := client.NewRequest("PUT", "/v1/sys/policy/nomad-server") if err := request.SetJSONBody(body); err != nil { - t.Fatalf("failed to set JSON body on legacy policy creation: %v", err) + require.NoError(t, err, "failed to set JSON body on legacy policy creation") } if _, err := client.RawRequest(request); err != nil { - t.Fatalf("failed to create legacy policy: %v", err) + require.NoError(t, err, "failed to create legacy policy") } } else { if err := sys.PutPolicy("nomad-server", policy); err != nil { - t.Fatalf("failed to create policy: %v", err) + require.NoError(t, err, "failed to create policy") } } @@ -416,12 +416,12 @@ func setupVault(t *testing.T, client *vapi.Client, vaultVersion string) string { } s, err := a.Create(&req) if err != nil { - t.Fatalf("failed to create child token: %v", err) + require.NoError(t, err, "failed to create child token") } // Get the client token if s == nil || s.Auth == nil { - t.Fatalf("bad secret response: %+v", s) + require.NoError(t, err, "bad secret response") } return s.Auth.ClientToken From 25f10e13e5575756674d624afea88499f7c8b05f Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:16:48 -0500 Subject: [PATCH 23/36] e2e: tweak failure messages Tweak the error messages for the flakiest tests, so that on test failure, we get more output --- e2e/connect/multi_service.go | 20 +++++++++++++------- e2e/consulacls/manage.go | 2 +- e2e/e2eutil/allocs.go | 10 ++++++++-- e2e/volumes/volumes.go | 2 +- 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/e2e/connect/multi_service.go b/e2e/connect/multi_service.go index 3a93ebedba2..139bfc9ce39 100644 --- a/e2e/connect/multi_service.go +++ b/e2e/connect/multi_service.go @@ -1,6 +1,7 @@ package connect import ( + "fmt" "strings" "time" @@ -9,6 +10,7 @@ import ( "github.com/hashicorp/nomad/e2e/framework" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/jobspec" + "github.com/hashicorp/nomad/testutil" "github.com/kr/pretty" "github.com/stretchr/testify/require" ) @@ -45,9 +47,9 @@ EVAL: case "complete": // Ok! case "failed", "canceled", "blocked": - t.Fatalf("eval %s\n%s\n", eval.Status, pretty.Sprint(eval)) + require.Failf(t, "expected complete status", "eval %s\n%s", eval.Status, pretty.Sprint(eval)) default: - t.Fatalf("unknown eval status: %s\n%s\n", eval.Status, pretty.Sprint(eval)) + require.Failf(t, "expected complete status", "unknown eval status: %s\n%s", eval.Status, pretty.Sprint(eval)) } // Assert there were 0 placement failures @@ -85,7 +87,7 @@ EVAL: allocIDs := make(map[string]bool, 1) for _, a := range allocs { if a.ClientStatus != "running" || a.DesiredStatus != "run" { - t.Fatalf("alloc %s (%s) terminal; client=%s desired=%s", a.TaskGroup, a.ID, a.ClientStatus, a.DesiredStatus) + require.Failf(t, "expected running status", "alloc %s (%s) terminal; client=%s desired=%s", a.TaskGroup, a.ID, a.ClientStatus, a.DesiredStatus) } allocIDs[a.ID] = true } @@ -94,7 +96,9 @@ EVAL: agentapi := tc.Consul().Agent() failing := map[string]*consulapi.AgentCheck{} - require.Eventually(t, func() bool { + testutil.WaitForResultRetries(60, func() (bool, error) { + defer time.Sleep(time.Second) + checks, err := agentapi.Checks() require.NoError(t, err) @@ -123,12 +127,14 @@ EVAL: } if len(failing) == 0 { - return true + return true, nil } t.Logf("still %d checks not passing", len(failing)) - return false - }, time.Minute, time.Second) + return false, fmt.Errorf("checks are not passing %v %v", len(failing), pretty.Sprint(failing)) + }, func(e error) { + require.NoError(t, err) + }) require.Len(t, failing, 0, pretty.Sprint(failing)) } diff --git a/e2e/consulacls/manage.go b/e2e/consulacls/manage.go index 785e803e942..bd6add3b01c 100644 --- a/e2e/consulacls/manage.go +++ b/e2e/consulacls/manage.go @@ -62,7 +62,7 @@ func (m *tfManager) Enable(t *testing.T) string { response, err := exec.CommandContext(ctx, "consulacls/consul-acls-manage.sh", "enable").CombinedOutput() - require.NoError(t, err) + require.NoError(t, err, "consul-acls-manage.sh failed: %v", string(response)) fmt.Println(string(response)) // Read the Consul ACL master token that was generated (or if the token diff --git a/e2e/e2eutil/allocs.go b/e2e/e2eutil/allocs.go index fcf6f605774..fe1a78f385c 100644 --- a/e2e/e2eutil/allocs.go +++ b/e2e/e2eutil/allocs.go @@ -9,17 +9,23 @@ import ( "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/testutil" + "github.com/kr/pretty" ) // WaitForAllocStatusExpected polls 'nomad job status' and exactly compares // the status of all allocations (including any previous versions) against the // expected list. func WaitForAllocStatusExpected(jobID, ns string, expected []string) error { - return WaitForAllocStatusComparison( + err := WaitForAllocStatusComparison( func() ([]string, error) { return AllocStatuses(jobID, ns) }, func(got []string) bool { return reflect.DeepEqual(got, expected) }, nil, ) + if err != nil { + allocs, _ := AllocsForJob(jobID, ns) + err = fmt.Errorf("%v\nallocs: %v", err, pretty.Sprint(allocs)) + } + return err } // WaitForAllocStatusComparison is a convenience wrapper that polls the query @@ -208,7 +214,7 @@ func AllocExec(allocID, taskID, execCmd, ns string, wc *WaitConfig) (string, err got, err = Command(cmd[0], cmd[1:]...) return err == nil, err }, func(e error) { - err = fmt.Errorf("exec failed: '%s': %v", strings.Join(cmd, " "), e) + err = fmt.Errorf("exec failed: '%s': %v\nGot: %v", strings.Join(cmd, " "), e, got) }) return got, err } diff --git a/e2e/volumes/volumes.go b/e2e/volumes/volumes.go index 0a38aa5480c..936c2099b27 100644 --- a/e2e/volumes/volumes.go +++ b/e2e/volumes/volumes.go @@ -71,7 +71,7 @@ func (tc *VolumesTest) TestVolumeMounts(f *framework.F) { out, err := e2e.AllocExec(allocID, "docker_task", cmdToExec, ns, nil) f.NoError(err, "could not exec into task: docker_task") - f.Equal(out, allocID+"\n", "alloc data is missing from docker_task") + f.Equal(allocID+"\n", out, "alloc data is missing from docker_task") out, err = e2e.AllocExec(allocID, "exec_task", cmdToExec, ns, nil) f.NoError(err, "could not exec into task: exec_task") From b49df6e9ae2ab0a7c16f4d8cf438cb19b69318c5 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:18:59 -0500 Subject: [PATCH 24/36] e2e: special case "Unexpected EOF" errors This is an attempt at deflaking the e2e exec tests, and a way to improve messages. e2e occasionally fail with "unexpected EOF" even though the exec output matches expectations. I suspect there is a race in handling EOF in server/http handling. Here, we special case this error and ensures we get all failures, to help debug the case better. --- e2e/nomadexec/exec.go | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/e2e/nomadexec/exec.go b/e2e/nomadexec/exec.go index 341ee83c754..73a113b88de 100644 --- a/e2e/nomadexec/exec.go +++ b/e2e/nomadexec/exec.go @@ -7,6 +7,7 @@ import ( "io" "reflect" "regexp" + "strings" "testing" "time" @@ -16,7 +17,6 @@ import ( "github.com/hashicorp/nomad/helper/uuid" dtestutils "github.com/hashicorp/nomad/plugins/drivers/testutils" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) type NomadExecE2ETest struct { @@ -90,30 +90,36 @@ func (tc *NomadExecE2ETest) TestExecBasicResponses(f *framework.F) { stdin, &stdout, &stderr, resizeCh, nil) - require.NoError(t, err) + // TODO: Occasionally, we get "Unexpected EOF" error, but with the correct output. + // investigate why + if err != nil && strings.Contains(err.Error(), io.ErrUnexpectedEOF.Error()) { + f.T().Logf("got unexpected EOF error, ignoring: %v", err) + } else { + assert.NoError(t, err) + } assert.Equal(t, c.ExitCode, exitCode) switch s := c.Stdout.(type) { case string: - require.Equal(t, s, stdout.String()) + assert.Equal(t, s, stdout.String()) case *regexp.Regexp: - require.Regexp(t, s, stdout.String()) + assert.Regexp(t, s, stdout.String()) case nil: - require.Empty(t, stdout.String()) + assert.Empty(t, stdout.String()) default: - require.Fail(t, "unexpected stdout type", "found %v (%v), but expected string or regexp", s, reflect.TypeOf(s)) + assert.Fail(t, "unexpected stdout type", "found %v (%v), but expected string or regexp", s, reflect.TypeOf(s)) } switch s := c.Stderr.(type) { case string: - require.Equal(t, s, stderr.String()) + assert.Equal(t, s, stderr.String()) case *regexp.Regexp: - require.Regexp(t, s, stderr.String()) + assert.Regexp(t, s, stderr.String()) case nil: - require.Empty(t, stderr.String()) + assert.Empty(t, stderr.String()) default: - require.Fail(t, "unexpected stderr type", "found %v (%v), but expected string or regexp", s, reflect.TypeOf(s)) + assert.Fail(t, "unexpected stderr type", "found %v (%v), but expected string or regexp", s, reflect.TypeOf(s)) } }) } From fe9929270c480e74fa5a5cfeb75a6c84eead0338 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:24:55 -0500 Subject: [PATCH 25/36] e2e: prefer testutil.WaitForResultRetries Prefer testutil.WaitForResultRetries that emits more descriptive errors on failures. `require.Evatually` fails with opaque "Condition never satisfied" error message. --- e2e/e2eutil/consul.go | 42 ++++++++++++++++++++++++++++++------------ e2e/e2eutil/utils.go | 38 ++++++++++++++++++++++---------------- e2e/metrics/metrics.go | 29 ++++++++++++++++------------- 3 files changed, 68 insertions(+), 41 deletions(-) diff --git a/e2e/e2eutil/consul.go b/e2e/e2eutil/consul.go index a4bdd087029..b39318be4b4 100644 --- a/e2e/e2eutil/consul.go +++ b/e2e/e2eutil/consul.go @@ -1,21 +1,25 @@ package e2eutil import ( + "fmt" "time" capi "github.com/hashicorp/consul/api" + "github.com/hashicorp/nomad/testutil" + "github.com/kr/pretty" "github.com/stretchr/testify/require" ) // RequireConsulStatus asserts the aggregate health of the service converges to the expected status. func RequireConsulStatus(require *require.Assertions, client *capi.Client, serviceName, expectedStatus string) { - require.Eventually(func() bool { + testutil.WaitForResultRetries(30, func() (bool, error) { + defer time.Sleep(time.Second) // needs a long time for killing tasks/clients + _, status := serviceStatus(require, client, serviceName) - return status == expectedStatus - }, 30*time.Second, time.Second, // needs a long time for killing tasks/clients - "timed out expecting %q to become %q", - serviceName, expectedStatus, - ) + return status == expectedStatus, fmt.Errorf("service %v: expected %v but found %v", serviceName, expectedStatus, status) + }, func(err error) { + require.NoError(err, "timedout waiting for consul status") + }) } // serviceStatus gets the aggregate health of the service and returns the []ServiceEntry for further checking. @@ -30,18 +34,32 @@ func serviceStatus(require *require.Assertions, client *capi.Client, serviceName // RequireConsulDeregistered asserts that the service eventually is de-registered from Consul. func RequireConsulDeregistered(require *require.Assertions, client *capi.Client, service string) { - require.Eventually(func() bool { + testutil.WaitForResultRetries(5, func() (bool, error) { + defer time.Sleep(time.Second) + services, _, err := client.Health().Service(service, "", false, nil) require.NoError(err) - return len(services) == 0 - }, 5*time.Second, time.Second) + if len(services) != 0 { + return false, fmt.Errorf("service %v: expected empty services but found %v %v", service, len(services), pretty.Sprint(services)) + } + return true, nil + }, func(err error) { + require.NoError(err) + }) } // RequireConsulRegistered assert that the service is registered in Consul. func RequireConsulRegistered(require *require.Assertions, client *capi.Client, service string, count int) { - require.Eventually(func() bool { + testutil.WaitForResultRetries(5, func() (bool, error) { + defer time.Sleep(time.Second) + services, _, err := client.Catalog().Service(service, "", nil) require.NoError(err) - return len(services) == count - }, 5*time.Second, time.Second) + if len(services) != count { + return false, fmt.Errorf("service %v: expected %v services but found %v %v", service, count, len(services), pretty.Sprint(services)) + } + return true, nil + }, func(err error) { + require.NoError(err) + }) } diff --git a/e2e/e2eutil/utils.go b/e2e/e2eutil/utils.go index 222413b5e57..adb0e455431 100644 --- a/e2e/e2eutil/utils.go +++ b/e2e/e2eutil/utils.go @@ -14,7 +14,6 @@ import ( "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" "github.com/kr/pretty" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -28,7 +27,7 @@ func WaitForLeader(t *testing.T, nomadClient *api.Client) { leader, err := statusAPI.Leader() return leader != "", err }, func(err error) { - t.Fatalf("failed to find leader: %v", err) + require.NoError(t, err, "failed to find leader") }) } @@ -53,7 +52,7 @@ func WaitForNodesReady(t *testing.T, nomadClient *api.Client, nodes int) { return eligibleNodes >= nodes, fmt.Errorf("only %d nodes ready (wanted at least %d)", eligibleNodes, nodes) }, func(err error) { - t.Fatalf("failed to get enough ready nodes: %v", err) + require.NoError(t, err, "failed to get enough ready nodes") }) } @@ -114,26 +113,33 @@ func RegisterAndWaitForAllocs(t *testing.T, nomadClient *api.Client, jobFile, jo evals := []*api.Evaluation{} // Wrap in retry to wait until placement - ok := assert.Eventually(t, func() bool { + testutil.WaitForResultRetries(retries, func() (bool, error) { + time.Sleep(time.Second) + allocs, _, err = jobs.Allocations(jobID, false, nil) - if len(allocs) < 1 { + if len(allocs) == 0 { evals, _, err = nomadClient.Jobs().Evaluations(jobID, nil) + return false, fmt.Errorf("no allocations for job %v", jobID) } - return len(allocs) > 0 - }, 30*time.Second, time.Second) - msg := fmt.Sprintf("allocations not placed for %s", jobID) - if !ok && len(evals) > 0 { + return true, nil + }, func(e error) { + msg := fmt.Sprintf("allocations not placed for %s", jobID) for _, eval := range evals { msg += fmt.Sprintf("\n %s - %s", eval.Status, eval.StatusDescription) } - } - require.Truef(t, ok, msg) + + require.Fail(t, msg, "full evals: %v", pretty.Sprint(evals)) + }) + require.NoError(t, err) // we only care about the last error + return allocs } func WaitForAllocRunning(t *testing.T, nomadClient *api.Client, allocID string) { + t.Helper() + testutil.WaitForResultRetries(retries, func() (bool, error) { time.Sleep(time.Millisecond * 100) alloc, _, err := nomadClient.Allocations().Info(allocID, nil) @@ -141,9 +147,9 @@ func WaitForAllocRunning(t *testing.T, nomadClient *api.Client, allocID string) return false, err } - return alloc.ClientStatus == structs.AllocClientStatusRunning, fmt.Errorf("expected status running, but was: %s", alloc.ClientStatus) + return alloc.ClientStatus == structs.AllocClientStatusRunning, fmt.Errorf("expected status running, but was: %s\n%v", alloc.ClientStatus, pretty.Sprint(alloc)) }, func(err error) { - t.Fatalf("failed to wait on alloc: %v", err) + require.NoError(t, err, "failed to wait on alloc") }) } @@ -169,7 +175,7 @@ func WaitForAllocNotPending(t *testing.T, nomadClient *api.Client, allocID strin return alloc.ClientStatus != structs.AllocClientStatusPending, fmt.Errorf("expected status not pending, but was: %s", alloc.ClientStatus) }, func(err error) { - t.Fatalf("failed to wait on alloc: %v", err) + require.NoError(t, err, "failed to wait on alloc") }) } @@ -204,7 +210,7 @@ func WaitForAllocStopped(t *testing.T, nomadClient *api.Client, allocID string) alloc.ClientStatus) } }, func(err error) { - t.Fatalf("failed to wait on alloc: %v", err) + require.NoError(t, err, "failed to wait on alloc") }) } @@ -249,7 +255,7 @@ func WaitForDeployment(t *testing.T, nomadClient *api.Client, deployID string, s ) }, func(err error) { - t.Fatalf("failed to wait on deployment: %v", err) + require.NoError(t, err, "failed to wait on deployment") }) } diff --git a/e2e/metrics/metrics.go b/e2e/metrics/metrics.go index 083120b283d..740e179cf81 100644 --- a/e2e/metrics/metrics.go +++ b/e2e/metrics/metrics.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/nomad/e2e/e2eutil" "github.com/hashicorp/nomad/e2e/framework" "github.com/hashicorp/nomad/helper/uuid" + "github.com/hashicorp/nomad/testutil" "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" @@ -118,9 +119,7 @@ func (tc *MetricsTest) runWorkloads(t *testing.T, workloads map[string]string) { tc.jobIDs = append(tc.jobIDs, jobID) file := "metrics/input/" + jobName + ".nomad" allocs := e2eutil.RegisterAndWaitForAllocs(t, tc.Nomad(), file, jobID, "") - if len(allocs) == 0 { - t.Fatalf("failed to register %s", jobID) - } + require.NotZerof(t, allocs, "failed to register %s", jobID) } } @@ -136,16 +135,21 @@ func (tc *MetricsTest) queryClientMetrics(t *testing.T, clientNodes []string) { } // we start with a very long timeout here because it takes a while for // prometheus to be live and for jobs to initially register metrics. - timeout := 60 * time.Second + retries := int64(60) for _, metric := range metrics { + var results model.Vector var err error - ok := assert.Eventually(t, func() bool { + + testutil.WaitForResultRetries(retries, func() (bool, error) { + defer time.Sleep(time.Second) + results, err = tc.promQuery(metric) if err != nil { - return false + return false, err } + instances := make(map[string]struct{}) for _, result := range results { instances[string(result.Metric["node_id"])] = struct{}{} @@ -155,19 +159,18 @@ func (tc *MetricsTest) queryClientMetrics(t *testing.T, clientNodes []string) { // and not just equal lengths for _, clientNode := range clientNodes { if _, ok := instances[clientNode]; !ok { - err = fmt.Errorf("expected metric '%s' for all clients. got:\n%v", - metric, results) - return false + return false, fmt.Errorf("expected metric '%s' for all clients. got:\n%v", metric, results) } } - return true - }, timeout, 1*time.Second) - require.Truef(t, ok, "prometheus query failed (%s): %v", metric, err) + return true, nil + }, func(err error) { + require.NoError(t, err) + }) // shorten the timeout after the first workload is successfully // queried so that we don't hang the whole test run if something's // wrong with only one of the jobs - timeout = 15 * time.Second + retries = 15 } } From 1290eb75f9948e9699e05474b6b4fefae2d447b4 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:27:35 -0500 Subject: [PATCH 26/36] e2e: vault increase timeout Increase the timeout for vaultsecrets. As the default interval is 0.1s, 10 retries mean it only retries for one second, a very short time for some waiting scenarios in the test (e.g. starting allocs, etc). --- e2e/vaultsecrets/vaultsecrets.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/vaultsecrets/vaultsecrets.go b/e2e/vaultsecrets/vaultsecrets.go index 5a03a365526..7f53a37922d 100644 --- a/e2e/vaultsecrets/vaultsecrets.go +++ b/e2e/vaultsecrets/vaultsecrets.go @@ -83,7 +83,7 @@ func (tc *VaultSecretsTest) TestVaultSecrets(f *framework.F) { pkiCertIssue := tc.pkiPath + "/issue/nomad" policyID := "access-secrets-" + testID index := 0 - wc := &e2e.WaitConfig{Retries: 10} + wc := &e2e.WaitConfig{Retries: 500} interval, retries := wc.OrDefault() setupCmds := []string{ From 78ccc93c2baac0515e8b32b8ef7185d53e86b5df Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:40:21 -0500 Subject: [PATCH 27/36] e2e: deflake nodedrain test The nodedrain deadline test asserts that all allocations are migrated by the deadline. However, when the deadline is short (e.g. 10s), the test may fail because of scheduler/client-propagation delays. In one failing test, it took ~15s from the RPC call to the moment to the moment the scheduler issued migration update, and then 3 seconds for the alloc to be stopped. Here, I increase the timeouts to avoid such false positives. --- e2e/nodedrain/input/drain_deadline.nomad | 2 +- e2e/nodedrain/nodedrain.go | 16 +++++++++------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/e2e/nodedrain/input/drain_deadline.nomad b/e2e/nodedrain/input/drain_deadline.nomad index c74b896f51b..d869234481c 100644 --- a/e2e/nodedrain/input/drain_deadline.nomad +++ b/e2e/nodedrain/input/drain_deadline.nomad @@ -11,7 +11,7 @@ job "drain_deadline" { task "task" { driver = "docker" - kill_timeout = "30s" + kill_timeout = "2m" config { image = "busybox:1" diff --git a/e2e/nodedrain/nodedrain.go b/e2e/nodedrain/nodedrain.go index d98b841da67..ff74c3f2fd7 100644 --- a/e2e/nodedrain/nodedrain.go +++ b/e2e/nodedrain/nodedrain.go @@ -258,16 +258,18 @@ func (tc *NodeDrainE2ETest) TestNodeDrainDeadline(f *framework.F) { f.Len(nodes, 1, "could not get nodes for job") nodeID := nodes[0] + f.T().Logf("draining node %v", nodeID) out, err := e2e.Command( "nomad", "node", "drain", "-deadline", "5s", "-enable", "-yes", "-detach", nodeID) - f.NoError(err, fmt.Sprintf("'nomad node drain' failed: %v\n%v", err, out)) + f.NoError(err, fmt.Sprintf("'nomad node drain %v' failed: %v\n%v", nodeID, err, out)) tc.nodeIDs = append(tc.nodeIDs, nodeID) - // the deadline is 5s but we can't guarantee its instantly terminated at - // that point, so we give it 10s which is well under the 30s kill_timeout in - // the job + // the deadline is 40s but we can't guarantee its instantly terminated at + // that point, so we give it 30s which is well under the 2m kill_timeout in + // the job. + // deadline here needs to account for scheduling and propagation delays. f.NoError(waitForNodeDrain(nodeID, func(got []map[string]string) bool { for _, alloc := range got { @@ -276,7 +278,7 @@ func (tc *NodeDrainE2ETest) TestNodeDrainDeadline(f *framework.F) { } } return false - }, &e2e.WaitConfig{Interval: time.Millisecond * 100, Retries: 100}, + }, &e2e.WaitConfig{Interval: time.Second, Retries: 40}, ), "node did not drain immediately following deadline") } @@ -304,7 +306,7 @@ func (tc *NodeDrainE2ETest) TestNodeDrainForce(f *framework.F) { tc.nodeIDs = append(tc.nodeIDs, nodeID) // we've passed -force but we can't guarantee its instantly terminated at - // that point, so we give it 20s which is under the 30s kill_timeout in + // that point, so we give it 30s which is under the 2m kill_timeout in // the job f.NoError(waitForNodeDrain(nodeID, func(got []map[string]string) bool { @@ -314,7 +316,7 @@ func (tc *NodeDrainE2ETest) TestNodeDrainForce(f *framework.F) { } } return false - }, &e2e.WaitConfig{Interval: time.Millisecond * 100, Retries: 200}, + }, &e2e.WaitConfig{Interval: time.Second, Retries: 40}, ), "node did not drain immediately when forced") } From 1349e49bec6ac6e3e166366bf9c1fad03fe78ba5 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 09:44:33 -0500 Subject: [PATCH 28/36] e2e: Disable Connect tests The connect tests are very disruptive: restart consul/nomad agents with new tokens. The test seems particularly flaky, failing 32 times out of 73 in my sample. The tests are particularly problematic because they are disruptive and affect other tests. On failure, the nomad or consul agent on the client can get into a wedged state, so health/deployment info in subsequent tests may be wrong. In some cases, the node will be deemed as fail, and then the subsequent tests may fail when the node is deemed lost and the test allocations get migrated unexpectedly. --- e2e/e2e_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 87c0cb9bec3..24fa5409dba 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -10,7 +10,8 @@ import ( _ "github.com/hashicorp/nomad/e2e/affinities" _ "github.com/hashicorp/nomad/e2e/clientstate" - _ "github.com/hashicorp/nomad/e2e/connect" + + // _ "github.com/hashicorp/nomad/e2e/connect" _ "github.com/hashicorp/nomad/e2e/consul" _ "github.com/hashicorp/nomad/e2e/consultemplate" _ "github.com/hashicorp/nomad/e2e/csi" From eb1a84821a4fb4154a1e21a1d19a6b87e5fe7a95 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Tue, 26 Jan 2021 09:06:22 -0600 Subject: [PATCH 29/36] Fix audit workflow action versions (#9877) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the version reference error seen in this workflow failure: https://github.com/hashicorp/nomad/actions/runs/504695096 I’ve also included an update to the sticky comment action version to address this warning in the above link: marocchino/sticky-pull-request-comment@33a6cfb looks like the shortened version of a commit SHA. Referencing actions by the short SHA will be disabled soon. Please see https://docs.github.com/en/actions/learn-github-actions/security-hardening-for-github-actions#using-third-party-actions. We were previously using 33a6cfb after the maintainer merged my PR to allow the comment to be read from a file, there was no released version with that, but it’s now included in v2.0.0. --- .github/workflows/ember-test-audit.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ember-test-audit.yml b/.github/workflows/ember-test-audit.yml index c08140d2e22..0bf6c8c601c 100644 --- a/.github/workflows/ember-test-audit.yml +++ b/.github/workflows/ember-test-audit.yml @@ -66,7 +66,7 @@ jobs: comparison-identifier: ${{ github.event.pull_request.head.sha }} timing-output-path: audit-diff.md flakiness-output-path: flakiness-report.md - - uses: marocchino/sticky-pull-request-comment@33a6cfb + - uses: marocchino/sticky-pull-request-comment@v2.0.0 with: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} path: audit-diff.md @@ -77,7 +77,7 @@ jobs: files: "flakiness-report.md" - name: comment PR if: steps.check_file.outputs.files_exists == 'true' - uses: machine-learning-apps/pr-comment@v1 + uses: machine-learning-apps/pr-comment@1.0.0 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} with: From c76b5e1f642b0b6ff199a786c7eebbad2ece4dac Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Tue, 26 Jan 2021 08:56:52 -0800 Subject: [PATCH 30/36] Use dominant-baseline instead of alignment-baseline to get firefox support --- ui/app/styles/charts/topo-viz-node.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/app/styles/charts/topo-viz-node.scss b/ui/app/styles/charts/topo-viz-node.scss index d067f0b4afc..a7c1c0502f9 100644 --- a/ui/app/styles/charts/topo-viz-node.scss +++ b/ui/app/styles/charts/topo-viz-node.scss @@ -55,7 +55,7 @@ .label { text-anchor: middle; - alignment-baseline: central; + dominant-baseline: central; font-weight: $weight-normal; fill: $grey; pointer-events: none; @@ -68,7 +68,7 @@ text { text-anchor: middle; - alignment-baseline: central; + dominant-baseline: central; } } From ff252db327ce6c78b25b58ea139fa5c048ec935c Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Tue, 26 Jan 2021 10:53:26 -0800 Subject: [PATCH 31/36] Use Hz instead of hz throughout the UI --- ui/app/templates/components/primary-metric.hbs | 2 +- ui/app/templates/components/topo-viz/datacenter.hbs | 2 +- ui/app/templates/components/topo-viz/node.hbs | 2 +- ui/app/templates/topology.hbs | 8 ++++---- ui/mirage/factories/job.js | 2 +- .../integration/components/topo-viz/datacenter-test.js | 2 +- ui/tests/integration/components/topo-viz/node-test.js | 2 +- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ui/app/templates/components/primary-metric.hbs b/ui/app/templates/components/primary-metric.hbs index 81950e698ea..56aba2d7402 100644 --- a/ui/app/templates/components/primary-metric.hbs +++ b/ui/app/templates/components/primary-metric.hbs @@ -19,7 +19,7 @@
{{#if (eq this.metric "cpu")}} - {{this.data.lastObject.used}} Mhz / {{this.reservedAmount}} Mhz reserved + {{this.data.lastObject.used}} MHz / {{this.reservedAmount}} MHz reserved {{else if (eq this.metric "memory")}} {{format-bytes this.data.lastObject.used}} / {{this.reservedAmount}} MiB reserved {{else}} diff --git a/ui/app/templates/components/topo-viz/datacenter.hbs b/ui/app/templates/components/topo-viz/datacenter.hbs index 582565d04a3..cf589c216cb 100644 --- a/ui/app/templates/components/topo-viz/datacenter.hbs +++ b/ui/app/templates/components/topo-viz/datacenter.hbs @@ -4,7 +4,7 @@ {{this.scheduledAllocations.length}} Allocs {{@datacenter.nodes.length}} Nodes {{this.aggregatedAllocationResources.memory}}/{{this.aggregatedNodeResources.memory}} MiB, - {{this.aggregatedAllocationResources.cpu}}/{{this.aggregatedNodeResources.cpu}} Mhz + {{this.aggregatedAllocationResources.cpu}}/{{this.aggregatedNodeResources.cpu}} MHz
diff --git a/ui/app/templates/components/topo-viz/node.hbs b/ui/app/templates/components/topo-viz/node.hbs index f3f7e43adac..409466caa94 100644 --- a/ui/app/templates/components/topo-viz/node.hbs +++ b/ui/app/templates/components/topo-viz/node.hbs @@ -8,7 +8,7 @@ {{/if}} {{@node.node.name}} {{this.count}} Allocs - {{@node.memory}} MiB, {{@node.cpu}} Mhz + {{@node.memory}} MiB, {{@node.cpu}} MHz

{{/unless}}
-

{{this.nodeUtilization.totalCPU}} Mhz of CPU

+

{{this.nodeUtilization.totalCPU}} MHz of CPU

@@ -128,7 +128,7 @@
- {{this.nodeUtilization.totalReservedCPU}} Mhz / {{this.nodeUtilization.totalCPU}} Mhz reserved + {{this.nodeUtilization.totalReservedCPU}} MHz / {{this.nodeUtilization.totalCPU}} MHz reserved
{{/let}} @@ -201,7 +201,7 @@
-

{{this.totalCPU}} Mhz of CPU

+

{{this.totalCPU}} MHz of CPU

@@ -218,7 +218,7 @@
- {{this.totalReservedCPU}} Mhz / {{this.totalCPU}} Mhz reserved + {{this.totalReservedCPU}} MHz / {{this.totalCPU}} MHz reserved
{{/if}} diff --git a/ui/mirage/factories/job.js b/ui/mirage/factories/job.js index a15d99ce65d..e7940461b0b 100644 --- a/ui/mirage/factories/job.js +++ b/ui/mirage/factories/job.js @@ -25,7 +25,7 @@ export default Factory.extend({ // When provided, the resourceSpec will inform how many task groups to create // and how much of each resource that task group reserves. // - // One task group, 256 MiB memory and 500 Mhz cpu + // One task group, 256 MiB memory and 500 MHz cpu // resourceSpec: ['M: 256, C: 500'] // // Two task groups diff --git a/ui/tests/integration/components/topo-viz/datacenter-test.js b/ui/tests/integration/components/topo-viz/datacenter-test.js index c505de8d424..52da79c89bf 100644 --- a/ui/tests/integration/components/topo-viz/datacenter-test.js +++ b/ui/tests/integration/components/topo-viz/datacenter-test.js @@ -108,7 +108,7 @@ module('Integration | Component | TopoViz::Datacenter', function(hooks) { assert.ok(TopoVizDatacenter.label.includes(`${this.datacenter.nodes.length} Nodes`)); assert.ok(TopoVizDatacenter.label.includes(`${allocs.length} Allocs`)); assert.ok(TopoVizDatacenter.label.includes(`${memoryReserved}/${memoryTotal} MiB`)); - assert.ok(TopoVizDatacenter.label.includes(`${cpuReserved}/${cpuTotal} Mhz`)); + assert.ok(TopoVizDatacenter.label.includes(`${cpuReserved}/${cpuTotal} MHz`)); }); test('when @isSingleColumn is true, the FlexMasonry layout gets one column, otherwise it gets two', async function(assert) { diff --git a/ui/tests/integration/components/topo-viz/node-test.js b/ui/tests/integration/components/topo-viz/node-test.js index a249074d55d..c3ffe66759c 100644 --- a/ui/tests/integration/components/topo-viz/node-test.js +++ b/ui/tests/integration/components/topo-viz/node-test.js @@ -96,7 +96,7 @@ module('Integration | Component | TopoViz::Node', function(hooks) { assert.ok(TopoVizNode.label.includes(node.node.name)); assert.ok(TopoVizNode.label.includes(`${this.node.allocations.length} Allocs`)); assert.ok(TopoVizNode.label.includes(`${this.node.memory} MiB`)); - assert.ok(TopoVizNode.label.includes(`${this.node.cpu} Mhz`)); + assert.ok(TopoVizNode.label.includes(`${this.node.cpu} MHz`)); }); test('the status icon indicates when the node is draining', async function(assert) { From e5bdc5cf716f0384963a48b62233fe2012b8616a Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 26 Jan 2021 10:29:45 -0500 Subject: [PATCH 32/36] e2e: use f.NoError instead of requires --- e2e/consul/consul.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/consul/consul.go b/e2e/consul/consul.go index dc61b730607..509a27fdcc6 100644 --- a/e2e/consul/consul.go +++ b/e2e/consul/consul.go @@ -176,7 +176,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { return true, nil }, func(err error) { - require.NoError(t, err, "error while waiting for deploys") + f.NoError(err, "error while waiting for deploys") }) allocID := activeDeploy.TaskGroups["consul_canary_test"].PlacedCanaries[0] @@ -196,7 +196,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { return *alloc.DeploymentStatus.Healthy, fmt.Errorf("expected healthy canary but found: %#v", alloc.DeploymentStatus) }, func(err error) { - require.NoError(t, err, "error waiting for canary to be healthy") + f.NoError(err, "error waiting for canary to be healthy") }) // Check Consul for canary tags @@ -212,7 +212,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { } return false, fmt.Errorf(`could not find service tags {"canary", "foo"}: %#v`, consulServices) }, func(err error) { - require.NoError(t, err, "error waiting for canary tags") + f.NoError(err, "error waiting for canary tags") }) // Promote canary From 38a7e73c91ff841a062bd73e7577f03926e8d911 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Wed, 27 Jan 2021 08:35:03 -0500 Subject: [PATCH 33/36] e2e: skip node drain deadline/force tests --- e2e/nodedrain/nodedrain.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/e2e/nodedrain/nodedrain.go b/e2e/nodedrain/nodedrain.go index ff74c3f2fd7..caa67d6c31e 100644 --- a/e2e/nodedrain/nodedrain.go +++ b/e2e/nodedrain/nodedrain.go @@ -245,6 +245,7 @@ func (tc *NodeDrainE2ETest) TestNodeDrainIgnoreSystem(f *framework.F) { // TestNodeDrainDeadline tests the enforcement of the node drain deadline so // that allocations are terminated even if they haven't gracefully exited. func (tc *NodeDrainE2ETest) TestNodeDrainDeadline(f *framework.F) { + f.T().Skip("The behavior is unclear and test assertions don't capture intent. Issue 9902") jobID := "test-node-drain-" + uuid.Generate()[0:8] f.NoError(e2e.Register(jobID, "nodedrain/input/drain_deadline.nomad")) @@ -272,6 +273,8 @@ func (tc *NodeDrainE2ETest) TestNodeDrainDeadline(f *framework.F) { // deadline here needs to account for scheduling and propagation delays. f.NoError(waitForNodeDrain(nodeID, func(got []map[string]string) bool { + // FIXME: check the drain job alloc specifically. test + // may pass if client had another completed alloc for _, alloc := range got { if alloc["Status"] == "complete" { return true @@ -285,6 +288,7 @@ func (tc *NodeDrainE2ETest) TestNodeDrainDeadline(f *framework.F) { // TestNodeDrainDeadline tests the enforcement of the node drain -force flag // so that allocations are terminated immediately. func (tc *NodeDrainE2ETest) TestNodeDrainForce(f *framework.F) { + f.T().Skip("The behavior is unclear and test assertions don't capture intent. Issue 9902") jobID := "test-node-drain-" + uuid.Generate()[0:8] f.NoError(e2e.Register(jobID, "nodedrain/input/drain_deadline.nomad")) @@ -310,6 +314,8 @@ func (tc *NodeDrainE2ETest) TestNodeDrainForce(f *framework.F) { // the job f.NoError(waitForNodeDrain(nodeID, func(got []map[string]string) bool { + // FIXME: check the drain job alloc specifically. test + // may pass if client had another completed alloc for _, alloc := range got { if alloc["Status"] == "complete" { return true From 3b42d7522579ed527b16a28b648fe5ab34cd20f7 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 26 Jan 2021 16:36:03 -0500 Subject: [PATCH 34/36] docs: add metrics from raft leadership transitions --- website/content/docs/operations/metrics.mdx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/website/content/docs/operations/metrics.mdx b/website/content/docs/operations/metrics.mdx index b53f8beb306..042415e9c45 100644 --- a/website/content/docs/operations/metrics.mdx +++ b/website/content/docs/operations/metrics.mdx @@ -428,6 +428,11 @@ those listed in [Key Metrics](#key-metrics) above. | `nomad.raft.leader.dispatchLog` | Time elapsed to write log, mark in flight, and start replication | Nanoseconds | Summary | host | | `nomad.raft.leader.dispatchNumLogs` | Count of logs dispatched | Integer | Gauge | host | | `nomad.raft.replication.appendEntries` | Raft transaction commit time | ms / Raft Log Append | Timer | | +| `nomad.raft.state.candidate` | Count of entering candidate state | Integer | Gauge | host | +| `nomad.raft.state.follower` | Count of entering follower state | Integer | Gauge | host | +| `nomad.raft.state.leader` | Count of entering leader state | Integer | Gauge | host | +| `nomad.raft.transition.heartbeat_timeout` | Count of failing to heartbeat and starting election | Integer | Gauge | host | +| `nomad.raft.transition.leader_lease_timeout` | Count of stepping down as leader after losing quorum | Integer | Gauge | host | | `nomad.runtime.free_count` | Count of objects freed from heap by go runtime GC | Integer | Gauge | host | | `nomad.runtime.gc_pause_ns` | Go runtime GC pause times | Nanoseconds | Summary | host | | `nomad.runtime.sys_bytes` | Go runtime GC metadata size | # of bytes | Gauge | host | From 494b90c995d3cfb4edec302360f0521fe311b23b Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 27 Jan 2021 10:13:05 -0800 Subject: [PATCH 35/36] Don't render association lines on resize when lines aren't supposed to be shown at all --- ui/app/components/topo-viz.js | 7 +++++++ ui/app/templates/components/topo-viz.hbs | 2 +- ui/tests/integration/components/topo-viz-test.js | 9 +++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/ui/app/components/topo-viz.js b/ui/app/components/topo-viz.js index d1254a0dccf..e46f6b10e24 100644 --- a/ui/app/components/topo-viz.js +++ b/ui/app/components/topo-viz.js @@ -237,6 +237,13 @@ export default class TopoViz extends Component { this.viewportColumns = this.element.clientWidth < 900 ? 1 : 2; } + @action + resizeEdges() { + if (this.activeEdges.length > 0) { + this.computedActiveEdges(); + } + } + @action computedActiveEdges() { // Wait a render cycle diff --git a/ui/app/templates/components/topo-viz.hbs b/ui/app/templates/components/topo-viz.hbs index 8953406f333..697c49747a4 100644 --- a/ui/app/templates/components/topo-viz.hbs +++ b/ui/app/templates/components/topo-viz.hbs @@ -45,7 +45,7 @@
{{#if this.activeAllocation}} - + {{#each this.activeEdges as |edge|}} diff --git a/ui/tests/integration/components/topo-viz-test.js b/ui/tests/integration/components/topo-viz-test.js index 76869b98e41..1ec288f357f 100644 --- a/ui/tests/integration/components/topo-viz-test.js +++ b/ui/tests/integration/components/topo-viz-test.js @@ -1,4 +1,5 @@ import { module, test } from 'qunit'; +import { triggerEvent } from '@ember/test-helpers'; import { setupRenderingTest } from 'ember-qunit'; import hbs from 'htmlbars-inline-precompile'; import { componentA11yAudit } from 'nomad-ui/tests/helpers/a11y-audit'; @@ -144,6 +145,10 @@ module('Integration | Component | TopoViz', function(hooks) { assert.ok(TopoViz.allocationAssociationsArePresent); assert.equal(TopoViz.allocationAssociations.length, selectedAllocations.length * 2); + // Lines get redrawn when the window resizes; make sure the lines persist. + await triggerEvent(window, 'resize'); + assert.equal(TopoViz.allocationAssociations.length, selectedAllocations.length * 2); + await TopoViz.datacenters[0].nodes[0].memoryRects[0].select(); assert.notOk(TopoViz.allocationAssociationsArePresent); }); @@ -167,6 +172,10 @@ module('Integration | Component | TopoViz', function(hooks) { await TopoViz.datacenters[0].nodes[0].memoryRects[0].select(); assert.equal(TopoViz.allocationAssociations.length, 0); + + // Lines get redrawn when the window resizes; make sure that doesn't make the lines show up again + await triggerEvent(window, 'resize'); + assert.equal(TopoViz.allocationAssociations.length, 0); }); test('when one or more nodes are missing the resources property, those nodes are filtered out of the topology view and onDataError is called', async function(assert) { From ac758a7947edf64920e3243ad7f4da673ab9c9f5 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 27 Jan 2021 10:24:03 -0800 Subject: [PATCH 36/36] Adjust the no-association-lines logic On very small clusters, the node count heuristic is impractical and leads to confusion. By additionally requiring 10+ sibling allocs, the lines will be shown more often. --- ui/app/components/topo-viz.js | 4 ++-- ui/tests/integration/components/topo-viz-test.js | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/ui/app/components/topo-viz.js b/ui/app/components/topo-viz.js index e46f6b10e24..9c9ac0fabfb 100644 --- a/ui/app/components/topo-viz.js +++ b/ui/app/components/topo-viz.js @@ -220,8 +220,8 @@ export default class TopoViz extends Component { }); } - // Only show the lines if the selected allocations are sparse (low count relative to the client count). - if (newAllocations.length < this.args.nodes.length * 0.75) { + // Only show the lines if the selected allocations are sparse (low count relative to the client count or low count generally). + if (newAllocations.length < 10 || newAllocations.length < this.args.nodes.length * 0.75) { this.computedActiveEdges(); } else { this.activeEdges = []; diff --git a/ui/tests/integration/components/topo-viz-test.js b/ui/tests/integration/components/topo-viz-test.js index 1ec288f357f..abc0b7a5209 100644 --- a/ui/tests/integration/components/topo-viz-test.js +++ b/ui/tests/integration/components/topo-viz-test.js @@ -157,8 +157,17 @@ module('Integration | Component | TopoViz', function(hooks) { this.setProperties({ nodes: [node('dc1', 'node0', 1000, 500), node('dc1', 'node1', 1000, 500)], allocations: [ + // There need to be at least 10 sibling allocations to trigger this behavior alloc('node0', 'job1', 'group', 100, 100), alloc('node0', 'job1', 'group', 100, 100), + alloc('node0', 'job1', 'group', 100, 100), + alloc('node0', 'job1', 'group', 100, 100), + alloc('node0', 'job1', 'group', 100, 100), + alloc('node0', 'job1', 'group', 100, 100), + alloc('node1', 'job1', 'group', 100, 100), + alloc('node1', 'job1', 'group', 100, 100), + alloc('node1', 'job1', 'group', 100, 100), + alloc('node1', 'job1', 'group', 100, 100), alloc('node1', 'job1', 'group', 100, 100), alloc('node1', 'job1', 'group', 100, 100), alloc('node0', 'job1', 'groupTwo', 100, 100),