Skip to content
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.

Sign up for GitHub

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

Jump to bottom

change: change the default router from radixtree uri to radixtree hos… #9047

Merged
merged 9 commits into from
Mar 17, 2023
Merged

change: change the default router from radixtree uri to radixtree hos… #9047

merged 9 commits into from
Mar 17, 2023

Conversation

monkeyDluffy6017
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 commented Mar 10, 2023 •
edited
Loading

Description

Update the default HTTP router from radixtree_uri to radixtree_host_uri .

when Apache APISIX uses radixtree_uri as the default HTTP router,
there is a confusing scenario (even we can say it's a bug): Let's say
we have two routes, the first one requires the host matches
*.example.com, and the URI path is /anything, And the second one needs
the host matches foo.example.com exactly, and the URI path is also
/anything. In such a case, if we send an API request, we may hit the
first route, which is counterintuitive.
We also have a lot of voices from the community that users have been
in trouble since they think Apache APISIX will consider the HTTP host
match by default. However, it's not the case. So I'm here to propose
changing the default HTTP router to radixtree_host_uri.

Fixes #8354

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@monkeyDluffy6017 monkeyDluffy6017 marked this pull request as draft March 10, 2023 06:35
@monkeyDluffy6017 monkeyDluffy6017 marked this pull request as ready for review March 10, 2023 08:21
spacewander
spacewander reviewed Mar 13, 2023
View reviewed changes
Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

We need to change the route setting in https://github.com/apache/apisix/blob/master/t/router/radixtree-uri-host.t and other similar tests too.

docs/en/latest/terminology/router.md Outdated

:::note

In version 3.2 and earlier, APISIX used `radixtree_uri` as the default Router. `radixtree_uri` has better performance than `radixtree_host_uri`, so if you have higher performance requirements and can live with the fact that `radixtree_uri` only matches uri, consider continuing to use `radixtree_uri` as the default Router.
Copy link
Member

Choose a reason for hiding this comment

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

Only use the uri as the primary index, not only matches uri

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@monkeyDluffy6017
Copy link
Contributor Author

monkeyDluffy6017 commented Mar 13, 2023 •
edited
Loading

We need to change the route setting in https://github.com/apache/apisix/blob/master/t/router/radixtree-uri-host.t and other similar tests too.

Done

spacewander
spacewander approved these changes Mar 14, 2023
View reviewed changes
t/router/radixtree-method.t
apisix:
node_listen: 1984
router:
http: 'radixtree_uri'
Copy link
Member

Choose a reason for hiding this comment

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

Can't this test pass with the default router?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can pass, should we use the default router here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be better. Adding special configuration here makes it like a special router is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if it would be better to specify the routing engine for these test cases on router, so that the default engine is changed and the test cases are not affected

Copy link
Member

Choose a reason for hiding this comment

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

It's fine if you want to keep it.

@spacewander spacewander merged commit 1f27b72 into apache:master Mar 17, 2023
hongbinhsu added a commit to fitphp/apix that referenced this pull request Mar 17, 2023
@hongbinhsu
Merge branch 'upstream/master' into github/master
9c6f9b8 
* upstream/master: (46 commits)
  fix(consumer): work if the etcd connection failed during starting (apache#9077)
  ci: fix low disk space error when loading saved docker images (apache#9080)
  change: change the default router from radixtree uri to radixtree hos… (apache#9047)
  chore(deps): bump dubbo from 2.7.18 to 2.7.21 in /t/lib/dubbo-backend/dubbo-backend-provider (apache#9041)
  fix: cli test on master (apache#9075)
  fix: Non wildcard origin in CORS should sent Vary header (apache#9010)
  feat: bump lua-resty-ldap version for ldap-auth (apache#9037)
  fix: invalidate cache in core.request.add_haeder and fix some calls (apache#8824)
  docs: remove unnecessary getting-started.md (apache#9054)
  docs: contribute Getting Started tutorials (apache#9046)
  docs: replace full-width quotation mark with half-width quotation mark (apache#8887)
  docs: fix typo and grammar (apache#9008)
  feat: ready to release 2.15.3 (apache#9021)
  ci: ensure the test can run with different repo name (apache#8832)
  chore(deps): bump golang.org/x/net from 0.0.0-20220722155237-a158d28d115b to 0.7.0 in /ci/pod/openfunction/function-example/test-uri (apache#9018)
  docs: improved SEO & fixed typo and localization issues (apache#8993)
  feat: release APISIX 3.2.0 (apache#8988)
  docs: fix grammar (apache#8935)
  docs: fix indent in encrypted-storage-fields (apache#8876)
  docs: k8s discovery: state the restriction on the use of port number (apache#8969)
  ...
@Yilialinn Yilialinn mentioned this pull request Mar 30, 2023
Merged
@Gallardot Gallardot mentioned this pull request May 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacewander spacewander spacewander approved these changes

@moonming moonming moonming approved these changes

@soulbird soulbird soulbird approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change the default HTTP router from radixtree_uri to radixtree_host_uri
4 participants
@monkeyDluffy6017 @spacewander @soulbird @moonming