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.

Sign up for GitHub

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

Jump to bottom

allow specifying network interface with "server-host" option #1280

Merged
merged 3 commits into from
Jan 11, 2019
Merged

allow specifying network interface with "server-host" option #1280

merged 3 commits into from
Jan 11, 2019

Conversation

nathanstitt
Copy link
Contributor

Description

This adds an option to allow specifying the network address to bind to. Previously graphql-engine would bind to the wildcard interface, while I'd prefer it only listen to localhost

What component does this PR affect?

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Requires changes from other components? If yes, please mark the components:

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System

Related Issue

Solution and Design

I'm very much a Haskel neophyte and pretty much just copied how server-port worked. I'm happy to rework if this could be accomplished in a better manner.

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs update
  • Community content

Checklist:

  • I have read the contributing guide and my code conforms to the guidelines.
  • This change requires a change in the documentation.
    • note I did not document this option since server-port is not documented. If you'd like I can add a mention for both of these to the README
  • I have updated the documentation accordingly.
  • I have added required tests.

@hasura-bot
Copy link
Contributor

Beep boop! 🤖

Hey @nathanstitt, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible. 🕐

Stay awesome! 😎

@hasura-bot
Copy link
Contributor

Review app for commit 5e5c3a8 deployed to Heroku: https://hge-ci-pull-1280.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1280-5e5c3a8

@0x777 0x777 self-requested a review January 2, 2019 08:10
@shahidhk shahidhk added the c/server Related to server label Jan 3, 2019
0x777
0x777 requested changes Jan 7, 2019
View reviewed changes
Copy link
Member

@0x777 0x777 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just a few changes around how the host value is stored in ServerOpts.

server/src-lib/Hasura/Server/Init.hs Outdated
@@ -33,6 +33,7 @@ type RawAuthHook = AuthHookG (Maybe T.Text) (Maybe AuthHookType)
data RawServeOptions
= RawServeOptions
{ rsoPort :: !(Maybe Int)
, rsoHost :: !(Maybe String)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of String we can directly use HostPreference (http://hackage.haskell.org/package/warp-3.2.25/docs/Network-Wai-Handler-Warp.html#t:HostPreference)

server/src-lib/Hasura/Server/Init.hs Outdated Show resolved Hide resolved
server/src-exec/Main.hs Outdated Show resolved Hide resolved
server/src-lib/Hasura/Server/Init.hs Outdated Show resolved Hide resolved
@nathanstitt
Copy link
Contributor Author

Thanks for the review @0x777 - I've made the changes to store value as HostPreference as soon as it's read. Note that I also had to extend the FromEnv class, which is hopefully correct.

@hasura-bot
Copy link
Contributor

Review app for commit 8b65ae4 deployed to Heroku: https://hge-ci-pull-1280.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1280-8b65ae4

@0x777
Copy link
Member

0x777 commented Jan 8, 2019

This looks good! Can you document the cli flag and the environment variable here: https://github.com/hasura/graphql-engine/blob/master/docs/graphql/manual/deployment/graphql-engine-flags/reference.rst

@nathanstitt
Copy link
Contributor Author

Oh neat! I had no idea those were documented. I only found the port option by grepping the code 😁

just pushed a update to docs, happy to reword if you'd like

@hasura-bot
Copy link
Contributor

Review app for commit 0c5bb8d deployed to Heroku: https://hge-ci-pull-1280.herokuapp.com
Docker image for server: hasura/graphql-engine:pull1280-0c5bb8d

@shahidhk shahidhk merged commit 1b9540f into hasura:master Jan 11, 2019
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-1280.herokuapp.com is deleted

@hasura-bot
Copy link
Contributor

Beep boop! 🤖

Whoa! 🎉 🎉 💃

GIF

Awesome work @nathanstitt! 💪 🏆 All of us at Hasura ❤️ what you did.

Thanks again 🤗

@pomazanbohdan
Copy link

Very very good.
Just figured out that the lack of this option does not allow to run several dockers from hasura, because there is a conflict over the IP address due to the default 0.0.0.0

@nathanstitt nathanstitt deleted the server-host branch January 14, 2019 19:47
This was referenced Dec 19, 2023
Open
Open
This was referenced Dec 19, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x777 0x777 0x777 approved these changes

Assignees
No one assigned
Labels
c/server Related to server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nathanstitt @hasura-bot @0x777 @pomazanbohdan @shahidhk