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

Add exec_config_source support to ConfigSource, allowing secrets to be generated by local scripts #21292

Closed
jamesmulcahy opened this issue May 13, 2022 · 12 comments
Assignees
Labels
area/configuration stale stalebot believes this issue/PR has not been touched recently

Comments

@jamesmulcahy
Copy link
Contributor

Description:

There are circumstances where we want to use our xds control plane to push configuration to envoy, but the control plane does not have the necessary information available to it to allow it to push a fully formed resource. One specific example of this is secrets which are only available on the instance itself – or secrets which may be known to the control plane, but are in an encrypted form and need decrypting on the instance before they can be consumed by envoy.

I've written up an outline of a design to address this use case

@jamesmulcahy jamesmulcahy added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels May 13, 2022
@mattklein123 mattklein123 added area/configuration and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels May 16, 2022
@mattklein123
Copy link
Member

Thanks for the feature request. I'm not thrilled with building execute directly into the API, as it's highly environment dependent what this means and what the capabilities are. I think it would be better to just have an extension point here if there isn't one already such that you can build your own local execute extension. cc @envoyproxy/api-shepherds for thoughts.

@markdroth
Copy link
Contributor

I agree, adding this general-purpose mechanism to ConfigSource seems like overkill.

If the goal here is to get TLS certificate data from an external command, I think a better approach would be to use the CertificateProvider framework being built in #19582.

@ggreenway
Copy link
Contributor

I explored the idea of Envoy exec-ing processes for health checks a very long time ago (#1772) and among other problems, it required O_CLOEXEC and friends to be sprinkled around on nearly every fd. But we decided it was much better to just support an external gRPC service for getting this information into Envoy. This is equivalent to what @markdroth is suggesting here, and I echo that advice.

@htuch
Copy link
Member

htuch commented May 17, 2022

I like the idea of a config provider extension point that Matt suggests. It's just a generic extension point that will have an API to act as an xDS provider. Then @jamesmulcahy can implement this exec as an extension.

@jamesmulcahy
Copy link
Contributor Author

Apologies for the delay in following up here, I appreciate the engagement!

@markdroth said:

I agree, adding this general-purpose mechanism to ConfigSource seems like overkill.
If the goal here is to get TLS certificate data from an external command, I think a better approach would be to use the CertificateProvider framework being built in #19582.

The specific use case I have in mind right now is to enable us to push OAuth2 credentials via our control plane. Today, application owners generally handle those credentials in encrypted form, and they can only be decrypted by the instance itself (because it possess the necessary TLS cert).

Currently, such secrets are handle via SDS -- hence proposing a ConfigSource which could be referenced via SDS. The use case isn't for TLS certificates. While I could imagine changes to the CertificateProvider work in #19582, that does seem to be quite a departure from what it's aiming to achieve as is.

Taking the other three comments together

@mattklein123 said:

Thanks for the feature request. I'm not thrilled with building execute directly into the API, as it's highly environment dependent what this means and what the capabilities are. I think it would be better to just have an extension point here if there isn't one already such that you can build your own local execute extension

@ggreenway said:

I explored the idea of Envoy exec-ing processes for health checks a very long time ago (#1772) and among other problems, it required O_CLOEXEC and friends to be sprinkled around on nearly every fd. But we decided it was much better to just support an external gRPC service for getting this information into Envoy. This is equivalent to what @markdroth is suggesting here, and I echo that advice.

@htuch said:

I like the idea of a config provider extension point that Matt suggests. It's just a generic extension point that will have an API to act as an xDS provider. Then @jamesmulcahy can implement this exec as an extension.

I appreciate the concerns raised re: environmental dependencies, along with the O_CLOEXEC concerns that Greg raised.

An extension point would work for us here, I think. If I'm following correctly, there seem to be two different approaches proposed. One is an explicit, in-tree gRPC API, another is an extension point where we'd provide the specific implementation (i.e by writing a custom extension). Am I following correctly?

One reservation I have around the gRPC API is that it may require us to run a sidecar to provide that API, though I guess it's possible we could make that work with a dedicated listener + extension, such that the logic was actually inside envoy anyway. If that avoids O_CLOEXEC issues, that might be the better way for us to go anyway.

@mattklein123
Copy link
Member

I think we should support an in-tree extension point, and then you can maintain a private extension for your use case? Does that work?

@jamesmulcahy
Copy link
Contributor Author

I think we should support an in-tree extension point, and then you can maintain a private extension for your use case? Does that work?

Yeah, that works for us!

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 23, 2022
@htuch htuch removed the stale stalebot believes this issue/PR has not been touched recently label Jul 24, 2022
@htuch
Copy link
Member

htuch commented Jul 24, 2022

@jamesmulcahy will you folks take on the implementation work here?

@jamesmulcahy
Copy link
Contributor Author

@htuch Sorry for the delay in replying; I was out on vacation.

Yes, we plan to take this on, but don't have line of sight to a specific point in time when we will get to this -- we're juggling a few other things at the moment.

@github-actions
Copy link

github-actions bot commented Sep 1, 2022

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 1, 2022
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as completed Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

No branches or pull requests

5 participants