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

fix: allow FQDNs as controlPlaneEndpoint #157

Merged
merged 4 commits into from
Apr 17, 2024
Merged

fix: allow FQDNs as controlPlaneEndpoint #157

merged 4 commits into from
Apr 17, 2024

Conversation

pborn-ionos
Copy link
Contributor

@pborn-ionos pborn-ionos commented Mar 26, 2024

Issue #, if available:
fixes #153

Description of changes:
Checks if the passed controlPlaneEndpoint.host is a resolvable FQDN, and if true, uses the IP from it for further validation.

Testing performed:
See chat.

Copy link
Member

@mcbenjemaa mcbenjemaa left a comment

Choose a reason for hiding this comment

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

This still accepts ip address ?

@pborn-ionos
Copy link
Contributor Author

pborn-ionos commented Mar 26, 2024

@mcbenjemaa yes. Just double checked it and added some testcases for plain ipv4 / ipv6 endpoints.

After all, all we do here is, prior to running our actual tests, checking if the controlPlaneEndpoint is a resolvable FQDN and if so, continue with it's IP :)

Copy link
Collaborator

@65278 65278 left a comment

Choose a reason for hiding this comment

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

This works, but is a bit awkward. Since we agreed that I'll be taking over, no further action is required.

A couple of remarks:

  • isValidFQDN is actually a test if a hostname resolves, and has nothing to do with FQDNs. There's no requirement on a hostname needing to be an FQDN to be valid.
  • You can resolve hostnames which are illegal as actual hostnames. As an example, _acme-challenge.your.domain is valid to resolve, but not a hostname. This is useful for text type records.
  • We're now implicitly depending on endpointHostIP not returning square brackets. I don't think this assumption holds

@pborn-ionos
Copy link
Contributor Author

  • We're now implicitly depending on endpointHostIP not returning square brackets. I don't think this assumption holds

It does and we should not accept them here. Otherwise we have to strip them again in other places, where it is passed to other resources. We've already been there and passing a controlplaneEndpoint.Host like [::1] just caused issues during provisioning.

@mcbenjemaa
Copy link
Member

I have tested ipv6, but it doesn't work for proxmoxcluster. But the cluster was created successfully

cluster.cluster.x-k8s.io/test-dev created
  1 err  kind-vault-onboard kube 
Warning: cannot create proxmox cluster test-dev
The ProxmoxCluster "test-dev" is invalid: spec.controlplaneEndpoint: Invalid value: "[2a01:c23:61ce:d200::ff6:99bf:cf43]": provided endpoint address is not a valid IP or FQDN

also when i put a hostname in the controlplaneendpoint, it works for cluster but not for ProxmoxCluster.

those cases don't work:

  • test.example.com
  • test-dev
  • "[2a01:c23:61ce:d200::ff6:99bf:cf43]"
  • "nginx.nginx.svc.cluster.local"

@pborn-ionos
Copy link
Contributor Author

With square brackets is not supposed to work. And if the other ones don't resolve, they're not supposed to work either, as with my change the IP behind the FQDN is required for further endpoint validation.

@mcbenjemaa
Copy link
Member

CAPI doesn't have any validation for this field, i think you can omit validation for the host.

@pborn-ionos
Copy link
Contributor Author

Either we validate the host, or we ditch the validateIPs function altogether. But then it should be kept in mind, that that function also validates, whether or not the given controlPlaneEndpoint collides with the cluster's given IP ranges.

@mcbenjemaa
Copy link
Member

We keep validations of IPs of the ipam, and the validation of collides with endpoint ip.
and we omit all validation to the endpoint host.
let's just keep it simple. then capi will actually through an error if the endpoint is not correct.

@mcbenjemaa
Copy link
Member

The workaround, in my opinion, is to let the user choose the control-plane endpoint, with no further validation,
and if the user chose a wrong endpoint , then capi will actually through an error state:

Screenshot 2024-04-02 at 14 58 48

@wikkyk
Copy link
Collaborator

wikkyk commented Apr 3, 2024

fixes #153

@wikkyk wikkyk added this to the v0.4.0 milestone Apr 3, 2024
@mcbenjemaa
Copy link
Member

mcbenjemaa commented Apr 3, 2024

I have raised this PR, that will make cluster to fail if machine has failed cloud-init run:
#166

@65278
Copy link
Collaborator

65278 commented Apr 8, 2024

This is done here. The PR is rather small, a couple of naming changes and a function to detect hostnames.
We error if our target is not an ip or a valid hostname, otherwise anything goes.

Copy link

@pborn-ionos pborn-ionos merged commit 59ad710 into main Apr 17, 2024
8 checks passed
@pborn-ionos pborn-ionos deleted the fix-153 branch April 17, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProxmoxCluster.spec.controlPlaneEndpoint.host does not accept FQDNs
4 participants