opendatahub-io / odh-model-controller

Apache License 2.0
3 stars 55 forks source link

bug: Error in odh-model-controller logs on deletion of ModelMesh isvc #133

Open vaibhavjainwiz opened 11 months ago

vaibhavjainwiz commented 11 months ago

Below error is occuring in odh-model-controller logs on deletion of ModelMesh isvc

2023-12-12T09:36:58Z ERROR controllers.InferenceService Unable to clean up resources {"InferenceService": "example-onnx-mnist", "namespace": "kserve-demo", "error": "failed to delete ServiceAccount: serviceaccounts \"modelmesh-serving-sa\" is forbidden: User \"system:serviceaccount:opendatahub:odh-model-controller\" cannot delete resource \"serviceaccounts\" in API group \"\" in the namespace \"kserve-demo\""}
github.com/opendatahub-io/odh-model-controller/controllers.(*OpenshiftInferenceServiceReconciler).Reconcile
/workspace/controllers/inferenceservice_controller.go:76
vaibhavjainwiz commented 11 months ago

On analysis, I found that odh-model-controller-role ClusterRole which is attached the odh-model-controller pod doesn't have access to delete serviceaccounts.

vaibhavjainwiz commented 11 months ago

Solution: Edit odh-model-controller-role ClusterRole to provide access for deletion of ServiceAccount.

- apiGroups:
    - ""
  resources:
    - serviceaccounts
  verbs:
    - create
    - get
    - list
    - patch
    - update
    - watch
    - delete
spolti commented 11 months ago

Solution: Edit odh-model-controller-role ClusterRole to provide access for deletion of ServiceAccount.

- apiGroups:
    - ""
  resources:
    - serviceaccounts
  verbs:
    - create
    - get
    - list
    - patch
    - update
    - watch
    - delete

Are all these roles needed for deletion?

vaibhavjainwiz commented 10 months ago

role for create, get, list, pathc, update and watch is already assigned to serviceaccounts resource. I just added delete role.

spolti commented 10 months ago

role for create, get, list, pathc, update and watch is already assigned to serviceaccounts resource. I just added delete role.

I see, thanks, we might need to revisit it later to make sure that we are not adding unneeded roles.