From 7b9f8b5cb15addeda286718546b6db2358910a7b Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Fri, 6 Nov 2020 08:21:38 -0600 Subject: [PATCH] Fix job detail crash when recommendations off (#9269) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this, visiting any job detail page on Nomad OSS would crash with an error like this: Error: Ember Data Request GET /v1/recommendations?job=ping%F0%9F%A5%B3&namespace=default returned a 404 Payload (text/xml) The problem was twofold. 1. The recommendation ability didn’t include anything about checking whether the feature was present. This adds a request to /v1/operator/license on application load to determine which features are present and store them in the system service. The ability now looks for 'Dynamic Application Sizing' in that feature list. 2. Second, I didn’t check permissions at all in the job-fetching or job detail templates. --- ui/app/abilities/abstract.js | 9 ++ ui/app/abilities/recommendation.js | 12 ++- ui/app/routes/application.js | 5 + ui/app/routes/jobs/job.js | 13 ++- ui/app/services/system.js | 22 +++++ .../templates/components/job-page/service.hbs | 8 +- .../templates/components/job-page/system.hbs | 8 +- ui/mirage/config.js | 14 +++ ui/mirage/models/feature.js | 3 + ui/tests/acceptance/job-detail-test.js | 21 +++- ui/tests/acceptance/optimize-test.js | 2 + ui/tests/acceptance/regions-test.js | 3 +- .../unit/abilities/recommendation-test.js | 96 ++++++++++++------- 13 files changed, 167 insertions(+), 49 deletions(-) create mode 100644 ui/mirage/models/feature.js diff --git a/ui/app/abilities/abstract.js b/ui/app/abilities/abstract.js index 3b0cc3fa5..2c0e810c6 100644 --- a/ui/app/abilities/abstract.js +++ b/ui/app/abilities/abstract.js @@ -53,6 +53,15 @@ export default class Abstract extends Ability { }); } + @computed('system.features.[]') + get features() { + return this.system.features; + } + + featureIsPresent(featureName) { + return this.features.includes(featureName); + } + // Chooses the closest namespace as described at the bottom here: // https://learn.hashicorp.com/tutorials/nomad/access-control-policies?in=nomad/access-control#namespace-rules _findMatchingNamespace(policyNamespaces, activeNamespace) { diff --git a/ui/app/abilities/recommendation.js b/ui/app/abilities/recommendation.js index d3cdf6f85..11decdb43 100644 --- a/ui/app/abilities/recommendation.js +++ b/ui/app/abilities/recommendation.js @@ -1,13 +1,21 @@ import AbstractAbility from './abstract'; import { computed } from '@ember/object'; -import { or } from '@ember/object/computed'; +import { and, or } from '@ember/object/computed'; export default class Recommendation extends AbstractAbility { - @or('bypassAuthorization', 'selfTokenIsManagement', 'policiesSupportAcceptingOnAnyNamespace') + @and('dynamicApplicationSizingIsPresent', 'hasPermissions') canAccept; + @or('bypassAuthorization', 'selfTokenIsManagement', 'policiesSupportAcceptingOnAnyNamespace') + hasPermissions; + @computed('capabilitiesForAllNamespaces.[]') get policiesSupportAcceptingOnAnyNamespace() { return this.capabilitiesForAllNamespaces.includes('submit-job'); } + + @computed('features.[]') + get dynamicApplicationSizingIsPresent() { + return this.featureIsPresent('Dynamic Application Sizing'); + } } diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 1e7500924..d89ab9445 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -30,9 +30,14 @@ export default class ApplicationRoute extends Route { .perform() .catch(); + const fetchLicense = this.get('system.fetchLicense') + .perform() + .catch(); + return RSVP.all([ this.get('system.regions'), this.get('system.defaultRegion'), + fetchLicense, fetchSelfTokenAndPolicies, ]).then(promises => { if (!this.get('system.shouldShowRegions')) return promises; diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index 7c3c60f4a..61daf1248 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -7,6 +7,7 @@ import classic from 'ember-classic-decorator'; @classic export default class JobRoute extends Route { + @service can; @service store; @service token; @@ -20,14 +21,20 @@ export default class JobRoute extends Route { const namespace = transition.to.queryParams.namespace || this.get('system.activeNamespace.id'); const name = params.job_name; const fullId = JSON.stringify([name, namespace || 'default']); + return this.store .findRecord('job', fullId, { reload: true }) .then(job => { - return RSVP.all([ + const relatedModelsQueries = [ job.get('allocations'), job.get('evaluations'), - job.get('recommendationSummaries'), - ]).then(() => job); + ]; + + if (this.can.can('accept recommendation')) { + relatedModelsQueries.push(job.get('recommendationSummaries')); + } + + return RSVP.all(relatedModelsQueries).then(() => job); }) .catch(notifyError(this)); } diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 359c85f62..7b32bb12d 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -1,10 +1,12 @@ import Service, { inject as service } from '@ember/service'; import { computed } from '@ember/object'; +import { alias } from '@ember/object/computed'; import PromiseObject from '../utils/classes/promise-object'; import PromiseArray from '../utils/classes/promise-array'; import { namespace } from '../adapters/application'; import jsonWithDefault from '../utils/json-with-default'; import classic from 'ember-classic-decorator'; +import { task } from 'ember-concurrency'; @classic export default class SystemService extends Service { @@ -144,4 +146,24 @@ export default class SystemService extends Service { this.set('activeNamespace', null); this.notifyPropertyChange('namespaces'); } + + @task(function*() { + const emptyLicense = { License: { Features: [] } }; + + try { + return yield this.token + .authorizedRawRequest(`/${namespace}/operator/license`) + .then(jsonWithDefault(emptyLicense)); + } catch (e) { + return emptyLicense; + } + }) + fetchLicense; + + @alias('fetchLicense.lastSuccessful.value') license; + + @computed('license.License.Features.[]') + get features() { + return this.get('license.License.Features') || []; + } } diff --git a/ui/app/templates/components/job-page/service.hbs b/ui/app/templates/components/job-page/service.hbs index cafa412a4..d8180f244 100644 --- a/ui/app/templates/components/job-page/service.hbs +++ b/ui/app/templates/components/job-page/service.hbs @@ -13,9 +13,11 @@ - {{#each this.job.recommendationSummaries as |summary|}} - - {{/each}} + {{#if (can "accept recommendations")}} + {{#each this.job.recommendationSummaries as |summary|}} + + {{/each}} + {{/if}} diff --git a/ui/app/templates/components/job-page/system.hbs b/ui/app/templates/components/job-page/system.hbs index 602d59042..5e0abbea2 100644 --- a/ui/app/templates/components/job-page/system.hbs +++ b/ui/app/templates/components/job-page/system.hbs @@ -13,9 +13,11 @@ - {{#each this.job.recommendationSummaries as |summary|}} - - {{/each}} + {{#if (can "accept recommendations")}} + {{#each this.job.recommendationSummaries as |summary|}} + + {{/each}} + {{/if}} diff --git a/ui/mirage/config.js b/ui/mirage/config.js index 9ad6c07c0..eec2126b4 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -410,6 +410,20 @@ export default function() { return this.serialize(regions.all()); }); + this.get('/operator/license', function({ features }) { + const records = features.all(); + + if (records.length) { + return { + License: { + Features: records.models.mapBy('name'), + } + }; + } + + return new Response(501, {}, null); + }); + const clientAllocationStatsHandler = function({ clientAllocationStats }, { params }) { return this.serialize(clientAllocationStats.find(params.id)); }; diff --git a/ui/mirage/models/feature.js b/ui/mirage/models/feature.js new file mode 100644 index 000000000..ddb04151d --- /dev/null +++ b/ui/mirage/models/feature.js @@ -0,0 +1,3 @@ +import { Model } from 'ember-cli-mirage'; + +export default Model.extend(); diff --git a/ui/tests/acceptance/job-detail-test.js b/ui/tests/acceptance/job-detail-test.js index d2d624ffd..449ffef95 100644 --- a/ui/tests/acceptance/job-detail-test.js +++ b/ui/tests/acceptance/job-detail-test.js @@ -96,7 +96,7 @@ module('Acceptance | job detail (with namespaces)', function(hooks) { setupApplicationTest(hooks); setupMirage(hooks); - let job, clientToken; + let job, managementToken, clientToken; hooks.beforeEach(function() { server.createList('namespace', 2); @@ -112,7 +112,7 @@ module('Acceptance | job detail (with namespaces)', function(hooks) { createRecommendations: true, }); - server.create('token'); + managementToken = server.create('token'); clientToken = server.create('token'); }); @@ -212,6 +212,9 @@ module('Acceptance | job detail (with namespaces)', function(hooks) { }); test('resource recommendations show when they exist and can be expanded, collapsed, and processed', async function(assert) { + server.create('feature', { name: 'Dynamic Application Sizing' }); + + window.localStorage.nomadTokenSecret = managementToken.secretId; await JobDetail.visit({ id: job.id, namespace: server.db.namespaces[1].name }); assert.equal(JobDetail.recommendations.length, job.taskGroups.length); @@ -247,4 +250,18 @@ module('Acceptance | job detail (with namespaces)', function(hooks) { assert.equal(JobDetail.recommendations.length, job.taskGroups.length - 1); }); + + test('resource recommendations are not fetched when the feature doesn’t exist', async function(assert) { + window.localStorage.nomadTokenSecret = managementToken.secretId; + await JobDetail.visit({ id: job.id, namespace: server.db.namespaces[1].name }); + + assert.equal(JobDetail.recommendations.length, 0); + + assert.equal( + server.pretender.handledRequests + .filter(request => request.url.includes('recommendations')) + .length, + 0 + ); + }); }); diff --git a/ui/tests/acceptance/optimize-test.js b/ui/tests/acceptance/optimize-test.js index 2cc0b01ec..096f364e9 100644 --- a/ui/tests/acceptance/optimize-test.js +++ b/ui/tests/acceptance/optimize-test.js @@ -28,6 +28,8 @@ module('Acceptance | optimize', function(hooks) { setupMirage(hooks); hooks.beforeEach(async function() { + server.create('feature', { name: 'Dynamic Application Sizing' }); + server.create('node'); server.createList('namespace', 2); diff --git a/ui/tests/acceptance/regions-test.js b/ui/tests/acceptance/regions-test.js index 36ac55138..59702bc15 100644 --- a/ui/tests/acceptance/regions-test.js +++ b/ui/tests/acceptance/regions-test.js @@ -162,7 +162,8 @@ module('Acceptance | regions (many)', function(hooks) { await PageLayout.gutter.visitClients(); await PageLayout.gutter.visitServers(); const [ - , + , // License request + , // Token/policies request regionsRequest, defaultRegionRequest, ...appRequests diff --git a/ui/tests/unit/abilities/recommendation-test.js b/ui/tests/unit/abilities/recommendation-test.js index a80db7496..3d62fe0d1 100644 --- a/ui/tests/unit/abilities/recommendation-test.js +++ b/ui/tests/unit/abilities/recommendation-test.js @@ -8,48 +8,74 @@ module('Unit | Ability | recommendation', function(hooks) { setupTest(hooks); setupAbility('recommendation')(hooks); - test('it permits accepting recommendations when ACLs are disabled', function(assert) { - const mockToken = Service.extend({ - aclEnabled: false, + module('when the Dynamic Application Sizing feature is present', function(hooks) { + hooks.beforeEach(function() { + const mockSystem = Service.extend({ + features: ['Dynamic Application Sizing'], + }); + + this.owner.register('service:system', mockSystem); }); - this.owner.register('service:token', mockToken); + test('it permits accepting recommendations when ACLs are disabled', function(assert) { + const mockToken = Service.extend({ + aclEnabled: false, + }); - assert.ok(this.ability.canAccept); + this.owner.register('service:token', mockToken); + + assert.ok(this.ability.canAccept); + }); + + test('it permits accepting recommendations for client tokens where any namespace has submit-job capabilities', function(assert) { + this.owner.lookup('service:system').set('activeNamespace', { + name: 'anotherNamespace', + }); + + const mockToken = Service.extend({ + aclEnabled: true, + selfToken: { type: 'client' }, + selfTokenPolicies: [ + { + rulesJSON: { + Namespaces: [ + { + Name: 'aNamespace', + Capabilities: [], + }, + { + Name: 'bNamespace', + Capabilities: ['submit-job'], + }, + ], + }, + }, + ], + }); + + this.owner.register('service:token', mockToken); + + assert.ok(this.ability.canAccept); + }); }); - test('it permits accepting recommendations for client tokens where any namespace has submit-job capabilities', function(assert) { - const mockSystem = Service.extend({ - aclEnabled: true, - activeNamespace: { - name: 'anotherNamespace', - }, + module('when the Dynamic Application Sizing feature is not present', function(hooks) { + hooks.beforeEach(function() { + const mockSystem = Service.extend({ + features: [], + }); + + this.owner.register('service:system', mockSystem); }); - const mockToken = Service.extend({ - aclEnabled: true, - selfToken: { type: 'client' }, - selfTokenPolicies: [ - { - rulesJSON: { - Namespaces: [ - { - Name: 'aNamespace', - Capabilities: [], - }, - { - Name: 'bNamespace', - Capabilities: ['submit-job'], - }, - ], - }, - }, - ], + test('it does not permit accepting recommendations regardless of ACL status', function(assert) { + const mockToken = Service.extend({ + aclEnabled: false, + }); + + this.owner.register('service:token', mockToken); + + assert.notOk(this.ability.canAccept); }); - - this.owner.register('service:system', mockSystem); - this.owner.register('service:token', mockToken); - - assert.ok(this.ability.canAccept); }); });