Skip to content

MINOR: Refine route-acl rules to prevent unintended prefix matches #692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fabianonunes
Copy link
Contributor

@fabianonunes fabianonunes commented Jan 9, 2025

Since route-acl annotated rules take precedence over others, this PR updates its behavior to ensure they do not unintentionally overwrite other rules that share the same word prefix.

For example, a rule matching the path prefix /api should not inadvertently handle requests to /apiary.

To address this, the rule { path -m beg /api } has been replaced with a alternative that validates the URL's termination, ensuring it matches only /api$ or /api/.*:

# before this PR:
use_backend app_api_http if { var(txn.host) -m str api.demo } { path -m beg /api } { ... }

# after:
use_backend app_api_http if { var(txn.host) -m str api.demo } { path -m reg ^/api($|/) } { ... }

For better maintainability, this PR also refactors the AddCustomRoute function to eliminate redundancy introduced in commit c28d620. The updated code removes repetition without add extra spaces.

@fabianonunes fabianonunes marked this pull request as draft January 9, 2025 11:14
@fabianonunes fabianonunes force-pushed the route-acl-prefix-match branch 5 times, most recently from 7f273cc to d40fa9d Compare January 9, 2025 11:31
@fabianonunes fabianonunes marked this pull request as ready for review January 9, 2025 12:00
Copy link

stale bot commented Feb 8, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 8, 2025
@oktalz oktalz removed the stale label Feb 10, 2025
@fabianonunes
Copy link
Contributor Author

fabianonunes commented Mar 2, 2025

I'd like to know if there's anything I can do to facilitate the merge of this PR.

I understand that using regex might have some performance impact (though I'm not sure about this). If that's a concern, we could implement two separate rules instead:

# matches all paths that begin with `/api/`
use_backend app_api_http if { var(txn.host) -m str api.demo } { path -m beg /api/ } { ... }
# matches all requests with the exact path `/api`
use_backend app_api_http if { var(txn.host) -m str api.demo } { path -m str /api } { ... }

This approach would achieve the same goal of preventing unintended matches (like /apiary) while potentially avoiding any regex performance overhead, if that's the case. Please let me know if there are any other concerns I can address to help move this PR forward.

Regarding the failed check HAProxy check commit message, I've already added the word acl to the dictionary and force-pushed, but the job hasn't been re-executed yet.

Copy link

stale bot commented Apr 1, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 1, 2025
@stale stale bot closed this Apr 15, 2025
@fabianonunes
Copy link
Contributor Author

Hi, @oktalz.

Could you please keep this pull request open until it can be reviewed? It was closed automatically. If there's anything I can do to help move it forward, let me know.

Thank you!

@oktalz oktalz reopened this Apr 15, 2025
@stale stale bot removed the stale label Apr 15, 2025
@oktalz
Copy link
Member

oktalz commented Apr 15, 2025

@fabianonunes yes, we will take care of it soon

Copy link

stale bot commented May 15, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 15, 2025
@oktalz oktalz removed the stale label May 18, 2025
@ivanmatmati
Copy link
Collaborator

Hi @fabianonunes , we need to discuss if this is something we want to integrate because the documentation says:

Note that this annotation is not compatible with an Ingress having multiple paths that will match a request. Without this annotation, the precedence is given first to the longest matching path. But with the annotation, the first use_backend rule in the config that matches the request will be used.

Your PR could be an enhancement but we need to check if we want to go this direction. There's also a failing e2e test, can you check what happened ?

@fabianonunes fabianonunes force-pushed the route-acl-prefix-match branch from d40fa9d to 05b98e8 Compare May 29, 2025 11:43
Refactors the `AddCustomRoute` function to eliminate redundancy introduced in commit c28d620.
The updated code removes repetition without add extra spaces.
Since `route-acl` annotated rules take precedence over others, this
commit updates its behavior to ensure they do not unintentionally
overwrite other rules that share the same prefix.

For example, a rule matching the path /api should not inadvertently
handle requests to /apiary.
@fabianonunes fabianonunes force-pushed the route-acl-prefix-match branch from 05b98e8 to 54bbfdf Compare May 29, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants