Skip to content

Conversation

@zwoop
Copy link
Contributor

@zwoop zwoop commented Apr 21, 2023

This could possibly be considered a bug, but certainly it's not a great behavior as it was. Before this patch, filters (as defined and used with .definefilter and .activatefilter), are always all evaluated, with a priority towards deny.

ip_allow.yaml however works by evaluating only filters up until one matches, and whatever action has triggered, is used.

This patch changes these remap filters to follow the behavior of ip_allow.yaml. First filter that matches is applied, and evaluation of filters stops.

I would really have liked to see this in 9.2.x, but I'm not so sure that it's a good idea since it does also break compatibility even if we consider this a bug.

This could possibly be considered a bug, but certainly it's
not a great behavior as it was. Before this patch, filters
(as defined and used with .definefilter and .activatefilter),
are always all evaluated, with a priority towards deny.

ip_allow.yaml however works by evaluating only filters up until
one matches, and whatever action has triggered, is used.

This patch changes these remap filters to follow the behavior
of ip_allow.yaml. First filter that matches is applied, and
evaluation of filters stops.

I would really have liked to see this in 9.2.x, but I'm not
so sure that it's a good idea since it does also break
compatibility even if we consider this a bug.
@zwoop zwoop added this to the 10.0.0 milestone Apr 21, 2023
@zwoop zwoop self-assigned this Apr 21, 2023
@zwoop
Copy link
Contributor Author

zwoop commented Apr 21, 2023

Seems likely that this would also address some of the issues reported in #1971. Thanks @mlibbey :).

@zwoop zwoop merged commit 1cccda1 into apache:master Apr 24, 2023
@zwoop zwoop deleted the FixRemapFilters branch April 24, 2023 23:18
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Jul 24, 2023
…ache#548)

This could possibly be considered a bug, but certainly it's
not a great behavior as it was. Before this patch, filters
(as defined and used with .definefilter and .activatefilter),
are always all evaluated, with a priority towards deny.

ip_allow.yaml however works by evaluating only filters up until
one matches, and whatever action has triggered, is used.

This patch changes these remap filters to follow the behavior
of ip_allow.yaml. First filter that matches is applied, and
evaluation of filters stops.

I would really have liked to see this in 9.2.x, but I'm not
so sure that it's a good idea since it does also break
compatibility even if we consider this a bug.

(cherry picked from commit 1cccda1)

Co-authored-by: Leif Hedstrom <zwoop@apache.org>
bneradt added a commit that referenced this pull request Feb 11, 2024
This continues in the vein of #9631 by implementing the following ACL changes for ATS 10 which makes remap.config ACLs behave more similarly to ip_allow.yaml:

* Fix @action=allow to deny transactions that are not in the allow list. Transactions with non-allowed methods simply didn't match the rule before, so they were not denied. This seems like a bug to me.
* Change in-line ACL rules to match before activated named ACL rules.
* If an ACL matches and is run, implicitly stop processing ip_allow.yaml rules.
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
* asf/master: (40 commits)
  Change remap filter behavior to match ip_allow.yaml (apache#9631)
  Cleanup: Get rid of dead code from Cache (apache#9621)
  Replace obsolete Debug() macro with Dbg() in SocksProxy.cc. (apache#9613)
  Updates for the new go-httpbin v2.6.0 release. (apache#9633)
  Fix debian symbol not found for test_HttpTransact (apache#9617)
  add traffic_ctl to cmake (apache#9628)
  Fix Proxy Protocol outbound (apache#9632)
  DOC: Fix variable name `proxy.config.exec_thread.autoconfig.enabled`. (apache#9629)
  traffic_ctl: metric monitor. Handle SIGINT to drop collected stats. (apache#9570)
  traffic_ctl: plugin msg command, print out the response from server. (apache#9610)
  Doc: document IP allow filter for remap. (apache#9626)
  Cleanup: Rename d with vol (apache#9619)
  Ensure a reason phrase when sending an HTTP/1 response (apache#9615)
  Cmake plugins and install things (apache#9597)
  quic: Fix session cleanup assert. (apache#9622)
  Enables switching SSL certificates on QUIC with QUICHE (apache#9347)
  Use FetchSM for OCSP HTTP requests (apache#9591)
  Make a couple of the threads configs correct (apache#9604)
  Change submit_and_wait to take ink_hrtime. Fix test_AIO for io_uring. (apache#9555)
  Update build_h3_tools for mac (apache#9608)
  ...
@bryancall bryancall mentioned this pull request Aug 14, 2024
91 tasks
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants