From af335e7eccece84abd7b0750153bb6ef4948a082 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Tue, 5 Jan 2021 15:54:23 +0000 Subject: [PATCH] ui: Make sure we pass the nspace through to the API for nodes (#9488) Nodes themselves are not namespaced, so we'd originally assumed we did not need to pass through the ns query parameter when listing or viewing nodes. As it turns out the API endpoints we use to list and view nodes (and related things) return things that are namespaced, therefore any API requests for nodes do require a the ns query parameter to be passed through to the request. This PR adds the necessary ns query param to all things Node, apart from the querying for the leader which only returns node related information. --- .changelog/9488.txt | 3 ++ 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 ++++++++++++------- 5 files changed, 56 insertions(+), 25 deletions(-) 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 +``` 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); }); });