fix(rest,satellite): encryption create-passphrase unlocks LUKS provisioning#143
Conversation
The LUKS RD-create gate (Bug 95) only consulted the legacy DrbdOptions/EncryptPassphrase controller prop, so the upstream-standard flow - linstor encryption create-passphrase followed by creating a LUKS-layered RD - failed with 400, and the error hint told operators to store a plaintext passphrase in a controller prop. Hoist the Secret access into a shared pkg/passphrase package and make the gate consult the encryption Secret first (primary, upstream-parity), keeping the legacy prop working for backward compatibility. The refusal hint now leads with encryption create-passphrase and marks the prop path deprecated. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…ayer The satellite lifts the LUKS key onto the LuksPassphrase wire prop only from the controller-scope props, so a cluster whose passphrase lives solely in the encryption Secret passed the RD-create gate but looped on 'Props.LuksPassphrase empty' at apply time. Fold the Secret value into the resolved effective props under the canonical DrbdOptions/EncryptPassphrase key before BuildDesired, so the passphrase travels the exact channel the legacy prop used (lifted to LuksPassphrase, stripped from rendered DRBD options). Operator-set props keep precedence so existing volumes keep their key. The Secret is read through the uncached APIReader in the controller namespace (new --controller-namespace flag, default $POD_NAMESPACE then blockstor-system); satellite RBAC already grants secrets read. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
L1 regression suite for the encryption-Secret flow: REST gate accepts LUKS RD creation after create-passphrase (or a directly seeded Secret), keeps the legacy controller-prop path working, and hints at create-passphrase on refusal; satellite-side injection delivers the Secret value to the LuksPassphrase wire prop with legacy-prop precedence, custom PassphraseSecretRef resolution, and safe no-ops for non-LUKS stacks and missing Secrets. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
📝 WalkthroughWalkthroughThis PR centralizes the cluster master encryption passphrase mechanism for LUKS volume support. It introduces a shared ChangesBug 023: Cluster Master Encryption Passphrase Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes access to the cluster master passphrase Secret under a new pkg/passphrase package to resolve Bug 023. Previously, the LUKS resource definition creation gate and satellite-side dispatch only checked the legacy plaintext controller property DrbdOptions/EncryptPassphrase, ignoring the Secret written by linstor encryption create-passphrase. The changes integrate the new package into the REST and satellite controllers, introduce a --controller-namespace flag to the satellite, and add comprehensive unit tests. The review feedback suggests optimizing the Secret retrieval in the satellite's injectLUKSMasterPassphrase function to use the cached client for resolving the Secret name, thereby reducing API server load while still avoiding informer stalls by using the uncached reader only for the Secret itself.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import ( | ||
| "context" | ||
| "strings" | ||
|
|
||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/log" | ||
|
|
||
| blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" | ||
| "github.com/cozystack/blockstor/pkg/passphrase" | ||
| ) |
There was a problem hiding this comment.
Add imports for corev1 and apierrors to support the optimized Secret retrieval in injectLUKSMasterPassphrase.
| import ( | |
| "context" | |
| "strings" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| "sigs.k8s.io/controller-runtime/pkg/log" | |
| blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" | |
| "github.com/cozystack/blockstor/pkg/passphrase" | |
| ) | |
| import ( | |
| "context" | |
| "strings" | |
| corev1 "k8s.io/api/core/v1" | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| "sigs.k8s.io/controller-runtime/pkg/log" | |
| blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" | |
| "github.com/cozystack/blockstor/pkg/passphrase" | |
| ) |
| pass, err := passphrase.Read(ctx, r.secretReader(), r.Config.Namespace) | ||
| if err != nil { | ||
| log.FromContext(ctx).Error(err, | ||
| "read cluster master passphrase Secret; LUKS apply will surface the missing key", | ||
| "namespace", r.Config.Namespace) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| if pass == "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
To avoid making direct, uncached API server calls for the ControllerConfig singleton on every periodic reconciliation loop (which runs every 10 seconds for active DRBD resources), we can use the cached client (r.Client) to resolve the Secret name. We only use the uncached APIReader (r.secretReader()) to read the Secret itself, which preserves the fix for the Bug 110 informer stall while significantly reducing API server load.
secretName, err := passphrase.SecretName(ctx, r.Client)
if err != nil {
log.FromContext(ctx).Error(err,
"resolve passphrase Secret name; LUKS apply will surface the missing key")
return
}
var sec corev1.Secret
if err := r.secretReader().Get(ctx, client.ObjectKey{Namespace: r.Config.Namespace, Name: secretName}, &sec); err != nil {
if !apierrors.IsNotFound(err) {
log.FromContext(ctx).Error(err,
"read cluster master passphrase Secret; LUKS apply will surface the missing key",
"namespace", r.Config.Namespace, "name", secretName)
}
return
}
pass := string(sec.Data[passphrase.SecretKey])
if pass == "" {
return
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/rest/luks_gate_bug023_test.go (1)
120-127: 💤 Low valueConsider asserting LayerStack length or order for stronger validation.
The test verifies LUKS is in the stack via
LayerInStack, but doesn't assert the full expected stack["DRBD", "LUKS", "STORAGE"]. If a bug caused layer reordering or dropping other layers, this test would still pass. Consider strengthening the assertion:if len(rd.LayerStack) != 3 || rd.LayerStack[0] != "DRBD" || rd.LayerStack[1] != "LUKS" || rd.LayerStack[2] != "STORAGE" { t.Errorf("LayerStack mismatch: got %v, want [DRBD LUKS STORAGE]", rd.LayerStack) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rest/luks_gate_bug023_test.go` around lines 120 - 127, The test only checks presence of LUKS via apiv1.LayerInStack but should assert the full expected order and length to catch reorders/drops; update the test around rd := ... ResourceDefinitions().Get(...) to additionally assert rd.LayerStack has length 3 and that rd.LayerStack[0]=="DRBD", rd.LayerStack[1]=="LUKS", and rd.LayerStack[2]=="STORAGE", emitting a clear t.Errorf message showing the actual rd.LayerStack when the check fails.cmd/satellite/main.go (1)
644-661: 💤 Low valueConsider extracting namespace resolution to a shared package.
The
resolveControllerNamespacefunction duplicates logic fromcmd/apiserver/main.go(context snippet lines 159-172). While the comment on line 649 acknowledges this is intentional to keep the binaries in sync, extracting the precedence logic to a shared package (e.g.,pkg/config) would eliminate duplication and ensure the two implementations never drift without requiring manual synchronization.If the duplication is deliberately maintained for build-time independence or deployment simplicity, this comment can be disregarded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/satellite/main.go` around lines 644 - 661, The resolveControllerNamespace function duplicates namespace-precedence logic found in cmd/apiserver/main.go; extract this logic into a shared package (e.g., pkg/config) as a single exported function like ResolveControllerNamespace or ControllerNamespace that implements the same precedence (flag -> POD_NAMESPACE -> "blockstor-system"), update cmd/satellite/main.go to call the new pkg/config function instead of its local resolveControllerNamespace, and update cmd/apiserver/main.go to use the same shared function so both binaries share one source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/satellite/main.go`:
- Around line 644-661: The resolveControllerNamespace function duplicates
namespace-precedence logic found in cmd/apiserver/main.go; extract this logic
into a shared package (e.g., pkg/config) as a single exported function like
ResolveControllerNamespace or ControllerNamespace that implements the same
precedence (flag -> POD_NAMESPACE -> "blockstor-system"), update
cmd/satellite/main.go to call the new pkg/config function instead of its local
resolveControllerNamespace, and update cmd/apiserver/main.go to use the same
shared function so both binaries share one source of truth.
In `@pkg/rest/luks_gate_bug023_test.go`:
- Around line 120-127: The test only checks presence of LUKS via
apiv1.LayerInStack but should assert the full expected order and length to catch
reorders/drops; update the test around rd := ... ResourceDefinitions().Get(...)
to additionally assert rd.LayerStack has length 3 and that
rd.LayerStack[0]=="DRBD", rd.LayerStack[1]=="LUKS", and
rd.LayerStack[2]=="STORAGE", emitting a clear t.Errorf message showing the
actual rd.LayerStack when the check fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00f3c363-b124-47f6-a191-92ddb5edf0a5
📒 Files selected for processing (13)
.golangci.ymlcmd/satellite/main.gopkg/passphrase/passphrase.gopkg/rest/encryption.gopkg/rest/luks_gate_bug023_test.gopkg/rest/resource_definitions.gopkg/satellite/controllers/config.gopkg/satellite/controllers/luks_passphrase.gopkg/satellite/controllers/luks_passphrase_internal_test.gopkg/satellite/controllers/resource.gopkg/satellite/stream/redial_spec_test.gostand/blockstor-satellite-daemonset.yamltests/scenarios/03-networking.md
1 participant
Problem
linstor encryption create-passphrasestores the cluster master passphrase in a Secret, but nothing downstream ever read that Secret:DrbdOptions/EncryptPassphrasecontroller property, so the upstream-standard flow (encryption create-passphrase→rd create --layer-list drbd,luks,storage) failed with 400 — and the error hint instructed operators to store a PLAINTEXT passphrase in a controller prop;LuksPassphrasewire prop only from controller-scope props, so even with the gate fixed, a Secret-only cluster would loop onLUKS in layer stack but Props.LuksPassphrase emptyat apply time.What changed
pkg/passphrasepackage centralises the Secret resolution (ControllerConfigPassphraseSecretRef→ defaultblockstor-cluster-passphrase); the REST encryption handlers now write/read through it.encryption create-passphrase.DrbdOptions/EncryptPassphrasekey beforeBuildDesired, so the passphrase travels the exact channel the legacy prop used — lifted onto theLuksPassphrasewire prop and stripped from the rendered DRBD options. Operator-set props keep precedence over the Secret so existing LUKS volumes keep unlocking with the key they were formatted with.--controller-namespaceflag defaulting to$POD_NAMESPACEthenblockstor-system, and the stand DaemonSet now passesPOD_NAMESPACEvia the downward API. Satellite RBAC already grants read access to Secrets.Testing
pkg/rest/luks_gate_bug023_test.go— create-passphrase → LUKS RD create succeeds (was 400), GitOps-seeded Secret accepted, legacy prop path still accepted, refusal hints at create-passphrase;pkg/satellite/controllers/luks_passphrase_internal_test.go— Secret value reaches theLuksPassphrasewire prop viaBuildDesired, legacy-prop precedence preserved, customPassphraseSecretRefhonoured, safe no-ops for non-LUKS stacks and missing Secrets.TestGroupKWFLUKSStackEndToEnd(operator CLI flow over envtest) goes red → green with this change.go test ./...green,golangci-lint run0 issues.Summary by CodeRabbit
Release Notes
New Features
--controller-namespaceCLI flag to the satellite process for specifying where cluster master passphrase secrets are located.Documentation
--controller-namespaceflag behavior and defaults.Tests