feat(plugin): azure serverless functions by bisakhmondal · Pull Request #5479 · apache/apisix

bisakhmondal · 2021-11-10T19:30:14Z

What this PR does / why we need it:

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

tokers · 2021-11-17T01:30:27Z

+function _M.access(conf, ctx)
+    local uri_args = core.request.get_uri_args(ctx)
+    local headers = core.request.headers(ctx) or {}
+    local req_body, _ = core.request.get_body()


What if the request body are large and we cannot load it into memory totally?

The body will be missing when it is larger than client_body_buffer_size (8k by default)
We can read the body by ourselves via ngx.req.socket? See https://github.com/openresty/lua-resty-upload/blob/master/lib/resty/upload.lua

We can do it in the next PR.

I just realize it uses core.request.get_body, which will read data from file if need. BTW, we should check the err returned by this function.

thanks for pointing it out @tokers!
@spacewander even if it reads the content from a file, it reads everything in one go

apisix/apisix/core/request.lua

Lines 172 to 181 in 2c12b36

local function get_file(file_name)

local f, err = io_open(file_name, 'r')

if not f then

return nil, err

end

local req_body = f:read("*all")

f:close()

return req_body

end

I think we should use chunked transfer encoding here to proxy the request with a large body size. WDYT?

I think normally people don't need to submit big data to azure functions. We can keep this function unchanged for now.

Agree, but it's better to add some hints in the document. @bisakhmondal @spacewander

spacewander · 2021-11-17T01:28:31Z

+    local res, err = httpc:request_uri(conf.function_uri, params)
+
+    if not res or err then
+        return core.response.exit(500, "failed to process azure function, err: " .. err)


Suggested change

return core.response.exit(500, "failed to process azure function, err: " .. err)

core.log.error("failed to process azure function, err: " .. err)

return 503

I think it will be safer not to return an internal error to the client.

spacewander · 2021-11-17T01:29:38Z

  - response-rewrite               # priority: 899
  #- dubbo-proxy                   # priority: 507
  - grpc-transcode                 # priority: 506
+  - azure-functions                # priority: 505


Better to put it ahead of serverless-post-function

What should be the priority value, negative?

priority: -1900?

spacewander · 2021-11-17T01:35:58Z

+add_block_preprocessor(sub {
+    my ($block) = @_;
+
+    if (!defined $block->additional_http_config) {


We can use http_config to inject HTTP config, see https://github.com/apache/apisix/blob/master/t/discovery/consul_kv.t

And I suggest you put azure-demo as

apisix/t/lib/server.lua

Line 33 in 2c12b36

function _M.hello()

There is no need to introduce a new server.

Done, Is it okay now?

spacewander · 2021-11-17T05:02:36Z

+        headers["x-functions-key"] = conf.authorization.apikey or ""
+        headers["x-functions-clientid"] = conf.authorization.clientid or ""
+    else
+        headers["x-functions-key"] = getenv(env_key.API)


We need to expose the AZURE_FUNCTIONS_APIKEY via env AZURE_FUNCTIONS_APIKEY, if this plugin is enabled. Nginx will hide all environment variables by default.

You mean like this env AZURE_FUNCTIONS_APIKEY="key" make run?
Or you are suggesting to update something?

Like

apisix/apisix/cli/ngx_tpl.lua

Line 52 in 2c12b36

env PATH; # for searching external plugin runner's binary

, but only when this plugin is enabled:

apisix/apisix/cli/ngx_tpl.lua

Line 209 in 2c12b36

{% if enabled_plugins["proxy-cache"] then %}

I change my mind. Maybe it could be easier to pass this argument via plugin metadata.

Metadata won't be a great fit, IMHO. There might be multiple instances of plugins linked to different routes and we can't force users to use the same key they put into the metadata schema(must be the master key, instead of function-specific keys). This clearly violates principle of Least Access Privileges, isn't it?

Removed env vars and added metadata as a fallback option

spacewander · 2021-11-17T08:03:33Z

+        method = ngx.req.get_method(),
+        body = req_body,
+        query = uri_args,
+        headers = headers,


Do we need to remove the host header? Otherwise, the host of APISIX will be used.

Done, It was the root of many problem.

spacewander · 2021-11-18T01:05:19Z

+=== TEST 5: http2 check response body and headers
+--- http2
+--- exec
+curl -XGET --http2 --http2-prior-knowledge localhost:1984/azure


We can use regular --- request to send http2 request, see
https://github.com/openresty/test-nginx/blob/9d5a20f884d67974f9eafeed5a1540cd32945fb2/lib/Test/Nginx/Socket.pm#L2310

Nice, it is. Updated

Co-authored-by: 罗泽轩 <[email protected]>

bzp2010 · 2021-11-18T08:12:56Z

I have a question, should we standardize the naming of all serverless plugins? such as xxx-function or xxx-serverless 🤔

spacewander · 2021-11-18T08:40:38Z

@bzp2010
I think we can use the product name directly (azure-functions in this case). It is easier to be searched.

bisakhmondal changed the title ~~feat: azure serverless functions plugin support~~ feat(plugin): azure serverless functions Nov 10, 2021

bisakhmondal force-pushed the azure-functions branch from a744b18 to 96f2fa0 Compare November 10, 2021 19:36

bisakhmondal added 2 commits November 16, 2021 17:45

azure function access phase

6b0a755

next iteration with test code

3712d80

bisakhmondal force-pushed the azure-functions branch from 96f2fa0 to 3712d80 Compare November 16, 2021 14:36

bisakhmondal marked this pull request as ready for review November 16, 2021 14:36

remove debug code

36eca09

tokers reviewed Nov 17, 2021

View reviewed changes

spacewander reviewed Nov 17, 2021

View reviewed changes

suggestions applied

02aa478

spacewander reviewed Nov 17, 2021

View reviewed changes

bisakhmondal added 3 commits November 17, 2021 19:34

plugin metadata, new tests, http2 header rewriting

92a70cb

documentation

fac88fd

priority fixed

44276bf

bisakhmondal requested a review from spacewander November 17, 2021 16:02

spacewander reviewed Nov 18, 2021

View reviewed changes

bisakhmondal and others added 2 commits November 18, 2021 10:38

remove "Host" header. Suggested by @spacewander

25601d5

Co-authored-by: 罗泽轩 <[email protected]>

update test case behaviour

eb4bcf1

membphis reviewed Nov 18, 2021

View reviewed changes

Comment thread apisix/plugins/azure-functions.lua Outdated

error syntax fix

76f7753

spacewander approved these changes Nov 18, 2021

View reviewed changes

tzssangglass approved these changes Nov 18, 2021

View reviewed changes

spacewander requested a review from tokers November 19, 2021 00:44

spacewander mentioned this pull request Nov 19, 2021

feat: add Apache OpenWhisk plugin #5518

Merged

4 tasks

membphis merged commit 3a6a4db into apache:master Nov 19, 2021

	local function get_file(file_name)
	local f, err = io_open(file_name, 'r')
	if not f then
	return nil, err
	end

	local req_body = f:read("*all")
	f:close()
	return req_body
	end

	return core.response.exit(500, "failed to process azure function, err: " .. err)
	core.log.error("failed to process azure function, err: " .. err)
	return 503

Uh oh!

Conversation

bisakhmondal commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bzp2010 commented Nov 18, 2021

Uh oh!

spacewander commented Nov 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bisakhmondal commented Nov 10, 2021 •

edited

Loading