kubeflow / kubeflow/dashboard

profile controller fails to update profile owner

Open
#33 19 comments 5 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
19
Forks
68
Avg merge
2d 16h
Merged PRs (30d)
10

Description

/kind bug

Related to: https://github.com/kubeflow/kubeflow/issues/6054

## Problem:

Currently, if a `Profile` resource has its `spec.owner.name` updated, the profile-controller will lose control of the associated Namespace, and fail to update the owner.

## Solution:

1. The profile-controller will not "take control" of a Namespace [unless it has the same `metadata.annotations.owner` as the `spec.owner.name` of the Profile](https://github.com/kubeflow/kubeflow/blob/5e09536fd36cf8265d76730de4cbbd5c6140b28e/components/profile-controller/controllers/profile_controller.go#L173-L175):
- ___SOLUTION:__ rather than checking for the presence of `metadata.annotations.owner` on the Namespace, we should check the "owner reference", by checking if the [`metadata.ownerReferences[]`](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#ownerreference-v1-meta) array contains the Profile resource_
- ___NOTE:__ after this change will need to provide a way to "import" existing namespaces, as it will be nearly impossible to set the correct "owner reference" manually (also requested in https://github.com/kubeflow/dashboard/issues/45)._
2. The profile-controller does not update the `metadata.annotations.owner` of existing resources because it ignores `metadata.*` in its diff calculation:
- `RoleBinding/namespaceAdmin`
- ___SOLUTION:__ we can check the full RoleBinding definition for differences [rather than only `RoleRef` and `Subjects`](https://github.com/kubeflow/kubeflow/blob/5e09536fd36cf8265d76730de4cbbd5c6140b28e/components/profile-controller/controllers/profile_controller.go#L627)_
- ___NOTE:__ because kfam uses the `metadata.annotations.user` annotation of the `RoleBinding/namespaceAdmin` resource to determine the Namespace owner, without this solution, the wrong user will have "admin" access in the central-dashboard UI after a profile owner changes._
- `AuthorizationPolicy/ns-owner-access-istio`
- ___SOLUTION:__ we can check the full AuthorizationPolicy definition for difference, [rather than only the `Spec`](https://github.com/kubeflow/kubeflow/blob/5e09536fd36cf8265d76730de4cbbd5c6140b28e/components/profile-controller/controllers/profile_controller.go#L512)_
- `ResourceQuota/kf-resource-quota`
- ___SOLUTION:__ we can check the full ResourceQuota definition for differences, [rather than only the `Spec`](https://github.com/kubeflow/kubeflow/blob/5e09536fd36cf8265d76730de4cbbd5c6140b28e/components/profile-controller/controllers/profile_controller.go#L546)_

## Current Workaround:

The following steps are required to work around these issues and change the owner of an existing Profile:

1. Patch the Profile's `spec.owner.name` to your new owner
2. Patch the existing Namespace's `metadata.annotations.owner` annotation to your new owner.
3. Manually delete the following resources from the Namespace (so they are recreated by the profile-controller):
- `RoleBinding/namespaceAdmin`
- `AuthorizationPolicy/ns-owner-access-istio`
- `ResourceQuota/kf-resource-quota`

Contributor guide

Open the contributing guide

Research direction

Start with components/profile-controller/controllers/profile_controller.go, especially the Namespace ownership check and the comparison logic around the RoleBinding, AuthorizationPolicy, and ResourceQuota references in the issue. Trace how Profile owner changes flow through these resources and verify behavior for existing namespaces. Done means owner changes retain controller control and update the affected resource metadata without the listed manual workaround.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, infrastructure
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.