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

BTP Status getting modified by unrelated GatewayClass #2724

Closed
arkodg opened this issue Feb 28, 2024 · 4 comments · Fixed by #2802
Closed

BTP Status getting modified by unrelated GatewayClass #2724

arkodg opened this issue Feb 28, 2024 · 4 comments · Fixed by #2802
Assignees
Labels
Milestone

Comments

@arkodg
Copy link
Contributor

arkodg commented Feb 28, 2024

          facing the same issue with BTP as in e2e test https://github.com/envoyproxy/gateway/actions/runs/8073622448/job/22057769156?pr=2665#step:6:2130

#2665 introduce merge gateways e2e test, that requires multiple-gatewayclass per controller feature. but this test will affects all the other test cases that have policies attached. because the new gatewayclass in controller will also update the policy status, even the policy is not belong to new GC's.

so by fixing #2631, I think this issue can be resolved. assigning myself to these two issues.

Originally posted by @shawnh2 in #2520 (comment)

@arkodg
Copy link
Contributor Author

arkodg commented Feb 28, 2024

@shawnh2 I'm not sure how the upstream PolicyStatus field will help us here ?
to solve this for v1.0, I'm thinking lets just skip populating gwv1a2.PolicyReasonTargetNotFound as a workaround

status.SetBackendTrafficPolicyCondition(policy,

Its not ideal, but its at least takes care of all these false positives

So as a user, if I dont see any Accepted condition on my policy, I should be worried that it never got applied on anything

@arkodg
Copy link
Contributor Author

arkodg commented Feb 29, 2024

based on comments from kubernetes-sigs/gateway-api#2755 (comment) looks like skipping is the best option for all our policies

@shawnh2
Copy link
Contributor

shawnh2 commented Mar 5, 2024

+1, for now lets just skiping the PolicyReasonTargetNotFound as a workround.

And as for PolicyStatus, I think this can solve the issue. For all the fresh new policies, we can populate its ControllerName and ancestors by what it is targeting to. If one BTP is targeting to a HTTPRoute, its ancestors will be HTTPRoute, Gateway and GatewayClass. Likewise, if one BTP is targeting to a Gateway, its ancestors will be Gateway and GatewayClass.

By the next reconcile, the controller will first determine whether the policies should be handled by this controller via its PolicyStatus.ControllerName. And then determining whether the policies should be handled by this GatewayClass via its PolicyStatus.Ancestors[i], if it should, then determining by Gateway or HTTPRoute in order.

@arkodg
Copy link
Contributor Author

arkodg commented Mar 5, 2024

yah sounds good @shawnh2 for v1.0.0 prefer if we take the simple route forward on gracefully skipping PolicyReasonTargetNotFound
for all our policy types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants