endpoint to expose all jobs across all namespaces #8001
Conversation
Allow a `/v1/jobs?all_namespaces=true` to list all jobs across all namespaces. The returned list is to contain a `Namespace` field indicating the job namespace. If ACL is enabled, the request token needs to be a management token or have `namespace:list-jobs` capability on all existing namespaces.
nomad/job_endpoint.go
Outdated
|
|
||
| for _, ns := range nses { | ||
| if !aclObj.AllowNsOp(ns, acl.NamespaceCapabilityListJobs) { | ||
| return structs.ErrPermissionDenied |
There was a problem hiding this comment.
since this is already linear-time/memory on the number of namespaces + the number of jobs, why not use the token as a filter?
There was a problem hiding this comment.
Not sure I follow the intent - is the suggestion to keep the filtering logic but call aclObj.AllowNsOps(...) on every job rather than every namespaces? That's seems unnecessary wasteful. I'd assume there are orders of magnitudes more jobs than namespaces.
Or is the suggestion to move it to job level but then only show the jobs that the acl is allowed to view. This is reasonable, but I'd be confused if I pass all_namespaces=true but got only a subset.
There was a problem hiding this comment.
The latter.
but I'd be confused if I pass all_namespaces=true but got only a subset.
That's exactly the behavior of GET /namespaces; the list of namespaces is filtered by ACL. I would not be confused; it's what I would expect. Regardless, that's what documentation is for. 😄
There was a problem hiding this comment.
Good point. I'll update it.
There was a problem hiding this comment.
I'll go further and argue that filtering is more consistent. It means that GET /jobs?all_namespaces=true is equivalent to GET /jobs | filter(_.Namespace in GET /namespaces).
Without a filtering approach, users have the situation where GET /namespaces returns a list of namespaces and GET /job?all_namespaces=true returns 403, so that they're required to loop over GET /jobs for all namespaces returns by GET /namespaces; this seems to me the exact situation that this modification is intended to avoid.
There was a problem hiding this comment.
(answered at the same time, didn't mean to pile on)
website/pages/api-docs/jobs.mdx
Outdated
| @@ -30,6 +30,12 @@ The table below shows this endpoint's support for | |||
| - `prefix` `(string: "")` - Specifies a string to filter jobs on based on | |||
| an index prefix. This is specified as a query string parameter. | |||
|
|
|||
| - `all_namespaces` `(bool: false)` - Specifies whether to return the all | |||
| known jobs across all namespaces. When set to `true`, the list stubs will | |||
There was a problem hiding this comment.
GitHub doesn't seem to let me make comments/recommendations on unchanged code :/, but I'm not keen on the idea that all_namespaces controls whether ListStub.Namespace is populated. this means, e.g., that code consuming the list of stubs has to know whether or not it was generated with all_namespaces == true. why not modify Job.Stub() to always set JobListStub.Namespace?
There was a problem hiding this comment.
I considered adding namespace on the stub - but worried about the increased size and we've been resisting adding more. Also noticed that we already noticed Job.Summary.Namespaces is already always populated.
In one sense, it's already a redundant field but I added it anyway because I found Job.Summary.Namespace a bit odd considering namespace is primarily a field on job rather than the summary.
There was a problem hiding this comment.
cool. i didn't notice that namespace was already in JobSummary.
There was a problem hiding this comment.
FWIW - I was mostly explaining my rationale but I'm still open to adding Namespace field indiscriminately mostly for purity/beauty prospective if someone advocates for it :). I just don't know how resistant we are to adding more fields here.
There was a problem hiding this comment.
no, i agree. i think the presence in JobSummary (presumably, to support GET /job/:id:summary) perhaps argues against putting it in the list stub at all.
i'd be curious to see what other folks think.
| @@ -4105,6 +4106,7 @@ type JobListStub struct { | |||
| ID string | |||
| ParentID string | |||
| Name string | |||
| Namespace string `json:",omitempty"` | |||
There was a problem hiding this comment.
https://github.com/hashicorp/nomad/blob/master/api/jobs.go#L866
api.JobListStub should be updated as well, for api users.
|
note, we'll be following this pattern to implement the same capability for |
|
Thank you! |
|
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. |
Allow a
/v1/jobs?all_namespaces=trueparameter to list all jobs across allnamespaces. The returned list is to contain a
Namespacefieldindicating the job namespace.
If ACL is enabled, the request token needs to be a management token or
have
namespace:list-jobscapability on all existing namespaces.