feat(reconcile): add a Webhook kind#1772
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds webhook endpoint validation and uniqueness checks, maps webhook service errors to API status codes, introduces webhook desired-state reconciliation and export support, registers the reconciler with the CLI, and documents webhook reconciliation semantics. ChangesWebhook endpoint handling
Webhook reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 29941506298Warning No base build found for commit Coverage: 46.59%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
71b308b to
45fd480
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/api/v1beta1connect/webhook.go (1)
15-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
webhookErrCodeisn't applied toListWebhooks/DeleteWebhook.
CreateWebhookandUpdateWebhooknow surface proper status codes viawebhookErrCode, butListWebhooks(line 88) andDeleteWebhook(line 108) still hardcodeconnect.CodeInternal. A delete of a nonexistent webhook would plausibly returnwebhook.ErrNotFoundfrom the service and should map toCodeNotFound, matching this handler's own new convention.♻️ Apply consistently
err := h.webhookService.DeleteEndpoint(ctx, webhookID) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteWebhook: webhook_id=%s: %w", webhookID, err)) + return nil, connect.NewError(webhookErrCode(err), fmt.Errorf("DeleteWebhook: webhook_id=%s: %w", webhookID, err)) }internal/reconcile/webhook.go (1)
81-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtract a shared normalize+validate+dedupe step; also trim URLs to match server normalization.
validateWebhookSpectreats a whitespace-only URL as empty but then parses/stores/compares the raw untrimmed value, whilecore/webhook/service.go'sCreateEndpoint/UpdateEndpointalwaysstrings.TrimSpacethe URL before validating/storing. A spec URL with incidental whitespace can pass client-side validation yet fail to match the server's (trimmed) URL indiffWebhooks'sbyURLlookup, producing a spurious "add" that the server then rejects as a duplicate. Separately, the duplicate-URL "seen"-map dedupe loop is written independently in bothdiffWebhooksandValidate.
internal/reconcile/webhook.go#L81-L95: trims.URL(e.g.s.URL = strings.TrimSpace(s.URL)) before parsing/validating, and use the trimmed value for storage/comparison indiffWebhooks.internal/reconcile/webhook_reconciler.go#L37-L53: replace the standalone validate+seen-map loop with a shared helper (e.g.normalizeAndValidateWebhookSpecs([]WebhookSpec) ([]WebhookSpec, error)) reused bydiffWebhooks, so both call sites trim/validate/dedupe identically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cbe9a70-6de0-457b-bfd2-93c50daea78f
📒 Files selected for processing (11)
cmd/reconcile.gocore/webhook/service.gocore/webhook/service_test.godocs/content/docs/reconcile.mdxdocs/content/docs/reference/cli.mdxinternal/api/v1beta1connect/webhook.gointernal/reconcile/role.gointernal/reconcile/webhook.gointernal/reconcile/webhook_reconciler.gointernal/reconcile/webhook_reconciler_test.gointernal/reconcile/webhook_test.go
| endpoint.URL = strings.TrimSpace(endpoint.URL) | ||
| if err := validateEndpoint(endpoint); err != nil { | ||
| return Endpoint{}, err | ||
| } | ||
| if err := s.ensureURLIsFree(ctx, endpoint.URL, endpoint.ID); err != nil { | ||
| return Endpoint{}, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
TOCTOU race on URL uniqueness, plus a full table scan on every write.
ensureURLIsFree lists all endpoints and checks in Go, then Create/UpdateByID runs as a separate, unsynchronized step. Two concurrent CreateEndpoint calls for the same URL can both pass the check and both persist — exactly the "two endpoints sharing a url" scenario that internal/reconcile/webhook.go's diffWebhooks has 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/webhook/service.go (1)
53-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize URLs before enforcing identity uniqueness.
Only surrounding whitespace is normalized here, while
ensureURLIsFreecompares strings byte-for-byte. Equivalent targets such ashttps://EXAMPLE.comandhttps://example.com/can therefore create multiple records for one webhook destination. Parse and canonicalize the URL once, persist that canonical form, and compare canonical values.Also applies to: 77-83
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3aa9d73-003c-438e-8cbc-29f324acdb9c
📒 Files selected for processing (6)
core/webhook/service.gocore/webhook/service_test.gointernal/api/v1beta1connect/webhook.gointernal/reconcile/webhook.gointernal/reconcile/webhook_reconciler.gointernal/reconcile/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/webhook/service_test.go
- internal/reconcile/webhook.go
- internal/reconcile/webhook_reconciler.go
- internal/reconcile/webhook_test.go
| func validateEndpoint(endpoint Endpoint) error { | ||
| u, err := url.Parse(endpoint.URL) | ||
| if err != nil || !u.IsAbs() { | ||
| return fmt.Errorf("%w: url must be a valid absolute URL", ErrInvalidDetail) | ||
| } | ||
| // The server dispatches events to this URL, so restrict it to http(s). This | ||
| // keeps other schemes (file, gopher, ...) out of the delivery path. | ||
| if u.Scheme != "http" && u.Scheme != "https" { | ||
| return fmt.Errorf("%w: url scheme must be http or https", ErrInvalidDetail) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant files and symbols first
git ls-files core/webhook
printf '\n--- outline: core/webhook/service.go ---\n'
ast-grep outline core/webhook/service.go --view expanded || true
printf '\n--- outline: any tests in core/webhook ---\n'
fd -a -t f . core/webhook | sed 's#^\./##' | sortRepository: raystack/frontier
Length of output: 1864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the webhook service implementation around validation and uniqueness.
sed -n '1,220p' core/webhook/service.go | cat -n
printf '\n--- search for URL uniqueness and trimming ---\n'
rg -n "ensureURLIsFree|validateEndpoint|TrimSpace|Parse\\(" core/webhook/service.go core/webhook -g '!**/*_test.go'Repository: raystack/frontier
Length of output: 10217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tests that may cover webhook URL validation/uniqueness.
fd -a -t f '.*_test\\.go' core/webhook | sort
printf '\n--- relevant test snippets ---\n'
rg -n "http:foo|http:///|IsAbs|Host|ensureURLIsFree|duplicate|TrimSpace" core/webhook -g '*_test.go'Repository: raystack/frontier
Length of output: 187
Require a host for HTTP(S) webhook URLs.
url.Parse + IsAbs() still accepts values like http:foo or http:///path, so endpoints without a destination host can be saved and only fail during delivery. Reject empty u.Host here.
Webhook description and state used keep-if-omitted: an omitted field kept the server value, so you could not clear a description or reset a disabled endpoint by leaving the field out. That breaks rule 2's one field model. resolve() now lays the file's present fields over the defaults (description empty, state enabled), so an omitted field converges to its default and nothing is merged from the server. Export already drops default-valued fields, so the round-trip still plans zero ops.
d2832dc to
e7a4043
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0fc7b71-abab-40ca-b59e-084866d9f105
📒 Files selected for processing (11)
cmd/reconcile.gocore/webhook/service.gocore/webhook/service_test.godocs/content/docs/reconcile.mdxdocs/content/docs/reference/cli.mdxinternal/api/v1beta1connect/webhook.gointernal/reconcile/role.gointernal/reconcile/webhook.gointernal/reconcile/webhook_reconciler.gointernal/reconcile/webhook_reconciler_test.gointernal/reconcile/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/reconcile/role.go
- core/webhook/service_test.go
- internal/api/v1beta1connect/webhook.go
- docs/content/docs/reference/cli.mdx
- internal/reconcile/webhook.go
- internal/reconcile/webhook_reconciler_test.go
- core/webhook/service.go
- internal/reconcile/webhook_reconciler.go
| - The URL must be a valid absolute 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. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the HTTP(S)-only URL constraint.
The service validates absolute http:// and https:// URLs, but this wording could imply that other absolute schemes are valid. Please state the supported schemes explicitly.
Proposed wording
-- The URL must be a valid absolute URL, and it is the identity.
+- The URL must be a valid absolute HTTP(S) URL, and it is the identity.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - The URL must be a valid absolute 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. | |
| - 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. |
2 participants
What
Adds a
Webhookkind to the reconcile flow, so webhook endpoints are managed from adesired-state file like platform users, permissions, roles, and preferences. Also adds the
server-side validation that lets the kind round-trip cleanly.
How the kind behaves
Follows the rules in RFC 0001. A webhook is an object keyed by its URL.
needs
delete: true.value under the one field model: a field written in the file is used as-is, and an omitted
field takes its default, nothing is merged from the server.
subscribed_eventsis the full desired set: an empty or omitted list means all events (theserver default), deduplicated and compared as a set.
descriptiondefaults to empty, so omitting it clears any description on the server.statedefaults toenabled, the state the server gives a new endpoint, so omitting itconverges the endpoint to enabled.
on read, so it is never in the file, a plan, or an export.
Files from
frontier exportwrite every managed field, so the normal edit-the-export workflowis unaffected. Only a hand-trimmed file that drops a field sees it reset to the default (an
omitted state re-enables a disabled endpoint, an omitted description clears it). This mirrors
the Role kind's field model from #1779.
Server-side validation (closes the round-trip gap)
The reconcile flow treats the URL as an endpoint's identity and manages state as
enabled/disabled. For export to round-trip over every reachable server state, the server must
not hold values the reconciler cannot represent. So the webhook service now validates on create
and update:
enabledordisabled(or empty, which defaults toenabled),The URL is trimmed before it is validated, stored, and compared, so the server and the
reconciler agree on the same value. Invalid input returns
InvalidArgument, and a duplicateURL returns
AlreadyExists. The handler maps webhook errors to status codes across create,update, list, and delete, so for example deleting a missing webhook reads as
NotFoundinstead of a generic internal error.
Uniqueness is enforced in the service. A DB unique index on
webhook_endpoints.urlwould be astronger guarantee (the service check is a best-effort fast path with a clear error, and has a
narrow check-then-write race); that index is a follow-up, since it is a migration with
prod-data implications. Either way, a server that already holds duplicate or malformed rows
from before this change would need a one-time cleanup.
Changes
internal/reconcile/webhook.go,webhook_reconciler.go: the kind, its diff (each fieldresolved to the whole desired value over its default), export, the per-document
Validate,and one shared step that trims, validates, and dedupes the URLs.
cmd/reconcile.go: register the kind and update the help text.core/webhook/service.go: validate the URL (absolute, http/https), trim it, validate state,enforce URL uniqueness, with a service test.
internal/api/v1beta1connect/webhook.go: map validation errors toInvalidArgument/AlreadyExists/NotFoundacross create, update, list, and delete.Testing
go test ./internal/reconcile/... ./core/webhook/...passes, including the diff, apply,export round-trip, dedup, URL trimming, the http(s) scheme check, the description and state
default-converging cases, and the server-side validation cases.
go build,go vet,gofmt, andgolangci-lintare clean.Not in this PR
Two SSRF and uniqueness hardening items from review are deferred on purpose:
hard constraint. It is a migration with prod-data implications, so it is a separate change.
pre-existing delivery path and is a security change of its own, best done in a dedicated PR.
This PR does restrict the URL scheme to http(s).