Skip to content

Fixes Calculating ParentRef Status Conditions#563

Merged
danehans merged 2 commits intoenvoyproxy:mainfrom
danehans:issue_562
Oct 17, 2022
Merged

Fixes Calculating ParentRef Status Conditions#563
danehans merged 2 commits intoenvoyproxy:mainfrom
danehans:issue_562

Conversation

@danehans
Copy link
Contributor

@danehans danehans commented Oct 13, 2022

Previously, ProcessHTTPRoutes() of the gatewayapi translator would set "Accepted=True" as the default status condition. With #516 the status conditions get reset for every HTTPRoute reconciliation and "Accepted=True" should only be set if no status conditions for the parentRef exist.

Fixes: #562

Signed-off-by: danehans daneyonhansen@gmail.com

Alice-Lilith
Alice-Lilith previously approved these changes Oct 13, 2022
Copy link
Member

@Alice-Lilith Alice-Lilith left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the fix.

Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans danehans added the kind/bug Something isn't working label Oct 14, 2022
@danehans danehans added this to the 0.2.0 milestone Oct 14, 2022
@danehans danehans requested a review from skriss October 14, 2022 21:49
@chauhanshubham
Copy link
Member

chauhanshubham commented Oct 15, 2022

@danehans I think this will be required for TLSRoute as well, could you please add a similar logic there too. thanks.

Signed-off-by: danehans <daneyonhansen@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #563 (4280bbf) into main (2103195) will decrease coverage by 0.27%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
- Coverage   60.40%   60.12%   -0.28%     
==========================================
  Files          45       46       +1     
  Lines        5564     5593      +29     
==========================================
+ Hits         3361     3363       +2     
- Misses       1996     2017      +21     
- Partials      207      213       +6     
Impacted Files Coverage Δ
internal/gatewayapi/translator.go 84.33% <85.00%> (-1.19%) ⬇️
internal/provider/kubernetes/tlsroute.go 53.02% <0.00%> (-6.70%) ⬇️
internal/infrastructure/kubernetes/configmap.go 75.86% <0.00%> (-6.60%) ⬇️
internal/provider/kubernetes/helpers.go 73.91% <0.00%> (-6.53%) ⬇️
...ternal/infrastructure/kubernetes/serviceaccount.go 77.96% <0.00%> (-4.80%) ⬇️
internal/gatewayapi/contexts.go 76.02% <0.00%> (-4.12%) ⬇️
internal/provider/kubernetes/httproute.go 58.51% <0.00%> (-0.62%) ⬇️
internal/provider/kubernetes/gateway.go 50.21% <0.00%> (-0.55%) ⬇️
internal/envoygateway/config/config.go 0.00% <0.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@danehans danehans requested a review from Alice-Lilith October 15, 2022 17:24
@danehans
Copy link
Contributor Author

danehans commented Oct 15, 2022

I think this will be required for TLSRoute as well, could you please add a similar logic there too. thanks.

@chauhanshubham 4280bbf adds a condition length check to ProcessTLSRoutes() before setting the default "Accepted=True" condition. Otherwise, TLSRoutes have no filters so we're good.

@danehans danehans merged commit 5105ab3 into envoyproxy:main Oct 17, 2022
@danehans danehans deleted the issue_562 branch October 17, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTPRoute with URL Rewrite Produces Accepted=True HTTPRoute Status Condition

5 participants