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

[Colin McCabe]

I'm not sure if this is ready for a vote yet.  In particular, I don't 
understand how it will work in the broker.  Having external secrets in the 
broker is something that a lot of people have been asking for -- it seems like 
a big omission to not talk about it at all in this KIP.

I also don't understand why we are not considering the case where configuration 
changes over time.  This was certainly one of the big motivating use-cases of 
KIP-297.  A lot of our software runs for a long period of time (like a streams 
job that might run indefinitely, or the broker, etc. etc.)  We want to be able 
to handle configuration changes transparently.

I'm concerned that we might be adding technical debt by creating an interface 
that doesn't support dynamic configuration changes.  It would be nice to think 
a little bit more about how this should work and support the use-cases people 
want.

best,
Colin


On Thu, Mar 21, 2019, at 13:49, TEJAL ADSUL wrote:
> 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
>

[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: [VOTE] KIP-421: Support resolving externalized secrets in AbstractConfig

[Colin McCabe]

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=$
> 
> {file:/path/to/secrets.properties:vault.secret.password}" then 
> getRawValue
> would always return "$
> 
> {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 
> 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 
> > 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 

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

[Andy Coates]

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

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

{file:/path/to/secrets.properties:vault.secret.password}" then getRawValue
would always return "$

{file:/path/to/secrets.properties:vault.secret.password}". I can see this
being useful.

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

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

[Rajini Sivaram]

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

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

[Ewen Cheslack-Postava]

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

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

[Colin McCabe]

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

I think Andy and Rajini bring up a good point.  If this change is limited to 
just Connect, then it's not completely clear why it needs to be in 
AbstractConfig.  On the other hand, if it applies to brokers and clients (and 
other things), then we should figure out how that integration will look.

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

It seems like it should be possible to keep Connect's behavior the same as it 
is now, but add automatic external configuration lookup to the Kafka broker.  
In order to do this, we could have an additional parameter that was set by the 
broker but not by Connect.

One candidate is we could have a Java parameter which describes which config 
key to look at to find the config providers.  Then the broker could set this, 
but connect could leave it unset.  Then people using the broker could describe 
their config providers in the configuration file itself, and connect users 
could do something different if desired.

best,
Colin

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

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

[Rajini Sivaram]

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

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

[Andy Coates]

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
>

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

[Colin McCabe]

On Wed, Jan 23, 2019, at 17:22, TEJAL ADSUL wrote:
> 
> 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.

OK.  This is consistent with the original KIP-297 behavior, right?  It would be 
good to mention it somewhere in the KIP.

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

OK.  Can you add this information to the KIP?  Perhaps mention it in "rejected 
alternatives" or something.

+1 (binding)

regards,
Colin

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

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

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

[Colin McCabe]

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

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

[Dongjin Lee]

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

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

[Ryanne Dolan]

+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

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

[tejal]

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