Rework networking logic and some miscellaneous code changes

[Kieren]

Update syno_pihole.sh

This patch reworks the network logic of the script, making a distinction between a CIDR notated range (by convenience) of addresses and an actual IPv4 subnet which has a (all ones host identifier) broadcast address. Conflating a range of addresses in CIDR notation within a (LAN) subnet with an actual subnet seems to be the reason for the original script logic.

This patch considers the PARAM_IP_RANGE parameter to designate a pool of addresses reserved for Docker to use in its generated Docker macvlan network called 'synology-pihole_macvlan' – as is indicated by the use of PARAM_IP_RANGE in the docker-compose.yml file. In the same way the network DHCP server reserves a pool of addresses, this range helps avoid address collisions on the network, especially with the DHCP server. Docker should allocate any container attached to the synology-pihole_macvlan network an address from this pool if a static address (PARAM_PIHOLE_IP) were not already specified.

While the Docker generated macvlan synology-pihole_macvlan uses PARAM_IP_RANGE to reserve a pool of addresses for its use, the host macvlan network (default name macvlan0) named by PARAM_VLAN_NAME in the function execute_create_macvlan only needs a new host address to attach to this interface in order for it to communicate with containers attached to synology-pihole_macvlan. Hence this patch introduces a new parameter (--host-ip | HOST_IP | PARAM_HOST_IP) to allow the user to specify an address for this purpose, otherwise a reasonable address is attempted.

In the process of writing the patch a number of other changes were made, notably:

The is_valid_cidr function variable CIDR_REGEX (line 168) - removed '([^0-9.]|$)' from the end of the regex because its purpose was unknown and did not seem to affect its function.

Edits to the Usage instructions were made to try and clarify parameter usage.

The parameter validation/initialisation functions were edited to allow a broader range of valid user configurations and to not consume more address space than necessary.

[markdumay]

Hi @Kieren, thanks a lot for the extensive PR! I do realize Docker networking is not my strong suit per se, so your code suggestions and feedback are much appreciated. Below my first comments.

Conflating a range of addresses in CIDR notation within a (LAN) subnet with an actual subnet seems to be the reason for the original script logic.

You are right, the original intent was to assign a static IP address to Pi-Hole on the same subnet where the Synology NAS resides. Imagine the following network environment:

• The network router is available at 192.168.0.1
• The main network (VLAN) has a subnet mask of 255.255.255.0
• The Synology NAS is available at 192.168.0.100
• Pi-Hole is to be assigned to 192.168.0.250

The script then assigns an address range of the subnet to a macvlan network, for example 192.168.0.248 to 192.168.0.251. The CIDR notation was introduced to simplify the definition of this IP address range, e.g. 192.168.0.248/30. The advantage being twofold: Pi-Hole would be available on a static IP address, and no additional network/routing settings would be required to connect to Pi-Hole from any device connected to the same subnet. The downside indeed is that such a setup introduces additional overhead, e.g. the network address at 192.168.0.248 and the broadcast address at 192.168.0.251.

This patch considers the PARAM_IP_RANGE parameter to designate a pool of addresses reserved for Docker to use in its generated Docker macvlan network called 'synology-pihole_macvlan' – as is indicated by the use of PARAM_IP_RANGE in the docker-compose.yml file.

If I understand your proposal correctly, you suggest the ability to assign a proper subnet (e.g. 10.0.0.0/24) to the macvlan network, where Docker would be more or less free to select an IP address of the pool of available addresses - similar to a DHCP server? And the new parameter IP_RANGE would allow you to specify this pool of addresses? Would HOST_IP then refer to the macvlan's network address (e.g. 10.0.0.1, the first available address within the subnet)?

In the process of writing the patch a number of other changes were made [...]

Would you be ok to update the README.md with your proposed changes too? I imagine it would be helpful to describe a few typical use cases, next to the updated commands and parameters. I do like your more precise approach towards the usage() function!

Some final thoughts, I noticed my shellcheck extension is going crazy. I haven't touched the code since September last year, so it seems some additional refactoring is needed. It makes more sense to do so once your PR is merged though - I'll create a new issue as a reminder for now.

[Kieren]

Hi @markdumay I'm glad this is a welcome contribution to your efforts. On diving into this my sense has been that there is some confusion in some of the internet (blog) content on both the linux networking and how docker utilises that in this sort of set up. I've come across a number of similar docker setup posts that seem to differ slightly in how they are described and implemented, contributing to confusion.

Some of this I think lies around the use of macvlan within docker and the additional macvlan interface that is manually configured on the host. The clearest description I have found on the macvlan bridge network as used in syno_pihole.sh is here Docker Docs: Get started with macvlan Network Driver. The first example there gives a clear description of the very same setup used here.

Note:

Comparing syno_pihole with the documentation there, parameters PARAM_IP_RANGE, PARAM_PIHOLE_IP and PARAM_GATEWAY are all on the very same subnet, namely PARAM_SUBNET. Consider your example:

The script then assigns an address range of the subnet to a macvlan network, for example 192.168.0.248 to 192.168.0.251. The CIDR notation was introduced to simplify the definition of this IP address range, e.g. 192.168.0.248/30. The advantage being twofold: Pi-Hole would be available on a static IP address, and no additional network/routing settings would be required to connect to Pi-Hole from any device connected to the same subnet. The downside indeed is that such a setup introduces additional overhead, e.g. the network address at 192.168.0.248 and the broadcast address at 192.168.0.251.

I don't think there is a downside as you mention 192.168.0.248 is not the network address of the Docker macvlan network, because it is not a new subnetwork, and certainly not the network address of PARAM_SUBNET. Similarly 192.168.0.251 is neither the broadcast address of the Docker macvlan network nor of PARAM_SUBNET. These addresses are just the first and last addresses of the range designated by PARAM_IP_RANGE within the subnet PARAM_SUBNET – that Docker will use to select an address to give to any container attached to the Docker network pihole_macvlan. Unless an address is otherwise specified, as done here, with PARAM_PIHOLE_IP. As I understand it the Docker macvlan driver is unaware of the the local network DHCP server; PARAM_IP_RANGE as used in the docker-compose.yml file is simply telling docker to use a range of addresses that do not overlap the range used by the likely already running DHCP server on the local network.

In short my patch defaults the size of PARAM_IP_RANGE to /32 unless otherwise specified and on validation checks that the actual network broadcast address of the subnet (192.168.0.255 in your example) is not used.

I think it's worth mentioning an oversight in this patch; it does not currently check that the network address (192.160.0.0 as per example) is not used.

I hope this clarifies the intent of my patch for the Docker macvlan network.

As for the host macvlan network named (PARAM_VLAN_NAME), I have found this article Bridge vs Macvlan helpful in clarifying its purpose, particularly why assigning it the range of addresses PARAM_IP_RANGE is not appropriate. This interface is only enabling the host and the pihole container to communicate and since the interface assigns the host a new MAC address, in my patch, PARAM_HOST_IP is assigned to it and used when communicating with the Pihole – as determined by the created route.

Again I hope this is helpful.

I will look at updating the README.md too.

As an aside there have already been a couple of bugs pop up that I've addressed in my patch. Would you prefer that I make any further changes and bug fixes etc on a seperate branch to this PR so it doesn't complicate the pull process?

[markdumay]

Hi @Kieren, thanks for your elaborate response. Both linked articles are indeed very clear and relevant. So in short, we revise the script to simply apply the macvlan network to the current subnet (PARAM_SUBNET). This subnet stays as is, the only thing we need to consider is to reserve a range of addresses (PARAM_IP_RANGE) that do not interfere with the DHCP server. It makes more sense now - I was getting confused too! ;-)

As an aside there have already been a couple of bugs pop up that I've addressed in my patch. Would you prefer that I make any further changes and bug fixes etc on a seperate branch to this PR so it doesn't complicate the pull process?

I suggest to track the individual issues (like #7) and to put the fixes on a separate branch indeed. It looks like you're on a roll, so I can imagine you'll find additional issues. ;-)

I will look at updating the README.md too.

I noticed your update - is the PR ready for review?

[markdumay]

Hi @Kieren,

I finally got around review your PR - apologies for the delay! Below my findings. Some of the observations appear unrelated to your code changes. However, I included them here with the updated line numbers for simplicity's sake. Let me know if you would like to address below observations yourself, or if I should merge the PR first so we can address them afterwards. Besides the observations, the revised script appears to be working fine. Thanks again for all your effort, much appreciated!

Observation 1
There is a typo in README.md at line #154: "Pi-hole administrator web interface"

Observation 2
The script syno_pihole.shgenerates an error at line #333: 3232235771: command not found. The number 3232235771 depends on the .env settings.

Observation 3
The error messages of syno_pihole.sh appear malformed when using an invalid PIHOLE_IP. For example, my subnet is 192.168.1.0/24. Setting PIHOLE_IP in the .env file to 192.168.0.251 (note the .0 instead of .1) generates below errors (the newline character\n is not parsed correctly). Probably we should use printf instead of echo in the functions terminate() and log().

Pi-hole IP address '192.168.0.251' is not valid in subnet '192.168.1.0/24\nHost IP address '192.168.0.252' is not valid in subnet '192.168.1.0/24\nDocker network IP address range is not in subnet '192.168.1.0/24'\n

Observation 4
The script syno_pihole.sh fails in case the host (Synology NAS) has an existing macvlan network already. Perhaps we should add a test and/or warning, as it seems we can only attach one macvlan network to a given subnet? The error message is as follows.

Creating network "synology-pihole-test_macvlan" with driver "macvlan"
failed to allocate gateway (192.168.1.1): Address already in use

Observation 5
The step in the script syno_pihole.sh to create a password fails if the container name differs from pihole. For example, setting PIHOLE_HOSTNAME=pihole2 in the .env generates below error.

Step 8 from 8: Setting Pi-hole password
Error: No such container: pihole

The code should be corrected to this.

execute_create_password() {
    print_status "Setting Pi-hole password"
    
    if [ -z "$PARAM_WEBPASSWORD" ] && [ "$FORCE" != 'true' ] ; then
        docker exec -it "$PARAM_PIHOLE_HOSTNAME" pihole -a -p
    else
        log "Skipped in forced mode"
    fi
}

Similarly, the code to detect the version of the currently running Pi-hole container is also incorrect.

define_pihole_versions() {
    […]
    PIHOLE_VERSION=$(docker exec "$PARAM_PIHOLE_HOSTNAME" pihole -v 2>/dev/null | grep 'FTL' | awk '{print $4}' \
        | cut -c2-)
    […]

The global variable at line #29 should be removed.

PIHOLE_CONTAINER='pihole'

[ghost]

@Kieren Thank for your help. I did not understand one thing, do you mean HOST_IP in the script, as the new IP address, and not the currently assigned IP address of Synology NAS. So it means we need 2 IP address for host(synology nas) - 1 is actual which is already used in the network and other IP is just to link(synology with macvlan(pihole container))?
And if its 2 different IP address for the Host on the same network, then is it possible to use just one IP address which is already on network?
Thanks

[Kieren]

@Kieren Thank for your help. I did not understand one thing, do you mean HOST_IP in the script, as the new IP address, and not the currently assigned IP address of Synology NAS. So it means we need 2 IP address for host(synology nas) - 1 is actual which is already used in the network and other IP is just to link(synology with macvlan(pihole container))?
And if its 2 different IP address for the Host on the same network, then is it possible to use just one IP address which is already on network?
Thanks

@krswin, yes that is what I mean, HOST_IP is a new IP address because the macvlan interface created to bridge the host to the pihole has a new MAC address – HOST_IP is assigned to that. It is worth mentioning this new IP address is another valid address on the network accessible from the rest of the network. Because of this, if the same IP address is used for both host MAC addresses, I believe you will probably have routing issues with your ethernet frames on the network just as you would with any other IP address clash on a network. This additional macvlan interface (and IP address) for the host is a workaround to enable the host to communicate with the Pi-hole container on it's own interface, however as mentioned it's effects apply to the network more broadly which you are right to consider.

I'm not aware of another way to provide host to Pi-hole container communication purely on the host.

[Kieren]

Hi @Kieren,

I finally got around review your PR - apologies for the delay! Below my findings. Some of the observations appear unrelated to your code changes. However, I included them here with the updated line numbers for simplicity's sake. Let me know if you would like to address below observations yourself, or if I should merge the PR first so we can address them afterwards. Besides the observations, the revised script appears to be working fine. Thanks again for all your effort, much appreciated!
...

Hi @markdumay,

Thanks for making all these observations, as you can see I've not spent time on this for a while. While I'd like to address all of them in a timely manner I can't guarantee that will happen, so perhaps merging the PR will make managing the script easier incase anybody has their own fixes? I agree the script is mostly functional with careful use to avoid the already noted issues.

[markdumay]

Done! Thanks again - I'll work on the observations and will add issues as needed. Feel free to contribute any time! ;-)