Hi Robert,
Thanks for posting this. In the past we've been kind of reluctant to add more
complexity to configuration. I think Connect does have a clear need for this
kind of functionality, though. As you mention, Connect integrates with
external systems, which are very likely to have passwords stored in Vault,
KeyWhiz or some other external system.
The KIP says that "Vault is very popular and has been described as 'the current
gold standard in secret management and provisioning'." I think this might be a
bit too much detail -- we don't really need to pick favorites, right? :)
I think we should make configuration consistent between the broker and Connect.
If people can use constructs like
jdbc.config.key="${vault:jdbc.user}${vault:jdbc.password}" in Connect, they'll
want to do it on the broker too, in a consistent way.
If I understand correctly, ConfigProvider represents an external configuration
source, such as VaultConfigProvider, KeyWhizConfigProvider, etc.
I think we should make the substitution part of the generic configuration code,
rather than specific to individual ConfigProviders. We don't really want it to
work differently for Vault vs. KeyWhiz vs. AWS secrets, etc. etc.
We should also spell out exactly how substitution works. For example, is
substitution limited to 1 level deep? In other words, If I have foo="${bar}"
and bar=${baz}, probably foo should just be set equal to "${baz}" rather than
chasing more than one level of indirection.
We should also spell out how this interacts with KIP-226 configurations. I
would suggest that KIP-226 variables not be subjected to substitution. The
reason is because in theory substitution could lead to different results on
different brokers, since the different brokers may not have the same
ConfigProviders configured. Also, having substitutions in the KIP-226
configuration makes it more difficult for the admin to understand what the
centrally managed configuration is.
It seems the main goal is the ability to load a batch of key/value pairs from
the ConfigProvider, and the ability to subscribe to notifications about changes
to certain parameters. Maybe a good generic interface would be like this:
> public interface ConfigProvider extends Closeable {
> // batched get is potentially more efficient
> Map<String, String> get(Collection<String> keys);
>
> // The ConfigProvider is responsible for making this callback whenever the
> key changes.
> // Some ConfigProviders may want to have a background thread with a
> configurable update interval.
> void subscribe(String key, ConfigurationChangeCallback callback);
>
> // Inverse of subscribe
> void unsubscribe(String key);
>
> // Close all subscriptions and clean up all resources
> void close();
> }
>
> interface ConfigurationChangeCallback {
> void onChange(String key, String value);
> }
With regard to ConfigTransformer: do we need to include all this code in the
KIP? Seems like an implementation detail.
> Other connectors such as the S3 connector are tightly coupled with a
> particular secret manager, and may
> wish to handle rotation on their own.
Is there a way to avoid this couping? Seems like some users might want to use
their own secret manager here.
best,
Colin
On Wed, May 9, 2018, at 16:32, Robert Yokota wrote:
> Hi Magesh,
>
> I updated the KIP with a link to a PR for a working prototype. The
> prototype does not yet use the Connect plugin machinery for class loader
> isolation, but should give you an idea of what the final implementation
> will look like. Here is the link:
> https://github.com/apache/kafka/pull/4990/files.
>
> I also added an example of a FileConfigProvider to the KIP.
>
> Thanks,
> Robert
>
> On Wed, May 9, 2018 at 10:04 AM, Robert Yokota <rayok...@gmail.com> wrote:
>
> > Hi Magesh,
> >
> > Thanks for the feedback!
> >
> > I will put together a PR to demonstrate what the implementation might look
> > like, as well as a reference FileConfigProvider.
> >
> > 1. The delayMs for a (potentially) scheduled reload is determined by the
> > ConfigProvider. For example, a (hypothetical) VaultConfigProvider, upon
> > contacting Vault for a particular secret, might also obtain a lease
> > duration indicating that the secret expires in 1 hour. The
> > VaultConfigProvider could then call scheduleConfigReload with delayMs set
> > to 3600000ms (1 hour). This would cause the Connector to restart in an
> > hour, forcing it to reload the configs and re-resolve all indirect
> > references.
> >
> > 2. Yes, the start() methods in SourceTask and SinkTask would get the
> > configs with all the indirect references resolved. Those config() methods
> > are for Connectors that want to get the latest configs (with all indirect
> > references re-resolved) at some time after start(). For example, if a Task
> > encountered some security exception because a secret expired, it could call
> > config() to get the config with the latest values. This is assuming that
> > the Task can gracefully recover from the security exception.
> >
> > 3. Yes, that is up to the ConfigProvider implementation and is out of
> > scope. If the ConfigProvider also needs some kind of secrets or other
> > data, it could possibly be passed in through the param properties
> > ("config.providers.vault.param.auth=/run/myauth"). For example Docker
> > might generate the auth info for Vault in an in-memory tmpfs file that
> > could then be passed as a param.
> >
> > Thanks,
> > Robert
> >
> >
> >
> > On Tue, May 8, 2018 at 10:10 PM, Magesh Nandakumar <mage...@confluent.io>
> > wrote:
> >
> >> Hi Robert,
> >>
> >> Thanks for the KIP. I think, this will be a great addition to the
> >> framework. I think, will be great if the KIP can elaborate a little bit
> >> more on how implementations would look like with an example.
> >> Also, would be good to provide a reference implementation as well.
> >>
> >> The other questions I had were
> >>
> >> 1. How would the framework get the delayMs for void scheduleConfigReload(
> >> long delayMs);
> >> 2. Would the start methods in SourceTask and SinkTask get the configs with
> >> all the indirect references resolved. If so, trying to understand
> >> the intent of the config() in SourceTaskContext and the SinkTaskContext
> >> 3. What if the provider itself needs some kind of secrets to be configured
> >> to connect to it? I assume that's out of scope for this proposal but
> >> wanted
> >> to clarify it.
> >>
> >> Thanks
> >> Magesh
> >>
> >> On Tue, May 8, 2018 at 1:52 PM, Robert Yokota <rayok...@gmail.com> wrote:
> >>
> >> > Hi,
> >> >
> >> > I would like to start a discussion for KIP-297 to externalize secrets
> >> from
> >> > Kafka Connect configurations. Any feedback is appreciated.
> >> > <
> >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> >> > >
> >> >
> >> > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> >> >
> >> > Thanks in advance,
> >> > Robert
> >> >
> >>
> >
> >
