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

REST Services v2 #2727

Merged
merged 58 commits into from Mar 23, 2017

Conversation

Projects
None yet
5 participants
@enykeev
Copy link
Member

enykeev commented Jun 2, 2016

As I said in #2686, refactoring st2api and starting to work on API v2 is essentially two different tasks that might be done at the same time, but doesn't have to. This PR shows an example of how we can make our web services easier to understand and to work with.

As we continue to add more different endpoints to the API, we figured our current approach of using @jsexpose decorator is "too magical" and requires us to dive deep into pecan code to figure out what's going on.

Web frameworks (pecan, flask, any other really) are full of features to appease a wide variety of users with even wider variety of needs (templating, out the box caching solutions, static serving, logging support, you name it). We've already figured out what exactly do we need and mostly reimplemented it on top of pecan anyway.

At the same time, we've reached the point where we want to stabilize our API and make it easier for both us and our users to work with it. We want to have a single place we can define our API and then generate routing table, docs, clients and tests from it.

Instead of monolithic framework, I propose to use a number of smaller single purpose libraries and build our router and controller around them. In this case, I've replaced pecan with three libraries:

  • WebOb provides a number of user-friendly utility functions to work with HTTP requests and responses. For example, it allows you to access json-encoded request data using req.json() instead of parsing it manually from WSGI environ object. It also allows you to construct the response and only call start_response at the last possible moment.
  • Routes handles URL matching and templating allowing us to comply with OpenAPI spec regarding path templating without diving deep into regex replacements.
  • swagger-spec-validator handles spec validation and helps us spot an error in our spec during initialization

Some features that are missing in this implementation:

  • Versioning. As I see it, we should have a separate spec.yml for every version with basePath field defining a proper prefix. Default (non-prefixed) version should be defined by router argument.
  • Request parameter validation and type casting. Parameter spec is JSONSchema-complaint so it's pretty straightforward. We also have a code for type casting already, but I have to admit, it was one of this confusing parts of jsexpose so maybe we should just do it inside controller instead.
  • Response validation. OpenAPI has a section defining possible responses for the endpoint. The question is how should we react when this validation had failed. One hand, we're breaking our own convention and returning an answer user don't expect to get. On the other, all the work that should have been done during the request is already done and returning an error to the user only hides the results of this work.
  • Using spec definitions as API models. They are serving the same purpose, really.
  • Unit and integration tests based on spec. We have schemes for both request and response parameters. Generating request from the first and matching the response with the second makes a very cheap test which can later be extended with more complex logic when it makes sense.
  • Automatic reloading for spec and\or controller code. Since the service is much more decoupled now, we can reload controller modules and specs without reloading the whole service. Productivity impact will depend on personal preferences, though.
  • Expose spec files on a separate endpoint. st2web and st2client may became much more flexible if they will be able to determine API models and endpoints on the fly instead of relying on hardcoded values. If we put an effort to it, we can really reduce the number of places you need to make a change to implement particular feature.

@enykeev enykeev added WIP RFR labels Jun 2, 2016

@enykeev

This comment has been minimized.

Copy link
Member

enykeev commented Jun 2, 2016

As you see, I've tried flask and connexion first but figured you can't redefine error reporting in this case.

With this approach, we're not only having smaller codebase while keeping full control, we can also transition our services to the new spec\router one controller at a time. And if we are willing to play safe, it won't be too hard to ensure responses for existing API won't change at all.

@manasdk

This comment has been minimized.

Copy link
Collaborator

manasdk commented Jun 2, 2016

I like the overall approach. It is very clear and does not involve a great deal of magic :)

It is a shame that we would need to write st2common/st2common/router.py but perhaps that is the cost of picking specific libraries.

@lakshmi-kannan

This comment has been minimized.

Copy link
Contributor

lakshmi-kannan commented Jun 2, 2016

TBH, I am unable to make up my mind. The custom 3 piece thing seems to work but I am unsure what issues we'll hit when we migrate all the endpoints. The problem is we won't know unless we do it. One thing though is libraries like routes aren't super active in terms of contributions as opposed to things like flask. So even if flask seems like excessive or is missing out on some features, it seems more battle tested.

@enykeev

This comment has been minimized.

Copy link
Member

enykeev commented Jun 3, 2016

As for being battle tested, both WebOb and routes are used by reddit and are also a parts of other frameworks (pecan uses webob and routes is a part of pylons) so they are getting plenty.

The problem is we won't know unless we do it.

Yep, we won't. But we can mitigate the risk by transitioning one controller at a time, first checking the request against controllers that have already been rewritten and then falling back to pecan if it could not be resolved there. There is nothing in Pecan that we can't do ourselves inside a controller or as a middleware on top of it, but even if we find something complex enough, it won't invalidate all the effort we already put into the transition.

@enykeev enykeev force-pushed the feature/rest_services_v2 branch from 2039858 to 44a3bd5 Jun 28, 2016

@enykeev enykeev referenced this pull request Sep 26, 2016

Merged

Extend auth to secure chatops #2920

3 of 6 tasks complete

@enykeev enykeev force-pushed the feature/rest_services_v2 branch from 40809b2 to 1791d8b Jan 20, 2017

@enykeev

This comment has been minimized.

Copy link
Member

enykeev commented Jan 20, 2017

I've force-pushed changes from feature/openapi here to avoid creating duplicate PR. This is how test coverage looks at the moment:

(virtualenv)vagrant@arkham:/dock/st2$ for file in st2api/tests/unit/controllers/v1/test_*.py; do nosetests $file > /dev/null 2>&1 && echo $file ok || echo $file fail; done
st2api/tests/unit/controllers/v1/test_action_alias.py fail
st2api/tests/unit/controllers/v1/test_actions.py ok
st2api/tests/unit/controllers/v1/test_actions_rbac.py fail
st2api/tests/unit/controllers/v1/test_action_views.py ok
st2api/tests/unit/controllers/v1/test_alias_execution.py fail
st2api/tests/unit/controllers/v1/test_alias_execution_rbac.py fail
st2api/tests/unit/controllers/v1/test_auth_api_keys.py ok
st2api/tests/unit/controllers/v1/test_auth.py fail
st2api/tests/unit/controllers/v1/test_base.py fail
st2api/tests/unit/controllers/v1/test_executions_filters.py ok
st2api/tests/unit/controllers/v1/test_executions_simple.py ok
st2api/tests/unit/controllers/v1/test_jsexpose_decorator.py fail
st2api/tests/unit/controllers/v1/test_kvps.py fail
st2api/tests/unit/controllers/v1/test_kvps_rbac.py fail
st2api/tests/unit/controllers/v1/test_pack_config_schema.py fail
st2api/tests/unit/controllers/v1/test_pack_configs.py fail
st2api/tests/unit/controllers/v1/test_packs.py fail
st2api/tests/unit/controllers/v1/test_packs_views.py fail
st2api/tests/unit/controllers/v1/test_policies.py fail
st2api/tests/unit/controllers/v1/test_rbac_for_supported_endpoints.py fail
st2api/tests/unit/controllers/v1/test_rule_enforcements.py fail
st2api/tests/unit/controllers/v1/test_rules.py ok
st2api/tests/unit/controllers/v1/test_rules_rbac.py fail
st2api/tests/unit/controllers/v1/test_rule_views.py ok
st2api/tests/unit/controllers/v1/test_runnertypes.py ok
st2api/tests/unit/controllers/v1/test_runnertypes_rbac.py fail
st2api/tests/unit/controllers/v1/test_sensortypes.py ok
st2api/tests/unit/controllers/v1/test_timers.py fail
st2api/tests/unit/controllers/v1/test_traces.py fail
st2api/tests/unit/controllers/v1/test_triggerinstances.py fail
st2api/tests/unit/controllers/v1/test_triggers.py fail
st2api/tests/unit/controllers/v1/test_triggertypes.py fail
st2api/tests/unit/controllers/v1/test_webhooks.py fail

Besides converting all the endpoints to the OpenAPI spec, there's two major points under consideration:

  • current implementation of RBAC doesn't work for OpenAPI as it heavily relies on positional arguments and method names neither of which has any value in context of OpenAPI spec. We have other reasons to want to review our approach to RBAC so it's hard for me to justify spending time on making current implementation work.
  • OpenAPI provides means for validation not only input data, but also endpoint's response. Problem is, by the time the response is ready for validation, all the work related to that response was already completed and in most cases can not be easily undone. Raising an error in such circumstances doesn't look right, but ignoring the fact response doesn't match the scheme isn't ok either as API consumers would rely on the spec. The only option I see is to set a header that would warn consumer of response being invalid. We would then check for this header in our tests to make sure we don't get a regression. I'm open for other suggestions, though.
@enykeev

This comment has been minimized.

Copy link
Member

enykeev commented Jan 20, 2017

Notable changes to controllers:

  • Pecan expects controller to be a class of certain methods (get_one, post, delete). OpenAPI deals with functions. Instead of instantiating controller classes implicitly during initialization as Pecan does it, OpenAPI router expects operationId to point to a static method of a class or a method of a class instance. See actions.py#L316 and openapi.yaml#L12 for example.
  • jsexpose decorator is out. We have no use for it anymore. See actions.py#L103
  • Controller functions should return an instance webob.Response (see actions.py#L146-L149). While they may still return arbitrary python object as a response (which will be implicitly converted to webob.Response by router router.py#L264-L265), there is no way to set status code or headers on the response.
  • If controller response need to be modified by some kind of middleware before it will be returned to the user, said middleware would have to repackage the response object. See resource.py#L414-L419 for example.
  • RBAC decorators should be commented out until we figure out what to do with them (actions.py#L87). There's relatively good chance that we would need them to be exposed in OpenAPI spec so we should probably start adding them there now (openapi.yaml#L13).
  • All mentions of pecan should be removed from controllers and replaced with OpenAPI analogs. pecan.response should be integrated to webob.Response (replacing this with that). pecan.request should be exposed through OpenAPI parameters or x-parameters.

Later, I want us to think about getting rid of controller classes completely as it might provide better readability. Doing it now as a part of this PR will just complicate the diff.

Add Webhook endpoint
The endpoint is incomplete and requires additional attention
@enykeev

This comment has been minimized.

Copy link
Member

enykeev commented Jan 24, 2017

Note: despite the fact that tests are passing on webhook endpoint, it requires additional attention due to lacking coverage and complex design. Avoid merging the PR until the problems are addressed.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 24, 2017

Codecov Report

Merging #2727 into master will increase coverage by 0.01%.
The diff coverage is 94.1%.

@@            Coverage Diff            @@
##           master   #2727      +/-   ##
=========================================
+ Coverage   77.78%   77.8%   +0.01%     
=========================================
  Files         433     428       -5     
  Lines       22430   22170     -260     
=========================================
- Hits        17447   17248     -199     
+ Misses       4983    4922      -61
Impacted Files Coverage Δ
st2api/st2api/config.py 52.17% <ø> (ø)
st2common/st2common/util/api.py 91.3% <ø> (+1.14%)
st2common/st2common/logging/formatters.py 95.1% <ø> (ø)
st2common/st2common/models/api/base.py 96.72% <ø> (+4.83%)
st2client/st2client/commands/pack.py 45.36% <ø> (-0.05%)
st2common/st2common/rbac/resolvers.py 91.78% <ø> (+1.83%)
st2api/st2api/cmd/api.py 0% <0%> (ø)
st2auth/st2auth/wsgi.py 0% <0%> (ø)
st2api/st2api/wsgi.py 0% <0%> (ø)
st2auth/st2auth/cmd/api.py 0% <0%> (ø)
... and 103 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e7572b...88c3d9e. Read the comment docs.

enykeev and others added some commits Jan 25, 2017

Add Response validation
When response would not match the scheme, the router will set a 'Warning' header to inform client that response could be malformed and requires additional client-side validation. We also check for the header during unit tests to try and catch regression earlier.
Anirudh Rekhi
@@ -116,21 +117,22 @@ def get_all(self, prefix=None, scope=FULL_SYSTEM_SCOPE, user=None, decrypt=False
scope = FULL_USER_SCOPE

scope = get_datastore_full_scope(scope)
requester_user = get_requester()
requester_user = cfg.CONF.system_user.user

This comment has been minimized.

@enykeev

enykeev Jan 26, 2017

Member

I have a feeling you've simplified this part too much. In this controller, we're dealing with two kinds of user: one requests the key and another one is on behalf of whom the key is requested. You provide username for the second one through user query parameter. First one, according to get_requester() is either user from auth context (one you've authenticated with) or system_user (stanley) for the auth=false case.

To fix that, you would need to get second user from WSGI context like https://github.com/StackStorm/st2/blob/feature/rest_services_v2/st2api/st2api/controllers/openapi.yaml#L408-L411. Problem is, now they have conflicting variable names.

enykeev and others added some commits Jan 25, 2017

Support additional path requirements
The additional check helps avoid the situation when static portion of the path gets resolved as dynamic due to lack of ordering or priority during patch resolving.

An example of that situation will be an attempt to resolve a path (`/rules/views`) against two matching endpoints (`/rules/views` and `/rules/{ref_or_id}`). Depending on the dictionary order, the path may be resolved to either of two. The additional requirement helps us to stear the algorithm to a particular resolution.
Anirudh Rekhi
Anirudh Rekhi
def extend_with_default(validator_class):
validate_properties = validator_class.VALIDATORS["properties"]

def set_defaults(validator, properties, instance, schema):

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

I know we already do similar thing for action parameters, etc. but the actual "extend with default" snippet posted on jsonschema webpage doesn't work so we need to implement our own thing to correctly handle default values everywhere.

It looks like that is not needed here and the "official" version is sufficient?

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

I have no idea why official snippet doesn't work for action parameters, never had any problems with it.

This comment has been minimized.

@Kami

Kami Mar 3, 2017

Member

IIRC, only of the problems with the default snippet is that it only handles validation for default values, but it doesn't actually manipulate the instance passed to the validator aka also assign the default values.

But yeah, if assigning is not needed, then it should work fine.

json_body = kwargs.pop('json_body')
else:
json_body = kwargs.pop('json')
body = json_encode(json_body).encode('UTF-8')

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Do we still correctly handle "user-friendly" response JSON encoding?

IIRC, in the past, by default we encoded it friendly for computer consumption by default so it was more compact, but if ?debug flag was used, we returned it user-friendly formatted.

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

Nope, currently we always return it formatted. Any numbers that make us think that saving on spaces is a good idea?

This comment has been minimized.

@Kami

Kami Mar 3, 2017

Member

Then we are fine 👍

for route in sorted(self.routes.matchlist, key=lambda r: r.routepath):
LOG.debug('Route registered: %+6s %s', route.conditions['method'][0], route.routepath)

def match(self, req):

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Is this method called on every request to figure out to which controller to dispatch it (it looks like it)?

If yes, just curious how performant it is...

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

Well, reddit uses it so I guess we're fine. Routes library have benchmarks in the repo, but I'm not sure with what we should compare it, really.

if match is None:
raise NotFoundException('No route matches "%s" path' % req.path)

# To account for situation when match may return multiple values

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Hm wouldn't multiple values indicate a programmer error - aka the route is not set up correctly since there should only be one controller match for each path?

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

Potentially. The url /some/views/ may match both /some/{ref_or_id} and /some/views/{empty_string}. I'm hesitant of making it a problem for our users, though. We need to catch this things during development time and try to gracefully recover in runtime.


if 'user' not in context:
raise auth_exc.NoAuthSourceProvidedError('One of Token or API key required.')
except (auth_exc.NoAuthSourceProvidedError,

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Ignore my comment above.

I see exception translation for auth related errors happens here now. Perhaps now a bad idea to refactor this in a special error translation / handling middleware or similar.

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

I though about using middleware for that but decided against it at the end. I see authentication an inalienable part of the application and if we decide to extend it, we have other means for that.

if cfg.CONF.rbac.enable:
user_db = context['user']

permission_type = endpoint.get('x-permissions', None)

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Ahh, I see.

So the permission checking which was previously performed on a decorator level for "global" controller related permissions now happens here?

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

Yes. It only works for most simple RBAC case though. We would have to figure complex cases during RBAC 2.0, but I want us to move in this general direction so we could display rbac requirements in API docs.


# Collect parameters
kw = {}
for param in endpoint.get('parameters', []) + endpoint.get('x-parameters', []):

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Quite some "magic" (aka new jsexpose in some ways) :P

It's definitively an improvement over jsexpose because now at least it uses global definitions, but I it's still a lot going on here so I would also prefer some standalone explicit tests with all the edge cases (aka what is not already tested with existing API controller tests) for this functionality if we don't have some already.

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

This is where OpenAPI really comes into play, the rest was just foreplay.

And sadly we can't avoid this complexity if we want controllers to get the list of arguments instead of dry (request, response, next) notation of proper http routers. The worst part is surely the 'body' type, but I still can sign under every single line in it, we need them all.

type: object
properties:
version:
type: string

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Perhaps also a good idea to make it a bit more friendly and also add description string to those two response attributes.

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

We need A LOT more descriptions and examples in this file. It's going to be a long journey.

type: array
items:
$ref: '#/definitions/Action'
examples:

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

I like this 👍

examples:
application/json:
ref: 'core.local'
# and stuff

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Ideally we could re-use existing test fixture or similar though.

If we already do and I misunderstood how it works, please ignore it.

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

We could try and reuse them. I'm hesitant because essentially, examples and fixtures serves different purposes and it would be a constant struggle to compose them in a way that would fit both.

x-as: requester_user
description: User running the action
responses:
'200':

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Just trying to understand how this works for non default, non-200 responses (e.g. 400 for conflict on save, etc.). I only see definition for unexpected error.

Do we need to define such things here or not?

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

Once again, we could define every single possible error for the particular endpoint or we could use an umbrella default scheme to signify any possible error since we stick to the same response scheme for them anyway (an object with "faultstring" property). It's up to us. I personally don't think it's worth the hassle to do it now, maybe in the release or two when we populate the rest of the file with proper descriptions or get this requested by users.

/api/v1/actions:
get:
operationId: st2api.controllers.v1.actions:actions_controller.get_all
x-permissions: {{ PERMISSION_TYPE.ACTION_LIST }}

This comment has been minimized.

@Kami

Kami Mar 2, 2017

Member

Bit confused, I see x-permissions here but not for some other controllers (e.g. create action, etc.).

Is this intentional for some reason or it still needs to be finished?

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

x-permissions only check that user has particular permission, it doesn't factor the data. Methods like POST, PUT or DELETE requires user to have particular permission to a particular entity and are not covered by our OpenAPI spec yet. Fixing it will be one of the problems we would have to solve during RBAC 2.0 work.

@@ -23,14 +23,18 @@
from st2common.exceptions.auth import NoNicknameOriginProvidedError, AmbiguousUserError

This comment has been minimized.

This comment has been minimized.

@enykeev

enykeev Mar 3, 2017

Member

now we'll see which one lands first =)

enykeev added some commits Mar 3, 2017

@enykeev enykeev force-pushed the feature/rest_services_v2 branch from d1fa7dd to 58175e5 Mar 7, 2017

enykeev added some commits Mar 9, 2017

Merge remote-tracking branch 'origin/master' into feature/openapi
# Conflicts:
#	fixed-requirements.txt
#	requirements.txt
#	st2api/st2api/controllers/v1/pack_configs.py

@enykeev enykeev force-pushed the feature/rest_services_v2 branch from 94e4795 to adff3bc Mar 20, 2017

@enykeev enykeev force-pushed the feature/rest_services_v2 branch from adff3bc to f06668a Mar 20, 2017

@enykeev enykeev removed the RFR label Mar 23, 2017

@enykeev enykeev merged commit a826898 into master Mar 23, 2017

8 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 93.78% of diff hit (target 77.82%)
Details
codecov/project 78.12% (+0.29%) compared to f534b14
Details
st2/e2e/ubuntu14 E2E tests have finished successfully
Details
st2/e2e/ubuntu14-enterprise E2E tests have finished successfully
Details
st2/e2e/ubuntu16 E2E tests have finished successfully
Details
st2/e2e/ubuntu16-enterprise E2E tests have finished successfully
Details
st2/packages Packages have been built
Details

@enykeev enykeev deleted the feature/rest_services_v2 branch Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment