Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Hi Tejal, Thanks for the updates. Looks good :) best, Colin On Wed, Apr 17, 2019, at 16:53, TEJAL ADSUL wrote: > Hi Rajini and Colin, > > I have incorporated your feedbacks and have enabled the the automatic > resolution of configs for all the components. Also I have removed the > flag to enable or disable the feature to address Rajini's feedback as > the user won't be able to enable or disable the feature so it dint add > anything. > > Please could you'll review the KIP and let me know if it has addressed > your concerns and any other feedback. > > Thanks, > Tejal > > On 2019/04/17 23:34:37, TEJAL ADSUL wrote: > > > > Thanks for the feedback Colin. And agree we should enable it for all the > > components as this feature is going to benefit all the components and not > > just Connect. I have updated the KIP to enable it for all the components. > > > > Thanks, > > Tejal > > > > On 2019/04/17 23:22:50, "Colin McCabe" wrote: > > > On Wed, Apr 17, 2019, at 07:49, TEJAL ADSUL wrote: > > > > > > > > Hi Colin, > > > > > > > > By default we are enabling this feature only Connect. All the other > > > > components can enable or disable the feature as needed. > > > > > > Hi Tejal, > > > > > > I believe we should enable automatically resolving external > > > configurations in all components, not just Connect. I really can't > > > emphasize this point enough. If all we care about is Connect, we don't > > > need a new KIP, because Connect already has external configuration > > > support. I see no reason why we shouldn't just enable external > > > configuration support in every component. > > > > > > > No it won't reload the configuration if its not configured using KIP > > > > 226 as in order for the dynamic configs to be reloaded it has to be > > > > triggered by the user using the Admin Client. For static configs we > > > > dont perform any reloading and happens only at the construction time. > > > > > > OK. We probably should have some generic way of triggering a reload of > > > configurations, whether or not they're stored in ZK. But I guess that > > > can wait for a different KIP. > > > > > > best, > > > Colin > > > > > > > > > > > Thanks, > > > > Tejal > > > > > > > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > > > > Hi Rajini, > > > > > > > > > > The user wont have the ability to choose whether config value should > > > > > be enabled/disabled. Its enable/disabled per component by hardcoding > > > > > the value. I have documented your recommendations in the > > > > > compatibility sections. > > > > > > > > > > Thanks, > > > > > Tejal > > > > > > > > > > > > > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Hi Rajini and Colin, I have incorporated your feedbacks and have enabled the the automatic resolution of configs for all the components. Also I have removed the flag to enable or disable the feature to address Rajini's feedback as the user won't be able to enable or disable the feature so it dint add anything. Please could you'll review the KIP and let me know if it has addressed your concerns and any other feedback. Thanks, Tejal On 2019/04/17 23:34:37, TEJAL ADSUL wrote: > > Thanks for the feedback Colin. And agree we should enable it for all the > components as this feature is going to benefit all the components and not > just Connect. I have updated the KIP to enable it for all the components. > > Thanks, > Tejal > > On 2019/04/17 23:22:50, "Colin McCabe" wrote: > > On Wed, Apr 17, 2019, at 07:49, TEJAL ADSUL wrote: > > > > > > Hi Colin, > > > > > > By default we are enabling this feature only Connect. All the other > > > components can enable or disable the feature as needed. > > > > Hi Tejal, > > > > I believe we should enable automatically resolving external configurations > > in all components, not just Connect. I really can't emphasize this point > > enough. If all we care about is Connect, we don't need a new KIP, because > > Connect already has external configuration support. I see no reason why we > > shouldn't just enable external configuration support in every component. > > > > > No it won't reload the configuration if its not configured using KIP > > > 226 as in order for the dynamic configs to be reloaded it has to be > > > triggered by the user using the Admin Client. For static configs we > > > dont perform any reloading and happens only at the construction time. > > > > OK. We probably should have some generic way of triggering a reload of > > configurations, whether or not they're stored in ZK. But I guess that can > > wait for a different KIP. > > > > best, > > Colin > > > > > > > > Thanks, > > > Tejal > > > > > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > > > Hi Rajini, > > > > > > > > The user wont have the ability to choose whether config value should be > > > > enabled/disabled. Its enable/disabled per component by hardcoding the > > > > value. I have documented your recommendations in the compatibility > > > > sections. > > > > > > > > Thanks, > > > > Tejal > > > > > > > > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Thanks for the feedback Colin. And agree we should enable it for all the components as this feature is going to benefit all the components and not just Connect. I have updated the KIP to enable it for all the components. Thanks, Tejal On 2019/04/17 23:22:50, "Colin McCabe" wrote: > On Wed, Apr 17, 2019, at 07:49, TEJAL ADSUL wrote: > > > > Hi Colin, > > > > By default we are enabling this feature only Connect. All the other > > components can enable or disable the feature as needed. > > Hi Tejal, > > I believe we should enable automatically resolving external configurations in > all components, not just Connect. I really can't emphasize this point > enough. If all we care about is Connect, we don't need a new KIP, because > Connect already has external configuration support. I see no reason why we > shouldn't just enable external configuration support in every component. > > > No it won't reload the configuration if its not configured using KIP > > 226 as in order for the dynamic configs to be reloaded it has to be > > triggered by the user using the Admin Client. For static configs we > > dont perform any reloading and happens only at the construction time. > > OK. We probably should have some generic way of triggering a reload of > configurations, whether or not they're stored in ZK. But I guess that can > wait for a different KIP. > > best, > Colin > > > > > Thanks, > > Tejal > > > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > > Hi Rajini, > > > > > > The user wont have the ability to choose whether config value should be > > > enabled/disabled. Its enable/disabled per component by hardcoding the > > > value. I have documented your recommendations in the compatibility > > > sections. > > > > > > Thanks, > > > Tejal > > > > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
On Wed, Apr 17, 2019, at 07:49, TEJAL ADSUL wrote: > > Hi Colin, > > By default we are enabling this feature only Connect. All the other > components can enable or disable the feature as needed. Hi Tejal, I believe we should enable automatically resolving external configurations in all components, not just Connect. I really can't emphasize this point enough. If all we care about is Connect, we don't need a new KIP, because Connect already has external configuration support. I see no reason why we shouldn't just enable external configuration support in every component. > No it won't reload the configuration if its not configured using KIP > 226 as in order for the dynamic configs to be reloaded it has to be > triggered by the user using the Admin Client. For static configs we > dont perform any reloading and happens only at the construction time. OK. We probably should have some generic way of triggering a reload of configurations, whether or not they're stored in ZK. But I guess that can wait for a different KIP. best, Colin > > Thanks, > Tejal > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > Hi Rajini, > > > > The user wont have the ability to choose whether config value should be > > enabled/disabled. Its enable/disabled per component by hardcoding the > > value. I have documented your recommendations in the compatibility sections. > > > > Thanks, > > Tejal > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Thanks for making the changes, Tejal. The KIP looks great. Randall On Wed, Apr 17, 2019 at 10:32 AM TEJAL ADSUL wrote: > > Thanks for the feedback Randall. I have incorporated the below changes in > the KIP. > > Thanks, > Tejal > > On 2019/04/17 15:42:31, Randall Hauch wrote: > > Overall this looks good. A few specific requests to hopefully improve the > > clarity of the proposal: > > > > 1) The motivation section should more clearly state that this feature > will > > allow it to be reused in multiple components, Connect will use this > feature > > to resolve variables in the worker configurations via the existing > > 'StandaloneConfig’ and ‘DistributedConfig’ subclasses of AbstractConfig, > > that Connect already supports variables in connector configs via KIP-297, > > and that other components will have to be modified in the future to use > > this feature. Essentially, this builds on KIP-297 for variables in > Connect > > workers, but it does it in a way that other components can use in the > > future to automatically resolve variables in their configs with few > changes > > in code. > > > > 2) The Public Interfaces section talks about “the constructor”, but there > > are two new constructors presented, and each should be described. It > should > > also be more clear that the existing constructors will not enable > > resolution by default. > > > > 3) In several places were the new reserved configs are described, this > text > > appears: > > > > “and "config.providers.providerName.variableName" to specify any > > additional configs required by providers.“ > > > > This needs to make it more clear that “providerName” and “variableName” > are > > not literals; maybe surround them with angle brackets and call this out > > explicitly. > > > > 4) The “Dynamic configurations for Brokers” makes it sound like the > > broker’s behavior will change, but based on the discussions above it > sounds > > like only Connect will enable this feature. Is it even necessary to > mention > > this here? > > > > 5) Include as a rejected alternative the enabling by default that > existing > > AbstractConfig constructors will automatically resolve variables using > > config providers defined in the same configs. Instead, automatic > resolution > > must be explicitly enabled/used by the subclass for all components except > > Connect’s worker configurations. > > > > Best regards, > > > > Randall > > > > On Wed, Apr 17, 2019 at 7:49 AM TEJAL ADSUL wrote: > > > > > > > > Hi Colin, > > > > > > By default we are enabling this feature only Connect. All the other > > > components can enable or disable the feature as needed. > > > > > > No it won't reload the configuration if its not configured using KIP > 226 > > > as in order for the dynamic configs to be reloaded it has to be > triggered > > > by the user using the Admin Client. For static configs we dont perform > any > > > reloading and happens only at the construction time. > > > > > > Thanks, > > > Tejal > > > > > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > > > Hi Rajini, > > > > > > > > The user wont have the ability to choose whether config value should > be > > > enabled/disabled. Its enable/disabled per component by hardcoding the > > > value. I have documented your recommendations in the compatibility > sections. > > > > > > > > Thanks, > > > > Tejal > > > > > > > > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Thanks for the feedback Randall. I have incorporated the below changes in the KIP. Thanks, Tejal On 2019/04/17 15:42:31, Randall Hauch wrote: > Overall this looks good. A few specific requests to hopefully improve the > clarity of the proposal: > > 1) The motivation section should more clearly state that this feature will > allow it to be reused in multiple components, Connect will use this feature > to resolve variables in the worker configurations via the existing > 'StandaloneConfig’ and ‘DistributedConfig’ subclasses of AbstractConfig, > that Connect already supports variables in connector configs via KIP-297, > and that other components will have to be modified in the future to use > this feature. Essentially, this builds on KIP-297 for variables in Connect > workers, but it does it in a way that other components can use in the > future to automatically resolve variables in their configs with few changes > in code. > > 2) The Public Interfaces section talks about “the constructor”, but there > are two new constructors presented, and each should be described. It should > also be more clear that the existing constructors will not enable > resolution by default. > > 3) In several places were the new reserved configs are described, this text > appears: > > “and "config.providers.providerName.variableName" to specify any > additional configs required by providers.“ > > This needs to make it more clear that “providerName” and “variableName” are > not literals; maybe surround them with angle brackets and call this out > explicitly. > > 4) The “Dynamic configurations for Brokers” makes it sound like the > broker’s behavior will change, but based on the discussions above it sounds > like only Connect will enable this feature. Is it even necessary to mention > this here? > > 5) Include as a rejected alternative the enabling by default that existing > AbstractConfig constructors will automatically resolve variables using > config providers defined in the same configs. Instead, automatic resolution > must be explicitly enabled/used by the subclass for all components except > Connect’s worker configurations. > > Best regards, > > Randall > > On Wed, Apr 17, 2019 at 7:49 AM TEJAL ADSUL wrote: > > > > > Hi Colin, > > > > By default we are enabling this feature only Connect. All the other > > components can enable or disable the feature as needed. > > > > No it won't reload the configuration if its not configured using KIP 226 > > as in order for the dynamic configs to be reloaded it has to be triggered > > by the user using the Admin Client. For static configs we dont perform any > > reloading and happens only at the construction time. > > > > Thanks, > > Tejal > > > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > > Hi Rajini, > > > > > > The user wont have the ability to choose whether config value should be > > enabled/disabled. Its enable/disabled per component by hardcoding the > > value. I have documented your recommendations in the compatibility sections. > > > > > > Thanks, > > > Tejal > > > > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Overall this looks good. A few specific requests to hopefully improve the clarity of the proposal: 1) The motivation section should more clearly state that this feature will allow it to be reused in multiple components, Connect will use this feature to resolve variables in the worker configurations via the existing 'StandaloneConfig’ and ‘DistributedConfig’ subclasses of AbstractConfig, that Connect already supports variables in connector configs via KIP-297, and that other components will have to be modified in the future to use this feature. Essentially, this builds on KIP-297 for variables in Connect workers, but it does it in a way that other components can use in the future to automatically resolve variables in their configs with few changes in code. 2) The Public Interfaces section talks about “the constructor”, but there are two new constructors presented, and each should be described. It should also be more clear that the existing constructors will not enable resolution by default. 3) In several places were the new reserved configs are described, this text appears: “and "config.providers.providerName.variableName" to specify any additional configs required by providers.“ This needs to make it more clear that “providerName” and “variableName” are not literals; maybe surround them with angle brackets and call this out explicitly. 4) The “Dynamic configurations for Brokers” makes it sound like the broker’s behavior will change, but based on the discussions above it sounds like only Connect will enable this feature. Is it even necessary to mention this here? 5) Include as a rejected alternative the enabling by default that existing AbstractConfig constructors will automatically resolve variables using config providers defined in the same configs. Instead, automatic resolution must be explicitly enabled/used by the subclass for all components except Connect’s worker configurations. Best regards, Randall On Wed, Apr 17, 2019 at 7:49 AM TEJAL ADSUL wrote: > > Hi Colin, > > By default we are enabling this feature only Connect. All the other > components can enable or disable the feature as needed. > > No it won't reload the configuration if its not configured using KIP 226 > as in order for the dynamic configs to be reloaded it has to be triggered > by the user using the Admin Client. For static configs we dont perform any > reloading and happens only at the construction time. > > Thanks, > Tejal > > On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > > Hi Rajini, > > > > The user wont have the ability to choose whether config value should be > enabled/disabled. Its enable/disabled per component by hardcoding the > value. I have documented your recommendations in the compatibility sections. > > > > Thanks, > > Tejal > > >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Hi Colin, By default we are enabling this feature only Connect. All the other components can enable or disable the feature as needed. No it won't reload the configuration if its not configured using KIP 226 as in order for the dynamic configs to be reloaded it has to be triggered by the user using the Admin Client. For static configs we dont perform any reloading and happens only at the construction time. Thanks, Tejal On 2019/04/17 14:36:33, TEJAL ADSUL wrote: > Hi Rajini, > > The user wont have the ability to choose whether config value should be > enabled/disabled. Its enable/disabled per component by hardcoding the value. > I have documented your recommendations in the compatibility sections. > > Thanks, > Tejal >
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
Hi Rajini, The user wont have the ability to choose whether config value should be enabled/disabled. Its enable/disabled per component by hardcoding the value. I have documented your recommendations in the compatibility sections. Thanks, Tejal
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
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 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 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 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 > 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" 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
Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.
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 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 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 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" 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.>> > > > > > >> > > > > > > > >> > > > > >
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 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 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 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" 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
> > > > > >> >
> >
