Implement webhook validations for the PyTorchJob

[tenzen-y]

What this PR does / why we need it:
I implemented webhook validations for the PyTorchJob.
Additionally, I didn't add any additional validations. The traininig-operator has the same validations the same as before.

1. Implement the cert-generation mechanism using the open-policy-agent/cert-controller.
2. Move existing PyTorchJob validations to the webhook validations and adjust validation results to the webhook form.
3. Add some manifests for RBAC and ValidatingWebhookConfiguration.

Note that as the first iteration, I didn't support cert-manager in this PR. Maybe we can support the release after the next release.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Part-of #1993

Checklist:

• Docs included if any changes are user facing

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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:

• OWNERS [tenzen-y]

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

[coveralls]

Pull Request Test Coverage Report for Build 8634700799

Details

• 56 of 174 (32.18%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.2%) to 35.178%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/webhooks/webhooks.go	0	3	0.0%
pkg/webhooks/pytorch/pytorchjob_webhook.go	56	83	67.47%
pkg/cert/cert.go	0	31	0.0%
cmd/training-operator.v1/main.go	0	57	0.0%

Files with Coverage Reduction	New Missed Lines	%
cmd/training-operator.v1/main.go	1	0.0%

Totals
Change from base Build 8585006097:	-0.2%
Covered Lines:	4335
Relevant Lines:	12323

---

💛 - Coveralls

[tenzen-y]

I prefer to implement these webhooks in other PRs so that we can keep this PR minimal required.

[tenzen-y]

/hold
for review.

[tenzen-y]

/assign @andreyvelich @johnugeorge

[tenzen-y]

@andreyvelich @johnugeorge I addressed all comments. PTAL!

[andreyvelich]

Does it make sense to keep it in the Training Operator install similar to Katib: https://github.com/kubeflow/katib/blob/master/manifests/v1beta1/installs/katib-standalone/kustomization.yaml#L38-L41 ?
In the future releases when we make the integration with Cert Manager for Kubeflow Install, we can remove this secret generator from Kubeflow Kustomize Overlay

[tenzen-y]

We need to keep this secret since the cert-controller doesn't create any Secret

• https://github.com/open-policy-agent/cert-controller/blob/190188dbfe07022a5160380800785dd81d511726/pkg/rotator/rotator.go#L315-L318
• https://github.com/open-policy-agent/cert-controller/blob/190188dbfe07022a5160380800785dd81d511726/pkg/rotator/rotator.go#L753-L762

[andreyvelich]

Yeah, I understand that. My suggestion is instead of having separate folder: /internalcert, update the standalone and Kubeflow Kustomize installs to generate that secret as follows:

secretGenerator:
  - name: training-operator-webhook-secret
    options:
      disableNameSuffixHash: true

[tenzen-y]

Oh, I see.
That makes sense.

[tenzen-y]

Done.

[andreyvelich]

@tenzen-y Do you prefer to introduce Controller Runtime logger for webhook in separate PR based on our discussion here: #2035 (comment) ?

[tenzen-y]

@tenzen-y Do you prefer to introduce Controller Runtime logger for webhook in separate PR based on our discussion here: #2035 (comment) ?

@andreyvelich Yes I do since We need to remove many loggers for the replacements.

[tenzen-y]

@andreyvelich @johnugeorge I believe that this PR is ready for the merge.

[andreyvelich]

Thank you @tenzen-y!
Just a small comment from me.
/assign @johnugeorge

[andreyvelich]

@tenzen-y Do you need to move this service to the webhook and name it as training-operator-service?
Why we can't just keep service name as it is in /manifests/base/service.yaml and just add another 9443 port ?

[andreyvelich]

Like what we do in Katib: https://github.com/kubeflow/katib/blob/master/manifests/v1beta1/components/controller/service.yaml

[tenzen-y]

Oh, this is a good catch!
At the some points, we needed to move the Service to the webhook directory to apply the kustomize patch, but currently, moving is not needed.

[tenzen-y]

Done.

[andreyvelich]

Do you need this renaming ?

[tenzen-y]

Based on this discussion, we changed this name. So, would you prefer to keep using the training-operator?

I'm ok with either name.

[andreyvelich]

Oh, sorry, I think I made a mistake there, tbh we should keep this name as training-operator.

[tenzen-y]

No worries :)
I reverted this change.

[andreyvelich]

For the secret name, would it be better to just name it: training-operator-webhook-cert. Similar to Katib and not add the Kubernetes resource name (e.g. secret) to the name ?

[tenzen-y]

Oh, you already mentioned here: #2035 (comment)

Secret name: training-operator-webhook-cert

I'll try to refine this name.

[tenzen-y]

Done.

[johnugeorge]

Thanks @tenzen-y for this contribution
LGTM

[andreyvelich]

Thanks a lot for working on this @tenzen-y!
/lgtm
/assign @kubeflow/wg-training-leads

[johnugeorge]

/hold cancel

[tenzen-y]

@andreyvelich @johnugeorge Thank you for the review!

[franciscojavierarceo]

@tenzen-y I posted this in slack but I think we need some instructions here for local development.

See #2069