fix: replace os.Exit(1) with error returns, fix /readyz, harden CopyFile#67
Open
tuxerrante wants to merge 1 commit into
Open
fix: replace os.Exit(1) with error returns, fix /readyz, harden CopyFile#67tuxerrante wants to merge 1 commit into
tuxerrante wants to merge 1 commit into
Conversation
…th to CopyFile Replace os.Exit(1) calls in compareLocalFiles, areProfilesReadable, and CopyFile with proper error returns. This shifts from fail-crash to fail-graceful behavior, allowing the poller to retry on the next cycle instead of crashing the entire DaemonSet pod. Error propagation has been verified through the full call chain: compareLocalFiles → HasTheSameContent → calculateProfileChanges → loadNewProfiles → pollNow areProfilesReadable → getNewProfiles → loadNewProfiles → pollNow CopyFile → loadProfile → loadNewProfiles → pollNow Additional fixes: - Add isSafePath validation at CopyFile entry point (defense in depth before os.Stat on unvalidated source path) - Fix /readyz endpoint: add else branch for len(desired) != len(loaded) and explicit nil-desired handling. Previously returned an empty 200 response (false positive readiness) when profiles couldn't be read. Test updates: - Un-skip TestGetNewProfiles_NonexistentDir (was skipped because of os.Exit) - Add TestAreProfilesReadable_NonexistentDir - Add TestCompareLocalFiles_ReadError - Add Test_CopyFile_UnsafePath and Test_CopyFile_NonexistentSource - Fix test temp dir paths to use /tmp for isSafePath compatibility Made-with: Cursor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR shifts several filesystem/profile operations from process-terminating failures (os.Exit(1)) to error-returning behavior, fixes a /readyz false-positive scenario, and adds additional path hardening and test coverage to support graceful retries in the poll loop.
Changes:
- Replace
os.Exit(1)with error/false returns incompareLocalFiles,areProfilesReadable, andCopyFile. - Fix
/readyzreadiness logic to return 503 when desired vs loaded profiles are out of sync (and when desired profiles can’t be read). - Expand unit tests to cover newly reachable error paths and validate
CopyFilepath safety behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/filesystemOperations.go | Converts fatal exits to returned errors/false; adds isSafePath guard at CopyFile entry and improves error wrapping. |
| src/app/healthz.go | Updates /readyz to avoid silent 200s when desired/loaded mismatch or desired profiles aren’t readable. |
| src/app/t_profiles_ops_test.go | Un-skips and updates nonexistent-dir test to assert graceful behavior. |
| src/app/t_hasTheSameContent_test.go | Adds tests for areProfilesReadable error path and HasTheSameContent read error path. |
| src/app/t_copy_test.go | Updates copy tests to use /tmp for isSafePath; adds unsafe-path and stat-error coverage. |
| src/app/t_profiles_test.go | Updates temp directory creation to ensure isSafePath-allowed paths for load/unload tests. |
| src/app/t_main_extended_test.go | Updates loadProfile success test temp path to comply with isSafePath. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
17
to
+33
| http.HandleFunc("/readyz", func(w http.ResponseWriter, r *http.Request) { | ||
| _, desired := getNewProfiles(cfg) | ||
| _, loaded, _ := getLoadedProfiles(cfg) | ||
|
|
||
| inSync := true | ||
| if desired == nil { | ||
| w.WriteHeader(http.StatusServiceUnavailable) | ||
| w.Write([]byte("NOT_READY: unable to read desired profiles")) | ||
|
|
||
| if len(desired) == len(loaded) { | ||
| for profile := range desired { | ||
| if !loaded[profile] { | ||
| inSync = false | ||
| return | ||
| } | ||
|
|
||
| break | ||
| } | ||
| } | ||
| if len(desired) != len(loaded) { | ||
| w.WriteHeader(http.StatusServiceUnavailable) | ||
| w.Write([]byte("NOT_READY")) | ||
|
|
||
| return | ||
| } |
Comment on lines
+175
to
+180
| tmp := makeSafeDirForTest(t) | ||
| existing := filepath.Join(tmp, "existing.profile") | ||
| os.WriteFile(existing, []byte(testProfileData), 0o644) | ||
|
|
||
| missing := filepath.Join(tmp, "missing.profile") | ||
| _, err := HasTheSameContent(nil, existing, missing) |
Comment on lines
+55
to
+64
| func Test_CopyFile_NonexistentSource(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| err := CopyFile("/tmp/nonexistent-file-abc123", "/tmp/dst") | ||
| if err == nil { | ||
| t.Fatal("expected error for nonexistent source") | ||
| } | ||
| if !strings.Contains(err.Error(), "stat source") { | ||
| t.Errorf("expected stat error, got: %v", err) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
os.Exit(1)with proper error returns incompareLocalFiles,areProfilesReadable, andCopyFile. This shifts from fail-crash to fail-graceful, letting the poller retry on the next cycle instead of crashing the DaemonSet pod./readyzendpoint silent false-positive: previously returned empty 200 whenlen(desired) != len(loaded)(noelsebranch). Now returns 503 with clear status message.isSafePathvalidation atCopyFileentry — defense in depth beforeos.Staton the source path.TestGetNewProfiles_NonexistentDir, add error path coverage forareProfilesReadable,compareLocalFiles, andCopyFile.Security Context
This application runs as a privileged DaemonSet managing kernel AppArmor profiles. The
/readyzbug meant kubelet could mark the pod as "ready" when it couldn't read its profile source — a false-positive readiness signal for a security control.Error propagation verified through the full call chain:
Test plan
TestGetNewProfiles_NonexistentDir— un-skipped, validates(false, nil)returnTestAreProfilesReadable_NonexistentDir— new test for ReadDir error pathTestCompareLocalFiles_ReadError— new test for file read error pathTest_CopyFile_UnsafePath— validates isSafePath rejectionTest_CopyFile_NonexistentSource— validates stat error returnMade with Cursor