Skip to content

Commit

Permalink
AIP-5879 Admission Webhook 1.22 Compatibility (kubeflow#6459)
Browse files Browse the repository at this point in the history
* init

* update dependancies

* refine

* update webhook

* update readme and makefile

Cherry-picked-from: 8be0d98
Reviewed-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
  • Loading branch information
aaron-arellano authored and kimwnasptd committed Aug 26, 2022
1 parent c4987c5 commit 5f27ce3
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 68 deletions.
13 changes: 4 additions & 9 deletions components/admission-webhook/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,8 @@ build:
go build -gcflags 'all=-N -l' -o bin/webhook .

docker-build:
docker build -t ${IMG}:${TAG} -f Dockerfile .

docker-push:
docker push ${IMG}:${TAG}

image: docker-build docker-push

build-gcr:
docker build -t $(IMG):$(TAG) .
@echo Built $(IMG):$(TAG)
@echo Built $(IMG):$(TAG)

docker-push:
docker push ${IMG}:${TAG}
16 changes: 12 additions & 4 deletions components/admission-webhook/README.md
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,18 @@ 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. Furthermore, any pod that is targeted by the `AdmissionWebhook` for mutation was effected and will fail to start if there are configuration collisions or some other issue, already running pods are not effected by this policy. Again, only pods created after the policy change may be effected.

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, a `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
23 changes: 11 additions & 12 deletions components/admission-webhook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (

settingsapi "github.com/kubeflow/kubeflow/components/admission-webhook/pkg/apis/settings/v1alpha1"
"github.com/mattbaird/jsonpatch"
"k8s.io/api/admission/v1beta1"
v1 "k8s.io/api/admission/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -59,8 +59,8 @@ func (c *Config) addFlags() {
"File containing the default x509 private key matching --tls-cert-file.")
}

func toAdmissionResponse(err error) *v1beta1.AdmissionResponse {
return &v1beta1.AdmissionResponse{
func toAdmissionResponse(err error) *v1.AdmissionResponse {
return &v1.AdmissionResponse{
Result: &metav1.Status{
Message: err.Error(),
},
Expand Down Expand Up @@ -242,7 +242,6 @@ func mergeEnv(envVars []corev1.EnvVar, podDefaults []*settingsapi.PodDefault) ([

func mergeEnvFrom(envSources []corev1.EnvFromSource, podDefaults []*settingsapi.PodDefault) ([]corev1.EnvFromSource, error) {
var mergedEnvFrom []corev1.EnvFromSource

mergedEnvFrom = append(mergedEnvFrom, envSources...)
for _, pd := range podDefaults {
mergedEnvFrom = append(mergedEnvFrom, pd.Spec.EnvFrom...)
Expand Down Expand Up @@ -506,7 +505,7 @@ func applyPodDefaultsOnContainer(ctr *corev1.Container, podDefaults []*settingsa
ctr.EnvFrom = envFrom
}

func mutatePods(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
func mutatePods(ar v1.AdmissionReview) *v1.AdmissionResponse {
klog.Info("Entering mutatePods in mutating webhook")
podResource := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}
if ar.Request.Resource != podResource {
Expand All @@ -521,7 +520,7 @@ func mutatePods(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
klog.Error(err)
return toAdmissionResponse(err)
}
reviewResponse := v1beta1.AdmissionResponse{}
reviewResponse := v1.AdmissionResponse{}
reviewResponse.Allowed = true
if pod.Namespace == "" {
klog.Infof("Namespace was not set explicitly in Pod manifest, falling back to the namespace-'%s' coming from AdmissionReview request", ar.Request.Namespace)
Expand Down Expand Up @@ -555,7 +554,7 @@ func mutatePods(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {

crdclient := getCrdClient()
list := &settingsapi.PodDefaultList{}
err := crdclient.List(context.TODO(), list, &client.ListOptions{Namespace: namespace})
err := crdclient.List(context.TODO(), list, &client.ListOptions{Namespace: pod.Namespace})
if meta.IsNoMatchError(err) {
klog.Errorf("%v (has the CRD been loaded?)", err)
return toAdmissionResponse(err)
Expand Down Expand Up @@ -621,13 +620,13 @@ func mutatePods(ar v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
jsonPatchBytes, _ := json.Marshal(jsonPatch)

reviewResponse.Patch = jsonPatchBytes
pt := v1beta1.PatchTypeJSONPatch
pt := v1.PatchTypeJSONPatch
reviewResponse.PatchType = &pt

return &reviewResponse
}

type admitFunc func(v1beta1.AdmissionReview) *v1beta1.AdmissionResponse
type admitFunc func(v1.AdmissionReview) *v1.AdmissionResponse

func serve(w http.ResponseWriter, r *http.Request, admit admitFunc) {
var body []byte
Expand All @@ -644,8 +643,8 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitFunc) {
return
}

var reviewResponse *v1beta1.AdmissionResponse
ar := v1beta1.AdmissionReview{}
var reviewResponse *v1.AdmissionResponse
ar := v1.AdmissionReview{}
deserializer := codecs.UniversalDeserializer()
if _, _, err := deserializer.Decode(body, nil, &ar); err != nil {
klog.Error(err)
Expand All @@ -654,7 +653,7 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitFunc) {
reviewResponse = admit(ar)
}

response := v1beta1.AdmissionReview{}
response := v1.AdmissionReview{}
if reviewResponse != nil {
response.Response = reviewResponse
response.Response.UID = ar.Request.UID
Expand Down
92 changes: 54 additions & 38 deletions components/admission-webhook/manifests/base/crd.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
apiVersion: apiextensions.k8s.io/v1beta1
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: poddefaults.kubeflow.org
Expand All @@ -8,46 +8,62 @@ spec:
kind: PodDefault
plural: poddefaults
singular: poddefault
preserveUnknownFields: false
scope: Namespaced
version: v1alpha1
validation:
openAPIV3Schema:
properties:
apiVersion:
type: string
kind:
type: string
metadata:
type: object
spec:
versions:
- name: v1alpha1
served: true
storage: true
schema:
openAPIV3Schema:
properties:
desc:
apiVersion:
type: string
serviceAccountName:
kind:
type: string
automountServiceAccountToken:
type: boolean
env:
items:
type: object
type: array
envFrom:
items:
type: object
type: array
selector:
metadata:
type: object
volumeMounts:
items:
type: object
type: array
volumes:
items:
type: object
type: array
required:
- selector
type: object
status:
spec:
properties:
desc:
type: string
serviceAccountName:
type: string
automountServiceAccountToken:
type: boolean
env:
items:
type: object
x-kubernetes-preserve-unknown-fields: true
type: array
x-kubernetes-preserve-unknown-fields: true
envFrom:
items:
type: object
x-kubernetes-preserve-unknown-fields: true
type: array
x-kubernetes-preserve-unknown-fields: true
selector:
type: object
x-kubernetes-preserve-unknown-fields: true
volumeMounts:
items:
type: object
x-kubernetes-preserve-unknown-fields: true
type: array
x-kubernetes-preserve-unknown-fields: true
volumes:
items:
type: object
x-kubernetes-preserve-unknown-fields: true
type: array
x-kubernetes-preserve-unknown-fields: true
required:
- selector
type: object
x-kubernetes-preserve-unknown-fields: true
status:
type: object
x-kubernetes-preserve-unknown-fields: true
type: object
type: object
x-kubernetes-preserve-unknown-fields: true
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
apiVersion: admissionregistration.k8s.io/v1beta1
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
name: mutating-webhook-configuration
webhooks:
- clientConfig:
- admissionReviewVersions:
- v1beta1
- v1
clientConfig:
caBundle: ""
service:
name: service
path: /apply-poddefault
sideEffects: None
failurePolicy: Fail
name: $(podDefaultsDeploymentName).kubeflow.org
namespaceSelector:
matchLabels:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
apiVersion: admissionregistration.k8s.io/v1beta1
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
name: mutating-webhook-configuration
Expand Down
4 changes: 2 additions & 2 deletions components/admission-webhook/scheme.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package main

import (
"github.com/kubeflow/kubeflow/components/admission-webhook/pkg/apis"
admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
Expand All @@ -33,6 +33,6 @@ func init() {

func addToScheme(scheme *runtime.Scheme) {
corev1.AddToScheme(scheme)
admissionregistrationv1beta1.AddToScheme(scheme)
admissionregistrationv1.AddToScheme(scheme)
apis.AddToScheme(scheme)
}

0 comments on commit 5f27ce3

Please sign in to comment.