From 8c0473a62267c57ab608631b220eb576b73867a8 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Mon, 4 Jan 2021 16:42:44 +0000 Subject: [PATCH] ui: [BUGFIX] Ensure namespace is used for node API requests (#9410) 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. Additionally here we decided to show 0 Services text in the node listing if there are nodes with no service instances within the namespace you are viewing, as this is clearer than showing nothing at all. We also cleaned up/standardized the text we use to in the empty state for service instances. --- .changelog/9410.txt | 3 ++ ui/packages/consul-ui/app/adapters/node.js | 21 +++++++-- .../app/components/consul/node/list/index.hbs | 4 +- .../consul-ui/app/routes/dc/nodes/index.js | 2 +- .../app/templates/dc/nodes/show/services.hbs | 4 +- .../tests/acceptance/page-navigation.feature | 13 +++--- .../tests/integration/adapters/node-test.js | 46 ++++++++++++------- 7 files changed, 60 insertions(+), 33 deletions(-) create mode 100644 .changelog/9410.txt diff --git a/.changelog/9410.txt b/.changelog/9410.txt new file mode 100644 index 0000000000..ff73f8c17d --- /dev/null +++ b/.changelog/9410.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: ensure namespace is used for node API requests +``` diff --git a/ui/packages/consul-ui/app/adapters/node.js b/ui/packages/consul-ui/app/adapters/node.js index a4e3672c5b..33f039938e 100644 --- a/ui/packages/consul-ui/app/adapters/node.js +++ b/ui/packages/consul-ui/app/adapters/node.js @@ -2,17 +2,27 @@ 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 class NodeAdapter extends Adapter { - requestForQuery(request, { dc, index, id, uri }) { + requestForQuery(request, { dc, ns, index, id, uri }) { return request` GET /v1/internal/ui/nodes?${{ dc }} X-Request-ID: ${uri} - ${{ index }} + ${{ + ...this.formatNspace(ns), + index, + }} `; } - requestForQueryRecord(request, { dc, index, id, uri }) { + requestForQueryRecord(request, { dc, ns, index, id, uri }) { if (typeof id === 'undefined') { throw new Error('You must specify an id'); } @@ -20,7 +30,10 @@ export default class NodeAdapter extends Adapter { GET /v1/internal/ui/node/${id}?${{ dc }} X-Request-ID: ${uri} - ${{ index }} + ${{ + ...this.formatNspace(ns), + index, + }} `; } diff --git a/ui/packages/consul-ui/app/components/consul/node/list/index.hbs b/ui/packages/consul-ui/app/components/consul/node/list/index.hbs index a7a9f871b1..c568346d8e 100644 --- a/ui/packages/consul-ui/app/components/consul/node/list/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/node/list/index.hbs @@ -29,11 +29,9 @@ as |item index|> {{#if (eq item.Address @leader.Address)}} Leader {{/if}} - {{#if (gt item.Services.length 0)}} - {{item.Services.length}} {{pluralize item.Services.length 'Service' without-count=true}} + {{format-number item.Services.length}} {{pluralize item.Services.length 'Service' without-count=true}} - {{/if}}
Address diff --git a/ui/packages/consul-ui/app/routes/dc/nodes/index.js b/ui/packages/consul-ui/app/routes/dc/nodes/index.js index 932ca87d80..fea305f737 100644 --- a/ui/packages/consul-ui/app/routes/dc/nodes/index.js +++ b/ui/packages/consul-ui/app/routes/dc/nodes/index.js @@ -20,7 +20,7 @@ export default class IndexRoute extends Route { model(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.data.source(uri => uri`/${nspace}/${dc}/leader`), diff --git a/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs b/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs index 52a8f3933a..3f2c003f81 100644 --- a/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs +++ b/ui/packages/consul-ui/app/templates/dc/nodes/show/services.hbs @@ -35,7 +35,7 @@ @sort={{sort}} @filters={{filters}} @search={{search}} - @items={{reject-by 'Service.Kind' 'connect-proxy' items}} + @items={{items}} as |collection|>

- There are no services. + This node has no service instances{{#if (gt items.length 0)}} matching that search{{/if}}.

diff --git a/ui/packages/consul-ui/tests/acceptance/page-navigation.feature b/ui/packages/consul-ui/tests/acceptance/page-navigation.feature index 614d841a1b..eef74e1da2 100644 --- a/ui/packages/consul-ui/tests/acceptance/page-navigation.feature +++ b/ui/packages/consul-ui/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 | # | settings | /settings | /v1/catalog/datacenters | @@ -39,11 +39,10 @@ Feature: page-navigation And I click "[data-test-back]" Then the url should be [Back] Where: - ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- - | Item | Model | URL | Endpoint | Back | - | kv | kvs | /dc-1/kv/0-key-value/edit | /v1/session/info/ee52203d-989f-4f7a-ab5a-2bef004164ca?dc=dc-1&ns=@namespace | /dc-1/kv | - # | acl | acls | /dc-1/acls/anonymous | /v1/acl/info/anonymous?dc=dc-1 | /dc-1/acls | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + ------------------------------------------------------------------------------------------------------------------------------------- + | Item | Model | URL | Endpoint | Back | + | kv | kvs | /dc-1/kv/0-key-value/edit | /v1/session/info/ee52203d-989f-4f7a-ab5a-2bef004164ca?dc=dc-1&ns=@namespace | /dc-1/kv | + ------------------------------------------------------------------------------------------------------------------------------------- Scenario: The node detail page calls the correct API endpoints When I visit the node page for yaml --- @@ -55,7 +54,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 --- Scenario: The kv detail page calls the correct API endpoints diff --git a/ui/packages/consul-ui/tests/integration/adapters/node-test.js b/ui/packages/consul-ui/tests/integration/adapters/node-test.js index bc9bb45f47..b718e984f8 100644 --- a/ui/packages/consul-ui/tests/integration/adapters/node-test.js +++ b/ui/packages/consul-ui/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');