Skip to content

Conversation

@zwoop
Copy link
Contributor

@zwoop zwoop commented May 3, 2024

An example use case is to conditionally enable / run the rate_limit.so plugin for some requests:

cond %{REMAP_PSEUDO_HOOK} [AND]
cond %{CLIENT-HEADER:X-Leif} ="yes"
     run-plugin rate_limit.so "--limit=300 --error=429"

@zwoop zwoop added the header_rewrite header_rewrite plugin label May 3, 2024
@zwoop zwoop added this to the 10.1.0 milestone May 3, 2024
@zwoop zwoop requested review from ezelkow1 and ywkaras May 3, 2024 22:36
@zwoop zwoop self-assigned this May 3, 2024
run-plugin <plugin-name>.so "<plugin-argument> ..."

This allows to run an existing remap plugin, conditionally, from within a
header rewrite rule.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this run on any hook other than the remap hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it can only run in a remap hook, but the plugin it runs can setup other hooks.

@ywkaras
Copy link
Contributor

ywkaras commented May 3, 2024

Testing schmesting I guess.

shukitchan
shukitchan previously approved these changes May 3, 2024
@shukitchan
Copy link
Contributor

actually there is something wrong with the test.

but basically with the plugin factory, you can achieve this anywhere else, like Lua plugin or txn_box

@ywkaras
Copy link
Contributor

ywkaras commented May 3, 2024

I'll be on vacation for 2 weeks after today, so I'll I have to leave any further review to others. Looks OK from a quick read through.

@zwoop
Copy link
Contributor Author

zwoop commented May 5, 2024

Testing schmesting I guess.

Yeh, well... I can't reproduce the build problem on either my Mac or my Linux box, but seems like it may be that the header_rewrite_test target needs a score dependency?

@zwoop
Copy link
Contributor Author

zwoop commented May 6, 2024

[approve ci]

@zwoop zwoop force-pushed the HRWRunPlugin branch 2 times, most recently from 1c7797e to 513fe05 Compare May 6, 2024 15:50
TSReleaseAssert(_plugin != nullptr);

if (res._rri && res.txnp) {
_plugin->doRemap(res.txnp, res._rri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious. should we pass on the return status of the doRemap() function somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now it is just ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, so we kinda butchered this a bit in all the refactoring and dealings with "chaining" remap plugins. In most cases, this return code is obsoleted, and we don't' really use it. So, I made a choice to ignore it in HRW, whereas in Cripts, the caller can chose to look at it and do something with it (albeit, I think that's unlikely).

I'm not sure the stop / continue works any more either, but almost no plugin that's in use uses that.

@zwoop zwoop merged commit aff07ef into apache:master May 6, 2024
@zwoop zwoop deleted the HRWRunPlugin branch May 6, 2024 20:03
zwoop added a commit to zwoop/trafficserver that referenced this pull request May 6, 2024
zwoop added a commit that referenced this pull request May 6, 2024
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 Jun 5, 2024
@cmcfarlen
Copy link
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

header_rewrite header_rewrite plugin New Feature

Projects

Status: picked-10.0.0

Development

Successfully merging this pull request may close these issues.

4 participants