-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
Add EventRecorder abstraction. #653
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
02788d0
Add EventRecorder abstraction.
ce77023
Formatting.
9bfc3ee
Use proper error message
a11bc9a
Rename module to `events`.
af4e0a6
Add missing `secondary_object` field.
0bd8310
Add notes detailing the name of the API field, for extra clarity.
b89f532
Keep the Display representation free of details already included in t…
04f0f39
Add pointers on how to get the controller pod name.
6f18276
Collapse small modules.
d1c21fc
Formatting.
7851684
Use secondary object in EventRecorder.
ebb354c
Add validation for event reason.
77e2ce1
Add validation for event action.
28467a0
Add validation for event note.
c2eba9a
Formatting.
495aea4
Fix note doc example.
a389fac
Change field names.
9c6e2b3
Typo.
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,346 @@ | ||
use k8s_openapi::api::core::v1::ObjectReference; | ||
use std::{convert::TryFrom, fmt::Formatter}; | ||
|
||
/// Required information to publish a new event via [`EventRecorder::publish`]. | ||
/// | ||
/// [`EventRecorder::publish`]: crate::events::EventRecorder::publish | ||
pub struct NewEvent { | ||
/// The action that was taken (either successfully or unsuccessfully) against | ||
/// the references object. | ||
/// | ||
/// `action` must be machine-readable. | ||
pub action: EventAction, | ||
/// The reason explaining why the `action` was taken. | ||
/// | ||
/// `reason` must be human-readable. | ||
pub reason: EventReason, | ||
/// A optional description of the status of the `action`. | ||
/// | ||
/// `note` must be human-readable. | ||
pub note: Option<EventNote>, | ||
/// The event severity. | ||
pub event_type: EventType, | ||
/// Some events are emitted for actions that affect multiple objects. | ||
/// `secondary_object` can be populated to capture this detail. | ||
/// | ||
/// For example: the event concerns a `Deployment` and it | ||
/// affects the current `ReplicaSet` underneath it. | ||
/// You would therefore populate `secondary_object` using the object | ||
/// reference of the `ReplicaSet`. | ||
/// | ||
/// Set `secondary_object` to `None`, instead, if the event | ||
/// affects only the object whose reference you passed | ||
/// to [`EventRecorder::new`]. | ||
/// | ||
/// # Naming note | ||
/// | ||
/// `secondary_object` is mapped to `related` in | ||
/// [`Events API`](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#event-v1-events-k8s-io). | ||
/// | ||
/// [`EventRecorder::new`]: crate::events::EventRecorder::new | ||
pub secondary_object: Option<ObjectReference>, | ||
} | ||
|
||
/// The event severity or type. | ||
#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] | ||
pub enum EventType { | ||
/// An event took place - nothing to worry about. | ||
Normal, | ||
/// Something is not working as expected - it might be worth to have a look. | ||
Warning, | ||
} | ||
|
||
/// Details about the event emitter. | ||
/// | ||
/// ```rust | ||
/// use std::convert::TryInto; | ||
/// use kube_runtime::events::EventSource; | ||
/// | ||
/// let event_source = EventSource { | ||
/// controller_pod: "my-awesome-controller-abcdef".try_into().unwrap(), | ||
/// controller: "my-awesome-controller".into(), | ||
/// }; | ||
/// ``` | ||
#[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
pub struct EventSource { | ||
/// The name of the controller publishing the event. | ||
/// | ||
/// E.g. `my-awesome-controller`. | ||
/// | ||
/// # Naming note | ||
/// | ||
/// `controller_name` is mapped to `reportingController` in | ||
/// [`Events API`](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#event-v1-events-k8s-io). | ||
pub controller: String, | ||
/// The name of the controller pod publishing the event. | ||
/// | ||
/// E.g. `my-awesome-controller-abcdef`. | ||
/// | ||
/// The name of the controller pod can be retrieved using Kubernetes' API or | ||
/// it can be injected as an environment variable using | ||
/// | ||
/// ```yaml | ||
/// env: | ||
/// - name: CONTROLLER_POD_NAME | ||
/// valueFrom: | ||
/// fieldRef: | ||
/// fieldPath: metadata.name | ||
/// ``` | ||
/// | ||
/// in the manifest of your controller. | ||
/// | ||
/// # Naming note | ||
/// | ||
/// `controller_pod_name` is mapped to `reportingInstance` in | ||
/// [`Events API`](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#event-v1-events-k8s-io). | ||
pub controller_pod: ControllerPodName, | ||
} | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq, Hash)] | ||
/// The name of the controller pod publishing the event. | ||
/// | ||
/// ```rust | ||
/// use std::convert::TryInto; | ||
/// use kube_runtime::events::ControllerPodName; | ||
/// | ||
/// let controller_pod_name: ControllerPodName = "my-awesome-controller-abcdef".try_into().unwrap(); | ||
/// ``` | ||
/// | ||
/// It must be: | ||
/// | ||
/// - shorter than 128 characters. | ||
pub struct ControllerPodName(String); | ||
|
||
impl TryFrom<&str> for ControllerPodName { | ||
type Error = ControllerPodNameParsingError; | ||
|
||
fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
Self::try_from(value.to_string()) | ||
} | ||
} | ||
|
||
impl TryFrom<String> for ControllerPodName { | ||
type Error = ControllerPodNameParsingError; | ||
|
||
fn try_from(v: String) -> Result<Self, Self::Error> { | ||
// Limit imposed by Kubernetes' API | ||
let n_chars = v.chars().count(); | ||
if n_chars > 128 { | ||
Err(ControllerPodNameParsingError { | ||
controller_pod_name: v, | ||
}) | ||
} else { | ||
Ok(Self(v)) | ||
} | ||
} | ||
} | ||
|
||
impl AsRef<str> for ControllerPodName { | ||
fn as_ref(&self) -> &str { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl Into<String> for ControllerPodName { | ||
fn into(self) -> String { | ||
self.0 | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
pub struct ControllerPodNameParsingError { | ||
controller_pod_name: String, | ||
} | ||
|
||
impl std::fmt::Display for ControllerPodNameParsingError { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
write!(f, "The controller pod name must be shorter than 128 characters.") | ||
} | ||
} | ||
|
||
impl std::error::Error for ControllerPodNameParsingError {} | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq, Hash)] | ||
/// The reason for an action that led to a published event. | ||
/// | ||
/// ```rust | ||
/// use std::convert::TryInto; | ||
/// use kube_runtime::events::EventReason; | ||
/// | ||
/// let reason: EventReason = "Scheduling".try_into().unwrap(); | ||
/// ``` | ||
/// | ||
/// It must be: | ||
/// | ||
/// - shorter than 128 characters. | ||
pub struct EventReason(String); | ||
|
||
impl TryFrom<&str> for EventReason { | ||
type Error = EventReasonParsingError; | ||
|
||
fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
Self::try_from(value.to_string()) | ||
} | ||
} | ||
|
||
impl TryFrom<String> for EventReason { | ||
type Error = EventReasonParsingError; | ||
|
||
fn try_from(v: String) -> Result<Self, Self::Error> { | ||
// Limit imposed by Kubernetes' API | ||
let n_chars = v.chars().count(); | ||
if n_chars > 128 { | ||
Err(EventReasonParsingError { reason: v }) | ||
} else { | ||
Ok(Self(v)) | ||
} | ||
} | ||
} | ||
|
||
impl AsRef<str> for EventReason { | ||
fn as_ref(&self) -> &str { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl Into<String> for EventReason { | ||
fn into(self) -> String { | ||
self.0 | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
pub struct EventReasonParsingError { | ||
reason: String, | ||
} | ||
|
||
impl std::fmt::Display for EventReasonParsingError { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
write!(f, "The reason for an event must be shorter than 128 characters.") | ||
} | ||
} | ||
|
||
impl std::error::Error for EventReasonParsingError {} | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq, Hash)] | ||
/// The action taken by the controller that led to a published event. | ||
/// | ||
/// ```rust | ||
/// use std::convert::TryInto; | ||
/// use kube_runtime::events::EventAction; | ||
/// | ||
/// let reason: EventAction = "Pulling".try_into().unwrap(); | ||
/// ``` | ||
/// | ||
/// It must be: | ||
/// | ||
/// - shorter than 128 characters. | ||
pub struct EventAction(String); | ||
|
||
impl TryFrom<&str> for EventAction { | ||
type Error = EventActionParsingError; | ||
|
||
fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
Self::try_from(value.to_string()) | ||
} | ||
} | ||
|
||
impl TryFrom<String> for EventAction { | ||
type Error = EventActionParsingError; | ||
|
||
fn try_from(v: String) -> Result<Self, Self::Error> { | ||
// Limit imposed by Kubernetes' API | ||
let n_chars = v.chars().count(); | ||
if n_chars > 128 { | ||
Err(EventActionParsingError { action: v }) | ||
} else { | ||
Ok(Self(v)) | ||
} | ||
} | ||
} | ||
|
||
impl AsRef<str> for EventAction { | ||
fn as_ref(&self) -> &str { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl Into<String> for EventAction { | ||
fn into(self) -> String { | ||
self.0 | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
pub struct EventActionParsingError { | ||
action: String, | ||
} | ||
|
||
impl std::fmt::Display for EventActionParsingError { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
write!(f, "The action for an event must be shorter than 128 characters.") | ||
} | ||
} | ||
|
||
impl std::error::Error for EventActionParsingError {} | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq, Hash)] | ||
/// The human-readable message attached to a published event. | ||
/// | ||
/// ```rust | ||
/// use std::convert::TryInto; | ||
/// use kube_runtime::events::EventNote; | ||
/// | ||
/// let note: EventNote = "Pulling `nginx` Docker image from DockerHub.".try_into().unwrap(); | ||
/// ``` | ||
/// | ||
/// It must be: | ||
/// | ||
/// - smaller than 1 kilobyte. | ||
pub struct EventNote(String); | ||
|
||
impl TryFrom<&str> for EventNote { | ||
type Error = EventNoteParsingError; | ||
|
||
fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
Self::try_from(value.to_string()) | ||
} | ||
} | ||
|
||
impl TryFrom<String> for EventNote { | ||
type Error = EventNoteParsingError; | ||
|
||
fn try_from(v: String) -> Result<Self, Self::Error> { | ||
// Limit imposed by Kubernetes' API | ||
if v.len() > 1024 { | ||
Err(Self::Error { note: v }) | ||
} else { | ||
Ok(Self(v)) | ||
} | ||
} | ||
} | ||
|
||
impl AsRef<str> for EventNote { | ||
fn as_ref(&self) -> &str { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl Into<String> for EventNote { | ||
fn into(self) -> String { | ||
self.0 | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
pub struct EventNoteParsingError { | ||
note: String, | ||
} | ||
|
||
impl std::fmt::Display for EventNoteParsingError { | ||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
write!(f, "The note for an event must be smaller than 1 kilobyte.") | ||
} | ||
} | ||
|
||
impl std::error::Error for EventNoteParsingError {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pub use event::{ControllerPodName, EventAction, EventNote, EventReason, EventSource, EventType, NewEvent}; | ||
pub use recorder::EventRecorder; | ||
|
||
mod event; | ||
mod recorder; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I am slightly unsure of whether we should provide this type of convenience validation here. On one hand it is nice and can catch things early, but on the other hand we have no way to know to enforce that the constants we put inside our code matches the kubernetes source (and i would rather rely on the api-server rejecting this, than reject things that are valid in the future).
In this particular case as well; if people get the pod name from the downward api (as in your documentation), it would be impossible to get an illegal name here, so I'm leaning towards just having an unchecked
String
here.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.
That said, it's nice for users to know as they are programming this. The event reason is very helpful for the user to know.
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.
This might be a question for maintainers for later on how we can do better client-side validation (without it getting out of date). For now, happy to leave this in. It's a nice convenience.
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.
Ideally this validation should live inside
k8s_openapi
, so that it can be associated to a specific version of the API server and can be changed to match when new versions are released.Happy to remove if you prefer to delegate to k8s API server.
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.
(Or Kubernetes packages up its validation stuff into a target that can be bound against multiple programming languages, ensuring consistent and up-to-date validation across the entire ecosystem for each version. Can a man dream?)
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.
The sources that's used to generate the structs unfortunately does not have the information we need to do any type of newtype magic inside
k8s-openapi
ork8s-pb
. So I think it's going to be a best-effort setup either way.In this case, we are presenting an api on top of core::v1::Event though, so it's probably better to leave the validation herein.
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.
Then this is ready to go 😁
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.
Thank you very much! Merged it in. Started a discussion internally around the future of client-side validation in #654
Will otherwise release this in the next version (after the super-crate issue gets resolved I am thinking).