spike: create warning events for parsing errors #3088
Conversation
czeslavo
added
do not merge
internal/dataplane/kong_client.go
Outdated
| if errors := p.GetParsingErrors(); errors != nil { | ||
| c.createParsingErrorsEvents(errors) | ||
| c.prometheusMetrics.TranslationCount.With(prometheus.Labels{ | ||
| metrics.SuccessKey: metrics.SuccessFalse, | ||
| }).Inc() | ||
| c.logger.Debugf("%d translation errors occurred when building data-plane configuration", len(errors)) |
There was a problem hiding this comment.
In case any translation error is detected, we consider that a failed translation thus propagating the metric with SuccessFalse value. IMO it makes sense as that will allow setting up alerting on that.
| func (c *KongClient) createParsingErrorsEvents(errors []parser.ParsingError) { | ||
| const reason = "TranslationToKongConfigurationFailed" | ||
| for _, err := range errors { | ||
| for _, obj := range err.RelatedObjects() { | ||
| c.eventsRecorder.Event(obj, corev1.EventTypeWarning, reason, err.Reason()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
eventsRecorder creates Kubernetes Event object associated with the obj.
| func (c *parsingErrorsCollector) ParsingError(reason string, relatedObjects ...client.Object) { | ||
| c.errors = append(c.errors, NewParsingError(reason, relatedObjects...)) | ||
| } |
There was a problem hiding this comment.
ParsingError is used to gather parsing errors during the translation. It accepts related/affected objects regarding the error.
| type ParsingError struct { | ||
| relatedObjects []client.Object | ||
| reason string | ||
| } |
There was a problem hiding this comment.
Every parsing error has a human-readable reason string and related kubernetes objects.
There was a problem hiding this comment.
Apart from this deserving its own file I feel this is a good approach.
| @@ -144,7 +144,18 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic) e | |||
| if err != nil { | |||
| return fmt.Errorf("%f is not a valid number of seconds to the timeout config for the kong client: %w", c.ProxyTimeoutSeconds, err) | |||
| } | |||
| dataplaneClient, err := dataplane.NewKongClient(deprecatedLogger, timeoutDuration, c.IngressClassName, c.EnableReverseSync, c.SkipCACertificates, diagnostic, kongConfig) | |||
|
|
|||
| dataplaneEventRecorder := mgr.GetEventRecorderFor("kubernetes-ingress-controller-data-plane") | |||
There was a problem hiding this comment.
It's straightforward to create an event recorder from the controller manager.
internal/dataplane/parser/parser.go
Outdated
| @@ -154,6 +191,12 @@ func (p *Parser) GenerateKubernetesObjectReport() []client.Object { | |||
| return report | |||
| } | |||
|
|
|||
| func (p *Parser) GetParsingErrors() []ParsingError { | |||
There was a problem hiding this comment.
I'd prefer to see a comment on this func and perhaps I'd suggest renaming it as getters usually are considered to not mutate the state of the object they are called on, whereas here that's not the case.
There was a problem hiding this comment.
Yeah, sure: e4c6a1b
| func (p *Parser) getCACerts( | ||
| log logrus.FieldLogger, | ||
| storer store.Storer, | ||
| ) []kong.CACertificate { |
There was a problem hiding this comment.
If this changes to have a *Parser receiver then I believe we don't need the params anymore do we? Given that those are stored in Parser anyway (and that's how it's being called).
There was a problem hiding this comment.
That's 100% true, thanks: c5b45ca
| // todo: adapt to implementation | ||
| _ = []kongstate.Plugin{ |
There was a problem hiding this comment.
Given the PR is already marked as ready to review, I believe this needs addressing.
There was a problem hiding this comment.
Please note it's marked as ready for review (as Michał has suggested doing), but still is not meant to be merged to main in this form. I can adapt the tests but IMO it's not worth it for now until we decide that we wanna go with this approach. The key output I wanted to get out of reviewing this is whether the team thinks it's the way we wanna go regarding events publishing (mainly, if it's ok to do that directly from Parser).
There was a problem hiding this comment.
👍
One note though w.r.t to introducing this as part of the user facing interface: we'd ideally document this in a relatively easily discoverable place (https://docs.konghq.com/kubernetes-ingress-controller/latest/ ? )
| type ParsingError struct { | ||
| relatedObjects []client.Object | ||
| reason string | ||
| } |
There was a problem hiding this comment.
Apart from this deserving its own file I feel this is a good approach.
| errors := p.errorsCollector.errors | ||
| p.errorsCollector.errors = nil |
There was a problem hiding this comment.
This would ideally be placed in parsingErrorsCollector
There was a problem hiding this comment.
I like the approach of emitting events related to specific parsing errors that can happen in the controller. Definitely 👍 for this approach. As far as I get, the purpose of this PR is only the validation of a concept, therefore I think we can discuss the implementation details in the actual PRs.
|
Closing this as work on the proper implementation has been kicked off under #3097 |
What this PR does / why we need it:
It adds a way to collect translation errors while running Parser and use them to publish Kubernetes API Event objects with a detailed message describing every failure. Every error can be associated with one or more k8s objects and one event per such object gets created.
Special notes for your reviewer:
This PR serves as a reference prototype and is meant to be thrown away. It's expected that some of the tests won't pass. Please focus on the general idea of introducing Events publishing to Parser.
Fully-fledged implementation will be done once we create an issue and groom it.
Please take a look at my in-line comments to quicker grasp the main points of the changes.