[jira] [Created] (KAFKA-8592) Broker Dynamic Configuration fails to resolve variables as per KIP-421

[TEJAL ADSUL (JIRA)]

TEJAL ADSUL created KAFKA-8592:
--

 Summary: Broker Dynamic Configuration fails to resolve variables 
as per KIP-421
 Key: KAFKA-8592
 URL: https://issues.apache.org/jira/browse/KAFKA-8592
 Project: Kafka
  Issue Type: Bug
  Components: config
Affects Versions: 2.3.0
Reporter: TEJAL ADSUL
 Fix For: 2.3.1


In add/alter new configs for DynamicConfigs it does not go through the 
KafkaConfig
eg: bin/kafka-configs --bootstrap-server localhost:9092 --entity-type brokers 
--entity-name 0 --alter --add-config log.cleaner.threads=2
However the bootstrap-server localhost is parsed through the kafkaConfig to 
create the adminClient but not the log.cleaner.thread. 

As the configs are not parsed using the KafkaConfig if we pass variables in 
configs they are bot resolved at run time.

In order to resolve the variables in alterConfig/addConfigs flow we need to 
parse the new configs  using KafkaConfig before they are parsed.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (KAFKA-8426) KIP 421 Bug: ConfigProvider configs param inconsistent with KIP-297

[TEJAL ADSUL (JIRA)]

TEJAL ADSUL created KAFKA-8426:
--

 Summary: KIP 421 Bug: ConfigProvider configs param inconsistent 
with KIP-297
 Key: KAFKA-8426
 URL: https://issues.apache.org/jira/browse/KAFKA-8426
 Project: Kafka
  Issue Type: Bug
Reporter: TEJAL ADSUL
 Fix For: 2.3.0


According to KIP-297 a parameter is passed to ConfigProvider with syntax 
"config.providers.\{name}.param.\{param-name}". Currently AbstractConfig allows 
parameters of the format "config.providers.\{name}.\{param-name}". With this 
fix AbstractConfig will be consistent with KIP-297 syntax.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (KAFKA-8425) KIP 421 Bug: Modifying Immutable Originals Map results in Java exception

[TEJAL ADSUL (JIRA)]

TEJAL ADSUL created KAFKA-8425:
--

 Summary: KIP 421 Bug: Modifying Immutable Originals Map results in 
Java exception 
 Key: KAFKA-8425
 URL: https://issues.apache.org/jira/browse/KAFKA-8425
 Project: Kafka
  Issue Type: Bug
  Components: config
Reporter: TEJAL ADSUL
 Fix For: 2.3.0


The originals map passed to the AbstractConfig class can be immutable. The 
resolveConfigVariable function was modifying the originals map. If this map is 
immutable it will result in an exception.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Re: [VOTE] KIP-421: Automatically resolve external configurations.

[TEJAL ADSUL]

Thanks Dongjin for taking time to review the KIP and vote 

Thanks,
Tejal
On 2019/04/24 01:19:36, Dongjin Lee  wrote: 
> @Colin I see. Thanks for the correction!
> @Tejal Thanks for the excellent proposal! +1k!
> 
> Thanks,
> Dongjin
> 
> On Mon, Apr 22, 2019 at 10:26 PM Colin McCabe  wrote:
> 
> > Hi Dongjin,
> >
> > Thanks for checking out the KIP.
> >
> > Randall is a committer, actually.  He was added two months ago.  It looks
> > like he isn't on the committers page yet, though.
> >
> > Randall, can you add your name to https://kafka.apache.org/committers.html
> > to help prevent confusion in the future?
> >
> > best,
> > Colin
> >
> >
> > On Mon, Apr 22, 2019, at 16:44, Dongjin Lee wrote:
> > > I just read the KIP. +1.
> > >
> > > Currently:
> > >
> > > Binding: +2 (Colin, Gwen)
> > > Non-binding: +2 (Randall, Dongjin)
> > >
> > > Thanks,
> > > Dongjin
> > >
> > > On Mon, Apr 22, 2019 at 10:26 AM TEJAL ADSUL  wrote:
> > >
> > > > Hi Folks,
> > > >
> > > > Just a reminder that I will be closing the vote for KIP-421 by today
> > EOD,
> > > > please cast your votes by today.
> > > >
> > > > Thanks Colin, Randall, Gwen for the votes.
> > > >
> > > > Thanks,
> > > > Tejal
> > > >
> > > >
> > > >
> > > > On 2019/04/19 22:43:25, "Colin McCabe"  wrote:
> > > > > +1.  Thanks, Tejal.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > On Thu, Apr 18, 2019, at 15:02, TEJAL ADSUL wrote:
> > > > > > Hi All,
> > > > > >
> > > > > > As we have reached a consensus on the design, I would like to
> > start a
> > > > > > vote for KIP-421. Below are the links for this proposal:
> > > > > >
> > > > > > KIP Link:
> > > > > >
> > > >
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100829515
> > > > > > DiscussionThread:
> > > > > >
> > > >
> > https://lists.apache.org/thread.html/a2f834d876e9f8fb3977db794bf161818c97f7f481edd1b10449d89f@%3Cdev.kafka.apache.org%3E
> > > > > >
> > > > > > Thanks,
> > > > > > Tejal
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > *Dongjin Lee*
> > >
> > > *A hitchhiker in the mathematical world.*
> > > *github:  <http://goog_969573159/>github.com/dongjinleekr
> > > <https://github.com/dongjinleekr>linkedin:
> > kr.linkedin.com/in/dongjinleekr
> > > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > speakerdeck.com/dongjin
> > > <https://speakerdeck.com/dongjin>*
> > >
> >
> 
> 
> -- 
> *Dongjin Lee*
> 
> *A hitchhiker in the mathematical world.*
> *github:  <http://goog_969573159/>github.com/dongjinleekr
> <https://github.com/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
> <https://kr.linkedin.com/in/dongjinleekr>speakerdeck: speakerdeck.com/dongjin
> <https://speakerdeck.com/dongjin>*
> 

Re: [VOTE] KIP-421: Automatically resolve external configurations.

[TEJAL ADSUL]

Hi all,

This KIP passes with three +1 (binding) - thanks Colin, Randall and Gwen!

For those interested, feel free to check out and join the ongoing PR review
here: https://github.com/apache/kafka/pull/6467

Thanks,
Tejal

On 2019/04/22 17:26:05, TEJAL ADSUL  wrote: 
> Hi Folks,
> 
> Just a reminder that I will be closing the vote for KIP-421 by today EOD, 
> please cast your votes by today.
> 
> Thanks Colin, Randall, Gwen for the votes.
> 
> Thanks,
> Tejal
> 
> 
> 
> On 2019/04/19 22:43:25, "Colin McCabe"  wrote: 
> > +1.  Thanks, Tejal.
> > 
> > best,
> > Colin
> > 
> > On Thu, Apr 18, 2019, at 15:02, TEJAL ADSUL wrote:
> > > Hi All,
> > > 
> > > As we have reached a consensus on the design, I would like to start a 
> > > vote for KIP-421. Below are the links for this proposal:
> > > 
> > > KIP Link: 
> > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100829515
> > > DiscussionThread: 
> > > https://lists.apache.org/thread.html/a2f834d876e9f8fb3977db794bf161818c97f7f481edd1b10449d89f@%3Cdev.kafka.apache.org%3E
> > > 
> > > Thanks,
> > > Tejal
> > >
> > 
> 

Re: [VOTE] KIP-421: Automatically resolve external configurations.

[TEJAL ADSUL]

Hi Folks,

Just a reminder that I will be closing the vote for KIP-421 by today EOD, 
please cast your votes by today.

Thanks Colin, Randall, Gwen for the votes.

Thanks,
Tejal



On 2019/04/19 22:43:25, "Colin McCabe"  wrote: 
> +1.  Thanks, Tejal.
> 
> best,
> Colin
> 
> On Thu, Apr 18, 2019, at 15:02, TEJAL ADSUL wrote:
> > Hi All,
> > 
> > As we have reached a consensus on the design, I would like to start a 
> > vote for KIP-421. Below are the links for this proposal:
> > 
> > KIP Link: 
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100829515
> > DiscussionThread: 
> > https://lists.apache.org/thread.html/a2f834d876e9f8fb3977db794bf161818c97f7f481edd1b10449d89f@%3Cdev.kafka.apache.org%3E
> > 
> > Thanks,
> > Tejal
> >
> 

[VOTE] KIP-421: Automatically resolve external configurations.

[TEJAL ADSUL]

Hi All,

As we have reached a consensus on the design, I would like to start a vote for 
KIP-421. Below are the links for this proposal:

KIP Link: 
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100829515
DiscussionThread: 
https://lists.apache.org/thread.html/a2f834d876e9f8fb3977db794bf161818c97f7f481edd1b10449d89f@%3Cdev.kafka.apache.org%3E

Thanks,
Tejal

Re: [DISCUSSION] KIP-421: Automatically resolve external configurations.

[TEJAL ADSUL]

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.

[TEJAL ADSUL]

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.

[TEJAL ADSUL]

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.

[TEJAL ADSUL]

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.

[TEJAL ADSUL]

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.

[Tejal Adsul]

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

[DISCUSS] KIP-421: Automatically resolve external configurations.

[TEJAL ADSUL]

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:36:24, Tejal Adsul  wrote: 
> @Colin we won’t be supporting the subscriber mode currently and it will be 
> added as a future work
> 2. By disabling the feature the constructor will work as it earlier. If we 
> know the configs don’t have any indirect values or we want the indirect 
> values to remain unresolved we will can just do so by using the enable flag. 
> We won’t be using this in broker as its dynamic and we have added it in 
> future section for now. 
> 
> On 2019/03/14 16:36:42, "Colin McCabe"  wrote: 
> > Hi Tejal,> 
> > 
> > Thanks for the update.> 
> > 
> > One of the critical parts of the ConfigProvider interface is the ability to 
> > monitor changes to a configuration key through ConfigProvider#subscribe and 
> > ConfigProvider#unsubscribe, etc.  I don't see how the proposed API supports 
> > this.  Can you clarify?> 
> > 
> > Also, it's not clear to me when you would want to enable KIP-421 
> > functionality and when you would want to disable it.  What is the purpose 
> > of making it possible to disable this?  Do you have examples of cases where 
> > we would use it and cases where we would not?  Would the broker use this 
> > functionality?> 
> > 
> > best,> 
> > Colin> 
> > 
> > 
> > On Mon, Mar 11, 2019, at 10:49, 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=$>>
>

[Vote] KIP-421: Support resolving externalized secrets in AbstractConfig

[TEJAL ADSUL]

Hi All,

I would like to start the vote thread for KIP-421: Support resolving 
externalized secrets in AbstractConfig.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-421%3A+Support+resolving+externalized+secrets+in+AbstractConfig

Thanks!

Regards,
Tejal

Re: [DISCUSSION] KIP-421: Support resolving externalized secrets in AbstractConfig

[Tejal Adsul]

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/

Re: [DISCUSSION] KIP-421: Support resolving externalized secrets in AbstractConfig

[Tejal Adsul]

@Colin we won’t be supporting the subscriber mode currently and it will be 
added as a future work
2. By disabling the feature the constructor will work as it earlier. If we know 
the configs don’t have any indirect values or we want the indirect values to 
remain unresolved we will can just do so by using the enable flag. We won’t be 
using this in broker as its dynamic and we have added it in future section for 
now. 

On 2019/03/14 16:36:42, "Colin McCabe"  wrote: 
> Hi Tejal,> 
> 
> Thanks for the update.> 
> 
> One of the critical parts of the ConfigProvider interface is the ability to 
> monitor changes to a configuration key through ConfigProvider#subscribe and 
> ConfigProvider#unsubscribe, etc.  I don't see how the proposed API supports 
> this.  Can you clarify?> 
> 
> Also, it's not clear to me when you would want to enable KIP-421 
> functionality and when you would want to disable it.  What is the purpose of 
> making it possible to disable this?  Do you have examples of cases where we 
> would use it and cases where we would not?  Would the broker use this 
> functionality?> 
> 
> best,> 
> Colin> 
> 
> 
> On Mon, Mar 11, 2019, at 10:49, 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 > 
> > > > > 
> > > > 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 
> > > configur

Re: [DISCUSSION] KIP-421: Support resolving externalized secrets in AbstractConfig

[Tejal Adsul]

gt; > > >> 
> > > > > 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 > 
> > > wrote:> 
> > > >> 
> > > > > On Thu, Jan 24, 2019, at 11:25, TEJAL ADSUL wrote:> 
> > > > > >> 
> > > > > >> 
> > > > > > On 2019/01/24 17:26:02, Andy Coates  wrote:> 
> > > > > > > I'm wondering why we're rejected changing AbstractConfig to> 
> > > > > automatically> 
> > > > > > > resolve the variables?> 
> > > > > > >> 
> > > > > > > > 1. Change AbstractConfig to *automatically* resolve v

Re: [VOTE] KIP-421: Support resolving externalized secrets in AbstractConfig

[TEJAL ADSUL]

On 2019/01/24 18:54:13, Rajini Sivaram  wrote: 
> Hi Tejal,
> 
> Thanks for the KIP. I have a couple of comments/questions.
> 
> It sounds like we are addressing password protection for clients, Connect
> etc., but not for brokers, even though the changes proposed are in classes
> common to brokers and clients. It is ok for the scope to be limited, but we
> should explicitly state this in the KIP since the use of the new
> constructor is not compatible with dynamic broker configs. It will also be
> good to think about how we can support this feature for brokers in future
> (or mention that this change is never intended for brokers).
> 
> Also, it wasn't clear to me from the KIP if this is a Connect-specific
> change since it wasn't obvious how it would be used in other clients, e.g.
> a KafkaConsumer. It will be useful to clarify that too.
> 
> Regards,
> 
> Rajini
> 
> 
> On Thu, Jan 24, 2019 at 5:26 PM Andy Coates  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.
> > 
> > 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/
> > 
> > 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 seamlessly /
> > transparently, because we're likely going to have this functionality for a
> > long time.
> >
> > Andy
> >
> >
> >
> >
> > On Tue, 22 Jan 2019 at 17:38, te...@confluent.io 
> > wrote:
> >
> > > Hi all,
> > >
> > > We would like to start vote on KIP-421 to to enhance the AbstractConfig
> > > base class to support replacing variables in configurations just prior to
> > > parsing and validation.
> > >
> > > Link for the KIP:
> > >
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-421%3A+Support+resolving+externalized+secrets+in+AbstractConfig
> > >
> > >
> > > Thanks,
> > > Tejal
> > >
> >
> Hi Rajini,

With this approach currently we wont be supporting dynamic broker configs I 
will update that in the limitations. 

Its not just connect specific but applicable to other cp components as well as 
streams, connect etc. 

Thanks,
Tejal

Re: [VOTE] KIP-421: Support resolving externalized secrets in AbstractConfig

[TEJAL ADSUL]

On 2019/01/24 17:26:02, Andy Coates  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
> 
> config.providers.file.
> 
> class=org.apache.kafka.connect.configs.FileConfigProvider
> 
> config.providers.file.param.path=
> 
> /mnt/secrets/passwords
> 
> foo.baz=/usr/temp/
> 
> 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
> 
>  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 seamlessly /
> transparently, because we're likely going to have this functionality for a
> long time.
> 
> Andy
> 
> 
> 
> 
> On Tue, 22 Jan 2019 at 17:38, te...@confluent.io  wrote:
> 
> > Hi all,
> >
> > We would like to start vote on KIP-421 to to enhance the AbstractConfig
> > base class to support replacing variables in configurations just prior to
> > parsing and validation.
> >
> > Link for the KIP:
> >
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-421%3A+Support+resolving+externalized+secrets+in+AbstractConfig
> >
> >
> > Thanks,
> > Tejal
> >
> Hi Andy,

So wanted to make sure that we come up with a simple approach with no side 
effects or additional changes to any components. The rejected approach would 
require a change in Connect's behavior and we dint want to make that for this 
approach. 

also regarding Point 1. yes thats exactly the expected behavior of 
loadConfigProviders, we will send a file to it and it will create the instances 
of the configProvider which will be consumed by the constructor.

Thanks,
Tejal

Re: [VOTE] KIP-421: Support resolving externalized secrets in AbstractConfig

[TEJAL ADSUL]

Hi Colin,

If the configProvider is not configured we will return the unresolved value as 
is, so from the example the value returned will be 
${file:/path/to/variables.properties:foo.bar} instead of the resolved value.

We currently wont be supporting mechanism to update via subscribe so it will be 
 snapshot of what the configuration key was when the constructor was invoked. 

Thanks,
Tejal

On 2019/01/23 22:09:02, "Colin McCabe"  wrote: 
> Hi Tejal,
> 
> Looks good overall.
> 
> Can you please add this KIP to the wiki page at 
> https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals 
> ? 
> 
> What is the behavior when there is a configuration key which specifies a 
> ConfigurationProvider which has not been supplied?  For example, if I have a 
> map with a key "foo.bar" which has a value 
> "${file:/path/to/variables.properties:foo.bar}", but no file provider is 
> configured, what will happen?
> 
> KIP-297 includes a mechanism for callers to "subscribe" to changes to a 
> configuration key.  If such a change happens here, are we going to change the 
> value of the key in the AbstractConfig?  Or will it continue to be a snapshot 
> of what the configuration key was when the constructor was invoked?
> 
> best,
> Colin
> 
> On Wed, Jan 23, 2019, at 04:21, Dongjin Lee wrote:
> > +1 (non-binding)
> > 
> > Best,
> > Dongjin
> > 
> > On Wed, Jan 23, 2019 at 2:54 AM Ryanne Dolan  wrote:
> > 
> > > +1 non-binding, thanks!
> > >
> > > Ryanne
> > >
> > > On Tue, Jan 22, 2019 at 11:38 AM te...@confluent.io 
> > > wrote:
> > > >
> > > > Hi all,
> > > >
> > > > We would like to start vote on KIP-421 to to enhance the AbstractConfig
> > > base class to support replacing variables in configurations just prior to
> > > parsing and validation.
> > > >
> > > > Link for the KIP:
> > > >
> > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-421%3A+Support+resolving+externalized+secrets+in+AbstractConfig
> > > >
> > > >
> > > > Thanks,
> > > > Tejal
> > >
> > -- 
> > *Dongjin Lee*
> > 
> > *A hitchhiker in the mathematical world.*
> > *github:  github.com/dongjinleekr
> > linkedin: kr.linkedin.com/in/dongjinleekr
> > speakerdeck: 
> > speakerdeck.com/dongjin
> > *
> >
> 

[jira] [Created] (KAFKA-7847) KIP-421: Support resolving externalized secrets in AbstractConfig

[TEJAL ADSUL (JIRA)]

TEJAL ADSUL created KAFKA-7847:
--

 Summary: KIP-421: Support resolving externalized secrets in 
AbstractConfig
 Key: KAFKA-7847
 URL: https://issues.apache.org/jira/browse/KAFKA-7847
 Project: Kafka
  Issue Type: Improvement
  Components: config
Reporter: TEJAL ADSUL


This proposal intends to enhance the AbstractConfig base class to support 
replacing variables in configurations just prior to parsing and validation. 
This simple change will make it very easy for client applications, Kafka 
Connect, and Kafka Streams to use shared code to easily incorporate 
externalized secrets and other variable replacements within their 
configurations. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Created] (KAFKA-7846) KIP-421: Support resolving externalized secrets in AbstractConfig

[TEJAL ADSUL (JIRA)]

TEJAL ADSUL created KAFKA-7846:
--

 Summary: KIP-421: Support resolving externalized secrets in 
AbstractConfig
 Key: KAFKA-7846
 URL: https://issues.apache.org/jira/browse/KAFKA-7846
 Project: Kafka
  Issue Type: Improvement
  Components: config
Reporter: TEJAL ADSUL


This proposal intends to enhance the AbstractConfig base class to support 
replacing variables in configurations just prior to parsing and validation. 
This simple change will make it very easy for client applications, Kafka 
Connect, and Kafka Streams to use shared code to easily incorporate 
externalized secrets and other variable replacements within their 
configurations. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Permissions to create KIP

[Tejal Adsul]

Hi,

I work at Confluent. Please could you grant me permission to create a KIP for 
apache kafka, I wanted to propose a change to AbstractConfig in Kafka.
Following are my details
Full NameTEJAL ADSUL
emailte...@confluent.io
ID: tejal

Thanks,
Tejal