Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[xserver] TLS support added to xserver, aggregator server, and aggregator client #4266
base: master
Are you sure you want to change the base?
[xserver] TLS support added to xserver, aggregator server, and aggregator client #4266
Changes from 7 commits
f252408
413d9d3
a57d5a6
3a206dc
f32c307
92e0920
4c798f8
8c4513a
446d2ef
d992b6f
74d9e77
09e549d
d841bcf
702563d
b89c701
94a5706
414711b
1ee36de
c450333
7997a01
8fd3c43
2c78899
19e99e0
3068241
1f1444d
4e05948
ec56885
f2594b2
d53144b
8582081
89e7e36
161a547
c4fb2d2
96aa72c
cae4342
6a44f2e
e848c97
a07157d
001ccb3
2e39e94
ed12ea4
2a2a17e
1b3ec54
5a81da4
b582bc5
8ba3134
e8b051c
f35027c
ace2a95
2e9ed48
343a770
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be caching the cert file? We recreate connections with some amount of frequency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe more appropriately, it just goes in the pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a TTL config cache on the connection level. It will be helpful for reconnecting but not for establishing new connections. The client has fewer connections than the server, so I believe it will be a neglectable overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, fair enough. Just for my understanding though--why not use the same cache for connecting as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because the write manager is responsible for creating connections(it can be considered as a connection pool). Theoretically, it can contain TLS options and use its TLS config cache to establish a connection. However, it knows nothing about connection parameters, and I don't want it to have that information because it doesn't seem to be something that should be responsible for secure connections. Also, even if it would manage the TLS configuration, a connection should do certificate rotation on its own in case of reconnection.