From 65c30fb7554d1e6d0e98012dc956baaba426aca2 Mon Sep 17 00:00:00 2001 From: Enrico Candino Date: Tue, 16 Jun 2026 12:16:48 +0200 Subject: [PATCH] addressed comments --- cli/cmds/kubeconfig.go | 3 +- pkg/controller/cluster/cluster.go | 19 +++-- pkg/controller/cluster/hcp.go | 62 +++++++------- pkg/controller/cluster/hcp_test.go | 35 ++++++-- pkg/controller/cluster/server/config.go | 2 +- pkg/controller/cluster/server/endpoint.go | 18 +++-- .../cluster/server/endpoint_test.go | 80 +++++++++++++++++++ pkg/controller/cluster/status.go | 12 +++ 8 files changed, 179 insertions(+), 52 deletions(-) create mode 100644 pkg/controller/cluster/server/endpoint_test.go diff --git a/cli/cmds/kubeconfig.go b/cli/cmds/kubeconfig.go index 1b3ad1b5..8d6e3cac 100644 --- a/cli/cmds/kubeconfig.go +++ b/cli/cmds/kubeconfig.go @@ -5,7 +5,6 @@ import ( "net/url" "os" "path/filepath" - "strings" "time" "github.com/sirupsen/logrus" @@ -147,7 +146,7 @@ func resolveServerHost(restConfigHost, override string) (string, error) { return "", err } - return strings.Split(u.Host, ":")[0], nil + return u.Hostname(), nil } func writeKubeconfigFile(cluster *v1beta1.Cluster, kubeconfig *clientcmdapi.Config, configName string) error { diff --git a/pkg/controller/cluster/cluster.go b/pkg/controller/cluster/cluster.go index 9a4601fd..6b5326bc 100644 --- a/pkg/controller/cluster/cluster.go +++ b/pkg/controller/cluster/cluster.go @@ -448,11 +448,13 @@ func (c *ClusterReconciler) reconcile(ctx context.Context, cluster *v1beta1.Clus return err } - // In hcp mode, derive the K3s installer command end-users run on their - // external nodes and surface it on the Cluster status. We also own the - // default/kubernetes Endpoints inside the virtual cluster (the apiserver - // reconciler is disabled for HCP) so external-node pods can reach the - // in-cluster apiserver ClusterIP. + // In hcp mode, validate that the cluster has an externally-routable + // API server endpoint (NodePort / LoadBalancer / Ingress) so external + // workers can join. The join command itself is printed by the CLI + // (`k3kcli cluster create` / `k3kcli kubeconfig generate`). + // We also own the default/kubernetes Endpoints/EndpointSlice inside the + // virtual cluster (the apiserver reconciler is disabled for HCP) so + // external-node pods can reach the in-cluster apiserver ClusterIP. if cluster.Spec.Mode == v1beta1.HCPClusterMode { if err := c.ensureHCPRegistration(ctx, cluster); err != nil { return err @@ -916,9 +918,10 @@ func (c *ClusterReconciler) bindClusterRoles(ctx context.Context, cluster *v1bet } func (c *ClusterReconciler) ensureAgent(ctx context.Context, cluster *v1beta1.Cluster, serviceIP, token string) error { - // hcp mode is BYO-node by design: external (out-of-host-cluster) nodes join - // using the standard K3s installer command surfaced via Status.HCPRegistration. - // k3k therefore does not provision any agent pods on the host cluster. + // hcp mode is BYO-node by design: external (out-of-host-cluster) nodes + // join using the standard K3s installer command, which is printed by + // `k3kcli cluster create` / `k3kcli kubeconfig generate`. k3k therefore + // does not provision any agent pods on the host cluster. if cluster.Spec.Mode == v1beta1.HCPClusterMode { return nil } diff --git a/pkg/controller/cluster/hcp.go b/pkg/controller/cluster/hcp.go index c6d5f299..18d44fd0 100644 --- a/pkg/controller/cluster/hcp.go +++ b/pkg/controller/cluster/hcp.go @@ -2,12 +2,12 @@ package cluster import ( "context" + "errors" "fmt" "net" "net/url" "strconv" - "k8s.io/apimachinery/pkg/api/meta" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" corev1 "k8s.io/api/core/v1" @@ -19,13 +19,22 @@ import ( "github.com/rancher/k3k/pkg/controller/cluster/server" ) -// ensureHCPRegistration computes the K3s installer command external nodes can -// run to join an HCP-mode cluster and stores it on cluster.Status.HCPRegistration. +// ErrHCPNoExternalEndpoint is returned by ensureHCPRegistration when an +// HCP-mode cluster has no externally-routable endpoint (no NodePort, +// LoadBalancer or Ingress) so external worker nodes cannot reach the API +// server. updateStatus translates it into a Ready=False condition with +// reason HCPNoExternalEndpoint instead of failing the reconcile outright. +var ErrHCPNoExternalEndpoint = errors.New("HCP cluster has no external endpoint") + +// ensureHCPRegistration verifies that an HCP-mode cluster exposes an +// externally-routable API server endpoint so external worker nodes can join. +// Join instructions (the `curl ... | sh -` line) are printed by the CLI +// (`k3kcli cluster create` / `k3kcli kubeconfig generate`); the controller +// does not persist them on the Cluster object. // -// When the cluster's Service is not externally reachable (no NodePort, -// LoadBalancer or Ingress configured) the command cannot be built; in that -// case the Ready condition is set to False with reason HCPNoExternalEndpoint -// so the operator surfaces the problem without failing the reconciliation. +// Returns ErrHCPNoExternalEndpoint when no NodePort, LoadBalancer or Ingress +// is configured, which updateStatus surfaces as Ready=False with reason +// HCPNoExternalEndpoint. func (c *ClusterReconciler) ensureHCPRegistration(ctx context.Context, cluster *v1beta1.Cluster) error { log := ctrl.LoggerFrom(ctx) @@ -35,15 +44,10 @@ func (c *ClusterReconciler) ensureHCPRegistration(ctx context.Context, cluster * } if !external { - log.Info("HCP cluster has no externally-routable endpoint; skipping registration command", + log.Info("HCP cluster has no externally-routable endpoint", "cluster", cluster.Name, "namespace", cluster.Namespace) - meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ - Type: ConditionReady, - Status: metav1.ConditionFalse, - Reason: ReasonHCPNoExternalEndpoint, - Message: "HCP cluster has no external endpoint; set spec.expose.nodePort, spec.expose.loadBalancer or spec.expose.ingress so external nodes can reach the API server", - }) + return ErrHCPNoExternalEndpoint } return nil @@ -103,8 +107,9 @@ func (c *ClusterReconciler) ensureHCPKubernetesEndpointSlice(ctx context.Context } if !external { - // ensureHCPRegistration already surfaces this via Ready=False; - // nothing for us to do here. + // Defensive: reconcile would have already short-circuited with + // ErrHCPNoExternalEndpoint via ensureHCPRegistration before reaching + // here, but skip gracefully if invoked directly. return nil } @@ -113,7 +118,7 @@ func (c *ClusterReconciler) ensureHCPKubernetesEndpointSlice(ctx context.Context return fmt.Errorf("parsing HCP server URL %q: %w", rawURL, err) } - addr, err := hcpEndpointAddress(host) + addr, err := hcpEndpointAddress(ctx, host) if err != nil { return err } @@ -185,8 +190,9 @@ func (c *ClusterReconciler) ensureHCPKubernetesEndpoints(ctx context.Context, cl } if !external { - // ensureHCPRegistration already surfaces this via Ready=False; - // nothing for us to do here. + // Defensive: reconcile would have already short-circuited with + // ErrHCPNoExternalEndpoint via ensureHCPRegistration before reaching + // here, but skip gracefully if invoked directly. return nil } @@ -195,7 +201,7 @@ func (c *ClusterReconciler) ensureHCPKubernetesEndpoints(ctx context.Context, cl return fmt.Errorf("parsing HCP server URL %q: %w", rawURL, err) } - addr, err := hcpEndpointAddress(host) + addr, err := hcpEndpointAddress(ctx, host) if err != nil { return err } @@ -282,10 +288,10 @@ func parseHCPHostPort(rawURL string) (string, int32, error) { // hcpEndpointAddress builds a corev1.EndpointAddress from the externally // reachable host. Endpoints require an IP; if the host is a DNS name we -// resolve it. The Hostname field is intentionally left unset: the -// kubernetes API validates it as a DNS-1123 label (no dots), so an FQDN -// like "host.example.com" would be rejected. -func hcpEndpointAddress(host string) (corev1.EndpointAddress, error) { +// resolve it. The Hostname field is intentionally left unset: +// the kubernetes API validates it as a DNS-1123 label (no dots), +// so an FQDN like "host.example.com" would be rejected. +func hcpEndpointAddress(ctx context.Context, host string) (corev1.EndpointAddress, error) { if ip := net.ParseIP(host); ip != nil { if ip.IsLoopback() { return corev1.EndpointAddress{}, fmt.Errorf("HCP endpoint host %q is a loopback address and cannot be used", host) @@ -294,16 +300,16 @@ func hcpEndpointAddress(host string) (corev1.EndpointAddress, error) { return corev1.EndpointAddress{IP: host}, nil } - ips, err := net.LookupIP(host) + ipAddrs, err := net.DefaultResolver.LookupIPAddr(ctx, host) if err != nil { return corev1.EndpointAddress{}, fmt.Errorf("HCP endpoint host %q is not an IP and does not resolve: %w", host, err) } var filteredIPs []net.IP - for _, ip := range ips { - if !ip.IsLoopback() { - filteredIPs = append(filteredIPs, ip) + for _, addr := range ipAddrs { + if !addr.IP.IsLoopback() { + filteredIPs = append(filteredIPs, addr.IP) } } diff --git a/pkg/controller/cluster/hcp_test.go b/pkg/controller/cluster/hcp_test.go index c85ef986..f264d2b2 100644 --- a/pkg/controller/cluster/hcp_test.go +++ b/pkg/controller/cluster/hcp_test.go @@ -2,11 +2,11 @@ package cluster import ( "context" + "errors" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -35,7 +35,7 @@ func Test_ensureHCPRegistration(t *testing.T) { }, } - t.Run("clusterip-only service sets degraded condition and clears registration", func(t *testing.T) { + t.Run("clusterip-only service returns ErrHCPNoExternalEndpoint", func(t *testing.T) { svc := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: server.ServiceName(cluster.Name), @@ -54,12 +54,31 @@ func Test_ensureHCPRegistration(t *testing.T) { r := &ClusterReconciler{Client: fakeClient} c := cluster.DeepCopy() - require.NoError(t, r.ensureHCPRegistration(context.Background(), c)) + err := r.ensureHCPRegistration(context.Background(), c) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrHCPNoExternalEndpoint)) + }) - cond := meta.FindStatusCondition(c.Status.Conditions, ConditionReady) - require.NotNil(t, cond) - assert.Equal(t, metav1.ConditionFalse, cond.Status) - assert.Equal(t, ReasonHCPNoExternalEndpoint, cond.Reason) + t.Run("nodeport service returns no error", func(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: server.ServiceName(cluster.Name), + Namespace: cluster.Namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + ClusterIP: "10.43.0.50", + Ports: []corev1.ServicePort{ + {Name: "k3s-server-port", Port: 443, NodePort: 30443}, + }, + }, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(svc).Build() + r := &ClusterReconciler{Client: fakeClient} + + c := cluster.DeepCopy() + assert.NoError(t, r.ensureHCPRegistration(context.Background(), c)) }) } @@ -240,7 +259,7 @@ func Test_hcpEndpointAddress(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := hcpEndpointAddress(tt.input) + got, err := hcpEndpointAddress(context.Background(), tt.input) if tt.wantErr { require.Error(t, err) return diff --git a/pkg/controller/cluster/server/config.go b/pkg/controller/cluster/server/config.go index c24279d1..f4635003 100644 --- a/pkg/controller/cluster/server/config.go +++ b/pkg/controller/cluster/server/config.go @@ -79,7 +79,7 @@ func buildServerConfig(cluster *v1beta1.Cluster, initServer bool, serviceIP, tok // shared and hcp modes both run K3s with --disable-agent (agentless server). switch cluster.Spec.Mode { - case v1beta1.SharedClusterMode: + case "", v1beta1.SharedClusterMode: serverConfig.DisableAgent = true serverConfig.EgressSelectorMode = "disabled" serverConfig.Disable = []string{"servicelb", "traefik", "metrics-server", "local-storage"} diff --git a/pkg/controller/cluster/server/endpoint.go b/pkg/controller/cluster/server/endpoint.go index 23962630..02736f17 100644 --- a/pkg/controller/cluster/server/endpoint.go +++ b/pkg/controller/cluster/server/endpoint.go @@ -51,12 +51,20 @@ func ServerURL(ctx context.Context, c client.Client, cluster *v1beta1.Cluster, h port = k3kService.Spec.Ports[0].NodePort } case corev1.ServiceTypeLoadBalancer: - external = true - if len(k3kService.Status.LoadBalancer.Ingress) > 0 { - ip = k3kService.Status.LoadBalancer.Ingress[0].IP - } else { - logrus.Warn("No ingress found in LoadBalancer service.") + ingress := k3kService.Status.LoadBalancer.Ingress[0] + switch { + case ingress.IP != "": + ip = ingress.IP + external = true + case ingress.Hostname != "": + ip = ingress.Hostname + external = true + } + } + + if !external { + logrus.Warn("No usable ingress address found in LoadBalancer service.") } if len(k3kService.Spec.Ports) > 0 { diff --git a/pkg/controller/cluster/server/endpoint_test.go b/pkg/controller/cluster/server/endpoint_test.go new file mode 100644 index 00000000..22075c14 --- /dev/null +++ b/pkg/controller/cluster/server/endpoint_test.go @@ -0,0 +1,80 @@ +package server + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/rancher/k3k/pkg/apis/k3k.io/v1beta1" +) + +func Test_ServerURL_LoadBalancer(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, v1beta1.AddToScheme(scheme)) + + cluster := &v1beta1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "demo", Namespace: "team-a"}, + Status: v1beta1.ClusterStatus{TLSSANs: []string{"203.0.113.10", "lb.example.com"}}, + } + + tests := []struct { + name string + ingress []corev1.LoadBalancerIngress + wantURL string + wantExternal bool + }{ + { + name: "no ingress yet (LB still provisioning)", + ingress: nil, + wantExternal: false, + }, + { + name: "ingress with IP", + ingress: []corev1.LoadBalancerIngress{{IP: "203.0.113.10"}}, + wantURL: "https://203.0.113.10", + wantExternal: true, + }, + { + name: "ingress with hostname only", + ingress: []corev1.LoadBalancerIngress{{Hostname: "lb.example.com"}}, + wantURL: "https://lb.example.com", + wantExternal: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: ServiceName(cluster.Name), + Namespace: cluster.Namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + ClusterIP: "10.43.0.50", + Ports: []corev1.ServicePort{{Name: "k3s-server-port", Port: 443}}, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{Ingress: tt.ingress}, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(svc).Build() + url, external, err := ServerURL(context.Background(), c, cluster, "", 0) + require.NoError(t, err) + assert.Equal(t, tt.wantExternal, external) + + if tt.wantExternal { + assert.Equal(t, tt.wantURL, url) + } + }) + } +} diff --git a/pkg/controller/cluster/status.go b/pkg/controller/cluster/status.go index cff1024d..7c61ed3f 100644 --- a/pkg/controller/cluster/status.go +++ b/pkg/controller/cluster/status.go @@ -70,6 +70,18 @@ func (c *ClusterReconciler) updateStatus(ctx context.Context, cluster *v1beta1.C return } + if errors.Is(reconcileErr, ErrHCPNoExternalEndpoint) { + cluster.Status.Phase = v1beta1.ClusterPending + meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ + Type: ConditionReady, + Status: metav1.ConditionFalse, + Reason: ReasonHCPNoExternalEndpoint, + Message: "HCP cluster has no external endpoint; set spec.expose.nodePort, spec.expose.loadBalancer or spec.expose.ingress so external nodes can reach the API server", + }) + + return + } + // If there's an error, but it's not a validation error, the cluster is in a failed state. if reconcileErr != nil { cluster.Status.Phase = v1beta1.ClusterFailed