Skip to content

xds: replace deprecated fields for Upstream and Downstream TlsContext - #7010

Merged
sanjaypujare merged 3 commits into
grpc:masterfrom
sanjaypujare:remove-deprecated-fields
May 7, 2020
Merged

xds: replace deprecated fields for Upstream and Downstream TlsContext#7010
sanjaypujare merged 3 commits into
grpc:masterfrom
sanjaypujare:remove-deprecated-fields

Conversation

@sanjaypujare

Copy link
Copy Markdown
Contributor

No description provided.

@sanjaypujare
sanjaypujare requested review from creamsoup and voidzcy May 5, 2020 23:42
@sanjaypujare
sanjaypujare force-pushed the remove-deprecated-fields branch from 9563cf8 to 009ae80 Compare May 6, 2020 01:14
updateBuilder.setUpstreamTlsContext(cluster.getTlsContext());
try {
UpstreamTlsContext upstreamTlsContext = getTlsContextFromCluster(cluster);
if (upstreamTlsContext != null && upstreamTlsContext.isInitialized()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isInitialized is always true for proto3 since it checks mandatory fields are set or not. this need to be fixed.
also, can upstreamTlsContext be null?

@sanjaypujare sanjaypujare May 6, 2020

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.

isInitialized only checks for mandatory fields? It's quite useless then. I'll then use hasCommonTlsContext() unless that too always returns true :-)

upstreamTlsContext can be null when we switch to v3 protos and cluster.getTlsContext() won't be there anymore so getTlsContextFromCluster will return null in that case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it was useful for proto2 because proto2 has required fields. in proto3, all fields are optional.

Listener xdsListener = Listener.fromEnvoyProtoListener(listener);
List<EnvoyServerProtoData.FilterChain> filterChains = xdsListener.getFilterChains();
EnvoyServerProtoData.FilterChain inFilter = filterChains.get(0);
assertThat(inFilter).isNotNull();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this always pass or it won't execute this line. i think you should check size of filterChains

private static DownstreamTlsContext getTlsContextFromFilterChain(
io.envoyproxy.envoy.api.v2.listener.FilterChain filterChain)
throws InvalidProtocolBufferException {
if (filterChain.hasTransportSocket() && "tls"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: && should be in front

if (condition
    && another_condition) {
  ...
}

@creamsoup creamsoup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@sanjaypujare

Copy link
Copy Markdown
Contributor Author

@voidzcy do you have any comments?

Comment thread xds/src/main/java/io/grpc/xds/XdsClientImpl.java Outdated
@sanjaypujare
sanjaypujare merged commit 67cc317 into grpc:master May 7, 2020
@sanjaypujare
sanjaypujare deleted the remove-deprecated-fields branch May 7, 2020 23:01
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 13, 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.

3 participants