webhook: Basic validation for multiple HTTPRoute Rule filters #506

cmluciano · 2020-12-16T20:04:07Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This is a skeleton admission webhook to validate duplicate HTTPRoute Rule.Filters

Which issue(s) this PR fixes:
Fixes #349

Notes For Reviewers

This PR still needs some further deploy config for actually setting up and using the webhook. I mainly wanted to add it early so we can validate if the logic looks sane.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-12-16T20:04:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cmluciano · 2020-12-16T20:04:31Z

/cc @hbagdi @robscott

robscott

Thanks for your work on this! Excited to see all the progress on this front!

pkg/admission/validator.go

pkg/admission/validator_test.go

pkg/admission/server.go

pkg/admission/validator.go

hbagdi

This is a good starting point!

pkg/admission/validator.go

pkg/admission/server.go

cmluciano · 2021-01-08T19:52:44Z

This is now ready for review @robscott @hbagdi

If you deploy the service-api crds and then ./deploy/webhook.yaml, it will configure the webhook. I also have two examples that you can test located under the deploy folder.

robscott

Thanks for all your work on this! Still need to test this out myself but it's looking really promising.

Dockerfile

cmd/controller/main.go

deploy/webhook.yaml

deploy/single_filter.yaml

deploy/webhook.yaml

pkg/admission/validator_test.go

robscott

Thanks for the work on this! I spun this up on a kind 1.20 cluster and the installation seemed to work until the webhook pod ran into a logging issue. Maybe we can build some form of basic e2e tests into this where it will spin up a kind cluster and apply the deploy yaml, the crd yaml, and some examples to make sure it all works well together.

.gitignore

deploy/admission_webhook.yaml

robscott · 2021-02-03T06:14:57Z

deploy/admission_webhook.yaml

+    spec:
+      containers:
+        - name: webhook
+          image: cmluciano/service-apis-admission:latest


I think you're already working on this, but just in case, here's a reminder to change the image used here.

hbagdi · 2021-02-04T12:15:40Z

@cmluciano This PR has had so many comments that it is hard to keep track of. There are a few issues from the early PR review that need fixing. Could you take a look at those?

k8s-ci-robot · 2021-03-25T20:08:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cmluciano
To complete the pull request process, please assign jpeach after the PR has been reviewed.
You can assign the PR to them by writing /assign @jpeach in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Christopher M. Luciano <cmluciano@isovalent.com>

k8s-ci-robot · 2021-03-25T20:35:52Z

@cmluciano: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-gateway-api-test	`a693f03`	link	`/test pull-gateway-api-test`
pull-gateway-api-verify	`a693f03`	link	`/test pull-gateway-api-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

robscott · 2021-03-30T04:06:18Z

Thanks for the work to update this @cmluciano! This feels like it's quite close, it just got tripped up by some of our API changes that made optional fields pointers.

danehans · 2021-03-30T16:01:11Z

Note that #597 intends to add a validation package that should be consumed by the webhook implementation.

danehans · 2021-04-09T17:04:22Z

@cmluciano PTAL at #597 that added a validation pkg. I have a branch that provides an example implementation for the admission controller. It would be great if this PR was updated, i.e. ValidateHTTPRoute(), to use the validation pkg. Ping me when this PR merges and I’ll submit a PR to add Gateway hostname validation to the admission controller. Does this make sense to you? cc: @robscott @bowei

hbagdi · 2021-04-12T20:05:21Z

I talked to @cmluciano today. I'll try to get this over the finish line this week.

A variation of this patch first appeared in kubernetes-sigs#506. This patch is derived from kubernetes-sigs#506 and has been reworked to include it in the validation package.

hbagdi · 2021-04-13T00:37:08Z

I'm reworking parts of this PR and breaking it up to get this over the finish line.
#606 is first of one such patch.
/hold

A variation of this patch first appeared in kubernetes-sigs#506. This patch is derived from kubernetes-sigs#506 and has been reworked to include it in the validation package. Co-authored-by: Christopher M. Luciano <cmluciano@us.ibm.com>

k8s-ci-robot · 2021-04-13T04:25:28Z

@cmluciano: 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.

A variation of this patch first appeared in kubernetes-sigs#506. This patch is derived from kubernetes-sigs#506 and has been reworked to include it in the validation package. Co-authored-by: Christopher M. Luciano <cmluciano@isovalent.com>

hbagdi · 2021-04-20T17:43:21Z

Superseded by #617

k8s-ci-robot requested review from danehans and hbagdi December 16, 2020 20:04

k8s-ci-robot requested a review from robscott December 16, 2020 20:04

cmluciano force-pushed the cml/reqmirrorwebhook branch from 3237166 to f3d9228 Compare December 17, 2020 16:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2020

robscott mentioned this pull request Dec 18, 2020

build: Run go test from Make #508

Merged

robscott reviewed Dec 18, 2020

View reviewed changes

howardjohn reviewed Dec 18, 2020

View reviewed changes

pkg/admission/validator.go Outdated Show resolved Hide resolved

cmluciano force-pushed the cml/reqmirrorwebhook branch from f3d9228 to d8aed04 Compare December 18, 2020 17:37

hbagdi reviewed Dec 21, 2020

View reviewed changes

pkg/admission/validator.go Outdated Show resolved Hide resolved

pkg/admission/server.go Show resolved Hide resolved

pkg/admission/server.go Outdated Show resolved Hide resolved

pkg/admission/server.go Show resolved Hide resolved

pkg/admission/server.go Outdated Show resolved Hide resolved

cmluciano mentioned this pull request Jan 5, 2021

Add test job for service-api tests kubernetes/test-infra#20281

Merged

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2021

cmluciano marked this pull request as ready for review January 8, 2021 19:50

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2021

cmluciano mentioned this pull request Jan 8, 2021

Implement a Validating Webhook #487

Closed

cmluciano force-pushed the cml/reqmirrorwebhook branch 2 times, most recently from 21c1758 to 7d72900 Compare January 8, 2021 21:51

robscott reviewed Jan 8, 2021

View reviewed changes

cmluciano force-pushed the cml/reqmirrorwebhook branch from 7d72900 to d2189f6 Compare January 11, 2021 20:08

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 11, 2021

cmluciano requested a review from robscott February 1, 2021 18:19

robscott reviewed Feb 3, 2021

View reviewed changes

cmluciano force-pushed the cml/reqmirrorwebhook branch from fca95ef to 3c1df19 Compare March 25, 2021 20:08

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2021

cmluciano force-pushed the cml/reqmirrorwebhook branch from 3c1df19 to f6c91f2 Compare March 25, 2021 20:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2021

webhook: Basic validation for multiple reqMirrors

a693f03

Signed-off-by: Christopher M. Luciano <cmluciano@isovalent.com>

cmluciano force-pushed the cml/reqmirrorwebhook branch from f6c91f2 to a693f03 Compare March 25, 2021 20:31

hbagdi mentioned this pull request Mar 30, 2021

Introduces a Validation Package #597

Merged

hbagdi mentioned this pull request Apr 13, 2021

httproute: add duplicate filter validation #606

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2021

hbagdi mentioned this pull request Apr 20, 2021

implement an admission webhook server #617

Merged

hbagdi closed this Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webhook: Basic validation for multiple HTTPRoute Rule filters #506

webhook: Basic validation for multiple HTTPRoute Rule filters #506

cmluciano commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

cmluciano commented Dec 16, 2020

robscott left a comment

hbagdi left a comment

cmluciano commented Jan 8, 2021

robscott left a comment

robscott left a comment

robscott Feb 3, 2021

hbagdi commented Feb 4, 2021

k8s-ci-robot commented Mar 25, 2021

k8s-ci-robot commented Mar 25, 2021

robscott commented Mar 30, 2021

danehans commented Mar 30, 2021

danehans commented Apr 9, 2021

hbagdi commented Apr 12, 2021

hbagdi commented Apr 13, 2021

k8s-ci-robot commented Apr 13, 2021

hbagdi commented Apr 20, 2021 •

edited

Loading

webhook: Basic validation for multiple HTTPRoute Rule filters #506

webhook: Basic validation for multiple HTTPRoute Rule filters #506

Conversation

cmluciano commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

cmluciano commented Dec 16, 2020

robscott left a comment

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

cmluciano commented Jan 8, 2021

robscott left a comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

robscott Feb 3, 2021

Choose a reason for hiding this comment

hbagdi commented Feb 4, 2021

k8s-ci-robot commented Mar 25, 2021

k8s-ci-robot commented Mar 25, 2021

robscott commented Mar 30, 2021

danehans commented Mar 30, 2021

danehans commented Apr 9, 2021

hbagdi commented Apr 12, 2021

hbagdi commented Apr 13, 2021

k8s-ci-robot commented Apr 13, 2021

hbagdi commented Apr 20, 2021 • edited Loading

hbagdi commented Apr 20, 2021 •

edited

Loading