fix notification bug #993

thierry-f-78 · 2021-06-22T09:31:19Z

The notification template are valided at notification time with a
"Must" function. So, if the templte is wrong, the final user get a
panic/backtrace like the floowing.

This patch fix this behavior, it compiles the template, catch the
error and retirn the error in the generated message (like processing
errors)

this ensure the service continue working, even if notification were wrong.

Jun 22 11:18:26 batch03 dkron[56662]: panic: template: report:1: function "JSEscaper" not defined
Jun 22 11:18:26 batch03 dkron[56662]: goroutine 293 [running]:
Jun 22 11:18:26 batch03 dkron[56662]: text/template.Must(...)
Jun 22 11:18:26 batch03 dkron[56662]: /usr/local/go/src/text/template/helper.go:25
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).buildTemplate(0xc00121ce70, 0xc000381ae0, 0x98, 0xc0001e8e70, 0x18)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:72 +0x6bc
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).callExecutionWebhook(0xc00121ce70, 0xc0001e8e70, 0x0, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:137 +0x86
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).Send(0xc00121ce70, 0xc0001e8e70, 0xc0011b6450, 0xc000e8c468)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:42 +0x85
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*GRPCServer).ExecutionDone(0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0011897d0, 0x0, 0x0, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/grpc.go:240 +0x1495
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/plugin/types._Dkron_ExecutionDone_Handler(0x2415f40, 0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0014b2c60, 0x0, 0x331caf8, 0xc0011897a0, 0xc001481980, 0x57)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/plugin/types/dkron_grpc.pb.go:233 +0x214
Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).processUnaryRPC(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0xc000ecc1e0, 0x4278258, 0x0, 0x0, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1217 +0x52b
Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).handleStream(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1540 +0xd0c
Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00117e810, 0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:878 +0xab
Jun 22 11:18:26 batch03 dkron[56662]: created by google.golang.org/grpc.(*Server).serveStreams.func1
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:876 +0x1fd

yvanoers · 2021-06-23T21:17:38Z

dkron/notifier.go

-	t := template.Must(template.New("report").Parse(templ))
+	t, e := template.New("report").Parse(templ)
+	if e != nil {
+		return bytes.NewBuffer([]byte("Failed to execute template: " + e.Error()))


IMHO the error should be along the lines of 'Failed to parse template', to distinguish it from the other error below.

You're right ! I change the message.

yvanoers · 2021-06-23T21:20:00Z

I think it would a good idea to make it fall back on a known-working template if it errs out somewhere.
That way the actual message doesn't get lost.

But this is beyond the scope of this PR.

thierry-f-78 · 2021-06-24T15:25:18Z

I agree. In other way I suggest the error message will be sent in logs, and not in the report message, because the component which will receive the message is probably not able to manage dkron internal errors.

So, as you understand, this PR is just to avoir the panic.

vcastellm

Nice one here, thanks @thierry-f-78, the only thing I would add here is also logging an error in that case.

thierry-f-78 · 2021-06-29T07:44:02Z

ok for the log. I also suggest not triggering the webhook is an errors occurs, or rollbacking on standard message. The goal is no returning dkron internal error to external system.

vcastellm · 2021-07-05T20:57:45Z

Can you add the missing logging here? so we can proceed with the merge, thanks!

thierry-f-78 · 2021-07-06T08:43:44Z

done.

yvanoers

I'm OK with it, although I liked it better when it said 'parsing template'.

The notification template are valided at notification time with a "Must" function. So, if the templte is wrong, the final user get a panic/backtrace like the floowing. This patch fix this behavior, it compiles the template, catch the error and retirn the error in the generated message (like processing errors) this ensure the service continue working, even if notification were wrong. Jun 22 11:18:26 batch03 dkron[56662]: panic: template: report:1: function "JSEscaper" not defined Jun 22 11:18:26 batch03 dkron[56662]: goroutine 293 [running]: Jun 22 11:18:26 batch03 dkron[56662]: text/template.Must(...) Jun 22 11:18:26 batch03 dkron[56662]: /usr/local/go/src/text/template/helper.go:25 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).buildTemplate(0xc00121ce70, 0xc000381ae0, 0x98, 0xc0001e8e70, 0x18) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:72 +0x6bc Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).callExecutionWebhook(0xc00121ce70, 0xc0001e8e70, 0x0, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:137 +0x86 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).Send(0xc00121ce70, 0xc0001e8e70, 0xc0011b6450, 0xc000e8c468) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:42 +0x85 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*GRPCServer).ExecutionDone(0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0011897d0, 0x0, 0x0, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/grpc.go:240 +0x1495 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/plugin/types._Dkron_ExecutionDone_Handler(0x2415f40, 0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0014b2c60, 0x0, 0x331caf8, 0xc0011897a0, 0xc001481980, 0x57) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/plugin/types/dkron_grpc.pb.go:233 +0x214 Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).processUnaryRPC(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0xc000ecc1e0, 0x4278258, 0x0, 0x0, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1217 +0x52b Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).handleStream(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1540 +0xd0c Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00117e810, 0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:878 +0xab Jun 22 11:18:26 batch03 dkron[56662]: created by google.golang.org/grpc.(*Server).serveStreams.func1 Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:876 +0x1fd

thierry-f-78 · 2021-07-06T19:30:46Z

I agree. that's done. I also fix mistake in the previous version of the PR.

yvanoers

LGTM

vcastellm

LGTM

yvanoers requested changes Jun 23, 2021

View reviewed changes

vcastellm reviewed Jun 29, 2021

View reviewed changes

yvanoers previously approved these changes Jul 6, 2021

View reviewed changes

ghost dismissed yvanoers’s stale review via 0c6f401 July 6, 2021 19:29

yvanoers approved these changes Jul 6, 2021

View reviewed changes

vcastellm approved these changes Jul 7, 2021

View reviewed changes

vcastellm merged commit 2aa3e2f into distribworks:master Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix notification bug #993

fix notification bug #993

thierry-f-78 commented Jun 22, 2021

yvanoers Jun 23, 2021

thierry-f-78 Jun 24, 2021

yvanoers commented Jun 23, 2021

thierry-f-78 commented Jun 24, 2021

vcastellm left a comment

thierry-f-78 commented Jun 29, 2021

vcastellm commented Jul 5, 2021

thierry-f-78 commented Jul 6, 2021

yvanoers left a comment

thierry-f-78 commented Jul 6, 2021 •

edited

Loading

yvanoers left a comment

vcastellm left a comment

fix notification bug #993

fix notification bug #993

Conversation

thierry-f-78 commented Jun 22, 2021

yvanoers Jun 23, 2021

Choose a reason for hiding this comment

thierry-f-78 Jun 24, 2021

Choose a reason for hiding this comment

yvanoers commented Jun 23, 2021

thierry-f-78 commented Jun 24, 2021

vcastellm left a comment

Choose a reason for hiding this comment

thierry-f-78 commented Jun 29, 2021

vcastellm commented Jul 5, 2021

thierry-f-78 commented Jul 6, 2021

yvanoers left a comment

Choose a reason for hiding this comment

thierry-f-78 commented Jul 6, 2021 • edited Loading

yvanoers left a comment

Choose a reason for hiding this comment

vcastellm left a comment

Choose a reason for hiding this comment

thierry-f-78 commented Jul 6, 2021 •

edited

Loading