'new' Event namespace validate failed #100125

h4ghhh · 2021-03-11T09:28:53Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

In events.k8s.io/v1, if involvedObject.namespace is "" , type Event's validation will fail.
When event.InvolvedObject.Namespace is "", event.Namespace is set "default", but compared with "kube-system".

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
	}
}

Which issue(s) this PR fixes:

Fixes # #100119

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kube-apiserver: events created via the `events.k8s.io` API group for cluster-scoped objects are now permitted in the default namespace as well for compatibility with events clients and the `v1` API

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/383-new-event-api-ga-graduation

k8s-ci-robot · 2021-03-11T09:29:02Z

Hi @h4ghhh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

liggitt · 2021-03-12T01:39:55Z

pkg/apis/core/validation/events.go

@@ -140,7 +140,7 @@ func legacyValidateEvent(event *core.Event) field.ErrorList {
 		}

 	} else {
-		if len(event.InvolvedObject.Namespace) == 0 && event.Namespace != metav1.NamespaceSystem {
+		if len(event.InvolvedObject.Namespace) == 0 && event.Namespace != metav1.NamespaceDefault {


/hold

should we fix the event producer? Won't this break existing clients that are currently working?

/hold

should we fix the event producer? Won't this break existing clients that are currently working?

No matter 'new' or 'old', the Event's default namespace is always 'default'.
Refer to line 132, where compared with NamespaceDefault.
These codes are introduced as new type Event check. If existing clients use events.k8s.io/v1 rather than core/v1, has this problem long been exposed?

refer to https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/383-new-event-api-ga-graduation
Namespace in which Event will live will be equal to

Namespace of Regarding object, if it's namespaced,

NamespaceSystem, if it's not.

I decide to fix event producer. Thx.

fixing the producer is the correct approach, but this PR is still trying to change API validation

I created an integration test that tries to create events for cluster-scoped objects against kube-system and default namespaces, via v1 Events and events.k8s.io/v1 Events

diff --git a/test/integration/events/events_test.go b/test/integration/events/events_test.go index bb8830076ef..3f931bb78dc 100644 --- a/test/integration/events/events_test.go +++ b/test/integration/events/events_test.go @@ -18,10 +18,12 @@ package events import ( "context" + "strings" "testing" "time" v1 "k8s.io/api/core/v1" + eventsv1 "k8s.io/api/events/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientset "k8s.io/client-go/kubernetes" @@ -92,4 +94,84 @@ func TestEventCompatibility(t *testing.T) { if err != nil { t.Fatalf("unexpected err: %v", err) } + + coreevents := []*v1.Event{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pass-core-kube-system-cluster-scoped", + Namespace: "kube-system", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + InvolvedObject: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "fail-core-default-cluster-scoped", + Namespace: "default", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + InvolvedObject: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + } + for _, e := range coreevents { + t.Run(e.Name, func(t *testing.T) { + _, err := client.CoreV1().Events(e.Namespace).Create(context.TODO(), e, metav1.CreateOptions{}) + if err == nil && !strings.HasPrefix(e.Name, "pass-") { + t.Fatalf("unexpected pass") + } + if err != nil && !strings.HasPrefix(e.Name, "fail-") { + t.Fatalf("unexpected error: %v", err) + } + }) + } + + v1events := []*eventsv1.Event{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pass-events-kube-system-cluster-scoped", + Namespace: "kube-system", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + Regarding: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "fail-events-default-cluster-scoped", + Namespace: "default", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + Regarding: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + } + for _, e := range v1events { + t.Run(e.Name, func(t *testing.T) { + _, err := client.EventsV1().Events(e.Namespace).Create(context.TODO(), e, metav1.CreateOptions{}) + if err == nil && !strings.HasPrefix(e.Name, "pass-") { + t.Fatalf("unexpected pass") + } + if err != nil && !strings.HasPrefix(e.Name, "fail-") { + t.Fatalf("unexpected error: %v", err) + } + }) + } }

Running that against 1.19, 1.20, 1.21, and 1.22 showed that events for cluster-scoped objects could only be created in the kube-system namespace in 1.19+

Revert the changes to this file, add the integration test, and fix the namespace used when recording events for cluster-scoped objects in the event producer

oh... that test is setting EventTime unnecessarily when creating corev1 events

This updated version of the test shows that you could create events for cluster-scoped objects in either the kube-system or default namespaces depending on whether you specific event time:

diff --git a/test/integration/events/events_test.go b/test/integration/events/events_test.go index bb8830076ef..54f4001fa34 100644 --- a/test/integration/events/events_test.go +++ b/test/integration/events/events_test.go @@ -18,10 +18,12 @@ package events import ( "context" + "strings" "testing" "time" v1 "k8s.io/api/core/v1" + eventsv1 "k8s.io/api/events/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientset "k8s.io/client-go/kubernetes" @@ -92,4 +94,108 @@ func TestEventCompatibility(t *testing.T) { if err != nil { t.Fatalf("unexpected err: %v", err) } + + coreevents := []*v1.Event{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "fail-core-kube-system-cluster-scoped-no-time", + Namespace: "kube-system", + }, + Type: "Warning", + Action: "myaction", + Reason: "myreason", + InvolvedObject: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pass-core-kube-system-cluster-scoped-with-time", + Namespace: "kube-system", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + InvolvedObject: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pass-core-default-cluster-scoped-no-time", + Namespace: "default", + }, + Type: "Warning", + Action: "myaction", + Reason: "myreason", + InvolvedObject: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "fail-core-default-cluster-scoped-with-time", + Namespace: "default", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + InvolvedObject: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + } + for _, e := range coreevents { + t.Run(e.Name, func(t *testing.T) { + _, err := client.CoreV1().Events(e.Namespace).Create(context.TODO(), e, metav1.CreateOptions{}) + if err == nil && !strings.HasPrefix(e.Name, "pass-") { + t.Errorf("unexpected pass") + } + if err != nil && !strings.HasPrefix(e.Name, "fail-") { + t.Fatalf("unexpected error: %v", err) + } + }) + } + + v1events := []*eventsv1.Event{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pass-events-kube-system-cluster-scoped", + Namespace: "kube-system", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + Regarding: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "fail-events-default-cluster-scoped", + Namespace: "default", + }, + EventTime: metav1.NewMicroTime(time.Now()), + Type: "Warning", + Action: "myaction", + Reason: "myreason", + Regarding: v1.ObjectReference{Name: "foo", Namespace: "", Kind: "Node", APIVersion: "v1"}, + ReportingController: "mycontroller", + ReportingInstance: "myinstance", + }, + } + for _, e := range v1events { + t.Run(e.Name, func(t *testing.T) { + _, err := client.EventsV1().Events(e.Namespace).Create(context.TODO(), e, metav1.CreateOptions{}) + if err == nil && !strings.HasPrefix(e.Name, "pass-") { + t.Errorf("unexpected pass") + } + if err != nil && !strings.HasPrefix(e.Name, "fail-") { + t.Fatalf("unexpected error: %v", err) + } + }) + } }

And #88815 in 1.18 made both event reporters collapse onto the default namespace.

h4ghhh · 2021-03-12T01:41:03Z

/sig api-machinery

jiahuif · 2021-03-13T17:34:18Z

/ok-to-test

h4ghhh · 2021-03-14T13:40:19Z

refer to https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/383-new-event-api-ga-graduation
Namespace in which Event will live will be equal to

Namespace of Regarding object, if it's namespaced,
NamespaceSystem, if it's not.

I decide to fix event producer. Thx.

h4ghhh · 2021-03-15T02:49:49Z

I'm modifying kube-controller-manger to use new Event API, but this problem must be solved first.
Please check if there is anything that not has been considered.

/cc @kubernetes/api-reviewers

liggitt · 2021-03-15T15:10:40Z

/assign @wojtek-t

I think this change looks good, but we need tests exercising the following:

creation of an events.k8s.io/v1 event for a cluster-scoped object that would have caught this issue
a test ensuring kubectl describe on a built-in cluster-scoped object finds events recorded by both the old and new events client

fejta · 2021-03-15T17:29:56Z

/uncc

fedebongio · 2021-03-16T20:06:14Z

/triage accepted

liggitt · 2021-09-15T12:36:38Z

/hold

actually, can you squash this to a single commit before merge? it will make cherry-picks easier

liggitt · 2021-09-16T01:39:01Z

/retest

h4ghhh · 2021-09-16T01:52:35Z

/hold

actually, can you squash this to a single commit before merge? it will make cherry-picks easier

ok

wojtek-t · 2021-09-16T08:49:25Z

/lgtm

/hold cancel

h4ghhh · 2021-09-16T10:24:48Z

/retest

liggitt · 2021-09-16T15:30:47Z

please open cherry-picks against 1.20-1.22

h4ghhh · 2021-09-17T07:59:12Z

please open cherry-picks against 1.20-1.22

ok

…25-upstream-release-1.20 Automated cherry pick of #100125: 'New' Event namespace validate failed

…25-upstream-release-1.22 Automated cherry pick of #100125: 'New' Event namespace validate failed

…25-upstream-release-1.21 Automated cherry pick of #100125: 'New' Event namespace validate failed

k8s-ci-robot requested review from fejta and krousey March 11, 2021 09:29

h4ghhh mentioned this pull request Mar 12, 2021

Events in events.k8s.io/v1 validate failed #100119

Closed

liggitt reviewed Mar 12, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2021

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 12, 2021

h4ghhh requested a review from liggitt March 12, 2021 05:39

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 13, 2021

k8s-ci-robot assigned wojtek-t Mar 15, 2021

k8s-ci-robot removed the request for review from fejta March 15, 2021 17:29

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Mar 16, 2021

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 15, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2021

'New' Event namespace validate failed

a2ef4d3

h4ghhh force-pushed the event_ns_validate branch from f87578f to a2ef4d3 Compare September 16, 2021 01:49

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 16, 2021

k8s-ci-robot merged commit 0734820 into kubernetes:master Sep 16, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 16, 2021

liggitt moved this from In progress to API review completed, 1.23 in API Reviews Sep 16, 2021

ccampo133 mentioned this pull request Sep 16, 2021

elbv2 multicert ccampo133/kubernetes#1

Closed

This was referenced Sep 17, 2021

Automated cherry pick of #100125: 'New' Event namespace validate failed #105087

Merged

Automated cherry pick of #100125: 'New' Event namespace validate failed #105088

Merged

Automated cherry pick of #100125: 'New' Event namespace validate failed #105089

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 19, 2021

Merge pull request #105087 from h4ghhh/automated-cherry-pick-of-#1001…

2624cc6

…25-upstream-release-1.20 Automated cherry pick of #100125: 'New' Event namespace validate failed

k8s-ci-robot added a commit that referenced this pull request Sep 19, 2021

Merge pull request #105089 from h4ghhh/automated-cherry-pick-of-#1001…

ee7ad32

…25-upstream-release-1.22 Automated cherry pick of #100125: 'New' Event namespace validate failed

k8s-ci-robot added a commit that referenced this pull request Sep 19, 2021

Merge pull request #105088 from h4ghhh/automated-cherry-pick-of-#1001…

baa4fc5

…25-upstream-release-1.21 Automated cherry pick of #100125: 'New' Event namespace validate failed

ibabou pushed a commit to ibabou/kubernetes that referenced this pull request Sep 28, 2021

'New' Event namespace validate failed (kubernetes#100125)

cdc1827

This was referenced Nov 12, 2021

[release-4.9] Bug 2006290: Fix up event recorder usage (again) openshift/sdn#357

Merged

WIP: Update from core.Event to events.Events ovn-org/ovn-kubernetes#2632

Closed

h4ghhh mentioned this pull request Dec 5, 2021

REQUEST: New membership for h4ghhh kubernetes/org#3128

Closed

7 tasks

louismrose mentioned this pull request Apr 8, 2022

Publishing events for cluster scoped resources broken for Kube <1.23 kube-rs/kube#870

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

'new' Event namespace validate failed #100125

'new' Event namespace validate failed #100125

h4ghhh commented Mar 11, 2021 •

edited by liggitt

k8s-ci-robot commented Mar 11, 2021

liggitt Mar 12, 2021

h4ghhh Mar 12, 2021

h4ghhh Mar 15, 2021 •

edited

liggitt Sep 3, 2021

liggitt Sep 3, 2021

liggitt Sep 3, 2021

liggitt Sep 3, 2021

h4ghhh commented Mar 12, 2021

jiahuif commented Mar 13, 2021

h4ghhh commented Mar 14, 2021 •

edited

h4ghhh commented Mar 15, 2021 •

edited

liggitt commented Mar 15, 2021

fejta commented Mar 15, 2021

fedebongio commented Mar 16, 2021

liggitt commented Sep 15, 2021

liggitt commented Sep 16, 2021

h4ghhh commented Sep 16, 2021

wojtek-t commented Sep 16, 2021

h4ghhh commented Sep 16, 2021

liggitt commented Sep 16, 2021

h4ghhh commented Sep 17, 2021

'new' Event namespace validate failed #100125

'new' Event namespace validate failed #100125

Conversation

h4ghhh commented Mar 11, 2021 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 11, 2021

liggitt Mar 12, 2021

Choose a reason for hiding this comment

h4ghhh Mar 12, 2021

Choose a reason for hiding this comment

h4ghhh Mar 15, 2021 • edited

Choose a reason for hiding this comment

liggitt Sep 3, 2021

Choose a reason for hiding this comment

liggitt Sep 3, 2021

Choose a reason for hiding this comment

liggitt Sep 3, 2021

Choose a reason for hiding this comment

liggitt Sep 3, 2021

Choose a reason for hiding this comment

h4ghhh commented Mar 12, 2021

jiahuif commented Mar 13, 2021

h4ghhh commented Mar 14, 2021 • edited

h4ghhh commented Mar 15, 2021 • edited

liggitt commented Mar 15, 2021

fejta commented Mar 15, 2021

fedebongio commented Mar 16, 2021

liggitt commented Sep 15, 2021

liggitt commented Sep 16, 2021

h4ghhh commented Sep 16, 2021

wojtek-t commented Sep 16, 2021

h4ghhh commented Sep 16, 2021

liggitt commented Sep 16, 2021

h4ghhh commented Sep 17, 2021

h4ghhh commented Mar 11, 2021 •

edited by liggitt

h4ghhh Mar 15, 2021 •

edited

h4ghhh commented Mar 14, 2021 •

edited

h4ghhh commented Mar 15, 2021 •

edited