Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

akka.http.server.transparent-head-requests should be opt-in, not opt-out #2088

Closed
radekg opened this issue Jun 22, 2018 · 9 comments
Closed
Assignees
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted t:core Issues related to the akka-http-core module
Milestone

Comments

@radekg
Copy link

radekg commented Jun 22, 2018

Hi akka team,

I just experienced the akka.http.server.transparent-head-requests situation and I was actually stunned to learn that the default value is on. The situation got a little bit out of control, I did finally manage to get it under control, but let me describe the complete scenario.

I am writing a HTTP service on top of akka-http. One of the tasks of my service is to communicate with S3, I shield S3 getObject and doesObjectExist. S3 getObject is GET /bucket/path, doesObjectExist is HEAD /bucket/path. For integration tests, I'm using the https://github.com/findify/s3mock library, which is also built on top of akka-http. The author of the library chosen not to implement head routes, one uses this default setting for soft fall back. I can understand author's reasoning, saves quite a lot of work. However, my service needs to handle head and get operations, I do need to have the setting set to off.

I do believe that the default behavior should be opt-in (default off), not opt-out. If a library author wants to fall back to HEAD -> GET, they should be forced to do this explicitly in their own configuration. As a regular user, I do expect that my HTTP service handles HTTP HEAD right from the start, without having to mess with actor system settings, which in this case means doing:

private implicit val actorSystem = ActorSystem(
    "my-actor-system",
    config
      .getConfig("my.app")
      .withOnlyPath("akka")
      .withFallback(config)
)
@raboof
Copy link
Member

raboof commented Jun 26, 2018

There is some more background to this decision in akka/akka#18020. While I can see how this could be confusing it seems like the right default to me.

Do I understand correctly that your specific problem popped up because you are disabling transparent-head-requests in your application, but s3mock relies on having this setting set to on to work correctly? It would be nicer if s3mock worked independently of whether this setting is set. Perhaps we could make that easier by also providing the 'transparent head requests'-like behavior as a directive?

(edit: s3mock relies on this setting set to on, of course)

@raboof raboof added 0 - new Ticket is unclear on it's purpose or if it is valid or not discuss Tickets that need some discussion before proceeding t:core Issues related to the akka-http-core module labels Jun 26, 2018
@radekg
Copy link
Author

radekg commented Jun 27, 2018

@raboof, thanks for the answer. I had a look at the issue you mentioned. This appears to be some kind of left over from play farmework / spray?

Don’t get me wrong, it is nice that such an option exists. I simply think that the setting should be opt-in (default off). The reason being, HEAD is a valid HTTP method in its own rights. It’s a bit unsettling when the client executes a HEAD request but a GET route is invoked. I was not sure if I had a bug somewhere. Took some time to verify that my client does really issue a HEAD request. And it caught me out when testing a pretty complex setup, with client cert authentication and such, so the option was definitely on the table. Next, I started looking for an akka-http bug, then I found a setting. It’s just surprising and non-standard. Took me good half an hour to realise „yes, akka-http really does that”.

To answer your question: no, s3mock requires the setting to be on, I need the setting to be off. I have 2 akka-http systems while running integration tests. I need to configure the actor system in a non-standard way (global akka config would apply to both actor systems thus breaking s3mock) to get a standard HTTP behavior. In my opinion, it would be better to require the library / system who requires this non-standard HEAD -> GET to explicitly enable it. The problem isn’t that „I have to configure it”, I know how to do this. Just not sure why someone who expects a correct HTTP behavior from a HTTP server has to go through these hoops.

@schoeneu
Copy link

schoeneu commented Mar 22, 2019

Hi @raboof , we ran into exactly this issue recently. I strongly concur with @radekg that this should not be enabled by default: It's possible to write route specs that map HEAD and GET to different routes on the same path that look completely correct, but never use the HEAD route. That's really weird behaviour.

However, if there are strong reasons to keep it enabled by default, JUnitRouteTest should also use the same default. We have integration tests covering this exact route which work perfectly, but then the functionality breaks on the deployed system. (I haven't dug into why the test behaves differently, but I suppose that the transparent-head-requests happen at an outer level of the HttpServerBlueprint which is not used in the route tests.)

@schoeneu
Copy link

Did some more research: #549 (moving transparent HEAD from blueprint to directives) would solve the issue of JUnitRouteTest behaving differently from a deployed route, right?

@huntc
Copy link
Contributor

huntc commented Oct 28, 2019

To add my own findings, if akka.http.server.transparent-head-requests=false isn't specified then any explicit HEAD routes are ignored. This was a surprise to me and took an hour or so to debug given research etc. I think that, at least if there are HEAD routes then we should automatically behave as per akka.http.server.transparent-head-requests=false.

@jrudolph jrudolph added this to the 10.2.0 milestone Oct 28, 2019
@jrudolph
Copy link
Member

I generally agree that the current solution is too much magic by default.

For 10.1.x, we can only make compatible changes. I could imagine that we issue a warning when the head directive is issued while transparent-head-requests = on.

For the future, we could imagine several alternatives:

  • just change the default value of transparent-head-requests
  • remove the setting and reimplement the feature as a directive in the routing layer, that would allow you to enable that feature where required without globally restricting what's going on (a similar suggestion was once suggested here: Transparent head handling moved from a blueprint to a directive (#40). #549 but we never merged it)

@huntc
Copy link
Contributor

huntc commented Oct 28, 2019

just change the default value of transparent-head-requests

This would be my vote. Thanks.

@jrudolph jrudolph added 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted and removed 0 - new Ticket is unclear on it's purpose or if it is valid or not labels Feb 6, 2020
@raboof raboof removed the discuss Tickets that need some discussion before proceeding label Feb 26, 2020
@raboof raboof self-assigned this Apr 15, 2020
raboof added a commit that referenced this issue Apr 15, 2020
@ennru
Copy link
Member

ennru commented Apr 16, 2020

The switch is now flipped with #3063.

@ennru ennru closed this as completed Apr 16, 2020
@radekg
Copy link
Author

radekg commented Apr 16, 2020

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted t:core Issues related to the akka-http-core module
Projects
None yet
Development

No branches or pull requests

6 participants