Chore: Replace short UID generation with more standard UUIDs #62731
Conversation
* Alerting: Fix template validation in provisioning api Fix issue where provisioning API accepts a malformed template having extra text outside of definition block and template name matching definition name.
…62033) * Start work on allowing certain resources to pass through Cache-Control headers. --------- Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Co-authored-by: Dan Cech <dcech@grafana.com>
ryantxu
changed the title
ryantxu
changed the title
ryantxu
changed the title
There was a problem hiding this comment.
Looks good, assuming we haven't broken anything else that used short uids. The one that seems like it might be an issue would be the short url service as they won't be so short any more.
One thought may be to instead add GenerateUID() and convert things over to that where it makes sense while leaving GenerateShortUID as-is, but obviously that will result in a somewhat larger change.
|
The change looks safe to me, the length of UID is limited to 40 bytes, which is backward compatible for db + unique index limitation. Since the generation is more strict and validation stays the same way, modify dashboard is not impacted. Just need to pay attention, the save dashboard service is used by both folder and dashboard create api, that means the folder service is gonna having the same uid pattern. We probably want to update some docs if this is going used outside, for example we allow user to modify fold/dashboard uid manually right now still. |
|
Coincidentally, I was in the middle of making a similar change but isolated to just the ngalert package. Not because of k8s naming standards but because the current short uid generator consistently creates case-insensitive conflicts when ran in a large enough tight loop. Normally this is fine, but if someone is using MySQL this can and does create key errors. Ex: #60494, https://github.com/grafana/support-escalations/issues/4887 And a couple other places where it's looming but rare. |
ryantxu
requested review from
zserge,
mildwonkey and
idafurjes
and removed request for
a team
We currently generate a UID using github.com/teris-io/shortid -- and then loop 3x in the database to make sure it is actually unique. This PR replaces that logic with a more standard UUID that we can depend on being unique.
This also makes sure the generated UUID starts with a string -- this is to ensure that if we want to use it as a k8s name, it can be https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
The new UIDs look like:
before... the generated UIDs look like:
These may look a little bit better in URLs ¯\(ツ)/¯ -- but given that they:
they can not be used as k8s names
NOTE: this change is not required, but it does seem worthwhile to stop creating invalid k8s names. This will help us have fewer things to migrate if we do want to map our current "uid" concept to a k8s name.