Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the in-process SCM webhook receiver to (1) verify inbound webhook authenticity using secrets stored on ScmProvider/ClusterScmProvider Secrets and (2) accelerate WebRequestCommitStatus (WRCS) reconciles by fanning out repository-scoped webhook events to matching WRCS resources, with optional per-WRCS payload filtering. It also updates generated API types/manifests/docs to reflect the new configuration and behavior, and bumps the dashboard’s react-router-dom.
Changes:
- Add webhook signature/token verification (
webhookSecret/webhookSignatureHeader) with an optional fail-closed “strict mode” configured viaControllerConfiguration.spec.webhookReceiver.strict. - Add WRCS webhook acceleration: repo identity parsing, repo→GitRepository→PromotionStrategy→WRCS fan-out, and optional
mode.webhook.filter.expressionevaluation. - Update generated CRDs/install bundles/applyconfigs/view types and documentation; bump
react-router-domto v7 and require Node >= 20 for the dashboard package.
Reviewed changes
Copilot reviewed 31 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/shared/src/types/generated/view.gen.ts | Updates generated View API TS types/docs for mode.webhook and webhook filter schemas. |
| ui/dashboard/package.json | Bumps react-router-dom to v7 and adds engines.node >=20. |
| ui/dashboard/package-lock.json | Locks dependency graph for the router v7 upgrade (and new transitive deps). |
| internal/webhookreceiver/signature.go | Implements secret/header resolution and signature verification logic. |
| internal/webhookreceiver/signature_test.go | Adds unit tests for signature verification, secret extraction, and filter evaluation. |
| internal/webhookreceiver/server.go | Adds strict-mode reading, webhook auth gating, repo parsing, and WRCS fan-out/filtering. |
| internal/webhookreceiver/server_test.go | Extends webhook receiver tests to cover WRCS fan-out, strict mode, and auth behavior. |
| internal/webhookreceiver/filter.go | Adds compiled-expression caching and evaluation for webhook payload filters. |
| internal/utils/utils.go | Adds exported helper to resolve ScmProvider+Secret from a GitRepository; validates secretRef presence. |
| internal/utils/gitrepository_repokey.go | Adds normalized repo-key helpers for provider-agnostic matching/indexing. |
| internal/utils/gitrepository_repokey_test.go | Adds unit tests for repo-key derivation across supported providers. |
| internal/settings/manager.go | Adds settings accessor for ControllerConfiguration.spec.webhookReceiver.strict. |
| internal/controller/webrequestcommitstatus_controller.go | Adds an external enqueue channel and exposes enqueue func for webhook-driven reconciles. |
| internal/controller/webrequestcommitstatus_controller_test.go | Adds integration test asserting webhook fan-out triggers a WRCS reconcile/HTTP call. |
| internal/controller/testdata/WebRequestCommitStatus.yaml | Documents mode.webhook.filter.expression usage in testdata YAML. |
| internal/controller/testdata/ControllerConfiguration.yaml | Documents spec.webhookReceiver.strict in testdata config. |
| internal/controller/suite_test.go | Wires WRCS enqueue func into the webhook receiver in controller test suite. |
| internal/controller/promotionstrategy_controller.go | Adds field index for PromotionStrategy→GitRepository reference to support fan-out queries. |
| internal/controller/gitrepository_controller.go | Adds field index for GitRepository repoKey to support repo-identity lookups. |
| internal/controller/fieldindex.go | Introduces constants for new field-index paths used across controllers/receiver. |
| docs/security.md | Documents webhook receiver hardening, secret keys, and strict mode behavior. |
| docs/roadmap.md | Updates roadmap item wording to reflect broader verified webhook handling. |
| docs/getting-started.md | Adds getting-started guidance to configure webhook secrets for production. |
| docs/gating-promotions/built-in-gates/web-request-commit-status/index.md | Documents WRCS webhook acceleration and mode.webhook.filter. |
| docs/crd-specs.md | Updates WRCS description to include the optional webhook filter/accelerator. |
| dist/install-without-ui.yaml | Regenerates install bundle with new CRD schema and controller config defaults. |
| dist/install-with-dashboard-cert-manager.yaml | Regenerates install bundle with new CRD schema and controller config defaults. |
| dist/install-with-dashboard-byo-cert.yaml | Regenerates install bundle with new CRD schema and controller config defaults. |
| config/crd/bases/promoter.argoproj.io_webrequestcommitstatuses.yaml | Regenerates WRCS CRD with mode.webhook.filter.expression schema. |
| config/crd/bases/promoter.argoproj.io_controllerconfigurations.yaml | Regenerates ControllerConfiguration CRD with spec.webhookReceiver.strict. |
| config/config/controllerconfiguration.yaml | Adds webhookReceiver.strict to the default ControllerConfiguration manifest. |
| cmd/main.go | Wires WRCS enqueue func + settings manager into the webhook receiver in the main controller binary. |
| applyconfiguration/utils.go | Extends applyconfiguration kind mapping for new webhook-related types. |
| applyconfiguration/api/v1alpha1/webhookreceiverconfiguration.go | Adds generated applyconfiguration for webhook receiver config. |
| applyconfiguration/api/v1alpha1/webhookmodespec.go | Adds generated applyconfiguration for WRCS webhook mode. |
| applyconfiguration/api/v1alpha1/webhookfilterspec.go | Adds generated applyconfiguration for webhook filter spec. |
| applyconfiguration/api/v1alpha1/modespec.go | Updates ModeSpec applyconfiguration to include the new webhook field/docs. |
| applyconfiguration/api/v1alpha1/controllerconfigurationspec.go | Updates ControllerConfigurationSpec applyconfiguration with webhookReceiver. |
| api/view/v1alpha1/zz_generated.openapi.go | Updates generated OpenAPI to include new webhook-related schemas/fields. |
| api/v1alpha1/zz_generated.model_name.go | Adds OpenAPI model names for new webhook-related API types. |
| api/v1alpha1/zz_generated.deepcopy.go | Adds deepcopy support for new webhook-related API types/fields. |
| api/v1alpha1/webrequestcommitstatus_types.go | Adds mode.webhook and new webhook filter/mode structs to WRCS API. |
| api/v1alpha1/controllerconfiguration_types.go | Adds spec.webhookReceiver to ControllerConfiguration API. |
| api/v1alpha1/constants.go | Adds ScmProvider Secret key constants for inbound webhook verification. |
Files not reviewed (9)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/v1alpha1/zz_generated.model_name.go: Generated file
- applyconfiguration/api/v1alpha1/controllerconfigurationspec.go: Generated file
- applyconfiguration/api/v1alpha1/modespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookfilterspec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookmodespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookreceiverconfiguration.go: Generated file
- applyconfiguration/utils.go: Generated file
- ui/dashboard/package-lock.json: Generated file
Bundle ReportBundle size has no change ✅ |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 44 changed files in this pull request and generated 2 comments.
Files not reviewed (9)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/v1alpha1/zz_generated.model_name.go: Generated file
- applyconfiguration/api/v1alpha1/controllerconfigurationspec.go: Generated file
- applyconfiguration/api/v1alpha1/modespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookfilterspec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookmodespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookreceiverconfiguration.go: Generated file
- applyconfiguration/utils.go: Generated file
- ui/dashboard/package-lock.json: Generated file
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1817 +/- ##
==========================================
+ Coverage 60.83% 61.44% +0.60%
==========================================
Files 138 142 +4
Lines 11105 11404 +299
Branches 457 458 +1
==========================================
+ Hits 6756 7007 +251
- Misses 4274 4325 +51
+ Partials 75 72 -3 ☔ View full report in Codecov by Harness. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 44 changed files in this pull request and generated no new comments.
Files not reviewed (9)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/v1alpha1/zz_generated.model_name.go: Generated file
- applyconfiguration/api/v1alpha1/controllerconfigurationspec.go: Generated file
- applyconfiguration/api/v1alpha1/modespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookfilterspec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookmodespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookreceiverconfiguration.go: Generated file
- applyconfiguration/utils.go: Generated file
- ui/dashboard/package-lock.json: Generated file
Comments suppressed due to low confidence (3)
api/v1alpha1/controllerconfiguration_types.go:95
ControllerConfigurationSpec.WebhookReceiveris added as a required, non-omitempty field. That makes upgrading potentially breaking for existing ControllerConfiguration manifests that don’t includespec.webhookReceiver, because they will fail validation on the next apply/update. If strict defaults to false, consider making this field optional (pointer +omitempty) or adding a structural default for the object itself (e.g.+kubebuilder:default={}) so older manifests remain valid without edits.
// WebhookReceiver contains configuration for the inbound SCM webhook receiver
// (signature verification and related fail-closed behavior).
// +required
WebhookReceiver WebhookReceiverConfiguration `json:"webhookReceiver"`
}
internal/webhookreceiver/signature.go:38
webhookSignatureHeaderis read directly from Secret data without trimming whitespace. If the Secret value is created from a file or copy/paste with a trailing newline/space, header lookup will never match and valid webhooks will be rejected with 401. Trim surrounding whitespace on the header name before falling back to the default.
internal/webhookreceiver/server.go:47- The field-index path strings are duplicated here to avoid an import cycle. This creates a silent drift risk: if
internal/controllerrenames any of these index paths, webhook verification / WRCS fan-out will stop matching resources with no compile-time signal. Consider moving these index-path constants to a small shared package (e.g.internal/indexfieldsorapi/v1alpha1) that both controller and webhookreceiver can import, so they stay in sync.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 44 changed files in this pull request and generated 1 comment.
Files not reviewed (9)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/v1alpha1/zz_generated.model_name.go: Generated file
- applyconfiguration/api/v1alpha1/controllerconfigurationspec.go: Generated file
- applyconfiguration/api/v1alpha1/modespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookfilterspec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookmodespec.go: Generated file
- applyconfiguration/api/v1alpha1/webhookreceiverconfiguration.go: Generated file
- applyconfiguration/utils.go: Generated file
- ui/dashboard/package-lock.json: Generated file
Comments suppressed due to low confidence (3)
internal/webhookreceiver/server.go:693
enqueueWRCSForRepologs an Info-level message for every WRCS enqueued. On busy repos or many WRCS, this can generate high-volume logs from every webhook delivery and make signal/noise worse. Consider lowering this to a debug verbosity (or sampling) and keep Info for exceptional conditions only.
docs/gating-promotions/built-in-gates/web-request-commit-status/index.md:94- This sentence claims deliveries without a parseable repository identity are rejected with 401 when any
webhookSecretis configured. In the implementation, missing repository identity is only rejected whenControllerConfiguration.spec.webhookReceiver.strictis enabled; otherwise the delivery can still be accepted (and the CTP SHA path may still run). The docs here should match the strict vs non-strict behavior described indocs/security.md.
When at least one ScmProvider Secret for the webhook’s repository has `webhookSecret` set, the receiver requires a valid signature (or shared token) before enqueueing WRCS or ChangeTransferPolicy work. Values with a `sha256=` prefix are verified as HMAC-SHA256 of the raw body; other values are compared as a shared token (constant-time). If no matching Secret configures `webhookSecret`, verification is skipped for backward compatibility. Deliveries without a parseable repository identity cannot be verified or fan out to WRCS; when any `webhookSecret` is configured they are rejected with **401** so CTP is not enqueued either. See [Webhook receiver hardening](../../../security.md#webhook-receiver-hardening).
internal/utils/utils.go:108
- The new
secretRef is required...error message is usingsecretNamespace(which is the namespace the Secret would live in), not the ScmProvider object's own namespace. ForClusterScmProviderthis can be confusing because the provider itself is cluster-scoped, while the secret lives incontrollerNamespace. Consider reporting the ScmProvider object identity in the message, and (optionally) include the expected secret namespace separately.
Merged
Extend the SCM webhook receiver to verify inbound deliveries using Provider secret keys (webhookSecret, webhookSignatureHeader) when ScmProvider.inboundWebhookVerification is RequireVerification, and to fan out repository-scoped webhooks to matching WebRequestCommitStatus resources with optional mode.webhook.filter expression filtering. Retain webhook secret keys in the Secret informer transform so verification works against the cached client used in production. Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
zachaller
force-pushed
the
wrcs-webhhook-support-1
branch
from
August 27, 2026 21:04
06fb828 to
b931188
Compare
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
…wrcs-webhhook-support-1
…wrcs-webhhook-support-1
…wrcs-webhhook-support-1 Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.