TNT-46700 Add proxy configuration #116

ericfichtel · 2023-02-09T22:16:31Z

Description

Adding a new option to the SDK config: proxyAgent: ProxyAgent

This ProxyAgent from the undici library, which now contains the default node fetch implementation starting with node 18.2, allows SDK users to configure a Proxy for the SDK fetch calls. The user implementation of this will vary due to the change in the default node implementation. For node 16.8+ users, the proxy agent is passed into the options argument as a dispatcher during fetch calls. This functionality will not work out of the box for versions of node older than 16.8, as undici fetch does not support node versions older than that. I will outline implementation options for specific versions in the testing section.

Related Issue

Ticket: TNT-46700

Motivation and Context

How Has This Been Tested?

I manually tested this by setting up a local proxy, instantiating a TargetClient with different Node versions, and checking if a connection to the proxy was made during getOffers calls. Here is how I was able to do that with different node versions:

for node versions >= 18.2, this works by just specifying the proxyAgent in the TargetClient options
for node versions between 16.8 and 18.1, this works by overriding the fetchApi with undici.fetch in addition to specifying the proxyAgent. That is because undici.fetch is not the default fetch for node for these versions.
for node versions less than 16.8 and greater than 14 I was not able to connect to the proxy. This is due to a bug in Node 16 with node-fetch and http-proxy-agent. If we go with the approach in this PR, I will do further research on how to get a proxy to work for these versions and document the solution.
for node 14, the proxy works with node-fetch and http-proxy-agent by passing in a custom fetchApi to the TargetClient options.

The setup for each of these can be found in packages/target-nodejs-sdk/demo/proxy-test/proxy_demo.js.

Due to the difference in implementation of fetch between different node versions, unit testing this would require a lot of creativity or discontinuing support for Node 14.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

coveralls · 2023-02-09T22:45:35Z

Coverage: ?%. Remained the same when pulling 4f4d13c on ericfichtel:TNT-46700 into a6e4f77 on adobe:main.

coveralls · 2023-02-09T22:45:38Z

coverage: 91.015% (-1.5%) from 92.502%
when pulling 35e3f73 on ericfichtel:TNT-46700
into a6e4f77 on adobe:main.

XDex

@ericfichtel Oveall looks good, just a couple minor comments

XDex · 2023-02-10T19:05:17Z

packages/target-decisioning-engine/src/geoProvider.js

-      })
+      const options = { headers };
+
+      if (config.proxyAgent != null) {


I'd suggest simplifying a bit

Suggested change

if (config.proxyAgent != null) {

if (config.proxyAgent) {

for consistency let's use isDefined please.

if(isDefined(config.proxyAgent)) {

XDex · 2023-02-10T19:06:02Z

packages/target-decisioning-engine/types/DecisioningConfig.d.ts

+  /**
+   * Proxy Agent to specify a proxy for the fetch implementation
+   */
+  proxyAgent: ProxyAgent;


Should probably also be nullable

Suggested change

proxyAgent: ProxyAgent;

proxyAgent?: ProxyAgent;

Do we need to add undici as a dependency and use this ProxyAgent object? What is the benefit? It is a standard? I wonder if we can define a basic proxy of our own in target tools.

undici fetch is expecting an undici Dispatcher in fetch.options.dispatcher, which is the recommended solution for adding a proxy.

Dispatcher extends node's events.EventEmitter, so it could be possible to omit the undici dependency through extending that, but that seems like it would be out of the scope of this ticket

XDex · 2023-02-10T19:07:55Z

packages/target-nodejs-sdk/demo/proxy-test/proxy_demo.js

+    });
+}
+
+// proxy test for Node 18.2+ users


I'd suggest having this one uncommented, and the ones for older Node versions commented out

XDex · 2023-02-10T19:10:07Z

packages/target-nodejs-sdk/demo/package.json

@@ -10,7 +10,10 @@
  "author": "",
  "license": "Apache-2.0",
  "dependencies": {


we should probably also add "@mutagen-d/node-proxy-server" to dependencies

I am not sure if it is necessary to include the local proxy server file in the repo

jasonwaters

I don't understand why we need to introduce proxyAgent to the SDK. It seems enough that our users have full control to implement this based on what they pass in for fetchImpl. What am I missing? Why is it worth adding these dependencies and complexity rather than just documenting how it can be done with fetchImpl?

jasonwaters · 2023-02-10T19:17:09Z

packages/target-decisioning-engine/src/geoProvider.js

-      })
+      const options = { headers };
+
+      if (config.proxyAgent != null) {


for consistency let's use isDefined please.

if(isDefined(config.proxyAgent)) {

jasonwaters · 2023-02-10T19:26:15Z

packages/target-decisioning-engine/types/DecisioningConfig.d.ts

+  /**
+   * Proxy Agent to specify a proxy for the fetch implementation
+   */
+  proxyAgent: ProxyAgent;


Do we need to add undici as a dependency and use this ProxyAgent object? What is the benefit? It is a standard? I wonder if we can define a basic proxy of our own in target tools.

XDex · 2023-02-10T22:10:26Z

@jasonwaters The idea is to make it simple for customers to configure a proxy, similarly to what we already have in the Java SDK with a simple configuration, instead of requiring fetchApi to be overriden, which is not that straightforward.
Undici is the unerlying fetch implementation starting with Node v18.2, however the ProxyAgent was not included in Node, and thus has to be pulled in from the standalone undici module.
Judging from this comment however, they have decided to support proxy via env vars directly in Node, but this was just 3 weeks ago, so it will take at least several months till it's released and supported in a future Node release. Considering this, maybe it does make sense to just provide a workaround with fetchApi override in a separate documentation section, rather then actually implementing ProxyAgent support, since the proper way of setting a proxy will change once again in an upcoming Node version..
Let's discuss with the team on Monday, meanwhile maybe Eric could test out the workaround with fetchApi override with ProxyAgent and add it to the demo app.. wdyt @jasonwaters @ericfichtel ?

XDex · 2023-02-10T22:17:30Z

@jasonwaters @ericfichtel In case we decide to support ProxyAgent, we can list undici as an optional peer dependency, which only customers that need to set a proxy would need to add to their projects, so the main Node SDK doesn't need to depend on it.

jasonwaters · 2023-02-10T22:17:46Z

Yes I think we should simply document how to do this by passing in a fetch implementation with a proxy configured.

We added proxy functionality to Java SDK because it was not possible to pull off in another way. But I think it better to be less prescriptive in JavaScript and simply expect a fetch implementation to be passed in -- configured with or without a proxy. Maximum freedom for users.

packages/target-nodejs-sdk/test/proxy.spec.js

jasonwaters · 2023-02-15T19:45:36Z

looks good @ericfichtel !

eric-fichtel-adobe added 2 commits February 9, 2023 14:13

initial commit for adding proxy support

e773710

renaming demo file to omit from tests

4f4d13c

ericfichtel requested review from XDex, dcottingham, jasonwaters and 5amfung February 9, 2023 23:07

XDex reviewed Feb 10, 2023

View reviewed changes

jasonwaters reviewed Feb 10, 2023

View reviewed changes

eric-fichtel-adobe added 2 commits February 13, 2023 13:59

removing proxy config option and editing proxy demo to reflect changes

35e3f73

adding a unit test for proxy config

c3521b8

ericfichtel requested review from jasonwaters and XDex February 14, 2023 20:17

jasonwaters requested changes Feb 14, 2023

View reviewed changes

packages/target-nodejs-sdk/test/proxy.spec.js Outdated Show resolved Hide resolved

packages/target-nodejs-sdk/test/proxy.spec.js Outdated Show resolved Hide resolved

modifying proxy unit test to not use node fetch

a32f701

ericfichtel requested a review from jasonwaters February 15, 2023 19:01

jasonwaters approved these changes Feb 15, 2023

View reviewed changes

ericfichtel merged commit c7aa8ef into adobe:main Feb 15, 2023

Nevon mentioned this pull request Mar 29, 2023

No HTTP proxy support pinecone-io/pinecone-ts-client#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TNT-46700 Add proxy configuration #116

TNT-46700 Add proxy configuration #116

ericfichtel commented Feb 9, 2023 •

edited by XDex

Loading

coveralls commented Feb 9, 2023

coveralls commented Feb 9, 2023 •

edited

Loading

XDex left a comment

XDex Feb 10, 2023

jasonwaters Feb 10, 2023

XDex Feb 10, 2023

jasonwaters Feb 10, 2023

ericfichtel Feb 10, 2023 •

edited

Loading

ericfichtel Feb 10, 2023

XDex Feb 10, 2023

XDex Feb 10, 2023

ericfichtel Feb 13, 2023 •

edited

Loading

jasonwaters left a comment •

edited

Loading

jasonwaters Feb 10, 2023

jasonwaters Feb 10, 2023

XDex commented Feb 10, 2023

XDex commented Feb 10, 2023

jasonwaters commented Feb 10, 2023

jasonwaters commented Feb 15, 2023

TNT-46700 Add proxy configuration #116

TNT-46700 Add proxy configuration #116

Conversation

ericfichtel commented Feb 9, 2023 • edited by XDex Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

coveralls commented Feb 9, 2023

coveralls commented Feb 9, 2023 • edited Loading

XDex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfichtel Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfichtel Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

jasonwaters left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XDex commented Feb 10, 2023

XDex commented Feb 10, 2023

jasonwaters commented Feb 10, 2023

jasonwaters commented Feb 15, 2023

ericfichtel commented Feb 9, 2023 •

edited by XDex

Loading

coveralls commented Feb 9, 2023 •

edited

Loading

ericfichtel Feb 10, 2023 •

edited

Loading

ericfichtel Feb 13, 2023 •

edited

Loading

jasonwaters left a comment •

edited

Loading