config services: services.cfg and validation

config services: services.cfg and validation

* Introduced services.cfg, a registry of services
* A service may expose a secure metadata endpoint that
  describes config patterns that the service may validate
* Removed validation.cfg: config service sends validation requests to
  other services based on their metadata
* Decreased Gitiles polling frequency to 1/10 min because of gitiles
  quota
* Added customizable service config_location in proto,
  but did not implement in gitiles_import.py yet

R=sergiyb@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-py/commit/e7059157d4d05ec55f8d1d9643a0f7edb52f23eb

[nodir, 2015-07-06 17:19:30]

PTAL. Recommended reviewing order:
1) CL description
2) README.md
3) config_service/proto/service_config.proto
4) components/components/config/validation.py
5) components/components/config/endpoint.py
6) config_service/validation.py
7) config_service/services.py

[nodir, 2015-07-06 17:22:36]

Patchset #4 (id:60001) has been deleted

[nodir, 2015-07-06 17:22:43]

Patchset #3 (id:40001) has been deleted

[nodir, 2015-07-06 17:22:48]

Patchset #1 (id:1) has been deleted

[nodir, 2015-07-06 17:22:54]

Patchset #1 (id:20001) has been deleted

[nodir, 2015-07-06 17:42:15]

cc:sergiyb

[Sergiy Byelozyorov, 2015-07-07 09:10:46]

sergiyb@chromium.org changed reviewers:
+ sergiyb@chromium.org

[Sergiy Byelozyorov, 2015-07-07 09:10:47]

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
File appengine/components/components/config/endpoint.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:8: * Validates configs.
TODO(nodir): implement.
This should probably be removed now.

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:53: def
is_trusted_requester():
please add a docstring explaining where this is used

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:73: class
ConfigPattern(messages.Message):
Why are these classes not generated via protoc from the .proto file?

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:133: """Describes a service.
Used by config service to discover other services.
Do I understand correctly that this is used by the config service to request
service metadata from some remote endpoint (e.g.
chromium-commit-queue.appspot.com/_ah/api/config/v1/metadata)? Why is this
endpoint defined on the config service itself? Or is this file intended to be
used as a template for endpoint implementation on those other sites?

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:139:
url='https://{hostname}/_ah/api/{name}/{version}/{path}validate'.format(
why does url end with 'validate'?

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:142:
version=self.api_info.version,
If this is a template for implementing an endpoint - what happens if the config
API version is upgraded and some other service is still using old version?

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
File appengine/components/components/config/validation.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/validation.py:272: "^"/"$", they are
added automatically.
please replace "^"/"$" with just ^/$, otherwise it is confusing where it is also
expected for each pattern to start/end with double quotes.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
File appengine/config_service/README.md (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/README.md:107: Config files are validated and invalid
ones are not imported. Configs of the
...and config sets containing invalid configs are not imported.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
File appengine/config_service/validation.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:58: if ':' not in pattern:
a lot of this code is duplicated in compile_pattern... why not just call from
here?

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:111: elif loc.storage_type ==
service_config_pb2.ConfigSetLocation.UNSET:
I would also add

elif not allow_relative_url and is_url_relative(loc.url):
  ctx.error('relative url is used, which is not allowed')

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:135: auth.Identity('user', email)
I think this is a misuse of the auth.Identity.__new__ constructor. Perhaps it is
better to extract validate_user_email as a static method.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
File appengine/config_service/validation_test.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation_test.py:288: path='regex:.+.\cfg',
regex is invalid, please correct: r'regex:.+\.cfg'
sorry for not noticing it last time

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation_test.py:297: path='regex:.+',
please also use raw string: r'regex:.+'

[nodir, 2015-07-07 15:54:18]

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
File appengine/components/components/config/endpoint.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:8: * Validates configs.
TODO(nodir): implement.
On 2015/07/07 09:10:46, Sergiy Byelozyorov wrote:
> This should probably be removed now.

Done.

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:53: def
is_trusted_requester():
On 2015/07/07 09:10:46, Sergiy Byelozyorov wrote:
> please add a docstring explaining where this is used

Done.

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:73: class
ConfigPattern(messages.Message):
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> Why are these classes not generated via protoc from the .proto file?

because Cloud Endpoints doesn't work with .proto. This is protorpc, a bit
different

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:133: """Describes a service.
Used by config service to discover other services.
On 2015/07/07 09:10:46, Sergiy Byelozyorov wrote:
> Do I understand correctly that this is used by the config service to request
> service metadata from some remote endpoint (e.g.
> chromium-commit-queue.appspot.com/_ah/api/config/v1/metadata)? 
Yes
> Why is this
> endpoint defined on the config service itself? 

This is not a config service. This is a config component, used in other apps,
such as buildbucket, CIA.

> Or is this file intended to be
> used as a template for endpoint implementation on those other sites?

It is used today:
https://apis-explorer.appspot.com/apis-explorer/?base=https://cr-buildbucket....
https://apis-explorer.appspot.com/apis-explorer/?base=https://chrome-infra-au...

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:139:
url='https://{hostname}/_ah/api/{name}/{version}/{path}validate'.format(
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> why does url end with 'validate'?

Because that's how the "validate" method in this class is called, and thus this
is the suffix for the HTTP endpoint for validation. This is a URL to an endpoint
that implements validation protocol

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:142:
version=self.api_info.version,
On 2015/07/07 09:10:46, Sergiy Byelozyorov wrote:
> If this is a template for implementing an endpoint - what happens if the
config
> API version is upgraded and some other service is still using old version?

I've included "version" attribute to the metadata protocol

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
File appengine/components/components/config/validation.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/validation.py:272: "^"/"$", they are
added automatically.
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> please replace "^"/"$" with just ^/$, otherwise it is confusing where it is
also
> expected for each pattern to start/end with double quotes.

Done.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
File appengine/config_service/README.md (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/README.md:107: Config files are validated and invalid
ones are not imported. Configs of the
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> ...and config sets containing invalid configs are not imported.

Done.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
File appengine/config_service/validation.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:58: if ':' not in pattern:
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> a lot of this code is duplicated in compile_pattern... why not just call from
> here?

Done.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:111: elif loc.storage_type ==
service_config_pb2.ConfigSetLocation.UNSET:
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> I would also add
> 
> elif not allow_relative_url and is_url_relative(loc.url):
>   ctx.error('relative url is used, which is not allowed')

gitiles.Location.parse ensures that the url is not relative
https://github.com/luci/luci-py/blob/master/appengine/components/components/g...

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:135: auth.Identity('user', email)
On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> I think this is a misuse of the auth.Identity.__new__ constructor. Perhaps it
is
> better to extract validate_user_email as a static method.

This is exactly what I did in a previous CL. Vadim suggested to reuse the
constructor and I agreed. I think it is fine: you suggested to call
compile_pattern in validate_pattern and one could say that is a misuse of
compile_pattern

[Sergiy Byelozyorov, 2015-07-07 23:04:38]

lgtm

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
File appengine/components/components/config/endpoint.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:133: """Describes a service.
Used by config service to discover other services.
On 2015/07/07 15:54:17, nodir wrote:
> On 2015/07/07 09:10:46, Sergiy Byelozyorov wrote:
> > Do I understand correctly that this is used by the config service to request
> > service metadata from some remote endpoint (e.g.
> > chromium-commit-queue.appspot.com/_ah/api/config/v1/metadata)? 
> Yes
> > Why is this
> > endpoint defined on the config service itself? 
> 
> This is not a config service. This is a config component, used in other apps,
> such as buildbucket, CIA.
> 
> > Or is this file intended to be
> > used as a template for endpoint implementation on those other sites?
> 
> It is used today:
>
https://apis-explorer.appspot.com/apis-explorer/?base=https://cr-buildbucket....
>
https://apis-explorer.appspot.com/apis-explorer/?base=https://chrome-infra-au...

Acknowledged.

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:139:
url='https://{hostname}/_ah/api/{name}/{version}/{path}validate'.format(
On 2015/07/07 15:54:17, nodir wrote:
> On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> > why does url end with 'validate'?
> 
> Because that's how the "validate" method in this class is called, and thus
this
> is the suffix for the HTTP endpoint for validation. This is a URL to an
endpoint
> that implements validation protocol

Acknowledged.

https://codereview.chromium.org/1221643020/diff/80001/appengine/components/co...
appengine/components/components/config/endpoint.py:142:
version=self.api_info.version,
On 2015/07/07 15:54:17, nodir wrote:
> On 2015/07/07 09:10:46, Sergiy Byelozyorov wrote:
> > If this is a template for implementing an endpoint - what happens if the
> config
> > API version is upgraded and some other service is still using old version?
> 
> I've included "version" attribute to the metadata protocol

Acknowledged.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
File appengine/config_service/validation.py (right):

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:111: elif loc.storage_type ==
service_config_pb2.ConfigSetLocation.UNSET:
On 2015/07/07 15:54:18, nodir wrote:
> On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> > I would also add
> > 
> > elif not allow_relative_url and is_url_relative(loc.url):
> >   ctx.error('relative url is used, which is not allowed')
> 
> gitiles.Location.parse ensures that the url is not relative
>
https://github.com/luci/luci-py/blob/master/appengine/components/components/g...

Acknowledged.

https://codereview.chromium.org/1221643020/diff/80001/appengine/config_servic...
appengine/config_service/validation.py:135: auth.Identity('user', email)
On 2015/07/07 15:54:18, nodir wrote:
> On 2015/07/07 09:10:47, Sergiy Byelozyorov wrote:
> > I think this is a misuse of the auth.Identity.__new__ constructor. Perhaps
it
> is
> > better to extract validate_user_email as a static method.
> 
> This is exactly what I did in a previous CL. Vadim suggested to reuse the
> constructor and I agreed. I think it is fine: you suggested to call
> compile_pattern in validate_pattern and one could say that is a misuse of
> compile_pattern

Acknowledged.

[Vadim Sh., 2015-07-08 03:05:59]

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
File appengine/components/components/config/endpoint.py (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
appengine/components/components/config/endpoint.py:55: def
is_trusted_requester():
I propose to reduce magicallity and store trusted_config_account in
common.ConfigSettings explicitly (as "user:abc@gserviceaccount.com"). That way
you don't need to mention googleplex; it works for non-default versions; it
works on local dev server (e.g. one can put 'anonymous:anonymous' there or
whatever).

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
appengine/components/components/config/endpoint.py:89: """Compatible with config
service's ServiceDynamicMetadata messsage."""
Describe exactly what you mean by "compatible", e.g. "If you add a field to
*.proto, add exact same field here too" (IIUC).

Also it might be better to name it exactly as proto message class.

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
appengine/components/components/config/endpoint.py:95: version =
messages.StringField(1, required=True)
Is it needed? Proto-flavored formats are supposed to be eternally backward
compatible (until API version switches at least).

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
File appengine/config_service/README.md (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/README.md:5: - Stores a registry of LUCI services.
"LUCI services that use config_service"?

auth_service also stores "a registry of LUCI services" in some way... There's no
real "registry".

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
File appengine/config_service/proto/service_config.proto (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/proto/service_config.proto:69: // Machine-generated
service metadata.
nit: Service metadata reported by the services itself via such-and-such
endpoints protocol.

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
File appengine/config_service/services.py (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:37: services = yield get_services()
I don't think get_services is defined. How did this pass unit tests? ;)

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:43: def _dict_to_dynamic_metadata(data):
is it the only place where proto message and protorpc message meet? Mention it
somewhere in docs for ServiceDynamicMetadata (or protorpc message), e.g. end
goal is for doc strings to describe how to easily add a new field if needed.

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:66: DynamicMetadataError if metadata
endpoint response is bad.
also ValueError, make it DynamicMetadataError too?

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:76: res =
net.json_request_async(service.metadata_url)
need auth here? I think yes, because validation configs leak info about
structure of configs.

[nodir, 2015-07-08 15:24:03]

Patchset #5 (id:160001) has been deleted

[nodir, 2015-07-08 15:24:29]

Patchset #5 (id:180001) has been deleted

[nodir, 2015-07-08 15:24:47]

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
File appengine/components/components/config/endpoint.py (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
appengine/components/components/config/endpoint.py:55: def
is_trusted_requester():
On 2015/07/08 03:05:58, Vadim Sh. wrote:
> I propose to reduce magicallity and store trusted_config_account in
> common.ConfigSettings explicitly (as "user:abc@gserviceaccount.com"). That way
> you don't need to mention googleplex; it works for non-default versions; it
> works on local dev server (e.g. one can put 'anonymous:anonymous' there or
> whatever).

Done.

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
appengine/components/components/config/endpoint.py:89: """Compatible with config
service's ServiceDynamicMetadata messsage."""
On 2015/07/08 03:05:58, Vadim Sh. wrote:
> Describe exactly what you mean by "compatible", e.g. "If you add a field to
> *.proto, add exact same field here too" (IIUC).
> 
> Also it might be better to name it exactly as proto message class.

Done.

https://codereview.chromium.org/1221643020/diff/140001/appengine/components/c...
appengine/components/components/config/endpoint.py:95: version =
messages.StringField(1, required=True)
On 2015/07/08 03:05:58, Vadim Sh. wrote:
> Is it needed? Proto-flavored formats are supposed to be eternally backward
> compatible (until API version switches at least).

config services talks to different services that may have different versions of
metadata formats. The API version is specified implicitly, as a part of URL in
services.cfg, so API versioning does not really work in this case. Finally, a
metadata endpoint does not have to be a cloud endpoints thing

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
File appengine/config_service/README.md (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/README.md:5: - Stores a registry of LUCI services.
On 2015/07/08 03:05:58, Vadim Sh. wrote:
> "LUCI services that use config_service"?
> 
> auth_service also stores "a registry of LUCI services" in some way... There's
no
> real "registry".

services.cfg is meant to be the registry. 
Also see
https://docs.google.com/document/d/1EW3hwi9GZiJAxwpYbhFPopQCzXFabZeMWJuBsj7vb...

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
File appengine/config_service/proto/service_config.proto (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/proto/service_config.proto:69: // Machine-generated
service metadata.
On 2015/07/08 03:05:58, Vadim Sh. wrote:
> nit: Service metadata reported by the services itself via such-and-such
> endpoints protocol.

Done.

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
File appengine/config_service/services.py (right):

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:37: services = yield get_services()
On 2015/07/08 03:05:58, Vadim Sh. wrote:
> I don't think get_services is defined. How did this pass unit tests? ;)

Yeah, this file needed a test. Fixed other bugs

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:43: def _dict_to_dynamic_metadata(data):
On 2015/07/08 03:05:59, Vadim Sh. wrote:
> is it the only place where proto message and protorpc message meet? Mention it
> somewhere in docs for ServiceDynamicMetadata (or protorpc message), e.g. end
> goal is for doc strings to describe how to easily add a new field if needed.

Done.

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:66: DynamicMetadataError if metadata
endpoint response is bad.
On 2015/07/08 03:05:59, Vadim Sh. wrote:
> also ValueError, make it DynamicMetadataError too?

Done.

https://codereview.chromium.org/1221643020/diff/140001/appengine/config_servi...
appengine/config_service/services.py:76: res =
net.json_request_async(service.metadata_url)
On 2015/07/08 03:05:59, Vadim Sh. wrote:
> need auth here? I think yes, because validation configs leak info about
> structure of configs.

Done.

[Vadim Sh., 2015-07-08 16:14:28]

lgtm

[nodir, 2015-07-08 17:11:19]

Committed patchset #5 (id:200001) manually as
e7059157d4d05ec55f8d1d9643a0f7edb52f23eb (presubmit successful).