sysctl.Create returns Changed: false whenever a KV state entry exists, without ever looking at the host. It is the only provider that still does this.
The code
internal/provider/node/sysctl/debian.go:
// Create deploys a new sysctl conf file and applies it. Idempotent:
// returns Changed: false if the key is already managed.
...
// Already managed — nothing to do.
kvEntry, err := d.stateKV.Get(ctx, stateKey)
if err == nil {
var state job.FileState
if unmarshalErr := json.Unmarshal(kvEntry.Value(), &state); unmarshalErr == nil {
if state.UndeployedAt == "" {
return &CreateResult{
Key: entry.Key,
Changed: false,
}, nil
}
}
}
The requested entry.Value is not compared against anything.
Three failure modes
- A hand-edited
/etc/sysctl.d/ file stands. The record still says managed, so create reports no change and walks away from the one host that needed the change.
- A deleted file is never restored. Same reason.
- Create with a different value silently does nothing.
Create(key, "1") then Create(key, "0") returns Changed: false both times and leaves 1 on the host.
The third is the worst, because it is reachable without anybody touching the host.
This is the bug c3e028e8 already fixed elsewhere
That commit fixed the same shape in the file provider: the idempotency check compared the content to deploy against the SHA the last deploy recorded, so a config edited by hand still carried that SHA and the check called it unchanged.
Every other provider is correct
| provider |
decides from |
| file |
hash of the file on disk |
| ntp |
hash of the file on disk |
| netplan |
hash of the file on disk |
| user ssh keys |
reads authorized_keys |
| cron, certificate, service |
delegate to fileDeployer |
| sysctl |
the KV record |
cron and ntp both document the right principle in comments. cron:
An entry already at this path is not a reason to stop: the deploy compares the content on disk and rewrites it if somebody has edited it.
ntp:
Idempotent on content rather than on existence: a drop-in already holding what was asked for reports Changed: false, and one that has been edited is rewritten, because the operation's job is to make the configuration true.
sysctl's comment states the bug as the intent: "returns Changed: false if the key is already managed."
Fix
Compare against the host, the way ntp does: read the conf file, hash it, compare with the content that would be written, and deploy when they differ. The KV record is written, not read; it serves status, staleness and audit.
isManagedFile style checks remain legitimate for scoping, which is how cron and certificate use them. Answering "is this file ours" is fine. Answering "is it already correct" is not.
Tests
The gap is not covered today. Needed:
- create on a key whose file was edited by hand asserts
Changed: true and the content restored
- create on a key whose file was deleted asserts
Changed: true
- create with a different value on a managed key asserts
Changed: true and the new value on disk
Contract: osapi-io/specs#238.
sysctl.CreatereturnsChanged: falsewhenever a KV state entry exists, without ever looking at the host. It is the only provider that still does this.The code
internal/provider/node/sysctl/debian.go:The requested
entry.Valueis not compared against anything.Three failure modes
/etc/sysctl.d/file stands. The record still says managed, so create reports no change and walks away from the one host that needed the change.Create(key, "1")thenCreate(key, "0")returnsChanged: falseboth times and leaves1on the host.The third is the worst, because it is reachable without anybody touching the host.
This is the bug c3e028e8 already fixed elsewhere
That commit fixed the same shape in the file provider: the idempotency check compared the content to deploy against the SHA the last deploy recorded, so a config edited by hand still carried that SHA and the check called it unchanged.
Every other provider is correct
authorized_keysfileDeployercron and ntp both document the right principle in comments. cron:
ntp:
sysctl's comment states the bug as the intent: "returns Changed: false if the key is already managed."
Fix
Compare against the host, the way ntp does: read the conf file, hash it, compare with the content that would be written, and deploy when they differ. The KV record is written, not read; it serves status, staleness and audit.
isManagedFilestyle checks remain legitimate for scoping, which is how cron and certificate use them. Answering "is this file ours" is fine. Answering "is it already correct" is not.Tests
The gap is not covered today. Needed:
Changed: trueand the content restoredChanged: trueChanged: trueand the new value on diskContract: osapi-io/specs#238.