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

Stack Host header into X-Forwarded-Host #1572

Merged
merged 4 commits into from Sep 5, 2018

Conversation

@ignasi35
Member

ignasi35 commented Sep 4, 2018

Fixes #1569

  • fix AkkaHttpServiceGateway
  • fix NettyServiceGateway
@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 4, 2018

If it's a clean backport I'm in favor of also making this available to 1.4.x users.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 5, 2018

related to akka/akka-http#2191

newRequest.headers.add(header.getKey, header.getValue)
}
}
// todo - add Forwarded and other headers
// todo - add Forwarded and other headers.

This comment has been minimized.

@ignasi35

ignasi35 Sep 5, 2018

Member

The original todo comment mentioned using Forwarded headers instead of X-Forwarded-Host. I preferred using X-Forwarded-Host over Forwarded because Forwarded is not supported in Akka-HTTP so we have the same implementation in both gateways.

headers should contain(s"X-Forwarded-Host: localhost")
// answer should contain(s"X-Forwarded-Host: localhost:$port")
}

This comment has been minimized.

@ignasi35

ignasi35 Sep 5, 2018

Member

I didn't add a test for Netty because of the cost of creating the test from scratch and the plans to discontinue the Netty service gateway. Can do on a separate PR.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 5, 2018

I've manually tested both in online-auction-java.

@renatocaval

LGTM, left some minor nitpicking comments. All optional.

@@ -63,7 +64,18 @@ class AkkaHttpServiceGateway(
case Some(upgrade) =>
handleWebSocketRequest(request, newUri, upgrade)
case None =>
http.singleRequest(request.withUri(newUri).withHeaders(filterHeaders(request.headers)))
val xForwardedHost = request.header[Host].to[Set].map(_.host).map(`X-Forwarded-Host`.apply)

This comment has been minimized.

@renatocaval

renatocaval Sep 5, 2018

Member

can we avoid the double mapping?
.map { h => `X-Forwarded-Host`(h.host) }

#nitpicking

) ++ xForwardedHost.toSeq)
.foldLeft[HttpHeaders](new DefaultHttpHeaders())((acc, header) => acc.add(header._1, header._2))
}

This comment has been minimized.

@renatocaval

renatocaval Sep 5, 2018

Member

I think that an imperative style here will make it more obvious, for example:

val headers = 
  new DefaultHttpHeaders()
    .add(HttpHeaderNames.HOST, s"${downstreamAddress.getHost}:${downstreamAddress.getPort}")
      
  if (originalRequest.headers().contains(HttpHeaderNames.HOST))
    headers.add("X-Forwarded-Host", originalRequest.headers().get(HttpHeaderNames.HOST))
  else
    headers

This comment has been minimized.

@ignasi35

ignasi35 Sep 5, 2018

Member

more readable. thanks

@marcospereira

👍

@marcospereira marcospereira merged commit 3fb1725 into lagom:master Sep 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 6, 2018

Not an immediate/clean backport. If we need this on 1.4.x we can do it on a separate PR.

@ignasi35 ignasi35 deleted the ignasi35:1569-service-gateway-x-forwarded-host-header branch Sep 6, 2018

@ignasi35 ignasi35 modified the milestones: Lagom 1.4.9, Lagom 1.5.0 Sep 6, 2018

benmccann pushed a commit to benmccann/lagom that referenced this pull request Sep 7, 2018

Bumps Akka 2.5.8, AkkaHttp 10.0.11 and Play 2.6.9 (lagom#1134)
* Bumps Akka to 2.5.8 and play to 2.6.9

* Update Akka Streams Kafka to 0.18

Also includes an update of Kafka to 0.11.0.1.

* Removes unnecessary override (see akka-http PR lagom#1572)

erip added a commit to erip/lagom that referenced this pull request Sep 30, 2018

Stack Host header into X-Forwarded-Host (lagom#1572)
* Stack Host header into X-Forwarded-Host (AkkaHTTP)

* Stack Host header into X-Forwarded-Host (Netty)

* Typos on code comments

* Code cleanup. More readable

@ignasi35 ignasi35 modified the milestones: Lagom 1.5.0, Lagom 1.5.0-M4 Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment