Kfserving: Uber Issue: KFServing admission hook causing widespread issues because its a global admission hook

[jlewi]

/kind bug

We are getting lots of reports about problems caused because the KFServing admission hook is unavailable preventing pods from being created. The error message looks like the following

4m58s       Warning   FailedCreate                   replicaset/activator-5484756f7b          Error creating: Internal error occurred: failed calling webhook "inferenceservice.kfserving-webhook-server.pod-mutator": Post https://kfserving-webhook-server-service.kubeflow.svc:443/mutate-pods?timeout=30s: service "kfserving-webhook-server-service" not found

Here's my understanding

• Currently AdmissionHooks can not be scoped by label; so a pod admission hook is being applied to all pods
• The KFServing Admission Hooks is being applied to all pods and then in the hook itself it checks whether the pod belongs to a KFServing resource and if it does applies the hook
• However, if the KFServing web hook deployment is unavailable pod creation can be blocked

• For a variety of reasons we are reaching into a deadlock state where

  • The WebHook is defined but the deployment for the hook is not defined so calls to the admission hook will fail
  • Pod creation now fails because the webhook is not defined

[jlewi]

How is the inference service currently scoped?

Here's the config for the spec

kubectl -n kubeflow get MutatingWebHookConfiguration inferenceservice.serving.kubeflow.org -o yaml 

inferceservice.yaml.txt

I notice the following

  failurePolicy: Fail
  name: inferenceservice.kfserving-webhook-server.pod-mutator
  namespaceSelector:
    matchExpressions:
    - key: control-plane
      operator: DoesNotExist
  rules:

• Does failurePolicy mean that if the hook is unavailable we block pod creation? Is that what we want?
• It looks like the namespace selector is opt out; i.e. namespaces have to add the label "control-plane" to not apply the hook to that namespace
  
  
  
  • Should it be opt in instead by default so that only Kubeflow namespaces (i.e. profile created namespaces) have it turned on by default?
  
  

It doesn't look like the kubeflow namespace has the label "control-plane"

kubectl get namespace kubeflow -o yaml
apiVersion: v1
kind: Namespace
metadata:
  creationTimestamp: "2019-11-04T13:51:37Z"
  labels:
    katib-metricscollector-injection: enabled
  name: kubeflow
  resourceVersion: "1176"
  selfLink: /api/v1/namespaces/kubeflow
  uid: 38b15187-ff0a-11e9-ad03-42010a8e011f
spec:
  finalizers:
  - kubernetes
status:
  phase: Active

We probably shouldn't be applying the hook in the kubeflow namespace; that would probably prevent us from the deadlock state where we can't deploy the pods for the admission hook because the hook isn't running but the mutating hook is defined.

@animeshsingh @ellis-bigelow @ryandawsonuk

What are the various options for a quick fix that can limit the blast radius of problems with the web hook?

[cliveseldon]

Short term fix could be to Ignore errors: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#failure-policy though that would mean kfserving deployments might be broken.

Longer term would be to match labels which is available from 1.15? https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-objectselector

[jlewi]

Possible fixes

1. Add the label control-plane to the kubeflow namespace

   kubectl label namespace kubeflow control-plane=true
   

1. Change the namespaceSelector to be opt in; match namespaces with specific labels

    * This won't work because the changes will be overwritten when the controller restarts because the controller creates the webhook

Ref: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#matching-requests-namespaceselector

Possible Work Arounds

• Add the label control-plane to the kubeflow namespace
• Update the inferservice webhook to change the namespace selector to be opt in.

A possible recipe

1. Get the inference spec

   kubectl -n kubeflow get MutatingWebHookConfiguration inferenceservice.serving.kubeflow.org -o yaml > /tmp/inferceservice.yaml
   

2. Change the matchSelector

   namespaceSelector: matchLabels: serving.kubeflow.org: "true"

3. Apply it

   kubectl apply -f /tmp/inferenceservice.yaml
   

4. Label any namespaces in which you want to use KFServing as

   kubectl label namespace ${NAMESPACE} serving.kubeflow.org=true
   

[ryandawsonuk]

So this is related to https://github.com/kubeflow/kfserving/issues/480. As long as the kfserving controller is available then things work, even though it is looking at every Pod. The way that all Pod submissions can fail is if the controller itself isn't available. When kubernetes tries to bring it back then the hook fires but the controller isn't there to implement the hook so you get a catch-22.

[ellistarn]

I would definitely lean to simply add a control-plane label to the kubeflow namespace.
This is one of the newer features from kubebuilder that was probably missed when the original kf controllers were set up.

Opt in is a pretty poor UX for KFServing. If we want to avoid adding the control-plane label, I'd much rather users not install KFServing by default or have the webhook fail open.

[jlewi]

From a Kubeflow perspective a cluster can have 3 types of namespaces

• kubeflow - This is the system namespace where controllers and other shared components should run

  • This is not the name space where users should consume KF resources
  • i.e. This is not the namespace in which I would expect users to be deploying models

• *kubeflow user namespaces * - These are the namespaces in which users consume KF services

  • These are the namespaces in which users would deploy models

• non-KF namespaces - These are all other namespaces

  • These namespaces should be "isolated" from t Kubeflow
  • i.e; admins/users of these namespaces shouldn't have to worry that they will be negatively impacted by Kubeflow
    
    
    
    • This should be enforced by RBAC, OPA and other policy enforcement tools
    
    

I think we want to require that KF not impact non-kf namespaces. KFServing's current configuration is not meeting this requirement because its selector forces non-KF namespaces to opt out by adding a label "control-plane". This is not a scalable approach. An admin/owner for non-KF namespaces shouldn't have to think about KF. More generally an admin/owner shouldn't have to worry about protecting themselves against the applications they aren't running.

The profile controller is responsible for setting up a namespace to be suitable for consumption of KF. So we can just make profile controller add any labels necessary to allow KFServing to be consumed by default so that no explicit action by the user is needed.

[yuzisun]

This is what we did before kubernetes 1.15, we have similar profile controller running in our cluster which adds a label to indicate which ones are user namespaces, so we have a namespace selector in mutating webhook configuration that only allow pods from user namespaces go through the kfserving pod mutator. Since we are on kubernetes 1.15 now we switched to use the object selector then only kfserving managed pods go through the webhook.

[jlewi]

@yuzisun the profile controller can also add labels to each namespace. This is currently a hard coded list
https://github.com/kubeflow/kubeflow/blob/183919dccbe1e5c77aece17fd5064992383dddb6/components/profile-controller/controllers/profile_controller.go#L118

So we should add an appropriate label for kfserving.

I think we want application specific labels because someone might want to enable/disable the KFServing webhook but not other hooks.

[jlewi]

To summarize some discussion in slack

• The webhook is currently defined & created by the controller and not using a kustomize manifest
  https://github.com/kubeflow/kfserving/blob/master/pkg/webhook/webhook.go#L118

• The code currently doesn't explicitly set that namespace criterion but relies on the default behavior of the controller runtime
  https://github.com/kubernetes-sigs/controller-runtime/blob/v0.1.9/pkg/webhook/bootstrap.go#L300

  • Any customization of the webhook would be overwritten the next time the controller starts
  • I confirmed this is the case so my solution above of modifying the webhook doesn't work.

[animeshsingh]

@jlewi three PRs have gone in, one each in kfserving, manifests and kubeflow profile controller to tackle this.

[jlewi]

Thanks @animeshsingh!

Can you link to the relevant PRs so we can ensure they land in 0.7.1.

It looks like #571 is the change to kfserving.

It looks like #631 updated manifests to kfserving 0.2.2. I assume 0.2.2 includes #571?

Can we just cherry-pick #631 on to the v0.7-branch of manifests to create a 0.7.1 KFDef manifest which is shipping KFServing 0.2.2

Before we do that we need to take care of kubeflow/manifests#643

It looks like kubeflow/kubeflow#4533 is the change to the profile controller to add the appropriate label to the Kubeflow namespaces to apply the KFServing mutating webhook.

So it looks like we need to do the following

1. Build a new image of the profile controller which has kubeflow/kubeflow#4533
2. Update profile controller manifest on master to use this new image
3. Pin the v0.7.0 KFDef specs to a particular commit for repos kubeflow/manifests#643
4. Create v0.7.1 KFDef specs
5. Cherrypick the manifest changes for KFServing and profile controller onto the v0.7.0 branch
6. Merge kubeflow/kfctl#123 and cherry-pick it onto the v0.7.0 branch
7. Cut a v0.7.1 release of kfctl kubeflow/kfctl#124

[jlewi]

@lluunn you are the buildcop this week can you help with the above steps and coordinating the 0.7.1 release.

[animeshsingh]

@lluunn am hoping you can cover this. Letme know if you will need any info. From our persepctive, two important PRs which need to land in 0.7.1 are:

https://github.com/kubeflow/manifests/pull/631

and

https://github.com/kubeflow/kfserving/pull/571

cc @hougangliu

[lluunn]

@animeshsingh
To confirm, kubeflow/manifests#631 is to move kfserving to 0.2.2, and 0.2.2 has #571, so after cherrypick kubeflow/manifests#631 to 0.7, I don't need to do other things with #571, right?

[animeshsingh]

@lluunn yes.

#571 and https://github.com/kubeflow/kubeflow/pull/4533 is what you need

[lluunn]

When I tried to cherrypick kubeflow/manifests#631 to 0.7, I got merge conflict on some kfserving files.
Can someone from kfserving help cherrypick and resolve conflict?

[jlewi]

@lluunn I think a suitable solution is just to overwrite the files on the v0.7-branch for kubeflow/kfserving with the files on master at the commit for #587

I think you can do this just by doing the following

1. Create a branch v0.7-branch

2. Check out the files from master onto that branch; something like

   git checkout master kfserving/
   

   • Although you may need to use a specific commit rather than the tip of master.

[animeshsingh]

@jlewi I synced up with @lluunn last Friday - we are hopefully past that

[jlewi]

Thanks @animeshsingh so IIUC

• manifests on 0.7-branch have been updated to include the updated KFServing manifests
• We still need to release an updated profile controller to enable the KFServing web-hook on profile created namespaces
  
  
  
  • Without this users have to manually label their namespaces to turn on the KFServing webhook
  
  

[animeshsingh]

This PR went into Profile controller and was cherry picked in release
https://github.com/kubeflow/kubeflow/pull/4533

[jlewi]

Thanks looks like profile controller manifests on master were updated in
kubeflow/manifests#646

Do we still need to cherry-pick this to the v0.7-branch of manifests?

[lluunn]

step 1~5 of https://github.com/kubeflow/kfserving/issues/568#issuecomment-561974987 is done.

We actually didn't have any release in kubeflow/kfctl repo before.
Should we release new 0.7.1 kfctl from kubeflow/kfctl?

[lluunn]

I tried the 0.7 kfctl (same as before) with the new 0.7.1 kfdef: https://raw.githubusercontent.com/kubeflow/manifests/v0.7-branch/kfdef/kfctl_gcp_iap.0.7.1.yaml.
Deployment succeeded, endpoint is accessible, and I can create and connect to a notebook.

[lluunn]

Should we close this one?

[jlewi]

@lluunn Thanks! To verify that things are working as expected in the 0.7.1 release can you

1. Paste into this issue the output of

   kubectl get namespace ${KF-USER-NAMESPACE}
   

   • we want to verify that the label for the KFServingMutatingWebHook is added to the namespace

2. The output of

   kubectl -n kubeflow get MutatingWebHookConfiguration inferenceservice.serving.kubeflow.org -o yaml

   • We want to verify that the hook is only applied to namespaces with specific labels and the labels match the labels set by the profile controller

[lluunn]

$ kg namespace kubeflow-lunkai -o yaml
apiVersion: v1
kind: Namespace
metadata:
  annotations:
    owner: [email protected]
  creationTimestamp: "2019-12-11T23:04:28Z"
  labels:
    istio-injection: enabled
    katib-metricscollector-injection: enabled
    serving.kubeflow.org/inferenceservice: enabled .   --> this one right?
  name: kubeflow-lunkai
...

Seems there are two versions:

apiVersion: admissionregistration.k8s.io/v1beta1
kind: MutatingWebhookConfiguration
metadata:
  creationTimestamp: "2019-12-11T23:03:15Z"
  generation: 1
  name: inferenceservice.serving.kubeflow.org
  resourceVersion: "3644"
  selfLink: /apis/admissionregistration.k8s.io/v1beta1/mutatingwebhookconfigurations/inferenceservice.serving.kubeflow.org
  uid: 699442b0-1c6a-11ea-b902-42010a80003a
webhooks:
- admissionReviewVersions:
  - v1beta1
    service:
      name: kfserving-webhook-server-service
      namespace: kubeflow
      path: /mutate-inferenceservices
  failurePolicy: Fail
  name: inferenceservice.kfserving-webhook-server.defaulter
  namespaceSelector:
    matchExpressions:
    - key: control-plane
      operator: DoesNotExist
  rules:
  - apiGroups:
    - serving.kubeflow.org
    apiVersions:
    - v1alpha2
    operations:
    - CREATE
    - UPDATE
    resources:
    - inferenceservices
    scope: '*'
  sideEffects: Unknown
  timeoutSeconds: 30
- admissionReviewVersions:
  - v1beta1
    service:
      name: kfserving-webhook-server-service
      namespace: kubeflow
      path: /mutate-pods
  failurePolicy: Fail
  name: inferenceservice.kfserving-webhook-server.pod-mutator
  namespaceSelector:
    matchLabels:
      serving.kubeflow.org/inferenceservice: enabled
  rules:
  - apiGroups:
    - ""
    apiVersions:
    - v1
    operations:
    - CREATE
    - UPDATE
    resources:
    - pods
    scope: '*'
  sideEffects: Unknown
  timeoutSeconds: 30

The inferenceservice.kfserving-webhook-server.pod-mutator indeed has serving.kubeflow.org/inferenceservice: enabled

[jlewi]

@animeshsingh should we modify the first service as well?

[yuzisun]

no, the first one is not global mutator, it is only for inference service crd.

[sshakeri]

I lost all the kubeflow pods all of a sudden. @jlewi's workaround here works, got the pods back thanks https://github.com/kubeflow/kfserving/issues/568#issuecomment-557619909

[yuzisun]

@animeshsingh we can close this one now right ?

[animeshsingh]

yes we are out of the woods on this one
cc @jlewi @ellis-bigelow

[animeshsingh]

/close

[k8s-ci-robot]

@animeshsingh: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[issue-label-bot[bot]]

Issue Label Bot is not confident enough to auto-label this issue.
See dashboard for more details.

[maganaluis]

@animeshsingh @jlewi We encountered this issue when I testing KFP multi-user support, I actually just re-verified this with the latest changes on the manifests repo. The problem first comes when control-plane is enabled on the kubeflow namespace, this prevents the istio sidecar injection from working on that namespace, so in order to have mutli-user support for KFP we removed the label. I didn't investigate any further why this happens, but I'd love to get some documentation on what KFServing is doing on those webooks.

The second issue is this deadlock outlined above, this happened when I deleted the kubeflow resources, then attempted to reinstall kubeflow (with istio already installed) this caused the widespread issue that prevented any pods from being created.

To avoid the deadlock and to have the sidecar injection on the kubeflow namespace, I had to re-apply the profile with istioctl (we are using 1.6), create the kubeflow namespace without the control-plane enabled and then proceed to install kubeflow, kfserving, knative etc...

It's quite strange but I suspect you guys or other will run into this issues so I wanted to post this information here.

[jlewi]

Thanks @maganaluis would you mind opening a new issue?

[animeshsingh]

thanks @maganaluis - the details of why the label control plane is needed are listed here
https://github.com/kubeflow/kfserving#standalone-kfserving-installation

cc @yuzisun @adrian555 @shawnzhu for awareness - multi-cloud KFP has brought a requirement to enable sidecar injection in kubeflow namespace, looks like resurfacing this old kfserving issue.

[yuzisun]

@maganaluis We need to use object selector on the mutating webhook configuration so that only kfserving labelled pods go through the KFServing pod mutator, the problem is that object selector is only supported kubernetes 1.15+ while kubeflow's minimal requirement is still kubernetes 1.14. If you are on kubernetes 1.15+ you can use following command to solve the issue.

kubectl patch mutatingwebhookconfiguration inferenceservice.serving.kubeflow.org --patch '{"webhooks":[{"name": "inferenceservice.kfserving-webhook-server.pod-mutator","objectSelector":{"matchExpressions":[{"key":"serving.kubeflow.org/inferenceservice", "operator": "Exists"}]}}]}'

[maganaluis]

@yuzisun Thank you that fixes the issue

[akshatgit]

I faced the same issue when I upgraded my GKE cluster from minor version 14 to 17.
@yuzisun's patch worked for me.

[Talador12]

@yuzisun this solution worked. However, this issue should be re-opened as the default behavior should match this solution. Otherwise, non-kf namespaces could have pods evicted en masse.