fix: avoid appending same Backends twice to resource map#6506
fix: avoid appending same Backends twice to resource map#6506zirain merged 4 commits intoenvoyproxy:mainfrom
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (13.04%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6506 +/- ##
==========================================
+ Coverage 70.68% 70.75% +0.07%
==========================================
Files 220 220
Lines 37722 37755 +33
==========================================
+ Hits 26662 26714 +52
+ Misses 9495 9485 -10
+ Partials 1565 1556 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
88e0dea to
48849ff
Compare
Signed-off-by: zirain <zirain2009@gmail.com>
|
|
||
| case egv1a1.KindBackend: | ||
| if !r.backendCRDExists { | ||
| r.log.Info("Backend CRD does not exist, skipping processing BackendRef") |
There was a problem hiding this comment.
Do we need/want this log output?
There was a problem hiding this comment.
I think it's fine to change level to 6, since we have status.
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
jukie
left a comment
There was a problem hiding this comment.
My approvals don't help much yet but LGTM!
your approvals is helpful, we need 1 from reviewer/mantainer, and 1 from maintainer. |
| backendRef = *oidc.Provider.BackendRef | ||
| } | ||
| if len(oidc.Provider.BackendRefs) > 0 { | ||
| backendRef = oidc.Provider.BackendRefs[0].BackendObjectReference |
There was a problem hiding this comment.
why arent we inserting all ?
There was a problem hiding this comment.
this bug also exists in ext auth :( , lets tackle it separately
There was a problem hiding this comment.
this change however breaks functionality, because before we maybe incorrectly processing here, but with processbackendRefs we were still processing all and they would be available in the gateway-api layer.
now since we have limited processing, its going to break those multi backendRef cases
There was a problem hiding this comment.
All the test case is showing about use just one backend, but I change the logic to keep same as before, we can fix it later.
supersede #6021