(fix)Add custom SA for lls by default (#115)

VaishnaviHire · web-flow · commit 5484f16804cc · 2025-07-18T18:43:06.000Z
- Add custom SA to allow root permissions to init container.
- This ensure we deploy lls without errors in restricted namespaces


Approved-by: leseb

Approved-by: rhdedgar

Approved-by: mfleader
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -27,6 +27,7 @@ rules:
 - apiGroups:
   - ""
   resources:
+  - serviceaccounts
   - services
   verbs:
   - create
@@ -86,3 +87,37 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - clusterrolebindings
+  verbs:
+  - create
+  - delete
+  - get
+  - list
+  - patch
+  - update
+  - watch
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - clusterroles
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - security.openshift.io
+  resources:
+  - securitycontextconstraints
+  verbs:
+  - use
+- apiGroups:
+  - security.openshift.io
+  resourceNames:
+  - anyuid
+  resources:
+  - securitycontextconstraints
+  verbs:
+  - use
diff --git a/controllers/kubebuilder_rbac.go b/controllers/kubebuilder_rbac.go
@@ -11,7 +11,15 @@ package controllers
 // Service permissions - controller creates and manages services
 //+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch;delete
 
-// PVC permissions - controller creates PVCs (immutable after creation, no update/patch needed)
+// ServiceAccount permissions - controller creates and manages service accounts for PVC permissions
+//+kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=get;list;watch;create;update;patch;delete
+
+//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles,verbs=get;list;watch
+
+//+kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,verbs=use
+//+kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,resourceNames=anyuid,verbs=use
+
 //+kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;create
 
 // ConfigMap permissions - controller reads user configmaps and manages operator config configmaps
diff --git a/controllers/llamastackdistribution_controller.go b/controllers/llamastackdistribution_controller.go
@@ -230,6 +230,11 @@ func (r *LlamaStackDistributionReconciler) reconcileResources(ctx context.Contex
 		return fmt.Errorf("failed to reconcile NetworkPolicy: %w", err)
 	}
 
+	// Reconcile manifest-based resources
+	if err := r.reconcileManifestResources(ctx, instance); err != nil {
+		return err
+	}
+
 	// Reconcile the Deployment
 	if err := r.reconcileDeployment(ctx, instance); err != nil {
 		return fmt.Errorf("failed to reconcile Deployment: %w", err)
@@ -242,11 +247,6 @@ func (r *LlamaStackDistributionReconciler) reconcileResources(ctx context.Contex
 		}
 	}
 
-	// Reconcile manifest-based resources
-	if err := r.reconcileManifestResources(ctx, instance); err != nil {
-		return err
-	}
-
 	return nil
 }
 
diff --git a/controllers/manifests/base/kustomization.yaml b/controllers/manifests/base/kustomization.yaml
@@ -3,6 +3,8 @@ kind: Kustomization
 
 resources:
 - pvc.yaml
+- serviceaccount.yaml
+- scc-binding.yaml
 
 labels:
 - includeSelectors: true
diff --git a/controllers/manifests/base/scc-binding.yaml b/controllers/manifests/base/scc-binding.yaml
@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: crb
+subjects:
+- kind: ServiceAccount
+  name: sa
+  namespace: default
+roleRef:
+  kind: ClusterRole
+  name: system:openshift:scc:anyuid
+  apiGroup: rbac.authorization.k8s.io
diff --git a/controllers/manifests/base/serviceaccount.yaml b/controllers/manifests/base/serviceaccount.yaml
@@ -0,0 +1,10 @@
+# This service account is used to grant privileges to the llama-stack init container
+# to update PVC permissions.
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: sa
+  annotations:
+    # This annotation is used by OpenShift to assign the anyuid SCC
+    # which allows the container to run as any user ID
+    openshift.io/scc: anyuid
diff --git a/controllers/resource_helper.go b/controllers/resource_helper.go
@@ -170,12 +170,15 @@ func configurePodStorage(instance *llamav1alpha1.LlamaStackDistribution, contain
 
 // configurePodOverrides applies pod-level overrides from the LlamaStackDistribution spec.
 func configurePodOverrides(instance *llamav1alpha1.LlamaStackDistribution, podSpec *corev1.PodSpec) {
-	if instance.Spec.Server.PodOverrides != nil {
-		// Set ServiceAccount name if specified
-		if instance.Spec.Server.PodOverrides.ServiceAccountName != "" {
-			podSpec.ServiceAccountName = instance.Spec.Server.PodOverrides.ServiceAccountName
-		}
+	// Set ServiceAccount name - use override if specified, otherwise use default
+	if instance.Spec.Server.PodOverrides != nil && instance.Spec.Server.PodOverrides.ServiceAccountName != "" {
+		podSpec.ServiceAccountName = instance.Spec.Server.PodOverrides.ServiceAccountName
+	} else {
+		podSpec.ServiceAccountName = instance.Name + "-sa"
+	}
 
+	// Apply other pod overrides if specified
+	if instance.Spec.Server.PodOverrides != nil {
 		// Add volumes if specified
 		if len(instance.Spec.Server.PodOverrides.Volumes) > 0 {
 			podSpec.Volumes = append(podSpec.Volumes, instance.Spec.Server.PodOverrides.Volumes...)
diff --git a/controllers/resource_helper_test.go b/controllers/resource_helper_test.go
@@ -561,8 +561,8 @@ func TestPodOverridesWithoutServiceAccount(t *testing.T) {
 	// Apply pod overrides
 	configurePodOverrides(instance, &deployment.Spec.Template.Spec)
 
-	// Verify ServiceAccount name is empty
-	if deployment.Spec.Template.Spec.ServiceAccountName != "" {
-		t.Errorf("expected empty ServiceAccountName, got %s", deployment.Spec.Template.Spec.ServiceAccountName)
+	// Verify ServiceAccount name is empty (default ServiceAccountName should be set when not explicitly provided)
+	if deployment.Spec.Template.Spec.ServiceAccountName != instance.Name+"-sa" {
+		t.Errorf("expected default ServiceAccountName when not explicitly provided, got %s", deployment.Spec.Template.Spec.ServiceAccountName)
 	}
 }
diff --git a/pkg/deploy/kustomizer.go b/pkg/deploy/kustomizer.go
@@ -86,6 +86,17 @@ func manageResource(
 		return fmt.Errorf("failed to unmarshal resource: %w", err)
 	}
 
+	// Check if ClusterRoleBinding references a ClusterRole that exists
+	if u.GetKind() == "ClusterRoleBinding" {
+		if shouldSkip, err := CheckClusterRoleExists(ctx, cli, u); err != nil {
+			return fmt.Errorf("failed to check ClusterRole existence: %w", err)
+		} else if shouldSkip {
+			log.FromContext(ctx).V(1).Info("Skipping ClusterRoleBinding - referenced ClusterRole not found",
+				"clusterRoleBinding", u.GetName())
+			return nil
+		}
+	}
+
 	kGvk := res.GetGvk()
 	gvk := schema.GroupVersionKind{
 		Group:   kGvk.Group,
@@ -204,6 +215,18 @@ func applyPlugins(resMap *resmap.ResMap, ownerInstance *llamav1alpha1.LlamaStack
 				TargetKind:        "PersistentVolumeClaim",
 				CreateIfNotExists: true,
 			},
+			{
+				SourceValue:       ownerInstance.GetNamespace(),
+				TargetField:       "subjects[0].namespace",
+				TargetKind:        "ClusterRoleBinding",
+				CreateIfNotExists: true,
+			},
+			{
+				SourceValue:       ownerInstance.GetName() + "-sa",
+				TargetField:       "subjects[0].name",
+				TargetKind:        "ClusterRoleBinding",
+				CreateIfNotExists: true,
+			},
 		},
 	})
 	if err := fieldTransformerPlugin.Transform(*resMap); err != nil {
@@ -233,3 +256,33 @@ func FilterExcludeKinds(resMap *resmap.ResMap, kindsToExclude []string) (*resmap
 	}
 	return &filteredResMap, nil
 }
+
+// CheckClusterRoleExists checks if a ClusterRoleBinding should be skipped due to missing ClusterRole.
+func CheckClusterRoleExists(ctx context.Context, cli client.Client, crb *unstructured.Unstructured) (bool, error) {
+	roleRef, found, _ := unstructured.NestedMap(crb.Object, "roleRef")
+	if !found {
+		return false, nil // No roleRef, don't skip
+	}
+
+	roleName, _, _ := unstructured.NestedString(roleRef, "name")
+	if roleName == "" {
+		return false, nil // Empty roleName, don't skip
+	}
+
+	// Check if the referenced ClusterRole exists
+	clusterRole := &unstructured.Unstructured{}
+	clusterRole.SetGroupVersionKind(schema.GroupVersionKind{
+		Group:   "rbac.authorization.k8s.io",
+		Version: "v1",
+		Kind:    "ClusterRole",
+	})
+	clusterRole.SetName(roleName)
+
+	err := cli.Get(ctx, client.ObjectKey{Name: roleName}, clusterRole)
+	if err != nil && k8serr.IsNotFound(err) {
+		return true, nil
+	} else if err != nil {
+		return false, err
+	}
+	return false, nil
+}
diff --git a/release/operator.yaml b/release/operator.yaml
@@ -2262,6 +2262,7 @@ rules:
 - apiGroups:
   - ""
   resources:
+  - serviceaccounts
   - services
   verbs:
   - create
@@ -2321,6 +2322,40 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - clusterrolebindings
+  verbs:
+  - create
+  - delete
+  - get
+  - list
+  - patch
+  - update
+  - watch
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - clusterroles
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - security.openshift.io
+  resources:
+  - securitycontextconstraints
+  verbs:
+  - use
+- apiGroups:
+  - security.openshift.io
+  resourceNames:
+  - anyuid
+  resources:
+  - securitycontextconstraints
+  verbs:
+  - use
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole

Original file line number	Diff line number	Diff line change
`@@ -230,6 +230,11 @@ func (r *LlamaStackDistributionReconciler) reconcileResources(ctx context.Contex`
`230`	`230`	`return fmt.Errorf("failed to reconcile NetworkPolicy: %w", err)`
`231`	`231`	`}`
`232`	`232`
	`233`	`+ // Reconcile manifest-based resources`
	`234`	`+ if err := r.reconcileManifestResources(ctx, instance); err != nil {`
	`235`	`+ return err`
	`236`	`+ }`
	`237`	`+`
`233`	`238`	`// Reconcile the Deployment`
`234`	`239`	`if err := r.reconcileDeployment(ctx, instance); err != nil {`
`235`	`240`	`return fmt.Errorf("failed to reconcile Deployment: %w", err)`
`@@ -242,11 +247,6 @@ func (r *LlamaStackDistributionReconciler) reconcileResources(ctx context.Contex`
`242`	`247`	`}`
`243`	`248`	`}`
`244`	`249`
`245`		`- // Reconcile manifest-based resources`
`246`		`- if err := r.reconcileManifestResources(ctx, instance); err != nil {`
`247`		`- return err`
`248`		`- }`
`249`		`-`
`250`	`250`	`return nil`
`251`	`251`	`}`
`252`	`252`
Original file line number	Diff line number	Diff line change
`@@ -561,8 +561,8 @@ func TestPodOverridesWithoutServiceAccount(t *testing.T) {`
`561`	`561`	`// Apply pod overrides`
`562`	`562`	`configurePodOverrides(instance, &deployment.Spec.Template.Spec)`
`563`	`563`
`564`		`- // Verify ServiceAccount name is empty`
`565`		`- if deployment.Spec.Template.Spec.ServiceAccountName != "" {`
`566`		`- t.Errorf("expected empty ServiceAccountName, got %s", deployment.Spec.Template.Spec.ServiceAccountName)`
	`564`	`+ // Verify ServiceAccount name is empty (default ServiceAccountName should be set when not explicitly provided)`
	`565`	`+ if deployment.Spec.Template.Spec.ServiceAccountName != instance.Name+"-sa" {`
	`566`	`+ t.Errorf("expected default ServiceAccountName when not explicitly provided, got %s", deployment.Spec.Template.Spec.ServiceAccountName)`
`567`	`567`	`}`
`568`	`568`	`}`