From 4531220195180eb11bd6e049bd8442c74e1f210d Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Mon, 11 Mar 2024 22:37:59 -0700 Subject: [PATCH 1/4] Resolve control plane endpoint when updating service Signed-off-by: Lubron Zhan --- pkg/ako-operator/lib.go | 3 ++- pkg/haprovider/haprovider.go | 45 +++++++++++++++++++------------ pkg/haprovider/haprovider_test.go | 34 ++++++++++++++++++++--- 3 files changed, 61 insertions(+), 21 deletions(-) diff --git a/pkg/ako-operator/lib.go b/pkg/ako-operator/lib.go index 06b18498..1cc8bda2 100644 --- a/pkg/ako-operator/lib.go +++ b/pkg/ako-operator/lib.go @@ -97,7 +97,8 @@ func IsLoadBalancerProvider(cluster *clusterv1.Cluster) (bool, error) { return true, nil } -// GetControlPlaneEndpoint returns cluster's API server address +// GetControlPlaneEndpoint returns cluster's API server address, this could be an FQDN, if +// that's the case, need to resolve to IP before putting it as service's address. func GetControlPlaneEndpoint(cluster *clusterv1.Cluster) (string, error) { apiServerEndpoint, _ := cluster.ObjectMeta.Annotations[ClusterControlPlaneAnnotations] if IsClusterClassBasedCluster(cluster) { diff --git a/pkg/haprovider/haprovider.go b/pkg/haprovider/haprovider.go index 387de669..b7c5e31e 100644 --- a/pkg/haprovider/haprovider.go +++ b/pkg/haprovider/haprovider.go @@ -5,10 +5,11 @@ package haprovider import ( "context" - "github.com/vmware-tanzu/load-balancer-operator-for-kubernetes/pkg/utils" "net" "sync" + "github.com/vmware-tanzu/load-balancer-operator-for-kubernetes/pkg/utils" + "github.com/pkg/errors" ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -37,9 +38,11 @@ type HAProvider struct { log logr.Logger } -var instance *HAProvider -var once sync.Once -var QueryFQDN = queryFQDNEndpoint +var ( + instance *HAProvider + once sync.Once + QueryFQDN = queryFQDNEndpoint +) // NewProvider make HAProvider as a singleton func NewProvider(c client.Client, log logr.Logger) *HAProvider { @@ -126,13 +129,14 @@ func (r *HAProvider) createService( }, Spec: corev1.ServiceSpec{ Type: corev1.ServiceTypeLoadBalancer, - //TODO:(chenlin) Add two ip families after AKO fully supports dual-stack load balancer type of service + // TODO:(chenlin) Add two ip families after AKO fully supports dual-stack load balancer type of service IPFamilies: []corev1.IPFamily{corev1.IPFamily(primaryIPFamily)}, - Ports: []corev1.ServicePort{{ - Protocol: "TCP", - Port: port, - TargetPort: intstr.FromInt(int(6443)), - }, + Ports: []corev1.ServicePort{ + { + Protocol: "TCP", + Port: port, + TargetPort: intstr.FromInt(int(6443)), + }, }, }, } @@ -184,7 +188,7 @@ func (r *HAProvider) annotateService(ctx context.Context, cluster *clusterv1.Clu if err != nil { return serviceAnnotation, err } - //no adc is selected for cluster, no annotation is needed. + // no adc is selected for cluster, no annotation is needed. if adcForCluster == nil { // for the management cluster, it needs to requeue until the install-ako-for-management-cluster AKODeploymentConfig created if _, ok := cluster.Labels[akoov1alpha1.TKGManagememtClusterRoleLabel]; ok { @@ -206,7 +210,7 @@ func (r *HAProvider) annotateService(ctx context.Context, cluster *clusterv1.Clu } } if aviInfraSetting != nil { - //add AVIInfraSetting annotation when creating HA svc + // add AVIInfraSetting annotation when creating HA svc serviceAnnotation[akoov1alpha1.HAAVIInfraSettingAnnotationsKey] = aviInfraSetting.Name } return serviceAnnotation, nil @@ -224,7 +228,6 @@ func (r *HAProvider) getADCForCluster(ctx context.Context, cluster *clusterv1.Cl } func (r *HAProvider) getAviInfraSettingFromAdc(ctx context.Context, adcForCluster *akoov1alpha1.AKODeploymentConfig) (*akov1beta1.AviInfraSetting, error) { - aviInfraSetting := &akov1beta1.AviInfraSetting{} aviInfraSettingName := GetAviInfraSettingName(adcForCluster) if err := r.Client.Get(ctx, client.ObjectKey{ @@ -261,11 +264,20 @@ func (r *HAProvider) updateClusterControlPlaneEndpoint(cluster *clusterv1.Cluste } func (r *HAProvider) updateControlPlaneEndpointToService(ctx context.Context, cluster *clusterv1.Cluster, service *corev1.Service) error { - service.Spec.LoadBalancerIP = cluster.Spec.ControlPlaneEndpoint.Host + host := cluster.Spec.ControlPlaneEndpoint.Host + var err error + if net.ParseIP(host) == nil { + host, err = QueryFQDN(host) + if err != nil { + r.log.Error(err, "Failed to resolve control plane endpoint ", "endpoint", host) + return err + } + } + service.Spec.LoadBalancerIP = host if service.Annotations == nil { service.Annotations = make(map[string]string) } - service.Annotations[akoov1alpha1.AkoPreferredIPAnnotation] = cluster.Spec.ControlPlaneEndpoint.Host + service.Annotations[akoov1alpha1.AkoPreferredIPAnnotation] = host if err := r.Update(ctx, service); err != nil { return errors.Wrapf(err, "Failed to update cluster endpoint to cluster control plane load balancer type of service <%s>\n", service.Name) } @@ -354,7 +366,7 @@ func (r *HAProvider) addMachineIpToEndpoints(endpoints *corev1.Endpoints, machin IP: machineAddress.Address, NodeName: &machine.Name, } - //Validate MachineIP before adding to Endpoint + // Validate MachineIP before adding to Endpoint if ipFamily == "V6" { if net.ParseIP(machineAddress.Address).To4() == nil { endpoints.Subsets[0].Addresses = append(endpoints.Subsets[0].Addresses, newAddress) @@ -404,7 +416,6 @@ func (r *HAProvider) CreateOrUpdateHAEndpoints(ctx context.Context, machine *clu ipFamily := "V4" if adcForCluster != nil && adcForCluster.Spec.ExtraConfigs.IpFamily != "" { ipFamily = adcForCluster.Spec.ExtraConfigs.IpFamily - } if !machine.DeletionTimestamp.IsZero() { r.log.Info("machine" + machine.Name + " is being deleted, remove the endpoint of the machine from " + r.getHAServiceName(cluster) + " Endpoints") diff --git a/pkg/haprovider/haprovider_test.go b/pkg/haprovider/haprovider_test.go index 9656f833..93080f8b 100644 --- a/pkg/haprovider/haprovider_test.go +++ b/pkg/haprovider/haprovider_test.go @@ -43,7 +43,7 @@ var _ = Describe("Control Plane HA provider", func() { haProvider = *NewProvider(fc, logger) }) - Context("Test_CreateOrUpdateHAService", func() { + FContext("Test_CreateOrUpdateHAService", func() { var ( cluster *clusterv1.Cluster svc *corev1.Service @@ -134,6 +134,36 @@ var _ = Describe("Control Plane HA provider", func() { Expect(svc.Annotations[akoov1alpha1.AkoPreferredIPAnnotation]).Should(Equal("fd01:3:4:2877:250:56ff:feb4:adaf")) }) }) + + When("ControlPlaneEndpoint.host has FQDN, it should be resolved before adding to service", func() { + BeforeEach(func() { + cluster.Spec.ControlPlaneEndpoint.Host = "google.com" + svc = &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: "default-test-cluster-control-plane", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{}, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{}, + }, + }, + } + key = client.ObjectKey{Name: haProvider.getHAServiceName(cluster), Namespace: cluster.Namespace} + Expect(haProvider.Client.Create(ctx, svc)).ShouldNot(HaveOccurred()) + QueryFQDN = func(fqdn string) (string, error) { + return "3.3.3.3", nil + } + }) + + It("test should pass without error", func() { + Expect(err).ShouldNot(HaveOccurred()) + Expect(haProvider.Client.Get(ctx, key, svc)).ShouldNot(HaveOccurred()) + Expect(svc.Spec.LoadBalancerIP).Should(Equal("3.3.3.3")) + Expect(svc.Annotations[akoov1alpha1.AkoPreferredIPAnnotation]).Should(Equal("3.3.3.3")) + }) + }) }) Describe("Test_CreateService", func() { @@ -503,8 +533,6 @@ var _ = Describe("Control Plane HA provider", func() { Expect(ep.Subsets[0].Addresses[0].IP).Should(Equal("1.1.1.1")) }) }) - }) - }) }) From fb500ecd5898ec2639eca9b80b08b6d63894a384 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Mon, 11 Mar 2024 22:50:57 -0700 Subject: [PATCH 2/4] Fix test Signed-off-by: Lubron Zhan --- pkg/haprovider/haprovider_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/haprovider/haprovider_test.go b/pkg/haprovider/haprovider_test.go index 93080f8b..0e63f25f 100644 --- a/pkg/haprovider/haprovider_test.go +++ b/pkg/haprovider/haprovider_test.go @@ -43,7 +43,7 @@ var _ = Describe("Control Plane HA provider", func() { haProvider = *NewProvider(fc, logger) }) - FContext("Test_CreateOrUpdateHAService", func() { + Context("Test_CreateOrUpdateHAService", func() { var ( cluster *clusterv1.Cluster svc *corev1.Service @@ -146,7 +146,11 @@ var _ = Describe("Control Plane HA provider", func() { Spec: corev1.ServiceSpec{}, Status: corev1.ServiceStatus{ LoadBalancer: corev1.LoadBalancerStatus{ - Ingress: []corev1.LoadBalancerIngress{}, + Ingress: []corev1.LoadBalancerIngress{ + { + IP: "1.1.1.1", + }, + }, }, }, } From c1a851a4444d23329d5ac2a62e1ff463ebf0b523 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Wed, 13 Mar 2024 15:22:58 -0700 Subject: [PATCH 3/4] Add integration test Signed-off-by: Lubron Zhan --- controllers/cluster/cluster_intg_test.go | 17 ++++++++++++++--- pkg/test/util/utils.go | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/controllers/cluster/cluster_intg_test.go b/controllers/cluster/cluster_intg_test.go index c3580425..34c6987a 100644 --- a/controllers/cluster/cluster_intg_test.go +++ b/controllers/cluster/cluster_intg_test.go @@ -23,7 +23,6 @@ import ( ) func intgTestEnsureClusterHAProvider() { - Context("EnsureHAService", func() { var ( ctx *builder.IntegrationTestContext @@ -104,7 +103,6 @@ func intgTestEnsureClusterHAProvider() { Name: serviceName, Namespace: ctx.Namespace, }, &corev1.Endpoints{}, testutil.EXIST) - }) }) @@ -143,9 +141,22 @@ func intgTestEnsureClusterHAProvider() { err := ctx.Client.Get(ctx, client.ObjectKey{Name: serviceName, Namespace: ctx.Namespace}, service) Expect(err).ShouldNot(HaveOccurred()) Expect(service.Annotations[akoov1alpha1.AkoPreferredIPAnnotation]).Should(Equal("10.1.2.1")) + // make sure the service has ip as ingress, not fqdn + Consistently(func() bool { + err := ctx.Client.Get(ctx, client.ObjectKey{Name: serviceName, Namespace: ctx.Namespace}, service) + if err != nil { + return false + } + if len(service.Status.LoadBalancer.Ingress) < 0 { + return false + } + if service.Status.LoadBalancer.Ingress[0].IP != "10.1.2.1" { + return false + } + return true + }, "30s").Should(BeTrue()) }) }) }) - }) } diff --git a/pkg/test/util/utils.go b/pkg/test/util/utils.go index 4fa32567..3ed0105c 100644 --- a/pkg/test/util/utils.go +++ b/pkg/test/util/utils.go @@ -128,7 +128,7 @@ func EnsureClusterAviLabelMatchExpectation(ctx *builder.IntegrationTestContext, func UpdateObjectLabels(ctx *builder.IntegrationTestContext, key client.ObjectKey, labels map[string]string) { Eventually(func() error { - var cluster = new(clusterv1.Cluster) + cluster := new(clusterv1.Cluster) if err := ctx.Client.Get(ctx, client.ObjectKey{ Name: key.Name, From 9be9129ee32250274387528db46cb2a027601930 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Wed, 13 Mar 2024 16:19:10 -0700 Subject: [PATCH 4/4] Fix test Signed-off-by: Lubron Zhan --- controllers/cluster/cluster_intg_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/controllers/cluster/cluster_intg_test.go b/controllers/cluster/cluster_intg_test.go index 34c6987a..8c83fd19 100644 --- a/controllers/cluster/cluster_intg_test.go +++ b/controllers/cluster/cluster_intg_test.go @@ -141,15 +141,18 @@ func intgTestEnsureClusterHAProvider() { err := ctx.Client.Get(ctx, client.ObjectKey{Name: serviceName, Namespace: ctx.Namespace}, service) Expect(err).ShouldNot(HaveOccurred()) Expect(service.Annotations[akoov1alpha1.AkoPreferredIPAnnotation]).Should(Equal("10.1.2.1")) - // make sure the service has ip as ingress, not fqdn + // Simulate AKO updates the ip for service. + service.Status.LoadBalancer.Ingress = []corev1.LoadBalancerIngress{{ + IP: "10.1.2.1", + }} + err = ctx.Client.Status().Update(ctx, service) + Expect(err).To(BeNil()) + // Ensure updateControlPlaneEndpointToService won't set fqdn as ingress.ip Consistently(func() bool { err := ctx.Client.Get(ctx, client.ObjectKey{Name: serviceName, Namespace: ctx.Namespace}, service) if err != nil { return false } - if len(service.Status.LoadBalancer.Ingress) < 0 { - return false - } if service.Status.LoadBalancer.Ingress[0].IP != "10.1.2.1" { return false }