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.

Sign up for GitHub

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

Jump to bottom

[configuration] support passing configuration in Data URLs #593

Merged
merged 3 commits into from
Feb 15, 2018
Merged

[configuration] support passing configuration in Data URLs #593

merged 3 commits into from
Feb 15, 2018

Conversation

mikz
Copy link
Contributor

@mikz mikz commented Feb 14, 2018 •
edited
Loading

When APIcast is started with the development (--dev) environment and there is no other configuration passed (THREESCALE_PORTAL_ENDPOINT or APICAST_CONFIGURATION) it is going to default to service with echo policy defined in https://github.com/3scale/apicast/blob/6592772bac74d8fca8e148693f1b7f61e1b51a25/gateway/config/development.lua#L3-L18

As a side effect it is possible now pass the whole configuration in Data URL.

APICAST_CONFIGURATION=data:application/json;base64,<base64 encoded data> bin/apicast
APICAST_CONFIGURATION=data:application/json;<url encoded data> bin/apicast

@mikz mikz mentioned this pull request Feb 14, 2018
Merged
mikz
mikz commented Feb 14, 2018
View reviewed changes
gateway/src/apicast/configuration_loader.lua Outdated
@@ -33,6 +34,8 @@ function _M.load(host)
env.set('THREESCALE_CONFIG_FILE', uri.path)
elseif scheme == 'http' or scheme == 'https' then
env.set('THREESCALE_PORTAL_ENDPOINT', uri)
elseif scheme == 'data' then
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 could probably get away without the change in 3scale/lua-resty-url#7.
Just by using the new regex in the data_url loader and expose another method .match that verifies it is data url. Or just execute the loader and return the value if it returns something. That is probably wise.

And 3scale/lua-resty-url#7 for the future with proper benchmark.

@mikz mikz changed the title [wip] bunch of fixes/improvements for APIcast Cloud Hosted [configuration] support passing configuration in Data URLs Feb 14, 2018
mikz
mikz commented Feb 14, 2018
View reviewed changes
gateway/config/development.lua Outdated
configuration_loader = 'lazy',
configuration_cache = 0,
configuration = [[data:application/json;charset=utf-8;base64,eyJzZXJ2aWNlcyI6W3siaWQiOjEyMzQsImJhY2tlbmRfdmVyc2lvbiI6MSwicHJveHkiOnsiYXBpX2JhY2tlbmQiOiJodHRwOi8vMTI3LjAuMC4xOjgwODEiLCJob3N0bmFtZV9yZXdyaXRlIjoiZWNobyIsImhvc3RzIjpbImxvY2FsaG9zdCIsIjEyNy4wLjAuMSJdLCJiYWNrZW5kIjp7ImVuZHBvaW50IjoiaHR0cDovLzEyNy4wLjAuMTo4MDgxIiwiaG9zdCI6ImVjaG8ifSwicG9saWN5X2NoYWluIjpbeyJuYW1lIjoiYXBpY2FzdC5wb2xpY3kuYXBpY2FzdCJ9XSwicHJveHlfcnVsZXMiOlt7Imh0dHBfbWV0aG9kIjoiR0VUIiwicGF0dGVybiI6Ii8iLCJtZXRyaWNfc3lzdGVtX25hbWUiOiJoaXRzIiwiZGVsdGEiOjF9LHsiaHR0cF9tZXRob2QiOiJQT1NUIiwicGF0dGVybiI6Ii8iLCJtZXRyaWNfc3lzdGVtX25hbWUiOiJoaXRzIiwiZGVsdGEiOjF9LHsiaHR0cF9tZXRob2QiOiJQVVQiLCJwYXR0ZXJuIjoiLyIsIm1ldHJpY19zeXN0ZW1fbmFtZSI6ImhpdHMiLCJkZWx0YSI6MX1dfX1dfQ==]]
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 probably should create the data url here from a lua table -> json -> base64 so it is obvious what configuration is inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please.

mikz
mikz commented Feb 14, 2018
View reviewed changes
gateway/config/development.lua
@@ -1,7 +1,8 @@
return {
worker_processes = '1',
master_process = 'off',
lua_code_cache = 'off',
lua_code_cache = 'on',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise ngx.ctx can't be shared between requests and fails horribly.

@mikz mikz requested a review from davidor February 14, 2018 17:17
davidor
davidor reviewed Feb 14, 2018
View reviewed changes
gateway/src/resty/mime.lua Outdated

local self = {}

-- The RFC defines that the type can include upper and lower-case chars.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have a link to the RFC here for reference.

davidor
davidor reviewed Feb 14, 2018
View reviewed changes
gateway/src/resty/mime.lua Outdated


function _M.new(media_type)
local match = re_split(media_type, [[\s*;\s*]], 'oj', nil, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably good to explain why we're deleting whitespaces around ';'.

@mikz mikz force-pushed the apicast-cloud-hosted-fixes branch 2 times, most recently from d821eb5 to ede3cad Compare February 14, 2018 17:25
davidor
davidor reviewed Feb 14, 2018
View reviewed changes
gateway/src/resty/mime.lua Outdated
function _M:parameter(name)
local parameters = self.parameters

local matches = re_match(parameters, format([[%s=(?:"(.+?)"|([^\s;"]+))\s*(?:;|$)]], name), 'oji')
Copy link
Contributor

Choose a reason for hiding this comment

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

How about extracting that regex into its own var?
Maybe even explain in a line or two its purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

davidor
davidor reviewed Feb 14, 2018
View reviewed changes
gateway/config/development.lua Outdated
configuration_loader = 'lazy',
configuration_cache = 0,
configuration = string.format([[data:application/json,%s]],ngx.escape_uri(configuration)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a link here to https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mikz mikz requested a review from davidor February 14, 2018 17:42
davidor
davidor reviewed Feb 15, 2018
View reviewed changes
spec/configuration_loader/data_url_spec.lua Outdated
assert.same(config, loader.call(url))
end)

it('accepts bug ignores charset in the data url', function()
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

davidor
davidor reviewed Feb 15, 2018
View reviewed changes
gateway/src/apicast/configuration_loader/data_url.lua
@@ -0,0 +1,58 @@
local MimeType = require('resty.mime')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a module description explaining its purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@mikz mikz merged commit fd53a64 into master Feb 15, 2018
@mikz mikz deleted the apicast-cloud-hosted-fixes branch February 15, 2018 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidor davidor davidor approved these changes

Assignees

@mikz mikz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikz @davidor