Skip to content
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.

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

refactor: remove engine binary path check #435

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 2 additions & 51 deletions pkg/process/process_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"crypto/sha256"
"encoding/hex"
"fmt"
"path/filepath"
"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -175,44 +174,6 @@ func isEngineProcess(p *Process) bool {
return p.PortCount == DefaultEnginePortCount
}

func decodeProcessPath(path string) (dir, image, binary string) {
path, binary = filepath.Split(filepath.Clean(path))
dir, image = filepath.Split(filepath.Clean(path))
return dir, image, binary
}

func isValidBinary(binary string) bool {
switch binary {
case "longhorn":
return true
default:
return false
}
}

func isValidDirectory(dir string) bool {
switch dir {
case "/engine-binaries/", "/host/var/lib/longhorn/engine-binaries/":
return true
default:
return false
}
}

func ensureValidProcessPath(path string) (string, error) {
dir, image, binary := decodeProcessPath(path)
logrus.Debugf("Process Manager: validate process path: %v dir: %v image: %v binary: %v", path, dir, image, binary)
if !isValidBinary(binary) {
return "", fmt.Errorf("unsupported binary %v", binary)
}

if !isValidDirectory(dir) {
return "", fmt.Errorf("unsupported process path %v", path)
}

return filepath.Join(dir, image, binary), nil
}

// ProcessCreate will create a process according to the request.
// If the specified process name exists already, the creation will fail.
func (pm *Manager) ProcessCreate(ctx context.Context, req *rpc.ProcessCreateRequest) (ret *rpc.ProcessResponse, err error) {
Expand All @@ -226,14 +187,9 @@ func (pm *Manager) ProcessCreate(ctx context.Context, req *rpc.ProcessCreateRequ
return nil, err
}

processPath, err := ensureValidProcessPath(req.Spec.Binary)
if err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}

p := &Process{
Name: req.Spec.Name,
Binary: processPath,
Binary: req.Spec.Binary,
Args: req.Spec.Args,
PortCount: req.Spec.PortCount,
PortArgs: req.Spec.PortArgs,
Expand Down Expand Up @@ -487,11 +443,6 @@ func (pm *Manager) ProcessReplace(ctx context.Context, req *rpc.ProcessReplaceRe
}
terminateSignal := syscall.SIGHUP

processPath, err := ensureValidProcessPath(req.Spec.Binary)
if err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}

logrus.Infof("Process Manager: prepare to replace process %v", req.Spec.Name)
logger, err := util.NewLonghornWriter(req.Spec.Name, pm.logsDir)
if err != nil {
Expand All @@ -500,7 +451,7 @@ func (pm *Manager) ProcessReplace(ctx context.Context, req *rpc.ProcessReplaceRe

p := &Process{
Name: req.Spec.Name,
Binary: processPath,
Binary: req.Spec.Binary,
Args: req.Spec.Args,
PortCount: req.Spec.PortCount,
PortArgs: req.Spec.PortArgs,
Expand Down
56 changes: 0 additions & 56 deletions pkg/process/process_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,62 +286,6 @@ func (s *TestSuite) TestProcessReplaceDuringDeletion(c *C) {
wg.Wait()
}

func (s *TestSuite) TestProcessInvalidProcessBinary(c *C) {

invalidBinaries := []string{
"/usr/bin/echo",
"/engine-binaries/invalid-binary",
"/host/var/lib/longhorn/engine-binaries/invalid-binary",
"/host/var/lib/longhorn/../longhorn/engine-binaries//../engine-binaries/valid-image/invalid-binary",
}
invalidPaths := []string{
"longhorn",
"/longhorn",
"/engine-binaries/invalid/image/longhorn",
"/engine-binaries/longhorn", // missing required image folder
"/invalid/path/longhorn",
}
invalidProcess := append(invalidBinaries, invalidPaths...)

// invalid process creation
for i, binary := range invalidProcess {
name := "test_invalid_process-" + strconv.Itoa(i)
createReq := &rpc.ProcessCreateRequest{
Spec: createProcessSpec(name, binary),
}
createResp, err := s.pm.ProcessCreate(nil, createReq)
c.Assert(createResp, IsNil)
c.Assert(err, NotNil)
c.Assert(status.Code(err), Equals, codes.InvalidArgument)
}

// valid process creation
name := "test_valid_process"
assertProcessCreation(c, s.pm, name, TestBinary)
defer func() {
// verify that the original process is not impacted by an invalid process replace call
assertProcessDeletion(c, s.pm, name)
deleted, err := waitForProcessListState(s.pm, func(processes map[string]*rpc.ProcessResponse) bool {
_, exists := processes[name]
return !exists
})
c.Assert(err, IsNil)
c.Assert(deleted, Equals, true)
}()

// invalid process replacement
for _, binary := range invalidProcess {
replaceReq := &rpc.ProcessReplaceRequest{
Spec: createProcessSpec(name, binary),
TerminateSignal: "SIGHUP",
}
rsp, err := s.pm.ProcessReplace(nil, replaceReq)
c.Assert(rsp, IsNil)
c.Assert(err, NotNil)
c.Assert(status.Code(err), Equals, codes.InvalidArgument)
}
}

func assertProcessReplace(c *C, pm *Manager, name, binary string) {
replaceReq := &rpc.ProcessReplaceRequest{
Spec: createProcessSpec(name, binary),
Expand Down