Skip to content

Commit

Permalink
connect: use deterministic injected dynamic exposed port
Browse files Browse the repository at this point in the history
This PR uses the checksum of the check for which a dynamic exposed
port is being generated (instead of a UUID prefix) so that the
generated port label is deterministic.

This fixes 2 bugs:
 - 'job plan' output is now idempotent for jobs making use of injected ports
 - tasks will no longer be destructively updated when jobs making use of
   injected ports are re-run without changing any user specified part of
   job config.

Closes: #10099
  • Loading branch information
shoenig committed Apr 30, 2021
1 parent 3dfb664 commit 08a766a
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 29 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ BUG FIXES:
* cli: Remove extra linefeeds in monitor.log files written by `nomad operator debug`. [[GH-10252](https://github.com/hashicorp/nomad/issues/10252)]
* client: Fixed log formatting when killing tasks. [[GH-10135](https://github.com/hashicorp/nomad/issues/10135)]
* client: Fixed a bug where small files would be assigned the wrong content type. [[GH-10348](https://github.com/hashicorp/nomad/pull/10348)]
* consul/connect: Fixed a bug where job plan always different when using expose checks. [[GH-10492](https://github.com/hashicorp/nomad/pull/10492)]
* consul/connect: Fixed a bug where HTTP ingress gateways could not use wildcard names. [[GH-10457](https://github.com/hashicorp/nomad/pull/10457)]
* csi: Fixed a bug where volume with IDs that are a substring prefix of another volume could use the wrong volume for feasibility checking. [[GH-10158](https://github.com/hashicorp/nomad/issues/10158)]
* scheduler: Fixed a bug where Nomad reports negative or incorrect running children counts for periodic jobs. [[GH-10145](https://github.com/hashicorp/nomad/issues/10145)]
Expand Down
18 changes: 12 additions & 6 deletions nomad/job_endpoint_hook_expose_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"strconv"
"strings"

"github.com/hashicorp/nomad/helper/uuid"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/pkg/errors"
)
Expand All @@ -22,7 +21,7 @@ func (jobExposeCheckHook) Name() string {
func (jobExposeCheckHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error, err error) {
for _, tg := range job.TaskGroups {
for _, s := range tg.Services {
for _, c := range s.Checks {
for i, c := range s.Checks {
if c.Expose {
// TG isn't validated yet, but validation
// may depend on mutation results.
Expand All @@ -33,7 +32,7 @@ func (jobExposeCheckHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []e
continue
}

if exposePath, err := exposePathForCheck(tg, s, c); err != nil {
if exposePath, err := exposePathForCheck(tg, s, c, i); err != nil {
return nil, nil, err
} else if exposePath != nil {
serviceExposeConfig := serviceExposeConfig(s)
Expand Down Expand Up @@ -180,7 +179,7 @@ func checkIsExposable(check *structs.ServiceCheck) bool {
// exposePathForCheck extrapolates the necessary expose path configuration for
// the given consul service check. If the check is not compatible, nil is
// returned.
func exposePathForCheck(tg *structs.TaskGroup, s *structs.Service, check *structs.ServiceCheck) (*structs.ConsulExposePath, error) {
func exposePathForCheck(tg *structs.TaskGroup, s *structs.Service, check *structs.ServiceCheck, i int) (*structs.ConsulExposePath, error) {
if !checkIsExposable(check) {
return nil, nil
}
Expand All @@ -197,9 +196,16 @@ func exposePathForCheck(tg *structs.TaskGroup, s *structs.Service, check *struct
//
// This lets PortLabel be optional for any exposed check.
if check.PortLabel == "" {

// Note: because the check label is not set yet, and we want to create a
// deterministic label based on the check itself, use the index of the check
// on the service as part of the service name as input into Hash, ensuring
// the hash for the check is unique.
suffix := check.Hash(fmt.Sprintf("%s_%d", s.Name, i))[:6]
port := structs.Port{
Label: fmt.Sprintf("svc_%s_ck_%s", s.Name, uuid.Generate()[:6]),
To: -1,
HostNetwork: "default",
Label: fmt.Sprintf("svc_%s_ck_%s", s.Name, suffix),
To: -1,
}

tg.Networks[0].DynamicPorts = append(tg.Networks[0].DynamicPorts, port)
Expand Down
69 changes: 46 additions & 23 deletions nomad/job_endpoint_hook_expose_check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ func TestJobExposeCheckHook_Validate(t *testing.T) {
func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) {
t.Parallel()

const checkIdx = 0

t.Run("not expose compatible", func(t *testing.T) {
c := &structs.ServiceCheck{
Type: "tcp", // not expose compatible
Expand All @@ -235,7 +237,7 @@ func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) {
}
ePath, err := exposePathForCheck(&structs.TaskGroup{
Services: []*structs.Service{s},
}, s, c)
}, s, c, checkIdx)
require.NoError(t, err)
require.Nil(t, ePath)
})
Expand All @@ -255,7 +257,7 @@ func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) {
ePath, err := exposePathForCheck(&structs.TaskGroup{
Name: "group1",
Services: []*structs.Service{s},
}, s, c)
}, s, c, checkIdx)
require.NoError(t, err)
require.Equal(t, &structs.ConsulExposePath{
Path: "/health",
Expand Down Expand Up @@ -286,7 +288,7 @@ func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) {
{Label: "sPort", Value: 4000},
},
}},
}, s, c)
}, s, c, checkIdx)
require.NoError(t, err)
require.Equal(t, &structs.ConsulExposePath{
Path: "/health",
Expand Down Expand Up @@ -317,38 +319,59 @@ func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) {
// service declares "sPort", but does not exist
},
}},
}, s, c)
}, s, c, checkIdx)
require.EqualError(t, err, `unable to determine local service port for service check group1->service1->check1`)
})

t.Run("empty check port", func(t *testing.T) {
c := &structs.ServiceCheck{
Name: "check1",
Type: "http",
Path: "/health",
}
s := &structs.Service{
Name: "service1",
PortLabel: "9999",
Checks: []*structs.ServiceCheck{c},
}
tg := &structs.TaskGroup{
Name: "group1",
Services: []*structs.Service{s},
Networks: structs.Networks{{
Mode: "bridge",
DynamicPorts: []structs.Port{},
}},
setup := func() (*structs.TaskGroup, *structs.Service, *structs.ServiceCheck) {
c := &structs.ServiceCheck{
Name: "check1",
Type: "http",
Path: "/health",
}
s := &structs.Service{
Name: "service1",
PortLabel: "9999",
Checks: []*structs.ServiceCheck{c},
}
tg := &structs.TaskGroup{
Name: "group1",
Services: []*structs.Service{s},
Networks: structs.Networks{{
Mode: "bridge",
DynamicPorts: []structs.Port{},
}},
}
return tg, s, c
}
ePath, err := exposePathForCheck(tg, s, c)

tg, s, c := setup()
ePath, err := exposePathForCheck(tg, s, c, checkIdx)
require.NoError(t, err)
require.Len(t, tg.Networks[0].DynamicPorts, 1)
require.Equal(t, "default", tg.Networks[0].DynamicPorts[0].HostNetwork)
require.Equal(t, "svc_", tg.Networks[0].DynamicPorts[0].Label[0:4])
require.Equal(t, &structs.ConsulExposePath{
Path: "/health",
Protocol: "",
LocalPathPort: 9999,
ListenerPort: tg.Networks[0].DynamicPorts[0].Label,
}, ePath)

t.Run("deterministic generated port label", func(t *testing.T) {
tg2, s2, c2 := setup()
ePath2, err2 := exposePathForCheck(tg2, s2, c2, checkIdx)
require.NoError(t, err2)
require.Equal(t, ePath, ePath2)
})

t.Run("unique on check index", func(t *testing.T) {
tg3, s3, c3 := setup()
ePath3, err3 := exposePathForCheck(tg3, s3, c3, checkIdx+1)
require.NoError(t, err3)
require.NotEqual(t, ePath.ListenerPort, ePath3.ListenerPort)
})
})

t.Run("missing network with no service check port label", func(t *testing.T) {
Expand All @@ -370,7 +393,7 @@ func TestJobExposeCheckHook_exposePathForCheck(t *testing.T) {
Services: []*structs.Service{s},
Networks: nil, // not set, should cause validation error
}
ePath, err := exposePathForCheck(tg, s, c)
ePath, err := exposePathForCheck(tg, s, c, checkIdx)
require.EqualError(t, err, `group "group1" must specify one bridge network for exposing service check(s)`)
require.Nil(t, ePath)
})
Expand Down

0 comments on commit 08a766a

Please sign in to comment.