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

Make SimpleBuilder listen on 0.0.0.0 #3284

Merged
merged 1 commit into from
Jun 8, 2024
Merged

Make SimpleBuilder listen on 0.0.0.0 #3284

merged 1 commit into from
Jun 8, 2024

Conversation

ImJeremyHe
Copy link
Member

@ImJeremyHe ImJeremyHe commented Jun 6, 2024

This PR:

For convenience, we usually prefer to run the SimpleBuilder in a Docker environment. Listening only to localhost makes the Builder API inaccessible from outside the docker container. Although I think this change is unreasonable for HotShot, it is currently the most suitable option.

This PR does not:

  • introduce any risk since the change is under the testing feature

@tbro
Copy link
Contributor

tbro commented Jun 7, 2024

Since we already added a Config to take in a port, why don't we add host there and take it in? If we use default when not provided, there would still not be any changes required on the dev-node. On the other hand, we could just add a Url param, and change builder_port param on dev-node to builder_url. Not sure which is better.

@ImJeremyHe
Copy link
Member Author

ImJeremyHe commented Jun 7, 2024

If anyone sees a url in the configuration, I think the first thought is that this application will be going to access some other services to fetch back the data. No one would expect that this url is what we listen on.
And if anyone sees a port in the configuration, I think the most reasonable thought of this would be that this application is going to run a server on this port.
Since the SimpleBuilder here is provided to be used as a server, we should not limit its listening address.

@tbro
Copy link
Contributor

tbro commented Jun 7, 2024

If anyone sees a url in the configuration, I think the first thought is that this application will be going to access some other services to fetch back the data. No one would expect that this url is what we listen on. And if anyone sees a port in the configuration, I think the most reasonable thought of this would be that this application is going to run a server on this port. Since the SimpleBuilder here is provided to be used as a server, we should not limit its listening address.

Yea, you are probably right about the expectation of url. I can still see potential issues w/ hard-coding a server to bind to all network devices. You often need to assign a server to a specific IP. And adding this to configuration only adds the possibility of doing that, it doesn't prevent anyone from binding to 0.0.0.0. If we make 0.0.0.0 the default it's really the same as you have here but w/ some future proofing.

@ImJeremyHe ImJeremyHe merged commit 2fd2897 into main Jun 8, 2024
24 checks passed
@ImJeremyHe ImJeremyHe deleted the jh/builder branch June 8, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants