Skip to content

Commit

Permalink
Validate port label for host address mode
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
schmichael committed Dec 8, 2017
1 parent 1fb31c9 commit f4e341e
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 6 deletions.
5 changes: 1 addition & 4 deletions client/task_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,6 @@ func testTaskRunnerFromAlloc(t *testing.T, restarts bool, alloc *structs.Allocat

upd := &MockTaskStateUpdater{}
task := alloc.Job.TaskGroups[0].Tasks[0]
// Initialize the port listing. This should be done by the offer process but
// we have a mock so that doesn't happen.
task.Resources.Networks[0].ReservedPorts = []structs.Port{{Label: "", Value: 80}}

allocDir := allocdir.NewAllocDir(testLogger(), filepath.Join(conf.AllocDir, alloc.ID))
if err := allocDir.Build(); err != nil {
Expand Down Expand Up @@ -347,7 +344,7 @@ func TestTaskRunner_Update(t *testing.T) {
task.Services[0].Checks[0] = &structs.ServiceCheck{
Name: "http-check",
Type: "http",
PortLabel: "web",
PortLabel: "http",
Path: "${NOMAD_META_foo}",
}
task.Driver = "mock_driver"
Expand Down
10 changes: 10 additions & 0 deletions command/agent/consul/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,13 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se
ops.scripts = append(ops.scripts, newScriptCheck(
allocID, task.Name, checkID, check, exec, c.client, c.logger, c.shutdownCh))

// Skip getAddress for script checks
checkReg, err := createCheckReg(serviceID, checkID, check, "", 0)
if err != nil {
return nil, fmt.Errorf("failed to add script check %q: %v", check.Name, err)
}
ops.regChecks = append(ops.regChecks, checkReg)
continue
}

// Default to the service's port but allow check to override
Expand Down Expand Up @@ -1098,6 +1105,9 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet
case structs.AddressModeHost:
// Default path: use host ip:port
ip, port := networks.Port(portLabel)
if ip == "" && port == 0 {
return "", 0, fmt.Errorf("invalid port %q: port label not found", portLabel)
}
return ip, port, nil

case structs.AddressModeDriver:
Expand Down
12 changes: 11 additions & 1 deletion command/agent/consul/unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1541,6 +1541,12 @@ func TestGetAddress(t *testing.T) {
},
ErrContains: "invalid port",
},
{
Name: "HostBadPort",
Mode: structs.AddressModeHost,
PortLabel: "bad-port-label",
ErrContains: "invalid port",
},
{
Name: "InvalidMode",
Mode: "invalid-mode",
Expand Down Expand Up @@ -1574,7 +1580,11 @@ func TestGetAddress(t *testing.T) {
if tc.ErrContains == "" {
assert.Nil(t, err)
} else {
assert.Contains(t, err.Error(), tc.ErrContains)
if err == nil {
t.Fatalf("expected error containing %q but err=nil", tc.ErrContains)
} else {
assert.Contains(t, err.Error(), tc.ErrContains)
}
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion nomad/mock/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func Job() *structs.Job {
},
{
Name: "${TASK}-admin",
PortLabel: "admin",
PortLabel: "main",
},
},
LogConfig: structs.DefaultLogConfig(),
Expand Down

0 comments on commit f4e341e

Please sign in to comment.