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

Split run into a public BuildContext and a private part #6378

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Jun 6, 2024

BuildContext can be used to set up a caddy context from a config, but not start any listeners or active components: The returned context has the configured apps provisioned, but otherwise is inert.

This is EXPERIMENTAL: Minimally it's missing documentation and the example for how this can be used to run unit tests.


For context: What I'm trying to do is write unit tests, where I take a (partial) configuration for caddy and then build a context from that. The context is then used to provision modules that need testing, possibly with some weird configuration etc. The problem with the current caddy codebase is that there's no good way to do get to such a context, as on the one hand it has some important private fields (apps), and on the other hand all functions making a context either throw it away again (Validate), or also Start the modules and potentially listeners etc.

By splitting the private run to have the function to build such a context public I can side-step that. For an example here's parts of a test for checking that we correctly implement caddyevents.Handler using this BuildContext function:

package caddy

import (
  // Stuff
)

func init() {
	caddy.RegisterModule(testModule{})
}

// A module that allows us to get a proper caddy context with a non-nil/non-empty `ancestry`
type testModule struct {
	ctx caddy.Context
}

func (t *testModule) Provision(ctx caddyimpl.Context) error {
	// Sorry, but not sorry.
	t.ctx = ctx
	return nil
}

func (testModule) CaddyModule() caddy.ModuleInfo {
	return caddy.ModuleInfo{
		ID: "test_module",
		New: func() caddy.Module {
			return new(testModule)
		},
	}
}

var _ caddy.Provisioner = (*testModule)(nil)

type testEvent struct {
	name   string
	params map[string]any
}

// testRecorder implements an internal interface that allows us to override
// the APM tooling and instead do something else.
type testRecorder struct {
	seenEvents []testEvent
}

func (t *testRecorder) RecordCustomEvent(name string, params map[string]any) {
	t.seenEvents = append(t.seenEvents, testEvent{name, params})
}

var _ recorder = (*testRecorder)(nil)

func TestHandle(t *testing.T) {
	tests := []struct {
		name                   string
		eventData              map[string]any
		expectedRecordedParams map[string]any
	}{
		// A test fixture that we can use to simulate this particular event.
		{
			name: "Records cached_managed_cert event shape",
			eventData: map[string]any{
				"sans": []string{"san1", "san2"},
			},
		},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			// Make a caddy context with the needed applications in them.
			caddyCtx, _ := caddy.BuildContext(&caddyimpl.Config{
				AppsRaw: caddyimpl.ModuleMap{
					"events":            json.RawMessage{},
					"our_own_apm": json.RawMessage{},
				},
			}, false)

			m, err := caddyCtx.App("events")
			assert.NoError(t, err)
			events := m.(*caddyevents.App)

			m, err = caddyCtx.App("our_own_apm")
			assert.NoError(t, err)
			monitoring := m.(*Monitoring)
			// Sneak in a different recorder, so we can watch the events coming in.
			testRecorder := &testRecorder{}
			monitoring.recorder = testRecorder

			// Simulate emitting an event from inside a module
			m, err = caddyCtx.LoadModuleByID("test_module", json.RawMessage{})
			assert.NoError(t, err)
			tm := m.(*testModule)

			// events.Emit() doesn't handle a nil ancestry correctly: In https://github.com/caddyserver/caddy/blob/101d3e740783581110340a68f0b0cbe5f1ab6dbb/modules/caddyevents/app.go#L228
			// `e.origin` can be nil. Work around that by capturing properly bound context.
			// Ugly, the alternative would be more code to expose a Emit on tm ...
			event := events.Emit(tm.ctx, "test", tt.eventData)
			assert.NoError(t, event.Aborted)

			// Check that the world is good
			if len(testRecorder.seenEvents) != 1 {
				t.Fatalf("no event captured")
			}
			assert.Equal(t, 1, len(testRecorder.seenEvents))
			assert.Equal(t, "CaddyEvent", testRecorder.seenEvents[0].name)
			for k, v := range tt.expectedRecordedParams {
				assert.Equal(t, v, testRecorder.seenEvents[0].params[k])
			}
		})
	}
}

ankon added 2 commits June 6, 2024 18:35
`BuildContext` can be used to set up a caddy context from a config, but not start any listeners
or active components: The returned context has the configured apps provisioned, but otherwise is
inert.

This is EXPERIMENTAL: Minimally it's missing documentation and the example for how this can be
used to run unit tests.
The config passed into `BuildContext` can be nil, in which case `BuildContext` will just make one
up that works. In either case that will end up in the finished context.
@ankon ankon force-pushed the feature/buildcontext branch from 1fd67d4 to 8640156 Compare June 6, 2024 16:35
@ankon ankon marked this pull request as ready for review June 6, 2024 16:39
caddy.go Outdated Show resolved Hide resolved
@francislavoie francislavoie added the feature ⚙️ New feature or request label Jun 6, 2024
@francislavoie francislavoie added this to the v2.8.5 milestone Jun 6, 2024
ankon added 2 commits June 6, 2024 22:15
The admin server is a global thing, and in the envisioned use case for `ProvisionContext`
shouldn't actually exist. Hide this detail in a private `provisionContext` instead, and
only expose it publicly with `replaceAdminServer` set to `false`.

This should reduce foot-shooting potential further; in addition the documentation comment
now clearly spells out that the exact interface and implementation details of `ProvisionContext`
are experimental and subject to change.
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This is good now, thanks!

@mholt mholt merged commit a10117f into caddyserver:master Jun 6, 2024
23 checks passed
ankon added a commit to framer/caddy that referenced this pull request Jun 7, 2024
…od (caddyserver#6378)

* Split `run` into a public `BuildContext` and a private part

`BuildContext` can be used to set up a caddy context from a config, but not start any listeners
or active components: The returned context has the configured apps provisioned, but otherwise is
inert.

This is EXPERIMENTAL: Minimally it's missing documentation and the example for how this can be
used to run unit tests.

* Use the config from the context

The config passed into `BuildContext` can be nil, in which case `BuildContext` will just make one
up that works. In either case that will end up in the finished context.

* Rename `BuildContext` to `ProvisionContext` to better match the function

* Hide the `replaceAdminServer` parts

The admin server is a global thing, and in the envisioned use case for `ProvisionContext`
shouldn't actually exist. Hide this detail in a private `provisionContext` instead, and
only expose it publicly with `replaceAdminServer` set to `false`.

This should reduce foot-shooting potential further; in addition the documentation comment
now clearly spells out that the exact interface and implementation details of `ProvisionContext`
are experimental and subject to change.
ankon added a commit to framer/caddy that referenced this pull request Jun 7, 2024
…od (caddyserver#6378)

* Split `run` into a public `BuildContext` and a private part

`BuildContext` can be used to set up a caddy context from a config, but not start any listeners
or active components: The returned context has the configured apps provisioned, but otherwise is
inert.

This is EXPERIMENTAL: Minimally it's missing documentation and the example for how this can be
used to run unit tests.

* Use the config from the context

The config passed into `BuildContext` can be nil, in which case `BuildContext` will just make one
up that works. In either case that will end up in the finished context.

* Rename `BuildContext` to `ProvisionContext` to better match the function

* Hide the `replaceAdminServer` parts

The admin server is a global thing, and in the envisioned use case for `ProvisionContext`
shouldn't actually exist. Hide this detail in a private `provisionContext` instead, and
only expose it publicly with `replaceAdminServer` set to `false`.

This should reduce foot-shooting potential further; in addition the documentation comment
now clearly spells out that the exact interface and implementation details of `ProvisionContext`
are experimental and subject to change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants