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

xds: mesh gateway CDS requests are now allowed to receive an empty CDS reply #6787

Merged
merged 2 commits into from
Nov 26, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Nov 13, 2019

This is the rest of the fix for #6543 that was incompletely fixed in #6576.

@rboyer rboyer added theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/envoy/xds Related to Envoy support labels Nov 13, 2019
@rboyer rboyer requested a review from a team November 13, 2019 18:48
@rboyer rboyer self-assigned this Nov 13, 2019
@@ -83,7 +83,7 @@ func (s *ConfigSnapshot) Valid() bool {
case structs.ServiceKindConnectProxy:
return s.Roots != nil && s.ConnectProxy.Leaf != nil
case structs.ServiceKindMeshGateway:
return s.Roots != nil && (s.MeshGateway.WatchedServicesSet || len(s.MeshGateway.WatchedServices) > 0)
return s.Roots != nil && (s.MeshGateway.WatchedServicesSet || len(s.MeshGateway.ServiceGroups) > 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is restoring the original length check which actually didn't have to get changed in the first PR.

@rboyer rboyer added this to the 1.6.x milestone Nov 14, 2019
@rboyer rboyer force-pushed the empty-mgw-ok-attempt2 branch from 0ddcd46 to cbde438 Compare November 25, 2019 16:36
@rboyer rboyer requested a review from s-mang November 25, 2019 16:36
@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #6787 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6787      +/-   ##
==========================================
- Coverage   65.82%   65.82%   -0.01%     
==========================================
  Files         431      431              
  Lines       52292    52285       -7     
==========================================
- Hits        34421    34415       -6     
- Misses      13747    13750       +3     
+ Partials     4124     4120       -4
Impacted Files Coverage Δ
agent/xds/server.go 78.1% <100%> (-0.74%) ⬇️
agent/proxycfg/snapshot.go 73.17% <100%> (ø) ⬆️
agent/consul/raft_rpc.go 78.72% <0%> (-4.26%) ⬇️
command/agent/agent.go 46.64% <0%> (-2.38%) ⬇️
agent/consul/rpc.go 78.57% <0%> (-1.13%) ⬇️
agent/consul/leader_connect.go 71.45% <0%> (-1.09%) ⬇️
agent/consul/connect_ca_endpoint.go 56.52% <0%> (-0.67%) ⬇️
command/debug/debug.go 65.97% <0%> (-0.6%) ⬇️
agent/config/builder.go 83.48% <0%> (+0.08%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40df8be...cbde438. Read the comment docs.

Copy link
Contributor

@s-mang s-mang left a comment

Choose a reason for hiding this comment

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

Looks fine, thanks for making that change.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM.

I'm still not 100% sure if the empty results filter is actually worth having at all. I added it originally thinking that was never valid. Then removed it to fix the "no healthy instances make sidecar hang" bug, then brought it back just because I realised there was a more semantically correct fix for that as noted in the comment.

At this point we've found another example of why it's valid so I'm totally fine with this as a conservative approach for now, but I feel like if we hit more reasonable cases where sending an empty response is correct and valid we should probably just ditch the check completely - it's one of those cases where "empty isn't necessarily the same as not-set-yet".

@rboyer rboyer merged commit 2011f3d into master Nov 26, 2019
@rboyer rboyer deleted the empty-mgw-ok-attempt2 branch November 26, 2019 21:55
@ghost
Copy link

ghost commented Jan 25, 2020

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

@ghost ghost locked and limited conversation to collaborators Jan 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants