From a95bd0b10fe8f1610ddf7259503309196e0fba7b Mon Sep 17 00:00:00 2001 From: John Cowen Date: Mon, 4 Jan 2021 17:06:44 +0000 Subject: [PATCH 1/2] ui: Make sure we pass the nspace through to the API for nodes --- ui-v2/app/adapters/node.js | 22 ++++++-- ui-v2/app/routes/dc/nodes/index.js | 2 +- .../tests/acceptance/page-navigation.feature | 4 +- ui-v2/tests/integration/adapters/node-test.js | 50 ++++++++++++------- 4 files changed, 53 insertions(+), 25 deletions(-) diff --git a/ui-v2/app/adapters/node.js b/ui-v2/app/adapters/node.js index f10364a11a60..c0fcc2ee519a 100644 --- a/ui-v2/app/adapters/node.js +++ b/ui-v2/app/adapters/node.js @@ -1,15 +1,26 @@ import Adapter from './application'; // TODO: Update to use this.formatDatacenter() + +// Node and Namespaces are a little strange in that Nodes don't belong in a +// namespace whereas things that belong to a Node do (Health Checks and +// Service Instances). So even though Nodes themselves don't require a +// namespace filter, you sill needs to pass the namespace through to API +// requests in order to get the correct information for the things that belong +// to the node. + export default Adapter.extend({ - requestForQuery: function(request, { dc, index, id, uri }) { + requestForQuery: function(request, { dc, ns, index, id, uri }) { return request` GET /v1/internal/ui/nodes?${{ dc }} X-Request-ID: ${uri} - ${{ index }} + ${{ + ...this.formatNspace(ns), + index, + }} `; }, - requestForQueryRecord: function(request, { dc, index, id, uri }) { + requestForQueryRecord: function(request, { dc, ns, index, id, uri }) { if (typeof id === 'undefined') { throw new Error('You must specify an id'); } @@ -17,7 +28,10 @@ export default Adapter.extend({ GET /v1/internal/ui/node/${id}?${{ dc }} X-Request-ID: ${uri} - ${{ index }} + ${{ + ...this.formatNspace(ns), + index, + }} `; }, requestForQueryLeader: function(request, { dc }) { diff --git a/ui-v2/app/routes/dc/nodes/index.js b/ui-v2/app/routes/dc/nodes/index.js index 783f1efa4fc8..b67cc65e6d3c 100644 --- a/ui-v2/app/routes/dc/nodes/index.js +++ b/ui-v2/app/routes/dc/nodes/index.js @@ -13,7 +13,7 @@ export default Route.extend({ }, model: function(params) { const dc = this.modelFor('dc').dc.Name; - const nspace = '*'; + const nspace = this.modelFor('nspace').nspace.substr(1); return hash({ items: this.data.source(uri => uri`/${nspace}/${dc}/nodes`), leader: this.repo.findByLeader(dc), diff --git a/ui-v2/tests/acceptance/page-navigation.feature b/ui-v2/tests/acceptance/page-navigation.feature index 6331a044edde..8aee1c5f5150 100644 --- a/ui-v2/tests/acceptance/page-navigation.feature +++ b/ui-v2/tests/acceptance/page-navigation.feature @@ -23,7 +23,7 @@ Feature: page-navigation Where: ------------------------------------------------------------------------------------- | Link | URL | Endpoint | - | nodes | /dc-1/nodes | /v1/internal/ui/nodes?dc=dc-1 | + | nodes | /dc-1/nodes | /v1/internal/ui/nodes?dc=dc-1&ns=@namespace | | kvs | /dc-1/kv | /v1/kv/?keys&dc=dc-1&separator=%2F&ns=@namespace | | acls | /dc-1/acls/tokens | /v1/acl/tokens?dc=dc-1&ns=@namespace | | intentions | /dc-1/intentions | /v1/connect/intentions?dc=dc-1 | @@ -58,7 +58,7 @@ Feature: page-navigation --- - /v1/catalog/datacenters - /v1/namespaces - - /v1/internal/ui/node/node-0?dc=dc-1 + - /v1/internal/ui/node/node-0?dc=dc-1&ns=@namespace - /v1/coordinate/nodes?dc=dc-1 - /v1/session/node/node-0?dc=dc-1&ns=@namespace --- diff --git a/ui-v2/tests/integration/adapters/node-test.js b/ui-v2/tests/integration/adapters/node-test.js index 3efca49adb3a..b718e984f86b 100644 --- a/ui-v2/tests/integration/adapters/node-test.js +++ b/ui-v2/tests/integration/adapters/node-test.js @@ -1,28 +1,42 @@ import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; +import { env } from '../../../env'; +const shouldHaveNspace = function(nspace) { + return typeof nspace !== 'undefined' && env('CONSUL_NSPACES_ENABLED'); +}; module('Integration | Adapter | node', function(hooks) { setupTest(hooks); const dc = 'dc-1'; const id = 'node-name'; - test('requestForQuery returns the correct url', function(assert) { - const adapter = this.owner.lookup('adapter:node'); - const client = this.owner.lookup('service:client/http'); - const expected = `GET /v1/internal/ui/nodes?dc=${dc}`; - const actual = adapter.requestForQuery(client.requestParams.bind(client), { - dc: dc, + const undefinedNspace = 'default'; + [undefinedNspace, 'team-1', undefined].forEach(nspace => { + test(`requestForQuery returns the correct url when nspace is ${nspace}`, function(assert) { + const adapter = this.owner.lookup('adapter:node'); + const client = this.owner.lookup('service:client/http'); + const expected = `GET /v1/internal/ui/nodes?dc=${dc}${ + shouldHaveNspace(nspace) ? `&ns=${nspace}` : `` + }`; + const actual = adapter.requestForQuery(client.requestParams.bind(client), { + dc: dc, + ns: nspace, + }); + assert.equal(`${actual.method} ${actual.url}`, expected); }); - assert.equal(`${actual.method} ${actual.url}`, expected); - }); - test('requestForQueryRecord returns the correct url', function(assert) { - const adapter = this.owner.lookup('adapter:node'); - const client = this.owner.lookup('service:client/http'); - const expected = `GET /v1/internal/ui/node/${id}?dc=${dc}`; - const actual = adapter.requestForQueryRecord(client.requestParams.bind(client), { - dc: dc, - id: id, + test(`requestForQueryRecord returns the correct url when the nspace is ${nspace}`, function(assert) { + const adapter = this.owner.lookup('adapter:node'); + const client = this.owner.lookup('service:client/http'); + const expected = `GET /v1/internal/ui/node/${id}?dc=${dc}${ + shouldHaveNspace(nspace) ? `&ns=${nspace}` : `` + }`; + const actual = adapter.requestForQueryRecord(client.requestParams.bind(client), { + dc: dc, + id: id, + ns: nspace, + }); + assert.equal(`${actual.method} ${actual.url}`, expected); }); - assert.equal(`${actual.method} ${actual.url}`, expected); }); + // the following don't require nspacing test("requestForQueryRecord throws if you don't specify an id", function(assert) { const adapter = this.owner.lookup('adapter:node'); const client = this.owner.lookup('service:client/http'); @@ -36,9 +50,9 @@ module('Integration | Adapter | node', function(hooks) { const adapter = this.owner.lookup('adapter:node'); const client = this.owner.lookup('service:client/http'); const expected = `GET /v1/status/leader?dc=${dc}`; - const actual = adapter.requestForQueryLeader(client.url, { + const actual = adapter.requestForQueryLeader(client.requestParams.bind(client), { dc: dc, }); - assert.equal(actual, expected); + assert.equal(`${actual.method} ${actual.url}`, expected); }); }); From 134b5e298f12bcf603a216c26e7abc94659043c3 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Mon, 4 Jan 2021 17:19:08 +0000 Subject: [PATCH 2/2] Add changelog --- .changelog/9488.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/9488.txt diff --git a/.changelog/9488.txt b/.changelog/9488.txt new file mode 100644 index 000000000000..ff73f8c17da9 --- /dev/null +++ b/.changelog/9488.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: ensure namespace is used for node API requests +```