Skip to content

fix(sysctl): decide from the drop-in, not from the record #567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
retr0h merged 1 commit into from
Oct 1, 2026
Merged

fix(sysctl): decide from the drop-in, not from the record #567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 8 additions & 19 deletions internal/provider/node/sysctl/debian.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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")
}

Expand Down
155 changes: 110 additions & 45 deletions internal/provider/node/sysctl/debian_public_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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{
Expand Down Expand Up @@ -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)
Expand All @@ -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"))
Expand All @@ -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")
})
Expand All @@ -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)
Expand Down
Loading