From a404ea0f56a21ed0672cb19ad288258541c1e1ec Mon Sep 17 00:00:00 2001 From: Matthias Bastian Date: Tue, 30 Apr 2024 11:32:27 +0200 Subject: [PATCH] Allow FQDN as the control plane endpoint host Until now, we assumed the control plane endpoint host to be an IP, but that's an unnecessary restriction. The template now consumes a new environment variable: `CONTROL_PLANE_ENDPOINT_HOST` It's optional and defaults to the value of the existing `CONTROL_PLANE_ENDPOINT_IP` environment variable. As the host is required to resolve to that IP, we'd actually not need it anymore to be given explicitly. However, as we still render the kube-vip manifest in the template and don't have anything nice there to do the translation automatically, we keep the explicit IP for now. The controller code is not aware of the `CONTROL_PLANE_ENDPOINT_IP` value, though. It only sees the control plane endpoint host, which can now be either an IP or an FQDN, so the controller must be able to resolve the name to use the corresponding IP to find the matching IP block and to find the correct IP failover group. Unit tests for DNS resolving were not as convenient as I expected. The stdlib tests don't provide any means to mock it, they even rely on external network reachability to resolve some Google domain names. They pointed to https://github.com/golang/go/issues/12503, though. There's an interesting link to https://github.com/ncruces/go-dns, showing how to inject custom behavior using a custom dialer, enabling things such as caching. It's overkill for us, though, so our used approach for unit testing is simpler. --- api/v1alpha1/ionoscloudcluster_types_test.go | 5 ++ docs/quickstart.md | 20 +++-- envfile.example | 1 + internal/service/cloud/ipblock.go | 27 ++++--- internal/service/cloud/network.go | 3 +- scope/cluster.go | 36 +++++++++ scope/cluster_test.go | 77 ++++++++++++++++++++ templates/cluster-template.yaml | 2 +- 8 files changed, 150 insertions(+), 21 deletions(-) diff --git a/api/v1alpha1/ionoscloudcluster_types_test.go b/api/v1alpha1/ionoscloudcluster_types_test.go index 2382821e..9e3d5b3b 100644 --- a/api/v1alpha1/ionoscloudcluster_types_test.go +++ b/api/v1alpha1/ionoscloudcluster_types_test.go @@ -73,6 +73,11 @@ var _ = Describe("IonosCloudCluster", func() { It("should allow creating valid clusters", func() { Expect(k8sClient.Create(context.Background(), defaultCluster())).To(Succeed()) }) + It("should work with a FQDN controlplane endpoint", func() { + cluster := defaultCluster() + cluster.Spec.ControlPlaneEndpoint.Host = "example.org" + Expect(k8sClient.Create(context.Background(), cluster)).To(Succeed()) + }) It("should not allow creating clusters with empty credential secret", func() { cluster := defaultCluster() cluster.Spec.CredentialsRef.Name = "" diff --git a/docs/quickstart.md b/docs/quickstart.md index 7a88a602..61bc3966 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -60,19 +60,23 @@ clusterctl init --infrastructure=ionoscloud CAPIC requires several environment variables to be set in order to create a Kubernetes cluster on IONOS Cloud. ```env -## -- Cloud specific environment variables -- ## +## -- Cloud-specific environment variables -- ## IONOS_TOKEN # The token of the IONOS Cloud account. -IONOS_API_URL # The API URL of the IONOS Cloud account. - # Defaults to https://api.ionos.com/cloudapi/v6 - -## -- Cluster API related environment variables -- ## -CONTROL_PLANE_ENDPOINT_IP # The IP address of the control plane endpoint. -CONTROL_PLANE_ENDPOINT_PORT # The port of the control plane endpoint. +IONOS_API_URL # The API URL of the IONOS Cloud account (optional). + # Defaults to https://api.ionos.com/cloudapi/v6. + +## -- Cluster API-related environment variables -- ## +CONTROL_PLANE_ENDPOINT_HOST # The control plane endpoint host (optional). + # If it's not an IP but an FQDN, the provider must be able to resolve it + # to the value for CONTROL_PLANE_ENDPOINT_IP. +CONTROL_PLANE_ENDPOINT_IP # The IPv4 address of the control plane endpoint. +CONTROL_PLANE_ENDPOINT_PORT # The port of the control plane endpoint (optional). + # Defaults to 6443. CONTROL_PLANE_ENDPOINT_LOCATION # The location of the control plane endpoint. CLUSTER_NAME # The name of the cluster. KUBERNETES_VERSION # The version of Kubernetes to be installed (can also be set via clusterctl). -## -- Kubernetes Cluster related environment variables -- ## +## -- Kubernetes Cluster-related environment variables -- ## IONOSCLOUD_CONTRACT_NUMBER # The contract number of the IONOS Cloud contract. IONOSCLOUD_DATACENTER_ID # The datacenter ID where the cluster should be created. IONOSCLOUD_MACHINE_NUM_CORES # The number of cores. diff --git a/envfile.example b/envfile.example index 33ec7eb2..83609fc9 100644 --- a/envfile.example +++ b/envfile.example @@ -6,6 +6,7 @@ export IONOS_API_URL="https://api.ionos.com/cloudapi/v6" # Cluster API related environment variables +export CONTROL_PLANE_ENDPOINT_HOST="example.org" export CONTROL_PLANE_ENDPOINT_IP="192.168.0.1" export CONTROL_PLANE_ENDPOINT_PORT=6443 export CONTROL_PLANE_ENDPOINT_LOCATION="de/txl" diff --git a/internal/service/cloud/ipblock.go b/internal/service/cloud/ipblock.go index 88e72e9d..6bf47099 100644 --- a/internal/service/cloud/ipblock.go +++ b/internal/service/cloud/ipblock.go @@ -253,11 +253,17 @@ func (s *Service) getControlPlaneEndpointIPBlock(ctx context.Context, cs *scope. if ipBlock != nil || ignoreNotFound(err) != nil { return ipBlock, err } + notFoundError := err s.logger.Info("IP block not found by ID, trying to find by listing IP blocks instead") - blocks, listErr := s.apiWithDepth(listIPBlocksDepth).ListIPBlocks(ctx) - if listErr != nil { - return nil, fmt.Errorf("failed to list IP blocks: %w", listErr) + blocks, err := s.apiWithDepth(listIPBlocksDepth).ListIPBlocks(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list IP blocks: %w", err) + } + + controlPlaneEndpointIP, err := cs.GetControlPlaneEndpointIP(ctx) + if err != nil { + return nil, err } var ( @@ -277,7 +283,7 @@ func (s *Service) getControlPlaneEndpointIPBlock(ctx context.Context, cs *scope. if err != nil { return nil, err } - case s.checkIfUserSetBlock(cs, props): + case s.checkIfUserSetBlock(controlPlaneEndpointIP, props): // NOTE: this is for when customers set IPs for the control plane endpoint themselves. foundBlock, err = s.cloudAPIStateInconsistencyWorkaround(ctx, &block) if err != nil { @@ -287,11 +293,11 @@ func (s *Service) getControlPlaneEndpointIPBlock(ctx context.Context, cs *scope. } if count > 1 { return nil, fmt.Errorf( - "cannot determine IP block for Control Plane Endpoint as there are multiple IP blocks with the name %s", + "cannot determine IP block for Control Plane Endpoint, as there are multiple IP blocks with the name %s", expectedName) } } - if count == 0 && cs.GetControlPlaneEndpoint().Host != "" { + if count == 0 && controlPlaneEndpointIP != "" { return nil, errUserSetIPNotFound } if foundBlock != nil { @@ -299,13 +305,12 @@ func (s *Service) getControlPlaneEndpointIPBlock(ctx context.Context, cs *scope. } // if we still can't find an IP block we return the potential // initial not found error. - return nil, err + return nil, notFoundError } -func (*Service) checkIfUserSetBlock(cs *scope.Cluster, props *sdk.IpBlockProperties) bool { - ip := cs.GetControlPlaneEndpoint().Host +func (*Service) checkIfUserSetBlock(controlPlaneEndpointIP string, props *sdk.IpBlockProperties) bool { ips := ptr.Deref(props.GetIps(), nil) - return ip != "" && slices.Contains(ips, ip) + return controlPlaneEndpointIP != "" && slices.Contains(ips, controlPlaneEndpointIP) } // cloudAPIStateInconsistencyWorkaround is a workaround for a bug where the API returns different states for the same @@ -322,7 +327,7 @@ func (s *Service) cloudAPIStateInconsistencyWorkaround(ctx context.Context, bloc func (s *Service) getIPBlockByID(ctx context.Context, ipBlockID string) (*sdk.IpBlock, error) { if ipBlockID == "" { - s.logger.Info("Could not find any IP block by ID as the provider ID is not set.") + s.logger.Info("Could not find any IP block by ID, as the provider ID is not set.") return nil, nil } ipBlock, err := s.ionosClient.GetIPBlock(ctx, ipBlockID) diff --git a/internal/service/cloud/network.go b/internal/service/cloud/network.go index e9103423..4604d2e6 100644 --- a/internal/service/cloud/network.go +++ b/internal/service/cloud/network.go @@ -278,7 +278,8 @@ func (s *Service) retrieveFailoverIPForMachine( log := s.logger.WithName("retrieveFailoverIPForMachine") if util.IsControlPlaneMachine(ms.Machine) { - return false, ms.ClusterScope.GetControlPlaneEndpoint().Host, nil + ip, err := ms.ClusterScope.GetControlPlaneEndpointIP(ctx) + return false, ip, err } failoverIP = ms.IonosMachine.Spec.FailoverIP diff --git a/scope/cluster.go b/scope/cluster.go index ff580fb8..c9d6945b 100644 --- a/scope/cluster.go +++ b/scope/cluster.go @@ -21,6 +21,9 @@ import ( "context" "errors" "fmt" + "net" + "net/netip" + "slices" "time" "k8s.io/client-go/util/retry" @@ -32,9 +35,17 @@ import ( infrav1 "github.com/ionos-cloud/cluster-api-provider-ionoscloud/api/v1alpha1" ) +// resolver is able to look up IP addresses from a given host name. +// The net.Resolver type (found at net.DefaultResolver) implements this interface. +// This is intended for testing. +type resolver interface { + LookupNetIP(ctx context.Context, network, host string) ([]netip.Addr, error) +} + // Cluster defines a basic cluster context for primary use in IonosCloudClusterReconciler. type Cluster struct { patchHelper *patch.Helper + resolver resolver Cluster *clusterv1.Cluster IonosCluster *infrav1.IonosCloudCluster } @@ -70,6 +81,7 @@ func NewCluster(params ClusterParams) (*Cluster, error) { Cluster: params.Cluster, IonosCluster: params.IonosCluster, patchHelper: helper, + resolver: net.DefaultResolver, } return clusterScope, nil @@ -80,6 +92,30 @@ func (c *Cluster) GetControlPlaneEndpoint() clusterv1.APIEndpoint { return c.IonosCluster.Spec.ControlPlaneEndpoint } +// GetControlPlaneEndpointIP returns the endpoint IP for the IonosCloudCluster. +// If the endpoint host is unset (neither an IP nor an FQDN), it will return an empty string. +func (c *Cluster) GetControlPlaneEndpointIP(ctx context.Context) (string, error) { + host := c.GetControlPlaneEndpoint().Host + if host == "" { + return "", nil + } + + if ip := net.ParseIP(host); ip != nil { + return ip.String(), nil + } + + // If the host is not an IP, try to resolve it. + ips, err := c.resolver.LookupNetIP(ctx, "ip4", host) + if err != nil { + return "", fmt.Errorf("failed to resolve control plane endpoint IP: %w", err) + } + + // Sort IPs to deal with random order intended for load balancing. + slices.SortFunc(ips, func(a, b netip.Addr) int { return a.Compare(b) }) + + return ips[0].String(), nil +} + // SetControlPlaneEndpointIPBlockID sets the IP block ID in the IonosCloudCluster status. func (c *Cluster) SetControlPlaneEndpointIPBlockID(id string) { c.IonosCluster.Status.ControlPlaneEndpointIPBlockID = id diff --git a/scope/cluster_test.go b/scope/cluster_test.go index d8165dfe..36602920 100644 --- a/scope/cluster_test.go +++ b/scope/cluster_test.go @@ -17,6 +17,9 @@ limitations under the License. package scope import ( + "context" + "net" + "net/netip" "testing" "github.com/stretchr/testify/require" @@ -81,7 +84,81 @@ func TestNewClusterMissingParams(t *testing.T) { params, err := NewCluster(test.params) require.NoError(t, err) require.NotNil(t, params) + require.Equal(t, net.DefaultResolver, params.resolver) } }) } } + +type mockResolver struct { + addrs map[string][]netip.Addr +} + +func (m *mockResolver) LookupNetIP(_ context.Context, _, host string) ([]netip.Addr, error) { + return m.addrs[host], nil +} + +func resolvesTo(ips ...string) []netip.Addr { + res := make([]netip.Addr, 0, len(ips)) + for _, ip := range ips { + res = append(res, netip.MustParseAddr(ip)) + } + return res +} + +func TestCluster_GetControlPlaneEndpointIP(t *testing.T) { + tests := []struct { + name string + host string + resolver resolver + want string + }{ + { + name: "host empty", + host: "", + want: "", + }, + { + name: "host is IP", + host: "127.0.0.1", + want: "127.0.0.1", + }, + { + name: "host is FQDN with single IP", + host: "localhost", + resolver: &mockResolver{ + addrs: map[string][]netip.Addr{ + "localhost": resolvesTo("127.0.0.1"), + }, + }, + want: "127.0.0.1", + }, + { + name: "host is FQDN with multiple IPs", + host: "example.org", + resolver: &mockResolver{ + addrs: map[string][]netip.Addr{ + "example.org": resolvesTo("2.3.4.5", "1.2.3.4"), + }, + }, + want: "1.2.3.4", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Cluster{ + resolver: tt.resolver, + IonosCluster: &infrav1.IonosCloudCluster{ + Spec: infrav1.IonosCloudClusterSpec{ + ControlPlaneEndpoint: clusterv1.APIEndpoint{ + Host: tt.host, + }, + }, + }, + } + got, err := c.GetControlPlaneEndpointIP(context.Background()) + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/templates/cluster-template.yaml b/templates/cluster-template.yaml index 61045484..5960918f 100644 --- a/templates/cluster-template.yaml +++ b/templates/cluster-template.yaml @@ -31,7 +31,7 @@ metadata: name: "${CLUSTER_NAME}" spec: controlPlaneEndpoint: - host: ${CONTROL_PLANE_ENDPOINT_IP} + host: ${CONTROL_PLANE_ENDPOINT_HOST:-${CONTROL_PLANE_ENDPOINT_IP}} port: ${CONTROL_PLANE_ENDPOINT_PORT:-6443} location: ${CONTROL_PLANE_ENDPOINT_LOCATION} contractNumber: "${IONOSCLOUD_CONTRACT_NUMBER}"