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 kserve/modelmesh-serving#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 #41 (comment))
    - if no path is extracted from the request `ModelPath`, use `_model` by default

#### Result

Resolves: #41


Signed-off-by: Travis Johnson <[email protected]>

Author: tjohnson31415

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
 

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)
 

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{}