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

api: recomputeRoute field in JWT #2612

Merged
merged 5 commits into from
Feb 23, 2024
Merged

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Feb 14, 2024

Add a field called useForRouting that signals to Envoy Gateway that the headers generated from the claims are used to make routing decisions

Internally this field will be used to
* insert a catch-all route with a 404 direct response identical to #2586 which makes sure the jwt filter with claimToHeader is applied before recomputing routing decision

Relates to #2452

@arkodg arkodg requested a review from a team as a code owner February 14, 2024 23:49
@arkodg
Copy link
Contributor Author

arkodg commented Feb 14, 2024

outlining a few other alternatives

  • add a similar field at top level within jwt instead of creating it within jwt.claimToHeaders , which will have the same effect internally, since these settings internally are applied at top level
  • automatically enable these settings w/o an API field if jwt.claimToHeaders is set. One downside here is, enabling clearRouteCache is expensive and may impact performance. This may not be ideal for a user who wants to jwt.claimToHeaders for ratelimiting

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.33%. Comparing base (fc7d6bc) to head (bc439f0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2612   +/-   ##
=======================================
  Coverage   63.33%   63.33%           
=======================================
  Files         119      119           
  Lines       19211    19211           
=======================================
  Hits        12168    12168           
  Misses       6244     6244           
  Partials      799      799           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

api/v1alpha1/jwt_types.go Outdated Show resolved Hide resolved
@arkodg arkodg requested a review from shawnh2 February 15, 2024 02:04
Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@yuluo-yx yuluo-yx left a comment

Choose a reason for hiding this comment

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

LGTM!

@arkodg arkodg force-pushed the claims-for-routing branch 2 times, most recently from fc1593f to eaef9f8 Compare February 20, 2024 21:38
@arkodg
Copy link
Contributor Author

arkodg commented Feb 20, 2024

ptal @guydc @zhaohuabing, I've limited the use of the field to only recalculate route, and not add a fallback/catch-all route,
we can use docs to add that manually

@arkodg
Copy link
Contributor Author

arkodg commented Feb 20, 2024

also looking at advice on API field naming, this is what ChatGPT said

Here are some alternate API field names for recomputeRoute:

calculateRoute
updateRoute
computeNewRoute
refreshRoute
recalculateRoute

Copy link
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

+1 to updateRoute

// This field must be enabled if the headers generated from the claim are used for
// route matching decisions.
//
// +optional
Copy link
Contributor

@guydc guydc Feb 20, 2024

Choose a reason for hiding this comment

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

Do we need to state something about the impact on other features? e.g. filters that fired prior to jwt (CORS, ExtAuth, Basic, ...) will only execute in the context of the initial match. Also, later filters (Fault, RL, ...) will only execute in the context of the new route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment around this

@zhaohuabing
Copy link
Member

recomputeRoute sounds good to me, "updateRoute" is a bit ambiguous.

zirain
zirain previously approved these changes Feb 21, 2024
@arkodg arkodg changed the title api: useForRouting field in JWT api: recomputeRoute field in JWT Feb 23, 2024
Add a field called `useForRouting` that signals to Envoy Gateway
that the headers generated from the claims are used to make routing
decisions

Internally this field will be used to
* insert a catch-all route with a 404 direct response
identical to envoyproxy#2586 which
makes sure the jwt filter with `claimToHeader` is applied before
recomputing routing decision
* enable `clear_route_cache` to recompute routing decision
https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/jwt_authn/v3/config.proto#extensions-filters-http-jwt-authn-v3-jwtprovider

Relates to envoyproxy#2452

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@arkodg arkodg merged commit 620053e into envoyproxy:main Feb 23, 2024
22 checks passed
@arkodg arkodg deleted the claims-for-routing branch February 23, 2024 03:45
This pull request was closed.
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.

6 participants