profile controller fails to update profile owner

[thesuperzapper]

/kind bug

Related to: kubeflow/kubeflow#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:
   • 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[] 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 Way to "import" existing namespaces #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
     • 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
   • ResourceQuota/kf-resource-quota
     • SOLUTION: we can check the full ResourceQuota definition for differences, rather than only the Spec

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

[thesuperzapper]

/priority p1

[thesuperzapper]

/cc @apo-ger @kimwnasptd

[Souheil-Yazji]

+1 we've run into this issue at Statistics Canada's Kubeflow hosting. We've had instances where profile ownership changes were needed and the associated rolebinding must be manually updated. Looking forward to seeing this bug fixed :)

[thesuperzapper]

@kimwnasptd we really should fix this one, lol

[thesuperzapper]

@tzstoyanov, you expressed an interest in finding a good first PR, and I think this issue is aligned with your experience with Go controllers.

[juliusvonkohout]

This sound like an important improvement. The controllers are also usually written in a very ugly ways. @tzstoyanov can refactor it at the same time and add the functionality to read further annotations or labels from environment variables and add them to the new profile namespace for PSS. Maybe Tara from capital one @tarat44 can help as well. Related to kubeflow/manifests#2455

[tarat44]

This sound like an important improvement. The controllers are also usually written in a very ugly ways. @tzstoyanov can refactor it at the same time and add the functionality to read further annotations or labels from environment variables and add them to the new profile namespace for PSS. Maybe Tara from capital one @tarat44 can help as well. Related to kubeflow/manifests#2455

I would be happy to help out

[juliusvonkohout]

@tarat44 just connect with @tzsoyanov on Slack. But since he might be on vacation, you can just start. I have created https://github.com/juliusvonkohout/kubeflow/tree/fix-profile-controller-security as work environment. Just push and rebase (not merge from master) as needed. I have added both of you as collaborators, as i did last time with kubeflow/manifests#2455 (comment). You should have access in both forks now. Just push to the fix-profile-controller security branch. i will review and also push changes to that branch as we did for the istio-cni branch. @tarat44 you might need to sign the google CLA as a private individual. See https://cla.developers.google.com/ for more info about Google's Contributor License Agreement (CLA).

[kimwnasptd]

Yep, we should try and fix this one

[juliusvonkohout]

@thesuperzapper what do you think of kubeflow/kubeflow#7276 ?

[thesuperzapper]

@juliusvonkohout I responded in kubeflow/kubeflow#7276 (comment), but unless others want it, because it's a big change, we might need to wait until Kubeflow 1.8.1/1.9.0 to ship it.

[juliusvonkohout]

I am on vacation, but we can push it in the next kubeflow notebooks meeting

[juliusvonkohout]

TODO: we should also think about deboarding a namespace i.e. deleting the kubeflow profile without deleting the namespace. I will create an issue for that.

[andreyvelich]

/transfer dashboard

[google-oss-prow]

@thesuperzapper: The label(s) kind/bug cannot be applied, because the repository doesn't have them.

In response to this:

/kind bug

Related to: kubeflow/kubeflow#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:
   • 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[] 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 Way to "import" existing namespaces #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
     • 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
   • ResourceQuota/kf-resource-quota
     • SOLUTION: we can check the full ResourceQuota definition for differences, rather than only the Spec

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

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.