Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow EKG_HOST to use IPv4 and Host/Domain names #1834

Merged
merged 9 commits into from
Oct 27, 2024

Conversation

TrevorBenson
Copy link
Collaborator

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
Copy link
Collaborator Author

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 TrevorBenson requested review from rdlrt and Scitz0 October 19, 2024 20:06
Scitz0
Scitz0 previously requested changes Oct 19, 2024
Copy link
Contributor

@Scitz0 Scitz0 left a comment

Choose a reason for hiding this comment

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

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

scripts/cnode-helper-scripts/cntools.sh Show resolved Hide resolved
@TrevorBenson
Copy link
Collaborator Author

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 TrevorBenson requested a review from Scitz0 October 20, 2024 04:09
@Scitz0
Copy link
Contributor

Scitz0 commented Oct 20, 2024

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
Copy link
Contributor

Scitz0 commented Oct 20, 2024

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

@TrevorBenson TrevorBenson force-pushed the ekg-host-and-isvalidip branch from 902f238 to 7528b0f Compare October 21, 2024 00:37
@TrevorBenson TrevorBenson force-pushed the ekg-host-and-isvalidip branch from 7528b0f to 700d252 Compare October 21, 2024 00:39
@TrevorBenson TrevorBenson dismissed Scitz0’s stale review October 22, 2024 23:44

Changes applied, comments responded to.

Copy link
Contributor

@Scitz0 Scitz0 left a comment

Choose a reason for hiding this comment

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

LGTM

@Scitz0 Scitz0 merged commit 97e37fa into cardano-community:alpha Oct 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants