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.

Sign up for GitHub

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

Jump to bottom

AIP-5879 Admission Webhook 1.22 Compatibility #6459

Merged
merged 5 commits into from
May 13, 2022
Merged

AIP-5879 Admission Webhook 1.22 Compatibility #6459

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4d7225
init
aaron-arellano Apr 27, 2022
d8bd93c
update dependancies
aaron-arellano Apr 27, 2022
db48982
refine
aaron-arellano Apr 28, 2022
a8d3d47
update webhook
aaron-arellano Apr 29, 2022
a947529
update readme and makefile
aaron-arellano May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 13 additions & 4 deletions components/admission-webhook/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
We need a way to inject common data (env vars, volumes) to pods (e.g. notebooks).
See [issue](https://github.com/kubeflow/kubeflow/issues/2641).
K8s has [PodPreset](https://v1-19.docs.kubernetes.io/docs/concepts/workloads/pods/podpreset/) resource with similar use-case, however it is in alpha.
K8s [admission-controller](https://godoc.org/k8s.io/api/admissionregistration/v1beta1#MutatingWebhookConfiguration) and CRD can be used to implement PodPreset as done in [here](https://github.com/jpeeler/podpreset-crd).
K8s [admission-controller](https://godoc.org/k8s.io/api/admissionregistration/v1#MutatingWebhookConfiguration) and CRD can be used to implement PodPreset as done in [here](https://github.com/jpeeler/podpreset-crd).
We borrowed this PodPreset implementation, customize it for Kubeflow and rename it to PodDefault to avoid confusion.
The code is not directly used as Kubeflow's use case for PodDefault controller is slightly different.
In fact, PodDefault in Kubeflow is defined as CRD without the custom controller (as opposed to [here](https://github.com/jpeeler/podpreset-crd)).
Expand Down Expand Up @@ -48,11 +48,11 @@ For the above PodDefault, when a pod creation request comes which has the label
to the pod as described in the PodDefault spec.

## Webhook Configuration
Define a [MutatingWebhookConfiguration](https://godoc.org/k8s.io/api/admissionregistration/v1beta1#MutatingWebhookConfiguration),
Define a [MutatingWebhookConfiguration](https://godoc.org/k8s.io/api/admissionregistration/v1#MutatingWebhookConfiguration),
for example:

```
apiVersion: admissionregistration.k8s.io/v1beta1
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
name: gcp-cred-webhook
Expand All @@ -75,10 +75,19 @@ This specifies
1. When there is a pod being created (see `rules`),
1. call the webhook service `gcp-cred-webhook.default` at path `/add-cred` (see `clientConfig`)

### admissionregistration.k8s.io/v1 default failurePolicy
In adopting `admissionregistration.k8s.io/v1` for the `MutatingWebhookConfiguration` we accept the default value
for `failurePolicy` to be `Fail` per [documentation](https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#failure-policy). Upon testing this default feature it was discovered if the `AdmissionWebhook`'s `mutating-webhook-configuration` failed to mutate a pod then the pod would fail to start, its associated `Deployment` or `StatefulSet` would continually attempt to create the pod until the process that created the pod was terminated. This continuous failure to mutate the target pod may block other target pods from mutation
until the failing process ends.
Copy link
Member

Choose a reason for hiding this comment

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

This continuous failure to mutate the target pod may block other target pods from mutation
until the failing process ends.

Could you clarify a little bit more what you mean here? By other Pods you are not referring to other unrelated Pods in the cluster right?

IIUC, as you described, the Deployment/StatefulSet will be retrying to create the Pod which will keep failing. Are there other side effects to other pods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. will clarify. Pretty much any pod that is targeted by the admission webhook for mutation is effected and will fail to start if there are configuration collisions or some other issue. Already running pods are not effected at all, again only pods being created after the policy change. Will add this to Readme for clarification.


Engineers should be mindful of this setting as it was also noted these failures can persist beyond the `timeoutSeconds` parameter that
is by default set to `10` seconds in the `v1` API. For example, Kubeflow Notebook `StatefulSet`s attempted to create notebook pods
despite its API request getting rejected by the `mutating-webhook-configuration`. Please refer to kubernetes documentation to read up on
the `failurePolicy: Ignore` parameter as this was the default value in `v1beta1`.

### Webhook implementation
The webhook should be a server that can handle request coming from the configured path (`/add-cred` in the above).
The request and response types are both [AdmissionReview](https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionReview)
The request and response types are both [AdmissionReview](https://godoc.org/k8s.io/api/admission/v1#AdmissionReview)

## Reference
1. [K8S PodPreset](https://v1-19.docs.kubernetes.io/docs/concepts/workloads/pods/podpreset/)
Expand Down
81 changes: 36 additions & 45 deletions components/admission-webhook/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,46 @@ module github.com/kubeflow/kubeflow/components/admission-webhook

go 1.17

replace (
k8s.io/api => k8s.io/api v0.0.0-20190222213804-5cb15d344471
k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20190221213512-86fb29eff628
k8s.io/client-go => k8s.io/client-go v10.0.0+incompatible
require (
github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a
github.com/onsi/gomega v1.17.0
golang.org/x/net v0.0.0-20211209124913-491a49abca63
k8s.io/api v0.23.0
k8s.io/apimachinery v0.23.0
k8s.io/client-go v0.23.0
k8s.io/klog v0.3.0
sigs.k8s.io/controller-runtime v0.11.0
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/ghodss/yaml v1.0.0 // indirect
github.com/go-logr/logr v0.1.0 // indirect
github.com/go-logr/zapr v0.1.1 // indirect
github.com/gogo/protobuf v1.2.1 // indirect
github.com/golang/protobuf v1.3.1 // indirect
github.com/google/btree v1.0.0 // indirect
github.com/google/gofuzz v1.0.0 // indirect
github.com/googleapis/gnostic v0.2.0 // indirect
github.com/gregjones/httpcache v0.0.0-20190212212710-3befbb6ad0cc // indirect
github.com/imdario/mergo v0.3.7 // indirect
github.com/json-iterator/go v1.1.6 // indirect
github.com/kr/pretty v0.1.0 // indirect
github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a
github.com/evanphx/json-patch v4.12.0+incompatible // indirect
github.com/go-logr/logr v1.2.0 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/google/go-cmp v0.5.5 // indirect
github.com/google/gofuzz v1.1.0 // indirect
github.com/googleapis/gnostic v0.5.5 // indirect
github.com/imdario/mergo v0.3.12 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.1 // indirect
github.com/onsi/ginkgo v1.8.0 // indirect
github.com/onsi/gomega v1.5.0
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
github.com/pkg/errors v0.8.1 // indirect
github.com/spf13/pflag v1.0.3 // indirect
github.com/stretchr/testify v1.3.0 // indirect
go.uber.org/atomic v1.3.2 // indirect
go.uber.org/multierr v1.1.0 // indirect
go.uber.org/zap v1.9.1 // indirect
golang.org/x/crypto v0.0.0-20190418165655-df01cb2cc480 // indirect
golang.org/x/net v0.0.0-20190415214537-1da14a5a36f2
golang.org/x/oauth2 v0.0.0-20190402181905-9f3314589c9a // indirect
golang.org/x/sys v0.0.0-20190429190828-d89cdac9e872 // indirect
golang.org/x/text v0.3.2 // indirect
golang.org/x/time v0.0.0-20190308202827-9d24e82272b4 // indirect
google.golang.org/appengine v1.5.0 // indirect
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/spf13/pflag v1.0.5 // indirect
golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f // indirect
golang.org/x/sys v0.0.0-20211029165221-6e7872819dc8 // indirect
golang.org/x/term v0.0.0-20210615171337-6886f2dfbf5b // indirect
golang.org/x/text v0.3.7 // indirect
golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/protobuf v1.27.1 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/yaml.v2 v2.2.2 // indirect
k8s.io/api v0.0.0-20190418212532-b8e4ab4b136a
k8s.io/apiextensions-apiserver v0.0.0-20190418213722-9c24cb44f89d // indirect
k8s.io/apimachinery v0.0.0-20190418212431-b3683fe6b520
k8s.io/client-go v10.0.0+incompatible
k8s.io/klog v0.3.0
k8s.io/utils v0.0.0-20190308190857-21c4ce38f2a7 // indirect
sigs.k8s.io/controller-runtime v0.1.10
sigs.k8s.io/testing_frameworks v0.1.1 // indirect
sigs.k8s.io/yaml v1.1.0 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
k8s.io/klog/v2 v2.30.0 // indirect
k8s.io/kube-openapi v0.0.0-20211115234752-e816edb12b65 // indirect
k8s.io/utils v0.0.0-20211116205334-6203023598ed // indirect
sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.2.1 // indirect
sigs.k8s.io/yaml v1.3.0 // indirect
)