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

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

Merged
merged 8 commits into from
Feb 27, 2024
Merged

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

merged 8 commits into from
Feb 27, 2024

Conversation

cnvergence
Copy link
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 Codecov
Copy link

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
Member Author

/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
@@ -173,6 +173,15 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
dstAddrTypeMap := make(map[ir.DestinationAddressType]int)

for _, backendRef := range rule.BackendRefs {
if len(backendRef.Filters) > 0 {
Copy link
Contributor

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
Member Author

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
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
Member Author

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
Contributor

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
Member Author

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

@cnvergence cnvergence assigned cnvergence and unassigned cnvergence Feb 15, 2024
@cnvergence
Copy link
Member Author

/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
@cnvergence cnvergence force-pushed the httproute-unsupported-filters branch from 5dfa61a to c76954d Compare February 26, 2024 10:03
@cnvergence
Copy link
Member Author

/retest

arkodg
arkodg previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
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 cnvergence force-pushed the httproute-unsupported-filters branch from c76954d to d59e52e Compare February 26, 2024 23:40
@cnvergence
Copy link
Member Author

sure, found the context in #1252

@shawnh2
Copy link
Contributor

shawnh2 commented Feb 27, 2024

/retest

@zirain
Copy link
Member

zirain commented Feb 27, 2024

/retest

1 similar comment
@cnvergence
Copy link
Member Author

/retest

@arkodg arkodg merged commit a64c94b into envoyproxy:main Feb 27, 2024
17 checks passed
@cnvergence cnvergence deleted the httproute-unsupported-filters branch February 27, 2024 17:12
zirain pushed a commit to zirain/gateway that referenced this pull request Feb 28, 2024
@cnvergence @zirain
fix: add unsupported status condition for filters within BackendRef (e…
ac72796 
…nvoyproxy#2620)

* add unsupported status condition for filters within BackendRef

Signed-off-by: Karol Szwaj <[email protected]>

* continue on unsupported backendref

Signed-off-by: Karol Szwaj <[email protected]>

* continue on unsupported backendref

Signed-off-by: Karol Szwaj <[email protected]>

* add BackendRefContext

Signed-off-by: Karol Szwaj <[email protected]>

* fix gosec lint

Signed-off-by: Karol Szwaj <[email protected]>

* clean up old funcs

Signed-off-by: Karol Szwaj <[email protected]>

* dedup code

Signed-off-by: Karol Szwaj <[email protected]>

* update old code

Signed-off-by: Karol Szwaj <[email protected]>

---------

Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: zirain <[email protected]>
zirain pushed a commit to zirain/gateway that referenced this pull request Feb 28, 2024
@cnvergence @zirain
fix: add unsupported status condition for filters within BackendRef (e…
30f598b 
…nvoyproxy#2620)

* add unsupported status condition for filters within BackendRef

Signed-off-by: Karol Szwaj <[email protected]>

* continue on unsupported backendref

Signed-off-by: Karol Szwaj <[email protected]>

* continue on unsupported backendref

Signed-off-by: Karol Szwaj <[email protected]>

* add BackendRefContext

Signed-off-by: Karol Szwaj <[email protected]>

* fix gosec lint

Signed-off-by: Karol Szwaj <[email protected]>

* clean up old funcs

Signed-off-by: Karol Szwaj <[email protected]>

* dedup code

Signed-off-by: Karol Szwaj <[email protected]>

* update old code

Signed-off-by: Karol Szwaj <[email protected]>

---------

Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: zirain <[email protected]>
@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

@shawnh2 shawnh2 shawnh2 approved these changes

@arkodg arkodg arkodg 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