On 2019/01/24 18:54:13, Rajini Sivaram <rajinisiva...@gmail.com> 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 <a...@confluent.io> wrote:
>
> > I'm wondering why we're rejected changing AbstractConfig to automatically
> > resolve the variables?
> >
> > > 1. Change AbstractConfig to *automatically* resolve variables of the form
> > specified in KIP-297. This was rejected because it would change the
> > behavior of existing code and might cause unexpected effects.
> >
> > Doing so seems to me to have two very large benefits:
> >
> > 1. It allows the config providers to be defined within the same file as the
> > config that uses the providers, e.g.
> >
> > config.providers=file,vault
> > <
> > https://cwiki.apache.org/confluence/display/KAFKA/config.providers=file,vault
> > >
> > config.providers.file.
> > <https://cwiki.apache.org/confluence/display/KAFKA/config.providers.file.>
> > class=org.apache.kafka.connect.configs.FileConfigProvider
> > <
> > https://cwiki.apache.org/confluence/display/KAFKA/org.apache.kafka.connect.configs.FileConfigProvider
> > >
> > config.providers.file.param.path=
> > <
> > https://cwiki.apache.org/confluence/display/KAFKA/config.providers.file.other.prop=another
> > >
> > /mnt/secrets/passwords
> >
> > foo.baz=/usr/temp/
> > <https://cwiki.apache.org/confluence/display/KAFKA/foo.baz=/usr/temp/>
> > foo.bar=$ <https://cwiki.apache.org/confluence/display/KAFKA/foo.bar=$>
> > {file:/path/to/variables.properties:foo.bar}
> >
> > Is this possible with what's currently being proposed? i.e could you load
> > the file and pass the map first to `loadConfigProviders` and then again to
> > the constructor?
> >
> > 2. It allows _all_ existing clients of the class, e.g. those in Apache
> > Kafka or in applications written by other people that use the class, to get
> > this functionality for free, i.e. without any code changes. (I realize
> > this is probably where the 'unexpected effects' comes from).
> >
> > I'm assuming the unexpected side effects come about if an existing
> > properties file already contains compatible config.providers
> > <
> > https://cwiki.apache.org/confluence/display/KAFKA/config.providers=file,vault
> > >
> > entries _and_ has other properties in the form ${xx:yy} or ${xx:yy:zz}.
> > While possible, these seems fairly unlikely unless for random client
> > property files. So I'm assuming there's a specific instance where we think
> > this is likely? Something to do with Connect config maybe?
> >
> > Personally, I think we should do our best to make this work 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 <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
