MSP-3392: add supporting apparmor to soperator by Uburro · Pull Request #288 · nebius/soperator

Uburro · 2024-12-30T16:52:44Z

Partially addresses issue but not completely. The mandatory linking to libnvidia-ml.so.1 was fixed in patch 22.05.5. We are currently using version 22.05.2.

internal/render/common/apparmorprofile.go

api/v1/slurmcluster_types.go

config/crd/bases/slurm.nebius.ai_slurmclusters.yaml

internal/render/common/apparmorprofile.go

Copilot

Pull request overview

This PR adds AppArmor profile support to the soperator for Slurm worker and login nodes, addressing issue #184. It introduces a new UseDefaultAppArmorProfile field (defaulting to true) that generates and applies a default AppArmor profile to restrict write access to NVIDIA libraries and binaries. The PR notes that the mandatory linking to libnvidia-ml.so.1 was fixed in Slurm patch 24.05.5, but the operator currently uses version 22.05.2.

Key changes:

Added UseDefaultAppArmorProfile boolean field to the SlurmCluster CRD with a default value of true
Implemented AppArmor profile generation with deny rules for NVIDIA library/binary write access
Integrated AppArmor profile reconciliation into the cluster controller workflow

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
api/v1/slurmcluster_types.go	Added `UseDefaultAppArmorProfile` field to SlurmClusterSpec
config/crd/bases/slurm.nebius.ai_slurmclusters.yaml	Updated CRD with new AppArmor profile field definition
helm/soperator-crds/templates/slurmcluster-crd.yaml	Synchronized CRD template with base CRD changes
helm/soperator/crds/slurmcluster-crd.yaml	Synchronized CRD with base CRD changes
helm/slurm-cluster/values.yaml	Added `useDefaultAppArmorProfile: true` configuration option
helm/slurm-cluster/templates/slurm-cluster-cr.yaml	Wired AppArmor profile setting to cluster CR
internal/render/common/apparmorprofile.go	Implemented AppArmor profile rendering and policy generation
internal/naming/naming.go	Added AppArmor profile naming function
internal/values/slurm_worker.go	Added AppArmor profile field to SlurmWorker and propagated value
internal/values/slurm_login.go	Added AppArmor profile field to SlurmLogin and propagated value
internal/values/slurm_cluster.go	Integrated AppArmor profile flag into cluster value building
internal/values/slurm_worker_test.go	Updated test to pass new AppArmor parameter
internal/render/worker/statefulset.go	Refactored annotations to conditionally use AppArmor profile
internal/render/login/statefulset.go	Refactored annotations to conditionally use AppArmor profile
internal/controller/reconciler/apparmorprofile.go	Added AppArmor profile reconciler implementation
internal/controller/reconciler/reconciler.go	Registered AppArmor profile type in reconciler
internal/controller/reconciler/k8s_statefulset.go	Added template annotations/labels to statefulset patching
internal/controller/clustercontroller/reconcile.go	Added AppArmorProfile reconciler field and initialization
internal/controller/clustercontroller/common.go	Integrated AppArmor profile reconciliation step
internal/check/installed_crd.go	Added AppArmor CRD installation check
cmd/main.go	Registered AppArmor scheme when CRD is installed
config/rbac/role.yaml	Added RBAC permissions for AppArmor profiles
helm/soperator/templates/manager-rbac.yaml	Added RBAC permissions for AppArmor profiles
helm/soperator/values.yaml	Added `isApparmorCrdInstalled` environment variable
helm/soperator/templates/deployment.yaml	Added AppArmor CRD environment variable to deployment
config/manager/manager.yaml	Added IS_APPARMOR_CRD_INSTALLED environment variable
go.mod	Added security-profiles-operator dependency
go.sum	Updated checksums for new dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-08T11:01:25Z

internal/render/common/apparmorprofile.go

+)
+
+func RenderAppArmorProfile(cluster *values.SlurmCluster) *apparmor.AppArmorProfile {
+	if !cluster.NodeLogin.UseDefaultAppArmorProfile || !cluster.NodeWorker.UseDefaultAppArmorProfile {


Logic error: The condition uses OR (||) which means the AppArmor profile will NOT be created if EITHER NodeLogin OR NodeWorker has UseDefaultAppArmorProfile set to false. This should use AND (&&) so that the profile is only skipped when BOTH are false. With the current logic, if only one node type wants to use the default profile, it won't be created.

Suggested change

if !cluster.NodeLogin.UseDefaultAppArmorProfile || !cluster.NodeWorker.UseDefaultAppArmorProfile {

if !cluster.NodeLogin.UseDefaultAppArmorProfile && !cluster.NodeWorker.UseDefaultAppArmorProfile {

Copilot · 2025-12-08T11:01:25Z

internal/controller/clustercontroller/common.go

+						stepLogger.Info("AppArmor CRD is not installed, skipping AppArmor profile reconciliation")
+						return nil
+					}
+					if !clusterValues.NodeLogin.UseDefaultAppArmorProfile || !clusterValues.NodeWorker.UseDefaultAppArmorProfile {


Logic error: Same as in RenderAppArmorProfile, this condition uses OR (||) when it should use AND (&&). The profile reconciliation should be skipped only when BOTH NodeLogin and NodeWorker have UseDefaultAppArmorProfile set to false, not when either one is false.

Suggested change

if !clusterValues.NodeLogin.UseDefaultAppArmorProfile || !clusterValues.NodeWorker.UseDefaultAppArmorProfile {

if !clusterValues.NodeLogin.UseDefaultAppArmorProfile && !clusterValues.NodeWorker.UseDefaultAppArmorProfile {

Copilot · 2025-12-08T11:01:26Z

internal/controller/reconciler/apparmorprofile.go

+			Error(err, "Failed to reconcile AppArmorProfile ")
+		return errors.Wrap(err, "reconciling AppArmorProfile ")


Trailing space in error messages. Both "Failed to reconcile AppArmorProfile " and "reconciling AppArmorProfile " have trailing spaces that should be removed for consistency.

Suggested change

Error(err, "Failed to reconcile AppArmorProfile ")

return errors.Wrap(err, "reconciling AppArmorProfile ")

Error(err, "Failed to reconcile AppArmorProfile")

return errors.Wrap(err, "reconciling AppArmorProfile")

Copilot · 2025-12-08T11:01:26Z

internal/render/common/apparmorprofile.go

+  /** lrixw,
+
+
+  # remove [^m], when bump slurm 24.05.5 or higher


The comment says "remove [^m], when bump slurm 24.05.5 or higher" but should probably say "when bumping to slurm 24.05.5 or higher" for better grammar. Additionally, consider "bump to" instead of "bump" for clarity.

Suggested change

# remove [^m], when bump slurm 24.05.5 or higher

# remove [^m] when bumping to slurm 24.05.5 or higher

Copilot · 2025-12-08T11:01:26Z

internal/controller/reconciler/apparmorprofile.go

+	deps ...metav1.Object,
+) error {
+	if desired == nil {
+		return errors.New("AppArmorProfile is not needed")


The error message "AppArmorProfile is not needed" is misleading for the error case. If desired is nil, it's not necessarily an error condition - the calling code checks if AppArmor is needed and may intentionally pass nil. This check should either be removed (let the calling code handle nil), or the error message should be clearer, such as "desired AppArmorProfile cannot be nil".

Suggested change

return errors.New("AppArmorProfile is not needed")

return errors.New("desired AppArmorProfile cannot be nil")

Copilot · 2025-12-08T11:01:27Z

helm/slurm-cluster/values.yaml

 clusterName: "slurm1"
 # Additional annotations for the cluster
 annotations: {}
+# Add appArmor profile to the cluster


Inconsistent capitalization: "appArmor" should be "AppArmor" to match the standard capitalization used throughout the codebase and in the API field name.

Suggested change

# Add appArmor profile to the cluster

# Add AppArmor profile to the cluster

Copilot · 2025-12-08T11:01:27Z

internal/render/worker/statefulset.go

+func renderAnnotations(worker *values.SlurmWorker, clusterName, namespace string) map[string]string {
+	mungeAppArmorProfile := worker.ContainerMunge.AppArmorProfile
+	workerAppArmorProfile := worker.ContainerSlurmd.AppArmorProfile
+
+	if worker.UseDefaultAppArmorProfile {
+		workerAppArmorProfile = fmt.Sprintf("%s/%s", "localhost", naming.BuildAppArmorProfileName(clusterName, namespace))
+		mungeAppArmorProfile = fmt.Sprintf("%s/%s", "localhost", naming.BuildAppArmorProfileName(clusterName, namespace))
+	}
+
+	annotations := map[string]string{
+		fmt.Sprintf(
+			"%s/%s", consts.AnnotationApparmorKey, consts.ContainerNameSlurmd,
+		): workerAppArmorProfile,
+		fmt.Sprintf(
+			"%s/%s", consts.AnnotationApparmorKey, consts.ContainerNameMunge,
+		): mungeAppArmorProfile,
+		consts.DefaultContainerAnnotationName: consts.ContainerNameSlurmd,
+	}
+
+	return annotations
+}


[nitpick] Code duplication: The renderAnnotations function in worker/statefulset.go and login/statefulset.go are nearly identical. Consider extracting this logic into a shared function in the common package to reduce duplication and improve maintainability.

Copilot · 2025-12-08T11:01:27Z

internal/render/common/apparmorprofile.go

+  deny /usr/lib/x86_64-linux-gnu/nvidia/wine/nvngx.dll w,
+  deny /usr/lib/x86_64-linux-gnu/nvidia/xorg/libglxserver_nvidia* w,
+  deny /usr/lib/x86_64-linux-gnu/nvidia/xorg/nvidia_drv.so w,
+  deny /usr/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia


Missing w, at the end of this deny rule. This line should end with w, to be consistent with the other deny rules and to ensure the AppArmor policy is syntactically correct.

Suggested change

deny /usr/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia

deny /usr/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia w,

Uburro requested review from asteny, dstaroff and rdjjke as code owners December 30, 2024 16:52

Uburro force-pushed the MSP-3392 branch from c070528 to c0dba9c Compare December 30, 2024 17:08

Uburro added the feature label Dec 30, 2024

rdjjke reviewed Dec 30, 2024

View reviewed changes

internal/render/common/apparmorprofile.go Outdated Show resolved Hide resolved

asteny reviewed Dec 31, 2024

View reviewed changes

api/v1/slurmcluster_types.go Outdated Show resolved Hide resolved

Uburro force-pushed the MSP-3392 branch from 2c75bf5 to 59df132 Compare December 31, 2024 09:56

asteny previously approved these changes Dec 31, 2024

View reviewed changes

rdjjke reviewed Dec 31, 2024

View reviewed changes

config/crd/bases/slurm.nebius.ai_slurmclusters.yaml Outdated Show resolved Hide resolved

Uburro dismissed asteny’s stale review via 54db746 December 31, 2024 11:04

asteny previously approved these changes Dec 31, 2024

View reviewed changes

rdjjke reviewed Dec 31, 2024

View reviewed changes

internal/render/common/apparmorprofile.go Outdated Show resolved Hide resolved

rdjjke reviewed Dec 31, 2024

View reviewed changes

internal/render/common/apparmorprofile.go Outdated Show resolved Hide resolved

Uburro dismissed asteny’s stale review via 8e87c11 December 31, 2024 12:25

rdjjke reviewed Dec 31, 2024

View reviewed changes

internal/render/common/apparmorprofile.go Show resolved Hide resolved

Uburro force-pushed the MSP-3392 branch from 34c9cef to ff560e1 Compare January 2, 2025 12:41

Uburro added 7 commits January 3, 2025 13:34

MSP-3392: add supporting apparmor to soperator

d890e9f

add nvidia-container-cli list of path

941b912

fix description of useDefaultAppArmorProfile

9ca6336

add aditinal apparmor deny

2354935

add comment for Security Profiles Operator

2ededcb

make helm with commets about Security Profiles Operator

7873376

remove /mnt/jail/ from apparmor profile

1985e97

Uburro force-pushed the MSP-3392 branch from 2454a97 to 1985e97 Compare January 3, 2025 12:41

add some more lib to apparmor profile

5e69abe

asteny approved these changes Jan 7, 2025

View reviewed changes

rdjjke approved these changes Jan 7, 2025

View reviewed changes

Uburro merged commit 74841e3 into dev Jan 7, 2025

Uburro deleted the MSP-3392 branch January 7, 2025 14:42

Uburro requested a review from Copilot December 8, 2025 10:53

Copilot started reviewing on behalf of Uburro December 8, 2025 10:54 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSP-3392: add supporting apparmor to soperator#288

MSP-3392: add supporting apparmor to soperator#288
Uburro merged 8 commits intodevfrom
MSP-3392

Uburro commented Dec 30, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Copilot AI Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if !cluster.NodeLogin.UseDefaultAppArmorProfile \|\| !cluster.NodeWorker.UseDefaultAppArmorProfile {
	if !cluster.NodeLogin.UseDefaultAppArmorProfile && !cluster.NodeWorker.UseDefaultAppArmorProfile {

	if !clusterValues.NodeLogin.UseDefaultAppArmorProfile \|\| !clusterValues.NodeWorker.UseDefaultAppArmorProfile {
	if !clusterValues.NodeLogin.UseDefaultAppArmorProfile && !clusterValues.NodeWorker.UseDefaultAppArmorProfile {

		Error(err, "Failed to reconcile AppArmorProfile ")
		return errors.Wrap(err, "reconciling AppArmorProfile ")

	# remove [^m], when bump slurm 24.05.5 or higher
	# remove [^m] when bumping to slurm 24.05.5 or higher

	return errors.New("AppArmorProfile is not needed")
	return errors.New("desired AppArmorProfile cannot be nil")

	# Add appArmor profile to the cluster
	# Add AppArmor profile to the cluster

	deny /usr/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia
	deny /usr/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia w,

Conversation

Uburro commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uburro commented Dec 30, 2024 •

edited

Loading