From a30effde5cb2618eade04fa0066ecba5ee49da3f Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Fri, 17 Jan 2025 13:01:18 +0200 Subject: [PATCH 01/23] classes: Add dict for Contributors in Profile --- src/profiles_management/pmr/classes.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/profiles_management/pmr/classes.py b/src/profiles_management/pmr/classes.py index 995fbe1..763cd6f 100644 --- a/src/profiles_management/pmr/classes.py +++ b/src/profiles_management/pmr/classes.py @@ -154,6 +154,21 @@ class Profile(BaseModel): resources: Optional[ResourceQuotaSpecModel] = None contributors: Optional[List[Contributor]] = [] + _contributors_dict: dict[str, List[ContributorRole]] + + def __init__(self, **data: Any) -> None: + super().__init__(**data) + + # Group contributors based on user name for more efficient retrieval + self._contributors_dict = {} + if self.contributors is None: + return + + for contributor in self.contributors: + self._contributors_dict[contributor.name] = self._contributors_dict.get( + contributor.name, [] + ) + [contributor.role] + class ProfilesManagementRepresentation: """A class representing the Profiles and Contributors. From 4b70fa9f06cd96fb6c5aa75705d06d21f8630e6a Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Fri, 17 Jan 2025 13:01:43 +0200 Subject: [PATCH 02/23] Extend create_or_update for creating/deleting RoleBindings --- src/profiles_management/create_or_update.py | 16 +- src/profiles_management/helpers/k8s.py | 48 ++++- src/profiles_management/helpers/kfam.py | 203 +++++++++++++++++++- 3 files changed, 260 insertions(+), 7 deletions(-) diff --git a/src/profiles_management/create_or_update.py b/src/profiles_management/create_or_update.py index 42465a5..5225168 100644 --- a/src/profiles_management/create_or_update.py +++ b/src/profiles_management/create_or_update.py @@ -16,7 +16,7 @@ from lightkube import Client from lightkube.generic_resource import GenericGlobalResource -from profiles_management.helpers import profiles +from profiles_management.helpers import kfam, profiles from profiles_management.helpers.k8s import get_name from profiles_management.helpers.kfam import ( list_contributor_authorization_policies, @@ -51,7 +51,10 @@ def remove_access_to_stale_profile(client: Client, profile: GenericGlobalResourc log.info("Deleted all KFAM AuthorizationPolicies") -def create_or_update_profiles(client: Client, pmr: ProfilesManagementRepresentation): +def create_or_update_profiles( + client: Client, + pmr: ProfilesManagementRepresentation, +): """Update the cluster to ensure Profiles and contributors are updated accordingly. The function ensures that: @@ -95,3 +98,12 @@ def create_or_update_profiles(client: Client, pmr: ProfilesManagementRepresentat log.info("Creating or updating the ResourceQuota for Profile %s", profile_name) profiles.update_resource_quota(client, existing_profile, profile) + + log.info("Deleting RoleBindings that don't match Profile Contributors.") + rbs = list_contributor_rolebindings(client, profile.name) + rbs = kfam.delete_rolebindings_not_matching_profile_contributors(client, profile, rbs) + + log.info("Creating RoleBindings for Profile Contributors.") + kfam.create_rolebindings_for_profile_contributors( + client, profile, existing_rolebindings=rbs + ) diff --git a/src/profiles_management/helpers/k8s.py b/src/profiles_management/helpers/k8s.py index ed440f0..647330a 100644 --- a/src/profiles_management/helpers/k8s.py +++ b/src/profiles_management/helpers/k8s.py @@ -7,6 +7,7 @@ from lightkube.core.exceptions import ApiError from lightkube.generic_resource import GenericGlobalResource, GenericNamespacedResource from lightkube.resources.core_v1 import Namespace +from lightkube.resources.rbac_authorization_v1 import RoleBinding log = logging.getLogger(__name__) @@ -18,7 +19,7 @@ class ObjectStillExistsError(Exception): pass -def get_name(res: GenericNamespacedResource | GenericGlobalResource) -> str: +def get_name(res: GenericNamespacedResource | GenericGlobalResource | RoleBinding) -> str: """Return the name from generic lightkube resource. Args: @@ -39,6 +40,51 @@ def get_name(res: GenericNamespacedResource | GenericGlobalResource) -> str: return res.metadata.name +def get_annotations(res: GenericNamespacedResource | RoleBinding) -> dict[str, str]: + """Return annotations of a RoleBinding or AuthorizationPolicy, or an empty dict. + + Args: + res: The resource to return its annotations. + + Returns: + A dictionary with the annotations, or empty dictionary if no annotations exist. + """ + if res.metadata and res.metadata.annotations: + return res.metadata.annotations + + return {} + + +def to_rfc1123_compliant(name: str) -> str: + """Transform a given string into an RFC 1123-compliant string. + + The resulting string will: + 1. Contain at most 63 characters. + 2. Contain only lowercase alphanumeric characters or '-'. + + Args: + name: The input string to transform. + + Returns: + The RFC 1123-compliant string. + """ + compliant_str = name.lower() + compliant_str = "".join(char if char.isalnum() else "-" for char in compliant_str) + + # remove starting non-alphanum chars + i = 0 + while not compliant_str[i].isalnum(): + i += 1 + compliant_str = compliant_str[i:63] + + # remove ending non-alphanum chars + j = len(compliant_str) + while not compliant_str[j - 1].isalnum(): + j -= 1 + + return compliant_str[:j] + + @tenacity.retry(stop=tenacity.stop_after_delay(300), wait=tenacity.wait_fixed(5), reraise=True) def ensure_namespace_is_deleted(namespace: str, client: Client): """Check if the name doesn't exist with retries. diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index 00ef8ea..a42e8ca 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -1,12 +1,16 @@ """Utility module for manipulating KFAM resources.""" import logging -from typing import List +from typing import List, TypeVar +from charmed_kubeflow_chisme.lightkube.batch import delete_many from lightkube import Client from lightkube.generic_resource import GenericNamespacedResource, create_namespaced_resource from lightkube.resources.rbac_authorization_v1 import RoleBinding +from profiles_management.helpers import k8s +from profiles_management.pmr import classes + log = logging.getLogger(__name__) AuthorizationPolicy = create_namespaced_resource( @@ -15,21 +19,32 @@ kind="AuthorizationPolicy", plural="authorizationpolicies", ) +KFAMResource = TypeVar("KFAMResource", GenericNamespacedResource, RoleBinding) def has_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> bool: """Check if resource has "user" and "role" KFAM annotations. + The function will also ensure the the value for "role", in the annotations" will have + one of the expected values: admin, edit, view + Args: resource: The RoleBinding or AuthorizationPolicy to check if it has KFAM annotations. Returns: A boolean if the provided resources has a `role` and `user` annotation. """ - if resource.metadata and resource.metadata.annotations: - return "role" in resource.metadata.annotations and "user" in resource.metadata.annotations + annotations = k8s.get_annotations(resource) + if "user" not in annotations or "role" not in annotations: + return False - return False + try: + classes.ContributorRole(annotations["role"]) + except ValueError: + # String in annotation doesn't match expected KFAM role + return False + + return True def resource_is_for_profile_owner(resource: GenericNamespacedResource | RoleBinding) -> bool: @@ -50,6 +65,104 @@ def resource_is_for_profile_owner(resource: GenericNamespacedResource | RoleBind return False +def get_contributor_user(resource: GenericNamespacedResource | RoleBinding) -> str: + """Return user in KFAM annotation. + + Raises: + ValueError: If the object does not have KFAM annotations. + + Returns: + The user defined in metadata.annotations.user of the resource. + """ + if not has_kfam_annotations(resource): + raise ValueError("Resource doesn't have KFAM metadata: %s" % k8s.get_name(resource)) + + annotations = k8s.get_annotations(resource) + return annotations["user"] + + +def get_contributor_role( + resource: GenericNamespacedResource | RoleBinding, +) -> classes.ContributorRole: + """Return role in KFAM annotation. + + Raises: + ValueError: If the object does not have valid KFAM annotations. + + Returns: + The user defined in metadata.annotations.user of the resource. + """ + if not has_kfam_annotations(resource): + raise ValueError("Resource doesn't have KFAM metadata: %s" % k8s.get_name(resource)) + + annotations = k8s.get_annotations(resource) + return classes.ContributorRole(annotations["role"]) + + +def resource_matches_profile_contributor( + resource: RoleBinding | GenericNamespacedResource, profile: classes.Profile +) -> bool: + """Check if the user and it's role in the RoleBinding match the PMR. + + Args: + resource: The AuthorizationPolicy or RoleBinding to check if it matches any + Contributor in the PMR Profile. + profile: The PMR Profile to check if the resource is matching it. + + Returns: + A boolean representing if the resources matches the expected contributor + """ + if profile.contributors is None: + return False + + if not has_kfam_annotations(resource): + return False + + role = get_contributor_role(resource) + user = get_contributor_user(resource) + for contributor_role in profile._contributors_dict.get(user, []): + if contributor_role == role: + return True + + return False + + +def generate_contributor_rolebinding( + contributor: classes.Contributor, namespace: str +) -> RoleBinding: + """Generate RoleBinding for a PMR Contributor. + + Args: + contributor: The PMR Contributor to generate a RoleBinding for. + namespace: The namespace to use for the RoleBinding. + + Returns: + The generated RoleBinding lightkube object for the contributor. + """ + name_rfc1123 = k8s.to_rfc1123_compliant(f"{contributor.name}-{contributor.role}") + + return RoleBinding.from_dict( + { + "metadata": { + "name": name_rfc1123, + "namespace": namespace, + "annotations": { + "user": contributor.name, + "role": contributor.role, + }, + }, + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": f"kubeflow-{contributor.role}", + }, + "subjects": [ + {"apiGroup": "rbac.authorization.k8s.io", "kind": "User", "name": contributor.name} + ], + }, + ) + + def list_contributor_rolebindings(client: Client, namespace="") -> List[RoleBinding]: """Return a list of KFAM RoleBindings. @@ -104,3 +217,85 @@ def list_contributor_authorization_policies( for ap in authorization_policies if has_kfam_annotations(ap) and not resource_is_for_profile_owner(ap) ] + + +def kfam_resources_list_to_roles_dict( + resources: List[RoleBinding] | List[GenericNamespacedResource], +) -> dict[str, List[classes.ContributorRole]]: + """Convert list of KFAM RoleBindings or AuthorizationPolicies to dict. + + The user of the resource will be used as a key and its role as the value. + + Args: + resources: List of KFAM RoleBindings or AuthorizationPolicies. + + Returns: + Dictionary with keys the user names and values the roles, derived from parsing all + the provided resources. + """ + contributor_roles_dict = {} + for resource in resources: + if has_kfam_annotations(resource): + user = get_contributor_user(resource) + role = get_contributor_role(resource) + contributor_roles_dict[user] = contributor_roles_dict.get(user, []) + [role] + + return contributor_roles_dict + + +def delete_rolebindings_not_matching_profile_contributors( + client: Client, + profile: classes.Profile, + existing_rolebindings: List[RoleBinding], +) -> List[RoleBinding]: + """Delete RoleBindings in the cluster that doesn't match Contributors in PMR Profile. + + Args: + client: The lightkube client to use. + profile: The PMR Profile to create RoleBindings based on its Contributors. + existing_rolebindings: RoleBindings in the cluster that will be evaluated for deletion. + + Returns: + The remaining resources, after removing the deleted ones from the existing_resources. + """ + role_bindings_to_delete = [] + remaining_role_bindings = [] + + for rb in existing_rolebindings: + if not resource_matches_profile_contributor(rb, profile): + log.info( + "RoleBinding '%s' doesn't belong to Profile. Will delete it.", + k8s.get_name(rb), + ) + role_bindings_to_delete.append(rb) + else: + remaining_role_bindings.append(rb) + + log.info("Deleting all resources that don't match the PMR.") + delete_many(client, role_bindings_to_delete, logger=log) + + return remaining_role_bindings + + +def create_rolebindings_for_profile_contributors( + client: Client, + profile: classes.Profile, + existing_rolebindings: List[RoleBinding], +): + """Create RoleBindings for all contributors defined in a Profile, in the PMR. + + If a RoleBinding already exists for the specific Contributor name and role, then + no API requests will happen. + + Args: + client: The lightkube client to use. + profile: The PMR to iterate over its Contributors. + existing_rolebindings: List of existing RoleBindings, to avoid doing redundant + API requests + """ + existing_contributor_roles = kfam_resources_list_to_roles_dict(existing_rolebindings) + + for contributor in profile.contributors or []: + if contributor.role not in existing_contributor_roles.get(contributor.name, []): + log.info("Will create RoleBinding for Contributor: %s", contributor) + client.apply(generate_contributor_rolebinding(contributor, profile.name)) From cdbc5011ff2038a2423e5ed5f21dc31861d92a0f Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Fri, 17 Jan 2025 13:01:50 +0200 Subject: [PATCH 03/23] Unit tests --- .../profiles-management/helpers/test_k8s.py | 31 ++++++++ .../profiles-management/helpers/test_kfam.py | 78 ++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 tests/unit/profiles-management/helpers/test_k8s.py diff --git a/tests/unit/profiles-management/helpers/test_k8s.py b/tests/unit/profiles-management/helpers/test_k8s.py new file mode 100644 index 0000000..82ac028 --- /dev/null +++ b/tests/unit/profiles-management/helpers/test_k8s.py @@ -0,0 +1,31 @@ +from lightkube.generic_resource import GenericNamespacedResource +from lightkube.models.meta_v1 import ObjectMeta + +from profiles_management.helpers.k8s import get_annotations, to_rfc1123_compliant + + +def test_annotations(): + resource = GenericNamespacedResource( + metadata=ObjectMeta(name="test", annotations={"test": "value"}) + ) + + assert get_annotations(resource) == {"test": "value"} + + +def test_no_annotations(): + resource = GenericNamespacedResource(metadata=ObjectMeta(name="test")) + + assert get_annotations(resource) == {} + + +def test_non_alphanum_to_hyphen(): + name = "kimonas@canonical.com" + + assert to_rfc1123_compliant(name) == "kimonas-canonical-com" + + +def test_more_than_63_char_name(): + name = "I-had-to-think-of-a-reeeally-long-string-to-use-for-the-test-which-was-tough" + + assert len(name) > 63 + assert len(to_rfc1123_compliant(name)) == 63 diff --git a/tests/unit/profiles-management/helpers/test_kfam.py b/tests/unit/profiles-management/helpers/test_kfam.py index 2cc04a7..a9045e7 100644 --- a/tests/unit/profiles-management/helpers/test_kfam.py +++ b/tests/unit/profiles-management/helpers/test_kfam.py @@ -1,8 +1,17 @@ import pytest from lightkube.generic_resource import GenericNamespacedResource from lightkube.models.meta_v1 import ObjectMeta +from lightkube.models.rbac_v1 import RoleRef +from lightkube.resources.rbac_authorization_v1 import RoleBinding -from profiles_management.helpers.kfam import has_kfam_annotations, resource_is_for_profile_owner +from profiles_management.helpers.kfam import ( + get_contributor_role, + get_contributor_user, + has_kfam_annotations, + resource_is_for_profile_owner, + resource_matches_profile_contributor, +) +from profiles_management.pmr.classes import Contributor, ContributorRole, Owner, Profile, UserKind def test_kfam_resource(): @@ -13,6 +22,14 @@ def test_kfam_resource(): assert has_kfam_annotations(resource) +def test_wrong_kfam_role_annotation(): + resource = GenericNamespacedResource( + metadata=ObjectMeta(name="test", annotations={"role": "overlord", "user": "test"}) + ) + + assert not has_kfam_annotations(resource) + + def test_non_kfam_resource(): resource = GenericNamespacedResource(metadata=ObjectMeta(name="test")) @@ -34,3 +51,62 @@ def test_non_profile_owner_resource(): resource = GenericNamespacedResource(metadata=ObjectMeta(name="random")) assert not resource_is_for_profile_owner(resource) + + +def test_contributor_getters(): + user = "test" + role = ContributorRole.ADMIN + resource = GenericNamespacedResource( + metadata=ObjectMeta(name="test", annotations={"role": role, "user": user}) + ) + + assert get_contributor_user(resource) == user + assert get_contributor_role(resource) == role + + +def test_rolebinding_not_matching_empty_profile_contributors(): + rb = RoleBinding( + metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), + roleRef=RoleRef(apiGroup="", kind="", name=""), + ) + + profile = Profile( + name="test", + owner=Owner(name="test", kind=UserKind.USER), + contributors=[], + resources={}, + ) + + assert not resource_matches_profile_contributor(rb, profile) + + +def test_rolebinding_not_matching_profile_contributors(): + rb = RoleBinding( + metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), + roleRef=RoleRef(apiGroup="", kind="", name=""), + ) + + profile = Profile( + name="test", + owner=Owner(name="test", kind=UserKind.USER), + contributors=[Contributor(name="test", role=ContributorRole.VIEW)], + resources={}, + ) + + assert not resource_matches_profile_contributor(rb, profile) + + +def test_rolebinding_matching_profile_contributors(): + rb = RoleBinding( + metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), + roleRef=RoleRef(apiGroup="", kind="", name=""), + ) + + profile = Profile( + name="test", + owner=Owner(name="test", kind=UserKind.USER), + contributors=[Contributor(name="test", role=ContributorRole.ADMIN)], + resources={}, + ) + + assert resource_matches_profile_contributor(rb, profile) From 076d6caa58a24f09d65153aa5b7753e736163d44 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Fri, 17 Jan 2025 13:01:57 +0200 Subject: [PATCH 04/23] Integration tests --- .../profiles_management/helpers/profiles.py | 22 ++++ .../test_create_or_update_profiles.py | 114 ++++++++++++++---- 2 files changed, 115 insertions(+), 21 deletions(-) diff --git a/tests/integration/profiles_management/helpers/profiles.py b/tests/integration/profiles_management/helpers/profiles.py index 964cb1d..a152697 100644 --- a/tests/integration/profiles_management/helpers/profiles.py +++ b/tests/integration/profiles_management/helpers/profiles.py @@ -105,3 +105,25 @@ def remove_profile(profile: GenericGlobalResource, client: Client, wait_namespac if wait_namespace: log.info("Waiting for created namespace to be deleted.") k8s.ensure_namespace_is_deleted(nm, client) + + +def apply_profile_and_resources( + client: Client, + profile_path: str, + namespace: str, + resources_path="", +) -> GenericGlobalResource: + context = {"namespace": namespace} + + # Load and apply all objects from files + log.info("Creating Profile and waiting for Namespace to be created...") + profile_contents = load_profile_from_file(profile_path, context) + profile = apply_profile(profile_contents, client) + + if resources_path: + resources = k8s.load_namespaced_objects_from_file(resources_path, context) + log.info("Applying all namespaced contributor resources.") + for resource in resources: + client.apply(resource) + + return profile diff --git a/tests/integration/profiles_management/test_create_or_update_profiles.py b/tests/integration/profiles_management/test_create_or_update_profiles.py index 84fc41a..c06bd08 100644 --- a/tests/integration/profiles_management/test_create_or_update_profiles.py +++ b/tests/integration/profiles_management/test_create_or_update_profiles.py @@ -6,17 +6,21 @@ from profiles_management.create_or_update import create_or_update_profiles from profiles_management.pmr import classes from profiles_management.pmr.classes import ( + Contributor, + ContributorRole, Owner, Profile, ProfilesManagementRepresentation, ResourceQuotaSpecModel, UserKind, ) -from tests.integration.profiles_management.helpers import k8s, kfam, profiles +from tests.integration.profiles_management.helpers import kfam, profiles log = logging.getLogger(__name__) TESTS_YAMLS_PATH = "tests/integration/profiles_management/yamls" +PROFILE_PATH = TESTS_YAMLS_PATH + "/profile.yaml" +RESOURCES_PATH = TESTS_YAMLS_PATH + "/contributor.yaml" @pytest.mark.asyncio @@ -26,21 +30,9 @@ async def test_remove_access_to_stale_profiles( await deploy_profiles_controller ns = "test" - context = {"namespace": ns} - - profile_path = TESTS_YAMLS_PATH + "/profile.yaml" - contributor_path = TESTS_YAMLS_PATH + "/contributor.yaml" - - # Load and apply all objects from files - profile_contents = profiles.load_profile_from_file(profile_path, context) - resources = k8s.load_namespaced_objects_from_file(contributor_path, context) - - log.info("Creating Profile and waiting for Namespace to be created...") - profile = profiles.apply_profile(profile_contents, lightkube_client) - - log.info("Applying all namespaced contributor resources.") - for resource in resources: - lightkube_client.apply(resource) + profile = profiles.apply_profile_and_resources( + lightkube_client, profile_path=PROFILE_PATH, resources_path=RESOURCES_PATH, namespace=ns + ) # Create the PMR, which should not contain the above test profile pmr = classes.ProfilesManagementRepresentation() @@ -91,13 +83,11 @@ async def test_update_resource_quota(lightkube_client: Client): log.info("Loading test YAMLs from: %s", profile_path) ns = "test" - context = {"namespace": ns} - profile_contents = profiles.load_profile_from_file(profile_path, context) + profile = profiles.apply_profile_and_resources( + lightkube_client, profile_path=PROFILE_PATH, namespace=ns + ) - log.info("Creating Profile and waiting for Namespace to be created...") - profile = profiles.apply_profile(profile_contents, lightkube_client, wait_namespace=True) log.info("Created Profile has quota: %s", profile["spec"]["resourceQuotaSpec"]) - expected_quota = ResourceQuotaSpecModel.model_validate({"hard": {"cpu": "1"}}) pmr_profile = Profile( name=ns, @@ -121,3 +111,85 @@ async def test_update_resource_quota(lightkube_client: Client): log.info("Removing test Profile and resources in it") profiles.remove_profile(profile, lightkube_client, wait_namespace=True) + + +def test_surplus_rolebindings_are_deleted(lightkube_client: Client): + ns = "test-surplus-rolebindings-are-deleted" + profile = profiles.apply_profile_and_resources( + lightkube_client, profile_path=PROFILE_PATH, resources_path=RESOURCES_PATH, namespace=ns + ) + + pmr_profile = Profile( + name=ns, + owner=Owner(name=ns, kind=UserKind.USER), + contributors=[], + resources={}, + ) + + log.info("Deleting superfluous RoleBindings from ") + create_or_update_profiles(lightkube_client, ProfilesManagementRepresentation([pmr_profile])) + + rbs = kfam.list_contributor_rolebindings(lightkube_client, ns) + assert len(rbs) == 0 + + profiles.remove_profile(profile, lightkube_client) + + +def test_existing_rolebindings_are_updated(lightkube_client: Client): + """Existing RoleBinding for "permissions" should be updated to "admin".""" + ns = "test-existing-rolebindings-updated" + profile = profiles.apply_profile_and_resources( + lightkube_client, profile_path=PROFILE_PATH, resources_path=RESOURCES_PATH, namespace=ns + ) + + user = "kimonas@canonical.com" + role = ContributorRole.ADMIN + pmr_profile = Profile( + name=ns, + owner=Owner(name=ns, kind=UserKind.USER), + contributors=[Contributor(name=user, role=role)], + resources={}, + ) + + log.info("Updating existing RoleBindings from edit to be admin.") + create_or_update_profiles(lightkube_client, ProfilesManagementRepresentation([pmr_profile])) + + rbs = kfam.list_contributor_rolebindings(lightkube_client, ns) + assert len(rbs) == 1 + + assert rbs[0].metadata is not None + assert rbs[0].metadata.annotations is not None + assert rbs[0].metadata.annotations["user"] == user + assert rbs[0].metadata.annotations["role"] == role + + profiles.remove_profile(profile, lightkube_client) + + +def test_rolebindings_are_created(lightkube_client: Client): + """Existing RoleBinding for "permissions" should be updated to "admin".""" + ns = "test-rolebindings-created" + profile = profiles.apply_profile_and_resources( + lightkube_client, profile_path=PROFILE_PATH, namespace=ns + ) + + user = "kimonas@canonical.com" + role = ContributorRole.ADMIN + pmr_profile = Profile( + name=ns, + owner=Owner(name=ns, kind=UserKind.USER), + contributors=[Contributor(name=user, role=role)], + resources={}, + ) + + log.info("Creating RoleBinding for admin role.") + create_or_update_profiles(lightkube_client, ProfilesManagementRepresentation([pmr_profile])) + + rbs = kfam.list_contributor_rolebindings(lightkube_client, ns) + assert len(rbs) == 1 + + assert rbs[0].metadata is not None + assert rbs[0].metadata.annotations is not None + assert rbs[0].metadata.annotations["user"] == user + assert rbs[0].metadata.annotations["role"] == role + + profiles.remove_profile(profile, lightkube_client) From fb80f7e228e66c278e1e8771de7bf3bf25ff7c7d Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 07:40:53 +0000 Subject: [PATCH 05/23] review(nit): Update log message --- src/profiles_management/create_or_update.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/profiles_management/create_or_update.py b/src/profiles_management/create_or_update.py index 5225168..1b25fba 100644 --- a/src/profiles_management/create_or_update.py +++ b/src/profiles_management/create_or_update.py @@ -99,11 +99,11 @@ def create_or_update_profiles( log.info("Creating or updating the ResourceQuota for Profile %s", profile_name) profiles.update_resource_quota(client, existing_profile, profile) - log.info("Deleting RoleBindings that don't match Profile Contributors.") + log.info("Deleting RoleBindings that don't match Profile: %s", profile_name) rbs = list_contributor_rolebindings(client, profile.name) rbs = kfam.delete_rolebindings_not_matching_profile_contributors(client, profile, rbs) - log.info("Creating RoleBindings for Profile Contributors.") + log.info("Creating RoleBindings for Profile: %s", profile_name) kfam.create_rolebindings_for_profile_contributors( client, profile, existing_rolebindings=rbs ) From 0385ffe2cf2ccaad4b42fa3ea2fc349cc38bf20c Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 07:44:58 +0000 Subject: [PATCH 06/23] review: Remove redundant KFPResource class --- src/profiles_management/helpers/kfam.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index a42e8ca..0758334 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -19,7 +19,6 @@ kind="AuthorizationPolicy", plural="authorizationpolicies", ) -KFAMResource = TypeVar("KFAMResource", GenericNamespacedResource, RoleBinding) def has_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> bool: From b080752f39cd491b0534e8b30fc55ff31d3eaca1 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 09:55:31 +0200 Subject: [PATCH 07/23] Update src/profiles_management/helpers/kfam.py Co-authored-by: Daniela Plascencia --- src/profiles_management/helpers/kfam.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index 0758334..f0f9c36 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -280,7 +280,7 @@ def create_rolebindings_for_profile_contributors( client: Client, profile: classes.Profile, existing_rolebindings: List[RoleBinding], -): +) -> None: """Create RoleBindings for all contributors defined in a Profile, in the PMR. If a RoleBinding already exists for the specific Contributor name and role, then From 08616e7a8ee4006323f836fc498330c147be8799 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 08:06:14 +0000 Subject: [PATCH 08/23] review: Use regex in rfc1123 helper --- src/profiles_management/helpers/k8s.py | 19 +++++++------------ .../profiles-management/helpers/test_k8s.py | 10 ++++++++-- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/profiles_management/helpers/k8s.py b/src/profiles_management/helpers/k8s.py index 647330a..9b4b92e 100644 --- a/src/profiles_management/helpers/k8s.py +++ b/src/profiles_management/helpers/k8s.py @@ -1,6 +1,7 @@ """Generic helpers for manipulating K8s objects, via lightkube.""" import logging +import re import tenacity from lightkube import Client @@ -68,21 +69,15 @@ def to_rfc1123_compliant(name: str) -> str: Returns: The RFC 1123-compliant string. """ - compliant_str = name.lower() - compliant_str = "".join(char if char.isalnum() else "-" for char in compliant_str) + if len(name) == 0: + raise ValueError("Can't convert to valid RFC1123 an empty string.") - # remove starting non-alphanum chars - i = 0 - while not compliant_str[i].isalnum(): - i += 1 - compliant_str = compliant_str[i:63] + compliant_str = name.lower() + compliant_str = re.sub(r"[^a-z0-9-]", "-", compliant_str) - # remove ending non-alphanum chars - j = len(compliant_str) - while not compliant_str[j - 1].isalnum(): - j -= 1 + compliant_str = compliant_str.lstrip("-").rstrip("-") - return compliant_str[:j] + return compliant_str[:63] @tenacity.retry(stop=tenacity.stop_after_delay(300), wait=tenacity.wait_fixed(5), reraise=True) diff --git a/tests/unit/profiles-management/helpers/test_k8s.py b/tests/unit/profiles-management/helpers/test_k8s.py index 82ac028..ed77965 100644 --- a/tests/unit/profiles-management/helpers/test_k8s.py +++ b/tests/unit/profiles-management/helpers/test_k8s.py @@ -18,14 +18,20 @@ def test_no_annotations(): assert get_annotations(resource) == {} -def test_non_alphanum_to_hyphen(): +def test_rfc1123_non_alphanum_to_hyphen(): name = "kimonas@canonical.com" assert to_rfc1123_compliant(name) == "kimonas-canonical-com" -def test_more_than_63_char_name(): +def test_rfc1123_more_than_63_char_name(): name = "I-had-to-think-of-a-reeeally-long-string-to-use-for-the-test-which-was-tough" assert len(name) > 63 assert len(to_rfc1123_compliant(name)) == 63 + + +def test_rfc1123_strip_starting_and_trailing_dashes(): + name = "-=shouldn't have trailing dashes!" + + assert to_rfc1123_compliant(name) == "shouldn-t-have-trailing-dashes" From 68426683245dceefdbc35b25e50981d1ea46aacf Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 08:35:28 +0000 Subject: [PATCH 09/23] review: Refactor delete_rolebindings_not_matching_profile_contributors --- src/profiles_management/create_or_update.py | 7 ++----- src/profiles_management/helpers/kfam.py | 20 ++++++-------------- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/src/profiles_management/create_or_update.py b/src/profiles_management/create_or_update.py index 1b25fba..0f29535 100644 --- a/src/profiles_management/create_or_update.py +++ b/src/profiles_management/create_or_update.py @@ -100,10 +100,7 @@ def create_or_update_profiles( profiles.update_resource_quota(client, existing_profile, profile) log.info("Deleting RoleBindings that don't match Profile: %s", profile_name) - rbs = list_contributor_rolebindings(client, profile.name) - rbs = kfam.delete_rolebindings_not_matching_profile_contributors(client, profile, rbs) + kfam.delete_rolebindings_not_matching_profile_contributors(client, profile) log.info("Creating RoleBindings for Profile: %s", profile_name) - kfam.create_rolebindings_for_profile_contributors( - client, profile, existing_rolebindings=rbs - ) + kfam.create_rolebindings_for_profile_contributors(client, profile) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index f0f9c36..97bbbbc 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -245,20 +245,15 @@ def kfam_resources_list_to_roles_dict( def delete_rolebindings_not_matching_profile_contributors( client: Client, profile: classes.Profile, - existing_rolebindings: List[RoleBinding], -) -> List[RoleBinding]: +) -> None: """Delete RoleBindings in the cluster that doesn't match Contributors in PMR Profile. Args: client: The lightkube client to use. profile: The PMR Profile to create RoleBindings based on its Contributors. - existing_rolebindings: RoleBindings in the cluster that will be evaluated for deletion. - - Returns: - The remaining resources, after removing the deleted ones from the existing_resources. """ + existing_rolebindings = list_contributor_rolebindings(client, profile.name) role_bindings_to_delete = [] - remaining_role_bindings = [] for rb in existing_rolebindings: if not resource_matches_profile_contributor(rb, profile): @@ -267,19 +262,15 @@ def delete_rolebindings_not_matching_profile_contributors( k8s.get_name(rb), ) role_bindings_to_delete.append(rb) - else: - remaining_role_bindings.append(rb) - - log.info("Deleting all resources that don't match the PMR.") - delete_many(client, role_bindings_to_delete, logger=log) - return remaining_role_bindings + if role_bindings_to_delete: + log.info("Deleting all resources that don't match the PMR.") + delete_many(client, role_bindings_to_delete, logger=log) def create_rolebindings_for_profile_contributors( client: Client, profile: classes.Profile, - existing_rolebindings: List[RoleBinding], ) -> None: """Create RoleBindings for all contributors defined in a Profile, in the PMR. @@ -292,6 +283,7 @@ def create_rolebindings_for_profile_contributors( existing_rolebindings: List of existing RoleBindings, to avoid doing redundant API requests """ + existing_rolebindings = list_contributor_rolebindings(client, profile.name) existing_contributor_roles = kfam_resources_list_to_roles_dict(existing_rolebindings) for contributor in profile.contributors or []: From 3bd4e277c1f82ae1a248d3ec10a46f92d0f57a56 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 08:55:08 +0000 Subject: [PATCH 10/23] review: Refactor resource_matches_profile_contributor --- src/profiles_management/helpers/kfam.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index 97bbbbc..8165010 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -111,17 +111,14 @@ def resource_matches_profile_contributor( Returns: A boolean representing if the resources matches the expected contributor """ - if profile.contributors is None: - return False - - if not has_kfam_annotations(resource): + if not profile.contributors or not has_kfam_annotations(resource): + log.info("No profile contributors or kfam annotations were found in the resource.") return False role = get_contributor_role(resource) user = get_contributor_user(resource) - for contributor_role in profile._contributors_dict.get(user, []): - if contributor_role == role: - return True + if role in profile._contributors_dict.get(user, []): + return True return False From 7a076f4cf92bf298d2be43bb9bf179d9933116cc Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 09:02:14 +0000 Subject: [PATCH 11/23] review: Different handling for empty contributors --- src/profiles_management/helpers/kfam.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index 8165010..a711a4d 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -283,7 +283,10 @@ def create_rolebindings_for_profile_contributors( existing_rolebindings = list_contributor_rolebindings(client, profile.name) existing_contributor_roles = kfam_resources_list_to_roles_dict(existing_rolebindings) - for contributor in profile.contributors or []: + if not profile.contributors: + return + + for contributor in profile.contributors: if contributor.role not in existing_contributor_roles.get(contributor.name, []): log.info("Will create RoleBinding for Contributor: %s", contributor) client.apply(generate_contributor_rolebinding(contributor, profile.name)) From bc814e88895763532bf7f392c024b9d55a74318a Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Tue, 21 Jan 2025 09:28:07 +0000 Subject: [PATCH 12/23] review: Rename to has_valid_kfam_annotations --- src/profiles_management/helpers/kfam.py | 50 +++++++++---------- .../profiles-management/helpers/test_kfam.py | 8 +-- 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index a711a4d..afa7f58 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -1,7 +1,7 @@ """Utility module for manipulating KFAM resources.""" import logging -from typing import List, TypeVar +from typing import List from charmed_kubeflow_chisme.lightkube.batch import delete_many from lightkube import Client @@ -9,7 +9,7 @@ from lightkube.resources.rbac_authorization_v1 import RoleBinding from profiles_management.helpers import k8s -from profiles_management.pmr import classes +from profiles_management.pmr.classes import Contributor, ContributorRole, Profile log = logging.getLogger(__name__) @@ -21,7 +21,7 @@ ) -def has_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> bool: +def has_valid_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> bool: """Check if resource has "user" and "role" KFAM annotations. The function will also ensure the the value for "role", in the annotations" will have @@ -34,16 +34,14 @@ def has_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> b A boolean if the provided resources has a `role` and `user` annotation. """ annotations = k8s.get_annotations(resource) - if "user" not in annotations or "role" not in annotations: - return False - - try: - classes.ContributorRole(annotations["role"]) - except ValueError: - # String in annotation doesn't match expected KFAM role - return False + if annotations: + return ( + "user" in annotations + and "role" in annotations + and annotations["role"].upper() in ContributorRole.__members__ + ) - return True + return False def resource_is_for_profile_owner(resource: GenericNamespacedResource | RoleBinding) -> bool: @@ -73,7 +71,7 @@ def get_contributor_user(resource: GenericNamespacedResource | RoleBinding) -> s Returns: The user defined in metadata.annotations.user of the resource. """ - if not has_kfam_annotations(resource): + if not has_valid_kfam_annotations(resource): raise ValueError("Resource doesn't have KFAM metadata: %s" % k8s.get_name(resource)) annotations = k8s.get_annotations(resource) @@ -82,7 +80,7 @@ def get_contributor_user(resource: GenericNamespacedResource | RoleBinding) -> s def get_contributor_role( resource: GenericNamespacedResource | RoleBinding, -) -> classes.ContributorRole: +) -> ContributorRole: """Return role in KFAM annotation. Raises: @@ -91,15 +89,15 @@ def get_contributor_role( Returns: The user defined in metadata.annotations.user of the resource. """ - if not has_kfam_annotations(resource): + if not has_valid_kfam_annotations(resource): raise ValueError("Resource doesn't have KFAM metadata: %s" % k8s.get_name(resource)) annotations = k8s.get_annotations(resource) - return classes.ContributorRole(annotations["role"]) + return ContributorRole(annotations["role"]) def resource_matches_profile_contributor( - resource: RoleBinding | GenericNamespacedResource, profile: classes.Profile + resource: RoleBinding | GenericNamespacedResource, profile: Profile ) -> bool: """Check if the user and it's role in the RoleBinding match the PMR. @@ -111,7 +109,7 @@ def resource_matches_profile_contributor( Returns: A boolean representing if the resources matches the expected contributor """ - if not profile.contributors or not has_kfam_annotations(resource): + if not profile.contributors or not has_valid_kfam_annotations(resource): log.info("No profile contributors or kfam annotations were found in the resource.") return False @@ -123,9 +121,7 @@ def resource_matches_profile_contributor( return False -def generate_contributor_rolebinding( - contributor: classes.Contributor, namespace: str -) -> RoleBinding: +def generate_contributor_rolebinding(contributor: Contributor, namespace: str) -> RoleBinding: """Generate RoleBinding for a PMR Contributor. Args: @@ -182,7 +178,7 @@ def list_contributor_rolebindings(client: Client, namespace="") -> List[RoleBind return [ rb for rb in role_bindings - if has_kfam_annotations(rb) and not resource_is_for_profile_owner(rb) + if has_valid_kfam_annotations(rb) and not resource_is_for_profile_owner(rb) ] @@ -211,13 +207,13 @@ def list_contributor_authorization_policies( return [ ap for ap in authorization_policies - if has_kfam_annotations(ap) and not resource_is_for_profile_owner(ap) + if has_valid_kfam_annotations(ap) and not resource_is_for_profile_owner(ap) ] def kfam_resources_list_to_roles_dict( resources: List[RoleBinding] | List[GenericNamespacedResource], -) -> dict[str, List[classes.ContributorRole]]: +) -> dict[str, List[ContributorRole]]: """Convert list of KFAM RoleBindings or AuthorizationPolicies to dict. The user of the resource will be used as a key and its role as the value. @@ -231,7 +227,7 @@ def kfam_resources_list_to_roles_dict( """ contributor_roles_dict = {} for resource in resources: - if has_kfam_annotations(resource): + if has_valid_kfam_annotations(resource): user = get_contributor_user(resource) role = get_contributor_role(resource) contributor_roles_dict[user] = contributor_roles_dict.get(user, []) + [role] @@ -241,7 +237,7 @@ def kfam_resources_list_to_roles_dict( def delete_rolebindings_not_matching_profile_contributors( client: Client, - profile: classes.Profile, + profile: Profile, ) -> None: """Delete RoleBindings in the cluster that doesn't match Contributors in PMR Profile. @@ -267,7 +263,7 @@ def delete_rolebindings_not_matching_profile_contributors( def create_rolebindings_for_profile_contributors( client: Client, - profile: classes.Profile, + profile: Profile, ) -> None: """Create RoleBindings for all contributors defined in a Profile, in the PMR. diff --git a/tests/unit/profiles-management/helpers/test_kfam.py b/tests/unit/profiles-management/helpers/test_kfam.py index a9045e7..3d7c1af 100644 --- a/tests/unit/profiles-management/helpers/test_kfam.py +++ b/tests/unit/profiles-management/helpers/test_kfam.py @@ -7,7 +7,7 @@ from profiles_management.helpers.kfam import ( get_contributor_role, get_contributor_user, - has_kfam_annotations, + has_valid_kfam_annotations, resource_is_for_profile_owner, resource_matches_profile_contributor, ) @@ -19,7 +19,7 @@ def test_kfam_resource(): metadata=ObjectMeta(name="test", annotations={"role": "admin", "user": "test"}) ) - assert has_kfam_annotations(resource) + assert has_valid_kfam_annotations(resource) def test_wrong_kfam_role_annotation(): @@ -27,13 +27,13 @@ def test_wrong_kfam_role_annotation(): metadata=ObjectMeta(name="test", annotations={"role": "overlord", "user": "test"}) ) - assert not has_kfam_annotations(resource) + assert not has_valid_kfam_annotations(resource) def test_non_kfam_resource(): resource = GenericNamespacedResource(metadata=ObjectMeta(name="test")) - assert not has_kfam_annotations(resource) + assert not has_valid_kfam_annotations(resource) @pytest.mark.parametrize( From 8439ef1260da03d6d4aaec6e497391228a87252b Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Wed, 22 Jan 2025 16:24:23 +0200 Subject: [PATCH 13/23] Update src/profiles_management/helpers/kfam.py Co-authored-by: Manos Vlassis <57320708+mvlassis@users.noreply.github.com> --- src/profiles_management/helpers/kfam.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index afa7f58..3bf4871 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -99,7 +99,7 @@ def get_contributor_role( def resource_matches_profile_contributor( resource: RoleBinding | GenericNamespacedResource, profile: Profile ) -> bool: - """Check if the user and it's role in the RoleBinding match the PMR. + """Check if the user and its role in the RoleBinding match the PMR. Args: resource: The AuthorizationPolicy or RoleBinding to check if it matches any From 7f7c54d0e7080b87b299aad2b7de242f21029db6 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Wed, 22 Jan 2025 16:24:33 +0200 Subject: [PATCH 14/23] Update src/profiles_management/helpers/kfam.py Co-authored-by: Manos Vlassis <57320708+mvlassis@users.noreply.github.com> --- src/profiles_management/helpers/kfam.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index 3bf4871..6a98dd1 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -24,7 +24,7 @@ def has_valid_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> bool: """Check if resource has "user" and "role" KFAM annotations. - The function will also ensure the the value for "role", in the annotations" will have + The function will also ensure that the value for "role", in the annotations will have one of the expected values: admin, edit, view Args: From 6d1516a074ab219b8e6441863b2ed48b0aca8adc Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Wed, 22 Jan 2025 14:26:54 +0000 Subject: [PATCH 15/23] review: Extend docstring --- src/profiles_management/helpers/k8s.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/profiles_management/helpers/k8s.py b/src/profiles_management/helpers/k8s.py index 9b4b92e..7f79856 100644 --- a/src/profiles_management/helpers/k8s.py +++ b/src/profiles_management/helpers/k8s.py @@ -24,7 +24,8 @@ def get_name(res: GenericNamespacedResource | GenericGlobalResource | RoleBindin """Return the name from generic lightkube resource. Args: - res: The resource to get it's name from metadata.name + res: The resource (Profile, AuthorizationPolicy or RoleBinding) to get it's name + from metadata.name Raises: ValueError: if the object doesn't have metadata or metadata.name From 490cd159687d33cf4f4fa827cac80e3cf8a251cf Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 07:21:07 +0000 Subject: [PATCH 16/23] review: Custom Exception for invalid KFAM Annotations --- src/profiles_management/create_or_update.py | 4 ++++ src/profiles_management/helpers/kfam.py | 21 +++++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/profiles_management/create_or_update.py b/src/profiles_management/create_or_update.py index 0f29535..1789299 100644 --- a/src/profiles_management/create_or_update.py +++ b/src/profiles_management/create_or_update.py @@ -72,6 +72,10 @@ def create_or_update_profiles( client: The lightkube client to use. pmr: The ProfilesManagementRepresentation expressing what Profiles and contributors should exist in the cluster. + + Raises: + InvalidKfamAnnotationsError: If a RoleBinding or AuthorizationPolicy does not have + KFAM valid annotations. """ log.info("Fetching all Profiles in the cluster") diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index 6a98dd1..d2f228e 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -21,6 +21,12 @@ ) +class InvalidKfamAnnotationsError(Exception): + """Exception for when KFAM Annotations were expected but not found in object.""" + + pass + + def has_valid_kfam_annotations(resource: GenericNamespacedResource | RoleBinding) -> bool: """Check if resource has "user" and "role" KFAM annotations. @@ -66,16 +72,17 @@ def get_contributor_user(resource: GenericNamespacedResource | RoleBinding) -> s """Return user in KFAM annotation. Raises: - ValueError: If the object does not have KFAM annotations. + InvalidKfamAnnotationsError: If the object does not have KFAM annotations. Returns: The user defined in metadata.annotations.user of the resource. """ if not has_valid_kfam_annotations(resource): - raise ValueError("Resource doesn't have KFAM metadata: %s" % k8s.get_name(resource)) + raise InvalidKfamAnnotationsError( + "Resource doesn't have valid KFAM metadata: %s" % k8s.get_name(resource) + ) - annotations = k8s.get_annotations(resource) - return annotations["user"] + return k8s.get_annotations(resource)["user"] def get_contributor_role( @@ -84,13 +91,15 @@ def get_contributor_role( """Return role in KFAM annotation. Raises: - ValueError: If the object does not have valid KFAM annotations. + InvalidKfamAnnotationsError: If the object does not have valid KFAM annotations. Returns: The user defined in metadata.annotations.user of the resource. """ if not has_valid_kfam_annotations(resource): - raise ValueError("Resource doesn't have KFAM metadata: %s" % k8s.get_name(resource)) + raise InvalidKfamAnnotationsError( + "Resource doesn't have invalid KFAM metadata: %s" % k8s.get_name(resource) + ) annotations = k8s.get_annotations(resource) return ContributorRole(annotations["role"]) From 5ab93d5ac01adc51b6ca7153a89348c727b7e078 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 07:22:40 +0000 Subject: [PATCH 17/23] review: Document raised ApiError exceptions --- src/profiles_management/create_or_update.py | 2 ++ src/profiles_management/helpers/kfam.py | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/profiles_management/create_or_update.py b/src/profiles_management/create_or_update.py index 1789299..72c920f 100644 --- a/src/profiles_management/create_or_update.py +++ b/src/profiles_management/create_or_update.py @@ -74,6 +74,8 @@ def create_or_update_profiles( should exist in the cluster. Raises: + ApiError: From lightkube if an error occurred while trying to create or delete + Profiles, RoleBindings or AuthorizationPolicies. InvalidKfamAnnotationsError: If a RoleBinding or AuthorizationPolicy does not have KFAM valid annotations. """ diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index d2f228e..fca932d 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -250,9 +250,16 @@ def delete_rolebindings_not_matching_profile_contributors( ) -> None: """Delete RoleBindings in the cluster that doesn't match Contributors in PMR Profile. + The function will be handling 404 errors, in case the RoleBinding doesn't exist in the + cluster. + Args: client: The lightkube client to use. profile: The PMR Profile to create RoleBindings based on its Contributors. + + Raises: + ApiError: From lightkube if something unexpected occurred while deleting the + resources. """ existing_rolebindings = list_contributor_rolebindings(client, profile.name) role_bindings_to_delete = [] @@ -284,6 +291,10 @@ def create_rolebindings_for_profile_contributors( profile: The PMR to iterate over its Contributors. existing_rolebindings: List of existing RoleBindings, to avoid doing redundant API requests + + Raises: + ApiError: From lightkube if there was an error while trying to create the + RoleBindings. """ existing_rolebindings = list_contributor_rolebindings(client, profile.name) existing_contributor_roles = kfam_resources_list_to_roles_dict(existing_rolebindings) From 9853be5a06d53207a19ae4025fda01ee4d962b82 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 07:28:27 +0000 Subject: [PATCH 18/23] review: Handling of empty name RFC 1123 --- src/profiles_management/helpers/k8s.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/profiles_management/helpers/k8s.py b/src/profiles_management/helpers/k8s.py index 7f79856..86c2998 100644 --- a/src/profiles_management/helpers/k8s.py +++ b/src/profiles_management/helpers/k8s.py @@ -70,8 +70,8 @@ def to_rfc1123_compliant(name: str) -> str: Returns: The RFC 1123-compliant string. """ - if len(name) == 0: - raise ValueError("Can't convert to valid RFC1123 an empty string.") + if not name: + return "" compliant_str = name.lower() compliant_str = re.sub(r"[^a-z0-9-]", "-", compliant_str) From c7440602f73addc98b08e39b4110bc531392347a Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 09:20:04 +0000 Subject: [PATCH 19/23] review: Parametrize rfc1123 unit tests --- .../profiles-management/helpers/test_k8s.py | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/tests/unit/profiles-management/helpers/test_k8s.py b/tests/unit/profiles-management/helpers/test_k8s.py index ed77965..11d53d9 100644 --- a/tests/unit/profiles-management/helpers/test_k8s.py +++ b/tests/unit/profiles-management/helpers/test_k8s.py @@ -1,3 +1,4 @@ +import pytest from lightkube.generic_resource import GenericNamespacedResource from lightkube.models.meta_v1 import ObjectMeta @@ -18,20 +19,21 @@ def test_no_annotations(): assert get_annotations(resource) == {} -def test_rfc1123_non_alphanum_to_hyphen(): - name = "kimonas@canonical.com" - - assert to_rfc1123_compliant(name) == "kimonas-canonical-com" - - -def test_rfc1123_more_than_63_char_name(): - name = "I-had-to-think-of-a-reeeally-long-string-to-use-for-the-test-which-was-tough" - - assert len(name) > 63 - assert len(to_rfc1123_compliant(name)) == 63 - - -def test_rfc1123_strip_starting_and_trailing_dashes(): - name = "-=shouldn't have trailing dashes!" - - assert to_rfc1123_compliant(name) == "shouldn-t-have-trailing-dashes" +@pytest.mark.parametrize( + "name,expected", + [ + ("kimonas@canonical.com", "kimonas-canonical-com"), + ( + "I-had-to-think-of-a-reeeally-long-string-to-use-for-the-test-which-was-tough", + "i-had-to-think-of-a-reeeally-long-string-to-use-for-the-test-wh", + ), + ("-=shouldn't have trailing dashes!", "shouldn-t-have-trailing-dashes"), + ("", ""), + ("abcdefg", "abcdefg"), + ("1234", "1234"), + ("$%%@#", ""), + ], +) +def test_rfc1123(name, expected): + assert len(to_rfc1123_compliant(name)) < 64 + assert to_rfc1123_compliant(name) == expected From babe4d5519685dfcac3dacbbac47d0d825733ab4 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 09:24:22 +0000 Subject: [PATCH 20/23] review: Parametrise annotation unit tests --- .../profiles-management/helpers/test_k8s.py | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/unit/profiles-management/helpers/test_k8s.py b/tests/unit/profiles-management/helpers/test_k8s.py index 11d53d9..5c8baea 100644 --- a/tests/unit/profiles-management/helpers/test_k8s.py +++ b/tests/unit/profiles-management/helpers/test_k8s.py @@ -5,18 +5,20 @@ from profiles_management.helpers.k8s import get_annotations, to_rfc1123_compliant -def test_annotations(): - resource = GenericNamespacedResource( - metadata=ObjectMeta(name="test", annotations={"test": "value"}) - ) - - assert get_annotations(resource) == {"test": "value"} - - -def test_no_annotations(): - resource = GenericNamespacedResource(metadata=ObjectMeta(name="test")) - - assert get_annotations(resource) == {} +@pytest.mark.parametrize( + "resource,expected_annotations", + [ + ( + GenericNamespacedResource( + metadata=ObjectMeta(name="test", annotations={"test": "value"}) + ), + {"test": "value"}, + ), + (GenericNamespacedResource(metadata=ObjectMeta(name="test")), {}), + ], +) +def test_annotations(resource, expected_annotations): + assert get_annotations(resource) == expected_annotations @pytest.mark.parametrize( From 03df48080ec4f17eaae3ea122623d2bf738eeae3 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 09:40:45 +0000 Subject: [PATCH 21/23] review: Parametrise kfam unit tests --- .../profiles-management/helpers/test_kfam.py | 149 +++++++++--------- 1 file changed, 72 insertions(+), 77 deletions(-) diff --git a/tests/unit/profiles-management/helpers/test_kfam.py b/tests/unit/profiles-management/helpers/test_kfam.py index 3d7c1af..f99fdc2 100644 --- a/tests/unit/profiles-management/helpers/test_kfam.py +++ b/tests/unit/profiles-management/helpers/test_kfam.py @@ -14,43 +14,38 @@ from profiles_management.pmr.classes import Contributor, ContributorRole, Owner, Profile, UserKind -def test_kfam_resource(): - resource = GenericNamespacedResource( - metadata=ObjectMeta(name="test", annotations={"role": "admin", "user": "test"}) - ) - - assert has_valid_kfam_annotations(resource) - - -def test_wrong_kfam_role_annotation(): - resource = GenericNamespacedResource( - metadata=ObjectMeta(name="test", annotations={"role": "overlord", "user": "test"}) - ) - - assert not has_valid_kfam_annotations(resource) - - -def test_non_kfam_resource(): - resource = GenericNamespacedResource(metadata=ObjectMeta(name="test")) - - assert not has_valid_kfam_annotations(resource) - - @pytest.mark.parametrize( - "resource", + "resource,has_annotations", [ - GenericNamespacedResource(metadata=ObjectMeta(name="namespaceAdmin")), - GenericNamespacedResource(metadata=ObjectMeta(name="ns-owner-access-istio")), + ( + GenericNamespacedResource( + metadata=ObjectMeta(name="test", annotations={"role": "admin", "user": "test"}) + ), + True, + ), + ( + GenericNamespacedResource( + metadata=ObjectMeta(name="test", annotations={"role": "overlord", "user": "test"}) + ), + False, + ), + (GenericNamespacedResource(metadata=ObjectMeta(name="test")), False), ], ) -def test_profile_owner_resource(resource): - assert resource_is_for_profile_owner(resource) +def test_kfam_annotations(resource, has_annotations): + assert has_valid_kfam_annotations(resource) == has_annotations -def test_non_profile_owner_resource(): - resource = GenericNamespacedResource(metadata=ObjectMeta(name="random")) - - assert not resource_is_for_profile_owner(resource) +@pytest.mark.parametrize( + "resource,is_for_owner", + [ + (GenericNamespacedResource(metadata=ObjectMeta(name="namespaceAdmin")), True), + (GenericNamespacedResource(metadata=ObjectMeta(name="ns-owner-access-istio")), True), + (GenericNamespacedResource(metadata=ObjectMeta(name="random")), False), + ], +) +def test_profile_owner_resource(resource, is_for_owner): + assert resource_is_for_profile_owner(resource) == is_for_owner def test_contributor_getters(): @@ -64,49 +59,49 @@ def test_contributor_getters(): assert get_contributor_role(resource) == role -def test_rolebinding_not_matching_empty_profile_contributors(): - rb = RoleBinding( - metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), - roleRef=RoleRef(apiGroup="", kind="", name=""), - ) - - profile = Profile( - name="test", - owner=Owner(name="test", kind=UserKind.USER), - contributors=[], - resources={}, - ) - - assert not resource_matches_profile_contributor(rb, profile) - - -def test_rolebinding_not_matching_profile_contributors(): - rb = RoleBinding( - metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), - roleRef=RoleRef(apiGroup="", kind="", name=""), - ) - - profile = Profile( - name="test", - owner=Owner(name="test", kind=UserKind.USER), - contributors=[Contributor(name="test", role=ContributorRole.VIEW)], - resources={}, - ) - - assert not resource_matches_profile_contributor(rb, profile) - - -def test_rolebinding_matching_profile_contributors(): - rb = RoleBinding( - metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), - roleRef=RoleRef(apiGroup="", kind="", name=""), - ) - - profile = Profile( - name="test", - owner=Owner(name="test", kind=UserKind.USER), - contributors=[Contributor(name="test", role=ContributorRole.ADMIN)], - resources={}, - ) - - assert resource_matches_profile_contributor(rb, profile) +@pytest.mark.parametrize( + "rb,profile,matches_contributors", + [ + ( + RoleBinding( + metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), + roleRef=RoleRef(apiGroup="", kind="", name=""), + ), + Profile( + name="test", + owner=Owner(name="test", kind=UserKind.USER), + contributors=[], + resources={}, + ), + False, + ), + ( + RoleBinding( + metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), + roleRef=RoleRef(apiGroup="", kind="", name=""), + ), + Profile( + name="test", + owner=Owner(name="test", kind=UserKind.USER), + contributors=[Contributor(name="test", role=ContributorRole.VIEW)], + resources={}, + ), + False, + ), + ( + RoleBinding( + metadata=ObjectMeta(annotations={"user": "test", "role": "admin"}), + roleRef=RoleRef(apiGroup="", kind="", name=""), + ), + Profile( + name="test", + owner=Owner(name="test", kind=UserKind.USER), + contributors=[Contributor(name="test", role=ContributorRole.ADMIN)], + resources={}, + ), + True, + ), + ], +) +def test_rolebinding_not_matching_empty_profile_contributors(rb, profile, matches_contributors): + assert resource_matches_profile_contributor(rb, profile) == matches_contributors From c5142ee9f69fb619df685dca88bc46d4d305a12b Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Thu, 23 Jan 2025 16:41:04 +0000 Subject: [PATCH 22/23] review: Refactor profiles contributor check --- src/profiles_management/helpers/kfam.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index fca932d..bae6675 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -118,10 +118,6 @@ def resource_matches_profile_contributor( Returns: A boolean representing if the resources matches the expected contributor """ - if not profile.contributors or not has_valid_kfam_annotations(resource): - log.info("No profile contributors or kfam annotations were found in the resource.") - return False - role = get_contributor_role(resource) user = get_contributor_user(resource) if role in profile._contributors_dict.get(user, []): @@ -264,13 +260,16 @@ def delete_rolebindings_not_matching_profile_contributors( existing_rolebindings = list_contributor_rolebindings(client, profile.name) role_bindings_to_delete = [] - for rb in existing_rolebindings: - if not resource_matches_profile_contributor(rb, profile): - log.info( - "RoleBinding '%s' doesn't belong to Profile. Will delete it.", - k8s.get_name(rb), - ) - role_bindings_to_delete.append(rb) + if not profile.contributors: + role_bindings_to_delete = existing_rolebindings + else: + for rb in existing_rolebindings: + if not resource_matches_profile_contributor(rb, profile): + log.info( + "RoleBinding '%s' doesn't belong to Profile. Will delete it.", + k8s.get_name(rb), + ) + role_bindings_to_delete.append(rb) if role_bindings_to_delete: log.info("Deleting all resources that don't match the PMR.") From e9c058abd031985f1ee349f493dfed50f864bf23 Mon Sep 17 00:00:00 2001 From: Kimonas Sotirchos Date: Fri, 24 Jan 2025 08:52:39 +0000 Subject: [PATCH 23/23] review: Omit the extra annotations check --- src/profiles_management/helpers/kfam.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/profiles_management/helpers/kfam.py b/src/profiles_management/helpers/kfam.py index bae6675..e53fdb3 100644 --- a/src/profiles_management/helpers/kfam.py +++ b/src/profiles_management/helpers/kfam.py @@ -232,10 +232,9 @@ def kfam_resources_list_to_roles_dict( """ contributor_roles_dict = {} for resource in resources: - if has_valid_kfam_annotations(resource): - user = get_contributor_user(resource) - role = get_contributor_role(resource) - contributor_roles_dict[user] = contributor_roles_dict.get(user, []) + [role] + user = get_contributor_user(resource) + role = get_contributor_role(resource) + contributor_roles_dict[user] = contributor_roles_dict.get(user, []) + [role] return contributor_roles_dict