From e9219a1ae0f3897a8eb1d0ced76b0ccf4db60cae Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 11 Jul 2022 16:42:17 -0400 Subject: [PATCH] Allow wildcard for Evaluations API (#13530) * Failing test and TODO for wildcard * Alias the namespace query parameter for Evals * eval: fix list when using ACLs and * namespace 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. * changelog: add entry for #13530 * ui: set namespace when querying eval Evals have a unique UUID as ID, but when querying them the Nomad API still expects a namespace query param, otherwise it assumes `default`. Co-authored-by: Luiz Aoqui --- .changelog/13530.txt | 7 ++ nomad/eval_endpoint.go | 112 +++++++++++++--------- nomad/eval_endpoint_test.go | 122 +++++++++++++++++------- nomad/structs/structs.go | 8 ++ ui/app/adapters/evaluation.js | 6 +- ui/app/controllers/evaluations/index.js | 2 +- ui/tests/acceptance/evaluations-test.js | 9 +- 7 files changed, 178 insertions(+), 88 deletions(-) create mode 100644 .changelog/13530.txt diff --git a/.changelog/13530.txt b/.changelog/13530.txt new file mode 100644 index 000000000..d62572144 --- /dev/null +++ b/.changelog/13530.txt @@ -0,0 +1,7 @@ +```release-note:bug +api: Fix listing evaluations with the wildcard namespace and an ACL token +``` + +```release-note:bug +ui: Fix a bug that prevented viewing the details of an evaluation in a non-default namespace +``` diff --git a/nomad/eval_endpoint.go b/nomad/eval_endpoint.go index 111ccf81b..575f7edd9 100644 --- a/nomad/eval_endpoint.go +++ b/nomad/eval_endpoint.go @@ -571,11 +571,14 @@ func (e *Eval) List(args *structs.EvalListRequest, reply *structs.EvalListRespon namespace := args.RequestNamespace() // Check for read-job permissions - if aclObj, err := e.srv.ResolveToken(args.AuthToken); err != nil { + aclObj, err := e.srv.ResolveToken(args.AuthToken) + if err != nil { return err - } else if aclObj != nil && !aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob) { + } + if !aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob) { return structs.ErrPermissionDenied } + allow := aclObj.AllowNsOpFunc(acl.NamespaceCapabilityReadJob) if args.Filter != "" { // Check for incompatible filtering. @@ -596,57 +599,72 @@ func (e *Eval) List(args *structs.EvalListRequest, reply *structs.EvalListRespon var iter memdb.ResultIterator var opts paginator.StructsTokenizerOptions - if prefix := args.QueryOptions.Prefix; prefix != "" { - iter, err = store.EvalsByIDPrefix(ws, namespace, prefix, sort) - opts = paginator.StructsTokenizerOptions{ - WithID: true, - } - } else if namespace != structs.AllNamespacesSentinel { - iter, err = store.EvalsByNamespaceOrdered(ws, namespace, sort) - opts = paginator.StructsTokenizerOptions{ - WithCreateIndex: true, - WithID: true, - } - } else { - iter, err = store.Evals(ws, sort) - opts = paginator.StructsTokenizerOptions{ - WithCreateIndex: true, - WithID: true, - } - } - if err != nil { + // Get the namespaces the user is allowed to access. + allowableNamespaces, err := allowedNSes(aclObj, store, allow) + if err == structs.ErrPermissionDenied { + // return empty evals if token isn't authorized for any + // namespace, matching other endpoints + reply.Evaluations = make([]*structs.Evaluation, 0) + } else if err != nil { return err - } - - iter = memdb.NewFilterIterator(iter, func(raw interface{}) bool { - if eval := raw.(*structs.Evaluation); eval != nil { - return args.ShouldBeFiltered(eval) + } else { + if prefix := args.QueryOptions.Prefix; prefix != "" { + iter, err = store.EvalsByIDPrefix(ws, namespace, prefix, sort) + opts = paginator.StructsTokenizerOptions{ + WithID: true, + } + } else if namespace != structs.AllNamespacesSentinel { + iter, err = store.EvalsByNamespaceOrdered(ws, namespace, sort) + opts = paginator.StructsTokenizerOptions{ + WithCreateIndex: true, + WithID: true, + } + } else { + iter, err = store.Evals(ws, sort) + opts = paginator.StructsTokenizerOptions{ + WithCreateIndex: true, + WithID: true, + } + } + if err != nil { + return err } - return false - }) - tokenizer := paginator.NewStructsTokenizer(iter, opts) - - var evals []*structs.Evaluation - paginator, err := paginator.NewPaginator(iter, tokenizer, nil, args.QueryOptions, - func(raw interface{}) error { - eval := raw.(*structs.Evaluation) - evals = append(evals, eval) - return nil + iter = memdb.NewFilterIterator(iter, func(raw interface{}) bool { + if eval := raw.(*structs.Evaluation); eval != nil { + return args.ShouldBeFiltered(eval) + } + return false }) - if err != nil { - return structs.NewErrRPCCodedf( - http.StatusBadRequest, "failed to create result paginator: %v", err) - } - nextToken, err := paginator.Page() - if err != nil { - return structs.NewErrRPCCodedf( - http.StatusBadRequest, "failed to read result page: %v", err) - } + tokenizer := paginator.NewStructsTokenizer(iter, opts) + filters := []paginator.Filter{ + paginator.NamespaceFilter{ + AllowableNamespaces: allowableNamespaces, + }, + } - reply.QueryMeta.NextToken = nextToken - reply.Evaluations = evals + var evals []*structs.Evaluation + paginator, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions, + func(raw interface{}) error { + eval := raw.(*structs.Evaluation) + evals = append(evals, eval) + return nil + }) + if err != nil { + return structs.NewErrRPCCodedf( + http.StatusBadRequest, "failed to create result paginator: %v", err) + } + + nextToken, err := paginator.Page() + if err != nil { + return structs.NewErrRPCCodedf( + http.StatusBadRequest, "failed to read result page: %v", err) + } + + reply.QueryMeta.NextToken = nextToken + reply.Evaluations = evals + } // Use the last index that affected the jobs table index, err := store.Index("evals") diff --git a/nomad/eval_endpoint_test.go b/nomad/eval_endpoint_test.go index bf05ce4d9..59b821125 100644 --- a/nomad/eval_endpoint_test.go +++ b/nomad/eval_endpoint_test.go @@ -1244,62 +1244,104 @@ func TestEvalEndpoint_List_ACL(t *testing.T) { defer cleanupS1() codec := rpcClient(t, s1) testutil.WaitForLeader(t, s1.RPC) - assert := assert.New(t) + + // Create dev namespace + devNS := mock.Namespace() + devNS.Name = "dev" + err := s1.fsm.State().UpsertNamespaces(999, []*structs.Namespace{devNS}) + require.NoError(t, err) // Create the register request eval1 := mock.Eval() eval1.ID = "aaaaaaaa-3350-4b4b-d185-0e1992ed43e9" eval2 := mock.Eval() eval2.ID = "aaaabbbb-3350-4b4b-d185-0e1992ed43e9" + eval3 := mock.Eval() + eval3.ID = "aaaacccc-3350-4b4b-d185-0e1992ed43e9" + eval3.Namespace = devNS.Name state := s1.fsm.State() - assert.Nil(state.UpsertEvals(structs.MsgTypeTestSetup, 1000, []*structs.Evaluation{eval1, eval2})) + err = state.UpsertEvals(structs.MsgTypeTestSetup, 1000, []*structs.Evaluation{eval1, eval2, eval3}) + require.NoError(t, err) // Create ACL tokens validToken := mock.CreatePolicyAndToken(t, state, 1003, "test-valid", mock.NamespacePolicy(structs.DefaultNamespace, "", []string{acl.NamespaceCapabilityReadJob})) invalidToken := mock.CreatePolicyAndToken(t, state, 1001, "test-invalid", mock.NamespacePolicy(structs.DefaultNamespace, "", []string{acl.NamespaceCapabilityListJobs})) + devToken := mock.CreatePolicyAndToken(t, state, 1005, "test-dev", + mock.NamespacePolicy("dev", "", []string{acl.NamespaceCapabilityReadJob})) - get := &structs.EvalListRequest{ - QueryOptions: structs.QueryOptions{ - Region: "global", - Namespace: structs.DefaultNamespace, + testCases := []struct { + name string + namespace string + token string + expectedEvals []string + expectedError string + }{ + { + name: "no token", + token: "", + namespace: structs.DefaultNamespace, + expectedError: structs.ErrPermissionDenied.Error(), + }, + { + name: "invalid token", + token: invalidToken.SecretID, + namespace: structs.DefaultNamespace, + expectedError: structs.ErrPermissionDenied.Error(), + }, + { + name: "valid token", + token: validToken.SecretID, + namespace: structs.DefaultNamespace, + expectedEvals: []string{eval1.ID, eval2.ID}, + }, + { + name: "root token default namespace", + token: root.SecretID, + namespace: structs.DefaultNamespace, + expectedEvals: []string{eval1.ID, eval2.ID}, + }, + { + name: "root token all namespaces", + token: root.SecretID, + namespace: structs.AllNamespacesSentinel, + expectedEvals: []string{eval1.ID, eval2.ID, eval3.ID}, + }, + { + name: "dev token all namespaces", + token: devToken.SecretID, + namespace: structs.AllNamespacesSentinel, + expectedEvals: []string{eval3.ID}, }, } - // Try without a token and expect permission denied - { - var resp structs.EvalListResponse - err := msgpackrpc.CallWithCodec(codec, "Eval.List", get, &resp) - assert.NotNil(err) - assert.Contains(err.Error(), structs.ErrPermissionDenied.Error()) - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + get := &structs.EvalListRequest{ + QueryOptions: structs.QueryOptions{ + AuthToken: tc.token, + Region: "global", + Namespace: tc.namespace, + }, + } - // Try with an invalid token and expect permission denied - { - get.AuthToken = invalidToken.SecretID - var resp structs.EvalListResponse - err := msgpackrpc.CallWithCodec(codec, "Eval.List", get, &resp) - assert.NotNil(err) - assert.Contains(err.Error(), structs.ErrPermissionDenied.Error()) - } + var resp structs.EvalListResponse + err := msgpackrpc.CallWithCodec(codec, "Eval.List", get, &resp) - // List evals with a valid token - { - get.AuthToken = validToken.SecretID - var resp structs.EvalListResponse - assert.Nil(msgpackrpc.CallWithCodec(codec, "Eval.List", get, &resp)) - assert.Equal(uint64(1000), resp.Index, "Bad index: %d %d", resp.Index, 1000) - assert.Lenf(resp.Evaluations, 2, "bad: %#v", resp.Evaluations) - } + if tc.expectedError != "" { + require.Contains(t, err.Error(), tc.expectedError) + } else { + require.NoError(t, err) + require.Equal(t, uint64(1000), resp.Index, "Bad index: %d %d", resp.Index, 1000) - // List evals with a root token - { - get.AuthToken = root.SecretID - var resp structs.EvalListResponse - assert.Nil(msgpackrpc.CallWithCodec(codec, "Eval.List", get, &resp)) - assert.Equal(uint64(1000), resp.Index, "Bad index: %d %d", resp.Index, 1000) - assert.Lenf(resp.Evaluations, 2, "bad: %#v", resp.Evaluations) + got := make([]string, len(resp.Evaluations)) + for i, eval := range resp.Evaluations { + got[i] = eval.ID + } + require.ElementsMatch(t, got, tc.expectedEvals) + } + }) } } @@ -1377,6 +1419,12 @@ func TestEvalEndpoint_List_PaginationFiltering(t *testing.T) { codec := rpcClient(t, s1) testutil.WaitForLeader(t, s1.RPC) + // Create non-default namespace + nondefaultNS := mock.Namespace() + nondefaultNS.Name = "non-default" + err := s1.fsm.State().UpsertNamespaces(999, []*structs.Namespace{nondefaultNS}) + require.NoError(t, err) + // create a set of evals and field values to filter on. these are // in the order that the state store will return them from the // iterator (sorted by create index), for ease of writing tests @@ -1388,7 +1436,7 @@ func TestEvalEndpoint_List_PaginationFiltering(t *testing.T) { }{ {ids: []string{"aaaa1111-3350-4b4b-d185-0e1992ed43e9"}, jobID: "example"}, // 0 {ids: []string{"aaaaaa22-3350-4b4b-d185-0e1992ed43e9"}, jobID: "example"}, // 1 - {ids: []string{"aaaaaa33-3350-4b4b-d185-0e1992ed43e9"}, namespace: "non-default"}, // 2 + {ids: []string{"aaaaaa33-3350-4b4b-d185-0e1992ed43e9"}, namespace: nondefaultNS.Name}, // 2 {ids: []string{"aaaaaaaa-3350-4b4b-d185-0e1992ed43e9"}, jobID: "example", status: "blocked"}, // 3 {ids: []string{"aaaaaabb-3350-4b4b-d185-0e1992ed43e9"}}, // 4 {ids: []string{"aaaaaacc-3350-4b4b-d185-0e1992ed43e9"}}, // 5 diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 031436d7c..9ce2fd72d 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -10956,6 +10956,14 @@ func (e *Evaluation) GetID() string { return e.ID } +// GetNamespace implements the NamespaceGetter interface, required for pagination. +func (e *Evaluation) GetNamespace() string { + if e == nil { + return "" + } + return e.Namespace +} + // GetCreateIndex implements the CreateIndexGetter interface, required for // pagination. func (e *Evaluation) GetCreateIndex() uint64 { diff --git a/ui/app/adapters/evaluation.js b/ui/app/adapters/evaluation.js index 7013b58a3..d927b71cb 100644 --- a/ui/app/adapters/evaluation.js +++ b/ui/app/adapters/evaluation.js @@ -10,10 +10,12 @@ export default class EvaluationAdapter extends ApplicationAdapter { } urlForFindRecord(_id, _modelName, snapshot) { - const url = super.urlForFindRecord(...arguments); + const namespace = snapshot.attr('namespace') || 'default'; + const baseURL = super.urlForFindRecord(...arguments); + const url = `${baseURL}?namespace=${namespace}`; if (snapshot.adapterOptions?.related) { - return `${url}?related=true`; + return `${url}&related=true`; } return url; } diff --git a/ui/app/controllers/evaluations/index.js b/ui/app/controllers/evaluations/index.js index 11afc9f12..cb13f3a49 100644 --- a/ui/app/controllers/evaluations/index.js +++ b/ui/app/controllers/evaluations/index.js @@ -35,7 +35,7 @@ export default class EvaluationsController extends Controller { 'currentEval', 'pageSize', 'status', - 'qpNamespace', + { qpNamespace: 'namespace' }, 'type', 'searchTerm', ]; diff --git a/ui/tests/acceptance/evaluations-test.js b/ui/tests/acceptance/evaluations-test.js index f79a08cf5..e94bb51b3 100644 --- a/ui/tests/acceptance/evaluations-test.js +++ b/ui/tests/acceptance/evaluations-test.js @@ -676,7 +676,14 @@ module('Acceptance | evaluations list', function (hooks) { module('evaluation detail', function () { test('clicking an evaluation opens the detail view', async function (assert) { server.get('/evaluations', getStandardRes); - server.get('/evaluation/:id', function (_, { params }) { + server.get('/evaluation/:id', function (_, { queryParams, params }) { + const expectedNamespaces = ['default', 'ted-lasso']; + assert.notEqual( + expectedNamespaces.indexOf(queryParams.namespace), + -1, + 'Eval details request has namespace query param' + ); + return { ...generateAcceptanceTestEvalMock(params.id), ID: params.id }; });