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

Proxy endpoint with path enhancement #146

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jaiswal-sumit
Copy link
Contributor

Motivation

  • Please give a brief description for the background of this change.
    In the case of proxy-endpoint at our side which we maintain and route the data tunnel request via our proxy-endpoint.
    We introduced two type of routing on our proxy-endpoint service
  1. subdomain based routing
  2. subpath based routing

Example:
subdomain based routing: tenant-datatunnel.ourdomain.com request will be routed to data.tunneling.iot.eu-west-1.amazonaws.com
subpath based routing: ourdomain.com/tenant-datatunnel request will be routed to data.tunneling.iot.eu-west-1.amazonaws.com

we found subpath based routing does works with localproxy as it tries to bind HOST with complete url to 443
[info] Attempting to establish web socket connection with endpoint wss://ourdomain.com/tenant-datatunnel:443

Hence this PR is to enhance localproxy code so it can work with subpath based request and for this added new arg variable (-p [ --endpoint-path ] arg (=/tunnel) )
./localproxy -d 22 -v 6 -e ourdomain.com -p "/tenant-datatunnel/tunnel"
With this it able to bind HOST correctly like this wss://ourdomain.com:443 and make call to GET /tenant-datatunnel/tunnel?local-proxy-mode=source HTTP/1.1 to the HOST

This change will work for all types of connection and does not contain any breaking change. -p is Made option with default value of "/tunnel".

Modifications

Change summary

3 files are modified.
main.cpp to handle -p argument value
LocalProxyConfig.h to carry path value
TcpAdapterProxy.cpp to use path variable for making GET call to service

Testing

**Is your change tested?
Yes, tested with following commands arguments for both source and destination

 -v 6 -e ourdomain.com -p "/tenant-datatunnel/tunnel"
-v 6 -e tenant-datatunnel.ourdomain.com
-v 6 -r eu-west-1

Please list your testing steps and test results.

  • CI test run result: NA

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jaiswal-sumit
Copy link
Contributor Author

@HarshGandhi-AWS & @RogerZhongAWS please check this enhancement. this is working in our integration test and need for our customer to use in this way. As mentioned in detail description it does not break any functionality and in normal condition it works well. @abeytr Kindly put your notes on integration test result.

/**
* The reverse proxy tunnel endpoint path.
*/
std::string url_path { };
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add details about this in readme?

@@ -161,6 +161,7 @@ bool process_cli(int argc, char ** argv, LocalproxyConfig &cfg, ptree &settings,
("access-token,t", value<string>()->required(), "Client access token")
("client-token,i", value<string>(), "Optional Client Token")
("proxy-endpoint,e", value<string>(), "Endpoint of proxy server with port (if not default 443). Example: data.tunneling.iot.us-east-1.amazonaws.com:443")
("endpoint-path,p", value<string>()->default_value("/tunnel"), "Endpoint path of proxy server if need extra path. Example: reverse-proxy.domain.com/aws-data-service/tunnel, so you connect with -e reverse-proxy.domain.com -p /aws-data-service/tunnel")
Copy link
Contributor

Choose a reason for hiding this comment

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

please set the endpoint default value in an global variable in header file and set it as default over there. It would be easier to maintain it that way. Also add about this default endpoint value in readme.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ig15
Copy link
Contributor

ig15 commented Sep 25, 2024

@jaiswal-sumit Can you address the open comments so that we can move ahead with the request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants