SOAP policy #567

davidor · 2018-01-30T17:55:13Z

This policy adds support for a very small subset of SOAP.

This policy basically expects a SOAPAction URI in the SOAPAction header or the content-type header.
The SOAPAction header is used in v1.1: https://www.w3.org/TR/2000/NOTE-SOAP-20000508/#_Toc478383528, whereas the Content-Type header is used in v1.2: https://www.w3.org/TR/soap12-part2/#ActionFeature

The SOAPAction URI is matched against the mapping rules defined in the configuration of the policy and the usage resulting from that is authorized and reported against 3scale's backend.

davidor · 2018-01-30T17:56:29Z

@mikz I didn't add a JSON schema. I'll wait until #565 is merged.

davidor · 2018-01-30T17:59:21Z

gateway/src/apicast/policy/soap/policy.lua

+    local soap_usage = context.service:get_usage(
+      ngx.req.get_method(), soap_action_uri)
+
+    context.add_to_usage = soap_usage


@mikz I considered several options here but this seemed the cleanest one. We could pass the URI instead and do the mapping rules matching later.

I wonder how several policies would interact. Or if this would be twice in the chain.

My idea was to make each policy to increment the values in the context. That way the policy can decide what to do: add, replace, remove, ...

Definitely. If in the future we have more policies doing the same thing, we'll need to update add_to_usage instead of simply assign it.

What I mean is that we can just context.usage.hits = (context.usage.hits or 0) + 1 in the rewrite phase. Or expose some table merger function to merge those. Or have them in an array and merge them in the proxy:access. It just does not feel right to have add_to_usage when the usage key is there too.

davidor · 2018-01-30T18:00:26Z

gateway/src/apicast/proxy.lua

  end
 end

-function _M:access(service)
+function _M:access(service, context)


@mikz I decided to pass the context instead of passing just the usage to be increased because in the future there might be other policies passing other information in the context. This way we won't need to change the signature of the access() method again.

I'm not sure. https://github.com/3scale/apicast/pull/556/files#diff-31313c92616b54028dac1ba183e6f79aR287 was all the info it needed. It is quite a lot of params, but not that bad imo.

I'm OK with adding 2 or 3 extract params. I'm more concerned about changing the signature of access() in the future and breaking backwards compatibility.

I agree. Maybe we can build on #556 and try to figure out out there and then just use it from here?

Sounds good 👍

andrewdavidmackenzie · 2018-01-31T10:04:58Z

gateway/src/apicast/policy/soap/policy.lua

+-- This policy adds support for a very small subset of SOAP.
+-- This policy basically expects a SOAPAction URI in the SOAPAction header or
+-- the content-type header.
+-- The SOAPAction header is used in v1.1:


"....v1.1 of the SOAP Standard"
(just to avoid any confusion in readers about versions of apicast or something?)

andrewdavidmackenzie · 2018-01-31T10:06:03Z

gateway/src/apicast/policy/soap/policy.lua

+-- https://www.w3.org/TR/soap12-part2/#ActionFeature
+-- The SOAPAction URI is matched against the mapping rules defined for the
+-- service and the usage resulting from that is authorized and reported
+-- against 3scale's backend.


"...and reported to the API Management platform using the Service Management API" or similar?

I usually refer to backend as '3scale's backend', sounds simpler than that, but would be good to reach an agreement on that. Should we call it by its new upstream name?

I'd call it 3scale's backend. Definitely try not to write its full name because I could not get it right and have to copy paste it from somehwere.

Agreed @mikz

mikz · 2018-01-31T12:49:28Z

gateway/src/apicast/policy/soap/policy.lua

+local _M = policy.new('SOAP policy')
+
+local soap_action_header = 'SOAPAction'
+local soap_action_ctype = 'application/soap+xml?action='


I think this has to be parsed as URI args. Because we can't control the action appears right after as it can have & and be the last param.

Also quick google search shows some people use ; to concat params. We should verify if that can really be the case.

You're right @mikz . I wrongly assumed that action would be the only param.

About using ;, I think we need to support that instead of ?...&... :
https://developer.mozilla.org/es/docs/Web/HTTP/Headers/Content-Type and https://tools.ietf.org/html/rfc1341

Yep. https://tools.ietf.org/html/rfc7231#section-3.1.1.1 says parameters are separated by ; and https://tools.ietf.org/html/rfc3902 says it is a parameter. I wonder where I've seen the example with ?. Can't find it now.

mikz · 2018-01-31T12:50:41Z

gateway/src/apicast/policy/soap/policy.lua

+
+  if soap_action_uri then
+    local soap_usage = context.service:get_usage(
+      ngx.req.get_method(), soap_action_uri)


So this is using service mapping rules? I'm not sure users can define full URLs there.
I though we will have some mapping rules in this policy.

I assumed it was possible to define something like /a_path#some_action in the mapping rules. That's why I decided to do it this way. I see now that it's not possible. With that constraint, I think we need to include mapping rules in the configuration of this policy.

@mikz Let me know what you think about this solution:

Add a mapping rules array in the policy config. Each element will contain: pattern, http_method, metric_name, and delta. The same fields we have for the service mapping rules.

Add a add_mapping_rule() method in Service. This would simply accept the fields above, add the ones needed to construct the rule, and then add it to the self.rules table. We'll need to make sure that add_mapping_rule() is only called once during the lifetime of the policy so we don't add duplicated rules.

The above will let us call the existing Service:get_usage(). This method returns the metrics to auth and report based on the mapping rules defined. We'll call this method only when we receive a SOAP action.

@davidor interesting idea. I'm not sure if I like the idea of merging then into one list. I think they should be applied separately. IMO there could be some rules that match both and because some are absolute and some relative it could be quite nasty as / would increment hits all the time and twice.
Maybe we need to exposeUsageExtractor:get_usage which is initialized with list of mapping rules. Basically exposing the low level API where you can inject own mapping rules.

I get your first point, but wouldn't that be coherent with the way mapping rules work currently?

Suppose that the policy adds this mapping rule: /a_path#a_soap_action => +1 a_soap. If the user has already defined /a_path => +1 a_path, we should increase both a_soap and a_path by 1. If the user already defined / => +1 hits, and hits is a parent metric of the other 2, well, it's going to be increased by 3(!), but that's how it currently works no?

Regarding your second suggestion about exposing UsageExtractor:get_usage, could you please expand a bit on it or provide some pseudo-code or examples?

I think the issue is when you have following rules:

/ => hits
/a => a

And in soap you'll have
http://example.com/soap#a => a

If you match http://example.com/soap#a it will return hits=1&a=1.

So it depends if this replaces mapping rules or complements them.
If it would first match mapping rules and then soap rules and merged the results then for a request:

GET / Content-Type: application/xml+soap?action=http://example.com/soap#a

it would match hits=2&a=1. Which is wrong IMO. Because the path mapping rules should not be applied to soap mapping rules (because they are relative and not absolute).
I'm not thinking about parent/child metrics, just about applying mapping rules to the request path/ soap action.
The point is that current path relative mapping rules like /a can unintentionally match soap services like http://example.com/a and double counting. And also match different services like http://example.com/a and http://foobar.com/a.

And the signature of UsageExtractor would be quite simple.

local usage_rules = UsageExtractor.new({ { pattern = 'http://example.com/a', http_method = 'GET', metric_name = 'a', delta = 1 } }) local usage = usage_rules:get_usage(...)

And the Service:get_usage would use this too. But we would just have an API to create own independent list of rules.

@mikz Thanks for the example.

It's clear to me now why you think that a url received via SOAP Action should not be matched against the service mapping rules. It should only be matched against the mapping rules specified in the SOAP policy. I agree with that.

Now, what happens when the SOAP policy is enabled but a request does not contain a SOAP Action ? I think we should apply the service mapping rules in that case.

I like the idea of extracting a UsageExtractor module from Service and Configuration. This is something I've been thinking about. Those 2 modules have many responsabilities and need to be splitted. This seems to be a good excuse to do it :) I'll address this task in a separate PR.

mikz · 2018-02-08T08:37:52Z

gateway/src/apicast/policy/soap/init.lua

@@ -0,0 +1 @@
+return require('soap')


So I started to think about this. And I think we can expose some support tooling in different module, so it can be unit tested.

Lets say the main policy code is in soap_policy.lua. Then there can be soap.lua that has stuff like "extracting the soap action" and it can be unit tested in busted.

That would allow us not exposing extra methods on the policy, but still exposing it internally (if the loading works) for tests. And policies should not be able to load other policies (but that is not enforced yet), so we should be fine and the code should be used only from tests.

Just some food for though. I'd like to hear your take.

That would be a nice improvement.

Otherwise, in some cases Proxy reports the usage associated with the service mapping rules instead of the merged one.

davidor · 2018-02-08T10:44:24Z

This is ready @mikz . There have been a few changes. Mainly because the code is now using the modules extracted in #571 , #573 , and #580 .

I also addressed your comments.

davidor · 2018-02-08T10:45:44Z

gateway/src/apicast/policy/soap/soap.lua

+
+local function usage_from_matching_rules(soap_action_uri, rules)
+  return mapping_rules_matcher.get_usage_from_matches(
+    nil, soap_action_uri, {}, rules)


@mikz Notice that I'm sending http_method = nil here. Not sure if we should take it into account for SOAP actions.

Hm. SOAP can be used with GET and POST.
Aha! The GET does not have the Content-Type header. So technically it should be only POST. I guess.

But it looks like the GET is used as some REST hybrid, so I guess we could leave that to the mapping rules.
I'd possibly even go for hardcording POST there.

I don't really know about this. The Content-Type is included in the request, not in the SOAP action URI.
Depending on what we decide here we'll need to add it to the JSON schema also. Notice that for now, I only included 'pattern', 'metric', and 'delta', and left out other fields that are present in the proxy rules.

I see. We are initializing the mapping rules directly from the config. I was thinking that we could just override the http_method attribute with POST there. And then pass the real http method here. So we match it only when the request is POST.
But it is probably not really important.

mikz · 2018-02-08T10:54:42Z

spec/policy/soap/policy_spec.lua

+          }
+        end
+
+        soap_policy:rewrite(context)


Oh. This is so good 🥇 Great we can unit test this.

mikz · 2018-02-08T10:58:30Z

gateway/src/apicast/policy/soap/soap.lua

+local function soap_action_in_ctype(headers)
+  local ctype = headers['Content-Type']
+
+  if ctype and starts_with(ctype, soap_action_ctype) then


I think this won't be that easy 🤔
The RFC says those are all equivalent:

text/html;charset=utf-8
text/html;charset=UTF-8
Text/HTML;Charset="utf-8"
text/html; charset="utf-8"

And looks like it can have whitespace before ; too. Look at the definition: media-type = type "/" subtype *( OWS ";" OWS parameter ). And OWS is optional white space.

You're right 👍

Check last commit.

mikz · 2018-02-08T12:33:24Z

gateway/src/apicast/policy/soap/soap.lua

+    local name = header_param_split[1]
+    local value = header_param_split[2]
+    if name == "action" then
+      return value


The value can be either "url" or just url. We should strip the quotes.

This is also solved in the new commit.

mikz · 2018-02-08T14:29:55Z

gateway/src/apicast/policy/soap/soap.lua

 local function soap_action_from_ctype_params(params)
-  local params_split = re.split(params, ";")
+  local params_without_blanks = ngx.re.gsub(params, '\\s', '')
+  local params_split = re.split(params_without_blanks, ";")


You could also split by [[\s*;\s*]] to remove spaces around ;, right?
Seems a bit evil to strip all spaces from it. What if there is a space in the url? Now when I think about it.
What happens when there is ; in soap action? That would be quite broken.

mikz · 2018-02-08T14:31:11Z

gateway/src/apicast/policy/soap/soap.lua

-  if ctype and starts_with(ctype, soap_action_ctype) then
+  -- The Content-Type can be a mix of upper and lower-case chars. Convert it to
+  -- include only lower-case chars to be able to compare it.
+  if ctype and starts_with(lower(ctype), soap_action_ctype) then


I think this would not work for application/xml+soap ; action="" because of the whitespace.

…value Accepted formats defined here: https://tools.ietf.org/html/rfc7231#section-3.1.1.1

mikz · 2018-02-08T16:43:21Z

spec/policy/soap/policy_spec.lua

+    local policy_config = {
+      mapping_rules = {
+        {
+          pattern = '/soap_action$',


Because the RFC says these should be full URL we should test also with full URLs. To verify all the escaping works.

mikz

👍 excellent 🥇

octobot assigned davidor Jan 30, 2018

davidor commented Jan 30, 2018

View reviewed changes

davidor force-pushed the soap-policy branch from ebe67b9 to 4f32753 Compare January 30, 2018 18:02

davidor requested a review from mikz January 30, 2018 18:07

andrewdavidmackenzie reviewed Jan 31, 2018

View reviewed changes

mikz reviewed Jan 31, 2018

View reviewed changes

davidor force-pushed the soap-policy branch 2 times, most recently from ab3ec72 to fd7c54f Compare January 31, 2018 16:50

davidor mentioned this pull request Feb 7, 2018

Extract MappingRulesMatcher and Usage modules #580

Merged

davidor force-pushed the soap-policy branch from fd7c54f to 23cbaa4 Compare February 7, 2018 17:14

davidor changed the title ~~SOAP policy~~ [WIP] SOAP policy Feb 7, 2018

davidor force-pushed the soap-policy branch from 23cbaa4 to e38066c Compare February 7, 2018 17:18

mikz reviewed Feb 8, 2018

View reviewed changes

proxy: keep merged usage in ctx and self

bd13a03

Otherwise, in some cases Proxy reports the usage associated with the service mapping rules instead of the merged one.

davidor force-pushed the soap-policy branch from e38066c to 541353e Compare February 8, 2018 10:14

davidor added 4 commits February 8, 2018 11:20

Add SOAP policy

221d880

t: test SOAP policy

4d86b6b

spec: add specs for SOAP policy

4c7deea

CHANGELOG: add entry for SOAP policy

97f86eb

davidor force-pushed the soap-policy branch from 541353e to 97f86eb Compare February 8, 2018 10:30

davidor changed the title ~~[WIP] SOAP policy~~ SOAP policy Feb 8, 2018

policy/soap: add JSON schema

70bbb43

davidor commented Feb 8, 2018

View reviewed changes

mikz reviewed Feb 8, 2018

View reviewed changes

spec/policy/soap/policy_spec.lua

}

end

soap_policy:rewrite(context)

Copy link

Contributor

mikz Feb 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. This is so good 🥇 Great we can unit test this.

mikz reviewed Feb 8, 2018

View reviewed changes

davidor force-pushed the soap-policy branch from 5efc505 to e959f79 Compare February 8, 2018 16:28

policy/soap: support all accepted formats in the content-type header …

36f6120

…value Accepted formats defined here: https://tools.ietf.org/html/rfc7231#section-3.1.1.1

davidor force-pushed the soap-policy branch from e959f79 to 36f6120 Compare February 8, 2018 16:31

mikz reviewed Feb 8, 2018

View reviewed changes

mikz and others added 2 commits February 8, 2018 17:46

primitive mime parser for extracting media types

65215ed

policy/soap: save media_type in lower-case

f08c1fb

davidor force-pushed the soap-policy branch from ed04f28 to d6b2d3e Compare February 8, 2018 17:07

policy/soap: delete unused method starts_with

a112ae1

davidor force-pushed the soap-policy branch from d6b2d3e to a112ae1 Compare February 8, 2018 18:09

spec/policy/soap: test case with full URL

2346bbd

davidor force-pushed the soap-policy branch from f342ebf to 2346bbd Compare February 8, 2018 19:53

davidor requested a review from mikz February 8, 2018 20:14

mikz approved these changes Feb 8, 2018

View reviewed changes

davidor merged commit 272aa57 into master Feb 9, 2018

davidor deleted the soap-policy branch February 9, 2018 09:31

SOAP policy #567

SOAP policy #567

Conversation

davidor commented Jan 30, 2018 • edited Loading

davidor commented Jan 30, 2018

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

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

davidor Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor Feb 7, 2018 • 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

davidor Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

mikz Feb 1, 2018 • 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

davidor commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor Feb 8, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

davidor commented Jan 30, 2018 •

edited

Loading

davidor Jan 31, 2018 •

edited

Loading

mikz Jan 31, 2018 •

edited

Loading

davidor Feb 7, 2018 •

edited

Loading

davidor Feb 1, 2018 •

edited

Loading

mikz Feb 1, 2018 •

edited

Loading

davidor Feb 8, 2018 •

edited

Loading