/cc @thockin @logicalhan @dgrisonnet

The first solution you propose will likely make the Regarding field of the Event invalid since both the name and that field are set based on the given object.

As for changing the name in the client, we will need to completely get rid of the regarded object name, otherwise the validation will never pass for IPAddresses because of the ':' character. But that would be quite a big change since AFAIK Events have always had that form and I am also not sure if any code was built with that naming structure in mind.

Another option could be to soften the event validation from:

/cc @wojtek-t

There are a few types which use name formats which are not DNS-subdomain compatible:

$ k get roles -A
NAMESPACE     NAME                                              CREATED AT
kube-public   system:controller:bootstrap-signer                2024-03-28T22:56:25Z
kube-system   system::leader-locking-cloud-controller-manager   2024-03-28T22:56:44Z
kube-system   system::leader-locking-kube-controller-manager    2024-03-28T22:56:25Z
kube-system   system::leader-locking-kube-scheduler             2024-03-28T22:56:25Z

$ k get rolebindings -A
NAMESPACE     NAME                                                ROLE                                                   AGE
kube-public   system:controller:bootstrap-signer                  Role/system:controller:bootstrap-signer                179d
kube-system   system::extension-apiserver-authentication-reader   Role/extension-apiserver-authentication-reader         179d
kube-system   system::leader-locking-cloud-controller-manager     Role/system::leader-locking-cloud-controller-manager   179d
kube-system   system::leader-locking-kube-controller-manager      Role/system::leader-locking-kube-controller-manager    179d
kube-system   system::leader-locking-kube-scheduler               Role/system::leader-locking-kube-scheduler             179d

As as aside: I wish we had not done this and I REALLY wish we had not used : in some places and :: in others (@liggitt may know the history).

I don't think it's a valid assumption that the "regarding" object's name can be the basis for the event's name. It's not at all a safe guarantee, and "name" is mostly irrelevant to events. It's common to have deployments and services named the same thing, so I could have events about both and they would just be mixed together.

staging/src/k8s.io/client-go/tools/events/event_recorder.go

func (recorder *recorderImpl) makeEvent(refRegarding *v1.ObjectReference, refRelated *v1.ObjectReference, timestamp metav1.MicroTime, eventtype, reason, message string, reportingController string, reportingInstance string, action string) *eventsv1.Event {                                   
    t := metav1.Time{Time: recorder.clock.Now()}    
    namespace := refRegarding.Namespace     
    if namespace == "" {    
        namespace = metav1.NamespaceDefault     
    }    
    return &eventsv1.Event{    
        ObjectMeta: metav1.ObjectMeta{    
            Name:      fmt.Sprintf("%v.%x", refRegarding.Name, t.UnixNano()),    
            Namespace: namespace,    
        },    

It might be better to use the refRegarding.UID if the goal is to collate events, but the UID may not be present. So maybe md5sum(refRegarding.Namespace + refRegarding.Name) or simply base64 encoded + truncate for length. Or just use generateName and give up on collation?

As as aside: I wish we had not done this and I REALLY wish we had not used : in some places and :: in others (@liggitt may know the history).

#45966 looks like the first and primary addition of :: ... I don't know the rationale there

/assign
/triage accepted

I don't think it's a valid assumption that the "regarding" object's name can be the basis for the event's name. It's not at all a safe guarantee, and "name" is mostly irrelevant to events.

Yes that's true, even kubectl doesn't show the event name by default, you'd have to specify -owide to see it, so I doubt anyone really use the names and it is very unlikely that users would delete or get specific events by name.

It might be better to use the refRegarding.UID if the goal is to collate events, but the UID may not be present. So maybe md5sum(refRegarding.Namespace + refRegarding.Name) or simply base64 encoded + truncate for length. Or just use generateName and give up on collation?

We are already combining events client-side based on the following fields:

We have a notion of event key in both implementations to merge similar events together, maybe we could reuse that and encode it?

core/v1 client code ref:

I don't think it's a valid assumption that the "regarding" object's name can be the basis for the event's name. It's not at all a safe guarantee, and "name" is mostly irrelevant to events.

+1

We have a notion of event key in both implementations to merge similar events together, maybe we could reuse that and encode it?

+1 to that - this logic was created to distinguish whether two events are about the same or about different things. Making a name to be function of it makes perfect sense to me.

@dgrisonnet @wojtek-t I'd like to get this merged #129790 (comment) and also explain why I think the proposal I'm sending of normalizing the name instead of using the event key is better.

To generate the eventKey you need to first create the event objects, so the code will look like

func (recorder *recorderImpl) makeEvent(refRegarding *v1.ObjectReference, refRelated *v1.ObjectReference, timestamp metav1.MicroTime, eventtype, reason, message string, reportingController string, reportingInstance string, action string) *eventsv1.Event {
	t := metav1.Time{Time: recorder.clock.Now()}
	namespace := refRegarding.Namespace
	if namespace == "" {
		namespace = metav1.NamespaceDefault
	}

   event := &eventsv1.Event{
}}}
event.Name = getEventKey(event)

I don't feel that adding this kind of pattern of generating a name of an object based on the same object without a field is ideal and just normalizing is more simpler

After this comment from @liggitt #129790 (comment) I may step back on my proposal, scratch my previous comment

I don't feel that adding this kind of pattern of generating a name of an object based on the same object without a field is ideal and just normalizing is more simpler

I think it is fine for events since we don't use their name to generate the keys or anything else. This field is just here because events are Kubernetes objects and they must have a name.

I think it is fine for events since we don't use their name to generate the keys or anything else. This field is just here because events are Kubernetes objects and they must have a name.

hmmm , we still have the same problem, the event key is not guaranteed to pass the event object name validation

I changed the approach #129790

1. generated name is valid -> keep it as it is today
2. is not valid -> change ref name by a random generated string of 63 alphanumeric characters