Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640813005:
Fold config service URL into datastore cache keys. (Closed)
DescriptionFold config service URL into datastore cache keys.
Currently, the config cache does not differentiate between cache entries
from different configuration services. Add the configuration service URL
so that, in case someone configures multiple configuration sources, they
don't cross-talk.
BUG=chromium:682465
TEST=unit
Review-Url: https://codereview.chromium.org/2640813005
Committed: https://github.com/luci/luci-go/commit/8807f3cf1eca13c363f1f53443daa657ccf694b2
Patch Set 1 #
Total comments: 3
Patch Set 2 : Infer config service URL from various formats. #Patch Set 3 : Rebase. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 20 (4 generated)
dnj@chromium.org changed reviewers: + iannucci@chromium.org, vadimsh@chromium.org
PTAL
https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... File luci_config/server/cfgclient/backend/client/client.go (right): https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... luci_config/server/cfgclient/backend/client/client.go:108: IsCompleteEndpointURL bool ew... Can we make this non-configurable and just pick one?
https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... File luci_config/server/cfgclient/backend/client/client.go (right): https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... luci_config/server/cfgclient/backend/client/client.go:108: IsCompleteEndpointURL bool On 2017/01/19 02:08:05, iannucci wrote: > ew... Can we make this non-configurable and just pick one? ATM not really. The "settings" accepts the base URL, while the GetServiceURL call from the config service returns the full URL (including the endpoint). I need to be able to plug the output of either into this based on where the value is coming from.
https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... File luci_config/server/cfgclient/backend/client/client.go (right): https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... luci_config/server/cfgclient/backend/client/client.go:108: IsCompleteEndpointURL bool On 2017/01/19 02:19:29, dnj wrote: > On 2017/01/19 02:08:05, iannucci wrote: > > ew... Can we make this non-configurable and just pick one? > > ATM not really. The "settings" accepts the base URL, while the GetServiceURL > call from the config service returns the full URL (including the endpoint). I > need to be able to plug the output of either into this based on where the value > is coming from. Can't we just have the settings page immediately convert it to the full URL so there's no discrepancy?
On 2017/01/19 02:21:01, iannucci wrote: > https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... > File luci_config/server/cfgclient/backend/client/client.go (right): > > https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... > luci_config/server/cfgclient/backend/client/client.go:108: IsCompleteEndpointURL > bool > On 2017/01/19 02:19:29, dnj wrote: > > On 2017/01/19 02:08:05, iannucci wrote: > > > ew... Can we make this non-configurable and just pick one? > > > > ATM not really. The "settings" accepts the base URL, while the GetServiceURL > > call from the config service returns the full URL (including the endpoint). I > > need to be able to plug the output of either into this based on where the > value > > is coming from. > > Can't we just have the settings page immediately convert it to the full URL so > there's no discrepancy? We'd have to have logic to do that conversion, since the settings page will have to deal with values installed in datastore. Really, if we want to make that sort of change, we should probably just convert this to the luci-config "host" and make the rest of the URL generated. However, this is larger than it seems: - LogDog RPC-to-microservice API. - common/config interface API. - Existing gaeconfig settings in datastore - gaeconfig package - Maybe other places. Maybe a better option would be to parse the value as a URL and say, "if there is no Path, default to adding the _ah/...". If there is no scheme, use HTTPS. This would let existing configs work, and also let users override with full URL or local URL if necessary at the expense of a bit of magic. WDYT?
On 2017/01/19 02:33:22, dnj wrote: > On 2017/01/19 02:21:01, iannucci wrote: > > > https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... > > File luci_config/server/cfgclient/backend/client/client.go (right): > > > > > https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... > > luci_config/server/cfgclient/backend/client/client.go:108: > IsCompleteEndpointURL > > bool > > On 2017/01/19 02:19:29, dnj wrote: > > > On 2017/01/19 02:08:05, iannucci wrote: > > > > ew... Can we make this non-configurable and just pick one? > > > > > > ATM not really. The "settings" accepts the base URL, while the GetServiceURL > > > call from the config service returns the full URL (including the endpoint). > I > > > need to be able to plug the output of either into this based on where the > > value > > > is coming from. > > > > Can't we just have the settings page immediately convert it to the full URL so > > there's no discrepancy? > > We'd have to have logic to do that conversion, since the settings page will have > to deal with values installed in datastore. > > Really, if we want to make that sort of change, we should probably just convert > this to the luci-config "host" and make the rest of the URL generated. However, > this is larger than it seems: > - LogDog RPC-to-microservice API. > - common/config interface API. > - Existing gaeconfig settings in datastore > - gaeconfig package > - Maybe other places. > > Maybe a better option would be to parse the value as a URL and say, "if there is > no Path, default to adding the _ah/...". If there is no scheme, use HTTPS. This > would let existing configs work, and also let users override with full URL or > local URL if necessary at the expense of a bit of magic. WDYT? Er... I was trying to make it less flexible and more explicit, rather than more flexible and implicit. It sounds like you're saying that this particular structure (and not the things that invoke it) must be flexible because clients using this structure store the value in a bunch of different ways?
On 2017/01/19 02:38:39, iannucci wrote: > On 2017/01/19 02:33:22, dnj wrote: > > On 2017/01/19 02:21:01, iannucci wrote: > > > > > > https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... > > > File luci_config/server/cfgclient/backend/client/client.go (right): > > > > > > > > > https://codereview.chromium.org/2640813005/diff/1/luci_config/server/cfgclien... > > > luci_config/server/cfgclient/backend/client/client.go:108: > > IsCompleteEndpointURL > > > bool > > > On 2017/01/19 02:19:29, dnj wrote: > > > > On 2017/01/19 02:08:05, iannucci wrote: > > > > > ew... Can we make this non-configurable and just pick one? > > > > > > > > ATM not really. The "settings" accepts the base URL, while the > GetServiceURL > > > > call from the config service returns the full URL (including the > endpoint). > > I > > > > need to be able to plug the output of either into this based on where the > > > value > > > > is coming from. > > > > > > Can't we just have the settings page immediately convert it to the full URL > so > > > there's no discrepancy? > > > > We'd have to have logic to do that conversion, since the settings page will > have > > to deal with values installed in datastore. > > > > Really, if we want to make that sort of change, we should probably just > convert > > this to the luci-config "host" and make the rest of the URL generated. > However, > > this is larger than it seems: > > - LogDog RPC-to-microservice API. > > - common/config interface API. > > - Existing gaeconfig settings in datastore > > - gaeconfig package > > - Maybe other places. > > > > Maybe a better option would be to parse the value as a URL and say, "if there > is > > no Path, default to adding the _ah/...". If there is no scheme, use HTTPS. > This > > would let existing configs work, and also let users override with full URL or > > local URL if necessary at the expense of a bit of magic. WDYT? > > Er... I was trying to make it less flexible and more explicit, rather than more > flexible and implicit. > > It sounds like you're saying that this particular structure (and not the things > that invoke it) must be flexible because clients using this structure store the > value in a bunch of different ways? In particular I think automatically adding the _ah stuff is dangerous: it means that this wouldn't be able to adapt to a webapp2-endpoints-style conversion, since _ah is reserved URL territory.
> > It sounds like you're saying that this particular structure (and not the > things > > that invoke it) must be flexible because clients using this structure store > the > > value in a bunch of different ways? Yep. > In particular I think automatically adding the _ah stuff is dangerous: it means > that this wouldn't be > able to adapt to a webapp2-endpoints-style conversion, since _ah is reserved URL > territory. Well, so the scheme that I proposed would handle this: > - LogDog RPC-to-microservice API. (full URL, will always work) > - common/config interface API (full URL, will always work) > - Existing gaeconfig settings in datastore (scheme/host, path will be automagically appended, if endpoint changes we can change the magic too transparently) > - gaeconfig package (currently same as previous) > - Maybe other places. (????) At the very last, introducing that scheme will make it so that we can start migrating other values over without them instantly breaking.
On 2017/01/19 02:43:57, dnj wrote: > > > It sounds like you're saying that this particular structure (and not the > > things > > > that invoke it) must be flexible because clients using this structure store > > the > > > value in a bunch of different ways? > > Yep. > > > In particular I think automatically adding the _ah stuff is dangerous: it > means > > that this wouldn't be > > able to adapt to a webapp2-endpoints-style conversion, since _ah is reserved > URL > > territory. > > Well, so the scheme that I proposed would handle this: > > - LogDog RPC-to-microservice API. (full URL, will always work) > > - common/config interface API (full URL, will always work) > > - Existing gaeconfig settings in datastore (scheme/host, path will be > automagically appended, if endpoint changes we can change the magic too > transparently) > > - gaeconfig package (currently same as previous) > > - Maybe other places. (????) > > At the very last, introducing that scheme will make it so that we can start > migrating other values over without them instantly breaking. What if we just log a warning if we hit the 'not-a-full-URL' path here. Later we can upgrade it to an error (should show up in alerts somewhere, I'd hope), and then remove the functionality entirely (after fixing said errors).
On 2017/01/19 03:17:48, iannucci wrote: > On 2017/01/19 02:43:57, dnj wrote: > > > > It sounds like you're saying that this particular structure (and not the > > > things > > > > that invoke it) must be flexible because clients using this structure > store > > > the > > > > value in a bunch of different ways? > > > > Yep. > > > > > In particular I think automatically adding the _ah stuff is dangerous: it > > means > > > that this wouldn't be > > > able to adapt to a webapp2-endpoints-style conversion, since _ah is reserved > > URL > > > territory. > > > > Well, so the scheme that I proposed would handle this: > > > - LogDog RPC-to-microservice API. (full URL, will always work) > > > - common/config interface API (full URL, will always work) > > > - Existing gaeconfig settings in datastore (scheme/host, path will be > > automagically appended, if endpoint changes we can change the magic too > > transparently) > > > - gaeconfig package (currently same as previous) > > > - Maybe other places. (????) > > > > At the very last, introducing that scheme will make it so that we can start > > migrating other values over without them instantly breaking. > > What if we just log a warning if we hit the 'not-a-full-URL' path here. Later we > can upgrade it to an error (should show up in alerts somewhere, I'd hope), and > then remove the functionality entirely (after fixing said errors). That would make all current GAE instances log a warning, since the current "gaeconfig" settings have the partual value: "https://luci-config.appspot.com". We know the problem and we can grep for usage some other time. I just don't think this CL is the place to upgrade everyone's usage of this field, or really deal with that problem, since it touches a lot of components.
On 2017/01/19 03:24:15, dnj wrote: > On 2017/01/19 03:17:48, iannucci wrote: > > On 2017/01/19 02:43:57, dnj wrote: > > > > > It sounds like you're saying that this particular structure (and not the > > > > things > > > > > that invoke it) must be flexible because clients using this structure > > store > > > > the > > > > > value in a bunch of different ways? > > > > > > Yep. > > > > > > > In particular I think automatically adding the _ah stuff is dangerous: it > > > means > > > > that this wouldn't be > > > > able to adapt to a webapp2-endpoints-style conversion, since _ah is > reserved > > > URL > > > > territory. > > > > > > Well, so the scheme that I proposed would handle this: > > > > - LogDog RPC-to-microservice API. (full URL, will always work) > > > > - common/config interface API (full URL, will always work) > > > > - Existing gaeconfig settings in datastore (scheme/host, path will be > > > automagically appended, if endpoint changes we can change the magic too > > > transparently) > > > > - gaeconfig package (currently same as previous) > > > > - Maybe other places. (????) > > > > > > At the very last, introducing that scheme will make it so that we can start > > > migrating other values over without them instantly breaking. > > > > What if we just log a warning if we hit the 'not-a-full-URL' path here. Later > we > > can upgrade it to an error (should show up in alerts somewhere, I'd hope), and > > then remove the functionality entirely (after fixing said errors). > > That would make all current GAE instances log a warning, since the current > "gaeconfig" settings have the partual value: "https://luci-config.appspot.com". > > We know the problem and we can grep for usage some other time. I just don't > think this CL is the place to upgrade everyone's usage of this field, or really > deal with that problem, since it touches a lot of components. I think it IS the place to do that. This is going from one way of doing it to two-ways-of-doing-it-with-additional-configuration. It's completely appropriate, IMO, that when introducing a new way to do it you deprecate and/or fix the old way of doing it.
On 2017/01/19 03:27:43, iannucci wrote: > On 2017/01/19 03:24:15, dnj wrote: > > On 2017/01/19 03:17:48, iannucci wrote: > > > On 2017/01/19 02:43:57, dnj wrote: > > > > > > It sounds like you're saying that this particular structure (and not > the > > > > > things > > > > > > that invoke it) must be flexible because clients using this structure > > > store > > > > > the > > > > > > value in a bunch of different ways? > > > > > > > > Yep. > > > > > > > > > In particular I think automatically adding the _ah stuff is dangerous: > it > > > > means > > > > > that this wouldn't be > > > > > able to adapt to a webapp2-endpoints-style conversion, since _ah is > > reserved > > > > URL > > > > > territory. > > > > > > > > Well, so the scheme that I proposed would handle this: > > > > > - LogDog RPC-to-microservice API. (full URL, will always work) > > > > > - common/config interface API (full URL, will always work) > > > > > - Existing gaeconfig settings in datastore (scheme/host, path will be > > > > automagically appended, if endpoint changes we can change the magic too > > > > transparently) > > > > > - gaeconfig package (currently same as previous) > > > > > - Maybe other places. (????) > > > > > > > > At the very last, introducing that scheme will make it so that we can > start > > > > migrating other values over without them instantly breaking. > > > > > > What if we just log a warning if we hit the 'not-a-full-URL' path here. > Later > > we > > > can upgrade it to an error (should show up in alerts somewhere, I'd hope), > and > > > then remove the functionality entirely (after fixing said errors). > > > > That would make all current GAE instances log a warning, since the current > > "gaeconfig" settings have the partual value: > "https://luci-config.appspot.com". > > > > We know the problem and we can grep for usage some other time. I just don't > > think this CL is the place to upgrade everyone's usage of this field, or > really > > deal with that problem, since it touches a lot of components. > > I think it IS the place to do that. This is going from one way of doing it to > two-ways-of-doing-it-with-additional-configuration. It's completely appropriate, > IMO, that when introducing a new way to do it you deprecate and/or fix the old > way of doing it. It's a value that had one flavor of magic, and I'm proposing generalizing that magic. Alternatively we could make this package take full URL and have every other package know to append the "_ah/api/..." string crap. I don't think that's a particularly great idea, since we have a nice central place here to do that logic.
rebased
lgtm
The CQ bit was checked by dnj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484877781615660,
"parent_rev": "9f924447611c5e3c40f9347b40a9f01fc1ae76c1", "commit_rev":
"8807f3cf1eca13c363f1f53443daa657ccf694b2"}
Message was sent while issue was closed.
Description was changed from ========== Fold config service URL into datastore cache keys. Currently, the config cache does not differentiate between cache entries from different configuration services. Add the configuration service URL so that, in case someone configures multiple configuration sources, they don't cross-talk. BUG=chromium:682465 TEST=unit ========== to ========== Fold config service URL into datastore cache keys. Currently, the config cache does not differentiate between cache entries from different configuration services. Add the configuration service URL so that, in case someone configures multiple configuration sources, they don't cross-talk. BUG=chromium:682465 TEST=unit Review-Url: https://codereview.chromium.org/2640813005 Committed: https://github.com/luci/luci-go/commit/8807f3cf1eca13c363f1f53443daa657ccf694b2 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/luci/luci-go/commit/8807f3cf1eca13c363f1f53443daa657ccf694b2 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
