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 29804701004Coverage increased (+0.4%) to 46.587%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
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.
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.subscribed_eventsis the fulldesired set: an empty or omitted list means all events (the server default), and it is
deduplicated and compared as a set.
descriptionandstateare managed the ordinary way.on read, so it is never in the file, a plan, or an export.
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),Invalid input now returns
InvalidArgument, and a duplicate URL returnsAlreadyExists, insteadof the previous generic internal error. Uniqueness is enforced in the service; a DB unique index
on
webhook_endpoints.urlwould be a stronger guarantee if maintainers prefer, and either way aserver 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, and export.cmd/reconcile.go: register the kind and update the help text.core/webhook/service.go: validate URL and state, enforce URL uniqueness, with a service test.internal/api/v1beta1connect/webhook.go: map validation errors toInvalidArgument/AlreadyExists.Testing
go test ./internal/reconcile/... ./core/webhook/...passes, including the diff, apply,export round-trip, dedup, and the new server-side validation cases.
go build,go vet,gofmt, andgolangci-lintare clean.Follow-up
When the
Validate()interface change (#1776) merges, this kind needs the sameper-document
Validate()method to satisfy the interface. It is a small method mirroring theother kinds, added when this branch rebases on that change.