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

add validating webhook for studyJob #383

Merged
merged 3 commits into from
Feb 20, 2019
Merged

add validating webhook for studyJob #383

merged 3 commits into from
Feb 20, 2019

Conversation

hougangliu
Copy link
Member

@hougangliu hougangliu commented Feb 15, 2019

If create/update a studyJob with bad CR manifest or invalid configuration, k8s api
server will reject the request.
Fixes: #314


This change is Reviewable

@hougangliu
Copy link
Member Author

@hougangliu
Copy link
Member Author

/retest

@richardsliu
Copy link
Contributor

@hougangliu Does this require any specific version of k8s server to work?

@hougangliu
Copy link
Member Author

api admission webhook introduced in k8s 1.9

@johnugeorge
Copy link
Member

In operators, job goes into failed state when the config is invalid. What is the recommended way?

@hougangliu
Copy link
Member Author

hougangliu commented Feb 16, 2019

when we submit a invalid k8s resource(saying deployment), API server reject it. so I think we'd better follow k8s way

@johnugeorge
Copy link
Member

I was thinking about the consistency across kubeflow components.

There are few pre requisites for API servers. https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#is-there-a-recommended-set-of-admission-controllers-to-use I am not sure if admission plugins are enabled by default for api servers across clouds.
https://istio.io/help/ops/setup/webhook/#verify-dynamic-admission-webhook-prerequisites

@hougangliu
Copy link
Member Author

In k8s official doc, ValidatingAdmissionWebhook is in recommended set of admission controllers to use and enabled by default.

@hougangliu
Copy link
Member Author

in 1.9, ValidatingAdmissionWebhook is RecommendedPlugin. After 1.10, it is enabled by default.

@johnugeorge
Copy link
Member

Sounds good. Can you add a test too?

If create/update a studyJob with bad CR manifest or invalid configuration, k8s api
server will reject the request.
Fixes: #314
@hougangliu
Copy link
Member Author

Sounds good. Can you add a test too?

sure, done! thanks!

@johnugeorge
Copy link
Member

/lgtm
I will wait for @richardsliu

@richardsliu
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardsliu

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 merged commit 5a1a791 into kubeflow:master Feb 20, 2019
@hougangliu hougangliu deleted the validatingwebhook branch April 24, 2019 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants