Proposal: Use UUID for Trigger Event IDs

[wlynch]

Short proposal I wanted to get feedback on before making a change.

Currently we generate Event IDs similar to generateName by randomly generating a 5 character string to associate to the event. While this has worked so far, we might find that the current 5 character solution isn't sufficient in the future, particularly if/when we start recording events that don't result in runs.

Instead of slowly growing the Event ID over time, I think we should switch over to UUIDs for Event ID generation. This is how k8s objects generate UIDs, and would be a fairly straight forward change for us since we treat these IDs as opaque strings today.

Let me know if anyone has any objections!

[dibyom]

Sounds reasonable to me!

[MarcelMue]

Sounds reasonable - some notes:

• Is the current ID used in any length limited fields (e.g. name)? Following RFC4122 we would have 36 characters instead of 5
• Do we expect the hyphens in the UUID to have any impact? It shouldn't in k8s but I am not sure where this ID is currently used
• I would like to introduce more debug logging in triggers, logging the full UUID might be very spammy. There are multiple solutions to shorten UUIDs for that purpose. We should at least think about it.

[afrittoli]

We used to have $(uid) in resource names in trigger templates - we still do in the getting started, which with the new uid is not possible, as the resource name becomes too long:

{"level":"error","logger":"eventlistener","caller":"sink/sink.go:237","msg":"couldn't create resource with group version kind \"tekton.dev/v1beta1, Resource=taskruns\": admission webhook \"validation.webhook.pipeline.tekton.dev\" denied the request: validation failed: Invalid resource name: length must be no more than 63 characters: metadata.name","knative.dev/controller":"eventlistener","/triggers-eventid":"06887615-0494-4714-93eb-1e79662ad218","/trigger":"tekton","logging.googleapis.com/labels":{},"logging.googleapis.com/sourceLocation":{"file":"github.com/tektoncd/triggers/pkg/sink/sink.go","line":"237","function":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger"},"stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:237\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:125"}

I think new UUID should be introduced as an opt-in, otherwise the new release will break many templates.