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

Use same IP address for both boots and nginx #43

Merged
merged 2 commits into from
Jan 27, 2021

Conversation

mrchrd
Copy link
Contributor

@mrchrd mrchrd commented Jan 24, 2021

Signed-off-by: Michael Richard michael.richard.ing@gmail.com

Description

This configures NGINX to listen on port 8080 and lets go the need to configure a second IP address on the host dedicated to NGINX.

Why is this needed

Setting up a second IP address to host NGINX on the same host is not always easy, especially when running tinkerbell on network devices like switches. The second IP address adds a useless level of complexity. In the future, all the code required to identify the host operating system and configure the IP address could even be removed and left as a prerequisite, since the host is likely to have an IP address already configured.

How Has This Been Tested?

The untouched vagrant_test.go test ran sucessfully.

How are existing users impacted? What migration steps/scripts do we need?

Simply re-applying the docker-compose.yml should be sufficient (untested).
Additional firewall rules to allow traffic on port 8080 could be required depending on user's network configuration.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@gianarb
Copy link
Contributor

gianarb commented Jan 24, 2021

Hello!

Thank you for this work, can you rebase, please?

Thanks

@mrchrd
Copy link
Contributor Author

mrchrd commented Jan 25, 2021

Done

@gianarb
Copy link
Contributor

gianarb commented Jan 25, 2021

Ops, I am sorry
This PR should fix the CI issue #44

@gianarb
Copy link
Contributor

gianarb commented Jan 25, 2021

I am not sure how this will impact users that are running Nginx and I am not sure if we have to mark it as breaking change or not. I will have a look tomorrow 👍

Signed-off-by: Michael Richard <michael.richard.ing@gmail.com>
@mrchrd
Copy link
Contributor Author

mrchrd commented Jan 25, 2021

Now it's fixed!

@mrchrd
Copy link
Contributor Author

mrchrd commented Jan 25, 2021

It's probably fair to consider it as a breaking change, even though it won't break anything in a majority of cases. Upgrading will fail if:

  1. Some service is already running on port 8080
  2. Firewall's blocking traffic on port 8080

@gianarb gianarb added breaking-change Denotes a PR that introduces potentially breaking changes that require user action. ready-to-merge Signal to Mergify to merge the PR. labels Jan 26, 2021
@gianarb gianarb removed the request for review from gauravgahlot January 26, 2021 07:53
@gianarb gianarb added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Jan 26, 2021
Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

I tried the migration and yes, a docker-compose rm, up does the magic! Thanks

@mergify mergify bot merged commit 3d387cd into tinkerbell:master Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Denotes a PR that introduces potentially breaking changes that require user action. ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants