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