Add custom labels, securityContext to DaemonSets#2120
Add custom labels, securityContext to DaemonSets#2120faganihajizada wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
faganihajizada
requested review from
ArangoGutierrez,
cdesiniotis,
karthikvetrivel,
shivamerla and
tariq1890
as code owners
Allow users to configure pod-level and container-level security contexts
globally for all DaemonSets via spec.daemonsets.podSecurityContext and
spec.daemonsets.securityContext. Pod-level replaces the existing context;
container-level merges into each container, preserving fields already set
by component transforms.
Propagate custom labels and annotations from the validator DaemonSet pod
template onto CUDA and plugin workload pods, skipping protected selector
labels ("app", "app.kubernetes.io/part-of").
Add commonLabels support to the NFD Helm subchart so users can inject
additional labels onto all NFD resources.
faganihajizada
force-pushed
the
feat/custom-labels-and-security-context
branch
from
57fee73 to
cc94407
Compare
| namespaceOverride: "" | ||
|
|
||
| # commonLabels are added to all resources created by this chart (DaemonSets, Deployments, etc.) | ||
| commonLabels: {} |
There was a problem hiding this comment.
We don't really edit the NFD subchart directly. We import it upstream
There was a problem hiding this comment.
Also, this change may be out-of-scope for this PR. Let's limit this PR to just SecurityContext-related changes
There was a problem hiding this comment.
I see. this makes sense, thanks!
There was a problem hiding this comment.
Reverted the NFD subchart changes so we don’t edit the upstream NFD chart.
controllers/object_controls.go
Outdated
| } | ||
|
|
||
| // mergeSecurityContext copies fields from defaults into target only when the target's field is nil. | ||
| func mergeSecurityContext(target, defaults *corev1.SecurityContext) { |
There was a problem hiding this comment.
Can we use a library like mergo instead to perform struct merges?
There was a problem hiding this comment.
I simplified the implementation to only add PodSecurityContext and removed the container-level SecurityContext field and the merge logic. There’s no struct merge anymore, we just assign the pod SecurityContext when set. I believe mergo isn’t needed for this PR.
api/nvidia/v1/clusterpolicy_types.go
Outdated
| PodSecurityContext *corev1.PodSecurityContext `json:"podSecurityContext,omitempty"` | ||
|
|
||
| // Optional: Set default container-level security context for all DaemonSet pods | ||
| SecurityContext *corev1.SecurityContext `json:"securityContext,omitempty"` |
There was a problem hiding this comment.
I see that we are applying SecurityContext to all of the containers and init-containers of a DaemonSet. Won't setting PodSecurityContext make this redundant?
If yes, we should probably just stick with PodSecurityContext
There was a problem hiding this comment.
Agreed. I’ve removed the container-level SecurityContext field and all related logic; the PR now only adds PodSecurityContext. Thanks!
faganihajizada
force-pushed
the
feat/custom-labels-and-security-context
branch
from
bd26132 to
7b5211a
Compare
Allow users to set pod-level security context for all DaemonSet pods via spec.daemonsets.podSecurityContext. Kubernetes applies it as defaults to all containers and init containers in the pod (e.g. runAsUser, runAsGroup, runAsNonRoot). Scope limited to SecurityContext-related changes only; no NFD or validator label changes in this PR.
faganihajizada
force-pushed
the
feat/custom-labels-and-security-context
branch
from
7b5211a to
0e21f29
Compare
2 participants
Description
Closes #1030
Adds three capabilities to ClusterPolicy:
Custom labels/annotations on validator workload pods: spec.daemonsets.labels and spec.daemonsets.annotations now propagate from the validator DaemonSet pod template onto the standalone CUDA and plugin validator workload pods. Protected selector labels (app, app.kubernetes.io/part-of) are skipped to preserve pod identity.
Custom labels on NFD components: New commonLabels value in the NFD Helm subchart, injected into the shared node-feature-discovery.labels template. Applies to all NFD-managed resources (DaemonSets, Deployments, etc.) without affecting selector labels.
Custom securityContext on DaemonSets:
Design notes
Pod-level SecurityContext is full replace, not merge. This matches the existing pattern for tolerations and priorityClassName in applyCommonDaemonsetConfig. No embedded DaemonSet manifest currently sets a pod-level SecurityContext, so there is nothing to lose on replace.
Container-level SecurityContext uses nil-field merge. Every container in the operator's DaemonSet manifests already sets privileged: true. A full replace would wipe that. The merge ensures global defaults only fill in what components haven't explicitly set.
applyCommonDaemonsetConfigruns before component transforms(t(obj, ...)), so transforms can still override individual fields afterward.mergeSecurityContext is explicit field-by-field (all 12 SecurityContext fields). This is deliberate over reflection or a library, it's readable, has no external dependencies, and matches the codebase style of explicit struct handling.
The issue references "Jobs" but the cuda-validator and plugin-validator are standalone Pods created by the validator DaemonSet, not Jobs. The label propagation targets these Pods.
Checklist
make lint)make validate-generated-assets)make validate-modules)Testing
mergeSecurityContext(5 cases: nil target, nil defaults, empty target, merge with existing, no-overwrite)applyCommonDaemonsetConfig(4 new cases: podSecurityContext, nil container SecurityContext, merge into existing, initContainers)applyDaemonsetMetadataToPod(4 cases: no-op, label skip, annotations, nil labels)applyCommonDaemonsetMetadata(existing tests still pass)