kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tejal Adsul <te...@confluent.io>
Subject Re: [DISCUSSION] KIP-421: Support resolving externalized secrets in AbstractConfig
Date Mon, 11 Mar 2019 17:49:03 GMT
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, tejal@confluent.io <>

> > > tejal@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> 
> > > > > >> 
> > > > >> 
> > > >> 
> > >> 
> >> 
> 
Mime
View raw message