Skip to content
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

events: Begin implementing event system #4984

Merged
merged 3 commits into from
Aug 25, 2022
Merged

events: Begin implementing event system #4984

merged 3 commits into from
Aug 25, 2022

Conversation

mholt
Copy link
Member

@mholt mholt commented Aug 24, 2022

This PR implements a new Caddy app called events that facilitates an eventing system. Modules may emit events with optional metadata, and modules may subscribe to events and run custom code or event handler modules.

Builds on but likely supersedes #4912 by @francislavoie who made a great start of this, and thought really hard about things. This PR takes his work further.

Notably, this is implemented outside the core of Caddy, and thus Caddy core cannot emit events lest we have an import cycle. I don't think that would be necessary, but if we find great value in emitting events from the core, we can move the events logic into Caddy core.

Users can configure event handling directly, or module code can also subscribe to handle events.

JSON config example:

{
	"apps": {
		"events": {
			"subscriptions": [
				{
					"events": ["cert_obtained"],
					"handlers": [
						{
							"handler": "exec",
							"command": "/bin/foobar",
							"args": ["--domain", "{event.data.name}"]
						}
					]
				}
			]
		}
	}
}

This app supports Caddyfile configuration via global options:

{
	events {
		on cert_obtained exec /bin/foobar --domain {event.data.name}
	}
}

Events are emitted synchronously. This confers great simplicity and power by allowing event handlers to give information back to the running code so it can decide whether or how to continue. Events can be emitted asynchronously and thus not block critical paths by spawning a goroutine, but I'm worried whether this will back up under heavy load and cause problems. Still thinking about that.

There are numerous places in the code base that will need to be updated to emit events. Currently, there's spikey emission of these events:

  • unhealthy when a reverse proxy backend goes down
  • healthy when a reverse proxy backend comes back up
  • Plus a few events emitted by CertMagic related to certificate issuance, TLS handshakes, etc. I need to spend some time updating CertMagic to better conform to our preferred interface(s) (CertMagic is not yet 1.0 so it might be a breaking change, but for the better).

Currently the only event handler module is exec, which runs a command when an event happens. Obviously, this is not ideal for extremely hot paths like HTTP requests, but may be more appropriate for lower-volume events like certificate issuance. This is handy as it can be used to have other programs reload the new certificate.

As Francis and I discussed in slack, this feature introduces the first time Caddy can run commands (without a third-party plugin). We've intentionally avoided it previously because it makes us nervous for Caddy to run commands on the system, as we don't want to introduce any security vulnerabilities. However, this implementation requires the command to be hard-coded into the config (no placeholders are expanded) and so it should not allow arbitrary programs or code to be run unless the command in the config allows it. We also do not do any special parsing or interpretation of the command and arguments (other than expanding placeholders in arguments). No shell evaluation, no bash scripting, etc. As usual, we expect that your system is secure (i.e. no one can access the admin endpoint, which would be game over regardless of this feature anyway).

I'd encourage anyone interested in this to try to find security issues before we release this. I believe it's probably safe in terms of not popping shells or allowing RCEs, etc. But I hope you will verify. 🙂

This is still definitely WIP, so there may be some big changes between now and the release.

Thanks to everyone who tests it, reviews it, etc!

Notably, this includes core changes to the caddy.Context.
We now keep a lineage of modules provisioned by each
context, and store the loaded modules as Module types,
not empty interfaces.
@mholt mholt added in progress 🏃‍♂️ Being actively worked on under review 🧐 Review is pending before merging do not merge ⛔ Not ready yet! labels Aug 24, 2022
@mholt mholt added this to the v2.6.0-beta.1 milestone Aug 24, 2022
modules/caddyevents/app.go Outdated Show resolved Hide resolved
modules/caddyevents/app.go Outdated Show resolved Hide resolved
@DeanMarkTaylor
Copy link

DeanMarkTaylor commented Aug 24, 2022

Reading the subject "events: implement event system" - it made me think of the specification CloudEvents with a good Primer here.

I only mention it here to see if it has been considered regarding events from Caddy...
... noting that Google's Eventarc is used in "updated" Google Cloud functionality / APIs (seemingly) across the board (e.g. Cloud Functions v2).

I have no skin in this game, just mentioning it so it's on your radar.

@francislavoie
Copy link
Member

Thanks for pointing that out @DeanMarkTaylor, hadn't heard of it.

My initial design was pretty heavily based on https://symfony.com/doc/current/event_dispatcher.html due to my familiarity with it as a PHP dev.

I think we'll probably not go the route of conforming to a specific spec up-front, but I'm glad to see that we're already quite close to the same thing give or take a field or two: we use the term name instead of type, we use origin as the source (it's the actual module that emitted it), we already use UUID for id, and we use a data KV map. We also add a ts which isn't in the spec, and also we have an Aborted field to let handling inside Caddy get cancelled early.

If someone finds a usecase where CloudEvent compat would help, it should be trivial to make an adapter for it. But for now, I'm not convinced there really would be any kind of interop that would make it viable.

@coolaj86
Copy link
Contributor

coolaj86 commented Aug 25, 2022

Is it possible to specify the process user and group id for the exec?

I know Caddy has to be run as root on macOS to use privileged ports.

@mholt
Copy link
Member Author

mholt commented Aug 25, 2022

@coolaj86 I thought MacOS doesn't require running as root for low ports anymore. 🤔

But sure, I can add that functionality easily enough. This is just bare-bones still.

I might actually pull the exec module out of here and make it a separate plugin.

Seems redundant to handlers. See if there's a real need for it.
@coolaj86
Copy link
Contributor

I thought MacOS doesn't require running as root for low ports anymore. 🤔

That's news to me. Just tested on Monterey and I think you're right:

echo "goodbye" | nc -l 80
echo "hello" | nc localhost 80

@mholt mholt marked this pull request as ready for review August 25, 2022 22:34
@mholt mholt removed in progress 🏃‍♂️ Being actively worked on do not merge ⛔ Not ready yet! labels Aug 25, 2022
@mholt
Copy link
Member Author

mholt commented Aug 25, 2022

@DeanMarkTaylor I implemented a simple CloudEvents-compatible exporter method. Let me know if that's not sufficient!


This PR is being merged into the event branch where work will continue.

Please follow #4912 for future updates on this feature.

@mholt mholt changed the title events: Implement event system events: Begin implementing event system Aug 25, 2022
@mholt mholt merged commit 10a2b1e into event Aug 25, 2022
@mholt mholt removed the under review 🧐 Review is pending before merging label Aug 25, 2022
@mholt mholt deleted the events-2 branch August 26, 2022 03:07
@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants