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

[PIO-31] Move from spray to akka-http #474

Merged
merged 25 commits into from Oct 13, 2018

Conversation

@takezoe
Copy link
Member

commented Sep 30, 2018

Ported following servers to Akka HTTP from Spray.

  • AdminServer (AdminAPI)
  • Dashboard
  • EventServer
  • PredictionServer (CreateServer)

First, I also tried to replace json4s with spray-json which is a default supported JSON library in Akka HTTP, but json4s is used deeply in PredictionIO. So I abandoned to do it. The current combination is Akka HTTP and json4s.

@takezoe takezoe force-pushed the takezoe:akka-http branch from 22c9d5b to 8b389f1 Sep 30, 2018
takezoe added 3 commits Sep 30, 2018
@takezoe takezoe force-pushed the takezoe:akka-http branch from 8afcbfe to 7728298 Oct 4, 2018
@takezoe takezoe force-pushed the takezoe:akka-http branch from 7728298 to 8acc61d Oct 4, 2018
takezoe added 4 commits Oct 5, 2018
@takezoe takezoe force-pushed the takezoe:akka-http branch from 3961024 to 5262303 Oct 5, 2018
takezoe added 4 commits Oct 5, 2018
@takezoe takezoe force-pushed the takezoe:akka-http branch from 2b3b883 to 8826ff7 Oct 5, 2018
takezoe added 3 commits Oct 5, 2018
@takezoe takezoe changed the title [WIP][PIO-31] Move from spray to akka-http [PIO-31] Move from spray to akka-http Oct 6, 2018
@takezoe

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2018

@shimamoto All tests passed. Could you take a look?

takezoe added 2 commits Oct 9, 2018
complete(StatusCodes.Unauthorized, challengeHeaders, Map("message" -> msg))
}
case ChannelRejection(msg) :: _ =>
// TODO complete(StatusCodes.Unauthorized, challengeHeaders, Map("message" -> msg))

This comment has been minimized.

Copy link
@shimamoto

shimamoto Oct 9, 2018

Member

unused?

List(allowOriginHeader)
)
def corsHandler(r: Route): Route = addAccessControlHeaders {
preflightRequestHandler ~ r

This comment has been minimized.

Copy link
@shimamoto

shimamoto Oct 9, 2018

Member

I think this trait doesn't meet CORS API specifications. Is this okay?

This comment has been minimized.

Copy link
@takezoe

takezoe Oct 9, 2018

Author Member

Hmm... Valid CORS support is not so easy and the original CORS support for spray looks also invalid. In fact, this CORS support is only for Ajax request in Dashboard. I wonder that we have to maintain this experimental Dashboard (and also AdminServer) with such pain.

This comment has been minimized.

Copy link
@shimamoto

shimamoto Oct 10, 2018

Member

Wow, I’m lost for words. As I see it, it's not proven to be very useful. 😕

This comment has been minimized.

Copy link
@dszeto

dszeto Oct 10, 2018

Contributor

Both dashboard and AdminServer were experimental and not feature complete. I don't think we need to worry about supporting them properly. We should rearchitect and do it properly.

This comment has been minimized.

Copy link
@shimamoto

shimamoto Oct 10, 2018

Member

You mean there is nothing wrong with them being incomplete?

If we don't need to support them properly, there is an option to remove them. In any case, we should not discuss about that here.

This comment has been minimized.

Copy link
@dszeto

dszeto Oct 10, 2018

Contributor

Sorry. What I mean is that we probably don’t want to spend too much effort on fixing CORS properly for dashboard and AdminServer in this PR, as they should be redone later.

This comment has been minimized.

Copy link
@shimamoto

shimamoto Oct 11, 2018

Member

Absolutely.

// responseAs[String] shouldEqual """{"status":1}"""
// }
// }
// }

This comment has been minimized.

Copy link
@shimamoto

shimamoto Oct 9, 2018

Member

unused?

@shimamoto

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

What about documentation?

takezoe added 2 commits Oct 9, 2018
takezoe added 2 commits Oct 9, 2018
@takezoe

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

Thanks. I removed unused code and fixed documentation! Regarding CORS support, how do you think about my above comment?

@shimamoto

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

LGTM except for CORS.

@dszeto

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

@takezoe Looks like SSL support is dropped. Should we create another PR to add that back?

@takezoe

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

My opinion is that SSL support is too much. I guess there are usually Nginx/Apache or any other load balancer in the front of API servers in a production environment. So we can enable SSL using them if we need it.

@dszeto

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

I used to think that way as well, but I humbly disagree now. After I learned about defense in depth (https://reinout.vanrees.org/weblog/2017/05/02/https-behind-proxy.html is a good read) and security audit, it is in fact a desirable feature. I can take on that as a separate PR.

@takezoe

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@dszeto I reverted SSL support for Akka HTTP in af75a80. But I realized that currently it's only used in Dashboard as same as CORS support. So I don't know if it's worth to keep maintaining.

@dszeto

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

@takezoe It should be in pio deploy as well:

engineFactoryName: String) extends Actor with SSLConfiguration with KeyAuthentication {

@takezoe

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@dszeto Oh, I see. Since diff is much large I missed that.

Then, should we make it available in EventServer and AdminiServer as well? (But it should be done in an another pull request because it modifies current capability, not just porting)

@takezoe

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@dszeto Anyway I reverted SSL support for CreateServer as well. Please take a look.

@dszeto
dszeto approved these changes Oct 12, 2018
Copy link
Contributor

left a comment

LGTM. Thanks for the hard work @takezoe !

@takezoe takezoe merged commit f762bee into apache:develop Oct 13, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takezoe takezoe deleted the takezoe:akka-http branch Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.