Skip to content

Commit c756c98

Browse files
authored
refactor(evalhub): remove control-plane jobs SA (tenant-only) (#657)
- Remove control-plane jobs ServiceAccount creation from the reconcile loop - Remove associated RBAC methods and constants (createJobsAPIAccessRole, createJobsAPIAccessRoleBinding, generateJobsAPIAccessRoleName, mlflowJobsAccessClusterRoleName) - Remove the three call sites in createServiceAccount() that wired up jobs-specific Roles, RoleBindings, and MLFlow bindings - Remove corresponding unit and integration tests for the deleted code path
1 parent 2006c90 commit c756c98

File tree

4 files changed

+5
-394
lines changed

4 files changed

+5
-394
lines changed

controllers/evalhub/evalhub_controller.go

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,6 @@ func (r *EvalHubReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
115115
return RequeueWithError(err)
116116
}
117117

118-
// Create ServiceAccount for jobs
119-
err = r.createJobsServiceAccount(ctx, instance)
120-
if err != nil {
121-
log.Error(err, "Failed to create Jobs ServiceAccount")
122-
instance.SetStatus("Ready", "Error", fmt.Sprintf("Failed to create Jobs ServiceAccount: %v", err), corev1.ConditionFalse)
123-
r.Status().Update(ctx, instance)
124-
return RequeueWithError(err)
125-
}
126-
127118
// Reconcile tenant namespace RBAC (non-fatal)
128119
if err := r.reconcileTenantNamespaces(ctx, instance); err != nil {
129120
log.Error(err, "Failed to reconcile tenant namespaces")

controllers/evalhub/proxy_rbac_test.go

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -198,33 +198,6 @@ var _ = Describe("EvalHub API RBAC", func() {
198198
Expect(roleBinding.RoleRef.Name).To(Equal(generateAPIAccessRoleName(evalHub)))
199199
})
200200

201-
It("should create namespace-scoped jobs API access RoleBinding referencing per-instance Role", func() {
202-
By("Creating service account")
203-
err := reconciler.createServiceAccount(ctx, evalHub)
204-
Expect(err).NotTo(HaveOccurred())
205-
206-
By("Verifying jobs API access RoleBinding exists in namespace")
207-
roleBinding := &rbacv1.RoleBinding{}
208-
rbName := evalHubName + "-jobs-api-access-rb"
209-
err = k8sClient.Get(ctx, types.NamespacedName{
210-
Name: rbName,
211-
Namespace: testNamespace,
212-
}, roleBinding)
213-
Expect(err).NotTo(HaveOccurred())
214-
215-
By("Checking RoleBinding is namespace-scoped")
216-
Expect(roleBinding.Namespace).To(Equal(testNamespace))
217-
218-
By("Checking subjects use -jobs SA")
219-
Expect(roleBinding.Subjects).To(HaveLen(1))
220-
Expect(roleBinding.Subjects[0].Kind).To(Equal("ServiceAccount"))
221-
Expect(roleBinding.Subjects[0].Name).To(Equal(evalHubName + "-jobs"))
222-
223-
By("Checking role reference points to per-instance Role (not ClusterRole)")
224-
Expect(roleBinding.RoleRef.Kind).To(Equal("Role"))
225-
Expect(roleBinding.RoleRef.Name).To(Equal(generateJobsAPIAccessRoleName(evalHub)))
226-
})
227-
228201
It("should handle existing service account gracefully", func() {
229202
By("Creating service account initially")
230203
err := reconciler.createServiceAccount(ctx, evalHub)

controllers/evalhub/service_accounts.go

Lines changed: 2 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,6 @@ func generateAPIAccessRoleName(instance *evalhubv1alpha1.EvalHub) string {
109109
return instance.Name + "-api-access-role"
110110
}
111111

112-
// generateJobsAPIAccessRoleName returns the name for the per-instance jobs API access Role.
113-
func generateJobsAPIAccessRoleName(instance *evalhubv1alpha1.EvalHub) string {
114-
return instance.Name + "-jobs-api-access-role"
115-
}
116-
117112
func generateAuthReviewerClusterRoleBindingName(instance *evalhubv1alpha1.EvalHub) string {
118113
return instance.Name + "-" + instance.Namespace + "-auth-reviewer-crb"
119114
}
@@ -172,11 +167,6 @@ func (r *EvalHubReconciler) createServiceAccount(ctx context.Context, instance *
172167
return err
173168
}
174169

175-
err = r.createJobsAPIAccessRole(ctx, instance)
176-
if err != nil {
177-
return err
178-
}
179-
180170
// Create namespace-scoped RoleBinding for EvalHub API access (evalhubs/proxy)
181171
err = r.createAPIAccessRoleBinding(ctx, instance, serviceAccountName)
182172
if err != nil {
@@ -194,25 +184,14 @@ func (r *EvalHubReconciler) createServiceAccount(ctx context.Context, instance *
194184
return err
195185
}
196186

197-
// Create RoleBinding for jobs ServiceAccount to access evalhubs/proxy in this namespace
198-
jobsServiceAccountName := generateJobsServiceAccountName(instance)
199-
err = r.createJobsAPIAccessRoleBinding(ctx, instance, jobsServiceAccountName)
200-
if err != nil {
201-
return err
202-
}
203-
204-
// Create MLFlow access RoleBindings for both ServiceAccounts.
187+
// Create MLFlow access RoleBinding for the API ServiceAccount.
205188
// MLFlow's kubernetes-auth plugin validates tokens via SubjectAccessReview against
206189
// the workspace namespace. The custom "evalhub-mlflow-access" ClusterRole provides
207-
// the required mlflow.kubeflow.org permissions for both the api and jobs SAs.
190+
// the required mlflow.kubeflow.org permissions for the API SA.
208191
err = r.createMLFlowAccessRoleBinding(ctx, instance, serviceAccountName, "api", mlflowAccessClusterRoleName)
209192
if err != nil {
210193
return err
211194
}
212-
err = r.createMLFlowAccessRoleBinding(ctx, instance, jobsServiceAccountName, "jobs", mlflowJobsAccessClusterRoleName)
213-
if err != nil {
214-
return err
215-
}
216195

217196
return nil
218197
}
@@ -235,10 +214,6 @@ const (
235214
// config/rbac/evalhub_mlflow_jobs_role.yaml).
236215
const mlflowAccessClusterRoleName = "trustyai-service-operator-evalhub-mlflow-access"
237216

238-
// mlflowJobsAccessClusterRoleName is a restricted MLflow ClusterRole for job pods.
239-
// Jobs only need create, get, list -- not update or delete.
240-
const mlflowJobsAccessClusterRoleName = "trustyai-service-operator-evalhub-mlflow-jobs-access"
241-
242217
// createAPIAccessRole creates a per-instance namespaced Role with resourceNames
243218
// scoped to this specific EvalHub instance. This ensures the SA can only access
244219
// its own instance's evalhubs/proxy subresource.
@@ -297,56 +272,6 @@ func (r *EvalHubReconciler) createAPIAccessRole(ctx context.Context, instance *e
297272
return nil
298273
}
299274

300-
// createJobsAPIAccessRole creates a per-instance namespaced Role for the jobs SA
301-
// with resourceNames scoped to this specific EvalHub instance.
302-
func (r *EvalHubReconciler) createJobsAPIAccessRole(ctx context.Context, instance *evalhubv1alpha1.EvalHub) error {
303-
log := log.FromContext(ctx)
304-
305-
roleName := generateJobsAPIAccessRoleName(instance)
306-
role := &rbacv1.Role{
307-
ObjectMeta: metav1.ObjectMeta{
308-
Name: roleName,
309-
Namespace: instance.Namespace,
310-
Labels: map[string]string{
311-
"app": "eval-hub",
312-
"app.kubernetes.io/name": roleName,
313-
"app.kubernetes.io/instance": instance.Name,
314-
"app.kubernetes.io/part-of": "eval-hub",
315-
"app.kubernetes.io/version": constants.Version,
316-
},
317-
},
318-
Rules: []rbacv1.PolicyRule{
319-
{
320-
APIGroups: []string{"trustyai.opendatahub.io"},
321-
Resources: []string{"evalhubs/proxy"},
322-
ResourceNames: []string{instance.Name},
323-
Verbs: []string{"get", "create"},
324-
},
325-
},
326-
}
327-
328-
if err := ctrl.SetControllerReference(instance, role, r.Scheme); err != nil {
329-
return err
330-
}
331-
332-
found := &rbacv1.Role{}
333-
err := r.Get(ctx, types.NamespacedName{Name: role.Name, Namespace: role.Namespace}, found)
334-
if err != nil && errors.IsNotFound(err) {
335-
log.Info("Creating jobs API access Role", "Namespace", role.Namespace, "Name", role.Name)
336-
return r.Create(ctx, role)
337-
} else if err != nil {
338-
return err
339-
}
340-
341-
if !equalPolicyRules(found.Rules, role.Rules) {
342-
found.Rules = role.Rules
343-
log.Info("Updating jobs API access Role rules", "Name", role.Name)
344-
return r.Update(ctx, found)
345-
}
346-
347-
return nil
348-
}
349-
350275
// createMLFlowAccessRoleBinding creates a RoleBinding for a ServiceAccount to
351276
// the specified MLflow ClusterRole in the instance namespace. This allows the
352277
// ServiceAccount to pass MLFlow's kubernetes-auth SubjectAccessReview checks
@@ -496,19 +421,6 @@ func (r *EvalHubReconciler) createAPIAccessRoleBinding(ctx context.Context, inst
496421
})
497422
}
498423

499-
// createJobsAPIAccessRoleBinding creates a namespace-scoped RoleBinding for the jobs
500-
// ServiceAccount to the per-instance Role for evalhubs/proxy access.
501-
func (r *EvalHubReconciler) createJobsAPIAccessRoleBinding(ctx context.Context, instance *evalhubv1alpha1.EvalHub, serviceAccountName string) error {
502-
roleBindingName := instance.Name + "-jobs-api-access-rb"
503-
roleName := generateJobsAPIAccessRoleName(instance)
504-
505-
return r.createGenericRoleBinding(ctx, instance, roleBindingName, serviceAccountName, rbacv1.RoleRef{
506-
Kind: "Role",
507-
Name: roleName,
508-
APIGroup: rbacv1.GroupName,
509-
})
510-
}
511-
512424
// createJobsWriterRoleBinding creates a RoleBinding for the API SA to the
513425
// jobs-writer ClusterRole (batch/jobs create,delete).
514426
func (r *EvalHubReconciler) createJobsWriterRoleBinding(ctx context.Context, instance *evalhubv1alpha1.EvalHub, serviceAccountName string) error {
@@ -715,46 +627,6 @@ func generateJobsServiceAccountName(instance *evalhubv1alpha1.EvalHub) string {
715627
return instance.Name + "-jobs"
716628
}
717629

718-
// createJobsServiceAccount creates a service account for jobs created by this EvalHub instance
719-
func (r *EvalHubReconciler) createJobsServiceAccount(ctx context.Context, instance *evalhubv1alpha1.EvalHub) error {
720-
serviceAccountName := generateJobsServiceAccountName(instance)
721-
722-
sa := &corev1.ServiceAccount{
723-
ObjectMeta: metav1.ObjectMeta{
724-
Name: serviceAccountName,
725-
Namespace: instance.Namespace,
726-
Labels: map[string]string{
727-
"app": "eval-hub",
728-
"app.kubernetes.io/name": normalizeDNS1123LabelValue(serviceAccountName),
729-
"app.kubernetes.io/instance": instance.Name,
730-
"app.kubernetes.io/part-of": "eval-hub",
731-
"app.kubernetes.io/component": "jobs",
732-
"app.kubernetes.io/version": constants.Version,
733-
},
734-
},
735-
}
736-
737-
// Set instance as the owner and controller
738-
if err := ctrl.SetControllerReference(instance, sa, r.Scheme); err != nil {
739-
return err
740-
}
741-
742-
// Check if this ServiceAccount already exists
743-
found := &corev1.ServiceAccount{}
744-
err := r.Get(ctx, types.NamespacedName{Name: sa.Name, Namespace: sa.Namespace}, found)
745-
if err != nil && errors.IsNotFound(err) {
746-
log.FromContext(ctx).Info("Creating a new Jobs ServiceAccount", "Namespace", sa.Namespace, "Name", sa.Name)
747-
err = r.Create(ctx, sa)
748-
if err != nil {
749-
return err
750-
}
751-
} else if err != nil {
752-
return err
753-
}
754-
755-
return nil
756-
}
757-
758630
// reconcileTenantNamespaces discovers namespaces with the tenant annotation and
759631
// provisions per-tenant RBAC (SA + RoleBindings) so the API SA can create jobs
760632
// in tenant namespaces. It also cleans up resources in namespaces that lost the

0 commit comments

Comments
 (0)