diff --git a/api/v1alpha1/tenantcontrolplane_types.go b/api/v1alpha1/tenantcontrolplane_types.go index 100949c..9174a72 100644 --- a/api/v1alpha1/tenantcontrolplane_types.go +++ b/api/v1alpha1/tenantcontrolplane_types.go @@ -356,6 +356,15 @@ type ServiceSpec struct { AdditionalPorts []AdditionalPort `json:"additionalPorts,omitempty"` // ServiceType allows specifying how to expose the Tenant Control Plane. ServiceType ServiceType `json:"serviceType"` + // AllocateLoadBalancerNodePorts defines whether NodePorts are automatically allocated + // for the Service when serviceType is LoadBalancer. It maps directly to the Service's + // spec.allocateLoadBalancerNodePorts. When nil, the Kubernetes default (true) applies, + // preserving existing behaviour. Set to false to expose the Tenant Control Plane only + // via the LoadBalancer IP and ClusterIP, without a per-node NodePort. This field is only + // valid when serviceType is LoadBalancer; setting it with any other serviceType is + // rejected by validation. + //+optional + AllocateLoadBalancerNodePorts *bool `json:"allocateLoadBalancerNodePorts,omitempty"` } // AddonSpec defines the spec for every addon. @@ -483,6 +492,7 @@ type DataStoreOverride struct { // +kubebuilder:validation:XValidation:rule="!has(oldSelf.dataStoreUsername) || has(self.dataStoreUsername)", message="unsetting the dataStoreUsername is not supported" // +kubebuilder:validation:XValidation:rule="!has(self.networkProfile.loadBalancerSourceRanges) || (size(self.networkProfile.loadBalancerSourceRanges) == 0 || self.controlPlane.service.serviceType == 'LoadBalancer')", message="LoadBalancer source ranges are supported only with LoadBalancer service type" // +kubebuilder:validation:XValidation:rule="!has(self.networkProfile.loadBalancerClass) || self.controlPlane.service.serviceType == 'LoadBalancer'", message="LoadBalancerClass is supported only with LoadBalancer service type" +// +kubebuilder:validation:XValidation:rule="!has(self.controlPlane.service.allocateLoadBalancerNodePorts) || self.controlPlane.service.serviceType == 'LoadBalancer'", message="allocateLoadBalancerNodePorts is supported only with LoadBalancer service type" // +kubebuilder:validation:XValidation:rule="self.controlPlane.service.serviceType != 'LoadBalancer' || (oldSelf.controlPlane.service.serviceType != 'LoadBalancer' && self.controlPlane.service.serviceType == 'LoadBalancer') || has(self.networkProfile.loadBalancerClass) == has(oldSelf.networkProfile.loadBalancerClass)",message="LoadBalancerClass cannot be set or unset at runtime" type TenantControlPlaneSpec struct { diff --git a/api/v1alpha1/tenantcontrolplane_types_test.go b/api/v1alpha1/tenantcontrolplane_types_test.go index b07cc1f..33ccc65 100644 --- a/api/v1alpha1/tenantcontrolplane_types_test.go +++ b/api/v1alpha1/tenantcontrolplane_types_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/gomega" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" ) var _ = Describe("Cluster controller", func() { @@ -118,4 +119,30 @@ var _ = Describe("Cluster controller", func() { Expect(err.Error()).To(ContainSubstring("advertiseAddress must be a valid IP address")) }) }) + + Context("AllocateLoadBalancerNodePorts", func() { + It("allows the field when service type is LoadBalancer", func() { + tcp.Spec.ControlPlane.Service.ServiceType = ServiceTypeLoadBalancer + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = ptr.To(false) + + err := k8sClient.Create(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + }) + + It("allows creation when the field is unset and service type is not LoadBalancer", func() { + tcp.Spec.ControlPlane.Service.ServiceType = ServiceTypeNodePort + + err := k8sClient.Create(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + }) + + It("denies the field when service type is not LoadBalancer", func() { + tcp.Spec.ControlPlane.Service.ServiceType = ServiceTypeNodePort + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = ptr.To(false) + + err := k8sClient.Create(ctx, tcp) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("allocateLoadBalancerNodePorts is supported only with LoadBalancer service type")) + }) + }) }) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 7335a7e..a83069e 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1564,6 +1564,16 @@ func (in *NetworkProfileSpec) DeepCopyInto(out *NetworkProfileSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.ServiceCIDRs != nil { + in, out := &in.ServiceCIDRs, &out.ServiceCIDRs + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.PodCIDRs != nil { + in, out := &in.PodCIDRs, &out.PodCIDRs + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.DNSServiceIPs != nil { in, out := &in.DNSServiceIPs, &out.DNSServiceIPs *out = make([]string, len(*in)) @@ -1724,6 +1734,11 @@ func (in *ServiceSpec) DeepCopyInto(out *ServiceSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.AllocateLoadBalancerNodePorts != nil { + in, out := &in.AllocateLoadBalancerNodePorts, &out.AllocateLoadBalancerNodePorts + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceSpec. diff --git a/charts/kamaji-crds/hack/kamaji.clastix.io_tenantcontrolplanes_spec.yaml b/charts/kamaji-crds/hack/kamaji.clastix.io_tenantcontrolplanes_spec.yaml index 5304d13..360b750 100644 --- a/charts/kamaji-crds/hack/kamaji.clastix.io_tenantcontrolplanes_spec.yaml +++ b/charts/kamaji-crds/hack/kamaji.clastix.io_tenantcontrolplanes_spec.yaml @@ -8339,6 +8339,16 @@ versions: - targetPort type: object type: array + allocateLoadBalancerNodePorts: + description: |- + AllocateLoadBalancerNodePorts defines whether NodePorts are automatically allocated + for the Service when serviceType is LoadBalancer. It maps directly to the Service's + spec.allocateLoadBalancerNodePorts. When nil, the Kubernetes default (true) applies, + preserving existing behaviour. Set to false to expose the Tenant Control Plane only + via the LoadBalancer IP and ClusterIP, without a per-node NodePort. This field is only + valid when serviceType is LoadBalancer; setting it with any other serviceType is + rejected by validation. + type: boolean serviceType: description: ServiceType allows specifying how to expose the Tenant Control Plane. enum: @@ -8695,6 +8705,8 @@ versions: rule: '!has(self.networkProfile.loadBalancerSourceRanges) || (size(self.networkProfile.loadBalancerSourceRanges) == 0 || self.controlPlane.service.serviceType == ''LoadBalancer'')' - message: LoadBalancerClass is supported only with LoadBalancer service type rule: '!has(self.networkProfile.loadBalancerClass) || self.controlPlane.service.serviceType == ''LoadBalancer''' + - message: allocateLoadBalancerNodePorts is supported only with LoadBalancer service type + rule: '!has(self.controlPlane.service.allocateLoadBalancerNodePorts) || self.controlPlane.service.serviceType == ''LoadBalancer''' - message: LoadBalancerClass cannot be set or unset at runtime rule: self.controlPlane.service.serviceType != 'LoadBalancer' || (oldSelf.controlPlane.service.serviceType != 'LoadBalancer' && self.controlPlane.service.serviceType == 'LoadBalancer') || has(self.networkProfile.loadBalancerClass) == has(oldSelf.networkProfile.loadBalancerClass) status: diff --git a/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml b/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml index 431bd97..0e4411a 100644 --- a/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml +++ b/charts/kamaji/crds/kamaji.clastix.io_tenantcontrolplanes.yaml @@ -8347,6 +8347,16 @@ spec: - targetPort type: object type: array + allocateLoadBalancerNodePorts: + description: |- + AllocateLoadBalancerNodePorts defines whether NodePorts are automatically allocated + for the Service when serviceType is LoadBalancer. It maps directly to the Service's + spec.allocateLoadBalancerNodePorts. When nil, the Kubernetes default (true) applies, + preserving existing behaviour. Set to false to expose the Tenant Control Plane only + via the LoadBalancer IP and ClusterIP, without a per-node NodePort. This field is only + valid when serviceType is LoadBalancer; setting it with any other serviceType is + rejected by validation. + type: boolean serviceType: description: ServiceType allows specifying how to expose the Tenant Control Plane. enum: @@ -8703,6 +8713,8 @@ spec: rule: '!has(self.networkProfile.loadBalancerSourceRanges) || (size(self.networkProfile.loadBalancerSourceRanges) == 0 || self.controlPlane.service.serviceType == ''LoadBalancer'')' - message: LoadBalancerClass is supported only with LoadBalancer service type rule: '!has(self.networkProfile.loadBalancerClass) || self.controlPlane.service.serviceType == ''LoadBalancer''' + - message: allocateLoadBalancerNodePorts is supported only with LoadBalancer service type + rule: '!has(self.controlPlane.service.allocateLoadBalancerNodePorts) || self.controlPlane.service.serviceType == ''LoadBalancer''' - message: LoadBalancerClass cannot be set or unset at runtime rule: self.controlPlane.service.serviceType != 'LoadBalancer' || (oldSelf.controlPlane.service.serviceType != 'LoadBalancer' && self.controlPlane.service.serviceType == 'LoadBalancer') || has(self.networkProfile.loadBalancerClass) == has(oldSelf.networkProfile.loadBalancerClass) status: diff --git a/docs/content/reference/api.md b/docs/content/reference/api.md index 1daec4f..d5b9be3 100644 --- a/docs/content/reference/api.md +++ b/docs/content/reference/api.md @@ -31169,6 +31169,19 @@ Defining the options for the Tenant Control Plane Service resource. which targets the Tenant Control Plane pods.
false + + allocateLoadBalancerNodePorts + boolean + + AllocateLoadBalancerNodePorts defines whether NodePorts are automatically allocated +for the Service when serviceType is LoadBalancer. It maps directly to the Service's +spec.allocateLoadBalancerNodePorts. When nil, the Kubernetes default (true) applies, +preserving existing behaviour. Set to false to expose the Tenant Control Plane only +via the LoadBalancer IP and ClusterIP, without a per-node NodePort. This field is only +valid when serviceType is LoadBalancer; setting it with any other serviceType is +rejected by validation.
+ + false diff --git a/internal/resources/k8s_service_resource.go b/internal/resources/k8s_service_resource.go index 8a3541f..217e0d3 100644 --- a/internal/resources/k8s_service_resource.go +++ b/internal/resources/k8s_service_resource.go @@ -134,6 +134,22 @@ func (r *KubernetesServiceResource) mutate(ctx context.Context, tenantControlPla case kamajiv1alpha1.ServiceTypeLoadBalancer: r.resource.Spec.Type = corev1.ServiceTypeLoadBalancer + // AllocateLoadBalancerNodePorts is a declarative knob: an unset (nil) field + // means "use the Kubernetes LoadBalancer default" (true). We write that default + // explicitly so that clearing the field reverts the Service to the default, and + // so reconciles do not churn — writing the same `true` the API server already + // defaults to produces no diff, unlike writing nil over a server-defaulted true. + allocate := ptr.Deref(tenantControlPlane.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts, true) + r.resource.Spec.AllocateLoadBalancerNodePorts = &allocate + // Kubernetes does not deallocate an already-assigned NodePort when allocation + // is turned off, and the port loop above copies the live NodePort back, so clear + // it explicitly when allocation is disabled. + if !allocate { + for i := range r.resource.Spec.Ports { + r.resource.Spec.Ports[i].NodePort = 0 + } + } + if tenantControlPlane.Spec.NetworkProfile.LoadBalancerClass != nil { r.resource.Spec.LoadBalancerClass = ptr.To(*tenantControlPlane.Spec.NetworkProfile.LoadBalancerClass) } diff --git a/internal/resources/k8s_service_resource_test.go b/internal/resources/k8s_service_resource_test.go new file mode 100644 index 0000000..c3399a6 --- /dev/null +++ b/internal/resources/k8s_service_resource_test.go @@ -0,0 +1,203 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package resources_test + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + kamajiv1alpha1 "github.com/clastix/kamaji/api/v1alpha1" + "github.com/clastix/kamaji/internal/resources" +) + +var _ = Describe("KubernetesServiceResource AllocateLoadBalancerNodePorts", func() { + var ( + ctx context.Context + tcp *kamajiv1alpha1.TenantControlPlane + ) + + // tcpName is shared by both the TenantControlPlane ObjectMeta and the existingService + // fixture so they can't silently drift apart (Define() derives the Service name from + // the TCP name, so they must match). + const tcpName = "test-tcp" + + // seededNodePort is an arbitrary fixed value in the NodePort range that we + // pre-populate on the Service fixture. The fake client never allocates NodePorts, + // so this value is fully deterministic in the test — unlike a real cluster, where + // Kubernetes assigns one at random from 30000-32767. + const seededNodePort int32 = 30654 + + // existingService mimics an already-reconciled LoadBalancer Service that + // already has a NodePort assigned (as Kubernetes would allocate by default). + existingService := func() *corev1.Service { + return &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: tcpName, Namespace: "default"}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{{ + Name: "kube-apiserver", + Protocol: corev1.ProtocolTCP, + Port: 6443, + TargetPort: intstr.FromInt32(6443), + NodePort: seededNodePort, + }}, + }, + } + } + + newResource := func(objs ...client.Object) *resources.KubernetesServiceResource { + fakeClient := fake.NewClientBuilder(). + WithScheme(runtimeScheme). + WithObjects(objs...). + Build() + + return &resources.KubernetesServiceResource{Client: fakeClient} + } + + BeforeEach(func() { + ctx = context.Background() + tcp = &kamajiv1alpha1.TenantControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: tcpName, Namespace: "default"}, + Spec: kamajiv1alpha1.TenantControlPlaneSpec{ + ControlPlane: kamajiv1alpha1.ControlPlane{ + Service: kamajiv1alpha1.ServiceSpec{ + ServiceType: kamajiv1alpha1.ServiceTypeLoadBalancer, + }, + }, + NetworkProfile: kamajiv1alpha1.NetworkProfileSpec{ + Port: 6443, + }, + }, + } + }) + + It("disables allocation and clears an existing NodePort when set to false", func() { + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = ptr.To(false) + resource := newResource(existingService()) + + Expect(resource.Define(ctx, tcp)).To(Succeed()) + _, err := resource.CreateOrUpdate(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + + svc := &corev1.Service{} + Expect(resource.Client.Get(ctx, client.ObjectKey{Name: tcp.Name, Namespace: tcp.Namespace}, svc)).To(Succeed()) + Expect(svc.Spec.AllocateLoadBalancerNodePorts).NotTo(BeNil()) + Expect(*svc.Spec.AllocateLoadBalancerNodePorts).To(BeFalse()) + Expect(svc.Spec.Ports).To(HaveLen(1)) + Expect(svc.Spec.Ports[0].NodePort).To(BeZero()) + }) + + It("clears NodePorts on every port, including additional ports, when set to false", func() { + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = ptr.To(false) + tcp.Spec.ControlPlane.Service.AdditionalPorts = []kamajiv1alpha1.AdditionalPort{{ + Name: "metrics", + Protocol: corev1.ProtocolTCP, + Port: 9443, + TargetPort: intstr.FromInt32(9443), + }} + resource := newResource(existingService()) + + Expect(resource.Define(ctx, tcp)).To(Succeed()) + _, err := resource.CreateOrUpdate(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + + svc := &corev1.Service{} + Expect(resource.Client.Get(ctx, client.ObjectKey{Name: tcp.Name, Namespace: tcp.Namespace}, svc)).To(Succeed()) + Expect(svc.Spec.AllocateLoadBalancerNodePorts).NotTo(BeNil()) + Expect(*svc.Spec.AllocateLoadBalancerNodePorts).To(BeFalse()) + + // Both the kube-apiserver port and the additional port must be present with their + // NodePort cleared — the clearing loop covers every port, not just the first one. + Expect(svc.Spec.Ports).To(HaveLen(2)) + byName := map[string]corev1.ServicePort{} + for _, p := range svc.Spec.Ports { + byName[p.Name] = p + } + Expect(byName).To(HaveKey("kube-apiserver")) + Expect(byName).To(HaveKey("metrics")) + Expect(byName["kube-apiserver"].NodePort).To(BeZero()) + Expect(byName["metrics"].NodePort).To(BeZero()) + Expect(byName["metrics"].Port).To(Equal(int32(9443))) + }) + + It("defaults to true when unset, preserving an existing NodePort", func() { + // An unset (nil) field means "use the Kubernetes LoadBalancer default" (true). + // The builder writes true explicitly so clearing the field reverts the Service to + // the default without churn. + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = nil + resource := newResource(existingService()) + + Expect(resource.Define(ctx, tcp)).To(Succeed()) + _, err := resource.CreateOrUpdate(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + + svc := &corev1.Service{} + Expect(resource.Client.Get(ctx, client.ObjectKey{Name: tcp.Name, Namespace: tcp.Namespace}, svc)).To(Succeed()) + Expect(svc.Spec.AllocateLoadBalancerNodePorts).NotTo(BeNil()) + Expect(*svc.Spec.AllocateLoadBalancerNodePorts).To(BeTrue()) + Expect(svc.Spec.Ports).To(HaveLen(1)) + Expect(svc.Spec.Ports[0].NodePort).To(Equal(seededNodePort)) + }) + + It("does not churn against a server-defaulted true when field is unset", func() { + // Writing the same default value (true) that the API server already defaulted to + // produces no diff on DeepEqual, preventing perpetual reconcile churn. + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = nil + existing := existingService() + existing.Spec.AllocateLoadBalancerNodePorts = ptr.To(true) // API server default on a live LB Service + resource := newResource(existing) + + Expect(resource.Define(ctx, tcp)).To(Succeed()) + _, err := resource.CreateOrUpdate(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + + svc := &corev1.Service{} + Expect(resource.Client.Get(ctx, client.ObjectKey{Name: tcp.Name, Namespace: tcp.Namespace}, svc)).To(Succeed()) + Expect(svc.Spec.AllocateLoadBalancerNodePorts).NotTo(BeNil()) + Expect(*svc.Spec.AllocateLoadBalancerNodePorts).To(BeTrue()) + }) + + It("reverts a previously-false allocation to the default when field is cleared (unset)", func() { + // Clearing the field (setting to nil in the TCP spec) is the declarative way to + // revert to the Kubernetes LoadBalancer default (true). + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = nil + existing := existingService() + existing.Spec.AllocateLoadBalancerNodePorts = ptr.To(false) // previously disabled + resource := newResource(existing) + + Expect(resource.Define(ctx, tcp)).To(Succeed()) + _, err := resource.CreateOrUpdate(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + + svc := &corev1.Service{} + Expect(resource.Client.Get(ctx, client.ObjectKey{Name: tcp.Name, Namespace: tcp.Namespace}, svc)).To(Succeed()) + Expect(svc.Spec.AllocateLoadBalancerNodePorts).NotTo(BeNil()) + Expect(*svc.Spec.AllocateLoadBalancerNodePorts).To(BeTrue()) + // NodePort re-allocation is not modeled by the fake client; no port assertion here. + }) + + It("propagates an explicit true and leaves the NodePort untouched", func() { + tcp.Spec.ControlPlane.Service.AllocateLoadBalancerNodePorts = ptr.To(true) + resource := newResource(existingService()) + + Expect(resource.Define(ctx, tcp)).To(Succeed()) + _, err := resource.CreateOrUpdate(ctx, tcp) + Expect(err).NotTo(HaveOccurred()) + + svc := &corev1.Service{} + Expect(resource.Client.Get(ctx, client.ObjectKey{Name: tcp.Name, Namespace: tcp.Namespace}, svc)).To(Succeed()) + Expect(svc.Spec.AllocateLoadBalancerNodePorts).NotTo(BeNil()) + Expect(*svc.Spec.AllocateLoadBalancerNodePorts).To(BeTrue()) + Expect(svc.Spec.Ports).To(HaveLen(1)) + Expect(svc.Spec.Ports[0].NodePort).To(Equal(seededNodePort)) + }) +})