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

fix(ai-proxy): prevent shared state #13028

Merged
merged 5 commits into from
Jun 6, 2024
Merged

fix(ai-proxy): prevent shared state #13028

merged 5 commits into from
Jun 6, 2024

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented May 14, 2024

The object initializer (new) would set properties on class instead of on the instance. Also cleans up some code.

I have added comments on the actual bug, everything else is cosmetic/cleanup. Please review with whitespace changes disabled in the github UI.

Summary

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Internal ticket: AG-44

-- @treturn[2] nil
-- @treturn[2] string An error message if instantiation failed
function _M.new_driver(conf, http_opts)
local self = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment to reviewers: self was pointing to _M in the old code. So setting self.conf, self.http_opts, and self.driver would actually modify the class, and not the instance.

if not ai_driver then
return internal_server_error(err)
end

-- if asked, introspect the request before proxying
kong.log.debug("introspecting request with LLM")
local new_request_body, err = llm:ai_introspect_body(
local new_request_body, err = ai_driver:ai_introspect_body(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment to reviewers: The old code would call on llm (the class) instead of on ai_driver (the instance).

@@ -123,7 +129,7 @@ function _M:access(conf)
-- if asked, introspect the request before proxying
kong.log.debug("introspecting response with LLM")

local new_response_body, err = llm:ai_introspect_body(
local new_response_body, err = ai_driver:ai_introspect_body(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment to reviewers: The old code would call on llm (the class) instead of on ai_driver (the instance).

@Tieske Tieske marked this pull request as ready for review May 15, 2024 12:57
@Tieske Tieske requested a review from tysoekong May 15, 2024 13:11
@subnetmarco
Copy link
Member

Are there any more changes requested on this PR? @Tieske @liverpool8056

@fffonion
Copy link
Contributor

fffonion commented Jun 5, 2024

Probably out scope of this PR, but just want to share for visibility: using of kong.ctx.shared.X to shared between plugins is
not very robust way as a single typo in the variable name X would cause logic to fail silently. Especially when the ai plugins
have lots of branchs and not easy to cover all in tests.
Ideally they should also be moved to a proper function call like llm.state.getX() llm.state.setX() as then a typo will cause a hard failure in error log. I started a small refactor of this as part of this PR 9f511ab.
After seeing this PR, it might make sense to let the kong.llm.state module store things in the instance (is it per request?).

@Tieske
Copy link
Member Author

Tieske commented Jun 5, 2024

@fffonion I think that makes sense as a follow up. For now, I think we should merge this as is.

Copy link
Contributor

@outsinre outsinre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve with minor suggestion.

@fffonion fffonion merged commit 18a189b into master Jun 6, 2024
25 checks passed
@fffonion fffonion deleted the fix/ai-proxy branch June 6, 2024 15:37
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13028-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13028-to-master-to-upstream
git checkout -b cherry-pick-13028-to-master-to-upstream
ancref=$(git merge-base 3df996a3ac14f38ba9e9d354aea37f1a08f530b9 ed0b56680cb0ec2eb9c264b34525f6ce30a07cb5)
git cherry-pick -x $ancref..ed0b56680cb0ec2eb9c264b34525f6ce30a07cb5

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Jun 6, 2024
@team-gateway-bot
Copy link
Collaborator

Backport failed for release/3.7.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release/3.7.x
git worktree add -d .worktree/backport-13028-to-release/3.7.x origin/release/3.7.x
cd .worktree/backport-13028-to-release/3.7.x
git switch --create backport-13028-to-release/3.7.x
git cherry-pick -x 18a189baff77ed23a32ec7a01d45d116aa6d6fce

@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13028-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13028-to-master-to-upstream
git checkout -b cherry-pick-13028-to-master-to-upstream
ancref=$(git merge-base 3df996a3ac14f38ba9e9d354aea37f1a08f530b9 ed0b56680cb0ec2eb9c264b34525f6ce30a07cb5)
git cherry-pick -x $ancref..ed0b56680cb0ec2eb9c264b34525f6ce30a07cb5

@AndyZhang0707
Copy link
Collaborator

Hi @Tieske , the auto backport and cherry-pick failed, could you please manually create the backport and cherry-pick PRs? Thanks.

Tieske added a commit that referenced this pull request Jun 7, 2024
The object initializer (new) would set properties on class instead of on the instance. Also cleans up some code.

AG-44

(cherry picked from commit 18a189b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants