Skip to content

Ts 3656#215

Closed
shinrich wants to merge 2 commits intoapache:masterfrom
shinrich:ts-3656
Closed

Ts 3656#215
shinrich wants to merge 2 commits intoapache:masterfrom
shinrich:ts-3656

Conversation

@shinrich
Copy link
Copy Markdown
Member

@shinrich shinrich commented Jun 9, 2015

Changes to allow activating follow redirection in send server response hook. Originally this would not work for the post case, because the buffers storing the post data for redirect were being freed too early. Delayed the cleanup until later. Also changed one of the buffers stored with the post state to be set up as local variables instead.

Also since the redirection decision is made late, we needed to make a copy (reference counted copy) of the post buffer regardless what the enable_redirection flag value happens to be at the time the post data is received from the client.

Given that the redirect and post logic are both squirrelly, I'd appreciate another set of eyes looking this over before I commit.

I have validation from a production environment that this patch works to enable the functionality.

@sudheerv
Copy link
Copy Markdown
Contributor

sudheerv commented Jun 9, 2015

Do we claim to support redirection for POST? POST is not an idempotent method and AFAIK, it is highly unreliable and not recommended to use redirection with POST method. See below link for more details:

http://www.alanflavell.org.uk/www/post-redirect.html

@shinrich
Copy link
Copy Markdown
Member Author

shinrich commented Jun 9, 2015

The existing code goes to some effort to support post redirect. That logic has been there since before I got involved. It is a broader discussion if we want to remove that support.

@sudheerv
Copy link
Copy Markdown
Contributor

sudheerv commented Jun 9, 2015

AFAIK, we have always maintained (within our prod at least) that the behavior of redirect follow w/ POST is not guaranteed. As such, the current code of not resubmitting the body to the follow at least seems to address the concerns above. I'm not sure if that should now be changed to resubmit the body. It could result in a significant break of things afaict.

@shinrich
Copy link
Copy Markdown
Member Author

shinrich commented Jun 9, 2015

The existing code does resubmit the body if the enable_redirect is set statically or set early enough in the life cycle of the transaction. This change adjust the logic to enable the body resubmit if the decision is made later. This was a customer request. We may want to reconsider our decision to redirect posts, but I think that is a different bug than this one.

@sudheerv
Copy link
Copy Markdown
Contributor

sudheerv commented Jun 9, 2015

Can you ask the customer/user not to use redirect with POST (with the supporting material from above) I believe I've done that on more than one occasion in the past successfully.

@SolidWallOfCode
Copy link
Copy Markdown
Member

It's not really feasible to ask the user agent to change behavior. In my view the bottom line is that ATS does this, it's an incompatible change to not do it, so we should make it work until we decide if such functionality should be dropped.

@sudheerv
Copy link
Copy Markdown
Contributor

This is not a real UA we are talking about. It's a plugin IIUC.

If you are talking about real UA:
(1) a real UA is not involved in a internal redirect follow done by ATS and
(2) fyi, what ATS does with internal redirect is non-standard. Per the RFC, UA handles redirect follow on its own.

@SolidWallOfCode
Copy link
Copy Markdown
Member

My understanding is it is a plugin that enables redirection following for the request but the request is from a user agent. I am aware this is not compliant but as I noted changing to be strictly RFC compliant in this regard is a incompatible change that is outside the scope of this bug. Until that change is made, we should make what functional does actually exist not be broken.

@asfgit asfgit closed this in 999946e Jun 16, 2015
asfgit pushed a commit that referenced this pull request Jul 6, 2015
…e hook does not work for post. This closes #215."

This reverts commit 999946e.
SolidWallOfCode pushed a commit to SolidWallOfCode/trafficserver that referenced this pull request May 7, 2016
YTSATS-782: Add data to the warning messages to aid in figuring out the ...
SolidWallOfCode pushed a commit to SolidWallOfCode/trafficserver that referenced this pull request May 10, 2021
YTSATS-3212: Don't SNI/host enforce for tls_version
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Sep 24, 2021
apache#215)

* Increased the maximum slice block size from 32MB to 128MB and added a unit test to validate blockbytes configuration behavior.

* Adjusted getopt approach in config unit test to ensure it works correctly on Linux.

(cherry picked from commit b626b47)

Co-authored-by: Jeff Elsloo <elsloo@users.noreply.github.com>
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.

3 participants