Skip to content

Repo webhook move - #1129

Draft
efiacor wants to merge 6 commits into
kptdev:mainfrom
Nordix:repo_webhook_move
Draft

Repo webhook move#1129
efiacor wants to merge 6 commits into
kptdev:mainfrom
Nordix:repo_webhook_move

Conversation

@efiacor

@efiacor efiacor commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Move Repository validating webhook to repository controller


Description

  • What changed: Moved the Repository validating webhook from the centralized apiserver to the repository controller, mirroring the design already established for the PackageRevision validating webhook. This decouples webhook logic from the apiserver and aligns both webhooks under their respective controller domains.

  • Why it's needed:

    • Separation of concerns: webhooks belong with the controllers they validate
    • Consistency: PackageRevision already follows this pattern
    • Scalability: allows independent webhook evolution and testing per controller
    • Fixed deployment issue: with both webhooks previously using the same resource name (validating-webhook-configuration), only one was being deployed. Now each has a unique name via the //+kubebuilder:webhookconfiguration: marker.
  • How it works:

    • Repository webhook validation logic now lives in controllers/repositories/pkg/webhooks/repository_webhook.go
    • Repository controller registers the webhook on its webhook server in config.Init()
    • Each controller generates its own webhook manifest with a unique resource name via code generation markers
    • Removed centralized webhook handling from pkg/apiserver/webhooks.go (no longer needed)

Related Issue(s)

  • Relates to: webhook consolidation and controller architecture alignment

Type of Change

  • Refactor
  • Enhancement

Checklist

  • Code follows project style guidelines
  • Self-reviewed changes
  • Tests added/updated (repository_webhook_test.go with existing test suite)
  • All tests and gating checks pass

Testing Instructions

  1. Deploy porch with the updated code: make run-in-kind-v1alpha2
  2. Verify both webhooks are deployed: kubectl get validatingwebhookconfigurations -n porch-system
    • Should see: packagerevision-validating-webhook-configuration and repository-validating-webhook-configuration
  3. Create a test repository to verify webhook validation: porchctl repo add ...
  4. Test conflict detection (duplicate git location) to confirm webhook is active

Additional Notes

  • Generated files: Webhook manifests are auto-generated via make generate using controller-gen markers
  • RBAC: Each controller maintains its own RBAC role via //+kubebuilder:rbac: markers
  • Deployment change: The porch-server deployment no longer includes webhook setup; webhooks are now hosted on the porch-controllers pod

AI Disclosure

  • I have used AI in the creation of this PR.
    • Kiro used for code refactoring, marker configuration

@efiacor efiacor self-assigned this Jul 22, 2026
@efiacor
efiacor requested review from a team July 22, 2026 15:06
@netlify

netlify Bot commented Jul 22, 2026

Copy link
Copy Markdown

Deploy Preview for kpt-porch ready!

Name Link
🔨 Latest commit 0645929
🔍 Latest deploy log https://app.netlify.com/projects/kpt-porch/deploys/6a6371f3d478730008359942
😎 Deploy Preview https://deploy-preview-1129--kpt-porch.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@dosubot dosubot Bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jul 22, 2026
@github-actions
github-actions Bot requested a review from Copilot July 22, 2026 15:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Moves Repository validating webhook responsibility from the centralized apiserver into the repositories controller, aligning architecture with the existing PackageRevision webhook and ensuring distinct ValidatingWebhookConfiguration names.

Changes:

  • Removed apiserver-hosted repository webhook implementation and its tests.
  • Added controller-runtime admission webhook handler + tests under the repositories controller, and registered it on the controller webhook server.
  • Updated controller-gen markers and generated webhook manifests to use unique ValidatingWebhookConfiguration names; adjusted porch-server deployment to stop exposing/setting up webhooks.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/apiserver/webhooks.go Removes centralized repository webhook server + cert management + conflict logic.
pkg/apiserver/webhooks_test.go Removes tests tied to the deleted apiserver webhook implementation.
pkg/apiserver/apiserver.go Removes webhook setup from porch-server runtime.
deployments/porch/3-porch-server.yaml Removes webhook cert volume/env/port from porch-server deployment.
controllers/repositories/pkg/webhooks/repository_webhook.go Adds RepositoryValidator admission handler + conflict detection helpers.
controllers/repositories/pkg/webhooks/repository_webhook_test.go Adds unit tests for validator handler + conflict helpers.
controllers/repositories/pkg/controllers/repository/config.go Registers repository webhook on the manager webhook server.
controllers/repositories/pkg/controllers/repository/repository_controller.go Adds controller-gen markers to generate a uniquely named ValidatingWebhookConfiguration.
controllers/repositories/config/webhook/manifests.yaml Adds generated ValidatingWebhookConfiguration manifest for repositories.
controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go Adds webhookconfiguration marker for unique Packagerevision VWC name generation.
controllers/packagerevisions/config/webhook/manifests.yaml Renames Packagerevision VWC to a unique name in generated manifest.
go.mod Moves fsnotify to indirect (no longer directly used after apiserver webhook removal).
.vscode/launch.json Removes webhook-related env vars from launch configs.

Comment thread controllers/repositories/pkg/controllers/repository/config.go Outdated
Comment thread controllers/repositories/pkg/webhooks/repository_webhook.go
Comment thread controllers/repositories/pkg/webhooks/repository_webhook_test.go
Comment thread controllers/repositories/pkg/controllers/repository/config.go Outdated
efiacor added 3 commits July 24, 2026 08:55
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
- Use GetAPIReader() instead of GetClient() for strong consistency during admission validation (prevents cache-induced race conditions)
- Scope repository list query to namespace level to reduce admission latency at scale
- Test UPDATE operation semantics explicitly for the 'updating self' test case
- Fix misleading documentation comment to link to actual deployment config

All unit tests pass. No lint issues.

Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
@efiacor
efiacor force-pushed the repo_webhook_move branch from 1e111cc to 280786b Compare July 24, 2026 08:17
efiacor added 3 commits July 24, 2026 12:57
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
@sonarqubecloud

Copy link
Copy Markdown

@dosubot dosubot Bot added the lgtm #ededed label Jul 27, 2026
@efiacor
efiacor marked this pull request as draft July 27, 2026 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm #ededed size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants