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

Add support for routing through an HTTPS proxy #2279

Merged
merged 11 commits into from
May 13, 2024
Merged

Add support for routing through an HTTPS proxy #2279

merged 11 commits into from
May 13, 2024

Conversation

Eric-Alvarez
Copy link
Contributor

@Eric-Alvarez Eric-Alvarez commented May 13, 2024 •
edited
Loading

Closes #2278

Before this PR

HTTPS proxies are not supported in dialogue

After this PR

==COMMIT_MSG==

==COMMIT_MSG==
If an HTTPS proxy is configured then dialogue will be able to successfully route through it

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented May 13, 2024 •
edited by Eric-Alvarez
Loading

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add support for routing through HTTPS proxies in the apache client when configured

Check the box to generate changelog(s)

  • Generate changelog entry

@Eric-Alvarez Eric-Alvarez requested a review from carterkozak May 13, 2024 15:00
Eric-Alvarez
Eric-Alvarez commented May 13, 2024
View reviewed changes
...apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java Outdated
/**
* Identical to {@link SystemDefaultRoutePlanner} but adds support for connecting to an HTTPS proxy.
*/
public final class HttpsProxyDefaultRoutePlanner extends DefaultRoutePlanner {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have preferred to override SystemDefaultRoutePlanner in case there's any changes there in the future, but the proxySelector is private so I would override determineProxy and copy everything anyway

carterkozak
carterkozak reviewed May 13, 2024
View reviewed changes
...apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java Outdated
/**
* Identical to {@link SystemDefaultRoutePlanner} but adds support for connecting to an HTTPS proxy.
*/
public final class HttpsProxyDefaultRoutePlanner extends DefaultRoutePlanner {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class should be package private because it's an implementation detail of dialogue -- we don't want folks to access this directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@Eric-Alvarez Eric-Alvarez requested a review from carterkozak May 13, 2024 17:39
carterkozak
carterkozak reviewed May 13, 2024
View reviewed changes
changelog/@unreleased/pr-2279.v2.yml Outdated
feature:
description: |-
Add support for routing through HTTPS proxies in the apache client when configured
<!-- User-facing outcomes this PR delivers -->
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: delete this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted, does changelog bot work differently here 🤔

@Eric-Alvarez Eric-Alvarez requested a review from carterkozak May 13, 2024 17:47
carterkozak
carterkozak reviewed May 13, 2024
View reviewed changes
...apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java
import org.apache.hc.core5.http.protocol.HttpContext;

/**
* Identical to {@link SystemDefaultRoutePlanner} but adds support for connecting to an HTTPS proxy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's link the original source here and describe that the original is licensed under apache 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit confused, it is linked, did you want it linked differently? Or you want a link to http://www.apache.org/licenses/LICENSE-2.0

Added a line below "Original version is licensed under Apache License, Version 2.0."

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to have a link to the original, at the version used at the point in time we modified from. This way if there are future changes, we can tell whether or not they may be impactful. The javadoc link will point toward whatever version we happen to depend on at any point in time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, updated the docs

@Eric-Alvarez Eric-Alvarez requested a review from carterkozak May 13, 2024 19:05
carterkozak
carterkozak approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

thanks!

@bulldozer-bot bulldozer-bot bot merged commit 510e16b into develop May 13, 2024
6 checks passed
@bulldozer-bot bulldozer-bot bot deleted the ea/https-proxy branch May 13, 2024 19:18
@svc-autorelease
Copy link
Collaborator

Released 3.134.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carterkozak carterkozak carterkozak approved these changes

Assignees
No one assigned
Labels
autorelease merge when ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for routing through HTTPS proxies
4 participants
@Eric-Alvarez @svc-autorelease @carterkozak @svc-changelog