Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement an admission webhook server #617

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

hbagdi
Copy link
Contributor

@hbagdi hbagdi commented Apr 20, 2021

Co-authored-by: Christopher M. Luciano cmluciano@isovalent.com

What type of PR is this?

/kind feature
What this PR does / why we need it:

This PR introduces a new admission webhook server that will be used for advanced validation functionality.
Which issue(s) this PR fixes:

Fixes #349

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 20, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hbagdi

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 20, 2021
@hbagdi
Copy link
Contributor Author

hbagdi commented Apr 20, 2021

The scope of this PR is to get this merged into mainline and then work on documentation, otherwise the scope of this work keeps on increasing and it gets very hard to actually land in the mainline. I had to invest significant amount of time reworking major area over #506 to get this working again.

How can a reviewer test this out?

Bad HTTPRoute: https://gist.github.com/hbagdi/b7d7cfde79610133846edf2644530e6c, create a local file out of the gist.

make crd
kubectl apply -f admission_webhook.yaml
kubectl apply -f certificate_config.yaml 
# you might have to restart the admission-server pod sometimes, the ordering is not there yet
kubectl apply -f path/to/bad-httproute.yaml
# this should show the right error, if you get an error for TLS, make sure the jobs in gateway-api namespace are completed and restart the server pod

deploy/admission_webhook.yaml Outdated Show resolved Hide resolved
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

This is very cool, thanks for all the work on this @hbagdi and @cmluciano! Excited to see this getting so close.

deploy/admission_webhook.yaml Outdated Show resolved Hide resolved
deploy/admission_webhook.yaml Outdated Show resolved Hide resolved
pkg/admission/server.go Outdated Show resolved Hide resolved
pkg/admission/server.go Outdated Show resolved Hide resolved
@robscott
Copy link
Member

Tested this out in a new cluster and it worked flawlessly, nicely done!

@hbagdi hbagdi force-pushed the feat/admission-webhook branch from a95b218 to 1104444 Compare April 20, 2021 20:29
@hbagdi
Copy link
Contributor Author

hbagdi commented Apr 20, 2021

@cmluciano @robscott I simplified the code to make the behavior across Create and Update operation exactly same. I don't think we have a case (yet) that requires validation to be different for update and create. Does that feel right?

@bowei @danehans Since this is an initial PR for some code that is going to grow in future, can you also review this?

@hbagdi hbagdi force-pushed the feat/admission-webhook branch from 1104444 to 8dab9ef Compare April 20, 2021 20:46
@robscott
Copy link
Member

LGTM, adding a hold until we can get an official image published.

/hold

@k8s-ci-robot 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 20, 2021
@hbagdi hbagdi force-pushed the feat/admission-webhook branch from 8dab9ef to 75129ff Compare April 21, 2021 21:50
@hbagdi
Copy link
Contributor Author

hbagdi commented Apr 21, 2021

@robscott I removed the docker image reference to TODO.
I considered adding in a comment or some documentation around 'this is still a work in progress' but couldn't find a suitable place for it.
Since we don't document the webhook server anywhere, I don't expect any users to run into it. Some contributors to this project might run into it and that would be a good place to have more conversations and seek contribution.

I'm open to adding more here if necessary.
If we can get another review and get this merged in, that would be helpful.

@robscott
Copy link
Member

Thanks @hbagdi! I think this is good to go no so we can unblock the follow up PRs. Removing the hold and will leave the final LGTM for @danehans.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2021
@robscott
Copy link
Member

/cc @danehans

@k8s-ci-robot k8s-ci-robot requested a review from danehans April 21, 2021 22:42
spec:
containers:
- name: webhook
# TODO(hbagdi): Swap image name to the k8s official image
Copy link
Contributor

Choose a reason for hiding this comment

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

@hbagdi I think it would be helpful include an xref the issue/pr in this TODO comment. Is it #621?

}

func handleValidation(request admission.AdmissionRequest) (
*admission.AdmissionResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a new line for the returned types?

Copy link
Contributor

@cmluciano cmluciano left a comment

Choose a reason for hiding this comment

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

One comment about testing v1beta1 submissions but otherwise lgtm. Thank you for carrying this forward :D


func TestServeHTTPSubmissions(t *testing.T) {
for _, apiVersion := range []string{
"admission.k8s.io/v1beta1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to accept this if we use only setup webhook v1 validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to v1 only.

Co-authored-by: Christopher M. Luciano <cmluciano@isovalent.com>
@hbagdi hbagdi force-pushed the feat/admission-webhook branch from 75129ff to c3807e6 Compare April 22, 2021 20:38
@danehans
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit ea6b404 into kubernetes-sigs:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPRequestMirrorFilter needs webhook validation
5 participants