Config revamp

[adam-cattermole]

Changes

This is moving towards a configuration based on the suggestion in #108.

Currently this is looking as follows:

services:
  auth-service:
    type: auth
    endpoint: auth-cluster
    failureMode: deny
    timeout: 10ms
  ratelimit-service:
    type: ratelimit
    endpoint: ratelimit-cluster
    failureMode: deny
actionSets:
  - name: rlp-ns-A/rlp-name-A
    routeRuleConditions:
      hostnames: [ "*.toystore.com" ]
      matches:
      - selector: request.url_path
        operator: startswith
        value: /get
      - selector: request.host
        operator: eq
        value: test.toystore.com
      - selector: request.method
        operator: eq
        value: GET
    actions:
    - service: ratelimit-service
      scope: rlp-ns-A/rlp-name-A
      conditions: []
      data:
      - selector:
          selector: request.headers.My-Custom-Header
      - static:
          key: admin
          value: "1"

I've separated all of the changes by commit to try make it easier to read the changes.

The primary change to non-config types is that we now have actions.conditions so we can conditionally perform an action - example provided in the envoy configuration that only calls the rate limiting service when user_id == 'alice':

make build && make local-setup

Port-forward envoy and watch the logs for all services:

kubectl port-forward --namespace default deployment/envoy 8000:8000
kubectl logs -f deployment/envoy
kubectl logs -f deployment/authorino
kubectl logs -f deployment/limitador

Test unauthenticated does not get rate limited:

curl -H "Host: test.b.multi.com" http://127.0.0.1:8000/get -i
# HTTP/1.1 401 Unauthorized

Bob is not rate limited (note no calls to limitador in logs):

while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H "Authorization: APIKEY IAMBOB" -H "Host: test.b.multi.com" http://127.0.0.1:8000/get | grep -E --color "\b(429)\b|$"; sleep 1; done

Alice has 5 requests per 10 seconds:

while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H "Authorization: APIKEY IAMALICE" -H "Host: test.b.multi.com" http://127.0.0.1:8000/get | grep -E --color "\b(429)\b|$"; sleep 1; done

Outstanding

• Tests for conditional actions
• Retrieve the scope from action.data (won't do)

For a future PR:

• Convert routeRuleConditions.matches and action.conditions from PatternExpression to CEL
• "Merge" subsequent actions of the same type to only make a single call for ratelimit

[eguzki]

Looking good

[eguzki]

Verification steps working

[adam-cattermole]

Latest commit removes action.scope and this information is retrieved from the data section now

[adam-cattermole]

I'm still writing tests for conditional actions, and realise that we need a coordinated merge of this so potentially this should be reviewed/merged to a feature branch

[eguzki]

Latest commit removes action.scope and this information is retrieved from the data section now

From data? This is unexpected. From a static data item with a key name (by convention) domain?

What if there are many, which one has preference?

[adam-cattermole]

From data? This is unexpected. From a static data item with a key name (by convention) domain?

I'm not tied to this change, and I can drop the commit. But I was working toward what we agreed in #108 where the data required to build the message was defined in the data of the action

- routeRuleConditions:
    hostnames:
    - "*.foo.com"
    - "users.bar.com"
    matches:
    - request.method == "GET"
    - request.path.startsWith('/bar')
  actions:
  - conditions:
    - "!['127.0.0.1', '192.168.1.1'].contains(source.ip)"
    action:
      service: limitador
      data:
        ip: source.ip
        magic_RL_name: "'1'"
  - conditions: []
    action:
      service: authorino
      data:
        authCfg: "'someIdentifier'"

[adam-cattermole]

I've dropped that commit fwiw. I tend to agree that right now the purpose of the data and scope is different and the way (we currently) process the data items means we need some custom logic to process a specific keyed data item, which doesn't seem right

[guicassolato]

Latest commit removes action.scope and this information is retrieved from the data section now

From data? This is unexpected. From a static data item with a key name (by convention) domain?

I agree with @eguzki. I don't like this change.

Whether as domain or as authCfg, in both cases this is a required piece of data and in both cases it has the meaning of “scope”. Inside of data, it feels out of place IMO. Unlike the rest of what goes there, scope is not as much "user-defined" data.

The way it was before, we could technically have an action without any data, which makes sense to me since we have default data that comes from the type of action (service) anyway. In fact, auth is an example of action that typically would involve zero user-defined data. And I could speculate with meaning to that for ratelimit as well (e.g. letting Limitador knows about request without activating any limit).

Anyway, I honestly liked more how it was before. Thanks for ditching the commit!

[adam-cattermole]

Thanks both for the constructive feedback, will continue on tests for conditional actions