Skip to content

fix: surface the status of listeners when MergeGateways enabled#2672

Merged
Xunzhuo merged 6 commits intoenvoyproxy:mainfrom
shawnh2:listener-status-surface
Feb 23, 2024
Merged

fix: surface the status of listeners when MergeGateways enabled#2672
Xunzhuo merged 6 commits intoenvoyproxy:mainfrom
shawnh2:listener-status-surface

Conversation

@shawnh2
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 commented Feb 22, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #2668

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2 shawnh2 requested a review from a team as a code owner February 22, 2024 10:20
@shawnh2 shawnh2 requested a review from cnvergence February 22, 2024 10:21
@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Feb 22, 2024

/retest

…us of all gateways under gatewayclass into a method

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@cnvergence
Copy link
Copy Markdown
Member

/retest

Comment on lines +454 to +459

for _, gateway := range gateways.Items {
gateway := gateway
r.updateStatusForGateway(ctx, &gateway)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
for _, gateway := range gateways.Items {
gateway := gateway
r.updateStatusForGateway(ctx, &gateway)
}
if len(gateways.Items) > 0 {
for _, gateway := range gateways.Items {
gateway := gateway
r.updateStatusForGateway(ctx, &gateway)
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's add the check

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this extra check for the loop is necessary, since the loop will not start if gateways.Item is empty.

Instead I have added this check to return error if gateways.Item is empty, meaning no gateways found under the specific gatewayclass.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

works either way, thanks

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Feb 22, 2024

@shawnh2 @cnvergence trying to understand the bug here, there are 2 paths for writing back status for Gateway
1) from gateway-api->provider for Gateway.Status.Conditions
2) provider reconciles the deployment/service associated with the Gateway and we update the Gateway.Address ~~~~and also Gateway.Status.Conditions

The above PR is working on a fix in 2., trying to understand how that is related since the new Condition is coming from 1.
~~Was 2. wiping out 1. ? ~~

already answered here #2668 (comment)

…ays under certain gatewayclass

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 68.00000% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 63.43%. Comparing base (4c79ef9) to head (4a27d7c).
Report is 8 commits behind head on main.

Files Patch % Lines
internal/provider/kubernetes/predicates.go 68.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2672      +/-   ##
==========================================
+ Coverage   63.38%   63.43%   +0.05%     
==========================================
  Files         119      119              
  Lines       19098    19477     +379     
==========================================
+ Hits        12106    12356     +250     
- Misses       6193     6323     +130     
+ Partials      799      798       -1     

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

Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
Copy link
Copy Markdown
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

/lgtm

@Xunzhuo Xunzhuo merged commit 78f7edd into envoyproxy:main Feb 23, 2024
@shawnh2 shawnh2 deleted the listener-status-surface branch February 23, 2024 09:30
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.

Conflicted listener status is not surfaced for gateways when MergeGateways enabled

4 participants