Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'new' Event namespace validate failed #100125

Merged
merged 1 commit into from Sep 16, 2021

Conversation

@h4ghhh
Copy link

@h4ghhh h4ghhh commented Mar 11, 2021

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
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 11, 2021

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.

Loading

@@ -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 {
Copy link
Member

@liggitt liggitt Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

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

Loading

Copy link
Author

@h4ghhh h4ghhh Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/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?

Loading

Copy link
Author

@h4ghhh h4ghhh Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@liggitt liggitt Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+

Loading

Copy link
Member

@liggitt liggitt Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@liggitt liggitt Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member

@liggitt liggitt Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@h4ghhh
Copy link
Author

@h4ghhh h4ghhh commented Mar 12, 2021

/sig api-machinery

Loading

@jiahuif
Copy link
Member

@jiahuif jiahuif commented Mar 13, 2021

/ok-to-test

Loading

@h4ghhh
Copy link
Author

@h4ghhh h4ghhh commented Mar 14, 2021

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.

Loading

@h4ghhh
Copy link
Author

@h4ghhh h4ghhh commented Mar 15, 2021

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

Loading

@liggitt
Copy link
Member

@liggitt liggitt commented Mar 15, 2021

/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

Loading

@fejta
Copy link
Contributor

@fejta fejta commented Mar 15, 2021

/uncc

Loading

@k8s-ci-robot k8s-ci-robot removed the request for review from fejta Mar 15, 2021
@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Mar 16, 2021

/triage accepted

Loading

@liggitt
Copy link
Member

@liggitt liggitt commented Sep 15, 2021

/priority important-soon

Loading

@liggitt
Copy link
Member

@liggitt liggitt commented Sep 15, 2021

/hold

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

Loading

@liggitt
Copy link
Member

@liggitt liggitt commented Sep 16, 2021

/retest

Loading

@h4ghhh h4ghhh force-pushed the event_ns_validate branch from f87578f to a2ef4d3 Sep 16, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 16, 2021
@h4ghhh
Copy link
Author

@h4ghhh h4ghhh commented Sep 16, 2021

/hold

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

ok

Loading

@wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Sep 16, 2021

/lgtm

/hold cancel

Loading

@h4ghhh
Copy link
Author

@h4ghhh h4ghhh commented Sep 16, 2021

/retest

Loading

@k8s-ci-robot k8s-ci-robot merged commit 0734820 into kubernetes:master Sep 16, 2021
15 checks passed
Loading
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 16, 2021
@liggitt liggitt moved this from In progress to API review completed, 1.23 in API Reviews Sep 16, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Sep 16, 2021

please open cherry-picks against 1.20-1.22

Loading

@h4ghhh
Copy link
Author

@h4ghhh h4ghhh commented Sep 17, 2021

please open cherry-picks against 1.20-1.22

ok

Loading

k8s-ci-robot added a commit that referenced this issue Sep 19, 2021
…25-upstream-release-1.20

Automated cherry pick of #100125: 'New' Event namespace validate failed
k8s-ci-robot added a commit that referenced this issue Sep 19, 2021
…25-upstream-release-1.22

Automated cherry pick of #100125: 'New' Event namespace validate failed
k8s-ci-robot added a commit that referenced this issue Sep 19, 2021
…25-upstream-release-1.21

Automated cherry pick of #100125: 'New' Event namespace validate failed
ibabou added a commit to ibabou/kubernetes that referenced this issue Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment