Hi All, I have updated the KIP to address the comments in the discussion. I have added the flow as to how dynamic config values will be resolved. Please could you’ll review the updated changes and let me know your feedback.
Thanks, Tejal On 2019/03/21 20:38:54, Tejal Adsul <t...@confluent.io> wrote: > I have addressed the comments 1 and 2 in the KIP.> > 3. The example is a bit misleading with the password in it. I have modified > it. We basically wanted to show that you cam pass any additional parameters > required by the config provider> > 4. Yes all the public config classes (ProducerConfig, ConsumerConfig, > ConnectorConfig etc.) will> > > > be extended to optionally use the new AbstractConfig constructors?>> > > > On 2019/03/14 11:49:46, Rajini Sivaram <r....@gmail.com> wrote: > > > Hi Tejal,> > > > > > > Thanks for the updates. A few comments:> > > > > > > > > > 1. In the standard KIP template, we have two sections `Public> > > > Interfaces` and `Proposed Changes`. Can you split the section > > `Proposal`> > > > into two so that public interface changes are more obvious?> > > > 2. Under `Public Interfaces`, can you separate out interface changes > > and> > > > new configurations since the config changes are sort of lost in the > > text?> > > > In particular, I think this KIP is proposing to reserve the config name> > > > > > `config.providers` as well as all config names starting with> > > > `config.providers.` to resolve configs.> > > > 3. The example looks a bit odd to me. It looks like we are removing> > > > local passwords like truststore password from a client config and > > instead> > > > adding a master password like vault password in cleartext into the > > file.> > > > Perhaps the intention is that the vault password won't be in the file > > for a> > > > vault provider?> > > > 4. The example instantiates AbstractConfig. I am not familiar with the> > > > > > usage of this class in Connect, but is the intention that all the > > public> > > > config classes (ProducerConfig, ConsumerConfig, ConnectorConfig etc.) > > will> > > > be extended to optionally use the new AbstractConfig constructors?> > > > > > > Regards,> > > > > > > Rajini> > > > > > > > > > On Mon, Mar 11, 2019 at 5:49 PM Tejal Adsul <te...@confluent.io> wrote:> > > > > > > > Hi Folks,> > > > >> > > > > I have accommodated most of the review comments for> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-421%3A+Support+resolving+externalized+secrets+in+AbstractConfig> > > > > > > > . Reopening the thread for further discussion. Please let me know your> > > > > thoughts on it.> > > > >> > > > > Thanks,> > > > > Tejal> > > > >> > > > > On 2019/01/25 19:11:07, "Colin McCabe" <c....@apache.org> wrote:> > > > > > On Fri, Jan 25, 2019, at 09:12, Andy Coates wrote:>> > > > > > > > Further, if we're worried about confusion about how to)>> > > > > > > load the two files, we could have a constructor that does that> > > > > default>> > > > > > > pattern for you.>> > > > > > > >> > > > > > > Yeah, I don't really see the need for this two step / two file> > > > > approach. I>> > > > > > > think the config providers should be listed in the main property > > > > > file,> > > > > not>> > > > > > > some secondary file, and we should avoid backwards compatibility> > > > > issues by,>> > > > > > > as Ewan says, having a new constructor, (deprecating the old), that> > > > > > > > > > allows>> > > > > > > the functionality to be turned on/off.>> > > > > >> > > > > > +1. In the case of the Kafka broker, it really seems like we should > > > > put> > > > > the config providers in the main config file. >> > > > > > It's more complex to have multiple configuration files, and it > > > > doesn't> > > > > seem to add any value.>> > > > > >> > > > > > In the case of other components like Connect, I don't have a strong> > > > > opinion. We can discuss this on a component-by-component basis. > > > Clearly> > > > > not all components manage configuration exactly the same way, and that> > > > > difference might motivate different strategies here.>> > > > > >> > > > > > > >> > > > > > > I suggest we also consider adding a new method to AbstractConfig to > > > > > >> > > > > > > allow>> > > > > > > applications to get the unresolved raw value, e.g. String>> > > > > > > getRawValue(String key). Given a config entry like ">> > > > > > > config.providers.vault.password=$>> > > > > > > <> > > > > https://cwiki.apache.org/confluence/display/KAFKA/config.providers.vault.password=$>>> > > > > > > >> > > > > > > {file:/path/to/secrets.properties:vault.secret.password}" then >> > > > > > > getRawValue>> > > > > > > would always return "$>> > > > > > > <> > > > > https://cwiki.apache.org/confluence/display/KAFKA/config.providers.vault.password=$>>> > > > > > > >> > > > > > > {file:/path/to/secrets.properties:vault.secret.password}". I can see > > > > > >> > > > > > > this>> > > > > > > being useful.>> > > > > >> > > > > > I think one of the problems with the interface proposed in KIP-421 is> > > > > > > > > that it doesn't give brokers any way to listen for changes to the> > > > > configuration. We've done a lot of work to make certain configuration > > > keys> > > > > dynamic, but we're basically saying if you use external secrets, you > > > can't> > > > > make use of that at all-- you have to restart the broker to change> > > > > configuration.>> > > > > >> > > > > > Unfortunately, the AbstractConfig interface isn't well suited to> > > > > listening for config changes. In order to do that, you probably need to> > > > > > > > use the KIP-297 interface directly. Which means that maybe we should go> > > > > > > > back to the drawing board here, unfortunately. :(>> > > > > >> > > > > > best,>> > > > > > Colin>> > > > > >> > > > > > > >> > > > > > > With regards to on-change subscription: surely all we'd need is to> > > > > provide>> > > > > > > a way for users to attach a callback for a given key, right? e.g.> > > > > `boolean>> > > > > > > subscribe(key, callback)`, where the return value is true if the key> > > > > > > > > > has a>> > > > > > > config provider, false if it doesn't. I think this would be> > > > > worthwhile>> > > > > > > including as it stops people having to build their own, doing the> > > > > parsing>> > > > > > > and wiring themselves.>> > > > > > > >> > > > > > > Andy>> > > > > > > >> > > > > > > On Fri, 25 Jan 2019 at 09:11, Rajini Sivaram <ra...@gmail.com>>> > > > > > > wrote:>> > > > > > > >> > > > > > > > *Regarding brokers, I think if we want to avoid exposing secrets>> > > > > > > > > > > > > > over DescribeConfigs responses, we'd probably need changes similar> > > > > > > > > > > to those>> > > > > > > > we needed to make for the Connect REST API. *>> > > > > > > >>> > > > > > > > Password configs are not returned in DescribeConfigs response in> > > > > the>> > > > > > > > broker. The response indicates that the config is sensitive and no> > > > > > > > > > > value is>> > > > > > > > returned.>> > > > > > > >>> > > > > > > >>> > > > > > > > On Thu, Jan 24, 2019 at 11:38 PM Ewen Cheslack-Postava <> > > > > ew...@confluent.io>>> > > > > > > > wrote:>> > > > > > > >>> > > > > > > > > > It allows _all_ existing clients of the class, e.g. those in> > > > > Apache>> > > > > > > > > Kafka or in applications written by other people that use the> > > > > class, to>> > > > > > > > get>> > > > > > > > > this functionality for free, i.e. without any code changes. (I> > > > > > > > > > > > realize>> > > > > > > > > this is probably where the 'unexpected effects' comes from).>> > > > > > > > >>> > > > > > > > > > Personally, I think we should do our best to make this work> > > > > seamlessly>> > > > > > > > />> > > > > > > > > transparently, because we're likely going to have this> > > > > functionality for>> > > > > > > > a>> > > > > > > > > long time.>> > > > > > > > >>> > > > > > > > > Connect (and connectors that may also use AbstractConfig for> > > > > themselves>> > > > > > > > > since they are supposed to expose a ConfigDef anyway) could> > > > > definitely be>> > > > > > > > > an issue. I'd imagine formats like this are rare, but we do know> > > > > > > > > > > > there>> > > > > > > > are>> > > > > > > > > some cases where people add new syntax, e.g. the landoop> > > > > connectors>> > > > > > > > support>> > > > > > > > > some sort of inline sql-like transformation. I don't know of any> > > > > > > > > > > > cases>> > > > > > > > that>> > > > > > > > > would specifically conflict with the syntax, but there is some> > > > > risk.>> > > > > > > > >>> > > > > > > > > I agree getting it automated would be ideal, and it is probably> > > > > > > > > > > > more>> > > > > > > > > reasonable to claim any issues would be unlike if unresolvable> > > > > cases>> > > > > > > > don't>> > > > > > > > > result in an exception. On the other hand, I think the vast> > > > > majority of>> > > > > > > > the>> > > > > > > > > benefit would come from making this work for brokers, Connect,> > > > > and>> > > > > > > > Streams>> > > > > > > > > (and in most applications making this work is pretty trivial > > > > > > > given> > > > > the>> > > > > > > > > answer to question (1) is that it works by passing same config > > > > > > > to> > > > > the>> > > > > > > > > static method then constructor).>> > > > > > > > >>> > > > > > > > > Tying this discussion also back to the question about > > > > > > > subscribing> > > > > for>> > > > > > > > > updates, apps would commonly need modification to support that,> > > > > > > > > > > > and I>> > > > > > > > think>> > > > > > > > > ideally you want to be using some sort of KMS where rotation is> > > > > > > > > > > > done>> > > > > > > > > automatically and you need to subscribe to updates. Since it's a> > > > > > > > > > > > pretty>> > > > > > > > > common pattern to only look up configs once instead of always> > > > > going back>> > > > > > > > to>> > > > > > > > > the AbstractConfig, you'd really only be able to get some of the> > > > > > > > > > > > long>> > > > > > > > term>> > > > > > > > > intended benefit of this improvement. We should definitely have > > > > > > > a> > > > > follow>> > > > > > > > up>> > > > > > > > > to this that deals with the subscriptions, but I think the > > > > > > > current> > > > > scope>> > > > > > > > is>> > > > > > > > > still a useful improvement -- Connect got this implemented> > > > > because>> > > > > > > > exposure>> > > > > > > > > of secrets via REST API was such a big problem. Making the > > > > > > > changes> > > > > in>> > > > > > > > > AbstractConfig is a better long term solution so we can get this> > > > > > > > > > > > working>> > > > > > > > > with all components.>> > > > > > > > >>> > > > > > > > > Regarding brokers, I think if we want to avoid exposing secrets> > > > > > > > > > > > over>> > > > > > > > > DescribeConfigs responses, we'd probably need changes similar to> > > > > > > > > > > > those we>> > > > > > > > > needed to make for the Connect REST API. Also agree we'd need to> > > > > > > > > > > > think>> > > > > > > > > about how to make this work with dynamic configs (which would > > > > > > > also> > > > > be a>> > > > > > > > > nice thing to extend to, e.g., Connect).>> > > > > > > > >>> > > > > > > > > As a practical suggestion, while it doesn't give you the update> > > > > > > > > > > > for free,>> > > > > > > > > we could consider also deprecating the existing constructor to> > > > > encourage>> > > > > > > > > people to update. Further, if we're worried about confusion > > > > > > > about> > > > > how to>> > > > > > > > > load the two files, we could have a constructor that does that> > > > > default>> > > > > > > > > pattern for you.>> > > > > > > > >>> > > > > > > > > -Ewen>> > > > > > > > >>> > > > > > > > > On Thu, Jan 24, 2019 at 11:36 AM Colin McCabe > > > > > > > <cm...@apache.org>>> > > > > > > > wrote:>> > > > > > > > >>> > > > > > > > > > On Thu, Jan 24, 2019, at 11:25, TEJAL ADSUL wrote:>> > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > > > On 2019/01/24 17:26:02, Andy Coates <an...@confluent.io>> > > > > wrote:>> > > > > > > > > > > > I'm wondering why we're rejected changing AbstractConfig > > > > > > > > > > to>> > > > > > > > > > automatically>> > > > > > > > > > > > resolve the variables?>> > > > > > > > > > > >>> > > > > > > > > > > > > 1. Change AbstractConfig to *automatically* resolve> > > > > variables of>> > > > > > > > > the>> > > > > > > > > > form>> > > > > > > > > > > > specified in KIP-297. This was rejected because it would> > > > > change the>> > > > > > > > > > > > behavior of existing code and might cause unexpected> > > > > effects.>> > > > > > > > > > > >>> > > > > > > > > > > > Doing so seems to me to have two very large benefits:>> > > > > > > > > > > >>> > > > > > > > > > > > 1. It allows the config providers to be defined within the> > > > > > > > > > > > > > > same>> > > > > > > > file>> > > > > > > > > > as the>> > > > > > > > > > > > config that uses the providers, e.g.>> > > > > > > > > > > >>> > > > > > > > > > > > config.providers=file,vault>> > > > > > > > > > > > <>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/config.providers=file,vault>> > > > > > > >> > > > > > > > > > >>> > > > > > > > > > > > config.providers.file.>> > > > > > > > > > > > <>> > > > > > > > > >>> > > > > > > >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/config.providers.file>> > > > > > > > > > > > .>>> > > > > > > > > > > > class=org.apache.kafka.connect.configs.FileConfigProvider>> > > > > > > > > > > > > > > > > > > > > > <>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/org.apache.kafka.connect.configs.FileConfigProvider>> > > > > > > >> > > > > > > > > > >>> > > > > > > > > > > > config.providers.file.param.path=>> > > > > > > > > > > > <>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/config.providers.file.other.prop=another>> > > > > > > >> > > > > > > > > > >>> > > > > > > > > > > > /mnt/secrets/passwords>> > > > > > > > > > > >>> > > > > > > > > > > > foo.baz=/usr/temp/>> > > > > > > > > > > > <>> > > > > > > > >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/foo.baz=/usr/temp/>>> > > > > > > > > > > > foo.bar=$ <>> > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/foo.bar=$>> > > > > > > > > > >>> > > > > > > > > > > > {file:/path/to/variables.properties:foo.bar}>> > > > > > > > > > > >>> > > > > > > > > > > > Is this possible with what's currently being proposed? i.e> > > > > > > > > > > > > > > could>> > > > > > > > you>> > > > > > > > > > load>> > > > > > > > > > > > the file and pass the map first to `loadConfigProviders` > > > > > > > > > > and> > > > > then>> > > > > > > > > > again to>> > > > > > > > > > > > the constructor?>> > > > > > > > > > > >>> > > > > > > > > > > > 2. It allows _all_ existing clients of the class, e.g. > > > > > > > > > > those> > > > > in>> > > > > > > > > Apache>> > > > > > > > > > > > Kafka or in applications written by other people that use> > > > > > > > > > > > > > > the>> > > > > > > > class,>> > > > > > > > > > to get>> > > > > > > > > > > > this functionality for free, i.e. without any code > > > > > > > > > > changes.> > > > > (I>> > > > > > > > > realize>> > > > > > > > > > > > this is probably where the 'unexpected effects' comes> > > > > from).>> > > > > > > > > > > >>> > > > > > > > > > > > I'm assuming the unexpected side effects come about if an> > > > > > > > > > > > > > > existing>> > > > > > > > > > > > properties file already contains compatible> > > > > config.providers>> > > > > > > > > > > > <>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >> > > > > https://cwiki.apache.org/confluence/display/KAFKA/config.providers=file,vault>> > > > > > > >> > > > > > > > > > >>> > > > > > > > > > > > entries _and_ has other properties in the form ${xx:yy} > > > > > > > > > > or>> > > > > > > > > > ${xx:yy:zz}.>> > > > > > > > > > > > While possible, these seems fairly unlikely unless for> > > > > random>> > > > > > > > client>> > > > > > > > > > > > property files. So I'm assuming there's a specific > > > > > > > > > > instance> > > > > where>> > > > > > > > we>> > > > > > > > > > think>> > > > > > > > > > > > this is likely? Something to do with Connect config > > > > > > > > > > maybe?>> > > > > > > > > > > >>> > > > > > > > > > > > Personally, I think we should do our best to make this > > > > > > > > > > work>> > > > > > > > > seam [message truncated...]
