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