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
> > > >> > > > >
> > > >> > >
> > > >> > >
> > > >>
> > > >
> > > >
> >
>
