Akka http server responds HEAD request as GET by SugandhaAgrawal · Pull Request #4494 · apache/openwhisk

SugandhaAgrawal · 2019-05-27T07:42:58Z

Akka http client by default treats any http HEAD request as GET unless the configuration sets it otherwise. There are use cases where user expects HEAD to be responded as HEAD and without a response body unlike GET.

akka/akka-http#2088 mentions that the default is set to treat a HEAD as a GET request by the Akka client.

Description

Update the controller configuration to process HEAD and GET requests as they are expected.

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

chetanmeh · 2019-05-28T07:01:40Z

@SugandhaAgrawal I believe this is Akka HTTP Server setting and not related to client.

If we enable this would HEAD request still work in OpenWhisk or would fail. I presume so far they work just that internally executed as GET. However with this change would they stop working if route does not implement HEAD support?

markusthoemmes · 2019-05-28T07:20:06Z

It'd be useful to get some context on the use-cases that trigger this issue.

mbehrendt · 2019-05-28T07:30:12Z

the context of this issue is actually quite straightforward: We have seen users trying to implement a web action that responds differently on HEAD than on GET. However, given __ow_method always had the same value (get), they weren't able to make that distinction in their action code.

markusthoemmes · 2019-05-28T07:32:32Z

Got it, that makes sense. I guess @chetanmeh was confused (so was I) because the initial description talks about akka-https client not behaving as expected.

@chetanmeh is also right though that this is a breaking change as currently working HEAD requests will now fail.

mbehrendt · 2019-05-28T07:37:02Z

i wouldn't consider to be a breaking change per se, but rather a bug fix. It was just flatout wrong that when a HEAD request came in, __ow_method showed GET. Of course, if people relied on that bug to be true forever, they would break, but that is true for probably most other bug fixes as well :-)

SugandhaAgrawal · 2019-05-28T07:42:30Z

@markusthoemmes I have the same thought as Michael's. I would say this is a bug fix. The breaking change in terms that the users need to change the implementation is fine but this is right way to handle the HEAD request.

SugandhaAgrawal · 2019-05-28T07:49:14Z

@markusthoemmes The initial description does mean that akka http client isnt behaving as expected by default. It responds HEAD with a GET and that isnt the right response in my opinion.
In order to achieve this you need to set explicitly the value to off. My expectation was that the default should have been off or do you not find this as an expected result and correct?

markusthoemmes · 2019-05-28T07:52:04Z

@SugandhaAgrawal the point is that it's the akka http server, not the client.

I'm indecisive on the bug fix vs. breaking change. I agree though that the amount of breakage is very small and arguably relying on undocumented/wrong behavior.

If @chetanmeh doesn't have strong feelings against it I'm fine merging this as-is. Otherwise we should take it to the dev-list.

SugandhaAgrawal · 2019-05-28T08:02:17Z

@markusthoemmes Thanks for the correction, server ;)

chetanmeh

Given web actions need to distinguish the request it makes sense for this change.

Would be good to have a test to assert this behavior i.e. _ow_method is indeed HEAD post this change

SugandhaAgrawal · 2019-05-28T08:45:17Z

@chetanmeh I will include test in the next PR. I will add the output of my manual tests here for supporting the change.

chetanmeh · 2019-05-28T08:57:37Z

If this fix is not very urgent would it be possible to include the test in this PR itself. I believe you can still enable this flag in your setups via system property

rabbah · 2019-05-28T11:25:46Z

I agree this is a bug - the intended behavior is for the HEAD to reach the function just like OPTIONS. You'll need a system level tests (in WskWebActionsTests) that "curls" (i.e., makes an HTTP request to) the web action. I tested this patch locally and don't see a difference in behavior (a GET is posted with and without the patch). So +1 from me to include a test in this PR.

rabbah · 2019-05-28T11:27:34Z

@chetanmeh I'm not sure if existing actions would fail --- at least not all. I suspect that you need actions which have already opted out of the default CORS response.

rabbah · 2019-06-04T11:10:43Z

@SugandhaAgrawal any luck adding a test for this fix?

SugandhaAgrawal · 2019-06-05T14:24:06Z

@rabbah Unfortunately not. I was on vacation and returned today. Will work on it

rabbah · 2019-06-05T14:27:26Z

welcome back!

dubee

@SugandhaAgrawal, there should be some existing webaction tests that you can use as a template to test this one. Hopefully you can do a unit test for this, can't remember since I haven't looked at those in awhile. Integration test will definitely work though.

Nice find, BTW!

codecov-io · 2019-06-25T17:13:03Z

Codecov Report

Merging #4494 into master will decrease coverage by 3.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4494      +/-   ##
==========================================
- Coverage   83.65%   80.06%   -3.59%     
==========================================
  Files         174      174              
  Lines        8023     8023              
  Branches      552      552              
==========================================
- Hits         6712     6424     -288     
- Misses       1311     1599     +288

Impacted Files	Coverage Δ
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.89%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-74.08%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`4% <0%> (-52%)`	⬇️
...re/database/cosmosdb/CollectionResourceUsage.scala	`72.22% <0%> (-27.78%)`	⬇️
...in/scala/org/apache/openwhisk/common/Counter.scala	`40% <0%> (-20%)`	⬇️
...penwhisk/core/database/cosmosdb/CosmosDBUtil.scala	`81.81% <0%> (-15.16%)`	⬇️
...nwhisk/core/database/cosmosdb/CosmosDBConfig.scala	`94.11% <0%> (-5.89%)`	⬇️
...in/scala/org/apache/openwhisk/common/Logging.scala	`69.75% <0%> (-4.33%)`	⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d92be7...321a7d5. Read the comment docs.

SugandhaAgrawal · 2019-06-27T07:46:38Z

Travis System Test

rabbah · 2019-06-27T10:48:03Z

@@ -0,0 +1,9 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more contributor
+// license agreements; and to You under the Apache License, Version 2.0.


Please use the long form license. We have to stop using the short form.

Wasnt aware of the 2 versions of license headers. I have made the update.

it was a recent change.

thanks for the update!

rabbah

modulo the comment about auth.

rabbah · 2019-06-27T12:51:58Z

+    val url = s"$host$testRoutePath/$namespace/default/$name"
+
+    assetHelper.withCleaner(wsk.action, name) { (action, _) =>
+      action.create(name, file, web = Some("true"), annotations = Map("require-whisk-auth" -> true.toJson))


i think you can skip the require-whisk-auth annotation here.

rabbah · 2019-06-27T12:52:11Z

+    }
+
+    val unauthorizedResponse = RestAssured.given().config(sslconfig).head(url)
+    unauthorizedResponse.statusCode shouldBe 401


and then remove this unless this is important to the test.

rabbah

LGTM.

SugandhaAgrawal · 2019-07-01T15:00:34Z

@rabbah Please merge this, if everything looks good!

rabbah · 2019-07-01T15:02:52Z

Thanks @SugandhaAgrawal 👍

chetanmeh approved these changes May 28, 2019

View reviewed changes

SugandhaAgrawal changed the title ~~Akka http client responds HEAD request as GET~~ Akka http server responds HEAD request as GET Jun 5, 2019

dubee reviewed Jun 11, 2019

View reviewed changes

SugandhaAgrawal force-pushed the head_get_akka branch from e684e33 to 21b08d1 Compare June 14, 2019 14:50

SugandhaAgrawal force-pushed the head_get_akka branch 3 times, most recently from f2ba1ca to 321a7d5 Compare June 25, 2019 14:14

rabbah reviewed Jun 25, 2019

View reviewed changes

Comment thread tests/src/test/scala/org/apache/openwhisk/core/cli/test/WskWebActionsTests.scala Outdated

Comment thread tests/src/test/scala/org/apache/openwhisk/core/cli/test/WskWebActionsTests.scala

SugandhaAgrawal force-pushed the head_get_akka branch from 321a7d5 to 9e713d3 Compare June 26, 2019 12:28

rabbah reviewed Jun 27, 2019

View reviewed changes

Sugandha Agrawal added 2 commits June 27, 2019 14:39

Keep HTTP head as head by akka client setting change

d2246cf

Added test

608d250

Add test

a9b342c

SugandhaAgrawal force-pushed the head_get_akka branch from 9e713d3 to de1678d Compare June 27, 2019 12:42

rabbah approved these changes Jun 27, 2019

View reviewed changes

Test enhancement

4549e0b

SugandhaAgrawal force-pushed the head_get_akka branch from de1678d to 4549e0b Compare June 27, 2019 12:58

rabbah approved these changes Jun 27, 2019

View reviewed changes

rabbah merged commit 740a4bf into apache:master Jul 1, 2019

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Allow actions to respond to HEAD requests (apache#4494)

2ea76f2

mkyutani mentioned this pull request Jul 4, 2021

Waiting for the Whisk invoker without HEAD but GET apache/openwhisk-devtools#334

Open

		@@ -0,0 +1,9 @@
		// Licensed to the Apache Software Foundation (ASF) under one or more contributor
		// license agreements; and to You under the Apache License, Version 2.0.

Conversation

SugandhaAgrawal commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

chetanmeh commented May 28, 2019

Uh oh!

markusthoemmes commented May 28, 2019

Uh oh!

mbehrendt commented May 28, 2019

Uh oh!

markusthoemmes commented May 28, 2019

Uh oh!

mbehrendt commented May 28, 2019

Uh oh!

SugandhaAgrawal commented May 28, 2019

Uh oh!

SugandhaAgrawal commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusthoemmes commented May 28, 2019

Uh oh!

SugandhaAgrawal commented May 28, 2019

Uh oh!

chetanmeh left a comment

Choose a reason for hiding this comment

Uh oh!

SugandhaAgrawal commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chetanmeh commented May 28, 2019

Uh oh!

rabbah commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabbah commented May 28, 2019

Uh oh!

rabbah commented Jun 4, 2019

Uh oh!

SugandhaAgrawal commented Jun 5, 2019

Uh oh!

rabbah commented Jun 5, 2019

Uh oh!

dubee left a comment • 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!

codecov-io commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

SugandhaAgrawal commented Jun 27, 2019

Uh oh!

rabbah Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

SugandhaAgrawal Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rabbah Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rabbah left a comment

Choose a reason for hiding this comment

Uh oh!

rabbah Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rabbah Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rabbah left a comment

Choose a reason for hiding this comment

Uh oh!

SugandhaAgrawal commented May 27, 2019 •

edited

Loading

SugandhaAgrawal commented May 28, 2019 •

edited

Loading

SugandhaAgrawal commented May 28, 2019 •

edited

Loading

rabbah commented May 28, 2019 •

edited

Loading

dubee left a comment •

edited

Loading

codecov-io commented Jun 25, 2019 •

edited

Loading