-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
WiP Add Bumping filter #22582
WiP Add Bumping filter #22582
Conversation
Introduce a CertificateProviderManager to parse the certificate_provider_instances config and instantiate certificate providers. Signed-off-by: Luyao Zhong <[email protected]>
* rename default_cert_provider to static_cert_provider * nest struct Certpair inside of the CertificateProvider class * avoid the copy of certificate provider config * update CertificateProvider interfaces * fix format_pre CI failure Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
StaticCertificateProvider supports generating identity certificates, it is supposed to be renamed later. Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
DefaultCertificateProvider supports generating certificates for handshake which shows how CertificateProvider asynchronous interfaces work. Signed-off-by: Luyao Zhong <[email protected]>
…r-instances Signed-off-by: Luyao Zhong <[email protected]>
…allbacks Signed-off-by: Luyao Zhong <[email protected]>
…r-instances Signed-off-by: Luyao Zhong <[email protected]>
…r-instances Signed-off-by: Luyao Zhong <[email protected]>
implement ca_certificate_provider_instance in CertificateValidationContext implement tls_certificate_provider_instance in CommonTlsContext Signed-off-by: Luyao Zhong <[email protected]>
…r-instances Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
consider current implementation for tls certificate config loading, certificate provider shoule provide at least one tls certificate, otherwise the Envoy will complain when loading the config. Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
Signed-off-by: Luyao Zhong <[email protected]>
Hi @liverbirdkte, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
should I add a writeDisable method to connection to stop triggering write event? I have to make sure prefetch cert happens before downstream handshake. |
@liverbirdkte Thank you for the contribution. However you did not indicate if this extension is sponsored by an existing maintainer. Please see the guideline https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md Without sponsorship you can add this extension to the |
assigning to Matt to first validate whether this should be an 'extension' or should be in 'contrib'. Also you need to fix format before CI can really run. /wait |
@liverbirdkte is on vacation, he will be back next week. I believe he was to ask for comments, this PR is not ready for test and merge. It seems not only us require this feature. During discussion on #18928, others want this feature as well. So maybe we should let it be a core extension. Not sure how to define if a filter is worth to be a core extension. But if it is not worth after your evaluation, contrib extensions still require an end-user sponsor according to https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md, then VMware can support us, we are collaborating with them. |
VMware is not an end user. Unless there is a known production use case this will need to go into contrib, and it can only go into contrib if there is a vetted end user sponsor. I'm becoming increasingly uncomfortable with the number of Intel extensions that have no known use production case. Feel free to follow up over email. Thanks! |
@mattklein123 hi, VMware users of this extension here. We have been using for years now Envoy as part of the Istio distribution that is embedded in Tanzu Service Mesh (SaaS). Contour also uses Envoy. Now, Envoy is a core component of a new product in the multi-cloud security space that runs in a PoP and is a SaaS service (user to app, and app to app traffic). If the inclusion criteria (as Users) is the one in https://github.com/envoyproxy/envoy/blob/main/SECURITY.md#membership-criteria then I believe that we fit in at least one of these categories
There is a lot of documentation of VMware regarding these initiatives, including the full disclosure of the use of Envoy. If VMware does meet the inclusion criteria now, it may be worth reviewing VMware as the last review was more than a year ago (06/21). If VMware does not meet it, could you please add more detail / help me better understand the criteria? |
If VMware is going to use this in a product, then it probably shouldn't be in contrib. But either way I would prefer to get some bake time on it as it has unclear usage to me. So since you will need to compile this yourself anyway, why don't you just maintain a private extension for a bit and then we can circle back for upstream inclusion once it has been vetted? Thank you. |
Yes we can do that. What would be an estimated baking time (with very high error margin)? I just want to make sure that if we use an "internal" extension, it can be upstreamed or in the end we would end up with a custom Envoy (and we do not want this in a production release of our products). Just to be clear: any product that uses Envoy in a data plane that manages inter cloud or user to cloud traffic needs TLS interception. I believe that's the reason of the interest of the community. |
If this is going to be in a public product and you are going to use public envoy, by definition it can't be in contrib. At the same time, if the feature never ships, we are left with code that very likely no one will use as we have not received this request before now. So I'm happy to see this as a non-contrib feature with intel/vmware sponsorship, but let's get a little internal testing time and then we can circle back in a couple of weeks to a month? Feel free to email me if you want to discuss further. Thank you. |
that makes sense, Matt. Thank you. |
Signed-off-by: LeiZhang <[email protected]>
Signed-off-by: lei zhang <[email protected]>
Signed-off-by: leizhang <[email protected]>
Signed-off-by: LeiZhang <[email protected]>
141b53d
to
5c40de9
Compare
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Hi @mattklein123 @lizan @ggreenway , to demonstrate how the bumping filter works, we have submitted a patch to demo all the related components(bumping filter, cert provider framework, local cert provider instance) at #23192. We tested it with some sites and things work fine. Could you take a look if the patch is on the right direction? Then we could polish and update each components separately for further review. Thanks. |
Implement bumping filter
Add a new network filter bumping filter
Implement cert prefetch based on tcp proxy filter
Risk Level: Low
Testing:
Docs Changes: N/A
Release Notes: N/A
Fixes #22581
Signed-off-by: leizhang [email protected]