Skip to content

Conversation

@ronething
Copy link
Contributor

@ronething ronething commented Sep 28, 2025

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

This PR adds more webhook validations. The resources for adding validations are HTTPRoute, GRPCRoute, TCPRoute, Gateway, and Ingress.

This PR has also added corresponding unit tests, while e2e tests have not been supplemented yet.

ref: #2580

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
@ronething ronething marked this pull request as ready for review September 28, 2025 07:49
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
@ronething ronething requested a review from AlinsRan September 29, 2025 04:17
@ronething ronething requested a review from Copilot September 29, 2025 05:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds webhook validation for secret and service resource references in HTTPRoute, GRPCRoute, TCPRoute, Gateway, and Ingress resources. The validation helps catch misconfigurations by warning users when referenced resources (Services/Secrets) don't exist in the cluster.

Key changes implemented:

  • Webhook validators for Gateway API resources (HTTPRoute, GRPCRoute, TCPRoute) that check service references
  • Enhanced Ingress webhook validator to check both service and secret references
  • Enhanced Gateway webhook validator to check TLS certificate secret references
  • Ownership checking logic to determine if resources are managed by the APISIX controller

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/webhook/v1/httproute_webhook.go New HTTPRoute webhook validator with service reference checking
internal/webhook/v1/grpcroute_webhook.go New GRPCRoute webhook validator with service reference checking
internal/webhook/v1/tcproute_webhook.go New TCPRoute webhook validator with service reference checking
internal/webhook/v1/gateway_webhook.go Enhanced Gateway validator with TLS secret reference checking
internal/webhook/v1/ingress_webhook.go Enhanced Ingress validator with service/secret reference checking
internal/webhook/v1/ownership.go New ownership checking logic for Gateway API resources
internal/manager/webhooks.go Registration of new webhook validators
config/webhook/manifests.yaml Webhook configuration manifests

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}
if err := webhookv1.SetupTCPRouteWebhookWithManager(mgr); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since UDPRoute PR is merged. Should we add that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but i think we can handle it in the next PR. This PR is getting more and more inflated.

@ronething ronething merged commit ec81917 into master Sep 29, 2025
23 checks passed
@ronething ronething deleted the feat/add_checker branch September 29, 2025 06:15
@ronething ronething mentioned this pull request Sep 29, 2025
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants