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

[Feature] Add Initial Metadata for Sds Grpc Client; Support Sds Grpc Client Multiplexing #1960

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

antJack
Copy link
Contributor

@antJack antJack commented Feb 17, 2022

Related Issue: #1959

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #1960 (b68f9a2) into master (a7c65f0) will increase coverage by 0.07%.
The diff coverage is 89.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1960      +/-   ##
==========================================
+ Coverage   61.51%   61.58%   +0.07%     
==========================================
  Files         398      398              
  Lines       34343    34391      +48     
==========================================
+ Hits        21125    21179      +54     
+ Misses      11133    11123      -10     
- Partials     2085     2089       +4     
Impacted Files Coverage Δ
pkg/mtls/secret_manager.go 82.63% <66.66%> (-0.70%) ⬇️
istio/istio152/sds/sds.go 76.40% <92.30%> (+3.74%) ⬆️
pkg/mtls/factory.go 81.25% <100.00%> (ø)
pkg/mtls/sds/client.go 87.50% <100.00%> (+0.83%) ⬆️
pkg/mtls/sds/subscriber.go 62.74% <0.00%> (-11.77%) ⬇️
pkg/module/http2/transport.go 78.47% <0.00%> (-0.29%) ⬇️
pkg/mosn/init.go 28.08% <0.00%> (ø)
pkg/module/http2/server.go 81.92% <0.00%> (+0.14%) ⬆️
pkg/server/handler.go 40.07% <0.00%> (+0.19%) ⬆️
pkg/upstream/servicediscovery/dubbod/bootstrap.go 68.75% <0.00%> (+4.16%) ⬆️
... and 1 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 a7c65f0...b68f9a2. Read the comment docs.

@@ -132,6 +144,11 @@ func (s *fakeSdsServer) StreamSecrets(stream envoy_sds.SecretDiscoveryService_St
log.DefaultLogger.Infof("server receive a ack request: %v", req)
continue
}
if len(s.requiredMetadata) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里完整的逻辑 是不是应该是 server里 针对不同的metadata 会返回不同的证书?只是因为目前这个server 没有实现这些逻辑,才这样写的?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

作为 sds client 的测试只需要确保 metadata 能够被 server 收到就行了,至于 server 返回不同证书这点,并不影响结果,所以也不需要在 client 的单测里模拟

Copy link
Contributor

Choose a reason for hiding this comment

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

其实这里还是有一些影响。设计的时候应该没有考虑过,就是比如都使用default 作为sds证书的名字,但是一个配置有metadata,一个配置没有metadata,这里就会有问题。
如果说不同的证书一定是不同的名字,那么metadata可能就是用于server去生成证书的参数需要?这里还是要明确一下行为的

Copy link
Contributor

Choose a reason for hiding this comment

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

其实这里还是有一些影响。设计的时候应该没有考虑过,就是比如都使用default 作为sds证书的名字,但是一个配置有metadata,一个配置没有metadata,这里就会有问题。 如果说不同的证书一定是不同的名字,那么metadata可能就是用于server去生成证书的参数需要?这里还是要明确一下行为的

有这样一个场景,返回的都是应用的证书(也就是证书的名字是同一个),但是在某些场景下需要返回该应用的国密证书,某些场景下需要返回国际证书,所以这个地方就需要设置一些 metadata 信息告知 nodeagent。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix, 按照设计,现在允许不同 tls 配置共用 default 作为 sds 证书的名字,底层 sds client 实现会根据 tls 配置的哈希值来创建不同的 grpc stream:拉取国际证书 (tls 配置不含 metadata) 和拉取国密证书 (tls 配置包含 metadata) 两个场景将使用不同的 stream,但底层共用同一条 uds 连接。

istio/istio152/sds/sds.go Show resolved Hide resolved
istio/istio152/sds/sds_test.go Show resolved Hide resolved
@antJack antJack changed the title [Feature] add initial metadata for sds grpc client [Feature] Add Initial Metadata for Sds Grpc Client; Support Sds Grpc Client Multiplexing Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants