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

Apply policies conditionally #744

Closed
mikz opened this issue Jun 5, 2018 · 11 comments
Closed

Apply policies conditionally #744

mikz opened this issue Jun 5, 2018 · 11 comments
Assignees
Milestone

Comments

@mikz
Copy link
Contributor

mikz commented Jun 5, 2018

Use Cases

Change Upstream API on matching HTTP Request Header. (#429)
Two endpoints of an API need different policies (for example rate limits).

Concerns

If every policy is left to implement own matching then are going to be left with a lots of duplication.
Also policy would need to be changed to implement matching.

Proposals

Conditional policy

As outlined in #173 (comment). This policy would be itself a policy chain and execute the chain only when the precondition matches.

That would allow us to implement all condition matching only in this policy and just nest desired actions within it.

Match phase

Match phase would be some shared code that the policies would get by default.
The APIcast policy schema should define default properties for the policy schema to have extra configuration properties available.

Match phase would be executed once per policy to see if it applies to the request.

Conditions per "rule"

Some policies can have several "rules" like: add header "foo", add header "bar", rate limit /test to 100 rps, etc.
Each of those rules might need different condition. Offering some standard definition of conditions in the APIcast policy schema would allow policies to define conditions for rules easily. This would have to be opt-in feature made by policy developers. For examples of such conditions see https://github.com/3scale/ostia/issues/17#issuecomment-396913897.

Comparison

We need to do deeper comparison of both approaches and possibly PoCs before committing to something.

Concerns

Defining conditions in JSON schema can be quite hard especially because missing oneOf support.
It might be possible to use Liquid somehow, but not really see how. But that would offer the best flexibility at least in the beginning.

This was referenced Jun 12, 2018
@davidor davidor self-assigned this Jul 5, 2018
@davidor
Copy link
Contributor

davidor commented Jul 5, 2018

@mikz I implemented a first version of the 'Conditional policy' option. Check this branch: https://github.com/3scale/apicast/tree/conditional-policy

It's just a first version without much documentation and there are some things that can be improved, but it shows how it would work, and has tests showing that it behaves as expected.

@davidor
Copy link
Contributor

davidor commented Jul 9, 2018

I implemented another prototype just to have another option to evaluate. It's very similar to the other one but it uses the sandbox lib (https://github.com/apitools/sandbox.lua) to run lua code instead of liquid: https://github.com/3scale/apicast/tree/conditional-policy-lua-sandbox

I think that there are 2 different topics for discussion. One is how to evaluate the condition specified in the policy (liquid, lua code, etc.) and the other is where to apply the matching (new conditional policy, match phase, etc.)

@davidor
Copy link
Contributor

davidor commented Jul 10, 2018

I pushed a new branch that achieves the same using a different approach: https://github.com/3scale/apicast/tree/policy-conditions

In this branch, I modified the policy phases to check whether self.condition is true and run the phase only when that's the case. The good thing about this approach is that it does not need a new policy. However, I don't see a way to avoid adding the condition property on each config schema so the 3scale UI can show this field.

Notice that both in this branch and the other branches I pushed, the condition is evaluated in every phase. I think this can be useful because if we use the policies shared context as part of the env for evaluating the condition, it can change between phases. For example, some policy might add something to the context in rewrite(). This is the downside I see in the 'match phase' approach. Using that approach, the condition would be applied just once per request.

@mikz let me know what you think.

@y-tabata
Copy link
Contributor

y-tabata commented Jul 11, 2018

This is just an idea.
Regarding the 'Conditional policy', I imagine something like a flow chart.
So more intuitively, the following implementation can be thought of an option, I think.

"policy_chain": {
  "policies": [
    { "id": 1, "name": "apicast.policy.token_introspection" },
    { "id": 2, "name": "apicast.policy.rate_limit" },
    { "id": 3, "name": "apicast.policy.rate_limit" },
    { "id": 4, "name": "apicast.policy.apicast" }
  ],
  "chains": [
    { "from": 1, "to": 2, "condition": "a = b" },
    { "from": 1, "to": 3, "condition": "a ~= b" },
    { "from": 2, "to": 4 },
    { "from": 3, "to": 4 }
  ]
}

How do you think?
Is it difficult to implement or troublesome to configure?

@davidor
Copy link
Contributor

davidor commented Jul 11, 2018

@y-tabata your solution allows modeling more complex flows. However, we should check for cycles, for example. I would personally choose the simple solution for now. However, if we discover a use case that needs complex flows we'll keep your solution in mind.

@davidor
Copy link
Contributor

davidor commented Jul 12, 2018

After some discussion with @mikz this is what we agreed on:

  • We're going to go for the option of implementing the conditional policy. The one in this branch: https://github.com/3scale/apicast/tree/conditional-policy
  • Regarding how to evaluate conditions, both liquid and Lua have their problems. For example, the liquid examples that we have are verbose even for simple conditions, and the lua sandbox library does not work correctly in luajit. We think that it's better to implement a DSL that allows to express conditions like request_path == "/abc" or request_method == "GET".

@pimg
Copy link
Contributor

pimg commented Jul 23, 2018

Does the conditional policy still covers the first use case mentioned in this issue:
Change Upstream API on matching HTTP Request Header. (#429)

If so, is it possible to give a small example of the configuration?

@davidor
Copy link
Contributor

davidor commented Jul 23, 2018

@pimg I'll add some docs and possibly some tests that show that the conditional policy covers that use case.

@davidor
Copy link
Contributor

davidor commented Jul 23, 2018

@pimg I added a small change to be able to support that use case : #819

Tomorrow, I'll add a specific README for the conditional policy and I'll include this use case as an example of what can be achieved with it.

@davidor
Copy link
Contributor

davidor commented Jul 24, 2018

@pimg I opened a PR with more docs #820
Hope it helps.

@pimg
Copy link
Contributor

pimg commented Jul 24, 2018

Thanks! The docs make it a lot clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants