Terraform in Equinix Metal: Fix NAT to reference correct interfaces #76
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Still need to do a full reprovision test. The commands worked manually. |
mmlb
reviewed
Apr 7, 2021
|
Updated per feedback from @mmlb. Looking for further feedback if this new approach is good. |
mmlb
approved these changes
Apr 8, 2021
mmlb
reviewed
Apr 8, 2021
| NAT_INTERFACE=$(cat .nat_interface) | ||
| fi | ||
| if [ -n "$NAT_INTERFACE" ] && ip addr show "$NAT_INTERFACE" &>/dev/null; then | ||
| # TODO(nshalman) the terraform code would just run these commands as-is once |
There was a problem hiding this comment.
ubuntu has some sort of iptables-persist package iirc. Yep iptables-persistent
There was a problem hiding this comment.
Should I try to get that wired up, or land as is with the TODO?
There was a problem hiding this comment.
todo is fine imo, hasn't been an issue so far I think.
This moves the NAT commands from terraform to setup.sh Signed-off-by: Nahum Shalman <nshalman@equinix.com>
|
Squashed and ready to land if everyone likes this. |
|
I say it is ready to ship |
3 tasks
mergify bot
added a commit
that referenced
this pull request
Apr 12, 2021
## Description This is a follow-up to #76 which introduced a failure: ``` provisioner: ./setup.sh: line 117: NAT_INTERFACE: unbound variable ``` ## Why is this needed Unbreak `setup.sh` when used by Vagrant Fixes #77 ## How Has This Been Tested? I used the following simple test case. It works now that the variable is declared first, but still breaks as reported without the fix. ```bash #!/bin/bash set -eu NAT_INTERFACE="" if [ -r .nat_interface ]; then NAT_INTERFACE=$(cat .nat_interface) fi if [ -n "$NAT_INTERFACE" ] && ip addr show "$NAT_INTERFACE" &>/dev/null; then echo "$NAT_INTERFACE" fi ``` ## How are existing users impacted? What migration steps/scripts do we need? Vagrant users are currently broken as reported in the community Slack. ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [ ] added unit or e2e tests - [ ] provided instructions on how to upgrade
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The NAT setup commands assume that the interface is named eth1, when clearly from the documentation it is named
enp1s0f1. This commit fixes the NAT setup commands accordingly.Why is this needed
NAT doesn't work by default on Equinix Metal when following the documentation
How Has This Been Tested?
How are existing users impacted? What migration steps/scripts do we need?
Existing sandboxes (that are broken) should either be rebuilt, or can run the commands manually to enable NAT
Checklist:
I have: