From dc1122676348344b572f7876683f11963b82c050 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Fri, 10 Apr 2020 17:42:32 -0600 Subject: [PATCH 1/2] jobspec: correctly parse proxy fields from jobspec Before, the proxy stanza did not parse non-object fields `local_service_port` and `local_service_address` from the connect `proxy` stanza. This change fixes that. --- jobspec/parse_service.go | 18 +++ jobspec/parse_test.go | 116 ++++++++++++++---- .../tg-service-connect-local-service.hcl | 17 +++ .../tg-service-connect-proxy.hcl | 42 +++++++ ... tg-service-connect-sidecar_task-name.hcl} | 0 5 files changed, 171 insertions(+), 22 deletions(-) create mode 100644 jobspec/test-fixtures/tg-service-connect-local-service.hcl create mode 100644 jobspec/test-fixtures/tg-service-connect-proxy.hcl rename jobspec/test-fixtures/{service-connect-sidecar_task-name.hcl => tg-service-connect-sidecar_task-name.hcl} (100%) diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go index 24c35176df7..fa53f087661 100644 --- a/jobspec/parse_service.go +++ b/jobspec/parse_service.go @@ -320,6 +320,24 @@ func parseProxy(o *ast.ObjectItem) (*api.ConsulProxy, error) { } var proxy api.ConsulProxy + var m map[string]interface{} + if err := hcl.DecodeObject(&m, o.Val); err != nil { + return nil, err + } + + delete(m, "upstreams") + delete(m, "expose") + delete(m, "config") + + dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + Result: &proxy, + }) + if err != nil { + return nil, err + } + if err := dec.Decode(m); err != nil { + return nil, fmt.Errorf("proxy: %v", err) + } var listVal *ast.ObjectList if ot, ok := o.Val.(*ast.ObjectType); ok { diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index c171f9acf47..8cd61d7ed24 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -894,28 +894,6 @@ func TestParse(t *testing.T) { }, false, }, - { - "service-connect-sidecar_task-name.hcl", - &api.Job{ - ID: helper.StringToPtr("sidecar_task_name"), - Name: helper.StringToPtr("sidecar_task_name"), - Type: helper.StringToPtr("service"), - TaskGroups: []*api.TaskGroup{{ - Name: helper.StringToPtr("group"), - Services: []*api.Service{{ - Name: "example", - Connect: &api.ConsulConnect{ - Native: false, - SidecarService: &api.ConsulSidecarService{}, - SidecarTask: &api.SidecarTask{ - Name: "my-sidecar", - }, - }, - }}, - }}, - }, - false, - }, { "reschedule-job.hcl", &api.Job{ @@ -1051,6 +1029,7 @@ func TestParse(t *testing.T) { SidecarService: &api.ConsulSidecarService{ Tags: []string{"side1", "side2"}, Proxy: &api.ConsulProxy{ + LocalServicePort: 8080, Upstreams: []*api.ConsulUpstream{ { DestinationName: "other-service", @@ -1172,6 +1151,99 @@ func TestParse(t *testing.T) { }, false, }, + { + "tg-service-connect-sidecar_task-name.hcl", + &api.Job{ + ID: helper.StringToPtr("sidecar_task_name"), + Name: helper.StringToPtr("sidecar_task_name"), + Type: helper.StringToPtr("service"), + TaskGroups: []*api.TaskGroup{{ + Name: helper.StringToPtr("group"), + Services: []*api.Service{{ + Name: "example", + Connect: &api.ConsulConnect{ + Native: false, + SidecarService: &api.ConsulSidecarService{}, + SidecarTask: &api.SidecarTask{ + Name: "my-sidecar", + }, + }, + }}, + }}, + }, + false, + }, + { + "tg-service-connect-proxy.hcl", + &api.Job{ + ID: helper.StringToPtr("service-connect-proxy"), + Name: helper.StringToPtr("service-connect-proxy"), + Type: helper.StringToPtr("service"), + TaskGroups: []*api.TaskGroup{{ + Name: helper.StringToPtr("group"), + Services: []*api.Service{{ + Name: "example", + Connect: &api.ConsulConnect{ + Native: false, + SidecarService: &api.ConsulSidecarService{ + Proxy: &api.ConsulProxy{ + LocalServiceAddress: "10.0.1.2", + LocalServicePort: 8080, + ExposeConfig: &api.ConsulExposeConfig{ + Path: []*api.ConsulExposePath{{ + Path: "/metrics", + Protocol: "http", + LocalPathPort: 9001, + ListenerPort: "metrics", + }, { + Path: "/health", + Protocol: "http", + LocalPathPort: 9002, + ListenerPort: "health", + }}, + }, + Upstreams: []*api.ConsulUpstream{{ + DestinationName: "upstream1", + LocalBindPort: 2001, + }, { + DestinationName: "upstream2", + LocalBindPort: 2002, + }}, + Config: map[string]interface{}{ + "foo": "bar", + }, + }, + }, + }, + }}, + }}, + }, + false, + }, + { + "tg-service-connect-local-service.hcl", + &api.Job{ + ID: helper.StringToPtr("connect-proxy-local-service"), + Name: helper.StringToPtr("connect-proxy-local-service"), + Type: helper.StringToPtr("service"), + TaskGroups: []*api.TaskGroup{{ + Name: helper.StringToPtr("group"), + Services: []*api.Service{{ + Name: "example", + Connect: &api.ConsulConnect{ + Native: false, + SidecarService: &api.ConsulSidecarService{ + Proxy: &api.ConsulProxy{ + LocalServiceAddress: "10.0.1.2", + LocalServicePort: 9876, + }, + }, + }, + }}, + }}, + }, + false, + }, { "tg-service-check-expose.hcl", &api.Job{ diff --git a/jobspec/test-fixtures/tg-service-connect-local-service.hcl b/jobspec/test-fixtures/tg-service-connect-local-service.hcl new file mode 100644 index 00000000000..dc97689b86e --- /dev/null +++ b/jobspec/test-fixtures/tg-service-connect-local-service.hcl @@ -0,0 +1,17 @@ +job "connect-proxy-local-service" { + type = "service" + + group "group" { + service { + name = "example" + connect { + sidecar_service { + proxy { + local_service_port = 9876 + local_service_address = "10.0.1.2" + } + } + } + } + } +} \ No newline at end of file diff --git a/jobspec/test-fixtures/tg-service-connect-proxy.hcl b/jobspec/test-fixtures/tg-service-connect-proxy.hcl new file mode 100644 index 00000000000..841742c920a --- /dev/null +++ b/jobspec/test-fixtures/tg-service-connect-proxy.hcl @@ -0,0 +1,42 @@ +job "service-connect-proxy" { + type = "service" + + group "group" { + service { + name = "example" + connect { + sidecar_service { + proxy { + local_service_port = 8080 + local_service_address = "10.0.1.2" + upstreams { + destination_name = "upstream1" + local_bind_port = 2001 + } + upstreams { + destination_name = "upstream2" + local_bind_port = 2002 + } + expose { + path { + path = "/metrics" + protocol = "http" + local_path_port = 9001 + listener_port = "metrics" + } + path { + path = "/health" + protocol = "http" + local_path_port = 9002 + listener_port = "health" + } + } + config { + foo = "bar" + } + } + } + } + } + } +} \ No newline at end of file diff --git a/jobspec/test-fixtures/service-connect-sidecar_task-name.hcl b/jobspec/test-fixtures/tg-service-connect-sidecar_task-name.hcl similarity index 100% rename from jobspec/test-fixtures/service-connect-sidecar_task-name.hcl rename to jobspec/test-fixtures/tg-service-connect-sidecar_task-name.hcl From eef81c3b4f400a3a01705b410f3440c51b7cd12e Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Fri, 10 Apr 2020 17:44:19 -0600 Subject: [PATCH 2/2] structs: fix compatibility between api and nomad/structs proxy definitions The field names within the structs representing the Connect proxy definition were not the same (nomad/structs/ vs api/), causing the values to be lost in translation for the 'nomad job inspect' command. Since the field names already shipped in v0.11.0 we cannot simply fix the names. Instead, use the json struct tag on the structs/ structs to remap the name to match the publicly expose api/ package on json encoding. This means existing jobs from v0.11.0 will continue to work, and the JSON API for job submission will remain backwards compatible. --- contributing/checklist-jobspec.md | 1 + nomad/structs/services.go | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/contributing/checklist-jobspec.md b/contributing/checklist-jobspec.md index 83db385f850..5d6215124b4 100644 --- a/contributing/checklist-jobspec.md +++ b/contributing/checklist-jobspec.md @@ -15,6 +15,7 @@ * [ ] Add structs/fields to `nomad/structs` package * Validation happens in this package and must be implemented * Implement other methods and tests from `api/` package + * Note that analogous struct field names should match with `api/` package * [ ] Add conversion between `api/` and `nomad/structs` in `command/agent/job_endpoint.go` * [ ] Add check for job diff in `nomad/structs/diff.go` * Note that fields must be listed in alphabetical order in `FieldDiff` slices in `nomad/structs/diff_test.go` diff --git a/nomad/structs/services.go b/nomad/structs/services.go index d12bb697f98..578b1dd22e3 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -889,7 +889,9 @@ type ConsulProxy struct { // Expose configures the consul proxy.expose stanza to "open up" endpoints // used by task-group level service checks using HTTP or gRPC protocols. - Expose *ConsulExposeConfig + // + // Use json tag to match with field name in api/ + Expose *ConsulExposeConfig `json:"ExposeConfig"` // Config is a proxy configuration. It is opaque to Nomad and passed // directly to Consul. @@ -905,7 +907,7 @@ func (p *ConsulProxy) Copy() *ConsulProxy { newP := &ConsulProxy{ LocalServiceAddress: p.LocalServiceAddress, LocalServicePort: p.LocalServicePort, - Expose: p.Expose, + Expose: p.Expose.Copy(), } if n := len(p.Upstreams); n > 0 { @@ -1009,7 +1011,8 @@ func (u *ConsulUpstream) Equals(o *ConsulUpstream) bool { // ExposeConfig represents a Consul Connect expose jobspec stanza. type ConsulExposeConfig struct { - Paths []ConsulExposePath + // Use json tag to match with field name in api/ + Paths []ConsulExposePath `json:"Path"` } type ConsulExposePath struct {