Skip to content

fix: avoid appending same Backends twice to resource map#6021

Closed
mathetake wants to merge 3 commits intoenvoyproxy:mainfrom
mathetake:fixduplicates
Closed

fix: avoid appending same Backends twice to resource map#6021
mathetake wants to merge 3 commits intoenvoyproxy:mainfrom
mathetake:fixduplicates

Conversation

@mathetake
Copy link
Member

@mathetake mathetake commented May 12, 2025

Previously, backends are appended twice due to the overlapping functionality between processBackends and processBackendRefs functions. This removes the former as we don't need anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

or can we simply remove this function? at the end of the day, all referenced backends are properly added later in processBackendRefs. @arkodg

Copy link
Contributor

Choose a reason for hiding this comment

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

@guydc was this added for the case where an external gvk can reference a backend ?

Copy link
Contributor

@guydc guydc May 12, 2025

Choose a reason for hiding this comment

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

If I remember correctly, the main purpose was to add the backends so that we can update their status to reflect the situation where the backend flag is not enabled, regardless of them being referenced by any backendref.

We can change the behavior to reflect that in the xRoute/Policy resource status instead. I think that the UX change is not very significant here.

@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.43%. Comparing base (932b8b1) to head (695368e).
Report is 134 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6021      +/-   ##
==========================================
- Coverage   70.45%   70.43%   -0.02%     
==========================================
  Files         220      220              
  Lines       36593    36576      -17     
==========================================
- Hits        25780    25762      -18     
- Misses       9287     9288       +1     
  Partials     1526     1526              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mathetake added 3 commits May 30, 2025 11:20
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
Copy link
Member Author

The failure seems legit 🤔🧐

@arkodg arkodg added this to the v1.5.0-rc.1 Release milestone Jun 3, 2025
@arkodg
Copy link
Contributor

arkodg commented Jun 6, 2025

this exposed 2 issues
1.

2025-05-30T19:07:20.0088061Z     --- FAIL: TestE2E/OIDC_with_BackendCluster (102.45s)
2025-05-30T19:07:20.0088518Z         --- FAIL: TestE2E/OIDC_with_BackendCluster/oidc_provider_represented_by_a_BackendCluster (102.20s)

OIDC Backend is not being reconciled in

oidc := policy.Spec.OIDC

2025-05-30T19:07:20.0050347Z     --- FAIL: TestE2E/EnvoyGatewayBackend (60.39s)
2025-05-30T19:07:20.0050603Z         --- FAIL: TestE2E/EnvoyGatewayBackend/of_type_FQDN (60.02s)

The Backend the route is pointing to is backend-fqdn-http2 but the backend created is backend-fqdn, so the backendRef name needs to change here
https://github.com/envoyproxy/gateway/blob/main/test/e2e/testdata/httproute-to-backend-fqdn.yaml

can you address these in this PR @mathetake ? Or I can pick this one up

@mathetake
Copy link
Member Author

yeah i would appreciate it if you could pick it up...

@arkodg
Copy link
Contributor

arkodg commented Jun 6, 2025

sure will do

@arkodg arkodg self-assigned this Jun 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2025

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jul 7, 2025
@arkodg
Copy link
Contributor

arkodg commented Jul 9, 2025

cc @zirain can you help with this

@github-actions github-actions bot removed the stale label Jul 9, 2025
@zirain
Copy link
Member

zirain commented Jul 10, 2025

ok, I will pick this up.

@arkodg
Copy link
Contributor

arkodg commented Jul 15, 2025

PR was taken forward and addressed with #6506

@arkodg arkodg closed this Jul 15, 2025
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.

5 participants