Looks great.
+1 (non-binding)
Regards,
Randall
On Fri, May 18, 2018 at 10:23 AM, Rajini Sivaram <rajinisiva...@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 <rayok...@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 <rajinisiva...@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 <
> > e...@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 <yuzhih...@gmail.com> wrote:
> > > >
> > > > > +1
> > > > > -------- Original message --------From: Magesh Nandakumar <
> > > > > mage...@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 <cmcc...@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 <
> > > rayok...@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 <
> > > cmcc...@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 <
> > > > cmcc...@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
> > > > > > > >> > > > <cmcc...@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
> > > > > > > >> > > > >
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
