You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Robert Yokota <ra...@gmail.com> on 2018/05/17 00:05:39 UTC

[VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Hello everyone,

After a good round of discussions with excellent feedback and no major
objections, I would like to start a vote on KIP-297 to externalize secrets
from Kafka Connect configurations.  My thanks in advance!

KIP: <
https://cwiki.apache.org/confluence/display/KAFKA/KIP-297%3A+Externalizing+Secrets+for+Connect+Configurations
>

JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>

Discussion thread: <
https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>

Best,
Robert

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Robert Yokota <ra...@gmail.com>.

Thanks Ewen!

> * Let's list in the KIP what package the ConfigProvider,
> ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
> defined in.

Does org.apache.kafka.common.config work for people?

> Also, I think ConfigData is left out of the list of new interfaces by
accident

Good catch, I've added it.

> I may have glanced past it, but we're not shipping any ConfigProviders
> out of the box?

I've updated the KIP to state we intend to provide a FileConfigProvider out
of the box based on a properties file format.

Thanks,
Robert







On Thu, May 17, 2018 at 9:41 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:

> Thanks for addressing this Robert, it's a pretty common user need.
>
> First, +1 (binding) generally.
>
> Two very minor comments that I think could be clarified but wouldn't affect
> votes:
>
> * Let's list in the KIP what package the ConfigProvider,
> ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
> defined in. Very, very minor, but given the aim to possibly reuse elsewhere
> and the fact that it'll likely end up in the common packages might mean
> devs focused more on the common/core packages will have strong opinions
> where they should be. I think it'd definitely be good to get input from
> folks focusing on the broker on where they think it should go since I think
> it would be very natural to extend this to security settings there. (Also,
> I think ConfigData is left out of the list of new interfaces by accident,
> but I think it's clear what's being added anyway.)
> * I may have glanced past it, but we're not shipping any ConfigProviders
> out of the box? This mentions file and vault, but just as examples. Just
> want to make sure everyone knows up front that this is a pluggable API, but
> you need to add more jars to take advantage of it. I think this is fine as
> I don't think there are truly common secrets provider
> formats/apis/protocols, just want to make sure it is clear.
>
> Thanks,
> Ewen
>
> On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com> wrote:
>
> > +1
> > -------- Original message --------From: Magesh Nandakumar <
> > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing Secrets
> > for Connect Configurations
> > Thanks Robert, this looks great
> >
> > +1 (non-binding)
> >
> > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org>
> wrote:
> >
> > > Thanks, Robert!
> > >
> > > +1 (non-binding)
> > >
> > > Colin
> > >
> > >
> > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > Hi Colin,
> > > >
> > > > I've changed the KIP to have a composite object returned from get().
> > > It's
> > > > probably the most straightforward option.  Please let me know if you
> > have
> > > > any other concerns.
> > > >
> > > > Thanks,
> > > > Robert
> > > >
> > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com>
> > > wrote:
> > > >
> > > > >
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > My last response was not that clear, so let me back up and explain
> a
> > > bit
> > > > > more.
> > > > >
> > > > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> > > notion of
> > > > > a lease duration or a TTL for a path.  Every path can have a
> > different
> > > > > TTL.  This is period after which the value of the keys at the given
> > > path
> > > > > may be invalid.  It can be used to indicate a rotation will be
> done.
> > > In
> > > > > the cause of the Vault integration with AWS, Vault will actually
> > > delete the
> > > > > secrets from AWS at the moment the TTL expires.  A TTL could be
> used
> > by
> > > > > other ConfigProviders, such as a FileConfigProvider, to indicate
> that
> > > all
> > > > > the secrets at a given path (file), will be rotated on a regular
> > basis.
> > > > >
> > > > > I would like to expose the TTL in the APIs somewhere.  The TTL can
> be
> > > made
> > > > > available at the time get() is called.  Connect already has a built
> > in
> > > > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> > > Connector
> > > > > restart.  Originally, I had exposed the TTL in a ConfigContext
> > > interface
> > > > > passed to the get() method.  To reduce the number of APIs, I placed
> > it
> > > on
> > > > > the onChange() method.  This means at the time of get(), onChange()
> > > would
> > > > > be called with a TTL.  The Connector's implementation of the
> callback
> > > would
> > > > > use onChange() with the TTL to schedule a restart.
> > > > >
> > > > > If you think this is overloading onChange() too much, I could add
> the
> > > > > ConfigContext back to get():
> > > > >
> > > > >
> > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > >
> > > > > public interface ConfigContext {
> > > > >
> > > > >     void willExpire(String path, long ttl);
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > or I could separate out the TTL method in the callback:
> > > > >
> > > > >
> > > > > public interface ConfigChangeCallback {
> > > > >
> > > > >     void willExpire(String path, long ttl);
> > > > >
> > > > >     void onChange(String path, Map<String, String> values);
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > Or we could return a composite object from get():
> > > > >
> > > > > ConfigData get(String path);
> > > > >
> > > > > public class ConfigData {
> > > > >
> > > > >   Map<String, String> data;
> > > > >   long ttl;
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > > Do you have a preference Colin?
> > > > >
> > > > > Thanks,
> > > > > Robert
> > > > >
> > > > >
> > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org>
> > > wrote:
> > > > >
> > > > >> Hi Robert,
> > > > >>
> > > > >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> > > > >> relying on the ConfigProvider to make a callback to you when the
> > > > >> configuration has changed.  So isn't that always the "push model"
> > > (where
> > > > >> the ConfigProvider pushes changes to Connect).  If you want the
> > "pull
> > > > >> model" where you initiate updates, you can simply call
> > > ConfigProvider#get
> > > > >> directly, right?
> > > > >>
> > > > >> The actual implementation of ConfigProvider subclasses will depend
> > on
> > > the
> > > > >> type of configuration storage mechanism on the backend.  In the
> case
> > > of
> > > > >> Vault, it sounds like we need to have something like a
> > > ScheduledExecutor
> > > > >> which re-fetches keys after a certain amount of time.
> > > > >>
> > > > >> As an aside, what does a "lease duration" mean for a configuration
> > > key?
> > > > >> Does that mean Vault will reject changes to the configuration key
> if
> > > I try
> > > > >> to make them within the lease duration?  Or is this like a period
> > > after
> > > > >> which a password is automatically rotated?
> > > > >>
> > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > >> > Hi Colin,
> > > > >> >
> > > > >> > > With regard to delayMs, can’t we just restart the
> > > > >> > > Connector when the keys are actually changed?
> > > > >> >
> > > > >> > Currently the VaultConfigProvider does not find out when values
> > for
> > > keys
> > > > >> > have changed.  You could do this with a poll model (with a
> > > > >> > background thread in the ConfigProvider), but since for each
> > > key-value
> > > > >> > pair, Vault provides a lease duration stating exactly when a
> value
> > > for a
> > > > >> > key will change in the future, this is an alternative model of
> > just
> > > > >> passing
> > > > >> > the lease duration to the client (in this case the Connector),
> to
> > > allow
> > > > >> it
> > > > >> > to determine what to do (such as schedule a restart).   This may
> > > allow
> > > > >> one
> > > > >> > to avoid the complexity of figuring out a proper poll interval
> > (with
> > > > >> lease
> > > > >> > durations of varying periods), or worrying about putting too
> much
> > > load
> > > > >> on
> > > > >> > the secrets manager by polling too often.
> > > > >>
> > > > >> Those things are still concerns if the Connector is polling,
> right?
> > > > >> Perhaps the connector poll too often and puts too much load on
> > > Vault.  And
> > > > >> so forth.  It seems like this problem needs to be solved either
> way
> > > (and
> > > > >> probably can be solved with reasonable default minimum fetch
> > > intervals).
> > > > >>
> > > > >> best,
> > > > >> Colin
> > > > >>
> > > > >>
> > > > >> >  In other words, by adding this
> > > > >> > one additional parameter, a ConfigProvider can provide both push
> > and
> > > > >> pull
> > > > >> > models to clients, perhaps with an additional configuration
> > > parameter to
> > > > >> > the ConfigProvider to determine which model (push or poll) to
> use.
> > > > >> >
> > > > >> > Thanks,
> > > > >> > Robert
> > > > >> >
> > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> cmccabe@apache.org
> > >
> > > > >> wrote:
> > > > >> >
> > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just restart
> > the
> > > > >> > > Connector when the keys are actually changed?  Or is the
> concern
> > > that
> > > > >> > > this would lengthen the effective key rotation time?  Can’t
> the
> > > user
> > > > >> > > just configure a slightly shorter key rotation time to
> > counteract
> > > > >> > > this concern?
> > > > >> > > Regards,
> > > > >> > > Colin
> > > > >> > >
> > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > >> > > > Hi Colin,
> > > > >> > > >
> > > > >> > > > Good questions.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > As a clarification about the indirections, what if I have
> > the
> > > > >> > > > > connect> configuration key foo set up as ${vault:bar}, and
> > in
> > > > >> Vault,
> > > > >> > > > have the bar> key set to ${file:baz}?
> > > > >> > > > > Does connect get foo as the contents of the baz file?  I
> > would
> > > > >> > > > > argue that> it should not (and in general, we shouldn't
> > allow
> > > > >> > > ConfigProviders to
> > > > >> > > > indirect to other
> > > > >> > > > > ConfigProviders) but I don't think it's spelled out right
> > now.
> > > > >> > > >
> > > > >> > > > I've added a clarification to the KIP that further
> > indirections
> > > are
> > > > >> > > > not> performed even if the values returned from
> > ConfigProviders
> > > > >> have the
> > > > >> > > > variable syntax.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > What's the behavior when a config key is not found in
> Vault
> > > > >> > > > > (or other> ConfigProvider)?  Does the variable get
> replaced
> > > with
> > > > >> the
> > > > >> > > empty
> > > > >> > > > string, or> with the literal ${vault:whatever} string?
> > > > >> > > >
> > > > >> > > > It would remain unresolved and still be of the form
> > > > >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > Do we really need "${provider:[path:]key}", or can it just
> > be
> > > > >> > > > ${provider:key}?
> > > > >> > > >
> > > > >> > > > The path is a separate parameter in the APIs, so I think
> it's
> > > > >> > > > important to> explicitly delineate it in the variable
> syntax.
> > > For
> > > > >> > > example, I
> > > > >> > > > currently> have a working VaultConfigProvider prototype and
> > the
> > > > >> syntax
> > > > >> > > for a
> > > > >> > > > Vault key> reference looks like
> > > > >> > > >
> > > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > > >> > > >
> > > > >> > > > I think it's important to standardize how to separate the
> path
> > > > >> > > > from the key> rather than leave it to each ConfigProvider to
> > > > >> determine a
> > > > >> > > possibly
> > > > >> > > > different way.  This will also make it easier to move
> secrets
> > > from
> > > > >> one>
> > > > >> > > ConfigProvider to another should one choose to do so.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > Do we really need delayMs?
> > > > >> > > >
> > > > >> > > > One of the goals of this KIP is to allow for secrets
> rotation
> > > > >> without>
> > > > >> > > having to modify existing connectors.  In the case of the
> > > > >> > > > VaultConfigProvider, it knows the lease durations and will
> be
> > > able
> > > > >> to>
> > > > >> > > schedule a restart of the Connector using an API in the
> Herder.
> > > The
> > > > >> > > > delayMs will simply be passed to the
> > > Herder.restartConnector(long
> > > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > > >> > > >
> > > > >> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > > > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > > >> > > connect/runtime/Herder.java#L170>
> > > > >> > > >
> > > > >> > > > Best,
> > > > >> > > > Robert
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > >> > > > <cm...@apache.org> wrote:>
> > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > >> > > > >
> > > > >> > > > > As a clarification about the indirections, what if I have
> > the
> > > > >> > > > > connect> > configuration key foo set up as ${vault:bar},
> and
> > > in
> > > > >> Vault,
> > > > >> > > have
> > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo
> as
> > > the
> > > > >> > > contents of
> > > > >> > > > > the baz> > file?  I would argue that it should not (and in
> > > > >> general, we
> > > > >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > > > >> > > ConfigProviders) but I
> > > > >> > > > > don't think> > it's spelled out right now.
> > > > >> > > > >
> > > > >> > > > > What's the behavior when a config key is not found in
> Vault
> > > > >> > > > > (or other> > ConfigProvider)?  Does the variable get
> > replaced
> > > > >> with the
> > > > >> > > empty
> > > > >> > > > > string, or> > with the literal ${vault:whatever} string?
> > > > >> > > > >
> > > > >> > > > > Do we really need "${provider:[path:]key}", or can it just
> > be
> > > > >> > > > > ${provider:key}?  It seems like the path can be rolled up
> > > into the
> > > > >> > > > > key.  So> > if you want to put your connect keys under
> > > > >> > > my.connect.path, you
> > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > > >> > > > >
> > > > >> > > > > >    // A delayMs of 0 indicates an immediate change; a
> > > positive
> > > > >> > > > > >    delayMs> > indicates
> > > > >> > > > > >    // that a future change is anticipated (such as a
> lease
> > > > >> > > > > >    duration)> > >    void onChange(String path,
> > Map<String,
> > > > >> String>
> > > > >> > > values, int
> > > > >> > > > > >    delayMs);> >
> > > > >> > > > > Do we really need delayMs?  It seems like if you get a
> > > callback
> > > > >> with>
> > > > >> > > > delayMs set, you don't know what the new values will be,
> only
> > > > >> > > > > that an> > update is coming, but not yet here.
> > > > >> > > > >
> > > > >> > > > > best,
> > > > >> > > > > Colin
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > >> > > > > > Hello everyone,
> > > > >> > > > > >
> > > > >> > > > > > After a good round of discussions with excellent
> feedback
> > > and no
> > > > >> > > > > > major> > > objections, I would like to start a vote on
> > > KIP-297
> > > > >> to
> > > > >> > > externalize> > secrets
> > > > >> > > > > > from Kafka Connect configurations.  My thanks in
> advance!
> > > > >> > > > > >
> > > > >> > > > > > KIP: <
> > > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > > >> > > > > > >
> > > > >> > > > > >
> > > > >> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886
> >
> > > > >> > > > > >
> > > > >> > > > > > Discussion thread: <
> > > > >> > > > > >
> > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > html
> > > > >> >
> > > > >> > > > > >
> > > > >> > > > > > Best,
> > > > >> > > > > > Robert
> > > > >> > > > >
> > > > >> > >
> > > > >> > >
> > > > >>
> > > > >
> > > > >
> > >
> >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Gwen Shapira <gw...@confluent.io>.

+1 from me too. Can't wait to use this feature.

On Mon, May 21, 2018 at 9:11 AM, Guozhang Wang <wa...@gmail.com> wrote:

> Thanks for the KIP. +1 from me (binding).
>
>
> Guozhang
>
> On Fri, May 18, 2018 at 9:46 AM, Randall Hauch <rh...@gmail.com> wrote:
>
> > Looks great.
> >
> > +1 (non-binding)
> >
> > Regards,
> > Randall
> >
> > On Fri, May 18, 2018 at 10:23 AM, Rajini Sivaram <
> rajinisivaram@gmail.com>
> > wrote:
> >
> > > Thanks, Robert! Sounds good. And thanks for the KIP.
> > >
> > > +1 (binding)
> > >
> > > Regards,
> > >
> > > Rajini
> > >
> > > On Fri, May 18, 2018 at 4:17 PM, Robert Yokota <ra...@gmail.com>
> > wrote:
> > >
> > > > HI Rajini,
> > > >
> > > > Good questions.
> > > >
> > > > First, if no ConfigProviders are configured, then config values of
> the
> > > form
> > > > ${vault:mypassword} will remain as is.
> > > >
> > > > Second, I mention in the KIP that if a provider does not have a value
> > > for a
> > > > given key, the variable will remain unresolved and the final value
> will
> > > be
> > > > of the form ${vault:mypassword} still.
> > > >
> > > > If one wants to use a config value ${vault:mypassword}, as well as
> the
> > > > VaultConfigProvider, one can choose to use a different prefix besides
> > > > "vault" when referring to the VaultConfigProvider since the prefixes
> > are
> > > > arbitrary and specified in a config file.
> > > >
> > > > Finally, if one want to use a config value ${vault:mypassword}, as
> well
> > > as
> > > > the VaultConfigProvider, and one wants to use the prefix "vault" and
> > not
> > > > something else, then yes, one could use a LiteralConfigProvider as
> you
> > > > described, or even put the ${vault:mypassword} in a different file
> and
> > > use
> > > > the FileConfigProvider to pull in the value (since there is only one
> > > level
> > > > of indirection).
> > > >
> > > > Thanks,
> > > > Robert
> > > >
> > > >
> > > >
> > > > On Fri, May 18, 2018 at 3:42 AM, Rajini Sivaram <
> > rajinisivaram@gmail.com
> > > >
> > > > wrote:
> > > >
> > > > > Hi Robert,
> > > > >
> > > > > A couple of questions:
> > > > >
> > > > >
> > > > >    1. Since we always expand config values, don't we also need a
> way
> > to
> > > > >    include values that never get expanded? I may want to use
> > > > >    "${vault:mypassword}" as my literal password without a lookup.
> > Since
> > > > we
> > > > >    allow only level of indirection, perhaps all we need is a
> > > > ConfigProvider
> > > > >    that uses the string inside, for example:
> > > > ${literal:${vault:mypassword}}
> > > > > ?
> > > > >    It would avoid having restrictions on what passwords can look
> > like.
> > > > >    2. What is the behaviour if I specify a password that is
> > > > >    "${notavault:something}" that matches the config provider
> syntax,
> > > but
> > > > > for
> > > > >    which there is no config provider?
> > > > >
> > > > >
> > > > >
> > > > > On Fri, May 18, 2018 at 5:41 AM, Ewen Cheslack-Postava <
> > > > ewen@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > Thanks for addressing this Robert, it's a pretty common user
> need.
> > > > > >
> > > > > > First, +1 (binding) generally.
> > > > > >
> > > > > > Two very minor comments that I think could be clarified but
> > wouldn't
> > > > > affect
> > > > > > votes:
> > > > > >
> > > > > > * Let's list in the KIP what package the ConfigProvider,
> > > > > > ConfigChangeCallback, ConfigData and ConfigTransformer interfaces
> > are
> > > > > > defined in. Very, very minor, but given the aim to possibly reuse
> > > > > elsewhere
> > > > > > and the fact that it'll likely end up in the common packages
> might
> > > mean
> > > > > > devs focused more on the common/core packages will have strong
> > > opinions
> > > > > > where they should be. I think it'd definitely be good to get
> input
> > > from
> > > > > > folks focusing on the broker on where they think it should go
> > since I
> > > > > think
> > > > > > it would be very natural to extend this to security settings
> there.
> > > > > (Also,
> > > > > > I think ConfigData is left out of the list of new interfaces by
> > > > accident,
> > > > > > but I think it's clear what's being added anyway.)
> > > > > > * I may have glanced past it, but we're not shipping any
> > > > ConfigProviders
> > > > > > out of the box? This mentions file and vault, but just as
> examples.
> > > > Just
> > > > > > want to make sure everyone knows up front that this is a
> pluggable
> > > API,
> > > > > but
> > > > > > you need to add more jars to take advantage of it. I think this
> is
> > > fine
> > > > > as
> > > > > > I don't think there are truly common secrets provider
> > > > > > formats/apis/protocols, just want to make sure it is clear.
> > > > > >
> > > > > > Thanks,
> > > > > > Ewen
> > > > > >
> > > > > > On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com>
> > wrote:
> > > > > >
> > > > > > > +1
> > > > > > > -------- Original message --------From: Magesh Nandakumar <
> > > > > > > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > > > > > > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297:
> Externalizing
> > > > > Secrets
> > > > > > > for Connect Configurations
> > > > > > > Thanks Robert, this looks great
> > > > > > >
> > > > > > > +1 (non-binding)
> > > > > > >
> > > > > > > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <
> > cmccabe@apache.org>
> > > > > > wrote:
> > > > > > >
> > > > > > > > Thanks, Robert!
> > > > > > > >
> > > > > > > > +1 (non-binding)
> > > > > > > >
> > > > > > > > Colin
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > > > > > > Hi Colin,
> > > > > > > > >
> > > > > > > > > I've changed the KIP to have a composite object returned
> from
> > > > > get().
> > > > > > > > It's
> > > > > > > > > probably the most straightforward option.  Please let me
> know
> > > if
> > > > > you
> > > > > > > have
> > > > > > > > > any other concerns.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Robert
> > > > > > > > >
> > > > > > > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <
> > > > > rayokota@gmail.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Hi Colin,
> > > > > > > > > >
> > > > > > > > > > My last response was not that clear, so let me back up
> and
> > > > > explain
> > > > > > a
> > > > > > > > bit
> > > > > > > > > > more.
> > > > > > > > > >
> > > > > > > > > > Some secret managers, such as Vault (and maybe Keywhiz)
> > have
> > > > the
> > > > > > > > notion of
> > > > > > > > > > a lease duration or a TTL for a path.  Every path can
> have
> > a
> > > > > > > different
> > > > > > > > > > TTL.  This is period after which the value of the keys at
> > the
> > > > > given
> > > > > > > > path
> > > > > > > > > > may be invalid.  It can be used to indicate a rotation
> will
> > > be
> > > > > > done.
> > > > > > > > In
> > > > > > > > > > the cause of the Vault integration with AWS, Vault will
> > > > actually
> > > > > > > > delete the
> > > > > > > > > > secrets from AWS at the moment the TTL expires.  A TTL
> > could
> > > be
> > > > > > used
> > > > > > > by
> > > > > > > > > > other ConfigProviders, such as a FileConfigProvider, to
> > > > indicate
> > > > > > that
> > > > > > > > all
> > > > > > > > > > the secrets at a given path (file), will be rotated on a
> > > > regular
> > > > > > > basis.
> > > > > > > > > >
> > > > > > > > > > I would like to expose the TTL in the APIs somewhere.
> The
> > > TTL
> > > > > can
> > > > > > be
> > > > > > > > made
> > > > > > > > > > available at the time get() is called.  Connect already
> > has a
> > > > > built
> > > > > > > in
> > > > > > > > > > ScheduledExecutor, so Connect can just use the TTL to
> > > schedule
> > > > a
> > > > > > > > Connector
> > > > > > > > > > restart.  Originally, I had exposed the TTL in a
> > > ConfigContext
> > > > > > > > interface
> > > > > > > > > > passed to the get() method.  To reduce the number of
> APIs,
> > I
> > > > > placed
> > > > > > > it
> > > > > > > > on
> > > > > > > > > > the onChange() method.  This means at the time of get(),
> > > > > onChange()
> > > > > > > > would
> > > > > > > > > > be called with a TTL.  The Connector's implementation of
> > the
> > > > > > callback
> > > > > > > > would
> > > > > > > > > > use onChange() with the TTL to schedule a restart.
> > > > > > > > > >
> > > > > > > > > > If you think this is overloading onChange() too much, I
> > could
> > > > add
> > > > > > the
> > > > > > > > > > ConfigContext back to get():
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > > > > > > >
> > > > > > > > > > public interface ConfigContext {
> > > > > > > > > >
> > > > > > > > > >     void willExpire(String path, long ttl);
> > > > > > > > > >
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > or I could separate out the TTL method in the callback:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > public interface ConfigChangeCallback {
> > > > > > > > > >
> > > > > > > > > >     void willExpire(String path, long ttl);
> > > > > > > > > >
> > > > > > > > > >     void onChange(String path, Map<String, String>
> values);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Or we could return a composite object from get():
> > > > > > > > > >
> > > > > > > > > > ConfigData get(String path);
> > > > > > > > > >
> > > > > > > > > > public class ConfigData {
> > > > > > > > > >
> > > > > > > > > >   Map<String, String> data;
> > > > > > > > > >   long ttl;
> > > > > > > > > >
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Do you have a preference Colin?
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Robert
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <
> > > > > cmccabe@apache.org>
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > >> Hi Robert,
> > > > > > > > > >>
> > > > > > > > > >> Hmm.  I thought that if you're using
> ConfigChangeCallback,
> > > you
> > > > > are
> > > > > > > > > >> relying on the ConfigProvider to make a callback to you
> > when
> > > > the
> > > > > > > > > >> configuration has changed.  So isn't that always the
> "push
> > > > > model"
> > > > > > > > (where
> > > > > > > > > >> the ConfigProvider pushes changes to Connect).  If you
> > want
> > > > the
> > > > > > > "pull
> > > > > > > > > >> model" where you initiate updates, you can simply call
> > > > > > > > ConfigProvider#get
> > > > > > > > > >> directly, right?
> > > > > > > > > >>
> > > > > > > > > >> The actual implementation of ConfigProvider subclasses
> > will
> > > > > depend
> > > > > > > on
> > > > > > > > the
> > > > > > > > > >> type of configuration storage mechanism on the backend.
> > In
> > > > the
> > > > > > case
> > > > > > > > of
> > > > > > > > > >> Vault, it sounds like we need to have something like a
> > > > > > > > ScheduledExecutor
> > > > > > > > > >> which re-fetches keys after a certain amount of time.
> > > > > > > > > >>
> > > > > > > > > >> As an aside, what does a "lease duration" mean for a
> > > > > configuration
> > > > > > > > key?
> > > > > > > > > >> Does that mean Vault will reject changes to the
> > > configuration
> > > > > key
> > > > > > if
> > > > > > > > I try
> > > > > > > > > >> to make them within the lease duration?  Or is this
> like a
> > > > > period
> > > > > > > > after
> > > > > > > > > >> which a password is automatically rotated?
> > > > > > > > > >>
> > > > > > > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > > > > > > >> > Hi Colin,
> > > > > > > > > >> >
> > > > > > > > > >> > > With regard to delayMs, can’t we just restart the
> > > > > > > > > >> > > Connector when the keys are actually changed?
> > > > > > > > > >> >
> > > > > > > > > >> > Currently the VaultConfigProvider does not find out
> when
> > > > > values
> > > > > > > for
> > > > > > > > keys
> > > > > > > > > >> > have changed.  You could do this with a poll model
> > (with a
> > > > > > > > > >> > background thread in the ConfigProvider), but since
> for
> > > each
> > > > > > > > key-value
> > > > > > > > > >> > pair, Vault provides a lease duration stating exactly
> > > when a
> > > > > > value
> > > > > > > > for a
> > > > > > > > > >> > key will change in the future, this is an alternative
> > > model
> > > > of
> > > > > > > just
> > > > > > > > > >> passing
> > > > > > > > > >> > the lease duration to the client (in this case the
> > > > Connector),
> > > > > > to
> > > > > > > > allow
> > > > > > > > > >> it
> > > > > > > > > >> > to determine what to do (such as schedule a restart).
> > >  This
> > > > > may
> > > > > > > > allow
> > > > > > > > > >> one
> > > > > > > > > >> > to avoid the complexity of figuring out a proper poll
> > > > interval
> > > > > > > (with
> > > > > > > > > >> lease
> > > > > > > > > >> > durations of varying periods), or worrying about
> putting
> > > too
> > > > > > much
> > > > > > > > load
> > > > > > > > > >> on
> > > > > > > > > >> > the secrets manager by polling too often.
> > > > > > > > > >>
> > > > > > > > > >> Those things are still concerns if the Connector is
> > polling,
> > > > > > right?
> > > > > > > > > >> Perhaps the connector poll too often and puts too much
> > load
> > > on
> > > > > > > > Vault.  And
> > > > > > > > > >> so forth.  It seems like this problem needs to be solved
> > > > either
> > > > > > way
> > > > > > > > (and
> > > > > > > > > >> probably can be solved with reasonable default minimum
> > fetch
> > > > > > > > intervals).
> > > > > > > > > >>
> > > > > > > > > >> best,
> > > > > > > > > >> Colin
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> >  In other words, by adding this
> > > > > > > > > >> > one additional parameter, a ConfigProvider can provide
> > > both
> > > > > push
> > > > > > > and
> > > > > > > > > >> pull
> > > > > > > > > >> > models to clients, perhaps with an additional
> > > configuration
> > > > > > > > parameter to
> > > > > > > > > >> > the ConfigProvider to determine which model (push or
> > poll)
> > > > to
> > > > > > use.
> > > > > > > > > >> >
> > > > > > > > > >> > Thanks,
> > > > > > > > > >> > Robert
> > > > > > > > > >> >
> > > > > > > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> > > > > > cmccabe@apache.org
> > > > > > > >
> > > > > > > > > >> wrote:
> > > > > > > > > >> >
> > > > > > > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we
> just
> > > > > restart
> > > > > > > the
> > > > > > > > > >> > > Connector when the keys are actually changed?  Or is
> > the
> > > > > > concern
> > > > > > > > that
> > > > > > > > > >> > > this would lengthen the effective key rotation time?
> > > > Can’t
> > > > > > the
> > > > > > > > user
> > > > > > > > > >> > > just configure a slightly shorter key rotation time
> to
> > > > > > > counteract
> > > > > > > > > >> > > this concern?
> > > > > > > > > >> > > Regards,
> > > > > > > > > >> > > Colin
> > > > > > > > > >> > >
> > > > > > > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > > > > > > >> > > > Hi Colin,
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > Good questions.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > > As a clarification about the indirections, what
> > if I
> > > > > have
> > > > > > > the
> > > > > > > > > >> > > > > connect> configuration key foo set up as
> > > ${vault:bar},
> > > > > and
> > > > > > > in
> > > > > > > > > >> Vault,
> > > > > > > > > >> > > > have the bar> key set to ${file:baz}?
> > > > > > > > > >> > > > > Does connect get foo as the contents of the baz
> > > > file?  I
> > > > > > > would
> > > > > > > > > >> > > > > argue that> it should not (and in general, we
> > > > shouldn't
> > > > > > > allow
> > > > > > > > > >> > > ConfigProviders to
> > > > > > > > > >> > > > indirect to other
> > > > > > > > > >> > > > > ConfigProviders) but I don't think it's spelled
> > out
> > > > > right
> > > > > > > now.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > I've added a clarification to the KIP that further
> > > > > > > indirections
> > > > > > > > are
> > > > > > > > > >> > > > not> performed even if the values returned from
> > > > > > > ConfigProviders
> > > > > > > > > >> have the
> > > > > > > > > >> > > > variable syntax.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > > What's the behavior when a config key is not
> found
> > > in
> > > > > > Vault
> > > > > > > > > >> > > > > (or other> ConfigProvider)?  Does the variable
> get
> > > > > > replaced
> > > > > > > > with
> > > > > > > > > >> the
> > > > > > > > > >> > > empty
> > > > > > > > > >> > > > string, or> with the literal ${vault:whatever}
> > string?
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > It would remain unresolved and still be of the
> form
> > > > > > > > > >> > > > ${provider:key}.  I've> added a clarification to
> the
> > > > KIP.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > > Do we really need "${provider:[path:]key}", or
> can
> > > it
> > > > > just
> > > > > > > be
> > > > > > > > > >> > > > ${provider:key}?
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > The path is a separate parameter in the APIs, so I
> > > think
> > > > > > it's
> > > > > > > > > >> > > > important to> explicitly delineate it in the
> > variable
> > > > > > syntax.
> > > > > > > > For
> > > > > > > > > >> > > example, I
> > > > > > > > > >> > > > currently> have a working VaultConfigProvider
> > > prototype
> > > > > and
> > > > > > > the
> > > > > > > > > >> syntax
> > > > > > > > > >> > > for a
> > > > > > > > > >> > > > Vault key> reference looks like
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > db_password=${vault:secret/
> staging:mysql_password}
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > I think it's important to standardize how to
> > separate
> > > > the
> > > > > > path
> > > > > > > > > >> > > > from the key> rather than leave it to each
> > > > ConfigProvider
> > > > > to
> > > > > > > > > >> determine a
> > > > > > > > > >> > > possibly
> > > > > > > > > >> > > > different way.  This will also make it easier to
> > move
> > > > > > secrets
> > > > > > > > from
> > > > > > > > > >> one>
> > > > > > > > > >> > > ConfigProvider to another should one choose to do
> so.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > > Do we really need delayMs?
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > One of the goals of this KIP is to allow for
> secrets
> > > > > > rotation
> > > > > > > > > >> without>
> > > > > > > > > >> > > having to modify existing connectors.  In the case
> of
> > > the
> > > > > > > > > >> > > > VaultConfigProvider, it knows the lease durations
> > and
> > > > will
> > > > > > be
> > > > > > > > able
> > > > > > > > > >> to>
> > > > > > > > > >> > > schedule a restart of the Connector using an API in
> > the
> > > > > > Herder.
> > > > > > > > The
> > > > > > > > > >> > > > delayMs will simply be passed to the
> > > > > > > > Herder.restartConnector(long
> > > > > > > > > >> > > > delayMs,> String connName, Callback cb) method
> here:
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > https://github.com/rayokota/
> > > > > kafka/blob/secrets-in-connect-
> > > > > > > > > >> > > configs/connect/runtime/src/
> > main/java/org/apache/kafka/
> > > > > > > > > >> > > connect/runtime/Herder.java#L170>
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > Best,
> > > > > > > > > >> > > > Robert
> > > > > > > > > >> > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > > > > > > >> > > > <cm...@apache.org> wrote:>
> > > > > > > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > As a clarification about the indirections, what
> > if I
> > > > > have
> > > > > > > the
> > > > > > > > > >> > > > > connect> > configuration key foo set up as
> > > > ${vault:bar},
> > > > > > and
> > > > > > > > in
> > > > > > > > > >> Vault,
> > > > > > > > > >> > > have
> > > > > > > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect
> > get
> > > > foo
> > > > > > as
> > > > > > > > the
> > > > > > > > > >> > > contents of
> > > > > > > > > >> > > > > the baz> > file?  I would argue that it should
> not
> > > > (and
> > > > > in
> > > > > > > > > >> general, we
> > > > > > > > > >> > > > > shouldn't allow> > ConfigProviders to indirect
> to
> > > > other
> > > > > > > > > >> > > ConfigProviders) but I
> > > > > > > > > >> > > > > don't think> > it's spelled out right now.
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > What's the behavior when a config key is not
> found
> > > in
> > > > > > Vault
> > > > > > > > > >> > > > > (or other> > ConfigProvider)?  Does the variable
> > get
> > > > > > > replaced
> > > > > > > > > >> with the
> > > > > > > > > >> > > empty
> > > > > > > > > >> > > > > string, or> > with the literal ${vault:whatever}
> > > > string?
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > Do we really need "${provider:[path:]key}", or
> can
> > > it
> > > > > just
> > > > > > > be
> > > > > > > > > >> > > > > ${provider:key}?  It seems like the path can be
> > > rolled
> > > > > up
> > > > > > > > into the
> > > > > > > > > >> > > > > key.  So> > if you want to put your connect keys
> > > under
> > > > > > > > > >> > > my.connect.path, you
> > > > > > > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.
> config},
> > > etc.
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > >    // A delayMs of 0 indicates an immediate
> > > change;
> > > > a
> > > > > > > > positive
> > > > > > > > > >> > > > > >    delayMs> > indicates
> > > > > > > > > >> > > > > >    // that a future change is anticipated
> (such
> > > as a
> > > > > > lease
> > > > > > > > > >> > > > > >    duration)> > >    void onChange(String
> path,
> > > > > > > Map<String,
> > > > > > > > > >> String>
> > > > > > > > > >> > > values, int
> > > > > > > > > >> > > > > >    delayMs);> >
> > > > > > > > > >> > > > > Do we really need delayMs?  It seems like if you
> > > get a
> > > > > > > > callback
> > > > > > > > > >> with>
> > > > > > > > > >> > > > delayMs set, you don't know what the new values
> will
> > > be,
> > > > > > only
> > > > > > > > > >> > > > > that an> > update is coming, but not yet here.
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > best,
> > > > > > > > > >> > > > > Colin
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > >
> > > > > > > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota
> > wrote:
> > > > > > > > > >> > > > > > Hello everyone,
> > > > > > > > > >> > > > > >
> > > > > > > > > >> > > > > > After a good round of discussions with
> excellent
> > > > > > feedback
> > > > > > > > and no
> > > > > > > > > >> > > > > > major> > > objections, I would like to start a
> > > vote
> > > > on
> > > > > > > > KIP-297
> > > > > > > > > >> to
> > > > > > > > > >> > > externalize> > secrets
> > > > > > > > > >> > > > > > from Kafka Connect configurations.  My thanks
> in
> > > > > > advance!
> > > > > > > > > >> > > > > >
> > > > > > > > > >> > > > > > KIP: <
> > > > > > > > > >> > > > > > https://cwiki.apache.org/
> > > > > confluence/display/KAFKA/KIP-
> > > > > > > > > >> > > > > 297%3A+Externalizing+Secrets+
> > > > for+Connect+Configurations
> > > > > > > > > >> > > > > > >
> > > > > > > > > >> > > > > >
> > > > > > > > > >> > > > > > JIRA: <https://issues.apache.org/
> > > > > jira/browse/KAFKA-6886
> > > > > > >
> > > > > > > > > >> > > > > >
> > > > > > > > > >> > > > > > Discussion thread: <
> > > > > > > > > >> > > > > >
> > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > > > > > > html
> > > > > > > > > >> >
> > > > > > > > > >> > > > > >
> > > > > > > > > >> > > > > > Best,
> > > > > > > > > >> > > > > > Robert
> > > > > > > > > >> > > > >
> > > > > > > > > >> > >
> > > > > > > > > >> > >
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
>
> --
> -- Guozhang
>



-- 
*Gwen Shapira*
Product Manager | Confluent
650.450.2760 | @gwenshap
Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
<http://www.confluent.io/blog>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Guozhang Wang <wa...@gmail.com>.

Thanks for the KIP. +1 from me (binding).


Guozhang

On Fri, May 18, 2018 at 9:46 AM, Randall Hauch <rh...@gmail.com> wrote:

> Looks great.
>
> +1 (non-binding)
>
> Regards,
> Randall
>
> On Fri, May 18, 2018 at 10:23 AM, Rajini Sivaram <ra...@gmail.com>
> wrote:
>
> > Thanks, Robert! Sounds good. And thanks for the KIP.
> >
> > +1 (binding)
> >
> > Regards,
> >
> > Rajini
> >
> > On Fri, May 18, 2018 at 4:17 PM, Robert Yokota <ra...@gmail.com>
> wrote:
> >
> > > HI Rajini,
> > >
> > > Good questions.
> > >
> > > First, if no ConfigProviders are configured, then config values of the
> > form
> > > ${vault:mypassword} will remain as is.
> > >
> > > Second, I mention in the KIP that if a provider does not have a value
> > for a
> > > given key, the variable will remain unresolved and the final value will
> > be
> > > of the form ${vault:mypassword} still.
> > >
> > > If one wants to use a config value ${vault:mypassword}, as well as the
> > > VaultConfigProvider, one can choose to use a different prefix besides
> > > "vault" when referring to the VaultConfigProvider since the prefixes
> are
> > > arbitrary and specified in a config file.
> > >
> > > Finally, if one want to use a config value ${vault:mypassword}, as well
> > as
> > > the VaultConfigProvider, and one wants to use the prefix "vault" and
> not
> > > something else, then yes, one could use a LiteralConfigProvider as you
> > > described, or even put the ${vault:mypassword} in a different file and
> > use
> > > the FileConfigProvider to pull in the value (since there is only one
> > level
> > > of indirection).
> > >
> > > Thanks,
> > > Robert
> > >
> > >
> > >
> > > On Fri, May 18, 2018 at 3:42 AM, Rajini Sivaram <
> rajinisivaram@gmail.com
> > >
> > > wrote:
> > >
> > > > Hi Robert,
> > > >
> > > > A couple of questions:
> > > >
> > > >
> > > >    1. Since we always expand config values, don't we also need a way
> to
> > > >    include values that never get expanded? I may want to use
> > > >    "${vault:mypassword}" as my literal password without a lookup.
> Since
> > > we
> > > >    allow only level of indirection, perhaps all we need is a
> > > ConfigProvider
> > > >    that uses the string inside, for example:
> > > ${literal:${vault:mypassword}}
> > > > ?
> > > >    It would avoid having restrictions on what passwords can look
> like.
> > > >    2. What is the behaviour if I specify a password that is
> > > >    "${notavault:something}" that matches the config provider syntax,
> > but
> > > > for
> > > >    which there is no config provider?
> > > >
> > > >
> > > >
> > > > On Fri, May 18, 2018 at 5:41 AM, Ewen Cheslack-Postava <
> > > ewen@confluent.io>
> > > > wrote:
> > > >
> > > > > Thanks for addressing this Robert, it's a pretty common user need.
> > > > >
> > > > > First, +1 (binding) generally.
> > > > >
> > > > > Two very minor comments that I think could be clarified but
> wouldn't
> > > > affect
> > > > > votes:
> > > > >
> > > > > * Let's list in the KIP what package the ConfigProvider,
> > > > > ConfigChangeCallback, ConfigData and ConfigTransformer interfaces
> are
> > > > > defined in. Very, very minor, but given the aim to possibly reuse
> > > > elsewhere
> > > > > and the fact that it'll likely end up in the common packages might
> > mean
> > > > > devs focused more on the common/core packages will have strong
> > opinions
> > > > > where they should be. I think it'd definitely be good to get input
> > from
> > > > > folks focusing on the broker on where they think it should go
> since I
> > > > think
> > > > > it would be very natural to extend this to security settings there.
> > > > (Also,
> > > > > I think ConfigData is left out of the list of new interfaces by
> > > accident,
> > > > > but I think it's clear what's being added anyway.)
> > > > > * I may have glanced past it, but we're not shipping any
> > > ConfigProviders
> > > > > out of the box? This mentions file and vault, but just as examples.
> > > Just
> > > > > want to make sure everyone knows up front that this is a pluggable
> > API,
> > > > but
> > > > > you need to add more jars to take advantage of it. I think this is
> > fine
> > > > as
> > > > > I don't think there are truly common secrets provider
> > > > > formats/apis/protocols, just want to make sure it is clear.
> > > > >
> > > > > Thanks,
> > > > > Ewen
> > > > >
> > > > > On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com>
> wrote:
> > > > >
> > > > > > +1
> > > > > > -------- Original message --------From: Magesh Nandakumar <
> > > > > > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > > > > > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing
> > > > Secrets
> > > > > > for Connect Configurations
> > > > > > Thanks Robert, this looks great
> > > > > >
> > > > > > +1 (non-binding)
> > > > > >
> > > > > > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <
> cmccabe@apache.org>
> > > > > wrote:
> > > > > >
> > > > > > > Thanks, Robert!
> > > > > > >
> > > > > > > +1 (non-binding)
> > > > > > >
> > > > > > > Colin
> > > > > > >
> > > > > > >
> > > > > > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > I've changed the KIP to have a composite object returned from
> > > > get().
> > > > > > > It's
> > > > > > > > probably the most straightforward option.  Please let me know
> > if
> > > > you
> > > > > > have
> > > > > > > > any other concerns.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Robert
> > > > > > > >
> > > > > > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <
> > > > rayokota@gmail.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi Colin,
> > > > > > > > >
> > > > > > > > > My last response was not that clear, so let me back up and
> > > > explain
> > > > > a
> > > > > > > bit
> > > > > > > > > more.
> > > > > > > > >
> > > > > > > > > Some secret managers, such as Vault (and maybe Keywhiz)
> have
> > > the
> > > > > > > notion of
> > > > > > > > > a lease duration or a TTL for a path.  Every path can have
> a
> > > > > > different
> > > > > > > > > TTL.  This is period after which the value of the keys at
> the
> > > > given
> > > > > > > path
> > > > > > > > > may be invalid.  It can be used to indicate a rotation will
> > be
> > > > > done.
> > > > > > > In
> > > > > > > > > the cause of the Vault integration with AWS, Vault will
> > > actually
> > > > > > > delete the
> > > > > > > > > secrets from AWS at the moment the TTL expires.  A TTL
> could
> > be
> > > > > used
> > > > > > by
> > > > > > > > > other ConfigProviders, such as a FileConfigProvider, to
> > > indicate
> > > > > that
> > > > > > > all
> > > > > > > > > the secrets at a given path (file), will be rotated on a
> > > regular
> > > > > > basis.
> > > > > > > > >
> > > > > > > > > I would like to expose the TTL in the APIs somewhere.  The
> > TTL
> > > > can
> > > > > be
> > > > > > > made
> > > > > > > > > available at the time get() is called.  Connect already
> has a
> > > > built
> > > > > > in
> > > > > > > > > ScheduledExecutor, so Connect can just use the TTL to
> > schedule
> > > a
> > > > > > > Connector
> > > > > > > > > restart.  Originally, I had exposed the TTL in a
> > ConfigContext
> > > > > > > interface
> > > > > > > > > passed to the get() method.  To reduce the number of APIs,
> I
> > > > placed
> > > > > > it
> > > > > > > on
> > > > > > > > > the onChange() method.  This means at the time of get(),
> > > > onChange()
> > > > > > > would
> > > > > > > > > be called with a TTL.  The Connector's implementation of
> the
> > > > > callback
> > > > > > > would
> > > > > > > > > use onChange() with the TTL to schedule a restart.
> > > > > > > > >
> > > > > > > > > If you think this is overloading onChange() too much, I
> could
> > > add
> > > > > the
> > > > > > > > > ConfigContext back to get():
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > > > > > >
> > > > > > > > > public interface ConfigContext {
> > > > > > > > >
> > > > > > > > >     void willExpire(String path, long ttl);
> > > > > > > > >
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > or I could separate out the TTL method in the callback:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > public interface ConfigChangeCallback {
> > > > > > > > >
> > > > > > > > >     void willExpire(String path, long ttl);
> > > > > > > > >
> > > > > > > > >     void onChange(String path, Map<String, String> values);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Or we could return a composite object from get():
> > > > > > > > >
> > > > > > > > > ConfigData get(String path);
> > > > > > > > >
> > > > > > > > > public class ConfigData {
> > > > > > > > >
> > > > > > > > >   Map<String, String> data;
> > > > > > > > >   long ttl;
> > > > > > > > >
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Do you have a preference Colin?
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Robert
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <
> > > > cmccabe@apache.org>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >> Hi Robert,
> > > > > > > > >>
> > > > > > > > >> Hmm.  I thought that if you're using ConfigChangeCallback,
> > you
> > > > are
> > > > > > > > >> relying on the ConfigProvider to make a callback to you
> when
> > > the
> > > > > > > > >> configuration has changed.  So isn't that always the "push
> > > > model"
> > > > > > > (where
> > > > > > > > >> the ConfigProvider pushes changes to Connect).  If you
> want
> > > the
> > > > > > "pull
> > > > > > > > >> model" where you initiate updates, you can simply call
> > > > > > > ConfigProvider#get
> > > > > > > > >> directly, right?
> > > > > > > > >>
> > > > > > > > >> The actual implementation of ConfigProvider subclasses
> will
> > > > depend
> > > > > > on
> > > > > > > the
> > > > > > > > >> type of configuration storage mechanism on the backend.
> In
> > > the
> > > > > case
> > > > > > > of
> > > > > > > > >> Vault, it sounds like we need to have something like a
> > > > > > > ScheduledExecutor
> > > > > > > > >> which re-fetches keys after a certain amount of time.
> > > > > > > > >>
> > > > > > > > >> As an aside, what does a "lease duration" mean for a
> > > > configuration
> > > > > > > key?
> > > > > > > > >> Does that mean Vault will reject changes to the
> > configuration
> > > > key
> > > > > if
> > > > > > > I try
> > > > > > > > >> to make them within the lease duration?  Or is this like a
> > > > period
> > > > > > > after
> > > > > > > > >> which a password is automatically rotated?
> > > > > > > > >>
> > > > > > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > > > > > >> > Hi Colin,
> > > > > > > > >> >
> > > > > > > > >> > > With regard to delayMs, can’t we just restart the
> > > > > > > > >> > > Connector when the keys are actually changed?
> > > > > > > > >> >
> > > > > > > > >> > Currently the VaultConfigProvider does not find out when
> > > > values
> > > > > > for
> > > > > > > keys
> > > > > > > > >> > have changed.  You could do this with a poll model
> (with a
> > > > > > > > >> > background thread in the ConfigProvider), but since for
> > each
> > > > > > > key-value
> > > > > > > > >> > pair, Vault provides a lease duration stating exactly
> > when a
> > > > > value
> > > > > > > for a
> > > > > > > > >> > key will change in the future, this is an alternative
> > model
> > > of
> > > > > > just
> > > > > > > > >> passing
> > > > > > > > >> > the lease duration to the client (in this case the
> > > Connector),
> > > > > to
> > > > > > > allow
> > > > > > > > >> it
> > > > > > > > >> > to determine what to do (such as schedule a restart).
> >  This
> > > > may
> > > > > > > allow
> > > > > > > > >> one
> > > > > > > > >> > to avoid the complexity of figuring out a proper poll
> > > interval
> > > > > > (with
> > > > > > > > >> lease
> > > > > > > > >> > durations of varying periods), or worrying about putting
> > too
> > > > > much
> > > > > > > load
> > > > > > > > >> on
> > > > > > > > >> > the secrets manager by polling too often.
> > > > > > > > >>
> > > > > > > > >> Those things are still concerns if the Connector is
> polling,
> > > > > right?
> > > > > > > > >> Perhaps the connector poll too often and puts too much
> load
> > on
> > > > > > > Vault.  And
> > > > > > > > >> so forth.  It seems like this problem needs to be solved
> > > either
> > > > > way
> > > > > > > (and
> > > > > > > > >> probably can be solved with reasonable default minimum
> fetch
> > > > > > > intervals).
> > > > > > > > >>
> > > > > > > > >> best,
> > > > > > > > >> Colin
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> >  In other words, by adding this
> > > > > > > > >> > one additional parameter, a ConfigProvider can provide
> > both
> > > > push
> > > > > > and
> > > > > > > > >> pull
> > > > > > > > >> > models to clients, perhaps with an additional
> > configuration
> > > > > > > parameter to
> > > > > > > > >> > the ConfigProvider to determine which model (push or
> poll)
> > > to
> > > > > use.
> > > > > > > > >> >
> > > > > > > > >> > Thanks,
> > > > > > > > >> > Robert
> > > > > > > > >> >
> > > > > > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> > > > > cmccabe@apache.org
> > > > > > >
> > > > > > > > >> wrote:
> > > > > > > > >> >
> > > > > > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just
> > > > restart
> > > > > > the
> > > > > > > > >> > > Connector when the keys are actually changed?  Or is
> the
> > > > > concern
> > > > > > > that
> > > > > > > > >> > > this would lengthen the effective key rotation time?
> > > Can’t
> > > > > the
> > > > > > > user
> > > > > > > > >> > > just configure a slightly shorter key rotation time to
> > > > > > counteract
> > > > > > > > >> > > this concern?
> > > > > > > > >> > > Regards,
> > > > > > > > >> > > Colin
> > > > > > > > >> > >
> > > > > > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > > > > > >> > > > Hi Colin,
> > > > > > > > >> > > >
> > > > > > > > >> > > > Good questions.
> > > > > > > > >> > > >
> > > > > > > > >> > > >
> > > > > > > > >> > > > > As a clarification about the indirections, what
> if I
> > > > have
> > > > > > the
> > > > > > > > >> > > > > connect> configuration key foo set up as
> > ${vault:bar},
> > > > and
> > > > > > in
> > > > > > > > >> Vault,
> > > > > > > > >> > > > have the bar> key set to ${file:baz}?
> > > > > > > > >> > > > > Does connect get foo as the contents of the baz
> > > file?  I
> > > > > > would
> > > > > > > > >> > > > > argue that> it should not (and in general, we
> > > shouldn't
> > > > > > allow
> > > > > > > > >> > > ConfigProviders to
> > > > > > > > >> > > > indirect to other
> > > > > > > > >> > > > > ConfigProviders) but I don't think it's spelled
> out
> > > > right
> > > > > > now.
> > > > > > > > >> > > >
> > > > > > > > >> > > > I've added a clarification to the KIP that further
> > > > > > indirections
> > > > > > > are
> > > > > > > > >> > > > not> performed even if the values returned from
> > > > > > ConfigProviders
> > > > > > > > >> have the
> > > > > > > > >> > > > variable syntax.
> > > > > > > > >> > > >
> > > > > > > > >> > > >
> > > > > > > > >> > > > > What's the behavior when a config key is not found
> > in
> > > > > Vault
> > > > > > > > >> > > > > (or other> ConfigProvider)?  Does the variable get
> > > > > replaced
> > > > > > > with
> > > > > > > > >> the
> > > > > > > > >> > > empty
> > > > > > > > >> > > > string, or> with the literal ${vault:whatever}
> string?
> > > > > > > > >> > > >
> > > > > > > > >> > > > It would remain unresolved and still be of the form
> > > > > > > > >> > > > ${provider:key}.  I've> added a clarification to the
> > > KIP.
> > > > > > > > >> > > >
> > > > > > > > >> > > >
> > > > > > > > >> > > > > Do we really need "${provider:[path:]key}", or can
> > it
> > > > just
> > > > > > be
> > > > > > > > >> > > > ${provider:key}?
> > > > > > > > >> > > >
> > > > > > > > >> > > > The path is a separate parameter in the APIs, so I
> > think
> > > > > it's
> > > > > > > > >> > > > important to> explicitly delineate it in the
> variable
> > > > > syntax.
> > > > > > > For
> > > > > > > > >> > > example, I
> > > > > > > > >> > > > currently> have a working VaultConfigProvider
> > prototype
> > > > and
> > > > > > the
> > > > > > > > >> syntax
> > > > > > > > >> > > for a
> > > > > > > > >> > > > Vault key> reference looks like
> > > > > > > > >> > > >
> > > > > > > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > > > > > > >> > > >
> > > > > > > > >> > > > I think it's important to standardize how to
> separate
> > > the
> > > > > path
> > > > > > > > >> > > > from the key> rather than leave it to each
> > > ConfigProvider
> > > > to
> > > > > > > > >> determine a
> > > > > > > > >> > > possibly
> > > > > > > > >> > > > different way.  This will also make it easier to
> move
> > > > > secrets
> > > > > > > from
> > > > > > > > >> one>
> > > > > > > > >> > > ConfigProvider to another should one choose to do so.
> > > > > > > > >> > > >
> > > > > > > > >> > > >
> > > > > > > > >> > > > > Do we really need delayMs?
> > > > > > > > >> > > >
> > > > > > > > >> > > > One of the goals of this KIP is to allow for secrets
> > > > > rotation
> > > > > > > > >> without>
> > > > > > > > >> > > having to modify existing connectors.  In the case of
> > the
> > > > > > > > >> > > > VaultConfigProvider, it knows the lease durations
> and
> > > will
> > > > > be
> > > > > > > able
> > > > > > > > >> to>
> > > > > > > > >> > > schedule a restart of the Connector using an API in
> the
> > > > > Herder.
> > > > > > > The
> > > > > > > > >> > > > delayMs will simply be passed to the
> > > > > > > Herder.restartConnector(long
> > > > > > > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > > > > > > >> > > >
> > > > > > > > >> > > > https://github.com/rayokota/
> > > > kafka/blob/secrets-in-connect-
> > > > > > > > >> > > configs/connect/runtime/src/
> main/java/org/apache/kafka/
> > > > > > > > >> > > connect/runtime/Herder.java#L170>
> > > > > > > > >> > > >
> > > > > > > > >> > > > Best,
> > > > > > > > >> > > > Robert
> > > > > > > > >> > > >
> > > > > > > > >> > > >
> > > > > > > > >> > > >
> > > > > > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > > > > > >> > > > <cm...@apache.org> wrote:>
> > > > > > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > As a clarification about the indirections, what
> if I
> > > > have
> > > > > > the
> > > > > > > > >> > > > > connect> > configuration key foo set up as
> > > ${vault:bar},
> > > > > and
> > > > > > > in
> > > > > > > > >> Vault,
> > > > > > > > >> > > have
> > > > > > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect
> get
> > > foo
> > > > > as
> > > > > > > the
> > > > > > > > >> > > contents of
> > > > > > > > >> > > > > the baz> > file?  I would argue that it should not
> > > (and
> > > > in
> > > > > > > > >> general, we
> > > > > > > > >> > > > > shouldn't allow> > ConfigProviders to indirect to
> > > other
> > > > > > > > >> > > ConfigProviders) but I
> > > > > > > > >> > > > > don't think> > it's spelled out right now.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > What's the behavior when a config key is not found
> > in
> > > > > Vault
> > > > > > > > >> > > > > (or other> > ConfigProvider)?  Does the variable
> get
> > > > > > replaced
> > > > > > > > >> with the
> > > > > > > > >> > > empty
> > > > > > > > >> > > > > string, or> > with the literal ${vault:whatever}
> > > string?
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > Do we really need "${provider:[path:]key}", or can
> > it
> > > > just
> > > > > > be
> > > > > > > > >> > > > > ${provider:key}?  It seems like the path can be
> > rolled
> > > > up
> > > > > > > into the
> > > > > > > > >> > > > > key.  So> > if you want to put your connect keys
> > under
> > > > > > > > >> > > my.connect.path, you
> > > > > > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config},
> > etc.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > >    // A delayMs of 0 indicates an immediate
> > change;
> > > a
> > > > > > > positive
> > > > > > > > >> > > > > >    delayMs> > indicates
> > > > > > > > >> > > > > >    // that a future change is anticipated (such
> > as a
> > > > > lease
> > > > > > > > >> > > > > >    duration)> > >    void onChange(String path,
> > > > > > Map<String,
> > > > > > > > >> String>
> > > > > > > > >> > > values, int
> > > > > > > > >> > > > > >    delayMs);> >
> > > > > > > > >> > > > > Do we really need delayMs?  It seems like if you
> > get a
> > > > > > > callback
> > > > > > > > >> with>
> > > > > > > > >> > > > delayMs set, you don't know what the new values will
> > be,
> > > > > only
> > > > > > > > >> > > > > that an> > update is coming, but not yet here.
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > best,
> > > > > > > > >> > > > > Colin
> > > > > > > > >> > > > >
> > > > > > > > >> > > > >
> > > > > > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota
> wrote:
> > > > > > > > >> > > > > > Hello everyone,
> > > > > > > > >> > > > > >
> > > > > > > > >> > > > > > After a good round of discussions with excellent
> > > > > feedback
> > > > > > > and no
> > > > > > > > >> > > > > > major> > > objections, I would like to start a
> > vote
> > > on
> > > > > > > KIP-297
> > > > > > > > >> to
> > > > > > > > >> > > externalize> > secrets
> > > > > > > > >> > > > > > from Kafka Connect configurations.  My thanks in
> > > > > advance!
> > > > > > > > >> > > > > >
> > > > > > > > >> > > > > > KIP: <
> > > > > > > > >> > > > > > https://cwiki.apache.org/
> > > > confluence/display/KAFKA/KIP-
> > > > > > > > >> > > > > 297%3A+Externalizing+Secrets+
> > > for+Connect+Configurations
> > > > > > > > >> > > > > > >
> > > > > > > > >> > > > > >
> > > > > > > > >> > > > > > JIRA: <https://issues.apache.org/
> > > > jira/browse/KAFKA-6886
> > > > > >
> > > > > > > > >> > > > > >
> > > > > > > > >> > > > > > Discussion thread: <
> > > > > > > > >> > > > > >
> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > > > > > html
> > > > > > > > >> >
> > > > > > > > >> > > > > >
> > > > > > > > >> > > > > > Best,
> > > > > > > > >> > > > > > Robert
> > > > > > > > >> > > > >
> > > > > > > > >> > >
> > > > > > > > >> > >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>



-- 
-- Guozhang

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Randall Hauch <rh...@gmail.com>.

Looks great.

+1 (non-binding)

Regards,
Randall

On Fri, May 18, 2018 at 10:23 AM, Rajini Sivaram <ra...@gmail.com>
wrote:

> Thanks, Robert! Sounds good. And thanks for the KIP.
>
> +1 (binding)
>
> Regards,
>
> Rajini
>
> On Fri, May 18, 2018 at 4:17 PM, Robert Yokota <ra...@gmail.com> wrote:
>
> > HI Rajini,
> >
> > Good questions.
> >
> > First, if no ConfigProviders are configured, then config values of the
> form
> > ${vault:mypassword} will remain as is.
> >
> > Second, I mention in the KIP that if a provider does not have a value
> for a
> > given key, the variable will remain unresolved and the final value will
> be
> > of the form ${vault:mypassword} still.
> >
> > If one wants to use a config value ${vault:mypassword}, as well as the
> > VaultConfigProvider, one can choose to use a different prefix besides
> > "vault" when referring to the VaultConfigProvider since the prefixes are
> > arbitrary and specified in a config file.
> >
> > Finally, if one want to use a config value ${vault:mypassword}, as well
> as
> > the VaultConfigProvider, and one wants to use the prefix "vault" and not
> > something else, then yes, one could use a LiteralConfigProvider as you
> > described, or even put the ${vault:mypassword} in a different file and
> use
> > the FileConfigProvider to pull in the value (since there is only one
> level
> > of indirection).
> >
> > Thanks,
> > Robert
> >
> >
> >
> > On Fri, May 18, 2018 at 3:42 AM, Rajini Sivaram <rajinisivaram@gmail.com
> >
> > wrote:
> >
> > > Hi Robert,
> > >
> > > A couple of questions:
> > >
> > >
> > >    1. Since we always expand config values, don't we also need a way to
> > >    include values that never get expanded? I may want to use
> > >    "${vault:mypassword}" as my literal password without a lookup. Since
> > we
> > >    allow only level of indirection, perhaps all we need is a
> > ConfigProvider
> > >    that uses the string inside, for example:
> > ${literal:${vault:mypassword}}
> > > ?
> > >    It would avoid having restrictions on what passwords can look like.
> > >    2. What is the behaviour if I specify a password that is
> > >    "${notavault:something}" that matches the config provider syntax,
> but
> > > for
> > >    which there is no config provider?
> > >
> > >
> > >
> > > On Fri, May 18, 2018 at 5:41 AM, Ewen Cheslack-Postava <
> > ewen@confluent.io>
> > > wrote:
> > >
> > > > Thanks for addressing this Robert, it's a pretty common user need.
> > > >
> > > > First, +1 (binding) generally.
> > > >
> > > > Two very minor comments that I think could be clarified but wouldn't
> > > affect
> > > > votes:
> > > >
> > > > * Let's list in the KIP what package the ConfigProvider,
> > > > ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
> > > > defined in. Very, very minor, but given the aim to possibly reuse
> > > elsewhere
> > > > and the fact that it'll likely end up in the common packages might
> mean
> > > > devs focused more on the common/core packages will have strong
> opinions
> > > > where they should be. I think it'd definitely be good to get input
> from
> > > > folks focusing on the broker on where they think it should go since I
> > > think
> > > > it would be very natural to extend this to security settings there.
> > > (Also,
> > > > I think ConfigData is left out of the list of new interfaces by
> > accident,
> > > > but I think it's clear what's being added anyway.)
> > > > * I may have glanced past it, but we're not shipping any
> > ConfigProviders
> > > > out of the box? This mentions file and vault, but just as examples.
> > Just
> > > > want to make sure everyone knows up front that this is a pluggable
> API,
> > > but
> > > > you need to add more jars to take advantage of it. I think this is
> fine
> > > as
> > > > I don't think there are truly common secrets provider
> > > > formats/apis/protocols, just want to make sure it is clear.
> > > >
> > > > Thanks,
> > > > Ewen
> > > >
> > > > On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com> wrote:
> > > >
> > > > > +1
> > > > > -------- Original message --------From: Magesh Nandakumar <
> > > > > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > > > > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing
> > > Secrets
> > > > > for Connect Configurations
> > > > > Thanks Robert, this looks great
> > > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > >
> > > > > > Thanks, Robert!
> > > > > >
> > > > > > +1 (non-binding)
> > > > > >
> > > > > > Colin
> > > > > >
> > > > > >
> > > > > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > I've changed the KIP to have a composite object returned from
> > > get().
> > > > > > It's
> > > > > > > probably the most straightforward option.  Please let me know
> if
> > > you
> > > > > have
> > > > > > > any other concerns.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Robert
> > > > > > >
> > > > > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <
> > > rayokota@gmail.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > My last response was not that clear, so let me back up and
> > > explain
> > > > a
> > > > > > bit
> > > > > > > > more.
> > > > > > > >
> > > > > > > > Some secret managers, such as Vault (and maybe Keywhiz) have
> > the
> > > > > > notion of
> > > > > > > > a lease duration or a TTL for a path.  Every path can have a
> > > > > different
> > > > > > > > TTL.  This is period after which the value of the keys at the
> > > given
> > > > > > path
> > > > > > > > may be invalid.  It can be used to indicate a rotation will
> be
> > > > done.
> > > > > > In
> > > > > > > > the cause of the Vault integration with AWS, Vault will
> > actually
> > > > > > delete the
> > > > > > > > secrets from AWS at the moment the TTL expires.  A TTL could
> be
> > > > used
> > > > > by
> > > > > > > > other ConfigProviders, such as a FileConfigProvider, to
> > indicate
> > > > that
> > > > > > all
> > > > > > > > the secrets at a given path (file), will be rotated on a
> > regular
> > > > > basis.
> > > > > > > >
> > > > > > > > I would like to expose the TTL in the APIs somewhere.  The
> TTL
> > > can
> > > > be
> > > > > > made
> > > > > > > > available at the time get() is called.  Connect already has a
> > > built
> > > > > in
> > > > > > > > ScheduledExecutor, so Connect can just use the TTL to
> schedule
> > a
> > > > > > Connector
> > > > > > > > restart.  Originally, I had exposed the TTL in a
> ConfigContext
> > > > > > interface
> > > > > > > > passed to the get() method.  To reduce the number of APIs, I
> > > placed
> > > > > it
> > > > > > on
> > > > > > > > the onChange() method.  This means at the time of get(),
> > > onChange()
> > > > > > would
> > > > > > > > be called with a TTL.  The Connector's implementation of the
> > > > callback
> > > > > > would
> > > > > > > > use onChange() with the TTL to schedule a restart.
> > > > > > > >
> > > > > > > > If you think this is overloading onChange() too much, I could
> > add
> > > > the
> > > > > > > > ConfigContext back to get():
> > > > > > > >
> > > > > > > >
> > > > > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > > > > >
> > > > > > > > public interface ConfigContext {
> > > > > > > >
> > > > > > > >     void willExpire(String path, long ttl);
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > or I could separate out the TTL method in the callback:
> > > > > > > >
> > > > > > > >
> > > > > > > > public interface ConfigChangeCallback {
> > > > > > > >
> > > > > > > >     void willExpire(String path, long ttl);
> > > > > > > >
> > > > > > > >     void onChange(String path, Map<String, String> values);
> > > > > > > > }
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Or we could return a composite object from get():
> > > > > > > >
> > > > > > > > ConfigData get(String path);
> > > > > > > >
> > > > > > > > public class ConfigData {
> > > > > > > >
> > > > > > > >   Map<String, String> data;
> > > > > > > >   long ttl;
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > >
> > > > > > > > Do you have a preference Colin?
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Robert
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <
> > > cmccabe@apache.org>
> > > > > > wrote:
> > > > > > > >
> > > > > > > >> Hi Robert,
> > > > > > > >>
> > > > > > > >> Hmm.  I thought that if you're using ConfigChangeCallback,
> you
> > > are
> > > > > > > >> relying on the ConfigProvider to make a callback to you when
> > the
> > > > > > > >> configuration has changed.  So isn't that always the "push
> > > model"
> > > > > > (where
> > > > > > > >> the ConfigProvider pushes changes to Connect).  If you want
> > the
> > > > > "pull
> > > > > > > >> model" where you initiate updates, you can simply call
> > > > > > ConfigProvider#get
> > > > > > > >> directly, right?
> > > > > > > >>
> > > > > > > >> The actual implementation of ConfigProvider subclasses will
> > > depend
> > > > > on
> > > > > > the
> > > > > > > >> type of configuration storage mechanism on the backend.  In
> > the
> > > > case
> > > > > > of
> > > > > > > >> Vault, it sounds like we need to have something like a
> > > > > > ScheduledExecutor
> > > > > > > >> which re-fetches keys after a certain amount of time.
> > > > > > > >>
> > > > > > > >> As an aside, what does a "lease duration" mean for a
> > > configuration
> > > > > > key?
> > > > > > > >> Does that mean Vault will reject changes to the
> configuration
> > > key
> > > > if
> > > > > > I try
> > > > > > > >> to make them within the lease duration?  Or is this like a
> > > period
> > > > > > after
> > > > > > > >> which a password is automatically rotated?
> > > > > > > >>
> > > > > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > > > > >> > Hi Colin,
> > > > > > > >> >
> > > > > > > >> > > With regard to delayMs, can’t we just restart the
> > > > > > > >> > > Connector when the keys are actually changed?
> > > > > > > >> >
> > > > > > > >> > Currently the VaultConfigProvider does not find out when
> > > values
> > > > > for
> > > > > > keys
> > > > > > > >> > have changed.  You could do this with a poll model (with a
> > > > > > > >> > background thread in the ConfigProvider), but since for
> each
> > > > > > key-value
> > > > > > > >> > pair, Vault provides a lease duration stating exactly
> when a
> > > > value
> > > > > > for a
> > > > > > > >> > key will change in the future, this is an alternative
> model
> > of
> > > > > just
> > > > > > > >> passing
> > > > > > > >> > the lease duration to the client (in this case the
> > Connector),
> > > > to
> > > > > > allow
> > > > > > > >> it
> > > > > > > >> > to determine what to do (such as schedule a restart).
>  This
> > > may
> > > > > > allow
> > > > > > > >> one
> > > > > > > >> > to avoid the complexity of figuring out a proper poll
> > interval
> > > > > (with
> > > > > > > >> lease
> > > > > > > >> > durations of varying periods), or worrying about putting
> too
> > > > much
> > > > > > load
> > > > > > > >> on
> > > > > > > >> > the secrets manager by polling too often.
> > > > > > > >>
> > > > > > > >> Those things are still concerns if the Connector is polling,
> > > > right?
> > > > > > > >> Perhaps the connector poll too often and puts too much load
> on
> > > > > > Vault.  And
> > > > > > > >> so forth.  It seems like this problem needs to be solved
> > either
> > > > way
> > > > > > (and
> > > > > > > >> probably can be solved with reasonable default minimum fetch
> > > > > > intervals).
> > > > > > > >>
> > > > > > > >> best,
> > > > > > > >> Colin
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> >  In other words, by adding this
> > > > > > > >> > one additional parameter, a ConfigProvider can provide
> both
> > > push
> > > > > and
> > > > > > > >> pull
> > > > > > > >> > models to clients, perhaps with an additional
> configuration
> > > > > > parameter to
> > > > > > > >> > the ConfigProvider to determine which model (push or poll)
> > to
> > > > use.
> > > > > > > >> >
> > > > > > > >> > Thanks,
> > > > > > > >> > Robert
> > > > > > > >> >
> > > > > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> > > > cmccabe@apache.org
> > > > > >
> > > > > > > >> wrote:
> > > > > > > >> >
> > > > > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just
> > > restart
> > > > > the
> > > > > > > >> > > Connector when the keys are actually changed?  Or is the
> > > > concern
> > > > > > that
> > > > > > > >> > > this would lengthen the effective key rotation time?
> > Can’t
> > > > the
> > > > > > user
> > > > > > > >> > > just configure a slightly shorter key rotation time to
> > > > > counteract
> > > > > > > >> > > this concern?
> > > > > > > >> > > Regards,
> > > > > > > >> > > Colin
> > > > > > > >> > >
> > > > > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > > > > >> > > > Hi Colin,
> > > > > > > >> > > >
> > > > > > > >> > > > Good questions.
> > > > > > > >> > > >
> > > > > > > >> > > >
> > > > > > > >> > > > > As a clarification about the indirections, what if I
> > > have
> > > > > the
> > > > > > > >> > > > > connect> configuration key foo set up as
> ${vault:bar},
> > > and
> > > > > in
> > > > > > > >> Vault,
> > > > > > > >> > > > have the bar> key set to ${file:baz}?
> > > > > > > >> > > > > Does connect get foo as the contents of the baz
> > file?  I
> > > > > would
> > > > > > > >> > > > > argue that> it should not (and in general, we
> > shouldn't
> > > > > allow
> > > > > > > >> > > ConfigProviders to
> > > > > > > >> > > > indirect to other
> > > > > > > >> > > > > ConfigProviders) but I don't think it's spelled out
> > > right
> > > > > now.
> > > > > > > >> > > >
> > > > > > > >> > > > I've added a clarification to the KIP that further
> > > > > indirections
> > > > > > are
> > > > > > > >> > > > not> performed even if the values returned from
> > > > > ConfigProviders
> > > > > > > >> have the
> > > > > > > >> > > > variable syntax.
> > > > > > > >> > > >
> > > > > > > >> > > >
> > > > > > > >> > > > > What's the behavior when a config key is not found
> in
> > > > Vault
> > > > > > > >> > > > > (or other> ConfigProvider)?  Does the variable get
> > > > replaced
> > > > > > with
> > > > > > > >> the
> > > > > > > >> > > empty
> > > > > > > >> > > > string, or> with the literal ${vault:whatever} string?
> > > > > > > >> > > >
> > > > > > > >> > > > It would remain unresolved and still be of the form
> > > > > > > >> > > > ${provider:key}.  I've> added a clarification to the
> > KIP.
> > > > > > > >> > > >
> > > > > > > >> > > >
> > > > > > > >> > > > > Do we really need "${provider:[path:]key}", or can
> it
> > > just
> > > > > be
> > > > > > > >> > > > ${provider:key}?
> > > > > > > >> > > >
> > > > > > > >> > > > The path is a separate parameter in the APIs, so I
> think
> > > > it's
> > > > > > > >> > > > important to> explicitly delineate it in the variable
> > > > syntax.
> > > > > > For
> > > > > > > >> > > example, I
> > > > > > > >> > > > currently> have a working VaultConfigProvider
> prototype
> > > and
> > > > > the
> > > > > > > >> syntax
> > > > > > > >> > > for a
> > > > > > > >> > > > Vault key> reference looks like
> > > > > > > >> > > >
> > > > > > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > > > > > >> > > >
> > > > > > > >> > > > I think it's important to standardize how to separate
> > the
> > > > path
> > > > > > > >> > > > from the key> rather than leave it to each
> > ConfigProvider
> > > to
> > > > > > > >> determine a
> > > > > > > >> > > possibly
> > > > > > > >> > > > different way.  This will also make it easier to move
> > > > secrets
> > > > > > from
> > > > > > > >> one>
> > > > > > > >> > > ConfigProvider to another should one choose to do so.
> > > > > > > >> > > >
> > > > > > > >> > > >
> > > > > > > >> > > > > Do we really need delayMs?
> > > > > > > >> > > >
> > > > > > > >> > > > One of the goals of this KIP is to allow for secrets
> > > > rotation
> > > > > > > >> without>
> > > > > > > >> > > having to modify existing connectors.  In the case of
> the
> > > > > > > >> > > > VaultConfigProvider, it knows the lease durations and
> > will
> > > > be
> > > > > > able
> > > > > > > >> to>
> > > > > > > >> > > schedule a restart of the Connector using an API in the
> > > > Herder.
> > > > > > The
> > > > > > > >> > > > delayMs will simply be passed to the
> > > > > > Herder.restartConnector(long
> > > > > > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > > > > > >> > > >
> > > > > > > >> > > > https://github.com/rayokota/
> > > kafka/blob/secrets-in-connect-
> > > > > > > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > > > > > >> > > connect/runtime/Herder.java#L170>
> > > > > > > >> > > >
> > > > > > > >> > > > Best,
> > > > > > > >> > > > Robert
> > > > > > > >> > > >
> > > > > > > >> > > >
> > > > > > > >> > > >
> > > > > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > > > > >> > > > <cm...@apache.org> wrote:>
> > > > > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > > > > >> > > > >
> > > > > > > >> > > > > As a clarification about the indirections, what if I
> > > have
> > > > > the
> > > > > > > >> > > > > connect> > configuration key foo set up as
> > ${vault:bar},
> > > > and
> > > > > > in
> > > > > > > >> Vault,
> > > > > > > >> > > have
> > > > > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect get
> > foo
> > > > as
> > > > > > the
> > > > > > > >> > > contents of
> > > > > > > >> > > > > the baz> > file?  I would argue that it should not
> > (and
> > > in
> > > > > > > >> general, we
> > > > > > > >> > > > > shouldn't allow> > ConfigProviders to indirect to
> > other
> > > > > > > >> > > ConfigProviders) but I
> > > > > > > >> > > > > don't think> > it's spelled out right now.
> > > > > > > >> > > > >
> > > > > > > >> > > > > What's the behavior when a config key is not found
> in
> > > > Vault
> > > > > > > >> > > > > (or other> > ConfigProvider)?  Does the variable get
> > > > > replaced
> > > > > > > >> with the
> > > > > > > >> > > empty
> > > > > > > >> > > > > string, or> > with the literal ${vault:whatever}
> > string?
> > > > > > > >> > > > >
> > > > > > > >> > > > > Do we really need "${provider:[path:]key}", or can
> it
> > > just
> > > > > be
> > > > > > > >> > > > > ${provider:key}?  It seems like the path can be
> rolled
> > > up
> > > > > > into the
> > > > > > > >> > > > > key.  So> > if you want to put your connect keys
> under
> > > > > > > >> > > my.connect.path, you
> > > > > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config},
> etc.
> > > > > > > >> > > > >
> > > > > > > >> > > > > >    // A delayMs of 0 indicates an immediate
> change;
> > a
> > > > > > positive
> > > > > > > >> > > > > >    delayMs> > indicates
> > > > > > > >> > > > > >    // that a future change is anticipated (such
> as a
> > > > lease
> > > > > > > >> > > > > >    duration)> > >    void onChange(String path,
> > > > > Map<String,
> > > > > > > >> String>
> > > > > > > >> > > values, int
> > > > > > > >> > > > > >    delayMs);> >
> > > > > > > >> > > > > Do we really need delayMs?  It seems like if you
> get a
> > > > > > callback
> > > > > > > >> with>
> > > > > > > >> > > > delayMs set, you don't know what the new values will
> be,
> > > > only
> > > > > > > >> > > > > that an> > update is coming, but not yet here.
> > > > > > > >> > > > >
> > > > > > > >> > > > > best,
> > > > > > > >> > > > > Colin
> > > > > > > >> > > > >
> > > > > > > >> > > > >
> > > > > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > > > > >> > > > > > Hello everyone,
> > > > > > > >> > > > > >
> > > > > > > >> > > > > > After a good round of discussions with excellent
> > > > feedback
> > > > > > and no
> > > > > > > >> > > > > > major> > > objections, I would like to start a
> vote
> > on
> > > > > > KIP-297
> > > > > > > >> to
> > > > > > > >> > > externalize> > secrets
> > > > > > > >> > > > > > from Kafka Connect configurations.  My thanks in
> > > > advance!
> > > > > > > >> > > > > >
> > > > > > > >> > > > > > KIP: <
> > > > > > > >> > > > > > https://cwiki.apache.org/
> > > confluence/display/KAFKA/KIP-
> > > > > > > >> > > > > 297%3A+Externalizing+Secrets+
> > for+Connect+Configurations
> > > > > > > >> > > > > > >
> > > > > > > >> > > > > >
> > > > > > > >> > > > > > JIRA: <https://issues.apache.org/
> > > jira/browse/KAFKA-6886
> > > > >
> > > > > > > >> > > > > >
> > > > > > > >> > > > > > Discussion thread: <
> > > > > > > >> > > > > >
> > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > > > > html
> > > > > > > >> >
> > > > > > > >> > > > > >
> > > > > > > >> > > > > > Best,
> > > > > > > >> > > > > > Robert
> > > > > > > >> > > > >
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Rajini Sivaram <ra...@gmail.com>.

Thanks, Robert! Sounds good. And thanks for the KIP.

+1 (binding)

Regards,

Rajini

On Fri, May 18, 2018 at 4:17 PM, Robert Yokota <ra...@gmail.com> wrote:

> HI Rajini,
>
> Good questions.
>
> First, if no ConfigProviders are configured, then config values of the form
> ${vault:mypassword} will remain as is.
>
> Second, I mention in the KIP that if a provider does not have a value for a
> given key, the variable will remain unresolved and the final value will be
> of the form ${vault:mypassword} still.
>
> If one wants to use a config value ${vault:mypassword}, as well as the
> VaultConfigProvider, one can choose to use a different prefix besides
> "vault" when referring to the VaultConfigProvider since the prefixes are
> arbitrary and specified in a config file.
>
> Finally, if one want to use a config value ${vault:mypassword}, as well as
> the VaultConfigProvider, and one wants to use the prefix "vault" and not
> something else, then yes, one could use a LiteralConfigProvider as you
> described, or even put the ${vault:mypassword} in a different file and use
> the FileConfigProvider to pull in the value (since there is only one level
> of indirection).
>
> Thanks,
> Robert
>
>
>
> On Fri, May 18, 2018 at 3:42 AM, Rajini Sivaram <ra...@gmail.com>
> wrote:
>
> > Hi Robert,
> >
> > A couple of questions:
> >
> >
> >    1. Since we always expand config values, don't we also need a way to
> >    include values that never get expanded? I may want to use
> >    "${vault:mypassword}" as my literal password without a lookup. Since
> we
> >    allow only level of indirection, perhaps all we need is a
> ConfigProvider
> >    that uses the string inside, for example:
> ${literal:${vault:mypassword}}
> > ?
> >    It would avoid having restrictions on what passwords can look like.
> >    2. What is the behaviour if I specify a password that is
> >    "${notavault:something}" that matches the config provider syntax, but
> > for
> >    which there is no config provider?
> >
> >
> >
> > On Fri, May 18, 2018 at 5:41 AM, Ewen Cheslack-Postava <
> ewen@confluent.io>
> > wrote:
> >
> > > Thanks for addressing this Robert, it's a pretty common user need.
> > >
> > > First, +1 (binding) generally.
> > >
> > > Two very minor comments that I think could be clarified but wouldn't
> > affect
> > > votes:
> > >
> > > * Let's list in the KIP what package the ConfigProvider,
> > > ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
> > > defined in. Very, very minor, but given the aim to possibly reuse
> > elsewhere
> > > and the fact that it'll likely end up in the common packages might mean
> > > devs focused more on the common/core packages will have strong opinions
> > > where they should be. I think it'd definitely be good to get input from
> > > folks focusing on the broker on where they think it should go since I
> > think
> > > it would be very natural to extend this to security settings there.
> > (Also,
> > > I think ConfigData is left out of the list of new interfaces by
> accident,
> > > but I think it's clear what's being added anyway.)
> > > * I may have glanced past it, but we're not shipping any
> ConfigProviders
> > > out of the box? This mentions file and vault, but just as examples.
> Just
> > > want to make sure everyone knows up front that this is a pluggable API,
> > but
> > > you need to add more jars to take advantage of it. I think this is fine
> > as
> > > I don't think there are truly common secrets provider
> > > formats/apis/protocols, just want to make sure it is clear.
> > >
> > > Thanks,
> > > Ewen
> > >
> > > On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com> wrote:
> > >
> > > > +1
> > > > -------- Original message --------From: Magesh Nandakumar <
> > > > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > > > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing
> > Secrets
> > > > for Connect Configurations
> > > > Thanks Robert, this looks great
> > > >
> > > > +1 (non-binding)
> > > >
> > > > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org>
> > > wrote:
> > > >
> > > > > Thanks, Robert!
> > > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > > > Hi Colin,
> > > > > >
> > > > > > I've changed the KIP to have a composite object returned from
> > get().
> > > > > It's
> > > > > > probably the most straightforward option.  Please let me know if
> > you
> > > > have
> > > > > > any other concerns.
> > > > > >
> > > > > > Thanks,
> > > > > > Robert
> > > > > >
> > > > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <
> > rayokota@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > My last response was not that clear, so let me back up and
> > explain
> > > a
> > > > > bit
> > > > > > > more.
> > > > > > >
> > > > > > > Some secret managers, such as Vault (and maybe Keywhiz) have
> the
> > > > > notion of
> > > > > > > a lease duration or a TTL for a path.  Every path can have a
> > > > different
> > > > > > > TTL.  This is period after which the value of the keys at the
> > given
> > > > > path
> > > > > > > may be invalid.  It can be used to indicate a rotation will be
> > > done.
> > > > > In
> > > > > > > the cause of the Vault integration with AWS, Vault will
> actually
> > > > > delete the
> > > > > > > secrets from AWS at the moment the TTL expires.  A TTL could be
> > > used
> > > > by
> > > > > > > other ConfigProviders, such as a FileConfigProvider, to
> indicate
> > > that
> > > > > all
> > > > > > > the secrets at a given path (file), will be rotated on a
> regular
> > > > basis.
> > > > > > >
> > > > > > > I would like to expose the TTL in the APIs somewhere.  The TTL
> > can
> > > be
> > > > > made
> > > > > > > available at the time get() is called.  Connect already has a
> > built
> > > > in
> > > > > > > ScheduledExecutor, so Connect can just use the TTL to schedule
> a
> > > > > Connector
> > > > > > > restart.  Originally, I had exposed the TTL in a ConfigContext
> > > > > interface
> > > > > > > passed to the get() method.  To reduce the number of APIs, I
> > placed
> > > > it
> > > > > on
> > > > > > > the onChange() method.  This means at the time of get(),
> > onChange()
> > > > > would
> > > > > > > be called with a TTL.  The Connector's implementation of the
> > > callback
> > > > > would
> > > > > > > use onChange() with the TTL to schedule a restart.
> > > > > > >
> > > > > > > If you think this is overloading onChange() too much, I could
> add
> > > the
> > > > > > > ConfigContext back to get():
> > > > > > >
> > > > > > >
> > > > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > > > >
> > > > > > > public interface ConfigContext {
> > > > > > >
> > > > > > >     void willExpire(String path, long ttl);
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > or I could separate out the TTL method in the callback:
> > > > > > >
> > > > > > >
> > > > > > > public interface ConfigChangeCallback {
> > > > > > >
> > > > > > >     void willExpire(String path, long ttl);
> > > > > > >
> > > > > > >     void onChange(String path, Map<String, String> values);
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Or we could return a composite object from get():
> > > > > > >
> > > > > > > ConfigData get(String path);
> > > > > > >
> > > > > > > public class ConfigData {
> > > > > > >
> > > > > > >   Map<String, String> data;
> > > > > > >   long ttl;
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > > Do you have a preference Colin?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Robert
> > > > > > >
> > > > > > >
> > > > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <
> > cmccabe@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > >> Hi Robert,
> > > > > > >>
> > > > > > >> Hmm.  I thought that if you're using ConfigChangeCallback, you
> > are
> > > > > > >> relying on the ConfigProvider to make a callback to you when
> the
> > > > > > >> configuration has changed.  So isn't that always the "push
> > model"
> > > > > (where
> > > > > > >> the ConfigProvider pushes changes to Connect).  If you want
> the
> > > > "pull
> > > > > > >> model" where you initiate updates, you can simply call
> > > > > ConfigProvider#get
> > > > > > >> directly, right?
> > > > > > >>
> > > > > > >> The actual implementation of ConfigProvider subclasses will
> > depend
> > > > on
> > > > > the
> > > > > > >> type of configuration storage mechanism on the backend.  In
> the
> > > case
> > > > > of
> > > > > > >> Vault, it sounds like we need to have something like a
> > > > > ScheduledExecutor
> > > > > > >> which re-fetches keys after a certain amount of time.
> > > > > > >>
> > > > > > >> As an aside, what does a "lease duration" mean for a
> > configuration
> > > > > key?
> > > > > > >> Does that mean Vault will reject changes to the configuration
> > key
> > > if
> > > > > I try
> > > > > > >> to make them within the lease duration?  Or is this like a
> > period
> > > > > after
> > > > > > >> which a password is automatically rotated?
> > > > > > >>
> > > > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > > > >> > Hi Colin,
> > > > > > >> >
> > > > > > >> > > With regard to delayMs, can’t we just restart the
> > > > > > >> > > Connector when the keys are actually changed?
> > > > > > >> >
> > > > > > >> > Currently the VaultConfigProvider does not find out when
> > values
> > > > for
> > > > > keys
> > > > > > >> > have changed.  You could do this with a poll model (with a
> > > > > > >> > background thread in the ConfigProvider), but since for each
> > > > > key-value
> > > > > > >> > pair, Vault provides a lease duration stating exactly when a
> > > value
> > > > > for a
> > > > > > >> > key will change in the future, this is an alternative model
> of
> > > > just
> > > > > > >> passing
> > > > > > >> > the lease duration to the client (in this case the
> Connector),
> > > to
> > > > > allow
> > > > > > >> it
> > > > > > >> > to determine what to do (such as schedule a restart).   This
> > may
> > > > > allow
> > > > > > >> one
> > > > > > >> > to avoid the complexity of figuring out a proper poll
> interval
> > > > (with
> > > > > > >> lease
> > > > > > >> > durations of varying periods), or worrying about putting too
> > > much
> > > > > load
> > > > > > >> on
> > > > > > >> > the secrets manager by polling too often.
> > > > > > >>
> > > > > > >> Those things are still concerns if the Connector is polling,
> > > right?
> > > > > > >> Perhaps the connector poll too often and puts too much load on
> > > > > Vault.  And
> > > > > > >> so forth.  It seems like this problem needs to be solved
> either
> > > way
> > > > > (and
> > > > > > >> probably can be solved with reasonable default minimum fetch
> > > > > intervals).
> > > > > > >>
> > > > > > >> best,
> > > > > > >> Colin
> > > > > > >>
> > > > > > >>
> > > > > > >> >  In other words, by adding this
> > > > > > >> > one additional parameter, a ConfigProvider can provide both
> > push
> > > > and
> > > > > > >> pull
> > > > > > >> > models to clients, perhaps with an additional configuration
> > > > > parameter to
> > > > > > >> > the ConfigProvider to determine which model (push or poll)
> to
> > > use.
> > > > > > >> >
> > > > > > >> > Thanks,
> > > > > > >> > Robert
> > > > > > >> >
> > > > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> > > cmccabe@apache.org
> > > > >
> > > > > > >> wrote:
> > > > > > >> >
> > > > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just
> > restart
> > > > the
> > > > > > >> > > Connector when the keys are actually changed?  Or is the
> > > concern
> > > > > that
> > > > > > >> > > this would lengthen the effective key rotation time?
> Can’t
> > > the
> > > > > user
> > > > > > >> > > just configure a slightly shorter key rotation time to
> > > > counteract
> > > > > > >> > > this concern?
> > > > > > >> > > Regards,
> > > > > > >> > > Colin
> > > > > > >> > >
> > > > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > > > >> > > > Hi Colin,
> > > > > > >> > > >
> > > > > > >> > > > Good questions.
> > > > > > >> > > >
> > > > > > >> > > >
> > > > > > >> > > > > As a clarification about the indirections, what if I
> > have
> > > > the
> > > > > > >> > > > > connect> configuration key foo set up as ${vault:bar},
> > and
> > > > in
> > > > > > >> Vault,
> > > > > > >> > > > have the bar> key set to ${file:baz}?
> > > > > > >> > > > > Does connect get foo as the contents of the baz
> file?  I
> > > > would
> > > > > > >> > > > > argue that> it should not (and in general, we
> shouldn't
> > > > allow
> > > > > > >> > > ConfigProviders to
> > > > > > >> > > > indirect to other
> > > > > > >> > > > > ConfigProviders) but I don't think it's spelled out
> > right
> > > > now.
> > > > > > >> > > >
> > > > > > >> > > > I've added a clarification to the KIP that further
> > > > indirections
> > > > > are
> > > > > > >> > > > not> performed even if the values returned from
> > > > ConfigProviders
> > > > > > >> have the
> > > > > > >> > > > variable syntax.
> > > > > > >> > > >
> > > > > > >> > > >
> > > > > > >> > > > > What's the behavior when a config key is not found in
> > > Vault
> > > > > > >> > > > > (or other> ConfigProvider)?  Does the variable get
> > > replaced
> > > > > with
> > > > > > >> the
> > > > > > >> > > empty
> > > > > > >> > > > string, or> with the literal ${vault:whatever} string?
> > > > > > >> > > >
> > > > > > >> > > > It would remain unresolved and still be of the form
> > > > > > >> > > > ${provider:key}.  I've> added a clarification to the
> KIP.
> > > > > > >> > > >
> > > > > > >> > > >
> > > > > > >> > > > > Do we really need "${provider:[path:]key}", or can it
> > just
> > > > be
> > > > > > >> > > > ${provider:key}?
> > > > > > >> > > >
> > > > > > >> > > > The path is a separate parameter in the APIs, so I think
> > > it's
> > > > > > >> > > > important to> explicitly delineate it in the variable
> > > syntax.
> > > > > For
> > > > > > >> > > example, I
> > > > > > >> > > > currently> have a working VaultConfigProvider prototype
> > and
> > > > the
> > > > > > >> syntax
> > > > > > >> > > for a
> > > > > > >> > > > Vault key> reference looks like
> > > > > > >> > > >
> > > > > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > > > > >> > > >
> > > > > > >> > > > I think it's important to standardize how to separate
> the
> > > path
> > > > > > >> > > > from the key> rather than leave it to each
> ConfigProvider
> > to
> > > > > > >> determine a
> > > > > > >> > > possibly
> > > > > > >> > > > different way.  This will also make it easier to move
> > > secrets
> > > > > from
> > > > > > >> one>
> > > > > > >> > > ConfigProvider to another should one choose to do so.
> > > > > > >> > > >
> > > > > > >> > > >
> > > > > > >> > > > > Do we really need delayMs?
> > > > > > >> > > >
> > > > > > >> > > > One of the goals of this KIP is to allow for secrets
> > > rotation
> > > > > > >> without>
> > > > > > >> > > having to modify existing connectors.  In the case of the
> > > > > > >> > > > VaultConfigProvider, it knows the lease durations and
> will
> > > be
> > > > > able
> > > > > > >> to>
> > > > > > >> > > schedule a restart of the Connector using an API in the
> > > Herder.
> > > > > The
> > > > > > >> > > > delayMs will simply be passed to the
> > > > > Herder.restartConnector(long
> > > > > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > > > > >> > > >
> > > > > > >> > > > https://github.com/rayokota/
> > kafka/blob/secrets-in-connect-
> > > > > > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > > > > >> > > connect/runtime/Herder.java#L170>
> > > > > > >> > > >
> > > > > > >> > > > Best,
> > > > > > >> > > > Robert
> > > > > > >> > > >
> > > > > > >> > > >
> > > > > > >> > > >
> > > > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > > > >> > > > <cm...@apache.org> wrote:>
> > > > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > > > >> > > > >
> > > > > > >> > > > > As a clarification about the indirections, what if I
> > have
> > > > the
> > > > > > >> > > > > connect> > configuration key foo set up as
> ${vault:bar},
> > > and
> > > > > in
> > > > > > >> Vault,
> > > > > > >> > > have
> > > > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect get
> foo
> > > as
> > > > > the
> > > > > > >> > > contents of
> > > > > > >> > > > > the baz> > file?  I would argue that it should not
> (and
> > in
> > > > > > >> general, we
> > > > > > >> > > > > shouldn't allow> > ConfigProviders to indirect to
> other
> > > > > > >> > > ConfigProviders) but I
> > > > > > >> > > > > don't think> > it's spelled out right now.
> > > > > > >> > > > >
> > > > > > >> > > > > What's the behavior when a config key is not found in
> > > Vault
> > > > > > >> > > > > (or other> > ConfigProvider)?  Does the variable get
> > > > replaced
> > > > > > >> with the
> > > > > > >> > > empty
> > > > > > >> > > > > string, or> > with the literal ${vault:whatever}
> string?
> > > > > > >> > > > >
> > > > > > >> > > > > Do we really need "${provider:[path:]key}", or can it
> > just
> > > > be
> > > > > > >> > > > > ${provider:key}?  It seems like the path can be rolled
> > up
> > > > > into the
> > > > > > >> > > > > key.  So> > if you want to put your connect keys under
> > > > > > >> > > my.connect.path, you
> > > > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > > > > >> > > > >
> > > > > > >> > > > > >    // A delayMs of 0 indicates an immediate change;
> a
> > > > > positive
> > > > > > >> > > > > >    delayMs> > indicates
> > > > > > >> > > > > >    // that a future change is anticipated (such as a
> > > lease
> > > > > > >> > > > > >    duration)> > >    void onChange(String path,
> > > > Map<String,
> > > > > > >> String>
> > > > > > >> > > values, int
> > > > > > >> > > > > >    delayMs);> >
> > > > > > >> > > > > Do we really need delayMs?  It seems like if you get a
> > > > > callback
> > > > > > >> with>
> > > > > > >> > > > delayMs set, you don't know what the new values will be,
> > > only
> > > > > > >> > > > > that an> > update is coming, but not yet here.
> > > > > > >> > > > >
> > > > > > >> > > > > best,
> > > > > > >> > > > > Colin
> > > > > > >> > > > >
> > > > > > >> > > > >
> > > > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > > > >> > > > > > Hello everyone,
> > > > > > >> > > > > >
> > > > > > >> > > > > > After a good round of discussions with excellent
> > > feedback
> > > > > and no
> > > > > > >> > > > > > major> > > objections, I would like to start a vote
> on
> > > > > KIP-297
> > > > > > >> to
> > > > > > >> > > externalize> > secrets
> > > > > > >> > > > > > from Kafka Connect configurations.  My thanks in
> > > advance!
> > > > > > >> > > > > >
> > > > > > >> > > > > > KIP: <
> > > > > > >> > > > > > https://cwiki.apache.org/
> > confluence/display/KAFKA/KIP-
> > > > > > >> > > > > 297%3A+Externalizing+Secrets+
> for+Connect+Configurations
> > > > > > >> > > > > > >
> > > > > > >> > > > > >
> > > > > > >> > > > > > JIRA: <https://issues.apache.org/
> > jira/browse/KAFKA-6886
> > > >
> > > > > > >> > > > > >
> > > > > > >> > > > > > Discussion thread: <
> > > > > > >> > > > > >
> > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > > > html
> > > > > > >> >
> > > > > > >> > > > > >
> > > > > > >> > > > > > Best,
> > > > > > >> > > > > > Robert
> > > > > > >> > > > >
> > > > > > >> > >
> > > > > > >> > >
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Robert Yokota <ra...@gmail.com>.

HI Rajini,

Good questions.

First, if no ConfigProviders are configured, then config values of the form
${vault:mypassword} will remain as is.

Second, I mention in the KIP that if a provider does not have a value for a
given key, the variable will remain unresolved and the final value will be
of the form ${vault:mypassword} still.

If one wants to use a config value ${vault:mypassword}, as well as the
VaultConfigProvider, one can choose to use a different prefix besides
"vault" when referring to the VaultConfigProvider since the prefixes are
arbitrary and specified in a config file.

Finally, if one want to use a config value ${vault:mypassword}, as well as
the VaultConfigProvider, and one wants to use the prefix "vault" and not
something else, then yes, one could use a LiteralConfigProvider as you
described, or even put the ${vault:mypassword} in a different file and use
the FileConfigProvider to pull in the value (since there is only one level
of indirection).

Thanks,
Robert



On Fri, May 18, 2018 at 3:42 AM, Rajini Sivaram <ra...@gmail.com>
wrote:

> Hi Robert,
>
> A couple of questions:
>
>
>    1. Since we always expand config values, don't we also need a way to
>    include values that never get expanded? I may want to use
>    "${vault:mypassword}" as my literal password without a lookup. Since we
>    allow only level of indirection, perhaps all we need is a ConfigProvider
>    that uses the string inside, for example: ${literal:${vault:mypassword}}
> ?
>    It would avoid having restrictions on what passwords can look like.
>    2. What is the behaviour if I specify a password that is
>    "${notavault:something}" that matches the config provider syntax, but
> for
>    which there is no config provider?
>
>
>
> On Fri, May 18, 2018 at 5:41 AM, Ewen Cheslack-Postava <ew...@confluent.io>
> wrote:
>
> > Thanks for addressing this Robert, it's a pretty common user need.
> >
> > First, +1 (binding) generally.
> >
> > Two very minor comments that I think could be clarified but wouldn't
> affect
> > votes:
> >
> > * Let's list in the KIP what package the ConfigProvider,
> > ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
> > defined in. Very, very minor, but given the aim to possibly reuse
> elsewhere
> > and the fact that it'll likely end up in the common packages might mean
> > devs focused more on the common/core packages will have strong opinions
> > where they should be. I think it'd definitely be good to get input from
> > folks focusing on the broker on where they think it should go since I
> think
> > it would be very natural to extend this to security settings there.
> (Also,
> > I think ConfigData is left out of the list of new interfaces by accident,
> > but I think it's clear what's being added anyway.)
> > * I may have glanced past it, but we're not shipping any ConfigProviders
> > out of the box? This mentions file and vault, but just as examples. Just
> > want to make sure everyone knows up front that this is a pluggable API,
> but
> > you need to add more jars to take advantage of it. I think this is fine
> as
> > I don't think there are truly common secrets provider
> > formats/apis/protocols, just want to make sure it is clear.
> >
> > Thanks,
> > Ewen
> >
> > On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com> wrote:
> >
> > > +1
> > > -------- Original message --------From: Magesh Nandakumar <
> > > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing
> Secrets
> > > for Connect Configurations
> > > Thanks Robert, this looks great
> > >
> > > +1 (non-binding)
> > >
> > > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org>
> > wrote:
> > >
> > > > Thanks, Robert!
> > > >
> > > > +1 (non-binding)
> > > >
> > > > Colin
> > > >
> > > >
> > > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > > Hi Colin,
> > > > >
> > > > > I've changed the KIP to have a composite object returned from
> get().
> > > > It's
> > > > > probably the most straightforward option.  Please let me know if
> you
> > > have
> > > > > any other concerns.
> > > > >
> > > > > Thanks,
> > > > > Robert
> > > > >
> > > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <
> rayokota@gmail.com>
> > > > wrote:
> > > > >
> > > > > >
> > > > > >
> > > > > > Hi Colin,
> > > > > >
> > > > > > My last response was not that clear, so let me back up and
> explain
> > a
> > > > bit
> > > > > > more.
> > > > > >
> > > > > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> > > > notion of
> > > > > > a lease duration or a TTL for a path.  Every path can have a
> > > different
> > > > > > TTL.  This is period after which the value of the keys at the
> given
> > > > path
> > > > > > may be invalid.  It can be used to indicate a rotation will be
> > done.
> > > > In
> > > > > > the cause of the Vault integration with AWS, Vault will actually
> > > > delete the
> > > > > > secrets from AWS at the moment the TTL expires.  A TTL could be
> > used
> > > by
> > > > > > other ConfigProviders, such as a FileConfigProvider, to indicate
> > that
> > > > all
> > > > > > the secrets at a given path (file), will be rotated on a regular
> > > basis.
> > > > > >
> > > > > > I would like to expose the TTL in the APIs somewhere.  The TTL
> can
> > be
> > > > made
> > > > > > available at the time get() is called.  Connect already has a
> built
> > > in
> > > > > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> > > > Connector
> > > > > > restart.  Originally, I had exposed the TTL in a ConfigContext
> > > > interface
> > > > > > passed to the get() method.  To reduce the number of APIs, I
> placed
> > > it
> > > > on
> > > > > > the onChange() method.  This means at the time of get(),
> onChange()
> > > > would
> > > > > > be called with a TTL.  The Connector's implementation of the
> > callback
> > > > would
> > > > > > use onChange() with the TTL to schedule a restart.
> > > > > >
> > > > > > If you think this is overloading onChange() too much, I could add
> > the
> > > > > > ConfigContext back to get():
> > > > > >
> > > > > >
> > > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > > >
> > > > > > public interface ConfigContext {
> > > > > >
> > > > > >     void willExpire(String path, long ttl);
> > > > > >
> > > > > > }
> > > > > >
> > > > > >
> > > > > >
> > > > > > or I could separate out the TTL method in the callback:
> > > > > >
> > > > > >
> > > > > > public interface ConfigChangeCallback {
> > > > > >
> > > > > >     void willExpire(String path, long ttl);
> > > > > >
> > > > > >     void onChange(String path, Map<String, String> values);
> > > > > > }
> > > > > >
> > > > > >
> > > > > >
> > > > > > Or we could return a composite object from get():
> > > > > >
> > > > > > ConfigData get(String path);
> > > > > >
> > > > > > public class ConfigData {
> > > > > >
> > > > > >   Map<String, String> data;
> > > > > >   long ttl;
> > > > > >
> > > > > > }
> > > > > >
> > > > > >
> > > > > > Do you have a preference Colin?
> > > > > >
> > > > > > Thanks,
> > > > > > Robert
> > > > > >
> > > > > >
> > > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <
> cmccabe@apache.org>
> > > > wrote:
> > > > > >
> > > > > >> Hi Robert,
> > > > > >>
> > > > > >> Hmm.  I thought that if you're using ConfigChangeCallback, you
> are
> > > > > >> relying on the ConfigProvider to make a callback to you when the
> > > > > >> configuration has changed.  So isn't that always the "push
> model"
> > > > (where
> > > > > >> the ConfigProvider pushes changes to Connect).  If you want the
> > > "pull
> > > > > >> model" where you initiate updates, you can simply call
> > > > ConfigProvider#get
> > > > > >> directly, right?
> > > > > >>
> > > > > >> The actual implementation of ConfigProvider subclasses will
> depend
> > > on
> > > > the
> > > > > >> type of configuration storage mechanism on the backend.  In the
> > case
> > > > of
> > > > > >> Vault, it sounds like we need to have something like a
> > > > ScheduledExecutor
> > > > > >> which re-fetches keys after a certain amount of time.
> > > > > >>
> > > > > >> As an aside, what does a "lease duration" mean for a
> configuration
> > > > key?
> > > > > >> Does that mean Vault will reject changes to the configuration
> key
> > if
> > > > I try
> > > > > >> to make them within the lease duration?  Or is this like a
> period
> > > > after
> > > > > >> which a password is automatically rotated?
> > > > > >>
> > > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > > >> > Hi Colin,
> > > > > >> >
> > > > > >> > > With regard to delayMs, can’t we just restart the
> > > > > >> > > Connector when the keys are actually changed?
> > > > > >> >
> > > > > >> > Currently the VaultConfigProvider does not find out when
> values
> > > for
> > > > keys
> > > > > >> > have changed.  You could do this with a poll model (with a
> > > > > >> > background thread in the ConfigProvider), but since for each
> > > > key-value
> > > > > >> > pair, Vault provides a lease duration stating exactly when a
> > value
> > > > for a
> > > > > >> > key will change in the future, this is an alternative model of
> > > just
> > > > > >> passing
> > > > > >> > the lease duration to the client (in this case the Connector),
> > to
> > > > allow
> > > > > >> it
> > > > > >> > to determine what to do (such as schedule a restart).   This
> may
> > > > allow
> > > > > >> one
> > > > > >> > to avoid the complexity of figuring out a proper poll interval
> > > (with
> > > > > >> lease
> > > > > >> > durations of varying periods), or worrying about putting too
> > much
> > > > load
> > > > > >> on
> > > > > >> > the secrets manager by polling too often.
> > > > > >>
> > > > > >> Those things are still concerns if the Connector is polling,
> > right?
> > > > > >> Perhaps the connector poll too often and puts too much load on
> > > > Vault.  And
> > > > > >> so forth.  It seems like this problem needs to be solved either
> > way
> > > > (and
> > > > > >> probably can be solved with reasonable default minimum fetch
> > > > intervals).
> > > > > >>
> > > > > >> best,
> > > > > >> Colin
> > > > > >>
> > > > > >>
> > > > > >> >  In other words, by adding this
> > > > > >> > one additional parameter, a ConfigProvider can provide both
> push
> > > and
> > > > > >> pull
> > > > > >> > models to clients, perhaps with an additional configuration
> > > > parameter to
> > > > > >> > the ConfigProvider to determine which model (push or poll) to
> > use.
> > > > > >> >
> > > > > >> > Thanks,
> > > > > >> > Robert
> > > > > >> >
> > > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> > cmccabe@apache.org
> > > >
> > > > > >> wrote:
> > > > > >> >
> > > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just
> restart
> > > the
> > > > > >> > > Connector when the keys are actually changed?  Or is the
> > concern
> > > > that
> > > > > >> > > this would lengthen the effective key rotation time?  Can’t
> > the
> > > > user
> > > > > >> > > just configure a slightly shorter key rotation time to
> > > counteract
> > > > > >> > > this concern?
> > > > > >> > > Regards,
> > > > > >> > > Colin
> > > > > >> > >
> > > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > > >> > > > Hi Colin,
> > > > > >> > > >
> > > > > >> > > > Good questions.
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > > > As a clarification about the indirections, what if I
> have
> > > the
> > > > > >> > > > > connect> configuration key foo set up as ${vault:bar},
> and
> > > in
> > > > > >> Vault,
> > > > > >> > > > have the bar> key set to ${file:baz}?
> > > > > >> > > > > Does connect get foo as the contents of the baz file?  I
> > > would
> > > > > >> > > > > argue that> it should not (and in general, we shouldn't
> > > allow
> > > > > >> > > ConfigProviders to
> > > > > >> > > > indirect to other
> > > > > >> > > > > ConfigProviders) but I don't think it's spelled out
> right
> > > now.
> > > > > >> > > >
> > > > > >> > > > I've added a clarification to the KIP that further
> > > indirections
> > > > are
> > > > > >> > > > not> performed even if the values returned from
> > > ConfigProviders
> > > > > >> have the
> > > > > >> > > > variable syntax.
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > > > What's the behavior when a config key is not found in
> > Vault
> > > > > >> > > > > (or other> ConfigProvider)?  Does the variable get
> > replaced
> > > > with
> > > > > >> the
> > > > > >> > > empty
> > > > > >> > > > string, or> with the literal ${vault:whatever} string?
> > > > > >> > > >
> > > > > >> > > > It would remain unresolved and still be of the form
> > > > > >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > > > Do we really need "${provider:[path:]key}", or can it
> just
> > > be
> > > > > >> > > > ${provider:key}?
> > > > > >> > > >
> > > > > >> > > > The path is a separate parameter in the APIs, so I think
> > it's
> > > > > >> > > > important to> explicitly delineate it in the variable
> > syntax.
> > > > For
> > > > > >> > > example, I
> > > > > >> > > > currently> have a working VaultConfigProvider prototype
> and
> > > the
> > > > > >> syntax
> > > > > >> > > for a
> > > > > >> > > > Vault key> reference looks like
> > > > > >> > > >
> > > > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > > > >> > > >
> > > > > >> > > > I think it's important to standardize how to separate the
> > path
> > > > > >> > > > from the key> rather than leave it to each ConfigProvider
> to
> > > > > >> determine a
> > > > > >> > > possibly
> > > > > >> > > > different way.  This will also make it easier to move
> > secrets
> > > > from
> > > > > >> one>
> > > > > >> > > ConfigProvider to another should one choose to do so.
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > > > Do we really need delayMs?
> > > > > >> > > >
> > > > > >> > > > One of the goals of this KIP is to allow for secrets
> > rotation
> > > > > >> without>
> > > > > >> > > having to modify existing connectors.  In the case of the
> > > > > >> > > > VaultConfigProvider, it knows the lease durations and will
> > be
> > > > able
> > > > > >> to>
> > > > > >> > > schedule a restart of the Connector using an API in the
> > Herder.
> > > > The
> > > > > >> > > > delayMs will simply be passed to the
> > > > Herder.restartConnector(long
> > > > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > > > >> > > >
> > > > > >> > > > https://github.com/rayokota/
> kafka/blob/secrets-in-connect-
> > > > > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > > > >> > > connect/runtime/Herder.java#L170>
> > > > > >> > > >
> > > > > >> > > > Best,
> > > > > >> > > > Robert
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > >
> > > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > > >> > > > <cm...@apache.org> wrote:>
> > > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > > >> > > > >
> > > > > >> > > > > As a clarification about the indirections, what if I
> have
> > > the
> > > > > >> > > > > connect> > configuration key foo set up as ${vault:bar},
> > and
> > > > in
> > > > > >> Vault,
> > > > > >> > > have
> > > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo
> > as
> > > > the
> > > > > >> > > contents of
> > > > > >> > > > > the baz> > file?  I would argue that it should not (and
> in
> > > > > >> general, we
> > > > > >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > > > > >> > > ConfigProviders) but I
> > > > > >> > > > > don't think> > it's spelled out right now.
> > > > > >> > > > >
> > > > > >> > > > > What's the behavior when a config key is not found in
> > Vault
> > > > > >> > > > > (or other> > ConfigProvider)?  Does the variable get
> > > replaced
> > > > > >> with the
> > > > > >> > > empty
> > > > > >> > > > > string, or> > with the literal ${vault:whatever} string?
> > > > > >> > > > >
> > > > > >> > > > > Do we really need "${provider:[path:]key}", or can it
> just
> > > be
> > > > > >> > > > > ${provider:key}?  It seems like the path can be rolled
> up
> > > > into the
> > > > > >> > > > > key.  So> > if you want to put your connect keys under
> > > > > >> > > my.connect.path, you
> > > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > > > >> > > > >
> > > > > >> > > > > >    // A delayMs of 0 indicates an immediate change; a
> > > > positive
> > > > > >> > > > > >    delayMs> > indicates
> > > > > >> > > > > >    // that a future change is anticipated (such as a
> > lease
> > > > > >> > > > > >    duration)> > >    void onChange(String path,
> > > Map<String,
> > > > > >> String>
> > > > > >> > > values, int
> > > > > >> > > > > >    delayMs);> >
> > > > > >> > > > > Do we really need delayMs?  It seems like if you get a
> > > > callback
> > > > > >> with>
> > > > > >> > > > delayMs set, you don't know what the new values will be,
> > only
> > > > > >> > > > > that an> > update is coming, but not yet here.
> > > > > >> > > > >
> > > > > >> > > > > best,
> > > > > >> > > > > Colin
> > > > > >> > > > >
> > > > > >> > > > >
> > > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > > >> > > > > > Hello everyone,
> > > > > >> > > > > >
> > > > > >> > > > > > After a good round of discussions with excellent
> > feedback
> > > > and no
> > > > > >> > > > > > major> > > objections, I would like to start a vote on
> > > > KIP-297
> > > > > >> to
> > > > > >> > > externalize> > secrets
> > > > > >> > > > > > from Kafka Connect configurations.  My thanks in
> > advance!
> > > > > >> > > > > >
> > > > > >> > > > > > KIP: <
> > > > > >> > > > > > https://cwiki.apache.org/
> confluence/display/KAFKA/KIP-
> > > > > >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > > > >> > > > > > >
> > > > > >> > > > > >
> > > > > >> > > > > > JIRA: <https://issues.apache.org/
> jira/browse/KAFKA-6886
> > >
> > > > > >> > > > > >
> > > > > >> > > > > > Discussion thread: <
> > > > > >> > > > > >
> > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > > html
> > > > > >> >
> > > > > >> > > > > >
> > > > > >> > > > > > Best,
> > > > > >> > > > > > Robert
> > > > > >> > > > >
> > > > > >> > >
> > > > > >> > >
> > > > > >>
> > > > > >
> > > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Rajini Sivaram <ra...@gmail.com>.

Hi Robert,

A couple of questions:


   1. Since we always expand config values, don't we also need a way to
   include values that never get expanded? I may want to use
   "${vault:mypassword}" as my literal password without a lookup. Since we
   allow only level of indirection, perhaps all we need is a ConfigProvider
   that uses the string inside, for example: ${literal:${vault:mypassword}}?
   It would avoid having restrictions on what passwords can look like.
   2. What is the behaviour if I specify a password that is
   "${notavault:something}" that matches the config provider syntax, but for
   which there is no config provider?



On Fri, May 18, 2018 at 5:41 AM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:

> Thanks for addressing this Robert, it's a pretty common user need.
>
> First, +1 (binding) generally.
>
> Two very minor comments that I think could be clarified but wouldn't affect
> votes:
>
> * Let's list in the KIP what package the ConfigProvider,
> ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
> defined in. Very, very minor, but given the aim to possibly reuse elsewhere
> and the fact that it'll likely end up in the common packages might mean
> devs focused more on the common/core packages will have strong opinions
> where they should be. I think it'd definitely be good to get input from
> folks focusing on the broker on where they think it should go since I think
> it would be very natural to extend this to security settings there. (Also,
> I think ConfigData is left out of the list of new interfaces by accident,
> but I think it's clear what's being added anyway.)
> * I may have glanced past it, but we're not shipping any ConfigProviders
> out of the box? This mentions file and vault, but just as examples. Just
> want to make sure everyone knows up front that this is a pluggable API, but
> you need to add more jars to take advantage of it. I think this is fine as
> I don't think there are truly common secrets provider
> formats/apis/protocols, just want to make sure it is clear.
>
> Thanks,
> Ewen
>
> On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com> wrote:
>
> > +1
> > -------- Original message --------From: Magesh Nandakumar <
> > mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> > dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing Secrets
> > for Connect Configurations
> > Thanks Robert, this looks great
> >
> > +1 (non-binding)
> >
> > On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org>
> wrote:
> >
> > > Thanks, Robert!
> > >
> > > +1 (non-binding)
> > >
> > > Colin
> > >
> > >
> > > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > > Hi Colin,
> > > >
> > > > I've changed the KIP to have a composite object returned from get().
> > > It's
> > > > probably the most straightforward option.  Please let me know if you
> > have
> > > > any other concerns.
> > > >
> > > > Thanks,
> > > > Robert
> > > >
> > > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com>
> > > wrote:
> > > >
> > > > >
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > My last response was not that clear, so let me back up and explain
> a
> > > bit
> > > > > more.
> > > > >
> > > > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> > > notion of
> > > > > a lease duration or a TTL for a path.  Every path can have a
> > different
> > > > > TTL.  This is period after which the value of the keys at the given
> > > path
> > > > > may be invalid.  It can be used to indicate a rotation will be
> done.
> > > In
> > > > > the cause of the Vault integration with AWS, Vault will actually
> > > delete the
> > > > > secrets from AWS at the moment the TTL expires.  A TTL could be
> used
> > by
> > > > > other ConfigProviders, such as a FileConfigProvider, to indicate
> that
> > > all
> > > > > the secrets at a given path (file), will be rotated on a regular
> > basis.
> > > > >
> > > > > I would like to expose the TTL in the APIs somewhere.  The TTL can
> be
> > > made
> > > > > available at the time get() is called.  Connect already has a built
> > in
> > > > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> > > Connector
> > > > > restart.  Originally, I had exposed the TTL in a ConfigContext
> > > interface
> > > > > passed to the get() method.  To reduce the number of APIs, I placed
> > it
> > > on
> > > > > the onChange() method.  This means at the time of get(), onChange()
> > > would
> > > > > be called with a TTL.  The Connector's implementation of the
> callback
> > > would
> > > > > use onChange() with the TTL to schedule a restart.
> > > > >
> > > > > If you think this is overloading onChange() too much, I could add
> the
> > > > > ConfigContext back to get():
> > > > >
> > > > >
> > > > > Map<String, String> get(ConfigContext ctx, String path);
> > > > >
> > > > > public interface ConfigContext {
> > > > >
> > > > >     void willExpire(String path, long ttl);
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > or I could separate out the TTL method in the callback:
> > > > >
> > > > >
> > > > > public interface ConfigChangeCallback {
> > > > >
> > > > >     void willExpire(String path, long ttl);
> > > > >
> > > > >     void onChange(String path, Map<String, String> values);
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > Or we could return a composite object from get():
> > > > >
> > > > > ConfigData get(String path);
> > > > >
> > > > > public class ConfigData {
> > > > >
> > > > >   Map<String, String> data;
> > > > >   long ttl;
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > > Do you have a preference Colin?
> > > > >
> > > > > Thanks,
> > > > > Robert
> > > > >
> > > > >
> > > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org>
> > > wrote:
> > > > >
> > > > >> Hi Robert,
> > > > >>
> > > > >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> > > > >> relying on the ConfigProvider to make a callback to you when the
> > > > >> configuration has changed.  So isn't that always the "push model"
> > > (where
> > > > >> the ConfigProvider pushes changes to Connect).  If you want the
> > "pull
> > > > >> model" where you initiate updates, you can simply call
> > > ConfigProvider#get
> > > > >> directly, right?
> > > > >>
> > > > >> The actual implementation of ConfigProvider subclasses will depend
> > on
> > > the
> > > > >> type of configuration storage mechanism on the backend.  In the
> case
> > > of
> > > > >> Vault, it sounds like we need to have something like a
> > > ScheduledExecutor
> > > > >> which re-fetches keys after a certain amount of time.
> > > > >>
> > > > >> As an aside, what does a "lease duration" mean for a configuration
> > > key?
> > > > >> Does that mean Vault will reject changes to the configuration key
> if
> > > I try
> > > > >> to make them within the lease duration?  Or is this like a period
> > > after
> > > > >> which a password is automatically rotated?
> > > > >>
> > > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > > >> > Hi Colin,
> > > > >> >
> > > > >> > > With regard to delayMs, can’t we just restart the
> > > > >> > > Connector when the keys are actually changed?
> > > > >> >
> > > > >> > Currently the VaultConfigProvider does not find out when values
> > for
> > > keys
> > > > >> > have changed.  You could do this with a poll model (with a
> > > > >> > background thread in the ConfigProvider), but since for each
> > > key-value
> > > > >> > pair, Vault provides a lease duration stating exactly when a
> value
> > > for a
> > > > >> > key will change in the future, this is an alternative model of
> > just
> > > > >> passing
> > > > >> > the lease duration to the client (in this case the Connector),
> to
> > > allow
> > > > >> it
> > > > >> > to determine what to do (such as schedule a restart).   This may
> > > allow
> > > > >> one
> > > > >> > to avoid the complexity of figuring out a proper poll interval
> > (with
> > > > >> lease
> > > > >> > durations of varying periods), or worrying about putting too
> much
> > > load
> > > > >> on
> > > > >> > the secrets manager by polling too often.
> > > > >>
> > > > >> Those things are still concerns if the Connector is polling,
> right?
> > > > >> Perhaps the connector poll too often and puts too much load on
> > > Vault.  And
> > > > >> so forth.  It seems like this problem needs to be solved either
> way
> > > (and
> > > > >> probably can be solved with reasonable default minimum fetch
> > > intervals).
> > > > >>
> > > > >> best,
> > > > >> Colin
> > > > >>
> > > > >>
> > > > >> >  In other words, by adding this
> > > > >> > one additional parameter, a ConfigProvider can provide both push
> > and
> > > > >> pull
> > > > >> > models to clients, perhaps with an additional configuration
> > > parameter to
> > > > >> > the ConfigProvider to determine which model (push or poll) to
> use.
> > > > >> >
> > > > >> > Thanks,
> > > > >> > Robert
> > > > >> >
> > > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <
> cmccabe@apache.org
> > >
> > > > >> wrote:
> > > > >> >
> > > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just restart
> > the
> > > > >> > > Connector when the keys are actually changed?  Or is the
> concern
> > > that
> > > > >> > > this would lengthen the effective key rotation time?  Can’t
> the
> > > user
> > > > >> > > just configure a slightly shorter key rotation time to
> > counteract
> > > > >> > > this concern?
> > > > >> > > Regards,
> > > > >> > > Colin
> > > > >> > >
> > > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > >> > > > Hi Colin,
> > > > >> > > >
> > > > >> > > > Good questions.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > As a clarification about the indirections, what if I have
> > the
> > > > >> > > > > connect> configuration key foo set up as ${vault:bar}, and
> > in
> > > > >> Vault,
> > > > >> > > > have the bar> key set to ${file:baz}?
> > > > >> > > > > Does connect get foo as the contents of the baz file?  I
> > would
> > > > >> > > > > argue that> it should not (and in general, we shouldn't
> > allow
> > > > >> > > ConfigProviders to
> > > > >> > > > indirect to other
> > > > >> > > > > ConfigProviders) but I don't think it's spelled out right
> > now.
> > > > >> > > >
> > > > >> > > > I've added a clarification to the KIP that further
> > indirections
> > > are
> > > > >> > > > not> performed even if the values returned from
> > ConfigProviders
> > > > >> have the
> > > > >> > > > variable syntax.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > What's the behavior when a config key is not found in
> Vault
> > > > >> > > > > (or other> ConfigProvider)?  Does the variable get
> replaced
> > > with
> > > > >> the
> > > > >> > > empty
> > > > >> > > > string, or> with the literal ${vault:whatever} string?
> > > > >> > > >
> > > > >> > > > It would remain unresolved and still be of the form
> > > > >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > Do we really need "${provider:[path:]key}", or can it just
> > be
> > > > >> > > > ${provider:key}?
> > > > >> > > >
> > > > >> > > > The path is a separate parameter in the APIs, so I think
> it's
> > > > >> > > > important to> explicitly delineate it in the variable
> syntax.
> > > For
> > > > >> > > example, I
> > > > >> > > > currently> have a working VaultConfigProvider prototype and
> > the
> > > > >> syntax
> > > > >> > > for a
> > > > >> > > > Vault key> reference looks like
> > > > >> > > >
> > > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > > >> > > >
> > > > >> > > > I think it's important to standardize how to separate the
> path
> > > > >> > > > from the key> rather than leave it to each ConfigProvider to
> > > > >> determine a
> > > > >> > > possibly
> > > > >> > > > different way.  This will also make it easier to move
> secrets
> > > from
> > > > >> one>
> > > > >> > > ConfigProvider to another should one choose to do so.
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > > Do we really need delayMs?
> > > > >> > > >
> > > > >> > > > One of the goals of this KIP is to allow for secrets
> rotation
> > > > >> without>
> > > > >> > > having to modify existing connectors.  In the case of the
> > > > >> > > > VaultConfigProvider, it knows the lease durations and will
> be
> > > able
> > > > >> to>
> > > > >> > > schedule a restart of the Connector using an API in the
> Herder.
> > > The
> > > > >> > > > delayMs will simply be passed to the
> > > Herder.restartConnector(long
> > > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > > >> > > >
> > > > >> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > > > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > > >> > > connect/runtime/Herder.java#L170>
> > > > >> > > >
> > > > >> > > > Best,
> > > > >> > > > Robert
> > > > >> > > >
> > > > >> > > >
> > > > >> > > >
> > > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > >> > > > <cm...@apache.org> wrote:>
> > > > >> > > > > Thanks, Robert.  Looks good overall.
> > > > >> > > > >
> > > > >> > > > > As a clarification about the indirections, what if I have
> > the
> > > > >> > > > > connect> > configuration key foo set up as ${vault:bar},
> and
> > > in
> > > > >> Vault,
> > > > >> > > have
> > > > >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo
> as
> > > the
> > > > >> > > contents of
> > > > >> > > > > the baz> > file?  I would argue that it should not (and in
> > > > >> general, we
> > > > >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > > > >> > > ConfigProviders) but I
> > > > >> > > > > don't think> > it's spelled out right now.
> > > > >> > > > >
> > > > >> > > > > What's the behavior when a config key is not found in
> Vault
> > > > >> > > > > (or other> > ConfigProvider)?  Does the variable get
> > replaced
> > > > >> with the
> > > > >> > > empty
> > > > >> > > > > string, or> > with the literal ${vault:whatever} string?
> > > > >> > > > >
> > > > >> > > > > Do we really need "${provider:[path:]key}", or can it just
> > be
> > > > >> > > > > ${provider:key}?  It seems like the path can be rolled up
> > > into the
> > > > >> > > > > key.  So> > if you want to put your connect keys under
> > > > >> > > my.connect.path, you
> > > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > > >> > > > >
> > > > >> > > > > >    // A delayMs of 0 indicates an immediate change; a
> > > positive
> > > > >> > > > > >    delayMs> > indicates
> > > > >> > > > > >    // that a future change is anticipated (such as a
> lease
> > > > >> > > > > >    duration)> > >    void onChange(String path,
> > Map<String,
> > > > >> String>
> > > > >> > > values, int
> > > > >> > > > > >    delayMs);> >
> > > > >> > > > > Do we really need delayMs?  It seems like if you get a
> > > callback
> > > > >> with>
> > > > >> > > > delayMs set, you don't know what the new values will be,
> only
> > > > >> > > > > that an> > update is coming, but not yet here.
> > > > >> > > > >
> > > > >> > > > > best,
> > > > >> > > > > Colin
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > >> > > > > > Hello everyone,
> > > > >> > > > > >
> > > > >> > > > > > After a good round of discussions with excellent
> feedback
> > > and no
> > > > >> > > > > > major> > > objections, I would like to start a vote on
> > > KIP-297
> > > > >> to
> > > > >> > > externalize> > secrets
> > > > >> > > > > > from Kafka Connect configurations.  My thanks in
> advance!
> > > > >> > > > > >
> > > > >> > > > > > KIP: <
> > > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > > >> > > > > > >
> > > > >> > > > > >
> > > > >> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886
> >
> > > > >> > > > > >
> > > > >> > > > > > Discussion thread: <
> > > > >> > > > > >
> > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > > html
> > > > >> >
> > > > >> > > > > >
> > > > >> > > > > > Best,
> > > > >> > > > > > Robert
> > > > >> > > > >
> > > > >> > >
> > > > >> > >
> > > > >>
> > > > >
> > > > >
> > >
> >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Ewen Cheslack-Postava <ew...@confluent.io>.

Thanks for addressing this Robert, it's a pretty common user need.

First, +1 (binding) generally.

Two very minor comments that I think could be clarified but wouldn't affect
votes:

* Let's list in the KIP what package the ConfigProvider,
ConfigChangeCallback, ConfigData and ConfigTransformer interfaces are
defined in. Very, very minor, but given the aim to possibly reuse elsewhere
and the fact that it'll likely end up in the common packages might mean
devs focused more on the common/core packages will have strong opinions
where they should be. I think it'd definitely be good to get input from
folks focusing on the broker on where they think it should go since I think
it would be very natural to extend this to security settings there. (Also,
I think ConfigData is left out of the list of new interfaces by accident,
but I think it's clear what's being added anyway.)
* I may have glanced past it, but we're not shipping any ConfigProviders
out of the box? This mentions file and vault, but just as examples. Just
want to make sure everyone knows up front that this is a pluggable API, but
you need to add more jars to take advantage of it. I think this is fine as
I don't think there are truly common secrets provider
formats/apis/protocols, just want to make sure it is clear.

Thanks,
Ewen

On Thu, May 17, 2018 at 6:19 PM Ted Yu <yu...@gmail.com> wrote:

> +1
> -------- Original message --------From: Magesh Nandakumar <
> mageshn@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To:
> dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing Secrets
> for Connect Configurations
> Thanks Robert, this looks great
>
> +1 (non-binding)
>
> On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org> wrote:
>
> > Thanks, Robert!
> >
> > +1 (non-binding)
> >
> > Colin
> >
> >
> > On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > > Hi Colin,
> > >
> > > I've changed the KIP to have a composite object returned from get().
> > It's
> > > probably the most straightforward option.  Please let me know if you
> have
> > > any other concerns.
> > >
> > > Thanks,
> > > Robert
> > >
> > > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com>
> > wrote:
> > >
> > > >
> > > >
> > > > Hi Colin,
> > > >
> > > > My last response was not that clear, so let me back up and explain a
> > bit
> > > > more.
> > > >
> > > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> > notion of
> > > > a lease duration or a TTL for a path.  Every path can have a
> different
> > > > TTL.  This is period after which the value of the keys at the given
> > path
> > > > may be invalid.  It can be used to indicate a rotation will be done.
> > In
> > > > the cause of the Vault integration with AWS, Vault will actually
> > delete the
> > > > secrets from AWS at the moment the TTL expires.  A TTL could be used
> by
> > > > other ConfigProviders, such as a FileConfigProvider, to indicate that
> > all
> > > > the secrets at a given path (file), will be rotated on a regular
> basis.
> > > >
> > > > I would like to expose the TTL in the APIs somewhere.  The TTL can be
> > made
> > > > available at the time get() is called.  Connect already has a built
> in
> > > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> > Connector
> > > > restart.  Originally, I had exposed the TTL in a ConfigContext
> > interface
> > > > passed to the get() method.  To reduce the number of APIs, I placed
> it
> > on
> > > > the onChange() method.  This means at the time of get(), onChange()
> > would
> > > > be called with a TTL.  The Connector's implementation of the callback
> > would
> > > > use onChange() with the TTL to schedule a restart.
> > > >
> > > > If you think this is overloading onChange() too much, I could add the
> > > > ConfigContext back to get():
> > > >
> > > >
> > > > Map<String, String> get(ConfigContext ctx, String path);
> > > >
> > > > public interface ConfigContext {
> > > >
> > > >     void willExpire(String path, long ttl);
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > or I could separate out the TTL method in the callback:
> > > >
> > > >
> > > > public interface ConfigChangeCallback {
> > > >
> > > >     void willExpire(String path, long ttl);
> > > >
> > > >     void onChange(String path, Map<String, String> values);
> > > > }
> > > >
> > > >
> > > >
> > > > Or we could return a composite object from get():
> > > >
> > > > ConfigData get(String path);
> > > >
> > > > public class ConfigData {
> > > >
> > > >   Map<String, String> data;
> > > >   long ttl;
> > > >
> > > > }
> > > >
> > > >
> > > > Do you have a preference Colin?
> > > >
> > > > Thanks,
> > > > Robert
> > > >
> > > >
> > > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org>
> > wrote:
> > > >
> > > >> Hi Robert,
> > > >>
> > > >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> > > >> relying on the ConfigProvider to make a callback to you when the
> > > >> configuration has changed.  So isn't that always the "push model"
> > (where
> > > >> the ConfigProvider pushes changes to Connect).  If you want the
> "pull
> > > >> model" where you initiate updates, you can simply call
> > ConfigProvider#get
> > > >> directly, right?
> > > >>
> > > >> The actual implementation of ConfigProvider subclasses will depend
> on
> > the
> > > >> type of configuration storage mechanism on the backend.  In the case
> > of
> > > >> Vault, it sounds like we need to have something like a
> > ScheduledExecutor
> > > >> which re-fetches keys after a certain amount of time.
> > > >>
> > > >> As an aside, what does a "lease duration" mean for a configuration
> > key?
> > > >> Does that mean Vault will reject changes to the configuration key if
> > I try
> > > >> to make them within the lease duration?  Or is this like a period
> > after
> > > >> which a password is automatically rotated?
> > > >>
> > > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > > >> > Hi Colin,
> > > >> >
> > > >> > > With regard to delayMs, can’t we just restart the
> > > >> > > Connector when the keys are actually changed?
> > > >> >
> > > >> > Currently the VaultConfigProvider does not find out when values
> for
> > keys
> > > >> > have changed.  You could do this with a poll model (with a
> > > >> > background thread in the ConfigProvider), but since for each
> > key-value
> > > >> > pair, Vault provides a lease duration stating exactly when a value
> > for a
> > > >> > key will change in the future, this is an alternative model of
> just
> > > >> passing
> > > >> > the lease duration to the client (in this case the Connector), to
> > allow
> > > >> it
> > > >> > to determine what to do (such as schedule a restart).   This may
> > allow
> > > >> one
> > > >> > to avoid the complexity of figuring out a proper poll interval
> (with
> > > >> lease
> > > >> > durations of varying periods), or worrying about putting too much
> > load
> > > >> on
> > > >> > the secrets manager by polling too often.
> > > >>
> > > >> Those things are still concerns if the Connector is polling, right?
> > > >> Perhaps the connector poll too often and puts too much load on
> > Vault.  And
> > > >> so forth.  It seems like this problem needs to be solved either way
> > (and
> > > >> probably can be solved with reasonable default minimum fetch
> > intervals).
> > > >>
> > > >> best,
> > > >> Colin
> > > >>
> > > >>
> > > >> >  In other words, by adding this
> > > >> > one additional parameter, a ConfigProvider can provide both push
> and
> > > >> pull
> > > >> > models to clients, perhaps with an additional configuration
> > parameter to
> > > >> > the ConfigProvider to determine which model (push or poll) to use.
> > > >> >
> > > >> > Thanks,
> > > >> > Robert
> > > >> >
> > > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cmccabe@apache.org
> >
> > > >> wrote:
> > > >> >
> > > >> > > Thanks, Robert.  With regard to delayMs, can’t we just restart
> the
> > > >> > > Connector when the keys are actually changed?  Or is the concern
> > that
> > > >> > > this would lengthen the effective key rotation time?  Can’t the
> > user
> > > >> > > just configure a slightly shorter key rotation time to
> counteract
> > > >> > > this concern?
> > > >> > > Regards,
> > > >> > > Colin
> > > >> > >
> > > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > >> > > > Hi Colin,
> > > >> > > >
> > > >> > > > Good questions.
> > > >> > > >
> > > >> > > >
> > > >> > > > > As a clarification about the indirections, what if I have
> the
> > > >> > > > > connect> configuration key foo set up as ${vault:bar}, and
> in
> > > >> Vault,
> > > >> > > > have the bar> key set to ${file:baz}?
> > > >> > > > > Does connect get foo as the contents of the baz file?  I
> would
> > > >> > > > > argue that> it should not (and in general, we shouldn't
> allow
> > > >> > > ConfigProviders to
> > > >> > > > indirect to other
> > > >> > > > > ConfigProviders) but I don't think it's spelled out right
> now.
> > > >> > > >
> > > >> > > > I've added a clarification to the KIP that further
> indirections
> > are
> > > >> > > > not> performed even if the values returned from
> ConfigProviders
> > > >> have the
> > > >> > > > variable syntax.
> > > >> > > >
> > > >> > > >
> > > >> > > > > What's the behavior when a config key is not found in Vault
> > > >> > > > > (or other> ConfigProvider)?  Does the variable get replaced
> > with
> > > >> the
> > > >> > > empty
> > > >> > > > string, or> with the literal ${vault:whatever} string?
> > > >> > > >
> > > >> > > > It would remain unresolved and still be of the form
> > > >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > > >> > > >
> > > >> > > >
> > > >> > > > > Do we really need "${provider:[path:]key}", or can it just
> be
> > > >> > > > ${provider:key}?
> > > >> > > >
> > > >> > > > The path is a separate parameter in the APIs, so I think it's
> > > >> > > > important to> explicitly delineate it in the variable syntax.
> > For
> > > >> > > example, I
> > > >> > > > currently> have a working VaultConfigProvider prototype and
> the
> > > >> syntax
> > > >> > > for a
> > > >> > > > Vault key> reference looks like
> > > >> > > >
> > > >> > > > db_password=${vault:secret/staging:mysql_password}
> > > >> > > >
> > > >> > > > I think it's important to standardize how to separate the path
> > > >> > > > from the key> rather than leave it to each ConfigProvider to
> > > >> determine a
> > > >> > > possibly
> > > >> > > > different way.  This will also make it easier to move secrets
> > from
> > > >> one>
> > > >> > > ConfigProvider to another should one choose to do so.
> > > >> > > >
> > > >> > > >
> > > >> > > > > Do we really need delayMs?
> > > >> > > >
> > > >> > > > One of the goals of this KIP is to allow for secrets rotation
> > > >> without>
> > > >> > > having to modify existing connectors.  In the case of the
> > > >> > > > VaultConfigProvider, it knows the lease durations and will be
> > able
> > > >> to>
> > > >> > > schedule a restart of the Connector using an API in the Herder.
> > The
> > > >> > > > delayMs will simply be passed to the
> > Herder.restartConnector(long
> > > >> > > > delayMs,> String connName, Callback cb) method here:
> > > >> > > >
> > > >> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > >> > > connect/runtime/Herder.java#L170>
> > > >> > > >
> > > >> > > > Best,
> > > >> > > > Robert
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > >> > > > <cm...@apache.org> wrote:>
> > > >> > > > > Thanks, Robert.  Looks good overall.
> > > >> > > > >
> > > >> > > > > As a clarification about the indirections, what if I have
> the
> > > >> > > > > connect> > configuration key foo set up as ${vault:bar}, and
> > in
> > > >> Vault,
> > > >> > > have
> > > >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo as
> > the
> > > >> > > contents of
> > > >> > > > > the baz> > file?  I would argue that it should not (and in
> > > >> general, we
> > > >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > > >> > > ConfigProviders) but I
> > > >> > > > > don't think> > it's spelled out right now.
> > > >> > > > >
> > > >> > > > > What's the behavior when a config key is not found in Vault
> > > >> > > > > (or other> > ConfigProvider)?  Does the variable get
> replaced
> > > >> with the
> > > >> > > empty
> > > >> > > > > string, or> > with the literal ${vault:whatever} string?
> > > >> > > > >
> > > >> > > > > Do we really need "${provider:[path:]key}", or can it just
> be
> > > >> > > > > ${provider:key}?  It seems like the path can be rolled up
> > into the
> > > >> > > > > key.  So> > if you want to put your connect keys under
> > > >> > > my.connect.path, you
> > > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > >> > > > >
> > > >> > > > > >    // A delayMs of 0 indicates an immediate change; a
> > positive
> > > >> > > > > >    delayMs> > indicates
> > > >> > > > > >    // that a future change is anticipated (such as a lease
> > > >> > > > > >    duration)> > >    void onChange(String path,
> Map<String,
> > > >> String>
> > > >> > > values, int
> > > >> > > > > >    delayMs);> >
> > > >> > > > > Do we really need delayMs?  It seems like if you get a
> > callback
> > > >> with>
> > > >> > > > delayMs set, you don't know what the new values will be, only
> > > >> > > > > that an> > update is coming, but not yet here.
> > > >> > > > >
> > > >> > > > > best,
> > > >> > > > > Colin
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > >> > > > > > Hello everyone,
> > > >> > > > > >
> > > >> > > > > > After a good round of discussions with excellent feedback
> > and no
> > > >> > > > > > major> > > objections, I would like to start a vote on
> > KIP-297
> > > >> to
> > > >> > > externalize> > secrets
> > > >> > > > > > from Kafka Connect configurations.  My thanks in advance!
> > > >> > > > > >
> > > >> > > > > > KIP: <
> > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > >> > > > > > >
> > > >> > > > > >
> > > >> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > > >> > > > > >
> > > >> > > > > > Discussion thread: <
> > > >> > > > > >
> https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> > html
> > > >> >
> > > >> > > > > >
> > > >> > > > > > Best,
> > > >> > > > > > Robert
> > > >> > > > >
> > > >> > >
> > > >> > >
> > > >>
> > > >
> > > >
> >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Ted Yu <yu...@gmail.com>.

+1
-------- Original message --------From: Magesh Nandakumar <ma...@confluent.io> Date: 5/17/18  6:05 PM  (GMT-08:00) To: dev@kafka.apache.org Subject: Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations 
Thanks Robert, this looks great

+1 (non-binding)

On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org> wrote:

> Thanks, Robert!
>
> +1 (non-binding)
>
> Colin
>
>
> On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > Hi Colin,
> >
> > I've changed the KIP to have a composite object returned from get().
> It's
> > probably the most straightforward option.  Please let me know if you have
> > any other concerns.
> >
> > Thanks,
> > Robert
> >
> > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com>
> wrote:
> >
> > >
> > >
> > > Hi Colin,
> > >
> > > My last response was not that clear, so let me back up and explain a
> bit
> > > more.
> > >
> > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> notion of
> > > a lease duration or a TTL for a path.  Every path can have a different
> > > TTL.  This is period after which the value of the keys at the given
> path
> > > may be invalid.  It can be used to indicate a rotation will be done.
> In
> > > the cause of the Vault integration with AWS, Vault will actually
> delete the
> > > secrets from AWS at the moment the TTL expires.  A TTL could be used by
> > > other ConfigProviders, such as a FileConfigProvider, to indicate that
> all
> > > the secrets at a given path (file), will be rotated on a regular basis.
> > >
> > > I would like to expose the TTL in the APIs somewhere.  The TTL can be
> made
> > > available at the time get() is called.  Connect already has a built in
> > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> Connector
> > > restart.  Originally, I had exposed the TTL in a ConfigContext
> interface
> > > passed to the get() method.  To reduce the number of APIs, I placed it
> on
> > > the onChange() method.  This means at the time of get(), onChange()
> would
> > > be called with a TTL.  The Connector's implementation of the callback
> would
> > > use onChange() with the TTL to schedule a restart.
> > >
> > > If you think this is overloading onChange() too much, I could add the
> > > ConfigContext back to get():
> > >
> > >
> > > Map<String, String> get(ConfigContext ctx, String path);
> > >
> > > public interface ConfigContext {
> > >
> > >     void willExpire(String path, long ttl);
> > >
> > > }
> > >
> > >
> > >
> > > or I could separate out the TTL method in the callback:
> > >
> > >
> > > public interface ConfigChangeCallback {
> > >
> > >     void willExpire(String path, long ttl);
> > >
> > >     void onChange(String path, Map<String, String> values);
> > > }
> > >
> > >
> > >
> > > Or we could return a composite object from get():
> > >
> > > ConfigData get(String path);
> > >
> > > public class ConfigData {
> > >
> > >   Map<String, String> data;
> > >   long ttl;
> > >
> > > }
> > >
> > >
> > > Do you have a preference Colin?
> > >
> > > Thanks,
> > > Robert
> > >
> > >
> > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > >> Hi Robert,
> > >>
> > >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> > >> relying on the ConfigProvider to make a callback to you when the
> > >> configuration has changed.  So isn't that always the "push model"
> (where
> > >> the ConfigProvider pushes changes to Connect).  If you want the "pull
> > >> model" where you initiate updates, you can simply call
> ConfigProvider#get
> > >> directly, right?
> > >>
> > >> The actual implementation of ConfigProvider subclasses will depend on
> the
> > >> type of configuration storage mechanism on the backend.  In the case
> of
> > >> Vault, it sounds like we need to have something like a
> ScheduledExecutor
> > >> which re-fetches keys after a certain amount of time.
> > >>
> > >> As an aside, what does a "lease duration" mean for a configuration
> key?
> > >> Does that mean Vault will reject changes to the configuration key if
> I try
> > >> to make them within the lease duration?  Or is this like a period
> after
> > >> which a password is automatically rotated?
> > >>
> > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > >> > Hi Colin,
> > >> >
> > >> > > With regard to delayMs, can’t we just restart the
> > >> > > Connector when the keys are actually changed?
> > >> >
> > >> > Currently the VaultConfigProvider does not find out when values for
> keys
> > >> > have changed.  You could do this with a poll model (with a
> > >> > background thread in the ConfigProvider), but since for each
> key-value
> > >> > pair, Vault provides a lease duration stating exactly when a value
> for a
> > >> > key will change in the future, this is an alternative model of just
> > >> passing
> > >> > the lease duration to the client (in this case the Connector), to
> allow
> > >> it
> > >> > to determine what to do (such as schedule a restart).   This may
> allow
> > >> one
> > >> > to avoid the complexity of figuring out a proper poll interval (with
> > >> lease
> > >> > durations of varying periods), or worrying about putting too much
> load
> > >> on
> > >> > the secrets manager by polling too often.
> > >>
> > >> Those things are still concerns if the Connector is polling, right?
> > >> Perhaps the connector poll too often and puts too much load on
> Vault.  And
> > >> so forth.  It seems like this problem needs to be solved either way
> (and
> > >> probably can be solved with reasonable default minimum fetch
> intervals).
> > >>
> > >> best,
> > >> Colin
> > >>
> > >>
> > >> >  In other words, by adding this
> > >> > one additional parameter, a ConfigProvider can provide both push and
> > >> pull
> > >> > models to clients, perhaps with an additional configuration
> parameter to
> > >> > the ConfigProvider to determine which model (push or poll) to use.
> > >> >
> > >> > Thanks,
> > >> > Robert
> > >> >
> > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org>
> > >> wrote:
> > >> >
> > >> > > Thanks, Robert.  With regard to delayMs, can’t we just restart the
> > >> > > Connector when the keys are actually changed?  Or is the concern
> that
> > >> > > this would lengthen the effective key rotation time?  Can’t the
> user
> > >> > > just configure a slightly shorter key rotation time to counteract
> > >> > > this concern?
> > >> > > Regards,
> > >> > > Colin
> > >> > >
> > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > >> > > > Hi Colin,
> > >> > > >
> > >> > > > Good questions.
> > >> > > >
> > >> > > >
> > >> > > > > As a clarification about the indirections, what if I have the
> > >> > > > > connect> configuration key foo set up as ${vault:bar}, and in
> > >> Vault,
> > >> > > > have the bar> key set to ${file:baz}?
> > >> > > > > Does connect get foo as the contents of the baz file?  I would
> > >> > > > > argue that> it should not (and in general, we shouldn't allow
> > >> > > ConfigProviders to
> > >> > > > indirect to other
> > >> > > > > ConfigProviders) but I don't think it's spelled out right now.
> > >> > > >
> > >> > > > I've added a clarification to the KIP that further indirections
> are
> > >> > > > not> performed even if the values returned from ConfigProviders
> > >> have the
> > >> > > > variable syntax.
> > >> > > >
> > >> > > >
> > >> > > > > What's the behavior when a config key is not found in Vault
> > >> > > > > (or other> ConfigProvider)?  Does the variable get replaced
> with
> > >> the
> > >> > > empty
> > >> > > > string, or> with the literal ${vault:whatever} string?
> > >> > > >
> > >> > > > It would remain unresolved and still be of the form
> > >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > >> > > >
> > >> > > >
> > >> > > > > Do we really need "${provider:[path:]key}", or can it just be
> > >> > > > ${provider:key}?
> > >> > > >
> > >> > > > The path is a separate parameter in the APIs, so I think it's
> > >> > > > important to> explicitly delineate it in the variable syntax.
> For
> > >> > > example, I
> > >> > > > currently> have a working VaultConfigProvider prototype and the
> > >> syntax
> > >> > > for a
> > >> > > > Vault key> reference looks like
> > >> > > >
> > >> > > > db_password=${vault:secret/staging:mysql_password}
> > >> > > >
> > >> > > > I think it's important to standardize how to separate the path
> > >> > > > from the key> rather than leave it to each ConfigProvider to
> > >> determine a
> > >> > > possibly
> > >> > > > different way.  This will also make it easier to move secrets
> from
> > >> one>
> > >> > > ConfigProvider to another should one choose to do so.
> > >> > > >
> > >> > > >
> > >> > > > > Do we really need delayMs?
> > >> > > >
> > >> > > > One of the goals of this KIP is to allow for secrets rotation
> > >> without>
> > >> > > having to modify existing connectors.  In the case of the
> > >> > > > VaultConfigProvider, it knows the lease durations and will be
> able
> > >> to>
> > >> > > schedule a restart of the Connector using an API in the Herder.
> The
> > >> > > > delayMs will simply be passed to the
> Herder.restartConnector(long
> > >> > > > delayMs,> String connName, Callback cb) method here:
> > >> > > >
> > >> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > >> > > connect/runtime/Herder.java#L170>
> > >> > > >
> > >> > > > Best,
> > >> > > > Robert
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > >> > > > <cm...@apache.org> wrote:>
> > >> > > > > Thanks, Robert.  Looks good overall.
> > >> > > > >
> > >> > > > > As a clarification about the indirections, what if I have the
> > >> > > > > connect> > configuration key foo set up as ${vault:bar}, and
> in
> > >> Vault,
> > >> > > have
> > >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo as
> the
> > >> > > contents of
> > >> > > > > the baz> > file?  I would argue that it should not (and in
> > >> general, we
> > >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > >> > > ConfigProviders) but I
> > >> > > > > don't think> > it's spelled out right now.
> > >> > > > >
> > >> > > > > What's the behavior when a config key is not found in Vault
> > >> > > > > (or other> > ConfigProvider)?  Does the variable get replaced
> > >> with the
> > >> > > empty
> > >> > > > > string, or> > with the literal ${vault:whatever} string?
> > >> > > > >
> > >> > > > > Do we really need "${provider:[path:]key}", or can it just be
> > >> > > > > ${provider:key}?  It seems like the path can be rolled up
> into the
> > >> > > > > key.  So> > if you want to put your connect keys under
> > >> > > my.connect.path, you
> > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > >> > > > >
> > >> > > > > >    // A delayMs of 0 indicates an immediate change; a
> positive
> > >> > > > > >    delayMs> > indicates
> > >> > > > > >    // that a future change is anticipated (such as a lease
> > >> > > > > >    duration)> > >    void onChange(String path, Map<String,
> > >> String>
> > >> > > values, int
> > >> > > > > >    delayMs);> >
> > >> > > > > Do we really need delayMs?  It seems like if you get a
> callback
> > >> with>
> > >> > > > delayMs set, you don't know what the new values will be, only
> > >> > > > > that an> > update is coming, but not yet here.
> > >> > > > >
> > >> > > > > best,
> > >> > > > > Colin
> > >> > > > >
> > >> > > > >
> > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > >> > > > > > Hello everyone,
> > >> > > > > >
> > >> > > > > > After a good round of discussions with excellent feedback
> and no
> > >> > > > > > major> > > objections, I would like to start a vote on
> KIP-297
> > >> to
> > >> > > externalize> > secrets
> > >> > > > > > from Kafka Connect configurations.  My thanks in advance!
> > >> > > > > >
> > >> > > > > > KIP: <
> > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > >> > > > > > >
> > >> > > > > >
> > >> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > >> > > > > >
> > >> > > > > > Discussion thread: <
> > >> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> html
> > >> >
> > >> > > > > >
> > >> > > > > > Best,
> > >> > > > > > Robert
> > >> > > > >
> > >> > >
> > >> > >
> > >>
> > >
> > >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Magesh Nandakumar <ma...@confluent.io>.

Thanks Robert, this looks great

+1 (non-binding)

On Thu, May 17, 2018 at 5:35 PM, Colin McCabe <cm...@apache.org> wrote:

> Thanks, Robert!
>
> +1 (non-binding)
>
> Colin
>
>
> On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > Hi Colin,
> >
> > I've changed the KIP to have a composite object returned from get().
> It's
> > probably the most straightforward option.  Please let me know if you have
> > any other concerns.
> >
> > Thanks,
> > Robert
> >
> > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com>
> wrote:
> >
> > >
> > >
> > > Hi Colin,
> > >
> > > My last response was not that clear, so let me back up and explain a
> bit
> > > more.
> > >
> > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> notion of
> > > a lease duration or a TTL for a path.  Every path can have a different
> > > TTL.  This is period after which the value of the keys at the given
> path
> > > may be invalid.  It can be used to indicate a rotation will be done.
> In
> > > the cause of the Vault integration with AWS, Vault will actually
> delete the
> > > secrets from AWS at the moment the TTL expires.  A TTL could be used by
> > > other ConfigProviders, such as a FileConfigProvider, to indicate that
> all
> > > the secrets at a given path (file), will be rotated on a regular basis.
> > >
> > > I would like to expose the TTL in the APIs somewhere.  The TTL can be
> made
> > > available at the time get() is called.  Connect already has a built in
> > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> Connector
> > > restart.  Originally, I had exposed the TTL in a ConfigContext
> interface
> > > passed to the get() method.  To reduce the number of APIs, I placed it
> on
> > > the onChange() method.  This means at the time of get(), onChange()
> would
> > > be called with a TTL.  The Connector's implementation of the callback
> would
> > > use onChange() with the TTL to schedule a restart.
> > >
> > > If you think this is overloading onChange() too much, I could add the
> > > ConfigContext back to get():
> > >
> > >
> > > Map<String, String> get(ConfigContext ctx, String path);
> > >
> > > public interface ConfigContext {
> > >
> > >     void willExpire(String path, long ttl);
> > >
> > > }
> > >
> > >
> > >
> > > or I could separate out the TTL method in the callback:
> > >
> > >
> > > public interface ConfigChangeCallback {
> > >
> > >     void willExpire(String path, long ttl);
> > >
> > >     void onChange(String path, Map<String, String> values);
> > > }
> > >
> > >
> > >
> > > Or we could return a composite object from get():
> > >
> > > ConfigData get(String path);
> > >
> > > public class ConfigData {
> > >
> > >   Map<String, String> data;
> > >   long ttl;
> > >
> > > }
> > >
> > >
> > > Do you have a preference Colin?
> > >
> > > Thanks,
> > > Robert
> > >
> > >
> > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > >> Hi Robert,
> > >>
> > >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> > >> relying on the ConfigProvider to make a callback to you when the
> > >> configuration has changed.  So isn't that always the "push model"
> (where
> > >> the ConfigProvider pushes changes to Connect).  If you want the "pull
> > >> model" where you initiate updates, you can simply call
> ConfigProvider#get
> > >> directly, right?
> > >>
> > >> The actual implementation of ConfigProvider subclasses will depend on
> the
> > >> type of configuration storage mechanism on the backend.  In the case
> of
> > >> Vault, it sounds like we need to have something like a
> ScheduledExecutor
> > >> which re-fetches keys after a certain amount of time.
> > >>
> > >> As an aside, what does a "lease duration" mean for a configuration
> key?
> > >> Does that mean Vault will reject changes to the configuration key if
> I try
> > >> to make them within the lease duration?  Or is this like a period
> after
> > >> which a password is automatically rotated?
> > >>
> > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > >> > Hi Colin,
> > >> >
> > >> > > With regard to delayMs, can’t we just restart the
> > >> > > Connector when the keys are actually changed?
> > >> >
> > >> > Currently the VaultConfigProvider does not find out when values for
> keys
> > >> > have changed.  You could do this with a poll model (with a
> > >> > background thread in the ConfigProvider), but since for each
> key-value
> > >> > pair, Vault provides a lease duration stating exactly when a value
> for a
> > >> > key will change in the future, this is an alternative model of just
> > >> passing
> > >> > the lease duration to the client (in this case the Connector), to
> allow
> > >> it
> > >> > to determine what to do (such as schedule a restart).   This may
> allow
> > >> one
> > >> > to avoid the complexity of figuring out a proper poll interval (with
> > >> lease
> > >> > durations of varying periods), or worrying about putting too much
> load
> > >> on
> > >> > the secrets manager by polling too often.
> > >>
> > >> Those things are still concerns if the Connector is polling, right?
> > >> Perhaps the connector poll too often and puts too much load on
> Vault.  And
> > >> so forth.  It seems like this problem needs to be solved either way
> (and
> > >> probably can be solved with reasonable default minimum fetch
> intervals).
> > >>
> > >> best,
> > >> Colin
> > >>
> > >>
> > >> >  In other words, by adding this
> > >> > one additional parameter, a ConfigProvider can provide both push and
> > >> pull
> > >> > models to clients, perhaps with an additional configuration
> parameter to
> > >> > the ConfigProvider to determine which model (push or poll) to use.
> > >> >
> > >> > Thanks,
> > >> > Robert
> > >> >
> > >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org>
> > >> wrote:
> > >> >
> > >> > > Thanks, Robert.  With regard to delayMs, can’t we just restart the
> > >> > > Connector when the keys are actually changed?  Or is the concern
> that
> > >> > > this would lengthen the effective key rotation time?  Can’t the
> user
> > >> > > just configure a slightly shorter key rotation time to counteract
> > >> > > this concern?
> > >> > > Regards,
> > >> > > Colin
> > >> > >
> > >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > >> > > > Hi Colin,
> > >> > > >
> > >> > > > Good questions.
> > >> > > >
> > >> > > >
> > >> > > > > As a clarification about the indirections, what if I have the
> > >> > > > > connect> configuration key foo set up as ${vault:bar}, and in
> > >> Vault,
> > >> > > > have the bar> key set to ${file:baz}?
> > >> > > > > Does connect get foo as the contents of the baz file?  I would
> > >> > > > > argue that> it should not (and in general, we shouldn't allow
> > >> > > ConfigProviders to
> > >> > > > indirect to other
> > >> > > > > ConfigProviders) but I don't think it's spelled out right now.
> > >> > > >
> > >> > > > I've added a clarification to the KIP that further indirections
> are
> > >> > > > not> performed even if the values returned from ConfigProviders
> > >> have the
> > >> > > > variable syntax.
> > >> > > >
> > >> > > >
> > >> > > > > What's the behavior when a config key is not found in Vault
> > >> > > > > (or other> ConfigProvider)?  Does the variable get replaced
> with
> > >> the
> > >> > > empty
> > >> > > > string, or> with the literal ${vault:whatever} string?
> > >> > > >
> > >> > > > It would remain unresolved and still be of the form
> > >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > >> > > >
> > >> > > >
> > >> > > > > Do we really need "${provider:[path:]key}", or can it just be
> > >> > > > ${provider:key}?
> > >> > > >
> > >> > > > The path is a separate parameter in the APIs, so I think it's
> > >> > > > important to> explicitly delineate it in the variable syntax.
> For
> > >> > > example, I
> > >> > > > currently> have a working VaultConfigProvider prototype and the
> > >> syntax
> > >> > > for a
> > >> > > > Vault key> reference looks like
> > >> > > >
> > >> > > > db_password=${vault:secret/staging:mysql_password}
> > >> > > >
> > >> > > > I think it's important to standardize how to separate the path
> > >> > > > from the key> rather than leave it to each ConfigProvider to
> > >> determine a
> > >> > > possibly
> > >> > > > different way.  This will also make it easier to move secrets
> from
> > >> one>
> > >> > > ConfigProvider to another should one choose to do so.
> > >> > > >
> > >> > > >
> > >> > > > > Do we really need delayMs?
> > >> > > >
> > >> > > > One of the goals of this KIP is to allow for secrets rotation
> > >> without>
> > >> > > having to modify existing connectors.  In the case of the
> > >> > > > VaultConfigProvider, it knows the lease durations and will be
> able
> > >> to>
> > >> > > schedule a restart of the Connector using an API in the Herder.
> The
> > >> > > > delayMs will simply be passed to the
> Herder.restartConnector(long
> > >> > > > delayMs,> String connName, Callback cb) method here:
> > >> > > >
> > >> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > >> > > connect/runtime/Herder.java#L170>
> > >> > > >
> > >> > > > Best,
> > >> > > > Robert
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > >> > > > <cm...@apache.org> wrote:>
> > >> > > > > Thanks, Robert.  Looks good overall.
> > >> > > > >
> > >> > > > > As a clarification about the indirections, what if I have the
> > >> > > > > connect> > configuration key foo set up as ${vault:bar}, and
> in
> > >> Vault,
> > >> > > have
> > >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo as
> the
> > >> > > contents of
> > >> > > > > the baz> > file?  I would argue that it should not (and in
> > >> general, we
> > >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > >> > > ConfigProviders) but I
> > >> > > > > don't think> > it's spelled out right now.
> > >> > > > >
> > >> > > > > What's the behavior when a config key is not found in Vault
> > >> > > > > (or other> > ConfigProvider)?  Does the variable get replaced
> > >> with the
> > >> > > empty
> > >> > > > > string, or> > with the literal ${vault:whatever} string?
> > >> > > > >
> > >> > > > > Do we really need "${provider:[path:]key}", or can it just be
> > >> > > > > ${provider:key}?  It seems like the path can be rolled up
> into the
> > >> > > > > key.  So> > if you want to put your connect keys under
> > >> > > my.connect.path, you
> > >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > >> > > > >
> > >> > > > > >    // A delayMs of 0 indicates an immediate change; a
> positive
> > >> > > > > >    delayMs> > indicates
> > >> > > > > >    // that a future change is anticipated (such as a lease
> > >> > > > > >    duration)> > >    void onChange(String path, Map<String,
> > >> String>
> > >> > > values, int
> > >> > > > > >    delayMs);> >
> > >> > > > > Do we really need delayMs?  It seems like if you get a
> callback
> > >> with>
> > >> > > > delayMs set, you don't know what the new values will be, only
> > >> > > > > that an> > update is coming, but not yet here.
> > >> > > > >
> > >> > > > > best,
> > >> > > > > Colin
> > >> > > > >
> > >> > > > >
> > >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > >> > > > > > Hello everyone,
> > >> > > > > >
> > >> > > > > > After a good round of discussions with excellent feedback
> and no
> > >> > > > > > major> > > objections, I would like to start a vote on
> KIP-297
> > >> to
> > >> > > externalize> > secrets
> > >> > > > > > from Kafka Connect configurations.  My thanks in advance!
> > >> > > > > >
> > >> > > > > > KIP: <
> > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > >> > > > > > >
> > >> > > > > >
> > >> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > >> > > > > >
> > >> > > > > > Discussion thread: <
> > >> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.
> html
> > >> >
> > >> > > > > >
> > >> > > > > > Best,
> > >> > > > > > Robert
> > >> > > > >
> > >> > >
> > >> > >
> > >>
> > >
> > >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Colin McCabe <cm...@apache.org>.

Thanks, Robert!

+1 (non-binding)

Colin


On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> Hi Colin,
> 
> I've changed the KIP to have a composite object returned from get().  It's
> probably the most straightforward option.  Please let me know if you have
> any other concerns.
> 
> Thanks,
> Robert
> 
> On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com> wrote:
> 
> >
> >
> > Hi Colin,
> >
> > My last response was not that clear, so let me back up and explain a bit
> > more.
> >
> > Some secret managers, such as Vault (and maybe Keywhiz) have the notion of
> > a lease duration or a TTL for a path.  Every path can have a different
> > TTL.  This is period after which the value of the keys at the given path
> > may be invalid.  It can be used to indicate a rotation will be done.  In
> > the cause of the Vault integration with AWS, Vault will actually delete the
> > secrets from AWS at the moment the TTL expires.  A TTL could be used by
> > other ConfigProviders, such as a FileConfigProvider, to indicate that all
> > the secrets at a given path (file), will be rotated on a regular basis.
> >
> > I would like to expose the TTL in the APIs somewhere.  The TTL can be made
> > available at the time get() is called.  Connect already has a built in
> > ScheduledExecutor, so Connect can just use the TTL to schedule a Connector
> > restart.  Originally, I had exposed the TTL in a ConfigContext interface
> > passed to the get() method.  To reduce the number of APIs, I placed it on
> > the onChange() method.  This means at the time of get(), onChange() would
> > be called with a TTL.  The Connector's implementation of the callback would
> > use onChange() with the TTL to schedule a restart.
> >
> > If you think this is overloading onChange() too much, I could add the
> > ConfigContext back to get():
> >
> >
> > Map<String, String> get(ConfigContext ctx, String path);
> >
> > public interface ConfigContext {
> >
> >     void willExpire(String path, long ttl);
> >
> > }
> >
> >
> >
> > or I could separate out the TTL method in the callback:
> >
> >
> > public interface ConfigChangeCallback {
> >
> >     void willExpire(String path, long ttl);
> >
> >     void onChange(String path, Map<String, String> values);
> > }
> >
> >
> >
> > Or we could return a composite object from get():
> >
> > ConfigData get(String path);
> >
> > public class ConfigData {
> >
> >   Map<String, String> data;
> >   long ttl;
> >
> > }
> >
> >
> > Do you have a preference Colin?
> >
> > Thanks,
> > Robert
> >
> >
> > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org> wrote:
> >
> >> Hi Robert,
> >>
> >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> >> relying on the ConfigProvider to make a callback to you when the
> >> configuration has changed.  So isn't that always the "push model" (where
> >> the ConfigProvider pushes changes to Connect).  If you want the "pull
> >> model" where you initiate updates, you can simply call ConfigProvider#get
> >> directly, right?
> >>
> >> The actual implementation of ConfigProvider subclasses will depend on the
> >> type of configuration storage mechanism on the backend.  In the case of
> >> Vault, it sounds like we need to have something like a ScheduledExecutor
> >> which re-fetches keys after a certain amount of time.
> >>
> >> As an aside, what does a "lease duration" mean for a configuration key?
> >> Does that mean Vault will reject changes to the configuration key if I try
> >> to make them within the lease duration?  Or is this like a period after
> >> which a password is automatically rotated?
> >>
> >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> >> > Hi Colin,
> >> >
> >> > > With regard to delayMs, can’t we just restart the
> >> > > Connector when the keys are actually changed?
> >> >
> >> > Currently the VaultConfigProvider does not find out when values for keys
> >> > have changed.  You could do this with a poll model (with a
> >> > background thread in the ConfigProvider), but since for each key-value
> >> > pair, Vault provides a lease duration stating exactly when a value for a
> >> > key will change in the future, this is an alternative model of just
> >> passing
> >> > the lease duration to the client (in this case the Connector), to allow
> >> it
> >> > to determine what to do (such as schedule a restart).   This may allow
> >> one
> >> > to avoid the complexity of figuring out a proper poll interval (with
> >> lease
> >> > durations of varying periods), or worrying about putting too much load
> >> on
> >> > the secrets manager by polling too often.
> >>
> >> Those things are still concerns if the Connector is polling, right?
> >> Perhaps the connector poll too often and puts too much load on Vault.  And
> >> so forth.  It seems like this problem needs to be solved either way (and
> >> probably can be solved with reasonable default minimum fetch intervals).
> >>
> >> best,
> >> Colin
> >>
> >>
> >> >  In other words, by adding this
> >> > one additional parameter, a ConfigProvider can provide both push and
> >> pull
> >> > models to clients, perhaps with an additional configuration parameter to
> >> > the ConfigProvider to determine which model (push or poll) to use.
> >> >
> >> > Thanks,
> >> > Robert
> >> >
> >> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org>
> >> wrote:
> >> >
> >> > > Thanks, Robert.  With regard to delayMs, can’t we just restart the
> >> > > Connector when the keys are actually changed?  Or is the concern that
> >> > > this would lengthen the effective key rotation time?  Can’t the user
> >> > > just configure a slightly shorter key rotation time to counteract
> >> > > this concern?
> >> > > Regards,
> >> > > Colin
> >> > >
> >> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> >> > > > Hi Colin,
> >> > > >
> >> > > > Good questions.
> >> > > >
> >> > > >
> >> > > > > As a clarification about the indirections, what if I have the
> >> > > > > connect> configuration key foo set up as ${vault:bar}, and in
> >> Vault,
> >> > > > have the bar> key set to ${file:baz}?
> >> > > > > Does connect get foo as the contents of the baz file?  I would
> >> > > > > argue that> it should not (and in general, we shouldn't allow
> >> > > ConfigProviders to
> >> > > > indirect to other
> >> > > > > ConfigProviders) but I don't think it's spelled out right now.
> >> > > >
> >> > > > I've added a clarification to the KIP that further indirections are
> >> > > > not> performed even if the values returned from ConfigProviders
> >> have the
> >> > > > variable syntax.
> >> > > >
> >> > > >
> >> > > > > What's the behavior when a config key is not found in Vault
> >> > > > > (or other> ConfigProvider)?  Does the variable get replaced with
> >> the
> >> > > empty
> >> > > > string, or> with the literal ${vault:whatever} string?
> >> > > >
> >> > > > It would remain unresolved and still be of the form
> >> > > > ${provider:key}.  I've> added a clarification to the KIP.
> >> > > >
> >> > > >
> >> > > > > Do we really need "${provider:[path:]key}", or can it just be
> >> > > > ${provider:key}?
> >> > > >
> >> > > > The path is a separate parameter in the APIs, so I think it's
> >> > > > important to> explicitly delineate it in the variable syntax.  For
> >> > > example, I
> >> > > > currently> have a working VaultConfigProvider prototype and the
> >> syntax
> >> > > for a
> >> > > > Vault key> reference looks like
> >> > > >
> >> > > > db_password=${vault:secret/staging:mysql_password}
> >> > > >
> >> > > > I think it's important to standardize how to separate the path
> >> > > > from the key> rather than leave it to each ConfigProvider to
> >> determine a
> >> > > possibly
> >> > > > different way.  This will also make it easier to move secrets from
> >> one>
> >> > > ConfigProvider to another should one choose to do so.
> >> > > >
> >> > > >
> >> > > > > Do we really need delayMs?
> >> > > >
> >> > > > One of the goals of this KIP is to allow for secrets rotation
> >> without>
> >> > > having to modify existing connectors.  In the case of the
> >> > > > VaultConfigProvider, it knows the lease durations and will be able
> >> to>
> >> > > schedule a restart of the Connector using an API in the Herder.  The
> >> > > > delayMs will simply be passed to the Herder.restartConnector(long
> >> > > > delayMs,> String connName, Callback cb) method here:
> >> > > >
> >> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> >> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> >> > > connect/runtime/Herder.java#L170>
> >> > > >
> >> > > > Best,
> >> > > > Robert
> >> > > >
> >> > > >
> >> > > >
> >> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> >> > > > <cm...@apache.org> wrote:>
> >> > > > > Thanks, Robert.  Looks good overall.
> >> > > > >
> >> > > > > As a clarification about the indirections, what if I have the
> >> > > > > connect> > configuration key foo set up as ${vault:bar}, and in
> >> Vault,
> >> > > have
> >> > > > > the bar> > key set to ${file:baz}?  Does connect get foo as the
> >> > > contents of
> >> > > > > the baz> > file?  I would argue that it should not (and in
> >> general, we
> >> > > > > shouldn't allow> > ConfigProviders to indirect to other
> >> > > ConfigProviders) but I
> >> > > > > don't think> > it's spelled out right now.
> >> > > > >
> >> > > > > What's the behavior when a config key is not found in Vault
> >> > > > > (or other> > ConfigProvider)?  Does the variable get replaced
> >> with the
> >> > > empty
> >> > > > > string, or> > with the literal ${vault:whatever} string?
> >> > > > >
> >> > > > > Do we really need "${provider:[path:]key}", or can it just be
> >> > > > > ${provider:key}?  It seems like the path can be rolled up into the
> >> > > > > key.  So> > if you want to put your connect keys under
> >> > > my.connect.path, you
> >> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> >> > > > >
> >> > > > > >    // A delayMs of 0 indicates an immediate change; a positive
> >> > > > > >    delayMs> > indicates
> >> > > > > >    // that a future change is anticipated (such as a lease
> >> > > > > >    duration)> > >    void onChange(String path, Map<String,
> >> String>
> >> > > values, int
> >> > > > > >    delayMs);> >
> >> > > > > Do we really need delayMs?  It seems like if you get a callback
> >> with>
> >> > > > delayMs set, you don't know what the new values will be, only
> >> > > > > that an> > update is coming, but not yet here.
> >> > > > >
> >> > > > > best,
> >> > > > > Colin
> >> > > > >
> >> > > > >
> >> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> >> > > > > > Hello everyone,
> >> > > > > >
> >> > > > > > After a good round of discussions with excellent feedback and no
> >> > > > > > major> > > objections, I would like to start a vote on KIP-297
> >> to
> >> > > externalize> > secrets
> >> > > > > > from Kafka Connect configurations.  My thanks in advance!
> >> > > > > >
> >> > > > > > KIP: <
> >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> >> > > > > > >
> >> > > > > >
> >> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> >> > > > > >
> >> > > > > > Discussion thread: <
> >> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html
> >> >
> >> > > > > >
> >> > > > > > Best,
> >> > > > > > Robert
> >> > > > >
> >> > >
> >> > >
> >>
> >
> >

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Robert Yokota <ra...@gmail.com>.

Hi Colin,

I've changed the KIP to have a composite object returned from get().  It's
probably the most straightforward option.  Please let me know if you have
any other concerns.

Thanks,
Robert

On Thu, May 17, 2018 at 11:44 AM, Robert Yokota <ra...@gmail.com> wrote:

>
>
> Hi Colin,
>
> My last response was not that clear, so let me back up and explain a bit
> more.
>
> Some secret managers, such as Vault (and maybe Keywhiz) have the notion of
> a lease duration or a TTL for a path.  Every path can have a different
> TTL.  This is period after which the value of the keys at the given path
> may be invalid.  It can be used to indicate a rotation will be done.  In
> the cause of the Vault integration with AWS, Vault will actually delete the
> secrets from AWS at the moment the TTL expires.  A TTL could be used by
> other ConfigProviders, such as a FileConfigProvider, to indicate that all
> the secrets at a given path (file), will be rotated on a regular basis.
>
> I would like to expose the TTL in the APIs somewhere.  The TTL can be made
> available at the time get() is called.  Connect already has a built in
> ScheduledExecutor, so Connect can just use the TTL to schedule a Connector
> restart.  Originally, I had exposed the TTL in a ConfigContext interface
> passed to the get() method.  To reduce the number of APIs, I placed it on
> the onChange() method.  This means at the time of get(), onChange() would
> be called with a TTL.  The Connector's implementation of the callback would
> use onChange() with the TTL to schedule a restart.
>
> If you think this is overloading onChange() too much, I could add the
> ConfigContext back to get():
>
>
> Map<String, String> get(ConfigContext ctx, String path);
>
> public interface ConfigContext {
>
>     void willExpire(String path, long ttl);
>
> }
>
>
>
> or I could separate out the TTL method in the callback:
>
>
> public interface ConfigChangeCallback {
>
>     void willExpire(String path, long ttl);
>
>     void onChange(String path, Map<String, String> values);
> }
>
>
>
> Or we could return a composite object from get():
>
> ConfigData get(String path);
>
> public class ConfigData {
>
>   Map<String, String> data;
>   long ttl;
>
> }
>
>
> Do you have a preference Colin?
>
> Thanks,
> Robert
>
>
> On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org> wrote:
>
>> Hi Robert,
>>
>> Hmm.  I thought that if you're using ConfigChangeCallback, you are
>> relying on the ConfigProvider to make a callback to you when the
>> configuration has changed.  So isn't that always the "push model" (where
>> the ConfigProvider pushes changes to Connect).  If you want the "pull
>> model" where you initiate updates, you can simply call ConfigProvider#get
>> directly, right?
>>
>> The actual implementation of ConfigProvider subclasses will depend on the
>> type of configuration storage mechanism on the backend.  In the case of
>> Vault, it sounds like we need to have something like a ScheduledExecutor
>> which re-fetches keys after a certain amount of time.
>>
>> As an aside, what does a "lease duration" mean for a configuration key?
>> Does that mean Vault will reject changes to the configuration key if I try
>> to make them within the lease duration?  Or is this like a period after
>> which a password is automatically rotated?
>>
>> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
>> > Hi Colin,
>> >
>> > > With regard to delayMs, can’t we just restart the
>> > > Connector when the keys are actually changed?
>> >
>> > Currently the VaultConfigProvider does not find out when values for keys
>> > have changed.  You could do this with a poll model (with a
>> > background thread in the ConfigProvider), but since for each key-value
>> > pair, Vault provides a lease duration stating exactly when a value for a
>> > key will change in the future, this is an alternative model of just
>> passing
>> > the lease duration to the client (in this case the Connector), to allow
>> it
>> > to determine what to do (such as schedule a restart).   This may allow
>> one
>> > to avoid the complexity of figuring out a proper poll interval (with
>> lease
>> > durations of varying periods), or worrying about putting too much load
>> on
>> > the secrets manager by polling too often.
>>
>> Those things are still concerns if the Connector is polling, right?
>> Perhaps the connector poll too often and puts too much load on Vault.  And
>> so forth.  It seems like this problem needs to be solved either way (and
>> probably can be solved with reasonable default minimum fetch intervals).
>>
>> best,
>> Colin
>>
>>
>> >  In other words, by adding this
>> > one additional parameter, a ConfigProvider can provide both push and
>> pull
>> > models to clients, perhaps with an additional configuration parameter to
>> > the ConfigProvider to determine which model (push or poll) to use.
>> >
>> > Thanks,
>> > Robert
>> >
>> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org>
>> wrote:
>> >
>> > > Thanks, Robert.  With regard to delayMs, can’t we just restart the
>> > > Connector when the keys are actually changed?  Or is the concern that
>> > > this would lengthen the effective key rotation time?  Can’t the user
>> > > just configure a slightly shorter key rotation time to counteract
>> > > this concern?
>> > > Regards,
>> > > Colin
>> > >
>> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
>> > > > Hi Colin,
>> > > >
>> > > > Good questions.
>> > > >
>> > > >
>> > > > > As a clarification about the indirections, what if I have the
>> > > > > connect> configuration key foo set up as ${vault:bar}, and in
>> Vault,
>> > > > have the bar> key set to ${file:baz}?
>> > > > > Does connect get foo as the contents of the baz file?  I would
>> > > > > argue that> it should not (and in general, we shouldn't allow
>> > > ConfigProviders to
>> > > > indirect to other
>> > > > > ConfigProviders) but I don't think it's spelled out right now.
>> > > >
>> > > > I've added a clarification to the KIP that further indirections are
>> > > > not> performed even if the values returned from ConfigProviders
>> have the
>> > > > variable syntax.
>> > > >
>> > > >
>> > > > > What's the behavior when a config key is not found in Vault
>> > > > > (or other> ConfigProvider)?  Does the variable get replaced with
>> the
>> > > empty
>> > > > string, or> with the literal ${vault:whatever} string?
>> > > >
>> > > > It would remain unresolved and still be of the form
>> > > > ${provider:key}.  I've> added a clarification to the KIP.
>> > > >
>> > > >
>> > > > > Do we really need "${provider:[path:]key}", or can it just be
>> > > > ${provider:key}?
>> > > >
>> > > > The path is a separate parameter in the APIs, so I think it's
>> > > > important to> explicitly delineate it in the variable syntax.  For
>> > > example, I
>> > > > currently> have a working VaultConfigProvider prototype and the
>> syntax
>> > > for a
>> > > > Vault key> reference looks like
>> > > >
>> > > > db_password=${vault:secret/staging:mysql_password}
>> > > >
>> > > > I think it's important to standardize how to separate the path
>> > > > from the key> rather than leave it to each ConfigProvider to
>> determine a
>> > > possibly
>> > > > different way.  This will also make it easier to move secrets from
>> one>
>> > > ConfigProvider to another should one choose to do so.
>> > > >
>> > > >
>> > > > > Do we really need delayMs?
>> > > >
>> > > > One of the goals of this KIP is to allow for secrets rotation
>> without>
>> > > having to modify existing connectors.  In the case of the
>> > > > VaultConfigProvider, it knows the lease durations and will be able
>> to>
>> > > schedule a restart of the Connector using an API in the Herder.  The
>> > > > delayMs will simply be passed to the Herder.restartConnector(long
>> > > > delayMs,> String connName, Callback cb) method here:
>> > > >
>> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
>> > > configs/connect/runtime/src/main/java/org/apache/kafka/
>> > > connect/runtime/Herder.java#L170>
>> > > >
>> > > > Best,
>> > > > Robert
>> > > >
>> > > >
>> > > >
>> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
>> > > > <cm...@apache.org> wrote:>
>> > > > > Thanks, Robert.  Looks good overall.
>> > > > >
>> > > > > As a clarification about the indirections, what if I have the
>> > > > > connect> > configuration key foo set up as ${vault:bar}, and in
>> Vault,
>> > > have
>> > > > > the bar> > key set to ${file:baz}?  Does connect get foo as the
>> > > contents of
>> > > > > the baz> > file?  I would argue that it should not (and in
>> general, we
>> > > > > shouldn't allow> > ConfigProviders to indirect to other
>> > > ConfigProviders) but I
>> > > > > don't think> > it's spelled out right now.
>> > > > >
>> > > > > What's the behavior when a config key is not found in Vault
>> > > > > (or other> > ConfigProvider)?  Does the variable get replaced
>> with the
>> > > empty
>> > > > > string, or> > with the literal ${vault:whatever} string?
>> > > > >
>> > > > > Do we really need "${provider:[path:]key}", or can it just be
>> > > > > ${provider:key}?  It seems like the path can be rolled up into the
>> > > > > key.  So> > if you want to put your connect keys under
>> > > my.connect.path, you
>> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
>> > > > >
>> > > > > >    // A delayMs of 0 indicates an immediate change; a positive
>> > > > > >    delayMs> > indicates
>> > > > > >    // that a future change is anticipated (such as a lease
>> > > > > >    duration)> > >    void onChange(String path, Map<String,
>> String>
>> > > values, int
>> > > > > >    delayMs);> >
>> > > > > Do we really need delayMs?  It seems like if you get a callback
>> with>
>> > > > delayMs set, you don't know what the new values will be, only
>> > > > > that an> > update is coming, but not yet here.
>> > > > >
>> > > > > best,
>> > > > > Colin
>> > > > >
>> > > > >
>> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
>> > > > > > Hello everyone,
>> > > > > >
>> > > > > > After a good round of discussions with excellent feedback and no
>> > > > > > major> > > objections, I would like to start a vote on KIP-297
>> to
>> > > externalize> > secrets
>> > > > > > from Kafka Connect configurations.  My thanks in advance!
>> > > > > >
>> > > > > > KIP: <
>> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
>> > > > > > >
>> > > > > >
>> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
>> > > > > >
>> > > > > > Discussion thread: <
>> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html
>> >
>> > > > > >
>> > > > > > Best,
>> > > > > > Robert
>> > > > >
>> > >
>> > >
>>
>
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Robert Yokota <ra...@gmail.com>.

Hi Colin,

My last response was not that clear, so let me back up and explain a bit
more.

Some secret managers, such as Vault (and maybe Keywhiz) have the notion of
a lease duration or a TTL for a path.  Every path can have a different
TTL.  This is period after which the value of the keys at the given path
may be invalid.  It can be used to indicate a rotation will be done.  In
the cause of the Vault integration with AWS, Vault will actually delete the
secrets from AWS at the moment the TTL expires.  A TTL could be used by
other ConfigProviders, such as a FileConfigProvider, to indicate that all
the secrets at a given path (file), will be rotated on a regular basis.

I would like to expose the TTL in the APIs somewhere.  The TTL can be made
available at the time get() is called.  Connect already has a built in
ScheduledExecutor, so Connect can just use the TTL to schedule a Connector
restart.  Originally, I had exposed the TTL in a ConfigContext interface
passed to the get() method.  To reduce the number of APIs, I placed it on
the onChange() method.  This means at the time of get(), onChange() would
be called with a TTL.  The Connector's implementation of the callback would
use onChange() with the TTL to schedule a restart.

If you think this is overloading onChange() too much, I could add the
ConfigContext back to get():


Map<String, String> get(ConfigContext ctx, String path);

public interface ConfigContext {

    void willExpire(String path, long ttl);

}



or I could separate out the TTL method in the callback:


public interface ConfigChangeCallback {

    void willExpire(String path, long ttl);

    void onChange(String path, Map<String, String> values);
}



Or we could return a composite object from get():

ConfigData get(String path);

public class ConfigData {

  Map<String, String> data;
  long ttl;

}


Do you have a preference Colin?

Thanks,
Robert


On Thu, May 17, 2018 at 9:27 AM, Colin McCabe <cm...@apache.org> wrote:

> Hi Robert,
>
> Hmm.  I thought that if you're using ConfigChangeCallback, you are relying
> on the ConfigProvider to make a callback to you when the configuration has
> changed.  So isn't that always the "push model" (where the ConfigProvider
> pushes changes to Connect).  If you want the "pull model" where you
> initiate updates, you can simply call ConfigProvider#get directly, right?
>
> The actual implementation of ConfigProvider subclasses will depend on the
> type of configuration storage mechanism on the backend.  In the case of
> Vault, it sounds like we need to have something like a ScheduledExecutor
> which re-fetches keys after a certain amount of time.
>
> As an aside, what does a "lease duration" mean for a configuration key?
> Does that mean Vault will reject changes to the configuration key if I try
> to make them within the lease duration?  Or is this like a period after
> which a password is automatically rotated?
>
> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > Hi Colin,
> >
> > > With regard to delayMs, can’t we just restart the
> > > Connector when the keys are actually changed?
> >
> > Currently the VaultConfigProvider does not find out when values for keys
> > have changed.  You could do this with a poll model (with a
> > background thread in the ConfigProvider), but since for each key-value
> > pair, Vault provides a lease duration stating exactly when a value for a
> > key will change in the future, this is an alternative model of just
> passing
> > the lease duration to the client (in this case the Connector), to allow
> it
> > to determine what to do (such as schedule a restart).   This may allow
> one
> > to avoid the complexity of figuring out a proper poll interval (with
> lease
> > durations of varying periods), or worrying about putting too much load on
> > the secrets manager by polling too often.
>
> Those things are still concerns if the Connector is polling, right?
> Perhaps the connector poll too often and puts too much load on Vault.  And
> so forth.  It seems like this problem needs to be solved either way (and
> probably can be solved with reasonable default minimum fetch intervals).
>
> best,
> Colin
>
>
> >  In other words, by adding this
> > one additional parameter, a ConfigProvider can provide both push and pull
> > models to clients, perhaps with an additional configuration parameter to
> > the ConfigProvider to determine which model (push or poll) to use.
> >
> > Thanks,
> > Robert
> >
> > On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org>
> wrote:
> >
> > > Thanks, Robert.  With regard to delayMs, can’t we just restart the
> > > Connector when the keys are actually changed?  Or is the concern that
> > > this would lengthen the effective key rotation time?  Can’t the user
> > > just configure a slightly shorter key rotation time to counteract
> > > this concern?
> > > Regards,
> > > Colin
> > >
> > > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > > Hi Colin,
> > > >
> > > > Good questions.
> > > >
> > > >
> > > > > As a clarification about the indirections, what if I have the
> > > > > connect> configuration key foo set up as ${vault:bar}, and in
> Vault,
> > > > have the bar> key set to ${file:baz}?
> > > > > Does connect get foo as the contents of the baz file?  I would
> > > > > argue that> it should not (and in general, we shouldn't allow
> > > ConfigProviders to
> > > > indirect to other
> > > > > ConfigProviders) but I don't think it's spelled out right now.
> > > >
> > > > I've added a clarification to the KIP that further indirections are
> > > > not> performed even if the values returned from ConfigProviders have
> the
> > > > variable syntax.
> > > >
> > > >
> > > > > What's the behavior when a config key is not found in Vault
> > > > > (or other> ConfigProvider)?  Does the variable get replaced with
> the
> > > empty
> > > > string, or> with the literal ${vault:whatever} string?
> > > >
> > > > It would remain unresolved and still be of the form
> > > > ${provider:key}.  I've> added a clarification to the KIP.
> > > >
> > > >
> > > > > Do we really need "${provider:[path:]key}", or can it just be
> > > > ${provider:key}?
> > > >
> > > > The path is a separate parameter in the APIs, so I think it's
> > > > important to> explicitly delineate it in the variable syntax.  For
> > > example, I
> > > > currently> have a working VaultConfigProvider prototype and the
> syntax
> > > for a
> > > > Vault key> reference looks like
> > > >
> > > > db_password=${vault:secret/staging:mysql_password}
> > > >
> > > > I think it's important to standardize how to separate the path
> > > > from the key> rather than leave it to each ConfigProvider to
> determine a
> > > possibly
> > > > different way.  This will also make it easier to move secrets from
> one>
> > > ConfigProvider to another should one choose to do so.
> > > >
> > > >
> > > > > Do we really need delayMs?
> > > >
> > > > One of the goals of this KIP is to allow for secrets rotation
> without>
> > > having to modify existing connectors.  In the case of the
> > > > VaultConfigProvider, it knows the lease durations and will be able
> to>
> > > schedule a restart of the Connector using an API in the Herder.  The
> > > > delayMs will simply be passed to the Herder.restartConnector(long
> > > > delayMs,> String connName, Callback cb) method here:
> > > >
> > > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > > configs/connect/runtime/src/main/java/org/apache/kafka/
> > > connect/runtime/Herder.java#L170>
> > > >
> > > > Best,
> > > > Robert
> > > >
> > > >
> > > >
> > > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > > <cm...@apache.org> wrote:>
> > > > > Thanks, Robert.  Looks good overall.
> > > > >
> > > > > As a clarification about the indirections, what if I have the
> > > > > connect> > configuration key foo set up as ${vault:bar}, and in
> Vault,
> > > have
> > > > > the bar> > key set to ${file:baz}?  Does connect get foo as the
> > > contents of
> > > > > the baz> > file?  I would argue that it should not (and in
> general, we
> > > > > shouldn't allow> > ConfigProviders to indirect to other
> > > ConfigProviders) but I
> > > > > don't think> > it's spelled out right now.
> > > > >
> > > > > What's the behavior when a config key is not found in Vault
> > > > > (or other> > ConfigProvider)?  Does the variable get replaced with
> the
> > > empty
> > > > > string, or> > with the literal ${vault:whatever} string?
> > > > >
> > > > > Do we really need "${provider:[path:]key}", or can it just be
> > > > > ${provider:key}?  It seems like the path can be rolled up into the
> > > > > key.  So> > if you want to put your connect keys under
> > > my.connect.path, you
> > > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > > >
> > > > > >    // A delayMs of 0 indicates an immediate change; a positive
> > > > > >    delayMs> > indicates
> > > > > >    // that a future change is anticipated (such as a lease
> > > > > >    duration)> > >    void onChange(String path, Map<String,
> String>
> > > values, int
> > > > > >    delayMs);> >
> > > > > Do we really need delayMs?  It seems like if you get a callback
> with>
> > > > delayMs set, you don't know what the new values will be, only
> > > > > that an> > update is coming, but not yet here.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > > > Hello everyone,
> > > > > >
> > > > > > After a good round of discussions with excellent feedback and no
> > > > > > major> > > objections, I would like to start a vote on KIP-297 to
> > > externalize> > secrets
> > > > > > from Kafka Connect configurations.  My thanks in advance!
> > > > > >
> > > > > > KIP: <
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > > > > >
> > > > > >
> > > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > > > > >
> > > > > > Discussion thread: <
> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>
> > > > > >
> > > > > > Best,
> > > > > > Robert
> > > > >
> > >
> > >
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Colin McCabe <cm...@apache.org>.

Hi Robert,

Hmm.  I thought that if you're using ConfigChangeCallback, you are relying on the ConfigProvider to make a callback to you when the configuration has changed.  So isn't that always the "push model" (where the ConfigProvider pushes changes to Connect).  If you want the "pull model" where you initiate updates, you can simply call ConfigProvider#get directly, right?

The actual implementation of ConfigProvider subclasses will depend on the type of configuration storage mechanism on the backend.  In the case of Vault, it sounds like we need to have something like a ScheduledExecutor which re-fetches keys after a certain amount of time.

As an aside, what does a "lease duration" mean for a configuration key?  Does that mean Vault will reject changes to the configuration key if I try to make them within the lease duration?  Or is this like a period after which a password is automatically rotated?

On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> Hi Colin,
> 
> > With regard to delayMs, can’t we just restart the
> > Connector when the keys are actually changed?
> 
> Currently the VaultConfigProvider does not find out when values for keys
> have changed.  You could do this with a poll model (with a
> background thread in the ConfigProvider), but since for each key-value
> pair, Vault provides a lease duration stating exactly when a value for a
> key will change in the future, this is an alternative model of just passing
> the lease duration to the client (in this case the Connector), to allow it
> to determine what to do (such as schedule a restart).   This may allow one
> to avoid the complexity of figuring out a proper poll interval (with lease
> durations of varying periods), or worrying about putting too much load on
> the secrets manager by polling too often.

Those things are still concerns if the Connector is polling, right?  Perhaps the connector poll too often and puts too much load on Vault.  And so forth.  It seems like this problem needs to be solved either way (and probably can be solved with reasonable default minimum fetch intervals).

best,
Colin


>  In other words, by adding this
> one additional parameter, a ConfigProvider can provide both push and pull
> models to clients, perhaps with an additional configuration parameter to
> the ConfigProvider to determine which model (push or poll) to use.
> 
> Thanks,
> Robert
> 
> On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org> wrote:
> 
> > Thanks, Robert.  With regard to delayMs, can’t we just restart the
> > Connector when the keys are actually changed?  Or is the concern that
> > this would lengthen the effective key rotation time?  Can’t the user
> > just configure a slightly shorter key rotation time to counteract
> > this concern?
> > Regards,
> > Colin
> >
> > On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > > Hi Colin,
> > >
> > > Good questions.
> > >
> > >
> > > > As a clarification about the indirections, what if I have the
> > > > connect> configuration key foo set up as ${vault:bar}, and in Vault,
> > > have the bar> key set to ${file:baz}?
> > > > Does connect get foo as the contents of the baz file?  I would
> > > > argue that> it should not (and in general, we shouldn't allow
> > ConfigProviders to
> > > indirect to other
> > > > ConfigProviders) but I don't think it's spelled out right now.
> > >
> > > I've added a clarification to the KIP that further indirections are
> > > not> performed even if the values returned from ConfigProviders have the
> > > variable syntax.
> > >
> > >
> > > > What's the behavior when a config key is not found in Vault
> > > > (or other> ConfigProvider)?  Does the variable get replaced with the
> > empty
> > > string, or> with the literal ${vault:whatever} string?
> > >
> > > It would remain unresolved and still be of the form
> > > ${provider:key}.  I've> added a clarification to the KIP.
> > >
> > >
> > > > Do we really need "${provider:[path:]key}", or can it just be
> > > ${provider:key}?
> > >
> > > The path is a separate parameter in the APIs, so I think it's
> > > important to> explicitly delineate it in the variable syntax.  For
> > example, I
> > > currently> have a working VaultConfigProvider prototype and the syntax
> > for a
> > > Vault key> reference looks like
> > >
> > > db_password=${vault:secret/staging:mysql_password}
> > >
> > > I think it's important to standardize how to separate the path
> > > from the key> rather than leave it to each ConfigProvider to determine a
> > possibly
> > > different way.  This will also make it easier to move secrets from one>
> > ConfigProvider to another should one choose to do so.
> > >
> > >
> > > > Do we really need delayMs?
> > >
> > > One of the goals of this KIP is to allow for secrets rotation without>
> > having to modify existing connectors.  In the case of the
> > > VaultConfigProvider, it knows the lease durations and will be able to>
> > schedule a restart of the Connector using an API in the Herder.  The
> > > delayMs will simply be passed to the Herder.restartConnector(long
> > > delayMs,> String connName, Callback cb) method here:
> > >
> > > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> > configs/connect/runtime/src/main/java/org/apache/kafka/
> > connect/runtime/Herder.java#L170>
> > >
> > > Best,
> > > Robert
> > >
> > >
> > >
> > > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > > <cm...@apache.org> wrote:>
> > > > Thanks, Robert.  Looks good overall.
> > > >
> > > > As a clarification about the indirections, what if I have the
> > > > connect> > configuration key foo set up as ${vault:bar}, and in Vault,
> > have
> > > > the bar> > key set to ${file:baz}?  Does connect get foo as the
> > contents of
> > > > the baz> > file?  I would argue that it should not (and in general, we
> > > > shouldn't allow> > ConfigProviders to indirect to other
> > ConfigProviders) but I
> > > > don't think> > it's spelled out right now.
> > > >
> > > > What's the behavior when a config key is not found in Vault
> > > > (or other> > ConfigProvider)?  Does the variable get replaced with the
> > empty
> > > > string, or> > with the literal ${vault:whatever} string?
> > > >
> > > > Do we really need "${provider:[path:]key}", or can it just be
> > > > ${provider:key}?  It seems like the path can be rolled up into the
> > > > key.  So> > if you want to put your connect keys under
> > my.connect.path, you
> > > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > > >
> > > > >    // A delayMs of 0 indicates an immediate change; a positive
> > > > >    delayMs> > indicates
> > > > >    // that a future change is anticipated (such as a lease
> > > > >    duration)> > >    void onChange(String path, Map<String, String>
> > values, int
> > > > >    delayMs);> >
> > > > Do we really need delayMs?  It seems like if you get a callback with>
> > > delayMs set, you don't know what the new values will be, only
> > > > that an> > update is coming, but not yet here.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > > Hello everyone,
> > > > >
> > > > > After a good round of discussions with excellent feedback and no
> > > > > major> > > objections, I would like to start a vote on KIP-297 to
> > externalize> > secrets
> > > > > from Kafka Connect configurations.  My thanks in advance!
> > > > >
> > > > > KIP: <
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > > > >
> > > > >
> > > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > > > >
> > > > > Discussion thread: <
> > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>
> > > > >
> > > > > Best,
> > > > > Robert
> > > >
> >
> >

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Robert Yokota <ra...@gmail.com>.

Hi Colin,

> With regard to delayMs, can’t we just restart the
> Connector when the keys are actually changed?

Currently the VaultConfigProvider does not find out when values for keys
have changed.  You could do this with a poll model (with a
background thread in the ConfigProvider), but since for each key-value
pair, Vault provides a lease duration stating exactly when a value for a
key will change in the future, this is an alternative model of just passing
the lease duration to the client (in this case the Connector), to allow it
to determine what to do (such as schedule a restart).   This may allow one
to avoid the complexity of figuring out a proper poll interval (with lease
durations of varying periods), or worrying about putting too much load on
the secrets manager by polling too often.  In other words, by adding this
one additional parameter, a ConfigProvider can provide both push and pull
models to clients, perhaps with an additional configuration parameter to
the ConfigProvider to determine which model (push or poll) to use.

Thanks,
Robert

On Wed, May 16, 2018 at 9:56 PM, Colin McCabe <cm...@apache.org> wrote:

> Thanks, Robert.  With regard to delayMs, can’t we just restart the
> Connector when the keys are actually changed?  Or is the concern that
> this would lengthen the effective key rotation time?  Can’t the user
> just configure a slightly shorter key rotation time to counteract
> this concern?
> Regards,
> Colin
>
> On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> > Hi Colin,
> >
> > Good questions.
> >
> >
> > > As a clarification about the indirections, what if I have the
> > > connect> configuration key foo set up as ${vault:bar}, and in Vault,
> > have the bar> key set to ${file:baz}?
> > > Does connect get foo as the contents of the baz file?  I would
> > > argue that> it should not (and in general, we shouldn't allow
> ConfigProviders to
> > indirect to other
> > > ConfigProviders) but I don't think it's spelled out right now.
> >
> > I've added a clarification to the KIP that further indirections are
> > not> performed even if the values returned from ConfigProviders have the
> > variable syntax.
> >
> >
> > > What's the behavior when a config key is not found in Vault
> > > (or other> ConfigProvider)?  Does the variable get replaced with the
> empty
> > string, or> with the literal ${vault:whatever} string?
> >
> > It would remain unresolved and still be of the form
> > ${provider:key}.  I've> added a clarification to the KIP.
> >
> >
> > > Do we really need "${provider:[path:]key}", or can it just be
> > ${provider:key}?
> >
> > The path is a separate parameter in the APIs, so I think it's
> > important to> explicitly delineate it in the variable syntax.  For
> example, I
> > currently> have a working VaultConfigProvider prototype and the syntax
> for a
> > Vault key> reference looks like
> >
> > db_password=${vault:secret/staging:mysql_password}
> >
> > I think it's important to standardize how to separate the path
> > from the key> rather than leave it to each ConfigProvider to determine a
> possibly
> > different way.  This will also make it easier to move secrets from one>
> ConfigProvider to another should one choose to do so.
> >
> >
> > > Do we really need delayMs?
> >
> > One of the goals of this KIP is to allow for secrets rotation without>
> having to modify existing connectors.  In the case of the
> > VaultConfigProvider, it knows the lease durations and will be able to>
> schedule a restart of the Connector using an API in the Herder.  The
> > delayMs will simply be passed to the Herder.restartConnector(long
> > delayMs,> String connName, Callback cb) method here:
> >
> > https://github.com/rayokota/kafka/blob/secrets-in-connect-
> configs/connect/runtime/src/main/java/org/apache/kafka/
> connect/runtime/Herder.java#L170>
> >
> > Best,
> > Robert
> >
> >
> >
> > On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> > <cm...@apache.org> wrote:>
> > > Thanks, Robert.  Looks good overall.
> > >
> > > As a clarification about the indirections, what if I have the
> > > connect> > configuration key foo set up as ${vault:bar}, and in Vault,
> have
> > > the bar> > key set to ${file:baz}?  Does connect get foo as the
> contents of
> > > the baz> > file?  I would argue that it should not (and in general, we
> > > shouldn't allow> > ConfigProviders to indirect to other
> ConfigProviders) but I
> > > don't think> > it's spelled out right now.
> > >
> > > What's the behavior when a config key is not found in Vault
> > > (or other> > ConfigProvider)?  Does the variable get replaced with the
> empty
> > > string, or> > with the literal ${vault:whatever} string?
> > >
> > > Do we really need "${provider:[path:]key}", or can it just be
> > > ${provider:key}?  It seems like the path can be rolled up into the
> > > key.  So> > if you want to put your connect keys under
> my.connect.path, you
> > > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> > >
> > > >    // A delayMs of 0 indicates an immediate change; a positive
> > > >    delayMs> > indicates
> > > >    // that a future change is anticipated (such as a lease
> > > >    duration)> > >    void onChange(String path, Map<String, String>
> values, int
> > > >    delayMs);> >
> > > Do we really need delayMs?  It seems like if you get a callback with>
> > delayMs set, you don't know what the new values will be, only
> > > that an> > update is coming, but not yet here.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > > Hello everyone,
> > > >
> > > > After a good round of discussions with excellent feedback and no
> > > > major> > > objections, I would like to start a vote on KIP-297 to
> externalize> > secrets
> > > > from Kafka Connect configurations.  My thanks in advance!
> > > >
> > > > KIP: <
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > > >
> > > >
> > > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > > >
> > > > Discussion thread: <
> > > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>
> > > >
> > > > Best,
> > > > Robert
> > >
>
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Colin McCabe <cm...@apache.org>.

Thanks, Robert.  With regard to delayMs, can’t we just restart the
Connector when the keys are actually changed?  Or is the concern that
this would lengthen the effective key rotation time?  Can’t the user
just configure a slightly shorter key rotation time to counteract
this concern?
Regards,
Colin

On Wed, May 16, 2018, at 19:13, Robert Yokota wrote:
> Hi Colin,
>
> Good questions.
>
>
> > As a clarification about the indirections, what if I have the
> > connect> configuration key foo set up as ${vault:bar}, and in Vault,
> have the bar> key set to ${file:baz}?
> > Does connect get foo as the contents of the baz file?  I would
> > argue that> it should not (and in general, we shouldn't allow ConfigProviders to
> indirect to other
> > ConfigProviders) but I don't think it's spelled out right now.
>
> I've added a clarification to the KIP that further indirections are
> not> performed even if the values returned from ConfigProviders have the
> variable syntax.
>
>
> > What's the behavior when a config key is not found in Vault
> > (or other> ConfigProvider)?  Does the variable get replaced with the empty
> string, or> with the literal ${vault:whatever} string?
>
> It would remain unresolved and still be of the form
> ${provider:key}.  I've> added a clarification to the KIP.
>
>
> > Do we really need "${provider:[path:]key}", or can it just be
> ${provider:key}?
>
> The path is a separate parameter in the APIs, so I think it's
> important to> explicitly delineate it in the variable syntax.  For example, I
> currently> have a working VaultConfigProvider prototype and the syntax for a
> Vault key> reference looks like
>
> db_password=${vault:secret/staging:mysql_password}
>
> I think it's important to standardize how to separate the path
> from the key> rather than leave it to each ConfigProvider to determine a possibly
> different way.  This will also make it easier to move secrets from one> ConfigProvider to another should one choose to do so.
>
>
> > Do we really need delayMs?
>
> One of the goals of this KIP is to allow for secrets rotation without> having to modify existing connectors.  In the case of the
> VaultConfigProvider, it knows the lease durations and will be able to> schedule a restart of the Connector using an API in the Herder.  The
> delayMs will simply be passed to the Herder.restartConnector(long
> delayMs,> String connName, Callback cb) method here:
>
> https://github.com/rayokota/kafka/blob/secrets-in-connect-configs/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Herder.java#L170>
>
> Best,
> Robert
>
>
>
> On Wed, May 16, 2018 at 6:16 PM, Colin McCabe
> <cm...@apache.org> wrote:>
> > Thanks, Robert.  Looks good overall.
> >
> > As a clarification about the indirections, what if I have the
> > connect> > configuration key foo set up as ${vault:bar}, and in Vault, have
> > the bar> > key set to ${file:baz}?  Does connect get foo as the contents of
> > the baz> > file?  I would argue that it should not (and in general, we
> > shouldn't allow> > ConfigProviders to indirect to other ConfigProviders) but I
> > don't think> > it's spelled out right now.
> >
> > What's the behavior when a config key is not found in Vault
> > (or other> > ConfigProvider)?  Does the variable get replaced with the empty
> > string, or> > with the literal ${vault:whatever} string?
> >
> > Do we really need "${provider:[path:]key}", or can it just be
> > ${provider:key}?  It seems like the path can be rolled up into the
> > key.  So> > if you want to put your connect keys under my.connect.path, you
> > ask for> > ${vault:my.connect.path.jdbc.config}, etc.
> >
> > >    // A delayMs of 0 indicates an immediate change; a positive
> > >    delayMs> > indicates
> > >    // that a future change is anticipated (such as a lease
> > >    duration)> > >    void onChange(String path, Map<String, String> values, int
> > >    delayMs);> >
> > Do we really need delayMs?  It seems like if you get a callback with> > delayMs set, you don't know what the new values will be, only
> > that an> > update is coming, but not yet here.
> >
> > best,
> > Colin
> >
> >
> > On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > > Hello everyone,
> > >
> > > After a good round of discussions with excellent feedback and no
> > > major> > > objections, I would like to start a vote on KIP-297 to externalize> > secrets
> > > from Kafka Connect configurations.  My thanks in advance!
> > >
> > > KIP: <
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > > >
> > >
> > > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> > >
> > > Discussion thread: <
> > > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>
> > >
> > > Best,
> > > Robert
> >

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Robert Yokota <ra...@gmail.com>.

Hi Colin,

Good questions.

> As a clarification about the indirections, what if I have the connect
configuration key foo set up as ${vault:bar}, and in Vault, have the bar
key set to ${file:baz}?
> Does connect get foo as the contents of the baz file?  I would argue that
it should not (and in general, we shouldn't allow ConfigProviders to
indirect to other
> ConfigProviders) but I don't think it's spelled out right now.

I've added a clarification to the KIP that further indirections are not
performed even if the values returned from ConfigProviders have the
variable syntax.

> What's the behavior when a config key is not found in Vault (or other
ConfigProvider)?  Does the variable get replaced with the empty string, or
with the literal ${vault:whatever} string?

It would remain unresolved and still be of the form ${provider:key}.  I've
added a clarification to the KIP.

> Do we really need "${provider:[path:]key}", or can it just be
${provider:key}?

The path is a separate parameter in the APIs, so I think it's important to
explicitly delineate it in the variable syntax.  For example, I currently
have a working VaultConfigProvider prototype and the syntax for a Vault key
reference looks like

db_password=${vault:secret/staging:mysql_password}

I think it's important to standardize how to separate the path from the key
rather than leave it to each ConfigProvider to determine a possibly
different way.  This will also make it easier to move secrets from one
ConfigProvider to another should one choose to do so.

> Do we really need delayMs?

One of the goals of this KIP is to allow for secrets rotation without
having to modify existing connectors.  In the case of the
VaultConfigProvider, it knows the lease durations and will be able to
schedule a restart of the Connector using an API in the Herder.  The
delayMs will simply be passed to the Herder.restartConnector(long delayMs,
String connName, Callback cb) method here:

https://github.com/rayokota/kafka/blob/secrets-in-connect-configs/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Herder.java#L170

Best,
Robert

On Wed, May 16, 2018 at 6:16 PM, Colin McCabe <cm...@apache.org> wrote:

> Thanks, Robert.  Looks good overall.
>
> As a clarification about the indirections, what if I have the connect
> configuration key foo set up as ${vault:bar}, and in Vault, have the bar
> key set to ${file:baz}?  Does connect get foo as the contents of the baz
> file?  I would argue that it should not (and in general, we shouldn't allow
> ConfigProviders to indirect to other ConfigProviders) but I don't think
> it's spelled out right now.
>
> What's the behavior when a config key is not found in Vault (or other
> ConfigProvider)?  Does the variable get replaced with the empty string, or
> with the literal ${vault:whatever} string?
>
> Do we really need "${provider:[path:]key}", or can it just be
> ${provider:key}?  It seems like the path can be rolled up into the key.  So
> if you want to put your connect keys under my.connect.path, you ask for
> ${vault:my.connect.path.jdbc.config}, etc.
>
> >    // A delayMs of 0 indicates an immediate change; a positive delayMs
> indicates
> >    // that a future change is anticipated (such as a lease duration)
> >    void onChange(String path, Map<String, String> values, int delayMs);
>
> Do we really need delayMs?  It seems like if you get a callback with
> delayMs set, you don't know what the new values will be, only that an
> update is coming, but not yet here.
>
> best,
> Colin
>
>
> On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> > Hello everyone,
> >
> > After a good round of discussions with excellent feedback and no major
> > objections, I would like to start a vote on KIP-297 to externalize
> secrets
> > from Kafka Connect configurations.  My thanks in advance!
> >
> > KIP: <
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 297%3A+Externalizing+Secrets+for+Connect+Configurations
> > >
> >
> > JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> >
> > Discussion thread: <
> > https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>
> >
> > Best,
> > Robert
>

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

Posted by Colin McCabe <cm...@apache.org>.

Thanks, Robert.  Looks good overall.

As a clarification about the indirections, what if I have the connect configuration key foo set up as ${vault:bar}, and in Vault, have the bar key set to ${file:baz}?  Does connect get foo as the contents of the baz file?  I would argue that it should not (and in general, we shouldn't allow ConfigProviders to indirect to other ConfigProviders) but I don't think it's spelled out right now.

What's the behavior when a config key is not found in Vault (or other ConfigProvider)?  Does the variable get replaced with the empty string, or with the literal ${vault:whatever} string?

Do we really need "${provider:[path:]key}", or can it just be ${provider:key}?  It seems like the path can be rolled up into the key.  So if you want to put your connect keys under my.connect.path, you ask for ${vault:my.connect.path.jdbc.config}, etc.

>    // A delayMs of 0 indicates an immediate change; a positive delayMs indicates 
>    // that a future change is anticipated (such as a lease duration)
>    void onChange(String path, Map<String, String> values, int delayMs);

Do we really need delayMs?  It seems like if you get a callback with delayMs set, you don't know what the new values will be, only that an update is coming, but not yet here.

best,
Colin

On Wed, May 16, 2018, at 17:05, Robert Yokota wrote:
> Hello everyone,
> 
> After a good round of discussions with excellent feedback and no major
> objections, I would like to start a vote on KIP-297 to externalize secrets
> from Kafka Connect configurations.  My thanks in advance!
> 
> KIP: <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-297%3A+Externalizing+Secrets+for+Connect+Configurations
> >
> 
> JIRA: <https://issues.apache.org/jira/browse/KAFKA-6886>
> 
> Discussion thread: <
> https://www.mail-archive.com/dev@kafka.apache.org/msg87638.html>
> 
> Best,
> Robert