Added header when reporting failing invocations or initializations #116 #128

Ro-M · 2020-06-13T21:48:26Z

Motivation

This fixes #116

We need to send a special header when we are reporting a failing invocation or when the runtime encounters an error during initialization.

Changes

The lambda-runtime-function-error-type header is added by the RuntimeClient. As of my understanding this should not be done by the HTTPClient
To allow this I added the option to provide additional headers in the post method of HTTPClient. Those headers will be added to the default headers of the HTTPClient
For testing purposes I opted into using the NIOHTTP1TestServer instead of the MockLambdaServer.
I am using RuntimeEngine's baseURL parameter as the primary source to set the ip and port property, falling back to searching in the environment if it wasn't provided. baseURL did exist as a parameter before but was unused. I needed a settable baseURL for the NIOHTTP1TestServer

Discussion

If we are adding an additionalHeaders parameter to the post method of the HTTPClient, should this parameter also be available in the get method? (It's not needed needed for this PR)
If using the NIOHTTP1TestServer is ok but evaluating the inbound reads and outbound writes is too much boilerplate code: Should this evaluation be refactored into helper methods? (similar helper methods are internally used in the swift-nio package)
Concerning the baseURL parameter of RuntimeEngine's initializer: Directly mapping a url to the ip and port property does not sound right: I would not expect an ip property to contain a url. Maybe rename ip to host?

If we want to use The NIOHTTP1TestServer for testing connections we need to use its ip and port. The default (currently unchangeable) port of the RuntimeEngine is 7000, the one of of NIOHTTP1TestServer is 0

…into rm-bug116

- use runtimClient instead of runner - remove extensions for requests and test directly in the method

swift-server-bot · 2020-06-13T21:48:29Z

Can one of the admins verify this patch?

swift-server-bot · 2020-06-13T21:48:29Z

Can one of the admins verify this patch?

swift-server-bot · 2020-06-13T21:48:29Z

Can one of the admins verify this patch?

swift-server-bot · 2020-06-13T21:48:29Z

Can one of the admins verify this patch?

fabianfett · 2020-06-13T22:00:40Z

@swift-server-bot test this please

…into rm-bug116

fabianfett

@Ro-M Thanks for this great pr. Have you tested this by any chance on Lambda itself?

I think we need to clarify the HTTPClients API first. Should the HTTPClient have static headers it merges with injected once or should the Lambda.RuntimeClient inject all headers on every call.

I think I would vote for the later but let's wait what @tomerd thinks.

fabianfett · 2020-06-13T22:10:39Z

Sources/AWSLambdaRuntimeCore/HTTPClient.swift

+    func post(url: String, body: ByteBuffer?, timeout: TimeAmount? = nil, additionalHeaders: HTTPHeaders? = nil) -> EventLoopFuture<Response> {
+        var headers = HTTPClient.headers
+        additionalHeaders.flatMap { headers.add(contentsOf: $0) }


@tomerd I would vote to inject all headers on every call and to not have them split over two files. wdyt? In hindsight having static headers directly on the HTTPClient doesn't make much sense IMHO.

hi @fabianfett do you mean have RuntimeClient fully control the headers instead of injecting "additional" ones? if so, that sounds good to me.

Would it be ok to include that change in this PR or should I make a separate one for this?

I suggest we fix it in this PR

Ok. I moved the static header declaration into an extension on RuntimeClient. The get and post methods on HTTPClient now have a non-optional headers parameter (without a default value). I also moved the headers parameter to come after the urlparameter ... that ordering made a more cohesive impression to me ^^

fabianfett · 2020-06-13T22:15:11Z

Tests/AWSLambdaRuntimeCoreTests/LambdaRuntimeClientTest.swift

+        XCTAssertNoThrow(try result.wait())
+    }
+
+    func testSuccessHeaders() {


I think I would rename this to testInvocationResponse because it isn't as generic as SuccessHeaders

Right. I think Headers is generally wrong at this point because the method is also testing a lot of other things as well.

Is testInvocationSuccessResponseok with you as well? To disambiguate between success and error tests

@Ro-M yes testInvocationSuccessResponse is perfect

tomerd · 2020-06-16T00:24:47Z

@swift-server-bot test this please

tomerd · 2020-06-16T00:26:40Z

Concerning the baseURL parameter of RuntimeEngine's initializer: Directly mapping a url to the ip and port property does not sound right: I would not expect an ip property to contain a url. Maybe rename ip to host?

happy to explore other names but keep in mind that Lambda will always bind to a localhost IP address, and AWS provides that as an env variables that combines the two as parsed by the code

Ro-M · 2020-06-16T10:27:47Z

Lambda will always bind to a localhost IP address

@tomerd I didn't think about that, thanks!
So ip and port seem to definitely be the correct names in that scenario and baseURL should be removed I guess?
If we keep ip and port, do we want to only set them through environment variables or do we want to provide a parameter (or multiple) to optionally set those properties via an initializer instead of the environment?

I would prefer the latter approach: providing the option to set them through an initializer but falling back to the environment and also default the ip and port parameters to nil. But on the other hand I can see the reasoning behind only using the environment if AWS is doing it that way ...

tomerd · 2020-06-16T17:58:36Z

If we keep ip and port, do we want to only set them through environment variables or do we want to provide a parameter (or multiple) to optionally set those properties via an initializer instead of the environment?

I would prefer the latter approach: providing the option to set them through an initializer but falling back to the environment and also default the ip and port parameters to nil. But on the other hand I can see the reasoning behind only using the environment if AWS is doing it that way ...

using only env variables makes testing hard, so I also prefer the approach of being able to set it via the config and falling back to the env variable

name wise, maybe "address"?

tomerd · 2020-06-16T18:10:59Z

@swift-server-bot test this please

tomerd · 2020-06-17T00:04:50Z

Sources/AWSLambdaRuntimeCore/LambdaRuntimeClient.swift

 import NIOHTTP1

+
+


remove empty lines

tomerd · 2020-06-17T00:05:06Z

Sources/AWSLambdaRuntimeCore/LambdaRuntimeClient.swift

        private let allocator = ByteBufferAllocator()
        private let httpClient: HTTPClient
-
+        


run ./scripts/sanity

fabianfett · 2020-06-17T09:48:28Z

@swift-server-bot test this please

fabianfett

Thanks @Ro-M for contributing. This looks really good now!

tomerd · 2020-06-17T13:55:48Z

thanks @Ro-M

Ro-M added 13 commits June 12, 2020 13:17

allow the users of client to provide headers

2841197

add initialization error header

4162c6b

add error header to failed invocations

a207384

add error header to failed invocations

03c33da

enable external setting of ip and port of RuntimeEngine

d75e2f2

If we want to use The NIOHTTP1TestServer for testing connections we need to use its ip and port. The default (currently unchangeable) port of the RuntimeEngine is 7000, the one of of NIOHTTP1TestServer is 0

test headers in failed initialization

5c81588

Merge branch 'rm-bug116' of github.com:Ro-M/swift-aws-lambda-runtime …

8da17fd

…into rm-bug116

use baseUrl instead of ip in RuntieEngine

1e57c4b

refactor test

f8da566

- use runtimClient instead of runner - remove extensions for requests and test directly in the method

invocation error test

72c2b8e

remove header change from get method

d0b3712

test that previous headers are present

196b335

check success case does not have error headers

1ccd68e

Ro-M and others added 2 commits June 13, 2020 23:56

swiftformat

c520b22

Merge branch 'master' into rm-bug116

f5bcbd7

Ro-M added 2 commits June 14, 2020 00:09

removed unneeded @testable

e5dd619

Merge branch 'rm-bug116' of github.com:Ro-M/swift-aws-lambda-runtime …

5f5f09c

…into rm-bug116

fabianfett reviewed Jun 13, 2020

View reviewed changes

fabianfett requested a review from tomerd June 13, 2020 22:27

Merge branch 'master' into rm-bug116

eabbc10

tomerd added this to the 0.1.1 milestone Jun 16, 2020

Merge branch 'master' into rm-bug116

c375ab6

Ro-M added 3 commits June 17, 2020 00:22

rename test method

84e19d2

rename runtimeEngines baseUrl to address

60c4dc4

move headers from HTTPClient into RuntimeClient

24c9d1b

tomerd self-assigned this Jun 16, 2020

tomerd reviewed Jun 17, 2020

View reviewed changes

Sources/AWSLambdaRuntimeCore/LambdaRuntimeClient.swift Outdated

import NIOHTTP1

Copy link

Contributor

tomerd Jun 17, 2020

Choose a reason for hiding this comment

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

remove empty lines

tomerd reviewed Jun 17, 2020

View reviewed changes

run sanity script

d8a18e1

fabianfett approved these changes Jun 17, 2020

View reviewed changes

tomerd approved these changes Jun 17, 2020

View reviewed changes

tomerd merged commit 7728066 into awslabs:master Jun 17, 2020

		private let allocator = ByteBufferAllocator()
		private let httpClient: HTTPClient

Added header when reporting failing invocations or initializations #116 #128

Added header when reporting failing invocations or initializations #116 #128

Uh oh!

Conversation

Ro-M commented Jun 13, 2020

Motivation

Changes

Discussion

Uh oh!

swift-server-bot commented Jun 13, 2020

Uh oh!

swift-server-bot commented Jun 13, 2020

Uh oh!

swift-server-bot commented Jun 13, 2020

Uh oh!

swift-server-bot commented Jun 13, 2020

Uh oh!

fabianfett commented Jun 13, 2020

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jun 16, 2020 • 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!

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!

tomerd commented Jun 16, 2020

Uh oh!

tomerd commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ro-M commented Jun 16, 2020

Uh oh!

tomerd commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Jun 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett commented Jun 17, 2020

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tomerd Jun 16, 2020 •

edited

Loading

tomerd commented Jun 16, 2020 •

edited

Loading

tomerd commented Jun 16, 2020 •

edited

Loading