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