Skip to content

Commit

Permalink
refactoring for same regression in job versions
Browse files Browse the repository at this point in the history
In job versions, if you have an ACL token with a write policy
you should be able to revert a job, however, that was not the
case here. This is because we're using ember-can to check if
the user can run a job. That permission relies on policiesSupportRunning
which uses a function called namespaceIncludesCapability. We're going to
need to refactor any cases that use this function.
  • Loading branch information
ChaiWithJai committed Jul 19, 2021
1 parent d92261e commit f98fd2f
Show file tree
Hide file tree
Showing 13 changed files with 280 additions and 24 deletions.
9 changes: 7 additions & 2 deletions ui/app/components/task-group-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@ export default class TaskGroupRow extends Component {
@oneWay('taskGroup.count') count;
@alias('taskGroup.job.runningDeployment') runningDeployment;

@computed('runningDeployment')
get namespace() {
return this.get('taskGroup.job.namespace.name');
}

@computed('runningDeployment', 'namespace')
get tooltipText() {
if (this.can.cannot('scale job')) return "You aren't allowed to scale task groups";
if (this.can.cannot('scale job', null, { namespace: this.namespace }))
return "You aren't allowed to scale task groups";
if (this.runningDeployment) return 'You cannot scale task groups during a deployment';
return undefined;
}
Expand Down
13 changes: 7 additions & 6 deletions ui/app/controllers/jobs/job/task-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import classic from 'ember-classic-decorator';

@classic
export default class TaskGroupController extends Controller.extend(
Sortable,
Searchable,
WithNamespaceResetting
) {
Sortable,
Searchable,
WithNamespaceResetting
) {
@service userSettings;
@service can;

Expand Down Expand Up @@ -66,9 +66,10 @@ export default class TaskGroupController extends Controller.extend(
})
shouldShowScaleEventTimeline;

@computed('model.job.runningDeployment')
@computed('model.job.{namespace,runningDeployment}')
get tooltipText() {
if (this.can.cannot('scale job')) return "You aren't allowed to scale task groups";
if (this.can.cannot('scale job', null, { namespace: this.model.job.namespace.get('name') }))
return "You aren't allowed to scale task groups";
if (this.model.job.runningDeployment) return 'You cannot scale task groups during a deployment';
return undefined;
}
Expand Down
4 changes: 2 additions & 2 deletions ui/app/routes/jobs/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ export default class RunRoute extends Route {
},
];

beforeModel() {
if (this.can.cannot('run job')) {
beforeModel(transition) {
if (this.can.cannot('run job', null, { namespace: transition.to.queryParams.namespace })) {
this.transitionTo('jobs');
}
}
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/job-version.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
</span>
<div class="pull-right">
{{#unless this.isCurrent}}
{{#if (can "run job")}}
{{#if (can "run job" namespace=this.version.job.namespace)}}
<TwoStepButton
data-test-revert-to
@classes={{hash
Expand Down
7 changes: 4 additions & 3 deletions ui/app/templates/components/task-group-row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,25 @@
{{#if this.taskGroup.scaling}}
<div
data-test-scale-controls
class="button-bar is-shadowless is-text bumper-left {{if (or this.runningDeployment (cannot "scale job")) "tooltip multiline"}}"
class="button-bar is-shadowless is-text bumper-left {{if (or this.runningDeployment (cannot "scale job" namespace=this.taskGroup.job.namespace.name)) "tooltip multiline"}}"
aria-label={{this.tooltipText}}>
<button
data-test-scale="decrement"
role="button"
aria-label="decrement"
class="button is-xsmall is-light"
disabled={{or this.isMinimum this.runningDeployment (cannot "scale job")}}
disabled={{or this.isMinimum this.runningDeployment (cannot "scale job" namespace=this.taskGroup.job.namespace.name)}}
onclick={{action "countDown"}}
type="button">
{{x-icon "minus-plain" class="is-text"}}
</button>
<button
data-test-scale-controls-increment
data-test-scale="increment"
role="button"
aria-label="increment"
class="button is-xsmall is-light"
disabled={{or this.isMaximum this.runningDeployment (cannot "scale job")}}
disabled={{or this.isMaximum this.runningDeployment (cannot "scale job" namespace=this.taskGroup.job.namespace.name)}}
onclick={{action "countUp"}}
type="button">
{{x-icon "plus-plain" class="is-text"}}
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/jobs/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
{{#if (media "isMobile")}}
<div class="toolbar-item is-right-aligned">
{{#if (can "run job" namespace=this.qpNamespace)}}
<LinkTo @route="jobs.run" data-test-run-job class="button is-primary">Run Job</LinkTo>
<LinkTo @route="jobs.run" @query={{hash namespace=this.qpNamespace}} data-test-run-job class="button is-primary">Run Job</LinkTo>
{{else}}
<button
data-test-run-job
Expand Down Expand Up @@ -67,7 +67,7 @@
{{#if (not (media "isMobile"))}}
<div class="toolbar-item is-right-aligned">
{{#if (can "run job" namespace=this.qpNamespace)}}
<LinkTo @route="jobs.run" data-test-run-job class="button is-primary">Run Job</LinkTo>
<LinkTo @route="jobs.run" @query={{hash namespace=this.qpNamespace}} data-test-run-job class="button is-primary">Run Job</LinkTo>
{{else}}
<button
data-test-run-job
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/jobs/job/task-group.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
@max={{this.model.scaling.max}}
@value={{this.model.count}}
@class="is-primary is-small"
@disabled={{or this.model.job.runningDeployment (cannot "scale job")}}
@disabled={{or this.model.job.runningDeployment (cannot "scale job" namespace=this.model.job.namespace.name)}}
@onChange={{action "scaleTaskGroup"}}>
Count
</StepperInput>
Expand Down
68 changes: 68 additions & 0 deletions ui/tests/acceptance/job-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,4 +253,72 @@ module('Acceptance | job detail (with namespaces)', function(hooks) {
0
);
});

test('when the dynamic autoscaler is applied, you can scale a task within the job detail page', async function(assert) {
const SCALE_AND_WRITE_NAMESPACE = 'scale-and-write-namespace';
const READ_ONLY_NAMESPACE = 'read-only-namespace';
const clientToken = server.create('token');

const namespace = server.create('namespace', { id: SCALE_AND_WRITE_NAMESPACE });
const secondNamespace = server.create('namespace', { id: READ_ONLY_NAMESPACE });

job = server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: SCALE_AND_WRITE_NAMESPACE,
});

const job2 = server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: READ_ONLY_NAMESPACE,
});
const scalingGroup2 = server.create('task-group', {
job: job2,
name: 'scaling',
count: 1,
shallow: true,
withScaling: true,
});
job2.update({ taskGroupIds: [scalingGroup2.id] });

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: SCALE_AND_WRITE_NAMESPACE,
Capabilities: ['scale-job', 'submit-job', 'read-job', 'list-jobs'],
},
{
Name: READ_ONLY_NAMESPACE,
Capabilities: ['list-jobs', 'read-job'],
},
],
},
});
const scalingGroup = server.create('task-group', {
job,
name: 'scaling',
count: 1,
shallow: true,
withScaling: true,
});
job.update({ taskGroupIds: [scalingGroup.id] });

clientToken.policyIds = [policy.id];
clientToken.save();
window.localStorage.nomadTokenSecret = clientToken.secretId;

await JobDetail.visit({ id: job.id, namespace: namespace.name });
assert.notOk(JobDetail.incrementButton.isDisabled);

await JobDetail.visit({ id: job2.id, namespace: secondNamespace.name });
assert.ok(JobDetail.incrementButton.isDisabled);
});
});
34 changes: 34 additions & 0 deletions ui/tests/acceptance/job-run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,38 @@ module('Acceptance | job run', function(hooks) {
await JobRun.visit();
assert.equal(currentURL(), '/jobs');
});

test('when using client token user can still go to job page if they have correct permissions', async function(assert) {
const clientTokenWithPolicy = server.create('token');
const newNamespace = 'second-namespace';

server.create('namespace', { id: newNamespace });
server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: newNamespace,
});

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: newNamespace,
Capabilities: ['scale-job', 'submit-job', 'read-job', 'list-jobs'],
},
],
},
});

clientTokenWithPolicy.policyIds = [policy.id];
clientTokenWithPolicy.save();
window.localStorage.nomadTokenSecret = clientTokenWithPolicy.secretId;

await JobRun.visit({ namespace: newNamespace });
assert.equal(currentURL(), `/jobs/run?namespace=${newNamespace}`);
});
});
69 changes: 63 additions & 6 deletions ui/tests/acceptance/job-versions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ module('Acceptance | job versions', function(hooks) {
test('all versions but the current one have a button to revert to that version', async function(assert) {
let versionRowToRevertTo;

Versions.versions.forEach((versionRow) => {
Versions.versions.forEach(versionRow => {
if (versionRow.number === job.version) {
assert.ok(versionRow.revertToButton.isHidden);
} else {
Expand All @@ -67,7 +67,9 @@ module('Acceptance | job versions', function(hooks) {
await versionRowToRevertTo.revertToButton.idle();
await versionRowToRevertTo.revertToButton.confirm();

const revertRequest = this.server.pretender.handledRequests.find(request => request.url.includes('revert'));
const revertRequest = this.server.pretender.handledRequests.find(request =>
request.url.includes('revert')
);

assert.equal(revertRequest.url, `/v1/job/${job.id}/revert?namespace=${namespace.id}`);

Expand All @@ -81,7 +83,9 @@ module('Acceptance | job versions', function(hooks) {
});

test('when reversion fails, the error message from the API is piped through to the alert', async function(assert) {
const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
const versionRowToRevertTo = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowToRevertTo) {
const message = 'A plaintext error message';
Expand All @@ -104,7 +108,9 @@ module('Acceptance | job versions', function(hooks) {
});

test('when reversion has no effect, the error message explains', async function(assert) {
const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
const versionRowToRevertTo = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowToRevertTo) {
// The default Mirage implementation updates the job version as passed in, this does nothing
Expand All @@ -116,7 +122,10 @@ module('Acceptance | job versions', function(hooks) {
assert.ok(Layout.inlineError.isShown);
assert.ok(Layout.inlineError.isWarning);
assert.ok(Layout.inlineError.title.includes('Reversion Had No Effect'));
assert.equal(Layout.inlineError.message, 'Reverting to an identical older version doesn’t produce a new version');
assert.equal(
Layout.inlineError.message,
'Reverting to an identical older version doesn’t produce a new version'
);
} else {
assert.expect(0);
}
Expand Down Expand Up @@ -154,7 +163,9 @@ module('Acceptance | job versions (with client token)', function(hooks) {
});

test('reversion buttons are disabled when the token lacks permissions', async function(assert) {
const versionRowWithReversion = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
const versionRowWithReversion = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowWithReversion) {
assert.ok(versionRowWithReversion.revertToButtonIsDisabled);
Expand All @@ -164,4 +175,50 @@ module('Acceptance | job versions (with client token)', function(hooks) {

window.localStorage.clear();
});

test('reversion buttons are available when the client token has permissions', async function(assert) {
const REVERT_NAMESPACE = 'revert-namespace';
window.localStorage.clear();
const clientToken = server.create('token');

server.create('namespace', { id: REVERT_NAMESPACE });

const job = server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: REVERT_NAMESPACE,
});

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: REVERT_NAMESPACE,
Capabilities: ['submit-job'],
},
],
},
});

clientToken.policyIds = [policy.id];
clientToken.save();

window.localStorage.nomadTokenSecret = clientToken.secretId;

versions = server.db.jobVersions.where({ jobId: job.id });
await Versions.visit({ id: job.id, namespace: REVERT_NAMESPACE });
const versionRowWithReversion = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowWithReversion) {
assert.ok(versionRowWithReversion.revertToButtonIsDisabled);
} else {
assert.expect(0);
}
});
});
Loading

0 comments on commit f98fd2f

Please sign in to comment.