guild-operators icon indicating copy to clipboard operation
guild-operators copied to clipboard

Allow EKG_HOST to use IPv4 and Host/Domain names

Open TrevorBenson opened this issue 1 year ago • 4 comments

Description

  • Splits the isValidIPv4 function so that it returns 1 on invalid IPv4 addresses.
  • Adds new isValidHostnameOrDomain function to provide the extra check that isValidIPv4 was performing to allow cntools.sh to validate the relays_ip_enter.
  • Updates the EKG_HOST IP check to work for IPv4 or valid hostname / domainnames

Where should the reviewer start?

  1. Review the pool registration steps.
  2. Decide if the original logic should remain valid: a. Should [d] A or AAAA DNS record perform host/domain record validation? b. Should [i] IPv4/v6 address accept a host or domain name or be changed to only accept IPv4 and IPv6 addresses?

Motivation and context

  1. Allowing EKG_HOST to be a non IPv4 address (for decoupled & containerized/k8s environments).
  2. Ensure that isValidIPv4 correctly invalidates IPv4 addresses.

Which issue it fixes?

Closes #1832 Closes #1833

How has this been tested?

  • Building locally into a test container and verifying that cncli.sh [sync|leaderlog|validatre] no longer errors on valid hostnames or IPv6 and can connect via EKG.
    • Resolvable names like EKG_HOST=cardano-node pass. The cncli.sh sync, leaderlog, & validate subcommands all work as expected.
    • EKG_HOST does not permit IPv6 as enabling IPv6 on the node does not appear to enable it for EKG_PORT.
  • NO TESTS YET:
    • Pool registration. Waiting on feedback on Where should reviewer start 2a and 2b to test the original logic, or make additional commits and test if

TrevorBenson avatar Oct 19 '24 20:10 TrevorBenson

Other thoughts:

isvalidIPv4 correctly invalidating non IPv4 addresses could be chained to an isNameResolvable function. Not included in this PR because it is beyond scope of the issues being addressed. However this could enable logic if pool registration happens on an online node via the function:

isNameResolvable() {
  local name=$1
  # Check if $1 is empty
  [[ -z ${name} ]] && return 1

  # Use dig to resolve the name and capture the first result
  local resolved_ip=$(dig +short ${name} | head -n1)
  [[ -z ${resolved_ip} ]] && return 1
  if ! isValidIPv4 ${resolved_ip} && ! isValidIPv6 ${resolved_ip} ; then
    return 1
  fi
  return 0
}

Then when entering an A or AAAA record the check could verify the record can be resolved and notify the operator when the relay would appear to not be reachable, something like:

  if isValidHostnameOrDomain "${relay_ip_enter}"; then
    if ! isResolvableName "${relay_ip_enter}"; then
      println ERROR "${FG_RED}ERROR${NC}: Hostname/domain name cannot be resolved!"
      exit 1

To avoid suggesting that a resolvable hostname like localhost or other values in /etc/hosts is valid to register on chain splitting the hostname validation from the domain name validation would be prudent.

TrevorBenson avatar Oct 19 '24 20:10 TrevorBenson

In addition to comment added, topologyupdater script also needs to be updated.

Fixed. Although, just for clarification the original isValidIPv4 and the new split for isValidIPv4 and isValidHostnameOrDomain essentially bypasses IPv4 validity because an invalidIPv4 address is a valid Host/Domain name based on the original regex.

i.e. 127.0.0.1.2, 10.256.384.512 and 256.512.768.1024 are all valid under the prior isValidIPv4 regex for hostnames, as well as the new isValidHostnameOrDomain (which only inherited the prior regex into a separate function).

TrevorBenson avatar Oct 20 '24 00:10 TrevorBenson

In addition to comment added, topologyupdater script also needs to be updated.

Fixed. Although, just for clarification the original isValidIPv4 and the new split for isValidIPv4 and isValidHostnameOrDomain essentially bypasses IPv4 validity because an invalidIPv4 address is a valid Host/Domain name based on the original regex.

i.e. 127.0.0.1.2, 10.256.384.512 and 256.512.768.1024 are all valid under the prior isValidIPv4 regex for hostnames, as well as the new isValidHostnameOrDomain (which only inherited the prior regex into a separate function).

This is true, Not sure how to best deal with that. I dont really think it matters, it will just fail to work and you have to fix your config.

Scitz0 avatar Oct 20 '24 12:10 Scitz0

The worst thing and what we dont want to happen is that we prevent valid input. If bad data slips through, thats fine.

Scitz0 avatar Oct 20 '24 12:10 Scitz0