> 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=$ <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. 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 <rajinisiva...@gmail.com> 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 <e...@confluent.io> > 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 <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 > > > > > > > > > >
