Hi Robert. Regarding your comment "use the lease duration to schedule a
configuration reload in the future", you might be interested in the code
that does refresh for OAuth Bearer Tokens in KIP-255; specifically, the
class
org.apache.kafka.common.security.oauthbearer.internal.expiring.ExpiringCredentialRefreshingLogin.
The class performs JAAS logins/relogins based on the expiration time of a
retrieved expiring credential. The implementation of that class is
inspired by the code that currently does refresh for Kerberos tickets but
is more reusable. I don't know if you will leverage JAAS for defining how
to go get a credential (you could since you have to provide credentials to
authenticate to the remote systems anyway), but regardless, that class
should be useful at least in some minimal sense if not more than that. See
https://github.com/apache/kafka/pull/4994.
Ron
Ron
On Tue, May 15, 2018 at 8:01 PM, Robert Yokota <rayok...@gmail.com> wrote:
> Hi Colin,
>
> Thanks for the feedback!
>
>
> > 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
> > favorites, right? :)
>
> I've removed this line :)
>
>
> > 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.
>
> Yes, the ConfigProviders merely serve up key-value pairs. A helper class
> like ConfigTransformer would perform the variable substitutions if desired.
>
>
> > We should also spell out exactly how substitution works.
>
> By one-level of indirection I just meant a simple replacement of variables
> (which are the indirect references). So if you have foo=${bar} and
> bar=${baz} and your file contains bar=hello, baz=world, then the final
> result would be foo=hello and bar=world. I've added this example to the
> KIP.
>
> You can see this as the DEFAULT_PATTERN in the ConfigTransformer. The
> ConfigTransformer only provides one level of indirection.
>
>
> > We should also spell out how this interacts with KIP-226 configurations.
>
> Yes, I mention at the beginning that KIP-226 could use the ConfigProvider
> but not the ConfigTransformer.
>
>
> > Maybe a good generic interface would be like this:
>
> I've added the subscription APIs but would like to keep the other APIs as I
> will need them for integration with Vault. With Vault I obtain the lease
> duration at the time the key is obtained, so at that time I would want to
> use the lease duration to schedule a configuration reload in the future.
> This is similar to how the integration between Vault and the Spring
> Framework works. Also, the lease duration would be included in the
> metadata map vs. the data map. Finally, I need an additional "path" or
> "bucket" parameter, which is used by Vault to indicate which set of
> key-values are to be retrieved.
>
>
> > With regard to ConfigTransformer: do we need to include all this code in
> the KIP? Seems like an implementation detail.
>
> I use the ConfigTransformer to show how the pattern ${provider:key} is
> defined and how the substitution only involves one level of indirection.
> If you feel it does not add anything to the text, I can remove it.
>
>
> > Is there a way to avoid this couping?
>
> I'd have to look into it and get back to you. However, I assume that the
> answer is not relevant for this KIP :)
>
>
> Thanks,
> Robert
>
>
>
>
>
> On Tue, May 15, 2018 at 4:04 PM, Colin McCabe <cmcc...@apache.org> wrote:
>
> > 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
> > > >> >
> > > >>
> > > >
> > > >
> >
>
