feat: Courier foreground worker with "kratos courier watch" #1033

mbonnell-wish · 2021-02-03T01:33:56Z

Related issue

Proposed changes

Following the discussion in #1024, this PR defines a new kratos courier watch command which runs the message courier in the foreground, as well as disabling the background courier by default when kratos serve is executed.
The --watch-courier flag can be passed to the serve command to execute the courier in the background as is the current behavior.

Checklist

I have read the contributing guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further comments

mattbonnell · 2021-02-03T14:12:40Z

Hey @aeneasr, would you happen to know the source of this error?

zepatrik

The error you get is caused by https:/ory/x/blob/87359d96f1cf9eb327949f693077968a50ecc283/configx/provider.go#L162-L166
Because of that, you have to add the flag to the config schema, just like it is done with e.g. the dev or help flag:

kratos/.schema/config.schema.json

Lines 1168 to 1184 in af200d1

 "dev": { 

 "type": "boolean" 

 }, 

 "help": { 

 "type": "boolean" 

 }, 

 "sqa-opt-out": { 

 "type": "boolean", 

 "default": false 

 }, 

 "config": { 

 "type": "array", 

 "items": { 

 "type": "string" 

 } 

 } 

 },

We should probably adjust the flag loading, but it will be possible to put that value in the config file as well 😉

Looks good otherwise 👍

mattbonnell · 2021-02-03T15:32:46Z

Thanks!

mbonnell-wish · 2021-02-03T16:41:31Z

What would we be looking for in terms of added material in the user guide docs?

mbonnell-wish · 2021-02-03T18:33:21Z

Hmm, I'm failing thevalidate CI job because the generated cli docs don't pass the formatting check. Is there some command i should be running other than make docs/cli?

zepatrik · 2021-02-04T09:08:05Z

make format will format everything, cd docs; npm run format only the docs 😉

aeneasr · 2021-02-05T07:57:19Z

I believe you need to update the e2e config to reflect the new changes as the mail tests are probably failing!

https:/ory/kratos/blob/master/test/e2e/run.sh#L141

aeneasr

This looks great!

I think it would make sense to explain this in the docs a bit:

https://www.ory.sh/kratos/docs/concepts/email-sms
https://www.ory.sh/kratos/docs/guides/account-activation-email-verification - a small sentence and a link will be enough - something along the lines of "set up mail worker (link)".
https://www.ory.sh/kratos/docs/guides/high-availability-ha - explain that with multiple nodes you need one worker node to handle the mail queue

Thank you!

aeneasr · 2021-02-05T08:20:28Z

cmd/courier/watch.go

+ Run: func(cmd *cobra.Command, args []string) {
+ d := driver.New(cmd.Context(), configx.WithFlags(cmd.Flags()))
+
+ go ServeHealth(d, cmd, args)


I think having health and metrics servers is honorable :) But we don't really need them IMO. It would make sense if this would handle HTTP API requests. Then, the LB needs to know if the service is up (health) and prometheus would want to know how much latency we have (for example).

So in my opinion, this can be removed!

Thanks for the review!

Yeah, after doing some more reading, I think we can do without liveness probes in this case, and just rely on k8s restarting the pod if the courier crashes. I'll remove this

On the metrics front, you're quite right that latency metrics wouldn't be very relevant here since it isn't handling HTTP requests, but we would like to be able to monitor the go runtime related metrics, particularly since we'd like to be able to measure how well it's performing as we add load and potentially tune things like the number of concurrent goroutines and batch fetch size in response to our observations.

I will work on these additional docs today

Ok draft of doc updates are in, take a read and don't hesitate to suggest improvements

aeneasr

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

cmd/courier/watch.go

docs/docs/cli/kratos-courier-watch.md

aeneasr

Great! Now we need a bit of testing and we're good to go! :)

driver/config/config.go

aeneasr · 2021-02-09T14:18:58Z

cmd/courier/watch.go

@@ -0,0 +1,82 @@
+package courier


Can you please add a test to see if this works (e.g. health port) as expected?

Yes, much needed. Where would the best place be to add it?

Two options: you extract as much from the cobra handler as possible (e.g. as a function) and then set it up, or you use the cobra test framework to run the command.

I would opt for the first option. Regarding courier tests and the SMTP integration, check out:

https:/ory/kratos/blob/master/courier/courier_test.go#L41

Cobra commands can be controlled from tests using:

import "github.com/ory/x/cmdx" // ... ce := &cmdx.CommandExecuter{New: func() *cobra.Command { return cmd.RootCmd // might need to be exported in kratos }, Ctx: ctx} ce.ExecNoErr(t, "your", "command")

Added some tests in cmd/courier/watch_test.go, let me know what you think!

mbonnell-wish · 2021-02-11T15:42:18Z

hm, the GO111MODULE=off go get github.com/mattn/goveralls github.com/ory/go-acc test job is failing with

cannot find package "github.com/hashicorp/hcl/hcl/printer" in any of:
	/usr/local/go/src/github.com/hashicorp/hcl/hcl/printer (from $GOROOT)
	/go/src/github.com/hashicorp/hcl/hcl/printer (from $GOPATH)

Exited with code exit status 1

Can't imagine it's related to my PR

mbonnell-wish · 2021-02-11T15:43:19Z

cmd/courier/watch.go

+ }
+ }()
+ <-ctx.Done()
+ return server.Shutdown(ctx)


Added this so the server can be shutdown by cancelling the context (eg in unit tests).
Let me know if there's a better way.

I think this looks ok

aeneasr · 2021-02-12T09:34:00Z

@mattbonnell , great work! The current problem for the tests is not your fault. I'll be trying to fix that now!

BREACKING CHANGES: This patch moves the courier watcher (responsible for sending mail) to its own foreground worker, which can be executed as a, for example, Kubernetes job. It is still possible to have the previous behaviour which would run the worker as a background task when running `kratos serve` by using the `--watch-courier` flag. To run the foreground worker, use `kratos courier watch -c your/config.yaml`. Closes #1033 Closes #1024 Co-authored-by: Matt Bonnell <[email protected]>

mbonnell-wish marked this pull request as draft February 3, 2021 01:35

zepatrik reviewed Feb 3, 2021

View reviewed changes

mbonnell-wish force-pushed the courier-foreground-worker branch from 48997fb to 8e3c74d Compare February 3, 2021 16:22

mbonnell-wish marked this pull request as ready for review February 3, 2021 16:34

mbonnell-wish force-pushed the courier-foreground-worker branch from d642442 to 71f4e83 Compare February 3, 2021 17:56

aeneasr reviewed Feb 5, 2021

View reviewed changes

mbonnell-wish force-pushed the courier-foreground-worker branch from 8ffbd5b to 326bfea Compare February 5, 2021 17:21

aeneasr reviewed Feb 8, 2021

View reviewed changes

cmd/courier/watch.go Outdated Show resolved Hide resolved

mbonnell-wish force-pushed the courier-foreground-worker branch from f2befe9 to c9b7f63 Compare February 8, 2021 23:50

mbonnell-wish commented Feb 9, 2021

View reviewed changes

docs/docs/cli/kratos-courier-watch.md Outdated Show resolved Hide resolved

aeneasr reviewed Feb 9, 2021

View reviewed changes

mbonnell-wish force-pushed the courier-foreground-worker branch from d4bd510 to c4ca6ad Compare February 11, 2021 15:05

mbonnell-wish commented Feb 11, 2021

View reviewed changes

aeneasr mentioned this pull request Feb 12, 2021

feat: courier foreground worker with "kratos courier watch" #1062

Merged

5 tasks

fix port issue

a0e1664

aeneasr changed the base branch from master to courier-foreground-worker February 12, 2021 13:58

aeneasr merged commit b52296c into ory:courier-foreground-worker Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Courier foreground worker with "kratos courier watch" #1033

feat: Courier foreground worker with "kratos courier watch" #1033

mbonnell-wish commented Feb 3, 2021 •

edited

Loading

mattbonnell commented Feb 3, 2021

zepatrik left a comment

mattbonnell commented Feb 3, 2021

mbonnell-wish commented Feb 3, 2021

mbonnell-wish commented Feb 3, 2021 •

edited

Loading

zepatrik commented Feb 4, 2021

aeneasr commented Feb 5, 2021

aeneasr left a comment

aeneasr Feb 5, 2021

mbonnell-wish Feb 5, 2021

mbonnell-wish Feb 5, 2021

mbonnell-wish Feb 5, 2021

aeneasr left a comment

aeneasr left a comment

aeneasr Feb 9, 2021

mbonnell-wish Feb 9, 2021

aeneasr Feb 9, 2021

mbonnell-wish Feb 11, 2021

mbonnell-wish commented Feb 11, 2021

mbonnell-wish Feb 11, 2021

aeneasr Feb 12, 2021

aeneasr commented Feb 12, 2021

	"dev": {
	"type": "boolean"
	},
	"help": {
	"type": "boolean"
	},
	"sqa-opt-out": {
	"type": "boolean",
	"default": false
	},
	"config": {
	"type": "array",
	"items": {
	"type": "string"
	}
	}
	},

feat: Courier foreground worker with "kratos courier watch" #1033

feat: Courier foreground worker with "kratos courier watch" #1033

Conversation

mbonnell-wish commented Feb 3, 2021 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

mattbonnell commented Feb 3, 2021

zepatrik left a comment

Choose a reason for hiding this comment

mattbonnell commented Feb 3, 2021

mbonnell-wish commented Feb 3, 2021

mbonnell-wish commented Feb 3, 2021 • edited Loading

zepatrik commented Feb 4, 2021

aeneasr commented Feb 5, 2021

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbonnell-wish commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Feb 12, 2021

mbonnell-wish commented Feb 3, 2021 •

edited

Loading

mbonnell-wish commented Feb 3, 2021 •

edited

Loading