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(clustering) support http forward proxy #9758

Merged
merged 25 commits into from
Nov 17, 2022
Merged

Conversation

fffonion
Copy link
Contributor

@fffonion fffonion commented Nov 15, 2022

Summary

Allow Dp -> Cp connection to go through a HTTP proxy tunnel. Only HTTP proxy is supported for now.

Things to do after feature frezze before code freeze:

  • Make our patch of lua-resty-websocket.client a patch instead of vendoring
  • Add support of talk to an HTTP tunnel in https

Full changelog

  • Add forwrd proxy support for clustering data plane

Issue reference

FTI-2996

@fffonion
Copy link
Contributor Author

fffonion commented Nov 15, 2022

For reviewers:

  • patch of websocket.client is done in a seperate commit
  • tests is coming
  • will move the vendored websocket.client to a patch once CI pass and review is good

@github-actions github-actions bot added the chore Not part of the core functionality of kong, but still needed label Nov 15, 2022
kong/clustering/utils.lua Outdated Show resolved Hide resolved
@@ -0,0 +1,440 @@
-- Copyright (C) Yichun Zhang (agentzh)
Copy link
Contributor

Choose a reason for hiding this comment

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

We now have our own fork of lua-resty-websocket for any of our extra features that haven't made it upstream yet.

I think we should try to get this change into the fork and then ship it with OSS (currently it's only packaged in EE, but it is trivial to add to OSS). It is currently installed by kong-build-tools in-place of resty.websocket, so any changes/additions just need to be backwards-compatible, but this hasn't proved to be too much of a challenge yet. Feel free to @/ping me for help and/or code review if needed.

If it's not feasible to do make this change before code freeze, would you be willing to do so in a follow-up PR? I'd like us to not have to maintain multiple versions of this library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flrgh exactly, vendoring this is not a good idea. RIght now i'm vendoring it so I can test it in a single PR (and
it's easier to tell what did we change).
We already have patch for lua-resty-websocket, so adding another one won't bring more burden. So yes I will make this a proper patch and remove the vendoring
after feature freeze.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have patch for lua-resty-websocket, so adding another one won't bring more burden.

To explain why I'm pushing for Kong/lua-resty-websocket over openresty-patches approach:

Any build that bundles Kong/lua-resty-websocket (currently just EE) overwrites the entire resty/websocket tree after OpenResty patches are applied. The reason this doesn't cause any breakage in EE today is because Kong/lua-resty-websocket was forked from OpenResty/lua-resty-websocket:master, so it already contains the unreleased client mtls commit(s).

What this means is that any new features/patches added to openresty-patches in kong-build-tools that don't already exist upstream must be ported to be Kong/lua-resty-websocket, or there will be breakage in EE. So it puts us in the place of needing to maintain separate patches in two different repositories against codebases that are already diverging over time.


An aside: the reason for overwriting resty.websocket rather than using our own namespace (i.e. kong.resty.websocket) is historical. I don't believe there's anything today stopping us from namespacing it and shipping it via luarocks rather than the clunky KBT install process, if we so choose. There just hasn't really been any friction with the current process until now.

@hbagdi
Copy link
Member

hbagdi commented Nov 15, 2022

Please make sure to add a kong.conf.default entry as part of this PR.

@fffonion fffonion force-pushed the feat/hybrid-forward-proxy branch 7 times, most recently from ebf99ed to 93b69dc Compare November 16, 2022 07:33
@github-actions github-actions bot removed the chore Not part of the core functionality of kong, but still needed label Nov 16, 2022
@fffonion fffonion force-pushed the feat/hybrid-forward-proxy branch 2 times, most recently from 6b39a40 to c222d1d Compare November 16, 2022 09:10
@fffonion fffonion added this to the 3.1 milestone Nov 16, 2022
kong/clustering/utils.lua Outdated Show resolved Hide resolved
kong/tools/utils.lua Outdated Show resolved Hide resolved
kong/tools/utils.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

Should we add proxy_server and cluster_use_proxy in spec/fixtures/custom_nginx.template?

@fffonion
Copy link
Contributor Author

@chronolaw proxy_server and cluster_use_proxy are kong.conf entries and won't affect the nginx template
(for now).

kong.conf.default Outdated Show resolved Hide resolved
Co-authored-by: Datong Sun <[email protected]>
Copy link
Member

@dndx dndx left a comment

Choose a reason for hiding this comment

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

Approved with minor style recommendation, and can be merged after they have been addressed and tests are green.

We need to remove the copied code from lua-resty-webaocket still, but it is decided to do that in a separate PR.

kong/clustering/utils.lua Outdated Show resolved Hide resolved
kong/clustering/utils.lua Show resolved Hide resolved
@fffonion fffonion merged commit 0fb15c0 into master Nov 17, 2022
@fffonion fffonion deleted the feat/hybrid-forward-proxy branch November 17, 2022 08:48
fffonion pushed a commit that referenced this pull request Nov 22, 2022
This adds our lua-resty-websocket fork as a dependency and removes
the vendored resty.websocket.client module that was added in #9758.
oowl pushed a commit that referenced this pull request Aug 15, 2024
Currently, the connect_timeout/read_timeout/send_timeout would
all set to be `null` if the deprecated timeout is explicitly
set to `null`. This commit fixes it.

FTI-6110
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.

5 participants