[THREESCALE-1709] Routing policy #976

davidor · 2019-01-08T14:50:46Z

Closes #972

This PR adds a new policy, routing. It can select an upstream based on the request path, a request header, a request query argument, and a JWT claim.

I added a README that explains how the policy works.

Depends on #974 and #975

Ref: https://issues.jboss.org/browse/THREESCALE-1709

gateway/src/apicast/policy/routing/upstream_selector.lua

davidor · 2019-01-08T15:13:14Z

gateway/src/apicast/policy/routing/request.lua

@@ -0,0 +1,35 @@
+local setmetatable = setmetatable


My idea is to use this module from other policies too.

I think we should instantiate it when the policies context is created for the current request, and store an instance of this module there. I think it would improve code organization, and also avoid reading several times the headers and the query arguments.

I'd rather do that in a different PR because it might affect many policies and need changes in some tests.

I agree that this should become part of the core API. For that, we need a carefully designed interface that is usable by all the policies we have so far. And that that surely wait for another PR 👍

davidor · 2019-01-10T16:14:47Z

@mikz I added support for conditions with 'or' and for liquid templating in the last two commits.

It's true that supporting those 2 things complicates a bit the config schema because it requires extra attributes. However, I think that those are 2 useful features and we should support them. I added them in 2 separate commits, so they can be reverted easily.

mikz

Good job 👍

I have a few nitpicks, but nothing too critical.

mikz · 2019-01-15T02:59:29Z

gateway/src/apicast/policy/routing/request.lua

@@ -0,0 +1,35 @@
+local setmetatable = setmetatable


I agree that this should become part of the core API. For that, we need a carefully designed interface that is usable by all the policies we have so far. And that that surely wait for another PR 👍

gateway/src/apicast/policy/routing/README.md

gateway/src/apicast/policy/routing/apicast-policy.json

gateway/src/apicast/policy/routing/routing_operation.lua

mikz · 2019-01-15T03:25:04Z

t/apicast-policy-routing.t

+--- no_error_log
+[error]
+
+=== TEST 22: the rule matches a jwt claim using "!=" and the condition is false


I think this integration test has way too many test cases to be readable and understandable for me.
Testing all possible options and their negations can be done in lower level tests line unit tests.

I'd rather leave this focused on high level operations, how it treats the http request, what Host header it passes, how it mangles request path, forwards post body etc.

Hmm I think that for each operation (matches , == ... ) it makes sense to test both the case where it evaluates to true and false. I see what you mean, this test is too verbose, but I think part of it is because sending a request with a JWT requires quite a few lines in the config and the request.

I think it makes sense to test both cases for each operation, but probably more in a unit test rather than integration one. Integration tests should be focused on the HTTP request itself.
I get that that unit tests might be a bit different because there is no real http request, but that is fine in my opinion. If we find out we have issues with it we try to solve that.

Maybe extracting those common parts into some shared helpers or something.

Fair enough. I think that for this particular case it's enough to test operations when they match. If we try to match a header and it works, it means that the code was able to extract the header specified correctly, which is one of the things we want to do in these integration tests. Adding an extra test with the same information but evaluating to false does not add much value.

All the operations are covered in the unit tests of the Condition module.

I deleted the redundant tests following that guideline. They went from 32 to 20: 5fa61d2

mikz · 2019-01-15T03:28:11Z

gateway/src/apicast/policy/routing/apicast-policy.json

+        "items": {
+          "type": "object",
+          "properties": {
+            "url": {


Do we need just this? We don't need to control whether it rewrites Host header?

I'm not sure. Do you think we need a way to prevent rewriting the Host header?
Currently, the host is always rewritten calling this method:
https://github.com/3scale/apicast/blob/75eb7e77d466d5ac6ba0aded9446838c87414e95/gateway/src/apicast/upstream.lua#L153

I think we should double check with out requirements and with people using this.
Host header can be different from the hostname where the traffic should go.
There might be a need to supply own Host header in some cases.

I think that we can leave this as it is and add the extra parameter later if needed.
Let's check with @vramosp

Is it feasible to offer the following set of options?

Host header overwrite by new upstream host (default)

Host header unchanged

Host header manual overwrite with a new fixed value

If it adds too much complexity then stick with the first one.

I created an issue for this: #981

Just a cosmetic change.

I think that for this particular case it's enough to test operations when they match. If we try to match a header and it works, it means that the code was able to extract the header specified correctly, which is one of the things we want to do in these integration tests. Adding an extra test with the same information but evaluating to false does not add much value.

davidor added 4 commits January 8, 2019 15:43

Add schema for new routing policy

e3c6b15

policy/routing: add Request

d4000ea

policy/routing: add RoutingOperation

7e21afa

policy/routing: add Rule

d2ae878

davidor requested a review from a team as a code owner January 8, 2019 14:50

codeclimate bot reviewed Jan 8, 2019

View reviewed changes

gateway/src/apicast/policy/routing/upstream_selector.lua Outdated Show resolved Hide resolved

davidor added 3 commits January 8, 2019 15:56

policy/routing: add UpstreamSelector

e4badb7

policy/routing: add the policy

edf9017

t: test Routing policy

96f1ff3

davidor force-pushed the routing-policy branch from c4139ed to 96f1ff3 Compare January 8, 2019 14:57

davidor changed the title ~~[WIP] Routing policy~~ [WIP] [THREESCALE-1709] Routing policy Jan 8, 2019

davidor commented Jan 8, 2019

View reviewed changes

CHANGELOG: add routing policy

a9c304d

davidor force-pushed the routing-policy branch from cf349de to a9c304d Compare January 8, 2019 15:53

davidor added 2 commits January 10, 2019 12:18

policy/routing: add support for conditions with or

b5da802

policy/routing: add support for liquid templating

c4eac99

davidor changed the title ~~[WIP] [THREESCALE-1709] Routing policy~~ [THREESCALE-1709] Routing policy Jan 10, 2019

policy/routing: add README

009e28e

davidor force-pushed the routing-policy branch from a5c2f75 to 009e28e Compare January 11, 2019 11:44

mikz approved these changes Jan 15, 2019

View reviewed changes

davidor mentioned this pull request Jan 15, 2019

Conditional Policy: Add more operations #821

Open

davidor added 2 commits January 15, 2019 10:57

policy/routing: rename 'thing_to_match' to 'match'

71e16d9

Just a cosmetic change.

davidor force-pushed the routing-policy branch from e375e7b to 5fa61d2 Compare January 15, 2019 11:43

mikz approved these changes Jan 15, 2019

View reviewed changes

davidor merged commit c214825 into master Jan 15, 2019

davidor deleted the routing-policy branch January 15, 2019 13:19

davidor mentioned this pull request Jan 17, 2019

Feature: add ability to modify Host header in the routing policy #981

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[THREESCALE-1709] Routing policy #976

[THREESCALE-1709] Routing policy #976

davidor commented Jan 8, 2019 •

edited

Loading

davidor Jan 8, 2019

mikz Jan 15, 2019

davidor Jan 15, 2019

davidor commented Jan 10, 2019 •

edited

Loading

mikz left a comment

mikz Jan 15, 2019

mikz Jan 15, 2019 •

edited

Loading

davidor Jan 15, 2019

mikz Jan 15, 2019

davidor Jan 15, 2019

mikz Jan 15, 2019

davidor Jan 15, 2019

mikz Jan 15, 2019

davidor Jan 15, 2019

MarkCheshire Jan 17, 2019 •

edited

Loading

davidor Jan 17, 2019 •

edited

Loading

[THREESCALE-1709] Routing policy #976

[THREESCALE-1709] Routing policy #976

Conversation

davidor commented Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Jan 10, 2019 • edited Loading

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCheshire Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

davidor Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

davidor commented Jan 8, 2019 •

edited

Loading

davidor commented Jan 10, 2019 •

edited

Loading

mikz Jan 15, 2019 •

edited

Loading

MarkCheshire Jan 17, 2019 •

edited

Loading

davidor Jan 17, 2019 •

edited

Loading