-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
83b9d80
to
6ee6134
Compare
-- @treturn[2] nil | ||
-- @treturn[2] string An error message if instantiation failed | ||
function _M.new_driver(conf, http_opts) | ||
local self = { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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).
Are there any more changes requested on this PR? @Tieske @liverpool8056 |
The object initializer (new) would set properties on the parent instead of on the instance.
Probably out scope of this PR, but just want to share for visibility: using of |
@fffonion I think that makes sense as a follow up. For now, I think we should merge this as is. |
There was a problem hiding this 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.
Cherry-pick failed for 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 |
Backport failed for 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 |
Cherry-pick failed for 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 |
Hi @Tieske , the auto backport and cherry-pick failed, could you please manually create the backport and cherry-pick PRs? Thanks. |
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
changelog/unreleased/kong
orskip-changelog
label added on PR if changelog is unnecessary. README.mdIssue reference
Internal ticket: AG-44