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: fix ServerXdsClient to return subscribed resources only for LDS #7689

Merged
merged 5 commits into from
Dec 10, 2020

Conversation

sanjaypujare
Copy link
Contributor

No description provided.

@sanjaypujare sanjaypujare added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 3, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 3, 2020
@sanjaypujare sanjaypujare added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 3, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 3, 2020
@sanjaypujare sanjaypujare added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 4, 2020
@grpc-kokoro grpc-kokoro removed kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary labels Dec 4, 2020
@voidzcy
Copy link
Contributor

voidzcy commented Dec 4, 2020

Some comment on other small issues in ServerXdsClient code. They are not related to this PR, most of them should be cosmetic things:

return address.hasSocketAddress() && (address.getSocketAddress().getPortValue() == portToMatch);

  • The field name newServerApi is not informative. It doesn't say imply anything useful.

  • The following small chunk of code involves 3-4 indirections of calling helper methods. There is very little code reuse involved while logics are fragmented into multiple pieces. It's a pretty bad review experience. It could be refactored and condensed.

private boolean isRequestedListener(Listener listener) {
if (newServerApi) {
return grpcServerResourceId.equals(listener.getName())
&& listener.getTrafficDirection().equals(TrafficDirection.INBOUND)
&& isAddressMatching(listener.getAddress(), listenerPort);
}
return isAddressMatching(listener.getAddress(), 15001)
&& hasMatchingFilter(listener.getFilterChainsList());
}
private boolean isAddressMatching(Address address, int portToMatch) {
return address.hasSocketAddress() && (address.getSocketAddress().getPortValue() == portToMatch);
}
private boolean hasMatchingFilter(List<FilterChain> filterChainsList) {
// TODO(sanjaypujare): if myIp to be checked against filterChainMatch.getPrefixRangesList()
for (FilterChain filterChain : filterChainsList) {
FilterChainMatch filterChainMatch = filterChain.getFilterChainMatch();
if (listenerPort == filterChainMatch.getDestinationPort().getValue()) {
return true;
}
}
return false;
}

@sanjaypujare
Copy link
Contributor Author

Some comment on other small issues in ServerXdsClient code. They are not related to this PR, most of them should be cosmetic things:

Probably valid comments and I'll fix some. Note that the newServerApi variable is going to go away and all the code for newServerApi == false is going to go away once TD code is in prod and tested. So not spending time to address those comments

@@ -636,6 +636,7 @@ public void streamClosedAndRetry() {
verify(requestObserver)
.onNext(eq(buildDiscoveryRequest(getNodeToVerify(), "",
ResourceType.LDS.typeUrlV2(), "")));
verifyNoMoreInteractions(requestObserver);
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the two copies of tests have already diverged. But it is minor...

@sanjaypujare sanjaypujare merged commit 20fc907 into grpc:master Dec 10, 2020
@sanjaypujare sanjaypujare deleted the server-xds-client-fix branch December 10, 2020 01:42
sanjaypujare added a commit to sanjaypujare/grpc-java that referenced this pull request Dec 10, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants