diff --git a/CHANGELOG.md b/CHANGELOG.md index 803645510ee1..0cf14ad79c60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)] diff --git a/nomad/job_endpoint_hook_expose_check.go b/nomad/job_endpoint_hook_expose_check.go index 7b2202438a3e..caff130c928e 100644 --- a/nomad/job_endpoint_hook_expose_check.go +++ b/nomad/job_endpoint_hook_expose_check.go @@ -5,7 +5,6 @@ import ( "strconv" "strings" - "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" "github.com/pkg/errors" ) @@ -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. @@ -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) @@ -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 } @@ -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) diff --git a/nomad/job_endpoint_hook_expose_check_test.go b/nomad/job_endpoint_hook_expose_check_test.go index 6196435c7fa2..f3a2cbe3a0ef 100644 --- a/nomad/job_endpoint_hook_expose_check_test.go +++ b/nomad/job_endpoint_hook_expose_check_test.go @@ -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 @@ -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) }) @@ -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", @@ -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", @@ -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) { @@ -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) })