search

kserve / modelmesh-runtime-adapter

Unified runtime-adapter image of the sidecar containers which run in the modelmesh pods
Apache License 2.0
21 stars 59 forks source link

Restore SecureJoin logic to puller #46

Closed RobGeada closed 1 year ago

RobGeada commented 1 year ago

@ckadner

Motivation

Replacing the SecureJoin from the Puller broke the loading of HTTP models, see the corresponding issue.

Modifications

Reverted the removal of the SecureJoin from merge #36:

From Merge 36:

modelFullPath := modelDir + string(filepath.Separator) + modelPathFilename

New: Original and Restored:

modelFullPath, joinErr := util.SecureJoin(modelDir, modelPathFilename)
if joinErr != nil {
    return nil, fmt.Errorf("Error joining paths '%s' and '%s': %w", modelDir, modelPathFilename, joinErr)
}

Result

HTTP models load as expected; tested via a direct comparison of building the current main image versus the image built from this PR in a deployment of opendatahub, using an HTTP model inference service. The main image displayed the same error raised in this issue, the one built from this PR worked fine.

Note

This returns the puller to its pre-merge 46 state, however, that PR did mention that the SecureJoin does not handle symlinks well; this PR does not address that concern.

kserve-oss-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RobGeada To complete the pull request process, please assign njhill after the PR has been reviewed. You can assign the PR to them by writing /assign @njhill in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kserve/modelmesh-runtime-adapter/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
RobGeada commented 1 year ago

@ckadner Any updates on this?

tjohnson31415 commented 1 year ago

Hi @RobGeada. I have started looking into this issue with the HTTP and PVC storage providers. The problem with this current PR is that it fixes HTTP but breaks PVC (the PVC impl uses symlinks). In my investigation, I found that there is a bug with the use of HTTP storage; I'm working on a fix for that. Once that bug is fixed, I think both HTTP and PVC support can work without changing the code here back to SecureJoin.

I will get a PR up with the fix soon.

ckadner commented 1 year ago

Thank you @tjohnson31415 -- just to tie it all together, your PR to address the same issue #41 is this one on the modelmesh-serving repo: https://github.com/kserve/modelmesh-serving/pull/382

tjohnson31415 commented 1 year ago

I ended up finding a few different bugs to fix as I went through the implementation. The PR that I consider to be the resolution for issue #41 is https://github.com/kserve/modelmesh-runtime-adapter/pull/52

I had thought that the change in https://github.com/kserve/modelmesh-serving/pull/382 would be a fix, but it requires additional changes captured in https://github.com/kserve/modelmesh-runtime-adapter/pull/52.