Allow custom ports for services and checks when using driver address_mode #3619

schmichael · 2017-12-05T19:40:47Z

Adds address_mode to checks (but no auto) and allows services and checks
to set literal port numbers when using address_mode=driver.

This allows SDNs, overlays, etc to advertise internal and host addresses
as well as do checks against either.

`task_runner.go` changes

Fixes a bug found when using fewer mocks in TaskRunner tests: changing only an interpolated variable used in a service or a check wouldn't actually update the service or check.

schmichael · 2017-12-05T19:45:56Z

Some test binaries for the brave:

old
~~linux_amd64.zip~~
~~windows_amd64.zip~~

2017-02-06 4:10pm Pacific
linux_amd64.zip

schmichael · 2017-12-07T00:07:28Z

client/driver/mock_driver.go

+			case <-h.doneCh:
+				// already closed
+			default:
+				close(h.doneCh)


I ran into a double close panic on the mock driver when testing, so I guarded all of these closes. Not sure why we had never hit it before...

schmichael · 2017-12-07T00:11:20Z

client/driver/mock_driver.go

@@ -133,7 +162,8 @@ func (m *MockDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse
 	}
 	m.logger.Printf("[DEBUG] driver.mock: starting task %q", task.Name)
 	go h.run()
-	return &StartResponse{Handle: &h}, nil
+
+	return &StartResponse{Handle: &h, Network: net}, nil


mock_driver now supports DriverNetwork! I should have added this when I added DriverNetwork originally...

schmichael · 2017-12-07T00:12:10Z

client/task_runner_test.go

+	allocDir     *allocdir.AllocDir
+	vault        *vaultclient.MockVaultClient
+	consul       *consul.MockAgent
+	consulClient *consul.ServiceClient


Removing this use of *mockConsulServiceClient is what exposed the task runner bug.

There's still a few tests that use that mock as they inspect the order of Consul operations which the mock makes very easy.

schmichael · 2017-12-07T00:13:06Z

command/agent/consul/client.go

@@ -1079,3 +1083,44 @@ func isNomadService(id string) bool {
 	const prefix = nomadServicePrefix + "-executor"
 	return strings.HasPrefix(id, prefix)
 }
+


Below is the core address resolution logic for both services and checks.

dadgar · 2017-12-07T00:50:51Z

jobspec/parse_test.go

@@ -580,6 +628,7 @@ func TestParse(t *testing.T) {
 			for _, d := range pretty.Diff(actual, tc.Result) {
 				t.Logf(d)
 			}
+			//t.Logf(pretty.Sprint(actual))


dadgar · 2017-12-07T00:52:49Z

website/source/docs/job-specification/service.html.md

@@ -120,6 +122,11 @@ the script will run inside the Docker container. If your task is running in a
 chroot, it will run in the chroot. Please keep this in mind when authoring check
 scripts.

+- `address_mode` `(string: "host")` - Same as `address_mode` on `service`.


I would add a full example for this and link it to this

Added a full section at the bottom of that page with a link

preetapan · 2017-12-07T01:04:36Z

nomad/structs/structs.go

@@ -3444,7 +3462,12 @@ func validateServices(t *Task) error {
 		knownServices[service.Name+service.PortLabel] = struct{}{}

 		if service.PortLabel != "" {
-			servicePorts[service.PortLabel] = append(servicePorts[service.PortLabel], service.Name)
+			if _, err := strconv.Atoi(service.PortLabel); service.AddressMode == "driver" && err == nil {


This is hard to read, and I am not sure if the logic is correct. If addressmode is not driver, and the strconv succeeds (if portlabel is an integer), then should it be allowed or should that throw an error? if not, why even do the parsing?

thetooth · 2017-12-07T12:59:04Z

I have setup a test nomad job but am not getting working health checks:
agent: socket connection failed ':0': dial tcp :0: getsockopt: connection refused

      service {
        name = "poc-server"
        address_mode = "driver"
        port = "6379"
        check {
          type     = "tcp"
          interval = "10s"
          timeout  = "2s"
        }
      }

preetapan · 2017-12-07T17:02:13Z

website/source/docs/job-specification/service.html.md

+  and port. `driver` advertises the IP used in the driver (e.g. Docker's
+  internal IP) and uses the container's port specified in the port map. The
+  default is `auto` which behaves the same as `host` unless the driver
+  determines its IP should be used.  This setting supported Docker since Nomad


Supported in Docker

preetapan · 2017-12-07T17:04:15Z

website/source/docs/job-specification/service.html.md

+  default is `auto` which behaves the same as `host` unless the driver
+  determines its IP should be used.  This setting supported Docker since Nomad
+  0.6 and rkt since Nomad 0.7. It will advertise the container IP if a network
+  plugin is used (e.g. weave).


I would make each valid option sub items or a table with option/behavior as headings, and 1/2 lines explaining them. I would also start with the default option auto.

preetapan · 2017-12-07T17:21:31Z

command/agent/consul/client.go

+		if err != nil {
+			return "", 0, fmt.Errorf("invalid port %q: %v", portLabel, err)
+		}
+


Should this have a check for port > 0 - If some job file misconfiguration leads to someone specifying 0 as the port label, we should not try to use it and fail fast..

preetapan

Had some questions and small suggestions to the documentation.

schmichael · 2017-12-08T06:15:23Z

@thetooth Thanks for testing! Fixed in f4e341e

Since checks inherit the service's port (a numeric port in this case) but do not inherit the address_mode - the numeric port 6379 was being looked as a port label due to the check's default host address mode. I didn't have validation to catch this error and the empty value (0) was being used for the port!

I can see that it's a bit confusing to have port inherited by check but not address_mode, but I think the behavior matches many people's configuration (Consul is running on the host so service checks should be against the host address). Let me know if there's anything other than this validation that could help prevent such errors!

Here's another build:

linux_amd64.zip

thetooth · 2017-12-08T12:04:13Z

@schmichael Some usability issues:

The following configuration does not work as expected. Since I am not using a port map I tried a deploy without a port label in resources,

      service {
        name = "poc-server"
        address_mode = "driver"
        port = "6379"
        check {
          type     = "tcp"
          interval = "10s"
          timeout  = "2s"
        }
      }
      resources {
        cpu    = 512 # MHz 
        memory = 512 # MB 
        network {
          mbits = 10
        }
      }

The error for this was unable to get address for check "service: \"poc-server\" check": invalid port "6379": port label not found, so then I defined a port label:

          port "redis" {
            static = 6379
          }

Which cased unable to get address for service "poc-server": invalid port "redis": strconv.Atoi: parsing "redis": invalid syntax, finally I changed the port label to the port itself:

          port "6379" {
            static = 6379
          }

This worked and I have working service checks to the docker network(weave mesh) however as I hinted at in my earlier comment, since the port label is used docker has been instructed to publish those ports to localhost: 127.0.0.1:6379->6379/tcp, 127.0.0.1:6379->6379/udp. It would be very useful from a security perspective to not have services exposed to local sessions as an artifact of using health checks.

preetapan · 2017-12-08T14:12:42Z

website/source/docs/job-specification/service.html.md

+  determines its IP should be used.  This setting is supported in Docker since
+  Nomad 0.6 and rkt since Nomad 0.7. Nomad will advertise the container IP if a
+  network plugin is used (e.g. weave). See [below for
+  details.](#using-driver-address-mode)


Did you see the other comment I had about making each of the modes sub list headers for clarity?

preetapan · 2017-12-08T14:14:59Z

command/agent/consul/client.go

+		if err != nil {
+			return "", 0, fmt.Errorf("invalid port %q: %v", portLabel, err)
+		}
+		if port == 0 {


Change to <=0

preetapan

Two small comments, LGTM otherwise

Fixes #3380 Adds address_mode to checks (but no auto) and allows services and checks to set literal port numbers when using address_mode=driver. This allows SDNs, overlays, etc to advertise internal and host addresses as well as do checks against either.

Rely less on the mockConsulServiceClient because the real consul.ServiceClient needs all the testing it can get!

Previously if only an interpolated variable used in a service or check was changed we interpolated the old and new services and checks with the new variable, so nothing appeared to have changed.

Also skip getting an address for script checks which don't use them. Fixed a weird invalid reserved port in a TaskRunner test helper as well as a problem with our mock Alloc/Job. Hopefully the latter doesn't cause other tests to fail, but we were referencing an invalid PortLabel and just not catching it before.

Fixes #3620 Previously we concatenated tags into task service IDs. This could break deregistration of tag names that contained double //s like some Fabio tags. This change breaks service ID backward compatibility so on upgrade all users services and checks will be removed and re-added with new IDs. This change has the side effect of including all service fields in the ID's hash, so we no longer have to track PortLabel and AddressMode changes independently.

schmichael · 2017-12-08T20:45:56Z

@thetooth Thanks for testing. The check is inheriting port=6379 from the service but not the service's address mode. This is by design as we want to maintain the old behavior of check.address_mode=host by default.

However, the error you're receiving is entirely unhelpful! The fix is to specify address_mode = "driver" on the check or expose a port via a resource & port map on the host for the check to use. I'll try to update the validation to reflect this.

schmichael · 2017-12-11T19:43:21Z

Not sure why TestConfig_Listener is failing on Travis, but it passes for me locally. Merging this 😬

--- FAIL: TestConfig_Listener (0.02s)
	config_test.go:524: err: listen tcp 127.0.0.1:24000: bind: address already in use

github-actions · 2023-03-16T02:15:27Z

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

schmichael mentioned this pull request Dec 5, 2017

Allow health checks to use exposed container ports on driver-assigned ip. #3380

Closed

schmichael force-pushed the f-3380-custom-ports branch 3 times, most recently from 9239f64 to 994cc07 Compare December 7, 2017 00:04

schmichael changed the title ~~WIP: allow custom service/check ports with driver networking~~ Allow custom ports for services and checks when using driver address_mode Dec 7, 2017

schmichael commented Dec 7, 2017

View reviewed changes

dadgar approved these changes Dec 7, 2017

View reviewed changes

preetapan reviewed Dec 7, 2017

View reviewed changes

preetapan suggested changes Dec 7, 2017

View reviewed changes

schmichael force-pushed the f-3380-custom-ports branch 3 times, most recently from 4c41676 to c68ed09 Compare December 8, 2017 06:10

preetapan reviewed Dec 8, 2017

View reviewed changes

preetapan approved these changes Dec 8, 2017

View reviewed changes

schmichael added 5 commits December 8, 2017 12:03

Test Consul from TaskRunner thoroughly

74e7d57

Rely less on the mockConsulServiceClient because the real consul.ServiceClient needs all the testing it can get!

Fix interpolation bug with service/check updates

29e6f7c

Previously if only an interpolated variable used in a service or check was changed we interpolated the old and new services and checks with the new variable, so nothing appeared to have changed.

Document service/check address_mode/port changes

f2d4715

Add Using Driver Address Mode docs

f11360a

schmichael added 5 commits December 8, 2017 12:03

Prevent using port 0 with address_mode=driver

fa5faa5

Improve port label validation and diff testing

7bf5cec

Move service hash logic to Service.Hash method

afd5bca

schmichael force-pushed the f-3380-custom-ports branch from a512a8c to afd5bca Compare December 8, 2017 20:03

schmichael added 2 commits December 8, 2017 12:27

Be more defensive in port checks

6f124eb

Expand port and address_mode docs

4196360

schmichael added 4 commits December 8, 2017 13:52

Improve validation and testing of service/check ports

2a33e10

Fix port labels on mock Alloc/Job/Node

e857225

Lowercase service IDs to prevent eye bleeding

63d515d

Fix test broken by mock updates

b79194f

schmichael mentioned this pull request Dec 11, 2017

[WIP] Log missing mapped ports instead of failing #3637

Closed

schmichael merged commit 82cbb70 into master Dec 11, 2017

schmichael deleted the f-3380-custom-ports branch December 11, 2017 19:43

pznamensky mentioned this pull request Dec 19, 2017

port label not found error in case when ports are not needed #3673

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom ports for services and checks when using driver address_mode #3619

Allow custom ports for services and checks when using driver address_mode #3619

schmichael commented Dec 5, 2017 •

edited

Loading

schmichael commented Dec 5, 2017 •

edited

Loading

schmichael Dec 7, 2017

schmichael Dec 7, 2017

schmichael Dec 7, 2017

schmichael Dec 7, 2017

dadgar Dec 7, 2017

dadgar Dec 7, 2017

schmichael Dec 8, 2017

preetapan Dec 7, 2017 •

edited

Loading

thetooth commented Dec 7, 2017

preetapan Dec 7, 2017

preetapan Dec 7, 2017

preetapan Dec 7, 2017

preetapan left a comment

schmichael commented Dec 8, 2017

thetooth commented Dec 8, 2017

preetapan Dec 8, 2017

preetapan Dec 8, 2017

preetapan left a comment

schmichael commented Dec 8, 2017

schmichael commented Dec 11, 2017

github-actions bot commented Mar 16, 2023

Allow custom ports for services and checks when using driver address_mode #3619

Allow custom ports for services and checks when using driver address_mode #3619

Conversation

schmichael commented Dec 5, 2017 • edited Loading

task_runner.go changes

schmichael commented Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

thetooth commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan left a comment

Choose a reason for hiding this comment

schmichael commented Dec 8, 2017

thetooth commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan left a comment

Choose a reason for hiding this comment

schmichael commented Dec 8, 2017

schmichael commented Dec 11, 2017

github-actions bot commented Mar 16, 2023

schmichael commented Dec 5, 2017 •

edited

Loading

`task_runner.go` changes

schmichael commented Dec 5, 2017 •

edited

Loading

preetapan Dec 7, 2017 •

edited

Loading