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
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
pkg/apis/core/validation/events.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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+
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
+ }
+ })
+ }
}
There was a problem hiding this comment.
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.
/sig api-machinery |
/ok-to-test |
refer to https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/383-new-event-api-ga-graduation
I decide to fix event producer. Thx. |
I'm modifying kube-controller-manger to use new Event API, but this problem must be solved first. /cc @kubernetes/api-reviewers |
/assign @wojtek-t I think this change looks good, but we need tests exercising the following:
|
/uncc |
/triage accepted |
/hold actually, can you squash this to a single commit before merge? it will make cherry-picks easier |
/retest |
f87578f
to
a2ef4d3
Compare
ok |
/lgtm /hold cancel |
/retest |
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
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".
Which issue(s) this PR fixes:
Fixes # #100119
Special notes for your reviewer:
Does this PR introduce a user-facing change?
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