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
Implement Certificate Provider Framework (#19308) #19582
Implement Certificate Provider Framework (#19308) #19582
Changes from 20 commits
d4eb3cb
4db7266
18464b7
732fb63
3bcbe0a
45f9f19
3c822c0
c179df3
e712dbd
b052975
d31b1d7
7df7915
d87daaa
932b282
0c4e685
797ba3e
9d0ef89
bc4e2d0
7bf603a
d0acb6b
73c06f4
a1c21d2
ba78629
6361493
c3467b1
7f6af44
814d7ec
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.
I'm not sure how to define this metadata and make it can be utilized by other cases easily.
Currently I only have bumping case, in which case I need to generate certs based on a real server cert, so I need to pass the information of real server cert to the cert provider instance. How can I make this interface generic?
Anybody has ideas?
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.
@lizan @markdroth Could you help review this?
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.
Can you elaborate how this callback will be used? It's not very clear from the design doc or this PR.
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.
addOnDemandUpdateCallback
is used to add callback passed from worker thread.Assuming we have a network filter to request cert upon data plane request, it needs to add a callback to cert provider and pass metadata(e.g., SNI) to provider used for resource update, cert provider will update the cert resources based on the metadata (on main thread, all config updates should be on main thread due to envoy design principle ). Once the update is done, the callback is invoked, it might contains some logic needs to be run on main thread and other logic wrapped in another nested callback needs to be posted back to worker thread and resume the network filter chain.
The callback depends on feature. Above is just an example. But no matter what feature, we need pass metadata from data plane request to cert provider, this is why I was asking how to define this
OnDemandUpdateMetadata
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.
The name is still very confusing :) I need a concrete example (or TLS bumping filter) to better understand this. But IIUC we need to think the threading model more carefully. It sounds similar to what DNS cache does in terms of threading model, so you might want to take a look on that.
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.
Which name are you confused with? Is it
OnDemandUpdateMetadata
?Yes, it's similar to DNS cache, the same thing is we post the task to main thread to do DNS resolve or cert update, and callback to worker thread to continue the filter chain. For DNS, it passes the
host
as a parameter to the function used for DNS resolve, here we need to passOnDemandUpdateMetadata
as a parameter to the function used for cert generation. Since it's not just for bumping, we need it can cover future cases as much as possible. One alternative is listing all common config items as optional fields for cert generating inOnDemandUpdateMetadata
, e.g. SANs, pkey type(RSA or ECDSA) etc. What do you think about this?I think TLS bumping filter will not be ready very soon, before it's ready, let's we think about if there is anything we can do for this PR?
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.
Is this actually used in this PR? I was looking at how listener/cluster would delay until the callback fires.
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.
No, this is not used in this PR. We are implementing a local mimic cert provider (code not submitted yet) which will implement this API, and it will work with bumping filter to mimic certs for downstream.
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.
Ok, thanks. I think this particular callback is quite interesting, you can do on-demand SDS, for example. Once it's hooked into upstream/downstream, the failure mode should be more clear (what happens if too many requests are made for certs, or one request times out).