Event recorder should enfoce API conventions for Event Reason

[derekwaynecarr]

As code produces events, we are not always enforcing our own API conventions for EventNames

An example PR where we had to make a change:
#14110

We should enforce proper event naming conventions as defined here:
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#events

The best way to stop this in the future is for the Event recorder to error/warn when an invalid event name is provided.

[derekwaynecarr]

fyi @bgrant0607 - assume you are agreeable to code enforcing our api-conventions for events?

[derekwaynecarr]

The validation for an Event should also enforce the naming convention:

https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/events.go#L26

[simon3z]

@derekwaynecarr given the description of reason:

'reason' is the reason this event is generated. 'reason' should be short and unique; it should be in UpperCamelCase format (starting with a capital letter). "reason" will be used to automate handling of events, so imagine people writing switch statements to handle them. You want to make that easy.

I'd go with something more structured as:

type EventReason string

const (
    /* Some meaningful explanation about CIDRNotAvailable */
    CIDRNotAvailable EventReason = "CIDRNotAvailable"

    /* Some meaningful explanation about CIDRAssignmentFailed */
    CIDRAssignmentFailed EventReason = "CIDRAssignmentFailed"

    ...
)

func (recorder *recorderImpl) Eventf(object runtime.Object, reason EventReason, ...) {
    ...
}

What do you think? (cc @bgrant0607 )

[jiangyaoguo]

@derekwaynecarr Reason Should be in CamelCase. At most we can validate it's one word begin with capital letter. Is this enough for validate reason if we want do this in event recorder?

[alex-mohr]

@lavalamp is this team/CSI? or you as interested events guru?

[bgrant0607]

Strengthening validation would be a breaking API change. As much as I'd like to enforce the conventions, this can't be the way to do it. Or maybe the check could be enabled only during tests.

Could we write a code validator instead?

It is also true that it seems hard to make the check stronger than just capitalization, but not capitalizing the first letter is one of the most common inconsistencies.

[bgrant0607]

Defining constants as in @simon3z's example also SGTM.

[pmorie]

@kubernetes/rh-cluster-infra @kubernetes/rh-ux

[simon3z]

Defining constants as in @simon3z's example also SGTM.

The advantage of that is that I suppose you can use reflection to validate the entries (list all the EventReason constants in the record package and check for no spaces, first capital letter, etc.)

Anyway having all the reasons in one place already would allow people to easily review that part of the code and spot inconsistencies.

[liggitt]

+1 for validation of whatever sort we can do in a compatible way

-1 for centralized reasons. I shouldn't have to change core event code to report new event reasons for a new component. Remember that event reporters might not even live in the kubernetes code base

[lavalamp]

Agree.

On Thu, Sep 17, 2015 at 3:18 PM, Jordan Liggitt notifications@github.com
wrote:

+1 for validation of whatever sort we can do in a compatible way

-1 for centralized reasons. I shouldn't have to change core event code to
report new event reasons for a new component. Remember that event reporters
might not even live in the kubernetes code base

—
Reply to this email directly or view it on GitHub
#14111 (comment)
.

[jiangyaoguo]

What about add a optional validation in apiserver and open it during tests？

[alex-mohr]

Re: centralized reasons, there's a strong benefit to people building automated systems that are clients of k8s to have a reasonably-explicit list of such things, rather than having to discover them at runtime in production when a new one starts being reported.

Re: event reporters outside the codebase, presumably some form of namespacing could allow them to report common established errors but define new ones where absolutely necessary.

And it may be instructive to look at http errors with a numeric generic-code plus more detailed text for inspiration?

[lavalamp]

@alex-mohr the problem with centralized reasons is that it is a recipe for brittle clients: the act of adding a reason becomes an api-breaking change. Event reasons are very explicitly not centralized for this reason.

[0xmichalis]

@kubernetes/sig-api-machinery-misc

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

[bgrant0607]

cc @gmarek

[gmarek]

/remove-lifecycle state
/lifecycle frozen

[gmarek]

@yastij - I wrote a bunch of validation logic for Events v2 in the initial PR, but it may be worth revisiting to check if all fields are properly validated, according to kubernetes/community#1659.

[yastij]

@gmarek - I'll take a look

[nikhita]

/remove-lifecycle stale

[sftim]

Strengthening validation would be a breaking API change. As much as I'd like to enforce the conventions, this can't be the way to do it. Or maybe the check could be enabled only during tests.

Could we write a code validator instead?

It is also true that it seems hard to make the check stronger than just capitalization, but not capitalizing the first letter is one of the most common inconsistencies.

We now have warnings and a framework for instrumentation, so maybe there's something we can revisit here.