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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] Use proxy exclusion instead of a separate proxied client #1039

Open
2 tasks done
NarsGNA opened this issue May 3, 2023 · 7 comments · May be fixed by #2028 or #6957
Open
2 tasks done

[FEATURE] Use proxy exclusion instead of a separate proxied client #1039

NarsGNA opened this issue May 3, 2023 · 7 comments · May be fixed by #2028 or #6957
Assignees
@SanchithHegde
Labels
A-framework Area: Framework C-refactor Category: Refactor good first issue Good for newcomers help wanted Extra attention is needed

Comments

@NarsGNA
Copy link
Contributor

NarsGNA commented May 3, 2023

Feature Description

Currently we provide the option for proxying all external requests made to Connectors. We also use reqwest clients to make API calls to basilisk/locker or other utilities.

Recently we started reusing the reqwest::Client (via once_cell) for performance reasons
Our current approach involves maintaining 2 clients:

  • Proxied Client for any requests going outside our system
  • Non-Proxied client for internal services

Currently we need to maintain a list of the services that should bypass proxy & choose a client accordingly. This adds a bit of a boilerplate whenever we try to make requests.

Possible Implementation

Expected behavior:

Instead we want to push this behavior down to the reqwest library…
we believe the no_proxy function should help us avoid this maintenance.

We would ideally accept a list of proxy_bypass_urls as part of our config which would then be used to create a single reqwest::Client which can be used for internal + external services

Have you spent some time to check if this feature request has been raised before?

  • I checked and didn't find similar issue

Have you read the Contributing Guidelines?

Are you willing to submit a PR?

No, I don't have time to work on this right now
@NarsGNA NarsGNA added C-feature Category: Feature request or enhancement S-awaiting-triage Status: New issues that have not been assessed yet labels May 3, 2023
@NarsGNA NarsGNA changed the title [FEATURE] Use Proxy exclusion instead of a separate proxy [FEATURE] Use proxy exclusion instead of a separate proxied client May 3, 2023
@NarsGNA NarsGNA added the good first issue Good for newcomers label May 3, 2023
@EliKalter
Copy link
Contributor

Hey everyone, I just started contributing very recently, I see this issue is tagged "good first issue", unfortunately there is a lot of knowledge I don't have that is needed to solve this issue,
Can someone please give me some guidance in the form of a roadmap + relevant links if possible for me to follow and learn in order to be able to understand and hopefully even contribute on this type of issue in the future?
To clarify, I don't know what a lot of basic terms mean, such as: "proxying external requests", "proxying", "connectors", "reqwest client", "basilisk", "locker" etc etc, but, I hope you'll be able to prioritize the more basic and important terms at first.
Thanks a lot!.

@SanchithHegde
Copy link
Member

Hey @EliKalter, I see that you're already assigned to #899 and are working on it. For now, we'll leave this issue open to any other interested contributors. You may pick this up once your work on #899 is completed, and if no other contributors have expressed interest on this. I hope that's fine by you.

Can someone please give me some guidance in the form of a roadmap + relevant links if possible for me to follow and learn in order to be able to understand and hopefully even contribute on this type of issue in the future?

Once assigned you can connect with a mentor in our Discord server for guidance on fixing the issue or implementation specific doubts.

To clarify, I don't know what a lot of basic terms mean, such as: "proxying external requests", "proxying", "connectors", "reqwest client", "basilisk", "locker" etc etc, but, I hope you'll be able to prioritize the more basic and important terms at first.

To explain the issue in more detail, we use a proxy to make external requests (there are a number of reasons people do this). However, we don't want to use proxy for some internal services (for example, our card locker):

hyperswitch/crates/router/src/services/api/client.rs

Lines 113 to 128 in 36cc13d

pub(super) fn proxy_bypass_urls(locker: &Locker) -> Vec<String> {
let locker_host = locker.host.to_owned();
let basilisk_host = locker.basilisk_host.to_owned();
vec![
format!("{locker_host}/cards/add"),
format!("{locker_host}/cards/retrieve"),
format!("{locker_host}/cards/delete"),
format!("{locker_host}/card/addCard"),
format!("{locker_host}/card/getCard"),
format!("{locker_host}/card/deleteCard"),
format!("{basilisk_host}/tokenize"),
format!("{basilisk_host}/tokenize/get"),
format!("{basilisk_host}/tokenize/delete"),
format!("{basilisk_host}/tokenize/delete/token"),
]
}

You can check the current implementation of the client, which we want to refactor with the no_proxy functionality.

To understand what a "connector" is, you can refer to our connector integration guide. As for "basilisk" and "locker", they are just some internal services which will be open-sourced as separate repositories once they are stable.

@SanchithHegde SanchithHegde added A-framework Area: Framework S-unassigned Status: This issue has no one assigned to address it C-refactor Category: Refactor help wanted Extra attention is needed and removed S-awaiting-triage Status: New issues that have not been assessed yet C-feature Category: Feature request or enhancement labels May 3, 2023
@divinenaman
Copy link
Contributor

Hey, can I work on this ?

@lsampras
Copy link
Member

Hey @divinenaman,
Sure you can take a stab at it,

I'd also recommend taking a look at #1919 & designing your solution based on that

let us know here (or discord) if you need any help...

@lsampras lsampras removed the S-unassigned Status: This issue has no one assigned to address it label Aug 13, 2023
@divinenaman
Copy link
Contributor

divinenaman commented Aug 26, 2023 •
edited
Loading

@lsampras few questions:

  • I just need to use no_proxy from reqwest right (passing the existing whitelist) or am I missing something ?
  • once_cell is allowing singleton pattern (or not ?) but how does a case where a connector has a different proxy settings than the existing client work ?
  • how can I test this change ?

@divinenaman divinenaman linked a pull request Aug 26, 2023 that will close this issue
Open
15 tasks
@lsampras
Copy link
Member

I just need to use no_proxy from reqwest right (passing the existing whitelist) or am I missing something ?

Yup, adding those URL's to no_proxy should work out,

once_cell is allowing singleton pattern (or not ?) but how does a case where a connector has a different proxy settings than the existing client work ?

Currently we can have differing client certificates that are specific to each connector. In that case we end up creating a new client instead of re-using. You can skip this case since it doesn't apply to our optimization. (If we are anyways creating a new client doesn't make sense to apply selective proxy rules)

how can I test this change ?

You'll need to use a local proxy (there seem to be quite a lot of options depending on your OS etc.),
you can add an connector URL to the proxy whitelist to test it out.

Testing the whitelist's for locker doesn't seem possible for now (since there's no way to host that locally)

@SanchithHegde
Copy link
Member

Since we've felt the need for this recently, I'll be taking this up.

@SanchithHegde SanchithHegde linked a pull request Dec 29, 2024 that will close this issue
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SanchithHegde SanchithHegde

Labels
A-framework Area: Framework C-refactor Category: Refactor good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
5 participants
@lsampras @SanchithHegde @divinenaman @NarsGNA @EliKalter