feat(helm): allow a per-set sssd.conf for LoginSet and NodeSet - #263
Open
AndreyZa wants to merge 1 commit into
Open
feat(helm): allow a per-set sssd.conf for LoginSet and NodeSet#263AndreyZa wants to merge 1 commit into
AndreyZa wants to merge 1 commit into
Conversation
`sssdConfRef` is a per-set field on both CRs, but the chart can only fill it from the cluster-wide `sssd.secretRef`: `slurm.sssdConf.name` reads `.Values.sssd.secretRef` and never sees the set it is rendering. Every LoginSet and NodeSet in a release therefore resolves identities from the same directory. That is limiting as soon as a cluster serves two populations from two directories — for example a shared login for staff and a separate login for students, each with its own domain. A single shared `sssd.conf` would give both sets both domains, which is exactly what such a split is meant to avoid. The only workaround today is to patch `sssdConfRef` on the CR after install, and that patch is silently lost when the global `sssd.secretRef` changes, the CR is recreated, or the release is reinstalled. `loginsets.<name>.sssd.secretRef` and `nodesets.<name>.sssd.secretRef` now take precedence, falling back to the cluster-wide value. Both are documented in `values.yaml` and the generated README. Rendered with `helm template` on a release with a global secret plus one overriding LoginSet and one overriding NodeSet: the overriding sets get their own secret, the others keep the global one, and a release with no overrides at all still renders `<release>-sssd-conf` exactly as before. `helm lint` passes.
AndreyZa
requested review from
SkylerMalinowski,
alanmutsch,
catblade,
vivian-hafener and
wickberg
as code owners
September 11, 2026 11:58
vivian-hafener
requested changes
Sep 11, 2026
vivian-hafener
left a comment
Contributor
There was a problem hiding this comment.
Please add Helm unit tests to cover the added behavior. Also, the commit does not include the project's required git trailers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sssdConfRefis a per-set field on bothLoginSetandNodeSet, but the chart can only fill it from the cluster-wide value:slurm.sssdConf.namereads.Values.sssd.secretRefand never sees the set it is rendering. Every LoginSet and NodeSet in a release therefore resolves identities from the same directory.That becomes limiting as soon as one cluster serves two populations from two directories. Our case: a shared login for staff, whose
sssd.confpoints at the institute directory and whose pod mounts the shared/home, and a separate login for students, whosesssd.confpoints at a different directory. A single sharedsssd.confwould give both sets both domains — precisely what the split exists to prevent, since the staff login exposes home directories the students must not reach.The only workaround today is to patch
sssdConfRefon the CR after install. That patch survives an ordinaryhelm upgrade(the field is identical in the old and new rendered manifests, so it is not part of the patch Helm computes), but it is silently lost when the globalsssd.secretRefchanges, when the CR is recreated, or when the release is reinstalled — and the failure mode is a login where users simply "do not exist".Change
loginsets.<name>.sssd.secretRefandnodesets.<name>.sssd.secretRefnow take precedence over the cluster-widesssd.secretRef, falling back to it when unset. Three small edits:templates/loginset/loginset-cr.yaml—digthe per-set value, default to the existing helper;templates/nodeset/nodeset-cr.yaml— same, beforessh.sssdConfRefis set;values.yaml— the new key documented underloginsetDefaultsandnodesetDefaults, plus the regenerated README.The helpers themselves are untouched, so the cluster-wide value keeps working exactly as before.
Related: #119 wired
sssd.secretRefthrough to the CR in the first place; this makes that wiring per-set.Checklist
values.yamlcomments and the helm-docs README.)Breaking Changes
None. With no per-set value present the rendered
sssdConfRefis byte-for-byte what it was.Testing Notes
helm templateagainst a release with a global secret plus one overriding LoginSet and one overriding NodeSet:sssdConfRef.namestaff(no override)global-sssdstudents(override)students-sssdcpu(no override)global-sssdedu(override)students-sssdDefaults unchanged: a release with no
sssd.secretRefand no per-set value renders<release>-sssd-conf; with only the global value set it renders that value.helm lint helm/slurmpasses,make helm-docsproduces no further diff.Additional Context
Found while adding a student-facing login to a cluster that already had a staff login: the second LoginSet came up with the first one's domain, and users of the second directory were unresolvable with no error anywhere — the pod is healthy,
getentsimply returns nothing.