-
Notifications
You must be signed in to change notification settings - Fork 44
feat(reconcile): add a Webhook kind #1772
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
Changes from all commits
d4e56f1
03118fe
e772c6d
49bcca0
a8ffd18
63acd9c
385797d
cb9c649
14828be
8db057c
9f2cc05
05029c8
ac9457c
9c7aa14
e7a4043
05a899b
0039399
987cce5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| package webhook_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/raystack/frontier/core/webhook" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // fakeEndpointRepo is an in-memory webhook.EndpointRepository for service tests. | ||
| type fakeEndpointRepo struct { | ||
| items []webhook.Endpoint | ||
| } | ||
|
|
||
| func (f *fakeEndpointRepo) Create(_ context.Context, e webhook.Endpoint) (webhook.Endpoint, error) { | ||
| f.items = append(f.items, e) | ||
| return e, nil | ||
| } | ||
|
|
||
| func (f *fakeEndpointRepo) UpdateByID(_ context.Context, e webhook.Endpoint) (webhook.Endpoint, error) { | ||
| for i := range f.items { | ||
| if f.items[i].ID == e.ID { | ||
| f.items[i] = e | ||
| return e, nil | ||
| } | ||
| } | ||
| return webhook.Endpoint{}, webhook.ErrNotFound | ||
| } | ||
|
|
||
| func (f *fakeEndpointRepo) Delete(_ context.Context, _ string) error { return nil } | ||
|
|
||
| func (f *fakeEndpointRepo) List(_ context.Context, _ webhook.EndpointFilter) ([]webhook.Endpoint, error) { | ||
| return f.items, nil | ||
| } | ||
|
|
||
| func TestServiceCreateEndpointValidation(t *testing.T) { | ||
| t.Run("rejects a non-absolute url", func(t *testing.T) { | ||
| s := webhook.NewService(&fakeEndpointRepo{}) | ||
| _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "not-a-url"}) | ||
| assert.ErrorIs(t, err, webhook.ErrInvalidDetail) | ||
| }) | ||
|
|
||
| t.Run("rejects an empty url", func(t *testing.T) { | ||
| s := webhook.NewService(&fakeEndpointRepo{}) | ||
| _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: " "}) | ||
| assert.ErrorIs(t, err, webhook.ErrInvalidDetail) | ||
| }) | ||
|
|
||
| t.Run("rejects a non-http(s) scheme", func(t *testing.T) { | ||
| s := webhook.NewService(&fakeEndpointRepo{}) | ||
| _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "ftp://a.example/hook"}) | ||
| assert.ErrorIs(t, err, webhook.ErrInvalidDetail) | ||
| }) | ||
|
|
||
| t.Run("rejects an http(s) url with no host", func(t *testing.T) { | ||
| // "https://" and "http:///path" parse as absolute http(s) URLs with an | ||
| // empty host, but a webhook with no host to deliver to is useless. | ||
| s := webhook.NewService(&fakeEndpointRepo{}) | ||
| _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "https://"}) | ||
| assert.ErrorIs(t, err, webhook.ErrInvalidDetail) | ||
| }) | ||
|
|
||
| t.Run("rejects an unknown state", func(t *testing.T) { | ||
| s := webhook.NewService(&fakeEndpointRepo{}) | ||
| _, err := s.CreateEndpoint(context.Background(), webhook.Endpoint{URL: "https://a.example/hook", State: "paused"}) | ||
| assert.ErrorIs(t, err, webhook.ErrInvalidDetail) | ||
| }) | ||
|
|
||
| t.Run("rejects a url another endpoint already uses", func(t *testing.T) { | ||
| repo := &fakeEndpointRepo{items: []webhook.Endpoint{{ID: "e1", URL: "https://a.example/hook"}}} | ||
| _, err := webhook.NewService(repo).CreateEndpoint(context.Background(), webhook.Endpoint{URL: "https://a.example/hook"}) | ||
| assert.ErrorIs(t, err, webhook.ErrConflict) | ||
| }) | ||
|
|
||
| t.Run("creates a valid endpoint, defaulting state and generating a secret", func(t *testing.T) { | ||
| got, err := webhook.NewService(&fakeEndpointRepo{}).CreateEndpoint( | ||
| context.Background(), webhook.Endpoint{URL: "https://a.example/hook"}) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, webhook.Enabled, got.State) // defaulted | ||
| assert.Len(t, got.Secrets, 1) // server-generated signing secret | ||
| }) | ||
| } | ||
|
|
||
| func TestServiceUpdateEndpointURLUniqueness(t *testing.T) { | ||
| t.Run("rejects a url used by a different endpoint", func(t *testing.T) { | ||
| repo := &fakeEndpointRepo{items: []webhook.Endpoint{ | ||
| {ID: "e1", URL: "https://a.example/hook", State: webhook.Enabled}, | ||
| {ID: "e2", URL: "https://b.example/hook", State: webhook.Enabled}, | ||
| }} | ||
| _, err := webhook.NewService(repo).UpdateEndpoint(context.Background(), | ||
| webhook.Endpoint{ID: "e2", URL: "https://a.example/hook", State: webhook.Enabled}) | ||
| assert.ErrorIs(t, err, webhook.ErrConflict) | ||
| }) | ||
|
|
||
| t.Run("lets an endpoint keep its own url", func(t *testing.T) { | ||
| repo := &fakeEndpointRepo{items: []webhook.Endpoint{ | ||
| {ID: "e1", URL: "https://a.example/hook", State: webhook.Enabled}, | ||
| }} | ||
| _, err := webhook.NewService(repo).UpdateEndpoint(context.Background(), | ||
| webhook.Endpoint{ID: "e1", URL: "https://a.example/hook", State: webhook.Disabled}) | ||
| assert.NoError(t, err) | ||
| }) | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -194,6 +194,45 @@ spec: | |||||||||||||
| - Export writes only the preferences whose value differs from the default, so settings at | ||||||||||||||
| their default stay out of the file. | ||||||||||||||
|
|
||||||||||||||
| ## The Webhook kind | ||||||||||||||
|
|
||||||||||||||
| `Webhook` manages webhook endpoints: a URL, the events it subscribes to, and whether it is | ||||||||||||||
| enabled. The URL is the identity and never changes. | ||||||||||||||
|
|
||||||||||||||
| ```yaml | ||||||||||||||
| apiVersion: v1 | ||||||||||||||
| kind: Webhook | ||||||||||||||
| spec: | ||||||||||||||
| - url: https://hooks.example.org/frontier | ||||||||||||||
| description: Ops notifications | ||||||||||||||
| subscribed_events: | ||||||||||||||
| - app.user.created | ||||||||||||||
| - app.group.created | ||||||||||||||
| state: enabled | ||||||||||||||
| - url: https://old.example.org/frontier | ||||||||||||||
| delete: true | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| - The URL must be a valid absolute HTTP(S) URL, and it is the identity. If two endpoints on the | ||||||||||||||
| server share a URL, the identity is ambiguous: the plan fails and names the ids so you can | ||||||||||||||
| remove the extra one by hand. | ||||||||||||||
|
Comment on lines
+216
to
+218
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Document URL trimming in the identity rule. The reconciliation contract identifies webhooks by trimmed URLs, but this section only says that the URL is the identity. State explicitly that surrounding whitespace is trimmed before validation and identity comparison. Proposed wording-- The URL must be a valid absolute HTTP(S) URL, and it is the identity.
+- The URL is trimmed before validation and identity comparison, and must be a valid absolute HTTP(S) URL.📝 Committable suggestion
Suggested change
|
||||||||||||||
| - `subscribed_events` is the full set of events the endpoint receives, compared as a set. An | ||||||||||||||
| empty list, or leaving the field out, means every event, which is the server default. It is | ||||||||||||||
| the complete desired set, not a keep-if-omitted field: dropping it sets the endpoint to all | ||||||||||||||
| events rather than keeping the current set, and export always writes it, showing `[]` for an | ||||||||||||||
| all-events endpoint. `description` and `state` (`enabled` or `disabled`) follow the same one | ||||||||||||||
| field model: a value you write is used as-is, and leaving the field out converges it to the | ||||||||||||||
| default. Description defaults to empty, so dropping it clears any description on the server; | ||||||||||||||
| state defaults to `enabled`, so dropping it re-enables the endpoint. | ||||||||||||||
| - Every endpoint on the server must appear in the file, kept or marked `delete: true`. An | ||||||||||||||
| endpoint that is missing fails the plan; nothing is deleted just because it is missing. | ||||||||||||||
| - The signing secret is server-owned. The server generates it when the endpoint is created | ||||||||||||||
| and never returns it on read, so it is not part of the file, never shows up in a plan, and | ||||||||||||||
| can never appear in an export. | ||||||||||||||
| - Export leaves out `state` when it is the default `enabled` and `description` when it is | ||||||||||||||
| empty, so reconciling an export plans nothing, and headers and metadata set through other | ||||||||||||||
| tools are carried through an update untouched. | ||||||||||||||
|
|
||||||||||||||
| ## Running it | ||||||||||||||
|
|
||||||||||||||
| Log in as a superuser. The bootstrap service user exists for exactly this; its client id | ||||||||||||||
|
|
@@ -236,7 +275,7 @@ The kind argument is case-insensitive and accepts a plural, so `platformuser` an | |||||||||||||
|
|
||||||||||||||
| ## More kinds | ||||||||||||||
|
|
||||||||||||||
| This page covers `PlatformUser`, `Permission`, `Role`, and `Preference`. The design and | ||||||||||||||
| This page covers `PlatformUser`, `Permission`, `Role`, `Preference`, and `Webhook`. The design and | ||||||||||||||
| the rules every kind follows live in | ||||||||||||||
| [RFC 0001](https://github.com/raystack/frontier/blob/main/docs/rfcs/0001-declarative-reconcile.md), | ||||||||||||||
| which also lists the kinds proposed next. The flag reference for both commands is in the | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
TOCTOU race on URL uniqueness, plus a full table scan on every write.
ensureURLIsFreelists all endpoints and checks in Go, thenCreate/UpdateByIDruns as a separate, unsynchronized step. Two concurrentCreateEndpointcalls for the same URL can both pass the check and both persist — exactly the "two endpoints sharing a url" scenario thatinternal/reconcile/webhook.go'sdiffWebhookshas to special-case as an unrecoverable error ("the url identity is ambiguous... delete the extra one by hand"). This check-then-act pattern is unreliable without a DB-level unique constraint or a lock/transaction spanning the check and the write.Separately,
s.eRepo.List(ctx, EndpointFilter{})fetches every endpoint on every create/update, which won't scale as the number of webhooks grows.Recommend enforcing uniqueness with a unique index at the storage layer (with the app-level check kept as a fast-path/better error message), and, if a targeted lookup by URL is available on the repository, using that instead of a full list scan.
Also applies to: 77-83, 108-123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferring this on purpose, tracked under "Not in this PR" in the description. The real fix is a DB unique index on
webhook_endpoints.url, which is a migration with prod-data implications, so it lands separately. Until then the reconcile diff fails loudly and names the duplicate ids to clean up, and the app-level check stays as a fast path with a clear error. The full-list scan on each write goes away with that same follow-up (a lookup by url).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohilsurana, acknowledged. Deferring the database-level uniqueness constraint is reasonable given the migration and production-data implications. The current application-level guard plus reconcile-time duplicate detection provides a clear interim failure mode.Would you like me to open a GitHub follow-up issue for the unique index and targeted URL lookup?
✏️ Learnings added