Skip to content

sysctl decides idempotency from its own record, so drift is never corrected #564

Description

@retr0h

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingkind/go

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions