-
Notifications
You must be signed in to change notification settings - Fork 454
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
Conversation
/retest |
@hougangliu Does this require any specific version of k8s server to work? |
api admission webhook introduced in k8s 1.9 |
In operators, job goes into failed state when the config is invalid. What is the recommended way? |
when we submit a invalid k8s resource(saying deployment), API server reject it. so I think we'd better follow k8s way |
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. |
In k8s official doc, ValidatingAdmissionWebhook is in recommended set of admission controllers to use and enabled by default. |
in 1.9, ValidatingAdmissionWebhook is RecommendedPlugin. After 1.10, it is enabled by default. |
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
sure, done! thanks! |
/lgtm |
/lgtm |
[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 |
If create/update a studyJob with bad CR manifest or invalid configuration, k8s api
server will reject the request.
Fixes: #314
This change is