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.

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

Bug with catch-all routing #2169

Open
istanko opened this issue Oct 9, 2024 · 13 comments
Open

Bug with catch-all routing #2169

istanko opened this issue Oct 9, 2024 · 13 comments
Labels
Autumn'24 Autumn 2024 release bug Identified as a potential bug Configuration Ocelot feature: Configuration Routing Ocelot feature: Routing
Milestone

Comments

@istanko
Copy link

istanko commented Oct 9, 2024

Hello,

I have a problem with two get routes. One of them is not getting hit.

Expected Behavior / New Feature

With the following configuration, expected behavior should be that calling /api/products?query=test is redirected to the downstream path template /api/products?query=test, and that calling /api/products/1 is redirected to the downstream path template /api/products/1

{
  "UpstreamPathTemplate": "/api/products?{everything}",
  "UpstreamHttpMethod": [
    "Get",
    "Options"
  ],
  "DownstreamPathTemplate": "/api/products-something?{everything}",
  "DownstreamScheme": "http",
  "DownstreamHostAndPorts": [
    {
      "Host": "localhost",
      "Port": 80
    }
  ]
},
{
  "UpstreamPathTemplate": "/api/products/{id}",
  "UpstreamHttpMethod": [
    "Get",
    "Options"
  ],
  "DownstreamPathTemplate": "/api/products/{id}",
  "DownstreamScheme": "http",
  "DownstreamHostAndPorts": [
    {
      "Host": "localhost",
      "Port": 80
    }
  ]
}

Actual Behavior / Motivation for New Feature

Calling /api/products/1 will be routed to downstream path template /api/products?1

Steps to Reproduce the Problem

Use the provided configuration and make calls to both endpoints.

Specifications

  • Version: 23.3.4
  • Platform: .NET 8
  • Subsystem: Windows
@istanko istanko closed this as completed Oct 10, 2024
@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Hello, Mr. I. Stanko! What's happening? Also, why was the issue closed? Was there no bug? :)

@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

{
  "UpstreamPathTemplate": "/api/products?{everything}",
  "DownstreamPathTemplate": "/api/products-something?{everything}",
 },
{
  "UpstreamPathTemplate": "/api/products/{id}",
  "DownstreamPathTemplate": "/api/products/{id}",
}

The configuration appears to be slightly incorrect. The 2nd route is a catch-all, while the first route processes all query strings. Defining the 1st route when the 2nd one is a catch-all seems redundant, as the 2nd route should be able to process all URLs from the first route, including all query strings.

Merging both routes into one could be done as follows:

{
  "UpstreamPathTemplate": "/api/products/{everything}",
  "DownstreamPathTemplate": "/api/products/{everything}",
}

Wouldn't this approach be effective?

@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Please read this doc carefully → Empty Placeholders

@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Actual Behavior / Motivation for New Feature

Calling /api/products/1 will be routed to downstream path template /api/products?1

Steps to Reproduce the Problem

Use the provided configuration and make calls to both endpoints.

Are you sure? 😉
Please attach logs of your testing session with log level Debug
And we will continue this funny talk...


Calling /api/products/1 will be routed to downstream path template /api/products?1

This ought to be verified by our routing tests, and it's concerning if it's not, as it may indicate a bug.
Yes, you've reported the old problem: here is more Routing fun

@raman-m raman-m reopened this Oct 10, 2024
@raman-m raman-m added bug Identified as a potential bug Configuration Ocelot feature: Configuration Routing Ocelot feature: Routing labels Oct 10, 2024
@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Duplicate of #2132

@raman-m raman-m marked this as a duplicate of #2132 Oct 10, 2024
@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Duplicate of #2064

@raman-m raman-m marked this as a duplicate of #2064 Oct 10, 2024
@istanko
Copy link
Author

istanko commented Oct 10, 2024

{
  "UpstreamPathTemplate": "/api/products?{everything}",
  "DownstreamPathTemplate": "/api/products-something?{everything}",
 },
{
  "UpstreamPathTemplate": "/api/products/{id}",
  "DownstreamPathTemplate": "/api/products/{id}",
}

The configuration appears to be slightly incorrect. The 2nd route is a catch-all, while the first route processes all query strings. Defining the 1st route when the 2nd one is a catch-all seems redundant, as the 2nd route should be able to process all URLs from the first route, including all query strings.

Merging both routes into one could be done as follows:

{
  "UpstreamPathTemplate": "/api/products/{everything}",
  "DownstreamPathTemplate": "/api/products/{everything}",
}

Wouldn't this approach be effective?

Hello,

Thanks for reply. The problem here is that the downstream route is different.

@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Thanks for reply. The problem here is that the downstream route is different.

OK, but when you have one service instance with different downstream paths, why do merge paths in upstream?
You have to have different upstream paths too!
Your final solution is:

{
  "UpstreamPathTemplate": "/api/products-something?{everything}",
  "DownstreamPathTemplate": "/api/products-something?{everything}",
},
{
  "UpstreamPathTemplate": "/api/products/{id}",
  "DownstreamPathTemplate": "/api/products/{id}",
}

and moreover, it must be one Catch All route ❗

{
  "UpstreamPathTemplate": "/{everything}",
  "DownstreamPathTemplate": "/{everything}",
},

Don't say "thank you"

@raman-m
Copy link
Member

raman-m commented Oct 10, 2024

Mr. I, what's your full name?
How do you use Ocelot now? Learning?
Do you have Ocelot app in Production environment?

@raman-m raman-m self-assigned this Oct 10, 2024
@raman-m raman-m added the Oct'24 October 2024 release label Oct 10, 2024
@raman-m raman-m added this to the October'24 milestone Oct 10, 2024
@istanko
Copy link
Author

istanko commented Oct 10, 2024

Thanks for reply. The problem here is that the downstream route is different.

OK, but when you have one service instance with different downstream paths, why do merge paths in upstream? You have to have different upstream paths too! Your final solution is:

{
  "UpstreamPathTemplate": "/api/products-something?{everything}",
  "DownstreamPathTemplate": "/api/products-something?{everything}",
},
{
  "UpstreamPathTemplate": "/api/products/{id}",
  "DownstreamPathTemplate": "/api/products/{id}",
}

and moreover, it must be one Catch All route ❗

{
  "UpstreamPathTemplate": "/{everything}",
  "DownstreamPathTemplate": "/{everything}",
},

Don't say "thank you"

The thing here is that the {id} should not be Catch All, but placeholder for variable.

@raman-m raman-m added Autumn'24 Autumn 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024
@raman-m raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024
@raman-m
Copy link
Member

raman-m commented Oct 26, 2024

Mr. @istanko, will you open a PR in November'24?

@raman-m
Copy link
Member

raman-m commented Nov 8, 2024

@int0x81, welcome to your assignment!

@raman-m raman-m removed their assignment Nov 8, 2024
@ggnaegi
Copy link
Member

ggnaegi commented Nov 13, 2024

@istanko could you please check if you observe the same behavior with #2200? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Autumn'24 Autumn 2024 release bug Identified as a potential bug Configuration Ocelot feature: Configuration Routing Ocelot feature: Routing
Projects
Status: Todo
Development

No branches or pull requests

3 participants