From fbcccaf97d04f56b49a8b85b6265a3ac6dcbdb08 Mon Sep 17 00:00:00 2001 From: Jorge Marey Date: Sat, 28 Jan 2023 17:32:46 +0100 Subject: [PATCH 1/4] Change api Fields for expose and paths --- api/consul.go | 22 +++++++++++++++------- api/consul_test.go | 6 +++--- command/agent/job_endpoint.go | 18 ++++++++++++++++-- command/agent/job_endpoint_test.go | 6 +++--- jobspec/parse_service.go | 6 +++--- jobspec/parse_test.go | 8 ++++---- nomad/structs/services.go | 7 ++----- 7 files changed, 46 insertions(+), 27 deletions(-) diff --git a/api/consul.go b/api/consul.go index 0452d2ff18f1..f2553010ae6a 100644 --- a/api/consul.go +++ b/api/consul.go @@ -135,11 +135,13 @@ func (st *SidecarTask) Canonicalize() { // ConsulProxy represents a Consul Connect sidecar proxy jobspec stanza. type ConsulProxy struct { - LocalServiceAddress string `mapstructure:"local_service_address" hcl:"local_service_address,optional"` - LocalServicePort int `mapstructure:"local_service_port" hcl:"local_service_port,optional"` - ExposeConfig *ConsulExposeConfig `mapstructure:"expose" hcl:"expose,block"` - Upstreams []*ConsulUpstream `hcl:"upstreams,block"` - Config map[string]interface{} `hcl:"config,block"` + LocalServiceAddress string `mapstructure:"local_service_address" hcl:"local_service_address,optional"` + LocalServicePort int `mapstructure:"local_service_port" hcl:"local_service_port,optional"` + Expose *ConsulExposeConfig `mapstructure:"expose" hcl:"expose,block"` + // Deprecated, only to maintain backwards compatibility. Use Expose instead + ExposeConfig *ConsulExposeConfig + Upstreams []*ConsulUpstream `hcl:"upstreams,block"` + Config map[string]interface{} `hcl:"config,block"` } func (cp *ConsulProxy) Canonicalize() { @@ -147,7 +149,7 @@ func (cp *ConsulProxy) Canonicalize() { return } - cp.ExposeConfig.Canonicalize() + cp.Expose.Canonicalize() if len(cp.Upstreams) == 0 { cp.Upstreams = nil @@ -234,7 +236,9 @@ func (cu *ConsulUpstream) Canonicalize() { } type ConsulExposeConfig struct { - Path []*ConsulExposePath `mapstructure:"path" hcl:"path,block"` + Paths []*ConsulExposePath `mapstructure:"path" hcl:"path,block"` + // Deprecated, only to maintain backwards compatibility. Use Paths instead + Path []*ConsulExposePath } func (cec *ConsulExposeConfig) Canonicalize() { @@ -242,6 +246,10 @@ func (cec *ConsulExposeConfig) Canonicalize() { return } + if len(cec.Paths) == 0 { + cec.Paths = nil + } + if len(cec.Path) == 0 { cec.Path = nil } diff --git a/api/consul_test.go b/api/consul_test.go index f44e47a1ca9a..bc714b65d273 100644 --- a/api/consul_test.go +++ b/api/consul_test.go @@ -133,7 +133,7 @@ func TestConsulProxy_Canonicalize(t *testing.T) { cp.Canonicalize() must.Eq(t, "", cp.LocalServiceAddress) must.Zero(t, cp.LocalServicePort) - must.Nil(t, cp.ExposeConfig) + must.Nil(t, cp.Expose) must.Nil(t, cp.Upstreams) must.MapEmpty(t, cp.Config) }) @@ -142,14 +142,14 @@ func TestConsulProxy_Canonicalize(t *testing.T) { cp := &ConsulProxy{ LocalServiceAddress: "127.0.0.1", LocalServicePort: 80, - ExposeConfig: new(ConsulExposeConfig), + Expose: new(ConsulExposeConfig), Upstreams: make([]*ConsulUpstream, 0), Config: make(map[string]interface{}), } cp.Canonicalize() must.Eq(t, "127.0.0.1", cp.LocalServiceAddress) must.Eq(t, 80, cp.LocalServicePort) - must.Eq(t, &ConsulExposeConfig{}, cp.ExposeConfig) + must.Eq(t, &ConsulExposeConfig{}, cp.Expose) must.Nil(t, cp.Upstreams) must.Nil(t, cp.Config) }) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index c23f46a322a0..13ed5575d1e7 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1646,11 +1646,18 @@ func apiConnectSidecarServiceProxyToStructs(in *api.ConsulProxy) *structs.Consul if in == nil { return nil } + + // TODO: to maintain backwards compatibility + expose := in.Expose + if in.ExposeConfig != nil { + expose = in.ExposeConfig + } + return &structs.ConsulProxy{ LocalServiceAddress: in.LocalServiceAddress, LocalServicePort: in.LocalServicePort, Upstreams: apiUpstreamsToStructs(in.Upstreams), - Expose: apiConsulExposeConfigToStructs(in.ExposeConfig), + Expose: apiConsulExposeConfigToStructs(expose), Config: maps.Clone(in.Config), } } @@ -1686,8 +1693,15 @@ func apiConsulExposeConfigToStructs(in *api.ConsulExposeConfig) *structs.ConsulE if in == nil { return nil } + + // TODO: to maintain backwards compatibility + paths := in.Paths + if in.Path != nil { + paths = in.Path + } + return &structs.ConsulExposeConfig{ - Paths: apiConsulExposePathsToStructs(in.Path), + Paths: apiConsulExposePathsToStructs(paths), } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 163fa5147b65..03deb35c74d4 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -3694,7 +3694,7 @@ func TestConversion_apiConsulExposeConfigToStructs(t *testing.T) { require.Equal(t, &structs.ConsulExposeConfig{ Paths: []structs.ConsulExposePath{{Path: "/health"}}, }, apiConsulExposeConfigToStructs(&api.ConsulExposeConfig{ - Path: []*api.ConsulExposePath{{Path: "/health"}}, + Paths: []*api.ConsulExposePath{{Path: "/health"}}, })) } @@ -3747,8 +3747,8 @@ func TestConversion_apiConnectSidecarServiceProxyToStructs(t *testing.T) { Upstreams: []*api.ConsulUpstream{{ DestinationName: "upstream", }}, - ExposeConfig: &api.ConsulExposeConfig{ - Path: []*api.ConsulExposePath{{ + Expose: &api.ConsulExposeConfig{ + Paths: []*api.ConsulExposePath{{ Path: "/health", }}, }, diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go index 7dc10214b38e..23ce1987cb58 100644 --- a/jobspec/parse_service.go +++ b/jobspec/parse_service.go @@ -819,7 +819,7 @@ func parseProxy(o *ast.ObjectItem) (*api.ConsulProxy, error) { if e, err := parseExpose(eo.Items[0]); err != nil { return nil, err } else { - proxy.ExposeConfig = e + proxy.Expose = e } } @@ -870,13 +870,13 @@ func parseExpose(eo *ast.ObjectItem) (*api.ConsulExposeConfig, error) { po := listVal.Filter("path") // array if len(po.Items) > 0 { - expose.Path = make([]*api.ConsulExposePath, len(po.Items)) + expose.Paths = make([]*api.ConsulExposePath, len(po.Items)) for i := range po.Items { p, err := parseExposePath(po.Items[i]) if err != nil { return nil, err } - expose.Path[i] = p + expose.Paths[i] = p } } diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 1e5cd8bb2b3f..f23c1f2eb48e 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -1282,8 +1282,8 @@ func TestParse(t *testing.T) { Connect: &api.ConsulConnect{ SidecarService: &api.ConsulSidecarService{ Proxy: &api.ConsulProxy{ - ExposeConfig: &api.ConsulExposeConfig{ - Path: []*api.ConsulExposePath{{ + Expose: &api.ConsulExposeConfig{ + Paths: []*api.ConsulExposePath{{ Path: "/health", Protocol: "http", LocalPathPort: 2222, @@ -1386,8 +1386,8 @@ func TestParse(t *testing.T) { Proxy: &api.ConsulProxy{ LocalServiceAddress: "10.0.1.2", LocalServicePort: 8080, - ExposeConfig: &api.ConsulExposeConfig{ - Path: []*api.ConsulExposePath{{ + Expose: &api.ConsulExposeConfig{ + Paths: []*api.ConsulExposePath{{ Path: "/metrics", Protocol: "http", LocalPathPort: 9001, diff --git a/nomad/structs/services.go b/nomad/structs/services.go index fa3f99f8d389..aaa0fd68ebe9 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -1356,9 +1356,7 @@ 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. - // - // Use json tag to match with field name in api/ - Expose *ConsulExposeConfig `json:"ExposeConfig"` + Expose *ConsulExposeConfig // Config is a proxy configuration. It is opaque to Nomad and passed // directly to Consul. @@ -1526,8 +1524,7 @@ func upstreamsEquals(a, b []ConsulUpstream) bool { // ConsulExposeConfig represents a Consul Connect expose jobspec stanza. type ConsulExposeConfig struct { - // Use json tag to match with field name in api/ - Paths []ConsulExposePath `json:"Path"` + Paths []ConsulExposePath } type ConsulExposePath struct { From da49a897bc0bec772b2b93f71bb3e85b9798170c Mon Sep 17 00:00:00 2001 From: Jorge Marey Date: Sat, 28 Jan 2023 17:38:21 +0100 Subject: [PATCH 2/4] Add changelog entry --- .changelog/15541.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/15541.txt diff --git a/.changelog/15541.txt b/.changelog/15541.txt new file mode 100644 index 000000000000..3e88677b90cb --- /dev/null +++ b/.changelog/15541.txt @@ -0,0 +1,3 @@ +```release-note:bug +api: Fixed a bug where exposeConfig field was not provided correctly when getting the jobs via the API +``` From 93df55c74449658e6d8304571d150d26a5a9255a Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 30 Jan 2023 08:43:49 -0600 Subject: [PATCH 3/4] changelog: add deprecation notes about connect fields --- .changelog/15541.txt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.changelog/15541.txt b/.changelog/15541.txt index 3e88677b90cb..ed1e9a7c950b 100644 --- a/.changelog/15541.txt +++ b/.changelog/15541.txt @@ -1,3 +1,11 @@ ```release-note:bug api: Fixed a bug where exposeConfig field was not provided correctly when getting the jobs via the API ``` + +```release-note:deprecation +api: The connect `ConsulProxy.ExposeConfig` field is deprecated in favor of `ConsulProxy.Expose` +``` + +```release-note:deprecation +api: The connect `ConsulExposeConfig.Path` field is deprecated in favor of `ConsulExposeConfig.Paths` +``` From 5cf10c2b53fb503cc05803c3b91dfbd866296136 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 30 Jan 2023 08:45:19 -0600 Subject: [PATCH 4/4] api: minor style tweaks --- api/consul.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/api/consul.go b/api/consul.go index f2553010ae6a..b8d6308faf78 100644 --- a/api/consul.go +++ b/api/consul.go @@ -135,13 +135,12 @@ func (st *SidecarTask) Canonicalize() { // ConsulProxy represents a Consul Connect sidecar proxy jobspec stanza. type ConsulProxy struct { - LocalServiceAddress string `mapstructure:"local_service_address" hcl:"local_service_address,optional"` - LocalServicePort int `mapstructure:"local_service_port" hcl:"local_service_port,optional"` - Expose *ConsulExposeConfig `mapstructure:"expose" hcl:"expose,block"` - // Deprecated, only to maintain backwards compatibility. Use Expose instead - ExposeConfig *ConsulExposeConfig - Upstreams []*ConsulUpstream `hcl:"upstreams,block"` - Config map[string]interface{} `hcl:"config,block"` + LocalServiceAddress string `mapstructure:"local_service_address" hcl:"local_service_address,optional"` + LocalServicePort int `mapstructure:"local_service_port" hcl:"local_service_port,optional"` + Expose *ConsulExposeConfig `mapstructure:"expose" hcl:"expose,block"` + ExposeConfig *ConsulExposeConfig // Deprecated: only to maintain backwards compatibility. Use Expose instead. + Upstreams []*ConsulUpstream `hcl:"upstreams,block"` + Config map[string]interface{} `hcl:"config,block"` } func (cp *ConsulProxy) Canonicalize() { @@ -237,8 +236,7 @@ func (cu *ConsulUpstream) Canonicalize() { type ConsulExposeConfig struct { Paths []*ConsulExposePath `mapstructure:"path" hcl:"path,block"` - // Deprecated, only to maintain backwards compatibility. Use Paths instead - Path []*ConsulExposePath + Path []*ConsulExposePath // Deprecated: only to maintain backwards compatibility. Use Paths instead. } func (cec *ConsulExposeConfig) Canonicalize() {