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

feat(balancer): refresh uri and tls scheme in ai-proxy-advanced balancer phase #14007

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

oowl
Copy link
Member

@oowl oowl commented Dec 11, 2024

Summary

  1. feat(patch): backport balancer.set_upstream_tls feature from openresty upstream feature: add balancer.set_upstream_tls(on). openresty/lua-resty-core#460
  2. feat(patch): refresh upstream uri variable when proxy pass balancer recreate
  3. feat(pdk): dynamic control upstream tls when kong.service.request.set_scheme was called
  4. feat(llm): recreate http request header and body when balancer retry

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

AG-183

kong/llm/plugin/base.lua Outdated Show resolved Hide resolved
@oowl oowl force-pushed the balancer-patch branch 4 times, most recently from 3f51327 to 65d9a31 Compare December 20, 2024 06:22
+#endif
+
+ } else {
+ if (ngx_http_proxy_eval(r, ctx, plcf) != NGX_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ADD-SP this would need some performance test to validate

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the performance impact here, is there a way to detect if the upstream tls option has been changed, and then decide if we need to do the config parsing again here? e.g. could we use ctx to mark if the options has been changed from lua side, and if not changed, we just skip the refresh of variables here?

Copy link
Member

@Oyami-Srk Oyami-Srk Jan 10, 2025

Choose a reason for hiding this comment

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

I don't think this patch is used to alter the TLS option. It looks like "reevaluating URI every time we make requests".
IIUC, before this patch, evaluating URI will perform only once when the config is processed and use evaluated value after that. Thus, altering some variables in URI will not be able to work normally.
I'm not sure if I understand correctly, Please correct me if I'm wrong. @oowl

Copy link
Member

@Oyami-Srk Oyami-Srk Jan 15, 2025

Choose a reason for hiding this comment

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

Just confirmed "feat(patch): refresh upstream uri variable when proxy pass balancer recreate" will cause regressions in RPS, latency and DP memory usage in change-route scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Oyami-Srk The option to alter TLS on/off is from another commit in this PR. The change here is to alter upstream_uri,
as kong uses a variable in proxy_pass, this patch make sure in a failover when we update the upstream_uri, the variable is re-evaluated so that nginx will build an updated request on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirmed "feat(patch): refresh upstream uri variable when proxy pass balancer recreate" will cause regressions in RPS, latency and DP memory usage in change-route scenario.

@oowl I guess what we can do next to make sure this ngx_http_proxy_eval is only called the second time (aka in a failover) in a request.

@ProBrian ProBrian self-requested a review January 17, 2025 01:49
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.

4 participants