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

[PECO-1042] Add proxy support #193

Merged
merged 8 commits into from
Oct 4, 2023
Merged

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Oct 3, 2023

Depends on #191

  • Proxy support for Thrift operations
  • Proxy support for auth providers (specifically, OAuth)
  • Proxy support for Cloud Fetch
  • Proxy support for staging ingestion
  • Tests (manual for now, proper tests will be added later)

Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

I love that this captures proxy behaviour for all http operations.

Question though: what if the customer needs a proxy to access Databricks but doesn't need a proxy for cloud fetch operations? Is there a way to disable the proxy for some ops?

// Obtain http agent each time when we need an OAuth client
// to ensure that we always use a valid agent instance
const connectionProvider = await this.context.getConnectionProvider();
this.agent = await connectionProvider.getAgent();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does http.Agent automatically establish a connection pool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A sort of. It manages a cache of sockets and can re-use them if possible. I don't know if we can call it a connection pool, but it serves a similar purpose

protocol: proxyProtocol,
});
} else {
this.agent = options.https ? new https.Agent(httpsAgentOptions) : new http.Agent(httpAgentOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

This nested if-then logic confuses me. Under what circumstances is line 71 executed? Is it just when a proxy isn't set as part of options?

If so, for readability I'd recommend swapping the order of the conditions. I'd also avoid the nested if by encapsulating the check on line 71 in its own function that returns either an httpAgentOptions or httpsAgentOptions when passed the options object. So the code would look like this:

if (options.proxy == undefined) {
    this.agent = buildAgentFromOptions(options)
} else {
    const proxyUrl = buildProxyUrl(options.proxy)
    ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. Will split this code a bit 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did it a bit differently, but it also should be good: ad01894 Please take one more look

Base automatically changed from add-client-context to main October 4, 2023 19:49
@kravets-levko kravets-levko temporarily deployed to azure-prod October 4, 2023 20:14 — with GitHub Actions Inactive
@kravets-levko kravets-levko marked this pull request as ready for review October 4, 2023 20:14
@databricks databricks deleted a comment from codecov-commenter Oct 4, 2023
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
@kravets-levko kravets-levko temporarily deployed to azure-prod October 4, 2023 20:21 — with GitHub Actions Inactive
Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Nice LGTM

@databricks databricks deleted a comment from codecov-commenter Oct 4, 2023
@kravets-levko kravets-levko merged commit fd36b2e into main Oct 4, 2023
5 checks passed
@kravets-levko kravets-levko deleted the PECO-1042-proxy-support branch October 4, 2023 20:43
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.

2 participants