fix: Replaced ClusterRoles with local RoleBindings (#4625)

* replaced fetching ClusterRoles with local RoleBindings Signed-off-by: Daniele Martinoli <[email protected]> * added missing namespace configurations Signed-off-by: Daniele Martinoli <[email protected]> --------- Signed-off-by: Daniele Martinoli <[email protected]>
feast-dev · Oct 15, 2024 · ca9fb9b · ca9fb9b
1 parent c0a1026
commit ca9fb9b
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 88 deletions.
diff --git a/docs/getting-started/components/authz_manager.md b/docs/getting-started/components/authz_manager.md
@@ -114,4 +114,14 @@ auth:
 
 In case the client cannot run on the same cluster as the servers, the client token can be injected using the `LOCAL_K8S_TOKEN` 
 environment variable on the client side. The value must refer to the token of a service account created on the servers cluster
-and linked to the desired RBAC roles.
+and linked to the desired RBAC roles.
+
+#### Setting Up Kubernetes RBAC for Feast
+
+To ensure the Kubernetes RBAC environment aligns with Feast's RBAC configuration, follow these guidelines:
+* The roles defined in Feast `Permission` instances must have corresponding Kubernetes RBAC `Role` names.
+* The Kubernetes RBAC `Role` must reside in the same namespace as the Feast service.
+* The client application can run in a different namespace, using its own dedicated `ServiceAccount`.
+* Finally, the `RoleBinding` that links the client `ServiceAccount` to the RBAC `Role` must be defined in the namespace of the Feast service.
+
+If the above rules are satisfied, the Feast service must be  granted permissions to fetch `RoleBinding` instances from the local namespace.
diff --git a/examples/rbac-remote/server/k8s/server_resources.yaml b/examples/rbac-remote/server/k8s/server_resources.yaml
@@ -5,23 +5,25 @@ metadata:
   namespace: feast-dev
 ---
 apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRole
+kind: Role
 metadata:
-  name: feast-cluster-role
+  name: feast-role
+  namespace: feast-dev
 rules:
   - apiGroups: ["rbac.authorization.k8s.io"]
-    resources: ["roles", "rolebindings", "clusterrolebindings"]
+    resources: ["roles", "rolebindings"]
     verbs: ["get", "list", "watch"]
 ---
 apiVersion: rbac.authorization.k8s.io/v1
-kind: ClusterRoleBinding
+kind: RoleBinding
 metadata:
-  name: feast-cluster-rolebinding
+  name: feast-rolebinding
+  namespace: feast-dev
 subjects:
   - kind: ServiceAccount
     name: feast-sa
     namespace: feast-dev
 roleRef:
   apiGroup: rbac.authorization.k8s.io
-  kind: ClusterRole
-  name: feast-cluster-role
+  kind: Role
+  name: feast-role
diff --git a/sdk/python/feast/permissions/auth/kubernetes_token_parser.py b/sdk/python/feast/permissions/auth/kubernetes_token_parser.py
@@ -11,6 +11,7 @@
 from feast.permissions.user import User
 
 logger = logging.getLogger(__name__)
+_namespace_file_path = "/var/run/secrets/kubernetes.io/serviceaccount/namespace"
 
 
 class KubernetesTokenParser(TokenParser):
@@ -48,24 +49,42 @@ async def user_details_from_access_token(self, access_token: str) -> User:
         if sa_name is not None and sa_name == intra_communication_base64:
             return User(username=sa_name, roles=[])
         else:
-            roles = self.get_roles(sa_namespace, sa_name)
-            logger.info(f"Roles for ServiceAccount {sa_name}: {roles}")
+            current_namespace = self._read_namespace_from_file()
+            logger.info(
+                f"Looking for ServiceAccount roles of {sa_namespace}:{sa_name} in {current_namespace}"
+            )
+            roles = self.get_roles(
+                current_namespace=current_namespace,
+                service_account_namespace=sa_namespace,
+                service_account_name=sa_name,
+            )
+            logger.info(f"Roles: {roles}")
 
             return User(username=current_user, roles=roles)
 
-    def get_roles(self, namespace: str, service_account_name: str) -> list[str]:
+    def _read_namespace_from_file(self):
+        try:
+            with open(_namespace_file_path, "r") as file:
+                namespace = file.read().strip()
+            return namespace
+        except Exception as e:
+            raise e
+
+    def get_roles(
+        self,
+        current_namespace: str,
+        service_account_namespace: str,
+        service_account_name: str,
+    ) -> list[str]:
         """
-        Fetches the Kubernetes `Role`s associated to the given `ServiceAccount` in the given `namespace`.
+        Fetches the Kubernetes `Role`s associated to the given `ServiceAccount` in `current_namespace` namespace.
 
-        The research also includes the `ClusterRole`s, so the running deployment must be granted enough permissions to query
-        for such instances in all the namespaces.
+        The running deployment must be granted enough permissions to query for such instances in this namespace.
 
         Returns:
-            list[str]: Name of the `Role`s and `ClusterRole`s associated to the service account. No string manipulation is performed on the role name.
+            list[str]: Name of the `Role`s associated to the service account. No string manipulation is performed on the role name.
         """
-        role_bindings = self.rbac_v1.list_namespaced_role_binding(namespace)
-        cluster_role_bindings = self.rbac_v1.list_cluster_role_binding()
-
+        role_bindings = self.rbac_v1.list_namespaced_role_binding(current_namespace)
         roles: set[str] = set()
 
         for binding in role_bindings.items:
@@ -74,16 +93,7 @@ def get_roles(self, namespace: str, service_account_name: str) -> list[str]:
                     if (
                         subject.kind == "ServiceAccount"
                         and subject.name == service_account_name
-                    ):
-                        roles.add(binding.role_ref.name)
-
-        for binding in cluster_role_bindings.items:
-            if binding.subjects is not None:
-                for subject in binding.subjects:
-                    if (
-                        subject.kind == "ServiceAccount"
-                        and subject.name == service_account_name
-                        and subject.namespace == namespace
+                        and subject.namespace == service_account_namespace
                     ):
                         roles.add(binding.role_ref.name)
 

diff --git a/sdk/python/tests/unit/permissions/auth/conftest.py b/sdk/python/tests/unit/permissions/auth/conftest.py
@@ -20,46 +20,28 @@ def sa_name():
 
 
 @pytest.fixture
-def namespace():
+def my_namespace():
     return "my-ns"
 
 
 @pytest.fixture
-def rolebindings(sa_name, namespace) -> dict:
-    roles = ["reader", "writer"]
-    items = []
-    for r in roles:
-        items.append(
-            client.V1RoleBinding(
-                metadata=client.V1ObjectMeta(name=r, namespace=namespace),
-                subjects=[
-                    client.V1Subject(
-                        kind="ServiceAccount",
-                        name=sa_name,
-                        api_group="rbac.authorization.k8s.io",
-                    )
-                ],
-                role_ref=client.V1RoleRef(
-                    kind="Role", name=r, api_group="rbac.authorization.k8s.io"
-                ),
-            )
-        )
-    return {"items": client.V1RoleBindingList(items=items), "roles": roles}
+def sa_namespace():
+    return "sa-ns"
 
 
 @pytest.fixture
-def clusterrolebindings(sa_name, namespace) -> dict:
-    roles = ["updater"]
+def rolebindings(my_namespace, sa_name, sa_namespace) -> dict:
+    roles = ["reader", "writer"]
     items = []
     for r in roles:
         items.append(
-            client.V1ClusterRoleBinding(
-                metadata=client.V1ObjectMeta(name=r, namespace=namespace),
+            client.V1RoleBinding(
+                metadata=client.V1ObjectMeta(name=r, namespace=my_namespace),
                 subjects=[
                     client.V1Subject(
                         kind="ServiceAccount",
                         name=sa_name,
-                        namespace=namespace,
+                        namespace=sa_namespace,
                         api_group="rbac.authorization.k8s.io",
                     )
                 ],

diff --git a/sdk/python/tests/unit/permissions/auth/server/mock_utils.py b/sdk/python/tests/unit/permissions/auth/server/mock_utils.py
@@ -53,11 +53,11 @@ async def mock_oath2(self, request):
 
 
 def mock_kubernetes(request, monkeypatch):
+    my_namespace = request.getfixturevalue("my_namespace")
     sa_name = request.getfixturevalue("sa_name")
-    namespace = request.getfixturevalue("namespace")
-    subject = f"system:serviceaccount:{namespace}:{sa_name}"
+    sa_namespace = request.getfixturevalue("sa_namespace")
+    subject = f"system:serviceaccount:{sa_namespace}:{sa_name}"
     rolebindings = request.getfixturevalue("rolebindings")
-    clusterrolebindings = request.getfixturevalue("clusterrolebindings")
 
     monkeypatch.setattr(
         "feast.permissions.auth.kubernetes_token_parser.config.load_incluster_config",
@@ -71,11 +71,11 @@ def mock_kubernetes(request, monkeypatch):
         "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_namespaced_role_binding",
         lambda *args, **kwargs: rolebindings["items"],
     )
-    monkeypatch.setattr(
-        "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_cluster_role_binding",
-        lambda *args, **kwargs: clusterrolebindings["items"],
-    )
     monkeypatch.setattr(
         "feast.permissions.client.kubernetes_auth_client_manager.KubernetesAuthClientManager.get_token",
         lambda self: "my-token",
     )
+    monkeypatch.setattr(
+        "feast.permissions.auth.kubernetes_token_parser.KubernetesTokenParser._read_namespace_from_file",
+        lambda self: my_namespace,
+    )
diff --git a/sdk/python/tests/unit/permissions/auth/test_token_parser.py b/sdk/python/tests/unit/permissions/auth/test_token_parser.py
@@ -145,27 +145,26 @@ async def mock_oath2(self, request):
 @patch(
     "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_namespaced_role_binding"
 )
-@patch(
-    "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_cluster_role_binding"
-)
 def test_k8s_token_validation_success(
-    mock_crb,
     mock_rb,
     mock_jwt,
     mock_config,
     rolebindings,
-    clusterrolebindings,
+    monkeypatch,
+    my_namespace,
+    sa_name,
+    sa_namespace,
 ):
-    sa_name = "my-name"
-    namespace = "my-ns"
-    subject = f"system:serviceaccount:{namespace}:{sa_name}"
+    monkeypatch.setattr(
+        "feast.permissions.auth.kubernetes_token_parser.KubernetesTokenParser._read_namespace_from_file",
+        lambda self: my_namespace,
+    )
+    subject = f"system:serviceaccount:{sa_namespace}:{sa_name}"
     mock_jwt.return_value = {"sub": subject}
 
     mock_rb.return_value = rolebindings["items"]
-    mock_crb.return_value = clusterrolebindings["items"]
 
     roles = rolebindings["roles"]
-    croles = clusterrolebindings["roles"]
 
     access_token = "aaa-bbb-ccc"
     token_parser = KubernetesTokenParser()
@@ -175,12 +174,10 @@ def test_k8s_token_validation_success(
 
     assertpy.assert_that(user).is_type_of(User)
     if isinstance(user, User):
-        assertpy.assert_that(user.username).is_equal_to(f"{namespace}:{sa_name}")
-        assertpy.assert_that(user.roles.sort()).is_equal_to((roles + croles).sort())
+        assertpy.assert_that(user.username).is_equal_to(f"{sa_namespace}:{sa_name}")
+        assertpy.assert_that(user.roles.sort()).is_equal_to(roles.sort())
         for r in roles:
             assertpy.assert_that(user.has_matching_role([r])).is_true()
-        for cr in croles:
-            assertpy.assert_that(user.has_matching_role([cr])).is_true()
         assertpy.assert_that(user.has_matching_role(["foo"])).is_false()
 
 
@@ -212,30 +209,29 @@ def test_k8s_inter_server_comm(
     oidc_config,
     request,
     rolebindings,
-    clusterrolebindings,
     monkeypatch,
 ):
     if is_intra_server:
         subject = f":::{intra_communication_val}"
     else:
         sa_name = request.getfixturevalue("sa_name")
-        namespace = request.getfixturevalue("namespace")
-        subject = f"system:serviceaccount:{namespace}:{sa_name}"
+        sa_namespace = request.getfixturevalue("sa_namespace")
+        my_namespace = request.getfixturevalue("my_namespace")
+        subject = f"system:serviceaccount:{sa_namespace}:{sa_name}"
         rolebindings = request.getfixturevalue("rolebindings")
-        clusterrolebindings = request.getfixturevalue("clusterrolebindings")
 
         monkeypatch.setattr(
             "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_namespaced_role_binding",
             lambda *args, **kwargs: rolebindings["items"],
         )
-        monkeypatch.setattr(
-            "feast.permissions.auth.kubernetes_token_parser.client.RbacAuthorizationV1Api.list_cluster_role_binding",
-            lambda *args, **kwargs: clusterrolebindings["items"],
-        )
         monkeypatch.setattr(
             "feast.permissions.client.kubernetes_auth_client_manager.KubernetesAuthClientManager.get_token",
             lambda self: "my-token",
         )
+        monkeypatch.setattr(
+            "feast.permissions.auth.kubernetes_token_parser.KubernetesTokenParser._read_namespace_from_file",
+            lambda self: my_namespace,
+        )
 
     monkeypatch.setattr(
         "feast.permissions.auth.kubernetes_token_parser.config.load_incluster_config",
@@ -248,7 +244,6 @@ def test_k8s_inter_server_comm(
     )
 
     roles = rolebindings["roles"]
-    croles = clusterrolebindings["roles"]
 
     access_token = "aaa-bbb-ccc"
     token_parser = KubernetesTokenParser()
@@ -263,10 +258,8 @@ def test_k8s_inter_server_comm(
     else:
         assertpy.assert_that(user).is_type_of(User)
         if isinstance(user, User):
-            assertpy.assert_that(user.username).is_equal_to(f"{namespace}:{sa_name}")
-            assertpy.assert_that(user.roles.sort()).is_equal_to((roles + croles).sort())
+            assertpy.assert_that(user.username).is_equal_to(f"{sa_namespace}:{sa_name}")
+            assertpy.assert_that(user.roles.sort()).is_equal_to(roles.sort())
             for r in roles:
                 assertpy.assert_that(user.has_matching_role([r])).is_true()
-            for cr in croles:
-                assertpy.assert_that(user.has_matching_role([cr])).is_true()
             assertpy.assert_that(user.has_matching_role(["foo"])).is_false()