Skip to content
This repository has been archived by the owner on Feb 9, 2022. It is now read-only.

validator server as k8s external admission hook #1218

Closed
wants to merge 28 commits into from

Conversation

jmuk
Copy link
Contributor

@jmuk jmuk commented Sep 6, 2017

This PR adds the validation server as an external admission webhook
of kubernetes. This also introduce the structural validation
through protobuf definition.

The integration with referential validation is not yet done, that's
a TODO.

Also this adds a new subcommand to mixs. mixs validator will start
a webserver for the webhook.

Release note:

validator server is added as a subcommand of mixs. Only structural validations right now.

This change is Reviewable

This PR adds the validation server as an external admission webhook
of kubernetes. This also introduce the structural validation
through protobuf definition.

The integration with referential validation is not yet done, that's
a TODO.

Also this adds a new subcommand to mixs. mixs validator will start
a webserver for the webhook.
@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: douglas-reid

Assign the PR to them by writing /assign @douglas-reid 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 OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

- fix typos causing build errors
- fix lint errors
Copy link
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

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

What was your plan for enabling this validation service via k8s webhook with GKE workaround? Use istioctl and non-HTTPS variant until k8s 1.8 with option of both schemes coexist for while?

// verify the content type is accurate
contentType := r.Header.Get("Content-Type")
if contentType != "application/json" {
glog.Errorf("contentType=%s, expect application/json", contentType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these errors return an HTTP response with appropriate status code instead of dropping request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if err != nil {
return nil
}
h.ServeHTTP(outBuf, req)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks!

@istio-merge-robot
Copy link

@jmuk PR needs rebase

@ayj
Copy link
Contributor

ayj commented Sep 7, 2017

I wrote a script to automate the GKE workaround + cert generation for Pilot admission webhook (see here). We could add something similar for Mixer validation if that makes sense. This may not really be suitable for e2e presubmit tests, but does provide a mechanism for users to experiment with the admission webhook validation before k8s 1.8 is available on GKE.

@jmuk
Copy link
Contributor Author

jmuk commented Sep 7, 2017

@ayj, your script should be really helpful and work well with my patch too, thanks for the headsup. Will imitate it (or maybe it's worth having it in istio/istio repository?)

@istio-merge-robot
Copy link

@jmuk PR needs rebase

@jmuk
Copy link
Contributor Author

jmuk commented Sep 19, 2017

reviewers, please take a look; I've confirmed this is working with @ayj's workaround script.

@codecov
Copy link

codecov bot commented Sep 26, 2017

Codecov Report

Merging #1218 into master will decrease coverage by 0.67%.
The diff coverage is 59.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   83.85%   83.17%   -0.68%     
==========================================
  Files         122      125       +3     
  Lines       12094    12420     +326     
==========================================
+ Hits        10141    10330     +189     
- Misses       1745     1876     +131     
- Partials      208      214       +6
Impacted Files Coverage Δ
pkg/config/store/store2.go 96.1% <ø> (ø) ⬆️
cmd/server/cmd/validator.go 0% <0%> (ø)
cmd/server/cmd/root.go 0% <0%> (ø) ⬆️
pkg/runtime/init.go 28.81% <0%> (ø) ⬆️
pkg/config/crd/init.go 0% <0%> (ø) ⬆️
pkg/config/manager.go 100% <100%> (ø) ⬆️
pkg/config/crd/store.go 96.42% <100%> (+0.06%) ⬆️
pkg/config/store/validator.go 84.78% <84.78%> (ø)
pkg/config/crd/validatorserver.go 91.01% <91.01%> (ø)
tools/codegen/pkg/inventory/generator.go 65.62% <0%> (-6.25%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 535eb56...9bf2732. Read the comment docs.

@istio-merge-robot
Copy link

@jmuk PR needs rebase

@jmuk jmuk mentioned this pull request Oct 20, 2017
6 tasks
@codecov-io
Copy link

codecov-io commented Oct 20, 2017

Codecov Report

Merging #1218 into master will decrease coverage by 0.63%.
The diff coverage is 61.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   84.66%   84.02%   -0.64%     
==========================================
  Files         122      125       +3     
  Lines       11922    12260     +338     
==========================================
+ Hits        10094    10302     +208     
- Misses       1632     1755     +123     
- Partials      196      203       +7
Impacted Files Coverage Δ
pkg/config/store/store2.go 96.29% <ø> (ø) ⬆️
pkg/runtime/init.go 28.81% <0%> (ø) ⬆️
cmd/server/cmd/validator.go 0% <0%> (ø)
pkg/config/crd/init.go 0% <0%> (ø) ⬆️
cmd/server/cmd/root.go 0% <0%> (ø) ⬆️
pkg/config/crd/store.go 96.42% <100%> (+0.06%) ⬆️
pkg/config/manager.go 100% <100%> (ø) ⬆️
pkg/config/crd/validatorserver.go 89.41% <89.41%> (ø)
pkg/config/store/validator.go 94.54% <94.54%> (ø)
adapter/prometheus/server.go 89.83% <0%> (-5.09%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4eba741...cadef5a. Read the comment docs.

@jmuk
Copy link
Contributor Author

jmuk commented Oct 21, 2017

@mandarjog ping

1 similar comment
@jmuk
Copy link
Contributor Author

jmuk commented Oct 23, 2017

@mandarjog ping

@geeknoid
Copy link
Contributor

Do we still want this PR?

@jmuk
Copy link
Contributor Author

jmuk commented Oct 25, 2017

Yes, absolutely, this is a part of validation for mixer config. This is still needed and I'm still waiting for reviews.

@istio-merge-robot
Copy link

@jmuk PR needs rebase

@geeknoid
Copy link
Contributor

geeknoid commented Nov 3, 2017

This repo is no longer accepting PRs. Please resubmit this change to the istio/istio repo.

Thanks.

@geeknoid geeknoid closed this Nov 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants