PDK: updating Service Request data updates also the Request data

Questions
1

I just submitted a PR (https://github.com/Kong/kong/pull/3840) to add getters to the PDK’s Service Request (pdk.service.request).

After digging into this, I realized that I have a concern related to the relation between:

  • the setters of the Service Request,
  • and the getters of Request and Service Request

In fact, the underlying Nginx data that are used to store some of the information (notably the query parameters and the HTTP headers and body) are the same whatever we are using the pdk.request or pdk.service.request. This might result in confusions if, for example, the sequence below is followed:

1/ Within a plugin, get data -for example the HTTP Headers- from the Request (using pdk.request.get_headers()).

2/ Then set the HTTP headers of the Service Request using pdk.service.request.set_headers(headers). At this stage, reading the Headers from the Service Request will obviously return the new set of headers just provided in the set_headers call…

3/ …but, much more confusing, reading the Headers of the Request as in step 1/ will now return the new set of Headers, and not the original one (while the “Request” is obviouly unchanged; only the Service Request is)!

This is very confusing for Plugin developers. I have the feeling that Request data should remain immutable, and setting a data in the Service Request should not affect the data of the Request.

For sure, in proxy mode, Nginx mandates the usage of specific data or functions to define the upstream data, so Kong has no other choice than setting these data (using ngx.req.set_uri_args for the query parameters, ngx.req.set_headers() for the headers, etc.). But when used in the PDK’s Service Request context, calling these functions also updates the Request’s original data :frowning: => should Kong PDK’s rather work on “copies” of the data?

  • Either, similarly to the ngx.var.upstream_host and ngx.var.upstream_uri that are used to compute some of the upstream data, we might define ngx.var.upstream_args, ngx.var.upstream_headers, ngx.var.upstream_body data that would be used by the Service Request setters.
    They would be initialized at request reception, by copying the Request data. And then, before proxying to upstream, Kong would copy back these data to the relevant Nginx variables.

  • Or we might consider that the Nginx data should be used to set the upstream data in the PDK’s Service Request, and we might rather have new ngx.var.downstream_args, ngx.var.downstream_headers, ngx.var.downstream_body data… that would be copies of the original Request data. These new data would be set at the request reception and used by the Request getters only (no setters: they are immutable).

In both cases, plugins would always be able to get the original Requests’ data (whatever the phase they are in), and plugins that update data for upstream services (such as the Request transformer plugin) would have the capability to get and set Service Request’s data. As we have a chain of plugins, it is important for these plugins to get data from the Service Request first (may have been generated/updated by a previous plugin in the chain), and then set the updated value.

What is you feeling about this?

I did not had a look to Service Response and Response… but maybe this is the same issue…

2

Hi @pamiel,

Thank you for the PR! Nice job :slight_smile:

Now, to answer your concerns:

should Kong PDK’s rather work on “copies” of the data?

plugins would always be able to get the original Requests’ data

Yes indeed, so is our vision. Portions of the current implementation of the PDK already rely on read-only values to provide this encapsulation of the original request/response data. Other portions explicitly triage the received data from the Kong-injected one (e.g. https://github.com/Kong/kong/blob/0.14.1/kong/pdk/service/response.lua#L128-L130). Specifically, kong.request and kong.service.response should always provide immutable data. On the contrary, kong.service.request and kong.response should always provide a state of what the request/response would look like if it were to be proxied away by Kong at that exact moment.

We would like to ensure that this encapsulation is enforced in the future, even in places where it may currently be lacking. Now that our initial abstraction is in place, it should be possible for us to offer this guarantee with no breaking changes in the PDK :slight_smile:

Another reason for maintaining Lua-land copies of the request/response data is performance: instead of crossing the Lua/C boundary upon each invocation of a getter, we should observe significant performance improvements in scenarios where multiple plugins on a given request/response path perform operations such as reading the requests headers or payloads.