Fix job detail crash when recommendations off (#9269)

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.
This commit is contained in:
Buck Doyle 2020-11-06 08:21:38 -06:00 committed by GitHub
parent 36ac4ba840
commit 8b5b2116ec
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 167 additions and 49 deletions

View File

@ -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) {

View File

@ -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');
}
}

View File

@ -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;

View File

@ -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));
}

View File

@ -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') || [];
}
}

View File

@ -13,9 +13,11 @@
</div>
</div>
{{#if (can "accept recommendations")}}
{{#each this.job.recommendationSummaries as |summary|}}
<Das::RecommendationAccordion @summary={{summary}} />
{{/each}}
{{/if}}
<JobPage::Parts::Summary @job={{this.job}} />

View File

@ -13,9 +13,11 @@
</div>
</div>
{{#if (can "accept recommendations")}}
{{#each this.job.recommendationSummaries as |summary|}}
<Das::RecommendationAccordion @summary={{summary}} />
{{/each}}
{{/if}}
<JobPage::Parts::Summary @job={{this.job}} />

View File

@ -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));
};

View File

@ -0,0 +1,3 @@
import { Model } from 'ember-cli-mirage';
export default Model.extend();

View File

@ -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 doesnt 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
);
});
});

View File

@ -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);

View File

@ -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

View File

@ -8,6 +8,15 @@ module('Unit | Ability | recommendation', function(hooks) {
setupTest(hooks);
setupAbility('recommendation')(hooks);
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);
});
test('it permits accepting recommendations when ACLs are disabled', function(assert) {
const mockToken = Service.extend({
aclEnabled: false,
@ -19,11 +28,8 @@ module('Unit | Ability | recommendation', function(hooks) {
});
test('it permits accepting recommendations for client tokens where any namespace has submit-job capabilities', function(assert) {
const mockSystem = Service.extend({
aclEnabled: true,
activeNamespace: {
this.owner.lookup('service:system').set('activeNamespace', {
name: 'anotherNamespace',
},
});
const mockToken = Service.extend({
@ -47,9 +53,29 @@ module('Unit | Ability | recommendation', function(hooks) {
],
});
this.owner.register('service:system', mockSystem);
this.owner.register('service:token', mockToken);
assert.ok(this.ability.canAccept);
});
});
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);
});
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);
});
});
});