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

fix ACL bugs in listing allocs across all namespaces #9278

Merged
merged 2 commits into from
Nov 5, 2020

Conversation

cgbaker
Copy link
Contributor

@cgbaker cgbaker commented Nov 5, 2020

resolves #9268

first commit adds a test that multi-namespace queries actually works... as well as a documenting test that ACLs with multi-namespace query didn't work.

second commit fixes the discovered issues, using the multi-namespace pattern employed by /v1/jobs and /v1/scaling/policies

return true
}

return namespaces[alloc.Namespace]
Copy link
Contributor Author

@cgbaker cgbaker Nov 5, 2020

Choose a reason for hiding this comment

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

this filter was backwards... it also didn't account for the case where namespaces == nil, which is what is returned by a short-circuit for management tokens.

the specificity of this method ultimately wasn't very useful.

require.NoError(t, err)
err = state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc})
require.NoError(t, err)
// two namespaces
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this previously didn't test multiple namespaces, so i expanded it while i was here

@@ -29,6 +29,10 @@ func (a *Alloc) List(args *structs.AllocListRequest, reply *structs.AllocListRes
}
defer metrics.MeasureSince([]string{"nomad", "alloc", "list"}, time.Now())

if args.RequestNamespace() == structs.AllNamespacesSentinel {
return a.listAllNamespaces(args, reply)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this early jump is the same pattern employed for /v1/jobs and /v1/scaling/policies:

nomad/nomad/job_endpoint.go

Lines 1301 to 1303 in 3926317

if args.RequestNamespace() == structs.AllNamespacesSentinel {
return j.listAllNamespaces(args, reply)
}

if args.RequestNamespace() == structs.AllNamespacesSentinel {
return p.listAllNamespaces(args, reply)
}

@cgbaker cgbaker requested review from notnoop and schmichael November 5, 2020 17:46
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Great catch. Thanks @DingoEatingFuzz and @cgbaker !

@cgbaker cgbaker merged commit b11d067 into master Nov 5, 2020
@cgbaker cgbaker deleted the b-9268-all-namespace-allocs-acl branch November 5, 2020 20:59
@github-actions
Copy link

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 Dec 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The allocations endpoint with namespace=* does not interact with ACLs as expected
2 participants