Add set-custom-body config item to header_rewrite #11472

jasmine-nahrain · 2024-06-24T04:12:00Z

Adding config item set-custom-body to header rewrite.
The config takes in a url. When triggered, there will be a secondary call the the specified endpoint to get a response body from. What the client will see is the response body from the second call.
The second call goes through as an internal ATS request and goes gets remapped.
The status of the original transaction will remain in all error status code cases. OK status codes will appear as a 500 error code due to the nature of TSHttpTxnErrorBodySet.

e.g.
set_custom_body.conf

cond %{READ_RESPONSE_HDR_HOOK} [AND]
cond %{STATUS} = 404
set-custom-body https://example.com/404

remap.config

map /home http://example.com/home @plugin=header_rewrite.so @pparam=set_custom_body.conf
map http://example.com/404 http://example.com/404

With the above configs, when /home returns a 404 status code, a second call will be made to /404 through ATS internal request
The response that is sent to the client is

The status code from the original transaction (/home)
The response body from the second transaction (/404)

(cherry picked from commit 5bd999953b2605898f15714453250cb7e5e403f9) (cherry picked from commit 08c614ef0089b175a5b6cee205b748416efb87cd)

* Update response body to exclude headers * Update tests to check both response with headers and response body only * Update header_rewrite_custom_body.test.py * Fix tests to check headers and body (cherry picked from commit cb552b63d6755a554edc5b67721f35678b38163b) (cherry picked from commit e12f4798d0d46bd90c810f8f3a7a067ea3b2c76f)

(cherry picked from commit 964b12cc21a9a7c3f9d3fd50286e4cd2d2757bcc)

Remove whitespaces doc formatting fix Doc formatting fix

JosiahWI · 2024-06-25T10:54:10Z

The AuTest prefetch_overflow failed.

   Run: 2-tr: Failed
     Starting TestRun 2-tr : No Issues found - Passed
        Reason: Started!
     Process: Default: Failed
       Test : Checking that ReturnCode == 0 - Passed
          Reason: Returned Value: 0 == 0
       file /tmp/sandbox/prefetch_overflow/_output/2-tr-Default/stream.stdout.txt : Checking that /tmp/sandbox/prefetch_overflow/_output/2-tr-Default/stream.stdout.txt matches prefetch_overflow.gold - Failed
          Reason: File differences
           Gold File : /home/jenkins/workspace/Github_Builds/autest/src/tests/gold_tests/pluginTest/prefetch/prefetch_overflow.gold
           Data File : /tmp/sandbox/prefetch_overflow/_output/2-tr-Default/stream.stdout.txt
             GET http://domain.in/texts/demo-3594967639391.txt HTTP/1.1
           - GET http://domain.in/texts/demo-3594967639392.txt HTTP/1.1
           - GET http://domain.in/texts/demo-3594967639393.txt HTTP/1.1
           - GET http://domain.in/texts/demo-3594967639394.txt HTTP/1.1

The AuTest prefetch_bignum failed:

   Run: 2-tr: Failed
     Starting TestRun 2-tr : No Issues found - Passed
        Reason: Started!
     Process: Default: Failed
       Test : Checking that ReturnCode == 0 - Passed
          Reason: Returned Value: 0 == 0
       file /tmp/sandbox/prefetch_bignum/_output/2-tr-Default/stream.stdout.txt : Checking that /tmp/sandbox/prefetch_bignum/_output/2-tr-Default/stream.stdout.txt matches prefetch_bignum.gold - Failed
          Reason: File differences
           Gold File : /home/jenkins/workspace/Github_Builds/autest/src/tests/gold_tests/pluginTest/prefetch/prefetch_bignum.gold
           Data File : /tmp/sandbox/prefetch_bignum/_output/2-tr-Default/stream.stdout.txt
             GET http://domain.in/texts/demo-3842948374928374982374982374.txt HTTP/1.1
           - GET http://domain.in/texts/demo-3842948374928374982374982375.txt HTTP/1.1
           - GET http://domain.in/texts/demo-3842948374928374982374982376.txt HTTP/1.1
           - GET http://domain.in/texts/demo-3842948374928374982374982377.txt HTTP/1.1

ywkaras · 2024-06-25T22:56:28Z

plugins/header_rewrite/operators.cc

+const unsigned int MAX_SIZE         = 256;
+const int          LOCAL_PORT       = 8080;
+
+static int


It doesn't need to be static. Putting it in an anonymous namespace makes it file scope.

ywkaras · 2024-06-25T23:00:52Z

plugins/header_rewrite/operators.cc

+    TSContDestroy(cont);
+    TSHttpTxnReenable(http_txn, TS_EVENT_HTTP_CONTINUE);
+  } break;
+  case TS_EVENT_HTTP_SEND_RESPONSE_HDR:


What's the purpose of executing the continuation on this hook (since nothing is done)?

This pauses the original transaction to allow time to get the response from the second endpoint. The second endpoint will reenable the first once it has completed. It is written in the code for readability

ywkaras · 2024-06-25T23:06:35Z

tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_from.test.py

+        self.runTraffic()
+
+
+HeaderRewriteSetBodyFromTest().run()


Could you add another test run where the fetch of the body URL fails?

JosiahWI · 2024-06-26T12:48:11Z

The AuTest transaction_data_sink failed.

     file /tmp/sandbox/transaction_data_sink/ts/log/traffic.out : The expected HTTP/2 response body was dumped. - Failed
        Reason: Contents of /tmp/sandbox/transaction_data_sink/ts/log/traffic.out did not contains expression: ""http2_response_body_dumped""

doc/admin-guide/plugins/header_rewrite.en.rst

plugins/header_rewrite/operators.h

tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_from.test.py

JosiahWI · 2024-06-26T15:06:07Z

plugins/header_rewrite/operators.cc

+    event_ids.failure_event_id = TS_EVENT_FETCHSM_FAILURE;
+    event_ids.timeout_event_id = TS_EVENT_FETCHSM_TIMEOUT;
+
+    struct sockaddr_in addr;


Include <sys/socket.h>?

ywkaras · 2024-06-26T15:31:37Z

plugins/header_rewrite/operators.cc

+void
+OperatorSetBodyFrom::initialize_hooks()
+{
+  add_allowed_hook(TS_HTTP_READ_RESPONSE_HDR_HOOK);


I notice that your test scenario is using header_rewrite as a remap plugin. Would this operator ever be useful when header_rewrite is used as a global plugin?

Yes - this works with both remap and plugin configs. I have just added some tests to check this aswell.

plugins/header_rewrite/operators.cc

ywkaras · 2024-06-27T01:22:30Z

tests/gold_tests/pluginTest/header_rewrite/rules/rule_set_body_from_remap.conf

+cond %{CLIENT-URL:PATH} = "200"
+set-body-from http://www.example.com/404.html
+
+cond %{READ_RESPONSE_HDR_HOOK}


Does the set-body-from operator require this hook condition in order to work properly? If so, maybe you should mention that in the documentation.

It does not. I am using the path to differentiate the tests.

ywkaras · 2024-06-27T01:54:23Z

tests/gold_tests/pluginTest/header_rewrite/header_rewrite_set_body_from.test.py

+             map http://www.example.com/plugin_fail http://127.0.0.1:{0}/plugin_fail
+             map http://www.example.com/404.html http://127.0.0.1:{0}/404.html
+             map http://www.example.com/502 http://127.0.0.1:{0}/502
+             """.format(self.server.Variables.Port, Test.RunDirectory))


I suggest you also have a remap for a body URL like this:

Test.GetTcpPort("bad_port") self.ts.Disk.remap_config.AddLine(f'map http://www.example.com/plugin_no_server http://127.0.0.1::{Test.Variables.bad_port}/plugin_no_server")

in order to verify proper behavior if it's not possible to connect to the server that provided the body that's set by the operator.

ywkaras · 2024-06-27T02:04:02Z

tests/gold_tests/pluginTest/header_rewrite/rules/rule_set_body_from_remap.conf

+
+cond %{READ_RESPONSE_HDR_HOOK}
+cond %{CLIENT-URL:PATH} = "remap_fail"
+set-body-from http://www.example.com/fail


So, in all the test cases, we still do the original request to upstream, then entirely throw it away. Wouldn't more realistic test cases be with conditions checking response status or response headers?

If the intent is to always discard the original response, wouldn't it be better for the operator to execute in the remap pseudo hook, and set an error status there, to block the original request?

statichit is an example of a plugin that blocks the request to upstream by setting an error status in the DoRemap funciton:

trafficserver/plugins/statichit/statichit.cc

Line 639 in 0d004b3

TSHttpTxnStatusSet(rh, TS_HTTP_STATUS_NOT_FOUND);

I have written the tests to replicate what happens in production. Do you have an example of another test that only checks the response status?

If the tests reflect how you would use this operator in production, why wouldn't you want to avoid the initial request to the upstream server, when the data it returns will always be entirely discarded?

The status is set from that initial request.

ywkaras · 2024-06-27T02:25:56Z

plugins/header_rewrite/operators.cc

+    // The transaction is reenabled with the FetchSM transaction
+    break;
+  default:
+    TSError("Warning: handleFetchEvents got unknown event: %d", event);


So in diags.log this will look like:
ERROR: Warning: ...
huh???

ywkaras · 2024-06-27T04:16:19Z

plugins/header_rewrite/operators.cc

+    TSCont fetchCont = TSContCreate(handleFetchEvents, TSMutexCreate());
+    TSContDataSet(fetchCont, static_cast<void *>(res.txnp));
+
+    TSHttpTxnHookAdd(res.txnp, TS_HTTP_SEND_RESPONSE_HDR_HOOK, fetchCont);


Suppose there is another continuation for the transaction, that is either after the continuation calling this exec func on the READ_RESPONSE_HDR_HOOK, or before fetchCont on the SEND_RESPONSE_HDR_HOOK And, suppose this continuation, like fetchEvent for the SEND_RESPONSE_HDR evert, does not call TxnReenable(). Seems like there could be a race condition, where the URL fetch body done event reenables for that other continuation by mistake.

Could you add a return value to the operator exec funtions, which would control whether the continuation running the exec functions called TxnReenable()? That way, no other continuations of transaction hooks would be run until the fetch of the body URL finished.

I see what you are saying but I dont see how the exec func returning something would help. I think a return value would only have impact on header_rewrite rather than all plugins. This would also make changes to all of header_rewrite.

Jasmine Emanouel added 3 commits June 24, 2024 13:47

Add set-body-custom to header_rewrite options

87ec9ac

(cherry picked from commit 5bd999953b2605898f15714453250cb7e5e403f9) (cherry picked from commit 08c614ef0089b175a5b6cee205b748416efb87cd)

Remove debug check due to known issue

49c65f0

(cherry picked from commit 964b12cc21a9a7c3f9d3fd50286e4cd2d2757bcc)

masaori335 assigned jasmine-nahrain Jun 24, 2024

masaori335 added Plugins header_rewrite header_rewrite plugin labels Jun 24, 2024

masaori335 added this to the 10.1.0 milestone Jun 24, 2024

Fix formatting

fbb0537

Remove whitespaces doc formatting fix Doc formatting fix

jasmine-nahrain force-pushed the set-custom-body branch from 1b7bf2f to fbb0537 Compare June 24, 2024 05:49

bryancall requested a review from ywkaras June 24, 2024 22:11

Rename to set-body-from

2e7f7b3

ywkaras reviewed Jun 25, 2024

View reviewed changes

Add test for failed second call

11b095a

JosiahWI requested changes Jun 26, 2024

View reviewed changes

ywkaras reviewed Jun 26, 2024

View reviewed changes

djcarlin mentioned this pull request Jun 26, 2024

header_rewrite set-body feature only supports the most basic of response bodies #11480

Open

ywkaras reviewed Jun 26, 2024

View reviewed changes

plugins/header_rewrite/operators.cc Outdated Show resolved Hide resolved

jasmine-nahrain added 2 commits June 27, 2024 10:23

Add more test cases

e71943a

Clarify test cases

dd1ceec

ywkaras reviewed Jun 27, 2024

View reviewed changes

Update tests

2285524

ywkaras reviewed Jun 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add set-custom-body config item to header_rewrite #11472

Add set-custom-body config item to header_rewrite #11472

jasmine-nahrain commented Jun 24, 2024

JosiahWI commented Jun 25, 2024

ywkaras Jun 25, 2024

ywkaras Jun 25, 2024

jasmine-nahrain Jun 26, 2024 •

edited

Loading

ywkaras Jun 25, 2024

JosiahWI commented Jun 26, 2024

JosiahWI Jun 26, 2024

ywkaras Jun 26, 2024

jasmine-nahrain Jun 27, 2024

ywkaras Jun 27, 2024

jasmine-nahrain Jun 27, 2024

ywkaras Jun 27, 2024

ywkaras Jun 27, 2024

ywkaras Jun 27, 2024

jasmine-nahrain Jun 27, 2024

ywkaras Jun 27, 2024

jasmine-nahrain Jun 28, 2024

ywkaras Jun 27, 2024

ywkaras Jun 27, 2024

jasmine-nahrain Jun 28, 2024 •

edited

Loading

		self.runTraffic()


		HeaderRewriteSetBodyFromTest().run()

Add set-custom-body config item to header_rewrite #11472

Are you sure you want to change the base?

Add set-custom-body config item to header_rewrite #11472

Conversation

jasmine-nahrain commented Jun 24, 2024

JosiahWI commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmine-nahrain Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosiahWI commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmine-nahrain Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

jasmine-nahrain Jun 26, 2024 •

edited

Loading

jasmine-nahrain Jun 28, 2024 •

edited

Loading