Backport #13530 to 1.2.x #13552
Backport #13530 to 1.2.x #13552
Conversation
| nsAllowed := allowableNamespaces == nil || | ||
| allowableNamespaces[eval.Namespace] |
There was a problem hiding this comment.
In 1.3.x this check is done using the paginator.NamespaceFilter, which doesn't exist in 1.2.x.
Instead of modifying args.ShouldBeFiltered I thought it would be better to make the verification here to keep ShouldBeFiltered the same in release/1.2.x and release/1.3.x.
nomad/eval_endpoint.go
Outdated
| iter = memdb.NewFilterIterator(iter, func(raw interface{}) bool { | ||
| if eval := raw.(*structs.Evaluation); eval != nil { | ||
| return args.ShouldBeFiltered(eval) | ||
| // check if user has permission to all namespaces |
There was a problem hiding this comment.
Nitpick: I think this comment is being copied from elsewhere, but "has permission to all namespaces" seems misleading when what we're really getting is the set of namespace we have permission to.
nomad/eval_endpoint.go
Outdated
| allow = func(string) bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
This seems like an example of "the right code looks wrong", because this means we've already passed the ACL check so we don't need to filter below. Maybe leave a comment for it?
| allow = func(string) bool { | |
| return true | |
| } | |
| allow = func(string) bool { | |
| return true // we've already passed the only ACL check | |
| } |
There was a problem hiding this comment.
Yeah this whole pattern has been copied around in a few places; it'd probably be worth refactoring in some way. At the least, this function could be something like,
AcceptedByACLs = func(string) bool {return true}
so you'd instead have clarity in returning like
default:
return AcceptedByACLs
There was a problem hiding this comment.
In rewriting this I ended up doing a larger refactoring to push most of this logic into the acl package.
#13606 is the main refactoring.
#13608 applies the changes to the existing job and alloc list endpoints.
#13530 applies the changes to the evals list endpoint to fix a bug.
I will redo this eval list bug backport once #13606 is merged into releases/1.2.x.
Apply the same verification process as in job, allocs and scaling policy list endpoints to handle the eval list when using an ACL token with limited namespace support but querying using the `*` wildcard namespace.
lgfa29
force-pushed
the
backport-1.2.x-13530
branch
from
a2c6d99 to
4cffad8
Compare
|
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. |
Backport the API portion of #13530 to
release/1.2.x. Wildcard namespace list support for the eval endpoint was added in 1.2.4, so this doesn't have to be backported to 1.1.x.