Skip to content

Commit 8793e10

Browse files
mxabmxab
authored
Fix token resolve error (#49)
* chore: test validate / create job token * fix: only really resolve token of admission controlling is required fixes #48
1 parent 8ac4ea1 commit 8793e10

File tree

2 files changed

+129
-32
lines changed

2 files changed

+129
-32
lines changed

‎cmd/nacp/nacp.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,8 @@ func newProxyHandler(nomadAddress *url.URL, jobHandler *admissionctrl.JobHandler
161161
}
162162

163163
token := r.Header.Get("X-Nomad-Token")
164-
if jobHandler.ResolveToken() {
164+
isAdmissionActionable := isRegister(r) || isPlan(r) || isValidate(r)
165+
if isAdmissionActionable && jobHandler.ResolveToken() {
165166
tokenInfo, err := resolveTokenAccessor(ctx, transport, nomadAddress, token)
166167
if err != nil {
167168
logger.ErrorContext(ctx, "Resolving token failed", "error", err)

‎cmd/nacp/nacp_test.go‎

Lines changed: 127 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,16 @@ func TestProxy(t *testing.T) {
4242
path string
4343
method string
4444

45-
token string
46-
resolveToken bool
47-
accessorID string
45+
token string
46+
resolveToken bool
47+
wantTokenResolveToBeCalled bool
4848

49-
requestSender func(*api.Client) (interface{}, *api.WriteMeta, error)
49+
accessorID string
50+
51+
requestSender func(*api.Client) (interface{}, any, error)
5052
wantNomadRequestJson string
51-
wantProxyResponse interface{}
53+
wantProxyResponse any
54+
wantError string
5255
nomadResponse string
5356
nomadResponseEncoding string
5457
// responseWarnings []error
@@ -57,13 +60,12 @@ func TestProxy(t *testing.T) {
5760
}
5861

5962
tests := []test{
60-
6163
{
6264
name: "create job adds hello meta",
6365
path: "/v1/jobs",
6466
method: "PUT",
6567

66-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
68+
requestSender: func(c *api.Client) (interface{}, any, error) {
6769
return c.Jobs().Register(testutil.ReadJob(t, "job.json"), nil)
6870
},
6971
wantNomadRequestJson: registerRequestJson(t, jobWithHelloWorldMeta(t)),
@@ -82,7 +84,7 @@ func TestProxy(t *testing.T) {
8284
path: "/v1/job/example/plan",
8385
method: "PUT",
8486

85-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
87+
requestSender: func(c *api.Client) (interface{}, any, error) {
8688
return c.Jobs().Plan(testutil.ReadJob(t, "job.json"), false, nil)
8789
},
8890

@@ -102,7 +104,7 @@ func TestProxy(t *testing.T) {
102104
path: "/v1/job/example/plan",
103105
method: "PUT",
104106

105-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
107+
requestSender: func(c *api.Client) (interface{}, any, error) {
106108
return c.Jobs().Plan(testutil.ReadJob(t, "job.json"), false, nil)
107109
},
108110

@@ -127,7 +129,7 @@ func TestProxy(t *testing.T) {
127129
path: "/v1/job/example/plan",
128130
method: "PUT",
129131

130-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
132+
requestSender: func(c *api.Client) (interface{}, any, error) {
131133
return c.Jobs().Plan(testutil.ReadJob(t, "job.json"), false, nil)
132134
},
133135

@@ -153,7 +155,7 @@ func TestProxy(t *testing.T) {
153155
path: "/v1/jobs",
154156
method: "PUT",
155157

156-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
158+
requestSender: func(c *api.Client) (interface{}, any, error) {
157159
return c.Jobs().Register(testutil.ReadJob(t, "job.json"), nil)
158160
},
159161

@@ -174,7 +176,7 @@ func TestProxy(t *testing.T) {
174176
path: "/v1/jobs",
175177
method: "PUT",
176178

177-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
179+
requestSender: func(c *api.Client) (interface{}, any, error) {
178180
return c.Jobs().Register(testutil.ReadJob(t, "job.json"), nil)
179181
},
180182

@@ -198,7 +200,7 @@ func TestProxy(t *testing.T) {
198200
path: "/v1/jobs",
199201
method: "PUT",
200202

201-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
203+
requestSender: func(c *api.Client) (interface{}, any, error) {
202204
return c.Jobs().Register(testutil.ReadJob(t, "job.json"), nil)
203205
},
204206

@@ -222,7 +224,7 @@ func TestProxy(t *testing.T) {
222224
name: "plan job adds warnings",
223225
path: "/v1/job/example/plan",
224226
method: "PUT",
225-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
227+
requestSender: func(c *api.Client) (interface{}, any, error) {
226228
return c.Jobs().Plan(testutil.ReadJob(t, "job.json"), false, nil)
227229
},
228230

@@ -243,7 +245,7 @@ func TestProxy(t *testing.T) {
243245
path: "/v1/validate/job",
244246
method: "PUT",
245247

246-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
248+
requestSender: func(c *api.Client) (interface{}, any, error) {
247249
return c.Jobs().Validate(testutil.ReadJob(t, "job.json"), nil)
248250
},
249251
wantNomadRequestJson: toJson(t, &api.JobValidateRequest{Job: jobWithHelloWorldMeta(t)}),
@@ -261,7 +263,7 @@ func TestProxy(t *testing.T) {
261263
path: "/v1/validate/job",
262264
method: "PUT",
263265

264-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
266+
requestSender: func(c *api.Client) (interface{}, any, error) {
265267
return c.Jobs().Validate(testutil.BaseJob(), nil)
266268
},
267269
wantNomadRequestJson: toJson(t, &api.JobValidateRequest{Job: testutil.BaseJob()}),
@@ -281,7 +283,7 @@ func TestProxy(t *testing.T) {
281283
path: "/v1/validate/job",
282284
method: "PUT",
283285

284-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
286+
requestSender: func(c *api.Client) (interface{}, any, error) {
285287
return c.Jobs().Validate(testutil.BaseJob(), nil)
286288
},
287289
wantNomadRequestJson: toJson(t, &api.JobValidateRequest{Job: testutil.BaseJob()}),
@@ -302,7 +304,7 @@ func TestProxy(t *testing.T) {
302304
path: "/v1/validate/job",
303305
method: "PUT",
304306

305-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
307+
requestSender: func(c *api.Client) (interface{}, any, error) {
306308
return c.Jobs().Validate(testutil.BaseJob(), nil)
307309
},
308310
wantNomadRequestJson: toJson(t, &api.JobValidateRequest{Job: testutil.BaseJob()}),
@@ -319,15 +321,16 @@ func TestProxy(t *testing.T) {
319321
mutators: []admissionctrl.JobMutator{},
320322
},
321323
{
322-
name: "resolves token during job creation",
324+
name: "resolves token during job validation",
323325
path: "/v1/validate/job",
324326

325-
method: "PUT",
326-
token: "test-token",
327-
resolveToken: true,
328-
accessorID: "test-accessor",
327+
method: "PUT",
328+
token: "test-token",
329+
resolveToken: true,
330+
wantTokenResolveToBeCalled: true,
331+
accessorID: "test-accessor",
329332

330-
requestSender: func(c *api.Client) (interface{}, *api.WriteMeta, error) {
333+
requestSender: func(c *api.Client) (interface{}, any, error) {
331334
return c.Jobs().Validate(testutil.BaseJob(), nil)
332335
},
333336
wantNomadRequestJson: toJson(t, &api.JobValidateRequest{Job: testutil.BaseJob()}),
@@ -343,9 +346,88 @@ func TestProxy(t *testing.T) {
343346
},
344347
mutators: []admissionctrl.JobMutator{},
345348
},
349+
{
350+
name: "resolves token during job registration",
351+
path: "/v1/jobs",
352+
353+
method: "PUT",
354+
token: "test-token",
355+
resolveToken: true,
356+
wantTokenResolveToBeCalled: true,
357+
accessorID: "test-accessor",
358+
359+
requestSender: func(c *api.Client) (interface{}, any, error) {
360+
return c.Jobs().Register(testutil.BaseJob(), nil)
361+
},
362+
wantNomadRequestJson: toJson(t, &api.JobRegisterRequest{Job: testutil.BaseJob()}),
363+
364+
wantProxyResponse: &api.JobRegisterResponse{
365+
Warnings: helper.MergeMultierrorWarnings(errors.New("some warning")),
366+
},
367+
368+
nomadResponse: toJson(t, &api.JobRegisterResponse{}),
369+
nomadResponseEncoding: "gzip",
370+
validators: []admissionctrl.JobValidator{
371+
testutil.MockValidatorReturningWarnings("some warning"),
372+
},
373+
mutators: []admissionctrl.JobMutator{},
374+
},
375+
{
376+
name: "resolves token auth error during job registration",
377+
path: "/v1/jobs",
378+
379+
method: "PUT",
380+
token: "bad-token",
381+
resolveToken: true,
382+
wantTokenResolveToBeCalled: true,
383+
accessorID: "test-accessor",
384+
wantError: "failed to resolve token",
385+
386+
requestSender: func(c *api.Client) (interface{}, any, error) {
387+
return c.Jobs().Register(testutil.BaseJob(), nil)
388+
},
389+
wantNomadRequestJson: toJson(t, &api.JobRegisterRequest{Job: testutil.BaseJob()}),
390+
391+
wantProxyResponse: &api.JobRegisterResponse{
392+
Warnings: helper.MergeMultierrorWarnings(errors.New("some warning")),
393+
},
394+
395+
nomadResponse: toJson(t, &api.JobRegisterResponse{}),
396+
nomadResponseEncoding: "gzip",
397+
validators: []admissionctrl.JobValidator{
398+
testutil.MockValidatorReturningWarnings("some warning"),
399+
},
400+
mutators: []admissionctrl.JobMutator{},
401+
},
402+
{
403+
404+
name: "resolves token auth error is not a problem for non admissionable endpoints",
405+
path: "/v1/jobs",
406+
407+
method: "GET",
408+
token: "bad-token",
409+
resolveToken: true,
410+
wantTokenResolveToBeCalled: false,
411+
accessorID: "test-accessor",
412+
413+
requestSender: func(c *api.Client) (interface{}, any, error) {
414+
return c.Jobs().List(&api.QueryOptions{})
415+
},
416+
wantNomadRequestJson: "",
417+
418+
wantProxyResponse: make([]*api.JobListStub, 0),
419+
420+
nomadResponse: toJson(t, []api.JobListStub{}),
421+
nomadResponseEncoding: "gzip",
422+
validators: []admissionctrl.JobValidator{
423+
testutil.MockValidatorReturningWarnings("some warning"),
424+
},
425+
mutators: []admissionctrl.JobMutator{},
426+
},
346427
}
347428

348429
for _, tc := range tests {
430+
349431
t.Run(tc.name, func(t *testing.T) {
350432
nomadBackendCalled := false
351433
tokenCalled := false
@@ -371,7 +453,11 @@ func TestProxy(t *testing.T) {
371453
assert.Equal(t, req.URL.Path, tc.path)
372454
jsonData, err := io.ReadAll(req.Body)
373455
require.NoError(t, err)
374-
assert.JSONEq(t, tc.wantNomadRequestJson, string(jsonData))
456+
if tc.wantNomadRequestJson == "" {
457+
assert.Equal(t, 0, len(jsonData), "Body should be empty")
458+
} else {
459+
assert.JSONEq(t, tc.wantNomadRequestJson, string(jsonData))
460+
}
375461

376462
if tc.nomadResponseEncoding == "gzip" {
377463
rw.Header().Set("Content-Encoding", "gzip")
@@ -406,13 +492,23 @@ func TestProxy(t *testing.T) {
406492
}
407493

408494
resp, _, err := tc.requestSender(nomadClient)
409-
if tc.resolveToken {
410-
assert.True(t, tokenCalled, "token resolution should be called")
411-
}
495+
assert.Equalf(t, tc.wantTokenResolveToBeCalled, tokenCalled, "token should %sbe called", func() string {
496+
if tc.wantTokenResolveToBeCalled {
497+
return ""
498+
} else {
499+
return "not "
500+
}
501+
}())
412502

413-
require.NoError(t, err, "No http call error")
414-
assert.Equal(t, tc.wantProxyResponse, resp, "Response matches")
415-
assert.True(t, nomadBackendCalled, "Nomad backend was called")
503+
if tc.wantError != "" {
504+
require.Error(t, err)
505+
assert.Contains(t, err.Error(), tc.wantError, "Error should match")
506+
return
507+
} else {
508+
require.NoError(t, err, "No http call error")
509+
assert.Equal(t, tc.wantProxyResponse, resp, "Response matches")
510+
assert.True(t, nomadBackendCalled, "Nomad backend was called")
511+
}
416512
})
417513
}
418514
}

0 commit comments

Comments
 (0)