From 3fd2814b799ed83def2c86057b0f05535b85e4ed Mon Sep 17 00:00:00 2001 From: Travis Johnson Date: Thu, 1 Jun 2023 16:56:42 -0600 Subject: [PATCH] fix(puller): fixes related to an empty model path (#52) #### Motivation Removes some buggy behavior related to the handling of an empty `modelPath` being passed into the puller #### Modifications - use `filepath.Join()` instead of manual string concatenation with the file separator to handle an empty `modelPathFilename` - in the http storageProvider, remove duplication of the `localPath` into the rendered path - just to not it: I think this bug has not been caught because using the HTTP provider with a `storageURI` means that the local path is empty - this will need to be fixed before https://github.com/kserve/modelmesh-serving/pull/382 can be merged - add check to ensure that the local path for files downloaded by the puller is always a path within the generated model dir (prevents the generated dir name from being the model file as was seen in https://github.com/kserve/modelmesh-runtime-adapter/issues/41#issuecomment-1564909111) - if no path is extracted from the request `ModelPath`, use `_model` by default #### Result Resolves: https://github.com/kserve/modelmesh-runtime-adapter/issues/41 Signed-off-by: Travis Johnson --- model-serving-puller/puller/puller.go | 22 +++++++++----- pullman/storageproviders/http/provider.go | 4 +-- .../storageproviders/http/provider_test.go | 29 +++++++++++++++++++ 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/model-serving-puller/puller/puller.go b/model-serving-puller/puller/puller.go index 785ef1dc..02a1e107 100644 --- a/model-serving-puller/puller/puller.go +++ b/model-serving-puller/puller/puller.go @@ -142,8 +142,16 @@ func (s *Puller) ProcessLoadModelRequest(ctx context.Context, req *mmesh.LoadMod // build and execute the pull command // name the local files based on the last element of the paths - // TODO: should have some sanitization for filenames - modelPathFilename := filepath.Base(req.ModelPath) + // or set a default if the ModelPath is empty or just /'s + var modelPathFilename string + switch basePath := filepath.Base(req.ModelPath); basePath { + case ".", string(filepath.Separator): + modelPathFilename = "_model" + default: + // TODO: should have some sanitization for filenames + modelPathFilename = basePath + } + targets := []pullman.Target{ { RemotePath: req.ModelPath, @@ -184,12 +192,10 @@ func (s *Puller) ProcessLoadModelRequest(ctx context.Context, req *mmesh.LoadMod } // update model path to an absolute path in the local filesystem - // commment out SecureJoin since it doesn't handle symlinks well - // modelFullPath, joinErr := util.SecureJoin(modelDir, modelPathFilename) - // if joinErr != nil { - // return nil, fmt.Errorf("Error joining paths '%s' and '%s': %w", modelDir, modelPathFilename, joinErr) - // } - modelFullPath := modelDir + string(filepath.Separator) + modelPathFilename + + // SecureJoin doesn't allow symlinks pointing outside the scope of the first element, which breaks PVC support since + // pullman will create a symlink to the mounted PVC in /pvc_mounts + modelFullPath := filepath.Join(modelDir, modelPathFilename) req.ModelPath = modelFullPath diff --git a/pullman/storageproviders/http/provider.go b/pullman/storageproviders/http/provider.go index c44b20a5..2d70d77a 100644 --- a/pullman/storageproviders/http/provider.go +++ b/pullman/storageproviders/http/provider.go @@ -160,9 +160,9 @@ func (r *httpRepository) Pull(ctx context.Context, pc pullman.PullCommand) error localPath = path.Base(pt.RemotePath) } - filePath, joinErr := util.SecureJoin(destDir, pt.LocalPath, localPath) + filePath, joinErr := util.SecureJoin(destDir, localPath) if joinErr != nil { - return fmt.Errorf("error joining filepaths '%s' and '%s': %w", pt.LocalPath, localPath, joinErr) + return fmt.Errorf("error joining filepaths '%s' and '%s': %w", destDir, localPath, joinErr) } r.log.V(1).Info("constructed local path to download file", "local", filePath, "remote", pt.RemotePath) diff --git a/pullman/storageproviders/http/provider_test.go b/pullman/storageproviders/http/provider_test.go index c1d4a137..b3e85efe 100644 --- a/pullman/storageproviders/http/provider_test.go +++ b/pullman/storageproviders/http/provider_test.go @@ -116,6 +116,35 @@ func Test_Download_SimpleFile(t *testing.T) { assert.NoError(t, err) } +func Test_Download_RenameFile(t *testing.T) { + _, _, testRepo, mockClient, _ := newTestMocks(t) + + testURL := "http://someurl:8080" + c := pullman.NewRepositoryConfig("http", nil) + c.Set("url", testURL) + + downloadDir := filepath.Join("test", "output") + inputPullCommand := pullman.PullCommand{ + RepositoryConfig: c, + Directory: downloadDir, + Targets: []pullman.Target{ + { + RemotePath: "models/some_model_file", + LocalPath: "local/path/my-model", + }, + }, + } + + expectedURL := testURL + "/models/some_model_file" + expectedFile := filepath.Join(downloadDir, "local", "path", "my-model") + mockClient.EXPECT().download(gomock.Any(), newHttpRequestMatcher("GET", expectedURL), gomock.Eq(expectedFile)). + Return(nil). + Times(1) + + err := testRepo.Pull(context.Background(), inputPullCommand) + assert.NoError(t, err) +} + func Test_GetKey(t *testing.T) { provider := httpProvider{}