- Notifications
You must be signed in to change notification settings - Fork927
feat: add keys to organization provision daemons#14627
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codersdk/provisionerdaemons.go Outdated
| const ( | ||
| ProvisionerKeyIDBuiltIn = "11111111-1111-1111-1111-111111111111" | ||
| ProvisionerKeyIDUserAuth = "22222222-2222-2222-2222-222222222222" | ||
| ProvisionerKeyIDPSK = "33333333-3333-3333-3333-333333333333" | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These will probably need to be hardcoded on the frontend as well, since this doesn't seem to get generated.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| INSERT INTO provisioner_keys (id, created_at, organization_id, name, hashed_secret, tags) VALUES ('22222222-2222-2222-2222-222222222222'::uuid, NOW(), (SELECT id FROM organizations WHERE is_default = true), 'user-auth', ''::bytea, '{}'); | ||
| INSERT INTO provisioner_keys (id, created_at, organization_id, name, hashed_secret, tags) VALUES ('33333333-3333-3333-3333-333333333333'::uuid, NOW(), (SELECT id FROM organizations WHERE is_default = true), 'psk', ''::bytea, '{}'); | ||
| ALTER TABLE provisioner_daemons ADD COLUMN key_id UUID REFERENCES provisioner_keys(id) ON DELETE CASCADE DEFAULT '11111111-1111-1111-1111-111111111111'::uuid NOT NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We have external provisioners running today on some customer deployments. Will this be ok if they are assigned to the built in?
Should we assign the existing rows to thepsk at33333333-3333-3333-3333-33333333333, and have the startup code fix the built in rows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So I did try to consider the outputs here and picked this one since it felt like the least impact, but let me know if you feel otherwise:
- There's no data in the DB that lets us know if it's a built-in or PSK today
- External provisioners will need to reconnect after a coderd upgrade anyways, and on reconnection they will be assigned the correct
333...id. - On the average deployment we would most likely see more built-ins than external, so if we have to pick one, built-in is probably less incorrect
- I considered allowing null and not back porting the reference but that felt extra messy in the resulting queries and handler logic.
So basically with this migration the external provisioners will be incorrectly labeled as built-in until the provisioners reconnect. Because we only show recent provisioners the ones that do not connect again (and stay incorrectly labeled) will only be shown for a short while before being removed from the list we show users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also worth noting that right now this key reference is only for grouping in read operations, no mutation logic is performed based on this value today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The reconnecting external provisioners makes sense 👍
I feel very weird about hardcoding certain UUIDs as a "type" indicator Also, I think I totally spaced before on this, we'll want the key name on the front-end as well |
Yeah I get the hesitation, I'll try to layout my thought process that led me here:
I'm down to change the structure, so even if we use special IDs on the backend we can shim it into a "fake key" for the frontend response if that's needed. I think listing each key with a list of provisioners embedded in each key may be the best format IMO. |
aslilac commentedSep 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
also this is kind of a minor thought, but I might suggest const (ProvisionerKeyIDBuiltIn="00000000-0000-0000-0000-000000000001"ProvisionerKeyIDUserAuth="00000000-0000-0000-0000-000000000002"ProvisionerKeyIDPSK="00000000-0000-0000-0000-000000000003") I don't necessarily expect the number of these to balloon, but the current way gives us a maximum of 14 variants, this would allow for billions, just by using the last segment of the ID. plus it's a bit less "busy" visually. |
I am not opposed to the special IDs. I think it is better than assigning them actual UUIDs. As for 0s or 1s or w/e, I also took a few moments considering what the best static UUIDs should be. I was looking at HexWords to see if anything would be fun |
I've added a new endpoint that returns a list of keys each with their own embedded list of associated daemons. I've also changed the UUIDs to match what@aslilac suggested. I opted for a new endpoint so we don't break dev, and in a followup PR we can remove the old list endpoint. |
| keyID, err := uuid.Parse(string(codersdk.ProvisionerKeyIDBuiltIn)) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("failed to parse built-in provisioner key ID: %w", err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| keyID,err:=uuid.Parse(string(codersdk.ProvisionerKeyIDBuiltIn)) | |
| iferr!=nil { | |
| returnnil,xerrors.Errorf("failed to parse built-in provisioner key ID: %w",err) | |
| } | |
| keyID:=uuid.MustParse(codersdk.ProvisionerKeyIDBuiltIn) |
- a built-in constant we control feels like a good target for
uuid.MustParse - these are already just basic strings, yeah?
const (ProvisionerKeyIDBuiltIn="00000000-0000-0000-0000-000000000001"ProvisionerKeyIDUserAuth="00000000-0000-0000-0000-000000000002"ProvisionerKeyIDPSK="00000000-0000-0000-0000-000000000003")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
They should be safe, but I'd rather just follow the no panic principal here so we at least return a proper error in the response
| @@ -71,7 +71,10 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
| httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(daemons, db2sdk.ProvisionerDaemon)) | |||
| // provisionerdserver.DefaultHeartbeatInterval*3 matches the healthcheck report staleInterval. | |||
| recentDaemons := db2sdk.RecentProvisionerDaemons(time.Now(), provisionerdserver.DefaultHeartbeatInterval*3, daemons) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this'll prevent the frontend from warning about non-recent provisioners. I could see this being useful as a query/filter parameter but I'm not sure it's the right default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The recency threshold here is the same as the deployment level provisioners page. If that page has non-recent warnings, I wonder if the warning threshold is lower than the "recent" threshold on the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess it's fine to match the health page for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this should be a query param. The current stale timer is a hack imo.
In the future, we should just show the current connected provisioner daemons. They all hold open a websocket to 1 Coderd. So we could implement a method of detecting when a provisioner goes away immediately, and not wait for heartbeats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The realtime data on connected provisioners was a reach goal for GA that we decided was not required, so I think it's totally something we can look at on a future pass.
enterprise/coderd/provisionerkeys.go Outdated
| // provisionerdserver.DefaultHeartbeatInterval*3 matches the healthcheck report staleInterval. | ||
| recentDaemons := db2sdk.RecentProvisionerDaemons(time.Now(), provisionerdserver.DefaultHeartbeatInterval*3, daemons) | ||
| pkDaemons := make([]codersdk.ProvisionerKeyDaemons, 0, len(sdkKeys)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if there's more than one provisioner per key on average, this'll reallocate. a safer upper-bound would belen(recentDaemons), but there is the possibility that it could overshoot by a bit. I'm fine with either, just making sure the tradeoffs have been considered!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reallocating is not a big deal.
hmm, it's scoped under /provisionerkeys. does it just list built-ins etc. under their "reserved" keys? feels like a bit of a weird place to grab them from, and we can do the grouping on the frontend. I guess this means we only need to make one request tho instead of joining two together. |
| @@ -89,7 +106,7 @@ func (api *API) provisionerKeys(rw http.ResponseWriter, r *http.Request) { | |||
| ctx := r.Context() | |||
| organization := httpmw.OrganizationParam(r) | |||
| pks, err := api.Database.ListProvisionerKeysByOrganization(ctx, organization.ID) | |||
| pks, err := api.Database.ListProvisionerKeysByOrganizationExcludeReserved(ctx, organization.ID) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why does the api exclude reserved? Why not include psk and built ins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This endpoint is used by the CLI and I don't think it's necessary to show them in isolation or without a reference to the daemon counterparts since they are more placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you think we can deprecate the old endpoint at some point in the future? Should we mark it deprecated in swagger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We might want to make a package that handles all this provisioner and grouping logic.
Not worth it now, but as provisioners are used more within the codebase, it would be annoying as a dev to remember about reserved keys and whatnot.
Uh oh!
There was an error while loading.Please reload this page.
f0ssel commentedSep 16, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yeah 💯 , I plan on taking another pass at this backend data structure and probably revising it since the fake key relations do have some shortcomings. I would look at this as a starting point to unblock the UI more than a finalized spec. |
335eb05 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#14439
What is changes:
KeyIDfield to provisioner daemonsbuilt-in,user-auth, andpskI'd like to add more that test deeper functionality on the get provisioner daemons endpoint. I'll do this in a followup so this can be merged and unblock UI work.