Skip to content

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

Merged
spacewander merged 9 commits into
apache:masterfrom
juststillthinking:change-the-default-router-from-radixtree-uri-to-radixtree-host-uri
Mar 17, 2023
Merged

change: change the default router from radixtree uri to radixtree hos…#9047
spacewander merged 9 commits into
apache:masterfrom
juststillthinking:change-the-default-router-from-radixtree-uri-to-radixtree-host-uri

Conversation

@juststillthinking

@juststillthinking juststillthinking commented Mar 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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)

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

@spacewander spacewander left a comment

Copy link
Copy Markdown
Member

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.

Comment thread 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.

@spacewander spacewander Mar 13, 2023

Copy link
Copy Markdown
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

@juststillthinking juststillthinking Mar 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@juststillthinking

juststillthinking commented Mar 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
Comment thread t/router/radixtree-method.t
apisix:
node_listen: 1984
router:
http: 'radixtree_uri'

@spacewander spacewander Mar 14, 2023

Copy link
Copy Markdown
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?

@juststillthinking juststillthinking Mar 14, 2023

Copy link
Copy Markdown
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?

@spacewander spacewander Mar 16, 2023

Copy link
Copy Markdown
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.

@juststillthinking juststillthinking Mar 16, 2023

Copy link
Copy Markdown
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

@spacewander spacewander Mar 17, 2023

Copy link
Copy Markdown
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
@soulbird soulbird soulbird approved these changes
@moonming moonming moonming 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

@juststillthinking @spacewander @soulbird @moonming