Skip to content

Fix issue with header prefixes, remove ambiguous header processing#105

Merged
zootalures merged 1 commit intomasterfrom
oc/tempFixHttpHeaders
May 4, 2020
Merged

Fix issue with header prefixes, remove ambiguous header processing#105
zootalures merged 1 commit intomasterfrom
oc/tempFixHttpHeaders

Conversation

@zootalures
Copy link
Copy Markdown
Member

@zootalures zootalures commented Apr 30, 2020

The python FDK has ambiguous and incorrect behaviour around header processing:

  • It incorrectly processes HTTP gateway headers, using lstrip instead of substring: this causes strange behaviour when an HTTP header starts with any strings matching [fnhtp-]+ and where that string is stripped instead of the actual http prefix "fn-http-h" (e.g. fn-http-h-good -> "good" but fn-http-h-hello -> "ello")
  • It incorrectly collapses HTTP headers into the function headers - this makes it impossible for a user to capture the exact HTTP headers sent to an API gateway as they are intermingled with the underlying function call headers
  • It ambiguously processes headers when this collapsing happens - (based on dictionary order) - so if I have two headers from functions "fn-http-h-myheader" "myheader" it may present either depending on the order the keys are evaluated in the dictionary

This fix resolves all three problems.

For the second problem, to avoid breaking an backwards compatibility I've added a ctx.HTTPHeaders() method which only returns the incoming request headers.

The testing behaviour was also ambiguous as it relies on the internal code in the FDK that is under test so I added a raw testing call that does not apply the internal transformations to headers to the test fixture.

Rather than using the function boundary to assert data I added a global capture to the fixture to retrieve the exact context presented to the function for testing.

@zootalures zootalures requested a review from tcoupland April 30, 2020 15:08
@zootalures zootalures force-pushed the oc/tempFixHttpHeaders branch 2 times, most recently from fb25fe5 to c5e6c2f Compare April 30, 2020 15:16
Comment thread fdk/fixtures.py Outdated
return await setup_fn_call_raw(handle_func, content, new_headers)


async def setup_fn_call_raw(handle_func, content=None, headers={}):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
async def setup_fn_call_raw(handle_func, content=None, headers={}):
async def setup_fn_call_raw(handle_func, content=None, headers=None):
if headers is None:
headers = {}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed.

@zootalures zootalures force-pushed the oc/tempFixHttpHeaders branch from c5e6c2f to ef8efd6 Compare April 30, 2020 17:13
Copy link
Copy Markdown
Contributor

@reclaro reclaro left a comment

Choose a reason for hiding this comment

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

LGTM I am wonder if we can add a small test of the http override where we highlight the precedence of http headers over functions ones

Comment thread fdk/headers.py
@zootalures zootalures force-pushed the oc/tempFixHttpHeaders branch 2 times, most recently from 98dd188 to 0ad1f5b Compare May 1, 2020 14:38
Comment thread fdk/headers.py Outdated
if headers is not None:
for k, v in headers.items():
k = k.lower()
print("looking at %s, %s-> %s" % (k, v, new_headers))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: i guess this is a leftover from the a debug session

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ooh yeah
thanks

Copy link
Copy Markdown
Contributor

@reclaro reclaro left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests and the logic to merge headers to we don't lose any value. The logic is a bit complicated and not perfect but this PR is fixing 2 bugs so i think we should merge it. There are a couple of unecessary print left in the code that should be removed.

Comment thread fdk/tests/test_headers.py Outdated
@zootalures zootalures force-pushed the oc/tempFixHttpHeaders branch from 0ad1f5b to 22e7d97 Compare May 2, 2020 17:32
@zootalures zootalures force-pushed the oc/tempFixHttpHeaders branch from 22e7d97 to f5ef6a2 Compare May 2, 2020 17:40
@zootalures zootalures merged commit 2750e96 into master May 4, 2020
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