From 031d0fb902a65059f45a9b1c9e471667cac4bfb2 Mon Sep 17 00:00:00 2001 From: kumarkavish Date: Tue, 6 Feb 2024 14:15:25 +0530 Subject: [PATCH 1/6] [NET-7571] - bug fix for SeatGeek: Services function in CT (consul-template) does not update the namespace for each services API call - opts should be defaulted to the opts at query creation. - then they should be merged with the opts present in Fetch. - currently the merge was the other way around. - ENT test support is not there, so no test cases added as of now. --- dependency/catalog_services.go | 14 ++++++++++-- dependency/catalog_services_test.go | 34 ++++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/dependency/catalog_services.go b/dependency/catalog_services.go index af62e2c1c..d8e4be1ea 100644 --- a/dependency/catalog_services.go +++ b/dependency/catalog_services.go @@ -71,11 +71,21 @@ func (d *CatalogServicesQuery) Fetch(clients *ClientSet, opts *QueryOptions) (in default: } - opts = opts.Merge(&QueryOptions{ + // this overrides the query params present in the query + // i.e. overrides the namespace and partition params with ones used the first time + // while creating NewCatalogServicesQuery + + // see bug [https://github.com/hashicorp/consul-template/pull/1842#issuecomment-1915723565] + // it should be other way around. + // default to the query params present while creating NewCatalogServicesQuery + // and then merge with the query params present in the query + defaultOpts := &QueryOptions{ Datacenter: d.dc, ConsulPartition: d.partition, ConsulNamespace: d.namespace, - }) + } + + opts = defaultOpts.Merge(opts) log.Printf("[TRACE] %s: GET %s", d, &url.URL{ Path: "/v1/catalog/services", diff --git a/dependency/catalog_services_test.go b/dependency/catalog_services_test.go index 928601e8c..93780be77 100644 --- a/dependency/catalog_services_test.go +++ b/dependency/catalog_services_test.go @@ -124,13 +124,41 @@ func TestCatalogServicesQuery_Fetch(t *testing.T) { }, }, }, + // no ENT support as of now. + //{ + // "namespace_bar", + // "?ns=bar&partition=default", + // []*CatalogSnippet{ + // { + // Name: "consul", + // Tags: ServiceTags([]string{}), + // }, + // { + // Name: "foobar-sidecar-proxy", + // Tags: ServiceTags([]string{}), + // }, + // { + // Name: "service-meta", + // Tags: ServiceTags([]string{"tag1"}), + // }, + // { + // Name: "service-taggedAddresses", + // Tags: ServiceTags([]string{}), + // }, + // }, + //}, } + var d *CatalogServicesQuery for i, tc := range cases { t.Run(fmt.Sprintf("%d_%s", i, tc.name), func(t *testing.T) { - d, err := NewCatalogServicesQuery(tc.i) - if err != nil { - t.Fatal(err) + + if i == 0 { + dq, err := NewCatalogServicesQuery(tc.i) + if err != nil { + t.Fatal(err) + } + d = dq } act, _, err := d.Fetch(testClients, nil) From 9bd3d8e23c7d564c6a6fde60599aeb20a30e9de6 Mon Sep 17 00:00:00 2001 From: kumarkavish Date: Tue, 6 Feb 2024 14:18:56 +0530 Subject: [PATCH 2/6] [NET-7571] - bug fix for SeatGeek: Services function in CT (consul-template) does not update the namespace for each services API call - opts should be defaulted to the opts at query creation. - then they should be merged with the opts present in Fetch. - currently the merge was the other way around. - ENT test support is not there, so no test cases added as of now. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a0fcaf4c..94a3eac1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ NEW FEATURES: * Add support for listing Consul peers [NET-6966](https://hashicorp.atlassian.net/browse/NET-6966) +BUG FIXES: +* Fetch services query not overriding opts correctly [NET-7571](https://hashicorp.atlassian.net/browse/NET-7571) + ## v0.36.0 (January 3, 2024) IMPROVEMENTS: From b30687a9958e92a1017a11ce879b85fec99c03bc Mon Sep 17 00:00:00 2001 From: kumarkavish Date: Tue, 6 Feb 2024 18:26:51 +0530 Subject: [PATCH 3/6] [NET-7571] - bug fix for SeatGeek: Services function in CT (consul-template) does not update the namespace for each services API call - opts should be defaulted to the opts at query creation. - then they should be merged with the opts present in Fetch. - currently the merge was the other way around. - ENT test support is not there, so no test cases added as of now. --- dependency/catalog_services_test.go | 53 ++++++++++++----------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/dependency/catalog_services_test.go b/dependency/catalog_services_test.go index 93780be77..0fc3ecac9 100644 --- a/dependency/catalog_services_test.go +++ b/dependency/catalog_services_test.go @@ -100,11 +100,14 @@ func TestCatalogServicesQuery_Fetch(t *testing.T) { cases := []struct { name string i string + opts *QueryOptions exp []*CatalogSnippet + err bool }{ { "all", "", + nil, []*CatalogSnippet{ { Name: "consul", @@ -123,49 +126,35 @@ func TestCatalogServicesQuery_Fetch(t *testing.T) { Tags: ServiceTags([]string{}), }, }, + false, + }, + //no ENT support for test cases as of now. + { + "namespace_bar", + "?ns=bar&partition=default", + &QueryOptions{ConsulPartition: "default", ConsulNamespace: "bar"}, + nil, + true, }, - // no ENT support as of now. - //{ - // "namespace_bar", - // "?ns=bar&partition=default", - // []*CatalogSnippet{ - // { - // Name: "consul", - // Tags: ServiceTags([]string{}), - // }, - // { - // Name: "foobar-sidecar-proxy", - // Tags: ServiceTags([]string{}), - // }, - // { - // Name: "service-meta", - // Tags: ServiceTags([]string{"tag1"}), - // }, - // { - // Name: "service-taggedAddresses", - // Tags: ServiceTags([]string{}), - // }, - // }, - //}, } - var d *CatalogServicesQuery for i, tc := range cases { t.Run(fmt.Sprintf("%d_%s", i, tc.name), func(t *testing.T) { - if i == 0 { - dq, err := NewCatalogServicesQuery(tc.i) - if err != nil { - t.Fatal(err) - } - d = dq + d, err := NewCatalogServicesQuery(tc.i) + if err != nil { + t.Fatal(err) } - act, _, err := d.Fetch(testClients, nil) - if err != nil { + act, _, err := d.Fetch(testClients, tc.opts) + if (err != nil) != tc.err { t.Fatal(err) } + if act == nil && tc.err { + return + } + assert.Equal(t, tc.exp, act) }) } From 8855d4a0095f64f1d7812d9780faa690a91cec2f Mon Sep 17 00:00:00 2001 From: kumarkavish Date: Tue, 6 Feb 2024 21:04:40 +0530 Subject: [PATCH 4/6] [NET-7571] - bug fix for SeatGeek: Services function in CT (consul-template) does not update the namespace for each services API call - opts should be defaulted to the opts at query creation. - then they should be merged with the opts present in Fetch. - currently the merge was the other way around. - ENT test support is not there, so no test cases added as of now. --- dependency/catalog_services_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dependency/catalog_services_test.go b/dependency/catalog_services_test.go index 0fc3ecac9..b68689552 100644 --- a/dependency/catalog_services_test.go +++ b/dependency/catalog_services_test.go @@ -129,13 +129,13 @@ func TestCatalogServicesQuery_Fetch(t *testing.T) { false, }, //no ENT support for test cases as of now. - { - "namespace_bar", - "?ns=bar&partition=default", - &QueryOptions{ConsulPartition: "default", ConsulNamespace: "bar"}, - nil, - true, - }, + //{ + // "namespace_bar", + // "?ns=bar&partition=default", + // &QueryOptions{ConsulPartition: "default", ConsulNamespace: "bar"}, + // nil, + // true, + //}, } for i, tc := range cases { From 0ea26c5bf45e64a520b04803c6a6f2d87838329b Mon Sep 17 00:00:00 2001 From: kumarkavish Date: Tue, 6 Feb 2024 21:07:32 +0530 Subject: [PATCH 5/6] [NET-7571] - bug fix for SeatGeek: Services function in CT (consul-template) does not update the namespace for each services API call - opts should be defaulted to the opts at query creation. - then they should be merged with the opts present in Fetch. - currently the merge was the other way around. - ENT test support is not there, so no test cases added as of now. --- dependency/catalog_services.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/dependency/catalog_services.go b/dependency/catalog_services.go index d8e4be1ea..c4376b8c3 100644 --- a/dependency/catalog_services.go +++ b/dependency/catalog_services.go @@ -71,12 +71,8 @@ func (d *CatalogServicesQuery) Fetch(clients *ClientSet, opts *QueryOptions) (in default: } - // this overrides the query params present in the query - // i.e. overrides the namespace and partition params with ones used the first time - // while creating NewCatalogServicesQuery - + // this overrides the query params present in the query with ones present while creating the query // see bug [https://github.com/hashicorp/consul-template/pull/1842#issuecomment-1915723565] - // it should be other way around. // default to the query params present while creating NewCatalogServicesQuery // and then merge with the query params present in the query defaultOpts := &QueryOptions{ From 5b66da61db099fa42612a5e41d50cb724a928229 Mon Sep 17 00:00:00 2001 From: kumarkavish Date: Wed, 7 Feb 2024 18:48:05 +0530 Subject: [PATCH 6/6] [NET-7571] - updating comments --- dependency/catalog_services_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/dependency/catalog_services_test.go b/dependency/catalog_services_test.go index b68689552..d37376569 100644 --- a/dependency/catalog_services_test.go +++ b/dependency/catalog_services_test.go @@ -128,14 +128,6 @@ func TestCatalogServicesQuery_Fetch(t *testing.T) { }, false, }, - //no ENT support for test cases as of now. - //{ - // "namespace_bar", - // "?ns=bar&partition=default", - // &QueryOptions{ConsulPartition: "default", ConsulNamespace: "bar"}, - // nil, - // true, - //}, } for i, tc := range cases {