Skip to content

fix: add unsupported status condition for filters within BackendRef#2620

Merged
arkodg merged 8 commits intoenvoyproxy:mainfrom
cnvergence:httproute-unsupported-filters
Feb 27, 2024
Merged

fix: add unsupported status condition for filters within BackendRef#2620
arkodg merged 8 commits intoenvoyproxy:mainfrom
cnvergence:httproute-unsupported-filters

Conversation

@cnvergence
Copy link
Copy Markdown
Member

@cnvergence cnvergence commented Feb 15, 2024 •
edited
Loading

What type of PR is this?

  • highlight unsupported BackendRef filters using a status condition
  • introduce BackendRefContext in translator

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.08%. Comparing base (72fadb7) to head (d59e52e).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2620      +/-   ##
==========================================
+ Coverage   63.00%   63.08%   +0.07%     
==========================================
  Files         122      122              
  Lines       19770    19811      +41     
==========================================
+ Hits        12457    12498      +41     
  Misses       6506     6506              
  Partials      807      807

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

@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 15, 2024

/retest

@cnvergence cnvergence marked this pull request as ready for review February 15, 2024 14:18
@cnvergence cnvergence requested a review from a team as a code owner February 15, 2024 14:18
arkodg
arkodg reviewed Feb 15, 2024
View reviewed changes
internal/gatewayapi/route.go Outdated
Copy link
Copy Markdown
Contributor

@arkodg arkodg Feb 15, 2024

Choose a reason for hiding this comment

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

should we continue, we have been failing fast and early in other places

Copy link
Copy Markdown
Member Author

@cnvergence cnvergence Feb 15, 2024

Choose a reason for hiding this comment

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

we could, let's fail fast then on this

Copy link
Copy Markdown
Contributor

@arkodg arkodg Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

rethinking again, this code should live inside validateBackendRef which gets called here

gateway/internal/gatewayapi/route.go

Line 1080 in ac02437

if !t.validateBackendRef(&backendRef, parentRef, route, resources, backendNamespace, routeType) {

which automatically converts the weight into a invalidWeight ensuring that a percentage of traffic gets 503'd

Copy link
Copy Markdown
Member Author

@cnvergence cnvergence Feb 16, 2024

Choose a reason for hiding this comment

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

looks good, but the field Filters is within gwapiv1.HTTPBackendRef or gwapiv1a1.GRPCBackendRef.
we process BackendRef based on gwapiv1.BackendRef

Copy link
Copy Markdown
Contributor

@arkodg arkodg Feb 16, 2024

Choose a reason for hiding this comment

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

we've had to do something similar for xRoutes, so we created RouteContext

gateway/internal/gatewayapi/contexts.go

Line 155 in cc88e9c

type RouteContext interface {

may need something similar here

Copy link
Copy Markdown
Member Author

@cnvergence cnvergence Feb 17, 2024

Choose a reason for hiding this comment

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

if we want to extend it, then sure
thanks for explaining

arkodg
arkodg reviewed Feb 15, 2024
View reviewed changes
@cnvergence cnvergence assigned cnvergence and unassigned cnvergence Feb 15, 2024
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 15, 2024

/retest

@cnvergence cnvergence force-pushed the httproute-unsupported-filters branch from fae5f06 to 063d326 Compare February 16, 2024 14:15
@cnvergence cnvergence force-pushed the httproute-unsupported-filters branch from 063d326 to 6a64316 Compare February 23, 2024 19:36
arkodg
arkodg reviewed Feb 24, 2024
View reviewed changes
@cnvergence cnvergence force-pushed the httproute-unsupported-filters branch from 5dfa61a to c76954d Compare February 26, 2024 10:03
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 26, 2024

/retest

arkodg
arkodg previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arkodg arkodg 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 for incorporating the feedback

@arkodg arkodg requested review from a team February 26, 2024 21:58
@cnvergence
add unsupported status condition for filters within BackendRef
4859d89 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
continue on unsupported backendref
f126666 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
continue on unsupported backendref
9f2da9e 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
add BackendRefContext
62817b5 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
fix gosec lint
28e3617 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
clean up old funcs
411513a 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
dedup code
b6c2a60 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence
update old code
d59e52e 
Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
@cnvergence cnvergence force-pushed the httproute-unsupported-filters branch from c76954d to d59e52e Compare February 26, 2024 23:40
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 26, 2024

sure, found the context in #1252

@shawnh2
Copy link
Copy Markdown
Contributor

shawnh2 commented Feb 27, 2024

/retest

@zirain
Copy link
Copy Markdown
Member

zirain commented Feb 27, 2024

/retest

1 similar comment
@cnvergence
Copy link
Copy Markdown
Member Author

cnvergence commented Feb 27, 2024

/retest

@arkodg arkodg merged commit a64c94b into envoyproxy:main Feb 27, 2024
@cnvergence cnvergence deleted the httproute-unsupported-filters branch February 27, 2024 17:12
@arkodg arkodg mentioned this pull request May 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg arkodg approved these changes

@shawnh2 shawnh2 shawnh2 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.

4 participants

@cnvergence @shawnh2 @zirain @arkodg