You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Tejal Adsul <te...@confluent.io> on 2019/04/16 05:06:04 UTC
Re: [DISCUSSION] KIP-421: Automatically resolve external
configurations.
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...]
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Posted by Rajini Sivaram <ra...@gmail.com>.
Thanks Tejal, KIP looks good. A couple of comments/questions:
The KIP changes the public `AbstractConfig` class to enable existing
components to choose whether or not to resolve external configs. For a
specific component, (e.g. broker), will this allow users to choose whether
or not to resolve external configs? If so, how would you specify that
option in the broker config? If not, is there a risk that existing broker
configs containing matching strings in custom configs or password may be
resolved accidentally, failing broker start up? It will be good to document
the behaviour and any compatibility issues.
In compatibility section, can you explicitly mention that we are reserving
configs starting with `config.providers`? What is the impact this would
have on broker configs that currently contain custom configs that match
this when users upgrade?
Thanks,
Rajini
On Wed, Apr 17, 2019 at 1:55 AM Colin McCabe <cm...@apache.org> wrote:
> Thanks, Tejal. Looks good overall.
>
> > This KIP will enable all the components broker, connect, producer,
> consumer, admin client,
> > and so forth. to automatically resolve the external configurations.
>
> It would be nice to spell out that these components all automatically
> resolve external configurations after this KIP is implemented, not just
> that they have that ability.
>
> Does the broker reload external configurations even if they are not
> configured via KIP-226 dynamic configs?
>
> best,
> Colin
>
>
> On Mon, Apr 15, 2019, at 22:24, Tejal Adsul wrote:
> > 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...]
>
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Posted by Colin McCabe <cm...@apache.org>.
Thanks, Tejal. Looks good overall.
> This KIP will enable all the components broker, connect, producer, consumer, admin client,
> and so forth. to automatically resolve the external configurations.
It would be nice to spell out that these components all automatically resolve external configurations after this KIP is implemented, not just that they have that ability.
Does the broker reload external configurations even if they are not configured via KIP-226 dynamic configs?
best,
Colin
On Mon, Apr 15, 2019, at 22:24, Tejal Adsul wrote:
> 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...]