Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix, ui] Allow running jobs from a namespace-limited token #13659

Merged
merged 10 commits into from
Jul 11, 2022

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Jul 8, 2022

Currently, there are a few things preventing a user from running a new job via the UI, in an environment where their ACL token has limited namespace abilities:

  1. We had previously tied the "Run job" button to an ability that depended on the current namespace. However, this means that your ability to run a job is dependent on filtering your jobs index table, which is pretty confusing to an end-user. Worse, if you don't have any jobs currently running, you don't get any option to set this namespace filter at all.
    • The abilities/job can run ability now looks at all abilities across all namespaces within your policy. This means there's a chance you submit a job for which you don't have write permissions, but good news: it gets caught at several steps along the way upon submission. It's better to not restrict access to this editor and this change reflects that.
  2. If you did somehow manage to make it to the /run page, ever since 1.2.6, Nomad job parsing requires an ACL token but namespaces have never been sent along with that request. Thus, if you had * { read } and myNamespace { write }, it would try to parse your job without a namespace and return a 403.
    • This is a pretty circular issue: the thing that let us determine the namespace of a job was the /parse request. But now that that parse request is conditional upon the namespace... you get where this is going.
    • So, we could try to move the parse() functionality into the browser, which comes with a lot of its own chance for error (Job HCL parsing within the Nomad UI doesn't sound like something we should be doing). Instead, this PR opts to include a Namespaces dropdown on the job/run UI. It uses this when passing the job to /parse.
      ^--- great news update: Turns out we don't need to parse the namespace, we just need a namespace. All other things being equal, a POST to /parse will 403 but /parse?namespace=* will 20x.

Side-effect: Includes better error messaging for ACL permission errors upon job submission.

image

@philrenaud philrenaud self-assigned this Jul 8, 2022
@philrenaud philrenaud marked this pull request as ready for review July 8, 2022 20:44
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

Ember Asset Size action

As of fce54f9

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +750 B +96 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

return availableNamespaces;
}

setFacetQueryParam(queryParam, selection) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive the naming conventions (qpNamespace, queryParam, etc.) - will revise. Was pullig from jobs/volumes routes.

@github-actions
Copy link

github-actions bot commented Jul 9, 2022

Ember Test Audit comparison

main fce54f9 change
passes 1287 1288 +1
failures 2 2 0
flaky 0 0 0
duration 000ms 000ms -000ms

@philrenaud philrenaud added this to the 1.3.2 milestone Jul 11, 2022
@philrenaud philrenaud added the backport/1.3.x backport to 1.3.x release line label Jul 11, 2022
@philrenaud philrenaud merged commit 6d3e807 into main Jul 11, 2022
@philrenaud philrenaud deleted the job-parse-with-namespaces-enabled branch July 11, 2022 16:33
@lgfa29 lgfa29 added backport/1.1.x backport to 1.1.x release line backport/1.2.x backport to 1.1.x release line labels Jul 11, 2022
lgfa29 pushed a commit that referenced this pull request Jul 11, 2022
* Allow running jobs from a namespace-limited token

* qpNamespace cleanup

* Looks like parse can deal with a * namespace

* A little diff cleanup

* Defensive destructuring

* Removing accidental friendly-fire on can-scale

* Testfix: Job run buttons from jobs index

* Testfix: activeRegion job adapter string

* Testfix: unit tests for job abilities correctly reflect the any-namespace rule

* Testfix: job editor test looks for requests with namespace applied on plan
lgfa29 pushed a commit that referenced this pull request Jul 11, 2022
* Allow running jobs from a namespace-limited token

* qpNamespace cleanup

* Looks like parse can deal with a * namespace

* A little diff cleanup

* Defensive destructuring

* Removing accidental friendly-fire on can-scale

* Testfix: Job run buttons from jobs index

* Testfix: activeRegion job adapter string

* Testfix: unit tests for job abilities correctly reflect the any-namespace rule

* Testfix: job editor test looks for requests with namespace applied on plan
lgfa29 pushed a commit that referenced this pull request Jul 11, 2022
… (#13687)

* Allow running jobs from a namespace-limited token

* qpNamespace cleanup

* Looks like parse can deal with a * namespace

* A little diff cleanup

* Defensive destructuring

* Removing accidental friendly-fire on can-scale

* Testfix: Job run buttons from jobs index

* Testfix: activeRegion job adapter string

* Testfix: unit tests for job abilities correctly reflect the any-namespace rule

* Testfix: job editor test looks for requests with namespace applied on plan

Co-authored-by: Phil Renaud <[email protected]>
lgfa29 added a commit that referenced this pull request Jul 11, 2022
lgfa29 added a commit that referenced this pull request Jul 11, 2022
lgfa29 pushed a commit that referenced this pull request Jul 13, 2022
… (#13688)

* Allow running jobs from a namespace-limited token

* qpNamespace cleanup

* Looks like parse can deal with a * namespace

* A little diff cleanup

* Defensive destructuring

* Removing accidental friendly-fire on can-scale

* Testfix: Job run buttons from jobs index

* Testfix: activeRegion job adapter string

* Testfix: unit tests for job abilities correctly reflect the any-namespace rule

* Testfix: job editor test looks for requests with namespace applied on plan

Co-authored-by: Phil Renaud <[email protected]>
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.1.x backport to 1.1.x release line backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants