feat(coderd): add endpoint to fetch provisioner key details #15505
Conversation
…sing its id fix: change from database.StringMap to codersdk.ProvisionerKeyTags in endpoint response improve annotations for endpoint move logic to enterprise part generate new doc move logic to enterprise part generate doc
3db02d2 to
995a046
Compare
There was a problem hiding this comment.
Looks good, but I think we've gone too narrow with this endpoint. I don't have the full context though so I might be mistaken, but based on what @johnstcn said in #15126 (comment) I don't think we need an endpoint just for tags
I'd agree, it would make sense to return an endpoint that shows all the details of the provisioner key used to authenticate (except hashed secret obvs) |
aca52f4 to
54437f2
Compare
There was a problem hiding this comment.
LGTM 👍
Left a few minor thoughts; deferring to @johnstcn for final review since he has more context.
| require.Equal(t, fetchedKey.Name, "my-test-key") | ||
| require.Equal(t, fetchedKey.Tags, codersdk.ProvisionerKeyTags{"key1": "value1", "key2": "value2"}) | ||
|
|
||
| erroneousPK, err := client.GetProvisionerKey(ctx, "abcdefghijklmnopqrstuvwxyz01234567890123456") |
There was a problem hiding this comment.
Nit: you're testing multiple scenarios in one test; it's closer to our style to use table tests in this case.
I also think you should add a test to validate the middleware/auth setup.
There was a problem hiding this comment.
Suggest testing following scenarios:
- No auth (403)
- API key auth (403 with message indicating required header)
- PSK auth (403 with message indicating required header)
- Provisioner key auth (200)
There was a problem hiding this comment.
I added some tests as discussed and changed to table tests - should give more coverage on the different cases.
|
@defelmnq can you also update the PR title & description to reflect the wider scope pls? |
7880e9c to
4a38977
Compare
The description looks half-completed, btw:
|
|
@johnstcn sorry for all the pings, yesterday evening I rebased which seemed to have impacted the PR but I pushed back the missing tests - we should now be good to go ! 🙏 |
@defelmnq yet another reason why rebase is evil 🥲 |
| @@ -200,17 +200,44 @@ func (api *API) deleteProvisionerKey(rw http.ResponseWriter, r *http.Request) { | |||
| httpapi.Write(ctx, rw, http.StatusNoContent, nil) | |||
| } | |||
|
|
|||
| // @Summary Fetch provisioner key details | |||
| // @ID fetch-provisioner-key-details | |||
| // @Security CoderSessionToken | |||
There was a problem hiding this comment.
This comment should actually be updated as a session token here isn't sufficient auth. Sorry I didn't catch this sooner!
There was a problem hiding this comment.
I checked the definition of the /provisionerdaemons/serve endpoint and it looks like it also specifies @Security CoderSessionToken. It looks like that's the only value we allow there currently -- swaggerparser.go enforces this with some special cases for certain endpoints.
There was a problem hiding this comment.
As discussed , there's currently some extra logic to change in swaggerparser.go -> https://github.com/coder/coder/blob/main/coderd/coderdtest/swaggerparser.go#L302
I can either keep it like that and create the follow-up issue / PR quickly or add this endpoint to the ignore list for now - as you prefer.
There was a problem hiding this comment.
Let's keep it as-is for the moment and create a follow-up PR to address this.
I like to think of it as a knife with no handle, just all blade. It'll do the job, but it just has lots of sharp edges! |
| require.NoError(t, err) | ||
|
|
||
| fetchedKey, err := client.GetProvisionerKey(ctx, "psk-testing-purpose") | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Please be quite specific in the error you're checking for. The intent is not expressed clearly enough here.
This test should effectively be saying "Provisioner PSKs are similar to provisioner keys, but they cannot be used to retrieve information about a provisioner, so we get this very specific error message".
There was a problem hiding this comment.
TBH I feel like i'm maybe missing something or we're over testing the same thing here. as the code is always coding the same method and filling the same header - we will always fall into the same test case in the middleware.
Not that I want to minimise the tests - but at the end :
https://github.com/coder/coder/pull/15505/files#diff-2eae6501c14fe87b5578bcd6fda3588cffb28d437c61765e3829ff530906927cR376 -> here we set the same header hardcoded.
https://github.com/coder/coder/blob/main/coderd/httpmw/provisionerdaemon.go#L28 -> This middleware will never really go into the PSK / CoderSession logic as they're never set on the other side.
I would rather add tests into the middleware if we want to increase coverage.
| require.NoError(t, err) | ||
|
|
||
| fetchedKey, err := client.GetProvisionerKey(ctx, client.SessionToken()) | ||
| require.Error(t, err) |
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| fetchedKey, err := client.GetProvisionerKey(ctx, "psk-testing-purpose") |
There was a problem hiding this comment.
Nit: use a const here
|
Small update - I'll create two follow-up issues (will link) after merging this PR :
|
This PR is the first step aiming to resolve #15126 -
Creating a new endpoint to return the details associated to a provisioner key.
This is an authenticated endpoints aiming to be used by the provisioner daemons - using the provisioner key as authentication method.
This endpoint is not ment to be used with PSK or User Sessions.