Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Router: Add support for templated sub-specifications #413

Closed
wants to merge 1 commit into from

Conversation

d00rman
Copy link
Contributor

@d00rman d00rman commented Nov 13, 2015

Our configuration file has become too long. Moreover, when something changes in it, we have to be careful to propagate them (correctly) into labs, staging, production and MW-Vagrant, making the (manual) process time-consuming and error-prone. This PR tries to minimise config change impacts by adding parts of the private routes (all usually contained in the config file) to specs/sys/v1. Concretely, Mathoid's and MobileApps' /sys/ routes have been moved there.

RESTBase's private routes usually connect it with a back-end service, whose IP changes depending on the environment it is being run in. Hence, this PR also introduces the Swig templating engine which is used to compile and render the subspecs found under the specs/ directory hierarchy. For each template, the complete service configuration is available (more precisely, services[0].conf as passed by service-runner is used).

@d00rman
Copy link
Contributor Author

d00rman commented Nov 13, 2015

cc @wikimedia/services

@gwicke
Copy link
Member

gwicke commented Nov 13, 2015

+:100: for the general direction! Yay for getting rid of all that ridiculous duplication. This will also let us get rid of some of the /sys/ entry points altogether.

My main worries are about the interaction of swig templates with our config files. The biggest concern is about escaping: We probably don't want swig's HTML escaping, but do need JS string literal encoding / escaping. The other concern is about possible conflicts between swig's syntax {{ var }}, and our own, very similar syntax. The latter might not matter so much in practice.

I was wondering if we could pass a $conf parameter into our template engine instead. That would avoid the escaping issues & possible syntax conflicts, but might be slightly more complex to implement.

Edit: Another thought might be to localize the configs a bit, so that we can specify the options pertaining to a spec right where it's pulled in, very similar to how we currently do this for modules. This would more clearly define the config interface for each module, and avoid littering the global config with all kinds of module-specific bits. It would also reflect that sub-specs are an alternative to modules.

@d00rman
Copy link
Contributor Author

d00rman commented Nov 13, 2015

We probably don't want swig's HTML escaping,

Correct, and we tell Swig not to do it.

but do need JS string literal encoding / escaping.

Not sure I see why. Templating is applied on start-up once, and variables will appear as specified in the config (in the exact same way that we do it now in prod with Puppet and ERB).

The other concern is about possible conflicts between swig's syntax {{ var }}, and our own, very similar syntax.

That does indeed look similar, but even if do make the mistake of using one instead of the other, it pretty likely the mistake will be made apparent pretty quickly. Alternatively, we might change Swig's delimiters to something like ERB's <% %>, which would give us config consistency with ops/puppet to some extent.

I was wondering if we could pass a $conf parameter into our template engine instead. That would avoid the escaping issues & possible syntax conflicts, but might be slightly more complex to implement.

I was wondering the same, but having static, one-time compilation and rendering seems like a good first step. The benefit here is that the templates are compiled and executed only once on start-up, so the net effect is that you have the exact same spec as before, hence no per-request additional overhead (even though, compared to the network request that is bound to happen after that is negligible).

Another thought might be to localize the configs a bit, so that we can specify the options pertaining to a spec right where it's pulled in, very similar to how we currently do this for modules. This would more clearly define the config interface for each module, and avoid littering the global config with all kinds of module-specific bits. It would also reflect that sub-specs are an alternative to modules.

Good point.

@gwicke
Copy link
Member

gwicke commented Nov 13, 2015

We probably don't want swig's HTML escaping,

Correct, and we tell Swig not to do it.

Ah, good. Missed that, sorry.

but do need JS string literal encoding / escaping.

Not sure I see why. Templating is applied on start-up once, and variables will appear as specified in the config (in the exact same way that we do it now in prod with Puppet and ERB).

Example:

uri: /some/{$.default($.request.params.foo, $conf.defaultFoo)}

Injecting $conf.defaultFoo with textual substitution ({{ $conf.defaultFoo }}) requires proper escaping / string encoding, so that it works out to a valid string constant in the expression. If we hook into the regular templating engine, we avoid the need for textual substitution.

@d00rman
Copy link
Contributor Author

d00rman commented Nov 13, 2015

Counter-example:

uri: {{ my_service_uri }}/path

Here we wouldn't want to escape chars, since my_service_uri might contain genuine paths. So, I think that escaping is something we'll have to deal with on per-case basis.

@gwicke
Copy link
Member

gwicke commented Nov 14, 2015

@d00rman, that shouldn't be an issue:

uri: {+ $conf.service_uri }/path

@gwicke
Copy link
Member

gwicke commented Nov 14, 2015

Another consideration is the ability to pass around more complex sub-structures, like entire objects or arrays. Doing that as strings might get quite ugly.

gwicke added a commit to gwicke/restbase that referenced this pull request Nov 14, 2015
This patch adds support for loading specs as a special kind of module. Similar
to @mobrovac's PR wikimedia#413, this makes our config more modular and less verbose.
As with other modules, spec modules can be parametrized with options.

Example config stanza:

```yaml
    /{module:graphoid}:
      x-modules:
        - path: graphoid_test
          type: spec
          options:
            host: http://graphoid.wikimedia.org
            header_whitelist:
              - content-type
              - x-foobar
```

The `options` specified for the module can be referenced in request templates
under `$$.conf`. Example:

```yaml
uri: '{+$$.conf.host}/{domain}/...'
headers: '{$$.filter($.request.headers, $$.conf.header_whitelist)}'
```
Note that we are passing an array to `$$.filter`, which works as we are
passing around references.
@gwicke gwicke mentioned this pull request Nov 14, 2015
@gwicke
Copy link
Member

gwicke commented Nov 14, 2015

@d00rman, I implemented a variant with module-like option support in #415.

gwicke added a commit to gwicke/restbase that referenced this pull request Nov 15, 2015
This patch adds support for loading specs as a special kind of module. Similar
to @mobrovac's PR wikimedia#413, this makes our config more modular and less verbose.
As with other modules, spec modules can be parametrized with options.

Example config stanza:

```yaml
    /{module:graphoid}:
      x-modules:
        - path: graphoid_test
          type: spec
          options:
            host: http://graphoid.wikimedia.org
            header_whitelist:
              - content-type
              - x-foobar
```

The `options` specified for the module can be referenced in request templates
under `$$.conf`. Example:

```yaml
uri: '{+$$.conf.host}/{domain}/...'
headers: '{$$.filter($.request.headers, $$.conf.header_whitelist)}'
```
Note that we are passing an array to `$$.filter`, which works as we are
passing around references.
Our configuration file has become too long. Moreover, when something
changes in it, we have to be careful to propagate them (correctly) into
labs, staging, production and MW-Vagrant, making the (manual) process
time-consuming and error-prone. This PR tries to minimise config change
impacts by adding parts of the private routes (all usually contained in
the config file) to specs/sys/v1. Concretely, Mathoid's and MobileApps'
/sys/ routes have been moved there.

RESTBase's private routes usually connect it with a back-end service,
whose IP changes depending on the environment it is being run in. Hence,
this PR also introduces the Swig templating engine which is used to
compile and render the subspecs found under the specs/ directory
hierarchy. For each template, the complete service configuration is
available (more precisely, services[0].conf from the service-runner
config is used).
gwicke added a commit to gwicke/restbase that referenced this pull request Nov 16, 2015
This patch adds support for loading specs as a special kind of module. Similar
to @mobrovac's PR wikimedia#413, this makes our config more modular and less verbose.
As with other modules, spec modules can be parametrized with options.

Example config stanza:

```yaml
    /{module:graphoid}:
      x-modules:
        - path: graphoid_test
          type: spec
          options:
            host: http://graphoid.wikimedia.org
            header_whitelist:
              - content-type
              - x-foobar
```

The `options` specified for the module can be referenced in request templates
under `$$.conf`. Example:

```yaml
uri: '{+$$.conf.host}/{domain}/...'
headers: '{$$.filter($.request.headers, $$.conf.header_whitelist)}'
```
Note that we are passing an array to `$$.filter`, which works as we are
passing around references.
gwicke added a commit to gwicke/restbase that referenced this pull request Nov 16, 2015
This patch adds support for loading specs as a special kind of module. Similar
to @mobrovac's PR wikimedia#413, this makes our config more modular and less verbose.
As with other modules, spec modules can be parametrized with options.

Example config stanza:

```yaml
    /{module:graphoid}:
      x-modules:
        - path: graphoid_test
          type: spec
          options:
            host: http://graphoid.wikimedia.org
            header_whitelist:
              - content-type
              - x-foobar
```

The `options` specified for the module can be referenced in request templates
under `$$.conf`. Example:

```yaml
uri: '{+$$.conf.host}/{domain}/...'
headers: '{$$.filter($.request.headers, $$.conf.header_whitelist)}'
```
Note that we are passing an array to `$$.filter`, which works as we are
passing around references.
@d00rman
Copy link
Contributor Author

d00rman commented Nov 17, 2015

Closing in favour of #415

@d00rman d00rman closed this Nov 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants