Hi Sudheer, > I may have missed if there was some discussion on this in the past, > but IMHO, this seems to be a little too specific and custom to be made > part of the core.
This has been merged to master awhile back, and there is some discussion linked from the PR: https://github.com/apache/trafficserver/pull/4145 This change was done in core because the problem it tries to address is in core itself and not in a plugin. I think perhaps if we want to move this kind of feature into a plugin, then that would necessitate removing the feature it configures from ATS into a plugin. > Not to mention, the interaction between the existing redirect follow > and cache isn’t super clean already and this new overload of that > config seems like it has the potential to make it a lot worse. Yeah, the interaction between cache and following redirects is not only a mess in configuration, but also it is a mess in concept and therefore also in implementation. Specifically, following redirects is by itself can be tricky, and it is not always clear in theory what functionality is most desirable: https://github.com/apache/trafficserver/issues/2742 As another example—I am not sure if there is any existing issue—each time we follow a redirect, we cache only the eventual result and do not cache the headers for any of the intermediate redirect responses. This has an unintended effect of neglecting valid Cache-Control information, such that if cache expires earlier for an intermediate redirect, I believe ATS continues to serve the eventual result while it is available from cache. All this to say, I concur it is a messy situation. This specific change is an incremental approach to harden what we have. I expect most users will accept the 9.x default (follow only globally routable addresses), and should 8.1.x users want to use the safer behavior without disabling the feature, this gives more than enough granularity to do that. At least, that was my thought in offering to back-port it. -- Derek On Sat, Aug 10, 2019 at 07:50:31PM -0700, Sudheer Vinukonda wrote: > > Sorry, I don’t have an answer to your question on 8.1.x specifically, but > looking at your PR, I’m thinking if this should be done as a plugin and not > in the core. I may have missed if there was some discussion on this in the > past, but IMHO, this seems to be a little too specific and custom to be made > part of the core. Not to mention, the interaction between the existing > redirect follow and cache isn’t super clean already and this new overload of > that config seems like it has the potential to make it a lot worse. > > Thanks, > > Sudheer > > > On Aug 10, 2019, at 12:32 PM, Derek Dagit <da...@apache.org> wrote: > > > > I want to port the following PR to the 8.x line, but as it is changing > > configs and adding a feature, it should target a minor release. > > > > https://github.com/apache/trafficserver/pull/4145 > > > > Earlier I fetched an 8.1.x branch, but now that I have a branch ready > > for a pull request it appears that branch is gone. > > > > This is the proposed PR: > > https://github.com/apache/trafficserver/compare/8.1.x_failed...d2r:handle-redirect-to-loopback-8.1.x > > > > What should I do? > > > > -- > > Derek
