fix: prevent false positives in already running k0s instance detection #5400
+207
−26
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import ( | |
| "github.com/k0sproject/k0s/internal/pkg/dir" | ||
| "github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1" | ||
| "github.com/k0sproject/k0s/pkg/constant" | ||
| "github.com/shirou/gopsutil/v4/process" | ||
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
|
||
|
|
@@ -97,9 +98,14 @@ func LoadRuntimeConfig(k0sVars *CfgVars) (*RuntimeConfigSpec, error) { | |
| // an error is returned, which allows the controller startup to proceed to | ||
| // initialize a new runtime config. | ||
| if spec.Pid != 0 { | ||
| isRunning, err := checkInstanceRunning(spec.Pid) | ||
| if !isRunning || err != nil { | ||
| defer func() { _ = spec.Cleanup() }() | ||
| if !isRunning { | ||
| return nil, errors.Join(ErrK0sNotRunning, err) | ||
| } else if err != nil { | ||
| return nil, fmt.Errorf("%w: failed to check if instance is running", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -192,3 +198,56 @@ func (r *RuntimeConfigSpec) Cleanup() error { | |
| } | ||
| return nil | ||
| } | ||
|
|
||
| // returns: | ||
| // - true, nil if the pid is the same k0s executable and running | ||
| // - false, nil if the pid is not running or another executable | ||
| // - false, error on any kind of internal error checking the status | ||
| // | ||
| // process does not exist -> error | ||
| // process exists and is same executable -> error | ||
| // process exists and is not same executable -> nil | ||
| func checkInstanceRunning(pid int) (bool, error) { | ||
| // create the process object | ||
| proc, err := process.NewProcess(int32(pid)) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to create process object: %w", err) | ||
| } | ||
|
|
||
| // first check if the pid is running | ||
| isRunning, err := proc.IsRunning() | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to check if process is running: %w", err) | ||
| } | ||
| if !isRunning { | ||
| return false, nil | ||
| } | ||
|
|
||
| // Get the executable path of the target pid | ||
| // - no need to resolve symlinks here, it is already a resolved path | ||
| exeTarget, err := proc.Exe() | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to get process executable path: %w", err) | ||
| } | ||
|
|
||
| // Get the executable path of the current process | ||
| currentExe, err := os.Executable() | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to get current executable: %w", err) | ||
| } | ||
|
|
||
| // Resolve symlinks | ||
| currentExe, err = filepath.EvalSymlinks(currentExe) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to resolve current executable symlinks: %w", err) | ||
| } | ||
|
|
||
| // check that the executable is the same in order to issue an error, if it would be a different | ||
| // than it can't be a running instance of k0s, which the check is all about | ||
| if exeTarget != currentExe { | ||
|
There was a problem hiding this comment. There's There was a problem hiding this comment. If we take this approach, there is a chance that another running copy of the k0s binary won't be detected as already running. Say k0s was started by the init system and is running in the background. Then someone grabs a fresh binary and runs |
||
| return false, nil | ||
| } | ||
|
|
||
| // return that k0s is running witht he same executable image | ||
| return true, nil | ||
| } | ||
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather hardcode something like
os.Stat(filepath.Join("/proc", pid, "exe"))here rather than pulling in a rather large dependency. This wouldn't work on Windows, but I'd revisit this another time. Ifruntime.GOOSis not"linux", you could simply skip this function call, or return successful unconditionally, or return an error that will unwrap toerrors.ErrUnsupportedand check for this on the call sites ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
os.Args[0]would be a platform-agnostic alternative?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually my first PR commit had just a linux impl using the procfs. @twz123 the reason for this using procfs or something else to find the executable path, is actually that you need to find the executable path of a foreign pid too, not just your own.
I also think it would be much better to just implement something for Linux, or maybe even drop the PR completely, if you plan to change the runtime config parts anyway soon. Here was the original minimal change to checkPid that will only work on Linux:
e19014e#diff-406aeff8e9fcab83e1f62a534a0b5f9292b2cad0bf7d874a526fc15a9f6e8fe8L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... of course 🤦♂️
Not sure about the soon part. It's definitely something worth doing, but that issue is lying around for years already, and is not really on the roadmap. You could try to tackle it, If you're interested! OTOH, we could also try to get this PR merged in the meantime.