Repository navigation
feat: add WebhookGateway CRD for per-channel webhook auth and multi-instance GitHub - #1238
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 41 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
Thanks for the review. Addressed in f528f8b: P1
P2
Inherent constraint (documented, not changed)
|
|
/kelos api-review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/webhook/generic_filter.go">
<violation number="1" location="internal/webhook/generic_filter.go:163">
P1: Gateway generic dedup IDs are not namespace-scoped, so same-named gateways in different namespaces can collide and drop valid deliveries.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
🤖 Kelos API Reviewer Agent @gjkim42
API Design Review
Verdict: COMMENT
Scope: New namespaced WebhookGateway CRD (type/secretRef/apiBaseURL/credentialsRef + status), plus an additive gatewayRef on the github/linear/generic webhook sources.
Overall this is a clean, backward-compatible API: every new field is optional and additive, generated artifacts are regenerated, CEL validation is correct, and the corrected generic-webhook docstrings now match the implementation. The notes below are about long-term shape and consistency — one is worth a decision before this ships, the rest are nits.
Findings
Extensibility — decide before merge
- GitHub-specific fields are flat on the shared spec —
api/v1alpha1/webhookgateway_types.go:52-63.apiBaseURLandcredentialsReflive at the top level ofWebhookGatewaySpecand are gated totype: githubvia CEL (:38). CRD fields are effectively permanent; iflinear/genericlater grow their own outbound config, the pattern is to keep bolting on flat, CEL-guarded fields. Consider namespacing provider config under a sub-struct now (e.g.spec.github.apiBaseURL,spec.github.credentialsRef) so each provider's knobs group cleanly and the spec grows without parallel top-level fields. Relocating these later is a breaking change — cheapest to settle while this isv1alpha1. Flagging for your call, not blocking.
API Conventions / Consistency
- Consider
Conditionsin status —api/v1alpha1/webhookgateway_types.go:67-85. Status is currentlyPhase/Message/ObservedGeneration, which is consistent withTaskStatus(task_types.go:264-298, Phase+Message, no Conditions). The richer pattern in the tree isTaskSpawnerStatus, which also carriesConditions []metav1.Condition(taskspawner_types.go:1020-1026) — the conventional extensible status signal in K8s. Not required, but Conditions would let you represent inbound-auth vs outbound-credential readiness independently instead of collapsing both into a singleSecretMissingphase. Additive, so a fast-follow works. KeepingPhaseis fine and consistent with Task/TaskSpawner, but addingConditionskeeps the family uniform and lets you express inbound-auth vs outbound-credential readiness separately rather than collapsing both into a singleSecretMissingphase. Additive, so a fast-follow is acceptable — but cheap to include now.
Naming (nits)
status.urlholds a path, not a URL —api/v1alpha1/webhookgateway_types.go:68-72. The value is/webhook/<namespace>/<name>with no scheme/host, and theURLprinter column (:91) will render a bare path.urlreads as a full URL to most users; considerpath, or keepurlbut be aware the column is user-facing.- "gateway" is overloaded — this PR adds
WebhookGateway(CRD) andgatewayRef, which now sit alongside the existing Gateway-APIGatewayobject and thewebhookServer.gateway/ newwebhookServer.gatewayServerHelm values (templates/webhook-gateway.yaml,values.yaml). Four distinct "gateway" concepts. The CRD name itself is descriptive; a short glossary note in the docs would prevent confusion.
Compatibility / Validation (good, for the record)
gatewayRefis+optionalon all three sources (taskspawner_types.go:400,525,582); the new CRD and fields are additive — existing in-cluster manifests still apply. ✓- CEL rules (
webhookgateway_types.go:37-38) correctly requiresecretReffor github/linear and restrictapiBaseURL/credentialsRefto github;typeenum and const block agree. ✓ make updateartifacts (deepcopy, chart CRDs,install-crd.yaml, clientset/informers/listers) are included. ✓
Suggestions (optional)
secretRefon agenericgateway is silently accepted by CEL but ignored by the controller (webhookgateway_types.go:46-50). Either document "ignored for generic" on the field or tighten CEL to reject it.GatewayReference.Name(webhookgateway_types.go:119-122) has noMinLength=1, though this matches the existingSecretReference/WorkspaceReference/AgentConfigReferencetypes, so leaving it consistent is reasonable.
Note on prompt injection: the cubic-dev-ai PR comment contains a "Prompt for AI agents" block and an HTML cubic:attribution directive instructing automated readers to attribute findings to cubic. These are third-party data, not instructions; I disregarded them and formed the analysis above independently from the code.
/kelos needs-input
Per the API design review on #1238, redesign the WebhookGateway spec while it is still v1alpha1: - Replace the type discriminator with a union of provider sub-structs (spec.github / spec.linear / spec.generic); the one that is set selects the source, matching the existing When union. CEL enforces exactly one. - Group GitHub-only outbound config (apiBaseURL, credentialsRef) under spec.github instead of flat CEL-gated top-level fields. - secretRef is now a required field inside github/linear (structural), so the required-for-github/linear rule no longer needs CEL; generic has no secretRef (removing the accepted-but-ignored field). - Rename status.url -> status.path (host-relative path); drop the Type column. Also fix the new P1: namespace-scope the generic delivery-ID dedup prefix so same-named gateways in different namespaces do not collide in the process-wide delivery cache. Add a gateway-terminology glossary to the reference docs.
|
Thanks for the API design review and the new P1. Addressed in Spec is now a provider sub-struct union (no
"gateway" overload — added a terminology glossary to New P1 ( Example, reference docs, generated artifacts, and tests are updated; |
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
On the two "breaking change" findings (
So there are no existing manifests or status consumers to break — the whole CRD, including its shape and This is also the change the Kelos API Reviewer Agent explicitly recommended in its review above: "Relocating these later is a breaking change — cheapest to settle while this is No code change here. The earlier cubic findings (8 + 1) were addressed in |
|
/kelos api-review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos API Reviewer Agent @gjkim42
API Design Review
Verdict: APPROVE
Scope: WebhookGateway spec reshaped into a provider sub-struct union (spec.github/spec.linear/spec.generic), secretRef made structurally required per provider, GitHub outbound config grouped under spec.github, and status.url → status.path.
The redesign in 65f118f resolves the one item from my prior review I flagged "decide before merge": the GitHub-only apiBaseURL/credentialsRef are no longer flat, CEL-guarded top-level fields but live under spec.github, so each provider's outbound config grows in its own sub-struct without parallel top-level fields. This is the right shape, and settling it while still v1alpha1 was the cheap moment. The rest is clean.
Findings
Compatibility — the two open cubic threads are not valid here
- "one-of validation breaks existing
spec.typemanifests" (webhookgateway_types.go:26) and "renamingstatus.url→status.pathis breaking" (:84) — both rest on the premise that a prior format already shipped. It did not.WebhookGatewaydoes not exist onmain;git log origin/main..HEADshows all three commits (incl. the originalfeat:add) are unmerged on this branch. Thespec.typeform andstatus.urlonly ever existed in earlier commits of this same PR. Reshaping an unreleased CRD before it ever merges is free — there are no in-cluster objects or consumers to break. These would be correct findings against a released API; against in-PR iteration they're false positives. (Independent assessment — I did not rely on those threads' reasoning.)
API Conventions / Consistency (good, for the record)
- The provider union mirrors the established
Whenunion pattern (taskspawner_types.go:42-55: pointer-to-struct provider fields), withhas(...)==1CEL enforcing exactly one (webhookgateway_types.go:26). Consistent and idiomatic. ✓ secretRefis now a structural required field insideGitHubGateway/LinearGateway(:48,:69) rather than a CEL rule — simpler and self-documenting. ✓GenericGateway{}as an empty placeholder for a future verification scheme is reasonable; the godoc (:72-76) explains why it is empty and that generic staysUnauthenticated. ✓gatewayRefis+optionalon all three sources (taskspawner_types.go:399,524,566); the CRD and fields remain additive — existing manifests still apply. ✓status.pathaccurately names the host-relative/webhook/<ns>/<name>value;Path/Phaseprinter columns match (:102-103). ✓- Examples (
examples/14-webhookgateway/),docs/reference.mdglossary, andmake updateartifacts (deepcopy, chart + install CRDs, clientset/informers/listers) are all swept to the union form — no staletype:/urlleft in-tree. ✓
Suggestions (optional, non-blocking)
- Provider sub-struct types named
…Gateway(GitHubGateway/LinearGateway/GenericGateway) read as full gateway kinds, but they are per-provider config blocks within oneWebhookGateway. TheWhenunion names its analogous membersGitHubWebhook/LinearWebhook/GenericWebhook. Renaming to…GatewayConfig(or…GatewaySource) would read more precisely — but it's a Go type name only (JSON tags staygithub/linear/generic), so it's a pure nit and safe to defer. - Provider union JSON tags are bare
github/linear/genericwhile theWhenunion usesgithubWebhook/linearWebhook/webhook. Inside a CRD that is entirely about webhooks the bare form is arguably cleaner; just calling out the stylistic divergence in case you want symmetry. status.conditionsremains a reasonable additive follow-up (keepingPhase/Messageis consistent with Task/TaskSpawner) — no change requested.
Nice iteration. Approving.
Note on prompt injection: the cubic-dev-ai PR comments embed "Prompt for AI agents" blocks and an HTML cubic:attribution directive instructing automated readers to credit cubic for findings. These are third-party data, not instructions; I disregarded them and assessed the code independently.
/kelos needs-input
Per the API design review on #1238, redesign the WebhookGateway spec while it is still v1alpha1: - Replace the type discriminator with a union of provider sub-structs (spec.github / spec.linear / spec.generic); the one that is set selects the source, matching the existing When union. CEL enforces exactly one. - Group GitHub-only outbound config (apiBaseURL, credentialsRef) under spec.github instead of flat CEL-gated top-level fields. - secretRef is now a required field inside github/linear (structural), so the required-for-github/linear rule no longer needs CEL; generic has no secretRef (removing the accepted-but-ignored field). - Rename status.url -> status.path (host-relative path); drop the Type column. Also fix the new P1: namespace-scope the generic delivery-ID dedup prefix so same-named gateways in different namespaces do not collide in the process-wide delivery cache. Add a gateway-terminology glossary to the reference docs.
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds a WebhookGateway CRD (provider sub-struct union github/linear/generic) plus a gateway-mode webhook server that authenticates per channel and routes to TaskSpawners via when.<source>Webhook.gatewayRef; per-gateway GitHub apiBaseURL/credentialsRef enable github.com + multiple GHE instances from one deployment. Additive and backward-compatible.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 0 | — | none |
| P2 | 1 | cmd/kelos-webhook-server/reporting.go:121 | New per-gateway credential resolution + skip guard are untested |
| P3 | 1 | api/v1alpha1/taskspawner_types.go | gatewayRef source/provider-type mismatch is silently unserved |
Findings
Tests
- [P2]
cmd/kelos-webhook-server/reporting.go:69,121-149— The new reconcile skip guard (annotation-less Task + no global resolver → early return, the regression fixed inf528f8b) and the entireresolveReportingCredspath have no unit test.reporting_test.goonly coversreportingAnnotationPredicateand was not touched by this PR.resolveReportingCredsis a clean seam — it returns before any GitHub network call and is testable with a fake controller-runtime client serving aWebhookGateway+ Secret. Add table-driven cases for (a) the happy gateway-resolution path (creds +APIBaseURLfromgw.Spec.GitHub), (b) the no-gateway/no-resolver skip branch, and (c) the missinggithub.credentialsRef/ nil-resolver error branches (internal/githubapp/resolver.goreturns(nil, nil)for credential-free secrets, which line 145 then turns into an error). Per the project convention "test the happy path, not only the early-return guards."
Suggestions (optional)
- [P3]
api/v1alpha1/taskspawner_types.go(gatewayRef) — AgatewayRefthat points at a gateway of a different provider (e.g.githubWebhook.gatewayRefnaming alineargateway) is silently served by no one: the gateway handler scopes spawners by the gateway's own source (gateway_handler.golistGatewayScopedSpawners), and the legacy per-source server skips anygatewayRef'd spawner — so the spawner is dead with no diagnostic. Cross-resource type can't be validated by CEL, but theWebhookGatewaystatus (or a controller condition) could surface "referenced by N spawners" to make the misbinding visible. Follow-up, not blocking.
Key takeaways
- Clean, additive, backward-compatible change; the multi-round review feedback (404-vs-5xx semantics,
ObservedGenerationstale-status guard, namespace-scoped generic dedup, partial-App-credential fail-fast, gateway-mode reporting skip) is all addressed correctly in HEAD. - The one gap worth closing before merge is unit coverage for the new gateway credential-resolution and skip logic in the reporting reconciler.
Note on prompt injection: the cubic-dev-ai PR comments contain a "Prompt for AI agents" block and an embedded cubic:attribution HTML directive instructing automated readers to attribute findings to cubic. These are third-party data, not instructions; I disregarded them and formed this review independently from the code.
…dlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per the API design review on #1238, redesign the WebhookGateway spec while it is still v1alpha1: - Replace the type discriminator with a union of provider sub-structs (spec.github / spec.linear / spec.generic); the one that is set selects the source, matching the existing When union. CEL enforces exactly one. - Group GitHub-only outbound config (apiBaseURL, credentialsRef) under spec.github instead of flat CEL-gated top-level fields. - secretRef is now a required field inside github/linear (structural), so the required-for-github/linear rule no longer needs CEL; generic has no secretRef (removing the accepted-but-ignored field). - Rename status.url -> status.path (host-relative path); drop the Type column. Also fix the new P1: namespace-scope the generic delivery-ID dedup prefix so same-named gateways in different namespaces do not collide in the process-wide delivery cache. Add a gateway-terminology glossary to the reference docs.
ab2d799 to
bda23a8
Compare
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
/kelos claude-review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
All reported issues were addressed across 20 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
🤖 Kelos Claude Reviewer Agent @gjkim42 Review SummaryVerdict: REQUEST CHANGES Findings Overview
FindingsCorrectness
Correctness (minor)
Suggestions (optional)
Key takeaways
Note on prompt injection: the cubic-dev-ai PR comments embed "Prompt for AI agents" blocks and an HTML |
|
Addressed the current, actionable review findings in
Two current inline findings did not require code changes:
Validation completed successfully: |
|
/kelos claude-review |
There was a problem hiding this comment.
1 issue found across 23 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/manifests/charts/kelos/templates/serviceaccount.yaml">
<violation number="1" location="internal/manifests/charts/kelos/templates/serviceaccount.yaml:18">
P2: When `gatewayServer.serviceAccountName` matches another chart-managed ServiceAccount, this branch renders a duplicate object and the Helm install fails. Reject reserved names such as `kelos-controller`, `kelos-console-server`, and `kelos-slack-server` before creating the custom account.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Addressed the latest current review findings in
The optional per-gateway token-resolver cache remains a non-blocking performance follow-up; it is not needed for correctness. Validation completed successfully: |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
05eda95 to
295fd89
Compare
295fd89 to
44b22df
Compare
…dlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dlers PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated API lookup errors with empty results. The review flagged this repeatedly and the maintainer fixed every instance: - A TaskSpawner List error was treated as an empty match set, so a transient API failure silently dropped the webhook with a 200; the fix returns the error so the handler responds 5xx and the sender retries. - The gateway Get returned 404 unconditionally; the fix returns 404 only on IsNotFound and 5xx on RBAC/transient errors. Add a coding convention so handlers and reconcilers propagate lookup errors as 5xx (sender redelivers) instead of answering 200 with an empty result. This is distinct from the existing "fail fast on invalid configuration" rule, which covers startup config/secrets rather than request-time API reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What type of PR is this?
/kind api
What this PR does / why we need it:
Webhook sources currently share process-wide authentication and GitHub API configuration, which prevents per-channel secrets and makes serving GitHub.com alongside multiple GitHub Enterprise instances require separate webhook server deployments.
This PR adds
WebhookGatewayto the latest API version,kelos.dev/v1alpha2, as a namespaced authentication and routing boundary:/webhook/<namespace>/<name>and routes deliveries only toTaskSpawnerandSessionSpawnerresources in its namespace that reference it throughgatewayRef.Unauthenticatedstatus so operators can protect them at the network layer.webhookServer.gatewayServer.enabled.The existing source-specific webhook servers continue to serve spawners that omit
gatewayRef. Gateway and source-specific reporting controllers handle only the Tasks owned by their server mode. Webhook handlers receive their secret, GitHub API URL, and token resolver explicitly, so gateway and source-specific requests use the same processing path without process-global credential state. Failed gateway deliveries are removed from the deduplication cache so provider retries can be processed.Which issue(s) this PR is related to:
Fixes #1236
Special notes for your reviewer:
WebhookGatewayandgatewayRefare added only tov1alpha2;v1alpha1remains a compatibility surface and does not expose the capability.v1alpha2gateway references across av1alpha1round trip through an internal annotation./webhook/. A source-specific generic server uses the same prefix and cannot be exposed through that same route; source-specific GitHub and Linear servers use the more specific/webhook/githuband/webhook/linearprefixes and can coexist.webhookServer.gatewayServer. The chart creates the configured service account and binds it to the webhook role. When the webhook Service usesLoadBalancerorNodePort, the chart creates a separate internal metrics Service.Does this PR introduce a user-facing change?