Repository: kubernetes/kubernetes (language: go)

Issue:
Events can not reference objects that use a different name validation
Seen in https://github.com/kubernetes/kubernetes/issues/127588

> E0924 12:16:50.003934       1 event_broadcaster.go:270] "Server rejected event (will not retry!)" err="Event \"fd00:10:233::103.17f82d400041d81f\" is invalid: metadata.name: Invalid value: \"fd00:10:233::103.17f82d400041d81f\":
a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])
?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')" event="&Event{ObjectMeta:{fd00:10:233::103.17f82d400041d81f  default    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] [] []},EventTime:2024-09-24 12:16:50.002035963 +0000 UTC
m=+179.166665408,Series:nil,ReportingController:ipallocator-repair-controller,ReportingInstance:ipallocator-repair-controller-svc-control-plane,Action:IPAddressAllocation,Reason:IPAddressNotAllocated,Regarding:{IPAddress  fd00:1
0:233::103 86ced933-1da5-4fac-b2e3-ab849185e2d5 networking.k8s.io/v1beta1 7070 },Related:nil,Note:IPAddress: fd00:10:233::103 for Service default/v6-service appears to have leaked: cleaning up,Type:Warning,DeprecatedSource:{ },D
eprecatedFirstTimestamp:0001-01-01 00:00:00 +0000 UTC,DeprecatedLastTimestamp:0001-01-01 00:00:00 +0000 UTC,DeprecatedCount:0,}"

An IPAddress object can contain an IPv6 address `fd00:10:233::103` 
The event recorder creates a new Event object based on the referenced object

> 		r.recorder.Eventf(ipAddress, nil, v1.EventTypeWarning, "IPAddressNotAllocated", "IPAddressAllocation", "IPAddress %s appears to have been modified, not referencing a Service %v: cleaning up", ipAddress.Name, ipAddress.Spec.ParentRef)

https://github.com/kubernetes/kubernetes/blob/464a994a10b71c45583f3426fd970291f8a5b756/staging/src/k8s.io/client-go/tools/events/event_recorder.go#L67-L75

I can see two options:
1. adapt the controller code to normalize the IPv6 addresses to meet the Event name convention
2. modify `r.recorder.Eventf` to make valid names on the Events objects

The result will be the same with 1 or 2, the difference is that option 2. will be valid for all the objects that use names not valid for the existing Events validation

/sig instrumentation



Discussion / hints:
/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: https://github.com/kubernetes/kubernetes/blob/4c24b9337fa189d99a721bfdc4785d62a744261b/pkg/apis/core/validation/events.go#L50
This check was added for the events/v1 API only, the core/v1 Events' name was never validated before and that's still the case today. I found some context for that change in https://github.com/kubernetes/kubernetes/pull/91645#discussion_r441328938, and it sounds fairly safe to soften the validation to support IPv6 values.

/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).

https://github.com/kubernetes/kubernetes/pull/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: https://github.com/kubernetes/kubernetes/blob/d99d3f7eb734dc9284a90374fcc6d8cc32d1946d/staging/src/k8s.io/client-go/tools/events/event_broadcaster.go#L58-L66
I don't think there are use-cases where anyone would need the name to have any particular meaning as all the relevant information to visualize and manage events are already present in the other 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:
https://github.com/kubernetes/kubernetes/blob/d99d3f7eb734dc9284a90374fcc6d8cc32d1946d/staging/src/k8s.io/client-go/tools/record/events_cache.go#L52-L83
> 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 https://github.com/kubernetes/kubernetes/pull/129790#discussion_r1950427007 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 https://github.com/kubernetes/kubernetes/pull/129790#discussion_r1951663509 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 https://github.com/kubernetes/kubernetes/pull/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

Fix patch:
diff --git a/staging/src/k8s.io/client-go/tools/events/event_recorder.go b/staging/src/k8s.io/client-go/tools/events/event_recorder.go
index 654317884f202..ba2ec7be4e93b 100644
--- a/staging/src/k8s.io/client-go/tools/events/event_recorder.go
+++ b/staging/src/k8s.io/client-go/tools/events/event_recorder.go
@@ -96,7 +96,7 @@ func (recorder *recorderImpl) makeEvent(refRegarding *v1.ObjectReference, refRel
 	}
 	return &eventsv1.Event{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf("%v.%x", refRegarding.Name, t.UnixNano()),
+			Name:      util.GenerateEventName(refRegarding.Name, t.UnixNano()),
 			Namespace: namespace,
 		},
 		EventTime:           timestamp,
diff --git a/staging/src/k8s.io/client-go/tools/record/event.go b/staging/src/k8s.io/client-go/tools/record/event.go
index 55947d2094b65..f97c5d612eac0 100644
--- a/staging/src/k8s.io/client-go/tools/record/event.go
+++ b/staging/src/k8s.io/client-go/tools/record/event.go
@@ -489,7 +489,7 @@ func (recorder *recorderImpl) makeEvent(ref *v1.ObjectReference, annotations map
 	}
 	return &v1.Event{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:        fmt.Sprintf("%v.%x", ref.Name, t.UnixNano()),
+			Name:        util.GenerateEventName(ref.Name, t.UnixNano()),
 			Namespace:   namespace,
 			Annotations: annotations,
 		},
diff --git a/staging/src/k8s.io/client-go/tools/record/util/util.go b/staging/src/k8s.io/client-go/tools/record/util/util.go
index afcc6a6a07ed3..7a82db0cd2fc3 100644
--- a/staging/src/k8s.io/client-go/tools/record/util/util.go
+++ b/staging/src/k8s.io/client-go/tools/record/util/util.go
@@ -17,10 +17,14 @@ limitations under the License.
 package util
 
 import (
+	"fmt"
 	"net/http"
 
+	"github.com/google/uuid"
+
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
 )
 
 // ValidateEventType checks that eventtype is an expected type of event
@@ -38,3 +42,16 @@ func IsKeyNotFoundError(err error) bool {
 
 	return statusErr != nil && statusErr.Status().Code == http.StatusNotFound
 }
+
+// GenerateEventName generates a valid Event name from the referenced name and the passed UNIX timestamp.
+// The referenced Object name may not be a valid name for Events and cause the Event to fail
+// to be created, so we need to generate a new one in that case.
+// Ref: https://issues.k8s.io/127594
+func GenerateEventName(refName string, unixNano int64) string {
+	name := fmt.Sprintf("%s.%x", refName, unixNano)
+	if errs := apimachineryvalidation.NameIsDNSSubdomain(name, false); len(errs) > 0 {
+		// Using an uuid guarantees uniqueness and correctness
+		name = uuid.New().String()
+	}
+	return name
+}


Test patch:
diff --git a/staging/src/k8s.io/client-go/tools/events/event_recorder_test.go b/staging/src/k8s.io/client-go/tools/events/event_recorder_test.go
new file mode 100644
index 0000000000000..0e9aed9cd8715
--- /dev/null
+++ b/staging/src/k8s.io/client-go/tools/events/event_recorder_test.go
@@ -0,0 +1,240 @@
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package events
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/google/go-cmp/cmp"
+
+	v1 "k8s.io/api/core/v1"
+	networkingv1 "k8s.io/api/networking/v1"
+
+	eventsv1 "k8s.io/api/events/v1"
+	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/watch"
+	testclocks "k8s.io/utils/clock/testing"
+)
+
+func TestEventf(t *testing.T) {
+	// use a fixed time for generated names that depend on the unix timestamp
+	fakeClock := testclocks.NewFakeClock(time.Date(2023, time.January, 1, 12, 0, 0, 0, time.UTC))
+
+	testCases := []struct {
+		desc          string
+		regarding     runtime.Object
+		related       runtime.Object
+		eventtype     string
+		reason        string
+		action        string
+		note          string
+		args          []interface{}
+		expectedEvent *eventsv1.Event
+	}{
+		{
+			desc: "normal event",
+			regarding: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "pod1",
+					Namespace: "ns1",
+					UID:       "12345",
+				},
+			},
+			eventtype: "Normal",
+			reason:    "Started",
+			action:    "starting",
+			note:      "Pod started",
+			expectedEvent: &eventsv1.Event{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fmt.Sprintf("pod1.%x", fakeClock.Now().UnixNano()),
+					Namespace: "ns1",
+				},
+				Regarding: v1.ObjectReference{
+					Kind:       "Pod",
+					Name:       "pod1",
+					Namespace:  "ns1",
+					UID:        "12345",
+					APIVersion: "v1",
+				},
+				Type:                "Normal",
+				Reason:              "Started",
+				Action:              "starting",
+				Note:                "Pod started",
+				ReportingController: "c1",
+				ReportingInstance:   "i1",
+			},
+		},
+		{
+			desc: "event with related object and format args",
+			regarding: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "pod1",
+					Namespace: "ns1",
+					UID:       "12345",
+				},
+			},
+			related: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node1",
+					UID:  "67890",
+				},
+			},
+			eventtype: "Warning",
+			reason:    "FailedScheduling",
+			action:    "scheduling",
+
+			note: "Pod failed to schedule on %s: %s",
+			args: []interface{}{"node1", "not enough resources"},
+			expectedEvent: &eventsv1.Event{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      fmt.Sprintf("pod1.%x", fakeClock.Now().UnixNano()),
+					Namespace: "ns1",
+				},
+				Regarding: v1.ObjectReference{
+					Kind:       "Pod",
+					Name:       "pod1",
+					Namespace:  "ns1",
+					UID:        "12345",
+					APIVersion: "v1",
+				},
+				Related: &v1.ObjectReference{
+					Kind:       "Node",
+					Name:       "node1",
+					UID:        "67890",
+					APIVersion: "v1",
+				},
+				Type:                "Warning",
+				Reason:              "FailedScheduling",
+				Action:              "scheduling",
+				Note:                "Pod failed to schedule on node1: not enough resources",
+				ReportingController: "c1",
+				ReportingInstance:   "i1",
+			},
+		}, {
+			desc: "event with invalid Event name",
+			regarding: &networkingv1.IPAddress{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "2001:db8::123",
+					UID:  "12345",
+				},
+			},
+			eventtype: "Warning",
+			reason:    "IPAddressNotAllocated",
+			action:    "IPAddressAllocation",
+			note:      "Service default/test appears to have leaked",
+
+			expectedEvent: &eventsv1.Event{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "default",
+				},
+				Regarding: v1.ObjectReference{
+					Kind:       "IPAddress",
+					Name:       "2001:db8::123",
+					UID:        "12345",
+					APIVersion: "networking.k8s.io/v1",
+				},
+				Type:                "Warning",
+				Reason:              "IPAddressNotAllocated",
+				Action:              "IPAddressAllocation",
+				Note:                "Service default/test appears to have leaked",
+				ReportingController: "c1",
+				ReportingInstance:   "i1",
+			},
+		}, {
+			desc: "large event name",
+			regarding: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      strings.Repeat("x", utilvalidation.DNS1123SubdomainMaxLength*4),
+					Namespace: "ns1",
+					UID:       "12345",
+				},
+			},
+			eventtype: "Normal",
+			reason:    "Started",
+			action:    "starting",
+			note:      "Pod started",
+			expectedEvent: &eventsv1.Event{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns1",
+				},
+				Regarding: v1.ObjectReference{
+					Kind:       "Pod",
+					Name:       strings.Repeat("x", utilvalidation.DNS1123SubdomainMaxLength*4),
+					Namespace:  "ns1",
+					UID:        "12345",
+					APIVersion: "v1",
+				},
+				Type:                "Normal",
+				Reason:              "Started",
+				Action:              "starting",
+				Note:                "Pod started",
+				ReportingController: "c1",
+				ReportingInstance:   "i1",
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			broadcaster := watch.NewBroadcaster(1, watch.WaitIfChannelFull)
+			recorder := &recorderImpl{
+				scheme:              runtime.NewScheme(),
+				reportingController: "c1",
+				reportingInstance:   "i1",
+				Broadcaster:         broadcaster,
+				clock:               fakeClock,
+			}
+
+			if err := v1.AddToScheme(recorder.scheme); err != nil {
+				t.Fatal(err)
+			}
+			if err := networkingv1.AddToScheme(recorder.scheme); err != nil {
+				t.Fatal(err)
+			}
+			ch, err := broadcaster.Watch()
+			if err != nil {
+				t.Fatal(err)
+			}
+			recorder.Eventf(tc.regarding, tc.related, tc.eventtype, tc.reason, tc.action, tc.note, tc.args...)
+
+			select {
+			case event := <-ch.ResultChan():
+				actualEvent := event.Object.(*eventsv1.Event)
+				if errs := apimachineryvalidation.NameIsDNSSubdomain(actualEvent.Name, false); len(errs) > 0 {
+					t.Errorf("Event Name = %s; not a valid name: %v", actualEvent.Name, errs)
+				} // Overwrite fields that are not relevant for comparison
+				tc.expectedEvent.EventTime = actualEvent.EventTime
+				// invalid event names generate random names
+				if tc.expectedEvent.Name == "" {
+					actualEvent.Name = ""
+				}
+				if diff := cmp.Diff(tc.expectedEvent, actualEvent); diff != "" {
+					t.Errorf("Unexpected event diff (-want, +got):\n%s", diff)
+				}
+			case <-time.After(time.Second):
+				t.Errorf("Timeout waiting for event")
+			}
+
+		})
+	}
+}
diff --git a/staging/src/k8s.io/client-go/tools/record/util/util_test.go b/staging/src/k8s.io/client-go/tools/record/util/util_test.go
new file mode 100644
index 0000000000000..d6be791eb5f87
--- /dev/null
+++ b/staging/src/k8s.io/client-go/tools/record/util/util_test.go
@@ -0,0 +1,72 @@
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"strings"
+	"testing"
+
+	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
+)
+
+func TestGenerateEventName(t *testing.T) {
+	timestamp := int64(105999103295324396)
+	testCases := []struct {
+		name     string
+		refName  string
+		expected string
+	}{
+		{
+			name:     "valid name",
+			refName:  "test-pod",
+			expected: "test-pod.178959f726d80ec",
+		},
+		{
+			name:    "invalid name - too long",
+			refName: strings.Repeat("x", 300),
+		},
+		{
+			name:    "invalid name - upper case",
+			refName: "test.POD",
+		},
+		{
+			name:    "invalid name - special chars",
+			refName: "test.pod/invalid!chars?",
+		},
+		{
+			name:    "invalid name - special chars and non alphanumeric starting character",
+			refName: "--test.pod/invalid!chars?",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := GenerateEventName(tc.refName, timestamp)
+
+			if errs := apimachineryvalidation.NameIsDNSSubdomain(actual, false); len(errs) > 0 {
+				t.Errorf("generateEventName(%s) = %s; not a valid name: %v", tc.refName, actual, errs)
+
+			}
+
+			if tc.expected != "" && (actual != tc.expected) {
+				t.Errorf("generateEventName(%s) returned %s expected %s", tc.refName, actual, tc.expected)
+			}
+
+		})
+
+	}
+}