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