Make name to uid conversion more consistent with grizzly.jsonnet

[MrFreezeex]

So far grizzly enforce that the name strictly equals to the uid and even
set the UID in the parse function.

This is problematic because some characters are allowed in the name
and not in the uid. For instance many mixins put a ".json" has a suffix
in the name and the "." character is not something allowed in an UID.

With this commit grizzly will now only enforce that the uid exist and is
not an empty string. It also set by default to the md5 of the name in
the parse function.

Signed-off-by: Arthur Outhenin-Chalandre arthur.outhenin-chalandre@cern.ch

[malcolmholmes]

The UID and the name are, in the case of dashboards, intended to be essentially the same thing. If you are using mixins, and this is causing you problems, it will be because the mixin is not providing a UID within the dashboard. This in itself is problematic as Grafana won't know how to uniquely identify the file.

Here's where this case is currently handled:

grizzly/pkg/grizzly/grizzly.jsonnet

Lines 28 to 31 in 978538c

	local uid(k, dashboard) =
	if std.objectHasAll(dashboard, "uid")
	then dashboard.uid
	else k;

Looking at how prometheus-ksonnet handles this problem, it would make sense to just take the md5sum of the filename if no UID is provided, in the above grizzly.jsonnet snippet. This would solve your problem.

https://github.com/grafana/jsonnet-libs/blob/5a128df878434da37969b811e99bb9cd0a3779e3/prometheus-ksonnet/grafana/dashboards.libsonnet#L20

[MrFreezeex]

Indeed I am using some mixins. I tried grizzly with node-mixins actually (https://github.com/prometheus/node_exporter/tree/master/docs/node-mixin/dashboards) which does not set the uid but I also tried to set the uid on top of node-mixin with the same result.

The issue is that grizzly alway set the uid to the name even if the uid is provided by the user https://github.com/grafana/grizzly/pull/199/files#diff-83b82171360f543df501efc9342cc3475ef71208603a96dba97d096a06598d3dL73 and there is also various failchecks to ensure that (see the content of the PR for more details). In the node-mixin, for instance one of the dashboard is named node.json which can not be a uid (a "." is not something that can be in a uid) so it fails.

My solution here is to use the uid if provided and defaults to the md5 of the name of the dashboard otherwise. This way users can override with whatever they want and grizzly provide a safe default if they don't. The name in grizzly is mapped to the "file name" so it has a similar behavior than what you are suggesting to do in jsonnet (but it doesn't try to override the uid if the user has provided one).

Maybe we should alway set the uid to the md5 of the name though which would essentially keep the various failchecks in grizzly now but with a md5 hash instead of a simple equality. I don't have a really strong opinion on that, but in any case right now with a name like "node.json" there is no way to apply the dashboard as long as the name is not changed which is not super convenient so IMO a fix of some forms should be brought to grizzly...

[malcolmholmes]

There's a few things to clear up here. Firstly, the way that mixins specify their content is kinda broken. They expose their content to the outside world via hidden elements. There's no way this can be a good idea. And it causes all sorts of complications.

Grizzly has moved away from specifying resources in this way, to a Kubernetes inspired explicit method. In this world, the name and the UID must match. That's something the user can control.

Now, what you are talking about is about backwards compatibility with the mixin/hidden elements approach. This happens in the pkg/grizzly/grizzly.jsonnet file, which aims to transform hidden element based Jsonnet into k8s style Jsonnet. That is the only place where your JSON keys have any form of existence (they have no meaning in the k8s world).

So really, if this patch is going to happen, it needs to be focusing on grizzly.jsonnet as a translation, rather than on core Grizzly functionality that has deliberately moved on from the hidden element approach.

[MrFreezeex]

I can do a patch for jsonnet but this also is a problem for the yaml/kubernetes style as well:

$ cat dashboard-simple.yaml
apiVersion: grizzly.grafana.com/v1alpha1
kind: Dashboard
metadata:
    name: not-valid.json
    folder: General
spec:
    schemaVersion: 17
    tags:
        - templated
    timezone: browser
    title: Should not be here
    uid: will-be-replaced
$ ./grr apply dashboard-simple.yaml
2022-01-26 10:21:04.390145 I | 10:21:04 Non-200 response from Grafana while applying dashboard not-valid.json: 400 Bad Request {"message":"uid contains illegal characters"}

[malcolmholmes]

Ahh yes, I remember now. The correct behaviour should, in k8s land, be:

• if no UID, use name
• if UID and name differ, refuse to proceed, with a clear error message.

Clearly I never got around to implementing that (was assuming I had). Does it make sense? Fancy taking a stab?

[MrFreezeex]

Ahh yes, I remember now. The correct behaviour should, in k8s land, be:

* if no UID, use name

* if UID and name differ, refuse to proceed, with a clear error message.

Clearly I never got around to implementing that (was assuming I had). Does it make sense? Fancy taking a stab?

Well this is almost what is implemented at least it was clearly intended to be this way apart that the Parse function always override the uid by the name. But even if I fix that it won't solve the underlying issue which is that not every character in the name could be in the uid...

If I don't have the line uid: will-be-replaced the error would be the same because grizzly will make the uid equals not-valid.json and this will fail. So there should be a function to translate the name to a uid and IMO a md5 should do the job perfectly...

[MrFreezeex]

If I don't have the line uid: will-be-replaced the error would be the same because grizzly will make the uid equals not-valid.json and this will fail. So there should be a function to translate the name to a uid and IMO a md5 should do the job perfectly...

I did that in my latest commit so it's the same behavior that you described except that I use uid=md5(name) instead of uid=name.

[malcolmholmes]

but the problem you are trying to solve, a dot in a filename, is inherently derived from the mixin world (i.e. the hidden variable world). Therefore, the fix should be in the hidden->k8s translation layer, which is grizzly.jsonnet. Once we're in k8s resource world, if there's a dot in your name, well, remove it! In the mixin case, the filename should already have been converted to an md5sum.

[MrFreezeex]

but the problem you are trying to solve, a dot in a filename, is inherently derived from the mixin world (i.e. the hidden variable world). Therefore, the fix should be in the hidden->k8s translation layer, which is grizzly.jsonnet. Once we're in k8s resource world, if there's a dot in your name, well, remove it! In the mixin case, the filename should already have been converted to an md5sum.

Oh actually looking at grizzly.jsonnet it should already handle that but the k8s layer disregard that by setting uid in all cases. I will check how to make the things that grizzly.jsonnet enforced be respected 👍.

[MrFreezeex]

Hmmm while checking more about this grizzly.jsonnet story I saw that I am in fact not using that but the lib in jsonnet-lib instead https://github.com/grafana/jsonnet-libs/tree/master/grizzly.

The issue is that the jsonnet-libs doesn't do any fancy uid/name conversion https://github.com/grafana/jsonnet-libs/blob/master/grizzly/resource.libsonnet whereas the jsonnet code embed in grizzly does https://github.com/grafana/grizzly/blob/master/pkg/grizzly/grizzly.jsonnet#L8 https://github.com/grafana/grizzly/blob/master/pkg/grizzly/grizzly.jsonnet#L16.

So is using the jsonnet-libs version is the right thing to do in your opinion?
Should we do the same name->uid conversion as https://github.com/grafana/grizzly/blob/master/pkg/grizzly/grizzly.jsonnet#L16 in the grizzly golang code for the kubernetes style?

[malcolmholmes]

Using the jsonnet-libs version is better, as it puts you in control much more. I'm totally happy to accept PRs against that lib too. Please do port useful functionality from grizzly.jsonnet over to there.

[MrFreezeex]

Using the jsonnet-libs version is better, as it puts you in control much more. I'm totally happy to accept PRs against that lib too. Please do port useful functionality from grizzly.jsonnet over to there.

Thanks I have created grafana/jsonnet-libs#735.

I also updated some code here to better reflect what grizzly.jsonnet is doing to convert name to uid (https://github.com/grafana/grizzly/blob/master/pkg/grizzly/grizzly.jsonnet#L16). It will mostly not be a concern for me with the new PR to jsonnet-libs though so if you don't want something like this feel free to simply close the PR.

[MrFreezeex]

I also updated some code here to better reflect what grizzly.jsonnet is doing to convert name to uid (https://github.com/grafana/grizzly/blob/master/pkg/grizzly/grizzly.jsonnet#L16). It will mostly not be a concern for me with the new PR to jsonnet-libs though so if you don't want something like this feel free to simply close the PR.

I suppose the same problem also holds for dashboards folders (like space in the name won't probably work right now). I can do the same change to folders if you want it.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}