Skip to content
This repository has been archived by the owner on Aug 15, 2018. It is now read-only.

added nat configuration #39

Merged
merged 4 commits into from Jun 6, 2018
Merged

added nat configuration #39

merged 4 commits into from Jun 6, 2018

Conversation

d78ui98
Copy link
Contributor

@d78ui98 d78ui98 commented Jun 5, 2018

I have added NAT configuration to the dhcp server.

dhcp/install.sh Outdated

#configuration
# fetching interface name
INTERNET=$(ifconfig | grep -m 1 RUNNING | cut -d':' -f1)
Copy link
Member

Choose a reason for hiding this comment

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

I understand that you have copied this from some example, and in the case of example it may give a correct result, but not always.
Try this one which is safer: ip route | grep default | cut -d' ' -f5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay. I will now prefer ip route over ifconfig.

Copy link
Member

Choose a reason for hiding this comment

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

okay. I will now prefer ip route over ifconfig

It is not a matter of preference, your first implementation is not guaranteed to work in all the cases.

dhcp/install.sh Outdated
apt --yes update
apt --yes --install-recommends install dnsmasq
export DEBIAN_FRONTEND=noninteractive
apt -yq --yes install iptables-persistent
Copy link
Member

Choose a reason for hiding this comment

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

Here -yq is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-yq flag is required to handle DEBIAN_FRONTEND variable.

Copy link
Member

Choose a reason for hiding this comment

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

-yq flag is required to handle DEBIAN_FRONTEND variable.

Are you sure about this? Where did you find this information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can share the source - https://serverfault.com/a/227194

Copy link
Member

Choose a reason for hiding this comment

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

Look up in the man page the meaning of -yq and then remove it.
You have spent me 1 our only for this, and it is not normal that I spend in the project as much time as you do.

dhcp/install.sh Outdated
#configuration
# fetching interface name
INTERNET=$(ifconfig | grep -m 1 RUNNING | cut -d':' -f1)
LOCAL=$(ifconfig | grep -m 2 RUNNING | cut -d':' -f1 | tail -n1)
Copy link
Member

Choose a reason for hiding this comment

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

Here try something like this:
ip route | grep -v default | cut -d' ' -f3 | grep -v $INTERNET | head -1

dhcp/install.sh Outdated
INTERNET=$(ifconfig | grep -m 1 RUNNING | cut -d':' -f1)
LOCAL=$(ifconfig | grep -m 2 RUNNING | cut -d':' -f1 | tail -n1)

# configuration
echo "dhcp-range=tftp,${NETWORK}.250,${NETWORK}.254" >> /etc/dnsmasq.conf
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we need this line with tftp? What does the documentation of dnsmasq say about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the documentation says If dnsmasq is providing a TFTP service then only the filename is required to enable network booting.
Some comment on stackoverflow gave an explanation to this line and concluded that it would reduce time required for network boot.

Copy link
Member

Choose a reason for hiding this comment

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

yes, the documentation says If dnsmasq is providing a TFTP service then only the filename is required to enable network booting.

We don't need TFTP service on the DHCP server.

Some comment on stackoverflow gave an explanation to this line and concluded that it would reduce time required for network boot.

You cannot trust all the comments on stackoverflow. Couldn't you find anything on the documentation of dnsmasq?

Copy link
Member

Choose a reason for hiding this comment

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

We don't need TFTP service on the DHCP server.

Please remove this line and fix the configuration of dnsmasq.

dhcp/install.sh Outdated
apt --yes update
apt --yes --install-recommends install dnsmasq
export DEBIAN_FRONTEND=noninteractive
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you remove (or comment out) this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is required to handle interactive post-install configuration. If we remove this we won't be able to handle that with our script. As a result we won't be able to install iptables-persistent

Copy link
Member

Choose a reason for hiding this comment

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

This line is required to handle interactive post-install configuration. If we remove this we won't be able to handle that with our script. As a result we won't be able to install iptables-persistent

Have you tried this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. I can share a screenshot of what happens if I remove that line. The provision script just freezes -
sa

Copy link
Member

Choose a reason for hiding this comment

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

yes. I can share a screenshot of what happens if I remove that line.

If I try to remove it and there is no problem, then you are going to have problems. OK?

Copy link
Contributor Author

@d78ui98 d78ui98 Jun 5, 2018

Choose a reason for hiding this comment

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

Sure. I can make it easy for you. I have created a video which has both cases(line commented and line not commented) https://youtu.be/WQHwKu9ebYA

Copy link
Member

Choose a reason for hiding this comment

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

Ok, do it like this:

DEBIAN_FRONTEND=noninteractive apt --yes install iptables-persistent

dhcp/install.sh Outdated
iptables-save > /etc/iptables.rules

# restarting service
service dnsmasq restart
Copy link
Member

Choose a reason for hiding this comment

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

You can move this line after changing the configuration (or place the configuration change above it), just to keep them together.

using ip route in place of ifconfig, adding proper comment, clubbing up code together
dhcp server now does not offer tftp services and interactive yes no box is handled by single line
Copy link
Member

@dashohoxha dashohoxha left a comment

Choose a reason for hiding this comment

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

I am not sure that the default configuration of dnsmasq is correct (without you customizing it).
But you can merge this branch if you wish and fix this problem later, if needed.

@d78ui98 d78ui98 merged commit 21b9060 into bionic Jun 6, 2018
@d78ui98 d78ui98 deleted the nat branch June 7, 2018 05:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants