⚠️ WIP: Fully support dual-stack/dual socket setups#716
⚠️ WIP: Fully support dual-stack/dual socket setups#716mchiappero wants to merge 12 commits intometal3-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
metal3-io-bot
added
the
needs-ok-to-test
|
Hi @mchiappero. Thanks for your PR. I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
metal3-io-bot
added
do-not-merge/work-in-progress
mchiappero
left a comment
mchiappero
left a comment
There was a problem hiding this comment.
I have left a few more considerations derived from the development process that would benefit from external input. Thank you!
There was a problem hiding this comment.
This new variable could actually be named PROVISIONING_HOSTNAME, to make it clearer that can be used for setting up the configuration for the provisioning network. Let me know your thoughts!
| fi | ||
|
|
||
| # Once determined if we have IPv4 and/or IPv6, override the hostname if provided | ||
| if [[ -n "$IRONIC_URL_HOSTNAME" ]]; then |
There was a problem hiding this comment.
If both PROVISIONING_INTERFACE and IRONIC_URL_HOSTNAME are provided we could (or maybe should) check that the IP addresses returned by both are coherent.
| if [[ "$(echo "$LISTEN_ALL_INTERFACES" | tr '[:upper:]' '[:lower:]')" == "true" ]]; then | ||
| export IRONIC_HOST_IP="::" | ||
| elif [[ -n env.ENABLE_IPV6 ]]; then | ||
| export IRONIC_HOST_IP="$IRONIC_IPV6" |
There was a problem hiding this comment.
IIRC host_ip allows the use of a hostname, so we could consider using IRONIC_URL_HOSTNAME here if set.
There was a problem hiding this comment.
I haven't changed the ProxyPass/ProxyPassReverse configuration although it would probably make sense to move to ::1 if IRONIC_IPV6 is set. Also, we might need to introduce an additional flag to allow a localhost socket/VirtualHost for the liveness & readiness tests when not listening to all: would you be in favour?
There was a problem hiding this comment.
Keep in mind that NGINX allows to disable IPv4-mapped IPv6 sockets via configuration option rather than at compile time and that in general it could be a better alternative to Apache.
The value of host_ip is determined twice within the ironic.conf.j2 template file, by means of a relatively hard to read set of conditions. Avoid this duplication and improve readability by exporting the correct value once in scripts/configure-ironic.sh. This also leave more room for more complex evaluations should these be needed in the future (e.g. IPv6) Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
Whenever PROVISIONING_INTERFACE is not set by the user, function get_provisioning_interface attempts to determine one, or provide "provisioning" as default value. However this can cause confusing errors down the line. Remove this default value and fail gracefully, with proper logging, if the PROVISIONING_INTERFACE value is not detected. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
The way the ironic-image processes are bound to internet sockets is mainly
by PROVISIONING_IP or PROVISIONING_INTERFACE, that is, by looking up a
specific address on an interface, or a specific interface for a workable
address.
Introduce two new utility functions in ironic-common.sh for these two
purposes:
get_interface_of_ip: returns the name of the interface where the IP address
provided as argument is found
get_ip_of_interface: returns the first IP associated to the interface
provided as argument
These two functions will be put into use in subsequent commits.
Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
Use the previously introduced get_interface_of_ip, to determine if the PROVISIONING_IP address is actually present on a network interface. This improves the code readability and enables additional debugging output. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
Commit 2742439 added logic to tentatively identify the interface name in get_provisioning_interface if the PROVISIONING_IP is provided. However the same process in then repeated in wait_for_interface_or_ip. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
The ironic scripts either use PROVISIONING_IP as an input or try to determine an IP address to bind the sockets to. This results in IRONIC_IP being defined once the process is complete, and it can carry either an IPv4 or an IPv6 address. Likely, the assumption is that on Linux, by default, IPv4-mapped IPv6 addresses can be leveraged to serve both IPv4 and IPv6 through a single socket. However this is not a good practice and two separate sockets should be used instead, whenever possible. This change modifies such logic by - introducing the variable IRONIC_IPV6 alongside the existing IRONIC_IP - matching IRONIC_IP and attempting to populate both variables Please note that hostname based URLs, with both A and AAAA records, are also required for a fully working dual-stack configuration. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
In a dual-stack scenario, especially when deploying in direct mode via virtual media, it might be useful to 1) use a hostname to enable "dual IP" URLs 2) have ironic bind to those two addresses, if found on the system. To make this possible, this commit introduces: - a new user environment variable named IRONIC_URL_HOSTNAME, to be used as immutable external only input, to derive IRONIC_URL_HOST and the IP addresses to bind on - a new utility function named "get_ip_of_hostname" to help look up the A and AAAA records - additional logic to look for the returned address on the system, for binding the processes; this new logic has lower priority than PROVISIONING_IP (which can then be used to enforce one specific IP version) and PROVISIONING_INTERFACE Note, while IRONIC_URL_HOSTNAME and PROVISIONING_IP are considered to be mutually exclusive, IRONIC_URL_HOSTNAME and PROVISIONING_INTERFACE are not. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
As per the Ironic documentation: "This field [my_ip] does accept an IPv6 address as an override for templates and URLs, however it is recommended that [DEFAULT]my_ipv6 is used along with DNS names for service URLs for dual-stack environments." Fill my_ipv6 when an IPv6 address has been found for binding. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
Prioritize IPv6 over IPv4 when available to set host_ip in ironic.conf when LISTEN_ALL_INTERFACES is not set to true. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
When LISTEN_ALL_INTERFACES is not set, Apache should make Ironic API avaiable on either or both IPv4 and IPv6 sockets, depending on the addresses requested or found on the system. Make sure to set the "Listen" directive according to ENABLE_IPV4 and ENABLE_IPV4, and the VirtualHost when IRONIC_URL_HOSTNAME is present. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
Enable the use of individual IPv4 and IPv6 sockets when the respective IP is detected and LISTEN_ALL_INTERFACES is not set to true. This allows to correctly bind to both the IPv4 and IPv6 addresses found and not just one of them. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
Enable the use of two separate sockets for IPv4 and IPv6 when LISTEN_ALL_INTERFACES is set to true. While desirable, on Linux Apache uses IPv4-mapped IPv6 addresses by default, thus leveraging a single IPv6 socket for IPv4 connections as well. This behaviour is far from being desirable and can be disabled at compile time via the "--disable-v4-mapped" flag, so make sure both an ANY address Listen directive is present for both IPv4 and IPv6. When Apache is compiled with "--enable-v4-mapped", the IPv4 socket will be simply ignored. Please see https://httpd.apache.org/docs/2.4/bind.html for more information. Signed-off-by: Marco Chiappero <marco.chiappero@suse.com>
|
/ok-to-test |
metal3-io-bot
added
ok-to-test
|
/cc @nuhakala /cc @dtantsur @elfosardo |
nuhakala
left a comment
nuhakala
left a comment
There was a problem hiding this comment.
Otherwise the code looks understandable, but in my opinion it would be clearer to rename IRONIC_IP into IRONIC_IPV4 to highlight that it is v4 address in a dual stack environment. And then drop the version from variables which can contain both v4 and v6 addresses, such as IRONIC_HOST_IP.
I would take the chance to highlight the need for proper documentation of the binding logic as well as proper input validation and logging: variables like PROVISIONING_IP, PROVISIONING_INTERFACE, PROVISIONING_MACS (and the new IRONIC_URL_HOSTNAME) can override each other or, in some cases, be optional, but no such logic is clearly stated. Last but not least, at this point bash is showing its limits and it could make sense to start migrating some of this potentially new code to a different programming language.
I am not the most familiar with this project but at this point I am not sure if there even is any certain logic behind the env variables. The variables and logic are most likely added to cover some edge cases when needed and hence they can overwrite each other.
I personally agree that this would be easier to write with more powerful programming language, but that is up to maintainers.
|
@mchiappero could you check the failing shellcheck? |
Sure, I can fix everything reported, although I'm about to go on PTO. I have been waiting to see if there were bigger general or design comments first. There is actually one main open point for me, I have come to realize in the meantime: the use of clusterIP; which is modified with this patch. My understanding is that clusterIP is not public API strictly speaking (it's not mentioned in https://github.com/metal3-io/ironic-image/blob/main/README.md), but it's actually consumed by the ironic scripts, in order to support the case where the API service will listen on any address, but the URLs will refer to a Load Balancer external IP. So, if this is the case, I would need to bring back this pre-existing behaviour, but we could also discuss about a longer term solution in terms of env variables/external API. I hope this makes sense, I can elaborate more in details otherwise. |
In my opinion, it wouldn't be bad to provide clear error messages or warning regarding conflicting or overlapping input, but also maybe a table in the official documentation better explaining the logic and priorities of these control variables. Of course it's not that handy in bash (and shifting to something else, e.g. python, is not effort free but definitely possible), but I might find some time to contribute to this if there is some consensus. |
Please do fix it! It is a bit discouraging for the reviewers when the PR is WIP and also has failing tests. Then we know there will need to be more changes to review later so we may just wait a bit and see if some of that would go away. I didn't understand the part about clusterIP. Which variable are you talking about? Adding one more who probably has good insights into this (although he is on vacation at the moment). Let's also try the tests and see how it goes. |
|
@mchiappero: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
metal3-io-bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi all, sorry for the very long delay, but a few other tasks got in the way... I have spent some time reworking the set with some small code changes, but especially reordering the commits, which means that the whole patchset is now split into four different blocks, addressing some refactoring, stricter checking and new features. As a consequence I'll be closing this PR and opening some new ones (#787, #788, #789, #791) that should be easier to review and discuss if something must change or be dropped. Please note that the branches are stacked, so should be reviewed and merged in the same order. |
5 participants
What this PR does / why we need it:
The objective of this PR is to enable a proper dual-stack configuration by means of specific environment variables. This implies some preliminary refactoring and slight changes to the control flow in the startup scripts prior to the addition of some logic to determine and set, whenever possible, both an IPv4 and an IPv6 address. Also, introduce the possibility to use a hostname, a key element to achieve dual-stack URLs.
This PR is still WIP as 1) code must reviewed and consensus must be reached (including on breaking this PR into multiple ones) 2) little testing has been run 3) dnsmasq changes have not been produced yet 4) likely more documentation is needed
I would take the chance to highlight the need for proper documentation of the binding logic as well as proper input validation and logging: variables like PROVISIONING_IP, PROVISIONING_INTERFACE, PROVISIONING_MACS (and the new IRONIC_URL_HOSTNAME) can override each other or, in some cases, be optional, but no such logic is clearly stated. Last but not least, at this point bash is showing its limits and it could make sense to start migrating some of this potentially new code to a different programming language.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #