Headers transformation by mmacai · Pull Request #76 · Accenture/reactive-interaction-gateway

mmacai · 2018-05-22T20:24:12Z

Fixed regex matching for proxy - previously /abc/def would match against /abc/def/1
Fixed response headers before response send - Elixir/Plug needs lower cased headers => led to duplicate headers and wrong responses
Added request headers transformations via add_headers, in future we can add remove_headers, etc.
Unit tests and smoke tests should cover everything

Steps to test:

create new API at POST localhost:4010/v1/apis with

{
  "id": "new-service2",
  "name": "new-service2",
  "auth_type": "jwt",
  "auth": {
    "use_header": true,
    "header_name": "Authorization",
    "use_query": false,
    "query_name": ""
  },
  "versioned": false,
  "version_data": {
    "default": {
    	"transform_request_headers": {
          "add_headers": {
            "a": "b",
            "c": "d"
          }
        },
		"endpoints": [
			{
				"id": "get-auth-register",
				"path": "/auth/register",
				"method": "GET",
				"not_secured": true,
				"transform_request_headers": true
			}
		]
    }
  },
  "proxy": {
    "use_env": true,
    "target_url": "IS_HOST",
    "port": 6666
  }
}

Send request to GET localhost:4000/auth/register with header a => proxy should overwrite it and add new header b
Omitting "transform_request_headers": true in endpoints will ignore header transformations

…licit occasions

kevinbader · 2018-05-24T19:17:33Z

  "versioned": false,
  "version_data": {
    "default": {

I don't really get this saying theres no versioning, but then configuring "version_data" ... I know it's done the same way in Tyk, and they probably had a reason for doing so, but it just looks very awkward to me 😅

kevinbader · 2018-05-24T19:21:57Z


+  # Transform request headers
+  @spec transform_req_headers(Proxy.endpoint(), Proxy.api_definition(), %Plug.Conn{}) ::
+          %Plug.Conn{}


I wonder, what's the difference between %Plug.Conn{} and Plug.Conn.t? I've seen the former, which refers to the module's defstruct quite often, but isn't the latter the real type we'd acutally would like to refer to? After all, the struct per se has only default values set, but no types. Might be wrong here. Any opinions?

Didn't know about Plug.Conn.t, but I think it makes sense since Strings are defined the same way.

kevinbader · 2018-05-24T19:25:37Z

+          %Plug.Conn{}
+  defp transform_req_headers(
+         %{"transform_request_headers" => true} = endpoint,
+         %{"versioned" => false} = api,


Can you please give me an RTFM link to what the meaning of "versioned" is here? Cause it's not obvious to me why versioning an endpoint would affect the headers 😕

My reasoning behind this is:

versioned: false => means you have only default API, no versions no magic logic around it, you care just about endpoints from default
versioned: true => you have multiple versions, you can't use only default, you have to take care about all versions

if you have versioned: true, you clearly have multiple versions of the same API => every version of API may require different headers transformation (thus how to transform is per version), every endpoint may or may not want do use this header transformation (thus endpoints have to decide if they want to use transformations)

but doesn't %{"transform_request_headers" => true} = endpoint say that the transformation should be enabled on this specific endpoint?

kevinbader · 2018-05-24T19:27:22Z

-    conn = %{conn | resp_headers: headers}
-    if Serializer.header_value?(conn, "transfer-encoding", "chunked") do
-      send_chunked_response(conn, headers, status_code, body)
+    down_cased_headers = headers |> Serializer.downcase_headers


hm it's downcase in the std lib, it's downcase in Serializer, so I'd go with downcased_headers here

kevinbader · 2018-05-24T19:28:17Z

+      send_chunked_response(conn, down_cased_headers, status_code, body)
    else
-      %{conn | resp_headers: headers} |> send_resp(status_code, body)
+      %{conn | resp_headers: down_cased_headers} |> send_resp(status_code, body)


isn't this replacing resp_headers a second time?

kevinbader · 2018-05-24T19:28:58Z

  alias Plug.Conn.Query
  alias RigInboundGateway.Proxy

+  @typep headers_list :: [{String.t, String.t}, ...]


I'd prefer either header_list or headers.

Also, can't old_headers be empty, ever? I mean, sending a request without any headers would look more like [{String.t, String.t}]

kevinbader · 2018-05-24T19:48:53Z

+    |> Enum.find({}, fn({header_key, _}) -> header_key == key end)
+    |> Tuple.to_list
+    |> Enum.member?(value)
+  end


hm this work unexpectedly for headers_list=[{"a", "b"}], key="a", value="a", because keys and values are mixed up.

May I suggest a slightly different implementation:

def header_value?(headers, key, value) do headers |> Enum.find(fn {^key, ^value} -> true _ -> false end) |> case do nil -> false _ -> true end end

Not sure about whether pattern matching makes it better or not, but the call to Tuple.to_list/1 definitely seems a bit odd here.

Nice catch, added test as well for it

kevinbader · 2018-05-24T19:50:20Z

+        key
        |> String.downcase
-      key_downcase == key
+      {key_downcase, value}


perhaps it would suffice to simply write

{String.downcase(key), value}

kevinbader · 2018-05-24T19:51:24Z

+    old_headers
+    |> Enum.filter(fn({old_key, _}) ->
+      new_headers
+      |> Enum.find(nil, fn({new_key, _}) -> old_key == new_key end)


nil is the default "default" value, no need to specify it explicitly

kevinbader · 2018-05-24T20:13:04Z

+      |> Enum.find(nil, fn({new_key, _}) -> old_key == new_key end)
+      |> is_nil
+    end)
+    |> Enum.concat(new_headers)


hm maybe I get this wrong, but it seems this

takes all old headers that don't occur in new_headers and

add the new headers to that list.
Would that be easier to read if written this way:

Map.merge(Map.new(old_headers), Map.new(new_headers)) |> Enum.to_list()

Definitely, totally overlooked it.

kevinbader · 2018-05-24T20:16:21Z

+    assert get_req_header(conn, "john") == []
+    assert conn.status == 200
+    assert conn.resp_body =~ "{\"status\":\"ok\"}"
+  end


it'd be nice to have a test around how it behaves with versioned set or unset

Since there is no support for versioning yet, it would crash with ** (FunctionClauseError) no function clause matching. Once we decide to add versioning it definitely makes sense.

kevinbader · 2018-05-25T09:17:46Z

+          %Plug.Conn{}
+  defp transform_req_headers(
+         %{"transform_request_headers" => true} = endpoint,
+         %{"versioned" => false} = api,


but doesn't %{"transform_request_headers" => true} = endpoint say that the transformation should be enabled on this specific endpoint?

kevinbader · 2018-05-25T09:28:40Z

+    conn = %{conn | resp_headers: downcased_headers}
+
+    if Serializer.header_value?(downcased_headers, "transfer-encoding", "chunked") do
+      send_chunked_response(conn, downcased_headers, status_code, body)


why are the headers passed here again - conn should already include them at this point

kevinbader · 2018-05-25T09:31:08Z

  alias Plug.Conn.Query
  alias RigInboundGateway.Proxy

+  @typep headers :: [{String.t, String.t}, ...]


What if old_headers is empty? I mean, sending a request without any headers would look more like [{String.t, String.t}]

mmacai added 3 commits May 20, 2018 21:13

fixed headers for reverse-proxy to downcase all headers and avoid dup…

88b446a

…licit occasions

added transformation logic for request headers

3531ce4

fixed reg ex match for proxy paths

ac143af

mmacai requested a review from kevinbader May 22, 2018 20:24

kevinbader self-assigned this May 24, 2018

kevinbader added this to the 2.0.0 milestone May 24, 2018

kevinbader suggested changes May 24, 2018

View reviewed changes

mmacai added 3 commits May 25, 2018 09:40

updated types and variable naming

ece9014

fixed headers logic and simplified pipes

5921806

removed unnecessary header modification before send

942d0d9

kevinbader reviewed May 25, 2018

View reviewed changes

mmacai added 3 commits May 25, 2018 12:16

simplified chunk reponse

9bfa2a5

generalized type for request headers

28e710e

added Not implemented handler for header transformations

c97aa5f

kevinbader approved these changes May 25, 2018

View reviewed changes

updated changelog

4cb74e9

mmacai merged commit ca83939 into master May 28, 2018

kevinbader deleted the headers-transformation branch May 30, 2018 13:57

Conversation

mmacai commented May 22, 2018

Uh oh!

kevinbader commented May 24, 2018

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!

mmacai May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mmacai May 25, 2018 •

edited

Loading