> 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 <cmcc...@apache.org> wrote: > On Thu, Jan 24, 2019, at 11:25, TEJAL ADSUL wrote: > > > > > > On 2019/01/24 17:26:02, 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, > > 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 > > >
