diff --git a/charts/kagenti-operator/templates/rbac/role.yaml b/charts/kagenti-operator/templates/rbac/role.yaml index 22603c34..16f4c4a5 100755 --- a/charts/kagenti-operator/templates/rbac/role.yaml +++ b/charts/kagenti-operator/templates/rbac/role.yaml @@ -140,6 +140,15 @@ rules: - get - list - watch +- apiGroups: + - mlflow.opendatahub.io + resources: + - mlflowexperiments + verbs: + - get + - list + - watch + - update - apiGroups: - networking.k8s.io resources: @@ -164,6 +173,7 @@ rules: - apiGroups: - rbac.authorization.k8s.io resources: + - roles - rolebindings verbs: - create diff --git a/kagenti-operator/config/rbac/role.yaml b/kagenti-operator/config/rbac/role.yaml index e5bdc840..69056fe6 100644 --- a/kagenti-operator/config/rbac/role.yaml +++ b/kagenti-operator/config/rbac/role.yaml @@ -129,6 +129,15 @@ rules: - get - list - watch +- apiGroups: + - mlflow.opendatahub.io + resources: + - mlflowexperiments + verbs: + - get + - list + - watch + - update - apiGroups: - networking.k8s.io resources: @@ -154,6 +163,7 @@ rules: - rbac.authorization.k8s.io resources: - rolebindings + - roles verbs: - create - get diff --git a/kagenti-operator/docs/mlflow-integration.md b/kagenti-operator/docs/mlflow-integration.md index 37136cb2..6e1946eb 100644 --- a/kagenti-operator/docs/mlflow-integration.md +++ b/kagenti-operator/docs/mlflow-integration.md @@ -80,15 +80,19 @@ The following annotations are set on the Deployment's PodTemplateSpec: | `mlflow.kagenti.io/tracking-uri` | MLflow tracking URI | | `mlflow.kagenti.io/tracking-auth` | `kubernetes-namespaced` | -## Authentication +## Authentication & RBAC -The MLflow controller uses Kubernetes namespace-scoped authentication: +The MLflow controller uses Kubernetes namespace-scoped authentication with least-privilege agent access: -1. The controller's own ServiceAccount token is used to call the MLflow REST API to create experiments. The `X-MLFLOW-WORKSPACE` header is set to the agent's namespace, scoping the experiment to that workspace. +1. **Operator access**: The controller-manager's ServiceAccount is bound (via a ClusterRoleBinding shipped in the Helm chart / kustomize) to the broad `mlflow-operator-mlflow-integration` ClusterRole. This lets the operator call the MLflow REST API to create experiments. The `X-MLFLOW-WORKSPACE` header is set to the agent's namespace, scoping the experiment to that workspace. -2. For agent-side access, the controller creates a **RoleBinding** named `kagenti-mlflow-` in the agent's namespace. This binds the agent's ServiceAccount to the `mlflow-operator-mlflow-integration` ClusterRole (created by the RHOAI MLflow operator). The agent authenticates to MLflow using its projected SA token at `/var/run/secrets/kubernetes.io/serviceaccount/token`. +2. **Agent access (scoped)**: For each agent Deployment the controller creates: + - A **Role** named `kagenti-mlflow-` with `get` and `update` on the `mlflowexperiments` resource, scoped to the agent's own experiment via `resourceNames`. This means agents **cannot** create/delete experiments or access `registeredmodels` or `gatewayendpoints`. + - A **RoleBinding** with the same name that binds the agent's ServiceAccount to the scoped Role. -3. The RoleBinding is owned by the Deployment — deleting the Deployment garbage-collects the RoleBinding automatically. + The agent authenticates to MLflow using its projected SA token at `/var/run/secrets/kubernetes.io/serviceaccount/token`. + +3. Both the Role and RoleBinding are owned by the Deployment — deleting the Deployment garbage-collects them automatically. ## Verification diff --git a/kagenti-operator/internal/controller/mlflow_controller.go b/kagenti-operator/internal/controller/mlflow_controller.go index 33658f06..007cf18b 100644 --- a/kagenti-operator/internal/controller/mlflow_controller.go +++ b/kagenti-operator/internal/controller/mlflow_controller.go @@ -40,9 +40,13 @@ import ( ) const ( - // DefaultMLflowClusterRole is the ClusterRole managed by the MLflow operator - // for agent access to MLflow resources (RHOAI 3.4+). - DefaultMLflowClusterRole = "mlflow-operator-mlflow-integration" + // MLflowExperimentsAPIGroup is the API group used by the MLflow Kubernetes + // authorization plugin for SubjectAccessReview checks. + MLflowExperimentsAPIGroup = "mlflow.opendatahub.io" + + // MLflowExperimentsResource is the virtual resource the MLflow gateway checks + // when authorizing experiment-level operations. + MLflowExperimentsResource = "mlflowexperiments" // MLflow annotation keys stored on the PodTemplateSpec. AnnotationMLflowExperimentID = "mlflow.kagenti.io/experiment-id" @@ -53,15 +57,16 @@ const ( // MLflowReconciler reconciles Deployments labelled kagenti.io/type=agent. // It auto-discovers MLflow availability via the mlflows.mlflow.opendatahub.io CRD. +// +// For each agent Deployment the reconciler creates a scoped Role granting only +// get+update on the agent's specific experiment (via resourceNames), and a +// RoleBinding for the agent SA. This ensures agents cannot access other +// experiments, registered models, or gateway endpoints. type MLflowReconciler struct { client.Client Scheme *runtime.Scheme Recorder record.EventRecorder - // MLflowClusterRole is the ClusterRole to bind agent SAs to. - // Defaults to DefaultMLflowClusterRole if empty. - MLflowClusterRole string - // NewMLflowClient creates an MLflow client for the given base URL. // If nil, a default client is used. NewMLflowClient func(baseURL string) *mlflow.Client @@ -72,6 +77,8 @@ type MLflowReconciler struct { } // +kubebuilder:rbac:groups=mlflow.opendatahub.io,resources=mlflows,verbs=get;list;watch +// +kubebuilder:rbac:groups=mlflow.opendatahub.io,resources=mlflowexperiments,verbs=get;list;watch;update +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=create;get;list;watch;update // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=create;get;list;watch;update // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;update;patch @@ -129,8 +136,8 @@ func (r *MLflowReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr logger.Info("deployment has no explicit serviceAccountName, falling back to 'default'", "deployment", dep.Name) } - if err := r.ensureRoleBinding(ctx, dep, saName); err != nil { - logger.Error(err, "Failed to ensure MLflow RoleBinding") + if err := r.ensureScopedRBAC(ctx, dep, saName, experimentName); err != nil { + logger.Error(err, "Failed to ensure scoped MLflow RBAC") return ctrl.Result{}, err } @@ -148,13 +155,6 @@ func (r *MLflowReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } -func (r *MLflowReconciler) clusterRoleName() string { - if r.MLflowClusterRole != "" { - return r.MLflowClusterRole - } - return DefaultMLflowClusterRole -} - func (r *MLflowReconciler) mlflowClient(baseURL string) *mlflow.Client { if r.NewMLflowClient != nil { return r.NewMLflowClient(baseURL) @@ -246,29 +246,55 @@ func (r *MLflowReconciler) configureDeployment(ctx context.Context, dep *appsv1. }) } -// ensureRoleBinding creates or updates the RoleBinding for the agent SA. -func (r *MLflowReconciler) ensureRoleBinding(ctx context.Context, dep *appsv1.Deployment, saName string) error { - rbName := fmt.Sprintf("kagenti-mlflow-%s", dep.Name) - rb := &rbacv1.RoleBinding{ +// ensureScopedRBAC creates a Role scoped to a single experiment (by resourceName) +// and a RoleBinding that grants the agent SA only get+update on that experiment. +// Both resources are owned by the Deployment so they are garbage-collected on deletion. +func (r *MLflowReconciler) ensureScopedRBAC(ctx context.Context, dep *appsv1.Deployment, saName, experimentName string) error { + roleName := fmt.Sprintf("kagenti-mlflow-%s", dep.Name) + + role := &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ - Name: rbName, + Name: roleName, Namespace: dep.Namespace, }, } + if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, role, func() error { + role.Labels = map[string]string{ + LabelManagedBy: LabelManagedByValue, + } + if err := controllerutil.SetOwnerReference(dep, role, r.Scheme); err != nil { + return err + } + role.Rules = []rbacv1.PolicyRule{ + { + APIGroups: []string{MLflowExperimentsAPIGroup}, + Resources: []string{MLflowExperimentsResource}, + ResourceNames: []string{experimentName}, + Verbs: []string{"get", "update"}, + }, + } + return nil + }); err != nil { + return fmt.Errorf("ensuring scoped Role: %w", err) + } - _, err := controllerutil.CreateOrUpdate(ctx, r.Client, rb, func() error { + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleName, + Namespace: dep.Namespace, + }, + } + if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, rb, func() error { rb.Labels = map[string]string{ LabelManagedBy: LabelManagedByValue, } - if err := controllerutil.SetOwnerReference(dep, rb, r.Scheme); err != nil { return err } - rb.RoleRef = rbacv1.RoleRef{ APIGroup: rbacv1.GroupName, - Kind: "ClusterRole", - Name: r.clusterRoleName(), + Kind: "Role", + Name: roleName, } rb.Subjects = []rbacv1.Subject{ { @@ -278,8 +304,11 @@ func (r *MLflowReconciler) ensureRoleBinding(ctx context.Context, dep *appsv1.De }, } return nil - }) - return err + }); err != nil { + return fmt.Errorf("ensuring scoped RoleBinding: %w", err) + } + + return nil } // setEnvVar sets an env var on a container, returning true if a change was made. @@ -302,6 +331,7 @@ func setEnvVar(container *corev1.Container, name, value string) bool { func (r *MLflowReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&appsv1.Deployment{}, builder.WithPredicates(agentLabelPredicate())). + Owns(&rbacv1.Role{}). Owns(&rbacv1.RoleBinding{}). Named("mlflow"). Complete(r) diff --git a/kagenti-operator/internal/controller/mlflow_controller_test.go b/kagenti-operator/internal/controller/mlflow_controller_test.go index 44372cff..3bf00687 100644 --- a/kagenti-operator/internal/controller/mlflow_controller_test.go +++ b/kagenti-operator/internal/controller/mlflow_controller_test.go @@ -228,7 +228,7 @@ var _ = Describe("MLflow Controller", func() { cleanup() }) - It("should create RoleBinding and inject env vars", func() { + It("should create scoped Role, RoleBinding, and inject env vars", func() { dep := newAgentDeployment("mlflow-full", namespace) Expect(k8sClient.Create(ctx, dep)).To(Succeed()) defer func() { _ = k8sClient.Delete(ctx, dep) }() @@ -236,13 +236,24 @@ var _ = Describe("MLflow Controller", func() { r := newReconcilerWithServer(server.URL, tokenPath) reconcileAndExpectNoOp(r, "mlflow-full", namespace) + role := &rbacv1.Role{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: "kagenti-mlflow-mlflow-full", Namespace: namespace, + }, role)).To(Succeed()) + Expect(role.Rules).To(HaveLen(1)) + Expect(role.Rules[0].APIGroups).To(Equal([]string{MLflowExperimentsAPIGroup})) + Expect(role.Rules[0].Resources).To(Equal([]string{MLflowExperimentsResource})) + Expect(role.Rules[0].ResourceNames).To(Equal([]string{"mlflow-full"})) + Expect(role.Rules[0].Verbs).To(Equal([]string{"get", "update"})) + rb := &rbacv1.RoleBinding{} Expect(k8sClient.Get(ctx, types.NamespacedName{ Name: "kagenti-mlflow-mlflow-full", Namespace: namespace, }, rb)).To(Succeed()) Expect(rb.Subjects).To(HaveLen(1)) Expect(rb.Subjects[0].Name).To(Equal("test-sa")) - Expect(rb.RoleRef.Name).To(Equal(DefaultMLflowClusterRole)) + Expect(rb.RoleRef.Kind).To(Equal("Role")) + Expect(rb.RoleRef.Name).To(Equal("kagenti-mlflow-mlflow-full")) updated := &appsv1.Deployment{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "mlflow-full", Namespace: namespace}, updated)).To(Succeed()) @@ -270,11 +281,18 @@ var _ = Describe("MLflow Controller", func() { r := newReconcilerWithServer(server.URL, tokenPath) reconcileAndExpectNoOp(r, "mlflow-default-sa", namespace) + role := &rbacv1.Role{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: "kagenti-mlflow-mlflow-default-sa", Namespace: namespace, + }, role)).To(Succeed()) + Expect(role.Rules[0].ResourceNames).To(Equal([]string{"mlflow-default-sa"})) + rb := &rbacv1.RoleBinding{} Expect(k8sClient.Get(ctx, types.NamespacedName{ Name: "kagenti-mlflow-mlflow-default-sa", Namespace: namespace, }, rb)).To(Succeed()) Expect(rb.Subjects[0].Name).To(Equal("default")) + Expect(rb.RoleRef.Kind).To(Equal("Role")) }) }) @@ -327,18 +345,6 @@ var _ = Describe("MLflow Controller", func() { }) var _ = Describe("MLflow Controller helpers", func() { - Describe("clusterRoleName", func() { - It("should return the default when MLflowClusterRole is empty", func() { - r := &MLflowReconciler{} - Expect(r.clusterRoleName()).To(Equal(DefaultMLflowClusterRole)) - }) - - It("should return the custom value when set", func() { - r := &MLflowReconciler{MLflowClusterRole: "custom-role"} - Expect(r.clusterRoleName()).To(Equal("custom-role")) - }) - }) - Describe("setEnvVar", func() { It("should add a new env var", func() { container := &corev1.Container{Name: "test"}