Hi All, I have updated the KIP to address the comments in the discussion. I have added the flow as to how dynamic config values will be resolved. Please could you’ll review the updated changes and let me know your feedback.
Thanks, Tejal On 2019/03/21 20:36:24, Tejal Adsul <te...@confluent.io> wrote: > @Colin we won’t be supporting the subscriber mode currently and it will be > added as a future work > 2. By disabling the feature the constructor will work as it earlier. If we > know the configs don’t have any indirect values or we want the indirect > values to remain unresolved we will can just do so by using the enable flag. > We won’t be using this in broker as its dynamic and we have added it in > future section for now. > > On 2019/03/14 16:36:42, "Colin McCabe" <c...@apache.org> wrote: > > Hi Tejal,> > > > > Thanks for the update.> > > > > One of the critical parts of the ConfigProvider interface is the ability to > > monitor changes to a configuration key through ConfigProvider#subscribe and > > ConfigProvider#unsubscribe, etc. I don't see how the proposed API supports > > this. Can you clarify?> > > > > Also, it's not clear to me when you would want to enable KIP-421 > > functionality and when you would want to disable it. What is the purpose > > of making it possible to disable this? Do you have examples of cases where > > we would use it and cases where we would not? Would the broker use this > > functionality?> > > > > best,> > > Colin> > > > > > > On Mon, Mar 11, 2019, at 10:49, Tejal Adsul wrote:> > > > Hi Folks,> > > > > > > > I have accommodated most of the review comments for > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-421%3A+Support+resolving+externalized+secrets+in+AbstractConfig > > > . Reopening the thread for further discussion. Please let me know your > > > thoughts on it.> > > > > > > > Thanks,> > > > Tejal> > > > > > > > On 2019/01/25 19:11:07, "Colin McCabe" <c....@apache.org> wrote: > > > > > 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=$> > > > > > > <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.> > > > > > > > > > > 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 <ra...@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 > > > > > > <ew...@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 <cm...@apache.org>> > > > > > > > > > > > > > > wrote:> > > > > > > > >> > > > > > > > > > On Thu, Jan 24, 2019, at 11:25, TEJAL ADSUL wrote:> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > > On 2019/01/24 17:26:02, Andy Coates <an...@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> > > > > [message truncated...]
