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

Extract MappingRulesMatcher and Usage modules #580

Merged
merged 11 commits into from
Feb 7, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Feb 7, 2018

This PR:

  • Extracts a Usage module. This allows us to 'merge' usages hiding the internal data structures.
  • Extracts a MappingRulesMatcher. This will allow us to match rules from policies.

These 2 things are needed to implement the SOAP policy (#567)

This will allow us to add usages independently of the implementation.
This simplifies the Service module because it no longer needs to be
responsible for matching mapping rules and collecting the usage.
@davidor davidor requested a review from mikz February 7, 2018 09:32
@davidor davidor force-pushed the extract-rules-matcher-and-usage branch from 5dfe4ce to 48238a1 Compare February 7, 2018 09:35
@@ -126,6 +127,31 @@ local function output_debug_headers(service, usage, credentials)
end
end

-- Converts a usage to the format expected by the 3scale backend client.
local function parse_usage(usage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nitpick. I think this should be called serialize_usage or format_usage. It is not parsing, but rather transforming, from one format to another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


var.cached_key = concat(cached_key, ':')

if debug_header_enabled then
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always true because debug_header_enabled is a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Forgot the ().

-- save those tables in context so they can be used in the backend client
ctx.usage = usage_params
ctx.usage = parsed_usage
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something we will change in the future, right? Exposing the real usage object rather than this.

How the SOAP policy could add usage when the Usage object is not exposed?

Copy link
Contributor Author

@davidor davidor Feb 7, 2018

Choose a reason for hiding this comment

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

Yes. This is related to what I mentioned in the PR description.

There are some changes we need to make here. First, we need to make some changes so Proxy receives the context via params in .rewrite. Second, it will need to merge with the usage in the context rather than just assigning it, and third, we'll need to move this formatting closer to the call to the backend client call. The context will always contain instances of Usage.

I realized that making the changes I mentioned above some tests break because they're too tied to the implementation details.

As you can see, in order to properly address this I'll need to change a few things and I felt there was already too much in this PR. I left it for the next so we can focus here in the MappingRulesMatcher and Usage that will be exposed and potentially used from other modules.

Copy link
Contributor Author

@davidor davidor Feb 7, 2018

Choose a reason for hiding this comment

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

I forgot to mention that the current version of the code also saves in ctx.usage the formatted usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added 3 new commits that fix what we discussed. In the end I decided to include them here to make sure that the approach taken works well.

The .parsed_usage() method will be moved to the backend client in a
future refactor.
@davidor davidor force-pushed the extract-rules-matcher-and-usage branch from 0271671 to 65f0d46 Compare February 7, 2018 11:11
These tests were passing just because proxy:authorize was being called
with the wrong arguments. We were sending a table with usages, but in
reality, this method expects a string with the usage formatted.
This allows other policies to add to the usage that will be authorized
and reported against 3scale's backend.
context.usage = context.usage or Usage.new()
context.usage:merge(usage)

ctx.usage = usage
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now both proxy and the Apicast policy depend on ctx and context. We'll need to change this to make this cleaner.

Copy link
Contributor

@mikz mikz Feb 7, 2018

Choose a reason for hiding this comment

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

So ctx.usage will be the formatted one and context.usage will be our Usage object for now, right?

Both ctx.usage and context.usage are going to be instance of Usage.

@@ -264,15 +293,22 @@ function _M:rewrite(service)
local var = ngx.var

-- save those tables in context so they can be used in the backend client
ctx.usage = usage_params
context.usage = context.usage or Usage.new()
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be provided by export, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Unfortunately, there's a bug in export(). It does not work correctly on service-level policies.
Something we'll need to fix soon :)

Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

👍

@davidor davidor merged commit bf2299b into master Feb 7, 2018
@davidor davidor deleted the extract-rules-matcher-and-usage branch February 7, 2018 13:03
@@ -126,10 +128,37 @@ local function output_debug_headers(service, usage, credentials)
end
end

-- Converts a usage to the format expected by the 3scale backend client.
local function format_usage(usage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it could be a method of usage.lua? Like self.format.
It would also facilitate its testing.

Copy link
Contributor Author

@davidor davidor Feb 7, 2018

Choose a reason for hiding this comment

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

I'd rather leave the responsibility of dealing with this to the backend client.

The reason is that, otherwise, we'll need to include a similar method on each of our 'model' classes that need to be sent in the params of a request.

In a future refactor, this format_usage() will be moved somewhere else.

@davidor davidor mentioned this pull request Feb 8, 2018
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

Successfully merging this pull request may close these issues.

3 participants