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 client option to disable following redirects #21027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add client option to disable following redirects #21027

wants to merge 1 commit into from

Conversation

alprusty
Copy link
Contributor

@alprusty alprusty commented Mar 12, 2024 •
edited
Loading

Description

During 412 release as part of OkHttp version upgrade, explicit client retry logic was removed in the event of a 307/308 redirect. This will cause 401: 'Unauthorized' error when trino client is talking to a Trino Cluster that is deployed behind a load balancer(For example AWS ALB) with a redirect mode(e.g. 307/308) routing policy.

Reference: 4be1401

Okhttp by default does follow redirects. However it drops Authorization headers if the redirect is across hosts.

Reference: https://github.com/square/okhttp/blob/okhttp_4.10.x/okhttp/src/main/kotlin/okhttp3/internal/http/RetryAndFollowUpInterceptor.kt#L323-L328

Solution:
This PR introduces a flag --disable-follow-redirects to give an option to clients to follow a redirect or not. By default the flag is set to false which means redirects are going to be followed.

Additional context and related issues

Fixes #21026

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 12, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: alprusty.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

1 similar comment
@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 12, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: alprusty.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@alprusty alprusty force-pushed the allow-redirect-trino-client branch from f9932c7 to 7b2a197 Compare March 12, 2024 09:22
@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 12, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: alprusty.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@wendigo wendigo requested a review from nineinchnick March 12, 2024 11:07
@oneonestar
Copy link
Member

We shouldn't make this change until we've a clear consensus on whether to include the Authorization header on redirects.
#21026 (comment)

@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 12, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: alprusty.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@alprusty alprusty mentioned this pull request Mar 12, 2024
Closed
@mosabua mosabua requested review from electrum and wendigo March 21, 2024 18:00
@mosabua
Copy link
Member

mosabua commented Mar 21, 2024

Can you submit a CLA @alprusty ? @martint can process it soon after and trigger check here .. but we can work on reviews and more in parallel.

@mosabua
Copy link
Member

mosabua commented Mar 21, 2024

Also fyi @sajjoseph

@mosabua
Copy link
Member

mosabua commented Mar 21, 2024

This was discussed with @electrum @dain @martint and others in the Trino Contributor Call from 21 March 2024.

@sajjoseph sajjoseph mentioned this pull request Mar 22, 2024
Merged
@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 22, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: alprusty.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

alprusty
alprusty commented Mar 22, 2024
View reviewed changes
client/trino-cli/src/main/java/io/trino/cli/ClientOptions.java
@@ -273,6 +273,9 @@ public class ClientOptions
@Option(names = "--disable-auto-suggestion", description = "Disable auto suggestion")
public boolean disableAutoSuggestion;

Copy link
Contributor Author

@alprusty alprusty Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Introduced a new client option to disable redirects.

@github-actions github-actions bot added docs jdbc Relates to Trino JDBC driver labels Mar 22, 2024
@mosabua
Copy link
Member

mosabua commented Mar 22, 2024

CLA is signed .. I am working with @alprusty to get configuration fixed

@mosabua
Copy link
Member

mosabua commented Mar 22, 2024

@cla-bot check

@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 22, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: alprusty.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot
Copy link

cla-bot bot commented Mar 22, 2024

The cla-bot has been summoned, and re-checked this pull request!

@alprusty alprusty force-pushed the allow-redirect-trino-client branch from fbdd26a to 4768a5d Compare March 22, 2024 22:53
@cla-bot cla-bot bot added the cla-signed label Mar 22, 2024
@oneonestar
Copy link
Member

@alprusty We also need a regression test on the client actually sent the Authorization header after a redirect.

@alprusty alprusty force-pushed the allow-redirect-trino-client branch 2 times, most recently from 1dcc9a5 to 4dffa6b Compare March 31, 2024 18:49
@alprusty
Copy link
Contributor Author

@oneonestar
I took care of all the review comments.

Please review again 🙏

@alprusty alprusty requested a review from oneonestar March 31, 2024 18:57
@sajjoseph
Copy link
Contributor

It will be great if this PR can be split into two.

  1. Control followRedirects in okhttp
  2. Retain authorization header during redirects

@alprusty alprusty force-pushed the allow-redirect-trino-client branch from fab4b2c to 854b91e Compare April 2, 2024 08:48
@alprusty alprusty requested a review from oneonestar April 2, 2024 08:50
oneonestar
oneonestar suggested changes Apr 2, 2024
View reviewed changes
client/trino-cli/src/main/java/io/trino/cli/ClientOptions.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientOptions.java Outdated Show resolved Hide resolved
client/trino-jdbc/src/test/java/io/trino/jdbc/TestTrinoDriverUri.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/JsonResponse.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Show resolved Hide resolved
@alprusty alprusty requested review from dain and oneonestar April 11, 2024 21:07
@alprusty alprusty changed the title Allow cross host client redirects in Trino client Add client option to disable following redirects Apr 12, 2024
oneonestar
oneonestar reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@oneonestar oneonestar left a comment

Choose a reason for hiding this comment

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

LGTM except some minor comments.

@dain @electrum Please could you take a look.
I hope we can land this in the next release, since #21203 is only an incomplete fix.

client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Outdated Show resolved Hide resolved
client/trino-cli/src/test/java/io/trino/cli/TestClientRedirect.java Show resolved Hide resolved
docs/src/main/sphinx/client/cli.md Outdated
@@ -149,6 +149,8 @@ mode:
processing statistics.
* - `--disable-auto-suggestion`
- Disables autocomplete suggestions.
* - `--disable-follow-redirects`
- If set to `true`, then disables client redirects. Default is `false`.
Copy link
Member

Choose a reason for hiding this comment

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

@mosabua Any comments for the docs?

@alprusty alprusty requested a review from oneonestar April 16, 2024 06:19
@alprusty alprusty force-pushed the allow-redirect-trino-client branch from ed706bd to c991e9b Compare April 17, 2024 08:46
@alprusty
Copy link
Contributor Author

squashed all commits as one

@alprusty alprusty force-pushed the allow-redirect-trino-client branch from c991e9b to f27709f Compare April 17, 2024 18:59
@alprusty alprusty requested a review from mosabua April 17, 2024 19:00
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label May 9, 2024
@mosabua mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels May 9, 2024
@mosabua
Copy link
Member

mosabua commented May 9, 2024 •
edited
Loading

We still want to continue with this. Can you rebase @alprusty

And can others chime in if any more feedback is needed.

Hopefully @martint @dain or @electrum can move this to merge

@alprusty alprusty force-pushed the allow-redirect-trino-client branch from f27709f to d28cb16 Compare May 28, 2024 01:36
@alprusty alprusty requested review from electrum and removed request for electrum May 28, 2024 05:35
@alprusty alprusty force-pushed the allow-redirect-trino-client branch from d28cb16 to f413537 Compare May 28, 2024 18:32
@mhassanzahid
Copy link

@alprusty @mosabua is this still going forward or has this been added in some other release?

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

@oneonestar oneonestar oneonestar approved these changes

@nineinchnick nineinchnick Awaiting requested review from nineinchnick

@wendigo wendigo Awaiting requested review from wendigo

@dain dain Awaiting requested review from dain

@mosabua mosabua Awaiting requested review from mosabua

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed docs jdbc Relates to Trino JDBC driver stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.
Milestone
No milestone
5 participants
@alprusty @oneonestar @mosabua @sajjoseph @mhassanzahid