You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "ywkaras (via GitHub)" <gi...@apache.org> on 2023/02/17 02:08:04 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request, #9432: webp_transform plugin: Use Accept header from client request.
ywkaras opened a new pull request, #9432:
URL: https://github.com/apache/trafficserver/pull/9432
Also switches from obsolete TSDebug() to TSDbg().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1437729720
I think it would be fruitless for me to argue with the people who control the origin servers with that behavior. It has to be supported in Yahoo internal TS. Maybe they would accept it if webp_transform only fell back on the request Accept if the response Accept was not present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1437698087
Yahoo origin servers sanitize some responses, which includes not putting the Accept header field in the response.
What's a plausible scenario where the server would communicate something to the client by changing the accept header?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1437840292
@bryancall any thougths? Our relevant Jira is YTSATS-3928.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] closed pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9432: webp_transform plugin: Use Accept header from client request.
URL: https://github.com/apache/trafficserver/pull/9432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#discussion_r1119515501
##########
plugins/experimental/webp_transform/ImageTransform.cc:
##########
@@ -183,7 +183,7 @@ class GlobalHookPlugin : public GlobalPlugin
// If we might need to convert check to see if what the browser supports
if (transaction_convert_to_webp == true || transaction_convert_to_jpeg == true) {
- std::string accept = transaction.getServerRequest().getHeaders().values("Accept");
+ std::string accept = transaction.getClientRequest().getHeaders().values("Accept");
bool webp_supported = accept.find("image/webp") != std::string::npos;
Review Comment:
If something modifies Accept header, the something should be responsible for returning a content that the client accept.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1435514934
OK changes made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1546763703
[approve ci cmake]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ezelkow1 commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ezelkow1 (via GitHub)" <gi...@apache.org>.
ezelkow1 commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1554941819
[approve ci centos]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1437307722
I'm not sure if we should make this change. Accept header on the origin side should be the same as the one on the client side unless something modifies it. If it's not the same, that means something modified it for some reason, and this change ignores it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1437720395
I don't see the connection with sanitizing responses. Accept header field in the response? Can you tell me some more detail?
I have no idea in what case one would want to modify Accept header, but if you need this change, something on your ATS modifies it, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1683214310
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1437773751
I still don't understand the context.
Your origin server includes Accept header in HTTP response?
How using Accept header from a client helps something in the situation?
In any case, the behavior should be configurable at a minimum because it could affect somebody else's use case. And the default behavior should be current behavior unless there's some justification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#issuecomment-1447282334
I agree we should use the client Accept header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#discussion_r1119493377
##########
plugins/experimental/webp_transform/ImageTransform.cc:
##########
@@ -183,7 +183,7 @@ class GlobalHookPlugin : public GlobalPlugin
// If we might need to convert check to see if what the browser supports
if (transaction_convert_to_webp == true || transaction_convert_to_jpeg == true) {
- std::string accept = transaction.getServerRequest().getHeaders().values("Accept");
+ std::string accept = transaction.getClientRequest().getHeaders().values("Accept");
Review Comment:
The next few lines of code decide whether and how to transform the body of the response to client to a different content type. So, it make more sense to look at what content types the client will accept, not what types we told the server ATS would accept.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#discussion_r1119494183
##########
plugins/experimental/webp_transform/ImageTransform.cc:
##########
@@ -183,7 +183,7 @@ class GlobalHookPlugin : public GlobalPlugin
// If we might need to convert check to see if what the browser supports
if (transaction_convert_to_webp == true || transaction_convert_to_jpeg == true) {
- std::string accept = transaction.getServerRequest().getHeaders().values("Accept");
+ std::string accept = transaction.getClientRequest().getHeaders().values("Accept");
bool webp_supported = accept.find("image/webp") != std::string::npos;
Review Comment:
Seems wrong that we just assume the client will accept "image/jpeg" regardless of what's in the accept header the client sent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9432: webp_transform plugin: Use Accept header from client request.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9432:
URL: https://github.com/apache/trafficserver/pull/9432#discussion_r1119514616
##########
plugins/experimental/webp_transform/ImageTransform.cc:
##########
@@ -183,7 +183,7 @@ class GlobalHookPlugin : public GlobalPlugin
// If we might need to convert check to see if what the browser supports
if (transaction_convert_to_webp == true || transaction_convert_to_jpeg == true) {
- std::string accept = transaction.getServerRequest().getHeaders().values("Accept");
+ std::string accept = transaction.getClientRequest().getHeaders().values("Accept");
Review Comment:
True, if there is not another transform plugin, which supposedly modified Accept header, between this plugin and the client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org