From af0f4ab96f3748166e5b58fae178ade142936a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Thu, 1 Oct 2026 10:57:38 -0700 Subject: [PATCH] fix(sysctl): decide from the drop-in, not from the record Create returned Changed: false whenever a KV state entry existed with no UndeployedAt, without reading the host and without comparing the value it was asked for. deploy already compares against the file on disk, so the pre-check was the only thing standing between a request and the right answer. Three cases it got wrong. A drop-in edited by hand stood, because the record still said managed. A deleted drop-in was never restored, for the same reason. And create with a different value on a managed key reported no change and left the old value in place, which needed nobody to touch the host at all. The test that covered this asserted the defect: the record held 0, the request asked for 1, and it expected Changed: false. It is replaced by four cases covering unchanged content, a changed value, a hand edit and a deletion. Against the previous implementation they fail ten times. Every other provider already decides this way. ntp and cron say so in their own comments. Closes #564 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FuKUsHFG1EqZXamffh9M2c --- internal/provider/node/sysctl/debian.go | 27 +-- .../node/sysctl/debian_public_test.go | 155 +++++++++++++----- 2 files changed, 118 insertions(+), 64 deletions(-) diff --git a/internal/provider/node/sysctl/debian.go b/internal/provider/node/sysctl/debian.go index f2ccd99a9..c6dadd81a 100644 --- a/internal/provider/node/sysctl/debian.go +++ b/internal/provider/node/sysctl/debian.go @@ -92,8 +92,11 @@ func NewDebianProvider( } } -// Create deploys a new sysctl conf file and applies it. Idempotent: -// returns Changed: false if the key is already managed. +// Create deploys a new sysctl conf file and applies it. 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, deleted, or holds a +// different value is rewritten, because the operation's job is to make the +// setting true. func (d *Debian) Create( ctx context.Context, entry Entry, @@ -114,23 +117,9 @@ func (d *Debian) Create( return nil, fmt.Errorf("sysctl create: %w", err) } - confPath := confPath(entry.Key) - stateKey := file.BuildStateKey(d.hostname, confPath) - - // 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 - } - } - } - + // Whether the file is recorded as managed does not decide anything. The + // record says what osapi last wrote, which is exactly what drift makes + // untrue, so deploy compares against the file on disk instead. return d.deploy(ctx, entry, "create") } diff --git a/internal/provider/node/sysctl/debian_public_test.go b/internal/provider/node/sysctl/debian_public_test.go index b1b5e8d3d..ffc04342f 100644 --- a/internal/provider/node/sysctl/debian_public_test.go +++ b/internal/provider/node/sysctl/debian_public_test.go @@ -117,11 +117,6 @@ func (suite *DebianPublicTestSuite) TestCreate() { Value: "1", }, setup: func() { - // Create reads the record once, to ask whether this key is - // already managed. Whether to write is decided from the disk. - suite.mockStateKV.EXPECT(). - Get(gomock.Any(), gomock.Any()). - Return(nil, errors.New("not found")) suite.mockStateKV.EXPECT(). Put(gomock.Any(), gomock.Any(), gomock.Any()). Return(uint64(1), nil) @@ -146,23 +141,18 @@ func (suite *DebianPublicTestSuite) TestCreate() { }, }, { - name: "when key already managed returns unchanged", + name: "when the drop-in already holds what was asked for", entry: sysctl.Entry{ Key: "net.ipv4.ip_forward", Value: "1", }, setup: func() { - stateBytes := managedStateJSON( - "net.ipv4.ip_forward", - "0", + suite.NoError(suite.memFs.MkdirAll("/etc/sysctl.d", 0o755)) + suite.NoError(suite.memFs.WriteFile( "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", - ) - mockEntry := jobmocks.NewMockKeyValueEntry(suite.ctrl) - mockEntry.EXPECT().Value().Return(stateBytes) - - suite.mockStateKV.EXPECT(). - Get(gomock.Any(), gomock.Any()). - Return(mockEntry, nil) + []byte("net.ipv4.ip_forward = 1\n"), + 0o644, + )) }, validateFunc: func( result *sysctl.CreateResult, @@ -174,6 +164,109 @@ func (suite *DebianPublicTestSuite) TestCreate() { suite.False(result.Changed) }, }, + { + // The record says this key is managed and holds 0. The request asks + // for 1. Deciding from the record reports no change and leaves the + // host on 0, which is the defect this case exists to catch. + name: "when a managed key is asked for a different value", + entry: sysctl.Entry{ + Key: "net.ipv4.ip_forward", + Value: "1", + }, + setup: func() { + suite.NoError(suite.memFs.MkdirAll("/etc/sysctl.d", 0o755)) + suite.NoError(suite.memFs.WriteFile( + "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", + []byte("net.ipv4.ip_forward = 0\n"), + 0o644, + )) + suite.mockStateKV.EXPECT(). + Put(gomock.Any(), gomock.Any(), gomock.Any()). + Return(uint64(1), nil) + suite.mockExec.EXPECT(). + RunPrivilegedCmd(gomock.Any(), "sysctl", []string{"-p", "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf"}). + Return("", nil) + }, + validateFunc: func( + result *sysctl.CreateResult, + err error, + ) { + suite.NoError(err) + suite.True(result.Changed) + + content, readErr := suite.memFs.ReadFile( + "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", + ) + suite.NoError(readErr) + suite.Equal("net.ipv4.ip_forward = 1\n", string(content)) + }, + }, + { + // Somebody edited the drop-in by hand. osapi owns the state, so the + // next create overwrites it. + name: "when the drop-in was edited by hand", + entry: sysctl.Entry{ + Key: "net.ipv4.ip_forward", + Value: "1", + }, + setup: func() { + suite.NoError(suite.memFs.MkdirAll("/etc/sysctl.d", 0o755)) + suite.NoError(suite.memFs.WriteFile( + "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", + []byte("# edited by hand\nnet.ipv4.ip_forward = 0\n"), + 0o644, + )) + suite.mockStateKV.EXPECT(). + Put(gomock.Any(), gomock.Any(), gomock.Any()). + Return(uint64(1), nil) + suite.mockExec.EXPECT(). + RunPrivilegedCmd(gomock.Any(), "sysctl", []string{"-p", "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf"}). + Return("", nil) + }, + validateFunc: func( + result *sysctl.CreateResult, + err error, + ) { + suite.NoError(err) + suite.True(result.Changed) + + content, readErr := suite.memFs.ReadFile( + "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", + ) + suite.NoError(readErr) + suite.Equal("net.ipv4.ip_forward = 1\n", string(content)) + }, + }, + { + // The record says managed, the file is gone. Restoring it is the + // whole point of running create again. + name: "when the drop-in was deleted", + entry: sysctl.Entry{ + Key: "net.ipv4.ip_forward", + Value: "1", + }, + setup: func() { + suite.mockStateKV.EXPECT(). + Put(gomock.Any(), gomock.Any(), gomock.Any()). + Return(uint64(1), nil) + suite.mockExec.EXPECT(). + RunPrivilegedCmd(gomock.Any(), "sysctl", []string{"-p", "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf"}). + Return("", nil) + }, + validateFunc: func( + result *sysctl.CreateResult, + err error, + ) { + suite.NoError(err) + suite.True(result.Changed) + + content, readErr := suite.memFs.ReadFile( + "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", + ) + suite.NoError(readErr) + suite.Equal("net.ipv4.ip_forward = 1\n", string(content)) + }, + }, { name: "when key is empty", entry: sysctl.Entry{ @@ -335,31 +428,12 @@ func (suite *DebianPublicTestSuite) TestCreate() { }, }, { - name: "when previously undeployed allows create", + name: "when a previously undeployed key is created again", entry: sysctl.Entry{ Key: "net.ipv4.ip_forward", Value: "1", }, setup: func() { - content := []byte("net.ipv4.ip_forward = 1\n") - state := job.FileState{ - Path: "/etc/sysctl.d/osapi-net.ipv4.ip_forward.conf", - SHA256: computeTestSHA256(content), - Mode: "0644", - DeployedAt: "2026-01-01T00:00:00Z", - UndeployedAt: "2026-02-01T00:00:00Z", - Metadata: map[string]string{ - "key": "net.ipv4.ip_forward", - "value": "1", - }, - } - stateBytes, _ := json.Marshal(state) - mockEntry := jobmocks.NewMockKeyValueEntry(suite.ctrl) - mockEntry.EXPECT().Value().Return(stateBytes) - - suite.mockStateKV.EXPECT(). - Get(gomock.Any(), gomock.Any()). - Return(mockEntry, nil) suite.mockStateKV.EXPECT(). Put(gomock.Any(), gomock.Any(), gomock.Any()). Return(uint64(1), nil) @@ -382,9 +456,6 @@ func (suite *DebianPublicTestSuite) TestCreate() { Value: "1", }, setup: func() { - suite.mockStateKV.EXPECT(). - Get(gomock.Any(), gomock.Any()). - Return(nil, errors.New("not found")) suite.mockStateKV.EXPECT(). Put(gomock.Any(), gomock.Any(), gomock.Any()). Return(uint64(0), errors.New("kv put error")) @@ -405,9 +476,6 @@ func (suite *DebianPublicTestSuite) TestCreate() { Value: "1", }, setup: func() { - suite.mockStateKV.EXPECT(). - Get(gomock.Any(), gomock.Any()). - Return(nil, errors.New("not found")) sysctl.SetMarshalJSON(func(_ interface{}) ([]byte, error) { return nil, errors.New("marshal error") }) @@ -428,9 +496,6 @@ func (suite *DebianPublicTestSuite) TestCreate() { Value: "1", }, setup: func() { - suite.mockStateKV.EXPECT(). - Get(gomock.Any(), gomock.Any()). - Return(nil, errors.New("not found")) suite.mockStateKV.EXPECT(). Put(gomock.Any(), gomock.Any(), gomock.Any()). Return(uint64(1), nil)