Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Initial implementation of validation webhook #441

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

richardwxn
Copy link
Contributor

@richardwxn richardwxn commented Oct 22, 2019

Changes include:

  1. Setup webhook server and register the validation path with self defined handler.
  2. Implemented the validation logic towards CR admission request, which is using the validation logic of IstioControlPlaneSpec
    3..Create configs for ValidatingWebhookConfiguration, Certificate/Issuer and update the corresponding operator service deployment

Update at 11/12:
Thanks for the help of @martonsereg , we update the matching rules of webhook configuration to be plural form of IstioControlPlane, otherwise the admission request would not trigger the validation call.

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 22, 2019
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 22, 2019
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2019
pkg/webhook/istiocontrolplane/server.go Outdated Show resolved Hide resolved
deploy/webhook.yaml Show resolved Hide resolved
deploy/service.yaml Outdated Show resolved Hide resolved
deploy/cert.yaml Show resolved Hide resolved
@istio-policy-bot istio-policy-bot added the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Oct 30, 2019
@richardwxn richardwxn removed the lifecycle/needs-triage Indicates a new PR or issue needs to be triaged label Oct 31, 2019
@richardwxn richardwxn removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 13, 2019
@richardwxn richardwxn changed the title [WIP] Initial implementation of validation webhook Initial implementation of validation webhook Nov 13, 2019
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 13, 2019
@richardwxn richardwxn marked this pull request as ready for review November 13, 2019 00:11
@richardwxn richardwxn requested a review from a team as a code owner November 13, 2019 00:11
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 13, 2019
@istio-testing
Copy link

@richardwxn: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
e2e_operator c93af51 link /test e2e_operator

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

What is the motivation for making a Webhook? We can do open API schema validation which gets us everything we want here, with way less complexity.

Two 2 webhooks we already have (galley/injector) already cause us a ton of pain, I am very hesitant to add another unless it is absolutely needed

@richardwxn
Copy link
Contributor Author

@howardjohn I think the motivation here is that validation webhook can do more than openAPI schema check does(basic type, pattern, range checking). So now our custom validation logic is not doing much more than that, but it is being added more custom logic(I would add more details). Also this webhook is managed by controller-runtime manager, similar with controller, so we don't need to have extra deployment or so for it, making it simpler to maintain.

@howardjohn
Copy link
Member

@richardwxn I am not suggesting the openAPI check can do everything a webhook can do. Of course the webhook can do whatever validation it wants so its much more flexible. It is a huge pain to manage the certs and other issues involved though, so we shouldn't take it lightly to add a webhook.

OpenAPI and webhook can coexist too. In my opinion, we the best option is to start with openAPI schema (which is still useful even if we add a webhook later) and only add a webhook if there are clearly defined benefits that openAPI does not provide

@ayj
Copy link
Contributor

ayj commented Nov 14, 2019

+1 to @howardjohn's suggestion to start with OpenAPI schema first and only add the webhook later if the schema isn't sufficient.

@richardwxn
Copy link
Contributor Author

@howardjohn @ayj Yep, that makes sense. I would put this PR on hold for now and move the basic schema checking part to openAPI first.

@richardwxn richardwxn added the do-not-merge Block automatic merging of a PR. label Nov 14, 2019
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 12, 2020
@istio-testing
Copy link

@richardwxn: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge Block automatic merging of a PR. needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants